Message-ID: <nwwucz3nrne5uv626qhtxozsztl76ygcrxlqtnop7wqreb46kc@3akiwafcdaun>
Date: Sat, 27 Jul 2024 14:34:54 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Jonas Karlman <jonas@...boo.se>
Cc: Maxime Ripard <mripard@...nel.org>, 
	Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>, 
	Robert Foss <rfoss@...nel.org>, Laurent Pinchart <Laurent.pinchart@...asonboard.com>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, 
	Daniel Vetter <daniel@...ll.ch>, Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>, 
	Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org, linux-sound@...r.kernel.org
Subject: Re: [PATCH RFC 3/5] drm/connector: implement generic HDMI codec
 helpers

On Thu, Jul 25, 2024 at 01:06:35PM GMT, Jonas Karlman wrote:
> On 2024-07-25 11:25, Maxime Ripard wrote:
> > On Thu, Jun 27, 2024 at 04:29:49PM GMT, Dmitry Baryshkov wrote:
> >> On Thu, Jun 27, 2024 at 11:49:37AM GMT, Maxime Ripard wrote:
> >>> On Wed, Jun 26, 2024 at 07:09:34PM GMT, Dmitry Baryshkov wrote:
> >>>> On Wed, Jun 26, 2024 at 04:05:01PM GMT, Maxime Ripard wrote:
> >>>>> On Fri, Jun 21, 2024 at 02:09:04PM GMT, Dmitry Baryshkov wrote:
> >>>>>> On Fri, 21 Jun 2024 at 12:27, Maxime Ripard <mripard@...nel.org> 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@...aro.org>
> >>>>>>>> ---
> >>>>>>>>  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,
> >>>>>
> >>>>> Is it misnamed?
> >>>>>
> >>>>> It's documented as:
> >>>>>
> >>>>>   Hook callback function to handle connector plug event. Optional.
> >>>>>
> >>>>>> 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.
> >>>>>
> >>>>> That's not clear to me either:
> >>>>>
> >>>>>   - rockchip/cdn-dp, msm/dp/dp-audio, dw-hdmi, seem to call it at
> >>>>>     enable/disable
> >>>>>
> >>>>>   - anx7625, mtk_hdmi and mtk_dp calls it in detect
> >>>>>
> >>>>>   - adv7511, ite-it66121, lontium-lt9611, lontium-lt9611uxc, sii902x,
> >>>>>     exynos, tda998x, msm_hdmi, sti, tegra, vc4 don't call it at all.
> >>>>>
> >>>>> So it doesn't look like there's a majority we can align with, and
> >>>>> neither should we: we need to figure out what we *need* to do and when,
> >>>>> and do that.
> >>>>>
> >>>>> From the documentation and quickly through the code though, handling it
> >>>>> in detect looks like the right call.
> >>>>
> >>>> It is tempting to have it in the hotplug call. However:
> >>>>
> >>>> - It is used to send events to the ASoC Jack, marking the output as
> >>>>   plugged or unplugged. Once the output is plugged, userspace might
> >>>>   consider using it for the audio output. Please correct me if I'm
> >>>>   wrong, but I don't think one can output audio to the HDMI plug unless
> >>>>   there is a video stream.
> >>>
> >>> That's something to check in the HDMI spec and with the ALSA
> >>> maintainers.
> >>
> >> Mark and Liam are on CC list. I've also pinged Mark on the IRC (on
> >> #alsa, if the channel logs are preserved somewhere)
> >>
> >> <lumag> I'm trying to implement a somewhat generic implementation that the drivers can hook in. The main discussion is at [link to this discussion]
> >> <lumag> So in theory that affects all ASoC platforms having HDMI or DP audio output
> >> <broonie> In that case I'd be conservative and try to follow the state of the physical connection as closely as possible.
> >>
> >> So it is really 'plugged'.
> > 
> > Ack.
> > 
> >>>> - Having it in the hotplug notification chain is also troublesome. As
> >>>>   Dave pointed out in the quoted piece of code, it should come after
> >>>>   reading the EDID on the connect event. On the disconnect event it
> >>>>   should probably come before calling the notification chain, to let
> >>>>   audio code interract correctly with the fully enabled display devices.
> >>>
> >>> EDIDs are fetched when hotplug is detected anyway, and we need it for
> >>> other things anyway (like CEC).
> >>
> >> I see that:
> >>
> >> - VC4 reads EDID and sets CEC address directly in hotplug notifier and
> >>   then again in get_modes callback. (why is it necessary in the hotplug
> >>   notifier, if it's done anyway in get_modes?)
> > 
> > vc4 is probably somewhat correct, but also a bit redundant here: the CEC
> > physical address is supposed to be set when we detect a sink.
> > 
> > When that sink is removed, we don't have a physical address anymore and
> > we thus need to call cec_phys_addr_invalidate.
> > 
> > When a sink is plugged again, we need to call cec_s_phys_addr() with the
> > sink address.
> > 
> > So what vc4_hdmi_handle_hotplug is doing is fine, but the one in the
> > get_modes might be redundant.

I see the logic here, but I don't like the duplication between
detect_ctx() and get_modes(). Doesn't it make drm_edid_read_ddc() in
get_modes() redundant?

> >> - sun4i sets CEC address from get_modes
> > 
> > Yeah, that doesn't work. 
> > 
> > If the display is switched to another one and if the userspace doesn't
> > react to the hotplug event by calling get_modes, the physical address
> > will be the old one.

Ack.

> > 
> > But since it's a polled hotplug, it's a situation that generally sucks.
> > 
> >> - ADV7511 does a trick and sets CEC address from edid_read() callback
> >>   (with the FIXME from Jani that basically tells us to move this to
> >>   get_modes)
> > 
> > Same situation than sun4i here, except for the fact that it can handle
> > hotplug through an interrupt.
> > 
> >> - omapdrm clears CEC address from hpd_notify, but sets it from the
> >>   edid_read() callback with the same FIXME.
> > 
> > I think it's still broken there. It properly clears the physical address
> > when the sink is disconnected, but relies on someone calling get_modes
> > to set it again, which isn't guaranteed.
> > 
> >> - i915 sets CEC address from .detect_ctx callback
> > 
> > That one is vc4 minus the get_modes redundancy.
> > 
> >> So there is no uniformity too. Handling it from drm_bridge_connector() /
> >> get_modes might help, but that requires clearing one of TODO items.
> > 
> > There's no uniformity, but I guess both vc4 and i915 are much more
> > battle-tested than sun4i, omapdrm, or adv7511 might be, and they both
> > behave pretty much the same.
> > 
> > Generally speaking (and it might be sad), but i915 is what userspace
> > expects, so it's usually what we want to follow.
> 
> For the dw-hdmi connector I also moved the edid read and cec phys addr
> handling from hpd irq to the detect callback in [1]. That seem to be the
> best option as it gets called by core after/during hpd processing and
> also more closely matches i915.

Yes, it seems that we should settle on this (and maybe document it as
the BCP).

> Was not fully sure what to do about the hdmi-codec callback so left it
> at enable/disable as that seemed to best match the expected plugged
> state. However, dw-hdmi connector will more than likely trigger a
> disconnected/connected state change and a disable/enable cycle during
> an edid refresh from the sink, i.e. when a connected AVR is powered on
> or off, so it does now always match the physical plugged state.

If I understood Mark correctly, the plugged callback should really
reflect the 'plugged' state (well, probably after reading the EDID to
make ELD available to ASoC subsys). Maybe we should have a separate
control for making the sink available for audio output, but nobody has
that yet.

> [1] https://lore.kernel.org/all/20240611155108.1436502-1-jonas@kwiboo.se/

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists