Hi Maxime
On Wed, 17 Nov 2021 at 09:45, Maxime Ripard maxime@cerno.tech 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@raspberrypi.com
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@cerno.tech
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