[Mesa-dev] [PATCH v3 4/6] nouveau: Add framebuffer modifier support
Thierry Reding
thierry.reding at gmail.com
Fri Mar 2 12:06:40 UTC 2018
On Thu, Mar 01, 2018 at 09:37:28AM -0500, Ilia Mirkin wrote:
> On Thu, Mar 1, 2018 at 8:54 AM, Thierry Reding <thierry.reding at gmail.com> wrote:
> > From: Thierry Reding <treding at nvidia.com>
> >
> > This adds support for framebuffer modifiers to Nouveau. This will be
> > used by the Tegra driver to share metadata about the format of buffers
> > (such as the tiling mode or compression).
> >
> > Changes in v2:
> > - remove unused parameters to nouveau_buffer_create()
> > - move format modifier query code to nvc0 backend
> > - restrict format modifiers to 2D textures
> > - implement ->query_dmabuf_modifiers()
> >
> > Acked-by: Emil Velikov <emil.velikov at collabora.com>
> > Tested-by: Andre Heider <a.heider at gmail.com>
> > Signed-off-by: Thierry Reding <treding at nvidia.com>
> > ---
> > src/gallium/drivers/nouveau/Android.mk | 3 +
> > src/gallium/drivers/nouveau/Makefile.am | 1 +
> > src/gallium/drivers/nouveau/nouveau_screen.c | 4 ++
> > src/gallium/drivers/nouveau/nv30/nv30_resource.c | 2 +
> > src/gallium/drivers/nouveau/nvc0/nvc0_miptree.c | 81 +++++++++++++++++++++++-
> > src/gallium/drivers/nouveau/nvc0/nvc0_resource.c | 59 ++++++++++++++++-
> > src/gallium/drivers/nouveau/nvc0/nvc0_resource.h | 3 +-
> > 7 files changed, 149 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/gallium/drivers/nouveau/Android.mk b/src/gallium/drivers/nouveau/Android.mk
> > index 2de22e73ec18..a446774a86e8 100644
> > --- a/src/gallium/drivers/nouveau/Android.mk
> > +++ b/src/gallium/drivers/nouveau/Android.mk
> > @@ -36,6 +36,9 @@ LOCAL_SRC_FILES := \
> > $(NVC0_CODEGEN_SOURCES) \
> > $(NVC0_C_SOURCES)
> >
> > +LOCAL_C_INCLUDES := \
> > + $(MESA_TOP)/include/drm-uapi
> > +
> > LOCAL_SHARED_LIBRARIES := libdrm_nouveau
> > LOCAL_MODULE := libmesa_pipe_nouveau
> >
> > diff --git a/src/gallium/drivers/nouveau/Makefile.am b/src/gallium/drivers/nouveau/Makefile.am
> > index 91547178e397..f6126b544811 100644
> > --- a/src/gallium/drivers/nouveau/Makefile.am
> > +++ b/src/gallium/drivers/nouveau/Makefile.am
> > @@ -24,6 +24,7 @@ include Makefile.sources
> > include $(top_srcdir)/src/gallium/Automake.inc
> >
> > AM_CPPFLAGS = \
> > + -I$(top_srcdir)/include/drm-uapi \
> > $(GALLIUM_DRIVER_CFLAGS) \
> > $(LIBDRM_CFLAGS) \
> > $(NOUVEAU_CFLAGS)
>
> Someone is likely to complain about forgetting about the N+1 build
> system, meson.
Interestingly I wasn't seeing any build issues without the Meson change.
I think that was probably just out of luck because I was building
against a locally modifier version of libdrm which had the UAPI changes.
Fixed now.
> > diff --git a/src/gallium/drivers/nouveau/nouveau_screen.c b/src/gallium/drivers/nouveau/nouveau_screen.c
> > index c144b39b2dd2..b84ef13ebe7f 100644
> > --- a/src/gallium/drivers/nouveau/nouveau_screen.c
> > +++ b/src/gallium/drivers/nouveau/nouveau_screen.c
> > @@ -1,3 +1,5 @@
> > +#include <drm_fourcc.h>
> > +
> > #include "pipe/p_defines.h"
> > #include "pipe/p_screen.h"
> > #include "pipe/p_state.h"
> > @@ -23,6 +25,8 @@
> > #include "nouveau_mm.h"
> > #include "nouveau_buffer.h"
> >
> > +#include "nvc0/nvc0_resource.h"
>
> Can't have that... why do you need it here?
I don't. This is left over from an earlier version where code later in
this file was calling into the nvc0 backend directly. I've since then
pushed that code into the nvc0 backend itself but forgot to remove the
include here.
Dropped now.
> > +
> > /* XXX this should go away */
> > #include "state_tracker/drm_driver.h"
> >
> > diff --git a/src/gallium/drivers/nouveau/nv30/nv30_resource.c b/src/gallium/drivers/nouveau/nv30/nv30_resource.c
> > index ff34f6e5f9fa..386bd3459bd3 100644
> > --- a/src/gallium/drivers/nouveau/nv30/nv30_resource.c
> > +++ b/src/gallium/drivers/nouveau/nv30/nv30_resource.c
> > @@ -23,6 +23,8 @@
> > *
> > */
> >
> > +#include <drm_fourcc.h>
> > +
> > #include "util/u_format.h"
> > #include "util/u_inlines.h"
> >
> > diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_miptree.c b/src/gallium/drivers/nouveau/nvc0/nvc0_miptree.c
> > index 27674f72a7c0..7983c4030876 100644
> > --- a/src/gallium/drivers/nouveau/nvc0/nvc0_miptree.c
> > +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_miptree.c
> > @@ -20,8 +20,11 @@
> > * OTHER DEALINGS IN THE SOFTWARE.
> > */
> >
> > +#include <drm_fourcc.h>
> > +
> > #include "pipe/p_state.h"
> > #include "pipe/p_defines.h"
> > +#include "state_tracker/drm_driver.h"
> > #include "util/u_inlines.h"
> > #include "util/u_format.h"
> >
> > @@ -233,9 +236,79 @@ nvc0_miptree_init_layout_tiled(struct nv50_miptree *mt)
> > }
> > }
> >
> > +static uint64_t nvc0_miptree_get_modifier(struct nv50_miptree *mt)
> > +{
> > + union nouveau_bo_config *config = &mt->base.bo->config;
> > + uint64_t modifier;
> > +
> > + if (mt->layout_3d)
> > + return DRM_FORMAT_MOD_INVALID;
> > +
> > + switch (config->nvc0.memtype) {
> > + case 0x00:
> > + modifier = DRM_FORMAT_MOD_LINEAR;
> > + break;
> > +
> > + case 0xfe:
> > + switch (NVC0_TILE_MODE_Y(config->nvc0.tile_mode)) {
> > + case 0:
> > + modifier = DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_ONE_GOB;
> > + break;
> > +
> > + case 1:
> > + modifier = DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_TWO_GOB;
> > + break;
> > +
> > + case 2:
> > + modifier = DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_FOUR_GOB;
> > + break;
> > +
> > + case 3:
> > + modifier = DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_EIGHT_GOB;
> > + break;
> > +
> > + case 4:
> > + modifier = DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_SIXTEEN_GOB;
> > + break;
> > +
> > + case 5:
> > + modifier = DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_THIRTYTWO_GOB;
> > + break;
> > +
> > + default:
> > + modifier = DRM_FORMAT_MOD_INVALID;
> > + break;
> > + }
> > + break;
> > +
> > + default:
> > + modifier = DRM_FORMAT_MOD_INVALID;
> > + break;
> > + }
> > +
> > + return modifier;
> > +}
> > +
> > +static boolean
> > +nvc0_miptree_get_handle(struct pipe_screen *pscreen,
> > + struct pipe_resource *pt,
> > + struct winsys_handle *whandle)
> > +{
> > + struct nv50_miptree *mt = nv50_miptree(pt);
> > + boolean ret;
> > +
> > + ret = nv50_miptree_get_handle(pscreen, pt, whandle);
> > + if (!ret)
> > + return ret;
> > +
> > + whandle->modifier = nvc0_miptree_get_modifier(mt);
> > +
> > + return true;
> > +}
> > +
> > const struct u_resource_vtbl nvc0_miptree_vtbl =
> > {
> > - nv50_miptree_get_handle, /* get_handle */
> > + nvc0_miptree_get_handle, /* get_handle */
> > nv50_miptree_destroy, /* resource_destroy */
> > nvc0_miptree_transfer_map, /* transfer_map */
> > u_default_transfer_flush_region, /* transfer_flush_region */
> > @@ -244,7 +317,8 @@ const struct u_resource_vtbl nvc0_miptree_vtbl =
> >
> > struct pipe_resource *
> > nvc0_miptree_create(struct pipe_screen *pscreen,
> > - const struct pipe_resource *templ)
> > + const struct pipe_resource *templ,
> > + const uint64_t *modifiers, unsigned int count)
> > {
> > struct nouveau_device *dev = nouveau_screen(pscreen)->device;
> > struct nouveau_drm *drm = nouveau_screen(pscreen)->drm;
> > @@ -277,6 +351,9 @@ nvc0_miptree_create(struct pipe_screen *pscreen,
> > }
> > }
> >
> > + if (count == 1 && modifiers[0] == DRM_FORMAT_MOD_LINEAR)
> > + pt->flags |= NOUVEAU_RESOURCE_FLAG_LINEAR;
> > +
> > if (pt->bind & PIPE_BIND_LINEAR)
> > pt->flags |= NOUVEAU_RESOURCE_FLAG_LINEAR;
> >
> > diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_resource.c b/src/gallium/drivers/nouveau/nvc0/nvc0_resource.c
> > index 9bafe3d835db..ec6257a89633 100644
> > --- a/src/gallium/drivers/nouveau/nvc0/nvc0_resource.c
> > +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_resource.c
> > @@ -1,3 +1,4 @@
> > +#include <drm_fourcc.h>
> >
> > #include "pipe/p_context.h"
> > #include "nvc0/nvc0_resource.h"
> > @@ -8,14 +9,68 @@ static struct pipe_resource *
> > nvc0_resource_create(struct pipe_screen *screen,
> > const struct pipe_resource *templ)
> > {
> > + const uint64_t modifier = DRM_FORMAT_MOD_INVALID;
> > +
> > switch (templ->target) {
> > case PIPE_BUFFER:
> > return nouveau_buffer_create(screen, templ);
> > default:
> > - return nvc0_miptree_create(screen, templ);
> > + return nvc0_miptree_create(screen, templ, &modifier, 1);
> > }
> > }
> >
> > +static struct pipe_resource *
> > +nvc0_resource_create_with_modifiers(struct pipe_screen *screen,
> > + const struct pipe_resource *templ,
> > + const uint64_t *modifiers, int count)
> > +{
> > + switch (templ->target) {
> > + case PIPE_BUFFER:
> > + return nouveau_buffer_create(screen, templ);
> > + default:
> > + return nvc0_miptree_create(screen, templ, modifiers, count);
> > + }
> > +}
> > +
> > +static void
> > +nvc0_query_dmabuf_modifiers(struct pipe_screen *screen,
> > + enum pipe_format format, int max,
>
> Maybe change this to "unsigned max"? That would avoid unnecessary
> complications below to check if it's negative.
I would like that very much, but I'm afraid it's probably too late to
change this now because the signedness is handed down directly from the
EGL_EXT_image_dma_buf_import_modifiers extension:
https://www.khronos.org/registry/EGL/extensions/EXT/EGL_EXT_image_dma_buf_import_modifiers.txt
Adding Pekka and Daniel for visibility, maybe there were reasons for why
these are explicitly signed. I'm not familiar with the process of
getting an extension changed, but, even though a very minor change, this
would be changing the API of a completed extension, which doesn't seem
like something that you're supposed to do.
That said, the revision history of the extension mentions that revision
4 introduced a new type for the modifiers, so perhaps there is some
leeway in what we can do.
I guess we could always force a cast in dri2_query_dma_buf_modifiers()
if changing the extension is not possible. That way we could at least
internally not worry about the signedness.
>
> > + uint64_t *modifiers, unsigned int *external_only,
> > + int *count)
> > +{
> > + static const uint64_t supported_modifiers[] = {
> > + DRM_FORMAT_MOD_LINEAR,
> > + DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_ONE_GOB,
> > + DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_TWO_GOB,
> > + DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_FOUR_GOB,
> > + DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_EIGHT_GOB,
> > + DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_SIXTEEN_GOB,
> > + DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK_THIRTYTWO_GOB,
> > + };
> > + int i, num = 0;
> > +
> > + if (max > ARRAY_SIZE(supported_modifiers))
> > + max = ARRAY_SIZE(supported_modifiers);
> > +
> > + if (!max) {
> > + max = ARRAY_SIZE(supported_modifiers);
> > + external_only = NULL;
> > + modifiers = NULL;
> > + }
> > +
> > + for (i = 0; i < max; i++) {
> > + if (modifiers)
> > + modifiers[num] = supported_modifiers[i];
> > +
> > + if (external_only)
> > + external_only[num] = 0;
> > +
> > + num++;
>
> num == i under all circumstances, no?
Yes. I was referencing the etnaviv and vc4/5 implementation of this
callback and they do additional filtering here which may cause both
to deviate. I wanted to keep that to make the code more futureproof
just in case we ever need to do additional filtering as well.
Thierry
> > + }
> > +
> > + *count = num;
> > +}
> > +
> > static struct pipe_resource *
> > nvc0_resource_from_handle(struct pipe_screen * screen,
> > const struct pipe_resource *templ,
> > @@ -60,6 +115,8 @@ void
> > nvc0_screen_init_resource_functions(struct pipe_screen *pscreen)
> > {
> > pscreen->resource_create = nvc0_resource_create;
> > + pscreen->resource_create_with_modifiers = nvc0_resource_create_with_modifiers;
> > + pscreen->query_dmabuf_modifiers = nvc0_query_dmabuf_modifiers;
> > pscreen->resource_from_handle = nvc0_resource_from_handle;
> > pscreen->resource_get_handle = u_resource_get_handle_vtbl;
> > pscreen->resource_destroy = u_resource_destroy_vtbl;
> > diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_resource.h b/src/gallium/drivers/nouveau/nvc0/nvc0_resource.h
> > index c68a50948360..b24fca004cad 100644
> > --- a/src/gallium/drivers/nouveau/nvc0/nvc0_resource.h
> > +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_resource.h
> > @@ -35,7 +35,8 @@ nvc0_screen_init_resource_functions(struct pipe_screen *pscreen);
> > */
> > struct pipe_resource *
> > nvc0_miptree_create(struct pipe_screen *pscreen,
> > - const struct pipe_resource *tmp);
> > + const struct pipe_resource *tmp,
> > + const uint64_t *modifiers, unsigned int count);
> >
> > const struct u_resource_vtbl nvc0_miptree_vtbl;
> >
> > --
> > 2.16.2
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180302/816810bd/attachment.sig>
More information about the mesa-dev
mailing list