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

[Dave Stevenson <dave.stevenson@raspberrypi.com>]

Hi Stefan

On Mon, 23 Mar 2020 at 18:53, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>
> Hi,
>
> Am 23.03.20 um 16:55 schrieb Dave Stevenson:
> > Hi Maxime
> >
> > On Mon, 23 Mar 2020 at 14:57, Maxime Ripard <maxime@cerno.tech> 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@suse.de> 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).
>
> i don't think that i'm able to prepare those patches, but i'm happy to
> test them.

Conversations at slight cross-purposes.
With the current driver we just need the limit lifting to 162MHz.
My comment:

> >>> Just adopting a 162MHz limit with a suitable comment is probably the
> >>> most sensible move here

above. And code change along the lines of:
-    if (mode->clock > HSM_CLOCK_FREQ / (1000 * 108 / 100))
+    if (mode->clock > 162000000)
with suitable comment.

Maxime was querying the plan for how the increased pixel rate/HSM
clock should be reflected in his patch set which adds Pi4 support to
the vc4 driver.

  Dave

> Thanks
> Stefan
>
> >
> > 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@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

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