Re: [PATCH v5 8/9] drm/vc4: hdmi: switch to using generic HDMI Codec infrastructure

[Maxime Ripard]

On Tue, Dec 03, 2024 at 02:19:41PM +0200, Dmitry Baryshkov wrote:
> On Mon, Dec 02, 2024 at 02:20:04PM +0100, Maxime Ripard wrote:
> > Hi,
> > 
> > Sorry, I've been drowning under work and couldn't review that series before.
> 
> No worries, at this point I'm more concerned about my IGT series rather
> than this one.
> 
> > 
> > I'll review the driver API for now, and we can focus on the exact
> > implementation later on.
> > 
> > On Sun, Dec 01, 2024 at 02:44:12AM +0200, Dmitry Baryshkov wrote:
> > > Drop driver-specific implementation and use the generic HDMI Codec
> > > framework in order to implement the HDMI audio support.
> > > 
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/vc4/vc4_hdmi.c | 68 ++++++++++--------------------------------
> > >  drivers/gpu/drm/vc4/vc4_hdmi.h |  2 --
> > >  2 files changed, 15 insertions(+), 55 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > index 7295834e75fb1ab0cd241ed274e675567e66870b..d0a9aff7ad43016647493263c00d593296a1e3ad 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > @@ -595,6 +595,9 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
> > >  	if (vc4_hdmi->variant->supports_hdr)
> > >  		max_bpc = 12;
> > >  
> > > +	connector->hdmi_codec.max_i2s_channels = 8;
> > > +	connector->hdmi_codec.i2s = 1;
> > > +
> > 
> > I guess it's a similar discussion than we had with HDMI2.0+ earlier
> > today, but I don't really like initializing by structs. Struct fields
> > are easy to miss, and can be easily uninitialized by mistake.
> > 
> > I think I'd prefer to have them as argument to the init function. And if
> > they are optional, we can explicitly mark them as unused.
> 
> Do you mean drm_connector_hdmi_init()? I think it's overloaded already,
> but I defintely can think about:
> 
> drmm_connector_hdmi_init(..., max_bpc, HDMI_CODEC_I2S_PLAYBACK(8) |
> HDMI_CODEC_NO_CAPTURE | HDMI_CODEC_DAI_ID(4));
> 
> or
> 
> ... | HDMI_CODEC_NO_DAI_ID)
> 
> The default (0) being equivalent to:
> 
> HDMI_CODEC_NO_I2S | HDMI_CODEC_NO_SPDIF | HDMI_CODEC_NO_CAPTURE | HDMI_CODEC_NO_DAI_ID
> 
> WDYT?

I know it's kind of contradictory, but it definitely looks overcrowded.

A bit after we merged the HDMI infrastructure, Thomas commented that it
might have been better to have a secondary init function instead of an
alloc/init function.

https://lore.kernel.org/all/5934b4b2-3a99-4b6b-b3e3-e57eb82b9b16@xxxxxxx/

It's still sitting in my inbox and haven't had the time to work on that,
but maybe that's how we should deal with this?

Switch to using drm_connector_init, then drm_connector_hdmi_init would
only take care of the video stuff, and we could have an additional
drm_connector_hdmi_audio_init?

That way, we could have both explicit stuff, and yet not overcrowd the
arguments list too much?

Maxime

Attachment: signature.asc
Description: PGP signature