Message ID | 1422891855-6293-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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 '*'
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 --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;