[alsa-devel] [Intel-gfx] [PATCH v4 1/5] drm/i915: setup bridge for HDMI LPE audio driver

[Anand, Jerome]

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
> Sent: Monday, January 23, 2017 4:24 PM
> To: Anand, Jerome <jerome.anand at intel.com>; intel-
> gfx at lists.freedesktop.org; alsa-devel at alsa-project.org
> Cc: tiwai at suse.de; broonie at kernel.org; Ughreja, Rakesh A
> <rakesh.a.ughreja at intel.com>
> Subject: Re: [Intel-gfx] [PATCH v4 1/5] drm/i915: setup bridge for HDMI LPE
> audio driver
> 
> On Sat, 21 Jan 2017, Jerome Anand <jerome.anand at intel.com> wrote:
> > Enable support for HDMI LPE audio mode on Baytrail and Cherrytrail
> > when HDaudio controller is not detected
> >
> > Setup minimum required resources during i915_driver_load:
> > 1. Create a platform device to share MMIO/IRQ resources 2. Make the
> > platform device child of i915 device for runtime PM.
> > 3. Create IRQ chip to forward HDMI LPE audio irqs.
> >
> > HDMI LPE audio driver (a standalone sound driver) probes the LPE audio
> > device and creates a new sound card.
> >
> > Signed-off-by: Pierre-Louis Bossart
> > <pierre-louis.bossart at linux.intel.com>
> > Signed-off-by: Jerome Anand <jerome.anand at intel.com>
> 
> Some comments inline. This is not a detailed technical review, more a high
> level glance at interfacing with the rest of i915.
> 

Sure - thanks for the quick review Jani

> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c index 4d22b4b..70d728b 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1131,7 +1131,8 @@ static void i915_driver_register(struct
> drm_i915_private *dev_priv)
> >  	if (IS_GEN5(dev_priv))
> >  		intel_gpu_ips_init(dev_priv);
> >
> > -	i915_audio_component_init(dev_priv);
> > +	if (intel_lpe_audio_init(dev_priv) < 0)
> > +		i915_audio_component_init(dev_priv);
> 
> I'd like this abstracted behind a single intel_audio_init() in intel_audio.c,
> which would do the right thing.
> 

Ok - I'll create two new API's in intel_audio.c

> >
> >  	/*
> >  	 * Some ports require correctly set-up hpd registers for detection
> > to @@ -1149,7 +1150,10 @@ static void i915_driver_register(struct
> drm_i915_private *dev_priv)
> >   */
> >  static void i915_driver_unregister(struct drm_i915_private *dev_priv)
> > {
> > -	i915_audio_component_cleanup(dev_priv);
> > +	if (HAS_LPE_AUDIO(dev_priv))
> > +		intel_lpe_audio_teardown(dev_priv);
> > +	else
> > +		i915_audio_component_cleanup(dev_priv);
> 
> Same here for cleanup.
> 

OK

> >
> >  	intel_gpu_ips_teardown();
> >  	acpi_video_unregister();
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h index f66eeede..cc7033a 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2449,6 +2449,12 @@ struct drm_i915_private {
> >  	/* Used to save the pipe-to-encoder mapping for audio */
> >  	struct intel_encoder *av_enc_map[I915_MAX_PIPES];
> >
> > +	/* necessary resource sharing with HDMI LPE audio driver. */
> > +	struct {
> > +		struct platform_device *platdev;
> > +		int	irq;
> > +	} lpe_audio;
> > +
> 
> I think the av_enc_map array is misplaced within dev_priv, and these should
> really be closer to the audio_component etc. members. But could be cleaned
> up later too.
> 

Will cleanup later.

> >  	/*
> >  	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your
> patch
> >  	 * will be rejected. Instead look for a better place.
> > @@ -2848,6 +2854,8 @@ intel_info(const struct drm_i915_private
> > *dev_priv)
> >
> >  #define HAS_POOLED_EU(dev_priv)	((dev_priv)->info.has_pooled_eu)
> >
> > +#define HAS_LPE_AUDIO(dev_priv) ((dev_priv)->lpe_audio.platdev !=
> > +NULL)
> 
> Please note that *all* of our HAS_ macros check for static features of the
> hardware, not dynamic state of the driver. Let's keep it that way.
> 

OK - will remove it from here.

> If you abstract the init/cleanup in intel_audio.c like I suggested, I think you
> can just check for dev_priv->lpe_audio.platdev directly, and get rid of
> HAS_LPE_AUDIO() altogether. Then it'll all be within intel_audio.c or
> intel_lpe_audio.c, and it's pretty obvious what it means.
> 

OK

> > +/**
> > + * intel_lpe_audio_detect() - check & setup lpe audio if present
> > + * @dev_priv: the i915 drm device private data
> > + *
> > + * Detect if lpe audio is present
> > + *
> > + * Return: true if lpe audio present else Return = false  */ bool
> > +intel_lpe_audio_detect(struct drm_i915_private *dev_priv)
> 
> Could this not be static?
> 

OK - wanted it to be as part of the interface. Not used anywhere now, so will make it static

> > +/**
> > + * intel_lpe_audio_setup() - setup the bridge between HDMI LPE Audio
> > + * driver and i915
> > + * @dev_priv: the i915 drm device private data
> > + *
> > + * set up the minimum required resources for the bridge: irq chip,
> > + * platform resource and platform device. i915 device is set as
> > +parent
> > + * of the new platform device.
> > + *
> > + * Return: 0 if successful. non-zero if allocation/initialization
> > +fails  */ int intel_lpe_audio_setup(struct drm_i915_private
> > +*dev_priv)
> 
> Could this not be static?
> 

OK

> BR,
> Jani.
> 
> 
> --
> Jani Nikula, Intel Open Source Technology Center