<div dir="ltr">Hi Mario<div> </div><div>Can you explain how I can test this? I'll test it out on an Intel+AMD system</div><div> </div><div>Cheers</div><div> </div><div>Mike</div></div> <div class="gmail_quote"><div dir="ltr">On Wed, 23 May 2018 at 05:51 Mario Kleiner <<a href="mailto:mario.kleiner.de@gmail.com">mario.kleiner.de@gmail.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, May 21, 2018 at 4:42 PM, Eric Engestrom <<a href="mailto:eric.engestrom@intel.com" target="_blank">eric.engestrom@intel.com</a>> 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 <<a href="mailto:mario.kleiner.de@gmail.com" target="_blank">mario.kleiner.de@gmail.com</a>> >> Cc: Daniel Stone <<a href="mailto:daniels@collabora.com" target="_blank">daniels@collabora.com</a>> >> --- >> 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 >> <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> >> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div>