[Mesa-dev] [PATCH 3/6] gbm: Introduce modifiers into surface/bo creation
Ben Widawsky
ben at bwidawsk.net
Tue Mar 14 21:08:34 UTC 2017
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 <ben at 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 <krh at bitplanet.net>
>>>Cc: Jason Ekstrand <jason at jlekstrand.net>
>>>References (v4): https://lists.freedesktop.org/archives/intel-gfx/2017-
>>>January/116636.html
>>>Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>>>Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com> (v1)
>>>Acked-by: Daniel Stone <daniels at 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
More information about the mesa-dev
mailing list