Nouveau - Feb 2020 - [PATCH 4/4] drm/nouveau: Remove struct nouveau_framebuffer

The format modifiers, when present, override the equivalent field in the 
BO.  Right now, that's probably not particularly useful.  However, as 
nouveau interfaces evolve towards the HW capabilities and add support 
for newer graphics APIs, saying an entire BO has a singular layout 
becomes less meaningful, so I suspect those fields will be effectively 
deprecated in favor of the modifier-defined and, for non-FB operations, 
strictly userspace defined layout.  Doing so will be much easier if the 
modifier support is already in place for applications to start making 
use of.

Thanks,
-James

On 2/6/20 7:28 AM, Roy Spliet wrote:
> (Re-sending to list rather than just to James)
> 
> Is this format modifier information not stored, or otherwise worth 
> storing, directly in the nouveau_bo object associated with the 
> framebuffer? I'm not particularly knowledgeable on the topic, but there
> already seem to exist some fields with very similar names in nouveau_bo.
> Best,
> 
> Roy
> 
> On 06/02/2020 15:17, James Jones wrote:
>> Note I'm adding some fields to nouveau_framebuffer in the series 
>> "drm/nouveau: Support NVIDIA format modifiers."? I sent out
v3 of that
>> yesterday.? It would probably still be possible to avoid them by 
>> re-extracting the relevant data from the format modifier on the fly 
>> when needed, but it is simpler and likely less error-prone with the 
>> wrapper struct.
>>
>> Thanks,
>> -James
>>
>> On 2/6/20 2:19 AM, Thomas Zimmermann wrote:
>>> After its cleanup, struct nouveau_framebuffer is only a wrapper
around
>>> struct drm_framebuffer. Use the latter directly.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>>> ---
>>> ? drivers/gpu/drm/nouveau/dispnv50/wndw.c?? | 26
+++++++++++------------
>>> ? drivers/gpu/drm/nouveau/nouveau_display.c | 14 ++++++------
>>> ? drivers/gpu/drm/nouveau/nouveau_display.h | 12 +----------
>>> ? drivers/gpu/drm/nouveau/nouveau_fbcon.c?? | 14 ++++++------
>>> ? 4 files changed, 28 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c 
>>> b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
>>> index ba1399965a1c..4a67a656e007 100644
>>> --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
>>> +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
>>> @@ -40,11 +40,11 @@ nv50_wndw_ctxdma_del(struct nv50_wndw_ctxdma 
>>> *ctxdma)
>>> ? }
>>> ? static struct nv50_wndw_ctxdma *
>>> -nv50_wndw_ctxdma_new(struct nv50_wndw *wndw, struct 
>>> nouveau_framebuffer *fb)
>>> +nv50_wndw_ctxdma_new(struct nv50_wndw *wndw, struct
drm_framebuffer
>>> *fb)
>>> ? {
>>> -??? struct nouveau_drm *drm = nouveau_drm(fb->base.dev);
>>> +??? struct nouveau_drm *drm = nouveau_drm(fb->dev);
>>> ????? struct nv50_wndw_ctxdma *ctxdma;
>>> -??? struct nouveau_bo *nvbo =
nouveau_gem_object(fb->base.obj[0]);
>>> +??? struct nouveau_bo *nvbo = nouveau_gem_object(fb->obj[0]);
>>> ????? const u8??? kind = nvbo->kind;
>>> ????? const u32 handle = 0xfb000000 | kind;
>>> ????? struct {
>>> @@ -236,16 +236,16 @@ nv50_wndw_atomic_check_acquire(struct
nv50_wndw
>>> *wndw, bool modeset,
>>> ???????????????????? struct nv50_wndw_atom *asyw,
>>> ???????????????????? struct nv50_head_atom *asyh)
>>> ? {
>>> -??? struct nouveau_framebuffer *fb = 
>>> nouveau_framebuffer(asyw->state.fb);
>>> +??? struct drm_framebuffer *fb = asyw->state.fb;
>>> ????? struct nouveau_drm *drm = nouveau_drm(wndw->plane.dev);
>>> -??? struct nouveau_bo *nvbo =
nouveau_gem_object(fb->base.obj[0]);
>>> +??? struct nouveau_bo *nvbo = nouveau_gem_object(fb->obj[0]);
>>> ????? int ret;
>>> ????? NV_ATOMIC(drm, "%s acquire\n",
wndw->plane.name);
>>> -??? if (asyw->state.fb != armw->state.fb ||
!armw->visible ||
>>> modeset) {
>>> -??????? asyw->image.w = fb->base.width;
>>> -??????? asyw->image.h = fb->base.height;
>>> +??? if (fb != armw->state.fb || !armw->visible || modeset) {
>>> +??????? asyw->image.w = fb->width;
>>> +??????? asyw->image.h = fb->height;
>>> ????????? asyw->image.kind = nvbo->kind;
>>> ????????? ret = nv50_wndw_atomic_check_acquire_rgb(asyw);
>>> @@ -261,13 +261,13 @@ nv50_wndw_atomic_check_acquire(struct
nv50_wndw
>>> *wndw, bool modeset,
>>> ????????????????? asyw->image.blockh = nvbo->mode >> 4;
>>> ????????????? else
>>> ????????????????? asyw->image.blockh = nvbo->mode;
>>> -??????????? asyw->image.blocks[0] = fb->base.pitches[0] /
64;
>>> +??????????? asyw->image.blocks[0] = fb->pitches[0] / 64;
>>> ????????????? asyw->image.pitch[0] = 0;
>>> ????????? } else {
>>> ????????????? asyw->image.layout = 1;
>>> ????????????? asyw->image.blockh = 0;
>>> ????????????? asyw->image.blocks[0] = 0;
>>> -??????????? asyw->image.pitch[0] = fb->base.pitches[0];
>>> +??????????? asyw->image.pitch[0] = fb->pitches[0];
>>> ????????? }
>>> ????????? if (!asyh->state.async_flip)
>>> @@ -486,16 +486,16 @@ nv50_wndw_cleanup_fb(struct drm_plane *plane,
>>> struct drm_plane_state *old_state)
>>> ? static int
>>> ? nv50_wndw_prepare_fb(struct drm_plane *plane, struct 
>>> drm_plane_state *state)
>>> ? {
>>> -??? struct nouveau_framebuffer *fb =
nouveau_framebuffer(state->fb);
>>> +??? struct drm_framebuffer *fb = state->fb;
>>> ????? struct nouveau_drm *drm = nouveau_drm(plane->dev);
>>> ????? struct nv50_wndw *wndw = nv50_wndw(plane);
>>> ????? struct nv50_wndw_atom *asyw = nv50_wndw_atom(state);
>>> -??? struct nouveau_bo *nvbo =
nouveau_gem_object(state->fb->obj[0]);
>>> +??? struct nouveau_bo *nvbo = nouveau_gem_object(fb->obj[0]);
>>> ????? struct nv50_head_atom *asyh;
>>> ????? struct nv50_wndw_ctxdma *ctxdma;
>>> ????? int ret;
>>> -??? NV_ATOMIC(drm, "%s prepare: %p\n", plane->name,
state->fb);
>>> +??? NV_ATOMIC(drm, "%s prepare: %p\n", plane->name,
fb);
>>> ????? if (!asyw->state.fb)
>>> ????????? return 0;
>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c 
>>> b/drivers/gpu/drm/nouveau/nouveau_display.c
>>> index bbbff55eb5d5..94f7fd48e1cf 100644
>>> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
>>> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
>>> @@ -207,10 +207,10 @@ int
>>> ? nouveau_framebuffer_new(struct drm_device *dev,
>>> ????????????? const struct drm_mode_fb_cmd2 *mode_cmd,
>>> ????????????? struct drm_gem_object *gem,
>>> -??????????? struct nouveau_framebuffer **pfb)
>>> +??????????? struct drm_framebuffer **pfb)
>>> ? {
>>> ????? struct nouveau_drm *drm = nouveau_drm(dev);
>>> -??? struct nouveau_framebuffer *fb;
>>> +??? struct drm_framebuffer *fb;
>>> ????? int ret;
>>> ????????? /* YUV overlays have special requirements pre-NV50 */
>>> @@ -236,10 +236,10 @@ nouveau_framebuffer_new(struct drm_device
*dev,
>>> ????? if (!(fb = *pfb = kzalloc(sizeof(*fb), GFP_KERNEL)))
>>> ????????? return -ENOMEM;
>>> -??? drm_helper_mode_fill_fb_struct(dev, &fb->base,
mode_cmd);
>>> -??? fb->base.obj[0] = gem;
>>> +??? drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
>>> +??? fb->obj[0] = gem;
>>> -??? ret = drm_framebuffer_init(dev, &fb->base, 
>>> &nouveau_framebuffer_funcs);
>>> +??? ret = drm_framebuffer_init(dev, fb,
&nouveau_framebuffer_funcs);
>>> ????? if (ret)
>>> ????????? kfree(fb);
>>> ????? return ret;
>>> @@ -250,7 +250,7 @@ nouveau_user_framebuffer_create(struct
drm_device
>>> *dev,
>>> ????????????????? struct drm_file *file_priv,
>>> ????????????????? const struct drm_mode_fb_cmd2 *mode_cmd)
>>> ? {
>>> -??? struct nouveau_framebuffer *fb;
>>> +??? struct drm_framebuffer *fb;
>>> ????? struct drm_gem_object *gem;
>>> ????? int ret;
>>> @@ -260,7 +260,7 @@ nouveau_user_framebuffer_create(struct
drm_device
>>> *dev,
>>> ????? ret = nouveau_framebuffer_new(dev, mode_cmd, gem, &fb);
>>> ????? if (ret == 0)
>>> -??????? return &fb->base;
>>> +??????? return fb;
>>> ????? drm_gem_object_put_unlocked(gem);
>>> ????? return ERR_PTR(ret);
>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h 
>>> b/drivers/gpu/drm/nouveau/nouveau_display.h
>>> index 56c1dec8fc28..082bb067d575 100644
>>> --- a/drivers/gpu/drm/nouveau/nouveau_display.h
>>> +++ b/drivers/gpu/drm/nouveau/nouveau_display.h
>>> @@ -8,21 +8,11 @@
>>> ? #include <drm/drm_framebuffer.h>
>>> -struct nouveau_framebuffer {
>>> -??? struct drm_framebuffer base;
>>> -};
>>> -
>>> -static inline struct nouveau_framebuffer *
>>> -nouveau_framebuffer(struct drm_framebuffer *fb)
>>> -{
>>> -??? return container_of(fb, struct nouveau_framebuffer, base);
>>> -}
>>> -
>>> ? int
>>> ? nouveau_framebuffer_new(struct drm_device *dev,
>>> ????????????? const struct drm_mode_fb_cmd2 *mode_cmd,
>>> ????????????? struct drm_gem_object *gem,
>>> -??????????? struct nouveau_framebuffer **pfb);
>>> +??????????? struct drm_framebuffer **pfb);
>>> ? struct nouveau_display {
>>> ????? void *priv;
>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c 
>>> b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
>>> index 02b36b44409c..d78bc03ad3b8 100644
>>> --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
>>> +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
>>> @@ -312,7 +312,7 @@ nouveau_fbcon_create(struct drm_fb_helper
*helper,
>>> ????? struct nouveau_drm *drm = nouveau_drm(dev);
>>> ????? struct nvif_device *device = &drm->client.device;
>>> ????? struct fb_info *info;
>>> -??? struct nouveau_framebuffer *fb;
>>> +??? struct drm_framebuffer *fb;
>>> ????? struct nouveau_channel *chan;
>>> ????? struct nouveau_bo *nvbo;
>>> ????? struct drm_mode_fb_cmd2 mode_cmd;
>>> @@ -367,7 +367,7 @@ nouveau_fbcon_create(struct drm_fb_helper
*helper,
>>> ????? }
>>> ????? /* setup helper */
>>> -??? fbcon->helper.fb = &fb->base;
>>> +??? fbcon->helper.fb = fb;
>>> ????? if (!chan)
>>> ????????? info->flags = FBINFO_HWACCEL_DISABLED;
>>> @@ -393,7 +393,7 @@ nouveau_fbcon_create(struct drm_fb_helper
*helper,
>>> ????? /* To allow resizeing without swapping buffers */
>>> ????? NV_INFO(drm, "allocated %dx%d fb: 0x%llx, bo %p\n",
>>> -??????? fb->base.width, fb->base.height, nvbo->bo.offset,
nvbo);
>>> +??????? fb->width, fb->height, nvbo->bo.offset, nvbo);
>>> ????? vga_switcheroo_client_fb_set(dev->pdev, info);
>>> ????? return 0;
>>> @@ -413,18 +413,18 @@ nouveau_fbcon_create(struct drm_fb_helper
*helper,
>>> ? static int
>>> ? nouveau_fbcon_destroy(struct drm_device *dev, struct
nouveau_fbdev
>>> *fbcon)
>>> ? {
>>> -??? struct nouveau_framebuffer *nouveau_fb = 
>>> nouveau_framebuffer(fbcon->helper.fb);
>>> +??? struct drm_framebuffer *fb = fbcon->helper.fb;
>>> ????? struct nouveau_bo *nvbo;
>>> ????? drm_fb_helper_unregister_fbi(&fbcon->helper);
>>> ????? drm_fb_helper_fini(&fbcon->helper);
>>> -??? if (nouveau_fb && nouveau_fb->base.obj[0]) {
>>> -??????? nvbo = nouveau_gem_object(nouveau_fb->base.obj[0]);
>>> +??? if (fb && fb->obj[0]) {
>>> +??????? nvbo = nouveau_gem_object(fb->obj[0]);
>>> ????????? nouveau_vma_del(&fbcon->vma);
>>> ????????? nouveau_bo_unmap(nvbo);
>>> ????????? nouveau_bo_unpin(nvbo);
>>> -??????? drm_framebuffer_put(&nouveau_fb->base);
>>> +??????? drm_framebuffer_put(fb);
>>> ????? }
>>> ????? return 0;
>>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau

So ... for Vulkan, the API requires allocating VA before declaring
what goes into that VA (e.g. an image or data). I believe our plan for
that was to move all this into userspace, which would allocate VA and
then just tell the kernel "make VA range X have memtype Y". At that
point, nouveau_bo would be left mainly for compat as well as for
things like framebuffer backing.

James, in what situation would the modifier be different than the bo's
memtype?

  -ilia

On Thu, Feb 6, 2020 at 10:43 AM James Jones <jajones at nvidia.com>
wrote:
>
> The format modifiers, when present, override the equivalent field in the
> BO.  Right now, that's probably not particularly useful.  However, as
> nouveau interfaces evolve towards the HW capabilities and add support
> for newer graphics APIs, saying an entire BO has a singular layout
> becomes less meaningful, so I suspect those fields will be effectively
> deprecated in favor of the modifier-defined and, for non-FB operations,
> strictly userspace defined layout.  Doing so will be much easier if the
> modifier support is already in place for applications to start making
> use of.
>
> Thanks,
> -James
>
> On 2/6/20 7:28 AM, Roy Spliet wrote:
> > (Re-sending to list rather than just to James)
> >
> > Is this format modifier information not stored, or otherwise worth
> > storing, directly in the nouveau_bo object associated with the
> > framebuffer? I'm not particularly knowledgeable on the topic, but
there
> > already seem to exist some fields with very similar names in
nouveau_bo.
> > Best,
> >
> > Roy
> >
> > On 06/02/2020 15:17, James Jones wrote:
> >> Note I'm adding some fields to nouveau_framebuffer in the
series
> >> "drm/nouveau: Support NVIDIA format modifiers."  I sent
out v3 of that
> >> yesterday.  It would probably still be possible to avoid them by
> >> re-extracting the relevant data from the format modifier on the
fly
> >> when needed, but it is simpler and likely less error-prone with
the
> >> wrapper struct.
> >>
> >> Thanks,
> >> -James
> >>
> >> On 2/6/20 2:19 AM, Thomas Zimmermann wrote:
> >>> After its cleanup, struct nouveau_framebuffer is only a
wrapper around
> >>> struct drm_framebuffer. Use the latter directly.
> >>>
> >>> Signed-off-by: Thomas Zimmermann <tzimmermann at
suse.de>
> >>> ---
> >>>   drivers/gpu/drm/nouveau/dispnv50/wndw.c   | 26
+++++++++++------------
> >>>   drivers/gpu/drm/nouveau/nouveau_display.c | 14 ++++++------
> >>>   drivers/gpu/drm/nouveau/nouveau_display.h | 12 +----------
> >>>   drivers/gpu/drm/nouveau/nouveau_fbcon.c   | 14 ++++++------
> >>>   4 files changed, 28 insertions(+), 38 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> >>> b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> >>> index ba1399965a1c..4a67a656e007 100644
> >>> --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> >>> +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> >>> @@ -40,11 +40,11 @@ nv50_wndw_ctxdma_del(struct
nv50_wndw_ctxdma
> >>> *ctxdma)
> >>>   }
> >>>   static struct nv50_wndw_ctxdma *
> >>> -nv50_wndw_ctxdma_new(struct nv50_wndw *wndw, struct
> >>> nouveau_framebuffer *fb)
> >>> +nv50_wndw_ctxdma_new(struct nv50_wndw *wndw, struct
drm_framebuffer
> >>> *fb)
> >>>   {
> >>> -    struct nouveau_drm *drm = nouveau_drm(fb->base.dev);
> >>> +    struct nouveau_drm *drm = nouveau_drm(fb->dev);
> >>>       struct nv50_wndw_ctxdma *ctxdma;
> >>> -    struct nouveau_bo *nvbo =
nouveau_gem_object(fb->base.obj[0]);
> >>> +    struct nouveau_bo *nvbo =
nouveau_gem_object(fb->obj[0]);
> >>>       const u8    kind = nvbo->kind;
> >>>       const u32 handle = 0xfb000000 | kind;
> >>>       struct {
> >>> @@ -236,16 +236,16 @@ nv50_wndw_atomic_check_acquire(struct
nv50_wndw
> >>> *wndw, bool modeset,
> >>>                      struct nv50_wndw_atom *asyw,
> >>>                      struct nv50_head_atom *asyh)
> >>>   {
> >>> -    struct nouveau_framebuffer *fb > >>>
nouveau_framebuffer(asyw->state.fb);
> >>> +    struct drm_framebuffer *fb = asyw->state.fb;
> >>>       struct nouveau_drm *drm =
nouveau_drm(wndw->plane.dev);
> >>> -    struct nouveau_bo *nvbo =
nouveau_gem_object(fb->base.obj[0]);
> >>> +    struct nouveau_bo *nvbo =
nouveau_gem_object(fb->obj[0]);
> >>>       int ret;
> >>>       NV_ATOMIC(drm, "%s acquire\n",
wndw->plane.name);
> >>> -    if (asyw->state.fb != armw->state.fb ||
!armw->visible ||
> >>> modeset) {
> >>> -        asyw->image.w = fb->base.width;
> >>> -        asyw->image.h = fb->base.height;
> >>> +    if (fb != armw->state.fb || !armw->visible ||
modeset) {
> >>> +        asyw->image.w = fb->width;
> >>> +        asyw->image.h = fb->height;
> >>>           asyw->image.kind = nvbo->kind;
> >>>           ret = nv50_wndw_atomic_check_acquire_rgb(asyw);
> >>> @@ -261,13 +261,13 @@ nv50_wndw_atomic_check_acquire(struct
nv50_wndw
> >>> *wndw, bool modeset,
> >>>                   asyw->image.blockh = nvbo->mode
>> 4;
> >>>               else
> >>>                   asyw->image.blockh = nvbo->mode;
> >>> -            asyw->image.blocks[0] = fb->base.pitches[0]
/ 64;
> >>> +            asyw->image.blocks[0] = fb->pitches[0] /
64;
> >>>               asyw->image.pitch[0] = 0;
> >>>           } else {
> >>>               asyw->image.layout = 1;
> >>>               asyw->image.blockh = 0;
> >>>               asyw->image.blocks[0] = 0;
> >>> -            asyw->image.pitch[0] = fb->base.pitches[0];
> >>> +            asyw->image.pitch[0] = fb->pitches[0];
> >>>           }
> >>>           if (!asyh->state.async_flip)
> >>> @@ -486,16 +486,16 @@ nv50_wndw_cleanup_fb(struct drm_plane
*plane,
> >>> struct drm_plane_state *old_state)
> >>>   static int
> >>>   nv50_wndw_prepare_fb(struct drm_plane *plane, struct
> >>> drm_plane_state *state)
> >>>   {
> >>> -    struct nouveau_framebuffer *fb =
nouveau_framebuffer(state->fb);
> >>> +    struct drm_framebuffer *fb = state->fb;
> >>>       struct nouveau_drm *drm = nouveau_drm(plane->dev);
> >>>       struct nv50_wndw *wndw = nv50_wndw(plane);
> >>>       struct nv50_wndw_atom *asyw = nv50_wndw_atom(state);
> >>> -    struct nouveau_bo *nvbo =
nouveau_gem_object(state->fb->obj[0]);
> >>> +    struct nouveau_bo *nvbo =
nouveau_gem_object(fb->obj[0]);
> >>>       struct nv50_head_atom *asyh;
> >>>       struct nv50_wndw_ctxdma *ctxdma;
> >>>       int ret;
> >>> -    NV_ATOMIC(drm, "%s prepare: %p\n",
plane->name, state->fb);
> >>> +    NV_ATOMIC(drm, "%s prepare: %p\n",
plane->name, fb);
> >>>       if (!asyw->state.fb)
> >>>           return 0;
> >>> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c
> >>> b/drivers/gpu/drm/nouveau/nouveau_display.c
> >>> index bbbff55eb5d5..94f7fd48e1cf 100644
> >>> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> >>> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> >>> @@ -207,10 +207,10 @@ int
> >>>   nouveau_framebuffer_new(struct drm_device *dev,
> >>>               const struct drm_mode_fb_cmd2 *mode_cmd,
> >>>               struct drm_gem_object *gem,
> >>> -            struct nouveau_framebuffer **pfb)
> >>> +            struct drm_framebuffer **pfb)
> >>>   {
> >>>       struct nouveau_drm *drm = nouveau_drm(dev);
> >>> -    struct nouveau_framebuffer *fb;
> >>> +    struct drm_framebuffer *fb;
> >>>       int ret;
> >>>           /* YUV overlays have special requirements pre-NV50
*/
> >>> @@ -236,10 +236,10 @@ nouveau_framebuffer_new(struct
drm_device *dev,
> >>>       if (!(fb = *pfb = kzalloc(sizeof(*fb), GFP_KERNEL)))
> >>>           return -ENOMEM;
> >>> -    drm_helper_mode_fill_fb_struct(dev, &fb->base,
mode_cmd);
> >>> -    fb->base.obj[0] = gem;
> >>> +    drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
> >>> +    fb->obj[0] = gem;
> >>> -    ret = drm_framebuffer_init(dev, &fb->base,
> >>> &nouveau_framebuffer_funcs);
> >>> +    ret = drm_framebuffer_init(dev, fb,
&nouveau_framebuffer_funcs);
> >>>       if (ret)
> >>>           kfree(fb);
> >>>       return ret;
> >>> @@ -250,7 +250,7 @@ nouveau_user_framebuffer_create(struct
drm_device
> >>> *dev,
> >>>                   struct drm_file *file_priv,
> >>>                   const struct drm_mode_fb_cmd2 *mode_cmd)
> >>>   {
> >>> -    struct nouveau_framebuffer *fb;
> >>> +    struct drm_framebuffer *fb;
> >>>       struct drm_gem_object *gem;
> >>>       int ret;
> >>> @@ -260,7 +260,7 @@ nouveau_user_framebuffer_create(struct
drm_device
> >>> *dev,
> >>>       ret = nouveau_framebuffer_new(dev, mode_cmd, gem,
&fb);
> >>>       if (ret == 0)
> >>> -        return &fb->base;
> >>> +        return fb;
> >>>       drm_gem_object_put_unlocked(gem);
> >>>       return ERR_PTR(ret);
> >>> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h
> >>> b/drivers/gpu/drm/nouveau/nouveau_display.h
> >>> index 56c1dec8fc28..082bb067d575 100644
> >>> --- a/drivers/gpu/drm/nouveau/nouveau_display.h
> >>> +++ b/drivers/gpu/drm/nouveau/nouveau_display.h
> >>> @@ -8,21 +8,11 @@
> >>>   #include <drm/drm_framebuffer.h>
> >>> -struct nouveau_framebuffer {
> >>> -    struct drm_framebuffer base;
> >>> -};
> >>> -
> >>> -static inline struct nouveau_framebuffer *
> >>> -nouveau_framebuffer(struct drm_framebuffer *fb)
> >>> -{
> >>> -    return container_of(fb, struct nouveau_framebuffer,
base);
> >>> -}
> >>> -
> >>>   int
> >>>   nouveau_framebuffer_new(struct drm_device *dev,
> >>>               const struct drm_mode_fb_cmd2 *mode_cmd,
> >>>               struct drm_gem_object *gem,
> >>> -            struct nouveau_framebuffer **pfb);
> >>> +            struct drm_framebuffer **pfb);
> >>>   struct nouveau_display {
> >>>       void *priv;
> >>> diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> >>> b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> >>> index 02b36b44409c..d78bc03ad3b8 100644
> >>> --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> >>> +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> >>> @@ -312,7 +312,7 @@ nouveau_fbcon_create(struct drm_fb_helper
*helper,
> >>>       struct nouveau_drm *drm = nouveau_drm(dev);
> >>>       struct nvif_device *device = &drm->client.device;
> >>>       struct fb_info *info;
> >>> -    struct nouveau_framebuffer *fb;
> >>> +    struct drm_framebuffer *fb;
> >>>       struct nouveau_channel *chan;
> >>>       struct nouveau_bo *nvbo;
> >>>       struct drm_mode_fb_cmd2 mode_cmd;
> >>> @@ -367,7 +367,7 @@ nouveau_fbcon_create(struct drm_fb_helper
*helper,
> >>>       }
> >>>       /* setup helper */
> >>> -    fbcon->helper.fb = &fb->base;
> >>> +    fbcon->helper.fb = fb;
> >>>       if (!chan)
> >>>           info->flags = FBINFO_HWACCEL_DISABLED;
> >>> @@ -393,7 +393,7 @@ nouveau_fbcon_create(struct drm_fb_helper
*helper,
> >>>       /* To allow resizeing without swapping buffers */
> >>>       NV_INFO(drm, "allocated %dx%d fb: 0x%llx, bo
%p\n",
> >>> -        fb->base.width, fb->base.height,
nvbo->bo.offset, nvbo);
> >>> +        fb->width, fb->height, nvbo->bo.offset,
nvbo);
> >>>       vga_switcheroo_client_fb_set(dev->pdev, info);
> >>>       return 0;
> >>> @@ -413,18 +413,18 @@ nouveau_fbcon_create(struct
drm_fb_helper *helper,
> >>>   static int
> >>>   nouveau_fbcon_destroy(struct drm_device *dev, struct
nouveau_fbdev
> >>> *fbcon)
> >>>   {
> >>> -    struct nouveau_framebuffer *nouveau_fb > >>>
nouveau_framebuffer(fbcon->helper.fb);
> >>> +    struct drm_framebuffer *fb = fbcon->helper.fb;
> >>>       struct nouveau_bo *nvbo;
> >>>       drm_fb_helper_unregister_fbi(&fbcon->helper);
> >>>       drm_fb_helper_fini(&fbcon->helper);
> >>> -    if (nouveau_fb && nouveau_fb->base.obj[0]) {
> >>> -        nvbo =
nouveau_gem_object(nouveau_fb->base.obj[0]);
> >>> +    if (fb && fb->obj[0]) {
> >>> +        nvbo = nouveau_gem_object(fb->obj[0]);
> >>>           nouveau_vma_del(&fbcon->vma);
> >>>           nouveau_bo_unmap(nvbo);
> >>>           nouveau_bo_unpin(nvbo);
> >>> -        drm_framebuffer_put(&nouveau_fb->base);
> >>> +        drm_framebuffer_put(fb);
> >>>       }
> >>>       return 0;
> >>>
> >> _______________________________________________
> >> Nouveau mailing list
> >> Nouveau at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/nouveau
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

What object would ultimately provide the physical backing for the VA you 
described below?  I assumed it would be a bo.  E.g., a Vulkan 
application creates or imports some image backed by some memory that 
gets represented as a bo in the kernel, then creates a framebuffer on 
that bo with a layout corresponding to the Vulkan usage to scan that 
image out.  Since only userspace would necessarily need to be aware of 
the actual layout at that point, it would use a modifier to communicate 
that layout to the kernel when creating the FB.

It's probably not something that will be done as commonly, but 
applications could do the same thing with EGLImage + modifiers in GL, at 
least at the API level and I believe on other vendors' Mesa drivers.

Thanks,
-James

On 2/6/20 7:47 AM, Ilia Mirkin wrote:
> So ... for Vulkan, the API requires allocating VA before declaring
> what goes into that VA (e.g. an image or data). I believe our plan for
> that was to move all this into userspace, which would allocate VA and
> then just tell the kernel "make VA range X have memtype Y". At
that
> point, nouveau_bo would be left mainly for compat as well as for
> things like framebuffer backing.
> 
> James, in what situation would the modifier be different than the bo's
memtype?
> 
>    -ilia
> 
> On Thu, Feb 6, 2020 at 10:43 AM James Jones <jajones at nvidia.com>
wrote:
>>
>> The format modifiers, when present, override the equivalent field in
the
>> BO.  Right now, that's probably not particularly useful.  However,
as
>> nouveau interfaces evolve towards the HW capabilities and add support
>> for newer graphics APIs, saying an entire BO has a singular layout
>> becomes less meaningful, so I suspect those fields will be effectively
>> deprecated in favor of the modifier-defined and, for non-FB operations,
>> strictly userspace defined layout.  Doing so will be much easier if the
>> modifier support is already in place for applications to start making
>> use of.
>>
>> Thanks,
>> -James
>>
>> On 2/6/20 7:28 AM, Roy Spliet wrote:
>>> (Re-sending to list rather than just to James)
>>>
>>> Is this format modifier information not stored, or otherwise worth
>>> storing, directly in the nouveau_bo object associated with the
>>> framebuffer? I'm not particularly knowledgeable on the topic,
but there
>>> already seem to exist some fields with very similar names in
nouveau_bo.
>>> Best,
>>>
>>> Roy
>>>
>>> On 06/02/2020 15:17, James Jones wrote:
>>>> Note I'm adding some fields to nouveau_framebuffer in the
series
>>>> "drm/nouveau: Support NVIDIA format modifiers."  I
sent out v3 of that
>>>> yesterday.  It would probably still be possible to avoid them
by
>>>> re-extracting the relevant data from the format modifier on the
fly
>>>> when needed, but it is simpler and likely less error-prone with
the
>>>> wrapper struct.
>>>>
>>>> Thanks,
>>>> -James
>>>>
>>>> On 2/6/20 2:19 AM, Thomas Zimmermann wrote:
>>>>> After its cleanup, struct nouveau_framebuffer is only a
wrapper around
>>>>> struct drm_framebuffer. Use the latter directly.
>>>>>
>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann at
suse.de>
>>>>> ---
>>>>>    drivers/gpu/drm/nouveau/dispnv50/wndw.c   | 26
+++++++++++------------
>>>>>    drivers/gpu/drm/nouveau/nouveau_display.c | 14
++++++------
>>>>>    drivers/gpu/drm/nouveau/nouveau_display.h | 12
+----------
>>>>>    drivers/gpu/drm/nouveau/nouveau_fbcon.c   | 14
++++++------
>>>>>    4 files changed, 28 insertions(+), 38 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
>>>>> b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
>>>>> index ba1399965a1c..4a67a656e007 100644
>>>>> --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
>>>>> +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
>>>>> @@ -40,11 +40,11 @@ nv50_wndw_ctxdma_del(struct
nv50_wndw_ctxdma
>>>>> *ctxdma)
>>>>>    }
>>>>>    static struct nv50_wndw_ctxdma *
>>>>> -nv50_wndw_ctxdma_new(struct nv50_wndw *wndw, struct
>>>>> nouveau_framebuffer *fb)
>>>>> +nv50_wndw_ctxdma_new(struct nv50_wndw *wndw, struct
drm_framebuffer
>>>>> *fb)
>>>>>    {
>>>>> -    struct nouveau_drm *drm =
nouveau_drm(fb->base.dev);
>>>>> +    struct nouveau_drm *drm = nouveau_drm(fb->dev);
>>>>>        struct nv50_wndw_ctxdma *ctxdma;
>>>>> -    struct nouveau_bo *nvbo =
nouveau_gem_object(fb->base.obj[0]);
>>>>> +    struct nouveau_bo *nvbo =
nouveau_gem_object(fb->obj[0]);
>>>>>        const u8    kind = nvbo->kind;
>>>>>        const u32 handle = 0xfb000000 | kind;
>>>>>        struct {
>>>>> @@ -236,16 +236,16 @@ nv50_wndw_atomic_check_acquire(struct
nv50_wndw
>>>>> *wndw, bool modeset,
>>>>>                       struct nv50_wndw_atom *asyw,
>>>>>                       struct nv50_head_atom *asyh)
>>>>>    {
>>>>> -    struct nouveau_framebuffer *fb >>>>>
nouveau_framebuffer(asyw->state.fb);
>>>>> +    struct drm_framebuffer *fb = asyw->state.fb;
>>>>>        struct nouveau_drm *drm =
nouveau_drm(wndw->plane.dev);
>>>>> -    struct nouveau_bo *nvbo =
nouveau_gem_object(fb->base.obj[0]);
>>>>> +    struct nouveau_bo *nvbo =
nouveau_gem_object(fb->obj[0]);
>>>>>        int ret;
>>>>>        NV_ATOMIC(drm, "%s acquire\n",
wndw->plane.name);
>>>>> -    if (asyw->state.fb != armw->state.fb ||
!armw->visible ||
>>>>> modeset) {
>>>>> -        asyw->image.w = fb->base.width;
>>>>> -        asyw->image.h = fb->base.height;
>>>>> +    if (fb != armw->state.fb || !armw->visible ||
modeset) {
>>>>> +        asyw->image.w = fb->width;
>>>>> +        asyw->image.h = fb->height;
>>>>>            asyw->image.kind = nvbo->kind;
>>>>>            ret = nv50_wndw_atomic_check_acquire_rgb(asyw);
>>>>> @@ -261,13 +261,13 @@ nv50_wndw_atomic_check_acquire(struct
nv50_wndw
>>>>> *wndw, bool modeset,
>>>>>                    asyw->image.blockh = nvbo->mode
>> 4;
>>>>>                else
>>>>>                    asyw->image.blockh = nvbo->mode;
>>>>> -            asyw->image.blocks[0] =
fb->base.pitches[0] / 64;
>>>>> +            asyw->image.blocks[0] = fb->pitches[0] /
64;
>>>>>                asyw->image.pitch[0] = 0;
>>>>>            } else {
>>>>>                asyw->image.layout = 1;
>>>>>                asyw->image.blockh = 0;
>>>>>                asyw->image.blocks[0] = 0;
>>>>> -            asyw->image.pitch[0] =
fb->base.pitches[0];
>>>>> +            asyw->image.pitch[0] = fb->pitches[0];
>>>>>            }
>>>>>            if (!asyh->state.async_flip)
>>>>> @@ -486,16 +486,16 @@ nv50_wndw_cleanup_fb(struct drm_plane
*plane,
>>>>> struct drm_plane_state *old_state)
>>>>>    static int
>>>>>    nv50_wndw_prepare_fb(struct drm_plane *plane, struct
>>>>> drm_plane_state *state)
>>>>>    {
>>>>> -    struct nouveau_framebuffer *fb =
nouveau_framebuffer(state->fb);
>>>>> +    struct drm_framebuffer *fb = state->fb;
>>>>>        struct nouveau_drm *drm =
nouveau_drm(plane->dev);
>>>>>        struct nv50_wndw *wndw = nv50_wndw(plane);
>>>>>        struct nv50_wndw_atom *asyw = nv50_wndw_atom(state);
>>>>> -    struct nouveau_bo *nvbo =
nouveau_gem_object(state->fb->obj[0]);
>>>>> +    struct nouveau_bo *nvbo =
nouveau_gem_object(fb->obj[0]);
>>>>>        struct nv50_head_atom *asyh;
>>>>>        struct nv50_wndw_ctxdma *ctxdma;
>>>>>        int ret;
>>>>> -    NV_ATOMIC(drm, "%s prepare: %p\n",
plane->name, state->fb);
>>>>> +    NV_ATOMIC(drm, "%s prepare: %p\n",
plane->name, fb);
>>>>>        if (!asyw->state.fb)
>>>>>            return 0;
>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c
>>>>> b/drivers/gpu/drm/nouveau/nouveau_display.c
>>>>> index bbbff55eb5d5..94f7fd48e1cf 100644
>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
>>>>> @@ -207,10 +207,10 @@ int
>>>>>    nouveau_framebuffer_new(struct drm_device *dev,
>>>>>                const struct drm_mode_fb_cmd2 *mode_cmd,
>>>>>                struct drm_gem_object *gem,
>>>>> -            struct nouveau_framebuffer **pfb)
>>>>> +            struct drm_framebuffer **pfb)
>>>>>    {
>>>>>        struct nouveau_drm *drm = nouveau_drm(dev);
>>>>> -    struct nouveau_framebuffer *fb;
>>>>> +    struct drm_framebuffer *fb;
>>>>>        int ret;
>>>>>            /* YUV overlays have special requirements
pre-NV50 */
>>>>> @@ -236,10 +236,10 @@ nouveau_framebuffer_new(struct
drm_device *dev,
>>>>>        if (!(fb = *pfb = kzalloc(sizeof(*fb), GFP_KERNEL)))
>>>>>            return -ENOMEM;
>>>>> -    drm_helper_mode_fill_fb_struct(dev, &fb->base,
mode_cmd);
>>>>> -    fb->base.obj[0] = gem;
>>>>> +    drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
>>>>> +    fb->obj[0] = gem;
>>>>> -    ret = drm_framebuffer_init(dev, &fb->base,
>>>>> &nouveau_framebuffer_funcs);
>>>>> +    ret = drm_framebuffer_init(dev, fb,
&nouveau_framebuffer_funcs);
>>>>>        if (ret)
>>>>>            kfree(fb);
>>>>>        return ret;
>>>>> @@ -250,7 +250,7 @@ nouveau_user_framebuffer_create(struct
drm_device
>>>>> *dev,
>>>>>                    struct drm_file *file_priv,
>>>>>                    const struct drm_mode_fb_cmd2 *mode_cmd)
>>>>>    {
>>>>> -    struct nouveau_framebuffer *fb;
>>>>> +    struct drm_framebuffer *fb;
>>>>>        struct drm_gem_object *gem;
>>>>>        int ret;
>>>>> @@ -260,7 +260,7 @@ nouveau_user_framebuffer_create(struct
drm_device
>>>>> *dev,
>>>>>        ret = nouveau_framebuffer_new(dev, mode_cmd, gem,
&fb);
>>>>>        if (ret == 0)
>>>>> -        return &fb->base;
>>>>> +        return fb;
>>>>>        drm_gem_object_put_unlocked(gem);
>>>>>        return ERR_PTR(ret);
>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h
>>>>> b/drivers/gpu/drm/nouveau/nouveau_display.h
>>>>> index 56c1dec8fc28..082bb067d575 100644
>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_display.h
>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_display.h
>>>>> @@ -8,21 +8,11 @@
>>>>>    #include <drm/drm_framebuffer.h>
>>>>> -struct nouveau_framebuffer {
>>>>> -    struct drm_framebuffer base;
>>>>> -};
>>>>> -
>>>>> -static inline struct nouveau_framebuffer *
>>>>> -nouveau_framebuffer(struct drm_framebuffer *fb)
>>>>> -{
>>>>> -    return container_of(fb, struct nouveau_framebuffer,
base);
>>>>> -}
>>>>> -
>>>>>    int
>>>>>    nouveau_framebuffer_new(struct drm_device *dev,
>>>>>                const struct drm_mode_fb_cmd2 *mode_cmd,
>>>>>                struct drm_gem_object *gem,
>>>>> -            struct nouveau_framebuffer **pfb);
>>>>> +            struct drm_framebuffer **pfb);
>>>>>    struct nouveau_display {
>>>>>        void *priv;
>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
>>>>> b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
>>>>> index 02b36b44409c..d78bc03ad3b8 100644
>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
>>>>> @@ -312,7 +312,7 @@ nouveau_fbcon_create(struct
drm_fb_helper *helper,
>>>>>        struct nouveau_drm *drm = nouveau_drm(dev);
>>>>>        struct nvif_device *device =
&drm->client.device;
>>>>>        struct fb_info *info;
>>>>> -    struct nouveau_framebuffer *fb;
>>>>> +    struct drm_framebuffer *fb;
>>>>>        struct nouveau_channel *chan;
>>>>>        struct nouveau_bo *nvbo;
>>>>>        struct drm_mode_fb_cmd2 mode_cmd;
>>>>> @@ -367,7 +367,7 @@ nouveau_fbcon_create(struct
drm_fb_helper *helper,
>>>>>        }
>>>>>        /* setup helper */
>>>>> -    fbcon->helper.fb = &fb->base;
>>>>> +    fbcon->helper.fb = fb;
>>>>>        if (!chan)
>>>>>            info->flags = FBINFO_HWACCEL_DISABLED;
>>>>> @@ -393,7 +393,7 @@ nouveau_fbcon_create(struct
drm_fb_helper *helper,
>>>>>        /* To allow resizeing without swapping buffers */
>>>>>        NV_INFO(drm, "allocated %dx%d fb: 0x%llx, bo
%p\n",
>>>>> -        fb->base.width, fb->base.height,
nvbo->bo.offset, nvbo);
>>>>> +        fb->width, fb->height, nvbo->bo.offset,
nvbo);
>>>>>        vga_switcheroo_client_fb_set(dev->pdev, info);
>>>>>        return 0;
>>>>> @@ -413,18 +413,18 @@ nouveau_fbcon_create(struct
drm_fb_helper *helper,
>>>>>    static int
>>>>>    nouveau_fbcon_destroy(struct drm_device *dev, struct
nouveau_fbdev
>>>>> *fbcon)
>>>>>    {
>>>>> -    struct nouveau_framebuffer *nouveau_fb
>>>>> nouveau_framebuffer(fbcon->helper.fb);
>>>>> +    struct drm_framebuffer *fb = fbcon->helper.fb;
>>>>>        struct nouveau_bo *nvbo;
>>>>>        drm_fb_helper_unregister_fbi(&fbcon->helper);
>>>>>        drm_fb_helper_fini(&fbcon->helper);
>>>>> -    if (nouveau_fb && nouveau_fb->base.obj[0])
{
>>>>> -        nvbo =
nouveau_gem_object(nouveau_fb->base.obj[0]);
>>>>> +    if (fb && fb->obj[0]) {
>>>>> +        nvbo = nouveau_gem_object(fb->obj[0]);
>>>>>            nouveau_vma_del(&fbcon->vma);
>>>>>            nouveau_bo_unmap(nvbo);
>>>>>            nouveau_bo_unpin(nvbo);
>>>>> -        drm_framebuffer_put(&nouveau_fb->base);
>>>>> +        drm_framebuffer_put(fb);
>>>>>        }
>>>>>        return 0;
>>>>>
>>>> _______________________________________________
>>>> Nouveau mailing list
>>>> Nouveau at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/nouveau
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau