Re: [PATCH v2 10/46] drm/mediatek: Introduce and use path/comp definition structures

["CK Hu (胡俊光)" <ck.hu@mediatek.com>]

On Tue, 2026-07-14 at 13:43 +0200, AngeloGioacchino Del Regno wrote:
> In preparation for a bigger restructuring of Display Controller
> HW sub-component handing and path building, introduce two new

handling

> mtk_drm_comp_definition and mtk_drm_path definition structures,
> meant to replace all of the {main,ext,third}_path pointers and
> supportive length variables.
> 
> Obviously, also change all of the functions that were previously
> using the old path array pointers to use the new structures.
> 
> This made possible to loop through the path definition structures,

This made it possible to

> shortening various code paths and hence improving readability but,
> as a nice side effect, now it is possible to easily extend the
> maximum number of outputs that are supported by the driver.
> Of course, even though that could be done by simply extending the
> already present mtk_crtc_path enumeration (which now also indexes
> the mtk_drm_path_definition array in mtk_mmsys_driver_data), doing
> so will increase the memory footprint for all platforms (even the
> ones with a single output): this is currently left unresolved, but
> this change makes performing a future optimization to add dynamic
> CRTC output indexing(/numbers) way easier than before.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---

[snip]

>  
> -int mtk_crtc_create(struct drm_device *drm_dev, const unsigned int *path,
> -		    unsigned int path_len, int priv_data_index,
> +int mtk_crtc_create(struct drm_device *drm_dev,
> +		    enum mtk_crtc_path path_sel, int priv_data_index,
>  		    const struct mtk_drm_route *conn_routes,
>  		    unsigned int num_conn_routes)
>  {
>  	struct mtk_drm_private *priv = drm_dev->dev_private;
> +	const struct mtk_drm_path_definition *output_path;
>  	struct device *dev = drm_dev->dev;
>  	struct mtk_ddp_comp *dma_comp;
>  	struct mtk_crtc *mtk_crtc;
> @@ -1043,16 +1046,14 @@ int mtk_crtc_create(struct drm_device *drm_dev, const unsigned int *path,
>  	struct drm_crtc *tmp;
>  	int crtc_i = 0;
>  
> -	if (!path)
> -		return 0;
> -
>  	priv = priv->all_drm_private[priv_data_index];
> +	output_path = &priv->data->output_paths[path_sel];
>  
>  	drm_for_each_crtc(tmp, drm_dev)
>  		crtc_i++;
>  
> -	for (i = 0; i < path_len; i++) {
> -		enum mtk_ddp_comp_id comp_id = path[i];
> +	for (i = 0; i < output_path[i].len; i++) {

i < output_path->len

> +		enum mtk_ddp_comp_id comp_id = output_path->comp[i].type;
>  		struct device_node *node;
>  		struct mtk_ddp_comp *comp;
>  
> @@ -1076,12 +1077,13 @@ int mtk_crtc_create(struct drm_device *drm_dev, const unsigned int *path,
>  	}
>  
>  	mtk_crtc = devm_kzalloc(dev,
> -				struct_size(mtk_crtc, ddp_comp, path_len + (conn_routes ? 1 : 0)),
> +				struct_size(mtk_crtc, ddp_comp,
> +					    output_path->len + (conn_routes ? 1 : 0)),
>  				GFP_KERNEL);
>  	if (!mtk_crtc)
>  		return -ENOMEM;
>  
> -	mtk_crtc->ddp_comp_nr = path_len;
> +	mtk_crtc->ddp_comp_nr = output_path->len;
>  	mtk_crtc->mmsys_dev = priv->mmsys_dev;
>  
>  	mtk_crtc->mutex = mtk_mutex_get(priv->mutex_dev);
> @@ -1092,7 +1094,7 @@ int mtk_crtc_create(struct drm_device *drm_dev, const unsigned int *path,
>  	}
>  
>  	for (i = 0, j = 0; i < mtk_crtc->ddp_comp_nr; i++, j++) {
> -		unsigned int comp_id = path[i];
> +		unsigned int comp_id = output_path->comp[i].type;
>  		struct mtk_ddp_comp *comp;
>  
>  		comp = mtk_ddp_comp_find_by_id(&priv->hlist, comp_id);
> @@ -1138,7 +1140,7 @@ int mtk_crtc_create(struct drm_device *drm_dev, const unsigned int *path,
>  	 * In the case of ovl_adaptor sub driver, it needs to use the
>  	 * dma_dev_get function to get representative dma dev.
>  	 */
> -	dma_comp = mtk_ddp_comp_find_by_id(&priv->hlist, path[0]);
> +	dma_comp = mtk_ddp_comp_find_by_id(&priv->hlist, output_path->comp[0].type);
>  	if (dma_comp == NULL) {
>  		dev_err(dev, "Could not find appropriate DMA device!\n");
>  		return -EINVAL;
> 

[snip]

>  
> -static bool mtk_ddp_path_available(const unsigned int *path,
> -				   unsigned int path_len,
> +static bool mtk_ddp_path_available(const struct mtk_drm_path_definition *output_path,
>  				   struct device_node **comp_node)

You have remove all the caller of mtk_ddp_path_available(), so remove this function in this patch.
If you need this function in later patch, add this function in that patch.

>  {
>  	unsigned int i;
>  
> -	if (!path || !path_len)
> -		return false;
> -
> -	for (i = 0U; i < path_len; i++) {
> +	for (i = 0U; i < output_path->len; i++) {
>  		/* OVL_ADAPTOR doesn't have a device node */
> -		if (path[i] == DDP_COMPONENT_DRM_OVL_ADAPTOR)
> +		if (output_path->comp[i].type == DDP_COMPONENT_DRM_OVL_ADAPTOR)
>  			continue;
>  
> -		if (!comp_node[path[i]])
> +		if (!comp_node[output_path->comp[i].type])
>  			return false;
>  	}
>  
> @@ -576,31 +568,14 @@ int mtk_find_possible_crtcs(struct drm_device *drm, struct device *dev)
>  		priv_n = private->all_drm_private[j];
>  		data = priv_n->data;
>  
> -		if (mtk_ddp_path_available(data->main_path, data->main_len,
> -					   priv_n->comp_node)) {
> -			if (mtk_ddp_comp_find(dev, data->main_path,
> -					      data->main_len,
> -					      &priv_n->hlist))
> -				return BIT(i);
> -			i++;
> -		}
> -
> -		if (mtk_ddp_path_available(data->ext_path, data->ext_len,
> -					   priv_n->comp_node)) {
> -			if (mtk_ddp_comp_find(dev, data->ext_path,
> -					      data->ext_len,
> -					      &priv_n->hlist))
> -				return BIT(i);
> -			i++;
> -		}
> +		for (i = 0; i < MAX_CRTC; i++) {
> +			if (!data->output_paths[i].len)
> +				continue;
>  
> -		if (mtk_ddp_path_available(data->third_path, data->third_len,
> -					   priv_n->comp_node)) {
> -			if (mtk_ddp_comp_find(dev, data->third_path,
> -					      data->third_len,
> -					      &priv_n->hlist))
> +			ret = mtk_ddp_comp_find(dev, &data->output_paths[i],
> +						&priv_n->hlist);
> +			if (ret)
>  				return BIT(i);
> -			i++;
>  		}
>  	}
>  

[snip]

>  
> -static const unsigned int mt7623_mtk_ddp_main[] = {
> -	DDP_COMPONENT_OVL0,
> -	DDP_COMPONENT_RDMA0,
> -	DDP_COMPONENT_COLOR0,
> -	DDP_COMPONENT_BLS,
> -	DDP_COMPONENT_DPI0,
> +struct mtk_drm_path_definition mt2701_legacy_paths[MAX_CRTC] = {

static const

> +	[CRTC_MAIN] = {
> +		.comp = mt2701_mtk_ddp_main,
> +		.len = ARRAY_SIZE(mt2701_mtk_ddp_main),
> +	},
> +	[CRTC_EXT] = {
> +		.comp = mt2701_mtk_ddp_ext,
> +		.len = ARRAY_SIZE(mt2701_mtk_ddp_ext),
> +	},
>  };
>  

[snip]

> @@ -895,8 +966,7 @@ static int mtk_drm_of_get_ddp_ep_cid(struct device_node *node,
>   * mtk_drm_of_ddp_path_build_one - Build a Display HW Pipeline for a CRTC Path
>   * @dev:          The mediatek-drm device
>   * @cpath:        CRTC Path relative to a VDO or MMSYS
> - * @out_path:     Pointer to an array that will contain the new pipeline
> - * @out_path_len: Number of entries in the pipeline array
> + * @out_path:     Pointer to the structure that will contain the new pipeline
>   *
>   * MediaTek SoCs can use different DDP hardware pipelines (or paths) depending
>   * on the board-specific desired display configuration; this function walks
> @@ -910,20 +980,19 @@ static int mtk_drm_of_get_ddp_ep_cid(struct device_node *node,
>   * * %-ENOMEM - Failure to allocate pipeline array to pass to the caller
>   */
>  static int mtk_drm_of_ddp_path_build_one(struct device *dev, enum mtk_crtc_path cpath,
> -					 const unsigned int **out_path,
> -					 unsigned int *out_path_len)
> +					 struct mtk_drm_path_definition *out_path)
>  {
> +	struct mtk_drm_comp_definition temp_path[MTK_DISP_CONTROLLER_MAX_COMP_PER_PATH];

You change the temp_path[] size to MTK_DISP_CONTROLLER_MAX_COMP_PER_PATH,
but below do-while check idx < DDP_COMPONENT_DRM_ID_MAX,
you need to change the idx limit.

>  	struct device_node *next = NULL, *prev, *vdo = dev->parent->of_node;
> -	unsigned int temp_path[DDP_COMPONENT_DRM_ID_MAX] = { 0 };
> -	unsigned int *final_ddp_path;
> -	unsigned short int idx = 0;
>  	bool ovl_adaptor_comp_added = false;
> +	unsigned short int idx = 0;
> +	size_t final_comp_sz;
>  	int ret;
>  
>  	/* Get the first entry for the temp_path array */
>  	ret = mtk_drm_of_get_ddp_ep_cid(vdo, 0, cpath, &next, &temp_path[idx]);
>  	if (ret) {
> -		if (next && temp_path[idx] == DDP_COMPONENT_DRM_OVL_ADAPTOR) {
> +		if (next && temp_path[idx].type == DDP_COMPONENT_DRM_OVL_ADAPTOR) {
>  			dev_dbg(dev, "Adding OVL Adaptor for %pOF\n", next);
>  			ovl_adaptor_comp_added = true;
>  		} else {
> @@ -957,7 +1026,7 @@ static int mtk_drm_of_ddp_path_build_one(struct device *dev, enum mtk_crtc_path
>  		 * to probe that component master driver of which only one instance
>  		 * is needed and possible.
>  		 */
> -		if (temp_path[idx] == DDP_COMPONENT_DRM_OVL_ADAPTOR) {
> +		if (temp_path[idx].type == DDP_COMPONENT_DRM_OVL_ADAPTOR) {
>  			if (!ovl_adaptor_comp_added)
>  				ovl_adaptor_comp_added = true;
>  			else
> @@ -973,7 +1042,7 @@ static int mtk_drm_of_ddp_path_build_one(struct device *dev, enum mtk_crtc_path
>  		return ret;
>  
>  	/* If the last entry is not a final display output, the configuration is wrong */
> -	switch (temp_path[idx - 1]) {
> +	switch (temp_path[idx - 1].type) {
>  	case DDP_COMPONENT_DP_INTF0:
>  	case DDP_COMPONENT_DP_INTF1:
>  	case DDP_COMPONENT_DPI0:
> @@ -985,30 +1054,31 @@ static int mtk_drm_of_ddp_path_build_one(struct device *dev, enum mtk_crtc_path
>  		break;
>  	default:
>  		dev_err(dev, "Invalid display hw pipeline. Last component: %d (ret=%d)\n",
> -			temp_path[idx - 1], ret);
> +			temp_path[idx - 1].type, ret);
>  		return -EINVAL;
>  	}
>  
> -	final_ddp_path = devm_kmemdup(dev, temp_path, idx * sizeof(temp_path[0]), GFP_KERNEL);
> -	if (!final_ddp_path)
> +	/* Pipeline built! */
> +	out_path->len = idx;
> +	final_comp_sz = out_path->len * sizeof(out_path->comp[0]);
> +	out_path->comp = devm_kmemdup(dev, temp_path, final_comp_sz, GFP_KERNEL);
> +	if (!out_path->comp)
>  		return -ENOMEM;
>  
>  	dev_dbg(dev, "Display HW Pipeline built with %d components.\n", idx);
>  
> -	/* Pipeline built! */
> -	*out_path = final_ddp_path;
> -	*out_path_len = idx;
> -
>  	return 0;
>  }
>  
>  static int mtk_drm_of_ddp_path_build(struct device *dev, struct device_node *node,
>  				     struct mtk_mmsys_driver_data *data)
>  {
> +	struct mtk_drm_path_definition *output_paths;
>  	struct device_node *ep_node;
>  	struct of_endpoint of_ep;
>  	bool output_present[MAX_CRTC] = { false };
> -	int ret;
> +	u8 num_outputs_present = 0;
> +	int i, ret;
>  
>  	for_each_endpoint_of_node(node, ep_node) {
>  		ret = of_graph_parse_endpoint(ep_node, &of_ep);
> @@ -1024,33 +1094,30 @@ static int mtk_drm_of_ddp_path_build(struct device *dev, struct device_node *nod
>  		}
>  
>  		output_present[of_ep.id] = true;
> +		num_outputs_present++;
>  	}
> +	if (ret == 0 && num_outputs_present == 0)

ret is not used before this line, so remove checking of 'ret == 0'.

> +		ret = dev_err_probe(dev, -ENXIO, "No display outputs found.\n");
>  
>  	if (ret) {
>  		of_node_put(ep_node);
>  		return ret;
>  	}
>  
> -	if (output_present[CRTC_MAIN]) {
> -		ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_MAIN,
> -						    &data->main_path, &data->main_len);
> -		if (ret && ret != -ENODEV)
> -			return ret;
> -	}
> +	/* This can be optimized after making CRTC numbers dynamic */
> +	output_paths = devm_kcalloc(dev, MAX_CRTC, sizeof(*output_paths), GFP_KERNEL);
> +	if (!output_paths)
> +		return -ENOMEM;
>  
> -	if (output_present[CRTC_EXT]) {
> -		ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_EXT,
> -						    &data->ext_path, &data->ext_len);
> -		if (ret && ret != -ENODEV)
> -			return ret;
> -	}
> +	for (i = 0; i < MAX_CRTC; i++) {
> +		if (!output_present[i])
> +			continue;
>  
> -	if (output_present[CRTC_THIRD]) {
> -		ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_THIRD,
> -						    &data->third_path, &data->third_len);
> +		ret = mtk_drm_of_ddp_path_build_one(dev, i, &output_paths[i]);
>  		if (ret && ret != -ENODEV)
>  			return ret;
>  	}
> +	data->output_paths = output_paths;
>  
>  	return 0;
>  }
> 

[snip]

> +
>  struct mtk_mmsys_driver_data {
> -	const unsigned int *main_path;
> -	unsigned int main_len;
> -	const unsigned int *ext_path;
> -	unsigned int ext_len;
> -	const unsigned int *third_path;
> -	unsigned int third_len;
> +	struct mtk_drm_path_definition *output_paths;
> +	u8 num_output_paths;

num_output_paths is not used. Remove it.

Regards,
CK

>  	const struct mtk_drm_route *conn_routes;
>  	unsigned int num_conn_routes;
>