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

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

2017-01-27 Thread Ville Syrjälä

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.

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

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

2017-01-27 Thread Ville Syrjälä

On Fri, Jan 27, 2017 at 08:47:50AM -0600, Pierre-Louis Bossart wrote:
> Thanks Jani and Ville for the comments. Couple of precisions needed below:
> >>
> >>>   #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).
> VLV has DisplayPort?

Yes. And as I said earlier the docs don't make any distinciton between
HDMI and DP in the audio programming sequence. So based on the docs we
should always unmute the amp manually.

>I thought this was an addition on CHT, and I really 
> don't know of any devices with this combination of HDaudio disabled+DP. 
> I'd rather keep this CHT-only until we find a device where this can be 
> tested.
> 
> On the powerwell, I could use more guidance. i tried this first solution 
> to see if streaming worked (and it did :-)). I don't mind moving the 
> code somewhere else but I have no idea where.

1. boot with drm.debug=0xe
1. 'intel_reg read 0x62f38' just after boot, to see that the
   chicken bit is as it should
2. disable all displays (eg. xset dpms force off)
3. grep 'power_well.*display' (make sure the last thing it says is
   "disabling"
4. turn on the displays (eg. xset dpms force on)
5. 'intel_reg read 0x62f38' to check the chicken bit again

If it didn't survive then we need to set it in either in
vlv_display_power_well_init(), or we could just set it whenever
we set the AMP_MUTE bit for any of the ports.

>
> >>> + 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.
> 
> I'll try to see if HDMI still works with this. We could tentatively add 
> unmute in all cases but I'll need to add a test for Baytrail (no PORT_D) 
> so in the end it's the same number of tests.
> 
> >
> > And I think we might want to mute things again when disabling audio.
> I was wondering if there would be side effects of writing to a register 
> controlling a port if that port is not connected any longer. I'll give 
> it a try.

Nothing apart from HPD circuitry really cares if there's anything
connectect or not.

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

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

2017-01-27 Thread Pierre-Louis Bossart





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


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2017-01-27 Thread Pierre-Louis Bossart


Thanks Jani and Ville for the comments. Couple of precisions needed below:



  #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).
VLV has DisplayPort? I thought this was an addition on CHT, and I really 
don't know of any devices with this combination of HDaudio disabled+DP. 
I'd rather keep this CHT-only until we find a device where this can be 
tested.


On the powerwell, I could use more guidance. i tried this first solution 
to see if streaming worked (and it did :-)). I don't mind moving the 
code somewhere else but I have no idea where.



+   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.


I'll try to see if HDMI still works with this. We could tentatively add 
unmute in all cases but I'll need to add a test for Baytrail (no PORT_D) 
so in the end it's the same number of tests.




And I think we might want to mute things again when disabling audio.
I was wondering if there would be side effects of writing to a register 
controlling a port if that port is not connected any longer. I'll give 
it a try.

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2017-01-27 Thread Takashi Iwai

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

---
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_REG0x62F38
 #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)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2017-01-27 Thread Ville Syrjälä

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 
> >  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 
> > > ---
> > >  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_DBG0x62F20
> > > +#define AUD_PORT_EN_C_DBG0x62F28
> > > +#define AUD_PORT_EN_D_DBG0x62F2C
> 
> 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

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

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

2017-01-27 Thread Ville Syrjälä

On Fri, Jan 27, 2017 at 03:51:34PM +0200, Jani Nikula wrote:
> On Fri, 27 Jan 2017, 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 
> >>  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 
> >> > 
> >> > ---
> >> >  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_SIZE0x1000
> >> >  
> >> > +/* 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.

(port)-PORT_B is easy enough to stick in there ;)

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

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

2017-01-27 Thread Jani Nikula

On Fri, 27 Jan 2017, 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 
>>  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 
>> > ---
>> >  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) {
>> > +

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

2017-01-27 Thread Ville Syrjälä

On Fri, Jan 27, 2017 at 12:08:58PM +0200, Jani Nikula wrote:
> On Thu, 26 Jan 2017, Pierre-Louis Bossart 
>  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 
> > ---
> >  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_REG0x62F38

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) {
> > +

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

2017-01-27 Thread Jani Nikula

On Thu, 26 Jan 2017, Pierre-Louis Bossart 
 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 
> ---
>  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)

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
> +#define AUD_PORT_EN_B_DBG0x62F20
> +#define AUD_PORT_EN_C_DBG0x62F28
> +#define AUD_PORT_EN_D_DBG0x62F2C

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


>  #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)) {
> + 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 */
> + 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2017-01-26 Thread Pierre-Louis Bossart

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 
---
 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)
+#define AUD_CHICKEN_BIT_REG0x62F38
+#define AUD_PORT_EN_B_DBG  0x62F20
+#define AUD_PORT_EN_C_DBG  0x62F28
+#define AUD_PORT_EN_D_DBG  0x62F2C
+#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)
+
 #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)) {
+   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 */
+   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);
-- 
2.7.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

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

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

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

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

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

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

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

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

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

11 matches

Site Navigation

Mail list logo

Footer information