| Message ID | 20240105134020.34312-1-andrzej.p@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: videobuf2: Add missing doc comment for waiting_in_dqbuf | expand |
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
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
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 >
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 >> > >
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 --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
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(-)