[Mesa-dev] [PATCH 2/3] gbm: Introduce modifiers into surface/bo creation
Ben Widawsky
ben at bwidawsk.net
Tue Mar 14 00:30:45 UTC 2017
On 17-03-10 11:34:19, Jason Ekstrand wrote:
>On Thu, Mar 9, 2017 at 6:52 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>
>> On Thu, Mar 9, 2017 at 5:48 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/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 */
>}
>
>
I don't think so. I don't see a reason to fail if modifiers were specified but
they cannot be used. Nowhere is there a guarantee built in to the API that if
you ask for a modifier you will get it (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
>>>
>>>
>>
--
Ben Widawsky, Intel Open Source Technology Center
More information about the mesa-dev
mailing list