[PATCH v3 0/2] drm/bridge: it66121: Fix display output on DVI monitors

[PATCH v3 0/2] drm/bridge: it66121: Fix display output on DVI monitors

[Javier Martinez Canillas]

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

[PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback

[Javier Martinez Canillas]

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

Re: [PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback

[Maxime Ripard]

[-- 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 --]

Re: [PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback

[Javier Martinez Canillas]

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

Re: [PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback

[Maxime Ripard]

[-- 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 --]

Re: [PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback

[Javier Martinez Canillas]

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

[PATCH v3 2/2] drm/bridge: ite-it66121: Select HDMI or DVI mode based on sink type

[Javier Martinez Canillas]

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