Re: [RFC,v3 6/9] media: platform: Add Mediatek ISP P1 V4L2 functions

[Jungo Lin <jungo.lin@mediatek.com>]

Hi, Tomasz:

On Tue, 2019-07-23 at 19:21 +0900, Tomasz Figa wrote:
> Hi Jungo,
> 
> On Thu, Jul 18, 2019 at 1:39 PM Jungo Lin <jungo.lin@mediatek.com> wrote:
> >
> > Hi, Tomasz:
> >
> > On Wed, 2019-07-10 at 18:54 +0900, Tomasz Figa wrote:
> > > Hi Jungo,
> > >
> > > On Tue, Jun 11, 2019 at 11:53:41AM +0800, Jungo Lin wrote:
> [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.
> > >
> >
> > We revised this function as below.
> > First to check the en-queue conditions:
> > a. stream on
> > b. The composer buffers in SCP are 3, so we only could has 3 jobs
> > at the same time.
> >
> >
> > Second, try to en-queue the frames in the pending job if possible and
> > move them into running job list if possible.
> >
> > The request has been inserted into pending job in mtk_cam_req_validate
> > which is used to validate media_request.
> 
> Thanks for replying to each of the comments, that's very helpful.
> Snipped out the parts that I agreed with.
> 
> Please note that req_validate is not supposed to change any driver
> state. It's only supposed to validate the request. req_queue is the
> right callback to insert the request into some internal driver
> bookkeeping structures.
> 

Yes, in req_validate function, we don't change any driver state.
Below is the function's implementation.

a. Call vb2_request_validate(req) to verify media request.
b. Update the buffer internal structure buffer.
c. Insert the request into pending_job_list to prepare en-queue.

static int mtk_cam_req_validate(struct media_request *req)
{
	struct mtk_cam_dev_request *cam_req = mtk_cam_req_to_dev_req(req);
	struct mtk_cam_dev *cam = container_of(req->mdev, struct mtk_cam_dev,
					       media_dev);
	struct media_request_object *req_obj;
	unsigned long flags;
	int ret;

	/* run buffer prepare function to initialize buffer DMA address */
	ret = vb2_request_validate(req);
	if (ret) {
		dev_err(cam->dev, "vb2_request_validate failed:%d\n", ret);
		return ret;
	}

	/* update frame_params */
	list_for_each_entry(req_obj, &req->objects, list) {
		struct vb2_buffer *vb;
		struct mtk_cam_dev_buffer *buf;

		if (!vb2_request_object_is_buffer(req_obj))
			continue;

		vb = container_of(req_obj, struct vb2_buffer, req_obj);
		buf = mtk_cam_vb2_buf_to_dev_buf(vb);
		cam_req->frame_params.dma_bufs[buf->node_id].iova = buf->daddr;
		cam_req->frame_params.dma_bufs[buf->node_id].scp_addr =
			buf->scp_addr;
	}
	atomic_set(&cam_req->buf_count, vb2_request_buffer_cnt(req));

	/* add to pending job list */
	spin_lock_irqsave(&cam->pending_job_lock, flags);
	list_add_tail(&cam_req->list, &cam->pending_job_list);
	spin_unlock_irqrestore(&cam->pending_job_lock, flags);

	return 0;
}

> >
> > void mtk_cam_dev_req_try_queue(struct mtk_cam_dev *cam_dev)
> > {
> >         struct mtk_cam_dev_request *req, *req_prev;
> >         struct list_head enqueue_job_list;
> >         int buffer_cnt = atomic_read(&cam_dev->running_job_count);
> >         unsigned long flags;
> >
> >         if (!cam_dev->streaming ||
> >             buffer_cnt >= MTK_ISP_MAX_RUNNING_JOBS) {
> 
> Do we have a guarantee that cam_dev->running_job_count doesn't
> decrement between the atomic_read() above and this line?
> 

Ok, we will use cam->pending_job_lock to protect
cam_dev->running_job_count access. Below is the revised version.

void mtk_cam_dev_req_try_queue(struct mtk_cam_dev *cam)
{
	struct mtk_cam_dev_request *req, *req_prev;
	unsigned long flags;

	if (!cam->streaming) {
		dev_dbg(cam->dev, "stream is off\n");
		return;
	}

	spin_lock_irqsave(&cam->pending_job_lock, flags);
	if (atomic_read(&cam->running_job_count) >= MTK_ISP_MAX_RUNNING_JOBS) {
		dev_dbg(cam->dev, "jobs are full\n");
		spin_unlock_irqrestore(&cam->pending_job_lock, flags);
		return;
	}
	list_for_each_entry_safe(req, req_prev, &cam->pending_job_list, list) {
		list_del(&req->list);
		spin_lock_irqsave(&cam->running_job_lock, flags);
		list_add_tail(&req->list, &cam->running_job_list);
		mtk_isp_req_enqueue(cam, req);
		spin_unlock_irqrestore(&cam->running_job_lock, flags);
		if (atomic_inc_return(&cam->running_job_count) >=
			MTK_ISP_MAX_RUNNING_JOBS)
			break;
	}
	spin_unlock_irqrestore(&cam->pending_job_lock, flags);
}

> >                 dev_dbg(cam_dev->dev, "stream off or buffers are full:%d\n",
> >                         buffer_cnt);
> >                 return;
> >         }
> >
> >         INIT_LIST_HEAD(&enqueue_job_list);
> >
> >         spin_lock(&cam_dev->pending_job_lock);
> >         list_for_each_entry_safe(req, req_prev,
> >                                  &cam_dev->pending_job_list, list) {
> >                 list_del(&req->list);
> >                 list_add_tail(&req->list, &enqueue_job_list);
> 
> What's the reason to use the second list? Could we just take one job
> from pending_job_list, enqueue it and then iterate again?
> 

Yes, we could simply the code block to remove enqueue_job_list.

> >                 if (atomic_inc_return(&cam_dev->running_job_count) >=
> >                         MTK_ISP_MAX_RUNNING_JOBS)
> >                         break;
> >         }
> >         spin_unlock(&cam_dev->pending_job_lock);
> >
> >         list_for_each_entry_safe(req, req_prev,
> >                                  &enqueue_job_list, list) {
> >                 list_del(&req->list);
> >                 spin_lock_irqsave(&cam_dev->running_job_lock, flags);
> >                 list_add_tail(&req->list, &cam_dev->running_job_list);
> >                 spin_unlock_irqrestore(&cam_dev->running_job_lock, flags);
> >
> 
> Do we have a guarantee that another thread doesn't run the same
> function ending up calling mtk_isp_req_enqueue() with another request
> before this one and thus making the order of running_job_list
> incorrect?
> 

In the new implementation, we use cam->pending_job_lock to protect this
scenario.

> >                 mtk_isp_req_enqueue(cam_dev, req);
> >         }
> > }
> >
> [snip]
> > > > +   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?
> > >
> >
> > yes, if the pixel bits of image format is 10, the byte alignment of bpl
> > should be 4. Otherwise, it is 8. We will revise this and add more
> > comments.
> 
> That means that the B10 format also needs the extra alignment, as
> opposed to what the original code did, right?
> 

Sorry for short code snippet.
This alignment checking is only applied to F10, no B10.
If you like to check the full function, you could check this in this
link[1].

static void cal_image_pix_mp(struct mtk_cam_dev *cam, unsigned int
node_id,
			     struct v4l2_pix_format_mplane *mp)
{
	unsigned int bpl, ppl;
	unsigned int pixel_bits = get_pixel_bits(mp->pixelformat);
	unsigned int width = mp->width;

	if (node_id == MTK_CAM_P1_MAIN_STREAM_OUT) {
		/* bayer encoding format & 2 bytes alignment */
		bpl = ALIGN(DIV_ROUND_UP(width * pixel_bits, 8), 2);
	} else if (node_id == MTK_CAM_P1_PACKED_BIN_OUT) {
		/*
		 * The FULL-G encoding format
		 * 1 G component per pixel
		 * 1 R component per 4 pixel
		 * 1 B component per 4 pixel
		 * Total 4G/1R/1B in 4 pixel (pixel per line:ppl)
		 */
		ppl = DIV_ROUND_UP(width * 6, 4);
		bpl = DIV_ROUND_UP(ppl * pixel_bits, 8);

		/* 4 bytes alignment for 10 bit & others are 8 bytes */
		if (pixel_bits == 10)
			bpl = ALIGN(bpl, 4);
		else
			bpl = ALIGN(bpl, 8);
	} 

[1]
https://crrev.com/c/1712885/2/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.c#303

> >
> > /* 4 bytes alignment for 10 bit other are 8 bytes alignment */
> >         if (pixel_bits == 10)
> >                 bpl = ALIGN(bpl, 4);
> >         else
> >                 bpl = ALIGN(bpl, 8);
> 
> SGTM, thanks.
> 
> [snip]

Thanks for your review.

> > > > +
> > > > +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?
> > >
> >
> > No, there is no compiler warning.
> > But, we will assign sensor to NULL to avoid unnecessary compiler warning
> > with different compiler options.
> >
> 
> Thanks. It would be useful if you could check why the compiler you're
> using doesn't show a warning here. We might be missing other
> uninitialized variables.
> 

We will feedback to your project team to check the possible reason about
compiler warning issue.

> [snip]
> > > > +}
> > > > +
> > > > +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.
> > >
> >
> > Yes, we will new comment as below.
> >
> >         /*
> >          * Check video nodes is enabled by link setup.
> >          * The pad index of video node should be less than
> >          * MTK_CAM_P1_TOTAL_NODES.
> >          */
> >         if (pad < MTK_CAM_P1_TOTAL_NODES)
> >                 cam_dev->vdev_nodes[pad].enabled =
> >                         !!(flags & MEDIA_LNK_FL_ENABLED);
> >
> 
> Could we rephrase this a bit. The comment still doesn't explain why
> the index should be less than the constant. Perhaps:
> 
> /*
>  * The video nodes exposed by the driver have pads indexes
>  * from 0 to MTK_CAM_P1_TOTAL_NODES - 1.
>  */
> 
> [snip]
> 

Thanks for your suggestion.
We will update this.

> > > > +
> > > > +   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.
> > >
> >
> > The purpose of non-request function in this driver is needed by
> > our camera middle-ware design. It needs 3A statistics buffers before
> > image buffers en-queue. So we need to en-queue 3A statistics with
> > non-request mode in this driver. After MW got the 3A statistics data, it
> > will en-queue the images, tuning buffer and other meta buffers with
> > request mode. Based on this requirement, do you have any suggestion?
> > For upstream driver, should we only consider request mode?
> >
> 
> Where does that requirement come from? Why the timing of queuing of
> the buffers to the driver is important?
> 
> [snip]

Basically, this requirement comes from our internal camera
middle-ware/3A hal in user space. Since this is not generic requirement,
we will follow your original suggestion to keep the request mode only
and remove other non-request design in other files. For upstream driver,
it should support request mode only.

> > > > +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()?
> > >
> >
> > We will follow your suggestion and below is our proposal for this
> > function.
> >
> > 1. Use !cam_dev->pipeline.streaming_count to decide the first node to
> > stream-on.
> > 2.a If yes, do the following steps
> >     2.a-1 Call media_pipeline_start function to prevent the link
> > configuration changes.
> >     2.a-2 Call mtk_cam_dev_init_stream function to calculate how many
> > video nodes are enabled and save it into cam_dev->enabled_node_count.
> >     2.a-3 Initialize ISP P1 HW in mtk_isp_hw_init function since end
> > user has called stream-on API
> > 2.b jump step 3.
> >
> > 3. Use cam_dev->streamed_node_count to track how many video nodes are
> > streamed by user space.
> > 4. Check all enabled video nodes are streamed or not based on
> > cam_dev->streamed_node_count & cam_dev->enabled_node_count.
> > 5. If yes, call s_stream on for P1 sub-device
> >
> > Do you think it is reasonable?
> >
> 
> That should work indeed.
> 
> [snip]

Ok, thanks for your confirmation.

> > > > +
> > > > +   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?
> > >
> >
> > For this function, below is the new code flow.
> >
> > 1. Check the first node to stream off based on
> > cam_dev->streamed_node_count & cam_dev->enabled_node_count.
> > 2. If yes, call all s_stream off for P1 sub-device
> > 3. Call mtk_cam_vb2_return_all_buffers for each node
> > 4. Check the last node to stream off
> > 5. If yes, call media_pipeline_stop to allow user space
> > to perform link configuration changes, such as disable link.
> >
> > But, for step 5, is it too late for end user to disable link?
> > For example, for first node, it has called stream off but
> > can't call disable link until the last node is stream off?
> >
> 
> I think that should be okay. From the userspace point of view, having
> one of the video nodes streaming implies that the related subdevice
> could be streaming as well. The links between the video nodes and the
> subdevices don't have the DYNAMIC flag, so the userspace should expect
> that it can't change any links connecting to the subdevice when the
> subdevice could be streaming.
> 

Ok, got your point. We will keep this design.

> [snip]
> > > > +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.
> > >
> >
> > First, if we don't implement this callback function, we will receive
> > kernel warning as below.
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-v4l2.c#L420
> >
> > Second, this function is only be called in __vb2_queue_cancel function.
> > Moreover, we will remove cam_dev->v4l2_dev.ctrl_handler in next patch.
> > So could we just implement dummy empty function?
> >
> >  * @buf_request_complete: a buffer that was never queued to the driver
> > but is
> >  *                      associated with a queued request was canceled.
> >  *                      The driver will have to mark associated objects in the
> >  *                      request as completed; required if requests are
> >  *                      supported.
> >
> 
> Good catch, thanks.
> 
> Sounds like we may indeed need to implement this callback. In
> particular, we may need to remove the request that the buffer was
> associated with from the driver queue and return the other buffers
> associated to it with an error state. This should be similar to
> handling a request failure.
> [snip]

Before calling this callback function, the VB2's stop_streaming has been
called. Normally, we will return the buffers belonged to this vb2 queu
with error state. On other hand, only if the state of request is
MEDIA_REQUEST_STATE_QUEUED, the buf_request_complete will be called in
__vb2_queue_cancel function. It hints this media request has been
validated and inserted into our driver's pending_job_list or
running_job_list. So we will call mtk_cam_dev_req_cleanup() remove these
requests from driver's list when streaming is off. Since we have no
v4l2_ctrl, do we need to do the above things which is already handled in
mtk_cam_vb2_stop_streaming function? Maybe is this callback function
only designed for v4l2_ctrl_request_complete usage?

static void mtk_cam_dev_req_cleanup(struct mtk_cam_dev *cam)
{
	struct mtk_cam_dev_request *req, *req_prev;
	unsigned long flags;

	dev_dbg(cam->dev, "%s\n", __func__);

	spin_lock_irqsave(&cam->pending_job_lock, flags);
	list_for_each_entry_safe(req, req_prev, &cam->pending_job_list, list)
		list_del(&req->list);
	spin_unlock_irqrestore(&cam->pending_job_lock, flags);

	spin_lock_irqsave(&cam->running_job_lock, flags);
	list_for_each_entry_safe(req, req_prev, &cam->running_job_list, list)
		list_del(&req->list);
	spin_unlock_irqrestore(&cam->running_job_lock, flags);
}

static void mtk_cam_vb2_stop_streaming(struct vb2_queue *vq)
{
	struct mtk_cam_dev *cam = vb2_get_drv_priv(vq);
	struct mtk_cam_video_device *node = mtk_cam_vbq_to_vdev(vq);
	struct device *dev = cam->dev;

	dev_dbg(dev, "%s node:%d count info:%d", __func__,
		node->id, atomic_read(&cam->stream_count));

	mutex_lock(&cam->op_lock);
	if (atomic_read(&cam->stream_count) == cam->enabled_count)
		if (v4l2_subdev_call(&cam->subdev, video, s_stream, 0))
			dev_err(dev, "failed to stop streaming\n");

	mtk_cam_vb2_return_all_buffers(cam, node, VB2_BUF_STATE_ERROR);

	/* Check the first node to stream-off */
	if (!atomic_dec_and_test(&cam->stream_count)) {
		mutex_unlock(&cam->op_lock);
		return;
	}
	mutex_unlock(&cam->op_lock);

	mtk_cam_dev_req_cleanup(cam);
	media_pipeline_stop(&node->vdev.entity);
}

> > > > +
> > > > +   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?
> > >
> >
> > Yes, ISP P1 HW output formats are always available without impact
> > by sensor formats.
> >
> > > > +   f->flags = 0;
> > >
> > > We need f->description too.
> > >
> >
> > For this description, do you suggest 1). we fill this field in this
> > function or 2). v4l_fill_fmtdesc function in v4l2-ioctl?
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-ioctl.c#L1152
> >
> > Basically, we prefer method 1.
> >
> 
> That should be v4l_fill_fmtdesc(), as it already includes other
> vendor-specific formats.
> 
> [snip]


Ok, got it. We will follow your suggestion.

> > > > +
> > > > +   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.
> > >
> >
> > For ISP P1 HW, we only cares these fields of v4l2_pix_format_mplane.
> > a. width
> > b. height
> > c. pixelformat
> > d. plane_fmt
> >     - sizeimage
> >     - bytesperline
> > e. num_planes
> > Other fields are consider constant.
> >
> > So if the user space passes one pixel format with un-supported, we will
> > apply the default format firstly and adjust width, height, sizeimage,
> > and bytesperline. We will focus on validate width & height.
> > Is it ok?
> 
> I'm not sure I understand your proposal, but let me describe the
> proper behavior here:
> 
> if (pixelformat is invalid)
>     pixelformat = some valid pixel format;
> 
> width = clamp(width, driver min, driver max);
> height = clamp(height, driver min, driver max);
> 
> num_planes = 1;
> 
> calculate_sizeimage_and_bytesperline(fmt);
> 
> fill_in_the_remaining_constant_fields(fmt);
> 
> Does it make sense?
> 
> [snip]

Yes, here is our new version.

static int mtk_cam_vidioc_try_fmt(struct file *file, void *fh,
				  struct v4l2_format *f)
{
	struct mtk_cam_dev *cam = video_drvdata(file);
	struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
	struct device *dev = cam->dev;
	const struct v4l2_format *dev_fmt;
	struct v4l2_format try_fmt;

