On Thu, 11 Apr 2019 09:59:19 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On 4/4/19 10:16 AM, Boris Brezillon wrote:
> > Support for multiplanar and singleplanar formats is mutually exclusive,
> > at least in practice. In our attempt to unify support for support for
> > mplane and !mplane in v4l, let's get rid of the
> > ->vidioc_enum_fmt_{vid,out}_cap_mplane() hooks and call
> > ->vidioc_enum_fmt_{vid,out}_cap() instead.
>
> This is a good idea. This can be done separately from the other patches in
> this series and merged.
Sure, I'll send this patch separately.
>
> One comment...
>
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > ---
> > Changes in v2:
> > - None
> > ---
> > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 2 +-
> > drivers/media/platform/exynos-gsc/gsc-m2m.c | 4 ++--
> > .../media/platform/exynos4-is/fimc-capture.c | 2 +-
> > .../platform/exynos4-is/fimc-isp-video.c | 2 +-
> > drivers/media/platform/exynos4-is/fimc-lite.c | 2 +-
> > drivers/media/platform/exynos4-is/fimc-m2m.c | 4 ++--
> > .../media/platform/mtk-jpeg/mtk_jpeg_core.c | 4 ++--
> > drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 4 ++--
> > .../platform/mtk-vcodec/mtk_vcodec_dec.c | 4 ++--
> > .../platform/mtk-vcodec/mtk_vcodec_enc.c | 4 ++--
> > .../media/platform/qcom/camss/camss-video.c | 2 +-
> > drivers/media/platform/qcom/venus/vdec.c | 4 ++--
> > drivers/media/platform/qcom/venus/venc.c | 4 ++--
> > drivers/media/platform/rcar_fdp1.c | 4 ++--
> > drivers/media/platform/rcar_jpu.c | 4 ++--
> > drivers/media/platform/renesas-ceu.c | 2 +-
> > drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 4 ++--
> > drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 4 ++--
> > drivers/media/platform/ti-vpe/vpe.c | 4 ++--
> > drivers/media/platform/vicodec/vicodec-core.c | 2 --
> > drivers/media/platform/vivid/vivid-core.c | 6 ++----
> > .../media/platform/vivid/vivid-vid-common.c | 20 ------------------
> > .../media/platform/vivid/vivid-vid-common.h | 2 --
> > drivers/media/v4l2-core/v4l2-dev.c | 2 --
> > drivers/media/v4l2-core/v4l2-ioctl.c | 21 ++++++++++---------
> > drivers/staging/media/ipu3/ipu3-v4l2.c | 4 ++--
> > .../media/rockchip/vpu/rockchip_vpu_enc.c | 4 ++--
> > include/media/v4l2-ioctl.h | 14 ++-----------
> > 28 files changed, 51 insertions(+), 88 deletions(-)
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > index f8020ebe9f05..c3b3af3c3b2f 100644
> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > @@ -1174,7 +1174,7 @@ static const struct v4l2_file_operations cio2_v4l2_fops = {
> >
> > static const struct v4l2_ioctl_ops cio2_v4l2_ioctl_ops = {
> > .vidioc_querycap = cio2_v4l2_querycap,
> > - .vidioc_enum_fmt_vid_cap_mplane = cio2_v4l2_enum_fmt,
> > + .vidioc_enum_fmt_vid_cap = cio2_v4l2_enum_fmt,
> > .vidioc_g_fmt_vid_cap_mplane = cio2_v4l2_g_fmt,
> > .vidioc_s_fmt_vid_cap_mplane = cio2_v4l2_s_fmt,
> > .vidioc_try_fmt_vid_cap_mplane = cio2_v4l2_try_fmt,
> > diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> > index c757f5d98bcc..9e2914d3d8f9 100644
> > --- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
> > +++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> > @@ -562,8 +562,8 @@ static int gsc_m2m_s_selection(struct file *file, void *fh,
> >
> > static const struct v4l2_ioctl_ops gsc_m2m_ioctl_ops = {
> > .vidioc_querycap = gsc_m2m_querycap,
> > - .vidioc_enum_fmt_vid_cap_mplane = gsc_m2m_enum_fmt_mplane,
> > - .vidioc_enum_fmt_vid_out_mplane = gsc_m2m_enum_fmt_mplane,
> > + .vidioc_enum_fmt_vid_cap = gsc_m2m_enum_fmt_mplane,
> > + .vidioc_enum_fmt_vid_out = gsc_m2m_enum_fmt_mplane,
>
> Please rename the functions as well, dropping the _mplane suffix. Here and in
> the other drivers.
>
Okay, I'm remove those _mplane suffix.
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index f6d663934648..97ba365218fb 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -1380,6 +1380,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> > static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > struct file *file, void *fh, void *arg)
> > {
> > + struct video_device *vdev = video_devdata(file);
> > struct v4l2_fmtdesc *p = arg;
> > int ret = check_fmt(file, p->type);
> >
> > @@ -1389,30 +1390,30 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> >
> > switch (p->type) {
> > case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > + case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> > + if (!!(vdev->device_caps & V4L2_CAP_VIDEO_CAPTURE_MPLANE) !=
>
> Hmm. I am not sure if all drivers that set V4L2_CAP_VIDEO_CAPTURE_MPLANE also
> fill in vdev->device_caps. While filling in vdev->device_caps is required for
> new drivers, older drivers often don't do this.
>
> You would have to check all drivers that set V4L2_CAP_VIDEO_CAPTURE/OUTPUT_MPLANE
> to verify that they also set vdev->device_caps. I'm fairly certain the Samsung
> drivers don't fill this in.
I'll check that and fix those that don't set the flag. Or do you have
another solution to handle that case?
