[PATCH] drm/atomic: swap_state should stall on cleanup_done

[PATCH] drm/atomic: swap_state should stall on cleanup_done

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Stalling on cleanup_done ensures that any atomic state related to a
nonblock commit no longer has dangling references to per-object state
that can be freed.

Otherwise, if a !nonblock commit completes after a nonblock commit has
swapped state (ie. the synchronous part of the nonblock commit comes
before the !nonblock commit), but before the asynchronous part of the
nonblock commit completes, what was the new per-object state in the
nonblock commit can be freed.

This shows up with the new self-refresh helper, as _update_avg_times()
dereferences the original old and new crtc_state.

Fixes: d4da4e33341c ("drm: Measure Self Refresh Entry/Exit times to avoid thrashing")
Cc: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
Other possibilities:
1) maybe block later before freeing atomic state?
2) refcount individual per-object state

 drivers/gpu/drm/drm_atomic_helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 3ef2ac52ce94..a5d95429f91b 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2711,7 +2711,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 			if (!commit)
 				continue;
 
-			ret = wait_for_completion_interruptible(&commit->hw_done);
+			ret = wait_for_completion_interruptible(&commit->cleanup_done);
 			if (ret)
 				return ret;
 		}
@@ -2722,7 +2722,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 			if (!commit)
 				continue;
 
-			ret = wait_for_completion_interruptible(&commit->hw_done);
+			ret = wait_for_completion_interruptible(&commit->cleanup_done);
 			if (ret)
 				return ret;
 		}
@@ -2733,7 +2733,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 			if (!commit)
 				continue;
 
-			ret = wait_for_completion_interruptible(&commit->hw_done);
+			ret = wait_for_completion_interruptible(&commit->cleanup_done);
 			if (ret)
 				return ret;
 		}
-- 
2.21.0

Re: [PATCH] drm/atomic: swap_state should stall on cleanup_done

[Maarten Lankhorst]

Op 31-10-2019 om 23:36 schreef Rob Clark:
> From: Rob Clark <robdclark@chromium.org>
>
> Stalling on cleanup_done ensures that any atomic state related to a
> nonblock commit no longer has dangling references to per-object state
> that can be freed.
>
> Otherwise, if a !nonblock commit completes after a nonblock commit has
> swapped state (ie. the synchronous part of the nonblock commit comes
> before the !nonblock commit), but before the asynchronous part of the
> nonblock commit completes, what was the new per-object state in the
> nonblock commit can be freed.
>
> This shows up with the new self-refresh helper, as _update_avg_times()
> dereferences the original old and new crtc_state.
>
> Fixes: d4da4e33341c ("drm: Measure Self Refresh Entry/Exit times to avoid thrashing")
> Cc: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
> Other possibilities:
> 1) maybe block later before freeing atomic state?
> 2) refcount individual per-object state
>
>  drivers/gpu/drm/drm_atomic_helper.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 3ef2ac52ce94..a5d95429f91b 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2711,7 +2711,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  			if (!commit)
>  				continue;
>  
> -			ret = wait_for_completion_interruptible(&commit->hw_done);
> +			ret = wait_for_completion_interruptible(&commit->cleanup_done);
>  			if (ret)
>  				return ret;
>  		}
> @@ -2722,7 +2722,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  			if (!commit)
>  				continue;
>  
> -			ret = wait_for_completion_interruptible(&commit->hw_done);
> +			ret = wait_for_completion_interruptible(&commit->cleanup_done);
>  			if (ret)
>  				return ret;
>  		}
> @@ -2733,7 +2733,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  			if (!commit)
>  				continue;
>  
> -			ret = wait_for_completion_interruptible(&commit->hw_done);
> +			ret = wait_for_completion_interruptible(&commit->cleanup_done);
>  			if (ret)
>  				return ret;
>  		}

Nack, hw_done means all new_crtc_state (from the old commit pov) dereferences are done.

Self refresh helpers should be fixed. :)

Re: [PATCH] drm/atomic: swap_state should stall on cleanup_done

[Rob Clark]

On Fri, Nov 1, 2019 at 7:47 AM Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
>
> Op 31-10-2019 om 23:36 schreef Rob Clark:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Stalling on cleanup_done ensures that any atomic state related to a
> > nonblock commit no longer has dangling references to per-object state
> > that can be freed.
> >
> > Otherwise, if a !nonblock commit completes after a nonblock commit has
> > swapped state (ie. the synchronous part of the nonblock commit comes
> > before the !nonblock commit), but before the asynchronous part of the
> > nonblock commit completes, what was the new per-object state in the
> > nonblock commit can be freed.
> >
> > This shows up with the new self-refresh helper, as _update_avg_times()
> > dereferences the original old and new crtc_state.
> >
> > Fixes: d4da4e33341c ("drm: Measure Self Refresh Entry/Exit times to avoid thrashing")
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> > Other possibilities:
> > 1) maybe block later before freeing atomic state?
> > 2) refcount individual per-object state
> >
> >  drivers/gpu/drm/drm_atomic_helper.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 3ef2ac52ce94..a5d95429f91b 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2711,7 +2711,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> >                       if (!commit)
> >                               continue;
> >
> > -                     ret = wait_for_completion_interruptible(&commit->hw_done);
> > +                     ret = wait_for_completion_interruptible(&commit->cleanup_done);
> >                       if (ret)
> >                               return ret;
> >               }
> > @@ -2722,7 +2722,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> >                       if (!commit)
> >                               continue;
> >
> > -                     ret = wait_for_completion_interruptible(&commit->hw_done);
> > +                     ret = wait_for_completion_interruptible(&commit->cleanup_done);
> >                       if (ret)
> >                               return ret;
> >               }
> > @@ -2733,7 +2733,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> >                       if (!commit)
> >                               continue;
> >
> > -                     ret = wait_for_completion_interruptible(&commit->hw_done);
> > +                     ret = wait_for_completion_interruptible(&commit->cleanup_done);
> >                       if (ret)
> >                               return ret;
> >               }
>
> Nack, hw_done means all new_crtc_state (from the old commit pov) dereferences are done.
>

hmm, it would be nice if the for_each_blah_in_state() iterators would
splat on incorrect usage, then..  it tool a while to track down what
was going wrong.  And Sean claimed the self refresh helpers worked for
him on rockchip/i915 (although I'm starting to suspect maybe he just
didn't have enough debug options enabled to poison freed memory?)

> Self refresh helpers should be fixed. :)

Looks like what they need out of crtc_state is pretty minimal, maybe
they could extract out crtc_state->self_refresh_active earlier..

BR,
-R

Re: [PATCH] drm/atomic: swap_state should stall on cleanup_done

[Maarten Lankhorst]

Op 01-11-2019 om 15:59 schreef Rob Clark:
> On Fri, Nov 1, 2019 at 7:47 AM Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>> Op 31-10-2019 om 23:36 schreef Rob Clark:
>>> From: Rob Clark <robdclark@chromium.org>
>>>
>>> Stalling on cleanup_done ensures that any atomic state related to a
>>> nonblock commit no longer has dangling references to per-object state
>>> that can be freed.
>>>
>>> Otherwise, if a !nonblock commit completes after a nonblock commit has
>>> swapped state (ie. the synchronous part of the nonblock commit comes
>>> before the !nonblock commit), but before the asynchronous part of the
>>> nonblock commit completes, what was the new per-object state in the
>>> nonblock commit can be freed.
>>>
>>> This shows up with the new self-refresh helper, as _update_avg_times()
>>> dereferences the original old and new crtc_state.
>>>
>>> Fixes: d4da4e33341c ("drm: Measure Self Refresh Entry/Exit times to avoid thrashing")
>>> Cc: Sean Paul <seanpaul@chromium.org>
>>> Signed-off-by: Rob Clark <robdclark@chromium.org>
>>> ---
>>> Other possibilities:
>>> 1) maybe block later before freeing atomic state?
>>> 2) refcount individual per-object state
>>>
>>>  drivers/gpu/drm/drm_atomic_helper.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>> index 3ef2ac52ce94..a5d95429f91b 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -2711,7 +2711,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>>>                       if (!commit)
>>>                               continue;
>>>
>>> -                     ret = wait_for_completion_interruptible(&commit->hw_done);
>>> +                     ret = wait_for_completion_interruptible(&commit->cleanup_done);
>>>                       if (ret)
>>>                               return ret;
>>>               }
>>> @@ -2722,7 +2722,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>>>                       if (!commit)
>>>                               continue;
>>>
>>> -                     ret = wait_for_completion_interruptible(&commit->hw_done);
>>> +                     ret = wait_for_completion_interruptible(&commit->cleanup_done);
>>>                       if (ret)
>>>                               return ret;
>>>               }
>>> @@ -2733,7 +2733,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>>>                       if (!commit)
>>>                               continue;
>>>
>>> -                     ret = wait_for_completion_interruptible(&commit->hw_done);
>>> +                     ret = wait_for_completion_interruptible(&commit->cleanup_done);
>>>                       if (ret)
>>>                               return ret;
>>>               }
>> Nack, hw_done means all new_crtc_state (from the old commit pov) dereferences are done.
>>
> hmm, it would be nice if the for_each_blah_in_state() iterators would
> splat on incorrect usage, then..  it tool a while to track down what
> was going wrong.  And Sean claimed the self refresh helpers worked for
> him on rockchip/i915 (although I'm starting to suspect maybe he just
> didn't have enough debug options enabled to poison freed memory?)
Could do a memset on the new arrays after hw_done?
>> Self refresh helpers should be fixed. :)
> Looks like what they need out of crtc_state is pretty minimal, maybe
> they could extract out crtc_state->self_refresh_active earlier..

Yeah, something like that would work. :)

Re: [PATCH] drm/atomic: swap_state should stall on cleanup_done

[Maarten Lankhorst]

Op 31-10-2019 om 23:36 schreef Rob Clark:
> From: Rob Clark <robdclark@chromium.org>
>
> Stalling on cleanup_done ensures that any atomic state related to a
> nonblock commit no longer has dangling references to per-object state
> that can be freed.
>
> Otherwise, if a !nonblock commit completes after a nonblock commit has
> swapped state (ie. the synchronous part of the nonblock commit comes
> before the !nonblock commit), but before the asynchronous part of the
> nonblock commit completes, what was the new per-object state in the
> nonblock commit can be freed.
>
> This shows up with the new self-refresh helper, as _update_avg_times()
> dereferences the original old and new crtc_state.
>
> Fixes: d4da4e33341c ("drm: Measure Self Refresh Entry/Exit times to avoid thrashing")
> Cc: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
> Other possibilities:
> 1) maybe block later before freeing atomic state?
> 2) refcount individual per-object state
>
>  drivers/gpu/drm/drm_atomic_helper.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 3ef2ac52ce94..a5d95429f91b 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2711,7 +2711,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  			if (!commit)
>  				continue;
>  
> -			ret = wait_for_completion_interruptible(&commit->hw_done);
> +			ret = wait_for_completion_interruptible(&commit->cleanup_done);
>  			if (ret)
>  				return ret;
>  		}
> @@ -2722,7 +2722,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  			if (!commit)
>  				continue;
>  
> -			ret = wait_for_completion_interruptible(&commit->hw_done);
> +			ret = wait_for_completion_interruptible(&commit->cleanup_done);
>  			if (ret)
>  				return ret;
>  		}
> @@ -2733,7 +2733,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  			if (!commit)
>  				continue;
>  
> -			ret = wait_for_completion_interruptible(&commit->hw_done);
> +			ret = wait_for_completion_interruptible(&commit->cleanup_done);
>  			if (ret)
>  				return ret;
>  		}

From setup_commit():

 * Completion of the hardware commit step must be signalled using
 * drm_atomic_helper_commit_hw_done(). After this step the driver is not allowed
 * to read or change any permanent software or hardware modeset state. The only
 * exception is state protected by other means than &drm_modeset_lock locks.
 * Only the free standing @state with pointers to the old state structures can
 * be inspected, e.g. to clean up old buffers using
 * drm_atomic_helper_cleanup_planes().

And the hw_done function says pretty much the same thing. :)