Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Tomasz Figa <tfiga@chromium.org>
To: Jerry-ch Chen <Jerry-ch.Chen@mediatek.com>
Cc: "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"Sean Cheng (鄭昇弘)" <Sean.Cheng@mediatek.com>,
	"laurent.pinchart+renesas@ideasonboard.com"
	<laurent.pinchart+renesas@ideasonboard.com>,
	"Rynn Wu (吳育恩)" <Rynn.Wu@mediatek.com>,
	srv_heupstream <srv_heupstream@mediatek.com>,
	"Po-Yang Huang (黃柏陽)" <po-yang.huang@mediatek.com>,
	"mchehab@kernel.org" <mchehab@kernel.org>,
	"suleiman@chromium.org" <suleiman@chromium.org>,
	"shik@chromium.org" <shik@chromium.org>,
	"Jungo Lin (林明俊)" <jungo.lin@mediatek.com>,
	"Sj Huang (黃信璋)" <sj.huang@mediatek.com>,
	"yuzhao@chromium.org" <yuzhao@chromium.org>,
	"linux-mediatek@lists.infradead.org"
	<linux-mediatek@lists.infradead.org>,
	"zwisler@chromium.org" <zwisler@chromium.org>,
	"matthias.bgg@gmail.com" <matthias.bgg@gmail.com>,
	"Christie Yu (游雅惠)" <christie.yu@mediatek.com>,
	"Frederic Chen (陳俊元)" <Frederic.Chen@mediatek.com>,
	"hans.verkuil@cisco.com" <hans.verkuil@cisco.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>
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: <CAAFQd5D2ketE19RPr20BVYGhqg2Lh2ZNTtAr5J2GoWU9RiSAsA@mail.gmail.com> (raw)
In-Reply-To: <1567671418.22453.41.camel@mtksdccf07>

On Thu, Sep 5, 2019 at 5:17 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> 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 <Jerry-ch.Chen@mediatek.com> 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 <Jerry-ch.Chen@mediatek.com> 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 <Jerry-ch.Chen@mediatek.com> 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 <Jerry-ch.Chen@mediatek.com> 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 <Jerry-ch.Chen@mediatek.com> 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
> > > > > > > > > > > > > <Jerry-ch.Chen@mediatek.com> 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 <Jerry-ch.Chen@mediatek.com> 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 <Jerry-ch.Chen@mediatek.com> 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 <Jerry-ch.Chen@mediatek.com> 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
> > > > > > > > > > > > > > > > > > > > > <Jerry-ch.Chen@mediatek.com> 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
> > > > > > > > > > > > > > > > > > > > > > > <Jerry-ch.Chen@mediatek.com> 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

  reply index

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-09  8:41 [RFC PATCH V2 0/4] media: platform: Add support for Face Detection (FD) on mt8183 SoC Jerry-ch Chen
2019-07-09  8:41 ` [RFC PATCH V2 1/4] dt-bindings: mt8183: Added FD dt-bindings Jerry-ch Chen
2019-07-12  7:16   ` CK Hu
2019-07-09  8:41 ` [RFC PATCH V2 2/4] dts: arm64: mt8183: Add FD nodes Jerry-ch Chen
2019-07-09  8:41 ` [RFC PATCH V2 3/4] media: platform: Add Mediatek FD driver KConfig Jerry-ch Chen
2019-09-05 12:30   ` Laurent Pinchart
2019-09-05 14:14     ` Jerry-ch Chen
2019-07-09  8:41 ` [RFC PATCH V2 4/4] platform: mtk-isp: Add Mediatek FD driver Jerry-ch Chen
2019-07-09 10:56   ` Enrico Weigelt, metux IT consult
2019-07-29  6:01     ` Jerry-ch Chen
2019-07-29  9:57       ` Tomasz Figa
2019-07-29 11:58         ` Jerry-ch Chen
2019-08-09  8:07           ` Tomasz Figa
2019-09-05 12:35             ` Laurent Pinchart
2019-08-02  8:28   ` Tomasz Figa
2019-08-25  9:18     ` Jerry-ch Chen
2019-08-26  6:36       ` Tomasz Figa
2019-08-28  2:00         ` Jerry-ch Chen
2019-08-30  8:33           ` Tomasz Figa
2019-09-02 11:47             ` Jerry-ch Chen
2019-09-03  5:19               ` Tomasz Figa
2019-09-03  6:44                 ` Jerry-ch Chen
2019-09-03  7:04                   ` Tomasz Figa
2019-09-03 11:46                     ` Jerry-ch Chen
2019-09-03 12:05                       ` Tomasz Figa
2019-09-04  3:38                         ` Jerry-ch Chen
2019-09-04  4:15                           ` Tomasz Figa
2019-09-04  6:09                             ` Jerry-ch Chen
2019-09-04  6:34                               ` Tomasz Figa
2019-09-04  8:09                                 ` Jerry-ch Chen
2019-09-04  8:25                                   ` Tomasz Figa
2019-09-04  9:01                                     ` Jerry-ch Chen
2019-09-04  9:03                                       ` Tomasz Figa
2019-09-04  9:26                                         ` Jerry-ch Chen
2019-09-04 13:12                                           ` Tomasz Figa
2019-09-04 13:19                                             ` Jerry-ch Chen
2019-09-05  7:02                                               ` Jerry-ch Chen
2019-09-05  7:13                                                 ` Tomasz Figa
2019-09-05  8:16                                                   ` Jerry-ch Chen
2019-09-05  8:52                                                     ` Tomasz Figa [this message]
2019-09-05 13:36                                                       ` Jerry-ch Chen

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAAFQd5D2ketE19RPr20BVYGhqg2Lh2ZNTtAr5J2GoWU9RiSAsA@mail.gmail.com \
    --to=tfiga@chromium.org \
    --cc=Frederic.Chen@mediatek.com \
    --cc=Jerry-ch.Chen@mediatek.com \
    --cc=Rynn.Wu@mediatek.com \
    --cc=Sean.Cheng@mediatek.com \
    --cc=christie.yu@mediatek.com \
    --cc=devicetree@vger.kernel.org \
    --cc=hans.verkuil@cisco.com \
    --cc=jungo.lin@mediatek.com \
    --cc=laurent.pinchart+renesas@ideasonboard.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=mchehab@kernel.org \
    --cc=po-yang.huang@mediatek.com \
    --cc=shik@chromium.org \
    --cc=sj.huang@mediatek.com \
    --cc=srv_heupstream@mediatek.com \
    --cc=suleiman@chromium.org \
    --cc=yuzhao@chromium.org \
    --cc=zwisler@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git