On Tue, Jan 28, 2025 at 02:17:19PM +0200, Dmitry Baryshkov wrote:
> 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 <dmitry.barysh...@linaro.org>
> > > ---
> > > 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.There's two things here: cec_pin and cec_adapter are equivalent. They provide the same feature, but one relies on an hardware controller, the other bitbangs a GPIO. They are also mutually exclusive. So I'd very much expect a different set of hooks there. Then we have the notifiers. AFAIK, it's useful when you don't have a CEC hardware and need to coordinate with an external block providing the feature. Again, I'm not sure why we should share anything there. Can't we just duplicate the function names? Maxime
signature.asc
Description: PGP signature
