Re: [PATCH 8/8] drm/i915: Add drrs_interval module parameter

[Daniel Vetter <daniel@ffwll.ch>]

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.

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.
-Daniel

> 
> Signed-off-by: Vandana Kannan <vandana.kannan@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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx