[Intel-gfx] [PATCH] drm/i915: Use the correct PCH transcoder for LPT/WPT in intel_sanitize_frame_start_delay()

[Shankar, Uma]

>-----Original Message-----
>From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>Sent: Friday, November 29, 2019 7:21 PM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: 'intel-gfx at lists.freedesktop.org' <intel-gfx at lists.freedesktop.org>
>Subject: Re: [PATCH] drm/i915: Use the correct PCH transcoder for LPT/WPT in
>intel_sanitize_frame_start_delay()
>
>On Fri, Nov 29, 2019 at 11:56:42AM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Shankar, Uma
>> >Sent: Friday, November 29, 2019 5:18 PM
>> >To: Ville Syrjala <ville.syrjala at linux.intel.com>;
>> >intel-gfx at lists.freedesktop.org
>> >Subject: RE: [PATCH] drm/i915: Use the correct PCH transcoder for
>> >LPT/WPT in
>> >intel_sanitize_frame_start_delay()
>> >
>> >
>> >
>> >>-----Original Message-----
>> >>From: Ville Syrjala <ville.syrjala at linux.intel.com>
>> >>Sent: Thursday, November 28, 2019 11:54 PM
>> >>To: intel-gfx at lists.freedesktop.org
>> >>Cc: Shankar, Uma <uma.shankar at intel.com>
>> >>Subject: [PATCH] drm/i915: Use the correct PCH transcoder for
>> >>LPT/WPT in
>> >>intel_sanitize_frame_start_delay()
>> >>
>> >>From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> >>
>> >>LPT/WPT only have PCH transcoder A. Make sure we poke at its chicken
>> >>register instead of some non-existent register when FDI is being
>> >>driven by pipe B or
>> >C.
>> >
>> >Change looks good to me.
>> >Reviewed-by: Uma Shankar <uma.shankar at intel.com>
>> >
>> >>Cc: Uma Shankar <uma.shankar at intel.com>
>> >>Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> >>---
>> >> drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
>> >> 1 file changed, 2 insertions(+), 1 deletion(-)
>> >>
>> >>diff --git a/drivers/gpu/drm/i915/display/intel_display.c
>> >>b/drivers/gpu/drm/i915/display/intel_display.c
>> >>index 53dc310a5f6d..f99dbc3d9696 100644
>> >>--- a/drivers/gpu/drm/i915/display/intel_display.c
>> >>+++ b/drivers/gpu/drm/i915/display/intel_display.c
>> >>@@ -17272,7 +17272,8 @@ static void
>> >>intel_sanitize_frame_start_delay(const struct intel_crtc_state *crtc
>> >> 		val |= TRANS_FRAME_START_DELAY(0);
>> >> 		I915_WRITE(reg, val);
>> >> 	} else {
>> >>-		i915_reg_t reg = TRANS_CHICKEN2(crtc->pipe);
>> >>+		enum pipe pch_transcoder = intel_crtc_pch_transcoder(crtc);
>>
>> Just an afterthought, not sure if this hold generically for all pipes
>> or is it programmable only for PIPE_A. Making it enabled on PIPE_A
>> when actually pipe B or C is used, is it the right thing. Should we discard
>programming this for PIPE B and C altogether.
>
>This is about PCH transcoder A instead of pipe A despite us using enum pipe here. We
>just happen to use enum pipe for PCH transcoders since it was easier than adding a
>new enum for it.
>
>So it's perfectly legal to have any of these configurations on HSW/BDW:
>pipe A -> CPU transcoder A -> DDI E -> FDI -> PCH transcoder A -> CRT pipe B -> CPU
>transcoder B -> DDI E -> FDI -> PCH transcoder A -> CRT pipe C -> CPU transcoder C ->
>DDI E -> FDI -> PCH transcoder A -> CRT
>
>So we want to do this for any pipe when has_pch_encoder indicates that the data is
>going over FDI to the PCH.

Yeah I was thinking on same line that PIPE_A is just a means to get PCH TRANSCODER rather
than having any significance with pipe here. Thanks for clarying this Ville. 

This patch is good from my perspective.

Regards,
Uma Shankar

>>
>> >>+		i915_reg_t reg = TRANS_CHICKEN2(pch_transcoder);
>> >> 		u32 val;
>> >>
>> >> 		val = I915_READ(reg);
>> >>--
>> >>2.23.0
>>
>
>--
>Ville Syrjälä
>Intel