diff mbox series

[RFC,9/9] omap3isp/rcar_fdp1/vsp1/xilinx: drop VB2_USERPTR

Message ID 20200221084531.576156-10-hverkuil-cisco@xs4all.nl (mailing list archive)
State New, archived
Headers show
Series Drop VB2_USERPTR + dma-contig combo | expand

Commit Message

Hans Verkuil Feb. 21, 2020, 8:45 a.m. UTC 
The combination of VB2_USERPTR and dma-contig makes no sense for
these devices, drop it.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/omap3isp/ispvideo.c | 2 +-
 drivers/media/platform/rcar_fdp1.c         | 4 ++--
 drivers/media/platform/vsp1/vsp1_video.c   | 2 +-
 drivers/media/platform/xilinx/xilinx-dma.c | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

Comments

Kieran Bingham Feb. 21, 2020, 9:01 a.m. UTC | #1 
Hi Hans,

On 21/02/2020 08:45, Hans Verkuil wrote:
> The combination of VB2_USERPTR and dma-contig makes no sense for
> these devices, drop it.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Agreed, the FDP1 and VSP1 expect contiguous physical memory, so a
user-pointer doesn't make much sense to be permitted.

I haven't lokoed at the Xilinx platform in regards to this but I would
be surprised if it wasn't the same issue there of course.

For VSP1/FDP1:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>



