Hi, On 06/16/2017 02:58 PM, Sakari Ailus wrote: > Hi Hans, > > Cc Sylwester and Marek as well. > > On Mon, May 08, 2017 at 04:35:05PM +0200, Hans Verkuil wrote: >> From: Hans Verkuil <hansv...@cisco.com> >> >> The type field in struct v4l2_selection is supposed to never use the >> _MPLANE variants. E.g. if the driver supports >> V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, >> then userspace should still pass V4L2_BUF_TYPE_VIDEO_CAPTURE. >> >> The reasons for this are lost in the mists of time, but it is really >> annoying. In addition, the exynos drivers didn't follow this rule and >> instead expected the _MPLANE type. >> >> To fix that code is added to the v4l2 core that maps the _MPLANE buffer >> types to their regular equivalents before calling the driver. >> >> Effectively this allows for userspace to use either _MPLANE or the regular >> buffer type. This keeps backwards compatibility while making things easier >> for userspace. >> >> Since drivers now never see the _MPLANE buffer types the exynos drivers >> had to be adapted as well. >> >> Signed-off-by: Hans Verkuil <hansv...@cisco.com> >> --- >> drivers/media/platform/exynos-gsc/gsc-core.c | 4 +- >> drivers/media/platform/exynos-gsc/gsc-m2m.c | 8 ++-- >> drivers/media/platform/exynos4-is/fimc-capture.c | 4 +- >> drivers/media/platform/exynos4-is/fimc-lite.c | 4 +- >> drivers/media/v4l2-core/v4l2-ioctl.c | 53 >> +++++++++++++++++++++--- >> 5 files changed, 57 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c >> b/drivers/media/platform/exynos-gsc/gsc-core.c >> index 59a634201830..107faa04c947 100644 >> --- a/drivers/media/platform/exynos-gsc/gsc-core.c >> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
>> +/* >> + * The selection API specified originally that the _MPLANE buffer types >> + * shouldn't be used. The reasons for this are lost in the mists of time >> + * (or just really crappy memories). Regardless, this is really annoying >> + * for userspace. So to keep things simple we map _MPLANE buffer types >> + * to their 'regular' counterparts before calling the driver. And we >> + * restore it afterwards. This way applications can use either buffer >> + * type and drivers don't need to check for both. I agree this is annoying the _MPLANE buffer types are excluded this way. I suspect one of the main reasons was bias of the original author of the selection API to the multi-planar API. Now the API got messy because I didn't adhere to this rule of buffer type change for VIDIOC_S_SELECTION, and the code propagated to the other driver. Sorry about that. I checked GStreamer and AFAIU there is no change of buffer type for S_SELECTION there: https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/sys/v4l2/gstv4l2object.c?id=3342d86d9b92cf60c419b728d10944968d77ecac#n3722 >> + */ >> +static int v4l_g_selection(const struct v4l2_ioctl_ops *ops, >> + struct file *file, void *fh, void *arg) >> +{ >> + struct v4l2_selection *p = arg; >> + u32 old_type = p->type; >> + int ret; >> + >> + if (p->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) >> + p->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> + else if (p->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >> + p->type = V4L2_BUF_TYPE_VIDEO_OUTPUT; >> + ret = ops->vidioc_g_selection(file, fh, p); >> + p->type = old_type; >> + return ret; >> +} >> + >> +static int v4l_s_selection(const struct v4l2_ioctl_ops *ops, >> + struct file *file, void *fh, void *arg) >> +{ >> + struct v4l2_selection *p = arg; >> + u32 old_type = p->type; >> + int ret; >> + >> + if (p->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) >> + p->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> + else if (p->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >> + p->type = V4L2_BUF_TYPE_VIDEO_OUTPUT; >> + ret = ops->vidioc_s_selection(file, fh, p); > > Can it be that ops->vidioc_s_selection() is NULL here? I don't think it's > checked anywhere. Same in v4l_g_selection(). I think it can't be, there is the valid_ioctls bitmap test before a call back to the driver, to see if driver actually implements an ioctl. And the bitmap is populated beforehand in determine_valid_ioctls(). -- Regards, Sylwester
