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

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

On Sun, Nov 19, 2017 at 02:55:06PM +0000, C, Ramalingam wrote:
> Thanks for reviewing these changes Rodrigo.
> 
> > -----Original Message-----
> > From: Vivi, Rodrigo
> > Sent: Saturday, November 18, 2017 12:24 AM
> > To: C, Ramalingam <ramalingam.c@intel.com>
> > Cc: Zanoni, Paulo R <paulo.r.zanoni@intel.com>; intel-
> > gfx@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@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)
> 
> 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?

Ohh! just now I noticed that this was actually "< 8" there...

Yeap, so no more concerns on this patch. Thanks for the explanation.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> -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@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