Hi Jungo,
On Tue, Jun 11, 2019 at 11:53:41AM +0800, Jungo Lin wrote:
> Implement standard V4L2 video driver that utilizes V4L2
> and media framework APIs. In this driver, supports one media
> device, one sub-device and seven video devices during
> initialization. Moreover, it also connects with sensor and
> seninf drivers with V4L2 async APIs.
>
> (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.)
>
> Signed-off-by: Jungo Lin <jungo....@mediatek.com>
> ---
> This patch depends on "media: support Mediatek sensor interface driver"[1].
>
> ISP P1 sub-device communicates with seninf sub-device with CIO.
>
> [1]. media: support Mediatek sensor interface driver
> https://patchwork.kernel.org/cover/10979135/
> ---
> .../platform/mtk-isp/isp_50/cam/Makefile | 1 +
> .../mtk-isp/isp_50/cam/mtk_cam-v4l2-util.c | 1674 +++++++++++++++++
> .../mtk-isp/isp_50/cam/mtk_cam-v4l2-util.h | 173 ++
> 3 files changed, 1848 insertions(+)
> 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
>
Thanks for the patch. Please see my comments inline.
[snip]
> +static void mtk_cam_req_try_isp_queue(struct mtk_cam_dev *cam_dev,
> + struct media_request *new_req)
> +{
> + struct mtk_cam_dev_request *req, *req_safe, *cam_dev_req;
> + struct device *dev = &cam_dev->pdev->dev;
> +
> + dev_dbg(dev, "%s new req:%d", __func__, !new_req);
> +
> + if (!cam_dev->streaming) {
> + cam_dev_req = mtk_cam_req_to_dev_req(new_req);
> + spin_lock(&cam_dev->req_lock);
> + list_add_tail(&cam_dev_req->list, &cam_dev->req_list);
> + spin_unlock(&cam_dev->req_lock);
> + dev_dbg(dev, "%s: stream off, no ISP enqueue\n", __func__);
> + return;
> + }
> +
> + /* Normal enqueue flow */
> + if (new_req) {
> + mtk_isp_req_enqueue(dev, new_req);
> + return;
> + }
> +
> + /* Flush all media requests wehen first stream on */
> + list_for_each_entry_safe(req, req_safe, &cam_dev->req_list, list) {
> + list_del(&req->list);
> + mtk_isp_req_enqueue(dev, &req->req);
> + }
> +}
This will have to be redone, as per the other suggestions, but generally one
would have a function that tries to queue as much as possible from a list to
the hardware and another function that adds a request to the list and calls
the first function.
> +
> +static void mtk_cam_req_queue(struct media_request *req)
> +{
> + struct mtk_cam_dev *cam_dev = mtk_cam_mdev_to_dev(req->mdev);
> +
> + vb2_request_queue(req);
> + mtk_cam_req_try_isp_queue(cam_dev, req);
Looks like this driver is suffering from versy similar problems in request
handling as the DIP driver used to.
I'd prefer to save my time and avoid repeating the same comments, so please
check my comments for the DIP driver and apply them to this one too:
https://patchwork.kernel.org/patch/10905223/
> +}
> +
> +static struct media_request *mtk_cam_req_alloc(struct media_device *mdev)
> +{
> + struct mtk_cam_dev_request *cam_dev_req;
> +
> + cam_dev_req = kzalloc(sizeof(*cam_dev_req), GFP_KERNEL);
> +
> + return &cam_dev_req->req;
> +}
> +
> +static void mtk_cam_req_free(struct media_request *req)
> +{
> + struct mtk_cam_dev_request *cam_dev_req = mtk_cam_req_to_dev_req(req);
> +
> + kfree(cam_dev_req);
> +}
> +
> +static __u32 img_get_pixel_byte_by_fmt(__u32 pix_fmt)
Doesn't this function return bits not bytes?
> +{
> + switch (pix_fmt) {
> + case V4L2_PIX_FMT_MTISP_B8:
> + case V4L2_PIX_FMT_MTISP_F8:
> + return 8;
> + case V4L2_PIX_FMT_MTISP_B10:
> + case V4L2_PIX_FMT_MTISP_F10:
> + return 10;
> + case V4L2_PIX_FMT_MTISP_B12:
> + case V4L2_PIX_FMT_MTISP_F12:
> + return 12;
> + case V4L2_PIX_FMT_MTISP_B14:
> + case V4L2_PIX_FMT_MTISP_F14:
> + return 14;
> + default:
> + return 0;
> + }
> +}
> +
> +static __u32 img_cal_main_stream_stride(struct device *dev, __u32 width,
> + __u32 pix_fmt)
> +{
> + __u32 stride;
> + __u32 pixel_byte = img_get_pixel_byte_by_fmt(pix_fmt);
The __ prefixed types should be used only inside UAPI. Please change the
driver to use the normal ones.
> +
> + width = ALIGN(width, 4);
If there is some alignment requirement for width, it should be handled by
TRY_/S_FMT and here we should already assume everything properly aligned.
> + stride = ALIGN(DIV_ROUND_UP(width * pixel_byte, 8), 2);
> +
> + dev_dbg(dev, "main width:%d, stride:%d\n", width, stride);
> +
> + return stride;
> +}
> +
> +static __u32 img_cal_packed_out_stride(struct device *dev, __u32 width,
> + __u32 pix_fmt)
> +{
> + __u32 stride;
> + __u32 pixel_byte = img_get_pixel_byte_by_fmt(pix_fmt);
> +
> + width = ALIGN(width, 4);
Ditto.
> + stride = DIV_ROUND_UP(width * 3, 2);
Could we introduce a local variable for this intermediate value, so that its
name could explain what the value is?
> + stride = DIV_ROUND_UP(stride * pixel_byte, 8);
> +
> + if (pix_fmt == V4L2_PIX_FMT_MTISP_F10)
> + stride = ALIGN(stride, 4);
Is it expected that only the F10 format needs this alignment?
> +
> + dev_dbg(dev, "packed width:%d, stride:%d\n", width, stride);
> +
> + return stride;
> +}
> +
> +static __u32 img_cal_stride(struct device *dev,
> + int node_id,
> + __u32 width,
> + __u32 pix_fmt)
> +{
> + __u32 bpl;
> +
> + /* Currently, only support one_pixel_mode */
> + if (node_id == MTK_CAM_P1_MAIN_STREAM_OUT)
> + bpl = img_cal_main_stream_stride(dev, width, pix_fmt);
> + else if (node_id == MTK_CAM_P1_PACKED_BIN_OUT)
> + bpl = img_cal_packed_out_stride(dev, width, pix_fmt);
> +
> + /* For DIP HW constrained, it needs 4 byte alignment */
> + bpl = ALIGN(bpl, 4);
> +
> + return bpl;
> +}
> +
> +static const struct v4l2_format *
> +mtk_cam_dev_find_fmt(struct mtk_cam_dev_node_desc *desc, u32 format)
> +{
> + unsigned int i;
> + const struct v4l2_format *dev_fmt;
> +
> + for (i = 0; i < desc->num_fmts; i++) {
> + dev_fmt = &desc->fmts[i];
> + if (dev_fmt->fmt.pix_mp.pixelformat == format)
> + return dev_fmt;
> + }
> +
> + return NULL;
> +}
> +
> +/* Calcuate mplane pix format */
> +static void
> +mtk_cam_dev_cal_mplane_fmt(struct device *dev,
> + struct v4l2_pix_format_mplane *dest_fmt,
> + unsigned int node_id)
> +{
> + unsigned int i;
> + __u32 bpl, sizeimage, imagsize;
Perhaps s/sizeimage/plane_size/ and s/imagsize/total_size/?
> +
> + imagsize = 0;
> + for (i = 0 ; i < dest_fmt->num_planes; ++i) {
> + bpl = img_cal_stride(dev,
> + node_id,
> + dest_fmt->width,
> + dest_fmt->pixelformat);
> + sizeimage = bpl * dest_fmt->height;
> + imagsize += sizeimage;
> + dest_fmt->plane_fmt[i].bytesperline = bpl;
> + dest_fmt->plane_fmt[i].sizeimage = sizeimage;
> + memset(dest_fmt->plane_fmt[i].reserved,
> + 0, sizeof(dest_fmt->plane_fmt[i].reserved));
This memset is not needed. The core clears the reserved fields
automatically:
https://elixir.bootlin.com/linux/v5.2/source/drivers/media/v4l2-core/v4l2-ioctl.c#L1559
(We may want to backport the patch that added that to our 4.19 branch.)
> + dev_dbg(dev, "plane:%d,bpl:%d,sizeimage:%u\n",
> + i, bpl, dest_fmt->plane_fmt[i].sizeimage);
> + }
> +
> + if (dest_fmt->num_planes == 1)
> + dest_fmt->plane_fmt[0].sizeimage = imagsize;
Hmm, we only seem to support 1 plane raw formats in this driver. Does the
hardware support any formats with more than 1 plane? If not, all the code
should be simplified to just assume 1 plane.
> +}
> +
> +static void
> +mtk_cam_dev_set_img_fmt(struct device *dev,
> + struct v4l2_pix_format_mplane *dest_fmt,
> + const struct v4l2_pix_format_mplane *src_fmt,
> + unsigned int node_id)
> +{
> + dest_fmt->width = src_fmt->width;
> + dest_fmt->height = src_fmt->height;
> + dest_fmt->pixelformat = src_fmt->pixelformat;
> + dest_fmt->field = src_fmt->field;
> + dest_fmt->colorspace = src_fmt->colorspace;
> + dest_fmt->num_planes = src_fmt->num_planes;
> + /* Use default */
> + dest_fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> + dest_fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> + dest_fmt->xfer_func =
> + V4L2_MAP_XFER_FUNC_DEFAULT(dest_fmt->colorspace);
> + memset(dest_fmt->reserved, 0, sizeof(dest_fmt->reserved));
Given that src_fmt should already be validated and have any fields adjusted
to match the driver requirements, wouldn't all the lines above be equivalent
to *dest_fmt = *src_fmt?
We probably want to move setting all the constant fields to
mtk_cam_vidioc_try_fmt().
> +
> + dev_dbg(dev, "%s: Dest Fmt:%c%c%c%c, w*h:%d*%d\n",
> + __func__,
> + (dest_fmt->pixelformat & 0xFF),
> + (dest_fmt->pixelformat >> 8) & 0xFF,
> + (dest_fmt->pixelformat >> 16) & 0xFF,
> + (dest_fmt->pixelformat >> 24) & 0xFF,
> + dest_fmt->width,
> + dest_fmt->height);
> +
> + mtk_cam_dev_cal_mplane_fmt(dev, dest_fmt, node_id);
This should have been called already before this function was called,
because src_fmt should be already expected to contain valid settings. In
fact, this is already called in mtk_cam_vidioc_try_fmt().
> +}
> +
> +/* Get the default format setting */
> +static void
> +mtk_cam_dev_load_default_fmt(struct device *dev,
Please don't pass struct device pointer around, but instead just the main
driver data struct, which should be much more convenient for accessing
various driver data. Please fix the other functions as well.
> + struct mtk_cam_dev_node_desc *queue_desc,
> + struct v4l2_format *dest)
> +{
> + const struct v4l2_format *default_fmt =
> + &queue_desc->fmts[queue_desc->default_fmt_idx];
> +
> + dest->type = queue_desc->buf_type;
> +
> + /* Configure default format based on node type */
> + if (queue_desc->image) {
> + mtk_cam_dev_set_img_fmt(dev,
> + &dest->fmt.pix_mp,
> + &default_fmt->fmt.pix_mp,
> + queue_desc->id);
We should probably just call mtk_cam_vidioc_s_fmt() here, with a dummy
v4l2_format struct and have any incorrect fields replaced by
mtk_cam_vidioc_try_fmt(), since it's the same logic, as if setting invalid
v4l2_format at runtime.
> + } else {
> + dest->fmt.meta.dataformat = default_fmt->fmt.meta.dataformat;
> + dest->fmt.meta.buffersize = default_fmt->fmt.meta.buffersize;
> + }
> +}
> +
> +static int mtk_cam_isp_open(struct file *file)
> +{
> + struct mtk_cam_dev *cam_dev = video_drvdata(file);
> + struct device *dev = &cam_dev->pdev->dev;
> + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> + int ret;
> +
> + mutex_lock(&cam_dev->lock);
> + ret = v4l2_fh_open(file);
> + if (ret)
> + goto unlock;
> +
> + ret = v4l2_pipeline_pm_use(&node->vdev.entity, 1);
Please don't power on open. Normally applications keep the device nodes open
all the time, so they would keep everything powered on.
Normally this should be done as late as possible, ideally when starting the
streaming.
> + if (ret)
> + dev_err(dev, "%s fail:%d", __func__, ret);
> +
> +unlock:
> + mutex_unlock(&cam_dev->lock);
> +
> + return ret;
> +}
> +
> +static int mtk_cam_isp_release(struct file *file)
> +{
> + struct mtk_cam_dev *cam_dev = video_drvdata(file);
> + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> +
> + mutex_lock(&cam_dev->lock);
> + v4l2_pipeline_pm_use(&node->vdev.entity, 0);
> + vb2_fop_release(file);
> + mutex_unlock(&cam_dev->lock);
> +
> + return 0;
> +}
If we remove power handling from open and release, we should be able to just
use v4l2_fh_open() and vb2_fop_release() directly in the
v4l2_file_operations struct.
> +
> +static struct v4l2_subdev *
> +mtk_cam_cio_get_active_sensor(struct mtk_cam_dev *cam_dev)
> +{
> + struct media_device *mdev = cam_dev->seninf->entity.graph_obj.mdev;
> + struct media_entity *entity;
> + struct device *dev = &cam_dev->pdev->dev;
> + struct v4l2_subdev *sensor;
This variable would be unitialized if there is no streaming sensor. Was
there no compiler warning generated for this?
> +
> + media_device_for_each_entity(entity, mdev) {
> + dev_dbg(dev, "media entity: %s:0x%x\n",
> + entity->name, entity->function);
> + if (entity->function == MEDIA_ENT_F_CAM_SENSOR &&
> + entity->stream_count) {
> + sensor = media_entity_to_v4l2_subdev(entity);
> + dev_dbg(dev, "Sensor found: %s\n", entity->name);
> + break;
> + }
> + }
> +
> + if (!sensor)
> + dev_err(dev, "Sensor is not connected\n");
> +
> + return sensor;
> +}
> +
> +static int mtk_cam_cio_stream_on(struct mtk_cam_dev *cam_dev)
> +{
> + struct device *dev = &cam_dev->pdev->dev;
> + int ret;
> +
> + /* Align vb2_core_streamon design */
> + if (cam_dev->streaming) {
> + dev_warn(dev, "already streaming\n", dev);
> + return 0;
> + }
Could we check this in the caller?
> +
> + if (!cam_dev->seninf) {
> + dev_err(dev, "no seninf connected:%d\n", ret);
> + return -EPERM;
I don't think -EPERM is a good error code here. It's about a missing seninf
device, so perhaps -ENODEV?
> + }
> +
> + /* Get active sensor from graph topology */
> + cam_dev->sensor = mtk_cam_cio_get_active_sensor(cam_dev);
> + if (!cam_dev->sensor)
> + return -EPERM;
> -ENODEV
> +
> + ret = mtk_isp_config(dev);
> + if (ret)
> + return -EPERM;
Maybe just return ret?
> +
> + /* Seninf must stream on first */
> + ret = v4l2_subdev_call(cam_dev->seninf, video, s_stream, 1);
> + if (ret) {
> + dev_err(dev, "%s stream on failed:%d\n",
> + cam_dev->seninf->entity.name, ret);
> + return -EPERM;
return ret?
> + }
> +
> + ret = v4l2_subdev_call(cam_dev->sensor, video, s_stream, 1);
> + if (ret) {
> + dev_err(dev, "%s stream on failed:%d\n",
> + cam_dev->sensor->entity.name, ret);
> + goto fail_sensor_on;
> + }
> +
> + cam_dev->streaming = true;
> + mtk_cam_req_try_isp_queue(cam_dev, NULL);
> + isp_composer_stream(dev, 1);
> + dev_dbg(dev, "streamed on Pass 1\n");
> +
> + return 0;
> +
> +fail_sensor_on:
> + v4l2_subdev_call(cam_dev->seninf, video, s_stream, 0);
> +
> + return -EPERM;
return ret?
> +}
> +
> +static int mtk_cam_cio_stream_off(struct mtk_cam_dev *cam_dev)
> +{
> + struct device *dev = &cam_dev->pdev->dev;
> + int ret;
> +
> + if (!cam_dev->streaming) {
> + dev_warn(dev, "already stream off");
> + return 0;
> + }
Could we check this in the caller?
> +
> + ret = v4l2_subdev_call(cam_dev->sensor, video, s_stream, 0);
> + if (ret) {
> + dev_err(dev, "%s stream off failed:%d\n",
> + cam_dev->sensor->entity.name, ret);
> + return -EPERM;
> + }
> +
> + ret = v4l2_subdev_call(cam_dev->seninf, video, s_stream, 0);
> + if (ret) {
> + dev_err(dev, "%s stream off failed:%d\n",
> + cam_dev->seninf->entity.name, ret);
> + return -EPERM;
> + }
> +
> + isp_composer_stream(dev, 0);
Shouldn't we synchronously wait for the streaming to stop here? Otherwise we
can't guarantee that the hardware releases all the memory that we're going
to free once this function returns.
> + cam_dev->streaming = false;
> + dev_dbg(dev, "streamed off Pass 1\n");
> +
> + return 0;
> +}
> +
> +static int mtk_cam_sd_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> + struct mtk_cam_dev *cam_dev = mtk_cam_subdev_to_dev(sd);
> +
> + if (enable)
> + return mtk_cam_cio_stream_on(cam_dev);
> + else
> + return mtk_cam_cio_stream_off(cam_dev);
> +}
> +
> +static int mtk_cam_sd_subscribe_event(struct v4l2_subdev *subdev,
> + struct v4l2_fh *fh,
> + struct v4l2_event_subscription *sub)
> +{
> + switch (sub->type) {
> + case V4L2_EVENT_FRAME_SYNC:
> + return v4l2_event_subscribe(fh, sub, 0, NULL);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mtk_cam_sd_s_power(struct v4l2_subdev *sd, int on)
> +{
> + struct mtk_cam_dev *cam_dev = mtk_cam_subdev_to_dev(sd);
> +
> + dev_dbg(&cam_dev->pdev->dev, "%s:%d", __func__, on);
> +
> + return on ? mtk_isp_power_init(cam_dev) :
> + mtk_isp_power_release(&cam_dev->pdev->dev);
s_power is a historical thing and we shouldn't be implementing it. Instead,
we should use runtime PM and call pm_runtime_get_sync(), pm_runtime_put()
whenever we start and stop streaming respectively.
> +}
> +
> +static int mtk_cam_media_link_setup(struct media_entity *entity,
> + const struct media_pad *local,
> + const struct media_pad *remote, u32 flags)
> +{
> + struct mtk_cam_dev *cam_dev =
> + container_of(entity, struct mtk_cam_dev, subdev.entity);
> + u32 pad = local->index;
> +
> + dev_dbg(&cam_dev->pdev->dev, "%s: %d -> %d flags:0x%x\n",
> + __func__, pad, remote->index, flags);
> +
> + if (pad < MTK_CAM_P1_TOTAL_NODES)
I assume this check is needed, because the pads with higher indexes are not
video nodes? If so, a comment would be helpful here.
> + cam_dev->vdev_nodes[pad].enabled =
> + !!(flags & MEDIA_LNK_FL_ENABLED);
> +
> + return 0;
> +}
> +
> +static void mtk_cam_vb2_buf_queue(struct vb2_buffer *vb)
> +{
> + struct mtk_cam_dev *mtk_cam_dev = vb2_get_drv_priv(vb->vb2_queue);
> + struct mtk_cam_video_device *node = mtk_cam_vbq_to_vdev(vb->vb2_queue);
> + struct device *dev = &mtk_cam_dev->pdev->dev;
> + struct mtk_cam_dev_buffer *buf;
> +
> + buf = mtk_cam_vb2_buf_to_dev_buf(vb);
This can be folded into the declaration.
> +
> + dev_dbg(dev, "%s: node:%d fd:%d idx:%d\n",
> + __func__,
> + node->id,
> + buf->vbb.request_fd,
> + buf->vbb.vb2_buf.index);
> +
> + /* For request buffers en-queue, handled in mtk_cam_req_try_queue */
> + if (vb->vb2_queue->uses_requests)
> + return;
I'd suggest removing non-request support from this driver. Even if we end up
with a need to provide compatibility for non-request mode, then it should be
built on top of the requests mode, so that the driver itself doesn't have to
deal with two modes.
> +
> + /* Added the buffer into the tracking list */
> + spin_lock(&node->slock);
> + list_add_tail(&buf->list, &node->pending_list);
> + spin_unlock(&node->slock);
> +
> + mtk_isp_enqueue(dev, node->desc.dma_port, buf);
> +}
> +
> +static int mtk_cam_vb2_buf_init(struct vb2_buffer *vb)
> +{
> + struct mtk_cam_dev *cam_dev = vb2_get_drv_priv(vb->vb2_queue);
> + struct device *smem_dev = cam_dev->smem_dev;
> + struct mtk_cam_video_device *node = mtk_cam_vbq_to_vdev(vb->vb2_queue);
> + struct mtk_cam_dev_buffer *buf;
> +
> + buf = mtk_cam_vb2_buf_to_dev_buf(vb);
> + buf->node_id = node->id;
> + buf->daddr = vb2_dma_contig_plane_dma_addr(&buf->vbb.vb2_buf, 0);
> + buf->scp_addr = 0;
Just a reminder that this will have to be reworked according to my comments
for the memory allocation patch.
> +
> + /* scp address is only valid for meta input buffer */
> + if (node->desc.smem_alloc)
> + buf->scp_addr = mtk_cam_smem_iova_to_scp_addr(smem_dev,
> + buf->daddr);
> +
> + return 0;
> +}
> +
> +static int mtk_cam_vb2_buf_prepare(struct vb2_buffer *vb)
> +{
> + struct vb2_v4l2_buffer *v4l2_buf = to_vb2_v4l2_buffer(vb);
> + struct mtk_cam_video_device *node = mtk_cam_vbq_to_vdev(vb->vb2_queue);
> + const struct v4l2_format *fmt = &node->vdev_fmt;
> + unsigned int size;
> +
> + if (vb->vb2_queue->type == V4L2_BUF_TYPE_META_OUTPUT ||
> + vb->vb2_queue->type == V4L2_BUF_TYPE_META_CAPTURE)
> + size = fmt->fmt.meta.buffersize;
> + else
> + size = fmt->fmt.pix_mp.plane_fmt[0].sizeimage;
> +
> + if (vb2_plane_size(vb, 0) < size)
> + return -EINVAL;
For OUTPUT buffers we need to check if vb2_get_plane_payload() == size.
Otherwise we could get not enough or invalid data.
> +
> + v4l2_buf->field = V4L2_FIELD_NONE;
> + vb2_set_plane_payload(vb, 0, size);
This shouldn't be called on OUTPUT buffers.
> +
> + return 0;
> +}
> +
> +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_video_device *node = mtk_cam_vbq_to_vdev(vq);
> + unsigned int max_buffer_count = node->desc.max_buf_count;
> + const struct v4l2_format *fmt = &node->vdev_fmt;
> + unsigned int size;
> +
> + /* Check the limitation of buffer size */
> + if (max_buffer_count)
> + *num_buffers = clamp_val(*num_buffers, 1, max_buffer_count);
> +
> + if (vq->type == V4L2_BUF_TYPE_META_OUTPUT ||
> + vq->type == V4L2_BUF_TYPE_META_CAPTURE)
> + size = fmt->fmt.meta.buffersize;
> + else
> + size = fmt->fmt.pix_mp.plane_fmt[0].sizeimage;
> +
> + /* Add for q.create_bufs with fmt.g_sizeimage(p) / 2 test */
> + if (*num_planes) {
We should also verify that *num_planes == 1, as we don't support more
planes in this driver.
> + if (sizes[0] < size)
> + return -EINVAL;
> + } else {
> + *num_planes = 1;
> + sizes[0] = size;
> + }
> +
> + return 0;
> +}
> +
> +static void mtk_cam_vb2_return_all_buffers(struct mtk_cam_dev *cam_dev,
> + struct mtk_cam_video_device *node,
> + enum vb2_buffer_state state)
> +{
> + struct mtk_cam_dev_buffer *b, *b0;
> + struct mtk_cam_dev_request *req, *req0;
> + struct media_request_object *obj, *obj0;
> + struct vb2_buffer *vb;
> +
> + dev_dbg(&cam_dev->pdev->dev, "%s: node:%s", __func__, node->vdev.name);
> +
> + /* Return all buffers */
> + spin_lock(&node->slock);
> + list_for_each_entry_safe(b, b0, &node->pending_list, list) {
nit: One would normally call the second argument "prev", or "b_prev".
> + vb = &b->vbb.vb2_buf;
> + if (vb->state == VB2_BUF_STATE_ACTIVE)
We shouldn't need to check the buffer state.
> + vb2_buffer_done(vb, state);
> + list_del(&b->list);
> + }
> + spin_unlock(&node->slock);
> +
> + spin_lock(&cam_dev->req_lock);
> + list_for_each_entry_safe(req, req0, &cam_dev->req_list, list) {
nit: Ditto.
> + list_for_each_entry_safe(obj, obj0, &req->req.objects, list) {
Need to check if the object is a buffer.
> + vb = container_of(obj, struct vb2_buffer, req_obj);
> + if (vb->state == VB2_BUF_STATE_ACTIVE)
vb->state shouldn't be accessed directly from the drivers.
Generally, the need to check the state here would suggest that there is
something wrong with how the driver manages the requests. The list that is
being iterated here shouldn't contain any requests that have buffers that
aren't active. That will be achieved if my comments for the request handling
in the DIP driver are applied to this driver as well.
> + vb2_buffer_done(vb, state);
> + }
> + list_del(&req->list);
> + }
> + spin_unlock(&cam_dev->req_lock);
> +
> + if (node->vbq.uses_requests)
> + mtk_isp_req_flush_buffers(&cam_dev->pdev->dev);
> +}
> +
> +static int mtk_cam_vb2_start_streaming(struct vb2_queue *vq,
> + unsigned int count)
> +{
> + struct mtk_cam_dev *cam_dev = vb2_get_drv_priv(vq);
> + struct mtk_cam_video_device *node = mtk_cam_vbq_to_vdev(vq);
> + struct device *dev = &cam_dev->pdev->dev;
> + unsigned int node_count = cam_dev->subdev.entity.use_count;
> + int ret;
> +
> + if (!node->enabled) {
How is this synchronized with mtk_cam_media_link_setup()?
> + dev_err(dev, "Node:%d is not enable\n", node->id);
> + ret = -ENOLINK;
> + goto fail_no_link;
> + }
> +
> + dev_dbg(dev, "%s: count info:%d:%d", __func__,
> + atomic_read(&cam_dev->streamed_node_count), node_count);
> +
> + if (atomic_inc_return(&cam_dev->streamed_node_count) < node_count)
> + return 0;
How do we guarantee that cam_dev->subdev.entity.use_count doesn't change
between calls to this function on different video nodes?
> +
> + /* Start streaming of the whole pipeline now */
> + ret = media_pipeline_start(&node->vdev.entity, &cam_dev->pipeline);
> + if (ret) {
> + dev_err(dev, "%s: Node:%d failed\n", __func__, node->id);
> + goto fail_start_pipeline;
> + }
> +
Related to the above comment: If we start the media pipeline when we start
streaming on the first node, we would naturally prevent the link
configuration changes until the last node stops streaming (as long as the
link is not DYNAMIC). Note that it would only mark the entities as
streaming, but it wouldn't call their s_stream, which I believe is exactly
what we would need to solve the problem above.
> + /* Stream on sub-devices node */
> + ret = v4l2_subdev_call(&cam_dev->subdev, video, s_stream, 1);
> + if (ret) {
> + dev_err(dev, "Node:%d s_stream on failed:%d\n", node->id, ret);
> + goto fail_stream_on;
> + }
> +
> + return 0;
> +
> +fail_stream_on:
> + media_pipeline_stop(&node->vdev.entity);
> +fail_start_pipeline:
> + atomic_dec(&cam_dev->streamed_node_count);
> +fail_no_link:
> + mtk_cam_vb2_return_all_buffers(cam_dev, node, VB2_BUF_STATE_QUEUED);
> +
> + return ret;
> +}
> +
> +static void mtk_cam_vb2_stop_streaming(struct vb2_queue *vq)
> +{
> + struct mtk_cam_dev *cam_dev = vb2_get_drv_priv(vq);
> + struct mtk_cam_video_device *node = mtk_cam_vbq_to_vdev(vq);
> + struct device *dev = &cam_dev->pdev->dev;
> +
> + if (!node->enabled)
> + return;
It shouldn't be possible for this to happen, because nobody could have
called start_streaming on a disabled node.
> +
> + mtk_cam_vb2_return_all_buffers(cam_dev, node, VB2_BUF_STATE_ERROR);
Shouldn't we stop streaming first, so that the hardware operation is
cancelled and any buffers owned by the hardware are released?
> +
> + dev_dbg(dev, "%s: count info:%d", __func__,
> + cam_dev->subdev.entity.stream_count);
> +
> + /* Check the first node to stream-off */
> + if (!cam_dev->subdev.entity.stream_count)
> + return;
> +
> + media_pipeline_stop(&node->vdev.entity);
> +
> + if (v4l2_subdev_call(&cam_dev->subdev, video, s_stream, 0))
> + dev_err(dev, "failed to stop streaming\n");
> +}
> +
> +static void mtk_cam_vb2_buf_request_complete(struct vb2_buffer *vb)
> +{
> + struct mtk_cam_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
> +
> + v4l2_ctrl_request_complete(vb->req_obj.req,
> + dev->v4l2_dev.ctrl_handler);
This would end up being called multiple times, once for each video node.
Instead, this should be called explicitly by the driver when it completed
the request - perhaps in the frame completion handler?
With that, we probably wouldn't even need this callback.
> +}
> +
> +static int mtk_cam_vidioc_querycap(struct file *file, void *fh,
> + struct v4l2_capability *cap)
> +{
> + struct mtk_cam_dev *cam_dev = video_drvdata(file);
> +
> + strscpy(cap->driver, MTK_CAM_DEV_P1_NAME, sizeof(cap->driver));
> + strscpy(cap->card, MTK_CAM_DEV_P1_NAME, sizeof(cap->card));
We could just use dev_driver_name(cam_dev->dev) for both.
> + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> + dev_name(cam_dev->media_dev.dev));
We should just store dev in cam_dev.
> +
> + return 0;
> +}
> +
> +static int mtk_cam_vidioc_enum_fmt(struct file *file, void *fh,
> + struct v4l2_fmtdesc *f)
> +{
> + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> +
> + if (f->index >= node->desc.num_fmts)
> + return -EINVAL;
> +
> + f->pixelformat = node->desc.fmts[f->index].fmt.pix_mp.pixelformat;
Is the set of formats available always the same regardless of the sensor
format?
> + f->flags = 0;
We need f->description too.
> +
> + return 0;
> +}
> +
> +static int mtk_cam_vidioc_g_fmt(struct file *file, void *fh,
> + struct v4l2_format *f)
> +{
> + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> +
> + if (!node->desc.num_fmts)
> + return -EINVAL;
When would that condition happen?
> +
> + f->fmt = node->vdev_fmt.fmt;
> +
> + return 0;
> +}
> +
> +static int mtk_cam_vidioc_try_fmt(struct file *file, void *fh,
> + struct v4l2_format *in_fmt)
> +{
> + struct mtk_cam_dev *cam_dev = video_drvdata(file);
> + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> + const struct v4l2_format *dev_fmt;
> + __u32 width, height;
Don't use __ types in implementation, they are here for UAPI purposes. There
is u32, which you could use instead, but for width and height you don't need
explicit size, so unsigned int should be good.
> +
> + dev_dbg(&cam_dev->pdev->dev, "%s: fmt:%c%c%c%c, w*h:%u*%u\n",
> + __func__,
> + (in_fmt->fmt.pix_mp.pixelformat & 0xFF),
> + (in_fmt->fmt.pix_mp.pixelformat >> 8) & 0xFF,
> + (in_fmt->fmt.pix_mp.pixelformat >> 16) & 0xFF,
> + (in_fmt->fmt.pix_mp.pixelformat >> 24) & 0xFF,
> + in_fmt->fmt.pix_mp.width, in_fmt->fmt.pix_mp.height);
> +
> + width = in_fmt->fmt.pix_mp.width;
> + height = in_fmt->fmt.pix_mp.height;
> +
> + dev_fmt = mtk_cam_dev_find_fmt(&node->desc,
> + in_fmt->fmt.pix_mp.pixelformat);
> + if (dev_fmt) {
> + mtk_cam_dev_set_img_fmt(&cam_dev->pdev->dev,
> + &in_fmt->fmt.pix_mp,
> + &dev_fmt->fmt.pix_mp,
> + node->id);
> + } else {
> + mtk_cam_dev_load_default_fmt(&cam_dev->pdev->dev,
> + &node->desc, in_fmt);
We shouldn't just load a default format. This function should validate all
the fields one by one and adjust them to something appropriate.
> + }
CodingStyle: No braces if both if and else bodies have only 1 statement
each.
> + in_fmt->fmt.pix_mp.width = clamp_t(u32,
> + width,
> + CAM_MIN_WIDTH,
> + in_fmt->fmt.pix_mp.width);
Shouldn't we clamp this with some maximum value too?
> + in_fmt->fmt.pix_mp.height = clamp_t(u32,
> + height,
> + CAM_MIN_HEIGHT,
> + in_fmt->fmt.pix_mp.height);
Ditto.
> + mtk_cam_dev_cal_mplane_fmt(&cam_dev->pdev->dev,
> + &in_fmt->fmt.pix_mp, node->id);
> +
> + return 0;
> +}
> +
> +static int mtk_cam_vidioc_s_fmt(struct file *file, void *fh,
> + struct v4l2_format *f)
> +{
> + struct mtk_cam_dev *cam_dev = video_drvdata(file);
> + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> +
> + if (cam_dev->streaming)
> + return -EBUSY;
I think this should rather be something like vb2_queue_is_busy(), which
would prevent format changes if buffers are allocated.
> +
> + /* Get the valid format */
> + mtk_cam_vidioc_try_fmt(file, fh, f);
> +
> + /* Configure to video device */
> + mtk_cam_dev_set_img_fmt(&cam_dev->pdev->dev,
> + &node->vdev_fmt.fmt.pix_mp,
> + &f->fmt.pix_mp,
> + node->id);
> +
> + return 0;
> +}
> +
> +static int mtk_cam_vidioc_enum_input(struct file *file, void *fh,
> + struct v4l2_input *input)
> +{
> + if (input->index)
> + return -EINVAL;
> +
> + strscpy(input->name, "camera", sizeof(input->name));
> + input->type = V4L2_INPUT_TYPE_CAMERA;
> +
> + return 0;
> +}
> +
> +static int mtk_cam_vidioc_g_input(struct file *file, void *fh,
> + unsigned int *input)
> +{
> + *input = 0;
> +
> + return 0;
> +}
> +
> +static int mtk_cam_vidioc_s_input(struct file *file,
> + void *fh, unsigned int input)
> +{
> + return input == 0 ? 0 : -EINVAL;
> +}
> +
> +static int mtk_cam_vidioc_enum_framesizes(struct file *filp, void *priv,
> + struct v4l2_frmsizeenum *sizes)
> +{
> + struct mtk_cam_video_device *node = file_to_mtk_cam_node(filp);
> + const struct v4l2_format *dev_fmt;
> +
> + dev_fmt = mtk_cam_dev_find_fmt(&node->desc, sizes->pixel_format);
> + if (!dev_fmt || sizes->index)
> + return -EINVAL;
> +
> + sizes->type = node->desc.frmsizes->type;
> + memcpy(&sizes->stepwise, &node->desc.frmsizes->stepwise,
> + sizeof(sizes->stepwise));
> +
> + return 0;
> +}
> +
> +static int mtk_cam_vidioc_meta_enum_fmt(struct file *file, void *fh,
> + struct v4l2_fmtdesc *f)
> +{
> + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> +
> + if (f->index)
> + return -EINVAL;
> +
> + strscpy(f->description, node->desc.description,
> + sizeof(node->desc.description));
> + f->pixelformat = node->vdev_fmt.fmt.meta.dataformat;
> + f->flags = 0;
> +
> + return 0;
> +}
> +
> +static int mtk_cam_vidioc_g_meta_fmt(struct file *file, void *fh,
> + struct v4l2_format *f)
> +{
> + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> +
> + f->fmt.meta.dataformat = node->vdev_fmt.fmt.meta.dataformat;
> + f->fmt.meta.buffersize = node->vdev_fmt.fmt.meta.buffersize;
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_core_ops mtk_cam_subdev_core_ops = {
> + .subscribe_event = mtk_cam_sd_subscribe_event,
> + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> + .s_power = mtk_cam_sd_s_power,
> +};
> +
> +static const struct v4l2_subdev_video_ops mtk_cam_subdev_video_ops = {
> + .s_stream = mtk_cam_sd_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 = {
nit: mtk_cam_media_entity_ops?
> + .link_setup = mtk_cam_media_link_setup,
> + .link_validate = v4l2_subdev_link_validate,
> +};
> +
> +static const struct vb2_ops mtk_cam_vb2_ops = {
> + .queue_setup = mtk_cam_vb2_queue_setup,
> + .wait_prepare = vb2_ops_wait_prepare,
> + .wait_finish = vb2_ops_wait_finish,
> + .buf_init = mtk_cam_vb2_buf_init,
> + .buf_prepare = mtk_cam_vb2_buf_prepare,
> + .start_streaming = mtk_cam_vb2_start_streaming,
> + .stop_streaming = mtk_cam_vb2_stop_streaming,
> + .buf_queue = mtk_cam_vb2_buf_queue,
> + .buf_request_complete = mtk_cam_vb2_buf_request_complete,
> +};
> +
> +static const struct v4l2_file_operations mtk_cam_v4l2_fops = {
> + .unlocked_ioctl = video_ioctl2,
> + .open = mtk_cam_isp_open,
> + .release = mtk_cam_isp_release,
> + .poll = vb2_fop_poll,
> + .mmap = vb2_fop_mmap,
> +#ifdef CONFIG_COMPAT
> + .compat_ioctl32 = v4l2_compat_ioctl32,
> +#endif
> +};
> +
> +static const struct media_device_ops mtk_cam_media_req_ops = {
nit: Those are media ops, so perhaps just mtk_cam_media_ops?
> + .link_notify = v4l2_pipeline_link_notify,
> + .req_alloc = mtk_cam_req_alloc,
> + .req_free = mtk_cam_req_free,
> + .req_validate = vb2_request_validate,
> + .req_queue = mtk_cam_req_queue,
> +};
> +
> +static int mtk_cam_media_register(struct device *dev,
> + struct media_device *media_dev)
> +{
> + media_dev->dev = dev;
> + strscpy(media_dev->model, MTK_CAM_DEV_P1_NAME,
Could we replace any use of this macro with dev_driver_string(dev) and then
delete the macro? The less name strings in the driver the better, as there
is less change for confusing the userspace if few different names are used
at the same time.
> + sizeof(media_dev->model));
> + snprintf(media_dev->bus_info, sizeof(media_dev->bus_info),
> + "platform:%s", dev_name(dev));
> + media_dev->hw_revision = 0;
> + media_device_init(media_dev);
> + media_dev->ops = &mtk_cam_media_req_ops;
> +
> + return media_device_register(media_dev);
> +}
> +
> +static int mtk_cam_video_register_device(struct mtk_cam_dev *cam_dev, u32 i)
> +{
> + struct device *dev = &cam_dev->pdev->dev;
> + struct mtk_cam_video_device *node = &cam_dev->vdev_nodes[i];
Would it make sense to pass node as an argument to this function instead of
(or in addition to) i?
> + struct video_device *vdev = &node->vdev;
> + struct vb2_queue *vbq = &node->vbq;
> + u32 output = !cam_dev->vdev_nodes[i].desc.capture;
Why not call it capture instead and avoid the inversion?
> + u32 link_flags = cam_dev->vdev_nodes[i].desc.link_flags;
> + int ret;
> +
> + cam_dev->subdev_pads[i].flags = output ?
> + MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE;
> +
> + /* Initialize media entities */
> + ret = media_entity_pads_init(&vdev->entity, 1, &node->vdev_pad);
> + if (ret) {
> + dev_err(dev, "failed initialize media pad:%d\n", ret);
> + return ret;
> + }
> + node->enabled = false;
Are all the nodes optional? If there is any required node, it should be
always enabled and have the MEDIA_LNK_FL_IMMUTABLE flag set.
> + node->id = i;
> + node->vdev_pad.flags = cam_dev->subdev_pads[i].flags;
Hmm, shouldn't the subdev pads have opposite directions (sink vs source)?
> + mtk_cam_dev_load_default_fmt(&cam_dev->pdev->dev,
> + &node->desc,
> + &node->vdev_fmt);
> +
> + /* Initialize vbq */
> + vbq->type = node->vdev_fmt.type;
> + if (vbq->type == V4L2_BUF_TYPE_META_OUTPUT)
> + vbq->io_modes = VB2_MMAP;
> + else
> + vbq->io_modes = VB2_MMAP | VB2_DMABUF;
> +
> + if (node->desc.smem_alloc) {
> + vbq->bidirectional = 1;
> + vbq->dev = cam_dev->smem_dev;
> + } else {
> + vbq->dev = &cam_dev->pdev->dev;
> + }
> +
> + if (vbq->type == V4L2_BUF_TYPE_META_CAPTURE)
> + vdev->entity.function =
> + MEDIA_ENT_F_PROC_VIDEO_STATISTICS;
This is a video node, so it's just a DMA, not a processing entity. I believe
all the entities corresponding to video nodes should use MEDIA_ENT_F_IO_V4L.
> + vbq->ops = &mtk_cam_vb2_ops;
> + vbq->mem_ops = &vb2_dma_contig_memops;
> + vbq->buf_struct_size = sizeof(struct mtk_cam_dev_buffer);
> + vbq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> + vbq->min_buffers_needed = 0; /* Can streamon w/o buffers */
> + /* Put the process hub sub device in the vb2 private data */
What is "process hub" and what "sub device" is this about?
> + vbq->drv_priv = cam_dev;
> + vbq->lock = &node->lock;
> + vbq->supports_requests = true;
> +
> + ret = vb2_queue_init(vbq);
> + if (ret) {
> + dev_err(dev, "failed to init. vb2 queue:%d\n", ret);
> + goto fail_vb2_queue;
> + }
> +
> + /* Initialize vdev */
> + snprintf(vdev->name, sizeof(vdev->name), "%s %s",
> + MTK_CAM_DEV_P1_NAME, node->desc.name);
> + /* set cap/type/ioctl_ops of the video device */
> + vdev->device_caps = node->desc.cap | V4L2_CAP_STREAMING;
> + vdev->ioctl_ops = node->desc.ioctl_ops;
> + vdev->fops = &mtk_cam_v4l2_fops;
> + vdev->release = video_device_release_empty;
> + vdev->lock = &node->lock;
> + vdev->v4l2_dev = &cam_dev->v4l2_dev;
> + vdev->queue = &node->vbq;
> + vdev->vfl_dir = output ? VFL_DIR_TX : VFL_DIR_RX;
> + vdev->entity.ops = NULL;
> + /* Enable private control for image video devices */
> + if (node->desc.image) {
> + mtk_cam_ctrl_init(cam_dev, &node->ctrl_handler);
> + vdev->ctrl_handler = &node->ctrl_handler;
> + }
> + video_set_drvdata(vdev, cam_dev);
> + dev_dbg(dev, "register vdev:%d:%s\n", i, vdev->name);
> +
> + ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> + if (ret) {
> + dev_err(dev, "failed to register vde:%d\n", ret);
> + goto fail_vdev;
> + }
> +
> + /* Create link between video node and the subdev pad */
> + if (output) {
> + ret = media_create_pad_link(&vdev->entity, 0,
> + &cam_dev->subdev.entity,
> + i, link_flags);
> + } else {
> + ret = media_create_pad_link(&cam_dev->subdev.entity,
> + i, &vdev->entity, 0,
> + link_flags);
> + }
> + if (ret)
> + goto fail_link;
> +
> + /* Initialize miscellaneous variables */
> + mutex_init(&node->lock);
> + spin_lock_init(&node->slock);
> + INIT_LIST_HEAD(&node->pending_list);
This should be all initialized before registering the video device.
Otherwise userspace could open the device before these are initialized.
> +
> + return 0;
> +
> +fail_link:
> + video_unregister_device(vdev);
> +fail_vdev:
> + vb2_queue_release(vbq);
> +fail_vb2_queue:
> + media_entity_cleanup(&vdev->entity);
> +
> + return ret;
> +}
> +
> +static int mtk_cam_mem2mem2_v4l2_register(struct mtk_cam_dev *cam_dev)
This function doesn't have anything to do with mem2mem. How about
mtk_cam_v4l2_register()?
Perhaps it would make sense to move any media related code into into
mtk_cam_media_register(), keep only V4L2 related code here and have the
caller call the former first and then this one, rather than having such deep
sequence of nested calls, which makes the driver harder to read.
> +{
> + struct device *dev = &cam_dev->pdev->dev;
How about just storing dev, instead of pdev in the struct? Also, calling the
argument "cam", would make it as short as cam->dev.
> + /* Total pad numbers is video devices + one seninf pad */
> + unsigned int num_subdev_pads = MTK_CAM_CIO_PAD_SINK + 1;
> + unsigned int i;
> + int ret;
> +
> + ret = mtk_cam_media_register(dev,
> + &cam_dev->media_dev);
> + if (ret) {
> + dev_err(dev, "failed to register media device:%d\n", ret);
> + return ret;
> + }
> + dev_info(dev, "Register media device: %s, 0x%pK",
> + MTK_CAM_DEV_P1_NAME, cam_dev->media_dev);
An info message should be useful to the user in some way. Printing kernel
pointers isn't useful. Something like "registered media0" could be useful to
let the user know which media device is associated with this driver if there
is more than one in the system.
> +
> + /* Set up v4l2 device */
> + cam_dev->v4l2_dev.mdev = &cam_dev->media_dev;
> + ret = v4l2_device_register(dev, &cam_dev->v4l2_dev);
> + if (ret) {
> + dev_err(dev, "failed to register V4L2 device:%d\n", ret);
> + goto fail_v4l2_dev;
Please call the labels after the cleanup step that needs to be done. It
makes it easier to spot any ordering errors.
> + }
> + dev_info(dev, "Register v4l2 device: 0x%pK", cam_dev->v4l2_dev);
Same as above.
> +
> + /* Initialize subdev media entity */
> + cam_dev->subdev_pads = devm_kcalloc(dev, num_subdev_pads,
> + sizeof(*cam_dev->subdev_pads),
> + GFP_KERNEL);
> + if (!cam_dev->subdev_pads) {
> + ret = -ENOMEM;
> + goto fail_subdev_pads;
> + }
> +
> + ret = media_entity_pads_init(&cam_dev->subdev.entity,
> + num_subdev_pads,
> + cam_dev->subdev_pads);
> + if (ret) {
> + dev_err(dev, "failed initialize media pads:%d:\n", ret);
Stray ":" at the end of the message.
> + goto fail_subdev_pads;
> + }
> +
> + /* Initialize all pads with MEDIA_PAD_FL_SOURCE */
> + for (i = 0; i < num_subdev_pads; i++)
> + cam_dev->subdev_pads[i].flags = MEDIA_PAD_FL_SOURCE;
> +
> + /* Customize the last one pad as CIO sink pad. */
> + cam_dev->subdev_pads[MTK_CAM_CIO_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> +
> + /* Initialize subdev */
> + v4l2_subdev_init(&cam_dev->subdev, &mtk_cam_subdev_ops);
> + cam_dev->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_STATISTICS;
> + cam_dev->subdev.entity.ops = &mtk_cam_media_ops;
> + cam_dev->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE |
> + V4L2_SUBDEV_FL_HAS_EVENTS;
> + snprintf(cam_dev->subdev.name, sizeof(cam_dev->subdev.name),
> + "%s", MTK_CAM_DEV_P1_NAME);
> + v4l2_set_subdevdata(&cam_dev->subdev, cam_dev);
> +
> + ret = v4l2_device_register_subdev(&cam_dev->v4l2_dev, &cam_dev->subdev);
> + if (ret) {
> + dev_err(dev, "failed initialize subdev:%d\n", ret);
> + goto fail_subdev;
> + }
> + dev_info(dev, "register subdev: %s\n", cam_dev->subdev.name);
> +
> + /* Create video nodes and links */
> + for (i = 0; i < MTK_CAM_P1_TOTAL_NODES; i++) {
> + ret = mtk_cam_video_register_device(cam_dev, i);
> + if (ret)
> + goto fail_video_register;
> + }
> +
> + vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
> +
> + return 0;
> +
> +fail_video_register:
> + i--;
This could be moved into the for clause, as the initialization statement.
> + for (; i >= 0; i--) {
i is unsigned. Did this compile without warnings?
> + video_unregister_device(&cam_dev->vdev_nodes[i].vdev);
> + media_entity_cleanup(&cam_dev->vdev_nodes[i].vdev.entity);
> + mutex_destroy(&cam_dev->vdev_nodes[i].lock);
Should we move this into mtk_cam_video_unregister_device() to be consistent
with registration?
> + }
> +fail_subdev:
> + media_entity_cleanup(&cam_dev->subdev.entity);
> +fail_subdev_pads:
> + v4l2_device_unregister(&cam_dev->v4l2_dev);
> +fail_v4l2_dev:
> + dev_err(dev, "fail_v4l2_dev mdev: 0x%pK:%d", &cam_dev->media_dev, ret);
Please print errors only where they actually happen, not at the cleanup.
> + media_device_unregister(&cam_dev->media_dev);
> + media_device_cleanup(&cam_dev->media_dev);
> +
> + return ret;
> +}
> +
> +static int mtk_cam_v4l2_unregister(struct mtk_cam_dev *cam_dev)
> +{
> + unsigned int i;
> + struct mtk_cam_video_device *dev;
nit: Move the declaration inside the for loop, since the variable is only
used there.
> +
> + for (i = 0; i < MTK_CAM_P1_TOTAL_NODES; i++) {
> + dev = &cam_dev->vdev_nodes[i];
> + video_unregister_device(&dev->vdev);
> + media_entity_cleanup(&dev->vdev.entity);
> + mutex_destroy(&dev->lock);
> + if (dev->desc.image)
> + v4l2_ctrl_handler_free(&dev->ctrl_handler);
> + }
> +
> + vb2_dma_contig_clear_max_seg_size(&cam_dev->pdev->dev);
> +
> + v4l2_device_unregister_subdev(&cam_dev->subdev);
> + media_entity_cleanup(&cam_dev->subdev.entity);
> + kfree(cam_dev->subdev_pads);
This was allocated using devm_kcalloc(), so no need to free it explicitly.
> +
> + v4l2_device_unregister(&cam_dev->v4l2_dev);
> + media_device_unregister(&cam_dev->media_dev);
> + media_device_cleanup(&cam_dev->media_dev);
> +
> + return 0;
> +}
> +
> +static int mtk_cam_dev_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct mtk_cam_dev *cam_dev =
> + container_of(notifier, struct mtk_cam_dev, notifier);
> + struct device *dev = &cam_dev->pdev->dev;
> + int ret;
> +
> + ret = media_create_pad_link(&cam_dev->seninf->entity,
> + MTK_CAM_CIO_PAD_SRC,
> + &cam_dev->subdev.entity,
> + MTK_CAM_CIO_PAD_SINK,
> + 0);
> + if (ret) {
> + dev_err(dev, "fail to create pad link %s %s err:%d\n",
> + cam_dev->seninf->entity.name,
> + cam_dev->subdev.entity.name,
> + ret);
> + return ret;
> + }
> +
> + dev_info(dev, "Complete the v4l2 registration\n");
dev_dbg()
> +
> + ret = v4l2_device_register_subdev_nodes(&cam_dev->v4l2_dev);
> + if (ret) {
> + dev_err(dev, "failed initialize subdev nodes:%d\n", ret);
> + return ret;
> + }
> +
> + return ret;
> +}
Why not just put the contents of this function inside
mtk_cam_dev_notifier_complete()?
> +
> +static int mtk_cam_dev_notifier_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *sd,
> + struct v4l2_async_subdev *asd)
> +{
> + struct mtk_cam_dev *cam_dev =
> + container_of(notifier, struct mtk_cam_dev, notifier);
> +
Should we somehow check that the entity we got is seninf indeed and there
was no mistake in DT?
> + cam_dev->seninf = sd;
> + dev_info(&cam_dev->pdev->dev, "%s is bounded\n", sd->entity.name);
bound
Also please make this dev_dbg().
> + return 0;
> +}
> +
> +static void mtk_cam_dev_notifier_unbind(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *sd,
> + struct v4l2_async_subdev *asd)
> +{
> + struct mtk_cam_dev *cam_dev =
> + container_of(notifier, struct mtk_cam_dev, notifier);
> +
> + cam_dev->seninf = NULL;
> + dev_dbg(&cam_dev->pdev->dev, "%s is unbounded\n", sd->entity.name);
unbound
> +}
> +
> +static int mtk_cam_dev_notifier_complete(struct v4l2_async_notifier
> *notifier)
> +{
> + return mtk_cam_dev_complete(notifier);
> +}
> +
> +static const struct v4l2_async_notifier_operations mtk_cam_async_ops = {
> + .bound = mtk_cam_dev_notifier_bound,
> + .unbind = mtk_cam_dev_notifier_unbind,
> + .complete = mtk_cam_dev_notifier_complete,
> +};
> +
> +static int mtk_cam_v4l2_async_register(struct mtk_cam_dev *cam_dev)
> +{
> + struct device *dev = &cam_dev->pdev->dev;
> + int ret;
> +
> + ret = v4l2_async_notifier_parse_fwnode_endpoints(dev,
> + &cam_dev->notifier, sizeof(struct v4l2_async_subdev),
> + NULL);
> + if (ret)
> + return ret;
> +
> + if (!cam_dev->notifier.num_subdevs)
> + return -ENODEV;
Could we print some error messages for the 2 cases above?
> +
> + cam_dev->notifier.ops = &mtk_cam_async_ops;
> + dev_info(&cam_dev->pdev->dev, "mtk_cam v4l2_async_notifier_register\n");
dev_dbg()
> + ret = v4l2_async_notifier_register(&cam_dev->v4l2_dev,
> + &cam_dev->notifier);
> + if (ret) {
> + dev_err(&cam_dev->pdev->dev,
> + "failed to register async notifier : %d\n", ret);
> + v4l2_async_notifier_cleanup(&cam_dev->notifier);
> + }
> +
> + return ret;
> +}
> +
> +static void mtk_cam_v4l2_async_unregister(struct mtk_cam_dev *cam_dev)
> +{
> + v4l2_async_notifier_unregister(&cam_dev->notifier);
> + v4l2_async_notifier_cleanup(&cam_dev->notifier);
> +}
> +
> +static const struct v4l2_ioctl_ops mtk_cam_v4l2_vcap_ioctl_ops = {
> + .vidioc_querycap = mtk_cam_vidioc_querycap,
> + .vidioc_enum_framesizes = mtk_cam_vidioc_enum_framesizes,
> + .vidioc_enum_fmt_vid_cap_mplane = mtk_cam_vidioc_enum_fmt,
> + .vidioc_g_fmt_vid_cap_mplane = mtk_cam_vidioc_g_fmt,
> + .vidioc_s_fmt_vid_cap_mplane = mtk_cam_vidioc_s_fmt,
> + .vidioc_try_fmt_vid_cap_mplane = mtk_cam_vidioc_try_fmt,
> + .vidioc_enum_input = mtk_cam_vidioc_enum_input,
> + .vidioc_g_input = mtk_cam_vidioc_g_input,
> + .vidioc_s_input = mtk_cam_vidioc_s_input,
I don't think we need vidioc_*_input. At least the current implementation in
this patch doesn't seem to do anything useful.
> + /* buffer queue management */
Drop this comment, as it's obvious that the callbacks with "buf" in the name
are related to buffers, there are some non-buffer callbacks below too (e.g.
vidioc_subscribe_event) and also the other ops structs below don't have such
comment.
> + .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 = vb2_ioctl_qbuf,
> + .vidioc_dqbuf = vb2_ioctl_dqbuf,
> + .vidioc_streamon = vb2_ioctl_streamon,
> + .vidioc_streamoff = vb2_ioctl_streamoff,
> + .vidioc_expbuf = vb2_ioctl_expbuf,
> + .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> + .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> +};
> +
> +static const struct v4l2_ioctl_ops mtk_cam_v4l2_meta_cap_ioctl_ops = {
> + .vidioc_querycap = mtk_cam_vidioc_querycap,
> + .vidioc_enum_fmt_meta_cap = mtk_cam_vidioc_meta_enum_fmt,
> + .vidioc_g_fmt_meta_cap = mtk_cam_vidioc_g_meta_fmt,
> + .vidioc_s_fmt_meta_cap = mtk_cam_vidioc_g_meta_fmt,
> + .vidioc_try_fmt_meta_cap = mtk_cam_vidioc_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 = vb2_ioctl_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_out_ioctl_ops = {
> + .vidioc_querycap = mtk_cam_vidioc_querycap,
> + .vidioc_enum_fmt_meta_out = mtk_cam_vidioc_meta_enum_fmt,
> + .vidioc_g_fmt_meta_out = mtk_cam_vidioc_g_meta_fmt,
> + .vidioc_s_fmt_meta_out = mtk_cam_vidioc_g_meta_fmt,
> + .vidioc_try_fmt_meta_out = mtk_cam_vidioc_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 = vb2_ioctl_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_format meta_fmts[] = {
> + {
> + .fmt.meta = {
> + .dataformat = V4L2_META_FMT_MTISP_PARAMS,
> + .buffersize = 128 * PAGE_SIZE,
PAGE_SIZE is a weird unit for specifying generic buffer sizes. How about
making it 512 * SZ_1K?
That said, it should normally be just sizeof(struct some_struct_used_here).
Same for the other entries below.
> + },
> + },
> + {
> + .fmt.meta = {
> + .dataformat = V4L2_META_FMT_MTISP_3A,
> + .buffersize = 300 * PAGE_SIZE,
> + },
> + },
> + {
> + .fmt.meta = {
> + .dataformat = V4L2_META_FMT_MTISP_AF,
> + .buffersize = 160 * PAGE_SIZE,
> + },
> + },
> + {
> + .fmt.meta = {
> + .dataformat = V4L2_META_FMT_MTISP_LCS,
> + .buffersize = 72 * PAGE_SIZE,
> + },
> + },
> + {
> + .fmt.meta = {
> + .dataformat = V4L2_META_FMT_MTISP_LMV,
> + .buffersize = 256,
> + },
> + },
> +};
> +
> +static const struct v4l2_format stream_out_fmts[] = {
> + {
> + .fmt.pix_mp = {
> + .width = IMG_MAX_WIDTH,
> + .height = IMG_MAX_HEIGHT,
> + .pixelformat = V4L2_PIX_FMT_MTISP_B8,
> + .field = V4L2_FIELD_NONE,
> + .colorspace = V4L2_COLORSPACE_SRGB,
> + .num_planes = 1,
> + },
> + },
> + {
> + .fmt.pix_mp = {
> + .width = IMG_MAX_WIDTH,
> + .height = IMG_MAX_HEIGHT,
> + .pixelformat = V4L2_PIX_FMT_MTISP_B10,
> + .field = V4L2_FIELD_NONE,
> + .colorspace = V4L2_COLORSPACE_SRGB,
> + .num_planes = 1,
> + },
> + },
> + {
> + .fmt.pix_mp = {
> + .width = IMG_MAX_WIDTH,
> + .height = IMG_MAX_HEIGHT,
> + .pixelformat = V4L2_PIX_FMT_MTISP_B12,
> + .field = V4L2_FIELD_NONE,
> + .colorspace = V4L2_COLORSPACE_SRGB,
> + .num_planes = 1,
> + },
> + },
> + {
> + .fmt.pix_mp = {
> + .width = IMG_MAX_WIDTH,
> + .height = IMG_MAX_HEIGHT,
> + .pixelformat = V4L2_PIX_FMT_MTISP_B14,
> + .field = V4L2_FIELD_NONE,
> + .colorspace = V4L2_COLORSPACE_SRGB,
> + .num_planes = 1,
> + },
> + },
> +};
> +
> +static const struct v4l2_format bin_out_fmts[] = {
> + {
> + .fmt.pix_mp = {
> + .width = RRZ_MAX_WIDTH,
> + .height = RRZ_MAX_HEIGHT,
> + .pixelformat = V4L2_PIX_FMT_MTISP_F8,
> + .field = V4L2_FIELD_NONE,
> + .colorspace = V4L2_COLORSPACE_RAW,
> + .num_planes = 1,
> + },
> + },
> + {
> + .fmt.pix_mp = {
> + .width = RRZ_MAX_WIDTH,
> + .height = RRZ_MAX_HEIGHT,
> + .pixelformat = V4L2_PIX_FMT_MTISP_F10,
> + .field = V4L2_FIELD_NONE,
> + .colorspace = V4L2_COLORSPACE_RAW,
> + .num_planes = 1,
> + },
> + },
> + {
> + .fmt.pix_mp = {
> + .width = RRZ_MAX_WIDTH,
> + .height = RRZ_MAX_HEIGHT,
> + .pixelformat = V4L2_PIX_FMT_MTISP_F12,
> + .field = V4L2_FIELD_NONE,
> + .colorspace = V4L2_COLORSPACE_RAW,
> + .num_planes = 1,
> + },
> + },
> + {
> + .fmt.pix_mp = {
> + .width = RRZ_MAX_WIDTH,
> + .height = RRZ_MAX_HEIGHT,
> + .pixelformat = V4L2_PIX_FMT_MTISP_F14,
> + .field = V4L2_FIELD_NONE,
> + .colorspace = V4L2_COLORSPACE_RAW,
> + .num_planes = 1,
> + },
> + },
> +};
> +
> +static const struct v4l2_frmsizeenum img_frm_size_nums[] = {
> + {
> + .index = 0,
> + .type = V4L2_FRMSIZE_TYPE_CONTINUOUS,
> + .stepwise = {
> + .max_width = IMG_MAX_WIDTH,
> + .min_width = IMG_MIN_WIDTH,
> + .max_height = IMG_MAX_HEIGHT,
> + .min_height = IMG_MIN_HEIGHT,
> + .step_height = 1,
> + .step_width = 1,
> + },
> + },
> + {
> + .index = 0,
> + .type = V4L2_FRMSIZE_TYPE_CONTINUOUS,
> + .stepwise = {
> + .max_width = RRZ_MAX_WIDTH,
> + .min_width = RRZ_MIN_WIDTH,
> + .max_height = RRZ_MAX_HEIGHT,
> + .min_height = RRZ_MIN_HEIGHT,
> + .step_height = 1,
> + .step_width = 1,
> + },
> + },
> +};
> +
> +static const struct
> +mtk_cam_dev_node_desc output_queues[MTK_CAM_P1_TOTAL_OUTPUT] = {
> + {
> + .id = MTK_CAM_P1_META_IN_0,
> + .name = "meta input",
> + .description = "ISP tuning parameters",
> + .cap = V4L2_CAP_META_OUTPUT,
> + .buf_type = V4L2_BUF_TYPE_META_OUTPUT,
> + .link_flags = 0,
> + .capture = false,
> + .image = false,
> + .smem_alloc = true,
> + .fmts = meta_fmts,
> + .num_fmts = ARRAY_SIZE(meta_fmts),
> + .default_fmt_idx = 0,
> + .max_buf_count = 10,
> + .ioctl_ops = &mtk_cam_v4l2_meta_out_ioctl_ops,
> + },
> +};
> +
> +static const struct
> +mtk_cam_dev_node_desc capture_queues[MTK_CAM_P1_TOTAL_CAPTURE] = {
> + {
> + .id = MTK_CAM_P1_MAIN_STREAM_OUT,
> + .name = "main stream",
> + .cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE,
> + .buf_type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
> + .link_flags = 0,
> + .capture = true,
> + .image = true,
> + .smem_alloc = false,
> + .dma_port = R_IMGO,
> + .fmts = stream_out_fmts,
> + .num_fmts = ARRAY_SIZE(stream_out_fmts),
> + .default_fmt_idx = 1,
Why not just make it always 0 and move the default format to the beginning
of stream_out_fmts[]?
> + .ioctl_ops = &mtk_cam_v4l2_vcap_ioctl_ops,
> + .frmsizes = &img_frm_size_nums[0],
nit: If you only need to point to these data once, you could define them in
place, as a compound literal, like
> .frmsizes = &(struct v4l2_framesizeenum) {
> // initialize here
> },
> + },
> + {
> + .id = MTK_CAM_P1_PACKED_BIN_OUT,
> + .name = "packed out",
> + .cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE,
> + .buf_type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
> + .link_flags = 0,
> + .capture = true,
> + .image = true,
> + .smem_alloc = false,
> + .dma_port = R_RRZO,
> + .fmts = bin_out_fmts,
> + .num_fmts = ARRAY_SIZE(bin_out_fmts),
> + .default_fmt_idx = 1,
> + .ioctl_ops = &mtk_cam_v4l2_vcap_ioctl_ops,
> + .frmsizes = &img_frm_size_nums[1],
> + },
> + {
> + .id = MTK_CAM_P1_META_OUT_0,
> + .name = "partial meta 0",
> + .description = "AE/AWB histogram",
> + .cap = V4L2_CAP_META_CAPTURE,
> + .buf_type = V4L2_BUF_TYPE_META_CAPTURE,
> + .link_flags = 0,
> + .capture = true,
> + .image = false,
> + .smem_alloc = false,
> + .dma_port = R_AAO | R_FLKO | R_PSO,
> + .fmts = meta_fmts,
> + .num_fmts = ARRAY_SIZE(meta_fmts),
> + .default_fmt_idx = 1,
> + .max_buf_count = 5,
> + .ioctl_ops = &mtk_cam_v4l2_meta_cap_ioctl_ops,
> + },
> + {
> + .id = MTK_CAM_P1_META_OUT_1,
> + .name = "partial meta 1",
> + .description = "AF histogram",
> + .cap = V4L2_CAP_META_CAPTURE,
> + .buf_type = V4L2_BUF_TYPE_META_CAPTURE,
> + .link_flags = 0,
> + .capture = true,
> + .image = false,
> + .smem_alloc = false,
> + .dma_port = R_AFO,
> + .fmts = meta_fmts,
> + .num_fmts = ARRAY_SIZE(meta_fmts),
> + .default_fmt_idx = 2,
> + .max_buf_count = 5,
> + .ioctl_ops = &mtk_cam_v4l2_meta_cap_ioctl_ops,
> + },
> + {
> + .id = MTK_CAM_P1_META_OUT_2,
> + .name = "partial meta 2",
> + .description = "Local contrast enhanced statistics",
> + .cap = V4L2_CAP_META_CAPTURE,
> + .buf_type = V4L2_BUF_TYPE_META_CAPTURE,
> + .link_flags = 0,
> + .capture = true,
> + .image = false,
> + .smem_alloc = false,
> + .dma_port = R_LCSO,
> + .fmts = meta_fmts,
> + .num_fmts = ARRAY_SIZE(meta_fmts),
> + .default_fmt_idx = 3,
> + .max_buf_count = 10,
> + .ioctl_ops = &mtk_cam_v4l2_meta_cap_ioctl_ops,
> + },
> + {
> + .id = MTK_CAM_P1_META_OUT_3,
> + .name = "partial meta 3",
> + .description = "Local motion vector histogram",
> + .cap = V4L2_CAP_META_CAPTURE,
> + .buf_type = V4L2_BUF_TYPE_META_CAPTURE,
> + .link_flags = 0,
link_flags seems to be 0 for all nodes.
> + .capture = true,
We already know this from .buf_type. We can check using the
V4L2_TYPE_IS_OUTPUT() macro.
> + .image = false,
> + .smem_alloc = false,
> + .dma_port = R_LMVO,
> + .fmts = meta_fmts,
> + .num_fmts = ARRAY_SIZE(meta_fmts),
I don't think this is correct. The description suggests one specific format
(local motion vector histogram), so the queue should probably be hardwired
to that format?
> + .default_fmt_idx = 4,
> + .max_buf_count = 10,
Where does this number come from?
> + .ioctl_ops = &mtk_cam_v4l2_meta_cap_ioctl_ops,
> + },
> +};
> +
> +/* The helper to configure the device context */
> +static void mtk_cam_dev_queue_setup(struct mtk_cam_dev *cam_dev)
> +{
> + unsigned int i, node_idx;
> +
> + node_idx = 0;
> +
> + /* Setup the output queue */
> + for (i = 0; i < MTK_CAM_P1_TOTAL_OUTPUT; i++)
< ARRAY_SIZE(output_queues)
> + cam_dev->vdev_nodes[node_idx++].desc = output_queues[i];
> +
> + /* Setup the capture queue */
> + for (i = 0; i < MTK_CAM_P1_TOTAL_CAPTURE; i++)
< ARRAY_SIZE(capture_queues)
> + cam_dev->vdev_nodes[node_idx++].desc = capture_queues[i];
> +}
> +
> +int mtk_cam_dev_init(struct platform_device *pdev,
> + struct mtk_cam_dev *cam_dev)
> +{
> + int ret;
> +
> + cam_dev->pdev = pdev;
Do we need this additional mtk_cam_dev struct? Couldn't we just use
mtk_isp_p1 here?
> + mtk_cam_dev_queue_setup(cam_dev);
> + /* v4l2 sub-device registration */
> +
> + dev_dbg(&cam_dev->pdev->dev, "mem2mem2.name: %s\n",
> + MTK_CAM_DEV_P1_NAME);
This debugging message doesn't seem very useful. Please remove.
> + ret = mtk_cam_mem2mem2_v4l2_register(cam_dev);
> + if (ret)
> + return ret;
> +
> + ret = mtk_cam_v4l2_async_register(cam_dev);
> + if (ret) {
> + mtk_cam_v4l2_unregister(cam_dev);
Please use an error path on the bottom of the function instead. With goto
labels named after the next clean-up step that needs to be performed.
> + return ret;
> + }
> +
> + spin_lock_init(&cam_dev->req_lock);
> + INIT_LIST_HEAD(&cam_dev->req_list);
> + mutex_init(&cam_dev->lock);
> +
> + return 0;
> +}
> +
> +int mtk_cam_dev_release(struct platform_device *pdev,
"release" is normally used for file_operations. How about "cleanup"?
> + struct mtk_cam_dev *cam_dev)
> +{
> + mtk_cam_v4l2_async_unregister(cam_dev);
> + mtk_cam_v4l2_unregister(cam_dev);
> +
> + mutex_destroy(&cam_dev->lock);
> +
> + return 0;
> +}
I'd suggest moving any generic API implementation code (platform_device,
V4L2, VB2, Media Controller, etc.) to mtk_cam.c and then moving any low
level hardware/firmware-related code from mtk_cam.c and mtk_cam-scp.c to
mtk_cam_hw.c.
> +
diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2-util.h
b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2-util.h
new file mode 100644
index 000000000000..825cdf20643a
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2-util.h
@@ -0,0 +1,173 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + */
> +
> +#ifndef __MTK_CAM_DEV_V4L2_H__
> +#define __MTK_CAM_DEV_V4L2_H__
> +
> +#include <linux/device.h>
> +#include <linux/types.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/videodev2.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/videobuf2-core.h>
> +#include <media/videobuf2-v4l2.h>
> +
> +#define MTK_CAM_DEV_P1_NAME "MTK-ISP-P1-V4L2"
Maybe it's not a critical thing, but generally it's a good practice to just
explicitly specific this name somewhere, e.g. struct
platform_driver::driver::name and then just refer to dev_driver_name(). It
makes it easier to make sure that the name stays the same everywhere.
> +
> +#define MTK_CAM_P1_META_IN_0 0
> +#define MTK_CAM_P1_TOTAL_OUTPUT 1
Since these are just some utility definitions, we can use enum instead of
assigning the values by hand.
> +
> +#define MTK_CAM_P1_MAIN_STREAM_OUT 1
> +#define MTK_CAM_P1_PACKED_BIN_OUT 2
> +#define MTK_CAM_P1_META_OUT_0 3
> +#define MTK_CAM_P1_META_OUT_1 4
> +#define MTK_CAM_P1_META_OUT_2 5
> +#define MTK_CAM_P1_META_OUT_3 6
> +#define MTK_CAM_P1_TOTAL_CAPTURE 6
Ditto.
> +
> +#define MTK_CAM_P1_TOTAL_NODES 7
Please just add the two totals together rather than manually specifying the
value.
> +
> +struct mtk_cam_dev_request {
> + struct media_request req;
> + struct list_head list;
> +};
> +
> +struct mtk_cam_dev_buffer {
> + struct vb2_v4l2_buffer vbb;
> + struct list_head list;
> + /* Intenal part */
> + dma_addr_t daddr;
> + dma_addr_t scp_addr;
> + unsigned int node_id;
> +};
Could you add kerneldoc comments for the 2 structs?
> +
> +/*
> + * struct mtk_cam_dev_node_desc - node attributes
> + *
> + * @id: id of the context queue
> + * @name: media entity name
> + * @description: descritpion of node
> + * @cap: mapped to V4L2 capabilities
> + * @buf_type: mapped to V4L2 buffer type
> + * @dma_port: the dma port associated to the buffer
> + * @link_flags: default media link flags
> + * @smem_alloc: using the cam_smem_drv as alloc ctx or not
> + * @capture: true for capture queue (device to user)
> + * false for output queue (from user to device)
> + * @image: true for image node, false for meta node
> + * @num_fmts: the number of supported formats
> + * @default_fmt_idx: default format of this node
> + * @max_buf_count: maximum V4L2 buffer count
> + * @ioctl_ops: mapped to v4l2_ioctl_ops
> + * @fmts: supported format
> + * @frmsizes: supported frame size number
> + *
> + */
> +struct mtk_cam_dev_node_desc {
> + u8 id;
> + char *name;
> + char *description;
> + u32 cap;
> + u32 buf_type;
> + u32 dma_port;
> + u32 link_flags;
> + u8 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;
> + const struct v4l2_format *fmts;
> + const struct v4l2_frmsizeenum *frmsizes;
> +};
> +
> +/*
> + * struct mtk_cam_video_device - Mediatek video device structure.
> + *
> + * @id: Id for mtk_cam_dev_node_desc or mem2mem2_nodes array
> + * @enabled: Indicate the device is enabled or not
> + * @vdev_fmt: The V4L2 format of video device
> + * @vdev_apd: The media pad graph object of video device
vdev_pad?
> + * @vbq: A videobuf queue of video device
> + * @desc: The node attributes of video device
> + * @ctrl_handler: The control handler of video device
> + * @pending_list: List for pending buffers before enqueuing into driver
> + * @lock: Serializes vb2 queue and video device operations.
> + * @slock: Protect for pending_list.
> + *
Please fix the order of the documentation to match the order of the struct.
> + */
> +struct mtk_cam_video_device {
> + unsigned int id;
> + unsigned int enabled;
> + struct v4l2_format vdev_fmt;
> + struct mtk_cam_dev_node_desc desc;
> + struct video_device vdev;
Not documented above.
> + struct media_pad vdev_pad;
> + struct vb2_queue vbq;
> + struct v4l2_ctrl_handler ctrl_handler;
> + struct list_head pending_list;
> + /* Used for vbq & vdev */
It's already documented in the kerneldoc comment.
> + struct mutex lock;
> + /* protect for pending_list */
It's already documented in the kerneldoc comment.
> + spinlock_t slock;
How about calling it pending_list_lock?
> +};
> +
> +/*
> + * struct mtk_cam_dev - Mediatek camera device structure.
> + *
> + * @pdev: Pointer to platform device
> + * @smem_pdev: Pointer to shared memory platform device
> + * @pipeline: Media pipeline information
> + * @media_dev: Media device
> + * @subdev: The V4L2 sub-device
> + * @v4l2_dev: The V4L2 device driver
> + * @notifier: The v4l2_device notifier data
> + * @subdev_pads: Pointer to the number of media pads of this sub-device
> + * @ctrl_handler: The control handler
> + * @vdev_nodes: The array list of mtk_cam_video_device nodes
> + * @seninf: Pointer to the seninf sub-device
> + * @sensor: Pointer to the active sensor V4L2 sub-device when streaming on
> + * @lock: The mutex protecting video device open/release operations
> + * @streaming: Indicate the overall streaming status is on or off
> + * @streamed_node_count: The number of V4L2 video device nodes are streaming
> on
> + * @req_list: Lins to keep media requests before streaming on
> + * @req_lock: Protect the req_list data
> + *
> + * Below is the graph topology for Camera IO connection.
> + * sensor 1 (main) --> sensor IF --> P1 sub-device
> + * sensor 2 (sub) -->
This probably isn't the best place for graph topology description. I think
we actually want a separate documentation file for this, similar to
Documentation/media/v4l-drivers/ipu3.rst.
> + *
> + */
> +struct mtk_cam_dev {
> + struct platform_device *pdev;
> + struct device *smem_dev;
> + struct media_pipeline pipeline;
> + struct media_device media_dev;
> + struct v4l2_subdev subdev;
> + struct v4l2_device v4l2_dev;
> + struct v4l2_async_notifier notifier;
> + struct media_pad *subdev_pads;
> + struct v4l2_ctrl_handler ctrl_handler;
> + struct mtk_cam_video_device vdev_nodes[MTK_CAM_P1_TOTAL_NODES];
> + struct v4l2_subdev *seninf;
> + struct v4l2_subdev *sensor;
> + /* protect video device open/release operations */
It's already documented in the kerneldoc comment.
> + struct mutex lock;
> + unsigned int streaming:1;
> + atomic_t streamed_node_count;
> + struct list_head req_list;
> + /* protect for req_list */
It's already documented in the kerneldoc comment.
> + spinlock_t req_lock;
How about calling it req_list_lock?
Best regards,
Tomasz
