Re: [PATCH v3] drm/rockchip: vop2: Scale the AXI clock to the bandwidth the mode needs

[sashiko-bot@kernel.org]

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

New issues:
- [High] Sleeping functions called from atomic commit tail path.

Pre-existing issues:
- [High] Preexisting sleeping functions called from CRTC atomic disable callback.
--

commit ba765a9b53922b23dd96c53d16b5f0f7d8aa8c99
Author: Igor Paunovic <royalnet026@gmail.com>

drm/rockchip: vop2: Scale the AXI clock to the bandwidth the mode needs

This patch dynamically scales the VOP2 AXI clock on RK3588 based on the
bandwidth requirements of the video mode. It tracks the clock requirement via
a global atomic state object and updates the clock in the atomic commit tail
path.

Link: https://lore.kernel.org/all/20260808104240.13776-1-royalnet026@gmail.com/
Link: https://lore.kernel.org/all/20767137.geO5KgaWL5@diego/ [1]

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index b2f3a579bbe52..cf574e48f9561 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
[ ... ]
> +static void vop2_commit_tail_begin(struct rockchip_drm_commit_hooks *hooks,
> +				   struct drm_atomic_commit *state)
> +{
> +	struct vop2 *vop2 = vop2_from_commit_hooks(hooks);
> +	struct drm_private_state *old_priv_state, *new_priv_state;
> +	struct vop2_aclk_state *old_aclk_state;
> +	unsigned int i;
> +
> +	old_priv_state = drm_atomic_get_old_private_obj_state(state, &vop2->aclk_obj);
> +	new_priv_state = drm_atomic_get_new_private_obj_state(state, &vop2->aclk_obj);
> +	if (!old_priv_state || !new_priv_state)
> +		return;
> +
> +	old_aclk_state = to_vop2_aclk_state(old_priv_state);
> +
> +	for (i = 0; i < ARRAY_SIZE(old_aclk_state->pending_commit); i++) {
> +		struct drm_crtc_commit *commit = old_aclk_state->pending_commit[i];
> +
> +		if (!commit)
> +			continue;
> +
> +		if (drm_crtc_commit_wait(commit))
> +			drm_err(vop2->drm, "timed out waiting for the commit on vp%u\n", i);

[Severity: High]
Does calling drm_crtc_commit_wait() here cause a kernel warning or potential 
deadlock? 

According to the DRM Subsystem Details, drm_atomic_helper_commit_tail() 
and its variants execute in atomic context where sleeping is forbidden. 
Since drm_crtc_commit_wait() uses wait_for_completion_timeout() internally, 
it can sleep and may lead to issues when called from the atomic commit tail
path.

> +
> +		drm_crtc_commit_put(commit);
> +		old_aclk_state->pending_commit[i] = NULL;
> +	}
> +
> +	vop2_set_aclk_rate(vop2,
> +			   max(vop2_aclk_rate(vop2, old_aclk_state),
> +			       vop2_aclk_rate(vop2, to_vop2_aclk_state(new_priv_state))));
> +}
> +
> +/*
> + * Every port has moved over: settle at the rate the new configuration needs.
> + * Runs before drm_atomic_helper_commit_hw_done(), so the next commit, which
> + * waits for hw_done, never sees this write land after its own.
> + */
> +static void vop2_commit_tail_end(struct rockchip_drm_commit_hooks *hooks,
> +				 struct drm_atomic_commit *state)
> +{
> +	struct vop2 *vop2 = vop2_from_commit_hooks(hooks);
> +	struct drm_private_state *priv_state;
> +
> +	priv_state = drm_atomic_get_new_private_obj_state(state, &vop2->aclk_obj);
> +	if (!priv_state)
> +		return;
> +
> +	vop2_set_aclk_rate(vop2, vop2_aclk_rate(vop2, to_vop2_aclk_state(priv_state)));

[Severity: High]
Can this call to vop2_set_aclk_rate() sleep while in an atomic context?

This path eventually calls clk_set_rate() which takes a mutex and is a 
sleeping function. Just like with drm_crtc_commit_wait() above, invoking 
this from the atomic commit tail could lead to warnings or deadlocks.

> +}
> +
>  static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
>  				     struct drm_atomic_commit *state)
>  {

[Severity: High]
This is a pre-existing issue, but it appears that the implementation of
vop2_crtc_atomic_disable() calls sleeping functions from an atomic context.

Specifically, it calls vop2_lock() which acquires a mutex, and 
wait_for_completion_timeout() for vp->dsp_hold_completion. The subsystem 
guidelines state that CRTC atomic disable callbacks run in atomic context 
where sleeping is forbidden. This might also be contributing to kernel
warnings or system instability.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910130824.6587-1-royalnet026@gmail.com?part=1