Message-ID: <ZnDKovBokBu5D_eb@atmark-techno.com>
Date: Tue, 18 Jun 2024 08:45:38 +0900
From: Dominique MARTINET <dominique.martinet@...ark-techno.com>
To: Adam Ford <aford173@...il.com>, Lucas Stach <l.stach@...gutronix.de>
Cc: linux-arm-kernel@...ts.infradead.org, marex@...x.de,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Andrzej Hajda <andrzej.hajda@...el.com>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Robert Foss <rfoss@...nel.org>, Jonas Karlman <jonas@...boo.se>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	NXP Linux Team <linux-imx@....com>, Vinod Koul <vkoul@...nel.org>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Liu Ying <victor.liu@....com>, dri-devel@...ts.freedesktop.org,
	linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org,
	Makoto Sato <makoto.sato@...ark-techno.com>
Subject: Re: 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)

Thanks for the replies, replying to both mails at once.

Adam Ford wrote on Mon, Jun 17, 2024 at 08:28:58AM -0500:
> > 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.
> 
> Are you proposing to replace 'return MODE_CLOCK_RANGE' with a printed warning?

Yes, something like that.

The imx93-mipi-dsi.c code has a check that's a bit more complex that
might be closer to what we want if you think the check is useful:
        if ((bridge->ops & DRM_BRIDGE_OP_DETECT) &&
            (bridge->ops & DRM_BRIDGE_OP_EDID)) {
                unsigned long pixel_clock_rate = mode->clock * 1000;
                unsigned long rounded_rate;

                /* Allow +/-0.5% pixel clock rate deviation */
                rounded_rate = clk_round_rate(dsi->clk_pixel, pixel_clock_rate);
                if (rounded_rate < pixel_clock_rate * 995 / 1000 ||
                    rounded_rate > pixel_clock_rate * 1005 / 1000) {
                        dev_dbg(dsi->dev, "failed to round clock for mode " DRM_MODE_FMT "\n",
                                DRM_MODE_ARG(mode));
                        return MODE_NOCLOCK;
                }
        }

However, for my particular case 0.5% wouldn't be enough to get something
to display unfortunately :/

> > 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.
> 
> I wonder if the HDMI PHY could be improved to better dynamically
> calculate values instead of the look tables.

That would probably be ideal, right... If we could do that we could
likely compute something within 0.5% of the required freq.

The original code from NXP was full of what seemed at the time magic
values, but with the new code it seems quite a bit clearer...
At least the values that are left seem to somewhat make sense:
https://gaia.codewreck.org/local/linux/hdmi/plot-combined.svg
https://gaia.codewreck.org/local/linux/hdmi/plot-1.svg
 -> dented linear values, per the intervals defined in
 fsl_samsung_hdmi_phy_configure_pixclk()
https://gaia.codewreck.org/local/linux/hdmi/plot-2.svg
 -> constant for each intervals?
https://gaia.codewreck.org/local/linux/hdmi/plot-3.svg
https://gaia.codewreck.org/local/linux/hdmi/plot-4.svg
 these don't really make sense to me...
https://gaia.codewreck.org/local/linux/hdmi/plot-5.svg
 that one 0 value at 154000000 looks odd,
 but that aside we could probably get away with constant 0x80
 if the value matters at all...
https://gaia.codewreck.org/local/linux/hdmi/plot-6.svg
 weird as well

I'm thinking the last few values just affect a very small % of the
values, but would need to check with a proper scope if I can get a hold
of the clock line...
Does any of you happen to have any datasheet for these registers,
or should we consider them to be magic values?

Lucas Stach wrote on Mon, Jun 17, 2024 at 06:32:45PM +0200:
> > 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'd expect displays to be tolerant to quite a few percents, but I didn't
know the spec defined something like that... iirc the HDMI spec isn't
public?

> > 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.
> 
> 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.

Ah, that's a very good point, if other modes are valid then we don't
want to present modes that are less likely to work, we should only allow
bigger variations if no other mode worked...
I don't think we have this info at validation time though?

I think that Adam's suggestion of making this more dynamic would be
great, if we can just generate finer frequencies for a reasonable range
then it wouldn't be a problem to limit the check to 0.5%.

Short of something really dynamic I can add a value for our particular
display based on what I've seen above (just pick a couple of points
along the lines for the two values I understood and whatever value
neighbors have for the rest & check with a scope it's somewhat close),
but I honestly would rather not get too far down this hole, we can't
cover all the quirky hardwares that exist manually...


Cheers,
-- 
Dominique

Powered by blists - more mailing lists