Message-ID: <20250526142808.GR17743@pendragon.ideasonboard.com>
Date: Mon, 26 May 2025 16:28:08 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Tommaso Merciai <tommaso.merciai.xr@...renesas.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
	Maxime Ripard <mripard@...nel.org>, tomm.merciai@...il.com,
	linux-renesas-soc@...r.kernel.org, biju.das.jz@...renesas.com,
	Andrzej Hajda <andrzej.hajda@...el.com>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Robert Foss <rfoss@...nel.org>, Jonas Karlman <jonas@...boo.se>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Douglas Anderson <dianders@...omium.org>,
	Adam Ford <aford173@...il.com>,
	Jesse Van Gavere <jesseevg@...il.com>,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/bridge: adv7511: Do not merge adv7511_mode_set()
 with atomic_enable()

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@...renesas.com>
> >>>>>>>> Closes: https://lore.kernel.org/all/aDB8bD6cF7qiSpKd@tom-desktop/
> >>>>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@...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@...rc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI- 
> >>>>>> A-1:800x600-56.25@...4
> >>>>>> 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@...rc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI- 
> >>>>>> A-1:800x600-56.25@...4
> >>>>>> 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.

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists