From patchwork Mon Jan 22 15:00:35 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [1/5] drm: Add drm_vblank_work_flush_all(). From: Chaitanya Kumar Borah X-Patchwork-Id: 575384 Message-Id: <20240122150039.2383484-2-chaitanya.kumar.borah@intel.com> To: intel-gfx-trybot@lists.freedesktop.org Date: Mon, 22 Jan 2024 20:30:35 +0530 From: Maarten Lankhorst In some cases we want to flush all vblank work, right before vblank_off for example. Add a simple function to make this possible. Signed-off-by: Maarten Lankhorst --- drivers/gpu/drm/drm_vblank_work.c | 22 ++++++++++++++++++++++ include/drm/drm_vblank_work.h | 2 ++ 2 files changed, 24 insertions(+) diff --git a/drivers/gpu/drm/drm_vblank_work.c b/drivers/gpu/drm/drm_vblank_work.c index 43cd5c0f4f6f..ff86f2b2e052 100644 --- a/drivers/gpu/drm/drm_vblank_work.c +++ b/drivers/gpu/drm/drm_vblank_work.c @@ -232,6 +232,28 @@ void drm_vblank_work_flush(struct drm_vblank_work *work) } EXPORT_SYMBOL(drm_vblank_work_flush); +/** + * drm_vblank_work_flush_all - flush all currently pending vblank work on crtc. + * @crtc: crtc for which vblank work to flush + * + * Wait until all currently queued vblank work on @crtc + * has finished executing once. + */ +void drm_vblank_work_flush_all(struct drm_crtc *crtc) +{ + struct drm_device *dev = crtc->dev; + struct drm_vblank_crtc *vblank = &dev->vblank[drm_crtc_index(crtc)]; + + spin_lock_irq(&dev->event_lock); + wait_event_lock_irq(vblank->work_wait_queue, + waitqueue_active(&vblank->work_wait_queue), + dev->event_lock); + spin_unlock_irq(&dev->event_lock); + + kthread_flush_worker(vblank->worker); +} +EXPORT_SYMBOL(drm_vblank_work_flush_all); + /** * drm_vblank_work_init - initialize a vblank work item * @work: vblank work item diff --git a/include/drm/drm_vblank_work.h b/include/drm/drm_vblank_work.h index eb41d0810c4f..e04d436b7297 100644 --- a/include/drm/drm_vblank_work.h +++ b/include/drm/drm_vblank_work.h @@ -17,6 +17,7 @@ struct drm_crtc; * drm_vblank_work_init() * drm_vblank_work_cancel_sync() * drm_vblank_work_flush() + * drm_vblank_work_flush_all() */ struct drm_vblank_work { /** @@ -67,5 +68,6 @@ void drm_vblank_work_init(struct drm_vblank_work *work, struct drm_crtc *crtc, void (*func)(struct kthread_work *work)); bool drm_vblank_work_cancel_sync(struct drm_vblank_work *work); void drm_vblank_work_flush(struct drm_vblank_work *work); +void drm_vblank_work_flush_all(struct drm_crtc *crtc); #endif /* !_DRM_VBLANK_WORK_H_ */ From patchwork Mon Jan 22 15:00:36 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [2/5] drm/i915: Use vblank worker to unpin old legacy cursor fb safely From: Chaitanya Kumar Borah X-Patchwork-Id: 575383 Message-Id: <20240122150039.2383484-3-chaitanya.kumar.borah@intel.com> To: intel-gfx-trybot@lists.freedesktop.org Date: Mon, 22 Jan 2024 20:30:36 +0530 From: Ville Syrjälä The cursor hardware only does sync updates, and thus the hardware will be scanning out from the old fb until the next start of vblank. So in order to make the legacy cursor fastpath actually safe we should not unpin the old fb until we're sure the hardware has ceased accessing it. The simplest approach is to just use a vblank work here to do the delayed unpin. Not 100% sure it's a good idea to put this onto the same high priority vblank worker as eg. our timing critical gamma updates. But let's keep it simple for now, and it we later discover that this is causing problems we can think about adding a lower priority worker for such things. This patch is slightly reworked by Maarten Cc: Maarten Lankhorst Signed-off-by: Ville Syrjälä Signed-off-by: Maarten Lankhorst --- drivers/gpu/drm/i915/display/intel_cursor.c | 26 +++++++++++++++++-- drivers/gpu/drm/i915/display/intel_display.c | 3 +++ .../drm/i915/display/intel_display_types.h | 3 +++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c index ecff90e233f0..d65c3b73df6c 100644 --- a/drivers/gpu/drm/i915/display/intel_cursor.c +++ b/drivers/gpu/drm/i915/display/intel_cursor.c @@ -653,6 +653,17 @@ static bool intel_cursor_format_mod_supported(struct drm_plane *_plane, return format == DRM_FORMAT_ARGB8888; } +static void intel_cursor_unpin_work(struct kthread_work *base) +{ + struct drm_vblank_work *work = to_drm_vblank_work(base); + struct intel_plane_state *plane_state = + container_of(work, typeof(*plane_state), unpin_work); + struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane); + + intel_plane_unpin_fb(plane_state); + intel_plane_destroy_state(&plane->base, &plane_state->uapi); +} + static int intel_legacy_cursor_update(struct drm_plane *_plane, struct drm_crtc *_crtc, @@ -780,14 +791,25 @@ intel_legacy_cursor_update(struct drm_plane *_plane, local_irq_enable(); - intel_plane_unpin_fb(old_plane_state); + if (old_plane_state->ggtt_vma != new_plane_state->ggtt_vma) { + drm_vblank_work_init(&old_plane_state->unpin_work, &crtc->base, + intel_cursor_unpin_work); + + drm_vblank_work_schedule(&old_plane_state->unpin_work, + drm_crtc_accurate_vblank_count(&crtc->base) + 1, + false); + + old_plane_state = NULL; + } else { + intel_plane_unpin_fb(old_plane_state); + } out_free: if (new_crtc_state) intel_crtc_destroy_state(&crtc->base, &new_crtc_state->uapi); if (ret) intel_plane_destroy_state(&plane->base, &new_plane_state->uapi); - else + else if (old_plane_state) intel_plane_destroy_state(&plane->base, &old_plane_state->uapi); return ret; diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index c5de4561f458..8dbb7bdc24de 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -64,6 +64,7 @@ #include "intel_crt.h" #include "intel_crtc.h" #include "intel_crtc_state_dump.h" +#include "intel_cursor.h" #include "intel_ddi.h" #include "intel_de.h" #include "intel_display_driver.h" @@ -6787,6 +6788,8 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state) continue; intel_crtc_disable_planes(state, crtc); + + drm_vblank_work_flush_all(&crtc->base); } /* Only disable port sync and MST slaves */ diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index ae2e8cff9d69..5fdb9eccab5a 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -713,6 +713,9 @@ struct intel_plane_state { struct intel_fb_view view; + /* for legacy cursor fb unpin */ + struct drm_vblank_work unpin_work; + /* Plane pxp decryption state */ bool decrypt; From patchwork Mon Jan 22 15:00:37 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [3/5] drm/i915: Use the same vblank worker for atomic unpin From: Chaitanya Kumar Borah X-Patchwork-Id: 575387 Message-Id: <20240122150039.2383484-4-chaitanya.kumar.borah@intel.com> To: intel-gfx-trybot@lists.freedesktop.org Date: Mon, 22 Jan 2024 20:30:37 +0530 From: Maarten Lankhorst In case of legacy cursor update, the cursor VMA needs to be unpinned only after vblank. This exceeds the lifetime of the whole atomic commit. Any trick I attempted to keep the atomic commit alive didn't work, as drm_atomic_helper_setup_commit() force throttles on any old commit that wasn't cleaned up. The only option remaining is to remove the plane from the atomic commit, and use the same path as the legacy cursor update to clean the state after vblank. Signed-off-by: Maarten Lankhorst --- .../gpu/drm/i915/display/intel_atomic_plane.c | 28 ++++++++++++++++++- .../gpu/drm/i915/display/intel_atomic_plane.h | 2 ++ drivers/gpu/drm/i915/display/intel_crtc.c | 28 +++++++++++++++++++ drivers/gpu/drm/i915/display/intel_cursor.c | 2 +- drivers/gpu/drm/i915/display/intel_cursor.h | 3 ++ 5 files changed, 61 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c index 06c2455bdd78..cb4153ca1867 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c @@ -42,6 +42,7 @@ #include "i915_reg.h" #include "intel_atomic_plane.h" #include "intel_cdclk.h" +#include "intel_cursor.h" #include "intel_display_rps.h" #include "intel_display_trace.h" #include "intel_display_types.h" @@ -1163,7 +1164,21 @@ intel_cleanup_plane_fb(struct drm_plane *plane, intel_display_rps_mark_interactive(dev_priv, state, false); - /* Should only be called after a successful intel_prepare_plane_fb()! */ + /* + * This branch can only ever be called after plane update is succesful, + * the error path will not cause unpin_work to be set. + */ + if (old_plane_state->unpin_work.vblank) { + int i = drm_plane_index(old_plane_state->uapi.plane); + + /* + * Remove plane from atomic commit, + * free is done from vblank worker + */ + memset(&state->base.planes[i], 0, sizeof(*state->base.planes)); + return; + } + intel_plane_unpin_fb(old_plane_state); } @@ -1176,3 +1191,14 @@ void intel_plane_helper_add(struct intel_plane *plane) { drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs); } + +void intel_plane_init_cursor_vblank_work(struct intel_plane_state *old_plane_state, + struct intel_plane_state *new_plane_state) +{ + if (!old_plane_state->ggtt_vma || + old_plane_state->ggtt_vma == new_plane_state->ggtt_vma) + return; + + drm_vblank_work_init(&old_plane_state->unpin_work, old_plane_state->uapi.crtc, + intel_cursor_unpin_work); +} diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h index 191dad0efc8e..5a897cf6fa02 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h @@ -66,5 +66,7 @@ int intel_plane_check_src_coordinates(struct intel_plane_state *plane_state); void intel_plane_set_invisible(struct intel_crtc_state *crtc_state, struct intel_plane_state *plane_state); void intel_plane_helper_add(struct intel_plane *plane); +void intel_plane_init_cursor_vblank_work(struct intel_plane_state *old_plane_state, + struct intel_plane_state *new_plane_state); #endif /* __INTEL_ATOMIC_PLANE_H__ */ diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c index 8a84a31c7b48..13fa7423b029 100644 --- a/drivers/gpu/drm/i915/display/intel_crtc.c +++ b/drivers/gpu/drm/i915/display/intel_crtc.c @@ -566,6 +566,19 @@ void intel_pipe_update_start(struct intel_atomic_state *state, if (intel_crtc_needs_vblank_work(new_crtc_state)) intel_crtc_vblank_work_init(new_crtc_state); + if (state->base.legacy_cursor_update) { + struct intel_plane *plane; + struct intel_plane_state *old_plane_state, *new_plane_state; + int i; + + for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, + new_plane_state, i) { + if (old_plane_state->uapi.crtc == &crtc->base) + intel_plane_init_cursor_vblank_work(old_plane_state, + new_plane_state); + } + } + intel_crtc_vblank_evade_scanlines(state, crtc, &min, &max, &vblank_start); if (min <= 0 || max <= 0) goto irq_disable; @@ -728,6 +741,21 @@ void intel_pipe_update_end(struct intel_atomic_state *state, new_crtc_state->uapi.event = NULL; } + if (state->base.legacy_cursor_update) { + struct intel_plane *plane; + struct intel_plane_state *old_plane_state; + int i; + + for_each_old_intel_plane_in_state(state, plane, old_plane_state, i) { + if (old_plane_state->uapi.crtc == &crtc->base && + old_plane_state->unpin_work.vblank) { + drm_vblank_work_schedule(&old_plane_state->unpin_work, + drm_crtc_accurate_vblank_count(&crtc->base) + 1, + false); + } + } + } + /* * Send VRR Push to terminate Vblank. If we are already in vblank * this has to be done _after_ sampling the frame counter, as diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c index d65c3b73df6c..2a1e7e03236c 100644 --- a/drivers/gpu/drm/i915/display/intel_cursor.c +++ b/drivers/gpu/drm/i915/display/intel_cursor.c @@ -653,7 +653,7 @@ static bool intel_cursor_format_mod_supported(struct drm_plane *_plane, return format == DRM_FORMAT_ARGB8888; } -static void intel_cursor_unpin_work(struct kthread_work *base) +void intel_cursor_unpin_work(struct kthread_work *base) { struct drm_vblank_work *work = to_drm_vblank_work(base); struct intel_plane_state *plane_state = diff --git a/drivers/gpu/drm/i915/display/intel_cursor.h b/drivers/gpu/drm/i915/display/intel_cursor.h index ce333bf4c2d5..e2d9ec710a86 100644 --- a/drivers/gpu/drm/i915/display/intel_cursor.h +++ b/drivers/gpu/drm/i915/display/intel_cursor.h @@ -9,9 +9,12 @@ enum pipe; struct drm_i915_private; struct intel_plane; +struct kthread_work; struct intel_plane * intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe); +void intel_cursor_unpin_work(struct kthread_work *base); + #endif From patchwork Mon Jan 22 15:00:38 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [4/5] drm/i915: do not destroy plane state if cursor unpin worker is scheduled From: Chaitanya Kumar Borah X-Patchwork-Id: 575385 Message-Id: <20240122150039.2383484-5-chaitanya.kumar.borah@intel.com> To: intel-gfx-trybot@lists.freedesktop.org Date: Mon, 22 Jan 2024 20:30:38 +0530 Don't destroy plane state if cursor unpin worker is scheduled Signed-off-by: Chaitanya Kumar Borah --- .../gpu/drm/i915/display/intel_atomic_plane.c | 27 ++++++++++++++++++- .../gpu/drm/i915/display/intel_atomic_plane.h | 2 ++ drivers/gpu/drm/i915/display/intel_crtc.c | 3 ++- drivers/gpu/drm/i915/display/intel_cursor.c | 6 ++++- 4 files changed, 35 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c index cb4153ca1867..a0a6388f0d92 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c @@ -136,6 +136,25 @@ intel_plane_destroy_state(struct drm_plane *plane, { struct intel_plane_state *plane_state = to_intel_plane_state(state); + if(plane_state->unpin_work.vblank) { + return; + } + + drm_WARN_ON(plane->dev, plane_state->ggtt_vma); + drm_WARN_ON(plane->dev, plane_state->dpt_vma); + + __drm_atomic_helper_plane_destroy_state(&plane_state->uapi); + if (plane_state->hw.fb) + drm_framebuffer_put(plane_state->hw.fb); + kfree(plane_state); +} + +void +intel_plane_destroy_state_custom(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct intel_plane_state *plane_state = to_intel_plane_state(state); + drm_WARN_ON(plane->dev, plane_state->ggtt_vma); drm_WARN_ON(plane->dev, plane_state->dpt_vma); @@ -1157,7 +1176,13 @@ intel_cleanup_plane_fb(struct drm_plane *plane, struct intel_atomic_state *state = to_intel_atomic_state(old_plane_state->uapi.state); struct drm_i915_private *dev_priv = to_i915(plane->dev); - struct drm_i915_gem_object *obj = intel_fb_obj(old_plane_state->hw.fb); + struct drm_i915_gem_object *obj = NULL; +// struct drm_i915_gem_object *obj = intel_fb_obj(old_plane_state->hw.fb); + + + if(!old_plane_state->unpin_work.vblank) { + obj = intel_fb_obj(old_plane_state->hw.fb); + } if (!obj) return; diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h index 5a897cf6fa02..24251cc91def 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h @@ -45,6 +45,8 @@ void intel_plane_free(struct intel_plane *plane); struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane); void intel_plane_destroy_state(struct drm_plane *plane, struct drm_plane_state *state); +void intel_plane_destroy_state_custom(struct drm_plane *plane, + struct drm_plane_state *state); void intel_crtc_planes_update_noarm(struct intel_atomic_state *state, struct intel_crtc *crtc); void intel_crtc_planes_update_arm(struct intel_atomic_state *state, diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c index 13fa7423b029..75499796e91e 100644 --- a/drivers/gpu/drm/i915/display/intel_crtc.c +++ b/drivers/gpu/drm/i915/display/intel_crtc.c @@ -573,9 +573,10 @@ void intel_pipe_update_start(struct intel_atomic_state *state, for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { - if (old_plane_state->uapi.crtc == &crtc->base) + if (old_plane_state->uapi.crtc == &crtc->base) { intel_plane_init_cursor_vblank_work(old_plane_state, new_plane_state); + } } } diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c index 2a1e7e03236c..92d3f3b72b0d 100644 --- a/drivers/gpu/drm/i915/display/intel_cursor.c +++ b/drivers/gpu/drm/i915/display/intel_cursor.c @@ -661,7 +661,8 @@ void intel_cursor_unpin_work(struct kthread_work *base) struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane); intel_plane_unpin_fb(plane_state); - intel_plane_destroy_state(&plane->base, &plane_state->uapi); + + intel_plane_destroy_state_custom(&plane->base, &plane_state->uapi); } static int @@ -721,6 +722,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane, !old_plane_state->uapi.fb != !fb) goto slow; + new_plane_state = to_intel_plane_state(intel_plane_duplicate_state(&plane->base)); if (!new_plane_state) return -ENOMEM; @@ -749,6 +751,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane, if (ret) goto out_free; + /* This is where ggtt_vma is assigned to new_plane_state */ ret = intel_plane_pin_fb(new_plane_state); if (ret) goto out_free; @@ -791,6 +794,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane, local_irq_enable(); + if (old_plane_state->ggtt_vma != new_plane_state->ggtt_vma) { drm_vblank_work_init(&old_plane_state->unpin_work, &crtc->base, intel_cursor_unpin_work); From patchwork Mon Jan 22 15:00:39 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [5/5] drm/i915: do not differ cleanup work From: Chaitanya Kumar Borah X-Patchwork-Id: 575386 Message-Id: <20240122150039.2383484-6-chaitanya.kumar.borah@intel.com> To: intel-gfx-trybot@lists.freedesktop.org Date: Mon, 22 Jan 2024 20:30:39 +0530 This is a test patch to check if the concept works in principle. Seems to be working locally. Signed-off-by: Chaitanya Kumar Borah --- drivers/gpu/drm/i915/display/intel_display.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 8dbb7bdc24de..c74a63556541 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -7013,10 +7013,8 @@ static void intel_atomic_commit_fence_wait(struct intel_atomic_state *intel_stat } } -static void intel_atomic_cleanup_work(struct work_struct *work) +static void intel_atomic_cleanup_work(struct intel_atomic_state *state) { - struct intel_atomic_state *state = - container_of(work, struct intel_atomic_state, base.commit_work); struct drm_i915_private *i915 = to_i915(state->base.dev); struct intel_crtc_state *old_crtc_state; struct intel_crtc *crtc; @@ -7288,8 +7286,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state) * schedule point (cond_resched()) here anyway to keep latencies * down. */ - INIT_WORK(&state->base.commit_work, intel_atomic_cleanup_work); - queue_work(system_highpri_wq, &state->base.commit_work); + + intel_atomic_cleanup_work(state); } static void intel_atomic_commit_work(struct work_struct *work)