public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
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

  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