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