| Message ID | 20160111150458.GI652@nuc-i3427.alporthouse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/01/16 15:04, Chris Wilson wrote: > On Mon, Jan 11, 2016 at 03:00:20PM +0000, Chris Wilson wrote: >> On Mon, Jan 11, 2016 at 02:47:17PM +0000, Tvrtko Ursulin wrote: >>> >>> On 11/01/16 14:38, Chris Wilson wrote: >>>> On Mon, Jan 11, 2016 at 02:08:41PM +0000, Tvrtko Ursulin wrote: >>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>> >>>>> VMA creation and GEM list management need the big lock. >>>>> >>>>> v2: >>>>> >>>>> Mutex unlock ended on the wrong path somehow. (0-day, Julia Lawall) >>>>> >>>>> Not to mention drm_gem_object_unreference was there in existing >>>>> code with no mutex held. >>>>> >>>>> v3: >>>>> >>>>> Some callers of i915_gem_object_create_stolen_for_preallocated >>>>> already hold the lock so move the mutex into the other caller >>>>> as well. >>>> >>>> intel_pm.c valleyview_setup_pctx? >>> >>> Already holds it traced by the WARN_ON at its top. >> >> Which is a nice little mutex inversion of its own. :| >> i.e. rpm vs struct_mutex bug > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index b1fb43fcfeea..c8f684f8799c 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -16018,9 +16018,7 @@ void intel_modeset_gem_init(struct drm_device *dev) > struct drm_crtc *c; > struct drm_i915_gem_object *obj; > > - mutex_lock(&dev->struct_mutex); > intel_init_gt_powersave(dev); > - mutex_unlock(&dev->struct_mutex); > > intel_modeset_init_hw(dev); > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index a082b4577599..90e5bf7a2402 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5224,8 +5224,6 @@ static void cherryview_setup_pctx(struct drm_device *dev) > u32 pcbr; > int pctx_size = 32*1024; > > - WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > - > pcbr = I915_READ(VLV_PCBR); > if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) { > DRM_DEBUG_DRIVER("BIOS didn't set up PCBR, fixing up\n"); > @@ -5247,8 +5245,7 @@ static void valleyview_setup_pctx(struct drm_device *dev) > u32 pcbr; > int pctx_size = 24*1024; > > - WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > - > + mutex_lock(&dev->struct_mutex); > pcbr = I915_READ(VLV_PCBR); > if (pcbr) { > /* BIOS set it up already, grab the pre-alloc'd space */ > @@ -5275,7 +5272,7 @@ static void valleyview_setup_pctx(struct drm_device *dev) > pctx = i915_gem_object_create_stolen(dev, pctx_size); > if (!pctx) { > DRM_DEBUG("not enough stolen space for PCTX, disabling\n"); > - return; > + goto out; > } > > pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->start; > @@ -5284,6 +5281,7 @@ static void valleyview_setup_pctx(struct drm_device *dev) > out: > DRM_DEBUG_DRIVER("PCBR: 0x%08x\n", I915_READ(VLV_PCBR)); > dev_priv->vlv_pctx = pctx; > + mutex_unlock(&dev->struct_mutex); > } > > static void valleyview_cleanup_pctx(struct drm_device *dev) 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. Regards, Tvrtko
On Mon, Jan 11, 2016 at 03:16:16PM +0000, Tvrtko Ursulin wrote: > > > On 11/01/16 15:04, Chris Wilson wrote: > > On Mon, Jan 11, 2016 at 03:00:20PM +0000, Chris Wilson wrote: > >> On Mon, Jan 11, 2016 at 02:47:17PM +0000, Tvrtko Ursulin wrote: > >>> > >>> On 11/01/16 14:38, Chris Wilson wrote: > >>>> On Mon, Jan 11, 2016 at 02:08:41PM +0000, Tvrtko Ursulin wrote: > >>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>>>> > >>>>> VMA creation and GEM list management need the big lock. > >>>>> > >>>>> v2: > >>>>> > >>>>> Mutex unlock ended on the wrong path somehow. (0-day, Julia Lawall) > >>>>> > >>>>> Not to mention drm_gem_object_unreference was there in existing > >>>>> code with no mutex held. > >>>>> > >>>>> v3: > >>>>> > >>>>> Some callers of i915_gem_object_create_stolen_for_preallocated > >>>>> already hold the lock so move the mutex into the other caller > >>>>> as well. > >>>> > >>>> intel_pm.c valleyview_setup_pctx? > >>> > >>> Already holds it traced by the WARN_ON at its top. > >> > >> Which is a nice little mutex inversion of its own. :| > >> i.e. rpm vs struct_mutex bug > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index b1fb43fcfeea..c8f684f8799c 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -16018,9 +16018,7 @@ void intel_modeset_gem_init(struct drm_device *dev) > > struct drm_crtc *c; > > struct drm_i915_gem_object *obj; > > > > - mutex_lock(&dev->struct_mutex); > > intel_init_gt_powersave(dev); > > - mutex_unlock(&dev->struct_mutex); > > > > intel_modeset_init_hw(dev); > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index a082b4577599..90e5bf7a2402 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -5224,8 +5224,6 @@ static void cherryview_setup_pctx(struct drm_device *dev) > > u32 pcbr; > > int pctx_size = 32*1024; > > > > - WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > > - > > pcbr = I915_READ(VLV_PCBR); > > if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) { > > DRM_DEBUG_DRIVER("BIOS didn't set up PCBR, fixing up\n"); > > @@ -5247,8 +5245,7 @@ static void valleyview_setup_pctx(struct drm_device *dev) > > u32 pcbr; > > int pctx_size = 24*1024; > > > > - WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > > - > > + mutex_lock(&dev->struct_mutex); > > pcbr = I915_READ(VLV_PCBR); > > if (pcbr) { > > /* BIOS set it up already, grab the pre-alloc'd space */ > > @@ -5275,7 +5272,7 @@ static void valleyview_setup_pctx(struct drm_device *dev) > > pctx = i915_gem_object_create_stolen(dev, pctx_size); > > if (!pctx) { > > DRM_DEBUG("not enough stolen space for PCTX, disabling\n"); > > - return; > > + goto out; > > } > > > > pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->start; > > @@ -5284,6 +5281,7 @@ static void valleyview_setup_pctx(struct drm_device *dev) > > out: > > DRM_DEBUG_DRIVER("PCBR: 0x%08x\n", I915_READ(VLV_PCBR)); > > dev_priv->vlv_pctx = pctx; > > + mutex_unlock(&dev->struct_mutex); > > } > > > > static void valleyview_cleanup_pctx(struct drm_device *dev) > > 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. And yeah, doesn't look like it's really protecting anything in the chv function, so can just be dropped there. The cleanup path could use the same treatemnt I think.
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. -Chris
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? Regards, Tvrtko
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
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. So my patch fixes the issue of struct_mutex not being held in intel_alloc_initial_plane_obj while calling i915_gem_object_create_stolen_for_preallocated and I'll leave struct_mutex/rps.lock locking inversion in RPM to someone else. Regards, Tvrtko
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
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
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. Regards, Tvrtko
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. --Imre
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@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
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b1fb43fcfeea..c8f684f8799c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -16018,9 +16018,7 @@ void intel_modeset_gem_init(struct drm_device *dev) struct drm_crtc *c; struct drm_i915_gem_object *obj; - mutex_lock(&dev->struct_mutex); intel_init_gt_powersave(dev); - mutex_unlock(&dev->struct_mutex); intel_modeset_init_hw(dev); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index a082b4577599..90e5bf7a2402 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5224,8 +5224,6 @@ static void cherryview_setup_pctx(struct drm_device *dev) u32 pcbr; int pctx_size = 32*1024; - WARN_ON(!mutex_is_locked(&dev->struct_mutex)); - pcbr = I915_READ(VLV_PCBR); if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) { DRM_DEBUG_DRIVER("BIOS didn't set up PCBR, fixing up\n"); @@ -5247,8 +5245,7 @@ static void valleyview_setup_pctx(struct drm_device *dev) u32 pcbr; int pctx_size = 24*1024; - WARN_ON(!mutex_is_locked(&dev->struct_mutex)); - + mutex_lock(&dev->struct_mutex); pcbr = I915_READ(VLV_PCBR); if (pcbr) { /* BIOS set it up already, grab the pre-alloc'd space */ @@ -5275,7 +5272,7 @@ static void valleyview_setup_pctx(struct drm_device *dev) pctx = i915_gem_object_create_stolen(dev, pctx_size); if (!pctx) { DRM_DEBUG("not enough stolen space for PCTX, disabling\n"); - return; + goto out; } pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->start; @@ -5284,6 +5281,7 @@ static void valleyview_setup_pctx(struct drm_device *dev) out: DRM_DEBUG_DRIVER("PCBR: 0x%08x\n", I915_READ(VLV_PCBR)); dev_priv->vlv_pctx = pctx; + mutex_unlock(&dev->struct_mutex); } static void valleyview_cleanup_pctx(struct drm_device *dev)