On ke, 2016-01-13 at 14:32 +0000, Chris Wilson wrote: > On Wed, Jan 13, 2016 at 02:11:42PM +0000, Tvrtko Ursulin wrote: > > > > On 13/01/16 13:36, Imre Deak wrote: > > > On ke, 2016-01-13 at 12:46 +0000, Tvrtko Ursulin wrote: > > > > On 11/01/16 16:56, Chris Wilson wrote: > > > > > On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä > > > > > wrote: > > > > > > On Mon, Jan 11, 2016 at 03:16:16PM +0000, Tvrtko Ursulin > > > > > > wrote: > > > > > > > Don't know, I leave this one to whoever grabbed the lock > > > > > > > around > > > > > > > intel_init_gt_powersave in the first place. Maybe there > > > > > > > was a > > > > > > > special > > > > > > > reason.. after git blame od intel_display.c eventually > > > > > > > completed, adding > > > > > > > Imre and Ville to cc. > > > > > > > > > > > > Hmm. I don't recall the details anymore, but looking at the > > > > > > code > > > > > > pushing > > > > > > the locking down to valleyview_setup_pctx() looks entirely > > > > > > reasonable to > > > > > > me. > > > > > > > > > > iirc, this locking only exists to keep the WARN() at bay. But > > > > > it is > > > > > pedagogical, I guess. > > > > > > > > Don't really know this area, but what about the > > > > intel_gen6_powersave_work->valleyview_enable_rps- > > > > > valleyview_check_pctx > > > > which dereferences the dev_priv->vlv_pctx, which is set/cleared > > > > in > > > > valleyview_setup_pctx/valleyview_cleanup_pctx, which would now > > > > be > > > > outside both struct_mutex and the rps lock? > > > > > > dev_priv->vlv_pctx is not protected on the premise that the > > > driver > > > init/cleanup functions can't race and gen6_powersave_work() is > > > scheduled only after intel_init_gt_powersave() and flushed > > > before intel_cleanup_gt_powersave(). > > > > > > rps_lock protects the RPS HW accesses themselves and struct_mutex > > > was > > > taken for the GEM allocation. Taking it at high level around > > > intel_init_gt_powersave() was kind of a copy&paste in the commit > > > you > > > found, there is more on that in 79f5b2c75992. > > > > Thanks for digging this out. > > > > It is more involved than what Chris pasted then, and while I hoped > > to be able to quickly shove that into a patch, I cannot allow the > > time for more the extensive analysis. > > Imre just confirmed that struct_mutex is only for the GEM > allocations in the vlv_setup_ctx, right Imre? Yes, that's my understanding. --Imre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
