<html><body><p> <pre> Hi, Shu-hsiang: On Tue, 2024-10-29 at 15:03 +0800, CK Hu wrote: > Hi, Shu-hsiang: > > On Wed, 2024-10-09 at 19:15 +0800, Shu-hsiang Yang wrote: > > 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, and MediaTek-specific features, > > and pipeline management, streaming control, error handling mechanism. > > Additionally, it aggregates sub-drivers for the camera interface, raw > > and yuv pipelines. > > > > Signed-off-by: Shu-hsiang Yang <Shu-hsiang.Yang@mediatek.com> > > --- > > [snip] > > > +struct mtk_cam_device { > > +struct device *dev; > > + > > +struct v4l2_device v4l2_dev; > > +struct v4l2_async_notifier notifier; > > +struct media_device media_dev; > > +void __iomem *base; > > + > > +struct mtk_scp *scp; > > +struct device *smem_dev; > > +phandle rproc_phandle; > > +struct rproc *rproc_handle; > > + > > +unsigned int composer_cnt; > > + > > +unsigned int num_seninf_devices; > > +unsigned int num_raw_devices; > > +unsigned int num_larb_devices; > > + > > +/* raw_pipe controller subdev */ > > +struct mtk_raw raw; > > +struct mutex queue_lock; /* protect queue request */ > > + > > +unsigned int max_stream_num; > > +unsigned int streaming_ctx; > > +unsigned int streaming_pipe; > > +struct mtk_cam_ctx *ctxs; > > + > > +/* request related */ > > +struct list_head pending_job_list; > > +spinlock_t pending_job_lock; /* protect pending_job_list */ > > +struct list_head running_job_list; > > +unsigned int running_job_count; > > +spinlock_t running_job_lock; /* protect running_job_list */ > > + > > +/* standard v4l2 buffer control */ > > +struct list_head dma_pending; > > +spinlock_t dma_pending_lock; /* protect dma_pending_list */ > > +struct list_head dma_processing; > > +spinlock_t dma_processing_lock; /* protect dma_processing_list and dma_processing_count */ > > +unsigned int dma_processing_count; > > I would like scp-related variable has the scp prefix. > > struct list_head scp_dma_processing; > spinlock_t scp_dma_processing_lock; > unsigned int scp_dma_processing_count; > > So it's easy to understand that scp_dma_processing_count's max value is 2. > One buffer is currently doing dma, and another one is waiting for dma. Both buffer are queued in scp. Forget previous comment. After review the buffer control, I think the buffer list should be simplified. dma_pending, dma_processing, using_buffer_list, composed_buffer_list, processing_buffer_list could be merge into one buf_list. The buffer in buf_list has different status. In init, the buffer is queued into driver and status is waiting. buf_list-> buf0(waiting)-> buf1(waiting)-> buf2(waiting)-> buf3(waiting)-> buf4(waiting) In mtk_cam_buf_try_queue(), use scp to generate cq buffer content of buf0 and buf1, so the status is scp_generate_cq. buf_list-> buf0(scp_generate_cq)-> buf1(scp_generate_cq)-> buf2(waiting)-> buf3(waiting)-> buf4(waiting) So the buf_entry is bound to buf0 and buf1, it's not necessary have using_buffer_list, composed_buffer_list, processing_buffer_list to manage buf_entry. In isp_composer_handler_ack(), scp has finish generating cq buffer content, so the status is cq_ready. In the meantime, use scp to generate cq buffer content of buf2. buf_list-> buf0(cq_ready)-> buf1(scp_generate_cq)-> buf2(scp_generate_cq)-> buf3(waiting)-> buf4(waiting) In mtk_camsys_raw_frame_start(), apply cq buffer to hardware, so the status is cq_apply buf_list-> buf0(cq_apply)-> buf1(scp_generate_cq)-> buf2(scp_generate_cq)-> buf3(waiting)-> buf4(waiting) In mtk_camsys_frame_done(), hardware has finished writing video into buffer, so the status is done buf_list-> buf0(done)-> buf1(scp_generate_cq)-> buf2(scp_generate_cq)-> buf3(waiting)-> buf4(waiting) In this design, just need one buf_list with status. The code would be more simple. Simple code would has less bug. Maybe you could drop so many debug utility. Regards, CK > > Regards, > CK > > > + > > +struct mtk_cam_debug_fs *debug_fs; > > +struct workqueue_struct *debug_wq; > > +struct workqueue_struct *debug_exception_wq; > > +wait_queue_head_t debug_exception_waitq; > > +}; > > + > > </pre> </p></body></html><pre>************* MEDIATEK Confidentiality Notice ******************** The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you! </pre>