s5p-tv/mixer_video.c weirdness

s5p-tv/mixer_video.c weirdness

[Hans Verkuil]

Hi Marek, Sylwester,

I've been investigating how multiplanar is used in various drivers, and I
came across this driver that is a bit weird.

querycap sets both single and multiple planar output caps:

        cap->capabilities = V4L2_CAP_STREAMING |
                V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_VIDEO_OUTPUT_MPLANE;

This suggests that both the single and multiplanar APIs are supported.

But mxr_ioctl_ops only implements these:

        /* format handling */
        .vidioc_enum_fmt_vid_out = mxr_enum_fmt,
        .vidioc_s_fmt_vid_out_mplane = mxr_s_fmt,
        .vidioc_g_fmt_vid_out_mplane = mxr_g_fmt,

Mixing single planar enum_fmt with multiplanar s/g_fmt makes little sense.

I suspect everything should be multiplanar.

BTW, I recommend running v4l2-compliance over your s5p drivers. I saw several
things it would fail on.

Regards,

	Hans

Re: s5p-tv/mixer_video.c weirdness

[Sylwester Nawrocki]

Hi Hans,

On 09/21/2012 12:07 PM, Hans Verkuil wrote:
> Hi Marek, Sylwester,
> 
> I've been investigating how multiplanar is used in various drivers, and I
> came across this driver that is a bit weird.
> 
> querycap sets both single and multiple planar output caps:
> 
>          cap->capabilities = V4L2_CAP_STREAMING |
>                  V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_VIDEO_OUTPUT_MPLANE;
> 
> This suggests that both the single and multiplanar APIs are supported.

Thanks for spotting this. Somehow this driver wasn't fixed at the time
this issue was addressed in s5p-mfc and s5p-fimc drivers in commits:

8f401543e [media] s5p-fimc: Remove single-planar capability flags,
43defb118 [media] v4l: s5p-mfc: fix reported capabilities.

The reason why these drivers were setting the both capability type flags
was that original idea was to have multi/single-plane buffer related 
ioctls translated in the kernel. So the v4l2-core would be turning 
a multi-plane only driver into a single-plane capable as well. But the 
in kernel conversion code was stripped at last minute during merge for 
known reasons. Looks like the s5p-tv driver haven't been receiving 
enough love, probably because HDMI and TV-out support for these SoCs 
now happens mainly in DRM.

I'll make sure there is a patch queued for 3.7, correcting those 
capabilities and enum_fmt.

BTW, I couldn't find a justification of making multi-planar a property
of fourcc, rather than of the memory/buffer, which it really is. As in 
this RFC [1]. Inventing multi-planar fourccs always seemed not a best
idea to me..

> But mxr_ioctl_ops only implements these:
> 
>          /* format handling */
>          .vidioc_enum_fmt_vid_out = mxr_enum_fmt,
>          .vidioc_s_fmt_vid_out_mplane = mxr_s_fmt,
>          .vidioc_g_fmt_vid_out_mplane = mxr_g_fmt,
> 
> Mixing single planar enum_fmt with multiplanar s/g_fmt makes little sense.
> 
> I suspect everything should be multiplanar.

Yes, it should be all multi-planar right from the beginning.

> BTW, I recommend running v4l2-compliance over your s5p drivers. I saw several
> things it would fail on.

I have it queued on my todo list, I'll see if it can be done for v3.8.

--

Regards,
Sylwester

[1] http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/11212

[PATCH RFC] s5p-tv: Report only multi-plane capabilities in vidioc_querycap

[Sylwester Nawrocki]

The mixer video node supports only multi-planar API so the driver
should not be setting V4L2_CAP_VIDEO_OUTPUT flags. Fix this and
also switch to device_caps. Additionally fix the VIDIOC_ENUM_FMT
ioctl handler which now works for V4L2_BUF_TYPE_CAPTURE, rather
than expected V4L2_BUF_TYPE_CAPTURE_MPLANE.

Cc: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-tv/mixer_video.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/s5p-tv/mixer_video.c b/drivers/media/platform/s5p-tv/mixer_video.c
index 8649de01..9876bd9 100644
--- a/drivers/media/platform/s5p-tv/mixer_video.c
+++ b/drivers/media/platform/s5p-tv/mixer_video.c
@@ -164,9 +164,8 @@ static int mxr_querycap(struct file *file, void *priv,
 	strlcpy(cap->driver, MXR_DRIVER_NAME, sizeof cap->driver);
 	strlcpy(cap->card, layer->vfd.name, sizeof cap->card);
 	sprintf(cap->bus_info, "%d", layer->idx);
-	cap->version = KERNEL_VERSION(0, 1, 0);
-	cap->capabilities = V4L2_CAP_STREAMING |
-		V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_VIDEO_OUTPUT_MPLANE;
+	cap->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_OUTPUT_MPLANE;
+	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;

 	return 0;
 }
@@ -727,7 +726,7 @@ static int mxr_streamoff(struct file *file, void *priv, enum v4l2_buf_type i)
 static const struct v4l2_ioctl_ops mxr_ioctl_ops = {
 	.vidioc_querycap = mxr_querycap,
 	/* format handling */
-	.vidioc_enum_fmt_vid_out = mxr_enum_fmt,
+	.vidioc_enum_fmt_vid_out_mplane = mxr_enum_fmt,
 	.vidioc_s_fmt_vid_out_mplane = mxr_s_fmt,
 	.vidioc_g_fmt_vid_out_mplane = mxr_g_fmt,
 	/* buffer control */
--
1.7.11.3