| Message ID | 20180307163939.59880-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2018-03-07 at 17:39 +0100, Maarten Lankhorst wrote: > Similar to enable_fbc, enable_psr was ignored at runtime if it was > changed. The easiest fix is to pretend enable_psr is ignored at > configure time, and never activate it for !enable_psr, so both cases > are handled without modesets. What about cases where psr_flush() is not called and consequently the module parameter is not checked? With HW tracking, PSR is enabled/disabled during modeset and the hardware is expected to exit and activate PSR without driver intervention. > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Tested-by: Benjamin Berg <bberg@redhat.com> > Cc: Benjamin Berg <bberg@redhat.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index 23175c5c4a50..ac3ce7a1c2a7 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -502,11 +502,6 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, > if (!CAN_PSR(dev_priv)) > return; > > - if (!i915_modparams.enable_psr) { > - DRM_DEBUG_KMS("PSR disable by flag\n"); > - return; > - } > - > /* > * HSW spec explicitly says PSR is tied to port A. > * BDW+ platforms with DDI implementation of PSR have different > @@ -559,7 +554,10 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, > > crtc_state->has_psr = true; > crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state); > - DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : ""); > + if (i915_modparams.enable_psr) > + DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : ""); > + else > + DRM_DEBUG_KMS("PSR disable by flag\n"); > } > > static void intel_psr_activate(struct intel_dp *intel_dp) > @@ -652,7 +650,9 @@ void intel_psr_enable(struct intel_dp *intel_dp, > dev_priv->psr.enable_source(intel_dp, crtc_state); > dev_priv->psr.enabled = intel_dp; > > - if (INTEL_GEN(dev_priv) >= 9) { > + if (!i915_modparams.enable_psr) { > + DRM_DEBUG_KMS("PSR disable by flag\n"); > + } else if (INTEL_GEN(dev_priv) >= 9) { > intel_psr_activate(intel_dp); > } else { > /* > @@ -843,7 +843,7 @@ static void intel_psr_work(struct work_struct *work) > * recheck. Since psr_flush first clears this and then reschedules we > * won't ever miss a flush when bailing out here. > */ > - if (dev_priv->psr.busy_frontbuffer_bits) > + if (dev_priv->psr.busy_frontbuffer_bits || !i915_modparams.enable_psr) > goto unlock; > > intel_psr_activate(intel_dp); > @@ -1015,7 +1015,8 @@ void intel_psr_flush(struct drm_i915_private *dev_priv, > return; > > mutex_lock(&dev_priv->psr.lock); > - if (!dev_priv->psr.enabled) { > + if (!dev_priv->psr.enabled || !i915_modparams.enable_psr) { > + intel_psr_exit(dev_priv); > mutex_unlock(&dev_priv->psr.lock); > return; > }
Op 07-03-18 om 23:22 schreef Pandiyan, Dhinakaran: > On Wed, 2018-03-07 at 17:39 +0100, Maarten Lankhorst wrote: >> Similar to enable_fbc, enable_psr was ignored at runtime if it was >> changed. The easiest fix is to pretend enable_psr is ignored at >> configure time, and never activate it for !enable_psr, so both cases >> are handled without modesets. > What about cases where psr_flush() is not called and consequently the > module parameter is not checked? With HW tracking, PSR is > enabled/disabled during modeset and the hardware is expected to exit and > activate PSR without driver intervention. It looks like intel_frontbuffer_flush always calls intel_psr_flush, so we at least get a PSR toggle after every atomic commit? ~Maarten >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Tested-by: Benjamin Berg <bberg@redhat.com> >> Cc: Benjamin Berg <bberg@redhat.com> >> --- >> drivers/gpu/drm/i915/intel_psr.c | 19 ++++++++++--------- >> 1 file changed, 10 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c >> index 23175c5c4a50..ac3ce7a1c2a7 100644 >> --- a/drivers/gpu/drm/i915/intel_psr.c >> +++ b/drivers/gpu/drm/i915/intel_psr.c >> @@ -502,11 +502,6 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, >> if (!CAN_PSR(dev_priv)) >> return; >> >> - if (!i915_modparams.enable_psr) { >> - DRM_DEBUG_KMS("PSR disable by flag\n"); >> - return; >> - } >> - >> /* >> * HSW spec explicitly says PSR is tied to port A. >> * BDW+ platforms with DDI implementation of PSR have different >> @@ -559,7 +554,10 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, >> >> crtc_state->has_psr = true; >> crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state); >> - DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : ""); >> + if (i915_modparams.enable_psr) >> + DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : ""); >> + else >> + DRM_DEBUG_KMS("PSR disable by flag\n"); >> } >> >> static void intel_psr_activate(struct intel_dp *intel_dp) >> @@ -652,7 +650,9 @@ void intel_psr_enable(struct intel_dp *intel_dp, >> dev_priv->psr.enable_source(intel_dp, crtc_state); >> dev_priv->psr.enabled = intel_dp; >> >> - if (INTEL_GEN(dev_priv) >= 9) { >> + if (!i915_modparams.enable_psr) { >> + DRM_DEBUG_KMS("PSR disable by flag\n"); >> + } else if (INTEL_GEN(dev_priv) >= 9) { >> intel_psr_activate(intel_dp); >> } else { >> /* >> @@ -843,7 +843,7 @@ static void intel_psr_work(struct work_struct *work) >> * recheck. Since psr_flush first clears this and then reschedules we >> * won't ever miss a flush when bailing out here. >> */ >> - if (dev_priv->psr.busy_frontbuffer_bits) >> + if (dev_priv->psr.busy_frontbuffer_bits || !i915_modparams.enable_psr) >> goto unlock; >> >> intel_psr_activate(intel_dp); >> @@ -1015,7 +1015,8 @@ void intel_psr_flush(struct drm_i915_private *dev_priv, >> return; >> >> mutex_lock(&dev_priv->psr.lock); >> - if (!dev_priv->psr.enabled) { >> + if (!dev_priv->psr.enabled || !i915_modparams.enable_psr) { >> + intel_psr_exit(dev_priv); >> mutex_unlock(&dev_priv->psr.lock); >> return; >> }
On Thu, 2018-03-08 at 08:07 +0100, Maarten Lankhorst wrote: > Op 07-03-18 om 23:22 schreef Pandiyan, Dhinakaran: > > On Wed, 2018-03-07 at 17:39 +0100, Maarten Lankhorst wrote: > >> Similar to enable_fbc, enable_psr was ignored at runtime if it was > >> changed. The easiest fix is to pretend enable_psr is ignored at > >> configure time, and never activate it for !enable_psr, so both cases > >> are handled without modesets. > > What about cases where psr_flush() is not called and consequently the > > module parameter is not checked? With HW tracking, PSR is > > enabled/disabled during modeset and the hardware is expected to exit and > > activate PSR without driver intervention. > It looks like intel_frontbuffer_flush always calls intel_psr_flush, > so we at least get a PSR toggle after every atomic commit? I have a patch to remove flush() from legacy_cursor_update(). We end up with an inconsistent behavior when that patch gets merged, cursor moves -> trigger psr exit but don't read module parameter commits -> trigger psr exit but read module parameter Eventually, when we get to removing flush() from commits, then this patch won't really be useful. And tests disabling/enabling PSR at runtime will probably fail. > > ~Maarten > > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Tested-by: Benjamin Berg <bberg@redhat.com> > >> Cc: Benjamin Berg <bberg@redhat.com> > >> --- > >> drivers/gpu/drm/i915/intel_psr.c | 19 ++++++++++--------- > >> 1 file changed, 10 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > >> index 23175c5c4a50..ac3ce7a1c2a7 100644 > >> --- a/drivers/gpu/drm/i915/intel_psr.c > >> +++ b/drivers/gpu/drm/i915/intel_psr.c > >> @@ -502,11 +502,6 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, > >> if (!CAN_PSR(dev_priv)) > >> return; > >> > >> - if (!i915_modparams.enable_psr) { > >> - DRM_DEBUG_KMS("PSR disable by flag\n"); > >> - return; > >> - } > >> - > >> /* > >> * HSW spec explicitly says PSR is tied to port A. > >> * BDW+ platforms with DDI implementation of PSR have different > >> @@ -559,7 +554,10 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, > >> > >> crtc_state->has_psr = true; > >> crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state); > >> - DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : ""); > >> + if (i915_modparams.enable_psr) > >> + DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : ""); > >> + else > >> + DRM_DEBUG_KMS("PSR disable by flag\n"); > >> } > >> > >> static void intel_psr_activate(struct intel_dp *intel_dp) > >> @@ -652,7 +650,9 @@ void intel_psr_enable(struct intel_dp *intel_dp, > >> dev_priv->psr.enable_source(intel_dp, crtc_state); > >> dev_priv->psr.enabled = intel_dp; > >> > >> - if (INTEL_GEN(dev_priv) >= 9) { > >> + if (!i915_modparams.enable_psr) { > >> + DRM_DEBUG_KMS("PSR disable by flag\n"); > >> + } else if (INTEL_GEN(dev_priv) >= 9) { > >> intel_psr_activate(intel_dp); > >> } else { > >> /* > >> @@ -843,7 +843,7 @@ static void intel_psr_work(struct work_struct *work) > >> * recheck. Since psr_flush first clears this and then reschedules we > >> * won't ever miss a flush when bailing out here. > >> */ > >> - if (dev_priv->psr.busy_frontbuffer_bits) > >> + if (dev_priv->psr.busy_frontbuffer_bits || !i915_modparams.enable_psr) > >> goto unlock; > >> > >> intel_psr_activate(intel_dp); > >> @@ -1015,7 +1015,8 @@ void intel_psr_flush(struct drm_i915_private *dev_priv, > >> return; > >> > >> mutex_lock(&dev_priv->psr.lock); > >> - if (!dev_priv->psr.enabled) { > >> + if (!dev_priv->psr.enabled || !i915_modparams.enable_psr) { > >> + intel_psr_exit(dev_priv); > >> mutex_unlock(&dev_priv->psr.lock); > >> return; > >> } > >
Op 08-03-18 om 18:43 schreef Pandiyan, Dhinakaran: > > > On Thu, 2018-03-08 at 08:07 +0100, Maarten Lankhorst wrote: >> Op 07-03-18 om 23:22 schreef Pandiyan, Dhinakaran: >>> On Wed, 2018-03-07 at 17:39 +0100, Maarten Lankhorst wrote: >>>> Similar to enable_fbc, enable_psr was ignored at runtime if it was >>>> changed. The easiest fix is to pretend enable_psr is ignored at >>>> configure time, and never activate it for !enable_psr, so both cases >>>> are handled without modesets. >>> What about cases where psr_flush() is not called and consequently the >>> module parameter is not checked? With HW tracking, PSR is >>> enabled/disabled during modeset and the hardware is expected to exit and >>> activate PSR without driver intervention. >> It looks like intel_frontbuffer_flush always calls intel_psr_flush, >> so we at least get a PSR toggle after every atomic commit? > I have a patch to remove flush() from legacy_cursor_update(). We end up > with an inconsistent behavior when that patch gets merged, > cursor moves -> trigger psr exit but don't read module parameter > commits -> trigger psr exit but read module parameter Legacy cursor updates are special, I don't mind them not changing PSR. > Eventually, when we get to removing flush() from commits, then this > patch won't really be useful. And tests disabling/enabling PSR at > runtime will probably fail. Could we transition to debugfs for changing it at runtime? ~Maarten
On Thu, 2018-03-08 at 18:52 +0100, Maarten Lankhorst wrote: > Op 08-03-18 om 18:43 schreef Pandiyan, Dhinakaran: > > > > > > On Thu, 2018-03-08 at 08:07 +0100, Maarten Lankhorst wrote: > >> Op 07-03-18 om 23:22 schreef Pandiyan, Dhinakaran: > >>> On Wed, 2018-03-07 at 17:39 +0100, Maarten Lankhorst wrote: > >>>> Similar to enable_fbc, enable_psr was ignored at runtime if it was > >>>> changed. The easiest fix is to pretend enable_psr is ignored at > >>>> configure time, and never activate it for !enable_psr, so both cases > >>>> are handled without modesets. > >>> What about cases where psr_flush() is not called and consequently the > >>> module parameter is not checked? With HW tracking, PSR is > >>> enabled/disabled during modeset and the hardware is expected to exit and > >>> activate PSR without driver intervention. > >> It looks like intel_frontbuffer_flush always calls intel_psr_flush, > >> so we at least get a PSR toggle after every atomic commit? > > I have a patch to remove flush() from legacy_cursor_update(). We end up > > with an inconsistent behavior when that patch gets merged, > > cursor moves -> trigger psr exit but don't read module parameter > > commits -> trigger psr exit but read module parameter > Legacy cursor updates are special, I don't mind them not changing PSR. > > Eventually, when we get to removing flush() from commits, then this > > patch won't really be useful. And tests disabling/enabling PSR at > > runtime will probably fail. > Could we transition to debugfs for changing it at runtime? That does sound like a better idea. > > ~Maarten
On Thu, Mar 08, 2018 at 10:07:05AM -0800, Pandiyan, Dhinakaran wrote: > > > > On Thu, 2018-03-08 at 18:52 +0100, Maarten Lankhorst wrote: > > Op 08-03-18 om 18:43 schreef Pandiyan, Dhinakaran: > > > > > > > > > On Thu, 2018-03-08 at 08:07 +0100, Maarten Lankhorst wrote: > > >> Op 07-03-18 om 23:22 schreef Pandiyan, Dhinakaran: > > >>> On Wed, 2018-03-07 at 17:39 +0100, Maarten Lankhorst wrote: > > >>>> Similar to enable_fbc, enable_psr was ignored at runtime if it was > > >>>> changed. The easiest fix is to pretend enable_psr is ignored at > > >>>> configure time, and never activate it for !enable_psr, so both cases > > >>>> are handled without modesets. > > >>> What about cases where psr_flush() is not called and consequently the > > >>> module parameter is not checked? With HW tracking, PSR is > > >>> enabled/disabled during modeset and the hardware is expected to exit and > > >>> activate PSR without driver intervention. > > >> It looks like intel_frontbuffer_flush always calls intel_psr_flush, > > >> so we at least get a PSR toggle after every atomic commit? > > > I have a patch to remove flush() from legacy_cursor_update(). We end up > > > with an inconsistent behavior when that patch gets merged, > > > cursor moves -> trigger psr exit but don't read module parameter > > > commits -> trigger psr exit but read module parameter > > Legacy cursor updates are special, I don't mind them not changing PSR. > > > Eventually, when we get to removing flush() from commits, then this > > > patch won't really be useful. And tests disabling/enabling PSR at > > > runtime will probably fail. > > Could we transition to debugfs for changing it at runtime? > > That does sound like a better idea. +1 > > > > > ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 23175c5c4a50..ac3ce7a1c2a7 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -502,11 +502,6 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, if (!CAN_PSR(dev_priv)) return; - if (!i915_modparams.enable_psr) { - DRM_DEBUG_KMS("PSR disable by flag\n"); - return; - } - /* * HSW spec explicitly says PSR is tied to port A. * BDW+ platforms with DDI implementation of PSR have different @@ -559,7 +554,10 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, crtc_state->has_psr = true; crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state); - DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : ""); + if (i915_modparams.enable_psr) + DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : ""); + else + DRM_DEBUG_KMS("PSR disable by flag\n"); } static void intel_psr_activate(struct intel_dp *intel_dp) @@ -652,7 +650,9 @@ void intel_psr_enable(struct intel_dp *intel_dp, dev_priv->psr.enable_source(intel_dp, crtc_state); dev_priv->psr.enabled = intel_dp; - if (INTEL_GEN(dev_priv) >= 9) { + if (!i915_modparams.enable_psr) { + DRM_DEBUG_KMS("PSR disable by flag\n"); + } else if (INTEL_GEN(dev_priv) >= 9) { intel_psr_activate(intel_dp); } else { /* @@ -843,7 +843,7 @@ static void intel_psr_work(struct work_struct *work) * recheck. Since psr_flush first clears this and then reschedules we * won't ever miss a flush when bailing out here. */ - if (dev_priv->psr.busy_frontbuffer_bits) + if (dev_priv->psr.busy_frontbuffer_bits || !i915_modparams.enable_psr) goto unlock; intel_psr_activate(intel_dp); @@ -1015,7 +1015,8 @@ void intel_psr_flush(struct drm_i915_private *dev_priv, return; mutex_lock(&dev_priv->psr.lock); - if (!dev_priv->psr.enabled) { + if (!dev_priv->psr.enabled || !i915_modparams.enable_psr) { + intel_psr_exit(dev_priv); mutex_unlock(&dev_priv->psr.lock); return; }