| Message ID | 20250424-drm-bridge-convert-to-alloc-api-v2-30-8f91a404d86b@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm: convert all bridges to devm_drm_bridge_alloc() | expand |
Hello Liu, On Tue, 29 Apr 2025 10:10:55 +0800 Liu Ying <victor.liu@nxp.com> wrote: > Hi, > > On 04/25/2025, Luca Ceresoli wrote: > > This is the new API for allocating DRM bridges. > > > > This driver embeds an array of channels in the main struct, and each > > channel embeds a drm_bridge. This prevents dynamic, refcount-based > > deallocation of the bridges. > > > > To make the new, dynamic bridge allocation possible: > > > > * change the array of channels into an array of channel pointers > > * allocate each channel using devm_drm_bridge_alloc() > > * adapt the code wherever using the channels > > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> [...] > > @@ -345,8 +351,8 @@ static int imx8qxp_pc_bridge_probe(struct platform_device *pdev) > > free_child: > > of_node_put(child); > > > > - if (i == 1 && pc->ch[0].next_bridge) > > - drm_bridge_remove(&pc->ch[0].bridge); > > + if (i == 1 && pc->ch[0]->next_bridge) > > Since this patch makes pc->ch[0] and pc->ch[1] be allocated separately, > pc->ch[0] could be NULL if channel0 is not available, hence a NULL pointer > dereference here... See below for this. > > + drm_bridge_remove(&pc->ch[0]->bridge); > > > > pm_runtime_disable(dev); > > return ret; > > @@ -359,7 +365,7 @@ static void imx8qxp_pc_bridge_remove(struct platform_device *pdev) > > int i; > > > > for (i = 0; i < 2; i++) { > > - ch = &pc->ch[i]; > > + ch = pc->ch[i]; > > > > if (!ch->is_available) > > ...and here too. This is indeed a bug, I should have checked the pointer for being non-NULL. Looking at that more closely, I think the is_available flag can be entirely removed now. The allocation itself (ch != NULL) now is equivalent. Do you think my reasoning is correct? Ouch! After writing the previous paragraph I realized you proposed this a few lines below! OK, removing is_available. :) [...] > On top of this patch series, this issue doesn't happen if I apply the below > change: [...] > @@ -351,7 +349,7 @@ static int imx8qxp_pc_bridge_probe(struct platform_device *pdev) > free_child: > of_node_put(child); > > - if (i == 1 && pc->ch[0]->next_bridge) > + if (i == 1 && pc->ch[0]) > drm_bridge_remove(&pc->ch[0]->bridge); Unrelated to this patch, but as I looked at it more in depth now, I'm not sure this whole logic is robust, even in the original code. The 'i == 1' check here seems to mean "if some error happened when handling channel@1, that means channel@0 was successfully initialized, so let's clean up channel 0". However my understanding of the bindings is that device tree is allowed to have the channel@1 node before the channel@0 node (or even channel@1 without channel@0, but that's less problematic here). In such case (channel@1 before channel@0), this would happen: 1. alloc and init ch[1], all OK 2. alloc and init ch[0], an error happens (e.g. of_graph_get_remote_node() fails) So we'd reach the free_child: label, and we should call drm_bridge_remove() for ch[1]->bridge, but there's no code to do that. To be robust in such a case, I think both channels need to be checked independently, as the status of one does not imply the status of the other. E.g.: for (i = 0; i < 2; i++) if (pc->ch[i] && pc->ch[i]->next_bridge) drm_bridge_remove(&pc->ch[i]->bridge); (which is similar to what .remove() does after the changes discussed in this thread, and which I have queued for v3) What's your opinion? Do you think I missed anything? Thanks for taking the time to dig into this! Best regards, Luca
diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c index 1f6fd488e7039e943351006d3373009f0c15cb08..40a8a5a53a781137e722309ff91692cf90d881da 100644 --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c @@ -68,7 +68,7 @@ struct imx8qxp_pc_channel { struct imx8qxp_pc { struct device *dev; - struct imx8qxp_pc_channel ch[2]; + struct imx8qxp_pc_channel *ch[2]; struct clk *clk_apb; void __iomem *base; }; @@ -307,7 +307,14 @@ static int imx8qxp_pc_bridge_probe(struct platform_device *pdev) goto free_child; } - ch = &pc->ch[i]; + ch = devm_drm_bridge_alloc(dev, struct imx8qxp_pc_channel, bridge, + &imx8qxp_pc_bridge_funcs); + if (IS_ERR(ch)) { + ret = PTR_ERR(ch); + goto free_child; + } + + pc->ch[i] = ch; ch->pc = pc; ch->stream_id = i; @@ -333,7 +340,6 @@ static int imx8qxp_pc_bridge_probe(struct platform_device *pdev) of_node_put(remote); ch->bridge.driver_private = ch; - ch->bridge.funcs = &imx8qxp_pc_bridge_funcs; ch->bridge.of_node = child; ch->is_available = true; @@ -345,8 +351,8 @@ static int imx8qxp_pc_bridge_probe(struct platform_device *pdev) free_child: of_node_put(child); - if (i == 1 && pc->ch[0].next_bridge) - drm_bridge_remove(&pc->ch[0].bridge); + if (i == 1 && pc->ch[0]->next_bridge) + drm_bridge_remove(&pc->ch[0]->bridge); pm_runtime_disable(dev); return ret; @@ -359,7 +365,7 @@ static void imx8qxp_pc_bridge_remove(struct platform_device *pdev) int i; for (i = 0; i < 2; i++) { - ch = &pc->ch[i]; + ch = pc->ch[i]; if (!ch->is_available) continue;
This is the new API for allocating DRM bridges. This driver embeds an array of channels in the main struct, and each channel embeds a drm_bridge. This prevents dynamic, refcount-based deallocation of the bridges. To make the new, dynamic bridge allocation possible: * change the array of channels into an array of channel pointers * allocate each channel using devm_drm_bridge_alloc() * adapt the code wherever using the channels Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- Cc: Liu Ying <victor.liu@nxp.com> --- drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)