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

[Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>]

Hi Dmitry

On Wed, 26 Jun 2024 at 17:11, Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> 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?

We hit it in trying to get Pipewire to do the right thing on
hotplugging HDMI cables, and updating the lists of available audio
destinations in desktop plugins.
My memory is a little hazy, but I seem to recall the logic was that
whilst changing audio destination when you unplug the currently
selected HDMI output is reasonable, but doing so because you changed
resolution or the screen saver kicked in was less user friendly.
mtk_hdmi was used as a basis for the patch, although it's all largely
boilerplate anyway.

Yes the audio has to stop on enable/disable as HDMI video dictates all
the timings.
I've just checked with aplay playing audio and kmstest to change video
mode - audio pauses as it is disabled and resumes when the new mode is
selected.
One observation that I can't immediately explain is that if I use
kmstest to disable the HDMI display that is playing the audio, aplay
still completes without any errors logged. Using "time" on aplay is
returning the same duration for the playback whether the HDMI output
is enabled or not. That may be down to the vc4 hardware with the HDMI
FIFO accepting the data at the correct rate whether the video side is
enabled or not, but that is just a guess.

  Dave

> >
> >   Dave
> >
> > [1] https://github.com/raspberrypi/linux/commit/051392bfdc6dc54563ed9909cc1164e8d734af43
> >
>
>
> --
> With best wishes
> Dmitry