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
