Re: [PATCH] media: sun6i-mipi-csi2: Propagate format to source pad in TRY mode

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello Arash,

Thank you for the patch.

On Mon, Jan 26, 2026 at 01:51:49PM +0330, Arash Golgol wrote:
> sun6i-mipi-csi2 does not support any format conversion. So the format
> on sink and source pad must always match. This limitation is handled for
> ACTIVE state via mbus_format member of bridge private structure.

It's not even handled correctly though, the sun6i_mipi_csi2_set_fmt()
function will accept setting the ACTIVE format on the source pad, when
it shouldn't. You also fix this in your patch. This should be explained
in the commit message.

> To enforce this limitation in TRY state, user space should only be able
> to set format on sink pad. The sink format must be propagated to source
> pad to ensure consistent behavior.
> 
> This also aligns the driver with userspace relying on media controller
> based format negotiation.
> 
> Signed-off-by: Arash Golgol <arash.golgol@gmail.com>
> ---
>  .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c   | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c
> index b06cb73015cd..6ee0f6685663 100644
> --- a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c
> +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c
> @@ -369,15 +369,26 @@ static int sun6i_mipi_csi2_set_fmt(struct v4l2_subdev *subdev,
>  	struct v4l2_mbus_framefmt *mbus_format = &format->format;
>  	struct mutex *lock = &csi2_dev->bridge.lock;
>  
> +	/* The format on the source pad always matches the sink pad. */
> +	if (format->pad != SUN6I_MIPI_CSI2_PAD_SINK)
> +		return v4l2_subdev_get_fmt(subdev, state, format);
> +
>  	mutex_lock(lock);
>  
>  	sun6i_mipi_csi2_mbus_format_prepare(mbus_format);
>  
> -	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> -		*v4l2_subdev_state_get_format(state, format->pad) =
> -			*mbus_format;
> -	else
> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> +		struct v4l2_mbus_framefmt *fmt;
> +
> +		fmt = v4l2_subdev_state_get_format(state, format->pad);
> +		*fmt = *mbus_format;
> +
> +		/* Propagate the format to the source pad. */
> +		fmt = v4l2_subdev_state_get_format(state, SUN6I_MIPI_CSI2_PAD_SOURCE);
> +		*fmt = *mbus_format;
> +	} else {
>  		csi2_dev->bridge.mbus_format = *mbus_format;
> +	}

If you have a bit more time, could I ask you to take it one step further
and convert the driver to framework-managed V4L2 subdev active ? This
will simplify the implementation by dropping bridge.mbus_format.

You can find an example of the conversion in commit a2514b9a634a
("media: i2c: imx290: Use V4L2 subdev active state"). In a nutshell, you
need to

- Call v4l2_subdev_init_finalize() at probe time and
  v4l2_subdev_cleanup() at remove time
- Drop the mutex (locking is now handled by the framework)
- Use v4l2_subdev_get_fmt() as the .get_fmt() handler
- Drop bridge.mbus_format and always use v4l2_subdev_get_pad_format() on
  the state passed to the subdev functions (which will be the ACTIVE or
  TRY state depending on the .which argument passed by userspace)
- In .s_stream(), access the state with
  v4l2_subdev_lock_and_get_active_state() as it's the only subdev
  operation that doesn't get the state as a parameter

You can ignore the control handler changes in commit a2514b9a634a, and
you can leave subdev.state_lock NULL (the framework will then use an
internal lock).

>  
>  	mutex_unlock(lock);
>  

-- 
Regards,

Laurent Pinchart