| Message ID | 20201021163242.1458885-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/atomic-helpers: remove legacy_cursor_update hacks | expand |
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); >
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); > > >
On Wed, Oct 21, 2020 at 9:32 AM Daniel Vetter <daniel.vetter@ffwll.ch> 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> This *completely* destroys fps when there is cursor movement, it would be a pretty bad regression, so nak BR, -R > --- > 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 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 22, 2020 at 10:02 AM Rob Clark <robdclark@gmail.com> wrote: > > On Wed, Oct 21, 2020 at 9:32 AM Daniel Vetter <daniel.vetter@ffwll.ch> 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> > > This *completely* destroys fps when there is cursor movement, it would > be a pretty bad regression, so nak Which I *guess* is due to dpu not wiring up the plane->async_* funcs, effectively making cursor updates synchronous.. but it will take some time to sort out :-( > BR, > -R > > > --- > > 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 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 22, 2020 at 7:22 PM Rob Clark <robdclark@gmail.com> wrote: > > On Thu, Oct 22, 2020 at 10:02 AM Rob Clark <robdclark@gmail.com> wrote: > > > > On Wed, Oct 21, 2020 at 9:32 AM Daniel Vetter <daniel.vetter@ffwll.ch> 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> > > > > This *completely* destroys fps when there is cursor movement, it would > > be a pretty bad regression, so nak > > Which I *guess* is due to dpu not wiring up the plane->async_* funcs, > effectively making cursor updates synchronous.. but it will take some > time to sort out :-( Does something like the below (not even compile tested) get dpu back in order? diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index 561bfa48841c..ec8b4f74da49 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->flip_done); + /* * Start timer if we don't already have an update pending * on this crtc: That way we could perhaps still move ahead with removing the hacks from shared helpers, and msm-dpu can keep doing what it does. The other hunk is in a function that dpu code doesn't even use, so can't see how that would change anything. -Daniel > > > BR, > > -R > > > > > --- > > > 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 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 22, 2020 at 12:16 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > On Thu, Oct 22, 2020 at 7:22 PM Rob Clark <robdclark@gmail.com> wrote: > > > > On Thu, Oct 22, 2020 at 10:02 AM Rob Clark <robdclark@gmail.com> wrote: > > > > > > On Wed, Oct 21, 2020 at 9:32 AM Daniel Vetter <daniel.vetter@ffwll.ch> 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> > > > > > > This *completely* destroys fps when there is cursor movement, it would > > > be a pretty bad regression, so nak > > > > Which I *guess* is due to dpu not wiring up the plane->async_* funcs, > > effectively making cursor updates synchronous.. but it will take some > > time to sort out :-( > > Does something like the below (not even compile tested) get dpu back in order? > > diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c > index 561bfa48841c..ec8b4f74da49 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->flip_done); > + > /* > * Start timer if we don't already have an update pending > * on this crtc: > > That way we could perhaps still move ahead with removing the hacks > from shared helpers, and msm-dpu can keep doing what it does. The > other hunk is in a function that dpu code doesn't even use, so can't > see how that would change anything. That causes massive explosions... angering WARN_ON(dpu_crtc->event); Seems it is probably the right idea but the wrong place? I'll try to make some time in next few days to look at this more, but juggling a bunch of different things atm (and I guess at any rate this won't be a 5.10 thing, so we have a bit of time) BR, -R > -Daniel > > > > > > BR, > > > -R > > > > > > > --- > > > > 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 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Fri, Oct 23, 2020 at 5:02 PM Rob Clark <robdclark@gmail.com> wrote: > > On Thu, Oct 22, 2020 at 12:16 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > On Thu, Oct 22, 2020 at 7:22 PM Rob Clark <robdclark@gmail.com> wrote: > > > > > > On Thu, Oct 22, 2020 at 10:02 AM Rob Clark <robdclark@gmail.com> wrote: > > > > > > > > On Wed, Oct 21, 2020 at 9:32 AM Daniel Vetter <daniel.vetter@ffwll.ch> 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> > > > > > > > > This *completely* destroys fps when there is cursor movement, it would > > > > be a pretty bad regression, so nak > > > > > > Which I *guess* is due to dpu not wiring up the plane->async_* funcs, > > > effectively making cursor updates synchronous.. but it will take some > > > time to sort out :-( > > > > Does something like the below (not even compile tested) get dpu back in order? > > > > diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c > > index 561bfa48841c..ec8b4f74da49 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->flip_done); > > + > > /* > > * Start timer if we don't already have an update pending > > * on this crtc: > > > > That way we could perhaps still move ahead with removing the hacks > > from shared helpers, and msm-dpu can keep doing what it does. The > > other hunk is in a function that dpu code doesn't even use, so can't > > see how that would change anything. > > That causes massive explosions... angering WARN_ON(dpu_crtc->event); > > Seems it is probably the right idea but the wrong place? I'll try to > make some time in next few days to look at this more, but juggling a > bunch of different things atm (and I guess at any rate this won't be a > 5.10 thing, so we have a bit of time) Yeah we have time for this, legacy_cursor_update being a mistake in atomic has been a thorn to my ego for years, it's not going to get worse with a bit more time itching :-) It's more that I want to really make sure we don't spread this further, since the hacks in atomic helpers really break the entire commit helper model quite badly all over. So maybe just ack on the documentation patch interim, while we figure out something at leasure? I've also broken i915, so maybe I figure out meanwhile how to reapply the duct-tape there. -Daniel > BR, > -R > > > -Daniel > > > > > > > > > BR, > > > > -R > > > > > > > > > --- > > > > > 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 > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
On Fri, Oct 23, 2020 at 9:00 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > On Fri, Oct 23, 2020 at 5:02 PM Rob Clark <robdclark@gmail.com> wrote: > > > > On Thu, Oct 22, 2020 at 12:16 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > > > On Thu, Oct 22, 2020 at 7:22 PM Rob Clark <robdclark@gmail.com> wrote: > > > > > > > > On Thu, Oct 22, 2020 at 10:02 AM Rob Clark <robdclark@gmail.com> wrote: > > > > > > > > > > On Wed, Oct 21, 2020 at 9:32 AM Daniel Vetter <daniel.vetter@ffwll.ch> 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> > > > > > > > > > > This *completely* destroys fps when there is cursor movement, it would > > > > > be a pretty bad regression, so nak > > > > > > > > Which I *guess* is due to dpu not wiring up the plane->async_* funcs, > > > > effectively making cursor updates synchronous.. but it will take some > > > > time to sort out :-( > > > > > > Does something like the below (not even compile tested) get dpu back in order? > > > > > > diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c > > > index 561bfa48841c..ec8b4f74da49 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->flip_done); > > > + > > > /* > > > * Start timer if we don't already have an update pending > > > * on this crtc: > > > > > > That way we could perhaps still move ahead with removing the hacks > > > from shared helpers, and msm-dpu can keep doing what it does. The > > > other hunk is in a function that dpu code doesn't even use, so can't > > > see how that would change anything. > > > > That causes massive explosions... angering WARN_ON(dpu_crtc->event); > > > > Seems it is probably the right idea but the wrong place? I'll try to > > make some time in next few days to look at this more, but juggling a > > bunch of different things atm (and I guess at any rate this won't be a > > 5.10 thing, so we have a bit of time) > > Yeah we have time for this, legacy_cursor_update being a mistake in > atomic has been a thorn to my ego for years, it's not going to get > worse with a bit more time itching :-) It's more that I want to really > make sure we don't spread this further, since the hacks in atomic > helpers really break the entire commit helper model quite badly all > over. > > So maybe just ack on the documentation patch interim, while we figure > out something at leasure? I've also broken i915, so maybe I figure out > meanwhile how to reapply the duct-tape there. yeah, makes sense, a-b for doc patch BR, -R > -Daniel > > > BR, > > -R > > > > > -Daniel > > > > > > > > > > > > BR, > > > > > -R > > > > > > > > > > > --- > > > > > > 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 > > > > > > > > > > > > _______________________________________________ > > > > > > Intel-gfx mailing list > > > > > > Intel-gfx@lists.freedesktop.org > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
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);
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(-)