| Message ID | 20180301212709.14505-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 01, 2018 at 01:27:09PM -0800, Dhinakaran Pandiyan wrote: > In fact, apply the Cannonlake resolution check for all >= Gen-10 platforms > to be safe. > > 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 | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index 05770790a4e9..66d04a8dd99e 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_h = crtc_state->base.adjusted_mode.crtc_hdisplay; > + int crtc_v = crtc_state->base.adjusted_mode.crtc_vdisplay; I'd probably call these hdisp/vdisp or something like that. "crtc_h" makes me think it's a height of a plane in crtc (pipe source) coordinates. > + int max_h, max_v; > > /* > * FIXME psr2_support is messed up. It's both computed > @@ -462,10 +463,11 @@ 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"); > + max_h = INTEL_GEN(dev_priv) >= 10 ? 4096 : 3640; > + max_v = 2304; GLK should use the higher limit too no? Looking at the *future* stuff for this it looks like we'll be getting different limits again soon. So I'd prep for that day by making this a full blown if ladder from the start. > + if (crtc_h > max_h || crtc_v > max_v) { > + DRM_DEBUG_KMS("PSR2 not enabled, resolution %dx%d > max supported %dx%d\n", > + crtc_h, crtc_v, max_h, max_v); > return false; > } > > -- > 2.14.1
On Thu, 2018-03-01 at 23:47 +0200, Ville Syrjälä wrote: > On Thu, Mar 01, 2018 at 01:27:09PM -0800, Dhinakaran Pandiyan wrote: > > In fact, apply the Cannonlake resolution check for all >= Gen-10 platforms > > to be safe. > > > > 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 | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > index 05770790a4e9..66d04a8dd99e 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_h = crtc_state->base.adjusted_mode.crtc_hdisplay; > > + int crtc_v = crtc_state->base.adjusted_mode.crtc_vdisplay; > > I'd probably call these hdisp/vdisp or something like that. "crtc_h" makes > me think it's a height of a plane in crtc (pipe source) coordinates. and h/vdisplay are specifically related to the mode? > > > + int max_h, max_v; I guess this is okay then? > > > > /* > > * FIXME psr2_support is messed up. It's both computed > > @@ -462,10 +463,11 @@ 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"); > > + max_h = INTEL_GEN(dev_priv) >= 10 ? 4096 : 3640; > > + max_v = 2304; > > GLK should use the higher limit too no? Yeah, I just checked and it makes sense to update GLK too. > > Looking at the *future* stuff for this it looks like we'll be getting > different limits again soon. So I'd prep for that day by making this > a full blown if ladder from the start. > > > + if (crtc_h > max_h || crtc_v > max_v) { > > + DRM_DEBUG_KMS("PSR2 not enabled, resolution %dx%d > max supported %dx%d\n", > > + crtc_h, crtc_v, max_h, max_v); > > return false; > > } > > > > -- > > 2.14.1 >
On Thu, Mar 01, 2018 at 10:04:04PM +0000, Pandiyan, Dhinakaran wrote: > > > > On Thu, 2018-03-01 at 23:47 +0200, Ville Syrjälä wrote: > > On Thu, Mar 01, 2018 at 01:27:09PM -0800, Dhinakaran Pandiyan wrote: > > > In fact, apply the Cannonlake resolution check for all >= Gen-10 platforms > > > to be safe. > > > > > > 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 | 14 ++++++++------ > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > > index 05770790a4e9..66d04a8dd99e 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_h = crtc_state->base.adjusted_mode.crtc_hdisplay; > > > + int crtc_v = crtc_state->base.adjusted_mode.crtc_vdisplay; > > > > I'd probably call these hdisp/vdisp or something like that. "crtc_h" makes > > me think it's a height of a plane in crtc (pipe source) coordinates. > > and h/vdisplay are specifically related to the mode? They are the active portion of the timings. > > > > > > + int max_h, max_v; > I guess this is okay then? max_h does make me immediately think "max height" so they could probably use a few more characters as well. > > > > > > > /* > > > * FIXME psr2_support is messed up. It's both computed > > > @@ -462,10 +463,11 @@ 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"); > > > + max_h = INTEL_GEN(dev_priv) >= 10 ? 4096 : 3640; > > > + max_v = 2304; > > > > GLK should use the higher limit too no? > > Yeah, I just checked and it makes sense to update GLK too. > > > > > Looking at the *future* stuff for this it looks like we'll be getting > > different limits again soon. So I'd prep for that day by making this > > a full blown if ladder from the start. > > > > > + if (crtc_h > max_h || crtc_v > max_v) { > > > + DRM_DEBUG_KMS("PSR2 not enabled, resolution %dx%d > max supported %dx%d\n", > > > + crtc_h, crtc_v, max_h, max_v); > > > return false; > > > } > > > > > > -- > > > 2.14.1 > >
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 05770790a4e9..66d04a8dd99e 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_h = crtc_state->base.adjusted_mode.crtc_hdisplay; + int crtc_v = crtc_state->base.adjusted_mode.crtc_vdisplay; + int max_h, max_v; /* * FIXME psr2_support is messed up. It's both computed @@ -462,10 +463,11 @@ 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"); + max_h = INTEL_GEN(dev_priv) >= 10 ? 4096 : 3640; + max_v = 2304; + if (crtc_h > max_h || crtc_v > max_v) { + DRM_DEBUG_KMS("PSR2 not enabled, resolution %dx%d > max supported %dx%d\n", + crtc_h, crtc_v, max_h, max_v); return false; }
In fact, apply the Cannonlake resolution check for all >= Gen-10 platforms to be safe. 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 | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)