Re: [PATCH v5 2/2] drm/msm/dp: reuse generic HDMI codec implementation

[Dmitry Baryshkov]

On Fri, Mar 14, 2025 at 08:53:34AM +0100, Maxime Ripard wrote:
> On Tue, Mar 11, 2025 at 05:58:19PM +0200, Dmitry Baryshkov wrote:
> > On Tue, Mar 11, 2025 at 09:41:13AM +0100, Maxime Ripard wrote:
> > > Hi,
> > > 
> > > On Mon, Mar 10, 2025 at 08:53:24PM +0200, Dmitry Baryshkov wrote:
> > > > On Mon, 10 Mar 2025 at 17:08, Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Fri, Mar 07, 2025 at 07:55:53AM +0200, Dmitry Baryshkov wrote:
> > > > > > From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > > >
> > > > > > The MSM DisplayPort driver implements several HDMI codec functions
> > > > > > in the driver, e.g. it manually manages HDMI codec device registration,
> > > > > > returning ELD and plugged_cb support. In order to reduce code
> > > > > > duplication reuse drm_hdmi_audio_* helpers and drm_bridge_connector
> > > > > > integration.
> > > > > >
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > > > ---
> > > > > >  drivers/gpu/drm/msm/Kconfig         |   1 +
> > > > > >  drivers/gpu/drm/msm/dp/dp_audio.c   | 131 ++++--------------------------------
> > > > > >  drivers/gpu/drm/msm/dp/dp_audio.h   |  27 ++------
> > > > > >  drivers/gpu/drm/msm/dp/dp_display.c |  28 ++------
> > > > > >  drivers/gpu/drm/msm/dp/dp_display.h |   6 --
> > > > > >  drivers/gpu/drm/msm/dp/dp_drm.c     |   8 +++
> > > > > >  6 files changed, 31 insertions(+), 170 deletions(-)
> > > > > >
> > 
> > [...]
> > 
> > > > > >
> > > > > >  static int msm_edp_bridge_atomic_check(struct drm_bridge *drm_bridge,
> > > > > > @@ -320,9 +324,13 @@ int msm_dp_bridge_init(struct msm_dp *msm_dp_display, struct drm_device *dev,
> > > > > >        */
> > > > > >       if (!msm_dp_display->is_edp) {
> > > > > >               bridge->ops =
> > > > > > +                     DRM_BRIDGE_OP_HDMI_AUDIO |
> > > > > >                       DRM_BRIDGE_OP_DETECT |
> > > > > >                       DRM_BRIDGE_OP_HPD |
> > > > > >                       DRM_BRIDGE_OP_MODES;
> > > > > > +             bridge->hdmi_audio_dev = &msm_dp_display->pdev->dev;
> > > > > > +             bridge->hdmi_audio_max_i2s_playback_channels = 8;
> > > > > > +             bridge->hdmi_audio_dai_port = -1;
> > > > > >       }
> > > > >
> > > > > I think I'd prefer the toggle to be OP_DP_AUDIO, even if the
> > > > > implementation is exactly the same. That way, we'll be able to condition
> > > > > it to the DP support when that arrives, and we have the latitude to
> > > > > rework it to accomodate some DP subtleties without affecting the drivers
> > > > > later on.
> > > > 
> > > > I don't think that there is a point in having OP_DP_AUDIO. There is
> > > > not so much difference in the driver. Also currently OP_HDMI_AUDIO
> > > > follows existing approach (which was pointed out by Laurent) - that
> > > > OP_foo should guard a particular set of callbacks. From this
> > > > perspective, OP_HDMI_AUDIO is fine - it guards usage of
> > > > hdmi_audio_foo(). OP_DP_AUDIO would duplicate that.
> > > 
> > > HDMI and DP are two competing standards, with different governing
> > > bodies. I don't think either have claimed that they will strictly adhere
> > > to what the other is doing, and I don't have the will to cross-check
> > > every given audio feature in both HDMI and DP right now.
> > 
> > Hmm. Currently (or before the first hdmi_audio patchset) everybody has
> > been plumbing hdmi-codec directly from the driver (even for DP audio).
> 
> We also didn't have an infrastructure for that before, so it's to be
> expected.
> 
> > > However, I think we should really have the flexibility to deal with that
> > > situation if it happens, and without having to do any major refactoring.
> > > That means providing an API that is consistent to the drivers, and
> > > provides what the driver needs. Here, it needs DP audio support, not
> > > HDMI's.
> > 
> > Would OP_HDMI_CODEC be a better name for the OP? (we can rename the
> > existing callbacks to be hdmi_codec instead of hdmi_audio too).
> 
> As said on your first patch, there's more to it than just the codec, so
> no, the current name is fine to me.
> 
> > > How we plumb it is an implementation detail, and I do agree we can use
> > > the same functions under the hood right now. But the driver is a DP
> > > driver, it wants DP infrastructure and DP audio support.
> > 
> > Would OP_DP_AUDIO require a different set of callbacks on the bridge
> > level?
> 
> Yes.

Okay, this answers my question in the thread for the first patch. Let's
go this way. Maybe once we have more of the infrastructure
implementation, these two sets will separate more and more, meaning that
you were right.

> 
> > I don't want to end up with too much of duplication. Maybe we
> > should the cdns bridges which implement both HDMI and DP functionality
> > IIRC.
> 
> We can (and we should) take the same prototype for both though, so
> drivers that have the same implementation can provide the same
> implementation to both.

Ack.


-- 
With best wishes
Dmitry