Thanks for reviewing these changes Rodrigo.
> -----Original Message-----
> From: Vivi, Rodrigo
> Sent: Saturday, November 18, 2017 12:24 AM
> To: C, Ramalingam <ramalingam.c@xxxxxxxxx>
> Cc: Zanoni, Paulo R <paulo.r.zanoni@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
