Re: [PATCH v2 1/2] drm/i915: Runtime disable for eDP DRRS

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Nov 07, 2017 at 06:38:23PM +0000, Ramalingam C wrote:
> From: "C, Ramalingam" <ramalingam.c@intel.com>
> 
> Debugfs called i915_drrs_ctl is added to enable and disable the
> eDP DRRS. Writing 0 will disable the feature, whereas non-zero
> will enable the feature.
> 
> Possibility of disabling the DRRS, enables the testing of the
> frontbuffer tracking based features (FBC, DRRS and PSR) as
> standalone or any combination of the set.
> 
> [v2]: ctl interface is moved from module parameter to debugfs [Rodrigo]

Thanks

> 
> Signed-off-by: C, Ramalingam <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 43 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 0bb6e01121fc..0c1501fe4c9f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4747,6 +4747,46 @@ static const struct file_operations i915_hpd_storm_ctl_fops = {
>  	.write = i915_hpd_storm_ctl_write
>  };
>  
> +static int i915_drrs_ctl_set(void *data, u64 val)
> +{
> +	struct drm_i915_private *dev_priv = data;
> +	struct drm_device *dev = &dev_priv->drm;
> +	struct intel_crtc *intel_crtc;
> +	struct intel_encoder *encoder;
> +	struct intel_dp *intel_dp;
> +
> +	if (INTEL_GEN(dev_priv) < 7)

I believe we need to define a HAS_DRRS(dev_priv)
which based on what is on intel_cpu_transcoder_set_m_n would be
(IS_CHERRYVIEW(dev_priv) || INTEL_GEN(dev_priv) < 8)

> +		return -ENODEV;
> +
> +	drm_modeset_lock_all(dev);

my first reaction to this was: "why do you need to lock all modeset?!"
But this simplify a lot the logic here...
This assure that drrs is really not getting changed from other places.

> +	for_each_intel_crtc(dev, intel_crtc) {
> +		if (!intel_crtc->base.state->active ||
> +					!intel_crtc->config->has_drrs)

I was going to say that this check already happens inside
enable and disable functions... But I see the reason why to
check before the unecessary noise.

> +			continue;
> +
> +		for_each_encoder_on_crtc(dev, &intel_crtc->base, encoder) {
> +			if (encoder->type != INTEL_OUTPUT_EDP)
> +				continue;
> +
> +			DRM_DEBUG_DRIVER("Manually %sabling DRRS. %llu\n",
> +						val ? "en" : "dis", val);
> +
> +			intel_dp = enc_to_intel_dp(&encoder->base);
> +			if (val)
> +				intel_edp_drrs_enable(intel_dp,
> +							intel_crtc->config);
> +			else
> +				intel_edp_drrs_disable(intel_dp,
> +							intel_crtc->config);
> +		}
> +	}
> +	drm_modeset_unlock_all(dev);
> +
> +	return 0;
> +}

It seems simple and effective. Simpler than I imagined...
My only question is about that HAS_DRRS and when to skip here...

> +
> +DEFINE_SIMPLE_ATTRIBUTE(i915_drrs_ctl_fops, NULL, i915_drrs_ctl_set, "%llu\n");
> +
>  static const struct drm_info_list i915_debugfs_list[] = {
>  	{"i915_capabilities", i915_capabilities, 0},
>  	{"i915_gem_objects", i915_gem_object_info, 0},
> @@ -4828,7 +4868,8 @@ static const struct i915_debugfs_files {
>  	{"i915_dp_test_active", &i915_displayport_test_active_fops},
>  	{"i915_guc_log_control", &i915_guc_log_control_fops},
>  	{"i915_hpd_storm_ctl", &i915_hpd_storm_ctl_fops},
> -	{"i915_ipc_status", &i915_ipc_status_fops}
> +	{"i915_ipc_status", &i915_ipc_status_fops},
> +	{"i915_drrs_ctl", &i915_drrs_ctl_fops}
>  };
>  
>  int i915_debugfs_register(struct drm_i915_private *dev_priv)
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx