diff mbox

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

Message ID s5hvat0ftsf.wl-tiwai@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Takashi Iwai Jan. 27, 2017, 2:42 p.m. UTC 
On Fri, 27 Jan 2017 15:35:47 +0100,
Ville Syrjälä wrote:
> 
> On Fri, Jan 27, 2017 at 03:17:34PM +0200, Ville Syrjälä 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@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.
> 
> Actually they just match one version of the spec I had lying around.
> Another versions says:
> 
> AUD_PORT_EN_B_DBG 0x62F20
> AUD_PORT_EN_C_DBG 0x62F30
> AUD_PORT_EN_D_DBG 0x62F34

That's it!  Now finally I can hear the audio from DP3 with the
additional patch below.


thanks,

Takashi

---

Comments

Pierre-Louis Bossart Jan. 27, 2017, 2:55 p.m. UTC | #1 
>>>>> +#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.
>> Actually they just match one version of the spec I had lying around.
>> Another versions says:
>>
>> AUD_PORT_EN_B_DBG 0x62F20
>> AUD_PORT_EN_C_DBG 0x62F30
>> AUD_PORT_EN_D_DBG 0x62F34
> That's it!  Now finally I can hear the audio from DP3 with the
> additional patch below.
Wow. Thanks Ville for looking into this, we could have lost a lot of 
time. Do you happen to know if those previous values are due to poor 
documentation or a different skew we'd need to support, e.g. with a 
PCI-Id quirk?
At any rate, 2 days to get DP audio working is pretty nice, this was a 
good week.
Ville Syrjälä Jan. 27, 2017, 3:46 p.m. UTC | #2 
On Fri, Jan 27, 2017 at 08:55:19AM -0600, Pierre-Louis Bossart wrote:
> 
> 
> >>>>> +#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.
> >> Actually they just match one version of the spec I had lying around.
> >> Another versions says:
> >>
> >> AUD_PORT_EN_B_DBG 0x62F20
> >> AUD_PORT_EN_C_DBG 0x62F30
> >> AUD_PORT_EN_D_DBG 0x62F34
> > That's it!  Now finally I can hear the audio from DP3 with the
> > additional patch below.
> Wow. Thanks Ville for looking into this, we could have lost a lot of 
> time. Do you happen to know if those previous values are due to poor 
> documentation or a different skew we'd need to support, e.g. with a 
> PCI-Id quirk?

No idea really. You should really test this on both CHV and VLV with all
possible port/pipe combinations to make sure we got it right. I trust
these VLV/CHV docs about as much as I trust most politicians.

Alternatively you could just read all those regs on both platforms and
see if the values you get from them conform to any visible pattern that
could tell us which offsets are the correct ones. They might not, in
which case actual testing is the best bet.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ee90f64b89e8..5c577d242078 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2066,8 +2066,8 @@  enum skl_disp_power_wells {
 #define AMP_UNMUTE		        (1 << 1)
 #define AUD_CHICKEN_BIT_REG		0x62F38
 #define AUD_PORT_EN_B_DBG		0x62F20
-#define AUD_PORT_EN_C_DBG		0x62F28
-#define AUD_PORT_EN_D_DBG		0x62F2C
+#define AUD_PORT_EN_C_DBG		0x62F30
+#define AUD_PORT_EN_D_DBG		0x62F34
 #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)