[PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler

AngeloGioacchino Del Regno angelogioacchino.delregno at somainline.org
Tue Feb 2 18:46:52 UTC 2021


Il 02/02/21 19:45, Rob Clark ha scritto:
> On Tue, Feb 2, 2021 at 6:32 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno at somainline.org> wrote:
>>
>> Il 01/02/21 18:31, Rob Clark ha scritto:
>>> On Mon, Feb 1, 2021 at 9:18 AM Rob Clark <robdclark at gmail.com> wrote:
>>>>
>>>> On Mon, Feb 1, 2021 at 9:05 AM Rob Clark <robdclark at gmail.com> wrote:
>>>>>
>>>>> On Mon, Feb 1, 2021 at 7:47 AM Rob Clark <robdclark at gmail.com> wrote:
>>>>>>
>>>>>> On Mon, Feb 1, 2021 at 2:11 AM AngeloGioacchino Del Regno
>>>>>> <angelogioacchino.delregno at somainline.org> wrote:
>>>>>>>
>>>>>>> Il 31/01/21 20:50, Rob Clark ha scritto:
>>>>>>>> On Sat, Jan 9, 2021 at 5:51 AM AngeloGioacchino Del Regno
>>>>>>>> <angelogioacchino.delregno at somainline.org> wrote:
>>>>>>>>>
>>>>>>>>> The VCO rate was being miscalculated due to a big overlook during
>>>>>>>>> the process of porting this driver from downstream to upstream:
>>>>>>>>> here we are really recalculating the rate of the VCO by reading
>>>>>>>>> the appropriate registers and returning a real frequency, while
>>>>>>>>> downstream the driver was doing something entirely different.
>>>>>>>>>
>>>>>>>>> In our case here, the recalculated rate was wrong, as it was then
>>>>>>>>> given back to the set_rate function, which was erroneously doing
>>>>>>>>> a division on the fractional value, based on the prescaler being
>>>>>>>>> either enabled or disabled: this was actually producing a bug for
>>>>>>>>> which the final VCO rate was being doubled, causing very obvious
>>>>>>>>> issues when trying to drive a DSI panel because the actual divider
>>>>>>>>> value was multiplied by two!
>>>>>>>>>
>>>>>>>>> To make things work properly, remove the multiplication of the
>>>>>>>>> reference clock by two from function dsi_pll_calc_dec_frac and
>>>>>>>>> account for the prescaler enablement in the vco_recalc_rate (if
>>>>>>>>> the prescaler is enabled, then the hardware will divide the rate
>>>>>>>>> by two).
>>>>>>>>>
>>>>>>>>> This will make the vco_recalc_rate function to pass the right
>>>>>>>>> frequency to the (clock framework) set_rate function when called,
>>>>>>>>> which will - in turn - program the right values in both the
>>>>>>>>> DECIMAL_DIV_START_1 and the FRAC_DIV_START_{LOW/MID/HIGH}_1
>>>>>>>>> registers, finally making the PLL to output the right clock.
>>>>>>>>>
>>>>>>>>> Also, while at it, remove the prescaler TODO by also adding the
>>>>>>>>> possibility of disabling the prescaler on the PLL (it is in the
>>>>>>>>> PLL_ANALOG_CONTROLS_ONE register).
>>>>>>>>> Of course, both prescaler-ON and OFF cases were tested.
>>>>>>>>
>>>>>>>> This somehow breaks things on sc7180 (display gets stuck at first
>>>>>>>> frame of splash screen).  (This is a setup w/ an ti-sn65dsi86 dsi->eDP
>>>>>>>> bridge)
>>>>>>>>
>>>>>>>
>>>>>>> First frame of the splash means that something is "a bit" wrong...
>>>>>>> ...like the DSI clock is a little off.
>>>>>>>
>>>>>>> I don't have such hardware, otherwise I would've tried... but what you
>>>>>>> describe is a bit strange.
>>>>>>> Is there any other older qcom platform using this chip? Any other
>>>>>>> non-qcom platform? Is the driver for the SN65DSI86 surely fine?
>>>>>>> Anyway, as you know, I would never propose untested patches nor
>>>>>>> partially working ones for any reason: I'm sorry that this happened.
>>>>>>
>>>>>> I don't think there is anything publicly avail w/ sc7180 (yet.. but very soon)
>>>>>>
>>>>>> The ti-sn65dsi86 bridge is used on a bunch of 845/850 devices (like
>>>>>> the snapdragon windows laptops).. and I think also the older 835
>>>>>> laptops.. ofc that doesn't mean that there isn't some bug, but I'd
>>>>>> guess maybe more likely that there is some small difference in DSI vs
>>>>>> older devices, or some cmd vs video mode difference.
>>>>>>
>>>>>> Anyways, seems like the screen did eventually recover so that gives me
>>>>>> a bit of confidence to bisect this series, which I'll do a bit later
>>>>>> today.
>>>>>
>>>>> fwiw, this series minus this patch, and everything looks ok.. let me
>>>>> take a closer look at what changes with this patch
>>>>
>>>> Btw, it looks like upstream, config->disable_prescaler is always
>>>> false.. I don't suppose you have anything WIP that changes this?
>>>
>>
>> Regarding that one, I have tested the driver in both cases, with
>> and without prescaler enabled (both worked fine), then I have decided
>> to leave the prescaler option exactly as the previous default.
>>
>> My plan about this was/still is:
>> 1. Wait until this one gets merged (gives me time to also look
>>      at the other billion patches that I've sent);
>> 2. Add the prescaler option DT property and explain that it has
>>      to be used only with "puny" displays (low resolution, low
>>      clocks) as with "good ones", enabling the prescaler gives less
>>      clock jitter (and some microamps more power consumption);
>> 3. Add the Spread Spectrum Clock (SSC) functionality with related
>>      DT properties.
>>
>> Point 2 and 3 would go in the same series, unless someone does
>> N.2 before I do... and N.3 requires a bit of extensive testing,
>> which I have already partially started on the FxTec phone.
>>
>>> 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... :(

> BR,
> -R
> 
> 
>>>>>
>>>>>>> In any case, just to be perfectly transparent, while being here waiting
>>>>>>> for review, this patch series got tested on more smartphones, even ones
>>>>>>> that I don't personally own, with different displays.
>>>>>>>
>>>>>>> For your reference, here's a list (all MSM8998..):
>>>>>>> - OnePlus 5               (1920x1080)
>>>>>>> - F(x)Tec Pro 1           (2160x1080)
>>>>>>> - Sony Xperia XZ1 Compact (1280x720)
>>>>>>> - Sony Xperia XZ1         (1920x1080)
>>>>>>> - Sony Xperia XZ Premium  (3840x2160)
>>>>>>>
>>>>>>
>>>>>> Yeah, no worries, I wasn't trying to imply that the patch was untested.
>>>>>>
>>
>> I know, of course!
>>
>>>>>> Out of curiosity, are any of those video mode panels?
>>
>> Yes and "also":
>> The FxTec Pro1 has a video mode panel, for which I'm trying to upstream
>> the driver...look here: https://lore.kernel.org/patchwork/patch/1365228/
>>
>> The Xperia XZ Premium has a Sharp LS055D1SX04 panel under NT35950, which
>> can be configured as command or as video mode... I tried both modes, but
>> there is some issue with the DPU1/DSI drivers and *DUAL DSI*, as cmd
>> does work with some tearing, but video doesn't even start (downstream it
>> works).
>>
>> So the only video mode panel that I could test is that BOE panel on the
>> FxTec phone (single dsi), which works just great.
>>
>>>>>>
>>>>>>>
>>>>>>>> Also, something (I assume DSI related) that I was testing on
>>>>>>>> msm-next-staging seems to have effected the colors on the panel (ie.
>>>>>>>> they are more muted).. which seems to persist across reboots (ie. when
>>>>>>>
>>>>>>> So much "fun". This makes me think something about the PCC block doing
>>>>>>> the wrong thing (getting misconfigured).
>>>>>>>
>>>>>>>> switching back to a good kernel), and interestingly if I reboot from a
>>>>>>>> good kernel I see part of the login prompt (or whatever was previously
>>>>>>>> on-screen) in the firmware ui screen !?!  (so maybe somehow triggered
>>>>>>>> the display to think it is in PSR mode??)
>>>>>>>>
>>>>>>>
>>>>>>>    From a fast read, the SN65DSI86 is on I2C.. giving it a wrong dsi clock
>>>>>>> cannot produce (logically, at least) this, so I say that it is very
>>>>>>> unlikely for this to be a consequence of the 10nm pll fixes...
>>>>>>>
>>>>>>
>>>>>> Note that the bridge can also be programmed via dsi cmd mode packets,
>>>>>> which I believe is the case on the 835 laptops (or at least one of
>>>>>> them).. but all the things I have are using i2c as the control path.
>>>>>>
>>>>>>> ...unless the bootloader is not configuring the DSI rates, but that's
>>>>>>> also veeeeery unlikely (it always does, or it always does not).
>>>>>>
>>>>>> I haven't looked at the bootloader display code, but booting back to
>>>>>> an old/good kernel didn't change anything..  even powering off didn't.
>>>>>> But the ghost image seemed to fade after some time, and by the next
>>>>>> morning it was fine.  Which is strange. (But tbf, I'm more a gpu guy
>>>>>> who works on display only when necessary.. ie. a gpu without a display
>>>>>> isn't so much fun ;-))
>>>>>>
>>
>> OpenCL all the way! lol :D
>>
>> On Qualcomm platforms, the first thing that I've ever done was to bring
>> up displays on 8974 Sony platforms... (we're talking about years ago).
>>
>> I'm a lil more on the display side of things (but growing a beard while
>> waiting between a frame and another due to no GPU isn't so much fun
>> either!).
>>
>>>>>>>> Not sure if that is caused by these patches, but if I can figure out
>>>>>>>> how to get the panel back to normal I can bisect.  I think for now
>>>>>>>> I'll drop this series.  Possibly it could be a
>>>>>>>> two-wrongs-makes-a-right situation that had things working before, but
>>>>>>>> I think someone from qcom who knows the DSI IP should take a look.
>>>>>>>>
>>>>>>>
>>>>>>> I would be happy if someone from Qualcomm takes a look: after all, there
>>>>>>> is no documentation and they're the only ones that can verify this kind
>>>>>>> of stuff. Please, Qualcomm.
>>>>>>
>>>>>> Hopefully someone can take a look.
>>>>>>
>>>>>>> Besides that, if there's anything I can help with to solve this riddle,
>>>>>>> I'm here for you.
>>>>>>
>>>>>> Thanks, like I said I'll try applying the patches one by one and see
>>>>>> if I can narrow down what made the panel go funny, and we can go from
>>>>>> there
>>>>>>
>>>>>> BR,
>>>>>> -R
>>>>>>
>>>>>>> Yours,
>>>>>>> -- Angelo
>>>>>>>
>>>>>>>> BR,
>>>>>>>> -R
>>>>>>>>
>>>>>>>>
>>>>>>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno at somainline.org>
>>>>>>>>> ---
>>>>>>>>>     drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 22 +++++++++-------------
>>>>>>>>>     1 file changed, 9 insertions(+), 13 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
>>>>>>>>> index 8b66e852eb36..5be562dfbf06 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
>>>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
>>>>>>>>> @@ -165,11 +165,7 @@ static void dsi_pll_calc_dec_frac(struct dsi_pll_10nm *pll)
>>>>>>>>>
>>>>>>>>>            pll_freq = pll->vco_current_rate;
>>>>>>>>>
>>>>>>>>> -       if (config->disable_prescaler)
>>>>>>>>> -               divider = fref;
>>>>>>>>> -       else
>>>>>>>>> -               divider = fref * 2;
>>>>>>>>> -
>>>>>>>>> +       divider = fref;
>>>>>>>>>            multiplier = 1 << config->frac_bits;
>>>>>>>>>            dec_multiple = div_u64(pll_freq * multiplier, divider);
>>>>>>>>>            dec = div_u64_rem(dec_multiple, multiplier, &frac);
>>>>>>>>> @@ -266,9 +262,11 @@ static void dsi_pll_ssc_commit(struct dsi_pll_10nm *pll)
>>>>>>>>>
>>>>>>>>>     static void dsi_pll_config_hzindep_reg(struct dsi_pll_10nm *pll)
>>>>>>>>>     {
>>>>>>>>> +       struct dsi_pll_config *config = &pll->pll_configuration;
>>>>>>>>>            void __iomem *base = pll->mmio;
>>>>>>>>> +       u32 val = config->disable_prescaler ? 0x0 : 0x80;
>>>>>>>>>
>>>>>>>>> -       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, 0x80);
>>>>>>>>> +       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, val);
>>>>>>>>>            pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_TWO, 0x03);
>>>>>>>>>            pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_THREE, 0x00);
>>>>>>>>>            pll_write(base + REG_DSI_10nm_PHY_PLL_DSM_DIVIDER, 0x00);
>>>>>>>>> @@ -499,17 +497,15 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct clk_hw *hw,
>>>>>>>>>            frac |= ((pll_read(base + REG_DSI_10nm_PHY_PLL_FRAC_DIV_START_HIGH_1) &
>>>>>>>>>                      0x3) << 16);
>>>>>>>>>
>>>>>>>>> -       /*
>>>>>>>>> -        * TODO:
>>>>>>>>> -        *      1. Assumes prescaler is disabled
>>>>>>>>> -        */
>>>>>>>>>            multiplier = 1 << config->frac_bits;
>>>>>>>>> -       pll_freq = dec * (ref_clk * 2);
>>>>>>>>> -       tmp64 = (ref_clk * 2 * frac);
>>>>>>>>> +       pll_freq = dec * ref_clk;
>>>>>>>>> +       tmp64 = ref_clk * frac;
>>>>>>>>>            pll_freq += div_u64(tmp64, multiplier);
>>>>>>>>> -
>>>>>>>>>            vco_rate = pll_freq;
>>>>>>>>>
>>>>>>>>> +       if (config->disable_prescaler)
>>>>>>>>> +               vco_rate = div_u64(vco_rate, 2);
>>>>>>>>> +
>>>>>>>>>            DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
>>>>>>>>>                pll_10nm->id, (unsigned long)vco_rate, dec, frac);
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> 2.29.2
>>>>>>>>>
>>>>>>>
>>



More information about the dri-devel mailing list