Re: [PATCH RFC 4/5] drm/bridge: connector: add support for HDMI codec framework

[Maxime Ripard <mripard@xxxxxxxxxx>]

On Sat, Jun 15, 2024 at 08:53:33PM GMT, Dmitry Baryshkov wrote:
> Add necessary glue code to be able to use new HDMI codec framework from
> the DRM bridge drivers. The drm_bridge implements a limited set of the
> hdmi_codec_ops interface, with the functions accepting both
> drm_connector and drm_bridge instead of just a generic void pointer.
> 
> This framework is integrated with the DRM HDMI Connector framework, but
> can also be used for DisplayPort connectors.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_bridge_connector.c | 130 ++++++++++++++++++++++++++++++++-
>  include/drm/drm_bridge.h               |  46 ++++++++++++
>  2 files changed, 174 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> index 0869b663f17e..71d6fdc2391f 100644
> --- a/drivers/gpu/drm/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/drm_bridge_connector.c
> @@ -20,6 +20,8 @@
>  #include <drm/drm_probe_helper.h>
>  #include <drm/display/drm_hdmi_state_helper.h>
>  
> +#include <sound/hdmi-codec.h>
> +
>  /**
>   * DOC: overview
>   *
> @@ -95,6 +97,14 @@ struct drm_bridge_connector {
>  	 * HDMI connector infrastructure, if any (see &DRM_BRIDGE_OP_HDMI).
>  	 */
>  	struct drm_bridge *bridge_hdmi;
> +	/**
> +	 * @bridge_hdmi_codec:
> +	 *
> +	 * The bridge in the chain that implements necessary support for the
> +	 * HDMI Audio Codec infrastructure, if any (see
> +	 * &DRM_BRIDGE_OP_HDMI_CODEC).
> +	 */
> +	struct drm_bridge *bridge_hdmi_codec;

Can we have a setup where one bridge would support the video stream and
another one the audio?

I think for now I'd rather make them both provided by the same bridge,
and we can always change that later on if we need to.

