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! > > 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@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 -- 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:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
