[PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@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@amd.com>
Cc: amd-gfx@lists.freedesktop.org
Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: chris@chris-wilson.co.uk
Cc: "Christian König" <christian.koenig@amd.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@gmail.com>
Cc: David Airlie <airlied@linux.ie>
Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: freedreno@lists.freedesktop.org
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Joonyoung Shim <jy0922.shim@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: linux-arm-msm@vger.kernel.org
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: martin.peres@free.fr
Cc: Rob Clark <robdclark@gmail.com>
Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sinclair Yeh <syeh@vmware.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: Vincent Abriou <vincent.abriou@st.com>
Cc: virtualization@lists.linux-foundation.org
Cc: VMware Graphics <linux-graphics-maintainer@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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 10/23] drm/msm: Stop consulting plane->fb

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We want to get rid of plane->fb on atomic drivers. Stop looking at it.

Cc: Rob Clark <robdclark@gmail.com>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
index 6e5e1aa54ce1..99ead8e37c72 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
@@ -201,7 +201,7 @@ static void blend_setup(struct drm_crtc *crtc)
 		int idx = idxs[pipe_id];
 		if (idx > 0) {
 			const struct mdp_format *format =
-					to_mdp_format(msm_framebuffer_format(plane->fb));
+					to_mdp_format(msm_framebuffer_format(plane->state->fb));
 			alpha[idx-1] = format->alpha_enable;
 		}
 	}
-- 
2.16.1

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 19/23] drm/msm: Stop updating plane->fb/crtc

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We want to get rid of plane->fb/crtc on atomic drivers. Stop setting
them.

Cc: Rob Clark <robdclark@gmail.com>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c  | 1 -
 drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 2 --
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 2 --
 3 files changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
index 99ead8e37c72..13f3f155dc67 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
@@ -664,7 +664,6 @@ struct drm_crtc *mdp4_crtc_init(struct drm_device *dev,
 	drm_crtc_init_with_planes(dev, crtc, plane, NULL, &mdp4_crtc_funcs,
 				  NULL);
 	drm_crtc_helper_add(crtc, &mdp4_crtc_helper_funcs);
-	plane->crtc = crtc;
 
 	return crtc;
 }
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
index 7a1ad3af08e3..782b1e27f040 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
@@ -182,8 +182,6 @@ static void mdp4_plane_set_scanout(struct drm_plane *plane,
 			msm_framebuffer_iova(fb, kms->aspace, 2));
 	mdp4_write(mdp4_kms, REG_MDP4_PIPE_SRCP3_BASE(pipe),
 			msm_framebuffer_iova(fb, kms->aspace, 3));
-
-	plane->fb = fb;
 }
 
 static void mdp4_write_csc_config(struct mdp4_kms *mdp4_kms,
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
index a9f31da7d45a..29c2988e8104 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
@@ -1043,8 +1043,6 @@ static int mdp5_plane_mode_set(struct drm_plane *plane,
 				     src_img_w, src_img_h,
 				     src_x + src_w, src_y, src_w, src_h);
 
-	plane->fb = fb;
-
 	return ret;
 }
 
-- 
2.16.1

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Daniel Vetter]

On Thu, Mar 22, 2018 at 05:22:50PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@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@amd.com>
> Cc: amd-gfx@lists.freedesktop.org
> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: chris@chris-wilson.co.uk
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@gmail.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: freedreno@lists.freedesktop.org
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: martin.peres@free.fr
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sinclair Yeh <syeh@vmware.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: Vincent Abriou <vincent.abriou@st.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: VMware Graphics <linux-graphics-maintainer@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

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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Noralf Trønnes]

Den 22.03.2018 16.22, skrev Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@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@amd.com>
> Cc: amd-gfx@lists.freedesktop.org
> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: chris@chris-wilson.co.uk
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@gmail.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: freedreno@lists.freedesktop.org
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: martin.peres@free.fr
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sinclair Yeh <syeh@vmware.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: Vincent Abriou <vincent.abriou@st.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: VMware Graphics <linux-graphics-maintainer@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

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(-)
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[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()...

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Noralf Trønnes]

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Emil Velikov]

Hi Ville,

On 22 March 2018 at 15:22, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@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
_______________________________________________
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

[Harry Wentland]

On 2018-03-22 01:54 PM, Emil Velikov wrote:
> Hi Ville,
> 
> On 22 March 2018 at 15:22, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
>> From: Ville Syrjälä <ville.syrjala@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?

Harry

> 
> HTH
> Emil
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Emil Velikov]

On 22 March 2018 at 18:03, Harry Wentland <harry.wentland@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@linux.intel.com> wrote:
>>> From: Ville Syrjälä <ville.syrjala@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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx