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

James Jones jajones at nvidia.com
Tue Feb 11 17:28:02 UTC 2020



On 2/10/20 3:35 PM, Ben Skeggs wrote:
> On Tue, 11 Feb 2020 at 09:17, James Jones <jajones at nvidia.com> wrote:
>>
>> On 2/10/20 12:25 AM, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 10.02.20 um 09:20 schrieb Ben Skeggs:
>>>> On Sat, 8 Feb 2020 at 07:10, James Jones <jajones at nvidia.com> wrote:
>>>>>
>>>>> I've sent out a v4 version of the format modifier patches which avoid
>>>>> caching values in the nouveau_framebuffer struct.  It will have a few
>>>>> trivial conflicts with your series, but should make them structurally
>>>>> compatible.
>>>>>
>>>>> I'm fine with either v3 or v4 of my series personally, but if these
>>>>> cleanup patches are taken, only v4 will work.
>>>> I've taken Tomas' cleanup patches in my tree, and will take James'
>>>> also once they've been fixed up to work on top of the cleanup.
>>>
>>> Thanks!
>>
>> After applying this series locally, I'm hitting a NULL deref loading the
>> nouveau module with fbconsole caused by patch 3/4.  I've sent out a
>> trivial fix for review separately.  Please have a look, and Ben, feel
>> free to squash it with Thomas's original patch if you prefer.
> Oops.  Squashed!
> 
>>
>>>>
>>>> James, are you happy for me to take the drm_fourcc.h patch that's on
>>>> dri-devel through my tree for the next merge window too?
>>
>> Yes, that would be great.  I couldn't find a public version of your tree
>> with Thomas's patches applied, but I pulled them in locally and rebased
>> my series on top of that as v5, resolving all the remaining trivial
>> conflicts.  Appologies for all the patch spam this generated.
> I've pulled in your patches now too.

Awesome.  Thanks!

-James

> Thank you!
> Ben.
>>
>> Thanks,
>> -James
>>
>>>> Ben.
>>>>
>>>>>
>>>>> Thanks,
>>>>> -James
>>>>>
>>>>> On 2/6/20 8:45 AM, James Jones wrote:
>>>>>> Yes, that's certainly viable.  If that's the general preference in
>>>>>> direction, I'll rework that patches to do so.
>>>>>>
>>>>>> Thanks,
>>>>>> -James
>>>>>>
>>>>>> On 2/6/20 7:49 AM, Thomas Zimmermann wrote:
>>>>>>> Hi James
>>>>>>>
>>>>>>> Am 06.02.20 um 16:17 schrieb James Jones:
>>>>>>>> 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 for the note.
>>>>>>>
>>>>>>> I just took a look at your patchset. I think struct nouveau_framebuffer
>>>>>>> should not store tile_mode and kind. AFAICT there are only two trivial
>>>>>>> places where these values are used and they can be extracted from the
>>>>>>> framebuffer at any time.
>>>>>>>
>>>>>>> I'd suggest to expand nouveau_decode_mod() to take a drm_framebuffer and
>>>>>>> return the correct values. Kind of what you do in
>>>>>>> nouveau_framebuffer_new() near line 330.
>>>>>>>
>>>>>>> Thoughts?
>>>>>>>
>>>>>>> Best regards
>>>>>>> Thomas
>>>>>>>
>>>>>>> [1] https://patchwork.freedesktop.org/series/70786/#rev3
>>>>>>>
>>>>>>>>
>>>>>>>> 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;
>>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> dri-devel mailing list
>>>>>>>> dri-devel at lists.freedesktop.org
>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>>>
>>>>>> _______________________________________________
>>>>>> dri-devel mailing list
>>>>>> dri-devel at lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 


More information about the dri-devel mailing list