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

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

Hi Hans,

On Mon, Dec 01, 2025 at 02:40:38PM +0100, Hans Verkuil wrote:
> On 01/12/2025 11:28, Hans Verkuil wrote:
> > On 22/10/2025 10:09, Sakari Ailus wrote:
> >> 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?
> > 
> > v4l2-compliance has seen quite a few changes w.r.t. the G/S_PARM tests,
> > and I think it is fine to just drop support for these ioctls.
> > 
> > I'll test this a bit more, and if I don't find any issues, then I'll
> > just remove support for these ioctls in omap3isp.
> 
> Actually, S_PARM is used to set the max framerate for the output of the omap3isp.
> 
> So it is in use.
> 
> I'm keeping this patch.

You're right; it's used on OUTPUT buffers indeed.

-- 
Sakari Ailus