>  };
>  
>  #define to_drm_bridge_connector(x) \
> @@ -343,10 +353,104 @@ static int drm_bridge_connector_write_infoframe(struct drm_connector *connector,
>  	return bridge->funcs->hdmi_write_infoframe(bridge, type, buffer, len);
>  }
>  
> +static int drm_bridge_connector_audio_startup(struct device *dev, void *data)
> +{
> +	struct drm_connector *connector = data;
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_bridge *bridge;
> +
> +	bridge = bridge_connector->bridge_hdmi_codec;
> +	if (!bridge)
> +		return -EINVAL;
> +
> +	if (bridge->funcs->hdmi_codec_audio_startup)
> +		return bridge->funcs->hdmi_codec_audio_startup(dev, connector, bridge);
> +	else
> +		return 0;
> +}
> +
> +static int drm_bridge_connector_prepare(struct device *dev, void *data,
> +					struct hdmi_codec_daifmt *fmt,
> +					struct hdmi_codec_params *hparms)
> +{
> +	struct drm_connector *connector = data;
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_bridge *bridge;
> +
> +	bridge = bridge_connector->bridge_hdmi_codec;
> +	if (!bridge)
> +		return -EINVAL;
> +
> +	return bridge->funcs->hdmi_codec_prepare(dev, connector, bridge, fmt, hparms);
> +}
> +
> +static void drm_bridge_connector_audio_shutdown(struct device *dev, void *data)
> +{
> +	struct drm_connector *connector = data;
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_bridge *bridge;
> +
> +	bridge = bridge_connector->bridge_hdmi_codec;
> +	if (!bridge)
> +		return;
> +
> +	bridge->funcs->hdmi_codec_audio_shutdown(dev, connector, bridge);
> +}
> +
> +static int drm_bridge_connector_mute_stream(struct device *dev, void *data,
> +					    bool enable, int direction)
> +{
> +	struct drm_connector *connector = data;
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_bridge *bridge;
> +
> +	bridge = bridge_connector->bridge_hdmi_codec;
> +	if (!bridge)
> +		return -EINVAL;
> +
> +	if (bridge->funcs->hdmi_codec_mute_stream)
> +		return bridge->funcs->hdmi_codec_mute_stream(dev, connector, bridge,
> +							     enable, direction);
> +	else
> +		return -ENOTSUPP;
> +}
> +
> +static int drm_bridge_connector_get_dai_id(struct snd_soc_component *comment,
> +					   struct device_node *endpoint,
> +					   void *data)
> +{
> +	struct drm_connector *connector = data;
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_bridge *bridge;
> +
> +	bridge = bridge_connector->bridge_hdmi_codec;
> +	if (!bridge)
> +		return -EINVAL;
> +
> +	if (bridge->funcs->hdmi_codec_get_dai_id)
> +		return bridge->funcs->hdmi_codec_get_dai_id(connector, bridge, endpoint);
> +	else
> +		return -ENOTSUPP;
> +}
> +
> +static const struct hdmi_codec_ops drm_bridge_connector_hdmi_codec_ops = {
> +	.audio_startup = drm_bridge_connector_audio_startup,
> +	.prepare = drm_bridge_connector_prepare,
> +	.audio_shutdown = drm_bridge_connector_audio_shutdown,
> +	.mute_stream = drm_bridge_connector_mute_stream,
> +	.get_dai_id = drm_bridge_connector_get_dai_id,
> +};
> +
>  static const struct drm_connector_hdmi_funcs drm_bridge_connector_hdmi_funcs = {
>  	.tmds_char_rate_valid = drm_bridge_connector_tmds_char_rate_valid,
>  	.clear_infoframe = drm_bridge_connector_clear_infoframe,
>  	.write_infoframe = drm_bridge_connector_write_infoframe,
> +	.codec_ops = &drm_bridge_connector_hdmi_codec_ops,
>  };
>  
>  /* -----------------------------------------------------------------------------
> @@ -427,6 +531,23 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  				max_bpc = bridge->max_bpc;
>  		}
>  
> +		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CODEC) {
> +			if (bridge_connector->bridge_hdmi_codec)
> +				return ERR_PTR(-EBUSY);
> +			if (!bridge->funcs->hdmi_codec_prepare ||
> +			    !bridge->funcs->hdmi_codec_audio_shutdown)
> +				return ERR_PTR(-EINVAL);
> +
> +			bridge_connector->bridge_hdmi_codec = bridge;
> +
> +			connector->hdmi_codec.parent_dev = bridge->parent;
> +			connector->hdmi_codec.i2s = bridge->hdmi_codec_i2s;
> +			connector->hdmi_codec.spdif = bridge->hdmi_codec_spdif;
> +			connector->hdmi_codec.max_i2s_channels = bridge->max_i2s_channels;
> +
> +			bridge->hdmi_codec = &connector->hdmi_codec;
> +		}
> +
>  		if (!drm_bridge_get_next_bridge(bridge))
>  			connector_type = bridge->type;
>  
> @@ -448,7 +569,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> -	if (bridge_connector->bridge_hdmi)
> +	if (bridge_connector->bridge_hdmi) {
>  		ret = drmm_connector_hdmi_init(drm, connector,
>  					       bridge_connector->bridge_hdmi->vendor,
>  					       bridge_connector->bridge_hdmi->product,
> @@ -457,10 +578,15 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  					       connector_type, ddc,
>  					       supported_formats,
>  					       max_bpc);
> -	else
> +	} else {
>  		ret = drmm_connector_init(drm, connector,
>  					  &drm_bridge_connector_funcs,
>  					  connector_type, ddc);
> +		if (!ret && bridge_connector->bridge_hdmi_codec) {
> +			ret = drmm_connector_hdmi_codec_alloc(drm, connector,
> +							      &drm_bridge_connector_hdmi_codec_ops);
> +		}
> +	}
>  	if (ret) {
>  		kfree(bridge_connector);
>  		return ERR_PTR(ret);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 75019d16be64..c4a95c489b00 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -41,6 +41,8 @@ struct drm_display_info;
>  struct drm_minor;
>  struct drm_panel;
>  struct edid;
> +struct hdmi_codec_daifmt;
> +struct hdmi_codec_params;
>  struct i2c_adapter;
>  
>  /**
> @@ -676,6 +678,29 @@ struct drm_bridge_funcs {
>  				    enum hdmi_infoframe_type type,
>  				    const u8 *buffer, size_t len);
>  
> +	int (*hdmi_codec_audio_startup)(struct device *dev,
> +					struct drm_connector *connector,
> +					struct drm_bridge *bridge);
> +
> +	int (*hdmi_codec_prepare)(struct device *dev,
> +				  struct drm_connector *connector,
> +				  struct drm_bridge *bridge,
> +				  struct hdmi_codec_daifmt *fmt,
> +				  struct hdmi_codec_params *hparms);
> +
> +	void (*hdmi_codec_audio_shutdown)(struct device *dev,
> +					  struct drm_connector *connector,
> +					  struct drm_bridge *bridge);
> +
> +	int (*hdmi_codec_mute_stream)(struct device *dev,
> +				      struct drm_connector *connector,
> +				      struct drm_bridge *bridge,
> +				      bool enable, int direction);
> +
> +	int (*hdmi_codec_get_dai_id)(struct drm_connector *connector,
> +				     struct drm_bridge *bridge,
> +				     struct device_node *endpoint);
> +
>  	/**
>  	 * @debugfs_init:
>  	 *
> @@ -761,6 +786,20 @@ enum drm_bridge_ops {
>  	 * drivers.
>  	 */
>  	DRM_BRIDGE_OP_HDMI = BIT(4),
> +	/**
> +	 * @DRM_BRIDGE_OP_HDMI_CODEC: The bridge provides HDMI Audio Codec
> +	 * operations. Bridges that set this flag must implement the
> +	 * &drm_bridge_funcs->hdmi_codec_prepare and
> +	 * &drm_bridge_funcs->hdmi_codec_audio_shutdown callbacks and set the
> +	 * relevant field in the &drm_bridge structure.
> +	 *
> +	 * This framework can be used by both HDMI and DisplayPort bridges.
> +	 *
> +	 * Note: currently there can be at most one bridge in a chain that sets
> +	 * this bit. This is to simplify corresponding glue code in connector
> +	 * drivers.
> +	 */
> +	DRM_BRIDGE_OP_HDMI_CODEC = BIT(5),

I think I'd go and make it one step further and make HDMI_CODEC imply HDMI.

Maxime

Attachment: signature.asc
Description: PGP signature