| Message ID | 1411664633-7364-1-git-send-email-clinton.a.taylor@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + > 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
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? -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 >
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 > >
>> > 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. You are correct. It's transcoder based. It only gets used in HDMI/DVI modes, so EDP doesn't get one. Broadwell was able to properly rename transcoder stuff, so these became TRANS_MULT.
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) PIPE_MULT (w/o the 'I') is what the spec called it. Best to follow the same naming conventiom to make it easier to search for things in the spec. I would also name the _MULTIPLY_x defines the same way. > > /* 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); > + } > + > 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; > + } else pixel_multiplier = 1; With those fixed/changed this look good. > > return true; > } > -- > 1.7.9.5
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); + } + 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; }