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@xxxxxxxxx>
> ---
> 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);
--
Regards,
Laurent Pinchart
