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

[Daniel Vetter]

On Wed, Jan 13, 2016 at 05:25:09PM +0200, Imre Deak wrote:
> On ke, 2016-01-13 at 14:53 +0000, Tvrtko Ursulin wrote:
> > On 13/01/16 14:41, Imre Deak wrote:
> > > 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.
> > 
> > I glanced at 79f5b2c75992 and saw it pulled out a bunch of locking
> > from 
> > lower down to the top level so it sounded like doing the reverse
> > would 
> > not be straightforward. Perhaps I overestimated it.
> 
> Ok, some more background info :)
> 
> Initially the HW access was also protected by struct_mutex but
> 4fc688ce7 added rps_lock for just the the HW access. So as I understand
> we don't need struct_mutex for anything else besides the GEM
> allocation. So feel free to add my ACK on the change moving the lock to
> vlv_setup_ctx().
> 
> Btw, we could also remove struct_mutex from intel_enable_gt_powersave()
> where it's taken for old platforms and rely on mchdev_lock or take
> rps_lock instead, but someone needs to double-check this.

For more context we needed dev->struct_mutex for ilk rc6 support, which
needed a powerctx allocated from gem (not just stolen). That support died
in fire in

commit a561165493e5fec2f74bd3ae0577ed659e44ab7f
Author: John Harrison <John.C.Harrison at Intel.com>
Date:   Thu Mar 5 14:03:03 2015 +0000

    drm/i915: Remove ironlake rc6 support

Maybe reference that too, on top of the commit Imre dug out?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch