[FFmpeg-devel] [PATCH 2/3] libavcodec: v4l2m2m: output AVDRMFrameDescriptor

[Dave Stevenson]

On 9 May 2018 at 15:31, Mark Thompson <sw at jkqxz.net> wrote:
> On 09/05/18 10:02, Jorge Ramirez-Ortiz wrote:
>> On 05/09/2018 01:33 AM, Mark Thompson wrote:
>>>> diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c
>>>> index efcb0426e4..9457fadb1e 100644
>>>> --- a/libavcodec/v4l2_context.c
>>>> +++ b/libavcodec/v4l2_context.c
>>>> @@ -393,22 +393,54 @@ static int v4l2_release_buffers(V4L2Context* ctx)
>>>>      struct v4l2_requestbuffers req = {
>>>>          .memory = V4L2_MEMORY_MMAP,
>>>>          .type = ctx->type,
>>>> -        .count = 0, /* 0 -> unmaps buffers from the driver */
>>>> +        .count = 0, /* 0 -> unmap all buffers from the driver */
>>>>      };
>>>> -    int i, j;
>>>> +    int ret, i, j;
>>>>
>>>>      for (i = 0; i < ctx->num_buffers; i++) {
>>>>          V4L2Buffer *buffer = &ctx->buffers[i];
>>>>
>>>>          for (j = 0; j < buffer->num_planes; j++) {
>>>>              struct V4L2Plane_info *p = &buffer->plane_info[j];
>>>> +
>>>> +            if (V4L2_TYPE_IS_OUTPUT(ctx->type)) {
>>>> +                /* output buffers are not EXPORTED */
>>>> +                goto unmap;
>>>> +            }
>>>> +
>>>> +            if (ctx_to_m2mctx(ctx)->output_drm) {
>>>> +                /* use the DRM frame to close */
>>>> +                if (buffer->drm_frame.objects[j].fd >= 0) {
>>>> +                    if (close(buffer->drm_frame.objects[j].fd) < 0) {
>>>> +                        av_log(logger(ctx), AV_LOG_ERROR, "%s close drm fd "
>>>> +                            "[buffer=%2d, plane=%d, fd=%2d] - %s \n",
>>>> +                            ctx->name, i, j, buffer->drm_frame.objects[j].fd,
>>>> +                            av_err2str(AVERROR(errno)));
>>>> +                    }
>>>> +                }
>>>> +            }
>>>> +unmap:
>>>>              if (p->mm_addr && p->length)
>>>>                  if (munmap(p->mm_addr, p->length) < 0)
>>>> -                    av_log(logger(ctx), AV_LOG_ERROR, "%s unmap plane (%s))\n", ctx->name, av_err2str(AVERROR(errno)));
>>>> +                    av_log(logger(ctx), AV_LOG_ERROR, "%s unmap plane (%s))\n",
>>>> +                        ctx->name, av_err2str(AVERROR(errno)));
>>>>          }
>>>>      }
>>>>
>>>> -    return ioctl(ctx_to_m2mctx(ctx)->fd, VIDIOC_REQBUFS, &req);
>>>> +    ret = ioctl(ctx_to_m2mctx(ctx)->fd, VIDIOC_REQBUFS, &req);
>>>> +    if (ret < 0) {
>>>> +            av_log(logger(ctx), AV_LOG_ERROR, "release all %s buffers (%s)\n",
>>>> +                ctx->name, av_err2str(AVERROR(errno)));
>>>> +
>>>> +            if (ctx_to_m2mctx(ctx)->output_drm)
>>>> +                av_log(logger(ctx), AV_LOG_ERROR,
>>>> +                    "Make sure the DRM client releases all FB/GEM objects before closing the codec (ie):\n"
>>>> +                    "for all buffers: \n"
>>>> +                    "  1. drmModeRmFB(..)\n"
>>>> +                    "  2. drmIoctl(.., DRM_IOCTL_GEM_CLOSE,... )\n");
>>> You should be able to keep references to all DRM PRIME frames as they leave the codec, and then only call this when all references have disappeared.
>>>
>>
>> yes, that is the way it was working for non DRM frames.
>>
>> If we need to guarantee that exact same behavior, ffmpeg needs to be able to remove the fb handles and close the gem objects on each buffer being released (so mirror the action we take with just  munmap)
>
> I believe the unref function should only need to close the PRIME file descriptors.  (Assuming they are new rather than magically reused somehow - VAAPI makes new fds which have to be closed, while Rockchip reuses them so they shouldn't be.  This is handled correctly by the buffer references in each case, the user does not need to do anything differently.)  Any framebuffer handles and related are created by the user rather than libavcodec, who won't unref the frame they are made from until everything derived from it is suitably released.

Videobuf2 (the V4L2 buffer allocation/handlier) will have created a
dmabuf fd for each buffer/plane via the VIDIOC_EXPBUF ioctl. It is up
to the client to close those.

There is an odd-ball in videobuf2. On the REQBUFS(0) call to release
all the buffers it checks the number of users of the buffers, and
fails the release call if anyone is using it. It's even documented
that way [1].
This had been raised with the V4L2 maintainers as strange and
annoying, but not resolved [2]. It probably needs picking up again and
getting merged into mainline, but that will then have a hard
requirement of needing a latest kernel.

The error message here is really for information that things haven't
cleaned up in the manner that you might expect. Either you just ignore
it and let V4L2 clean up eventually when the main V4L2 device fd gets
closed, or you need applications to behave in a particular manner.
Ignoring it has potential knockon issues as calls like S_FMT to
request a different format/resolution will typically fail if buffers
are allocated.

>> This is what I mean:
>> https://github.com/BayLibre/ffmpeg-drm/blob/master/main.c#L391
>>
>> I think this would mean that the libavcodec should open the drm device instead of the client application doing it and perform the actions above in unref.
>> would that be acceptable?
>
> Is there necessarily an explicit DRM device associated with a V4L2 decoder?  (And if so, how do you find it?)  For comparison, there isn't one for Rockchip - there we create a dummy device to host the frames context.

No, videobuf2 will have made the allocation and created the dmabuf
object you get the fd for. That could be imported into DRM (via
DRM_IOCTL_PRIME_FD_TO_HANDLE), EGL (via EGL_LINUX_DMA_BUF_EXT), a V4L2
encoder, or any other subsystem that supports importing dmabufs.
There is no reliance at all on DRM or any particular DRM device, only dmabuf.

  Dave

[1] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/vidioc-reqbufs.html
"Applications can call ioctl VIDIOC_REQBUFS again to change the number
of buffers, however this cannot succeed when any buffers are still
mapped. A count value of zero frees all buffers, after aborting or
finishing any DMA in progress, an implicit VIDIOC_STREAMOFF."
[2] https://patchwork.kernel.org/patch/7404111/