diff mbox series

media: videobuf2: Add missing doc comment for waiting_in_dqbuf

Message ID 20240105134020.34312-1-andrzej.p@collabora.com (mailing list archive)
State Changes Requested
Delegated to: Hans Verkuil
Headers
Series media: videobuf2: Add missing doc comment for waiting_in_dqbuf |

Commit Message

Andrzej Pietrasiewicz Jan. 5, 2024, 1:40 p.m. UTC 
  While at it rearrange other comments to match the order of struct members.

Fixes: d65842f7126a ("media: vb2: add waiting_in_dqbuf flag")
Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 include/media/videobuf2-core.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Nicolas Dufresne Jan. 9, 2024, 4:11 p.m. UTC | #1 
Le vendredi 05 janvier 2024 à 14:40 +0100, Andrzej Pietrasiewicz a écrit :
> While at it rearrange other comments to match the order of struct members.
> 
> Fixes: d65842f7126a ("media: vb2: add waiting_in_dqbuf flag")
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
>  include/media/videobuf2-core.h | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index e41204df19f0..5020e052eda0 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -72,6 +72,10 @@ struct vb2_buffer;
>   *		 argument to other ops in this structure.
>   * @put_userptr: inform the allocator that a USERPTR buffer will no longer
>   *		 be used.
> + * @prepare:	called every time the buffer is passed from userspace to the
> + *		driver, useful for cache synchronisation, optional.
> + * @finish:	called every time the buffer is passed back from the driver
> + *		to the userspace, also optional.
>   * @attach_dmabuf: attach a shared &struct dma_buf for a hardware operation;
>   *		   used for DMABUF memory types; dev is the alloc device
>   *		   dbuf is the shared dma_buf; returns ERR_PTR() on failure;
> @@ -86,10 +90,6 @@ struct vb2_buffer;
>   *		dmabuf.
>   * @unmap_dmabuf: releases access control to the dmabuf - allocator is notified
>   *		  that this driver is done using the dmabuf for now.
> - * @prepare:	called every time the buffer is passed from userspace to the
> - *		driver, useful for cache synchronisation, optional.
> - * @finish:	called every time the buffer is passed back from the driver
> - *		to the userspace, also optional.
>   * @vaddr:	return a kernel virtual address to a given memory buffer
>   *		associated with the passed private structure or NULL if no
>   *		such mapping exists.
> @@ -484,7 +484,6 @@ struct vb2_buf_ops {
>   *		caller. For example, for V4L2, it should match
>   *		the types defined on &enum v4l2_buf_type.
>   * @io_modes:	supported io methods (see &enum vb2_io_modes).
> - * @alloc_devs:	&struct device memory type/allocator-specific per-plane device
>   * @dev:	device to use for the default allocation context if the driver
>   *		doesn't fill in the @alloc_devs array.
>   * @dma_attrs:	DMA attributes to use for the DMA.
> @@ -550,6 +549,7 @@ struct vb2_buf_ops {
>   *		@start_streaming can be called. Used when a DMA engine
>   *		cannot be started unless at least this number of buffers
>   *		have been queued into the driver.
> + * @alloc_devs:	&struct device memory type/allocator-specific per-plane device
>   */
>  /*
>   * Private elements (won't appear at the uAPI book):
> @@ -571,6 +571,8 @@ struct vb2_buf_ops {
>   * @waiting_for_buffers: used in poll() to check if vb2 is still waiting for
>   *		buffers. Only set for capture queues if qbuf has not yet been
>   *		called since poll() needs to return %EPOLLERR in that situation.
> + * @waiting_in_dqbuf: set whenever vb2_queue->lock is released while waiting for
> + *		a buffer to arrive so that -EBUSY can be returned when appropriate
>   * @is_multiplanar: set if buffer type is multiplanar
>   * @is_output:	set if buffer type is output
>   * @copy_timestamp: set if vb2-core should set timestamps
Tomasz Figa Jan. 10, 2024, 12:41 p.m. UTC | #2 
On Fri, Jan 5, 2024 at 10:40 PM Andrzej Pietrasiewicz
<andrzej.p@collabora.com> wrote:
>
> While at it rearrange other comments to match the order of struct members.
>
> Fixes: d65842f7126a ("media: vb2: add waiting_in_dqbuf flag")
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> ---
>  include/media/videobuf2-core.h | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index e41204df19f0..5020e052eda0 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -72,6 +72,10 @@ struct vb2_buffer;
>   *              argument to other ops in this structure.
>   * @put_userptr: inform the allocator that a USERPTR buffer will no longer
>   *              be used.
> + * @prepare:   called every time the buffer is passed from userspace to the
> + *             driver, useful for cache synchronisation, optional.
> + * @finish:    called every time the buffer is passed back from the driver
> + *             to the userspace, also optional.
>   * @attach_dmabuf: attach a shared &struct dma_buf for a hardware operation;
>   *                used for DMABUF memory types; dev is the alloc device
>   *                dbuf is the shared dma_buf; returns ERR_PTR() on failure;
> @@ -86,10 +90,6 @@ struct vb2_buffer;
>   *             dmabuf.
>   * @unmap_dmabuf: releases access control to the dmabuf - allocator is notified
>   *               that this driver is done using the dmabuf for now.
> - * @prepare:   called every time the buffer is passed from userspace to the
> - *             driver, useful for cache synchronisation, optional.
> - * @finish:    called every time the buffer is passed back from the driver
> - *             to the userspace, also optional.
>   * @vaddr:     return a kernel virtual address to a given memory buffer
>   *             associated with the passed private structure or NULL if no
>   *             such mapping exists.
> @@ -484,7 +484,6 @@ struct vb2_buf_ops {
>   *             caller. For example, for V4L2, it should match
>   *             the types defined on &enum v4l2_buf_type.
>   * @io_modes:  supported io methods (see &enum vb2_io_modes).
> - * @alloc_devs:        &struct device memory type/allocator-specific per-plane device
>   * @dev:       device to use for the default allocation context if the driver
>   *             doesn't fill in the @alloc_devs array.
>   * @dma_attrs: DMA attributes to use for the DMA.
> @@ -550,6 +549,7 @@ struct vb2_buf_ops {
>   *             @start_streaming can be called. Used when a DMA engine
>   *             cannot be started unless at least this number of buffers
>   *             have been queued into the driver.
> + * @alloc_devs:        &struct device memory type/allocator-specific per-plane device
>   */
>  /*
>   * Private elements (won't appear at the uAPI book):
> @@ -571,6 +571,8 @@ struct vb2_buf_ops {
>   * @waiting_for_buffers: used in poll() to check if vb2 is still waiting for
>   *             buffers. Only set for capture queues if qbuf has not yet been
>   *             called since poll() needs to return %EPOLLERR in that situation.
> + * @waiting_in_dqbuf: set whenever vb2_queue->lock is released while waiting for
> + *             a buffer to arrive so that -EBUSY can be returned when appropriate

Appreciate documentation improvements. Thanks!

Just one comment: Could we make it more clear who sets it? For example:

    Set by the core for the duration of a blocking DQBUF, when it has
to wait for
    a buffer to become available with vb2_queue->lock released. Used to prevent
    destroying the queue by other threads.

WDYT?

Best regards,
Tomasz
Andrzej Pietrasiewicz Jan. 12, 2024, 9:26 p.m. UTC | #3 
Hi Tomasz,

W dniu 10.01.2024 o 13:41, Tomasz Figa pisze:
> On Fri, Jan 5, 2024 at 10:40 PM Andrzej Pietrasiewicz
> <andrzej.p@collabora.com> wrote:
>>
>> While at it rearrange other comments to match the order of struct members.
>>
>> Fixes: d65842f7126a ("media: vb2: add waiting_in_dqbuf flag")
>> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>> ---
>>   include/media/videobuf2-core.h | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index e41204df19f0..5020e052eda0 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -72,6 +72,10 @@ struct vb2_buffer;
>>    *              argument to other ops in this structure.
>>    * @put_userptr: inform the allocator that a USERPTR buffer will no longer
>>    *              be used.
>> + * @prepare:   called every time the buffer is passed from userspace to the
>> + *             driver, useful for cache synchronisation, optional.
>> + * @finish:    called every time the buffer is passed back from the driver
>> + *             to the userspace, also optional.
>>    * @attach_dmabuf: attach a shared &struct dma_buf for a hardware operation;
>>    *                used for DMABUF memory types; dev is the alloc device
>>    *                dbuf is the shared dma_buf; returns ERR_PTR() on failure;
>> @@ -86,10 +90,6 @@ struct vb2_buffer;
>>    *             dmabuf.
>>    * @unmap_dmabuf: releases access control to the dmabuf - allocator is notified
>>    *               that this driver is done using the dmabuf for now.
>> - * @prepare:   called every time the buffer is passed from userspace to the
>> - *             driver, useful for cache synchronisation, optional.
>> - * @finish:    called every time the buffer is passed back from the driver
>> - *             to the userspace, also optional.
>>    * @vaddr:     return a kernel virtual address to a given memory buffer
>>    *             associated with the passed private structure or NULL if no
>>    *             such mapping exists.
>> @@ -484,7 +484,6 @@ struct vb2_buf_ops {
>>    *             caller. For example, for V4L2, it should match
>>    *             the types defined on &enum v4l2_buf_type.
>>    * @io_modes:  supported io methods (see &enum vb2_io_modes).
>> - * @alloc_devs:        &struct device memory type/allocator-specific per-plane device
>>    * @dev:       device to use for the default allocation context if the driver
>>    *             doesn't fill in the @alloc_devs array.
>>    * @dma_attrs: DMA attributes to use for the DMA.
>> @@ -550,6 +549,7 @@ struct vb2_buf_ops {
>>    *             @start_streaming can be called. Used when a DMA engine
>>    *             cannot be started unless at least this number of buffers
>>    *             have been queued into the driver.
>> + * @alloc_devs:        &struct device memory type/allocator-specific per-plane device
>>    */
>>   /*
>>    * Private elements (won't appear at the uAPI book):
>> @@ -571,6 +571,8 @@ struct vb2_buf_ops {
>>    * @waiting_for_buffers: used in poll() to check if vb2 is still waiting for
>>    *             buffers. Only set for capture queues if qbuf has not yet been
>>    *             called since poll() needs to return %EPOLLERR in that situation.
>> + * @waiting_in_dqbuf: set whenever vb2_queue->lock is released while waiting for
>> + *             a buffer to arrive so that -EBUSY can be returned when appropriate
> 
> Appreciate documentation improvements. Thanks!
>

I haven't been hunting for opportunities to improve the documentation,
the opportunity has found me ;P

> Just one comment: Could we make it more clear who sets it? For example >      Set by the core for the duration of a blocking DQBUF, when it has
> to wait for
>      a buffer to become available with vb2_queue->lock released. Used to prevent
>      destroying the queue by other threads.

Makes sense for me.

@Nicolas are you ok with changing the text and retaining your R-b?

Andrzej
> 
> WDYT?
> 
> Best regards,
> Tomasz
>
Hans Verkuil Feb. 12, 2024, 10:52 a.m. UTC | #4 
On 12/01/2024 22:26, Andrzej Pietrasiewicz wrote:
> Hi Tomasz,
> 
> W dniu 10.01.2024 o 13:41, Tomasz Figa pisze:
>> On Fri, Jan 5, 2024 at 10:40 PM Andrzej Pietrasiewicz
>> <andrzej.p@collabora.com> wrote:
>>>
>>> While at it rearrange other comments to match the order of struct members.
>>>
>>> Fixes: d65842f7126a ("media: vb2: add waiting_in_dqbuf flag")
>>> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>>> ---
>>>   include/media/videobuf2-core.h | 12 +++++++-----
>>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>>> index e41204df19f0..5020e052eda0 100644
>>> --- a/include/media/videobuf2-core.h
>>> +++ b/include/media/videobuf2-core.h
>>> @@ -72,6 +72,10 @@ struct vb2_buffer;
>>>    *              argument to other ops in this structure.
>>>    * @put_userptr: inform the allocator that a USERPTR buffer will no longer
>>>    *              be used.
>>> + * @prepare:   called every time the buffer is passed from userspace to the
>>> + *             driver, useful for cache synchronisation, optional.
>>> + * @finish:    called every time the buffer is passed back from the driver
>>> + *             to the userspace, also optional.
>>>    * @attach_dmabuf: attach a shared &struct dma_buf for a hardware operation;
>>>    *                used for DMABUF memory types; dev is the alloc device
>>>    *                dbuf is the shared dma_buf; returns ERR_PTR() on failure;
>>> @@ -86,10 +90,6 @@ struct vb2_buffer;
>>>    *             dmabuf.
>>>    * @unmap_dmabuf: releases access control to the dmabuf - allocator is notified
>>>    *               that this driver is done using the dmabuf for now.
>>> - * @prepare:   called every time the buffer is passed from userspace to the
>>> - *             driver, useful for cache synchronisation, optional.
>>> - * @finish:    called every time the buffer is passed back from the driver
>>> - *             to the userspace, also optional.
>>>    * @vaddr:     return a kernel virtual address to a given memory buffer
>>>    *             associated with the passed private structure or NULL if no
>>>    *             such mapping exists.
>>> @@ -484,7 +484,6 @@ struct vb2_buf_ops {
>>>    *             caller. For example, for V4L2, it should match
>>>    *             the types defined on &enum v4l2_buf_type.
>>>    * @io_modes:  supported io methods (see &enum vb2_io_modes).
>>> - * @alloc_devs:        &struct device memory type/allocator-specific per-plane device
>>>    * @dev:       device to use for the default allocation context if the driver
>>>    *             doesn't fill in the @alloc_devs array.
>>>    * @dma_attrs: DMA attributes to use for the DMA.
>>> @@ -550,6 +549,7 @@ struct vb2_buf_ops {
>>>    *             @start_streaming can be called. Used when a DMA engine
>>>    *             cannot be started unless at least this number of buffers
>>>    *             have been queued into the driver.
>>> + * @alloc_devs:        &struct device memory type/allocator-specific per-plane device
>>>    */
>>>   /*
>>>    * Private elements (won't appear at the uAPI book):
>>> @@ -571,6 +571,8 @@ struct vb2_buf_ops {
>>>    * @waiting_for_buffers: used in poll() to check if vb2 is still waiting for
>>>    *             buffers. Only set for capture queues if qbuf has not yet been
>>>    *             called since poll() needs to return %EPOLLERR in that situation.
>>> + * @waiting_in_dqbuf: set whenever vb2_queue->lock is released while waiting for
>>> + *             a buffer to arrive so that -EBUSY can be returned when appropriate
>>
>> Appreciate documentation improvements. Thanks!
>>
> 
> I haven't been hunting for opportunities to improve the documentation,
> the opportunity has found me ;P
> 
>> Just one comment: Could we make it more clear who sets it? For example >      Set by the core for the duration of a blocking DQBUF, when it has
>> to wait for
>>      a buffer to become available with vb2_queue->lock released. Used to prevent
>>      destroying the queue by other threads.
> 
> Makes sense for me.
> 
> @Nicolas are you ok with changing the text and retaining your R-b?
> 
> Andrzej

Since this patch no longer applies, I think it is best if you make a v2.

I'll mark this one as "Changes Requested" in patchwork.

Regards,

	Hans


>>
>> WDYT?
>>
>> Best regards,
>> Tomasz
>>
> 
>
Andrzej Pietrasiewicz Feb. 12, 2024, 7:13 p.m. UTC | #5 
Hi Hans,

dniu 12.02.2024 o 11:52, Hans Verkuil pisze:
> On 12/01/2024 22:26, Andrzej Pietrasiewicz wrote:
>> Hi Tomasz,
>>
>> W dniu 10.01.2024 o 13:41, Tomasz Figa pisze:
>>> On Fri, Jan 5, 2024 at 10:40 PM Andrzej Pietrasiewicz
>>> <andrzej.p@collabora.com> wrote:
>>>>
>>>> While at it rearrange other comments to match the order of struct members.
>>>>
>>>> Fixes: d65842f7126a ("media: vb2: add waiting_in_dqbuf flag")
>>>> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>>>> ---
>>>>    include/media/videobuf2-core.h | 12 +++++++-----
>>>>    1 file changed, 7 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>>>> index e41204df19f0..5020e052eda0 100644
>>>> --- a/include/media/videobuf2-core.h
>>>> +++ b/include/media/videobuf2-core.h
>>>> @@ -72,6 +72,10 @@ struct vb2_buffer;
>>>>     *              argument to other ops in this structure.
>>>>     * @put_userptr: inform the allocator that a USERPTR buffer will no longer
>>>>     *              be used.
>>>> + * @prepare:   called every time the buffer is passed from userspace to the
>>>> + *             driver, useful for cache synchronisation, optional.
>>>> + * @finish:    called every time the buffer is passed back from the driver
>>>> + *             to the userspace, also optional.
>>>>     * @attach_dmabuf: attach a shared &struct dma_buf for a hardware operation;
>>>>     *                used for DMABUF memory types; dev is the alloc device
>>>>     *                dbuf is the shared dma_buf; returns ERR_PTR() on failure;
>>>> @@ -86,10 +90,6 @@ struct vb2_buffer;
>>>>     *             dmabuf.
>>>>     * @unmap_dmabuf: releases access control to the dmabuf - allocator is notified
>>>>     *               that this driver is done using the dmabuf for now.
>>>> - * @prepare:   called every time the buffer is passed from userspace to the
>>>> - *             driver, useful for cache synchronisation, optional.
>>>> - * @finish:    called every time the buffer is passed back from the driver
>>>> - *             to the userspace, also optional.
>>>>     * @vaddr:     return a kernel virtual address to a given memory buffer
>>>>     *             associated with the passed private structure or NULL if no
>>>>     *             such mapping exists.
>>>> @@ -484,7 +484,6 @@ struct vb2_buf_ops {
>>>>     *             caller. For example, for V4L2, it should match
>>>>     *             the types defined on &enum v4l2_buf_type.
>>>>     * @io_modes:  supported io methods (see &enum vb2_io_modes).
>>>> - * @alloc_devs:        &struct device memory type/allocator-specific per-plane device
>>>>     * @dev:       device to use for the default allocation context if the driver
>>>>     *             doesn't fill in the @alloc_devs array.
>>>>     * @dma_attrs: DMA attributes to use for the DMA.
>>>> @@ -550,6 +549,7 @@ struct vb2_buf_ops {
>>>>     *             @start_streaming can be called. Used when a DMA engine
>>>>     *             cannot be started unless at least this number of buffers
>>>>     *             have been queued into the driver.
>>>> + * @alloc_devs:        &struct device memory type/allocator-specific per-plane device
>>>>     */
>>>>    /*
>>>>     * Private elements (won't appear at the uAPI book):
>>>> @@ -571,6 +571,8 @@ struct vb2_buf_ops {
>>>>     * @waiting_for_buffers: used in poll() to check if vb2 is still waiting for
>>>>     *             buffers. Only set for capture queues if qbuf has not yet been
>>>>     *             called since poll() needs to return %EPOLLERR in that situation.
>>>> + * @waiting_in_dqbuf: set whenever vb2_queue->lock is released while waiting for
>>>> + *             a buffer to arrive so that -EBUSY can be returned when appropriate
>>>
>>> Appreciate documentation improvements. Thanks!
>>>
>>
>> I haven't been hunting for opportunities to improve the documentation,
>> the opportunity has found me ;P
>>
>>> Just one comment: Could we make it more clear who sets it? For example >      Set by the core for the duration of a blocking DQBUF, when it has
>>> to wait for
>>>       a buffer to become available with vb2_queue->lock released. Used to prevent
>>>       destroying the queue by other threads.
>>
>> Makes sense for me.
>>
>> @Nicolas are you ok with changing the text and retaining your R-b?
>>
>> Andrzej
> 
> Since this patch no longer applies, I think it is best if you make a v2.
> 
> I'll mark this one as "Changes Requested" in patchwork.
> 
> Regards,
> 
> 	Hans
> 
> 

Rebased onto latest media_tree and sent v2.

Regards,

Andrzej
diff mbox series

Patch

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index e41204df19f0..5020e052eda0 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -72,6 +72,10 @@  struct vb2_buffer;
  *		 argument to other ops in this structure.
  * @put_userptr: inform the allocator that a USERPTR buffer will no longer
  *		 be used.
+ * @prepare:	called every time the buffer is passed from userspace to the
+ *		driver, useful for cache synchronisation, optional.
+ * @finish:	called every time the buffer is passed back from the driver
+ *		to the userspace, also optional.
  * @attach_dmabuf: attach a shared &struct dma_buf for a hardware operation;
  *		   used for DMABUF memory types; dev is the alloc device
  *		   dbuf is the shared dma_buf; returns ERR_PTR() on failure;
@@ -86,10 +90,6 @@  struct vb2_buffer;
  *		dmabuf.
  * @unmap_dmabuf: releases access control to the dmabuf - allocator is notified
  *		  that this driver is done using the dmabuf for now.
- * @prepare:	called every time the buffer is passed from userspace to the
- *		driver, useful for cache synchronisation, optional.
- * @finish:	called every time the buffer is passed back from the driver
- *		to the userspace, also optional.
  * @vaddr:	return a kernel virtual address to a given memory buffer
  *		associated with the passed private structure or NULL if no
  *		such mapping exists.
@@ -484,7 +484,6 @@  struct vb2_buf_ops {
  *		caller. For example, for V4L2, it should match
  *		the types defined on &enum v4l2_buf_type.
  * @io_modes:	supported io methods (see &enum vb2_io_modes).
- * @alloc_devs:	&struct device memory type/allocator-specific per-plane device
  * @dev:	device to use for the default allocation context if the driver
  *		doesn't fill in the @alloc_devs array.
  * @dma_attrs:	DMA attributes to use for the DMA.
@@ -550,6 +549,7 @@  struct vb2_buf_ops {
  *		@start_streaming can be called. Used when a DMA engine
  *		cannot be started unless at least this number of buffers
  *		have been queued into the driver.
+ * @alloc_devs:	&struct device memory type/allocator-specific per-plane device
  */
 /*
  * Private elements (won't appear at the uAPI book):
@@ -571,6 +571,8 @@  struct vb2_buf_ops {
  * @waiting_for_buffers: used in poll() to check if vb2 is still waiting for
  *		buffers. Only set for capture queues if qbuf has not yet been
  *		called since poll() needs to return %EPOLLERR in that situation.
+ * @waiting_in_dqbuf: set whenever vb2_queue->lock is released while waiting for
+ *		a buffer to arrive so that -EBUSY can be returned when appropriate
  * @is_multiplanar: set if buffer type is multiplanar
  * @is_output:	set if buffer type is output
  * @copy_timestamp: set if vb2-core should set timestamps