Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Tomasz Figa <tfiga@chromium.org>
Cc: devicetree@vger.kernel.org, eddie.cai.linux@gmail.com,
	kernel@collabora.com,
	Dafna Hirschfeld <dafna.hirschfeld@collabora.com>,
	heiko@sntech.de, jacob2.chen@rock-chips.com,
	jeffy.chen@rock-chips.com, zyc@rock-chips.com,
	linux-kernel@vger.kernel.org,
	Allon Huang <allon.huang@rock-chips.com>,
	linux-rockchip@lists.infradead.org,
	Helen Koike <helen.koike@collabora.com>,
	Jacob Chen <cc@rock-chips.com>,
	hans.verkuil@cisco.com, sakari.ailus@linux.intel.com,
	zhengsq@rock-chips.com, mchehab@kernel.org,
	ezequiel@collabora.com, linux-arm-kernel@lists.infradead.org,
	linux-media@vger.kernel.org
Subject: Re: [PATCH v8 05/14] media: rkisp1: add Rockchip ISP1 subdev driver
Date: Wed, 22 Jul 2020 19:30:45 +0300
Message-ID: <20200722163045.GN29813@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20200722152459.GC1828171@chromium.org>

Hi Dafna and Tomasz,

On Wed, Jul 22, 2020 at 03:24:59PM +0000, Tomasz Figa wrote:
> On Sat, Jul 11, 2020 at 01:04:31PM +0200, Dafna Hirschfeld wrote:
> > On 16.08.19 02:13, Laurent Pinchart wrote:
> > > On Tue, Jul 30, 2019 at 03:42:47PM -0300, Helen Koike wrote:
>
> [snip]
>
> > > > +static void rkisp1_isp_queue_event_sof(struct rkisp1_isp_subdev *isp)
> > > > +{
> > > > +	struct v4l2_event event = {
> > > > +		.type = V4L2_EVENT_FRAME_SYNC,
> > > > +		.u.frame_sync.frame_sequence =
> > > > +			atomic_inc_return(&isp->frm_sync_seq) - 1,
> > > 
> > > I would move the increment to the caller, hiding it in this function is
> > > error-prone (and if you look at the caller I'm pointing out one possible
> > > error :-)).
> > > 
> > > In general usage of frm_sync_seq through the driver seems to be very
> > > race-prone. It's read in various IRQ handling functions, all coming from
> > > the same IRQ, so that part is fine (and wouldn't require an atomic
> > > variable), but when read from the buffer queue handlers I really get a
> > > red light flashing in my head. I'll try to investigate more when
> > > reviewing the next patches.
> > 
> > I see that the only place were 'frame_sequence' is read outside of the irq
> > handlers is in the capture in 'rkisp1_vb2_buf_queue':
> > 
> > 	/*
> >          * If there's no next buffer assigned, queue this buffer directly
> >          * as the next buffer, and update the memory interface.
> >          */
> >         if (cap->is_streaming && !cap->buf.next &&
> >             atomic_read(&cap->rkisp1->isp.frame_sequence) == -1) {
> >                 cap->buf.next = ispbuf;
> >                 rkisp1_set_next_buf(cap);
> >         } else {
> >                 list_add_tail(&ispbuf->queue, &cap->buf.queue);
> >         }
> >
> > This "if" condition seems very specific, a case where we already stream but v-start was not yet received.
> > I think it is possible to remove the test 'atomic_read(&cap->rkisp1->isp.frame_sequence) == -1'
> > from the above condition so that the next buffer is updated in case it is null not just before the first
> > v-start signal.
> 
> We don't have this special case in the Chrome OS code.
> 
> I suppose it would make it possible to resume the capture 1 frame
> earlier after a queue underrun, as otherwise the new buffer would be
> only programmed after the next frame start interrupt and used for the
> next-next frame.  However, it's racy, because programming of the buffer
> addresses is not atomic and could end up with the hardware using few
> plane addresses from the new buffer and few from the dummy buffer.
> 
> Given that and also the fact that a queue underrun is a very special
> case, where the system was already having problems catching up, I'd just
> remove this special case.
> 
> [snip]
>
> > > > +void rkisp1_isp_isr(unsigned int isp_mis, struct rkisp1_device *dev)
> > > > +{
> > > > +	void __iomem *base = dev->base_addr;
> > > > +	unsigned int isp_mis_tmp = 0;
> > > 
> > > _tmp are never good names :-S
> > > 
> > > > +	unsigned int isp_err = 0;
> > > 
> > > Neither of these variable need to be initialised to 0.
> > > 
> > > > +
> > > > +	/* start edge of v_sync */
> > > > +	if (isp_mis & CIF_ISP_V_START) {
> > > > +		rkisp1_isp_queue_event_sof(&dev->isp_sdev);
> > > 
> > > This will increment the frame sequence number. What if the interrupt is
> > > slightly delayed and the next frame starts before we get a change to
> > > copy the sequence number to the buffers (before they will complete
> > > below) ?
> > 
> > Do you mean that we get two sequental v-start signals and then the next
> > frame-end signal in MI_MIS belongs to the first v-start signal of the two?
> > How can this be solved? I wonder if any v-start signal has a later signal
> > that correspond to the same frame so that we can follow it?
> > 
> > Maybe we should have one counter that is incremented on v-start signal,
> > and another counter that is incremented uppon some other signal?
> 
> We're talking about a hard IRQ. I can't imagine the interrupt handler
> being delayed for a time close to a full frame interval (~16ms for 60
> fps) to trigger such scenario.

I've been burnt too many times by making such statements and then seeing
a wifi driver disablign interrupts for 40ms... :-S We can only perform
as well as the system and the hardware allow us to, I understand we
can't solve all issues related to long interrupt delays as that would
require more hardware support. I'm not sure what an appropriate best
effort level is though.

> > > > +
> > > > +		writel(CIF_ISP_V_START, base + CIF_ISP_ICR);
> > > 
> > > Do you need to clear all interrupt bits individually, can't you write
> > > isp_mis to CIF_ISP_ICR at the beginning of the function to clear them
> > > all in one go ?
> > > 
> > > > +		isp_mis_tmp = readl(base + CIF_ISP_MIS);
> > > > +		if (isp_mis_tmp & CIF_ISP_V_START)
> > > > +			v4l2_err(&dev->v4l2_dev, "isp icr v_statr err: 0x%x\n",
> > > > +				 isp_mis_tmp);
> > > 
> > > This require some explanation. It looks like a naive way to protect
> > > against something, but I think it could trigger under normal
> > > circumstances if IRQ handling is delayed, and wouldn't do much anyway.
> > > Same for the similar constructs below.
> > > 
> > > > +	}
> > > > +
> > > > +	if ((isp_mis & CIF_ISP_PIC_SIZE_ERROR)) {
> > > > +		/* Clear pic_size_error */
> > > > +		writel(CIF_ISP_PIC_SIZE_ERROR, base + CIF_ISP_ICR);
> > > > +		isp_err = readl(base + CIF_ISP_ERR);
> > > > +		v4l2_err(&dev->v4l2_dev,
> > > > +			 "CIF_ISP_PIC_SIZE_ERROR (0x%08x)", isp_err);
> > > 
> > > What does this mean ?
> > > 
> > > > +		writel(isp_err, base + CIF_ISP_ERR_CLR);
> > > > +	} else if ((isp_mis & CIF_ISP_DATA_LOSS)) {
> > > 
> > > Are CIF_ISP_PIC_SIZE_ERROR and CIF_ISP_DATA_LOSS mutually exclusive ?
> > > 
> > > > +		/* Clear data_loss */
> > > > +		writel(CIF_ISP_DATA_LOSS, base + CIF_ISP_ICR);
> > > > +		v4l2_err(&dev->v4l2_dev, "CIF_ISP_DATA_LOSS\n");
> > > > +		writel(CIF_ISP_DATA_LOSS, base + CIF_ISP_ICR);
> > > > +	}
> > > > +
> > > > +	/* sampled input frame is complete */
> > > > +	if (isp_mis & CIF_ISP_FRAME_IN) {
> > > > +		writel(CIF_ISP_FRAME_IN, base + CIF_ISP_ICR);
> > > > +		isp_mis_tmp = readl(base + CIF_ISP_MIS);
> > > > +		if (isp_mis_tmp & CIF_ISP_FRAME_IN)
> > > > +			v4l2_err(&dev->v4l2_dev, "isp icr frame_in err: 0x%x\n",
> > > > +				 isp_mis_tmp);
> > > > +	}
> > > > +
> > > > +	/* frame was completely put out */
> > > 
> > > "put out" ? :-) What's the difference between ISP_FRAME_IN and ISP_FRAME
> > > ? The two comments could do with a bit of brush up, and I think the
> > > ISP_FRAME_IN interrupt could be disabled as it doesn't perform any
> > > action.
> > 
> > Those two oneline comments are just copy-paste from the datasheet.
> > 
> > ""
> > 5 MIS_FRAME_IN sampled input frame is complete
> > 1 MIS_FRAME frame was completely put out
> > ""
> > 
> > Unfrotunately, the datasheet does not add any further explanation about those signals.
> 
> My loose recollection is that the former is signaled when then frame
> is fully input to the ISP and the latter when the ISP completes
> outputting the frame to the next block in the pipeline, but someone
> would need to verify this, for example by printing timestamps for all
> the various interrupts.
> 
> > > 
> > > > +	if (isp_mis & CIF_ISP_FRAME) {
> > > > +		u32 isp_ris = 0;
> > > 
> > > No need to initialise this to 0.
> > > 
> > > > +		/* Clear Frame In (ISP) */
> > > > +		writel(CIF_ISP_FRAME, base + CIF_ISP_ICR);
> > > > +		isp_mis_tmp = readl(base + CIF_ISP_MIS);
> > > > +		if (isp_mis_tmp & CIF_ISP_FRAME)
> > > > +			v4l2_err(&dev->v4l2_dev,
> > > > +				 "isp icr frame end err: 0x%x\n", isp_mis_tmp);
> > > > +
> > > > +		isp_ris = readl(base + CIF_ISP_RIS);
> > > > +		if (isp_ris & (CIF_ISP_AWB_DONE | CIF_ISP_AFM_FIN |
> > > > +			       CIF_ISP_EXP_END | CIF_ISP_HIST_MEASURE_RDY))
> > > > +			rkisp1_stats_isr(&dev->stats_vdev, isp_ris);
> > > 
> > > Is there a guarantee that the statistics will be fully written out
> > > before the video frame itself ? And doesn't this test if any of the
> > > statistics is complete, not all of them ? I think the logic is wrong, it
> > 
> > The datasheet does not add any explanation of what is expected to come first.
> > Should we wait until all statistics measurements are done? In the struct
> > sent to userspace there is a bitmaks for which of the statistics are read.
> > I think that if only part of the statistics are ready, we can already send the once
> > that are ready to userspace.
> 
> If we look further into the code, rkisp1_stats_isr() checks the
> interrupt status mask passed to it and reads out only the parameters
> with indicated completion. The statistics metadata buffer format
> includes a bit mask which tells the userspace which measurements are
> available.
> 
> However, I think I've spotted a bug there. At the beginning of
> rkisp1_stats_isr(), all the 4 interrupt status bits are cleared,
> regardless of the mask used later to decide which readouts need to be
> done. This could mean that with an unfortunate timing, some measurements
> would be lost. So at least the code should be fixed to only clear the
> interrupts bits really handled.
> 
> As for whether to send separate buffers for each measurement, I guess
> it's not a bad thing to let the userspace access the ones available
> earlier. Now I only don't recall why we decided to put all the
> measurements into one metadata structure, rather than splitting the 4
> into their own structures and buffer queues...
> 
> > > seems it should be moved out of the CIF_ISP_FRAME test, to a test of its
> > > own. It's hard to tell for sure without extra information though (for
> > > instance why are the stats-related bits read from CIF_ISP_RIS, when
> > > they seem to be documented as valid in CIF_ISP_ISR), but this should be
> > > validated, and most probably fixed. Care should be taken to keep
> > > synchronisation of sequence number between the different queues.
> > 
> > I see that the capture buffers are done before incrementing the frame_sequence with
> > the following explanation:
> > 
> > 	/*
> >          * Call rkisp1_capture_isr() first to handle the frame that
> >          * potentially completed using the current frame_sequence number before
> >          * it is potentially incremented by rkisp1_isp_isr() in the vertical
> >          * sync.
> >          */
> > 
> > I think reading the stats/params should also be done before calling rkisp1_capture_isr
> > for the same reason. (so to match the correct frame_sequence)
> 
> My recollection of the sequence of interrupts in this hardware is like
> this:
> 
> CIF_ISP_V_START (frame 0)
>   CIF_ISP_FRAME_IN (frame 0)
>     CIF_ISP_FRAME (frame 0)
>       CIF_ISP_AWB_DONE
>       CIF_ISP_AFM_FIN
>       CIF_ISP_EXP_END
>       CIF_ISP_HIST_MEASURE_RDY
>       CIF_MI_FRAME*
>       CIF_ISP_V_START (frame 1)
>         CIF_ISP_FRAME_IN (frame 1)
>           CIF_ISP_FRAME (frame 1)
>             ...
> 
> where the interrupts at the same indentation level can happen
> independently of each other. Again, someone would have to verify this.

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply index

Thread overview: 76+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-30 18:42 [PATCH v8 00/14] Rockchip ISP1 Driver Helen Koike
2019-07-30 18:42 ` [PATCH v8 01/14] media: videodev2.h, v4l2-ioctl: add rkisp1 meta buffer format Helen Koike
2019-08-15 13:30   ` Laurent Pinchart
2019-07-30 18:42 ` [PATCH v8 02/14] media: doc: add document for " Helen Koike
2019-08-07 13:09   ` Sakari Ailus
2019-08-15 13:51   ` Laurent Pinchart
2019-07-30 18:42 ` [PATCH v8 03/14] media: rkisp1: Add user space ABI definitions Helen Koike
2019-08-15 18:46   ` Laurent Pinchart
2020-07-10 12:59     ` Dafna Hirschfeld
2020-07-10 13:36       ` Laurent Pinchart
2020-07-10 14:30         ` Dafna Hirschfeld
2019-07-30 18:42 ` [PATCH v8 04/14] media: rkisp1: add Rockchip MIPI Synopsys DPHY driver Helen Koike
2019-08-07 13:05   ` Sakari Ailus
2019-08-07 13:37     ` Helen Koike
2019-08-15 17:54       ` Laurent Pinchart
2019-08-15 18:26         ` Heiko Stübner
2019-08-21 21:46         ` Helen Koike
2019-08-22  2:32           ` Laurent Pinchart
2019-07-30 18:42 ` [PATCH v8 05/14] media: rkisp1: add Rockchip ISP1 subdev driver Helen Koike
2019-08-06 18:51   ` Helen Koike
2019-08-07 10:39     ` Hans Verkuil
2019-08-15 19:35       ` Laurent Pinchart
2020-03-25  6:34         ` Dafna Hirschfeld
2020-03-25  7:11           ` Laurent Pinchart
2020-03-25  8:51             ` Dafna Hirschfeld
2020-03-25  9:11               ` Laurent Pinchart
2019-08-08  9:14   ` Sakari Ailus
2019-08-15  0:58     ` Helen Koike
2019-08-15  8:24       ` Sakari Ailus
2019-08-15 10:29         ` Tomasz Figa
2019-08-15 10:45           ` Sakari Ailus
2019-08-15 13:17         ` Sakari Ailus
2020-01-31 19:38           ` Dafna Hirschfeld
2020-02-12 21:13             ` Sakari Ailus
2020-02-13 12:50               ` Dafna Hirschfeld
2019-08-16  0:13   ` Laurent Pinchart
2020-07-11 11:04     ` Dafna Hirschfeld
2020-07-17  7:46       ` Dafna Hirschfeld
2020-07-22 16:01         ` Tomasz Figa
2020-07-22 15:24       ` Tomasz Figa
2020-07-22 16:30         ` Laurent Pinchart [this message]
2020-07-22 17:12           ` Tomasz Figa
2020-07-22 17:50             ` Laurent Pinchart
2020-08-05 21:10         ` Dafna Hirschfeld
2020-08-06  9:21           ` Dafna Hirschfeld
2020-08-06 12:22             ` Tomasz Figa
2020-08-07 16:08               ` Dafna Hirschfeld
2020-08-13  6:17                 ` Dafna Hirschfeld
2020-08-06 12:08           ` Tomasz Figa
2020-08-07 16:02             ` Dafna Hirschfeld
2019-07-30 18:42 ` [PATCH v8 06/14] media: rkisp1: add ISP1 statistics driver Helen Koike
2019-08-08  9:37   ` Sakari Ailus
2019-07-30 18:42 ` [PATCH v8 07/14] media: rkisp1: add ISP1 params driver Helen Koike
2019-07-30 18:42 ` [PATCH v8 09/14] media: rkisp1: add rockchip isp1 core driver Helen Koike
2019-08-07 15:27   ` Sakari Ailus
2019-08-08 21:59     ` Helen Koike
2019-08-09 12:05       ` Sakari Ailus
2019-08-07 15:36   ` Sakari Ailus
2019-07-30 18:42 ` [PATCH v8 10/14] dt-bindings: Document the Rockchip ISP1 bindings Helen Koike
2019-08-16  0:21   ` Laurent Pinchart
2019-07-30 18:42 ` [PATCH v8 11/14] dt-bindings: Document the Rockchip MIPI RX D-PHY bindings Helen Koike
2019-08-15 18:14   ` Laurent Pinchart
2019-07-30 18:42 ` [PATCH v8 12/14] arm64: dts: rockchip: add isp0 node for rk3399 Helen Koike
2019-07-30 18:42 ` [PATCH v8 13/14] arm64: dts: rockchip: add rx0 mipi-phy " Helen Koike
2019-07-30 18:42 ` [PATCH v8 14/14] MAINTAINERS: add entry for Rockchip ISP1 driver Helen Koike
2019-08-15 13:56   ` Laurent Pinchart
2019-07-30 20:15 ` [PATCH v8 00/14] Rockchip ISP1 Driver Hans Verkuil
2019-07-30 20:50   ` Helen Koike
2019-07-31  0:08     ` Helen Koike
2019-07-31  4:29       ` Hans Verkuil
2019-07-31  4:33         ` Hans Verkuil
2019-07-31  4:55           ` Hans Verkuil
2019-07-31 14:42             ` Helen Koike
2019-08-07 15:37 ` Sakari Ailus
2019-08-07 17:57   ` Helen Koike
2019-08-09 18:45 ` Manivannan Sadhasivam

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200722163045.GN29813@pendragon.ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=allon.huang@rock-chips.com \
    --cc=cc@rock-chips.com \
    --cc=dafna.hirschfeld@collabora.com \
    --cc=devicetree@vger.kernel.org \
    --cc=eddie.cai.linux@gmail.com \
    --cc=ezequiel@collabora.com \
    --cc=hans.verkuil@cisco.com \
    --cc=heiko@sntech.de \
    --cc=helen.koike@collabora.com \
    --cc=jacob2.chen@rock-chips.com \
    --cc=jeffy.chen@rock-chips.com \
    --cc=kernel@collabora.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=mchehab@kernel.org \
    --cc=sakari.ailus@linux.intel.com \
    --cc=tfiga@chromium.org \
    --cc=zhengsq@rock-chips.com \
    --cc=zyc@rock-chips.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git