Re: [PATCH v2 23/46] drm/mediatek: Fully migrate to new Display Controller HW indexing

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

On Tue, 2026-07-14 at 13:43 +0200, AngeloGioacchino Del Regno wrote:
> Perform a full migration to HW Type -> HW Instance ID indexing
> for all of the currently supported Display Controller hardware
> sub-components.
> 
> This allows to remove the big fixed-size catch-all components
> array (that contains as many element as much as the number of
> supported hardware and number of instances of that), which is
> currently growing bigger and bigger at every sub-ip support
> addition in mediatek-drm.
> 
> Note that this cleanup became necessary seen the need to add
> support for new components found in MT8196, MT8994 and other
> variants of it, and of course those SoCs won't be the last
> ones to be upstreamed... so the size of that array would get
> out of control pretty fast (and again, it's already very big).
> 
> As a side effect, while saving memory compared to before this
> change, this also allows to have up to 24 (as an extensible fixed
> size declared in this driver) instances of the same HW Type with
> no further increase in memory footprint.
> Similarly, adding support for new hardware will not produce an
> increase of memory footprint on all SoCs (if not only temporarily
> while the probe process is in place) as there's no more catch-all
> array, but only a dynamically allocated list of per display output
> components.
> 
> Unfortunately, this change needs to go along with the related one
> in the mtk-mmsys driver, otherwise functionality of the Display
> Controller will regress.
> 
> The only way to make this possible in two steps is to duplicate
> all of the routes structure arrays for all of the SoCs, which
> would result in a total of around ~1200 lines changed twice, and
> that ignores the big increase in size for this driver during the
> migration process so, in order to avoid useless bloat, I opted
> for an inter-dependency between the two changes: mediatek-drm
> and mtk-mmsys.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---

[snip]

> @@ -1143,25 +1158,15 @@ int mtk_crtc_create(struct drm_device *drm_dev,
>  		crtc_i++;
>  
>  	for (i = 0; i < output_path[i].len; i++) {
> -		enum mtk_ddp_comp_id comp_id = output_path->comp[i].type;
> -		struct device_node *node;
> +		enum mtk_ddp_comp_type comp_type = output_path->comp[i].type;
> +		u8 comp_inst = output_path->comp[i].inst_id;
>  		struct mtk_ddp_comp *comp;
>  
> -		node = priv->comp_node[comp_id];
> -		comp = mtk_ddp_comp_find_by_id(&priv->hlist, comp_id);
> -
> -		/* Not all drm components have a DTS device node, such as ovl_adaptor,
> -		 * which is the drm bring up sub driver
> -		 */
> -		if (!node && comp_id != DDP_COMPONENT_DRM_OVL_ADAPTOR) {
> -			dev_info(dev,
> -				"Not creating crtc %d because component %d is disabled or missing\n",
> -				crtc_i, comp_id);
> -			return 0;

The original behavior for disabled component would return 0, but you change the behavior here.
Is is safe for legacy SoC?

> -		}
> -
> +		comp = mtk_ddp_comp_find_by_id(&priv->hlist, comp_type, comp_inst);
>  		if (!comp || !comp->dev) {
> -			dev_err(dev, "Component %pOF not initialized\n", node);
> +			dev_err(dev,
> +				"CRTC%d: Component type=%u inst=%u not initialized\n",
> +				crtc_i, comp_type, comp_inst);
>  			return -ENODEV;
>  		}
>  	}
> 

[snip]

>  static bool mtk_ddp_find_comp_dev_in_table(const struct mtk_drm_comp_list *hlist,
> -					   const unsigned int comp_id,
> +					   const unsigned int comp_type,
>  					   struct device *dev)
>  {
>  	struct mtk_ddp_comp *ddp_comp;
>  
> -	hash_for_each_possible(hlist->ddp_list, ddp_comp, lnode, comp_id) {
> +	hash_for_each_possible(hlist->ddp_list, ddp_comp, lnode, comp_type) {

You change the comp_id to comp_type.
If the correct match is DPI0, after this patch, the DPI1 would match?

>  		if (ddp_comp->dev == dev)
>  			return true;
>  	}
> 

[snip]

>  int mtk_ddp_comp_get_id(struct device_node *node,
>  			enum mtk_ddp_comp_type comp_type)
>  {
> -	int id = of_alias_get_id(node, mtk_ddp_comp_stem[comp_type]);
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(mtk_ddp_matches); i++) {
> -		if (comp_type == mtk_ddp_matches[i].type &&
> -		    (id < 0 || id == mtk_ddp_matches[i].alias_id))
> -			return i;
> +	/* If there's an alias, return the ID from that */
> +	if (mtk_ddp_comp_stem[comp_type]) {
> +		int alias_id = of_alias_get_id(node, mtk_ddp_comp_stem[comp_type]);
> +		if (alias_id >= 0)
> +			return alias_id;
>  	}
>  
> -	return -EINVAL;
> +	return 0;

0 is valid id so this function would not return error.
You change this behavior, so I would like some explain.

>  }
>  
>  int mtk_find_possible_crtcs(struct drm_device *drm, struct device *dev)
> 

[snip]

> @@ -16,6 +16,7 @@
>  #include <drm/drm_modes.h>
>  
>  #define MTK_DISP_CONTROLLER_MAX_COMP_PER_PATH		24
> +#define MTK_DISP_CONTROLLER_MAX_HW_COMP_INSTANCE	32

MTK_DISP_CONTROLLER_MAX_HW_COMP_INSTANCE is useless, so drop it.

>  
>  struct device;
>  struct device_node;
> 

[snip]

> @@ -924,18 +928,16 @@ static int mtk_drm_probe(struct platform_device *pdev)
>  			continue;
>  		}
>  
> -		comp_id = mtk_ddp_comp_get_id(node, comp_type);
> -		if (comp_id < 0) {
> +		comp_inst_id = mtk_ddp_comp_get_id(node, comp_type);
> +		if (comp_inst_id < 0) {

comp_inst_id is u8, so this would never happen.

Regards,
CK

>  			dev_warn(dev, "Skipping unknown component %pOF\n",
>  				 node);
>  			continue;
>  		}
>  
> -		if (!mtk_drm_find_mmsys_comp(private, comp_id))
> +		if (!mtk_drm_find_mmsys_comp(private, comp_type, comp_inst_id))
>  			continue;
>  
> -		private->comp_node[comp_id] = of_node_get(node);
> -
>  		/*
>  		 * Currently only the AAL, CCORR, COLOR, GAMMA, MERGE, OVL, RDMA, DSI, and DPI
>  		 * blocks have separate component platform drivers and initialize their own
>