Re: [PATCH v2 12/23] drm/msm/dcp: Add DCP planes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] drm/msm/dcp: missing HFI pipe detach in dcp_plane_atomic_update()
- [Critical] drm/msm/dcp: unlocked plane->state access in dcp_plane_pipe_busy()
- [High] drm/msm/dcp: stale plane visibility used in dcp_plane_assign_pipe()
- [High] drm/msm/dcp: 90/270 rotation ignored in dcp_plane_atomic_check()
- [High] drm/msm/dcp: dcp_plane_atomic_create_state() returns NULL on OOM
- [High] drm/msm/dcp: broken IS_ERR() and NULL checks in dcp_plane_atomic_check()
- [High] drm/msm/dcp: divide-by-zero in dcp_plane_calc_bw() on zero vblank
- [Medium] drm/msm/dcp: chroma planes omitted in dcp_plane_calc_perf() bpp
- [Medium] drm/msm/dcp: non-alpha formats not forced to opaque blending in dcp_plane
- [Medium] drm/msm/dcp: missing minimum source size check for RGB formats in dcp_plane_atomic_check()
--

commit 22e98b5154e2eb947949b5c49894d72a16ccd158
Author: Jayant Shekhar <jayant.shekhar@oss.qualcomm.com>

drm/msm/dcp: Add DCP planes

This commit adds DRM plane support for the DCP display driver, including
format and scaling validation, firmware pipe assignment, bandwidth and
clock calculation, and HFI property packet construction and submission.

> diff --git a/drivers/gpu/drm/msm/dcp/dcp_plane.c b/drivers/gpu/drm/msm/dcp/dcp_plane.c
> new file mode 100644
> index 0000000000000..af8aa2162b03d
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dcp/dcp_plane.c
[ ... ]
> +static bool dcp_plane_pipe_busy(struct drm_atomic_commit *state,
> +				struct drm_plane *self, u32 pipe_id)
> +{
> +	struct drm_plane *plane;
> +	struct drm_plane_state *plane_state;
> +	struct drm_plane *iter_plane;
> +	struct drm_plane_state *new_plane_state;
> +	int i;
> +
> +	drm_for_each_plane(plane, state->dev) {
> +		if (plane == self)
> +			continue;
> +
> +		plane_state = plane->state;
> +		for_each_new_plane_in_state(state, iter_plane, new_plane_state, i) {
> +			if (iter_plane == plane) {
> +				plane_state = new_plane_state;
> +				break;
> +			}
> +		}
> +
> +		if (plane_state && plane_state->crtc && plane_state->visible &&
> +		    to_dcp_plane_state(plane_state)->hfi_pipe_id == pipe_id)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static u32 dcp_plane_assign_pipe(struct dcp_dpu_kms *dcp_kms,
> +				 struct drm_atomic_commit *state,
> +				 struct drm_plane *plane,
> +				 struct dcp_plane_state *pstate,
> +				 bool yuv, bool scaling)
> +{
> +	const struct dcp_caps *caps = dcp_kms->caps;
> +	u32 i, pipe_id;
> +
> +	/* YUV scanout requires a VIG pipe with the CSC path. */
> +	if (dcp_plane_hfi_pipe_valid(dcp_kms, pstate->hfi_pipe_id) &&
> +	    (!yuv || dcp_plane_pipe_is_vig(caps, pstate->hfi_pipe_id)) &&
> +	    (!scaling || dcp_plane_pipe_is_vig(caps, pstate->hfi_pipe_id)) &&
> +	    !dcp_plane_pipe_busy(state, plane, pstate->hfi_pipe_id))
> +		return pstate->hfi_pipe_id;
> +
> +	/*
> +	 * R1/virtual pipes are intentionally not allocated as independent
> +	 * planes yet. They require transactional physical-pipe/R1 ownership
> +	 * and multirect programming, which will be added with the resource
> +	 * manager.
> +	 */
> +	for (i = 0; i < caps->num_vig_pipes; i++) {
> +		pipe_id = caps->vig_pipe_ids[i];
> +		if (!dcp_plane_pipe_busy(state, plane, pipe_id))
> +			return pipe_id;
> +	}

[Severity: Critical]
Is it safe for dcp_plane_pipe_busy() to read plane->state for planes that
are not part of the current drm_atomic_commit without holding plane->mutex?

When dcp_plane_atomic_check() calls dcp_plane_assign_pipe() ->
dcp_plane_pipe_busy(), it iterates over all planes on the device via
drm_for_each_plane() and falls back to plane_state = plane->state for any
plane not present in state.

Because DCP supports multiple CRTCs and planes are shared across
possible_crtcs, a concurrent atomic commit on another CRTC holding that
other plane's mutex can swap plane->state in drm_atomic_helper_swap_state()
and free the old state via drm_atomic_commit_put() ->
dcp_plane_atomic_destroy_state() -> kfree() while dcp_plane_pipe_busy() is
dereferencing plane_state->crtc, plane_state->visible, and
to_dcp_plane_state(plane_state)->hfi_pipe_id.

In addition, without serializing pipe assignment through a shared
drm_private_obj state or lock, can two concurrent atomic checks on
different CRTCs both see the same pipe_id as free in dcp_plane_pipe_busy()
and assign the same hardware HFI pipe to two active planes?

[ ... ]
> +static u64 dcp_plane_calc_bw(u32 bpp,
> +			     const struct drm_display_mode *mode,
> +			     const struct drm_rect *src,
> +			     const struct drm_rect *dst)
> +{
> +	int src_w = drm_rect_width(src) >> 16;
> +	int src_h = drm_rect_height(src) >> 16;
> +	int dst_h = drm_rect_height(dst);
> +	int fps   = drm_mode_vrefresh(mode);
> +	int vbp   = mode->vtotal - mode->vsync_end;
> +	int vpw   = mode->vsync_end - mode->vsync_start;
> +	int vfp   = mode->vsync_start - mode->vdisplay;
> +	u32 hw_latency = 25;
> +	u64 scale_factor = (src_h > dst_h) ? mult_frac(src_h, 1, dst_h) : 1;
> +	u64 plane_bw, prefill_bw;
> +
> +	plane_bw = (u64)src_w * mode->vtotal * fps * bpp * scale_factor;
> +	prefill_bw = plane_bw * hw_latency;
> +
> +	if ((vbp + vpw) > hw_latency)
> +		do_div(prefill_bw, (vbp + vpw));
> +	else if ((vbp + vpw + vfp) < hw_latency)
> +		do_div(prefill_bw, (vbp + vpw + vfp));
> +	else
> +		do_div(prefill_bw, hw_latency);

[Severity: High]
Can dcp_plane_calc_bw() trigger a divide-by-zero when (vbp + vpw + vfp) is
zero?

Here, vbp + vpw + vfp equals mode->vtotal - mode->vdisplay. Neither
drm_mode_validate_basic() nor dcp_crtc_mode_valid() rejects a mode with
vdisplay == vsync_start == vsync_end == vtotal.

If userspace submits an atomic commit or DRM_MODE_ATOMIC_TEST_ONLY check
with such a mode and a visible plane, (vbp + vpw) > hw_latency is false and
(vbp + vpw + vfp) < hw_latency is true (0 < 25), causing do_div(prefill_bw,
(vbp + vpw + vfp)) to divide by zero.

[ ... ]
> +void dcp_plane_calc_perf(const struct drm_plane_state *state,
> +			 const struct drm_display_mode *mode,
> +			 struct dcp_plane_perf *perf)
> +{
> +	struct drm_rect src, dst;
> +	u32 bpp;
> +
> +	memset(perf, 0, sizeof(*perf));
> +
> +	if (!state || !state->visible || !state->fb || !mode || !mode->clock ||
> +	    !drm_mode_vrefresh(mode))
> +		return;
> +
> +	src = drm_plane_state_src(state);
> +	dst = drm_plane_state_dest(state);
> +	bpp = drm_format_info_bpp(state->fb->format, 0);
> +
> +	perf->fetch_bw = dcp_plane_calc_bw(bpp, mode, &src, &dst);
> +	perf->clk = dcp_plane_calc_clk(mode, &src, &dst);
> +}

[Severity: Medium]
Does querying drm_format_info_bpp(state->fb->format, 0) in
dcp_plane_calc_perf() undercount the bandwidth for multi-planar YUV
formats?

For formats such as DRM_FORMAT_NV12, DRM_FORMAT_NV21, DRM_FORMAT_YUV420,
DRM_FORMAT_YVU420, and DRM_FORMAT_P010, plane 0 only accounts for the luma
plane (8 bpp for 8-bit 4:2:0 or 16 bpp for P010), omitting the chroma
planes (which add another 4 bpp or 8 bpp) from perf->fetch_bw.

[ ... ]
> +static int dcp_plane_build_one_hfi_props_packet(struct dcp_plane_state *pstate,
> +						const struct drm_plane_state *state,
> +						u32 session_id, u32 packet_id,
> +						u8 *packet)
> +{
[ ... ]
> +	hfi_rotation = dcp_drm_to_hfi_rotation(state->rotation);
> +	hfi_blend_op = dcp_drm_to_hfi_blend_op(state->pixel_blend_mode);
> +
> +	src_x = state->src_x >> 16;
> +	src_y = state->src_y >> 16;
> +	src_w = state->src_w >> 16;
> +	src_h = state->src_h >> 16;
> +	dst_x = state->crtc_x;
> +	dst_y = state->crtc_y;
> +	dst_w = state->crtc_w;
> +	dst_h = state->crtc_h;
> +
> +	hfi_alpha = dcp_hfi_scale_alpha(state, state->alpha);
> +	hfi_bg_alpha = dcp_hfi_scale_alpha(state,
> +					   dcp_compute_bg_alpha(state->pixel_blend_mode,
> +								state->alpha));

[Severity: Medium]
When !fb->format->has_alpha (for example with DRM_FORMAT_XRGB8888,
DRM_FORMAT_RGB565, or YUV formats), should dcp_plane_build_one_hfi_props_packet()
fall back to DRM_MODE_BLEND_PIXEL_NONE instead of using
state->pixel_blend_mode directly?

dcp_plane_init() attaches the pixel blend mode property with
DRM_MODE_BLEND_PREMULTI as the default. When a non-alpha framebuffer is
used, passing DRM_MODE_BLEND_PREMULTI or DRM_MODE_BLEND_COVERAGE to
dcp_drm_to_hfi_blend_op() and dcp_compute_bg_alpha() programs
HFI_BLEND_OP_PREMULTIPLIED or HFI_BLEND_OP_COVERAGE and sets bg_alpha to
fg_alpha (or 0 when opaque) rather than HFI_BLEND_OP_OPAQUE with
DRM_BLEND_ALPHA_OPAQUE - fg_alpha.

[ ... ]
> +static int dcp_plane_atomic_check(struct drm_plane *plane,
> +				  struct drm_atomic_commit *state)
> +{
[ ... ]
> +	if (!new_state->crtc)
> +		return 0;
> +
> +	crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
> +	if (IS_ERR(crtc_state)) {
> +		ret = PTR_ERR(crtc_state);
> +		drm_err(plane->dev, "plane%u: failed to get CRTC state: %d\n",
> +			plane->base.id, ret);
> +		return ret;
> +	}

[Severity: High]
drm_atomic_get_new_crtc_state() returns NULL when the CRTC is not in state,
rather than an ERR_PTR(). Should this check for !crtc_state instead of
IS_ERR(crtc_state) so a NULL crtc_state is not passed to
drm_atomic_helper_check_plane_state()?

Similarly, in dcp_plane_assign_pipe(), dcp_plane_hfi_pipe_valid() checks
if (!dcp_kms->caps), but when it returns false, dcp_plane_assign_pipe()
immediately dereferences caps->num_vig_pipes and caps->num_dma_pipes
without checking if caps is NULL.

> +	if (!new_state->fb)
> +		return 0;
> +
> +	scaling = new_state->src_w != ((u32)new_state->crtc_w << 16) ||
> +			  new_state->src_h != ((u32)new_state->crtc_h << 16);
> +
> +	pstate->hfi_pipe_id = dcp_plane_assign_pipe(dcp_kms, state, plane,
> +						    pstate, new_state->fb->format->is_yuv,
> +						    scaling);
> +	if (pstate->hfi_pipe_id == U32_MAX) {
> +		drm_err(plane->dev, "plane%u: no compatible HFI pipe available\n",
> +			plane->base.id);
> +		return -ENOSPC;
> +	}
> +
> +	if (dcp_plane_pipe_is_vig(dcp_kms->caps, pstate->hfi_pipe_id)) {
> +		min_scale = FRAC_16_16(1, DCP_MAX_UPSCALE_RATIO);
> +		max_scale = DCP_MAX_DOWNSCALE_RATIO << 16;
> +	}
> +
> +	ret = drm_atomic_helper_check_plane_state(new_state, crtc_state,
> +						  min_scale, max_scale,
> +						  true, true);

[Severity: High]
Because dcp_plane_assign_pipe() -> dcp_plane_pipe_busy() runs before
drm_atomic_helper_check_plane_state() computes new_state->visible, won't
dcp_plane_pipe_busy() inspect stale visibility flags?

Specifically:
1. When drm_atomic_helper_check_planes() checks planes in ascending index
   order, any higher-indexed plane in the same atomic commit has not run
   dcp_plane_atomic_check() yet, so its new_plane_state->visible still
   holds the old state's visibility. If that higher-indexed plane was
   previously disabled and is being enabled in this commit,
   dcp_plane_pipe_busy() sees new_plane_state->visible == false and allows
   an earlier plane to take its hfi_pipe_id.
2. If the current plane is clipped off-screen so
   drm_atomic_helper_check_plane_state() sets new_state->visible = false,
   it has already claimed a pipe in dcp_plane_assign_pipe().
3. When a previously visible plane is disabled (!new_state->crtc or
   !new_state->fb), dcp_plane_atomic_check() returns 0 early before calling
   drm_atomic_helper_check_plane_state(), leaving new_state->visible set to
   true on the disabled state.

> +	if (ret) {
> +		drm_err(plane->dev, "plane%u: plane state check failed: %d\n",
> +			plane->base.id, ret);
> +		return ret;
> +	}
> +
> +	if (!new_state->visible)
> +		return 0;
[ ... ]
> +	dcp_mode_limits(&crtc_state->adjusted_mode,
> +			dcp_kms->caps->max_width, dcp_kms->caps->max_height,
> +			&max_width, &max_height);
> +
> +	if (new_state->fb->width > max_width ||
> +	    new_state->fb->height > max_height) {
> +		drm_err(plane->dev,
> +			"plane%u: framebuffer %ux%u exceeds caps %ux%u\n",
> +			plane->base.id, new_state->fb->width, new_state->fb->height,
> +			dcp_kms->caps->max_width, dcp_kms->caps->max_height);
> +		return -E2BIG;
> +	}

[Severity: High]
How does dcp_plane_atomic_check() handle 90 and 270 degree rotations?

dcp_plane_init() advertises DRM_MODE_ROTATE_90 and DRM_MODE_ROTATE_270, and
drm_atomic_helper_check_plane_state() rotates the source rectangle via
drm_rect_rotate() before checking hscale and vscale. However:
1. The scaling check earlier in dcp_plane_atomic_check():
     scaling = new_state->src_w != ((u32)new_state->crtc_w << 16) ||
               new_state->src_h != ((u32)new_state->crtc_h << 16);
   compares unrotated source dimensions against crtc_w and crtc_h. For a
   non-square plane rotated 90 or 270 degrees, an unscaled plane (where
   src_w == crtc_h << 16 and src_h == crtc_w << 16) is treated as scaled
   and forced onto a VIG pipe, whereas a scaled plane with
   src_w == crtc_w << 16 and src_h == crtc_h << 16 is treated as unscaled,
   assigned a DMA pipe with DRM_PLANE_NO_SCALING, and rejected by
   drm_atomic_helper_check_plane_state() with -ERANGE.
2. The dcp_mode_limits() check compares unrotated new_state->fb->width and
   new_state->fb->height against mode-oriented max_width and max_height,
   rejecting a landscape framebuffer rotated 90/270 degrees onto a portrait
   mode with -E2BIG.
3. dcp_plane_calc_perf() passes unrotated drm_plane_state_src(state) to
   dcp_plane_calc_bw() and dcp_plane_calc_clk(), using unrotated src_w and
   src_h for bandwidth and clock scaling calculations.

> +	for (i = 0; i < new_state->fb->format->num_planes; i++) {
> +		if (new_state->fb->pitches[i] > DCP_SSPP_MAX_PITCH_SIZE) {
> +			drm_err(plane->dev,
> +				"plane%u: pitch[%d]=%u exceeds limit %u\n",
> +				plane->base.id, i, new_state->fb->pitches[i],
> +				DCP_SSPP_MAX_PITCH_SIZE);
> +			return -E2BIG;
> +		}
> +	}
> +
> +	src = drm_plane_state_src(new_state);
> +
> +	pstate->src_img_w = new_state->fb->width;
> +	pstate->src_img_h = new_state->fb->height;
> +
> +	if (new_state->fb->format->is_yuv) {
> +		u32 src_x = src.x1 >> 16;
> +		u32 src_y = src.y1 >> 16;
> +		u32 src_w = drm_rect_width(&src) >> 16;
> +		u32 src_h = drm_rect_height(&src) >> 16;
> +
> +		if (src_w < 2 || src_h < 2) {
> +			drm_err(plane->dev,
> +				"plane%u: YUV source too small: %ux%u\n",
> +				plane->base.id, src_w, src_h);
> +			return -EINVAL;
> +		}

[Severity: Medium]
Should non-YUV formats also enforce a minimum integer source size of
src_w >= 1 and src_h >= 1?

When an RGB plane is assigned to a VIG pipe, min_scale is
FRAC_16_16(1, DCP_MAX_UPSCALE_RATIO), which allows sub-pixel 16.16 source
dimensions (such as src_w or src_h in [3276, 65535] with crtc_w = 1 or
crtc_h = 1) to pass drm_atomic_helper_check_plane_state() with
new_state->visible = true.

Because minimum source dimensions are only checked inside the is_yuv block,
state->src_w >> 16 or state->src_h >> 16 then truncates to 0 in
dcp_plane_calc_bw() (resulting in a 0 bandwidth vote for a visible plane)
and in dcp_plane_build_one_hfi_props_packet() (sending a 0-width or
0-height HFI_PROPERTY_LAYER_SRC_ROI to the firmware).

[ ... ]
> +static void dcp_plane_atomic_update(struct drm_plane *plane,
> +				    struct drm_atomic_commit *state)
> +{
> +	struct drm_plane_state *new_state =
> +		drm_atomic_get_new_plane_state(state, plane);
> +	struct dcp_plane_state *pstate = to_dcp_plane_state(new_state);
> +	int ret;
> +
> +	drm_dbg_driver(plane->dev,
> +		       "plane%u: atomic_update visible=%d fb=%u pipe_id=%u\n",
> +		       plane->base.id, new_state->visible,
> +		       new_state->fb ? new_state->fb->base.id : 0,
> +		       pstate->hfi_pipe_id);
> +
> +	pstate->prop_pkt_size = 0;
> +
> +	if (!new_state->visible || !new_state->fb)
> +		return;
> +
> +	ret = dcp_plane_submit_update(plane, new_state);
> +	if (ret) {
> +		drm_err(plane->dev,
> +			"plane%u: HFI update failed: %d\n", plane->base.id, ret);
> +		dcp_plane_set_hfi_error(new_state, ret);
> +		return;
> +	}
> +}
> +
> +static void dcp_plane_atomic_disable(struct drm_plane *plane,
> +				     struct drm_atomic_commit *state)
> +{
> +	struct drm_plane_state *old_state =
> +		drm_atomic_get_old_plane_state(state, plane);
> +	struct dcp_plane_state *pstate = to_dcp_plane_state(old_state);
> +	int ret;
> +
> +	drm_dbg_driver(plane->dev,
> +		       "plane%u: atomic_disable pipe_id=%u\n",
> +		       plane->base.id, pstate->hfi_pipe_id);
> +
> +	ret = dcp_plane_submit_disable(plane, old_state);

[Severity: Critical]
Can HFI pipes remain attached to unmapped framebuffers or be erroneously
detached from another plane during atomic commits?

There appear to be three related issues in the update/disable paths:
1. When an active visible plane remains bound to a CRTC and framebuffer
   (new_state->crtc != NULL and new_state->fb != NULL) but is moved
   off-screen so new_state->visible becomes false,
   drm_atomic_plane_disabling() is false and
   drm_atomic_helper_commit_planes() calls dcp_plane_atomic_update() rather
   than dcp_plane_atomic_disable(). dcp_plane_atomic_update() returns early
   at if (!new_state->visible || !new_state->fb) without detaching
   old_state->hfi_pipe_id, leaving the DCP firmware scanning out from the
   old framebuffer IOVA after dcp_plane_cleanup_fb() ->
   msm_framebuffer_cleanup() unmaps it.
2. When an already-visible plane is reassigned to a different pipe by
   dcp_plane_assign_pipe() (for example, switching from an unscaled RGB
   buffer on a DMA pipe to a YUV or scaled buffer requiring a VIG pipe, so
   old_pstate->hfi_pipe_id != pstate->hfi_pipe_id),
   dcp_plane_atomic_update() attaches the new pipe but never sends
   HFI_PROPERTY_DISPLAY_DETACH_LAYER for old_pstate->hfi_pipe_id.
3. Because dcp_plane_pipe_busy() treats an invisible or disabling plane's
   hfi_pipe_id as free, a lower-indexed plane can be assigned and attach
   that pipe in dcp_plane_atomic_update() before the higher-indexed plane
   runs dcp_plane_atomic_disable() (which executes in ascending plane index
   order and does not check old_state->visible), causing
   dcp_plane_atomic_disable() to detach the pipe that the lower-indexed
   plane just attached.

[ ... ]
> +static struct drm_plane_state *
> +dcp_plane_atomic_create_state(struct drm_plane *plane)
> +{
> +	struct dcp_plane_state *pstate;
> +	u32 hfi_pipe_id = to_dcp_plane(plane)->hfi_pipe_id;
> +
> +	pstate = kzalloc_obj(*pstate, GFP_KERNEL);
> +	if (!pstate)
> +		return NULL;

[Severity: High]
Should dcp_plane_atomic_create_state() return ERR_PTR(-ENOMEM) instead of
NULL on allocation failure?

Its caller drm_mode_config_plane_create_state() checks for errors using
IS_ERR(plane_state), so returning NULL bypasses error handling and leaves
plane->state as NULL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-jshekhar-dcp-hlos-v2-0-7cf014efdee1@oss.qualcomm.com?part=12