[PATCH v2 0/6] drm/vc4: kms: Misc fixes for HVS commits

[PATCH v2 0/6] drm/vc4: kms: Misc fixes for HVS commits

[Maxime Ripard]

Hi,

The conversion to DRM commit helpers (f3c420fe19f8, "drm/vc4: kms: Convert to
atomic helpers") introduced a number of issues in corner cases, most of them
showing themselves in the form of either a vblank timeout or use-after-free
error.

These patches should fix most of them, some of them still being debugged.

Maxime

Changes from v1:
  - Prevent a null pointer dereference

Maxime Ripard (6):
  drm/vc4: kms: Wait for the commit before increasing our clock rate
  drm/vc4: kms: Fix return code check
  drm/vc4: kms: Add missing drm_crtc_commit_put
  drm/vc4: kms: Clear the HVS FIFO commit pointer once done
  drm/vc4: kms: Don't duplicate pending commit
  drm/vc4: kms: Fix previous HVS commit wait

 drivers/gpu/drm/vc4/vc4_kms.c | 42 ++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

-- 
2.33.1

[PATCH v2 1/6] drm/vc4: kms: Wait for the commit before increasing our clock rate

[Maxime Ripard]

Several DRM/KMS atomic commits can run in parallel if they affect
different CRTC. These commits share the global HVS state, so we have
some code to make sure we run commits in sequence. This synchronization
code is one of the first thing that runs in vc4_atomic_commit_tail().

Another constraints we have is that we need to make sure the HVS clock
gets a boost during the commit. That code relies on clk_set_min_rate and
will remove the old minimum and set a new one. We also need another,
temporary, minimum for the duration of the commit.

The algorithm is thus to set a temporary minimum, drop the previous
one, do the commit, and finally set the minimum for the current mode.

However, the part that sets the temporary minimum and drops the older
one runs before the commit synchronization code.

Thus, under the proper conditions, we can end up mixing up the minimums
and ending up with the wrong one for our current step.

To avoid it, let's move the clock setup in the protected section.

Fixes: d7d96c00e585 ("drm/vc4: hvs: Boost the core clock during modeset")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index f0b3e4cf5bce..764ddb41a4ce 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -353,9 +353,6 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 		vc4_hvs_mask_underrun(dev, vc4_crtc_state->assigned_channel);
 	}
 
-	if (vc4->hvs->hvs5)
-		clk_set_min_rate(hvs->core_clk, 500000000);
-
 	old_hvs_state = vc4_hvs_get_old_global_state(state);
 	if (!old_hvs_state)
 		return;
@@ -377,6 +374,9 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 			drm_err(dev, "Timed out waiting for commit\n");
 	}
 
+	if (vc4->hvs->hvs5)
+		clk_set_min_rate(hvs->core_clk, 500000000);
+
 	drm_atomic_helper_commit_modeset_disables(dev, state);
 
 	vc4_ctm_commit(vc4, state);
-- 
2.33.1

[PATCH v2 2/6] drm/vc4: kms: Fix return code check

[Maxime Ripard]

The HVS global state functions return an error pointer, but in most
cases we check if it's NULL, possibly resulting in an invalid pointer
dereference.

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, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 764ddb41a4ce..3f780c195749 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -354,7 +354,7 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 	}
 
 	old_hvs_state = vc4_hvs_get_old_global_state(state);
-	if (!old_hvs_state)
+	if (IS_ERR(old_hvs_state))
 		return;
 
 	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
@@ -410,8 +410,8 @@ static int vc4_atomic_commit_setup(struct drm_atomic_state *state)
 	unsigned int i;
 
 	hvs_state = vc4_hvs_get_new_global_state(state);
-	if (!hvs_state)
-		return -EINVAL;
+	if (WARN_ON(IS_ERR(hvs_state)))
+		return PTR_ERR(hvs_state);
 
 	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		struct vc4_crtc_state *vc4_crtc_state =
@@ -762,8 +762,8 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev,
 	unsigned int i;
 
 	hvs_new_state = vc4_hvs_get_global_state(state);
-	if (!hvs_new_state)
-		return -EINVAL;
+	if (IS_ERR(hvs_new_state))
+		return PTR_ERR(hvs_new_state);
 
 	for (i = 0; i < ARRAY_SIZE(hvs_new_state->fifo_state); i++)
 		if (!hvs_new_state->fifo_state[i].in_use)
