[PATCH v3 0/2] drm/bridge: adv7511: get edid in hpd_work to update CEC phys address

[PATCH v3 0/2] drm/bridge: adv7511: get edid in hpd_work to update CEC phys address

[Alvin Šipraga]

This series fixes a small bug where the CEC adapter could have an
invalid CEC address even though we got a hotplug connect and could have
read it.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
Changes in v3:
- rebase on latest drm-misc-fixes
- remove redundant NULL check before kfree()
- collect Robert's Reviewed-by
- Link to v2: https://lore.kernel.org/r/20231124-adv7511-cec-edid-v2-0-f0e5eeafdfc2@bang-olufsen.dk

Changes in v2:
- Rearrange driver code to avoid the previous prototype of
  adv7511_get_edid(), per Laurent's feedback
- Free the returned EDID to prevent a memory leak, per Jani's comment
- Link to v1: https://lore.kernel.org/r/20231014-adv7511-cec-edid-v1-1-a58ceae0b57e@bang-olufsen.dk

---
Alvin Šipraga (2):
      drm/bridge: adv7511: rearrange hotplug work code
      drm/bridge: adv7511: get edid in hpd_work to update CEC phys address

 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 153 ++++++++++++++++-----------
 1 file changed, 89 insertions(+), 64 deletions(-)
---
base-commit: 335126937753844d36036984e96a8f343538a778
change-id: 20231014-adv7511-cec-edid-ff75bd3ac929

[PATCH v3 1/2] drm/bridge: adv7511: rearrange hotplug work code

[Alvin Šipraga]

From: Alvin Šipraga <alsi@bang-olufsen.dk>

In preparation for calling EDID helpers from the hotplug work, move the
hotplug work below the EDID helper section. No functional change.

Reviewed-by: Robert Foss <rfoss@kernel.org>
Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 120 ++++++++++++++-------------
 1 file changed, 62 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 8be235144f6d..5ffc5904bd59 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -406,64 +406,6 @@ static void adv7511_power_off(struct adv7511 *adv7511)
  * Interrupt and hotplug detection
  */
 
-static bool adv7511_hpd(struct adv7511 *adv7511)
-{
-	unsigned int irq0;
-	int ret;
-
-	ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), &irq0);
-	if (ret < 0)
-		return false;
-
-	if (irq0 & ADV7511_INT0_HPD) {
-		regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
-			     ADV7511_INT0_HPD);
-		return true;
-	}
-
-	return false;
-}
-
-static void adv7511_hpd_work(struct work_struct *work)
-{
-	struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work);
-	enum drm_connector_status status;
-	unsigned int val;
-	int ret;
-
-	ret = regmap_read(adv7511->regmap, ADV7511_REG_STATUS, &val);
-	if (ret < 0)
-		status = connector_status_disconnected;
-	else if (val & ADV7511_STATUS_HPD)
-		status = connector_status_connected;
-	else
-		status = connector_status_disconnected;
-
-	/*
-	 * The bridge resets its registers on unplug. So when we get a plug
-	 * event and we're already supposed to be powered, cycle the bridge to
-	 * restore its state.
-	 */
-	if (status == connector_status_connected &&
-	    adv7511->connector.status == connector_status_disconnected &&
-	    adv7511->powered) {
-		regcache_mark_dirty(adv7511->regmap);
-		adv7511_power_on(adv7511);
-	}
-
-	if (adv7511->connector.status != status) {
-		adv7511->connector.status = status;
-
-		if (adv7511->connector.dev) {
-			if (status == connector_status_disconnected)
-				cec_phys_addr_invalidate(adv7511->cec_adap);
-			drm_kms_helper_hotplug_event(adv7511->connector.dev);
-		} else {
-			drm_bridge_hpd_notify(&adv7511->bridge, status);
-		}
-	}
-}
-
 static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
 {
 	unsigned int irq0, irq1;
@@ -600,6 +542,68 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
 	return 0;
 }
 
+/* -----------------------------------------------------------------------------
+ * Hotplug handling
+ */
+
+static bool adv7511_hpd(struct adv7511 *adv7511)
+{
+	unsigned int irq0;
+	int ret;
+
+	ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), &irq0);
+	if (ret < 0)
+		return false;
+
+	if (irq0 & ADV7511_INT0_HPD) {
+		regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
+			     ADV7511_INT0_HPD);
+		return true;
+	}
+
+	return false;
+}
+
+static void adv7511_hpd_work(struct work_struct *work)
+{
+	struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work);
+	enum drm_connector_status status;
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(adv7511->regmap, ADV7511_REG_STATUS, &val);
+	if (ret < 0)
+		status = connector_status_disconnected;
+	else if (val & ADV7511_STATUS_HPD)
+		status = connector_status_connected;
+	else
+		status = connector_status_disconnected;
+
+	/*
+	 * The bridge resets its registers on unplug. So when we get a plug
+	 * event and we're already supposed to be powered, cycle the bridge to
+	 * restore its state.
+	 */
+	if (status == connector_status_connected &&
+	    adv7511->connector.status == connector_status_disconnected &&
+	    adv7511->powered) {
+		regcache_mark_dirty(adv7511->regmap);
+		adv7511_power_on(adv7511);
+	}
+
+	if (adv7511->connector.status != status) {
+		adv7511->connector.status = status;
+
+		if (adv7511->connector.dev) {
+			if (status == connector_status_disconnected)
+				cec_phys_addr_invalidate(adv7511->cec_adap);
+			drm_kms_helper_hotplug_event(adv7511->connector.dev);
+		} else {
+			drm_bridge_hpd_notify(&adv7511->bridge, status);
+		}
+	}
+}
+
 /* -----------------------------------------------------------------------------
  * ADV75xx helpers
  */

-- 
2.43.1

Re: [PATCH v3 1/2] drm/bridge: adv7511: rearrange hotplug work code

[Laurent Pinchart]

Hi Alvin,

Thank you for the patch.

On Mon, Feb 19, 2024 at 09:12:58PM +0100, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> In preparation for calling EDID helpers from the hotplug work, move the
> hotplug work below the EDID helper section. No functional change.
> 
> Reviewed-by: Robert Foss <rfoss@kernel.org>
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 120 ++++++++++++++-------------
>  1 file changed, 62 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 8be235144f6d..5ffc5904bd59 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -406,64 +406,6 @@ static void adv7511_power_off(struct adv7511 *adv7511)
>   * Interrupt and hotplug detection
>   */
>  
> -static bool adv7511_hpd(struct adv7511 *adv7511)
> -{
> -	unsigned int irq0;
> -	int ret;
> -
> -	ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), &irq0);
> -	if (ret < 0)
> -		return false;
> -
> -	if (irq0 & ADV7511_INT0_HPD) {
> -		regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
> -			     ADV7511_INT0_HPD);
> -		return true;
> -	}
> -
> -	return false;
> -}
> -
> -static void adv7511_hpd_work(struct work_struct *work)
> -{
> -	struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work);
> -	enum drm_connector_status status;
> -	unsigned int val;
> -	int ret;
> -
> -	ret = regmap_read(adv7511->regmap, ADV7511_REG_STATUS, &val);
> -	if (ret < 0)
> -		status = connector_status_disconnected;
> -	else if (val & ADV7511_STATUS_HPD)
> -		status = connector_status_connected;
> -	else
> -		status = connector_status_disconnected;
> -
> -	/*
> -	 * The bridge resets its registers on unplug. So when we get a plug
> -	 * event and we're already supposed to be powered, cycle the bridge to
> -	 * restore its state.
> -	 */
> -	if (status == connector_status_connected &&
> -	    adv7511->connector.status == connector_status_disconnected &&
> -	    adv7511->powered) {
> -		regcache_mark_dirty(adv7511->regmap);
> -		adv7511_power_on(adv7511);
> -	}
> -
> -	if (adv7511->connector.status != status) {
> -		adv7511->connector.status = status;
> -
> -		if (adv7511->connector.dev) {
> -			if (status == connector_status_disconnected)
> -				cec_phys_addr_invalidate(adv7511->cec_adap);
> -			drm_kms_helper_hotplug_event(adv7511->connector.dev);
> -		} else {
> -			drm_bridge_hpd_notify(&adv7511->bridge, status);
> -		}
> -	}
> -}
> -
>  static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
>  {
>  	unsigned int irq0, irq1;
> @@ -600,6 +542,68 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
>  	return 0;
>  }
>  
> +/* -----------------------------------------------------------------------------
> + * Hotplug handling
> + */
> +
> +static bool adv7511_hpd(struct adv7511 *adv7511)
> +{
> +	unsigned int irq0;
> +	int ret;
> +
> +	ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), &irq0);
> +	if (ret < 0)
> +		return false;
> +
> +	if (irq0 & ADV7511_INT0_HPD) {
> +		regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
> +			     ADV7511_INT0_HPD);
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static void adv7511_hpd_work(struct work_struct *work)
> +{
> +	struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work);
> +	enum drm_connector_status status;
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(adv7511->regmap, ADV7511_REG_STATUS, &val);
> +	if (ret < 0)
> +		status = connector_status_disconnected;
> +	else if (val & ADV7511_STATUS_HPD)
> +		status = connector_status_connected;
> +	else
> +		status = connector_status_disconnected;
> +
> +	/*
> +	 * The bridge resets its registers on unplug. So when we get a plug
> +	 * event and we're already supposed to be powered, cycle the bridge to
> +	 * restore its state.
> +	 */
> +	if (status == connector_status_connected &&
> +	    adv7511->connector.status == connector_status_disconnected &&
> +	    adv7511->powered) {
> +		regcache_mark_dirty(adv7511->regmap);
> +		adv7511_power_on(adv7511);
> +	}
> +
> +	if (adv7511->connector.status != status) {
> +		adv7511->connector.status = status;
> +
> +		if (adv7511->connector.dev) {
> +			if (status == connector_status_disconnected)
> +				cec_phys_addr_invalidate(adv7511->cec_adap);
> +			drm_kms_helper_hotplug_event(adv7511->connector.dev);
> +		} else {
> +			drm_bridge_hpd_notify(&adv7511->bridge, status);
> +		}
> +	}
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * ADV75xx helpers
>   */

-- 
Regards,

Laurent Pinchart

[PATCH v3 2/2] drm/bridge: adv7511: get edid in hpd_work to update CEC phys address

[Alvin Šipraga]

From: Alvin Šipraga <alsi@bang-olufsen.dk>

The adv7511 driver is solely responsible for setting the physical
address of its CEC adapter. To do this, it must read the EDID. However,
EDID is only read when either the drm_bridge_funcs :: get_edid or
drm_connector_helper_funcs :: get_modes ops are called. Without loss of
generality, it cannot be assumed that these ops are called when a sink
gets attached. Therefore there exist scenarios in which the CEC physical
address will be invalid (f.f.f.f), rendering the CEC adapter inoperable.

Address this problem by always fetching the EDID in the HPD work when we
detect a connection. The CEC physical address is set in the process.
This is done by moving the EDID DRM helper into an internal helper
function so that it can be cleanly called from an earlier section of
the code. The EDID getter has not changed in practice.

Reviewed-by: Robert Foss <rfoss@kernel.org>
Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 73 ++++++++++++++++++----------
 1 file changed, 47 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 5ffc5904bd59..d823b372ff43 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -542,6 +542,36 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
 	return 0;
 }
 
+static struct edid *__adv7511_get_edid(struct adv7511 *adv7511,
+				       struct drm_connector *connector)
+{
+	struct edid *edid;
+
+	/* Reading the EDID only works if the device is powered */
+	if (!adv7511->powered) {
+		unsigned int edid_i2c_addr =
+					(adv7511->i2c_edid->addr << 1);
+
+		__adv7511_power_on(adv7511);
+
+		/* Reset the EDID_I2C_ADDR register as it might be cleared */
+		regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
+			     edid_i2c_addr);
+	}
+
+	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
+
+	if (!adv7511->powered)
+		__adv7511_power_off(adv7511);
+
+	adv7511_set_config_csc(adv7511, connector, adv7511->rgb,
+			       drm_detect_hdmi_monitor(edid));
+
+	cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
+
+	return edid;
+}
+
 /* -----------------------------------------------------------------------------
  * Hotplug handling
  */
@@ -595,8 +625,23 @@ static void adv7511_hpd_work(struct work_struct *work)
 		adv7511->connector.status = status;
 
 		if (adv7511->connector.dev) {
-			if (status == connector_status_disconnected)
+			if (status == connector_status_disconnected) {
 				cec_phys_addr_invalidate(adv7511->cec_adap);
+			} else {
+				struct edid *edid;
+
+				/*
+				 * Get the updated EDID so that the CEC
+				 * subsystem gets informed of any change in CEC
+				 * address. The helper returns a newly allocated
+				 * edid structure, so free it to prevent
+				 * leakage.
+				 */
+				edid = __adv7511_get_edid(adv7511,
+							  &adv7511->connector);
+				kfree(edid);
+			}
+
 			drm_kms_helper_hotplug_event(adv7511->connector.dev);
 		} else {
 			drm_bridge_hpd_notify(&adv7511->bridge, status);
@@ -611,31 +656,7 @@ static void adv7511_hpd_work(struct work_struct *work)
 static struct edid *adv7511_get_edid(struct adv7511 *adv7511,
 				     struct drm_connector *connector)
 {
-	struct edid *edid;
-
-	/* Reading the EDID only works if the device is powered */
-	if (!adv7511->powered) {
-		unsigned int edid_i2c_addr =
-					(adv7511->i2c_edid->addr << 1);
-
-		__adv7511_power_on(adv7511);
-
-		/* Reset the EDID_I2C_ADDR register as it might be cleared */
-		regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
-			     edid_i2c_addr);
-	}
-
-	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
-
-	if (!adv7511->powered)
-		__adv7511_power_off(adv7511);
-
-	adv7511_set_config_csc(adv7511, connector, adv7511->rgb,
-			       drm_detect_hdmi_monitor(edid));
-
-	cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
-
-	return edid;
+	return __adv7511_get_edid(adv7511, connector);
 }
 
 static int adv7511_get_modes(struct adv7511 *adv7511,

-- 
2.43.1

Re: [PATCH v3 2/2] drm/bridge: adv7511: get edid in hpd_work to update CEC phys address

[Robert Foss]

Sorry to ask for this again, but this series has non-trivial merge
conflicts with upstream again.

Could you rebase it and send out an updated version?

Re: [PATCH v3 2/2] drm/bridge: adv7511: get edid in hpd_work to update CEC phys address

[Jani Nikula]

On Tue, 05 Mar 2024, Robert Foss <rfoss@kernel.org> wrote:
> Sorry to ask for this again, but this series has non-trivial merge
> conflicts with upstream again.
>
> Could you rebase it and send out an updated version?

Not just rebase, but all of bridge has been switched over to struct
drm_edid based interfaces.

BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH v3 2/2] drm/bridge: adv7511: get edid in hpd_work to update CEC phys address

[Laurent Pinchart]

Hello Alvin,

Thank you for the patch.

On Mon, Feb 19, 2024 at 09:12:59PM +0100, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> The adv7511 driver is solely responsible for setting the physical
> address of its CEC adapter. To do this, it must read the EDID. However,
> EDID is only read when either the drm_bridge_funcs :: get_edid or
> drm_connector_helper_funcs :: get_modes ops are called. Without loss of
> generality, it cannot be assumed that these ops are called when a sink
> gets attached.

I wonder if that should be fixed, but it would likely be a quite big
rework of the DRM core. I've been thinking for several years now that
hotplug handling could do with more love.

