All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] vb2: clear timestamp if buffer mem is reacquired
@ 2019-02-02 17:03 Hans Verkuil
  2019-02-02 20:59 ` Nicolas Dufresne
  0 siblings, 1 reply; 3+ messages in thread
From: Hans Verkuil @ 2019-02-02 17:03 UTC (permalink / raw)
  To: Linux Media Mailing List, Tomasz Figa, Alexandre Courbot,
	Paul Kocialkowski

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.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
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);

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] vb2: clear timestamp if buffer mem is reacquired
  2019-02-02 17:03 [PATCH] vb2: clear timestamp if buffer mem is reacquired Hans Verkuil
@ 2019-02-02 20:59 ` Nicolas Dufresne
  2019-02-03  8:23   ` Hans Verkuil
  0 siblings, 1 reply; 3+ messages in thread
From: Nicolas Dufresne @ 2019-02-02 20:59 UTC (permalink / raw)
  To: Hans Verkuil, Linux Media Mailing List, Tomasz Figa,
	Alexandre Courbot, Paul Kocialkowski

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.

Nicolas

> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> 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);


^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] vb2: clear timestamp if buffer mem is reacquired
  2019-02-02 20:59 ` Nicolas Dufresne
@ 2019-02-03  8:23   ` Hans Verkuil
  0 siblings, 0 replies; 3+ messages in thread
From: Hans Verkuil @ 2019-02-03  8:23 UTC (permalink / raw)
  To: Nicolas Dufresne, Linux Media Mailing List, Tomasz Figa,
	Alexandre Courbot, Paul Kocialkowski

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@cisco.com>
>> ---
>> 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);
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-02-03  8:23 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-02 17:03 [PATCH] vb2: clear timestamp if buffer mem is reacquired Hans Verkuil
2019-02-02 20:59 ` Nicolas Dufresne
2019-02-03  8:23   ` Hans Verkuil

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.