Re: [PATCH 08/12] drm/i915: Fix struct mutex vs. RPS lock inversion

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: "Chris Wilson" <chris@chris-wilson.co.uk>,
	Intel-gfx@lists.freedesktop.org,
	"Tvrtko Ursulin" <tvrtko.ursulin@intel.com>,
	"Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Imre Deak" <imre.deak@intel.com>,
	"Daniel Vetter" <daniel.vetter@ffwll.ch>
Subject: Re: [PATCH 08/12] drm/i915: Fix struct mutex vs. RPS lock inversion
Date: Tue, 2 Feb 2016 14:13:43 +0000	[thread overview]
Message-ID: <56B0B997.60007@linux.intel.com> (raw)
In-Reply-To: <20160202131605.GL15851@nuc-i3427.alporthouse.com>

On 02/02/16 13:16, Chris Wilson wrote:
> On Tue, Feb 02, 2016 at 11:06:26AM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> RPS lock must be taken before the struct_mutex to avoid
>> locking inversion. So stop grabbing it for the whole
>> powersave initialization and instead only take it during
>> the sections which need it.
>>
>> Also, struct_mutex is not needed any more since dedicated
>> RPS lock was added in:
>>
>>     commit 4fc688ce79772496503d22263d61b071a8fb596e
>>     Author: Jesse Barnes <jbarnes@virtuousgeek.org>
>>     Date:   Fri Nov 2 11:14:01 2012 -0700
>>
>>         drm/i915: protect RPS/RC6 related accesses (including PCU) with a new mutex
>>
>> Based on prototype patch by Chris Wilson and a subsequent
>> mailing list discussion involving Ville, Imre, Chris and
>> Daniel.
>>
>> v2: More details in the commit.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 4 ----
>>   drivers/gpu/drm/i915/intel_pm.c      | 9 +++++----
>>   2 files changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 5018295cd92b..af0d33a3697a 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -15995,9 +15995,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
>>   	struct drm_i915_gem_object *obj;
>>   	int ret;
>>
>> -	mutex_lock(&dev->struct_mutex);
>>   	intel_init_gt_powersave(dev);
>> -	mutex_unlock(&dev->struct_mutex);
>>
>>   	intel_modeset_init_hw(dev);
>>
>> @@ -16077,9 +16075,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
>>
>>   	intel_cleanup_overlay(dev);
>>
>> -	mutex_lock(&dev->struct_mutex);
>>   	intel_cleanup_gt_powersave(dev);
>> -	mutex_unlock(&dev->struct_mutex);
>
> The whitespace no longer conveys meaning, it used to be to clearly mark
> the mutex section.

Guess I can remove more lines of code and get credits for that. :D

>> @@ -5235,6 +5233,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)
>> @@ -5244,8 +5243,10 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
>>   	if (WARN_ON(!dev_priv->vlv_pctx))
>>   		return;
>>
>> +	mutex_lock(&dev->struct_mutex);
>>   	drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
>>   	dev_priv->vlv_pctx = NULL;
>> +	mutex_unlock(&dev->struct_mutex);
>
> This made me smile.

Yeah mechanical- want unreference_unlocked instead?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx