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

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

[Tommaso Merciai]

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@bp.renesas.com>
Closes: https://lore.kernel.org/all/aDB8bD6cF7qiSpKd@tom-desktop/
Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 22 ++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 8b7548448615..7a874bf645af 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -647,6 +647,7 @@ adv7511_detect(struct adv7511 *adv7511)
 }
 
 static void adv7511_mode_set(struct adv7511 *adv7511,
+			     const struct drm_display_mode *mode,
 			     const struct drm_display_mode *adj_mode)
 {
 	unsigned int low_refresh_rate;
@@ -717,11 +718,11 @@ static void adv7511_mode_set(struct adv7511 *adv7511,
 			vsync_polarity = 1;
 	}
 
-	if (drm_mode_vrefresh(adj_mode) <= 24)
+	if (drm_mode_vrefresh(mode) <= 24)
 		low_refresh_rate = ADV7511_LOW_REFRESH_RATE_24HZ;
-	else if (drm_mode_vrefresh(adj_mode) <= 25)
+	else if (drm_mode_vrefresh(mode) <= 25)
 		low_refresh_rate = ADV7511_LOW_REFRESH_RATE_25HZ;
-	else if (drm_mode_vrefresh(adj_mode) <= 30)
+	else if (drm_mode_vrefresh(mode) <= 30)
 		low_refresh_rate = ADV7511_LOW_REFRESH_RATE_30HZ;
 	else
 		low_refresh_rate = ADV7511_LOW_REFRESH_RATE_NONE;
@@ -743,7 +744,7 @@ static void adv7511_mode_set(struct adv7511 *adv7511,
 	 * supposed to give better results.
 	 */
 
-	adv7511->f_tmds = adj_mode->clock;
+	adv7511->f_tmds = mode->clock;
 }
 
 static int adv7511_connector_init(struct adv7511 *adv)
@@ -795,8 +796,6 @@ static void adv7511_bridge_atomic_enable(struct drm_bridge *bridge,
 
 	adv7511_set_config_csc(adv, connector, adv->rgb);
 
-	adv7511_mode_set(adv, &crtc_state->adjusted_mode);
-
 	drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);
 }
 
@@ -821,6 +820,16 @@ adv7511_bridge_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
+static void adv7511_bridge_mode_set(struct drm_bridge *bridge,
+				    const struct drm_display_mode *mode,
+				    const struct drm_display_mode *adj_mode)
+{
+	struct adv7511 *adv = bridge_to_adv7511(bridge);
+
+	/* should not be merged into atomic_enable() */
+	adv7511_mode_set(adv, mode, adj_mode);
+}
+
 static enum drm_mode_status adv7511_bridge_mode_valid(struct drm_bridge *bridge,
 						      const struct drm_display_info *info,
 		const struct drm_display_mode *mode)
@@ -917,6 +926,7 @@ static int adv7511_bridge_hdmi_write_infoframe(struct drm_bridge *bridge,
 }
 
 static const struct drm_bridge_funcs adv7511_bridge_funcs = {
+	.mode_set = adv7511_bridge_mode_set,
 	.mode_valid = adv7511_bridge_mode_valid,
 	.attach = adv7511_bridge_attach,
 	.detect = adv7511_bridge_detect,
-- 
2.43.0

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

[Geert Uytterhoeven]

Hi Tommaso,

On Mon, 26 May 2025 at 10:55, Tommaso Merciai
<tommaso.merciai.xr@bp.renesas.com> 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.

Thanks for your patch!

> Fixes: 0a9e2f0a6466 ("drm/bridge: adv7511: switch to the HDMI connector helpers")

I can't find that commit? I guess you mean:
Fixes: ae01d3183d2763ed ("drm/bridge: adv7511: switch to the HDMI
connector helpers")

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Tommaso Merciai]

Hi Geert,
Thanks for your comment.

On 26/05/25 11:13, Geert Uytterhoeven wrote:
> Hi Tommaso,
> 
> On Mon, 26 May 2025 at 10:55, Tommaso Merciai
> <tommaso.merciai.xr@bp.renesas.com> 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.
> 
> Thanks for your patch!
> 
>> Fixes: 0a9e2f0a6466 ("drm/bridge: adv7511: switch to the HDMI connector helpers")
> 
> I can't find that commit? I guess you mean:
> Fixes: ae01d3183d2763ed ("drm/bridge: adv7511: switch to the HDMI
> connector helpers")

Yes, sorry.
You are completely right!

https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/bridge/adv7511?id=ae01d3183d2763ed27ab71f4ef5402b683d9ad8a

> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

Thanks & Regards,
Tommaso

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

[Biju Das]

Hi Tommaso,

Thanks for the patch.

> -----Original Message-----
> From: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> Sent: 26 May 2025 09:55
> Subject: [PATCH] drm/bridge: adv7511: Do not merge adv7511_mode_set() with atomic_enable()
> 
> 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@bp.renesas.com>
> Closes: https://lore.kernel.org/all/aDB8bD6cF7qiSpKd@tom-desktop/
> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>

Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

The modetest started working again on RZ/V2L SMARC EVK connected to ADV7535.

So,

Tested-by: Biju Das <biju.das.jz@bp.renesas.com>

Cheers,
Biju


> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 22 ++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 8b7548448615..7a874bf645af 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -647,6 +647,7 @@ adv7511_detect(struct adv7511 *adv7511)  }
> 
>  static void adv7511_mode_set(struct adv7511 *adv7511,
> +			     const struct drm_display_mode *mode,
>  			     const struct drm_display_mode *adj_mode)  {
>  	unsigned int low_refresh_rate;
> @@ -717,11 +718,11 @@ static void adv7511_mode_set(struct adv7511 *adv7511,
>  			vsync_polarity = 1;
>  	}
> 
> -	if (drm_mode_vrefresh(adj_mode) <= 24)
> +	if (drm_mode_vrefresh(mode) <= 24)
>  		low_refresh_rate = ADV7511_LOW_REFRESH_RATE_24HZ;
> -	else if (drm_mode_vrefresh(adj_mode) <= 25)
> +	else if (drm_mode_vrefresh(mode) <= 25)
>  		low_refresh_rate = ADV7511_LOW_REFRESH_RATE_25HZ;
> -	else if (drm_mode_vrefresh(adj_mode) <= 30)
> +	else if (drm_mode_vrefresh(mode) <= 30)
>  		low_refresh_rate = ADV7511_LOW_REFRESH_RATE_30HZ;
>  	else
>  		low_refresh_rate = ADV7511_LOW_REFRESH_RATE_NONE; @@ -743,7 +744,7 @@ static void
> adv7511_mode_set(struct adv7511 *adv7511,
>  	 * supposed to give better results.
>  	 */
> 
> -	adv7511->f_tmds = adj_mode->clock;
> +	adv7511->f_tmds = mode->clock;
>  }
> 
>  static int adv7511_connector_init(struct adv7511 *adv) @@ -795,8 +796,6 @@ static void
> adv7511_bridge_atomic_enable(struct drm_bridge *bridge,
> 
>  	adv7511_set_config_csc(adv, connector, adv->rgb);
> 
> -	adv7511_mode_set(adv, &crtc_state->adjusted_mode);
> -
>  	drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);  }
> 
> @@ -821,6 +820,16 @@ adv7511_bridge_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
>  	return MODE_OK;
>  }
> 
> +static void adv7511_bridge_mode_set(struct drm_bridge *bridge,
> +				    const struct drm_display_mode *mode,
> +				    const struct drm_display_mode *adj_mode) {
> +	struct adv7511 *adv = bridge_to_adv7511(bridge);
> +
> +	/* should not be merged into atomic_enable() */
> +	adv7511_mode_set(adv, mode, adj_mode); }
> +
>  static enum drm_mode_status adv7511_bridge_mode_valid(struct drm_bridge *bridge,
>  						      const struct drm_display_info *info,
>  		const struct drm_display_mode *mode)
> @@ -917,6 +926,7 @@ static int adv7511_bridge_hdmi_write_infoframe(struct drm_bridge *bridge,  }
> 
>  static const struct drm_bridge_funcs adv7511_bridge_funcs = {
> +	.mode_set = adv7511_bridge_mode_set,
>  	.mode_valid = adv7511_bridge_mode_valid,
>  	.attach = adv7511_bridge_attach,
>  	.detect = adv7511_bridge_detect,
> --
> 2.43.0

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 867 bytes --]

Hi,

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@bp.renesas.com>
> Closes: https://lore.kernel.org/all/aDB8bD6cF7qiSpKd@tom-desktop/
> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@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.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Tommaso Merciai]

Hi Maxime,
Thanks for your comment.

On 26/05/25 11:26, Maxime Ripard wrote:
> Hi,
> 
> 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@bp.renesas.com>
>> Closes: https://lore.kernel.org/all/aDB8bD6cF7qiSpKd@tom-desktop/
>> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@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@smarc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI-A-1:800x600-56.25@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@smarc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI-A-1:800x600-56.25@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 do you think? Thanks in advance.

Thanks & Regards,
Tommaso

> 
> Maxime

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

[Laurent Pinchart]

On Mon, May 26, 2025 at 11:58:37AM +0200, Tommaso Merciai wrote:
> Hi Maxime,
> Thanks for your comment.
> 
> On 26/05/25 11:26, Maxime Ripard wrote:
> > Hi,
> > 
> > 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@bp.renesas.com>
> >> Closes: https://lore.kernel.org/all/aDB8bD6cF7qiSpKd@tom-desktop/
> >> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@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@smarc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI-A-1:800x600-56.25@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@smarc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI-A-1:800x600-56.25@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.

> 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.

-- 
Regards,

Laurent Pinchart

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

[Tommaso Merciai]

Hi Laurent,
Thanks for your comment.

On 26/05/25 12:49, Laurent Pinchart wrote:
> On Mon, May 26, 2025 at 11:58:37AM +0200, Tommaso Merciai wrote:
>> Hi Maxime,
>> Thanks for your comment.
>>
>> On 26/05/25 11:26, Maxime Ripard wrote:
>>> Hi,
>>>
>>> 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@bp.renesas.com>
>>>> Closes: https://lore.kernel.org/all/aDB8bD6cF7qiSpKd@tom-desktop/
>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@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@smarc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI-A-1:800x600-56.25@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@smarc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI-A-1:800x600-56.25@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.

Thanks & Regards,
Tommaso

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3450 bytes --]

On Mon, May 26, 2025 at 01:19:23PM +0200, Tommaso Merciai wrote:
> Hi Laurent,
> Thanks for your comment.
> 
> On 26/05/25 12:49, Laurent Pinchart wrote:
> > On Mon, May 26, 2025 at 11:58:37AM +0200, Tommaso Merciai wrote:
> > > Hi Maxime,
> > > Thanks for your comment.
> > > 
> > > On 26/05/25 11:26, Maxime Ripard wrote:
> > > > Hi,
> > > > 
> > > > 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@bp.renesas.com>
> > > > > Closes: https://lore.kernel.org/all/aDB8bD6cF7qiSpKd@tom-desktop/
> > > > > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@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@smarc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI-A-1:800x600-56.25@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@smarc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI-A-1:800x600-56.25@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.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Dmitry Baryshkov]

On 26/05/2025 14:40, Maxime Ripard wrote:
> On Mon, May 26, 2025 at 01:19:23PM +0200, Tommaso Merciai wrote:
>> Hi Laurent,
>> Thanks for your comment.
>>
>> On 26/05/25 12:49, Laurent Pinchart wrote:
>>> On Mon, May 26, 2025 at 11:58:37AM +0200, Tommaso Merciai wrote:
>>>> Hi Maxime,
>>>> Thanks for your comment.
>>>>
>>>> On 26/05/25 11:26, Maxime Ripard wrote:
>>>>> Hi,
>>>>>
>>>>> 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@bp.renesas.com>
>>>>>> Closes: https://lore.kernel.org/all/aDB8bD6cF7qiSpKd@tom-desktop/
>>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@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@smarc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI-A-1:800x600-56.25@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@smarc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI-A-1:800x600-56.25@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.

-- 
With best wishes
Dmitry

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

[Tommaso Merciai]

Hi All,
Thanks for your comments.

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:
>>> Hi Laurent,
>>> Thanks for your comment.
>>>
>>> On 26/05/25 12:49, Laurent Pinchart wrote:
>>>> On Mon, May 26, 2025 at 11:58:37AM +0200, Tommaso Merciai wrote:
>>>>> Hi Maxime,
>>>>> Thanks for your comment.
>>>>>
>>>>> On 26/05/25 11:26, Maxime Ripard wrote:
>>>>>> Hi,
>>>>>>
>>>>>> 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@bp.renesas.com>
>>>>>>> Closes: https://lore.kernel.org/all/aDB8bD6cF7qiSpKd@tom-desktop/
>>>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@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@smarc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI- 
>>>>> A-1:800x600-56.25@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@smarc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI- 
>>>>> A-1:800x600-56.25@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.

Unfortunately, since I don't have the ADV7535 datasheet, I believe this 
issue may be related to the functions call sequence.

I agree with Dmitry's theory.

Let me gently know if you need some more test on my side. Thanks in advance.

Regards,
Tommaso

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

[Tommaso Merciai]

On 26/05/25 16:02, Tommaso Merciai wrote:
> Hi All,
> Thanks for your comments.
> 
> 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:
>>>> Hi Laurent,
>>>> Thanks for your comment.
>>>>
>>>> On 26/05/25 12:49, Laurent Pinchart wrote:
>>>>> On Mon, May 26, 2025 at 11:58:37AM +0200, Tommaso Merciai wrote:
>>>>>> Hi Maxime,
>>>>>> Thanks for your comment.
>>>>>>
>>>>>> On 26/05/25 11:26, Maxime Ripard wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> 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@bp.renesas.com>
>>>>>>>> Closes: https://lore.kernel.org/all/aDB8bD6cF7qiSpKd@tom-desktop/
>>>>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@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@smarc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI- 
>>>>>> A-1:800x600-56.25@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@smarc-rzg3e:~# modetest -M rzg2l-du -d -s HDMI- 
>>>>>> A-1:800x600-56.25@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

Thanks & Regards,
Tommaso


> 
> Unfortunately, since I don't have the ADV7535 datasheet, I believe this 
> issue may be related to the functions call sequence.
> 
> I agree with Dmitry's theory.
> 
> Let me gently know if you need some more test on my side. Thanks in 
> advance.
> 
> Regards,
> Tommaso
> 
> 
> 
> 
> 
> 
> 
> 

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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

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

[Tommaso Merciai]

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

Thanks & Regards,
Tommaso

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

[Tommaso Merciai]

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