Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hi Marek,

Am Dienstag, 4. Juni 2024, 18:17:53 CEST schrieb Marek Vasut:
> On 6/4/24 1:35 PM, Alexander Stein wrote:
> > Hi Marek,
> 
> Hi,
> 
> > Am Montag, 3. Juni 2024, 23:27:34 CEST schrieb Marek Vasut:
> >> On 6/3/24 2:45 PM, Alexander Stein wrote:
> >>
> >> Hi,
> >>
> >>>> @@ -1631,6 +1643,18 @@ static int tc_edp_atomic_check(struct drm_bridge *bridge,
> >>>>    			       struct drm_crtc_state *crtc_state,
> >>>>    			       struct drm_connector_state *conn_state)
> >>>>    {
> >>>> +	struct tc_data *tc = bridge_to_tc(bridge);
> >>>> +	int adjusted_clock = 0;
> >>>> +	int ret;
> >>>> +
> >>>> +	ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
> >>>> +			      crtc_state->adjusted_mode.clock * 1000,
> >>>> +			      &adjusted_clock, NULL);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	crtc_state->adjusted_mode.clock = adjusted_clock / 1000;
> >>>
> >>> This is prone to rounding errors. Debug output in my case:
> >>>> [   16.007127] tc358767 1-000f: enable video stream
> >>>> [   16.007148] tc358767 1-000f: PLL: requested 148500000 pixelclock, ref 26000000
> >>>> [   16.007163] tc358767 1-000f: PLL: got 147333333, delta -1166667
> >>>> [   16.007169] tc358767 1-000f: PLL: 26000000 / 1 / 1 * 17 / 3
> >>>> [   16.027112] tc358767 1-000f: set mode 1920x1080
> >>>> [   16.027138] tc358767 1-000f: H margin 148,88 sync 44
> >>>> [   16.027144] tc358767 1-000f: V margin 36,4 sync 5
> >>>> [   16.027150] tc358767 1-000f: total: 2200x1125
> >>>> [   16.059426] tc358767 1-000f: PLL: requested 147333000 pixelclock, ref 26000000
> >>>> [   16.059455] tc358767 1-000f: PLL: got 146250000, delta -1083000
> >>>> [   16.059461] tc358767 1-000f: PLL: 26000000 / 1 / 4 * 45 / 2
> >>>> [   16.095724] tc358767 1-000f: PLL: requested 146250000 pixelclock, ref 26000000
> >>>> [   16.095739] tc358767 1-000f: PLL: got 146250000, delta 0
> >>>> [   16.095745] tc358767 1-000f: PLL: 26000000 / 1 / 4 * 45 / 2
> >>>
> >>> The accuracy degrades with each call, until a full kHz frequency is reached,
> >>> because drm_display_mode.clock only accounts for kHz, but the PLL
> >>> calculation takes Hz into account.
> >>
> >> Hmmmmm, I need to take a closer look at this one.
> >>
> >> Do you have any quick hints ?
> 
> So the fix is real simple, try this extra diff:
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c 
> b/drivers/gpu/drm/bridge/tc358767.c
> index 32639865fea07..5d76285dcf245 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1629,7 +1629,7 @@ static int tc_dpi_atomic_check(struct drm_bridge 
> *bridge,
>          int ret;
> 
>          ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
> -                             crtc_state->adjusted_mode.clock * 1000,
> +                             crtc_state->mode.clock * 1000,
>                                &adjusted_clock, NULL);
>          if (ret)
>                  return ret;
> @@ -1653,7 +1653,7 @@ static int tc_edp_atomic_check(struct drm_bridge 
> *bridge,
>          int ret;
> 
>          ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
> -                             crtc_state->adjusted_mode.clock * 1000,
> +                             crtc_state->mode.clock * 1000,
>                                &adjusted_clock, NULL);
>          if (ret)
>                  return ret;
> 

Ah, right. That seems simple. But I noticed another thing:
The TC9595 PLL is configured to 147333333 while the lcdif configures
the display clock to 147333000, the value actually stored in
adjusted_mode.clock. I do not know if this small difference can be neglected.

> > No, sorry. I'm not sure about those VFIFO overruns/underruns you mentioned
> > in the commit message. Does this maybe only apply to DPI input?
> 
> No, this actually happens with DSI input in both DPI and (e)DP output 
> modes, it is only really well visible in some resolutions it seems.

Ah, i see.

> > At least for 148.5MHz (1080p) apparently it is not possible to that
> > exact clock anyway.
> 
> Right, which sucks, but the TC9595 datasheet explicitly states that the 
> FRMSYNC mode should always be enabled (and is apparently force-enabled 
> on newer bridge chips with no option to disable it) unless the Pixel 
> clock are sourced from DSI clock (which is never the case with this 
> driver). That's what the [1] patch does.
> 
> But messing with the HFP isn't really working for all resolutions, so 
> this patch instead adjusts the input pixel clock to fit the Pixel PLL 
> limits, which avoids the VFIFO issues altogether. And that makes the 
> FRMSYNC mode work for all kinds of resolutions.

I can't tell if VFIFO issue arise here, because I'm currently trying to
get a reliable and stable output for DP. The documentation is somewhat
limited, but FRAMSYNC seems almost necessary in any case, unless you
utilize DSI bus 100% for this device to get the correct display timings
using DSI packets.

