On Mon, Aug 21, 2017 at 09:39:24PM +0200, Daniel Vetter wrote: > On Mon, Aug 21, 2017 at 11:21:49AM +0100, Chris Wilson wrote: > > Quoting Chris Wilson (2017-08-21 10:53:36) > > > Quoting Arkadiusz Hiler (2017-08-21 10:42:25) > > > > On Mon, Aug 21, 2017 at 08:05:58AM +0000, Dec, Katarzyna wrote: > > > > > I understand we do not want to check registers in IGT tests. What > > > > > about reading interrupt masks from debugfs (i915_frequency_info). > > > > > > > > Hey Kasia > > > > > > > > It would be pretty much the same thing, but instead of us reading the > > > > PMINTRMASK directly we would ask the kernel to do that on our behalf. > > > > > > > > That would just hide register read, not get rid of it. > > > > > > > > > > > > I think you are missing the point. The idea is that we do not want to > > > > test details of in-kernel implementation, not ban the register reads > > > > completely. > > > > > > > > Reading register directly, especially just to make sure that the kernel > > > > set something correctly is a good indicator that we are trying to do > > > > just that - test the internal details. > > > > > > > > > Would that be better approach? You guys suggested to get interested in > > > > > kselftests for having such checks, but I am afraid that it could be > > > > > too much job and we have too few hands to work. > > > > > > > > How much of an effort would the kselftest be, since it seems that you did some > > > > investigation already? > > > > > > It doesn't even require a whole selftest, just something like > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index 448e71af4772..e83b67fe0354 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -7733,7 +7733,8 @@ void intel_suspend_gt_powersave(struct drm_i915_private *dev_priv) > > > if (cancel_delayed_work_sync(&dev_priv->rps.autoenable_work)) > > > intel_runtime_pm_put(dev_priv); > > > > > > - /* gen6_rps_idle() will be called later to disable interrupts */ > > > + WARN_ON(I915_READ(GEN6_PMINTRMSK) != > > > + gen6_sanitize_rps_pm_mask(dev_priv, ~0)); > > > } > > > > Wrong spot. We actually need a call from > > intel_runtime_pm_disable_interrupts. > > Yeah, for consistency checks which are very closely tied to the > implementation we tend to sprinkle WARN_ON all over the place. In some > cases those checks are too expensive for production, then we add a > compile-time-option to hide them (e.g. GEM_BUG_ON). > > I chatted with Radek, and if I understand things correctly, the main value > you derive from these is making sure a frankenstein port to an older > kernel doesn't miss or miss-apply any critical patches. In-kernel > consistency checks unfortunately don't really help with that, but we > heavily rely on these for validation. Having that stated on the mailing list from the beginning (e.g. in the commit message or as one of the first replies) would help directing the whole discussion on the right track and make us understand your needs better. I agree with Daniel's earlier statement that we should be very (over)verbose about the changes we are making and purpose they are serving. > There's also examples of this (and again, they're very important) outside > of i915, like kasan, lockdep (and maybe we'll even get kmemleak somehow > integrated into CI/igt eventually). > > So still no idea what would be the best suggestion here for your team. Kasia and Radek, can you elaborate a little more on the "frankenstein port" and your use cases for such tests? How is that comparable to backports to stable/LTS kernel branches? Hopefully we can work out how to approach similar cases, because it looks like it's might be a recurring theme in the future -- Cheers, Arek _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
