Re: [PATCH] drm/i915: WARN if interrupts aren't on in en/disable_pipestat

Daniel Vetter
2014-08-27T13:29:01Z

On Wed, Aug 27, 2014 at 1:28 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Aug 27, 2014 at 12:23:47PM +0200, Daniel Vetter wrote:
>> On Wed, Aug 27, 2014 at 09:51:58AM +0100, Chris Wilson wrote:
>> > On Wed, Aug 27, 2014 at 10:43:37AM +0200, Daniel Vetter wrote:
>> > > Now that vlv has runtime pm we kinda should check for that like on the
>> > > pch split platforms. Looks like this was simply lost in the vlv rpm
>> > > enabling.
>> >
>> > Is there a reason why setting up the pipestat prior to enabling
>> > interrupts is verboten, or is it just not done today?
>>
>> The similar warning on pch platforms was added to catch runtime pm bugs
>> where someone enabled some interrupt source, but didn't hold a runtime pm
>> reference. That's just not going to work. So motivation for this patch is
>> vlv/chv runtime pm really.
>>
>> But I also think that this check provides value for driver
>> load/resume/suspend since if we change interrupt settings after the
>> interrupts have been disabled or before enabling something is at least
>> fishy. At least insofar as that our irq install hooks nowadays overwrite
>> all of them with default values.
>
> That's a better blurp. But doesn't it sound a little overly generic just
> to keep whether or not pipestat_enable is called after uninstall or
> before install? Since we track the expected register value, could we
> just semi-randomly check that the register matches that value?

I guess we could do both, but I don't really see a good place. Maybe
in the enable/disable functions, but I'm not even sure we track the
expected state for everyone really.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch