On Mon, Mar 26, 2018 at 03:02:53PM +0900, Tomasz Figa wrote:
> Hi Sakari,
>
> I would have appreciated being CCed on this series, but anyway, thanks
> for sending it. Please see my comments inline.
>
> On Sat, Mar 24, 2018 at 6:17 AM, Sakari Ailus
> <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> > Associate a buffer to a request when it is queued and disassociate when it
> > is done.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/common/videobuf2/videobuf2-core.c | 43 ++++++++++++++++++++++++-
> > drivers/media/common/videobuf2/videobuf2-v4l2.c | 40 ++++++++++++++++++++++-
> > include/media/videobuf2-core.h | 19 +++++++++++
> > include/media/videobuf2-v4l2.h | 28 ++++++++++++++++
> > 4 files changed, 128 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index d3f7bb3..b8535de 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -346,6 +346,9 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> > break;
> > }
> >
> > + if (q->class)
> > + media_request_object_init(q->class, &vb->req_obj);
> > +
> > vb->state = VB2_BUF_STATE_DEQUEUED;
> > vb->vb2_queue = q;
> > vb->num_planes = num_planes;
> > @@ -520,7 +523,10 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
> > /* Free videobuf buffers */
> > for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
> > ++buffer) {
> > - kfree(q->bufs[buffer]);
> > + if (q->class)
> > + media_request_object_put(&q->bufs[buffer]->req_obj);
> > + else
> > + kfree(q->bufs[buffer]);
> > q->bufs[buffer] = NULL;
> > }
> >
> > @@ -944,6 +950,10 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
> > default:
> > /* Inform any processes that may be waiting for buffers */
> > wake_up(&q->done_wq);
> > + if (vb->req_ref) {
> > + media_request_ref_complete(vb->req_ref);
> > + vb->req_ref = NULL;
> > + }
> > break;
> > }
> > }
> > @@ -1249,6 +1259,32 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
> > return -EIO;
> > }
> >
> > + if (vb->request_fd) {
>
> 0 is a perfectly valid FD.
Agreed. This part was written before Hans's RFC albeit sent afterwards.
>
> > + struct media_request *req;
> > + struct media_request_ref *ref;
> > +
> > + if (!q->class) {
> > + dprintk(1, "requests not enabled for the queue\n");
> > + return -EINVAL;
> > + }
> > +
> > + req = media_request_find(q->class->mdev, vb->request_fd);
> > + if (IS_ERR(req)) {
> > + dprintk(1, "no request found for fd %d (%ld)\n",
> > + vb->request_fd, PTR_ERR(req));
> > + return PTR_ERR(req);
> > + }
> > +
> > + ref = media_request_object_bind(req,
> > + &q->bufs[vb->index]->req_obj);
> > + media_request_put(req);
> > +
> > + if (IS_ERR(ref))
> > + return PTR_ERR(ref);
> > +
> > + vb->req_ref = ref;
> > + }
> > +
>
> I'm not sure how this would work. The client calling QBUF with request
> FD would end up queuing the buffer to the driver, which I'd say isn't
> an expected side effect of something that is usually done early as
> part of preparing the request.
>
> As we agreed in the UAPI RFC, the buffer should be only prepared and
> queued at request queue time and I believe Alex had it implemented
> properly in his latest RFC v4. We should reuse that patch instead,
> since we spent quite a bit of time to go through all the corner cases
> and make sure it works for the most exotic use case we could imagine.
Yes, we need more than the above patch. No disagreement there.
--
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
