Hi Maxime
On Wed, 17 Nov 2021 at 09:45, Maxime Ripard <maxime@xxxxxxxxxx> wrote:
>
> Our current code is supposed to serialise the commits by waiting for all
> the drm_crtc_commits associated to the previous HVS state.
>
> However, assuming we have two CRTCs running and being configured and we
> configure each one alternatively, we end up in a situation where we're
s/alternatively/alternately
Otherwise the series looks fine to the extent that I understand the issues.
So the series is
Reviewed-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> not waiting at all.
>
> Indeed, starting with a state (state 0) where both CRTCs are running,
> and doing a commit (state 1) on the first CRTC (CRTC 0), we'll associate
> its commit to its assigned FIFO in vc4_hvs_state.
>
> If we get a new commit (state 2), this time affecting the second CRTC
> (CRTC 1), the DRM core will allow both commits to execute in parallel
> (assuming they don't have any share resources).
>
> Our code in vc4_atomic_commit_tail is supposed to make sure we only get
> one commit at a time and serialised by order of submission. It does so
> by using for_each_old_crtc_in_state, making sure that the CRTC has a
> FIFO assigned, is used, and has a commit pending. If it does, then we'll
> wait for the commit before going forward.
>
> During the transition from state 0 to state 1, as our old CRTC state we
> get the CRTC 0 state 0, its commit, we wait for it, everything works fine.
>
> During the transition from state 1 to state 2 though, the use of
> for_each_old_crtc_in_state is wrong. Indeed, while the code assumes it's
> returning the state of the CRTC in the old state (so CRTC 0 state 1), it
> actually returns the old state of the CRTC affected by the current
> commit, so CRTC 0 state 0 since it wasn't part of state 1.
>
> Due to this, if we alternate between the configuration of CRTC 0 and
> CRTC 1, we never actually wait for anything since we should be waiting
> on the other every time, but it never is affected by the previous
> commit.
>
> Change the logic to, at every commit, look at every FIFO in the previous
> HVS state, and if it's in use and has a commit associated to it, wait
> for that commit.
>
> Fixes: 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a commit")
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> ---
> drivers/gpu/drm/vc4/vc4_kms.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index d9b3e3ad71ea..b61792d2aa65 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -337,10 +337,10 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
> struct drm_device *dev = state->dev;
> struct vc4_dev *vc4 = to_vc4_dev(dev);
> struct vc4_hvs *hvs = vc4->hvs;
> - struct drm_crtc_state *old_crtc_state;
> struct drm_crtc_state *new_crtc_state;
> struct drm_crtc *crtc;
> struct vc4_hvs_state *old_hvs_state;
> + unsigned int channel;
> int i;
>
> for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> @@ -357,16 +357,10 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
> if (IS_ERR(old_hvs_state))
> return;
>
> - for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
> - struct vc4_crtc_state *vc4_crtc_state =
> - to_vc4_crtc_state(old_crtc_state);
> - unsigned int channel = vc4_crtc_state->assigned_channel;
> + for (channel = 0; channel < HVS_NUM_CHANNELS; channel++) {
> struct drm_crtc_commit *commit;
> int ret;
>
> - if (channel == VC4_HVS_CHANNEL_DISABLED)
> - continue;
> -
> if (!old_hvs_state->fifo_state[channel].in_use)
> continue;
>
> --
> 2.33.1
>
