New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linux-dmabuf with n_planes > 1 #724

Merged
merged 2 commits into from Mar 23, 2018

Conversation

Projects
None yet
4 participants
@agx
Contributor

agx commented Mar 15, 2018

This aims to add support for formats with more than one plane like NV12.

Test plan

  • ./weston-simple-dmabuf-drm --import-format=NV12 (new functionality)
  • ./weston-simple-dmabuf-drm --import-format=XRGB (make sure old code still works)

The drivers I have don't support NV12 but maybe someone's does. Therefore marking as WiP since I can't really test it atm but I'll see if can come up with s.th. else that has more than one plane.

According to https://cgit.freedesktop.org/wayland/weston/commit/?id=ee58911912d88caacf340a5cb6a9e25fcba24996 freedreno supports NV12 if somebody running rootston on freedreno could test it that would be great.

@@ -343,6 +337,52 @@ EGLImage wlr_egl_create_image_from_dmabuf(struct wlr_egl *egl,
attribs[atti++] = EGL_DMA_BUF_PLANE0_MODIFIER_HI_EXT;
attribs[atti++] = attributes->modifier[0] >> 32;
}
if (attributes->n_planes > 0) {

This comment has been minimized.

@ddevault

ddevault Mar 15, 2018

Member

Would prefer to do this in some kind of loop rather than as 3 if-statements with copypasta'd bodies

This comment has been minimized.

@agx

agx Mar 15, 2018

Contributor

I thought the same but since the constants vary (they have the plane number included) I went that way. Otherwise we'd have to setup all 5 constants upfront with 5 arrays:

pitch_ex_consts[] = [ EGL_DMABUF_PLANE0_PITCH_EX, EGL_DMABUF_PLANE1_EX, ...]

(same for fd, offset, modifier_lo, modifier_hi) and loop over these too or use some macro magic so I went for the open coded version.

This comment has been minimized.

@ddevault

ddevault Mar 15, 2018

Member

I'd rather set up the array like you suppose.

@agx agx changed the title from [WiP] Linux-dmabuf with n_planes > 1 to [NeedsTest] Linux-dmabuf with n_planes > 1 Mar 15, 2018

@agx agx referenced this pull request Mar 15, 2018

Open

YUV support #54

@agx

This comment has been minimized.

Contributor

agx commented Mar 16, 2018

It turns out egl on my intel advertises NV12 (#726) but not the buffer modifier used by weston-simple-dmabuf-drm (MOD_SAMSUNG_64_32_TILE). If I use another buffer modifier I get something rendered so it doesn't look too bad.

@emersion

This comment has been minimized.

Member

emersion commented Mar 16, 2018

Nice! Can you provide a weston-simple-dmabuf-drm patch so we can try it?

@agx

This comment has been minimized.

Contributor

agx commented Mar 16, 2018

It really only disables the modifier atm:

agx/weston@4f788cf

so it displays only garbage but we can see that the multiple planes get added at least.

@agx

This comment has been minimized.

Contributor

agx commented Mar 16, 2018

@emersion I have a more complete NV12 test client now

https://github.com/agx/weston/tree/nv12

it renders correctly on weston but rootston seems to pick up the first plane as RGB, I'll have a look

@agx

This comment has been minimized.

Contributor

agx commented Mar 19, 2018

This renders correctly on my intel card. Besides @SirCmpwn comment I need to cleanup the last patch that handles GL_TEXTURE_EXTERNAL_OES.

@@ -365,6 +359,52 @@ EGLImage wlr_egl_create_image_from_dmabuf(struct wlr_egl *egl,
attribs[atti++] = EGL_DMA_BUF_PLANE0_MODIFIER_HI_EXT;
attribs[atti++] = attributes->modifier[0] >> 32;
}
if (attributes->n_planes > 0) {

This comment has been minimized.

@ascent12

ascent12 Mar 19, 2018

Collaborator

You're adding plane 0 to the array twice.

@agx

This comment has been minimized.

Contributor

agx commented Mar 20, 2018

@agx agx referenced this pull request Mar 20, 2018

Merged

Gles2 tex target #736

@agx agx changed the title from [NeedsTest] Linux-dmabuf with n_planes > 1 to Linux-dmabuf with n_planes > 1 Mar 20, 2018

@agx

This comment has been minimized.

Contributor

agx commented Mar 20, 2018

Depends on #736 which should go in first.

}
}
attribs[atti++] = EGL_NONE;

This comment has been minimized.

@emersion

emersion Mar 21, 2018

Member

Maybe adding an assertion here would be safer:

assert(atti < sizeof(attribs)/sizeof(attribs[0]));

This comment has been minimized.

@agx

agx Mar 21, 2018

Contributor

Since the size of attribs is fixed we're safe but in case we change the number of planes we might forget to increase it so I added the assert.

@emersion

This comment has been minimized.

Member

emersion commented Mar 21, 2018

Is it expected that XRGB and NV12 don't render the same image?

@agx

This comment has been minimized.

Contributor

agx commented Mar 21, 2018

@emersion where do we render a different image?

@emersion

This comment has been minimized.

Member

emersion commented Mar 21, 2018

I mean, weston-simple-dmabuf-drm --import-format=NV12 and weston-simple-dmabuf-drm --import-format=XRGB don't show the same image.

@agx

This comment has been minimized.

Contributor

agx commented Mar 21, 2018

@SirCmpwn is using a loop for the attributes a must have? I do like the open coded version better since it makes it obvious what's happening. Once we support > 4 planes I would agree to not open code it.

@agx

This comment has been minimized.

Contributor

agx commented Mar 21, 2018

@emersion ahh...thought so. Yes, I didn't bother to reproduce it exactly for the time being, it will make it simpler to find out which method is actually used.

@ascent12

This comment has been minimized.

Collaborator

ascent12 commented Mar 21, 2018

I don't believe any format exists that has more than 4 planes. In fact, I don't know of any formats with more than 3, although I guess the 4th plane could be for an alpha channel or something.

I'd much prefer a loop as well. Unrolling is fine for maybe a few lines of code, but what's going on there is too much.
It wouldn't be hard to do it with an unnamed struct and an array.

@agx

This comment has been minimized.

Contributor

agx commented Mar 21, 2018

@ascent12 I don't think we'll have more than 4 planes anytime soon either (in fact NV12 only uses two and for YUV we have more work ahead).

I'll use a loop then.

@agx

This comment has been minimized.

Contributor

agx commented Mar 21, 2018

Removed the unrolled loop now

EGL_DMA_BUF_PLANE ## N ## _PITCH_EXT, \
EGL_DMA_BUF_PLANE ## N ## _MODIFIER_LO_EXT, \
EGL_DMA_BUF_PLANE ## N ## _MODIFIER_HI_EXT \
}

This comment has been minimized.

@ddevault

ddevault Mar 21, 2018

Member

We don't use macros, find another way

@ddevault

This comment has been minimized.

Member

ddevault commented Mar 21, 2018

LGTM aside from the macro

@agx

This comment has been minimized.

Contributor

agx commented Mar 21, 2018

@emersion

This comment has been minimized.

Member

emersion commented Mar 21, 2018

Droped the macro in favour or spelled out constants.

It seems the macro is still here.

@agx

This comment has been minimized.

Contributor

agx commented Mar 21, 2018

@agx

This comment has been minimized.

Contributor

agx commented Mar 23, 2018

Rebased against recent gles2 changes.

@emersion

LGTM

@ddevault ddevault merged commit 7cc042f into swaywm:master Mar 23, 2018

1 check passed

builds.sr.ht builds.sr.ht job completed successfully
Details
@agx

This comment has been minimized.

Contributor

agx commented Mar 23, 2018

@ddevault

This comment has been minimized.

Member

ddevault commented Mar 23, 2018

Thanks for the hard work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment