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

[Jani Nikula]

On Fri, 27 Jan 2017, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> On Fri, Jan 27, 2017 at 12:08:58PM +0200, Jani Nikula wrote:
>> On Thu, 26 Jan 2017, Pierre-Louis Bossart <pierre-louis.bossart at 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 at 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().

Works for ports A, B, C, but here we have ports B, C, D.

BR,
Jani.


>
>> 
>> 
>> >  #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

-- 
Jani Nikula, Intel Open Source Technology Center