Re: [PATCH] drm/i915: Move drm_framebuffer_unreference out of struct_mutex for takeover

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 26 Mar 2015, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 03/26/2015 01:30 PM, Ville Syrjälä wrote:
>> On Thu, Mar 26, 2015 at 12:39:40PM +0000, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> intel_user_framebuffer_destroy() requires the struct_mutex for its
>>> object bookkeeping, so this means that all calls to
>>> drm_framebuffer_unreference must be held without that lock.
>>>
>>> This is a simplified version of the identically named patch by Chris Wilson.
>>>
>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=89166
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index cb50854..0788507 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -14020,11 +14020,21 @@ void intel_modeset_gem_init(struct drm_device *dev)
>>>   					       c->primary->fb,
>>>   					       c->primary->state,
>>>   					       NULL)) {
>>> +			/*
>>> +			 * We must drop struct_mutex when calling
>>> +			 * drm_framebuffer_unreference and it is safe to do so
>>> +			 * because it is not needed at this point anyway.
>>> +			 * At this stage the driver is still single-threaded and
>>> +			 * we are taking it only to silence a warning in
>>> +			 * intel_pin_and_fence_fb_obj.
>>> +			 */
>>> +			mutex_unlock(&dev->struct_mutex);
>>>   			DRM_ERROR("failed to pin boot fb on pipe %d\n",
>>>   				  to_intel_crtc(c)->pipe);
>>>   			drm_framebuffer_unreference(c->primary->fb);
>>>   			c->primary->fb = NULL;
>>>   			update_state_fb(c->primary);
>>> +			mutex_lock(&dev->struct_mutex);
>>>   		}
>>>   	}
>>>   	mutex_unlock(&dev->struct_mutex);
>>
>> Just grab the mutex around the pin_and_fence inside the loop. It doesn't
>> protect anything else.
>
> Well the comment says so, but this way it only grabs and releases it 
> once if there are multiple active crtcs and nothing fails. So I was 
> hoping the comment was enough to explain the reality, even though the 
> other option would be more obvious code strictly speaking.

Tvrtko & Ville, can you reach a solution on this one? Or is there a
new patch that I may have missed?

BR,
Jani.


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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx