[PATCH v3] drm/i915/psr: Update PSR2 resolution check for Cannonlake

[PATCH v3] drm/i915/psr: Update PSR2 resolution check for Cannonlake

[Dhinakaran Pandiyan]

In fact, apply the Cannonlake resolution check for all >= Gen-10 platforms
to be safe.

v3: Update GLK too. (Ville)
    Longer variable names.
    if-else in place of ternary operator.
v2: Use local variables for resolution limits and print them (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Elio Martinez Monroy <elio.martinez.monroy@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 05770790a4e9..23175c5c4a50 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -451,8 +451,9 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 {
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
-	const struct drm_display_mode *adjusted_mode =
-		&crtc_state->base.adjusted_mode;
+	int crtc_hdisplay = crtc_state->base.adjusted_mode.crtc_hdisplay;
+	int crtc_vdisplay = crtc_state->base.adjusted_mode.crtc_vdisplay;
+	int psr_max_h = 0, psr_max_v = 0;
 
 	/*
 	 * FIXME psr2_support is messed up. It's both computed
@@ -462,10 +463,18 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 	if (!dev_priv->psr.psr2_support)
 		return false;
 
-	/* PSR2 is restricted to work with panel resolutions up to 3640x2304 */
-	if (adjusted_mode->crtc_hdisplay > 3640 ||
-	    adjusted_mode->crtc_vdisplay > 2304) {
-		DRM_DEBUG_KMS("PSR2 not enabled, panel resolution too big\n");
+	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
+		psr_max_h = 4096;
+		psr_max_v = 2304;
+	} else if (IS_GEN9(dev_priv)) {
+		psr_max_h = 3640;
+		psr_max_v = 2304;
+	}
+
+	if (crtc_hdisplay > psr_max_h || crtc_vdisplay > psr_max_v) {
+		DRM_DEBUG_KMS("PSR2 not enabled, resolution %dx%d > max supported %dx%d\n",
+			      crtc_hdisplay, crtc_vdisplay,
+			      psr_max_h, psr_max_v);
 		return false;
 	}
 
-- 
2.14.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/psr: Update PSR2 resolution check for Cannonlake

[Ville Syrjälä]

On Tue, Mar 06, 2018 at 12:33:55PM -0800, Dhinakaran Pandiyan wrote:
> In fact, apply the Cannonlake resolution check for all >= Gen-10 platforms
> to be safe.
> 
> v3: Update GLK too. (Ville)
>     Longer variable names.
>     if-else in place of ternary operator.
> v2: Use local variables for resolution limits and print them (Ville)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Elio Martinez Monroy <elio.martinez.monroy@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 05770790a4e9..23175c5c4a50 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -451,8 +451,9 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
>  {
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> -	const struct drm_display_mode *adjusted_mode =
> -		&crtc_state->base.adjusted_mode;
> +	int crtc_hdisplay = crtc_state->base.adjusted_mode.crtc_hdisplay;
> +	int crtc_vdisplay = crtc_state->base.adjusted_mode.crtc_vdisplay;
            ^^^^^
The crtc_ prefix is pretty much redundant.

> +	int psr_max_h = 0, psr_max_v = 0;

And this still reads as "max height" to my brain, but meh.

>  
>  	/*
>  	 * FIXME psr2_support is messed up. It's both computed
> @@ -462,10 +463,18 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
>  	if (!dev_priv->psr.psr2_support)
>  		return false;
>  
> -	/* PSR2 is restricted to work with panel resolutions up to 3640x2304 */
> -	if (adjusted_mode->crtc_hdisplay > 3640 ||
> -	    adjusted_mode->crtc_vdisplay > 2304) {
> -		DRM_DEBUG_KMS("PSR2 not enabled, panel resolution too big\n");
> +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
> +		psr_max_h = 4096;
> +		psr_max_v = 2304;
> +	} else if (IS_GEN9(dev_priv)) {
> +		psr_max_h = 3640;
> +		psr_max_v = 2304;
> +	}

pre-SKL?

> +
> +	if (crtc_hdisplay > psr_max_h || crtc_vdisplay > psr_max_v) {
> +		DRM_DEBUG_KMS("PSR2 not enabled, resolution %dx%d > max supported %dx%d\n",
> +			      crtc_hdisplay, crtc_vdisplay,
> +			      psr_max_h, psr_max_v);
>  		return false;
>  	}
>  
> -- 
> 2.14.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/psr: Update PSR2 resolution check for Cannonlake

[Pandiyan, Dhinakaran]

On Tue, 2018-03-06 at 22:38 +0200, Ville Syrjälä wrote:
> On Tue, Mar 06, 2018 at 12:33:55PM -0800, Dhinakaran Pandiyan wrote:
> > In fact, apply the Cannonlake resolution check for all >= Gen-10 platforms
> > to be safe.
> > 
> > v3: Update GLK too. (Ville)
> >     Longer variable names.
> >     if-else in place of ternary operator.
> > v2: Use local variables for resolution limits and print them (Ville)
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Elio Martinez Monroy <elio.martinez.monroy@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 21 +++++++++++++++------
> >  1 file changed, 15 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 05770790a4e9..23175c5c4a50 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -451,8 +451,9 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> >  {
> >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > -	const struct drm_display_mode *adjusted_mode =
> > -		&crtc_state->base.adjusted_mode;
> > +	int crtc_hdisplay = crtc_state->base.adjusted_mode.crtc_hdisplay;
> > +	int crtc_vdisplay = crtc_state->base.adjusted_mode.crtc_vdisplay;
>             ^^^^^
> The crtc_ prefix is pretty much redundant.

display_mode has members named vdisplay and hdisplay and this avoids any
potential confusion.


> 
> > +	int psr_max_h = 0, psr_max_v = 0;
> 
> And this still reads as "max height" to my brain, but meh.

And here I thought this version leaves no room for confusion :) I should
just ask someone else to write this patch.

> 
> >  
> >  	/*
> >  	 * FIXME psr2_support is messed up. It's both computed
> > @@ -462,10 +463,18 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> >  	if (!dev_priv->psr.psr2_support)
> >  		return false;
> >  
> > -	/* PSR2 is restricted to work with panel resolutions up to 3640x2304 */
> > -	if (adjusted_mode->crtc_hdisplay > 3640 ||
> > -	    adjusted_mode->crtc_vdisplay > 2304) {
> > -		DRM_DEBUG_KMS("PSR2 not enabled, panel resolution too big\n");
> > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
> > +		psr_max_h = 4096;
> > +		psr_max_v = 2304;
> > +	} else if (IS_GEN9(dev_priv)) {
> > +		psr_max_h = 3640;
> > +		psr_max_v = 2304;
> > +	}
> 
> pre-SKL?

No PSR2 on pre-skl

If we do somehow end up here, returning false and printing a debug
message will be useful.

> 
> > +
> > +	if (crtc_hdisplay > psr_max_h || crtc_vdisplay > psr_max_v) {
> > +		DRM_DEBUG_KMS("PSR2 not enabled, resolution %dx%d > max supported %dx%d\n",
> > +			      crtc_hdisplay, crtc_vdisplay,
> > +			      psr_max_h, psr_max_v);
> >  		return false;
> >  	}
> >  
> > -- 
> > 2.14.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/psr: Update PSR2 resolution check for Cannonlake (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/psr: Update PSR2 resolution check for Cannonlake (rev3)
URL   : https://patchwork.freedesktop.org/series/39238/
State : success

== Summary ==

Series 39238v3 drm/i915/psr: Update PSR2 resolution check for Cannonlake
https://patchwork.freedesktop.org/api/1.0/series/39238/revisions/3/mbox/

---- Known issues:

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:422s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:423s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:369s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:508s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:277s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:490s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:492s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:482s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:474s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:405s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:575s
fi-cfl-u         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:504s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:413s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:289s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:518s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:398s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:407s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:463s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:422s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:466s
fi-kbl-7560u     total:108  pass:104  dwarn:0   dfail:0   fail:0   skip:3  
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:459s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:504s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:587s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:431s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:516s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:533s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:499s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:483s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:423s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:428s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:520s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:388s
fi-cnl-y3 failed to collect. IGT log at Patchwork_8250/fi-cnl-y3/run0.log

72d7aac2959d8b0dcdbe6d5e7f072a07fbe2d377 drm-tip: 2018y-03m-06d-19h-09m-24s UTC integration manifest
1740e0ee7678 drm/i915/psr: Update PSR2 resolution check for Cannonlake

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8250/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/psr: Update PSR2 resolution check for Cannonlake

[Ville Syrjälä]

On Tue, Mar 06, 2018 at 08:45:44PM +0000, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Tue, 2018-03-06 at 22:38 +0200, Ville Syrjälä wrote:
> > On Tue, Mar 06, 2018 at 12:33:55PM -0800, Dhinakaran Pandiyan wrote:
> > > In fact, apply the Cannonlake resolution check for all >= Gen-10 platforms
> > > to be safe.
> > > 
> > > v3: Update GLK too. (Ville)
> > >     Longer variable names.
> > >     if-else in place of ternary operator.
> > > v2: Use local variables for resolution limits and print them (Ville)
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Elio Martinez Monroy <elio.martinez.monroy@intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 21 +++++++++++++++------
> > >  1 file changed, 15 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > index 05770790a4e9..23175c5c4a50 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -451,8 +451,9 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> > >  {
> > >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > >  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > > -	const struct drm_display_mode *adjusted_mode =
> > > -		&crtc_state->base.adjusted_mode;
> > > +	int crtc_hdisplay = crtc_state->base.adjusted_mode.crtc_hdisplay;
> > > +	int crtc_vdisplay = crtc_state->base.adjusted_mode.crtc_vdisplay;
> >             ^^^^^
> > The crtc_ prefix is pretty much redundant.
> 
> display_mode has members named vdisplay and hdisplay and this avoids any
> potential confusion.
> 
> 
> > 
> > > +	int psr_max_h = 0, psr_max_v = 0;
> > 
> > And this still reads as "max height" to my brain, but meh.
> 
> And here I thought this version leaves no room for confusion :) I should
> just ask someone else to write this patch.
> 
> > 
> > >  
> > >  	/*
> > >  	 * FIXME psr2_support is messed up. It's both computed
> > > @@ -462,10 +463,18 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> > >  	if (!dev_priv->psr.psr2_support)
> > >  		return false;
> > >  
> > > -	/* PSR2 is restricted to work with panel resolutions up to 3640x2304 */
> > > -	if (adjusted_mode->crtc_hdisplay > 3640 ||
> > > -	    adjusted_mode->crtc_vdisplay > 2304) {
> > > -		DRM_DEBUG_KMS("PSR2 not enabled, panel resolution too big\n");
> > > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
> > > +		psr_max_h = 4096;
> > > +		psr_max_v = 2304;
> > > +	} else if (IS_GEN9(dev_priv)) {
> > > +		psr_max_h = 3640;
> > > +		psr_max_v = 2304;
> > > +	}
> > 
> > pre-SKL?
> 
> No PSR2 on pre-skl

OK. I'd drop the IS_GEN9 then. Would be less confusing for my brain at
least.

> 
> If we do somehow end up here, returning false and printing a debug
> message will be useful.

Seems a bit overly protective. The has_psr2 check is just above. IMO
adding basically dead code "just in case" is not helpful in making the
code easy to read.

Since you say pre-skl is not a problem here:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> > 
> > > +
> > > +	if (crtc_hdisplay > psr_max_h || crtc_vdisplay > psr_max_v) {
> > > +		DRM_DEBUG_KMS("PSR2 not enabled, resolution %dx%d > max supported %dx%d\n",
> > > +			      crtc_hdisplay, crtc_vdisplay,
> > > +			      psr_max_h, psr_max_v);
> > >  		return false;
> > >  	}
> > >  
> > > -- 
> > > 2.14.1
> > 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/psr: Update PSR2 resolution check for Cannonlake

[Pandiyan, Dhinakaran]

On Tue, 2018-03-06 at 23:24 +0200, Ville Syrjälä wrote:
> On Tue, Mar 06, 2018 at 08:45:44PM +0000, Pandiyan, Dhinakaran wrote:
> > 
> > 
> > 
> > On Tue, 2018-03-06 at 22:38 +0200, Ville Syrjälä wrote:
> > > On Tue, Mar 06, 2018 at 12:33:55PM -0800, Dhinakaran Pandiyan wrote:
> > > > In fact, apply the Cannonlake resolution check for all >= Gen-10 platforms
> > > > to be safe.
> > > > 
> > > > v3: Update GLK too. (Ville)
> > > >     Longer variable names.
> > > >     if-else in place of ternary operator.
> > > > v2: Use local variables for resolution limits and print them (Ville)
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Elio Martinez Monroy <elio.martinez.monroy@intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_psr.c | 21 +++++++++++++++------
> > > >  1 file changed, 15 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 05770790a4e9..23175c5c4a50 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -451,8 +451,9 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> > > >  {
> > > >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > >  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > > > -	const struct drm_display_mode *adjusted_mode =
> > > > -		&crtc_state->base.adjusted_mode;
> > > > +	int crtc_hdisplay = crtc_state->base.adjusted_mode.crtc_hdisplay;
> > > > +	int crtc_vdisplay = crtc_state->base.adjusted_mode.crtc_vdisplay;
> > >             ^^^^^
> > > The crtc_ prefix is pretty much redundant.
> > 
> > display_mode has members named vdisplay and hdisplay and this avoids any
> > potential confusion.
> > 
> > 
> > > 
> > > > +	int psr_max_h = 0, psr_max_v = 0;
> > > 
> > > And this still reads as "max height" to my brain, but meh.
> > 
> > And here I thought this version leaves no room for confusion :) I should
> > just ask someone else to write this patch.
> > 
> > > 
> > > >  
> > > >  	/*
> > > >  	 * FIXME psr2_support is messed up. It's both computed
> > > > @@ -462,10 +463,18 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> > > >  	if (!dev_priv->psr.psr2_support)
> > > >  		return false;
> > > >  
> > > > -	/* PSR2 is restricted to work with panel resolutions up to 3640x2304 */
> > > > -	if (adjusted_mode->crtc_hdisplay > 3640 ||
> > > > -	    adjusted_mode->crtc_vdisplay > 2304) {
> > > > -		DRM_DEBUG_KMS("PSR2 not enabled, panel resolution too big\n");
> > > > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
> > > > +		psr_max_h = 4096;
> > > > +		psr_max_v = 2304;
> > > > +	} else if (IS_GEN9(dev_priv)) {
> > > > +		psr_max_h = 3640;
> > > > +		psr_max_v = 2304;
> > > > +	}
> > > 
> > > pre-SKL?
> > 
> > No PSR2 on pre-skl
> 
> OK. I'd drop the IS_GEN9 then. Would be less confusing for my brain at
> least.
> 
> > 
> > If we do somehow end up here, returning false and printing a debug
> > message will be useful.
> 
> Seems a bit overly protective. The has_psr2 check is just above. IMO

A recent patch that added this function had a version without the
has_psr2 check. So, I am just paranoid a similar patch might slip
through and the meaning of that flag is inconsistent too. But, I get the
idea of not adding redundant checks. Let's go with this for now and I'll
remove it when I get around to addressing the FIXME above.

> adding basically dead code "just in case" is not helpful in making the
> code easy to read.
> 
> Since you say pre-skl is not a problem here:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 

Thanks!


> > 
> > > 
> > > > +
> > > > +	if (crtc_hdisplay > psr_max_h || crtc_vdisplay > psr_max_v) {
> > > > +		DRM_DEBUG_KMS("PSR2 not enabled, resolution %dx%d > max supported %dx%d\n",
> > > > +			      crtc_hdisplay, crtc_vdisplay,
> > > > +			      psr_max_h, psr_max_v);
> > > >  		return false;
> > > >  	}
> > > >  
> > > > -- 
> > > > 2.14.1
> > > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/psr: Update PSR2 resolution check for Cannonlake

[Rodrigo Vivi]

On Tue, Mar 06, 2018 at 11:24:15PM +0200, Ville Syrjälä wrote:
> On Tue, Mar 06, 2018 at 08:45:44PM +0000, Pandiyan, Dhinakaran wrote:
> > 
> > 
> > 
> > On Tue, 2018-03-06 at 22:38 +0200, Ville Syrjälä wrote:
> > > On Tue, Mar 06, 2018 at 12:33:55PM -0800, Dhinakaran Pandiyan wrote:
> > > > In fact, apply the Cannonlake resolution check for all >= Gen-10 platforms
> > > > to be safe.
> > > > 
> > > > v3: Update GLK too. (Ville)
> > > >     Longer variable names.
> > > >     if-else in place of ternary operator.
> > > > v2: Use local variables for resolution limits and print them (Ville)
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Elio Martinez Monroy <elio.martinez.monroy@intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_psr.c | 21 +++++++++++++++------
> > > >  1 file changed, 15 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 05770790a4e9..23175c5c4a50 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -451,8 +451,9 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> > > >  {
> > > >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > >  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > > > -	const struct drm_display_mode *adjusted_mode =
> > > > -		&crtc_state->base.adjusted_mode;
> > > > +	int crtc_hdisplay = crtc_state->base.adjusted_mode.crtc_hdisplay;
> > > > +	int crtc_vdisplay = crtc_state->base.adjusted_mode.crtc_vdisplay;
> > >             ^^^^^
> > > The crtc_ prefix is pretty much redundant.
> > 
> > display_mode has members named vdisplay and hdisplay and this avoids any
> > potential confusion.
> > 
> > 
> > > 
> > > > +	int psr_max_h = 0, psr_max_v = 0;
> > > 
> > > And this still reads as "max height" to my brain, but meh.
> > 
> > And here I thought this version leaves no room for confusion :) I should
> > just ask someone else to write this patch.
> > 
> > > 
> > > >  
> > > >  	/*
> > > >  	 * FIXME psr2_support is messed up. It's both computed
> > > > @@ -462,10 +463,18 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> > > >  	if (!dev_priv->psr.psr2_support)
> > > >  		return false;
> > > >  
> > > > -	/* PSR2 is restricted to work with panel resolutions up to 3640x2304 */
> > > > -	if (adjusted_mode->crtc_hdisplay > 3640 ||
> > > > -	    adjusted_mode->crtc_vdisplay > 2304) {
> > > > -		DRM_DEBUG_KMS("PSR2 not enabled, panel resolution too big\n");
> > > > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
> > > > +		psr_max_h = 4096;
> > > > +		psr_max_v = 2304;
> > > > +	} else if (IS_GEN9(dev_priv)) {
> > > > +		psr_max_h = 3640;
> > > > +		psr_max_v = 2304;
> > > > +	}
> > > 
> > > pre-SKL?
> > 
> > No PSR2 on pre-skl
> 
> OK. I'd drop the IS_GEN9 then. Would be less confusing for my brain at
> least.
> 
> > 
> > If we do somehow end up here, returning false and printing a debug
> > message will be useful.
> 
> Seems a bit overly protective. The has_psr2 check is just above. IMO
> adding basically dead code "just in case" is not helpful in making the
> code easy to read.
> 
> Since you say pre-skl is not a problem here:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 

pushed, thanks

> > 
> > > 
> > > > +
> > > > +	if (crtc_hdisplay > psr_max_h || crtc_vdisplay > psr_max_v) {
> > > > +		DRM_DEBUG_KMS("PSR2 not enabled, resolution %dx%d > max supported %dx%d\n",
> > > > +			      crtc_hdisplay, crtc_vdisplay,
> > > > +			      psr_max_h, psr_max_v);
> > > >  		return false;
> > > >  	}
> > > >  
> > > > -- 
> > > > 2.14.1
> > > 
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915/psr: Update PSR2 resolution check for Cannonlake (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/psr: Update PSR2 resolution check for Cannonlake (rev3)
URL   : https://patchwork.freedesktop.org/series/39238/
State : success

== Summary ==

---- Possible new issues:

Test kms_busy:
        Subgroup extended-modeset-hang-newfb-render-b:
                dmesg-warn -> PASS       (shard-hsw)
Test kms_chv_cursor_fail:
        Subgroup pipe-c-64x64-right-edge:
                incomplete -> PASS       (shard-hsw)

---- Known issues:

Test gem_softpin:
        Subgroup noreloc-s3:
                incomplete -> PASS       (shard-hsw) fdo#103540
Test kms_chv_cursor_fail:
        Subgroup pipe-b-128x128-left-edge:
                dmesg-warn -> PASS       (shard-snb) fdo#105185 +3
Test kms_flip:
        Subgroup 2x-dpms-vs-vblank-race:
                pass       -> FAIL       (shard-hsw) fdo#103060
        Subgroup plain-flip-fb-recreate:
                pass       -> FAIL       (shard-hsw) fdo#100368
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-primscrn-spr-indfb-draw-blt:
                skip       -> PASS       (shard-snb) fdo#101623
Test kms_sysfs_edid_timing:
                pass       -> WARN       (shard-apl) fdo#100047
Test pm_lpsp:
        Subgroup screens-disabled:
                fail       -> PASS       (shard-hsw) fdo#104941

fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
fdo#104941 https://bugs.freedesktop.org/show_bug.cgi?id=104941

shard-apl        total:3381 pass:1779 dwarn:2   dfail:0   fail:7   skip:1591 time:11872s
shard-hsw        total:3467 pass:1771 dwarn:1   dfail:0   fail:3   skip:1691 time:11910s
shard-snb        total:3467 pass:1362 dwarn:2   dfail:0   fail:2   skip:2101 time:7006s
Blacklisted hosts:
shard-kbl        total:3467 pass:1947 dwarn:1   dfail:0   fail:8   skip:1511 time:9338s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8250/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx