[Intel-gfx] [PATCH 7/7] drm/i915: GEM operations need to be done under the big lock

[Imre Deak]

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.

--Imre