Re: [PATCH 3/7] drm/msm: check UBWC framebuffers against their layout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm: broken P010 UBWC pitch check in msm_framebuffer_init()
--

commit 692349e38091784388304159b902edd6edf3ad3a
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm: check UBWC framebuffers against their layout

This commit updates msm_framebuffer_init() to validate UBWC framebuffers
against their computed UBWC plane pitches and sizes rather than checking
them as linear buffers.

> diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
> index 1e236416e15f0..dd24aa7a5ee80 100644
> --- a/drivers/gpu/drm/msm/msm_fb.c
> +++ b/drivers/gpu/drm/msm/msm_fb.c

[ ... ]

> @@ -177,14 +179,41 @@ msm_framebuffer_init(struct drm_device *dev, const struct drm_format_info *info,
>  		goto fail;
>  	}
>  
> +	if (format->fetch_mode == MDP_FETCH_UBWC)
> +		mdp_format_ubwc_plane_sizes(format, mode_cmd->width,
> +					    mode_cmd->height, ubwc_pitch,
> +					    ubwc_size);
> +
>  	for (i = 0; i < n; i++) {
>  		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
>  		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
> -		unsigned int min_size;
> -
> -		min_size = (height - 1) * mode_cmd->pitches[i]
> -			 + width * info->cpp[i]
> -			 + mode_cmd->offsets[i];
> +		u64 min_size;
> +
> +		if (format->fetch_mode == MDP_FETCH_UBWC) {
> +			/*
> +			 * The CbCr planes follow the Y data in the first buffer,
> +			 * offsets[1] and pitches[1] are not used.
> +			 */
> +			if (i > 0) {
> +				if (bos[i] != bos[0]) {
> +					ret = UERR(EINVAL, dev,
> +						   "plane %d not in the first buffer", i);
> +					goto fail;
> +				}
> +				min_size = 0;
> +			} else if (mode_cmd->pitches[0] != ubwc_pitch[0]) {
> +				ret = UERR(EINVAL, dev, "pitch %u, expected %u",
> +					   mode_cmd->pitches[0], ubwc_pitch[0]);
> +				goto fail;

[Severity: High]
Does this break creating DRM_FORMAT_P010 framebuffers with
DRM_FORMAT_MOD_QCOM_COMPRESSED?

In drivers/gpu/drm/msm/disp/mdp_format.c, mdp_formats_ubwc[] defines the
P010 entry using PSEUDO_YUV_FMT_TILED, which sets
MSM_FORMAT_FLAG_UNPACK_TIGHT:

drivers/gpu/drm/msm/disp/mdp_format.c:
	PSEUDO_YUV_FMT_TILED(P010,
		BPC8, BPC8, BPC8,
		C1_B_Cb, C2_R_Cr,
		CHROMA_420, MSM_FORMAT_FLAG_DX,
		MDP_TILE_HEIGHT_UBWC),

When mdp_format_ubwc_plane_sizes() is called on a DX YUV format with
MSM_FORMAT_FLAG_UNPACK_TIGHT set, it computes the tightly-packed TP10
stride (4/3 bytes per pixel) rather than the 2 bytes-per-pixel P010 stride:

drivers/gpu/drm/msm/disp/mdp_format.c:mdp_format_ubwc_plane_sizes() {
	...
		if (MSM_FORMAT_IS_DX(fmt)) {
			if (fmt->flags & MSM_FORMAT_FLAG_UNPACK_TIGHT) {
				/* can't use round_up() here because 192 is NPoT */
				stride = roundup(width, 192);
				stride = round_up(stride * 4 / 3, 256);
				y_tile_width = 48;
			} else {
				stride = round_up(width * 2, 256);
				y_tile_width = 32;
			}
	...
}

However, before msm_framebuffer_init() is reached,
drm_internal_framebuffer_create() -> framebuffer_check() in
drivers/gpu/drm/drm_framebuffer.c checks r->pitches[0] against
drm_format_info_min_pitch() for DRM_FORMAT_P010 (which has
char_per_block[0] = 2 and block_w[0] = 1), rejecting any framebuffer where
pitches[0] < width * 2:

drivers/gpu/drm/drm_framebuffer.c:framebuffer_check() {
	...
		if (block_size && r->pitches[i] < min_pitch) {
			drm_dbg_kms(dev, "bad pitch %u for plane %d\n", r->pitches[i], i);
			return -EINVAL;
		}
	...
}

Because roundup(width, 192) * 4 / 3 is strictly less than width * 2 for all
widths > 256 (as well as 129 <= width <= 192), no pitches[0] value can
satisfy both framebuffer_check() (pitches[0] >= width * 2) and
msm_framebuffer_init() (pitches[0] == ubwc_pitch[0]). For example, at
width = 1920, framebuffer_check() requires pitches[0] >= 3840 while
ubwc_pitch[0] is 2560, causing ADDFB2 to fail unconditionally with -EINVAL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-ubwc-format-docs-v1-0-389044012b4e@oss.qualcomm.com?part=3