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

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

[Tvrtko Ursulin]

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.

    Regression from commit ab8d66752a9c28cd6c94fa173feacdfc1554aa03
    Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
    Date:   Mon Feb 2 15:44:15 2015 +0000

        drm/i915: Track old framebuffer instead of object

v2: Bikeshedding.

References: https://bugs.freedesktop.org/show_bug.cgi?id=89166
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 97922fb..5fb11bc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14739,6 +14739,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *c;
 	struct drm_i915_gem_object *obj;
+	int ret;
 
 	mutex_lock(&dev->struct_mutex);
 	intel_init_gt_powersave(dev);
@@ -14763,16 +14764,18 @@ void intel_modeset_gem_init(struct drm_device *dev)
 	 * pinned & fenced.  When we do the allocation it's too early
 	 * for this.
 	 */
-	mutex_lock(&dev->struct_mutex);
 	for_each_crtc(dev, c) {
 		obj = intel_fb_obj(c->primary->fb);
 		if (obj == NULL)
 			continue;
 
-		if (intel_pin_and_fence_fb_obj(c->primary,
-					       c->primary->fb,
-					       c->primary->state,
-					       NULL)) {
+		mutex_lock(&dev->struct_mutex);
+		ret = intel_pin_and_fence_fb_obj(c->primary,
+						 c->primary->fb,
+						 c->primary->state,
+						 NULL);
+		mutex_unlock(&dev->struct_mutex);
+		if (ret) {
 			DRM_ERROR("failed to pin boot fb on pipe %d\n",
 				  to_intel_crtc(c)->pipe);
 			drm_framebuffer_unreference(c->primary->fb);
@@ -14780,7 +14783,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
 			update_state_fb(c->primary);
 		}
 	}
-	mutex_unlock(&dev->struct_mutex);
 
 	intel_backlight_register(dev);
 }
-- 
2.3.5

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

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

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6186
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -4              276/276              272/276
ILK                                  301/301              301/301
SNB                 -22              316/316              294/316
IVB                 -1              328/328              327/328
BYT                                  285/285              285/285
HSW                                  394/394              394/394
BDW                                  321/321              321/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 PNV  igt@gem_userptr_blits@coherency-unsync      CRASH(2)PASS(4)      CRASH(1)PASS(1)
 PNV  igt@gen3_render_linear_blits      FAIL(4)PASS(7)      FAIL(1)PASS(1)
 PNV  igt@gen3_render_mixed_blits      FAIL(5)PASS(6)      FAIL(1)PASS(1)
 PNV  igt@gen3_render_tiledx_blits      FAIL(5)PASS(7)      FAIL(1)PASS(1)
 SNB  igt@kms_cursor_crc@cursor-size-change      NSPT(1)PASS(1)      NSPT(2)
 SNB  igt@kms_flip_event_leak      NSPT(1)PASS(1)      NSPT(2)
 SNB  igt@kms_mmio_vs_cs_flip@setcrtc_vs_cs_flip      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@kms_mmio_vs_cs_flip@setplane_vs_cs_flip      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@kms_rotation_crc@primary-rotation      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@kms_rotation_crc@sprite-rotation      NSPT(2)PASS(3)      NSPT(2)
 SNB  igt@pm_rpm@cursor      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@cursor-dpms      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@dpms-non-lpsp      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@drm-resources-equal      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@fences      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@fences-dpms      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@gem-execbuf      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@gem-mmap-cpu      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@gem-mmap-gtt      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@gem-pread      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@i2c      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@modeset-non-lpsp      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@modeset-non-lpsp-stress-no-wait      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@pci-d3-state      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@rte      NSPT(2)PASS(1)      NSPT(2)
 IVB  igt@gem_pwrite_pread@uncached-copy-performance      DMESG_WARN(1)PASS(7)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...blitter_ring_idle@Hangcheck timer elapsed... blitter ring idle
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Mon, Apr 13, 2015 at 04:03:03PM +0100, 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.
> 
>     Regression from commit ab8d66752a9c28cd6c94fa173feacdfc1554aa03
>     Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>     Date:   Mon Feb 2 15:44:15 2015 +0000
> 
>         drm/i915: Track old framebuffer instead of object
> 
> v2: Bikeshedding.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=89166
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 97922fb..5fb11bc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14739,6 +14739,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *c;
>  	struct drm_i915_gem_object *obj;
> +	int ret;
>  
>  	mutex_lock(&dev->struct_mutex);
>  	intel_init_gt_powersave(dev);
> @@ -14763,16 +14764,18 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  	 * pinned & fenced.  When we do the allocation it's too early
>  	 * for this.
>  	 */
> -	mutex_lock(&dev->struct_mutex);
>  	for_each_crtc(dev, c) {
>  		obj = intel_fb_obj(c->primary->fb);
>  		if (obj == NULL)
>  			continue;
>  
> -		if (intel_pin_and_fence_fb_obj(c->primary,
> -					       c->primary->fb,
> -					       c->primary->state,
> -					       NULL)) {
> +		mutex_lock(&dev->struct_mutex);
> +		ret = intel_pin_and_fence_fb_obj(c->primary,
> +						 c->primary->fb,
> +						 c->primary->state,
> +						 NULL);
> +		mutex_unlock(&dev->struct_mutex);
> +		if (ret) {
>  			DRM_ERROR("failed to pin boot fb on pipe %d\n",
>  				  to_intel_crtc(c)->pipe);
>  			drm_framebuffer_unreference(c->primary->fb);
> @@ -14780,7 +14783,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  			update_state_fb(c->primary);
>  		}
>  	}
> -	mutex_unlock(&dev->struct_mutex);
>  
>  	intel_backlight_register(dev);
>  }
> -- 
> 2.3.5

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Tue, 14 Apr 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Apr 13, 2015 at 04:03:03PM +0100, 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.
>> 
>>     Regression from commit ab8d66752a9c28cd6c94fa173feacdfc1554aa03
>>     Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>     Date:   Mon Feb 2 15:44:15 2015 +0000
>> 
>>         drm/i915: Track old framebuffer instead of object
>> 
>> v2: Bikeshedding.
>> 
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=89166
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pushed to drm-intel-next-fixes, thanks for the patch and review.

