From 7dfbf8a07cf8c936b0d6cc810df6ae7923954d5b Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Tue, 30 Jun 2020 16:27:24 +0100 Subject: drm/i915: Skip stale object handle for debugfs per-file-stats As we close a handle GEM object, we update the drm_file's idr with an error^W NULL pointer to indicate the in-progress closure, and finally removing it. If we read the idr directly, we may then see an invalid object pointer, and in our debugfs per_file_stats() we therefore need to protect against the entry being invalid. [ 1016.651637] RIP: 0010:per_file_stats+0xe/0x16e [ 1016.651646] Code: d2 41 0f b6 8e 69 8c 00 00 48 89 df 48 c7 c6 7b 74 8c be 31 c0 e8 0c 89 cf ff eb d2 0f 1f 44 00 00 55 48 89 e5 41 57 41 56 53 <8b> 06 85 c0 0f 84 4d 01 00 00 49 89 d6 48 89 f3 3d ff ff ff 7f 73 [ 1016.651651] RSP: 0018:ffffad3a01337ba0 EFLAGS: 00010293 [ 1016.651656] RAX: 0000000000000018 RBX: ffff96fe040d65e0 RCX: 0000000000000002 [ 1016.651660] RDX: ffffad3a01337c50 RSI: 0000000000000000 RDI: 00000000000001e8 [ 1016.651663] RBP: ffffad3a01337bb8 R08: 0000000000000000 R09: 00000000000001c0 [ 1016.651667] R10: 0000000000000000 R11: ffffffffbdbe5fce R12: 0000000000000000 [ 1016.651671] R13: ffffffffbdbe5fce R14: ffffad3a01337c50 R15: 0000000000000001 [ 1016.651676] FS: 00007a597e2d7480(0000) GS:ffff96ff3bb00000(0000) knlGS:0000000000000000 [ 1016.651680] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 1016.651683] CR2: 0000000000000000 CR3: 0000000171fc2001 CR4: 00000000003606e0 [ 1016.651687] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 1016.651690] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 1016.651693] Call Trace: [ 1016.651693] Call Trace: [ 1016.651703] idr_for_each+0x8a/0xe8 [ 1016.651711] i915_gem_object_info+0x2a3/0x3eb [ 1016.651720] seq_read+0x162/0x3ca [ 1016.651727] full_proxy_read+0x5b/0x8d [ 1016.651733] __vfs_read+0x45/0x1bb [ 1016.651741] vfs_read+0xc9/0x15e [ 1016.651746] ksys_read+0x7e/0xde [ 1016.651752] do_syscall_64+0x54/0x68 [ 1016.651758] entry_SYSCALL_64_after_hwframe+0x44/0xa9 Reported-by: Guenter Roeck Fixes: a8c15954d64a ("drm/i915: Protect debugfs per_file_stats with RCU lock") Signed-off-by: Chris Wilson Cc: Mika Kuoppala Cc: Guenter Roeck Cc: stable@vger.kernel.org Reviewed-by: Mika Kuoppala Link: https://patchwork.freedesktop.org/patch/msgid/20200630152724.3734-1-chris@chris-wilson.co.uk (cherry picked from commit c1b9fd3d310177b31621d5e661f06885869cae12) Signed-off-by: Rodrigo Vivi --- drivers/gpu/drm/i915/i915_debugfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index bca036ac6621..e7532e7d74e9 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -230,7 +230,7 @@ static int per_file_stats(int id, void *ptr, void *data) struct file_stats *stats = data; struct i915_vma *vma; - if (!kref_get_unless_zero(&obj->base.refcount)) + if (IS_ERR_OR_NULL(obj) || !kref_get_unless_zero(&obj->base.refcount)) return 0; stats->count++; -- cgit v1.2.3-58-ga151 From 9eb0463cfe65d826c97fa26b904a64f52c94300d Mon Sep 17 00:00:00 2001 From: Ville Syrjälä Date: Wed, 29 Apr 2020 13:10:25 +0300 Subject: drm/i915/fbc: Fix fence_y_offset handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The current fence_y_offset calculation is broken. I think it more or less used to do the right thing, but then I changed the plane code to put the final x/y source offsets back into the src rectangle so now it's just subtraacting the same value from itself. The code would never have worked if we allowed the framebuffer to have a non-zero offset. Let's do this in a better way by just calculating the fence_y_offset from the final plane surface offset. Note that we don't align the plane surface address to fence rows so with horizontal panning there's often a horizontal offset from the fence start to the surface address as well. We have no way to tell the hardware about that so we just ignore it. Based on some quick tests the invlidation still happens correctly. I presume due to the invalidation nuking at least the full line (or a segment of multiple lines). Fixes: 54d4d719fa11 ("drm/i915: Overcome display engine stride limits via GTT remapping") Signed-off-by: Ville Syrjälä Link: https://patchwork.freedesktop.org/patch/msgid/20200429101034.8208-4-ville.syrjala@linux.intel.com Reviewed-by: Matt Roper (cherry picked from commit 5331889b5ffb11d6257953e418291a9f04c02bed) Signed-off-by: Rodrigo Vivi --- drivers/gpu/drm/i915/display/intel_display.c | 11 ++++++++++ drivers/gpu/drm/i915/display/intel_display.h | 1 + drivers/gpu/drm/i915/display/intel_fbc.c | 32 +++++++++------------------- drivers/gpu/drm/i915/i915_drv.h | 6 +++--- 4 files changed, 25 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 9ea1a397d1b5..26996e1839e2 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -3822,6 +3822,17 @@ skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state, return true; } +unsigned int +intel_plane_fence_y_offset(const struct intel_plane_state *plane_state) +{ + int x = 0, y = 0; + + intel_plane_adjust_aligned_offset(&x, &y, plane_state, 0, + plane_state->color_plane[0].offset, 0); + + return y; +} + static int skl_check_main_surface(struct intel_plane_state *plane_state) { struct drm_i915_private *dev_priv = to_i915(plane_state->uapi.plane->dev); diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h index efb4da205ea2..3a06f72c9859 100644 --- a/drivers/gpu/drm/i915/display/intel_display.h +++ b/drivers/gpu/drm/i915/display/intel_display.h @@ -608,6 +608,7 @@ unsigned int i9xx_plane_max_stride(struct intel_plane *plane, u32 pixel_format, u64 modifier, unsigned int rotation); int bdw_get_pipemisc_bpp(struct intel_crtc *crtc); +unsigned int intel_plane_fence_y_offset(const struct intel_plane_state *plane_state); struct intel_display_error_state * intel_display_capture_error_state(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c index 1c26673acb2d..a65d9d8b79a7 100644 --- a/drivers/gpu/drm/i915/display/intel_fbc.c +++ b/drivers/gpu/drm/i915/display/intel_fbc.c @@ -47,19 +47,6 @@ #include "intel_fbc.h" #include "intel_frontbuffer.h" -/* - * In some platforms where the CRTC's x:0/y:0 coordinates doesn't match the - * frontbuffer's x:0/y:0 coordinates we lie to the hardware about the plane's - * origin so the x and y offsets can actually fit the registers. As a - * consequence, the fence doesn't really start exactly at the display plane - * address we program because it starts at the real start of the buffer, so we - * have to take this into consideration here. - */ -static unsigned int get_crtc_fence_y_offset(struct intel_fbc *fbc) -{ - return fbc->state_cache.plane.y - fbc->state_cache.plane.adjusted_y; -} - /* * For SKL+, the plane source size used by the hardware is based on the value we * write to the PLANE_SIZE register. For BDW-, the hardware looks at the value @@ -141,7 +128,7 @@ static void i8xx_fbc_activate(struct drm_i915_private *dev_priv) fbc_ctl2 |= FBC_CTL_CPU_FENCE; intel_de_write(dev_priv, FBC_CONTROL2, fbc_ctl2); intel_de_write(dev_priv, FBC_FENCE_OFF, - params->crtc.fence_y_offset); + params->fence_y_offset); } /* enable it... */ @@ -175,7 +162,7 @@ static void g4x_fbc_activate(struct drm_i915_private *dev_priv) if (params->fence_id >= 0) { dpfc_ctl |= DPFC_CTL_FENCE_EN | params->fence_id; intel_de_write(dev_priv, DPFC_FENCE_YOFF, - params->crtc.fence_y_offset); + params->fence_y_offset); } else { intel_de_write(dev_priv, DPFC_FENCE_YOFF, 0); } @@ -243,7 +230,7 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv) intel_de_write(dev_priv, SNB_DPFC_CTL_SA, SNB_CPU_FENCE_ENABLE | params->fence_id); intel_de_write(dev_priv, DPFC_CPU_FENCE_OFFSET, - params->crtc.fence_y_offset); + params->fence_y_offset); } } else { if (IS_GEN(dev_priv, 6)) { @@ -253,7 +240,7 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv) } intel_de_write(dev_priv, ILK_DPFC_FENCE_YOFF, - params->crtc.fence_y_offset); + params->fence_y_offset); /* enable it... */ intel_de_write(dev_priv, ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN); @@ -320,7 +307,7 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv) intel_de_write(dev_priv, SNB_DPFC_CTL_SA, SNB_CPU_FENCE_ENABLE | params->fence_id); intel_de_write(dev_priv, DPFC_CPU_FENCE_OFFSET, - params->crtc.fence_y_offset); + params->fence_y_offset); } else if (dev_priv->ggtt.num_fences) { intel_de_write(dev_priv, SNB_DPFC_CTL_SA, 0); intel_de_write(dev_priv, DPFC_CPU_FENCE_OFFSET, 0); @@ -631,8 +618,8 @@ static bool rotation_is_valid(struct drm_i915_private *dev_priv, /* * For some reason, the hardware tracking starts looking at whatever we * programmed as the display plane base address register. It does not look at - * the X and Y offset registers. That's why we look at the crtc->adjusted{x,y} - * variables instead of just looking at the pipe/plane size. + * the X and Y offset registers. That's why we include the src x/y offsets + * instead of just looking at the plane size. */ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) { @@ -705,7 +692,6 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, cache->plane.src_h = drm_rect_height(&plane_state->uapi.src) >> 16; cache->plane.adjusted_x = plane_state->color_plane[0].x; cache->plane.adjusted_y = plane_state->color_plane[0].y; - cache->plane.y = plane_state->uapi.src.y1 >> 16; cache->plane.pixel_blend_mode = plane_state->hw.pixel_blend_mode; @@ -713,6 +699,8 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, cache->fb.stride = fb->pitches[0]; cache->fb.modifier = fb->modifier; + cache->fence_y_offset = intel_plane_fence_y_offset(plane_state); + drm_WARN_ON(&dev_priv->drm, plane_state->flags & PLANE_HAS_FENCE && !plane_state->vma->fence); @@ -883,10 +871,10 @@ static void intel_fbc_get_reg_params(struct intel_crtc *crtc, memset(params, 0, sizeof(*params)); params->fence_id = cache->fence_id; + params->fence_y_offset = cache->fence_y_offset; params->crtc.pipe = crtc->pipe; params->crtc.i9xx_plane = to_intel_plane(crtc->base.primary)->i9xx_plane; - params->crtc.fence_y_offset = get_crtc_fence_y_offset(fbc); params->fb.format = cache->fb.format; params->fb.stride = cache->fb.stride; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index adb9bf34cf97..f79f118bf192 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -410,8 +410,6 @@ struct intel_fbc { int adjusted_x; int adjusted_y; - int y; - u16 pixel_blend_mode; } plane; @@ -420,6 +418,8 @@ struct intel_fbc { unsigned int stride; u64 modifier; } fb; + + unsigned int fence_y_offset; u16 gen9_wa_cfb_stride; s8 fence_id; } state_cache; @@ -435,7 +435,6 @@ struct intel_fbc { struct { enum pipe pipe; enum i9xx_plane_id i9xx_plane; - unsigned int fence_y_offset; } crtc; struct { @@ -444,6 +443,7 @@ struct intel_fbc { } fb; int cfb_size; + unsigned int fence_y_offset; u16 gen9_wa_cfb_stride; s8 fence_id; bool plane_visible; -- cgit v1.2.3-58-ga151 From 42723673a193d5f8e30dba6ea9826d42262a502b Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Thu, 2 Jul 2020 09:32:03 +0100 Subject: drm/i915: Drop vm.ref for duplicate vma on construction As we allow for parallel threads to create the same vma instance concurrently, and we only filter out the duplicates upon reacquiring the spinlock for the rbtree, we have to free the loser of the constructors' race. When freeing, we should also drop any resource references acquired for the redundant vma. Fixes: 2850748ef876 ("drm/i915: Pull i915_vma_pin under the vm->mutex") Signed-off-by: Chris Wilson Cc: Tvrtko Ursulin Cc: # v5.5+ Reviewed-by: Tvrtko Ursulin Link: https://patchwork.freedesktop.org/patch/msgid/20200702083225.20044-1-chris@chris-wilson.co.uk (cherry picked from commit 2377427cdd2b7514eb4c40241cf5c4dec63c1bec) Signed-off-by: Rodrigo Vivi --- drivers/gpu/drm/i915/i915_vma.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index fc14ebf9a0b7..2d60fd1f3637 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -198,6 +198,7 @@ vma_create(struct drm_i915_gem_object *obj, cmp = i915_vma_compare(pos, vm, view); if (cmp == 0) { spin_unlock(&obj->vma.lock); + i915_vm_put(vm); i915_vma_free(vma); return pos; } -- cgit v1.2.3-58-ga151 From cf1976b11372cac3b57fbae1831f66a4486355d3 Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Thu, 2 Jul 2020 22:10:15 +0100 Subject: drm/i915: Also drop vm.ref along error paths for vma construction Not only do we need to release the vm.ref we acquired for the vma on the duplicate insert branch, but also for the normal error paths, so roll them all into one. Reported-by: Andi Shyti Suggested-by: Andi Shyti Fixes: 2850748ef876 ("drm/i915: Pull i915_vma_pin under the vm->mutex") Signed-off-by: Chris Wilson Cc: Andi Shyti Cc: # v5.5+ Reviewed-by: Andi Shyti Link: https://patchwork.freedesktop.org/patch/msgid/20200702211015.29604-1-chris@chris-wilson.co.uk (cherry picked from commit 03fca66b7a36b52da8915341eee388267f6d5b73) Signed-off-by: Rodrigo Vivi --- drivers/gpu/drm/i915/i915_vma.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 2d60fd1f3637..1f9cd33b35cb 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -104,6 +104,7 @@ vma_create(struct drm_i915_gem_object *obj, struct i915_address_space *vm, const struct i915_ggtt_view *view) { + struct i915_vma *pos = ERR_PTR(-E2BIG); struct i915_vma *vma; struct rb_node *rb, **p; @@ -184,7 +185,6 @@ vma_create(struct drm_i915_gem_object *obj, rb = NULL; p = &obj->vma.tree.rb_node; while (*p) { - struct i915_vma *pos; long cmp; rb = *p; @@ -196,17 +196,12 @@ vma_create(struct drm_i915_gem_object *obj, * and dispose of ours. */ cmp = i915_vma_compare(pos, vm, view); - if (cmp == 0) { - spin_unlock(&obj->vma.lock); - i915_vm_put(vm); - i915_vma_free(vma); - return pos; - } - if (cmp < 0) p = &rb->rb_right; - else + else if (cmp > 0) p = &rb->rb_left; + else + goto err_unlock; } rb_link_node(&vma->obj_node, rb, p); rb_insert_color(&vma->obj_node, &obj->vma.tree); @@ -229,8 +224,9 @@ vma_create(struct drm_i915_gem_object *obj, err_unlock: spin_unlock(&obj->vma.lock); err_vma: + i915_vm_put(vm); i915_vma_free(vma); - return ERR_PTR(-E2BIG); + return pos; } static struct i915_vma * -- cgit v1.2.3-58-ga151 From 5a383d443b29a140094430f3ad1d02fa1acc2b80 Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Mon, 6 Jul 2020 18:01:38 +0100 Subject: drm/i915/gt: Pin the rings before marking active On eviction, we acquire the vm->mutex and then wait on the vma->active. Therefore when binding and pinning the vma, we must follow the same sequence, lock/pin the vma then mark it active. Otherwise, we mark the vma as active, then wait for the vm->mutex, and meanwhile the evictor holding the mutex waits upon us to complete our activity. Fixes: 8ccfc20a7d56 ("drm/i915/gt: Mark ring->vma as active while pinned") Signed-off-by: Chris Wilson Cc: Tvrtko Ursulin Cc: # v5.6+ Reviewed-by: Matthew Auld Link: https://patchwork.freedesktop.org/patch/msgid/20200706170138.8993-1-chris@chris-wilson.co.uk (cherry picked from commit 8567774e87e23a57155e5102f81208729b992ae6) Signed-off-by: Rodrigo Vivi --- drivers/gpu/drm/i915/gt/intel_context.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c index e4aece20bc80..52db2bde44a3 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.c +++ b/drivers/gpu/drm/i915/gt/intel_context.c @@ -204,25 +204,25 @@ static int __ring_active(struct intel_ring *ring) { int err; - err = i915_active_acquire(&ring->vma->active); + err = intel_ring_pin(ring); if (err) return err; - err = intel_ring_pin(ring); + err = i915_active_acquire(&ring->vma->active); if (err) - goto err_active; + goto err_pin; return 0; -err_active: - i915_active_release(&ring->vma->active); +err_pin: + intel_ring_unpin(ring); return err; } static void __ring_retire(struct intel_ring *ring) { - intel_ring_unpin(ring); i915_active_release(&ring->vma->active); + intel_ring_unpin(ring); } __i915_active_call -- cgit v1.2.3-58-ga151