From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Subject: Re: [RFC PATCH V2 4/4] platform: mtk-isp: Add Mediatek FD driver Date: Thu, 5 Sep 2019 17:52:47 +0900 Message-ID: References: <1562661672-22439-1-git-send-email-Jerry-Ch.chen@mediatek.com> <1562661672-22439-5-git-send-email-Jerry-Ch.chen@mediatek.com> <20190802082815.GA203993@chromium.org> <1566724680.20680.8.camel@mtksdccf07> <1566957625.20680.33.camel@mtksdccf07> <1567424859.18318.32.camel@mtksdccf07> <1567493081.18318.49.camel@mtksdccf07> <1567511169.18318.65.camel@mtksdccf07> <1567568281.18318.80.camel@mtksdccf07> <1567577389.18318.100.camel@mtksdccf07> <1567584577.22453.11.camel@mtksdccf07> <1567587708.22453.15.camel@mtksdccf07> <1567589188.22453.24.camel@mtksdccf07> <1567603143.22453.27.camel@mtksdccf07> <1567666940.22453.31.camel@mtksdccf07> <1567671418.22453.41.camel@mtksdccf07> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1567671418.22453.41.camel@mtksdccf07> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Jerry-ch Chen Cc: "devicetree@vger.kernel.org" , =?UTF-8?B?U2VhbiBDaGVuZyAo6YSt5piH5byYKQ==?= , "laurent.pinchart+renesas@ideasonboard.com" , =?UTF-8?B?UnlubiBXdSAo5ZCz6IKy5oGpKQ==?= , srv_heupstream , =?UTF-8?B?UG8tWWFuZyBIdWFuZyAo6buD5p+P6Zm9KQ==?= , "mchehab@kernel.org" , "suleiman@chromium.org" , "shik@chromium.org" , =?UTF-8?B?SnVuZ28gTGluICjmnpfmmI7kv4op?= , =?UTF-8?B?U2ogSHVhbmcgKOm7g+S/oeeSiyk=?= , "yuzhao@chromium.org" , "linux-mediatek@lists.infradead.org" , "zwisler@chromium.org" , "matthias.bgg@gmail.com" List-Id: devicetree@vger.kernel.org On Thu, Sep 5, 2019 at 5:17 PM Jerry-ch Chen wrote: > > Hi Tomasz, > > On Thu, 2019-09-05 at 15:13 +0800, Tomasz Figa wrote: > > On Thu, Sep 5, 2019 at 4:02 PM Jerry-ch Chen wrote: > > > > > > Hi Tomasz, > > > > > > On Wed, 2019-09-04 at 21:19 +0800, Jerry-ch Chen wrote: > > > > On Wed, 2019-09-04 at 21:12 +0800, Tomasz Figa wrote: > > > > > On Wed, Sep 4, 2019 at 6:26 PM Jerry-ch Chen wrote: > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > On Wed, 2019-09-04 at 17:03 +0800, Tomasz Figa wrote: > > > > > > > On Wed, Sep 4, 2019 at 6:02 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > On Wed, 2019-09-04 at 16:25 +0800, Tomasz Figa wrote: > > > > > > > > > On Wed, Sep 4, 2019 at 5:09 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > On Wed, 2019-09-04 at 14:34 +0800, Tomasz Figa wrote: > > > > > > > > > > > On Wed, Sep 4, 2019 at 3:09 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > On Wed, 2019-09-04 at 12:15 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > On Wed, Sep 4, 2019 at 12:38 PM Jerry-ch Chen > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 2019-09-03 at 20:05 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > On Tue, Sep 3, 2019 at 8:46 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 2019-09-03 at 15:04 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > > > On Tue, Sep 3, 2019 at 3:44 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 2019-09-03 at 13:19 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > > > > > On Mon, Sep 2, 2019 at 8:47 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, 2019-08-30 at 16:33 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > > > > > > > On Wed, Aug 28, 2019 at 11:00 AM Jerry-ch Chen > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, 2019-08-26 at 14:36 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > > > > > > > > > Hi Jerry, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Sun, Aug 25, 2019 at 6:18 PM Jerry-ch Chen > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, 2019-08-02 at 16:28 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Hi Jerry, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Jul 09, 2019 at 04:41:12PM +0800, Jerry-ch Chen wrote: > > > > > > [snip] > > > > > > > > > > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq) > > > > > > > > > > { > > > > > > > > > > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq); > > > > > > > > > > struct mtk_fd_dev *fd = ctx->fd_dev; > > > > > > > > > > struct vb2_v4l2_buffer *vb; > > > > > > > > > > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx; > > > > > > > > > > struct v4l2_m2m_queue_ctx *queue_ctx; > > > > > > > > > > u32 ret; > > > > > > > > > > > > > > > > > > > > if (!fd->fd_irq_done.done) > > > > > > > > > > > > > > > > > > We shouldn't access internal fields of completion. > > > > > > > > > > > > > > > > > > > ret = wait_for_completion_timeout(&fd->fd_irq_done, > > > > > > > > > > msecs_to_jiffies( > > > > > > > > > > MTK_FD_HW_TIMEOUT)); > > > > > > > > > > queue_ctx = V4L2_TYPE_IS_OUTPUT(vq->type) ? > > > > > > > > > > &m2m_ctx->out_q_ctx : > > > > > > > > > > &m2m_ctx->cap_q_ctx; > > > > > > > > > > while ((vb = v4l2_m2m_buf_remove(queue_ctx))) > > > > > > > > > > v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR); > > > > > > > > > > > > > > > > > > > > if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > > > > > > > > > > mtk_fd_hw_disconnect(fd); > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > I've also tried to wait completion unconditionally for both queues and > > > > > > > > > > the second time will wait until timeout, as a result, it takes longer to > > > > > > > > > > swap the camera every time and close the camera app. > > > > > > > > > > > > > > > > > > I think it should work better if we call complete_all() instead of complete(). > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > I use complete_all(), and it works fine now. > > > > > > > > > > > > > > > > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq) > > > > > > > > { > > > > > > > > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq); > > > > > > > > struct mtk_fd_dev *fd = ctx->fd_dev; > > > > > > > > struct vb2_v4l2_buffer *vb; > > > > > > > > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx; > > > > > > > > struct v4l2_m2m_queue_ctx *queue_ctx; > > > > > > > > > > > > > > > > wait_for_completion_timeout(&fd->fd_irq_done, > > > > > > > > msecs_to_jiffies(MTK_FD_HW_TIMEOUT)); > > > > > > > > > > > > > > Shouldn't we still send some command to the hardware to stop? Like a > > > > > > > reset. Otherwise we don't know if it isn't still accessing the memory. > > > > > > > > > > > > > I thought no more jobs will be enqueued here when stop_streaming so we > > > > > > don't need it. > > > > > > > > > > That's true for the case when the wait completed successfully, but we > > > > > also need to ensure the hardware is stopped even if a timeout happens. > > > > > > > > > > > We still could send an ipi command to reset the HW, and wait for it's > > > > > > callback or we could set the register MTK_FD_REG_OFFSET_HW_ENABLE to > > > > > > zero to disable the HW. > > > > > > > > > > Since it's for handling a timeout, a reset should be more likely to > > > > > bring the hardware back to a reasonable state. > > > > > > > > > > > > > Ok, I will send the ipi command to reset the HW. > > > > > > > > Thanks and best regards, > > > > Jerry > > > I've tested and will refine as following: > > > > > > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq) > > > { > > > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq); > > > struct mtk_fd_dev *fd = ctx->fd_dev; > > > struct vb2_v4l2_buffer *vb; > > > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx; > > > struct v4l2_m2m_queue_ctx *queue_ctx; > > > u32 ret; > > > > > > ret = wait_for_completion_timeout(&fd->fd_irq_done, > > > msecs_to_jiffies(MTK_FD_HW_TIMEOUT)); > > > /* Disable FD HW */ > > > if(!ret) { > > > struct ipi_message fd_ipi_msg; > > > > > > fd_ipi_msg.cmd_id = MTK_FD_IPI_CMD_RESET; > > > ret = scp_ipi_send(fd->scp_pdev, SCP_IPI_FD_CMD, &fd_ipi_msg, > > > sizeof(fd_ipi_msg), MTK_FD_IPI_SEND_TIMEOUT); > > > if (ret) > > > dev_err(fd->dev, "FD Reset HW error\n"); > > > } > > > > Would you also put the same code in suspend handler? If so, perhaps > > it's better to keep this in a helper function (mtk_fd_job_abort()) as > > we had before? > > > > Ok, done, It will reset the HW and return ETIMEOUT if the last job is > timeout, the return value will be used in suspend for further action. > > static int mtk_fd_job_abort(struct mtk_fd_dev *fd) > { > u32 ret; > > ret = wait_for_completion_timeout(&fd->fd_irq_done, > msecs_to_jiffies(MTK_FD_HW_TIMEOUT)); > /* Reset FD HW */ > if (!ret) { > struct ipi_message fd_ipi_msg; > > fd_ipi_msg.cmd_id = MTK_FD_IPI_CMD_RESET; > if (scp_ipi_send(fd->scp_pdev, SCP_IPI_FD_CMD, &fd_ipi_msg, > sizeof(fd_ipi_msg), MTK_FD_IPI_SEND_TIMEOUT)) > dev_err(fd->dev, "FD Reset HW error\n"); > return -ETIMEDOUT; > } > return 0; > } > > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq) > { > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq); > struct mtk_fd_dev *fd = ctx->fd_dev; > struct vb2_v4l2_buffer *vb; > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx; > struct v4l2_m2m_queue_ctx *queue_ctx; > > mtk_fd_job_abort(fd); > queue_ctx = V4L2_TYPE_IS_OUTPUT(vq->type) ? > &m2m_ctx->out_q_ctx : > &m2m_ctx->cap_q_ctx; > while ((vb = v4l2_m2m_buf_remove(queue_ctx))) > v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR); > > if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > mtk_fd_hw_disconnect(fd); > } > > static int mtk_fd_suspend(struct device *dev) > { > struct mtk_fd_dev *fd = dev_get_drvdata(dev); > > if (pm_runtime_suspended(dev)) > return 0; > > if (fd->fd_stream_count) > if (mtk_fd_job_abort(fd)) > mtk_fd_hw_job_finish(fd, VB2_BUF_STATE_ERROR); Wouldn't this cause the next job to be run? > > /* suspend FD HW */ > writel(0x0, fd->fd_base + MTK_FD_REG_OFFSET_INT_EN); > writel(0x0, fd->fd_base + MTK_FD_REG_OFFSET_HW_ENABLE); > clk_disable_unprepare(fd->fd_clk); > dev_dbg(dev, "%s:disable clock\n", __func__); > > return 0; > } > > > > queue_ctx = V4L2_TYPE_IS_OUTPUT(vq->type) ? > > > &m2m_ctx->out_q_ctx : > > > &m2m_ctx->cap_q_ctx; > > > while ((vb = v4l2_m2m_buf_remove(queue_ctx))) > > > v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR); > > > > > > if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > > > mtk_fd_hw_disconnect(fd); > > > } > > > > > > If there is no other concern, may I send the RFC v3 patch for review? > > > > Thanks, technically it looks good now. Just one comment about avoiding > > code duplication above. > > > > Thanks, > > I will send the v3 if the above fix-up is accepted, I think there is a bigger issue here actually, related to how the m2m helpers work. Let's just keep the code as you proposed and post v3. We can continue the discussion there. Best regards, Tomasz 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=-0.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 43A2CC3A5A5 for ; Thu, 5 Sep 2019 09:00:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id F055E21848 for ; Thu, 5 Sep 2019 09:00:32 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="N+BMQkP3" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731794AbfIEJAc (ORCPT ); Thu, 5 Sep 2019 05:00:32 -0400 Received: from mail-ed1-f66.google.com ([209.85.208.66]:39684 "EHLO mail-ed1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726231AbfIEJAc (ORCPT ); Thu, 5 Sep 2019 05:00:32 -0400 Received: by mail-ed1-f66.google.com with SMTP id u6so1919352edq.6 for ; Thu, 05 Sep 2019 02:00:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=dr4pHmSbqe8BWrRQh4f+8Z19xbBALuiWZmuc5+cRwSA=; b=N+BMQkP3QqTkvqLo+EcuPIVYJckeqq9bSRoz0BzotfagDCNrtzKCjLkr30aS0dzOSO MTzbf55qcGOEw7kCvHI/7OWISUdJmAc6vgOuzppMXxUTyMG1OeSdBtVOhGIeeS6IPdj5 gT58JuSqIqaK7d7xgkbGcg5k0XvK2QTKqllRo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=dr4pHmSbqe8BWrRQh4f+8Z19xbBALuiWZmuc5+cRwSA=; b=mlIDzss/XmDpaclFrJI+/p9o9K0Kg7bzqdY4yGAfHql3T+3hzvqcsKmFiGb6CXh7T9 KXu8XNgZ89GuPy9VLr8+lMCMxCiQ/AlCQ5c8c3oWuuVDa6DCegGo/y8SlPuNddS2vVwe jz+G0P9ukd7wWwJgyct8OAVafnEgJLlm3u4E5qhoX53by4oWxYuDsFeB3TF9g46O3tzR X1nDb+3c86L5B8bwxoUy/qOgL+MRGQyVgdQh7U4cSRsuWkWkMLnqqqOhaHUfVZPq3qWU 3mmDPy4g+KSmY0GouaNPzDZy1WPi0aQ+4/COfDNAk0Dq9hc62eF7Y1nWxiOUCd+3ydDZ EKuA== X-Gm-Message-State: APjAAAVUkcwdCuHWmskASwXS3xu/P0X5Vg9jte1YYzVx++r1Zx8oh5Ne J0ML9U0kmYsbymy0iwnoPC04qSv8GbyNyQ== X-Google-Smtp-Source: APXvYqzDP2yKRjngjyTtdoEMVwb3quxCOB2OM2x9TogCEPOM+d1eN/T7ycs/XibyZNsXf2Zu62gkFg== X-Received: by 2002:a17:906:85c8:: with SMTP id i8mr1894469ejy.178.1567674029442; Thu, 05 Sep 2019 02:00:29 -0700 (PDT) Received: from mail-wr1-f51.google.com (mail-wr1-f51.google.com. [209.85.221.51]) by smtp.gmail.com with ESMTPSA id a13sm168716ejj.23.2019.09.05.02.00.29 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 05 Sep 2019 02:00:29 -0700 (PDT) Received: by mail-wr1-f51.google.com with SMTP id h7so744439wrw.8 for ; Thu, 05 Sep 2019 02:00:29 -0700 (PDT) X-Received: by 2002:a5d:6585:: with SMTP id q5mr1530322wru.162.1567673580253; Thu, 05 Sep 2019 01:53:00 -0700 (PDT) MIME-Version: 1.0 References: <1562661672-22439-1-git-send-email-Jerry-Ch.chen@mediatek.com> <1562661672-22439-5-git-send-email-Jerry-Ch.chen@mediatek.com> <20190802082815.GA203993@chromium.org> <1566724680.20680.8.camel@mtksdccf07> <1566957625.20680.33.camel@mtksdccf07> <1567424859.18318.32.camel@mtksdccf07> <1567493081.18318.49.camel@mtksdccf07> <1567511169.18318.65.camel@mtksdccf07> <1567568281.18318.80.camel@mtksdccf07> <1567577389.18318.100.camel@mtksdccf07> <1567584577.22453.11.camel@mtksdccf07> <1567587708.22453.15.camel@mtksdccf07> <1567589188.22453.24.camel@mtksdccf07> <1567603143.22453.27.camel@mtksdccf07> <1567666940.22453.31.camel@mtksdccf07> <1567671418.22453.41.camel@mtksdccf07> In-Reply-To: <1567671418.22453.41.camel@mtksdccf07> From: Tomasz Figa Date: Thu, 5 Sep 2019 17:52:47 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC PATCH V2 4/4] platform: mtk-isp: Add Mediatek FD driver To: Jerry-ch Chen Cc: "yuzhao@chromium.org" , "zwisler@chromium.org" , "linux-mediatek@lists.infradead.org" , "linux-arm-kernel@lists.infradead.org" , =?UTF-8?B?U2VhbiBDaGVuZyAo6YSt5piH5byYKQ==?= , =?UTF-8?B?U2ogSHVhbmcgKOm7g+S/oeeSiyk=?= , =?UTF-8?B?Q2hyaXN0aWUgWXUgKOa4uOmbheaDoCk=?= , =?UTF-8?B?RnJlZGVyaWMgQ2hlbiAo6Zmz5L+K5YWDKQ==?= , =?UTF-8?B?SnVuZ28gTGluICjmnpfmmI7kv4op?= , =?UTF-8?B?UnlubiBXdSAo5ZCz6IKy5oGpKQ==?= , =?UTF-8?B?UG8tWWFuZyBIdWFuZyAo6buD5p+P6Zm9KQ==?= , "shik@chromium.org" , "suleiman@chromium.org" , "linux-media@vger.kernel.org" , srv_heupstream , "devicetree@vger.kernel.org" , "laurent.pinchart+renesas@ideasonboard.com" , "hans.verkuil@cisco.com" , "mchehab@kernel.org" , "matthias.bgg@gmail.com" Content-Type: text/plain; charset="UTF-8" Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On Thu, Sep 5, 2019 at 5:17 PM Jerry-ch Chen wrote: > > Hi Tomasz, > > On Thu, 2019-09-05 at 15:13 +0800, Tomasz Figa wrote: > > On Thu, Sep 5, 2019 at 4:02 PM Jerry-ch Chen wrote: > > > > > > Hi Tomasz, > > > > > > On Wed, 2019-09-04 at 21:19 +0800, Jerry-ch Chen wrote: > > > > On Wed, 2019-09-04 at 21:12 +0800, Tomasz Figa wrote: > > > > > On Wed, Sep 4, 2019 at 6:26 PM Jerry-ch Chen wrote: > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > On Wed, 2019-09-04 at 17:03 +0800, Tomasz Figa wrote: > > > > > > > On Wed, Sep 4, 2019 at 6:02 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > On Wed, 2019-09-04 at 16:25 +0800, Tomasz Figa wrote: > > > > > > > > > On Wed, Sep 4, 2019 at 5:09 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > On Wed, 2019-09-04 at 14:34 +0800, Tomasz Figa wrote: > > > > > > > > > > > On Wed, Sep 4, 2019 at 3:09 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > On Wed, 2019-09-04 at 12:15 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > On Wed, Sep 4, 2019 at 12:38 PM Jerry-ch Chen > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 2019-09-03 at 20:05 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > On Tue, Sep 3, 2019 at 8:46 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 2019-09-03 at 15:04 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > > > On Tue, Sep 3, 2019 at 3:44 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 2019-09-03 at 13:19 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > > > > > On Mon, Sep 2, 2019 at 8:47 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, 2019-08-30 at 16:33 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > > > > > > > On Wed, Aug 28, 2019 at 11:00 AM Jerry-ch Chen > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, 2019-08-26 at 14:36 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > > > > > > > > > Hi Jerry, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Sun, Aug 25, 2019 at 6:18 PM Jerry-ch Chen > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, 2019-08-02 at 16:28 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Hi Jerry, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Jul 09, 2019 at 04:41:12PM +0800, Jerry-ch Chen wrote: > > > > > > [snip] > > > > > > > > > > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq) > > > > > > > > > > { > > > > > > > > > > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq); > > > > > > > > > > struct mtk_fd_dev *fd = ctx->fd_dev; > > > > > > > > > > struct vb2_v4l2_buffer *vb; > > > > > > > > > > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx; > > > > > > > > > > struct v4l2_m2m_queue_ctx *queue_ctx; > > > > > > > > > > u32 ret; > > > > > > > > > > > > > > > > > > > > if (!fd->fd_irq_done.done) > > > > > > > > > > > > > > > > > > We shouldn't access internal fields of completion. > > > > > > > > > > > > > > > > > > > ret = wait_for_completion_timeout(&fd->fd_irq_done, > > > > > > > > > > msecs_to_jiffies( > > > > > > > > > > MTK_FD_HW_TIMEOUT)); > > > > > > > > > > queue_ctx = V4L2_TYPE_IS_OUTPUT(vq->type) ? > > > > > > > > > > &m2m_ctx->out_q_ctx : > > > > > > > > > > &m2m_ctx->cap_q_ctx; > > > > > > > > > > while ((vb = v4l2_m2m_buf_remove(queue_ctx))) > > > > > > > > > > v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR); > > > > > > > > > > > > > > > > > > > > if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > > > > > > > > > > mtk_fd_hw_disconnect(fd); > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > I've also tried to wait completion unconditionally for both queues and > > > > > > > > > > the second time will wait until timeout, as a result, it takes longer to > > > > > > > > > > swap the camera every time and close the camera app. > > > > > > > > > > > > > > > > > > I think it should work better if we call complete_all() instead of complete(). > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > I use complete_all(), and it works fine now. > > > > > > > > > > > > > > > > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq) > > > > > > > > { > > > > > > > > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq); > > > > > > > > struct mtk_fd_dev *fd = ctx->fd_dev; > > > > > > > > struct vb2_v4l2_buffer *vb; > > > > > > > > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx; > > > > > > > > struct v4l2_m2m_queue_ctx *queue_ctx; > > > > > > > > > > > > > > > > wait_for_completion_timeout(&fd->fd_irq_done, > > > > > > > > msecs_to_jiffies(MTK_FD_HW_TIMEOUT)); > > > > > > > > > > > > > > Shouldn't we still send some command to the hardware to stop? Like a > > > > > > > reset. Otherwise we don't know if it isn't still accessing the memory. > > > > > > > > > > > > > I thought no more jobs will be enqueued here when stop_streaming so we > > > > > > don't need it. > > > > > > > > > > That's true for the case when the wait completed successfully, but we > > > > > also need to ensure the hardware is stopped even if a timeout happens. > > > > > > > > > > > We still could send an ipi command to reset the HW, and wait for it's > > > > > > callback or we could set the register MTK_FD_REG_OFFSET_HW_ENABLE to > > > > > > zero to disable the HW. > > > > > > > > > > Since it's for handling a timeout, a reset should be more likely to > > > > > bring the hardware back to a reasonable state. > > > > > > > > > > > > > Ok, I will send the ipi command to reset the HW. > > > > > > > > Thanks and best regards, > > > > Jerry > > > I've tested and will refine as following: > > > > > > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq) > > > { > > > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq); > > > struct mtk_fd_dev *fd = ctx->fd_dev; > > > struct vb2_v4l2_buffer *vb; > > > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx; > > > struct v4l2_m2m_queue_ctx *queue_ctx; > > > u32 ret; > > > > > > ret = wait_for_completion_timeout(&fd->fd_irq_done, > > > msecs_to_jiffies(MTK_FD_HW_TIMEOUT)); > > > /* Disable FD HW */ > > > if(!ret) { > > > struct ipi_message fd_ipi_msg; > > > > > > fd_ipi_msg.cmd_id = MTK_FD_IPI_CMD_RESET; > > > ret = scp_ipi_send(fd->scp_pdev, SCP_IPI_FD_CMD, &fd_ipi_msg, > > > sizeof(fd_ipi_msg), MTK_FD_IPI_SEND_TIMEOUT); > > > if (ret) > > > dev_err(fd->dev, "FD Reset HW error\n"); > > > } > > > > Would you also put the same code in suspend handler? If so, perhaps > > it's better to keep this in a helper function (mtk_fd_job_abort()) as > > we had before? > > > > Ok, done, It will reset the HW and return ETIMEOUT if the last job is > timeout, the return value will be used in suspend for further action. > > static int mtk_fd_job_abort(struct mtk_fd_dev *fd) > { > u32 ret; > > ret = wait_for_completion_timeout(&fd->fd_irq_done, > msecs_to_jiffies(MTK_FD_HW_TIMEOUT)); > /* Reset FD HW */ > if (!ret) { > struct ipi_message fd_ipi_msg; > > fd_ipi_msg.cmd_id = MTK_FD_IPI_CMD_RESET; > if (scp_ipi_send(fd->scp_pdev, SCP_IPI_FD_CMD, &fd_ipi_msg, > sizeof(fd_ipi_msg), MTK_FD_IPI_SEND_TIMEOUT)) > dev_err(fd->dev, "FD Reset HW error\n"); > return -ETIMEDOUT; > } > return 0; > } > > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq) > { > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq); > struct mtk_fd_dev *fd = ctx->fd_dev; > struct vb2_v4l2_buffer *vb; > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx; > struct v4l2_m2m_queue_ctx *queue_ctx; > > mtk_fd_job_abort(fd); > queue_ctx = V4L2_TYPE_IS_OUTPUT(vq->type) ? > &m2m_ctx->out_q_ctx : > &m2m_ctx->cap_q_ctx; > while ((vb = v4l2_m2m_buf_remove(queue_ctx))) > v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR); > > if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > mtk_fd_hw_disconnect(fd); > } > > static int mtk_fd_suspend(struct device *dev) > { > struct mtk_fd_dev *fd = dev_get_drvdata(dev); > > if (pm_runtime_suspended(dev)) > return 0; > > if (fd->fd_stream_count) > if (mtk_fd_job_abort(fd)) > mtk_fd_hw_job_finish(fd, VB2_BUF_STATE_ERROR); Wouldn't this cause the next job to be run? > > /* suspend FD HW */ > writel(0x0, fd->fd_base + MTK_FD_REG_OFFSET_INT_EN); > writel(0x0, fd->fd_base + MTK_FD_REG_OFFSET_HW_ENABLE); > clk_disable_unprepare(fd->fd_clk); > dev_dbg(dev, "%s:disable clock\n", __func__); > > return 0; > } > > > > queue_ctx = V4L2_TYPE_IS_OUTPUT(vq->type) ? > > > &m2m_ctx->out_q_ctx : > > > &m2m_ctx->cap_q_ctx; > > > while ((vb = v4l2_m2m_buf_remove(queue_ctx))) > > > v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR); > > > > > > if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > > > mtk_fd_hw_disconnect(fd); > > > } > > > > > > If there is no other concern, may I send the RFC v3 patch for review? > > > > Thanks, technically it looks good now. Just one comment about avoiding > > code duplication above. > > > > Thanks, > > I will send the v3 if the above fix-up is accepted, I think there is a bigger issue here actually, related to how the m2m helpers work. Let's just keep the code as you proposed and post v3. We can continue the discussion there. Best regards, Tomasz 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=-0.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED 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 81915C3A5A5 for ; Thu, 5 Sep 2019 08:53:14 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 5167D20578 for ; Thu, 5 Sep 2019 08:53:14 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="iWPzyDZd"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="N+BMQkP3" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5167D20578 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+infradead-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=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=mg9zHYSqFHAGwEaRVLU6rbR+nrDtkvjqWETLl15ZVOM=; b=iWPzyDZde1Rk+h zq/Qc2Vk/H82ZJzFXg/jxHpD3km4EDvC8U66dS1pRZscyQngXhzXBUZYMSRN0CDDmxGj83kEf/css dkZ+Qk/6Eq0hLydLvZcGAaPzD4/4eWGse9tqRX8eJOZ+Z0KEn0tvvuiwuECaXmL9fqNyAdum9Y7qK 8C9tfFGPJBsLN0ZhKT3+13f8wk7yZqam5HPZq2vmUuiAMeIgf4FneljFklFMmCGGIDoRZ/yjX84BM b9ExgL6WxVAeirZWb688QMzudmgVIpL+nTS3w/ukadMfccRNDK+rTzQ0vtDwTsaz4PgB3yMPryyIE RnvsDKc8LjzuuC9qEqHg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1i5nVu-0007dZ-AL; Thu, 05 Sep 2019 08:53:10 +0000 Received: from mail-ed1-x542.google.com ([2a00:1450:4864:20::542]) by bombadil.infradead.org with esmtps (Exim 4.92 #3 (Red Hat Linux)) id 1i5nVp-0007cJ-K6 for linux-arm-kernel@lists.infradead.org; Thu, 05 Sep 2019 08:53:08 +0000 Received: by mail-ed1-x542.google.com with SMTP id c19so1863068edy.10 for ; Thu, 05 Sep 2019 01:53:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=dr4pHmSbqe8BWrRQh4f+8Z19xbBALuiWZmuc5+cRwSA=; b=N+BMQkP3QqTkvqLo+EcuPIVYJckeqq9bSRoz0BzotfagDCNrtzKCjLkr30aS0dzOSO MTzbf55qcGOEw7kCvHI/7OWISUdJmAc6vgOuzppMXxUTyMG1OeSdBtVOhGIeeS6IPdj5 gT58JuSqIqaK7d7xgkbGcg5k0XvK2QTKqllRo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=dr4pHmSbqe8BWrRQh4f+8Z19xbBALuiWZmuc5+cRwSA=; b=UZ/cDyEw7/goHl7x02nbpGHaso95J46vWWqdbeQfkvzGxapK2kQRcoDFbRNVf0gZEt wHyRaDu8jDd8qZ+CSLpSBkf61aMhc9PR/EYJJC6N/XjfAzyhXmIVFb1V9eQio7jLBlMS 3H/HSmtXJ9wgWZcmkzoGelSJO/WbeT3W+KwKLPz4goOfuvtGKS/95Zdor0ylinRnQ6li HUNgRZI8ZX0IwrTPacfVnX6RGu2N+zIWMZvoJXajpZeNIwYBp5J9eiqkrZvtlpb+cHSY m7aTcQ9PXCrsqL+y3FtQcgDsnHJqOD+90rUph1qmaxea5byCYA3lYFWDDE9GeIuIfDp6 VUug== X-Gm-Message-State: APjAAAVyWRFcLlCog1HLXCipP+vqGV9QGIszlrc+/WH202v1jXV56thT /+KRlA82Y6fRFjeSwvS5KXUn4UPi41kxjg== X-Google-Smtp-Source: APXvYqz3WizLlZnoALit2B0rrVwtfsdK+i87tD/QiIVbE1UQ74hiJEX4+N2GcjQ+8HnpEV8OzNyKpw== X-Received: by 2002:a50:ef01:: with SMTP id m1mr42005eds.11.1567673583289; Thu, 05 Sep 2019 01:53:03 -0700 (PDT) Received: from mail-wr1-f53.google.com (mail-wr1-f53.google.com. [209.85.221.53]) by smtp.gmail.com with ESMTPSA id ga12sm171476ejb.40.2019.09.05.01.53.00 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 05 Sep 2019 01:53:01 -0700 (PDT) Received: by mail-wr1-f53.google.com with SMTP id h7so715519wrw.8 for ; Thu, 05 Sep 2019 01:53:00 -0700 (PDT) X-Received: by 2002:a5d:6585:: with SMTP id q5mr1530322wru.162.1567673580253; Thu, 05 Sep 2019 01:53:00 -0700 (PDT) MIME-Version: 1.0 References: <1562661672-22439-1-git-send-email-Jerry-Ch.chen@mediatek.com> <1562661672-22439-5-git-send-email-Jerry-Ch.chen@mediatek.com> <20190802082815.GA203993@chromium.org> <1566724680.20680.8.camel@mtksdccf07> <1566957625.20680.33.camel@mtksdccf07> <1567424859.18318.32.camel@mtksdccf07> <1567493081.18318.49.camel@mtksdccf07> <1567511169.18318.65.camel@mtksdccf07> <1567568281.18318.80.camel@mtksdccf07> <1567577389.18318.100.camel@mtksdccf07> <1567584577.22453.11.camel@mtksdccf07> <1567587708.22453.15.camel@mtksdccf07> <1567589188.22453.24.camel@mtksdccf07> <1567603143.22453.27.camel@mtksdccf07> <1567666940.22453.31.camel@mtksdccf07> <1567671418.22453.41.camel@mtksdccf07> In-Reply-To: <1567671418.22453.41.camel@mtksdccf07> From: Tomasz Figa Date: Thu, 5 Sep 2019 17:52:47 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC PATCH V2 4/4] platform: mtk-isp: Add Mediatek FD driver To: Jerry-ch Chen X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190905_015305_712875_2D95E812 X-CRM114-Status: GOOD ( 27.69 ) 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" , =?UTF-8?B?U2VhbiBDaGVuZyAo6YSt5piH5byYKQ==?= , "laurent.pinchart+renesas@ideasonboard.com" , =?UTF-8?B?UnlubiBXdSAo5ZCz6IKy5oGpKQ==?= , srv_heupstream , =?UTF-8?B?UG8tWWFuZyBIdWFuZyAo6buD5p+P6Zm9KQ==?= , "mchehab@kernel.org" , "suleiman@chromium.org" , "shik@chromium.org" , =?UTF-8?B?SnVuZ28gTGluICjmnpfmmI7kv4op?= , =?UTF-8?B?U2ogSHVhbmcgKOm7g+S/oeeSiyk=?= , "yuzhao@chromium.org" , "linux-mediatek@lists.infradead.org" , "zwisler@chromium.org" , "matthias.bgg@gmail.com" , =?UTF-8?B?Q2hyaXN0aWUgWXUgKOa4uOmbheaDoCk=?= , =?UTF-8?B?RnJlZGVyaWMgQ2hlbiAo6Zmz5L+K5YWDKQ==?= , "hans.verkuil@cisco.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+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Sep 5, 2019 at 5:17 PM Jerry-ch Chen wrote: > > Hi Tomasz, > > On Thu, 2019-09-05 at 15:13 +0800, Tomasz Figa wrote: > > On Thu, Sep 5, 2019 at 4:02 PM Jerry-ch Chen wrote: > > > > > > Hi Tomasz, > > > > > > On Wed, 2019-09-04 at 21:19 +0800, Jerry-ch Chen wrote: > > > > On Wed, 2019-09-04 at 21:12 +0800, Tomasz Figa wrote: > > > > > On Wed, Sep 4, 2019 at 6:26 PM Jerry-ch Chen wrote: > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > On Wed, 2019-09-04 at 17:03 +0800, Tomasz Figa wrote: > > > > > > > On Wed, Sep 4, 2019 at 6:02 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > On Wed, 2019-09-04 at 16:25 +0800, Tomasz Figa wrote: > > > > > > > > > On Wed, Sep 4, 2019 at 5:09 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > On Wed, 2019-09-04 at 14:34 +0800, Tomasz Figa wrote: > > > > > > > > > > > On Wed, Sep 4, 2019 at 3:09 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > On Wed, 2019-09-04 at 12:15 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > On Wed, Sep 4, 2019 at 12:38 PM Jerry-ch Chen > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 2019-09-03 at 20:05 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > On Tue, Sep 3, 2019 at 8:46 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 2019-09-03 at 15:04 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > > > On Tue, Sep 3, 2019 at 3:44 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 2019-09-03 at 13:19 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > > > > > On Mon, Sep 2, 2019 at 8:47 PM Jerry-ch Chen wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, 2019-08-30 at 16:33 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > > > > > > > On Wed, Aug 28, 2019 at 11:00 AM Jerry-ch Chen > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, 2019-08-26 at 14:36 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > > > > > > > > > Hi Jerry, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Sun, Aug 25, 2019 at 6:18 PM Jerry-ch Chen > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, 2019-08-02 at 16:28 +0800, Tomasz Figa wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Hi Jerry, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Jul 09, 2019 at 04:41:12PM +0800, Jerry-ch Chen wrote: > > > > > > [snip] > > > > > > > > > > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq) > > > > > > > > > > { > > > > > > > > > > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq); > > > > > > > > > > struct mtk_fd_dev *fd = ctx->fd_dev; > > > > > > > > > > struct vb2_v4l2_buffer *vb; > > > > > > > > > > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx; > > > > > > > > > > struct v4l2_m2m_queue_ctx *queue_ctx; > > > > > > > > > > u32 ret; > > > > > > > > > > > > > > > > > > > > if (!fd->fd_irq_done.done) > > > > > > > > > > > > > > > > > > We shouldn't access internal fields of completion. > > > > > > > > > > > > > > > > > > > ret = wait_for_completion_timeout(&fd->fd_irq_done, > > > > > > > > > > msecs_to_jiffies( > > > > > > > > > > MTK_FD_HW_TIMEOUT)); > > > > > > > > > > queue_ctx = V4L2_TYPE_IS_OUTPUT(vq->type) ? > > > > > > > > > > &m2m_ctx->out_q_ctx : > > > > > > > > > > &m2m_ctx->cap_q_ctx; > > > > > > > > > > while ((vb = v4l2_m2m_buf_remove(queue_ctx))) > > > > > > > > > > v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR); > > > > > > > > > > > > > > > > > > > > if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > > > > > > > > > > mtk_fd_hw_disconnect(fd); > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > I've also tried to wait completion unconditionally for both queues and > > > > > > > > > > the second time will wait until timeout, as a result, it takes longer to > > > > > > > > > > swap the camera every time and close the camera app. > > > > > > > > > > > > > > > > > > I think it should work better if we call complete_all() instead of complete(). > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > I use complete_all(), and it works fine now. > > > > > > > > > > > > > > > > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq) > > > > > > > > { > > > > > > > > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq); > > > > > > > > struct mtk_fd_dev *fd = ctx->fd_dev; > > > > > > > > struct vb2_v4l2_buffer *vb; > > > > > > > > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx; > > > > > > > > struct v4l2_m2m_queue_ctx *queue_ctx; > > > > > > > > > > > > > > > > wait_for_completion_timeout(&fd->fd_irq_done, > > > > > > > > msecs_to_jiffies(MTK_FD_HW_TIMEOUT)); > > > > > > > > > > > > > > Shouldn't we still send some command to the hardware to stop? Like a > > > > > > > reset. Otherwise we don't know if it isn't still accessing the memory. > > > > > > > > > > > > > I thought no more jobs will be enqueued here when stop_streaming so we > > > > > > don't need it. > > > > > > > > > > That's true for the case when the wait completed successfully, but we > > > > > also need to ensure the hardware is stopped even if a timeout happens. > > > > > > > > > > > We still could send an ipi command to reset the HW, and wait for it's > > > > > > callback or we could set the register MTK_FD_REG_OFFSET_HW_ENABLE to > > > > > > zero to disable the HW. > > > > > > > > > > Since it's for handling a timeout, a reset should be more likely to > > > > > bring the hardware back to a reasonable state. > > > > > > > > > > > > > Ok, I will send the ipi command to reset the HW. > > > > > > > > Thanks and best regards, > > > > Jerry > > > I've tested and will refine as following: > > > > > > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq) > > > { > > > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq); > > > struct mtk_fd_dev *fd = ctx->fd_dev; > > > struct vb2_v4l2_buffer *vb; > > > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx; > > > struct v4l2_m2m_queue_ctx *queue_ctx; > > > u32 ret; > > > > > > ret = wait_for_completion_timeout(&fd->fd_irq_done, > > > msecs_to_jiffies(MTK_FD_HW_TIMEOUT)); > > > /* Disable FD HW */ > > > if(!ret) { > > > struct ipi_message fd_ipi_msg; > > > > > > fd_ipi_msg.cmd_id = MTK_FD_IPI_CMD_RESET; > > > ret = scp_ipi_send(fd->scp_pdev, SCP_IPI_FD_CMD, &fd_ipi_msg, > > > sizeof(fd_ipi_msg), MTK_FD_IPI_SEND_TIMEOUT); > > > if (ret) > > > dev_err(fd->dev, "FD Reset HW error\n"); > > > } > > > > Would you also put the same code in suspend handler? If so, perhaps > > it's better to keep this in a helper function (mtk_fd_job_abort()) as > > we had before? > > > > Ok, done, It will reset the HW and return ETIMEOUT if the last job is > timeout, the return value will be used in suspend for further action. > > static int mtk_fd_job_abort(struct mtk_fd_dev *fd) > { > u32 ret; > > ret = wait_for_completion_timeout(&fd->fd_irq_done, > msecs_to_jiffies(MTK_FD_HW_TIMEOUT)); > /* Reset FD HW */ > if (!ret) { > struct ipi_message fd_ipi_msg; > > fd_ipi_msg.cmd_id = MTK_FD_IPI_CMD_RESET; > if (scp_ipi_send(fd->scp_pdev, SCP_IPI_FD_CMD, &fd_ipi_msg, > sizeof(fd_ipi_msg), MTK_FD_IPI_SEND_TIMEOUT)) > dev_err(fd->dev, "FD Reset HW error\n"); > return -ETIMEDOUT; > } > return 0; > } > > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq) > { > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq); > struct mtk_fd_dev *fd = ctx->fd_dev; > struct vb2_v4l2_buffer *vb; > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx; > struct v4l2_m2m_queue_ctx *queue_ctx; > > mtk_fd_job_abort(fd); > queue_ctx = V4L2_TYPE_IS_OUTPUT(vq->type) ? > &m2m_ctx->out_q_ctx : > &m2m_ctx->cap_q_ctx; > while ((vb = v4l2_m2m_buf_remove(queue_ctx))) > v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR); > > if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > mtk_fd_hw_disconnect(fd); > } > > static int mtk_fd_suspend(struct device *dev) > { > struct mtk_fd_dev *fd = dev_get_drvdata(dev); > > if (pm_runtime_suspended(dev)) > return 0; > > if (fd->fd_stream_count) > if (mtk_fd_job_abort(fd)) > mtk_fd_hw_job_finish(fd, VB2_BUF_STATE_ERROR); Wouldn't this cause the next job to be run? > > /* suspend FD HW */ > writel(0x0, fd->fd_base + MTK_FD_REG_OFFSET_INT_EN); > writel(0x0, fd->fd_base + MTK_FD_REG_OFFSET_HW_ENABLE); > clk_disable_unprepare(fd->fd_clk); > dev_dbg(dev, "%s:disable clock\n", __func__); > > return 0; > } > > > > queue_ctx = V4L2_TYPE_IS_OUTPUT(vq->type) ? > > > &m2m_ctx->out_q_ctx : > > > &m2m_ctx->cap_q_ctx; > > > while ((vb = v4l2_m2m_buf_remove(queue_ctx))) > > > v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR); > > > > > > if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) > > > mtk_fd_hw_disconnect(fd); > > > } > > > > > > If there is no other concern, may I send the RFC v3 patch for review? > > > > Thanks, technically it looks good now. Just one comment about avoiding > > code duplication above. > > > > Thanks, > > I will send the v3 if the above fix-up is accepted, I think there is a bigger issue here actually, related to how the m2m helpers work. Let's just keep the code as you proposed and post v3. We can continue the discussion there. Best regards, Tomasz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel