All fields in struct nouveau_framebuffer appear to be obsolete. The data structure can be replaced by struct drm_framebuffer entirely.
Patch 1 removes several unused fields from struct nouveau_framebuffer.
Patch 2 moves the field vma to struct nouveau_fbdev. The information in vma is only relevant for fbdev emulation, and as such he field is only used there.
Patch 3 removes nvbo from struct nouveau_framebuffer. The nouveau buffer object is based on gem, and as such should be stored in obj[0] of struct drm_framebuffer. This also enables the use of several generic GEM framebuffer functions.
Finally patch 4 removes struct nouveau_framebuffer. At this point it's merely a wrapper around struct drm_framebuffer.
The patchset has been smoke-tested on NV34 HW by running fbcon and X11.
Future directions: There are still functions for creating frameuffers. With further refinements of nouveau's fbcon code, GEM framebuffer helpers could be used here.
Thomas Zimmermann (4): drm/nouveau: Remove unused fields from struct nouveau_framebuffer drm/nouveau: Move struct nouveau_framebuffer.vma to struct nouveau_fbdev drm/nouveau: Remove field nvbo from struct nouveau_framebuffer drm/nouveau: Remove struct nouveau_framebuffer
drivers/gpu/drm/nouveau/dispnv04/crtc.c | 19 ++++----- drivers/gpu/drm/nouveau/dispnv04/disp.c | 21 +++++----- drivers/gpu/drm/nouveau/dispnv04/overlay.c | 21 +++++----- drivers/gpu/drm/nouveau/dispnv50/wndw.c | 45 ++++++++++++--------- drivers/gpu/drm/nouveau/nouveau_display.c | 47 ++++++---------------- drivers/gpu/drm/nouveau/nouveau_display.h | 25 +++--------- drivers/gpu/drm/nouveau/nouveau_fbcon.c | 42 ++++++++++--------- drivers/gpu/drm/nouveau/nouveau_fbcon.h | 3 ++ drivers/gpu/drm/nouveau/nv50_fbcon.c | 9 ++--- drivers/gpu/drm/nouveau/nvc0_fbcon.c | 9 ++--- 10 files changed, 108 insertions(+), 133 deletions(-)
-- 2.25.0
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/nouveau/nouveau_display.h | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h index 6e8e66882e45..e397b3d246e5 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.h +++ b/drivers/gpu/drm/nouveau/nouveau_display.h @@ -12,11 +12,6 @@ struct nouveau_framebuffer { struct drm_framebuffer base; struct nouveau_bo *nvbo; struct nouveau_vma *vma; - u32 r_handle; - u32 r_format; - u32 r_pitch; - struct nvif_object h_base[4]; - struct nvif_object h_core; };
static inline struct nouveau_framebuffer *
The vma field of struct nouveau_framebuffer is a special field for the the accelerated fbdev console. Hence there's at most one single instance for the active console. Moving it into struct nouveau_fbdev makes struct nouveau_framebuffer slightly smaller and brings it closer to struct drm_framebuffer.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/nouveau/nouveau_display.h | 1 - drivers/gpu/drm/nouveau/nouveau_fbcon.c | 6 +++--- drivers/gpu/drm/nouveau/nouveau_fbcon.h | 3 +++ drivers/gpu/drm/nouveau/nv50_fbcon.c | 9 ++++----- drivers/gpu/drm/nouveau/nvc0_fbcon.c | 9 ++++----- 5 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h index e397b3d246e5..0b3eb04b95a7 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.h +++ b/drivers/gpu/drm/nouveau/nouveau_display.h @@ -11,7 +11,6 @@ struct nouveau_framebuffer { struct drm_framebuffer base; struct nouveau_bo *nvbo; - struct nouveau_vma *vma; };
static inline struct nouveau_framebuffer * diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c index 0c5cdda3c336..6b2f46b0c115 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c @@ -353,7 +353,7 @@ nouveau_fbcon_create(struct drm_fb_helper *helper,
chan = nouveau_nofbaccel ? NULL : drm->channel; if (chan && device->info.family >= NV_DEVICE_INFO_V0_TESLA) { - ret = nouveau_vma_new(nvbo, chan->vmm, &fb->vma); + ret = nouveau_vma_new(nvbo, chan->vmm, &fbcon->vma); if (ret) { NV_ERROR(drm, "failed to map fb into chan: %d\n", ret); chan = NULL; @@ -400,7 +400,7 @@ nouveau_fbcon_create(struct drm_fb_helper *helper,
out_unlock: if (chan) - nouveau_vma_del(&fb->vma); + nouveau_vma_del(&fbcon->vma); nouveau_bo_unmap(fb->nvbo); out_unpin: nouveau_bo_unpin(fb->nvbo); @@ -419,7 +419,7 @@ nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *fbcon) drm_fb_helper_fini(&fbcon->helper);
if (nouveau_fb && nouveau_fb->nvbo) { - nouveau_vma_del(&nouveau_fb->vma); + nouveau_vma_del(&fbcon->vma); nouveau_bo_unmap(nouveau_fb->nvbo); nouveau_bo_unpin(nouveau_fb->nvbo); drm_framebuffer_put(&nouveau_fb->base); diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.h b/drivers/gpu/drm/nouveau/nouveau_fbcon.h index 73a7eeba3973..1796d8824580 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.h +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.h @@ -31,6 +31,8 @@
#include "nouveau_display.h"
+struct nouveau_vma; + struct nouveau_fbdev { struct drm_fb_helper helper; /* must be first */ unsigned int saved_flags; @@ -41,6 +43,7 @@ struct nouveau_fbdev { struct nvif_object gdi; struct nvif_object blit; struct nvif_object twod; + struct nouveau_vma *vma;
struct mutex hotplug_lock; bool hotplug_waiting; diff --git a/drivers/gpu/drm/nouveau/nv50_fbcon.c b/drivers/gpu/drm/nouveau/nv50_fbcon.c index facd18564e0d..47428f79ede8 100644 --- a/drivers/gpu/drm/nouveau/nv50_fbcon.c +++ b/drivers/gpu/drm/nouveau/nv50_fbcon.c @@ -149,7 +149,6 @@ int nv50_fbcon_accel_init(struct fb_info *info) { struct nouveau_fbdev *nfbdev = info->par; - struct nouveau_framebuffer *fb = nouveau_framebuffer(nfbdev->helper.fb); struct drm_device *dev = nfbdev->helper.dev; struct nouveau_drm *drm = nouveau_drm(dev); struct nouveau_channel *chan = drm->channel; @@ -240,8 +239,8 @@ nv50_fbcon_accel_init(struct fb_info *info) OUT_RING(chan, info->fix.line_length); OUT_RING(chan, info->var.xres_virtual); OUT_RING(chan, info->var.yres_virtual); - OUT_RING(chan, upper_32_bits(fb->vma->addr)); - OUT_RING(chan, lower_32_bits(fb->vma->addr)); + OUT_RING(chan, upper_32_bits(nfbdev->vma->addr)); + OUT_RING(chan, lower_32_bits(nfbdev->vma->addr)); BEGIN_NV04(chan, NvSub2D, 0x0230, 2); OUT_RING(chan, format); OUT_RING(chan, 1); @@ -249,8 +248,8 @@ nv50_fbcon_accel_init(struct fb_info *info) OUT_RING(chan, info->fix.line_length); OUT_RING(chan, info->var.xres_virtual); OUT_RING(chan, info->var.yres_virtual); - OUT_RING(chan, upper_32_bits(fb->vma->addr)); - OUT_RING(chan, lower_32_bits(fb->vma->addr)); + OUT_RING(chan, upper_32_bits(nfbdev->vma->addr)); + OUT_RING(chan, lower_32_bits(nfbdev->vma->addr)); FIRE_RING(chan);
return 0; diff --git a/drivers/gpu/drm/nouveau/nvc0_fbcon.c b/drivers/gpu/drm/nouveau/nvc0_fbcon.c index c0deef4fe727..cb56163ed608 100644 --- a/drivers/gpu/drm/nouveau/nvc0_fbcon.c +++ b/drivers/gpu/drm/nouveau/nvc0_fbcon.c @@ -150,7 +150,6 @@ nvc0_fbcon_accel_init(struct fb_info *info) { struct nouveau_fbdev *nfbdev = info->par; struct drm_device *dev = nfbdev->helper.dev; - struct nouveau_framebuffer *fb = nouveau_framebuffer(nfbdev->helper.fb); struct nouveau_drm *drm = nouveau_drm(dev); struct nouveau_channel *chan = drm->channel; int ret, format; @@ -240,8 +239,8 @@ nvc0_fbcon_accel_init(struct fb_info *info) OUT_RING (chan, info->fix.line_length); OUT_RING (chan, info->var.xres_virtual); OUT_RING (chan, info->var.yres_virtual); - OUT_RING (chan, upper_32_bits(fb->vma->addr)); - OUT_RING (chan, lower_32_bits(fb->vma->addr)); + OUT_RING (chan, upper_32_bits(nfbdev->vma->addr)); + OUT_RING (chan, lower_32_bits(nfbdev->vma->addr)); BEGIN_NVC0(chan, NvSub2D, 0x0230, 10); OUT_RING (chan, format); OUT_RING (chan, 1); @@ -251,8 +250,8 @@ nvc0_fbcon_accel_init(struct fb_info *info) OUT_RING (chan, info->fix.line_length); OUT_RING (chan, info->var.xres_virtual); OUT_RING (chan, info->var.yres_virtual); - OUT_RING (chan, upper_32_bits(fb->vma->addr)); - OUT_RING (chan, lower_32_bits(fb->vma->addr)); + OUT_RING (chan, upper_32_bits(nfbdev->vma->addr)); + OUT_RING (chan, lower_32_bits(nfbdev->vma->addr)); FIRE_RING (chan);
return 0;
The buffer object stored in nvbo is also available GEM object in obj[0] of struct drm_framebuffer. Therefore remove nvbo in favor obj[0] and replace all references accordingly. This may require an additional cast.
With this change we can already replace nouveau_user_framebuffer_destroy() and nouveau_user_framebuffer_create_handle() with generic GEM helpers. Calls to nouveau_framebuffer_new() receive a GEM object.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/nouveau/dispnv04/crtc.c | 19 ++++++------ drivers/gpu/drm/nouveau/dispnv04/disp.c | 21 ++++++------- drivers/gpu/drm/nouveau/dispnv04/overlay.c | 21 +++++++------ drivers/gpu/drm/nouveau/dispnv50/wndw.c | 25 +++++++++------- drivers/gpu/drm/nouveau/nouveau_display.c | 35 ++++------------------ drivers/gpu/drm/nouveau/nouveau_display.h | 9 +++--- drivers/gpu/drm/nouveau/nouveau_fbcon.c | 28 +++++++++-------- 7 files changed, 74 insertions(+), 84 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c index 37c50ea8f847..ece877c727cd 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c @@ -605,15 +605,16 @@ static int nv_crtc_swap_fbs(struct drm_crtc *crtc, struct drm_framebuffer *old_fb) { struct nv04_display *disp = nv04_display(crtc->dev); - struct nouveau_framebuffer *nvfb = nouveau_framebuffer(crtc->primary->fb); + struct drm_framebuffer *fb = crtc->primary->fb; + struct nouveau_bo *nvbo = nouveau_gem_object(fb->obj[0]); struct nouveau_crtc *nv_crtc = nouveau_crtc(crtc); int ret;
- ret = nouveau_bo_pin(nvfb->nvbo, TTM_PL_FLAG_VRAM, false); + ret = nouveau_bo_pin(nvbo, TTM_PL_FLAG_VRAM, false); if (ret == 0) { if (disp->image[nv_crtc->index]) nouveau_bo_unpin(disp->image[nv_crtc->index]); - nouveau_bo_ref(nvfb->nvbo, &disp->image[nv_crtc->index]); + nouveau_bo_ref(nvbo, &disp->image[nv_crtc->index]); }
return ret; @@ -822,8 +823,8 @@ nv04_crtc_do_mode_set_base(struct drm_crtc *crtc, struct drm_device *dev = crtc->dev; struct nouveau_drm *drm = nouveau_drm(dev); struct nv04_crtc_reg *regp = &nv04_display(dev)->mode_reg.crtc_reg[nv_crtc->index]; + struct nouveau_bo *nvbo; struct drm_framebuffer *drm_fb; - struct nouveau_framebuffer *fb; int arb_burst, arb_lwm;
NV_DEBUG(drm, "index %d\n", nv_crtc->index); @@ -839,13 +840,12 @@ nv04_crtc_do_mode_set_base(struct drm_crtc *crtc, */ if (atomic) { drm_fb = passed_fb; - fb = nouveau_framebuffer(passed_fb); } else { drm_fb = crtc->primary->fb; - fb = nouveau_framebuffer(crtc->primary->fb); }
- nv_crtc->fb.offset = fb->nvbo->bo.offset; + nvbo = nouveau_gem_object(drm_fb->obj[0]); + nv_crtc->fb.offset = nvbo->bo.offset;
if (nv_crtc->lut.depth != drm_fb->format->depth) { nv_crtc->lut.depth = drm_fb->format->depth; @@ -1143,8 +1143,9 @@ nv04_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, const int swap_interval = (flags & DRM_MODE_PAGE_FLIP_ASYNC) ? 0 : 1; struct drm_device *dev = crtc->dev; struct nouveau_drm *drm = nouveau_drm(dev); - struct nouveau_bo *old_bo = nouveau_framebuffer(crtc->primary->fb)->nvbo; - struct nouveau_bo *new_bo = nouveau_framebuffer(fb)->nvbo; + struct drm_framebuffer *old_fb = crtc->primary->fb; + struct nouveau_bo *old_bo = nouveau_gem_object(old_fb->obj[0]); + struct nouveau_bo *new_bo = nouveau_gem_object(fb->obj[0]); struct nv04_page_flip_state *s; struct nouveau_channel *chan; struct nouveau_cli *cli; diff --git a/drivers/gpu/drm/nouveau/dispnv04/disp.c b/drivers/gpu/drm/nouveau/dispnv04/disp.c index 44ee82d0c9b6..0f4ebefed1fd 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv04/disp.c @@ -30,6 +30,7 @@ #include "nouveau_encoder.h" #include "nouveau_connector.h" #include "nouveau_bo.h" +#include "nouveau_gem.h"
#include <nvif/if0004.h>
@@ -52,13 +53,13 @@ nv04_display_fini(struct drm_device *dev, bool suspend)
/* Un-pin FB and cursors so they'll be evicted to system memory. */ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { - struct nouveau_framebuffer *nouveau_fb; + struct drm_framebuffer *fb = crtc->primary->fb; + struct nouveau_bo *nvbo;
- nouveau_fb = nouveau_framebuffer(crtc->primary->fb); - if (!nouveau_fb || !nouveau_fb->nvbo) + if (!fb || !fb->obj[0]) continue; - - nouveau_bo_unpin(nouveau_fb->nvbo); + nvbo = nouveau_gem_object(fb->obj[0]); + nouveau_bo_unpin(nvbo); }
list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { @@ -104,13 +105,13 @@ nv04_display_init(struct drm_device *dev, bool resume, bool runtime)
/* Re-pin FB/cursors. */ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { - struct nouveau_framebuffer *nouveau_fb; + struct drm_framebuffer *fb = crtc->primary->fb; + struct nouveau_bo *nvbo;
- nouveau_fb = nouveau_framebuffer(crtc->primary->fb); - if (!nouveau_fb || !nouveau_fb->nvbo) + if (!fb || !fb->obj[0]) continue; - - ret = nouveau_bo_pin(nouveau_fb->nvbo, TTM_PL_FLAG_VRAM, true); + nvbo = nouveau_gem_object(fb->obj[0]); + ret = nouveau_bo_pin(nvbo, TTM_PL_FLAG_VRAM, true); if (ret) NV_ERROR(drm, "Could not pin framebuffer\n"); } diff --git a/drivers/gpu/drm/nouveau/dispnv04/overlay.c b/drivers/gpu/drm/nouveau/dispnv04/overlay.c index a3a0a73ae8ab..6248fd1dbc6d 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/overlay.c +++ b/drivers/gpu/drm/nouveau/dispnv04/overlay.c @@ -31,6 +31,7 @@ #include "nouveau_bo.h" #include "nouveau_connector.h" #include "nouveau_display.h" +#include "nouveau_gem.h" #include "nvreg.h" #include "disp.h"
@@ -120,9 +121,9 @@ nv10_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, struct nvif_object *dev = &drm->client.device.object; struct nouveau_plane *nv_plane = container_of(plane, struct nouveau_plane, base); - struct nouveau_framebuffer *nv_fb = nouveau_framebuffer(fb); struct nouveau_crtc *nv_crtc = nouveau_crtc(crtc); struct nouveau_bo *cur = nv_plane->cur; + struct nouveau_bo *nvbo; bool flip = nv_plane->flip; int soff = NV_PCRTC0_SIZE * nv_crtc->index; int soff2 = NV_PCRTC0_SIZE * !nv_crtc->index; @@ -140,17 +141,18 @@ nv10_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, if (ret) return ret;
- ret = nouveau_bo_pin(nv_fb->nvbo, TTM_PL_FLAG_VRAM, false); + nvbo = nouveau_gem_object(fb->obj[0]); + ret = nouveau_bo_pin(nvbo, TTM_PL_FLAG_VRAM, false); if (ret) return ret;
- nv_plane->cur = nv_fb->nvbo; + nv_plane->cur = nvbo;
nvif_mask(dev, NV_PCRTC_ENGINE_CTRL + soff, NV_CRTC_FSEL_OVERLAY, NV_CRTC_FSEL_OVERLAY); nvif_mask(dev, NV_PCRTC_ENGINE_CTRL + soff2, NV_CRTC_FSEL_OVERLAY, 0);
nvif_wr32(dev, NV_PVIDEO_BASE(flip), 0); - nvif_wr32(dev, NV_PVIDEO_OFFSET_BUFF(flip), nv_fb->nvbo->bo.offset); + nvif_wr32(dev, NV_PVIDEO_OFFSET_BUFF(flip), nvbo->bo.offset); nvif_wr32(dev, NV_PVIDEO_SIZE_IN(flip), src_h << 16 | src_w); nvif_wr32(dev, NV_PVIDEO_POINT_IN(flip), src_y << 16 | src_x); nvif_wr32(dev, NV_PVIDEO_DS_DX(flip), (src_w << 20) / crtc_w); @@ -172,7 +174,7 @@ nv10_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, if (format & NV_PVIDEO_FORMAT_PLANAR) { nvif_wr32(dev, NV_PVIDEO_UVPLANE_BASE(flip), 0); nvif_wr32(dev, NV_PVIDEO_UVPLANE_OFFSET_BUFF(flip), - nv_fb->nvbo->bo.offset + fb->offsets[1]); + nvbo->bo.offset + fb->offsets[1]); } nvif_wr32(dev, NV_PVIDEO_FORMAT(flip), format | fb->pitches[0]); nvif_wr32(dev, NV_PVIDEO_STOP, 0); @@ -368,8 +370,8 @@ nv04_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, struct nvif_object *dev = &nouveau_drm(plane->dev)->client.device.object; struct nouveau_plane *nv_plane = container_of(plane, struct nouveau_plane, base); - struct nouveau_framebuffer *nv_fb = nouveau_framebuffer(fb); struct nouveau_bo *cur = nv_plane->cur; + struct nouveau_bo *nvbo; uint32_t overlay = 1; int brightness = (nv_plane->brightness - 512) * 62 / 512; int ret, i; @@ -384,11 +386,12 @@ nv04_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, if (ret) return ret;
- ret = nouveau_bo_pin(nv_fb->nvbo, TTM_PL_FLAG_VRAM, false); + nvbo = nouveau_gem_object(fb->obj[0]); + ret = nouveau_bo_pin(nvbo, TTM_PL_FLAG_VRAM, false); if (ret) return ret;
- nv_plane->cur = nv_fb->nvbo; + nv_plane->cur = nvbo;
nvif_wr32(dev, NV_PVIDEO_OE_STATE, 0); nvif_wr32(dev, NV_PVIDEO_SU_STATE, 0); @@ -396,7 +399,7 @@ nv04_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
for (i = 0; i < 2; i++) { nvif_wr32(dev, NV_PVIDEO_BUFF0_START_ADDRESS + 4 * i, - nv_fb->nvbo->bo.offset); + nvbo->bo.offset); nvif_wr32(dev, NV_PVIDEO_BUFF0_PITCH_LENGTH + 4 * i, fb->pitches[0]); nvif_wr32(dev, NV_PVIDEO_BUFF0_OFFSET + 4 * i, 0); diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c b/drivers/gpu/drm/nouveau/dispnv50/wndw.c index 890315291b01..ba1399965a1c 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c @@ -29,6 +29,7 @@ #include <drm/drm_fourcc.h>
#include "nouveau_bo.h" +#include "nouveau_gem.h"
static void nv50_wndw_ctxdma_del(struct nv50_wndw_ctxdma *ctxdma) @@ -43,7 +44,8 @@ nv50_wndw_ctxdma_new(struct nv50_wndw *wndw, struct nouveau_framebuffer *fb) { struct nouveau_drm *drm = nouveau_drm(fb->base.dev); struct nv50_wndw_ctxdma *ctxdma; - const u8 kind = fb->nvbo->kind; + struct nouveau_bo *nvbo = nouveau_gem_object(fb->base.obj[0]); + const u8 kind = nvbo->kind; const u32 handle = 0xfb000000 | kind; struct { struct nv_dma_v0 base; @@ -236,6 +238,7 @@ nv50_wndw_atomic_check_acquire(struct nv50_wndw *wndw, bool modeset, { struct nouveau_framebuffer *fb = nouveau_framebuffer(asyw->state.fb); struct nouveau_drm *drm = nouveau_drm(wndw->plane.dev); + struct nouveau_bo *nvbo = nouveau_gem_object(fb->base.obj[0]); int ret;
NV_ATOMIC(drm, "%s acquire\n", wndw->plane.name); @@ -243,7 +246,7 @@ nv50_wndw_atomic_check_acquire(struct nv50_wndw *wndw, bool modeset, if (asyw->state.fb != armw->state.fb || !armw->visible || modeset) { asyw->image.w = fb->base.width; asyw->image.h = fb->base.height; - asyw->image.kind = fb->nvbo->kind; + asyw->image.kind = nvbo->kind;
ret = nv50_wndw_atomic_check_acquire_rgb(asyw); if (ret) { @@ -255,9 +258,9 @@ nv50_wndw_atomic_check_acquire(struct nv50_wndw *wndw, bool modeset, if (asyw->image.kind) { asyw->image.layout = 0; if (drm->client.device.info.chipset >= 0xc0) - asyw->image.blockh = fb->nvbo->mode >> 4; + asyw->image.blockh = nvbo->mode >> 4; else - asyw->image.blockh = fb->nvbo->mode; + asyw->image.blockh = nvbo->mode; asyw->image.blocks[0] = fb->base.pitches[0] / 64; asyw->image.pitch[0] = 0; } else { @@ -469,14 +472,15 @@ nv50_wndw_atomic_check(struct drm_plane *plane, struct drm_plane_state *state) static void nv50_wndw_cleanup_fb(struct drm_plane *plane, struct drm_plane_state *old_state) { - struct nouveau_framebuffer *fb = nouveau_framebuffer(old_state->fb); struct nouveau_drm *drm = nouveau_drm(plane->dev); + struct nouveau_bo *nvbo;
NV_ATOMIC(drm, "%s cleanup: %p\n", plane->name, old_state->fb); if (!old_state->fb) return;
- nouveau_bo_unpin(fb->nvbo); + nvbo = nouveau_gem_object(old_state->fb->obj[0]); + nouveau_bo_unpin(nvbo); }
static int @@ -486,6 +490,7 @@ nv50_wndw_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state) 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 nv50_head_atom *asyh; struct nv50_wndw_ctxdma *ctxdma; int ret; @@ -494,22 +499,22 @@ nv50_wndw_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state) if (!asyw->state.fb) return 0;
- ret = nouveau_bo_pin(fb->nvbo, TTM_PL_FLAG_VRAM, true); + ret = nouveau_bo_pin(nvbo, TTM_PL_FLAG_VRAM, true); if (ret) return ret;
if (wndw->ctxdma.parent) { ctxdma = nv50_wndw_ctxdma_new(wndw, fb); if (IS_ERR(ctxdma)) { - nouveau_bo_unpin(fb->nvbo); + nouveau_bo_unpin(nvbo); return PTR_ERR(ctxdma); }
asyw->image.handle[0] = ctxdma->object.handle; }
- asyw->state.fence = dma_resv_get_excl_rcu(fb->nvbo->bo.base.resv); - asyw->image.offset[0] = fb->nvbo->bo.offset; + asyw->state.fence = dma_resv_get_excl_rcu(nvbo->bo.base.resv); + asyw->image.offset[0] = nvbo->bo.offset;
if (wndw->func->prepare) { asyh = nv50_head_atom_get(asyw->state.state, asyw->state.crtc); diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c index 53f9bceaf17a..bbbff55eb5d5 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -31,6 +31,7 @@ #include <drm/drm_crtc_helper.h> #include <drm/drm_fb_helper.h> #include <drm/drm_fourcc.h> +#include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_probe_helper.h> #include <drm/drm_vblank.h>
@@ -197,37 +198,15 @@ nouveau_display_vblank_init(struct drm_device *dev) return 0; }
-static void -nouveau_user_framebuffer_destroy(struct drm_framebuffer *drm_fb) -{ - struct nouveau_framebuffer *fb = nouveau_framebuffer(drm_fb); - - if (fb->nvbo) - drm_gem_object_put_unlocked(&fb->nvbo->bo.base); - - drm_framebuffer_cleanup(drm_fb); - kfree(fb); -} - -static int -nouveau_user_framebuffer_create_handle(struct drm_framebuffer *drm_fb, - struct drm_file *file_priv, - unsigned int *handle) -{ - struct nouveau_framebuffer *fb = nouveau_framebuffer(drm_fb); - - return drm_gem_handle_create(file_priv, &fb->nvbo->bo.base, handle); -} - static const struct drm_framebuffer_funcs nouveau_framebuffer_funcs = { - .destroy = nouveau_user_framebuffer_destroy, - .create_handle = nouveau_user_framebuffer_create_handle, + .destroy = drm_gem_fb_destroy, + .create_handle = drm_gem_fb_create_handle, };
int nouveau_framebuffer_new(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cmd, - struct nouveau_bo *nvbo, + struct drm_gem_object *gem, struct nouveau_framebuffer **pfb) { struct nouveau_drm *drm = nouveau_drm(dev); @@ -258,7 +237,7 @@ nouveau_framebuffer_new(struct drm_device *dev, return -ENOMEM;
drm_helper_mode_fill_fb_struct(dev, &fb->base, mode_cmd); - fb->nvbo = nvbo; + fb->base.obj[0] = gem;
ret = drm_framebuffer_init(dev, &fb->base, &nouveau_framebuffer_funcs); if (ret) @@ -272,16 +251,14 @@ nouveau_user_framebuffer_create(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cmd) { struct nouveau_framebuffer *fb; - struct nouveau_bo *nvbo; struct drm_gem_object *gem; int ret;
gem = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]); if (!gem) return ERR_PTR(-ENOENT); - nvbo = nouveau_gem_object(gem);
- ret = nouveau_framebuffer_new(dev, mode_cmd, nvbo, &fb); + ret = nouveau_framebuffer_new(dev, mode_cmd, gem, &fb); if (ret == 0) return &fb->base;
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h index 0b3eb04b95a7..56c1dec8fc28 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.h +++ b/drivers/gpu/drm/nouveau/nouveau_display.h @@ -10,7 +10,6 @@
struct nouveau_framebuffer { struct drm_framebuffer base; - struct nouveau_bo *nvbo; };
static inline struct nouveau_framebuffer * @@ -19,9 +18,11 @@ nouveau_framebuffer(struct drm_framebuffer *fb) return container_of(fb, struct nouveau_framebuffer, base); }
-int nouveau_framebuffer_new(struct drm_device *, - const struct drm_mode_fb_cmd2 *, - struct nouveau_bo *, struct nouveau_framebuffer **); +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 nouveau_display { void *priv; diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c index 6b2f46b0c115..02b36b44409c 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c @@ -335,7 +335,7 @@ nouveau_fbcon_create(struct drm_fb_helper *helper, goto out; }
- ret = nouveau_framebuffer_new(dev, &mode_cmd, nvbo, &fb); + ret = nouveau_framebuffer_new(dev, &mode_cmd, &nvbo->bo.base, &fb); if (ret) goto out_unref;
@@ -376,12 +376,12 @@ nouveau_fbcon_create(struct drm_fb_helper *helper, FBINFO_HWACCEL_FILLRECT | FBINFO_HWACCEL_IMAGEBLIT; info->fbops = &nouveau_fbcon_sw_ops; - info->fix.smem_start = fb->nvbo->bo.mem.bus.base + - fb->nvbo->bo.mem.bus.offset; - info->fix.smem_len = fb->nvbo->bo.mem.num_pages << PAGE_SHIFT; + info->fix.smem_start = nvbo->bo.mem.bus.base + + nvbo->bo.mem.bus.offset; + info->fix.smem_len = nvbo->bo.mem.num_pages << PAGE_SHIFT;
- info->screen_base = nvbo_kmap_obj_iovirtual(fb->nvbo); - info->screen_size = fb->nvbo->bo.mem.num_pages << PAGE_SHIFT; + info->screen_base = nvbo_kmap_obj_iovirtual(nvbo); + info->screen_size = nvbo->bo.mem.num_pages << PAGE_SHIFT;
drm_fb_helper_fill_info(info, &fbcon->helper, sizes);
@@ -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, fb->nvbo->bo.offset, nvbo); + fb->base.width, fb->base.height, nvbo->bo.offset, nvbo);
vga_switcheroo_client_fb_set(dev->pdev, info); return 0; @@ -401,11 +401,11 @@ nouveau_fbcon_create(struct drm_fb_helper *helper, out_unlock: if (chan) nouveau_vma_del(&fbcon->vma); - nouveau_bo_unmap(fb->nvbo); + nouveau_bo_unmap(nvbo); out_unpin: - nouveau_bo_unpin(fb->nvbo); + nouveau_bo_unpin(nvbo); out_unref: - nouveau_bo_ref(NULL, &fb->nvbo); + nouveau_bo_ref(NULL, &nvbo); out: return ret; } @@ -414,14 +414,16 @@ static int nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *fbcon) { struct nouveau_framebuffer *nouveau_fb = nouveau_framebuffer(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->nvbo) { + if (nouveau_fb && nouveau_fb->base.obj[0]) { + nvbo = nouveau_gem_object(nouveau_fb->base.obj[0]); nouveau_vma_del(&fbcon->vma); - nouveau_bo_unmap(nouveau_fb->nvbo); - nouveau_bo_unpin(nouveau_fb->nvbo); + nouveau_bo_unmap(nvbo); + nouveau_bo_unpin(nvbo); drm_framebuffer_put(&nouveau_fb->base); }
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;
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@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;
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_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]);
drm_framebuffer_put(&nouveau_fb->base);
drm_framebuffer_put(fb);}
return 0;
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
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
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
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.
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
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.
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_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);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
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_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);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
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@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.
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.
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@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
On Tue, 11 Feb 2020 at 09:17, James Jones jajones@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@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.
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@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
On 2/10/20 3:35 PM, Ben Skeggs wrote:
On Tue, 11 Feb 2020 at 09:17, James Jones jajones@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@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@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
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Ben Skeggs -
James Jones -
Thomas Zimmermann