[PATCHv3 00/13] media/drm: cec: add HPD_PRESENT

[PATCHv3 00/13] media/drm: cec: add HPD_PRESENT

[Hans Verkuil]

Using the media CEC framework the application receives events when the
Physical and/or Logical Addresses change. However, if the Physical Address
changes to f.f.f.f (Invalid), it does not know whether that is because the
HDMI link is disconnected, or because the EDID does not provide a valid
Physical Address. That can happen because the display does not support CEC,
as is the case for DisplayPort inputs or PC monitors.

This is important information to have but this is currently missing.

This series adds support for that.

Currently drivers set the physical address either with cec_s_phys_addr() or
with cec_phys_addr_invalidate(), where the second is just a helper
function that calls cec_s_phys_addr with CEC_PHYS_ADDR_INVALID. There
are also cec_notifier_ counterparts.

Change the behavior so that cec_s_phys_addr() is called whenever the HPD
is high. In that case CEC_PHYS_ADDR_INVALID means that the EDID is
present but does not contain a valid PA. And cec_phys_addr_invalidate()
is only called when the video link is disconnected.

Most CEC drivers already did that, except for the few that are fixed in the
first 6 patches.

The remaining patches implement the new functionality where the CEC
framework actually reports whether or not the HPD was present, based
on whether cec_s_phys_addr or cec_phys_addr_invalidate was called.

Note: I think cec_phys_addr_invalidate should be renamed to something like
cec_phys_addr_disconnect given the changes semantics of this function.
I will tackle that once this series is in.

The drm patches can either be queued up in the drm subsystem independently
of the remaining patches, or I can take them through the media subsytem
as part of the series, but then I need an Acked/Reviewed-by.

Regards,

	Hans

Changes in v3:

- Renamed EDID_PRESENT to HPD_PRESENT, since that's what it really is.

Changes in v2 (based on sashiko reviews):

- missed one incorrect cec_notifier_set_phys_addr call in sti_hdmi.c
- cec_notifier_cec_adap_register didn't call cec_phys_addr_invalidate
  if an earlier cec_notifier_phys_addr_invalidate call indicated that
  there is no EDID.
- sending the initial state change event didn't set the
  CEC_EVENT_FL_STATE_EDID_PRESENT flag if needed.

Hans Verkuil (13):
  gpu: drm: sti: call cec_notifier_phys_addr_invalidate if no HPD
  gpu: drm: exynos: call cec_notifier_phys_addr_invalidate if no HPD
  gpu: drm: omapdrm: call cec_s_phys_addr only if HPD is present
  media: i2c: adv7511-v4l2: call cec_s_phys_addr only if HPD is high
  media: cec: extron-da-hd-4k-plus: call cec_s_phys_addr only if HPD is
    high
  media: cec: core: improve cec_notifier_phys_addr_invalidate
  media: uapi: cec.h: add
    CEC_CAP_HAS_HPD_PRESENT/CEC_LOG_ADDRS_FL_HPD_PRESENT
  Documentation: media: document HPD_PRESENT cap and flag.
  media: cec: core: add support for HPD_PRESENT
  media: uapi: cec.h: add CEC_EVENT_FL_STATE_HPD_PRESENT flag
  Documentation: media: cec-ioc-dqevent.rst: document STATE_HPD_PRESENT
    flag
  media: cec: core: implement CEC_EVENT_FL_STATE_HPD_PRESENT
  Documentation: media: document cec_phys_addr_invalidate

 Documentation/driver-api/media/cec-core.rst   | 10 +++-
 .../media/cec/cec-ioc-adap-g-caps.rst         | 11 ++++
 .../media/cec/cec-ioc-adap-g-log-addrs.rst    | 10 ++++
 .../media/cec/cec-ioc-dqevent.rst             | 10 ++++
 drivers/gpu/drm/exynos/exynos_hdmi.c          |  2 +-
 drivers/gpu/drm/omapdrm/dss/hdmi4.c           |  7 ++-
 drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c       |  5 ++
 drivers/gpu/drm/omapdrm/dss/hdmi4_cec.h       |  5 ++
 drivers/gpu/drm/sti/sti_hdmi.c                |  4 +-
 drivers/media/cec/core/cec-adap.c             | 51 ++++++++++++++++---
 drivers/media/cec/core/cec-api.c              | 13 +++--
 drivers/media/cec/core/cec-core.c             |  4 +-
 drivers/media/cec/core/cec-notifier.c         | 26 +++++++++-
 drivers/media/cec/core/cec-priv.h             |  3 +-
 .../extron-da-hd-4k-plus.c                    |  8 ++-
 drivers/media/i2c/adv7511-v4l2.c              |  2 +-
 include/media/cec-notifier.h                  | 33 +++++++-----
 include/media/cec.h                           | 27 +++++-----
 include/uapi/linux/cec.h                      | 29 ++++++-----
 19 files changed, 193 insertions(+), 67 deletions(-)

-- 
2.53.0

[PATCHv3 01/13] gpu: drm: sti: call cec_notifier_phys_addr_invalidate if no HPD

[Hans Verkuil]

If a HPD is present, then call cec_notifier_set_phys_addr. If not, then
call cec_notifier_phys_addr_invalidate. This helps the CEC framework
differentiate between CEC_PHYS_ADDR_INVALID due to no Hotplug Detect, or
due to an EDID without a Physical Address.

This driver is one of the few that was using cec_notifier_set_phys_addr
on disconnect instead of cec_notifier_phys_addr_invalidate.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
Cc: Alain Volmat <alain.volmat@foss.st.com>
Cc: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/sti/sti_hdmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index 7ba9e0b7ba6c..12e05505bbde 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -790,7 +790,7 @@ static void sti_hdmi_disable(struct drm_bridge *bridge,
 
 	hdmi->enabled = false;
 
-	cec_notifier_set_phys_addr(hdmi->notifier, CEC_PHYS_ADDR_INVALID);
+	cec_notifier_phys_addr_invalidate(hdmi->notifier);
 }
 
 /*
@@ -1068,7 +1068,7 @@ sti_hdmi_connector_detect(struct drm_connector *connector, bool force)
 	}
 
 	DRM_DEBUG_DRIVER("hdmi cable disconnected\n");
-	cec_notifier_set_phys_addr(hdmi->notifier, CEC_PHYS_ADDR_INVALID);
+	cec_notifier_phys_addr_invalidate(hdmi->notifier);
 	return connector_status_disconnected;
 }
 
-- 
2.53.0

[PATCHv3 02/13] gpu: drm: exynos: call cec_notifier_phys_addr_invalidate if no HPD

[Hans Verkuil]

If a HPD is present, then call cec_notifier_set_phys_addr. If not, then
call cec_notifier_phys_addr_invalidate. This helps the CEC framework
differentiate between CEC_PHYS_ADDR_INVALID due to no Hotplug Detect, or
due to an EDID without a Physical Address.

This driver is one of the few that was using cec_notifier_set_phys_addr
on disconnect instead of cec_notifier_phys_addr_invalidate.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Peter Griffin <peter.griffin@linaro.org>
Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 055dc04f9b3e..d03f4e531e05 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -857,7 +857,7 @@ static enum drm_connector_status hdmi_detect(struct drm_connector *connector,
 	if (gpiod_get_value(hdata->hpd_gpio))
 		return connector_status_connected;
 
-	cec_notifier_set_phys_addr(hdata->notifier, CEC_PHYS_ADDR_INVALID);
+	cec_notifier_phys_addr_invalidate(hdata->notifier);
 	return connector_status_disconnected;
 }
 
-- 
2.53.0

Re: [PATCHv3 02/13] gpu: drm: exynos: call cec_notifier_phys_addr_invalidate if no HPD

[Krzysztof Kozlowski]

On 05/11/2026 08:22, Hans Verkuil wrote:
> If a HPD is present, then call cec_notifier_set_phys_addr. If not, then
> call cec_notifier_phys_addr_invalidate. This helps the CEC framework
> differentiate between CEC_PHYS_ADDR_INVALID due to no Hotplug Detect, or
> due to an EDID without a Physical Address.
> 
> This driver is one of the few that was using cec_notifier_set_phys_addr
> on disconnect instead of cec_notifier_phys_addr_invalidate.
> 
> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: Peter Griffin <peter.griffin@linaro.org>
> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> ---


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

[PATCHv3 03/13] gpu: drm: omapdrm: call cec_s_phys_addr only if HPD is present

[Hans Verkuil]

If a HPD is present, then call cec_s_phys_addr. If not, then
call cec_phys_addr_invalidate. This helps the CEC framework differentiate
between CEC_PHYS_ADDR_INVALID due to no Hotplug Detect, or due to an EDID
without a Physical Address.

This driver is one of the few that was doing this inconsistently.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/omapdrm/dss/hdmi4.c     | 7 +++----
 drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c | 5 +++++
 drivers/gpu/drm/omapdrm/dss/hdmi4_cec.h | 5 +++++
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
index bd6c4d2dcdc8..056bede2ec8a 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
@@ -439,7 +439,7 @@ static void hdmi4_bridge_hpd_notify(struct drm_bridge *bridge,
 		if (hdmi_runtime_get(hdmi))
 			return;
 
-		hdmi4_cec_set_phys_addr(&hdmi->core, CEC_PHYS_ADDR_INVALID);
+		hdmi4_cec_phys_addr_invalidate(&hdmi->core);
 		hdmi_runtime_put(hdmi);
 	}
 }
@@ -487,12 +487,11 @@ static const struct drm_edid *hdmi4_bridge_edid_read(struct drm_bridge *bridge,
 		unsigned int len = (edid->extensions + 1) * EDID_LENGTH;
 
 		cec_addr = cec_get_edid_phys_addr((u8 *)edid, len, NULL);
+		hdmi4_cec_set_phys_addr(&hdmi->core, cec_addr);
 	} else {
-		cec_addr = CEC_PHYS_ADDR_INVALID;
+		hdmi4_cec_phys_addr_invalidate(&hdmi->core);
 	}
 
-	hdmi4_cec_set_phys_addr(&hdmi->core, cec_addr);
-
 	if (need_enable)
 		hdmi4_core_disable(&hdmi->core);
 
diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
index 852987e67e40..60d0d85dccad 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c
@@ -322,6 +322,11 @@ void hdmi4_cec_set_phys_addr(struct hdmi_core_data *core, u16 pa)
 	cec_s_phys_addr(core->adap, pa, false);
 }
 
+void hdmi4_cec_phys_addr_invalidate(struct hdmi_core_data *core)
+{
+	cec_phys_addr_invalidate(core->adap);
+}
+
 int hdmi4_cec_init(struct platform_device *pdev, struct hdmi_core_data *core,
 		  struct hdmi_wp_data *wp)
 {
diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.h b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.h
index 1ca5b5ca8a99..234690dd3a1e 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.h
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_cec.h
@@ -15,6 +15,7 @@ struct platform_device;
 /* HDMI CEC funcs */
 #ifdef CONFIG_OMAP4_DSS_HDMI_CEC
 void hdmi4_cec_set_phys_addr(struct hdmi_core_data *core, u16 pa);
+void hdmi4_cec_phys_addr_invalidate(struct hdmi_core_data *core);
 void hdmi4_cec_irq(struct hdmi_core_data *core);
 int hdmi4_cec_init(struct platform_device *pdev, struct hdmi_core_data *core,
 		  struct hdmi_wp_data *wp);
@@ -24,6 +25,10 @@ static inline void hdmi4_cec_set_phys_addr(struct hdmi_core_data *core, u16 pa)
 {
 }
 
+static inline void hdmi4_cec_phys_addr_invalidate(struct hdmi_core_data *core)
+{
+}
+
 static inline void hdmi4_cec_irq(struct hdmi_core_data *core)
 {
 }
-- 
2.53.0

[PATCHv3 04/13] media: i2c: adv7511-v4l2: call cec_s_phys_addr only if HPD is high

[Hans Verkuil]

If the HPD is high, then call cec_s_phys_addr. If not, then call
cec_phys_addr_invalidate. This helps the CEC framework differentiate
between CEC_PHYS_ADDR_INVALID due to no HPD/EDID, or due to an EDID without
a Physical Address.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/i2c/adv7511-v4l2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv7511-v4l2.c b/drivers/media/i2c/adv7511-v4l2.c
index 27e0e2c31dbd..1cc87049c6be 100644
--- a/drivers/media/i2c/adv7511-v4l2.c
+++ b/drivers/media/i2c/adv7511-v4l2.c
@@ -1466,7 +1466,7 @@ static void adv7511_notify_no_edid(struct v4l2_subdev *sd)
 	ed.present = false;
 	ed.segment = adv7511_rd(sd, 0xc4);
 	ed.phys_addr = CEC_PHYS_ADDR_INVALID;
-	cec_s_phys_addr(state->cec_adap, ed.phys_addr, false);
+	cec_phys_addr_invalidate(state->cec_adap);
 	v4l2_subdev_notify(sd, ADV7511_EDID_DETECT, (void *)&ed);
 	v4l2_ctrl_s_ctrl(state->have_edid0_ctrl, 0x0);
 }
-- 
2.53.0

[PATCHv3 05/13] media: cec: extron-da-hd-4k-plus: call cec_s_phys_addr only if HPD is high

[Hans Verkuil]

If the HPD is high, then call cec_s_phys_addr. If not, then call
cec_phys_addr_invalidate. This helps the CEC framework differentiate
between CEC_PHYS_ADDR_INVALID due to no HPD/EDID, or due to an EDID without
a Physical Address.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 .../cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c   | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c b/drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c
index 1f8ed7ff31e5..ae3628767ac5 100644
--- a/drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c
+++ b/drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c
@@ -643,8 +643,12 @@ static void extron_irq_work_handler(struct work_struct *work)
 			extron_read_edid(port);
 		}
 	}
-	if (update_pa)
-		cec_s_phys_addr(port->adap, pa, false);
+	if (update_pa || update_has_edid) {
+		if (has_edid)
+			cec_s_phys_addr(port->adap, pa, false);
+		else
+			cec_phys_addr_invalidate(port->adap);
+	}
 }
 
 static void extron_process_received(struct extron_port *port, const char *data)
-- 
2.53.0

[PATCHv3 06/13] media: cec: core: improve cec_notifier_phys_addr_invalidate

[Hans Verkuil]

cec_notifier_phys_addr_invalidate must call cec_phys_addr_invalidate
instead of cec_s_phys_addr. This ensures that the CEC framework knowns
that there is no HPD.

Also update cec_notifier_cec_adap_register(): it has to remember if
the last set physical address was from an EDID or because there was
a disconnect.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/cec/core/cec-notifier.c | 26 +++++++++++++++++++--
 include/media/cec-notifier.h          | 33 +++++++++++++++------------
 2 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/drivers/media/cec/core/cec-notifier.c b/drivers/media/cec/core/cec-notifier.c
index 8cf1474d0361..5a4588487ea3 100644
--- a/drivers/media/cec/core/cec-notifier.c
+++ b/drivers/media/cec/core/cec-notifier.c
@@ -29,6 +29,7 @@ struct cec_notifier {
 	struct cec_adapter *cec_adap;
 
 	u16 phys_addr;
+	bool edid_present;
 };
 
 static LIST_HEAD(cec_notifiers);
@@ -113,6 +114,7 @@ cec_notifier_conn_register(struct device *hdmi_dev, const char *port_name,
 
 	mutex_lock(&n->lock);
 	n->phys_addr = CEC_PHYS_ADDR_INVALID;
+	n->edid_present = false;
 	if (conn_info)
 		n->conn_info = *conn_info;
 	else
@@ -135,6 +137,7 @@ void cec_notifier_conn_unregister(struct cec_notifier *n)
 	mutex_lock(&n->lock);
 	memset(&n->conn_info, 0, sizeof(n->conn_info));
 	n->phys_addr = CEC_PHYS_ADDR_INVALID;
+	n->edid_present = false;
 	if (n->cec_adap) {
 		if (!n->cec_adap->adap_controls_phys_addr)
 			cec_phys_addr_invalidate(n->cec_adap);
@@ -162,8 +165,12 @@ cec_notifier_cec_adap_register(struct device *hdmi_dev, const char *port_name,
 	n->cec_adap = adap;
 	adap->conn_info = n->conn_info;
 	adap->notifier = n;
-	if (!adap->adap_controls_phys_addr)
-		cec_s_phys_addr(adap, n->phys_addr, false);
+	if (!adap->adap_controls_phys_addr) {
+		if (n->phys_addr == CEC_PHYS_ADDR_INVALID && !n->edid_present)
+			cec_phys_addr_invalidate(adap);
+		else
+			cec_s_phys_addr(adap, n->phys_addr, false);
+	}
 	mutex_unlock(&n->lock);
 	return n;
 }
@@ -190,6 +197,7 @@ void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa)
 
 	mutex_lock(&n->lock);
 	n->phys_addr = pa;
+	n->edid_present = true;
 	if (n->cec_adap && !n->cec_adap->adap_controls_phys_addr)
 		cec_s_phys_addr(n->cec_adap, n->phys_addr, false);
 	mutex_unlock(&n->lock);
@@ -216,6 +224,20 @@ void cec_notifier_set_phys_addr_from_edid(struct cec_notifier *n,
 }
 EXPORT_SYMBOL_GPL(cec_notifier_set_phys_addr_from_edid);
 
+void cec_notifier_phys_addr_invalidate(struct cec_notifier *n)
+{
+	if (!n)
+		return;
+
+	mutex_lock(&n->lock);
+	n->phys_addr = CEC_PHYS_ADDR_INVALID;
+	n->edid_present = false;
+	if (n->cec_adap && !n->cec_adap->adap_controls_phys_addr)
+		cec_phys_addr_invalidate(n->cec_adap);
+	mutex_unlock(&n->lock);
+}
+EXPORT_SYMBOL_GPL(cec_notifier_phys_addr_invalidate);
+
 struct device *cec_notifier_parse_hdmi_phandle(struct device *dev)
 {
 	struct platform_device *hdmi_pdev;
diff --git a/include/media/cec-notifier.h b/include/media/cec-notifier.h
index b1c839734124..15e9bcb5b887 100644
--- a/include/media/cec-notifier.h
+++ b/include/media/cec-notifier.h
@@ -80,7 +80,10 @@ void cec_notifier_cec_adap_unregister(struct cec_notifier *n,
  * @n: the CEC notifier
  * @pa: the CEC physical address
  *
- * Set a new CEC physical address.
+ * Set a new CEC physical address. @pa may be f.f.f.f, indicating that
+ * the EDID does not contain a valid Physical Address.
+ * Use @cec_notifier_phys_addr_invalidate() when there is no EDID at all,
+ * i.e. there is no video sink connected.
  * Does nothing if @n == NULL.
  */
 void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa);
@@ -96,6 +99,17 @@ void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa);
 void cec_notifier_set_phys_addr_from_edid(struct cec_notifier *n,
 					  const struct edid *edid);
 
+/**
+ * cec_notifier_phys_addr_invalidate() - set the physical address to INVALID
+ *
+ * @n: the CEC notifier
+ *
+ * This is a simple helper function to invalidate the physical
+ * address when the video sink disappears.
+ * Does nothing if @n == NULL.
+ */
+void cec_notifier_phys_addr_invalidate(struct cec_notifier *n);
+
 /**
  * cec_notifier_parse_hdmi_phandle - find the hdmi device from "hdmi-phandle"
  * @dev: the device with the "hdmi-phandle" device tree property
@@ -143,6 +157,10 @@ static inline void cec_notifier_set_phys_addr_from_edid(struct cec_notifier *n,
 {
 }
 
+static inline void cec_notifier_phys_addr_invalidate(struct cec_notifier *n)
+{
+}
+
 static inline struct device *cec_notifier_parse_hdmi_phandle(struct device *dev)
 {
 	return ERR_PTR(-ENODEV);
@@ -150,17 +168,4 @@ static inline struct device *cec_notifier_parse_hdmi_phandle(struct device *dev)
 
 #endif
 
-/**
- * cec_notifier_phys_addr_invalidate() - set the physical address to INVALID
- *
- * @n: the CEC notifier
- *
- * This is a simple helper function to invalidate the physical
- * address. Does nothing if @n == NULL.
- */
-static inline void cec_notifier_phys_addr_invalidate(struct cec_notifier *n)
-{
-	cec_notifier_set_phys_addr(n, CEC_PHYS_ADDR_INVALID);
-}
-
 #endif
-- 
2.53.0

Re: [PATCHv3 06/13] media: cec: core: improve cec_notifier_phys_addr_invalidate

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] media: cec: no-op HPD distinction in cec_notifier_phys_addr_invalidate()
- [Low] media: cec: invalid @ prefix on cec_notifier_phys_addr_invalidate() doc ref
--

commit 6215e9e4297c5764a7d09898f00380f708aab802
Author: Hans Verkuil <hverkuil+cisco@kernel.org>

media: cec: core: improve cec_notifier_phys_addr_invalidate

This commit moves cec_notifier_phys_addr_invalidate() into cec-notifier.c
so it calls cec_phys_addr_invalidate() instead of cec_s_phys_addr(), and
tracks edid_present in struct cec_notifier so cec_notifier_cec_adap_register()
can distinguish between a disconnected sink and an EDID without a valid
physical address.

> diff --git a/drivers/media/cec/core/cec-notifier.c b/drivers/media/cec/core/cec-notifier.c
> index 8cf1474d0361c..5a4588487ea34 100644
> --- a/drivers/media/cec/core/cec-notifier.c
> +++ b/drivers/media/cec/core/cec-notifier.c

[ ... ]

> @@ -162,8 +165,12 @@ cec_notifier_cec_adap_register(struct device *hdmi_dev, const char *port_name,
>  	n->cec_adap = adap;
>  	adap->conn_info = n->conn_info;
>  	adap->notifier = n;
> -	if (!adap->adap_controls_phys_addr)
> -		cec_s_phys_addr(adap, n->phys_addr, false);
> +	if (!adap->adap_controls_phys_addr) {
> +		if (n->phys_addr == CEC_PHYS_ADDR_INVALID && !n->edid_present)
> +			cec_phys_addr_invalidate(adap);
> +		else
> +			cec_s_phys_addr(adap, n->phys_addr, false);
> +	}
>  	mutex_unlock(&n->lock);
>  	return n;
>  }
> @@ -190,6 +197,7 @@ void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa)
>  
>  	mutex_lock(&n->lock);
>  	n->phys_addr = pa;
> +	n->edid_present = true;
>  	if (n->cec_adap && !n->cec_adap->adap_controls_phys_addr)
>  		cec_s_phys_addr(n->cec_adap, n->phys_addr, false);
>  	mutex_unlock(&n->lock);
> @@ -216,6 +224,20 @@ void cec_notifier_set_phys_addr_from_edid(struct cec_notifier *n,
>  }
>  EXPORT_SYMBOL_GPL(cec_notifier_set_phys_addr_from_edid);
>  
> +void cec_notifier_phys_addr_invalidate(struct cec_notifier *n)
> +{
> +	if (!n)
> +		return;
> +
> +	mutex_lock(&n->lock);
> +	n->phys_addr = CEC_PHYS_ADDR_INVALID;
> +	n->edid_present = false;
> +	if (n->cec_adap && !n->cec_adap->adap_controls_phys_addr)
> +		cec_phys_addr_invalidate(n->cec_adap);
> +	mutex_unlock(&n->lock);
> +}
> +EXPORT_SYMBOL_GPL(cec_notifier_phys_addr_invalidate);

[Severity: Medium]
Does calling cec_phys_addr_invalidate() instead of cec_s_phys_addr()
actually allow the CEC framework to distinguish whether HPD or an EDID is
present?

In drivers/media/cec/core/cec-notifier.c, cec_notifier_cec_adap_register()
and cec_notifier_phys_addr_invalidate() use n->edid_present to choose
between calling cec_phys_addr_invalidate(adap) and
cec_s_phys_addr(adap, CEC_PHYS_ADDR_INVALID, false).

However, in include/media/cec.h, cec_phys_addr_invalidate() is defined as:

static inline void cec_phys_addr_invalidate(struct cec_adapter *adap)
{
	cec_s_phys_addr(adap, CEC_PHYS_ADDR_INVALID, false);
}

Because cec_phys_addr_invalidate() simply calls
cec_s_phys_addr(adap, CEC_PHYS_ADDR_INVALID, false), both branches execute
the exact same call and n->edid_present has no observable effect on the
CEC core. Is a separate change to cec_phys_addr_invalidate() missing?

> diff --git a/include/media/cec-notifier.h b/include/media/cec-notifier.h
> index b1c8397341249..15e9bcb5b8879 100644
> --- a/include/media/cec-notifier.h
> +++ b/include/media/cec-notifier.h
> @@ -80,7 +80,10 @@ void cec_notifier_cec_adap_unregister(struct cec_notifier *n,
>   * @n: the CEC notifier
>   * @pa: the CEC physical address
>   *
> - * Set a new CEC physical address.
> + * Set a new CEC physical address. @pa may be f.f.f.f, indicating that
> + * the EDID does not contain a valid Physical Address.
> + * Use @cec_notifier_phys_addr_invalidate() when there is no EDID at all,
> + * i.e. there is no video sink connected.
>   * Does nothing if @n == NULL.
>   */
>  void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa);

[Severity: Low]
Should @cec_notifier_phys_addr_invalidate() in the kernel-doc comment for
cec_notifier_set_phys_addr() in include/media/cec-notifier.h be written as
cec_notifier_phys_addr_invalidate() without the leading @?

When building documentation via Documentation/driver-api/media/cec-core.rst,
the @ prefix causes kernel-doc (RestFormat in
tools/lib/python/kdoc/kdoc_output.py) to match the reference as a
function-pointer parameter (type_fp_param) and escape the parentheses,
which prevents Sphinx from generating a function cross-reference link.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/e03c6b786e243e6a0062c61a77ce478dd561c27b.1793863347.git.hverkuil+cisco@kernel.org?part=1

[PATCHv3 07/13] media: uapi: cec.h: add CEC_CAP_HAS_HPD_PRESENT/CEC_LOG_ADDRS_FL_HPD_PRESENT

[Hans Verkuil]

Add a capability and a new flag that is used to indicate if a
HPD is present.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 include/uapi/linux/cec.h | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/include/uapi/linux/cec.h b/include/uapi/linux/cec.h
index fdfc97a6e4ec..b1b5f4748583 100644
--- a/include/uapi/linux/cec.h
+++ b/include/uapi/linux/cec.h
@@ -325,27 +325,27 @@ static inline int cec_is_unconfigured(__u16 log_addr_mask)
 #define CEC_MODE_FOLLOWER_MSK		0xf0
 
 /* Userspace has to configure the physical address */
-#define CEC_CAP_PHYS_ADDR	(1 << 0)
+#define CEC_CAP_PHYS_ADDR		(1 << 0)
 /* Userspace has to configure the logical addresses */
-#define CEC_CAP_LOG_ADDRS	(1 << 1)
+#define CEC_CAP_LOG_ADDRS		(1 << 1)
 /* Userspace can transmit messages (and thus become follower as well) */
-#define CEC_CAP_TRANSMIT	(1 << 2)
-/*
- * Passthrough all messages instead of processing them.
- */
-#define CEC_CAP_PASSTHROUGH	(1 << 3)
+#define CEC_CAP_TRANSMIT		(1 << 2)
+/* Passthrough all messages instead of processing them. */
+#define CEC_CAP_PASSTHROUGH		(1 << 3)
 /* Supports remote control */
-#define CEC_CAP_RC		(1 << 4)
+#define CEC_CAP_RC			(1 << 4)
 /* Hardware can monitor all messages, not just directed and broadcast. */
-#define CEC_CAP_MONITOR_ALL	(1 << 5)
+#define CEC_CAP_MONITOR_ALL		(1 << 5)
 /* Hardware can use CEC only if the HDMI HPD pin is high. */
-#define CEC_CAP_NEEDS_HPD	(1 << 6)
+#define CEC_CAP_NEEDS_HPD		(1 << 6)
 /* Hardware can monitor CEC pin transitions */
-#define CEC_CAP_MONITOR_PIN	(1 << 7)
+#define CEC_CAP_MONITOR_PIN		(1 << 7)
 /* CEC_ADAP_G_CONNECTOR_INFO is available */
-#define CEC_CAP_CONNECTOR_INFO	(1 << 8)
+#define CEC_CAP_CONNECTOR_INFO		(1 << 8)
 /* CEC_MSG_FL_REPLY_VENDOR_ID is available */
-#define CEC_CAP_REPLY_VENDOR_ID	(1 << 9)
+#define CEC_CAP_REPLY_VENDOR_ID		(1 << 9)
+/* CEC_LOG_ADDRS_FL_HPD_PRESENT is available */
+#define CEC_CAP_HAS_HPD_PRESENT	(1 << 10)
 
 /**
  * struct cec_caps - CEC capabilities structure.
@@ -405,6 +405,8 @@ struct cec_log_addrs {
 #define CEC_LOG_ADDRS_FL_CDC_ONLY		(1 << 2)
 /* Configuration failed */
 #define CEC_LOG_ADDRS_FL_CONFIG_FAILED		(1 << 3)
+/* EDID is present */
+#define CEC_LOG_ADDRS_FL_HPD_PRESENT		(1 << 4)
 
 /**
  * struct cec_drm_connector_info - tells which drm connector is
-- 
2.53.0

Re: [PATCHv3 07/13] media: uapi: cec.h: add CEC_CAP_HAS_HPD_PRESENT/CEC_LOG_ADDRS_FL_HPD_PRESENT

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] media: uapi: cec.h: unimplemented and undocumented HPD_PRESENT UAPI flags
- [Low] media: uapi: cec.h: mismatched EDID comment on CEC_LOG_ADDRS_FL_HPD_PRESENT
--

commit 2490741b0b49047176c24c5b5fcdf74889b9d347
Author: Hans Verkuil <hverkuil+cisco@kernel.org>

media: uapi: cec.h: add CEC_CAP_HAS_HPD_PRESENT/CEC_LOG_ADDRS_FL_HPD_PRESENT

This commit adds the CEC_CAP_HAS_HPD_PRESENT capability and the
CEC_LOG_ADDRS_FL_HPD_PRESENT logical address flag to include/uapi/linux/cec.h
to indicate whether HPD is present.

> diff --git a/include/uapi/linux/cec.h b/include/uapi/linux/cec.h
> index fdfc97a6e4ec0..b1b5f47485836 100644
> --- a/include/uapi/linux/cec.h
> +++ b/include/uapi/linux/cec.h
> @@ -325,27 +325,27 @@ static inline int cec_is_unconfigured(__u16 log_addr_mask)
>  #define CEC_MODE_FOLLOWER_MSK		0xf0
>  
>  /* Userspace has to configure the physical address */
> -#define CEC_CAP_PHYS_ADDR	(1 << 0)
> +#define CEC_CAP_PHYS_ADDR		(1 << 0)
>  /* Userspace has to configure the logical addresses */
> -#define CEC_CAP_LOG_ADDRS	(1 << 1)
> +#define CEC_CAP_LOG_ADDRS		(1 << 1)
>  /* Userspace can transmit messages (and thus become follower as well) */
> -#define CEC_CAP_TRANSMIT	(1 << 2)
> -/*
> - * Passthrough all messages instead of processing them.
> - */
> -#define CEC_CAP_PASSTHROUGH	(1 << 3)
> +#define CEC_CAP_TRANSMIT		(1 << 2)
> +/* Passthrough all messages instead of processing them. */
> +#define CEC_CAP_PASSTHROUGH		(1 << 3)
>  /* Supports remote control */
> -#define CEC_CAP_RC		(1 << 4)
> +#define CEC_CAP_RC			(1 << 4)
>  /* Hardware can monitor all messages, not just directed and broadcast. */
> -#define CEC_CAP_MONITOR_ALL	(1 << 5)
> +#define CEC_CAP_MONITOR_ALL		(1 << 5)
>  /* Hardware can use CEC only if the HDMI HPD pin is high. */
> -#define CEC_CAP_NEEDS_HPD	(1 << 6)
> +#define CEC_CAP_NEEDS_HPD		(1 << 6)
>  /* Hardware can monitor CEC pin transitions */
> -#define CEC_CAP_MONITOR_PIN	(1 << 7)
> +#define CEC_CAP_MONITOR_PIN		(1 << 7)
>  /* CEC_ADAP_G_CONNECTOR_INFO is available */
> -#define CEC_CAP_CONNECTOR_INFO	(1 << 8)
> +#define CEC_CAP_CONNECTOR_INFO		(1 << 8)
>  /* CEC_MSG_FL_REPLY_VENDOR_ID is available */
> -#define CEC_CAP_REPLY_VENDOR_ID	(1 << 9)
> +#define CEC_CAP_REPLY_VENDOR_ID		(1 << 9)
> +/* CEC_LOG_ADDRS_FL_HPD_PRESENT is available */
> +#define CEC_CAP_HAS_HPD_PRESENT	(1 << 10)
>  
>  /**
>   * struct cec_caps - CEC capabilities structure.
> @@ -405,6 +405,8 @@ struct cec_log_addrs {
>  #define CEC_LOG_ADDRS_FL_CDC_ONLY		(1 << 2)
>  /* Configuration failed */
>  #define CEC_LOG_ADDRS_FL_CONFIG_FAILED		(1 << 3)
> +/* EDID is present */
> +#define CEC_LOG_ADDRS_FL_HPD_PRESENT		(1 << 4)

[Severity: Medium]
Are CEC_CAP_HAS_HPD_PRESENT and CEC_LOG_ADDRS_FL_HPD_PRESENT missing their
kernel implementation and documentation?

Neither symbol is referenced anywhere in the kernel tree or documented in
Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst and
Documentation/userspace-api/media/cec/cec-ioc-adap-g-log-addrs.rst.

When userspace calls the CEC_ADAP_S_LOG_ADDRS ioctl, cec_adap_s_log_addrs()
strips CEC_LOG_ADDRS_FL_HPD_PRESENT before calling __cec_s_log_addrs():

drivers/media/cec/core/cec-api.c:cec_adap_s_log_addrs() {
	...
	log_addrs.flags &= CEC_LOG_ADDRS_FL_ALLOW_UNREG_FALLBACK |
			   CEC_LOG_ADDRS_FL_ALLOW_RC_PASSTHRU |
			   CEC_LOG_ADDRS_FL_CDC_ONLY;
	mutex_lock(&adap->lock);
	if (!adap->is_claiming_log_addrs && !adap->is_configuring &&
	    (!log_addrs.num_log_addrs || !adap->is_configured) &&
	    !cec_is_busy(adap, fh)) {
		err = __cec_s_log_addrs(adap, &log_addrs, block);
	...
}

Nothing in the CEC core or drivers ever sets or checks
CEC_CAP_HAS_HPD_PRESENT in adap->capabilities or CEC_LOG_ADDRS_FL_HPD_PRESENT
in adap->log_addrs.flags. Should these UAPI definitions be introduced
together with the kernel code and documentation that use them?

[Severity: Low]
Is the comment "EDID is present" above CEC_LOG_ADDRS_FL_HPD_PRESENT intended?
The macro name and commit message both describe indicating whether HPD is
present rather than EDID.

Also, this isn't a bug, but the patch re-indents all existing CEC_CAP_*
definitions with an extra tab to align their values, while the new
CEC_CAP_HAS_HPD_PRESENT uses a single tab and leaves `(1 << 10)` unaligned
with the rest of the block.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/4cd2dd652db11b7410a6f9608c5c15bef45f5df5.1793863347.git.hverkuil+cisco@kernel.org?part=1

[PATCHv3 08/13] Documentation: media: document HPD_PRESENT cap and flag.

[Hans Verkuil]

Document CEC_CAP_HAS_HPD_PRESENT and CEC_LOG_ADDRS_FL_HPD_PRESENT.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 .../userspace-api/media/cec/cec-ioc-adap-g-caps.rst    | 10 ++++++++++
 .../media/cec/cec-ioc-adap-g-log-addrs.rst             | 10 ++++++++++
 2 files changed, 20 insertions(+)

diff --git a/Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst b/Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst
index 1d5248979a6d..bf734cbee0dd 100644
--- a/Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst
+++ b/Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst
@@ -143,6 +143,16 @@ returns the information to the application. The ioctl never fails.
       - 0x00000200
       - If this capability is set, then
         :ref:`CEC_MSG_FL_REPLY_VENDOR_ID <cec-msg-flags>` can be used.
+    * .. _`CEC-CAP-HAS-HPD-PRESENT`:
+
+      - ``CEC_CAP_HAS_HPD_PRESENT``
+      - 0x00000400
+      - If this capability is set, then
+        :ref:`CEC_LOG_ADDRS_FL_HPD_PRESENT <cec-log-addrs-flags>` is valid.
+	This makes it possible to tell apart the case where the Physical Address
+	is f.f.f.f because no HPD (Hotplug Detect) is present, and the case
+	where it is f.f.f.f because the EDID does not provide a valid Physical
+	Address.
 
 Return Value
 ============
diff --git a/Documentation/userspace-api/media/cec/cec-ioc-adap-g-log-addrs.rst b/Documentation/userspace-api/media/cec/cec-ioc-adap-g-log-addrs.rst
index 8397bf573798..e37898750ab5 100644
--- a/Documentation/userspace-api/media/cec/cec-ioc-adap-g-log-addrs.rst
+++ b/Documentation/userspace-api/media/cec/cec-ioc-adap-g-log-addrs.rst
@@ -192,6 +192,16 @@ logical address types are already defined will return with error ``EBUSY``.
       - If this flag is set, then the CEC device failed to claim a free logical
         address and is in the unconfigured state. This can never happen if
 	``CEC_LOG_ADDRS_FL_ALLOW_UNREG_FALLBACK`` was set as well.
+    * .. _`CEC-LOG-ADDRS-FL-HPD-PRESENT`:
+
+      - ``CEC_LOG_ADDRS_FL_HPD_PRESENT``
+      - 16
+      - If this flag is set, then the HPD (Hotplug Detect) is present. This
+	 helps tell apart the case where the Physical Address is f.f.f.f
+	 because no HPD is present, and the case where it is f.f.f.f because the
+	 EDID does not provide a valid Physical Address. This flag can only be
+	 used if ``CEC_CAP_HAS_HPD_PRESENT`` is set in the adapter
+	 capabilities.
 
 .. tabularcolumns:: |p{7.8cm}|p{1.0cm}|p{8.5cm}|
 
-- 
2.53.0

Re: [PATCHv3 08/13] Documentation: media: document HPD_PRESENT cap and flag.

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] media: cec: docs: CEC_CAP_HAS_HPD_PRESENT and flag missing from uAPI header
- [Low] media: cec: docs: malformed rST indentation in CEC_LOG_ADDRS_FL_HPD_PRESENT
--

commit 53c76b7e379d8741e299203dcf5382f4b9c83f39
Author: Hans Verkuil <hverkuil+cisco@kernel.org>

Documentation: media: document HPD_PRESENT cap and flag.

This commit documents the CEC_CAP_HAS_HPD_PRESENT capability and the
CEC_LOG_ADDRS_FL_HPD_PRESENT logical address flag in the CEC userspace API
documentation.

> diff --git a/Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst b/Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst
> index 1d5248979a6da..bf734cbee0dda 100644
> --- a/Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst
> +++ b/Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst
> @@ -143,6 +143,16 @@ returns the information to the application. The ioctl never fails.
>        - 0x00000200
>        - If this capability is set, then
>          :ref:`CEC_MSG_FL_REPLY_VENDOR_ID <cec-msg-flags>` can be used.
> +    * .. _`CEC-CAP-HAS-HPD-PRESENT`:
> +
> +      - ``CEC_CAP_HAS_HPD_PRESENT``
> +      - 0x00000400
> +      - If this capability is set, then
> +        :ref:`CEC_LOG_ADDRS_FL_HPD_PRESENT <cec-log-addrs-flags>` is valid.
> +	This makes it possible to tell apart the case where the Physical Address
> +	is f.f.f.f because no HPD (Hotplug Detect) is present, and the case
> +	where it is f.f.f.f because the EDID does not provide a valid Physical
> +	Address.

[Severity: Medium]
Are CEC_CAP_HAS_HPD_PRESENT and CEC_LOG_ADDRS_FL_HPD_PRESENT missing from
include/uapi/linux/cec.h?

In include/uapi/linux/cec.h, the capability definitions currently end at
CEC_CAP_REPLY_VENDOR_ID:

    /* CEC_MSG_FL_REPLY_VENDOR_ID is available */
    #define CEC_CAP_REPLY_VENDOR_ID	(1 << 9)

and the logical address flags end at CEC_LOG_ADDRS_FL_CONFIG_FAILED:

    /* Configuration failed */
    #define CEC_LOG_ADDRS_FL_CONFIG_FAILED		(1 << 3)

Neither constant appears to be defined in include/uapi/linux/cec.h or used in
drivers/media/cec/, so userspace applications consulting this documentation
cannot compile with these flags.

> diff --git a/Documentation/userspace-api/media/cec/cec-ioc-adap-g-log-addrs.rst b/Documentation/userspace-api/media/cec/cec-ioc-adap-g-log-addrs.rst
> index 8397bf5737988..e37898750ab5d 100644
> --- a/Documentation/userspace-api/media/cec/cec-ioc-adap-g-log-addrs.rst
> +++ b/Documentation/userspace-api/media/cec/cec-ioc-adap-g-log-addrs.rst
> @@ -192,6 +192,16 @@ logical address types are already defined will return with error ``EBUSY``.
>        - If this flag is set, then the CEC device failed to claim a free logical
>          address and is in the unconfigured state. This can never happen if
>  	``CEC_LOG_ADDRS_FL_ALLOW_UNREG_FALLBACK`` was set as well.
> +    * .. _`CEC-LOG-ADDRS-FL-HPD-PRESENT`:
> +
> +      - ``CEC_LOG_ADDRS_FL_HPD_PRESENT``
> +      - 16
> +      - If this flag is set, then the HPD (Hotplug Detect) is present. This
> +	 helps tell apart the case where the Physical Address is f.f.f.f
> +	 because no HPD is present, and the case where it is f.f.f.f because the
> +	 EDID does not provide a valid Physical Address. This flag can only be
> +	 used if ``CEC_CAP_HAS_HPD_PRESENT`` is set in the adapter
> +	 capabilities.

[Severity: Low]
Will Sphinx misparse the CEC_LOG_ADDRS_FL_HPD_PRESENT description as a
definition list when building htmldocs?

The first line of the bullet item starts at column 8 (six spaces, a hyphen,
and a space), whereas the continuation lines are indented with a tab plus an
extra space, placing them at column 9. In reStructuredText, indenting
continuation lines relative to the first line without a blank line causes
the first line to be rendered as a bold definition term and the remaining
lines as an indented definition block instead of a single paragraph.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/e98308c0db382c09ce698b34ca54f798ab0e3557.1793863347.git.hverkuil+cisco@kernel.org?part=1

[PATCHv3 09/13] media: cec: core: add support for HPD_PRESENT

[Hans Verkuil]

There can be two reasons for the CEC Physical Address to be invalid:
either there is no HPD, or there is an EDID, but it does not contain
a Physical Address, or the Physical Address is set to f.f.f.f in the EDID.

For debugging it is very useful to know why the PA is invalid.

Add support for reporting this to userspace by changing the behavior of
two CEC functions: cec_s_phys_addr is called with the PA read from the
EDID (or CEC_PHYS_ADDR_INVALID if the EDID has no PA or the PA is f.f.f.f),
and cec_phys_addr_invalidate is only called if the HPD goes low, so the
EDID is gone.

The first function sets the new CEC_LOG_ADDRS_FL_HPD_PRESENT flag, and
the second function removes it.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/cec/core/cec-adap.c | 49 ++++++++++++++++++++++++++-----
 drivers/media/cec/core/cec-api.c  |  3 +-
 drivers/media/cec/core/cec-core.c |  4 ++-
 drivers/media/cec/core/cec-priv.h |  3 +-
 include/media/cec.h               | 27 +++++++++--------
 5 files changed, 63 insertions(+), 23 deletions(-)

diff --git a/drivers/media/cec/core/cec-adap.c b/drivers/media/cec/core/cec-adap.c
index ee8a200b9803..55b2e711fe77 100644
--- a/drivers/media/cec/core/cec-adap.c
+++ b/drivers/media/cec/core/cec-adap.c
@@ -1727,18 +1727,35 @@ int cec_adap_enable(struct cec_adapter *adap)
  *
  * This function is called with adap->lock held.
  */
-void __cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block)
+void __cec_s_phys_addr(struct cec_adapter *adap, bool hpd_present,
+		       u16 phys_addr, bool block)
 {
 	bool becomes_invalid = phys_addr == CEC_PHYS_ADDR_INVALID;
 	bool is_invalid = adap->phys_addr == CEC_PHYS_ADDR_INVALID;
+	bool hpd_was_present =
+		adap->log_addrs.flags & CEC_LOG_ADDRS_FL_HPD_PRESENT;
 
-	if (phys_addr == adap->phys_addr)
-		return;
+	if (adap->capabilities & CEC_CAP_HAS_HPD_PRESENT) {
+		if (hpd_present)
+			adap->log_addrs.flags |= CEC_LOG_ADDRS_FL_HPD_PRESENT;
+		else
+			adap->log_addrs.flags &= ~CEC_LOG_ADDRS_FL_HPD_PRESENT;
+	} else {
+		hpd_present = false;
+	}
 	if (!becomes_invalid && adap->devnode.unregistered)
 		return;
+	if (phys_addr == adap->phys_addr) {
+		if (hpd_was_present != hpd_present) {
+			dprintk(1, "HPD is %spresent\n",
+				hpd_present ? "" : "not ");
+			cec_post_state_event(adap);
+		}
+		return;
+	}
 
-	dprintk(1, "new physical address %x.%x.%x.%x\n",
-		cec_phys_addr_exp(phys_addr));
+	dprintk(1, "new physical address %x.%x.%x.%x, HPD is %spresent\n",
+		cec_phys_addr_exp(phys_addr), hpd_present ? "" : "not ");
 	if (becomes_invalid || !is_invalid) {
 		adap->phys_addr = CEC_PHYS_ADDR_INVALID;
 		cec_adap_unconfigure(adap);
@@ -1767,11 +1784,22 @@ void cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block)
 		return;
 
 	mutex_lock(&adap->lock);
-	__cec_s_phys_addr(adap, phys_addr, block);
+	__cec_s_phys_addr(adap, true, phys_addr, block);
 	mutex_unlock(&adap->lock);
 }
 EXPORT_SYMBOL_GPL(cec_s_phys_addr);
 
+void cec_phys_addr_invalidate(struct cec_adapter *adap)
+{
+	if (IS_ERR_OR_NULL(adap))
+		return;
+
+	mutex_lock(&adap->lock);
+	__cec_s_phys_addr(adap, false, CEC_PHYS_ADDR_INVALID, false);
+	mutex_unlock(&adap->lock);
+}
+EXPORT_SYMBOL_GPL(cec_phys_addr_invalidate);
+
 /*
  * Note: In the drm subsystem, prefer calling (if possible):
  *
@@ -1782,10 +1810,15 @@ void cec_s_phys_addr_from_edid(struct cec_adapter *adap,
 {
 	u16 pa = CEC_PHYS_ADDR_INVALID;
 
+	if (IS_ERR_OR_NULL(adap))
+		return;
+
 	if (edid && edid->extensions)
 		pa = cec_get_edid_phys_addr((const u8 *)edid,
 				EDID_LENGTH * (edid->extensions + 1), NULL);
-	cec_s_phys_addr(adap, pa, false);
+	mutex_lock(&adap->lock);
+	__cec_s_phys_addr(adap, edid, pa, false);
+	mutex_unlock(&adap->lock);
 }
 EXPORT_SYMBOL_GPL(cec_s_phys_addr_from_edid);
 
@@ -1969,6 +2002,8 @@ int __cec_s_log_addrs(struct cec_adapter *adap,
 	}
 
 	log_addrs->log_addr_mask = adap->log_addrs.log_addr_mask;
+	log_addrs->flags |=
+		adap->log_addrs.flags & CEC_LOG_ADDRS_FL_HPD_PRESENT;
 	adap->log_addrs = *log_addrs;
 	err = cec_adap_enable(adap);
 	if (!err && adap->phys_addr != CEC_PHYS_ADDR_INVALID)
diff --git a/drivers/media/cec/core/cec-api.c b/drivers/media/cec/core/cec-api.c
index a491d7fc8a71..1ba080d5a123 100644
--- a/drivers/media/cec/core/cec-api.c
+++ b/drivers/media/cec/core/cec-api.c
@@ -136,7 +136,8 @@ static long cec_adap_s_phys_addr(struct cec_adapter *adap, struct cec_fh *fh,
 	if (cec_is_busy(adap, fh))
 		err = -EBUSY;
 	else
-		__cec_s_phys_addr(adap, phys_addr, block);
+		__cec_s_phys_addr(adap, phys_addr != CEC_PHYS_ADDR_INVALID,
+				  phys_addr, block);
 	mutex_unlock(&adap->lock);
 	return err;
 }
diff --git a/drivers/media/cec/core/cec-core.c b/drivers/media/cec/core/cec-core.c
index 230f8a413e74..49589e71d240 100644
--- a/drivers/media/cec/core/cec-core.c
+++ b/drivers/media/cec/core/cec-core.c
@@ -178,7 +178,7 @@ static void cec_devnode_unregister(struct cec_adapter *adap)
 	mutex_unlock(&devnode->lock);
 
 	mutex_lock(&adap->lock);
-	__cec_s_phys_addr(adap, CEC_PHYS_ADDR_INVALID, false);
+	__cec_s_phys_addr(adap, false, CEC_PHYS_ADDR_INVALID, false);
 	__cec_s_log_addrs(adap, NULL, false);
 	// Disable the adapter (since adap->devnode.unregistered is true)
 	cec_adap_enable(adap);
@@ -279,6 +279,8 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops,
 	adap->log_addrs.cec_version = CEC_OP_CEC_VERSION_2_0;
 	adap->log_addrs.vendor_id = CEC_VENDOR_ID_NONE;
 	adap->capabilities = caps | CEC_CAP_REPLY_VENDOR_ID;
+	if (!(caps & CEC_CAP_PHYS_ADDR))
+		adap->capabilities |= CEC_CAP_HAS_HPD_PRESENT;
 	if (debug_phys_addr)
 		adap->capabilities |= CEC_CAP_PHYS_ADDR;
 	adap->needs_hpd = caps & CEC_CAP_NEEDS_HPD;
diff --git a/drivers/media/cec/core/cec-priv.h b/drivers/media/cec/core/cec-priv.h
index ce42a37c4ac0..c16792a30752 100644
--- a/drivers/media/cec/core/cec-priv.h
+++ b/drivers/media/cec/core/cec-priv.h
@@ -46,7 +46,8 @@ void cec_monitor_pin_cnt_dec(struct cec_adapter *adap);
 int cec_adap_status(struct seq_file *file, void *priv);
 int cec_thread_func(void *_adap);
 int cec_adap_enable(struct cec_adapter *adap);
-void __cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block);
+void __cec_s_phys_addr(struct cec_adapter *adap, bool hpd_present,
+		       u16 phys_addr, bool block);
 int __cec_s_log_addrs(struct cec_adapter *adap,
 		      struct cec_log_addrs *log_addrs, bool block);
 int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg,
diff --git a/include/media/cec.h b/include/media/cec.h
index 4c8b0950b377..7bb9ada696ca 100644
--- a/include/media/cec.h
+++ b/include/media/cec.h
@@ -381,6 +381,16 @@ void cec_s_conn_info(struct cec_adapter *adap,
 int cec_transmit_msg(struct cec_adapter *adap, struct cec_msg *msg,
 		     bool block);
 
+/**
+ * cec_phys_addr_invalidate() - set the physical address to INVALID
+ *
+ * @adap:	the CEC adapter
+ *
+ * This is a simple helper function to invalidate the physical
+ * address when the HPD is lost.
+ */
+void cec_phys_addr_invalidate(struct cec_adapter *adap);
+
 /* Called by the adapter */
 void cec_transmit_done_ts(struct cec_adapter *adap, u8 status,
 			  u8 arb_lost_cnt, u8 nack_cnt, u8 low_drive_cnt,
@@ -491,6 +501,10 @@ static inline void cec_s_phys_addr_from_edid(struct cec_adapter *adap,
 {
 }
 
+static inline void cec_phys_addr_invalidate(struct cec_adapter *adap)
+{
+}
+
 static inline u16 cec_get_edid_phys_addr(const u8 *edid, unsigned int size,
 					 unsigned int *offset)
 {
@@ -513,19 +527,6 @@ cec_fill_conn_info_from_drm(struct cec_connector_info *conn_info,
 
 #endif
 
-/**
- * cec_phys_addr_invalidate() - set the physical address to INVALID
- *
- * @adap:	the CEC adapter
- *
- * This is a simple helper function to invalidate the physical
- * address.
- */
-static inline void cec_phys_addr_invalidate(struct cec_adapter *adap)
-{
-	cec_s_phys_addr(adap, CEC_PHYS_ADDR_INVALID, false);
-}
-
 /**
  * cec_get_edid_spa_location() - find location of the Source Physical Address
  *
-- 
2.53.0

Re: [PATCHv3 09/13] media: cec: core: add support for HPD_PRESENT

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] media: cec: undeclared CEC_CAP_HAS_HPD_PRESENT and CEC_LOG_ADDRS_FL_HPD_PRESENT
- [Medium] media: cec: cec_s_phys_addr() falsely sets HPD_PRESENT via cec_notifier
--

commit 9869442bbf59dec664959906e52999513a57f708
Author: Hans Verkuil <hverkuil+cisco@kernel.org>

media: cec: core: add support for HPD_PRESENT

This commit adds support for tracking and reporting whether the HPD is
present via the CEC_LOG_ADDRS_FL_HPD_PRESENT flag and
CEC_CAP_HAS_HPD_PRESENT capability. It updates __cec_s_phys_addr(),
cec_s_phys_addr(), cec_s_phys_addr_from_edid(), and
cec_phys_addr_invalidate() to update this state.

> diff --git a/drivers/media/cec/core/cec-adap.c b/drivers/media/cec/core/cec-adap.c
> index ee8a200b9803f..55b2e711fe77f 100644
> --- a/drivers/media/cec/core/cec-adap.c
> +++ b/drivers/media/cec/core/cec-adap.c
> @@ -1727,18 +1727,35 @@ int cec_adap_enable(struct cec_adapter *adap)
>   *
>   * This function is called with adap->lock held.
>   */
> -void __cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block)
> +void __cec_s_phys_addr(struct cec_adapter *adap, bool hpd_present,
> +		       u16 phys_addr, bool block)
>  {
>  	bool becomes_invalid = phys_addr == CEC_PHYS_ADDR_INVALID;
>  	bool is_invalid = adap->phys_addr == CEC_PHYS_ADDR_INVALID;
> +	bool hpd_was_present =
> +		adap->log_addrs.flags & CEC_LOG_ADDRS_FL_HPD_PRESENT;
>  
> -	if (phys_addr == adap->phys_addr)
> -		return;
> +	if (adap->capabilities & CEC_CAP_HAS_HPD_PRESENT) {
> +		if (hpd_present)
> +			adap->log_addrs.flags |= CEC_LOG_ADDRS_FL_HPD_PRESENT;
> +		else
> +			adap->log_addrs.flags &= ~CEC_LOG_ADDRS_FL_HPD_PRESENT;
> +	} else {
> +		hpd_present = false;
> +	}

[Severity: Medium]
Was a hunk for include/uapi/linux/cec.h left out of this patch?

In drivers/media/cec/core/cec-adap.c, __cec_s_phys_addr() and
__cec_s_log_addrs() reference CEC_LOG_ADDRS_FL_HPD_PRESENT and
CEC_CAP_HAS_HPD_PRESENT, and cec_allocate_adapter() in
drivers/media/cec/core/cec-core.c also references CEC_CAP_HAS_HPD_PRESENT:

	if (!(caps & CEC_CAP_PHYS_ADDR))
		adap->capabilities |= CEC_CAP_HAS_HPD_PRESENT;

However, include/uapi/linux/cec.h is not updated by this commit and neither
macro is defined in the tree, causing the build to fail when CONFIG_CEC_CORE
is enabled.

>  	if (!becomes_invalid && adap->devnode.unregistered)
>  		return;
> +	if (phys_addr == adap->phys_addr) {
> +		if (hpd_was_present != hpd_present) {
> +			dprintk(1, "HPD is %spresent\n",
> +				hpd_present ? "" : "not ");
> +			cec_post_state_event(adap);
> +		}
> +		return;
> +	}
>  
> -	dprintk(1, "new physical address %x.%x.%x.%x\n",
> -		cec_phys_addr_exp(phys_addr));
> +	dprintk(1, "new physical address %x.%x.%x.%x, HPD is %spresent\n",
> +		cec_phys_addr_exp(phys_addr), hpd_present ? "" : "not ");
>  	if (becomes_invalid || !is_invalid) {
>  		adap->phys_addr = CEC_PHYS_ADDR_INVALID;
>  		cec_adap_unconfigure(adap);
> @@ -1767,11 +1784,22 @@ void cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block)
>  		return;
>  
>  	mutex_lock(&adap->lock);
> -	__cec_s_phys_addr(adap, phys_addr, block);
> +	__cec_s_phys_addr(adap, true, phys_addr, block);
>  	mutex_unlock(&adap->lock);
>  }
>  EXPORT_SYMBOL_GPL(cec_s_phys_addr);

[Severity: Medium]
Does passing hpd_present = true unconditionally in cec_s_phys_addr() cause
CEC_LOG_ADDRS_FL_HPD_PRESENT to be falsely set for adapters using
cec_notifier or invalidating the physical address via cec_s_phys_addr()?

In drivers/media/cec/core/cec-core.c, cec_allocate_adapter() sets
CEC_CAP_HAS_HPD_PRESENT on all adapters without CEC_CAP_PHYS_ADDR, which
includes cec_notifier-based drivers.

In drivers/media/cec/core/cec-notifier.c, cec_notifier_cec_adap_register()
initializes the adapter's physical address via cec_s_phys_addr():

	if (!adap->adap_controls_phys_addr)
		cec_s_phys_addr(adap, n->phys_addr, false);

When no sink is connected at registration time, n->phys_addr is
CEC_PHYS_ADDR_INVALID, so calling cec_s_phys_addr() sets
CEC_LOG_ADDRS_FL_HPD_PRESENT and posts a state change event even though HPD
is low.

Similarly, when HPD goes low, cec_notifier_phys_addr_invalidate() in
include/media/cec-notifier.h and cec_notifier_set_phys_addr_from_edid(n, NULL)
in drivers/media/cec/core/cec-notifier.c both route through
cec_notifier_set_phys_addr(n, CEC_PHYS_ADDR_INVALID):

	if (n->cec_adap && !n->cec_adap->adap_controls_phys_addr)
		cec_s_phys_addr(n->cec_adap, n->phys_addr, false);

Because this calls cec_s_phys_addr() rather than cec_phys_addr_invalidate(),
and other callers such as drm_dp_cec_attach() (when called from
drm_dp_cec_set_edid(aux, NULL)), hdmi4_bridge_hpd_notify(), and
adv7511_check_monitor_present_status() also pass CEC_PHYS_ADDR_INVALID to
cec_s_phys_addr() on disconnect, won't CEC_LOG_ADDRS_FL_HPD_PRESENT remain
set when HPD is lost?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/ae93656d52c429861504836a10c07b9eac3cd470.1793863347.git.hverkuil+cisco@kernel.org?part=1

[PATCHv3 10/13] media: uapi: cec.h: add CEC_EVENT_FL_STATE_HPD_PRESENT flag

[Hans Verkuil]

The CEC_EVENT_FL_STATE_HPD_PRESENT will report whether the HPD is
present or not.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 include/uapi/linux/cec.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/cec.h b/include/uapi/linux/cec.h
index b1b5f4748583..d595ad744431 100644
--- a/include/uapi/linux/cec.h
+++ b/include/uapi/linux/cec.h
@@ -455,6 +455,7 @@ struct cec_connector_info {
 
 #define CEC_EVENT_FL_INITIAL_STATE	(1 << 0)
 #define CEC_EVENT_FL_DROPPED_EVENTS	(1 << 1)
+#define CEC_EVENT_FL_STATE_HPD_PRESENT	(1 << 2)
 
 /**
  * struct cec_event_state_change - used when the CEC adapter changes state.
-- 
2.53.0

Re: [PATCHv3 10/13] media: uapi: cec.h: add CEC_EVENT_FL_STATE_HPD_PRESENT flag

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] media: cec: unimplemented and undocumented CEC_EVENT_FL_STATE_HPD_PRESENT flag
--

commit 6f18f1544823dafcf8a17b474c711348256f2340
Author: Hans Verkuil <hverkuil+cisco@kernel.org>

media: uapi: cec.h: add CEC_EVENT_FL_STATE_HPD_PRESENT flag

This commit adds the CEC_EVENT_FL_STATE_HPD_PRESENT flag to
include/uapi/linux/cec.h to report whether HPD is present.

> diff --git a/include/uapi/linux/cec.h b/include/uapi/linux/cec.h
> index fdfc97a6e4ec0..d9300a6010806 100644
> --- a/include/uapi/linux/cec.h
> +++ b/include/uapi/linux/cec.h
> @@ -453,6 +453,7 @@ struct cec_connector_info {
>  
>  #define CEC_EVENT_FL_INITIAL_STATE	(1 << 0)
>  #define CEC_EVENT_FL_DROPPED_EVENTS	(1 << 1)
> +#define CEC_EVENT_FL_STATE_HPD_PRESENT	(1 << 2)

[Severity: Medium]
The commit message states that CEC_EVENT_FL_STATE_HPD_PRESENT will report
whether HPD is present or not, but nothing in the kernel ever sets or
references this flag.

When userspace dequeues CEC_EVENT_STATE_CHANGE events via CEC_DQEVENT, the
events are populated in cec_post_state_event() in
drivers/media/cec/core/cec-adap.c:

	struct cec_event ev = {
		.event = CEC_EVENT_STATE_CHANGE,
	};

and on device open in cec_open() in drivers/media/cec/core/cec-api.c:

	struct cec_event ev = {
		.event = CEC_EVENT_STATE_CHANGE,
		.flags = CEC_EVENT_FL_INITIAL_STATE,
	};

Neither path sets CEC_EVENT_FL_STATE_HPD_PRESENT in ev.flags, so any
application checking ev.flags & CEC_EVENT_FL_STATE_HPD_PRESENT will always
observe 0.

In addition, Documentation/userspace-api/media/cec/cec-ioc-dqevent.rst
(under the CEC Event Flags table) and include/uapi/linux/cec.h do not
document CEC_EVENT_FL_STATE_HPD_PRESENT or specify which CEC_EVENT_* types
carry it.

Finally, without a corresponding capability flag or tri-state indicator
(unlike struct cec_event_state_change.have_conn_info, which is gated by
CEC_CAP_CONNECTOR_INFO), how can userspace distinguish between HPD being
low, a CEC driver that does not report HPD state, and an older kernel that
predates CEC_EVENT_FL_STATE_HPD_PRESENT? Should this UAPI definition be
held back until an in-kernel user, documentation, and capability signaling
are included?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/81a832c5568748e37f081d217998f80880ab8dc4.1793863347.git.hverkuil+cisco@kernel.org?part=1

[PATCHv3 11/13] Documentation: media: cec-ioc-dqevent.rst: document STATE_HPD_PRESENT flag

[Hans Verkuil]

Document the CEC_EVENT_FL_STATE_HPD_PRESENT event flag.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 .../userspace-api/media/cec/cec-ioc-adap-g-caps.rst    |  3 ++-
 .../userspace-api/media/cec/cec-ioc-dqevent.rst        | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst b/Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst
index bf734cbee0dd..0afb0e11fd72 100644
--- a/Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst
+++ b/Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst
@@ -148,7 +148,8 @@ returns the information to the application. The ioctl never fails.
       - ``CEC_CAP_HAS_HPD_PRESENT``
       - 0x00000400
       - If this capability is set, then
-        :ref:`CEC_LOG_ADDRS_FL_HPD_PRESENT <cec-log-addrs-flags>` is valid.
+        :ref:`CEC_LOG_ADDRS_FL_HPD_PRESENT <cec-log-addrs-flags>` and
+        :ref:`CEC_EVENT_FL_STATE_HPD_PRESENT <cec-event-flags>` are valid.
 	This makes it possible to tell apart the case where the Physical Address
 	is f.f.f.f because no HPD (Hotplug Detect) is present, and the case
 	where it is f.f.f.f because the EDID does not provide a valid Physical
diff --git a/Documentation/userspace-api/media/cec/cec-ioc-dqevent.rst b/Documentation/userspace-api/media/cec/cec-ioc-dqevent.rst
index 71d6a9e81f75..b399c5f1b847 100644
--- a/Documentation/userspace-api/media/cec/cec-ioc-dqevent.rst
+++ b/Documentation/userspace-api/media/cec/cec-ioc-dqevent.rst
@@ -224,6 +224,16 @@ it is guaranteed that the state did change in between the two events.
       - 2
       - Set if one or more events of the given event type have been dropped.
         This is an indication that the application cannot keep up.
+    * .. _`CEC-EVENT-FL-STATE-HPD-PRESENT`:
+
+      - ``CEC_EVENT_FL_STATE_HPD_PRESENT``
+      - 4
+      - Only set for the ``CEC_EVENT_STATE_CHANGE`` event type.
+        If this flag is set, then an HPD is present. This helps tell apart the
+        case where the Physical Address is f.f.f.f because no HPD (Hotplug
+        Detect) is present, and the case where it is f.f.f.f because the EDID
+        does not provide a valid Physical Address. This flag can only be used
+        if ``CEC_CAP_HAS_HPD_PRESENT`` is set in the adapter capabilities.
 
 
 Return Value
-- 
2.53.0

[PATCHv3 12/13] media: cec: core: implement CEC_EVENT_FL_STATE_HPD_PRESENT

[Hans Verkuil]

Set CEC_EVENT_FL_STATE_HPD_PRESENT if needed when posting the state
change event.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 drivers/media/cec/core/cec-adap.c |  2 ++
 drivers/media/cec/core/cec-api.c  | 10 ++++++----
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/media/cec/core/cec-adap.c b/drivers/media/cec/core/cec-adap.c
index 55b2e711fe77..fec7a58aff65 100644
--- a/drivers/media/cec/core/cec-adap.c
+++ b/drivers/media/cec/core/cec-adap.c
@@ -323,6 +323,8 @@ static void cec_post_state_event(struct cec_adapter *adap)
 	ev.state_change.log_addr_mask = adap->log_addrs.log_addr_mask;
 	ev.state_change.have_conn_info =
 		adap->conn_info.type != CEC_CONNECTOR_TYPE_NO_CONNECTOR;
+	if (adap->log_addrs.flags & CEC_LOG_ADDRS_FL_HPD_PRESENT)
+		ev.flags |= CEC_EVENT_FL_STATE_HPD_PRESENT;
 	cec_queue_event(adap, &ev);
 }
 
diff --git a/drivers/media/cec/core/cec-api.c b/drivers/media/cec/core/cec-api.c
index 1ba080d5a123..4507bf081159 100644
--- a/drivers/media/cec/core/cec-api.c
+++ b/drivers/media/cec/core/cec-api.c
@@ -560,10 +560,7 @@ static int cec_open(struct inode *inode, struct file *filp)
 	 * Initial events that are automatically sent when the cec device is
 	 * opened.
 	 */
-	struct cec_event ev = {
-		.event = CEC_EVENT_STATE_CHANGE,
-		.flags = CEC_EVENT_FL_INITIAL_STATE,
-	};
+	struct cec_event ev = {};
 	unsigned int i;
 	int err;
 
@@ -589,12 +586,17 @@ static int cec_open(struct inode *inode, struct file *filp)
 	filp->private_data = fh;
 
 	/* Queue up initial state events */
+	ev.event = CEC_EVENT_STATE_CHANGE;
 	ev.state_change.phys_addr = adap->phys_addr;
 	ev.state_change.log_addr_mask = adap->log_addrs.log_addr_mask;
 	ev.state_change.have_conn_info =
 		adap->conn_info.type != CEC_CONNECTOR_TYPE_NO_CONNECTOR;
+	ev.flags = CEC_EVENT_FL_INITIAL_STATE;
+	if (adap->log_addrs.flags & CEC_LOG_ADDRS_FL_HPD_PRESENT)
+		ev.flags |= CEC_EVENT_FL_STATE_HPD_PRESENT;
 	cec_queue_event_fh(fh, &ev, 0);
 #ifdef CONFIG_CEC_PIN
+	ev.flags = CEC_EVENT_FL_INITIAL_STATE;
 	if (adap->pin && adap->pin->ops->read_hpd &&
 	    !adap->devnode.unregistered) {
 		err = adap->pin->ops->read_hpd(adap);
-- 
2.53.0

Re: [PATCHv3 12/13] media: cec: core: implement CEC_EVENT_FL_STATE_HPD_PRESENT

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: cec: undeclared and unpopulated HPD_PRESENT flags in cec-adap and cec-api
--

commit 7888dab6341569ef2e99296246cbc101a56dde4e
Author: Hans Verkuil <hverkuil+cisco@kernel.org>

media: cec: core: implement CEC_EVENT_FL_STATE_HPD_PRESENT

This commit updates cec_post_state_event() and cec_open() to set
CEC_EVENT_FL_STATE_HPD_PRESENT on state change events when
CEC_LOG_ADDRS_FL_HPD_PRESENT is set in adap->log_addrs.flags.

> diff --git a/drivers/media/cec/core/cec-adap.c b/drivers/media/cec/core/cec-adap.c
> index ee8a200b9803f..1920377d6b6e0 100644
> --- a/drivers/media/cec/core/cec-adap.c
> +++ b/drivers/media/cec/core/cec-adap.c
> @@ -323,6 +323,8 @@ static void cec_post_state_event(struct cec_adapter *adap)
>  	ev.state_change.log_addr_mask = adap->log_addrs.log_addr_mask;
>  	ev.state_change.have_conn_info =
>  		adap->conn_info.type != CEC_CONNECTOR_TYPE_NO_CONNECTOR;
> +	if (adap->log_addrs.flags & CEC_LOG_ADDRS_FL_HPD_PRESENT)
> +		ev.flags |= CEC_EVENT_FL_STATE_HPD_PRESENT;
>  	cec_queue_event(adap, &ev);
>  }
>  
> diff --git a/drivers/media/cec/core/cec-api.c b/drivers/media/cec/core/cec-api.c
> index a491d7fc8a71a..f938668065fcb 100644
> --- a/drivers/media/cec/core/cec-api.c
> +++ b/drivers/media/cec/core/cec-api.c
> @@ -559,10 +559,7 @@ static int cec_open(struct inode *inode, struct file *filp)
>  	 * Initial events that are automatically sent when the cec device is
>  	 * opened.
>  	 */
> -	struct cec_event ev = {
> -		.event = CEC_EVENT_STATE_CHANGE,
> -		.flags = CEC_EVENT_FL_INITIAL_STATE,
> -	};
> +	struct cec_event ev = {};
>  	unsigned int i;
>  	int err;
>  
> @@ -588,12 +585,17 @@ static int cec_open(struct inode *inode, struct file *filp)
>  	filp->private_data = fh;
>  
>  	/* Queue up initial state events */
> +	ev.event = CEC_EVENT_STATE_CHANGE;
>  	ev.state_change.phys_addr = adap->phys_addr;
>  	ev.state_change.log_addr_mask = adap->log_addrs.log_addr_mask;
>  	ev.state_change.have_conn_info =
>  		adap->conn_info.type != CEC_CONNECTOR_TYPE_NO_CONNECTOR;
> +	ev.flags = CEC_EVENT_FL_INITIAL_STATE;
> +	if (adap->log_addrs.flags & CEC_LOG_ADDRS_FL_HPD_PRESENT)
> +		ev.flags |= CEC_EVENT_FL_STATE_HPD_PRESENT;

[Severity: High]
Does this break the build when CONFIG_CEC_CORE is enabled?

Both cec_post_state_event() in drivers/media/cec/core/cec-adap.c and
cec_open() in drivers/media/cec/core/cec-api.c reference
CEC_LOG_ADDRS_FL_HPD_PRESENT and CEC_EVENT_FL_STATE_HPD_PRESENT, but neither
symbol is defined in include/uapi/linux/cec.h (or anywhere else in the tree):

include/uapi/linux/cec.h:
    #define CEC_EVENT_FL_INITIAL_STATE	(1 << 0)
    #define CEC_EVENT_FL_DROPPED_EVENTS	(1 << 1)

In addition, nothing in drivers/media/cec/ or any CEC driver sets
CEC_LOG_ADDRS_FL_HPD_PRESENT in adap->log_addrs.flags, and
cec_adap_s_log_addrs() in drivers/media/cec/core/cec-api.c masks out any other
flags before __cec_s_log_addrs() overwrites adap->log_addrs:

drivers/media/cec/core/cec-api.c:cec_adap_s_log_addrs() {
    ...
	log_addrs.flags &= CEC_LOG_ADDRS_FL_ALLOW_UNREG_FALLBACK |
			   CEC_LOG_ADDRS_FL_ALLOW_RC_PASSTHRU |
			   CEC_LOG_ADDRS_FL_CDC_ONLY;
    ...
}

Were the header definitions and the code that populates
CEC_LOG_ADDRS_FL_HPD_PRESENT left out of this commit?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/b57a1fc1bb55bd53c9cfc938b0129a6b8b832e90.1793863347.git.hverkuil+cisco@kernel.org?part=1

[PATCHv3 13/13] Documentation: media: document cec_phys_addr_invalidate

[Hans Verkuil]

This function was not document, add it. Also clarify
cec_adap_s_phys_addr, explaining that the PA comes from the EDID.

Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
---
 Documentation/driver-api/media/cec-core.rst | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/driver-api/media/cec-core.rst b/Documentation/driver-api/media/cec-core.rst
index f1ffdec388f3..10927833992c 100644
--- a/Documentation/driver-api/media/cec-core.rst
+++ b/Documentation/driver-api/media/cec-core.rst
@@ -437,7 +437,8 @@ transmitted, otherwise just queue it and return.
 .. c:function::
    void cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block);
 
-Change the physical address. This function will set adap->phys_addr and
+Change the physical address. Use cec_s_phys_addr() to set the Physical Address
+as read from an EDID. This function will set adap->phys_addr and
 send an event if it has changed. If cec_s_log_addrs() has been called and
 the physical address has become valid, then the CEC framework will start
 claiming the logical addresses. If block is true, then this function won't
@@ -449,6 +450,13 @@ then the CEC adapter will be disabled. If you change a valid physical address
 to another valid physical address, then this function will first set the
 address to CEC_PHYS_ADDR_INVALID before enabling the new physical address.
 
+.. c:function::
+   void cec_phys_addr_invalidate(struct cec_adapter *adap);
+
+Invalidates the Physical Address when the EDID was lost, e.g. if the HDMI
+cable was unplugged. Do not use this to report an invalid Physical Address
+that you read from an EDID, use cec_s_phys_addr() for that.
+
 .. c:function::
    void cec_s_phys_addr_from_edid(struct cec_adapter *adap, \
 				  const struct edid *edid);
-- 
2.53.0