Re: [PATCH] drm/mediatek: mtk_dsi: enable hs clock during pre-enable

On 7/28/26 09:08, AngeloGioacchino Del Regno wrote:
> On 7/27/26 18:34, Adam Thiede wrote:
>> On 7/27/26 10:54, AngeloGioacchino Del Regno wrote:
>>> On 7/27/26 16:23, Thorsten Leemhuis wrote:
>>>> On 7/27/26 14:48, Gary Bisson wrote:
>>>>> On Mon, Jul 27, 2026 at 02:28:56PM +0200, Thorsten Leemhuis wrote:
>>>>>> [top-posting to facilitate]
>>>>>>
>>>>>> Gary, Angelo, what's the status here? It looks like this fell through
>>>>>> the cracks -- or was this issue fixed in between somehow? If yes: 
>>>>>> great!
>>>>>> If not: Would be good to finally resolve this, as we are long past 
>>>>>> "fix
>>>>>> within a week" rule of thumb from Linus:
>>>>>> https://www.kernel.org/doc/html/latest/process/handling- 
>>>>>> regressions.html#on- how-quickly-regressions-should-be-fixed
>>>>>
>>>>> Not much happened I'm afraid. The status as I see is:
>>>>> - this patch is necessary to have TI SN65DSI83 working
>>>>> - this patch also follows the DRM guidelines saying that HS clock must
>>>>>    be enabled during pre_enable (see previous answer / [1])
>>>>> - this patch has been successfully tested against MIPI-DSI panels (by
>>>>>    Angelo) and LVDS panels via TI bridge (by myself)
>>>>> - Adam reported an issue with another bridge (PS8640) where resume is
>>>>>    broken
>>>>> - Esben offered a patch to the TI bridge that would fix the issue we
>>>>>    were seeing in the first place.
>>>>>
>>>>> But for the last two points, I'm not sure this calls for a revert yet:
>>>>> - enabling HS clock in pre_enable still is what should be done [1]
>>>>> - maybe the issue Adam is facing is due to the bridge driver 
>>>>> instead as
>>>>>    it could not be reproduced with another setup
>>>>> - Esben patch would break the SN65DSI83 init sequence, suggesting that
>>>>>    the culprit really is the MIPI bridge for not following the HS 
>>>>> clock
>>>>>    requirement in pre_enable instead [2]
>>>> It's tricky, yes, but I suspect the right time for a revert was weeks
>>>> ago. What Linus afaics basically wants in situations like this (espe.
>>>> with two reporters) boils down to "something broke recently, no fix was
>>>> found within a week or maybe two, so we go back to the previous 
>>>> state to
>>>> restart from there -- and this is nothing bad, that's just done to buy
>>>> us time."
>>>>
>>>> If something is right by some hardware or DRM specs often doesn't 
>>>> matter
>>>> much. What makes this tricky is the fact that the revert might cause a
>>>> regression for those that relied on the functionally the change 
>>>> brought.
>>>> But I suspect even then Linus prefers a revert, as it's a recent change
>>>> that only went into 7.1. The quoted from Linus on this page cover 
>>>> this iirc:
>>>> https://www.kernel.org/doc/html/latest/process/handling- 
>>>> regressions.html#quotes-from-linus-about-regression
>>>>
>>>> If we can't agree on this, we maybe should ask Simona or Airlied for 
>>>> advice.
>>>>
>>>> Ciao, Thorsten
>>>
>>> Thorsten,
>>> I agree with you, and it's fine for me to get this commit reverted, 
>>> for the sake
>>> of following the rule (which is something I agree about: as a user, 
>>> the last thing
>>> that should happen is to be afraid of what breaks if I update my 
>>> system!!!!),
>>> even though reverting this actually breaks anything that is not PS8640.
>>>
>>> ......though......
>>>
>>> Everyone, please, look at how fundamentally broken PS8640 is... 
>>> Qualcomm people had
>>> the same issue as what is currently happening here.
>>>
>>> They had to fix their DSI driver, as it was kind-of broken in a 
>>> vagualy similar way
>>> compared to MediaTek - and then they found out that they needed (and 
>>> upstreamed) a
>>> *hack* that had to be done specifically for the PS8640 Bridge [1]: 
>>> their issue was
>>> never really resolved, as then the same bridge started casually 
>>> working for unknown
>>> reasons (is it still working?), so they removed the hack [2] and 
>>> called it a day.
>>>
>>> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux- 
>>> next.git/commit/ drivers/gpu/drm/msm/dsi/dsi_manager.c? 
>>> h=next-20260726&id=ec7981e6c614254937b37ce0af9eac09901c05c5
>>> [2]: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux- 
>>> next.git/commit/ drivers/gpu/drm/msm/dsi/dsi_manager.c? 
>>> h=next-20260726&id=9e15123eca7942caa8a3e1f58ec0df7d088df149
>>>
>>> Especially in [2], the assmption from Dmitry sadly isn't correct, 
>>> because this is
>>> happening now on MediaTek, and here Runtime PM makes sure that the 
>>> PS8640 is also
>>> being enabled "a wee bit" earlier than DSI... but that doesn't seem 
>>> to be enough.
>>>
>>>
>>> Now, here I'm kind of shotgunning at the driver, but I wonder if 
>>> anyone can try
>>> the following patch before reverting this commit in mtk_dsi? Adam?
>>>
>>> ---
>>>   drivers/gpu/drm/bridge/parade-ps8640.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/ 
>>> drm/ bridge/ parade-ps8640.c
>>> index 96332721cb69..54ebf2fc8d1c 100644
>>> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
>>> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
>>> @@ -665,6 +665,7 @@ static int ps8640_probe(struct i2c_client *client)
>>>
>>>       ps_bridge->bridge.of_node = dev->of_node;
>>>       ps_bridge->bridge.type = DRM_MODE_CONNECTOR_eDP;
>>> +    ps_bridge->bridge.pre_enable_prev_first = true;
>>>
>>>       /*
>>>        * Get MIPI DSI resources early. These can return -EPROBE_DEFER so
>> This does not fix the issue. I built 7.1.5 with this patch applied, 
>> and without the revert.
> 
> I think I know what's going on.
> 
> This is just mediatek-drm calling the ddp_start() callback, which is 
> needed to
> start enabling the DSI, but putting the lane_ready(), hs_mode() calls in 
> the
> mtk_dsi_poweron() function is wrong, because that's enabling the HS 
> clock way
> before DRM bridge callbacks are executed.
> 
> I said multiple times that I wanted to remove that ddp_start() and 
> ddp_stop()
> nonsense from mediatek-drm, and I think if what I think is happening is 
> what
> is actually happening, I have one more reason to clean up that .. stuff 
> (more
> than what I've just done with my more-than-50-commits of cleanup).
> 
> Adam, one more test please. That's not shotgunning, and I'm fairly 
> confident
> that this is going to work.
> 
> ---
>   drivers/gpu/drm/mediatek/mtk_dsi.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/ 
> mediatek/mtk_dsi.c
> index dc0c4e867179..c729a163bda6 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -1165,8 +1165,6 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>           goto err_disable_dsi_and_digital_clk;
> 
>       mtk_dsi_set_interrupt_enable(dsi);
> -    mtk_dsi_lane_ready(dsi);
> -    mtk_dsi_clk_hs_mode(dsi, 1);
> 
>       return 0;
> 
> @@ -1284,6 +1282,9 @@ static void 
> mtk_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>       ret = mtk_dsi_poweron(dsi);
>       if (ret < 0)
>           drm_err(drm, "failed to power on dsi\n");
> +
> +    mtk_dsi_lane_ready(dsi);
> +    mtk_dsi_clk_hs_mode(dsi, 1);
>   }
> 
>   static void mtk_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
This works - screen goes off and comes back on. Thank you!