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? > + > + 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
