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

[Jorge Ramirez-Ortiz]

On 08/06/2018 10:12 PM, Mark Thompson wrote:
> On 06/08/18 16:44, Jorge Ramirez-Ortiz wrote:
>> On 08/04/2018 11:43 PM, 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)));
>>>>            }
>>> (This whole function feels like it might fit better in v4l2_buffers.c?)
>>>
>>> To check my understanding here of what you've got currently (please correct me if I make a mistake here):
>>> * When making a new set of buffers (on start or format change), VIDIOC_EXPBUF is called once for each V4L2 buffer to make a DRM PRIME fd for it.
>>> * Whenever you want to return a buffer, return the fd instead if using DRM PRIME output.
>>> * When returning a set of buffers (on close or format change), wait for all references to disappear and then close all of the fds before releasing the V4L2 buffers.
>> We kept it as a context operation since release_buffers is not a per buffer operation (it just an operation that applies on all buffers, like all context operations IIRC ).
>> The problem is that even if we close the file descriptors when all references have gone, the client might still have GEM objects associated so we would fail at releasing the buffers.
> Ok.
>
>> This was noticed during testing (fixed in the test code with this commit) [1]
>> [1] https://github.com/BayLibre/ffmpeg-drm/commit/714288ab9d86397dd8230068fd9a7d3d4d76b802
>>
>> And a reminder was added to ffmpeg below
>>
>>>>        }
>>>>    -    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");
>>> Is it possible to hit this case?  Waiting for all references to disappear seems like it should cover it.  (And if the user has freed an object they are still using then that's certainly undefined behaviour, so if that's the only case here it would probably be better to abort() so that it's caught immediately.)
>>>
>> yes (as per the above explanation)
> Does errno indicate that we've hit this case specifically rather than e.g. some sort of hardware problem (decoder device physically disconnected or whatever)?

it just returns the standard "Device or resource busy" when we try to 
release the buffers since they are still in use

>
> Since it's a use-after-free on the part of the API user and therefore undefined behaviour, we should av_assert0() that it doesn't happen if we can identify it.

yes I agree.
should we assert on top of the error message or better get rid of the 
message and just add a comment to the code?

>
> (The KMS/GEM comments would also be rather confusing if the sink is something else - GL/EGL seems likely to be common, and OpenCL or Vulkan are certainly possible too.  Maybe make that more generic.)
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel