[PATCH v2 1/2] media: coda: Fix reported H264 profile

[PATCH v2 1/2] media: coda: Fix reported H264 profile

[Fabio Estevam]

From: Nicolas Dufresne <nicolas.dufresne@collabora.com>

The CODA960 manual states that ASO/FMO features of baseline are not
supported, so for this reason this driver should only report
constrained baseline support.

This fixes negotiation issue with constrained baseline content
on GStreamer 1.17.1.

ASO/FMO features are unsupported for the encoder and untested for the
decoder because there is currently no userspace support. Neither GStreamer
parsers nor FFMPEG parsers support ASO/FMO.

Cc: stable@vger.kernel.org
Fixes: 42a68012e67c2 ("media: coda: add read-only h.264 decoder profile/level controls")
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Tested-by: Pascal Speck <kernel@iktek.de>
Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v1:
- Followed Phillip's suggestion to change the commit message to say that
ASO/FMO features are unsupported for the encoder and untested for the
decoder because there is no userspace support.
https://patchwork.kernel.org/project/linux-media/patch/20200717034923.219524-1-ezequiel@collabora.com/
- Added Pascal's Tested-by tag.

 drivers/media/platform/coda/coda-common.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index a57822b05070..53b2dd1b268c 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2344,8 +2344,8 @@ static void coda_encode_ctrls(struct coda_ctx *ctx)
 		V4L2_CID_MPEG_VIDEO_H264_CHROMA_QP_INDEX_OFFSET, -12, 12, 1, 0);
 	v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops,
 		V4L2_CID_MPEG_VIDEO_H264_PROFILE,
-		V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE, 0x0,
-		V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE);
+		V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE, 0x0,
+		V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE);
 	if (ctx->dev->devtype->product == CODA_HX4 ||
 	    ctx->dev->devtype->product == CODA_7541) {
 		v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops,
@@ -2426,7 +2426,7 @@ static void coda_decode_ctrls(struct coda_ctx *ctx)
 	ctx->h264_profile_ctrl = v4l2_ctrl_new_std_menu(&ctx->ctrls,
 		&coda_ctrl_ops, V4L2_CID_MPEG_VIDEO_H264_PROFILE,
 		V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
-		~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE) |
+		~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE) |
 		  (1 << V4L2_MPEG_VIDEO_H264_PROFILE_MAIN) |
 		  (1 << V4L2_MPEG_VIDEO_H264_PROFILE_HIGH)),
 		V4L2_MPEG_VIDEO_H264_PROFILE_HIGH);
-- 
2.25.1

[PATCH v2 2/2] media: coda: Add more H264 levels for CODA960

[Fabio Estevam]

From: Nicolas Dufresne <nicolas.dufresne@collabora.com>

This add H264 level 4.1, 4.2 and 5.0 to the list of supported formats.
While the hardware does not fully support these levels, it do support
most of them. The constraints on frame size and pixel formats already
cover the limitation.

This fixes negotiation of level on GStreamer 1.17.1.

Cc: stable@vger.kernel.org
Fixes: 42a68012e67c2 ("media: coda: add read-only h.264 decoder profile/level controls")
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Tested-by: Pascal Speck <kernel@iktek.de>
Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v1:
- None - only added Pascal's Tested-by tag.

 drivers/media/platform/coda/coda-common.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 53b2dd1b268c..f1234ad24f65 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2364,7 +2364,10 @@ static void coda_encode_ctrls(struct coda_ctx *ctx)
 			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_0) |
 			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_1) |
 			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_2) |
-			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_0)),
+			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_0) |
+			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_1) |
+			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_2) |
+			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_5_0)),
 			V4L2_MPEG_VIDEO_H264_LEVEL_4_0);
 	}
 	v4l2_ctrl_new_std(&ctx->ctrls, &coda_ctrl_ops,
@@ -2437,7 +2440,7 @@ static void coda_decode_ctrls(struct coda_ctx *ctx)
 	    ctx->dev->devtype->product == CODA_7541)
 		max = V4L2_MPEG_VIDEO_H264_LEVEL_4_0;
 	else if (ctx->dev->devtype->product == CODA_960)