	dev_dbg(dev, "%s: fmt:%c%c%c%c, w*h:%u*%u\n",
		__func__,
		(f->fmt.pix_mp.pixelformat & 0xFF),
		(f->fmt.pix_mp.pixelformat >> 8) & 0xFF,
		(f->fmt.pix_mp.pixelformat >> 16) & 0xFF,
		(f->fmt.pix_mp.pixelformat >> 24) & 0xFF,
		f->fmt.pix_mp.width, f->fmt.pix_mp.height);

	memset(&try_fmt, 0, sizeof(try_fmt));
	try_fmt.type = f->type;

	/* Validate pixelformat */
	dev_fmt = mtk_cam_dev_find_fmt(&node->desc, f->fmt.pix_mp.pixelformat);
	if (!dev_fmt) {
		dev_dbg(dev, "unknown fmt:%d\n", f->fmt.pix_mp.pixelformat);
		dev_fmt = &node->desc.fmts[node->desc.default_fmt_idx];
	}
	try_fmt.fmt.pix_mp.pixelformat = dev_fmt->fmt.pix_mp.pixelformat;

	/* Validate image width & height range */
	try_fmt.fmt.pix_mp.width = clamp_val(f->fmt.pix_mp.width,
					     IMG_MIN_WIDTH, IMG_MAX_WIDTH);
	try_fmt.fmt.pix_mp.height = clamp_val(f->fmt.pix_mp.height,
					      IMG_MIN_HEIGHT, IMG_MAX_HEIGHT);

	/* 4 bytes alignment for width */
	try_fmt.fmt.pix_mp.width = ALIGN(try_fmt.fmt.pix_mp.width, 4);

	/* bytesperline & sizeimage calculation */
	cal_image_pix_mp(cam, node->id, &try_fmt.fmt.pix_mp);

	/* Constant format fields */
	try_fmt.fmt.pix_mp.colorspace = V4L2_COLORSPACE_SRGB;
	try_fmt.fmt.pix_mp.field = V4L2_FIELD_NONE;
	try_fmt.fmt.pix_mp.num_planes = 1;
	try_fmt.fmt.pix_mp.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
	try_fmt.fmt.pix_mp.quantization = V4L2_QUANTIZATION_DEFAULT;
	try_fmt.fmt.pix_mp.xfer_func = V4L2_XFER_FUNC_SRGB;

	*f = try_fmt;

	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.
> > >
> >
> > Since vb2_queue_is_busy is static function, would we paste its
> > implementation in this function to check like this?
> >
> >         if (node->vdev.queue->owner &&
> >                 node->vdev.queue->owner != file->private_data) {
> >                 dev_err(cam_dev->dev, "%s err: buffer allocated\n", __func__);
> >                 return -EBUSY;
> >         }
> >
> 
> Sorry, I mixed up the function name. That should've been vb2_is_busy().
> 
> [snip]

Got it. Thanks for your suggestion.

> > > > +   /* 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.
> > >
> >
> > Here is the new log info.
> >
> > dev_info(dev, "media%d register",cam->media_dev.devnode->minor);
> >
> 
> Let's fix the missing space and making a bit more readable:
> 
> dev_info(dev, "Registered media%d", cam->media_dev.devnode->minor);
> 

Ok, we will apply this change.

> >
> > > > +
> > > > +   /* 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.
> > >
> >
> > Will fix in next patch.
> >
> > > > +   }
> > > > +   dev_info(dev, "Register v4l2 device: 0x%pK", cam_dev->v4l2_dev);
> > >
> > > Same as above.
> > >
> >
> > Ditto.
> >
> > dev_info(dev, "Register v4l2 device: %s", cam->v4l2_dev.name);
> >
> 
> Perhaps just "Registered %s" to be consistent with the above media log?
> 
> [snip]

Ditto.

> > > > +
> > > > +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?
> > >
> >
> > How about to check the entity function of seninf device?
> >
> > if (!(sd->entity.function & MEDIA_ENT_F_VID_IF_BRIDGE)) {
> >         dev_dbg(cam->dev, "No MEDIA_ENT_F_VID_IF_BRIDGE function\n");
> >                 return -ENODEV;
> > }
> >
> > If we need to check DT, may we need to implement this in parse_endpoint
> > callback function of v4l2_async_notifier_parse_fwnode_endpoints?
> >
> 
> Yes, checking the entity function is indeed the right way to do this.
> 
> [snip]

Thanks for your confirm.

> > > > +           .default_fmt_idx = 4,
> > > > +           .max_buf_count = 10,
> > >
> > > Where does this number come from?
> > >
> >
> > The default maximum VB2 buffer count is 32.
> > In order to limit memory usage, we like to limit the maximum buffer
> > counts in the driver layer. The maximum buffer count is estimated
> > according to our camera MW.
> >
> > #define VB2_MAX_FRAME   (32)
> >
> 
> Okay, thanks.
> 
> [snip]
> > > > +   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.
> > >
> >
> > Fixed in next patch.
> > Btw, if we remove this, we will got complain from checkpatch.pl script.
> >
> 
> Oh really, that's weird. Okay, please keep it then, sorry for the confusion.
> 
> Best regards,
> Tomasz

Ok, thanks for your understanding.
We will rollback this change to avoid checkpatch's complains.


Best regards,

Jungo