| Message ID | 20190412092027.20410-1-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] media: exynos4-is: Properly report _MPLANE caps | expand |
On 4/12/19 11:20 AM, Boris Brezillon wrote: > The fimc-isp-video.c and fimc-lite.c were missing the > V4L2_CAP_VIDEO_CAPTURE_MPLANE flag when reporting device caps. > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > Changes in v3: > - New patch > --- > drivers/media/platform/exynos4-is/fimc-isp-video.c | 4 +++- > drivers/media/platform/exynos4-is/fimc-lite.c | 2 +- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/exynos4-is/fimc-isp-video.c b/drivers/media/platform/exynos4-is/fimc-isp-video.c > index bb35a2017f21..8cd7b1c6565c 100644 > --- a/drivers/media/platform/exynos4-is/fimc-isp-video.c > +++ b/drivers/media/platform/exynos4-is/fimc-isp-video.c > @@ -349,7 +349,9 @@ static int isp_video_querycap(struct file *file, void *priv, > { > struct fimc_isp *isp = video_drvdata(file); > > - __fimc_vidioc_querycap(&isp->pdev->dev, cap, V4L2_CAP_STREAMING); > + __fimc_vidioc_querycap(&isp->pdev->dev, cap, > + V4L2_CAP_STREAMING | > + V4L2_CAP_VIDEO_CAPTURE_MPLANE); > return 0; > } > > diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c > index 96f0a8a0dcae..13dfa5e39139 100644 > --- a/drivers/media/platform/exynos4-is/fimc-lite.c > +++ b/drivers/media/platform/exynos4-is/fimc-lite.c > @@ -659,7 +659,7 @@ static int fimc_lite_querycap(struct file *file, void *priv, > snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", > dev_name(&fimc->pdev->dev)); > > - cap->device_caps = V4L2_CAP_STREAMING; > + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_STREAMING; > cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS; That's not what should happen. The driver should set device_caps in the struct video_device, and then it can drop setting cap->device_caps and cap->capabilities from the querycap implementation (it will be set by the v4l2 core based on the vdev->device_caps value). It definitely is a (surprising!) bug that V4L2_CAP_VIDEO_CAPTURE_MPLANE wasn't set here, but the real thing that you should check MPLANE capable drivers for is that they set vdev->device_caps. Regards, Hans > return 0; > } >
Hi, On 4/12/19 11:20, Boris Brezillon wrote: > The fimc-isp-video.c and fimc-lite.c were missing the > V4L2_CAP_VIDEO_CAPTURE_MPLANE flag when reporting device caps. Th omission was intentional, as these video nodes cannot be used by "standard" V4L2 capture applications. Some sub-devices need to be configured first in order to use these video nodes. It was agreed to not set these capabilities in the driver, instead user space library, e.g. libv4l2 with driver-specific plugin handling the media controller and v4l2 subdev calls would set the capability for the applications. Has something changed?
On Fri, 12 Apr 2019 12:16:42 +0200 Sylwester Nawrocki <s.nawrocki@samsung.com> wrote: > Hi, > > On 4/12/19 11:20, Boris Brezillon wrote: > > The fimc-isp-video.c and fimc-lite.c were missing the > > V4L2_CAP_VIDEO_CAPTURE_MPLANE flag when reporting device caps. > > Th omission was intentional, as these video nodes cannot be used > by "standard" V4L2 capture applications. Some sub-devices need to > be configured first in order to use these video nodes. It was > agreed to not set these capabilities in the driver, instead user > space library, e.g. libv4l2 with driver-specific plugin handling > the media controller and v4l2 subdev calls would set the capability > for the applications. Has something changed? > Hm, maybe I should just drop the extra consistency check I added in v4l_enum_fmt() and call ->vidioc_enum_fmt_vid_{cap,out}() unconditionally.
On 4/12/19 12:16 PM, Sylwester Nawrocki wrote: > Hi, > > On 4/12/19 11:20, Boris Brezillon wrote: >> The fimc-isp-video.c and fimc-lite.c were missing the >> V4L2_CAP_VIDEO_CAPTURE_MPLANE flag when reporting device caps. > > Th omission was intentional, as these video nodes cannot be used > by "standard" V4L2 capture applications. Some sub-devices need to > be configured first in order to use these video nodes. It was > agreed to not set these capabilities in the driver, instead user > space library, e.g. libv4l2 with driver-specific plugin handling > the media controller and v4l2 subdev calls would set the capability > for the applications. Has something changed? > I remember that this was discussed, but nobody actually followed that guideline. Just do: git grep -l V4L2_CAP_VIDEO_CAPTURE drivers/media/platform It really doesn't make sense to leave it out since userspace has to know if it is single or multiplanar, and if you don't set this capability, then how can it tell? I always thought we need a separate CAP or some other way of signaling this, but the few times I proposed something it was always shot down. So to be consistent with the other MC drivers you should set this MPLANE cap as well in the fimc driver. Regards, Hans
On 4/12/19 12:32, Hans Verkuil wrote: > On 4/12/19 12:16 PM, Sylwester Nawrocki wrote: >> On 4/12/19 11:20, Boris Brezillon wrote: >>> The fimc-isp-video.c and fimc-lite.c were missing the >>> V4L2_CAP_VIDEO_CAPTURE_MPLANE flag when reporting device caps. >> >> Th omission was intentional, as these video nodes cannot be used >> by "standard" V4L2 capture applications. Some sub-devices need to >> be configured first in order to use these video nodes. It was >> agreed to not set these capabilities in the driver, instead user >> space library, e.g. libv4l2 with driver-specific plugin handling >> the media controller and v4l2 subdev calls would set the capability >> for the applications. Has something changed? > > I remember that this was discussed, but nobody actually followed that > guideline. Just do: > > git grep -l V4L2_CAP_VIDEO_CAPTURE drivers/media/platform > > It really doesn't make sense to leave it out since userspace has to know if > it is single or multiplanar, and if you don't set this capability, then how > can it tell? Presumably you can't by only querying the video device, this would require MC and driver-specific user space. > I always thought we need a separate CAP or some other way of signaling this, > but the few times I proposed something it was always shot down. > > So to be consistent with the other MC drivers you should set this MPLANE cap > as well in the fimc driver. I'm not against the $subject patch, rather the opposite. Especially if it is a part of reasonable API updates. But removing the capabilities was actually a requirement to get the driver merged, not a guideline. And was rather painful then, now the driver is probably not in use any more so I don't really mind.
Hi Sylwester, On Fri, Apr 12, 2019 at 01:14:38PM +0200, Sylwester Nawrocki wrote: > On 4/12/19 12:32, Hans Verkuil wrote: > > On 4/12/19 12:16 PM, Sylwester Nawrocki wrote: > >> On 4/12/19 11:20, Boris Brezillon wrote: > >>> The fimc-isp-video.c and fimc-lite.c were missing the > >>> V4L2_CAP_VIDEO_CAPTURE_MPLANE flag when reporting device caps. > >> > >> Th omission was intentional, as these video nodes cannot be used > >> by "standard" V4L2 capture applications. Some sub-devices need to > >> be configured first in order to use these video nodes. It was > >> agreed to not set these capabilities in the driver, instead user > >> space library, e.g. libv4l2 with driver-specific plugin handling > >> the media controller and v4l2 subdev calls would set the capability > >> for the applications. Has something changed? > > > > I remember that this was discussed, but nobody actually followed that > > guideline. Just do: > > > > git grep -l V4L2_CAP_VIDEO_CAPTURE drivers/media/platform > > > > It really doesn't make sense to leave it out since userspace has to know if > > it is single or multiplanar, and if you don't set this capability, then how > > can it tell? > > Presumably you can't by only querying the video device, this would require > MC and driver-specific user space. > > > I always thought we need a separate CAP or some other way of signaling this, > > but the few times I proposed something it was always shot down. > > > > So to be consistent with the other MC drivers you should set this MPLANE cap > > as well in the fimc driver. > > I'm not against the $subject patch, rather the opposite. Especially if > it is a part of reasonable API updates. But removing the capabilities was > actually a requirement to get the driver merged, not a guideline. > And was rather painful then, now the driver is probably not in use any more > so I don't really mind. As this driver is quite old I assume there's little interest in investing in it, but is there any chance Samsung would be interested in contributing to libcamera for newer generations ? :-)
Hi Laurent, On 4/19/19 00:35, Laurent Pinchart wrote: > As this driver is quite old I assume there's little interest in > investing in it, but is there any chance Samsung would be interested in > contributing to libcamera for newer generations ? :-) I'm not aware of such plans at the moment, I assume the prerequisite would be mainline drivers for the camera subsystem which in turn requires whole SoC support in the mainline tree. And there have been quite a few newer Exynos SoC released for which I haven't seen efforts of adding mainline support. Of course things may change any time and I may not know everything so nothing is impossible.
diff --git a/drivers/media/platform/exynos4-is/fimc-isp-video.c b/drivers/media/platform/exynos4-is/fimc-isp-video.c index bb35a2017f21..8cd7b1c6565c 100644 --- a/drivers/media/platform/exynos4-is/fimc-isp-video.c +++ b/drivers/media/platform/exynos4-is/fimc-isp-video.c @@ -349,7 +349,9 @@ static int isp_video_querycap(struct file *file, void *priv, { struct fimc_isp *isp = video_drvdata(file); - __fimc_vidioc_querycap(&isp->pdev->dev, cap, V4L2_CAP_STREAMING); + __fimc_vidioc_querycap(&isp->pdev->dev, cap, + V4L2_CAP_STREAMING | + V4L2_CAP_VIDEO_CAPTURE_MPLANE); return 0; } diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c index 96f0a8a0dcae..13dfa5e39139 100644 --- a/drivers/media/platform/exynos4-is/fimc-lite.c +++ b/drivers/media/platform/exynos4-is/fimc-lite.c @@ -659,7 +659,7 @@ static int fimc_lite_querycap(struct file *file, void *priv, snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", dev_name(&fimc->pdev->dev)); - cap->device_caps = V4L2_CAP_STREAMING; + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_STREAMING; cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS; return 0; }
The fimc-isp-video.c and fimc-lite.c were missing the V4L2_CAP_VIDEO_CAPTURE_MPLANE flag when reporting device caps. Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> --- Changes in v3: - New patch --- drivers/media/platform/exynos4-is/fimc-isp-video.c | 4 +++- drivers/media/platform/exynos4-is/fimc-lite.c | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-)