[PATCH 1/2] drm/i915: Move drm_framebuffer_unreference out of struct_mutex for flips

[PATCH 1/2] drm/i915: Move drm_framebuffer_unreference out of struct_mutex for flips

[Chris Wilson]

intel_user_framebuffer_destroy() requires the struct_mutex for its
object bookkeeping, so this means that all calls to
drm_framebuffer_reference must be held without that lock.

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

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89166
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3b0fe9f1f3c9..6e1da7da5cca 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9107,7 +9107,6 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	mutex_lock(&dev->struct_mutex);
 	intel_unpin_fb_obj(intel_fb_obj(work->old_fb));
 	drm_gem_object_unreference(&work->pending_flip_obj->base);
-	drm_framebuffer_unreference(work->old_fb);
 
 	intel_fbc_update(dev);
 
@@ -9116,6 +9115,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	mutex_unlock(&dev->struct_mutex);
 
 	intel_frontbuffer_flip_complete(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
+	drm_framebuffer_unreference(work->old_fb);
 
 	BUG_ON(atomic_read(&to_intel_crtc(work->crtc)->unpin_work_count) == 0);
 	atomic_dec(&to_intel_crtc(work->crtc)->unpin_work_count);
@@ -9778,10 +9778,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (atomic_read(&intel_crtc->unpin_work_count) >= 2)
 		flush_workqueue(dev_priv->wq);
 
-	ret = i915_mutex_lock_interruptible(dev);
-	if (ret)
-		goto cleanup;
-
 	/* Reference the objects for the scheduled work. */
 	drm_framebuffer_reference(work->old_fb);
 	drm_gem_object_reference(&obj->base);
@@ -9791,6 +9787,10 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 
 	work->pending_flip_obj = obj;
 
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		goto cleanup;
+
 	atomic_inc(&intel_crtc->unpin_work_count);
 	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
 
@@ -9855,13 +9855,14 @@ cleanup_unpin:
 	intel_unpin_fb_obj(obj);
 cleanup_pending:
 	atomic_dec(&intel_crtc->unpin_work_count);
+	mutex_unlock(&dev->struct_mutex);
+cleanup:
 	crtc->primary->fb = old_fb;
 	update_state_fb(crtc->primary);
+
+	drm_gem_object_unreference_unlocked(&obj->base);
 	drm_framebuffer_unreference(work->old_fb);
-	drm_gem_object_unreference(&obj->base);
-	mutex_unlock(&dev->struct_mutex);
 
-cleanup:
 	spin_lock_irq(&dev->event_lock);
 	intel_crtc->unpin_work = NULL;
 	spin_unlock_irq(&dev->event_lock);
-- 
2.1.4

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

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

[Chris Wilson]

intel_user_framebuffer_destroy() requires the struct_mutex for its
object bookkeeping, so this means that all calls to
drm_framebuffer_reference must be held without that lock.

References: https://bugs.freedesktop.org/show_bug.cgi?id=89166
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6e1da7da5cca..aba36662d511 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13672,6 +13672,8 @@ 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;
+	struct drm_plane *unused[I915_MAX_PIPES];
+	int n_unused = 0;
 
 	mutex_lock(&dev->struct_mutex);
 	intel_init_gt_powersave(dev);
@@ -13707,13 +13709,18 @@ void intel_modeset_gem_init(struct drm_device *dev)
 					       NULL)) {
 			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);
+			unused[n_unused++] = c->primary;
 		}
 	}
 	mutex_unlock(&dev->struct_mutex);
 
+	while (n_unused--) {
+		struct drm_plane *p = unused[n_unused];
+		drm_framebuffer_unreference(p->fb);
+		p->fb = NULL;
+		update_state_fb(p);
+	}
+
 	intel_backlight_register(dev);
 }
 
-- 
2.1.4

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

Re: [PATCH 2/2] 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: 5783
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -1              277/277              276/277
ILK                                  313/313              313/313
SNB                 -1              309/309              308/309
IVB                                  382/382              382/382
BYT                                  296/296              296/296
HSW                                  425/425              425/425
BDW                 -1              318/318              317/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_drm_vma_limiter_cached      NRUN(1)PASS(1)      NO_RESULT(1)PASS(1)
*SNB  igt_kms_plane_plane-position-covered-pipe-B-plane-1      PASS(2)      DMESG_WARN(1)PASS(1)
*BDW  igt_gem_gtt_hog      PASS(5)      DMESG_WARN(1)PASS(1)
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 1/2] drm/i915: Move drm_framebuffer_unreference out of struct_mutex for flips

[Daniel Vetter]

On Mon, Feb 16, 2015 at 02:31:49PM +0000, Chris Wilson wrote:
> intel_user_framebuffer_destroy() requires the struct_mutex for its
> object bookkeeping, so this means that all calls to
> drm_framebuffer_reference must be held without that lock.

                 ^unref... you mean I guess? Same type in patch 2.
> 
> 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
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89166
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Tvrtko can you please review these two?

Thanks, Daniel

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3b0fe9f1f3c9..6e1da7da5cca 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9107,7 +9107,6 @@ static void intel_unpin_work_fn(struct work_struct *__work)
>  	mutex_lock(&dev->struct_mutex);
>  	intel_unpin_fb_obj(intel_fb_obj(work->old_fb));
>  	drm_gem_object_unreference(&work->pending_flip_obj->base);
> -	drm_framebuffer_unreference(work->old_fb);
>  
>  	intel_fbc_update(dev);
>  
> @@ -9116,6 +9115,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	intel_frontbuffer_flip_complete(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
> +	drm_framebuffer_unreference(work->old_fb);
>  
>  	BUG_ON(atomic_read(&to_intel_crtc(work->crtc)->unpin_work_count) == 0);
>  	atomic_dec(&to_intel_crtc(work->crtc)->unpin_work_count);
> @@ -9778,10 +9778,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	if (atomic_read(&intel_crtc->unpin_work_count) >= 2)
>  		flush_workqueue(dev_priv->wq);
>  
> -	ret = i915_mutex_lock_interruptible(dev);
> -	if (ret)
> -		goto cleanup;
> -
>  	/* Reference the objects for the scheduled work. */
>  	drm_framebuffer_reference(work->old_fb);
>  	drm_gem_object_reference(&obj->base);
> @@ -9791,6 +9787,10 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  
>  	work->pending_flip_obj = obj;
>  
> +	ret = i915_mutex_lock_interruptible(dev);
> +	if (ret)
> +		goto cleanup;
> +
>  	atomic_inc(&intel_crtc->unpin_work_count);
>  	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
>  
> @@ -9855,13 +9855,14 @@ cleanup_unpin:
>  	intel_unpin_fb_obj(obj);
>  cleanup_pending:
>  	atomic_dec(&intel_crtc->unpin_work_count);
> +	mutex_unlock(&dev->struct_mutex);
> +cleanup:
>  	crtc->primary->fb = old_fb;
>  	update_state_fb(crtc->primary);
> +
> +	drm_gem_object_unreference_unlocked(&obj->base);
>  	drm_framebuffer_unreference(work->old_fb);
> -	drm_gem_object_unreference(&obj->base);
> -	mutex_unlock(&dev->struct_mutex);
>  
> -cleanup:
>  	spin_lock_irq(&dev->event_lock);
>  	intel_crtc->unpin_work = NULL;
>  	spin_unlock_irq(&dev->event_lock);
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Move drm_framebuffer_unreference out of struct_mutex for flips

[Tvrtko Ursulin]

On 02/16/2015 02:31 PM, Chris Wilson wrote:
> intel_user_framebuffer_destroy() requires the struct_mutex for its
> object bookkeeping, so this means that all calls to
> drm_framebuffer_reference must be held without that lock.

Maybe "drm_framebuffer_unreference must be made without that lock", hm, 
actually "requires the struct_mutex" is misleading, should be "will 
grab/take struct_mutex".

But code itself:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

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

[Tvrtko Ursulin]

Hi,

On 02/16/2015 02:31 PM, Chris Wilson wrote:
> intel_user_framebuffer_destroy() requires the struct_mutex for its
> object bookkeeping, so this means that all calls to
> drm_framebuffer_reference must be held without that lock.

Same comment on the commit message as 1/2.

> References: https://bugs.freedesktop.org/show_bug.cgi?id=89166
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6e1da7da5cca..aba36662d511 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13672,6 +13672,8 @@ 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;
> +	struct drm_plane *unused[I915_MAX_PIPES];
> +	int n_unused = 0;
>
>   	mutex_lock(&dev->struct_mutex);
>   	intel_init_gt_powersave(dev);
> @@ -13707,13 +13709,18 @@ void intel_modeset_gem_init(struct drm_device *dev)
>   					       NULL)) {
>   			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);
> +			unused[n_unused++] = c->primary;
>   		}
>   	}
>   	mutex_unlock(&dev->struct_mutex);
>
> +	while (n_unused--) {
> +		struct drm_plane *p = unused[n_unused];
> +		drm_framebuffer_unreference(p->fb);
> +		p->fb = NULL;
> +		update_state_fb(p);
> +	}
> +

For this one I am not sure. Should c->primary->fb = NULL remain under 
the locked loop? If not what is the the mutex protecting then?

Regards,

Tvrtko

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

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

[Chris Wilson]

On Tue, Mar 10, 2015 at 12:02:28PM +0000, Tvrtko Ursulin wrote:
> >@@ -13707,13 +13709,18 @@ void intel_modeset_gem_init(struct drm_device *dev)
> >  					       NULL)) {
> >  			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);
> >+			unused[n_unused++] = c->primary;
> >  		}
> >  	}
> >  	mutex_unlock(&dev->struct_mutex);
> >
> >+	while (n_unused--) {
> >+		struct drm_plane *p = unused[n_unused];
> >+		drm_framebuffer_unreference(p->fb);
> >+		p->fb = NULL;
> >+		update_state_fb(p);
> >+	}
> >+
> 
> For this one I am not sure. Should c->primary->fb = NULL remain
> under the locked loop? If not what is the the mutex protecting then?

It's a dummy mutex that only exists to keep the WARNs quiet. This phase
of initialisation is explicitly single-threaded.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 03/10/2015 12:19 PM, Chris Wilson wrote:
> On Tue, Mar 10, 2015 at 12:02:28PM +0000, Tvrtko Ursulin wrote:
>>> @@ -13707,13 +13709,18 @@ void intel_modeset_gem_init(struct drm_device *dev)
>>>   					       NULL)) {
>>>   			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);
>>> +			unused[n_unused++] = c->primary;
>>>   		}
>>>   	}
>>>   	mutex_unlock(&dev->struct_mutex);
>>>
>>> +	while (n_unused--) {
>>> +		struct drm_plane *p = unused[n_unused];
>>> +		drm_framebuffer_unreference(p->fb);
>>> +		p->fb = NULL;
>>> +		update_state_fb(p);
>>> +	}
>>> +
>>
>> For this one I am not sure. Should c->primary->fb = NULL remain
>> under the locked loop? If not what is the the mutex protecting then?
>
> It's a dummy mutex that only exists to keep the WARNs quiet. This phase
> of initialisation is explicitly single-threaded.

Would it be a simpler fix then to move the mutex only around 
pin_and_fence_fb_obj?

Regards,

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

Re: [PATCH 1/2] drm/i915: Move drm_framebuffer_unreference out of struct_mutex for flips

[Daniel Vetter]

On Tue, Mar 10, 2015 at 11:43:10AM +0000, Tvrtko Ursulin wrote:
> On 02/16/2015 02:31 PM, Chris Wilson wrote:
> >intel_user_framebuffer_destroy() requires the struct_mutex for its
> >object bookkeeping, so this means that all calls to
> >drm_framebuffer_reference must be held without that lock.
> 
> Maybe "drm_framebuffer_unreference must be made without that lock", hm,
> actually "requires the struct_mutex" is misleading, should be "will
> grab/take struct_mutex".

I've changed the commit message to "... must not hold that lock."
> 
> But code itself:
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Both merged, thanks.
-Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Tue, Mar 10, 2015 at 12:32:43PM +0000, Tvrtko Ursulin wrote:
> 
> On 03/10/2015 12:19 PM, Chris Wilson wrote:
> > On Tue, Mar 10, 2015 at 12:02:28PM +0000, Tvrtko Ursulin wrote:
> >>> @@ -13707,13 +13709,18 @@ void intel_modeset_gem_init(struct drm_device *dev)
> >>>   					       NULL)) {
> >>>   			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);
> >>> +			unused[n_unused++] = c->primary;
> >>>   		}
> >>>   	}
> >>>   	mutex_unlock(&dev->struct_mutex);
> >>>
> >>> +	while (n_unused--) {
> >>> +		struct drm_plane *p = unused[n_unused];
> >>> +		drm_framebuffer_unreference(p->fb);
> >>> +		p->fb = NULL;
> >>> +		update_state_fb(p);
> >>> +	}
> >>> +
> >>
> >> For this one I am not sure. Should c->primary->fb = NULL remain
> >> under the locked loop? If not what is the the mutex protecting then?
> >
> > It's a dummy mutex that only exists to keep the WARNs quiet. This phase
> > of initialisation is explicitly single-threaded.
> 
> Would it be a simpler fix then to move the mutex only around 
> pin_and_fence_fb_obj?

That would be much nicer indeed.

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