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

Jani Nikula
2015-04-13T12:07:32Z

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