On Thu, Mar 9, 2017 at 6:52 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> On Thu, Mar 9, 2017 at 5:48 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/egl/drivers/dri2/platform_drm.c | 19 ++++++++++++--- >> src/gbm/backends/dri/gbm_dri.c | 42 >> ++++++++++++++++++++++++++------ >> src/gbm/gbm-symbols-check | 2 ++ >> src/gbm/main/gbm.c | 29 ++++++++++++++++++++-- >> src/gbm/main/gbm.h | 12 +++++++++ >> src/gbm/main/gbmint.h | 12 +++++++-- >> src/mesa/drivers/dri/i965/intel_screen.c | 18 ++++++++++++++ >> 7 files changed, 119 insertions(+), 15 deletions(-) >> >> diff --git a/src/egl/drivers/dri2/platform_drm.c >> b/src/egl/drivers/dri2/platform_drm.c >> index e5e8c60596..cf35ce8a1f 100644 >> --- a/src/egl/drivers/dri2/platform_drm.c >> +++ b/src/egl/drivers/dri2/platform_drm.c >> @@ -230,10 +230,21 @@ get_back_bo(struct dri2_egl_surface *dri2_surf) >> >> if (dri2_surf->back == NULL) >> return -1; >> - if (dri2_surf->back->bo == NULL) >> - dri2_surf->back->bo = gbm_bo_create(&dri2_dpy->gbm_dri->base.base, >> - surf->base.width, >> surf->base.height, >> - surf->base.format, >> surf->base.flags); >> + if (dri2_surf->back->bo == NULL) { >> + if (surf->base.modifiers) >> + dri2_surf->back->bo = gbm_bo_create_with_modifiers(& >> dri2_dpy->gbm_dri->base.base, >> + >> surf->base.width, surf->base.height, >> + >> surf->base.format, >> + >> surf->base.modifiers, >> + >> surf->base.count); >> + else >> + dri2_surf->back->bo = gbm_bo_create(&dri2_dpy->gbm_d >> ri->base.base, >> + surf->base.width, >> + surf->base.height, >> + surf->base.format, >> + surf->base.flags); >> + >> + } >> if (dri2_surf->back->bo == NULL) >> return -1; >> >> diff --git a/src/gbm/backends/dri/gbm_dri.c >> b/src/gbm/backends/dri/gbm_dri.c >> index 7106dc1229..d45ba94080 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,21 @@ 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"); >> > > This seems a bit on the sketchy side. Do we want to fail, set errno to > ENOSYS, and return NULL in this case? I'm not really sure how GBM > versioning works. Daniel? Kristian? > Thinking about this a bit more, I think what you want is if (modifiers) { if (!dri->image || dri->image->base.version < 14 || !dri->image->createImageWithModifiers) { errno = ENOSYS; /* Clean up */ return NULL; } /* Create with modifiers */ } else { /* Create without modifiers */ } > + >> + 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,7 +1182,8 @@ 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_surface *surf; >> >> @@ -1179,6 +1197,15 @@ gbm_dri_surface_create(struct gbm_device *gbm, >> surf->base.format = format; >> surf->base.flags = flags; >> > > Dumb question, but do we also want to check the DRI version here as well? > Maybe something like > > if (modifiers && > (!dri->image || dri->image->base.version < 14 || > !dri->image->createImageWithModifiers)) { > errno = ENOSYS; > return NULL; > } > > That way the user can't create a surface that will fail to get a back > buffer above. Also, do we want to set errno? > > >> + surf->base.modifiers = calloc(count, sizeof(*modifiers)); >> + if (count && !surf->base.modifiers) { >> + free(surf); >> + return NULL; >> + } >> > + >> + surf->base.count = count; >> + memcpy(surf->base.modifiers, modifiers, count * sizeof(*modifiers)); >> + >> return &surf->base; >> } >> >> @@ -1187,6 +1214,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 7ff78ab400..c137c6cd93 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..0fb62657ed 100644 >> --- a/src/gbm/main/gbm.c >> +++ b/src/gbm/main/gbm.c >> @@ -369,9 +369,23 @@ 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; >> + } >> + >> + return gbm->bo_create(gbm, width, height, format, 0, modifiers, >> count); >> +} >> /** >> * Create a gbm buffer object from an foreign object >> * >> @@ -477,7 +491,18 @@ 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) >> +{ >> + 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 { >> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c >> b/src/mesa/drivers/dri/i965/intel_screen.c >> index 3452572874..395a71b3b3 100644 >> --- a/src/mesa/drivers/dri/i965/intel_screen.c >> +++ b/src/mesa/drivers/dri/i965/intel_screen.c >> @@ -41,6 +41,10 @@ >> #include "utils.h" >> #include "xmlpool.h" >> >> +#ifndef DRM_FORMAT_MOD_INVALID >> +#define DRM_FORMAT_MOD_INVALID ((1ULL<<56) - 1) >> +#endif >> + >> static const __DRIconfigOptionsExtension brw_config_options = { >> .base = { __DRI_CONFIG_OPTIONS, 1 }, >> .xml = >> @@ -509,6 +513,14 @@ intel_destroy_image(__DRIimage *image) >> free(image); >> } >> >> +static uint64_t >> +select_best_modifier(struct gen_device_info *devinfo, >> + const uint64_t *modifiers, >> + const unsigned count) >> +{ >> + return DRM_FORMAT_MOD_INVALID; >> +} >> + >> static __DRIimage * >> __intel_create_image(__DRIscreen *dri_screen, >> int width, int height, int format, >> @@ -529,6 +541,12 @@ __intel_create_image(__DRIscreen *dri_screen, >> */ >> assert(!(use && count)); >> >> + uint64_t modifier = select_best_modifier(&screen->devinfo, >> modifiers, count); >> + assert(modifier == DRM_FORMAT_MOD_INVALID); >> + >> + /* Historically, X-tiled was the default, and so lack of modifier >> means >> + * X-tiled. >> + */ >> tiling = I915_TILING_X; >> if (use & __DRI_IMAGE_USE_CURSOR) { >> if (width != 64 || height != 64) >> -- >> 2.12.0 >> >> >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
