[RFC xserver v5 13/14] glamor: Use gbm_bo_create_with_modifiers for internal pixmap allocation

[Jason Ekstrand]

On Mon, Nov 6, 2017 at 1:30 PM, Louis-Francis Ratté-Boulianne <
lfrb at collabora.com> wrote:

> Using modifier might allow the driver to use a more optimal format
> (e.g. tiled/compressed). Let's try to use those if possible.
>
> Signed-off-by: Louis-Francis Ratté-Boulianne <lfrb at collabora.com>
> ---
>  glamor/glamor_egl.c           | 44 ++++++++++++++++++++++++++++++
> ++++++++-----
>  hw/xwayland/xwayland-glamor.c | 22 +++++++++++++++++++---
>  2 files changed, 58 insertions(+), 8 deletions(-)
>
> diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
> index bcf36cf9b..fdda56c04 100644
> --- a/glamor/glamor_egl.c
> +++ b/glamor/glamor_egl.c
> @@ -270,13 +270,47 @@ glamor_make_pixmap_exportable(PixmapPtr pixmap)
>          return FALSE;
>      }
>
> -    bo = gbm_bo_create(glamor_egl->gbm, width, height,
> -                       GBM_FORMAT_ARGB8888,
> +#ifdef GBM_BO_WITH_MODIFIERS
> +    if (glamor_egl->dmabuf_capable) {
> +        uint32_t num_modifiers;
> +        uint64_t *modifiers = NULL;
> +        int i, j;
> +
> +        glamor_get_modifiers(screen, DRM_FORMAT_ARGB8888,
> +                             &num_modifiers, &modifiers);
> +
> +        /* Filter out multi-plane modifiers */
> +        for (i = num_modifiers - 1; i >= 0; i--) {
> +            if (gbm_device_get_format_modifier_plane_count(glamor_
> egl->gbm,
> +
>  DRM_FORMAT_ARGB8888,
> +                                                           modifiers[i])
> == 1)
> +                continue;
> +            for (j = i + 1; j < num_modifiers; j++)
> +                modifiers[j - 1] = modifiers[j];
> +            num_modifiers--;
> +        }
>

I was browsing through your branch today and was confused by this.  Why are
we filtering out multi-planar formats in glamor?  The only time we need to
filter out multi-planr formats is when we will be doing front-buffer
rendering on a surface that is actively being scanned out.  It should be
safe to front-buffer render to multi-planar images that are shared with a
compositor because it's unlikely that the 3D engine will race with itself.
This is causing X to disable compression internally for the temporary
pixmaps that get handed off to the compositor which is not what we want.
If you did this to fix corruption you may have seen when using a
compositor, that would be because we have a nasty texture_from_pixmap bug
in mesa related to CCS that I'm currently in the process of fixing.
Patches should be incoming before the end of the week.

There are some theoretical ways this could get messed up if the kernel is
preempting too aggressively, but I think it will be ok.  If we do see a
problem, then I think we can do something in mesa where we disable
preemption whenever texturing from a pixmap or something like that.


> +        if (num_modifiers == 0 && modifiers) {
> +            free(modifiers);
> +            modifiers = NULL;
> +        }
> +
> +        bo = gbm_bo_create_with_modifiers(glamor_egl->gbm, width, height,
> +                                          GBM_FORMAT_ARGB8888,
> +                                          modifiers, num_modifiers);
> +        free(modifiers);
> +    }
> +    else
> +#endif
> +    {
> +        bo = gbm_bo_create(glamor_egl->gbm, width, height,
> +                GBM_FORMAT_ARGB8888,
>  #ifdef GLAMOR_HAS_GBM_LINEAR
> -                       (pixmap->usage_hint == CREATE_PIXMAP_USAGE_SHARED ?
> -                        GBM_BO_USE_LINEAR : 0) |
> +                (pixmap->usage_hint == CREATE_PIXMAP_USAGE_SHARED ?
> +                 GBM_BO_USE_LINEAR : 0) |
>  #endif
> -                       GBM_BO_USE_RENDERING | GBM_BO_USE_SCANOUT);
> +                GBM_BO_USE_RENDERING | GBM_BO_USE_SCANOUT);
> +    }
> +
>      if (!bo) {
>          xf86DrvMsg(scrn->scrnIndex, X_ERROR,
>                     "Failed to make %dx%dx%dbpp GBM bo\n",
> diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
> index e3a7f6b5b..40772fb9a 100644
> --- a/hw/xwayland/xwayland-glamor.c
> +++ b/hw/xwayland/xwayland-glamor.c
> @@ -226,14 +226,30 @@ xwl_glamor_create_pixmap(ScreenPtr screen,
>  {
>      struct xwl_screen *xwl_screen = xwl_screen_get(screen);
>      struct gbm_bo *bo;
> +    uint32_t format;
>
>      if (width > 0 && height > 0 && depth >= 15 &&
>          (hint == 0 ||
>           hint == CREATE_PIXMAP_USAGE_BACKING_PIXMAP ||
>           hint == CREATE_PIXMAP_USAGE_SHARED)) {
> -        bo = gbm_bo_create(xwl_screen->gbm, width, height,
> -                           gbm_format_for_depth(depth),
> -                           GBM_BO_USE_SCANOUT | GBM_BO_USE_RENDERING);
> +        format = gbm_format_for_depth(depth);
> +
> +#ifdef GBM_BO_WITH_MODIFIERS
> +        if (xwl_screen->dmabuf_capable) {
> +            uint32_t num_modifiers;
> +            uint64_t *modifiers = NULL;
> +
> +            glamor_get_modifiers(screen, format, &num_modifiers,
> &modifiers);
> +            bo = gbm_bo_create_with_modifiers(xwl_screen->gbm, width,
> height,
> +                                              format, modifiers,
> num_modifiers);
> +            free(modifiers);
> +        }
> +        else
> +#endif
> +        {
> +            bo = gbm_bo_create(xwl_screen->gbm, width, height, format,
> +                               GBM_BO_USE_SCANOUT | GBM_BO_USE_RENDERING);
> +        }
>
>          if (bo)
>              return xwl_glamor_create_pixmap_for_bo(screen, bo, depth);
> --
> 2.13.0
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.x.org/archives/xorg-devel/attachments/20171107/038a4fe1/attachment.html>