On 12 August 2018 at 22:25, Jorge Ramirez-Ortiz <jrami...@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@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
