<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Mar 9, 2017 at 5:48 PM, Ben Widawsky <<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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 <<a href="mailto:krh@bitplanet.net" target="_blank">krh@bitplanet.net</a>> Cc: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> References (v4): <a href="https://lists.freedesktop.org/archives/intel-gfx/2017-January/116636.html" rel="noreferrer" target="_blank">https://lists.freedesktop.org/archives/intel-gfx/2017-January/116636.html</a> Signed-off-by: Ben Widawsky <<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>> Reviewed-by: Eric Engestrom <<a href="mailto:eric.engestrom@imgtec.com" target="_blank">eric.engestrom@imgtec.com</a>> (v1) Acked-by: Daniel Stone <<a href="mailto:daniels@collabora.com" target="_blank">daniels@collabora.com</a>> --- 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_dri->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"); </blockquote><div> </div><div>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? </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + 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; </blockquote><div> Dumb question, but do we also want to check the DRI version here as well? Maybe something like </div><div>if (modifiers && (!dri->image || dri->image->base.version < 14 || !dri->image->createImageWithModifiers)) { </div><div> errno = ENOSYS; </div><div> return NULL; } </div><div>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? </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + surf->base.modifiers = calloc(count, sizeof(*modifiers)); + if (count && !surf->base.modifiers) { + free(surf); + return NULL; + } </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + 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 </blockquote></div> </div></div>