[Intel-gfx] [PATCH v2 1/2] drm/i915: Runtime disable for eDP DRRS

[C, Ramalingam]

Thanks for reviewing these changes Rodrigo.

> -----Original Message-----
> From: Vivi, Rodrigo
> Sent: Saturday, November 18, 2017 12:24 AM
> To: C, Ramalingam <ramalingam.c at intel.com>
> Cc: Zanoni, Paulo R <paulo.r.zanoni at intel.com>; intel-
> gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: Runtime disable for eDP DRRS
> 
> On Tue, Nov 07, 2017 at 06:38:23PM +0000, Ramalingam C wrote:
> > From: "C, Ramalingam" <ramalingam.c at 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 at 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)

Actually programming of pll divider m and n changes between platforms. Hence this check was there.
But all the platforms from Gen 7 has the DRRS support. Do we still need a macro for this check?

-Ram
> 
> > +		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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx