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

[Chris Wilson]

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?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre