Re: [RFCv11 PATCH 27/29] vim2m: support requests
On 04/17/2018 01:42 PM, Paul Kocialkowski wrote: > Hi, > > On Mon, 2018-04-09 at 16:20 +0200, Hans Verkuil wrote: >> From: Hans Verkuil >> >> Add support for requests to vim2m. > > Please find a nit below. > >> Signed-off-by: Hans Verkuil >> --- >> drivers/media/platform/vim2m.c | 25 + >> 1 file changed, 25 insertions(+) >> >> diff --git a/drivers/media/platform/vim2m.c >> b/drivers/media/platform/vim2m.c >> index 9b18b32c255d..2dcf0ea85705 100644 >> --- a/drivers/media/platform/vim2m.c >> +++ b/drivers/media/platform/vim2m.c >> @@ -387,8 +387,26 @@ static void device_run(void *priv) >> src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx); >> dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); >> >> +/* Apply request if needed */ > > This comment suggests that this is where request submission is handled. > I suggest making it clear that this the place where the *controls* > attached to the request are applied, instead. True. Fixed here and below. Thanks! Hans > >> +if (src_buf->vb2_buf.req_obj.req) >> +v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req, >> +&ctx->hdl); >> +if (dst_buf->vb2_buf.req_obj.req && >> +dst_buf->vb2_buf.req_obj.req != src_buf- >>> vb2_buf.req_obj.req) >> +v4l2_ctrl_request_setup(dst_buf->vb2_buf.req_obj.req, >> +&ctx->hdl); >> + >> device_process(ctx, src_buf, dst_buf); >> >> +/* Complete request if needed */ > > Ditto here. > >> +if (src_buf->vb2_buf.req_obj.req) >> +v4l2_ctrl_request_complete(src_buf- >>> vb2_buf.req_obj.req, >> +&ctx->hdl); >> +if (dst_buf->vb2_buf.req_obj.req && >> +dst_buf->vb2_buf.req_obj.req != src_buf- >>> vb2_buf.req_obj.req) >> +v4l2_ctrl_request_complete(dst_buf- >>> vb2_buf.req_obj.req, >> +&ctx->hdl); >> + >> /* Run a timer, which simulates a hardware irq */ >> schedule_irq(dev, ctx->transtime); >> } >> @@ -823,6 +841,8 @@ static void vim2m_stop_streaming(struct vb2_queue >> *q) >> vbuf = v4l2_m2m_dst_buf_remove(ctx- >>> fh.m2m_ctx); >> if (vbuf == NULL) >> return; >> +v4l2_ctrl_request_complete(vbuf->vb2_buf.req_obj.req, >> + &ctx->hdl); >> spin_lock_irqsave(&ctx->dev->irqlock, flags); >> v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR); >> spin_unlock_irqrestore(&ctx->dev->irqlock, flags); >> @@ -1003,6 +1023,10 @@ static const struct v4l2_m2m_ops m2m_ops = { >> .job_abort = job_abort, >> }; >> >> +static const struct media_device_ops m2m_media_ops = { >> +.req_queue = vb2_request_queue, >> +}; >> + >> static int vim2m_probe(struct platform_device *pdev) >> { >> struct vim2m_dev *dev; >> @@ -1027,6 +1051,7 @@ static int vim2m_probe(struct platform_device >> *pdev) >> dev->mdev.dev = &pdev->dev; >> strlcpy(dev->mdev.model, "vim2m", sizeof(dev->mdev.model)); >> media_device_init(&dev->mdev); >> +dev->mdev.ops = &m2m_media_ops; >> dev->v4l2_dev.mdev = &dev->mdev; >> dev->pad[0].flags = MEDIA_PAD_FL_SINK; >> dev->pad[1].flags = MEDIA_PAD_FL_SOURCE;
Re: [RFCv11 PATCH 27/29] vim2m: support requests
On 04/17/2018 06:37 AM, Alexandre Courbot wrote: > On Mon, Apr 9, 2018 at 11:20 PM Hans Verkuil wrote: > >> From: Hans Verkuil > >> Add support for requests to vim2m. > >> Signed-off-by: Hans Verkuil >> --- >> drivers/media/platform/vim2m.c | 25 + >> 1 file changed, 25 insertions(+) > >> diff --git a/drivers/media/platform/vim2m.c > b/drivers/media/platform/vim2m.c >> index 9b18b32c255d..2dcf0ea85705 100644 >> --- a/drivers/media/platform/vim2m.c >> +++ b/drivers/media/platform/vim2m.c >> @@ -387,8 +387,26 @@ static void device_run(void *priv) >> src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx); >> dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); > >> + /* Apply request if needed */ >> + if (src_buf->vb2_buf.req_obj.req) >> + v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req, >> + &ctx->hdl); >> + if (dst_buf->vb2_buf.req_obj.req && >> + dst_buf->vb2_buf.req_obj.req != src_buf->vb2_buf.req_obj.req) >> + v4l2_ctrl_request_setup(dst_buf->vb2_buf.req_obj.req, >> + &ctx->hdl); > > This implies that we can have two different requests active at the same time > for the same device. Do we want to open that can of worms? > > Valid scenarios for requests should be clearly defined. In the case of a > simple > buffer processor like vim2m, something like "request required for source > buffer, > optional and must be same as source if specified for destination", looks > simple > and sane. Is it? It is sane for this specific use-case, but for other m2m devices you might require the request for the sink buffer instead of the source buffer. Or both. So for one driver you would have one set of rules, and for another driver another set of rules. That said, this probably requires some more discussion. I went with the generic case here, and it was really very little code to make that happen. And it avoids creating ad-hoc 'rules'. Regards, Hans > >> + >> device_process(ctx, src_buf, dst_buf); > >> + /* Complete request if needed */ >> + if (src_buf->vb2_buf.req_obj.req) >> + v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req, >> + &ctx->hdl); >> + if (dst_buf->vb2_buf.req_obj.req && >> + dst_buf->vb2_buf.req_obj.req != src_buf->vb2_buf.req_obj.req) >> + v4l2_ctrl_request_complete(dst_buf->vb2_buf.req_obj.req, >> + &ctx->hdl); >> + >> /* Run a timer, which simulates a hardware irq */ >> schedule_irq(dev, ctx->transtime); >> } >> @@ -823,6 +841,8 @@ static void vim2m_stop_streaming(struct vb2_queue *q) >> vbuf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx); >> if (vbuf == NULL) >> return; >> + v4l2_ctrl_request_complete(vbuf->vb2_buf.req_obj.req, >> + &ctx->hdl); >> spin_lock_irqsave(&ctx->dev->irqlock, flags); >> v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR); >> spin_unlock_irqrestore(&ctx->dev->irqlock, flags); >> @@ -1003,6 +1023,10 @@ static const struct v4l2_m2m_ops m2m_ops = { >> .job_abort = job_abort, >> }; > >> +static const struct media_device_ops m2m_media_ops = { >> + .req_queue = vb2_request_queue, >> +}; >> + >> static int vim2m_probe(struct platform_device *pdev) >> { >> struct vim2m_dev *dev; >> @@ -1027,6 +1051,7 @@ static int vim2m_probe(struct platform_device *pdev) >> dev->mdev.dev = &pdev->dev; >> strlcpy(dev->mdev.model, "vim2m", sizeof(dev->mdev.model)); >> media_device_init(&dev->mdev); >> + dev->mdev.ops = &m2m_media_ops; >> dev->v4l2_dev.mdev = &dev->mdev; >> dev->pad[0].flags = MEDIA_PAD_FL_SINK; >> dev->pad[1].flags = MEDIA_PAD_FL_SOURCE; >> -- >> 2.16.3
Re: [RFCv11 PATCH 27/29] vim2m: support requests
On 04/12/2018 11:15 AM, Tomasz Figa wrote: > Hi Hans, > > On Mon, Apr 9, 2018 at 11:20 PM Hans Verkuil wrote: > >> From: Hans Verkuil > >> Add support for requests to vim2m. > >> Signed-off-by: Hans Verkuil >> --- >> drivers/media/platform/vim2m.c | 25 + >> 1 file changed, 25 insertions(+) > >> diff --git a/drivers/media/platform/vim2m.c > b/drivers/media/platform/vim2m.c >> index 9b18b32c255d..2dcf0ea85705 100644 >> --- a/drivers/media/platform/vim2m.c >> +++ b/drivers/media/platform/vim2m.c >> @@ -387,8 +387,26 @@ static void device_run(void *priv) >> src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx); >> dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); > >> + /* Apply request if needed */ >> + if (src_buf->vb2_buf.req_obj.req) >> + v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req, >> + &ctx->hdl); >> + if (dst_buf->vb2_buf.req_obj.req && >> + dst_buf->vb2_buf.req_obj.req != src_buf->vb2_buf.req_obj.req) >> + v4l2_ctrl_request_setup(dst_buf->vb2_buf.req_obj.req, >> + &ctx->hdl); > > I'm not sure I understand what's going on here. How is it possible that we > have 2 different requests? You can have one request with buffers for both queues, or two requests, one for each queue. Or a request for just one of the queues and the other without any requests. So you can have 0, 1 or 2 requests associated with these two queues. But you don't want to call v4l2_ctrl_request_setup twice if the same request is associated with both queues. (Well, you can call it twice and the second call would not do anything, but that's a waste of CPU cycles) Regards, Hans
Re: [RFCv11 PATCH 27/29] vim2m: support requests
Hi, On Mon, 2018-04-09 at 16:20 +0200, Hans Verkuil wrote: > From: Hans Verkuil > > Add support for requests to vim2m. Please find a nit below. > Signed-off-by: Hans Verkuil > --- > drivers/media/platform/vim2m.c | 25 + > 1 file changed, 25 insertions(+) > > diff --git a/drivers/media/platform/vim2m.c > b/drivers/media/platform/vim2m.c > index 9b18b32c255d..2dcf0ea85705 100644 > --- a/drivers/media/platform/vim2m.c > +++ b/drivers/media/platform/vim2m.c > @@ -387,8 +387,26 @@ static void device_run(void *priv) > src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx); > dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); > > + /* Apply request if needed */ This comment suggests that this is where request submission is handled. I suggest making it clear that this the place where the *controls* attached to the request are applied, instead. > + if (src_buf->vb2_buf.req_obj.req) > + v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req, > + &ctx->hdl); > + if (dst_buf->vb2_buf.req_obj.req && > + dst_buf->vb2_buf.req_obj.req != src_buf- > >vb2_buf.req_obj.req) > + v4l2_ctrl_request_setup(dst_buf->vb2_buf.req_obj.req, > + &ctx->hdl); > + > device_process(ctx, src_buf, dst_buf); > > + /* Complete request if needed */ Ditto here. > + if (src_buf->vb2_buf.req_obj.req) > + v4l2_ctrl_request_complete(src_buf- > >vb2_buf.req_obj.req, > + &ctx->hdl); > + if (dst_buf->vb2_buf.req_obj.req && > + dst_buf->vb2_buf.req_obj.req != src_buf- > >vb2_buf.req_obj.req) > + v4l2_ctrl_request_complete(dst_buf- > >vb2_buf.req_obj.req, > + &ctx->hdl); > + > /* Run a timer, which simulates a hardware irq */ > schedule_irq(dev, ctx->transtime); > } > @@ -823,6 +841,8 @@ static void vim2m_stop_streaming(struct vb2_queue > *q) > vbuf = v4l2_m2m_dst_buf_remove(ctx- > >fh.m2m_ctx); > if (vbuf == NULL) > return; > + v4l2_ctrl_request_complete(vbuf->vb2_buf.req_obj.req, > +&ctx->hdl); > spin_lock_irqsave(&ctx->dev->irqlock, flags); > v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR); > spin_unlock_irqrestore(&ctx->dev->irqlock, flags); > @@ -1003,6 +1023,10 @@ static const struct v4l2_m2m_ops m2m_ops = { > .job_abort = job_abort, > }; > > +static const struct media_device_ops m2m_media_ops = { > + .req_queue = vb2_request_queue, > +}; > + > static int vim2m_probe(struct platform_device *pdev) > { > struct vim2m_dev *dev; > @@ -1027,6 +1051,7 @@ static int vim2m_probe(struct platform_device > *pdev) > dev->mdev.dev = &pdev->dev; > strlcpy(dev->mdev.model, "vim2m", sizeof(dev->mdev.model)); > media_device_init(&dev->mdev); > + dev->mdev.ops = &m2m_media_ops; > dev->v4l2_dev.mdev = &dev->mdev; > dev->pad[0].flags = MEDIA_PAD_FL_SINK; > dev->pad[1].flags = MEDIA_PAD_FL_SOURCE; -- Paul Kocialkowski, Bootlin (formerly Free Electrons) Embedded Linux and kernel engineering https://bootlin.com signature.asc Description: This is a digitally signed message part
Re: [RFCv11 PATCH 27/29] vim2m: support requests
On Mon, Apr 9, 2018 at 11:20 PM Hans Verkuil wrote: > From: Hans Verkuil > Add support for requests to vim2m. > Signed-off-by: Hans Verkuil > --- > drivers/media/platform/vim2m.c | 25 + > 1 file changed, 25 insertions(+) > diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c > index 9b18b32c255d..2dcf0ea85705 100644 > --- a/drivers/media/platform/vim2m.c > +++ b/drivers/media/platform/vim2m.c > @@ -387,8 +387,26 @@ static void device_run(void *priv) > src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx); > dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); > + /* Apply request if needed */ > + if (src_buf->vb2_buf.req_obj.req) > + v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req, > + &ctx->hdl); > + if (dst_buf->vb2_buf.req_obj.req && > + dst_buf->vb2_buf.req_obj.req != src_buf->vb2_buf.req_obj.req) > + v4l2_ctrl_request_setup(dst_buf->vb2_buf.req_obj.req, > + &ctx->hdl); This implies that we can have two different requests active at the same time for the same device. Do we want to open that can of worms? Valid scenarios for requests should be clearly defined. In the case of a simple buffer processor like vim2m, something like "request required for source buffer, optional and must be same as source if specified for destination", looks simple and sane. > + > device_process(ctx, src_buf, dst_buf); > + /* Complete request if needed */ > + if (src_buf->vb2_buf.req_obj.req) > + v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req, > + &ctx->hdl); > + if (dst_buf->vb2_buf.req_obj.req && > + dst_buf->vb2_buf.req_obj.req != src_buf->vb2_buf.req_obj.req) > + v4l2_ctrl_request_complete(dst_buf->vb2_buf.req_obj.req, > + &ctx->hdl); > + > /* Run a timer, which simulates a hardware irq */ > schedule_irq(dev, ctx->transtime); > } > @@ -823,6 +841,8 @@ static void vim2m_stop_streaming(struct vb2_queue *q) > vbuf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx); > if (vbuf == NULL) > return; > + v4l2_ctrl_request_complete(vbuf->vb2_buf.req_obj.req, > + &ctx->hdl); > spin_lock_irqsave(&ctx->dev->irqlock, flags); > v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR); > spin_unlock_irqrestore(&ctx->dev->irqlock, flags); > @@ -1003,6 +1023,10 @@ static const struct v4l2_m2m_ops m2m_ops = { > .job_abort = job_abort, > }; > +static const struct media_device_ops m2m_media_ops = { > + .req_queue = vb2_request_queue, > +}; > + > static int vim2m_probe(struct platform_device *pdev) > { > struct vim2m_dev *dev; > @@ -1027,6 +1051,7 @@ static int vim2m_probe(struct platform_device *pdev) > dev->mdev.dev = &pdev->dev; > strlcpy(dev->mdev.model, "vim2m", sizeof(dev->mdev.model)); > media_device_init(&dev->mdev); > + dev->mdev.ops = &m2m_media_ops; > dev->v4l2_dev.mdev = &dev->mdev; > dev->pad[0].flags = MEDIA_PAD_FL_SINK; > dev->pad[1].flags = MEDIA_PAD_FL_SOURCE; > -- > 2.16.3
Re: [RFCv11 PATCH 27/29] vim2m: support requests
Hi Hans, On Mon, Apr 9, 2018 at 11:20 PM Hans Verkuil wrote: > From: Hans Verkuil > Add support for requests to vim2m. > Signed-off-by: Hans Verkuil > --- > drivers/media/platform/vim2m.c | 25 + > 1 file changed, 25 insertions(+) > diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c > index 9b18b32c255d..2dcf0ea85705 100644 > --- a/drivers/media/platform/vim2m.c > +++ b/drivers/media/platform/vim2m.c > @@ -387,8 +387,26 @@ static void device_run(void *priv) > src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx); > dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); > + /* Apply request if needed */ > + if (src_buf->vb2_buf.req_obj.req) > + v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req, > + &ctx->hdl); > + if (dst_buf->vb2_buf.req_obj.req && > + dst_buf->vb2_buf.req_obj.req != src_buf->vb2_buf.req_obj.req) > + v4l2_ctrl_request_setup(dst_buf->vb2_buf.req_obj.req, > + &ctx->hdl); I'm not sure I understand what's going on here. How is it possible that we have 2 different requests? Best regards, Tomasz
Re: [RFCv11 PATCH 27/29] vim2m: support requests
Hi, On Mon, 2018-04-09 at 16:20 +0200, Hans Verkuil wrote: > From: Hans Verkuil > > Add support for requests to vim2m. Depending on where STREAMON happens in the sequence, there is a possibility that v4l2_m2m_try_schedule is never called and thus that device_run never runs when submitting the request. More specifically, the m2m fashion of the STREAMON ioctl handler will normally call v4l2_m2m_try_schedule. Hence, there is no issue if it's called after submitting the request. On the other hand, if the request was not submitted when calling STREAMON (which is a valid use case), buffers are not available and v4l2_m2m_try_schedule won't schedule anything. Once the request is submitted, the internal plumbing will detect that STREAMON was requested but did not take effect, so it will call vb2_streamon. And of course, vb2_streamon has no provision for trying to schedule a m2m device run. One way to fix this is to call v4l2_m2m_try_schedule from a workqueue triggered in the driver's start_streaming qop. The same is also probably necessary in buf_queue and buf_prepare since those qops are not called from the m2m ioctl handler either. What do you think? > Signed-off-by: Hans Verkuil > --- > drivers/media/platform/vim2m.c | 25 + > 1 file changed, 25 insertions(+) > > diff --git a/drivers/media/platform/vim2m.c > b/drivers/media/platform/vim2m.c > index 9b18b32c255d..2dcf0ea85705 100644 > --- a/drivers/media/platform/vim2m.c > +++ b/drivers/media/platform/vim2m.c > @@ -387,8 +387,26 @@ static void device_run(void *priv) > src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx); > dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); > > + /* Apply request if needed */ > + if (src_buf->vb2_buf.req_obj.req) > + v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req, > + &ctx->hdl); > + if (dst_buf->vb2_buf.req_obj.req && > + dst_buf->vb2_buf.req_obj.req != src_buf- > >vb2_buf.req_obj.req) > + v4l2_ctrl_request_setup(dst_buf->vb2_buf.req_obj.req, > + &ctx->hdl); > + > device_process(ctx, src_buf, dst_buf); > > + /* Complete request if needed */ > + if (src_buf->vb2_buf.req_obj.req) > + v4l2_ctrl_request_complete(src_buf- > >vb2_buf.req_obj.req, > + &ctx->hdl); > + if (dst_buf->vb2_buf.req_obj.req && > + dst_buf->vb2_buf.req_obj.req != src_buf- > >vb2_buf.req_obj.req) > + v4l2_ctrl_request_complete(dst_buf- > >vb2_buf.req_obj.req, > + &ctx->hdl); > + > /* Run a timer, which simulates a hardware irq */ > schedule_irq(dev, ctx->transtime); > } > @@ -823,6 +841,8 @@ static void vim2m_stop_streaming(struct vb2_queue > *q) > vbuf = v4l2_m2m_dst_buf_remove(ctx- > >fh.m2m_ctx); > if (vbuf == NULL) > return; > + v4l2_ctrl_request_complete(vbuf->vb2_buf.req_obj.req, > +&ctx->hdl); > spin_lock_irqsave(&ctx->dev->irqlock, flags); > v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR); > spin_unlock_irqrestore(&ctx->dev->irqlock, flags); > @@ -1003,6 +1023,10 @@ static const struct v4l2_m2m_ops m2m_ops = { > .job_abort = job_abort, > }; > > +static const struct media_device_ops m2m_media_ops = { > + .req_queue = vb2_request_queue, > +}; > + > static int vim2m_probe(struct platform_device *pdev) > { > struct vim2m_dev *dev; > @@ -1027,6 +1051,7 @@ static int vim2m_probe(struct platform_device > *pdev) > dev->mdev.dev = &pdev->dev; > strlcpy(dev->mdev.model, "vim2m", sizeof(dev->mdev.model)); > media_device_init(&dev->mdev); > + dev->mdev.ops = &m2m_media_ops; > dev->v4l2_dev.mdev = &dev->mdev; > dev->pad[0].flags = MEDIA_PAD_FL_SINK; > dev->pad[1].flags = MEDIA_PAD_FL_SOURCE; -- Paul Kocialkowski, Bootlin (formerly Free Electrons) Embedded Linux and kernel engineering https://bootlin.com signature.asc Description: This is a digitally signed message part