On Tue, Feb 2, 2021 at 11:08 AM Rob Clark <robdclark@xxxxxxxxx> wrote: > > On Tue, Feb 2, 2021 at 10:46 AM AngeloGioacchino Del Regno > <angelogioacchino.delregno@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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/heads/main/chipset-qc845/dev-util/testclock/files/testclock.py > > 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
