Re: [Intel-gfx] [PATCH v2 5/6] drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.

[Maarten Lankhorst]

Op 07-09-17 om 12:05 schreef Daniel Vetter:
> On Mon, Sep 04, 2017 at 12:48:37PM +0200, Maarten Lankhorst wrote:
>> Currently we neatly track the crtc state, but forget to look at
>> plane/connector state.
>>
>> When doing a nonblocking modeset, immediately followed by a setprop
>> before the modeset completes, the setprop will see the modesets new
>> state as the old state and free it.
>>
>> This has to be solved by waiting for hw_done on the connector, even
>> if it's not assigned to a crtc. When a connector is unbound we take
>> the last crtc commit, and when it stays unbound we create a new
>> fake crtc commit for that gets signaled on hw_done for all the
>> planes/connectors.
>>
>> We wait for it the same way as we do for crtc's, which will make
>> sure we never run into a use-after-free situation.
>>
>> Changes since v1:
>> - Only create a single disable commit. (danvet)
>> - Fix leak in intel_legacy_cursor_update.
>> Changes since v2:
>> - Make reference counting in drm_atomic_helper_setup_commit
>>   more obvious. (pinchartl)
>> - Call cleanup_done for fake commit. (danvet)
>> - Add comments to drm_atomic_helper_setup_commit. (danvet, pinchartl)
>> - Add comment to drm_atomic_helper_swap_state. (pinchartl)
>>
>> Signed-off-by: Maarten Lankhorst 
>> Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind*
>> Cc: Laurent Pinchart 
>> ---
>>  drivers/gpu/drm/drm_atomic.c |   4 +
>>  drivers/gpu/drm/drm_atomic_helper.c  | 172 
>> +--
>>  drivers/gpu/drm/i915/intel_display.c |   2 +
>>  include/drm/drm_atomic.h |  12 +++
>>  include/drm/drm_connector.h  |   7 ++
>>  include/drm/drm_plane.h  |   7 ++
>>  6 files changed, 198 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 2cce48f203e0..75f5f74de9bf 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -192,6 +192,10 @@ void drm_atomic_state_default_clear(struct 
>> drm_atomic_state *state)
>>  }
>>  state->num_private_objs = 0;
>>  
>> +if (state->fake_commit) {
>> +drm_crtc_commit_put(state->fake_commit);
>> +state->fake_commit = NULL;
>> +}
>>  }
>>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
>>  
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
>> b/drivers/gpu/drm/drm_atomic_helper.c
>> index 04629d883114..c81d46927a74 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1667,6 +1667,38 @@ static void release_crtc_commit(struct completion 
>> *completion)
>>  drm_crtc_commit_put(commit);
>>  }
>>  
>> +static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc 
>> *crtc)
>> +{
>> +init_completion(>flip_done);
>> +init_completion(>hw_done);
>> +init_completion(>cleanup_done);
>> +INIT_LIST_HEAD(>commit_entry);
>> +kref_init(>ref);
>> +commit->crtc = crtc;
>> +}
>> +
>> +static struct drm_crtc_commit *
>> +crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
> Bikeshed: Would be nice if this function directly increases the refcount,
> instead of imposing this on all callers. Would need a rename too like
> crtc_or_fake_commit_get().
>
> But since this bug is randomly killing our hsw CI and causing lots of
> noise better to push as-is and polish later on.
>
> Reviewed-by: Daniel Vetter 
I chose not to, to make it explicit that a extra refcount is used on the state 
object.
But sending one final version to trybot to make sure that things don't blow up 
with the merge conflicts in patch 6. :)
>> +{
>> +if (crtc) {
>> +struct drm_crtc_state *new_crtc_state;
>> +
>> +new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>> +
>> +return new_crtc_state->commit;
>> +}
>> +
>> +if (!state->fake_commit) {
>> +state->fake_commit = kzalloc(sizeof(*state->fake_commit), 
>> GFP_KERNEL);
>> +if (!state->fake_commit)
>> +return NULL;
>> +
>> +init_commit(state->fake_commit, NULL);
>> +}
>> +
>> +return state->fake_commit;
>> +}
>> +
>>  /**
>>   * drm_atomic_helper_setup_commit - setup possibly nonblocking commit
>>   * @state: new modeset state to be committed
>> @@ -1715,6 +1747,10 @@ int drm_atomic_helper_setup_commit(struct 
>> drm_atomic_state *state,
>>  {
>>  struct drm_crtc *crtc;
>>  struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>> +struct drm_connector *conn;
>> +struct drm_connector_state *old_conn_state, *new_conn_state;
>> +struct drm_plane *plane;
>> +struct drm_plane_state *old_plane_state, *new_plane_state;
>>  struct drm_crtc_commit *commit;
>>  int i, ret;
>>  
>> @@ -1723,12 +1759,7 @@ int drm_atomic_helper_setup_commit(struct 
>> drm_atomic_state *state,

Re: [Intel-gfx] [PATCH v2 5/6] drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.

[Daniel Vetter]

On Mon, Sep 04, 2017 at 12:48:37PM +0200, Maarten Lankhorst wrote:
> Currently we neatly track the crtc state, but forget to look at
> plane/connector state.
> 
> When doing a nonblocking modeset, immediately followed by a setprop
> before the modeset completes, the setprop will see the modesets new
> state as the old state and free it.
> 
> This has to be solved by waiting for hw_done on the connector, even
> if it's not assigned to a crtc. When a connector is unbound we take
> the last crtc commit, and when it stays unbound we create a new
> fake crtc commit for that gets signaled on hw_done for all the
> planes/connectors.
> 
> We wait for it the same way as we do for crtc's, which will make
> sure we never run into a use-after-free situation.
> 
> Changes since v1:
> - Only create a single disable commit. (danvet)
> - Fix leak in intel_legacy_cursor_update.
> Changes since v2:
> - Make reference counting in drm_atomic_helper_setup_commit
>   more obvious. (pinchartl)
> - Call cleanup_done for fake commit. (danvet)
> - Add comments to drm_atomic_helper_setup_commit. (danvet, pinchartl)
> - Add comment to drm_atomic_helper_swap_state. (pinchartl)
> 
> Signed-off-by: Maarten Lankhorst 
> Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind*
> Cc: Laurent Pinchart 
> ---
>  drivers/gpu/drm/drm_atomic.c |   4 +
>  drivers/gpu/drm/drm_atomic_helper.c  | 172 
> +--
>  drivers/gpu/drm/i915/intel_display.c |   2 +
>  include/drm/drm_atomic.h |  12 +++
>  include/drm/drm_connector.h  |   7 ++
>  include/drm/drm_plane.h  |   7 ++
>  6 files changed, 198 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 2cce48f203e0..75f5f74de9bf 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -192,6 +192,10 @@ void drm_atomic_state_default_clear(struct 
> drm_atomic_state *state)
>   }
>   state->num_private_objs = 0;
>  
> + if (state->fake_commit) {
> + drm_crtc_commit_put(state->fake_commit);
> + state->fake_commit = NULL;
> + }
>  }
>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
>  
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 04629d883114..c81d46927a74 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1667,6 +1667,38 @@ static void release_crtc_commit(struct completion 
> *completion)
>   drm_crtc_commit_put(commit);
>  }
>  
> +static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc 
> *crtc)
> +{
> + init_completion(>flip_done);
> + init_completion(>hw_done);
> + init_completion(>cleanup_done);
> + INIT_LIST_HEAD(>commit_entry);
> + kref_init(>ref);
> + commit->crtc = crtc;
> +}
> +
> +static struct drm_crtc_commit *
> +crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)

Bikeshed: Would be nice if this function directly increases the refcount,
instead of imposing this on all callers. Would need a rename too like
crtc_or_fake_commit_get().

But since this bug is randomly killing our hsw CI and causing lots of
noise better to push as-is and polish later on.

Reviewed-by: Daniel Vetter 

> +{
> + if (crtc) {
> + struct drm_crtc_state *new_crtc_state;
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +
> + return new_crtc_state->commit;
> + }
> +
> + if (!state->fake_commit) {
> + state->fake_commit = kzalloc(sizeof(*state->fake_commit), 
> GFP_KERNEL);
> + if (!state->fake_commit)
> + return NULL;
> +
> + init_commit(state->fake_commit, NULL);
> + }
> +
> + return state->fake_commit;
> +}
> +
>  /**
>   * drm_atomic_helper_setup_commit - setup possibly nonblocking commit
>   * @state: new modeset state to be committed
> @@ -1715,6 +1747,10 @@ int drm_atomic_helper_setup_commit(struct 
> drm_atomic_state *state,
>  {
>   struct drm_crtc *crtc;
>   struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> + struct drm_connector *conn;
> + struct drm_connector_state *old_conn_state, *new_conn_state;
> + struct drm_plane *plane;
> + struct drm_plane_state *old_plane_state, *new_plane_state;
>   struct drm_crtc_commit *commit;
>   int i, ret;
>  
> @@ -1723,12 +1759,7 @@ int drm_atomic_helper_setup_commit(struct 
> drm_atomic_state *state,
>   if (!commit)
>   return -ENOMEM;
>  
> - init_completion(>flip_done);
> - init_completion(>hw_done);
> - init_completion(>cleanup_done);
> - INIT_LIST_HEAD(>commit_entry);
> - kref_init(>ref);
> - commit->crtc = crtc;
> +