[PATCH 06/42] drm/mediatek: Use hashtable for components discovery and registration
AngeloGioacchino Del Regno
angelogioacchino.delregno at collabora.com
Tue Jul 14 04:30:07 PDT 2026
On 7/7/26 13:56, AngeloGioacchino Del Regno wrote:
> On 7/6/26 09:02, CK Hu (胡俊光) wrote:
>> 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 at 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.
>>
>
> I'm not inventing conn_route, that's something that was already there as a dynamic
> replacement of the last component in a stack of possible outputs for a single
> full-fledged display pipeline.
>
> The change here is about changing the search/retrieval of a `route_ddp` from a
> number that comes from an enumeration, to a full component structure instead....
>
>>> + 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.
>>
>
> .... and the component structure, with the new code, has to be *initialized*: this
> is because we cannot assume that a component that was not registered will actually
> be registered later (which was a broken behavior of mediatek-drm!).
>
>
>
> ----> IMPORTANT PART: <-----
> Note that, at this point, "comp" is *not* pointing to a *statically* initialized
> array of components, but to a *dynamically* initialized one: while before this the
> comp pointer was guaranteed to be *always* a valid pointer to a comp structure, now
> it is *not guaranteed anymore*, because again - it's *dynamically* allocated, so
> we NEED to check for comp != NULL, or we're exposing ourselves to a possible NULL
> pointer kernel panic (or worse)!
>
>
>
> If the component is not initialized at this point, it simply means that it cannot
> be - in any way - used by mediatek-drm, so.. if !comp -> continue means effectively
> "if the component is not registered, we cannot use it, hence don't try to evaluate
> the possibility of using it".
>
> Is it clear now?
>
>>> 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.
>>
>
> In mtk_crtc_create, at this point, the component (which always has a node, pointed
> by node = priv->comp_node[comp_id]) has to be initialized by mtk_ddp_comp.
>
> Keeping in mind that the component is actually a device, if the component itself
> was not initialized at this point, this means that there is NO DEVice associated
> with it.
>
> This is why -ENODEV.
>
> Also, we're printing %pOF (like before) because this gives hints about precisely
> which one was not registered at this point.
>
>>> 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.
>>
>
> The iterator here tries to still setup a display pipeline in case a component is
> missing, and this actually works in a couple of corner cases, which were not
> possible before the restructuring of mediatek-drm.
>
> In later commits, this case will be handled differently: the problem here is rather
> that the code readability is not the best at this point in time (of the series),
> but this will make a lot more sense when going forward in this exact series.
>
> The main issue here is that mediatek-drm's complexity is very, very high, and the
> maintainability suffers a lot: doing each and every change as a perfectly clean
> (and perfectly readable) diff means adding another ~4k thousand lines of changes
> that are simply addition and removal of the same lines over and over again.
>
>>> + 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.
>>
>
> Yeah, I agree. If you're really worried about seeing this instead of !dma_comp I
> can change it here... but if you can ignore that, in later commits this will be
> removed and completely changed so that it becomes
>
> ret = mtk_crtc_find_suitable_dma_dev(mtk_crtc);
> if (ret) {
> dev_err(dev, "Could not find appropriate DMA device!\n");
> return -EINVAL;
> }
> dev_dbg(dev, "Using DMA device %pOF\n", mtk_crtc->dma_dev->of_node);
>
> Note that if I change this simple line here, I'll have to fix other patches as
> they won't apply anymore because of the changed line... so again, if we could
> ignore that it would be great...
>
>>> + 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.
>>
>
> Already explained why we need comp to be checked for NULL.
>
>>> 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,
>>
>
> That was only done for readability purposes. This will be entirely moved out of
> here in a later commit.
Actually, no, I gave the wrong information here.
Registering with mtk_ddp_comp_init(dev, xxxx ...) will make the called function to
assign a pointer to mediatek-drm's device, but since the mtk_ddp_comp_init function
changed, this needs to pass a pointer to the OVL ADAPTOR's device, not to the main
mediatek-drm device.
So this has to be mtk_ddp_comp_init(&ovl_adaptor->dev, ...) otherwise this will not
work as intended.
Cheers,
Angelo
More information about the Linux-mediatek
mailing list