[PATCH] drm/bridge: adv7511: Do not merge adv7511_mode_set() with atomic_enable()

Tue May 27 10:11:49 UTC 2025

Hi All,

On 26/05/25 18:43, Tommaso Merciai wrote:
> Hi All,
> 
> On 26/05/25 16:28, Laurent Pinchart wrote:
>> On Mon, May 26, 2025 at 04:13:23PM +0200, Tommaso Merciai wrote:
>>> On 26/05/25 16:02, Tommaso Merciai wrote:
>>>> On 26/05/25 15:18, Dmitry Baryshkov wrote:
>>>>> On 26/05/2025 14:40, Maxime Ripard wrote:
>>>>>> On Mon, May 26, 2025 at 01:19:23PM +0200, Tommaso Merciai wrote:
>>>>>>> On 26/05/25 12:49, Laurent Pinchart wrote:
>>>>>>>> On Mon, May 26, 2025 at 11:58:37AM +0200, Tommaso Merciai wrote:
>>>>>>>>> On 26/05/25 11:26, Maxime Ripard wrote:
>>>>>>>>>> On Mon, May 26, 2025 at 10:54:52AM +0200, Tommaso Merciai wrote:
>>>>>>>>>>> After adv7511_mode_set() was merged into .atomic_enable(), 
>>>>>>>>>>> only the
>>>>>>>>>>> native resolution is working when using modetest.
>>>>>>>>>>>
>>>>>>>>>>> This is caused by incorrect timings: adv7511_mode_set() must 
>>>>>>>>>>> not be
>>>>>>>>>>> merged into .atomic_enable().
>>>>>>>>>>>
>>>>>>>>>>> Move adv7511_mode_set() back to the .mode_set() callback in
>>>>>>>>>>> drm_bridge_funcs to restore correct behavior.
>>>>>>>>>>>
>>>>>>>>>>> Fixes: 0a9e2f0a6466 ("drm/bridge: adv7511: switch to the HDMI
>>>>>>>>>>> connector helpers")
>>>>>>>>>>> Reported-by: Biju Das <biju.das.jz at bp.renesas.com>
>>>>>>>>>>> Closes: https://lore.kernel.org/all/aDB8bD6cF7qiSpKd@tom- 
>>>>>>>>>>> desktop/
>>>>>>>>>>> Signed-off-by: Tommaso Merciai 
>>>>>>>>>>> <tommaso.merciai.xr at bp.renesas.com>
>>>>>>>>>>
>>>>>>>>>> Explaining why, both in the commit log and the comments, would be
>>>>>>>>>> nice.
>>>>>>>>>> Because I can't think of any good reason it just can't work 
>>>>>>>>>> for that
>>>>>>>>>> bridge.
>>>>>>>>>
>>>>>>>>> Sorry, let me clarify and share with you some details:
>>>>>>>>>
>>>>>>>>> adv7511_mode_set:
>>>>>>>>>      - Is setting up timings registers for the DSI2HDMI bridge in
>>>>>>>>> our case
>>>>>>>>>        we are using ADV7535 bridge.
>>>>>>>>>
>>>>>>>>> rzg2l_mipi_dsi_atomic_enable:
>>>>>>>>>      - Is setting up the vclock for the DSI ip
>>>>>>>>>
>>>>>>>>> Testing new/old implementation a bit we found the following:
>>>>>>>>>
>>>>>>>>> root at smarc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI-
>>>>>>>>> A-1:800x600-56.25 at XR24
>>>>>>>>> setting mode 800x600-56.25Hz on connectors HDMI-A-1, crtc 62
>>>>>>>>> [   49.273134] adv7511_mode_set_old: drm_mode_vrefresh(mode) = 56
>>>>>>>>> [   49.281006] rzg2l_mipi_dsi_atomic_enable: mode->clock: 36000
>>>>>>>>>
>>>>>>>>> root at smarc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI-
>>>>>>>>> A-1:800x600-56.25 at XR24
>>>>>>>>> setting mode 800x600-56.25Hz on connectors HDMI-A-1, crtc 62
>>>>>>>>> [   74.076881] rzg2l_mipi_dsi_atomic_enable: mode->clock: 36000
>>>>>>>>> [   74.092130] adv7511_mode_set: drm_mode_vrefresh(adj_mode) = 56
>>>>>>>>>
>>>>>>>>> Same result but different timing (in function call perspective):
>>>>>>>>>
>>>>>>>>>      - old: adv7511_mode_set() is call before
>>>>>>>>> rzg2l_mipi_dsi_atomic_enable()
>>>>>>>>>      - new: adv7511_mode_set() is call after
>>>>>>>>> rzg2l_mipi_dsi_atomic_enable()
>>>>>>>>
>>>>>>>> What is "old" and "new" here ? Is it before and after Dmitry's
>>>>>>>> patch, or
>>>>>>>> before and after yours ? Please be precise when describing 
>>>>>>>> problems.
>>>>>>>
>>>>>>> Sorry, you are completely right:
>>>>>>>
>>>>>>>    - old --> before Dmitry's patch
>>>>>>>    - new --> after Dmitry's patch
>>>>>>>
>>>>>>>>
>>>>>>>>> What do you think? Thanks in advance.
>>>>>>>>
>>>>>>>> You're only explaining above what the "old" and "new" behaviours 
>>>>>>>> are,
>>>>>>>> and claiming one of them is causing an issue, but you're not
>>>>>>>> explaining
>>>>>>>> *why* it causes an issue. That's what your commit message is
>>>>>>>> expected to
>>>>>>>> detail.
>>>>>>>>
>>>>>>>
>>>>>>> Thanks for the clarification! :)
>>>>>>> I will send v2 explaining better this.
>>>>>>
>>>>>> In particular, if the driver needs to have mode_set called before
>>>>>> atomic_enable, you should say why moving the call to mode_set 
>>>>>> earlier in
>>>>>> the function wouldn't work.
>>>>>
>>>>> It might be the same thing as we had on PS8640: it had to be brought
>>>>> up before the host starts the DSI link, so that there is no clock
>>>>> input on the DSI clock lane.
>>>>>
>>>>
>>>> Some updates on my side:
>>>>
>>>> I'm not seeing any differences from a regs perspective when using the
>>>> old driver version (before Dmitry's patch) and the new driver version
>>>> (after Dmitry's patch).
>>>>
>>>> In particular, i2cdump -f -y 7 0x4c shows me the same result.
>>>
>>> Please ignore this (wrong address)
>>>
>>> The right test is: i2cdump -f -y 7 0x3d
>>>
>>> And I'm seeing the following differences:
>>>
>>> # WORK:
>>> reg | val
>>> 0x3d → 0x00
>>> 0x3e → 0x00
>>>
>>> # DON't WORK
>>> reg | val
>>> 0x3d → 0x10
>>> 0x3e → 0x40
>>>
>>>> Unfortunately, since I don't have the ADV7535 datasheet, I believe this
>>>> issue may be related to the functions call sequence.
>>
>> You could try to get the documentation from Analog Devices.
>>
>> This being said, the above registers are documented in the ADV7511
>> programming guide, which is publicly available. They may differ in the
>> ADV7535 though.
>>
>>>> I agree with Dmitry's theory.
>>>>
>>>> Let me gently know if you need some more test on my side. Thanks in
>>>> advance.
>>
> 
> FYI, I've tested the following pipeline:
> 
> DU1 (RGB Output) -> adv7513 -> HDMI Panel
> 
> All working fine on my side with the Dmitry's patch.
> Same driver, But broken on DSI interface(ADV7535)

Updating horizontal/vertical porch params after drm_mode_copy() into the 
adv7511_mode_set() fixes the issue.

In particular:

adv7511_bridge_atomic_enable() call adv7511_power_on(), then 
adv7511_dsi_config_timing_gen() that is responsible to update h/v porch 
params.

But during the adv7511_mode_set() adv7511->curr_mode change and this is 
not reflected into the h/v porch regs, then h/w porch regs are keeping 
the old values.

I will send fix in next version.

Thanks & Regards,
Tommaso

> 
> Thanks & Regards,
> Tommaso
> 
> 