[Mesa-dev] [PATCH 14/34] gbm: Get modifiers from DRI
Ben Widawsky
ben at bwidawsk.net
Sun Feb 5 21:15:17 UTC 2017
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.
>
>> +
>> + 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
More information about the mesa-dev
mailing list