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?

Thanks!
Maxime