> Therefore there exist scenarios in which the CEC physical
> address will be invalid (f.f.f.f), rendering the CEC adapter inoperable.
> 
> Address this problem by always fetching the EDID in the HPD work when we
> detect a connection. The CEC physical address is set in the process.
> This is done by moving the EDID DRM helper into an internal helper
> function so that it can be cleanly called from an earlier section of
> the code. The EDID getter has not changed in practice.
> 
> Reviewed-by: Robert Foss <rfoss@kernel.org>
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 73 ++++++++++++++++++----------
>  1 file changed, 47 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 5ffc5904bd59..d823b372ff43 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -542,6 +542,36 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
>  	return 0;
>  }
>  
> +static struct edid *__adv7511_get_edid(struct adv7511 *adv7511,
> +				       struct drm_connector *connector)
> +{
> +	struct edid *edid;
> +
> +	/* Reading the EDID only works if the device is powered */
> +	if (!adv7511->powered) {
> +		unsigned int edid_i2c_addr =
> +					(adv7511->i2c_edid->addr << 1);
> +
> +		__adv7511_power_on(adv7511);
> +
> +		/* Reset the EDID_I2C_ADDR register as it might be cleared */
> +		regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> +			     edid_i2c_addr);
> +	}
> +
> +	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
> +
> +	if (!adv7511->powered)
> +		__adv7511_power_off(adv7511);
> +
> +	adv7511_set_config_csc(adv7511, connector, adv7511->rgb,
> +			       drm_detect_hdmi_monitor(edid));
> +
> +	cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
> +
> +	return edid;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Hotplug handling
>   */
> @@ -595,8 +625,23 @@ static void adv7511_hpd_work(struct work_struct *work)
>  		adv7511->connector.status = status;
>  
>  		if (adv7511->connector.dev) {
> -			if (status == connector_status_disconnected)
> +			if (status == connector_status_disconnected) {
>  				cec_phys_addr_invalidate(adv7511->cec_adap);
> +			} else {
> +				struct edid *edid;
> +
> +				/*
> +				 * Get the updated EDID so that the CEC
> +				 * subsystem gets informed of any change in CEC
> +				 * address. The helper returns a newly allocated
> +				 * edid structure, so free it to prevent
> +				 * leakage.
> +				 */
> +				edid = __adv7511_get_edid(adv7511,
> +							  &adv7511->connector);
> +				kfree(edid);

This means that we will, in most case, fetch EDID twice when a monitor
is plugged in: once here, and once when the DRM core will call the
.get_edid() operation. I wonder, would it make sense to cache the EDID
here, and return the cache version from adv7511_get_edid() ?

> +			}
> +
>  			drm_kms_helper_hotplug_event(adv7511->connector.dev);
>  		} else {
>  			drm_bridge_hpd_notify(&adv7511->bridge, status);
> @@ -611,31 +656,7 @@ static void adv7511_hpd_work(struct work_struct *work)
>  static struct edid *adv7511_get_edid(struct adv7511 *adv7511,
>  				     struct drm_connector *connector)
>  {
> -	struct edid *edid;
> -
> -	/* Reading the EDID only works if the device is powered */
> -	if (!adv7511->powered) {
> -		unsigned int edid_i2c_addr =
> -					(adv7511->i2c_edid->addr << 1);
> -
> -		__adv7511_power_on(adv7511);
> -
> -		/* Reset the EDID_I2C_ADDR register as it might be cleared */
> -		regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> -			     edid_i2c_addr);
> -	}
> -
> -	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
> -
> -	if (!adv7511->powered)
> -		__adv7511_power_off(adv7511);
> -
> -	adv7511_set_config_csc(adv7511, connector, adv7511->rgb,
> -			       drm_detect_hdmi_monitor(edid));
> -
> -	cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
> -
> -	return edid;
> +	return __adv7511_get_edid(adv7511, connector);
>  }
>  
>  static int adv7511_get_modes(struct adv7511 *adv7511,
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/2] drm/bridge: adv7511: get edid in hpd_work to update CEC phys address

[Jani Nikula]

On Mon, 19 Feb 2024, Alvin Šipraga <alvin@pqrs.dk> wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
>
> The adv7511 driver is solely responsible for setting the physical
> address of its CEC adapter. To do this, it must read the EDID. However,
> EDID is only read when either the drm_bridge_funcs :: get_edid or
> drm_connector_helper_funcs :: get_modes ops are called. Without loss of
> generality, it cannot be assumed that these ops are called when a sink
> gets attached. Therefore there exist scenarios in which the CEC physical
> address will be invalid (f.f.f.f), rendering the CEC adapter inoperable.
>
> Address this problem by always fetching the EDID in the HPD work when we
> detect a connection. The CEC physical address is set in the process.
> This is done by moving the EDID DRM helper into an internal helper
> function so that it can be cleanly called from an earlier section of
> the code. The EDID getter has not changed in practice.
>
> Reviewed-by: Robert Foss <rfoss@kernel.org>
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 73 ++++++++++++++++++----------
>  1 file changed, 47 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 5ffc5904bd59..d823b372ff43 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -542,6 +542,36 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
>  	return 0;
>  }
>  
> +static struct edid *__adv7511_get_edid(struct adv7511 *adv7511,
> +				       struct drm_connector *connector)
> +{
> +	struct edid *edid;
> +
> +	/* Reading the EDID only works if the device is powered */
> +	if (!adv7511->powered) {
> +		unsigned int edid_i2c_addr =
> +					(adv7511->i2c_edid->addr << 1);
> +
> +		__adv7511_power_on(adv7511);
> +
> +		/* Reset the EDID_I2C_ADDR register as it might be cleared */
> +		regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> +			     edid_i2c_addr);
> +	}
> +
> +	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
> +
> +	if (!adv7511->powered)
> +		__adv7511_power_off(adv7511);
> +
> +	adv7511_set_config_csc(adv7511, connector, adv7511->rgb,
> +			       drm_detect_hdmi_monitor(edid));
> +
> +	cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);

It really would be better to do drm_edid_read_custom(),
drm_edid_connector_update(), and cec_s_phys_addr() with the physical
address from connector->display_info.source_physical_address initialized
by drm_edid_connector_update().

The point is, cec_s_phys_addr_from_edid() has its own duplicate EDID
parsing, which is slightly different from drm_edid_connector_update()
and of course redundant.


BR,
Jani.


> +
> +	return edid;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Hotplug handling
>   */
> @@ -595,8 +625,23 @@ static void adv7511_hpd_work(struct work_struct *work)
>  		adv7511->connector.status = status;
>  
>  		if (adv7511->connector.dev) {
> -			if (status == connector_status_disconnected)
> +			if (status == connector_status_disconnected) {
>  				cec_phys_addr_invalidate(adv7511->cec_adap);
> +			} else {
> +				struct edid *edid;
> +
> +				/*
> +				 * Get the updated EDID so that the CEC
> +				 * subsystem gets informed of any change in CEC
> +				 * address. The helper returns a newly allocated
> +				 * edid structure, so free it to prevent
> +				 * leakage.
> +				 */
> +				edid = __adv7511_get_edid(adv7511,
> +							  &adv7511->connector);
> +				kfree(edid);
> +			}
> +
>  			drm_kms_helper_hotplug_event(adv7511->connector.dev);
>  		} else {
>  			drm_bridge_hpd_notify(&adv7511->bridge, status);
> @@ -611,31 +656,7 @@ static void adv7511_hpd_work(struct work_struct *work)
>  static struct edid *adv7511_get_edid(struct adv7511 *adv7511,
>  				     struct drm_connector *connector)
>  {
> -	struct edid *edid;
> -
> -	/* Reading the EDID only works if the device is powered */
> -	if (!adv7511->powered) {
> -		unsigned int edid_i2c_addr =
> -					(adv7511->i2c_edid->addr << 1);
> -
> -		__adv7511_power_on(adv7511);
> -
> -		/* Reset the EDID_I2C_ADDR register as it might be cleared */
> -		regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> -			     edid_i2c_addr);
> -	}
> -
> -	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
> -
> -	if (!adv7511->powered)
> -		__adv7511_power_off(adv7511);
> -
> -	adv7511_set_config_csc(adv7511, connector, adv7511->rgb,
> -			       drm_detect_hdmi_monitor(edid));
> -
> -	cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
> -
> -	return edid;
> +	return __adv7511_get_edid(adv7511, connector);
>  }
>  
>  static int adv7511_get_modes(struct adv7511 *adv7511,

-- 
Jani Nikula, Intel