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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx