On Mon, Apr 13, 2015 at 02:37:41PM +0100, Tvrtko Ursulin wrote: > > On 04/13/2015 01:09 PM, Jani Nikula wrote: > > On Thu, 26 Mar 2015, Tvrtko Ursulin <tvrtko.ursu...@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.ursu...@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 <ch...@chris-wilson.co.uk> > >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursu...@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? > > It was pretty much bike shedding - I am happy with this version since it > has a single lock/unlock on the normal path, compared to one pair per > active display with what Ville wanted. > > Either approach makes for unclear code so needs a big comment anyway. > Which leaves only the exact placement of mutex_lock/unlock under discussion.
I don't see what's unclear about locking only around the call that needs the lock. > > If we want to spend this much time on this that is. > > Regards, > > Tvrtko -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
