Re: [PATCH 2/10] drm/i915: Initialize DRRS delayed work

[Ramalingam C <ramalingam.c@xxxxxxxxx>]

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@xxxxxxxxx>
---
 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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx