Re: [Intel-gfx] [PATCH v2 2/6] drm/i915/psr: Always wait for idle state when disabling PSR
On Fri, 2018-10-19 at 23:46 +, Souza, Jose wrote: > On Fri, 2018-10-19 at 13:42 -0700, Dhinakaran Pandiyan wrote: > > On Wednesday, October 10, 2018 5:41:20 PM PDT José Roberto de Souza > > wrote: > > > It should always wait for idle state when disabling PSR because "It" is ambiguous here. How about something along the lines of - "Unconditionally wait for PSR hardware to idle when disabling PSR. Even if a preceding front buffer invalidate already disabled the feature, a wait for idle is required before for re-enabling". Feel free to ignore. > > > PSR > > > could be inactive due a call to intel_psr_exit() and while PSR is > > > still being disabled asynchronously userspace could change the > > > modeset causing a call to psr_disable() that will not wait for > > > PSR > > > idle and then PSR will be enabled again while PSR is still not > > > idle. > > > > > > v2: rebased on top of the patch reusing psr_exit() > > > > > > Cc: Rodrigo Vivi > > > Cc: Dhinakaran Pandiyan > > > Signed-off-by: José Roberto de Souza > > > --- > > > drivers/gpu/drm/i915/intel_psr.c | 41 ++ > > > -- > > > 1 file changed, 18 insertions(+), 23 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c index > > > f698b3f45c6d..16d0e3df7de0 > > > 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -661,8 +661,12 @@ static void intel_psr_exit(struct > > > drm_i915_private > > > *dev_priv) { > > > u32 val; > > > > > > - if (!dev_priv->psr.active) > > > + if (!dev_priv->psr.active) { > > > + if (INTEL_GEN(dev_priv) >= 9) > > > + WARN_ON(I915_READ(EDP_PSR2_CTL) & > > > EDP_PSR2_ENABLE); > > > + WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE); > > > > Do we really need so many WARN_ONs needing MMIO reads? There are > > almost as > > many lines of warnings as code in this function. I think at this > > point, we > > know this code works and the warnings in psr_exit() should be > > removed. > > I'm just moving what intel_psr_disable_source() had but yes I think > we > can remove some WARN_ON() in intel_psr but I guess is better do in > another patch. > Please do so :) Reviewed-by: Dhinakaran Pandiyan > > > > > return; > > > + } > > > > > > if (dev_priv->psr.psr2_enabled) { > > > val = I915_READ(EDP_PSR2_CTL); > > > @@ -680,32 +684,23 @@ static void > > > intel_psr_disable_source(struct intel_dp *intel_dp) > > > { > > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > > + i915_reg_t psr_status; > > > + u32 psr_status_mask; > > > > > > - if (dev_priv->psr.active) { > > > - i915_reg_t psr_status; > > > - u32 psr_status_mask; > > > - > > > - intel_psr_exit(dev_priv); > > > + intel_psr_exit(dev_priv); > > > > > > - if (dev_priv->psr.psr2_enabled) { > > > - psr_status = EDP_PSR2_STATUS; > > > - psr_status_mask = EDP_PSR2_STATUS_STATE_MASK; > > > - } else { > > > - psr_status = EDP_PSR_STATUS; > > > - psr_status_mask = EDP_PSR_STATUS_STATE_MASK; > > > - } > > > - > > > - /* Wait till PSR is idle */ > > > - if (intel_wait_for_register(dev_priv, > > > - psr_status, > > > psr_status_mask, 0, > > > - 2000)) > > > - DRM_ERROR("Timed out waiting for PSR Idle > > > State\n"); > > > + if (dev_priv->psr.psr2_enabled) { > > > + psr_status = EDP_PSR2_STATUS; > > > + psr_status_mask = EDP_PSR2_STATUS_STATE_MASK; > > > } else { > > > - if (dev_priv->psr.psr2_enabled) > > > - WARN_ON(I915_READ(EDP_PSR2_CTL) & > > > EDP_PSR2_ENABLE); > > > - else > > > - WARN_ON(I915_READ(EDP_PSR_CTL) & > > > EDP_PSR_ENABLE); > > > + psr_status = EDP_PSR_STATUS; > > > + psr_status_mask = EDP_PSR_STATUS_STATE_MASK; > > > } > > > + > > > + /* Wait till PSR is idle */ > > > + if (intel_wait_for_register(dev_priv, psr_status, > > > psr_status_mask, 0, > > > + 2000)) > > > + DRM_ERROR("Timed out waiting for PSR Idle State\n"); > > > } > > > > > > static void intel_psr_disable_locked(struct intel_dp *intel_dp) > > > > > > > > ___ > 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
Re: [Intel-gfx] [PATCH v2 2/6] drm/i915/psr: Always wait for idle state when disabling PSR
On Fri, 2018-10-19 at 13:42 -0700, Dhinakaran Pandiyan wrote: > On Wednesday, October 10, 2018 5:41:20 PM PDT José Roberto de Souza > wrote: > > It should always wait for idle state when disabling PSR because PSR > > could be inactive due a call to intel_psr_exit() and while PSR is > > still being disabled asynchronously userspace could change the > > modeset causing a call to psr_disable() that will not wait for PSR > > idle and then PSR will be enabled again while PSR is still not > > idle. > > > > v2: rebased on top of the patch reusing psr_exit() > > > > Cc: Rodrigo Vivi > > Cc: Dhinakaran Pandiyan > > Signed-off-by: José Roberto de Souza > > --- > > drivers/gpu/drm/i915/intel_psr.c | 41 ++ > > -- > > 1 file changed, 18 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c index f698b3f45c6d..16d0e3df7de0 > > 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -661,8 +661,12 @@ static void intel_psr_exit(struct > > drm_i915_private > > *dev_priv) { > > u32 val; > > > > - if (!dev_priv->psr.active) > > + if (!dev_priv->psr.active) { > > + if (INTEL_GEN(dev_priv) >= 9) > > + WARN_ON(I915_READ(EDP_PSR2_CTL) & > > EDP_PSR2_ENABLE); > > + WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE); > > Do we really need so many WARN_ONs needing MMIO reads? There are > almost as > many lines of warnings as code in this function. I think at this > point, we > know this code works and the warnings in psr_exit() should be > removed. I'm just moving what intel_psr_disable_source() had but yes I think we can remove some WARN_ON() in intel_psr but I guess is better do in another patch. > > > return; > > + } > > > > if (dev_priv->psr.psr2_enabled) { > > val = I915_READ(EDP_PSR2_CTL); > > @@ -680,32 +684,23 @@ static void > > intel_psr_disable_source(struct intel_dp *intel_dp) > > { > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > + i915_reg_t psr_status; > > + u32 psr_status_mask; > > > > - if (dev_priv->psr.active) { > > - i915_reg_t psr_status; > > - u32 psr_status_mask; > > - > > - intel_psr_exit(dev_priv); > > + intel_psr_exit(dev_priv); > > > > - if (dev_priv->psr.psr2_enabled) { > > - psr_status = EDP_PSR2_STATUS; > > - psr_status_mask = EDP_PSR2_STATUS_STATE_MASK; > > - } else { > > - psr_status = EDP_PSR_STATUS; > > - psr_status_mask = EDP_PSR_STATUS_STATE_MASK; > > - } > > - > > - /* Wait till PSR is idle */ > > - if (intel_wait_for_register(dev_priv, > > - psr_status, > > psr_status_mask, 0, > > - 2000)) > > - DRM_ERROR("Timed out waiting for PSR Idle > > State\n"); > > + if (dev_priv->psr.psr2_enabled) { > > + psr_status = EDP_PSR2_STATUS; > > + psr_status_mask = EDP_PSR2_STATUS_STATE_MASK; > > } else { > > - if (dev_priv->psr.psr2_enabled) > > - WARN_ON(I915_READ(EDP_PSR2_CTL) & > > EDP_PSR2_ENABLE); > > - else > > - WARN_ON(I915_READ(EDP_PSR_CTL) & > > EDP_PSR_ENABLE); > > + psr_status = EDP_PSR_STATUS; > > + psr_status_mask = EDP_PSR_STATUS_STATE_MASK; > > } > > + > > + /* Wait till PSR is idle */ > > + if (intel_wait_for_register(dev_priv, psr_status, > > psr_status_mask, 0, > > + 2000)) > > + DRM_ERROR("Timed out waiting for PSR Idle State\n"); > > } > > > > static void intel_psr_disable_locked(struct intel_dp *intel_dp) > > > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 2/6] drm/i915/psr: Always wait for idle state when disabling PSR
On Fri, 2018-10-19 at 13:42 -0700, Dhinakaran Pandiyan wrote: > On Wednesday, October 10, 2018 5:41:20 PM PDT José Roberto de Souza > wrote: > > It should always wait for idle state when disabling PSR because PSR > > could be inactive due a call to intel_psr_exit() and while PSR is > > still being disabled asynchronously userspace could change the > > modeset causing a call to psr_disable() that will not wait for PSR > > idle and then PSR will be enabled again while PSR is still not > > idle. > > > > v2: rebased on top of the patch reusing psr_exit() > > > > Cc: Rodrigo Vivi > > Cc: Dhinakaran Pandiyan > > Signed-off-by: José Roberto de Souza > > --- > > drivers/gpu/drm/i915/intel_psr.c | 41 ++ > > -- > > 1 file changed, 18 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c index f698b3f45c6d..16d0e3df7de0 > > 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -661,8 +661,12 @@ static void intel_psr_exit(struct > > drm_i915_private > > *dev_priv) { > > u32 val; > > > > - if (!dev_priv->psr.active) > > + if (!dev_priv->psr.active) { > > + if (INTEL_GEN(dev_priv) >= 9) > > + WARN_ON(I915_READ(EDP_PSR2_CTL) & > > EDP_PSR2_ENABLE); > > + WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE); > > Do we really need so many WARN_ONs needing MMIO reads? There are > almost as > many lines of warnings as code in this function. I think at this > point, we > know this code works and the warnings in psr_exit() should be > removed. > > > return; > > + } > > > > if (dev_priv->psr.psr2_enabled) { > > val = I915_READ(EDP_PSR2_CTL); > > @@ -680,32 +684,23 @@ static void > > intel_psr_disable_source(struct intel_dp *intel_dp) > > { > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > + i915_reg_t psr_status; > > + u32 psr_status_mask; > > > > - if (dev_priv->psr.active) { > > - i915_reg_t psr_status; > > - u32 psr_status_mask; > > - > > - intel_psr_exit(dev_priv); > > + intel_psr_exit(dev_priv); > > > > - if (dev_priv->psr.psr2_enabled) { > > - psr_status = EDP_PSR2_STATUS; > > - psr_status_mask = EDP_PSR2_STATUS_STATE_MASK; > > - } else { > > - psr_status = EDP_PSR_STATUS; > > - psr_status_mask = EDP_PSR_STATUS_STATE_MASK; > > - } > > - > > - /* Wait till PSR is idle */ > > - if (intel_wait_for_register(dev_priv, > > - psr_status, > > psr_status_mask, 0, > > - 2000)) > > - DRM_ERROR("Timed out waiting for PSR Idle > > State\n"); > > + if (dev_priv->psr.psr2_enabled) { > > + psr_status = EDP_PSR2_STATUS; > > + psr_status_mask = EDP_PSR2_STATUS_STATE_MASK; > > } else { > > - if (dev_priv->psr.psr2_enabled) > > - WARN_ON(I915_READ(EDP_PSR2_CTL) & > > EDP_PSR2_ENABLE); > > - else > > - WARN_ON(I915_READ(EDP_PSR_CTL) & > > EDP_PSR_ENABLE); > > + psr_status = EDP_PSR_STATUS; > > + psr_status_mask = EDP_PSR_STATUS_STATE_MASK; > > } > > + > > + /* Wait till PSR is idle */ > > + if (intel_wait_for_register(dev_priv, psr_status, > > psr_status_mask, 0, > > + 2000)) > > + DRM_ERROR("Timed out waiting for PSR Idle State\n"); nitpick: Mind writing this as "Timed out waiting PSR idle state\n" if you end up sending another version without the warnings? > > } > > > > static void intel_psr_disable_locked(struct intel_dp *intel_dp) > > > > > ___ > 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
Re: [Intel-gfx] [PATCH v2 2/6] drm/i915/psr: Always wait for idle state when disabling PSR
On Wednesday, October 10, 2018 5:41:20 PM PDT José Roberto de Souza wrote: > It should always wait for idle state when disabling PSR because PSR > could be inactive due a call to intel_psr_exit() and while PSR is > still being disabled asynchronously userspace could change the > modeset causing a call to psr_disable() that will not wait for PSR > idle and then PSR will be enabled again while PSR is still not idle. > > v2: rebased on top of the patch reusing psr_exit() > > Cc: Rodrigo Vivi > Cc: Dhinakaran Pandiyan > Signed-off-by: José Roberto de Souza > --- > drivers/gpu/drm/i915/intel_psr.c | 41 ++-- > 1 file changed, 18 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > b/drivers/gpu/drm/i915/intel_psr.c index f698b3f45c6d..16d0e3df7de0 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -661,8 +661,12 @@ static void intel_psr_exit(struct drm_i915_private > *dev_priv) { > u32 val; > > - if (!dev_priv->psr.active) > + if (!dev_priv->psr.active) { > + if (INTEL_GEN(dev_priv) >= 9) > + WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE); > + WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE); Do we really need so many WARN_ONs needing MMIO reads? There are almost as many lines of warnings as code in this function. I think at this point, we know this code works and the warnings in psr_exit() should be removed. > return; > + } > > if (dev_priv->psr.psr2_enabled) { > val = I915_READ(EDP_PSR2_CTL); > @@ -680,32 +684,23 @@ static void > intel_psr_disable_source(struct intel_dp *intel_dp) > { > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > + i915_reg_t psr_status; > + u32 psr_status_mask; > > - if (dev_priv->psr.active) { > - i915_reg_t psr_status; > - u32 psr_status_mask; > - > - intel_psr_exit(dev_priv); > + intel_psr_exit(dev_priv); > > - if (dev_priv->psr.psr2_enabled) { > - psr_status = EDP_PSR2_STATUS; > - psr_status_mask = EDP_PSR2_STATUS_STATE_MASK; > - } else { > - psr_status = EDP_PSR_STATUS; > - psr_status_mask = EDP_PSR_STATUS_STATE_MASK; > - } > - > - /* Wait till PSR is idle */ > - if (intel_wait_for_register(dev_priv, > - psr_status, psr_status_mask, 0, > - 2000)) > - DRM_ERROR("Timed out waiting for PSR Idle State\n"); > + if (dev_priv->psr.psr2_enabled) { > + psr_status = EDP_PSR2_STATUS; > + psr_status_mask = EDP_PSR2_STATUS_STATE_MASK; > } else { > - if (dev_priv->psr.psr2_enabled) > - WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE); > - else > - WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE); > + psr_status = EDP_PSR_STATUS; > + psr_status_mask = EDP_PSR_STATUS_STATE_MASK; > } > + > + /* Wait till PSR is idle */ > + if (intel_wait_for_register(dev_priv, psr_status, psr_status_mask, 0, > + 2000)) > + DRM_ERROR("Timed out waiting for PSR Idle State\n"); > } > > static void intel_psr_disable_locked(struct intel_dp *intel_dp) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx