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

[Maxime Ripard <mripard@xxxxxxxxxx>]

On Wed, Jun 26, 2024 at 09:55:21PM GMT, Dmitry Baryshkov wrote:
> On Wed, Jun 26, 2024 at 06:07:50PM GMT, Dave Stevenson wrote:
> > 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.
> 
> Hmm, I should check how this is handled on the standard desktops. With
> the DisplayPort and link training it might take a significant amount of
> time to switch the mode.
> 
> > 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.
> 
> I guess so. With msm/hdmi and with msm/dp we should be getting an error
> when the video is turned off. I don't remember if it is an immediate
> error or something that happens at the end of the period. Adding Srini,
> our audio expert, he should know it better.
> 
> For external HDMI bridges I completely have no idea, but I guess we
> don't need to worry too much, as they are just taking I2S or SPDIF audio
> from the bus.
> 
> In the worst case we conclude that the calling point is driver-dependent
> and as such it is not suitable to call the plugged callback from the
> drm_bridge_connector.

It would really surprise me here: the spec will require something, ALSA
will require something else, and we'll have to bridge the gap, but
there's nothing really device specific here, it's all software.

Maxime

Attachment: signature.asc
Description: PGP signature