public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v3 0/2] drm/bridge: it66121: Fix display output on DVI monitors
@ 2026-05-12 13:22 Javier Martinez Canillas
  2026-05-12 13:22 ` [PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback Javier Martinez Canillas
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Javier Martinez Canillas @ 2026-05-12 13:22 UTC (permalink / raw)
  To: linux-kernel
  Cc: Javier Martinez Canillas, Andrzej Hajda, David Airlie,
	Jernej Skrabec, Jonas Karlman, Laurent Pinchart, Luca Ceresoli,
	Maarten Lankhorst, Maxime Ripard, Neil Armstrong, Phong LE,
	Robert Foss, Simona Vetter, Thomas Zimmermann, dri-devel

Display output does not work when connecting a AM625 BeaglePlay board to a
DVI monitor, because the it66121 bridge driver assumes that the sink type
is always HDMI. This patch series fixes the issue.

Patch #1 moves the transmission mode and AVI infoframes enablement to the
.atomic_enable handler, because currently this logic is in the .mode_set
handler but that is called before .atomic_enable that is where the query
of the connector is done (that contains the display sink type).

Patch #2 then queries the display information to determine whether HDMI
or DVI mode should be set.

This is a v3 of the series, that addresses issues pointed out by Maxime.

Changes in v3:
- Move the HDMI/DVI mode set to the .atomic_enable handler (Maxime Ripard).

Changes in v2:
- Don't store the sink type in a per-commit bridge state (Maxime Ripard).

Javier Martinez Canillas (2):
  drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback
  drm/bridge: ite-it66121: Select HDMI or DVI mode based on sink type

 drivers/gpu/drm/bridge/ite-it66121.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

-- 
2.54.0

base-commit: 19d584a634fe999786acfb0ac5289710cc84a5f6
branch: it66121-fix-dvi-mode-v3


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback
  2026-05-12 13:22 [PATCH v3 0/2] drm/bridge: it66121: Fix display output on DVI monitors Javier Martinez Canillas
@ 2026-05-12 13:22 ` Javier Martinez Canillas
  2026-05-12 14:07   ` Maxime Ripard
  2026-05-16  3:19   ` Claude review: " Claude Code Review Bot
  2026-05-12 13:22 ` [PATCH v3 2/2] drm/bridge: ite-it66121: Select HDMI or DVI mode based on sink type Javier Martinez Canillas
  2026-05-16  3:19 ` Claude review: drm/bridge: it66121: Fix display output on DVI monitors Claude Code Review Bot
  2 siblings, 2 replies; 10+ messages in thread
From: Javier Martinez Canillas @ 2026-05-12 13:22 UTC (permalink / raw)
  To: linux-kernel
  Cc: Javier Martinez Canillas, Andrzej Hajda, David Airlie,
	Jernej Skrabec, Jonas Karlman, Laurent Pinchart, Luca Ceresoli,
	Maarten Lankhorst, Maxime Ripard, Neil Armstrong, Phong LE,
	Robert Foss, Simona Vetter, Thomas Zimmermann, dri-devel

The HDMI transmission mode set and AVI infoframes enable are done in the
.mode_set callback, but it is more correct to do this in .atomic_enable.

Because the information about the sink type is in the struct drm_connector
display_info.is_hdmi and this might not be available when the .mode_set
callback is executed.

Currently the driver is not checking display info to determine whether the
mode has to be set to HDMI or DVI, but this is a bug that will be fixed by
a follow-up change.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

(no changes since v1)

 drivers/gpu/drm/bridge/ite-it66121.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 19a027d75b61..648ca50712df 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -669,6 +669,22 @@ static int it66121_set_mute(struct it66121_ctx *ctx, bool mute)
 			    IT66121_PKT_GEN_CTRL_ON | IT66121_PKT_GEN_CTRL_RPT);
 }
 
+static void it66121_set_tx_mode(struct it66121_ctx *ctx)
+{
+	mutex_lock(&ctx->lock);
+
+	/* Enable AVI infoframe */
+	if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
+			 IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT))
+		goto unlock;
+
+	/* Set TX mode to HDMI */
+	regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI);
+
+unlock:
+	mutex_unlock(&ctx->lock);
+}
+
 #define MAX_OUTPUT_SEL_FORMATS	1
 
 static u32 *it66121_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
@@ -729,6 +745,8 @@ static void it66121_bridge_enable(struct drm_bridge *bridge,
 	ctx->connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
 
 	it66121_set_mute(ctx, false);
+
+	it66121_set_tx_mode(ctx);
 }
 
 static void it66121_bridge_disable(struct drm_bridge *bridge,
-- 
2.54.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 2/2] drm/bridge: ite-it66121: Select HDMI or DVI mode based on sink type
  2026-05-12 13:22 [PATCH v3 0/2] drm/bridge: it66121: Fix display output on DVI monitors Javier Martinez Canillas
  2026-05-12 13:22 ` [PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback Javier Martinez Canillas
@ 2026-05-12 13:22 ` Javier Martinez Canillas
  2026-05-16  3:19   ` Claude review: " Claude Code Review Bot
  2026-05-16  3:19 ` Claude review: drm/bridge: it66121: Fix display output on DVI monitors Claude Code Review Bot
  2 siblings, 1 reply; 10+ messages in thread
From: Javier Martinez Canillas @ 2026-05-12 13:22 UTC (permalink / raw)
  To: linux-kernel
  Cc: Javier Martinez Canillas, Andrzej Hajda, David Airlie,
	Jernej Skrabec, Jonas Karlman, Laurent Pinchart, Luca Ceresoli,
	Maarten Lankhorst, Maxime Ripard, Neil Armstrong, Phong LE,
	Robert Foss, Simona Vetter, Thomas Zimmermann, dri-devel

The driver assumes that the sink type is always HDMI and unconditionally
configures the bridge in this mode and enables the transmission of AVI
infoframe packets.

But this cause issues with DVI monitors, that can fail to interpret the
video signal and lead to not having any display output.

Check the connector display information sink type to decide whether DVI or
HDMI mode should be set and if the AVI infoframes packets should be sent.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v3:
- Move the HDMI/DVI mode set to the .atomic_enable handler (Maxime Ripard).

Changes in v2:
- Don't store the sink type in a per-commit bridge state (Maxime Ripard).

 drivers/gpu/drm/bridge/ite-it66121.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 648ca50712df..94fd513481b5 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -182,6 +182,7 @@
 
 #define IT66121_HDMI_MODE_REG			0xC0
 #define IT66121_HDMI_MODE_HDMI			BIT(0)
+#define IT66121_HDMI_MODE_DVI			0
 
 #define IT66121_SYS_STATUS_REG			0x0E
 #define IT66121_SYS_STATUS_ACTIVE_IRQ		BIT(7)
@@ -671,15 +672,23 @@ static int it66121_set_mute(struct it66121_ctx *ctx, bool mute)
 
 static void it66121_set_tx_mode(struct it66121_ctx *ctx)
 {
+	unsigned int avi_pkt = 0;
+	unsigned int mode = IT66121_HDMI_MODE_DVI;
+	struct drm_connector *connector = ctx->connector;
+
+	if (connector->display_info.is_hdmi) {
+		mode = IT66121_HDMI_MODE_HDMI;
+		avi_pkt = IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT;
+	}
+
 	mutex_lock(&ctx->lock);
 
-	/* Enable AVI infoframe */
-	if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
-			 IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT))
+	/* Enable or disable AVI infoframe */
+	if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG, avi_pkt))
 		goto unlock;
 
-	/* Set TX mode to HDMI */
-	regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI);
+	/* Set TX mode to DVI or HDMI */
+	regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, mode);
 
 unlock:
 	mutex_unlock(&ctx->lock);
-- 
2.54.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback
  2026-05-12 13:22 ` [PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback Javier Martinez Canillas
@ 2026-05-12 14:07   ` Maxime Ripard
  2026-05-12 18:54     ` Javier Martinez Canillas
  2026-05-16  3:19   ` Claude review: " Claude Code Review Bot
  1 sibling, 1 reply; 10+ messages in thread
From: Maxime Ripard @ 2026-05-12 14:07 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: linux-kernel, Andrzej Hajda, David Airlie, Jernej Skrabec,
	Jonas Karlman, Laurent Pinchart, Luca Ceresoli, Maarten Lankhorst,
	Neil Armstrong, Phong LE, Robert Foss, Simona Vetter,
	Thomas Zimmermann, dri-devel

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

Hi,

On Tue, May 12, 2026 at 03:22:15PM +0200, Javier Martinez Canillas wrote:
> The HDMI transmission mode set and AVI infoframes enable are done in the
> .mode_set callback, but it is more correct to do this in .atomic_enable.
> 
> Because the information about the sink type is in the struct drm_connector
> display_info.is_hdmi and this might not be available when the .mode_set
> callback is executed.
> 
> Currently the driver is not checking display info to determine whether the
> mode has to be set to HDMI or DVI, but this is a bug that will be fixed by
> a follow-up change.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/bridge/ite-it66121.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 19a027d75b61..648ca50712df 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -669,6 +669,22 @@ static int it66121_set_mute(struct it66121_ctx *ctx, bool mute)
>  			    IT66121_PKT_GEN_CTRL_ON | IT66121_PKT_GEN_CTRL_RPT);
>  }
>  
> +static void it66121_set_tx_mode(struct it66121_ctx *ctx)
> +{
> +	mutex_lock(&ctx->lock);
> +
> +	/* Enable AVI infoframe */
> +	if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
> +			 IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT))
> +		goto unlock;
> +
> +	/* Set TX mode to HDMI */
> +	regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI);
> +
> +unlock:
> +	mutex_unlock(&ctx->lock);
> +}
> +
>  #define MAX_OUTPUT_SEL_FORMATS	1
>  
>  static u32 *it66121_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
> @@ -729,6 +745,8 @@ static void it66121_bridge_enable(struct drm_bridge *bridge,
>  	ctx->connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
>  
>  	it66121_set_mute(ctx, false);
> +
> +	it66121_set_tx_mode(ctx);
>  }

Having some part of it in mode_set and some part in enable is still kind
of weird. The best there would be to put everything in enable (and
pre_enable), and drop mode_set entirely.

Maxime

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback
  2026-05-12 14:07   ` Maxime Ripard
@ 2026-05-12 18:54     ` Javier Martinez Canillas
  2026-05-13 11:55       ` Maxime Ripard
  0 siblings, 1 reply; 10+ messages in thread
From: Javier Martinez Canillas @ 2026-05-12 18:54 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: linux-kernel, Andrzej Hajda, David Airlie, Jernej Skrabec,
	Jonas Karlman, Laurent Pinchart, Luca Ceresoli, Maarten Lankhorst,
	Neil Armstrong, Phong LE, Robert Foss, Simona Vetter,
	Thomas Zimmermann, dri-devel

Maxime Ripard <mripard@kernel.org> writes:

Thanks again for your feedback!

> Hi,
>
> On Tue, May 12, 2026 at 03:22:15PM +0200, Javier Martinez Canillas wrote:
>> The HDMI transmission mode set and AVI infoframes enable are done in the
>> .mode_set callback, but it is more correct to do this in .atomic_enable.
>> 
>> Because the information about the sink type is in the struct drm_connector
>> display_info.is_hdmi and this might not be available when the .mode_set
>> callback is executed.
>> 
>> Currently the driver is not checking display info to determine whether the
>> mode has to be set to HDMI or DVI, but this is a bug that will be fixed by
>> a follow-up change.
>> 
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>> 
>> (no changes since v1)
>> 
>>  drivers/gpu/drm/bridge/ite-it66121.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
>> index 19a027d75b61..648ca50712df 100644
>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
>> @@ -669,6 +669,22 @@ static int it66121_set_mute(struct it66121_ctx *ctx, bool mute)
>>  			    IT66121_PKT_GEN_CTRL_ON | IT66121_PKT_GEN_CTRL_RPT);
>>  }
>>  
>> +static void it66121_set_tx_mode(struct it66121_ctx *ctx)
>> +{
>> +	mutex_lock(&ctx->lock);
>> +
>> +	/* Enable AVI infoframe */
>> +	if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
>> +			 IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT))
>> +		goto unlock;
>> +
>> +	/* Set TX mode to HDMI */
>> +	regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI);
>> +
>> +unlock:
>> +	mutex_unlock(&ctx->lock);
>> +}
>> +
>>  #define MAX_OUTPUT_SEL_FORMATS	1
>>  
>>  static u32 *it66121_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>> @@ -729,6 +745,8 @@ static void it66121_bridge_enable(struct drm_bridge *bridge,
>>  	ctx->connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
>>  
>>  	it66121_set_mute(ctx, false);
>> +
>> +	it66121_set_tx_mode(ctx);
>>  }
>
> Having some part of it in mode_set and some part in enable is still kind
> of weird. The best there would be to put everything in enable (and
> pre_enable), and drop mode_set entirely.
>

I agree that this will be the correct approach. I wonder though if could be
acceptable to land the changes in this series as a minimal fix for the DVI
mode issue, and then do further refactoring as a follow-up.

Note that what I'm doing in this version is aligned to what the sii902x
bridge driver also does:

static void sii902x_bridge_atomic_enable(struct drm_bridge *bridge,
					 struct drm_atomic_commit *state)
{
...
	u8 output_mode = SII902X_SYS_CTRL_OUTPUT_DVI;

	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
	if (connector && connector->display_info.is_hdmi)
		output_mode = SII902X_SYS_CTRL_OUTPUT_HDMI;
...
	regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA,
			   SII902X_SYS_CTRL_OUTPUT_MODE, output_mode);
...
}

So that driver would also need the cleanup to get rid of the legacy .mode_set.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback
  2026-05-12 18:54     ` Javier Martinez Canillas
@ 2026-05-13 11:55       ` Maxime Ripard
  2026-05-13 12:38         ` Javier Martinez Canillas
  0 siblings, 1 reply; 10+ messages in thread
From: Maxime Ripard @ 2026-05-13 11:55 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: linux-kernel, Andrzej Hajda, David Airlie, Jernej Skrabec,
	Jonas Karlman, Laurent Pinchart, Luca Ceresoli, Maarten Lankhorst,
	Neil Armstrong, Phong LE, Robert Foss, Simona Vetter,
	Thomas Zimmermann, dri-devel

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

On Tue, May 12, 2026 at 08:54:13PM +0200, Javier Martinez Canillas wrote:
> Maxime Ripard <mripard@kernel.org> writes:
> 
> Thanks again for your feedback!
> 
> > Hi,
> >
> > On Tue, May 12, 2026 at 03:22:15PM +0200, Javier Martinez Canillas wrote:
> >> The HDMI transmission mode set and AVI infoframes enable are done in the
> >> .mode_set callback, but it is more correct to do this in .atomic_enable.
> >> 
> >> Because the information about the sink type is in the struct drm_connector
> >> display_info.is_hdmi and this might not be available when the .mode_set
> >> callback is executed.
> >> 
> >> Currently the driver is not checking display info to determine whether the
> >> mode has to be set to HDMI or DVI, but this is a bug that will be fixed by
> >> a follow-up change.
> >> 
> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> >> ---
> >> 
> >> (no changes since v1)
> >> 
> >>  drivers/gpu/drm/bridge/ite-it66121.c | 18 ++++++++++++++++++
> >>  1 file changed, 18 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> >> index 19a027d75b61..648ca50712df 100644
> >> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> >> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> >> @@ -669,6 +669,22 @@ static int it66121_set_mute(struct it66121_ctx *ctx, bool mute)
> >>  			    IT66121_PKT_GEN_CTRL_ON | IT66121_PKT_GEN_CTRL_RPT);
> >>  }
> >>  
> >> +static void it66121_set_tx_mode(struct it66121_ctx *ctx)
> >> +{
> >> +	mutex_lock(&ctx->lock);
> >> +
> >> +	/* Enable AVI infoframe */
> >> +	if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
> >> +			 IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT))
> >> +		goto unlock;
> >> +
> >> +	/* Set TX mode to HDMI */
> >> +	regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI);
> >> +
> >> +unlock:
> >> +	mutex_unlock(&ctx->lock);
> >> +}
> >> +
> >>  #define MAX_OUTPUT_SEL_FORMATS	1
> >>  
> >>  static u32 *it66121_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
> >> @@ -729,6 +745,8 @@ static void it66121_bridge_enable(struct drm_bridge *bridge,
> >>  	ctx->connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
> >>  
> >>  	it66121_set_mute(ctx, false);
> >> +
> >> +	it66121_set_tx_mode(ctx);
> >>  }
> >
> > Having some part of it in mode_set and some part in enable is still kind
> > of weird. The best there would be to put everything in enable (and
> > pre_enable), and drop mode_set entirely.
> >
> 
> I agree that this will be the correct approach. I wonder though if could be
> acceptable to land the changes in this series as a minimal fix for the DVI
> mode issue, and then do further refactoring as a follow-up.

The thing is that you're now making the driver and its interaction with
the hardware more complex. Like, what would happen if mode_set is now
called after atomic_enable? How does the hardware behavies if you modify
the SII902X_SYS_CTRL_DATA register only after it's been enabled?

Moving everything to atomic_enable keeps things simple and keep the
hardware access ordering the same.

And if you don't plan on sending that to fixes, the fact that the patch
is small is kind of irrelevant anyway.

Maxime

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback
  2026-05-13 11:55       ` Maxime Ripard
@ 2026-05-13 12:38         ` Javier Martinez Canillas
  0 siblings, 0 replies; 10+ messages in thread
From: Javier Martinez Canillas @ 2026-05-13 12:38 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: linux-kernel, Andrzej Hajda, David Airlie, Jernej Skrabec,
	Jonas Karlman, Laurent Pinchart, Luca Ceresoli, Maarten Lankhorst,
	Neil Armstrong, Phong LE, Robert Foss, Simona Vetter,
	Thomas Zimmermann, dri-devel

Maxime Ripard <mripard@kernel.org> writes:

> On Tue, May 12, 2026 at 08:54:13PM +0200, Javier Martinez Canillas wrote:

[...]

>> >
>> > Having some part of it in mode_set and some part in enable is still kind
>> > of weird. The best there would be to put everything in enable (and
>> > pre_enable), and drop mode_set entirely.
>> >
>> 
>> I agree that this will be the correct approach. I wonder though if could be
>> acceptable to land the changes in this series as a minimal fix for the DVI
>> mode issue, and then do further refactoring as a follow-up.
>
> The thing is that you're now making the driver and its interaction with
> the hardware more complex. Like, what would happen if mode_set is now
> called after atomic_enable? How does the hardware behavies if you modify
> the SII902X_SYS_CTRL_DATA register only after it's been enabled?
>

That's a good point.

> Moving everything to atomic_enable keeps things simple and keep the
> hardware access ordering the same.
>

Yes, I agree.

> And if you don't plan on sending that to fixes, the fact that the patch
> is small is kind of irrelevant anyway.
>

That's true too. I'll then prepare a v4 doing it properly then even if may
be a bigger refactoring.

Thanks again for the feedback and comments!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Claude review: drm/bridge: it66121: Fix display output on DVI monitors
  2026-05-12 13:22 [PATCH v3 0/2] drm/bridge: it66121: Fix display output on DVI monitors Javier Martinez Canillas
  2026-05-12 13:22 ` [PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback Javier Martinez Canillas
  2026-05-12 13:22 ` [PATCH v3 2/2] drm/bridge: ite-it66121: Select HDMI or DVI mode based on sink type Javier Martinez Canillas
@ 2026-05-16  3:19 ` Claude Code Review Bot
  2 siblings, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-05-16  3:19 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/bridge: it66121: Fix display output on DVI monitors
Author: Javier Martinez Canillas <javierm@redhat.com>
Patches: 7
Reviewed: 2026-05-16T13:19:45.709854

---

This is a well-motivated 2-patch series fixing a real bug: the it66121 bridge driver unconditionally sets HDMI TX mode and enables AVI infoframes, which breaks display output on DVI monitors. The approach is sound — patch 1 moves the TX mode/infoframe configuration from `.mode_set` to `.atomic_enable` (where `ctx->connector` is available), and patch 2 makes the configuration conditional on `display_info.is_hdmi`.

However, there is a significant issue: **patch 1 does not remove the existing TX mode and AVI infoframe writes from `.mode_set`**. The new `it66121_set_tx_mode()` function added in `.atomic_enable` duplicates the `IT66121_HDMI_MODE_REG` and `IT66121_AVI_INFO_PKT_REG` writes that remain in `it66121_bridge_mode_set()` (lines 796-803 of the current tree). This means both callbacks write these registers, and the `.mode_set` write (which runs first) still unconditionally sets HDMI mode. The fix in patch 2 only changes the new function in `.atomic_enable`, so the `.mode_set` code will still force HDMI mode before `.atomic_enable` overrides it with the correct value. While the final state should be correct (`.atomic_enable` runs after `.mode_set`), this is confusing and fragile — the duplicate writes should be cleaned up.

The series is otherwise clean and correctly addresses the v2 feedback from Maxime.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Claude review: drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback
  2026-05-12 13:22 ` [PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback Javier Martinez Canillas
  2026-05-12 14:07   ` Maxime Ripard
@ 2026-05-16  3:19   ` Claude Code Review Bot
  1 sibling, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-05-16  3:19 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Issue: Duplicate register writes not removed from `.mode_set`**

The patch adds a new `it66121_set_tx_mode()` called from `it66121_bridge_enable()`, but the existing code in `it66121_bridge_mode_set()` still writes the same registers:

```c
/* Enable AVI infoframe */
if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
		 IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT))
	goto unlock;

/* Set TX mode to HDMI */
if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI))
	goto unlock;
```

These lines at the current tree's lines 796-803 are not removed. The diffstat shows `1 file changed, 18 insertions(+)` with zero deletions, confirming the old code remains. The commit message says "it is more correct to do this in .atomic_enable" but doesn't explain why the old writes are kept. These should be removed from `.mode_set` to avoid writing the same registers twice per mode change, and to avoid confusion about which callback is the authoritative one.

**Minor: Error handling style**

```c
if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
		 IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT))
	goto unlock;

regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI);
```

The first `regmap_write` checks for errors and jumps to `unlock`, but the second `regmap_write` result is silently discarded. This is inconsistent — either both should check for errors or neither should (the existing `.mode_set` code checks both). This is a minor nit since the function returns void and there's nothing to do on error except release the lock, but consistency is preferred.

**Ordering observation**: The call to `it66121_set_tx_mode()` is placed *after* `it66121_set_mute(ctx, false)` in `.atomic_enable`. This means the display is unmuted before the TX mode is configured. On a DVI monitor receiving HDMI-mode infoframes during this window, there could be a brief glitch. Consider placing `it66121_set_tx_mode()` before the unmute. This ordering also matters because in the existing `.mode_set`, the TX mode/infoframe setup is done *before* the AFE configuration and *before* unmute — moving it after unmute changes the sequencing.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Claude review: drm/bridge: ite-it66121: Select HDMI or DVI mode based on sink type
  2026-05-12 13:22 ` [PATCH v3 2/2] drm/bridge: ite-it66121: Select HDMI or DVI mode based on sink type Javier Martinez Canillas
@ 2026-05-16  3:19   ` Claude Code Review Bot
  0 siblings, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-05-16  3:19 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness: The core logic is correct**

The approach of checking `connector->display_info.is_hdmi` and conditionally setting the mode/AVI infoframes is the right fix:

```c
if (connector->display_info.is_hdmi) {
	mode = IT66121_HDMI_MODE_HDMI;
	avi_pkt = IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT;
}
```

For DVI sinks, `mode` stays `IT66121_HDMI_MODE_DVI` (0) and `avi_pkt` stays 0, which correctly disables HDMI mode and AVI infoframes.

**Issue: `connector->display_info.is_hdmi` accessed outside mutex**

```c
struct drm_connector *connector = ctx->connector;

if (connector->display_info.is_hdmi) {
	...
}

mutex_lock(&ctx->lock);
```

The `display_info.is_hdmi` check happens before `ctx->lock` is taken. While `display_info` is typically stable once the EDID has been read (it's set during `drm_edid_connector_update()` which happens at detect time), and `ctx->connector` was just set in the calling function, this is not a serious concern in practice — the lock here is protecting the regmap writes, not the connector state. But it's worth noting the code reads `ctx->connector` without the lock.

**Issue: Duplicate writes in `.mode_set` still not addressed**

As mentioned in patch 1, the `.mode_set` callback still unconditionally writes `IT66121_HDMI_MODE_HDMI` and enables AVI infoframes. So the sequence during a mode change is:
1. `.mode_set` writes HDMI mode + AVI infoframes ON (always)
2. `.atomic_enable` writes DVI mode + AVI infoframes OFF (for DVI sinks)

While the end state is correct, the DVI monitor momentarily receives HDMI-mode signaling between steps 1 and 2. This could cause transient issues depending on the monitor. Removing the writes from `.mode_set` would eliminate this.

**Good: `IT66121_HDMI_MODE_DVI` define**

```c
#define IT66121_HDMI_MODE_DVI			0
```

Adding a named constant for the DVI mode value (0) instead of using a bare 0 is good practice and makes the code self-documenting.

**Observation: AVI infoframe content still written in `.mode_set` for DVI**

Even after this series, the `.mode_set` callback still writes AVI infoframe *content* to the `IT66121_AVIINFO_DB1_REG` registers and the checksum register for DVI sinks. While the AVI infoframe *transmission* is disabled (the PKT register is cleared in `.atomic_enable`), writing infoframe content to hardware for a DVI connection is wasted work. This is not a bug — just unnecessary register writes. A cleaner approach would be to skip the AVI infoframe preparation in `.mode_set` for DVI sinks, but that would require `connector` to be available in `.mode_set`, which is the whole problem this series is solving. So this is acceptable as-is; the infoframe content simply goes unused.

**Summary**: The series fixes a real bug correctly but needs the duplicate register writes removed from `.mode_set` in patch 1 (or a clear explanation in the commit message for why they're intentionally kept). The enable ordering (unmute before TX mode set) is also worth reconsidering.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2026-05-16  3:19 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-12 13:22 [PATCH v3 0/2] drm/bridge: it66121: Fix display output on DVI monitors Javier Martinez Canillas
2026-05-12 13:22 ` [PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback Javier Martinez Canillas
2026-05-12 14:07   ` Maxime Ripard
2026-05-12 18:54     ` Javier Martinez Canillas
2026-05-13 11:55       ` Maxime Ripard
2026-05-13 12:38         ` Javier Martinez Canillas
2026-05-16  3:19   ` Claude review: " Claude Code Review Bot
2026-05-12 13:22 ` [PATCH v3 2/2] drm/bridge: ite-it66121: Select HDMI or DVI mode based on sink type Javier Martinez Canillas
2026-05-16  3:19   ` Claude review: " Claude Code Review Bot
2026-05-16  3:19 ` Claude review: drm/bridge: it66121: Fix display output on DVI monitors Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox