Re: [PATCH 08/14] media: omap3isp: implement create/prepare_bufs

[Hans Verkuil <hverkuil+cisco@kernel.org>]

On 22/10/2025 10:06, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Oct 17, 2025 at 03:26:45PM +0200, Hans Verkuil wrote:
>> Add missing ioctls. This makes v4l2-compliance happier:
>>
>> 	warn: v4l2-test-buffers.cpp(813): VIDIOC_CREATE_BUFS not supported
>> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
>>
>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
>> ---
>>  drivers/media/platform/ti/omap3isp/ispvideo.c | 47 ++++++++++++++++++-
>>  1 file changed, 45 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
>> index 69d17e4dcd36..471defa6e7fb 100644
>> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
>> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
>> @@ -325,6 +325,13 @@ static int isp_video_queue_setup(struct vb2_queue *queue,
>>  	struct isp_video_fh *vfh = vb2_get_drv_priv(queue);
>>  	struct isp_video *video = vfh->video;
>>  
>> +	if (*num_planes) {
>> +		if (*num_planes != 1)
>> +			return -EINVAL;
>> +		if (sizes[0] < vfh->format.fmt.pix.sizeimage)
>> +			return -EINVAL;
>> +		return 0;
>> +	}
>>  	*num_planes = 1;
>>  
>>  	sizes[0] = vfh->format.fmt.pix.sizeimage;
>> @@ -340,6 +347,7 @@ static int isp_video_buffer_prepare(struct vb2_buffer *buf)
>>  {
>>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(buf);
>>  	struct isp_video_fh *vfh = vb2_get_drv_priv(buf->vb2_queue);
>> +	unsigned int size = vfh->format.fmt.pix.sizeimage;
>>  	struct isp_buffer *buffer = to_isp_buffer(vbuf);
>>  	struct isp_video *video = vfh->video;
>>  	dma_addr_t addr;
>> @@ -360,8 +368,13 @@ static int isp_video_buffer_prepare(struct vb2_buffer *buf)
>>  		return -EINVAL;
>>  	}
>>  
>> -	vb2_set_plane_payload(&buffer->vb.vb2_buf, 0,
>> -			      vfh->format.fmt.pix.sizeimage);
>> +	if (vb2_plane_size(&buffer->vb.vb2_buf, 0) < size) {
>> +		dev_dbg(video->isp->dev,
>> +			"data will not fit into plane (%lu < %u)\n",
>> +			vb2_plane_size(&buffer->vb.vb2_buf, 0), size);
>> +		return -EINVAL;
>> +	}
>> +	vb2_set_plane_payload(&buffer->vb.vb2_buf, 0, size);
>>  	buffer->dma = addr;
>>  
>>  	return 0;
>> @@ -935,6 +948,20 @@ isp_video_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *rb)
>>  	return ret;
>>  }
>>  
>> +static int
>> +isp_video_create_bufs(struct file *file, void *fh, struct v4l2_create_buffers *p)
>> +{
>> +	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
>> +	struct isp_video *video = video_drvdata(file);
>> +	int ret;
>> +
>> +	mutex_lock(&video->queue_lock);
>> +	ret = vb2_create_bufs(&vfh->queue, p);
>> +	mutex_unlock(&video->queue_lock);
>> +
>> +	return ret;
>> +}
>> +
>>  static int
>>  isp_video_querybuf(struct file *file, void *fh, struct v4l2_buffer *b)
>>  {
>> @@ -949,6 +976,20 @@ isp_video_querybuf(struct file *file, void *fh, struct v4l2_buffer *b)
>>  	return ret;
>>  }
>>  
>> +static int
>> +isp_video_prepare_buf(struct file *file, void *fh, struct v4l2_buffer *b)
>> +{
>> +	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
>> +	struct isp_video *video = video_drvdata(file);
>> +	int ret;
>> +
>> +	mutex_lock(&video->queue_lock);
>> +	ret = vb2_prepare_buf(&vfh->queue, video->video.v4l2_dev->mdev, b);
>> +	mutex_unlock(&video->queue_lock);
>> +
>> +	return ret;
> 
> Also:
> 
> #include <linux/cleanup.h>
> 
> ...
> 
> 	guard(mutex)(&video->queue_lock);
> 
> 	return vb2_prepare_buf(&vfh->queue, video->video.v4l2_dev->mdev, b);
> 
> Same for create_bufs implementation.

Hmm, I'm hesitant to do that for two reasons:

1) Consistency with existing vb2 callbacks
2) It's harder to backport this to older kernels.

Since this is an old driver, the chances someone might want to backport this
to an old kernel is higher than usual.

If you really want this, then I prefer to make a new patch that introduces
the guards for all vb2 callbacks in this driver.

Regards,

	Hans

> 
> 
>> +}
>> +
>>  static int
>>  isp_video_qbuf(struct file *file, void *fh, struct v4l2_buffer *b)
>>  {
>> @@ -1303,7 +1344,9 @@ static const struct v4l2_ioctl_ops isp_video_ioctl_ops = {
>>  	.vidioc_g_parm			= isp_video_get_param,
>>  	.vidioc_s_parm			= isp_video_set_param,
>>  	.vidioc_reqbufs			= isp_video_reqbufs,
>> +	.vidioc_create_bufs		= isp_video_create_bufs,
>>  	.vidioc_querybuf		= isp_video_querybuf,
>> +	.vidioc_prepare_buf		= isp_video_prepare_buf,
>>  	.vidioc_qbuf			= isp_video_qbuf,
>>  	.vidioc_dqbuf			= isp_video_dqbuf,
>>  	.vidioc_streamon		= isp_video_streamon,
>