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

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

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@xxxxxxxxxx> 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(+)
> > > > >

[...]

> > > > > +
> > > > > +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