[libcamera-devel] [PATCH v4 4/4] v4l2: v4l2_compat: Add V4L2 compatibility layer

[Laurent Pinchart]

Hi Jacopo,

On Thu, Jan 02, 2020 at 09:20:34AM +0100, Jacopo Mondi wrote:
> On Thu, Jan 02, 2020 at 05:02:14AM +0200, Laurent Pinchart wrote:
> > Hi Paul,
> >
> > Thank you for the patch. Nearly there, there remaining comments are
> > small.
> 
> [snip]
> 
> >>> +void V4L2Camera::requestComplete(Request *request)
> >>> +{
> >>> +	if (request->status() == Request::RequestCancelled)
> >>> +		return;
> >>> +
> >>> +	/* We only have one stream at the moment. */
> >>> +	bufferLock_.lock();
> >>> +	Buffer *buffer = request->buffers().begin()->second;
> >>> +	std::unique_ptr<FrameMetadata> metadata =
> >>> +		utils::make_unique<FrameMetadata>(buffer);
> >>> +	completedBuffers_.push_back(std::move(metadata));
> >>> +	bufferLock_.unlock();
> >>> +
> >>> +	bufferSema_.release();
> >>
> >> The usage of this semaphore from two different classes looks weird :/
> >
> > We could encapsulate it, but given that V4L2Camera and V4L2CameraProxy
> > are internal to the V4L2 compatibility layer, I think it's overkill.
> 
> Encapsulate would have been syntactic sugar. I would have had the
> V4L2Camera handle locking internally, but that's fine.

The whole point of a semaphore is to handle consumers and producers
living in different threads, in this case in different objects. We could
encapsulate it in a V4L2Camera method (something like
V4L2Camera::waitForBuffer() for instance), that may be oevrkill.

> >>> +}
> 
> [snip]
> 
> >>> +
> >>> +int V4L2CameraProxy::vidioc_querybuf(struct v4l2_buffer *arg)
> >>> +{
> >>> +	LOG(V4L2Compat, Debug) << "Servicing vidioc_querybuf";
> >>> +	Stream *stream = streamConfig_.stream();
> >>> +
> >>> +	if (!validateStreamType(arg->type) ||
> >>> +	    arg->index >= stream->buffers().size())
> >>> +		return -EINVAL;
> >>> +
> >>> +	updateBuffers();
> >>
> >> updateBuffers() retreives information on completed buffers, while
> >> querybuf can be called anytime after buffers have been requested.
> 
> You below answer does not address this issue. What if querybuf is
> called and not buffer completed yet (because say, streaming has not
> started) ?

That's not a problem. buffers_ contains the most up-to-date information,
and that's what querybuf will return.

> >> From V4L2 specs:
> >> This ioctl is part of the streaming I/O method. It can be used to
> >> query the status of a buffer at any time after buffers have been
> >> allocated with the ioctl VIDIOC_REQBUFS ioctl.
> >>
> >> It should thus depend on static information of allocated buffers not
> >> just for completed ones...
> >>
> >> I suspect I am missing something, as this updateBuffers() thing
> >> looks weird to me both here and in dqbuf.
> >
> > The idea is that the buffer status is kept in V4L2CameraProxy, and
> > updated with updateBuffers(). The method is called both at querybuf and
> > dqbuf time to ensure we have the latest status.
> >
> >>> +
> >>> +	memcpy(arg, &buffers_[arg->index], sizeof(*arg));
> >>> +
> >>> +	struct v4l2_buffer &buf = buffers_[arg->index];
> >>
> >> I'm not sure  this is right, index should be set by us, not received
> >> from the application, there is no guarantee it is valid at all, am I wrong ?
> >
> > That's correct. This should be
> >
> > 	struct v4l2_buffer &buf = buffers_[currentBuf_];
> >
> >> From V4L2 specs:
> >> Applications call the VIDIOC_DQBUF ioctl to dequeue a filled
> >> (capturing) or displayed (output) buffer from the driver’s outgoing
> >> queue. They just set the type, memory and reserved fields of a struct
> >> v4l2_buffer as above, when VIDIOC_DQBUF is called with a pointer to
> >> this structure the driver fills the remaining fields or returns an
> >> error code.
> >>
> >> I'm not a huge fan of this whole updateBuffers() mechanism, I agree
> >> the V4L2Camera should keep a list of completed buffers, but why
> >> would we want to copy here information for all of them when we dequeue
> >> one only ?
> >
> > For performance reasons, and because we need to do so for querybuf too
> > anyway.
> >
> >> I would simply keep a list of completed buffers in V4L2Camera, and get
> >> the older on when dequeuing a buffer here.
> >>
> 
> If the two of you prefer this I'm fine. I still think it's unnecessary
> clunky and we could have lived with V4L2CameraProxy picking the latest
> completed buffer here at the expense of one inter-thread call per
> request. We should probably profile this to decide what is more
> efficient between an inter-thread call and a copy of a vector of
> buffers.

I'll bet on the copy of the vector :-)

-- 
Regards,

Laurent Pinchart