Re: [PATCH 4/5] drm/msm/dpu: Add SM7250 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm/dpu: missing qcom,sm7250 entry in qcom_ubwc_configs[] breaks probe
- [Medium] drm/msm/dpu: wrong VIG scaler sblk and decimation caps in sm7250 catalog
--

commit c585a6f10d3dc7f0ad4fb0db73946dc4b06530db
Author: Ishu Brar <patches@brar.si>

drm/msm/dpu: Add SM7250 support

This commit adds hardware catalog definitions and device tree matching for
the DPU 6.1 display controller on the Qualcomm SM7250 platform.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_1_sm7250.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_1_sm7250.h
> new file mode 100644
> index 0000000000000..802a96598f362
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_1_sm7250.h
> @@ -0,0 +1,311 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2022. Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2015-2018, 2020 The Linux Foundation. All rights reserved.
> + *
> + * SM7250, SDE hardware revision 6.1.0. Block offsets are relative to the
> + * "mdp" register region.
> + */
> +
> +#ifndef _DPU_6_1_SM7250_H
> +#define _DPU_6_1_SM7250_H
> +
> +static const struct dpu_caps sm7250_dpu_caps = {
> +	.max_mixer_width = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
> +	.max_mixer_blendstages = 0x9,
> +	.has_src_split = true,
> +	.has_dim_layer = true,
> +	.has_idle_pc = true,
> +	.has_3d_merge = true,
> +	.max_linewidth = 2880,
> +	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> +	.max_hdeci_exp = MAX_HORZ_DECIMATION,
> +	.max_vdeci_exp = MAX_VERT_DECIMATION,
> +};

[ ... ]

> +static const struct dpu_sspp_cfg sm7250_sspp[] = {
> +	{
> +		.name = "sspp_0", .id = SSPP_VIG0,
> +		.base = 0x4000, .len = 0x1f8,
> +		.features = VIG_SDM845_MASK_SDMA,
> +		.sblk = &dpu_vig_sblk_qseed3_2_4,
> +		.xin_id = 0,
> +		.type = SSPP_TYPE_VIG,
> +		.clk_ctrl = DPU_CLK_CTRL_VIG0,
> +	}, {
> +		.name = "sspp_1", .id = SSPP_VIG1,
> +		.base = 0x6000, .len = 0x1f8,
> +		.features = VIG_SDM845_MASK_SDMA,
> +		.sblk = &dpu_vig_sblk_qseed3_2_4,
> +		.xin_id = 4,
> +		.type = SSPP_TYPE_VIG,
> +		.clk_ctrl = DPU_CLK_CTRL_VIG1,
> +	}, {

[Severity: Medium]
Should SSPP_VIG0 and SSPP_VIG1 in sm7250_sspp[] use dpu_vig_sblk_qseed3_3_0
instead of dpu_vig_sblk_qseed3_2_4, and should max_hdeci_exp and
max_vdeci_exp be omitted from sm7250_dpu_caps?

During dpu_kms_hw_init() -> dpu_rm_init() -> dpu_hw_sspp_init(),
sm7250_sspp[] binds sspp_0 and sspp_1 to the DPU 5.x QSEED3LITE v2.4
sub-block (dpu_vig_sblk_qseed3_2_4) and sm7250_dpu_caps advertises hardware
decimation limits.

All other DPU 6.x catalog definitions with a VIG scaler (SM8250, SC7180,
SM6115, SM6350, and SM6375) use dpu_vig_sblk_qseed3_3_0 and omit
max_hdeci_exp and max_vdeci_exp, as hardware decimation was removed in DPU
6.0 (similar to the fixes for SM8250 and SM6115 in commits 03c0c3cb22a4 and
38164e990a42).

[ ... ]

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index da3556eb6ecc2..6eb6f65ada91d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1506,6 +1506,7 @@ static const struct of_device_id dpu_dt_match[] = {
>  	{ .compatible = "qcom,sm6350-dpu", .data = &dpu_sm6350_cfg, },
>  	{ .compatible = "qcom,sm6375-dpu", .data = &dpu_sm6375_cfg, },
>  	{ .compatible = "qcom,sm7150-dpu", .data = &dpu_sm7150_cfg, },
> +	{ .compatible = "qcom,sm7250-dpu", .data = &dpu_sm7250_cfg, },
>  	{ .compatible = "qcom,sm8150-dpu", .data = &dpu_sm8150_cfg, },
>  	{ .compatible = "qcom,sm8250-dpu", .data = &dpu_sm8250_cfg, },
>  	{ .compatible = "qcom,sm8350-dpu", .data = &dpu_sm8350_cfg, },

[Severity: High]
Will probing fail on SM7250 without a corresponding "qcom,sm7250" entry in
qcom_ubwc_configs[] in drivers/soc/qcom/ubwc_config.c?

Both dpu_kms_hw_init() and msm_mdss_init() call qcom_ubwc_config_get_data():

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c:dpu_kms_hw_init() {
	...
	dpu_kms->mdss = qcom_ubwc_config_get_data();
	if (IS_ERR(dpu_kms->mdss)) {
		rc = PTR_ERR(dpu_kms->mdss);
		DPU_ERROR("failed to get UBWC config data: %d\n", rc);
		goto err_pm_put;
	}
	...
}

Because qcom_ubwc_configs[] has no entry matching "qcom,sm7250",
of_machine_get_match_data(qcom_ubwc_configs) returns NULL and
qcom_ubwc_config_get_data() returns ERR_PTR(-EINVAL), aborting both MDSS and
DPU initialization.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031-dpu-sm7250-v1-0-159f403ec405@brar.si?part=4