Re: [Intel-gfx] [PATCH v3] drm/i915/psr: Update PSR2 resolution check for Cannonlake
On Tue, Mar 06, 2018 at 11:24:15PM +0200, Ville Syrjälä wrote: > On Tue, Mar 06, 2018 at 08:45:44PM +, 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ä > > > > Cc: Rodrigo Vivi > > > > Cc: Elio Martinez Monroy > > > > Signed-off-by: Dhinakaran Pandiyan > > > > --- > > > > 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ä > 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
Re: [Intel-gfx] [PATCH v3] drm/i915/psr: Update PSR2 resolution check for Cannonlake
On Tue, 2018-03-06 at 23:24 +0200, Ville Syrjälä wrote: > On Tue, Mar 06, 2018 at 08:45:44PM +, 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ä > > > > Cc: Rodrigo Vivi > > > > Cc: Elio Martinez Monroy > > > > Signed-off-by: Dhinakaran Pandiyan > > > > --- > > > > 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ä > 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: [Intel-gfx] [PATCH v3] drm/i915/psr: Update PSR2 resolution check for Cannonlake
On Tue, Mar 06, 2018 at 08:45:44PM +, 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ä > > > Cc: Rodrigo Vivi > > > Cc: Elio Martinez Monroy > > > Signed-off-by: Dhinakaran Pandiyan > > > --- > > > 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ä > > > > > > + > > > + 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: [Intel-gfx] [PATCH v3] drm/i915/psr: Update PSR2 resolution check for Cannonlake
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ä > > Cc: Rodrigo Vivi > > Cc: Elio Martinez Monroy > > Signed-off-by: Dhinakaran Pandiyan > > --- > > 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
Re: [Intel-gfx] [PATCH v3] drm/i915/psr: Update PSR2 resolution check for Cannonlake
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ä > Cc: Rodrigo Vivi > Cc: Elio Martinez Monroy > Signed-off-by: Dhinakaran Pandiyan > --- > 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