Re: [PATCH 2/7] drm/i915: add DisplayPort amp unmute for LPE audio mode

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Feb 01, 2017 at 03:53:49PM +0100, Takashi Iwai wrote:
> On Wed, 01 Feb 2017 15:45:24 +0100,
> Ville Syrjälä wrote:
> > 
> > On Tue, Jan 31, 2017 at 10:36:44PM +0100, Takashi Iwai wrote:
> > > From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> > > 
> > > Enable unmute/mute amp notification. This doesn't seem to affect
> > > HDMI support so this is done unconditionally.
> > > 
> > > An earlier version of this patch set a chicken bit at address 0x62F38
> > > prior to the mute/unmute but this register doesn't seem to do anything
> > > so this phase was removed.
> > > 
> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h        | 10 ++++++++++
> > >  drivers/gpu/drm/i915/intel_lpe_audio.c | 17 +++++++++++++++++
> > >  2 files changed, 27 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index a9ffc8df241b..8fcc80cb864b 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -2061,6 +2061,16 @@ 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 VLV_AUD_PORT_EN_B_DBG		(VLV_DISPLAY_BASE + 0x62F20)
> > > +#define VLV_AUD_PORT_EN_C_DBG		(VLV_DISPLAY_BASE + 0x62F30)
> > > +#define VLV_AUD_PORT_EN_D_DBG		(VLV_DISPLAY_BASE + 0x62F34)
> > > +#define VLV_AUD_PORT_EN_DBG(port)	_MMIO_PORT3((port) - PORT_B,	   \
> > > +						    VLV_AUD_PORT_EN_B_DBG, \
> > > +						    VLV_AUD_PORT_EN_C_DBG, \
> > > +						    VLV_AUD_PORT_EN_D_DBG)
> > > +#define VLV_AMP_MUTE		        (1 << 1)
> > > +
> > >  #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 245523e14418..f95624a46f27 100644
> > > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > @@ -337,15 +337,25 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> > >  {
> > >  	unsigned long irq_flags;
> > >  	struct intel_hdmi_lpe_audio_pdata *pdata = NULL;
> > > +	u32 audio_enable;
> > > +	i915_reg_t mmio;
> > >  
> > >  	if (!HAS_LPE_AUDIO(dev_priv))
> > >  		return;
> > >  
> > > +	if (port == PORT_A) {
> > > +		DRM_ERROR("PORT_A is not valid for HDMI/DP usages\n");
> > > +		return;
> > > +	}
> > 
> > Port A doesn't exists on these platforms at all. So this is
> > just dead code.
> > 
> > > +
> > >  	pdata = dev_get_platdata(
> > >  		&(dev_priv->lpe_audio.platdev->dev));
> > >  
> > >  	spin_lock_irqsave(&pdata->lpe_audio_slock, irq_flags);
> > >  
> > > +	mmio = VLV_AUD_PORT_EN_DBG(port);
> > 
> > I'm not a fan of these temporary reg offset variables. Makes it hard to
> > figure out later which register is being frobbed, especially when the
> > variable name doesn't say what it actually is (like in this case). So
> > I'd like to see this killed off.
> 
> How about something like below?
> 
> 
> thanks,
> 
> Takashi
> 
> -- 8< --
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Subject: [PATCH v2] drm/i915: add DisplayPort amp unmute for LPE audio mode
> 
> Enable unmute/mute amp notification. This doesn't seem to affect
> HDMI support so this is done unconditionally.
> 
> An earlier version of this patch set a chicken bit at address 0x62F38
> prior to the mute/unmute but this register doesn't seem to do anything
> so this phase was removed.
> 
> v1->v2: Drop needless pipe A check, avoid temporary reg offset variable.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_reg.h        | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_lpe_audio.c | 12 ++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index a9ffc8df241b..8fcc80cb864b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2061,6 +2061,16 @@ 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 VLV_AUD_PORT_EN_B_DBG		(VLV_DISPLAY_BASE + 0x62F20)
> +#define VLV_AUD_PORT_EN_C_DBG		(VLV_DISPLAY_BASE + 0x62F30)
> +#define VLV_AUD_PORT_EN_D_DBG		(VLV_DISPLAY_BASE + 0x62F34)

We generally like to prefix the raw register offsets with '_' to make
it clear they're not to be used directly.

With that fixed the i915 patches are
Acked-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>

> +#define VLV_AUD_PORT_EN_DBG(port)	_MMIO_PORT3((port) - PORT_B,	   \
> +						    VLV_AUD_PORT_EN_B_DBG, \
> +						    VLV_AUD_PORT_EN_C_DBG, \
> +						    VLV_AUD_PORT_EN_D_DBG)
> +#define VLV_AMP_MUTE		        (1 << 1)
> +
>  #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 245523e14418..5da14f40f94a 100644
> --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> @@ -337,6 +337,7 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
>  {
>  	unsigned long irq_flags;
>  	struct intel_hdmi_lpe_audio_pdata *pdata = NULL;
> +	u32 audio_enable;
>  
>  	if (!HAS_LPE_AUDIO(dev_priv))
>  		return;
> @@ -346,6 +347,8 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
>  
>  	spin_lock_irqsave(&pdata->lpe_audio_slock, irq_flags);
>  
> +	audio_enable = I915_READ(VLV_AUD_PORT_EN_DBG(port));
> +
>  	if (eld != NULL) {
>  		memcpy(pdata->eld.eld_data, eld,
>  			HDMI_MAX_ELD_BYTES);
> @@ -357,11 +360,20 @@ 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;
> +
> +		/* Unmute the amp for both DP and HDMI */
> +		I915_WRITE(VLV_AUD_PORT_EN_DBG(port),
> +			   audio_enable & ~VLV_AMP_MUTE);
> +
>  	} else {
>  		memset(pdata->eld.eld_data, 0,
>  			HDMI_MAX_ELD_BYTES);
>  		pdata->hdmi_connected = false;
>  		pdata->dp_output = false;
> +
> +		/* Mute the amp for both DP and HDMI */
> +		I915_WRITE(VLV_AUD_PORT_EN_DBG(port),
> +			   audio_enable | VLV_AMP_MUTE);
>  	}
>  
>  	if (pdata->notify_audio_lpe)
> -- 
> 2.11.0

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux