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

[Daniel Stone]

Hi,

On 6 February 2017 at 19:22, Jason Ekstrand  wrote:
> On Sun, Feb 5, 2017 at 1:15 PM, Ben Widawsky  wrote:
>> 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?

Hm, it's a little less clean, but sure, works for me.

>> 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?

That's impressively contrived, which is a polite way of saying deeply
stupid; wouldn't that break Mesa anyway? I'm happy to ban that.

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Jason Ekstrand]

On Sun, Feb 5, 2017 at 1:15 PM, Ben Widawsky  wrote:

> On 17-01-31 12:38:44, Jason Ekstrand wrote:
>
>> On Mon, Jan 23, 2017 at 10:21 PM, Ben Widawsky  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 
>>> Signed-off-by: Ben Widawsky 
>>> Reviewed-by: Eric Engestrom 
>>> Acked-by: Daniel Stone 
>>> ---
>>>  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 
>>>  #include 
>>>  #include 
>>> +#include 
>>>
>>>  #include  /* dri_interface needs GL types */
>>>  #include 
>>> @@ -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,
>>> );
>>> +   ret = (uint64_t)mod << 32;
>>> +
>>> +   dri->image->queryImage(bo->image, __DRI_IMAGE_ATTRIB_MODIFIER_LOWER,
>>> );
>>> +   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. 

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

[Ben Widawsky]

On 17-01-31 12:38:44, Jason Ekstrand wrote:

On Mon, Jan 23, 2017 at 10:21 PM, Ben Widawsky  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 
Signed-off-by: Ben Widawsky 
Reviewed-by: Eric Engestrom 
Acked-by: Daniel Stone 
---
 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 
 #include 
 #include 
+#include 

 #include  /* dri_interface needs GL types */
 #include 
@@ -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,
);
+   ret = (uint64_t)mod << 32;
+
+   dri->image->queryImage(bo->image, __DRI_IMAGE_ATTRIB_MODIFIER_LOWER,
);
+   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,
   >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;
   

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

[Jason Ekstrand]

On Mon, Jan 23, 2017 at 10:21 PM, Ben Widawsky  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.
>
> NOTE: The extension version is still set to 13, 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 
> Signed-off-by: Ben Widawsky 
> Reviewed-by: Eric Engestrom 
> Acked-by: Daniel Stone 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>
>  #include  /* dri_interface needs GL types */
>  #include 
> @@ -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"


> +   }
> +
> +   /* 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?


> +
> +   uint64_t ret = 0;
> +   int mod;
> +   dri->image->queryImage(bo->image, __DRI_IMAGE_ATTRIB_MODIFIER_UPPER,
> );
> +   ret = (uint64_t)mod << 32;
> +
> +   dri->image->queryImage(bo->image, __DRI_IMAGE_ATTRIB_MODIFIER_LOWER,
> );
> +   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?


> dri->image->queryImage(bo->image, __DRI_IMAGE_ATTRIB_HANDLE,
>>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