[Intel-gfx] [PATCH] drm/i915/cnl: Enable Audio Pin Buffer.

[Pandiyan, Dhinakaran]

On Thu, 2017-08-17 at 18:55 +0000, Vivi, Rodrigo wrote:
> On Thu, 2017-08-17 at 09:26 +0530, Sanyog Kale wrote:
> > On Wed, Aug 16, 2017 at 06:46:26PM -0500, Pandiyan, Dhinakaran wrote:
> > > On Thu, 2017-07-06 at 14:03 -0700, Rodrigo Vivi wrote:
> > > > Starting on CNL, we need to enable Audio Pin Buffer.
> > > > 
> > > > By the spec it seems that this is part of audio programming,
> > > 
> > > I am not very clear where the pin buffer enabling/disabling step falls
> > > in the audio programming sequence. From what I understand, audio codec
> > > enable/disable is controlled by i915, if pin buffer enable/disable is
> > > part of that, then we don't need a call back. It's difficult to know
> > > where this function is going to be used without seeing the audio driver
> > > patch that makes of use of this.
> 
> Sanyog, I couldn't find all the old emails with spec pointing out the
> time that we needed to set this bit. Do you still have?
> Anything you could share when publishing the patches?
> 
> > >
> > 
> > From audio point of view, we are working on correct sequence where this ops
> > will be called. However during CNL Power-ON we enabled the pin buf using ops
> > as part of azx_reset.
> 
> Ok, so let's put this on hold while audio drivers are not ready.
> It will be on internal branch anyways.
> When audio driver gets ready we re-submit all together.
> 

That makes a lot of sense to me.

> >  
> > > > so let's give them the hability to set/unset this as needed.
> > > 
> > > typo s/hability/ability
> > > > 
> > > > v2: With a hook so audio driver can control it.
> > > > v3: Put back reg definition lost on v2.
> > > > 
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > > Cc: Jani Nikula <jani.nikula at intel.com>
> > > > Cc: Sanyog Kale <sanyog.r.kale at intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_reg.h    |  3 +++
> > > >  drivers/gpu/drm/i915/intel_audio.c | 16 ++++++++++++++++
> > > >  include/drm/i915_component.h       |  6 ++++++
> > > >  3 files changed, 25 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 64cc674..aab38da 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -2643,6 +2643,9 @@ enum skl_disp_power_wells {
> > > >  #define I915_HDMI_LPE_AUDIO_BASE	(VLV_DISPLAY_BASE + 0x65000)
> > > >  #define I915_HDMI_LPE_AUDIO_SIZE	0x1000
> > > >  
> > > > +#define AUDIO_PIN_BUF_CTL		_MMIO(0x48414)
> > > > +#define AUDIO_PIN_BUF_ENABLE		(1 << 31)
> > > > +
> > > >  /* DisplayPort Audio w/ LPE */
> > > >  #define VLV_AUD_CHICKEN_BIT_REG		_MMIO(VLV_DISPLAY_BASE + 0x62F38)
> > > >  #define VLV_CHICKEN_BIT_DBG_ENABLE	(1 << 0)
> > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > > > index d805b6e..0c83254 100644
> > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > @@ -865,6 +865,21 @@ static int i915_audio_component_get_eld(struct device *kdev, int port,
> > > >  	return ret;
> > > >  }
> > > >  
> > > > +static void i915_audio_component_pin_buf(struct device *kdev, bool enable)
> > > > +{
> > > 
> > > Does it matter whether the audio codec is enabled or disabled when this
> > > call comes in? i.e., do we need some sort of check here? Or do we assume
> > > the audio driver knows exactly when to enable or disable pin buffer?
> > > 
> > > 
> > > 
> > > > +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> > > > +
> > > > +	if (!IS_CANNONLAKE(dev_priv))
> > > 
> > > Should this be a INTEL_GEN() < 10 since the register is gen10+? I am not
> > > particular about either of the options.
> > > 
> > > > +		return;
> > > > +
> > > > +	if (enable)
> > > > +		I915_WRITE(AUDIO_PIN_BUF_CTL, I915_READ(AUDIO_PIN_BUF_CTL) |
> > > > +			   AUDIO_PIN_BUF_ENABLE);
> > > > +	else
> > > > +		I915_WRITE(AUDIO_PIN_BUF_CTL, I915_READ(AUDIO_PIN_BUF_CTL) &
> > > > +			   ~AUDIO_PIN_BUF_ENABLE);
> > > > +}
> > > > +
> > > >  static const struct i915_audio_component_ops i915_audio_component_ops = {
> > > >  	.owner		= THIS_MODULE,
> > > >  	.get_power	= i915_audio_component_get_power,
> > > > @@ -873,6 +888,7 @@ static int i915_audio_component_get_eld(struct device *kdev, int port,
> > > >  	.get_cdclk_freq	= i915_audio_component_get_cdclk_freq,
> > > >  	.sync_audio_rate = i915_audio_component_sync_audio_rate,
> > > >  	.get_eld	= i915_audio_component_get_eld,
> > > > +	.pin_buf	= i915_audio_component_pin_buf,
> > > >  };
> > > >  
> > > >  static int i915_audio_component_bind(struct device *i915_kdev,
> > > > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> > > > index 545c6e0..b8875d4 100644
> > > > --- a/include/drm/i915_component.h
> > > > +++ b/include/drm/i915_component.h
> > > > @@ -79,6 +79,12 @@ struct i915_audio_component_ops {
> > > >  	 */
> > > >  	int (*get_eld)(struct device *, int port, int pipe, bool *enabled,
> > > >  		       unsigned char *buf, int max_bytes);
> > > > +	/**
> > > > +	 * @pin_buf: Enable or disable pin buffer.
> > > > +	 *
> > > > +	 * Allow audio driver the toggle pin buffer.
> > > > +	 */
> > > > +	void (*pin_buf)(struct device *, bool enable);
> > > >  };
> > > >  
> > > >  /**
> > 
>