diff mbox series

[v3,032/105] drm/vc4: crtc: Enable and disable the PV in atomic_enable / disable

Message ID d2c1850e38e14f3def4c0307240e6826e296c14b.1590594512.git-series.maxime@cerno.tech (mailing list archive)
State New, archived
Headers show
Series drm/vc4: Support BCM2711 Display Pipeline | expand

Commit Message

Maxime Ripard May 27, 2020, 3:48 p.m. UTC
The VIDEN bit in the pixelvalve currently being used to enable or disable
the pixelvalve seems to not be enough in some situations, which whill end
up with the pixelvalve stalling.

In such a case, even re-enabling VIDEN doesn't bring it back and we need to
clear the FIFO. This can only be done if the pixelvalve is disabled though.

In order to overcome this, we can configure the pixelvalve during
mode_set_no_fb, but only enable it in atomic_enable and flush the FIFO
there, and in atomic_disable disable the pixelvalve again.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_crtc.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Maxime Ripard June 2, 2020, 2:12 p.m. UTC | #1
Hi Eric

On Wed, May 27, 2020 at 09:54:44AM -0700, Eric Anholt wrote:
> On Wed, May 27, 2020 at 8:50 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > The VIDEN bit in the pixelvalve currently being used to enable or disable
> > the pixelvalve seems to not be enough in some situations, which whill end
> > up with the pixelvalve stalling.
> >
> > In such a case, even re-enabling VIDEN doesn't bring it back and we need to
> > clear the FIFO. This can only be done if the pixelvalve is disabled though.
> >
> > In order to overcome this, we can configure the pixelvalve during
> > mode_set_no_fb, but only enable it in atomic_enable and flush the FIFO
> > there, and in atomic_disable disable the pixelvalve again.
> 
> What displays has this been tested with?  Getting this sequencing
> right is so painful, and things like DSI are tricky to get to light
> up.

That FIFO is between the HVS and the HDMI PVs, so this was obviously
tested against that. Dave also tested the DSI output IIRC, so we should
be covered here.

Maxime
Dave Stevenson June 2, 2020, 3:02 p.m. UTC | #2
Hi Maxime and Eric

On Tue, 2 Jun 2020 at 15:12, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Eric
>
> On Wed, May 27, 2020 at 09:54:44AM -0700, Eric Anholt wrote:
> > On Wed, May 27, 2020 at 8:50 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > The VIDEN bit in the pixelvalve currently being used to enable or disable
> > > the pixelvalve seems to not be enough in some situations, which whill end
> > > up with the pixelvalve stalling.
> > >
> > > In such a case, even re-enabling VIDEN doesn't bring it back and we need to
> > > clear the FIFO. This can only be done if the pixelvalve is disabled though.
> > >
> > > In order to overcome this, we can configure the pixelvalve during
> > > mode_set_no_fb, but only enable it in atomic_enable and flush the FIFO
> > > there, and in atomic_disable disable the pixelvalve again.
> >
> > What displays has this been tested with?  Getting this sequencing
> > right is so painful, and things like DSI are tricky to get to light
> > up.
>
> That FIFO is between the HVS and the HDMI PVs, so this was obviously
> tested against that. Dave also tested the DSI output IIRC, so we should
> be covered here.

DSI wasn't working on the first patch set that Maxime sent - I haven't
tested this one as yet but will do so.
DPI was working early on to both an Adafruit 800x480 DPI panel, and
via a VGA666 as VGA.
HDMI is obviously working.
VEC is being ignored now. The clock structure is more restricted than
earlier chips, so to get the required clocks for the VEC without using
fractional divides it compromises the clock that other parts of the
system can run at (IIRC including the ARM). That's why the VEC has to
be explicitly enabled for the firmware to enable it as the only
output. It's annoying, but that's just a restriction of the chip.

  Dave
Eric Anholt June 2, 2020, 7:31 p.m. UTC | #3
On Tue, Jun 2, 2020 at 8:02 AM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Maxime and Eric
>
> On Tue, 2 Jun 2020 at 15:12, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi Eric
> >
> > On Wed, May 27, 2020 at 09:54:44AM -0700, Eric Anholt wrote:
> > > On Wed, May 27, 2020 at 8:50 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > The VIDEN bit in the pixelvalve currently being used to enable or disable
> > > > the pixelvalve seems to not be enough in some situations, which whill end
> > > > up with the pixelvalve stalling.
> > > >
> > > > In such a case, even re-enabling VIDEN doesn't bring it back and we need to
> > > > clear the FIFO. This can only be done if the pixelvalve is disabled though.
> > > >
> > > > In order to overcome this, we can configure the pixelvalve during
> > > > mode_set_no_fb, but only enable it in atomic_enable and flush the FIFO
> > > > there, and in atomic_disable disable the pixelvalve again.
> > >
> > > What displays has this been tested with?  Getting this sequencing
> > > right is so painful, and things like DSI are tricky to get to light
> > > up.
> >
> > That FIFO is between the HVS and the HDMI PVs, so this was obviously
> > tested against that. Dave also tested the DSI output IIRC, so we should
> > be covered here.
>
> DSI wasn't working on the first patch set that Maxime sent - I haven't
> tested this one as yet but will do so.
> DPI was working early on to both an Adafruit 800x480 DPI panel, and
> via a VGA666 as VGA.
> HDMI is obviously working.
> VEC is being ignored now. The clock structure is more restricted than
> earlier chips, so to get the required clocks for the VEC without using
> fractional divides it compromises the clock that other parts of the
> system can run at (IIRC including the ARM). That's why the VEC has to
> be explicitly enabled for the firmware to enable it as the only
> output. It's annoying, but that's just a restriction of the chip.

I'm more concerned with "make sure we don't regress pre-pi4 with this
series" than "pi4 displays all work from the beginning"
Stefan Wahren June 2, 2020, 8:03 p.m. UTC | #4
Hi,

Am 02.06.20 um 21:31 schrieb Eric Anholt:
> On Tue, Jun 2, 2020 at 8:02 AM Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
>> Hi Maxime and Eric
>>
>> On Tue, 2 Jun 2020 at 15:12, Maxime Ripard <maxime@cerno.tech> wrote:
>>> Hi Eric
>>>
>>> On Wed, May 27, 2020 at 09:54:44AM -0700, Eric Anholt wrote:
>>>> On Wed, May 27, 2020 at 8:50 AM Maxime Ripard <maxime@cerno.tech> wrote:
>>>>> The VIDEN bit in the pixelvalve currently being used to enable or disable
>>>>> the pixelvalve seems to not be enough in some situations, which whill end
>>>>> up with the pixelvalve stalling.
>>>>>
>>>>> In such a case, even re-enabling VIDEN doesn't bring it back and we need to
>>>>> clear the FIFO. This can only be done if the pixelvalve is disabled though.
>>>>>
>>>>> In order to overcome this, we can configure the pixelvalve during
>>>>> mode_set_no_fb, but only enable it in atomic_enable and flush the FIFO
>>>>> there, and in atomic_disable disable the pixelvalve again.
>>>> What displays has this been tested with?  Getting this sequencing
>>>> right is so painful, and things like DSI are tricky to get to light
>>>> up.
>>> That FIFO is between the HVS and the HDMI PVs, so this was obviously
>>> tested against that. Dave also tested the DSI output IIRC, so we should
>>> be covered here.
>> DSI wasn't working on the first patch set that Maxime sent - I haven't
>> tested this one as yet but will do so.
>> DPI was working early on to both an Adafruit 800x480 DPI panel, and
>> via a VGA666 as VGA.
>> HDMI is obviously working.
>> VEC is being ignored now. The clock structure is more restricted than
>> earlier chips, so to get the required clocks for the VEC without using
>> fractional divides it compromises the clock that other parts of the
>> system can run at (IIRC including the ARM). That's why the VEC has to
>> be explicitly enabled for the firmware to enable it as the only
>> output. It's annoying, but that's just a restriction of the chip.
> I'm more concerned with "make sure we don't regress pre-pi4 with this
> series" than "pi4 displays all work from the beginning"

unfortuntely i can confirm this. With this patch series (using Maxime's
git repo with multi_v7_defconfig) my Raspberry Pi 3 B hangs up while
starting X (screen stays black, heartbeat stops, no more output at the
debug UART). AFAIR v2 didn't had this issue.

Stefan

>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Maxime Ripard June 3, 2020, 1:14 p.m. UTC | #5
Hi Stefan,

On Tue, Jun 02, 2020 at 10:03:13PM +0200, Stefan Wahren wrote:
> Am 02.06.20 um 21:31 schrieb Eric Anholt:
> > On Tue, Jun 2, 2020 at 8:02 AM Dave Stevenson
> > <dave.stevenson@raspberrypi.com> wrote:
> >> Hi Maxime and Eric
> >>
> >> On Tue, 2 Jun 2020 at 15:12, Maxime Ripard <maxime@cerno.tech> wrote:
> >>> Hi Eric
> >>>
> >>> On Wed, May 27, 2020 at 09:54:44AM -0700, Eric Anholt wrote:
> >>>> On Wed, May 27, 2020 at 8:50 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >>>>> The VIDEN bit in the pixelvalve currently being used to enable or disable
> >>>>> the pixelvalve seems to not be enough in some situations, which whill end
> >>>>> up with the pixelvalve stalling.
> >>>>>
> >>>>> In such a case, even re-enabling VIDEN doesn't bring it back and we need to
> >>>>> clear the FIFO. This can only be done if the pixelvalve is disabled though.
> >>>>>
> >>>>> In order to overcome this, we can configure the pixelvalve during
> >>>>> mode_set_no_fb, but only enable it in atomic_enable and flush the FIFO
> >>>>> there, and in atomic_disable disable the pixelvalve again.
> >>>> What displays has this been tested with?  Getting this sequencing
> >>>> right is so painful, and things like DSI are tricky to get to light
> >>>> up.
> >>> That FIFO is between the HVS and the HDMI PVs, so this was obviously
> >>> tested against that. Dave also tested the DSI output IIRC, so we should
> >>> be covered here.
> >> DSI wasn't working on the first patch set that Maxime sent - I haven't
> >> tested this one as yet but will do so.
> >> DPI was working early on to both an Adafruit 800x480 DPI panel, and
> >> via a VGA666 as VGA.
> >> HDMI is obviously working.
> >> VEC is being ignored now. The clock structure is more restricted than
> >> earlier chips, so to get the required clocks for the VEC without using
> >> fractional divides it compromises the clock that other parts of the
> >> system can run at (IIRC including the ARM). That's why the VEC has to
> >> be explicitly enabled for the firmware to enable it as the only
> >> output. It's annoying, but that's just a restriction of the chip.
> > I'm more concerned with "make sure we don't regress pre-pi4 with this
> > series" than "pi4 displays all work from the beginning"
> 
> unfortuntely i can confirm this. With this patch series (using Maxime's
> git repo with multi_v7_defconfig) my Raspberry Pi 3 B hangs up while
> starting X (screen stays black, heartbeat stops, no more output at the
> debug UART). AFAIR v2 didn't had this issue.

Did it happen with a DSI display or something else?

I've been trying to setup the DSI display on an RPi3 today, but noticed
that it looks like there's a regression in next that prevents the HDMI
driver to load entirely (without my patches).

Maxime
Stefan Wahren June 3, 2020, 3:26 p.m. UTC | #6
Hi Maxime,

