| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <j2ej27mzia326q6fjqk2c3xui6dhbx2chswjjmgaoxgiajstl2@dcmgdtkzbjht>
Date: Wed, 5 Feb 2025 17:01:09 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Maxime Ripard <mripard@...nel.org>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>, Dave Stevenson <dave.stevenson@...pberrypi.com>,
Maíra Canal <mcanal@...lia.com>, Raspberry Pi Kernel Maintenance <kernel-list@...pberrypi.com>,
Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>,
Robert Foss <rfoss@...nel.org>, Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 02/10] drm/display: add CEC helpers code
On Wed, Feb 05, 2025 at 03:27:05PM +0100, Maxime Ripard wrote:
> 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.baryshkov@...aro.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.
That's the point. Normal CEC adapters would need to implement these
callbacks. CEC pin adapter will have a different set of callbacks.
Notifiers do not need any extra callbacks at all.
Maybe I should restructure the patchset, pushing helpers closer to the
end of the series.
> 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?
Which function names?
--
With best wishes
Dmitry
Powered by blists - more mailing lists