Re: [Mesa-dev] [PATCH 4/5] nouveau: Add framebuffer modifier support
On Wed, Feb 21, 2018 at 04:37:45PM +, Emil Velikov wrote:
> Hi Thierry,
>
> On 21 February 2018 at 15:30, Thierry Reding wrote:
>
> >
> > struct pipe_resource *
> > nouveau_buffer_create(struct pipe_screen *pscreen,
> > - const struct pipe_resource *templ);
> > + const struct pipe_resource *templ,
> > + const uint64_t *modifiers, unsigned int count);
> >
> The extra arguments seem unused. I guess it's a left-over from earlier
> iteration?
> Or perhaps you have extra patches that depend on this?
I don't exactly recall why I added those. I guess I must've thought that
the call to nouveau_buffer_create() should be symmetric with the call to
nvc0_miptree_create().
But you're right, I don't think it makes sense to have modifiers for
PIPE_BUFFER, so I'll drop these.
>
>
> > struct pipe_resource *
> > nouveau_user_buffer_create(struct pipe_screen *screen, void *ptr,
> > diff --git a/src/gallium/drivers/nouveau/nouveau_screen.c
> > b/src/gallium/drivers/nouveau/nouveau_screen.c
> > index c144b39b2dd2..d651cc7f4b8c 100644
> > --- a/src/gallium/drivers/nouveau/nouveau_screen.c
> > +++ b/src/gallium/drivers/nouveau/nouveau_screen.c
> > @@ -1,3 +1,5 @@
> > +#include
> > +
> > #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"
> > +
> > +static uint64_t nouveau_bo_get_modifier(struct nouveau_bo *bo)
> > +{
> > + struct nouveau_device *dev = bo->device;
> > +
> > + if (dev->chipset >= 0xc0)
> > + return nvc0_bo_get_modifier(bo);
> > +
> > + return DRM_FORMAT_MOD_INVALID;
> > +}
> >
> Normally the backends (include and) call into the core nouveau code.
> Calling In the opposite direction is achieved via vfuncs, IIRC.
I think I've figured out a much nicer way to fix this (see also my reply
to Ilia's comment). I'll follow up with a patch to show what I mean.
>
>
> > switch (templ->target) {
> > case PIPE_BUFFER:
> > - return nouveau_buffer_create(screen, templ);
> > + return nouveau_buffer_create(screen, templ, &modifier, 1);
> > default:
> >return nv50_miptree_create(screen, templ);
> > }
> > diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_miptree.c
> > b/src/gallium/drivers/nouveau/nvc0/nvc0_miptree.c
> > index 27674f72a7c0..627d6b7346c3 100644
> > --- a/src/gallium/drivers/nouveau/nvc0/nvc0_miptree.c
> > +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_miptree.c
> > @@ -20,6 +20,8 @@
> > * OTHER DEALINGS IN THE SOFTWARE.
> > */
> >
> > +#include
> > +
> > #include "pipe/p_state.h"
> > #include "pipe/p_defines.h"
> > #include "util/u_inlines.h"
> > @@ -244,7 +246,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 +280,9 @@ nvc0_miptree_create(struct pipe_screen *pscreen,
> >}
> > }
> >
> > + if (count == 1 && modifiers[0] == DRM_FORMAT_MOD_LINEAR)
> > + pt->flags |= NOUVEAU_RESOURCE_FLAG_LINEAR;
> > +
> Through the patch count 1 + DRM_FORMAT_MOD_INVALID is used, yet
> DRM_FORMAT_MOD_LINEAR is checked above.
> Am I having a silly moment and those should be the same or ?
count == 1 and DRM_FORMAT_MOD_INVALID is used in those cases where we
don't care about modifiers.
In this case, however, the idea is that if we're passed in a single
modifier that happens to be DRM_FORMAT_MOD_LINEAR, we do want to make
sure that we're getting a pitch-linear buffer because it's been
requested.
Note that this is a somewhat minimal way of dealing with modifiers. To
be correct we'd have to pass along exactly the modifiers that we got in
the list. For example a caller could pass a list of block-linear
modifiers with different block heights each, in order of priority. That
is something which we would then have to use to override the values
chosen by nvc0_tex_choose_tile_dims_helper().
In practice, however, we don't really care. Typically we'll just run
with whatever Nouveau has determined to be the best tile_mode. For any
2D texture we should be able to deal with it in the display engine. So
I don't really forsee anyone passing in a specific block height, but
either only DRM_FORMAT_MOD_LINEAR (if they want to render to it using
the CPU, for example) or DRM_FORMAT_MOD_INVALID (don't care, use what
Nouveau thinks is best).
>
> > +static void
> > +nvc0_query_dmabuf_modifiers(struct pipe_screen *screen,
> > +enum pipe_format format, int max,
> > +
Re: [Mesa-dev] [PATCH 4/5] nouveau: Add framebuffer modifier support
On Wed, Feb 21, 2018 at 11:05:45AM -0500, Ilia Mirkin wrote:
> On Wed, Feb 21, 2018 at 10:30 AM, Thierry Reding
> wrote:
> > From: Thierry Reding
> >
> > 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).
> >
> > Signed-off-by: Thierry Reding
> > ---
> > src/gallium/drivers/nouveau/Android.mk | 3 +
> > src/gallium/drivers/nouveau/Makefile.am | 1 +
> > src/gallium/drivers/nouveau/nouveau_buffer.c | 3 +-
> > src/gallium/drivers/nouveau/nouveau_buffer.h | 3 +-
> > src/gallium/drivers/nouveau/nouveau_screen.c | 14 +
> > src/gallium/drivers/nouveau/nv30/nv30_resource.c | 6 +-
> > src/gallium/drivers/nouveau/nv50/nv50_resource.c | 5 +-
> > src/gallium/drivers/nouveau/nvc0/nvc0_miptree.c | 8 ++-
> > src/gallium/drivers/nouveau/nvc0/nvc0_resource.c | 80
> > +++-
> > src/gallium/drivers/nouveau/nvc0/nvc0_resource.h | 5 +-
> > 10 files changed, 120 insertions(+), 8 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)
> > diff --git a/src/gallium/drivers/nouveau/nouveau_buffer.c
> > b/src/gallium/drivers/nouveau/nouveau_buffer.c
> > index 2c604419ce05..73afff961115 100644
> > --- a/src/gallium/drivers/nouveau/nouveau_buffer.c
> > +++ b/src/gallium/drivers/nouveau/nouveau_buffer.c
> > @@ -636,7 +636,8 @@ const struct u_resource_vtbl nouveau_buffer_vtbl =
> >
> > struct pipe_resource *
> > nouveau_buffer_create(struct pipe_screen *pscreen,
> > - const struct pipe_resource *templ)
> > + const struct pipe_resource *templ,
> > + const uint64_t *modifiers, unsigned int count)
> > {
> > struct nouveau_screen *screen = nouveau_screen(pscreen);
> > struct nv04_resource *buffer;
> > diff --git a/src/gallium/drivers/nouveau/nouveau_buffer.h
> > b/src/gallium/drivers/nouveau/nouveau_buffer.h
> > index 3a33fae9ce2f..466f8cc2b466 100644
> > --- a/src/gallium/drivers/nouveau/nouveau_buffer.h
> > +++ b/src/gallium/drivers/nouveau/nouveau_buffer.h
> > @@ -89,7 +89,8 @@ nouveau_resource_mapped_by_gpu(struct pipe_resource
> > *resource)
> >
> > struct pipe_resource *
> > nouveau_buffer_create(struct pipe_screen *pscreen,
> > - const struct pipe_resource *templ);
> > + const struct pipe_resource *templ,
> > + const uint64_t *modifiers, unsigned int count);
> >
> > struct pipe_resource *
> > nouveau_user_buffer_create(struct pipe_screen *screen, void *ptr,
> > diff --git a/src/gallium/drivers/nouveau/nouveau_screen.c
> > b/src/gallium/drivers/nouveau/nouveau_screen.c
> > index c144b39b2dd2..d651cc7f4b8c 100644
> > --- a/src/gallium/drivers/nouveau/nouveau_screen.c
> > +++ b/src/gallium/drivers/nouveau/nouveau_screen.c
> > @@ -1,3 +1,5 @@
> > +#include
> > +
> > #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"
> > +
> > /* XXX this should go away */
> > #include "state_tracker/drm_driver.h"
> >
> > @@ -124,6 +128,15 @@ nouveau_screen_bo_from_handle(struct pipe_screen
> > *pscreen,
> > return bo;
> > }
> >
> > +static uint64_t nouveau_bo_get_modifier(struct nouveau_bo *bo)
> > +{
> > + struct nouveau_device *dev = bo->device;
> > +
> > + if (dev->chipset >= 0xc0)
> > + return nvc0_bo_get_modifier(bo);
> > +
> > + return DRM_FORMAT_MOD_INVALID;
> > +}
> >
> > bool
> > nouveau_screen_bo_get_handle(struct pipe_screen *pscreen,
> > @@ -131,6 +144,7 @@ nouveau_screen_bo_get_handle(struct pipe_screen
> > *pscreen,
> > unsigned stride,
> > struct winsys_handle *whandle)
> > {
> > + whandle->modifier = nouveau_bo_get_modifier(bo);
> > whandle->stride
Re: [Mesa-dev] [PATCH 4/5] nouveau: Add framebuffer modifier support
Hi Thierry,
On 21 February 2018 at 15:30, Thierry Reding wrote:
>
> struct pipe_resource *
> nouveau_buffer_create(struct pipe_screen *pscreen,
> - const struct pipe_resource *templ);
> + const struct pipe_resource *templ,
> + const uint64_t *modifiers, unsigned int count);
>
The extra arguments seem unused. I guess it's a left-over from earlier
iteration?
Or perhaps you have extra patches that depend on this?
> struct pipe_resource *
> nouveau_user_buffer_create(struct pipe_screen *screen, void *ptr,
> diff --git a/src/gallium/drivers/nouveau/nouveau_screen.c
> b/src/gallium/drivers/nouveau/nouveau_screen.c
> index c144b39b2dd2..d651cc7f4b8c 100644
> --- a/src/gallium/drivers/nouveau/nouveau_screen.c
> +++ b/src/gallium/drivers/nouveau/nouveau_screen.c
> @@ -1,3 +1,5 @@
> +#include
> +
> #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"
> +
> +static uint64_t nouveau_bo_get_modifier(struct nouveau_bo *bo)
> +{
> + struct nouveau_device *dev = bo->device;
> +
> + if (dev->chipset >= 0xc0)
> + return nvc0_bo_get_modifier(bo);
> +
> + return DRM_FORMAT_MOD_INVALID;
> +}
>
Normally the backends (include and) call into the core nouveau code.
Calling In the opposite direction is achieved via vfuncs, IIRC.
> switch (templ->target) {
> case PIPE_BUFFER:
> - return nouveau_buffer_create(screen, templ);
> + return nouveau_buffer_create(screen, templ, &modifier, 1);
> default:
>return nv50_miptree_create(screen, templ);
> }
> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_miptree.c
> b/src/gallium/drivers/nouveau/nvc0/nvc0_miptree.c
> index 27674f72a7c0..627d6b7346c3 100644
> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_miptree.c
> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_miptree.c
> @@ -20,6 +20,8 @@
> * OTHER DEALINGS IN THE SOFTWARE.
> */
>
> +#include
> +
> #include "pipe/p_state.h"
> #include "pipe/p_defines.h"
> #include "util/u_inlines.h"
> @@ -244,7 +246,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 +280,9 @@ nvc0_miptree_create(struct pipe_screen *pscreen,
>}
> }
>
> + if (count == 1 && modifiers[0] == DRM_FORMAT_MOD_LINEAR)
> + pt->flags |= NOUVEAU_RESOURCE_FLAG_LINEAR;
> +
Through the patch count 1 + DRM_FORMAT_MOD_INVALID is used, yet
DRM_FORMAT_MOD_LINEAR is checked above.
Am I having a silly moment and those should be the same or ?
> +static void
> +nvc0_query_dmabuf_modifiers(struct pipe_screen *screen,
> +enum pipe_format format, int max,
> +uint64_t *modifiers, unsigned int *external_only,
> +int *count)
> +{
> +}
Add a TODO/WIP/EMPTY note with some brief info why it's empty?
-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/5] nouveau: Add framebuffer modifier support
On Wed, Feb 21, 2018 at 10:30 AM, Thierry Reding
wrote:
> From: Thierry Reding
>
> 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).
>
> Signed-off-by: Thierry Reding
> ---
> src/gallium/drivers/nouveau/Android.mk | 3 +
> src/gallium/drivers/nouveau/Makefile.am | 1 +
> src/gallium/drivers/nouveau/nouveau_buffer.c | 3 +-
> src/gallium/drivers/nouveau/nouveau_buffer.h | 3 +-
> src/gallium/drivers/nouveau/nouveau_screen.c | 14 +
> src/gallium/drivers/nouveau/nv30/nv30_resource.c | 6 +-
> src/gallium/drivers/nouveau/nv50/nv50_resource.c | 5 +-
> src/gallium/drivers/nouveau/nvc0/nvc0_miptree.c | 8 ++-
> src/gallium/drivers/nouveau/nvc0/nvc0_resource.c | 80
> +++-
> src/gallium/drivers/nouveau/nvc0/nvc0_resource.h | 5 +-
> 10 files changed, 120 insertions(+), 8 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)
> diff --git a/src/gallium/drivers/nouveau/nouveau_buffer.c
> b/src/gallium/drivers/nouveau/nouveau_buffer.c
> index 2c604419ce05..73afff961115 100644
> --- a/src/gallium/drivers/nouveau/nouveau_buffer.c
> +++ b/src/gallium/drivers/nouveau/nouveau_buffer.c
> @@ -636,7 +636,8 @@ const struct u_resource_vtbl nouveau_buffer_vtbl =
>
> struct pipe_resource *
> nouveau_buffer_create(struct pipe_screen *pscreen,
> - const struct pipe_resource *templ)
> + const struct pipe_resource *templ,
> + const uint64_t *modifiers, unsigned int count)
> {
> struct nouveau_screen *screen = nouveau_screen(pscreen);
> struct nv04_resource *buffer;
> diff --git a/src/gallium/drivers/nouveau/nouveau_buffer.h
> b/src/gallium/drivers/nouveau/nouveau_buffer.h
> index 3a33fae9ce2f..466f8cc2b466 100644
> --- a/src/gallium/drivers/nouveau/nouveau_buffer.h
> +++ b/src/gallium/drivers/nouveau/nouveau_buffer.h
> @@ -89,7 +89,8 @@ nouveau_resource_mapped_by_gpu(struct pipe_resource
> *resource)
>
> struct pipe_resource *
> nouveau_buffer_create(struct pipe_screen *pscreen,
> - const struct pipe_resource *templ);
> + const struct pipe_resource *templ,
> + const uint64_t *modifiers, unsigned int count);
>
> struct pipe_resource *
> nouveau_user_buffer_create(struct pipe_screen *screen, void *ptr,
> diff --git a/src/gallium/drivers/nouveau/nouveau_screen.c
> b/src/gallium/drivers/nouveau/nouveau_screen.c
> index c144b39b2dd2..d651cc7f4b8c 100644
> --- a/src/gallium/drivers/nouveau/nouveau_screen.c
> +++ b/src/gallium/drivers/nouveau/nouveau_screen.c
> @@ -1,3 +1,5 @@
> +#include
> +
> #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"
> +
> /* XXX this should go away */
> #include "state_tracker/drm_driver.h"
>
> @@ -124,6 +128,15 @@ nouveau_screen_bo_from_handle(struct pipe_screen
> *pscreen,
> return bo;
> }
>
> +static uint64_t nouveau_bo_get_modifier(struct nouveau_bo *bo)
> +{
> + struct nouveau_device *dev = bo->device;
> +
> + if (dev->chipset >= 0xc0)
> + return nvc0_bo_get_modifier(bo);
> +
> + return DRM_FORMAT_MOD_INVALID;
> +}
>
> bool
> nouveau_screen_bo_get_handle(struct pipe_screen *pscreen,
> @@ -131,6 +144,7 @@ nouveau_screen_bo_get_handle(struct pipe_screen *pscreen,
> unsigned stride,
> struct winsys_handle *whandle)
> {
> + whandle->modifier = nouveau_bo_get_modifier(bo);
> whandle->stride = stride;
>
> if (whandle->type == DRM_API_HANDLE_TYPE_SHARED) {
> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_resource.c
> b/src/gallium/drivers/nouveau/nv30/nv30_resource.c
> index ff34f6e5f9fa..38d2b2e41c30 100644
> --- a/src/gallium/drivers/nouveau/nv30/nv30_resource.c
> +++ b/src/gallium/drivers/n
