Re: [PATCH 09/14] media: omap3isp: better VIDIOC_G/S_PARM handling

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

Hi Hans,

On Fri, Oct 17, 2025 at 03:26:46PM +0200, Hans Verkuil wrote:
> Fix various v4l2-compliance errors relating to timeperframe.
> 
> VIDIOC_G/S_PARM is only supported for Video Output, so disable
> these ioctls for Capture devices.
> 
> Ensure numerator and denominator are never 0.
> 
> Set missing V4L2_CAP_TIMEPERFRAME capability for VIDIOC_S_PARM.
> 
> v4l2-compliance:
> 
> 	fail: v4l2-test-formats.cpp(1388): out->timeperframe.numerator == 0 || out->timeperframe.denominator == 0
> test VIDIOC_G/S_PARM: FAIL
> 
> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> ---
>  drivers/media/platform/ti/omap3isp/ispvideo.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> index 471defa6e7fb..5603586271f5 100644
> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> @@ -928,7 +928,10 @@ isp_video_set_param(struct file *file, void *fh, struct v4l2_streamparm *a)
>  
>  	if (a->parm.output.timeperframe.denominator == 0)
>  		a->parm.output.timeperframe.denominator = 1;
> +	if (a->parm.output.timeperframe.numerator == 0)
> +		a->parm.output.timeperframe.numerator = 1;

I believe S_PARM support has probably been added for v4l2-compliance in the
past. Should there be either a dummy implementation for more or less all
Media device centric drivers or could this be simply omitted?

>  
> +	a->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
>  	vfh->timeperframe = a->parm.output.timeperframe;
>  
>  	return 0;
> @@ -1413,6 +1416,7 @@ static int isp_video_open(struct file *file)
>  	handle->format.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
>  	isp_video_pix_to_mbus(&handle->format.fmt.pix, &fmt);
>  	isp_video_mbus_to_pix(video, &fmt, &handle->format.fmt.pix);
> +	handle->timeperframe.numerator = 1;
>  	handle->timeperframe.denominator = 1;
>  
>  	handle->video = video;
> @@ -1532,12 +1536,15 @@ int omap3isp_video_init(struct isp_video *video, const char *name)
>  	video->video.vfl_type = VFL_TYPE_VIDEO;
>  	video->video.release = video_device_release_empty;
>  	video->video.ioctl_ops = &isp_video_ioctl_ops;
> -	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
>  		video->video.device_caps = V4L2_CAP_VIDEO_CAPTURE
>  					 | V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
> -	else
> +		v4l2_disable_ioctl(&video->video, VIDIOC_S_PARM);
> +		v4l2_disable_ioctl(&video->video, VIDIOC_G_PARM);
> +	} else {
>  		video->video.device_caps = V4L2_CAP_VIDEO_OUTPUT
>  					 | V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
> +	}
>  
>  	video->pipe.stream_state = ISP_PIPELINE_STREAM_STOPPED;
>  

-- 
Kind regards,

Sakari Ailus