| Message ID | 1418244777-4964-9-git-send-email-vandana.kannan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 364/364 364/364
ILK +1-3 364/366 362/366
SNB 448/450 448/450
IVB 497/498 497/498
BYT 289/289 289/289
HSW 563/564 563/564
BDW 417/417 417/417
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*ILK igt_kms_render_direct-render PASS(7, M26) DMESG_WARN(1, M26)
ILK igt_kms_flip_blocking-absolute-wf_vblank-interruptible DMESG_WARN(1, M26)PASS(7, M26) DMESG_WARN(1, M26)
ILK igt_kms_flip_vblank-vs-hang DMESG_WARN(2, M26)PASS(2, M26) DMESG_WARN(1, M26)
ILK igt_kms_flip_wf_vblank-ts-check DMESG_WARN(6, M26)PASS(20, M26M37) PASS(1, M26)
Note: You need to pay more attention to line start with '*'
On Thu, Dec 11, 2014 at 02:22:57AM +0530, Vandana Kannan wrote: > Adding i915 module parameter for setting drrs_interval. If this param is > set to 0, then drrs is disabled. If changed in runtime, then the new interval > value will be considered for scheduling the next drrs work. > drrs_interval is set to 0 by default, i.e. DRRS is disabled by default. Nope, please don't hide power saving features behind module options by default. New stuff must be enabled by default, otherwise it'll bitrot and merging to upstream is fairly useless since we still have the rebase pain (just spread out over more people) with none of the testing. Also such debug options need to be marked using the _debug variants to make sure that people report issues and don't keep using them. Now talking about validation gets me to the next point: Do we have a basic igt to make sure DRRS doesn't break right after merging? I don't think we need the full-blown test setup like for psr/fbc, but a basic test to make sure it enters/exit RR mode should be there. -Daniel > > Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_params.c | 8 ++++++++ > drivers/gpu/drm/i915/intel_dp.c | 11 ++++++++++- > 3 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 370cbaa..dba5844 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2381,6 +2381,7 @@ struct i915_params { > int enable_ips; > int invert_brightness; > int enable_cmd_parser; > + int drrs_interval; > /* leave bools at the end to not create holes */ > bool enable_hangcheck; > bool fastboot; > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index c91cb20..80492e8 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -51,6 +51,7 @@ struct i915_params i915 __read_mostly = { > .disable_vtd_wa = 0, > .use_mmio_flip = 0, > .mmio_debug = 0, > + .drrs_interval = 0, > }; > > module_param_named(modeset, i915.modeset, int, 0400); > @@ -173,3 +174,10 @@ module_param_named(mmio_debug, i915.mmio_debug, bool, 0600); > MODULE_PARM_DESC(mmio_debug, > "Enable the MMIO debug code (default: false). This may negatively " > "affect performance."); > + > +module_param_named(drrs_interval, i915.drrs_interval, int, 0600); > +MODULE_PARM_DESC(drrs_interval, > + "DRRS idleness detection interval (default: 0 ms). " > + "If this field is set to 0, then seamless DRRS feature " > + "based on idleness detection is disabled. " > + "The interval is to be set in milliseconds."); > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 092ef91..88f46906 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4931,6 +4931,9 @@ void intel_edp_drrs_invalidate(struct drm_device *dev, > if (!dev_priv->drrs.dp) > return; > > + if (i915.drrs_interval == 0) > + return; > + > mutex_lock(&dev_priv->drrs.mutex); > crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc; > pipe = to_intel_crtc(crtc)->pipe; > @@ -4958,6 +4961,9 @@ void intel_edp_drrs_flush(struct drm_device *dev, > if (!dev_priv->drrs.dp) > return; > > + if (i915.drrs_interval == 0) > + return; > + > mutex_lock(&dev_priv->drrs.mutex); > crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc; > pipe = to_intel_crtc(crtc)->pipe; > @@ -4968,7 +4974,7 @@ void intel_edp_drrs_flush(struct drm_device *dev, > if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR && > !dev_priv->drrs.busy_frontbuffer_bits) > schedule_delayed_work(&dev_priv->drrs.work, > - msecs_to_jiffies(1000)); > + msecs_to_jiffies(i915.drrs_interval)); > mutex_unlock(&dev_priv->drrs.mutex); > } > > @@ -4991,6 +4997,9 @@ intel_dp_drrs_init(struct intel_connector *intel_connector, > return NULL; > } > > + if (i915.drrs_interval == 0) > + DRM_DEBUG_KMS("DRRS disable by flag\n"); > + > downclock_mode = intel_find_panel_downclock > (dev, fixed_mode, connector); > > -- > 2.0.1 >
On 15-Dec-14 3:17 PM, Daniel Vetter wrote: > On Thu, Dec 11, 2014 at 02:22:57AM +0530, Vandana Kannan wrote: >> Adding i915 module parameter for setting drrs_interval. If this param is >> set to 0, then drrs is disabled. If changed in runtime, then the new interval >> value will be considered for scheduling the next drrs work. >> drrs_interval is set to 0 by default, i.e. DRRS is disabled by default. > > Nope, please don't hide power saving features behind module options by > default. New stuff must be enabled by default, otherwise it'll bitrot and > merging to upstream is fairly useless since we still have the rebase pain > (just spread out over more people) with none of the testing. ok.. so, shall I just make the delay (drrs_interval) fixed at 1 second or let user set this delay at runtime through the same module param (excluding the disable feature if interval is 0 part) ? > > Also such debug options need to be marked using the _debug variants to > make sure that people report issues and don't keep using them. > > Now talking about validation gets me to the next point: Do we have a basic > igt to make sure DRRS doesn't break right after merging? I don't think we > need the full-blown test setup like for psr/fbc, but a basic test to make > sure it enters/exit RR mode should be there. libdrm has vbltest which prints refresh rate.. Apart from this, I'm adding downclock mode (if supported) in kms_flip. Shall I add a test which involves low RR trigger after idle time and then forces some activity on screen and switches back to high RR ? - Vandana > -Daniel > >> >> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/i915_params.c | 8 ++++++++ >> drivers/gpu/drm/i915/intel_dp.c | 11 ++++++++++- >> 3 files changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 370cbaa..dba5844 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2381,6 +2381,7 @@ struct i915_params { >> int enable_ips; >> int invert_brightness; >> int enable_cmd_parser; >> + int drrs_interval; >> /* leave bools at the end to not create holes */ >> bool enable_hangcheck; >> bool fastboot; >> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c >> index c91cb20..80492e8 100644 >> --- a/drivers/gpu/drm/i915/i915_params.c >> +++ b/drivers/gpu/drm/i915/i915_params.c >> @@ -51,6 +51,7 @@ struct i915_params i915 __read_mostly = { >> .disable_vtd_wa = 0, >> .use_mmio_flip = 0, >> .mmio_debug = 0, >> + .drrs_interval = 0, >> }; >> >> module_param_named(modeset, i915.modeset, int, 0400); >> @@ -173,3 +174,10 @@ module_param_named(mmio_debug, i915.mmio_debug, bool, 0600); >> MODULE_PARM_DESC(mmio_debug, >> "Enable the MMIO debug code (default: false). This may negatively " >> "affect performance."); >> + >> +module_param_named(drrs_interval, i915.drrs_interval, int, 0600); >> +MODULE_PARM_DESC(drrs_interval, >> + "DRRS idleness detection interval (default: 0 ms). " >> + "If this field is set to 0, then seamless DRRS feature " >> + "based on idleness detection is disabled. " >> + "The interval is to be set in milliseconds."); >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 092ef91..88f46906 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -4931,6 +4931,9 @@ void intel_edp_drrs_invalidate(struct drm_device *dev, >> if (!dev_priv->drrs.dp) >> return; >> >> + if (i915.drrs_interval == 0) >> + return; >> + >> mutex_lock(&dev_priv->drrs.mutex); >> crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc; >> pipe = to_intel_crtc(crtc)->pipe; >> @@ -4958,6 +4961,9 @@ void intel_edp_drrs_flush(struct drm_device *dev, >> if (!dev_priv->drrs.dp) >> return; >> >> + if (i915.drrs_interval == 0) >> + return; >> + >> mutex_lock(&dev_priv->drrs.mutex); >> crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc; >> pipe = to_intel_crtc(crtc)->pipe; >> @@ -4968,7 +4974,7 @@ void intel_edp_drrs_flush(struct drm_device *dev, >> if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR && >> !dev_priv->drrs.busy_frontbuffer_bits) >> schedule_delayed_work(&dev_priv->drrs.work, >> - msecs_to_jiffies(1000)); >> + msecs_to_jiffies(i915.drrs_interval)); >> mutex_unlock(&dev_priv->drrs.mutex); >> } >> >> @@ -4991,6 +4997,9 @@ intel_dp_drrs_init(struct intel_connector *intel_connector, >> return NULL; >> } >> >> + if (i915.drrs_interval == 0) >> + DRM_DEBUG_KMS("DRRS disable by flag\n"); >> + >> downclock_mode = intel_find_panel_downclock >> (dev, fixed_mode, connector); >> >> -- >> 2.0.1 >> >
On Mon, Dec 15, 2014 at 04:25:32PM +0530, Kannan, Vandana wrote: > > > On 15-Dec-14 3:17 PM, Daniel Vetter wrote: > >On Thu, Dec 11, 2014 at 02:22:57AM +0530, Vandana Kannan wrote: > >>Adding i915 module parameter for setting drrs_interval. If this param is > >>set to 0, then drrs is disabled. If changed in runtime, then the new interval > >>value will be considered for scheduling the next drrs work. > >>drrs_interval is set to 0 by default, i.e. DRRS is disabled by default. > > > >Nope, please don't hide power saving features behind module options by > >default. New stuff must be enabled by default, otherwise it'll bitrot and > >merging to upstream is fairly useless since we still have the rebase pain > >(just spread out over more people) with none of the testing. > ok.. so, shall I just make the delay (drrs_interval) fixed at 1 second > or let user set this delay at runtime through the same module param > (excluding the disable feature if interval is 0 part) ? Imo we should just set an optimal value (does vbt have any hints?). > >Also such debug options need to be marked using the _debug variants to > >make sure that people report issues and don't keep using them. > > > >Now talking about validation gets me to the next point: Do we have a basic > >igt to make sure DRRS doesn't break right after merging? I don't think we > >need the full-blown test setup like for psr/fbc, but a basic test to make > >sure it enters/exit RR mode should be there. > libdrm has vbltest which prints refresh rate.. > Apart from this, I'm adding downclock mode (if supported) in kms_flip. > Shall I add a test which involves low RR trigger after idle time and then > forces some activity on screen and switches back to high RR ? kms_flip is already huge, probably better to start a new kms_drrs testcase, similar to kms_fbc_crc (but without crc checks since that's not ineteresting here). And yeah you should trigger RR entry and then check in debugfs that it has indeed happened, then pageflip and check that we're out of RR again. That should be enough to have a decent smoketest for DRRS. We can add anything missing later on. -Daniel
On 15-Dec-14 7:46 PM, Daniel Vetter wrote: > On Mon, Dec 15, 2014 at 04:25:32PM +0530, Kannan, Vandana wrote: >> >> >> On 15-Dec-14 3:17 PM, Daniel Vetter wrote: >>> On Thu, Dec 11, 2014 at 02:22:57AM +0530, Vandana Kannan wrote: >>>> Adding i915 module parameter for setting drrs_interval. If this param is >>>> set to 0, then drrs is disabled. If changed in runtime, then the new interval >>>> value will be considered for scheduling the next drrs work. >>>> drrs_interval is set to 0 by default, i.e. DRRS is disabled by default. >>> >>> Nope, please don't hide power saving features behind module options by >>> default. New stuff must be enabled by default, otherwise it'll bitrot and >>> merging to upstream is fairly useless since we still have the rebase pain >>> (just spread out over more people) with none of the testing. >> ok.. so, shall I just make the delay (drrs_interval) fixed at 1 second >> or let user set this delay at runtime through the same module param >> (excluding the disable feature if interval is 0 part) ? > > Imo we should just set an optimal value (does vbt have any hints?). > VBT does not contain a delay value.. Based on data collected from testing so far, 1 second seems stable.. Maybe it can go down to 800ms or so - I can test it out.. Anything as low as 100ms just triggered too many RR switches back and forth. - Vandana >>> Also such debug options need to be marked using the _debug variants to >>> make sure that people report issues and don't keep using them. >>> >>> Now talking about validation gets me to the next point: Do we have a basic >>> igt to make sure DRRS doesn't break right after merging? I don't think we >>> need the full-blown test setup like for psr/fbc, but a basic test to make >>> sure it enters/exit RR mode should be there. >> libdrm has vbltest which prints refresh rate.. >> Apart from this, I'm adding downclock mode (if supported) in kms_flip. >> Shall I add a test which involves low RR trigger after idle time and then >> forces some activity on screen and switches back to high RR ? > > kms_flip is already huge, probably better to start a new kms_drrs > testcase, similar to kms_fbc_crc (but without crc checks since that's not > ineteresting here). And yeah you should trigger RR entry and then check in > debugfs that it has indeed happened, then pageflip and check that we're > out of RR again. That should be enough to have a decent smoketest for > DRRS. We can add anything missing later on. > -Daniel >
On Mon, Dec 15, 2014 at 07:56:08PM +0530, Kannan, Vandana wrote: > > > On 15-Dec-14 7:46 PM, Daniel Vetter wrote: > >On Mon, Dec 15, 2014 at 04:25:32PM +0530, Kannan, Vandana wrote: > >> > >> > >>On 15-Dec-14 3:17 PM, Daniel Vetter wrote: > >>>On Thu, Dec 11, 2014 at 02:22:57AM +0530, Vandana Kannan wrote: > >>>>Adding i915 module parameter for setting drrs_interval. If this param is > >>>>set to 0, then drrs is disabled. If changed in runtime, then the new interval > >>>>value will be considered for scheduling the next drrs work. > >>>>drrs_interval is set to 0 by default, i.e. DRRS is disabled by default. > >>> > >>>Nope, please don't hide power saving features behind module options by > >>>default. New stuff must be enabled by default, otherwise it'll bitrot and > >>>merging to upstream is fairly useless since we still have the rebase pain > >>>(just spread out over more people) with none of the testing. > >>ok.. so, shall I just make the delay (drrs_interval) fixed at 1 second > >>or let user set this delay at runtime through the same module param > >>(excluding the disable feature if interval is 0 part) ? > > > >Imo we should just set an optimal value (does vbt have any hints?). > > > VBT does not contain a delay value.. > Based on data collected from testing so far, 1 second seems stable.. > Maybe it can go down to 800ms or so - I can test it out.. > Anything as low as 100ms just triggered too many RR switches back and forth. There's not need imo to go to the lowest possible value, there's always a tradeoff. 1s sounds good (psr has even worse timeout on some panels ...). -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 370cbaa..dba5844 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2381,6 +2381,7 @@ struct i915_params { int enable_ips; int invert_brightness; int enable_cmd_parser; + int drrs_interval; /* leave bools at the end to not create holes */ bool enable_hangcheck; bool fastboot; diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index c91cb20..80492e8 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -51,6 +51,7 @@ struct i915_params i915 __read_mostly = { .disable_vtd_wa = 0, .use_mmio_flip = 0, .mmio_debug = 0, + .drrs_interval = 0, }; module_param_named(modeset, i915.modeset, int, 0400); @@ -173,3 +174,10 @@ module_param_named(mmio_debug, i915.mmio_debug, bool, 0600); MODULE_PARM_DESC(mmio_debug, "Enable the MMIO debug code (default: false). This may negatively " "affect performance."); + +module_param_named(drrs_interval, i915.drrs_interval, int, 0600); +MODULE_PARM_DESC(drrs_interval, + "DRRS idleness detection interval (default: 0 ms). " + "If this field is set to 0, then seamless DRRS feature " + "based on idleness detection is disabled. " + "The interval is to be set in milliseconds."); diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 092ef91..88f46906 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4931,6 +4931,9 @@ void intel_edp_drrs_invalidate(struct drm_device *dev, if (!dev_priv->drrs.dp) return; + if (i915.drrs_interval == 0) + return; + mutex_lock(&dev_priv->drrs.mutex); crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc; pipe = to_intel_crtc(crtc)->pipe; @@ -4958,6 +4961,9 @@ void intel_edp_drrs_flush(struct drm_device *dev, if (!dev_priv->drrs.dp) return; + if (i915.drrs_interval == 0) + return; + mutex_lock(&dev_priv->drrs.mutex); crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc; pipe = to_intel_crtc(crtc)->pipe; @@ -4968,7 +4974,7 @@ void intel_edp_drrs_flush(struct drm_device *dev, if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR && !dev_priv->drrs.busy_frontbuffer_bits) schedule_delayed_work(&dev_priv->drrs.work, - msecs_to_jiffies(1000)); + msecs_to_jiffies(i915.drrs_interval)); mutex_unlock(&dev_priv->drrs.mutex); } @@ -4991,6 +4997,9 @@ intel_dp_drrs_init(struct intel_connector *intel_connector, return NULL; } + if (i915.drrs_interval == 0) + DRM_DEBUG_KMS("DRRS disable by flag\n"); + downclock_mode = intel_find_panel_downclock (dev, fixed_mode, connector);
Adding i915 module parameter for setting drrs_interval. If this param is set to 0, then drrs is disabled. If changed in runtime, then the new interval value will be considered for scheduling the next drrs work. drrs_interval is set to 0 by default, i.e. DRRS is disabled by default. Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_params.c | 8 ++++++++ drivers/gpu/drm/i915/intel_dp.c | 11 ++++++++++- 3 files changed, 19 insertions(+), 1 deletion(-)