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

[Imre Deak]

On Sun, Apr 07, 2019 at 02:55:14PM +0300, Imre Deak wrote:
> 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).

Note the similar logic for crtc powerrefs in
intel_modeset_setup_hw_state()/modeset_get_crtc_power_domains().

> 
> > -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx