Re: [PATCH 07/14] media: omap3isp: rework isp_video_try/set_format

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Hans,

On Fri, Oct 17, 2025 at 03:26:44PM +0200, Hans Verkuil wrote:
> isp_video_set_format now calls isp_video_try_format first, ensuring
> consistent behavior and removing duplicate code in both functions.
> 
> This fixes an v4l2-compliance error:
> 
> 	fail: v4l2-test-formats.cpp(519): !pix.sizeimage
> test VIDIOC_S_FMT: FAIL
> 
> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> ---
>  drivers/media/platform/ti/omap3isp/ispvideo.c | 60 +++++++++----------
>  1 file changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> index d3fe28506fb0..69d17e4dcd36 100644
> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> @@ -701,11 +701,15 @@ isp_video_get_format(struct file *file, void *fh, struct v4l2_format *format)
>  }
>  
>  static int
> -isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
> +isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
>  {
> -	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
>  	struct isp_video *video = video_drvdata(file);
> -	struct v4l2_mbus_framefmt fmt;
> +	struct v4l2_subdev_format fmt = {
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +	};
> +	struct v4l2_subdev *subdev;
> +	u32 pad;
> +	int ret;
>  
>  	if (format->type != video->type)
>  		return -EINVAL;
> @@ -745,32 +749,11 @@ isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
>  		break;
>  	}
>  
> -	/* Fill the bytesperline and sizeimage fields by converting to media bus
> -	 * format and back to pixel format.
> -	 */
> -	isp_video_pix_to_mbus(&format->fmt.pix, &fmt);
> -	isp_video_mbus_to_pix(video, &fmt, &format->fmt.pix);
> -
> -	mutex_lock(&video->mutex);
> -	vfh->format = *format;
> -	mutex_unlock(&video->mutex);
> -
> -	return 0;
> -}
> -
> -static int
> -isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
> -{
> -	struct isp_video *video = video_drvdata(file);
> -	struct v4l2_subdev_format fmt = {
> -		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> -	};
> -	struct v4l2_subdev *subdev;
> -	u32 pad;
> -	int ret;
> -
> -	if (format->type != video->type)
> -		return -EINVAL;
> +	if (video->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> +		isp_video_pix_to_mbus(&format->fmt.pix, &fmt.format);
> +		isp_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix);
> +		return 0;
> +	}
>  
>  	subdev = isp_video_remote_subdev(video, &pad);
>  	if (subdev == NULL)
> @@ -781,12 +764,29 @@ isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
>  	fmt.pad = pad;
>  	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
>  	if (ret)
> -		return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
> +		return ret == -ENOIOCTLCMD ? -EINVAL : ret;

I believe -ENOTTY was right. Yet this should not happen (albeit I haven't
checked) as all omap3isp sub-device drivers with connected video devices
presumably implement get_fmt.

>  
>  	isp_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix);
>  	return 0;
>  }
>  
> +static int
> +isp_video_set_format(struct file *file, void *fh, struct v4l2_format *format)
> +{
> +	struct isp_video_fh *vfh = file_to_isp_video_fh(file);
> +	struct isp_video *video = video_drvdata(file);
> +	int ret = isp_video_try_format(file, fh, format);

I'd do this assignment separately: initialising variables in declaration
that requires error handling isn't very nice.

> +
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&video->mutex);
> +	vfh->format = *format;
> +	mutex_unlock(&video->mutex);
> +
> +	return 0;
> +}
> +
>  static int
>  isp_video_get_selection(struct file *file, void *fh, struct v4l2_selection *sel)
>  {

-- 
Kind regards,

Sakari Ailus