| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 21 Apr 2020 08:45:06 +0000
From: Vishal Sagar <vsagar@...inx.com>
To: Luca Ceresoli <luca@...aceresoli.net>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>
CC: Hyun Kwon <hyunk@...inx.com>,
"mchehab@...nel.org" <mchehab@...nel.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"mark.rutland@....com" <mark.rutland@....com>,
Michal Simek <michals@...inx.com>,
"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"hans.verkuil@...co.com" <hans.verkuil@...co.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Dinesh Kumar <dineshk@...inx.com>,
Sandip Kothari <sandipk@...inx.com>,
Jacopo Mondi <jacopo@...ndi.org>, Hyun Kwon <hyunk@...inx.com>,
Kondalarao Polisetti <kpolise@...inx.com>
Subject: RE: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx
Subsystem driver
Hi Luca,
> -----Original Message-----
> From: Luca Ceresoli <luca@...aceresoli.net>
> Sent: Tuesday, April 21, 2020 1:16 PM
> To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> Cc: Vishal Sagar <vsagar@...inx.com>; Hyun Kwon <hyunk@...inx.com>;
> mchehab@...nel.org; robh+dt@...nel.org; mark.rutland@....com; Michal
> Simek <michals@...inx.com>; linux-media@...r.kernel.org;
> devicetree@...r.kernel.org; hans.verkuil@...co.com; linux-arm-
> kernel@...ts.infradead.org; linux-kernel@...r.kernel.org; Dinesh Kumar
> <dineshk@...inx.com>; Sandip Kothari <sandipk@...inx.com>; Jacopo Mondi
> <jacopo@...ndi.org>; Hyun Kwon <hyunk@...inx.com>
> Subject: Re: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx
> Subsystem driver
>
> 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
Thanks for sharing your observation.
Getting a stream line buffer full condition indicates a design issue.
Stopping, resetting using video_aresetn and starting is valid way to start MIPI CSI-2 Rx SS but masks the issue.
Hence the current implementation is to warn and stop streaming.
Regards
Vishal Sagar
Powered by blists - more mailing lists