Re: [PATCH v5 03/14] media: cadence: csi2rx: Add get_fmt and set_fmt pad ops

[Pratyush Yadav <p.yadav@ti.com>]

Hi Laurent,

On 30/12/21 06:32AM, Laurent Pinchart wrote:
> Hi Pratyush,
> 
> Thank you for the patch.
> 
> On Fri, Dec 24, 2021 at 12:46:04AM +0530, Pratyush Yadav wrote:
> > The format is needed to calculate the link speed for the external DPHY
> > configuration. It is not right to query the format from the source
> > subdev. Add get_fmt and set_fmt pad operations so that the format can be
> > configured and correct bpp be selected.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > 
> > ---
> > 
> > Changes in v5:
> > - Use YUV 1X16 formats instead of 2X8.
> > - New in v5.
> > 
> >  drivers/media/platform/cadence/cdns-csi2rx.c | 137 +++++++++++++++++++
> >  1 file changed, 137 insertions(+)
> > 
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> > index 2547903f2e8e..4a2a5a9d019b 100644
> > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > @@ -54,6 +54,11 @@ enum csi2rx_pads {
> >  	CSI2RX_PAD_MAX,
> >  };
> >  
> > +struct csi2rx_fmt {
> > +	u32				code;
> > +	u8				bpp;
> > +};
> > +
> >  struct csi2rx_priv {
> >  	struct device			*dev;
> >  	unsigned int			count;
> > @@ -79,12 +84,43 @@ struct csi2rx_priv {
> >  	struct v4l2_subdev		subdev;
> >  	struct v4l2_async_notifier	notifier;
> >  	struct media_pad		pads[CSI2RX_PAD_MAX];
> > +	struct v4l2_mbus_framefmt	fmt;
> >  
> >  	/* Remote source */
> >  	struct v4l2_subdev		*source_subdev;
> >  	int				source_pad;
> >  };
> >  
> > +static const struct csi2rx_fmt formats[] = {
> > +	{
> > +		.code	= MEDIA_BUS_FMT_YUYV8_1X16,
> > +		.bpp	= 16,
> > +	},
> > +	{
> > +		.code	= MEDIA_BUS_FMT_UYVY8_1X16,
> > +		.bpp	= 16,
> > +	},
> > +	{
> > +		.code	= MEDIA_BUS_FMT_YVYU8_1X16,
> > +		.bpp	= 16,
> > +	},
> > +	{
> > +		.code	= MEDIA_BUS_FMT_VYUY8_1X16,
> > +		.bpp	= 16,
> > +	},
> > +};
> 
> bpp isn't used. Unless you need it in a subsequent patch in the series,
> you can turn the formats array into a u32 array.

It is used in the next patch for calculating DPHY parameters.

> 
> > +
> > +static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(formats); i++)
> > +		if (formats[i].code == code)
> > +			return &formats[i];
> > +
> > +	return NULL;
> > +}
> > +
> >  static inline
> >  struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
> >  {
> > @@ -236,12 +272,109 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
> >  	return ret;
> >  }
> >  
> > +static struct v4l2_mbus_framefmt *
> > +csi2rx_get_pad_format(struct csi2rx_priv *csi2rx,
> > +		      struct v4l2_subdev_state *state,
> > +		      unsigned int pad, u32 which)
> > +{
> > +	switch (which) {
> > +	case V4L2_SUBDEV_FORMAT_TRY:
> > +		return v4l2_subdev_get_try_format(&csi2rx->subdev, state, pad);
> > +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> > +		return &csi2rx->fmt;
> > +	default:
> > +		return NULL;
> > +	}
> > +}
> > +
> > +static int csi2rx_get_fmt(struct v4l2_subdev *subdev,
> > +			  struct v4l2_subdev_state *state,
> > +			  struct v4l2_subdev_format *format)
> > +{
> > +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > +	struct v4l2_mbus_framefmt *framefmt;
> > +
> > +	mutex_lock(&csi2rx->lock);
> > +
> > +	framefmt = csi2rx_get_pad_format(csi2rx, state, format->pad,
> > +					 format->which);
> > +	mutex_unlock(&csi2rx->lock);
> > +
> > +	if (!framefmt)
> > +		return -EINVAL;
> 
> This can't happen, you can drop the check.

The function returns NULL when format->which is neither 
V4L2_SUBDEV_FORMAT_TRY nor V4L2_SUBDEV_FORMAT_ACTIVE. Does V4L2 core 
verify that which is always one of TRY or ACTIVE, and nothing else. Does 
it also guarantee that this would *never* change (like adding a new 
value for example)? If so, I am fine with dropping this check. Otherwise 
I would like to keep it.

> 
> > +
> > +	format->format = *framefmt;
> 
> This is the assignment that needs to be protected by the lock.
> csi2rx_get_pad_format() returns a pointer to the storage, it doesn't
> modify it.
> 
> 	framefmt = csi2rx_get_pad_format(csi2rx, state, format->pad,
> 					 format->which);
> 
> 	mutex_lock(&csi2rx->lock);
> 	format->format = *framefmt;
> 	mutex_unlock(&csi2rx->lock);

Ah, you're right. I feel very stupid now ;-)

> 
> Same comments for csi2rx_set_fmt().

The assignment is csi2rx_set_fmt() is done under the lock. But I should 
move the lock to after csi2rx_get_pad_format() is called so we only hold 
the lock for as little as possible:

	framefmt = csi2rx_get_pad_format(csi2rx, state, format->pad,
					 format->which);
	if (!framefmt) {
		mutex_unlock(&csi2rx->lock);
		return -EINVAL;
	}

	mutex_lock(&csi2rx->lock);
	*framefmt = format->format;
	mutex_unlock(&csi2rx->lock);


> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int csi2rx_set_fmt(struct v4l2_subdev *subdev,
> > +			  struct v4l2_subdev_state *state,
> > +			  struct v4l2_subdev_format *format)
> > +{
> > +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > +	struct v4l2_mbus_framefmt *framefmt;
> > +	const struct csi2rx_fmt *fmt;
> > +
> > +	/* No transcoding, source and sink formats must match. */
> > +	if (format->pad != CSI2RX_PAD_SINK)
> > +		return csi2rx_get_fmt(subdev, state, format);
> > +
> > +	fmt = csi2rx_get_fmt_by_code(format->format.code);
> > +	if (!fmt)
> > +		return -EOPNOTSUPP;
> 
> This should not return an error, but instead adjust the code:
> 
> 	if (!csi2rx_get_fmt_by_code(format->format.code))
> 		format->format.code = formats[0].code;

Ok. I figured it would be better to fail loudly if an unsupported format 
is requested. But if this behavior is preferred that is also fine.

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

Thanks.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.