On Wed, Jul 31, 2019 at 12:26 PM Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx> 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@xxxxxxx> > Cc: David Francis <david.francis@xxxxxxx> > Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx> Series is: Acked-by: Alex Deucher <alexander.deucher@xxxxxxx> > --- > .../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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
