drm/bridge/imx8mp-hdmi-tx: Allow inexact pixel clock frequencies (Was: [PATCH V8 10/12] drm/bridge: imx: add bridge wrapper driver for i.MX8MP DWC HDMI)

[Adam Ford]

On Thu, Aug 15, 2024 at 3:19 AM Frieder Schrempf
<frieder.schrempf at kontron.de> wrote:
>
> Hi Dominique, hi Lucas,
>
> On 17.06.24 6:32 PM, Lucas Stach wrote:
> > Hi Dominique,
> >
> > Am Montag, dem 17.06.2024 um 15:16 +0900 schrieb Dominique MARTINET:
> >> Adam Ford wrote on Sat, Feb 03, 2024 at 10:52:50AM -0600:
> >>> From: Lucas Stach <l.stach at pengutronix.de>
> >>>
> >>> Add a simple wrapper driver for the DWC HDMI bridge driver that
> >>> implements the few bits that are necessary to abstract the i.MX8MP
> >>> SoC integration.
> >>
> >> Hi Lucas, Adam,
> >> (trimmed ccs a bit)
> >>
> >> First, thank you for the effort of upstreaming all of this!! It's really
> >> appreciated, and with display working I'll really be wanting to upstream
> >> our DTS as well as soon as I have time (which is going to be a while,
> >> but better late than never ?)
> >>
> >> Until then, it's been a few months but I've got a question on this bit:
> >>
> >>> diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> >>> new file mode 100644
> >>> index 000000000000..89fc432ac611
> >>> --- /dev/null
> >>> +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> >>> +static enum drm_mode_status
> >>> +imx8mp_hdmi_mode_valid(struct dw_hdmi *dw_hdmi, void *data,
> >>> +                  const struct drm_display_info *info,
> >>> +                  const struct drm_display_mode *mode)
> >>> +{
> >>> +   struct imx8mp_hdmi *hdmi = (struct imx8mp_hdmi *)data;
> >>> +
> >>> +   if (mode->clock < 13500)
> >>> +           return MODE_CLOCK_LOW;
> >>> +
> >>> +   if (mode->clock > 297000)
> >>> +           return MODE_CLOCK_HIGH;
> >>> +
> >>> +   if (clk_round_rate(hdmi->pixclk, mode->clock * 1000) !=
> >>> +       mode->clock * 1000)
> >>> +           return MODE_CLOCK_RANGE;
> >>
> >> Do you know why such a check is here?
> >
> > Sending a HDMI signal with a different rate than what the display
> > expects rarely ends well, so this check avoids that.
> >
> > However, this check is a bit overcautious in that it only allows exact
> > rate matches. IIRC HDMI allows a rate mismatch of +- 0.5%, so this
> > check could be relaxed quite a bit to allow for that.
>
> I checked with a 1080p display that reports 23 possible modes via EDID.
> Out of these 15 are accepted by the driver with the strict check.
>
> Two more would be accepted with a relaxed check that allows a 0.5% margin.
>
> For the remaining six modes, the pixel clock would deviate up to ~5%
> from what the display expects. Still, if I remove the check altogether,
> all 23 modes seem to work just fine.
>
> >>
> >> When plugging in a screen with no frequency identically supported in its
> >> EDID this check causes the screen to stay black, and we've been telling
> >> customers to override the EDID but it's a huge pain.
> >>
> >> Commit 6ad082bee902 ("phy: freescale: add Samsung HDMI PHY") already
> >> "fixed" the samsung hdmi phy driver to return the next frequency if an
> >> exact match hasn't been found (NXP tree's match frequencies exactly, but
> >> this gets the first clock with pixclk <= rate), so if this check is also
> >> relaxed our displays would work out of the box.
> >>
> >> I also don't see any other bridge doing this kind of check.
> >> drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c has a similar check with a
> >> 0.5% leeway, and all the other drivers don't check anything.
> >> If you want to add some level of safety, I think we could make this work
> >> with a 5% margin easily... Printing a warning in dmesg could work if
> >> you're worried about artifacts, but litteraly anything is better than a
> >> black screen with no error message in my opinion.
> >>
> >>
> >> In practice the screen I'm looking at has an EDID which only supports
> >> 51.2MHz and the closest frequency supported by the Samsung HDMI phy is
> >> 50.4MHz, so that's a ~1.5% difference and it'd be great if it could work
> >> out of the box.
> >
> > For rate mismatches larger than the 0.5% allowed by the HDMI spec it
> > would be better to actually add PHY PLL parameters matching those
> > rates.
>
> I'd really like to be able to add more PHY PLL setpoints for displays
> that use non-CEA-861 modes. Unfortunately I didn't manage to figure out
> the fractional-n PLL parameter calculation so far.
>
> The i.MX8MP Reference Manual provides formulas to calculate the
> parameters based on the register values and I tried to make sense of it
> using the existing register values in the driver. But somehow it doesn't
> add up for me.
>
> Lucas, did you already work with the PLL parameters? By any chance, do
> you now how the math behind them works?

I spent a little time trying to understand it a bit.  I created a PMS
calculator similar to the one used on the DSI controller, except that
the M seems to be fixed at a value of 62 per the data sheet, so it's
more of a PS calculator.

Anyway, When I run my P-S calculator to generate the 'best rate' I get
a value that's usually 0.2% variance from nominal, but I only verified
a handful of values:

Several which were +0.2%
297600000 vs 297000000 (4k at 30)
148800000 vs 148500000 (1080p60)
74400 vs 74200

One value was -0.16%
24800000 vs 25200000

If the M value was a bit more flexible, we might be able to reduce
that variance more.

If / when I get some time, I might play with trying to disable the
fractional mode and just use the PMS calculator for simplicity despite
the inaccuracy.  Maybe we could fall back to using the PMS calculator
if the desired frequency isn't in the look-up-table.

adam

>
> >
> > We could potentially add some more leeway for displays like yours that
> > aren't actually HDMI (as it doesn't seem to have a standard HDMI
> > resolution). But that's more of a last resort option, as it may
> > introduce other problems for displays that aren't as tolerant with
> > their input rates. Remember the mode_valid call is used to filter modes
> > that aren't compatible with the source, so for a display with multiple
> > modes allowing too much leeway may lead to incompatible modes not
> > getting tossed, in turn allowing userspace to set a mode that the
> > display may not like due to too much rate deviation, instead of only
> > presenting a list of valid modes. This again would present the user
> > with a black-screen without warning situation.
>
> What about adding some option to relax or remove the check for debugging
> purposes? Maybe combined with printing a warning message?
>
> I agree that we should prevent incompatible modes from passing the
> filter, but it would be really helpful if people had an easy way to
> relax/remove the check to see whether their display could potentially
> work without them needing to modify and recompile the kernel.
>
> Thanks
> Frieder