Re: [PATCH 06/42] drm/mediatek: Use hashtable for components discovery and registration

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

On Wed, 2026-07-01 at 14:20 +0200, AngeloGioacchino Del Regno wrote:
> As a preparation for refactoring the concept of hardware component
> identification, search, and final usage, remove the ddp_comp array
> of components and replace it with a hashtable, indexed by ID.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_crtc.c     | 44 ++++++++++++++++-------
>  drivers/gpu/drm/mediatek/mtk_ddp_comp.c | 48 ++++++++++++++++++-------
>  drivers/gpu/drm/mediatek/mtk_ddp_comp.h | 23 +++++++++++-
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c  |  7 ++--
>  drivers/gpu/drm/mediatek/mtk_drm_drv.h  |  2 +-
>  5 files changed, 94 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
> index f39f197057a7..3f4d6ab1bfc2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_crtc.c
> @@ -725,8 +725,12 @@ static void mtk_crtc_update_output(struct drm_crtc *crtc,
>  		crtc_state->connectors_changed, encoder_mask, crtc_index);
>  
>  	for (i = 0; i < mtk_crtc->num_conn_routes; i++) {
> -		unsigned int comp_id = mtk_crtc->conn_routes[i].route_ddp;
> -		struct mtk_ddp_comp *comp = &priv->ddp_comp[comp_id];
> +		const struct mtk_drm_route *conn_route = &mtk_crtc->conn_routes[i];
> +		struct mtk_ddp_comp *comp;
> +
> +		comp = mtk_ddp_comp_find_by_id(&priv->hlist, conn_route->route_ddp);

I don't know why you invent conn_route? Keep comp_id use it here looks the same.

> +		if (!comp)
> +			continue;

In original code, it never check comp is null or not.
I think it base on an assumption that it would not be null.
If it's null here, print error message and it's not necessary to do the rest thing.

>  
>  		if (comp->encoder_index >= 0 &&
>  		    (encoder_mask & BIT(comp->encoder_index))) {
> @@ -1028,10 +1032,11 @@ int mtk_crtc_create(struct drm_device *drm_dev, const unsigned int *path,
>  {
>  	struct mtk_drm_private *priv = drm_dev->dev_private;
>  	struct device *dev = drm_dev->dev;
> +	struct mtk_ddp_comp *dma_comp;
>  	struct mtk_crtc *mtk_crtc;
>  	unsigned int num_comp_planes = 0;
>  	int ret;
> -	int i;
> +	int i, j;
>  	bool has_ctm = false;
>  	uint gamma_lut_size = 0;
>  	struct drm_crtc *tmp;
> @@ -1051,7 +1056,7 @@ int mtk_crtc_create(struct drm_device *drm_dev, const unsigned int *path,
>  		struct mtk_ddp_comp *comp;
>  
>  		node = priv->comp_node[comp_id];
> -		comp = &priv->ddp_comp[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
> @@ -1063,7 +1068,7 @@ int mtk_crtc_create(struct drm_device *drm_dev, const unsigned int *path,
>  			return 0;
>  		}
>  
> -		if (!comp->dev) {
> +		if (!comp || !comp->dev) {
>  			dev_err(dev, "Component %pOF not initialized\n", node);

I think you should print other error message and return other value for !comp.

>  			return -ENODEV;
>  		}
> @@ -1089,12 +1094,17 @@ int mtk_crtc_create(struct drm_device *drm_dev, const unsigned int *path,
>  		return ret;
>  	}
>  
> -	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
> +	for (i = 0, j = 0; i < mtk_crtc->ddp_comp_nr; i++, j++) {
>  		unsigned int comp_id = path[i];
>  		struct mtk_ddp_comp *comp;
>  
> -		comp = &priv->ddp_comp[comp_id];
> -		mtk_crtc->ddp_comp[i] = comp;
> +		comp = mtk_ddp_comp_find_by_id(&priv->hlist, comp_id);
> +		if (!comp) {

In original code, it never check comp is null or not.
I think it base on an assumption that it would not be null.
If it's null here, print error message and it's not necessary to do the rest thing.

> +			j--;
> +			dev_dbg(dev, "Cannot find component %d.\n", comp_id);
> +			continue;
> +		}
> +		mtk_crtc->ddp_comp[j] = comp;
>  
>  		if (comp->funcs) {
>  			if (comp->funcs->gamma_set && comp->funcs->gamma_get_lut_size) {
> @@ -1131,7 +1141,14 @@ 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.
>  	 */
> -	mtk_crtc->dma_dev = mtk_ddp_comp_dma_dev_get(&priv->ddp_comp[path[0]]);
> +	dma_comp = mtk_ddp_comp_find_by_id(&priv->hlist, path[0]);
> +	if (dma_comp == NULL) {

if (!dma_comp) is more simple.

> +		dev_err(dev, "Could not find appropriate DMA device!\n");
> +		return -EINVAL;
> +	}
> +
> +	mtk_crtc->dma_dev = mtk_ddp_comp_dma_dev_get(dma_comp);
> +	dev_dbg(dev, "Using DMA device %pOF\n", mtk_crtc->dma_dev->of_node);
>  
>  	ret = mtk_crtc_init(drm_dev, mtk_crtc, crtc_i);
>  	if (ret < 0)
> @@ -1188,17 +1205,18 @@ int mtk_crtc_create(struct drm_device *drm_dev, const unsigned int *path,
>  		for (i = 0; i < num_conn_routes; i++) {
>  			unsigned int comp_id = conn_routes[i].route_ddp;
>  			struct device_node *node = priv->comp_node[comp_id];
> -			struct mtk_ddp_comp *comp = &priv->ddp_comp[comp_id];
> +			struct mtk_ddp_comp *comp = mtk_ddp_comp_find_by_id(&priv->hlist, comp_id);
>  
> -			if (!comp->dev) {
> +			if (!comp || !comp->dev) {

In original code, it never check comp is null or not.
I think it base on an assumption that it would not be null.
If it's null here, print error message and it's not necessary to do the rest thing.

>  				dev_dbg(dev, "comp_id:%d, Component %pOF not initialized\n",
>  					comp_id, node);
>  				/* mark encoder_index to -1, if route comp device is not enabled */
> -				comp->encoder_index = -1;
> +				if (comp)
> +					comp->encoder_index = -1;
>  				continue;
>  			}
>  
> -			mtk_ddp_comp_encoder_index_set(&priv->ddp_comp[comp_id]);
> +			mtk_ddp_comp_encoder_index_set(comp);
>  
> 

[snip]

> 		}
>  @@ -1116,8 +1118,7 @@ static int mtk_drm_probe(struct platform_device *pdev)
>  							    PLATFORM_DEVID_AUTO,
>  							    (void *)private->mmsys_dev,
>  							    sizeof(*private->mmsys_dev));
> -		private->ddp_comp[DDP_COMPONENT_DRM_OVL_ADAPTOR].dev = &ovl_adaptor->dev;
> -		mtk_ddp_comp_init(dev, NULL, &private->ddp_comp[DDP_COMPONENT_DRM_OVL_ADAPTOR],
> +		mtk_ddp_comp_init(&ovl_adaptor->dev, NULL, &private->hlist,

mtk_ddp_comp_init(dev, NULL, &private->hlist,

Regards,
CK

>  				  DDP_COMPONENT_DRM_OVL_ADAPTOR);
>  		component_match_add(dev, &match, compare_dev, &ovl_adaptor->dev);
>  	}
>