| Message ID | 20250126-drm-hdmi-connector-cec-v3-8-5b5b2d4956da@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/display: generic HDMI CEC helpers | expand |
On Sun, Jan 26, 2025 at 03:29:13PM +0200, Dmitry Baryshkov wrote: > /* ----------------------------------------------------------------------------- > * Bridge Connector Initialisation > */ > @@ -633,6 +711,21 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > if (ret) > return ERR_PTR(ret); > } > + > + if (bridge->hdmi_cec_adapter_name) { > + if (!bridge->funcs->hdmi_cec_enable || > + !bridge->funcs->hdmi_cec_log_addr || > + !bridge->funcs->hdmi_cec_transmit) > + return ERR_PTR(-EINVAL); > + > + ret = drm_connector_hdmi_cec_register(connector, > + &drm_bridge_connector_hdmi_cec_ops, > + bridge->hdmi_cec_adapter_name, > + bridge->hdmi_cec_available_las, > + bridge->hdmi_dev); > + if (ret) > + return ERR_PTR(ret); > + } Maybe we can use a different bridge feature flag to trigger the CEC code support instead? Maxime
On Tue, Jan 28, 2025 at 05:14:06PM +0100, Maxime Ripard wrote: > On Sun, Jan 26, 2025 at 03:29:13PM +0200, Dmitry Baryshkov wrote: > > /* ----------------------------------------------------------------------------- > > * Bridge Connector Initialisation > > */ > > @@ -633,6 +711,21 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > if (ret) > > return ERR_PTR(ret); > > } > > + > > + if (bridge->hdmi_cec_adapter_name) { > > + if (!bridge->funcs->hdmi_cec_enable || > > + !bridge->funcs->hdmi_cec_log_addr || > > + !bridge->funcs->hdmi_cec_transmit) > > + return ERR_PTR(-EINVAL); > > + > > + ret = drm_connector_hdmi_cec_register(connector, > > + &drm_bridge_connector_hdmi_cec_ops, > > + bridge->hdmi_cec_adapter_name, > > + bridge->hdmi_cec_available_las, > > + bridge->hdmi_dev); > > + if (ret) > > + return ERR_PTR(ret); > > + } > > Maybe we can use a different bridge feature flag to trigger the CEC code > support instead? it is possible, but what is the possible usecase? DP drivers should be using DP_AUX CEC instead. And I think there are no other kinds of bridges which implement CEC support. Meson driver does something strange by registering CEC notifier from meson_encoder_hdmi. I think instead this should be moved to the DW HDMI bridge itself
On 29/01/2025 00:44, Dmitry Baryshkov wrote: > On Tue, Jan 28, 2025 at 05:14:06PM +0100, Maxime Ripard wrote: >> On Sun, Jan 26, 2025 at 03:29:13PM +0200, Dmitry Baryshkov wrote: >>> /* ----------------------------------------------------------------------------- >>> * Bridge Connector Initialisation >>> */ >>> @@ -633,6 +711,21 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, >>> if (ret) >>> return ERR_PTR(ret); >>> } >>> + >>> + if (bridge->hdmi_cec_adapter_name) { >>> + if (!bridge->funcs->hdmi_cec_enable || >>> + !bridge->funcs->hdmi_cec_log_addr || >>> + !bridge->funcs->hdmi_cec_transmit) >>> + return ERR_PTR(-EINVAL); >>> + >>> + ret = drm_connector_hdmi_cec_register(connector, >>> + &drm_bridge_connector_hdmi_cec_ops, >>> + bridge->hdmi_cec_adapter_name, >>> + bridge->hdmi_cec_available_las, >>> + bridge->hdmi_dev); >>> + if (ret) >>> + return ERR_PTR(ret); >>> + } >> >> Maybe we can use a different bridge feature flag to trigger the CEC code >> support instead? > > it is possible, but what is the possible usecase? DP drivers should be > using DP_AUX CEC instead. And I think there are no other kinds of > bridges which implement CEC support. Meson driver does something strange > by registering CEC notifier from meson_encoder_hdmi. I think instead > this should be moved to the DW HDMI bridge itself > It was done before bridge_connector has any support for CEC to keep the functionnality, I'll be happy to switch to this. Neil
On Wed, Jan 29, 2025 at 05:44:31PM +0100, Neil Armstrong wrote: > On 29/01/2025 00:44, Dmitry Baryshkov wrote: > > On Tue, Jan 28, 2025 at 05:14:06PM +0100, Maxime Ripard wrote: > > > On Sun, Jan 26, 2025 at 03:29:13PM +0200, Dmitry Baryshkov wrote: > > > > /* ----------------------------------------------------------------------------- > > > > * Bridge Connector Initialisation > > > > */ > > > > @@ -633,6 +711,21 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > > > if (ret) > > > > return ERR_PTR(ret); > > > > } > > > > + > > > > + if (bridge->hdmi_cec_adapter_name) { > > > > + if (!bridge->funcs->hdmi_cec_enable || > > > > + !bridge->funcs->hdmi_cec_log_addr || > > > > + !bridge->funcs->hdmi_cec_transmit) > > > > + return ERR_PTR(-EINVAL); > > > > + > > > > + ret = drm_connector_hdmi_cec_register(connector, > > > > + &drm_bridge_connector_hdmi_cec_ops, > > > > + bridge->hdmi_cec_adapter_name, > > > > + bridge->hdmi_cec_available_las, > > > > + bridge->hdmi_dev); > > > > + if (ret) > > > > + return ERR_PTR(ret); > > > > + } > > > > > > Maybe we can use a different bridge feature flag to trigger the CEC code > > > support instead? > > > > it is possible, but what is the possible usecase? DP drivers should be > > using DP_AUX CEC instead. And I think there are no other kinds of > > bridges which implement CEC support. Meson driver does something strange > > by registering CEC notifier from meson_encoder_hdmi. I think instead > > this should be moved to the DW HDMI bridge itself > > It was done before bridge_connector has any support for CEC to keep the > functionnality, I'll be happy to switch to this. Well... Converting dw-hdmi to use all HDMI API is going to be interesting anyway: - iMX8, iMX6 attach with flags=0, so dw-hdmi creates connector (with all its fanciness). - Ingenic and RCAR-DU use simple drm_bridge_connector, so they will just get all new features. - Meson uses drm_bridge_connector + additional bits on top of it (like CEC, max_bpc, HDR). Either we end up with a monstruous commit that reworks dw-hdmi _and_ drop extra bits from meson driver at the same time or we need some well-thought plan. For example, first make some of the features of Meson optional and dependent on their generic counterparts being registered by drm_bridge_connector, then convert dw-hdmi _and_ use drm_bridge_connector internally, finally drop extra meson features now consumed by the core.
diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c index 9f234bc647d5c0880d4c42aea130074b7fa54573..5b77fd59d79abddd419e611a7868b001857ccb37 100644 --- a/drivers/gpu/drm/display/drm_bridge_connector.c +++ b/drivers/gpu/drm/display/drm_bridge_connector.c @@ -9,6 +9,8 @@ #include <linux/property.h> #include <linux/slab.h> +#include <media/cec.h> + #include <drm/drm_atomic_state_helper.h> #include <drm/drm_bridge.h> #include <drm/drm_bridge_connector.h> @@ -497,6 +499,82 @@ static const struct drm_connector_hdmi_audio_funcs drm_bridge_connector_hdmi_aud .mute_stream = drm_bridge_connector_audio_mute_stream, }; +static int drm_bridge_connector_hdmi_cec_enable(struct drm_connector *connector, bool enable) +{ + struct drm_bridge_connector *bridge_connector = + to_drm_bridge_connector(connector); + struct drm_bridge *bridge; + + bridge = bridge_connector->bridge_hdmi; + + return bridge->funcs->hdmi_cec_enable(bridge, enable); +} + +static int drm_bridge_connector_hdmi_cec_log_addr(struct drm_connector *connector, u8 logical_addr) +{ + struct drm_bridge_connector *bridge_connector = + to_drm_bridge_connector(connector); + struct drm_bridge *bridge; + + bridge = bridge_connector->bridge_hdmi; + + return bridge->funcs->hdmi_cec_log_addr(bridge, logical_addr); +} + +static int drm_bridge_connector_hdmi_cec_transmit(struct drm_connector *connector, + u8 attempts, + u32 signal_free_time, + struct cec_msg *msg) +{ + struct drm_bridge_connector *bridge_connector = + to_drm_bridge_connector(connector); + struct drm_bridge *bridge; + + bridge = bridge_connector->bridge_hdmi; + + return bridge->funcs->hdmi_cec_transmit(bridge, attempts, + signal_free_time, + msg); +} + +static int drm_bridge_connector_hdmi_cec_init(struct drm_connector *connector) +{ + struct drm_bridge_connector *bridge_connector = + to_drm_bridge_connector(connector); + struct drm_bridge *bridge; + + bridge = bridge_connector->bridge_hdmi; + + drm_bridge_cec_data_set(bridge, connector); + + if (!bridge->funcs->hdmi_cec_init) + return 0; + + return bridge->funcs->hdmi_cec_init(connector, bridge); +} + +static void drm_bridge_connector_hdmi_cec_unregister(struct drm_connector *connector) +{ + struct drm_bridge_connector *bridge_connector = + to_drm_bridge_connector(connector); + struct drm_bridge *bridge; + + bridge = bridge_connector->bridge_hdmi; + + drm_bridge_cec_data_set(bridge, NULL); + + drm_connector_hdmi_cec_unregister(connector); +} + +static const struct drm_connector_hdmi_cec_adapter_ops drm_bridge_connector_hdmi_cec_ops = { + .base.unregister = drm_bridge_connector_hdmi_cec_unregister, + .init = drm_bridge_connector_hdmi_cec_init, + .enable = drm_bridge_connector_hdmi_cec_enable, + .log_addr = drm_bridge_connector_hdmi_cec_log_addr, + .transmit = drm_bridge_connector_hdmi_cec_transmit, +}; + + /* ----------------------------------------------------------------------------- * Bridge Connector Initialisation */ @@ -633,6 +711,21 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, if (ret) return ERR_PTR(ret); } + + if (bridge->hdmi_cec_adapter_name) { + if (!bridge->funcs->hdmi_cec_enable || + !bridge->funcs->hdmi_cec_log_addr || + !bridge->funcs->hdmi_cec_transmit) + return ERR_PTR(-EINVAL); + + ret = drm_connector_hdmi_cec_register(connector, + &drm_bridge_connector_hdmi_cec_ops, + bridge->hdmi_cec_adapter_name, + bridge->hdmi_cec_available_las, + bridge->hdmi_dev); + if (ret) + return ERR_PTR(ret); + } } else { ret = drmm_connector_init(drm, connector, &drm_bridge_connector_funcs, diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index df1d72c7e176c75585283684acc2ef2ffb2f8bff..b55e80a57758e8b652eac0cd01cb245a04e221f5 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -32,6 +32,7 @@ #include <drm/drm_mode_object.h> #include <drm/drm_modes.h> +struct cec_msg; struct device_node; struct drm_bridge; @@ -729,6 +730,16 @@ struct drm_bridge_funcs { struct drm_bridge *bridge, bool enable, int direction); + int (*hdmi_cec_init)(struct drm_connector *connector, + struct drm_bridge *bridge); + + int (*hdmi_cec_enable)(struct drm_bridge *bridge, bool enable); + + int (*hdmi_cec_log_addr)(struct drm_bridge *bridge, u8 logical_addr); + + int (*hdmi_cec_transmit)(struct drm_bridge *bridge, u8 attempts, + u32 signal_free_time, struct cec_msg *msg); + /** * @debugfs_init: * @@ -924,6 +935,16 @@ struct drm_bridge { */ bool hdmi_cec_notifier; + /** + * @hdmi_cec_adapter_name: the name of the adapter to register + */ + const char *hdmi_cec_adapter_name; + + /** + * @hdmi_cec_available_las: number of logical addresses, CEC_MAX_LOG_ADDRS if unset + */ + u8 hdmi_cec_available_las; + /** private: */ /** * @hpd_mutex: Protects the @hpd_cb and @hpd_data fields.
Implement necessary glue code to let DRM bridge drivers to implement CEC adapters support. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/display/drm_bridge_connector.c | 93 ++++++++++++++++++++++++++ include/drm/drm_bridge.h | 21 ++++++ 2 files changed, 114 insertions(+)