| Message ID | 1415983961-4485-9-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>-----Original Message----- >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Rodrigo Vivi >Sent: Friday, November 14, 2014 10:23 PM >To: intel-gfx@lists.freedesktop.org >Cc: Vivi, Rodrigo >Subject: [Intel-gfx] [PATCH 09/15] drm/i915: Fix intel_psr_is_enabled function and document it. > >This function can be used to check if psr feature got enabled. >However on HSW and BDW we currently force psr exit by disabling >EDP_PSR_ENABLE bit at EDP_PSR_CTL(dev). So this function was actually >returning the active/inactive state that is different from the enable/disable >meaning and had the risk of false negative. > >So let's just return the presence of intel_dp at dev_priv->psr.enabled. > >It would be more easy to just check this presence directly but let's keep >it more organized. Agreed, but shouldn't we protect it using psr.lock ? Thanks, Durga > >Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >--- > drivers/gpu/drm/i915/intel_psr.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > >diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c >index 66d24c2..c296a89 100644 >--- a/drivers/gpu/drm/i915/intel_psr.c >+++ b/drivers/gpu/drm/i915/intel_psr.c >@@ -61,6 +61,16 @@ static bool is_edp_psr(struct intel_dp *intel_dp) > return intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED; > } > >+/** >+ * intel_psr_is_enabled - Is PSR enabled? >+ * @dev: DRM Device >+ * >+ * This function can be used to verify if PSR feature is enabled. >+ * Since on Haswell+ we force the exit by disabling >+ * EDP_PSR_ENABLE bit at EDP_PSR_CTL(dev) >+ * the most reliable way to export if psr feature is enabled is to >+ * check the presence of intel_dp at dev_priv->psr.enabled. >+ */ > bool intel_psr_is_enabled(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; >@@ -68,7 +78,7 @@ bool intel_psr_is_enabled(struct drm_device *dev) > if (!HAS_PSR(dev)) > return false; > >- return I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; >+ return (bool)dev_priv->psr.enabled; > } > > static void intel_psr_write_vsc(struct intel_dp *intel_dp, >-- >1.9.3 > >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Nov 18, 2014 at 06:24:30PM +0000, R, Durgadoss wrote: > >-----Original Message----- > >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Rodrigo Vivi > >Sent: Friday, November 14, 2014 10:23 PM > >To: intel-gfx@lists.freedesktop.org > >Cc: Vivi, Rodrigo > >Subject: [Intel-gfx] [PATCH 09/15] drm/i915: Fix intel_psr_is_enabled function and document it. > > > >This function can be used to check if psr feature got enabled. > >However on HSW and BDW we currently force psr exit by disabling > >EDP_PSR_ENABLE bit at EDP_PSR_CTL(dev). So this function was actually > >returning the active/inactive state that is different from the enable/disable > >meaning and had the risk of false negative. > > > >So let's just return the presence of intel_dp at dev_priv->psr.enabled. > > > >It would be more easy to just check this presence directly but let's keep > >it more organized. > > Agreed, but shouldn't we protect it using psr.lock ? This function is used by DRRS and in a pretty crazy way. Sprinkling docs and locking over it won't make this any better, what we need is some state bits in the pipe config so that PSR and DRRS don't step on each anothers feet. This kind of stuff here doesn't really work as soon as you throw in the atomic check/commit semeantics we'll eventually have. If possible I'd vote to just drop this, DRRS isn't enabled by default currently anyway. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 66d24c2..c296a89 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -61,6 +61,16 @@ static bool is_edp_psr(struct intel_dp *intel_dp) return intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED; } +/** + * intel_psr_is_enabled - Is PSR enabled? + * @dev: DRM Device + * + * This function can be used to verify if PSR feature is enabled. + * Since on Haswell+ we force the exit by disabling + * EDP_PSR_ENABLE bit at EDP_PSR_CTL(dev) + * the most reliable way to export if psr feature is enabled is to + * check the presence of intel_dp at dev_priv->psr.enabled. + */ bool intel_psr_is_enabled(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -68,7 +78,7 @@ bool intel_psr_is_enabled(struct drm_device *dev) if (!HAS_PSR(dev)) return false; - return I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; + return (bool)dev_priv->psr.enabled; } static void intel_psr_write_vsc(struct intel_dp *intel_dp,
This function can be used to check if psr feature got enabled. However on HSW and BDW we currently force psr exit by disabling EDP_PSR_ENABLE bit at EDP_PSR_CTL(dev). So this function was actually returning the active/inactive state that is different from the enable/disable meaning and had the risk of false negative. So let's just return the presence of intel_dp at dev_priv->psr.enabled. It would be more easy to just check this presence directly but let's keep it more organized. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)