Am 03.06.20 um 15:14 schrieb Maxime Ripard:
> Hi Stefan,
>
> On Tue, Jun 02, 2020 at 10:03:13PM +0200, Stefan Wahren wrote:
>> Am 02.06.20 um 21:31 schrieb Eric Anholt:
>>> On Tue, Jun 2, 2020 at 8:02 AM Dave Stevenson
>>> <dave.stevenson@raspberrypi.com> wrote:
>>>> Hi Maxime and Eric
>>>>
>>>> On Tue, 2 Jun 2020 at 15:12, Maxime Ripard <maxime@cerno.tech> wrote:
>>>>> Hi Eric
>>>>>
>>>>> On Wed, May 27, 2020 at 09:54:44AM -0700, Eric Anholt wrote:
>>>>>> On Wed, May 27, 2020 at 8:50 AM Maxime Ripard <maxime@cerno.tech> wrote:
>>>>>>> The VIDEN bit in the pixelvalve currently being used to enable or disable
>>>>>>> the pixelvalve seems to not be enough in some situations, which whill end
>>>>>>> up with the pixelvalve stalling.
>>>>>>>
>>>>>>> In such a case, even re-enabling VIDEN doesn't bring it back and we need to
>>>>>>> clear the FIFO. This can only be done if the pixelvalve is disabled though.
>>>>>>>
>>>>>>> In order to overcome this, we can configure the pixelvalve during
>>>>>>> mode_set_no_fb, but only enable it in atomic_enable and flush the FIFO
>>>>>>> there, and in atomic_disable disable the pixelvalve again.
>>>>>> What displays has this been tested with?  Getting this sequencing
>>>>>> right is so painful, and things like DSI are tricky to get to light
>>>>>> up.
>>>>> That FIFO is between the HVS and the HDMI PVs, so this was obviously
>>>>> tested against that. Dave also tested the DSI output IIRC, so we should
>>>>> be covered here.
>>>> DSI wasn't working on the first patch set that Maxime sent - I haven't
>>>> tested this one as yet but will do so.
>>>> DPI was working early on to both an Adafruit 800x480 DPI panel, and
>>>> via a VGA666 as VGA.
>>>> HDMI is obviously working.
>>>> VEC is being ignored now. The clock structure is more restricted than
>>>> earlier chips, so to get the required clocks for the VEC without using
>>>> fractional divides it compromises the clock that other parts of the
>>>> system can run at (IIRC including the ARM). That's why the VEC has to
>>>> be explicitly enabled for the firmware to enable it as the only
>>>> output. It's annoying, but that's just a restriction of the chip.
>>> I'm more concerned with "make sure we don't regress pre-pi4 with this
>>> series" than "pi4 displays all work from the beginning"
>> unfortuntely i can confirm this. With this patch series (using Maxime's
>> git repo with multi_v7_defconfig) my Raspberry Pi 3 B hangs up while
>> starting X (screen stays black, heartbeat stops, no more output at the
>> debug UART). AFAIR v2 didn't had this issue.
> Did it happen with a DSI display or something else?
with my HDMI display (HP ZR2440w). At first everything looks good
(Raspbian splash screen is displayed), but before the real workspace is
displayed this issue happens.
> I've been trying to setup the DSI display on an RPi3 today, but noticed
> that it looks like there's a regression in next that prevents the HDMI
> driver to load entirely (without my patches).

I took your rpi4-kms branch and switched before your series and
everything works fine. I will try to bisect the offending commit.

Stefan

>
> Maxime
Maxime Ripard June 12, 2020, 3:35 p.m. UTC | #7
Hi Eric,

On Tue, Jun 02, 2020 at 12:31:37PM -0700, Eric Anholt wrote:
> On Tue, Jun 2, 2020 at 8:02 AM Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi Maxime and Eric
> >
> > On Tue, 2 Jun 2020 at 15:12, Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > Hi Eric
> > >
> > > On Wed, May 27, 2020 at 09:54:44AM -0700, Eric Anholt wrote:
> > > > On Wed, May 27, 2020 at 8:50 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > >
> > > > > The VIDEN bit in the pixelvalve currently being used to enable or disable
> > > > > the pixelvalve seems to not be enough in some situations, which whill end
> > > > > up with the pixelvalve stalling.
> > > > >
> > > > > In such a case, even re-enabling VIDEN doesn't bring it back and we need to
> > > > > clear the FIFO. This can only be done if the pixelvalve is disabled though.
> > > > >
> > > > > In order to overcome this, we can configure the pixelvalve during
> > > > > mode_set_no_fb, but only enable it in atomic_enable and flush the FIFO
> > > > > there, and in atomic_disable disable the pixelvalve again.
> > > >
> > > > What displays has this been tested with?  Getting this sequencing
> > > > right is so painful, and things like DSI are tricky to get to light
> > > > up.
> > >
> > > That FIFO is between the HVS and the HDMI PVs, so this was obviously
> > > tested against that. Dave also tested the DSI output IIRC, so we should
> > > be covered here.
> >
> > DSI wasn't working on the first patch set that Maxime sent - I haven't
> > tested this one as yet but will do so.
> > DPI was working early on to both an Adafruit 800x480 DPI panel, and
> > via a VGA666 as VGA.
> > HDMI is obviously working.
> > VEC is being ignored now. The clock structure is more restricted than
> > earlier chips, so to get the required clocks for the VEC without using
> > fractional divides it compromises the clock that other parts of the
> > system can run at (IIRC including the ARM). That's why the VEC has to
> > be explicitly enabled for the firmware to enable it as the only
> > output. It's annoying, but that's just a restriction of the chip.
> 
> I'm more concerned with "make sure we don't regress pre-pi4 with this
> series" than "pi4 displays all work from the beginning"

I tested the DSI today on an RPI3, and I had to fix an issue with the
HVS-PV muxing, but otherwise it works with this series. Otherwise, the
HDMI and TXP work on the Pi3 too, I'll give a try to the VGA adapter on
it next week to test DPI too.

As a side note, my DSI display without my patches gives DSI transfer
timeouts at boot, since at least (mainline) 5.4, I haven't tested
further back. It just stalls the modeset for a while, but the display
works fine after that.

Does that ring any bell?

Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 15c72afb226f..580b37ad514d 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -372,9 +372,7 @@  static void vc4_crtc_config_pv(struct drm_crtc *crtc)
 		   PV_CONTROL_TRIGGER_UNDERFLOW |
 		   PV_CONTROL_WAIT_HSTART |
 		   VC4_SET_FIELD(vc4_encoder->clock_select,
-				 PV_CONTROL_CLK_SELECT) |
-		   PV_CONTROL_FIFO_CLR |
-		   PV_CONTROL_EN);
+				 PV_CONTROL_CLK_SELECT));
 }
 
 static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc)
@@ -465,6 +463,8 @@  static void vc4_crtc_atomic_disable(struct drm_crtc *crtc,
 	ret = wait_for(!(CRTC_READ(PV_V_CONTROL) & PV_VCONTROL_VIDEN), 1);
 	WARN_ONCE(ret, "Timeout waiting for !PV_VCONTROL_VIDEN\n");
 
+	CRTC_WRITE(PV_CONTROL, CRTC_READ(PV_CONTROL) & ~PV_CONTROL_EN);
+
 	if (HVS_READ(SCALER_DISPCTRLX(chan)) &
 	    SCALER_DISPCTRLX_ENABLE) {
 		HVS_WRITE(SCALER_DISPCTRLX(chan),
@@ -552,6 +552,10 @@  static void vc4_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	require_hvs_enabled(dev);
 
+	/* Reset the PV fifo. */
+	CRTC_WRITE(PV_CONTROL, CRTC_READ(PV_CONTROL) |
+		   PV_CONTROL_FIFO_CLR | PV_CONTROL_EN);
+
 	/* Enable vblank irq handling before crtc is started otherwise
 	 * drm_crtc_get_vblank() fails in vc4_crtc_update_dlist().
 	 */