Re: [PATCH] ARM: dts: bcm283x: Use firmware PM driver for V3D

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Maxime

On Mon, 23 Mar 2020 at 14:57, Maxime Ripard <maxime@xxxxxxxxxx> wrote:
>
> Hi Dave,
>
> On Mon, Mar 16, 2020 at 01:57:13PM +0000, Dave Stevenson wrote:
> >  Hi Stefan and Nicolas
> >
> > On Mon, 16 Mar 2020 at 12:40, Nicolas Saenz Julienne
> > <nsaenzjulienne@xxxxxxx> wrote:
> > >
> > > Hi Stefan,
> > > thanks for taking the time with this. That was a hard to find one, specially
> > > given the race in X11.
> > >
> > > On Sun, 2020-03-15 at 20:16 +0100, Stefan Wahren wrote:
> > > > Hi Nicolas,
> > > >
> > > > [adjust audience]
> > > >
> > > > i've narrowed down the issue. From kernel 4.19 until 5.1 the DRM
> > > > emulated driver was responsible for a working X on my Raspberry Pi 3
> > > > with HP ZR2440w. Starting with 5.2 the vc4drmfb took over and with 5.3 X
> > > > didn't start anymore (display freeze).
> > > >
> > > > So i start bisecting and this was the commit where the freezing started:
> > > >
> > > > e08ab74bd4 drm/modes: Rewrite the command line parser
> > > >
> > > > After this i enabled drm debug and saw that suggest mode 1920x1200 by
> > > > the firmware is rejected by the driver because the pixel clock would be
> > > > too high (154 MHz, max = 148.5). This wasn't a problem before since the
> > > > firmware provided video cmdline parameter wasn't parseable:
> > > >
> > > > [drm] parse error at position 69 in video mode
> > > > '1920x1200M@60,margin_left=0,margin_right=0,margin_top=0,margin_bottom=0'
> > > >
> > > > After mentioned commit the display just freezes (no try to use
> > > > 1920x1080, no error message).
> > > >
> > > > For comparison i switched to the vendor tree with firmware kms driver
> > > > and noticed that the driver switches to 1920x1200 with a pixel at 154 MHz.
> > > >
> > > > So this patch works for me:
> > > >
> > > > ---
> > > >  drivers/gpu/drm/vc4/vc4_hdmi.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > index cea18dc..647803e 100644
> > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > @@ -681,11 +681,12 @@ static enum drm_mode_status
> > > >  vc4_hdmi_encoder_mode_valid(struct drm_encoder *crtc,
> > > >                  const struct drm_display_mode *mode)
> > > >  {
> > > > -    /* HSM clock must be 108% of the pixel clock.
> > >
> > > I think it'd be nice to understand how Eric came by this number. Maybe just
> > > empirically with 1080p60? That said, I think your change is pretty much
> > > harmless.
> > >
> > > I'll add a reminder to Maxime's series for him to update RPi[0-3]'s max
> > > frequency to 1920x1200@60's.
> > >
> > > > -     * the AXI clock needs to be at least 25% of pixel clock, but
> > > > -     * HSM ends up being the limiting factor.
> > > > +    /* According to spec the HSM clock must be 108% of the pixel clock.
> > > > +     * Additionally, the AXI clock needs to be at least 25% of pixel clock,
> > > > +     * but HSM ends up being the limiting factor.
> > > > +     * It seems that operating the pixel clock at 154 MHz still works.
> > > >       */
> > > > -    if (mode->clock > HSM_CLOCK_FREQ / (1000 * 108 / 100))
> > > > +    if (mode->clock > HSM_CLOCK_FREQ / (1000 * 106 / 100))
> > >
> > > Isn't hard-coding the HSM clock kind of limited, one could overclock it, isn't
> > > it? I remember reading someone did it to support wider resolutions.
> >
> > Checking the docs it does state the restriction that Eric quotes.
> >
> > HDMI Core Clock (state machine clock)
> > Most of the HDMI logic operates on that clock. It
> > is asynchronous to system core clock and pixel
> > clock. Source for this clock can be chosen from
> > various PLLs in the chip. See CPR Manager for
> > details.
> >
> > This clock is also used for clocking pixel valve
> > when HDMI peripheral is used. See Pixel Valve
> > for details.axi_clock >= hdmi_core_clock > 108% of
> > pixel_clock.
> >
> >
> > The default max pixel clock from the firmware side is 162MHz.
> >
> > In the firmware source we have a comment
> >          // HDMI state machine clock must be faster than pixel clock -
> > infinitessimally faster tested in simulation.
> >          // Otherwise, exact value is unimportant - for HDMI operation.
> >          // hdmi state machine clock now derived from PLLC_PER (typ
> > 500MHz, see relevant platform.c).
> >          //
> >          // However, CEC bit clock is derived from the HSM clock, and
> > the (programmable) CEC timing table
> >          // is configured for a 40.00kHz CEC clock.
> >          const unsigned margin = 1*1000*1000;
> >          unsigned min_hsm_clock = margin + timings->pixel_freq;
> >          const unsigned max_hsm_clock_for_cec = max(163682864,
> > hdmi_pixel_freq_limit);
> >
> >          unsigned hdmi_state_machine_clock = max_hsm_clock_for_cec;
> >
> > So it adds 1MHz to the pixel clock for min_hsm_clock, but then doesn't
> > use the value.
> > Unless you do request a higher hdmi_pixel_freq_limit then the HSM is
> > running at the same 163.68MHz that Eric's driver hard codes, and our
> > max pixel clock is 162MHz.
> > Keeping it a fixed value makes sorting the divider for CEC easier.
> >
> > Just adopting a 162MHz limit with a suitable comment is probably the
> > most sensible move here, and Maxime's patches can pick up the same
> > value.
>
> It's kind of related, but one of the changes we did to support the
> RPi4 is to change that rate calculation to increase the HSM clock for
> pixel clocks higher than 148.5MHz (so typically 4k), while keeping it
> as low as possible to reduce the power consumption.
>
> How would that interact with this change?

I'd forgotten that your patches mean we change the HSM clock on Pi3.
As you're aware, whilst I have some extra docs, many of them aren't as
comprehensive as one would hope. We can go back to the Broadcom and
RTL if absolutely necessary, but it's a pain. Broadcom don't
necessarily have the personnel who designed the blocks still working
there.

Your patches appear to recompute the HSM clock based on pixel_clock *
108%, with a min of 108MHz, so effectively the same limit as the old
version did by fixing the HSM rate.

Checking the firmware for Pi4, it sets the HSM (M2MC) clock to
pixel_rate * 1.01, clipped to 120MHz and 600Mhz. (Audio drops out if
less than 108MHz. "Pick 120 to have an integer divider with some
margin." I'd need to check which divider that is referring to).
As noted above, the Pi3 firmware sets the HSM clock to 163.6MHz.

I'd suggest that we:
a) Increase max_pixel_clock for vc4 (Pi0-3) to 162MHz.
b) Set HSM clock to 101% of the pixel clock, with a min of 120MHz
(4k60 with pixel clock 594MHz would go to a 599.94MHz HSM clock which
is still within range)
c) Test it! I know we have some 1920x1200 monitors in the office (when
I'm back in there).

Whilst the firmware would appear to use a fixed HSM clock on Pi0-3, I
don't anticipate there being any issue with varying it. It looks like
there was the expectation of it being variable in the past, but
someone has refactored it and either accidentally or deliberately
given up on the idea.

  Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [CentOS ARM]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]

  Powered by Linux