[Intel-gfx] [PATCH 4/5] drm/i915: take power well refs when needed

[Paulo Zanoni]

2013/10/15 Jesse Barnes <jbarnes at virtuousgeek.org>:
> On Tue, 15 Oct 2013 17:47:20 -0300
> Paulo Zanoni <przanoni at gmail.com> wrote:
>
>> 2013/10/15 Jesse Barnes <jbarnes at virtuousgeek.org>:
>> > On Tue, 15 Oct 2013 16:54:00 -0300
>> > Paulo Zanoni <przanoni at gmail.com> wrote:
>> >
>> >> 2013/10/14 Jesse Barnes <jbarnes at virtuousgeek.org>:
>> >> > When accessing the display regs for hw state readout or cross check, we
>> >> > need to make sure the power well is enabled so we can read valid
>> >> > register state.
>> >>
>> >> On the current code (HSW) we already check for the power wells in the
>> >> HW state readout code: if the power well is disabled, then transcoders
>> >> A/B/C are disabled. The current code takes care to not touch registers
>> >> on a disabled power well.
>> >
>> > Yeah and we should probably preserve that behavior, for the readout
>> > code at least.
>> >
>> >> > +       intel_display_power_put(dev, POWER_DOMAIN_PIPE(0));
>> >> > +
>> >>
>> >> Why is this here? It certainly deserves a comment in the code.
>> >
>> > It probably needs to be removed.  The refcounting for the initial load
>> > is still screwy due to the unconditional enable later on.
>> >
>> >> So now HSW uses global_resources while VLV uses crtc enable/disable. I
>> >> really think both platforms should try to do the same thing.
>> >
>> > Definitely agreed.
>> >
>> >> By the way, at least on Haswell, if we do an equivalent change we'll
>> >> have problems, because when you disable the power well at
>> >> crtc_disable, then everything you did at crtc_mode_set will be undone,
>> >> and it won't be redone at crtc_enable. When you reenable the power
>> >> well, the registers go back to their default values, not the values
>> >> that were previously there. Did you check if VLV behaves the same?
>> >
>> > No that's taken into account here.  In __intel_set_mode we take a
>> > private ref on the appropriate power well so that we'll preserve state
>> > until we do the first crtc_enable.  From then on, the ref is tracked
>> > there and we drop the private one in __intel_set_mode
>>
>> Yeah, but then after all this is done, at some point we'll call
>> crtc_disable, then we'll put the last ref back and then the power well
>> will be disabled. Then the user moves the mouse and we call
>> crtc_enable, so we enable the power well, all its registers to back to
>> their reset state, then crtc_enable sets some of the registers, but
>> that won't really be enough since no one called crtc_mode_set again,
>> and all the state set by crtc_mode_set (not touched by crtc_enable) is
>> back to the default values. No? What am I missing?
>
> That's where the save/restore state of the later patch comes in.  We
> need that if we're going to support DPMS and runtime suspend w/o a mode
> set.

Oh... I wasn't even thinking about it since it's on a later patch. But
makes sense.

But instead of the save/restore thing, shouldn't we just move all the
code that touches registers from mode_set to crtc_enable? IMHO it's
the shiny solution here. And anything else that we may need to
save/restore should be moved to crtc enable/disable and be saved in
"struct intel_crtc" every time it is touched. I really think that
removing stuff from the save/restore code is the way to go. Also, with
my suggestion, crtc_enable will fully implement the "mode set
sequence" from BSpec, so people like the Audio guys will have an
easier time understanding our code :)

>
> --
> Jesse Barnes, Intel Open Source Technology Center



-- 
Paulo Zanoni