| Message ID | 1379576249-16909-1-git-send-email-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 19, 2013 at 4:37 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > __setup_offsets fills the v4l2_planes' mem_offset fields, which is only valid > for V4L2_MEMORY_MMAP type buffers. For V4L2_MEMORY_DMABUF and _USERPTR buffers, > this incorrectly overwrites the fd and userptr fields. I'm not particularly against this change, but I'm curious if anything that you were doing was broken by this call? The buffers are created here, so their fields don't contain anything that could be overwritten (although keeping them at 0 is preferable). > > Reported-by: Michael Olbrich <m.olbrich@pengutronix.de> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/media/v4l2-core/videobuf2-core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index 9c865da..95a798e 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -241,7 +241,8 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory, > q->bufs[q->num_buffers + buffer] = vb; > } > > - __setup_offsets(q, buffer); > + if (memory == V4L2_MEMORY_MMAP) > + __setup_offsets(q, buffer); > > dprintk(1, "Allocated %d buffers, %d plane(s) each\n", > buffer, num_planes); > -- > 1.8.4.rc3 >
Hello, On 9/19/2013 9:37 AM, Philipp Zabel wrote: > __setup_offsets fills the v4l2_planes' mem_offset fields, which is only valid > for V4L2_MEMORY_MMAP type buffers. For V4L2_MEMORY_DMABUF and _USERPTR buffers, > this incorrectly overwrites the fd and userptr fields. > > Reported-by: Michael Olbrich <m.olbrich@pengutronix.de> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/media/v4l2-core/videobuf2-core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index 9c865da..95a798e 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -241,7 +241,8 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory, > q->bufs[q->num_buffers + buffer] = vb; > } > > - __setup_offsets(q, buffer); > + if (memory == V4L2_MEMORY_MMAP) > + __setup_offsets(q, buffer); > > dprintk(1, "Allocated %d buffers, %d plane(s) each\n", > buffer, num_planes); Best regards
Hi Pawel, Am Donnerstag, den 19.09.2013, 16:54 +0900 schrieb Pawel Osciak: > On Thu, Sep 19, 2013 at 4:37 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > > __setup_offsets fills the v4l2_planes' mem_offset fields, which is only valid > > for V4L2_MEMORY_MMAP type buffers. For V4L2_MEMORY_DMABUF and _USERPTR buffers, > > this incorrectly overwrites the fd and userptr fields. > > I'm not particularly against this change, but I'm curious if anything > that you were doing was broken by this call? The buffers are created > here, so their fields don't contain anything that could be overwritten > (although keeping them at 0 is preferable). nothing was actually broken, but even though the spec doesn't say anything about the QUERYBUF return values in the DMABUF/USERPTR cases, setting them to some random initial value doesn't seem right. Maybe the documentation could be amended to mention fd and userptr, although in this case the fd should probably be set to -1 initially. QUERYBUF could then be used to find free slots. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks Philipp. Acked-by: Pawel Osciak <pawel@osciak.com> On Thu, Sep 19, 2013 at 5:30 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Hi Pawel, > > Am Donnerstag, den 19.09.2013, 16:54 +0900 schrieb Pawel Osciak: >> On Thu, Sep 19, 2013 at 4:37 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: >> > __setup_offsets fills the v4l2_planes' mem_offset fields, which is only valid >> > for V4L2_MEMORY_MMAP type buffers. For V4L2_MEMORY_DMABUF and _USERPTR buffers, >> > this incorrectly overwrites the fd and userptr fields. >> >> I'm not particularly against this change, but I'm curious if anything >> that you were doing was broken by this call? The buffers are created >> here, so their fields don't contain anything that could be overwritten >> (although keeping them at 0 is preferable). > > nothing was actually broken, but even though the spec doesn't say > anything about the QUERYBUF return values in the DMABUF/USERPTR cases, > setting them to some random initial value doesn't seem right. > > Maybe the documentation could be amended to mention fd and userptr, > although in this case the fd should probably be set to -1 initially. > QUERYBUF could then be used to find free slots. > > regards > Philipp >
On 09/19/2013 09:37 AM, Philipp Zabel wrote: > __setup_offsets fills the v4l2_planes' mem_offset fields, which is only valid > for V4L2_MEMORY_MMAP type buffers. For V4L2_MEMORY_DMABUF and _USERPTR buffers, > this incorrectly overwrites the fd and userptr fields. > > Reported-by: Michael Olbrich <m.olbrich@pengutronix.de> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> Regards, Hans > --- > drivers/media/v4l2-core/videobuf2-core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index 9c865da..95a798e 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -241,7 +241,8 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory, > q->bufs[q->num_buffers + buffer] = vb; > } > > - __setup_offsets(q, buffer); > + if (memory == V4L2_MEMORY_MMAP) > + __setup_offsets(q, buffer); > > dprintk(1, "Allocated %d buffers, %d plane(s) each\n", > buffer, num_planes); > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c index 9c865da..95a798e 100644 --- a/drivers/media/v4l2-core/videobuf2-core.c +++ b/drivers/media/v4l2-core/videobuf2-core.c @@ -241,7 +241,8 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory, q->bufs[q->num_buffers + buffer] = vb; } - __setup_offsets(q, buffer); + if (memory == V4L2_MEMORY_MMAP) + __setup_offsets(q, buffer); dprintk(1, "Allocated %d buffers, %d plane(s) each\n", buffer, num_planes);
__setup_offsets fills the v4l2_planes' mem_offset fields, which is only valid for V4L2_MEMORY_MMAP type buffers. For V4L2_MEMORY_DMABUF and _USERPTR buffers, this incorrectly overwrites the fd and userptr fields. Reported-by: Michael Olbrich <m.olbrich@pengutronix.de> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/media/v4l2-core/videobuf2-core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)