diff mbox series

vb2: warn for or disable the CMA + USERPTR combination

Message ID a8932f2c-5342-2cd8-9b98-1db0de756190@xs4all.nl (mailing list archive)
State New
Delegated to: Hans Verkuil
Headers
Series vb2: warn for or disable the CMA + USERPTR combination |

Commit Message

Hans Verkuil Nov. 11, 2021, 11:02 a.m. UTC 
  If one or more pages of the user-allocated buffer memory were
allocated in CMA memory, then when the buffer is prepared any
attempt to pin such pages will fail since user-allocated pages
in CMA memory are supposed to be moveable, and pinning them in
place would defeat the purpose of CMA.

CONFIG_CMA is typically only used with embedded systems, and
in that case the use of DMABUF is preferred.

So warn for this combination, and also add a new config option
to disable USERPTR support altogether if CONFIG_CMA is set.

I've chosen to put this under a config option since disabling
it unconditionally might cause userspace breakage.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
Should USERPTR just be disabled unconditionally if CONFIG_CMA is set?
Feedback would be welcome.

I noticed this issue when testing on a VM instance which had CMA
set and had 4 GB memory allocated to it. The test-media regression
test started failing because of this issue. Increasing the memory
to 16 GB 'solved' it, but that's just papering over the real problem.
Hence this patch.
---
 drivers/media/common/videobuf2/Kconfig         | 11 +++++++++++
 .../media/common/videobuf2/videobuf2-core.c    | 18 ++++++++++++++++++
 2 files changed, 29 insertions(+)

Comments

Laurent Pinchart Nov. 11, 2021, 2:07 p.m. UTC | #1 
Hi Hans,

Thank you for the patch.

On Thu, Nov 11, 2021 at 12:02:52PM +0100, Hans Verkuil wrote:
> If one or more pages of the user-allocated buffer memory were
> allocated in CMA memory, then when the buffer is prepared any
> attempt to pin such pages will fail since user-allocated pages
> in CMA memory are supposed to be moveable, and pinning them in
> place would defeat the purpose of CMA.
> 
> CONFIG_CMA is typically only used with embedded systems, and
> in that case the use of DMABUF is preferred.
> 
> So warn for this combination, and also add a new config option
> to disable USERPTR support altogether if CONFIG_CMA is set.
> 
> I've chosen to put this under a config option since disabling
> it unconditionally might cause userspace breakage.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> Should USERPTR just be disabled unconditionally if CONFIG_CMA is set?
> Feedback would be welcome.

I've long advocated for deprecating USERPTR (if not universally, at
least in most use cases), so this would be my preference.

