Re: [PATCH v4 6/7] arm64: dts: renesas: sparrow-hawk: Enable DisplayPort by adding DSC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Incorrect PM Runtime error handling and improper use of `WARN_ON`.
- [High] DRM bridge registration occurs before PM runtime is enabled.
- [High] Blind dereference of an unvalidated bridge pointer when skipping the DSC block.
- [High] Hardcoding the DSC bridge in the SoC `.dtsi` breaks the display for boards that disable the DSC.
- [Low] Call to a non-existent function `of_drm_get_bridge_by_endpoint`.
--

commit 9f241229b3364b6a9d83e1e7ed9e9076efdd0876
Author: Marek Vasut <marek.vasut+renesas@mailbox.org>

arm64: dts: renesas: sparrow-hawk: Enable DisplayPort by adding DSC

This commit enables the DisplayPort output on the sparrowhawk board by
activating the newly defined DSC block in the SoC device tree. It completes
the DSC series by wiring up the components for this specific board.

> diff --git a/arch/arm64/boot/dts/renesas/r8a779g0.dtsi b/arch/arm64/boot/dts/renesas/r8a779g0.dtsi
> index 82a7278836e57..123baaea8db45 100644
> --- a/arch/arm64/boot/dts/renesas/r8a779g0.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a779g0.dtsi
> @@ -2298,6 +2298,35 @@ du_out_dsi0: endpoint {
>  				port@1 {
>  					reg = <1>;
>  					du_out_dsi1: endpoint {
> +						remote-endpoint = <&dsc_in_dsi1>;

[Severity: High]
Will hardcoding the remote-endpoint to the DSC bridge break the display for
boards that disable the DSC?

Because the DSC node is marked status = "disabled" by default below, the DRM
OF helpers will return -ENODEV when the DU driver traverses this graph. This
would cause the DU probe to return -EPROBE_DEFER or -ENODEV indefinitely,
breaking the display pipeline for all boards based on this SoC that do not
explicitly enable the DSC.

> +					};
> +				};
> +			};
> +		};
> +
> +		dsc: bridge@feb8d000 {
[ ... ]
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_dsc.c b/drivers/gpu/drm/renesas/rcar-du/rcar_dsc.c
> new file mode 100644
> index 0000000000000..5a2ff31491360
> --- /dev/null
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_dsc.c
[ ... ]
> +static void rcar_dsc_atomic_enable(struct drm_bridge *bridge,
> +				   struct drm_atomic_commit *commit)
> +{
> +	struct rcar_dsc *dsc = bridge_to_rcar_dsc(bridge);
> +
> +	WARN_ON(pm_runtime_get_sync(dsc->dev));

[Severity: High]
Is it safe to wrap pm_runtime_get_sync() directly in a WARN_ON()?

If the device is already active, pm_runtime_get_sync() returns 1, which will
falsely trigger a warning and stack trace here on valid hot paths. If the
function returns a negative error code, the warning is printed but the
PM usage counter is not decremented, leaking the PM reference and attempting
to configure an unpowered device.

> +}
[ ... ]
> +static int rcar_dsc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rcar_dsc *dsc;
> +	int ret;
> +
> +	dsc = devm_drm_bridge_alloc(dev, struct rcar_dsc, bridge,
> +				    &rcar_dsc_bridge_ops);
> +	if (IS_ERR(dsc))
> +		return PTR_ERR(dsc);
> +
> +	platform_set_drvdata(pdev, dsc);
> +
> +	dsc->dev = &pdev->dev;
> +
> +	dsc->bridge.next_bridge = of_drm_get_bridge_by_endpoint(dev->of_node,
> +								1, 0);

[Severity: Low]
Could you clarify if the function of_drm_get_bridge_by_endpoint() exists in
the kernel tree?

It appears the correct API might be devm_drm_of_get_bridge() or
drm_of_find_panel_or_bridge(). Using a non-existent function will cause an
immediate build failure due to -Werror=implicit-function-declaration.

> +	if (IS_ERR(dsc->bridge.next_bridge))
> +		return PTR_ERR(dsc->bridge.next_bridge);
> +
> +	dsc->bridge.of_node = dev->of_node;
> +
> +	ret = devm_drm_bridge_add(dev, &dsc->bridge);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_enable(&pdev->dev);

[Severity: High]
Can registering the DRM bridge before enabling PM runtime introduce a race
condition?

Calling devm_drm_bridge_add() exposes the bridge globally to other drivers
like the DU driver while its PM runtime is still disabled. If another driver
attaches and enables the bridge during this window, pm_runtime_get_sync() will
fail with -EACCES.

> +
> +	return 0;
> +}
[ ... ]
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c
> index 1a246ebbfc613..5679983fc4d1a 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c
> @@ -754,6 +754,15 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
>  	     (BIT(RCAR_DU_OUTPUT_DSI0) | BIT(RCAR_DU_OUTPUT_DSI1)))) {
>  		struct drm_bridge *bridge = rcdu->dsi[rcrtc->index];
>  
> +		/*
> +		 * When we have a DSC block between the DU and the DSI,
> +		 * the "bridge" points to the DSC. Detect the DSC by looking
> +		 * at the bridge type, and skip the DSC if the bridge is not
> +		 * the DSI bridge.
> +		 */
> +		if (bridge->type != DRM_MODE_CONNECTOR_DSI)
> +			bridge = bridge->next_bridge;
> +
>  		rcar_mipi_dsi_pclk_enable(bridge, state);

[Severity: High]
What prevents a blind dereference of bridge->next_bridge from causing an
invalid pointer computation here?

If next_bridge is NULL (e.g., due to a disconnected or missing OF graph node)
or is not a MIPI DSI bridge, passing it into rcar_mipi_dsi_pclk_enable() will
cause bridge_to_rcar_mipi_dsi() to compute an invalid pointer via
container_of. This regression could result in memory corruption or an
immediate system crash.

>  	}
>  
> @@ -796,6 +805,15 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
>  	     (BIT(RCAR_DU_OUTPUT_DSI0) | BIT(RCAR_DU_OUTPUT_DSI1)))) {
>  		struct drm_bridge *bridge = rcdu->dsi[rcrtc->index];
>  
> +		/*
> +		 * When we have a DSC block between the DU and the DSI,
> +		 * the "bridge" points to the DSC. Detect the DSC by looking
> +		 * at the bridge type, and skip the DSC if the bridge is not
> +		 * the DSI bridge.
> +		 */
> +		if (bridge->type != DRM_MODE_CONNECTOR_DSI)
> +			bridge = bridge->next_bridge;
> +
>  		/*
>  		 * Disable the DSI clock output, see
>  		 * rcar_du_crtc_atomic_enable().

[Severity: High]
Similarly to the enable path, is this unchecked dereference of
bridge->next_bridge safe in the disable path?

If next_bridge is NULL or not a MIPI DSI bridge, it will be passed to
rcar_mipi_dsi_pclk_disable() where bridge_to_rcar_mipi_dsi() will compute an
invalid pointer, leading to a similar regression where memory is corrupted.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-rcar-du-dsc-v4-0-93096a1b56a3@ideasonboard.com?part=6