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

From: Luca Ceresoli <luca@lucaceresoli.net>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Vishal Sagar <vishal.sagar@xilinx.com>,
	Hyun Kwon <hyunk@xilinx.com>,
	mchehab@kernel.org, robh+dt@kernel.org, mark.rutland@arm.com,
	Michal Simek <michals@xilinx.com>,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	hans.verkuil@cisco.com, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Dinesh Kumar <dineshk@xilinx.com>,
	Sandip Kothari <sandipk@xilinx.com>,
	Jacopo Mondi <jacopo@jmondi.org>,
	Hyun Kwon <hyun.kwon@xilinx.com>
Subject: Re: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem driver
Date: Tue, 21 Apr 2020 11:53:50 +0200	[thread overview]
Message-ID: <66ad4341-8066-6b09-5473-12825fe26828@lucaceresoli.net> (raw)
In-Reply-To: <20200421083807.GB5983@pendragon.ideasonboard.com>

Hi Laurent,

On 21/04/20 10:38, Laurent Pinchart wrote:
> Hi Luca,
> 
> On Tue, Apr 21, 2020 at 09:45:56AM +0200, Luca Ceresoli wrote:
>> On 20/04/20 21:57, Laurent Pinchart wrote:
>>> On Mon, Apr 20, 2020 at 09:24:25PM +0200, Luca Ceresoli wrote:
>>>> On 19/04/20 20:02, Laurent Pinchart wrote:
>>>> [...]
>>>>>> +static irqreturn_t xcsi2rxss_irq_handler(int irq, void *dev_id)
>>>>>> +{
>>>>>> +	struct xcsi2rxss_state *state = (struct xcsi2rxss_state *)dev_id;
>>>>>> +	struct xcsi2rxss_core *core = &state->core;
>>>>>> +	u32 status;
>>>>>> +
>>>>>> +	status = xcsi2rxss_read(core, XCSI_ISR_OFFSET) & XCSI_ISR_ALLINTR_MASK;
>>>>>> +	dev_dbg_ratelimited(core->dev, "interrupt status = 0x%08x\n", status);
>>>>>
>>>>> As this is expected to occur for every frame, I would drop the message,
>>>>> even if rate-limited.
>>>>>
>>>>>> +
>>>>>> +	if (!status)
>>>>>> +		return IRQ_NONE;
>>>>>> +
>>>>>> +	/* Received a short packet */
>>>>>> +	if (status & XCSI_ISR_SPFIFONE) {
>>>>>> +		dev_dbg_ratelimited(core->dev, "Short packet = 0x%08x\n",
>>>>>> +				    xcsi2rxss_read(core, XCSI_SPKTR_OFFSET));
>>>>>> +	}
>>>>>
>>>>> Same here, this will occur all the time, I'd remove this message. You
>>>>> need to read XCSI_SPKTR_OFFSET though, and you should do so in a loop
>>>>> until the XCSI_CSR_SPFIFONE in XCSI_CSR_OFFSET is cleared in case
>>>>> multiple short packets are received before the interrupt handler
>>>>> executes.
>>>>>
>>>>> I also wonder if it would make sense to extract the frame number from
>>>>> the FS short packet, and make it available through the subdev API. I
>>>>> think it should be reported through a V4L2_EVENT_FRAME_SYNC event. This
>>>>> can be implemented later.
>>>>>
>>>>>> +
>>>>>> +	/* Short packet FIFO overflow */
>>>>>> +	if (status & XCSI_ISR_SPFIFOF)
>>>>>> +		dev_dbg_ratelimited(core->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(core->dev, "Stream Line Buffer Full!\n");
>>>>>> +		xcsi2rxss_stop_stream(state);
>>>>>> +		if (core->rst_gpio) {
>>>>>> +			gpiod_set_value(core->rst_gpio, 1);
>>>>>> +			/* minimum 40 dphy_clk_200M cycles */
>>>>>> +			ndelay(250);
>>>>>> +			gpiod_set_value(core->rst_gpio, 0);
>>>>>> +		}
>>>>>
>>>>> I don't think you should stop the core here. xcsi2rxss_stop_stream()
>>>>> calls the source .s_stream(0) operation, which usually involves I2C
>>>>> writes that will sleep.
>>>>>
>>>>> You should instead report an event to userspace (it looks like we have
>>>>> no error event defined in V4L2, one should be added), and rely on the
>>>>> normal stop procedure.
>>>>
>>>> FWIW, since a long time I've been using a modified version of this
>>>> routine, where after a Stream Line Buffer Full condition I just stop and
>>>> restart the csi2rx core and the stream continues after a minimal glitch.
>>>> Other subdev are unaware that anything has happened and keep on streaming.
>>>>
>>>> Not sure this is the correct thing to do, but it's working for me. Also
>>>> I proposed this topic in one of the previous iterations of this patch,
>>>> but the situation was different because the stream on/off was not
>>>> propagated back at that time.
>>>
>>> Thanks for the feedback. How often does this occur in practice ?
>>
>> Quite often indeed in my case, as the MIPI stream comes from a remote
>> sensor via a video serdes chipset, and both the cable and the remote
>> sensor module are subject to heavy EMI. Depending on the setup I
>> observed SLBF happening up to 5~10 times per hour.
> 
> Ouch, that is a lot !

That is the worst case, but yes, its a lot.

> Is that really caused by EMI though ? I thought
> SLBF was due to the downstream components applying backpressure.

Hum, good point. I might be wrong, I did the tests several months ago
and cannot do them again at the moment to confirm. But at some point my
suspect was that in case of noise at the upstream side, on the MIPI line
there can be an excess of packets w.r.t. the normal streams (perhaps
short packets?) that produces frames with more lines than expected. But
it's just a wild idea I got, never had an opportunity to examine it in
depth, sorry.

-- 
Luca