[Mesa-dev] [PATCH 3/3] gbm: Export a get modifiers

[Emil Velikov]

Hi Ben,

Just a minor nitpick below.

On 10 March 2017 at 01:49, Ben Widawsky <ben at bwidawsk.net> wrote:
> This patch originally had i965 specific code and was named:
> commit 61cd3c52b868cf8cb90b06e53a382a921eb42754
> Author: Ben Widawsky <ben at bwidawsk.net>
> Date:   Thu Oct 20 18:21:24 2016 -0700
>
>     gbm: Get modifiers from DRI
>
> To accomplish this, two new query tokens are added to the extension:
> __DRI_IMAGE_ATTRIB_MODIFIER_UPPER
> __DRI_IMAGE_ATTRIB_MODIFIER_LOWER
>
> The query extension only supported 32b queries, and modifiers are 64b,
> so we needed two of them.
>
> NOTE: The extension version is still set to 13, so none of this will
> actually be called.
>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
>  src/gbm/backends/dri/gbm_dri.c | 36 ++++++++++++++++++++++++++++++++++++
>  src/gbm/gbm-symbols-check      |  1 +
>  src/gbm/main/gbm.c             | 19 +++++++++++++++++++
>  src/gbm/main/gbm.h             |  3 +++
>  src/gbm/main/gbmint.h          |  1 +
>  5 files changed, 60 insertions(+)
>
> diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_dri.c
> index d45ba94080..2c467127c2 100644
> --- a/src/gbm/backends/dri/gbm_dri.c
> +++ b/src/gbm/backends/dri/gbm_dri.c
> @@ -39,6 +39,7 @@
>  #include <unistd.h>
>  #include <dlfcn.h>
>  #include <xf86drm.h>
> +#include <drm_fourcc.h>
>
>  #include <GL/gl.h> /* dri_interface needs GL types */
>  #include <GL/internal/dri_interface.h>
> @@ -53,6 +54,14 @@
>  #include "../../../egl/wayland/wayland-drm/wayland-drm.h"
>  #endif
>
> +#ifndef DRM_FORMAT_MOD_INVALID
> +#define DRM_FORMAT_MOD_INVALID ((1ULL<<56) - 1)
> +#endif
> +
> +#ifndef DRM_FORMAT_MOD_LINEAR
> +#define DRM_FORMAT_MOD_LINEAR 0
> +#endif
> +
>  static __DRIimage *
>  dri_lookup_egl_image(__DRIscreen *screen, void *image, void *data)
>  {
> @@ -735,6 +744,32 @@ gbm_dri_bo_get_offset(struct gbm_bo *_bo, int plane)
>     return (uint32_t)offset;
>  }
>
> +static uint64_t
> +gbm_dri_bo_get_modifier(struct gbm_bo *_bo)
> +{
> +   struct gbm_dri_device *dri = gbm_dri_device(_bo->gbm);
> +   struct gbm_dri_bo *bo = gbm_dri_bo(_bo);
> +
> +   if (!dri->image || dri->image->base.version < 14) {
> +      errno = ENOSYS;
> +      return DRM_FORMAT_MOD_INVALID;
> +   }
> +
> +   /* Dumb buffers have no modifiers */
> +   if (!bo->image)
> +      return DRM_FORMAT_MOD_LINEAR;
> +
> +   uint64_t ret = 0;
> +   int mod;
> +   dri->image->queryImage(bo->image, __DRI_IMAGE_ATTRIB_MODIFIER_UPPER, &mod);
> +   ret = (uint64_t)mod << 32;
> +
> +   dri->image->queryImage(bo->image, __DRI_IMAGE_ATTRIB_MODIFIER_LOWER, &mod);
> +   ret |= mod;
> +
Queries of newer attributes, like here, can fail. Yes in 99% of the
cases those would be driver bugs, but a graceful fail is always
better.
Seems like the i965 driver does not handle the above queries, which
would explain my some of my earlier questions/suggestions.

Namely:
No new functionality is exposed/available - barring the new GBM entry
points [which atm, will attempt to use the old createImage() with
incomplete/incorrect usage flags]

With some queryImage() error checking above:
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

-Emil