[Mesa-dev] [PATCH 14/34] gbm: Get modifiers from DRI

Jason Ekstrand jason at jlekstrand.net
Mon Feb 6 19:22:30 UTC 2017


On Sun, Feb 5, 2017 at 1:15 PM, Ben Widawsky <ben at bwidawsk.net> wrote:

> On 17-01-31 12:38:44, Jason Ekstrand wrote:
>
>> On Mon, Jan 23, 2017 at 10:21 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
>>
>> Replace the naive, 'save all the modifiers' with a proper query for just
>>> the modifier that was selected. 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.
>>>
>>> Yes>> NOTE: The extension version is still set to 12, so none of this
> will
>
> actually be called.
>>>
>>> v2: Use stored modifiers from create instead of queryImage
>>>
>>> v3: Make sure not to query modifiers for dumb buffers (Daniel)
>>> Fixed comments in functions.
>>>
>>> Cc: Daniel Stone <daniel at fooishbar.org>
>>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>>> Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>
>>> Acked-by: Daniel Stone <daniels at collabora.com>
>>> ---
>>>  src/gbm/backends/dri/gbm_dri.c           | 37
>>> ++++++++++++++++++++++++--------
>>>  src/gbm/gbm-symbols-check                |  1 +
>>>  src/gbm/main/gbm.c                       | 19 ++++++++++++++++
>>>  src/gbm/main/gbm.h                       |  3 +++
>>>  src/gbm/main/gbmint.h                    |  5 +----
>>>  src/mesa/drivers/dri/i965/intel_screen.c |  6 ++++++
>>>  6 files changed, 58 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/src/gbm/backends/dri/gbm_dri.c
>>> b/src/gbm/backends/dri/gbm_dri
>>> .c
>>> index a777f1a984..d5b458aa38 100644
>>> --- a/src/gbm/backends/dri/gbm_dri.c
>>> +++ b/src/gbm/backends/dri/gbm_dri.c
>>> @@ -38,6 +38,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>
>>> @@ -732,6 +733,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 0;
>>>
>>>
>> Do we want to return the invalid modifier in the error case?  I thought 0
>> was "linear"
>>
>>
>>
> Introducing the LINEAR modifier (which happened after v2 of this series)
> did
> make things complex because it's possible in some horrific future that a
> image
> doesn't support linear. As a result, you are correct. I think for this
> case, the
> client can handle it pretty easily, and returning INVALID is the right
> thing to
> do.
>
> Daniel, are you okay with changing this to return DRM_FORMAT_MOD_INVALID?
>
> +   }
>>> +
>>> +   /* Dumb buffers have no modifiers */
>>> +   if (!bo->image)
>>> +      return 0;
>>>
>>>
>> Same here.  I'm not really sure that this is an error, but saying it's
>> linear might be a lie.  I guess this is a static function so maybe it
>> doesn't matter?
>>
>>
> Yeah, this is also a lie but way trickier than the above. Again before
> this rev
> of the series, 0 meant DRM_FORMAT_MOD_NONE, and that was actually legit,
> however, now it does mean LINEAR. I believe it's safe to assume that all
> dumb
> BOs are linear, but it should probably be baked in somewhere better. One
> option
> would be to create a proper DRIimage for a dumb BO, but I think the best
> bet is
> to just replace 0 with DRM_FORMAT_MOD_LINEAR.
>

That sounds fairly reasonable to me.  I guess someone could create a BO
with GBM and then call the kernel ioctl to set the tiling mode to X-tiled
and then ask what it has.  However, short of calling into the driver and
having it query the kernel, I don't see a good way to get around that.  I
think I'd be ok with just returning LINEAR and saying "don't do that".
Daniel?

--Jason


>
>> +
>>> +   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;
>>> +
>>> +   return ret;
>>> +}
>>> +
>>>  static void
>>>  gbm_dri_bo_destroy(struct gbm_bo *_bo)
>>>  {
>>> @@ -1074,15 +1101,6 @@ gbm_dri_bo_create(struct gbm_device *gbm,
>>>     if (bo->image == NULL)
>>>        goto failed;
>>>
>>> -   bo->base.base.modifiers = calloc(count, sizeof(*modifiers));
>>> -   if (count && !bo->base.base.modifiers) {
>>> -      dri->image->destroyImage(bo->image);
>>> -      goto failed;
>>> -   }
>>> -
>>> -   bo->base.base.count = count;
>>> -   memcpy(bo->base.base.modifiers, modifiers, count *
>>> sizeof(*modifiers));
>>> -
>>>
>>>
>> What's going on here?  Is this in the right patch?
>>
>>
>>
> Yes, but no. Originally all the modifiers were saved/stored at creation
> and I
> did something with the list at query time. Over time this changed and the
> modifier is chosen at creation time (at this point in the series,
> __intel_create_image()). As a result, the original code which saves all the
> modifiers is bogus and can be deleted. The first logically place to remove
> this
> code is when we return the new modifier, here, but it's all bogus until now
> anyway. Essentially, this hunk needs to be squashed into where it was
> introduced:
>   gbm: Introduce modifiers into surface/bo creation
>
> GBM Surface creation still needs this however since there is a more complex
> deferred BO allocation there (the surface create API basically does
> nothing).
>
>
>     dri->image->queryImage(bo->image, __DRI_IMAGE_ATTRIB_HANDLE,
>>>                            &bo->base.base.handle.s32);
>>>     dri->image->queryImage(bo->image, __DRI_IMAGE_ATTRIB_STRIDE,
>>> @@ -1230,6 +1248,7 @@ dri_device_create(int fd)
>>>     dri->base.base.bo_get_handle = gbm_dri_bo_get_handle_for_plane;
>>>     dri->base.base.bo_get_stride = gbm_dri_bo_get_stride;
>>>     dri->base.base.bo_get_offset = gbm_dri_bo_get_offset;
>>> +   dri->base.base.bo_get_modifier = gbm_dri_bo_get_modifier;
>>>     dri->base.base.bo_destroy = gbm_dri_bo_destroy;
>>>     dri->base.base.destroy = dri_destroy;
>>>     dri->base.base.surface_create = gbm_dri_surface_create;
>>> diff --git a/src/gbm/gbm-symbols-check b/src/gbm/gbm-symbols-check
>>> index c137c6cd93..c72fb66b03 100755
>>> --- a/src/gbm/gbm-symbols-check
>>> +++ b/src/gbm/gbm-symbols-check
>>> @@ -23,6 +23,7 @@ gbm_bo_get_handle
>>>  gbm_bo_get_fd
>>>  gbm_bo_get_plane_count
>>>  gbm_bo_get_handle_for_plane
>>> +gbm_bo_get_modifier
>>>  gbm_bo_write
>>>  gbm_bo_set_user_data
>>>  gbm_bo_get_user_data
>>> diff --git a/src/gbm/main/gbm.c b/src/gbm/main/gbm.c
>>> index bfdd009bef..8780b41914 100644
>>> --- a/src/gbm/main/gbm.c
>>> +++ b/src/gbm/main/gbm.c
>>> @@ -280,6 +280,25 @@ gbm_bo_get_handle_for_plane(struct gbm_bo *bo, int
>>> plane)
>>>     return bo->gbm->bo_get_handle(bo, plane);
>>>  }
>>>
>>> +/**
>>> + * Get the chosen modifier for the buffer object
>>> + *
>>> + * This function returns the modifier that was chosen for the object.
>>> These
>>> + * properties may be generic, or platform/implementation dependent.
>>> + *
>>> + * \param bo The buffer object
>>> + * \return Returns the selected modifier (chosen by the implementation)
>>> for the
>>> + * BO.
>>> + * \sa gbm_bo_create_with_modifiers() where possible modifiers are set
>>> + * \sa gbm_surface_create_with_modifiers() where possible modifiers are
>>> set
>>> + * \sa define DRM_FORMAT_MOD_* in drm_fourcc.h for possible modifiers
>>> + */
>>> +GBM_EXPORT uint64_t
>>> +gbm_bo_get_modifier(struct gbm_bo *bo)
>>> +{
>>> +   return bo->gbm->bo_get_modifier(bo);
>>> +}
>>> +
>>>  /** Write data into the buffer object
>>>   *
>>>   * If the buffer object was created with the GBM_BO_USE_WRITE flag,
>>> diff --git a/src/gbm/main/gbm.h b/src/gbm/main/gbm.h
>>> index 39fb9d2d29..b52137ed01 100644
>>> --- a/src/gbm/main/gbm.h
>>> +++ b/src/gbm/main/gbm.h
>>> @@ -327,6 +327,9 @@ gbm_bo_get_handle(struct gbm_bo *bo);
>>>  int
>>>  gbm_bo_get_fd(struct gbm_bo *bo);
>>>
>>> +uint64_t
>>> +gbm_bo_get_modifier(struct gbm_bo *bo);
>>> +
>>>  int
>>>  gbm_bo_get_plane_count(struct gbm_bo *bo);
>>>
>>> diff --git a/src/gbm/main/gbmint.h b/src/gbm/main/gbmint.h
>>> index cd437df021..c27a7a560a 100644
>>> --- a/src/gbm/main/gbmint.h
>>> +++ b/src/gbm/main/gbmint.h
>>> @@ -82,6 +82,7 @@ struct gbm_device {
>>>     union gbm_bo_handle (*bo_get_handle)(struct gbm_bo *bo, int plane);
>>>     uint32_t (*bo_get_stride)(struct gbm_bo *bo, int plane);
>>>     uint32_t (*bo_get_offset)(struct gbm_bo *bo, int plane);
>>> +   uint64_t (*bo_get_modifier)(struct gbm_bo *bo);
>>>     void (*bo_destroy)(struct gbm_bo *bo);
>>>
>>>     struct gbm_surface *(*surface_create)(struct gbm_device *gbm,
>>> @@ -107,10 +108,6 @@ struct gbm_bo {
>>>     uint32_t height;
>>>     uint32_t stride;
>>>     uint32_t format;
>>> -   struct {
>>> -      uint64_t *modifiers;
>>> -      unsigned int count;
>>> -   };
>>>
>>>
>> Same here.
>>
>>
>>     union gbm_bo_handle  handle;
>>>     void *user_data;
>>>     void (*destroy_user_data)(struct gbm_bo *, void *);
>>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
>>> b/src/mesa/drivers/dri/i965/intel_screen.c
>>> index eb4f3d7e6b..91b89f0a93 100644
>>> --- a/src/mesa/drivers/dri/i965/intel_screen.c
>>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>>> @@ -743,6 +743,12 @@ intel_query_image(__DRIimage *image, int attrib, int
>>> *value)
>>>     case __DRI_IMAGE_ATTRIB_OFFSET:
>>>        *value = image->offset;
>>>        return true;
>>> +   case __DRI_IMAGE_ATTRIB_MODIFIER_LOWER:
>>> +      *value = (image->modifier & 0xffffffff);
>>> +      return true;
>>> +   case __DRI_IMAGE_ATTRIB_MODIFIER_UPPER:
>>> +      *value = ((image->modifier >> 32) & 0xffffffff);
>>> +      return true;
>>>
>>>    default:
>>>        return false;
>>>
>>
> D
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170206/292ae091/attachment-0001.html>


More information about the mesa-dev mailing list