Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers
On Thu, Mar 22, 2018 at 05:22:50PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä > > I really just wanted to fix i915 to re-enable its planes afer load > detection (a two line patch). This is what I actually ended up with > after I ran into a framebuffer refcount leak with said two line patch. > > I've tested this on a few i915 boxes and so far it's looking > good. Everything else is just compile tested. > > Entire series available here: > git://github.com/vsyrjala/linux.git plane_fb_crtc_nuke > > Cc: Alex Deucher > Cc: amd-...@lists.freedesktop.org > Cc: Benjamin Gaignard > Cc: Boris Brezillon > Cc: ch...@chris-wilson.co.uk > Cc: "Christian König" > Cc: Daniel Vetter > Cc: Dave Airlie > Cc: David Airlie > Cc: "David (ChunMing) Zhou" > Cc: Eric Anholt > Cc: freedr...@lists.freedesktop.org > Cc: Gerd Hoffmann > Cc: Harry Wentland > Cc: Inki Dae > Cc: Joonyoung Shim > Cc: Kyungmin Park > Cc: linux-arm-...@vger.kernel.org > Cc: Maarten Lankhorst > Cc: martin.pe...@free.fr > Cc: Rob Clark > Cc: Seung-Woo Kim > Cc: Shawn Guo > Cc: Sinclair Yeh > Cc: Thomas Hellstrom > Cc: Vincent Abriou > Cc: virtualizat...@lists.linux-foundation.org > Cc: VMware Graphics > > Ville Syrjälä (23): > Revert "drm/atomic-helper: Fix leak in disable_all" > drm/atomic-helper: Make drm_atomic_helper_disable_all() update the > plane->fb pointers > drm: Clear crtc->primary->crtc when disabling the crtc via setcrtc() > drm/atomic-helper: WARN if legacy plane fb pointers are bogus when > committing duplicated state > drm: Add local 'plane' variable for primary/cursor planes > drm: Adjust whitespace for legibility > drm: Make the fb refcount handover less magic > drm: Use plane->state->fb over plane->fb > drm/i915: Stop consulting plane->fb > drm/msm: Stop consulting plane->fb > drm/sti: Stop consulting plane->fb > drm/vmwgfx: Stop consulting plane->fb > drm/zte: Stop consulting plane->fb > drm/atmel-hlcdc: Stop using plane->fb > drm: Stop updating plane->crtc/fb/old_fb on atomic drivers > drm/amdgpu/dc: Stop updating plane->fb > drm/i915: Stop updating plane->fb/crtc > drm/exynos: Stop updating plane->crtc > drm/msm: Stop updating plane->fb/crtc > drm/virtio: Stop updating plane->fb > drm/vc4: Stop updating plane->fb/crtc > drm/i915: Restore planes after load detection > drm/i915: Make force_load_detect effective even w/ DMI quirks/hotplug Ok, I reviewed the core patches, looks all good. Also starting auditing all the drivers. I think there's some small oversights in there, and I need to update my grep foo a bit, but by and large looks all reasonable. Imo we should start merging, that will also make the auditing easier. -Daniel > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 - > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 12 +--- > drivers/gpu/drm/drm_atomic.c | 55 ++-- > drivers/gpu/drm/drm_atomic_helper.c | 79 > ++- > drivers/gpu/drm/drm_crtc.c| 51 ++- > drivers/gpu/drm/drm_fb_helper.c | 7 -- > drivers/gpu/drm/drm_framebuffer.c | 5 -- > drivers/gpu/drm/drm_plane.c | 64 +++--- > drivers/gpu/drm/drm_plane_helper.c| 4 +- > drivers/gpu/drm/exynos/exynos_drm_plane.c | 2 - > drivers/gpu/drm/i915/intel_crt.c | 6 ++ > drivers/gpu/drm/i915/intel_display.c | 9 +-- > drivers/gpu/drm/i915/intel_fbdev.c| 2 +- > drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 3 +- > drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c| 2 - > drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c| 2 - > drivers/gpu/drm/sti/sti_plane.c | 9 +-- > drivers/gpu/drm/vc4/vc4_crtc.c| 3 - > drivers/gpu/drm/virtio/virtgpu_display.c | 2 - > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 6 +- > drivers/gpu/drm/zte/zx_vou.c | 2 +- > include/drm/drm_atomic.h | 3 - > 22 files changed, 143 insertions(+), 187 deletions(-) > > -- > 2.16.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers
Den 22.03.2018 19.49, skrev Ville Syrjälä: On Thu, Mar 22, 2018 at 05:51:35PM +0100, Noralf Trønnes wrote: tinydrm is also using plane->fb: $ grep -r "plane\.fb" drivers/gpu/drm/tinydrm/ drivers/gpu/drm/tinydrm/repaper.c: if (tdev->pipe.plane.fb != fb) drivers/gpu/drm/tinydrm/mipi-dbi.c: if (tdev->pipe.plane.fb != fb) drivers/gpu/drm/tinydrm/mipi-dbi.c: struct drm_framebuffer *fb = mipi->tinydrm.pipe.plane.fb; Oh dear, and naturally it's the most annoying one of the bunch. So we want to take the plane lock in the fb.dirty() hook to look at the fb, but mipi-dbi.c calls that directly from the bowels of its .atomic_enable() hook. So that would deadlock pretty neatly if the commit happens while already holding the lock. So we'd either need to plumb an acquire context into fb.dirty(), or maybe have tinydrm provide a lower level lockless dirty() hook that gets called by both (there are just too many layers in this particular cake to immediately see where such a hook would be best placed). Or maybe there's some other solution I didn't think of. Looking at this I'm also left wondering how this is supposed handle fb.dirty() getting called concurrently with a modeset. The dirty_mutex only seems to offer any protection for fb.dirty() against another fb.dirty()... I have been waiting for the 'page-flip with damage'[1] work to come to fruition so I could handle all flushing during atomic commit. The idea is to use the same damage rect for a generic dirtyfb callback. Maybe a temporary tinydrm specific solution is possible until that work lands, but I don't know enough about how to implement such a dirtyfb callback. I imagine it could look something like this: struct tinydrm_device { + struct drm_clip_rect dirtyfb_rect; }; static int tinydrm_fb_dirty(struct drm_framebuffer *fb, struct drm_file *file_priv, unsigned int flags, unsigned int color, struct drm_clip_rect *clips, unsigned int num_clips) { struct tinydrm_device *tdev = fb->dev->dev_private; struct drm_framebuffer *planefb; /* Grab whatever lock needed to check this */ planefb = tdev->pipe.plane.state->fb; /* fbdev can flush even when we're not interested */ if (fb != planefb) return 0; /* Protect dirtyfb_rect with a lock */ tinydrm_merge_clips(&tdev->dirty_rectfb, clips, num_clips, flags, fb->width, fb->height); /* * Somehow do an atomic commit that results in the atomic update * callback being called */ ret = drm_atomic_commit(state); ... } static void mipi_dbi_flush(struct drm_framebuffer *fb, struct drm_clip_rect *clip) { /* Flush out framebuffer */ } void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *old_state) { struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); struct drm_framebuffer *fb = pipe->plane.state->fb; struct drm_crtc *crtc = &tdev->pipe.crtc; if (!fb || (fb == old_state->fb && !tdev->dirty_rect.x2)) goto out_vblank; /* Don't flush if the controller isn't initialized yet */ if (!mipi->enabled) goto out_vblank; if (fb != old_state->fb) { /* Page flip or new, flush all */ mipi_dbi_flush(fb, NULL); } else { /* dirtyfb ioctl */ mipi_dbi_flush(fb, &tdev->dirtyfb_rect); memset(&tdev->dirtyfb_rect, 0, sizeof(tdev->dirtyfb_rect)); } out_vblank: if (crtc->state->event) { spin_lock_irq(&crtc->dev->event_lock); drm_crtc_send_vblank_event(crtc, crtc->state->event); spin_unlock_irq(&crtc->dev->event_lock); crtc->state->event = NULL; } } This is called from the driver pipe enable callback after the controller has been initialized: void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe, struct drm_crtc_state *crtc_state) { struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); - struct drm_framebuffer *fb = pipe->plane.fb; + struct drm_framebuffer *fb = pipe->plane.state->fb; DRM_DEBUG_KMS("\n"); mipi->enabled = true; - if (fb) - fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0); + mipi_dbi_flush(fb, NULL); tinydrm_enable_backlight(mipi->backlight); } I can make patches if this makes any sense and you can help me with the dirtyfb implementation. Noralf. [1] https://lists.freedesktop.org/archives/dri-devel/2017-December/161003.html ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers
On Thu, Mar 22, 2018 at 05:51:35PM +0100, Noralf Trønnes wrote: > tinydrm is also using plane->fb: > > $ grep -r "plane\.fb" drivers/gpu/drm/tinydrm/ > drivers/gpu/drm/tinydrm/repaper.c: if (tdev->pipe.plane.fb != fb) > drivers/gpu/drm/tinydrm/mipi-dbi.c: if (tdev->pipe.plane.fb != fb) > drivers/gpu/drm/tinydrm/mipi-dbi.c: struct drm_framebuffer *fb = > mipi->tinydrm.pipe.plane.fb; Oh dear, and naturally it's the most annoying one of the bunch. So we want to take the plane lock in the fb.dirty() hook to look at the fb, but mipi-dbi.c calls that directly from the bowels of its .atomic_enable() hook. So that would deadlock pretty neatly if the commit happens while already holding the lock. So we'd either need to plumb an acquire context into fb.dirty(), or maybe have tinydrm provide a lower level lockless dirty() hook that gets called by both (there are just too many layers in this particular cake to immediately see where such a hook would be best placed). Or maybe there's some other solution I didn't think of. Looking at this I'm also left wondering how this is supposed handle fb.dirty() getting called concurrently with a modeset. The dirty_mutex only seems to offer any protection for fb.dirty() against another fb.dirty()... -- Ville Syrjälä Intel OTC ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers
On 22 March 2018 at 18:03, Harry Wentland wrote: > On 2018-03-22 01:54 PM, Emil Velikov wrote: >> Hi Ville, >> >> On 22 March 2018 at 15:22, Ville Syrjala >> wrote: >>> From: Ville Syrjälä >>> >>> I really just wanted to fix i915 to re-enable its planes afer load >>> detection (a two line patch). This is what I actually ended up with >>> after I ran into a framebuffer refcount leak with said two line patch. >>> >>> I've tested this on a few i915 boxes and so far it's looking >>> good. Everything else is just compile tested. >>> >> Mostly thinking out loud: >> >> Wondering if one cannot somehow (re)move plane->fb/crtc altogether. >> Otherwise drivers will reintroduce similar code, despite the WARNs and >> beefy documentation :-\ > > Wouldn't that require an atomic conversion of all remaining drivers? > That or maybe move into plane->legacy->{fb,crtc}. Feel free to swap 'legacy' with flashier name. Hmm back in 2015 we had a GSoC that updated BOCHS and CIRRUS drivers, but they never got merged. Don't recall the details - from memory the conversion seemed fine, but there was either shortage on review/other. Might be worth reviving that... regardless it's getting a bit off-topic. -Emil [1] https://www.google-melange.com/archive/gsoc/2015/orgs/xorg/projects/johnhunter.html ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers
On 2018-03-22 01:54 PM, Emil Velikov wrote: > Hi Ville, > > On 22 March 2018 at 15:22, Ville Syrjala > wrote: >> From: Ville Syrjälä >> >> I really just wanted to fix i915 to re-enable its planes afer load >> detection (a two line patch). This is what I actually ended up with >> after I ran into a framebuffer refcount leak with said two line patch. >> >> I've tested this on a few i915 boxes and so far it's looking >> good. Everything else is just compile tested. >> > Mostly thinking out loud: > > Wondering if one cannot somehow (re)move plane->fb/crtc altogether. > Otherwise drivers will reintroduce similar code, despite the WARNs and > beefy documentation :-\ Wouldn't that require an atomic conversion of all remaining drivers? Harry > > HTH > Emil > ___ > 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
Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers
Hi Ville, On 22 March 2018 at 15:22, Ville Syrjala wrote: > From: Ville Syrjälä > > I really just wanted to fix i915 to re-enable its planes afer load > detection (a two line patch). This is what I actually ended up with > after I ran into a framebuffer refcount leak with said two line patch. > > I've tested this on a few i915 boxes and so far it's looking > good. Everything else is just compile tested. > Mostly thinking out loud: Wondering if one cannot somehow (re)move plane->fb/crtc altogether. Otherwise drivers will reintroduce similar code, despite the WARNs and beefy documentation :-\ HTH Emil ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers
Den 22.03.2018 16.22, skrev Ville Syrjala: From: Ville Syrjälä I really just wanted to fix i915 to re-enable its planes afer load detection (a two line patch). This is what I actually ended up with after I ran into a framebuffer refcount leak with said two line patch. I've tested this on a few i915 boxes and so far it's looking good. Everything else is just compile tested. Entire series available here: git://github.com/vsyrjala/linux.git plane_fb_crtc_nuke Cc: Alex Deucher Cc: amd-...@lists.freedesktop.org Cc: Benjamin Gaignard Cc: Boris Brezillon Cc: ch...@chris-wilson.co.uk Cc: "Christian König" Cc: Daniel Vetter Cc: Dave Airlie Cc: David Airlie Cc: "David (ChunMing) Zhou" Cc: Eric Anholt Cc: freedr...@lists.freedesktop.org Cc: Gerd Hoffmann Cc: Harry Wentland Cc: Inki Dae Cc: Joonyoung Shim Cc: Kyungmin Park Cc: linux-arm-...@vger.kernel.org Cc: Maarten Lankhorst Cc: martin.pe...@free.fr Cc: Rob Clark Cc: Seung-Woo Kim Cc: Shawn Guo Cc: Sinclair Yeh Cc: Thomas Hellstrom Cc: Vincent Abriou Cc: virtualizat...@lists.linux-foundation.org Cc: VMware Graphics Ville Syrjälä (23): Revert "drm/atomic-helper: Fix leak in disable_all" drm/atomic-helper: Make drm_atomic_helper_disable_all() update the plane->fb pointers drm: Clear crtc->primary->crtc when disabling the crtc via setcrtc() drm/atomic-helper: WARN if legacy plane fb pointers are bogus when committing duplicated state drm: Add local 'plane' variable for primary/cursor planes drm: Adjust whitespace for legibility drm: Make the fb refcount handover less magic drm: Use plane->state->fb over plane->fb drm/i915: Stop consulting plane->fb drm/msm: Stop consulting plane->fb drm/sti: Stop consulting plane->fb drm/vmwgfx: Stop consulting plane->fb drm/zte: Stop consulting plane->fb drm/atmel-hlcdc: Stop using plane->fb drm: Stop updating plane->crtc/fb/old_fb on atomic drivers drm/amdgpu/dc: Stop updating plane->fb drm/i915: Stop updating plane->fb/crtc drm/exynos: Stop updating plane->crtc drm/msm: Stop updating plane->fb/crtc drm/virtio: Stop updating plane->fb drm/vc4: Stop updating plane->fb/crtc drm/i915: Restore planes after load detection drm/i915: Make force_load_detect effective even w/ DMI quirks/hotplug tinydrm is also using plane->fb: $ grep -r "plane\.fb" drivers/gpu/drm/tinydrm/ drivers/gpu/drm/tinydrm/repaper.c: if (tdev->pipe.plane.fb != fb) drivers/gpu/drm/tinydrm/mipi-dbi.c: if (tdev->pipe.plane.fb != fb) drivers/gpu/drm/tinydrm/mipi-dbi.c: struct drm_framebuffer *fb = mipi->tinydrm.pipe.plane.fb; drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c: pipe->plane.fb = fb; drivers/gpu/drm/tinydrm/ili9225.c: if (tdev->pipe.plane.fb != fb) drivers/gpu/drm/tinydrm/st7586.c: if (tdev->pipe.plane.fb != fb) Noralf. drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 - drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 12 +--- drivers/gpu/drm/drm_atomic.c | 55 ++-- drivers/gpu/drm/drm_atomic_helper.c | 79 ++- drivers/gpu/drm/drm_crtc.c| 51 ++- drivers/gpu/drm/drm_fb_helper.c | 7 -- drivers/gpu/drm/drm_framebuffer.c | 5 -- drivers/gpu/drm/drm_plane.c | 64 +++--- drivers/gpu/drm/drm_plane_helper.c| 4 +- drivers/gpu/drm/exynos/exynos_drm_plane.c | 2 - drivers/gpu/drm/i915/intel_crt.c | 6 ++ drivers/gpu/drm/i915/intel_display.c | 9 +-- drivers/gpu/drm/i915/intel_fbdev.c| 2 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 3 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c| 2 - drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c| 2 - drivers/gpu/drm/sti/sti_plane.c | 9 +-- drivers/gpu/drm/vc4/vc4_crtc.c| 3 - drivers/gpu/drm/virtio/virtgpu_display.c | 2 - drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 6 +- drivers/gpu/drm/zte/zx_vou.c | 2 +- include/drm/drm_atomic.h | 3 - 22 files changed, 143 insertions(+), 187 deletions(-) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers
From: Ville Syrjälä I really just wanted to fix i915 to re-enable its planes afer load detection (a two line patch). This is what I actually ended up with after I ran into a framebuffer refcount leak with said two line patch. I've tested this on a few i915 boxes and so far it's looking good. Everything else is just compile tested. Entire series available here: git://github.com/vsyrjala/linux.git plane_fb_crtc_nuke Cc: Alex Deucher Cc: amd-...@lists.freedesktop.org Cc: Benjamin Gaignard Cc: Boris Brezillon Cc: ch...@chris-wilson.co.uk Cc: "Christian König" Cc: Daniel Vetter Cc: Dave Airlie Cc: David Airlie Cc: "David (ChunMing) Zhou" Cc: Eric Anholt Cc: freedr...@lists.freedesktop.org Cc: Gerd Hoffmann Cc: Harry Wentland Cc: Inki Dae Cc: Joonyoung Shim Cc: Kyungmin Park Cc: linux-arm-...@vger.kernel.org Cc: Maarten Lankhorst Cc: martin.pe...@free.fr Cc: Rob Clark Cc: Seung-Woo Kim Cc: Shawn Guo Cc: Sinclair Yeh Cc: Thomas Hellstrom Cc: Vincent Abriou Cc: virtualizat...@lists.linux-foundation.org Cc: VMware Graphics Ville Syrjälä (23): Revert "drm/atomic-helper: Fix leak in disable_all" drm/atomic-helper: Make drm_atomic_helper_disable_all() update the plane->fb pointers drm: Clear crtc->primary->crtc when disabling the crtc via setcrtc() drm/atomic-helper: WARN if legacy plane fb pointers are bogus when committing duplicated state drm: Add local 'plane' variable for primary/cursor planes drm: Adjust whitespace for legibility drm: Make the fb refcount handover less magic drm: Use plane->state->fb over plane->fb drm/i915: Stop consulting plane->fb drm/msm: Stop consulting plane->fb drm/sti: Stop consulting plane->fb drm/vmwgfx: Stop consulting plane->fb drm/zte: Stop consulting plane->fb drm/atmel-hlcdc: Stop using plane->fb drm: Stop updating plane->crtc/fb/old_fb on atomic drivers drm/amdgpu/dc: Stop updating plane->fb drm/i915: Stop updating plane->fb/crtc drm/exynos: Stop updating plane->crtc drm/msm: Stop updating plane->fb/crtc drm/virtio: Stop updating plane->fb drm/vc4: Stop updating plane->fb/crtc drm/i915: Restore planes after load detection drm/i915: Make force_load_detect effective even w/ DMI quirks/hotplug drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 - drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 12 +--- drivers/gpu/drm/drm_atomic.c | 55 ++-- drivers/gpu/drm/drm_atomic_helper.c | 79 ++- drivers/gpu/drm/drm_crtc.c| 51 ++- drivers/gpu/drm/drm_fb_helper.c | 7 -- drivers/gpu/drm/drm_framebuffer.c | 5 -- drivers/gpu/drm/drm_plane.c | 64 +++--- drivers/gpu/drm/drm_plane_helper.c| 4 +- drivers/gpu/drm/exynos/exynos_drm_plane.c | 2 - drivers/gpu/drm/i915/intel_crt.c | 6 ++ drivers/gpu/drm/i915/intel_display.c | 9 +-- drivers/gpu/drm/i915/intel_fbdev.c| 2 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 3 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c| 2 - drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c| 2 - drivers/gpu/drm/sti/sti_plane.c | 9 +-- drivers/gpu/drm/vc4/vc4_crtc.c| 3 - drivers/gpu/drm/virtio/virtgpu_display.c | 2 - drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 6 +- drivers/gpu/drm/zte/zx_vou.c | 2 +- include/drm/drm_atomic.h | 3 - 22 files changed, 143 insertions(+), 187 deletions(-) -- 2.16.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel