| Message ID | 1351241899-7870-6-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi 2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>: > We need to set the timing override chicken bit after fdi link training > has completed and before we enable the dp transcoder. We also have to > clear that bit again after disabling the pch dp transcoder. > > See "Graphics BSpec: vol4g North Display Engine Registers [IVB], > Display Mode Set Sequence" and "Graphics BSpec: vol4h South Display > Engine Registers [CPT, PPT], South Display Engine Transcoder and FDI > Control, Transcoder Debug and DFT, TRANS_CHICKEN_2" bit 31: > > "Workaround : Enable the override prior to enabling the transcoder. > Disable the override after disabling the transcoder." > > While at it, use the _PIPE macro for the other TRANS_DP register. What confuses me is that the bit name is actually "Autotrain TimingGen Override" and after a quick check it seems we don't use autotrain. Also, by looking at the IVB mode set sequence it seems Auto Train is the *recommended* method since manual is listed as "testing only". So I'm not sure if we need this code now, but it seems we could try to implement the auto train... Also, I'd move the code that sets/unsets the chicken bit to intel_enable_transcoder/intel_disable_transcoder. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_reg.h | 8 +++++++- > drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++++- > 2 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index c7c4b96..84b09ee 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4062,7 +4062,7 @@ > #define TRANS_DP_CTL_A 0xe0300 > #define TRANS_DP_CTL_B 0xe1300 > #define TRANS_DP_CTL_C 0xe2300 > -#define TRANS_DP_CTL(pipe) (TRANS_DP_CTL_A + (pipe) * 0x01000) > +#define TRANS_DP_CTL(pipe) _PIPE(pipe, TRANS_DP_CTL_A, TRANS_DP_CTL_B) > #define TRANS_DP_OUTPUT_ENABLE (1<<31) > #define TRANS_DP_PORT_SEL_B (0<<29) > #define TRANS_DP_PORT_SEL_C (1<<29) > @@ -4082,6 +4082,12 @@ > #define TRANS_DP_HSYNC_ACTIVE_LOW 0 > #define TRANS_DP_SYNC_MASK (3<<3) > > +#define TRANS_CHICKEN_2_A 0xf0064 > +#define TRANS_CHICKEN_2_B 0xf1064 > +#define TRANS_CHICKEN_2_C 0xf2064 > +#define TRANS_CHICKEN_2(pipe) _PIPE(pipe, TRANS_CHICKEN_2_A, TRANS_CHICKEN_2_B) > +#define TRANS_CHICKEN2_TIMING_OVERRIDE (1<<31) > + > /* SNB eDP training params */ > /* SNB A-stepping */ > #define EDP_LINK_TRAIN_400MV_0DB_SNB_A (0x38<<22) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index aa80ecb..8ab0fa5 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2687,7 +2687,6 @@ static void ironlake_fdi_pll_enable(struct intel_crtc *intel_crtc) > int pipe = intel_crtc->pipe; > u32 reg, temp; > > - > /* enable PCH FDI RX PLL, wait warmup plus DMI latency */ > reg = FDI_RX_CTL(pipe); > temp = I915_READ(reg); > @@ -3060,6 +3059,14 @@ static void ironlake_pch_enable(struct drm_crtc *crtc) > (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT) || > intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) { > u32 bpc = (I915_READ(PIPECONF(pipe)) & PIPE_BPC_MASK) >> 5; > + > + /* Workaround: Set the timing override bit before enabling the > + * DP pch transcoder. */ > + reg = TRANS_CHICKEN2(pipe); > + temp = I915_READ(reg); > + temp |= TRANS_CHICKEN2_TIMING_OVERRIDE; > + I915_WRITE(reg, temp); > + > reg = TRANS_DP_CTL(pipe); > temp = I915_READ(reg); > temp &= ~(TRANS_DP_PORT_SEL_MASK | > @@ -3365,6 +3372,12 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) > BUG(); /* wtf */ > } > I915_WRITE(PCH_DPLL_SEL, temp); > + > + /* Workaround: Clear the timing override chicken bit again. */ > + reg = TRANS_CHICKEN2(pipe); > + temp = I915_READ(reg); > + temp &= ~TRANS_CHICKEN2_TIMING_OVERRIDE; > + I915_WRITE(reg, temp); > } > > /* disable PCH DPLL */ > -- > 1.7.11.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Oct 26, 2012 at 12:21:11PM -0200, Paulo Zanoni wrote: > Hi > > 2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>: > > We need to set the timing override chicken bit after fdi link training > > has completed and before we enable the dp transcoder. We also have to > > clear that bit again after disabling the pch dp transcoder. > > > > See "Graphics BSpec: vol4g North Display Engine Registers [IVB], > > Display Mode Set Sequence" and "Graphics BSpec: vol4h South Display > > Engine Registers [CPT, PPT], South Display Engine Transcoder and FDI > > Control, Transcoder Debug and DFT, TRANS_CHICKEN_2" bit 31: > > > > "Workaround : Enable the override prior to enabling the transcoder. > > Disable the override after disabling the transcoder." > > > > While at it, use the _PIPE macro for the other TRANS_DP register. > > What confuses me is that the bit name is actually "Autotrain TimingGen > Override" and after a quick check it seems we don't use autotrain. > Also, by looking at the IVB mode set sequence it seems Auto Train is > the *recommended* method since manual is listed as "testing only". So > I'm not sure if we need this code now, but it seems we could try to > implement the auto train... Well, us using manual fdi train is a different matter altogether. And since there's nothing in the docs indicating that we don't need this w/a if we don't do autotraining, I prefer to just stick it into the code. And Damien tried to do the auto training, but somehow it failed ... > Also, I'd move the code that sets/unsets the chicken bit to > intel_enable_transcoder/intel_disable_transcoder. According to my reading of the modeset sequence we need to do this after the fdi train is fully done, but before we enable the pch dp transcoder. So I think this is the right place, and I can't actually move it anyplace else. And since the pch dp transcoder is a cpt/ppt-only feature, it' fits rather naturally. Cheers, Daniel
2012/10/29 Daniel Vetter <daniel@ffwll.ch>: > On Fri, Oct 26, 2012 at 12:21:11PM -0200, Paulo Zanoni wrote: >> Hi >> >> 2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>: >> > We need to set the timing override chicken bit after fdi link training >> > has completed and before we enable the dp transcoder. We also have to >> > clear that bit again after disabling the pch dp transcoder. >> > >> > See "Graphics BSpec: vol4g North Display Engine Registers [IVB], >> > Display Mode Set Sequence" and "Graphics BSpec: vol4h South Display >> > Engine Registers [CPT, PPT], South Display Engine Transcoder and FDI >> > Control, Transcoder Debug and DFT, TRANS_CHICKEN_2" bit 31: >> > >> > "Workaround : Enable the override prior to enabling the transcoder. >> > Disable the override after disabling the transcoder." >> > >> > While at it, use the _PIPE macro for the other TRANS_DP register. >> >> What confuses me is that the bit name is actually "Autotrain TimingGen >> Override" and after a quick check it seems we don't use autotrain. >> Also, by looking at the IVB mode set sequence it seems Auto Train is >> the *recommended* method since manual is listed as "testing only". So >> I'm not sure if we need this code now, but it seems we could try to >> implement the auto train... > > Well, us using manual fdi train is a different matter altogether. And > since there's nothing in the docs indicating that we don't need this w/a > if we don't do autotraining, I prefer to just stick it into the code. > > And Damien tried to do the auto training, but somehow it failed ... > >> Also, I'd move the code that sets/unsets the chicken bit to >> intel_enable_transcoder/intel_disable_transcoder. > > According to my reading of the modeset sequence we need to do this after > the fdi train is fully done, but before we enable the pch dp transcoder. From what I could understand your patch does the following: Enable: - Step 9.g (configure PCH timings) - Apply workaround - Step 9.h (configure TRANS_DP_CTL) - Step 9.i (enable PCH transcoder) Disable: - Step 10.e (wait PCH transcoder off) - Step 10.f (disable TRANS_DP_CTL) - Apply workaround - Step 10.g (disable transcoder DPLL) My understand is that we should be doing: Enable: - Step 9.g (configure PCH timings) - Step 9.h (configure TRANS_DP_CTL) - Apply workaround - Step 9.i (enable PCH transcoder) Disable: - Step 10.e (wait PCH transcoder off) - Apply workaround - Step 10.f (disable TRANS_DP_CTL) - Step 10.g (disale transcoder DPLL) The description of the chicken bit used in the workaround even says that: "Workaround: Enable the override prior to enabling the transcoder. Disable the override after disabling the transcoder". That's why I said we should implement the workaround in intel_enable_transcoder and intel_disable_transcoder: it matches the register description and also matches what I understood from our mode set sequence. > So I think this is the right place, and I can't actually move it anyplace > else. And since the pch dp transcoder is a cpt/ppt-only feature, it' fits > rather naturally. > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Oct 29, 2012 at 6:02 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > The description of the chicken bit used in the workaround even says that: > "Workaround: Enable the override prior to enabling the transcoder. > Disable the override after disabling the transcoder". > > That's why I said we should implement the workaround in > intel_enable_transcoder and intel_disable_transcoder: it matches the > register description and also matches what I understood from our mode > set sequence. I think that's what my code does, we set it before/after we enable the w/a. The difference between my code and your suggestion is that you either have to enable this w/a unconditionally, whereas I think we should only do this if we will use the dp transcoder on the pch, not in general. I agree that if we decide that this should be done in general, it's better done together with the other pch enable/disable stuff. -Daniel
Hi 2012/10/29 Daniel Vetter <daniel@ffwll.ch>: > On Mon, Oct 29, 2012 at 6:02 PM, Paulo Zanoni <przanoni@gmail.com> wrote: >> The description of the chicken bit used in the workaround even says that: >> "Workaround: Enable the override prior to enabling the transcoder. >> Disable the override after disabling the transcoder". >> >> That's why I said we should implement the workaround in >> intel_enable_transcoder and intel_disable_transcoder: it matches the >> register description and also matches what I understood from our mode >> set sequence. > > I think that's what my code does, we set it before/after we enable the > w/a. The difference between my code and your suggestion is that you > either have to enable this w/a unconditionally, whereas I think we > should only do this if we will use the dp transcoder on the pch, not > in general. > > I agree that if we decide that this should be done in general, it's > better done together with the other pch enable/disable stuff. LPT does not have DP and it still requires the workaround. Check HSW doc "Sequence for CRT port", step 26 on enable path (inside "enable transcoder") and step 11 on disable path (inside "disable transcoder"). > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index c7c4b96..84b09ee 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4062,7 +4062,7 @@ #define TRANS_DP_CTL_A 0xe0300 #define TRANS_DP_CTL_B 0xe1300 #define TRANS_DP_CTL_C 0xe2300 -#define TRANS_DP_CTL(pipe) (TRANS_DP_CTL_A + (pipe) * 0x01000) +#define TRANS_DP_CTL(pipe) _PIPE(pipe, TRANS_DP_CTL_A, TRANS_DP_CTL_B) #define TRANS_DP_OUTPUT_ENABLE (1<<31) #define TRANS_DP_PORT_SEL_B (0<<29) #define TRANS_DP_PORT_SEL_C (1<<29) @@ -4082,6 +4082,12 @@ #define TRANS_DP_HSYNC_ACTIVE_LOW 0 #define TRANS_DP_SYNC_MASK (3<<3) +#define TRANS_CHICKEN_2_A 0xf0064 +#define TRANS_CHICKEN_2_B 0xf1064 +#define TRANS_CHICKEN_2_C 0xf2064 +#define TRANS_CHICKEN_2(pipe) _PIPE(pipe, TRANS_CHICKEN_2_A, TRANS_CHICKEN_2_B) +#define TRANS_CHICKEN2_TIMING_OVERRIDE (1<<31) + /* SNB eDP training params */ /* SNB A-stepping */ #define EDP_LINK_TRAIN_400MV_0DB_SNB_A (0x38<<22) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index aa80ecb..8ab0fa5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2687,7 +2687,6 @@ static void ironlake_fdi_pll_enable(struct intel_crtc *intel_crtc) int pipe = intel_crtc->pipe; u32 reg, temp; - /* enable PCH FDI RX PLL, wait warmup plus DMI latency */ reg = FDI_RX_CTL(pipe); temp = I915_READ(reg); @@ -3060,6 +3059,14 @@ static void ironlake_pch_enable(struct drm_crtc *crtc) (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT) || intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) { u32 bpc = (I915_READ(PIPECONF(pipe)) & PIPE_BPC_MASK) >> 5; + + /* Workaround: Set the timing override bit before enabling the + * DP pch transcoder. */ + reg = TRANS_CHICKEN2(pipe); + temp = I915_READ(reg); + temp |= TRANS_CHICKEN2_TIMING_OVERRIDE; + I915_WRITE(reg, temp); + reg = TRANS_DP_CTL(pipe); temp = I915_READ(reg); temp &= ~(TRANS_DP_PORT_SEL_MASK | @@ -3365,6 +3372,12 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) BUG(); /* wtf */ } I915_WRITE(PCH_DPLL_SEL, temp); + + /* Workaround: Clear the timing override chicken bit again. */ + reg = TRANS_CHICKEN2(pipe); + temp = I915_READ(reg); + temp &= ~TRANS_CHICKEN2_TIMING_OVERRIDE; + I915_WRITE(reg, temp); } /* disable PCH DPLL */
We need to set the timing override chicken bit after fdi link training has completed and before we enable the dp transcoder. We also have to clear that bit again after disabling the pch dp transcoder. See "Graphics BSpec: vol4g North Display Engine Registers [IVB], Display Mode Set Sequence" and "Graphics BSpec: vol4h South Display Engine Registers [CPT, PPT], South Display Engine Transcoder and FDI Control, Transcoder Debug and DFT, TRANS_CHICKEN_2" bit 31: "Workaround : Enable the override prior to enabling the transcoder. Disable the override after disabling the transcoder." While at it, use the _PIPE macro for the other TRANS_DP register. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_reg.h | 8 +++++++- drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-)