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

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <860c27da-eba0-ddcb-719b-52b2725bd9bf@lucaceresoli.net>
Date:   Mon, 20 Apr 2020 21:24:25 +0200
From:   Luca Ceresoli <luca@...aceresoli.net>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Vishal Sagar <vishal.sagar@...inx.com>
Cc:     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 <hyun.kwon@...inx.com>
Subject: Re: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx
 Subsystem driver

Hi Laurent, Vishal,

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.


-- 
Luca