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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <r5226a5zzbp2d7zmxbskll6ed7coy4qbxhd6aaqulqzyuom6sp@dliwhreaqmtl>
Date: Wed, 26 Jun 2024 19:11:17 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Dave Stevenson <dave.stevenson@...pberrypi.com>
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>, 
	Jonas Karlman <jonas@...boo.se>, 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 Wed, Jun 26, 2024 at 04:10:05PM GMT, Dave Stevenson wrote:
> Hi Maxime and Dmitry
> 
> On Wed, 26 Jun 2024 at 15:05, Maxime Ripard <mripard@...nel.org> 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(+)
> > > > >

[...]

> > > > > +
> > > > > +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.
> 
> FWIW I have a patch in the next set that adds the call to vc4. The
> downstream version of the patch is at [1].

Nice!

> > 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.
> 
> We concluded that hotplug detect appeared to be the right place as well.

Probably you also stumbled upon hotplug vs enable/disable. Could you
please comment, why you made your decision towards hotplug path?

> 
>   Dave
> 
> [1] https://github.com/raspberrypi/linux/commit/051392bfdc6dc54563ed9909cc1164e8d734af43
> 


-- 
With best wishes
Dmitry