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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
