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
