Re: [PATCH v7 10/15] drm/imx: dc-ed: Support getting source selection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/imx: dc-ed: invalid link IDs accepted in dc_ed_pec_src_sel_get()
--

commit e94375137aef3b831a8ffe20e951e2c25abaed0f
Author: Liu Ying <victor.liu@nxp.com>

drm/imx: dc-ed: Support getting source selection

This commit adds dc_ed_pec_src_sel_get() to read and validate the ExtDst
source selection from PIXENGCFG_DYNAMIC.

> diff --git a/drivers/gpu/drm/imx/dc/dc-ed.c b/drivers/gpu/drm/imx/dc/dc-ed.c
> index de1b71315eabd..67163dca1363c 100644
> --- a/drivers/gpu/drm/imx/dc/dc-ed.c
> +++ b/drivers/gpu/drm/imx/dc/dc-ed.c
> @@ -149,6 +149,35 @@ void dc_ed_pec_src_sel(struct dc_ed *ed, enum dc_link_id src)
>  	}
>  }
>  
> +int dc_ed_pec_src_sel_get(struct dc_ed *ed, enum dc_link_id *src)
> +{
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(ed->reg_pec, PIXENGCFG_DYNAMIC, &val);
> +	if (ret)
> +		return -EINVAL;
> +
> +	switch (val) {
> +	case LINK_ID_NONE:
> +	case LINK_ID_CONSTFRAME0:
> +	case LINK_ID_CONSTFRAME4:
> +	case LINK_ID_CONSTFRAME1:
> +	case LINK_ID_CONSTFRAME5:
> +	case LINK_ID_FETCHWARP2:
> +	case LINK_ID_FETCHLAYER0:
> +	case LINK_ID_LAYERBLEND0:
> +	case LINK_ID_LAYERBLEND1:
> +	case LINK_ID_LAYERBLEND2:
> +	case LINK_ID_LAYERBLEND3:
> +		*src = val;
> +		dev_dbg(ed->dev, "get source selection: 0x%02x\n", val);
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}

[Severity: Medium]
Should dc_ed_pec_src_sel_get() validate the value read from PIXENGCFG_DYNAMIC
against src_sels[] rather than accepting LINK_ID_FETCHWARP2 and
LINK_ID_FETCHLAYER0?

In drivers/gpu/drm/imx/dc/dc-ed.c, src_sels[] defines the valid source link
IDs for the ExtDst unit and is used by dc_ed_pec_src_sel():

static const enum dc_link_id src_sels[] = {
	LINK_ID_NONE,
	LINK_ID_CONSTFRAME0,
	LINK_ID_CONSTFRAME1,
	LINK_ID_CONSTFRAME4,
	LINK_ID_CONSTFRAME5,
	LINK_ID_LAYERBLEND3,
	LINK_ID_LAYERBLEND2,
	LINK_ID_LAYERBLEND1,
	LINK_ID_LAYERBLEND0,
};

void dc_ed_pec_src_sel(struct dc_ed *ed, enum dc_link_id src)
{
	int i;

	for (i = 0; i < ARRAY_SIZE(src_sels); i++) {
		if (src_sels[i] == src) {
			regmap_write(ed->reg_pec, PIXENGCFG_DYNAMIC, src);
			return;
		}
	}
}

Because fetch units only connect to LayerBlend secondary inputs (sec_sels[] in
dc-lb.c), LINK_ID_FETCHWARP2 and LINK_ID_FETCHLAYER0 are rejected by
dc_ed_pec_src_sel(). However, dc_ed_pec_src_sel_get() accepts them and returns
0 instead of -EINVAL, which could cause callers such as
dc_crtc_disable_at_boot() to treat an invalid ExtDst source as an active
pipeline.

Would it be better to iterate over src_sels[] in dc_ed_pec_src_sel_get() just
like dc_ed_pec_src_sel() does?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-imx8-dc-prefetch-v7-0-a0b50cc4fc65@nxp.com?part=10