media/drm: cec: add HPD_PRESENT
Subsystems (10)
v3 cover media/drm: cec: add HPD_PRESENT
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(-)
Signature · 1 line
2.53.0
PATCH v3 1/13 gpu: drm: sti: call cec_notifier_phys_addr_invalidate if no HPD
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.
--- drivers/gpu/drm/sti/sti_hdmi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/sti/sti_hdmi.c +2 -2
@@ -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; }
Signature · 1 line
2.53.0
PATCH v3 2/13 gpu: drm: exynos: call cec_notifier_phys_addr_invalidate if no HPD
In reply to Hans VerkuilIf 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.
--- drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/exynos/exynos_hdmi.c +1 -1
@@ -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; }
Signature · 1 line
2.53.0
Re: [PATCHv3 02/13] gpu: drm: exynos: call cec_notifier_phys_addr_invalidate if no HPD
Quoted 8 lines · Hans Verkuil
If a HPD is present, then call
cec_notifier_set_phys_addr. If not, then callcec_notifier_phys_addr_invalidate. This helps the CEC framework differentiate betweenCEC_PHYS_ADDR_INVALIDdue 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_addron disconnect instead ofcec_notifier_phys_addr_invalidate.
Quoted 8 lines
Signed-off-by Hans Verkuil <[email protected]>Cc Inki Dae <[email protected]>Cc Kyungmin Park <[email protected]>Cc Krzysztof Kozlowski <[email protected]>Cc Peter Griffin <[email protected]>Cc Alim Akhtar <[email protected]>Cc Maxime Ripard <[email protected]>---
Best regards,
Krzysztof
PATCH v3 3/13 gpu: drm: omapdrm: call cec_s_phys_addr only if HPD is present
In reply to Hans VerkuilIf 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.
--- 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(-)
drivers/gpu/drm/omapdrm/dss/hdmi4.c +3 -4
@@ -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);
drivers/gpu/drm/omapdrm/dss/hdmi4_cec.c +5 -0
@@ -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) {
drivers/gpu/drm/omapdrm/dss/hdmi4_cec.h +5 -0
@@ -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) { }
Signature · 1 line
2.53.0
PATCH v3 4/13 media: i2c: adv7511-v4l2: call cec_s_phys_addr only if HPD is high
In reply to Hans VerkuilIf 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.
--- drivers/media/i2c/adv7511-v4l2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/i2c/adv7511-v4l2.c +1 -1
@@ -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 ); }
Signature · 1 line
2.53.0
PATCH v3 5/13 media: cec: extron-da-hd-4k-plus: call cec_s_phys_addr only if HPD is high
In reply to Hans VerkuilIf 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.
--- .../cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c +6 -2
@@ -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)
Signature · 1 line
2.53.0
PATCH v3 6/13 media: cec: core: improve cec_notifier_phys_addr_invalidate
In reply to Hans Verkuilcec_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.
--- drivers/media/cec/core/cec-notifier.c | 26 +++++++++++++++++++-- include/media/cec-notifier.h | 33 +++++++++++++++------------ 2 files changed, 43 insertions(+), 16 deletions(-)
drivers/media/cec/core/cec-notifier.c +24 -2
@@ -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;
include/media/cec-notifier.h +19 -14
@@ -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
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCHv3 06/13] media: cec: core: improve cec_notifier_phys_addr_invalidate
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…
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 <[email protected]>
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.
Quoted 4 lines
drivers/media/cec/core/cec-notifier.c +0
-0
[ ... ]
Quoted 30 lines
drivers/media/cec/core/cec-notifier.c +10
-2@@ -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)
drivers/media/cec/core/cec-notifier.c void cec_notifier_set_phys_addr_from_edid(struct cec_notifier *n, · line 239
Quoted 10 lines
drivers/media/cec/core/cec-notifier.c +10
-0+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?
Quoted 6 lines
include/media/cec-notifier.h +0
-0@@ -80,7 +80,10 @@ void cec_notifier_cec_adap_unregister(struct cec_notifier *n,* @n: the CEC notifier
include/media/cec-notifier.h void cec_notifier_cec_adap_unregister(struct cec_notifier *n, · line 89
Quoted 10 lines
include/media/cec-notifier.h +4
-1* @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.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/e03c6b786e243e6a0062c61a77ce478dd561c27b.1793863347.git.hverkuil+cisco@kernel.org?part=1
PATCH v3 7/13 media: uapi: cec.h: add CEC_CAP_HAS_HPD_PRESENT/CEC_LOG_ADDRS_FL_HPD_PRESENT
In reply to Hans VerkuilAdd a capability and a new flag that is used to indicate if a HPD is present.
--- include/uapi/linux/cec.h | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-)
include/uapi/linux/cec.h +15 -13
@@ -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
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCHv3 07/13] media: uapi: cec.h: add CEC_CAP_HAS_HPD_PRESENT/CEC_LOG_ADDRS_FL_HPD_PRESENT
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…
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 <[email protected]>
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.
Quoted 46 lines
include/uapi/linux/cec.h +13
-13@@ -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.
include/uapi/linux/cec.h struct cec_log_addrs { · line 409
Quoted 5 lines
include/uapi/linux/cec.h +2
-0@@ -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.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/4cd2dd652db11b7410a6f9608c5c15bef45f5df5.1793863347.git.hverkuil+cisco@kernel.org?part=1
PATCH v3 8/13 Documentation: media: document HPD_PRESENT cap and flag.
In reply to Hans VerkuilDocument CEC_CAP_HAS_HPD_PRESENT and CEC_LOG_ADDRS_FL_HPD_PRESENT.
--- .../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(+)
Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst +10 -0
@@ -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 ============
Documentation/userspace-api/media/cec/cec-ioc-adap-g-log-addrs.rst +10 -0
@@ -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}|
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCHv3 08/13] Documentation: media: document HPD_PRESENT cap and flag.
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…
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 <[email protected]>
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.
Quoted 8 lines
Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst +0
-0@@ -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.
Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst returns the information to the application. The ioctl never fails. · line 155
Quoted 10 lines
Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst +10
-0+ * .. _`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.
Quoted 8 lines
Documentation/userspace-api/media/cec/cec-ioc-adap-g-log-addrs.rst +0
-0@@ -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.
Documentation/userspace-api/media/cec/cec-ioc-adap-g-log-addrs.rst logical address types are already defined will return with error ``EBUSY``. · line 204
Quoted 10 lines
Documentation/userspace-api/media/cec/cec-ioc-adap-g-log-addrs.rst +10
-0+ * .. _`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.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/e98308c0db382c09ce698b34ca54f798ab0e3557.1793863347.git.hverkuil+cisco@kernel.org?part=1
PATCH v3 9/13 media: cec: core: add support for HPD_PRESENT
In reply to Hans VerkuilThere 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.
--- 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(-)
drivers/media/cec/core/cec-adap.c +42 -7
@@ -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 )
drivers/media/cec/core/cec-api.c +2 -1
@@ -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; }
drivers/media/cec/core/cec-core.c +3 -1
@@ -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 ;
drivers/media/cec/core/cec-priv.h +2 -1
@@ -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,
include/media/cec.h +14 -13
@@ -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 *
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCHv3 09/13] media: cec: core: add support for HPD_PRESENT
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]…
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 <[email protected]>
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.
Quoted 17 lines
drivers/media/cec/core/cec-adap.c +4
-1@@ -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 ;
drivers/media/cec/core/cec-adap.c int cec_adap_enable(struct cec_adapter *adap) · line 1745
Quoted 10 lines
drivers/media/cec/core/cec-adap.c +8
-2-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.
Quoted 19 lines
drivers/media/cec/core/cec-adap.c +10
-2if (!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);
drivers/media/cec/core/cec-adap.c void cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block) · line 1790
Quoted 8 lines
drivers/media/cec/core/cec-adap.c +1
-1@@ -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?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/ae93656d52c429861504836a10c07b9eac3cd470.1793863347.git.hverkuil+cisco@kernel.org?part=1
PATCH v3 10/13 media: uapi: cec.h: add CEC_EVENT_FL_STATE_HPD_PRESENT flag
In reply to Hans VerkuilThe CEC_EVENT_FL_STATE_HPD_PRESENT will report whether the HPD is present or not.
--- include/uapi/linux/cec.h | 1 + 1 file changed, 1 insertion(+)
include/uapi/linux/cec.h +1 -0
@@ -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.
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCHv3 10/13] media: uapi: cec.h: add CEC_EVENT_FL_STATE_HPD_PRESENT flag
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 --
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 <[email protected]>
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.
include/uapi/linux/cec.h struct cec_connector_info { · line 456
Quoted 9 lines
include/uapi/linux/cec.h +1
-0@@ -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?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/81a832c5568748e37f081d217998f80880ab8dc4.1793863347.git.hverkuil+cisco@kernel.org?part=1
PATCH v3 11/13 Documentation: media: cec-ioc-dqevent.rst: document STATE_HPD_PRESENT flag
In reply to Hans VerkuilDocument the CEC_EVENT_FL_STATE_HPD_PRESENT event flag.
--- .../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(-)
Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst +2 -1
@@ -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
Documentation/userspace-api/media/cec/cec-ioc-dqevent.rst +10 -0
@@ -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
Signature · 1 line
2.53.0
PATCH v3 12/13 media: cec: core: implement CEC_EVENT_FL_STATE_HPD_PRESENT
In reply to Hans VerkuilSet CEC_EVENT_FL_STATE_HPD_PRESENT if needed when posting the state change event.
--- drivers/media/cec/core/cec-adap.c | 2 ++ drivers/media/cec/core/cec-api.c | 10 ++++++---- 2 files changed, 8 insertions(+), 4 deletions(-)
drivers/media/cec/core/cec-adap.c +2 -0
@@ -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); }
drivers/media/cec/core/cec-api.c +6 -4
@@ -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);
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCHv3 12/13] media: cec: core: implement CEC_EVENT_FL_STATE_HPD_PRESENT
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 --
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 <[email protected]>
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.
Quoted 31 lines
drivers/media/cec/core/cec-adap.c +2
-0@@ -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); }drivers/media/cec/core/cec-api.c +1
-4@@ -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;
drivers/media/cec/core/cec-api.c static int cec_open(struct inode *inode, struct file *filp) · line 595
Quoted 10 lines
drivers/media/cec/core/cec-api.c +4
-0/* 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?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/b57a1fc1bb55bd53c9cfc938b0129a6b8b832e90.1793863347.git.hverkuil+cisco@kernel.org?part=1
PATCH v3 13/13 Documentation: media: document cec_phys_addr_invalidate
In reply to Hans VerkuilThis function was not document, add it. Also clarify cec_adap_s_phys_addr, explaining that the PA comes from the EDID.
--- Documentation/driver-api/media/cec-core.rst | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
Documentation/driver-api/media/cec-core.rst +9 -1
@@ -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);
Signature · 1 line
2.53.0