Re: Proposed updates and guidelines for MPEG-2, H.264 and H.265 stateless support

Hans Verkuil
2019-06-07T08:23:09Z

On 6/7/19 8:45 AM, Hans Verkuil wrote:
> On 6/7/19 8:11 AM, Tomasz Figa wrote:
>> On Wed, May 22, 2019 at 7:56 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>>> I share the same experience. Bitstream buffers are usually so small that
>>>> you can always find a physically contiguous memory region for them and a
>>>> memcpy() will be faster than the overhead of getting an IOMMU involved.
>>>> This obviously depends on the specific hardware, but there's always some
>>>> threshold before which mapping through an IOMMU just doesn't make sense
>>>> from a fragmentation and/or performance point of view.
>>>>
>>>> I wonder, though, if it's not possible to keep userptr buffers around
>>>> and avoid the constant mapping/unmapping. If we only performed cache
>>>> maintenance on them as necessary, perhaps that could provide a viable,
>>>> maybe even good, zero-copy mechanism.
>>>
>>> The vb2 framework will keep the mapping for a userptr as long as userspace
>>> uses the same userptr for every buffer.
>>>
>>> I.e. the first time a buffer with index I is queued the userptr is mapped.
>>> If that buffer is later dequeued and then requeued again with the same
>>> userptr the vb2 core will reuse the old mapping. Otherwise it will unmap
>>> and map again with the new userptr.
>>
>> That's a good point. I forgot that we've been seeing random memory
>> corruptions (fortunately of the userptr memory only, not random system
>> memory) because of this behavior and carrying a patch in all
>> downstream branches to remove this caching.
>>
>> I can see that we keep references on the pages that corresponded to
>> the user VMA at the time the buffer was queued, but are we guaranteed
>> that the list of pages backing that VMA hasn't changed over time?
>
> Since you are seeing memory corruptions, the answer to this is perhaps 'no'?
>
> I think the (quite possibly faulty) reasoning was that while memory is mapped,
> userspace can't do a free()/malloc() pair and end up with the same address.
>
> I suspect this might be a wrong assumption, and in that case we're better off
> removing this check.
>
> But I'd like to have some confirmation that it is really wrong.

I did some testing, and indeed, this doesn't work.

A patch fixing this will be posted soon.

Regards,

Hans

>
> USERPTR isn't used very often, so it wouldn't surprise me if it is buggy.
>
> Regards,
>
> Hans
>
>>
>>>
>>> The same is done for dmabuf, BTW. So if userspace keeps changing dmabuf
>>> fds for each buffer, then that is not optimal.
>>
>> We could possibly try to search through the other buffers and reuse
>> the mapping if there is a match?
>>
>> Best regards,
>> Tomasz
>>
>