On Tue, Jan 28, 2025 at 11:36:06AM +0100, Maxime Ripard wrote: > On Sun, Jan 26, 2025 at 03:29:07PM +0200, Dmitry Baryshkov wrote: > > Add generic CEC helpers to be used by HDMI drivers. Both notifier and > > and adapter are supported for registration. Once registered, the driver > > can call common set of functions to update physical address, to > > invalidate it or to unregister CEC data. > > > > Signed-off-by: Dmitry Baryshkov <[email protected]> > > --- > > drivers/gpu/drm/display/Kconfig | 5 + > > drivers/gpu/drm/display/Makefile | 2 + > > drivers/gpu/drm/display/drm_hdmi_cec_helper.c | 209 > > ++++++++++++++++++++++++++ > > include/drm/display/drm_hdmi_cec_helper.h | 61 ++++++++ > > 4 files changed, 277 insertions(+) > > > > diff --git a/drivers/gpu/drm/display/Kconfig > > b/drivers/gpu/drm/display/Kconfig > > index > > 8d22b7627d41f7bc015decf24ae02a05bc00f055..49da9b768acf3e5f84f2cefae4bb042cfd57a50c > > 100644 > > --- a/drivers/gpu/drm/display/Kconfig > > +++ b/drivers/gpu/drm/display/Kconfig > > @@ -82,6 +82,11 @@ config DRM_DISPLAY_HDMI_AUDIO_HELPER > > DRM display helpers for HDMI Audio functionality (generic HDMI Codec > > implementation). > > > > +config DRM_DISPLAY_HDMI_CEC_HELPER > > + bool > > + help > > + DRM display helpers for HDMI CEC implementation. > > + > > config DRM_DISPLAY_HDMI_HELPER > > bool > > help > > diff --git a/drivers/gpu/drm/display/Makefile > > b/drivers/gpu/drm/display/Makefile > > index > > b17879b957d5401721396e247fa346387cf6c48a..2cd078e2b81c1a9e6b336c4187b444bcb8a50e51 > > 100644 > > --- a/drivers/gpu/drm/display/Makefile > > +++ b/drivers/gpu/drm/display/Makefile > > @@ -16,6 +16,8 @@ drm_display_helper-$(CONFIG_DRM_DISPLAY_DSC_HELPER) += \ > > drm_display_helper-$(CONFIG_DRM_DISPLAY_HDCP_HELPER) += drm_hdcp_helper.o > > drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_AUDIO_HELPER) += \ > > drm_hdmi_audio_helper.o > > +drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_CEC_HELPER) += \ > > + drm_hdmi_cec_helper.o > > drm_display_helper-$(CONFIG_DRM_DISPLAY_HDMI_HELPER) += \ > > drm_hdmi_helper.o \ > > drm_scdc_helper.o > > diff --git a/drivers/gpu/drm/display/drm_hdmi_cec_helper.c > > b/drivers/gpu/drm/display/drm_hdmi_cec_helper.c > > new file mode 100644 > > index > > 0000000000000000000000000000000000000000..a6ed5f0fc3835b013a83308f5285ea0819c5702c > > --- /dev/null > > +++ b/drivers/gpu/drm/display/drm_hdmi_cec_helper.c > > @@ -0,0 +1,209 @@ > > +// SPDX-License-Identifier: MIT > > +/* > > + * Copyright (c) 2024 Linaro Ltd > > + */ > > + > > +#include <drm/drm_bridge.h> > > +#include <drm/drm_connector.h> > > +#include <drm/display/drm_hdmi_cec_helper.h> > > + > > +#include <linux/mutex.h> > > + > > +#include <media/cec.h> > > +#include <media/cec-notifier.h> > > + > > +void drm_connector_hdmi_cec_unregister(struct drm_connector *connector) > > +{ > > + cec_unregister_adapter(connector->cec.adapter); > > + connector->cec.adapter = NULL; > > + > > + cec_notifier_conn_unregister(connector->cec.notifier); > > + connector->cec.notifier = NULL; > > + > > + connector->cec.funcs = NULL; > > +} > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_unregister); > > + > > +static const struct drm_connector_cec_funcs drm_connector_hdmi_cec_funcs = > > { > > + .unregister = drm_connector_hdmi_cec_unregister, > > +}; > > + > > +int drm_connector_hdmi_cec_notifier_register(struct drm_connector > > *connector, > > + const char *port_name, > > + struct device *dev) > > +{ > > + struct cec_connector_info conn_info; > > + struct cec_notifier *notifier; > > + int ret; > > + > > + mutex_lock(&connector->cec.mutex); > > + > > + if (connector->cec.funcs) { > > + ret = -EBUSY; > > + goto err_unlock; > > + } > > + > > + cec_fill_conn_info_from_drm(&conn_info, connector); > > + > > + notifier = cec_notifier_conn_register(dev, port_name, &conn_info); > > + if (!notifier) { > > + ret = -ENOMEM; > > + goto err_unlock; > > + } > > + > > + connector->cec.notifier = notifier; > > + connector->cec.funcs = &drm_connector_hdmi_cec_funcs; > > + > > + mutex_unlock(&connector->cec.mutex); > > + > > + return 0; > > + > > +err_unlock: > > + mutex_unlock(&connector->cec.mutex); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_notifier_register); > > + > > +#define to_hdmi_cec_adapter_ops(ops) \ > > + container_of(ops, struct drm_connector_hdmi_cec_adapter_ops, base) > > + > > +static int drm_connector_hdmi_cec_adap_enable(struct cec_adapter *adap, > > bool enable) > > +{ > > + struct drm_connector *connector = cec_get_drvdata(adap); > > + struct drm_connector_hdmi_cec_adapter_ops *ops = > > + to_hdmi_cec_adapter_ops(connector->cec.funcs); > > + > > + return ops->enable(connector, enable); > > +} > > + > > +static int drm_connector_hdmi_cec_adap_log_addr(struct cec_adapter *adap, > > u8 logical_addr) > > +{ > > + struct drm_connector *connector = cec_get_drvdata(adap); > > + struct drm_connector_hdmi_cec_adapter_ops *ops = > > + to_hdmi_cec_adapter_ops(connector->cec.funcs); > > + > > + return ops->log_addr(connector, logical_addr); > > +} > > + > > +static int drm_connector_hdmi_cec_adap_transmit(struct cec_adapter *adap, > > u8 attempts, > > + u32 signal_free_time, struct > > cec_msg *msg) > > +{ > > + struct drm_connector *connector = cec_get_drvdata(adap); > > + struct drm_connector_hdmi_cec_adapter_ops *ops = > > + to_hdmi_cec_adapter_ops(connector->cec.funcs); > > + > > + return ops->transmit(connector, attempts, signal_free_time, msg); > > +} > > + > > +static const struct cec_adap_ops drm_connector_hdmi_cec_adap_ops = { > > + .adap_enable = drm_connector_hdmi_cec_adap_enable, > > + .adap_log_addr = drm_connector_hdmi_cec_adap_log_addr, > > + .adap_transmit = drm_connector_hdmi_cec_adap_transmit, > > +}; > > + > > +int drm_connector_hdmi_cec_register(struct drm_connector *connector, > > + const struct > > drm_connector_hdmi_cec_adapter_ops *ops, > > + const char *name, > > + u8 available_las, > > + struct device *dev) > > +{ > > + struct cec_connector_info conn_info; > > + struct cec_adapter *cec_adap; > > + int ret; > > + > > + if (!ops->base.unregister || > > + !ops->init || !ops->enable || !ops->log_addr || !ops->transmit) > > + return -EINVAL; > > + > > + mutex_lock(&connector->cec.mutex); > > + > > + if (connector->cec.funcs) { > > + ret = -EBUSY; > > + goto err_unlock; > > + } > > + > > + cec_adap = cec_allocate_adapter(&drm_connector_hdmi_cec_adap_ops, > > connector, name, > > + CEC_CAP_DEFAULTS | > > CEC_CAP_CONNECTOR_INFO, > > + available_las ? : CEC_MAX_LOG_ADDRS); > > + ret = PTR_ERR_OR_ZERO(cec_adap); > > + if (ret < 0) > > + goto err_unlock; > > + > > + cec_fill_conn_info_from_drm(&conn_info, connector); > > + cec_s_conn_info(cec_adap, &conn_info); > > + > > + connector->cec.adapter = cec_adap; > > + connector->cec.funcs = &ops->base; > > + > > + ret = ops->init(connector); > > + if (ret < 0) > > + goto err_delete_adapter; > > + > > + ret = cec_register_adapter(cec_adap, dev); > > + if (ret < 0) > > + goto err_delete_adapter; > > + > > + mutex_unlock(&connector->cec.mutex); > > + > > + return 0; > > + > > +err_delete_adapter: > > + cec_delete_adapter(cec_adap); > > + > > + connector->cec.adapter = NULL; > > + > > +err_unlock: > > + mutex_unlock(&connector->cec.mutex); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_register); > > + > > +void drm_connector_hdmi_cec_received_msg(struct drm_connector *connector, > > + struct cec_msg *msg) > > +{ > > + cec_received_msg(connector->cec.adapter, msg); > > +} > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_received_msg); > > + > > +void drm_connector_hdmi_cec_transmit_attempt_done(struct drm_connector > > *connector, > > + u8 status) > > +{ > > + cec_transmit_attempt_done(connector->cec.adapter, status); > > +} > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_transmit_attempt_done); > > + > > +void drm_connector_hdmi_cec_transmit_done(struct drm_connector *connector, > > + u8 status, > > + u8 arb_lost_cnt, u8 nack_cnt, > > + u8 low_drive_cnt, u8 error_cnt) > > +{ > > + cec_transmit_done(connector->cec.adapter, status, > > + arb_lost_cnt, nack_cnt, low_drive_cnt, error_cnt); > > +} > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_transmit_done); > > + > > +void drm_connector_hdmi_cec_phys_addr_invalidate(struct drm_connector > > *connector) > > +{ > > + mutex_lock(&connector->cec.mutex); > > + > > + cec_phys_addr_invalidate(connector->cec.adapter); > > + cec_notifier_phys_addr_invalidate(connector->cec.notifier); > > + > > + mutex_unlock(&connector->cec.mutex); > > +} > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_phys_addr_invalidate); > > + > > +void drm_connector_hdmi_cec_phys_addr_set(struct drm_connector *connector) > > +{ > > + mutex_lock(&connector->cec.mutex); > > + > > + cec_s_phys_addr(connector->cec.adapter, > > + connector->display_info.source_physical_address, false); > > + cec_notifier_set_phys_addr(connector->cec.notifier, > > + > > connector->display_info.source_physical_address); > > + > > + mutex_unlock(&connector->cec.mutex); > > +} > > +EXPORT_SYMBOL(drm_connector_hdmi_cec_phys_addr_set); > > diff --git a/include/drm/display/drm_hdmi_cec_helper.h > > b/include/drm/display/drm_hdmi_cec_helper.h > > new file mode 100644 > > index > > 0000000000000000000000000000000000000000..cd6274e4ee9b3e41a2d85289c4a420b854340e19 > > --- /dev/null > > +++ b/include/drm/display/drm_hdmi_cec_helper.h > > @@ -0,0 +1,61 @@ > > +/* SPDX-License-Identifier: MIT */ > > + > > +#ifndef DRM_DISPLAY_HDMI_CEC_HELPER > > +#define DRM_DISPLAY_HDMI_CEC_HELPER > > + > > +#include <drm/drm_connector.h> > > + > > +#include <linux/types.h> > > + > > +struct drm_connector; > > + > > +struct cec_msg; > > +struct device; > > + > > +struct drm_connector_hdmi_cec_adapter_ops { > > + struct drm_connector_cec_funcs base; > > + > > + int (*init)(struct drm_connector *connector); > > + void (*uninit)(struct drm_connector *connector); > > + > > + int (*enable)(struct drm_connector *connector, bool enable); > > + int (*log_addr)(struct drm_connector *connector, u8 logical_addr); > > + int (*transmit)(struct drm_connector *connector, u8 attempts, > > + u32 signal_free_time, struct cec_msg *msg); > > +}; > > Why can't we merge drm_connector_cec_funcs and > drm_connector_cec_adapter_ops? They look equivalent to me?
Well, not exactly. The funcs is a generic interface. Notifiers do not need the adapter_ops. And cec_pin (sun4i) would also require a different set of callbacks. Thus I decided that it's easier to subclass funcs instead of adding all possible callbacks there. > > We should also document those hooks. Ack, I forgot to add docs here. > > Aside from the mutex discussion, the rest of the patch looks good to me. > > Maxime -- With best wishes Dmitry
