Re: [PATCH 10/15] drm/i915: check the power well when capturing error state

From: Paulo Zanoni <przanoni@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni <paulo.r.zanoni@intel.com>
Subject: Re: [PATCH 10/15] drm/i915: check the power well when capturing error state
Date: Thu, 21 Mar 2013 19:12:04 -0300	[thread overview]
Message-ID: <CA+gsUGSO9qz49xw7upg_9=aWgDhjJ-XSRezRRph1fsa8bqCdeg@mail.gmail.com> (raw)
In-Reply-To: <20130317204637.GP9021@phenom.ffwll.local>

Hi

2013/3/17 Daniel Vetter <daniel@ffwll.ch>:
> On Wed, Mar 06, 2013 at 08:03:17PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> This solves many "unclaimed register" messages when the power well is
>> down and we get a GPU hang.
>>
>> Also print the power well register and each pipe's CPU transcoder on
>> the error state to allow proper interpratation of the registers. And
>> kzalloc the error state structure since we may not read some of the
>> registers (to avoid the "unclaimed register" messages).
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Ok, I've thought some more about all and you're all going to hate me.
>
> Essentially I don't like that we have rather invasive patches for a minor
> feature with _very_ delicate limits as to what hits the debug yelling and
> what doesn't. Furthermore the debug feature isn't that clear-cut since
> interrupts get in the way (and we cant fix that since that would also
> disable underrun interrupts, which we want). And the audio driver also
> randomly inteferes.
>
> Now where it's justified that a register doesn't exist I don't mind
> merging the patches - it's always good to have accurate documentation of
> what's really used by the hw. But for debug features I'm really uneasy
> with runtime checks, since it might very well be that those are wrong.
>
> Hence I vote for a I915_READ_NOCHECK to avoid all this madness. This one
> will never check for errors, but silently clear them after a read. Note
> that we don't need a I915_WRITE_NOCHECK, and imo that's good since writing
> random crap could indeed be a serious bug.

I don't understand your way of thinking: your argument is that "this
is difficult to maintain", but then you're proposing the addition of a
new i915_read, which IMHO is way much more complex and feels like a
hackish solution to the problem. Every patch to the "normal"
i915_read#x will have to also patch i915_read_nocheck#x. And on patch
0014, which you also suggested to use the "I915_READ_NOCHECK"
function, our code will be relying on the _coincidence_ that when we
read a register that does not exist, we read 0. I really don't think
this is a sane approach.

Our hardware is getting much more complex with respect to these reads,
and I think we should never read ever read from a register that does
not exist or is powered off. I'm open to more suggestions on how to
write this patch, but I really think we shouldn't read registers that
don't exist or are powered off.

>
> So I'll punt on this patch since imo it's too messy for future
> maintenance. Same for the other debug patches (e.g. the assert_pipe_enabled
> check).
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Paulo Zanoni