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 <varadgau...@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;
> + 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.
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.
>
> 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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
