[PATCH 3/3] drm/amd/display: Don't replace the dc_state for fast updates

[Francis, David]

Series is:

Reviewed-by: David Francis <david.francis at amd.com>

________________________________
From: Alex Deucher <alexdeucher at gmail.com>
Sent: August 1, 2019 2:58:56 PM
To: Kazlauskas, Nicholas <Nicholas.Kazlauskas at amd.com>
Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>; Li, Sun peng (Leo) <Sunpeng.Li at amd.com>; Francis, David <David.Francis at amd.com>
Subject: Re: [PATCH 3/3] drm/amd/display: Don't replace the dc_state for fast updates

On Wed, Jul 31, 2019 at 12:26 PM Nicholas Kazlauskas
<nicholas.kazlauskas at amd.com> wrote:
>
> [Why]
> DRM private objects have no hw_done/flip_done fencing mechanism on their
> own and cannot be used to sequence commits accordingly.
>
> When issuing commits that don't touch the same set of hardware resources
> like page-flips on different CRTCs we can run into the issue below
> because of this:
>
> 1. Client requests non-blocking Commit #1, has a new dc_state #1,
> state is swapped, commit tail is deferred to work queue
>
> 2. Client requests non-blocking Commit #2, has a new dc_state #2,
> state is swapped, commit tail is deferred to work queue
>
> 3. Commit #2 work starts, commit tail finishes,
> atomic state is cleared, dc_state #1 is freed
>
> 4. Commit #1 work starts,
> commit tail encounters null pointer deref on dc_state #1
>
> In order to change the DC state as in the private object we need to
> ensure that we wait for all outstanding commits to finish and that
> any other pending commits must wait for the current one to finish as
> well.
>
> We do this for MEDIUM and FULL updates. But not for FAST updates, nor
> would we want to since it would cause stuttering from the delays.
>
> FAST updates that go through dm_determine_update_type_for_commit always
> create a new dc_state and lock the DRM private object if there are
> any changed planes.
>
> We need the old state to validate, but we don't actually need the new
> state here.
>
> [How]
> If the commit isn't a full update then the use after free can be
> resolved by simply discarding the new state entirely and retaining
> the existing one instead.
>
> With this change the sequence above can be reexamined. Commit #2 will
> still free Commit #1's reference, but before this happens we actually
> added an additional reference as part of Commit #2.
>
> If an update comes in during this that needs to change the dc_state
> it will need to wait on Commit #1 and Commit #2 to finish. Then it'll
> swap the state, finish the work in commit tail and drop the last
> reference on Commit #2's dc_state.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204181
> Cc: Leo Li <sunpeng.li at amd.com>
> Cc: David Francis <david.francis at amd.com>
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>

Series is:
Acked-by: Alex Deucher <alexander.deucher at amd.com>

> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 4c90662e9fa2..fe5291b16193 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -7288,6 +7288,29 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>                         ret = -EINVAL;
>                         goto fail;
>                 }
> +       } else {
> +               /*
> +                * The commit is a fast update. Fast updates shouldn't change
> +                * the DC context, affect global validation, and can have their
> +                * commit work done in parallel with other commits not touching
> +                * the same resource. If we have a new DC context as part of
> +                * the DM atomic state from validation we need to free it and
> +                * retain the existing one instead.
> +                */
> +               struct dm_atomic_state *new_dm_state, *old_dm_state;
> +
> +               new_dm_state = dm_atomic_get_new_state(state);
> +               old_dm_state = dm_atomic_get_old_state(state);
> +
> +               if (new_dm_state && old_dm_state) {
> +                       if (new_dm_state->context)
> +                               dc_release_state(new_dm_state->context);
> +
> +                       new_dm_state->context = old_dm_state->context;
> +
> +                       if (old_dm_state->context)
> +                               dc_retain_state(old_dm_state->context);
> +               }
>         }
>
>         /* Must be success */
> --
> 2.17.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20190801/43e291e5/attachment-0001.html>