Re: [RFCv4,13/21] media: videobuf2-v4l2: support for requests

[Alexandre Courbot]

On Thu, Mar 8, 2018 at 1:50 AM, Paul Kocialkowski
 wrote:
> Hi,
>
> On Tue, 2018-02-20 at 13:44 +0900, Alexandre Courbot wrote:
>> Add a new vb2_qbuf_request() (a request-aware version of vb2_qbuf())
>> that request-aware drivers can call to queue a buffer into a request
>> instead of directly into the vb2 queue if relevent.
>>
>> This function expects that drivers invoking it are using instances of
>> v4l2_request_entity and v4l2_request_entity_data to describe their
>> entity and entity data respectively.
>>
>> Also add the vb2_request_submit() helper function which drivers can
>> invoke in order to queue all the buffers previously queued into a
>> request into the regular vb2 queue.
>
> See a comment/proposal below about an issue I encountered when using
> multi-planar formats.
>
>> Signed-off-by: Alexandre Courbot 
>> ---
>>  .../media/common/videobuf2/videobuf2-v4l2.c   | 129
>> +-
>>  include/media/videobuf2-v4l2.h|  59 
>>  2 files changed, 187 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 6d4d184aa68e..0627c3339572 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>
> [...]
>
>> +#if IS_ENABLED(CONFIG_MEDIA_REQUEST_API)
>> +int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer *b,
>> +  struct media_request_entity *entity)
>> +{
>> + struct v4l2_request_entity_data *data;
>> + struct v4l2_vb2_request_buffer *qb;
>> + struct media_request *req;
>> + struct vb2_buffer *vb;
>> + int ret = 0;
>> +
>> + if (b->request_fd <= 0)
>> + return vb2_qbuf(q, b);
>> +
>> + if (!q->allow_requests)
>> + return -EINVAL;
>> +
>> + req = media_request_get_from_fd(b->request_fd);
>> + if (!req)
>> + return -EINVAL;
>> +
>> + data = to_v4l2_entity_data(media_request_get_entity_data(req,
>> entity));
>> + if (IS_ERR(data)) {
>> + ret = PTR_ERR(data);
>> + goto out;
>> + }
>> +
>> + mutex_lock(>lock);
>> +
>> + if (req->state != MEDIA_REQUEST_STATE_IDLE) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
>> + if (ret)
>> + goto out;
>> +
>> + vb = q->bufs[b->index];
>> + switch (vb->state) {
>> + case VB2_BUF_STATE_DEQUEUED:
>> + break;
>> + case VB2_BUF_STATE_PREPARED:
>> + break;
>> + case VB2_BUF_STATE_PREPARING:
>> + dprintk(1, "buffer still being prepared\n");
>> + ret = -EINVAL;
>> + goto out;
>> + default:
>> + dprintk(1, "invalid buffer state %d\n", vb->state);
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + /* do we already have a buffer for this request in the queue?
>> */
>> + list_for_each_entry(qb, >queued_buffers, node) {
>> + if (qb->queue == q) {
>> + ret = -EBUSY;
>> + goto out;
>> + }
>> + }
>> +
>> + qb = kzalloc(sizeof(*qb), GFP_KERNEL);
>> + if (!qb) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + /*
>> +  * TODO should be prepare the buffer here if needed, to
>> report errors
>> +  * early?
>> +  */
>> + qb->pre_req_state = vb->state;
>> + qb->queue = q;
>> + memcpy(>v4l2_buf, b, sizeof(*b));
>
> I am getting data corruption on qb->v4l2_buf.m.planes from this when
> using planar buffers, only after exiting the ioctl handler (i.e. when
> accessing this buffer later from the queue).
>
> I initially thought this was because the planes pointer was copied as-is
> from userspace, but Maxime Ripard suggested that this would have
> automatically triggered a visible fault in the kernel.
>
> Thus, my best guess is that the data is properly copied from userspace
> but freed when leaving the ioctl handler, which doesn't work our with
> the request API.
>
> A dirty fix that I came up with consists in re-allocating the planes
> buffer here and copying its contents from "b", so that it can live
> beyond the ioctl call.
>
> I am not too sure whether this should be fixed here or in the part of
> the v4l2 common code that frees this data. What do you think?

Oh, nice catch. Copying plane information indeed requires more work
than a dumb memcpy(). I suppose this should be handled here, but let
me come back to this after a good night of sleep. :)

Thanks! I will try to fix this tomorrow and push a temporary version
somewhere so you can move forward.

Alex.

Re: [RFCv4,13/21] media: videobuf2-v4l2: support for requests

[Alexandre Courbot]

On Thu, Mar 8, 2018 at 1:50 AM, Paul Kocialkowski
 wrote:
> Hi,
>
> On Tue, 2018-02-20 at 13:44 +0900, Alexandre Courbot wrote:
>> Add a new vb2_qbuf_request() (a request-aware version of vb2_qbuf())
>> that request-aware drivers can call to queue a buffer into a request
>> instead of directly into the vb2 queue if relevent.
>>
>> This function expects that drivers invoking it are using instances of
>> v4l2_request_entity and v4l2_request_entity_data to describe their
>> entity and entity data respectively.
>>
>> Also add the vb2_request_submit() helper function which drivers can
>> invoke in order to queue all the buffers previously queued into a
>> request into the regular vb2 queue.
>
> See a comment/proposal below about an issue I encountered when using
> multi-planar formats.
>
>> Signed-off-by: Alexandre Courbot 
>> ---
>>  .../media/common/videobuf2/videobuf2-v4l2.c   | 129
>> +-
>>  include/media/videobuf2-v4l2.h|  59 
>>  2 files changed, 187 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 6d4d184aa68e..0627c3339572 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>
> [...]
>
>> +#if IS_ENABLED(CONFIG_MEDIA_REQUEST_API)
>> +int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer *b,
>> +  struct media_request_entity *entity)
>> +{
>> + struct v4l2_request_entity_data *data;
>> + struct v4l2_vb2_request_buffer *qb;
>> + struct media_request *req;
>> + struct vb2_buffer *vb;
>> + int ret = 0;
>> +
>> + if (b->request_fd <= 0)
>> + return vb2_qbuf(q, b);
>> +
>> + if (!q->allow_requests)
>> + return -EINVAL;
>> +
>> + req = media_request_get_from_fd(b->request_fd);
>> + if (!req)
>> + return -EINVAL;
>> +
>> + data = to_v4l2_entity_data(media_request_get_entity_data(req,
>> entity));
>> + if (IS_ERR(data)) {
>> + ret = PTR_ERR(data);
>> + goto out;
>> + }
>> +
>> + mutex_lock(>lock);
>> +
>> + if (req->state != MEDIA_REQUEST_STATE_IDLE) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
>> + if (ret)
>> + goto out;
>> +
>> + vb = q->bufs[b->index];
>> + switch (vb->state) {
>> + case VB2_BUF_STATE_DEQUEUED:
>> + break;
>> + case VB2_BUF_STATE_PREPARED:
>> + break;
>> + case VB2_BUF_STATE_PREPARING:
>> + dprintk(1, "buffer still being prepared\n");
>> + ret = -EINVAL;
>> + goto out;
>> + default:
>> + dprintk(1, "invalid buffer state %d\n", vb->state);
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + /* do we already have a buffer for this request in the queue?
>> */
>> + list_for_each_entry(qb, >queued_buffers, node) {
>> + if (qb->queue == q) {
>> + ret = -EBUSY;
>> + goto out;
>> + }
>> + }
>> +
>> + qb = kzalloc(sizeof(*qb), GFP_KERNEL);
>> + if (!qb) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + /*
>> +  * TODO should be prepare the buffer here if needed, to
>> report errors
>> +  * early?
>> +  */
>> + qb->pre_req_state = vb->state;
>> + qb->queue = q;
>> + memcpy(>v4l2_buf, b, sizeof(*b));
>
> I am getting data corruption on qb->v4l2_buf.m.planes from this when
> using planar buffers, only after exiting the ioctl handler (i.e. when
> accessing this buffer later from the queue).
>
> I initially thought this was because the planes pointer was copied as-is
> from userspace, but Maxime Ripard suggested that this would have
> automatically triggered a visible fault in the kernel.
>
> Thus, my best guess is that the data is properly copied from userspace
> but freed when leaving the ioctl handler, which doesn't work our with
> the request API.
>
> A dirty fix that I came up with consists in re-allocating the planes
> buffer here and copying its contents from "b", so that it can live
> beyond the ioctl call.
>
> I am not too sure whether this should be fixed here or in the part of
> the v4l2 common code that frees this data. What do you think?

Oh, nice catch. Copying plane information indeed requires more work
than a dumb memcpy(). I suppose this should be handled here, but let
me come back to this after a good night of sleep. :)

Thanks! I will try to fix this tomorrow and push a temporary version
somewhere so you can move forward.

Alex.

Re: [RFCv4,13/21] media: videobuf2-v4l2: support for requests

[Paul Kocialkowski]

Hi,

On Tue, 2018-02-20 at 13:44 +0900, Alexandre Courbot wrote:
> Add a new vb2_qbuf_request() (a request-aware version of vb2_qbuf())
> that request-aware drivers can call to queue a buffer into a request
> instead of directly into the vb2 queue if relevent.
> 
> This function expects that drivers invoking it are using instances of
> v4l2_request_entity and v4l2_request_entity_data to describe their
> entity and entity data respectively.
> 
> Also add the vb2_request_submit() helper function which drivers can
> invoke in order to queue all the buffers previously queued into a
> request into the regular vb2 queue.

See a comment/proposal below about an issue I encountered when using
multi-planar formats.

> Signed-off-by: Alexandre Courbot 
> ---
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 129
> +-
>  include/media/videobuf2-v4l2.h|  59 
>  2 files changed, 187 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 6d4d184aa68e..0627c3339572 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c

[...]

> +#if IS_ENABLED(CONFIG_MEDIA_REQUEST_API)
> +int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer *b,
> +  struct media_request_entity *entity)
> +{
> + struct v4l2_request_entity_data *data;
> + struct v4l2_vb2_request_buffer *qb;
> + struct media_request *req;
> + struct vb2_buffer *vb;
> + int ret = 0;
> +
> + if (b->request_fd <= 0)
> + return vb2_qbuf(q, b);
> +
> + if (!q->allow_requests)
> + return -EINVAL;
> +
> + req = media_request_get_from_fd(b->request_fd);
> + if (!req)
> + return -EINVAL;
> +
> + data = to_v4l2_entity_data(media_request_get_entity_data(req,
> entity));
> + if (IS_ERR(data)) {
> + ret = PTR_ERR(data);
> + goto out;
> + }
> +
> + mutex_lock(>lock);
> +
> + if (req->state != MEDIA_REQUEST_STATE_IDLE) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> + if (ret)
> + goto out;
> +
> + vb = q->bufs[b->index];
> + switch (vb->state) {
> + case VB2_BUF_STATE_DEQUEUED:
> + break;
> + case VB2_BUF_STATE_PREPARED:
> + break;
> + case VB2_BUF_STATE_PREPARING:
> + dprintk(1, "buffer still being prepared\n");
> + ret = -EINVAL;
> + goto out;
> + default:
> + dprintk(1, "invalid buffer state %d\n", vb->state);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + /* do we already have a buffer for this request in the queue?
> */
> + list_for_each_entry(qb, >queued_buffers, node) {
> + if (qb->queue == q) {
> + ret = -EBUSY;
> + goto out;
> + }
> + }
> +
> + qb = kzalloc(sizeof(*qb), GFP_KERNEL);
> + if (!qb) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + /*
> +  * TODO should be prepare the buffer here if needed, to
> report errors
> +  * early?
> +  */
> + qb->pre_req_state = vb->state;
> + qb->queue = q;
> + memcpy(>v4l2_buf, b, sizeof(*b));

I am getting data corruption on qb->v4l2_buf.m.planes from this when
using planar buffers, only after exiting the ioctl handler (i.e. when
accessing this buffer later from the queue).

I initially thought this was because the planes pointer was copied as-is 
from userspace, but Maxime Ripard suggested that this would have
automatically triggered a visible fault in the kernel.

Thus, my best guess is that the data is properly copied from userspace
but freed when leaving the ioctl handler, which doesn't work our with
the request API.

A dirty fix that I came up with consists in re-allocating the planes
buffer here and copying its contents from "b", so that it can live
beyond the ioctl call.

I am not too sure whether this should be fixed here or in the part of
the v4l2 common code that frees this data. What do you think?

Cheers!

-- 
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: [RFCv4,13/21] media: videobuf2-v4l2: support for requests

[Paul Kocialkowski]

Hi,

On Tue, 2018-02-20 at 13:44 +0900, Alexandre Courbot wrote:
> Add a new vb2_qbuf_request() (a request-aware version of vb2_qbuf())
> that request-aware drivers can call to queue a buffer into a request
> instead of directly into the vb2 queue if relevent.
> 
> This function expects that drivers invoking it are using instances of
> v4l2_request_entity and v4l2_request_entity_data to describe their
> entity and entity data respectively.
> 
> Also add the vb2_request_submit() helper function which drivers can
> invoke in order to queue all the buffers previously queued into a
> request into the regular vb2 queue.

See a comment/proposal below about an issue I encountered when using
multi-planar formats.

> Signed-off-by: Alexandre Courbot 
> ---
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 129
> +-
>  include/media/videobuf2-v4l2.h|  59 
>  2 files changed, 187 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 6d4d184aa68e..0627c3339572 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c

[...]

> +#if IS_ENABLED(CONFIG_MEDIA_REQUEST_API)
> +int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer *b,
> +  struct media_request_entity *entity)
> +{
> + struct v4l2_request_entity_data *data;
> + struct v4l2_vb2_request_buffer *qb;
> + struct media_request *req;
> + struct vb2_buffer *vb;
> + int ret = 0;
> +
> + if (b->request_fd <= 0)
> + return vb2_qbuf(q, b);
> +
> + if (!q->allow_requests)
> + return -EINVAL;
> +
> + req = media_request_get_from_fd(b->request_fd);
> + if (!req)
> + return -EINVAL;
> +
> + data = to_v4l2_entity_data(media_request_get_entity_data(req,
> entity));
> + if (IS_ERR(data)) {
> + ret = PTR_ERR(data);
> + goto out;
> + }
> +
> + mutex_lock(>lock);
> +
> + if (req->state != MEDIA_REQUEST_STATE_IDLE) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> + if (ret)
> + goto out;
> +
> + vb = q->bufs[b->index];
> + switch (vb->state) {
> + case VB2_BUF_STATE_DEQUEUED:
> + break;
> + case VB2_BUF_STATE_PREPARED:
> + break;
> + case VB2_BUF_STATE_PREPARING:
> + dprintk(1, "buffer still being prepared\n");
> + ret = -EINVAL;
> + goto out;
> + default:
> + dprintk(1, "invalid buffer state %d\n", vb->state);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + /* do we already have a buffer for this request in the queue?
> */
> + list_for_each_entry(qb, >queued_buffers, node) {
> + if (qb->queue == q) {
> + ret = -EBUSY;
> + goto out;
> + }
> + }
> +
> + qb = kzalloc(sizeof(*qb), GFP_KERNEL);
> + if (!qb) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + /*
> +  * TODO should be prepare the buffer here if needed, to
> report errors
> +  * early?
> +  */
> + qb->pre_req_state = vb->state;
> + qb->queue = q;
> + memcpy(>v4l2_buf, b, sizeof(*b));

I am getting data corruption on qb->v4l2_buf.m.planes from this when
using planar buffers, only after exiting the ioctl handler (i.e. when
accessing this buffer later from the queue).

I initially thought this was because the planes pointer was copied as-is 
from userspace, but Maxime Ripard suggested that this would have
automatically triggered a visible fault in the kernel.

Thus, my best guess is that the data is properly copied from userspace
but freed when leaving the ioctl handler, which doesn't work our with
the request API.

A dirty fix that I came up with consists in re-allocating the planes
buffer here and copying its contents from "b", so that it can live
beyond the ioctl call.

I am not too sure whether this should be fixed here or in the part of
the v4l2 common code that frees this data. What do you think?

Cheers!

-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

signature.asc
Description: This is a digitally signed message part