Re: [PATCH] drm/bridge: Fail an unsatisfiable bus format negotiation with -EINVAL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/bridge: drm_test_bridge_rgb_yuv_no_path() fails due to
  -ENOTSUPP change
- [Low] drm/bridge: missing Fixes: trailer on commit fixing -ENOTSUPP
  leak
--

commit 13f296e3d34a65b76d6c2d283252f11982c4fa06
Author: Arun Iyengar <aiyengar1990@gmail.com>

> drm/bridge: Fail an unsatisfiable bus format negotiation with -EINVAL
>
> drm_atomic_bridge_chain_select_bus_fmts() reports that no chain of bus
> formats fits with -ENOTSUPP, and drm_atomic_bridge_chain_check() passes
> that straight back through drm_atomic_helper_check_modeset() to the
> atomic and legacy modeset ioctls. -ENOTSUPP (524) is a kernel-internal
> code that userspace does not know; for a configuration the hardware
> cannot do the atomic API returns -EINVAL. Since the connector "color
> format" property makes the negotiation fail for any requested format a
> bridge chain cannot output, userspace now hits this routinely: a DW DP
> connector asked for YCbCr 4:2:0 on a sink without 4:2:0 support gets
> here with no matching format and would fail the atomic commit with
> errno 524, where an HDMI connector asked for the same returns EINVAL
> from the HDMI state helper.
>
> Return -EINVAL when the negotiation finds no chain, with a debug
> message naming the encoder.
>
> Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>

[Severity: Low]
Since this fixes leaking -ENOTSUPP to userspace during bus format
negotiation, should it include a Fixes: tag?

  Fixes: f32df58acc68 ("drm/bridge: Add the necessary bits to support bus format negotiation")

> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 8ae2f9b1db442..6a97b579c604c 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -1437,6 +1437,17 @@ int drm_atomic_bridge_chain_check(struct drm_bridge *bridge,
>  
>  	ret = drm_atomic_bridge_chain_select_bus_fmts(bridge, crtc_state,
>  						      conn_state);
> +	/*
> +	 * -ENOTSUPP is the internal "no bus format chain" code of the
> +	 * negotiation. It is not a valid userspace error code; an atomic
> +	 * check that cannot be satisfied fails with -EINVAL.
> +	 */
> +	if (ret == -ENOTSUPP) {
> +		drm_dbg_atomic(bridge->dev,
> +			       "[ENCODER:%d:%s] no bus format chain found\n",
> +			       bridge->encoder->base.id, bridge->encoder->name);
> +		return -EINVAL;
> +	}
>  	if (ret)
>  		return ret;

[Severity: Medium]
Does drm_test_bridge_rgb_yuv_no_path() also need to be updated to expect
-EINVAL?

drivers/gpu/drm/tests/drm_bridge_test.c:drm_test_bridge_rgb_yuv_no_path() {
    ...
	ret = drm_atomic_commit(state);
	if (ret == -EDEADLK) {
		drm_modeset_backoff(&ctx);
		goto retry_commit;
	}
	KUNIT_EXPECT_EQ(test, ret, -ENOTSUPP);
    ...
}

Because drm_atomic_commit() calls into drm_atomic_bridge_chain_check(), this
KUnit test now receives -EINVAL when no valid bridge bus format path exists
and fails the -ENOTSUPP assertion.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-drm-bridge-busfmt-einval-v1-1-c0494bd23264@gmail.com?part=1