| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fb72f1f5-dc94-4e33-a20c-d46a081091bc@baylibre.com>
Date: Fri, 21 Mar 2025 10:19:41 +0100
From: Alexandre Mergnat <amergnat@...libre.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
Neil Armstrong <neil.armstrong@...aro.org>,
Jessica Zhang <quic_jesszhan@...cinc.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Chun-Kuang Hu <chunkuang.hu@...nel.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
Matthias Brugger <matthias.bgg@...il.com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
dri-devel@...ts.freedesktop.org, linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v8 2/3] drm/panel: startek-kd070fhfid015: add another init
step
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@...libre.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
Powered by blists - more mailing lists