[PATCH 0/3] cedrus: v4l2-compliance fixes

[PATCH 0/3] cedrus: v4l2-compliance fixes

[Hans Verkuil]

When testing the cedrus driver on my Cubieboard I noticed
various v4l2-compliance failures. This series fixes them.

Not tested with the -s option since v4l2-compliance doesn't yet
support stateless MPEG-2/H.264 decoders.

Regards,

	Hans

Hans Verkuil (3):
  cedrus: fill in bus_info for media device
  cedrus: choose default pixelformat in try_fmt
  cedrus: fix various format-related compliance issues

 drivers/staging/media/sunxi/cedrus/cedrus.c   | 12 +++
 .../staging/media/sunxi/cedrus/cedrus_video.c | 74 ++++++-------------
 .../staging/media/sunxi/cedrus/cedrus_video.h |  1 +
 3 files changed, 36 insertions(+), 51 deletions(-)

-- 
2.23.0.rc1

[PATCH 1/3] cedrus: fill in bus_info for media device

[Hans Verkuil]

Fixes this compliance warning:

$ v4l2-compliance -m0
v4l2-compliance SHA: b514d615166bdc0901a4c71261b87db31e89f464, 32 bits

Compliance test for cedrus device /dev/media0:

Media Driver Info:
        Driver name      : cedrus
        Model            : cedrus
        Serial           :
        Bus info         :
        Media version    : 5.3.0
        Hardware revision: 0x00000000 (0)
        Driver version   : 5.3.0

Required ioctls:
                warn: v4l2-test-media.cpp(51): empty bus_info
        test MEDIA_IOC_DEVICE_INFO: OK

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/staging/media/sunxi/cedrus/cedrus.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 2d3ea8b74dfd..3439f6ad6338 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -357,6 +357,8 @@ static int cedrus_probe(struct platform_device *pdev)
 
 	dev->mdev.dev = &pdev->dev;
 	strscpy(dev->mdev.model, CEDRUS_NAME, sizeof(dev->mdev.model));
+	strscpy(dev->mdev.bus_info, "platform:" CEDRUS_NAME,
+		sizeof(dev->mdev.bus_info));
 
 	media_device_init(&dev->mdev);
 	dev->mdev.ops = &cedrus_m2m_media_ops;
-- 
2.23.0.rc1

Re: [PATCH 1/3] cedrus: fill in bus_info for media device

[Jernej Škrabec]

Hi!

Dne petek, 30. avgust 2019 ob 11:26:22 CEST je Hans Verkuil napisal(a):
> Fixes this compliance warning:
> 
> $ v4l2-compliance -m0
> v4l2-compliance SHA: b514d615166bdc0901a4c71261b87db31e89f464, 32 bits
> 
> Compliance test for cedrus device /dev/media0:
> 
> Media Driver Info:
>         Driver name      : cedrus
>         Model            : cedrus
>         Serial           :
>         Bus info         :
>         Media version    : 5.3.0
>         Hardware revision: 0x00000000 (0)
>         Driver version   : 5.3.0
> 
> Required ioctls:
>                 warn: v4l2-test-media.cpp(51): empty bus_info
>         test MEDIA_IOC_DEVICE_INFO: OK
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Jernej Skrabec <jernej.skrabec@siol.net>

Best regards,
Jernej

[PATCH 2/3] cedrus: choose default pixelformat in try_fmt

[Hans Verkuil]

If an unsupported pixelformat is passed to try_fmt, then pick
the first valid pixelformat instead. This is more standard V4L2
behavior.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 .../staging/media/sunxi/cedrus/cedrus_video.c | 46 ++++++++-----------
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index eeee3efd247b..d69c9bcdb8e2 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -62,33 +62,30 @@ static inline struct cedrus_ctx *cedrus_file2ctx(struct file *file)
 static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions,
 						unsigned int capabilities)
 {
+	struct cedrus_format *first_valid_fmt = NULL;
 	struct cedrus_format *fmt;
 	unsigned int i;
 
 	for (i = 0; i < CEDRUS_FORMATS_COUNT; i++) {
 		fmt = &cedrus_formats[i];
 
-		if (fmt->capabilities && (fmt->capabilities & capabilities) !=
-		    fmt->capabilities)
+		if ((fmt->capabilities & capabilities) != fmt->capabilities ||
+		    !(fmt->directions & directions))
 			continue;
 
-		if (fmt->pixelformat == pixelformat &&
-		    (fmt->directions & directions) != 0)
+		if (fmt->pixelformat == pixelformat)
 			break;
+
+		if (!first_valid_fmt)
+			first_valid_fmt = fmt;
 	}
 
 	if (i == CEDRUS_FORMATS_COUNT)
-		return NULL;
+		return first_valid_fmt;
 
 	return &cedrus_formats[i];
 }
 
-static bool cedrus_check_format(u32 pixelformat, u32 directions,
-				unsigned int capabilities)
-{
-	return cedrus_find_format(pixelformat, directions, capabilities);
-}
-
 static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt)
 {
 	unsigned int width = pix_fmt->width;
@@ -252,11 +249,14 @@ static int cedrus_try_fmt_vid_cap(struct file *file, void *priv,
 	struct cedrus_ctx *ctx = cedrus_file2ctx(file);
 	struct cedrus_dev *dev = ctx->dev;
 	struct v4l2_pix_format *pix_fmt = &f->fmt.pix;
+	struct cedrus_format *fmt =
+		cedrus_find_format(pix_fmt->pixelformat, CEDRUS_DECODE_DST,
+				   dev->capabilities);
 
-	if (!cedrus_check_format(pix_fmt->pixelformat, CEDRUS_DECODE_DST,
-				 dev->capabilities))
+	if (!fmt)
 		return -EINVAL;
 
+	pix_fmt->pixelformat = fmt->pixelformat;
 	cedrus_prepare_format(pix_fmt);
 
 	return 0;
@@ -268,15 +268,18 @@ static int cedrus_try_fmt_vid_out(struct file *file, void *priv,
 	struct cedrus_ctx *ctx = cedrus_file2ctx(file);
 	struct cedrus_dev *dev = ctx->dev;
 	struct v4l2_pix_format *pix_fmt = &f->fmt.pix;
+	struct cedrus_format *fmt =
+		cedrus_find_format(pix_fmt->pixelformat, CEDRUS_DECODE_SRC,
+				   dev->capabilities);
 
-	if (!cedrus_check_format(pix_fmt->pixelformat, CEDRUS_DECODE_SRC,
-				 dev->capabilities))
+	if (!fmt)
 		return -EINVAL;
 
 	/* Source image size has to be provided by userspace. */
 	if (pix_fmt->sizeimage == 0)
 		return -EINVAL;
 
+	pix_fmt->pixelformat = fmt->pixelformat;
 	cedrus_prepare_format(pix_fmt);
 
 	return 0;
@@ -364,21 +367,12 @@ static int cedrus_queue_setup(struct vb2_queue *vq, unsigned int *nbufs,
 			      struct device *alloc_devs[])
 {
 	struct cedrus_ctx *ctx = vb2_get_drv_priv(vq);
-	struct cedrus_dev *dev = ctx->dev;
 	struct v4l2_pix_format *pix_fmt;
-	u32 directions;
 
-	if (V4L2_TYPE_IS_OUTPUT(vq->type)) {
-		directions = CEDRUS_DECODE_SRC;
+	if (V4L2_TYPE_IS_OUTPUT(vq->type))
 		pix_fmt = &ctx->src_fmt;
-	} else {
-		directions = CEDRUS_DECODE_DST;
+	else
 		pix_fmt = &ctx->dst_fmt;
-	}
-
-	if (!cedrus_check_format(pix_fmt->pixelformat, directions,
-				 dev->capabilities))
-		return -EINVAL;
 
 	if (*nplanes) {
 		if (sizes[0] < pix_fmt->sizeimage)
-- 
2.23.0.rc1

Re: [PATCH 2/3] cedrus: choose default pixelformat in try_fmt

[Jernej Škrabec]

Hi!

Dne petek, 30. avgust 2019 ob 11:26:23 CEST je Hans Verkuil napisal(a):
> If an unsupported pixelformat is passed to try_fmt, then pick
> the first valid pixelformat instead. This is more standard V4L2
> behavior.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  .../staging/media/sunxi/cedrus/cedrus_video.c | 46 ++++++++-----------
>  1 file changed, 20 insertions(+), 26 deletions(-)

Reviewed-by: Jernej Skrabec <jernej.skrabec@siol.net>

Best regards,
Jernej

[PATCH 3/3] cedrus: fix various format-related compliance issues

[Hans Verkuil]

Initialize the context on open() with valid capture and output
formats. It is good practice to always have valid formats internally.

This solves one vb2 warning in the kernel log where the sizeimage
value of the output format was 0 when requesting buffers, which is
not allowed.

It also simplifies the g_fmt ioctl implementations since they no longer
have to check if a valid format was ever set.

cedrus_prepare_format() now also validates sizeimage for the output
formats, ensuring userspace can't set it to 0 since that would cause
the same vb2 warning.

Finally remove the sizeimage == 0 check in cedrus_try_fmt_vid_out()
since cedrus_prepare_format() will now adjust this value.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/staging/media/sunxi/cedrus/cedrus.c   | 10 +++++++
 .../staging/media/sunxi/cedrus/cedrus_video.c | 28 ++-----------------
 .../staging/media/sunxi/cedrus/cedrus_video.h |  1 +
 3 files changed, 14 insertions(+), 25 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 3439f6ad6338..0cf637c8a1e3 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -241,6 +241,16 @@ static int cedrus_open(struct file *file)
 		ret = PTR_ERR(ctx->fh.m2m_ctx);
 		goto err_ctrls;
 	}
+	ctx->dst_fmt.pixelformat = V4L2_PIX_FMT_SUNXI_TILED_NV12;
+	cedrus_prepare_format(&ctx->dst_fmt);
+	ctx->src_fmt.pixelformat = V4L2_PIX_FMT_MPEG2_SLICE;
+	/*
+	 * TILED_NV12 has more strict requirements, so copy the width and
+	 * height to src_fmt to ensure that is matches the dst_fmt resolution.
+	 */
+	ctx->src_fmt.width = ctx->dst_fmt.width;
+	ctx->src_fmt.height = ctx->dst_fmt.height;
+	cedrus_prepare_format(&ctx->src_fmt);
 
 	v4l2_fh_add(&ctx->fh);
 
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index d69c9bcdb8e2..3ec3a2db790c 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -86,7 +86,7 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions,
 	return &cedrus_formats[i];
 }
 
-static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt)
+void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt)
 {
 	unsigned int width = pix_fmt->width;
 	unsigned int height = pix_fmt->height;
@@ -104,7 +104,8 @@ static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt)
 	case V4L2_PIX_FMT_H264_SLICE:
 		/* Zero bytes per line for encoded source. */
 		bytesperline = 0;
-
+		/* Choose some minimum size since this can't be 0 */
+		sizeimage = max_t(u32, SZ_1K, sizeimage);
 		break;
 
 	case V4L2_PIX_FMT_SUNXI_TILED_NV12:
@@ -211,16 +212,7 @@ static int cedrus_g_fmt_vid_cap(struct file *file, void *priv,
 {
 	struct cedrus_ctx *ctx = cedrus_file2ctx(file);
 
-	/* Fall back to dummy default by lack of hardware configuration. */
-	if (!ctx->dst_fmt.width || !ctx->dst_fmt.height) {
-		f->fmt.pix.pixelformat = V4L2_PIX_FMT_SUNXI_TILED_NV12;
-		cedrus_prepare_format(&f->fmt.pix);
-
-		return 0;
-	}
-
 	f->fmt.pix = ctx->dst_fmt;
-
 	return 0;
 }
 
@@ -229,17 +221,7 @@ static int cedrus_g_fmt_vid_out(struct file *file, void *priv,
 {
 	struct cedrus_ctx *ctx = cedrus_file2ctx(file);
 
-	/* Fall back to dummy default by lack of hardware configuration. */
-	if (!ctx->dst_fmt.width || !ctx->dst_fmt.height) {
-		f->fmt.pix.pixelformat = V4L2_PIX_FMT_MPEG2_SLICE;
-		f->fmt.pix.sizeimage = SZ_1K;
-		cedrus_prepare_format(&f->fmt.pix);
-
-		return 0;
-	}
-
 	f->fmt.pix = ctx->src_fmt;
-
 	return 0;
 }
 
@@ -275,10 +257,6 @@ static int cedrus_try_fmt_vid_out(struct file *file, void *priv,
 	if (!fmt)
 		return -EINVAL;
 
-	/* Source image size has to be provided by userspace. */
-	if (pix_fmt->sizeimage == 0)
-		return -EINVAL;
-
 	pix_fmt->pixelformat = fmt->pixelformat;
 	cedrus_prepare_format(pix_fmt);
 
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.h b/drivers/staging/media/sunxi/cedrus/cedrus_video.h
index 0e4f7a8cccf2..05050c0a0921 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.h
@@ -26,5 +26,6 @@ extern const struct v4l2_ioctl_ops cedrus_ioctl_ops;
 
 int cedrus_queue_init(void *priv, struct vb2_queue *src_vq,
 		      struct vb2_queue *dst_vq);
+void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt);
 
 #endif
-- 
2.23.0.rc1

Re: [PATCH 3/3] cedrus: fix various format-related compliance issues

[Jernej Škrabec]

Hi!

Dne petek, 30. avgust 2019 ob 11:26:24 CEST je Hans Verkuil napisal(a):
> Initialize the context on open() with valid capture and output
> formats. It is good practice to always have valid formats internally.
> 
> This solves one vb2 warning in the kernel log where the sizeimage
> value of the output format was 0 when requesting buffers, which is
> not allowed.
> 
> It also simplifies the g_fmt ioctl implementations since they no longer
> have to check if a valid format was ever set.
> 
> cedrus_prepare_format() now also validates sizeimage for the output
> formats, ensuring userspace can't set it to 0 since that would cause
> the same vb2 warning.
> 
> Finally remove the sizeimage == 0 check in cedrus_try_fmt_vid_out()
> since cedrus_prepare_format() will now adjust this value.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.c   | 10 +++++++
>  .../staging/media/sunxi/cedrus/cedrus_video.c | 28 ++-----------------
>  .../staging/media/sunxi/cedrus/cedrus_video.h |  1 +
>  3 files changed, 14 insertions(+), 25 deletions(-)

Selecting default format needs to be expanded to select default format based 
on compatible. SoCs with Display Engine 2 and 3 don't know how to display 
tiled formats directly. But that's out of scope for this series.

Reviewed-by: Jernej Skrabec <jernej.skrabec@siol.net>

Best regards,
Jernej