[Intel-gfx] [PATCH] drm/i915: disable VGA mem when reenabling the power well

[Paulo Zanoni]

2013/12/11 Daniel Vetter <daniel at ffwll.ch>:
> On Wed, Dec 11, 2013 at 6:44 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
>>
>> After we re-enable the power well, every time we read/write the VGA
>> address 0x3d5 we get an "unclaimed register" interrupt (ERR_INT) and
>> print error messages. If we write anything to the VGA MSR register (it
>> doesn't really matter which value you write to bit 0), any
>> reads/writes to 0x3d5 _don't_ trigger the "unclaimed register" errors
>> anymore (even if MSR bit 0 is zero). So what happens with the current
>> code is that when we unbind i915 and bind vgacon, we call
>> console_unlock(). Function console_unlock() is responsible for
>> printing any messages that were supposed to be print when the console
>> was locked, so it calls the TTY layer, which calls the console layer,
>> which calls vgacon to print the messages. At this point, vgacon
>> eventually calls vgacon_set_cursor_size(), which touches 0x3d5, which
>> triggers unclaimed register interrupts. The problem is that when we
>> get these interrupts, we print the error messages, so we add more work
>> to console_unlock(), which will try to print it again, and then call
>> vgacon again, trigger a new interrupt, which will put more stuff to
>> the buffer, and then we'll be stuck at console_unlock() forever.
>>
>> If you patch intel_uncore.c to not print anything when we detect
>> unclaimed registers, we won't get into the console_unlock() infinite
>> loop and the driver unbind will work just fine. We will still be
>> getting interrupts every time vgacon touches those registers, but we
>> will survive. This is a valid experiment, but IMHO it's not the real
>> fix: if we don't print any error messages we will still keep getting
>> the interrupts, and if we disable ERR_INT we won't get the interrupt
>> anymore, but we will also stop getting all the other error interrupts.
>>
>> I talked about this problem with the HW engineer and his
>> recommendation is "So don't do any VGA I/O or memory access while the
>> power well is disabled, and make to re-program MSR after enabling the
>> power well and before using VGA I/O or memory accesses.".
>
> Impressive piece of debugging. And a great story ;-)
>
>> Based on this, I could write an even-smaller patch that just touches
>> the MSR register to avoid the problem, also providing a nice comment
>> and an improved commit message. Or I could also write another
>> implementation that actually saves the value of MSR before we disable
>> the power well, then restores the correct value after we re-enable it:
>> but that wouldn't help vgacon too much. IMHO one of these new patches
>> would be the appropriate way to solve the problem, but it seems that
>> Daniel/Ville didn't like the previous patches, so I'm not sure they
>> will be willing to accept the next version either. Since so far I
>> haven't seen any alternative implementation/suggestion that actually
>> fixes the problem, and since module_reload is not working on Haswell
>> for months (possibly hiding other module_reload bugs), my vote is
>> obviously to accept the next patch because at least it fixes the
>> 6-months-old regression. But if I don't get an "ack" on this I won't
>> spend my time working on this. What do you think?
>
> I'd go with re-disabling vga unconditionally through the msr. We don't
> bother with restoring the vga plane anyway at module unload, so vga is
> hosed no matter what. And it sounds like this is the more minimal fix.
> So ack for the minimal fix to just poke the MSR at the right times.

Just to be clear: by "at the right times" you mean "when re-enabling
the power well", right?

>
>> Please also notice that this only fixes the case where the power well
>> is already enabled when we unbind. The fix for when the power well is
>> _disabled_ when we unbind depends on this, and IMHO we should discuss
>> it later.
>
> I guess I need to resurrect my "kill vgacon" patch and figure out why
> it doesn't work.

See comment #45 of the bug report for an explanation on how it fails.
I'm not sure if it's possible to do what you want.


> Or could we just force-enable the power well when we
> start our module unload sequence instead?

As I mentioned before, driver unbind (a requirement for module unload)
happens before we call any of the unload functions, so we never even
get to run i915_driver_unload.


> I'd prefer the most minimal
> fix, and even better if we don't have to touch the vgacon hell at all

Me too :)

> ...
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni