On Fri, 21 Jun 2024 at 12:27, Maxime Ripard <mripard@xxxxxxxxxx> wrote:
>
> Hi,
>
> Sorry for taking some time to review this series.
No problem, that's not long.
>
> 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.
I see, this API is probably misnamed. The hdmi_codec_ops use the
'plugged' term, but most of the drivers notify the ASoC / codec during
atomic_enable / atomic_disable path, because usually the audio path
can not work with the video path being disabled. I'll rename this
function to something like ..hdmi_codec_enable. or ...
hdmi_codec_set_enabled.
>
> 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?
I didn't want to change the hdmi-codec's logic too much. But maybe
it's really better to have generic ops implementation here that calls
into the driver-specific callbacks.
> > + *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.
I'll see what I can do. I had more or less the same goals, being hit
by the lack of the plugged_cb and get_eld support in the bridge's
implementation.
> 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.
It registers driver_data for the created device, it doesn't touch parent_dev.
>
> 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.
Hmm, maybe I'm missing something. The snd_soc_card is a different
story. The bridges just provide the hdmi_codec_ops, the card itself is
handled by the other driver.
>
> > + 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?
See the last patch,
https://lore.kernel.org/dri-devel/20240615-drm-bridge-hdmi-connector-v1-5-d59fc7865ab2@xxxxxxxxxx/
if the bridge driver gets unbound, we should also unregister the codec
device. The codec infrastructure uses drmm to allocate data and a drmm
action to unregister the codec device. However the bridge drivers are
not bound by the drmm lifecycle. So we have to do that manually.
>
> > +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.
Ack.
>
> > };
> >
> > /**
> > @@ -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 :)
Yep, as I wrote in the cover letter, underdocumented.
>
>
> > /**
> > * 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.
It can. DisplayPort drivers also use hdmi_codec_ops. They should be
able to benefit from this implementation.
--
With best wishes
Dmitry
