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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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,
-- 
2.55.0