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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1: Type: text/plain, Size: 13426 bytes --]

Hi

Am 10.02.20 um 09:20 schrieb Ben Skeggs:
> On Sat, 8 Feb 2020 at 07:10, James Jones <jajones@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!

> 
> 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?
> 
> 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@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@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel