| Message ID | 20240203165307.7806-11-aford173@gmail.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | soc: imx8mp: Add support for HDMI | expand |
On Sat, 3 Feb 2024 10:52:50 -0600 Adam Ford <aford173@gmail.com> wrote: > From: Lucas Stach <l.stach@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. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Tested-by: Marek Vasut <marex@denx.de> > Tested-by: Adam Ford <aford173@gmail.com> #imx8mp-beacon > Tested-by: Richard Leitner <richard.leitner@skidata.com> > Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de> > Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > Signed-off-by: Adam Ford <aford173@gmail.com> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Adam Ford wrote on Sat, Feb 03, 2024 at 10:52:50AM -0600: > From: Lucas Stach <l.stach@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? 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 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,
On Mon, Jun 17, 2024 at 1:17 AM Dominique MARTINET <dominique.martinet@atmark-techno.com> wrote: > > Adam Ford wrote on Sat, Feb 03, 2024 at 10:52:50AM -0600: > > From: Lucas Stach <l.stach@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? 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@lists.infradead.org > https://lists.infradead.org/mailman/listinfo/linux-phy
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@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. > > 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. 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. Regards, Lucas > > 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 wrote on Tue, Jun 18, 2024 at 08:45:38AM +0900: > 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? Answering to myself on this point, the registers are properly described in the iMX8MP reference manual... I don't make much sense of it at this point (what's SDC..?), but I guess it does sound likely we could generate the values with some work. I'll wait for replies a bit first in case one of you understands this better than I (which is quite likely!), but can give this a few more hours to help.
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@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? > > 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
On Thu, Aug 15, 2024 at 3:19 AM Frieder Schrempf <frieder.schrempf@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@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@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
Adam Ford wrote on Tue, Aug 20, 2024 at 09:49:03PM -0500: > > > 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. I can confirm the displays I tested also seem pretty tolerant in practice (up to ~3-4% at least), but I agree with Lucas that this isn't something we can rely on for a general purpose driver as having examples of things being tolerant doesn't mean all hardware will be as flexible.. > > 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, Great! I'll admit this also flies over my head and I don't have the time to study this, so this is much appreciated. > 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. Hmm... PHY_REG2 in the datasheet only lists '0011_1110b - 62' as example(?) values, but it doesn't say other values are reserved either, I'm not sure what to make of it. In the current driver PHY_REG_02 (driver macro) is assigned the first field of .pll_div_regs, which goes anywhere from 0x4b to 0x7b -- pretty far from 62(0x3e)... Since other frequencies have been adjusting this main diviser ratio we actually tried copying neighboring values and adjusting only that reg 2 (so the M if I get this right?), but the end result ended up not synchronizing properly every time... We didn't have time to check with a scope if the generated signal was ugly or if it just didn't lock properly, but the display worked when we just re-used the neighboring values without changing anything despite being ~3-4% off, so we took the easy way out here and didn't dig much further. > 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@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. Yes, I think the M value could be more flexible, but that'd require checking if it actually works, whereas having slightly off frequencies will most likely be OK so I don't think it's worth the effort -- happy to stick to what the datasheet describes. > 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. That'd be greatly appreciated, I don't have any strong opinion on whether that should completely replace the table, or only be used if there is no exact match in the table as these are values we know will work; but we can definitely test any patch you can throw at us here. Cheers,
On Tue, Aug 20, 2024 at 10:58 PM Dominique MARTINET <dominique.martinet@atmark-techno.com> wrote: > > Adam Ford wrote on Tue, Aug 20, 2024 at 09:49:03PM -0500: > > > > 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. > > I can confirm the displays I tested also seem pretty tolerant in > practice (up to ~3-4% at least), but I agree with Lucas that this isn't > something we can rely on for a general purpose driver as having examples > of things being tolerant doesn't mean all hardware will be as flexible.. > > > > 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, > > Great! I'll admit this also flies over my head and I don't have the > time to study this, so this is much appreciated. > > > 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. > > Hmm... PHY_REG2 in the datasheet only lists '0011_1110b - 62' as > example(?) values, but it doesn't say other values are reserved either, > I'm not sure what to make of it. > In the current driver PHY_REG_02 (driver macro) is assigned the first > field of .pll_div_regs, which goes anywhere from 0x4b to 0x7b -- pretty > far from 62(0x3e)... OK. I will experiment with increasing the range of M from being fixed at 3e to a range of 3b to 7b to see if my PMS integer calculator can get more accurate. > > Since other frequencies have been adjusting this main diviser ratio we > actually tried copying neighboring values and adjusting only that reg 2 > (so the M if I get this right?), but the end result ended up not > synchronizing properly every time... We didn't have time to check with a > scope if the generated signal was ugly or if it just didn't lock > properly, but the display worked when we just re-used the neighboring > values without changing anything despite being ~3-4% off, so we took the > easy way out here and didn't dig much further. > > > 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@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. > > Yes, I think the M value could be more flexible, but that'd require > checking if it actually works, whereas having slightly off frequencies > will most likely be OK so I don't think it's worth the effort -- happy > to stick to what the datasheet describes. > > > 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. > > That'd be greatly appreciated, I don't have any strong opinion on > whether that should completely replace the table, or only be used if > there is no exact match in the table as these are values we know will > work; but we can definitely test any patch you can throw at us here. I can't promise it'll be quick. I am not fully certain I understand how the whole thing works, but as a rule, I don't generally like look up tables if they can be calculated dynamically. adam > > > Cheers, > -- > Dominique > >
On Wed, Aug 21, 2024 at 7:45 AM Adam Ford <aford173@gmail.com> wrote: > > On Tue, Aug 20, 2024 at 10:58 PM Dominique MARTINET > <dominique.martinet@atmark-techno.com> wrote: > > > > Adam Ford wrote on Tue, Aug 20, 2024 at 09:49:03PM -0500: > > > > > 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. > > > > I can confirm the displays I tested also seem pretty tolerant in > > practice (up to ~3-4% at least), but I agree with Lucas that this isn't > > something we can rely on for a general purpose driver as having examples > > of things being tolerant doesn't mean all hardware will be as flexible.. > > > > > > 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, > > > > Great! I'll admit this also flies over my head and I don't have the > > time to study this, so this is much appreciated. > > > > > 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. > > > > Hmm... PHY_REG2 in the datasheet only lists '0011_1110b - 62' as > > example(?) values, but it doesn't say other values are reserved either, > > I'm not sure what to make of it. > > In the current driver PHY_REG_02 (driver macro) is assigned the first > > field of .pll_div_regs, which goes anywhere from 0x4b to 0x7b -- pretty > > far from 62(0x3e)... > > OK. I will experiment with increasing the range of M from being fixed > at 3e to a range of 3b to 7b to see if my PMS integer calculator can > get more accurate. > > > > > Since other frequencies have been adjusting this main diviser ratio we > > actually tried copying neighboring values and adjusting only that reg 2 > > (so the M if I get this right?), but the end result ended up not > > synchronizing properly every time... We didn't have time to check with a > > scope if the generated signal was ugly or if it just didn't lock > > properly, but the display worked when we just re-used the neighboring > > values without changing anything despite being ~3-4% off, so we took the > > easy way out here and didn't dig much further. > > > > > 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@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. > > > > Yes, I think the M value could be more flexible, but that'd require > > checking if it actually works, whereas having slightly off frequencies > > will most likely be OK so I don't think it's worth the effort -- happy > > to stick to what the datasheet describes. > > > > > 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. > > > > That'd be greatly appreciated, I don't have any strong opinion on > > whether that should completely replace the table, or only be used if > > there is no exact match in the table as these are values we know will > > work; but we can definitely test any patch you can throw at us here. > > I can't promise it'll be quick. I am not fully certain I understand > how the whole thing works, but as a rule, I don't generally like look > up tables if they can be calculated dynamically. I updated my PMS calculator, and I randomly selected 5 resolutions and they all returned an exact clock match, so I'll attempt to use the PMS integer clock instead of the fractional one. It'll be a little while longer before I can do anything with it. adam > > adam > > > > > > Cheers, > > -- > > Dominique > > > >
On Wed, Aug 21, 2024 at 8:59 PM Adam Ford <aford173@gmail.com> wrote: > > On Wed, Aug 21, 2024 at 7:45 AM Adam Ford <aford173@gmail.com> wrote: > > > > On Tue, Aug 20, 2024 at 10:58 PM Dominique MARTINET > > <dominique.martinet@atmark-techno.com> wrote: > > > > > > Adam Ford wrote on Tue, Aug 20, 2024 at 09:49:03PM -0500: > > > > > > 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. > > > > > > I can confirm the displays I tested also seem pretty tolerant in > > > practice (up to ~3-4% at least), but I agree with Lucas that this isn't > > > something we can rely on for a general purpose driver as having examples > > > of things being tolerant doesn't mean all hardware will be as flexible.. > > > > > > > > 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, > > > > > > Great! I'll admit this also flies over my head and I don't have the > > > time to study this, so this is much appreciated. > > > > > > > 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. > > > > > > Hmm... PHY_REG2 in the datasheet only lists '0011_1110b - 62' as > > > example(?) values, but it doesn't say other values are reserved either, > > > I'm not sure what to make of it. > > > In the current driver PHY_REG_02 (driver macro) is assigned the first > > > field of .pll_div_regs, which goes anywhere from 0x4b to 0x7b -- pretty > > > far from 62(0x3e)... > > > > OK. I will experiment with increasing the range of M from being fixed > > at 3e to a range of 3b to 7b to see if my PMS integer calculator can > > get more accurate. > > > > > > > > Since other frequencies have been adjusting this main diviser ratio we > > > actually tried copying neighboring values and adjusting only that reg 2 > > > (so the M if I get this right?), but the end result ended up not > > > synchronizing properly every time... We didn't have time to check with a > > > scope if the generated signal was ugly or if it just didn't lock > > > properly, but the display worked when we just re-used the neighboring > > > values without changing anything despite being ~3-4% off, so we took the > > > easy way out here and didn't dig much further. > > > > > > > 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@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. > > > > > > Yes, I think the M value could be more flexible, but that'd require > > > checking if it actually works, whereas having slightly off frequencies > > > will most likely be OK so I don't think it's worth the effort -- happy > > > to stick to what the datasheet describes. > > > > > > > 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. > > > > > > That'd be greatly appreciated, I don't have any strong opinion on > > > whether that should completely replace the table, or only be used if > > > there is no exact match in the table as these are values we know will > > > work; but we can definitely test any patch you can throw at us here. > > > > I can't promise it'll be quick. I am not fully certain I understand > > how the whole thing works, but as a rule, I don't generally like look > > up tables if they can be calculated dynamically. > > I updated my PMS calculator, and I randomly selected 5 resolutions and > they all returned an exact clock match, so I'll attempt to use the PMS > integer clock instead of the fractional one. > > It'll be a little while longer before I can do anything with it. Frieder, Using my PMS calculator and Rev 2 of the Tech Ref Manual, I think I can generate you a table entry for 51.2MHz. I don't have the ability to test it. I am still working on figuring out an algorithm to calculate the fractional divider, but 51.2MHz x 5 does not' appear to need the fractional clock divider, so I think I can just get away with the standard PMS calculations. My algorithm showed: HDMI PHY Best P = 1 HDMI PHY Best M = 64 HDMI PHY Best S = 6 HDMI PHY Best freq = 256000000 I'll try to generate a patch to get these values into the table if you're willing to test it. All, I still think it's a good idea to fall back to the PMS calculator if the fractional stuff isn't found. We could then determine which clock frequency is closer between the nearest table entry or the PMS calculator until someone can come up with an algorithm for it. adam > > adam > > > > adam > > > > > > > > > Cheers, > > > -- > > > Dominique > > > > > >
Hi Adam, On 27.08.24 2:25 AM, Adam Ford wrote: > On Wed, Aug 21, 2024 at 8:59 PM Adam Ford <aford173@gmail.com> wrote: >> >> On Wed, Aug 21, 2024 at 7:45 AM Adam Ford <aford173@gmail.com> wrote: >>> >>> On Tue, Aug 20, 2024 at 10:58 PM Dominique MARTINET >>> <dominique.martinet@atmark-techno.com> wrote: >>>> >>>> Adam Ford wrote on Tue, Aug 20, 2024 at 09:49:03PM -0500: >>>>>>> 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. >>>> >>>> I can confirm the displays I tested also seem pretty tolerant in >>>> practice (up to ~3-4% at least), but I agree with Lucas that this isn't >>>> something we can rely on for a general purpose driver as having examples >>>> of things being tolerant doesn't mean all hardware will be as flexible.. >>>> >>>>>> 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, >>>> >>>> Great! I'll admit this also flies over my head and I don't have the >>>> time to study this, so this is much appreciated. >>>> >>>>> 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. >>>> >>>> Hmm... PHY_REG2 in the datasheet only lists '0011_1110b - 62' as >>>> example(?) values, but it doesn't say other values are reserved either, >>>> I'm not sure what to make of it. >>>> In the current driver PHY_REG_02 (driver macro) is assigned the first >>>> field of .pll_div_regs, which goes anywhere from 0x4b to 0x7b -- pretty >>>> far from 62(0x3e)... >>> >>> OK. I will experiment with increasing the range of M from being fixed >>> at 3e to a range of 3b to 7b to see if my PMS integer calculator can >>> get more accurate. >>> >>>> >>>> Since other frequencies have been adjusting this main diviser ratio we >>>> actually tried copying neighboring values and adjusting only that reg 2 >>>> (so the M if I get this right?), but the end result ended up not >>>> synchronizing properly every time... We didn't have time to check with a >>>> scope if the generated signal was ugly or if it just didn't lock >>>> properly, but the display worked when we just re-used the neighboring >>>> values without changing anything despite being ~3-4% off, so we took the >>>> easy way out here and didn't dig much further. >>>> >>>>> 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@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. >>>> >>>> Yes, I think the M value could be more flexible, but that'd require >>>> checking if it actually works, whereas having slightly off frequencies >>>> will most likely be OK so I don't think it's worth the effort -- happy >>>> to stick to what the datasheet describes. >>>> >>>>> 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. >>>> >>>> That'd be greatly appreciated, I don't have any strong opinion on >>>> whether that should completely replace the table, or only be used if >>>> there is no exact match in the table as these are values we know will >>>> work; but we can definitely test any patch you can throw at us here. >>> >>> I can't promise it'll be quick. I am not fully certain I understand >>> how the whole thing works, but as a rule, I don't generally like look >>> up tables if they can be calculated dynamically. >> >> I updated my PMS calculator, and I randomly selected 5 resolutions and >> they all returned an exact clock match, so I'll attempt to use the PMS >> integer clock instead of the fractional one. >> >> It'll be a little while longer before I can do anything with it. > > Frieder, > > Using my PMS calculator and Rev 2 of the Tech Ref Manual, I think I > can generate you a table entry for 51.2MHz. I don't have the ability > to test it. I am still working on figuring out an algorithm to > calculate the fractional divider, but 51.2MHz x 5 does not' appear to > need the fractional clock divider, so I think I can just get away with > the standard PMS calculations. Thanks Adam for all the efforts. The initial request for the 51.2 MHz clock was from Dominique, not me. I was just jumping onto the bandwagon with my own use-cases. > > My algorithm showed: > HDMI PHY Best P = 1 > HDMI PHY Best M = 64 > HDMI PHY Best S = 6 > HDMI PHY Best freq = 256000000 > > I'll try to generate a patch to get these values into the table if > you're willing to test it. Any chance you can share your algorithm, so I can try to generate some table entries for the missing frequencies I encounter? If we could figure out the calculation of the fractional divider, we could generate table entries for arbitrary frequencies, which would be an easy way to add support for any strange displays out there. > > All, > > I still think it's a good idea to fall back to the PMS calculator if > the fractional stuff isn't found. We could then determine which clock > frequency is closer between the nearest table entry or the PMS > calculator until someone can come up with an algorithm for it. Yes, I think that's a good idea. Thanks Frieder
Frieder Schrempf wrote on Tue, Aug 27, 2024 at 09:00:34AM +0200: > > Using my PMS calculator and Rev 2 of the Tech Ref Manual, I think I > > can generate you a table entry for 51.2MHz. I don't have the ability > > to test it. I am still working on figuring out an algorithm to > > calculate the fractional divider, but 51.2MHz x 5 does not' appear to > > need the fractional clock divider, so I think I can just get away with > > the standard PMS calculations. > > Thanks Adam for all the efforts. The initial request for the 51.2 MHz > clock was from Dominique, not me. I was just jumping onto the bandwagon > with my own use-cases. We still have that 51.2 MHz display around, so happy to test -- might take a little bit of time as this already has a workaround but it's definitely something I'd like to clean up so more than happy to help. > > I still think it's a good idea to fall back to the PMS calculator if > > the fractional stuff isn't found. We could then determine which clock > > frequency is closer between the nearest table entry or the PMS > > calculator until someone can come up with an algorithm for it. > > Yes, I think that's a good idea. Seconded; we can keep the code as fallback of the table at first and if it works well maybe even remove the table altogether later, but just having fallback first would be great.
diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig index a4d13331e320..5965e8027529 100644 --- a/drivers/gpu/drm/bridge/imx/Kconfig +++ b/drivers/gpu/drm/bridge/imx/Kconfig @@ -3,6 +3,17 @@ if ARCH_MXC || COMPILE_TEST config DRM_IMX_LDB_HELPER tristate +config DRM_IMX8MP_DW_HDMI_BRIDGE + tristate "Freescale i.MX8MP HDMI-TX bridge support" + depends on OF + depends on COMMON_CLK + select DRM_DW_HDMI + select DRM_IMX8MP_HDMI_PVI + select PHY_FSL_SAMSUNG_HDMI_PHY + help + Choose this to enable support for the internal HDMI encoder found + on the i.MX8MP SoC. + config DRM_IMX8MP_HDMI_PVI tristate "Freescale i.MX8MP HDMI PVI bridge support" depends on OF diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile index e2c2106509fa..edb0a7b71b30 100644 --- a/drivers/gpu/drm/bridge/imx/Makefile +++ b/drivers/gpu/drm/bridge/imx/Makefile @@ -1,4 +1,5 @@ obj-$(CONFIG_DRM_IMX_LDB_HELPER) += imx-ldb-helper.o +obj-$(CONFIG_DRM_IMX8MP_DW_HDMI_BRIDGE) += imx8mp-hdmi-tx.o obj-$(CONFIG_DRM_IMX8MP_HDMI_PVI) += imx8mp-hdmi-pvi.o obj-$(CONFIG_DRM_IMX8QM_LDB) += imx8qm-ldb.o obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o 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 @@ -0,0 +1,154 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/* + * Copyright (C) 2022 Pengutronix, Lucas Stach <kernel@pengutronix.de> + */ + +#include <linux/clk.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <drm/bridge/dw_hdmi.h> +#include <drm/drm_modes.h> + +struct imx8mp_hdmi { + struct dw_hdmi_plat_data plat_data; + struct dw_hdmi *dw_hdmi; + struct clk *pixclk; +}; + +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; + + /* We don't support double-clocked and Interlaced modes */ + if ((mode->flags & DRM_MODE_FLAG_DBLCLK) || + (mode->flags & DRM_MODE_FLAG_INTERLACE)) + return MODE_BAD; + + return MODE_OK; +} + +static int imx8mp_hdmi_phy_init(struct dw_hdmi *dw_hdmi, void *data, + const struct drm_display_info *display, + const struct drm_display_mode *mode) +{ + return 0; +} + +static void imx8mp_hdmi_phy_disable(struct dw_hdmi *dw_hdmi, void *data) +{ +} + +static void im8mp_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data) +{ + /* + * Just release PHY core from reset, all other power management is done + * by the PHY driver. + */ + dw_hdmi_phy_gen1_reset(hdmi); + + dw_hdmi_phy_setup_hpd(hdmi, data); +} + +static const struct dw_hdmi_phy_ops imx8mp_hdmi_phy_ops = { + .init = imx8mp_hdmi_phy_init, + .disable = imx8mp_hdmi_phy_disable, + .setup_hpd = im8mp_hdmi_phy_setup_hpd, + .read_hpd = dw_hdmi_phy_read_hpd, + .update_hpd = dw_hdmi_phy_update_hpd, +}; + +static int imx8mp_dw_hdmi_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct dw_hdmi_plat_data *plat_data; + struct imx8mp_hdmi *hdmi; + + hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL); + if (!hdmi) + return -ENOMEM; + + plat_data = &hdmi->plat_data; + + hdmi->pixclk = devm_clk_get(dev, "pix"); + if (IS_ERR(hdmi->pixclk)) + return dev_err_probe(dev, PTR_ERR(hdmi->pixclk), + "Unable to get pixel clock\n"); + + plat_data->mode_valid = imx8mp_hdmi_mode_valid; + plat_data->phy_ops = &imx8mp_hdmi_phy_ops; + plat_data->phy_name = "SAMSUNG HDMI TX PHY"; + plat_data->priv_data = hdmi; + plat_data->phy_force_vendor = true; + + hdmi->dw_hdmi = dw_hdmi_probe(pdev, plat_data); + if (IS_ERR(hdmi->dw_hdmi)) + return PTR_ERR(hdmi->dw_hdmi); + + platform_set_drvdata(pdev, hdmi); + + return 0; +} + +static int imx8mp_dw_hdmi_remove(struct platform_device *pdev) +{ + struct imx8mp_hdmi *hdmi = platform_get_drvdata(pdev); + + dw_hdmi_remove(hdmi->dw_hdmi); + + return 0; +} + +static int __maybe_unused imx8mp_dw_hdmi_pm_suspend(struct device *dev) +{ + return 0; +} + +static int __maybe_unused imx8mp_dw_hdmi_pm_resume(struct device *dev) +{ + struct imx8mp_hdmi *hdmi = dev_get_drvdata(dev); + + dw_hdmi_resume(hdmi->dw_hdmi); + + return 0; +} + +static const struct dev_pm_ops imx8mp_dw_hdmi_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(imx8mp_dw_hdmi_pm_suspend, + imx8mp_dw_hdmi_pm_resume) +}; + +static const struct of_device_id imx8mp_dw_hdmi_of_table[] = { + { .compatible = "fsl,imx8mp-hdmi-tx" }, + { /* Sentinel */ } +}; +MODULE_DEVICE_TABLE(of, imx8mp_dw_hdmi_of_table); + +static struct platform_driver imx8mp_dw_hdmi_platform_driver = { + .probe = imx8mp_dw_hdmi_probe, + .remove = imx8mp_dw_hdmi_remove, + .driver = { + .name = "imx8mp-dw-hdmi-tx", + .of_match_table = imx8mp_dw_hdmi_of_table, + .pm = &imx8mp_dw_hdmi_pm_ops, + }, +}; + +module_platform_driver(imx8mp_dw_hdmi_platform_driver); + +MODULE_DESCRIPTION("i.MX8MP HDMI encoder driver"); +MODULE_LICENSE("GPL");