On Mon, Nov 24, 2014 at 04:23:36PM +0100, Daniel Vetter wrote: > The crc code doesn't handle anything really that could drop the > register state (by design so that we have less complexity). Which > means userspace may only start crc capture once the pipe is fully set > up. > > With an i-g-t patch this will be the case, but there's still the > problem that this results in obscure unclaimed register write > failures. Which is a pain to debug. > > So instead make sure we don't have the basic unclaimed register write > failure by grabbing runtime pm references. And reject completely > invalid requests with -EIO. This is still racy of course, but for a > test library we don't really care - if userspace shuts down the pipe > right afterwards the entire setup will be lost anyway. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=86092 > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 319da61354b0..68a76e58e8fd 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -3309,6 +3309,14 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe, > if (pipe_crc->source && source) > return -EINVAL; > > + intel_runtime_pm_get(dev_priv); Do we need this intel_runtime_pm_get() here? > + if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PIPE(pipe))) { > + DRM_DEBUG_KMS("Trying to capture CRC while pipe is off\n"); > + ret = -EIO; > + goto out; > + } > + > if (IS_GEN2(dev)) > ret = i8xx_pipe_crc_ctl_reg(&source, &val); > else if (INTEL_INFO(dev)->gen < 5) > @@ -3321,7 +3329,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe, > ret = ivb_pipe_crc_ctl_reg(dev, pipe, &source, &val); > > if (ret != 0) > - return ret; > + goto out; > > /* none -> real source transition */ > if (source) { > @@ -3331,8 +3339,10 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe, > pipe_crc->entries = kzalloc(sizeof(*pipe_crc->entries) * > INTEL_PIPE_CRC_ENTRIES_NR, > GFP_KERNEL); > - if (!pipe_crc->entries) > - return -ENOMEM; > + if (!pipe_crc->entries) { > + ret = -ENOMEM; > + goto out; > + } > > /* > * When IPS gets enabled, the pipe CRC changes. Since IPS gets > @@ -3383,8 +3393,10 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe, > > hsw_enable_ips(crtc); > } > +out: > + intel_runtime_pm_get(dev_priv); That's a second intel_runtime_pm_get(); > > - return 0; > + return ret; > } > > /* > -- > 2.1.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
