| Message ID | 1510079903-29441-1-git-send-email-ramalingam.c@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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? -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
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
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) + return -ENODEV; + + drm_modeset_lock_all(dev); + for_each_intel_crtc(dev, intel_crtc) { + if (!intel_crtc->base.state->active || + !intel_crtc->config->has_drrs) + 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; +} + +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)