Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2017-01-05 Thread Pandiyan, Dhinakaran
On Wed, 2017-01-04 at 11:42 +0100, Peter Frühberger wrote:
> Forgot to CC the list, sorry.
> 
> On Wed, Jan 4, 2017 at 11:42 AM, Peter Frühberger 
> wrote:
> Hi Jani,
> thanks for your reply
> 
> On Wed, Jan 4, 2017 at 10:34 AM, Jani Nikula
>  wrote:
> On Wed, 04 Jan 2017, Peter Frühberger
>  wrote:
> > Hi
> >
> > On Sun, Nov 6, 2016 at 1:23 AM, Pandiyan, Dhinakaran
> <
> > dhinakaran.pandi...@intel.com> wrote:
> >
> >> On Sat, 2016-11-05 at 21:40 +0200, Jani Nikula
> wrote:
> >> > On Fri, 04 Nov 2016, "Pandiyan, Dhinakaran" <
> >> dhinakaran.pandi...@intel.com> wrote:
> >> > > On Fri, 2016-11-04 at 17:48 +0200, Jani Nikula
> wrote:
> >> > >> On Wed, 26 Oct 2016, Dhinakaran Pandiyan <
> >> dhinakaran.pandi...@intel.com> wrote:
> >> > >> > Enabling DP audio stall fix is necessary to
> play audio over DP
> >> HBR2. So,
> >> > >> > let's set this bit right before enabling the
> audio codec. Playing
> >> audio
> >> > >> > without setting this bit results in pipe
> FIFO underruns.
> >> > >> >
> >> > >> > This workaround is applicable only for audio
> sample rates up to
> >> 96kHz. For
> >> > >> > frequencies above 96kHz, this is
> insufficient and cdclk should be
> >> increased
> >> > >> > to at least 432 MHz, just like BDW. Since,
> the audio driver does not
> >> > >> > support sample rates > 48 kHz, we are safe
> with this fix for now.
> >> > >>
> >> > >> Do we still need this patch now that these two
> have been pushed?
> >> > >>
> >> > >> b30ce9e0552a drm/i915/dp: BDW cdclk fix for DP
> audio
> >> > >> 9c7540241885 drm/i915/dp: Extend BDW DP audio
> workaround to GEN9
> >> platforms
> >> > >>
> >> > >> BR,
> >> > >> Jani.
> >> > >>
> >> > >>
> >> > >>
> >> > >
> >> > > No, we are good afaik. This patch would have
> helped us to make use of a
> >> > > lower cdclk (337.5 MHz), with constraints on
> audio bit rate. Operating
> >> > > at 432 MHz, like we do now, rules out the need
> for this patch.
> >> >
> >> > Hmm, what about 5.4 Gbps link with 1 or 2 lanes?
> >> >
> >> > BR,
> >> > Jani.
> >> >
> >>
> >> Good point, I think it will depend on the audio
> sampling rate. But, I
> >> have to figure out a way to play high sampling rate
> audio (> 96 KHz) and
> >> test 5.4 Gbps with 1 or 2 lanes.
> >>
> >> The other option is to play safe and apply this
> patch with even lesser
> >> restrictions, say link rate >= 2.7 Gbps.
> >>
> >>
> >> -DK
> >>
> >
> > as we are currently talking about high samplerates
> in this context. I
> > wanted to post a perhaps related issue. On my Apollo
> Lake (J4205) I have
> > two outputs. One DVI and one HDMI 2.0 via internal
> DP. Via DVI the
> > following works without issues, via DP it fails. As
> the original commit
> > mentions HBR, I think there is still something
> missing. We submit TrueHD,
> > DTS-HD via 192 khz and 16 bit format while setting
> AES0=2
> >
> > You can easily reproduce with (you obviously need a
> DTS-HD, TrueHD capable
> > AVR attached to your HDMI 2.0 (DP) out):
> 
+Libin


> Just to clarify, is the DP -> HDMI2.0 converter
> internal to the machine?
> LSPCON related messages in the dmesg with
> drm.debug=14? Do you have a DP
> or an HDMI physical connector in the chassis?
> 
> 
> The chip used is: https://media.digikey.com/pdf/Data%
> 20Sheets/MegaChips%20PDFs/MCDP28x0_Datasheet.pdf which is the
> same on all 

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2017-01-04 Thread Peter Frühberger
Forgot to CC the list, sorry.

On Wed, Jan 4, 2017 at 11:42 AM, Peter Frühberger  wrote:

> Hi Jani,
> thanks for your reply
>
> On Wed, Jan 4, 2017 at 10:34 AM, Jani Nikula 
> wrote:
>
>> On Wed, 04 Jan 2017, Peter Frühberger  wrote:
>> > Hi
>> >
>> > On Sun, Nov 6, 2016 at 1:23 AM, Pandiyan, Dhinakaran <
>> > dhinakaran.pandi...@intel.com> wrote:
>> >
>> >> On Sat, 2016-11-05 at 21:40 +0200, Jani Nikula wrote:
>> >> > On Fri, 04 Nov 2016, "Pandiyan, Dhinakaran" <
>> >> dhinakaran.pandi...@intel.com> wrote:
>> >> > > On Fri, 2016-11-04 at 17:48 +0200, Jani Nikula wrote:
>> >> > >> On Wed, 26 Oct 2016, Dhinakaran Pandiyan <
>> >> dhinakaran.pandi...@intel.com> wrote:
>> >> > >> > Enabling DP audio stall fix is necessary to play audio over DP
>> >> HBR2. So,
>> >> > >> > let's set this bit right before enabling the audio codec.
>> Playing
>> >> audio
>> >> > >> > without setting this bit results in pipe FIFO underruns.
>> >> > >> >
>> >> > >> > This workaround is applicable only for audio sample rates up to
>> >> 96kHz. For
>> >> > >> > frequencies above 96kHz, this is insufficient and cdclk should
>> be
>> >> increased
>> >> > >> > to at least 432 MHz, just like BDW. Since, the audio driver
>> does not
>> >> > >> > support sample rates > 48 kHz, we are safe with this fix for
>> now.
>> >> > >>
>> >> > >> Do we still need this patch now that these two have been pushed?
>> >> > >>
>> >> > >> b30ce9e0552a drm/i915/dp: BDW cdclk fix for DP audio
>> >> > >> 9c7540241885 drm/i915/dp: Extend BDW DP audio workaround to GEN9
>> >> platforms
>> >> > >>
>> >> > >> BR,
>> >> > >> Jani.
>> >> > >>
>> >> > >>
>> >> > >>
>> >> > >
>> >> > > No, we are good afaik. This patch would have helped us to make use
>> of a
>> >> > > lower cdclk (337.5 MHz), with constraints on audio bit rate.
>> Operating
>> >> > > at 432 MHz, like we do now, rules out the need for this patch.
>> >> >
>> >> > Hmm, what about 5.4 Gbps link with 1 or 2 lanes?
>> >> >
>> >> > BR,
>> >> > Jani.
>> >> >
>> >>
>> >> Good point, I think it will depend on the audio sampling rate. But, I
>> >> have to figure out a way to play high sampling rate audio (> 96 KHz)
>> and
>> >> test 5.4 Gbps with 1 or 2 lanes.
>> >>
>> >> The other option is to play safe and apply this patch with even lesser
>> >> restrictions, say link rate >= 2.7 Gbps.
>> >>
>> >>
>> >> -DK
>> >>
>> >
>> > as we are currently talking about high samplerates in this context. I
>> > wanted to post a perhaps related issue. On my Apollo Lake (J4205) I have
>> > two outputs. One DVI and one HDMI 2.0 via internal DP. Via DVI the
>> > following works without issues, via DP it fails. As the original commit
>> > mentions HBR, I think there is still something missing. We submit
>> TrueHD,
>> > DTS-HD via 192 khz and 16 bit format while setting AES0=2
>> >
>> > You can easily reproduce with (you obviously need a DTS-HD, TrueHD
>> capable
>> > AVR attached to your HDMI 2.0 (DP) out):
>>
>> Just to clarify, is the DP -> HDMI2.0 converter internal to the machine?
>> LSPCON related messages in the dmesg with drm.debug=14? Do you have a DP
>> or an HDMI physical connector in the chassis?
>>
>
> The chip used is: https://media.digikey.com/pdf/Data%20Sheets/MegaChips%
> 20PDFs/MCDP28x0_Datasheet.pdf which is the same on all intel nucs,
> including the new Kabilake ones. So it's internal.
> Mainboard: http://www.asrock.com/MB/Intel/J4205-ITX/index.us.asp (DVI-D,
> HDMI (2.0 via this above chip), VGA)
> Here is the output of the a boot up with drm.debug=14:
> http://paste.ubuntu.com/23738282/
> (I used a pastebin site to not spam the ML, is that okay for the future?)
>
> Best regards
> Peter
>
>
>>
>>
>> BR,
>> Jani.
>>
>> >
>> > #TrueHD
>> > aplay -D 'hdmi:CARD=PCH,DEV=0,AES0=2' -c8 -fs16_le -r192000
>> > testi.truehd.anssi1.ff.60s.spdif
>> > #DTS-HD
>> > aplay -D 'hdmi:CARD=PCH,DEV=0,AES0=2' -c8 -fs16_le -r192000
>> > testi.dtshd.anssi1.ma-71-24.spdif
>> > Samples:
>> > http://www.avenard.org/files/media/mediatest/audiotest/HDAUD
>> IO/Passthrough/
>> >
>> > For the old HDMI 1.x chips it was fixed via:
>> > https://bugs.freedesktop.org/show_bug.cgi?id=49055
>> >
>> > Is this also planned for DP within that patch series?
>> >
>> > Best regards
>> > Peter
>> >
>> >>
>> >> > >
>> >> > > -DK
>> >> > >
>> >> > >> >
>> >> > >> > v2: Inlined the code change within hsw_audio_codec_enable()
>> (Jani)
>> >> > >> > Fixed the port clock typo
>> >> > >> > Added TODO comment
>> >> > >> > Signed-off-by: Dhinakaran Pandiyan <
>> dhinakaran.pandi...@intel.com>
>> >> > >> > ---
>> >> > >> >  drivers/gpu/drm/i915/i915_reg.h|  5 +
>> >> > >> >  drivers/gpu/drm/i915/intel_audio.c | 30
>> >> +-
>> >> > >> >  2 files changed, 34 insertions(+), 1 deletion(-)
>> >> > >> >
>> >> > >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> >> b/drivers/gpu/drm/i915/i915_reg.h
>> >> > >> > index 00efaa1..76dac48 100644
>> >> > >> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> >> > >> > +++ b/drivers/gpu

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2017-01-04 Thread Jani Nikula
On Wed, 04 Jan 2017, Peter Frühberger  wrote:
> Hi
>
> On Sun, Nov 6, 2016 at 1:23 AM, Pandiyan, Dhinakaran <
> dhinakaran.pandi...@intel.com> wrote:
>
>> On Sat, 2016-11-05 at 21:40 +0200, Jani Nikula wrote:
>> > On Fri, 04 Nov 2016, "Pandiyan, Dhinakaran" <
>> dhinakaran.pandi...@intel.com> wrote:
>> > > On Fri, 2016-11-04 at 17:48 +0200, Jani Nikula wrote:
>> > >> On Wed, 26 Oct 2016, Dhinakaran Pandiyan <
>> dhinakaran.pandi...@intel.com> wrote:
>> > >> > Enabling DP audio stall fix is necessary to play audio over DP
>> HBR2. So,
>> > >> > let's set this bit right before enabling the audio codec. Playing
>> audio
>> > >> > without setting this bit results in pipe FIFO underruns.
>> > >> >
>> > >> > This workaround is applicable only for audio sample rates up to
>> 96kHz. For
>> > >> > frequencies above 96kHz, this is insufficient and cdclk should be
>> increased
>> > >> > to at least 432 MHz, just like BDW. Since, the audio driver does not
>> > >> > support sample rates > 48 kHz, we are safe with this fix for now.
>> > >>
>> > >> Do we still need this patch now that these two have been pushed?
>> > >>
>> > >> b30ce9e0552a drm/i915/dp: BDW cdclk fix for DP audio
>> > >> 9c7540241885 drm/i915/dp: Extend BDW DP audio workaround to GEN9
>> platforms
>> > >>
>> > >> BR,
>> > >> Jani.
>> > >>
>> > >>
>> > >>
>> > >
>> > > No, we are good afaik. This patch would have helped us to make use of a
>> > > lower cdclk (337.5 MHz), with constraints on audio bit rate. Operating
>> > > at 432 MHz, like we do now, rules out the need for this patch.
>> >
>> > Hmm, what about 5.4 Gbps link with 1 or 2 lanes?
>> >
>> > BR,
>> > Jani.
>> >
>>
>> Good point, I think it will depend on the audio sampling rate. But, I
>> have to figure out a way to play high sampling rate audio (> 96 KHz) and
>> test 5.4 Gbps with 1 or 2 lanes.
>>
>> The other option is to play safe and apply this patch with even lesser
>> restrictions, say link rate >= 2.7 Gbps.
>>
>>
>> -DK
>>
>
> as we are currently talking about high samplerates in this context. I
> wanted to post a perhaps related issue. On my Apollo Lake (J4205) I have
> two outputs. One DVI and one HDMI 2.0 via internal DP. Via DVI the
> following works without issues, via DP it fails. As the original commit
> mentions HBR, I think there is still something missing. We submit TrueHD,
> DTS-HD via 192 khz and 16 bit format while setting AES0=2
>
> You can easily reproduce with (you obviously need a DTS-HD, TrueHD capable
> AVR attached to your HDMI 2.0 (DP) out):

Just to clarify, is the DP -> HDMI2.0 converter internal to the machine?
LSPCON related messages in the dmesg with drm.debug=14? Do you have a DP
or an HDMI physical connector in the chassis?


BR,
Jani.

>
> #TrueHD
> aplay -D 'hdmi:CARD=PCH,DEV=0,AES0=2' -c8 -fs16_le -r192000
> testi.truehd.anssi1.ff.60s.spdif
> #DTS-HD
> aplay -D 'hdmi:CARD=PCH,DEV=0,AES0=2' -c8 -fs16_le -r192000
> testi.dtshd.anssi1.ma-71-24.spdif
> Samples:
> http://www.avenard.org/files/media/mediatest/audiotest/HDAUDIO/Passthrough/
>
> For the old HDMI 1.x chips it was fixed via:
> https://bugs.freedesktop.org/show_bug.cgi?id=49055
>
> Is this also planned for DP within that patch series?
>
> Best regards
> Peter
>
>>
>> > >
>> > > -DK
>> > >
>> > >> >
>> > >> > v2: Inlined the code change within hsw_audio_codec_enable() (Jani)
>> > >> > Fixed the port clock typo
>> > >> > Added TODO comment
>> > >> > Signed-off-by: Dhinakaran Pandiyan 
>> > >> > ---
>> > >> >  drivers/gpu/drm/i915/i915_reg.h|  5 +
>> > >> >  drivers/gpu/drm/i915/intel_audio.c | 30
>> +-
>> > >> >  2 files changed, 34 insertions(+), 1 deletion(-)
>> > >> >
>> > >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> > >> > index 00efaa1..76dac48 100644
>> > >> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > >> > @@ -6236,6 +6236,11 @@ enum {
>> > >> >  #define SLICE_ECO_CHICKEN0  _MMIO(0x7308)
>> > >> >  #define   PIXEL_MASK_CAMMING_DISABLE(1 << 14)
>> > >> >
>> > >> > +#define _CHICKEN_TRANS_A0x420C0
>> > >> > +#define _CHICKEN_TRANS_B0x420C4
>> > >> > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A,
>> _CHICKEN_TRANS_B)
>> > >> > +#define SPARE_13(1<<13)
>> > >> > +
>> > >> >  /* WaCatErrorRejectionIssue */
>> > >> >  #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG  _MMIO(0x9030)
>> > >> >  #define  GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB   (1<<11)
>> > >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
>> b/drivers/gpu/drm/i915/intel_audio.c
>> > >> > index 7093cfb..894f11e 100644
>> > >> > --- a/drivers/gpu/drm/i915/intel_audio.c
>> > >> > +++ b/drivers/gpu/drm/i915/intel_audio.c
>> > >> > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct
>> intel_encoder *encoder)
>> > >> >  {
>> > >> >  struct drm_i915_private *dev_priv =
>> to_i915(encoder->base.dev);
>> > >> >  s

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2017-01-04 Thread Peter Frühberger
Hi

On Sun, Nov 6, 2016 at 1:23 AM, Pandiyan, Dhinakaran <
dhinakaran.pandi...@intel.com> wrote:

> On Sat, 2016-11-05 at 21:40 +0200, Jani Nikula wrote:
> > On Fri, 04 Nov 2016, "Pandiyan, Dhinakaran" <
> dhinakaran.pandi...@intel.com> wrote:
> > > On Fri, 2016-11-04 at 17:48 +0200, Jani Nikula wrote:
> > >> On Wed, 26 Oct 2016, Dhinakaran Pandiyan <
> dhinakaran.pandi...@intel.com> wrote:
> > >> > Enabling DP audio stall fix is necessary to play audio over DP
> HBR2. So,
> > >> > let's set this bit right before enabling the audio codec. Playing
> audio
> > >> > without setting this bit results in pipe FIFO underruns.
> > >> >
> > >> > This workaround is applicable only for audio sample rates up to
> 96kHz. For
> > >> > frequencies above 96kHz, this is insufficient and cdclk should be
> increased
> > >> > to at least 432 MHz, just like BDW. Since, the audio driver does not
> > >> > support sample rates > 48 kHz, we are safe with this fix for now.
> > >>
> > >> Do we still need this patch now that these two have been pushed?
> > >>
> > >> b30ce9e0552a drm/i915/dp: BDW cdclk fix for DP audio
> > >> 9c7540241885 drm/i915/dp: Extend BDW DP audio workaround to GEN9
> platforms
> > >>
> > >> BR,
> > >> Jani.
> > >>
> > >>
> > >>
> > >
> > > No, we are good afaik. This patch would have helped us to make use of a
> > > lower cdclk (337.5 MHz), with constraints on audio bit rate. Operating
> > > at 432 MHz, like we do now, rules out the need for this patch.
> >
> > Hmm, what about 5.4 Gbps link with 1 or 2 lanes?
> >
> > BR,
> > Jani.
> >
>
> Good point, I think it will depend on the audio sampling rate. But, I
> have to figure out a way to play high sampling rate audio (> 96 KHz) and
> test 5.4 Gbps with 1 or 2 lanes.
>
> The other option is to play safe and apply this patch with even lesser
> restrictions, say link rate >= 2.7 Gbps.
>
>
> -DK
>

as we are currently talking about high samplerates in this context. I
wanted to post a perhaps related issue. On my Apollo Lake (J4205) I have
two outputs. One DVI and one HDMI 2.0 via internal DP. Via DVI the
following works without issues, via DP it fails. As the original commit
mentions HBR, I think there is still something missing. We submit TrueHD,
DTS-HD via 192 khz and 16 bit format while setting AES0=2

You can easily reproduce with (you obviously need a DTS-HD, TrueHD capable
AVR attached to your HDMI 2.0 (DP) out):

#TrueHD
aplay -D 'hdmi:CARD=PCH,DEV=0,AES0=2' -c8 -fs16_le -r192000
testi.truehd.anssi1.ff.60s.spdif
#DTS-HD
aplay -D 'hdmi:CARD=PCH,DEV=0,AES0=2' -c8 -fs16_le -r192000
testi.dtshd.anssi1.ma-71-24.spdif
Samples:
http://www.avenard.org/files/media/mediatest/audiotest/HDAUDIO/Passthrough/

For the old HDMI 1.x chips it was fixed via:
https://bugs.freedesktop.org/show_bug.cgi?id=49055

Is this also planned for DP within that patch series?

Best regards
Peter

>
> > >
> > > -DK
> > >
> > >> >
> > >> > v2: Inlined the code change within hsw_audio_codec_enable() (Jani)
> > >> > Fixed the port clock typo
> > >> > Added TODO comment
> > >> > Signed-off-by: Dhinakaran Pandiyan 
> > >> > ---
> > >> >  drivers/gpu/drm/i915/i915_reg.h|  5 +
> > >> >  drivers/gpu/drm/i915/intel_audio.c | 30
> +-
> > >> >  2 files changed, 34 insertions(+), 1 deletion(-)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> > >> > index 00efaa1..76dac48 100644
> > >> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > >> > @@ -6236,6 +6236,11 @@ enum {
> > >> >  #define SLICE_ECO_CHICKEN0  _MMIO(0x7308)
> > >> >  #define   PIXEL_MASK_CAMMING_DISABLE(1 << 14)
> > >> >
> > >> > +#define _CHICKEN_TRANS_A0x420C0
> > >> > +#define _CHICKEN_TRANS_B0x420C4
> > >> > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A,
> _CHICKEN_TRANS_B)
> > >> > +#define SPARE_13(1<<13)
> > >> > +
> > >> >  /* WaCatErrorRejectionIssue */
> > >> >  #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG  _MMIO(0x9030)
> > >> >  #define  GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB   (1<<11)
> > >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> b/drivers/gpu/drm/i915/intel_audio.c
> > >> > index 7093cfb..894f11e 100644
> > >> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > >> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > >> > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct
> intel_encoder *encoder)
> > >> >  {
> > >> >  struct drm_i915_private *dev_priv =
> to_i915(encoder->base.dev);
> > >> >  struct intel_crtc *intel_crtc =
> to_intel_crtc(encoder->base.crtc);
> > >> > +struct intel_crtc_state *crtc_config =  intel_crtc->config;
> > >> > +enum transcoder cpu_transcoder =
> crtc_config->cpu_transcoder;
> > >> >  enum pipe pipe = intel_crtc->pipe;
> > >> >  uint32_t tmp;
> > >> >
> > >> > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct
> intel_

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2016-11-05 Thread Pandiyan, Dhinakaran
On Sat, 2016-11-05 at 21:40 +0200, Jani Nikula wrote:
> On Fri, 04 Nov 2016, "Pandiyan, Dhinakaran"  
> wrote:
> > On Fri, 2016-11-04 at 17:48 +0200, Jani Nikula wrote:
> >> On Wed, 26 Oct 2016, Dhinakaran Pandiyan  
> >> wrote:
> >> > Enabling DP audio stall fix is necessary to play audio over DP HBR2. So,
> >> > let's set this bit right before enabling the audio codec. Playing audio
> >> > without setting this bit results in pipe FIFO underruns.
> >> >
> >> > This workaround is applicable only for audio sample rates up to 96kHz. 
> >> > For
> >> > frequencies above 96kHz, this is insufficient and cdclk should be 
> >> > increased
> >> > to at least 432 MHz, just like BDW. Since, the audio driver does not
> >> > support sample rates > 48 kHz, we are safe with this fix for now.
> >> 
> >> Do we still need this patch now that these two have been pushed?
> >> 
> >> b30ce9e0552a drm/i915/dp: BDW cdclk fix for DP audio
> >> 9c7540241885 drm/i915/dp: Extend BDW DP audio workaround to GEN9 platforms
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> 
> >
> > No, we are good afaik. This patch would have helped us to make use of a
> > lower cdclk (337.5 MHz), with constraints on audio bit rate. Operating
> > at 432 MHz, like we do now, rules out the need for this patch.
> 
> Hmm, what about 5.4 Gbps link with 1 or 2 lanes?
> 
> BR,
> Jani.
> 

Good point, I think it will depend on the audio sampling rate. But, I
have to figure out a way to play high sampling rate audio (> 96 KHz) and
test 5.4 Gbps with 1 or 2 lanes.

The other option is to play safe and apply this patch with even lesser
restrictions, say link rate >= 2.7 Gbps.


-DK

> >
> > -DK
> >
> >> >
> >> > v2: Inlined the code change within hsw_audio_codec_enable() (Jani)
> >> > Fixed the port clock typo
> >> > Added TODO comment
> >> > Signed-off-by: Dhinakaran Pandiyan 
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_reg.h|  5 +
> >> >  drivers/gpu/drm/i915/intel_audio.c | 30 +-
> >> >  2 files changed, 34 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> >> > b/drivers/gpu/drm/i915/i915_reg.h
> >> > index 00efaa1..76dac48 100644
> >> > --- a/drivers/gpu/drm/i915/i915_reg.h
> >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> > @@ -6236,6 +6236,11 @@ enum {
> >> >  #define SLICE_ECO_CHICKEN0  _MMIO(0x7308)
> >> >  #define   PIXEL_MASK_CAMMING_DISABLE(1 << 14)
> >> >  
> >> > +#define _CHICKEN_TRANS_A0x420C0
> >> > +#define _CHICKEN_TRANS_B0x420C4
> >> > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, 
> >> > _CHICKEN_TRANS_B)
> >> > +#define SPARE_13(1<<13)
> >> > +
> >> >  /* WaCatErrorRejectionIssue */
> >> >  #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG  _MMIO(0x9030)
> >> >  #define  GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB   (1<<11)
> >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c 
> >> > b/drivers/gpu/drm/i915/intel_audio.c
> >> > index 7093cfb..894f11e 100644
> >> > --- a/drivers/gpu/drm/i915/intel_audio.c
> >> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> >> > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct 
> >> > intel_encoder *encoder)
> >> >  {
> >> >  struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >> >  struct intel_crtc *intel_crtc = 
> >> > to_intel_crtc(encoder->base.crtc);
> >> > +struct intel_crtc_state *crtc_config =  intel_crtc->config;
> >> > +enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
> >> >  enum pipe pipe = intel_crtc->pipe;
> >> >  uint32_t tmp;
> >> >  
> >> > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct 
> >> > intel_encoder *encoder)
> >> >  
> >> >  mutex_lock(&dev_priv->av_mutex);
> >> >  
> >> > +/*Disable DP audio stall fix for HBR2*/
> >> > +if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) 
> >> > &&
> >> > +crtc_config->port_clock >= 54) {
> >> > +tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
> >> > +tmp &= ~SPARE_13;
> >> > +I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
> >> > +}
> >> > +
> >> >  /* Disable timestamps */
> >> >  tmp = I915_READ(HSW_AUD_CFG(pipe));
> >> >  tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> >> >  tmp |= AUD_CONFIG_N_PROG_ENABLE;
> >> >  tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> >> >  tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> >> > -if (intel_crtc_has_dp_encoder(intel_crtc->config))
> >> > +if (intel_crtc_has_dp_encoder(crtc_config))
> >> >  tmp |= AUD_CONFIG_N_VALUE_INDEX;
> >> >  I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> >> >  
> >> > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct 
> >> > drm_connector *connector,
> >> >  {
> >> >  struct drm_i915_private *dev_priv = to_i915(connector->dev);
> >> >  struct intel_crtc *intel_crtc 

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2016-11-05 Thread Jani Nikula
On Fri, 04 Nov 2016, "Pandiyan, Dhinakaran"  
wrote:
> On Fri, 2016-11-04 at 17:48 +0200, Jani Nikula wrote:
>> On Wed, 26 Oct 2016, Dhinakaran Pandiyan  
>> wrote:
>> > Enabling DP audio stall fix is necessary to play audio over DP HBR2. So,
>> > let's set this bit right before enabling the audio codec. Playing audio
>> > without setting this bit results in pipe FIFO underruns.
>> >
>> > This workaround is applicable only for audio sample rates up to 96kHz. For
>> > frequencies above 96kHz, this is insufficient and cdclk should be increased
>> > to at least 432 MHz, just like BDW. Since, the audio driver does not
>> > support sample rates > 48 kHz, we are safe with this fix for now.
>> 
>> Do we still need this patch now that these two have been pushed?
>> 
>> b30ce9e0552a drm/i915/dp: BDW cdclk fix for DP audio
>> 9c7540241885 drm/i915/dp: Extend BDW DP audio workaround to GEN9 platforms
>> 
>> BR,
>> Jani.
>> 
>> 
>> 
>
> No, we are good afaik. This patch would have helped us to make use of a
> lower cdclk (337.5 MHz), with constraints on audio bit rate. Operating
> at 432 MHz, like we do now, rules out the need for this patch.

Hmm, what about 5.4 Gbps link with 1 or 2 lanes?

BR,
Jani.


>
> -DK
>
>> >
>> > v2: Inlined the code change within hsw_audio_codec_enable() (Jani)
>> > Fixed the port clock typo
>> > Added TODO comment
>> > Signed-off-by: Dhinakaran Pandiyan 
>> > ---
>> >  drivers/gpu/drm/i915/i915_reg.h|  5 +
>> >  drivers/gpu/drm/i915/intel_audio.c | 30 +-
>> >  2 files changed, 34 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> > b/drivers/gpu/drm/i915/i915_reg.h
>> > index 00efaa1..76dac48 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -6236,6 +6236,11 @@ enum {
>> >  #define SLICE_ECO_CHICKEN0_MMIO(0x7308)
>> >  #define   PIXEL_MASK_CAMMING_DISABLE  (1 << 14)
>> >  
>> > +#define _CHICKEN_TRANS_A  0x420C0
>> > +#define _CHICKEN_TRANS_B  0x420C4
>> > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, 
>> > _CHICKEN_TRANS_B)
>> > +#define SPARE_13  (1<<13)
>> > +
>> >  /* WaCatErrorRejectionIssue */
>> >  #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG_MMIO(0x9030)
>> >  #define  GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB (1<<11)
>> > diff --git a/drivers/gpu/drm/i915/intel_audio.c 
>> > b/drivers/gpu/drm/i915/intel_audio.c
>> > index 7093cfb..894f11e 100644
>> > --- a/drivers/gpu/drm/i915/intel_audio.c
>> > +++ b/drivers/gpu/drm/i915/intel_audio.c
>> > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct 
>> > intel_encoder *encoder)
>> >  {
>> >struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> >struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>> > +  struct intel_crtc_state *crtc_config =  intel_crtc->config;
>> > +  enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
>> >enum pipe pipe = intel_crtc->pipe;
>> >uint32_t tmp;
>> >  
>> > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct 
>> > intel_encoder *encoder)
>> >  
>> >mutex_lock(&dev_priv->av_mutex);
>> >  
>> > +  /*Disable DP audio stall fix for HBR2*/
>> > +  if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
>> > +  crtc_config->port_clock >= 54) {
>> > +  tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
>> > +  tmp &= ~SPARE_13;
>> > +  I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
>> > +  }
>> > +
>> >/* Disable timestamps */
>> >tmp = I915_READ(HSW_AUD_CFG(pipe));
>> >tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>> >tmp |= AUD_CONFIG_N_PROG_ENABLE;
>> >tmp &= ~AUD_CONFIG_UPPER_N_MASK;
>> >tmp &= ~AUD_CONFIG_LOWER_N_MASK;
>> > -  if (intel_crtc_has_dp_encoder(intel_crtc->config))
>> > +  if (intel_crtc_has_dp_encoder(crtc_config))
>> >tmp |= AUD_CONFIG_N_VALUE_INDEX;
>> >I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>> >  
>> > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct 
>> > drm_connector *connector,
>> >  {
>> >struct drm_i915_private *dev_priv = to_i915(connector->dev);
>> >struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
>> > +  struct intel_crtc_state *crtc_config =  intel_crtc->config;
>> > +  enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
>> >enum pipe pipe = intel_crtc->pipe;
>> >enum port port = intel_encoder->port;
>> >const uint8_t *eld = connector->eld;
>> > @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct 
>> > drm_connector *connector,
>> >  
>> >mutex_lock(&dev_priv->av_mutex);
>> >  
>> > +  /* Enable DP audio stall fix for HBR2
>> > +   *
>> > +   * TODO: This workaround is applicable only for audio sample rates up
>> > +   * to 96kHz. For frequencies above 96kHz, this is insufficient and
>> > +   * cdclk should be increased to at least 432 MHz, just like BDW. Since,
>> > +   * the

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2016-11-04 Thread Pandiyan, Dhinakaran
On Fri, 2016-11-04 at 17:48 +0200, Jani Nikula wrote:
> On Wed, 26 Oct 2016, Dhinakaran Pandiyan  
> wrote:
> > Enabling DP audio stall fix is necessary to play audio over DP HBR2. So,
> > let's set this bit right before enabling the audio codec. Playing audio
> > without setting this bit results in pipe FIFO underruns.
> >
> > This workaround is applicable only for audio sample rates up to 96kHz. For
> > frequencies above 96kHz, this is insufficient and cdclk should be increased
> > to at least 432 MHz, just like BDW. Since, the audio driver does not
> > support sample rates > 48 kHz, we are safe with this fix for now.
> 
> Do we still need this patch now that these two have been pushed?
> 
> b30ce9e0552a drm/i915/dp: BDW cdclk fix for DP audio
> 9c7540241885 drm/i915/dp: Extend BDW DP audio workaround to GEN9 platforms
> 
> BR,
> Jani.
> 
> 
> 

No, we are good afaik. This patch would have helped us to make use of a
lower cdclk (337.5 MHz), with constraints on audio bit rate. Operating
at 432 MHz, like we do now, rules out the need for this patch.

-DK

> >
> > v2: Inlined the code change within hsw_audio_codec_enable() (Jani)
> > Fixed the port clock typo
> > Added TODO comment
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h|  5 +
> >  drivers/gpu/drm/i915/intel_audio.c | 30 +-
> >  2 files changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 00efaa1..76dac48 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6236,6 +6236,11 @@ enum {
> >  #define SLICE_ECO_CHICKEN0 _MMIO(0x7308)
> >  #define   PIXEL_MASK_CAMMING_DISABLE   (1 << 14)
> >  
> > +#define _CHICKEN_TRANS_A   0x420C0
> > +#define _CHICKEN_TRANS_B   0x420C4
> > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, 
> > _CHICKEN_TRANS_B)
> > +#define SPARE_13   (1<<13)
> > +
> >  /* WaCatErrorRejectionIssue */
> >  #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG _MMIO(0x9030)
> >  #define  GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB  (1<<11)
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c 
> > b/drivers/gpu/drm/i915/intel_audio.c
> > index 7093cfb..894f11e 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct 
> > intel_encoder *encoder)
> >  {
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> > +   struct intel_crtc_state *crtc_config =  intel_crtc->config;
> > +   enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
> > enum pipe pipe = intel_crtc->pipe;
> > uint32_t tmp;
> >  
> > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct 
> > intel_encoder *encoder)
> >  
> > mutex_lock(&dev_priv->av_mutex);
> >  
> > +   /*Disable DP audio stall fix for HBR2*/
> > +   if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
> > +   crtc_config->port_clock >= 54) {
> > +   tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
> > +   tmp &= ~SPARE_13;
> > +   I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
> > +   }
> > +
> > /* Disable timestamps */
> > tmp = I915_READ(HSW_AUD_CFG(pipe));
> > tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> > tmp |= AUD_CONFIG_N_PROG_ENABLE;
> > tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> > tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> > -   if (intel_crtc_has_dp_encoder(intel_crtc->config))
> > +   if (intel_crtc_has_dp_encoder(crtc_config))
> > tmp |= AUD_CONFIG_N_VALUE_INDEX;
> > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> >  
> > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct drm_connector 
> > *connector,
> >  {
> > struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
> > +   struct intel_crtc_state *crtc_config =  intel_crtc->config;
> > +   enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
> > enum pipe pipe = intel_crtc->pipe;
> > enum port port = intel_encoder->port;
> > const uint8_t *eld = connector->eld;
> > @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct 
> > drm_connector *connector,
> >  
> > mutex_lock(&dev_priv->av_mutex);
> >  
> > +   /* Enable DP audio stall fix for HBR2
> > +*
> > +* TODO: This workaround is applicable only for audio sample rates up
> > +* to 96kHz. For frequencies above 96kHz, this is insufficient and
> > +* cdclk should be increased to at least 432 MHz, just like BDW. Since,
> > +* the audio driver does not support sample rates > 48 kHz, we are safe
> > +* with this fix for now.
> > +*/
> > +
> > +   if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
> > +   

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2016-11-04 Thread Jani Nikula
On Wed, 26 Oct 2016, Dhinakaran Pandiyan  wrote:
> Enabling DP audio stall fix is necessary to play audio over DP HBR2. So,
> let's set this bit right before enabling the audio codec. Playing audio
> without setting this bit results in pipe FIFO underruns.
>
> This workaround is applicable only for audio sample rates up to 96kHz. For
> frequencies above 96kHz, this is insufficient and cdclk should be increased
> to at least 432 MHz, just like BDW. Since, the audio driver does not
> support sample rates > 48 kHz, we are safe with this fix for now.

Do we still need this patch now that these two have been pushed?

b30ce9e0552a drm/i915/dp: BDW cdclk fix for DP audio
9c7540241885 drm/i915/dp: Extend BDW DP audio workaround to GEN9 platforms

BR,
Jani.



>
> v2: Inlined the code change within hsw_audio_codec_enable() (Jani)
> Fixed the port clock typo
> Added TODO comment
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/i915/i915_reg.h|  5 +
>  drivers/gpu/drm/i915/intel_audio.c | 30 +-
>  2 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 00efaa1..76dac48 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6236,6 +6236,11 @@ enum {
>  #define SLICE_ECO_CHICKEN0   _MMIO(0x7308)
>  #define   PIXEL_MASK_CAMMING_DISABLE (1 << 14)
>  
> +#define _CHICKEN_TRANS_A 0x420C0
> +#define _CHICKEN_TRANS_B 0x420C4
> +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, 
> _CHICKEN_TRANS_B)
> +#define SPARE_13 (1<<13)
> +
>  /* WaCatErrorRejectionIssue */
>  #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG   _MMIO(0x9030)
>  #define  GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB(1<<11)
> diff --git a/drivers/gpu/drm/i915/intel_audio.c 
> b/drivers/gpu/drm/i915/intel_audio.c
> index 7093cfb..894f11e 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct intel_encoder 
> *encoder)
>  {
>   struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>   struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> + struct intel_crtc_state *crtc_config =  intel_crtc->config;
> + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
>   enum pipe pipe = intel_crtc->pipe;
>   uint32_t tmp;
>  
> @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct 
> intel_encoder *encoder)
>  
>   mutex_lock(&dev_priv->av_mutex);
>  
> + /*Disable DP audio stall fix for HBR2*/
> + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
> + crtc_config->port_clock >= 54) {
> + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
> + tmp &= ~SPARE_13;
> + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
> + }
> +
>   /* Disable timestamps */
>   tmp = I915_READ(HSW_AUD_CFG(pipe));
>   tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>   tmp |= AUD_CONFIG_N_PROG_ENABLE;
>   tmp &= ~AUD_CONFIG_UPPER_N_MASK;
>   tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> - if (intel_crtc_has_dp_encoder(intel_crtc->config))
> + if (intel_crtc_has_dp_encoder(crtc_config))
>   tmp |= AUD_CONFIG_N_VALUE_INDEX;
>   I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>  
> @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct drm_connector 
> *connector,
>  {
>   struct drm_i915_private *dev_priv = to_i915(connector->dev);
>   struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
> + struct intel_crtc_state *crtc_config =  intel_crtc->config;
> + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
>   enum pipe pipe = intel_crtc->pipe;
>   enum port port = intel_encoder->port;
>   const uint8_t *eld = connector->eld;
> @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct drm_connector 
> *connector,
>  
>   mutex_lock(&dev_priv->av_mutex);
>  
> + /* Enable DP audio stall fix for HBR2
> +  *
> +  * TODO: This workaround is applicable only for audio sample rates up
> +  * to 96kHz. For frequencies above 96kHz, this is insufficient and
> +  * cdclk should be increased to at least 432 MHz, just like BDW. Since,
> +  * the audio driver does not support sample rates > 48 kHz, we are safe
> +  * with this fix for now.
> +  */
> +
> + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
> + crtc_config->port_clock >= 54) {
> + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
> + tmp |= SPARE_13;
> + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
> + }
> +
>   /* Enable audio presence detect, invalidate ELD */
>   tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
>   tmp |= AUDIO_OUTPUT_ENABLE(pipe);

-- 
Jani Nikula, Intel Open Source Technol

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2016-10-27 Thread Yang, Libin

> -Original Message-
> From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of
> Pandiyan, Dhinakaran
> Sent: Thursday, October 27, 2016 2:14 AM
> To: ville.syrj...@linux.intel.com
> Cc: Nikula, Jani ; Kp, Jeeja ;
> intel-gfx@lists.freedesktop.org; libin.y...@linux.intel.com
> Subject: Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix
> for gen9 platforms
> 
> On Wed, 2016-10-26 at 12:11 +0300, Ville Syrjälä wrote:
> > On Tue, Oct 25, 2016 at 07:37:36PM -0700, Dhinakaran Pandiyan wrote:
> > > Enabling DP audio stall fix is necessary to play audio over DP HBR2.
> > > So, let's set this bit right before enabling the audio codec.
> > > Playing audio without setting this bit results in pipe FIFO underruns.
> > >
> > > This workaround is applicable only for audio sample rates up to
> > > 96kHz. For frequencies above 96kHz, this is insufficient and cdclk
> > > should be increased to at least 432 MHz, just like BDW. Since, the
> > > audio driver does not support sample rates > 48 kHz, we are safe with
> this fix for now.
> > >
> > > v2: Inlined the code change within hsw_audio_codec_enable() (Jani)
> > > Fixed the port clock typo
> > > Added TODO comment
> > > Signed-off-by: Dhinakaran Pandiyan 
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h|  5 +
> > >  drivers/gpu/drm/i915/intel_audio.c | 30
> > > +-
> > >  2 files changed, 34 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h index 00efaa1..76dac48 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -6236,6 +6236,11 @@ enum {
> > >  #define SLICE_ECO_CHICKEN0   _MMIO(0x7308)
> > >  #define   PIXEL_MASK_CAMMING_DISABLE (1 << 14)
> > >
> > > +#define _CHICKEN_TRANS_A 0x420C0
> > > +#define _CHICKEN_TRANS_B 0x420C4
> > > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran,
> _CHICKEN_TRANS_A, _CHICKEN_TRANS_B)
> > > +#define SPARE_13 (1<<13)
> > > +
> > >  /* WaCatErrorRejectionIssue */
> > >  #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG
>   _MMIO(0x9030)
> > >  #define  GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB(1<<11)
> > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > > b/drivers/gpu/drm/i915/intel_audio.c
> > > index 7093cfb..894f11e 100644
> > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct
> > > intel_encoder *encoder)  {
> > >   struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > >   struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> > > + struct intel_crtc_state *crtc_config =  intel_crtc->config;
> > > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
> > >   enum pipe pipe = intel_crtc->pipe;
> > >   uint32_t tmp;
> > >
> > > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct
> > > intel_encoder *encoder)
> > >
> > >   mutex_lock(&dev_priv->av_mutex);
> > >
> > > + /*Disable DP audio stall fix for HBR2*/
> > > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
> > > + crtc_config->port_clock >= 54) {
> > > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
> > > + tmp &= ~SPARE_13;
> > > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
> > > + }
> > > +
> > >   /* Disable timestamps */
> > >   tmp = I915_READ(HSW_AUD_CFG(pipe));
> > >   tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> > >   tmp |= AUD_CONFIG_N_PROG_ENABLE;
> > >   tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> > >   tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> > > - if (intel_crtc_has_dp_encoder(intel_crtc->config))
> > > + if (intel_crtc_has_dp_encoder(crtc_config))
> > >   tmp |= AUD_CONFIG_N_VALUE_INDEX;
> > >   I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > >
> > > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct
> > > drm_connector *connector,  {
> > >   struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > >   struct intel_crtc *intel_crtc =
> > > to_intel_crtc(intel_encoder->base.crtc);
>

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2016-10-27 Thread Pandiyan, Dhinakaran
On Wed, 2016-10-26 at 18:14 +, Pandiyan, Dhinakaran wrote:
> On Wed, 2016-10-26 at 12:11 +0300, Ville Syrjälä wrote:
> > On Tue, Oct 25, 2016 at 07:37:36PM -0700, Dhinakaran Pandiyan wrote:
> > > Enabling DP audio stall fix is necessary to play audio over DP HBR2. So,
> > > let's set this bit right before enabling the audio codec. Playing audio
> > > without setting this bit results in pipe FIFO underruns.
> > > 
> > > This workaround is applicable only for audio sample rates up to 96kHz. For
> > > frequencies above 96kHz, this is insufficient and cdclk should be 
> > > increased
> > > to at least 432 MHz, just like BDW. Since, the audio driver does not
> > > support sample rates > 48 kHz, we are safe with this fix for now.
> > > 
> > > v2: Inlined the code change within hsw_audio_codec_enable() (Jani)
> > > Fixed the port clock typo
> > > Added TODO comment
> > > Signed-off-by: Dhinakaran Pandiyan 
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h|  5 +
> > >  drivers/gpu/drm/i915/intel_audio.c | 30 +-
> > >  2 files changed, 34 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 00efaa1..76dac48 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -6236,6 +6236,11 @@ enum {
> > >  #define SLICE_ECO_CHICKEN0   _MMIO(0x7308)
> > >  #define   PIXEL_MASK_CAMMING_DISABLE (1 << 14)
> > >  
> > > +#define _CHICKEN_TRANS_A 0x420C0
> > > +#define _CHICKEN_TRANS_B 0x420C4
> > > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, 
> > > _CHICKEN_TRANS_B)
> > > +#define SPARE_13 (1<<13)
> > > +
> > >  /* WaCatErrorRejectionIssue */
> > >  #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG   _MMIO(0x9030)
> > >  #define  GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB(1<<11)
> > > diff --git a/drivers/gpu/drm/i915/intel_audio.c 
> > > b/drivers/gpu/drm/i915/intel_audio.c
> > > index 7093cfb..894f11e 100644
> > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct 
> > > intel_encoder *encoder)
> > >  {
> > >   struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > >   struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> > > + struct intel_crtc_state *crtc_config =  intel_crtc->config;
> > > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
> > >   enum pipe pipe = intel_crtc->pipe;
> > >   uint32_t tmp;
> > >  
> > > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct 
> > > intel_encoder *encoder)
> > >  
> > >   mutex_lock(&dev_priv->av_mutex);
> > >  
> > > + /*Disable DP audio stall fix for HBR2*/
> > > + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
> > > + crtc_config->port_clock >= 54) {
> > > + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
> > > + tmp &= ~SPARE_13;
> > > + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
> > > + }
> > > +
> > >   /* Disable timestamps */
> > >   tmp = I915_READ(HSW_AUD_CFG(pipe));
> > >   tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> > >   tmp |= AUD_CONFIG_N_PROG_ENABLE;
> > >   tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> > >   tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> > > - if (intel_crtc_has_dp_encoder(intel_crtc->config))
> > > + if (intel_crtc_has_dp_encoder(crtc_config))
> > >   tmp |= AUD_CONFIG_N_VALUE_INDEX;
> > >   I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > >  
> > > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct 
> > > drm_connector *connector,
> > >  {
> > >   struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > >   struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
> > > + struct intel_crtc_state *crtc_config =  intel_crtc->config;
> > > + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
> > >   enum pipe pipe = intel_crtc->pipe;
> > >   enum port port = intel_encoder->port;
> > >   const uint8_t *eld = connector->eld;
> > > @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct 
> > > drm_connector *connector,
> > >  
> > >   mutex_lock(&dev_priv->av_mutex);
> > >  
> > > + /* Enable DP audio stall fix for HBR2
> > > +  *
> > > +  * TODO: This workaround is applicable only for audio sample rates up
> > > +  * to 96kHz. For frequencies above 96kHz, this is insufficient and
> > > +  * cdclk should be increased to at least 432 MHz, just like BDW. Since,
> > > +  * the audio driver does not support sample rates > 48 kHz, we are safe
> > > +  * with this fix for now.
> > 
> > Where in the sound driver is this supposed 96kHz limit? I see a lot of
> > stuff for >96kHz in the code at least.
> > 
> 
> Libin/Jeeja can you help me here?
> 
> 

I see another problem, we cannot change cdclk without a full modeset. So
choosing the workarounds based on audio sampling rate is going to be
tricky. I guess

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2016-10-27 Thread Pandiyan, Dhinakaran
On Wed, 2016-10-26 at 12:11 +0300, Ville Syrjälä wrote:
> On Tue, Oct 25, 2016 at 07:37:36PM -0700, Dhinakaran Pandiyan wrote:
> > Enabling DP audio stall fix is necessary to play audio over DP HBR2. So,
> > let's set this bit right before enabling the audio codec. Playing audio
> > without setting this bit results in pipe FIFO underruns.
> > 
> > This workaround is applicable only for audio sample rates up to 96kHz. For
> > frequencies above 96kHz, this is insufficient and cdclk should be increased
> > to at least 432 MHz, just like BDW. Since, the audio driver does not
> > support sample rates > 48 kHz, we are safe with this fix for now.
> > 
> > v2: Inlined the code change within hsw_audio_codec_enable() (Jani)
> > Fixed the port clock typo
> > Added TODO comment
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h|  5 +
> >  drivers/gpu/drm/i915/intel_audio.c | 30 +-
> >  2 files changed, 34 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 00efaa1..76dac48 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6236,6 +6236,11 @@ enum {
> >  #define SLICE_ECO_CHICKEN0 _MMIO(0x7308)
> >  #define   PIXEL_MASK_CAMMING_DISABLE   (1 << 14)
> >  
> > +#define _CHICKEN_TRANS_A   0x420C0
> > +#define _CHICKEN_TRANS_B   0x420C4
> > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, 
> > _CHICKEN_TRANS_B)
> > +#define SPARE_13   (1<<13)
> > +
> >  /* WaCatErrorRejectionIssue */
> >  #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG _MMIO(0x9030)
> >  #define  GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB  (1<<11)
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c 
> > b/drivers/gpu/drm/i915/intel_audio.c
> > index 7093cfb..894f11e 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct 
> > intel_encoder *encoder)
> >  {
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> > +   struct intel_crtc_state *crtc_config =  intel_crtc->config;
> > +   enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
> > enum pipe pipe = intel_crtc->pipe;
> > uint32_t tmp;
> >  
> > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct 
> > intel_encoder *encoder)
> >  
> > mutex_lock(&dev_priv->av_mutex);
> >  
> > +   /*Disable DP audio stall fix for HBR2*/
> > +   if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
> > +   crtc_config->port_clock >= 54) {
> > +   tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
> > +   tmp &= ~SPARE_13;
> > +   I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
> > +   }
> > +
> > /* Disable timestamps */
> > tmp = I915_READ(HSW_AUD_CFG(pipe));
> > tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> > tmp |= AUD_CONFIG_N_PROG_ENABLE;
> > tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> > tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> > -   if (intel_crtc_has_dp_encoder(intel_crtc->config))
> > +   if (intel_crtc_has_dp_encoder(crtc_config))
> > tmp |= AUD_CONFIG_N_VALUE_INDEX;
> > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> >  
> > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct drm_connector 
> > *connector,
> >  {
> > struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
> > +   struct intel_crtc_state *crtc_config =  intel_crtc->config;
> > +   enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
> > enum pipe pipe = intel_crtc->pipe;
> > enum port port = intel_encoder->port;
> > const uint8_t *eld = connector->eld;
> > @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct 
> > drm_connector *connector,
> >  
> > mutex_lock(&dev_priv->av_mutex);
> >  
> > +   /* Enable DP audio stall fix for HBR2
> > +*
> > +* TODO: This workaround is applicable only for audio sample rates up
> > +* to 96kHz. For frequencies above 96kHz, this is insufficient and
> > +* cdclk should be increased to at least 432 MHz, just like BDW. Since,
> > +* the audio driver does not support sample rates > 48 kHz, we are safe
> > +* with this fix for now.
> 
> Where in the sound driver is this supposed 96kHz limit? I see a lot of
> stuff for >96kHz in the code at least.
> 

Libin/Jeeja can you help me here?


> > +*/
> > +
> > +   if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
> > +   crtc_config->port_clock >= 54) {
> > +   tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
> > +   tmp |= SPARE_13;
> > +   I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
> > +   }
> > +
> > /* Enable audio presence detect, invali

[Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2016-10-27 Thread Dhinakaran Pandiyan
Enabling DP audio stall fix is necessary to play audio over DP HBR2. So,
let's set this bit right before enabling the audio codec. Playing audio
without setting this bit results in pipe FIFO underruns.

This workaround is applicable only for audio sample rates up to 96kHz. For
frequencies above 96kHz, this is insufficient and cdclk should be increased
to at least 432 MHz, just like BDW. Since, the audio driver does not
support sample rates > 48 kHz, we are safe with this fix for now.

v2: Inlined the code change within hsw_audio_codec_enable() (Jani)
Fixed the port clock typo
Added TODO comment
Signed-off-by: Dhinakaran Pandiyan 
---
 drivers/gpu/drm/i915/i915_reg.h|  5 +
 drivers/gpu/drm/i915/intel_audio.c | 30 +-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 00efaa1..76dac48 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6236,6 +6236,11 @@ enum {
 #define SLICE_ECO_CHICKEN0 _MMIO(0x7308)
 #define   PIXEL_MASK_CAMMING_DISABLE   (1 << 14)
 
+#define _CHICKEN_TRANS_A   0x420C0
+#define _CHICKEN_TRANS_B   0x420C4
+#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, 
_CHICKEN_TRANS_B)
+#define SPARE_13   (1<<13)
+
 /* WaCatErrorRejectionIssue */
 #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG _MMIO(0x9030)
 #define  GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB  (1<<11)
