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

Dave Stevenson dave.stevenson at raspberrypi.org
Mon Aug 13 11:57:54 EEST 2018


On 12 August 2018 at 22:25, Jorge Ramirez-Ortiz <jramirez at baylibre.com> wrote:
> 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

There was a kernel patch proposed back in Oct 2015 to remove this
restriction - https://lore.kernel.org/patchwork/patch/607853/
It was missing the associated documentation changes so has never been
merged. It's been on my list of things to address, but I haven't sent
it to the linux-media list as yet.

If you want to work with older kernels then you'll have to work with
the current behaviour anyway.

>>
>> 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
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


More information about the ffmpeg-devel mailing list