diff mbox

drm/i915: Track old framebuffer instead of object

Message ID 1422891855-6293-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin Feb. 2, 2015, 3:44 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Daniel Vetter spotted a bug while reviewing some of my refactoring in this
are of the code. I'll quote:

"""
> @@ -9764,6 +9768,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	work->event = event;
>  	work->crtc = crtc;
>  	work->old_fb_obj = intel_fb_obj(old_fb);
> +	work->old_tiling_mode = to_intel_framebuffer(old_fb)->tiling_mode;

Hm, that's actually an interesting bugfix - currently userspace could be
sneaky and destroy the old fb immediately after the flip completes and the
change the tiling of the underlying object before the unpin work had a
chance to run (needs some fudgin with rt prios to starve workers to make
this work though).

Imo the right fix is to hold a reference onto the fb and not the
underlying gem object. With that tiling is guaranteed not to change.
"""

This patch tries to implement the above proposed change.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 14 +++++++-------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

Comments

Shuang He Feb. 3, 2015, 2:58 a.m. UTC | #1 
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5695
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  353/353              353/353
ILK                 -1              344/344              343/344
SNB                                  400/422              400/422
IVB              +1-1              485/487              485/487
BYT                                  296/296              296/296
HSW              +1-1              507/508              507/508
BDW                                  401/402              401/402
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt_kms_flip_flip-vs-panning      PASS(2, M26)      TIMEOUT(1, M26)
 IVB  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance      DMESG_WARN(8, M34M21)PASS(10, M4M34)      PASS(1, M34)
 IVB  igt_gem_storedw_batches_loop_secure-dispatch      DMESG_WARN(1, M34)PASS(8, M34M4)      DMESG_WARN(1, M34)
*HSW  igt_gem_pwrite_pread_display-copy-performance      PASS(6, M40M20)      DMESG_WARN(1, M40)
 HSW  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance      DMESG_WARN(2, M40)PASS(22, M40M20)      PASS(1, M40)
Note: You need to pay more attention to line start with '*'
Daniel Vetter Feb. 3, 2015, 12:10 p.m. UTC | #2 
On Mon, Feb 02, 2015 at 03:44:15PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Daniel Vetter spotted a bug while reviewing some of my refactoring in this
> are of the code. I'll quote:
> 
> """
> > @@ -9764,6 +9768,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >  	work->event = event;
> >  	work->crtc = crtc;
> >  	work->old_fb_obj = intel_fb_obj(old_fb);
> > +	work->old_tiling_mode = to_intel_framebuffer(old_fb)->tiling_mode;
> 
> Hm, that's actually an interesting bugfix - currently userspace could be
> sneaky and destroy the old fb immediately after the flip completes and the
> change the tiling of the underlying object before the unpin work had a
> chance to run (needs some fudgin with rt prios to starve workers to make
> this work though).
> 
> Imo the right fix is to hold a reference onto the fb and not the
> underlying gem object. With that tiling is guaranteed not to change.
> """
> 
> This patch tries to implement the above proposed change.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 +++++++-------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1a689b3..24904cc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9111,9 +9111,9 @@ static void intel_unpin_work_fn(struct work_struct *__work)
>  	enum pipe pipe = to_intel_crtc(work->crtc)->pipe;
>  
>  	mutex_lock(&dev->struct_mutex);
> -	intel_unpin_fb_obj(work->old_fb_obj);
> +	intel_unpin_fb_obj(intel_fb_obj(work->old_fb));
>  	drm_gem_object_unreference(&work->pending_flip_obj->base);
> -	drm_gem_object_unreference(&work->old_fb_obj->base);
> +	drm_framebuffer_unreference(work->old_fb);
>  
>  	intel_fbc_update(dev);
>  
> @@ -9816,7 +9816,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  
>  	work->event = event;
>  	work->crtc = crtc;
> -	work->old_fb_obj = intel_fb_obj(old_fb);
> +	work->old_fb = old_fb;
>  	INIT_WORK(&work->work, intel_unpin_work_fn);
>  
>  	ret = drm_crtc_vblank_get(crtc);
> @@ -9852,7 +9852,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  		goto cleanup;
>  
>  	/* Reference the objects for the scheduled work. */
> -	drm_gem_object_reference(&work->old_fb_obj->base);
> +	drm_framebuffer_reference(work->old_fb);
>  	drm_gem_object_reference(&obj->base);
>  
>  	crtc->primary->fb = fb;
> @@ -9867,7 +9867,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  
>  	if (IS_VALLEYVIEW(dev)) {
>  		ring = &dev_priv->ring[BCS];
> -		if (obj->tiling_mode != work->old_fb_obj->tiling_mode)
> +		if (obj->tiling_mode != intel_fb_obj(work->old_fb)->tiling_mode)
>  			/* vlv: DISPLAY_FLIP fails to change tiling */
>  			ring = NULL;
>  	} else if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
> @@ -9908,7 +9908,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe);
>  	work->enable_stall_check = true;
>  
> -	i915_gem_track_fb(work->old_fb_obj, obj,
> +	i915_gem_track_fb(intel_fb_obj(work->old_fb), obj,
>  			  INTEL_FRONTBUFFER_PRIMARY(pipe));
>  
>  	intel_fbc_disable(dev);
> @@ -9924,7 +9924,7 @@ cleanup_unpin:
>  cleanup_pending:
>  	atomic_dec(&intel_crtc->unpin_work_count);
>  	crtc->primary->fb = old_fb;
> -	drm_gem_object_unreference(&work->old_fb_obj->base);
> +	drm_framebuffer_unreference(work->old_fb);
>  	drm_gem_object_unreference(&obj->base);
>  	mutex_unlock(&dev->struct_mutex);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index de513ec..a8f895f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -712,7 +712,7 @@ intel_get_crtc_for_plane(struct drm_device *dev, int plane)
>  struct intel_unpin_work {
>  	struct work_struct work;
>  	struct drm_crtc *crtc;
> -	struct drm_i915_gem_object *old_fb_obj;
> +	struct drm_framebuffer *old_fb;
>  	struct drm_i915_gem_object *pending_flip_obj;
>  	struct drm_pending_vblank_event *event;
>  	atomic_t pending;

Random style comment: For internal structs/funcs we're slowing trying to
move over to intel_ structs/pointers. And somehow I now find intel_fb_obj
a misleading function. Anway queued for -next, thanks for the patch.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1a689b3..24904cc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9111,9 +9111,9 @@  static void intel_unpin_work_fn(struct work_struct *__work)
 	enum pipe pipe = to_intel_crtc(work->crtc)->pipe;
 
 	mutex_lock(&dev->struct_mutex);
-	intel_unpin_fb_obj(work->old_fb_obj);
+	intel_unpin_fb_obj(intel_fb_obj(work->old_fb));
 	drm_gem_object_unreference(&work->pending_flip_obj->base);
-	drm_gem_object_unreference(&work->old_fb_obj->base);
+	drm_framebuffer_unreference(work->old_fb);
 
 	intel_fbc_update(dev);
 
@@ -9816,7 +9816,7 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 
 	work->event = event;
 	work->crtc = crtc;
-	work->old_fb_obj = intel_fb_obj(old_fb);
+	work->old_fb = old_fb;
 	INIT_WORK(&work->work, intel_unpin_work_fn);
 
 	ret = drm_crtc_vblank_get(crtc);
@@ -9852,7 +9852,7 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		goto cleanup;
 
 	/* Reference the objects for the scheduled work. */
-	drm_gem_object_reference(&work->old_fb_obj->base);
+	drm_framebuffer_reference(work->old_fb);
 	drm_gem_object_reference(&obj->base);
 
 	crtc->primary->fb = fb;
@@ -9867,7 +9867,7 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 
 	if (IS_VALLEYVIEW(dev)) {
 		ring = &dev_priv->ring[BCS];
-		if (obj->tiling_mode != work->old_fb_obj->tiling_mode)
+		if (obj->tiling_mode != intel_fb_obj(work->old_fb)->tiling_mode)
 			/* vlv: DISPLAY_FLIP fails to change tiling */
 			ring = NULL;
 	} else if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
@@ -9908,7 +9908,7 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe);
 	work->enable_stall_check = true;
 
-	i915_gem_track_fb(work->old_fb_obj, obj,
+	i915_gem_track_fb(intel_fb_obj(work->old_fb), obj,
 			  INTEL_FRONTBUFFER_PRIMARY(pipe));
 
 	intel_fbc_disable(dev);
@@ -9924,7 +9924,7 @@  cleanup_unpin:
 cleanup_pending:
 	atomic_dec(&intel_crtc->unpin_work_count);
 	crtc->primary->fb = old_fb;
-	drm_gem_object_unreference(&work->old_fb_obj->base);
+	drm_framebuffer_unreference(work->old_fb);
 	drm_gem_object_unreference(&obj->base);
 	mutex_unlock(&dev->struct_mutex);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index de513ec..a8f895f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -712,7 +712,7 @@  intel_get_crtc_for_plane(struct drm_device *dev, int plane)
 struct intel_unpin_work {
 	struct work_struct work;
 	struct drm_crtc *crtc;
-	struct drm_i915_gem_object *old_fb_obj;
+	struct drm_framebuffer *old_fb;
 	struct drm_i915_gem_object *pending_flip_obj;
 	struct drm_pending_vblank_event *event;
 	atomic_t pending;