On Wed, Aug 7, 2019 at 11:11 AM Jungo Lin [off-list ref] wrote:

Hi, Tomasz:

On Tue, 2019-08-06 at 18:47 +0900, Tomasz Figa wrote:

quoted

Hi Jungo,

On Fri, Jul 26, 2019 at 4:24 PM Jungo Lin [off-list ref] wrote:

quoted

Hi, Tomasz:

On Thu, 2019-07-25 at 18:23 +0900, Tomasz Figa wrote:

quoted

.Hi Jungo,

On Sat, Jul 20, 2019 at 6:58 PM Jungo Lin [off-list ref] wrote:

quoted

Hi, Tomasz:

On Wed, 2019-07-10 at 18:56 +0900, Tomasz Figa wrote:

quoted

Hi Jungo,

On Tue, Jun 11, 2019 at 11:53:42AM +0800, Jungo Lin wrote:

[snip]

I just keep some questions to be clarified.
[snip]

quoted

quoted

quoted

quoted

quoted

quoted

+           isp_dev->meta0_vb2_index = meta0_vb2_index;
+           isp_dev->meta1_vb2_index = meta1_vb2_index;
+   } else {
+           if (irq_status & SOF_INT_ST) {
+                   isp_dev->current_frame = hw_frame_num;
+                   isp_dev->meta0_vb2_index = meta0_vb2_index;
+                   isp_dev->meta1_vb2_index = meta1_vb2_index;
+           }
+           irq_handle_notify_event(isp_dev, irq_status, dma_status, 1);
+   }

The if and else blocks do almost the same things just in different order. Is
it really expected?

If we receive HW_PASS1_DON_ST & SOF_INT_ST IRQ events at the same time,
the correct sequence should be handle HW_PASS1_DON_ST firstly to check
any de-queued frame and update the next frame setting later.
Normally, this is a corner case or system performance issue.

So it sounds like HW_PASS1_DON_ST means that all data from current
frame has been written, right? If I understand your explanation above
correctly, that would mean following handling of each interrupt:

HW_PASS1_DON_ST:
 - CQ executes with next CQ buffer to prepare for next frame. <- how
is this handled? does the CQ hardware automatically receive this event
from the ISP hadware?
 - return VB2 buffers,
 - complete requests.

SOF_INT_ST:
 - send VSYNC event to userspace,
 - program next CQ buffer to CQ,

SW_PASS1_DON_ST:
 - reclaim CQ buffer and enqueue next frame to composing if available

Sorry for our implementation of HW_PASS1_DON_ST.
It is confusing.
Below is the revised version based on your conclusion.
So in our new implemmenation, we just handle SOF_INT_ST &
SW_PASS1_DON_ST events. We just add one warning message for
HW_PASS1_DON_ST

HW_PASS1_DON_ST:
- CQ executes with next CQ buffer to prepare for next frame.

SOF_INT_ST:
- send VSYNC event to userspace,
- program next CQ buffer to CQ,

SW_PASS1_DON_ST:
- reclaim CQ buffer and enqueue next frame to composing if available
- return VB2 buffers,
- complete requests.

For CQ HW operations, it is listed below:

a. The CQ buffer has two kinds of information
 - Which ISP registers needs to be updated.
 - Where the corresponding ISP register data to be read.
b. The CQ buffer loading procedure is triggered by HW_PASS1_DONT_ST IRQ
event periodically.
 - Normally, if the ISP HW receives the completed frame and it will
trigger W_PASS1_DONT_ST IRQ and perform CQ buffer loading immediately.
-  So the CQ buffer loading is performed by ISP HW automatically.
c. The ISP HW will read CQ base address register(REG_CQ_THR0_BASEADDR)
to decide which CQ buffer is loaded.
   - So we configure the next CQ base address in SOF.
d. For CQ buffer loading, CQ will read the ISP registers from CQ buffer
and update the ISP register values into HW.
   - SCP composer will compose one dummy CQ buffer and assign it to
REG_CQ_THR0_BASEADDR of each CQ buffer.
   - Dummy CQ buffer has no updated ISP registers comparing with other
CQ buffers.
   - With this design, if there is no updated new CQ buffer by driver
which may be caused no en-queue frames from user space. The CQ HW will
load dummy CQ buffer and do nothing.

Does the set of registers programmed by CQ include destination buffer
addresses to? If yes, we would end up overwriting previous frames if
no new buffers are provided.

Yes, the buffer addresses are changed per frame request. We need to
compose CQ to include these DMA destination addresses. For your concern,
we have DMA flow buffer control (FBC) in HW. If there is no FBC counter
increased due to no buffer for each DMA, the ISP HW doesn't output the
data to the corresponding DMA address.

Below is the simple descriptor of CQ buffer.
a. ISP registers in tuning buffer, including 3A registers.
b. All capture buffers informations.
   - DMA buffer destination address
   - FBC counter
c. Some specif ISP registers for meta DMAs, such as LCE or LMVO.
d. frame sequence number register

Okay, with the FBC counter it sounds fine. Thanks for clarifying.

quoted

quoted

f. The CQ buffer loading is guaranteed by HW to finish before the next
SOF.

Okay, thanks a lot for the explanation. This is much more clear now.

[snip]

quoted

quoted

quoted

quoted

quoted

+static const struct dev_pm_ops mtk_isp_pm_ops = {
+   SET_SYSTEM_SLEEP_PM_OPS(mtk_isp_suspend, mtk_isp_resume)
+   SET_RUNTIME_PM_OPS(mtk_isp_suspend, mtk_isp_resume, NULL)

For V4L2 drivers system and runtime PM ops would normally be completely
different. Runtime PM ops would be called when the hardware is idle already
or is about to become active. System PM ops would be called at system power
state change and the hardware might be both idle or active. Please also see
my comments to mtk_isp_suspend() and mtk_isp_resume() above.

Here is the new implementation. It should be clear to show the
difference between system and runtime PM ops.

static const struct dev_pm_ops mtk_isp_pm_ops = {
        SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
                                pm_runtime_force_resume)
        SET_RUNTIME_PM_OPS(mtk_isp_runtime_suspend, mtk_isp_runtime_resume,
NULL)
};

That's still not correct. In runtime suspend/resume ops we already are
not streaming anymore, because we call pm_runtime_get/put_*() when
starting and stopping streaming. In system suspend/resume ops we might
be streaming and that's when we need to stop the hardware and wait for
it to finish. Please implement these ops separately.

Best regards,
Tomasz

Ok, got your point.
Below is the new implementation for your review.

static int mtk_isp_pm_suspend(struct device *dev)
{
        struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(dev);
        u32 val;
        int ret;

        dev_dbg(dev, "- %s\n", __func__);

        /* Check ISP is streaming or not */
        if (!p1_dev->cam_dev.streaming)
                goto done;

We would normally check here for pm_runtime_suspended(). Although they
both should be equivalent. Still, there is no need to call
pm_runtime_force_suspend() if the latter is true, so we could just
return 0 instantly.

Ok, here is the fixed version.

static int mtk_isp_pm_suspend(struct device *dev)
{
        struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(dev);
        u32 val;
        int ret;

        dev_dbg(dev, "- %s\n", __func__);

        if (pm_runtime_suspended(dev))
                return 0;

        /* Disable ISP's view finder and wait for TG idle */
        dev_dbg(dev, "cam suspend, disable VF\n");
        val = readl(p1_dev->regs + REG_TG_VF_CON);
        writel(val & (~TG_VF_CON_VFDATA_EN), p1_dev->regs + REG_TG_VF_CON);
        ret = readl_poll_timeout_atomic(p1_dev->regs + REG_TG_INTER_ST, val,
                                        (val & TG_CS_MASK) == TG_IDLE_ST,
                                        USEC_PER_MSEC, MTK_ISP_STOP_HW_TIMEOUT);
        if (ret)
                dev_warn(dev, "can't stop HW:%d:0x%x\n", ret, val);

What happens in this case? Is it safe to continue?

        /* Disable CMOS */
        val = readl(p1_dev->regs + REG_TG_SEN_MODE);
        writel(val & (~TG_SEN_MODE_CMOS_EN), p1_dev->regs + REG_TG_SEN_MODE);

        /* Force ISP HW to idle */
        ret = pm_runtime_force_suspend(dev);
        if (ret)
                return ret;

We should probably reenable the hardware if the above failed, so that
we hopefully end up in the same state as before the suspend.

        return 0;
}
[snip]

quoted

quoted

static int mtk_isp_pm_resume(struct device *dev)
{
        struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(dev);
        u32 val;
        int ret;

        dev_dbg(dev, "- %s\n", __func__);

        /* Force ISP HW to resume if needed */
        ret = pm_runtime_force_resume(dev);
        if (ret)
                return ret;

We should do this conditionally based on what pm_runtime_suspended()
returns. If it's non-zero then we can just return 0 instantly.

Ok, here is the fixed version.

static int mtk_isp_pm_resume(struct device *dev)
{
        struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(dev);
        u32 val;
        int ret;

        dev_dbg(dev, "- %s\n", __func__);

        if (pm_runtime_suspended(dev))
                return 0;

        /* Force ISP HW to resume */
        ret = pm_runtime_force_resume(dev);
        if (ret)
                return ret;

        /* Enable CMOS */
        dev_dbg(dev, "cam resume, enable CMOS/VF\n");
        val = readl(p1_dev->regs + REG_TG_SEN_MODE);
        writel(val | TG_SEN_MODE_CMOS_EN, p1_dev->regs + REG_TG_SEN_MODE);

        /* Enable VF */
        val = readl(p1_dev->regs + REG_TG_VF_CON);
        writel(val | TG_VF_CON_VFDATA_EN, p1_dev->regs + REG_TG_VF_CON);

        return 0;
}

[snip]

quoted

quoted

static int mtk_isp_runtime_suspend(struct device *dev)
{
        struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(dev);

        dev_dbg(dev, "- %s\n", __func__);

        if (pm_runtime_suspended(dev))
                return 0;

Sorry, I guess I wasn't clear in my reply. It's not possible to get
this callback called if the device is already runtime suspended.

Ok, got it. Need to remove pm_runtime_suspended(dev) checking and move
it into mtk_isp_pm_* functions. If I still don't get your point, could
you kindly provide one sample driver for reference?

The above implementation is okay, thanks. :)

Based on current
implementation, it is similar to below drivers.
https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/mtk-mdp/mtk_mdp_core.c#L255
https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/exynos4-is/fimc-is-i2c.c#L113

The first one is an m2m device so it has slightly different rules -
the runtime PM is allowed to suspend between frames if the idle time
is long enough. The second one is a dummy driver for some fake i2c
bus, so it doesn't really have any meaningful implementation.

I think you could take a look at
https://elixir.bootlin.com/linux/v5.3-rc3/source/drivers/media/platform/exynos4-is/fimc-lite.c#L1550
, which is an online capture device too.

static int mtk_isp_runtime_suspend(struct device *dev)
{
        struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(dev);

        dev_dbg(dev, "%s:disable clock\n", __func__);
        clk_bulk_disable_unprepare(p1_dev->num_clks, p1_dev->clks);

        return 0;
}

[snip]

quoted

quoted

static int mtk_isp_runtime_resume(struct device *dev)
{
        struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(dev);
        int ret;

        dev_dbg(dev, "- %s\n", __func__);

        if (pm_runtime_suspended(dev))
                return 0;

In this case the above call would always return non-zero, so the
behavior wouldn't be very good.

Same as above.

static int mtk_isp_runtime_resume(struct device *dev)
{
        struct mtk_isp_p1_device *p1_dev = dev_get_drvdata(dev);
        int ret;

        dev_dbg(dev, "%s:enable clock\n", __func__);
        ret = clk_bulk_prepare_enable(p1_dev->num_clks, p1_dev->clks);
        if (ret) {
                dev_err(dev, "failed to enable clock:%d\n", ret);
                return ret;
        }

        return 0;
}

Makes sense, thanks!

Best regards,
Tomasz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel