diff mbox series

drm/i915: Use the correct PCH transcoder for LPT/WPT in intel_sanitize_frame_start_delay()

Message ID 20191128182358.14477-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Use the correct PCH transcoder for LPT/WPT in intel_sanitize_frame_start_delay() | expand

Commit Message

Ville Syrjälä Nov. 28, 2019, 6:23 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@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.

Cc: Uma Shankar <uma.shankar@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Shankar, Uma Nov. 29, 2019, 11:47 a.m. UTC | #1 
>-----Original Message-----
>From: Ville Syrjala <ville.syrjala@linux.intel.com>
>Sent: Thursday, November 28, 2019 11:54 PM
>To: intel-gfx@lists.freedesktop.org
>Cc: Shankar, Uma <uma.shankar@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@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@intel.com>

>Cc: Uma Shankar <uma.shankar@intel.com>
>Signed-off-by: Ville Syrjälä <ville.syrjala@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);
>+		i915_reg_t reg = TRANS_CHICKEN2(pch_transcoder);
> 		u32 val;
>
> 		val = I915_READ(reg);
>--
>2.23.0
Shankar, Uma Nov. 29, 2019, 11:56 a.m. UTC | #2 
>-----Original Message-----
>From: Shankar, Uma
>Sent: Friday, November 29, 2019 5:18 PM
>To: Ville Syrjala <ville.syrjala@linux.intel.com>; intel-gfx@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@linux.intel.com>
>>Sent: Thursday, November 28, 2019 11:54 PM
>>To: intel-gfx@lists.freedesktop.org
>>Cc: Shankar, Uma <uma.shankar@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@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@intel.com>
>
>>Cc: Uma Shankar <uma.shankar@intel.com>
>>Signed-off-by: Ville Syrjälä <ville.syrjala@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.

>>+		i915_reg_t reg = TRANS_CHICKEN2(pch_transcoder);
>> 		u32 val;
>>
>> 		val = I915_READ(reg);
>>--
>>2.23.0
Ville Syrjälä Nov. 29, 2019, 1:50 p.m. UTC | #3 
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@linux.intel.com>; intel-gfx@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@linux.intel.com>
> >>Sent: Thursday, November 28, 2019 11:54 PM
> >>To: intel-gfx@lists.freedesktop.org
> >>Cc: Shankar, Uma <uma.shankar@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@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@intel.com>
> >
> >>Cc: Uma Shankar <uma.shankar@intel.com>
> >>Signed-off-by: Ville Syrjälä <ville.syrjala@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.

> 
> >>+		i915_reg_t reg = TRANS_CHICKEN2(pch_transcoder);
> >> 		u32 val;
> >>
> >> 		val = I915_READ(reg);
> >>--
> >>2.23.0
>
Shankar, Uma Nov. 29, 2019, 5:52 p.m. UTC | #4 
>-----Original Message-----
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>Sent: Friday, November 29, 2019 7:21 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: 'intel-gfx@lists.freedesktop.org' <intel-gfx@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@linux.intel.com>;
>> >intel-gfx@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@linux.intel.com>
>> >>Sent: Thursday, November 28, 2019 11:54 PM
>> >>To: intel-gfx@lists.freedesktop.org
>> >>Cc: Shankar, Uma <uma.shankar@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@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@intel.com>
>> >
>> >>Cc: Uma Shankar <uma.shankar@intel.com>
>> >>Signed-off-by: Ville Syrjälä <ville.syrjala@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
diff mbox series

Patch

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);
+		i915_reg_t reg = TRANS_CHICKEN2(pch_transcoder);
 		u32 val;
 
 		val = I915_READ(reg);