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@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,
>>>>>
>>>>> 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@xxxxxxxxx/
Regards,
Jonas
>
> Maxime
