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

Submitted by Takashi Iwai on Jan. 31, 2017, 9:36 p.m.

Details

Message ID 20170131213649.13689-3-tiwai@suse.de
State New
Headers show
Series "DisplayPort audio support on Cherrytrail" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Takashi Iwai Jan. 31, 2017, 9:36 p.m.
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

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@linux.intel.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/i915/i915_reg.h        | 10 ++++++++++
 drivers/gpu/drm/i915/intel_lpe_audio.c | 17 +++++++++++++++++
 2 files changed, 27 insertions(+)

Patch hide | download patch | download mbox

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;
+	}
+
 	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);
+	audio_enable = I915_READ(mmio);
+
 	if (eld != NULL) {
 		memcpy(pdata->eld.eld_data, eld,
 			HDMI_MAX_ELD_BYTES);
@@ -357,11 +367,18 @@  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(mmio, 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(mmio, audio_enable | VLV_AMP_MUTE);
 	}
 
 	if (pdata->notify_audio_lpe)

Comments

On Tue, Jan 31, 2017 at 10:36:44PM +0100, Takashi Iwai wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> 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@linux.intel.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  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.

> +	audio_enable = I915_READ(mmio);
> +
>  	if (eld != NULL) {
>  		memcpy(pdata->eld.eld_data, eld,
>  			HDMI_MAX_ELD_BYTES);
> @@ -357,11 +367,18 @@ 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(mmio, 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(mmio, audio_enable | VLV_AMP_MUTE);
>  	}
>  
>  	if (pdata->notify_audio_lpe)
> -- 
> 2.11.0
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@linux.intel.com>
> > 
> > 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@linux.intel.com>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  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@linux.intel.com>
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@linux.intel.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/i915/i915_reg.h        | 10 ++++++++++
 drivers/gpu/drm/i915/intel_lpe_audio.c | 12 ++++++++++++
 2 files changed, 22 insertions(+)
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@linux.intel.com>
> > > 
> > > 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@linux.intel.com>
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > >  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@linux.intel.com>
> 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@linux.intel.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  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@linux.intel.com>

> +#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
On Wed, 01 Feb 2017 16:11:37 +0100,
Ville Syrjälä wrote:
> 
> > +/* 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@linux.intel.com>

Alright, below is the respinned one.


Thanks!

Takashi

-- 8< --
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Subject: [PATCH v3] 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.
v2->v3: Add "_" prefix to VLV_AUD_PORT_EN_X_DBG as they are internal.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/i915/i915_reg.h        | 10 ++++++++++
 drivers/gpu/drm/i915/intel_lpe_audio.c | 12 ++++++++++++
 2 files changed, 22 insertions(+)
On Tue, 31 Jan 2017 22:36:44 +0100,
Takashi Iwai wrote:
> 
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> 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.

Sorry, this seems like a wrong result.  It worked yesterday, but when
I retested the latest code today, it didn't work any longer.
Then, after setting the chicken bit, it starts working again.

But now I can't make it broken again.  Turn off the monitor, turn off
the machine, DPMS off, suspend/resume...  All still works without
chicken bit set.

Also, the chicken bit of the register 0x62f38 couldn't be read back.
The register always returns zero when read.

So the chicken bit helps definitely something on a device here, but
it's still mystery how it works.

I'll send an additional patch to re-add the audio chicken bit stuff.


thanks,

Takashi

> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  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;
> +	}
> +
>  	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);
> +	audio_enable = I915_READ(mmio);
> +
>  	if (eld != NULL) {
>  		memcpy(pdata->eld.eld_data, eld,
>  			HDMI_MAX_ELD_BYTES);
> @@ -357,11 +367,18 @@ 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(mmio, 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(mmio, audio_enable | VLV_AMP_MUTE);
>  	}
>  
>  	if (pdata->notify_audio_lpe)
> -- 
> 2.11.0
>
On Thu, Feb 02, 2017 at 10:57:30AM +0100, Takashi Iwai wrote:
> On Tue, 31 Jan 2017 22:36:44 +0100,
> Takashi Iwai wrote:
> > 
> > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > 
> > 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.
> 
> Sorry, this seems like a wrong result.  It worked yesterday, but when
> I retested the latest code today, it didn't work any longer.
> Then, after setting the chicken bit, it starts working again.
> 
> But now I can't make it broken again.  Turn off the monitor, turn off
> the machine, DPMS off, suspend/resume...  All still works without
> chicken bit set.
> 
> Also, the chicken bit of the register 0x62f38 couldn't be read back.
> The register always returns zero when read.

The docs do say it's write-only actually.
On Thu, 02 Feb 2017 11:06:05 +0100,
Ville Syrjälä wrote:
> 
> On Thu, Feb 02, 2017 at 10:57:30AM +0100, Takashi Iwai wrote:
> > On Tue, 31 Jan 2017 22:36:44 +0100,
> > Takashi Iwai wrote:
> > > 
> > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > 
> > > 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.
> > 
> > Sorry, this seems like a wrong result.  It worked yesterday, but when
> > I retested the latest code today, it didn't work any longer.
> > Then, after setting the chicken bit, it starts working again.
> > 
> > But now I can't make it broken again.  Turn off the monitor, turn off
> > the machine, DPMS off, suspend/resume...  All still works without
> > chicken bit set.
> > 
> > Also, the chicken bit of the register 0x62f38 couldn't be read back.
> > The register always returns zero when read.
> 
> The docs do say it's write-only actually.

OK, thanks for confirmation!


Takashi