msm_framebuffer_cleanup() releases a framebuffer as soon as
drm_atomic_helper_cleanup_planes() runs. msm_atomic_commit_tail() waits
only for ->wait_flush() before that, which for video mode waits for
CTL_FLUSH to read back zero, ie. for the new configuration to be latched;
the frame in flight with the old one is still being fetched. Since
commit 111fdd2198e6 ("drm/msm: drm_gpuvm conversion") the unpin also
detaches the vma, so the display is left reading unmapped memory:
arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402,
iova=0x007eb100, fsynr=0x3f0023, cbfrsynra=0xc20, cb=27
Hand the retired framebuffer to the crtc instead and drop the pin and the
vma reference from a drm_vblank_work, as i915 does for its cursor
framebuffers. The work holds a reference on the framebuffer, and
drm_vblank_work_schedule() holds a vblank reference until it runs.
The pin count is dropped by the deferred work rather than by
->cleanup_fb(), so a framebuffer scanned out by several crtcs stays pinned
until the last of them has passed a vblank, not until the last one has
retired it. Deferring here also covers the async plane update path, which
has no commit tail at all: drm_atomic_helper_async_commit() programs the
hardware and drm_atomic_helper_unprepare_planes() releases the old
framebuffer straight away.
A crtc which is being switched off stops fetching: its encoders are
disabled first, and both DPU and MDP5 turn the timing engine off there
and wait for a vsync to latch it. No further vblank arrives to run the
pending works then, so keep them on a list per crtc and release them by
hand from msm_crtc_vblank_off(). A framebuffer retired once that has
happened is released directly. Whether the vblank is off is tracked
next to the list rather than read from crtc->state, which a following
commit may already have swapped in, and a work is added to the list and
scheduled under the same lock, so that msm_crtc_vblank_off() cannot free
it in between.
Fixes: 111fdd2198e6 ("drm/msm: drm_gpuvm conversion")
Assisted-by: LLM
Signed-off-by: Dmitry Baryshkov <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 4 +-
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 3 +-
drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 4 +-
drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 2 +-
drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 4 +-
drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 2 +-
drivers/gpu/drm/msm/msm_drv.h | 4 +-
drivers/gpu/drm/msm/msm_fb.c | 19 ++-
drivers/gpu/drm/msm/msm_kms.c | 142 +++++++++++++++++++++
drivers/gpu/drm/msm/msm_kms.h | 19 +++
11 files changed, 190 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 42d0a529b4d5..1f8fce366a1d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1213,7 +1213,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
}
/* Disable/save vblank irq handling */
- drm_crtc_vblank_off(crtc);
+ msm_crtc_vblank_off(crtc);
drm_for_each_encoder_mask(encoder, crtc->dev,
old_crtc_state->encoder_mask) {
@@ -1301,7 +1301,7 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
}
/* Enable/restore vblank irq handling */
- drm_crtc_vblank_on(crtc);
+ msm_crtc_vblank_on(crtc);
}
static bool dpu_crtc_needs_dirtyfb(struct drm_crtc_state *cstate)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 22433bfbea1e..5db33e49c345 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -615,7 +615,7 @@ static void dpu_encoder_phys_wb_cleanup_wb_job(struct
dpu_encoder_phys *phys_enc
if (!job->fb)
return;
- msm_framebuffer_cleanup(job->fb, false);
+ msm_framebuffer_cleanup(job->fb, NULL, false);
wb_enc->wb_job = NULL;
wb_enc->wb_conn = NULL;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 7b92082d35a6..0e986b533bf0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -684,7 +684,8 @@ static void dpu_plane_cleanup_fb(struct drm_plane *plane,
DPU_DEBUG_PLANE(pdpu, "FB[%u]\n", old_state->fb->base.id);
- msm_framebuffer_cleanup(old_state->fb, old_pstate->needs_dirtyfb);
+ msm_framebuffer_cleanup(old_state->fb, old_state->crtc,
+ old_pstate->needs_dirtyfb);
}
static int dpu_plane_check_inline_rotation(struct dpu_plane *pdpu,
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
index 57dfce58450b..2d73ac3d85f2 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
@@ -267,7 +267,7 @@ static void mdp4_crtc_atomic_disable(struct drm_crtc *crtc,
return;
/* Disable/save vblank irq handling before power is disabled */
- drm_crtc_vblank_off(crtc);
+ msm_crtc_vblank_off(crtc);
mdp_irq_unregister(&mdp4_kms->base, &mdp4_crtc->err);
mdp4_disable(mdp4_kms);
@@ -297,7 +297,7 @@ static void mdp4_crtc_atomic_enable(struct drm_crtc *crtc,
mdp4_enable(mdp4_kms);
/* Restore vblank irq handling after power is enabled */
- drm_crtc_vblank_on(crtc);
+ msm_crtc_vblank_on(crtc);
mdp_irq_register(&mdp4_kms->base, &mdp4_crtc->err);
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
index 9459f70ce0ba..5f669a02d798 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
@@ -97,7 +97,7 @@ static void mdp4_plane_cleanup_fb(struct drm_plane *plane,
return;
DBG("%s: cleanup: FB[%u]", mdp4_plane->name, fb->base.id);
- msm_framebuffer_cleanup(fb, false);
+ msm_framebuffer_cleanup(fb, old_state->crtc, false);
}
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
index 4c4a897fc1ee..e9e55927655e 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
@@ -499,7 +499,7 @@ static void mdp5_crtc_atomic_disable(struct drm_crtc *crtc,
return;
/* Disable/save vblank irq handling before power is disabled */
- drm_crtc_vblank_off(crtc);
+ msm_crtc_vblank_off(crtc);
if (mdp5_cstate->cmd_mode)
mdp_irq_unregister(&mdp5_kms->base, &mdp5_crtc->pp_done);
@@ -527,7 +527,7 @@ static void mdp5_crtc_vblank_on(struct drm_crtc *crtc)
count = intf->mode == MDP5_INTF_DSI_MODE_COMMAND ? 0 : 0xffffffff;
drm_crtc_set_max_vblank_count(crtc, count);
- drm_crtc_vblank_on(crtc);
+ msm_crtc_vblank_on(crtc);
}
static void mdp5_crtc_atomic_enable(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
index 841f444a8d68..dacb387d9bb6 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
@@ -155,7 +155,7 @@ static void mdp5_plane_cleanup_fb(struct drm_plane *plane,
return;
DBG("%s: cleanup: FB[%u]", plane->name, fb->base.id);
- msm_framebuffer_cleanup(fb, needed_dirtyfb);
+ msm_framebuffer_cleanup(fb, old_state->crtc, needed_dirtyfb);
}
static int mdp5_plane_atomic_check_with_state(struct drm_crtc_state
*crtc_state,
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 2840e8a5052d..9fad2c191ff2 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -254,7 +254,9 @@ int msm_gem_prime_pin(struct drm_gem_object *obj);
void msm_gem_prime_unpin(struct drm_gem_object *obj);
int msm_framebuffer_prepare(struct drm_framebuffer *fb, bool needs_dirtyfb);
-void msm_framebuffer_cleanup(struct drm_framebuffer *fb, bool needed_dirtyfb);
+void msm_framebuffer_cleanup(struct drm_framebuffer *fb, struct drm_crtc *crtc,
+ bool needed_dirtyfb);
+void msm_framebuffer_unpin(struct drm_framebuffer *fb);
uint32_t msm_framebuffer_iova(struct drm_framebuffer *fb, int plane);
struct drm_gem_object *msm_framebuffer_bo(struct drm_framebuffer *fb, int
plane);
const struct msm_format *msm_framebuffer_format(struct drm_framebuffer *fb);
diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
index 792d47b4acc4..cc71c42d4e12 100644
--- a/drivers/gpu/drm/msm/msm_fb.c
+++ b/drivers/gpu/drm/msm/msm_fb.c
@@ -130,13 +130,10 @@ int msm_framebuffer_prepare(struct drm_framebuffer *fb,
bool needs_dirtyfb)
return 0;
}
-void msm_framebuffer_cleanup(struct drm_framebuffer *fb, bool needed_dirtyfb)
+void msm_framebuffer_unpin(struct drm_framebuffer *fb)
{
struct msm_framebuffer *msm_fb = to_msm_framebuffer(fb);
- if (needed_dirtyfb)
- refcount_dec(&msm_fb->dirtyfb);
-
guard(mutex)(&msm_fb->lock);
if (--msm_fb->prepare_count)
@@ -146,6 +143,20 @@ void msm_framebuffer_cleanup(struct drm_framebuffer *fb,
bool needed_dirtyfb)
msm_framebuffer_unpin_planes(fb, fb->format->num_planes);
}
+void msm_framebuffer_cleanup(struct drm_framebuffer *fb, struct drm_crtc *crtc,
+ bool needed_dirtyfb)
+{
+ struct msm_framebuffer *msm_fb = to_msm_framebuffer(fb);
+
+ if (needed_dirtyfb)
+ refcount_dec(&msm_fb->dirtyfb);
+
+ if (crtc && msm_crtc_queue_fb_unpin(crtc, fb))
+ return;
+
+ msm_framebuffer_unpin(fb);
+}
+
uint32_t msm_framebuffer_iova(struct drm_framebuffer *fb, int plane)
{
struct msm_framebuffer *msm_fb = to_msm_framebuffer(fb);
diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
index 9a207c1e8583..d836d6dd7f3e 100644
--- a/drivers/gpu/drm/msm/msm_kms.c
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -11,8 +11,10 @@
#include <uapi/linux/sched/types.h>
#include <drm/drm_drv.h>
+#include <drm/drm_framebuffer.h>
#include <drm/drm_mode_config.h>
#include <drm/drm_vblank.h>
+#include <drm/drm_vblank_work.h>
#include <drm/clients/drm_client_setup.h>
#include "disp/msm_disp_snapshot.h"
@@ -165,6 +167,146 @@ void msm_crtc_disable_vblank(struct drm_crtc *crtc)
vblank_ctrl_queue_work(priv, crtc, false);
}
+struct msm_fb_unpin_work {
+ struct drm_vblank_work base;
+ struct list_head node;
+ struct msm_kms_fb_unpin *pending;
+ struct drm_framebuffer *fb;
+};
+
+static void msm_kms_fb_unpin_release(struct msm_fb_unpin_work *unpin)
+{
+ msm_framebuffer_unpin(unpin->fb);
+ drm_framebuffer_put(unpin->fb);
+ kfree(unpin);
+}
+
+static void msm_kms_fb_unpin_work(struct kthread_work *work)
+{
+ struct msm_fb_unpin_work *unpin =
+ container_of(to_drm_vblank_work(work), struct msm_fb_unpin_work,
+ base);
+ struct msm_kms_fb_unpin *pending = unpin->pending;
+
+ scoped_guard(spinlock, &pending->lock) {
+ if (list_empty(&unpin->node))
+ return;
+ list_del_init(&unpin->node);
+ }
+
+ msm_kms_fb_unpin_release(unpin);
+}
+
+void msm_crtc_vblank_on(struct drm_crtc *crtc)
+{
+ struct msm_drm_private *priv = crtc->dev->dev_private;
+ struct msm_kms *kms = priv->kms;
+ unsigned int idx = drm_crtc_index(crtc);
+
+ drm_crtc_vblank_on(crtc);
+
+ if (!kms || idx >= ARRAY_SIZE(kms->fb_unpin))
+ return;
+
+ scoped_guard(spinlock, &kms->fb_unpin[idx].lock)
+ kms->fb_unpin[idx].vblank_off = false;
+}
+
+/*
+ * The crtc stops fetching here, and with it the vblanks the pending works are
+ * waiting for, so release the framebuffers directly.
+ */
+static void msm_kms_fb_unpin_release_all(struct msm_kms_fb_unpin *pending)
+{
+ struct msm_fb_unpin_work *unpin;
+
+ scoped_guard(spinlock, &pending->lock)
+ pending->vblank_off = true;
+
+ for (;;) {
+ scoped_guard(spinlock, &pending->lock) {
+ unpin = list_first_entry_or_null(&pending->fbs,
+ typeof(*unpin), node);
+ if (unpin)
+ list_del_init(&unpin->node);
+ }
+
+ if (!unpin)
+ break;
+
+ drm_vblank_work_cancel_sync(&unpin->base);
+ msm_kms_fb_unpin_release(unpin);
+ }
+}
+
+void msm_crtc_vblank_off(struct drm_crtc *crtc)
+{
+ struct msm_drm_private *priv = crtc->dev->dev_private;
+ struct msm_kms *kms = priv->kms;
+ unsigned int idx = drm_crtc_index(crtc);
+
+ if (kms && idx < ARRAY_SIZE(kms->fb_unpin))
+ msm_kms_fb_unpin_release_all(&kms->fb_unpin[idx]);
+
+ drm_crtc_vblank_off(crtc);
+}
+
+/*
+ * Publish and schedule the work in one go under the lock, as
+ * msm_crtc_vblank_off() frees whatever it finds on the list.
+ */
+static bool msm_kms_fb_unpin_schedule(struct msm_kms_fb_unpin *pending,
+ struct msm_fb_unpin_work *unpin,
+ struct drm_crtc *crtc)
+{
+ guard(spinlock)(&pending->lock);
+
+ if (pending->vblank_off)
+ return false;
+
+ list_add_tail(&unpin->node, &pending->fbs);
+
+ if (drm_vblank_work_schedule(&unpin->base,
+ drm_crtc_vblank_count(crtc) + 1, true) ==
1)
+ return true;
+
+ list_del_init(&unpin->node);
+
+ return false;
+}
+
+bool msm_crtc_queue_fb_unpin(struct drm_crtc *crtc, struct drm_framebuffer *fb)
+{
+ struct msm_drm_private *priv = crtc->dev->dev_private;
+ struct msm_kms *kms = priv->kms;
+ unsigned int idx = drm_crtc_index(crtc);
+ struct msm_kms_fb_unpin *pending;
+ struct msm_fb_unpin_work *unpin;
+
+ if (!kms || idx >= ARRAY_SIZE(kms->fb_unpin))
+ return false;
+
+ pending = &kms->fb_unpin[idx];
+
+ unpin = kzalloc_obj(*unpin);
+ if (!unpin)
+ return false;
+
+ unpin->fb = fb;
+ unpin->pending = pending;
+ drm_framebuffer_get(fb);
+
+ drm_vblank_work_init(&unpin->base, crtc, msm_kms_fb_unpin_work);
+
+ if (msm_kms_fb_unpin_schedule(pending, unpin, crtc))
+ return true;
+
+ drm_framebuffer_put(fb);
+ kfree(unpin);
+
+ return false;
+}
+
static int msm_kms_fault_handler(void *arg, unsigned long iova, int flags,
void *data)
{
struct msm_kms *kms = arg;
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index b5e1cd38cd8a..a33c2a0db206 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -133,6 +133,13 @@ struct msm_drm_thread {
struct kthread_worker *worker;
};
+struct msm_kms_fb_unpin {
+ /* protects the list of framebuffers waiting for a vblank: */
+ spinlock_t lock;
+ struct list_head fbs;
+ bool vblank_off;
+};
+
struct msm_kms {
const struct msm_kms_funcs *funcs;
struct drm_device *dev;
@@ -168,8 +175,14 @@ struct msm_kms {
struct workqueue_struct *wq;
struct msm_drm_thread event_thread[MAX_CRTCS];
+
+ struct msm_kms_fb_unpin fb_unpin[MAX_CRTCS];
};
+bool msm_crtc_queue_fb_unpin(struct drm_crtc *crtc, struct drm_framebuffer
*fb);
+void msm_crtc_vblank_on(struct drm_crtc *crtc);
+void msm_crtc_vblank_off(struct drm_crtc *crtc);
+
static inline int msm_kms_init(struct msm_kms *kms,
const struct msm_kms_funcs *funcs)
{
@@ -179,6 +192,12 @@ static inline int msm_kms_init(struct msm_kms *kms,
for (i = 0; i < ARRAY_SIZE(kms->commit_lock); i++)
mutex_init(&kms->commit_lock[i]);
+ for (i = 0; i < ARRAY_SIZE(kms->fb_unpin); i++) {
+ spin_lock_init(&kms->fb_unpin[i].lock);
+ INIT_LIST_HEAD(&kms->fb_unpin[i].fbs);
+ kms->fb_unpin[i].vblank_off = true;
+ }
+
kms->funcs = funcs;
kms->wq = alloc_ordered_workqueue("msm", 0);
--
2.47.3
