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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@intel.com wrote:
> > >>From: Clint Taylor <clinton.a.taylor@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@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.

-- 
Ville Syrjälä
Intel OTC