subject:"\[Mesa\-dev\] \[PATCH 3\/6\] gbm\: Introduce modifiers into surface\/bo creation"

Re: [Mesa-dev] [PATCH 3/6] gbm: Introduce modifiers into surface/bo creation

2017-03-14 Thread Ben Widawsky


On 17-03-14 08:53:45, Jason Ekstrand wrote:

On Mon, Mar 13, 2017 at 10:24 PM, Ben Widawsky  wrote:


The idea behind modifiers like this is that the user of GBM will have
some mechanism to query what properties the hardware supports for its BO
or surface. This information is directly passed in (and stored) so that
the DRI implementation can create an image with the appropriate
attributes.

A getter() will be added later so that the user GBM will be able to
query what modifier should be used.

Only in surface creation, the modifiers are stored until the BO is
actually allocated. In regular buffer allocation, the correct modifier
can (will be, in future patches be chosen at creation time.

v2: Make sure to check if count is non-zero in addition to testing if
calloc fails. (Daniel)

v3: Remove "usage" and "flags" from modifier creation. Requested by
Kristian.

v4: Take advantage of the "INVALID" modifier added by the GET_PLANE2
series.

v5: Don't bother with storing modifiers for gbm_bo_create because that's
a synchronous operation and we can actually select the correct modifier
at create time (done in a later patch) (Jason)

Cc: Kristian Høgsberg 
Cc: Jason Ekstrand 
References (v4): https://lists.freedesktop.org/archives/intel-gfx/2017-
January/116636.html
Signed-off-by: Ben Widawsky 
Reviewed-by: Eric Engestrom  (v1)
Acked-by: Daniel Stone 
---
 src/gbm/backends/dri/gbm_dri.c | 62 ++
++--
 src/gbm/gbm-symbols-check  |  2 ++
 src/gbm/main/gbm.c | 39 --
 src/gbm/main/gbm.h | 12 
 src/gbm/main/gbmint.h  | 12 ++--
 5 files changed, 115 insertions(+), 12 deletions(-)

diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_
dri.c
index 7106dc1229..1224ce4476 100644
--- a/src/gbm/backends/dri/gbm_dri.c
+++ b/src/gbm/backends/dri/gbm_dri.c
@@ -1023,13 +1023,20 @@ free_bo:
 static struct gbm_bo *
 gbm_dri_bo_create(struct gbm_device *gbm,
   uint32_t width, uint32_t height,
-  uint32_t format, uint32_t usage)
+  uint32_t format, uint32_t usage,
+  const uint64_t *modifiers,
+  const unsigned int count)
 {
struct gbm_dri_device *dri = gbm_dri_device(gbm);
struct gbm_dri_bo *bo;
int dri_format;
unsigned dri_use = 0;

+   /* Callers of this may specify a modifier, or a dri usage, but not
both. The
+* newer modifier interface deprecates the older usage flags.
+*/
+   assert(!(usage && count));
+
if (usage & GBM_BO_USE_WRITE || dri->image == NULL)
   return create_dumb(gbm, width, height, format, usage);

@@ -1087,11 +1094,24 @@ gbm_dri_bo_create(struct gbm_device *gbm,
/* Gallium drivers requires shared in order to get the handle/stride */
dri_use |= __DRI_IMAGE_USE_SHARE;

-   bo->image =
-  dri->image->createImage(dri->screen,
-  width, height,
-  dri_format, dri_use,
-  bo);
+   if (!dri->image || dri->image->base.version < 14 ||
+   !dri->image->createImageWithModifiers) {
+  if (modifiers) {



This logic still isn't correct.  As written, it will always call
createImageWithModifiers if it's available even if no modifiers are
provided.  I think you want the "if (modifiers)" to be the outside bit of
control-flow




I don't see a problem here. Calling createImageWithModifiers without modifiers
(provided count is correct) is a perfectly acceptable thing to do.


+ fprintf(stderr, "Modifiers specified, but DRI is too old\n");
+ errno = ENOSYS;
+ goto failed;
+  }
+
+  bo->image = dri->image->createImage(dri->screen, width, height,
+  dri_format, dri_use, bo);
+   } else {
+  bo->image =
+ dri->image->createImageWithModifiers(dri->screen,
+  width, height,
+  dri_format,
+  modifiers, count,
+  bo);
+   }
if (bo->image == NULL)
   goto failed;

@@ -1165,19 +1185,44 @@ gbm_dri_bo_unmap(struct gbm_bo *_bo, void
*map_data)
 static struct gbm_surface *
 gbm_dri_surface_create(struct gbm_device *gbm,
uint32_t width, uint32_t height,
-  uint32_t format, uint32_t flags)
+  uint32_t format, uint32_t flags,
+   const uint64_t *modifiers, const unsigned count)
 {
+   struct gbm_dri_device *dri = gbm_dri_device(gbm);
struct gbm_dri_surface *surf;

+   if (modifiers &&
+   (!dri->image || dri->image->base.version < 14 ||
+!dri->image->createImageWithModifiers)) {
+  errno = ENOSYS;
+  return NULL;

Re: [Mesa-dev] [PATCH 3/6] gbm: Introduce modifiers into surface/bo creation

2017-03-14 Thread Jason Ekstrand

The comments below are my only remaining questions on this 6-patch series.
All of the *other* patches are

Reviewed-by: Jason Ekstrand 

On Tue, Mar 14, 2017 at 8:53 AM, Jason Ekstrand 
wrote:

> On Mon, Mar 13, 2017 at 10:24 PM, Ben Widawsky  wrote:
>
>> The idea behind modifiers like this is that the user of GBM will have
>> some mechanism to query what properties the hardware supports for its BO
>> or surface. This information is directly passed in (and stored) so that
>> the DRI implementation can create an image with the appropriate
>> attributes.
>>
>> A getter() will be added later so that the user GBM will be able to
>> query what modifier should be used.
>>
>> Only in surface creation, the modifiers are stored until the BO is
>> actually allocated. In regular buffer allocation, the correct modifier
>> can (will be, in future patches be chosen at creation time.
>>
>> v2: Make sure to check if count is non-zero in addition to testing if
>> calloc fails. (Daniel)
>>
>> v3: Remove "usage" and "flags" from modifier creation. Requested by
>> Kristian.
>>
>> v4: Take advantage of the "INVALID" modifier added by the GET_PLANE2
>> series.
>>
>> v5: Don't bother with storing modifiers for gbm_bo_create because that's
>> a synchronous operation and we can actually select the correct modifier
>> at create time (done in a later patch) (Jason)
>>
>> Cc: Kristian Høgsberg 
>> Cc: Jason Ekstrand 
>> References (v4): https://lists.freedesktop.org/
>> archives/intel-gfx/2017-January/116636.html
>> Signed-off-by: Ben Widawsky 
>> Reviewed-by: Eric Engestrom  (v1)
>> Acked-by: Daniel Stone 
>> ---
>>  src/gbm/backends/dri/gbm_dri.c | 62 ++
>> ++--
>>  src/gbm/gbm-symbols-check  |  2 ++
>>  src/gbm/main/gbm.c | 39 --
>>  src/gbm/main/gbm.h | 12 
>>  src/gbm/main/gbmint.h  | 12 ++--
>>  5 files changed, 115 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/gbm/backends/dri/gbm_dri.c
>> b/src/gbm/backends/dri/gbm_dri.c
>> index 7106dc1229..1224ce4476 100644
>> --- a/src/gbm/backends/dri/gbm_dri.c
>> +++ b/src/gbm/backends/dri/gbm_dri.c
>> @@ -1023,13 +1023,20 @@ free_bo:
>>  static struct gbm_bo *
>>  gbm_dri_bo_create(struct gbm_device *gbm,
>>uint32_t width, uint32_t height,
>> -  uint32_t format, uint32_t usage)
>> +  uint32_t format, uint32_t usage,
>> +  const uint64_t *modifiers,
>> +  const unsigned int count)
>>  {
>> struct gbm_dri_device *dri = gbm_dri_device(gbm);
>> struct gbm_dri_bo *bo;
>> int dri_format;
>> unsigned dri_use = 0;
>>
>> +   /* Callers of this may specify a modifier, or a dri usage, but not
>> both. The
>> +* newer modifier interface deprecates the older usage flags.
>> +*/
>> +   assert(!(usage && count));
>> +
>> if (usage & GBM_BO_USE_WRITE || dri->image == NULL)
>>return create_dumb(gbm, width, height, format, usage);
>>
>> @@ -1087,11 +1094,24 @@ gbm_dri_bo_create(struct gbm_device *gbm,
>> /* Gallium drivers requires shared in order to get the handle/stride
>> */
>> dri_use |= __DRI_IMAGE_USE_SHARE;
>>
>> -   bo->image =
>> -  dri->image->createImage(dri->screen,
>> -  width, height,
>> -  dri_format, dri_use,
>> -  bo);
>> +   if (!dri->image || dri->image->base.version < 14 ||
>> +   !dri->image->createImageWithModifiers) {
>> +  if (modifiers) {
>>
>
> This logic still isn't correct.  As written, it will always call
> createImageWithModifiers if it's available even if no modifiers are
> provided.  I think you want the "if (modifiers)" to be the outside bit of
> control-flow
>
>
>> + fprintf(stderr, "Modifiers specified, but DRI is too old\n");
>> + errno = ENOSYS;
>> + goto failed;
>> +  }
>> +
>> +  bo->image = dri->image->createImage(dri->screen, width, height,
>> +  dri_format, dri_use, bo);
>> +   } else {
>> +  bo->image =
>> + dri->image->createImageWithModifiers(dri->screen,
>> +  width, height,
>> +  dri_format,
>> +  modifiers, count,
>> +  bo);
>> +   }
>> if (bo->image == NULL)
>>goto failed;
>>
>> @@ -1165,19 +1185,44 @@ gbm_dri_bo_unmap(struct gbm_bo *_bo, void
>> *map_data)
>>  static struct gbm_surface *
>>  gbm_dri_surface_create(struct gbm_device *gbm,
>> uint32_t width, uint32_t height,
>> -  uint32_t format, uint32_t flags)
>> +

Re: [Mesa-dev] [PATCH 3/6] gbm: Introduce modifiers into surface/bo creation

2017-03-14 Thread Jason Ekstrand

On Mon, Mar 13, 2017 at 10:24 PM, Ben Widawsky  wrote:

> The idea behind modifiers like this is that the user of GBM will have
> some mechanism to query what properties the hardware supports for its BO
> or surface. This information is directly passed in (and stored) so that
> the DRI implementation can create an image with the appropriate
> attributes.
>
> A getter() will be added later so that the user GBM will be able to
> query what modifier should be used.
>
> Only in surface creation, the modifiers are stored until the BO is
> actually allocated. In regular buffer allocation, the correct modifier
> can (will be, in future patches be chosen at creation time.
>
> v2: Make sure to check if count is non-zero in addition to testing if
> calloc fails. (Daniel)
>
> v3: Remove "usage" and "flags" from modifier creation. Requested by
> Kristian.
>
> v4: Take advantage of the "INVALID" modifier added by the GET_PLANE2
> series.
>
> v5: Don't bother with storing modifiers for gbm_bo_create because that's
> a synchronous operation and we can actually select the correct modifier
> at create time (done in a later patch) (Jason)
>
> Cc: Kristian Høgsberg 
> Cc: Jason Ekstrand 
> References (v4): https://lists.freedesktop.org/archives/intel-gfx/2017-
> January/116636.html
> Signed-off-by: Ben Widawsky 
> Reviewed-by: Eric Engestrom  (v1)
> Acked-by: Daniel Stone 
> ---
>  src/gbm/backends/dri/gbm_dri.c | 62 ++
> ++--
>  src/gbm/gbm-symbols-check  |  2 ++
>  src/gbm/main/gbm.c | 39 --
>  src/gbm/main/gbm.h | 12 
>  src/gbm/main/gbmint.h  | 12 ++--
>  5 files changed, 115 insertions(+), 12 deletions(-)
>
> diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_
> dri.c
> index 7106dc1229..1224ce4476 100644
> --- a/src/gbm/backends/dri/gbm_dri.c
> +++ b/src/gbm/backends/dri/gbm_dri.c
> @@ -1023,13 +1023,20 @@ free_bo:
>  static struct gbm_bo *
>  gbm_dri_bo_create(struct gbm_device *gbm,
>uint32_t width, uint32_t height,
> -  uint32_t format, uint32_t usage)
> +  uint32_t format, uint32_t usage,
> +  const uint64_t *modifiers,
> +  const unsigned int count)
>  {
> struct gbm_dri_device *dri = gbm_dri_device(gbm);
> struct gbm_dri_bo *bo;
> int dri_format;
> unsigned dri_use = 0;
>
> +   /* Callers of this may specify a modifier, or a dri usage, but not
> both. The
> +* newer modifier interface deprecates the older usage flags.
> +*/
> +   assert(!(usage && count));
> +
> if (usage & GBM_BO_USE_WRITE || dri->image == NULL)
>return create_dumb(gbm, width, height, format, usage);
>
> @@ -1087,11 +1094,24 @@ gbm_dri_bo_create(struct gbm_device *gbm,
> /* Gallium drivers requires shared in order to get the handle/stride */
> dri_use |= __DRI_IMAGE_USE_SHARE;
>
> -   bo->image =
> -  dri->image->createImage(dri->screen,
> -  width, height,
> -  dri_format, dri_use,
> -  bo);
> +   if (!dri->image || dri->image->base.version < 14 ||
> +   !dri->image->createImageWithModifiers) {
> +  if (modifiers) {
>

This logic still isn't correct.  As written, it will always call
createImageWithModifiers if it's available even if no modifiers are
provided.  I think you want the "if (modifiers)" to be the outside bit of
control-flow


> + fprintf(stderr, "Modifiers specified, but DRI is too old\n");
> + errno = ENOSYS;
> + goto failed;
> +  }
> +
> +  bo->image = dri->image->createImage(dri->screen, width, height,
> +  dri_format, dri_use, bo);
> +   } else {
> +  bo->image =
> + dri->image->createImageWithModifiers(dri->screen,
> +  width, height,
> +  dri_format,
> +  modifiers, count,
> +  bo);
> +   }
> if (bo->image == NULL)
>goto failed;
>
> @@ -1165,19 +1185,44 @@ gbm_dri_bo_unmap(struct gbm_bo *_bo, void
> *map_data)
>  static struct gbm_surface *
>  gbm_dri_surface_create(struct gbm_device *gbm,
> uint32_t width, uint32_t height,
> -  uint32_t format, uint32_t flags)
> +  uint32_t format, uint32_t flags,
> +   const uint64_t *modifiers, const unsigned count)
>  {
> +   struct gbm_dri_device *dri = gbm_dri_device(gbm);
> struct gbm_dri_surface *surf;
>
> +   if (modifiers &&
> +   (!dri->image || dri->image->base.version < 14 ||
> +!dri->image->createImageWithModifiers)) {
> +  errno =

[Mesa-dev] [PATCH 3/6] gbm: Introduce modifiers into surface/bo creation

2017-03-13 Thread Ben Widawsky

The idea behind modifiers like this is that the user of GBM will have
some mechanism to query what properties the hardware supports for its BO
or surface. This information is directly passed in (and stored) so that
the DRI implementation can create an image with the appropriate
attributes.

A getter() will be added later so that the user GBM will be able to
query what modifier should be used.

Only in surface creation, the modifiers are stored until the BO is
actually allocated. In regular buffer allocation, the correct modifier
can (will be, in future patches be chosen at creation time.

v2: Make sure to check if count is non-zero in addition to testing if
calloc fails. (Daniel)

v3: Remove "usage" and "flags" from modifier creation. Requested by
Kristian.

v4: Take advantage of the "INVALID" modifier added by the GET_PLANE2
series.

v5: Don't bother with storing modifiers for gbm_bo_create because that's
a synchronous operation and we can actually select the correct modifier
at create time (done in a later patch) (Jason)

Cc: Kristian Høgsberg 
Cc: Jason Ekstrand 
References (v4): 
https://lists.freedesktop.org/archives/intel-gfx/2017-January/116636.html
Signed-off-by: Ben Widawsky 
Reviewed-by: Eric Engestrom  (v1)
Acked-by: Daniel Stone 
---
 src/gbm/backends/dri/gbm_dri.c | 62 --
 src/gbm/gbm-symbols-check  |  2 ++
 src/gbm/main/gbm.c | 39 --
 src/gbm/main/gbm.h | 12 
 src/gbm/main/gbmint.h  | 12 ++--
 5 files changed, 115 insertions(+), 12 deletions(-)

diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_dri.c
index 7106dc1229..1224ce4476 100644
--- a/src/gbm/backends/dri/gbm_dri.c
+++ b/src/gbm/backends/dri/gbm_dri.c
@@ -1023,13 +1023,20 @@ free_bo:
 static struct gbm_bo *
 gbm_dri_bo_create(struct gbm_device *gbm,
   uint32_t width, uint32_t height,
-  uint32_t format, uint32_t usage)
+  uint32_t format, uint32_t usage,
+  const uint64_t *modifiers,
+  const unsigned int count)
 {
struct gbm_dri_device *dri = gbm_dri_device(gbm);
struct gbm_dri_bo *bo;
int dri_format;
unsigned dri_use = 0;
 
+   /* Callers of this may specify a modifier, or a dri usage, but not both. The
+* newer modifier interface deprecates the older usage flags.
+*/
+   assert(!(usage && count));
+
if (usage & GBM_BO_USE_WRITE || dri->image == NULL)
   return create_dumb(gbm, width, height, format, usage);
 
@@ -1087,11 +1094,24 @@ gbm_dri_bo_create(struct gbm_device *gbm,
/* Gallium drivers requires shared in order to get the handle/stride */
dri_use |= __DRI_IMAGE_USE_SHARE;
 
-   bo->image =
-  dri->image->createImage(dri->screen,
-  width, height,
-  dri_format, dri_use,
-  bo);
+   if (!dri->image || dri->image->base.version < 14 ||
+   !dri->image->createImageWithModifiers) {
+  if (modifiers) {
+ fprintf(stderr, "Modifiers specified, but DRI is too old\n");
+ errno = ENOSYS;
+ goto failed;
+  }
+
+  bo->image = dri->image->createImage(dri->screen, width, height,
+  dri_format, dri_use, bo);
+   } else {
+  bo->image =
+ dri->image->createImageWithModifiers(dri->screen,
+  width, height,
+  dri_format,
+  modifiers, count,
+  bo);
+   }
if (bo->image == NULL)
   goto failed;
 
@@ -1165,19 +1185,44 @@ gbm_dri_bo_unmap(struct gbm_bo *_bo, void *map_data)
 static struct gbm_surface *
 gbm_dri_surface_create(struct gbm_device *gbm,
uint32_t width, uint32_t height,
-  uint32_t format, uint32_t flags)
+  uint32_t format, uint32_t flags,
+   const uint64_t *modifiers, const unsigned count)
 {
+   struct gbm_dri_device *dri = gbm_dri_device(gbm);
struct gbm_dri_surface *surf;
 
+   if (modifiers &&
+   (!dri->image || dri->image->base.version < 14 ||
+!dri->image->createImageWithModifiers)) {
+  errno = ENOSYS;
+  return NULL;
+   }
+
surf = calloc(1, sizeof *surf);
-   if (surf == NULL)
+   if (surf == NULL) {
+  errno = ENOMEM;
   return NULL;
+   }
 
surf->base.gbm = gbm;
surf->base.width = width;
surf->base.height = height;
surf->base.format = format;
surf->base.flags = flags;
+   if (!modifiers) {
+  assert(!count);
+  return >base;
+   }
+
+   surf->base.modifiers = calloc(count, sizeof(*modifiers));
+   if (count && !surf->base.modifiers) {
+  errno = ENOMEM;
+

Re: [Mesa-dev] [PATCH 3/6] gbm: Introduce modifiers into surface/bo creation

Re: [Mesa-dev] [PATCH 3/6] gbm: Introduce modifiers into surface/bo creation

Re: [Mesa-dev] [PATCH 3/6] gbm: Introduce modifiers into surface/bo creation

[Mesa-dev] [PATCH 3/6] gbm: Introduce modifiers into surface/bo creation

4 matches

Site Navigation

Mail list logo

Footer information