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

[RFC PATCH] 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.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---

Hi, I'm sending this as an RFC patch because:

- The added prototype is a bit crude, but I want feedback on the idea
  first before moving around a bunch of functions to avoid said
  prototype
- I'm not sure if the rationale stated above is correct. I am not very
  familiar with the drm subsystem so perhaps there is something obvious
  that I am missing. At any rate, this solved the issue of an inoperable
  CEC adapter when the downstream MIPI-DSI was not used.

I'm also wondering if this is robust enough to work when a sink just
pulses HPD to signal that its EDID has changed. Currently the hotplug
notifier is only invoked when the connected state changes (e.g. from
disconnected to connected), and so I only check the EDID in such
scenarios too. But is that always the case? Might we get an HPD
interrupt and the connectivity state remains the same?

Thanks in advance for any feedback.

---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 005bf18682ff..fb4ca277e4be 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -424,6 +424,9 @@ static bool adv7511_hpd(struct adv7511 *adv7511)
 	return false;
 }
 
+static struct edid *adv7511_get_edid(struct adv7511 *adv7511,
+				     struct drm_connector *connector);
+
 static void adv7511_hpd_work(struct work_struct *work)
 {
 	struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work);
@@ -457,6 +460,9 @@ static void adv7511_hpd_work(struct work_struct *work)
 		if (adv7511->connector.dev) {
 			if (status == connector_status_disconnected)
 				cec_phys_addr_invalidate(adv7511->cec_adap);
+			else
+				adv7511_get_edid(adv7511, &adv7511->connector);
+
 			drm_kms_helper_hotplug_event(adv7511->connector.dev);
 		} else {
 			drm_bridge_hpd_notify(&adv7511->bridge, status);
-- 
2.35.1

[RFC PATCH] 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.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---

Hi, I'm sending this as an RFC patch because:

- The added prototype is a bit crude, but I want feedback on the idea
  first before moving around a bunch of functions to avoid said
  prototype
- I'm not sure if the rationale stated above is correct. I am not very
  familiar with the drm subsystem so perhaps there is something obvious
  that I am missing. At any rate, this solved the issue of an inoperable
  CEC adapter when the downstream MIPI-DSI was not used.

I'm also wondering if this is robust enough to work when a sink just
pulses HPD to signal that its EDID has changed. Currently the hotplug
notifier is only invoked when the connected state changes (e.g. from
disconnected to connected), and so I only check the EDID in such
scenarios too. But is that always the case? Might we get an HPD
interrupt and the connectivity state remains the same?

Thanks in advance for any feedback.

---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 005bf18682ff..fb4ca277e4be 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -424,6 +424,9 @@ static bool adv7511_hpd(struct adv7511 *adv7511)
 	return false;
 }
 
+static struct edid *adv7511_get_edid(struct adv7511 *adv7511,
+				     struct drm_connector *connector);
+
 static void adv7511_hpd_work(struct work_struct *work)
 {
 	struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work);
@@ -457,6 +460,9 @@ static void adv7511_hpd_work(struct work_struct *work)
 		if (adv7511->connector.dev) {
 			if (status == connector_status_disconnected)
 				cec_phys_addr_invalidate(adv7511->cec_adap);
+			else
+				adv7511_get_edid(adv7511, &adv7511->connector);
+
 			drm_kms_helper_hotplug_event(adv7511->connector.dev);
 		} else {
 			drm_bridge_hpd_notify(&adv7511->bridge, status);
-- 
2.35.1