Re: [PATCH 04/14] media: omap3isp: implement enum_fmt_vid_cap/out

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

On 22/10/2025 09:56, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Oct 17, 2025 at 03:26:41PM +0200, Hans Verkuil wrote:
>> Add missing ioctls. This makes v4l2-compliance happier:
>>
>> fail: v4l2-test-formats.cpp(516): pixelformat 59565955 (UYVY) for buftype 1 not reported by ENUM_FMT
>> 	test VIDIOC_G_FMT: FAIL
>> fail: v4l2-test-formats.cpp(516): pixelformat 59565955 (UYVY) for buftype 1 not reported by ENUM_FMT
>> 	test VIDIOC_TRY_FMT: FAIL
>> fail: v4l2-test-formats.cpp(516): pixelformat 56595559 (YUYV) for buftype 1 not reported by ENUM_FMT
>> 	test VIDIOC_S_FMT: FAIL
>>
>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
>> ---
>>  drivers/media/platform/ti/omap3isp/ispvideo.c | 34 +++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>
>> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
>> index 864d38140b87..77beea00d507 100644
>> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
>> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
>> @@ -652,6 +652,38 @@ isp_video_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
>>  	return 0;
>>  }
>>  
>> +static int
>> +isp_video_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
>> +{
>> +	struct isp_video *video = video_drvdata(file);
>> +	unsigned int i, j;
>> +	unsigned int skip_last_fmts = 1;
>> +
>> +	if (f->type != video->type)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * The last two formats have the same pixelformat as the two
>> +	 * formats before them, but they do have different mediabus
>> +	 * codes. So to avoid reporting duplicate pixelformats we skip
>> +	 * those two, provided f->mbus_code is 0.
>> +	 */
>> +	if (!f->mbus_code)
>> +		skip_last_fmts += 2;
>> +	for (i = 0, j = 0; i < ARRAY_SIZE(formats) - skip_last_fmts; i++) {
>> +		if (f->mbus_code && formats[i].code != f->mbus_code)
>> +			continue;
> 
> How about, instead of the skip_last_fmts thingy, using this:
> 
> 		/* Weed out pixelformats with the same mbus code. */
> 		if (i && formats[i - 1].code == formats[i].code)
> 			continue;

Good idea, but it should be this:

               /* Weed out duplicate pixelformats with different mbus codes */
               if (!f->mbus_code && i &&
                   formats[i - 1].pixelformat == formats[i].pixelformat)
                       continue;

And the duplicate pixelformats in the formats array must be together in the
array for this to work. Easy enough to change.

Regards,

	Hans

> 
>> +
>> +		if (j == f->index) {
>> +			f->pixelformat = formats[i].pixelformat;
>> +			return 0;
>> +		}
>> +		j++;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>>  static int
>>  isp_video_get_format(struct file *file, void *fh, struct v4l2_format *format)
>>  {
>> @@ -1258,9 +1290,11 @@ isp_video_s_input(struct file *file, void *fh, unsigned int input)
>>  
>>  static const struct v4l2_ioctl_ops isp_video_ioctl_ops = {
>>  	.vidioc_querycap		= isp_video_querycap,
>> +	.vidioc_enum_fmt_vid_cap	= isp_video_enum_format,
>>  	.vidioc_g_fmt_vid_cap		= isp_video_get_format,
>>  	.vidioc_s_fmt_vid_cap		= isp_video_set_format,
>>  	.vidioc_try_fmt_vid_cap		= isp_video_try_format,
>> +	.vidioc_enum_fmt_vid_out	= isp_video_enum_format,
>>  	.vidioc_g_fmt_vid_out		= isp_video_get_format,
>>  	.vidioc_s_fmt_vid_out		= isp_video_set_format,
>>  	.vidioc_try_fmt_vid_out		= isp_video_try_format,
>