[Mesa-dev] [PATCH 2/3] gbm: Introduce modifiers into surface/bo creation

[Emil Velikov]

On 10 March 2017 at 01:48, Ben Widawsky <ben at bwidawsk.net> wrote:
> The idea behind modifiers like this is that the user of GBM will have
> some mechanism to query what properties the hardware supports for its BO
> or surface. This information is directly passed in (and stored) so that
> the DRI implementation can create an image with the appropriate
> attributes.
>
> A getter() will be added later so that the user GBM will be able to
> query what modifier should be used.
>
> Only in surface creation, the modifiers are stored until the BO is
> actually allocated. In regular buffer allocation, the correct modifier
> can (will be, in future patches be chosen at creation time.
>
> v2: Make sure to check if count is non-zero in addition to testing if
> calloc fails. (Daniel)
>
> v3: Remove "usage" and "flags" from modifier creation. Requested by
> Kristian.
>
> v4: Take advantage of the "INVALID" modifier added by the GET_PLANE2
> series.
>
> v5: Don't bother with storing modifiers for gbm_bo_create because that's
> a synchronous operation and we can actually select the correct modifier
> at create time (done in a later patch) (Jason)
>
> Cc: Kristian Høgsberg <krh at bitplanet.net>
> Cc: Jason Ekstrand <jason at jlekstrand.net>
> References (v4): https://lists.freedesktop.org/archives/intel-gfx/2017-January/116636.html
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com> (v1)
> Acked-by: Daniel Stone <daniels at collabora.com>

> ---
>  src/egl/drivers/dri2/platform_drm.c      | 19 ++++++++++++---
Worth splitting the patches - patch N+1

>  src/gbm/backends/dri/gbm_dri.c           | 42 ++++++++++++++++++++++++++------
>  src/gbm/gbm-symbols-check                |  2 ++
>  src/gbm/main/gbm.c                       | 29 ++++++++++++++++++++--
>  src/gbm/main/gbm.h                       | 12 +++++++++
>  src/gbm/main/gbmint.h                    | 12 +++++++--
Patch N

>  src/mesa/drivers/dri/i965/intel_screen.c | 18 ++++++++++++++
Patch N+Y

> --- a/src/gbm/backends/dri/gbm_dri.c
> +++ b/src/gbm/backends/dri/gbm_dri.c
> @@ -1023,13 +1023,20 @@ free_bo:
>  static struct gbm_bo *
>  gbm_dri_bo_create(struct gbm_device *gbm,
>                    uint32_t width, uint32_t height,
> -                  uint32_t format, uint32_t usage)
> +                  uint32_t format, uint32_t usage,
> +                  const uint64_t *modifiers,
> +                  const unsigned int count)
>  {
>     struct gbm_dri_device *dri = gbm_dri_device(gbm);
>     struct gbm_dri_bo *bo;
>     int dri_format;
>     unsigned dri_use = 0;
>
> +   /* Callers of this may specify a modifier, or a dri usage, but not both. The
> +    * newer modifier interface deprecates the older usage flags.
> +    */
> +   assert(!(usage && count));
> +
Similar to last patch - XOR ?

>     if (usage & GBM_BO_USE_WRITE || dri->image == NULL)
>        return create_dumb(gbm, width, height, format, usage);
>
> @@ -1087,11 +1094,21 @@ gbm_dri_bo_create(struct gbm_device *gbm,
>     /* Gallium drivers requires shared in order to get the handle/stride */
>     dri_use |= __DRI_IMAGE_USE_SHARE;
>
> -   bo->image =
> -      dri->image->createImage(dri->screen,
> -                              width, height,
> -                              dri_format, dri_use,
> -                              bo);
> +   if (!dri->image || dri->image->base.version < 14 ||
> +       !dri->image->createImageWithModifiers) {
> +      if (modifiers)
> +         fprintf(stderr, "Modifiers specified, but DRI is too old\n");
> +
> +      bo->image = dri->image->createImage(dri->screen, width, height,
> +                                          dri_format, dri_use, bo);
With the "modifiers or use" above how do we get sane value for dri_use
in this fall-back ?
Seems like things we'll miss/ignore the scanout/curson/linear flags.

Perhaps it's worth simply bailing out ?


> --- a/src/gbm/gbm-symbols-check
> +++ b/src/gbm/gbm-symbols-check

> +gbm_bo_create_with_modifiers

> +gbm_surface_create_with_modifiers
Thank you :-)


> +GBM_EXPORT struct gbm_bo *
> +gbm_bo_create_with_modifiers(struct gbm_device *gbm,
> +                             uint32_t width, uint32_t height,
> +                             uint32_t format,
> +                             const uint64_t *modifiers,
> +                             const unsigned int count)
> +{
> +   if (width == 0 || height == 0) {
> +      errno = EINVAL;
> +      return NULL;
> +   }
> +
Should we validate modifiers and/or count at this level as well ?


> +GBM_EXPORT struct gbm_surface *
> +gbm_surface_create_with_modifiers(struct gbm_device *gbm,
> +                                  uint32_t width, uint32_t height,
> +                                  uint32_t format,
> +                                  const uint64_t *modifiers,
> +                                  const unsigned int count)
> +{
Ditto ? What happens if we feed zero width/height ?
Worth adding a note, even if things are expected to blow up ?


> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c

> +static uint64_t
> +select_best_modifier(struct gen_device_info *devinfo,
> +                     const uint64_t *modifiers,
> +                     const unsigned count)
> +{
> +   return DRM_FORMAT_MOD_INVALID;
Worth adding a note why any combination of modifiers is ignored ?

-Emil