RE: [PATCH v13 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem driver

[Vishal Sagar]

Hi Luca,

Thanks for reviewing!

> -----Original Message-----
> From: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx>
> Sent: Monday, May 25, 2020 6:44 PM
> To: Vishal Sagar <vsagar@xxxxxxxxxx>; Hyun Kwon <hyunk@xxxxxxxxxx>;
> laurent.pinchart@xxxxxxxxxxxxxxxx; mchehab@xxxxxxxxxx;
> robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; Michal Simek
> <michals@xxxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; hans.verkuil@xxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Dinesh Kumar
> <dineshk@xxxxxxxxxx>; Sandip Kothari <sandipk@xxxxxxxxxx>; Jacopo Mondi
> <jacopo@xxxxxxxxxx>
> Cc: Hyun Kwon <hyunk@xxxxxxxxxx>
> Subject: Re: [PATCH v13 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx
> Subsystem driver
> 
> Hi Vishal,
> 
> thanks. I have only a few minor nitpicking comments.
> 
> On 12/05/20 17:19, Vishal Sagar wrote:
> > The Xilinx MIPI CSI-2 Rx Subsystem soft IP is used to capture images
> > from MIPI CSI-2 camera sensors and output AXI4-Stream video data ready
> > for image processing. Please refer to PG232 for details.
> >
> > The CSI2 Rx controller filters out all packets except for the packets
> > with data type fixed in hardware. RAW8 packets are always allowed to
> > pass through.
> >
> > It is also used to setup and handle interrupts and enable the core. It
> > logs all the events in respective counters between streaming on and off.
> >
> > The driver supports only the video format bridge enabled configuration.
> > Some data types like YUV 422 10bpc, RAW16, RAW20 are supported when
> > the CSI v2.0 feature is enabled in design. When the VCX feature is
> > enabled, the maximum number of virtual channels becomes 16 from 4.
> >
> > Signed-off-by: Vishal Sagar <vishal.sagar@xxxxxxxxxx>
> > Reviewed-by: Hyun Kwon <hyun.kwon@xxxxxxxxxx>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> 
> [...]
> 
> > +static int xcsi2rxss_start_stream(struct xcsi2rxss_state *state) {
> > +	int ret = 0;
> > +
> > +	/* enable core */
> > +	xcsi2rxss_set(state, XCSI_CCR_OFFSET, XCSI_CCR_ENABLE);
> > +
> > +	ret = xcsi2rxss_soft_reset(state);
> > +	if (ret < 0) {
> 
> 'if (ret)' is enough, it's a classic nonzero-on-error return value.
> 

Agreed. I will fix it in next version.

> > +/**
> > + * xcsi2rxss_irq_handler - Interrupt handler for CSI-2
> > + * @irq: IRQ number
> > + * @data: Pointer to device state
> > + *
> > + * In the interrupt handler, a list of event counters are updated for
> > + * corresponding interrupts. This is useful to get status / debug.
> > + *
> > + * Return: IRQ_HANDLED after handling interrupts  */ static
> > +irqreturn_t xcsi2rxss_irq_handler(int irq, void *data) {
> > +	struct xcsi2rxss_state *state = (struct xcsi2rxss_state *)data;
> > +	struct device *dev = state->dev;
> > +	u32 status;
> > +
> > +	status = xcsi2rxss_read(state, XCSI_ISR_OFFSET) &
> XCSI_ISR_ALLINTR_MASK;
> > +	xcsi2rxss_write(state, XCSI_ISR_OFFSET, status);
> > +
> > +	/* Received a short packet */
> > +	if (status & XCSI_ISR_SPFIFONE) {
> > +		u32 count = 0;
> > +
> > +		/*
> > +		 * Drain generic short packet FIFO by reading max 31
> > +		 * (fifo depth) short packets from fifo or till fifo is empty.
> > +		 */
> > +		for (count = 0; count < XCSI_SPKT_FIFO_DEPTH; ++count) {
> > +			u32 spfifostat, spkt;
> > +
> > +			spkt = xcsi2rxss_read(state, XCSI_SPKTR_OFFSET);
> > +			dev_dbg(dev, "Short packet = 0x%08x\n", spkt);
> > +			spfifostat = xcsi2rxss_read(state, XCSI_ISR_OFFSET);
> > +			spfifostat &= XCSI_ISR_SPFIFONE;
> > +			if (!spfifostat)
> > +				break;
> > +			xcsi2rxss_write(state, XCSI_ISR_OFFSET, spfifostat);
> > +		}
> > +	}
> > +
> > +	/* Short packet FIFO overflow */
> > +	if (status & XCSI_ISR_SPFIFOF)
> > +		dev_dbg_ratelimited(dev, "Short packet FIFO overflowed\n");
> > +
> > +	/*
> > +	 * Stream line buffer full
> > +	 * This means there is a backpressure from downstream IP
> > +	 */
> > +	if (status & XCSI_ISR_SLBF) {
> > +		dev_alert_ratelimited(dev, "Stream Line Buffer Full!\n");
> > +
> > +		/* disable interrupts */
> > +		xcsi2rxss_clr(state, XCSI_IER_OFFSET, XCSI_IER_INTR_MASK);
> > +		xcsi2rxss_clr(state, XCSI_GIER_OFFSET, XCSI_GIER_GIE);
> > +
> > +		/* disable core */
> > +		xcsi2rxss_clr(state, XCSI_CCR_OFFSET, XCSI_CCR_ENABLE);
> > +		state->streaming = false;
> > +
> > +		/*
> > +		 * The IP needs to be hard reset before it can be used now.
> > +		 * This will be done in streamoff.
> > +		 */
> > +
> > +		/*
> > +		 * TODO: Notify the whole pipeline with v4l2_subdev_notify()
> to
> > +		 * inform userspace.
> > +		 */
> > +	}
> > +
> > +	/* Increment event counters */
> > +	if (status & XCSI_ISR_ALLINTR_MASK) {
> > +		unsigned int i;
> > +
> > +		for (i = 0; i < XCSI_NUM_EVENTS; i++) {
> > +			if (!(status & xcsi2rxss_events[i].mask))
> > +				continue;
> > +			state->events[i]++;
> > +			dev_dbg_ratelimited(dev, "%s: %u\n",
> > +					    xcsi2rxss_events[i].name,
> > +					    state->events[i]);
> > +		}
> > +
> > +		if (status & XCSI_ISR_VCXFE && state->en_vcx) {
> > +			u32 vcxstatus;
> > +
> > +			vcxstatus = xcsi2rxss_read(state, XCSI_VCXR_OFFSET);
> > +			vcxstatus &= XCSI_VCXR_VCERR;
> > +			for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++) {
> > +				if (!(vcxstatus & (1 << i)))
> 
> You can use BIT(i) instead of (1 << i).

Yep that is a good alternative.

> 
> > +/**
> > + * xcsi2rxss_set_format - This is used to set the pad format
> > + * @sd: Pointer to V4L2 Sub device structure
> > + * @cfg: Pointer to sub device pad information structure
> > + * @fmt: Pointer to pad level media bus format
> > + *
> > + * This function is used to set the pad format. Since the pad format
> > +is fixed
> > + * in hardware, it can't be modified on run time. So when a format
> > +set is
> > + * requested by application, all parameters except the format type is
> > +saved
> > + * for the pad and the original pad format is sent back to the application.
> > + *
> > + * Return: 0 on success
> > + */
> > +static int xcsi2rxss_set_format(struct v4l2_subdev *sd,
> > +				struct v4l2_subdev_pad_config *cfg,
> > +				struct v4l2_subdev_format *fmt)
> > +{
> > +	struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> > +	struct v4l2_mbus_framefmt *__format;
> > +	u32 dt;
> > +
> > +	/* only sink pad format can be updated */
> 
> This comment should be placed...
> 
> > +	mutex_lock(&xcsi2rxss->lock);
> > +
> > +	/*
> > +	 * Only the format->code parameter matters for CSI as the
> > +	 * CSI format cannot be changed at runtime.
> > +	 * Ensure that format to set is copied to over to CSI pad format
> > +	 */
> > +	__format = __xcsi2rxss_get_pad_format(xcsi2rxss, cfg,
> > +					      fmt->pad, fmt->which);
> > +
> 
> ...here.
> 

Ok will move the comment here.

> > +	if (fmt->pad == XVIP_PAD_SOURCE) {
> > +		fmt->format = *__format;
> > +		mutex_unlock(&xcsi2rxss->lock);
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * RAW8 is supported in all datatypes. So if requested media bus
> format
> > +	 * is of RAW8 type, then allow to be set. In case core is configured to
> > +	 * other RAW, YUV422 8/10 or RGB888, set appropriate media bus
> format.
> > +	 */
> > +	dt = xcsi2rxss_get_dt(fmt->format.code);
> > +	if (dt != xcsi2rxss->datatype && dt != XCSI_DT_RAW8) {
> > +		dev_dbg(xcsi2rxss->dev, "Unsupported media bus format");
> > +		/* set the default format for the data type */
> > +		fmt->format.code = xcsi2rxss_get_nth_mbus(xcsi2rxss-
> >datatype,
> > +							  0);
> > +	}
> > +
> > +	*__format = fmt->format;
> > +	mutex_unlock(&xcsi2rxss->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * xcsi2rxss_enum_mbus_code - Handle pixel format enumeration
> > + * @sd : pointer to v4l2 subdev structure
> > + * @cfg: V4L2 subdev pad configuration
> > + * @code : pointer to v4l2_subdev_mbus_code_enum structure
> 
> Remove space before colon here.
> 
> Looks good otherwise, and my comments are minor details so:
> Reviewed-by: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx>

Thatâs great. Thanks for reviewing this again.

> 
> I tried to runtime test this driver as well replacing the v10 driver that I'm using
> at the moment, but ran into many problems, apparently in the media entity
> navigation. The diff between v10 and v13 does not justify these problems, so
> I'm assuming v13 needs a more recent kernel than the 4.19 I'm currentl stuck
> on.
> 
> --
> Luca Ceresoli

Regards
Vishal Sagar