[Mesa-dev] [PATCH 4/5] egl/wayland: Allow client->server format conversion for PRIME offload.
Mike Lothian
mike at fireburn.co.uk
Wed May 23 07:20:38 UTC 2018
Hi Mario
Can you explain how I can test this? I'll test it out on an Intel+AMD system
Cheers
Mike
On Wed, 23 May 2018 at 05:51 Mario Kleiner <mario.kleiner.de at gmail.com>
wrote:
> On Mon, May 21, 2018 at 4:42 PM, Eric Engestrom
> <eric.engestrom at intel.com> wrote:
> > On Saturday, 2018-05-19 05:32:41 +0200, Mario Kleiner wrote:
> >> Support PRIME render offload between a Wayland server gpu and a Wayland
> >> client gpu with different channel ordering for their color formats,
> >> e.g., between Intel drivers which currently only support ARGB2101010
> >> and XRGB2101010 import/display and nouveau which only supports
> ABGR2101010
> >> rendering and display on nv-50 and later.
> >>
> >> In the wl_visuals table, we also store for each format an alternate
> >> sibling format which stores colors at the same precision, but with
> >> different channel ordering, e.g., ARGB2101010 <-> ABGR2101010.
> >>
> >> If a given client-gpu renderable format is not supported by the server
> >> for import, but the alternate format is supported by the server, expose
> >> the client-gpu renderable format as a valid EGLConfig to the client. At
> >> eglSwapBuffers time, during the blitImage() detiling blit from the
> client
> >> backbuffer to the linear buffer, the client format is converted to the
> >> server supported format. As we have to do a copy for PRIME anyway,
> >> this channel swizzling conversion comes essentially for free.
> >>
> >> Note that even if a server gpu in principle does support sampling
> >> from the clients native format, this conversion will be a performance
> >> advantage if it allows to convert to the servers preferred format
> >> for direct scanout, as the Wayland compositor may then be able to
> >> directly page-flip a fullscreen client wl_buffer onto the primary
> >> plane, or onto a hardware overlay plane, avoiding an extra data copy
> >> for desktop composition.
> >>
> >> Tested so far under Weston with: nouveau single-gpu, Intel single-gpu,
> >> AMD single-gpu, "Optimus" Intel server iGPU for display + NVidia
> >> client dGPU for rendering.
> >>
> >> Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
> >> Cc: Daniel Stone <daniels at collabora.com>
> >> ---
> >> src/egl/drivers/dri2/platform_wayland.c | 67
> ++++++++++++++++++++++++++++-----
> >> 1 file changed, 58 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/src/egl/drivers/dri2/platform_wayland.c
> b/src/egl/drivers/dri2/platform_wayland.c
> >> index 89a7f90118..fb364a6233 100644
> >> --- a/src/egl/drivers/dri2/platform_wayland.c
> >> +++ b/src/egl/drivers/dri2/platform_wayland.c
> >> @@ -68,49 +68,50 @@ static const struct dri2_wl_visual {
> >> uint32_t wl_drm_format;
> >> uint32_t wl_shm_format;
> >> int dri_image_format;
> >> + int alt_dri_image_format;
> >
>
> Thanks for the review.
>
> > A comment here wouldn't hurt, to explain what this "alt" is to someone
> > who sees the code after it landed :)
> >
>
> Will do, although i usually feel a bit guilty that my patches usually
> suffer from over-commenting and essay length commit messages :)
>
> >> int bpp;
> >> unsigned int rgba_masks[4];
> >> } dri2_wl_visuals[] = {
> >> {
> >> "XRGB2101010",
> >> WL_DRM_FORMAT_XRGB2101010, WL_SHM_FORMAT_XRGB2101010,
> >> - __DRI_IMAGE_FORMAT_XRGB2101010, 32,
> >> + __DRI_IMAGE_FORMAT_XRGB2101010, __DRI_IMAGE_FORMAT_XBGR2101010,
> 32,
> >> { 0x3ff00000, 0x000ffc00, 0x000003ff, 0x00000000 }
> >> },
> >> {
> >> "ARGB2101010",
> >> WL_DRM_FORMAT_ARGB2101010, WL_SHM_FORMAT_ARGB2101010,
> >> - __DRI_IMAGE_FORMAT_ARGB2101010, 32,
> >> + __DRI_IMAGE_FORMAT_ARGB2101010, __DRI_IMAGE_FORMAT_ABGR2101010,
> 32,
> >> { 0x3ff00000, 0x000ffc00, 0x000003ff, 0xc0000000 }
> >> },
> >> {
> >> "XBGR2101010",
> >> WL_DRM_FORMAT_XBGR2101010, WL_SHM_FORMAT_XBGR2101010,
> >> - __DRI_IMAGE_FORMAT_XBGR2101010, 32,
> >> + __DRI_IMAGE_FORMAT_XBGR2101010, __DRI_IMAGE_FORMAT_XRGB2101010,
> 32,
> >> { 0x000003ff, 0x000ffc00, 0x3ff00000, 0x00000000 }
> >> },
> >> {
> >> "ABGR2101010",
> >> WL_DRM_FORMAT_ABGR2101010, WL_SHM_FORMAT_ABGR2101010,
> >> - __DRI_IMAGE_FORMAT_ABGR2101010, 32,
> >> + __DRI_IMAGE_FORMAT_ABGR2101010, __DRI_IMAGE_FORMAT_ARGB2101010,
> 32,
> >> { 0x000003ff, 0x000ffc00, 0x3ff00000, 0xc0000000 }
> >> },
> >> {
> >> "XRGB8888",
> >> WL_DRM_FORMAT_XRGB8888, WL_SHM_FORMAT_XRGB8888,
> >> - __DRI_IMAGE_FORMAT_XRGB8888, 32,
> >> + __DRI_IMAGE_FORMAT_XRGB8888, __DRI_IMAGE_FORMAT_NONE, 32,
> >> { 0x00ff0000, 0x0000ff00, 0x000000ff, 0x00000000 }
> >> },
> >> {
> >> "ARGB8888",
> >> WL_DRM_FORMAT_ARGB8888, WL_SHM_FORMAT_ARGB8888,
> >> - __DRI_IMAGE_FORMAT_ARGB8888, 32,
> >> + __DRI_IMAGE_FORMAT_ARGB8888, __DRI_IMAGE_FORMAT_NONE, 32,
> >> { 0x00ff0000, 0x0000ff00, 0x000000ff, 0xff000000 }
> >> },
> >> {
> >> "RGB565",
> >> WL_DRM_FORMAT_RGB565, WL_SHM_FORMAT_RGB565,
> >> - __DRI_IMAGE_FORMAT_RGB565, 16,
> >> + __DRI_IMAGE_FORMAT_RGB565, __DRI_IMAGE_FORMAT_NONE, 16,
> >> { 0xf800, 0x07e0, 0x001f, 0x0000 }
> >> },
> >> };
> >> @@ -450,15 +451,23 @@ get_back_bo(struct dri2_egl_surface *dri2_surf)
> >> int use_flags;
> >> int visual_idx;
> >> unsigned int dri_image_format;
> >> + unsigned int linear_dri_image_format;
> >> uint64_t *modifiers;
> >> int num_modifiers;
> >>
> >> visual_idx = dri2_wl_visual_idx_from_fourcc(dri2_surf->format);
> >> assert(visual_idx != -1);
> >> dri_image_format = dri2_wl_visuals[visual_idx].dri_image_format;
> >> + linear_dri_image_format = dri_image_format;
> >> modifiers = u_vector_tail(&dri2_dpy->wl_modifiers[visual_idx]);
> >> num_modifiers =
> u_vector_length(&dri2_dpy->wl_modifiers[visual_idx]);
> >>
> >> + /* Substitute dri image format if server does not support original
> format */
> >> + if (!(dri2_dpy->formats & (1 << visual_idx)))
> >> + linear_dri_image_format =
> dri2_wl_visuals[visual_idx].alt_dri_image_format;
> >
> > Could we test that the substitution would actually work better before
> > doing it?
> >
>
> We test that, but already in dri2_wl_add_configs_for_visuals() below.
> The only way to get to the if statement (ie. visual_idx != -1) and
> then have it trigger the linear_dri_image_format substitution is if
> the new hunk of code in dri2_wl_add_configs_for_visuals() found out
> that the substitution is necessary, possible and valid. A EGLconfig
> can support the client format with the given visual_idx if either the
> wayland-server supports it (so the above if statement will skip the
> substitution), or if the server doesn't support it, but does support
> sampling/displaying the substituted format, according to the checks
> done in dri2_wl_add_configs_for_visuals before adding the EGLconfig to
> the list of client configs.
>
> I could add a test to document this, which would be redundant though.
> Or maybe a code comment? Or maybe the test in an assert() to document
> that that test should always succeed in the absence of bugs in the
> code?
>
> >> +
> >> + assert(linear_dri_image_format != __DRI_IMAGE_FORMAT_NONE);
> >> +
> >> /* There might be a buffer release already queued that wasn't
> processed */
> >> wl_display_dispatch_queue_pending(dri2_dpy->wl_dpy,
> dri2_surf->wl_queue);
> >>
> >> @@ -505,7 +514,7 @@ get_back_bo(struct dri2_egl_surface *dri2_surf)
> >>
> dri2_dpy->image->createImageWithModifiers(dri2_dpy->dri_screen,
> >>
> dri2_surf->base.Width,
> >>
> dri2_surf->base.Height,
> >> - dri_image_format,
> >> +
> linear_dri_image_format,
> >> &linear_mod,
> >> 1,
> >> NULL);
> >> @@ -514,7 +523,7 @@ get_back_bo(struct dri2_egl_surface *dri2_surf)
> >> dri2_dpy->image->createImage(dri2_dpy->dri_screen,
> >> dri2_surf->base.Width,
> >> dri2_surf->base.Height,
> >> - dri_image_format,
> >> + linear_dri_image_format,
> >> use_flags |
> >> __DRI_IMAGE_USE_LINEAR,
> >> NULL);
> >> @@ -1278,8 +1287,11 @@ dri2_wl_add_configs_for_visuals(_EGLDriver *drv,
> _EGLDisplay *disp)
> >> struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> >> unsigned int format_count[ARRAY_SIZE(dri2_wl_visuals)] = { 0 };
> >> unsigned int count = 0;
> >> + bool assigned;
> >>
> >> for (unsigned i = 0; dri2_dpy->driver_configs[i]; i++) {
> >> + assigned = false;
> >> +
> >> for (unsigned j = 0; j < ARRAY_SIZE(dri2_wl_visuals); j++) {
> >> struct dri2_egl_config *dri2_conf;
> >>
> >> @@ -1292,6 +1304,43 @@ dri2_wl_add_configs_for_visuals(_EGLDriver *drv,
> _EGLDisplay *disp)
> >> if (dri2_conf->base.ConfigID == count + 1)
> >> count++;
> >> format_count[j]++;
> >> + assigned = true;
> >> + }
> >> + }
> >> +
> >> + if (!assigned && dri2_dpy->is_different_gpu) {
> >> + struct dri2_egl_config *dri2_conf;
> >> + int alt_dri_image_format, j, k;
> >
> > Let's avoid reusing the same names as the loop variables above?
>
> Okay, c for client visual index and s for substitute visual index are
> still free as short names.
>
> thanks,
> -mario
>
> >
> >> +
> >> + /* No match for config. Try if we can blitImage convert to a
> visual */
> >> + j = dri2_wl_visual_idx_from_config(dri2_dpy,
> >> +
> dri2_dpy->driver_configs[i]);
> >> +
> >> + if (j == -1)
> >> + continue;
> >> +
> >> + /* Find optimal target visual for blitImage conversion, if
> any. */
> >> + alt_dri_image_format =
> dri2_wl_visuals[j].alt_dri_image_format;
> >> + k =
> dri2_wl_visual_idx_from_dri_image_format(alt_dri_image_format);
> >> +
> >> + if (k == -1 || !(dri2_dpy->formats & (1 << k)))
> >> + continue;
> >> +
> >> + /* Visual k works for the Wayland server, and j can be
> converted into k
> >> + * by our client gpu during PRIME blitImage conversion to a
> linear
> >> + * wl_buffer, so add visual j as supported by the client
> renderer.
> >> + */
> >> + dri2_conf = dri2_add_config(disp, dri2_dpy->driver_configs[i],
> >> + count + 1, EGL_WINDOW_BIT, NULL,
> >> + dri2_wl_visuals[j].rgba_masks);
> >> + if (dri2_conf) {
> >> + if (dri2_conf->base.ConfigID == count + 1)
> >> + count++;
> >> + format_count[j]++;
> >> + if (format_count[j] == 1)
> >> + _eglLog(_EGL_DEBUG, "Client format %s to server format
> %s via "
> >> + "PRIME blitImage.",
> dri2_wl_visuals[j].format_name,
> >> + dri2_wl_visuals[k].format_name);
> >> }
> >> }
> >> }
> >> --
> >> 2.13.0.rc1.294.g07d810a77f
> >>
> >> _______________________________________________
> >> mesa-dev mailing list
> >> mesa-dev at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180523/ed4157cc/attachment-0001.html>
More information about the mesa-dev
mailing list