Ville Syrjala
2018-Mar-22 15:22 UTC
[PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers
From: Ville Syrj?l? <ville.syrjala at linux.intel.com> 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 <alexander.deucher at amd.com> Cc: amd-gfx at lists.freedesktop.org Cc: Benjamin Gaignard <benjamin.gaignard at linaro.org> Cc: Boris Brezillon <boris.brezillon at free-electrons.com> Cc: chris at chris-wilson.co.uk Cc: "Christian K?nig" <christian.koenig at amd.com> Cc: Daniel Vetter <daniel.vetter at ffwll.ch> Cc: Dave Airlie <airlied at gmail.com> Cc: David Airlie <airlied at linux.ie> Cc: "David (ChunMing) Zhou" <David1.Zhou at amd.com> Cc: Eric Anholt <eric at anholt.net> Cc: freedreno at lists.freedesktop.org Cc: Gerd Hoffmann <kraxel at redhat.com> Cc: Harry Wentland <harry.wentland at amd.com> Cc: Inki Dae <inki.dae at samsung.com> Cc: Joonyoung Shim <jy0922.shim at samsung.com> Cc: Kyungmin Park <kyungmin.park at samsung.com> Cc: linux-arm-msm at vger.kernel.org Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com> Cc: martin.peres at free.fr Cc: Rob Clark <robdclark at gmail.com> Cc: Seung-Woo Kim <sw0312.kim at samsung.com> Cc: Shawn Guo <shawnguo at kernel.org> Cc: Sinclair Yeh <syeh at vmware.com> Cc: Thomas Hellstrom <thellstrom at vmware.com> Cc: Vincent Abriou <vincent.abriou at st.com> Cc: virtualization at lists.linux-foundation.org Cc: VMware Graphics <linux-graphics-maintainer at vmware.com> 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
From: Ville Syrj?l? <ville.syrjala at linux.intel.com> We want to get rid of plane->fb on atomic drivers. Stop setting it. Cc: David Airlie <airlied at linux.ie> Cc: Gerd Hoffmann <kraxel at redhat.com> Cc: virtualization at lists.linux-foundation.org Signed-off-by: Ville Syrj?l? <ville.syrjala at linux.intel.com> --- drivers/gpu/drm/virtio/virtgpu_display.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index 8cc8c34d67f5..42e842ceb53c 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -302,8 +302,6 @@ static int vgdev_output_init(struct virtio_gpu_device *vgdev, int index) drm_crtc_init_with_planes(dev, crtc, primary, cursor, &virtio_gpu_crtc_funcs, NULL); drm_crtc_helper_add(crtc, &virtio_gpu_crtc_helper_funcs); - primary->crtc = crtc; - cursor->crtc = crtc; drm_connector_init(dev, connector, &virtio_gpu_connector_funcs, DRM_MODE_CONNECTOR_VIRTUAL); -- 2.16.1
> We want to get rid of plane->fb on atomic drivers. Stop setting it.> - primary->crtc = crtc; > - cursor->crtc = crtc;commit msg vs. patch content mismatch here ... cheers, Gerd
Noralf Trønnes
2018-Mar-22 16:51 UTC
[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? <ville.syrjala at linux.intel.com> > > 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 <alexander.deucher at amd.com> > Cc: amd-gfx at lists.freedesktop.org > Cc: Benjamin Gaignard <benjamin.gaignard at linaro.org> > Cc: Boris Brezillon <boris.brezillon at free-electrons.com> > Cc: chris at chris-wilson.co.uk > Cc: "Christian K?nig" <christian.koenig at amd.com> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch> > Cc: Dave Airlie <airlied at gmail.com> > Cc: David Airlie <airlied at linux.ie> > Cc: "David (ChunMing) Zhou" <David1.Zhou at amd.com> > Cc: Eric Anholt <eric at anholt.net> > Cc: freedreno at lists.freedesktop.org > Cc: Gerd Hoffmann <kraxel at redhat.com> > Cc: Harry Wentland <harry.wentland at amd.com> > Cc: Inki Dae <inki.dae at samsung.com> > Cc: Joonyoung Shim <jy0922.shim at samsung.com> > Cc: Kyungmin Park <kyungmin.park at samsung.com> > Cc: linux-arm-msm at vger.kernel.org > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com> > Cc: martin.peres at free.fr > Cc: Rob Clark <robdclark at gmail.com> > Cc: Seung-Woo Kim <sw0312.kim at samsung.com> > Cc: Shawn Guo <shawnguo at kernel.org> > Cc: Sinclair Yeh <syeh at vmware.com> > Cc: Thomas Hellstrom <thellstrom at vmware.com> > Cc: Vincent Abriou <vincent.abriou at st.com> > Cc: virtualization at lists.linux-foundation.org > Cc: VMware Graphics <linux-graphics-maintainer at vmware.com> > > 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/hotplugtinydrm 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(-) >
Ville Syrjala
2018-Mar-22 17:40 UTC
[PATCH v2 20/23] drm/virtio: Stop updating plane->crtc
From: Ville Syrj?l? <ville.syrjala at linux.intel.com> We want to get rid of plane->crtc on atomic drivers. Stop setting it. v2: s/fb/crtc/ in the commit message (Gerd) Cc: David Airlie <airlied at linux.ie> Cc: Gerd Hoffmann <kraxel at redhat.com> Cc: virtualization at lists.linux-foundation.org Signed-off-by: Ville Syrj?l? <ville.syrjala at linux.intel.com> --- drivers/gpu/drm/virtio/virtgpu_display.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index 8cc8c34d67f5..42e842ceb53c 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -302,8 +302,6 @@ static int vgdev_output_init(struct virtio_gpu_device *vgdev, int index) drm_crtc_init_with_planes(dev, crtc, primary, cursor, &virtio_gpu_crtc_funcs, NULL); drm_crtc_helper_add(crtc, &virtio_gpu_crtc_helper_funcs); - primary->crtc = crtc; - cursor->crtc = crtc; drm_connector_init(dev, connector, &virtio_gpu_connector_funcs, DRM_MODE_CONNECTOR_VIRTUAL); -- 2.16.1
Emil Velikov
2018-Mar-22 17:54 UTC
[PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers
Hi Ville, On 22 March 2018 at 15:22, Ville Syrjala <ville.syrjala at linux.intel.com> wrote:> From: Ville Syrj?l? <ville.syrjala at linux.intel.com> > > 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
Emil Velikov
2018-Mar-22 18:34 UTC
[PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers
On 22 March 2018 at 18:03, Harry Wentland <harry.wentland at amd.com> wrote:> On 2018-03-22 01:54 PM, Emil Velikov wrote: >> Hi Ville, >> >> On 22 March 2018 at 15:22, Ville Syrjala <ville.syrjala at linux.intel.com> wrote: >>> From: Ville Syrj?l? <ville.syrjala at linux.intel.com> >>> >>> 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
Ville Syrjälä
2018-Mar-22 18:49 UTC
[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
Daniel Vetter
2018-Mar-27 08:21 UTC
[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? <ville.syrjala at linux.intel.com> > > 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 <alexander.deucher at amd.com> > Cc: amd-gfx at lists.freedesktop.org > Cc: Benjamin Gaignard <benjamin.gaignard at linaro.org> > Cc: Boris Brezillon <boris.brezillon at free-electrons.com> > Cc: chris at chris-wilson.co.uk > Cc: "Christian K?nig" <christian.koenig at amd.com> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch> > Cc: Dave Airlie <airlied at gmail.com> > Cc: David Airlie <airlied at linux.ie> > Cc: "David (ChunMing) Zhou" <David1.Zhou at amd.com> > Cc: Eric Anholt <eric at anholt.net> > Cc: freedreno at lists.freedesktop.org > Cc: Gerd Hoffmann <kraxel at redhat.com> > Cc: Harry Wentland <harry.wentland at amd.com> > Cc: Inki Dae <inki.dae at samsung.com> > Cc: Joonyoung Shim <jy0922.shim at samsung.com> > Cc: Kyungmin Park <kyungmin.park at samsung.com> > Cc: linux-arm-msm at vger.kernel.org > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com> > Cc: martin.peres at free.fr > Cc: Rob Clark <robdclark at gmail.com> > Cc: Seung-Woo Kim <sw0312.kim at samsung.com> > Cc: Shawn Guo <shawnguo at kernel.org> > Cc: Sinclair Yeh <syeh at vmware.com> > Cc: Thomas Hellstrom <thellstrom at vmware.com> > Cc: Vincent Abriou <vincent.abriou at st.com> > Cc: virtualization at lists.linux-foundation.org > Cc: VMware Graphics <linux-graphics-maintainer at vmware.com> > > 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/hotplugOk, 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
Reasonably Related Threads
- [PATCH] drm/core: Remove drm_dev_unref() and it's uses
- [PATCH] drm/core: Remove drm_dev_unref() and it's uses
- [PATCH] drm/core: Remove drm_dev_unref() and it's uses
- [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers
- [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers