[Mesa-dev] [PATCH 4/5] nouveau: Add framebuffer modifier support

[Thierry Reding]

On Wed, Feb 21, 2018 at 04:37:45PM +0000, Emil Velikov wrote:
> Hi Thierry,
> 
> On 21 February 2018 at 15:30, Thierry Reding <thierry.reding at gmail.com> 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 <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"
> > +
> > +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 <drm_fourcc.h>
> > +
> >  #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,
> > +                            uint64_t *modifiers, unsigned int *external_only,
> > +                            int *count)
> > +{
> > +}
> Add a TODO/WIP/EMPTY note with some brief info why it's empty?

Actually I think I'll just implement it. I don't recall why I left it
empty, but there's nothing here that sounds like it couldn't be easily
implemented.

Thanks for the review!

Thierry
-------------- 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/20180222/e5b38522/attachment.sig>