> I noticed this issue when testing on a VM instance which had CMA
> set and had 4 GB memory allocated to it. The test-media regression
> test started failing because of this issue. Increasing the memory
> to 16 GB 'solved' it, but that's just papering over the real problem.
> Hence this patch.
> ---
>  drivers/media/common/videobuf2/Kconfig         | 11 +++++++++++
>  .../media/common/videobuf2/videobuf2-core.c    | 18 ++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/media/common/videobuf2/Kconfig b/drivers/media/common/videobuf2/Kconfig
> index d2223a12c95f..d89042cbb5cf 100644
> --- a/drivers/media/common/videobuf2/Kconfig
> +++ b/drivers/media/common/videobuf2/Kconfig
> @@ -7,6 +7,17 @@ config VIDEOBUF2_CORE
>  config VIDEOBUF2_V4L2
>  	tristate
> 
> +config VIDEOBUF2_DISABLE_USERPTR_AND_CMA
> +	bool "Disable use of V4L2 USERPTR streaming if CMA is enabled"
> +	depends on CMA
> +	depends on VIDEOBUF2_V4L2
> +	help
> +	  Say Y here to disable V4L2 USERPTR streaming mode if CMA is enabled.
> +	  If some of the pages of the buffer memory were allocated in CMA memory,
> +	  then attempting to pin those pages in place will fail with an error.
> +
> +	  When in doubt, say N.
> +
>  config VIDEOBUF2_MEMOPS
>  	tristate
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 2266bbd239ab..17166d4212d0 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -662,6 +662,20 @@ static int __verify_userptr_ops(struct vb2_queue *q)
>  	    !q->mem_ops->put_userptr)
>  		return -EINVAL;
> 
> +#ifdef CONFIG_CMA
> +	/*
> +	 * If one or more pages of the user-allocated buffer memory were
> +	 * allocated in CMA memory, then when the buffer is prepared any
> +	 * attempt to pin such pages will fail since user-allocated pages
> +	 * in CMA memory are supposed to be moveable, and pinning them in
> +	 * place would defeat the purpose of CMA.
> +	 *
> +	 * CONFIG_CMA is typically only used with embedded systems, and
> +	 * in that case the use of DMABUF is preferred.
> +	 */
> +	pr_warn_once("The USERPTR I/O streaming mode is unreliable if CMA is enabled.\n");
> +	pr_warn_once("Use the DMABUF I/O streaming mode instead.\n");
> +#endif
>  	return 0;
>  }
> 
> @@ -2399,6 +2413,10 @@ int vb2_core_queue_init(struct vb2_queue *q)
>  	if (WARN_ON(q->supports_requests && q->min_buffers_needed))
>  		return -EINVAL;
> 
> +#ifdef CONFIG_VIDEOBUF2_DISABLE_USERPTR_AND_CMA
> +	q->io_modes &= ~VB2_USERPTR;
> +#endif
> +
>  	INIT_LIST_HEAD(&q->queued_list);
>  	INIT_LIST_HEAD(&q->done_list);
>  	spin_lock_init(&q->done_lock);
Tomasz Figa Nov. 29, 2021, 9:37 a.m. UTC | #2 
On Thu, Nov 11, 2021 at 8:03 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> If one or more pages of the user-allocated buffer memory were
> allocated in CMA memory, then when the buffer is prepared any
> attempt to pin such pages will fail since user-allocated pages
> in CMA memory are supposed to be moveable, and pinning them in
> place would defeat the purpose of CMA.
>
> CONFIG_CMA is typically only used with embedded systems, and
> in that case the use of DMABUF is preferred.
>
> So warn for this combination, and also add a new config option
> to disable USERPTR support altogether if CONFIG_CMA is set.
>
> I've chosen to put this under a config option since disabling
> it unconditionally might cause userspace breakage.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> Should USERPTR just be disabled unconditionally if CONFIG_CMA is set?
> Feedback would be welcome.
>
> I noticed this issue when testing on a VM instance which had CMA
> set and had 4 GB memory allocated to it. The test-media regression
> test started failing because of this issue. Increasing the memory
> to 16 GB 'solved' it, but that's just papering over the real problem.
> Hence this patch.
> ---
>  drivers/media/common/videobuf2/Kconfig         | 11 +++++++++++
>  .../media/common/videobuf2/videobuf2-core.c    | 18 ++++++++++++++++++
>  2 files changed, 29 insertions(+)
>
> diff --git a/drivers/media/common/videobuf2/Kconfig b/drivers/media/common/videobuf2/Kconfig
> index d2223a12c95f..d89042cbb5cf 100644
> --- a/drivers/media/common/videobuf2/Kconfig
> +++ b/drivers/media/common/videobuf2/Kconfig
> @@ -7,6 +7,17 @@ config VIDEOBUF2_CORE
>  config VIDEOBUF2_V4L2
>         tristate
>
> +config VIDEOBUF2_DISABLE_USERPTR_AND_CMA
> +       bool "Disable use of V4L2 USERPTR streaming if CMA is enabled"
> +       depends on CMA
> +       depends on VIDEOBUF2_V4L2
> +       help
> +         Say Y here to disable V4L2 USERPTR streaming mode if CMA is enabled.
> +         If some of the pages of the buffer memory were allocated in CMA memory,
> +         then attempting to pin those pages in place will fail with an error.
> +
> +         When in doubt, say N.
> +
>  config VIDEOBUF2_MEMOPS
>         tristate
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 2266bbd239ab..17166d4212d0 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -662,6 +662,20 @@ static int __verify_userptr_ops(struct vb2_queue *q)
>             !q->mem_ops->put_userptr)
>                 return -EINVAL;
>
> +#ifdef CONFIG_CMA
> +       /*
> +        * If one or more pages of the user-allocated buffer memory were
> +        * allocated in CMA memory, then when the buffer is prepared any
> +        * attempt to pin such pages will fail since user-allocated pages
> +        * in CMA memory are supposed to be moveable, and pinning them in
> +        * place would defeat the purpose of CMA.
> +        *
> +        * CONFIG_CMA is typically only used with embedded systems, and
> +        * in that case the use of DMABUF is preferred.
> +        */
> +       pr_warn_once("The USERPTR I/O streaming mode is unreliable if CMA is enabled.\n");
> +       pr_warn_once("Use the DMABUF I/O streaming mode instead.\n");
> +#endif
>         return 0;
>  }
>
> @@ -2399,6 +2413,10 @@ int vb2_core_queue_init(struct vb2_queue *q)
>         if (WARN_ON(q->supports_requests && q->min_buffers_needed))
>                 return -EINVAL;
>
> +#ifdef CONFIG_VIDEOBUF2_DISABLE_USERPTR_AND_CMA
> +       q->io_modes &= ~VB2_USERPTR;
> +#endif
> +
>         INIT_LIST_HEAD(&q->queued_list);
>         INIT_LIST_HEAD(&q->done_list);
>         spin_lock_init(&q->done_lock);
> --
> 2.33.0
>

I think this is a good first step. I wonder if we should explore the
possibility of officially declaring USERPTR deprecated in the
documentation?

Acked-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz
diff mbox series

Patch

diff --git a/drivers/media/common/videobuf2/Kconfig b/drivers/media/common/videobuf2/Kconfig
index d2223a12c95f..d89042cbb5cf 100644
--- a/drivers/media/common/videobuf2/Kconfig
+++ b/drivers/media/common/videobuf2/Kconfig
@@ -7,6 +7,17 @@  config VIDEOBUF2_CORE
 config VIDEOBUF2_V4L2
 	tristate

+config VIDEOBUF2_DISABLE_USERPTR_AND_CMA
+	bool "Disable use of V4L2 USERPTR streaming if CMA is enabled"
+	depends on CMA
+	depends on VIDEOBUF2_V4L2
+	help
+	  Say Y here to disable V4L2 USERPTR streaming mode if CMA is enabled.
+	  If some of the pages of the buffer memory were allocated in CMA memory,
+	  then attempting to pin those pages in place will fail with an error.
+
+	  When in doubt, say N.
+
 config VIDEOBUF2_MEMOPS
 	tristate

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 2266bbd239ab..17166d4212d0 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -662,6 +662,20 @@  static int __verify_userptr_ops(struct vb2_queue *q)
 	    !q->mem_ops->put_userptr)
 		return -EINVAL;

+#ifdef CONFIG_CMA
+	/*
+	 * If one or more pages of the user-allocated buffer memory were
+	 * allocated in CMA memory, then when the buffer is prepared any
+	 * attempt to pin such pages will fail since user-allocated pages
+	 * in CMA memory are supposed to be moveable, and pinning them in
+	 * place would defeat the purpose of CMA.
+	 *
+	 * CONFIG_CMA is typically only used with embedded systems, and
+	 * in that case the use of DMABUF is preferred.
+	 */
+	pr_warn_once("The USERPTR I/O streaming mode is unreliable if CMA is enabled.\n");
+	pr_warn_once("Use the DMABUF I/O streaming mode instead.\n");
+#endif
 	return 0;
 }

@@ -2399,6 +2413,10 @@  int vb2_core_queue_init(struct vb2_queue *q)
 	if (WARN_ON(q->supports_requests && q->min_buffers_needed))
 		return -EINVAL;

+#ifdef CONFIG_VIDEOBUF2_DISABLE_USERPTR_AND_CMA
+	q->io_modes &= ~VB2_USERPTR;
+#endif
+
 	INIT_LIST_HEAD(&q->queued_list);
 	INIT_LIST_HEAD(&q->done_list);
 	spin_lock_init(&q->done_lock);