[Mesa-dev] [RFC] st/dri: implement createImageWithModifiers
Varad Gautam
varadgautam at gmail.com
Mon May 15 11:01:55 UTC 2017
Hi Lucas,
On Fri, May 12, 2017 at 4:22 PM, Lucas Stach <l.stach at pengutronix.de> wrote:
> Hi Varad,
>
> Am Freitag, den 12.05.2017, 15:11 +0530 schrieb Varad Gautam:
>> gallium doesn't have a way to pass modifiers to the driver when creating
>> resources. we require this to support
>> dri2ImageExtension->createImageWithModifiers() to get
>> gbm_bo_create_with_modifiers() to work.
>>
>> this adds a pscreen->resource_create_with_modifier() to pass the modifier
>> flags to the driver when allocating textures, and implements image creation
>> with modifiers.
>>
>> requires cherry-picking the following patches from the EGL modifiers series:
>> https://patchwork.freedesktop.org/patch/155308/
>> https://patchwork.freedesktop.org/patch/155309/
>> https://patchwork.freedesktop.org/patch/155312/
>>
>> complete tree, with the egl series:
>> https://git.collabora.com/cgit/user/varad/mesa.git/log/?h=egl-modifiers-v4
>>
>> Signed-off-by: Varad Gautam <varadgautam at gmail.com>
>> ---
>> src/gallium/include/pipe/p_screen.h | 13 ++++++
>> src/gallium/state_trackers/dri/dri2.c | 80 ++++++++++++++++++++++++++++-------
>> 2 files changed, 77 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/gallium/include/pipe/p_screen.h b/src/gallium/include/pipe/p_screen.h
>> index 8b4239c..7d248ec 100644
>> --- a/src/gallium/include/pipe/p_screen.h
>> +++ b/src/gallium/include/pipe/p_screen.h
>> @@ -328,6 +328,19 @@ struct pipe_screen {
>> * driver doesn't support an on-disk shader cache.
>> */
>> struct disk_cache *(*get_disk_shader_cache)(struct pipe_screen *screen);
>> +
>> + /**
>> + * Create a new texture object from the given template info, taking
>> + * format modifier into account. \p modifier adheres to the format
>> + * modifier tokens specified in drm_fourcc.h.
>> + *
>> + * Returns NULL if the \p modifier is unsupported by the driver.
>> + */
>> + struct pipe_resource * (*resource_create_with_modifier)(
>> + struct pipe_screen *,
>> + const struct pipe_resource *templat,
>> + uint64_t modifier);
>> +
>> };
>>
>>
>> diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
>> index 42fa155..54ca5fe 100644
>> --- a/src/gallium/state_trackers/dri/dri2.c
>> +++ b/src/gallium/state_trackers/dri/dri2.c
>> @@ -977,9 +977,12 @@ dri2_create_image_from_renderbuffer(__DRIcontext *context,
>> }
>>
>> static __DRIimage *
>> -dri2_create_image(__DRIscreen *_screen,
>> - int width, int height, int format,
>> - unsigned int use, void *loaderPrivate)
>> +dri2_create_image_common(__DRIscreen *_screen,
>> + int width, int height,
>> + int format, unsigned int use,
>> + const uint64_t *modifiers,
>> + const unsigned count,
>> + void *loaderPrivate)
>> {
>> struct dri_screen *screen = dri_screen(_screen);
>> __DRIimage *img;
>> @@ -987,17 +990,29 @@ dri2_create_image(__DRIscreen *_screen,
>> unsigned tex_usage;
>> enum pipe_format pf;
>>
>> + /* createImageWithModifiers does not take a usage flag, the caller can
>> + * specify either modifiers or usage.
>> + */
>> + assert(!(use && (modifiers != NULL)));
>> +
>> + /* XXX: it is probably a bad idea to assume that all drivers will support
>> + * usage = PIPE_BIND_RENDER_TARGET | PIPE_BIND_SAMPLER_VIEW with all of
>> + * their modifiers. tex_usage must be determined in the driver instead
>> + * when modifiers are present.
>> + */
>
> I don't think we can push this to the driver, because what we want is
> the driver to select a modifier, that is actually able to be used with a
> least RENDER_TARGET. Probably even SCANOUT also. At least the current
> use of the new entry point for GBM create_surface implies those 2
> usages.
>
>> tex_usage = PIPE_BIND_RENDER_TARGET | PIPE_BIND_SAMPLER_VIEW;
>> - if (use & __DRI_IMAGE_USE_SCANOUT)
>> - tex_usage |= PIPE_BIND_SCANOUT;
>> - if (use & __DRI_IMAGE_USE_SHARE)
>> - tex_usage |= PIPE_BIND_SHARED;
>> - if (use & __DRI_IMAGE_USE_LINEAR)
>> - tex_usage |= PIPE_BIND_LINEAR;
>> - if (use & __DRI_IMAGE_USE_CURSOR) {
>> - if (width != 64 || height != 64)
>> - return NULL;
>> - tex_usage |= PIPE_BIND_CURSOR;
>> + if (use) {
>> + if (use & __DRI_IMAGE_USE_SCANOUT)
>> + tex_usage |= PIPE_BIND_SCANOUT;
>> + if (use & __DRI_IMAGE_USE_SHARE)
>> + tex_usage |= PIPE_BIND_SHARED;
>> + if (use & __DRI_IMAGE_USE_LINEAR)
>> + tex_usage |= PIPE_BIND_LINEAR;
>> + if (use & __DRI_IMAGE_USE_CURSOR) {
>> + if (width != 64 || height != 64)
>> + return NULL;
>> + tex_usage |= PIPE_BIND_CURSOR;
>> + }
>> }
>>
>> pf = dri2_format_to_pipe_format (format);
>> @@ -1018,7 +1033,18 @@ dri2_create_image(__DRIscreen *_screen,
>> templ.depth0 = 1;
>> templ.array_size = 1;
>>
>> - img->texture = screen->base.screen->resource_create(screen->base.screen, &templ);
>> + if (modifiers && screen->base.screen->resource_create_with_modifier)
>> + /* pick the first modifier for alloc. */
>> + img->texture = screen->base.screen
>> + ->resource_create_with_modifier(screen->base.screen,
>> + &templ,
>> + modifiers[0]);
>> + else if (modifiers)
>> + /* the driver doesn't support tex creation with modifiers, return */
>> + img->texture = NULL;
>
> This is wrong. Returning a NULL texture is different from not supporting
> the extension at all. What you need to do is only advertise the dri2
> creatImageWithModifiers entry point when the pipe driver supports the
> new resource create function. Like this in dri2_init_screen():
>
> if (pscreen->resource_create_with_modifiers)
> dri2ImageExtension.createImageWithModifiers = dri2_create_image_modifiers;
>
Done.
>> + else
>> + img->texture = screen->base.screen->resource_create(screen->base.screen,
>> + &templ);
>> if (!img->texture) {
>> FREE(img);
>> return NULL;
>> @@ -1029,12 +1055,34 @@ dri2_create_image(__DRIscreen *_screen,
>> img->dri_format = format;
>> img->dri_components = 0;
>> img->use = use;
>> - img->modifier = DRM_FORMAT_MOD_INVALID;
>> + img->modifier = modifiers ? modifiers[0] : DRM_FORMAT_MOD_INVALID;
>
> So the pipe driver should modify the modifiers array to put the chosen
> modifier at entry 0? This needs at least a documentation hint.
>
Done - I have deferred modifier selection to the driver instead of
arbitrarily picking modifiers[0] here.
> I would prefer to pass the modifier through the winsys_handle, as done
> in your patch "st/dri: implement DRIimage creation from dmabufs with
> modifiers" and query the chosen modifier by creating a winsys_handle
> from the texture.
Could you elaborate on this a bit? While importing from dmabufs, since
we already have a backing buffer, using winsys_handle works with
setting whandle->handle = dmabuf_fd. Can I pass an empty
whandle->handle to allocate a bo from scratch? Or do you mean
something like the following for dri2_create_image_with_modifiers(),
instead of introducing a new pscreen->resource_create_with_modifiers()
?
/* create regular tex - no modifiers */
prsc = pscreen->resource_create(...);
/* get whandle for tex */
pscreen->resource_get_handle(..., prsc, &whandle, ...);
/* force-set modifier - to be selected by the driver */
whandle.modifier = select_modifier(modifiers);
/* import with selected modifier */
img->texture = pscreen->resource_from_handle(.., prsc, whandle, ..);
/* we now have a tex with modifiers! */
Thank you for looking at this!
Varad
>>
>> img->loader_private = loaderPrivate;
>> return img;
>> }
>>
>> +static __DRIimage *
>> +dri2_create_image(__DRIscreen *_screen,
>> + int width, int height, int format,
>> + unsigned int use, void *loaderPrivate)
>> +{
>> + return dri2_create_image_common(_screen, width, height, format, use,
>> + NULL /* modifiers */, 0 /* count */,
>> + loaderPrivate);
>> +}
>> +
>> +static __DRIimage *
>> +dri2_create_image_with_modifiers(__DRIscreen *dri_screen,
>> + int width, int height, int format,
>> + const uint64_t *modifiers,
>> + const unsigned count,
>> + void *loaderPrivate)
>> +{
>> + return dri2_create_image_common(dri_screen, width, height, format,
>> + 0 /* use */, modifiers, count,
>> + loaderPrivate);
>> +}
>> +
>> static GLboolean
>> dri2_query_image(__DRIimage *image, int attrib, int *value)
>> {
>> @@ -1450,7 +1498,7 @@ static __DRIimageExtension dri2ImageExtension = {
>> .getCapabilities = dri2_get_capabilities,
>> .mapImage = dri2_map_image,
>> .unmapImage = dri2_unmap_image,
>> - .createImageWithModifiers = NULL,
>> + .createImageWithModifiers = dri2_create_image_with_modifiers,
>> };
>
>
> Regards,
> Lucas
>
More information about the mesa-dev
mailing list