[PATCH] drm/panel/panel-sitronix-st7701: Move init sequence from prepare() to enable()

[Mimoja]

On 29.08.23 16:35, Jagan Teki wrote:
> On Sun, Aug 27, 2023 at 12:03 AM Mimoja <mimoja at mimoja.de> wrote:
>> I appreciate you taking the time to respond!
>>
>> On 26.08.23 17:18, Marek Vasut wrote:
>>> On 8/26/23 11:55, Mimoja wrote:
>>>> "The .prepare() function is typically called before the display
>>>> controller
>>>> starts to transmit video data."
>>>> and
>>>> "After the display controller has started transmitting video data,
>>>> it's safe
>>>>    to call the .enable() function."
>>> DSI commands are not DSI video, so this should be OK ?
>> You are correct, my commit message is mixing things up here. I wanted to
>> emphasize roughly the thought of
>> "when enable() is called the dsi core is expected to have its clock
>> initialized". Will take note to clarify this if I succeed to
>> make a case for this patch below :)
>>
>>>> While generally fine this can lead to a fillup of the transmission
>>>> queue before
>>>> the transmission is set up on certain dsi bridges.
>>>> This issue can also be seen on downstream imx8m* kernels.
>>> Can you reproduce this with current mainline Linux or linux-next tree ?
>>> I recall the display pipeline in the NXP downstream stuff is very
>>> different from mainline .
>> You are very much correct. The NXP downstream kernel is completely
>> different from the upstream one
>> and is really a great example to show the issue (code cleaned up for
>> readability):
>>
>> https://github.com/varigit/linux-imx/blob/5.15-2.0.x-imx_var01/drivers/gpu/drm/bridge/sec-dsim.c#L1368
>> ```
>>       ret = drm_panel_prepare(dsim->panel);
>>       if (unlikely(ret)) [...]
>>
>>       /* config esc clock, byte clock and etc */
>>       sec_mipi_dsim_config_clkctrl(dsim);
>>
>>       ret = drm_panel_enable(dsim->panel);
>>       if (unlikely(ret)) [...]
>>
>> ```
>>
>>> Which SoC does have this problem ?
>> Sadly I don't have any SoCs available which would work perfectly with
>> linux-next, let alone are confirmed affected :/
>>
>> I were able to make my Kingway Panel work (Custom one and so far
>> unsupported by the st7701 driver) with this
>> patch on downstream 5.4 and 5.15 imx8mn as well as on a raspberry pi CM4
>> with 6.1. However raspberrypi/linux brings
>> SPI support to the st7701 driver which should not affect this but I
>> would just like to document it here.
>> I could not find any success story with st7701 and the rpi on 6.1 online
>> after a short search (and only one
>> reference with 5.10 which seems to me a bit different in a short
>> comparison)  but again I can only offer
>> circumstantial evidence. Sorry :/
> If I understand correctly, 5.10 and 5.15 Would work as it is if the
> DSI host calls the panel's prepare and enable directly from encoder
> enable. Did you check that?
>
No, I fear the downstream NXP driver is not following the correct / 
expected calling flow
which upstream has implemented. After investigating this and the RPi 
issue I would like to
follow Marek's recommendation and simply sunset this change in favor of 
fixing the
downstream issues in the downstream tree(s).

This seems to be mainly an issue for vendor-custom downstream host 
kernels, and while there might be
a case of convention where panels don't sent commands in prepare() and 
only in enable()
I don't think this patch should have a future solely to be cherry-picked 
into downstream kernels,
when we can not make a case for current linux/next. After all: It is now 
on the internet for better
or worse for others to pick up if needed.

I appreciate you all taking the time to discuss and I will post the 
enablement code
for the Kingway HW-021P0Z002-01 panel I tested this with in a bit.

Cheers and thankful regards
Mimoja