GPU/DRM issue with DSI commands on msm 8916

[Archit Taneja]

Hi Daniel,

On Tuesday 17 April 2018 05:51 PM, Daniel Mack wrote:
> (cc Stephen)
> 
> Hi Archit,
> 
> On Monday, April 16, 2018 07:06 PM, Daniel Mack wrote:
>> On Monday, April 09, 2018 03:08 PM, Archit Taneja wrote:
>>>>> You could comment out the pm_runtime_put_sync() calls in
>>>>> drivers/gpu/drm/msm/dsi/dsi_host.c, in case some registers got
>>>>> reset during put_sync and weren't restored correctly after get_sync().
>>>>
>>>> That was my first suspicion too, but unfortunately, that's not the culprit.
>>>> I think it would be good to comment out the put_sync() calls in
>>> drivers/gpu/drm/msm/mdp/mdp5/mdp5_mdss.c and
>>> drivers/gpu/drm/msm/msm_drv.c too, since there is a parent-child
>>> hierarchy between DSI
>>> and the top level MDSS block. Commenting out the put_syncs() just
>>> in put_sync() might still result in the MDSS GDSC to switch off.
>>
>> I spent some more time debugging this today and it turns out that
>> calling into dsi_link_clk_enable() from msm_dsi_host_xfer_prepare() is
>> already causing the drop-outs, even when no command buffers, DMA
>> transfers etc. are involved. I then drilled further down and it showed
>> that at least
>>
>>    clk_set_rate(msm_host->byte_clk, msm_host->byte_clk_rate);
>>
>> in dsi_link_clk_enable_6g() one of the culprits. If I don't touch the
>> clocks anymore after the initialization is done, everything is fine.
> 
> Okay, I finally found the bits between the two trees that make the
> difference. It's a downstream patch that is included in the Linaro 4.9
> branch but that's missing upstream and in their 4.14 branch:
> 
> 
> https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/commit/?h=release/qcomlt-4.9&id=4ce2d6108d
> 
> What this patch does as a side-effect is that is sets the clock's actual
> rate to the requested rate (core->rate = core->new_rate), and that fixes
> the problem I'm seeing.
> 

Thanks for debugging this so thoroughly.

> It shows an underlying problem in the msm driver's clock components
> though, because without this patch, the clocks will be effectively
> slightly off from what was requested. For instance, when
> gcc_mdss_byte0_clk is configured to 56250000 Hz by the msm drm driver,
> the clk core will let the DSI PLL recalculate its actual rate which is
> 56246337 Hz. Hence, clk_set_rate() will always end up fiddling with the
> knobs of that clock provider. That's what happens every time DSI
> commands are sent, and that causes the image to flicker.

If I understood right, the main cause of the flicker is that we always
end up re-locking/reconfiguring the DSI PLL every time we send a command
(since dsi_link_clk_enable() is called in msm_dsi_host_xfer_prepare())?
The re-configuration results in a glitch on the DSI clock, which is
probably unacceptable for DSI Video mode transfer, especially for panels
that don't have their own timing generators, which rely entirely on
DSI clock lanes for scanning out the pixel data.

According to you, the reason why the reconfiguration happens is because
the DSI PLL was never set exactly to 56.25 Mhz in the first place, and
the core clock framework notices a difference in the requested rate and 
the current rate (56.246 Mhz), and goes ahead to configure the PLL
again when it's not needed. And this was averted in the downstream
patch you mentioned as a side affect?


> 
> The same problem applies to other clocks too. dsi0vco_clk for example
> will be 449970703 rather than the requested 450000000 etc.
> 

One easy way to get around this would be to not try to set the clock
rates every time we try to send a command. We just enable/disable them.
It would hide the real problem, though.

> I guess a way to fix this properly would be to use
> msm_dsi_pll_helper_clk_round_rate() to actually round the rates, but I'm
> not sure.

Stephen,

Do you have any suggestions on how we can manage this? I.e we set a
clock rate, it goes through, but it's very slightly different than what
we asked for. The next time we try to set the same rate, the clock
provider driver goes through the whole ordeal of reconfiguring the
HW to reach to the same configuration.

Thanks,
Archit

> 
> 
> Thanks,
> Daniel
>