* [PATCH v4 01/10] drm/msm: serialise framebuffer pin state
2026-10-03 0:30 [PATCH v4 00/10] drm/msm: fix SMMU fault dumps Dmitry Baryshkov
@ 2026-10-03 0:30 ` Dmitry Baryshkov
2026-10-03 0:30 ` [PATCH v4 02/10] drm/msm: fix framebuffer pin refcount leak on prepare failure Dmitry Baryshkov
` (8 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2026-10-03 0:30 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Antonino Maniscalco, Kalyan Thota, Federico Amedeo Izzo,
Helen Koike, Vignesh Raman, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel
prepare_count, iova[] and the pins they describe are updated locklessly,
but drm_atomic_helper_commit() prepares a new state in parallel with the
completion of the previous one -- stall_checks() only stalls on the second
previous commit. Commit N+1's ->prepare_fb() thus runs while commit N is
in ->cleanup_fb(), and for a double-buffered flip that is the same
framebuffer:
cleanup: prepare_count 1 -> 0
prepare: prepare_count 0 -> 1, pins, stores iova[]
cleanup: memset(iova, 0)
leaving the plane programmed with a NULL base address:
arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402,
iova=0x00000100, fsynr=0x3e0023, cbfrsynra=0x1c00, cb=11
The opposite order is broken too since commit 8ac37c88f991 ("drm/msm:
Refcount framebuffer pins"): a prepare which finds the count non-zero
returns at once, assuming iova[] is populated.
Both callbacks may sleep, so a mutex will do. drm_framebuffer_init()
adds the framebuffer to the object idr, from where userspace can reach it
before msm_framebuffer_init() returns, so take the private state out of
its way and initialise it first.
Fixes: 8ac37c88f991 ("drm/msm: Refcount framebuffer pins")
Assisted-by: LLM
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/msm_fb.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
index 60c108d35d2a..a38e5b3a1d1e 100644
--- a/drivers/gpu/drm/msm/msm_fb.c
+++ b/drivers/gpu/drm/msm/msm_fb.c
@@ -22,9 +22,12 @@ struct msm_framebuffer {
/* Count of # of attached planes which need dirtyfb: */
refcount_t dirtyfb;
+ /* Protects the pin state below: */
+ struct mutex lock;
+
/* Framebuffer per-plane address, if pinned, else zero: */
uint64_t iova[DRM_FORMAT_MAX_PLANES];
- atomic_t prepare_count;
+ unsigned int prepare_count;
};
#define to_msm_framebuffer(x) container_of(x, struct msm_framebuffer, base)
@@ -45,9 +48,17 @@ static int msm_framebuffer_dirtyfb(struct drm_framebuffer *fb,
clips, num_clips);
}
+static void msm_framebuffer_destroy(struct drm_framebuffer *fb)
+{
+ struct msm_framebuffer *msm_fb = to_msm_framebuffer(fb);
+
+ mutex_destroy(&msm_fb->lock);
+ drm_gem_fb_destroy(fb);
+}
+
static const struct drm_framebuffer_funcs msm_framebuffer_funcs = {
.create_handle = drm_gem_fb_create_handle,
- .destroy = drm_gem_fb_destroy,
+ .destroy = msm_framebuffer_destroy,
.dirty = msm_framebuffer_dirtyfb,
};
@@ -81,7 +92,9 @@ int msm_framebuffer_prepare(struct drm_framebuffer *fb, bool needs_dirtyfb)
if (needs_dirtyfb)
refcount_inc(&msm_fb->dirtyfb);
- if (atomic_inc_return(&msm_fb->prepare_count) > 1)
+ guard(mutex)(&msm_fb->lock);
+
+ if (msm_fb->prepare_count++)
return 0;
for (i = 0; i < n; i++) {
@@ -106,7 +119,9 @@ void msm_framebuffer_cleanup(struct drm_framebuffer *fb, bool needed_dirtyfb)
if (needed_dirtyfb)
refcount_dec(&msm_fb->dirtyfb);
- if (atomic_dec_return(&msm_fb->prepare_count))
+ guard(mutex)(&msm_fb->lock);
+
+ if (--msm_fb->prepare_count)
return;
memset(msm_fb->iova, 0, sizeof(msm_fb->iova));
@@ -199,14 +214,15 @@ msm_framebuffer_init(struct drm_device *dev, const struct drm_format_info *info,
drm_helper_mode_fill_fb_struct(dev, fb, info, mode_cmd);
+ refcount_set(&msm_fb->dirtyfb, 1);
+ mutex_init(&msm_fb->lock);
+
ret = drm_framebuffer_init(dev, fb, &msm_framebuffer_funcs);
if (ret) {
DRM_DEV_ERROR(dev->dev, "framebuffer init failed: %d\n", ret);
goto fail;
}
- refcount_set(&msm_fb->dirtyfb, 1);
-
drm_dbg_state(dev, "create: FB ID: %d (%p)\n", fb->base.id, fb);
return fb;
--
2.47.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 02/10] drm/msm: fix framebuffer pin refcount leak on prepare failure
2026-10-03 0:30 [PATCH v4 00/10] drm/msm: fix SMMU fault dumps Dmitry Baryshkov
2026-10-03 0:30 ` [PATCH v4 01/10] drm/msm: serialise framebuffer pin state Dmitry Baryshkov
@ 2026-10-03 0:30 ` Dmitry Baryshkov
2026-10-03 0:30 ` [PATCH v4 03/10] drm/msm: release scanout framebuffers only after a vblank Dmitry Baryshkov
` (7 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2026-10-03 0:30 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Antonino Maniscalco, Kalyan Thota, Federico Amedeo Izzo,
Helen Koike, Vignesh Raman, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel
msm_framebuffer_prepare() bumps prepare_count before pinning, but returns
straight out of the pin loop on error, leaking the count, the
msm_gem_vma_get() reference and any planes already pinned.
drm_atomic_helper_prepare_planes() does not call cleanup_fb() for the
plane whose prepare_fb() failed, so nothing ever drops it.
Since commit 8ac37c88f991 ("drm/msm: Refcount framebuffer pins") a
prepare which finds the count already non-zero returns early, assuming
iova[] is populated. With the count stuck, every later prepare of that
framebuffer reports success while iova[] is still zero, and DPU scans out
from a NULL base address:
arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402,
iova=0x00000100, fsynr=0x3e0023, cbfrsynra=0x1c00, cb=11
Unwind properly on failure instead.
Fixes: 8ac37c88f991 ("drm/msm: Refcount framebuffer pins")
Assisted-by: LLM
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/msm_fb.c | 32 +++++++++++++++++++++++---------
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
index a38e5b3a1d1e..792d47b4acc4 100644
--- a/drivers/gpu/drm/msm/msm_fb.c
+++ b/drivers/gpu/drm/msm/msm_fb.c
@@ -80,6 +80,18 @@ void msm_framebuffer_describe(struct drm_framebuffer *fb, struct seq_file *m)
}
#endif
+static void msm_framebuffer_unpin_planes(struct drm_framebuffer *fb, int n)
+{
+ struct msm_drm_private *priv = fb->dev->dev_private;
+ struct drm_gpuvm *vm = priv->kms->vm;
+ int i;
+
+ for (i = 0; i < n; i++) {
+ msm_gem_unpin_iova(fb->obj[i], vm);
+ msm_gem_vma_put(fb->obj[i]);
+ }
+}
+
/* prepare/pin all the fb's bo's for scanout.
*/
int msm_framebuffer_prepare(struct drm_framebuffer *fb, bool needs_dirtyfb)
@@ -102,8 +114,17 @@ int msm_framebuffer_prepare(struct drm_framebuffer *fb, bool needs_dirtyfb)
ret = msm_gem_get_and_pin_iova(fb->obj[i], vm, &msm_fb->iova[i]);
drm_dbg_state(fb->dev, "FB[%u]: iova[%d]: %08llx (%d)\n",
fb->base.id, i, msm_fb->iova[i], ret);
- if (ret)
+ if (ret) {
+ msm_gem_vma_put(fb->obj[i]);
+ msm_framebuffer_unpin_planes(fb, i);
+ memset(msm_fb->iova, 0, sizeof(msm_fb->iova));
+ msm_fb->prepare_count--;
+
+ if (needs_dirtyfb)
+ refcount_dec(&msm_fb->dirtyfb);
+
return ret;
+ }
}
return 0;
@@ -111,10 +132,7 @@ int msm_framebuffer_prepare(struct drm_framebuffer *fb, bool needs_dirtyfb)
void msm_framebuffer_cleanup(struct drm_framebuffer *fb, bool needed_dirtyfb)
{
- struct msm_drm_private *priv = fb->dev->dev_private;
- struct drm_gpuvm *vm = priv->kms->vm;
struct msm_framebuffer *msm_fb = to_msm_framebuffer(fb);
- int i, n = fb->format->num_planes;
if (needed_dirtyfb)
refcount_dec(&msm_fb->dirtyfb);
@@ -125,11 +143,7 @@ void msm_framebuffer_cleanup(struct drm_framebuffer *fb, bool needed_dirtyfb)
return;
memset(msm_fb->iova, 0, sizeof(msm_fb->iova));
-
- for (i = 0; i < n; i++) {
- msm_gem_unpin_iova(fb->obj[i], vm);
- msm_gem_vma_put(fb->obj[i]);
- }
+ msm_framebuffer_unpin_planes(fb, fb->format->num_planes);
}
uint32_t msm_framebuffer_iova(struct drm_framebuffer *fb, int plane)
--
2.47.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 03/10] drm/msm: release scanout framebuffers only after a vblank
2026-10-03 0:30 [PATCH v4 00/10] drm/msm: fix SMMU fault dumps Dmitry Baryshkov
2026-10-03 0:30 ` [PATCH v4 01/10] drm/msm: serialise framebuffer pin state Dmitry Baryshkov
2026-10-03 0:30 ` [PATCH v4 02/10] drm/msm: fix framebuffer pin refcount leak on prepare failure Dmitry Baryshkov
@ 2026-10-03 0:30 ` Dmitry Baryshkov
2026-10-03 1:34 ` sashiko-bot
2026-10-03 0:30 ` [PATCH v4 04/10] drm/msm/mdp5: flush the interface before starting the timing engine Dmitry Baryshkov
` (6 subsequent siblings)
9 siblings, 1 reply; 13+ messages in thread
From: Dmitry Baryshkov @ 2026-10-03 0:30 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Antonino Maniscalco, Kalyan Thota, Federico Amedeo Izzo,
Helen Koike, Vignesh Raman, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel
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, and its interface has
already been disabled by the time the helpers get to its planes, so no
further vblank arrives to run the pending works: 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 <dmitry.baryshkov@oss.qualcomm.com>
---
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 eb4bbae8557b..dc279a99e257 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 7e1df472592d..cb497d419ab8 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 517622367cc0..1764a1849f4e 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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v4 03/10] drm/msm: release scanout framebuffers only after a vblank
2026-10-03 0:30 ` [PATCH v4 03/10] drm/msm: release scanout framebuffers only after a vblank Dmitry Baryshkov
@ 2026-10-03 1:34 ` sashiko-bot
2026-10-08 13:52 ` Dmitry Baryshkov
0 siblings, 1 reply; 13+ messages in thread
From: sashiko-bot @ 2026-10-03 1:34 UTC (permalink / raw)
To: Dmitry Baryshkov; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm: premature unpin of framebuffers causing IOMMU fault
--
commit 86394032733ab2293f0e387866c3cbd603bc3285
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Subject: drm/msm: release scanout framebuffers only after a vblank
This commit defers scanout framebuffer release until a vblank by handing them
to a drm_vblank_work per CRTC. On CRTC disable, pending works are flushed and
framebuffers are unpinned directly from msm_crtc_vblank_off().
> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index 7e1df472592d3..cb497d419ab83 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c
[ ... ]
> +/*
> + * 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)
> +{
[ ... ]
> + 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);
> +}
[ ... ]
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 77a2d1f6700d0..35daf46fc4257 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);
[Severity: High]
Can this lead to an IOMMU context fault if the display hardware is still
fetching the frame?
When a userspace application issues an async plane update followed closely by
a CRTC disable, the ongoing hardware fetch might hit an IOMMU fault because
the memory is unmapped immediately during the disable sequence.
Because msm_crtc_vblank_off() now synchronously unpins and unmaps all pending
framebuffers via msm_kms_fb_unpin_release_all(), executing this before the
hardware is done fetching creates a race window.
Looking at dpu_crtc_disable(), we wait for the hardware to finish its current
frame fetch much later in the function:
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:dpu_crtc_disable() {
...
msm_crtc_vblank_off(crtc);
...
if (_dpu_crtc_wait_for_frame_done(crtc))
DPU_ERROR(...);
...
}
Does this sequence leave a race window where the display is left reading
unmapped memory?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261003-fd-kms-fix-smmu-v4-0-2a44d4d8ac77@oss.qualcomm.com?part=3
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4 03/10] drm/msm: release scanout framebuffers only after a vblank
2026-10-03 1:34 ` sashiko-bot
@ 2026-10-08 13:52 ` Dmitry Baryshkov
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2026-10-08 13:52 UTC (permalink / raw)
To: sashiko-reviews; +Cc: dri-devel
On Sat, Oct 03, 2026 at 01:34:29AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] drm/msm: premature unpin of framebuffers causing IOMMU fault
> --
>
> commit 86394032733ab2293f0e387866c3cbd603bc3285
> Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Subject: drm/msm: release scanout framebuffers only after a vblank
>
> This commit defers scanout framebuffer release until a vblank by handing them
> to a drm_vblank_work per CRTC. On CRTC disable, pending works are flushed and
> framebuffers are unpinned directly from msm_crtc_vblank_off().
>
> > diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> > index 7e1df472592d3..cb497d419ab83 100644
> > --- a/drivers/gpu/drm/msm/msm_kms.c
> > +++ b/drivers/gpu/drm/msm/msm_kms.c
> [ ... ]
> > +/*
> > + * 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)
> > +{
> [ ... ]
> > + 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);
> > +}
> [ ... ]
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index 77a2d1f6700d0..35daf46fc4257 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);
>
> [Severity: High]
> Can this lead to an IOMMU context fault if the display hardware is still
> fetching the frame?
>
> When a userspace application issues an async plane update followed closely by
> a CRTC disable, the ongoing hardware fetch might hit an IOMMU fault because
> the memory is unmapped immediately during the disable sequence.
>
> Because msm_crtc_vblank_off() now synchronously unpins and unmaps all pending
> framebuffers via msm_kms_fb_unpin_release_all(), executing this before the
> hardware is done fetching creates a race window.
>
> Looking at dpu_crtc_disable(), we wait for the hardware to finish its current
> frame fetch much later in the function:
>
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:dpu_crtc_disable() {
> ...
> msm_crtc_vblank_off(crtc);
> ...
> if (_dpu_crtc_wait_for_frame_done(crtc))
> DPU_ERROR(...);
> ...
> }
>
> Does this sequence leave a race window where the display is left reading
> unmapped memory?
I think it is not a problem: the atomic helpers disable the encoders
before the CRTCs. dpu_encoder_virt_atomic_disable() waits for the
pending kickoffs to complete and dpu_encoder_phys_vid_disable() turns
off the timing engine and waits for the vsync which latches the to the
hardware. For the MDP5 driver mdp5_vid_encoder_disable() does the same.
After that the interface no longer fetches when dpu_crtc_disable() calls
msm_crtc_vblank_off(). Any async update is flushed by ->wait_flush() at
the start of msm_atomic_commit_tail(), before the disables.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 04/10] drm/msm/mdp5: flush the interface before starting the timing engine
2026-10-03 0:30 [PATCH v4 00/10] drm/msm: fix SMMU fault dumps Dmitry Baryshkov
` (2 preceding siblings ...)
2026-10-03 0:30 ` [PATCH v4 03/10] drm/msm: release scanout framebuffers only after a vblank Dmitry Baryshkov
@ 2026-10-03 0:30 ` Dmitry Baryshkov
2026-10-03 0:30 ` [PATCH v4 05/10] drm/msm/dpu: clear the CWB blocks left by a previous reservation Dmitry Baryshkov
` (5 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2026-10-03 0:30 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Antonino Maniscalco, Kalyan Thota, Federico Amedeo Izzo,
Helen Koike, Vignesh Raman, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel
Disabling a CRTC and then enabling it again can produce an SMMU fault on
MSM8996 as soon as the timing engine restarts, e.g. when fbcon takes over
after kms_atomic:
arm-smmu d00000.iommu: Unhandled context fault: fsr=0x402, iova=0x01fa6e00
The faulting address is inside the framebuffer that was scanned out
before the disable, which has been released since. The pipes keep that
address latched (CURRENT_SRC0_ADDR still points to the released buffer)
because the plane removal flushed after the timing engine stopped never
takes effect. On the next enable the pipes are programmed and flushed
before the encoder is enabled, but that flush doesn't include the
interface: the encoder enables the timing engine first and only then
flushes the INTF. The first frame is fetched with the latched pipe
configuration, from the released buffer.
Flush the interface before enabling the timing engine, the order used by
both the DPU driver and the downstream mdss driver. With the INTF flush
pending, the whole pending flush takes effect when the timing engine
starts and the first frame is fetched with the new configuration.
Fixes: 06c0dd96bfbb ("drm/msm: add mdp5/apq8x74")
Assisted-by: LLM
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_encoder.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_encoder.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_encoder.c
index eaba3b2d73b5..e9eb1db92775 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_encoder.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_encoder.c
@@ -164,10 +164,11 @@ static void mdp5_vid_encoder_enable(struct drm_encoder *encoder)
if (WARN_ON(mdp5_encoder->enabled))
return;
+ /* the pipes fetch the first frame with stale addresses otherwise */
+ mdp5_ctl_commit(ctl, pipeline, mdp_ctl_flush_mask_encoder(intf), true);
spin_lock_irqsave(&mdp5_encoder->intf_lock, flags);
mdp5_write(mdp5_kms, REG_MDP5_INTF_TIMING_ENGINE_EN(intfn), 1);
spin_unlock_irqrestore(&mdp5_encoder->intf_lock, flags);
- mdp5_ctl_commit(ctl, pipeline, mdp_ctl_flush_mask_encoder(intf), true);
mdp5_ctl_set_encoder_state(ctl, pipeline, true);
--
2.47.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 05/10] drm/msm/dpu: clear the CWB blocks left by a previous reservation
2026-10-03 0:30 [PATCH v4 00/10] drm/msm: fix SMMU fault dumps Dmitry Baryshkov
` (3 preceding siblings ...)
2026-10-03 0:30 ` [PATCH v4 04/10] drm/msm/mdp5: flush the interface before starting the timing engine Dmitry Baryshkov
@ 2026-10-03 0:30 ` Dmitry Baryshkov
2026-10-03 0:30 ` [PATCH v4 06/10] drm/msm/dpu: clear the CDM block " Dmitry Baryshkov
` (4 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2026-10-03 0:30 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Antonino Maniscalco, Kalyan Thota, Federico Amedeo Izzo,
Helen Koike, Vignesh Raman, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel
dpu_encoder_virt_atomic_mode_set() only writes the first num_cwb entries
of dpu_enc->hw_cwb[], and not at all when the encoder is not the
writeback of a clone, so a modeset which drops the clone, or reserves
fewer CWB blocks, keeps the pointers from the previous reservation.
dpu_encoder_helper_phys_setup_cwb() then walks every non-NULL entry: it
programs a CWB mux the crtc no longer owns, and looks its pingpong up past
the pingpongs actually assigned, in the uninitialised part of rt_pp_list[].
Store NULL for the entries which have no CWB block, as is done for the
pingpongs.
Fixes: 5008375443ed ("drm/msm/dpu: Reserve resources for CWB")
Assisted-by: LLM
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1f20695f81e3..f06b52579740 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1205,9 +1205,10 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
ARRAY_SIZE(hw_pp));
}
- for (i = 0; i < num_cwb; i++) {
- dpu_enc->hw_cwb[i] = to_dpu_hw_cwb(hw_cwb[i]);
- cwb_mask |= BIT(dpu_enc->hw_cwb[i]->idx - CWB_0);
+ for (i = 0; i < MAX_CWB_PER_ENC; i++) {
+ dpu_enc->hw_cwb[i] = i < num_cwb ? to_dpu_hw_cwb(hw_cwb[i]) : NULL;
+ if (dpu_enc->hw_cwb[i])
+ cwb_mask |= BIT(dpu_enc->hw_cwb[i]->idx - CWB_0);
}
dpu_enc->cwb_mask = cwb_mask;
--
2.47.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 06/10] drm/msm/dpu: clear the CDM block left by a previous reservation
2026-10-03 0:30 [PATCH v4 00/10] drm/msm: fix SMMU fault dumps Dmitry Baryshkov
` (4 preceding siblings ...)
2026-10-03 0:30 ` [PATCH v4 05/10] drm/msm/dpu: clear the CWB blocks left by a previous reservation Dmitry Baryshkov
@ 2026-10-03 0:30 ` Dmitry Baryshkov
2026-10-03 0:30 ` [PATCH v4 07/10] drm/msm/dpu: clear the DSPP pointer when no DSPP is assigned Dmitry Baryshkov
` (3 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2026-10-03 0:30 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Antonino Maniscalco, Kalyan Thota, Federico Amedeo Izzo,
Helen Koike, Vignesh Raman, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel
dpu_encoder_virt_atomic_mode_set() only updates cur_master->hw_cdm for a
writeback with a job or for DP, so a modeset of a writeback without a job
keeps the CDM of the previous reservation, which the crtc may no longer
own. dpu_encoder_helper_phys_cleanup() later unbinds that CDM from its
pingpong and flushes it, changing a block which may by then belong to
another display.
Store NULL when no CDM is assigned.
Fixes: 8b45a26f2ba9 ("drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output")
Assisted-by: LLM
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index f06b52579740..6129b01c35e6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1160,6 +1160,7 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC];
struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
struct dpu_hw_blk *hw_cwb[MAX_CHANNELS_PER_ENC];
+ struct dpu_hw_blk *hw_cdm = NULL;
int num_ctl, num_pp, num_dsc, num_pp_per_intf;
int num_cwb = 0;
bool is_cwb_encoder;
@@ -1231,14 +1232,12 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
dpu_enc->dsc_mask = dsc_mask;
if ((dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) ||
- dpu_enc->disp_info.intf_type == INTF_DP) {
- struct dpu_hw_blk *hw_cdm = NULL;
-
+ dpu_enc->disp_info.intf_type == INTF_DP)
dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
drm_enc->crtc, DPU_HW_BLK_CDM,
&hw_cdm, 1);
- dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL;
- }
+
+ dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL;
/*
* There may be 4 PP and 2 INTF for quad pipe case, so INTF is not
--
2.47.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 07/10] drm/msm/dpu: clear the DSPP pointer when no DSPP is assigned
2026-10-03 0:30 [PATCH v4 00/10] drm/msm: fix SMMU fault dumps Dmitry Baryshkov
` (5 preceding siblings ...)
2026-10-03 0:30 ` [PATCH v4 06/10] drm/msm/dpu: clear the CDM block " Dmitry Baryshkov
@ 2026-10-03 0:30 ` Dmitry Baryshkov
2026-10-03 0:30 ` [PATCH v4 08/10] drm/msm/dpu: clear the DSC blocks left by a previous reservation Dmitry Baryshkov
` (2 subsequent siblings)
9 siblings, 0 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2026-10-03 0:30 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Antonino Maniscalco, Kalyan Thota, Federico Amedeo Izzo,
Helen Koike, Vignesh Raman, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel
dpu_crtc_assign_resources() only stores mixers[i].hw_dspp when the new
topology has a DSPP for that mixer, and dpu_crtc_duplicate_state() copies
the state with kmemdup(), so a crtc which once used a DSPP keeps a pointer
to it for the rest of its life -- across reservations which no longer own
that block.
Assign NULL when there is no DSPP for the mixer.
Fixes: 4259ff7ae509 ("drm/msm/dpu: add support for pcc color block in dpu driver")
Assisted-by: LLM
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 1f8fce366a1d..2ff0dab33710 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1480,8 +1480,8 @@ static int dpu_crtc_assign_resources(struct drm_crtc *crtc,
cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]);
cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]);
- if (i < num_dspp)
- cstate->mixers[i].hw_dspp = to_dpu_hw_dspp(hw_dspp[i]);
+ cstate->mixers[i].hw_dspp = i < num_dspp ?
+ to_dpu_hw_dspp(hw_dspp[i]) : NULL;
}
cstate->num_mixers = num_lm;
--
2.47.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 08/10] drm/msm/dpu: clear the DSC blocks left by a previous reservation
2026-10-03 0:30 [PATCH v4 00/10] drm/msm: fix SMMU fault dumps Dmitry Baryshkov
` (6 preceding siblings ...)
2026-10-03 0:30 ` [PATCH v4 07/10] drm/msm/dpu: clear the DSPP pointer when no DSPP is assigned Dmitry Baryshkov
@ 2026-10-03 0:30 ` Dmitry Baryshkov
2026-10-03 0:30 ` [PATCH v4 09/10] drm/msm/dpu: only reassign resources when the encoder is reprogrammed Dmitry Baryshkov
2026-10-03 0:30 ` [PATCH v4 10/10] drm/ci: mark pixel-format tests as passing on SC7180 Dmitry Baryshkov
9 siblings, 0 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2026-10-03 0:30 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Antonino Maniscalco, Kalyan Thota, Federico Amedeo Izzo,
Helen Koike, Vignesh Raman, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel
dpu_encoder_virt_atomic_mode_set() fills dpu_enc->hw_dsc[] with the DSC
blocks of the new reservation, but only the first num_dsc entries. The
array is never cleared, so an encoder which once drove two DSC blocks
keeps pointing at the second one after it has been given a topology with
a single DSC.
dpu_encoder_use_dsc_merge() counts the non-NULL entries of the whole
array and compares them against the number of interfaces, so the leftover
pointer makes it report DSC merge for a topology which has none, and
dpu_encoder_prep_dsc() then programs DSC_MODE_MULTIPLEX into the only DSC
block that is really used.
Assign NULL when the new reservation has no block for the slot.
Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder")
Assisted-by: LLM
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 6129b01c35e6..c9ea303a4b2f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1224,9 +1224,10 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
num_dsc = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
drm_enc->crtc, DPU_HW_BLK_DSC,
hw_dsc, ARRAY_SIZE(hw_dsc));
- for (i = 0; i < num_dsc; i++) {
- dpu_enc->hw_dsc[i] = to_dpu_hw_dsc(hw_dsc[i]);
- dsc_mask |= BIT(dpu_enc->hw_dsc[i]->idx - DSC_0);
+ for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
+ dpu_enc->hw_dsc[i] = i < num_dsc ? to_dpu_hw_dsc(hw_dsc[i]) : NULL;
+ if (dpu_enc->hw_dsc[i])
+ dsc_mask |= BIT(dpu_enc->hw_dsc[i]->idx - DSC_0);
}
dpu_enc->dsc_mask = dsc_mask;
--
2.47.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 09/10] drm/msm/dpu: only reassign resources when the encoder is reprogrammed
2026-10-03 0:30 [PATCH v4 00/10] drm/msm: fix SMMU fault dumps Dmitry Baryshkov
` (7 preceding siblings ...)
2026-10-03 0:30 ` [PATCH v4 08/10] drm/msm/dpu: clear the DSC blocks left by a previous reservation Dmitry Baryshkov
@ 2026-10-03 0:30 ` Dmitry Baryshkov
2026-10-03 0:30 ` [PATCH v4 10/10] drm/ci: mark pixel-format tests as passing on SC7180 Dmitry Baryshkov
9 siblings, 0 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2026-10-03 0:30 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Antonino Maniscalco, Kalyan Thota, Federico Amedeo Izzo,
Helen Koike, Vignesh Raman, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel
Running kms_plane@pixel-format-source-clamping causes SMMU faults
(fsr=0x402, iova=0x0001fb00): it sets a CTM, after which the display stops
updating, so the test times out waiting for a CRC and the pipe is still
fetching the framebuffers released when the runner kills it.
dpu_crtc_atomic_check() reassigns resources whenever color_mgmt_changed is
set, but crtc_set_mode() only calls ->atomic_mode_set() for mode_changed or
connectors_changed. A colour management update therefore re-reserves and
can get a different CTL: cstate->mixers[].lm_ctl points at the new one
while phys->hw_ctl still points at the old, so the flush mask is
accumulated on one and triggered on the other. CTL_FLUSH is written with
zero, so the pipe is not reprogrammed again until the next modeset.
Reassign only when the encoder is reprogrammed too. Enabling colour
management still needs a modeset, as that is when DSPPs join the topology,
so ask for one in dpu_crtc_check_mode_changed() and only there. Drop the
msm_atomic_check() hack, which forced a modeset both ways, ignored
GAMMA_LUT and overrode allow_modeset behind userspace's back.
Fixes: 39a750ff5fc9 ("drm/msm/dpu: Add DSPP GC driver to provide GAMMA_LUT DRM property")
Assisted-by: LLM
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 28 ++++++++++++++++++++++++++--
drivers/gpu/drm/msm/msm_atomic.c | 17 +----------------
2 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 2ff0dab33710..b1aea5769777 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1496,6 +1496,26 @@ static int dpu_crtc_assign_resources(struct drm_crtc *crtc,
*
* Check if the changes in the object properties demand full mode set.
*/
+static bool dpu_crtc_needs_dspp(const struct drm_crtc_state *crtc_state)
+{
+ return crtc_state->ctm || crtc_state->gamma_lut;
+}
+
+static bool dpu_crtc_has_dspp(const struct drm_crtc_state *crtc_state)
+{
+ const struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state);
+ unsigned int i;
+
+ if (!cstate->num_mixers)
+ return false;
+
+ for (i = 0; i < cstate->num_mixers; i++)
+ if (!cstate->mixers[i].hw_dspp)
+ return false;
+
+ return true;
+}
+
int dpu_crtc_check_mode_changed(struct drm_crtc_state *old_crtc_state,
struct drm_crtc_state *new_crtc_state)
{
@@ -1506,6 +1526,11 @@ int dpu_crtc_check_mode_changed(struct drm_crtc_state *old_crtc_state,
DRM_DEBUG_ATOMIC("%d\n", crtc->base.id);
+ /* DSPPs are only reserved during a modeset */
+ if (dpu_crtc_needs_dspp(new_crtc_state) &&
+ !dpu_crtc_has_dspp(old_crtc_state))
+ new_crtc_state->mode_changed = true;
+
/* there might be cases where encoder needs a modeset too */
drm_for_each_encoder_mask(drm_enc, crtc->dev, new_crtc_state->encoder_mask) {
if (dpu_encoder_needs_modeset(drm_enc, new_crtc_state->state))
@@ -1535,8 +1560,7 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
bool needs_dirtyfb = dpu_crtc_needs_dirtyfb(crtc_state);
/* don't reallocate resources if only ACTIVE has beeen changed */
- if (crtc_state->mode_changed || crtc_state->connectors_changed ||
- crtc_state->color_mgmt_changed) {
+ if (crtc_state->mode_changed || crtc_state->connectors_changed) {
rc = dpu_crtc_assign_resources(crtc, crtc_state);
if (rc < 0)
return rc;
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index a8babf1dbe0d..7b480981becc 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -185,22 +185,7 @@ int msm_atomic_check(struct drm_device *dev, struct drm_atomic_commit *state)
{
struct msm_drm_private *priv = dev->dev_private;
struct msm_kms *kms = priv->kms;
- struct drm_crtc_state *old_crtc_state, *new_crtc_state;
- struct drm_crtc *crtc;
- int i, ret = 0;
-
- /*
- * FIXME: stop setting allow_modeset and move this check to the DPU
- * driver.
- */
- for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
- new_crtc_state, i) {
- if ((old_crtc_state->ctm && !new_crtc_state->ctm) ||
- (!old_crtc_state->ctm && new_crtc_state->ctm)) {
- new_crtc_state->mode_changed = true;
- state->allow_modeset = true;
- }
- }
+ int ret = 0;
if (kms && kms->funcs && kms->funcs->check_mode_changed)
ret = kms->funcs->check_mode_changed(kms, state);
--
2.47.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 10/10] drm/ci: mark pixel-format tests as passing on SC7180
2026-10-03 0:30 [PATCH v4 00/10] drm/msm: fix SMMU fault dumps Dmitry Baryshkov
` (8 preceding siblings ...)
2026-10-03 0:30 ` [PATCH v4 09/10] drm/msm/dpu: only reassign resources when the encoder is reprogrammed Dmitry Baryshkov
@ 2026-10-03 0:30 ` Dmitry Baryshkov
9 siblings, 0 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2026-10-03 0:30 UTC (permalink / raw)
To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
Antonino Maniscalco, Kalyan Thota, Federico Amedeo Izzo,
Helen Koike, Vignesh Raman, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: linux-arm-msm, dri-devel, freedreno, linux-kernel
kms_plane@pixel-format and kms_plane@pixel-format-source-clamping timed out
waiting for the first CRC once the test had set a legacy gamma LUT. They
pass since "drm/msm/dpu: only reassign resources when the encoder is
reprogrammed", so drop them from the expected failures.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
drivers/gpu/drm/ci/xfails/msm-sc7180-trogdor-kingoftown-fails.txt | 2 --
drivers/gpu/drm/ci/xfails/msm-sc7180-trogdor-lazor-limozeen-fails.txt | 2 --
2 files changed, 4 deletions(-)
diff --git a/drivers/gpu/drm/ci/xfails/msm-sc7180-trogdor-kingoftown-fails.txt b/drivers/gpu/drm/ci/xfails/msm-sc7180-trogdor-kingoftown-fails.txt
index 2daf4ab879a9..ea3013faa37c 100644
--- a/drivers/gpu/drm/ci/xfails/msm-sc7180-trogdor-kingoftown-fails.txt
+++ b/drivers/gpu/drm/ci/xfails/msm-sc7180-trogdor-kingoftown-fails.txt
@@ -18,5 +18,3 @@ kms_pipe_crc_basic@compare-crc-sanitycheck-nv12,Fail
kms_plane_alpha_blend@alpha-7efc,Fail
kms_plane_alpha_blend@coverage-7efc,Fail
kms_plane_alpha_blend@coverage-vs-premult-vs-constant,Fail
-kms_plane@pixel-format,Fail
-kms_plane@pixel-format-source-clamping,Fail
diff --git a/drivers/gpu/drm/ci/xfails/msm-sc7180-trogdor-lazor-limozeen-fails.txt b/drivers/gpu/drm/ci/xfails/msm-sc7180-trogdor-lazor-limozeen-fails.txt
index 2daf4ab879a9..ea3013faa37c 100644
--- a/drivers/gpu/drm/ci/xfails/msm-sc7180-trogdor-lazor-limozeen-fails.txt
+++ b/drivers/gpu/drm/ci/xfails/msm-sc7180-trogdor-lazor-limozeen-fails.txt
@@ -18,5 +18,3 @@ kms_pipe_crc_basic@compare-crc-sanitycheck-nv12,Fail
kms_plane_alpha_blend@alpha-7efc,Fail
kms_plane_alpha_blend@coverage-7efc,Fail
kms_plane_alpha_blend@coverage-vs-premult-vs-constant,Fail
-kms_plane@pixel-format,Fail
-kms_plane@pixel-format-source-clamping,Fail
--
2.47.3
^ permalink raw reply related [flat|nested] 13+ messages in thread