[Intel-gfx] [PATCH 1/2] drm/i915: Get power refs in encoder->get_power_domain()

[Imre Deak]

On Sun, Apr 07, 2019 at 12:41:08PM +0100, Chris Wilson wrote:
> Quoting Imre Deak (2019-04-07 11:25:37)
> > On Sat, Apr 06, 2019 at 05:05:53PM +0100, Chris Wilson wrote:
> > > Quoting Imre Deak (2019-04-05 16:36:56)
> > > > Push getting the reference for the encoders' power domains into the
> > > > encoder get_power_domain() hook instead of doing this from the caller.
> > > > This way the encoder can store away the corresponding wakerefs.
> > > > 
> > > > This fixes the DSI encoder disabling, which didn't release these
> > > > power references it acquired during HW state readout.
> > > 
> > > The io_wakeref is for the paired io_enable/io_disable.
> > > 
> > > get_encoder_power_domains() is the owner of these wakerefs, and they
> > > then belong to the atomic state from preparation through use to final
> > > release.
> > 
> > Yes, we have two cases:
> > 
> > 1. HW readout
> > 
> > encoder->get_power_domains() -> io_enable() -> takes wakerefs
> > encoder->disable() -> io_disable() -> releases wakerefs
> 
> However, get_power_domains is used for the atomic state wakerefs (as
> well?) and they have different much longer lifetimes.

It's only used during HW readout, if the encoder is active. Then we need
to take the wakerefs the same way encoder->enable() would take them,
since ->enable() won't be called. Then the refs taken by
->get_power_domains() will be held for long, until ->disable() is
called, so they have the same lifetime as in the ->enable()/->disable()
case.

> Clear io_wakerefs, and put a WARN_ON(io_wakeref[]) prior to filling it
> to convince me that it is being used to track a temporary wakeref.

Ok, I thought about it too just after you wrote. Yes, at that point
there should be no wakerefs stored there, which we should check for.
Will send v2.

> get_power_domains() and atomic needs to take responsibility for the
> overlapping set of powerwells imo.

->get_power_domains() is essentially the powerref acquisition part of the
encoder->get_hw_state(). We have a separate hook for it since the latter
one is also called during verification, where we don't want to take the
long time refs (they must have been acquired already by the modeset we
are verifying).

> -Chris