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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
