<html> <head> <meta content="text/html; charset=utf-8" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> Hi chris <div class="moz-cite-prefix">On Sunday 11 January 2015 06:22 PM, Chris Wilson wrote: </div> <blockquote cite="mid:20150111125208.GB27134@nuc-i3427.alporthouse.com" type="cite"> <pre wrap="">On Sat, Jan 10, 2015 at 02:25:57AM +0530, Vandana Kannan wrote: </pre> <blockquote type="cite"> <pre wrap="">Add DRRS work function to trigger a switch to low refresh rate when activity is detected on screen. </pre> </blockquote> <pre wrap=""> Where is this function used? How can I judge that it does the right thing?</pre> </blockquote> 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. <blockquote cite="mid:20150111125208.GB27134@nuc-i3427.alporthouse.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">Signed-off-by: Vandana Kannan <a class="moz-txt-link-rfc2396E" href="mailto:vandana.kannan@intel.com"><vandana.kannan@intel.com></a> --- 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) </pre> </blockquote> <pre wrap=""> intel_edp_drrs_downclock_work() would be more self-descriptive</pre> </blockquote> Agreed. I will rename it in next iteration <blockquote cite="mid:20150111125208.GB27134@nuc-i3427.alporthouse.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+{ + 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; </pre> </blockquote> <pre wrap=""> Does dev_priv->drrs.mutex not also protect dev_priv->drrs.dp? </pre> </blockquote> It should have protected. Will cover drrs.dp with drrs.mutex in next patch <blockquote cite="mid:20150111125208.GB27134@nuc-i3427.alporthouse.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ /* - * 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. */ </pre> </blockquote> <pre wrap=""> 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)?</pre> </blockquote> 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. <blockquote cite="mid:20150111125208.GB27134@nuc-i3427.alporthouse.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">- 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, </pre> </blockquote> <pre wrap=""> Would it not be sensible for intel_dp_set_drrs_state() check for the no-op itself?</pre> </blockquote> 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. <blockquote cite="mid:20150111125208.GB27134@nuc-i3427.alporthouse.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ intel_dp->attached_connector->panel. + downclock_mode->vrefresh); </pre> </blockquote> <pre wrap="">-Chris </pre> </blockquote> -Ram </body> </html>