From: Imre Deak <imre.deak@intel.com>
To: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: intel-gfx@lists.freedesktop.org,
Paulo R Zanoni <paulo.r.zanoni@intel.com>
Subject: Re: [RFC] Runtime display PM for VLV/BYT
Date: Tue, 15 Oct 2013 21:15:19 +0300 [thread overview]
Message-ID: <1381860919.3361.37.camel@ideak-mobl> (raw)
In-Reply-To: <20131015092311.1dadcf35@jbarnes-desktop>
On Tue, 2013-10-15 at 09:23 -0700, Jesse Barnes wrote:
> On Tue, 15 Oct 2013 15:16:11 +0300
> Imre Deak <imre.deak@intel.com> wrote:
>
> > On Tue, 2013-10-15 at 11:06 +0300, Ville Syrjälä wrote:
> > > On Mon, Oct 14, 2013 at 04:07:44PM -0700, Jesse Barnes wrote:
> > > > This set adds bits needed for runtime power support, currently only
> > > > lightly tested on VLV/BYT:
> > > > 1) suspend/resume callbacks for different platforms
> > > > 2) save/restore of display state across a power well toggle
> > > > 3) get/put of display power well in critical places
> > > >
> > > > The TODO list still has a few items on it, and I'm looking for feedback:
> > > > 1) sprinkle around some power well WARNs so we can catch things easily
> > > > 2) add some tests using DPMS and NULL mode sets and comparing power
> > > > well state
> > > > 3) better debugfs support for multiple wells
> > > > 4) refcount of power well in debugfs (with ref holders?)
> > > > 5) more testing - I think the load time ref is still busted here and
> > > > on HSW
> > > > 6) convert HSW as well so DPMS will shut things down, not just mode
> > > > sets
> > > >
> > > > Thoughts or comments?
> > >
> > > I'd also like to see what Imre cooked up, and then come up with some
> > > grand unified design. Based on our discussions I think his power well
> > > abstraction sounded somewhat nicer and more general.
> >
> > I've pushed what I have so far to:
> > https://github.com/ideak/linux/commits/powerwells
> >
> > I've tested this on VLV with VGA output so far and somewhat on HSW. I'd
> > still have to check the need to do any HW state save/restore and the GFX
> > clock forcing, afaics Jesse has already code for these in his patchset.
>
> I think a few can be pushed upstream now:
>
> drm/i915: change power_well->lock to be mutex
> drm/i915: factor out is_always_on_domain
> drm/i915: factor out reset_vblank_counter
> drm/i915: fix HAS_POWER_WELL->IS_HASWELL - pushed this but it doesn't
> look like Daniel has picked it up yet
> drm/i915: check powerwell in get_pipe_config
Ok, I can send the missing ones to the list.
> drm/i915: enable only the needed power domains during modeset
> Looks good too, and I guess there's no harm in pushing the earlier
> patch to move the get of the power wells out of the HSW global res
> function, even if we do move to get/put later.
Agreed, with the above we'd preserve the current behavior and later
things can be fine grained.
> For drm/i915: support for multiple power wells, I think I prefer a
> single uncore function taking a power well, rather than an array of
> power_well structs. If we went that direction, we could probably
> rename the power_well variable to audio_power_wells or something and
> just track the ones we need to expose to the audio driver there, rather
> than everything. That might be a little clearer than what we have
> today, but I guess I'd have to see it coded to be sure.
I added the array mainly so that we can ref count each power well
separately. Currently this isn't a problem as we only need to refcount a
single power well, but on future HW with more of those we need to keep
the ref counters somewhere. I also store the mask of domains for which
we need the given power well there and having a vtable for Gen dependent
HW accessors looked like a nice way.
I agree the interface for requesting power wells for audio could be
better. We could add a new POWER_DOMAIN_AUDIO for that and map it to the
required power wells. But yea, this could be done later.
> For drm/i915: add intel_encoder_get_hw_state, do you think it would be
> clearer to take refs in the caller of the encoder->get_hw_state instead?
What would be the point, if all the callers would take a ref anyway? Or
do you want to take the reference once and do additional things besides
get_hw_state()?
Related to this: I made intel_encoder_get_hw_state() only check if the
power well is on and return false if it's not to indicate that the
encoder is off. I also thought of doing the same as you and take a ref
instead, not sure what's the right way. Maybe doing the readout only if
the power is on, but also making sure we have a reference in this case?
So with a new helper we'd have in intel_encoder_get_hw_state():
{
if (!intel_display_power_get_if_enabled(...))
return false;
... do the readout
intel_display_power_put(...)
}
> I like drm/i915: get port power domains for connector detect and
> drm/i915: add output power domains too, we'll need those if we want to
> do fine grained output control on BYT and future chips. But they'll
> probably need to be rebased on top of the above once it goes upstream.
Yea, we can reconsider them once we agree about the above.
> Overall I don't think there's a ton of conflict between our patchsets,
> they seem mostly complimentary and let me remove hacks in mine.
Ok, thanks a lot for the review,
Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2013-10-15 18:15 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
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 [this message]
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=1381860919.3361.37.camel@ideak-mobl \
--to=imre.deak@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jbarnes@virtuousgeek.org \
--cc=paulo.r.zanoni@intel.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