On 17-03-14 13:31:01, Ben Widawsky wrote:
On 17-03-14 08:53:45, Jason Ekstrand wrote:
On Mon, Mar 13, 2017 at 10:24 PM, Ben Widawsky <b...@bwidawsk.net> 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 <k...@bitplanet.net>
Cc: Jason Ekstrand <ja...@jlekstrand.net>
References (v4): https://lists.freedesktop.org/archives/intel-gfx/2017-
January/116636.html
Signed-off-by: Ben Widawsky <b...@bwidawsk.net>
Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com> (v1)
Acked-by: Daniel Stone <dani...@collabora.com>
---
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.


Oh dri_use is probably going to get missed. NVM me. I'll fix this.

+         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 &surf->base;
+   }
+
+   surf->base.modifiers = calloc(count, sizeof(*modifiers));
+   if (count && !surf->base.modifiers) {
+      errno = ENOMEM;
+      free(surf);
+      return NULL;
+   }
+
+   surf->base.count = count;
+   memcpy(surf->base.modifiers, modifiers, count * sizeof(*modifiers));

   return &surf->base;
}
@@ -1187,6 +1232,7 @@ gbm_dri_surface_destroy(struct gbm_surface *_surf)
{
   struct gbm_dri_surface *surf = gbm_dri_surface(_surf);

+   free(surf->base.modifiers);
   free(surf);
}

diff --git a/src/gbm/gbm-symbols-check b/src/gbm/gbm-symbols-check
index 81da1e0793..0550baddc4 100755
--- a/src/gbm/gbm-symbols-check
+++ b/src/gbm/gbm-symbols-check
@@ -8,6 +8,7 @@ gbm_device_is_format_supported
gbm_device_destroy
gbm_create_device
gbm_bo_create
+gbm_bo_create_with_modifiers
gbm_bo_import
gbm_bo_map
gbm_bo_unmap
@@ -27,6 +28,7 @@ gbm_bo_set_user_data
gbm_bo_get_user_data
gbm_bo_destroy
gbm_surface_create
+gbm_surface_create_with_modifiers
gbm_surface_needs_lock_front_buffer
gbm_surface_lock_front_buffer
gbm_surface_release_buffer
diff --git a/src/gbm/main/gbm.c b/src/gbm/main/gbm.c
index afcca63da3..7bacd8b86a 100644
--- a/src/gbm/main/gbm.c
+++ b/src/gbm/main/gbm.c
@@ -369,9 +369,28 @@ gbm_bo_create(struct gbm_device *gbm,
      return NULL;
   }

-   return gbm->bo_create(gbm, width, height, format, usage);
+   return gbm->bo_create(gbm, width, height, format, usage, NULL, 0);
}

+GBM_EXPORT struct gbm_bo *
+gbm_bo_create_with_modifiers(struct gbm_device *gbm,
+                             uint32_t width, uint32_t height,
+                             uint32_t format,
+                             const uint64_t *modifiers,
+                             const unsigned int count)
+{
+   if (width == 0 || height == 0) {
+      errno = EINVAL;
+      return NULL;
+   }
+
+   if ((count && !modifiers) || (modifiers && !count)) {
+      errno = EINVAL;
+      return NULL;
+   }


Is modifiers == NULL ever allowed?  If so, what does it mean?  If the
current mechanism is going to work, modifiers == NULL needs to have exactly
the same meaning as flags == 0.


+
+   return gbm->bo_create(gbm, width, height, format, 0, modifiers, count);
+}
/**
 * Create a gbm buffer object from an foreign object
 *
@@ -477,7 +496,23 @@ gbm_surface_create(struct gbm_device *gbm,
                   uint32_t width, uint32_t height,
                  uint32_t format, uint32_t flags)
{
-   return gbm->surface_create(gbm, width, height, format, flags);
+   return gbm->surface_create(gbm, width, height, format, flags, NULL, 0);
+}
+
+GBM_EXPORT struct gbm_surface *
+gbm_surface_create_with_modifiers(struct gbm_device *gbm,
+                                  uint32_t width, uint32_t height,
+                                  uint32_t format,
+                                  const uint64_t *modifiers,
+                                  const unsigned int count)
+{
+   if ((count && !modifiers) || (modifiers && !count)) {
+      errno = EINVAL;
+      return NULL;
+   }
+
+   return gbm->surface_create(gbm, width, height, format, 0,
+                              modifiers, count);
}

/**
diff --git a/src/gbm/main/gbm.h b/src/gbm/main/gbm.h
index e3e5d34d97..5f588dab58 100644
--- a/src/gbm/main/gbm.h
+++ b/src/gbm/main/gbm.h
@@ -243,6 +243,12 @@ gbm_bo_create(struct gbm_device *gbm,
              uint32_t width, uint32_t height,
              uint32_t format, uint32_t flags);

+struct gbm_bo *
+gbm_bo_create_with_modifiers(struct gbm_device *gbm,
+                             uint32_t width, uint32_t height,
+                             uint32_t format,
+                             const uint64_t *modifiers,
+                             const unsigned int count);
#define GBM_BO_IMPORT_WL_BUFFER         0x5501
#define GBM_BO_IMPORT_EGL_IMAGE         0x5502
#define GBM_BO_IMPORT_FD                0x5503
@@ -345,6 +351,12 @@ gbm_surface_create(struct gbm_device *gbm,
                   uint32_t width, uint32_t height,
                  uint32_t format, uint32_t flags);

+struct gbm_surface *
+gbm_surface_create_with_modifiers(struct gbm_device *gbm,
+                                  uint32_t width, uint32_t height,
+                                  uint32_t format,
+                                  const uint64_t *modifiers,
+                                  const unsigned int count);
int
gbm_surface_needs_lock_front_buffer(struct gbm_surface *surface);

diff --git a/src/gbm/main/gbmint.h b/src/gbm/main/gbmint.h
index a6541d91c5..d8c9f6e5d7 100644
--- a/src/gbm/main/gbmint.h
+++ b/src/gbm/main/gbmint.h
@@ -65,7 +65,9 @@ struct gbm_device {
   struct gbm_bo *(*bo_create)(struct gbm_device *gbm,
                               uint32_t width, uint32_t height,
                               uint32_t format,
-                               uint32_t usage);
+                               uint32_t usage,
+                               const uint64_t *modifiers,
+                               const unsigned int count);
   struct gbm_bo *(*bo_import)(struct gbm_device *gbm, uint32_t type,
                               void *buffer, uint32_t usage);
   void *(*bo_map)(struct gbm_bo *bo,
@@ -84,7 +86,9 @@ struct gbm_device {

   struct gbm_surface *(*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_bo *(*surface_lock_front_buffer)(struct gbm_surface
*surface);
   void (*surface_release_buffer)(struct gbm_surface *surface,
                                  struct gbm_bo *bo);
@@ -114,6 +118,10 @@ struct gbm_surface {
   uint32_t height;
   uint32_t format;
   uint32_t flags;
+   struct {
+      uint64_t *modifiers;
+      unsigned count;
+   };
};

struct gbm_backend {
--
2.12.0



--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to