diff --git a/drivers/gpu/drm/i915/intel_audio.c 
b/drivers/gpu/drm/i915/intel_audio.c
index 7093cfb..894f11e 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct intel_encoder 
*encoder)
 {
struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+   struct intel_crtc_state *crtc_config =  intel_crtc->config;
+   enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
enum pipe pipe = intel_crtc->pipe;
uint32_t tmp;
 
@@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct intel_encoder 
*encoder)
 
mutex_lock(&dev_priv->av_mutex);
 
+   /*Disable DP audio stall fix for HBR2*/
+   if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
+   crtc_config->port_clock >= 54) {
+   tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
+   tmp &= ~SPARE_13;
+   I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
+   }
+
/* Disable timestamps */
tmp = I915_READ(HSW_AUD_CFG(pipe));
tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
tmp |= AUD_CONFIG_N_PROG_ENABLE;
tmp &= ~AUD_CONFIG_UPPER_N_MASK;
tmp &= ~AUD_CONFIG_LOWER_N_MASK;
-   if (intel_crtc_has_dp_encoder(intel_crtc->config))
+   if (intel_crtc_has_dp_encoder(crtc_config))
tmp |= AUD_CONFIG_N_VALUE_INDEX;
I915_WRITE(HSW_AUD_CFG(pipe), tmp);
 
