Re: [PATCH v2 08/13] media: platform: mediatek: add isp_7x camsys unit

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

On Mon, 2025-07-07 at 09:31 +0800, shangyao lin wrote:
> From: "shangyao.lin" <shangyao.lin@mediatek.com>
> 
> Introduces the top media device driver for the MediaTek ISP7X CAMSYS. The driver maintains the camera system, including sub-device management, DMA operations, and integration with the V4L2 framework. It handles request stream data, buffer management, MediaTek-specific features, pipeline management, streaming control, and error handling mechanisms. Additionally, it aggregates sub-drivers for the camera interface, raw, and yuv pipelines.
> 
> ---
> 
> Changes in v2:
> - Removed mtk_cam-ufbc-def.h along with related code
> - Various fixes per review comments
> 
> ---
> 
> [Question for CK]:
> 
> Hi CK,
> 
> Is your suggestion to simplify buffer management to a single buf_list with a buffer status (estate) state machine, so all buffers stay in one list and only the estate field changes (no more multiple lists)? The buffer life cycle would be: WAITING → SCP_GENERATE_CQ → CQ_READY → CQ_APPLY → DONE.
> 
> I have some concerns about concurrency and race conditions:
> 
> Multiple lists can use different locks to reduce contention.
> With a single list, all state changes need to lock the same list, which may require more careful lock design to avoid race conditions or deadlocks.
> When multiple threads (interrupts, workqueues, user requests) operate at the same time, synchronization becomes more difficult.
> Do you have any reference code or best practices for this kind of design?

Deadlock happen only when code hold two lock at the same time.
I think when you hold buf_list lock, it's not necessary to hold other lock, so it's not necessary to worry about deadlock.
The buffer management in this driver is simple.
All the buffer is processed step by step in order.
I have no sample code to provide to you.
You may point out the worry code in this patch and we could discuss on that point.

> 
> Thanks!
> 
> 
> 
> Reply regarding stream_id and pipe_id naming:
> 
> Hi CK,
> 
> Thank you for your suggestion. In our current design, stream_id and pipe_id are not always in a 1-to-1 relationship. In some cases, one stream may correspond to multiple pipes, or vice versa. That’s why we use both names in different contexts. If you have any suggestions on how to unify the naming while keeping this distinction clear, please let us know.
> 

So stream_id and pipe_id are different concept.
Below is the code in this patch.

+struct mtk_cam_request_stream_data*
+mtk_cam_get_req_s_data(struct mtk_cam_ctx *ctx, unsigned int pipe_id,
+                        unsigned int frame_seq_no)
 
+struct mtk_cam_request *mtk_cam_get_req(struct mtk_cam_ctx *ctx,
+                                              unsigned int frame_seq_no)
+{
+       struct mtk_cam_request_stream_data *req_stream_data;
+
+       req_stream_data = mtk_cam_get_req_s_data(ctx, ctx->stream_id, frame_seq_no);

The second parameter of mtk_cam_get_req_s_data() is pipe_id, but you pass stream_id into it.
Because stream_id and pipe_id are different concept, so you should not do this.
You should pass pipe_id into mtk_cam_get_req_s_data().

Regards,
CK

+       if (!req_stream_data)
+                 return NULL;
+
+       return req_stream_data->req;
+}


> Thanks!
> 
> Best regards,
> Shangyao
> 
> Signed-off-by: shangyao.lin <shangyao.lin@mediatek.com>
> ---