Re: [PATCH v8 2/3] drm/panel: startek-kd070fhfid015: add another init step

[Alexandre Mergnat <amergnat@baylibre.com>]

Hi Angelo,

On 15/04/2025 16:46, AngeloGioacchino Del Regno wrote:
> Il 15/04/25 16:13, Alexandre Mergnat ha scritto:
>> Hi Angelo,
>>
>> Gentle ping
>>
>> Let me shortly summarize my problem: I see the panel driver sending commands to the display before 
>> it is ready. My approach to prevent that is to delay sending commands until bridge enable. Your 
>> concern was that during the panel's .prepare() the panel driver should already be able to send 
>> commands through the bridge. Can you please clarify what you think should be the approach to fix 
>> that?
>>
> 
> Please don't top post.
> 
> Anyway - sorry but I missed your reply, that wasn't intentional - thanks for the
> ping (or I wouldn't have replied, duh!).
> 
> What is not ready? The Startek display or the MediaTek display controller?
> 

MediaTek display controller (DSI)

> The display controller shall be able to send commands when the *panel*'s .prepare()
> callback gets executed - if not, there's something wrong at the display controller
> side (driver).
> 

It's explained at the end.

> You're probably getting confused by the bridge en/disable callbacks, btw... please
> check include/drm/drm_panel.h, struct drm_panel_funcs.
> 

panel_bridge_atomic_pre_enable => drm_panel_prepare => (drm_panel_funcs.prepare) stk_panel_prepare
panel_bridge_atomic_enable => drm_panel_enable => (drm_panel_funcs.enable) stk_panel_enable

The bridge en/disable callbacks call panel and DSI enable/disable callbacks, they are linked.

> In short, the panel's prepare() should be used for whatever setup is required by
> the panel to become available to *receive the video transmission* from the display
> controller: this implies that if the panel needs DSI commands for setup, this is
> allowed and it's a perfectly fine case.
> 
> So, if you are unable to "turn the panel on and wait for it to become ready" in
> the panel's .prepare() callback, there's something wrong either in your panel
> driver, on in the display controller (the DSI driver) instead.
> 
> Since this wasn't happening before your mtk_dsi cleanup, this probably means that
> the cleanup is done wrong - and that removing the .start/.stop custom callbacks
> from that driver needs you to do something more than just that in order to avoid
> regressions.
> 

Here the current call order:
[   13.715959] mtk_dsi_ddp_start ( => dsi poweron)
[   13.716797] stk_panel_prepare ( => panel poweron + enable)
[   13.939473] mtk_dsi_bridge_atomic_pre_enable ( => dsi poweron)
[   13.939488] mtk_output_dsi_enable ( => dsi enable)

As you can see, dsi poweron is called twice. According to your comment [1] asking me to remove
custom init function in favor of DRM API call, I've removed "mtk_dsi_ddp_start". Since I don't
find any API to poweron DSI before stk_panel_prepare call, it has been split to do enable
sequence after DSI poweron+enable, because it requiere mipi dsi interface enabled to do
panel enable.

Patched solution:
[   14.164136] stk_panel_prepare ( => panel poweron)
[   14.213300] mtk_dsi_bridge_atomic_pre_enable ( => dsi poweron)
[   14.213623] mtk_output_dsi_enable ( => dsi enable)
[   14.215116] stk_panel_enable ( => panel enable)

The prepare/enable order is fixed by the DRM framework [2]

We still misaligned about the panel's prepare() should be, but even if I try to implement
whatever setup is required by the panel to become available to *receive the video
transmission* from the display controller, the DRM init order doesn't allow it.
I can move stk_panel_prepare content into stk_panel_enable if you prefer, but it's less clean
IMHO because I like to have a first callback for HW/power setup, and a second callback for SW
setup, which fit with bridge callback descriptions.
I don't see a better way to cleanup custom init, and my apologies for your time if I missed something.

[1]: https://lore.kernel.org/all/c2154240-efa1-4c73-aabe-74e938a75af1@collabora.com/
[2]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_bridge.c#n769

> Unfortunately, I'm pretty busy these days, otherwise I would've gladly made some
> research to try and give you some more hints.. but eh :-)
> 
> Cheers,
> Angelo
> 
>> Regards,
>> Alexandre
>>
>> On 21/03/2025 10:19, Alexandre Mergnat wrote:
>>> Hi Angelo,
>>> Thanks for the fast feedback :)
>>>
>>> On 20/03/2025 13:37, AngeloGioacchino Del Regno wrote:
>>>> Il 20/03/25 09:48, Alexandre Mergnat ha scritto:
>>>>> Currently, the panel set power, set gpio and enable the display link
>>>>> in stk_panel_prepare, pointed by drm_panel_funcs.prepare, called by
>>>>> panel_bridge_atomic_pre_enable, pointed by
>>>>> drm_bridge_funcs.atomic_pre_enable. According to the drm_bridge.h,
>>>>> atomic_pre_enable must not enable the display link
>>>>>
>>>>> Since the DSI driver is properly inited by the DRM, the panel try to
>>>>> communicate with the panel before DSI is powered on.
>>>>>
>>>>
>>>> The panel driver shall still be able to send commands in the .prepare() callback
>>>> and if this is not happening anymore... well, there's a problem!
>>>
>>> Sorry I don't think so, according to that def:
>>>      /**
>>>       * @pre_enable:
>>>       *
>>>       * This callback should enable the bridge. It is called right before
>>>       * the preceding element in the display pipe is enabled. If the
>>>       * preceding element is a bridge this means it's called before that
>>>       * bridge's @pre_enable function. If the preceding element is a
>>>       * &drm_encoder it's called right before the encoder's
>>>       * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
>>>       * &drm_encoder_helper_funcs.dpms hook.
>>>       *
>>>       * The display pipe (i.e. clocks and timing signals) feeding this bridge
>>>       * will not yet be running when this callback is called. The bridge must
>>>       * not enable the display link feeding the next bridge in the chain (if
>>>       * there is one) when this callback is called.
>>>       *
>>>       * The @pre_enable callback is optional.
>>>       *
>>>       * NOTE:
>>>       *
>>>       * This is deprecated, do not use!
>>>       * New drivers shall use &drm_bridge_funcs.atomic_pre_enable.
>>>       */
>>>      void (*pre_enable)(struct drm_bridge *bridge);
>>>
>>>      /**
>>>       * @enable:
>>>       *
>>>       * This callback should enable the bridge. It is called right after
>>>       * the preceding element in the display pipe is enabled. If the
>>>       * preceding element is a bridge this means it's called after that
>>>       * bridge's @enable function. If the preceding element is a
>>>       * &drm_encoder it's called right after the encoder's
>>>       * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
>>>       * &drm_encoder_helper_funcs.dpms hook.
>>>       *
>>>       * The bridge can assume that the display pipe (i.e. clocks and timing
>>>       * signals) feeding it is running when this callback is called. This
>>>       * callback must enable the display link feeding the next bridge in the
>>>       * chain if there is one.
>>>       *
>>>       * The @enable callback is optional.
>>>       *
>>>       * NOTE:
>>>       *
>>>       * This is deprecated, do not use!
>>>       * New drivers shall use &drm_bridge_funcs.atomic_enable.
>>>       */
>>>      void (*enable)(struct drm_bridge *bridge);
>>>
>>> => "The bridge must not enable the display link feeding the next bridge in the
>>> => chain (if there is one) when this callback is called."
>>>
>>> Additionally, you ask for something impossible because here is the init order
>>> fixed by the framework:
>>>
>>> [   10.753139] panel_bridge_atomic_pre_enable
>>> [   10.963505] mtk_dsi_bridge_atomic_pre_enable
>>> [   10.963518] mtk_dsi_bridge_atomic_enable
>>> [   10.963527] panel_bridge_atomic_enable
>>> [   10.963532] drm_panel_enable
>>>
>>> If panel want to use the DSI link in panel_bridge_atomic_pre_enable, nothing
>>> will happen and  you will get a timeout.
>>>
>>> So, IMHO, this patch make sense.
>>>
>>>>
>>>>> To solve that, use stk_panel_enable to enable the display link because
>>>>> it's called after the mtk_dsi_bridge_atomic_pre_enable which is power
>>>>> on the DSI.
>>>>>
>>>>> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
>>>>> ---
>>>>>   .../gpu/drm/panel/panel-startek-kd070fhfid015.c    | 25 +++++++++++++---------
>>>>>   1 file changed, 15 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c b/drivers/gpu/ 
>>>>> drm/panel/panel-startek-kd070fhfid015.c
>>>>> index c0c95355b7435..bc3c4038bf4f5 100644
>>>>> --- a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
>>>>> +++ b/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
>>>>> @@ -135,19 +135,9 @@ static int stk_panel_prepare(struct drm_panel *panel)
>>>>>       gpiod_set_value(stk->enable_gpio, 1);
>>>>>       mdelay(20);
>>>>>       gpiod_set_value(stk->reset_gpio, 1);
>>>>> -    mdelay(10);
>>>>> -    ret = stk_panel_init(stk);
>>>>> -    if (ret < 0)
>>>>> -        goto poweroff;
>>>>
>>>> Also, you're moving both init and set_display_on to the enable callback...
>>>> this is suboptimal.
>>>>
>>>> You should do the DrIC setup in .prepare() (can include SLEEP OUT), and then you
>>>> should have a .enable() callback that calls DISP ON, a .disable() callback that
>>>> calls DISP OFF, and .unprepare() that turns everything off.
>>>
>>> This is not what I understand from the pre_enable's definition above, and also
>>> the function call order by the framework. :)
>>>
>>>>
>>>> Cheers,
>>>> Angelo
>>>>
>>>>> -
>>>>> -    ret = stk_panel_on(stk);
>>>>> -    if (ret < 0)
>>>>> -        goto poweroff;
>>>>>       return 0;
>>>>> -poweroff:
>>>>> -    regulator_disable(stk->supplies[POWER].consumer);
>>>>>   iovccoff:
>>>>>       regulator_disable(stk->supplies[IOVCC].consumer);
>>>>>       gpiod_set_value(stk->reset_gpio, 0);
>>>>> @@ -156,6 +146,20 @@ static int stk_panel_prepare(struct drm_panel *panel)
>>>>>       return ret;
>>>>>   }
>>>>> +static int stk_panel_enable(struct drm_panel *panel)
>>>>> +{
>>>>> +    struct stk_panel *stk = to_stk_panel(panel);
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = stk_panel_init(stk);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    ret = stk_panel_on(stk);
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>>   static const struct drm_display_mode default_mode = {
>>>>>           .clock = 163204,
>>>>>           .hdisplay = 1200,
>>>>> @@ -239,6 +243,7 @@ drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi)
>>>>>   }
>>>>>   static const struct drm_panel_funcs stk_panel_funcs = {
>>>>> +    .enable = stk_panel_enable,
>>>>>       .unprepare = stk_panel_unprepare,
>>>>>       .prepare = stk_panel_prepare,
>>>>>       .get_modes = stk_panel_get_modes,
>>>>>
>>>>
>>>>
>>>>
>>>
>>
> 

-- 
Regards,
Alexandre