> ---
>  drivers/media/platform/omap3isp/ispvideo.c | 2 +-
>  drivers/media/platform/rcar_fdp1.c         | 4 ++--
>  drivers/media/platform/vsp1/vsp1_video.c   | 2 +-
>  drivers/media/platform/xilinx/xilinx-dma.c | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
> index e8c46ff1aeb4..1104654ba438 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -1319,7 +1319,7 @@ static int isp_video_open(struct file *file)
>  
>  	queue = &handle->queue;
>  	queue->type = video->type;
> -	queue->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
> +	queue->io_modes = VB2_MMAP | VB2_DMABUF;
>  	queue->drv_priv = handle;
>  	queue->ops = &isp_video_queue_ops;
>  	queue->mem_ops = &vb2_dma_contig_memops;
> diff --git a/drivers/media/platform/rcar_fdp1.c b/drivers/media/platform/rcar_fdp1.c
> index 97bed45360f0..df081f66575f 100644
> --- a/drivers/media/platform/rcar_fdp1.c
> +++ b/drivers/media/platform/rcar_fdp1.c
> @@ -2047,7 +2047,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
>  	int ret;
>  
>  	src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> -	src_vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
> +	src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
>  	src_vq->drv_priv = ctx;
>  	src_vq->buf_struct_size = sizeof(struct fdp1_buffer);
>  	src_vq->ops = &fdp1_qops;
> @@ -2061,7 +2061,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
>  		return ret;
>  
>  	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> -	dst_vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
> +	dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
>  	dst_vq->drv_priv = ctx;
>  	dst_vq->buf_struct_size = sizeof(struct fdp1_buffer);
>  	dst_vq->ops = &fdp1_qops;
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
> index 5e59ed2c3614..112e2092f6d3 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -1300,7 +1300,7 @@ struct vsp1_video *vsp1_video_create(struct vsp1_device *vsp1,
>  	video_set_drvdata(&video->video, video);
>  
>  	video->queue.type = video->type;
> -	video->queue.io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
> +	video->queue.io_modes = VB2_MMAP | VB2_DMABUF;
>  	video->queue.lock = &video->lock;
>  	video->queue.drv_priv = video;
>  	video->queue.buf_struct_size = sizeof(struct vsp1_vb2_buffer);
> diff --git a/drivers/media/platform/xilinx/xilinx-dma.c b/drivers/media/platform/xilinx/xilinx-dma.c
> index b211380a11f2..57e52ad720dd 100644
> --- a/drivers/media/platform/xilinx/xilinx-dma.c
> +++ b/drivers/media/platform/xilinx/xilinx-dma.c
> @@ -708,7 +708,7 @@ int xvip_dma_init(struct xvip_composite_device *xdev, struct xvip_dma *dma,
>  	 * instead of 'cat' isn't really a drawback.
>  	 */
>  	dma->queue.type = type;
> -	dma->queue.io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
> +	dma->queue.io_modes = VB2_MMAP | VB2_DMABUF;
>  	dma->queue.lock = &dma->lock;
>  	dma->queue.drv_priv = dma;
>  	dma->queue.buf_struct_size = sizeof(struct xvip_dma_buffer);
>
Laurent Pinchart Feb. 21, 2020, 2:31 p.m. UTC | #2 
Hi Hans,

CC'ing Sakari for the omap3isp part.

On Fri, Feb 21, 2020 at 09:01:02AM +0000, Kieran Bingham wrote:
> On 21/02/2020 08:45, Hans Verkuil wrote:
> > The combination of VB2_USERPTR and dma-contig makes no sense for
> > these devices, drop it.
> > 
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Agreed, the FDP1 and VSP1 expect contiguous physical memory, so a
> user-pointer doesn't make much sense to be permitted.
> 
> I haven't lokoed at the Xilinx platform in regards to this but I would
> be surprised if it wasn't the same issue there of course.
> 
> For VSP1/FDP1:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

for the same.

For the Xilinx driver, you may want to ask Xilinx if they use USERPTR.

> > ---
> >  drivers/media/platform/omap3isp/ispvideo.c | 2 +-
> >  drivers/media/platform/rcar_fdp1.c         | 4 ++--
> >  drivers/media/platform/vsp1/vsp1_video.c   | 2 +-
> >  drivers/media/platform/xilinx/xilinx-dma.c | 2 +-
> >  4 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
> > index e8c46ff1aeb4..1104654ba438 100644
> > --- a/drivers/media/platform/omap3isp/ispvideo.c
> > +++ b/drivers/media/platform/omap3isp/ispvideo.c
> > @@ -1319,7 +1319,7 @@ static int isp_video_open(struct file *file)
> >  
> >  	queue = &handle->queue;
> >  	queue->type = video->type;
> > -	queue->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
> > +	queue->io_modes = VB2_MMAP | VB2_DMABUF;
> >  	queue->drv_priv = handle;
> >  	queue->ops = &isp_video_queue_ops;
> >  	queue->mem_ops = &vb2_dma_contig_memops;
> > diff --git a/drivers/media/platform/rcar_fdp1.c b/drivers/media/platform/rcar_fdp1.c
> > index 97bed45360f0..df081f66575f 100644
> > --- a/drivers/media/platform/rcar_fdp1.c
> > +++ b/drivers/media/platform/rcar_fdp1.c
> > @@ -2047,7 +2047,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
> >  	int ret;
> >  
> >  	src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> > -	src_vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
> > +	src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> >  	src_vq->drv_priv = ctx;
> >  	src_vq->buf_struct_size = sizeof(struct fdp1_buffer);
> >  	src_vq->ops = &fdp1_qops;
> > @@ -2061,7 +2061,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
> >  		return ret;
> >  
> >  	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> > -	dst_vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
> > +	dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> >  	dst_vq->drv_priv = ctx;
> >  	dst_vq->buf_struct_size = sizeof(struct fdp1_buffer);
> >  	dst_vq->ops = &fdp1_qops;
> > diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
> > index 5e59ed2c3614..112e2092f6d3 100644
> > --- a/drivers/media/platform/vsp1/vsp1_video.c
> > +++ b/drivers/media/platform/vsp1/vsp1_video.c
> > @@ -1300,7 +1300,7 @@ struct vsp1_video *vsp1_video_create(struct vsp1_device *vsp1,
> >  	video_set_drvdata(&video->video, video);
> >  
> >  	video->queue.type = video->type;
> > -	video->queue.io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
> > +	video->queue.io_modes = VB2_MMAP | VB2_DMABUF;
> >  	video->queue.lock = &video->lock;
> >  	video->queue.drv_priv = video;
> >  	video->queue.buf_struct_size = sizeof(struct vsp1_vb2_buffer);
> > diff --git a/drivers/media/platform/xilinx/xilinx-dma.c b/drivers/media/platform/xilinx/xilinx-dma.c
> > index b211380a11f2..57e52ad720dd 100644
> > --- a/drivers/media/platform/xilinx/xilinx-dma.c
> > +++ b/drivers/media/platform/xilinx/xilinx-dma.c
> > @@ -708,7 +708,7 @@ int xvip_dma_init(struct xvip_composite_device *xdev, struct xvip_dma *dma,
> >  	 * instead of 'cat' isn't really a drawback.
> >  	 */
> >  	dma->queue.type = type;
> > -	dma->queue.io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
> > +	dma->queue.io_modes = VB2_MMAP | VB2_DMABUF;
> >  	dma->queue.lock = &dma->lock;
> >  	dma->queue.drv_priv = dma;
> >  	dma->queue.buf_struct_size = sizeof(struct xvip_dma_buffer);
Sakari Ailus Feb. 21, 2020, 7:46 p.m. UTC | #3 
Hi Hans, Laurent,

On Fri, Feb 21, 2020 at 04:31:01PM +0200, Laurent Pinchart wrote:
> Hi Hans,
> 
> CC'ing Sakari for the omap3isp part.

Thanks.

The omap3isp is behind an IOMMU, so the USERPTR memory does not need to be
physically contiguous. I don't see a reason to drop userptr support from
the driver.
Laurent Pinchart Feb. 22, 2020, 10:43 a.m. UTC | #4 
Hi Sakari,

On Fri, Feb 21, 2020 at 09:46:41PM +0200, Sakari Ailus wrote:
> Hi Hans, Laurent,
> 
> On Fri, Feb 21, 2020 at 04:31:01PM +0200, Laurent Pinchart wrote:
> > Hi Hans,
> > 
> > CC'ing Sakari for the omap3isp part.
> 
> Thanks.
> 
> The omap3isp is behind an IOMMU, so the USERPTR memory does not need to be
> physically contiguous. I don't see a reason to drop userptr support from
> the driver.

Apart from the fact that this API should be discouraged :-) I wonder if
it's used, that's my real question. As we can't rule it out, I'd be
cautious about dropping it.
Sakari Ailus Feb. 22, 2020, 1:13 p.m. UTC | #5 
Hi Laurent,

On Sat, Feb 22, 2020 at 12:43:48PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Fri, Feb 21, 2020 at 09:46:41PM +0200, Sakari Ailus wrote:
> > Hi Hans, Laurent,
> > 
> > On Fri, Feb 21, 2020 at 04:31:01PM +0200, Laurent Pinchart wrote:
> > > Hi Hans,
> > > 
> > > CC'ing Sakari for the omap3isp part.
> > 
> > Thanks.
> > 
> > The omap3isp is behind an IOMMU, so the USERPTR memory does not need to be
> > physically contiguous. I don't see a reason to drop userptr support from
> > the driver.
> 
> Apart from the fact that this API should be discouraged :-) I wonder if
> it's used, that's my real question. As we can't rule it out, I'd be
> cautious about dropping it.

Indeed. If we'd drop USERPTR, it should apply to the uAPI, the framework
and all the drivers, not an individual driver. But again that would be
certain to break a lot of applications...

I guess we could document USERPTR as deprecated, and not recommended for
new applications?
diff mbox series

Patch

diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index e8c46ff1aeb4..1104654ba438 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1319,7 +1319,7 @@  static int isp_video_open(struct file *file)
 
 	queue = &handle->queue;
 	queue->type = video->type;
-	queue->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
+	queue->io_modes = VB2_MMAP | VB2_DMABUF;
 	queue->drv_priv = handle;
 	queue->ops = &isp_video_queue_ops;
 	queue->mem_ops = &vb2_dma_contig_memops;
diff --git a/drivers/media/platform/rcar_fdp1.c b/drivers/media/platform/rcar_fdp1.c
index 97bed45360f0..df081f66575f 100644
--- a/drivers/media/platform/rcar_fdp1.c
+++ b/drivers/media/platform/rcar_fdp1.c
@@ -2047,7 +2047,7 @@  static int queue_init(void *priv, struct vb2_queue *src_vq,
 	int ret;
 
 	src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
-	src_vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
+	src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
 	src_vq->drv_priv = ctx;
 	src_vq->buf_struct_size = sizeof(struct fdp1_buffer);
 	src_vq->ops = &fdp1_qops;
@@ -2061,7 +2061,7 @@  static int queue_init(void *priv, struct vb2_queue *src_vq,
 		return ret;
 
 	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
-	dst_vq->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
+	dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
 	dst_vq->drv_priv = ctx;
 	dst_vq->buf_struct_size = sizeof(struct fdp1_buffer);
 	dst_vq->ops = &fdp1_qops;
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 5e59ed2c3614..112e2092f6d3 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -1300,7 +1300,7 @@  struct vsp1_video *vsp1_video_create(struct vsp1_device *vsp1,
 	video_set_drvdata(&video->video, video);
 
 	video->queue.type = video->type;
-	video->queue.io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
+	video->queue.io_modes = VB2_MMAP | VB2_DMABUF;
 	video->queue.lock = &video->lock;
 	video->queue.drv_priv = video;
 	video->queue.buf_struct_size = sizeof(struct vsp1_vb2_buffer);
diff --git a/drivers/media/platform/xilinx/xilinx-dma.c b/drivers/media/platform/xilinx/xilinx-dma.c
index b211380a11f2..57e52ad720dd 100644
--- a/drivers/media/platform/xilinx/xilinx-dma.c
+++ b/drivers/media/platform/xilinx/xilinx-dma.c
@@ -708,7 +708,7 @@  int xvip_dma_init(struct xvip_composite_device *xdev, struct xvip_dma *dma,
 	 * instead of 'cat' isn't really a drawback.
 	 */
 	dma->queue.type = type;
-	dma->queue.io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
+	dma->queue.io_modes = VB2_MMAP | VB2_DMABUF;
 	dma->queue.lock = &dma->lock;
 	dma->queue.drv_priv = dma;
 	dma->queue.buf_struct_size = sizeof(struct xvip_dma_buffer);