dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] drm/atomic-helpers: remove legacy_cursor_update hacks
@ 2020-10-23 12:39 Daniel Vetter
  2020-10-23 12:39 ` [PATCH 2/3] drm/vc4: Drop legacy_cursor_update override Daniel Vetter
  2020-10-23 12:39 ` [PATCH 3/3] drm/doc: Document legacy_cursor_update better Daniel Vetter
  0 siblings, 2 replies; 7+ messages in thread
From: Daniel Vetter @ 2020-10-23 12:39 UTC (permalink / raw)
  To: DRI Development
  Cc: Daniel Vetter, Intel Graphics Development, Kazlauskas, Nicholas,
	Daniel Vetter, mikita.lipski, Michel Dänzer

The stuff never really worked, and leads to lots of fun because it
out-of-order frees atomic states. Which upsets KASAN, among other
things.

For async updates we now have a more solid solution with the
->atomic_async_check and ->atomic_async_commit hooks. Support for that
for msm and vc4 landed. nouveau and i915 have their own commit
routines, doing something similar.

For everyone else it's probably better to remove the use-after-free
bug, and encourage folks to use the async support instead. The
affected drivers which register a legacy cursor plane and don't either
use the new async stuff or their own commit routine are: amdgpu,
atmel, mediatek, qxl, rockchip, sti, sun4i, tegra, virtio, and vmwgfx.

Inspired by an amdgpu bug report.

v2: Drop RFC, I think with amdgpu converted over to use
atomic_async_check/commit done in

commit 674e78acae0dfb4beb56132e41cbae5b60f7d662
Author: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Date:   Wed Dec 5 14:59:07 2018 -0500

    drm/amd/display: Add fast path for cursor plane updates

we don't have any driver anymore where we have userspace expecting
solid legacy cursor support _and_ they are using the atomic helpers in
their fully glory. So we can retire this.

v3: Paper over msm and i915 regression. The complete_all is the only
thing missing afaict.

References: https://bugzilla.kernel.org/show_bug.cgi?id=199425
Cc: mikita.lipski@amd.com
Cc: Michel Dänzer <michel@daenzer.net>
Cc: harry.wentland@amd.com
Cc: Rob Clark <robdclark@gmail.com>
Cc: "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c          | 13 -------------
 drivers/gpu/drm/i915/display/intel_display.c | 13 +++++++++++++
 drivers/gpu/drm/msm/msm_atomic.c             |  2 ++
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index a7bcb4b4586c..549a31e6042c 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1481,13 +1481,6 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 	int i, ret;
 	unsigned crtc_mask = 0;
 
-	 /*
-	  * Legacy cursor ioctls are completely unsynced, and userspace
-	  * relies on that (by doing tons of cursor updates).
-	  */
-	if (old_state->legacy_cursor_update)
-		return;
-
 	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
 		if (!new_crtc_state->active)
 			continue;
@@ -2106,12 +2099,6 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 			continue;
 		}
 
-		/* Legacy cursor updates are fully unsynced. */
-		if (state->legacy_cursor_update) {
-			complete_all(&commit->flip_done);
-			continue;
-		}
-
 		if (!new_crtc_state->event) {
 			commit->event = kzalloc(sizeof(*commit->event),
 						GFP_KERNEL);
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 130303e0298a..4257ad7c69c7 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -16122,6 +16122,19 @@ static int intel_atomic_commit(struct drm_device *dev,
 				state->base.legacy_cursor_update = false;
 	}
 
+	/*
+	 * FIXME: Cut over to (async) commit helpers instead of hand-rolling
+	 * everything.
+	 */
+	if (state->base.legacy_cursor_update) {
+		struct intel_crtc_state *new_crtc_state;
+		struct intel_crtc *crtc;
+		int i;
+
+		for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
+			complete_all(&new_crtc_state->uapi.commit->flip_done);
+	}
+
 	ret = intel_atomic_prepare_commit(state);
 	if (ret) {
 		drm_dbg_atomic(&dev_priv->drm,
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 561bfa48841c..e0599b16a4ef 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -215,6 +215,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 		/* async updates are limited to single-crtc updates: */
 		WARN_ON(crtc_mask != drm_crtc_mask(async_crtc));
 
+		complete_all(&async_crtc->state->commit->flip_done);
+
 		/*
 		 * Start timer if we don't already have an update pending
 		 * on this crtc:
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/3] drm/vc4: Drop legacy_cursor_update override
  2020-10-23 12:39 [PATCH 1/3] drm/atomic-helpers: remove legacy_cursor_update hacks Daniel Vetter
@ 2020-10-23 12:39 ` Daniel Vetter
  2020-10-23 12:39 ` [PATCH 3/3] drm/doc: Document legacy_cursor_update better Daniel Vetter
  1 sibling, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2020-10-23 12:39 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter

With the removal of helper support it doesn't do anything anymore.
Also, we already have async plane update code in vc4.

Acked-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 149825ff5df8..bf0da77ab2e6 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -353,12 +353,6 @@ static int vc4_atomic_commit(struct drm_device *dev,
 		return 0;
 	}
 
-	/* We know for sure we don't want an async update here. Set
-	 * state->legacy_cursor_update to false to prevent
-	 * drm_atomic_helper_setup_commit() from auto-completing
-	 * commit->flip_done.
-	 */
-	state->legacy_cursor_update = false;
 	ret = drm_atomic_helper_setup_commit(state, nonblock);
 	if (ret)
 		return ret;
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/3] drm/doc: Document legacy_cursor_update better
  2020-10-23 12:39 [PATCH 1/3] drm/atomic-helpers: remove legacy_cursor_update hacks Daniel Vetter
  2020-10-23 12:39 ` [PATCH 2/3] drm/vc4: Drop legacy_cursor_update override Daniel Vetter
@ 2020-10-23 12:39 ` Daniel Vetter
  2020-10-27  9:36   ` Daniel Vetter
  1 sibling, 1 reply; 7+ messages in thread
From: Daniel Vetter @ 2020-10-23 12:39 UTC (permalink / raw)
  To: DRI Development
  Cc: David Airlie, Daniel Vetter, Intel Graphics Development,
	Thomas Zimmermann, Daniel Vetter, Sean Paul

It's the horror and shouldn't be used. Realized we're not clear on
this in a discussion with Rob about what msm is doing to better
support async commits.

v2: Refine existing todo item to include this (Thomas)

Cc: Sean Paul <sean@poorly.run>
Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 Documentation/gpu/todo.rst |  4 ++++
 include/drm/drm_atomic.h   | 12 +++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 700637e25ecd..6b224ef14455 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -105,6 +105,10 @@ converted over to the new infrastructure.
 One issue with the helpers is that they require that drivers handle completion
 events for atomic commits correctly. But fixing these bugs is good anyway.
 
+Somewhat related is the legacy_cursor_update hack, which should be replaced with
+the new atomic_async_check/commit functionality in the helpers in drivers that
+still look at that flag.
+
 Contact: Daniel Vetter, respective driver maintainers
 
 Level: Advanced
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index d07c851d255b..413fd0ca56a8 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -308,7 +308,6 @@ struct __drm_private_objs_state {
  * struct drm_atomic_state - the global state object for atomic updates
  * @ref: count of all references to this state (will not be freed until zero)
  * @dev: parent DRM device
- * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
  * @async_update: hint for asynchronous plane update
  * @planes: pointer to array of structures with per-plane data
  * @crtcs: pointer to array of CRTC pointers
@@ -336,6 +335,17 @@ struct drm_atomic_state {
 	 * drm_atomic_crtc_needs_modeset().
 	 */
 	bool allow_modeset : 1;
+	/**
+	 * @legacy_cursor_update:
+	 *
+	 * Hint to enforce legacy cursor IOCTL semantics.
+	 *
+	 * WARNING: This is thoroughly broken and pretty much impossible to
+	 * implement correctly. Drivers must ignore this and should instead
+	 * implement &drm_plane_helper_funcs.atomic_async_check and
+	 * &drm_plane_helper_funcs.atomic_async_commit hooks. New users of this
+	 * flag are not allowed.
+	 */
 	bool legacy_cursor_update : 1;
 	bool async_update : 1;
 	/**
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/3] drm/doc: Document legacy_cursor_update better
  2020-10-23 12:39 ` [PATCH 3/3] drm/doc: Document legacy_cursor_update better Daniel Vetter
@ 2020-10-27  9:36   ` Daniel Vetter
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2020-10-27  9:36 UTC (permalink / raw)
  To: DRI Development
  Cc: David Airlie, Daniel Vetter, Intel Graphics Development,
	Thomas Zimmermann, Daniel Vetter, Sean Paul

On Fri, Oct 23, 2020 at 02:39:25PM +0200, Daniel Vetter wrote:
> It's the horror and shouldn't be used. Realized we're not clear on
> this in a discussion with Rob about what msm is doing to better
> support async commits.
> 
> v2: Refine existing todo item to include this (Thomas)
> 
> Cc: Sean Paul <sean@poorly.run>
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>

Pushed this one with Rob's irc-ack, since the functional changes will take
a bit longer to figure out.
-Daniel

> ---
>  Documentation/gpu/todo.rst |  4 ++++
>  include/drm/drm_atomic.h   | 12 +++++++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 700637e25ecd..6b224ef14455 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -105,6 +105,10 @@ converted over to the new infrastructure.
>  One issue with the helpers is that they require that drivers handle completion
>  events for atomic commits correctly. But fixing these bugs is good anyway.
>  
> +Somewhat related is the legacy_cursor_update hack, which should be replaced with
> +the new atomic_async_check/commit functionality in the helpers in drivers that
> +still look at that flag.
> +
>  Contact: Daniel Vetter, respective driver maintainers
>  
>  Level: Advanced
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index d07c851d255b..413fd0ca56a8 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -308,7 +308,6 @@ struct __drm_private_objs_state {
>   * struct drm_atomic_state - the global state object for atomic updates
>   * @ref: count of all references to this state (will not be freed until zero)
>   * @dev: parent DRM device
> - * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
>   * @async_update: hint for asynchronous plane update
>   * @planes: pointer to array of structures with per-plane data
>   * @crtcs: pointer to array of CRTC pointers
> @@ -336,6 +335,17 @@ struct drm_atomic_state {
>  	 * drm_atomic_crtc_needs_modeset().
>  	 */
>  	bool allow_modeset : 1;
> +	/**
> +	 * @legacy_cursor_update:
> +	 *
> +	 * Hint to enforce legacy cursor IOCTL semantics.
> +	 *
> +	 * WARNING: This is thoroughly broken and pretty much impossible to
> +	 * implement correctly. Drivers must ignore this and should instead
> +	 * implement &drm_plane_helper_funcs.atomic_async_check and
> +	 * &drm_plane_helper_funcs.atomic_async_commit hooks. New users of this
> +	 * flag are not allowed.
> +	 */
>  	bool legacy_cursor_update : 1;
>  	bool async_update : 1;
>  	/**
> -- 
> 2.28.0
> 

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] drm/atomic-helpers: remove legacy_cursor_update hacks
  2020-10-22 13:36 ` Kazlauskas, Nicholas
@ 2020-10-22 14:03   ` Daniel Vetter
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2020-10-22 14:03 UTC (permalink / raw)
  To: Kazlauskas, Nicholas
  Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
	Daniel Vetter, mikita.lipski, Michel Dänzer

On Thu, Oct 22, 2020 at 09:36:23AM -0400, Kazlauskas, Nicholas wrote:
> On 2020-10-21 12:32 p.m., Daniel Vetter wrote:
> > The stuff never really worked, and leads to lots of fun because it
> > out-of-order frees atomic states. Which upsets KASAN, among other
> > things.
> > 
> > For async updates we now have a more solid solution with the
> > ->atomic_async_check and ->atomic_async_commit hooks. Support for that
> > for msm and vc4 landed. nouveau and i915 have their own commit
> > routines, doing something similar.
> > 
> > For everyone else it's probably better to remove the use-after-free
> > bug, and encourage folks to use the async support instead. The
> > affected drivers which register a legacy cursor plane and don't either
> > use the new async stuff or their own commit routine are: amdgpu,
> > atmel, mediatek, qxl, rockchip, sti, sun4i, tegra, virtio, and vmwgfx.
> > 
> > Inspired by an amdgpu bug report.
> > 
> > v2: Drop RFC, I think with amdgpu converted over to use
> > atomic_async_check/commit done in
> > 
> > commit 674e78acae0dfb4beb56132e41cbae5b60f7d662
> > Author: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> > Date:   Wed Dec 5 14:59:07 2018 -0500
> > 
> >      drm/amd/display: Add fast path for cursor plane updates
> > 
> > we don't have any driver anymore where we have userspace expecting
> > solid legacy cursor support _and_ they are using the atomic helpers in
> > their fully glory. So we can retire this.
> > 
> > References: https://bugzilla.kernel.org/show_bug.cgi?id=199425
> > Cc: mikita.lipski@amd.com
> > Cc: Michel Dänzer <michel@daenzer.net>
> > Cc: harry.wentland@amd.com
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> I'm fine with the idea but it looks like we need modification to amdgpu to
> not break anything:
> 
> if (state->legacy_cursor_update) {
> /* ... */
> 		state->async_update =
> 			!drm_atomic_helper_async_check(dev, state);
> 
> 
> We only check async update for legacy_cursor_updates here which won't cover
> the atomic path. I think it's safe to drop the check here but that should
> probably be done before or as part of this series.

This part is fine, you're essentially duplicating what the helpers are
doing too. I'm not sure whether we should lift this to core atomic
semantics or something else, but should be all ok as-is. Might still be
good to test this in case something isn't 100% complete and amdgpu atomic
commit still relies on legacy_cursor_update semantics somewhere.

But after this patch your atomic code and atomic helpers check/commit
functions match (I think), so we /should/ be good.

Cheers, Daniel

> 
> Regards,
> Nicholas Kazlauskas
> 
> > ---
> >   drivers/gpu/drm/drm_atomic_helper.c | 13 -------------
> >   1 file changed, 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index a7bcb4b4586c..549a31e6042c 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1481,13 +1481,6 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
> >   	int i, ret;
> >   	unsigned crtc_mask = 0;
> > -	 /*
> > -	  * Legacy cursor ioctls are completely unsynced, and userspace
> > -	  * relies on that (by doing tons of cursor updates).
> > -	  */
> > -	if (old_state->legacy_cursor_update)
> > -		return;
> > -
> >   	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> >   		if (!new_crtc_state->active)
> >   			continue;
> > @@ -2106,12 +2099,6 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> >   			continue;
> >   		}
> > -		/* Legacy cursor updates are fully unsynced. */
> > -		if (state->legacy_cursor_update) {
> > -			complete_all(&commit->flip_done);
> > -			continue;
> > -		}
> > -
> >   		if (!new_crtc_state->event) {
> >   			commit->event = kzalloc(sizeof(*commit->event),
> >   						GFP_KERNEL);
> > 
> 

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] drm/atomic-helpers: remove legacy_cursor_update hacks
  2020-10-21 16:32 [PATCH 1/3] drm/atomic-helpers: remove legacy_cursor_update hacks Daniel Vetter
@ 2020-10-22 13:36 ` Kazlauskas, Nicholas
  2020-10-22 14:03   ` Daniel Vetter
  0 siblings, 1 reply; 7+ messages in thread
From: Kazlauskas, Nicholas @ 2020-10-22 13:36 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: Daniel Vetter, Intel Graphics Development, mikita.lipski,
	Michel Dänzer

On 2020-10-21 12:32 p.m., Daniel Vetter wrote:
> The stuff never really worked, and leads to lots of fun because it
> out-of-order frees atomic states. Which upsets KASAN, among other
> things.
> 
> For async updates we now have a more solid solution with the
> ->atomic_async_check and ->atomic_async_commit hooks. Support for that
> for msm and vc4 landed. nouveau and i915 have their own commit
> routines, doing something similar.
> 
> For everyone else it's probably better to remove the use-after-free
> bug, and encourage folks to use the async support instead. The
> affected drivers which register a legacy cursor plane and don't either
> use the new async stuff or their own commit routine are: amdgpu,
> atmel, mediatek, qxl, rockchip, sti, sun4i, tegra, virtio, and vmwgfx.
> 
> Inspired by an amdgpu bug report.
> 
> v2: Drop RFC, I think with amdgpu converted over to use
> atomic_async_check/commit done in
> 
> commit 674e78acae0dfb4beb56132e41cbae5b60f7d662
> Author: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Date:   Wed Dec 5 14:59:07 2018 -0500
> 
>      drm/amd/display: Add fast path for cursor plane updates
> 
> we don't have any driver anymore where we have userspace expecting
> solid legacy cursor support _and_ they are using the atomic helpers in
> their fully glory. So we can retire this.
> 
> References: https://bugzilla.kernel.org/show_bug.cgi?id=199425
> Cc: mikita.lipski@amd.com
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: harry.wentland@amd.com
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

I'm fine with the idea but it looks like we need modification to amdgpu 
to not break anything:

if (state->legacy_cursor_update) {
/* ... */
		state->async_update =
			!drm_atomic_helper_async_check(dev, state);


We only check async update for legacy_cursor_updates here which won't 
cover the atomic path. I think it's safe to drop the check here but that 
should probably be done before or as part of this series.

Regards,
Nicholas Kazlauskas

> ---
>   drivers/gpu/drm/drm_atomic_helper.c | 13 -------------
>   1 file changed, 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index a7bcb4b4586c..549a31e6042c 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1481,13 +1481,6 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
>   	int i, ret;
>   	unsigned crtc_mask = 0;
>   
> -	 /*
> -	  * Legacy cursor ioctls are completely unsynced, and userspace
> -	  * relies on that (by doing tons of cursor updates).
> -	  */
> -	if (old_state->legacy_cursor_update)
> -		return;
> -
>   	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
>   		if (!new_crtc_state->active)
>   			continue;
> @@ -2106,12 +2099,6 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>   			continue;
>   		}
>   
> -		/* Legacy cursor updates are fully unsynced. */
> -		if (state->legacy_cursor_update) {
> -			complete_all(&commit->flip_done);
> -			continue;
> -		}
> -
>   		if (!new_crtc_state->event) {
>   			commit->event = kzalloc(sizeof(*commit->event),
>   						GFP_KERNEL);
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/3] drm/atomic-helpers: remove legacy_cursor_update hacks
@ 2020-10-21 16:32 Daniel Vetter
  2020-10-22 13:36 ` Kazlauskas, Nicholas
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel Vetter @ 2020-10-21 16:32 UTC (permalink / raw)
  To: DRI Development
  Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter,
	mikita.lipski, Michel Dänzer

The stuff never really worked, and leads to lots of fun because it
out-of-order frees atomic states. Which upsets KASAN, among other
things.

For async updates we now have a more solid solution with the
->atomic_async_check and ->atomic_async_commit hooks. Support for that
for msm and vc4 landed. nouveau and i915 have their own commit
routines, doing something similar.

For everyone else it's probably better to remove the use-after-free
bug, and encourage folks to use the async support instead. The
affected drivers which register a legacy cursor plane and don't either
use the new async stuff or their own commit routine are: amdgpu,
atmel, mediatek, qxl, rockchip, sti, sun4i, tegra, virtio, and vmwgfx.

Inspired by an amdgpu bug report.

v2: Drop RFC, I think with amdgpu converted over to use
atomic_async_check/commit done in

commit 674e78acae0dfb4beb56132e41cbae5b60f7d662
Author: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Date:   Wed Dec 5 14:59:07 2018 -0500

    drm/amd/display: Add fast path for cursor plane updates

we don't have any driver anymore where we have userspace expecting
solid legacy cursor support _and_ they are using the atomic helpers in
their fully glory. So we can retire this.

References: https://bugzilla.kernel.org/show_bug.cgi?id=199425
Cc: mikita.lipski@amd.com
Cc: Michel Dänzer <michel@daenzer.net>
Cc: harry.wentland@amd.com
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index a7bcb4b4586c..549a31e6042c 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1481,13 +1481,6 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 	int i, ret;
 	unsigned crtc_mask = 0;
 
-	 /*
-	  * Legacy cursor ioctls are completely unsynced, and userspace
-	  * relies on that (by doing tons of cursor updates).
-	  */
-	if (old_state->legacy_cursor_update)
-		return;
-
 	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
 		if (!new_crtc_state->active)
 			continue;
@@ -2106,12 +2099,6 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 			continue;
 		}
 
-		/* Legacy cursor updates are fully unsynced. */
-		if (state->legacy_cursor_update) {
-			complete_all(&commit->flip_done);
-			continue;
-		}
-
 		if (!new_crtc_state->event) {
 			commit->event = kzalloc(sizeof(*commit->event),
 						GFP_KERNEL);
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-10-27  9:36 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-23 12:39 [PATCH 1/3] drm/atomic-helpers: remove legacy_cursor_update hacks Daniel Vetter
2020-10-23 12:39 ` [PATCH 2/3] drm/vc4: Drop legacy_cursor_update override Daniel Vetter
2020-10-23 12:39 ` [PATCH 3/3] drm/doc: Document legacy_cursor_update better Daniel Vetter
2020-10-27  9:36   ` Daniel Vetter
  -- strict thread matches above, loose matches on Subject: below --
2020-10-21 16:32 [PATCH 1/3] drm/atomic-helpers: remove legacy_cursor_update hacks Daniel Vetter
2020-10-22 13:36 ` Kazlauskas, Nicholas
2020-10-22 14:03   ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).