Message-ID: <90607371-2826-4448-a459-9b8a1cf6079f@kwiboo.se>
Date: Thu, 25 Jul 2024 13:06:35 +0200
From: Jonas Karlman <jonas@...boo.se>
To: Maxime Ripard <mripard@...nel.org>, Dmitry Baryshkov
 <dmitry.baryshkov@...aro.org>
Cc: 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 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.
> 
>> - 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.
> 
> 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.

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.

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

Regards,
Jonas

> 
> Maxime

Powered by blists - more mailing lists