On Tue, Feb 2, 2021 at 11:08 AM Rob Clark robdclark@gmail.com wrote:
On Tue, Feb 2, 2021 at 10:46 AM AngeloGioacchino Del Regno angelogioacchino.delregno@somainline.org wrote:
Il 02/02/21 19:45, Rob Clark ha scritto:
On Tue, Feb 2, 2021 at 6:32 AM AngeloGioacchino Del Regno angelogioacchino.delregno@somainline.org wrote:
Il 01/02/21 18:31, Rob Clark ha scritto:
fwiw, this is the clk_summary diff with and without this patch:
270,282c270,282 < dsi0_pll_out_div_clk 1 1 0 887039941 0 0 50000 Y < dsi0_pll_post_out_div_clk 0 0 0 221759985 0 0 50000 Y < dsi0_pll_bit_clk 2 2 0 887039941 0 0 50000 Y < dsi0_pclk_mux 1 1 0 887039941 0 0 50000 Y < dsi0_phy_pll_out_dsiclk 1 1 0 147839991 0 0 50000 Y < disp_cc_mdss_pclk0_clk_src 1 1 0 147839991 0 0 50000 Y < disp_cc_mdss_pclk0_clk 1 1 0 147839991 0 0 50000 Y < dsi0_pll_by_2_bit_clk 0 0 0 443519970 0 0 50000 Y < dsi0_phy_pll_out_byteclk 1 1 0 110879992 0 0 50000 Y < disp_cc_mdss_byte0_clk_src 2 2 0 110879992 0 0 50000 Y < disp_cc_mdss_byte0_div_clk_src 1 1 0 55439996 0 0 50000 Y < disp_cc_mdss_byte0_intf_clk 1 1 0 55439996 0 0 50000 Y < disp_cc_mdss_byte0_clk 1 1 0 110879992 0 0 50000 Y
dsi0_pll_out_div_clk 1 1 0 887039978 0 0 50000 Y dsi0_pll_post_out_div_clk 0 0 0 221759994 0 0 50000 Y dsi0_pll_bit_clk 2 2 0 887039978 0 0 50000 Y dsi0_pclk_mux 1 1 0 887039978 0 0 50000 Y dsi0_phy_pll_out_dsiclk 1 1 0 147839997 0 0 50000 Y disp_cc_mdss_pclk0_clk_src 1 1 0 147839997 0 0 50000 Y disp_cc_mdss_pclk0_clk 1 1 0 147839997 0 0 50000 Y dsi0_pll_by_2_bit_clk 0 0 0 443519989 0 0 50000 Y dsi0_phy_pll_out_byteclk 1 1 0 110879997 0 0 50000 Y disp_cc_mdss_byte0_clk_src 2 2 0 110879997 0 0 50000 Y disp_cc_mdss_byte0_div_clk_src 1 1 0 55439999 0 0 50000 Y disp_cc_mdss_byte0_intf_clk 1 1 0 55439999 0 0 50000 Y disp_cc_mdss_byte0_clk 1 1 0 110879997 0 0 50000 Y
This is almost exactly what I saw on my devices as well, you get a difference of "just some Hz" (which can be totally ignored), because of how the calculation is done now.
Thing is, what you see as PIXEL and BYTE clocks *before* the change is Linux thinking that your DSI is at that frequency, while the PLL will output *half* the rate, which is exactly what the patch fixes.
"Fun" story is: the Xperia XZ1 (8998) and XZ (8996) have got the same display... by lowering the DSI rate on the MSM8996 phone by half, I get the same *identical* issues as the 8998 one without this patch. The clocks all match between one and another, because.. it's.. the same display, after all.
It is because of the aforementioned test that I have raised doubts about the TI chip driver (or anything else really).. but then, anything is possible.
It does look like, *so far* the TI bridge chip is only used on qc platforms (according to grep'ing dts), so I suppose I can't rule out bugs which cancel each other out. Although there are various other bridges used (for ex, the sdm845 rb3 board has some dsi->hdmi bridge)
Argh...
I guess it would be useful if we could measure the clk somehow to confirm that it is running at the rate we think it is..
I totally agree with you on this, I actually wanted to do it the proper way, but then these clocks are really too high for my cheap oscilloscope and I couldn't... :(
Ok, there might actually be a way to do this.. there is a testclock utility (added sboyd who wrote it in CC):
https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/refs/...
there is a similar thing for sc7180.. for other devices, I guess it would require some porting..
Looking at disp_cc_mdss_byte0_clk and disp_cc_mdss_pclk0_clk on sc7180, the rates returned by testclock roughly match what is in clk_summary, ie. within rounding error
So Doug Anderson pointed out that we can actually read the DSI clock from the bridge, if we put it in "automatic" mode, from him:
1. Boot up. 2. i2cget -f -y 2 0x2d 0x12 # reads calculated rate that we programmed 3. i2cset -f -y 2 0x2d 0x12 0 # switch to automatic mode 4. i2cget -f -y 2 0x2d 0x12 # reads measured rate; repeat this a bunch and it should go up/down by 1 since measurement isn't perfect.
What I see without this patch:
localhost ~ # i2cget -f -y 2 0x2d 0x12 0x58 localhost ~ # i2cset -f -y 2 0x2d 0x12 0 localhost ~ # i2cget -f -y 2 0x2d 0x12 0x58 localhost ~ # i2cget -f -y 2 0x2d 0x12 0x59 localhost ~ # i2cget -f -y 2 0x2d 0x12 0x58 localhost ~ # i2cget -f -y 2 0x2d 0x12 0x58 localhost ~ # i2cget -f -y 2 0x2d 0x12 0x58 localhost ~ #
And with this patch:
localhost ~ # i2cget -f -y 2 0x2d 0x12 0x58 localhost ~ # i2cset -f -y 2 0x2d 0x12 0 localhost ~ # i2cget -f -y 2 0x2d 0x12 0xb1 localhost ~ # i2cget -f -y 2 0x2d 0x12 0xb2 localhost ~ # i2cget -f -y 2 0x2d 0x12 0xb2 localhost ~ #
So this patch is doubling the rate
BR, -R