From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 396E2C54FC9 for ; Tue, 21 Apr 2020 09:54:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1C2A32087E for ; Tue, 21 Apr 2020 09:54:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728608AbgDUJyL (ORCPT ); Tue, 21 Apr 2020 05:54:11 -0400 Received: from hostingweb31-40.netsons.net ([89.40.174.40]:56002 "EHLO hostingweb31-40.netsons.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726874AbgDUJyK (ORCPT ); Tue, 21 Apr 2020 05:54:10 -0400 Received: from [37.161.214.89] (port=49523 helo=[192.168.42.159]) by hostingweb31.netsons.net with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.93) (envelope-from ) id 1jQpbN-009P7k-IS; Tue, 21 Apr 2020 11:54:05 +0200 Subject: Re: [PATCH v11 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem driver To: Laurent Pinchart Cc: Vishal Sagar , Hyun Kwon , mchehab@kernel.org, robh+dt@kernel.org, mark.rutland@arm.com, Michal Simek , 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 , Sandip Kothari , Jacopo Mondi , Hyun Kwon References: <20200409194424.45555-1-vishal.sagar@xilinx.com> <20200409194424.45555-3-vishal.sagar@xilinx.com> <20200419180222.GB8117@pendragon.ideasonboard.com> <860c27da-eba0-ddcb-719b-52b2725bd9bf@lucaceresoli.net> <20200420195714.GB8195@pendragon.ideasonboard.com> <0a3ea86b-cb4c-a1db-664e-cfa555d8ccf8@lucaceresoli.net> <20200421083807.GB5983@pendragon.ideasonboard.com> From: Luca Ceresoli Message-ID: <66ad4341-8066-6b09-5473-12825fe26828@lucaceresoli.net> Date: Tue, 21 Apr 2020 11:53:50 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200421083807.GB5983@pendragon.ideasonboard.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - hostingweb31.netsons.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - lucaceresoli.net X-Get-Message-Sender-Via: hostingweb31.netsons.net: authenticated_id: luca@lucaceresoli.net X-Authenticated-Sender: hostingweb31.netsons.net: luca@lucaceresoli.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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