[PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock
Alexander Stein
alexander.stein at ew.tq-group.com
Wed Jun 5 10:52:35 UTC 2024
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 at 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 at 0 {
> reg = <0>;
>
> tc_bridge_in: endpoint {
> data-lanes = <1 2 3 4>;
> remote-endpoint = <&dsi_out>;
> };
> };
>
> port at 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 at 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/
More information about the dri-devel
mailing list