> You might be wondering why not source pixel clock from DSI HS clock and 
> disable FRMSYNC. For one thing, this would limit the ability to pick 
> faster DSI HS clock and make good use of the DSI burst mode (the DSI 
> link can go into LP state after transmitting entire line of pixel data 
> for longer with faster DSI HS clock). The other thing is, to drive the 
> Pixel PLL (not to be confused with pixel clock) from DSI HS clock, the 
> DSI HS clock have to be set to specific clock rates (13*4*7=364 MHz and 
> 13*4*9=468 MHz), which is really limiting.

This is not mentioned in the datasheet at all, but just in a small note
in the calculation sheet, without any description. On a different platform
DSI HS clock running at 445.5MHz seems also possible, even if unsupported.

> >>> BTW: Which platform are you testing on?
> >>
> >> MX8MP with LCDIFv3 -> DSIM -> TC9595 -> DP output.
> >>
> >> The TC9595 is 2nd generation (automotive?) replacement for TC358767 (1st
> >> generation replacement is TC358867) .
> > 
> > Same here. But fail to get output on a DP monitor if I'm running from
> > external refclk. Using DSICLK/4 seems necessary for some reason, but it
> > is very unreliable to get a proper image.
> 
> Do you have all the patches in place ? This is what you should have:
> 
> drm/bridge: tc358767: Enable FRMSYNC timing generator
> drm: bridge: samsung-dsim: Initialize bridge on attach
> drm/bridge: tc358767: Reset chip again on attach
> clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate
> drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock
> drm/bridge: tc358767: Fix comment in tc_edp_mode_valid
> drm/bridge: tc358767: Check if fully initialized before signalling HPD 
> event via IRQ
> drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation 
> and enablement
> drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock
> drm/bridge: tc358767: Drop line_pixel_subtract
> drm/bridge: tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS
> drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1
> Revert "drm/bridge: tc358767: Set default CLRSIPO count"
> drm/bridge: tc358767: Add configurable default preemphasis

Thanks, I was missing the lcdif and clk-imx8mp patches. I saw them separately
on the mailing list, but didn't realize would need them.

> I only use external refclk, at 26 MHz.

Same for me, difference is I'm using a crystal oscillator instead of
CCM_CLKOUT. And yes, this is the TQ platform TQMa8MPxL/MBa8MPxL.

> > Which display are you using? Do you mind sharing your DT?
> 
> HP LA2405wg , ancient 1920x1200 one.

ASUS PB238, 1920x1080 here.

> Relevant parts are below:
> 
> // This is in I2C controller node, it actually is modified
> // arch/arm64/boot/dts/freescale/imx8mp-dhcom-som.dtsi
> tc_bridge: bridge@f {
>          compatible = "toshiba,tc9595", "toshiba,tc358767";
>          pinctrl-names = "default";
>          pinctrl-0 = <&pinctrl_tc9595>;
>          reg = <0xf>;
>          clock-names = "ref";
>          clocks = <&clk IMX8MP_CLK_CLKOUT2>;
>          assigned-clocks = <&clk IMX8MP_CLK_CLKOUT2_SEL>,
>                            <&clk IMX8MP_CLK_CLKOUT2>,
>                            <&clk IMX8MP_AUDIO_PLL2_OUT>;
>          assigned-clock-parents = <&clk IMX8MP_AUDIO_PLL2_OUT>;
>          assigned-clock-rates = <26000000>, <26000000>, <416000000>;
>          reset-gpios = <&gpio4 1 GPIO_ACTIVE_HIGH>;
>          status = "okay";
> 
>          ports {
>                  #address-cells = <1>;
>                  #size-cells = <0>;
> 
>                  port@0 {
>                          reg = <0>;
> 
>                          tc_bridge_in: endpoint {
>                                  data-lanes = <1 2 3 4>;
>                                  remote-endpoint = <&dsi_out>;
>                          };
>                  };
> 
>                  port@2 { // This is optional
>                          reg = <2>;
>                          toshiba,pre-emphasis = /bits/ 8 <1 1>;
>                  };
>          };
> };
> 
> // 1 GHz burst clock is the maximum the bridge can do
> &mipi_dsi {
>          samsung,burst-clock-frequency = <1000000000>;
>          samsung,esc-clock-frequency = <10000000>;
>          status = "okay";
> 
>          ports {
>                  port@1 {
>                          reg = <1>;
> 
>                          dsi_out: endpoint {
>                                  data-lanes = <1 2 3 4>;
>                                  remote-endpoint = <&tc_bridge_in>;
>                          };
>                  };
>          };
> };

Thanks for sharing, despite the fixed-clock and IMX8MP_CLK_CLKOUT2 this
looks very much the same. Unfortunately I get an output on DP just sometimes.
As Bit 0 in register 0x464 is not cleared, I suspect the bridge is not
recognizing DSI (VSYNC) packets properly :(
At some point this bridge is lenient, but picky otherwise.

> // This is to let LCDIF configure the pixel clock,
> // it removes the fixed Video PLL configuration from DT
> &media_blk_ctrl {
>         assigned-clocks = <&clk IMX8MP_CLK_MEDIA_AXI>,
>                           <&clk IMX8MP_CLK_MEDIA_APB>,
>                           <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>,
>                           <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>;
>         assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>,
>                                  <&clk IMX8MP_SYS_PLL1_800M>,
>                                  <&clk IMX8MP_VIDEO_PLL1_OUT>,
>                                  <&clk IMX8MP_SYS_PLL3_OUT>;
>         assigned-clock-rates = <500000000>, <200000000>,
>                                <0>, <0>;
> };

I get the intention, but this might change once you want to enable ISP/DWE.
But that is a different matter for now.

Best regards,
Alexander

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/