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

[Jacopo Mondi]

Hi Laurent,

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.

> > > +}

[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) ?

> >
> > 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.

Thanks
   j
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200102/f63eac99/attachment.sig>