On Tue, Jun 06, 2017 at 09:04:40AM +0200, Maarten Lankhorst wrote: > Op 01-06-17 om 16:36 schreef ville.syrjala@xxxxxxxxxxxxxxx: > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > 830 more or less requires both pipes and DPLLs to remain on as long > > as either pipe is needed. However, when neither pipe is actually needed, > > we can save a bit of power by turning everything off. To do that we add > > a new "power well" that turns both pipes and DPLLs on and off in the > > right order. Seems to save ~50mW on my Fujitsu-Siemens Lifebook S6010. > > > > This also avoids having to abuse the load detection to force pipe A on > > at init time. That was never very robust, and it only worked for one > > pipe, whereas 830 really needs both pipes enabled. As a bonus the 830 > > pipe quirk is now a bit more isolated from the rest of the mode setting > > infrastructure, which should mean that it's much less likely someone > > will accidentally break it in the future. The extra cost is of course > > slight code duplication, but that seems like a worthwile tradeoff here. > > > > v2; s/BIT/BIT_ULL/ > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > --- > > drivers/gpu/drm/i915/intel_display.c | 92 ++++++++++++++++++++++++++++++++- > > drivers/gpu/drm/i915/intel_drv.h | 2 + > > drivers/gpu/drm/i915/intel_runtime_pm.c | 64 +++++++++++++++++++++++ > > 3 files changed, 157 insertions(+), 1 deletion(-) > > For patch 1-3: > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > > If you replace the remainder of crtc->config with pipe_config in > i9xx_crtc_enable and i9xx_crtc_disable and create a crtc_state for > disabling, won't it become possible to re-use some of the called > functions there instead of hardcoding the writes here? I considered that, but it would tie the whole thing more closely with atomic instead of making it more independent. So I decided that making it stand on its own should result in less regressions. If we had some way to test this code on modern platforms then I think we could go with that approach without having to worry about regressions so much. I have a somewhat similar conundrum with intel_crtc_mode_get(). I have a branch where I changed it to reuse the normal state readout, but I'm a little worried about people changing the called functions to require a full top level atomic state. I guess if we changed all the state readout to use a free standing top level atomic state, like Daniel has suggested, this issue would perhaps disappear since I'd just add the top level state to intel_crtc_mode_get() as well. > > For the rest of the series, > > Acked-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
