[PATCH 1/2] Revert "media: camss: Make use of V4L2_CAP_IO_MC"

[PATCH 1/2] Revert "media: camss: Make use of V4L2_CAP_IO_MC"

[Andrey Konovalov]

This reverts commit c90f1178dcac30dee5ddd29ec0513e7589aa866e.

The assumption of "Each entry in formats[] table has unique mbus_code"
is valid for the RDI entities, but not for the PIX ones.

Reverting this patch and creating a new one which handles the PIX entities
correctly results in smaller and more straitforward patch than doing the
changes on top of the current version.

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 .../media/platform/qcom/camss/camss-video.c   | 67 ++++---------------
 1 file changed, 13 insertions(+), 54 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
index 114c3ae4a4ab..20673de9020e 100644
--- a/drivers/media/platform/qcom/camss/camss-video.c
+++ b/drivers/media/platform/qcom/camss/camss-video.c
@@ -535,16 +535,17 @@ static int video_querycap(struct file *file, void *fh,
 	return 0;
 }
 
-/*
- *  Returns the index in the video->formats[] array of the element which
- *  has the "ndx"th unique value of pixelformat field.
- *  If not found (no more unique pixelformat's) returns -EINVAL.
- */
-static int video_get_unique_pixelformat_by_index(struct camss_video *video,
-						 int ndx)
+static int video_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
 {
+	struct camss_video *video = video_drvdata(file);
 	int i, j, k;
 
+	if (f->type != video->type)
+		return -EINVAL;
+
+	if (f->index >= video->nformats)
+		return -EINVAL;
+
 	/* find index "i" of "k"th unique pixelformat in formats array */
 	k = -1;
 	for (i = 0; i < video->nformats; i++) {
@@ -557,53 +558,11 @@ static int video_get_unique_pixelformat_by_index(struct camss_video *video,
 		if (j == i)
 			k++;
 
-		if (k == ndx)
-			return i;
-	}
-
-	return -EINVAL;
-}
-
-/*
- *  Returns the index in the video->formats[] array of the element which
- *  has code equal to mcode.
- *  If not found returns -EINVAL.
- */
-static int video_get_pixelformat_by_mbus_code(struct camss_video *video,
-					      u32 mcode)
-{
-	int i;
-
-	for (i = 0; i < video->nformats; i++) {
-		if (video->formats[i].code == mcode)
-			return i;
-	}
-
-	return -EINVAL;
-}
-
-static int video_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
-{
-	struct camss_video *video = video_drvdata(file);
-	int i;
-
-	if (f->type != video->type)
-		return -EINVAL;
-
-	if (f->index >= video->nformats)
-		return -EINVAL;
-
-	if (f->mbus_code) {
-		/* Each entry in formats[] table has unique mbus_code */
-		if (f->index > 0)
-			return -EINVAL;
-
-		i = video_get_pixelformat_by_mbus_code(video, f->mbus_code);
-	} else {
-		i = video_get_unique_pixelformat_by_index(video, f->index);
+		if (k == f->index)
+			break;
 	}
 
-	if (i < 0)
+	if (k < f->index)
 		return -EINVAL;
 
 	f->pixelformat = video->formats[i].pixelformat;
@@ -989,8 +948,8 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
 	}
 
 	vdev->fops = &msm_vid_fops;
-	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_STREAMING
-			  | V4L2_CAP_READWRITE | V4L2_CAP_IO_MC;
+	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_STREAMING |
+							V4L2_CAP_READWRITE;
 	vdev->ioctl_ops = &msm_vid_ioctl_ops;
 	vdev->release = msm_video_release;
 	vdev->v4l2_dev = v4l2_dev;
-- 
2.17.1

[PATCH 2/2] media: camss: Make use of V4L2_CAP_IO_MC

[Andrey Konovalov]

Implement mbus_code filtering for format enumeration.

Without this patch libcamera errors out with:
"ERROR V4L2 v4l2_videodevice.cpp:982 /dev/video0[cap]: Media bus code
filtering not supported by the device"

This is the second version of this change which handles the case of
several pixel formats corresponding to one media bus format correctly.

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-video.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
index 20673de9020e..60737b771d52 100644
--- a/drivers/media/platform/qcom/camss/camss-video.c
+++ b/drivers/media/platform/qcom/camss/camss-video.c
@@ -539,6 +539,7 @@ static int video_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
 {
 	struct camss_video *video = video_drvdata(file);
 	int i, j, k;
+	u32 mcode = f->mbus_code;
 
 	if (f->type != video->type)
 		return -EINVAL;
@@ -549,7 +550,12 @@ static int video_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
 	/* find index "i" of "k"th unique pixelformat in formats array */
 	k = -1;
 	for (i = 0; i < video->nformats; i++) {
+		if (mcode != 0 && video->formats[i].code != mcode)
+			continue;
+
 		for (j = 0; j < i; j++) {
+			if (mcode != 0 && video->formats[j].code != mcode)
+				continue;
 			if (video->formats[i].pixelformat ==
 					video->formats[j].pixelformat)
 				break;
@@ -948,8 +954,8 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
 	}
 
 	vdev->fops = &msm_vid_fops;
-	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_STREAMING |
-							V4L2_CAP_READWRITE;
+	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_STREAMING
+			  | V4L2_CAP_READWRITE | V4L2_CAP_IO_MC;
 	vdev->ioctl_ops = &msm_vid_ioctl_ops;
 	vdev->release = msm_video_release;
 	vdev->v4l2_dev = v4l2_dev;
-- 
2.17.1

Re: [PATCH 1/2] Revert "media: camss: Make use of V4L2_CAP_IO_MC"

[Marc Gonzalez]

On 25/11/2020 13:37, Andrey Konovalov wrote:

> This reverts commit c90f1178dcac30dee5ddd29ec0513e7589aa866e.
> 
> The assumption of "Each entry in formats[] table has unique mbus_code"
> is valid for the RDI entities, but not for the PIX ones.
> 
> Reverting this patch and creating a new one which handles the PIX entities
> correctly results in smaller and more straitforward patch than doing the
> changes on top of the current version.


s/straitforward/straightforward  ;-)

Re: [PATCH 1/2] Revert "media: camss: Make use of V4L2_CAP_IO_MC"

[Robert Foss]

Hey Andrey,

Thanks for finding this issue, reverting and coming up with a better
solution seems like a good idea.
With the above nit fix, feel free to add my r-b.

Reviewed-by: Robert Foss <robert.foss@linaro.org>

On Thu, 26 Nov 2020 at 11:20, Marc Gonzalez <marc.w.gonzalez@free.fr> wrote:
>
> On 25/11/2020 13:37, Andrey Konovalov wrote:
>
> > This reverts commit c90f1178dcac30dee5ddd29ec0513e7589aa866e.
> >
> > The assumption of "Each entry in formats[] table has unique mbus_code"
> > is valid for the RDI entities, but not for the PIX ones.
> >
> > Reverting this patch and creating a new one which handles the PIX entities
> > correctly results in smaller and more straitforward patch than doing the
> > changes on top of the current version.
>
>
> s/straitforward/straightforward  ;-)

Re: [PATCH 2/2] media: camss: Make use of V4L2_CAP_IO_MC

[Robert Foss]

Hey Andrey,

I've suggested a small change, with that feel free to add my r-b.

Reviewed-by: Robert Foss <robert.foss@linaro.org>

On Wed, 25 Nov 2020 at 13:37, Andrey Konovalov
<andrey.konovalov@linaro.org> wrote:
>
> Implement mbus_code filtering for format enumeration.
>
> Without this patch libcamera errors out with:
> "ERROR V4L2 v4l2_videodevice.cpp:982 /dev/video0[cap]: Media bus code
> filtering not supported by the device"
>
> This is the second version of this change which handles the case of
> several pixel formats corresponding to one media bus format correctly.
>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  drivers/media/platform/qcom/camss/camss-video.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
> index 20673de9020e..60737b771d52 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.c
> +++ b/drivers/media/platform/qcom/camss/camss-video.c
> @@ -539,6 +539,7 @@ static int video_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
>  {
>         struct camss_video *video = video_drvdata(file);
>         int i, j, k;
> +       u32 mcode = f->mbus_code;
>
>         if (f->type != video->type)
>                 return -EINVAL;
> @@ -549,7 +550,12 @@ static int video_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
>         /* find index "i" of "k"th unique pixelformat in formats array */

Maybe this is a good place to explain how mcode is used, and for which
extension it is required.

>         k = -1;
>         for (i = 0; i < video->nformats; i++) {
> +               if (mcode != 0 && video->formats[i].code != mcode)
> +                       continue;
> +
>                 for (j = 0; j < i; j++) {
> +                       if (mcode != 0 && video->formats[j].code != mcode)
> +                               continue;
>                         if (video->formats[i].pixelformat ==
>                                         video->formats[j].pixelformat)
>                                 break;
> @@ -948,8 +954,8 @@ int msm_video_register(struct camss_video *video, struct v4l2_device *v4l2_dev,
>         }
>
>         vdev->fops = &msm_vid_fops;
> -       vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_STREAMING |
> -                                                       V4L2_CAP_READWRITE;
> +       vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_STREAMING
> +                         | V4L2_CAP_READWRITE | V4L2_CAP_IO_MC;
>         vdev->ioctl_ops = &msm_vid_ioctl_ops;
>         vdev->release = msm_video_release;
>         vdev->v4l2_dev = v4l2_dev;
> --
> 2.17.1
>