[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