-		max = V4L2_MPEG_VIDEO_H264_LEVEL_4_1;
+		max = V4L2_MPEG_VIDEO_H264_LEVEL_5_0;
 	else
 		return;
 	ctx->h264_level_ctrl = v4l2_ctrl_new_std_menu(&ctx->ctrls,
-- 
2.25.1

Re: [PATCH v2 1/2] media: coda: Fix reported H264 profile

[Philipp Zabel]

On Mi, 2022-03-09 at 11:33 -0300, Fabio Estevam wrote:
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> The CODA960 manual states that ASO/FMO features of baseline are not
> supported, so for this reason this driver should only report
> constrained baseline support.
> 
> This fixes negotiation issue with constrained baseline content
> on GStreamer 1.17.1.
> 
> ASO/FMO features are unsupported for the encoder and untested for the
> decoder because there is currently no userspace support. Neither
> GStreamer
> parsers nor FFMPEG parsers support ASO/FMO.
> 
> Cc: stable@vger.kernel.org
> Fixes: 42a68012e67c2 ("media: coda: add read-only h.264 decoder
> profile/level controls")
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Tested-by: Pascal Speck <kernel@iktek.de>
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
> Changes since v1:
> - Followed Phillip's suggestion to change the commit message to say

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH v2 2/2] media: coda: Add more H264 levels for CODA960

[Philipp Zabel]

On Mi, 2022-03-09 at 11:33 -0300, Fabio Estevam wrote:
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> This add H264 level 4.1, 4.2 and 5.0 to the list of supported
> formats.
> While the hardware does not fully support these levels, it do support
> most of them. The constraints on frame size and pixel formats already
> cover the limitation.
> 
> This fixes negotiation of level on GStreamer 1.17.1.
> 
> Cc: stable@vger.kernel.org
> Fixes: 42a68012e67c2 ("media: coda: add read-only h.264 decoder
> profile/level controls")
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Tested-by: Pascal Speck <kernel@iktek.de>
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
> Changes since v1:
> - None - only added Pascal's Tested-by tag.
> 
>  drivers/media/platform/coda/coda-common.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/coda/coda-common.c
> b/drivers/media/platform/coda/coda-common.c
> index 53b2dd1b268c..f1234ad24f65 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -2364,7 +2364,10 @@ static void coda_encode_ctrls(struct coda_ctx
> *ctx)
>                           (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_0) |
>                           (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_1) |
>                           (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_2) |
> -                         (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_0)),
> +                         (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_0) |
> +                         (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_1) |
> +                         (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_2) |
> +                         (1 << V4L2_MPEG_VIDEO_H264_LEVEL_5_0)),

I still think this is wrong [1], the vendor only advertises support for
level 4.0. At least level 5.0 must be dropped, as we don't support the
frame size requirement.

[1] https://lore.kernel.org/all/110980fea9c24ee449487b5d28822dccf7962494.camel@pengutronix.de/#t

>                         V4L2_MPEG_VIDEO_H264_LEVEL_4_0);
>         }
>         v4l2_ctrl_new_std(&ctx->ctrls, &coda_ctrl_ops,
> @@ -2437,7 +2440,7 @@ static void coda_decode_ctrls(struct coda_ctx
> *ctx)
>             ctx->dev->devtype->product == CODA_7541)
>                 max = V4L2_MPEG_VIDEO_H264_LEVEL_4_0;
>         else if (ctx->dev->devtype->product == CODA_960)
> -               max = V4L2_MPEG_VIDEO_H264_LEVEL_4_1;
> +               max = V4L2_MPEG_VIDEO_H264_LEVEL_5_0;

I'm not so sure about this one, but I think it is wrong as well, for
the same reason. Nicolas wanted to reduce this to level 4.2 in v2 [2].

[2] https://lore.kernel.org/all/f409d4ddad0a352ca7ec84699c94a64e5dbf0407.camel@collabora.com/

regards
Philipp

Re: [PATCH v2 2/2] media: coda: Add more H264 levels for CODA960

[Philipp Zabel]

Hi Fabio, Nicolas,

On Mi, 2022-03-09 at 17:20 +0100, Philipp Zabel wrote:
[...]
> I still think this is wrong [1], the vendor only advertises support
> for level 4.0. At least level 5.0 must be dropped, as we don't
> support the frame size requirement.

Looking at my notes, I've never seen the encoder produce streams with
levels 1.1, 1.2, 1.3, 2.1, 2.2, or 4.1. Has anybody else?
Level 4.2 streams can be produced though, just not at realtime speeds.

Also, this encoder control change has no effect unless max is changed
as well. I think it should look as follows:

 	if (ctx->dev->devtype->product == CODA_960) {
 		v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops,
 			V4L2_CID_MPEG_VIDEO_H264_LEVEL,
-			V4L2_MPEG_VIDEO_H264_LEVEL_4_0,
-			~((1 << V4L2_MPEG_VIDEO_H264_LEVEL_2_0) |
+			V4L2_MPEG_VIDEO_H264_LEVEL_4_2,
+			~((1 << V4L2_MPEG_VIDEO_H264_LEVEL_1_0) |
+			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_2_0) |
 			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_0) |
 			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_1) |
 			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_2) |
-			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_0)),
+			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_0) |
+			  (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_2)),
 			V4L2_MPEG_VIDEO_H264_LEVEL_4_0);
 	}
 	v4l2_ctrl_new_std(&ctx->ctrls, &coda_ctrl_ops,

regards
Philipp

Re: [PATCH v2 2/2] media: coda: Add more H264 levels for CODA960

[Fabio Estevam]

Hi Philipp,

On Wed, Mar 9, 2022 at 1:29 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:

> Looking at my notes, I've never seen the encoder produce streams with
> levels 1.1, 1.2, 1.3, 2.1, 2.2, or 4.1. Has anybody else?

On my tests, I only saw 3.1 and 3.2 levels.

> Level 4.2 streams can be produced though, just not at realtime speeds.
>
> Also, this encoder control change has no effect unless max is changed
> as well. I think it should look as follows:
>
>         if (ctx->dev->devtype->product == CODA_960) {
>                 v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops,
>                         V4L2_CID_MPEG_VIDEO_H264_LEVEL,
> -                       V4L2_MPEG_VIDEO_H264_LEVEL_4_0,
> -                       ~((1 << V4L2_MPEG_VIDEO_H264_LEVEL_2_0) |
> +                       V4L2_MPEG_VIDEO_H264_LEVEL_4_2,
> +                       ~((1 << V4L2_MPEG_VIDEO_H264_LEVEL_1_0) |
> +                         (1 << V4L2_MPEG_VIDEO_H264_LEVEL_2_0) |
>                           (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_0) |
>                           (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_1) |
>                           (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_2) |
> -                         (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_0)),
> +                         (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_0) |
> +                         (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_2)),
>                         V4L2_MPEG_VIDEO_H264_LEVEL_4_0);
>         }
>         v4l2_ctrl_new_std(&ctx->ctrls, &coda_ctrl_ops,

You are right.

With Nicolas' original patch I see the following levels being reported:

h264_level 0x00990a67 (menu)   : min=0 max=11 default=11 value=11
5: 2
8: 3
9: 3.1
10: 3.2
11: 4

With your proposal I get:

 h264_level 0x00990a67 (menu)   : min=0 max=13 default=11 value=11
0: 1
5: 2
8: 3
9: 3.1
10: 3.2
11: 4
13: 4.2

I will submit a v3 with your proposal.

Thanks