From: Jesse Barnes <jbarnes@virtuousgeek.org>
To: Paulo Zanoni <przanoni@gmail.com>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 4/5] drm/i915: take power well refs when needed
Date: Tue, 15 Oct 2013 13:57:00 -0700 [thread overview]
Message-ID: <20131015135700.3aaba36c@jbarnes-desktop> (raw)
In-Reply-To: <CA+gsUGRWu2-LXJxBfMm+Wri9S5EbXDkVb53iGPFgRoav2UhcsQ@mail.gmail.com>
On Tue, 15 Oct 2013 17:47:20 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:
> 2013/10/15 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > On Tue, 15 Oct 2013 16:54:00 -0300
> > Paulo Zanoni <przanoni@gmail.com> wrote:
> >
> >> 2013/10/14 Jesse Barnes <jbarnes@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.
--
Jesse Barnes, Intel Open Source Technology Center
next prev parent reply other threads:[~2013-10-15 20:56 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-10-14 23:07 [RFC] Runtime display PM for VLV/BYT Jesse Barnes
2013-10-14 23:07 ` [PATCH 1/5] drm/i915/vlv: power well support " Jesse Barnes
2013-10-14 23:07 ` [PATCH 2/5] drm/i915: add display power well report out to debugfs Jesse Barnes
2013-10-14 23:07 ` [PATCH 3/5] drm/i915/vlv: suspend/resume fixes for VLV/BYT Jesse Barnes
2013-10-14 23:07 ` [PATCH 4/5] drm/i915: take power well refs when needed Jesse Barnes
2013-10-15 19:54 ` Paulo Zanoni
2013-10-15 20:40 ` Jesse Barnes
2013-10-15 20:47 ` Paulo Zanoni
2013-10-15 20:57 ` Jesse Barnes [this message]
2013-10-15 21:03 ` Paulo Zanoni
2013-10-16 11:10 ` Imre Deak
2013-10-16 15:08 ` Jesse Barnes
2013-10-17 13:01 ` Imre Deak
2013-10-14 23:07 ` [PATCH 5/5] drm/i915/vlv: support save/restore of display state around power well toggle Jesse Barnes
2013-10-15 20:09 ` Paulo Zanoni
2013-10-15 20:42 ` Jesse Barnes
2013-10-16 8:54 ` Daniel Vetter
2013-10-15 8:06 ` [RFC] Runtime display PM for VLV/BYT Ville Syrjälä
2013-10-15 12:16 ` Imre Deak
2013-10-15 16:23 ` Jesse Barnes
2013-10-15 18:15 ` Imre Deak
2013-10-15 22:09 ` Daniel Vetter
2013-10-16 14:45 ` Imre Deak
2013-10-15 9:59 ` Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131015135700.3aaba36c@jbarnes-desktop \
--to=jbarnes@virtuousgeek.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=przanoni@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox