Re: [PATCH 1/2] drm/msm/mdp4: keep the bus clock enabled while a CRTC is active

[sashiko-bot@kernel.org]

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

New issues:
- [High] Sleeping functions called from atomic context.
- [High] Unbalanced reference counting on AXI clock enable failure leads to premature clock disable for active CRTCs.

Pre-existing issues:
- [High] Failure to enforce IOMMU requirement by allowing a NULL VM fallback in drm_gpuvm conversion.
--

commit f1ffff35bea0debf56db480bb3da6443ad42c41a
Author: Alexandre MINETTE <contact@alex-min.fr>
Subject: drm/msm/mdp4: keep the bus clock enabled while a CRTC is active

This patch keeps the AXI/bus clock enabled for the lifetime of each active
CRTC to avoid temporary MDP4 disable paths gating the bus clock while
scanout is still running.

> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
> index e8066f9fd5343..9e53c9d956ca8 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
> @@ -271,6 +271,7 @@ static void mdp4_crtc_atomic_disable(struct drm_crtc *crtc,
>  
>  	mdp_irq_unregister(&mdp4_kms->base, &mdp4_crtc->err);
>  	mdp4_disable(mdp4_kms);
> +	mdp4_crtc_bus_put(mdp4_kms);

[Severity: High]
Does calling mdp4_crtc_bus_put() here introduce a sleeping lock into an atomic
context?

CRTC atomic disable callbacks execute in an atomic context where sleeping is
forbidden, but mdp4_crtc_bus_put() acquires a mutex lock.

[ ... ]

> @@ -295,6 +296,7 @@ static void mdp4_crtc_atomic_enable(struct drm_crtc *crtc,
>  		return;
>  
>  	mdp4_enable(mdp4_kms);
> +	mdp4_crtc_bus_get(mdp4_kms);

[Severity: High]
Similarly, does this introduce sleeping calls into the atomic enable path?

mdp4_crtc_bus_get() uses both mutex_lock() and clk_prepare_enable(), which
may sleep.

[ ... ]

> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> index 809ca191e9de7..4c60b49fb7842 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> @@ -118,6 +118,39 @@ static long mdp4_round_pixclk(struct msm_kms *kms, unsigned long rate,
[ ... ]
> +void mdp4_crtc_bus_get(struct mdp4_kms *mdp4_kms)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&mdp4_kms->clock_lock);
> +
> +	if (!mdp4_kms->crtc_bus_count)
> +		ret = clk_prepare_enable(mdp4_kms->axi_clk);
> +
> +	if (!ret)
> +		mdp4_kms->crtc_bus_count++;
> +
> +	mutex_unlock(&mdp4_kms->clock_lock);
> +
> +	WARN_ON(ret);
> +}
> +
> +void mdp4_crtc_bus_put(struct mdp4_kms *mdp4_kms)
> +{
> +	mutex_lock(&mdp4_kms->clock_lock);
> +	if (!mdp4_kms->crtc_bus_count)
> +		goto unlock;
> +
> +	mdp4_kms->crtc_bus_count--;

[Severity: High]
Could this result in unbalanced reference counting and premature clock disable?

If clk_prepare_enable() fails in mdp4_crtc_bus_get(), crtc_bus_count is not
incremented, but the error isn't propagated since the function returns void.
If a subsequent CRTC successfully enables the clock, the counter becomes 1.

Later, when the first (failed) CRTC is disabled, mdp4_crtc_bus_put() will
unconditionally decrement crtc_bus_count to 0 and disable the clock. This
steals the reference belonging to the second active CRTC, leaving it scanning
out with its bus clock disabled.

[ ... ]

While reviewing this file, I noticed a pre-existing issue in mdp4_kms_init():

drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c:mdp4_kms_init() {
    ...
	vm = msm_kms_init_vm(mdp4_kms->dev, NULL);
	if (IS_ERR(vm)) {
		ret = PTR_ERR(vm);
		goto fail;
	}

	kms->vm = vm;
    ...
}

[Severity: High]
This is a pre-existing issue, but does this bypass the IOMMU requirement if
msm_kms_init_vm() returns NULL?

The DRM subsystem requires IOMMU support for drm_gpuvm and does not support
fallback modes. Since msm_kms_init_vm() might return NULL when IOMMU is
unavailable, checking only IS_ERR(vm) allows driver initialization to proceed
with a NULL VM instead of failing with an error.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-mainline-fix-mdp4-blue-screen-sending-v1-v1-0-22925600e4f2@alex-min.fr?part=1