[Intel-gfx] [RFC] Runtime display PM for VLV/BYT

[Imre Deak]

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 at 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