[03/12,media] vb2: add in-fence support to QBUF

  From: Gustavo Padovan <gustavo.padovan@collabora.com>

Receive in-fence from userspace and add support for waiting on them
before queueing the buffer to the driver. Buffers are only queued
to the driver once they are ready. A buffer is ready when its
in-fence signals.

v2:
	- fix vb2_queue_or_prepare_buf() ret check
	- remove check for VB2_MEMORY_DMABUF only (Javier)
	- check num of ready buffers to start streaming
	- when queueing, start from the first ready buffer
	- handle queue cancel

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 drivers/media/Kconfig                    |  1 +
 drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
 drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
 include/media/videobuf2-core.h           |  7 ++-
 4 files changed, 99 insertions(+), 21 deletions(-)
  

Hi Gustavo,

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.12-rc5 next-20170616]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Gustavo-Padovan/vb2-add-explicit-fence-user-API/20170618-210740
base:   git://linuxtv.org/media_tree.git master
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org)
   arch/x86/include/asm/uaccess_32.h:1: warning: no structured comments found
   include/linux/init.h:1: warning: no structured comments found
   include/linux/mod_devicetable.h:686: warning: Excess struct/union/enum/typedef member 'ver_major' description in 'fsl_mc_device_id'
   include/linux/mod_devicetable.h:686: warning: Excess struct/union/enum/typedef member 'ver_minor' description in 'fsl_mc_device_id'
   kernel/sched/core.c:2088: warning: No description found for parameter 'rf'
   kernel/sched/core.c:2088: warning: Excess function parameter 'cookie' description in 'try_to_wake_up_local'
   include/linux/kthread.h:26: warning: Excess function parameter '...' description in 'kthread_create'
   kernel/sys.c:1: warning: no structured comments found
   include/linux/device.h:969: warning: No description found for parameter 'dma_ops'
   drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
   include/linux/iio/iio.h:597: warning: No description found for parameter 'trig_readonly'
   include/linux/iio/trigger.h:151: warning: No description found for parameter 'indio_dev'
   include/linux/iio/trigger.h:151: warning: No description found for parameter 'trig'
   include/linux/device.h:970: warning: No description found for parameter 'dma_ops'
   include/linux/usb/gadget.h:230: warning: No description found for parameter 'claimed'
   include/linux/usb/gadget.h:230: warning: No description found for parameter 'enabled'
   include/linux/usb/gadget.h:408: warning: No description found for parameter 'quirk_altset_not_supp'
   include/linux/usb/gadget.h:408: warning: No description found for parameter 'quirk_stall_not_supp'
   include/linux/usb/gadget.h:408: warning: No description found for parameter 'quirk_zlp_not_supp'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'set_busid'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_handler'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_preinstall'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_postinstall'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_uninstall'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'debugfs_init'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_open_object'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_close_object'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'prime_handle_to_fd'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'prime_fd_to_handle'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_export'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_import'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_pin'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_unpin'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_res_obj'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_get_sg_table'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_import_sg_table'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_vmap'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_vunmap'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_mmap'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_vm_ops'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'major'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'minor'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'patchlevel'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'name'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'desc'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'date'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'driver_features'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'ioctls'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'num_ioctls'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'fops'
   include/drm/drm_color_mgmt.h:1: warning: no structured comments found
   drivers/gpu/drm/drm_plane_helper.c:403: warning: No description found for parameter 'ctx'
   drivers/gpu/drm/drm_plane_helper.c:404: warning: No description found for parameter 'ctx'
   drivers/gpu/drm/i915/intel_lpe_audio.c:355: warning: No description found for parameter 'dp_output'
   drivers/gpu/drm/i915/intel_lpe_audio.c:355: warning: No description found for parameter 'link_rate'
   drivers/gpu/drm/i915/intel_lpe_audio.c:356: warning: No description found for parameter 'dp_output'
   drivers/gpu/drm/i915/intel_lpe_audio.c:356: warning: No description found for parameter 'link_rate'
>> include/media/videobuf2-core.h:735: warning: No description found for parameter 'fence'
   Documentation/core-api/assoc_array.rst:13: WARNING: Enumerated list ends without a blank line; unexpected unindent.
   Documentation/doc-guide/sphinx.rst:126: ERROR: Unknown target name: "sphinx c domain".
   kernel/sched/fair.c:7650: WARNING: Inline emphasis start-string without end-string.
   kernel/time/timer.c:1200: ERROR: Unexpected indentation.
   kernel/time/timer.c:1202: ERROR: Unexpected indentation.
   kernel/time/timer.c:1203: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:122: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:125: ERROR: Unexpected indentation.
   include/linux/wait.h:127: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/time/hrtimer.c:990: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/signal.c:322: WARNING: Inline literal start-string without end-string.
   include/linux/iio/iio.h:219: ERROR: Unexpected indentation.
   include/linux/iio/iio.h:220: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/iio/iio.h:226: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/iio/industrialio-core.c:638: ERROR: Unknown target name: "iio_val".
   drivers/iio/industrialio-core.c:645: ERROR: Unknown target name: "iio_val".
   drivers/message/fusion/mptbase.c:5051: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/tty/serial/serial_core.c:1898: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/pci/pci.c:3457: ERROR: Unexpected indentation.
   include/linux/regulator/driver.h:271: ERROR: Unknown target name: "regulator_regmap_x_voltage".
   include/linux/spi/spi.h:370: ERROR: Unexpected indentation.
   drivers/gpu/drm/drm_scdc_helper.c:203: ERROR: Unexpected indentation.
   drivers/gpu/drm/drm_scdc_helper.c:204: WARNING: Block quote ends without a blank line; unexpected unindent.
   drivers/gpu/drm/drm_ioctl.c:690: WARNING: Definition list ends without a blank line; unexpected unindent.
   Documentation/gpu/todo.rst:111: ERROR: Unknown target name: "drm_fb".
   sound/soc/soc-core.c:2670: ERROR: Unknown target name: "snd_soc_daifmt".
   sound/core/jack.c:312: ERROR: Unknown target name: "snd_jack_btn".
   Documentation/userspace-api/unshare.rst:108: WARNING: Inline emphasis start-string without end-string.
   Documentation/usb/typec.rst:: WARNING: document isn't included in any toctree
   Documentation/usb/usb3-debug-port.rst:: WARNING: document isn't included in any toctree
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 43: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 56: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 69: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 82: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 96: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 109: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 122: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 133: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 164: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 193: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 43: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 56: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 69: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 82: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 96: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 109: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 122: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 133: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 164: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 193: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 43: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 56: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 69: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 82: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 96: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 109: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 122: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 133: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 164: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 193: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 43: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 56: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 69: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 82: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 96: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 109: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 122: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 133: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 164: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 193: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 43: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 56: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 69: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 82: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 96: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 109: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 122: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 133: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 164: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 193: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 43: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 56: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 69: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 82: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 96: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 109: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 122: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 133: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 164: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 193: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 43: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 56: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 69: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 82: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 96: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 109: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 122: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 133: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 164: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 193: Having multiple values in <test> isn't supported and may not work as expected

vim +/fence +735 include/media/videobuf2-core.h

   719	 *		in driver
   720	 *
   721	 * Should be called from vidioc_qbuf ioctl handler of a driver.
   722	 * The passed buffer should have been verified.
   723	 *
   724	 * This function:
   725	 *
   726	 * #) if necessary, calls buf_prepare callback in the driver (if provided), in
   727	 *    which driver-specific buffer initialization can be performed,
   728	 * #) if streaming is on, queues the buffer in driver by the means of
   729	 *    &vb2_ops->buf_queue callback for processing.
   730	 *
   731	 * The return values from this function are intended to be directly returned
   732	 * from vidioc_qbuf handler in driver.
   733	 */
   734	int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
 > 735			  struct dma_fence *fence);
   736	
   737	/**
   738	 * vb2_core_dqbuf() - Dequeue a buffer to the userspace
   739	 * @q:		videobuf2 queue
   740	 * @pindex:	pointer to the buffer index. May be NULL
   741	 * @pb:		buffer structure passed from userspace to vidioc_dqbuf handler
   742	 *		in driver
   743	 * @nonblocking: if true, this call will not sleep waiting for a buffer if no

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
  

Em Fri, 16 Jun 2017 16:39:06 +0900
Gustavo Padovan <gustavo@padovan.org> escreveu:

> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> Receive in-fence from userspace and add support for waiting on them
> before queueing the buffer to the driver. Buffers are only queued
> to the driver once they are ready. A buffer is ready when its
> in-fence signals.
> 
> v2:
> 	- fix vb2_queue_or_prepare_buf() ret check
> 	- remove check for VB2_MEMORY_DMABUF only (Javier)
> 	- check num of ready buffers to start streaming
> 	- when queueing, start from the first ready buffer
> 	- handle queue cancel
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  drivers/media/Kconfig                    |  1 +
>  drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
>  include/media/videobuf2-core.h           |  7 ++-
>  4 files changed, 99 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> index 55d9c2b..3cd1d3d 100644
> --- a/drivers/media/Kconfig
> +++ b/drivers/media/Kconfig
> @@ -11,6 +11,7 @@ config CEC_NOTIFIER
>  menuconfig MEDIA_SUPPORT
>  	tristate "Multimedia support"
>  	depends on HAS_IOMEM
> +	select SYNC_FILE
>  	help
>  	  If you want to use Webcams, Video grabber devices and/or TV devices
>  	  enable this option and other options below.
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index ea83126..29aa9d4 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1279,6 +1279,22 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
>  	return 0;
>  }
>  
> +static int __get_num_ready_buffers(struct vb2_queue *q)
> +{
> +	struct vb2_buffer *vb;
> +	int ready_count = 0;
> +
> +	/* count num of buffers ready in front of the queued_list */
> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> +			break;
> +
> +		ready_count++;

Hmm... maybe that's one of the reasons why out of order explicit fences is not
working. With the current logic, if explicit fences is enabled, this function
will always return 0 or 1, even if more buffers are ready.

IMHO, the correct logic here should be, instead:

		if (!vb->in_fence || dma_fence_is_signaled(vb->in_fence))
			ready_count++;

> +	}
> +
> +	return ready_count;
> +}
> +
>  int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
>  {
>  	struct vb2_buffer *vb;
> @@ -1324,8 +1340,15 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  	 * If any buffers were queued before streamon,
>  	 * we can now pass them to driver for processing.
>  	 */
> -	list_for_each_entry(vb, &q->queued_list, queued_entry)
> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +		if (vb->state != VB2_BUF_STATE_QUEUED)
> +			continue;
> +
> +		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> +			break;
> +
>  		__enqueue_in_driver(vb);

Same as before, the correct logic here seems to be:

		if (!vb->in_fence || dma_fence_is_signaled(vb->in_fence))
			__enqueue_in_driver(vb);	

> +	}
>  
>  	/* Tell the driver to start streaming */
>  	q->start_streaming_called = 1;
> @@ -1369,33 +1392,55 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  
>  static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
>  {
> +	struct vb2_buffer *b;
>  	int ret;
>  
>  	/*
>  	 * If already streaming, give the buffer to driver for processing.
>  	 * If not, the buffer will be given to driver on next streamon.
>  	 */
> -	if (q->start_streaming_called)
> -		__enqueue_in_driver(vb);
>  
> -	/*
> -	 * If streamon has been called, and we haven't yet called
> -	 * start_streaming() since not enough buffers were queued, and
> -	 * we now have reached the minimum number of queued buffers,
> -	 * then we can finally call start_streaming().
> -	 */
> -	if (q->streaming && !q->start_streaming_called &&
> -	    q->queued_count >= q->min_buffers_needed) {
> -		ret = vb2_start_streaming(q);
> -		if (ret)
> -			return ret;
> +	if (q->start_streaming_called) {
> +		list_for_each_entry(b, &q->queued_list, queued_entry) {
> +			if (b->state != VB2_BUF_STATE_QUEUED)
> +				continue;
> +
> +			if (b->in_fence && !dma_fence_is_signaled(b->in_fence))
> +				break;
> +
> +			__enqueue_in_driver(b);

Same here:

		if (!vb->in_fence || dma_fence_is_signaled(vb->in_fence))
			__enqueue_in_driver(vb);	


There is, however, a behavior change here (even without the above
proposal.

Before this patch, if fences is not used (for example, for DVB or
for some other mechanism at V4L2), the driver would be doing:

	if (q->start_streaming_called)
		__enqueue_in_driver(vb);

So, __enqueue_in_driver() would be called just once. Now, after
the change, it will be doing, instead:

	list_for_each_entry(vb, &q->queued_list, queued_entry)
		__enqueue_in_driver(vb);

With can queue several buffers at once.

I've no idea how this will affect non-fences behavior for V4L2
and for DVB. More tests are required to check if this badly affect
drivers, or if it would bring some performance or latency change.

> +		}
> +	} else {

Why did you add an else here? I guess there was a very strong reason
to not have an else at the original code - just can't remember why, and
I' too lazy today to dig into VB2 changeset descriptions :-)

Please don't change the behavior together with a patch that add new
features as it can cause regressions, and it would be a way harder
to track if you fold different changes at the same patch.

If this behavior change is due to some bug, it should be submitted
in a separate, patch, provided with a very detailed description.

If otherwise, this is a requiement just for fences, you should first
test if fences is enabled before changing the behavior.

> +		/*
> +		 * If streamon has been called, and we haven't yet called
> +		 * start_streaming() since not enough buffers were queued, and
> +		 * we now have reached the minimum number of queued buffers
> +		 * that are ready, then we can finally call start_streaming().
> +		 */
> +		if (q->streaming &&
> +		    __get_num_ready_buffers(q) >= q->min_buffers_needed) {
> +			ret = vb2_start_streaming(q);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
>  	return 0;
>  }
>  
> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> +static void vb2_qbuf_fence_cb(struct dma_fence *f, struct dma_fence_cb *cb)
> +{
> +	struct vb2_buffer *vb = container_of(cb, struct vb2_buffer, fence_cb);
> +
> +	dma_fence_put(vb->in_fence);
> +	vb->in_fence = NULL;
> +
> +	__vb2_core_qbuf(vb, vb->vb2_queue);
> +}
> +
> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> +		  struct dma_fence *fence)
>  {
>  	struct vb2_buffer *vb;
>  	int ret;
> @@ -1436,6 +1481,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  	if (pb)
>  		call_void_bufop(q, fill_user_buffer, vb, pb);
>  
> +	vb->in_fence = fence;
> +	if (fence && !dma_fence_add_callback(fence, &vb->fence_cb,
> +					     vb2_qbuf_fence_cb))
> +		return 0;

Maybe we should provide some error or debug log here or a WARN_ON(), if 
dma_fence_add_callback() fails instead of silently ignore any errors.

> +
>  	return __vb2_core_qbuf(vb, q);
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_qbuf);
> @@ -1647,6 +1697,7 @@ EXPORT_SYMBOL_GPL(vb2_core_dqbuf);
>  static void __vb2_queue_cancel(struct vb2_queue *q)
>  {
>  	unsigned int i;
> +	struct vb2_buffer *vb;
>  
>  	/*
>  	 * Tell driver to stop all transactions and release all queued
> @@ -1669,6 +1720,14 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>  		WARN_ON(atomic_read(&q->owned_by_drv_count));
>  	}
>  
> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +		if (vb->in_fence) {
> +			dma_fence_remove_callback(vb->in_fence, &vb->fence_cb);
> +			dma_fence_put(vb->in_fence);
> +			vb->in_fence = NULL;
> +		}
> +	}
> +
>  	q->streaming = 0;
>  	q->start_streaming_called = 0;
>  	q->queued_count = 0;
> @@ -1735,7 +1794,7 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type)
>  	 * Tell driver to start streaming provided sufficient buffers
>  	 * are available.
>  	 */
> -	if (q->queued_count >= q->min_buffers_needed) {
> +	if (__get_num_ready_buffers(q) >= q->min_buffers_needed) {
>  		ret = v4l_vb2q_enable_media_source(q);
>  		if (ret)
>  			return ret;
> @@ -2250,7 +2309,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
>  		 * Queue all buffers.
>  		 */
>  		for (i = 0; i < q->num_buffers; i++) {
> -			ret = vb2_core_qbuf(q, i, NULL);
> +			ret = vb2_core_qbuf(q, i, NULL, NULL);
>  			if (ret)
>  				goto err_reqbufs;
>  			fileio->bufs[i].queued = 1;
> @@ -2429,7 +2488,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>  
>  		if (copy_timestamp)
>  			b->timestamp = ktime_get_ns();
> -		ret = vb2_core_qbuf(q, index, NULL);
> +		ret = vb2_core_qbuf(q, index, NULL, NULL);
>  		dprintk(5, "vb2_dbuf result: %d\n", ret);
>  		if (ret)
>  			return ret;
> @@ -2532,7 +2591,7 @@ static int vb2_thread(void *data)
>  		if (copy_timestamp)
>  			vb->timestamp = ktime_get_ns();;
>  		if (!threadio->stop)
> -			ret = vb2_core_qbuf(q, vb->index, NULL);
> +			ret = vb2_core_qbuf(q, vb->index, NULL, NULL);
>  		call_void_qop(q, wait_prepare, q);
>  		if (ret || threadio->stop)
>  			break;
> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> index 110fb45..e6ad77f 100644
> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> @@ -23,6 +23,7 @@
>  #include <linux/sched.h>
>  #include <linux/freezer.h>
>  #include <linux/kthread.h>
> +#include <linux/sync_file.h>
>  
>  #include <media/v4l2-dev.h>
>  #include <media/v4l2-fh.h>
> @@ -560,6 +561,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
>  
>  int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  {
> +	struct dma_fence *fence = NULL;
>  	int ret;
>  
>  	if (vb2_fileio_is_active(q)) {
> @@ -568,7 +570,18 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  	}
>  
>  	ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> -	return ret ? ret : vb2_core_qbuf(q, b->index, b);
> +	if (ret)
> +		return ret;
> +
> +	if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
> +		fence = sync_file_get_fence(b->fence_fd);
> +		if (!fence) {
> +			dprintk(1, "failed to get in-fence from fd\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);

No need to check for ret again. You could just do:

	return vb2_core_qbuf(q, b->index, b, fence);

>  }
>  EXPORT_SYMBOL_GPL(vb2_qbuf);
>  
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index cb97c22..aa43e43 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -16,6 +16,7 @@
>  #include <linux/mutex.h>
>  #include <linux/poll.h>
>  #include <linux/dma-buf.h>
> +#include <linux/dma-fence.h>
>  
>  #define VB2_MAX_FRAME	(32)
>  #define VB2_MAX_PLANES	(8)
> @@ -259,6 +260,9 @@ struct vb2_buffer {
>  
>  	struct list_head	queued_entry;
>  	struct list_head	done_entry;
> +
> +	struct dma_fence	*in_fence;
> +	struct dma_fence_cb	fence_cb;
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  	/*
>  	 * Counters for how often these buffer-related ops are
> @@ -727,7 +731,8 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
>   * The return values from this function are intended to be directly returned
>   * from vidioc_qbuf handler in driver.
>   */
> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb);
> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> +		  struct dma_fence *fence);
>  
>  /**
>   * vb2_core_dqbuf() - Dequeue a buffer to the userspace
  

Hi Mauro,

2017-06-30 Mauro Carvalho Chehab <mchehab@osg.samsung.com>:

> Em Fri, 16 Jun 2017 16:39:06 +0900
> Gustavo Padovan <gustavo@padovan.org> escreveu:
> 
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > 
> > Receive in-fence from userspace and add support for waiting on them
> > before queueing the buffer to the driver. Buffers are only queued
> > to the driver once they are ready. A buffer is ready when its
> > in-fence signals.
> > 
> > v2:
> > 	- fix vb2_queue_or_prepare_buf() ret check
> > 	- remove check for VB2_MEMORY_DMABUF only (Javier)
> > 	- check num of ready buffers to start streaming
> > 	- when queueing, start from the first ready buffer
> > 	- handle queue cancel
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> > ---
> >  drivers/media/Kconfig                    |  1 +
> >  drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
> >  drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
> >  include/media/videobuf2-core.h           |  7 ++-
> >  4 files changed, 99 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> > index 55d9c2b..3cd1d3d 100644
> > --- a/drivers/media/Kconfig
> > +++ b/drivers/media/Kconfig
> > @@ -11,6 +11,7 @@ config CEC_NOTIFIER
> >  menuconfig MEDIA_SUPPORT
> >  	tristate "Multimedia support"
> >  	depends on HAS_IOMEM
> > +	select SYNC_FILE
> >  	help
> >  	  If you want to use Webcams, Video grabber devices and/or TV devices
> >  	  enable this option and other options below.
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index ea83126..29aa9d4 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -1279,6 +1279,22 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
> >  	return 0;
> >  }
> >  
> > +static int __get_num_ready_buffers(struct vb2_queue *q)
> > +{
> > +	struct vb2_buffer *vb;
> > +	int ready_count = 0;
> > +
> > +	/* count num of buffers ready in front of the queued_list */
> > +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> > +		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> > +			break;
> > +
> > +		ready_count++;
> 
> Hmm... maybe that's one of the reasons why out of order explicit fences is not
> working. With the current logic, if explicit fences is enabled, this function
> will always return 0 or 1, even if more buffers are ready.
> 
> IMHO, the correct logic here should be, instead:
> 
> 		if (!vb->in_fence || dma_fence_is_signaled(vb->in_fence))
> 			ready_count++;

If we do like you propose then we will be getting the wrong ready_count
and queue buffers unordered (in the next two places this code snipet
appears.

In this function I want to know how many ready buffers are in the front
of the buffer. A buffer will be ready if:

* it has no fence
* it has a fence but it was signaled already

Then we start walking the queue looking for such buffers in order and
stop as soon as we find a buffer that doesn't meet these criteria. Hence
the 'break' command to interrupt the loop. If we don't break we may
queue the next buffer on the queue (if it matches the criteria) without
queueing the current one. So we really need break out from the loop
here.

> 
> > +	}
> > +
> > +	return ready_count;
> > +}
> > +
> >  int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
> >  {
> >  	struct vb2_buffer *vb;
> > @@ -1324,8 +1340,15 @@ static int vb2_start_streaming(struct vb2_queue *q)
> >  	 * If any buffers were queued before streamon,
> >  	 * we can now pass them to driver for processing.
> >  	 */
> > -	list_for_each_entry(vb, &q->queued_list, queued_entry)
> > +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> > +		if (vb->state != VB2_BUF_STATE_QUEUED)
> > +			continue;
> > +
> > +		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> > +			break;
> > +
> >  		__enqueue_in_driver(vb);
> 
> Same as before, the correct logic here seems to be:
> 
> 		if (!vb->in_fence || dma_fence_is_signaled(vb->in_fence))
> 			__enqueue_in_driver(vb);	
> 
> > +	}
> >  
> >  	/* Tell the driver to start streaming */
> >  	q->start_streaming_called = 1;
> > @@ -1369,33 +1392,55 @@ static int vb2_start_streaming(struct vb2_queue *q)
> >  
> >  static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
> >  {
> > +	struct vb2_buffer *b;
> >  	int ret;
> >  
> >  	/*
> >  	 * If already streaming, give the buffer to driver for processing.
> >  	 * If not, the buffer will be given to driver on next streamon.
> >  	 */
> > -	if (q->start_streaming_called)
> > -		__enqueue_in_driver(vb);
> >  
> > -	/*
> > -	 * If streamon has been called, and we haven't yet called
> > -	 * start_streaming() since not enough buffers were queued, and
> > -	 * we now have reached the minimum number of queued buffers,
> > -	 * then we can finally call start_streaming().
> > -	 */
> > -	if (q->streaming && !q->start_streaming_called &&
> > -	    q->queued_count >= q->min_buffers_needed) {
> > -		ret = vb2_start_streaming(q);
> > -		if (ret)
> > -			return ret;
> > +	if (q->start_streaming_called) {
> > +		list_for_each_entry(b, &q->queued_list, queued_entry) {
> > +			if (b->state != VB2_BUF_STATE_QUEUED)
> > +				continue;
> > +
> > +			if (b->in_fence && !dma_fence_is_signaled(b->in_fence))
> > +				break;
> > +
> > +			__enqueue_in_driver(b);
> 
> Same here:
> 
> 		if (!vb->in_fence || dma_fence_is_signaled(vb->in_fence))
> 			__enqueue_in_driver(vb);	
> 
> 
> There is, however, a behavior change here (even without the above
> proposal.
> 
> Before this patch, if fences is not used (for example, for DVB or
> for some other mechanism at V4L2), the driver would be doing:
> 
> 	if (q->start_streaming_called)
> 		__enqueue_in_driver(vb);
> 
> So, __enqueue_in_driver() would be called just once. Now, after
> the change, it will be doing, instead:
> 
> 	list_for_each_entry(vb, &q->queued_list, queued_entry)
> 		__enqueue_in_driver(vb);
> 
> With can queue several buffers at once.
> 
> I've no idea how this will affect non-fences behavior for V4L2
> and for DVB. More tests are required to check if this badly affect
> drivers, or if it would bring some performance or latency change.

I don't believe it affects the behaviour at all, without fences the
queue will never have more than one buffer in the queue. The queue will
only have more buffers on the queue when fences are used and fences for
the buffers in front of the queue didn't signal yet.

> 
> > +		}
> > +	} else {
> 
> Why did you add an else here? I guess there was a very strong reason
> to not have an else at the original code - just can't remember why, and
> I' too lazy today to dig into VB2 changeset descriptions :-)
> 
> Please don't change the behavior together with a patch that add new
> features as it can cause regressions, and it would be a way harder
> to track if you fold different changes at the same patch.
> 
> If this behavior change is due to some bug, it should be submitted
> in a separate, patch, provided with a very detailed description.
> 
> If otherwise, this is a requiement just for fences, you should first
> test if fences is enabled before changing the behavior.

Rigth. This should have been put in a separated patch, but it is not
changing the behaviour at all. The 'if' above this one was

	if (q->start_streaming_called)


and the 'if' below had a !q->start_streaming_called condition, so it
was a sort of else already.

> 
> > +		/*
> > +		 * If streamon has been called, and we haven't yet called
> > +		 * start_streaming() since not enough buffers were queued, and
> > +		 * we now have reached the minimum number of queued buffers
> > +		 * that are ready, then we can finally call start_streaming().
> > +		 */
> > +		if (q->streaming &&
> > +		    __get_num_ready_buffers(q) >= q->min_buffers_needed) {
> > +			ret = vb2_start_streaming(q);
> > +			if (ret)
> > +				return ret;
> > +		}
> >  	}
> >  
> >  	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
> >  	return 0;
> >  }
> >  
> > -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> > +static void vb2_qbuf_fence_cb(struct dma_fence *f, struct dma_fence_cb *cb)
> > +{
> > +	struct vb2_buffer *vb = container_of(cb, struct vb2_buffer, fence_cb);
> > +
> > +	dma_fence_put(vb->in_fence);
> > +	vb->in_fence = NULL;
> > +
> > +	__vb2_core_qbuf(vb, vb->vb2_queue);
> > +}
> > +
> > +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> > +		  struct dma_fence *fence)
> >  {
> >  	struct vb2_buffer *vb;
> >  	int ret;
> > @@ -1436,6 +1481,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> >  	if (pb)
> >  		call_void_bufop(q, fill_user_buffer, vb, pb);
> >  
> > +	vb->in_fence = fence;
> > +	if (fence && !dma_fence_add_callback(fence, &vb->fence_cb,
> > +					     vb2_qbuf_fence_cb))
> > +		return 0;
> 
> Maybe we should provide some error or debug log here or a WARN_ON(), if 
> dma_fence_add_callback() fails instead of silently ignore any errors.

This is not an error. If the if succeeds it mean we have installed a
callback for the fence. If not, it means the fence signaled already and
we don't can call __vb2_core_qbuf right away.

> 
> > +
> >  	return __vb2_core_qbuf(vb, q);
> >  }
> >  EXPORT_SYMBOL_GPL(vb2_core_qbuf);
> > @@ -1647,6 +1697,7 @@ EXPORT_SYMBOL_GPL(vb2_core_dqbuf);
> >  static void __vb2_queue_cancel(struct vb2_queue *q)
> >  {
> >  	unsigned int i;
> > +	struct vb2_buffer *vb;
> >  
> >  	/*
> >  	 * Tell driver to stop all transactions and release all queued
> > @@ -1669,6 +1720,14 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> >  		WARN_ON(atomic_read(&q->owned_by_drv_count));
> >  	}
> >  
> > +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> > +		if (vb->in_fence) {
> > +			dma_fence_remove_callback(vb->in_fence, &vb->fence_cb);
> > +			dma_fence_put(vb->in_fence);
> > +			vb->in_fence = NULL;
> > +		}
> > +	}
> > +
> >  	q->streaming = 0;
> >  	q->start_streaming_called = 0;
> >  	q->queued_count = 0;
> > @@ -1735,7 +1794,7 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type)
> >  	 * Tell driver to start streaming provided sufficient buffers
> >  	 * are available.
> >  	 */
> > -	if (q->queued_count >= q->min_buffers_needed) {
> > +	if (__get_num_ready_buffers(q) >= q->min_buffers_needed) {
> >  		ret = v4l_vb2q_enable_media_source(q);
> >  		if (ret)
> >  			return ret;
> > @@ -2250,7 +2309,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> >  		 * Queue all buffers.
> >  		 */
> >  		for (i = 0; i < q->num_buffers; i++) {
> > -			ret = vb2_core_qbuf(q, i, NULL);
> > +			ret = vb2_core_qbuf(q, i, NULL, NULL);
> >  			if (ret)
> >  				goto err_reqbufs;
> >  			fileio->bufs[i].queued = 1;
> > @@ -2429,7 +2488,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
> >  
> >  		if (copy_timestamp)
> >  			b->timestamp = ktime_get_ns();
> > -		ret = vb2_core_qbuf(q, index, NULL);
> > +		ret = vb2_core_qbuf(q, index, NULL, NULL);
> >  		dprintk(5, "vb2_dbuf result: %d\n", ret);
> >  		if (ret)
> >  			return ret;
> > @@ -2532,7 +2591,7 @@ static int vb2_thread(void *data)
> >  		if (copy_timestamp)
> >  			vb->timestamp = ktime_get_ns();;
> >  		if (!threadio->stop)
> > -			ret = vb2_core_qbuf(q, vb->index, NULL);
> > +			ret = vb2_core_qbuf(q, vb->index, NULL, NULL);
> >  		call_void_qop(q, wait_prepare, q);
> >  		if (ret || threadio->stop)
> >  			break;
> > diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > index 110fb45..e6ad77f 100644
> > --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> > +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > @@ -23,6 +23,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/freezer.h>
> >  #include <linux/kthread.h>
> > +#include <linux/sync_file.h>
> >  
> >  #include <media/v4l2-dev.h>
> >  #include <media/v4l2-fh.h>
> > @@ -560,6 +561,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
> >  
> >  int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >  {
> > +	struct dma_fence *fence = NULL;
> >  	int ret;
> >  
> >  	if (vb2_fileio_is_active(q)) {
> > @@ -568,7 +570,18 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >  	}
> >  
> >  	ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> > -	return ret ? ret : vb2_core_qbuf(q, b->index, b);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
> > +		fence = sync_file_get_fence(b->fence_fd);
> > +		if (!fence) {
> > +			dprintk(1, "failed to get in-fence from fd\n");
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
> 
> No need to check for ret again. You could just do:
> 
> 	return vb2_core_qbuf(q, b->index, b, fence);

Sure.

Gustavo
  

On 06/16/17 09:39, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> Receive in-fence from userspace and add support for waiting on them
> before queueing the buffer to the driver. Buffers are only queued
> to the driver once they are ready. A buffer is ready when its
> in-fence signals.
> 
> v2:
> 	- fix vb2_queue_or_prepare_buf() ret check
> 	- remove check for VB2_MEMORY_DMABUF only (Javier)
> 	- check num of ready buffers to start streaming
> 	- when queueing, start from the first ready buffer
> 	- handle queue cancel
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  drivers/media/Kconfig                    |  1 +
>  drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
>  include/media/videobuf2-core.h           |  7 ++-
>  4 files changed, 99 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> index 55d9c2b..3cd1d3d 100644
> --- a/drivers/media/Kconfig
> +++ b/drivers/media/Kconfig
> @@ -11,6 +11,7 @@ config CEC_NOTIFIER
>  menuconfig MEDIA_SUPPORT
>  	tristate "Multimedia support"
>  	depends on HAS_IOMEM
> +	select SYNC_FILE

Is this the right place for this? Shouldn't this be selected in
'config VIDEOBUF2_CORE'?

Fences are specific to vb2 after all.

>  	help
>  	  If you want to use Webcams, Video grabber devices and/or TV devices
>  	  enable this option and other options below.
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index ea83126..29aa9d4 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1279,6 +1279,22 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
>  	return 0;
>  }
>  
> +static int __get_num_ready_buffers(struct vb2_queue *q)
> +{
> +	struct vb2_buffer *vb;
> +	int ready_count = 0;
> +
> +	/* count num of buffers ready in front of the queued_list */
> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> +			break;

Obviously the break is wrong as Mauro mentioned.

> +
> +		ready_count++;
> +	}
> +
> +	return ready_count;
> +}
> +
>  int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
>  {
>  	struct vb2_buffer *vb;
> @@ -1324,8 +1340,15 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  	 * If any buffers were queued before streamon,
>  	 * we can now pass them to driver for processing.
>  	 */
> -	list_for_each_entry(vb, &q->queued_list, queued_entry)
> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +		if (vb->state != VB2_BUF_STATE_QUEUED)
> +			continue;

I think this test is unnecessary.

> +
> +		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> +			break;
> +
>  		__enqueue_in_driver(vb);

I would move the above test (after fixing it as Mauro said) to __enqueue_in_driver.
I.e. if this is waiting for a fence then __enqueue_in_driver does nothing.

> +	}
>  
>  	/* Tell the driver to start streaming */
>  	q->start_streaming_called = 1;
> @@ -1369,33 +1392,55 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  
>  static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
>  {
> +	struct vb2_buffer *b;
>  	int ret;
>  
>  	/*
>  	 * If already streaming, give the buffer to driver for processing.
>  	 * If not, the buffer will be given to driver on next streamon.
>  	 */
> -	if (q->start_streaming_called)
> -		__enqueue_in_driver(vb);
>  
> -	/*
> -	 * If streamon has been called, and we haven't yet called
> -	 * start_streaming() since not enough buffers were queued, and
> -	 * we now have reached the minimum number of queued buffers,
> -	 * then we can finally call start_streaming().
> -	 */
> -	if (q->streaming && !q->start_streaming_called &&
> -	    q->queued_count >= q->min_buffers_needed) {
> -		ret = vb2_start_streaming(q);
> -		if (ret)
> -			return ret;
> +	if (q->start_streaming_called) {
> +		list_for_each_entry(b, &q->queued_list, queued_entry) {
> +			if (b->state != VB2_BUF_STATE_QUEUED)
> +				continue;
> +
> +			if (b->in_fence && !dma_fence_is_signaled(b->in_fence))
> +				break;

Again, if this test is in __enqueue_in_driver, then you can keep the
original code. Why would you need to loop over all buffers anyway?

If a fence is ready then the callback will call this function for that
buffer. Everything works fine AFAICT without looping over buffers here.

> +
> +			__enqueue_in_driver(b);
> +		}
> +	} else {
> +		/*
> +		 * If streamon has been called, and we haven't yet called
> +		 * start_streaming() since not enough buffers were queued, and
> +		 * we now have reached the minimum number of queued buffers
> +		 * that are ready, then we can finally call start_streaming().
> +		 */
> +		if (q->streaming &&
> +		    __get_num_ready_buffers(q) >= q->min_buffers_needed) {

Just combine this with the 'else' to an 'else if'. Saves an extra level of
indentation.

To follow-up from Mauro's comment: having an 'else if' here is fine.

> +			ret = vb2_start_streaming(q);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
>  	return 0;
>  }
>  
> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> +static void vb2_qbuf_fence_cb(struct dma_fence *f, struct dma_fence_cb *cb)
> +{
> +	struct vb2_buffer *vb = container_of(cb, struct vb2_buffer, fence_cb);
> +
> +	dma_fence_put(vb->in_fence);
> +	vb->in_fence = NULL;
> +
> +	__vb2_core_qbuf(vb, vb->vb2_queue);
> +}
> +
> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> +		  struct dma_fence *fence)
>  {
>  	struct vb2_buffer *vb;
>  	int ret;
> @@ -1436,6 +1481,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  	if (pb)
>  		call_void_bufop(q, fill_user_buffer, vb, pb);
>  
> +	vb->in_fence = fence;
> +	if (fence && !dma_fence_add_callback(fence, &vb->fence_cb,
> +					     vb2_qbuf_fence_cb))
> +		return 0;

Shouldn't this return an error? How would userspace know this failed?

> +
>  	return __vb2_core_qbuf(vb, q);
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_qbuf);
> @@ -1647,6 +1697,7 @@ EXPORT_SYMBOL_GPL(vb2_core_dqbuf);
>  static void __vb2_queue_cancel(struct vb2_queue *q)
>  {
>  	unsigned int i;
> +	struct vb2_buffer *vb;
>  
>  	/*
>  	 * Tell driver to stop all transactions and release all queued
> @@ -1669,6 +1720,14 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>  		WARN_ON(atomic_read(&q->owned_by_drv_count));
>  	}
>  
> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +		if (vb->in_fence) {
> +			dma_fence_remove_callback(vb->in_fence, &vb->fence_cb);
> +			dma_fence_put(vb->in_fence);
> +			vb->in_fence = NULL;
> +		}
> +	}
> +
>  	q->streaming = 0;
>  	q->start_streaming_called = 0;
>  	q->queued_count = 0;
> @@ -1735,7 +1794,7 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type)
>  	 * Tell driver to start streaming provided sufficient buffers
>  	 * are available.
>  	 */
> -	if (q->queued_count >= q->min_buffers_needed) {
> +	if (__get_num_ready_buffers(q) >= q->min_buffers_needed) {
>  		ret = v4l_vb2q_enable_media_source(q);
>  		if (ret)
>  			return ret;
> @@ -2250,7 +2309,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
>  		 * Queue all buffers.
>  		 */
>  		for (i = 0; i < q->num_buffers; i++) {
> -			ret = vb2_core_qbuf(q, i, NULL);
> +			ret = vb2_core_qbuf(q, i, NULL, NULL);
>  			if (ret)
>  				goto err_reqbufs;
>  			fileio->bufs[i].queued = 1;
> @@ -2429,7 +2488,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>  
>  		if (copy_timestamp)
>  			b->timestamp = ktime_get_ns();
> -		ret = vb2_core_qbuf(q, index, NULL);
> +		ret = vb2_core_qbuf(q, index, NULL, NULL);
>  		dprintk(5, "vb2_dbuf result: %d\n", ret);
>  		if (ret)
>  			return ret;
> @@ -2532,7 +2591,7 @@ static int vb2_thread(void *data)
>  		if (copy_timestamp)
>  			vb->timestamp = ktime_get_ns();;
>  		if (!threadio->stop)
> -			ret = vb2_core_qbuf(q, vb->index, NULL);
> +			ret = vb2_core_qbuf(q, vb->index, NULL, NULL);
>  		call_void_qop(q, wait_prepare, q);
>  		if (ret || threadio->stop)
>  			break;
> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> index 110fb45..e6ad77f 100644
> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> @@ -23,6 +23,7 @@
>  #include <linux/sched.h>
>  #include <linux/freezer.h>
>  #include <linux/kthread.h>
> +#include <linux/sync_file.h>
>  
>  #include <media/v4l2-dev.h>
>  #include <media/v4l2-fh.h>
> @@ -560,6 +561,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
>  
>  int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  {
> +	struct dma_fence *fence = NULL;
>  	int ret;
>  
>  	if (vb2_fileio_is_active(q)) {
> @@ -568,7 +570,18 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  	}
>  
>  	ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> -	return ret ? ret : vb2_core_qbuf(q, b->index, b);
> +	if (ret)
> +		return ret;
> +
> +	if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
> +		fence = sync_file_get_fence(b->fence_fd);
> +		if (!fence) {
> +			dprintk(1, "failed to get in-fence from fd\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
>  }
>  EXPORT_SYMBOL_GPL(vb2_qbuf);
>  
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index cb97c22..aa43e43 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -16,6 +16,7 @@
>  #include <linux/mutex.h>
>  #include <linux/poll.h>
>  #include <linux/dma-buf.h>
> +#include <linux/dma-fence.h>
>  
>  #define VB2_MAX_FRAME	(32)
>  #define VB2_MAX_PLANES	(8)
> @@ -259,6 +260,9 @@ struct vb2_buffer {
>  
>  	struct list_head	queued_entry;
>  	struct list_head	done_entry;
> +
> +	struct dma_fence	*in_fence;
> +	struct dma_fence_cb	fence_cb;
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  	/*
>  	 * Counters for how often these buffer-related ops are
> @@ -727,7 +731,8 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
>   * The return values from this function are intended to be directly returned
>   * from vidioc_qbuf handler in driver.
>   */
> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb);
> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> +		  struct dma_fence *fence);
>  
>  /**
>   * vb2_core_dqbuf() - Dequeue a buffer to the userspace
> 

Regards,

	Hans
  

On 06/16/17 09:39, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> Receive in-fence from userspace and add support for waiting on them
> before queueing the buffer to the driver. Buffers are only queued
> to the driver once they are ready. A buffer is ready when its
> in-fence signals.
> 
> v2:
> 	- fix vb2_queue_or_prepare_buf() ret check
> 	- remove check for VB2_MEMORY_DMABUF only (Javier)
> 	- check num of ready buffers to start streaming
> 	- when queueing, start from the first ready buffer
> 	- handle queue cancel
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  drivers/media/Kconfig                    |  1 +
>  drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
>  include/media/videobuf2-core.h           |  7 ++-
>  4 files changed, 99 insertions(+), 21 deletions(-)
> 

<snip>

> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> index 110fb45..e6ad77f 100644
> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> @@ -23,6 +23,7 @@
>  #include <linux/sched.h>
>  #include <linux/freezer.h>
>  #include <linux/kthread.h>
> +#include <linux/sync_file.h>
>  
>  #include <media/v4l2-dev.h>
>  #include <media/v4l2-fh.h>
> @@ -560,6 +561,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
>  
>  int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  {
> +	struct dma_fence *fence = NULL;
>  	int ret;
>  
>  	if (vb2_fileio_is_active(q)) {
> @@ -568,7 +570,18 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  	}
>  
>  	ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> -	return ret ? ret : vb2_core_qbuf(q, b->index, b);
> +	if (ret)
> +		return ret;
> +
> +	if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
> +		fence = sync_file_get_fence(b->fence_fd);
> +		if (!fence) {
> +			dprintk(1, "failed to get in-fence from fd\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
>  }
>  EXPORT_SYMBOL_GPL(vb2_qbuf);

You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
if there is a fence pending. It should also fill in fence_fd.

Regards,

	Hans
  

On 07/03/17 20:16, Gustavo Padovan wrote:
>>> @@ -1436,6 +1481,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>>>  	if (pb)
>>>  		call_void_bufop(q, fill_user_buffer, vb, pb);
>>>  
>>> +	vb->in_fence = fence;
>>> +	if (fence && !dma_fence_add_callback(fence, &vb->fence_cb,
>>> +					     vb2_qbuf_fence_cb))
>>> +		return 0;
>>
>> Maybe we should provide some error or debug log here or a WARN_ON(), if 
>> dma_fence_add_callback() fails instead of silently ignore any errors.
> 
> This is not an error. If the if succeeds it mean we have installed a
> callback for the fence. If not, it means the fence signaled already and
> we don't can call __vb2_core_qbuf right away.

I had the same question as Mauro. After looking at the dma_fence_add_callback
code I see what you mean, but a comment would certainly be helpful.

Also, should you set vb->in_fence to NULL if the fence signaled already?
I'm not sure if you need to call 'dma_fence_put(vb->in_fence);' as well.
You would know that better than I do.

Regards,

	Hans
  

2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:

> On 07/03/17 20:16, Gustavo Padovan wrote:
> >>> @@ -1436,6 +1481,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> >>>  	if (pb)
> >>>  		call_void_bufop(q, fill_user_buffer, vb, pb);
> >>>  
> >>> +	vb->in_fence = fence;
> >>> +	if (fence && !dma_fence_add_callback(fence, &vb->fence_cb,
> >>> +					     vb2_qbuf_fence_cb))
> >>> +		return 0;
> >>
> >> Maybe we should provide some error or debug log here or a WARN_ON(), if 
> >> dma_fence_add_callback() fails instead of silently ignore any errors.
> > 
> > This is not an error. If the if succeeds it mean we have installed a
> > callback for the fence. If not, it means the fence signaled already and
> > we don't can call __vb2_core_qbuf right away.
> 
> I had the same question as Mauro. After looking at the dma_fence_add_callback
> code I see what you mean, but a comment would certainly be helpful.

Sure, I'll add a comment explaining it.

> 
> Also, should you set vb->in_fence to NULL if the fence signaled already?
> I'm not sure if you need to call 'dma_fence_put(vb->in_fence);' as well.
> You would know that better than I do.

You got it right. I should be doing that.

Gustavo
  

2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:

> On 06/16/17 09:39, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > 
> > Receive in-fence from userspace and add support for waiting on them
> > before queueing the buffer to the driver. Buffers are only queued
> > to the driver once they are ready. A buffer is ready when its
> > in-fence signals.
> > 
> > v2:
> > 	- fix vb2_queue_or_prepare_buf() ret check
> > 	- remove check for VB2_MEMORY_DMABUF only (Javier)
> > 	- check num of ready buffers to start streaming
> > 	- when queueing, start from the first ready buffer
> > 	- handle queue cancel
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> > ---
> >  drivers/media/Kconfig                    |  1 +
> >  drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
> >  drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
> >  include/media/videobuf2-core.h           |  7 ++-
> >  4 files changed, 99 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> > index 55d9c2b..3cd1d3d 100644
> > --- a/drivers/media/Kconfig
> > +++ b/drivers/media/Kconfig
> > @@ -11,6 +11,7 @@ config CEC_NOTIFIER
> >  menuconfig MEDIA_SUPPORT
> >  	tristate "Multimedia support"
> >  	depends on HAS_IOMEM
> > +	select SYNC_FILE
> 
> Is this the right place for this? Shouldn't this be selected in
> 'config VIDEOBUF2_CORE'?
> 
> Fences are specific to vb2 after all.

Definitely.

> 
> >  	help
> >  	  If you want to use Webcams, Video grabber devices and/or TV devices
> >  	  enable this option and other options below.
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index ea83126..29aa9d4 100644

> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -1279,6 +1279,22 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
> >  	return 0;
> >  }
> >  
> > +static int __get_num_ready_buffers(struct vb2_queue *q)
> > +{
> > +	struct vb2_buffer *vb;
> > +	int ready_count = 0;
> > +
> > +	/* count num of buffers ready in front of the queued_list */
> > +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> > +		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> > +			break;
> 
> Obviously the break is wrong as Mauro mentioned.

I replied this in the other email to Mauro, if a fence is not signaled
it is not ready te be queued by the driver nor is all buffers following
it. Hence the break. They need all to be in order and in front of the
queue.

In any case I'll check this again as now there is two people saying I'm
wrong! :)

> 
> > +
> > +		ready_count++;
> > +	}
> > +
> > +	return ready_count;
> > +}
> > +
> >  int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
> >  {
> >  	struct vb2_buffer *vb;
> > @@ -1324,8 +1340,15 @@ static int vb2_start_streaming(struct vb2_queue *q)
> >  	 * If any buffers were queued before streamon,
> >  	 * we can now pass them to driver for processing.
> >  	 */
> > -	list_for_each_entry(vb, &q->queued_list, queued_entry)
> > +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> > +		if (vb->state != VB2_BUF_STATE_QUEUED)
> > +			continue;
> 
> I think this test is unnecessary.

It might be indeed. It is necessary in __vb2_core_qbuf() so I think I
just copied it here without thinking.

> 
> > +
> > +		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> > +			break;
> > +
> >  		__enqueue_in_driver(vb);
> 
> I would move the above test (after fixing it as Mauro said) to __enqueue_in_driver.
> I.e. if this is waiting for a fence then __enqueue_in_driver does nothing.

Yes, having this check inside __enqueue_in_driver() makes it looks much
nicer.

> 
> > +	}
> >  
> >  	/* Tell the driver to start streaming */
> >  	q->start_streaming_called = 1;
> > @@ -1369,33 +1392,55 @@ static int vb2_start_streaming(struct vb2_queue *q)
> >  
> >  static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
> >  {
> > +	struct vb2_buffer *b;
> >  	int ret;
> >  
> >  	/*
> >  	 * If already streaming, give the buffer to driver for processing.
> >  	 * If not, the buffer will be given to driver on next streamon.
> >  	 */
> > -	if (q->start_streaming_called)
> > -		__enqueue_in_driver(vb);
> >  
> > -	/*
> > -	 * If streamon has been called, and we haven't yet called
> > -	 * start_streaming() since not enough buffers were queued, and
> > -	 * we now have reached the minimum number of queued buffers,
> > -	 * then we can finally call start_streaming().
> > -	 */
> > -	if (q->streaming && !q->start_streaming_called &&
> > -	    q->queued_count >= q->min_buffers_needed) {
> > -		ret = vb2_start_streaming(q);
> > -		if (ret)
> > -			return ret;
> > +	if (q->start_streaming_called) {
> > +		list_for_each_entry(b, &q->queued_list, queued_entry) {
> > +			if (b->state != VB2_BUF_STATE_QUEUED)
> > +				continue;
> > +
> > +			if (b->in_fence && !dma_fence_is_signaled(b->in_fence))
> > +				break;
> 
> Again, if this test is in __enqueue_in_driver, then you can keep the
> original code. Why would you need to loop over all buffers anyway?
> 
> If a fence is ready then the callback will call this function for that
> buffer. Everything works fine AFAICT without looping over buffers here.

That seems correct. I'll just remove this loop.

	Gustavo
  

2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:

> On 06/16/17 09:39, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > 
> > Receive in-fence from userspace and add support for waiting on them
> > before queueing the buffer to the driver. Buffers are only queued
> > to the driver once they are ready. A buffer is ready when its
> > in-fence signals.
> > 
> > v2:
> > 	- fix vb2_queue_or_prepare_buf() ret check
> > 	- remove check for VB2_MEMORY_DMABUF only (Javier)
> > 	- check num of ready buffers to start streaming
> > 	- when queueing, start from the first ready buffer
> > 	- handle queue cancel
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> > ---
> >  drivers/media/Kconfig                    |  1 +
> >  drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
> >  drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
> >  include/media/videobuf2-core.h           |  7 ++-
> >  4 files changed, 99 insertions(+), 21 deletions(-)
> > 
> 
> <snip>
> 
> > diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > index 110fb45..e6ad77f 100644
> > --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> > +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > @@ -23,6 +23,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/freezer.h>
> >  #include <linux/kthread.h>
> > +#include <linux/sync_file.h>
> >  
> >  #include <media/v4l2-dev.h>
> >  #include <media/v4l2-fh.h>
> > @@ -560,6 +561,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
> >  
> >  int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >  {
> > +	struct dma_fence *fence = NULL;
> >  	int ret;
> >  
> >  	if (vb2_fileio_is_active(q)) {
> > @@ -568,7 +570,18 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >  	}
> >  
> >  	ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> > -	return ret ? ret : vb2_core_qbuf(q, b->index, b);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
> > +		fence = sync_file_get_fence(b->fence_fd);
> > +		if (!fence) {
> > +			dprintk(1, "failed to get in-fence from fd\n");
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
> >  }
> >  EXPORT_SYMBOL_GPL(vb2_qbuf);
> 
> You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
> if there is a fence pending. It should also fill in fence_fd.

It was userspace who sent the fence_fd in the first place. Why do we
need to send it back? The initial plan was - from a userspace point of view
- to send the in-fence in the fence_fd field and receive the out-fence
 in the same field.

As per setting the IN_FENCE flag, that is racy, as the fence can signal
just after we called __fill_v4l2_buffer. Why is it important to set it?

	Gustavo
  

On 07/07/2017 04:00 AM, Gustavo Padovan wrote:
> 2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:
> 
>> On 06/16/17 09:39, Gustavo Padovan wrote:
>>> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>>>
>>> Receive in-fence from userspace and add support for waiting on them
>>> before queueing the buffer to the driver. Buffers are only queued
>>> to the driver once they are ready. A buffer is ready when its
>>> in-fence signals.
>>>
>>> v2:
>>> 	- fix vb2_queue_or_prepare_buf() ret check
>>> 	- remove check for VB2_MEMORY_DMABUF only (Javier)
>>> 	- check num of ready buffers to start streaming
>>> 	- when queueing, start from the first ready buffer
>>> 	- handle queue cancel
>>>
>>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>>> ---
>>>   drivers/media/Kconfig                    |  1 +
>>>   drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
>>>   drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
>>>   include/media/videobuf2-core.h           |  7 ++-
>>>   4 files changed, 99 insertions(+), 21 deletions(-)
>>>
>>
>> <snip>
>>
>>> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
>>> index 110fb45..e6ad77f 100644
>>> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
>>> @@ -23,6 +23,7 @@
>>>   #include <linux/sched.h>
>>>   #include <linux/freezer.h>
>>>   #include <linux/kthread.h>
>>> +#include <linux/sync_file.h>
>>>   
>>>   #include <media/v4l2-dev.h>
>>>   #include <media/v4l2-fh.h>
>>> @@ -560,6 +561,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
>>>   
>>>   int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>>>   {
>>> +	struct dma_fence *fence = NULL;
>>>   	int ret;
>>>   
>>>   	if (vb2_fileio_is_active(q)) {
>>> @@ -568,7 +570,18 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>>>   	}
>>>   
>>>   	ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
>>> -	return ret ? ret : vb2_core_qbuf(q, b->index, b);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
>>> +		fence = sync_file_get_fence(b->fence_fd);
>>> +		if (!fence) {
>>> +			dprintk(1, "failed to get in-fence from fd\n");
>>> +			return -EINVAL;
>>> +		}
>>> +	}
>>> +
>>> +	return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
>>>   }
>>>   EXPORT_SYMBOL_GPL(vb2_qbuf);
>>
>> You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
>> if there is a fence pending. It should also fill in fence_fd.
> 
> It was userspace who sent the fence_fd in the first place. Why do we
> need to send it back? The initial plan was - from a userspace point of view
> - to send the in-fence in the fence_fd field and receive the out-fence
>   in the same field.
> 
> As per setting the IN_FENCE flag, that is racy, as the fence can signal
> just after we called __fill_v4l2_buffer. Why is it important to set it?

Because when running VIDIOC_QUERYBUF it should return the current state of
the buffer, including whether it has a fence. You can do something like
v4l2-ctl --list-buffers to see how many buffers are queued up and the state
of each buffer. Can be useful to e.g. figure out why a video capture seems
to stall. Knowing that all queued buffers are all waiting for a fence is
very useful information. Whether the fence_fd should also be set here or
only in dqbuf is something I don't know (not enough knowledge about how
fences are supposed to work). The IN/OUT_FENCE flags should definitely be
reported though.

Anyway, remember that this callback is called from various vb2 ioctls:
qbuf, dqbuf, querybuf and prepare_buf.

Querybuf especially can be called at any time.

Regards,

	Hans
  

On 07/07/2017 03:53 AM, Gustavo Padovan wrote:
>>
>>>   	help
>>>   	  If you want to use Webcams, Video grabber devices and/or TV devices
>>>   	  enable this option and other options below.
>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>>> index ea83126..29aa9d4 100644
> 
>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>> @@ -1279,6 +1279,22 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
>>>   	return 0;
>>>   }
>>>   
>>> +static int __get_num_ready_buffers(struct vb2_queue *q)
>>> +{
>>> +	struct vb2_buffer *vb;
>>> +	int ready_count = 0;
>>> +
>>> +	/* count num of buffers ready in front of the queued_list */
>>> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
>>> +		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
>>> +			break;
>>
>> Obviously the break is wrong as Mauro mentioned.
> 
> I replied this in the other email to Mauro, if a fence is not signaled
> it is not ready te be queued by the driver nor is all buffers following
> it. Hence the break. They need all to be in order and in front of the
> queue.
> 
> In any case I'll check this again as now there is two people saying I'm
> wrong! :)

I think this comes back to the 'ordered' requirement and what that means
exactly. In this particular case if I have buffers queued up in vb2 without
a fence (or the fence was signaled), why shouldn't it possible to queue them
up to the driver right away?

Of course, if all buffers are waiting for a fence, then __get_num_ready_buffers
returns 0 and nothing happens.

My understanding is that the ordered requirement is for the hardware,
i.e. queueing buffers A, B, C to ordered hardware requires that they come
out in the same order.

If 'ordered' means that the qbuf/dqbuf sequence must be ordered which implies
that vb2 also needs to keep them ordered, then I need to review the code again.

Can you explain (or point to an explanation) the reason behind the ordered
requirement?

I think you explained it to me when we met during a conference, but I've
forgotten the details.

Regards,

	Hans
  

2017-07-07 Hans Verkuil <hverkuil@xs4all.nl>:

> On 07/07/2017 04:00 AM, Gustavo Padovan wrote:
> > 2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:
> > 
> > > On 06/16/17 09:39, Gustavo Padovan wrote:
> > > > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > > > 
> > > > Receive in-fence from userspace and add support for waiting on them
> > > > before queueing the buffer to the driver. Buffers are only queued
> > > > to the driver once they are ready. A buffer is ready when its
> > > > in-fence signals.
> > > > 
> > > > v2:
> > > > 	- fix vb2_queue_or_prepare_buf() ret check
> > > > 	- remove check for VB2_MEMORY_DMABUF only (Javier)
> > > > 	- check num of ready buffers to start streaming
> > > > 	- when queueing, start from the first ready buffer
> > > > 	- handle queue cancel
> > > > 
> > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> > > > ---
> > > >   drivers/media/Kconfig                    |  1 +
> > > >   drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
> > > >   drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
> > > >   include/media/videobuf2-core.h           |  7 ++-
> > > >   4 files changed, 99 insertions(+), 21 deletions(-)
> > > > 
> > > 
> > > <snip>
> > > 
> > > > diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > > > index 110fb45..e6ad77f 100644
> > > > --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> > > > +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > > > @@ -23,6 +23,7 @@
> > > >   #include <linux/sched.h>
> > > >   #include <linux/freezer.h>
> > > >   #include <linux/kthread.h>
> > > > +#include <linux/sync_file.h>
> > > >   #include <media/v4l2-dev.h>
> > > >   #include <media/v4l2-fh.h>
> > > > @@ -560,6 +561,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
> > > >   int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> > > >   {
> > > > +	struct dma_fence *fence = NULL;
> > > >   	int ret;
> > > >   	if (vb2_fileio_is_active(q)) {
> > > > @@ -568,7 +570,18 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> > > >   	}
> > > >   	ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> > > > -	return ret ? ret : vb2_core_qbuf(q, b->index, b);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
> > > > +		fence = sync_file_get_fence(b->fence_fd);
> > > > +		if (!fence) {
> > > > +			dprintk(1, "failed to get in-fence from fd\n");
> > > > +			return -EINVAL;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
> > > >   }
> > > >   EXPORT_SYMBOL_GPL(vb2_qbuf);
> > > 
> > > You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
> > > if there is a fence pending. It should also fill in fence_fd.
> > 
> > It was userspace who sent the fence_fd in the first place. Why do we
> > need to send it back? The initial plan was - from a userspace point of view
> > - to send the in-fence in the fence_fd field and receive the out-fence
> >   in the same field.
> > 
> > As per setting the IN_FENCE flag, that is racy, as the fence can signal
> > just after we called __fill_v4l2_buffer. Why is it important to set it?
> 
> Because when running VIDIOC_QUERYBUF it should return the current state of
> the buffer, including whether it has a fence. You can do something like
> v4l2-ctl --list-buffers to see how many buffers are queued up and the state
> of each buffer. Can be useful to e.g. figure out why a video capture seems
> to stall. Knowing that all queued buffers are all waiting for a fence is
> very useful information. Whether the fence_fd should also be set here or
> only in dqbuf is something I don't know (not enough knowledge about how
> fences are supposed to work). The IN/OUT_FENCE flags should definitely be
> reported though.

I didn't know about this usecase. Thanks for explaining. It certainly
makes sense to set the IN/OUT_FENCE flags here. Regarding the fence_fd
I would expect the application to know the fence fd associated to than
buffer. If we expect an application other than the one which issued
QBUF than I'd say we also need to provide the fd on VIDIOC_QUERYBUF
for inspection purposes. Is that the case?

Gustavo
  

2017-07-07 Hans Verkuil <hverkuil@xs4all.nl>:

> On 07/07/2017 03:53 AM, Gustavo Padovan wrote:
> > > 
> > > >   	help
> > > >   	  If you want to use Webcams, Video grabber devices and/or TV devices
> > > >   	  enable this option and other options below.
> > > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > > > index ea83126..29aa9d4 100644
> > 
> > > > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > > > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > > > @@ -1279,6 +1279,22 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
> > > >   	return 0;
> > > >   }
> > > > +static int __get_num_ready_buffers(struct vb2_queue *q)
> > > > +{
> > > > +	struct vb2_buffer *vb;
> > > > +	int ready_count = 0;
> > > > +
> > > > +	/* count num of buffers ready in front of the queued_list */
> > > > +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> > > > +		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> > > > +			break;
> > > 
> > > Obviously the break is wrong as Mauro mentioned.
> > 
> > I replied this in the other email to Mauro, if a fence is not signaled
> > it is not ready te be queued by the driver nor is all buffers following
> > it. Hence the break. They need all to be in order and in front of the
> > queue.
> > 
> > In any case I'll check this again as now there is two people saying I'm
> > wrong! :)
> 
> I think this comes back to the 'ordered' requirement and what that means
> exactly. In this particular case if I have buffers queued up in vb2 without
> a fence (or the fence was signaled), why shouldn't it possible to queue them
> up to the driver right away?
> 
> Of course, if all buffers are waiting for a fence, then __get_num_ready_buffers
> returns 0 and nothing happens.
> 
> My understanding is that the ordered requirement is for the hardware,
> i.e. queueing buffers A, B, C to ordered hardware requires that they come
> out in the same order.

That is correct. I thought I had to queue to the hardware in the order we
receive from userspace. So that makes that loop indeed wrong, as we
should queue the buffers right away.

The ordered requirement is for the OUT_FENCE side because after we queue
the buffer to the hardware and send the BUF_QUEUED event out userspace
might just go ahead and issue an DRM Atomic Request containing that
buffer and the out-fence fd received. DRM then needs to wait on that
fence before any scanout, but it may wait after the scanout is not
allowed to fail anymore.

Thus if a buffer requeuing happens at buffer_done() the fence won't
signal and DRM/KMS won't have a buffer to display. That is reason behind
it.

Alternatively we can ignore this and live with the fact that sometimes a
requeuing may affect the scanout pipeline.

	Gustavo
  

2017-07-10 Gustavo Padovan <gustavo@padovan.org>:

> 2017-07-07 Hans Verkuil <hverkuil@xs4all.nl>:
> 
> > On 07/07/2017 04:00 AM, Gustavo Padovan wrote:
> > > 2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:
> > > 
> > > > On 06/16/17 09:39, Gustavo Padovan wrote:
> > > > > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > > > > 
> > > > > Receive in-fence from userspace and add support for waiting on them
> > > > > before queueing the buffer to the driver. Buffers are only queued
> > > > > to the driver once they are ready. A buffer is ready when its
> > > > > in-fence signals.
> > > > > 
> > > > > v2:
> > > > > 	- fix vb2_queue_or_prepare_buf() ret check
> > > > > 	- remove check for VB2_MEMORY_DMABUF only (Javier)
> > > > > 	- check num of ready buffers to start streaming
> > > > > 	- when queueing, start from the first ready buffer
> > > > > 	- handle queue cancel
> > > > > 
> > > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> > > > > ---
> > > > >   drivers/media/Kconfig                    |  1 +
> > > > >   drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
> > > > >   drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
> > > > >   include/media/videobuf2-core.h           |  7 ++-
> > > > >   4 files changed, 99 insertions(+), 21 deletions(-)
> > > > > 
> > > > 
> > > > <snip>
> > > > 
> > > > > diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > > > > index 110fb45..e6ad77f 100644
> > > > > --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> > > > > +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > > > > @@ -23,6 +23,7 @@
> > > > >   #include <linux/sched.h>
> > > > >   #include <linux/freezer.h>
> > > > >   #include <linux/kthread.h>
> > > > > +#include <linux/sync_file.h>
> > > > >   #include <media/v4l2-dev.h>
> > > > >   #include <media/v4l2-fh.h>
> > > > > @@ -560,6 +561,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
> > > > >   int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> > > > >   {
> > > > > +	struct dma_fence *fence = NULL;
> > > > >   	int ret;
> > > > >   	if (vb2_fileio_is_active(q)) {
> > > > > @@ -568,7 +570,18 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> > > > >   	}
> > > > >   	ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> > > > > -	return ret ? ret : vb2_core_qbuf(q, b->index, b);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
> > > > > +		fence = sync_file_get_fence(b->fence_fd);
> > > > > +		if (!fence) {
> > > > > +			dprintk(1, "failed to get in-fence from fd\n");
> > > > > +			return -EINVAL;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
> > > > >   }
> > > > >   EXPORT_SYMBOL_GPL(vb2_qbuf);
> > > > 
> > > > You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
> > > > if there is a fence pending. It should also fill in fence_fd.
> > > 
> > > It was userspace who sent the fence_fd in the first place. Why do we
> > > need to send it back? The initial plan was - from a userspace point of view
> > > - to send the in-fence in the fence_fd field and receive the out-fence
> > >   in the same field.
> > > 
> > > As per setting the IN_FENCE flag, that is racy, as the fence can signal
> > > just after we called __fill_v4l2_buffer. Why is it important to set it?
> > 
> > Because when running VIDIOC_QUERYBUF it should return the current state of
> > the buffer, including whether it has a fence. You can do something like
> > v4l2-ctl --list-buffers to see how many buffers are queued up and the state
> > of each buffer. Can be useful to e.g. figure out why a video capture seems
> > to stall. Knowing that all queued buffers are all waiting for a fence is
> > very useful information. Whether the fence_fd should also be set here or
> > only in dqbuf is something I don't know (not enough knowledge about how
> > fences are supposed to work). The IN/OUT_FENCE flags should definitely be
> > reported though.
> 
> I didn't know about this usecase. Thanks for explaining. It certainly
> makes sense to set the IN/OUT_FENCE flags here. Regarding the fence_fd
> I would expect the application to know the fence fd associated to than
> buffer. If we expect an application other than the one which issued
> QBUF than I'd say we also need to provide the fd on VIDIOC_QUERYBUF
> for inspection purposes. Is that the case?

I just realized that if we want to also set the in-fence fd here we
actually need to get a new unused fd, as either it is a different pid or
the app already closed the fd it was using previously. Given this extra
complication I'd say we shouldn't set fence fd unless someone has an
usecase in mind.

	Gustavo
  

On 10/07/17 22:26, Gustavo Padovan wrote:
> 2017-07-10 Gustavo Padovan <gustavo@padovan.org>:
> 
>> 2017-07-07 Hans Verkuil <hverkuil@xs4all.nl>:
>>
>>> On 07/07/2017 04:00 AM, Gustavo Padovan wrote:
>>>> 2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:
>>>>
>>>>> On 06/16/17 09:39, Gustavo Padovan wrote:
>>>>>> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>>>>>>
>>>>>> Receive in-fence from userspace and add support for waiting on them
>>>>>> before queueing the buffer to the driver. Buffers are only queued
>>>>>> to the driver once they are ready. A buffer is ready when its
>>>>>> in-fence signals.
>>>>>>
>>>>>> v2:
>>>>>> 	- fix vb2_queue_or_prepare_buf() ret check
>>>>>> 	- remove check for VB2_MEMORY_DMABUF only (Javier)
>>>>>> 	- check num of ready buffers to start streaming
>>>>>> 	- when queueing, start from the first ready buffer
>>>>>> 	- handle queue cancel
>>>>>>
>>>>>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>>>>>> ---
>>>>>>   drivers/media/Kconfig                    |  1 +
>>>>>>   drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
>>>>>>   drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
>>>>>>   include/media/videobuf2-core.h           |  7 ++-
>>>>>>   4 files changed, 99 insertions(+), 21 deletions(-)
>>>>>>
>>>>>
>>>>> <snip>
>>>>>
>>>>>> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
>>>>>> index 110fb45..e6ad77f 100644
>>>>>> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
>>>>>> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
>>>>>> @@ -23,6 +23,7 @@
>>>>>>   #include <linux/sched.h>
>>>>>>   #include <linux/freezer.h>
>>>>>>   #include <linux/kthread.h>
>>>>>> +#include <linux/sync_file.h>
>>>>>>   #include <media/v4l2-dev.h>
>>>>>>   #include <media/v4l2-fh.h>
>>>>>> @@ -560,6 +561,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
>>>>>>   int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>>>>>>   {
>>>>>> +	struct dma_fence *fence = NULL;
>>>>>>   	int ret;
>>>>>>   	if (vb2_fileio_is_active(q)) {
>>>>>> @@ -568,7 +570,18 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>>>>>>   	}
>>>>>>   	ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
>>>>>> -	return ret ? ret : vb2_core_qbuf(q, b->index, b);
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
>>>>>> +		fence = sync_file_get_fence(b->fence_fd);
>>>>>> +		if (!fence) {
>>>>>> +			dprintk(1, "failed to get in-fence from fd\n");
>>>>>> +			return -EINVAL;
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>> +	return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
>>>>>>   }
>>>>>>   EXPORT_SYMBOL_GPL(vb2_qbuf);
>>>>>
>>>>> You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
>>>>> if there is a fence pending. It should also fill in fence_fd.
>>>>
>>>> It was userspace who sent the fence_fd in the first place. Why do we
>>>> need to send it back? The initial plan was - from a userspace point of view
>>>> - to send the in-fence in the fence_fd field and receive the out-fence
>>>>   in the same field.
>>>>
>>>> As per setting the IN_FENCE flag, that is racy, as the fence can signal
>>>> just after we called __fill_v4l2_buffer. Why is it important to set it?
>>>
>>> Because when running VIDIOC_QUERYBUF it should return the current state of
>>> the buffer, including whether it has a fence. You can do something like
>>> v4l2-ctl --list-buffers to see how many buffers are queued up and the state
>>> of each buffer. Can be useful to e.g. figure out why a video capture seems
>>> to stall. Knowing that all queued buffers are all waiting for a fence is
>>> very useful information. Whether the fence_fd should also be set here or
>>> only in dqbuf is something I don't know (not enough knowledge about how
>>> fences are supposed to work). The IN/OUT_FENCE flags should definitely be
>>> reported though.
>>
>> I didn't know about this usecase. Thanks for explaining. It certainly
>> makes sense to set the IN/OUT_FENCE flags here. Regarding the fence_fd
>> I would expect the application to know the fence fd associated to than
>> buffer. If we expect an application other than the one which issued
>> QBUF than I'd say we also need to provide the fd on VIDIOC_QUERYBUF
>> for inspection purposes. Is that the case?
> 
> I just realized that if we want to also set the in-fence fd here we
> actually need to get a new unused fd, as either it is a different pid or
> the app already closed the fd it was using previously. Given this extra
> complication I'd say we shouldn't set fence fd unless someone has an
> usecase in mind.

Fair enough. Just make sure the fence_fd is some fixed value (-1?) in
that case.

Regards,

	Hans
  

2017-07-11 Hans Verkuil <hverkuil@xs4all.nl>:

> On 10/07/17 22:26, Gustavo Padovan wrote:
> > 2017-07-10 Gustavo Padovan <gustavo@padovan.org>:
> > 
> >> 2017-07-07 Hans Verkuil <hverkuil@xs4all.nl>:
> >>
> >>> On 07/07/2017 04:00 AM, Gustavo Padovan wrote:
> >>>> 2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:
> >>>>
> >>>>> On 06/16/17 09:39, Gustavo Padovan wrote:
> >>>>>> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> >>>>>>
> >>>>>> Receive in-fence from userspace and add support for waiting on them
> >>>>>> before queueing the buffer to the driver. Buffers are only queued
> >>>>>> to the driver once they are ready. A buffer is ready when its
> >>>>>> in-fence signals.
> >>>>>>
> >>>>>> v2:
> >>>>>> 	- fix vb2_queue_or_prepare_buf() ret check
> >>>>>> 	- remove check for VB2_MEMORY_DMABUF only (Javier)
> >>>>>> 	- check num of ready buffers to start streaming
> >>>>>> 	- when queueing, start from the first ready buffer
> >>>>>> 	- handle queue cancel
> >>>>>>
> >>>>>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> >>>>>> ---
> >>>>>>   drivers/media/Kconfig                    |  1 +
> >>>>>>   drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
> >>>>>>   drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
> >>>>>>   include/media/videobuf2-core.h           |  7 ++-
> >>>>>>   4 files changed, 99 insertions(+), 21 deletions(-)
> >>>>>>
> >>>>>
> >>>>> <snip>
> >>>>>
> >>>>>> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> >>>>>> index 110fb45..e6ad77f 100644
> >>>>>> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> >>>>>> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> >>>>>> @@ -23,6 +23,7 @@
> >>>>>>   #include <linux/sched.h>
> >>>>>>   #include <linux/freezer.h>
> >>>>>>   #include <linux/kthread.h>
> >>>>>> +#include <linux/sync_file.h>
> >>>>>>   #include <media/v4l2-dev.h>
> >>>>>>   #include <media/v4l2-fh.h>
> >>>>>> @@ -560,6 +561,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
> >>>>>>   int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >>>>>>   {
> >>>>>> +	struct dma_fence *fence = NULL;
> >>>>>>   	int ret;
> >>>>>>   	if (vb2_fileio_is_active(q)) {
> >>>>>> @@ -568,7 +570,18 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >>>>>>   	}
> >>>>>>   	ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> >>>>>> -	return ret ? ret : vb2_core_qbuf(q, b->index, b);
> >>>>>> +	if (ret)
> >>>>>> +		return ret;
> >>>>>> +
> >>>>>> +	if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
> >>>>>> +		fence = sync_file_get_fence(b->fence_fd);
> >>>>>> +		if (!fence) {
> >>>>>> +			dprintk(1, "failed to get in-fence from fd\n");
> >>>>>> +			return -EINVAL;
> >>>>>> +		}
> >>>>>> +	}
> >>>>>> +
> >>>>>> +	return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
> >>>>>>   }
> >>>>>>   EXPORT_SYMBOL_GPL(vb2_qbuf);
> >>>>>
> >>>>> You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
> >>>>> if there is a fence pending. It should also fill in fence_fd.
> >>>>
> >>>> It was userspace who sent the fence_fd in the first place. Why do we
> >>>> need to send it back? The initial plan was - from a userspace point of view
> >>>> - to send the in-fence in the fence_fd field and receive the out-fence
> >>>>   in the same field.
> >>>>
> >>>> As per setting the IN_FENCE flag, that is racy, as the fence can signal
> >>>> just after we called __fill_v4l2_buffer. Why is it important to set it?
> >>>
> >>> Because when running VIDIOC_QUERYBUF it should return the current state of
> >>> the buffer, including whether it has a fence. You can do something like
> >>> v4l2-ctl --list-buffers to see how many buffers are queued up and the state
> >>> of each buffer. Can be useful to e.g. figure out why a video capture seems
> >>> to stall. Knowing that all queued buffers are all waiting for a fence is
> >>> very useful information. Whether the fence_fd should also be set here or
> >>> only in dqbuf is something I don't know (not enough knowledge about how
> >>> fences are supposed to work). The IN/OUT_FENCE flags should definitely be
> >>> reported though.
> >>
> >> I didn't know about this usecase. Thanks for explaining. It certainly
> >> makes sense to set the IN/OUT_FENCE flags here. Regarding the fence_fd
> >> I would expect the application to know the fence fd associated to than
> >> buffer. If we expect an application other than the one which issued
> >> QBUF than I'd say we also need to provide the fd on VIDIOC_QUERYBUF
> >> for inspection purposes. Is that the case?
> > 
> > I just realized that if we want to also set the in-fence fd here we
> > actually need to get a new unused fd, as either it is a different pid or
> > the app already closed the fd it was using previously. Given this extra
> > complication I'd say we shouldn't set fence fd unless someone has an
> > usecase in mind.
> 
> Fair enough. Just make sure the fence_fd is some fixed value (-1?) in
> that case.

Sure. -1 is the default value for these cases.

	Gustavo