| Message ID | 1525133500-30566-1-git-send-email-clinton.a.taylor@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 30 Apr 2018, clinton.a.taylor@intel.com wrote: > From: Clint Taylor <clinton.a.taylor@intel.com> > > In commit dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for eDP > if available."), the patch was always selecting the alternate refresh rate > even though user space was asking for the higher rate. This patch confirms > the alt mode setup time meets requirements and only uses the alt mode if > psr is enable for the platform. I'm confused. If the panel supports two refresh rates, why would PSR have to be a requirement for selecting one of them? If we now have bugs about only being able to select one, won't we then get bugs about only being able to select the other? I don't think this should be tied to PSR. BR, Jani. > > V2: use clock instead of vrefresh for compare. > V3: Confirm PSR is enabled and alt mode setup time is valid > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469 > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 4 +++- > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_psr.c | 35 +++++++++++++++++++++++------------ > 3 files changed, 28 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 83da50b..59b8a5c 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1864,7 +1864,9 @@ static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1, > intel_connector->panel.alt_fixed_mode; > struct drm_display_mode *req_mode = &pipe_config->base.mode; > > - if (!intel_edp_compare_alt_mode(req_mode, panel_mode)) > + if ((!intel_edp_compare_alt_mode(req_mode, panel_mode) && > + intel_psr_setup_time_valid(intel_dp, req_mode)) || > + (!i915_modparams.enable_psr)) > panel_mode = intel_connector->panel.fixed_mode; > > drm_mode_debug_printmodeline(panel_mode); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 11a1932..e69ce10 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1908,6 +1908,8 @@ void intel_psr_flush(struct drm_i915_private *dev_priv, > void intel_psr_init(struct drm_i915_private *dev_priv); > void intel_psr_single_frame_update(struct drm_i915_private *dev_priv, > unsigned frontbuffer_bits); > +bool intel_psr_setup_time_valid(struct intel_dp *intel_dp, > + const struct drm_display_mode *mode); > void intel_psr_compute_config(struct intel_dp *intel_dp, > struct intel_crtc_state *crtc_state); > void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug); > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index 6233a32..9cc4109 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -578,6 +578,27 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp, > return true; > } > > +bool intel_psr_setup_time_valid(struct intel_dp *intel_dp, const struct drm_display_mode *mode) > +{ > + int psr_setup_time = 0; > + > + psr_setup_time = drm_dp_psr_setup_time(intel_dp->psr_dpcd); > + if (psr_setup_time < 0) { > + DRM_DEBUG_KMS("PSR condition failed: Invalid PSR setup time (0x%02x)\n", > + intel_dp->psr_dpcd[1]); > + return false; > + } > + > + if (intel_usecs_to_scanlines(mode, psr_setup_time) > > + mode->crtc_vtotal - mode->crtc_vdisplay - 1) { > + DRM_DEBUG_KMS("PSR condition failed: PSR setup time (%d us) too long\n", > + psr_setup_time); > + return false; > + } > + > + return true; > +} > + > void intel_psr_compute_config(struct intel_dp *intel_dp, > struct intel_crtc_state *crtc_state) > { > @@ -585,7 +606,6 @@ void intel_psr_compute_config(struct intel_dp *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 psr_setup_time; > > if (!CAN_PSR(dev_priv)) > return; > @@ -626,17 +646,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, > return; > } > > - psr_setup_time = drm_dp_psr_setup_time(intel_dp->psr_dpcd); > - if (psr_setup_time < 0) { > - DRM_DEBUG_KMS("PSR condition failed: Invalid PSR setup time (0x%02x)\n", > - intel_dp->psr_dpcd[1]); > - return; > - } > - > - if (intel_usecs_to_scanlines(adjusted_mode, psr_setup_time) > > - adjusted_mode->crtc_vtotal - adjusted_mode->crtc_vdisplay - 1) { > - DRM_DEBUG_KMS("PSR condition failed: PSR setup time (%d us) too long\n", > - psr_setup_time); > + if (!intel_psr_setup_time_valid(intel_dp, adjusted_mode)) { > + DRM_DEBUG_KMS("PSR Setup invalid\n"); > return; > }
On Wed, May 02, 2018 at 10:22:16AM +0300, Jani Nikula wrote: > On Mon, 30 Apr 2018, clinton.a.taylor@intel.com wrote: > > From: Clint Taylor <clinton.a.taylor@intel.com> > > > > In commit dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for eDP > > if available."), the patch was always selecting the alternate refresh rate > > even though user space was asking for the higher rate. This patch confirms > > the alt mode setup time meets requirements and only uses the alt mode if > > psr is enable for the platform. > > I'm confused. > > If the panel supports two refresh rates, why would PSR have to be a > requirement for selecting one of them? If we now have bugs about only > being able to select one, won't we then get bugs about only being able > to select the other? > > I don't think this should be tied to PSR. The biggest reason behind the initial test was to allow us to select the 48Hz mode in order to test PSR in most of the panels we had around here since with 60Hz the vblank period was to short for PSR. But it seems we introduced a bad bug by doing that. Probably a revert is the best option here since I agree with Jani that we shouldn't do any mode selection based on a feture like PSR enabled. Well, I honestly believe there should be an easy way... If 48Hz is available and requested we should respect that and if we don't select anything we stick with the preferred one that is probably 60Hz. But since we are not able to find the correct way of allowing that in a clean way let's just revert the original patch. Thanks, Rodrigo. > > BR, > Jani. > > > > > V2: use clock instead of vrefresh for compare. > > V3: Confirm PSR is enabled and alt mode setup time is valid > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469 > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 4 +++- > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > drivers/gpu/drm/i915/intel_psr.c | 35 +++++++++++++++++++++++------------ > > 3 files changed, 28 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 83da50b..59b8a5c 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1864,7 +1864,9 @@ static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1, > > intel_connector->panel.alt_fixed_mode; > > struct drm_display_mode *req_mode = &pipe_config->base.mode; > > > > - if (!intel_edp_compare_alt_mode(req_mode, panel_mode)) > > + if ((!intel_edp_compare_alt_mode(req_mode, panel_mode) && > > + intel_psr_setup_time_valid(intel_dp, req_mode)) || > > + (!i915_modparams.enable_psr)) > > panel_mode = intel_connector->panel.fixed_mode; > > > > drm_mode_debug_printmodeline(panel_mode); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 11a1932..e69ce10 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1908,6 +1908,8 @@ void intel_psr_flush(struct drm_i915_private *dev_priv, > > void intel_psr_init(struct drm_i915_private *dev_priv); > > void intel_psr_single_frame_update(struct drm_i915_private *dev_priv, > > unsigned frontbuffer_bits); > > +bool intel_psr_setup_time_valid(struct intel_dp *intel_dp, > > + const struct drm_display_mode *mode); > > void intel_psr_compute_config(struct intel_dp *intel_dp, > > struct intel_crtc_state *crtc_state); > > void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug); > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > index 6233a32..9cc4109 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -578,6 +578,27 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp, > > return true; > > } > > > > +bool intel_psr_setup_time_valid(struct intel_dp *intel_dp, const struct drm_display_mode *mode) > > +{ > > + int psr_setup_time = 0; > > + > > + psr_setup_time = drm_dp_psr_setup_time(intel_dp->psr_dpcd); > > + if (psr_setup_time < 0) { > > + DRM_DEBUG_KMS("PSR condition failed: Invalid PSR setup time (0x%02x)\n", > > + intel_dp->psr_dpcd[1]); > > + return false; > > + } > > + > > + if (intel_usecs_to_scanlines(mode, psr_setup_time) > > > + mode->crtc_vtotal - mode->crtc_vdisplay - 1) { > > + DRM_DEBUG_KMS("PSR condition failed: PSR setup time (%d us) too long\n", > > + psr_setup_time); > > + return false; > > + } > > + > > + return true; > > +} > > + > > void intel_psr_compute_config(struct intel_dp *intel_dp, > > struct intel_crtc_state *crtc_state) > > { > > @@ -585,7 +606,6 @@ void intel_psr_compute_config(struct intel_dp *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 psr_setup_time; > > > > if (!CAN_PSR(dev_priv)) > > return; > > @@ -626,17 +646,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, > > return; > > } > > > > - psr_setup_time = drm_dp_psr_setup_time(intel_dp->psr_dpcd); > > - if (psr_setup_time < 0) { > > - DRM_DEBUG_KMS("PSR condition failed: Invalid PSR setup time (0x%02x)\n", > > - intel_dp->psr_dpcd[1]); > > - return; > > - } > > - > > - if (intel_usecs_to_scanlines(adjusted_mode, psr_setup_time) > > > - adjusted_mode->crtc_vtotal - adjusted_mode->crtc_vdisplay - 1) { > > - DRM_DEBUG_KMS("PSR condition failed: PSR setup time (%d us) too long\n", > > - psr_setup_time); > > + if (!intel_psr_setup_time_valid(intel_dp, adjusted_mode)) { > > + DRM_DEBUG_KMS("PSR Setup invalid\n"); > > return; > > } > > -- > Jani Nikula, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 83da50b..59b8a5c 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1864,7 +1864,9 @@ static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1, intel_connector->panel.alt_fixed_mode; struct drm_display_mode *req_mode = &pipe_config->base.mode; - if (!intel_edp_compare_alt_mode(req_mode, panel_mode)) + if ((!intel_edp_compare_alt_mode(req_mode, panel_mode) && + intel_psr_setup_time_valid(intel_dp, req_mode)) || + (!i915_modparams.enable_psr)) panel_mode = intel_connector->panel.fixed_mode; drm_mode_debug_printmodeline(panel_mode); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 11a1932..e69ce10 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1908,6 +1908,8 @@ void intel_psr_flush(struct drm_i915_private *dev_priv, void intel_psr_init(struct drm_i915_private *dev_priv); void intel_psr_single_frame_update(struct drm_i915_private *dev_priv, unsigned frontbuffer_bits); +bool intel_psr_setup_time_valid(struct intel_dp *intel_dp, + const struct drm_display_mode *mode); void intel_psr_compute_config(struct intel_dp *intel_dp, struct intel_crtc_state *crtc_state); void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug); diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 6233a32..9cc4109 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -578,6 +578,27 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp, return true; } +bool intel_psr_setup_time_valid(struct intel_dp *intel_dp, const struct drm_display_mode *mode) +{ + int psr_setup_time = 0; + + psr_setup_time = drm_dp_psr_setup_time(intel_dp->psr_dpcd); + if (psr_setup_time < 0) { + DRM_DEBUG_KMS("PSR condition failed: Invalid PSR setup time (0x%02x)\n", + intel_dp->psr_dpcd[1]); + return false; + } + + if (intel_usecs_to_scanlines(mode, psr_setup_time) > + mode->crtc_vtotal - mode->crtc_vdisplay - 1) { + DRM_DEBUG_KMS("PSR condition failed: PSR setup time (%d us) too long\n", + psr_setup_time); + return false; + } + + return true; +} + void intel_psr_compute_config(struct intel_dp *intel_dp, struct intel_crtc_state *crtc_state) { @@ -585,7 +606,6 @@ void intel_psr_compute_config(struct intel_dp *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 psr_setup_time; if (!CAN_PSR(dev_priv)) return; @@ -626,17 +646,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, return; } - psr_setup_time = drm_dp_psr_setup_time(intel_dp->psr_dpcd); - if (psr_setup_time < 0) { - DRM_DEBUG_KMS("PSR condition failed: Invalid PSR setup time (0x%02x)\n", - intel_dp->psr_dpcd[1]); - return; - } - - if (intel_usecs_to_scanlines(adjusted_mode, psr_setup_time) > - adjusted_mode->crtc_vtotal - adjusted_mode->crtc_vdisplay - 1) { - DRM_DEBUG_KMS("PSR condition failed: PSR setup time (%d us) too long\n", - psr_setup_time); + if (!intel_psr_setup_time_valid(intel_dp, adjusted_mode)) { + DRM_DEBUG_KMS("PSR Setup invalid\n"); return; }