[Intel-gfx] [PATCH] drm/i915: Enable pixel replicated modes on BDW and HSW.

[Daniel Vetter]

On Thu, Sep 25, 2014 at 07:11:11AM +0300, Ville Syrjälä wrote:
> On Wed, Sep 24, 2014 at 09:42:18PM +0200, Daniel Vetter wrote:
> > On Wed, Sep 24, 2014 at 08:44:42AM -0700, Clint Taylor wrote:
> > > On 09/24/2014 01:51 AM, Daniel Vetter wrote:
> > > >On Tue, Sep 23, 2014 at 11:06:56AM -0700, clinton.a.taylor at intel.com wrote:
> > > >>From: Clint Taylor <clinton.a.taylor at intel.com>
> > > >>
> > > >>Haswell and later silicon has added a new pixel replication register
> > > >>to the pipe timings for each transcoder. Now in addition to the
> > > >>DPLL_A_MD register for the pixel clock double, we also need to write to
> > > >>the TRANS_MULT_n (0x6002c) register to double the pixel data. Writing
> > > >>to the DPLL only double the pixel clock.
> > > >>
> > > >>Signed-off-by: Clint Taylor <clinton.a.taylor at intel.com>
> > > >>---
> > > >>  drivers/gpu/drm/i915/i915_reg.h      |    3 +++
> > > >>  drivers/gpu/drm/i915/intel_display.c |    6 +++++-
> > > >>  2 files changed, 8 insertions(+), 1 deletion(-)
> > > >>
> > > >>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > >>index 15c0eaa..7c078d9 100644
> > > >>--- a/drivers/gpu/drm/i915/i915_reg.h
> > > >>+++ b/drivers/gpu/drm/i915/i915_reg.h
> > > >>@@ -2431,6 +2431,7 @@ enum punit_power_well {
> > > >>  #define _PIPEASRC	0x6001c
> > > >>  #define _BCLRPAT_A	0x60020
> > > >>  #define _VSYNCSHIFT_A	0x60028
> > > >>+#define _MULTIPLY_A	0x6002c
> > > >>
> > > >>  /* Pipe B timing regs */
> > > >>  #define _HTOTAL_B	0x61000
> > > >>@@ -2442,6 +2443,7 @@ enum punit_power_well {
> > > >>  #define _PIPEBSRC	0x6101c
> > > >>  #define _BCLRPAT_B	0x61020
> > > >>  #define _VSYNCSHIFT_B	0x61028
> > > >>+#define _MULTIPLY_B	0x6102c
> > > >>
> > > >>  #define TRANSCODER_A_OFFSET 0x60000
> > > >>  #define TRANSCODER_B_OFFSET 0x61000
> > > >>@@ -2462,6 +2464,7 @@ enum punit_power_well {
> > > >>  #define BCLRPAT(trans) _TRANSCODER2(trans, _BCLRPAT_A)
> > > >>  #define VSYNCSHIFT(trans) _TRANSCODER2(trans, _VSYNCSHIFT_A)
> > > >>  #define PIPESRC(trans) _TRANSCODER2(trans, _PIPEASRC)
> > > >>+#define MULTIPLY(trans) _TRANSCODER2(trans, _MULTIPLY_A)
> > > >
> > > >MULTIPLY is a bit generic and doesn't even match Bspec lingo. I'd just go
> > > >with PIPE_MULTI instead to match Bspec and give it a nice PIPE_ prefix.
> > > >>
> > > >>  /* HSW+ eDP PSR registers */
> > > >>  #define EDP_PSR_BASE(dev)                       (IS_HASWELL(dev) ? 0x64800 : 0x6f800)
> > > >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > >>index c092ff4..e58fcde 100644
> > > >>--- a/drivers/gpu/drm/i915/intel_display.c
> > > >>+++ b/drivers/gpu/drm/i915/intel_display.c
> > > >>@@ -4152,6 +4152,9 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
> > > >>
> > > >>  	intel_set_pipe_timings(intel_crtc);
> > > >>
> > > >>+	I915_WRITE(MULTIPLY(intel_crtc->config.cpu_transcoder),
> > > >
> > > >This register is per-pipe, so needs to be indexed with intel_crtc->pipe.
> > > >Same below.
> > > >
> > > The MULTIPLY Macro calls the _TRANSCODER2 MACRO which already indexes the
> > > register based on intel_crtc->pipe. This should be all that's required.
> > 
> > I don't see where it indexes with intel_crtc->pipe ...
> > 
> > But it doesn't matter since the register is clearly in the transcoder
> > block, and the reason why Bspec says is per-pipe is that the edp
> > transcoder doesn't have it. So on second consideration I guess we can keep
> > this part as-is then.
> 
> ? If it doesn't exist for EDP we can't just go passing cpu_transcoder
> to it.
> 
> BDW BSpec seems to claim that it really is a transcoder register and not
> a pipe register (just looking at the offset isn't enoguh to tell that
> as PIPESRC shows). So in that sense using cpu_transcoder is more
> appropriate, but if we do that we must not write the register when
> cpu_transcoder == EDP. I suppose that even makes sense since it's only
> valid for HDMI/DVI and that's not supported on the EDP transcoder.
> But someone really must verify that it really is a transcoder and not a
> pipe register and that it has no effect on transcoder EDP.

I guess we could use the cpu_transcoder and add a WARN_ON(cpu_transcoder
== EDP). Makes stuff consistent and if we ever botch this up we'll know.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch