On 02/02/2019 09:59 PM, Nicolas Dufresne wrote:
> Le samedi 02 février 2019 à 18:03 +0100, Hans Verkuil a écrit :
>> Stateless codecs have to find buffers based on a timestamp (vb2_find_timestamp).
>> The timestamp is set to 0 initially, so prohibit finding timestamp 0 since it
>> could find unused buffers without associated memory (userptr or dmabuf).
>>
>> The memory associated with a buffer will also disappear if the same buffer was
>> requeued with a different userptr address or dmabuf fd. Detect this and set the
>> timestamp of that buffer to 0 if this happens.
>
> Just a small concern, does it mean 0 is considered an invalid timestamp
> ? In streaming it would be quite normal for a first picture to have PTS
> 0.
Good point. Perhaps I should make ~0ULL the invalid timestamp instead.
I'll look at that tomorrow.
Regards,
Hans
>
> Nicolas
>
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> ---
>> Note: I think it is still necessary to lock a buffer when it is in use as
>> a reference frame, otherwise a userspace application can queue it again with
>> a different dmabuf fd, which could free the memory of the old dmabuf.
>>
>> vb2_find_buffer should probably do that.
>> ---
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index e07b6bdb6982..b664d9790330 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1043,6 +1043,8 @@ static int __prepare_userptr(struct vb2_buffer *vb)
>> reacquired = true;
>> call_void_vb_qop(vb, buf_cleanup, vb);
>> }
>> + if (!q->is_output)
>> + vb->timestamp = 0;
>> call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv);
>> }
>>
>> @@ -1157,6 +1159,8 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>> /* Skip the plane if already verified */
>> if (dbuf == vb->planes[plane].dbuf &&
>> vb->planes[plane].length == planes[plane].length) {
>> + if (!q->is_output)
>> + vb->timestamp = 0;
>> dma_buf_put(dbuf);
>> continue;
>> }
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 3aeaea3af42a..8e966fa81b7e 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -603,6 +603,9 @@ int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
>> {
>> unsigned int i;
>>
>> + if (!timestamp)
>> + return -1;
>> +
>> for (i = start_idx; i < q->num_buffers; i++)
>> if (q->bufs[i]->timestamp == timestamp)
>> return i;
>> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
>> index 8a10889dc2fd..01bf4b2199c7 100644
>> --- a/include/media/videobuf2-v4l2.h
>> +++ b/include/media/videobuf2-v4l2.h
>> @@ -59,14 +59,14 @@ struct vb2_v4l2_buffer {
>> * vb2_find_timestamp() - Find buffer with given timestamp in the queue
>> *
>> * @q: pointer to &struct vb2_queue with videobuf2 queue.
>> - * @timestamp: the timestamp to find.
>> + * @timestamp: the timestamp to find. Must be > 0.
>> * @start_idx: the start index (usually 0) in the buffer array to start
>> * searching from. Note that there may be multiple buffers
>> * with the same timestamp value, so you can restart the search
>> * by setting @start_idx to the previously found index + 1.
>> *
>> * Returns the buffer index of the buffer with the given @timestamp, or
>> - * -1 if no buffer with @timestamp was found.
>> + * -1 if no buffer with @timestamp was found or if @timestamp was 0.
>> */
>> int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
>> unsigned int start_idx);
>
