[PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock
Marek Vasut
marex at denx.de
Tue Jun 4 16:17:53 UTC 2024
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;
> 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.
> 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.
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.
>>> 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
I only use external refclk, at 26 MHz.
> Which display are you using? Do you mind sharing your DT?
HP LA2405wg , ancient 1920x1200 one.
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>;
};
};
};
};
// 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>;
};
More information about the dri-devel
mailing list