diff mbox series

[04/10] drm/i915: Always read out M2_N2 in intel_cpu_transcoder_get_m_n

Message ID 20181011100457.8776-5-maarten.lankhorst@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Remove low hanging crtc->config fruit, part 2. | expand

Commit Message

Maarten Lankhorst Oct. 11, 2018, 10:04 a.m. UTC 
has_drrs is a flag we can't read out. We set it when seamless DRRS is
enabled in pipe_config, so intel_dump_pipe_config() and
intel_pipe_config_compare() will continue to do the right thing when
has_drrs is set on the real state.

This removes one more dereference of crtc->config.
While at it, fixup the comment and also read out M2_N2 for CHV, since
we program it in the set_m_n function.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Comments

Ville Syrjälä Oct. 11, 2018, 11:56 a.m. UTC | #1 
On Thu, Oct 11, 2018 at 12:04:51PM +0200, Maarten Lankhorst wrote:
> has_drrs is a flag we can't read out. We set it when seamless DRRS is
> enabled in pipe_config, so intel_dump_pipe_config() and
> intel_pipe_config_compare() will continue to do the right thing when
> has_drrs is set on the real state.
> 
> This removes one more dereference of crtc->config.
> While at it, fixup the comment and also read out M2_N2 for CHV, since
> we program it in the set_m_n function.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bb16a9acf117..7812fab31646 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6893,8 +6893,8 @@ static void intel_cpu_transcoder_set_m_n(const struct intel_crtc_state *crtc_sta
>  		I915_WRITE(PIPE_LINK_M1(transcoder), m_n->link_m);
>  		I915_WRITE(PIPE_LINK_N1(transcoder), m_n->link_n);
>  		/* M2_N2 registers to be set only for gen < 8 (M2_N2 available
> -		 * for gen < 8) and if DRRS is supported (to make sure the
> -		 * registers are not unnecessarily accessed).
> +		 * for gen < 8 and CHV) and if DRRS is supported (to make sure
> +		 * the registers are not unnecessarily accessed).
>  		 */
>  		if (m2_n2 && (IS_CHERRYVIEW(dev_priv) ||
>  		    INTEL_GEN(dev_priv) < 8) && crtc_state->has_drrs) {

I think what I'd really like to see is splitting the m/n and m2/n2 stuff
into two distinct pieces, and when we don't use drrs we could just
set m2_n2 = m_n, and program the everything exactly the same way as
when drrs is enabled.

Also we could nuke that "m/n vs. m2/n2" enum and just pass the correct
struct to foo_set_m_n() directly.

> @@ -8747,11 +8747,10 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc,
>  		m_n->tu = ((I915_READ(PIPE_DATA_M1(transcoder))
>  			    & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1;
>  		/* Read M2_N2 registers only for gen < 8 (M2_N2 available for
> -		 * gen < 8) and if DRRS is supported (to make sure the
> -		 * registers are not unnecessarily read).
> +		 * gen < 8 and CHV).
>  		 */
> -		if (m2_n2 && INTEL_GEN(dev_priv) < 8 &&
> -			crtc->config->has_drrs) {
> +		if (m2_n2 && (INTEL_GEN(dev_priv) < 8 ||
> +			      IS_CHERRYVIEW(dev_priv))) {

I think this is maybe the third installment of this check. Could
perhaps use a small has_m2_n2() helper to avoid the repetition?

Either way
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  			m2_n2->link_m = I915_READ(PIPE_LINK_M2(transcoder));
>  			m2_n2->link_n =	I915_READ(PIPE_LINK_N2(transcoder));
>  			m2_n2->gmch_m =	I915_READ(PIPE_DATA_M2(transcoder))
> -- 
> 2.19.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Maarten Lankhorst Oct. 11, 2018, 6:17 p.m. UTC | #2 
Op 11-10-18 om 13:56 schreef Ville Syrjälä:
> On Thu, Oct 11, 2018 at 12:04:51PM +0200, Maarten Lankhorst wrote:
>> has_drrs is a flag we can't read out. We set it when seamless DRRS is
>> enabled in pipe_config, so intel_dump_pipe_config() and
>> intel_pipe_config_compare() will continue to do the right thing when
>> has_drrs is set on the real state.
>>
>> This removes one more dereference of crtc->config.
>> While at it, fixup the comment and also read out M2_N2 for CHV, since
>> we program it in the set_m_n function.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 11 +++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index bb16a9acf117..7812fab31646 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6893,8 +6893,8 @@ static void intel_cpu_transcoder_set_m_n(const struct intel_crtc_state *crtc_sta
>>  		I915_WRITE(PIPE_LINK_M1(transcoder), m_n->link_m);
>>  		I915_WRITE(PIPE_LINK_N1(transcoder), m_n->link_n);
>>  		/* M2_N2 registers to be set only for gen < 8 (M2_N2 available
>> -		 * for gen < 8) and if DRRS is supported (to make sure the
>> -		 * registers are not unnecessarily accessed).
>> +		 * for gen < 8 and CHV) and if DRRS is supported (to make sure
>> +		 * the registers are not unnecessarily accessed).
>>  		 */
>>  		if (m2_n2 && (IS_CHERRYVIEW(dev_priv) ||
>>  		    INTEL_GEN(dev_priv) < 8) && crtc_state->has_drrs) {
> I think what I'd really like to see is splitting the m/n and m2/n2 stuff
> into two distinct pieces, and when we don't use drrs we could just
> set m2_n2 = m_n, and program the everything exactly the same way as
> when drrs is enabled.
>
> Also we could nuke that "m/n vs. m2/n2" enum and just pass the correct
> struct to foo_set_m_n() directly.
>
>> @@ -8747,11 +8747,10 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc,
>>  		m_n->tu = ((I915_READ(PIPE_DATA_M1(transcoder))
>>  			    & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1;
>>  		/* Read M2_N2 registers only for gen < 8 (M2_N2 available for
>> -		 * gen < 8) and if DRRS is supported (to make sure the
>> -		 * registers are not unnecessarily read).
>> +		 * gen < 8 and CHV).
>>  		 */
>> -		if (m2_n2 && INTEL_GEN(dev_priv) < 8 &&
>> -			crtc->config->has_drrs) {
>> +		if (m2_n2 && (INTEL_GEN(dev_priv) < 8 ||
>> +			      IS_CHERRYVIEW(dev_priv))) {
> I think this is maybe the third installment of this check. Could
> perhaps use a small has_m2_n2() helper to avoid the repetition?
>
> Either way
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Hm looks like this is causing a unclaimed register read in debugfs. Could we limit reads/writes to eDP trasncoder?
>>  			m2_n2->link_m = I915_READ(PIPE_LINK_M2(transcoder));
>>  			m2_n2->link_n =	I915_READ(PIPE_LINK_N2(transcoder));
>>  			m2_n2->gmch_m =	I915_READ(PIPE_DATA_M2(transcoder))
>> -- 
>> 2.19.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Oct. 11, 2018, 7:09 p.m. UTC | #3 
On Thu, Oct 11, 2018 at 08:17:49PM +0200, Maarten Lankhorst wrote:
> Op 11-10-18 om 13:56 schreef Ville Syrjälä:
> > On Thu, Oct 11, 2018 at 12:04:51PM +0200, Maarten Lankhorst wrote:
> >> has_drrs is a flag we can't read out. We set it when seamless DRRS is
> >> enabled in pipe_config, so intel_dump_pipe_config() and
> >> intel_pipe_config_compare() will continue to do the right thing when
> >> has_drrs is set on the real state.
> >>
> >> This removes one more dereference of crtc->config.
> >> While at it, fixup the comment and also read out M2_N2 for CHV, since
> >> we program it in the set_m_n function.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 11 +++++------
> >>  1 file changed, 5 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index bb16a9acf117..7812fab31646 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -6893,8 +6893,8 @@ static void intel_cpu_transcoder_set_m_n(const struct intel_crtc_state *crtc_sta
> >>  		I915_WRITE(PIPE_LINK_M1(transcoder), m_n->link_m);
> >>  		I915_WRITE(PIPE_LINK_N1(transcoder), m_n->link_n);
> >>  		/* M2_N2 registers to be set only for gen < 8 (M2_N2 available
> >> -		 * for gen < 8) and if DRRS is supported (to make sure the
> >> -		 * registers are not unnecessarily accessed).
> >> +		 * for gen < 8 and CHV) and if DRRS is supported (to make sure
> >> +		 * the registers are not unnecessarily accessed).
> >>  		 */
> >>  		if (m2_n2 && (IS_CHERRYVIEW(dev_priv) ||
> >>  		    INTEL_GEN(dev_priv) < 8) && crtc_state->has_drrs) {
> > I think what I'd really like to see is splitting the m/n and m2/n2 stuff
> > into two distinct pieces, and when we don't use drrs we could just
> > set m2_n2 = m_n, and program the everything exactly the same way as
> > when drrs is enabled.
> >
> > Also we could nuke that "m/n vs. m2/n2" enum and just pass the correct
> > struct to foo_set_m_n() directly.
> >
> >> @@ -8747,11 +8747,10 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc,
> >>  		m_n->tu = ((I915_READ(PIPE_DATA_M1(transcoder))
> >>  			    & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1;
> >>  		/* Read M2_N2 registers only for gen < 8 (M2_N2 available for
> >> -		 * gen < 8) and if DRRS is supported (to make sure the
> >> -		 * registers are not unnecessarily read).
> >> +		 * gen < 8 and CHV).
> >>  		 */
> >> -		if (m2_n2 && INTEL_GEN(dev_priv) < 8 &&
> >> -			crtc->config->has_drrs) {
> >> +		if (m2_n2 && (INTEL_GEN(dev_priv) < 8 ||
> >> +			      IS_CHERRYVIEW(dev_priv))) {
> > I think this is maybe the third installment of this check. Could
> > perhaps use a small has_m2_n2() helper to avoid the repetition?
> >
> > Either way
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Hm looks like this is causing a unclaimed register read in debugfs. Could we limit reads/writes to eDP trasncoder?

That's not a thing on most platforms.

transcoder_has_m2_n2()
{
	if (IS_HASWELL)
		return trans == EDP;
	return IS_ILK || IS_SNB || IS_IVB || IS_VLV || IS_CHV;
}

?

Though intel_dp_set_drrs_state() only claims to support ivb+, even
though ilk/snb should work with the same code as ivb. Hmm, oh right
the fdi bits are a bit busted on those platforms. So drrs won't work
at least on the pch ports with the current code. I guess it might
work on port A eDP.

> >>  			m2_n2->link_m = I915_READ(PIPE_LINK_M2(transcoder));
> >>  			m2_n2->link_n =	I915_READ(PIPE_LINK_N2(transcoder));
> >>  			m2_n2->gmch_m =	I915_READ(PIPE_DATA_M2(transcoder))
> >> -- 
> >> 2.19.0
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bb16a9acf117..7812fab31646 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6893,8 +6893,8 @@  static void intel_cpu_transcoder_set_m_n(const struct intel_crtc_state *crtc_sta
 		I915_WRITE(PIPE_LINK_M1(transcoder), m_n->link_m);
 		I915_WRITE(PIPE_LINK_N1(transcoder), m_n->link_n);
 		/* M2_N2 registers to be set only for gen < 8 (M2_N2 available
-		 * for gen < 8) and if DRRS is supported (to make sure the
-		 * registers are not unnecessarily accessed).
+		 * for gen < 8 and CHV) and if DRRS is supported (to make sure
+		 * the registers are not unnecessarily accessed).
 		 */
 		if (m2_n2 && (IS_CHERRYVIEW(dev_priv) ||
 		    INTEL_GEN(dev_priv) < 8) && crtc_state->has_drrs) {
@@ -8747,11 +8747,10 @@  static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc,
 		m_n->tu = ((I915_READ(PIPE_DATA_M1(transcoder))
 			    & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1;
 		/* Read M2_N2 registers only for gen < 8 (M2_N2 available for
-		 * gen < 8) and if DRRS is supported (to make sure the
-		 * registers are not unnecessarily read).
+		 * gen < 8 and CHV).
 		 */
-		if (m2_n2 && INTEL_GEN(dev_priv) < 8 &&
-			crtc->config->has_drrs) {
+		if (m2_n2 && (INTEL_GEN(dev_priv) < 8 ||
+			      IS_CHERRYVIEW(dev_priv))) {
 			m2_n2->link_m = I915_READ(PIPE_LINK_M2(transcoder));
 			m2_n2->link_n =	I915_READ(PIPE_LINK_N2(transcoder));
 			m2_n2->gmch_m =	I915_READ(PIPE_DATA_M2(transcoder))