| Message ID | 1427373580-20512-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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. Regards, Tvrtko
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6061
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV -1 276/276 275/276
ILK 303/303 303/303
SNB 304/304 304/304
IVB 339/339 339/339
BYT 287/287 287/287
HSW 362/362 362/362
BDW 310/310 310/310
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*PNV igt@gem_userptr_blits@minor-unsync-interruptible PASS(2) DMESG_WARN(2)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/i915_gem_evict.c:#i915_gem_evict_vm[i915]()@WARNING:.* at .* i915_gem_evict_vm+0x
Note: You need to pay more attention to line start with '*'
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
On 04/13/2015 01:09 PM, Jani Nikula wrote: > 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? 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. If we want to spend this much time on this that is. Regards, Tvrtko
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.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? > > 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
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);