From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ramalingam C Subject: Re: [PATCH 2/10] drm/i915: Initialize DRRS delayed work Date: Wed, 21 Jan 2015 16:34:11 +0530 Message-ID: <54BF87AB.80709@intel.com> References: <1420836965-10068-1-git-send-email-vandana.kannan@intel.com> <1420836965-10068-3-git-send-email-vandana.kannan@intel.com> <20150111125208.GB27134@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0767903559==" Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id BB9B46E6FC for ; Wed, 21 Jan 2015 03:09:11 -0800 (PST) In-Reply-To: <20150111125208.GB27134@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson , Vandana Kannan , intel-gfx@lists.freedesktop.org, paulo.r.zanoni@intel.com, rodrigo.vivi@intel.com List-Id: intel-gfx@lists.freedesktop.org This is a multi-part message in MIME format. --===============0767903559== Content-Type: multipart/alternative; boundary="------------020401080907070809090900" This is a multi-part message in MIME format. --------------020401080907070809090900 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Hi chris On Sunday 11 January 2015 06:22 PM, Chris Wilson wrote: > On Sat, Jan 10, 2015 at 02:25:57AM +0530, Vandana Kannan wrote: >> Add DRRS work function to trigger a switch to low refresh rate when activity >> is detected on screen. > Where is this function used? How can I judge that it does the right > thing? Thanks for catching this. There is an error in the commit message. This DRRS work function will trigger a switch to low refresh rate, when there is no activity on the screen for more than 1 sec. And this function is set as a deferred work from intel_edp_drrs_flush(). Functionality of this function can be verified from the debug logs in dmesg (lower refresh rate set will be printed out). Addition to that I am working to enable a debugfs to share the refreshrate switch info also for the debugging/testing purpose. > >> Signed-off-by: Vandana Kannan >> --- >> drivers/gpu/drm/i915/intel_dp.c | 36 ++++++++++++++++++++++++++++-------- >> 1 file changed, 28 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 778dcd0..30b3aa1 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -4814,20 +4814,38 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate) >> I915_WRITE(reg, val); >> } >> >> + dev_priv->drrs.refresh_rate_type = index; >> + >> + DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate); >> +} >> + >> +static void intel_edp_drrs_work(struct work_struct *work) > intel_edp_drrs_downclock_work() would be more self-descriptive Agreed. I will rename it in next iteration > >> +{ >> + struct drm_i915_private *dev_priv = >> + container_of(work, typeof(*dev_priv), drrs.work.work); >> + struct intel_dp *intel_dp = dev_priv->drrs.dp; >> + >> + mutex_lock(&dev_priv->drrs.mutex); >> + >> + if (!intel_dp) >> + goto unlock; > Does dev_priv->drrs.mutex not also protect dev_priv->drrs.dp? It should have protected. Will cover drrs.dp with drrs.mutex in next patch >> + >> /* >> - * mutex taken to ensure that there is no race between differnt >> - * drrs calls trying to update refresh rate. This scenario may occur >> - * in future when idleness detection based DRRS in kernel and >> - * possible calls from user space to set differnt RR are made. >> + * The delayed work can race with an invalidate hence we need to >> + * recheck. >> */ > This comment no longer applies to all the other callers of > intel_dp_set_drrs_state()? Or did you miss adding the > lockdep_assert_held(&dev_priv->drrs.mutex)? This comment was added considering the requests from userspace for new refreshrates. But a part of MIPI DRRS and media playback DRRS implementation (currently in development), I am addressing the possible race condition. So at this point in time this comment is irrelevant, hence vandana removed it. > >> - mutex_lock(&dev_priv->drrs.mutex); >> + if (dev_priv->drrs.busy_frontbuffer_bits) >> + goto unlock; >> >> - dev_priv->drrs.refresh_rate_type = index; >> + if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR) >> + intel_dp_set_drrs_state(dev_priv->dev, > Would it not be sensible for intel_dp_set_drrs_state() check for the > no-op itself? If refresh_rate_type is already LOW_RR then we should exit the work function with no call to intel_dp_set_drrs_state(). Thats the reason the call is kept under the if condition. intel_dp_set_drrs_state() already handles if the requested vrefresh is same as the vrefresh of the current refresh_rate type. > >> + intel_dp->attached_connector->panel. >> + downclock_mode->vrefresh); > -Chris > -Ram --------------020401080907070809090900 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 7bit Hi chris
On Sunday 11 January 2015 06:22 PM, Chris Wilson wrote:
On Sat, Jan 10, 2015 at 02:25:57AM +0530, Vandana Kannan wrote:

Add DRRS work function to trigger a switch to low refresh rate when activity
is detected on screen.

Where is this function used? How can I judge that it does the right
thing?
Thanks for catching this. There is an error in the commit message. This DRRS work function
will trigger a switch to low refresh rate, when there is no activity on the screen for more than 1 sec.
And this function is set as a deferred work from intel_edp_drrs_flush().
Functionality of this function can be verified from the debug logs in dmesg (lower refresh rate set
will be printed out). Addition to that I am working to enable a debugfs to share the refreshrate
switch info also for the debugging/testing purpose.


Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 778dcd0..30b3aa1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4814,20 +4814,38 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 		I915_WRITE(reg, val);
 	}
 
+	dev_priv->drrs.refresh_rate_type = index;
+
+	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
+}
+
+static void intel_edp_drrs_work(struct work_struct *work)

intel_edp_drrs_downclock_work() would be more self-descriptive
Agreed. I will rename it in next iteration


+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, typeof(*dev_priv), drrs.work.work);
+	struct intel_dp *intel_dp = dev_priv->drrs.dp;
+
+	mutex_lock(&dev_priv->drrs.mutex);
+
+	if (!intel_dp)
+		goto unlock;

Does dev_priv->drrs.mutex not also protect dev_priv->drrs.dp?
It should have protected. Will cover drrs.dp with drrs.mutex in next patch

      

        
+
 	/*
-	 * mutex taken to ensure that there is no race between differnt
-	 * drrs calls trying to update refresh rate. This scenario may occur
-	 * in future when idleness detection based DRRS in kernel and
-	 * possible calls from user space to set differnt RR are made.
+	 * The delayed work can race with an invalidate hence we need to
+	 * recheck.
 	 */


      

      
This comment no longer applies to all the other callers of
intel_dp_set_drrs_state()? Or did you miss adding the
lockdep_assert_held(&dev_priv->drrs.mutex)?

    

    This comment was added considering the requests from
      userspace for new refreshrates.

      But a part of MIPI DRRS and media playback DRRS implementation
      (currently in development),

      I am addressing the possible race condition. So at
      this point in time this comment is irrelevant,

      hence vandana removed it.

    

      



      

        
-	mutex_lock(&dev_priv->drrs.mutex);
+	if (dev_priv->drrs.busy_frontbuffer_bits)
+		goto unlock;
 
-	dev_priv->drrs.refresh_rate_type = index;
+	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR)
+		intel_dp_set_drrs_state(dev_priv->dev,


      

      
Would it not be sensible for intel_dp_set_drrs_state() check for the
no-op itself?

    

    If refresh_rate_type is already LOW_RR then we should exit
      the work function with no call to intel_dp_set_drrs_state().

      Thats the reason the call is kept under the if condition.
      intel_dp_set_drrs_state() already handles if the

      requested vrefresh is same as the vrefresh of the current
      refresh_rate type.

    

      



      

        
+			intel_dp->attached_connector->panel.
+			downclock_mode->vrefresh);


      

      
-Chris



    

    -Ram

  


--------------020401080907070809090900--

--===============0767903559==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline

X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4
IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0
cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK

--===============0767903559==--