Re: [PATCH 3/4] drm/i915: Make turning on/off PW1 and Misc I/O part of the init/fini sequences

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 05, 2015 at 03:28:54PM -0300, Paulo Zanoni wrote:
> 2015-08-05 5:30 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Thu, Jul 30, 2015 at 06:20:28PM -0300, Paulo Zanoni wrote:
> >> From: Damien Lespiau <damien.lespiau@intel.com>
> >>
> >> Before this patch, we used the intel_display_power_{get,put} functions
> >> to make sure the PW1 and Misc I/O power wells were enabled all the
> >> time while LCPLL was enabled. We called a get() at
> >> intel_ddi_pll_init() when we discovered that LCPLL was enabled, then
> >> we would call put/get at skl_{un,}init_cdclk().
> >>
> >> The problem is that skl_uninit_cdclk() is indirectly called by
> >> intel_runtime_suspend(). So it will only release its power well
> >> _after_ we already decided to runtime suspend. But since we only
> >> decide to runtime suspend after all power wells and refcounts are
> >> released, that basically means we will never decide to runtime
> >> suspend.
> >>
> >> So what this patch does to fix that problem is move the PW1 + Misc I/O
> >> power well handling out of the runtime PM mechanism: instead of
> >> calling intel_display_power_{get_put} - functions that touch the
> >> refcount -, we'll call the low level intel_power_well_{en,dis}able,
> >> which don't change the refcount. This way, it is now possible for the
> >> refcount to actually reach zero, and we'll now start runtime
> >> suspending/resuming.
> >>
> >> v2 (from Paulo):
> >>   - Write a commit message since the original patch left it empty.
> >>   - Rebase after the intel_power_well_{en,dis}able rename.
> >>   - Use lookup_power_well() instead of hardcoded indexes.
> >>
> >> Testcase: igt/pm_rpm/rte (and every other rpm test)
> >> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > This is imo too much of a hack. If we go with this then we should either
> > completely remove the pw1 and misc pw from the power well code and just
> > directly call the relevant functions.
> 
> What do you mean by "the relevant functions"? Notice that the patch
> already moved us outside of the "power domains" framework, but not the
> "power wells" framework, since those are actual power wells. I'm still
> trying to fully understand what you wanted here.

The power wells abstraction doesn't buy anything here if we have a power
well that we always enable/disable with something else (device rpm here).
So instead of the lookup_power_well + enable call just remove pw1 and
pw-misc from the list of power wells and call the enable code directly.

Otherwise we just have a bit of abstraction that gets in the way. Also
note that dmc has similar requirements of enable pw1 and lcpll together to
avoid upsetting the firmware.

The overall idea is that abstraction should only be used when it actually
makes sense for a given platform. And I think using power wells
abstraction for pw1 and pw-misc on skl doesn't make sense since we can't
actually use it as an independent power well from the software pov - the
hw itself is different, but that's all managed by dmc firmware for us.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx