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
> <[email protected]>
> ---
> 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);
> }
>
