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=-4.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS 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 A742FC433E3 for ; Wed, 22 Jul 2020 15:26:39 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 62CA52071A for ; Wed, 22 Jul 2020 15:26:39 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="GrTu29b0"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="aG/jx05y" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 62CA52071A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=/xZvNWoa6HjtDJXt5YziBwzvrTVplXvAwELs4WyAEqM=; b=GrTu29b0I5DSrYQRkB+MtugJc Zq5afKEd+BQIjTIn5tI3Zs3JQwtRJH11x9xhloJefMtwU0ygmoQPIjsD3YZjr9lnm05cecRIEtk/Y U94cWaoDR+b3qWA5SUxQWWnro/YuxImJWuECIysg15Aog6KnIyv2nxq+ajGxNEsr1gmSZhzZBYQUY T4WwUhIudWOmfuu+6hl1IkZEXb1uVtMJxkiCdoGwNNWcyeaBwhbQu2sLHtWHjFyKFblL+ta5XLWe/ +9PBWyqB7DQBrJRaTuZ7t/xWbkOT5RAs7zA9KpctcDj3kwnwr40/r94+Ju6Sfw5eVS1Tr6bjL0l/m GpbUXo0zg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jyGcE-0006BD-Ja; Wed, 22 Jul 2020 15:25:06 +0000 Received: from mail-wm1-x341.google.com ([2a00:1450:4864:20::341]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jyGcB-00069W-DK for linux-arm-kernel@lists.infradead.org; Wed, 22 Jul 2020 15:25:04 +0000 Received: by mail-wm1-x341.google.com with SMTP id f18so2388993wml.3 for ; Wed, 22 Jul 2020 08:25:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=GzpjkunuhHZpNCtp+UnJMkYMMtPlO9A+rgbio4BwCmw=; b=aG/jx05ydxlbuWKp3bVJ7fWM5H1emziGd8sHOgk9BSu1xFZju0i2/PlThhHwqx/2d9 jH59xbqJ0lgmFtMjmX1zgTe2RQwJ+n2UMk2N2TYZf/cXIJ5NHfbIX4ClbQFlRBdz9cx5 U3dHbF8rISWcHZfAsdbfG3B002bN7swP3Ahzc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=GzpjkunuhHZpNCtp+UnJMkYMMtPlO9A+rgbio4BwCmw=; b=V8MLTehanrOC0rA3ykc4cGUJPLyuHVbJoZ/n+FKp0cYYL0JKiWNQmrx5BE1fPI5Ygp gx99/PYLuPr5QU4UnYu9z71ldXdXfz03bQ+RMRd70cpbD7eSyW8rJDrd+Af1tGi4Vyhi Ucj6m7zd5ziZjnIgjdxcrH7LkwnlEm1cu4ISilNJACbJjQrIgZxS6jrBfOgVoVGCrqmL hyb4pZNA3FQOnvv8dF2Plh0R2WUCOldyYOVfU/5bT/6DhJWPKMANTGvmOFpyw0y8NmtO AH8QCkvNZgJ5Y/l7othFAtAAKuOE/jjoeBwpxXSiOVSyZwBYIo+bNR6RLxPeRehirtLz nOZg== X-Gm-Message-State: AOAM531O0wAsnUJGdcUoHDiUZnMHrghjPVCut9J15Cd4u7qD111BYj0t OCynOkAQ46Kdj95CT20Rkfcf5Q== X-Google-Smtp-Source: ABdhPJyli9nYQTECIz58WQcrAIR9eMkF5YpVYAe6Yijs2QsfIgsl3ieTCFbf++acxjDD1+j7Wr+8QQ== X-Received: by 2002:a1c:6583:: with SMTP id z125mr138284wmb.173.1595431502265; Wed, 22 Jul 2020 08:25:02 -0700 (PDT) Received: from chromium.org (205.215.190.35.bc.googleusercontent.com. [35.190.215.205]) by smtp.gmail.com with ESMTPSA id r10sm318795wrm.17.2020.07.22.08.25.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 22 Jul 2020 08:25:01 -0700 (PDT) Date: Wed, 22 Jul 2020 15:24:59 +0000 From: Tomasz Figa To: Dafna Hirschfeld Subject: Re: [PATCH v8 05/14] media: rkisp1: add Rockchip ISP1 subdev driver Message-ID: <20200722152459.GC1828171@chromium.org> References: <20190730184256.30338-1-helen.koike@collabora.com> <20190730184256.30338-6-helen.koike@collabora.com> <20190816001323.GF5011@pendragon.ideasonboard.com> <30b6367d-9088-d755-d041-904ff2a48130@collabora.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <30b6367d-9088-d755-d041-904ff2a48130@collabora.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200722_112503_595207_82156A2D X-CRM114-Status: GOOD ( 57.80 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, eddie.cai.linux@gmail.com, kernel@collabora.com, heiko@sntech.de, jacob2.chen@rock-chips.com, jeffy.chen@rock-chips.com, zyc@rock-chips.com, linux-kernel@vger.kernel.org, Allon Huang , linux-rockchip@lists.infradead.org, Helen Koike , Jacob Chen , hans.verkuil@cisco.com, Laurent Pinchart , sakari.ailus@linux.intel.com, zhengsq@rock-chips.com, mchehab@kernel.org, ezequiel@collabora.com, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Dafna, On Sat, Jul 11, 2020 at 01:04:31PM +0200, Dafna Hirschfeld wrote: > Hi Laurent, > > On 16.08.19 02:13, Laurent Pinchart wrote: > > Hello Helen, > > > > Thank you for the patch. > > > > On Tue, Jul 30, 2019 at 03:42:47PM -0300, Helen Koike wrote: [snip] > > > +static void rkisp1_isp_queue_event_sof(struct rkisp1_isp_subdev *isp) > > > +{ > > > + struct v4l2_event event = { > > > + .type = V4L2_EVENT_FRAME_SYNC, > > > + .u.frame_sync.frame_sequence = > > > + atomic_inc_return(&isp->frm_sync_seq) - 1, > > > > I would move the increment to the caller, hiding it in this function is > > error-prone (and if you look at the caller I'm pointing out one possible > > error :-)). > > > > In general usage of frm_sync_seq through the driver seems to be very > > race-prone. It's read in various IRQ handling functions, all coming from > > the same IRQ, so that part is fine (and wouldn't require an atomic > > variable), but when read from the buffer queue handlers I really get a > > red light flashing in my head. I'll try to investigate more when > > reviewing the next patches. > > I see that the only place were 'frame_sequence' is read outside of the irq > handlers is in the capture in 'rkisp1_vb2_buf_queue': > > /* > * If there's no next buffer assigned, queue this buffer directly > * as the next buffer, and update the memory interface. > */ > if (cap->is_streaming && !cap->buf.next && > atomic_read(&cap->rkisp1->isp.frame_sequence) == -1) { > cap->buf.next = ispbuf; > rkisp1_set_next_buf(cap); > } else { > list_add_tail(&ispbuf->queue, &cap->buf.queue); > } > This "if" condition seems very specific, a case where we already stream but v-start was not yet received. > I think it is possible to remove the test 'atomic_read(&cap->rkisp1->isp.frame_sequence) == -1' > from the above condition so that the next buffer is updated in case it is null not just before the first > v-start signal. > We don't have this special case in the Chrome OS code. I suppose it would make it possible to resume the capture 1 frame earlier after a queue underrun, as otherwise the new buffer would be only programmed after the next frame start interrupt and used for the next-next frame. However, it's racy, because programming of the buffer addresses is not atomic and could end up with the hardware using few plane addresses from the new buffer and few from the dummy buffer. Given that and also the fact that a queue underrun is a very special case, where the system was already having problems catching up, I'd just remove this special case. [snip] > > > +void rkisp1_isp_isr(unsigned int isp_mis, struct rkisp1_device *dev) > > > +{ > > > + void __iomem *base = dev->base_addr; > > > + unsigned int isp_mis_tmp = 0; > > > > _tmp are never good names :-S > > > > > + unsigned int isp_err = 0; > > > > Neither of these variable need to be initialised to 0. > > > > > + > > > + /* start edge of v_sync */ > > > + if (isp_mis & CIF_ISP_V_START) { > > > + rkisp1_isp_queue_event_sof(&dev->isp_sdev); > > > > This will increment the frame sequence number. What if the interrupt is > > slightly delayed and the next frame starts before we get a change to > > copy the sequence number to the buffers (before they will complete > > below) ? > > Do you mean that we get two sequental v-start signals and then the next > frame-end signal in MI_MIS belongs to the first v-start signal of the two? > How can this be solved? I wonder if any v-start signal has a later signal > that correspond to the same frame so that we can follow it? > > Maybe we should have one counter that is incremented on v-start signal, > and another counter that is incremented uppon some other signal? > We're talking about a hard IRQ. I can't imagine the interrupt handler being delayed for a time close to a full frame interval (~16ms for 60 fps) to trigger such scenario. > > > > > + > > > + writel(CIF_ISP_V_START, base + CIF_ISP_ICR); > > > > Do you need to clear all interrupt bits individually, can't you write > > isp_mis to CIF_ISP_ICR at the beginning of the function to clear them > > all in one go ? > > > > > + isp_mis_tmp = readl(base + CIF_ISP_MIS); > > > + if (isp_mis_tmp & CIF_ISP_V_START) > > > + v4l2_err(&dev->v4l2_dev, "isp icr v_statr err: 0x%x\n", > > > + isp_mis_tmp); > > > > This require some explanation. It looks like a naive way to protect > > against something, but I think it could trigger under normal > > circumstances if IRQ handling is delayed, and wouldn't do much anyway. > > Same for the similar constructs below. > > > > > + } > > > + > > > + if ((isp_mis & CIF_ISP_PIC_SIZE_ERROR)) { > > > + /* Clear pic_size_error */ > > > + writel(CIF_ISP_PIC_SIZE_ERROR, base + CIF_ISP_ICR); > > > + isp_err = readl(base + CIF_ISP_ERR); > > > + v4l2_err(&dev->v4l2_dev, > > > + "CIF_ISP_PIC_SIZE_ERROR (0x%08x)", isp_err); > > > > What does this mean ? > > > > > + writel(isp_err, base + CIF_ISP_ERR_CLR); > > > + } else if ((isp_mis & CIF_ISP_DATA_LOSS)) { > > > > Are CIF_ISP_PIC_SIZE_ERROR and CIF_ISP_DATA_LOSS mutually exclusive ? > > > > > + /* Clear data_loss */ > > > + writel(CIF_ISP_DATA_LOSS, base + CIF_ISP_ICR); > > > + v4l2_err(&dev->v4l2_dev, "CIF_ISP_DATA_LOSS\n"); > > > + writel(CIF_ISP_DATA_LOSS, base + CIF_ISP_ICR); > > > + } > > > + > > > + /* sampled input frame is complete */ > > > + if (isp_mis & CIF_ISP_FRAME_IN) { > > > + writel(CIF_ISP_FRAME_IN, base + CIF_ISP_ICR); > > > + isp_mis_tmp = readl(base + CIF_ISP_MIS); > > > + if (isp_mis_tmp & CIF_ISP_FRAME_IN) > > > + v4l2_err(&dev->v4l2_dev, "isp icr frame_in err: 0x%x\n", > > > + isp_mis_tmp); > > > + } > > > + > > > + /* frame was completely put out */ > > > > "put out" ? :-) What's the difference between ISP_FRAME_IN and ISP_FRAME > > ? The two comments could do with a bit of brush up, and I think the > > ISP_FRAME_IN interrupt could be disabled as it doesn't perform any > > action. > > Those two oneline comments are just copy-paste from the datasheet. > > "" > 5 MIS_FRAME_IN sampled input frame is complete > 1 MIS_FRAME frame was completely put out > "" > > Unfrotunately, the datasheet does not add any further explanation about those signals. > > My loose recollection is that the former is signaled when then frame is fully input to the ISP and the latter when the ISP completes outputting the frame to the next block in the pipeline, but someone would need to verify this, for example by printing timestamps for all the various interrupts. > > > > > + if (isp_mis & CIF_ISP_FRAME) { > > > + u32 isp_ris = 0; > > > > No need to initialise this to 0. > > > > > + /* Clear Frame In (ISP) */ > > > + writel(CIF_ISP_FRAME, base + CIF_ISP_ICR); > > > + isp_mis_tmp = readl(base + CIF_ISP_MIS); > > > + if (isp_mis_tmp & CIF_ISP_FRAME) > > > + v4l2_err(&dev->v4l2_dev, > > > + "isp icr frame end err: 0x%x\n", isp_mis_tmp); > > > + > > > + isp_ris = readl(base + CIF_ISP_RIS); > > > + if (isp_ris & (CIF_ISP_AWB_DONE | CIF_ISP_AFM_FIN | > > > + CIF_ISP_EXP_END | CIF_ISP_HIST_MEASURE_RDY)) > > > + rkisp1_stats_isr(&dev->stats_vdev, isp_ris); > > > > Is there a guarantee that the statistics will be fully written out > > before the video frame itself ? And doesn't this test if any of the > > statistics is complete, not all of them ? I think the logic is wrong, it > > The datasheet does not add any explanation of what is expected to come first. > Should we wait until all statistics measurements are done? In the struct > sent to userspace there is a bitmaks for which of the statistics are read. > I think that if only part of the statistics are ready, we can already send the once > that are ready to userspace. > If we look further into the code, rkisp1_stats_isr() checks the interrupt status mask passed to it and reads out only the parameters with indicated completion. The statistics metadata buffer format includes a bit mask which tells the userspace which measurements are available. However, I think I've spotted a bug there. At the beginning of rkisp1_stats_isr(), all the 4 interrupt status bits are cleared, regardless of the mask used later to decide which readouts need to be done. This could mean that with an unfortunate timing, some measurements would be lost. So at least the code should be fixed to only clear the interrupts bits really handled. As for whether to send separate buffers for each measurement, I guess it's not a bad thing to let the userspace access the ones available earlier. Now I only don't recall why we decided to put all the measurements into one metadata structure, rather than splitting the 4 into their own structures and buffer queues... > > seems it should be moved out of the CIF_ISP_FRAME test, to a test of its > > own. It's hard to tell for sure without extra information though (for > > instance why are the stats-related bits read from CIF_ISP_RIS, when > > they seem to be documented as valid in CIF_ISP_ISR), but this should be > > validated, and most probably fixed. Care should be taken to keep > > synchronisation of sequence number between the different queues. > > I see that the capture buffers are done before incrementing the frame_sequence with > the following explanation: > > /* > * Call rkisp1_capture_isr() first to handle the frame that > * potentially completed using the current frame_sequence number before > * it is potentially incremented by rkisp1_isp_isr() in the vertical > * sync. > */ > > I think reading the stats/params should also be done before calling rkisp1_capture_isr > for the same reason. (so to match the correct frame_sequence) My recollection of the sequence of interrupts in this hardware is like this: CIF_ISP_V_START (frame 0) CIF_ISP_FRAME_IN (frame 0) CIF_ISP_FRAME (frame 0) CIF_ISP_AWB_DONE CIF_ISP_AFM_FIN CIF_ISP_EXP_END CIF_ISP_HIST_MEASURE_RDY CIF_MI_FRAME* CIF_ISP_V_START (frame 1) CIF_ISP_FRAME_IN (frame 1) CIF_ISP_FRAME (frame 1) ... where the interrupts at the same indentation level can happen independently of each other. Again, someone would have to verify this. Best regards, Tomasz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel