Re: [PATCH 4/7] drm/fourcc: describe the Qualcomm UBWC layout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/fourcc: documented UBWC P010 layout mismatches msm driver
--

commit 9d1d25812fbfd6458cdaf0aaead9ba0fe219bc18
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/fourcc: describe the Qualcomm UBWC layout

This commit documents the buffer layout, plane ordering, tile sizes, and
alignment requirements for DRM_FORMAT_MOD_QCOM_COMPRESSED across RGB,
DRM_FORMAT_NV12, and DRM_FORMAT_P010 formats.

> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 0bb049f91524e..4b6a4a827f6b8 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -820,13 +820,53 @@ extern "C" {

[ ... ]

> + * DRM_FORMAT_P010, two planes, Y then CbCr, 16 bits per sample:
> + *   Y data: pitch ALIGN(width * 2, 256), ALIGN(height, 16) lines
> + *   Y metadata: 32x4 pixel tiles, stride ALIGN(DIV_ROUND_UP(width, 32), 64),
> + *   ALIGN(DIV_ROUND_UP(height, 4), 16) lines
> + *   CbCr data: pitch ALIGN(width * 2, 256),
> + *   ALIGN(DIV_ROUND_UP(height, 2), 16) lines
> + *   CbCr metadata: 16x4 sample tiles,
> + *   stride ALIGN(DIV_ROUND_UP(DIV_ROUND_UP(width, 2), 16), 64),
> + *   ALIGN(DIV_ROUND_UP(DIV_ROUND_UP(height, 2), 4), 16) lines

[Severity: Medium]
Does the documented layout for DRM_FORMAT_P010 match what the msm driver
currently expects for DRM_FORMAT_MOD_QCOM_COMPRESSED?

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

drivers/gpu/drm/msm/disp/mdp_format.c:
    #define PSEUDO_YUV_FMT_TILED(fmt, a, r, g, b, e0, e1, chroma, flg, tile_height) \
        ...
        .fetch_mode = MDP_FETCH_UBWC,                                     \
        .flags = MSM_FORMAT_FLAG_UNPACK_TIGHT |                           \
                 MSM_FORMAT_FLAG_YUV |                                    \
                 MSM_FORMAT_FLAG_COMPRESSED | flg,                        \

    static const struct msm_format mdp_formats_ubwc[] = {
        ...
        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 DRM_IOCTL_MODE_ADDFB2 calls msm_framebuffer_create() ->
msm_framebuffer_init() -> mdp_format_ubwc_plane_sizes(), the presence of
MSM_FORMAT_FLAG_UNPACK_TIGHT causes it to take the tightly packed TP10/QC10C
branch instead of the 16-bit per sample else branch described in this
comment:

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;
        }
    ...
}

Because msm_framebuffer_init() checks that mode_cmd->pitches[0] equals
ubwc_pitch[0]:

drivers/gpu/drm/msm/msm_fb.c:msm_framebuffer_init() {
    ...
    } 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;
    }
    ...
}

won't a DRM_FORMAT_P010 framebuffer allocated with
DRM_FORMAT_MOD_QCOM_COMPRESSED according to this documentation (with pitch
ALIGN(width * 2, 256) and 32x4 Y tiles) be rejected with -EINVAL by
msm_framebuffer_init(), or have the DPU unpacker configured with
UNPACK_TIGHT = 1 and UNPACK_ALIGN_MSB = 0 in dpu_hw_setup_format_impl() and
dpu_hw_wb_setup_format()?

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