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

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

On Fri, Sep 26, 2014 at 03:04:22PM -0700, Clint Taylor wrote:
> On 09/26/2014 09:38 AM, Ville Syrjälä wrote:
> > On Thu, Sep 25, 2014 at 10:03:53AM -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.
> >>
> >> ver2: Macro name change from MULTIPLY to PIPE_MULTI. (Daniel)
> >> ver3: Do not set pixel multiplier if transcoder is eDP (Ville)
> >>
> >> Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Cc: Jani Nikula <jani.nikula@intel.com>
> >>
> >> 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 |   10 +++++++++-
> >>   2 files changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index ad8179b..035d58c 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -2443,6 +2443,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
> >> @@ -2454,6 +2455,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
> >> @@ -2474,6 +2476,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 PIPE_MULTI(trans) _TRANSCODER2(trans, _MULTIPLY_A)
> >>
> >>   /* 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 858011d..f8c1f11 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4168,6 +4168,11 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
> >>
> >>   	intel_set_pipe_timings(intel_crtc);
> >>
> >> +	if (intel_crtc->config.cpu_transcoder != TRANSCODER_EDP) {
> >> +		I915_WRITE(PIPE_MULTI(intel_crtc->config.cpu_transcoder),
> >> +				intel_crtc->config.pixel_multiplier - 1);
> >> +	}
> >
> > So did you verify that the register really is a transcoder register?
> > Eg. set PIPE_MULT(A) to >1x and use pipe A to drive the EDP transcoder.
> 
> I did not verify. This change was done based on the fact that the 
> register does not exist in the VPG HTML version of the BPEC for 
> Transcoder_EDP, only TRANS_MULT_A, _B, and _C are defined.
> 
> Do we have an SI contact that can confirm?

Cc:ing Art.

Art, the confusion here is whether PIPE_MULT is a transcoder register
or a pipe register. BSpec seems to be telling us that it's a transcoder
register but the confusion comes from the fact that the EDP transcoder
doesn't have this register. My theory is that it is a transcoder register,
but since pixel repeat isn't needed for eDP the register isn't present
(or relevant) in the EDP transcoder. Can you clarify this?

Although in this case it would be very easy to test this theory on
actual hardware as I previously suggested.

> 
> -Clint
> 
> 
> >
> >> +
> >>   	if (intel_crtc->config.has_pch_encoder) {
> >>   		intel_cpu_transcoder_set_m_n(intel_crtc,
> >>   				     &intel_crtc->config.fdi_m_n, NULL);
> >> @@ -7853,7 +7858,10 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> >>   		pipe_config->ips_enabled = hsw_crtc_supports_ips(crtc) &&
> >>   			(I915_READ(IPS_CTL) & IPS_ENABLE);
> >>
> >> -	pipe_config->pixel_multiplier = 1;
> >> +	if (pipe_config->cpu_transcoder != TRANSCODER_EDP) {
> >> +		pipe_config->pixel_multiplier =
> >> +			I915_READ(PIPE_MULTI(pipe_config->cpu_transcoder)) + 1;
> >> +	}
> >>
> >>   	return true;
> >>   }
> >> --
> >> 1.7.9.5
> >

-- 
Ville Syrjälä
Intel OTC