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.
--
With best wishes
Dmitry
[Index of Archives]
[Pulseaudio]
[Linux Audio Users]
[ALSA Devel]
[Fedora Desktop]
[Fedora SELinux]
[Big List of Linux Books]
[Yosemite News]
[KDE Users]
