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

[Luca Ceresoli <luca@lucaceresoli.net>]

Hi Laurent,

On 20/04/20 21:57, Laurent Pinchart wrote:
> Hi Luca,
> 
> 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.

-- 
Luca