Re: [RFC PATCH 4/5] drm: i915: add DisplayPort amp unmute for LPE audio mode

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Jan 27, 2017 at 12:08:58PM +0200, Jani Nikula wrote:
> On Thu, 26 Jan 2017, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:
> > Enable chicken bit on LPE mode setup and unmute amp on
> > notification
> >
> > FIXME: should these two phases done somewhere else?
> >
> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h        | 12 ++++++++++++
> >  drivers/gpu/drm/i915/intel_lpe_audio.c | 27 +++++++++++++++++++++++++++
> >  2 files changed, 39 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index a9ffc8d..ee90f64 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -2061,6 +2061,18 @@ enum skl_disp_power_wells {
> >  #define I915_HDMI_LPE_AUDIO_BASE	(VLV_DISPLAY_BASE + 0x65000)
> >  #define I915_HDMI_LPE_AUDIO_SIZE	0x1000
> >  
> > +/* DisplayPort Audio w/ LPE */
> > +#define CHICKEN_BIT_DBG_ENABLE		(1 << 0)
> > +#define AMP_UNMUTE		        (1 << 1)

That should be called AMP_MUTE I think,

> 
> The convention is to define registers first and the contents/bits for
> each register immedialy below. For groups of registers (like
> PORT_EN_B/C/D below) define all registers first and bits immediately
> below. (But note that the chicken register is not part of the group.)
> 
> > +#define AUD_CHICKEN_BIT_REG		0x62F38

Spec calls this AUD_CHICKENBIT_REG. Might as well follow it to the
letter.

> > +#define AUD_PORT_EN_B_DBG		0x62F20
> > +#define AUD_PORT_EN_C_DBG		0x62F28
> > +#define AUD_PORT_EN_D_DBG		0x62F2C

These match the spec. But to match the standard i915 convention they
should be called _AUD_PORT_EN_B_DBG etc. Same forthe chicken bit
register.

> 
> Please don't define these separately without the display base, use
> (VLV_DISPLAY_BASE + 0x62f38) style instead. All the other uses of
> VLV_DISPLAY_BASE in the file follow the same convention.
> 
> > +#define VLV_AUD_CHICKEN_BIT_REG		_MMIO(VLV_DISPLAY_BASE + AUD_CHICKEN_BIT_REG)
> > +#define VLV_AUD_PORT_EN_B_DBG		_MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_B_DBG)
> > +#define VLV_AUD_PORT_EN_C_DBG		_MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_C_DBG)
> > +#define VLV_AUD_PORT_EN_D_DBG		_MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_D_DBG)
> > +
> 
> Would be nice to have a macro VLV_AUD_PORT_EN_DBG(port), but damn those
> reg offsets don't make it easy...

_MMIO_PORT3().

> 
> 
> >  #define GEN6_BSD_RNCID			_MMIO(0x12198)
> >  
> >  #define GEN7_FF_THREAD_MODE		_MMIO(0x20a0)
> > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > index 245523e..b3134ef 100644
> > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > @@ -248,6 +248,15 @@ static int lpe_audio_setup(struct drm_i915_private *dev_priv)
> >  		goto err_free_irq;
> >  	}
> >  
> > +	/* Enable DPAudio debug bits by default */
> > +	if (IS_CHERRYVIEW(dev_priv)) {

VLV too. And like I said we might need this in the powerwell code as
well. You should make a test to see if the register value is retained
across the display power well being turned off. Eg. simply disable all
displays, check the log to make sure it really did turn off the display
power well, then re-enable some displays, and finally check if the
register value was retained or not).

> > +		u32 chicken_bit;
> > +
> > +		chicken_bit = I915_READ(VLV_AUD_CHICKEN_BIT_REG);
> > +		I915_WRITE(VLV_AUD_CHICKEN_BIT_REG,
> > +			   chicken_bit | CHICKEN_BIT_DBG_ENABLE);
> > +	}
> > +
> >  	return 0;
> >  err_free_irq:
> >  	irq_free_desc(dev_priv->lpe_audio.irq);
> > @@ -357,6 +366,24 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> >  			pdata->tmds_clock_speed = tmds_clk_speed;
> >  		if (link_rate)
> >  			pdata->link_rate = link_rate;
> > +
> > +		if (dp_output) { /* unmute the amp */

The spec doesn't distinquish DP vs. HDMI here. So I presume we should be
able to do this always.

And I think we might want to mute things again when disabling audio.

> > +			u32 audio_enable;
> > +
> > +			if (port == PORT_B) {
> > +				audio_enable = I915_READ(VLV_AUD_PORT_EN_B_DBG);
> > +				I915_WRITE(VLV_AUD_PORT_EN_B_DBG,
> > +					   audio_enable & ~AMP_UNMUTE);
> > +			} else if (port == PORT_C) {
> > +				audio_enable = I915_READ(VLV_AUD_PORT_EN_C_DBG);
> > +				I915_WRITE(VLV_AUD_PORT_EN_C_DBG,
> > +					   audio_enable & ~AMP_UNMUTE);
> > +			} else if (port == PORT_D) {
> > +				audio_enable = I915_READ(VLV_AUD_PORT_EN_D_DBG);
> > +				I915_WRITE(VLV_AUD_PORT_EN_D_DBG,
> > +					   audio_enable & ~AMP_UNMUTE);
> > +			}
> > +		}
> >  	} else {
> >  		memset(pdata->eld.eld_data, 0,
> >  			HDMI_MAX_ELD_BYTES);
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx