* [PATCH] drm/i915: Track old framebuffer instead of object
@ 2015-02-02 15:44 Tvrtko Ursulin
2015-02-03 2:58 ` shuang.he
2015-02-03 12:10 ` Daniel Vetter
0 siblings, 2 replies; 3+ messages in thread
From: Tvrtko Ursulin @ 2015-02-02 15:44 UTC (permalink / raw)
To: Intel-gfx; +Cc: Daniel Vetter
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;
--
2.2.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/i915: Track old framebuffer instead of object
2015-02-02 15:44 [PATCH] drm/i915: Track old framebuffer instead of object Tvrtko Ursulin
@ 2015-02-03 2:58 ` shuang.he
2015-02-03 12:10 ` Daniel Vetter
1 sibling, 0 replies; 3+ messages in thread
From: shuang.he @ 2015-02-03 2:58 UTC (permalink / raw)
To: shuang.he, ethan.gao, intel-gfx, tvrtko.ursulin
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 '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/i915: Track old framebuffer instead of object
2015-02-02 15:44 [PATCH] drm/i915: Track old framebuffer instead of object Tvrtko Ursulin
2015-02-03 2:58 ` shuang.he
@ 2015-02-03 12:10 ` Daniel Vetter
1 sibling, 0 replies; 3+ messages in thread
From: Daniel Vetter @ 2015-02-03 12:10 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Daniel Vetter, Intel-gfx
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
--
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-02-03 12:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-02 15:44 [PATCH] drm/i915: Track old framebuffer instead of object Tvrtko Ursulin
2015-02-03 2:58 ` shuang.he
2015-02-03 12:10 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.