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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Lockless access to `crtc->state` of unrelated CRTCs causes data races and potential Use-After-Free during concurrent atomic commits.
--

--- Patch [1]: [PATCH] drm/rockchip: vop2: Scale the AXI clock to the bandwidth the mode needs ---
commit a53c1bd3571ebff2298a00e2f974ac4c2299ce6e
Author: Igor Paunovic <[email protected]>

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

This commit dynamically scales the AXI clock on RK3588 devices to meet the
bandwidth requirements of active video ports. It calculates the necessary
clock rate by checking the pixel rate of all active CRTCs to prevent
hardware FIFO underruns.

Link: https://lore.kernel.org/all/[email protected]/
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 4cce3e336f5b3..5dda1fc1c8a14 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c

[ ... ]

> @@ -1008,6 +1022,32 @@ static bool vop2_gamma_lut_in_use(struct vop2 *vop2, struct vop2_video_port *vp)
>  	return gamma_en_vp_id != nr_vps && gamma_en_vp_id != vp->id;
>  }
>  
> +/*
> + * Pick the AXI clock rate that satisfies every video port that is scanning
> + * out.  Taking the maximum over the active ports rather than counting them
> + * means a port being disabled can never drop the rate below what a port that
> + * is still scanning needs.
> + */
> +static void vop2_update_aclk_rate(struct vop2 *vop2)
> +{
> +	unsigned long rate = vop2->aclk_rate_normal;
> +	struct drm_crtc *crtc;
> +
> +	if (vop2->version != VOP_VERSION_RK3588)
> +		return;
> +
> +	drm_for_each_crtc(crtc, vop2->drm) {
> +		if (!crtc->state->active)
> +			continue;
> +
> +		if (crtc->state->adjusted_mode.crtc_clock > VOP2_HIGH_BW_PIXCLK_KHZ &&
> +		    rate < VOP2_ACLK_RATE_HIGH)
> +			rate = VOP2_ACLK_RATE_HIGH;
> +	}

[Severity: High]
Is it safe to access the state of unrelated CRTCs locklessly here?

When vop2_update_aclk_rate() is called from vop2_crtc_atomic_enable() or
vop2_crtc_atomic_disable(), the thread typically only holds the modeset
lock for the specific CRTC being committed, along with the driver-private
vop2_lock.

If userspace performs a concurrent non-blocking atomic commit on a
different CRTC, drm_atomic_helper_swap_state() could swap that CRTC's
state pointer and free the old state since it doesn't acquire the private
vop2_lock. Could this result in a use-after-free when reading
crtc->state->active or crtc->state->adjusted_mode.crtc_clock?

> +
> +	clk_set_rate(vop2->aclk, rate);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1