BR,
Jani.


>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++------
>>  1 file changed, 8 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 97922fb..5fb11bc 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14739,6 +14739,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct drm_crtc *c;
>>  	struct drm_i915_gem_object *obj;
>> +	int ret;
>>  
>>  	mutex_lock(&dev->struct_mutex);
>>  	intel_init_gt_powersave(dev);
>> @@ -14763,16 +14764,18 @@ void intel_modeset_gem_init(struct drm_device *dev)
>>  	 * pinned & fenced.  When we do the allocation it's too early
>>  	 * for this.
>>  	 */
>> -	mutex_lock(&dev->struct_mutex);
>>  	for_each_crtc(dev, c) {
>>  		obj = intel_fb_obj(c->primary->fb);
>>  		if (obj == NULL)
>>  			continue;
>>  
>> -		if (intel_pin_and_fence_fb_obj(c->primary,
>> -					       c->primary->fb,
>> -					       c->primary->state,
>> -					       NULL)) {
>> +		mutex_lock(&dev->struct_mutex);
>> +		ret = intel_pin_and_fence_fb_obj(c->primary,
>> +						 c->primary->fb,
>> +						 c->primary->state,
>> +						 NULL);
>> +		mutex_unlock(&dev->struct_mutex);
>> +		if (ret) {
>>  			DRM_ERROR("failed to pin boot fb on pipe %d\n",
>>  				  to_intel_crtc(c)->pipe);
>>  			drm_framebuffer_unreference(c->primary->fb);
>> @@ -14780,7 +14783,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
>>  			update_state_fb(c->primary);
>>  		}
>>  	}
>> -	mutex_unlock(&dev->struct_mutex);
>>  
>>  	intel_backlight_register(dev);
>>  }
>> -- 
>> 2.3.5
>
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> 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

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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

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

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

[shuang.he]

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 '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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);
-- 
2.3.2

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