Re: [PATCH 1/6] drm: bridge: samsung-dsim: Support multi-lane calculations

[Marek Szyprowski]

On 21.04.2023 10:40, Lucas Stach wrote:
> Am Donnerstag, dem 20.04.2023 um 16:51 -0500 schrieb Adam Ford:
>> On Thu, Apr 20, 2023 at 8:43 AM Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote:
>>> Am Donnerstag, dem 20.04.2023 um 08:24 -0500 schrieb Adam Ford:
>>>> On Thu, Apr 20, 2023 at 8:06 AM Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote:
>>>>> Hi Adam,
>>>>>
>>>>> Am Mittwoch, dem 19.04.2023 um 05:47 -0500 schrieb Adam Ford:
>>>>>> On Mon, Apr 17, 2023 at 6:55 AM Adam Ford <aford173@xxxxxxxxx> wrote:
>>>>>>> On Mon, Apr 17, 2023 at 3:43 AM Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote:
>>>>>>>> Hi Adam,
>>>>>>>>
>>>>>>>> Am Samstag, dem 15.04.2023 um 05:40 -0500 schrieb Adam Ford:
>>>>>>>>> If there is more than one lane, the HFP, HBP, and HSA is calculated in
>>>>>>>>> bytes/pixel, then they are divided amongst the different lanes with some
>>>>>>>>> additional overhead. This is necessary to achieve higher resolutions while
>>>>>>>>> keeping the pixel clocks lower as the number of lanes increase.
>>>>>>>>>
>>>>>>>> In the testing I did to come up with my patch "drm: bridge: samsung-
>>>>>>>> dsim: fix blanking packet size calculation" the number of lanes didn't
>>>>>>>> make any difference. My testing might be flawed, as I could only
>>>>>>>> measure the blanking after translation from MIPI DSI to DPI, so I'm
>>>>>>>> interested to know what others did here. How did you validate the
>>>>>>>> blanking with your patch? Would you have a chance to test my patch and
>>>>>>>> see if it works or breaks in your setup?
>>>>>> Lucas,
>>>>>>
>>>>>> I tried your patch instead of mine.  Yours is dependent on the
>>>>>> hs_clock being always set to the burst clock which is configured by
>>>>>> the device tree.  I unrolled a bit of my stuff and replaced it with
>>>>>> yours.  It worked at 1080p, but when I tried a few other resolutions,
>>>>>> they did not work.  I assume it's because the DSI clock is fixed and
>>>>>> not changing based on the pixel clock.  In the version I did, I only
>>>>>> did that math when the lanes were > 1. In your patch, you divide by 8,
>>>>>> and in mine, I fetch the bits-per-pixel (which is 8) and I divide by
>>>>>> that just in case the bpp ever changes from 8.  Overall,  I think our
>>>>>> patches basically do the same thing.
>>>>> The calculations in your and my patch are quite different. I'm not
>>>>> taking into account the number of lanes or the MIPI format. I'm basing
>> I was taking the number of lanes into account in order to calculate
>> the clock rate, since 4-lanes can run slower.
>>
> Ah that makes sense if you aren't running at full clock burst clock
> rate.
>
>>>> I was looking more at the division by 8 and the overhead correction of 6.
>>>> This number 6 also appears in the NXP downstream kernel [1].  I know
>>>> Marek V had some concerns about that.
>>>>
>>> Yea, I don't fully remember the details about the overhead. Need to
>>> page that back in. The division by 8 in my patch is just to get from
>>> the bit to a byte clock, nothing to do with the MIPI format bits per
>>> channel or something like that.
>>>
>>>>> the blanking size purely on the ratio between MIPI DSI byte clock and
>>>>> the DPI interface clock. It's quite counter-intuitive that the host
>>>>> would scale the blanking to the number of lanes automatically, but
>>>>> still require the MIPI packet offset removed, but that's what my
>>>>> measurements showed to produce the correct blanking after translation
>>>>> to DPI by the TC358767 bridge chip.
>>>> How many lanes is your DSI interface using?
>>>>
>>> When I did the measurements to come up with the patch, I varied the
>>> number of lanes between 1 and 4. Different number of lanes didn't make
>>> a difference. In fact trying to compensate for the number of lanes when
>>> calculating the blanking size to program into the controller lead to
>>> wildly wrong blanking on the DPI side of the external bridge.
>>>
>>>>> If you dynamically scale the HS clock, then you would need to input the
>>>>> real used HS clock to the calculation in my patch, instead of the fixed
>>>>> burst mode rate.
>>>> I think what you're saying makes sense.
>>>>
>>>> The code I originally modeled this from was from the NXP downstream
>>>> kernel where they define the calculation as being in words [2]. I am
>>>> not saying the NXP code is perfect, but the NXP code works.  With this
>>>> series, my monitors are able to sync a bunch of different resolutions
>>>> from 1080p down to 640x480 and a bunch in between with various refresh
>>>> rates too. That was the goal of this series.
>>>>
>>>> Instead of just using your patch as-is, I will adapt yours to use the
>>>> scaled clock to see how it behaves and get back to you.
>>>>
>>> Thanks, that would be very much appreciated.
>> Lucas,
>>
>> I took your patch and added a dsi state variable named "hs_clock"  to
>> keep track of the output of samsung_dsim_set_pll which should be the
>> active high-speed clock.
>>
>> I then replaced one line in your code to reference the hs_clock
>> instead of the burst clock:
>>
>> @@ -960,7 +962,7 @@ static void samsung_dsim_set_display_mode(struct
>> samsung_dsim *dsi)
>>          u32 reg;
>>
>>          if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
>> -               int byte_clk_khz = dsi->burst_clk_rate / 1000 / 8;
>> +               int byte_clk_khz = dsi->hs_clock / 1000 / 8;
>>                  int hfp = (m->hsync_start - m->hdisplay) *
>> byte_clk_khz / m->clock;
>>
>> With that change, your patch works with the rest of my code, and I
>> think it's easier to read, and it doesn't involve recalculating the
>> clock speed each time since it's cached.  If you're OK with that, I'll
>> incorporate your code into V2 of my series, and I'll apply my changes
>> as a subsequent patch.  I hope to be able to send out V2 this weekend.
>>
> That's good to hear! Seems we are converging here. Feel free to pick up
> the patch, that's also easier for me as I don't have to resend with CC
> fixed.
>
>> I would be curious to know frm Marek Szyprowski what the impact is on
>> the Samsung devices, if any.
>>
> Since I messed up the list CC you also couldn't see his reply to my
> patch:
>
> | Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> |
> | Works fine on the Exynos based boards I have in my test farm.

I didn't follow this discussion, I'm a bit busy with other stuff. I've 
just tested this series and patch #3 break Exynos based board. If you 
want me to test anything else (might be a work-in-progress code), just 
let me know by the separate mail.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland