| Message ID | 1420836965-10068-3-git-send-email-vandana.kannan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > 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 > +{ > + 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? > + > /* > - * 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)? > - 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? > + intel_dp->attached_connector->panel. > + downclock_mode->vrefresh); -Chris
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
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) +{ + 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; + /* - * 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. */ - 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, + intel_dp->attached_connector->panel. + downclock_mode->vrefresh); - mutex_unlock(&dev_priv->drrs.mutex); +unlock: - DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate); + mutex_unlock(&dev_priv->drrs.mutex); } static struct drm_display_mode * @@ -4857,6 +4875,8 @@ intel_dp_drrs_init(struct intel_connector *intel_connector, return NULL; } + INIT_DELAYED_WORK(&dev_priv->drrs.work, intel_edp_drrs_work); + mutex_init(&dev_priv->drrs.mutex); dev_priv->drrs.type = dev_priv->vbt.drrs_type;
Add DRRS work function to trigger a switch to low refresh rate when activity is detected on screen. Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-)