diff mbox series

[v6,09/22] drm: bridge: dw_hdmi: Invalidate CEC phys addr from connector detect

Message ID 20260516183838.2024991-10-jonas@kwiboo.se (mailing list archive)
State New
Headers
Series drm: bridge: dw_hdmi: Misc enable/disable, CEC and EDID cleanup |

Commit Message

Jonas Karlman May 16, 2026, 6:38 p.m. UTC 
Wait until the connector detect ops is called to invalidate CEC phys
addr instead of doing it directly from the irq handler.

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # Rock64, RockPro64, Quartz64-B
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v6: Collect t-b tag
v5: No change
v4: No change
v3: No change
v2: Collect r-b tag
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

sashiko-bot@kernel.org May 16, 2026, 7:07 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Moving CEC physical address invalidation exclusively to dw_hdmi_connector_detect breaks CEC functionality when dw_hdmi operates purely as a DRM bridge.
--

commit 05206110d2cbe02dc5e2cae3b6c3e7d2d8cdbe48
Author: Jonas Karlman <jonas@kwiboo.se>
Subject: drm: bridge: dw_hdmi: Invalidate CEC phys addr from connector detect

This commit delays the CEC physical address invalidation from the IRQ
handler until the connector detect operation is called.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 5fd26ff8f55b4..aae1b890167b1 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -2472,7 +2472,17 @@ dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
>  {
>  	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
>  					     connector);
> -	return dw_hdmi_detect(hdmi);
> +	enum drm_connector_status status;
> +
> +	status = dw_hdmi_detect(hdmi);
> +
> +	if (status == connector_status_disconnected) {
> +		mutex_lock(&hdmi->cec_notifier_mutex);
> +		cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
> +		mutex_unlock(&hdmi->cec_notifier_mutex);
> +	}
> +
> +	return status;
>  }

Does moving the invalidation exclusively to dw_hdmi_connector_detect break
CEC functionality when the hardware operates purely as a DRM bridge?

Before this change, cec_notifier_phys_addr_invalidate was called directly
from the hardware interrupt handler, guaranteeing execution on disconnect
regardless of how the driver was configured.

In bridge-only mode, the connector is created externally and
dw_hdmi_connector_detect is never executed. The DRM core or downstream
component instead calls dw_hdmi_bridge_detect, which was not updated here
to include the CEC invalidation logic. The CEC physical address might not
be invalidated on unplug in bridge mode, which would be a regression.

I see this is addressed later in the series by commit 795c172faddab (drm:
bridge: dw_hdmi: Update EDID and CEC phys addr in bridge detect()), but
this intermediate patch appears to introduce a functional regression that
might affect bisection.

[ ... ]

> @@ -3106,12 +3116,6 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>  				       phy_stat & HDMI_PHY_HPD,
>  				       phy_stat & HDMI_PHY_RX_SENSE);
>  
> -		if ((phy_stat & (HDMI_PHY_RX_SENSE | HDMI_PHY_HPD)) == 0) {
> -			mutex_lock(&hdmi->cec_notifier_mutex);
> -			cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
> -			mutex_unlock(&hdmi->cec_notifier_mutex);
> -		}
> -
>  		if ((intr_stat & HDMI_IH_PHY_STAT0_HPD) &&
>  		    (phy_stat & HDMI_PHY_HPD))
>  			status = connector_status_connected;
Jonas Karlman May 16, 2026, 7:12 p.m. UTC | #2 
On 5/16/2026 9:07 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Moving CEC physical address invalidation exclusively to dw_hdmi_connector_detect breaks CEC functionality when dw_hdmi operates purely as a DRM bridge.
> --
> 
> commit 05206110d2cbe02dc5e2cae3b6c3e7d2d8cdbe48
> Author: Jonas Karlman <jonas@kwiboo.se>
> Subject: drm: bridge: dw_hdmi: Invalidate CEC phys addr from connector detect
> 
> This commit delays the CEC physical address invalidation from the IRQ
> handler until the connector detect operation is called.
> 
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index 5fd26ff8f55b4..aae1b890167b1 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -2472,7 +2472,17 @@ dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
>>  {
>>  	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
>>  					     connector);
>> -	return dw_hdmi_detect(hdmi);
>> +	enum drm_connector_status status;
>> +
>> +	status = dw_hdmi_detect(hdmi);
>> +
>> +	if (status == connector_status_disconnected) {
>> +		mutex_lock(&hdmi->cec_notifier_mutex);
>> +		cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
>> +		mutex_unlock(&hdmi->cec_notifier_mutex);
>> +	}
>> +
>> +	return status;
>>  }
> 
> Does moving the invalidation exclusively to dw_hdmi_connector_detect break
> CEC functionality when the hardware operates purely as a DRM bridge?

No, hdmi->cec_notifier is going to be NULL when the hardware operates
purely as a DRM bridge, CEC notifier was only created for the dw-hdmi
connector prior to this series.

Regards,
Jonas

> Before this change, cec_notifier_phys_addr_invalidate was called directly
> from the hardware interrupt handler, guaranteeing execution on disconnect
> regardless of how the driver was configured.
> 
> In bridge-only mode, the connector is created externally and
> dw_hdmi_connector_detect is never executed. The DRM core or downstream
> component instead calls dw_hdmi_bridge_detect, which was not updated here
> to include the CEC invalidation logic. The CEC physical address might not
> be invalidated on unplug in bridge mode, which would be a regression.
> 
> I see this is addressed later in the series by commit 795c172faddab (drm:
> bridge: dw_hdmi: Update EDID and CEC phys addr in bridge detect()), but
> this intermediate patch appears to introduce a functional regression that
> might affect bisection.
> 
> [ ... ]
> 
>> @@ -3106,12 +3116,6 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>>  				       phy_stat & HDMI_PHY_HPD,
>>  				       phy_stat & HDMI_PHY_RX_SENSE);
>>  
>> -		if ((phy_stat & (HDMI_PHY_RX_SENSE | HDMI_PHY_HPD)) == 0) {
>> -			mutex_lock(&hdmi->cec_notifier_mutex);
>> -			cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
>> -			mutex_unlock(&hdmi->cec_notifier_mutex);
>> -		}
>> -
>>  		if ((intr_stat & HDMI_IH_PHY_STAT0_HPD) &&
>>  		    (phy_stat & HDMI_PHY_HPD))
>>  			status = connector_status_connected;
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 5fd26ff8f55b..aae1b890167b 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2472,7 +2472,17 @@  dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
 {
 	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
 					     connector);
-	return dw_hdmi_detect(hdmi);
+	enum drm_connector_status status;
+
+	status = dw_hdmi_detect(hdmi);
+
+	if (status == connector_status_disconnected) {
+		mutex_lock(&hdmi->cec_notifier_mutex);
+		cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
+		mutex_unlock(&hdmi->cec_notifier_mutex);
+	}
+
+	return status;
 }
 
 static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
@@ -3106,12 +3116,6 @@  static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
 				       phy_stat & HDMI_PHY_HPD,
 				       phy_stat & HDMI_PHY_RX_SENSE);
 
-		if ((phy_stat & (HDMI_PHY_RX_SENSE | HDMI_PHY_HPD)) == 0) {
-			mutex_lock(&hdmi->cec_notifier_mutex);
-			cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
-			mutex_unlock(&hdmi->cec_notifier_mutex);
-		}
-
 		if ((intr_stat & HDMI_IH_PHY_STAT0_HPD) &&
 		    (phy_stat & HDMI_PHY_HPD))
 			status = connector_status_connected;