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

[Alexandre Mergnat <amergnat@baylibre.com>]

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