| Message ID | 20180328100526.36467-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 28 Mar 2018, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > Adding a i915_fifo_underrun_reset debugfs file will make it possible > for IGT tests to clear FIFO underrun fallout at the start of each > subtest, and make re-enable FBC so tests always have maximum exposure > to features used by IGT. FIFO underruns and FBC bugs will no longer > hide when an earlier subtests disables both. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105685 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105681 FWIW, ack on the idea, didn't look at the implementation. BR, Jani. > --- > drivers/gpu/drm/i915/i915_debugfs.c | 62 ++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++------- > drivers/gpu/drm/i915/intel_drv.h | 3 ++ > drivers/gpu/drm/i915/intel_fbc.c | 26 +++++++++++++++ > 4 files changed, 109 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 7816cd53100a..27188de2c32b 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -4731,6 +4731,67 @@ static int i915_drrs_ctl_set(void *data, u64 val) > > DEFINE_SIMPLE_ATTRIBUTE(i915_drrs_ctl_fops, NULL, i915_drrs_ctl_set, "%llu\n"); > > +static ssize_t > +i915_fifo_underrun_reset_write(struct file *filp, > + const char __user *ubuf, > + size_t cnt, loff_t *ppos) > +{ > + struct drm_i915_private *dev_priv = filp->private_data; > + struct intel_crtc *intel_crtc; > + struct drm_device *dev = &dev_priv->drm; > + int ret; > + bool reset; > + > + ret = kstrtobool_from_user(ubuf, cnt, &reset); > + if (ret) > + return ret; > + > + if (!reset) > + return cnt; > + > + for_each_intel_crtc(dev, intel_crtc) { > + struct drm_crtc_commit *commit; > + struct intel_crtc_state *crtc_state; > + > + ret = drm_modeset_lock_single_interruptible(&intel_crtc->base.mutex); > + if (ret) > + return ret; > + > + crtc_state = to_intel_crtc_state(intel_crtc->base.state); > + commit = crtc_state->base.commit; > + if (commit) { > + ret = wait_for_completion_interruptible(&commit->hw_done); > + if (!ret) > + ret = wait_for_completion_interruptible(&commit->flip_done); > + } > + > + if (!ret && crtc_state->base.active) { > + DRM_DEBUG_KMS("Re-arming FIFO underruns on pipe %c\n", > + pipe_name(intel_crtc->pipe)); > + > + intel_crtc_arm_fifo_underrun(intel_crtc, crtc_state); > + } > + > + drm_modeset_unlock(&intel_crtc->base.mutex); > + > + if (ret) > + return ret; > + } > + > + ret = intel_fbc_reset_underrun(dev_priv); > + if (ret) > + return ret; > + > + return cnt; > +} > + > +static const struct file_operations i915_fifo_underrun_reset_ops = { > + .owner = THIS_MODULE, > + .open = simple_open, > + .write = i915_fifo_underrun_reset_write, > + .llseek = default_llseek, > +}; > + > static const struct drm_info_list i915_debugfs_list[] = { > {"i915_capabilities", i915_capabilities, 0}, > {"i915_gem_objects", i915_gem_object_info, 0}, > @@ -4798,6 +4859,7 @@ static const struct i915_debugfs_files { > {"i915_error_state", &i915_error_state_fops}, > {"i915_gpu_info", &i915_gpu_info_fops}, > #endif > + {"i915_fifo_underrun_reset", &i915_fifo_underrun_reset_ops}, > {"i915_next_seqno", &i915_next_seqno_fops}, > {"i915_display_crc_ctl", &i915_display_crc_ctl_fops}, > {"i915_pri_wm_latency", &i915_pri_wm_latency_fops}, > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 4c30c7c04f9c..3df0e8193b83 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12960,10 +12960,25 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, > intel_cstate); > } > > +void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc, > + struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + > + if (!IS_GEN2(dev_priv)) > + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true); > + > + if (crtc_state->has_pch_encoder) { > + enum pipe pch_transcoder = > + intel_crtc_pch_transcoder(crtc); > + > + intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder, true); > + } > +} > + > static void intel_finish_crtc_commit(struct drm_crtc *crtc, > struct drm_crtc_state *old_crtc_state) > { > - struct drm_i915_private *dev_priv = to_i915(crtc->dev); > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_atomic_state *old_intel_state = > to_intel_atomic_state(old_crtc_state->state); > @@ -12974,17 +12989,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc, > > if (new_crtc_state->update_pipe && > !needs_modeset(&new_crtc_state->base) && > - old_crtc_state->mode.private_flags & I915_MODE_FLAG_INHERITED) { > - if (!IS_GEN2(dev_priv)) > - intel_set_cpu_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true); > - > - if (new_crtc_state->has_pch_encoder) { > - enum pipe pch_transcoder = > - intel_crtc_pch_transcoder(intel_crtc); > - > - intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder, true); > - } > - } > + old_crtc_state->mode.private_flags & I915_MODE_FLAG_INHERITED) > + intel_crtc_arm_fifo_underrun(intel_crtc, new_crtc_state); > } > > /** > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index d1452fd2a58d..ca005c989e9f 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1592,6 +1592,8 @@ void hsw_disable_ips(const struct intel_crtc_state *crtc_state); > enum intel_display_power_domain intel_port_to_power_domain(enum port port); > void intel_mode_from_pipe_config(struct drm_display_mode *mode, > struct intel_crtc_state *pipe_config); > +void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc, > + struct intel_crtc_state *crtc_state); > > int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state); > int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state); > @@ -1777,6 +1779,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv, > unsigned int frontbuffer_bits, enum fb_op_origin origin); > void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv); > void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv); > +int intel_fbc_reset_underrun(struct drm_i915_private *dev_priv); > > /* intel_hdmi.c */ > void intel_hdmi_init(struct drm_i915_private *dev_priv, i915_reg_t hdmi_reg, > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index 707d49c12638..308bdd136d33 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -1272,6 +1272,32 @@ static void intel_fbc_underrun_work_fn(struct work_struct *work) > mutex_unlock(&fbc->lock); > } > > +/* > + * intel_fbc_reset_underrun - reset FBC fifo underrun status. > + * @dev_priv: i915 device instance > + * > + * See intel_fbc_handle_fifo_underrun_irq(). For automated testing we > + * want to re-enable FBC after an underrun to increase test coverage. > + */ > +int intel_fbc_reset_underrun(struct drm_i915_private *dev_priv) > +{ > + int ret; > + > + cancel_work_sync(&dev_priv->fbc.underrun_work); > + > + ret = mutex_lock_interruptible(&dev_priv->fbc.lock); > + if (ret) > + return ret; > + > + if (dev_priv->fbc.underrun_detected) > + DRM_DEBUG_KMS("Re-allowing FBC after fifo underrun\n"); > + > + dev_priv->fbc.underrun_detected = false; > + mutex_unlock(&dev_priv->fbc.lock); > + > + return 0; > +} > + > /** > * intel_fbc_handle_fifo_underrun_irq - disable FBC when we get a FIFO underrun > * @dev_priv: i915 device instance
Op 28-03-18 om 12:21 schreef Jani Nikula: > On Wed, 28 Mar 2018, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: >> Adding a i915_fifo_underrun_reset debugfs file will make it possible >> for IGT tests to clear FIFO underrun fallout at the start of each >> subtest, and make re-enable FBC so tests always have maximum exposure >> to features used by IGT. FIFO underruns and FBC bugs will no longer >> hide when an earlier subtests disables both. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105685 >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105681 > FWIW, ack on the idea, didn't look at the implementation. Well I had a NV12 test that produced FIFO underruns, did some quick testing against the i915_fifo_underrun_reset file and manually writing it does reset FIFO underruns. Immediately after i915_fbc_status still reads "FBC disabled: underrun detected", but this is cleared by the next atomic commit. So I think it works, and can be used for igt in the way I wrote it. ~Maarten
On Wed, Mar 28, 2018 at 06:20:26PM +0200, Maarten Lankhorst wrote: > Op 28-03-18 om 12:21 schreef Jani Nikula: > > On Wed, 28 Mar 2018, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > >> Adding a i915_fifo_underrun_reset debugfs file will make it possible > >> for IGT tests to clear FIFO underrun fallout at the start of each > >> subtest, and make re-enable FBC so tests always have maximum exposure > >> to features used by IGT. FIFO underruns and FBC bugs will no longer > >> hide when an earlier subtests disables both. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105685 > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105681 > > FWIW, ack on the idea, didn't look at the implementation. > Well I had a NV12 test that produced FIFO underruns, did some quick testing against the i915_fifo_underrun_reset file and manually writing it does reset FIFO underruns. > Immediately after i915_fbc_status still reads "FBC disabled: underrun detected", but this is cleared by the next atomic commit. So I think it works, and can be used for igt in the way I wrote it. What about a error message change on fbc_status to reflect this temporary state? Just to avoid later confusion on expectation. But the approach and code look sane for me, so anyways: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > ~Maarten > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 09-04-18 om 21:48 schreef Rodrigo Vivi: > On Wed, Mar 28, 2018 at 06:20:26PM +0200, Maarten Lankhorst wrote: >> Op 28-03-18 om 12:21 schreef Jani Nikula: >>> On Wed, 28 Mar 2018, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: >>>> Adding a i915_fifo_underrun_reset debugfs file will make it possible >>>> for IGT tests to clear FIFO underrun fallout at the start of each >>>> subtest, and make re-enable FBC so tests always have maximum exposure >>>> to features used by IGT. FIFO underruns and FBC bugs will no longer >>>> hide when an earlier subtests disables both. >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105685 >>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105681 >>> FWIW, ack on the idea, didn't look at the implementation. >> Well I had a NV12 test that produced FIFO underruns, did some quick testing against the i915_fifo_underrun_reset file and manually writing it does reset FIFO underruns. >> Immediately after i915_fbc_status still reads "FBC disabled: underrun detected", but this is cleared by the next atomic commit. So I think it works, and can be used for igt in the way I wrote it. > What about a error message change on fbc_status to reflect this temporary state? > Just to avoid later confusion on expectation. Done, if an underrun is cleared, the reason will be set to 'FIFO underrun cleared'. > But the approach and code look sane for me, so anyways: > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Thanks for reviewing, pushed. :)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 7816cd53100a..27188de2c32b 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -4731,6 +4731,67 @@ static int i915_drrs_ctl_set(void *data, u64 val) DEFINE_SIMPLE_ATTRIBUTE(i915_drrs_ctl_fops, NULL, i915_drrs_ctl_set, "%llu\n"); +static ssize_t +i915_fifo_underrun_reset_write(struct file *filp, + const char __user *ubuf, + size_t cnt, loff_t *ppos) +{ + struct drm_i915_private *dev_priv = filp->private_data; + struct intel_crtc *intel_crtc; + struct drm_device *dev = &dev_priv->drm; + int ret; + bool reset; + + ret = kstrtobool_from_user(ubuf, cnt, &reset); + if (ret) + return ret; + + if (!reset) + return cnt; + + for_each_intel_crtc(dev, intel_crtc) { + struct drm_crtc_commit *commit; + struct intel_crtc_state *crtc_state; + + ret = drm_modeset_lock_single_interruptible(&intel_crtc->base.mutex); + if (ret) + return ret; + + crtc_state = to_intel_crtc_state(intel_crtc->base.state); + commit = crtc_state->base.commit; + if (commit) { + ret = wait_for_completion_interruptible(&commit->hw_done); + if (!ret) + ret = wait_for_completion_interruptible(&commit->flip_done); + } + + if (!ret && crtc_state->base.active) { + DRM_DEBUG_KMS("Re-arming FIFO underruns on pipe %c\n", + pipe_name(intel_crtc->pipe)); + + intel_crtc_arm_fifo_underrun(intel_crtc, crtc_state); + } + + drm_modeset_unlock(&intel_crtc->base.mutex); + + if (ret) + return ret; + } + + ret = intel_fbc_reset_underrun(dev_priv); + if (ret) + return ret; + + return cnt; +} + +static const struct file_operations i915_fifo_underrun_reset_ops = { + .owner = THIS_MODULE, + .open = simple_open, + .write = i915_fifo_underrun_reset_write, + .llseek = default_llseek, +}; + static const struct drm_info_list i915_debugfs_list[] = { {"i915_capabilities", i915_capabilities, 0}, {"i915_gem_objects", i915_gem_object_info, 0}, @@ -4798,6 +4859,7 @@ static const struct i915_debugfs_files { {"i915_error_state", &i915_error_state_fops}, {"i915_gpu_info", &i915_gpu_info_fops}, #endif + {"i915_fifo_underrun_reset", &i915_fifo_underrun_reset_ops}, {"i915_next_seqno", &i915_next_seqno_fops}, {"i915_display_crc_ctl", &i915_display_crc_ctl_fops}, {"i915_pri_wm_latency", &i915_pri_wm_latency_fops}, diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 4c30c7c04f9c..3df0e8193b83 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12960,10 +12960,25 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, intel_cstate); } +void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc, + struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + + if (!IS_GEN2(dev_priv)) + intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true); + + if (crtc_state->has_pch_encoder) { + enum pipe pch_transcoder = + intel_crtc_pch_transcoder(crtc); + + intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder, true); + } +} + static void intel_finish_crtc_commit(struct drm_crtc *crtc, struct drm_crtc_state *old_crtc_state) { - struct drm_i915_private *dev_priv = to_i915(crtc->dev); struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_atomic_state *old_intel_state = to_intel_atomic_state(old_crtc_state->state); @@ -12974,17 +12989,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc, if (new_crtc_state->update_pipe && !needs_modeset(&new_crtc_state->base) && - old_crtc_state->mode.private_flags & I915_MODE_FLAG_INHERITED) { - if (!IS_GEN2(dev_priv)) - intel_set_cpu_fifo_underrun_reporting(dev_priv, intel_crtc->pipe, true); - - if (new_crtc_state->has_pch_encoder) { - enum pipe pch_transcoder = - intel_crtc_pch_transcoder(intel_crtc); - - intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder, true); - } - } + old_crtc_state->mode.private_flags & I915_MODE_FLAG_INHERITED) + intel_crtc_arm_fifo_underrun(intel_crtc, new_crtc_state); } /** diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index d1452fd2a58d..ca005c989e9f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1592,6 +1592,8 @@ void hsw_disable_ips(const struct intel_crtc_state *crtc_state); enum intel_display_power_domain intel_port_to_power_domain(enum port port); void intel_mode_from_pipe_config(struct drm_display_mode *mode, struct intel_crtc_state *pipe_config); +void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc, + struct intel_crtc_state *crtc_state); int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state); int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state); @@ -1777,6 +1779,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv, unsigned int frontbuffer_bits, enum fb_op_origin origin); void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv); void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv); +int intel_fbc_reset_underrun(struct drm_i915_private *dev_priv); /* intel_hdmi.c */ void intel_hdmi_init(struct drm_i915_private *dev_priv, i915_reg_t hdmi_reg, diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 707d49c12638..308bdd136d33 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -1272,6 +1272,32 @@ static void intel_fbc_underrun_work_fn(struct work_struct *work) mutex_unlock(&fbc->lock); } +/* + * intel_fbc_reset_underrun - reset FBC fifo underrun status. + * @dev_priv: i915 device instance + * + * See intel_fbc_handle_fifo_underrun_irq(). For automated testing we + * want to re-enable FBC after an underrun to increase test coverage. + */ +int intel_fbc_reset_underrun(struct drm_i915_private *dev_priv) +{ + int ret; + + cancel_work_sync(&dev_priv->fbc.underrun_work); + + ret = mutex_lock_interruptible(&dev_priv->fbc.lock); + if (ret) + return ret; + + if (dev_priv->fbc.underrun_detected) + DRM_DEBUG_KMS("Re-allowing FBC after fifo underrun\n"); + + dev_priv->fbc.underrun_detected = false; + mutex_unlock(&dev_priv->fbc.lock); + + return 0; +} + /** * intel_fbc_handle_fifo_underrun_irq - disable FBC when we get a FIFO underrun * @dev_priv: i915 device instance
Adding a i915_fifo_underrun_reset debugfs file will make it possible for IGT tests to clear FIFO underrun fallout at the start of each subtest, and make re-enable FBC so tests always have maximum exposure to features used by IGT. FIFO underruns and FBC bugs will no longer hide when an earlier subtests disables both. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105685 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105681 --- drivers/gpu/drm/i915/i915_debugfs.c | 62 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++------- drivers/gpu/drm/i915/intel_drv.h | 3 ++ drivers/gpu/drm/i915/intel_fbc.c | 26 +++++++++++++++ 4 files changed, 109 insertions(+), 12 deletions(-)