The comments below are my only remaining questions on this 6-patch series. All of the *other* patches are
Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net> On Tue, Mar 14, 2017 at 8:53 AM, Jason Ekstrand <ja...@jlekstrand.net> 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 > > >> + 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 >> >> >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
