Hi Tomasz,
Thanks for the review.
On Fri, May 17, 2024 at 8:11 PM Tomasz Figa <tfiga@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> > .../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
