Re: [PATCH RFC 3/5] drm/connector: implement generic HDMI codec helpers

[Maxime Ripard <mripard@xxxxxxxxxx>]

Hi,

Sorry for taking some time to review this series.

On Sat, Jun 15, 2024 at 08:53:32PM GMT, Dmitry Baryshkov wrote:
> Several DRM drivers implement HDMI codec support (despite its name it
> applies to both HDMI and DisplayPort drivers). Implement generic
> framework to be used by these drivers. This removes a requirement to
> implement get_eld() callback and provides default implementation for
> codec's plug handling.
> 
> The framework is integrated with the DRM HDMI Connector framework, but
> can be used by DisplayPort drivers.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/Makefile                   |   1 +
>  drivers/gpu/drm/drm_connector.c            |   8 ++
>  drivers/gpu/drm/drm_connector_hdmi_codec.c | 157 +++++++++++++++++++++++++++++
>  include/drm/drm_connector.h                |  33 ++++++
>  4 files changed, 199 insertions(+)
> 
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 68cc9258ffc4..e113a6eade23 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -45,6 +45,7 @@ drm-y := \
>  	drm_client_modeset.o \
>  	drm_color_mgmt.o \
>  	drm_connector.o \
> +	drm_connector_hdmi_codec.o \
>  	drm_crtc.o \
>  	drm_displayid.o \
>  	drm_drv.o \
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 3d73a981004c..66d6e9487339 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -279,6 +279,7 @@ static int __drm_connector_init(struct drm_device *dev,
>  	mutex_init(&connector->mutex);
>  	mutex_init(&connector->edid_override_mutex);
>  	mutex_init(&connector->hdmi.infoframes.lock);
> +	mutex_init(&connector->hdmi_codec.lock);
>  	connector->edid_blob_ptr = NULL;
>  	connector->epoch_counter = 0;
>  	connector->tile_blob_ptr = NULL;
> @@ -529,6 +530,12 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
>  
>  	connector->hdmi.funcs = hdmi_funcs;
>  
> +	if (connector->hdmi_codec.i2s || connector->hdmi_codec.spdif) {
> +		ret = drmm_connector_hdmi_codec_alloc(dev, connector, hdmi_funcs->codec_ops);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(drmm_connector_hdmi_init);
> @@ -665,6 +672,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  		connector->funcs->atomic_destroy_state(connector,
>  						       connector->state);
>  
> +	mutex_destroy(&connector->hdmi_codec.lock);
>  	mutex_destroy(&connector->hdmi.infoframes.lock);
>  	mutex_destroy(&connector->mutex);
>  
> diff --git a/drivers/gpu/drm/drm_connector_hdmi_codec.c b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> new file mode 100644
> index 000000000000..a3a7ad117f6f
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_connector_hdmi_codec.c
> @@ -0,0 +1,157 @@
> +/*
> + * Copyright (c) 2024 Linaro Ltd
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and its
> + * documentation for any purpose is hereby granted without fee, provided that
> + * the above copyright notice appear in all copies and that both that copyright
> + * notice and this permission notice appear in supporting documentation, and
> + * that the name of the copyright holders not be used in advertising or
> + * publicity pertaining to distribution of the software without specific,
> + * written prior permission.  The copyright holders make no representations
> + * about the suitability of this software for any purpose.  It is provided "as
> + * is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> + * OF THIS SOFTWARE.
> + */
> +
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +
> +#include <drm/drm_connector.h>
> +#include <drm/drm_managed.h>
> +
> +#include <sound/hdmi-codec.h>
> +
> +static int drm_connector_hdmi_codec_get_eld(struct device *dev, void *data,
> +					    uint8_t *buf, size_t len)
> +{
> +	struct drm_connector *connector = data;
> +
> +	//  FIXME: locking against drm_edid_to_eld ?
> +	memcpy(buf, connector->eld, min(sizeof(connector->eld), len));
> +
> +	return 0;
> +}
> +
> +static int drm_connector_hdmi_codec_hook_plugged_cb(struct device *dev,
> +						    void *data,
> +						    hdmi_codec_plugged_cb fn,
> +						    struct device *codec_dev)
> +{
> +	struct drm_connector *connector = data;
> +
> +	mutex_lock(&connector->hdmi_codec.lock);
> +
> +	connector->hdmi_codec.plugged_cb = fn;
> +	connector->hdmi_codec.plugged_cb_dev = codec_dev;
> +
> +	fn(codec_dev, connector->hdmi_codec.last_state);
> +
> +	mutex_unlock(&connector->hdmi_codec.lock);
> +
> +	return 0;
> +}
> +
> +void drm_connector_hdmi_codec_plugged_notify(struct drm_connector *connector,
> +					     bool plugged)
> +{
> +	mutex_lock(&connector->hdmi_codec.lock);
> +
> +	connector->hdmi_codec.last_state = plugged;
> +
> +	if (connector->hdmi_codec.plugged_cb &&
> +	    connector->hdmi_codec.plugged_cb_dev)
> +		connector->hdmi_codec.plugged_cb(connector->hdmi_codec.plugged_cb_dev,
> +						 connector->hdmi_codec.last_state);
> +
> +	mutex_unlock(&connector->hdmi_codec.lock);
> +}
> +EXPORT_SYMBOL(drm_connector_hdmi_codec_plugged_notify);

I think we should do this the other way around, or rather, like we do
for drm_connector_hdmi_init. We'll need a hotplug handler for multiple
things (CEC, HDMI 2.0, audio), so it would be best to have a single
function to call from drivers, that will perform whatever is needed
depending on the driver's capabilities.

So something like drm_connector_hdmi_handle_hotplug, which would then do
the above if there's audio support.

> +static void drm_connector_hdmi_codec_cleanup_action(struct drm_device *dev,
> +						    void *ptr)
> +{
> +	struct platform_device *pdev = ptr;
> +
> +	platform_device_unregister(pdev);
> +}
> +
> +/**
> + * drmm_connector_hdmi_alloc - Allocate HDMI Codec device for the DRM connector
> + * @dev: DRM device
> + * @connector: A pointer to the connector to allocate codec for
> + * @ops: callbacks for this connector
> + *
> + * Create a HDMI codec device to be used with the specified connector.
> + *
> + * Cleanup is automatically handled with in a DRM-managed action.
> + *
> + * The connector structure should be allocated with drmm_kzalloc().
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drmm_connector_hdmi_codec_alloc(struct drm_device *dev,
> +				    struct drm_connector *connector,
> +				    const struct hdmi_codec_ops *base_ops)
> +{
> +	struct hdmi_codec_pdata codec_pdata = {};
> +	struct platform_device *pdev;
> +	struct hdmi_codec_ops *ops;
> +	int ret;
> +
> +	ops = drmm_kmalloc(dev, sizeof(*ops), GFP_KERNEL);
> +	if (!ops)
> +		return -ENOMEM;

Do we actually need to allocate a new structure here?

> +	*ops = *base_ops;
> +
> +	ops->get_eld = drm_connector_hdmi_codec_get_eld;
> +	ops->hook_plugged_cb = drm_connector_hdmi_codec_hook_plugged_cb;
> +
> +	codec_pdata.ops = ops;
> +	codec_pdata.i2s = connector->hdmi_codec.i2s,
> +	codec_pdata.spdif = connector->hdmi_codec.spdif,
> +	codec_pdata.max_i2s_channels = connector->hdmi_codec.max_i2s_channels,
> +	codec_pdata.data = connector;
> +
> +	pdev = platform_device_register_data(connector->hdmi_codec.parent_dev,
> +					     HDMI_CODEC_DRV_NAME,
> +					     PLATFORM_DEVID_AUTO,
> +					     &codec_pdata, sizeof(codec_pdata));

I think parent_dev should be setup by drm_connector_hdmi_init. I guess
what I'm trying to say is that the reason HDMI support has been so
heterogenous is precisely because of the proliferation of functions they
needed to call, and so most drivers were doing the bare minimum until it
worked (or they encountered a bug).

What I was trying to do with the HDMI connector stuff was to make the
easiest approach the one that works according to the spec, for
everything.

Audio is optional, so it should be a togglable thing (either by an
additional function or parameter), but the drivers shouldn't have to set
everything more than what the function requires.

Also, parent_dev is going to be an issue there. IIRC, ASoC will set its
structure as the device data and overwrite whatever we put there.

We worked around it in vc4 by making sure that snd_soc_card was right at
the start of the driver structure and thus both pointers would be equal,
but we have to deal with it here too.

> +	if (IS_ERR(pdev))
> +		return PTR_ERR(pdev);
> +
> +	ret = drmm_add_action_or_reset(dev, drm_connector_hdmi_codec_cleanup_action, pdev);
> +	if (ret)
> +		return ret;
> +
> +	connector->hdmi_codec.codec_pdev = pdev;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drmm_connector_hdmi_codec_alloc);
> +
> +/**
> + * drmm_connector_hdmi_codec_free - rollback drmm_connector_hdmi_codec_alloc
> + * @dev: DRM device
> + * @hdmi_codec: A pointer to the HDMI codec data
> + *
> + * Rollback the drmm_connector_hdmi_codec_alloc() and free allocated data.
> + * While this function should not be necessary for a typical driver, DRM bridge
> + * drivers have to call it from the remove callback if the bridge uses
> + * Connector's HDMI Codec interface.
> + */
> +void drmm_connector_hdmi_codec_free(struct drm_device *dev,
> +				    struct drm_connector_hdmi_codec *hdmi_codec)
> +{
> +	drmm_release_action(dev, drm_connector_hdmi_codec_cleanup_action,
> +			    hdmi_codec->codec_pdev);
> +}

What would it be useful for?

> +EXPORT_SYMBOL(drmm_connector_hdmi_codec_free);
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index f750765d8fbc..0eb8d8ed9495 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -46,6 +46,7 @@ struct drm_property_blob;
>  struct drm_printer;
>  struct drm_privacy_screen;
>  struct edid;
> +struct hdmi_codec_ops;
>  struct i2c_adapter;
>  
>  enum drm_connector_force {
> @@ -1199,6 +1200,8 @@ struct drm_connector_hdmi_funcs {
>  	int (*write_infoframe)(struct drm_connector *connector,
>  			       enum hdmi_infoframe_type type,
>  			       const u8 *buffer, size_t len);
> +
> +	const struct hdmi_codec_ops *codec_ops;

I think I'd rather have the HDMI connector framework provide the ASoC
hooks, and make the needed pointer casts / lookups to provide a
consistent API to drivers using it.

This will probably also solve the issue mentioned above.

>  };
>  
>  /**
> @@ -1706,6 +1709,22 @@ struct drm_connector_hdmi {
>  	} infoframes;
>  };
>  
> +struct drm_connector_hdmi_codec {
> +	struct device *parent_dev;
> +	struct platform_device *codec_pdev;
> +
> +	const struct drm_connector_hdmi_codec_funcs *funcs;
> +
> +	struct mutex lock; /* protects last_state and plugged_cb */
> +	void (*plugged_cb)(struct device *dev, bool plugged);
> +	struct device *plugged_cb_dev;
> +	bool last_state;
> +
> +	int max_i2s_channels;
> +	uint i2s: 1;
> +	uint spdif: 1;
> +};

It would be great to have some documentation on what those are,
last_state and the mutex especially raise attention :)


>  /**
>   * struct drm_connector - central DRM connector control structure
>   *
> @@ -2119,6 +2138,12 @@ struct drm_connector {
>  	 * @hdmi: HDMI-related variable and properties.
>  	 */
>  	struct drm_connector_hdmi hdmi;
> +
> +	/**
> +	 * @hdmi_codec: HDMI codec properties and variables. Also might be used
> +	 * for DisplayPort audio.
> +	 */
> +	struct drm_connector_hdmi_codec hdmi_codec;

I'd rather make this part of drm_connector_hdmi, it cannot work without it.

Maxime

Attachment: signature.asc
Description: PGP signature