| Message ID | 20180306203355.29292-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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 > >
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 > > > >
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
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; }
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(-)