| Message ID | 1549348966-14451-1-git-send-email-frederic.chen@mediatek.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: platform: Add support for ISP Pass 1 on mt8183 SoC | expand |
(() . ( strHi Frederic, Jungo, On Tue, Feb 5, 2019 at 3:43 PM Frederic Chen <frederic.chen@mediatek.com> wrote: > > From: Jungo Lin <jungo.lin@mediatek.com> > > This patch adds the driver for Pass unit in Mediatek's camera > ISP system. Pass 1 unit is embedded in Mediatek SOCs. It > provides RAW processing which includes optical black correction, > defect pixel correction, W/IR imbalance correction and lens > shading correction. > > The mtk-isp directory will contain drivers for multiple IP > blocks found in Mediatek ISP system. It will include ISP Pass 1 > driver, sensor interface driver, DIP driver and face detection > driver. Thanks for the patches! Please see my comments inline. [snip] > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-ctx.h b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-ctx.h > new file mode 100644 > index 0000000..11a60a6 > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-ctx.h > @@ -0,0 +1,327 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2018 MediaTek Inc. > + * Author: Frederic Chen <frederic.chen@mediatek.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#ifndef __MTK_CAM_CTX_H__ > +#define __MTK_CAM_CTX_H__ > + > +#include <linux/types.h> > +#include <linux/videodev2.h> > +#include <media/v4l2-ctrls.h> > +#include <media/videobuf2-core.h> > +#include <media/v4l2-subdev.h> > +#include "mtk_cam-v4l2-util.h" > + > +#define MTK_CAM_CTX_QUEUES (16) > +#define MTK_CAM_CTX_FRAME_BUNDLE_BUFFER_MAX (MTK_CAM_CTX_QUEUES) > +#define MTK_CAM_CTX_DESC_MAX (MTK_CAM_CTX_QUEUES) > + > +#define MTK_CAM_CTX_MODE_DEBUG_OFF (0) > +#define MTK_CAM_CTX_MODE_DEBUG_BYPASS_JOB_TRIGGER (1) > +#define MTK_CAM_CTX_MODE_DEBUG_BYPASS_ALL (2) nit: No need for parentheses for simple values. Also please align macro values together using tabs. > + > +#define MTK_CAM_GET_CTX_ID_FROM_SEQUENCE(sequence) \ > + ((sequence) >> 16 & 0x0000FFFF) Sounds like a job for a static inline function, with appropriate argument and return types enforced by the compiler. > + > +#define MTK_CAM_CTX_META_BUF_DEFAULT_SIZE (1110 * 1024) > + > +struct mtk_cam_ctx; > +struct mtk_cam_ctx_open_param; > +struct mtk_cam_ctx_release_param; > +struct mtk_cam_ctx_streamon_param; > +struct mtk_cam_ctx_streamoff_param; > +struct mtk_cam_ctx_start_param; > +struct mtk_cam_ctx_finish_param; > + > +/* struct mtk_cam_ctx_ops - background hardware driving ops */ > +/* sdefines background driver specific callback APIs */ > +struct mtk_cam_ctx_ops { > + int (*open)(struct mtk_cam_ctx *dev_ctx, > + struct mtk_cam_ctx_open_param *param); > + int (*release)(struct mtk_cam_ctx *dev_ctx, > + struct mtk_cam_ctx_release_param *param); > + int (*start)(struct mtk_cam_ctx *dev_ctx, > + struct mtk_cam_ctx_start_param *param); > + int (*finish)(struct mtk_cam_ctx *dev_ctx, > + struct mtk_cam_ctx_finish_param *param); > + int (*streamon)(struct mtk_cam_ctx *dev_ctx, > + struct mtk_cam_ctx_streamon_param *param); > + int (*streamoff)(struct mtk_cam_ctx *dev_ctx, > + struct mtk_cam_ctx_streamoff_param *param); > +}; We only have one driver here and these ops look really close to what vb2 already provides. Why do we need this indirection? > + > +/* Attributes setup by device context owner */ > +struct mtk_cam_ctx_queue_desc { > + int id; /* id of the context queue */ Please use kerneldoc syntax for documenting the structures. (https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txtjjj > + char *name; > + /* Will be exported to media entity name */ > + int capture; > + /* true for capture queue (device to user), false for output queue */ > + /* (from user to device) */ > + int image; > + /* Using the cam_smem_drv as alloc ctx or not */ > + int smem_alloc; > + /* true for image, false for meta data */ > + unsigned int dma_port; /*The dma port associated to the buffer*/ > + /* Supported format */ > + struct mtk_cam_ctx_format *fmts; > + int num_fmts; > + /* Default format of this queue */ > + int default_fmt_idx; > +}; > + > +/* Supported format and the information used for */ > +/* size calculation */ CodingStyle: /* * This is the kernel style for multi-line * comments. */ > +struct mtk_cam_ctx_meta_format { > + u32 dataformat; > + u32 max_buffer_size; > + u8 flags; > +}; > + > +struct mtk_cam_ctx_img_format { > + u32 pixelformat; > + u8 depth[VIDEO_MAX_PLANES]; > + u8 row_depth[VIDEO_MAX_PLANES]; > + u8 num_planes; > + u32 flags; > +}; > + > +struct mtk_cam_ctx_format { > + union { > + struct mtk_cam_ctx_meta_format meta; > + struct mtk_cam_ctx_img_format img; > + } fmt; > +}; > + > +union mtk_v4l2_fmt { > + struct v4l2_pix_format_mplane pix_mp; > + struct v4l2_meta_format meta; > +}; Why not just use the standard v4l2_format? > + > +/* Attributes setup by device context owner */ > +struct mtk_cam_ctx_queues_setting { > + int master; > + /* The master input node to trigger the frame data enqueue */ > + struct mtk_cam_ctx_queue_desc *output_queue_descs; > + int total_output_queues; > + struct mtk_cam_ctx_queue_desc *capture_queue_descs; > + int total_capture_queues; > +}; > + > +struct mtk_cam_ctx_queue_attr { > + int master; > + int input_offset; > + int total_num; > +}; > + > +/* Video node context. Since we use */ > +/* mtk_cam_ctx_frame_bundle to manage enqueued */ > +/* buffers by frame now, we don't use bufs filed of */ > +/* mtk_cam_ctx_queue now */ > +struct mtk_cam_ctx_queue { > + union mtk_v4l2_fmt fmt; > + struct mtk_cam_ctx_format *ctx_fmt; > + /* Currently we used in standard v4l2 image format */ > + /* in the device context */ > + unsigned int width_pad; /* bytesperline, reserved */ What's the meaning of this field? > + struct mtk_cam_ctx_queue_desc desc; > + struct list_head bufs; /* Reserved, not used now */ > +}; > + > +enum mtk_cam_ctx_frame_bundle_state { > + MTK_CAM_CTX_FRAME_NEW, /* Not allocated */ > + MTK_CAM_CTX_FRAME_PREPARED, /* Allocated but has not be processed */ > + MTK_CAM_CTX_FRAME_PROCESSING, /* Queued, waiting to be filled */ > +}; Feels like duplicating the media_request::state. (and eliminating the benefit of the state machine being fully managed by the Request API core). > + > +/* The definiation is compatible with DIP driver's state definiation */ > +/* currently and will be decoupled after further integration */ > +enum mtk_cam_ctx_frame_data_state { > + MTK_CAM_CTX_FRAME_DATA_EMPTY = 0, /* FRAME_STATE_INIT */ > + MTK_CAM_CTX_FRAME_DATA_DONE = 3, /* FRAME_STATE_DONE */ > + MTK_CAM_CTX_FRAME_DATA_STREAMOFF_DONE = 4, /*FRAME_STATE_STREAMOFF*/ > + MTK_CAM_CTX_FRAME_DATA_ERROR = 5, /*FRAME_STATE_ERROR*/ > +}; What are these states for? > + > +struct mtk_cam_ctx_frame_bundle { > + struct mtk_cam_ctx_buffer* > + buffers[MTK_CAM_CTX_FRAME_BUNDLE_BUFFER_MAX]; > + int id; > + int num_img_capture_bufs; > + int num_img_output_bufs; > + int num_meta_capture_bufs; > + int num_meta_output_bufs; > + int last_index; > + int state; > + struct list_head list; > +}; > + > +struct mtk_cam_ctx_frame_bundle_list { > + struct list_head list; > +}; This sounds like duplicating the request API core functionality, which aggregates all the buffers in the request. > + > +struct mtk_cam_ctx { > + struct platform_device *pdev; > + struct platform_device *smem_device; > + /* buffer queues will be added later */ > + unsigned short ctx_id; > + char *device_name; > + const struct mtk_cam_ctx_ops *ops; > + struct mtk_cam_dev_node_mapping *mtk_cam_dev_node_map; > + unsigned int dev_node_num; > + /* mtk_cam_ctx_queue is the context for the video nodes */ > + struct mtk_cam_ctx_queue queue[MTK_CAM_CTX_QUEUES]; > + struct mtk_cam_ctx_queue_attr queues_attr; > + atomic_t frame_param_sequence; > + int streaming; > + void *default_vb2_alloc_ctx; > + void *smem_vb2_alloc_ctx; > + struct v4l2_subdev_fh *fh; > + struct mtk_cam_ctx_frame_bundle frame_bundles[VB2_MAX_FRAME]; > + struct mtk_cam_ctx_frame_bundle_list processing_frames; > + struct mtk_cam_ctx_frame_bundle_list free_frames; > + int enabled_dma_ports; > + int num_frame_bundle; > + spinlock_t qlock; /* frame queues protection */ > +}; > + > +enum mtk_cam_ctx_buffer_state { > + MTK_CAM_CTX_BUFFER_NEW, > + MTK_CAM_CTX_BUFFER_PROCESSING, > + MTK_CAM_CTX_BUFFER_DONE, > + MTK_CAM_CTX_BUFFER_FAILED, > +}; Why these custom states? VB2 already manages buffer states for you and the driver should just obtain available buffers in the .buf_queue callback and return them with appropriate state using vb2_buffer_done() when they are no longer in use. > + > +struct mtk_cam_ctx_buffer { > + union mtk_v4l2_fmt fmt; > + struct mtk_cam_ctx_format *ctx_fmt; > + int capture; > + int image; > + int frame_id; > + int user_sequence; /* Sequence number assigned by user */ What's this user_sequence? There shouldn't be any sequence coming from the user, since the requests are used to aggregate buffers. > + dma_addr_t daddr; > + void *vaddr; We shouldn't need to touch the buffers from the kernel driver. > + phys_addr_t paddr; We shouldn't need physical address either. I suppose this is for the SCP, but then it should be a DMA address obtained from dma_map_*() with struct device pointer of the SCP. > + unsigned int queue; > + enum mtk_cam_ctx_buffer_state state; > + struct list_head list; > +}; > + > +struct mtk_cam_ctx_setting { > + struct mtk_cam_ctx_ops *ops; > + char *device_name; > +}; > + > +struct mtk_cam_ctx_desc { > + char *proc_dev_phandle; > + /* The context device's compatble string name in device tree*/ > + int (*init)(struct mtk_cam_ctx *ctx); > + /* configure the core functions of the device context */ > +}; > + > +struct mtk_cam_ctx_init_table { > + int total_dev_ctx; > + struct mtk_cam_ctx_desc *ctx_desc_tbl; > +}; > + > +struct mtk_cam_ctx_open_param { > + /* Bitmask used to notify that the DMA port is enabled or not */ > + unsigned int enabled_dma_ports; > +}; > + > +struct mtk_cam_ctx_streamon_param { > + unsigned int enabled_dma_ports; > +}; > + > +struct mtk_cam_ctx_streamoff_param { > + unsigned int enabled_dma_ports; > +}; Hmm, the 3 structs above are identical. Is there a need to have all 3 of them? > + > +struct mtk_cam_ctx_start_param { > + /* carry buffer information of the frame */ > + struct mtk_cam_ctx_frame_bundle *frame_bundle; > +}; > + > +struct mtk_cam_ctx_release_param { > + unsigned int enabled_dma_ports; > +}; One more identical struct. [snip] > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-dev-ctx-core.c b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-dev-ctx-core.c > new file mode 100644 > index 0000000..7d0197b > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-dev-ctx-core.c > @@ -0,0 +1,986 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018 MediaTek Inc. > + * Author: Frederic Chen <frederic.chen@mediatek.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/device.h> > +#include <linux/platform_device.h> > +#include <media/videobuf2-dma-contig.h> > +#include <linux/dma-mapping.h> > +#include <media/v4l2-event.h> > + > +#include "mtk_cam-dev.h" > +#include "mtk_cam-v4l2-util.h" > +#include "mtk_cam-v4l2.h" > +#include "mtk_cam-smem.h" > + > +static struct mtk_cam_ctx_format *mtk_cam_ctx_find_fmt > + (struct mtk_cam_ctx_queue *queue, > + u32 format); > + > +static int mtk_cam_ctx_process_frame(struct mtk_cam_ctx *dev_ctx, > + struct mtk_cam_ctx_frame_bundle > + *frame_bundle); > + > +static int mtk_cam_ctx_free_frame(struct mtk_cam_ctx *dev_ctx, > + struct mtk_cam_ctx_frame_bundle > + *frame_bundle); > + > +static struct mtk_cam_ctx_frame_bundle *mtk_cam_ctx_get_free_frame > + (struct mtk_cam_ctx *dev_ctx); > + > +static struct mtk_cam_ctx_frame_bundle *mtk_cam_ctx_get_processing_frame > +(struct mtk_cam_ctx *dev_ctx, int frame_id); > + > +static int mtk_cam_ctx_init_frame_bundles(struct mtk_cam_ctx *dev_ctx); > + > +static void mtk_cam_ctx_queue_event_frame_done > + (struct mtk_cam_ctx *dev_ctx, > + struct mtk_cam_dev_frame_done_event_data *fdone); > + > +static void debug_bundle(struct mtk_cam_ctx *dev_ctx, > + struct mtk_cam_ctx_frame_bundle *bundle_data); Could you reorder the functions in this file so that there is no need for forward declarations? In general, a kernel source file should be ordered in such way that the reader can understand what's happening by reading from the top to bottom, without jumping between functions. > + > +struct vb2_v4l2_buffer *mtk_cam_ctx_buffer_get_vb2_v4l2_buffer > +(struct mtk_cam_ctx_buffer *ctx_buf) > +{ > + struct mtk_cam_dev_buffer *dev_buf = NULL; > + > + if (!ctx_buf) { > + pr_err("Failed to convert ctx_buf to dev_buf: Null pointer\n"); > + return NULL; > + } > + > + dev_buf = mtk_cam_ctx_buf_to_dev_buf(ctx_buf); > + > + return &dev_buf->m2m2_buf.vbb; > +} I also added a comment to where mtk_cam_dev_buffer struct is defined, but let me repeat here. I don't think we need to split these structs so much. We just need one driver-specific struct that encapsulates the vb2_v4l2_buffer struct, e.g. mtk_cam_buffer. > + > +/* The helper to configure the device context */ > +int mtk_cam_ctx_core_steup(struct mtk_cam_ctx *ctx, > + struct mtk_cam_ctx_setting *ctx_setting) > +{ > + if (!ctx || !ctx_setting) > + return -EINVAL; > + > + ctx->ops = ctx_setting->ops; > + ctx->device_name = ctx_setting->device_name; > + > + return 0; > +} I don't understand what's the point of this. We only have one device, P1, and only one context ops, for this P1 device. Do we need this abstraction? > + > +int mtk_cam_ctx_core_queue_setup(struct mtk_cam_ctx *ctx, > + struct mtk_cam_ctx_queues_setting > + *queues_setting) > +{ > + int queue_idx = 0; > + int i = 0; > + > + for (i = 0; i < queues_setting->total_output_queues; i++) { > + struct mtk_cam_ctx_queue_desc *queue_desc = > + queues_setting->output_queue_descs + i; The typical kernel convention would be &queues_setting->output_queue_descs[i]. > + > + if (!queue_desc) > + return -EINVAL; I don't think this can ever happen, especially for i > 0. > + > + /* Since the *ctx->queue has been initialized to 0 */ > + /* when it is allocated with mtk_cam_dev , */ > + /* I don't initialize the struct here */ > + ctx->queue[queue_idx].desc = *queue_desc; > + queue_idx++; > + } > + > + ctx->queues_attr.input_offset = queue_idx; This input_offset doesn't seem to be used in the driver in any meaningful way. The only place it's read is in mtk_cam_dev_mem2mem2_init() and that could be easily replaced with checking for (!isp_dev->ctx.queue[i].capture). > + > + /* Setup the capture queue */ > + for (i = 0; i < queues_setting->total_capture_queues; i++) { > + struct mtk_cam_ctx_queue_desc *queue_desc = > + queues_setting->capture_queue_descs + i; > + > + if (!queue_desc) > + return -EINVAL; Ditto. > + > + /* Since the *ctx->queue has been initialized to 0 */ > + /* when allocating the memory, I don't */ > + /* reinitialied the struct here */ > + ctx->queue[queue_idx].desc = *queue_desc; > + queue_idx++; > + } > + > + ctx->queues_attr.master = queues_setting->master; > + ctx->queues_attr.total_num = queue_idx; > + ctx->dev_node_num = ctx->queues_attr.total_num; > + return 0; > +} This function just copies some compile-time constant data at runtime. Why couldn't we just use the compile-time constants directly? Also, I don't see vb2 queues handled here. What a V4L2 driver would normally do is: - have a driver-private struct to represent the queue (e.g. mtk_cam_queue) and encapsulating vb2_queue, - set vb2_queue::drv_priv to point to the mtk_cam_queue struct encapsulating it, - both the vb2_queue and internal data fields would be initialized in the same function, to keep queue-initialization code in the same part of the driver. > + > +/* Mediatek ISP context core initialization */ > +int mtk_cam_ctx_core_init(struct mtk_cam_ctx *ctx, > + struct platform_device *pdev, int ctx_id, > + struct mtk_cam_ctx_desc *ctx_desc, > + struct platform_device *proc_pdev, > + struct platform_device *smem_pdev) > +{ > + /* Initialize main data structure */ > + int r = 0; Please don't initialize return value variables, unless really necessary. It prevents the compiler from detecting forgotten assignments further in the function. > + > + ctx->smem_vb2_alloc_ctx = &smem_pdev->dev; > + ctx->default_vb2_alloc_ctx = &pdev->dev; > + > + if (IS_ERR((__force void *)ctx->smem_vb2_alloc_ctx)) > + pr_err("Failed to alloc vb2 dma context: smem_vb2_alloc_ctx"); > + > + if (IS_ERR((__force void *)ctx->default_vb2_alloc_ctx)) > + pr_err("Failed to alloc vb2 dma context: default_vb2_alloc_ctx"); It's impossible for the two conditions above to happen. > + > + ctx->pdev = pdev; > + ctx->ctx_id = ctx_id; > + /* keep th smem pdev to use related iommu functions */ > + ctx->smem_device = smem_pdev; nit: Perhaps the field could be renamed to smem_pdev? > + > + /* initialized the global frame index of the device context */ > + atomic_set(&ctx->frame_param_sequence, 0); > + spin_lock_init(&ctx->qlock); > + > + /* setup the core operation of the device context */ > + if (ctx_desc && ctx_desc->init) > + r = ctx_desc->init(ctx); There is only one possible descriptor in this driver - mtk_cam_ctx_desc_p1. Please remove this unnecessary abstraction and just call the right functions directly. > + > + return r; > +} > +EXPORT_SYMBOL_GPL(mtk_cam_ctx_core_init); > + > +int mtk_cam_ctx_core_exit(struct mtk_cam_ctx *ctx) > +{ > + ctx->smem_vb2_alloc_ctx = NULL; > + ctx->default_vb2_alloc_ctx = NULL; ctx is going to be freed soon after this function returns, so there is no need to set these pointers to NULL. (And so, no need for this function at all.) > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(mtk_cam_ctx_core_exit); > + > +/* Get the corrospnd FH of a specific buffer */ typo: corresponding What's FH? This function doesn't seem to accept a buffer pointer, so not sure what you mean by "a specific buffer". Also, the function increments ctx->frame_param_sequence, so it's not just a "get". > +int mtk_cam_ctx_next_global_frame_sequence(struct mtk_cam_ctx *ctx, > + int locked) > +{ > + int global_frame_sequence = > + atomic_inc_return(&ctx->frame_param_sequence); > + > + if (!locked) > + spin_lock(&ctx->qlock); > + > + global_frame_sequence = > + (global_frame_sequence & 0x0000FFFF) | (ctx->ctx_id << 16); > + > + if (!locked) > + spin_unlock(&ctx->qlock); > + > + return global_frame_sequence; > +} > +EXPORT_SYMBOL_GPL(mtk_cam_ctx_next_global_frame_sequence); Please don't make a function change locking behavior based on arguments - it's super error prone and hard to debug. Instead, please provide 2 variants: __mtk_cam_ctx_next_global_frame_sequence_locked() which does everything except locking and mtk_cam_ctx_next_global_frame_sequence() which simply grabs the lock and calls __mtk_cam_ctx_next_global_frame_sequence_locked(). But, do you really need a _locked() variant here? I can see this function being called only once in mtk_cam_ctx_trigger_job(), with dev_ctx->ctx_id as the second argument, which sounds like a bug. Also, why do you even need to acquire ctx->qlock here, if ctx->frame_param_sequence is atomic? > + > +static void mtk_cam_ctx_buffer_done > + (struct mtk_cam_ctx_buffer *ctx_buf, int state) > +{ > + if (!ctx_buf || > + state != MTK_CAM_CTX_BUFFER_DONE || > + state != MTK_CAM_CTX_BUFFER_FAILED) > + return; > + > + ctx_buf->state = state; > +} I don't see why this function could even be useful. VB2 already tracks buffer states, as I mentioned in my earlier comment. [snip] > +#define file_to_mtk_cam_node(__file) \ > + container_of(video_devdata(__file),\ > + struct mtk_cam_dev_video_device, vdev) > + > +#define mtk_cam_ctx_to_dev(__ctx) \ > + container_of(__ctx,\ > + struct mtk_cam_dev, ctx) > + > +#define mtk_cam_m2m_to_dev(__m2m) \ > + container_of(__m2m,\ > + struct mtk_cam_dev, mem2mem2) > + > +#define mtk_cam_subdev_to_dev(__sd) \ > + container_of(__sd, \ > + struct mtk_cam_dev, mem2mem2.subdev) > + > +#define mtk_cam_vbq_to_isp_node(__vq) \ > + container_of(__vq, \ > + struct mtk_cam_dev_video_device, vbq) > + > +#define mtk_cam_ctx_buf_to_dev_buf(__ctx_buf) \ > + container_of(__ctx_buf, \ > + struct mtk_cam_dev_buffer, ctx_buf) > + > +#define mtk_cam_vb2_buf_to_dev_buf(__vb) \ > + container_of(vb, \ > + struct mtk_cam_dev_buffer, \ > + m2m2_buf.vbb.vb2_buf) > + > +#define mtk_cam_vb2_buf_to_m2m_buf(__vb) \ > + container_of(__vb, \ > + struct mtk_cam_mem2mem2_buffer, \ > + vbb.vb2_buf) > + > +#define mtk_cam_subdev_to_m2m(__sd) \ > + container_of(__sd, \ > + struct mtk_cam_mem2mem2_device, subdev) All the macros above need to be replaced with static inline functions to guarantee the right argument and return types at compilation time. > + > +struct mtk_cam_mem2mem2_device; > + > +struct mtk_cam_mem2mem2_buffer { > + struct vb2_v4l2_buffer vbb; > + struct list_head list; > +}; > + > +struct mtk_cam_dev_buffer { > + struct mtk_cam_mem2mem2_buffer m2m2_buf; > + /* Intenal part */ > + struct mtk_cam_ctx_buffer ctx_buf; Why do we need to separate this? All the data here belongs to this driver. The reply is quite long already, so let me send it. Second part to follow. Best regards, Tomasz
Dear Thomas: Thanks for your comments. We will revise the source codes based on your comments. Since there are many comments to fix/revise, we will categorize & prioritize these with below list: 1. Coding style issues. 2. Coding defects, including unused codes. 3. Driver architecture refactoring, such as removing mtk_cam_ctx, unnecessary abstraction layer, etc. On Wed, 2019-02-13 at 18:50 +0900, Tomasz Figa wrote: > (() . ( strHi Frederic, Jungo, > > On Tue, Feb 5, 2019 at 3:43 PM Frederic Chen <frederic.chen@mediatek.com> wrote: > > > > From: Jungo Lin <jungo.lin@mediatek.com> > > > > This patch adds the driver for Pass unit in Mediatek's camera > > ISP system. Pass 1 unit is embedded in Mediatek SOCs. It > > provides RAW processing which includes optical black correction, > > defect pixel correction, W/IR imbalance correction and lens > > shading correction. > > > > The mtk-isp directory will contain drivers for multiple IP > > blocks found in Mediatek ISP system. It will include ISP Pass 1 > > driver, sensor interface driver, DIP driver and face detection > > driver. > > Thanks for the patches! Please see my comments inline. > > [snip] > > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-ctx.h b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-ctx.h > > new file mode 100644 > > index 0000000..11a60a6 > > --- /dev/null > > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-ctx.h > > @@ -0,0 +1,327 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (c) 2018 MediaTek Inc. > > + * Author: Frederic Chen <frederic.chen@mediatek.com> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#ifndef __MTK_CAM_CTX_H__ > > +#define __MTK_CAM_CTX_H__ > > + > > +#include <linux/types.h> > > +#include <linux/videodev2.h> > > +#include <media/v4l2-ctrls.h> > > +#include <media/videobuf2-core.h> > > +#include <media/v4l2-subdev.h> > > +#include "mtk_cam-v4l2-util.h" > > + > > +#define MTK_CAM_CTX_QUEUES (16) > > +#define MTK_CAM_CTX_FRAME_BUNDLE_BUFFER_MAX (MTK_CAM_CTX_QUEUES) > > +#define MTK_CAM_CTX_DESC_MAX (MTK_CAM_CTX_QUEUES) > > + > > +#define MTK_CAM_CTX_MODE_DEBUG_OFF (0) > > +#define MTK_CAM_CTX_MODE_DEBUG_BYPASS_JOB_TRIGGER (1) > > +#define MTK_CAM_CTX_MODE_DEBUG_BYPASS_ALL (2) > > nit: No need for parentheses for simple values. Also please align > macro values together using tabs. > 1. Coding style issues - Fix in next release. > > + > > +#define MTK_CAM_GET_CTX_ID_FROM_SEQUENCE(sequence) \ > > + ((sequence) >> 16 & 0x0000FFFF) > > Sounds like a job for a static inline function, with appropriate > argument and return types enforced by the compiler. > 2. Coding defects - Plan to fix. > > + > > +#define MTK_CAM_CTX_META_BUF_DEFAULT_SIZE (1110 * 1024) > > + > > +struct mtk_cam_ctx; > > +struct mtk_cam_ctx_open_param; > > +struct mtk_cam_ctx_release_param; > > +struct mtk_cam_ctx_streamon_param; > > +struct mtk_cam_ctx_streamoff_param; > > +struct mtk_cam_ctx_start_param; > > +struct mtk_cam_ctx_finish_param; > > + > > +/* struct mtk_cam_ctx_ops - background hardware driving ops */ > > +/* sdefines background driver specific callback APIs */ > > +struct mtk_cam_ctx_ops { > > + int (*open)(struct mtk_cam_ctx *dev_ctx, > > + struct mtk_cam_ctx_open_param *param); > > + int (*release)(struct mtk_cam_ctx *dev_ctx, > > + struct mtk_cam_ctx_release_param *param); > > + int (*start)(struct mtk_cam_ctx *dev_ctx, > > + struct mtk_cam_ctx_start_param *param); > > + int (*finish)(struct mtk_cam_ctx *dev_ctx, > > + struct mtk_cam_ctx_finish_param *param); > > + int (*streamon)(struct mtk_cam_ctx *dev_ctx, > > + struct mtk_cam_ctx_streamon_param *param); > > + int (*streamoff)(struct mtk_cam_ctx *dev_ctx, > > + struct mtk_cam_ctx_streamoff_param *param); > > +}; > > We only have one driver here and these ops look really close to what > vb2 already provides. Why do we need this indirection? > 3. Driver architecture refactoring - Plan to fix. > > + > > +/* Attributes setup by device context owner */ > > +struct mtk_cam_ctx_queue_desc { > > + int id; /* id of the context queue */ > > Please use kerneldoc syntax for documenting the structures. > (https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txtjjj > 1. Coding style issues - Fix in next release. > > + char *name; > > + /* Will be exported to media entity name */ > > + int capture; > > + /* true for capture queue (device to user), false for output queue */ > > + /* (from user to device) */ > > + int image; > > + /* Using the cam_smem_drv as alloc ctx or not */ > > + int smem_alloc; > > + /* true for image, false for meta data */ > > + unsigned int dma_port; /*The dma port associated to the buffer*/ > > + /* Supported format */ > > + struct mtk_cam_ctx_format *fmts; > > + int num_fmts; > > + /* Default format of this queue */ > > + int default_fmt_idx; > > +}; > > + > > +/* Supported format and the information used for */ > > +/* size calculation */ > > CodingStyle: > /* > * This is the kernel style for multi-line > * comments. > */ > 1. Coding style issues - Fix in next release. > > +struct mtk_cam_ctx_meta_format { > > + u32 dataformat; > > + u32 max_buffer_size; > > + u8 flags; > > +}; > > + > > +struct mtk_cam_ctx_img_format { > > + u32 pixelformat; > > + u8 depth[VIDEO_MAX_PLANES]; > > + u8 row_depth[VIDEO_MAX_PLANES]; > > + u8 num_planes; > > + u32 flags; > > +}; > > + > > +struct mtk_cam_ctx_format { > > + union { > > + struct mtk_cam_ctx_meta_format meta; > > + struct mtk_cam_ctx_img_format img; > > + } fmt; > > +}; > > + > > +union mtk_v4l2_fmt { > > + struct v4l2_pix_format_mplane pix_mp; > > + struct v4l2_meta_format meta; > > +}; > > Why not just use the standard v4l2_format? > 3. Driver architecture refactoring - yes, it is possible to use v4l2_format and will depended on how to remove mtk_cam_ctx structure. > > + > > +/* Attributes setup by device context owner */ > > +struct mtk_cam_ctx_queues_setting { > > + int master; > > + /* The master input node to trigger the frame data enqueue */ > > + struct mtk_cam_ctx_queue_desc *output_queue_descs; > > + int total_output_queues; > > + struct mtk_cam_ctx_queue_desc *capture_queue_descs; > > + int total_capture_queues; > > +}; > > + > > +struct mtk_cam_ctx_queue_attr { > > + int master; > > + int input_offset; > > + int total_num; > > +}; > > + > > +/* Video node context. Since we use */ > > +/* mtk_cam_ctx_frame_bundle to manage enqueued */ > > +/* buffers by frame now, we don't use bufs filed of */ > > +/* mtk_cam_ctx_queue now */ > > +struct mtk_cam_ctx_queue { > > + union mtk_v4l2_fmt fmt; > > + struct mtk_cam_ctx_format *ctx_fmt; > > + /* Currently we used in standard v4l2 image format */ > > + /* in the device context */ > > + unsigned int width_pad; /* bytesperline, reserved */ > > What's the meaning of this field? 2. Coding defects - Unused field. Should be removed in next release. > > > + struct mtk_cam_ctx_queue_desc desc; > > + struct list_head bufs; /* Reserved, not used now */ > > +}; > > + > > +enum mtk_cam_ctx_frame_bundle_state { > > + MTK_CAM_CTX_FRAME_NEW, /* Not allocated */ > > + MTK_CAM_CTX_FRAME_PREPARED, /* Allocated but has not be processed */ > > + MTK_CAM_CTX_FRAME_PROCESSING, /* Queued, waiting to be filled */ > > +}; > > Feels like duplicating the media_request::state. (and eliminating the > benefit of the state machine being fully managed by the Request API > core). > 3. Driver architecture refactoring - Plan to remove based on Request API mechanism. > > + > > +/* The definiation is compatible with DIP driver's state definiation */ > > +/* currently and will be decoupled after further integration */ > > +enum mtk_cam_ctx_frame_data_state { > > + MTK_CAM_CTX_FRAME_DATA_EMPTY = 0, /* FRAME_STATE_INIT */ > > + MTK_CAM_CTX_FRAME_DATA_DONE = 3, /* FRAME_STATE_DONE */ > > + MTK_CAM_CTX_FRAME_DATA_STREAMOFF_DONE = 4, /*FRAME_STATE_STREAMOFF*/ > > + MTK_CAM_CTX_FRAME_DATA_ERROR = 5, /*FRAME_STATE_ERROR*/ > > +}; > > What are these states for? > 3. Driver architecture refactoring - Plan to replace with VB2 state. > > + > > +struct mtk_cam_ctx_frame_bundle { > > + struct mtk_cam_ctx_buffer* > > + buffers[MTK_CAM_CTX_FRAME_BUNDLE_BUFFER_MAX]; > > + int id; > > + int num_img_capture_bufs; > > + int num_img_output_bufs; > > + int num_meta_capture_bufs; > > + int num_meta_output_bufs; > > + int last_index; > > + int state; > > + struct list_head list; > > +}; > > + > > +struct mtk_cam_ctx_frame_bundle_list { > > + struct list_head list; > > +}; > > This sounds like duplicating the request API core functionality, which > aggregates all the buffers in the request. > 3. Driver architecture refactoring - Plan to remove based on Request API mechanism. > > + > > +struct mtk_cam_ctx { > > + struct platform_device *pdev; > > + struct platform_device *smem_device; > > + /* buffer queues will be added later */ > > + unsigned short ctx_id; > > + char *device_name; > > + const struct mtk_cam_ctx_ops *ops; > > + struct mtk_cam_dev_node_mapping *mtk_cam_dev_node_map; > > + unsigned int dev_node_num; > > + /* mtk_cam_ctx_queue is the context for the video nodes */ > > + struct mtk_cam_ctx_queue queue[MTK_CAM_CTX_QUEUES]; > > + struct mtk_cam_ctx_queue_attr queues_attr; > > + atomic_t frame_param_sequence; > > + int streaming; > > + void *default_vb2_alloc_ctx; > > + void *smem_vb2_alloc_ctx; > > + struct v4l2_subdev_fh *fh; > > + struct mtk_cam_ctx_frame_bundle frame_bundles[VB2_MAX_FRAME]; > > + struct mtk_cam_ctx_frame_bundle_list processing_frames; > > + struct mtk_cam_ctx_frame_bundle_list free_frames; > > + int enabled_dma_ports; > > + int num_frame_bundle; > > + spinlock_t qlock; /* frame queues protection */ > > +}; > > + > > +enum mtk_cam_ctx_buffer_state { > > + MTK_CAM_CTX_BUFFER_NEW, > > + MTK_CAM_CTX_BUFFER_PROCESSING, > > + MTK_CAM_CTX_BUFFER_DONE, > > + MTK_CAM_CTX_BUFFER_FAILED, > > +}; > > Why these custom states? VB2 already manages buffer states for you and > the driver should just obtain available buffers in the .buf_queue > callback and return them with appropriate state using > vb2_buffer_done() when they are no longer in use. > 3. Driver architecture refactoring - Plan to replace with VB2 state. > > + > > +struct mtk_cam_ctx_buffer { > > + union mtk_v4l2_fmt fmt; > > + struct mtk_cam_ctx_format *ctx_fmt; > > + int capture; > > + int image; > > + int frame_id; > > + int user_sequence; /* Sequence number assigned by user */ > > What's this user_sequence? There shouldn't be any sequence coming from > the user, since the requests are used to aggregate buffers. > 2. Coding defects - It is used without Request API support. In current design, it should be removed in next release. > > + dma_addr_t daddr; > > + void *vaddr; > > We shouldn't need to touch the buffers from the kernel driver. > 2. Coding defects - Will remove in next release. > > + phys_addr_t paddr; > > We shouldn't need physical address either. I suppose this is for the > SCP, but then it should be a DMA address obtained from dma_map_*() > with struct device pointer of the SCP. > Yes, this physical address is designed for SCP. For tuning buffer, it will be touched by SCP and SCP can't get the physical address by itself. So we need to get this physical address in the kernel space via mtk_cam_smem_iova_to_phys function call and pass it to the SCP. At the same time, DMA address (daddr) is used for ISP HW. > > + unsigned int queue; > > + enum mtk_cam_ctx_buffer_state state; > > + struct list_head list; > > +}; > > + > > +struct mtk_cam_ctx_setting { > > + struct mtk_cam_ctx_ops *ops; > > + char *device_name; > > +}; > > + > > +struct mtk_cam_ctx_desc { > > + char *proc_dev_phandle; > > + /* The context device's compatble string name in device tree*/ > > + int (*init)(struct mtk_cam_ctx *ctx); > > + /* configure the core functions of the device context */ > > +}; > > + > > +struct mtk_cam_ctx_init_table { > > + int total_dev_ctx; > > + struct mtk_cam_ctx_desc *ctx_desc_tbl; > > +}; > > + > > +struct mtk_cam_ctx_open_param { > > + /* Bitmask used to notify that the DMA port is enabled or not */ > > + unsigned int enabled_dma_ports; > > +}; > > + > > +struct mtk_cam_ctx_streamon_param { > > + unsigned int enabled_dma_ports; > > +}; > > + > > +struct mtk_cam_ctx_streamoff_param { > > + unsigned int enabled_dma_ports; > > +}; > > Hmm, the 3 structs above are identical. Is there a need to have all 3 of them? > 3. Driver architecture refactoring - Plan to revise. > > + > > +struct mtk_cam_ctx_start_param { > > + /* carry buffer information of the frame */ > > + struct mtk_cam_ctx_frame_bundle *frame_bundle; > > +}; > > + > > +struct mtk_cam_ctx_release_param { > > + unsigned int enabled_dma_ports; > > +}; > > One more identical struct. > Same as above. > [snip] > > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-dev-ctx-core.c b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-dev-ctx-core.c > > new file mode 100644 > > index 0000000..7d0197b > > --- /dev/null > > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-dev-ctx-core.c > > @@ -0,0 +1,986 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2018 MediaTek Inc. > > + * Author: Frederic Chen <frederic.chen@mediatek.com> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#include <linux/device.h> > > +#include <linux/platform_device.h> > > +#include <media/videobuf2-dma-contig.h> > > +#include <linux/dma-mapping.h> > > +#include <media/v4l2-event.h> > > + > > +#include "mtk_cam-dev.h" > > +#include "mtk_cam-v4l2-util.h" > > +#include "mtk_cam-v4l2.h" > > +#include "mtk_cam-smem.h" > > + > > +static struct mtk_cam_ctx_format *mtk_cam_ctx_find_fmt > > + (struct mtk_cam_ctx_queue *queue, > > + u32 format); > > + > > +static int mtk_cam_ctx_process_frame(struct mtk_cam_ctx *dev_ctx, > > + struct mtk_cam_ctx_frame_bundle > > + *frame_bundle); > > + > > +static int mtk_cam_ctx_free_frame(struct mtk_cam_ctx *dev_ctx, > > + struct mtk_cam_ctx_frame_bundle > > + *frame_bundle); > > + > > +static struct mtk_cam_ctx_frame_bundle *mtk_cam_ctx_get_free_frame > > + (struct mtk_cam_ctx *dev_ctx); > > + > > +static struct mtk_cam_ctx_frame_bundle *mtk_cam_ctx_get_processing_frame > > +(struct mtk_cam_ctx *dev_ctx, int frame_id); > > + > > +static int mtk_cam_ctx_init_frame_bundles(struct mtk_cam_ctx *dev_ctx); > > + > > +static void mtk_cam_ctx_queue_event_frame_done > > + (struct mtk_cam_ctx *dev_ctx, > > + struct mtk_cam_dev_frame_done_event_data *fdone); > > + > > +static void debug_bundle(struct mtk_cam_ctx *dev_ctx, > > + struct mtk_cam_ctx_frame_bundle *bundle_data); > > Could you reorder the functions in this file so that there is no need > for forward declarations? In general, a kernel source file should be > ordered in such way that the reader can understand what's happening by > reading from the top to bottom, without jumping between functions. > 2. Coding defects - Will fix in next release. > > + > > +struct vb2_v4l2_buffer *mtk_cam_ctx_buffer_get_vb2_v4l2_buffer > > +(struct mtk_cam_ctx_buffer *ctx_buf) > > +{ > > + struct mtk_cam_dev_buffer *dev_buf = NULL; > > + > > + if (!ctx_buf) { > > + pr_err("Failed to convert ctx_buf to dev_buf: Null pointer\n"); > > + return NULL; > > + } > > + > > + dev_buf = mtk_cam_ctx_buf_to_dev_buf(ctx_buf); > > + > > + return &dev_buf->m2m2_buf.vbb; > > +} > > I also added a comment to where mtk_cam_dev_buffer struct is defined, > but let me repeat here. I don't think we need to split these structs > so much. We just need one driver-specific struct that encapsulates the > vb2_v4l2_buffer struct, e.g. mtk_cam_buffer. > 3. Driver architecture refactoring - Yes, we will plan to refactoring current implementation. > > + > > +/* The helper to configure the device context */ > > +int mtk_cam_ctx_core_steup(struct mtk_cam_ctx *ctx, > > + struct mtk_cam_ctx_setting *ctx_setting) > > +{ > > + if (!ctx || !ctx_setting) > > + return -EINVAL; > > + > > + ctx->ops = ctx_setting->ops; > > + ctx->device_name = ctx_setting->device_name; > > + > > + return 0; > > +} > > I don't understand what's the point of this. We only have one device, > P1, and only one context ops, for this P1 device. Do we need this > abstraction? > 3. Driver architecture refactoring - Yes, we will plan to remove this abstraction. > > + > > +int mtk_cam_ctx_core_queue_setup(struct mtk_cam_ctx *ctx, > > + struct mtk_cam_ctx_queues_setting > > + *queues_setting) > > +{ > > + int queue_idx = 0; > > + int i = 0; > > + > > + for (i = 0; i < queues_setting->total_output_queues; i++) { > > + struct mtk_cam_ctx_queue_desc *queue_desc = > > + queues_setting->output_queue_descs + i; > > The typical kernel convention would be &queues_setting->output_queue_descs[i]. > 2. Coding defects - Fix in next release. > > + > > + if (!queue_desc) > > + return -EINVAL; > > I don't think this can ever happen, especially for i > 0. > 2. Coding defects - Remove in next release. > > + > > + /* Since the *ctx->queue has been initialized to 0 */ > > + /* when it is allocated with mtk_cam_dev , */ > > + /* I don't initialize the struct here */ > > + ctx->queue[queue_idx].desc = *queue_desc; > > + queue_idx++; > > + } > > + > > + ctx->queues_attr.input_offset = queue_idx; > > This input_offset doesn't seem to be used in the driver in any > meaningful way. The only place it's read is in > mtk_cam_dev_mem2mem2_init() and that could be easily replaced with > checking for (!isp_dev->ctx.queue[i].capture). > 2. Coding defects - Plan to revise in next release. > > + > > + /* Setup the capture queue */ > > + for (i = 0; i < queues_setting->total_capture_queues; i++) { > > + struct mtk_cam_ctx_queue_desc *queue_desc = > > + queues_setting->capture_queue_descs + i; > > + > > + if (!queue_desc) > > + return -EINVAL; > > Ditto. > Same as above. > > + > > + /* Since the *ctx->queue has been initialized to 0 */ > > + /* when allocating the memory, I don't */ > > + /* reinitialied the struct here */ > > + ctx->queue[queue_idx].desc = *queue_desc; > > + queue_idx++; > > + } > > + > > + ctx->queues_attr.master = queues_setting->master; > > + ctx->queues_attr.total_num = queue_idx; > > + ctx->dev_node_num = ctx->queues_attr.total_num; > > + return 0; > > +} > > This function just copies some compile-time constant data at runtime. > Why couldn't we just use the compile-time constants directly? > > Also, I don't see vb2 queues handled here. What a V4L2 driver would > normally do is: > - have a driver-private struct to represent the queue (e.g. > mtk_cam_queue) and encapsulating vb2_queue, > - set vb2_queue::drv_priv to point to the mtk_cam_queue struct encapsulating it, > - both the vb2_queue and internal data fields would be initialized in > the same function, to keep queue-initialization code in the same part > of the driver. > 3. Driver architecture refactoring - Yes, we will plan to refactoring current implementation. > > + > > +/* Mediatek ISP context core initialization */ > > +int mtk_cam_ctx_core_init(struct mtk_cam_ctx *ctx, > > + struct platform_device *pdev, int ctx_id, > > + struct mtk_cam_ctx_desc *ctx_desc, > > + struct platform_device *proc_pdev, > > + struct platform_device *smem_pdev) > > +{ > > + /* Initialize main data structure */ > > + int r = 0; > > Please don't initialize return value variables, unless really > necessary. It prevents the compiler from detecting forgotten > assignments further in the function. > 2. Coding defects - Fix in next release. > > + > > + ctx->smem_vb2_alloc_ctx = &smem_pdev->dev; > > + ctx->default_vb2_alloc_ctx = &pdev->dev; > > + > > + if (IS_ERR((__force void *)ctx->smem_vb2_alloc_ctx)) > > + pr_err("Failed to alloc vb2 dma context: smem_vb2_alloc_ctx"); > > + > > + if (IS_ERR((__force void *)ctx->default_vb2_alloc_ctx)) > > + pr_err("Failed to alloc vb2 dma context: default_vb2_alloc_ctx"); > > It's impossible for the two conditions above to happen. > 2. Coding defects - Remove in next release. > > + > > + ctx->pdev = pdev; > > + ctx->ctx_id = ctx_id; > > + /* keep th smem pdev to use related iommu functions */ > > + ctx->smem_device = smem_pdev; > > nit: Perhaps the field could be renamed to smem_pdev? > 2. Coding defects - Fix in next release. > > + > > + /* initialized the global frame index of the device context */ > > + atomic_set(&ctx->frame_param_sequence, 0); > > + spin_lock_init(&ctx->qlock); > > + > > + /* setup the core operation of the device context */ > > + if (ctx_desc && ctx_desc->init) > > + r = ctx_desc->init(ctx); > > There is only one possible descriptor in this driver - > mtk_cam_ctx_desc_p1. Please remove this unnecessary abstraction and > just call the right functions directly. > 3. Driver architecture refactoring - Yes, we will plan to remove this abstraction. > > + > > + return r; > > +} > > +EXPORT_SYMBOL_GPL(mtk_cam_ctx_core_init); > > + > > +int mtk_cam_ctx_core_exit(struct mtk_cam_ctx *ctx) > > +{ > > + ctx->smem_vb2_alloc_ctx = NULL; > > + ctx->default_vb2_alloc_ctx = NULL; > > ctx is going to be freed soon after this function returns, so there is > no need to set these pointers to NULL. (And so, no need for this > function at all.) > 2. Coding defects - Remove in next release. > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(mtk_cam_ctx_core_exit); > > + > > +/* Get the corrospnd FH of a specific buffer */ > > typo: corresponding > What's FH? > This function doesn't seem to accept a buffer pointer, so not sure > what you mean by "a specific buffer". > Also, the function increments ctx->frame_param_sequence, so it's not > just a "get". > 2. Coding defects - The purpose of this function is to generate one unique frame sequence. We will correct the comment of this function. > > +int mtk_cam_ctx_next_global_frame_sequence(struct mtk_cam_ctx *ctx, > > + int locked) > > +{ > > + int global_frame_sequence = > > + atomic_inc_return(&ctx->frame_param_sequence); > > + > > + if (!locked) > > + spin_lock(&ctx->qlock); > > + > > + global_frame_sequence = > > + (global_frame_sequence & 0x0000FFFF) | (ctx->ctx_id << 16); > > + > > + if (!locked) > > + spin_unlock(&ctx->qlock); > > + > > + return global_frame_sequence; > > +} > > +EXPORT_SYMBOL_GPL(mtk_cam_ctx_next_global_frame_sequence); > > Please don't make a function change locking behavior based on > arguments - it's super error prone and hard to debug. Instead, please > provide 2 variants: > > __mtk_cam_ctx_next_global_frame_sequence_locked() which does > everything except locking > > and > > mtk_cam_ctx_next_global_frame_sequence() which simply grabs the lock > and calls __mtk_cam_ctx_next_global_frame_sequence_locked(). > > But, do you really need a _locked() variant here? I can see this > function being called only once in mtk_cam_ctx_trigger_job(), with > dev_ctx->ctx_id as the second argument, which sounds like a bug. Also, > why do you even need to acquire ctx->qlock here, if > ctx->frame_param_sequence is atomic? > 3. Driver architecture refactoring - We will plan to remove context implementation in the P1 driver. So this function may be removed in future. If we still keep this function, we will revise based on your suggestion. > > + > > +static void mtk_cam_ctx_buffer_done > > + (struct mtk_cam_ctx_buffer *ctx_buf, int state) > > +{ > > + if (!ctx_buf || > > + state != MTK_CAM_CTX_BUFFER_DONE || > > + state != MTK_CAM_CTX_BUFFER_FAILED) > > + return; > > + > > + ctx_buf->state = state; > > +} > > I don't see why this function could even be useful. VB2 already tracks > buffer states, as I mentioned in my earlier comment. > > [snip] > 3. Driver architecture refactoring - Plan to remove during driver refactoring. > > +#define file_to_mtk_cam_node(__file) \ > > + container_of(video_devdata(__file),\ > > + struct mtk_cam_dev_video_device, vdev) > > + > > +#define mtk_cam_ctx_to_dev(__ctx) \ > > + container_of(__ctx,\ > > + struct mtk_cam_dev, ctx) > > + > > +#define mtk_cam_m2m_to_dev(__m2m) \ > > + container_of(__m2m,\ > > + struct mtk_cam_dev, mem2mem2) > > + > > +#define mtk_cam_subdev_to_dev(__sd) \ > > + container_of(__sd, \ > > + struct mtk_cam_dev, mem2mem2.subdev) > > + > > +#define mtk_cam_vbq_to_isp_node(__vq) \ > > + container_of(__vq, \ > > + struct mtk_cam_dev_video_device, vbq) > > + > > +#define mtk_cam_ctx_buf_to_dev_buf(__ctx_buf) \ > > + container_of(__ctx_buf, \ > > + struct mtk_cam_dev_buffer, ctx_buf) > > + > > +#define mtk_cam_vb2_buf_to_dev_buf(__vb) \ > > + container_of(vb, \ > > + struct mtk_cam_dev_buffer, \ > > + m2m2_buf.vbb.vb2_buf) > > + > > +#define mtk_cam_vb2_buf_to_m2m_buf(__vb) \ > > + container_of(__vb, \ > > + struct mtk_cam_mem2mem2_buffer, \ > > + vbb.vb2_buf) > > + > > +#define mtk_cam_subdev_to_m2m(__sd) \ > > + container_of(__sd, \ > > + struct mtk_cam_mem2mem2_device, subdev) > > All the macros above need to be replaced with static inline functions > to guarantee the right argument and return types at compilation time. > 3. Driver architecture refactoring - Plan to revise during driver refactoring. > > + > > +struct mtk_cam_mem2mem2_device; > > + > > +struct mtk_cam_mem2mem2_buffer { > > + struct vb2_v4l2_buffer vbb; > > + struct list_head list; > > +}; > > + > > +struct mtk_cam_dev_buffer { > > + struct mtk_cam_mem2mem2_buffer m2m2_buf; > > + /* Intenal part */ > > + struct mtk_cam_ctx_buffer ctx_buf; > > Why do we need to separate this? All the data here belongs to this driver. > 3. Driver architecture refactoring - Plan to revise during driver refactoring. > The reply is quite long already, so let me send it. Second part to follow. > > Best regards, > Tomasz Appreciate your patience to provide these helpful comments. We look forward to further comments. Thanks, Jungo
Hi Jungo, On Sun, Feb 17, 2019 at 11:56 AM Jungo Lin <jungo.lin@mediatek.com> wrote: > > On Wed, 2019-02-13 at 18:50 +0900, Tomasz Figa wrote: > > (() . ( strHi Frederic, Jungo, > > > > On Tue, Feb 5, 2019 at 3:43 PM Frederic Chen <frederic.chen@mediatek.com> wrote: > > > > > > From: Jungo Lin <jungo.lin@mediatek.com> > > > > > > This patch adds the driver for Pass unit in Mediatek's camera > > > ISP system. Pass 1 unit is embedded in Mediatek SOCs. It > > > provides RAW processing which includes optical black correction, > > > defect pixel correction, W/IR imbalance correction and lens > > > shading correction. > > > > > > The mtk-isp directory will contain drivers for multiple IP > > > blocks found in Mediatek ISP system. It will include ISP Pass 1 > > > driver, sensor interface driver, DIP driver and face detection > > > driver. > > > > Thanks for the patches! Please see my comments inline. > > > > Dear Thomas: > > Thanks for your comments. > > We will revise the source codes based on your comments. > Since there are many comments to fix/revise, we will categorize & > prioritize these with below list: > > 1. Coding style issues. > 2. Coding defects, including unused codes. > 3. Driver architecture refactoring, such as removing mtk_cam_ctx, > unnecessary abstraction layer, etc. > Thanks for replying to the comments! Just to clarify, there is no need to hurry with resending a next patch set with only a subset of the changes. Please take your time to address all the comments before sending the next revision. This prevents forgetting about some remaining comments and also lets other reviewers come with new comments or some alternative ideas for already existing comments. Second part of my review is coming too. P.S. Please avoid top-posting on mailing lists. If replying to a message, please reply below the related part of that message. (I've moved your reply to the place in the email where it should be.) [snip] > > > + phys_addr_t paddr; > > > > We shouldn't need physical address either. I suppose this is for the > > SCP, but then it should be a DMA address obtained from dma_map_*() > > with struct device pointer of the SCP. > > > > Yes, this physical address is designed for SCP. > For tuning buffer, it will be touched by SCP and > SCP can't get the physical address by itself. So we need to get > this physical address in the kernel space via mtk_cam_smem_iova_to_phys > function call and pass it to the SCP. At the same time, DMA address > (daddr) is used for ISP HW. > Right, but my point is that in the kernel phys_addr_t is the physical address from the CPU point of view. Any address from device point of view is dma_addr_t and should be obtained from the DMA mapping API (even if it's numerically the same as physical address). Best regards, Tomasz
On Tue, 2019-02-19 at 17:51 +0900, Tomasz Figa wrote: > Hi Jungo, > > On Sun, Feb 17, 2019 at 11:56 AM Jungo Lin <jungo.lin@mediatek.com> wrote: > > > > On Wed, 2019-02-13 at 18:50 +0900, Tomasz Figa wrote: > > > (() . ( strHi Frederic, Jungo, > > > > > > On Tue, Feb 5, 2019 at 3:43 PM Frederic Chen <frederic.chen@mediatek.com> wrote: > > > > > > > > From: Jungo Lin <jungo.lin@mediatek.com> > > > > > > > > This patch adds the driver for Pass unit in Mediatek's camera > > > > ISP system. Pass 1 unit is embedded in Mediatek SOCs. It > > > > provides RAW processing which includes optical black correction, > > > > defect pixel correction, W/IR imbalance correction and lens > > > > shading correction. > > > > > > > > The mtk-isp directory will contain drivers for multiple IP > > > > blocks found in Mediatek ISP system. It will include ISP Pass 1 > > > > driver, sensor interface driver, DIP driver and face detection > > > > driver. > > > > > > Thanks for the patches! Please see my comments inline. > > > > > > > Dear Thomas: > > > > Thanks for your comments. > > > > We will revise the source codes based on your comments. > > Since there are many comments to fix/revise, we will categorize & > > prioritize these with below list: > > > > 1. Coding style issues. > > 2. Coding defects, including unused codes. > > 3. Driver architecture refactoring, such as removing mtk_cam_ctx, > > unnecessary abstraction layer, etc. > > > > Thanks for replying to the comments! > > Just to clarify, there is no need to hurry with resending a next patch > set with only a subset of the changes. Please take your time to > address all the comments before sending the next revision. This > prevents forgetting about some remaining comments and also lets other > reviewers come with new comments or some alternative ideas for already > existing comments. Second part of my review is coming too. > > P.S. Please avoid top-posting on mailing lists. If replying to a > message, please reply below the related part of that message. (I've > moved your reply to the place in the email where it should be.) > > [snip] Hi, Tomasz, Thanks for your advice. We will prepare the next patch set after all comments are revised. > > > > + phys_addr_t paddr; > > > > > > We shouldn't need physical address either. I suppose this is for the > > > SCP, but then it should be a DMA address obtained from dma_map_*() > > > with struct device pointer of the SCP. > > > > > > > Yes, this physical address is designed for SCP. > > For tuning buffer, it will be touched by SCP and > > SCP can't get the physical address by itself. So we need to get > > this physical address in the kernel space via mtk_cam_smem_iova_to_phys > > function call and pass it to the SCP. At the same time, DMA address > > (daddr) is used for ISP HW. > > > > Right, but my point is that in the kernel phys_addr_t is the physical > address from the CPU point of view. Any address from device point of > view is dma_addr_t and should be obtained from the DMA mapping API > (even if it's numerically the same as physical address). > OK. In order to clarify the address usage, is it ok to rename "dma_addr_t scp_addr"? Moreover, below function will be also renamed. dma_addr_t mtk_cam_smem_iova_to_scp_phys(struct device *dev, dma_addr_t iova) Could you help us to review? If you have any better suggestion, please kindly let us know. Best regards, Jungo > Best regards, > Tomasz > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
Hi Frederic, Jungo, Sorry for making you wait. Here comes the second part. Please see my comments inline. [snip] > + > +#define MTK_CAM_IO_CON_PADS (1) > + > +/* mtk_cam_io_connection --> sensor IF --> sensor 1 */ > +/* --> sensor 2 */ nit: Comment style. Also, isn't the direction opposite? > +struct mtk_cam_io_connection { > + const char *name; Do we need this name? Could we just use subdev.name instead? > + struct platform_device *pdev; This is assigned to mtk_cam_dev::pdev in the code and mtk_cam_dev is a superclass of this struct, so we always have access to it and can get pdev from it. No need to store it here as well. (And not sure why one would normally need to access pdev. Perhaps storing the struct device pointer inside mtk_cam_dev would be more practical?) > + struct v4l2_subdev subdev; Do we need a subdev for an IO connection? Normally the processing part of the ISP would be represented as a subdev and then it would have its pads linked to other entities. > + int enable; > + /* sensor connected */ > + struct v4l2_subdev *sensor; > + /* sensor interface connected */ > + struct v4l2_subdev *sensor_if; > + struct media_pad subdev_pads[MTK_CAM_IO_CON_PADS]; > + /* Current sensor input format*/ > + struct v4l2_mbus_framefmt subdev_fmt; To summarize, it sounds like only "enable", "sensor", "sensor_if" and "subdev_fmt" would be appropriate attributes for an "IO connection". All the rest would normally be considered to be a part of the ISP device (and "pdev" and "name" are redundant). > +}; > + > +struct mtk_cam_dev_video_device { nit: Why not just mtk_cam_video_device? > + const char *name; Why not use vdev.name? > + int output; Already available in vdev.vfl_dir, although not sure why it would be normally needed. > + int immutable; Only master queue seems to be immutable, so maybe just check if the queue is master, wherever this is used currently? > + int enabled; > + int queued; Looks unused. > + struct v4l2_format vdev_fmt; > + struct video_device vdev; > + struct media_pad vdev_pad; > + struct v4l2_mbus_framefmt pad_fmt; > + struct vb2_queue vbq; > + struct list_head buffers; > + struct mutex lock; /* vb2 queue and video device data protection */ It's actually about operations, not data. I'd write it "Serializes vb2 queue and video device operations.". > + atomic_t sequence; > +}; > + > +struct mtk_cam_mem2mem2_device { > + const char *name; > + const char *model; For both of the fields above, they seem to be always MTK_CAM_DEV_P1_NAME, so we can just use the macro directly whenever needed. No need for this indirection. > + struct device *dev; > + int num_nodes; > + struct mtk_cam_dev_video_device *nodes; > + const struct vb2_mem_ops *vb2_mem_ops; This is always "vb2_dma_contig_memops", so it can be used directly. > + unsigned int buf_struct_size; This is always sizeof(struct mtk_cam_dev_buffer), so no need to save it in the struct. > + int streaming; > + struct v4l2_device *v4l2_dev; > + struct media_device *media_dev; These 2 fields are already in mtk_cam_dev which is a superclass of this struct. One can just access them from there directly. > + struct media_pipeline pipeline; > + struct v4l2_subdev subdev; Could you remind me what was the media topology exposed by this driver? This is already the second subdev I spotted in this patch, which looks strange. > + struct media_pad *subdev_pads; > + struct v4l2_file_operations v4l2_file_ops; > + const struct file_operations fops; > +}; Given most of the comments above, it looks like the remaining useful fields in this struct could be just moved to mtk_cam_dev, without the need for this separate struct. > + > +struct mtk_cam_dev { > + struct platform_device *pdev; > + struct mtk_cam_dev_video_device mem2mem2_nodes[MTK_CAM_DEV_NODE_MAX]; > + int queue_enabled[MTK_CAM_DEV_NODE_MAX]; Isn't this redundant with struct mtk_cam_dev_video_device::enabled? > + struct mtk_cam_mem2mem2_device mem2mem2; > + struct mtk_cam_io_connection cio; > + struct v4l2_device v4l2_dev; > + struct media_device media_dev; > + struct mtk_cam_ctx ctx; > + struct v4l2_async_notifier notifier; > + struct mutex lock; /* device level data protection */ Just a side note: As a rule of thumb, mutex is normally used to protect operations not data (with some exceptions). For simple data protection, where accesses would normally take very short time, spinlock would be used. Of course a mutex would technically work too, so let's keep it for now and we can optimize later. > +}; > + > +int mtk_cam_media_register(struct device *dev, > + struct media_device *media_dev, > + const char *model); > +int mtk_cam_v4l2_register(struct device *dev, > + struct media_device *media_dev, > + struct v4l2_device *v4l2_dev, > + struct v4l2_ctrl_handler *ctrl_handler); > +int mtk_cam_v4l2_unregister(struct mtk_cam_dev *dev); > +int mtk_cam_mem2mem2_v4l2_register(struct mtk_cam_dev *dev, > + struct media_device *media_dev, > + struct v4l2_device *v4l2_dev); > + > +int mtk_cam_v4l2_async_register(struct mtk_cam_dev *isp_dev); > + > +void mtk_cam_v4l2_async_unregister(struct mtk_cam_dev *isp_dev); > + > +int mtk_cam_v4l2_discover_sensor(struct mtk_cam_dev *isp_dev); > + > +void mtk_cam_v4l2_buffer_done(struct vb2_buffer *vb, > + enum vb2_buffer_state state); > +extern int mtk_cam_dev_queue_buffers > + (struct mtk_cam_dev *dev, int initial); Remove extern (here and other places too). > +extern int mtk_cam_dev_get_total_node > + (struct mtk_cam_dev *mtk_cam_dev); > +extern char *mtk_cam_dev_get_node_name > + (struct mtk_cam_dev *mtk_cam_dev_obj, int node); > +int mtk_cam_dev_init(struct mtk_cam_dev *isp_dev, > + struct platform_device *pdev, > + struct media_device *media_dev, > + struct v4l2_device *v4l2_dev); > +extern void mtk_cam_dev_mem2mem2_exit > + (struct mtk_cam_dev *mtk_cam_dev_obj); > +int mtk_cam_dev_mem2mem2_init(struct mtk_cam_dev *isp_dev, > + struct media_device *media_dev, > + struct v4l2_device *v4l2_dev); > +int mtk_cam_dev_get_queue_id_of_dev_node > + (struct mtk_cam_dev *mtk_cam_dev_obj, > + struct mtk_cam_dev_video_device *node); > +int mtk_cam_dev_core_init(struct platform_device *pdev, > + struct mtk_cam_dev *isp_dev, > + struct mtk_cam_ctx_desc *ctx_desc); > +int mtk_cam_dev_core_init_ext(struct platform_device *pdev, > + struct mtk_cam_dev *isp_dev, > + struct mtk_cam_ctx_desc *ctx_desc, > + struct media_device *media_dev, > + struct v4l2_device *v4l2_dev); > +extern int mtk_cam_dev_core_release > +(struct platform_device *pdev, struct mtk_cam_dev *isp_dev); nit: Strange line break. Some more usual examples: int mtk_cam_dev_core_release(struct platform_device *pdev, struct mtk_cam_dev *isp_dev); struct long_struct_name *mtk_cam_dev_core_release( struct platform_device *pdev, struct mtk_cam_dev *isp_dev); struct very_very_very_long_struct_name * my_function(int a, int b); > + > +#endif /* __MTK_CAM_DEV_H__ */ > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-regs.h b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-regs.h > new file mode 100644 > index 0000000..b5067d6 > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-regs.h > @@ -0,0 +1,146 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2018 MediaTek Inc. > + * Author: Ryan Yu <ryan.yu@mediatek.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#ifndef _CAM_REGS_H > +#define _CAM_REGS_H > + > +/* TG Bit Mask */ > +#define VFDATA_EN_BIT BIT(0) > +#define CMOS_EN_BIT BIT(0) > + > +/* normal signal bit */ > +#define VS_INT_ST BIT(0) > +#define HW_PASS1_DON_ST BIT(11) > +#define SOF_INT_ST BIT(12) > +#define SW_PASS1_DON_ST BIT(30) > + > +/* err status bit */ > +#define TG_ERR_ST BIT(4) > +#define TG_GBERR_ST BIT(5) > +#define CQ_CODE_ERR_ST BIT(6) > +#define CQ_APB_ERR_ST BIT(7) > +#define CQ_VS_ERR_ST BIT(8) > +#define AMX_ERR_ST BIT(15) > +#define RMX_ERR_ST BIT(16) > +#define BMX_ERR_ST BIT(17) > +#define RRZO_ERR_ST BIT(18) > +#define AFO_ERR_ST BIT(19) > +#define IMGO_ERR_ST BIT(20) > +#define AAO_ERR_ST BIT(21) > +#define PSO_ERR_ST BIT(22) > +#define LCSO_ERR_ST BIT(23) > +#define BNR_ERR_ST BIT(24) > +#define LSCI_ERR_ST BIT(25) > +#define DMA_ERR_ST BIT(29) > + > +/* CAM DMA done status */ > +#define FLKO_DONE_ST BIT(4) > +#define AFO_DONE_ST BIT(5) > +#define AAO_DONE_ST BIT(7) > +#define PSO_DONE_ST BIT(14) > + > +/* IRQ signal mask */ > +#define INT_ST_MASK_CAM ( \ > + VS_INT_ST |\ > + HW_PASS1_DON_ST |\ > + SOF_INT_ST |\ > + SW_PASS1_DON_ST) > + > +/* IRQ Warning Mask */ > +#define INT_ST_MASK_CAM_WARN (\ > + RRZO_ERR_ST |\ > + AFO_ERR_ST |\ > + IMGO_ERR_ST |\ > + AAO_ERR_ST |\ > + PSO_ERR_ST | \ > + LCSO_ERR_ST |\ > + BNR_ERR_ST |\ > + LSCI_ERR_ST) > + > +/* IRQ Error Mask */ > +#define INT_ST_MASK_CAM_ERR (\ > + TG_ERR_ST |\ > + TG_GBERR_ST |\ > + CQ_CODE_ERR_ST |\ > + CQ_APB_ERR_ST |\ > + CQ_VS_ERR_ST |\ > + BNR_ERR_ST |\ > + RMX_ERR_ST |\ > + BMX_ERR_ST |\ > + BNR_ERR_ST |\ > + LSCI_ERR_ST |\ > + DMA_ERR_ST) > + > +/* IRQ Signal Log Mask */ > +#define INT_ST_LOG_MASK_CAM (\ > + SOF_INT_ST |\ > + SW_PASS1_DON_ST |\ > + VS_INT_ST |\ > + TG_ERR_ST |\ > + TG_GBERR_ST |\ > + RRZO_ERR_ST |\ > + AFO_ERR_ST |\ > + IMGO_ERR_ST |\ > + AAO_ERR_ST |\ > + DMA_ERR_ST) > + > +/* DMA Event Notification Mask */ > +#define DMA_ST_MASK_CAM (\ > + AFO_DONE_ST |\ > + AAO_DONE_ST |\ > + PSO_DONE_ST |\ > + FLKO_DONE_ST) > + > +/* Status check */ > +#define REG_CTL_EN 0x0004 > +#define REG_CTL_DMA_EN 0x0008 > +#define REG_CTL_FMT_SEL 0x0010 > +#define REG_CTL_EN2 0x0018 > +#define REG_CTL_RAW_INT_EN 0x0020 > +#define REG_CTL_RAW_INT_STAT 0x0024 > +#define REG_CTL_RAW_INT2_STAT 0x0034 > +#define REG_CTL_RAW_INT3_STAT 0x00C4 nit: Please use lowercase hex literals. [snip] > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem-drv.c b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem-drv.c > new file mode 100644 > index 0000000..020c38c > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem-drv.c I don't think we need any of the code that is in this file. We should just use the DMA API. We should be able to create appropriate reserved memory pools in DT and properly assign them to the right allocating devices. Skipping review of this file for the time being. [snip] > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem.h b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem.h > new file mode 100644 > index 0000000..4e1cf20 > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem.h Ditto. [snip] > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2-util.c b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2-util.c > new file mode 100644 > index 0000000..7da312d > --- /dev/null > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2-util.c > @@ -0,0 +1,1555 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018 Mediatek Corporation. > + * Copyright (c) 2017 Intel Corporation. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License version > + * 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * MTK_CAM-v4l2 is highly based on Intel IPU 3 chrome driver To be precise, it's not a "chrome driver". I think you could just say "IPU3 ImgU driver". (The driver was merged in Linux 5.0.) > + * > + */ > + > +#include <linux/module.h> > +#include <linux/pm_runtime.h> > +#include <linux/videodev2.h> > +#include <media/v4l2-ioctl.h> > +#include <media/videobuf2-dma-contig.h> > +#include <media/v4l2-subdev.h> > +#include <media/v4l2-event.h> > +#include <media/v4l2-fwnode.h> > +#include <linux/device.h> > +#include <linux/platform_device.h> > +#include <linux/of.h> > +#include <linux/of_graph.h> > +#include <media/v4l2-common.h> > +#include <media/media-entity.h> > +#include <media/v4l2-async.h> > + > +#include "mtk_cam.h" > +#include "mtk_cam-dev.h" > +#include "mtk_cam-v4l2.h" > +#include "mtk_cam-v4l2-util.h" > + > +#define CONFIG_MEDIATEK_MEDIA_REQUEST I guess this is for testing, but I'd suggest switching to the target setup sooner than later and dropping this and related #ifdefs. > + > +#define MTK_CAM_SENSOR_MAIN_PAD_SRC 0 > +#define MTK_CAM_SENSOR_SUB_PAD_SRC 0 > +#define MTK_CAM_SENSOR_IF_PAD_MAIN_SINK 0 > +#define MTK_CAM_SENSOR_IF_PAD_SUB_SINK 1 > +#define MTK_CAM_SENSOR_IF_PAD_SRC 4 > +#define MTK_CAM_CIO_PAD_SINK 0 > + > +static u32 mtk_cam_node_get_v4l2_cap > + (struct mtk_cam_ctx_queue *node_ctx); > + > +static int mtk_cam_videoc_s_meta_fmt(struct file *file, > + void *fh, struct v4l2_format *f); > + > +static int mtk_cam_subdev_open(struct v4l2_subdev *sd, > + struct v4l2_subdev_fh *fh) > +{ > + struct mtk_cam_dev *isp_dev = mtk_cam_subdev_to_dev(sd); > + > + isp_dev->ctx.fh = fh; > + return mtk_cam_ctx_open(&isp_dev->ctx); > +} > + > +static int mtk_cam_subdev_close(struct v4l2_subdev *sd, > + struct v4l2_subdev_fh *fh) > +{ > + struct mtk_cam_dev *isp_dev = mtk_cam_subdev_to_dev(sd); > + > + return mtk_cam_ctx_release(&isp_dev->ctx); > +} > + > +static int mtk_cam_subdev_s_stream(struct v4l2_subdev *sd, > + int enable) > +{ > + int ret = 0; > + struct mtk_cam_dev *isp_dev = mtk_cam_subdev_to_dev(sd); > + struct mtk_cam_io_connection *cio = &isp_dev->cio; > + > + if (enable) { Please extract enable and disable cases into separate functions. > + /* Get sensor interace and sensor sub device */ > + /* If the call succeeds, sensor if and sensor are filled */ > + /* in isp_dev->cio->sensor_if and isp_dev->cio->sensor */ > + ret = mtk_cam_v4l2_discover_sensor(isp_dev); > + if (ret) { > + dev_err(&isp_dev->pdev->dev, > + "no sensor or sensor if connected (%d)\n", > + ret); > + return -EPERM; > + } > + > + /* seninf must stream on first */ > + ret = v4l2_subdev_call(cio->sensor_if, video, s_stream, 1); > + if (ret) { > + dev_err(&isp_dev->pdev->dev, > + "sensor-if(%s) stream on failed (%d)\n", > + cio->sensor_if->entity.name, ret); > + return -EPERM; > + } > + > + dev_dbg(&isp_dev->pdev->dev, "streamed on sensor-if(%s)\n", > + cio->sensor_if->entity.name); > + > + ret = v4l2_subdev_call(cio->sensor, video, s_stream, 1); > + if (ret) { > + dev_err(&isp_dev->pdev->dev, > + "sensor(%s) stream on failed (%d)\n", > + cio->sensor->entity.name, ret); > + return -EPERM; We should undo any previous operations. > + } > + > + dev_dbg(&isp_dev->pdev->dev, "streamed on sensor(%s)\n", > + cio->sensor->entity.name); > + > + ret = mtk_cam_ctx_streamon(&isp_dev->ctx); > + if (ret) { > + dev_err(&isp_dev->pdev->dev, > + "Pass 1 stream on failed (%d)\n", ret); > + return -EPERM; > + } > + > + isp_dev->mem2mem2.streaming = enable; > + > + ret = mtk_cam_dev_queue_buffers(isp_dev, true); > + if (ret) > + dev_err(&isp_dev->pdev->dev, > + "failed to queue initial buffers (%d)", ret); > + > + dev_dbg(&isp_dev->pdev->dev, "streamed on Pass 1\n"); > + } else { > + if (cio->sensor) { Is it possible to have cio->sensor NULL here? This function would have failed if it wasn't found when enabling. > + ret = v4l2_subdev_call(cio->sensor, video, s_stream, 0); > + if (ret) { > + dev_err(&isp_dev->pdev->dev, > + "sensor(%s) stream off failed (%d)\n", > + cio->sensor->entity.name, ret); > + } else { > + dev_dbg(&isp_dev->pdev->dev, > + "streamed off sensor(%s)\n", > + cio->sensor->entity.name); > + cio->sensor = NULL; > + } > + } else { > + dev_err(&isp_dev->pdev->dev, > + "Can't find sensor connected\n"); > + } > + > + if (cio->sensor_if) { Ditto. > + ret = v4l2_subdev_call(cio->sensor_if, video, > + s_stream, 0); > + if (ret) { > + dev_err(&isp_dev->pdev->dev, > + "sensor if(%s) stream off failed (%d)\n", > + cio->sensor_if->entity.name, ret); > + } else { > + dev_dbg(&isp_dev->pdev->dev, > + "streamed off sensor-if(%s)\n", > + cio->sensor_if->entity.name); > + cio->sensor_if = NULL; > + } > + } else { > + dev_err(&isp_dev->pdev->dev, > + "Can't find sensor-if connected\n"); > + } > + > + ret = mtk_cam_ctx_streamoff(&isp_dev->ctx); > + if (ret) { > + dev_err(&isp_dev->pdev->dev, > + "Pass 1 stream off failed (%d)\n", ret); > + return -EPERM; > + } > + > + isp_dev->mem2mem2.streaming = false; > + > + dev_dbg(&isp_dev->pdev->dev, "streamed off Pass 1\n"); > + } > + > + return 0; > +} > + > +static void v4l2_event_merge(const struct v4l2_event *old, > + struct v4l2_event *new) > +{ > + struct mtk_cam_dev_stat_event_data *old_evt_stat_data = > + (void *)old->u.data; > + struct mtk_cam_dev_stat_event_data *new_evt_stat_data = > + (void *)new->u.data; > + > + if (old->type == V4L2_EVENT_MTK_CAM_ENGINE_STATE && > + new->type == V4L2_EVENT_MTK_CAM_ENGINE_STATE) { > + pr_debug("%s, merge IRQ, old(type(0x%x) frame no(%d) IRQ(0x%x) DMA(0x%x)), new(type(0x%x) frame_number(%d) IRQ(0x%x) DMA(0x%x))", > + __func__, > + old->type, > + old_evt_stat_data->frame_number, > + old_evt_stat_data->irq_status_mask, > + old_evt_stat_data->dma_status_mask, > + new->type, > + new_evt_stat_data->frame_number, > + new_evt_stat_data->irq_status_mask, > + new_evt_stat_data->dma_status_mask); > + > + /* merge IRQ event */ > + new_evt_stat_data->irq_status_mask |= > + old_evt_stat_data->irq_status_mask; > + new_evt_stat_data->dma_status_mask |= > + old_evt_stat_data->dma_status_mask; > + } What are these custom events for? Userspace shouldn't need to know anything about IRQ and DMA. [snip] > +static void mtk_cam_vb2_buf_queue(struct vb2_buffer *vb) > +{ > + struct mtk_cam_mem2mem2_device *m2m2 = vb2_get_drv_priv(vb->vb2_queue); > + > + struct mtk_cam_dev *mtk_cam_dev = mtk_cam_m2m_to_dev(m2m2); > + > + struct device *dev = &mtk_cam_dev->pdev->dev; > + nit: Please remove these unnecessary blank lines between variables. > + struct mtk_cam_dev_buffer *buf = NULL; > + > + struct vb2_v4l2_buffer *v4l2_buf = NULL; Please default-initialize local variables only if really needed. Otherwise it prevents the compiler from catching missing assignments for you. > + > + struct mtk_cam_dev_video_device *node = > + mtk_cam_vbq_to_isp_node(vb->vb2_queue); > + > + int queue = mtk_cam_dev_get_queue_id_of_dev_node(mtk_cam_dev, node); Why not just add queue_id field to mtk_cam_dev_video_device? > + > + dev_dbg(dev, > + "queue vb2_buf: Node(%s) queue id(%d)\n", > + node->name, > + queue); > + > + if (queue < 0) { > + dev_err(m2m2->dev, "Invalid mtk_cam_dev node.\n"); > + return; > + } I don't think this is possible. > + > + if (!vb) > + pr_err("VB can't be null\n"); Ditto. > + > + buf = mtk_cam_vb2_buf_to_dev_buf(vb); > + > + if (!buf) > + pr_err("buf can't be null\n"); Ditto. > + > + v4l2_buf = to_vb2_v4l2_buffer(vb); > + > + if (!v4l2_buf) > + pr_err("v4l2_buf can't be null\n"); Ditto. > + > + mutex_lock(&mtk_cam_dev->lock); > + > + pr_debug("init mtk_cam_ctx_buf, sequence(%d)\n", v4l2_buf->sequence); > + > + /* the dma address will be filled in later frame buffer handling*/ > + mtk_cam_ctx_buf_init(&buf->ctx_buf, queue, (dma_addr_t)0); > + pr_debug("set mtk_cam_ctx_buf_init: user seq=%d\n", > + buf->ctx_buf.user_sequence); This should normally happen in the buf_init vb2 operation. By the way, what's the point of having the DMA address argument to mtk_cam_ctx_buf_init() if it's always 0? Given the above, is there even a reason for mtk_cam_ctx_buf_init() to exist? It sounds like the structure could be just initialize directly in buf_init. > + > + /* Added the buffer into the tracking list */ > + list_add_tail(&buf->m2m2_buf.list, > + &m2m2->nodes[node - m2m2->nodes].buffers); > + mutex_unlock(&mtk_cam_dev->lock); If this mutex is only for the buffer list, I'd consider replacing it with a spinlock. > + > + /* Enqueue the buffer */ > + if (mtk_cam_dev->mem2mem2.streaming) { > + pr_debug("%s: mtk_cam_dev_queue_buffers\n", > + node->name); > + mtk_cam_dev_queue_buffers(mtk_cam_dev, false); Doesn't mtk_cam_dev_queue_buffers() already check if streaming is active? Okay, let me send the comments above and continue third part of the review in another email. Best regards, Tomasz
On Thu, 2019-03-07 at 19:04 +0900, Tomasz Figa wrote: > Hi Frederic, Jungo, > > Sorry for making you wait. Here comes the second part. Please see my > comments inline. > > [snip] > > + > > +#define MTK_CAM_IO_CON_PADS (1) > > + > > +/* mtk_cam_io_connection --> sensor IF --> sensor 1 */ > > +/* --> sensor 2 */ > > nit: Comment style. > > Also, isn't the direction opposite? > Hi Tomasz: Thanks for your part 2's comment. Below is our feedback. For coding style issue, we will fix in next patch. > > +struct mtk_cam_io_connection { > > + const char *name; > > Do we need this name? Could we just use subdev.name instead? > > > + struct platform_device *pdev; > > This is assigned to mtk_cam_dev::pdev in the code and mtk_cam_dev is a > superclass of this struct, so we always have access to it and can get > pdev from it. No need to store it here as well. (And not sure why one > would normally need to access pdev. Perhaps storing the struct device > pointer inside mtk_cam_dev would be more practical?) > Yes, you are right. We will remove these two fields in next patch. > > + struct v4l2_subdev subdev; > > Do we need a subdev for an IO connection? Normally the processing part > of the ISP would be represented as a subdev and then it would have its > pads linked to other entities. > After internal discussion, we will revise the current CIO design. We will use the same ISP sub-dev to connect with sensor-IF without creating one new sub-device. > > + int enable; > > + /* sensor connected */ > > + struct v4l2_subdev *sensor; > > + /* sensor interface connected */ > > + struct v4l2_subdev *sensor_if; > > + struct media_pad subdev_pads[MTK_CAM_IO_CON_PADS]; > > + /* Current sensor input format*/ > > + struct v4l2_mbus_framefmt subdev_fmt; > > To summarize, it sounds like only "enable", "sensor", "sensor_if" and > "subdev_fmt" would be appropriate attributes for an "IO connection". > All the rest would normally be considered to be a part of the ISP > device (and "pdev" and "name" are redundant). > After simplifying, we will only keep "enable", "sensor" and "sensor_f" for an "IO connection" as below: /* * struct mtk_cam_io_connection - CIO (Camera IO) information. * * @enabled: indicate stream on or off * @sensor: sensor sub-device * @sensor_if: sensor_if sub-device * * Below is the graph topology. * sensor 1 (main) --> sensor IF --> P1 sub-device * sensor 2 (sub) --> * */ struct mtk_cam_io_connection { int enabled; struct v4l2_subdev *sensor; struct v4l2_subdev *sensor_if; }; > > +}; > > + > > +struct mtk_cam_dev_video_device { > > nit: Why not just mtk_cam_video_device? > OK, rename in next patch. > > + const char *name; > > Why not use vdev.name? > OK, remove this field and change to use vdev.name in next patch. > > + int output; > > Already available in vdev.vfl_dir, although not sure why it would be > normally needed. > OK, remove this field in next patch. > > + int immutable; > It is removed based on comment part 1's change. > Only master queue seems to be immutable, so maybe just check if the > queue is master, wherever this is used currently? > > > + int enabled; > > + int queued; > > Looks unused. > For "enabled", it indicates the video device is enabled or not during the media link setup phase. Based on this information, we could know how many video devices (DMAs) are enabled. So we will keep this. For "queued", we will remove in next patch. > > + struct v4l2_format vdev_fmt; > > + struct video_device vdev; > > + struct media_pad vdev_pad; > > + struct v4l2_mbus_framefmt pad_fmt; > > + struct vb2_queue vbq; > > + struct list_head buffers; > > + struct mutex lock; /* vb2 queue and video device data protection */ > > It's actually about operations, not data. I'd write it "Serializes vb2 > queue and video device operations.". > Ok, we will fix the comment wording to clarify the correct usage. Below is the revised structure. /* * struct mtk_cam_video_device - Mediatek video device structure. * * @enabled: indicate stream on or off * @queue_id: queue id for mtk_cam_ctx_queue * @pending_list: list for pending buffer * @lock: serializes vb2 queue and video device operations. * @slock: protect for pending_list. * */ struct mtk_cam_video_device { int enabled; int queue_id; struct v4l2_format vdev_fmt; struct video_device vdev; struct media_pad vdev_pad; struct vb2_queue vbq; struct list_head pending_list; /* Used for vbq & vdev */ struct mutex lock; /* protect for pending_list */ spinlock_t slock; }; > > + atomic_t sequence; > > +}; > > + > > +struct mtk_cam_mem2mem2_device { > > + const char *name; > > + const char *model; > > For both of the fields above, they seem to be always > MTK_CAM_DEV_P1_NAME, so we can just use the macro directly whenever > needed. No need for this indirection. > OK. These two fields will be removed in next patch. > > + struct device *dev; > > + int num_nodes; > > + struct mtk_cam_dev_video_device *nodes; > > + const struct vb2_mem_ops *vb2_mem_ops; > > This is always "vb2_dma_contig_memops", so it can be used directly. > Ditto. > > + unsigned int buf_struct_size; > > This is always sizeof(struct mtk_cam_dev_buffer), so no need to save > it in the struct. > Ditto. > > + int streaming; > > + struct v4l2_device *v4l2_dev; > > + struct media_device *media_dev; > > These 2 fields are already in mtk_cam_dev which is a superclass of > this struct. One can just access them from there directly. > Ditto. > > + struct media_pipeline pipeline; > > + struct v4l2_subdev subdev; > > Could you remind me what was the media topology exposed by this > driver? This is already the second subdev I spotted in this patch, > which looks strange. > For sub-device design, we will remove the sub-device for CIO and keep only one sub-device for ISP driver in next patch. We will also provide the media topology in RFC v1 patch to clarify. > > + struct media_pad *subdev_pads; > > + struct v4l2_file_operations v4l2_file_ops; > > + const struct file_operations fops; > > +}; > > Given most of the comments above, it looks like the remaining useful > fields in this struct could be just moved to mtk_cam_dev, without the > need for this separate struct. > This is the final revision for these two structures. Do you suggest to merge it to simplify? struct mtk_cam_mem2mem2_device { struct mtk_cam_video_device *nodes; struct media_pipeline pipeline; struct v4l2_subdev subdev; struct media_pad *subdev_pads; }; struct mtk_cam_dev { struct platform_device *pdev; struct mtk_cam_video_device mem2mem2_nodes[MTK_CAM_DEV_NODE_MAX]; struct mtk_cam_mem2mem2_device mem2mem2; struct mtk_cam_io_connection cio; struct v4l2_device v4l2_dev; struct media_device media_dev; struct mtk_cam_ctx ctx; struct v4l2_async_notifier notifier; }; > > + > > +struct mtk_cam_dev { > > + struct platform_device *pdev; > > + struct mtk_cam_dev_video_device mem2mem2_nodes[MTK_CAM_DEV_NODE_MAX]; > > + int queue_enabled[MTK_CAM_DEV_NODE_MAX]; > > Isn't this redundant with struct mtk_cam_dev_video_device::enabled? > Yes, we have removed this. > > + struct mtk_cam_mem2mem2_device mem2mem2; > > + struct mtk_cam_io_connection cio; > > + struct v4l2_device v4l2_dev; > > + struct media_device media_dev; > > + struct mtk_cam_ctx ctx; > > + struct v4l2_async_notifier notifier; > > + struct mutex lock; /* device level data protection */ > > Just a side note: As a rule of thumb, mutex is normally used to > protect operations not data (with some exceptions). For simple data > protection, where accesses would normally take very short time, > spinlock would be used. Of course a mutex would technically work too, > so let's keep it for now and we can optimize later. > After review, the mutex is useless and will remove it in next patch. Btw, thanks for your explanation the difference between mutex & spinlock. > > +}; > > + > > +int mtk_cam_media_register(struct device *dev, > > + struct media_device *media_dev, > > + const char *model); > > +int mtk_cam_v4l2_register(struct device *dev, > > + struct media_device *media_dev, > > + struct v4l2_device *v4l2_dev, > > + struct v4l2_ctrl_handler *ctrl_handler); > > +int mtk_cam_v4l2_unregister(struct mtk_cam_dev *dev); > > +int mtk_cam_mem2mem2_v4l2_register(struct mtk_cam_dev *dev, > > + struct media_device *media_dev, > > + struct v4l2_device *v4l2_dev); > > + > > +int mtk_cam_v4l2_async_register(struct mtk_cam_dev *isp_dev); > > + > > +void mtk_cam_v4l2_async_unregister(struct mtk_cam_dev *isp_dev); > > + > > +int mtk_cam_v4l2_discover_sensor(struct mtk_cam_dev *isp_dev); > > + > > +void mtk_cam_v4l2_buffer_done(struct vb2_buffer *vb, > > + enum vb2_buffer_state state); > > +extern int mtk_cam_dev_queue_buffers > > + (struct mtk_cam_dev *dev, int initial); > > Remove extern (here and other places too). > This coding style issue has been fixed in comment part 1. > > +extern int mtk_cam_dev_get_total_node > > + (struct mtk_cam_dev *mtk_cam_dev); > > +extern char *mtk_cam_dev_get_node_name > > + (struct mtk_cam_dev *mtk_cam_dev_obj, int node); > > +int mtk_cam_dev_init(struct mtk_cam_dev *isp_dev, > > + struct platform_device *pdev, > > + struct media_device *media_dev, > > + struct v4l2_device *v4l2_dev); > > +extern void mtk_cam_dev_mem2mem2_exit > > + (struct mtk_cam_dev *mtk_cam_dev_obj); > > +int mtk_cam_dev_mem2mem2_init(struct mtk_cam_dev *isp_dev, > > + struct media_device *media_dev, > > + struct v4l2_device *v4l2_dev); > > +int mtk_cam_dev_get_queue_id_of_dev_node > > + (struct mtk_cam_dev *mtk_cam_dev_obj, > > + struct mtk_cam_dev_video_device *node); > > +int mtk_cam_dev_core_init(struct platform_device *pdev, > > + struct mtk_cam_dev *isp_dev, > > + struct mtk_cam_ctx_desc *ctx_desc); > > +int mtk_cam_dev_core_init_ext(struct platform_device *pdev, > > + struct mtk_cam_dev *isp_dev, > > + struct mtk_cam_ctx_desc *ctx_desc, > > + struct media_device *media_dev, > > + struct v4l2_device *v4l2_dev); > > +extern int mtk_cam_dev_core_release > > +(struct platform_device *pdev, struct mtk_cam_dev *isp_dev); > > nit: Strange line break. Some more usual examples: > > int mtk_cam_dev_core_release(struct platform_device *pdev, > struct mtk_cam_dev *isp_dev); > > struct long_struct_name *mtk_cam_dev_core_release( > struct platform_device *pdev, > struct mtk_cam_dev *isp_dev); > > struct very_very_very_long_struct_name * > my_function(int a, int b); > Thanks for your comment. We will revise our coding style based on your hint. > > + > > +#endif /* __MTK_CAM_DEV_H__ */ > > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-regs.h b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-regs.h > > new file mode 100644 > > index 0000000..b5067d6 > > --- /dev/null > > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-regs.h > > @@ -0,0 +1,146 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (c) 2018 MediaTek Inc. > > + * Author: Ryan Yu <ryan.yu@mediatek.com> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#ifndef _CAM_REGS_H > > +#define _CAM_REGS_H > > + > > +/* TG Bit Mask */ > > +#define VFDATA_EN_BIT BIT(0) > > +#define CMOS_EN_BIT BIT(0) > > + > > +/* normal signal bit */ > > +#define VS_INT_ST BIT(0) > > +#define HW_PASS1_DON_ST BIT(11) > > +#define SOF_INT_ST BIT(12) > > +#define SW_PASS1_DON_ST BIT(30) > > + > > +/* err status bit */ > > +#define TG_ERR_ST BIT(4) > > +#define TG_GBERR_ST BIT(5) > > +#define CQ_CODE_ERR_ST BIT(6) > > +#define CQ_APB_ERR_ST BIT(7) > > +#define CQ_VS_ERR_ST BIT(8) > > +#define AMX_ERR_ST BIT(15) > > +#define RMX_ERR_ST BIT(16) > > +#define BMX_ERR_ST BIT(17) > > +#define RRZO_ERR_ST BIT(18) > > +#define AFO_ERR_ST BIT(19) > > +#define IMGO_ERR_ST BIT(20) > > +#define AAO_ERR_ST BIT(21) > > +#define PSO_ERR_ST BIT(22) > > +#define LCSO_ERR_ST BIT(23) > > +#define BNR_ERR_ST BIT(24) > > +#define LSCI_ERR_ST BIT(25) > > +#define DMA_ERR_ST BIT(29) > > + > > +/* CAM DMA done status */ > > +#define FLKO_DONE_ST BIT(4) > > +#define AFO_DONE_ST BIT(5) > > +#define AAO_DONE_ST BIT(7) > > +#define PSO_DONE_ST BIT(14) > > + > > +/* IRQ signal mask */ > > +#define INT_ST_MASK_CAM ( \ > > + VS_INT_ST |\ > > + HW_PASS1_DON_ST |\ > > + SOF_INT_ST |\ > > + SW_PASS1_DON_ST) > > + > > +/* IRQ Warning Mask */ > > +#define INT_ST_MASK_CAM_WARN (\ > > + RRZO_ERR_ST |\ > > + AFO_ERR_ST |\ > > + IMGO_ERR_ST |\ > > + AAO_ERR_ST |\ > > + PSO_ERR_ST | \ > > + LCSO_ERR_ST |\ > > + BNR_ERR_ST |\ > > + LSCI_ERR_ST) > > + > > +/* IRQ Error Mask */ > > +#define INT_ST_MASK_CAM_ERR (\ > > + TG_ERR_ST |\ > > + TG_GBERR_ST |\ > > + CQ_CODE_ERR_ST |\ > > + CQ_APB_ERR_ST |\ > > + CQ_VS_ERR_ST |\ > > + BNR_ERR_ST |\ > > + RMX_ERR_ST |\ > > + BMX_ERR_ST |\ > > + BNR_ERR_ST |\ > > + LSCI_ERR_ST |\ > > + DMA_ERR_ST) > > + > > +/* IRQ Signal Log Mask */ > > +#define INT_ST_LOG_MASK_CAM (\ > > + SOF_INT_ST |\ > > + SW_PASS1_DON_ST |\ > > + VS_INT_ST |\ > > + TG_ERR_ST |\ > > + TG_GBERR_ST |\ > > + RRZO_ERR_ST |\ > > + AFO_ERR_ST |\ > > + IMGO_ERR_ST |\ > > + AAO_ERR_ST |\ > > + DMA_ERR_ST) > > + > > +/* DMA Event Notification Mask */ > > +#define DMA_ST_MASK_CAM (\ > > + AFO_DONE_ST |\ > > + AAO_DONE_ST |\ > > + PSO_DONE_ST |\ > > + FLKO_DONE_ST) > > + > > +/* Status check */ > > +#define REG_CTL_EN 0x0004 > > +#define REG_CTL_DMA_EN 0x0008 > > +#define REG_CTL_FMT_SEL 0x0010 > > +#define REG_CTL_EN2 0x0018 > > +#define REG_CTL_RAW_INT_EN 0x0020 > > +#define REG_CTL_RAW_INT_STAT 0x0024 > > +#define REG_CTL_RAW_INT2_STAT 0x0034 > > +#define REG_CTL_RAW_INT3_STAT 0x00C4 > > nit: Please use lowercase hex literals. > > [snip] Ok, we will fix this coding style issue in next patch. > > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem-drv.c b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem-drv.c > > new file mode 100644 > > index 0000000..020c38c > > --- /dev/null > > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem-drv.c > > I don't think we need any of the code that is in this file. We should > just use the DMA API. We should be able to create appropriate reserved > memory pools in DT and properly assign them to the right allocating > devices. > > Skipping review of this file for the time being. > For this file, we may need your help. Its purpose is same as DIP SMEM driver. It is used for creating the ISP P1 specific vb2 buffer allocation context with reserved memory. Unfortunately, the implementation of mtk_cam-smem-drive.c is our best solution now. Could you give us more hints how to implement? Or do you think we could leverage the implementation from "Samsung S5P Multi Format Codec driver"? drivers/media/platform/s5p-mfc/s5p_mfc.c - s5p_mfc_configure_dma_memory function - s5p_mfc_configure_2port_memory - s5p_mfc_alloc_memdev > [snip] > > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem.h b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem.h > > new file mode 100644 > > index 0000000..4e1cf20 > > --- /dev/null > > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem.h > > Ditto. > > [snip] > > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2-util.c b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2-util.c > > new file mode 100644 > > index 0000000..7da312d > > --- /dev/null > > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2-util.c > > @@ -0,0 +1,1555 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2018 Mediatek Corporation. > > + * Copyright (c) 2017 Intel Corporation. > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License version > > + * 2 as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * MTK_CAM-v4l2 is highly based on Intel IPU 3 chrome driver > > To be precise, it's not a "chrome driver". I think you could just say > "IPU3 ImgU driver". (The driver was merged in Linux 5.0.) > OK, we will revise our wording. > > + * > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/videodev2.h> > > +#include <media/v4l2-ioctl.h> > > +#include <media/videobuf2-dma-contig.h> > > +#include <media/v4l2-subdev.h> > > +#include <media/v4l2-event.h> > > +#include <media/v4l2-fwnode.h> > > +#include <linux/device.h> > > +#include <linux/platform_device.h> > > +#include <linux/of.h> > > +#include <linux/of_graph.h> > > +#include <media/v4l2-common.h> > > +#include <media/media-entity.h> > > +#include <media/v4l2-async.h> > > + > > +#include "mtk_cam.h" > > +#include "mtk_cam-dev.h" > > +#include "mtk_cam-v4l2.h" > > +#include "mtk_cam-v4l2-util.h" > > + > > +#define CONFIG_MEDIATEK_MEDIA_REQUEST > > I guess this is for testing, but I'd suggest switching to the target > setup sooner than later and dropping this and related #ifdefs. > Yes, it is testing purpose and has been removed in our latest codebase. > > + > > +#define MTK_CAM_SENSOR_MAIN_PAD_SRC 0 > > +#define MTK_CAM_SENSOR_SUB_PAD_SRC 0 > > +#define MTK_CAM_SENSOR_IF_PAD_MAIN_SINK 0 > > +#define MTK_CAM_SENSOR_IF_PAD_SUB_SINK 1 > > +#define MTK_CAM_SENSOR_IF_PAD_SRC 4 > > +#define MTK_CAM_CIO_PAD_SINK 0 > > + > > +static u32 mtk_cam_node_get_v4l2_cap > > + (struct mtk_cam_ctx_queue *node_ctx); > > + > > +static int mtk_cam_videoc_s_meta_fmt(struct file *file, > > + void *fh, struct v4l2_format *f); > > + > > +static int mtk_cam_subdev_open(struct v4l2_subdev *sd, > > + struct v4l2_subdev_fh *fh) > > +{ > > + struct mtk_cam_dev *isp_dev = mtk_cam_subdev_to_dev(sd); > > + > > + isp_dev->ctx.fh = fh; > > + return mtk_cam_ctx_open(&isp_dev->ctx); > > +} > > + > > +static int mtk_cam_subdev_close(struct v4l2_subdev *sd, > > + struct v4l2_subdev_fh *fh) > > +{ > > + struct mtk_cam_dev *isp_dev = mtk_cam_subdev_to_dev(sd); > > + > > + return mtk_cam_ctx_release(&isp_dev->ctx); > > +} > > + > > +static int mtk_cam_subdev_s_stream(struct v4l2_subdev *sd, > > + int enable) > > +{ > > + int ret = 0; > > + struct mtk_cam_dev *isp_dev = mtk_cam_subdev_to_dev(sd); > > + struct mtk_cam_io_connection *cio = &isp_dev->cio; > > + > > + if (enable) { > > Please extract enable and disable cases into separate functions. > Ok, we will revise current implementation as below: static int mtk_cam_subdev_s_stream(struct v4l2_subdev *sd, int enable) { struct mtk_cam_dev *isp_dev = mtk_cam_subdev_to_dev(sd); struct mtk_cam_io_connection *cio = &isp_dev->cio; if (enable) return mtk_cam_cio_stream_on(isp_dev, cio); else return mtk_cam_cio_stream_off(isp_dev, cio); } > > + /* Get sensor interace and sensor sub device */ > > + /* If the call succeeds, sensor if and sensor are filled */ > > + /* in isp_dev->cio->sensor_if and isp_dev->cio->sensor */ > > + ret = mtk_cam_v4l2_discover_sensor(isp_dev); > > + if (ret) { > > + dev_err(&isp_dev->pdev->dev, > > + "no sensor or sensor if connected (%d)\n", > > + ret); > > + return -EPERM; > > + } > > + > > + /* seninf must stream on first */ > > + ret = v4l2_subdev_call(cio->sensor_if, video, s_stream, 1); > > + if (ret) { > > + dev_err(&isp_dev->pdev->dev, > > + "sensor-if(%s) stream on failed (%d)\n", > > + cio->sensor_if->entity.name, ret); > > + return -EPERM; > > + } > > + > > + dev_dbg(&isp_dev->pdev->dev, "streamed on sensor-if(%s)\n", > > + cio->sensor_if->entity.name); > > + > > + ret = v4l2_subdev_call(cio->sensor, video, s_stream, 1); > > + if (ret) { > > + dev_err(&isp_dev->pdev->dev, > > + "sensor(%s) stream on failed (%d)\n", > > + cio->sensor->entity.name, ret); > > + return -EPERM; > > We should undo any previous operations. > OK, we will add "undo procedure" in the new function. > > + } > > + > > + dev_dbg(&isp_dev->pdev->dev, "streamed on sensor(%s)\n", > > + cio->sensor->entity.name); > > + > > + ret = mtk_cam_ctx_streamon(&isp_dev->ctx); > > + if (ret) { > > + dev_err(&isp_dev->pdev->dev, > > + "Pass 1 stream on failed (%d)\n", ret); > > + return -EPERM; > > + } > > + > > + isp_dev->mem2mem2.streaming = enable; > > + > > + ret = mtk_cam_dev_queue_buffers(isp_dev, true); > > + if (ret) > > + dev_err(&isp_dev->pdev->dev, > > + "failed to queue initial buffers (%d)", ret); > > + > > + dev_dbg(&isp_dev->pdev->dev, "streamed on Pass 1\n"); > > + } else { > > + if (cio->sensor) { > > Is it possible to have cio->sensor NULL here? This function would have > failed if it wasn't found when enabling. > In the original design, it is protected to avoid abnormal double stream off (s_stream) call from upper layer. For stability reason, it is better to check. > > + ret = v4l2_subdev_call(cio->sensor, video, s_stream, 0); > > + if (ret) { > > + dev_err(&isp_dev->pdev->dev, > > + "sensor(%s) stream off failed (%d)\n", > > + cio->sensor->entity.name, ret); > > + } else { > > + dev_dbg(&isp_dev->pdev->dev, > > + "streamed off sensor(%s)\n", > > + cio->sensor->entity.name); > > + cio->sensor = NULL; > > + } > > + } else { > > + dev_err(&isp_dev->pdev->dev, > > + "Can't find sensor connected\n"); > > + } > > + > > + if (cio->sensor_if) { > > Ditto. > > > + ret = v4l2_subdev_call(cio->sensor_if, video, > > + s_stream, 0); > > + if (ret) { > > + dev_err(&isp_dev->pdev->dev, > > + "sensor if(%s) stream off failed (%d)\n", > > + cio->sensor_if->entity.name, ret); > > + } else { > > + dev_dbg(&isp_dev->pdev->dev, > > + "streamed off sensor-if(%s)\n", > > + cio->sensor_if->entity.name); > > + cio->sensor_if = NULL; > > + } > > + } else { > > + dev_err(&isp_dev->pdev->dev, > > + "Can't find sensor-if connected\n"); > > + } > > + > > + ret = mtk_cam_ctx_streamoff(&isp_dev->ctx); > > + if (ret) { > > + dev_err(&isp_dev->pdev->dev, > > + "Pass 1 stream off failed (%d)\n", ret); > > + return -EPERM; > > + } > > + > > + isp_dev->mem2mem2.streaming = false; > > + > > + dev_dbg(&isp_dev->pdev->dev, "streamed off Pass 1\n"); > > + } > > + > > + return 0; > > +} > > + > > +static void v4l2_event_merge(const struct v4l2_event *old, > > + struct v4l2_event *new) > > +{ > > + struct mtk_cam_dev_stat_event_data *old_evt_stat_data = > > + (void *)old->u.data; > > + struct mtk_cam_dev_stat_event_data *new_evt_stat_data = > > + (void *)new->u.data; > > + > > + if (old->type == V4L2_EVENT_MTK_CAM_ENGINE_STATE && > > + new->type == V4L2_EVENT_MTK_CAM_ENGINE_STATE) { > > + pr_debug("%s, merge IRQ, old(type(0x%x) frame no(%d) IRQ(0x%x) DMA(0x%x)), new(type(0x%x) frame_number(%d) IRQ(0x%x) DMA(0x%x))", > > + __func__, > > + old->type, > > + old_evt_stat_data->frame_number, > > + old_evt_stat_data->irq_status_mask, > > + old_evt_stat_data->dma_status_mask, > > + new->type, > > + new_evt_stat_data->frame_number, > > + new_evt_stat_data->irq_status_mask, > > + new_evt_stat_data->dma_status_mask); > > + > > + /* merge IRQ event */ > > + new_evt_stat_data->irq_status_mask |= > > + old_evt_stat_data->irq_status_mask; > > + new_evt_stat_data->dma_status_mask |= > > + old_evt_stat_data->dma_status_mask; > > + } > > What are these custom events for? Userspace shouldn't need to know > anything about IRQ and DMA. > We will remove custom event and change to use V4L2_EVENT_FRAME_SYNC event. > [snip] > > +static void mtk_cam_vb2_buf_queue(struct vb2_buffer *vb) > > +{ > > + struct mtk_cam_mem2mem2_device *m2m2 = vb2_get_drv_priv(vb->vb2_queue); > > + > > + struct mtk_cam_dev *mtk_cam_dev = mtk_cam_m2m_to_dev(m2m2); > > + > > + struct device *dev = &mtk_cam_dev->pdev->dev; > > + > > nit: Please remove these unnecessary blank lines between variables. > Ok, we will fix this kind of coding style issue in next patch. > > + struct mtk_cam_dev_buffer *buf = NULL; > > + > > + struct vb2_v4l2_buffer *v4l2_buf = NULL; > > Please default-initialize local variables only if really needed. > Otherwise it prevents the compiler from catching missing assignments > for you. > Ditto. > > + > > + struct mtk_cam_dev_video_device *node = > > + mtk_cam_vbq_to_isp_node(vb->vb2_queue); > > + > > + int queue = mtk_cam_dev_get_queue_id_of_dev_node(mtk_cam_dev, node); > > Why not just add queue_id field to mtk_cam_dev_video_device? > Yes, it is a good idea. We will add this field for mtk_cam_dev_video_device. > > + > > + dev_dbg(dev, > > + "queue vb2_buf: Node(%s) queue id(%d)\n", > > + node->name, > > + queue); > > + > > + if (queue < 0) { > > + dev_err(m2m2->dev, "Invalid mtk_cam_dev node.\n"); > > + return; > > + } > > I don't think this is possible. > Ok, we will remove this kind of logic check. > > + > > + if (!vb) > > + pr_err("VB can't be null\n"); > > Ditto. > > > + > > + buf = mtk_cam_vb2_buf_to_dev_buf(vb); > > + > > + if (!buf) > > + pr_err("buf can't be null\n"); > > Ditto. > > > + > > + v4l2_buf = to_vb2_v4l2_buffer(vb); > > + > > + if (!v4l2_buf) > > + pr_err("v4l2_buf can't be null\n"); > > Ditto. > > > + > > + mutex_lock(&mtk_cam_dev->lock); > > + > > + pr_debug("init mtk_cam_ctx_buf, sequence(%d)\n", v4l2_buf->sequence); > > + > > + /* the dma address will be filled in later frame buffer handling*/ > > + mtk_cam_ctx_buf_init(&buf->ctx_buf, queue, (dma_addr_t)0); > > + pr_debug("set mtk_cam_ctx_buf_init: user seq=%d\n", > > + buf->ctx_buf.user_sequence); > > This should normally happen in the buf_init vb2 operation. > > By the way, what's the point of having the DMA address argument to > mtk_cam_ctx_buf_init() if it's always 0? > > Given the above, is there even a reason for mtk_cam_ctx_buf_init() to > exist? It sounds like the structure could be just initialize directly > in buf_init. > Based on comment part 1, we have revised this implementation. There is no need to perform buffer initialization in this function. > > + > > + /* Added the buffer into the tracking list */ > > + list_add_tail(&buf->m2m2_buf.list, > > + &m2m2->nodes[node - m2m2->nodes].buffers); > > + mutex_unlock(&mtk_cam_dev->lock); > > If this mutex is only for the buffer list, I'd consider replacing it > with a spinlock. > Yes, in the new version, we change to use spinlock to protect list data, not mutex lock. e.g. buf = mtk_cam_vb2_buf_to_dev_buf(vb); /* Added the buffer into the tracking list */ spin_lock(&node->slock); list_add_tail(&buf->list, &node->pending_list); spin_unlock(&node->slock); > > + > > + /* Enqueue the buffer */ > > + if (mtk_cam_dev->mem2mem2.streaming) { > > + pr_debug("%s: mtk_cam_dev_queue_buffers\n", > > + node->name); > > + mtk_cam_dev_queue_buffers(mtk_cam_dev, false); > > Doesn't mtk_cam_dev_queue_buffers() already check if streaming is active? > Yes, we could remove the logic check in this function and count on the checking of mtk_cam_dev_queue_buffers() function. > Okay, let me send the comments above and continue third part of the > review in another email. > > Best regards, > Tomasz Thanks for your valued comments on part 2. It is helpful for us to make our driver implementation better. We'd like to know your opinion about the schedule for RFC V1. Do you suggest us to send RFC V1 patch set after revising all comments on part 1 & 2 or wait for part 3 review? Best regards, Jungo
Hi Frederic, Jungo, Please see more comments inline. > +static int mtk_cam_vb2_queue_setup(struct vb2_queue *vq, > + unsigned int *num_buffers, > + unsigned int *num_planes, > + unsigned int sizes[], > + struct device *alloc_devs[]) > +{ > + struct mtk_cam_mem2mem2_device *m2m2 = vb2_get_drv_priv(vq); > + struct mtk_cam_dev_video_device *node = > + mtk_cam_vbq_to_isp_node(vq); > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > + struct device *dev = &isp_dev->pdev->dev; > + void *buf_alloc_ctx = NULL; Don't initialize by default, if not strictly necessary. > + int queue_id = mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > + > + /* Get V4L2 format with the following method */ > + const struct v4l2_format *fmt = &node->vdev_fmt; > + > + *num_planes = 1; This doesn't handle VIDIOC_CREATE_BUFS, which triggers a .queue_setup() call with *num_planes > 0 and sizes[] already initialized. The driver needs to validate that the sizes and number of planes are valid in that case and return -EINVAL otherwise. Perhaps you should try running v4l2-compliance (https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance) on this driver, which should catch issues like this. > + *num_buffers = clamp_val(*num_buffers, 1, VB2_MAX_FRAME); > + > + if (isp_dev->ctx.queue[queue_id].desc.smem_alloc) { > + buf_alloc_ctx = isp_dev->ctx.smem_vb2_alloc_ctx; > + dev_dbg(dev, "Select smem_vb2_alloc_ctx(%llx)\n", > + (unsigned long long)buf_alloc_ctx); Use %p for printing pointers. > + } else { > + buf_alloc_ctx = isp_dev->ctx.default_vb2_alloc_ctx; > + dev_dbg(dev, "Select default_vb2_alloc_ctx(%llx)\n", > + (unsigned long long)buf_alloc_ctx); > + } > + > + vq->dma_attrs |= DMA_ATTR_NON_CONSISTENT; > + dev_dbg(dev, "queue(%d): cached mmap enabled\n", queue_id); This isn't supported in upstream. (By the way, neither it is in Chrome OS 4.19 kernel. If we really need cached mmap for some reason, we should propose a proper solution upstream. I'd still first investigate why write-combine mmap is slow for operations that should be simple one-time writes or reads.) > + > + if (vq->type == V4L2_BUF_TYPE_META_CAPTURE || > + vq->type == V4L2_BUF_TYPE_META_OUTPUT) > + sizes[0] = fmt->fmt.meta.buffersize; > + else > + sizes[0] = fmt->fmt.pix_mp.plane_fmt[0].sizeimage; > + > + alloc_devs[0] = (struct device *)buf_alloc_ctx; Please don't add random type casts. If the compiler gives a type error, that normally means that there is something wrong elsewhere in the code (i.e. the type of buf_alloc_ctx variable is wrong here - it should be struct device *) and casting just masks the original problem. > + > + dev_dbg(dev, "queue(%d):type(%d),size(%d),alloc_ctx(%llx)\n", > + queue_id, vq->type, sizes[0], > + (unsigned long long)buf_alloc_ctx); > + > + /* Initialize buffer queue */ > + INIT_LIST_HEAD(&node->buffers); This is incorrect. .queue_setup() can be also called on VIDIOC_CREATE_BUFS, which must preserve the other buffers in the queue. > + > + return 0; > +} [snip] > +static int mtk_cam_vb2_start_streaming(struct vb2_queue *vq, unsigned int count) > +{ > + struct mtk_cam_mem2mem2_device *m2m2 = vb2_get_drv_priv(vq); > + struct mtk_cam_dev_video_device *node = > + mtk_cam_vbq_to_isp_node(vq); > + int r; nit: "ret" is more common and already used by some other functions in this patch. > + > + if (m2m2->streaming) { > + r = -EBUSY; > + goto fail_return_bufs; > + } We shouldn't need to check this ourselves. It's not possible to have this call on an already streaming vb2 queue. Since we start streaming the m2m2 subdev only when all video nodes start streaming, this should never happen. > + > + if (!node->enabled) { > + pr_err("Node (%ld) is not enable\n", node - m2m2->nodes); > + r = -EINVAL; > + goto fail_return_bufs; > + } > + > + r = media_pipeline_start(&node->vdev.entity, &m2m2->pipeline); > + if (r < 0) { > + pr_err("Node (%ld) media_pipeline_start failed\n", > + node - m2m2->nodes); > + goto fail_return_bufs; > + } > + > + if (!mtk_cam_all_nodes_streaming(m2m2, node)) > + return 0; > + > + /* Start streaming of the whole pipeline now */ > + > + r = v4l2_subdev_call(&m2m2->subdev, video, s_stream, 1); > + if (r < 0) { > + pr_err("Node (%ld) v4l2_subdev_call s_stream failed\n", > + node - m2m2->nodes); > + goto fail_stop_pipeline; > + } > + return 0; > + > +fail_stop_pipeline: > + media_pipeline_stop(&node->vdev.entity); > +fail_return_bufs: > + mtk_cam_return_all_buffers(m2m2, node, VB2_BUF_STATE_QUEUED); > + > + return r; > +} > + > +static void mtk_cam_vb2_stop_streaming(struct vb2_queue *vq) > +{ > + struct mtk_cam_mem2mem2_device *m2m2 = vb2_get_drv_priv(vq); > + struct mtk_cam_dev_video_device *node = > + mtk_cam_vbq_to_isp_node(vq); > + int r; > + > + WARN_ON(!node->enabled); > + > + /* Was this the first node with streaming disabled? */ > + if (mtk_cam_all_nodes_streaming(m2m2, node)) { > + /* Yes, really stop streaming now */ > + r = v4l2_subdev_call(&m2m2->subdev, video, s_stream, 0); > + if (r) > + dev_err(m2m2->dev, "failed to stop streaming\n"); > + } > + > + mtk_cam_return_all_buffers(m2m2, node, VB2_BUF_STATE_ERROR); > + media_pipeline_stop(&node->vdev.entity); > +} > + > +static int mtk_cam_videoc_querycap(struct file *file, void *fh, > + struct v4l2_capability *cap) > +{ > + struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file); > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > + int queue_id = > + mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > + struct mtk_cam_ctx_queue *node_ctx = &isp_dev->ctx.queue[queue_id]; It feels like this could be just stored as node->ctx. > + > + strlcpy(cap->driver, m2m2->name, sizeof(cap->driver)); > + strlcpy(cap->card, m2m2->model, sizeof(cap->card)); > + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", > + node->name); > + > + cap->device_caps = > + mtk_cam_node_get_v4l2_cap(node_ctx) | V4L2_CAP_STREAMING; > + cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS; No need to set these 2 fields manually. They are filled in automatically from struct video_device::device_caps. > + > + return 0; > +} > + > +/* Propagate forward always the format from the mtk_cam_dev subdev */ It doesn't seem to match what the function is doing, i.e. returning the format structure of the node itself. I'd just drop this comment. The code should be written in a self-explaining way anyway. > +static int mtk_cam_videoc_g_fmt(struct file *file, void *fh, > + struct v4l2_format *f) > +{ > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > + > + f->fmt = node->vdev_fmt.fmt; > + > + return 0; > +} > + > +static int mtk_cam_videoc_try_fmt(struct file *file, > + void *fh, > + struct v4l2_format *f) > +{ > + struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file); > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > + struct mtk_cam_ctx *dev_ctx = &isp_dev->ctx; > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > + int queue_id = > + mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > + int ret = 0; > + > + ret = mtk_cam_ctx_fmt_set_img(dev_ctx, queue_id, > + &f->fmt.pix_mp, > + &node->vdev_fmt.fmt.pix_mp); Doesn't this actually change the current node format? VIDIOC_TRY_FMT must not have any side effects on current driver state. > + > + /* Simply set the format to the node context in the initial version */ > + if (ret) { > + pr_warn("Fmt(%d) not support for queue(%d), will load default fmt\n", > + f->fmt.pix_mp.pixelformat, queue_id); No need for this warning. > + > + ret = mtk_cam_ctx_format_load_default_fmt > + (&dev_ctx->queue[queue_id], f); Wouldn't this also change the current node state? Also, something wrong with the number of spaces after "ret =". > + } > + > + if (!ret) { > + node->vdev_fmt.fmt.pix_mp = f->fmt.pix_mp; > + dev_ctx->queue[queue_id].fmt.pix_mp = node->vdev_fmt.fmt.pix_mp; Ditto. > + } > + > + return ret; VIDIOC_TRY_FMT must not return an error unless for the cases described in https://www.kernel.org/doc/html/latest/media/uapi/v4l/vidioc-g-fmt.html#return-value . > +} > + > +static int mtk_cam_videoc_s_fmt(struct file *file, void *fh, > + struct v4l2_format *f) > +{ > + struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file); > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > + struct mtk_cam_ctx *dev_ctx = &isp_dev->ctx; > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > + int queue_id = mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > + int ret = 0; > + > + ret = mtk_cam_ctx_fmt_set_img(dev_ctx, queue_id, > + &f->fmt.pix_mp, > + &node->vdev_fmt.fmt.pix_mp); > + > + /* Simply set the format to the node context in the initial version */ > + if (!ret) > + dev_ctx->queue[queue_id].fmt.pix_mp = node->vdev_fmt.fmt.pix_mp; > + else > + dev_warn(&isp_dev->pdev->dev, > + "s_fmt, format not support\n"); No need for error messages for userspace errors. > + > + return ret; Instead of opencoding most of this function, one would normally call mtk_cam_videoc_try_fmt() first to adjust the format struct and then just update the driver state with the adjusted format. Also, similarly to VIDIOC_TRY_FMT, VIDIOC_SET_FMT doesn't fail unless in the very specific cases, as described in https://www.kernel.org/doc/html/latest/media/uapi/v4l/vidioc-g-fmt.html#return-value . > +} > + > +static int mtk_cam_enum_framesizes(struct file *filp, void *priv, > + struct v4l2_frmsizeenum *sizes) > +{ > + struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(filp); > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > + struct mtk_cam_dev_video_device *node = > + file_to_mtk_cam_node(filp); > + int queue_id = mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > + > + if (sizes->index != 0) > + return -EINVAL; > + > + if (queue_id == MTK_CAM_CTX_P1_MAIN_STREAM_OUT) { > + sizes->type = V4L2_FRMSIZE_TYPE_CONTINUOUS; > + sizes->stepwise.max_width = CAM_B_MAX_WIDTH; > + sizes->stepwise.min_width = CAM_MIN_WIDTH; > + sizes->stepwise.max_height = CAM_B_MAX_HEIGHT; > + sizes->stepwise.min_height = CAM_MIN_HEIGHT; > + sizes->stepwise.step_height = 1; > + sizes->stepwise.step_width = 1; > + } else if (queue_id == MTK_CAM_CTX_P1_PACKED_BIN_OUT) { > + sizes->type = V4L2_FRMSIZE_TYPE_CONTINUOUS; > + sizes->stepwise.max_width = RRZ_MAX_WIDTH; > + sizes->stepwise.min_width = RRZ_MIN_WIDTH; > + sizes->stepwise.max_height = RRZ_MAX_HEIGHT; > + sizes->stepwise.min_height = RRZ_MIN_HEIGHT; > + sizes->stepwise.step_height = 1; > + sizes->stepwise.step_width = 1; > + } > + > + return 0; > +} > + > +static int mtk_cam_meta_enum_format(struct file *file, > + void *fh, struct v4l2_fmtdesc *f) > +{ > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > + > + if (f->index > 0 || f->type != node->vbq.type) > + return -EINVAL; f->type is already checked by the V4L2 core. See v4l_enum_fmt(). > + > + f->pixelformat = node->vdev_fmt.fmt.meta.dataformat; > + > + return 0; > +} > + > +static int mtk_cam_videoc_s_meta_fmt(struct file *file, > + void *fh, struct v4l2_format *f) > +{ > + struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file); > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > + struct mtk_cam_ctx *dev_ctx = &isp_dev->ctx; > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > + int queue_id = mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > + No need for this blank line. > + int ret = 0; Please don't default-initialize without a good reason. > + > + if (f->type != node->vbq.type) > + return -EINVAL; Ditto. > + > + ret = mtk_cam_ctx_format_load_default_fmt(&dev_ctx->queue[queue_id], f); > + No need for this blank line. > + if (!ret) { > + node->vdev_fmt.fmt.meta = f->fmt.meta; > + dev_ctx->queue[queue_id].fmt.meta = node->vdev_fmt.fmt.meta; > + } else { > + dev_warn(&isp_dev->pdev->dev, > + "s_meta_fm failed, format not support\n"); No need for this warning. > + } > + > + return ret; > +} Actually, why do we even need to do all the things? Do we support multiple different meta formats on the same video node? If not, we can just have all the TRY_FMT/S_FMT/G_FMT return the hardcoded format. > + > +static int mtk_cam_videoc_g_meta_fmt(struct file *file, > + void *fh, struct v4l2_format *f) > +{ > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > + > + if (f->type != node->vbq.type) > + return -EINVAL; Not needed. > + > + f->fmt = node->vdev_fmt.fmt; > + > + return 0; > +} > + > +int mtk_cam_videoc_qbuf(struct file *file, void *priv, struct v4l2_buffer *p) > +{ > + struct video_device *vdev = video_devdata(file); > + struct vb2_buffer *vb; > + struct mtk_cam_dev_buffer *db; > + int r = 0; > + > + /* check if vb2 queue is busy */ > + if (vdev->queue->owner && > + vdev->queue->owner != file->private_data) > + return -EBUSY; This should be already handled by the core. > + > + /* Keep the value of sequence in v4l2_buffer */ > + /* in ctx buf since vb2_qbuf will set it to 0 */ > + vb = vdev->queue->bufs[p->index]; Why do you need a sequence number for buffers on queue time? The field is not specified to be set by the userspace and should be ignored by the driver. The driver should rely on the Request API to match any buffers together anyway. > + > + if (vb) { > + db = mtk_cam_vb2_buf_to_dev_buf(vb); > + pr_debug("qbuf: p:%llx,vb:%llx, db:%llx\n", > + (unsigned long long)p, > + (unsigned long long)vb, > + (unsigned long long)db); > + db->ctx_buf.user_sequence = p->sequence; > + } > + Generally this driver shouldn't need to implement this callback itself. Instead it can just use the vb2_ioctl_qbuf() helper. > + r = vb2_qbuf(vdev->queue, vdev->v4l2_dev->mdev, p); > + > + if (r) > + pr_err("vb2_qbuf failed(err=%d): buf idx(%d)\n", > + r, p->index); > + > + return r; > +} > +EXPORT_SYMBOL_GPL(mtk_cam_videoc_qbuf); > + > +/******************** function pointers ********************/ > + > +/* subdev internal operations */ > +static const struct v4l2_subdev_internal_ops mtk_cam_subdev_internal_ops = { > + .open = mtk_cam_subdev_open, > + .close = mtk_cam_subdev_close, > +}; > + > +static const struct v4l2_subdev_core_ops mtk_cam_subdev_core_ops = { > + .subscribe_event = mtk_cam_subdev_subscribe_event, > + .unsubscribe_event = mtk_cam_subdev_unsubscribe_event, > +}; > + > +static const struct v4l2_subdev_video_ops mtk_cam_subdev_video_ops = { > + .s_stream = mtk_cam_subdev_s_stream, > +}; > + > +static const struct v4l2_subdev_ops mtk_cam_subdev_ops = { > + .core = &mtk_cam_subdev_core_ops, > + .video = &mtk_cam_subdev_video_ops, > +}; > + > +static const struct media_entity_operations mtk_cam_media_ops = { > + .link_setup = mtk_cam_link_setup, > + .link_validate = v4l2_subdev_link_validate, > +}; > + > +#ifdef CONFIG_MEDIATEK_MEDIA_REQUEST > +static void mtk_cam_vb2_buf_request_complete(struct vb2_buffer *vb) > +{ > + struct mtk_cam_mem2mem2_device *m2m2 = vb2_get_drv_priv(vb->vb2_queue); > + > + v4l2_ctrl_request_complete(vb->req_obj.req, > + m2m2->v4l2_dev->ctrl_handler); > +} > +#endif > + > +static const struct vb2_ops mtk_cam_vb2_ops = { > + .buf_queue = mtk_cam_vb2_buf_queue, > + .queue_setup = mtk_cam_vb2_queue_setup, > + .start_streaming = mtk_cam_vb2_start_streaming, > + .stop_streaming = mtk_cam_vb2_stop_streaming, > + .wait_prepare = vb2_ops_wait_prepare, > + .wait_finish = vb2_ops_wait_finish, > +#ifdef CONFIG_MEDIATEK_MEDIA_REQUEST > + .buf_request_complete = mtk_cam_vb2_buf_request_complete, > +#endif > +}; > + > +static const struct v4l2_file_operations mtk_cam_v4l2_fops = { > + .unlocked_ioctl = video_ioctl2, > + .open = v4l2_fh_open, > + .release = vb2_fop_release, > + .poll = vb2_fop_poll, > + .mmap = vb2_fop_mmap, > +#ifdef CONFIG_COMPAT > + .compat_ioctl32 = v4l2_compat_ioctl32, > +#endif > +}; > + > +static const struct v4l2_ioctl_ops mtk_cam_v4l2_ioctl_ops = { > + .vidioc_querycap = mtk_cam_videoc_querycap, > + .vidioc_enum_framesizes = mtk_cam_enum_framesizes, > + > + .vidioc_g_fmt_vid_cap_mplane = mtk_cam_videoc_g_fmt, > + .vidioc_s_fmt_vid_cap_mplane = mtk_cam_videoc_s_fmt, > + .vidioc_try_fmt_vid_cap_mplane = mtk_cam_videoc_try_fmt, > + > + .vidioc_g_fmt_vid_out_mplane = mtk_cam_videoc_g_fmt, > + .vidioc_s_fmt_vid_out_mplane = mtk_cam_videoc_s_fmt, > + .vidioc_try_fmt_vid_out_mplane = mtk_cam_videoc_try_fmt, > + > + /* buffer queue management */ > + .vidioc_reqbufs = vb2_ioctl_reqbufs, > + .vidioc_create_bufs = vb2_ioctl_create_bufs, > + .vidioc_prepare_buf = vb2_ioctl_prepare_buf, > + .vidioc_querybuf = vb2_ioctl_querybuf, > + .vidioc_qbuf = mtk_cam_videoc_qbuf, > + .vidioc_dqbuf = vb2_ioctl_dqbuf, > + .vidioc_streamon = vb2_ioctl_streamon, > + .vidioc_streamoff = vb2_ioctl_streamoff, > + .vidioc_expbuf = vb2_ioctl_expbuf, > +}; > + > +static const struct v4l2_ioctl_ops mtk_cam_v4l2_meta_ioctl_ops = { > + .vidioc_querycap = mtk_cam_videoc_querycap, > + > + .vidioc_enum_fmt_meta_cap = mtk_cam_meta_enum_format, > + .vidioc_g_fmt_meta_cap = mtk_cam_videoc_g_meta_fmt, > + .vidioc_s_fmt_meta_cap = mtk_cam_videoc_s_meta_fmt, > + .vidioc_try_fmt_meta_cap = mtk_cam_videoc_g_meta_fmt, > + > + .vidioc_enum_fmt_meta_out = mtk_cam_meta_enum_format, > + .vidioc_g_fmt_meta_out = mtk_cam_videoc_g_meta_fmt, > + .vidioc_s_fmt_meta_out = mtk_cam_videoc_s_meta_fmt, > + .vidioc_try_fmt_meta_out = mtk_cam_videoc_g_meta_fmt, > + > + .vidioc_reqbufs = vb2_ioctl_reqbufs, > + .vidioc_create_bufs = vb2_ioctl_create_bufs, > + .vidioc_prepare_buf = vb2_ioctl_prepare_buf, > + .vidioc_querybuf = vb2_ioctl_querybuf, > + .vidioc_qbuf = mtk_cam_videoc_qbuf, > + .vidioc_dqbuf = vb2_ioctl_dqbuf, > + .vidioc_streamon = vb2_ioctl_streamon, > + .vidioc_streamoff = vb2_ioctl_streamoff, > + .vidioc_expbuf = vb2_ioctl_expbuf, > +}; The ops should be split for each node type, {VIDEO, META} x {OUTPUT, CAPTURE}. Then the core would validate the type given to all the ioctls automatically. > + > +static u32 mtk_cam_node_get_v4l2_cap(struct mtk_cam_ctx_queue *node_ctx) > +{ > + u32 cap = 0; > + > + if (node_ctx->desc.capture) > + if (node_ctx->desc.image) > + cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE; > + else > + cap = V4L2_CAP_META_CAPTURE; > + else > + if (node_ctx->desc.image) > + cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE; > + else > + cap = V4L2_CAP_META_OUTPUT; > + > + return cap; > +} Why not just have this defined statically as node_ctx->desc.cap? > + > +static u32 mtk_cam_node_get_format_type(struct mtk_cam_ctx_queue *node_ctx) > +{ > + u32 type; > + > + if (node_ctx->desc.capture) > + if (node_ctx->desc.image) > + type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; > + else > + type = V4L2_BUF_TYPE_META_CAPTURE; > + else > + if (node_ctx->desc.image) > + type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE; > + else > + type = V4L2_BUF_TYPE_META_OUTPUT; > + > + return type; > +} Why not just have this defined statically as node_ctx->desc.buf_type? > + > +static const struct v4l2_ioctl_ops *mtk_cam_node_get_ioctl_ops > + (struct mtk_cam_ctx_queue *node_ctx) > +{ > + const struct v4l2_ioctl_ops *ops = NULL; > + > + if (node_ctx->desc.image) > + ops = &mtk_cam_v4l2_ioctl_ops; > + else > + ops = &mtk_cam_v4l2_meta_ioctl_ops; > + return ops; > +} It's also preferable to just put this inside some structure rather than have getter functions. (node_ctx->desc.ioctl_ops?) > + > +/* Config node's video properties */ > +/* according to the device context requirement */ > +static void mtk_cam_node_to_v4l2(struct mtk_cam_dev *isp_dev, u32 node, > + struct video_device *vdev, > + struct v4l2_format *f) > +{ > + u32 cap; > + struct mtk_cam_ctx *device_ctx = &isp_dev->ctx; > + struct mtk_cam_ctx_queue *node_ctx = &device_ctx->queue[node]; > + > + WARN_ON(node >= mtk_cam_dev_get_total_node(isp_dev)); > + WARN_ON(!node_ctx); How is this possible? > + > + /* set cap of the node */ > + cap = mtk_cam_node_get_v4l2_cap(node_ctx); > + f->type = mtk_cam_node_get_format_type(node_ctx); > + vdev->ioctl_ops = mtk_cam_node_get_ioctl_ops(node_ctx); > + > + if (mtk_cam_ctx_format_load_default_fmt(&device_ctx->queue[node], f)) { > + dev_err(&isp_dev->pdev->dev, > + "Can't load default for node (%d): (%s)", > + node, device_ctx->queue[node].desc.name); mtk_cam_ctx_format_load_default_fmt() doesn't fail (and that's right). It should be actually made void. > + } else { > + if (device_ctx->queue[node].desc.image) { > + dev_dbg(&isp_dev->pdev->dev, > + "Node (%d): (%s), dfmt (f:0x%x w:%d: h:%d s:%d)\n", > + node, device_ctx->queue[node].desc.name, > + f->fmt.pix_mp.pixelformat, > + f->fmt.pix_mp.width, > + f->fmt.pix_mp.height, > + f->fmt.pix_mp.plane_fmt[0].sizeimage > + ); > + node_ctx->fmt.pix_mp = f->fmt.pix_mp; > + } else { > + dev_dbg(&isp_dev->pdev->dev, > + "Node (%d): (%s), dfmt (f:0x%x s:%u)\n", > + node, device_ctx->queue[node].desc.name, > + f->fmt.meta.dataformat, > + f->fmt.meta.buffersize > + ); > + node_ctx->fmt.meta = f->fmt.meta; > + } Drop the debugging messages and just replace the whole if/else block with: node_ctx->fmt = f->fmt; Sorry, ran out of time for today. Fourth part will come. :) Best regards, Tomasz
On Tue, 2019-03-12 at 19:04 +0900, Tomasz Figa wrote: > Hi Frederic, Jungo, > > Please see more comments inline. > Hi Tomasz: Thanks for your part 3 comments. Below is our feedback. > > +static int mtk_cam_vb2_queue_setup(struct vb2_queue *vq, > > + unsigned int *num_buffers, > > + unsigned int *num_planes, > > + unsigned int sizes[], > > + struct device *alloc_devs[]) > > +{ > > + struct mtk_cam_mem2mem2_device *m2m2 = vb2_get_drv_priv(vq); > > + struct mtk_cam_dev_video_device *node = > > + mtk_cam_vbq_to_isp_node(vq); > > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > > + struct device *dev = &isp_dev->pdev->dev; > > + void *buf_alloc_ctx = NULL; > > Don't initialize by default, if not strictly necessary. > Ok, fixed in next patch. > > + int queue_id = mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > > + > > + /* Get V4L2 format with the following method */ > > + const struct v4l2_format *fmt = &node->vdev_fmt; > > + > > + *num_planes = 1; > > This doesn't handle VIDIOC_CREATE_BUFS, which triggers a > .queue_setup() call with *num_planes > 0 and sizes[] already > initialized. The driver needs to validate that the sizes and number of > planes are valid in that case and return -EINVAL otherwise. Perhaps > you should try running v4l2-compliance > (https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance) on > this driver, which should catch issues like this. > Ok, we will add code check logic for this user case. The function will be similar to below: https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/vivid/vivid-vid-cap.c#L87 https://elixir.bootlin.com/linux/latest/source/drivers/staging/media/ipu3/ipu3-v4l2.c#L388 > > + *num_buffers = clamp_val(*num_buffers, 1, VB2_MAX_FRAME); > > + > > + if (isp_dev->ctx.queue[queue_id].desc.smem_alloc) { > > + buf_alloc_ctx = isp_dev->ctx.smem_vb2_alloc_ctx; > > + dev_dbg(dev, "Select smem_vb2_alloc_ctx(%llx)\n", > > + (unsigned long long)buf_alloc_ctx); > > Use %p for printing pointers. > For security reason, we will change to use "%pK" for printing kernel pointers. > > + } else { > > + buf_alloc_ctx = isp_dev->ctx.default_vb2_alloc_ctx; > > + dev_dbg(dev, "Select default_vb2_alloc_ctx(%llx)\n", > > + (unsigned long long)buf_alloc_ctx); > > + } > > + > > + vq->dma_attrs |= DMA_ATTR_NON_CONSISTENT; > > + dev_dbg(dev, "queue(%d): cached mmap enabled\n", queue_id); > > This isn't supported in upstream. (By the way, neither it is in Chrome > OS 4.19 kernel. If we really need cached mmap for some reason, we > should propose a proper solution upstream. I'd still first investigate > why write-combine mmap is slow for operations that should be simple > one-time writes or reads.) > Ok, we will remove this in upstream version and follow your suggestion to find out better solution. > > + > > + if (vq->type == V4L2_BUF_TYPE_META_CAPTURE || > > + vq->type == V4L2_BUF_TYPE_META_OUTPUT) > > + sizes[0] = fmt->fmt.meta.buffersize; > > + else > > + sizes[0] = fmt->fmt.pix_mp.plane_fmt[0].sizeimage; > > + > > + alloc_devs[0] = (struct device *)buf_alloc_ctx; > > Please don't add random type casts. If the compiler gives a type > error, that normally means that there is something wrong elsewhere in > the code (i.e. the type of buf_alloc_ctx variable is wrong here - it > should be struct device *) and casting just masks the original > problem. > Ok, we will fix this coding defect. > > + > > + dev_dbg(dev, "queue(%d):type(%d),size(%d),alloc_ctx(%llx)\n", > > + queue_id, vq->type, sizes[0], > > + (unsigned long long)buf_alloc_ctx); > > + > > + /* Initialize buffer queue */ > > + INIT_LIST_HEAD(&node->buffers); > > This is incorrect. .queue_setup() can be also called on > VIDIOC_CREATE_BUFS, which must preserve the other buffers in the > queue. > In our new version, we have removed this for request-API new design. > > + > > + return 0; > > +} > [snip] > > +static int mtk_cam_vb2_start_streaming(struct vb2_queue *vq, unsigned int count) > > +{ > > + struct mtk_cam_mem2mem2_device *m2m2 = vb2_get_drv_priv(vq); > > + struct mtk_cam_dev_video_device *node = > > + mtk_cam_vbq_to_isp_node(vq); > > + int r; > > nit: "ret" is more common and already used by some other functions in > this patch. > We will rename this variable in next patch and try to unify the variable naming in our driver. > > + > > + if (m2m2->streaming) { > > + r = -EBUSY; > > + goto fail_return_bufs; > > + } > > We shouldn't need to check this ourselves. It's not possible to have > this call on an already streaming vb2 queue. Since we start streaming > the m2m2 subdev only when all video nodes start streaming, this should > never happen. > Ok, we will remove this redundant checking in next patch. > > + > > + if (!node->enabled) { > > + pr_err("Node (%ld) is not enable\n", node - m2m2->nodes); > > + r = -EINVAL; > > + goto fail_return_bufs; > > + } > > + > > + r = media_pipeline_start(&node->vdev.entity, &m2m2->pipeline); > > + if (r < 0) { > > + pr_err("Node (%ld) media_pipeline_start failed\n", > > + node - m2m2->nodes); > > + goto fail_return_bufs; > > + } > > + > > + if (!mtk_cam_all_nodes_streaming(m2m2, node)) > > + return 0; > > + > > + /* Start streaming of the whole pipeline now */ > > + > > + r = v4l2_subdev_call(&m2m2->subdev, video, s_stream, 1); > > + if (r < 0) { > > + pr_err("Node (%ld) v4l2_subdev_call s_stream failed\n", > > + node - m2m2->nodes); > > + goto fail_stop_pipeline; > > + } > > + return 0; > > + > > +fail_stop_pipeline: > > + media_pipeline_stop(&node->vdev.entity); > > +fail_return_bufs: > > + mtk_cam_return_all_buffers(m2m2, node, VB2_BUF_STATE_QUEUED); > > + > > + return r; > > +} > > + > > +static void mtk_cam_vb2_stop_streaming(struct vb2_queue *vq) > > +{ > > + struct mtk_cam_mem2mem2_device *m2m2 = vb2_get_drv_priv(vq); > > + struct mtk_cam_dev_video_device *node = > > + mtk_cam_vbq_to_isp_node(vq); > > + int r; > > + > > + WARN_ON(!node->enabled); > > + > > + /* Was this the first node with streaming disabled? */ > > + if (mtk_cam_all_nodes_streaming(m2m2, node)) { > > + /* Yes, really stop streaming now */ > > + r = v4l2_subdev_call(&m2m2->subdev, video, s_stream, 0); > > + if (r) > > + dev_err(m2m2->dev, "failed to stop streaming\n"); > > + } > > + > > + mtk_cam_return_all_buffers(m2m2, node, VB2_BUF_STATE_ERROR); > > + media_pipeline_stop(&node->vdev.entity); > > +} > > + > > +static int mtk_cam_videoc_querycap(struct file *file, void *fh, > > + struct v4l2_capability *cap) > > +{ > > + struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file); > > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > > + int queue_id = > > + mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > > + struct mtk_cam_ctx_queue *node_ctx = &isp_dev->ctx.queue[queue_id]; > > It feels like this could be just stored as node->ctx. > After refactoring this function, the node_ctx is removed. Moreover, we will follow your suggestion to store mtk_cam_ctx_queue pointer in mtk_cam_dev_video_device for better access. > > + > > + strlcpy(cap->driver, m2m2->name, sizeof(cap->driver)); > > + strlcpy(cap->card, m2m2->model, sizeof(cap->card)); > > + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", > > + node->name); > > + > > + cap->device_caps = > > + mtk_cam_node_get_v4l2_cap(node_ctx) | V4L2_CAP_STREAMING; > > + cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS; > > No need to set these 2 fields manually. They are filled in > automatically from struct video_device::device_caps. > We will remove this redundant code. > > + > > + return 0; > > +} > > + > > +/* Propagate forward always the format from the mtk_cam_dev subdev */ > > It doesn't seem to match what the function is doing, i.e. returning > the format structure of the node itself. I'd just drop this comment. > The code should be written in a self-explaining way anyway. > Ok, we will remove this comment to avoid misunderstanding. > > +static int mtk_cam_videoc_g_fmt(struct file *file, void *fh, > > + struct v4l2_format *f) > > +{ > > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > > + > > + f->fmt = node->vdev_fmt.fmt; > > + > > + return 0; > > +} > > + > > +static int mtk_cam_videoc_try_fmt(struct file *file, > > + void *fh, > > + struct v4l2_format *f) > > +{ > > + struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file); > > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > > + struct mtk_cam_ctx *dev_ctx = &isp_dev->ctx; > > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > > + int queue_id = > > + mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > > + int ret = 0; > > + > > + ret = mtk_cam_ctx_fmt_set_img(dev_ctx, queue_id, > > + &f->fmt.pix_mp, > > + &node->vdev_fmt.fmt.pix_mp); > > Doesn't this actually change the current node format? VIDIOC_TRY_FMT > must not have any side effects on current driver state. > This is a bug in our implementation. We will fix it in next patch. > > + > > + /* Simply set the format to the node context in the initial version */ > > + if (ret) { > > + pr_warn("Fmt(%d) not support for queue(%d), will load default fmt\n", > > + f->fmt.pix_mp.pixelformat, queue_id); > > No need for this warning. > Ok, we will remove this in next patch. > > + > > + ret = mtk_cam_ctx_format_load_default_fmt > > + (&dev_ctx->queue[queue_id], f); > > Wouldn't this also change the current node state? > > Also, something wrong with the number of spaces after "ret =". > Ditto. > > + } > > + > > + if (!ret) { > > + node->vdev_fmt.fmt.pix_mp = f->fmt.pix_mp; > > + dev_ctx->queue[queue_id].fmt.pix_mp = node->vdev_fmt.fmt.pix_mp; > > Ditto. > > > + } > > + > > + return ret; > > VIDIOC_TRY_FMT must not return an error unless for the cases described > in https://www.kernel.org/doc/html/latest/media/uapi/v4l/vidioc-g-fmt.html#return-value > . > Ok, we will try to follow the VIDIOC_TYPE_FMT API description of V4L2 manual. > > +} > > + > > +static int mtk_cam_videoc_s_fmt(struct file *file, void *fh, > > + struct v4l2_format *f) > > +{ > > + struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file); > > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > > + struct mtk_cam_ctx *dev_ctx = &isp_dev->ctx; > > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > > + int queue_id = mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > > + int ret = 0; > > + > > + ret = mtk_cam_ctx_fmt_set_img(dev_ctx, queue_id, > > + &f->fmt.pix_mp, > > + &node->vdev_fmt.fmt.pix_mp); > > + > > + /* Simply set the format to the node context in the initial version */ > > + if (!ret) > > + dev_ctx->queue[queue_id].fmt.pix_mp = node->vdev_fmt.fmt.pix_mp; > > + else > > + dev_warn(&isp_dev->pdev->dev, > > + "s_fmt, format not support\n"); > > No need for error messages for userspace errors. > Ok, we will remove this check. > > + > > + return ret; > > Instead of opencoding most of this function, one would normally call > mtk_cam_videoc_try_fmt() first to adjust the format struct and then > just update the driver state with the adjusted format. > > Also, similarly to VIDIOC_TRY_FMT, VIDIOC_SET_FMT doesn't fail unless > in the very specific cases, as described in > https://www.kernel.org/doc/html/latest/media/uapi/v4l/vidioc-g-fmt.html#return-value > . > Ok, below is our revised version of this function. int mtk_cam_videoc_s_fmt(struct file *file, void *fh, struct v4l2_format *f) { struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file); struct mtk_cam_dev *cam_dev = mtk_cam_m2m_to_dev(m2m2); struct mtk_cam_video_device *node = file_to_mtk_cam_node(file); /* Get the valid format*/ mtk_cam_videoc_try_fmt(file, fh, f); /* Configure to video device */ mtk_cam_ctx_fmt_set_img(&cam_dev->pdev->dev, &node->vdev_fmt.fmt.pix_mp, &f->fmt.pix_mp, node->queue_id); return 0; } > > +} > > + > > +static int mtk_cam_enum_framesizes(struct file *filp, void *priv, > > + struct v4l2_frmsizeenum *sizes) > > +{ > > + struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(filp); > > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > > + struct mtk_cam_dev_video_device *node = > > + file_to_mtk_cam_node(filp); > > + int queue_id = mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > > + > > + if (sizes->index != 0) > > + return -EINVAL; > > + > > + if (queue_id == MTK_CAM_CTX_P1_MAIN_STREAM_OUT) { > > + sizes->type = V4L2_FRMSIZE_TYPE_CONTINUOUS; > > + sizes->stepwise.max_width = CAM_B_MAX_WIDTH; > > + sizes->stepwise.min_width = CAM_MIN_WIDTH; > > + sizes->stepwise.max_height = CAM_B_MAX_HEIGHT; > > + sizes->stepwise.min_height = CAM_MIN_HEIGHT; > > + sizes->stepwise.step_height = 1; > > + sizes->stepwise.step_width = 1; > > + } else if (queue_id == MTK_CAM_CTX_P1_PACKED_BIN_OUT) { > > + sizes->type = V4L2_FRMSIZE_TYPE_CONTINUOUS; > > + sizes->stepwise.max_width = RRZ_MAX_WIDTH; > > + sizes->stepwise.min_width = RRZ_MIN_WIDTH; > > + sizes->stepwise.max_height = RRZ_MAX_HEIGHT; > > + sizes->stepwise.min_height = RRZ_MIN_HEIGHT; > > + sizes->stepwise.step_height = 1; > > + sizes->stepwise.step_width = 1; > > + } > > + > > + return 0; > > +} > > + > > +static int mtk_cam_meta_enum_format(struct file *file, > > + void *fh, struct v4l2_fmtdesc *f) > > +{ > > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > > + > > + if (f->index > 0 || f->type != node->vbq.type) > > + return -EINVAL; > > f->type is already checked by the V4L2 core. See v4l_enum_fmt(). > We will remove the redundant checking in next patch. > > + > > + f->pixelformat = node->vdev_fmt.fmt.meta.dataformat; > > + > > + return 0; > > +} > > + > > +static int mtk_cam_videoc_s_meta_fmt(struct file *file, > > + void *fh, struct v4l2_format *f) > > +{ > > + struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file); > > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > > + struct mtk_cam_ctx *dev_ctx = &isp_dev->ctx; > > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > > + int queue_id = mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > > + > > No need for this blank line. > We will fix this coding style in next patch. > > + int ret = 0; > > Please don't default-initialize without a good reason. > Ok, fix in next patch. > > + > > + if (f->type != node->vbq.type) > > + return -EINVAL; > > Ditto. > Ok, fix in next patch. > > + > > + ret = mtk_cam_ctx_format_load_default_fmt(&dev_ctx->queue[queue_id], f); > > + > > No need for this blank line. > Ok, fix in next patch. > > + if (!ret) { > > + node->vdev_fmt.fmt.meta = f->fmt.meta; > > + dev_ctx->queue[queue_id].fmt.meta = node->vdev_fmt.fmt.meta; > > + } else { > > + dev_warn(&isp_dev->pdev->dev, > > + "s_meta_fm failed, format not support\n"); > > No need for this warning. > Ok, fix in next patch. > > + } > > + > > + return ret; > > +} > > Actually, why do we even need to do all the things? Do we support > multiple different meta formats on the same video node? If not, we can > just have all the TRY_FMT/S_FMT/G_FMT return the hardcoded format. > Ok, it is a good idea. We will return the hardcode format for meta video devices. Below is the revision version. int mtk_cam_meta_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f) { struct mtk_cam_video_device *node = file_to_mtk_cam_node(file); f->pixelformat = node->vdev_fmt.fmt.meta.dataformat; return 0; } const struct v4l2_ioctl_ops mtk_cam_v4l2_meta_cap_ioctl_ops = { .vidioc_querycap = mtk_cam_videoc_querycap, .vidioc_enum_fmt_meta_cap = mtk_cam_meta_enum_format, .vidioc_g_fmt_meta_cap = mtk_cam_videoc_g_meta_fmt, .vidioc_s_fmt_meta_cap = mtk_cam_videoc_g_meta_fmt, .vidioc_try_fmt_meta_cap = mtk_cam_videoc_g_meta_fmt, ... > > + > > +static int mtk_cam_videoc_g_meta_fmt(struct file *file, > > + void *fh, struct v4l2_format *f) > > +{ > > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > > + > > + if (f->type != node->vbq.type) > > + return -EINVAL; > > Not needed. > Ok, remove it in next patch. > > + > > + f->fmt = node->vdev_fmt.fmt; > > + > > + return 0; > > +} > > + > > +int mtk_cam_videoc_qbuf(struct file *file, void *priv, struct v4l2_buffer *p) > > +{ > > + struct video_device *vdev = video_devdata(file); > > + struct vb2_buffer *vb; > > + struct mtk_cam_dev_buffer *db; > > + int r = 0; > > + > > + /* check if vb2 queue is busy */ > > + if (vdev->queue->owner && > > + vdev->queue->owner != file->private_data) > > + return -EBUSY; > > This should be already handled by the core. > Ok, remove it in next patch. > > + > > + /* Keep the value of sequence in v4l2_buffer */ > > + /* in ctx buf since vb2_qbuf will set it to 0 */ > > + vb = vdev->queue->bufs[p->index]; > > Why do you need a sequence number for buffers on queue time? The field > is not specified to be set by the userspace and should be ignored by > the driver. The driver should rely on the Request API to match any > buffers together anyway. > It is our old design for frame-sync mechanism. However, we change to use "Request API" design and this function is removed in new version. > > + > > + if (vb) { > > + db = mtk_cam_vb2_buf_to_dev_buf(vb); > > + pr_debug("qbuf: p:%llx,vb:%llx, db:%llx\n", > > + (unsigned long long)p, > > + (unsigned long long)vb, > > + (unsigned long long)db); > > + db->ctx_buf.user_sequence = p->sequence; > > + } > > + > > Generally this driver shouldn't need to implement this callback > itself. Instead it can just use the vb2_ioctl_qbuf() helper. > Same as above. This function is removed in new version. > > + r = vb2_qbuf(vdev->queue, vdev->v4l2_dev->mdev, p); > > + > > + if (r) > > + pr_err("vb2_qbuf failed(err=%d): buf idx(%d)\n", > > + r, p->index); > > + > > + return r; > > +} > > +EXPORT_SYMBOL_GPL(mtk_cam_videoc_qbuf); > > + > > +/******************** function pointers ********************/ > > + > > +/* subdev internal operations */ > > +static const struct v4l2_subdev_internal_ops mtk_cam_subdev_internal_ops = { > > + .open = mtk_cam_subdev_open, > > + .close = mtk_cam_subdev_close, > > +}; > > + > > +static const struct v4l2_subdev_core_ops mtk_cam_subdev_core_ops = { > > + .subscribe_event = mtk_cam_subdev_subscribe_event, > > + .unsubscribe_event = mtk_cam_subdev_unsubscribe_event, > > +}; > > + > > +static const struct v4l2_subdev_video_ops mtk_cam_subdev_video_ops = { > > + .s_stream = mtk_cam_subdev_s_stream, > > +}; > > + > > +static const struct v4l2_subdev_ops mtk_cam_subdev_ops = { > > + .core = &mtk_cam_subdev_core_ops, > > + .video = &mtk_cam_subdev_video_ops, > > +}; > > + > > +static const struct media_entity_operations mtk_cam_media_ops = { > > + .link_setup = mtk_cam_link_setup, > > + .link_validate = v4l2_subdev_link_validate, > > +}; > > + > > +#ifdef CONFIG_MEDIATEK_MEDIA_REQUEST > > +static void mtk_cam_vb2_buf_request_complete(struct vb2_buffer *vb) > > +{ > > + struct mtk_cam_mem2mem2_device *m2m2 = vb2_get_drv_priv(vb->vb2_queue); > > + > > + v4l2_ctrl_request_complete(vb->req_obj.req, > > + m2m2->v4l2_dev->ctrl_handler); > > +} > > +#endif > > + > > +static const struct vb2_ops mtk_cam_vb2_ops = { > > + .buf_queue = mtk_cam_vb2_buf_queue, > > + .queue_setup = mtk_cam_vb2_queue_setup, > > + .start_streaming = mtk_cam_vb2_start_streaming, > > + .stop_streaming = mtk_cam_vb2_stop_streaming, > > + .wait_prepare = vb2_ops_wait_prepare, > > + .wait_finish = vb2_ops_wait_finish, > > +#ifdef CONFIG_MEDIATEK_MEDIA_REQUEST > > + .buf_request_complete = mtk_cam_vb2_buf_request_complete, > > +#endif > > +}; > > + > > +static const struct v4l2_file_operations mtk_cam_v4l2_fops = { > > + .unlocked_ioctl = video_ioctl2, > > + .open = v4l2_fh_open, > > + .release = vb2_fop_release, > > + .poll = vb2_fop_poll, > > + .mmap = vb2_fop_mmap, > > +#ifdef CONFIG_COMPAT > > + .compat_ioctl32 = v4l2_compat_ioctl32, > > +#endif > > +}; > > + > > +static const struct v4l2_ioctl_ops mtk_cam_v4l2_ioctl_ops = { > > + .vidioc_querycap = mtk_cam_videoc_querycap, > > + .vidioc_enum_framesizes = mtk_cam_enum_framesizes, > > + > > + .vidioc_g_fmt_vid_cap_mplane = mtk_cam_videoc_g_fmt, > > + .vidioc_s_fmt_vid_cap_mplane = mtk_cam_videoc_s_fmt, > > + .vidioc_try_fmt_vid_cap_mplane = mtk_cam_videoc_try_fmt, > > + > > + .vidioc_g_fmt_vid_out_mplane = mtk_cam_videoc_g_fmt, > > + .vidioc_s_fmt_vid_out_mplane = mtk_cam_videoc_s_fmt, > > + .vidioc_try_fmt_vid_out_mplane = mtk_cam_videoc_try_fmt, > > + > > + /* buffer queue management */ > > + .vidioc_reqbufs = vb2_ioctl_reqbufs, > > + .vidioc_create_bufs = vb2_ioctl_create_bufs, > > + .vidioc_prepare_buf = vb2_ioctl_prepare_buf, > > + .vidioc_querybuf = vb2_ioctl_querybuf, > > + .vidioc_qbuf = mtk_cam_videoc_qbuf, > > + .vidioc_dqbuf = vb2_ioctl_dqbuf, > > + .vidioc_streamon = vb2_ioctl_streamon, > > + .vidioc_streamoff = vb2_ioctl_streamoff, > > + .vidioc_expbuf = vb2_ioctl_expbuf, > > +}; > > + > > +static const struct v4l2_ioctl_ops mtk_cam_v4l2_meta_ioctl_ops = { > > + .vidioc_querycap = mtk_cam_videoc_querycap, > > + > > + .vidioc_enum_fmt_meta_cap = mtk_cam_meta_enum_format, > > + .vidioc_g_fmt_meta_cap = mtk_cam_videoc_g_meta_fmt, > > + .vidioc_s_fmt_meta_cap = mtk_cam_videoc_s_meta_fmt, > > + .vidioc_try_fmt_meta_cap = mtk_cam_videoc_g_meta_fmt, > > + > > + .vidioc_enum_fmt_meta_out = mtk_cam_meta_enum_format, > > + .vidioc_g_fmt_meta_out = mtk_cam_videoc_g_meta_fmt, > > + .vidioc_s_fmt_meta_out = mtk_cam_videoc_s_meta_fmt, > > + .vidioc_try_fmt_meta_out = mtk_cam_videoc_g_meta_fmt, > > + > > + .vidioc_reqbufs = vb2_ioctl_reqbufs, > > + .vidioc_create_bufs = vb2_ioctl_create_bufs, > > + .vidioc_prepare_buf = vb2_ioctl_prepare_buf, > > + .vidioc_querybuf = vb2_ioctl_querybuf, > > + .vidioc_qbuf = mtk_cam_videoc_qbuf, > > + .vidioc_dqbuf = vb2_ioctl_dqbuf, > > + .vidioc_streamon = vb2_ioctl_streamon, > > + .vidioc_streamoff = vb2_ioctl_streamoff, > > + .vidioc_expbuf = vb2_ioctl_expbuf, > > +}; > > The ops should be split for each node type, {VIDEO, META} x {OUTPUT, > CAPTURE}. Then the core would validate the type given to all the > ioctls automatically. > Ok, below is new implementation. static const struct v4l2_ioctl_ops mtk_cam_v4l2_vcap_ioctl_ops static const struct v4l2_ioctl_ops mtk_cam_v4l2_vout_ioctl_ops static const struct v4l2_ioctl_ops mtk_cam_v4l2_meta_cap_ioctl_ops static const struct v4l2_ioctl_ops mtk_cam_v4l2_meta_out_ioctl_ops > > + > > +static u32 mtk_cam_node_get_v4l2_cap(struct mtk_cam_ctx_queue *node_ctx) > > +{ > > + u32 cap = 0; > > + > > + if (node_ctx->desc.capture) > > + if (node_ctx->desc.image) > > + cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE; > > + else > > + cap = V4L2_CAP_META_CAPTURE; > > + else > > + if (node_ctx->desc.image) > > + cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE; > > + else > > + cap = V4L2_CAP_META_OUTPUT; > > + > > + return cap; > > +} > > Why not just have this defined statically as node_ctx->desc.cap? > Ok, it is refactoring done. > > + > > +static u32 mtk_cam_node_get_format_type(struct mtk_cam_ctx_queue *node_ctx) > > +{ > > + u32 type; > > + > > + if (node_ctx->desc.capture) > > + if (node_ctx->desc.image) > > + type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; > > + else > > + type = V4L2_BUF_TYPE_META_CAPTURE; > > + else > > + if (node_ctx->desc.image) > > + type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE; > > + else > > + type = V4L2_BUF_TYPE_META_OUTPUT; > > + > > + return type; > > +} > > Why not just have this defined statically as node_ctx->desc.buf_type? > Same as above. > > + > > +static const struct v4l2_ioctl_ops *mtk_cam_node_get_ioctl_ops > > + (struct mtk_cam_ctx_queue *node_ctx) > > +{ > > + const struct v4l2_ioctl_ops *ops = NULL; > > + > > + if (node_ctx->desc.image) > > + ops = &mtk_cam_v4l2_ioctl_ops; > > + else > > + ops = &mtk_cam_v4l2_meta_ioctl_ops; > > + return ops; > > +} > > It's also preferable to just put this inside some structure rather > than have getter functions. (node_ctx->desc.ioctl_ops?) > Same as above. Below is the new version for struct mtk_cam_ctx_queue_desc /* * struct mtk_cam_ctx_queue_desc - queue attributes * setup by device context owner * @id: id of the context queue * @name: media entity name * @cap: mapped to V4L2 capabilities * @buf_type: mapped to V4L2 buffer type * @capture: true for capture queue (device to user) * false for output queue (from user to device) * @image: true for image, false for meta data * @smem_alloc: Using the cam_smem_drv as alloc ctx or not * @dma_port: the dma port associated to the buffer * @fmts: supported format * @num_fmts: the number of supported formats * @default_fmt_idx: default format of this queue * @max_buf_count: maximum V4L2 buffer count * @max_buf_count: mapped to v4l2_ioctl_ops */ struct mtk_cam_ctx_queue_desc { u8 id; char *name; u32 cap; u32 buf_type; u32 dma_port; u32 smem_alloc:1; u8 capture:1; u8 image:1; u8 num_fmts; u8 default_fmt_idx; u8 max_buf_count; const struct v4l2_ioctl_ops *ioctl_ops; struct v4l2_format *fmts; }; > > + > > +/* Config node's video properties */ > > +/* according to the device context requirement */ > > +static void mtk_cam_node_to_v4l2(struct mtk_cam_dev *isp_dev, u32 node, > > + struct video_device *vdev, > > + struct v4l2_format *f) > > +{ > > + u32 cap; > > + struct mtk_cam_ctx *device_ctx = &isp_dev->ctx; > > + struct mtk_cam_ctx_queue *node_ctx = &device_ctx->queue[node]; > > + > > + WARN_ON(node >= mtk_cam_dev_get_total_node(isp_dev)); > > + WARN_ON(!node_ctx); > > How is this possible? > Ok, we will remove this in next version. > > + > > + /* set cap of the node */ > > + cap = mtk_cam_node_get_v4l2_cap(node_ctx); > > + f->type = mtk_cam_node_get_format_type(node_ctx); > > + vdev->ioctl_ops = mtk_cam_node_get_ioctl_ops(node_ctx); > > + > > + if (mtk_cam_ctx_format_load_default_fmt(&device_ctx->queue[node], f)) { > > + dev_err(&isp_dev->pdev->dev, > > + "Can't load default for node (%d): (%s)", > > + node, device_ctx->queue[node].desc.name); > > mtk_cam_ctx_format_load_default_fmt() doesn't fail (and that's right). > It should be actually made void. > Ok, we will revise this in next version. > > + } else { > > + if (device_ctx->queue[node].desc.image) { > > + dev_dbg(&isp_dev->pdev->dev, > > + "Node (%d): (%s), dfmt (f:0x%x w:%d: h:%d s:%d)\n", > > + node, device_ctx->queue[node].desc.name, > > + f->fmt.pix_mp.pixelformat, > > + f->fmt.pix_mp.width, > > + f->fmt.pix_mp.height, > > + f->fmt.pix_mp.plane_fmt[0].sizeimage > > + ); > > + node_ctx->fmt.pix_mp = f->fmt.pix_mp; > > + } else { > > + dev_dbg(&isp_dev->pdev->dev, > > + "Node (%d): (%s), dfmt (f:0x%x s:%u)\n", > > + node, device_ctx->queue[node].desc.name, > > + f->fmt.meta.dataformat, > > + f->fmt.meta.buffersize > > + ); > > + node_ctx->fmt.meta = f->fmt.meta; > > + } > > Drop the debugging messages and just replace the whole if/else block with: > > node_ctx->fmt = f->fmt; > Same as above. > Sorry, ran out of time for today. Fourth part will come. :) > > Best regards, > Tomasz > Appreciate your support and hard working on this review. We will look forward your part 4 review. Best regards, Jungo > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
On Wed, Feb 20, 2019 at 4:31 PM Jungo Lin <jungo.lin@mediatek.com> wrote: > > On Tue, 2019-02-19 at 17:51 +0900, Tomasz Figa wrote: > > Hi Jungo, > > > > On Sun, Feb 17, 2019 at 11:56 AM Jungo Lin <jungo.lin@mediatek.com> wrote: > > > > > > On Wed, 2019-02-13 at 18:50 +0900, Tomasz Figa wrote: > > > > (() . ( strHi Frederic, Jungo, > > > > > > > > On Tue, Feb 5, 2019 at 3:43 PM Frederic Chen <frederic.chen@mediatek.com> wrote: > > > > > > > > > > From: Jungo Lin <jungo.lin@mediatek.com> > > > > > > > > > > This patch adds the driver for Pass unit in Mediatek's camera > > > > > ISP system. Pass 1 unit is embedded in Mediatek SOCs. It > > > > > provides RAW processing which includes optical black correction, > > > > > defect pixel correction, W/IR imbalance correction and lens > > > > > shading correction. > > > > > > > > > > The mtk-isp directory will contain drivers for multiple IP > > > > > blocks found in Mediatek ISP system. It will include ISP Pass 1 > > > > > driver, sensor interface driver, DIP driver and face detection > > > > > driver. > > > > > > > > Thanks for the patches! Please see my comments inline. > > > > > > > > > > Dear Thomas: > > > > > > Thanks for your comments. > > > > > > We will revise the source codes based on your comments. > > > Since there are many comments to fix/revise, we will categorize & > > > prioritize these with below list: > > > > > > 1. Coding style issues. > > > 2. Coding defects, including unused codes. > > > 3. Driver architecture refactoring, such as removing mtk_cam_ctx, > > > unnecessary abstraction layer, etc. > > > > > > > Thanks for replying to the comments! > > > > Just to clarify, there is no need to hurry with resending a next patch > > set with only a subset of the changes. Please take your time to > > address all the comments before sending the next revision. This > > prevents forgetting about some remaining comments and also lets other > > reviewers come with new comments or some alternative ideas for already > > existing comments. Second part of my review is coming too. > > > > P.S. Please avoid top-posting on mailing lists. If replying to a > > message, please reply below the related part of that message. (I've > > moved your reply to the place in the email where it should be.) > > > > [snip] > > Hi, Tomasz, > > Thanks for your advice. > We will prepare the next patch set after all comments are revised. > > > > > > > + phys_addr_t paddr; > > > > > > > > We shouldn't need physical address either. I suppose this is for the > > > > SCP, but then it should be a DMA address obtained from dma_map_*() > > > > with struct device pointer of the SCP. > > > > > > > > > > Yes, this physical address is designed for SCP. > > > For tuning buffer, it will be touched by SCP and > > > SCP can't get the physical address by itself. So we need to get > > > this physical address in the kernel space via mtk_cam_smem_iova_to_phys > > > function call and pass it to the SCP. At the same time, DMA address > > > (daddr) is used for ISP HW. > > > > > > > Right, but my point is that in the kernel phys_addr_t is the physical > > address from the CPU point of view. Any address from device point of > > view is dma_addr_t and should be obtained from the DMA mapping API > > (even if it's numerically the same as physical address). > > > > OK. > In order to clarify the address usage, is it ok to rename "dma_addr_t > scp_addr"? Sounds good to me. > Moreover, below function will be also renamed. > dma_addr_t mtk_cam_smem_iova_to_scp_phys(struct device *dev, > dma_addr_t iova) Perhaps mtk_cam_smem_iova_to_scp_addr() for consistency with the struct field above?
On Tue, Mar 12, 2019 at 5:16 PM Jungo Lin <jungo.lin@mediatek.com> wrote: > > On Thu, 2019-03-07 at 19:04 +0900, Tomasz Figa wrote: [snip] > > > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem-drv.c b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem-drv.c > > > new file mode 100644 > > > index 0000000..020c38c > > > --- /dev/null > > > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem-drv.c > > > > I don't think we need any of the code that is in this file. We should > > just use the DMA API. We should be able to create appropriate reserved > > memory pools in DT and properly assign them to the right allocating > > devices. > > > > Skipping review of this file for the time being. > > > > For this file, we may need your help. > Its purpose is same as DIP SMEM driver. > It is used for creating the ISP P1 specific vb2 buffer allocation > context with reserved memory. Unfortunately, the implementation of > mtk_cam-smem-drive.c is our best solution now. > > Could you give us more hints how to implement? > Or do you think we could leverage the implementation from "Samsung S5P > Multi Format Codec driver"? > drivers/media/platform/s5p-mfc/s5p_mfc.c > - s5p_mfc_configure_dma_memory function > - s5p_mfc_configure_2port_memory > - s5p_mfc_alloc_memdev I think we can indeed take some ideas from there. I need some time to check this and give you more details. [snip] > > > + } > > > + > > > + dev_dbg(&isp_dev->pdev->dev, "streamed on sensor(%s)\n", > > > + cio->sensor->entity.name); > > > + > > > + ret = mtk_cam_ctx_streamon(&isp_dev->ctx); > > > + if (ret) { > > > + dev_err(&isp_dev->pdev->dev, > > > + "Pass 1 stream on failed (%d)\n", ret); > > > + return -EPERM; > > > + } > > > + > > > + isp_dev->mem2mem2.streaming = enable; > > > + > > > + ret = mtk_cam_dev_queue_buffers(isp_dev, true); > > > + if (ret) > > > + dev_err(&isp_dev->pdev->dev, > > > + "failed to queue initial buffers (%d)", ret); > > > + > > > + dev_dbg(&isp_dev->pdev->dev, "streamed on Pass 1\n"); > > > + } else { > > > + if (cio->sensor) { > > > > Is it possible to have cio->sensor NULL here? This function would have > > failed if it wasn't found when enabling. > > > > In the original design, it is protected to avoid abnormal double stream > off (s_stream) call from upper layer. For stability reason, it is better > to check. If so, having some state (e.g. field in a struct) for tracking the streaming state would make the code much easier to understand. Also, the error message on the else case is totally misleading, because it complains about a missing sensor, rather than double s_stream. [snip] > Thanks for your valued comments on part 2. > It is helpful for us to make our driver implementation better. > > We'd like to know your opinion about the schedule for RFC V1. > Do you suggest us to send RFC V1 patch set after revising all comments > on part 1 & 2 or wait for part 3 review? I'm going to be a bit busy for the next few days, so it may be a good idea to address the comments for parts 1, 2 and 3 and send RFC V1. Also, for the more general comments, please check if they don't apply to the other drivers too (DIP, FD, Seninf, MDP). Thanks in advance! Best regards, Tomasz
On Tue, Mar 12, 2019 at 5:16 PM Jungo Lin <jungo.lin@mediatek.com> wrote: > > On Thu, 2019-03-07 at 19:04 +0900, Tomasz Figa wrote: [snip] > > > +struct mtk_cam_mem2mem2_device { > > > + const char *name; > > > + const char *model; > > > > For both of the fields above, they seem to be always > > MTK_CAM_DEV_P1_NAME, so we can just use the macro directly whenever > > needed. No need for this indirection. > > > > OK. These two fields will be removed in next patch. > > > > + struct device *dev; > > > + int num_nodes; > > > + struct mtk_cam_dev_video_device *nodes; > > > + const struct vb2_mem_ops *vb2_mem_ops; > > > > This is always "vb2_dma_contig_memops", so it can be used directly. > > > > Ditto. > > > > + unsigned int buf_struct_size; > > > > This is always sizeof(struct mtk_cam_dev_buffer), so no need to save > > it in the struct. > > > > Ditto. > > > > + int streaming; > > > + struct v4l2_device *v4l2_dev; > > > + struct media_device *media_dev; > > > > These 2 fields are already in mtk_cam_dev which is a superclass of > > this struct. One can just access them from there directly. > > > > Ditto. > > > > + struct media_pipeline pipeline; > > > + struct v4l2_subdev subdev; > > > > Could you remind me what was the media topology exposed by this > > driver? This is already the second subdev I spotted in this patch, > > which looks strange. > > > > > For sub-device design, we will remove the sub-device for CIO and keep > only one sub-device for ISP driver in next patch. We will also provide > the media topology in RFC v1 patch to clarify. > > > > + struct media_pad *subdev_pads; > > > + struct v4l2_file_operations v4l2_file_ops; > > > + const struct file_operations fops; > > > +}; > > > > Given most of the comments above, it looks like the remaining useful > > fields in this struct could be just moved to mtk_cam_dev, without the > > need for this separate struct. > > > > This is the final revision for these two structures. > Do you suggest to merge it to simplify? > > struct mtk_cam_mem2mem2_device { > struct mtk_cam_video_device *nodes; > struct media_pipeline pipeline; > struct v4l2_subdev subdev; > struct media_pad *subdev_pads; > }; > > struct mtk_cam_dev { > struct platform_device *pdev; > struct mtk_cam_video_device mem2mem2_nodes[MTK_CAM_DEV_NODE_MAX]; > struct mtk_cam_mem2mem2_device mem2mem2; > struct mtk_cam_io_connection cio; > struct v4l2_device v4l2_dev; > struct media_device media_dev; > struct mtk_cam_ctx ctx; > struct v4l2_async_notifier notifier; > }; > I feel like there is not much benefit in having this split. Similarly, I'm not sure if there is a reason to have separate structs for mtk_cam_io_connection and mtk_cam_ctx. (Sorry, missed this one in previous reply.) Best regards, Tomasz
On Wed, Mar 13, 2019 at 3:54 PM Jungo Lin <jungo.lin@mediatek.com> wrote: > > On Tue, 2019-03-12 at 19:04 +0900, Tomasz Figa wrote: [snip] > > Instead of opencoding most of this function, one would normally call > > mtk_cam_videoc_try_fmt() first to adjust the format struct and then > > just update the driver state with the adjusted format. > > > > Also, similarly to VIDIOC_TRY_FMT, VIDIOC_SET_FMT doesn't fail unless > > in the very specific cases, as described in > > https://www.kernel.org/doc/html/latest/media/uapi/v4l/vidioc-g-fmt.html#return-value > > . > > > > Ok, below is our revised version of this function. > > int mtk_cam_videoc_s_fmt(struct file *file, void *fh, > struct v4l2_format *f) > { > struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file); > struct mtk_cam_dev *cam_dev = mtk_cam_m2m_to_dev(m2m2); > struct mtk_cam_video_device *node = file_to_mtk_cam_node(file); > > /* Get the valid format*/ > mtk_cam_videoc_try_fmt(file, fh, f); > /* Configure to video device */ > mtk_cam_ctx_fmt_set_img(&cam_dev->pdev->dev, > &node->vdev_fmt.fmt.pix_mp, > &f->fmt.pix_mp, > node->queue_id); > > return 0; > } > Looks almost good. We still need to signal the -EBUSY error condition if an attempt to change the format is made while streaming is active. [snip] > > > +static int mtk_cam_videoc_s_meta_fmt(struct file *file, > > > + void *fh, struct v4l2_format *f) > > > +{ > > > + struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file); > > > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > > > + struct mtk_cam_ctx *dev_ctx = &isp_dev->ctx; > > > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > > > + int queue_id = mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > > > + > > > > No need for this blank line. > > > > We will fix this coding style in next patch. > > > > + int ret = 0; > > > > Please don't default-initialize without a good reason. > > > > Ok, fix in next patch. > > > > + > > > + if (f->type != node->vbq.type) > > > + return -EINVAL; > > > > Ditto. > > > > Ok, fix in next patch. > > > > + > > > + ret = mtk_cam_ctx_format_load_default_fmt(&dev_ctx->queue[queue_id], f); > > > + > > > > No need for this blank line. > > > > Ok, fix in next patch. > > > > + if (!ret) { > > > + node->vdev_fmt.fmt.meta = f->fmt.meta; > > > + dev_ctx->queue[queue_id].fmt.meta = node->vdev_fmt.fmt.meta; > > > + } else { > > > + dev_warn(&isp_dev->pdev->dev, > > > + "s_meta_fm failed, format not support\n"); > > > > No need for this warning. > > > > Ok, fix in next patch. > > > > + } > > > + > > > + return ret; > > > +} > > > > Actually, why do we even need to do all the things? Do we support > > multiple different meta formats on the same video node? If not, we can > > just have all the TRY_FMT/S_FMT/G_FMT return the hardcoded format. > > > > Ok, it is a good idea. We will return the hardcode format for meta video > devices. > Below is the revision version. > > int mtk_cam_meta_enum_format(struct file *file, > void *fh, struct v4l2_fmtdesc *f) > { > struct mtk_cam_video_device *node = file_to_mtk_cam_node(file); > > f->pixelformat = node->vdev_fmt.fmt.meta.dataformat; > > return 0; > } Need to error out if f->index > 0. Also need to initialize the other output fields - flags and description. [snip] > > > +static u32 mtk_cam_node_get_v4l2_cap(struct mtk_cam_ctx_queue *node_ctx) > > > +{ > > > + u32 cap = 0; > > > + > > > + if (node_ctx->desc.capture) > > > + if (node_ctx->desc.image) > > > + cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE; > > > + else > > > + cap = V4L2_CAP_META_CAPTURE; > > > + else > > > + if (node_ctx->desc.image) > > > + cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE; > > > + else > > > + cap = V4L2_CAP_META_OUTPUT; > > > + > > > + return cap; > > > +} > > > > Why not just have this defined statically as node_ctx->desc.cap? > > > > Ok, it is refactoring done. > > > > + > > > +static u32 mtk_cam_node_get_format_type(struct mtk_cam_ctx_queue *node_ctx) > > > +{ > > > + u32 type; > > > + > > > + if (node_ctx->desc.capture) > > > + if (node_ctx->desc.image) > > > + type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; > > > + else > > > + type = V4L2_BUF_TYPE_META_CAPTURE; > > > + else > > > + if (node_ctx->desc.image) > > > + type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE; > > > + else > > > + type = V4L2_BUF_TYPE_META_OUTPUT; > > > + > > > + return type; > > > +} > > > > Why not just have this defined statically as node_ctx->desc.buf_type? > > > > Same as above. > > > > + > > > +static const struct v4l2_ioctl_ops *mtk_cam_node_get_ioctl_ops > > > + (struct mtk_cam_ctx_queue *node_ctx) > > > +{ > > > + const struct v4l2_ioctl_ops *ops = NULL; > > > + > > > + if (node_ctx->desc.image) > > > + ops = &mtk_cam_v4l2_ioctl_ops; > > > + else > > > + ops = &mtk_cam_v4l2_meta_ioctl_ops; > > > + return ops; > > > +} > > > > It's also preferable to just put this inside some structure rather > > than have getter functions. (node_ctx->desc.ioctl_ops?) > > > > Same as above. > Below is the new version for struct mtk_cam_ctx_queue_desc > > /* > * struct mtk_cam_ctx_queue_desc - queue attributes > * setup by device context owner > * @id: id of the context queue > * @name: media entity name > * @cap: mapped to V4L2 capabilities > * @buf_type: mapped to V4L2 buffer type > * @capture: true for capture queue (device to user) > * false for output queue (from user to device) > * @image: true for image, false for meta data > * @smem_alloc: Using the cam_smem_drv as alloc ctx or not > * @dma_port: the dma port associated to the buffer > * @fmts: supported format > * @num_fmts: the number of supported formats > * @default_fmt_idx: default format of this queue > * @max_buf_count: maximum V4L2 buffer count > * @max_buf_count: mapped to v4l2_ioctl_ops > */ > struct mtk_cam_ctx_queue_desc { > u8 id; > char *name; > u32 cap; > u32 buf_type; > u32 dma_port; > u32 smem_alloc:1; > u8 capture:1; > u8 image:1; > u8 num_fmts; > u8 default_fmt_idx; > u8 max_buf_count; > const struct v4l2_ioctl_ops *ioctl_ops; > struct v4l2_format *fmts; > }; SGTM +/- the missing kerneldoc for the new fields. [snip] > > Sorry, ran out of time for today. Fourth part will come. :) > > > > Best regards, > > Tomasz > > > > Appreciate your support and hard working on this review. > We will look forward your part 4 review. Thanks for replying to all the comments, it's very helpful. As I mentioned in another reply, I'm going to be busy for the next few days, so I'd suggest addressing the existing comments, fixing any v4l2-compliance issues and also checking if any changes could be applied to the other drivers (DIP, FD, Seninf, MDP) too and then sending RFC V1. Best regards, Tomasz
On Thu, 2019-03-21 at 12:33 +0900, Tomasz Figa wrote: > On Wed, Feb 20, 2019 at 4:31 PM Jungo Lin <jungo.lin@mediatek.com> wrote: > > > > On Tue, 2019-02-19 at 17:51 +0900, Tomasz Figa wrote: > > > Hi Jungo, > > > > > > On Sun, Feb 17, 2019 at 11:56 AM Jungo Lin <jungo.lin@mediatek.com> wrote: > > > > > > > > On Wed, 2019-02-13 at 18:50 +0900, Tomasz Figa wrote: > > > > > (() . ( strHi Frederic, Jungo, > > > > > > > > > > On Tue, Feb 5, 2019 at 3:43 PM Frederic Chen <frederic.chen@mediatek.com> wrote: > > > > > > > > > > > > From: Jungo Lin <jungo.lin@mediatek.com> > > > > > > > > > > > > This patch adds the driver for Pass unit in Mediatek's camera > > > > > > ISP system. Pass 1 unit is embedded in Mediatek SOCs. It > > > > > > provides RAW processing which includes optical black correction, > > > > > > defect pixel correction, W/IR imbalance correction and lens > > > > > > shading correction. > > > > > > > > > > > > The mtk-isp directory will contain drivers for multiple IP > > > > > > blocks found in Mediatek ISP system. It will include ISP Pass 1 > > > > > > driver, sensor interface driver, DIP driver and face detection > > > > > > driver. > > > > > > > > > > Thanks for the patches! Please see my comments inline. > > > > > > > > > > > > > Dear Thomas: > > > > > > > > Thanks for your comments. > > > > > > > > We will revise the source codes based on your comments. > > > > Since there are many comments to fix/revise, we will categorize & > > > > prioritize these with below list: > > > > > > > > 1. Coding style issues. > > > > 2. Coding defects, including unused codes. > > > > 3. Driver architecture refactoring, such as removing mtk_cam_ctx, > > > > unnecessary abstraction layer, etc. > > > > > > > > > > Thanks for replying to the comments! > > > > > > Just to clarify, there is no need to hurry with resending a next patch > > > set with only a subset of the changes. Please take your time to > > > address all the comments before sending the next revision. This > > > prevents forgetting about some remaining comments and also lets other > > > reviewers come with new comments or some alternative ideas for already > > > existing comments. Second part of my review is coming too. > > > > > > P.S. Please avoid top-posting on mailing lists. If replying to a > > > message, please reply below the related part of that message. (I've > > > moved your reply to the place in the email where it should be.) > > > > > > [snip] > > > > Hi, Tomasz, > > > > Thanks for your advice. > > We will prepare the next patch set after all comments are revised. > > > > > > > > > > + phys_addr_t paddr; > > > > > > > > > > We shouldn't need physical address either. I suppose this is for the > > > > > SCP, but then it should be a DMA address obtained from dma_map_*() > > > > > with struct device pointer of the SCP. > > > > > > > > > > > > > Yes, this physical address is designed for SCP. > > > > For tuning buffer, it will be touched by SCP and > > > > SCP can't get the physical address by itself. So we need to get > > > > this physical address in the kernel space via mtk_cam_smem_iova_to_phys > > > > function call and pass it to the SCP. At the same time, DMA address > > > > (daddr) is used for ISP HW. > > > > > > > > > > Right, but my point is that in the kernel phys_addr_t is the physical > > > address from the CPU point of view. Any address from device point of > > > view is dma_addr_t and should be obtained from the DMA mapping API > > > (even if it's numerically the same as physical address). > > > > > > > OK. > > In order to clarify the address usage, is it ok to rename "dma_addr_t > > scp_addr"? > > Sounds good to me. > > > Moreover, below function will be also renamed. > > dma_addr_t mtk_cam_smem_iova_to_scp_phys(struct device *dev, > > dma_addr_t iova) > > Perhaps mtk_cam_smem_iova_to_scp_addr() for consistency with the > struct field above? Ok, we will align the function name to struct field. This fix will be included in v1 version. Thanks for your comments. Best regards, Jungo
On Thu, 2019-03-21 at 12:45 +0900, Tomasz Figa wrote: > On Tue, Mar 12, 2019 at 5:16 PM Jungo Lin <jungo.lin@mediatek.com> wrote: > > > > On Thu, 2019-03-07 at 19:04 +0900, Tomasz Figa wrote: > [snip] > > > > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem-drv.c b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem-drv.c > > > > new file mode 100644 > > > > index 0000000..020c38c > > > > --- /dev/null > > > > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem-drv.c > > > > > > I don't think we need any of the code that is in this file. We should > > > just use the DMA API. We should be able to create appropriate reserved > > > memory pools in DT and properly assign them to the right allocating > > > devices. > > > > > > Skipping review of this file for the time being. > > > > > > > For this file, we may need your help. > > Its purpose is same as DIP SMEM driver. > > It is used for creating the ISP P1 specific vb2 buffer allocation > > context with reserved memory. Unfortunately, the implementation of > > mtk_cam-smem-drive.c is our best solution now. > > > > Could you give us more hints how to implement? > > Or do you think we could leverage the implementation from "Samsung S5P > > Multi Format Codec driver"? > > drivers/media/platform/s5p-mfc/s5p_mfc.c > > - s5p_mfc_configure_dma_memory function > > - s5p_mfc_configure_2port_memory > > - s5p_mfc_alloc_memdev > > I think we can indeed take some ideas from there. I need some time to > check this and give you more details. > > [snip] Thanks for your support. If you have any input, please kindly let us know. We will list this revision in the to-do list of V1 version. At the same time, we will also continue to investigate how to implement based on current information. > > > > + } > > > > + > > > > + dev_dbg(&isp_dev->pdev->dev, "streamed on sensor(%s)\n", > > > > + cio->sensor->entity.name); > > > > + > > > > + ret = mtk_cam_ctx_streamon(&isp_dev->ctx); > > > > + if (ret) { > > > > + dev_err(&isp_dev->pdev->dev, > > > > + "Pass 1 stream on failed (%d)\n", ret); > > > > + return -EPERM; > > > > + } > > > > + > > > > + isp_dev->mem2mem2.streaming = enable; > > > > + > > > > + ret = mtk_cam_dev_queue_buffers(isp_dev, true); > > > > + if (ret) > > > > + dev_err(&isp_dev->pdev->dev, > > > > + "failed to queue initial buffers (%d)", ret); > > > > + > > > > + dev_dbg(&isp_dev->pdev->dev, "streamed on Pass 1\n"); > > > > + } else { > > > > + if (cio->sensor) { > > > > > > Is it possible to have cio->sensor NULL here? This function would have > > > failed if it wasn't found when enabling. > > > > > > > In the original design, it is protected to avoid abnormal double stream > > off (s_stream) call from upper layer. For stability reason, it is better > > to check. > > If so, having some state (e.g. field in a struct) for tracking the > streaming state would make the code much easier to understand. > Also, the error message on the else case is totally misleading, > because it complains about a missing sensor, rather than double > s_stream. > > [snip] Yes, your suggestion is helpful. We will correct our implementation to make it more clear in next version. > > Thanks for your valued comments on part 2. > > It is helpful for us to make our driver implementation better. > > > > We'd like to know your opinion about the schedule for RFC V1. > > Do you suggest us to send RFC V1 patch set after revising all comments > > on part 1 & 2 or wait for part 3 review? > > I'm going to be a bit busy for the next few days, so it may be a good > idea to address the comments for parts 1, 2 and 3 and send RFC V1. > Also, for the more general comments, please check if they don't apply > to the other drivers too (DIP, FD, Seninf, MDP). Thanks in advance! > > Best regards, > Tomasz > Ok, we plan to send our RFC V1 for ISP P1 driver next week after revising current comments. For DIP & FD drivers, they also have common implementations with P1 driver. We will sync current comments to them. For Seninf & MDP drivers. we will share our coding style & coding defect issues to them. Moreover, we will provide our V4L2_Compliance testing report from RFC V1. Best regards, Jungo > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
On Thu, 2019-03-21 at 12:48 +0900, Tomasz Figa wrote: > On Tue, Mar 12, 2019 at 5:16 PM Jungo Lin <jungo.lin@mediatek.com> wrote: > > > > On Thu, 2019-03-07 at 19:04 +0900, Tomasz Figa wrote: > [snip] > > > > +struct mtk_cam_mem2mem2_device { > > > > + const char *name; > > > > + const char *model; > > > > > > For both of the fields above, they seem to be always > > > MTK_CAM_DEV_P1_NAME, so we can just use the macro directly whenever > > > needed. No need for this indirection. > > > > > > > OK. These two fields will be removed in next patch. > > > > > > + struct device *dev; > > > > + int num_nodes; > > > > + struct mtk_cam_dev_video_device *nodes; > > > > + const struct vb2_mem_ops *vb2_mem_ops; > > > > > > This is always "vb2_dma_contig_memops", so it can be used directly. > > > > > > > Ditto. > > > > > > + unsigned int buf_struct_size; > > > > > > This is always sizeof(struct mtk_cam_dev_buffer), so no need to save > > > it in the struct. > > > > > > > Ditto. > > > > > > + int streaming; > > > > + struct v4l2_device *v4l2_dev; > > > > + struct media_device *media_dev; > > > > > > These 2 fields are already in mtk_cam_dev which is a superclass of > > > this struct. One can just access them from there directly. > > > > > > > Ditto. > > > > > > + struct media_pipeline pipeline; > > > > + struct v4l2_subdev subdev; > > > > > > Could you remind me what was the media topology exposed by this > > > driver? This is already the second subdev I spotted in this patch, > > > which looks strange. > > > > > > > > > For sub-device design, we will remove the sub-device for CIO and keep > > only one sub-device for ISP driver in next patch. We will also provide > > the media topology in RFC v1 patch to clarify. > > > > > > + struct media_pad *subdev_pads; > > > > + struct v4l2_file_operations v4l2_file_ops; > > > > + const struct file_operations fops; > > > > +}; > > > > > > Given most of the comments above, it looks like the remaining useful > > > fields in this struct could be just moved to mtk_cam_dev, without the > > > need for this separate struct. > > > > > > > This is the final revision for these two structures. > > Do you suggest to merge it to simplify? > > > > struct mtk_cam_mem2mem2_device { > > struct mtk_cam_video_device *nodes; > > struct media_pipeline pipeline; > > struct v4l2_subdev subdev; > > struct media_pad *subdev_pads; > > }; > > > > struct mtk_cam_dev { > > struct platform_device *pdev; > > struct mtk_cam_video_device mem2mem2_nodes[MTK_CAM_DEV_NODE_MAX]; > > struct mtk_cam_mem2mem2_device mem2mem2; > > struct mtk_cam_io_connection cio; > > struct v4l2_device v4l2_dev; > > struct media_device media_dev; > > struct mtk_cam_ctx ctx; > > struct v4l2_async_notifier notifier; > > }; > > > > I feel like there is not much benefit in having this split. Similarly, > I'm not sure if there is a reason to have separate structs for > mtk_cam_io_connection and mtk_cam_ctx. > > (Sorry, missed this one in previous reply.) > > Best regards, > Tomasz Ok, agree your comment. We will remove both mtk_cam_io_connection and mtk_cam_ctx and merge those fields into mtk_cam_dev. Thanks for your suggestion. Best regards, Jungo
On Thu, 2019-03-21 at 12:59 +0900, Tomasz Figa wrote: > On Wed, Mar 13, 2019 at 3:54 PM Jungo Lin <jungo.lin@mediatek.com> wrote: > > > > On Tue, 2019-03-12 at 19:04 +0900, Tomasz Figa wrote: > [snip] > > > Instead of opencoding most of this function, one would normally call > > > mtk_cam_videoc_try_fmt() first to adjust the format struct and then > > > just update the driver state with the adjusted format. > > > > > > Also, similarly to VIDIOC_TRY_FMT, VIDIOC_SET_FMT doesn't fail unless > > > in the very specific cases, as described in > > > https://www.kernel.org/doc/html/latest/media/uapi/v4l/vidioc-g-fmt.html#return-value > > > . > > > > > > > Ok, below is our revised version of this function. > > > > int mtk_cam_videoc_s_fmt(struct file *file, void *fh, > > struct v4l2_format *f) > > { > > struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file); > > struct mtk_cam_dev *cam_dev = mtk_cam_m2m_to_dev(m2m2); > > struct mtk_cam_video_device *node = file_to_mtk_cam_node(file); > > > > /* Get the valid format*/ > > mtk_cam_videoc_try_fmt(file, fh, f); > > /* Configure to video device */ > > mtk_cam_ctx_fmt_set_img(&cam_dev->pdev->dev, > > &node->vdev_fmt.fmt.pix_mp, > > &f->fmt.pix_mp, > > node->queue_id); > > > > return 0; > > } > > > > Looks almost good. We still need to signal the -EBUSY error condition > if an attempt to change the format is made while streaming is active. > > [snip] Ok, we will add streaming status checking in this function. > > > > +static int mtk_cam_videoc_s_meta_fmt(struct file *file, > > > > + void *fh, struct v4l2_format *f) > > > > +{ > > > > + struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file); > > > > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > > > > + struct mtk_cam_ctx *dev_ctx = &isp_dev->ctx; > > > > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > > > > + int queue_id = mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > > > > + > > > > > > No need for this blank line. > > > > > > > We will fix this coding style in next patch. > > > > > > + int ret = 0; > > > > > > Please don't default-initialize without a good reason. > > > > > > > Ok, fix in next patch. > > > > > > + > > > > + if (f->type != node->vbq.type) > > > > + return -EINVAL; > > > > > > Ditto. > > > > > > > Ok, fix in next patch. > > > > > > + > > > > + ret = mtk_cam_ctx_format_load_default_fmt(&dev_ctx->queue[queue_id], f); > > > > + > > > > > > No need for this blank line. > > > > > > > Ok, fix in next patch. > > > > > > + if (!ret) { > > > > + node->vdev_fmt.fmt.meta = f->fmt.meta; > > > > + dev_ctx->queue[queue_id].fmt.meta = node->vdev_fmt.fmt.meta; > > > > + } else { > > > > + dev_warn(&isp_dev->pdev->dev, > > > > + "s_meta_fm failed, format not support\n"); > > > > > > No need for this warning. > > > > > > > Ok, fix in next patch. > > > > > > + } > > > > + > > > > + return ret; > > > > +} > > > > > > Actually, why do we even need to do all the things? Do we support > > > multiple different meta formats on the same video node? If not, we can > > > just have all the TRY_FMT/S_FMT/G_FMT return the hardcoded format. > > > > > > > Ok, it is a good idea. We will return the hardcode format for meta video > > devices. > > Below is the revision version. > > > > int mtk_cam_meta_enum_format(struct file *file, > > void *fh, struct v4l2_fmtdesc *f) > > { > > struct mtk_cam_video_device *node = file_to_mtk_cam_node(file); > > > > f->pixelformat = node->vdev_fmt.fmt.meta.dataformat; > > > > return 0; > > } > > Need to error out if f->index > 0. Also need to initialize the other > output fields - flags and description. > > [snip] Ok, we will add format index & type checking in the beginning of this function. Moreover, we will initialize flags & description based on current meta format value. > > > > +static u32 mtk_cam_node_get_v4l2_cap(struct mtk_cam_ctx_queue *node_ctx) > > > > +{ > > > > + u32 cap = 0; > > > > + > > > > + if (node_ctx->desc.capture) > > > > + if (node_ctx->desc.image) > > > > + cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE; > > > > + else > > > > + cap = V4L2_CAP_META_CAPTURE; > > > > + else > > > > + if (node_ctx->desc.image) > > > > + cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE; > > > > + else > > > > + cap = V4L2_CAP_META_OUTPUT; > > > > + > > > > + return cap; > > > > +} > > > > > > Why not just have this defined statically as node_ctx->desc.cap? > > > > > > > Ok, it is refactoring done. > > > > > > + > > > > +static u32 mtk_cam_node_get_format_type(struct mtk_cam_ctx_queue *node_ctx) > > > > +{ > > > > + u32 type; > > > > + > > > > + if (node_ctx->desc.capture) > > > > + if (node_ctx->desc.image) > > > > + type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; > > > > + else > > > > + type = V4L2_BUF_TYPE_META_CAPTURE; > > > > + else > > > > + if (node_ctx->desc.image) > > > > + type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE; > > > > + else > > > > + type = V4L2_BUF_TYPE_META_OUTPUT; > > > > + > > > > + return type; > > > > +} > > > > > > Why not just have this defined statically as node_ctx->desc.buf_type? > > > > > > > Same as above. > > > > > > + > > > > +static const struct v4l2_ioctl_ops *mtk_cam_node_get_ioctl_ops > > > > + (struct mtk_cam_ctx_queue *node_ctx) > > > > +{ > > > > + const struct v4l2_ioctl_ops *ops = NULL; > > > > + > > > > + if (node_ctx->desc.image) > > > > + ops = &mtk_cam_v4l2_ioctl_ops; > > > > + else > > > > + ops = &mtk_cam_v4l2_meta_ioctl_ops; > > > > + return ops; > > > > +} > > > > > > It's also preferable to just put this inside some structure rather > > > than have getter functions. (node_ctx->desc.ioctl_ops?) > > > > > > > Same as above. > > Below is the new version for struct mtk_cam_ctx_queue_desc > > > > /* > > * struct mtk_cam_ctx_queue_desc - queue attributes > > * setup by device context owner > > * @id: id of the context queue > > * @name: media entity name > > * @cap: mapped to V4L2 capabilities > > * @buf_type: mapped to V4L2 buffer type > > * @capture: true for capture queue (device to user) > > * false for output queue (from user to device) > > * @image: true for image, false for meta data > > * @smem_alloc: Using the cam_smem_drv as alloc ctx or not > > * @dma_port: the dma port associated to the buffer > > * @fmts: supported format > > * @num_fmts: the number of supported formats > > * @default_fmt_idx: default format of this queue > > * @max_buf_count: maximum V4L2 buffer count > > * @max_buf_count: mapped to v4l2_ioctl_ops > > */ > > struct mtk_cam_ctx_queue_desc { > > u8 id; > > char *name; > > u32 cap; > > u32 buf_type; > > u32 dma_port; > > u32 smem_alloc:1; > > u8 capture:1; > > u8 image:1; > > u8 num_fmts; > > u8 default_fmt_idx; > > u8 max_buf_count; > > const struct v4l2_ioctl_ops *ioctl_ops; > > struct v4l2_format *fmts; > > }; > > SGTM +/- the missing kerneldoc for the new fields. > > [snip] Ok, we will fix the missing part. > > > Sorry, ran out of time for today. Fourth part will come. :) > > > > > > Best regards, > > > Tomasz > > > > > > > Appreciate your support and hard working on this review. > > We will look forward your part 4 review. > > Thanks for replying to all the comments, it's very helpful. > > As I mentioned in another reply, I'm going to be busy for the next few > days, so I'd suggest addressing the existing comments, fixing any > v4l2-compliance issues and also checking if any changes could be > applied to the other drivers (DIP, FD, Seninf, MDP) too and then > sending RFC V1. > > Best regards, > Tomasz Ok, we will deliver the RFC V1 next week. Btw, for v4l2-compliance testing, we have some questions about this. We list these two issues below. If you have time, please kindly provide your comments. Or we could discuss these two issues in RFC V1. 1. Kernel minor issue for 32 bit test program. => fail: v4l2-test-media.cpp(368): check_0(links.reserved, sizeof(links.reserved)) This issue is similar to below patch. https://patchwork.kernel.org/patch/8578421/ The reserved fields of media_links_enum is missing copy from kernel space to user space in media_device_enum_links32 function. So it makes the above failed. Do we provide our patch to fix it? 2. Some v4l2-compliance testing results are failed in the sensor drivers's sub devices nodes. which are implemented in ov5695 & ov2685 driver. Do you suggest us to fix them? Best regards, Jungo
On Fri, Mar 22, 2019 at 11:21 AM Jungo Lin <jungo.lin@mediatek.com> wrote: > > On Thu, 2019-03-21 at 12:59 +0900, Tomasz Figa wrote: > > On Wed, Mar 13, 2019 at 3:54 PM Jungo Lin <jungo.lin@mediatek.com> wrote: > > > > > > On Tue, 2019-03-12 at 19:04 +0900, Tomasz Figa wrote: > > [snip] > > > > Instead of opencoding most of this function, one would normally call > > > > mtk_cam_videoc_try_fmt() first to adjust the format struct and then > > > > just update the driver state with the adjusted format. > > > > > > > > Also, similarly to VIDIOC_TRY_FMT, VIDIOC_SET_FMT doesn't fail unless > > > > in the very specific cases, as described in > > > > https://www.kernel.org/doc/html/latest/media/uapi/v4l/vidioc-g-fmt.html#return-value > > > > . > > > > > > > > > > Ok, below is our revised version of this function. > > > > > > int mtk_cam_videoc_s_fmt(struct file *file, void *fh, > > > struct v4l2_format *f) > > > { > > > struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file); > > > struct mtk_cam_dev *cam_dev = mtk_cam_m2m_to_dev(m2m2); > > > struct mtk_cam_video_device *node = file_to_mtk_cam_node(file); > > > > > > /* Get the valid format*/ > > > mtk_cam_videoc_try_fmt(file, fh, f); > > > /* Configure to video device */ > > > mtk_cam_ctx_fmt_set_img(&cam_dev->pdev->dev, > > > &node->vdev_fmt.fmt.pix_mp, > > > &f->fmt.pix_mp, > > > node->queue_id); > > > > > > return 0; > > > } > > > > > > > Looks almost good. We still need to signal the -EBUSY error condition > > if an attempt to change the format is made while streaming is active. > > > > [snip] > > Ok, we will add streaming status checking in this function. > > > > > > +static int mtk_cam_videoc_s_meta_fmt(struct file *file, > > > > > + void *fh, struct v4l2_format *f) > > > > > +{ > > > > > + struct mtk_cam_mem2mem2_device *m2m2 = video_drvdata(file); > > > > > + struct mtk_cam_dev *isp_dev = mtk_cam_m2m_to_dev(m2m2); > > > > > + struct mtk_cam_ctx *dev_ctx = &isp_dev->ctx; > > > > > + struct mtk_cam_dev_video_device *node = file_to_mtk_cam_node(file); > > > > > + int queue_id = mtk_cam_dev_get_queue_id_of_dev_node(isp_dev, node); > > > > > + > > > > > > > > No need for this blank line. > > > > > > > > > > We will fix this coding style in next patch. > > > > > > > > + int ret = 0; > > > > > > > > Please don't default-initialize without a good reason. > > > > > > > > > > Ok, fix in next patch. > > > > > > > > + > > > > > + if (f->type != node->vbq.type) > > > > > + return -EINVAL; > > > > > > > > Ditto. > > > > > > > > > > Ok, fix in next patch. > > > > > > > > + > > > > > + ret = mtk_cam_ctx_format_load_default_fmt(&dev_ctx->queue[queue_id], f); > > > > > + > > > > > > > > No need for this blank line. > > > > > > > > > > Ok, fix in next patch. > > > > > > > > + if (!ret) { > > > > > + node->vdev_fmt.fmt.meta = f->fmt.meta; > > > > > + dev_ctx->queue[queue_id].fmt.meta = node->vdev_fmt.fmt.meta; > > > > > + } else { > > > > > + dev_warn(&isp_dev->pdev->dev, > > > > > + "s_meta_fm failed, format not support\n"); > > > > > > > > No need for this warning. > > > > > > > > > > Ok, fix in next patch. > > > > > > > > + } > > > > > + > > > > > + return ret; > > > > > +} > > > > > > > > Actually, why do we even need to do all the things? Do we support > > > > multiple different meta formats on the same video node? If not, we can > > > > just have all the TRY_FMT/S_FMT/G_FMT return the hardcoded format. > > > > > > > > > > Ok, it is a good idea. We will return the hardcode format for meta video > > > devices. > > > Below is the revision version. > > > > > > int mtk_cam_meta_enum_format(struct file *file, > > > void *fh, struct v4l2_fmtdesc *f) > > > { > > > struct mtk_cam_video_device *node = file_to_mtk_cam_node(file); > > > > > > f->pixelformat = node->vdev_fmt.fmt.meta.dataformat; > > > > > > return 0; > > > } > > > > Need to error out if f->index > 0. Also need to initialize the other > > output fields - flags and description. > > > > [snip] > > Ok, we will add format index & type checking in the beginning of this > function. Moreover, we will initialize flags & description based on > current meta format value. > > > > > > +static u32 mtk_cam_node_get_v4l2_cap(struct mtk_cam_ctx_queue *node_ctx) > > > > > +{ > > > > > + u32 cap = 0; > > > > > + > > > > > + if (node_ctx->desc.capture) > > > > > + if (node_ctx->desc.image) > > > > > + cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE; > > > > > + else > > > > > + cap = V4L2_CAP_META_CAPTURE; > > > > > + else > > > > > + if (node_ctx->desc.image) > > > > > + cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE; > > > > > + else > > > > > + cap = V4L2_CAP_META_OUTPUT; > > > > > + > > > > > + return cap; > > > > > +} > > > > > > > > Why not just have this defined statically as node_ctx->desc.cap? > > > > > > > > > > Ok, it is refactoring done. > > > > > > > > + > > > > > +static u32 mtk_cam_node_get_format_type(struct mtk_cam_ctx_queue *node_ctx) > > > > > +{ > > > > > + u32 type; > > > > > + > > > > > + if (node_ctx->desc.capture) > > > > > + if (node_ctx->desc.image) > > > > > + type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; > > > > > + else > > > > > + type = V4L2_BUF_TYPE_META_CAPTURE; > > > > > + else > > > > > + if (node_ctx->desc.image) > > > > > + type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE; > > > > > + else > > > > > + type = V4L2_BUF_TYPE_META_OUTPUT; > > > > > + > > > > > + return type; > > > > > +} > > > > > > > > Why not just have this defined statically as node_ctx->desc.buf_type? > > > > > > > > > > Same as above. > > > > > > > > + > > > > > +static const struct v4l2_ioctl_ops *mtk_cam_node_get_ioctl_ops > > > > > + (struct mtk_cam_ctx_queue *node_ctx) > > > > > +{ > > > > > + const struct v4l2_ioctl_ops *ops = NULL; > > > > > + > > > > > + if (node_ctx->desc.image) > > > > > + ops = &mtk_cam_v4l2_ioctl_ops; > > > > > + else > > > > > + ops = &mtk_cam_v4l2_meta_ioctl_ops; > > > > > + return ops; > > > > > +} > > > > > > > > It's also preferable to just put this inside some structure rather > > > > than have getter functions. (node_ctx->desc.ioctl_ops?) > > > > > > > > > > Same as above. > > > Below is the new version for struct mtk_cam_ctx_queue_desc > > > > > > /* > > > * struct mtk_cam_ctx_queue_desc - queue attributes > > > * setup by device context owner > > > * @id: id of the context queue > > > * @name: media entity name > > > * @cap: mapped to V4L2 capabilities > > > * @buf_type: mapped to V4L2 buffer type > > > * @capture: true for capture queue (device to user) > > > * false for output queue (from user to device) > > > * @image: true for image, false for meta data > > > * @smem_alloc: Using the cam_smem_drv as alloc ctx or not > > > * @dma_port: the dma port associated to the buffer > > > * @fmts: supported format > > > * @num_fmts: the number of supported formats > > > * @default_fmt_idx: default format of this queue > > > * @max_buf_count: maximum V4L2 buffer count > > > * @max_buf_count: mapped to v4l2_ioctl_ops > > > */ > > > struct mtk_cam_ctx_queue_desc { > > > u8 id; > > > char *name; > > > u32 cap; > > > u32 buf_type; > > > u32 dma_port; > > > u32 smem_alloc:1; > > > u8 capture:1; > > > u8 image:1; > > > u8 num_fmts; > > > u8 default_fmt_idx; > > > u8 max_buf_count; > > > const struct v4l2_ioctl_ops *ioctl_ops; > > > struct v4l2_format *fmts; > > > }; > > > > SGTM +/- the missing kerneldoc for the new fields. > > > > [snip] > > Ok, we will fix the missing part. > > > > > Sorry, ran out of time for today. Fourth part will come. :) > > > > > > > > Best regards, > > > > Tomasz > > > > > > > > > > Appreciate your support and hard working on this review. > > > We will look forward your part 4 review. > > > > Thanks for replying to all the comments, it's very helpful. > > > > As I mentioned in another reply, I'm going to be busy for the next few > > days, so I'd suggest addressing the existing comments, fixing any > > v4l2-compliance issues and also checking if any changes could be > > applied to the other drivers (DIP, FD, Seninf, MDP) too and then > > sending RFC V1. > > > > Best regards, > > Tomasz > > Ok, we will deliver the RFC V1 next week. > Btw, for v4l2-compliance testing, we have some questions about this. > We list these two issues below. > If you have time, please kindly provide your comments. > Or we could discuss these two issues in RFC V1. > > > 1. Kernel minor issue for 32 bit test program. > => fail: v4l2-test-media.cpp(368): check_0(links.reserved, > sizeof(links.reserved)) > This issue is similar to below patch. > https://patchwork.kernel.org/patch/8578421/ > The reserved fields of media_links_enum is missing copy from kernel > space to user space in media_device_enum_links32 function. > So it makes the above failed. > Do we provide our patch to fix it? Yes, it would be nice if you could send a patch fixing it. > > 2. Some v4l2-compliance testing results are failed in the sensor > drivers's sub devices nodes. which are implemented in ov5695 & ov2685 > driver. Do you suggest us to fix them? Let's look at this off the list and figure out. Please send me an email with the results. Other than that, just to make sure we're all on the same page, please try to fix any remaining v4l2-compliance issues of Mediatek drivers for RFC V1. Best regards, Tomasz
Hello, This is a first version of the RFC patch series adding the driver for Pass 1 (P1) unit in Mediatek's camera ISP system on mt8183 SoC, which will be used in camera features of CrOS. It's the first time Mediatek develops ISP kernel drivers based on V4L2 and media controller framework. I posted the main part of the ISP Pass 1 driver as RFC to discuss first and would like some review comments on the overall architecture of the driver. Pass 1 unit processes image signal from sensor devices and accepts the tuning parameters to adjust the image quality. It performs optical black correction, defect pixel correction, W/IR imbalance correction and lens shading correction for RAW processing. The driver is implemented with V4L2 and media controller framework so we have the following entities to describe the ISP pass 1 path. (The current metadata interface used in meta input and partial meta nodes is only a temporary solution to kick off the driver development and is not ready to be reviewed yet): 1. meta input (output video device): connects to ISP P1 sub device. It accepts the tuning buffer from user. 2. ISP P1 (sub device): connects to partial meta 0, partial meta 1, main stream and packed out video devices. When processing an image, Pass 1 hardware supports multiple output images with different sizes and formats so it needs two capture video devices ("main stream" and "packed out") to return the image data to the user. 3. partial meta 0 (capture video device): return the statistics metadata. 4. partial meta 1 (capture video device): return the statistics metadata. 5. main stream (capture video device): return the processed image data which is used in capture scenario. 6. packed out (capture video device): return the processed image data which is used in preview scenario. The overall file structure of the ISP Pass 1 driver is as following: * mtk_cam.c: Controls the hardware dependent flow and configuration. * mtk_cam-v4l2.c: High-level software context configuration. * mtk_cam-v4l2-util.c: Implements V4L2 and vb2 ops. * mtk_cam-dev-ctx-core.c: Common software flow of the driver. * mtk_cam-dev.c: Implements context independent flow. * mtk_cam-vpu.c: Communicates with the co-processor on the SoC through the VPU driver. * mtk_cam-smem-drv.c: Provides the shared memory management required operations. We reserved a memory region for the co-processor and Pass 1 unit to exchange the tuning and configuration data. Frederic Chen (2): [media] dt-bindings: mt8183: Add binding for ISP Pass 1 shared memory media: platform: Add Mediatek ISP Pass 1 driver KConfig Jungo Lin (5): dts: arm64: mt8183: Add ISP Pass 1 shared memory node [media] dt-bindings: mt8183: Added CAM-SMEM dt-bindings [media] dt-bindings: mt8183: Added camera ISP Pass 1 dt-bindings dts: arm64: mt8183: Add ISP Pass 1 nodes [media] platform: mtk-isp: Add Mediatek ISP Pass 1 driver .../bindings/media/mediatek,cam_smem.txt | 32 + .../bindings/media/mediatek,mt8183-camisp.txt | 59 + .../mediatek,reserve-memory-cam_smem.txt | 44 + arch/arm64/boot/dts/mediatek/mt8183.dtsi | 54 + drivers/media/platform/Kconfig | 2 + drivers/media/platform/Makefile | 2 + drivers/media/platform/mtk-isp/Kconfig | 21 + drivers/media/platform/mtk-isp/Makefile | 14 + drivers/media/platform/mtk-isp/isp_50/Makefile | 17 + drivers/media/platform/mtk-isp/isp_50/cam/Makefile | 35 + .../platform/mtk-isp/isp_50/cam/mtk_cam-ctx.h | 327 ++++ .../mtk-isp/isp_50/cam/mtk_cam-dev-ctx-core.c | 986 +++++++++++++ .../platform/mtk-isp/isp_50/cam/mtk_cam-dev.c | 381 +++++ .../platform/mtk-isp/isp_50/cam/mtk_cam-dev.h | 204 +++ .../platform/mtk-isp/isp_50/cam/mtk_cam-regs.h | 146 ++ .../platform/mtk-isp/isp_50/cam/mtk_cam-smem-drv.c | 452 ++++++ .../platform/mtk-isp/isp_50/cam/mtk_cam-smem.h | 27 + .../mtk-isp/isp_50/cam/mtk_cam-v4l2-util.c | 1555 ++++++++++++++++++++ .../mtk-isp/isp_50/cam/mtk_cam-v4l2-util.h | 49 + .../platform/mtk-isp/isp_50/cam/mtk_cam-v4l2.c | 288 ++++ .../platform/mtk-isp/isp_50/cam/mtk_cam-v4l2.h | 40 + .../platform/mtk-isp/isp_50/cam/mtk_cam-vpu.c | 466 ++++++ .../platform/mtk-isp/isp_50/cam/mtk_cam-vpu.h | 158 ++ .../media/platform/mtk-isp/isp_50/cam/mtk_cam.c | 1235 ++++++++++++++++ .../media/platform/mtk-isp/isp_50/cam/mtk_cam.h | 347 +++++ 25 files changed, 6941 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/mediatek,cam_smem.txt create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8183-camisp.txt create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-cam_smem.txt create mode 100644 drivers/media/platform/mtk-isp/Kconfig create mode 100644 drivers/media/platform/mtk-isp/Makefile create mode 100644 drivers/media/platform/mtk-isp/isp_50/Makefile create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/Makefile create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-ctx.h create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-dev-ctx-core.c create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-dev.c create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-dev.h create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-regs.h create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem-drv.c create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-smem.h create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2-util.c create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2-util.h create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2.c create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2.h create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-vpu.c create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-vpu.h create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.c create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.h