Re: [PATCH v8 2/3] drm/panel: startek-kd070fhfid015: add another init step
From: AngeloGioacchino Del Regno
Date: Tue Apr 15 2025 - 10:46:50 EST
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?
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).
You're probably getting confused by the bridge en/disable callbacks, btw... please
check include/drm/drm_panel.h, struct drm_panel_funcs.
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.
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@xxxxxxxxxxxx>
---
.../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,