Skip to contentSkip to conversation

drm/msm/dpu: clear pending flush state before physical cleanup

PATCH In review Saim Shujah 3 participants 4 messages Aug 26 – Sep 14

Subsystems (4)
Stable branch DRM drivers Arm Qualcomm mailing list DRM driver for Qualcomm display hardware
lore
Share & tools
lore

Saim Shujah

author
#0 lore raw

PATCH 1/1 drm/msm/dpu: clear pending flush state before physical cleanup

Hardware can consume an active-frame CTL flush while the cached software pending-flush mask retains the submitted bits. Video-mode disable stops the timing engine before physical cleanup, which then adds teardown bits to the stale cached mask.

The teardown trigger can replay source-pipe bits after timing stops, leaving one pending and preventing the first commit after re-enable.

Clear the cached pending-flush bookkeeping before constructing the teardown state. clear_pending_flush() updates software bookkeeping only; it does not write the hardware CTL_FLUSH register.

Fixes 22cb02bc96ff ("drm/msm/disp/dpu: reset the datapath after timing engine disable")
Cc [email protected]
Signed-off-by Saim Shujah <[email protected]>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 1 +
 1 file changed, 1 insertion(+)
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +1 -0
index 778e231..f3b1fdc 100644
@@ -2264,6 +2264,7 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
 
 	dpu_enc = to_dpu_encoder_virt(phys_enc->parent);
 
+	ctl->ops.clear_pending_flush(ctl);
 	ctl->ops.reset(ctl);
 
 	dpu_encoder_helper_reset_mixers(phys_enc);
Signature · 1 line
2.55.0
AI review
#1 lore raw
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] dpu_hw_ctl_clear_pending_flush() fails to clear pending_periph_flush_mask, leaving stale peripheral flush state across encoder teardowns.
- [High] Sleeping function usleep_range() is called from display driver atomic contexts during encoder cleanup.
--

commit 8f92b7c700308d037e1db8696d21fde498d38560
Author: Saim Shujah <[email protected]>

drm/msm/dpu: clear pending flush state before physical cleanup

Hardware can consume an active-frame CTL flush while the cached software pending-flush mask retains the submitted bits. The patch clears the cached pending-flush bookkeeping before constructing the teardown state.

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc) · line 2266

Quoted 9 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +1 -0
index 1f20695f81e35..45189b6076bc4 100644
@@ -2263,6 +2263,7 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
 
 	dpu_enc = to_dpu_encoder_virt(phys_enc->parent);
 
+	ctl->ops.clear_pending_flush(ctl);

[Severity: High]
This isn't a bug introduced by this patch, but does dpu_hw_ctl_clear_pending_flush() leave stale peripheral flush state across encoder teardowns?

In drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c:dpu_hw_ctl_clear_pending_flush():

	ctx->pending_flush_mask = 0x0;
	ctx->pending_intf_flush_mask = 0;
	ctx->pending_wb_flush_mask = 0;
	ctx->pending_cwb_flush_mask = 0;
	ctx->pending_merge_3d_flush_mask = 0;
	ctx->pending_dsc_flush_mask = 0;
	ctx->pending_cdm_flush_mask = 0;

	memset(ctx->pending_dspp_flush_mask, 0,
		sizeof(ctx->pending_dspp_flush_mask));

The function clears most software state caches, but ctx->pending_periph_flush_mask is missed.

Because pending_periph_flush_mask is omitted from the clear operation, any peripheral flush bits set from the previous active frame remain in the software cache. When the encoder is later re-enabled or flushed, this stale mask could be inadvertently flushed to the CTL_PERIPH_FLUSH hardware register, potentially causing unexpected hardware states or hangs.

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc) · line 2268

Quoted 1 line
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +0 -0
 	ctl->ops.reset(ctl);

[Severity: High]
This is a pre-existing issue, but does this hardware cleanup execute a sleeping polling path while in an atomic context?

dpu_encoder_helper_phys_cleanup() is called from the encoder atomic disable callback. ctl->ops.reset(ctl) resolves to dpu_hw_ctl_reset_control(), which polls the hardware and calls usleep_range():

drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c:dpu_hw_ctl_poll_reset_status():

		if (status)
			usleep_range(20, 50);

Calling sleeping functions like usleep_range() from atomic context paths in display drivers can cause kernel warnings, system instability, and potential deadlocks.

Quoted 2 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +0 -0
 	dpu_encoder_helper_reset_mixers(phys_enc);
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Reviewed-by on v1 1/1
#2 lore raw
In reply to Saim Shujah
Quoted 12 lines · Saim Shujah

Hardware can consume an active-frame CTL flush while the cached software pending-flush mask retains the submitted bits. Video-mode disable stops the timing engine before physical cleanup, which then adds teardown bits to the stale cached mask.

The teardown trigger can replay source-pipe bits after timing stops, leaving one pending and preventing the first commit after re-enable.

Clear the cached pending-flush bookkeeping before constructing the teardown state. clear_pending_flush() updates software bookkeeping only; it does not write the hardware CTL_FLUSH register.

Quoted 6 lines
Fixes 22cb02bc96ff ("drm/msm/disp/dpu: reset the datapath after timing engine disable")
Cc [email protected]
Signed-off-by Saim Shujah <[email protected]>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 1 +
 1 file changed, 1 insertion(+)
Reviewed-by Dmitry Baryshkov <[email protected]>
Quoted 1 line
Signature · 2 lines
With best wishes
Dmitry
#3 lore raw
Quoted 13 lines · Dmitry Baryshkov
Quoted 12 lines · Saim Shujah

Hardware can consume an active-frame CTL flush while the cached software pending-flush mask retains the submitted bits. Video-mode disable stops the timing engine before physical cleanup, which then adds teardown bits to the stale cached mask.

The teardown trigger can replay source-pipe bits after timing stops, leaving one pending and preventing the first commit after re-enable.

Clear the cached pending-flush bookkeeping before constructing the teardown state. clear_pending_flush() updates software bookkeeping only; it does not write the hardware CTL_FLUSH register.

Quoted 8 lines
Quoted 6 lines
Fixes 22cb02bc96ff ("drm/msm/disp/dpu: reset the datapath after timing engine disable")
Cc [email protected]
Signed-off-by Saim Shujah <[email protected]>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 1 +
 1 file changed, 1 insertion(+)
Reviewed-by Dmitry Baryshkov <[email protected]>

Unfortunately this patch breaks several IGT tests on sc7180:

kms_plane_alpha_blend@constant-alpha-max
kms_plane_alpha_blend@constant-alpha-mid
kms_rotation_crc@cursor-rotation-180
kms_rotation_crc@primary-rotation-180

Signature · 2 lines
With best wishes
Dmitry