Re: [PATCH v2 10/13] media: rcar-csi2: Move format to subdev state

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jacopo,

Thank you for the patch.

On Sun, Oct 17, 2021 at 08:24:46PM +0200, Jacopo Mondi wrote:
> Move format handling to the v4l2_subdev state and store it per
> (pad, stream) combination.
> 
> Now that the image format is stored in the subdev state, it can be
> accessed through v4l2_subdev_get_fmt() instead of open-coding it.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 50 ++++++++++++---------
>  1 file changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 451a6da26e03..b60845b1e563 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -722,34 +722,42 @@ static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
>  				struct v4l2_subdev_state *sd_state,
>  				struct v4l2_subdev_format *format)
>  {
> -	struct rcar_csi2 *priv = sd_to_csi2(sd);
> -	struct v4l2_mbus_framefmt *framefmt;
> +	struct v4l2_mbus_framefmt *fmt;
> +	struct v4l2_subdev_state *state;
> +	int ret = 0;
>  
>  	if (!rcsi2_code_to_fmt(format->format.code))
>  		format->format.code = rcar_csi2_formats[0].code;
>  
> -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> -		priv->mf = format->format;
> -	} else {
> -		framefmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> -		*framefmt = format->format;
> -	}
> +	/*
> +	 * Format is propagated from sink streams to source streams, so
> +	 * disallow setting format on the source pads.
> +	 */
> +	if (format->pad > RCAR_CSI2_SINK)
> +		return -EINVAL;

You should return v4l2_subdev_get_fmt() instead.

>  
> -	return 0;
> -}
> +	state = v4l2_subdev_validate_and_lock_state(sd, sd_state);

With v10 of the streams series I think you can use
v4l2_subdev_lock_state().

> +	fmt = v4l2_state_get_stream_format(state, format->pad,
> +					   format->stream);
> +	if (!fmt) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	*fmt = format->format;

I would get the pointers to both formats before updating any of them, to
avoid a partial update in case of error:

	sink_fmt = v4l2_state_get_stream_format(state, format->pad,
						format->stream);
	source_fmt = v4l2_state_get_opposite_stream_format(state, format->pad,
							   format->stream);
	if (!sink_fmt || !source_fmt) {
		ret = -EINVAL;
		goto out;
	}

	*sink_fmt = format->format;
	*source_fmt = format->format;

Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>

>  
> -static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> -				struct v4l2_subdev_state *sd_state,
> -				struct v4l2_subdev_format *format)
> -{
> -	struct rcar_csi2 *priv = sd_to_csi2(sd);
> +	/* Propagate format to the other end of the route. */
> +	fmt = v4l2_state_get_opposite_stream_format(state, format->pad,
> +						    format->stream);
> +	if (!fmt) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	*fmt = format->format;
>  
> -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -		format->format = priv->mf;
> -	else
> -		format->format = *v4l2_subdev_get_try_format(sd, sd_state, 0);
> +out:
> +	v4l2_subdev_unlock_state(state);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int _rcsi2_set_routing(struct v4l2_subdev *sd,
> @@ -829,7 +837,7 @@ static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
>  static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
>  	.init_cfg = rcsi2_init_cfg,
>  	.set_fmt = rcsi2_set_pad_format,
> -	.get_fmt = rcsi2_get_pad_format,
> +	.get_fmt = v4l2_subdev_get_fmt,
>  	.set_routing = rcsi2_set_routing,
>  };
>  

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux