Re: [PATCH 2/2] drm/msm/mdp4: keep inherited display clocks enabled until modeset

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unchecked clk_prepare_enable() return values can lead to synchronous external aborts during MMIO access or unbalanced clock disables.
- [High] The patch explicitly turns off `pclk` and `axi_clk` during probe, which can break the inherited display pipeline.
- [High] Inherited display clocks are leaked indefinitely if DRM components fail to bind, if the CRTC is disabled without being enabled first, or if userspace never initiates a modeset.

Pre-existing issues:
- [High] Calling sleeping functions from atomic context in CRTC atomic enable callback.
--

commit deec425702ee01b881e4c0a6e40e6ec4f167024f
Author: Alexandre MINETTE <contact@alex-min.fr>

drm/msm/mdp4: keep inherited display clocks enabled until modeset

This patch detects if the MDP4 display pipeline was already enabled by
firmware and keeps the required display clocks prepared to prevent panel
glitches. It drops the extra clock references on the first DRM CRTC
enable when the driver takes ownership.

> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
> @@ -297,6 +297,7 @@ static void mdp4_crtc_atomic_enable(struct drm_crtc *crtc,
>  
>  	mdp4_enable(mdp4_kms);
>  	mdp4_crtc_bus_get(mdp4_kms);
> +	mdp4_inherited_display_clocks_put(mdp4_kms);

[Severity: High]
Will the inherited clock references be leaked if the display components
never bind or if userspace never initiates a modeset?

If msm_drv_probe() successfully adds the master component but the required
display components fail to bind, the mdp4_destroy() callback is never
executed. If userspace never initiates a modeset or explicitly disables
the display first, atomic_enable is never reached. In these cases, it
appears the driver holds explicit clk_prepare_enable references that will
bypass the clock framework's late initcalls, leaving the hardware fully
powered on indefinitely.

[Severity: High]
This is a pre-existing issue, but does adding this call here worsen a
violation of atomic context rules?

CRTC atomic enable callbacks run in atomic context during non-blocking
commits, where sleeping is strictly forbidden. The mdp4_crtc_atomic_enable()
function already contained sleeping calls like mdp4_enable(), and this
patch adds mdp4_inherited_display_clocks_put() which acquires a mutex and
calls clk_disable_unprepare(). Both of these operations can sleep. Could
this lead to kernel might_sleep() warnings or deadlocks during non-blocking
commits?

>  
>  	/* Restore vblank irq handling after power is enabled */
>  	drm_crtc_vblank_on(crtc);

[ ... ]

> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
[ ... ]
> +static void mdp4_inherited_display_clocks_get(struct mdp4_kms *mdp4_kms)
> +{
> +	bool inherited_display;
> +
> +	mutex_lock(&mdp4_kms->clock_lock);
> +
> +	clk_prepare_enable(mdp4_kms->clk);
> +	clk_prepare_enable(mdp4_kms->pclk);
> +	clk_prepare_enable(mdp4_kms->lut_clk);
> +	clk_prepare_enable(mdp4_kms->axi_clk);

[Severity: High]
Is it safe to proceed with hardware MMIO reads if any of these clocks fail
to enable?

The return values from clk_prepare_enable() are completely ignored here.
If enabling the core or bus clock fails, the driver will proceed to perform
MMIO reads in mdp4_has_inherited_display_state() on unclocked hardware. On
MSM platforms, accessing unclocked registers typically triggers an unhandled
synchronous external abort, resulting in a kernel panic. Should these
return values be checked and handled?

> +
> +	inherited_display = mdp4_has_inherited_display_state(mdp4_kms);
> +
> +	if (inherited_display) {
> +		clk_prepare_enable(mdp4_kms->clk);
> +		clk_prepare_enable(mdp4_kms->lut_clk);
> +		mdp4_kms->inherited_display_clocks_held = true;
> +	}
> +
> +	clk_disable_unprepare(mdp4_kms->clk);
> +	clk_disable_unprepare(mdp4_kms->pclk);
> +	clk_disable_unprepare(mdp4_kms->lut_clk);
> +	clk_disable_unprepare(mdp4_kms->axi_clk);

[Severity: High]
Will unconditionally disabling pclk and axi_clk here break the inherited
display pipeline?

The Linux Common Clock Framework initializes clocks left on by the
bootloader with a software usage count of 0. By temporarily enabling and
then disabling them, the usage count goes from 0 to 1 and back to 0. This
triggers the framework to physically turn off the hardware clocks
immediately. Disabling the pixel clock (pclk) and memory bus clock
(axi_clk) while the hardware is actively scanning out a frame will
typically starve the pipeline, causing bus hangs, underflows, or the exact
blank screen this patch aims to prevent. Should the active state of these
clocks be preserved if an inherited display is detected?

> +
> +	mutex_unlock(&mdp4_kms->clock_lock);
> +}

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