Re: [PATCH v8 07/16] media: cadence: csi2rx: Add get_fmt and set_fmt pad ops

[Jai Luthra]

Hi Tomi,

Thanks for the review.

On Aug 02, 2023 at 11:48:52 +0300, Tomi Valkeinen wrote:
> On 31/07/2023 11:29, Jai Luthra wrote:
> > From: Pratyush Yadav <p.yadav@xxxxxx>
> > 
> > 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.
> > 
> > Initialize and use the v4l2 subdev active state to keep track of the
> > active formats. Also propagate the new format from the sink pad to all
> > the source pads.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@xxxxxx>
> > Co-authored-by: Jai Luthra <j-luthra@xxxxxx>
> > Signed-off-by: Jai Luthra <j-luthra@xxxxxx>
> > ---
> > v7->v8:
> > - Use active subdev state to use v4l2_subdev_get_fmt
> > - Propagate formats from sink to source pads
> > - Drop Laurent's R-by because of the above changes
> > 
> >   drivers/media/platform/cadence/cdns-csi2rx.c | 107 ++++++++++++++++++++++++++-
> >   1 file changed, 106 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> > index 83d1fadd592b..4f9654366485 100644
> > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > @@ -61,6 +61,11 @@ enum csi2rx_pads {
> >   	CSI2RX_PAD_MAX,
> >   };
> > +struct csi2rx_fmt {
> > +	u32				code;
> > +	u8				bpp;
> > +};
> > +
> >   struct csi2rx_priv {
> >   	struct device			*dev;
> >   	unsigned int			count;
> > @@ -95,6 +100,36 @@ struct csi2rx_priv {
> >   	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,
> > +	},
> > +};
> 
> I think you could just squash the "media: cadence: csi2rx: Support RAW8 and
> RAW10 formats" into this one. Also, the lines could well be one-liners:
> 
> 	{ .code	= MEDIA_BUS_FMT_YUYV8_1X16, .bpp = 16, },
> 

Will fix.

> 
> > +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)
> >   {
> > @@ -303,12 +338,75 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
> >   	return ret;
> >   }
> > +static int csi2rx_set_fmt(struct v4l2_subdev *subdev,
> > +			  struct v4l2_subdev_state *state,
> > +			  struct v4l2_subdev_format *format)
> > +{
> > +	struct v4l2_mbus_framefmt *fmt;
> > +	unsigned int i;
> > +
> > +	/* No transcoding, source and sink formats must match. */
> > +	if (format->pad != CSI2RX_PAD_SINK)
> > +		return v4l2_subdev_get_fmt(subdev, state, format);
> > +
> > +	if (!csi2rx_get_fmt_by_code(format->format.code))
> > +		format->format.code = formats[0].code;
> > +
> > +	format->format.field = V4L2_FIELD_NONE;
> > +
> > +	/* Set sink format */
> > +	fmt = v4l2_subdev_get_pad_format(subdev, state, format->pad);
> > +	if (!fmt)
> > +		return -EINVAL;
> > +
> > +	*fmt = format->format;
> > +
> > +	/* Propagate to source formats */
> > +	for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++) {
> > +		fmt = v4l2_subdev_get_pad_format(subdev, state, i);
> > +		if (!fmt)
> > +			return -EINVAL;
> > +		*fmt = format->format;
> > +	}
> 
> Not really part of this patch, but why does csi2rx create more than one
> source pad? It doesn't support streams, so how are those pads used?
> 

These pads correspond to the 4 output pixel data "streams" the Cadence 
IP provides. We can route the incoming pixel data over MIPI CSI to 
different peripherals in the SoC, while doing some optional filtering on 
VC and DT using the STREAM_DATA_CFG register.

Different vendors may integrate these streams in their own way. For 
example on AM62A this is how they are mapped in the hardware [1]:

pad1 -> shim   -> upto 32x dma chans
pad2 ->
pad3 -> vp0    -> ISP
pad4 -> vp1

For shim the idea is to pass-through all VCs and DTs, as shim can filter 
and send different v4l2-streams to different dma chans.

For the ISP (when supported) we will have to filter for a specific VC or 
DT here, in case multiple streams are coming over CSI.

[1] TRM, Section 12.6.4 https://www.ti.com/lit/ug/spruj16/spruj16.pdf 

> > +	return 0;
> > +}
> > +
> > +static int csi2rx_init_cfg(struct v4l2_subdev *subdev,
> > +			   struct v4l2_subdev_state *state)
> > +{
> > +	struct v4l2_subdev_format format = {
> > +		.which = state ? V4L2_SUBDEV_FORMAT_TRY
> > +			: V4L2_SUBDEV_FORMAT_ACTIVE,
> 
> I don't think this is correct. If you enable the active state, you'll always
> get a state here, and thus this function doesn't really know if it is TRY or
> ACTIVE state (nor should it care).

My bad I forgot to clean this up. Will fix in next revision.

> 
> > +		.pad = CSI2RX_PAD_SINK,
> > +		.format = {
> > +			.width = 640,
> > +			.height = 480,
> > +			.code = MEDIA_BUS_FMT_UYVY8_1X16,
> > +			.field = V4L2_FIELD_NONE,
> > +			.colorspace = V4L2_COLORSPACE_SRGB,
> > +			.ycbcr_enc = V4L2_YCBCR_ENC_601,
> > +			.quantization = V4L2_QUANTIZATION_LIM_RANGE,
> > +			.xfer_func = V4L2_XFER_FUNC_SRGB,
> > +		},
> > +	};
> > +
> > +	return csi2rx_set_fmt(subdev, state, &format);
> > +}
> > +
> > +static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = {
> > +	.get_fmt	= v4l2_subdev_get_fmt,
> > +	.set_fmt	= csi2rx_set_fmt,
> > +	.init_cfg	= csi2rx_init_cfg,
> > +};
> > +
> >   static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
> >   	.s_stream	= csi2rx_s_stream,
> >   };
> >   static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
> >   	.video		= &csi2rx_video_ops,
> > +	.pad		= &csi2rx_pad_ops,
> >   };
> >   static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
> > @@ -532,9 +630,13 @@ static int csi2rx_probe(struct platform_device *pdev)
> >   	if (ret)
> >   		goto err_cleanup;
> > +	ret = v4l2_subdev_init_finalize(&csi2rx->subdev);
> > +	if (ret)
> > +		goto err_cleanup;
> > +
> >   	ret = v4l2_async_register_subdev(&csi2rx->subdev);
> >   	if (ret < 0)
> > -		goto err_cleanup;
> > +		goto err_free_state;
> >   	dev_info(&pdev->dev,
> >   		 "Probed CSI2RX with %u/%u lanes, %u streams, %s D-PHY\n",
> > @@ -544,6 +646,8 @@ static int csi2rx_probe(struct platform_device *pdev)
> >   	return 0;
> > +err_free_state:
> > +	v4l2_subdev_cleanup(&csi2rx->subdev);
> >   err_cleanup:
> >   	v4l2_async_nf_unregister(&csi2rx->notifier);
> >   	v4l2_async_nf_cleanup(&csi2rx->notifier);
> > @@ -560,6 +664,7 @@ static void csi2rx_remove(struct platform_device *pdev)
> >   	v4l2_async_nf_unregister(&csi2rx->notifier);
> >   	v4l2_async_nf_cleanup(&csi2rx->notifier);
> >   	v4l2_async_unregister_subdev(&csi2rx->subdev);
> > +	v4l2_subdev_cleanup(&csi2rx->subdev);
> >   	media_entity_cleanup(&csi2rx->subdev.entity);
> >   	kfree(csi2rx);
> >   }
> > 
> 

-- 
Thanks,
Jai

GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145

Attachment: signature.asc
Description: PGP signature