@@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct drm_connector 
*connector,
 {
struct drm_i915_private *dev_priv = to_i915(connector->dev);
struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
+   struct intel_crtc_state *crtc_config =  intel_crtc->config;
+   enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
enum pipe pipe = intel_crtc->pipe;
enum port port = intel_encoder->port;
const uint8_t *eld = connector->eld;
@@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct drm_connector 
*connector,
 
mutex_lock(&dev_priv->av_mutex);
 
+   /* Enable DP audio stall fix for HBR2
+*
+* TODO: This workaround is applicable only for audio sample rates up
+* to 96kHz. For frequencies above 96kHz, this is insufficient and
+* cdclk should be increased to at least 432 MHz, just like BDW. Since,
+* the audio driver does not support sample rates > 48 kHz, we are safe
+* with this fix for now.
+*/
+
+   if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
+   crtc_config->port_clock >= 54) {
+   tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
+   tmp |= SPARE_13;
+   I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
+   }
+
/* Enable audio presence detect, invalidate ELD */
tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
tmp |= AUDIO_OUTPUT_ENABLE(pipe);
-- 
2.7.4

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


Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2016-10-27 Thread Jani Nikula
On Wed, 26 Oct 2016, Dhinakaran Pandiyan  wrote:
> Enabling DP audio stall fix is necessary to play audio over DP HBR2. So,
> let's set this bit right before enabling the audio codec. Playing audio
> without setting this bit results in pipe FIFO underruns.
>
> This workaround is applicable only for audio sample rates up to 96kHz. For
> frequencies above 96kHz, this is insufficient and cdclk should be increased
> to at least 432 MHz, just like BDW. Since, the audio driver does not
> support sample rates > 48 kHz, we are safe with this fix for now.
>
> v2: Inlined the code change within hsw_audio_codec_enable() (Jani)
> Fixed the port clock typo
> Added TODO comment
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/i915/i915_reg.h|  5 +
>  drivers/gpu/drm/i915/intel_audio.c | 30 +-
>  2 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 00efaa1..76dac48 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6236,6 +6236,11 @@ enum {
>  #define SLICE_ECO_CHICKEN0   _MMIO(0x7308)
>  #define   PIXEL_MASK_CAMMING_DISABLE (1 << 14)
>  
> +#define _CHICKEN_TRANS_A 0x420C0
> +#define _CHICKEN_TRANS_B 0x420C4
> +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, 
> _CHICKEN_TRANS_B)
> +#define SPARE_13 (1<<13)
> +
>  /* WaCatErrorRejectionIssue */
>  #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG   _MMIO(0x9030)
>  #define  GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB(1<<11)
> diff --git a/drivers/gpu/drm/i915/intel_audio.c 
> b/drivers/gpu/drm/i915/intel_audio.c
> index 7093cfb..894f11e 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct intel_encoder 
> *encoder)
>  {
>   struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>   struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> + struct intel_crtc_state *crtc_config =  intel_crtc->config;
> + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
>   enum pipe pipe = intel_crtc->pipe;
>   uint32_t tmp;
>  
> @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct 
> intel_encoder *encoder)
>  
>   mutex_lock(&dev_priv->av_mutex);
>  
> + /*Disable DP audio stall fix for HBR2*/

Nitpick, spaces after /* and before */.

> + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
> + crtc_config->port_clock >= 54) {
> + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
> + tmp &= ~SPARE_13;
> + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
> + }

Hmm. Why don't we just do this unconditionally?

> +
>   /* Disable timestamps */
>   tmp = I915_READ(HSW_AUD_CFG(pipe));
>   tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>   tmp |= AUD_CONFIG_N_PROG_ENABLE;
>   tmp &= ~AUD_CONFIG_UPPER_N_MASK;
>   tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> - if (intel_crtc_has_dp_encoder(intel_crtc->config))
> + if (intel_crtc_has_dp_encoder(crtc_config))
>   tmp |= AUD_CONFIG_N_VALUE_INDEX;
>   I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>  
> @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct drm_connector 
> *connector,
>  {
>   struct drm_i915_private *dev_priv = to_i915(connector->dev);
>   struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
> + struct intel_crtc_state *crtc_config =  intel_crtc->config;
> + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
>   enum pipe pipe = intel_crtc->pipe;
>   enum port port = intel_encoder->port;
>   const uint8_t *eld = connector->eld;
> @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct drm_connector 
> *connector,
>  
>   mutex_lock(&dev_priv->av_mutex);
>  
> + /* Enable DP audio stall fix for HBR2
> +  *
> +  * TODO: This workaround is applicable only for audio sample rates up
> +  * to 96kHz. For frequencies above 96kHz, this is insufficient and
> +  * cdclk should be increased to at least 432 MHz, just like BDW. Since,
> +  * the audio driver does not support sample rates > 48 kHz, we are safe
> +  * with this fix for now.
> +  */

Is this TODO required if you already have that check in patch 2/2?

> +
> + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
> + crtc_config->port_clock >= 54) {
> + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
> + tmp |= SPARE_13;
> + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
> + }
> +
>   /* Enable audio presence detect, invalidate ELD */
>   tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
>   tmp |= AUDIO_OUTPUT_ENABLE(pipe);

-- 
Jani Nikula, Intel Open Source Technology Center
_

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2016-10-26 Thread Pandiyan, Dhinakaran
On Wed, 2016-10-26 at 22:06 +0300, Jani Nikula wrote:
> On Wed, 26 Oct 2016, "Pandiyan, Dhinakaran"  
> wrote:
> > On Wed, 2016-10-26 at 11:57 +0300, Jani Nikula wrote:
> >> On Wed, 26 Oct 2016, Dhinakaran Pandiyan  
> >> wrote:
> >> > Enabling DP audio stall fix is necessary to play audio over DP HBR2. So,
> >> > let's set this bit right before enabling the audio codec. Playing audio
> >> > without setting this bit results in pipe FIFO underruns.
> >> >
> >> > This workaround is applicable only for audio sample rates up to 96kHz. 
> >> > For
> >> > frequencies above 96kHz, this is insufficient and cdclk should be 
> >> > increased
> >> > to at least 432 MHz, just like BDW. Since, the audio driver does not
> >> > support sample rates > 48 kHz, we are safe with this fix for now.
> >> >
> >> > v2: Inlined the code change within hsw_audio_codec_enable() (Jani)
> >> > Fixed the port clock typo
> >> > Added TODO comment
> >> > Signed-off-by: Dhinakaran Pandiyan 
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_reg.h|  5 +
> >> >  drivers/gpu/drm/i915/intel_audio.c | 30 +-
> >> >  2 files changed, 34 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> >> > b/drivers/gpu/drm/i915/i915_reg.h
> >> > index 00efaa1..76dac48 100644
> >> > --- a/drivers/gpu/drm/i915/i915_reg.h
> >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> > @@ -6236,6 +6236,11 @@ enum {
> >> >  #define SLICE_ECO_CHICKEN0  _MMIO(0x7308)
> >> >  #define   PIXEL_MASK_CAMMING_DISABLE(1 << 14)
> >> >  
> >> > +#define _CHICKEN_TRANS_A0x420C0
> >> > +#define _CHICKEN_TRANS_B0x420C4
> >> > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, 
> >> > _CHICKEN_TRANS_B)
> >> > +#define SPARE_13(1<<13)
> >> > +
> >> >  /* WaCatErrorRejectionIssue */
> >> >  #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG  _MMIO(0x9030)
> >> >  #define  GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB   (1<<11)
> >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c 
> >> > b/drivers/gpu/drm/i915/intel_audio.c
> >> > index 7093cfb..894f11e 100644
> >> > --- a/drivers/gpu/drm/i915/intel_audio.c
> >> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> >> > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct 
> >> > intel_encoder *encoder)
> >> >  {
> >> >  struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >> >  struct intel_crtc *intel_crtc = 
> >> > to_intel_crtc(encoder->base.crtc);
> >> > +struct intel_crtc_state *crtc_config =  intel_crtc->config;
> >> > +enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
> >> >  enum pipe pipe = intel_crtc->pipe;
> >> >  uint32_t tmp;
> >> >  
> >> > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct 
> >> > intel_encoder *encoder)
> >> >  
> >> >  mutex_lock(&dev_priv->av_mutex);
> >> >  
> >> > +/*Disable DP audio stall fix for HBR2*/
> >> 
> >> Nitpick, spaces after /* and before */.
> >> 
> >> > +if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) 
> >> > &&
> >> > +crtc_config->port_clock >= 54) {
> >> > +tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
> >> > +tmp &= ~SPARE_13;
> >> > +I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
> >> > +}
> >> 
> >> Hmm. Why don't we just do this unconditionally?
> >> 
> >
> > That bit is disabled by default, so avoiding  two MMIO ops. that are not
> > required.
> 
> That makes no difference on the modeset path.
> 

Because we do a lot of MMIO ops. anyway?


> >
> >> > +
> >> >  /* Disable timestamps */
> >> >  tmp = I915_READ(HSW_AUD_CFG(pipe));
> >> >  tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> >> >  tmp |= AUD_CONFIG_N_PROG_ENABLE;
> >> >  tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> >> >  tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> >> > -if (intel_crtc_has_dp_encoder(intel_crtc->config))
> >> > +if (intel_crtc_has_dp_encoder(crtc_config))
> >> >  tmp |= AUD_CONFIG_N_VALUE_INDEX;
> >> >  I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> >> >  
> >> > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct 
> >> > drm_connector *connector,
> >> >  {
> >> >  struct drm_i915_private *dev_priv = to_i915(connector->dev);
> >> >  struct intel_crtc *intel_crtc = 
> >> > to_intel_crtc(intel_encoder->base.crtc);
> >> > +struct intel_crtc_state *crtc_config =  intel_crtc->config;
> >> > +enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
> >> >  enum pipe pipe = intel_crtc->pipe;
> >> >  enum port port = intel_encoder->port;
> >> >  const uint8_t *eld = connector->eld;
> >> > @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct 
> >> > drm_connector *connector,
> >> >  
> >> >  mutex_lock(&dev_priv->av_mutex);
> >> >  
> >> > +/* E

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2016-10-26 Thread Jani Nikula
On Wed, 26 Oct 2016, "Pandiyan, Dhinakaran"  
wrote:
> On Wed, 2016-10-26 at 11:57 +0300, Jani Nikula wrote:
>> On Wed, 26 Oct 2016, Dhinakaran Pandiyan  
>> wrote:
>> > Enabling DP audio stall fix is necessary to play audio over DP HBR2. So,
>> > let's set this bit right before enabling the audio codec. Playing audio
>> > without setting this bit results in pipe FIFO underruns.
>> >
>> > This workaround is applicable only for audio sample rates up to 96kHz. For
>> > frequencies above 96kHz, this is insufficient and cdclk should be increased
>> > to at least 432 MHz, just like BDW. Since, the audio driver does not
>> > support sample rates > 48 kHz, we are safe with this fix for now.
>> >
>> > v2: Inlined the code change within hsw_audio_codec_enable() (Jani)
>> > Fixed the port clock typo
>> > Added TODO comment
>> > Signed-off-by: Dhinakaran Pandiyan 
>> > ---
>> >  drivers/gpu/drm/i915/i915_reg.h|  5 +
>> >  drivers/gpu/drm/i915/intel_audio.c | 30 +-
>> >  2 files changed, 34 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> > b/drivers/gpu/drm/i915/i915_reg.h
>> > index 00efaa1..76dac48 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -6236,6 +6236,11 @@ enum {
>> >  #define SLICE_ECO_CHICKEN0_MMIO(0x7308)
>> >  #define   PIXEL_MASK_CAMMING_DISABLE  (1 << 14)
>> >  
>> > +#define _CHICKEN_TRANS_A  0x420C0
>> > +#define _CHICKEN_TRANS_B  0x420C4
>> > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, 
>> > _CHICKEN_TRANS_B)
>> > +#define SPARE_13  (1<<13)
>> > +
>> >  /* WaCatErrorRejectionIssue */
>> >  #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG_MMIO(0x9030)
>> >  #define  GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB (1<<11)
>> > diff --git a/drivers/gpu/drm/i915/intel_audio.c 
>> > b/drivers/gpu/drm/i915/intel_audio.c
>> > index 7093cfb..894f11e 100644
>> > --- a/drivers/gpu/drm/i915/intel_audio.c
>> > +++ b/drivers/gpu/drm/i915/intel_audio.c
>> > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct 
>> > intel_encoder *encoder)
>> >  {
>> >struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> >struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>> > +  struct intel_crtc_state *crtc_config =  intel_crtc->config;
>> > +  enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
>> >enum pipe pipe = intel_crtc->pipe;
>> >uint32_t tmp;
>> >  
>> > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct 
>> > intel_encoder *encoder)
>> >  
>> >mutex_lock(&dev_priv->av_mutex);
>> >  
>> > +  /*Disable DP audio stall fix for HBR2*/
>> 
>> Nitpick, spaces after /* and before */.
>> 
>> > +  if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
>> > +  crtc_config->port_clock >= 54) {
>> > +  tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
>> > +  tmp &= ~SPARE_13;
>> > +  I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
>> > +  }
>> 
>> Hmm. Why don't we just do this unconditionally?
>> 
>
> That bit is disabled by default, so avoiding  two MMIO ops. that are not
> required.

That makes no difference on the modeset path.

>
>> > +
>> >/* Disable timestamps */
>> >tmp = I915_READ(HSW_AUD_CFG(pipe));
>> >tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>> >tmp |= AUD_CONFIG_N_PROG_ENABLE;
>> >tmp &= ~AUD_CONFIG_UPPER_N_MASK;
>> >tmp &= ~AUD_CONFIG_LOWER_N_MASK;
>> > -  if (intel_crtc_has_dp_encoder(intel_crtc->config))
>> > +  if (intel_crtc_has_dp_encoder(crtc_config))
>> >tmp |= AUD_CONFIG_N_VALUE_INDEX;
>> >I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>> >  
>> > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct 
>> > drm_connector *connector,
>> >  {
>> >struct drm_i915_private *dev_priv = to_i915(connector->dev);
>> >struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
>> > +  struct intel_crtc_state *crtc_config =  intel_crtc->config;
>> > +  enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
>> >enum pipe pipe = intel_crtc->pipe;
>> >enum port port = intel_encoder->port;
>> >const uint8_t *eld = connector->eld;
>> > @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct 
>> > drm_connector *connector,
>> >  
>> >mutex_lock(&dev_priv->av_mutex);
>> >  
>> > +  /* Enable DP audio stall fix for HBR2
>> > +   *
>> > +   * TODO: This workaround is applicable only for audio sample rates up
>> > +   * to 96kHz. For frequencies above 96kHz, this is insufficient and
>> > +   * cdclk should be increased to at least 432 MHz, just like BDW. Since,
>> > +   * the audio driver does not support sample rates > 48 kHz, we are safe
>> > +   * with this fix for now.
>> > +   */
>> 
>> Is this TODO required if you already have that check in patch 2/2?
>> 
>
> In fact, this patch itself is not required if we are increasing the
> cdclk to 432 MHz

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2016-10-26 Thread Pandiyan, Dhinakaran
On Wed, 2016-10-26 at 11:57 +0300, Jani Nikula wrote:
> On Wed, 26 Oct 2016, Dhinakaran Pandiyan  
> wrote:
> > Enabling DP audio stall fix is necessary to play audio over DP HBR2. So,
> > let's set this bit right before enabling the audio codec. Playing audio
> > without setting this bit results in pipe FIFO underruns.
> >
> > This workaround is applicable only for audio sample rates up to 96kHz. For
> > frequencies above 96kHz, this is insufficient and cdclk should be increased
> > to at least 432 MHz, just like BDW. Since, the audio driver does not
> > support sample rates > 48 kHz, we are safe with this fix for now.
> >
> > v2: Inlined the code change within hsw_audio_codec_enable() (Jani)
> > Fixed the port clock typo
> > Added TODO comment
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h|  5 +
> >  drivers/gpu/drm/i915/intel_audio.c | 30 +-
> >  2 files changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 00efaa1..76dac48 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6236,6 +6236,11 @@ enum {
> >  #define SLICE_ECO_CHICKEN0 _MMIO(0x7308)
> >  #define   PIXEL_MASK_CAMMING_DISABLE   (1 << 14)
> >  
> > +#define _CHICKEN_TRANS_A   0x420C0
> > +#define _CHICKEN_TRANS_B   0x420C4
> > +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, 
> > _CHICKEN_TRANS_B)
> > +#define SPARE_13   (1<<13)
> > +
> >  /* WaCatErrorRejectionIssue */
> >  #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG _MMIO(0x9030)
> >  #define  GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB  (1<<11)
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c 
> > b/drivers/gpu/drm/i915/intel_audio.c
> > index 7093cfb..894f11e 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct 
> > intel_encoder *encoder)
> >  {
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> > +   struct intel_crtc_state *crtc_config =  intel_crtc->config;
> > +   enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
> > enum pipe pipe = intel_crtc->pipe;
> > uint32_t tmp;
> >  
> > @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct 
> > intel_encoder *encoder)
> >  
> > mutex_lock(&dev_priv->av_mutex);
> >  
> > +   /*Disable DP audio stall fix for HBR2*/
> 
> Nitpick, spaces after /* and before */.
> 
> > +   if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
> > +   crtc_config->port_clock >= 54) {
> > +   tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
> > +   tmp &= ~SPARE_13;
> > +   I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
> > +   }
> 
> Hmm. Why don't we just do this unconditionally?
> 

That bit is disabled by default, so avoiding  two MMIO ops. that are not
required.

> > +
> > /* Disable timestamps */
> > tmp = I915_READ(HSW_AUD_CFG(pipe));
> > tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> > tmp |= AUD_CONFIG_N_PROG_ENABLE;
> > tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> > tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> > -   if (intel_crtc_has_dp_encoder(intel_crtc->config))
> > +   if (intel_crtc_has_dp_encoder(crtc_config))
> > tmp |= AUD_CONFIG_N_VALUE_INDEX;
> > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> >  
> > @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct drm_connector 
> > *connector,
> >  {
> > struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
> > +   struct intel_crtc_state *crtc_config =  intel_crtc->config;
> > +   enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
> > enum pipe pipe = intel_crtc->pipe;
> > enum port port = intel_encoder->port;
> > const uint8_t *eld = connector->eld;
> > @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct 
> > drm_connector *connector,
> >  
> > mutex_lock(&dev_priv->av_mutex);
> >  
> > +   /* Enable DP audio stall fix for HBR2
> > +*
> > +* TODO: This workaround is applicable only for audio sample rates up
> > +* to 96kHz. For frequencies above 96kHz, this is insufficient and
> > +* cdclk should be increased to at least 432 MHz, just like BDW. Since,
> > +* the audio driver does not support sample rates > 48 kHz, we are safe
> > +* with this fix for now.
> > +*/
> 
> Is this TODO required if you already have that check in patch 2/2?
> 

In fact, this patch itself is not required if we are increasing the
cdclk to 432 MHz for gen9 platforms (like patch 2/2). Having this
workaround gives us the option running the pipeline at a lower cdclk
frequency i.e., 337.5 MHz, which I believe should be better in te

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Enable DP audio stall fix for gen9 platforms

2016-10-26 Thread Ville Syrjälä
On Tue, Oct 25, 2016 at 07:37:36PM -0700, Dhinakaran Pandiyan wrote:
> Enabling DP audio stall fix is necessary to play audio over DP HBR2. So,
> let's set this bit right before enabling the audio codec. Playing audio
> without setting this bit results in pipe FIFO underruns.
> 
> This workaround is applicable only for audio sample rates up to 96kHz. For
> frequencies above 96kHz, this is insufficient and cdclk should be increased
> to at least 432 MHz, just like BDW. Since, the audio driver does not
> support sample rates > 48 kHz, we are safe with this fix for now.
> 
> v2: Inlined the code change within hsw_audio_codec_enable() (Jani)
> Fixed the port clock typo
> Added TODO comment
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/i915/i915_reg.h|  5 +
>  drivers/gpu/drm/i915/intel_audio.c | 30 +-
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 00efaa1..76dac48 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6236,6 +6236,11 @@ enum {
>  #define SLICE_ECO_CHICKEN0   _MMIO(0x7308)
>  #define   PIXEL_MASK_CAMMING_DISABLE (1 << 14)
>  
> +#define _CHICKEN_TRANS_A 0x420C0
> +#define _CHICKEN_TRANS_B 0x420C4
> +#define CHICKEN_TRANS(tran) _MMIO_TRANS(tran, _CHICKEN_TRANS_A, 
> _CHICKEN_TRANS_B)
> +#define SPARE_13 (1<<13)
> +
>  /* WaCatErrorRejectionIssue */
>  #define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG   _MMIO(0x9030)
>  #define  GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB(1<<11)
> diff --git a/drivers/gpu/drm/i915/intel_audio.c 
> b/drivers/gpu/drm/i915/intel_audio.c
> index 7093cfb..894f11e 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -283,6 +283,8 @@ static void hsw_audio_codec_disable(struct intel_encoder 
> *encoder)
>  {
>   struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>   struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> + struct intel_crtc_state *crtc_config =  intel_crtc->config;
> + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
>   enum pipe pipe = intel_crtc->pipe;
>   uint32_t tmp;
>  
> @@ -290,13 +292,21 @@ static void hsw_audio_codec_disable(struct 
> intel_encoder *encoder)
>  
>   mutex_lock(&dev_priv->av_mutex);
>  
> + /*Disable DP audio stall fix for HBR2*/
> + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
> + crtc_config->port_clock >= 54) {
> + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
> + tmp &= ~SPARE_13;
> + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
> + }
> +
>   /* Disable timestamps */
>   tmp = I915_READ(HSW_AUD_CFG(pipe));
>   tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>   tmp |= AUD_CONFIG_N_PROG_ENABLE;
>   tmp &= ~AUD_CONFIG_UPPER_N_MASK;
>   tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> - if (intel_crtc_has_dp_encoder(intel_crtc->config))
> + if (intel_crtc_has_dp_encoder(crtc_config))
>   tmp |= AUD_CONFIG_N_VALUE_INDEX;
>   I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>  
> @@ -315,6 +325,8 @@ static void hsw_audio_codec_enable(struct drm_connector 
> *connector,
>  {
>   struct drm_i915_private *dev_priv = to_i915(connector->dev);
>   struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
> + struct intel_crtc_state *crtc_config =  intel_crtc->config;
> + enum transcoder cpu_transcoder = crtc_config->cpu_transcoder;
>   enum pipe pipe = intel_crtc->pipe;
>   enum port port = intel_encoder->port;
>   const uint8_t *eld = connector->eld;
> @@ -326,6 +338,22 @@ static void hsw_audio_codec_enable(struct drm_connector 
> *connector,
>  
>   mutex_lock(&dev_priv->av_mutex);
>  
> + /* Enable DP audio stall fix for HBR2
> +  *
> +  * TODO: This workaround is applicable only for audio sample rates up
> +  * to 96kHz. For frequencies above 96kHz, this is insufficient and
> +  * cdclk should be increased to at least 432 MHz, just like BDW. Since,
> +  * the audio driver does not support sample rates > 48 kHz, we are safe
> +  * with this fix for now.

Where in the sound driver is this supposed 96kHz limit? I see a lot of
stuff for >96kHz in the code at least.

> +  */
> +
> + if (IS_GEN9(dev_priv) && intel_crtc_has_dp_encoder(crtc_config) &&
> + crtc_config->port_clock >= 54) {
> + tmp = I915_READ(CHICKEN_TRANS(cpu_transcoder));
> + tmp |= SPARE_13;
> + I915_WRITE(CHICKEN_TRANS(cpu_transcoder), tmp);
> + }
> +
>   /* Enable audio presence detect, invalidate ELD */
>   tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
>   tmp |= AUDIO_OUTPUT_ENABLE(pipe);
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Int