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