Re: [PATCH v8 4/5] media: platform: mediatek: isp: add mediatek ISP3.0 camsv

["CK Hu (胡俊光)" <ck.hu@mediatek.com>]

Hi, Julien:

On Wed, 2025-01-22 at 14:59 +0100, Julien Stephan wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> From: Phi-bang Nguyen <pnguyen@baylibre.com>
> 
> This driver provides a path to bypass the SoC ISP so that image data
> coming from the SENINF can go directly into memory without any image
> processing. This allows the use of an external ISP.
> 
> Signed-off-by: Phi-bang Nguyen <pnguyen@baylibre.com>
> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> [Paul Elder fix irq locking]
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Co-developed-by: Julien Stephan <jstephan@baylibre.com>
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> ---

[snip]

> +
> +/*
> + * Camsv module is able to handle underrruns using the FBC
> + *
> + *     - FBC_FB_NUM: the number of userspace buffers available (fifo depth)
> + *     - RCNT_INC: user writes 1 when buffer is queued/unqueued
> + *     - FBC_CNT: decreased at SOF indicating that the number of buffers filled
> + *       by write-DMA decreases. Increased when write-dma done indicating that
> + *       the number of buffers filled by write-DMA increases
> + *     - RCNT: increased at the end of enqueue indicating that the software
> + *       moves to the next buffer for reading
> + *     - WCNT: increased to indicate that write-dma writes to the next buffer
> + *     Drop condition: FBC_CNT == FBC_NUM

The description more confuse me.
Below is what I guess:

RCNT_INT: This is used to increase 1 of RCNT
RCNT: Read pointer which point to the last software prepared buffer count
WCNT: Write pointer which point to the hardware last write buffer count
Drop condition: WCNT == RCNT

My guess is totally different with your description.
So let me ask question about your description.

RCNT_INC: user writes 1 when buffer is queued/unqueued
=>
Describe what it actually do, does it increase any counter? Which counter? FBC_CNT or RCNT?
From the naming, I guess this increase RCNT by 1.


- FBC_CNT: decreased at SOF indicating that the number of buffers filled
       by write-DMA decreases. Increased when write-dma done indicating that
       the number of buffers filled by write-DMA increases
=>
FBC_CNT decrease when SOF, and increase when write-dma done.
If FBC_CNT is 5 initially, it would change to 4, 5, 4, 5, 4, 5 periodically.
When SOF, it change to 4. When frame done, it change to 5.
It seems that this counter could not be used to control drop or not because software could not control it.

Drop condition: FBC_CNT == FBC_NUM
=>
If RCNT_INC is to increase FBC_CNT, FBC_CNT would finally be larger then FBC_NUM, this is weird.
If RCNT_INT is to increase RCNT, and sotfware could not control FBC_CNT and FBC_NUM, how hardware would drop correctly?

Regards,
CK

> + */
> +
> +void mtk_camsv_fbc_init(struct mtk_cam_dev *cam_dev,
> +                                unsigned int num_buffers)
> +{
> +       unsigned int fbc_val;
> +
> +       if (pm_runtime_resume_and_get(cam_dev->dev) < 0) {
> +               dev_err(cam_dev->dev, "failed to get pm_runtime\n");
> +               return;
> +       }
> +
> +       fbc_val = mtk_camsv_read(cam_dev, CAMSV_IMGO_FBC);
> +       fbc_val &= ~CAMSV_IMGO_FBC_FB_NUM;
> +       fbc_val |= CAMSV_IMGO_FBC_EN;
> +       fbc_val |= FIELD_PREP(CAMSV_IMGO_FBC_FB_NUM, num_buffers);
> +       mtk_camsv_write(cam_dev, CAMSV_IMGO_FBC, fbc_val);
> +
> +       pm_runtime_mark_last_busy(cam_dev->dev);
> +       pm_runtime_put_autosuspend(cam_dev->dev);
> +}
> +
>