Commit ab8d6675 authored by Tvrtko Ursulin's avatar Tvrtko Ursulin Committed by Daniel Vetter

drm/i915: Track old framebuffer instead of object

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: default avatarTvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
parent 3f678c96
...@@ -9055,9 +9055,9 @@ static void intel_unpin_work_fn(struct work_struct *__work) ...@@ -9055,9 +9055,9 @@ static void intel_unpin_work_fn(struct work_struct *__work)
enum pipe pipe = to_intel_crtc(work->crtc)->pipe; enum pipe pipe = to_intel_crtc(work->crtc)->pipe;
mutex_lock(&dev->struct_mutex); 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->pending_flip_obj->base);
drm_gem_object_unreference(&work->old_fb_obj->base); drm_framebuffer_unreference(work->old_fb);
intel_fbc_update(dev); intel_fbc_update(dev);
...@@ -9760,7 +9760,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, ...@@ -9760,7 +9760,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
work->event = event; work->event = event;
work->crtc = crtc; 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); INIT_WORK(&work->work, intel_unpin_work_fn);
ret = drm_crtc_vblank_get(crtc); ret = drm_crtc_vblank_get(crtc);
...@@ -9796,7 +9796,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, ...@@ -9796,7 +9796,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
goto cleanup; goto cleanup;
/* Reference the objects for the scheduled work. */ /* 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); drm_gem_object_reference(&obj->base);
crtc->primary->fb = fb; crtc->primary->fb = fb;
...@@ -9818,7 +9818,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, ...@@ -9818,7 +9818,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
if (IS_VALLEYVIEW(dev)) { if (IS_VALLEYVIEW(dev)) {
ring = &dev_priv->ring[BCS]; 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 */ /* vlv: DISPLAY_FLIP fails to change tiling */
ring = NULL; ring = NULL;
} else if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) { } else if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
...@@ -9859,7 +9859,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, ...@@ -9859,7 +9859,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe); work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe);
work->enable_stall_check = true; 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_FRONTBUFFER_PRIMARY(pipe));
intel_fbc_disable(dev); intel_fbc_disable(dev);
...@@ -9875,7 +9875,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, ...@@ -9875,7 +9875,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
cleanup_pending: cleanup_pending:
atomic_dec(&intel_crtc->unpin_work_count); atomic_dec(&intel_crtc->unpin_work_count);
crtc->primary->fb = old_fb; 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); drm_gem_object_unreference(&obj->base);
mutex_unlock(&dev->struct_mutex); mutex_unlock(&dev->struct_mutex);
......
...@@ -710,7 +710,7 @@ intel_get_crtc_for_plane(struct drm_device *dev, int plane) ...@@ -710,7 +710,7 @@ intel_get_crtc_for_plane(struct drm_device *dev, int plane)
struct intel_unpin_work { struct intel_unpin_work {
struct work_struct work; struct work_struct work;
struct drm_crtc *crtc; 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_i915_gem_object *pending_flip_obj;
struct drm_pending_vblank_event *event; struct drm_pending_vblank_event *event;
atomic_t pending; atomic_t pending;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment