Re: [PATCH 4/7] drm/i915: Add i830 "pipes power well"

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Jun 06, 2017 at 09:04:40AM +0200, Maarten Lankhorst wrote:
> Op 01-06-17 om 16:36 schreef ville.syrjala@linux.intel.com:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > 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@linux.intel.com>
> > ---
> >  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@linux.intel.com>
> 
> 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@linux.intel.com>

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx