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@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!
>
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
