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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx