[Intel-gfx] [PATCH 8/8] drm/i915: Add drrs_interval module parameter

[Kannan, Vandana]

On 15-Dec-14 3:17 PM, Daniel Vetter wrote:
> On Thu, Dec 11, 2014 at 02:22:57AM +0530, Vandana Kannan wrote:
>> Adding i915 module parameter for setting drrs_interval. If this param is
>> set to 0, then drrs is disabled. If changed in runtime, then the new interval
>> value will be considered for scheduling the next drrs work.
>> drrs_interval is set to 0 by default, i.e. DRRS is disabled by default.
>
> Nope, please don't hide power saving features behind module options by
> default. New stuff must be enabled by default, otherwise it'll bitrot and
> merging to upstream is fairly useless since we still have the rebase pain
> (just spread out over more people) with none of the testing.
ok.. so, shall I just make the delay (drrs_interval) fixed at 1 second
or let user set this delay at runtime through the same module param 
(excluding the disable feature if interval is 0 part) ?
>
> Also such debug options need to be marked using the _debug variants to
> make sure that people report issues and don't keep using them.
>
> Now talking about validation gets me to the next point: Do we have a basic
> igt to make sure DRRS doesn't break right after merging? I don't think we
> need the full-blown test setup like for psr/fbc, but a basic test to make
> sure it enters/exit RR mode should be there.
libdrm has vbltest which prints refresh rate..
Apart from this, I'm adding downclock mode (if supported) in kms_flip.
Shall I add a test which involves low RR trigger after idle time and 
then forces some activity on screen and switches back to high RR ?
- Vandana

> -Daniel
>
>>
>> Signed-off-by: Vandana Kannan <vandana.kannan at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h    |  1 +
>>   drivers/gpu/drm/i915/i915_params.c |  8 ++++++++
>>   drivers/gpu/drm/i915/intel_dp.c    | 11 ++++++++++-
>>   3 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 370cbaa..dba5844 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2381,6 +2381,7 @@ struct i915_params {
>>   	int enable_ips;
>>   	int invert_brightness;
>>   	int enable_cmd_parser;
>> +	int drrs_interval;
>>   	/* leave bools at the end to not create holes */
>>   	bool enable_hangcheck;
>>   	bool fastboot;
>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>> index c91cb20..80492e8 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -51,6 +51,7 @@ struct i915_params i915 __read_mostly = {
>>   	.disable_vtd_wa = 0,
>>   	.use_mmio_flip = 0,
>>   	.mmio_debug = 0,
>> +	.drrs_interval = 0,
>>   };
>>
>>   module_param_named(modeset, i915.modeset, int, 0400);
>> @@ -173,3 +174,10 @@ module_param_named(mmio_debug, i915.mmio_debug, bool, 0600);
>>   MODULE_PARM_DESC(mmio_debug,
>>   	"Enable the MMIO debug code (default: false). This may negatively "
>>   	"affect performance.");
>> +
>> +module_param_named(drrs_interval, i915.drrs_interval, int, 0600);
>> +MODULE_PARM_DESC(drrs_interval,
>> +	"DRRS idleness detection interval  (default: 0 ms). "
>> +	"If this field is set to 0, then seamless DRRS feature "
>> +	"based on idleness detection is disabled. "
>> +	"The interval is to be set in milliseconds.");
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 092ef91..88f46906 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4931,6 +4931,9 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
>>   	if (!dev_priv->drrs.dp)
>>   		return;
>>
>> +	if (i915.drrs_interval == 0)
>> +		return;
>> +
>>   	mutex_lock(&dev_priv->drrs.mutex);
>>   	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
>>   	pipe = to_intel_crtc(crtc)->pipe;
>> @@ -4958,6 +4961,9 @@ void intel_edp_drrs_flush(struct drm_device *dev,
>>   	if (!dev_priv->drrs.dp)
>>   		return;
>>
>> +	if (i915.drrs_interval == 0)
>> +		return;
>> +
>>   	mutex_lock(&dev_priv->drrs.mutex);
>>   	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
>>   	pipe = to_intel_crtc(crtc)->pipe;
>> @@ -4968,7 +4974,7 @@ void intel_edp_drrs_flush(struct drm_device *dev,
>>   	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR &&
>>   			!dev_priv->drrs.busy_frontbuffer_bits)
>>   		schedule_delayed_work(&dev_priv->drrs.work,
>> -				msecs_to_jiffies(1000));
>> +				msecs_to_jiffies(i915.drrs_interval));
>>   	mutex_unlock(&dev_priv->drrs.mutex);
>>   }
>>
>> @@ -4991,6 +4997,9 @@ intel_dp_drrs_init(struct intel_connector *intel_connector,
>>   		return NULL;
>>   	}
>>
>> +	if (i915.drrs_interval == 0)
>> +		DRM_DEBUG_KMS("DRRS disable by flag\n");
>> +
>>   	downclock_mode = intel_find_panel_downclock
>>   					(dev, fixed_mode, connector);
>>
>> --
>> 2.0.1
>>
>