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

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

Hi, Tomasz:

On Fri, 2019-07-26 at 14:49 +0900, Tomasz Figa wrote:
> On Wed, Jul 24, 2019 at 1:31 PM Jungo Lin <jungo.lin@mediatek.com> wrote:
> >
> > 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.
> >
> 
> Adding to a list is changing driver state. The callback must not
> modify anything else than the request itself.
> 
> Queuing to driver's list should happen in req_queue instead.
> 
> [snip]

Ok, got your point. We will move these implementation to .req_queue.

static const struct media_device_ops 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 void mtk_cam_req_queue(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);

	atomic_set(&cam_req->buf_count, vb2_request_buffer_cnt(req));

	/* add to pending job list */
	spin_lock_irq(&cam->pending_job_lock);
	list_add_tail(&cam_req->list, &cam->pending_job_list);
	spin_unlock_irq(&cam->pending_job_lock);

	vb2_request_queue(req);
}

> > > >
> > > > 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) {
> 
> If we use a spin_lock to protect the counter, perhaps we don't need
> the atomic type anymore?
> 

Ok, we will remove atomic type usage.


> >                 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) {
> 
> Could we instead check the counter here and break if it's >=
> MTK_ISP_MAX_RUNNING_JOBS?
> Then we could increment it here too to simplify the code.
> 

Thanks for your advice.
We simplified this function as below:

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_irq(&cam->pending_job_lock);
	spin_lock_irqsave(&cam->running_job_lock, flags);
	list_for_each_entry_safe(req, req_prev, &cam->pending_job_list, list) {
		if (cam->running_job_count >= MTK_ISP_MAX_RUNNING_JOBS) {
			dev_dbg(cam->dev, "jobs are full\n");
			break;
		}
		cam->running_job_count++;
		list_del(&req->list);
		list_add_tail(&req->list, &cam->running_job_list);
		mtk_isp_req_enqueue(cam, req);
	}
	spin_unlock_irqrestore(&cam->running_job_lock, flags);
	spin_unlock_irq(&cam->pending_job_lock);
}

> >                 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;
> 
> With the above suggestion, this if block would go away.
> 
> [snip]

Ditto.

> > > >                 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
> >
> 
> Got it, thanks!
> 
> [snip]
> > > > > > +
> > > > > > +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.
> >
> 
> Do you mean that it was the Clang toolchain used on Chromium OS (e.g.
> emerge chromeos-kernel-4_19)?

> [snip]

Yes, I checked this comment in the Chromium OS build environment.
But, I think I have made the mistake here. I need to check the build
status in the Mediatek's kernel upstream environment. I will pay
attention in next path set upstream.

> > > > > > +
> > > > > > +   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.
> >
> 
> Note that Chromium OS will use the "upstream driver" and we don't want
> to diverge, so please make the userspace also use only requests. I
> don't see a reason why there would be any need to submit any buffers
> outside of a request.
> 
> [snip]

Ok, I have raised your concern to our colleagues and let him to discuss
with you in another communication channel. 

> > > > > > +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?
> 
> Are you sure that this callback can be only called after
> stop_streaming? Also wouldn't that be after stop_streaming only on 1
> queue? The other queues could still remain streaming, but we still
> have to return corresponding buffers I believe.
> 
> Hans, could you clarify what exactly this callback is supposed to do?
> 

Ok, we will look forward Hans' comments on this.

> >
> > 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);
> > }
> 
> [keeping the context for Hans]
> 
> Best regards,
> Tomasz
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek


Best regards,


Jungo