-- 
2.33.1

[PATCH v2 3/6] drm/vc4: kms: Add missing drm_crtc_commit_put

[Maxime Ripard]

Commit 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a
commit") introduced a global state for the HVS, with each FIFO storing
the current CRTC commit so that we can properly synchronize commits.

However, the refcounting was off and we thus ended up leaking the
drm_crtc_commit structure every commit. Add a drm_crtc_commit_put to
prevent the leakage.

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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 3f780c195749..7c1d0c3beba2 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -361,6 +361,7 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 		struct vc4_crtc_state *vc4_crtc_state =
 			to_vc4_crtc_state(old_crtc_state);
 		unsigned int channel = vc4_crtc_state->assigned_channel;
+		struct drm_crtc_commit *commit;
 		int ret;
 
 		if (channel == VC4_HVS_CHANNEL_DISABLED)
@@ -369,9 +370,15 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 		if (!old_hvs_state->fifo_state[channel].in_use)
 			continue;
 
-		ret = drm_crtc_commit_wait(old_hvs_state->fifo_state[channel].pending_commit);
+		commit = old_hvs_state->fifo_state[channel].pending_commit;
+		if (!commit)
+			continue;
+
+		ret = drm_crtc_commit_wait(commit);
 		if (ret)
 			drm_err(dev, "Timed out waiting for commit\n");
+
+		drm_crtc_commit_put(commit);
 	}
 
 	if (vc4->hvs->hvs5)
-- 
2.33.1

[PATCH v2 4/6] drm/vc4: kms: Clear the HVS FIFO commit pointer once done

[Maxime Ripard]

Commit 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a
commit") introduced a wait on the previous commit done on a given HVS
FIFO.

However, we never cleared that pointer once done. Since
drm_crtc_commit_put can free the drm_crtc_commit structure directly if
we were the last user, this means that it can lead to a use-after free
if we were to duplicate the state, and that stale pointer would even be
copied to the new state.

Set the pointer to NULL once we're done with the wait so that we don't
carry over a pointer to a free'd structure.

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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 7c1d0c3beba2..f80370e87e98 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -379,6 +379,7 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
 			drm_err(dev, "Timed out waiting for commit\n");
 
 		drm_crtc_commit_put(commit);
+		old_hvs_state->fifo_state[channel].pending_commit = NULL;
 	}
 
 	if (vc4->hvs->hvs5)
-- 
2.33.1

[PATCH v2 5/6] drm/vc4: kms: Don't duplicate pending commit

[Maxime Ripard]

Our HVS global state, when duplicated, will also copy the pointer to the
drm_crtc_commit (and increase the reference count) for each FIFO if the
pointer is not NULL.

However, our atomic_setup function will overwrite that pointer without
putting the reference back leading to a memory leak.

Since the commit is only relevant during the atomic commit process, it
doesn't make sense to duplicate the reference to the commit anyway.
Let's remove it.

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 | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index f80370e87e98..d9b3e3ad71ea 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -676,12 +676,6 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj)
 
 	for (i = 0; i < HVS_NUM_CHANNELS; i++) {
 		state->fifo_state[i].in_use = old_state->fifo_state[i].in_use;
-
-		if (!old_state->fifo_state[i].pending_commit)
-			continue;
-
-		state->fifo_state[i].pending_commit =
-			drm_crtc_commit_get(old_state->fifo_state[i].pending_commit);
 	}
 
 	return &state->base;
-- 
2.33.1

[PATCH v2 6/6] drm/vc4: kms: Fix previous HVS commit wait

[Maxime Ripard]

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
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

Re: [PATCH v2 0/6] drm/vc4: kms: Misc fixes for HVS commits

[Jian-Hong Pan]

Maxime Ripard <maxime@cerno.tech> 於 2021年11月17日 週三 下午5:45寫道：
>
> Hi,
>
> The conversion to DRM commit helpers (f3c420fe19f8, "drm/vc4: kms: Convert to
> atomic helpers") introduced a number of issues in corner cases, most of them
> showing themselves in the form of either a vblank timeout or use-after-free
> error.
>
> These patches should fix most of them, some of them still being debugged.
>
> Maxime
>
> Changes from v1:
>   - Prevent a null pointer dereference
>
> Maxime Ripard (6):
>   drm/vc4: kms: Wait for the commit before increasing our clock rate
>   drm/vc4: kms: Fix return code check
>   drm/vc4: kms: Add missing drm_crtc_commit_put
>   drm/vc4: kms: Clear the HVS FIFO commit pointer once done
>   drm/vc4: kms: Don't duplicate pending commit
>   drm/vc4: kms: Fix previous HVS commit wait
>
>  drivers/gpu/drm/vc4/vc4_kms.c | 42 ++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 23 deletions(-)

I tested the v2 patches based on latest mainline kernel with RPi 4B.
System can boot up into desktop environment.

Although it still hit the bug [1], which might be under debugging, I
think these patches LGTM.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=214991

Tested-by: Jian-Hong Pan <jhp@endlessos.org>

Re: [PATCH v2 0/6] drm/vc4: kms: Misc fixes for HVS commits

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1648 bytes --]

On Thu, Nov 18, 2021 at 02:42:58PM +0800, Jian-Hong Pan wrote:
> Maxime Ripard <maxime@cerno.tech> 於 2021年11月17日 週三 下午5:45寫道：
> > The conversion to DRM commit helpers (f3c420fe19f8, "drm/vc4: kms: Convert to
> > atomic helpers") introduced a number of issues in corner cases, most of them
> > showing themselves in the form of either a vblank timeout or use-after-free
> > error.
> >
> > These patches should fix most of them, some of them still being debugged.
> >
> > Maxime
> >
> > Changes from v1:
> >   - Prevent a null pointer dereference
> >
> > Maxime Ripard (6):
> >   drm/vc4: kms: Wait for the commit before increasing our clock rate
> >   drm/vc4: kms: Fix return code check
> >   drm/vc4: kms: Add missing drm_crtc_commit_put
> >   drm/vc4: kms: Clear the HVS FIFO commit pointer once done
> >   drm/vc4: kms: Don't duplicate pending commit
> >   drm/vc4: kms: Fix previous HVS commit wait
> >
> >  drivers/gpu/drm/vc4/vc4_kms.c | 42 ++++++++++++++++-------------------
> >  1 file changed, 19 insertions(+), 23 deletions(-)
> 
> I tested the v2 patches based on latest mainline kernel with RPi 4B.
> System can boot up into desktop environment.

So the thing that was broken initially isn't anymore?

> Although it still hit the bug [1], which might be under debugging, I
> think these patches LGTM.

The vblank timeout stuff is a symptom of various different bugs. Can you
share your setup, your config.txt, and what you're doing to trigger it?

> [1] https://bugzilla.kernel.org/show_bug.cgi?id=214991
> 
> Tested-by: Jian-Hong Pan <jhp@endlessos.org>

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 0/6] drm/vc4: kms: Misc fixes for HVS commits

[Jian-Hong Pan]

Maxime Ripard <maxime@cerno.tech> 於 2021年11月18日 週四 下午6:40寫道：
>
> On Thu, Nov 18, 2021 at 02:42:58PM +0800, Jian-Hong Pan wrote:
> > Maxime Ripard <maxime@cerno.tech> 於 2021年11月17日 週三 下午5:45寫道：
> > > The conversion to DRM commit helpers (f3c420fe19f8, "drm/vc4: kms: Convert to
> > > atomic helpers") introduced a number of issues in corner cases, most of them
> > > showing themselves in the form of either a vblank timeout or use-after-free
> > > error.
> > >
> > > These patches should fix most of them, some of them still being debugged.
> > >
> > > Maxime
> > >
> > > Changes from v1:
> > >   - Prevent a null pointer dereference
> > >
> > > Maxime Ripard (6):
> > >   drm/vc4: kms: Wait for the commit before increasing our clock rate
> > >   drm/vc4: kms: Fix return code check
> > >   drm/vc4: kms: Add missing drm_crtc_commit_put
> > >   drm/vc4: kms: Clear the HVS FIFO commit pointer once done
> > >   drm/vc4: kms: Don't duplicate pending commit
> > >   drm/vc4: kms: Fix previous HVS commit wait
> > >
> > >  drivers/gpu/drm/vc4/vc4_kms.c | 42 ++++++++++++++++-------------------
> > >  1 file changed, 19 insertions(+), 23 deletions(-)
> >
> > I tested the v2 patches based on latest mainline kernel with RPi 4B.
> > System can boot up into desktop environment.
>
> So the thing that was broken initially isn't anymore?

No.  It does not appear anymore.

> > Although it still hit the bug [1], which might be under debugging, I
> > think these patches LGTM.
>
> The vblank timeout stuff is a symptom of various different bugs. Can you
> share your setup, your config.txt, and what you're doing to trigger it?

I get the RPi boot firmware files from raspberrypi's repository at tag
1.20211029 [1]
And, make system boots:
RPi firmware -> U-Boot -> Linux kernel (aarch64) with corresponding DTB

The config.txt only has:
enable_uart=1
arm_64bit=1
kernel=uboot.bin

This bug can be reproduced with es2gears command easily.  May need to
wait it running a while.

Mesa: 21.2.2
libdrm: 2.4.107
xserver/wayland: 1.20.11  Using wayland

This bug happens with direct boot path as well:
RPi firmware -> Linux kernel (aarch64) with corresponding DTB

I tried with Endless OS and Ubuntu's RPi 4B images.
An easy setup is using Ubuntu 21.10 RPi 4B image [2].  Then, replace
the kernel & device tree blob and edit the config.txt.

[1] https://github.com/raspberrypi/firmware/tree/1.20211029/boot
[2] https://ubuntu.com/download/raspberry-pi

Jian-Hong Pan

> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=214991
> >
> > Tested-by: Jian-Hong Pan <jhp@endlessos.org>
>
> Thanks!
> Maxime

Re: [PATCH v2 0/6] drm/vc4: kms: Misc fixes for HVS commits

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2747 bytes --]

On Fri, Nov 19, 2021 at 06:24:34PM +0800, Jian-Hong Pan wrote:
> Maxime Ripard <maxime@cerno.tech> 於 2021年11月18日 週四 下午6:40寫道：
> >
> > On Thu, Nov 18, 2021 at 02:42:58PM +0800, Jian-Hong Pan wrote:
> > > Maxime Ripard <maxime@cerno.tech> 於 2021年11月17日 週三 下午5:45寫道：
> > > > The conversion to DRM commit helpers (f3c420fe19f8, "drm/vc4: kms: Convert to
> > > > atomic helpers") introduced a number of issues in corner cases, most of them
> > > > showing themselves in the form of either a vblank timeout or use-after-free
> > > > error.
> > > >
> > > > These patches should fix most of them, some of them still being debugged.
> > > >
> > > > Maxime
> > > >
> > > > Changes from v1:
> > > >   - Prevent a null pointer dereference
> > > >
> > > > Maxime Ripard (6):
> > > >   drm/vc4: kms: Wait for the commit before increasing our clock rate
> > > >   drm/vc4: kms: Fix return code check
> > > >   drm/vc4: kms: Add missing drm_crtc_commit_put
> > > >   drm/vc4: kms: Clear the HVS FIFO commit pointer once done
> > > >   drm/vc4: kms: Don't duplicate pending commit
> > > >   drm/vc4: kms: Fix previous HVS commit wait
> > > >
> > > >  drivers/gpu/drm/vc4/vc4_kms.c | 42 ++++++++++++++++-------------------
> > > >  1 file changed, 19 insertions(+), 23 deletions(-)
> > >
> > > I tested the v2 patches based on latest mainline kernel with RPi 4B.
> > > System can boot up into desktop environment.
> >
> > So the thing that was broken initially isn't anymore?
> 
> No.  It does not appear anymore.
> 
> > > Although it still hit the bug [1], which might be under debugging, I
> > > think these patches LGTM.
> >
> > The vblank timeout stuff is a symptom of various different bugs. Can you
> > share your setup, your config.txt, and what you're doing to trigger it?
> 
> I get the RPi boot firmware files from raspberrypi's repository at tag
> 1.20211029 [1]
> And, make system boots:
> RPi firmware -> U-Boot -> Linux kernel (aarch64) with corresponding DTB
> 
> The config.txt only has:
> enable_uart=1
> arm_64bit=1
> kernel=uboot.bin
> 
> This bug can be reproduced with es2gears command easily.  May need to
> wait it running a while.
> 
> Mesa: 21.2.2
> libdrm: 2.4.107
> xserver/wayland: 1.20.11  Using wayland
> 
> This bug happens with direct boot path as well:
> RPi firmware -> Linux kernel (aarch64) with corresponding DTB
> 
> I tried with Endless OS and Ubuntu's RPi 4B images.
> An easy setup is using Ubuntu 21.10 RPi 4B image [2].  Then, replace
> the kernel & device tree blob and edit the config.txt.

Does it still appear if you raise the core clock in the config.txt file
using: core_freq_min=600 ?

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 0/6] drm/vc4: kms: Misc fixes for HVS commits

[Jian-Hong Pan]

Maxime Ripard <maxime@cerno.tech> 於 2021年11月26日 週五 下午11:45寫道：
>
> On Fri, Nov 19, 2021 at 06:24:34PM +0800, Jian-Hong Pan wrote:
> > Maxime Ripard <maxime@cerno.tech> 於 2021年11月18日 週四 下午6:40寫道：
> > >
> > > On Thu, Nov 18, 2021 at 02:42:58PM +0800, Jian-Hong Pan wrote:
> > > > Maxime Ripard <maxime@cerno.tech> 於 2021年11月17日 週三 下午5:45寫道：
> > > > > The conversion to DRM commit helpers (f3c420fe19f8, "drm/vc4: kms: Convert to
> > > > > atomic helpers") introduced a number of issues in corner cases, most of them
> > > > > showing themselves in the form of either a vblank timeout or use-after-free
> > > > > error.
> > > > >
> > > > > These patches should fix most of them, some of them still being debugged.
> > > > >
> > > > > Maxime
> > > > >
> > > > > Changes from v1:
> > > > >   - Prevent a null pointer dereference
> > > > >
> > > > > Maxime Ripard (6):
> > > > >   drm/vc4: kms: Wait for the commit before increasing our clock rate
> > > > >   drm/vc4: kms: Fix return code check
> > > > >   drm/vc4: kms: Add missing drm_crtc_commit_put
> > > > >   drm/vc4: kms: Clear the HVS FIFO commit pointer once done
> > > > >   drm/vc4: kms: Don't duplicate pending commit
> > > > >   drm/vc4: kms: Fix previous HVS commit wait
> > > > >
> > > > >  drivers/gpu/drm/vc4/vc4_kms.c | 42 ++++++++++++++++-------------------
> > > > >  1 file changed, 19 insertions(+), 23 deletions(-)
> > > >
> > > > I tested the v2 patches based on latest mainline kernel with RPi 4B.
> > > > System can boot up into desktop environment.
> > >
> > > So the thing that was broken initially isn't anymore?
> >
> > No.  It does not appear anymore.
> >
> > > > Although it still hit the bug [1], which might be under debugging, I
> > > > think these patches LGTM.
> > >
> > > The vblank timeout stuff is a symptom of various different bugs. Can you
> > > share your setup, your config.txt, and what you're doing to trigger it?
> >
> > I get the RPi boot firmware files from raspberrypi's repository at tag
> > 1.20211029 [1]
> > And, make system boots:
> > RPi firmware -> U-Boot -> Linux kernel (aarch64) with corresponding DTB
> >
> > The config.txt only has:
> > enable_uart=1
> > arm_64bit=1
> > kernel=uboot.bin
> >
> > This bug can be reproduced with es2gears command easily.  May need to
> > wait it running a while.
> >
> > Mesa: 21.2.2
> > libdrm: 2.4.107
> > xserver/wayland: 1.20.11  Using wayland
> >
> > This bug happens with direct boot path as well:
> > RPi firmware -> Linux kernel (aarch64) with corresponding DTB
> >
> > I tried with Endless OS and Ubuntu's RPi 4B images.
> > An easy setup is using Ubuntu 21.10 RPi 4B image [2].  Then, replace
> > the kernel & device tree blob and edit the config.txt.
>
> Does it still appear if you raise the core clock in the config.txt file
> using: core_freq_min=600 ?

It still appears when I raise the core clock in the config.txt file:
core_freq_min=600

Jian-Hong Pan

Re: [PATCH v2 6/6] drm/vc4: kms: Fix previous HVS commit wait

[Dave Stevenson]

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
>

Re: [PATCH v2 0/6] drm/vc4: kms: Misc fixes for HVS commits

[Maxime Ripard]

On Wed, 17 Nov 2021 10:45:21 +0100, Maxime Ripard wrote:
> The conversion to DRM commit helpers (f3c420fe19f8, "drm/vc4: kms: Convert to
> atomic helpers") introduced a number of issues in corner cases, most of them
> showing themselves in the form of either a vblank timeout or use-after-free
> error.
> 
> These patches should fix most of them, some of them still being debugged.
> 
> [...]

Applied to drm/drm-misc (drm-misc-fixes).

Thanks!
Maxime

Re: [PATCH v2 0/6] drm/vc4: kms: Misc fixes for HVS commits

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3483 bytes --]

On Mon, Nov 29, 2021 at 04:31:57PM +0800, Jian-Hong Pan wrote:
> Maxime Ripard <maxime@cerno.tech> 於 2021年11月26日 週五 下午11:45寫道：
> >
> > On Fri, Nov 19, 2021 at 06:24:34PM +0800, Jian-Hong Pan wrote:
> > > Maxime Ripard <maxime@cerno.tech> 於 2021年11月18日 週四 下午6:40寫道：
> > > >
> > > > On Thu, Nov 18, 2021 at 02:42:58PM +0800, Jian-Hong Pan wrote:
> > > > > Maxime Ripard <maxime@cerno.tech> 於 2021年11月17日 週三 下午5:45寫道：
> > > > > > The conversion to DRM commit helpers (f3c420fe19f8, "drm/vc4: kms: Convert to
> > > > > > atomic helpers") introduced a number of issues in corner cases, most of them
> > > > > > showing themselves in the form of either a vblank timeout or use-after-free
> > > > > > error.
> > > > > >
> > > > > > These patches should fix most of them, some of them still being debugged.
> > > > > >
> > > > > > Maxime
> > > > > >
> > > > > > Changes from v1:
> > > > > >   - Prevent a null pointer dereference
> > > > > >
> > > > > > Maxime Ripard (6):
> > > > > >   drm/vc4: kms: Wait for the commit before increasing our clock rate
> > > > > >   drm/vc4: kms: Fix return code check
> > > > > >   drm/vc4: kms: Add missing drm_crtc_commit_put
> > > > > >   drm/vc4: kms: Clear the HVS FIFO commit pointer once done
> > > > > >   drm/vc4: kms: Don't duplicate pending commit
> > > > > >   drm/vc4: kms: Fix previous HVS commit wait
> > > > > >
> > > > > >  drivers/gpu/drm/vc4/vc4_kms.c | 42 ++++++++++++++++-------------------
> > > > > >  1 file changed, 19 insertions(+), 23 deletions(-)
> > > > >
> > > > > I tested the v2 patches based on latest mainline kernel with RPi 4B.
> > > > > System can boot up into desktop environment.
> > > >
> > > > So the thing that was broken initially isn't anymore?
> > >
> > > No.  It does not appear anymore.
> > >
> > > > > Although it still hit the bug [1], which might be under debugging, I
> > > > > think these patches LGTM.
> > > >
> > > > The vblank timeout stuff is a symptom of various different bugs. Can you
> > > > share your setup, your config.txt, and what you're doing to trigger it?
> > >
> > > I get the RPi boot firmware files from raspberrypi's repository at tag
> > > 1.20211029 [1]
> > > And, make system boots:
> > > RPi firmware -> U-Boot -> Linux kernel (aarch64) with corresponding DTB
> > >
> > > The config.txt only has:
> > > enable_uart=1
> > > arm_64bit=1
> > > kernel=uboot.bin
> > >
> > > This bug can be reproduced with es2gears command easily.  May need to
> > > wait it running a while.
> > >
> > > Mesa: 21.2.2
> > > libdrm: 2.4.107
> > > xserver/wayland: 1.20.11  Using wayland
> > >
> > > This bug happens with direct boot path as well:
> > > RPi firmware -> Linux kernel (aarch64) with corresponding DTB
> > >
> > > I tried with Endless OS and Ubuntu's RPi 4B images.
> > > An easy setup is using Ubuntu 21.10 RPi 4B image [2].  Then, replace
> > > the kernel & device tree blob and edit the config.txt.
> >
> > Does it still appear if you raise the core clock in the config.txt file
> > using: core_freq_min=600 ?
> 
> It still appears when I raise the core clock in the config.txt file:
> core_freq_min=600

That's weird, we had some issues like that already but could never point
exactly what was going on.

Is testing the official raspberrypi kernel an option for you? If so,
trying the same workload with fkms would certainly help

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 0/6] drm/vc4: kms: Misc fixes for HVS commits

[Jian-Hong Pan]

Maxime Ripard <maxime@cerno.tech> 於 2021年12月3日 週五 下午10:03寫道：
>
> On Mon, Nov 29, 2021 at 04:31:57PM +0800, Jian-Hong Pan wrote:
> > Maxime Ripard <maxime@cerno.tech> 於 2021年11月26日 週五 下午11:45寫道：
> > >
> > > On Fri, Nov 19, 2021 at 06:24:34PM +0800, Jian-Hong Pan wrote:
> > > > Maxime Ripard <maxime@cerno.tech> 於 2021年11月18日 週四 下午6:40寫道：
> > > > >
> > > > > On Thu, Nov 18, 2021 at 02:42:58PM +0800, Jian-Hong Pan wrote:
> > > > > > Maxime Ripard <maxime@cerno.tech> 於 2021年11月17日 週三 下午5:45寫道：
> > > > > > > The conversion to DRM commit helpers (f3c420fe19f8, "drm/vc4: kms: Convert to
> > > > > > > atomic helpers") introduced a number of issues in corner cases, most of them
> > > > > > > showing themselves in the form of either a vblank timeout or use-after-free
> > > > > > > error.
> > > > > > >
> > > > > > > These patches should fix most of them, some of them still being debugged.
> > > > > > >
> > > > > > > Maxime
> > > > > > >
> > > > > > > Changes from v1:
> > > > > > >   - Prevent a null pointer dereference
> > > > > > >
> > > > > > > Maxime Ripard (6):
> > > > > > >   drm/vc4: kms: Wait for the commit before increasing our clock rate
> > > > > > >   drm/vc4: kms: Fix return code check
> > > > > > >   drm/vc4: kms: Add missing drm_crtc_commit_put
> > > > > > >   drm/vc4: kms: Clear the HVS FIFO commit pointer once done
> > > > > > >   drm/vc4: kms: Don't duplicate pending commit
> > > > > > >   drm/vc4: kms: Fix previous HVS commit wait
> > > > > > >
> > > > > > >  drivers/gpu/drm/vc4/vc4_kms.c | 42 ++++++++++++++++-------------------
> > > > > > >  1 file changed, 19 insertions(+), 23 deletions(-)
> > > > > >
> > > > > > I tested the v2 patches based on latest mainline kernel with RPi 4B.
> > > > > > System can boot up into desktop environment.
> > > > >
> > > > > So the thing that was broken initially isn't anymore?
> > > >
> > > > No.  It does not appear anymore.
> > > >
> > > > > > Although it still hit the bug [1], which might be under debugging, I
> > > > > > think these patches LGTM.
> > > > >
> > > > > The vblank timeout stuff is a symptom of various different bugs. Can you
> > > > > share your setup, your config.txt, and what you're doing to trigger it?
> > > >
> > > > I get the RPi boot firmware files from raspberrypi's repository at tag
> > > > 1.20211029 [1]
> > > > And, make system boots:
> > > > RPi firmware -> U-Boot -> Linux kernel (aarch64) with corresponding DTB
> > > >
> > > > The config.txt only has:
> > > > enable_uart=1
> > > > arm_64bit=1
> > > > kernel=uboot.bin
> > > >
> > > > This bug can be reproduced with es2gears command easily.  May need to
> > > > wait it running a while.
> > > >
> > > > Mesa: 21.2.2
> > > > libdrm: 2.4.107
> > > > xserver/wayland: 1.20.11  Using wayland
> > > >
> > > > This bug happens with direct boot path as well:
> > > > RPi firmware -> Linux kernel (aarch64) with corresponding DTB
> > > >
> > > > I tried with Endless OS and Ubuntu's RPi 4B images.
> > > > An easy setup is using Ubuntu 21.10 RPi 4B image [2].  Then, replace
> > > > the kernel & device tree blob and edit the config.txt.
> > >
> > > Does it still appear if you raise the core clock in the config.txt file
> > > using: core_freq_min=600 ?
> >
> > It still appears when I raise the core clock in the config.txt file:
> > core_freq_min=600
>
> That's weird, we had some issues like that already but could never point
> exactly what was going on.
>
> Is testing the official raspberrypi kernel an option for you? If so,
> trying the same workload with fkms would certainly help

I tested the raspberrypi kernel on rpi-5.16.y branch at commit
bcb52df6df52 ("xhci: add a quirk to work around a suspected cache bug
on VLI controllers").  Also, enabled the fkms.  So, vc4 and v3d are
loaded.  However, the "flip_done timed out" error does not appear like
mainline kernel.

Jian-Hong Pan