| Message ID | 20240403091306.1308878-2-yunkec@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: videobuf2-core: attach once if multiple planes share the same dbuf | expand |
Hi Yunke, On Wed, Apr 03, 2024 at 06:13:04PM +0900, Yunke Cao wrote: > The existing implementation, validating planes, checking if the planes > changed, releasing previous planes and reaquiring new planes all happens in > the same for loop. > > Split the for loop into 3 parts > 1. In the first for loop, validate planes and check if planes changed. > 2. Call __vb2_buf_dmabuf_put() to release all planes. > 3. In the second for loop, reaquire new planes. > > Signed-off-by: Yunke Cao <yunkec@chromium.org> > --- > .../media/common/videobuf2/videobuf2-core.c | 64 ++++++++++--------- > 1 file changed, 34 insertions(+), 30 deletions(-) > Thanks for the second revision and sorry for the delay. Please check my comments inline. > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index b6bf8f232f48..702f7b6f783a 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -1341,11 +1341,13 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > for (plane = 0; plane < vb->num_planes; ++plane) { > struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd); > > + planes[plane].dbuf = dbuf; > + > if (IS_ERR_OR_NULL(dbuf)) { > dprintk(q, 1, "invalid dmabuf fd for plane %d\n", > plane); > ret = -EINVAL; > - goto err; > + goto err_put_dbuf; nit: Maybe err_put_planes, since we're cleaning up the planes[] array? > } > > /* use DMABUF size if length is not provided */ > @@ -1356,17 +1358,14 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > dprintk(q, 1, "invalid dmabuf length %u for plane %d, minimum length %u\n", > planes[plane].length, plane, > vb->planes[plane].min_length); > - dma_buf_put(dbuf); > ret = -EINVAL; > - goto err; > + goto err_put_dbuf; > } > > /* Skip the plane if already verified */ > if (dbuf == vb->planes[plane].dbuf && > - vb->planes[plane].length == planes[plane].length) { > - dma_buf_put(dbuf); > + vb->planes[plane].length == planes[plane].length) > continue; > - } > > dprintk(q, 3, "buffer for plane %d changed\n", plane); > > @@ -1375,29 +1374,30 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > vb->copied_timestamp = 0; > call_void_vb_qop(vb, buf_cleanup, vb); Would it make sense to also move these two to the if (reacquired) part below, since they are done once for the entire vb? > } > + } > > - /* Release previously acquired memory if present */ > - __vb2_plane_dmabuf_put(vb, &vb->planes[plane]); > - vb->planes[plane].bytesused = 0; > - vb->planes[plane].length = 0; > - vb->planes[plane].m.fd = 0; > - vb->planes[plane].data_offset = 0; I don't see the code below setting the 4 fields above to zero. Is it intended? > + if (reacquired) { > + __vb2_buf_dmabuf_put(vb); > + > + for (plane = 0; plane < vb->num_planes; ++plane) { > + /* Acquire each plane's memory */ > + mem_priv = call_ptr_memop(attach_dmabuf, > + vb, > + q->alloc_devs[plane] ? : q->dev, > + planes[plane].dbuf, > + planes[plane].length); > + if (IS_ERR(mem_priv)) { > + dprintk(q, 1, "failed to attach dmabuf\n"); > + ret = PTR_ERR(mem_priv); > + goto err_put_dbuf; Hmm, I think in this case we need to also clean up the partially acquired planes of vb. > + } > > - /* Acquire each plane's memory */ > - mem_priv = call_ptr_memop(attach_dmabuf, > - vb, > - q->alloc_devs[plane] ? : q->dev, > - dbuf, > - planes[plane].length); > - if (IS_ERR(mem_priv)) { > - dprintk(q, 1, "failed to attach dmabuf\n"); > - ret = PTR_ERR(mem_priv); > - dma_buf_put(dbuf); > - goto err; > + vb->planes[plane].dbuf = planes[plane].dbuf; > + vb->planes[plane].mem_priv = mem_priv; > } > - > - vb->planes[plane].dbuf = dbuf; > - vb->planes[plane].mem_priv = mem_priv; > + } else { > + for (plane = 0; plane < vb->num_planes; ++plane) > + dma_buf_put(planes[plane].dbuf); > } > > /* > @@ -1413,7 +1413,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > if (ret) { > dprintk(q, 1, "failed to map dmabuf for plane %d\n", > plane); > - goto err; > + goto err_put_vb2_buf; > } > vb->planes[plane].dbuf_mapped = 1; > } I think this entire loop can also go under the (reacquired) case, since (!reacquired) means that all the planes were identical (and thus are alreday mapped). Given that now we release all the planes in one go, we could even simplify it by dropping the dbuf_mapped check from the loop. > @@ -1437,7 +1437,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > ret = call_vb_qop(vb, buf_init, vb); > if (ret) { > dprintk(q, 1, "buffer initialization failed\n"); > - goto err; > + goto err_put_vb2_buf; > } > } Same for this block. > > @@ -1445,11 +1445,15 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > if (ret) { > dprintk(q, 1, "buffer preparation failed\n"); > call_void_vb_qop(vb, buf_cleanup, vb); > - goto err; > + goto err_put_vb2_buf; > } > > return 0; > -err: > + > +err_put_dbuf: > + for (plane = 0; plane < vb->num_planes; ++plane) dma_buf_put() will throw a warning if the dmabuf pointer is NULL and just plain crash if IS_ERR(), so we shouldn't call it on array elements that we didn't succeed for. > + dma_buf_put(planes[plane].dbuf); > +err_put_vb2_buf: > /* In case of errors, release planes that were already acquired */ > __vb2_buf_dmabuf_put(vb); Actually, would it make sense to invert the order of clean-up steps here? In case if only the first loop fails, we don't really need to do anything with vb. Or am I missing something? Best regards, Tomasz
Hi Tomasz, Thanks for the review. On Fri, May 17, 2024 at 8:11 PM Tomasz Figa <tfiga@chromium.org> wrote: > > Hi Yunke, > > On Wed, Apr 03, 2024 at 06:13:04PM +0900, Yunke Cao wrote: > > The existing implementation, validating planes, checking if the planes > > changed, releasing previous planes and reaquiring new planes all happens in > > the same for loop. > > > > Split the for loop into 3 parts > > 1. In the first for loop, validate planes and check if planes changed. > > 2. Call __vb2_buf_dmabuf_put() to release all planes. > > 3. In the second for loop, reaquire new planes. > > > > Signed-off-by: Yunke Cao <yunkec@chromium.org> > > --- > > .../media/common/videobuf2/videobuf2-core.c | 64 ++++++++++--------- > > 1 file changed, 34 insertions(+), 30 deletions(-) > > > > Thanks for the second revision and sorry for the delay. Please check my > comments inline. > > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > > index b6bf8f232f48..702f7b6f783a 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-core.c > > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > > @@ -1341,11 +1341,13 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > > for (plane = 0; plane < vb->num_planes; ++plane) { > > struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd); > > > > + planes[plane].dbuf = dbuf; > > + > > if (IS_ERR_OR_NULL(dbuf)) { > > dprintk(q, 1, "invalid dmabuf fd for plane %d\n", > > plane); > > ret = -EINVAL; > > - goto err; > > + goto err_put_dbuf; > > nit: Maybe err_put_planes, since we're cleaning up the planes[] array? > err_put_planes sounds good to me. > > } > > > > /* use DMABUF size if length is not provided */ > > @@ -1356,17 +1358,14 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > > dprintk(q, 1, "invalid dmabuf length %u for plane %d, minimum length %u\n", > > planes[plane].length, plane, > > vb->planes[plane].min_length); > > - dma_buf_put(dbuf); > > ret = -EINVAL; > > - goto err; > > + goto err_put_dbuf; > > } > > > > /* Skip the plane if already verified */ > > if (dbuf == vb->planes[plane].dbuf && > > - vb->planes[plane].length == planes[plane].length) { > > - dma_buf_put(dbuf); > > + vb->planes[plane].length == planes[plane].length) > > continue; > > - } > > > > dprintk(q, 3, "buffer for plane %d changed\n", plane); > > > > @@ -1375,29 +1374,30 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > > vb->copied_timestamp = 0; > > call_void_vb_qop(vb, buf_cleanup, vb); > > Would it make sense to also move these two to the if (reacquired) part > below, since they are done once for the entire vb? > Yes, Will do in the next version. > > } > > + } > > > > - /* Release previously acquired memory if present */ > > - __vb2_plane_dmabuf_put(vb, &vb->planes[plane]); > > - vb->planes[plane].bytesused = 0; > > - vb->planes[plane].length = 0; > > - vb->planes[plane].m.fd = 0; > > - vb->planes[plane].data_offset = 0; > > I don't see the code below setting the 4 fields above to zero. Is it > intended? > I thought these were not necessary anymore. But now that I look more carefully, it is useful when there is an error below. I will add them back in the next version. Thanks for catching this! > > + if (reacquired) { > > + __vb2_buf_dmabuf_put(vb); > > + > > + for (plane = 0; plane < vb->num_planes; ++plane) { > > + /* Acquire each plane's memory */ > > + mem_priv = call_ptr_memop(attach_dmabuf, > > + vb, > > + q->alloc_devs[plane] ? : q->dev, > > + planes[plane].dbuf, > > + planes[plane].length); > > + if (IS_ERR(mem_priv)) { > > + dprintk(q, 1, "failed to attach dmabuf\n"); > > + ret = PTR_ERR(mem_priv); > > + goto err_put_dbuf; > > Hmm, I think in this case we need to also clean up the partially acquired > planes of vb. > > > + } > > > > - /* Acquire each plane's memory */ > > - mem_priv = call_ptr_memop(attach_dmabuf, > > - vb, > > - q->alloc_devs[plane] ? : q->dev, > > - dbuf, > > - planes[plane].length); > > - if (IS_ERR(mem_priv)) { > > - dprintk(q, 1, "failed to attach dmabuf\n"); > > - ret = PTR_ERR(mem_priv); > > - dma_buf_put(dbuf); > > - goto err; > > + vb->planes[plane].dbuf = planes[plane].dbuf; > > + vb->planes[plane].mem_priv = mem_priv; > > } > > - > > - vb->planes[plane].dbuf = dbuf; > > - vb->planes[plane].mem_priv = mem_priv; > > + } else { > > + for (plane = 0; plane < vb->num_planes; ++plane) > > + dma_buf_put(planes[plane].dbuf); > > } > > > > /* > > @@ -1413,7 +1413,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > > if (ret) { > > dprintk(q, 1, "failed to map dmabuf for plane %d\n", > > plane); > > - goto err; > > + goto err_put_vb2_buf; > > } > > vb->planes[plane].dbuf_mapped = 1; > > } > > I think this entire loop can also go under the (reacquired) case, since > (!reacquired) means that all the planes were identical (and thus are > alreday mapped). Given that now we release all the planes in one go, we > could even simplify it by dropping the dbuf_mapped check from the loop. > > > @@ -1437,7 +1437,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > > ret = call_vb_qop(vb, buf_init, vb); > > if (ret) { > > dprintk(q, 1, "buffer initialization failed\n"); > > - goto err; > > + goto err_put_vb2_buf; > > } > > } > > Same for this block. > > > > > @@ -1445,11 +1445,15 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > > if (ret) { > > dprintk(q, 1, "buffer preparation failed\n"); > > call_void_vb_qop(vb, buf_cleanup, vb); > > - goto err; > > + goto err_put_vb2_buf; > > } > > > > return 0; > > -err: > > + > > +err_put_dbuf: > > + for (plane = 0; plane < vb->num_planes; ++plane) > > dma_buf_put() will throw a warning if the dmabuf pointer is NULL and just > plain crash if IS_ERR(), so we shouldn't call it on array elements that we > didn't succeed for. > I see. Will do in the next version. > > + dma_buf_put(planes[plane].dbuf); > > +err_put_vb2_buf: > > /* In case of errors, release planes that were already acquired */ > > __vb2_buf_dmabuf_put(vb); > > Actually, would it make sense to invert the order of clean-up steps here? > In case if only the first loop fails, we don't really need to do anything with > vb. Or am I missing something? > It seems the original implementation will call __vb2_buf_dmabuf_put(vb) whenever dma_buf_get() returns err or length < min_length. I was trying to keep the same behavior here. Do you have any preference? Also, if "call_vb_qop(vb, buf_prepare, vb);" fails, I think we only need __vb2_buf_dmabuf_put(), but not dma_buf_put(). Best, Yunke > Best regards, > Tomasz
On Thu, May 30, 2024 at 01:13:13PM +0900, Yunke Cao wrote: > Hi Tomasz, > > Thanks for the review. > > On Fri, May 17, 2024 at 8:11 PM Tomasz Figa <tfiga@chromium.org> wrote: > > > > Hi Yunke, > > > > On Wed, Apr 03, 2024 at 06:13:04PM +0900, Yunke Cao wrote: > > > The existing implementation, validating planes, checking if the planes > > > changed, releasing previous planes and reaquiring new planes all happens in > > > the same for loop. > > > > > > Split the for loop into 3 parts > > > 1. In the first for loop, validate planes and check if planes changed. > > > 2. Call __vb2_buf_dmabuf_put() to release all planes. > > > 3. In the second for loop, reaquire new planes. > > > > > > Signed-off-by: Yunke Cao <yunkec@chromium.org> > > > --- > > > .../media/common/videobuf2/videobuf2-core.c | 64 ++++++++++--------- > > > 1 file changed, 34 insertions(+), 30 deletions(-) > > > > > > > Thanks for the second revision and sorry for the delay. Please check my > > comments inline. > > > > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > > > index b6bf8f232f48..702f7b6f783a 100644 > > > --- a/drivers/media/common/videobuf2/videobuf2-core.c > > > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > > > @@ -1341,11 +1341,13 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > > > for (plane = 0; plane < vb->num_planes; ++plane) { > > > struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd); > > > > > > + planes[plane].dbuf = dbuf; > > > + > > > if (IS_ERR_OR_NULL(dbuf)) { > > > dprintk(q, 1, "invalid dmabuf fd for plane %d\n", > > > plane); > > > ret = -EINVAL; > > > - goto err; > > > + goto err_put_dbuf; > > > > nit: Maybe err_put_planes, since we're cleaning up the planes[] array? > > > > err_put_planes sounds good to me. > > > > } > > > > > > /* use DMABUF size if length is not provided */ > > > @@ -1356,17 +1358,14 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > > > dprintk(q, 1, "invalid dmabuf length %u for plane %d, minimum length %u\n", > > > planes[plane].length, plane, > > > vb->planes[plane].min_length); > > > - dma_buf_put(dbuf); > > > ret = -EINVAL; > > > - goto err; > > > + goto err_put_dbuf; > > > } > > > > > > /* Skip the plane if already verified */ > > > if (dbuf == vb->planes[plane].dbuf && > > > - vb->planes[plane].length == planes[plane].length) { > > > - dma_buf_put(dbuf); > > > + vb->planes[plane].length == planes[plane].length) > > > continue; > > > - } > > > > > > dprintk(q, 3, "buffer for plane %d changed\n", plane); > > > > > > @@ -1375,29 +1374,30 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > > > vb->copied_timestamp = 0; > > > call_void_vb_qop(vb, buf_cleanup, vb); > > > > Would it make sense to also move these two to the if (reacquired) part > > below, since they are done once for the entire vb? > > > > Yes, Will do in the next version. > > > > } > > > + } > > > > > > - /* Release previously acquired memory if present */ > > > - __vb2_plane_dmabuf_put(vb, &vb->planes[plane]); > > > - vb->planes[plane].bytesused = 0; > > > - vb->planes[plane].length = 0; > > > - vb->planes[plane].m.fd = 0; > > > - vb->planes[plane].data_offset = 0; > > > > I don't see the code below setting the 4 fields above to zero. Is it > > intended? > > > > I thought these were not necessary anymore. > But now that I look more carefully, it is useful when there is an error below. > I will add them back in the next version. Thanks for catching this! > Actually, original code would leave the fields in a weird state in case of an error too. Maybe we could set all the fields to 0 in __vb2_plane_dmabuf_put(), so that we always have the vb2 struct in a clean state regardless of where we fail? (Could be a separate patch.) > > > + if (reacquired) { > > > + __vb2_buf_dmabuf_put(vb); > > > + > > > + for (plane = 0; plane < vb->num_planes; ++plane) { > > > + /* Acquire each plane's memory */ > > > + mem_priv = call_ptr_memop(attach_dmabuf, > > > + vb, > > > + q->alloc_devs[plane] ? : q->dev, > > > + planes[plane].dbuf, > > > + planes[plane].length); > > > + if (IS_ERR(mem_priv)) { > > > + dprintk(q, 1, "failed to attach dmabuf\n"); > > > + ret = PTR_ERR(mem_priv); > > > + goto err_put_dbuf; > > > > Hmm, I think in this case we need to also clean up the partially acquired > > planes of vb. > > > > > + } > > > > > > - /* Acquire each plane's memory */ > > > - mem_priv = call_ptr_memop(attach_dmabuf, > > > - vb, > > > - q->alloc_devs[plane] ? : q->dev, > > > - dbuf, > > > - planes[plane].length); > > > - if (IS_ERR(mem_priv)) { > > > - dprintk(q, 1, "failed to attach dmabuf\n"); > > > - ret = PTR_ERR(mem_priv); > > > - dma_buf_put(dbuf); > > > - goto err; > > > + vb->planes[plane].dbuf = planes[plane].dbuf; > > > + vb->planes[plane].mem_priv = mem_priv; > > > } > > > - > > > - vb->planes[plane].dbuf = dbuf; > > > - vb->planes[plane].mem_priv = mem_priv; > > > + } else { > > > + for (plane = 0; plane < vb->num_planes; ++plane) > > > + dma_buf_put(planes[plane].dbuf); > > > } > > > > > > /* > > > @@ -1413,7 +1413,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > > > if (ret) { > > > dprintk(q, 1, "failed to map dmabuf for plane %d\n", > > > plane); > > > - goto err; > > > + goto err_put_vb2_buf; > > > } > > > vb->planes[plane].dbuf_mapped = 1; > > > } > > > > I think this entire loop can also go under the (reacquired) case, since > > (!reacquired) means that all the planes were identical (and thus are > > alreday mapped). Given that now we release all the planes in one go, we > > could even simplify it by dropping the dbuf_mapped check from the loop. > > > > > @@ -1437,7 +1437,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > > > ret = call_vb_qop(vb, buf_init, vb); > > > if (ret) { > > > dprintk(q, 1, "buffer initialization failed\n"); > > > - goto err; > > > + goto err_put_vb2_buf; > > > } > > > } > > > > Same for this block. > > > > > > > > @@ -1445,11 +1445,15 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > > > if (ret) { > > > dprintk(q, 1, "buffer preparation failed\n"); > > > call_void_vb_qop(vb, buf_cleanup, vb); > > > - goto err; > > > + goto err_put_vb2_buf; > > > } > > > > > > return 0; > > > -err: > > > + > > > +err_put_dbuf: > > > + for (plane = 0; plane < vb->num_planes; ++plane) > > > > dma_buf_put() will throw a warning if the dmabuf pointer is NULL and just > > plain crash if IS_ERR(), so we shouldn't call it on array elements that we > > didn't succeed for. > > > > I see. Will do in the next version. > > > > + dma_buf_put(planes[plane].dbuf); > > > +err_put_vb2_buf: > > > /* In case of errors, release planes that were already acquired */ > > > __vb2_buf_dmabuf_put(vb); > > > > Actually, would it make sense to invert the order of clean-up steps here? > > In case if only the first loop fails, we don't really need to do anything with > > vb. Or am I missing something? > > > > It seems the original implementation will call __vb2_buf_dmabuf_put(vb) > whenever dma_buf_get() returns err or length < min_length. I was trying > to keep the same behavior here. Do you have any preference? > > Also, if "call_vb_qop(vb, buf_prepare, vb);" fails, I think we only need > __vb2_buf_dmabuf_put(), but not dma_buf_put(). I see, fair enough. I also reread the function to double check that we're not double cleaning anything and it looks good to me. Best regards, Tomasz
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index b6bf8f232f48..702f7b6f783a 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -1341,11 +1341,13 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) for (plane = 0; plane < vb->num_planes; ++plane) { struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd); + planes[plane].dbuf = dbuf; + if (IS_ERR_OR_NULL(dbuf)) { dprintk(q, 1, "invalid dmabuf fd for plane %d\n", plane); ret = -EINVAL; - goto err; + goto err_put_dbuf; } /* use DMABUF size if length is not provided */ @@ -1356,17 +1358,14 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) dprintk(q, 1, "invalid dmabuf length %u for plane %d, minimum length %u\n", planes[plane].length, plane, vb->planes[plane].min_length); - dma_buf_put(dbuf); ret = -EINVAL; - goto err; + goto err_put_dbuf; } /* Skip the plane if already verified */ if (dbuf == vb->planes[plane].dbuf && - vb->planes[plane].length == planes[plane].length) { - dma_buf_put(dbuf); + vb->planes[plane].length == planes[plane].length) continue; - } dprintk(q, 3, "buffer for plane %d changed\n", plane); @@ -1375,29 +1374,30 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) vb->copied_timestamp = 0; call_void_vb_qop(vb, buf_cleanup, vb); } + } - /* Release previously acquired memory if present */ - __vb2_plane_dmabuf_put(vb, &vb->planes[plane]); - vb->planes[plane].bytesused = 0; - vb->planes[plane].length = 0; - vb->planes[plane].m.fd = 0; - vb->planes[plane].data_offset = 0; + if (reacquired) { + __vb2_buf_dmabuf_put(vb); + + for (plane = 0; plane < vb->num_planes; ++plane) { + /* Acquire each plane's memory */ + mem_priv = call_ptr_memop(attach_dmabuf, + vb, + q->alloc_devs[plane] ? : q->dev, + planes[plane].dbuf, + planes[plane].length); + if (IS_ERR(mem_priv)) { + dprintk(q, 1, "failed to attach dmabuf\n"); + ret = PTR_ERR(mem_priv); + goto err_put_dbuf; + } - /* Acquire each plane's memory */ - mem_priv = call_ptr_memop(attach_dmabuf, - vb, - q->alloc_devs[plane] ? : q->dev, - dbuf, - planes[plane].length); - if (IS_ERR(mem_priv)) { - dprintk(q, 1, "failed to attach dmabuf\n"); - ret = PTR_ERR(mem_priv); - dma_buf_put(dbuf); - goto err; + vb->planes[plane].dbuf = planes[plane].dbuf; + vb->planes[plane].mem_priv = mem_priv; } - - vb->planes[plane].dbuf = dbuf; - vb->planes[plane].mem_priv = mem_priv; + } else { + for (plane = 0; plane < vb->num_planes; ++plane) + dma_buf_put(planes[plane].dbuf); } /* @@ -1413,7 +1413,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) if (ret) { dprintk(q, 1, "failed to map dmabuf for plane %d\n", plane); - goto err; + goto err_put_vb2_buf; } vb->planes[plane].dbuf_mapped = 1; } @@ -1437,7 +1437,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) ret = call_vb_qop(vb, buf_init, vb); if (ret) { dprintk(q, 1, "buffer initialization failed\n"); - goto err; + goto err_put_vb2_buf; } } @@ -1445,11 +1445,15 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) if (ret) { dprintk(q, 1, "buffer preparation failed\n"); call_void_vb_qop(vb, buf_cleanup, vb); - goto err; + goto err_put_vb2_buf; } return 0; -err: + +err_put_dbuf: + for (plane = 0; plane < vb->num_planes; ++plane) + dma_buf_put(planes[plane].dbuf); +err_put_vb2_buf: /* In case of errors, release planes that were already acquired */ __vb2_buf_dmabuf_put(vb);
The existing implementation, validating planes, checking if the planes changed, releasing previous planes and reaquiring new planes all happens in the same for loop. Split the for loop into 3 parts 1. In the first for loop, validate planes and check if planes changed. 2. Call __vb2_buf_dmabuf_put() to release all planes. 3. In the second for loop, reaquire new planes. Signed-off-by: Yunke Cao <yunkec@chromium.org> --- .../media/common/videobuf2/videobuf2-core.c | 64 ++++++++++--------- 1 file changed, 34 insertions(+), 30 deletions(-)