Date: Mon, 17 Jun 2024 08:28:58 -0500
From: Adam Ford <aford173@...il.com>
To: Dominique MARTINET <dominique.martinet@...ark-techno.com>
Cc: Lucas Stach <l.stach@...gutronix.de>, 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)

On Mon, Jun 17, 2024 at 1:17 AM Dominique MARTINET
<dominique.martinet@...ark-techno.com> wrote:
>
> Adam Ford wrote on Sat, Feb 03, 2024 at 10:52:50AM -0600:
> > From: Lucas Stach <l.stach@...gutronix.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?

I didn't write the original code, so I'll defer to Lucas here.  I just
tried to edit/fix issues as they were identified to get it pushed
upstream.

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

Are you proposing to replace 'return MODE_CLOCK_RANGE' with a printed warning?

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

I wonder if the HDMI PHY could be improved to better dynamically
calculate values instead of the look tables.

adam
>
> For reference, the output of edid-decode is as follow:
> ---
> edid-decode /sys/devices/platform/display-subsystem/drm/car
> d1/card1-HDMI-A-1/edid
> edid-decode (hex):
>
> 00 ff ff ff ff ff ff 00 3a 49 03 00 01 00 00 00
> 20 1e 01 03 80 10 09 00 0a 00 00 00 00 00 00 00
> 00 00 00 00 00 00 01 01 01 01 01 01 01 01 01 01
> 01 01 01 01 01 01 00 14 00 40 41 58 23 20 a0 20
> c8 00 9a 56 00 00 00 18 00 00 00 10 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 10 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 10
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 9a
>
> ----------------
>
> Block 0, Base EDID:
>   EDID Structure Version & Revision: 1.3
>   Vendor & Product Identification:
>     Manufacturer: NRI
>     Model: 3
>     Serial Number: 1
>     Made in: week 32 of 2020
>   Basic Display Parameters & Features:
>     Digital display
>     Maximum image size: 16 cm x 9 cm
>     Gamma: 1.00
>     RGB color display
>     First detailed timing is the preferred timing
>   Color Characteristics:
>     Red  : 0.0000, 0.0000
>     Green: 0.0000, 0.0000
>     Blue : 0.0000, 0.0000
>     White: 0.0000, 0.0000
>   Established Timings I & II: none
>   Standard Timings: none
>   Detailed Timing Descriptors:
>     DTD 1:  1024x600    59.993 Hz 128:75   38.095 kHz  51.200 MHz (154 mm x 86 m
> m)
>                  Hfront  160 Hsync  32 Hback 128 Hpol N
>                  Vfront   12 Vsync   8 Vback  15 Vpol N
>     Dummy Descriptor:
>     Dummy Descriptor:
>     Dummy Descriptor:
> Checksum: 0x9a
> ---
>
>
> Thanks,
> --
> Dominique Martinet
>
>
>
> --
> linux-phy mailing list
> linux-phy@...ts.infradead.org
> https://lists.infradead.org/mailman/listinfo/linux-phy

Powered by blists - more mailing lists