* Re: [PATCH] drm/i915/psr: Update PSR2 resolution check for Cannonlake
2018-03-01 20:47 ` Ville Syrjälä
@ 2018-03-01 20:50 ` Pandiyan, Dhinakaran
2018-03-01 21:27 ` [PATCH v2] " Dhinakaran Pandiyan
2018-03-01 22:09 ` [PATCH] " Rodrigo Vivi
2 siblings, 0 replies; 11+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-01 20:50 UTC (permalink / raw)
To: ville.syrjala@linux.intel.com
Cc: intel-gfx@lists.freedesktop.org, Vivi, Rodrigo
On Thu, 2018-03-01 at 22:47 +0200, Ville Syrjälä wrote:
> On Thu, Mar 01, 2018 at 12:38:58PM -0800, Dhinakaran Pandiyan wrote:
> > In fact, apply the Cannonlake resolution check for all > Gen-9 platforms to
> > be safe.
> >
> > 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 | 11 ++++++-----
> > 1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 05770790a4e9..2a2c696c4109 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -451,8 +451,8 @@ 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;
> >
> > /*
> > * FIXME psr2_support is messed up. It's both computed
> > @@ -462,9 +462,10 @@ 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) {
> > + if (crtc_h > 4096 || crtc_v > 2304) {
> > + DRM_DEBUG_KMS("PSR2 not enabled, panel resolution too big\n");
> > + return false;
> > + } else if (IS_GEN9(dev_priv) && (crtc_h > 3640 || crtc_v > 2304)) {
>
> I would suggest introducing max_w/h or something similar so that you
> don't have to duplicate the actual code.
>
> When debugging things from logs one might want to know both the requested
> size and the max. So printing those out might be a good idea.
Agreed on both points.
>
> > DRM_DEBUG_KMS("PSR2 not enabled, panel resolution too big\n");
> > return false;
> > }
> > --
> > 2.14.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v2] drm/i915/psr: Update PSR2 resolution check for Cannonlake
2018-03-01 20:47 ` Ville Syrjälä
2018-03-01 20:50 ` Pandiyan, Dhinakaran
@ 2018-03-01 21:27 ` Dhinakaran Pandiyan
2018-03-01 21:47 ` Ville Syrjälä
2018-03-01 22:09 ` [PATCH] " Rodrigo Vivi
2 siblings, 1 reply; 11+ messages in thread
From: Dhinakaran Pandiyan @ 2018-03-01 21:27 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan, Rodrigo Vivi
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;
+ 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;
}
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2] drm/i915/psr: Update PSR2 resolution check for Cannonlake
2018-03-01 21:27 ` [PATCH v2] " Dhinakaran Pandiyan
@ 2018-03-01 21:47 ` Ville Syrjälä
2018-03-01 22:04 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 11+ messages in thread
From: Ville Syrjälä @ 2018-03-01 21:47 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: intel-gfx, Rodrigo Vivi
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
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] drm/i915/psr: Update PSR2 resolution check for Cannonlake
2018-03-01 21:47 ` Ville Syrjälä
@ 2018-03-01 22:04 ` Pandiyan, Dhinakaran
2018-03-02 8:13 ` Ville Syrjälä
0 siblings, 1 reply; 11+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-01 22:04 UTC (permalink / raw)
To: ville.syrjala@linux.intel.com
Cc: intel-gfx@lists.freedesktop.org, Vivi, Rodrigo
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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] drm/i915/psr: Update PSR2 resolution check for Cannonlake
2018-03-01 22:04 ` Pandiyan, Dhinakaran
@ 2018-03-02 8:13 ` Ville Syrjälä
0 siblings, 0 replies; 11+ messages in thread
From: Ville Syrjälä @ 2018-03-02 8:13 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx@lists.freedesktop.org, Vivi, Rodrigo
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
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915/psr: Update PSR2 resolution check for Cannonlake
2018-03-01 20:47 ` Ville Syrjälä
2018-03-01 20:50 ` Pandiyan, Dhinakaran
2018-03-01 21:27 ` [PATCH v2] " Dhinakaran Pandiyan
@ 2018-03-01 22:09 ` Rodrigo Vivi
2 siblings, 0 replies; 11+ messages in thread
From: Rodrigo Vivi @ 2018-03-01 22:09 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Dhinakaran Pandiyan
On Thu, Mar 01, 2018 at 10:47:05PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 01, 2018 at 12:38:58PM -0800, Dhinakaran Pandiyan wrote:
> > In fact, apply the Cannonlake resolution check for all > Gen-9 platforms to
> > be safe.
> >
> > 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 | 11 ++++++-----
> > 1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 05770790a4e9..2a2c696c4109 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -451,8 +451,8 @@ 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;
> >
> > /*
> > * FIXME psr2_support is messed up. It's both computed
> > @@ -462,9 +462,10 @@ 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) {
> > + if (crtc_h > 4096 || crtc_v > 2304) {
> > + DRM_DEBUG_KMS("PSR2 not enabled, panel resolution too big\n");
> > + return false;
> > + } else if (IS_GEN9(dev_priv) && (crtc_h > 3640 || crtc_v > 2304)) {
>
> I would suggest introducing max_w/h or something similar so that you
> don't have to duplicate the actual code.
I was going to suggest same thing. So it gets more clear that it is
per platform based and avoid duplication of code
>
> When debugging things from logs one might want to know both the requested
> size and the max. So printing those out might be a good idea.
>
> > DRM_DEBUG_KMS("PSR2 not enabled, panel resolution too big\n");
> > return false;
> > }
> > --
> > 2.14.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread