On Tue, 11 Feb 2020 at 09:17, James Jones <jajones@xxxxxxxxxx> 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@xxxxxxxxxx> 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. 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@xxxxxxx> > >>>>>>> --- > >>>>>>> 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@xxxxxxxxxxxxxxxxxxxxx > >>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > >>>>> > >>>> _______________________________________________ > >>>> dri-devel mailing list > >>>> dri-devel@xxxxxxxxxxxxxxxxxxxxx > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > >>> _______________________________________________ > >>> dri-devel mailing list > >>> dri-devel@xxxxxxxxxxxxxxxxxxxxx > >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
