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]

 



On Sun, Jan 23, 2022 at 04:20:01PM +0200, Laurent Pinchart wrote:
> On Sun, Jan 23, 2022 at 04:19:16PM +0200, Laurent Pinchart wrote:
> > 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>
> 
> Actually, the rest of the driver still uses priv->mf, so this patch will
> cause a breakage. You can squash it with 11/13.

Or, looking at 11/13, it may be best to replace all usages of priv->mf
with the format from the state in this patch, and keep 11/13 separate.

> > >  
> > > -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