LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 for 5.4 0/4] media: hantro: Collected fixes for v5.4
@ 2019-10-07 17:45 Ezequiel Garcia
  2019-10-07 17:45 ` [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes Ezequiel Garcia
                   ` (3 more replies)
  0 siblings, 4 replies; 28+ messages in thread
From: Ezequiel Garcia @ 2019-10-07 17:45 UTC (permalink / raw)
  To: linux-media
  Cc: kernel, Nicolas Dufresne, Tomasz Figa, linux-rockchip,
	Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, linux-kernel, Douglas Anderson,
	Ezequiel Garcia

Some pending fixes. The first patch is needed to allow
dynamic resolution changes, as per the upcoming stateless
decoder API. This patch was posted before and received
some comments from Philipp Zabel.

The second patch was posted by Jonas Karlman as part
of his series to add support for interlaced content.
The fix can be applied independently so I'm including it
here.

Patches 3 and 4 correspond to fixes found by Francois Buergisser
while doing some tests on ChromeOS.

Ezequiel Garcia (1):
  media: hantro: Fix s_fmt for dynamic resolution changes

Francois Buergisser (2):
  media: hantro: Fix motion vectors usage condition
  media: hantro: Fix picture order count table enable

Jonas Karlman (1):
  media: hantro: Fix H264 max frmsize supported on RK3288

 .../staging/media/hantro/hantro_g1_h264_dec.c |  6 ++--
 drivers/staging/media/hantro/hantro_v4l2.c    | 28 +++++++++++++------
 drivers/staging/media/hantro/rk3288_vpu_hw.c  |  4 +--
 3 files changed, 24 insertions(+), 14 deletions(-)

-- 
2.22.0


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes
  2019-10-07 17:45 [PATCH v2 for 5.4 0/4] media: hantro: Collected fixes for v5.4 Ezequiel Garcia
@ 2019-10-07 17:45 ` Ezequiel Garcia
  2019-11-08 10:19   ` Boris Brezillon
  2019-10-07 17:45 ` [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288 Ezequiel Garcia
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 28+ messages in thread
From: Ezequiel Garcia @ 2019-10-07 17:45 UTC (permalink / raw)
  To: linux-media
  Cc: kernel, Nicolas Dufresne, Tomasz Figa, linux-rockchip,
	Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, linux-kernel, Douglas Anderson,
	Ezequiel Garcia

Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders")
changed the conditions under S_FMT was allowed for OUTPUT
CAPTURE buffers.

However, and according to the mem-to-mem stateless decoder specification,
in order to support dynamic resolution changes, S_FMT should be allowed
even if OUTPUT buffers have been allocated.

Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a resolution
modification, provided the pixel format stays the same.

Tested on RK3288 platforms using ChromiumOS Video Decode/Encode Accelerator Unittests.

Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders")
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
--
v2:
* Call try_fmt_out before using the format,
  pointed out by Philipp.

 drivers/staging/media/hantro/hantro_v4l2.c | 28 +++++++++++++++-------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
index 3dae52abb96c..586d243cc3cc 100644
--- a/drivers/staging/media/hantro/hantro_v4l2.c
+++ b/drivers/staging/media/hantro/hantro_v4l2.c
@@ -367,19 +367,26 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
 {
 	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
 	struct hantro_ctx *ctx = fh_to_ctx(priv);
+	struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
 	const struct hantro_fmt *formats;
 	unsigned int num_fmts;
-	struct vb2_queue *vq;
 	int ret;
 
-	/* Change not allowed if queue is busy. */
-	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
-	if (vb2_is_busy(vq))
-		return -EBUSY;
+	ret = vidioc_try_fmt_out_mplane(file, priv, f);
+	if (ret)
+		return ret;
 
 	if (!hantro_is_encoder_ctx(ctx)) {
 		struct vb2_queue *peer_vq;
 
+		/*
+		 * In other to support dynamic resolution change,
+		 * the decoder admits a resolution change, as long
+		 * as the pixelformat remains. Can't be done if streaming.
+		 */
+		if (vb2_is_streaming(vq) || (vb2_is_busy(vq) &&
+		    pix_mp->pixelformat != ctx->src_fmt.pixelformat))
+			return -EBUSY;
 		/*
 		 * Since format change on the OUTPUT queue will reset
 		 * the CAPTURE queue, we can't allow doing so
@@ -389,12 +396,15 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
 					  V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
 		if (vb2_is_busy(peer_vq))
 			return -EBUSY;
+	} else {
+		/*
+		 * The encoder doesn't admit a format change if
+		 * there are OUTPUT buffers allocated.
+		 */
+		if (vb2_is_busy(vq))
+			return -EBUSY;
 	}
 
-	ret = vidioc_try_fmt_out_mplane(file, priv, f);
-	if (ret)
-		return ret;
-
 	formats = hantro_get_formats(ctx, &num_fmts);
 	ctx->vpu_src_fmt = hantro_find_format(formats, num_fmts,
 					      pix_mp->pixelformat);
-- 
2.22.0


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288
  2019-10-07 17:45 [PATCH v2 for 5.4 0/4] media: hantro: Collected fixes for v5.4 Ezequiel Garcia
  2019-10-07 17:45 ` [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes Ezequiel Garcia
@ 2019-10-07 17:45 ` Ezequiel Garcia
  2019-10-08  5:27   ` Tomasz Figa
  2019-10-07 17:45 ` [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition Ezequiel Garcia
  2019-10-07 17:45 ` [PATCH v2 for 5.4 4/4] media: hantro: Fix picture order count table enable Ezequiel Garcia
  3 siblings, 1 reply; 28+ messages in thread
From: Ezequiel Garcia @ 2019-10-07 17:45 UTC (permalink / raw)
  To: linux-media
  Cc: kernel, Nicolas Dufresne, Tomasz Figa, linux-rockchip,
	Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, linux-kernel, Douglas Anderson

From: Jonas Karlman <jonas@kwiboo.se>

TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
change frmsize max_width/max_height to match TRM.

Fixes: 760327930e10 ("media: hantro: Enable H264 decoding on rk3288")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v2:
* No changes.

 drivers/staging/media/hantro/rk3288_vpu_hw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
index 6bfcc47d1e58..ebb017b8a334 100644
--- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
+++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
@@ -67,10 +67,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
 		.max_depth = 2,
 		.frmsize = {
 			.min_width = 48,
-			.max_width = 3840,
+			.max_width = 4096,
 			.step_width = H264_MB_DIM,
 			.min_height = 48,
-			.max_height = 2160,
+			.max_height = 2304,
 			.step_height = H264_MB_DIM,
 		},
 	},
-- 
2.22.0


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition
  2019-10-07 17:45 [PATCH v2 for 5.4 0/4] media: hantro: Collected fixes for v5.4 Ezequiel Garcia
  2019-10-07 17:45 ` [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes Ezequiel Garcia
  2019-10-07 17:45 ` [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288 Ezequiel Garcia
@ 2019-10-07 17:45 ` Ezequiel Garcia
  2019-10-07 18:33   ` Jonas Karlman
  2019-10-07 17:45 ` [PATCH v2 for 5.4 4/4] media: hantro: Fix picture order count table enable Ezequiel Garcia
  3 siblings, 1 reply; 28+ messages in thread
From: Ezequiel Garcia @ 2019-10-07 17:45 UTC (permalink / raw)
  To: linux-media
  Cc: kernel, Nicolas Dufresne, Tomasz Figa, linux-rockchip,
	Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, linux-kernel, Douglas Anderson,
	Ezequiel Garcia

From: Francois Buergisser <fbuergisser@chromium.org>

The setting of the motion vectors usage and the setting of motion
vectors address are currently done under different conditions.

When decoding pre-recorded videos, this results of leaving the motion
vectors address unset, resulting in faulty memory accesses. Fix it
by using the same condition everywhere, which matches the profiles
that support motion vectors.

Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
Signed-off-by: Francois Buergisser <fbuergisser@chromium.org>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
v2:
* New patch.

 drivers/staging/media/hantro/hantro_g1_h264_dec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
index 7ab534936843..c92460407613 100644
--- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
@@ -35,7 +35,7 @@ static void set_params(struct hantro_ctx *ctx)
 	if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
 		reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E;
 	reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E;
-	if (dec_param->nal_ref_idc)
+	if (sps->profile_idc > 66)
 		reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E;
 
 	if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&
-- 
2.22.0


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 for 5.4 4/4] media: hantro: Fix picture order count table enable
  2019-10-07 17:45 [PATCH v2 for 5.4 0/4] media: hantro: Collected fixes for v5.4 Ezequiel Garcia
                   ` (2 preceding siblings ...)
  2019-10-07 17:45 ` [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition Ezequiel Garcia
@ 2019-10-07 17:45 ` Ezequiel Garcia
  3 siblings, 0 replies; 28+ messages in thread
From: Ezequiel Garcia @ 2019-10-07 17:45 UTC (permalink / raw)
  To: linux-media
  Cc: kernel, Nicolas Dufresne, Tomasz Figa, linux-rockchip,
	Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, linux-kernel, Douglas Anderson,
	Ezequiel Garcia

From: Francois Buergisser <fbuergisser@chromium.org>

The picture order count table only makes sense for profiles
higher than Baseline. This is confirmed by the H.264 specification
(See 8.2.1 Decoding process for picture order count), which
clarifies how POC are used for features not present in Baseline.

"""
Picture order counts are used to determine initial picture orderings
for reference pictures in the decoding of B slices, to represent picture
order differences between frames or fields for motion vector derivation
in temporal direct mode, for implicit mode weighted prediction in B slices,
and for decoder conformance checking.
"""

As a side note, this change matches various vendors downstream codebases,
including ChromiumOS and IMX VPU libraries.

Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
Signed-off-by: Francois Buergisser <fbuergisser@chromium.org>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
v2:
* New patch.

 drivers/staging/media/hantro/hantro_g1_h264_dec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
index c92460407613..05576dbd39e2 100644
--- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
@@ -34,9 +34,9 @@ static void set_params(struct hantro_ctx *ctx)
 	reg = G1_REG_DEC_CTRL0_DEC_AXI_WR_ID(0x0);
 	if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
 		reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E;
-	reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E;
 	if (sps->profile_idc > 66)
-		reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E;
+		reg = G1_REG_DEC_CTRL0_PICORD_COUNT_E |
+		      G1_REG_DEC_CTRL0_WRITE_MVS_E;
 
 	if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&
 	    (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD ||
-- 
2.22.0


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition
  2019-10-07 17:45 ` [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition Ezequiel Garcia
@ 2019-10-07 18:33   ` Jonas Karlman
  2019-10-08  3:29     ` Tomasz Figa
  0 siblings, 1 reply; 28+ messages in thread
From: Jonas Karlman @ 2019-10-07 18:33 UTC (permalink / raw)
  To: Ezequiel Garcia, linux-media
  Cc: kernel, Nicolas Dufresne, Tomasz Figa, linux-rockchip,
	Heiko Stuebner, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, linux-kernel, Douglas Anderson

On 2019-10-07 19:45, Ezequiel Garcia wrote:
> From: Francois Buergisser <fbuergisser@chromium.org>
>
> The setting of the motion vectors usage and the setting of motion
> vectors address are currently done under different conditions.
>
> When decoding pre-recorded videos, this results of leaving the motion
> vectors address unset, resulting in faulty memory accesses. Fix it
> by using the same condition everywhere, which matches the profiles
> that support motion vectors.

This does not fully match hantro sdk:

  enable direct MV writing and POC tables for high/main streams.
  enable it also for any "baseline" stream which have main/high tools enabled.

  (sps->profile_idc > 66 && sps->constrained_set0_flag == 0) ||
  sps->frame_mbs_only_flag != 1 ||
  sps->chroma_format_idc != 1 ||
  sps->scaling_matrix_present_flag != 0 ||
  pps->entropy_coding_mode_flag != 0 ||
  pps->weighted_pred_flag != 0 ||
  pps->weighted_bi_pred_idc != 0 ||
  pps->transform8x8_flag != 0 ||
  pps->scaling_matrix_present_flag != 0

Above check is used when DIR_MV_BASE should be written.
And WRITE_MVS_E is set to nal_ref_idc != 0 when above is true.

I think it may be safer to always set DIR_MV_BASE and keep the existing nal_ref_idc check for WRITE_MVS_E.
(That is what I did in my "media: hantro: H264 fixes and improvements" series, v2 is incoming)
Or have you found any video that is having issues in such case?

Regards,
Jonas

>
> Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
> Signed-off-by: Francois Buergisser <fbuergisser@chromium.org>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
> v2:
> * New patch.
>
>  drivers/staging/media/hantro/hantro_g1_h264_dec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> index 7ab534936843..c92460407613 100644
> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> @@ -35,7 +35,7 @@ static void set_params(struct hantro_ctx *ctx)
>  	if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
>  		reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E;
>  	reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E;
> -	if (dec_param->nal_ref_idc)
> +	if (sps->profile_idc > 66)
>  		reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E;
>  
>  	if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition
  2019-10-07 18:33   ` Jonas Karlman
@ 2019-10-08  3:29     ` Tomasz Figa
  2019-10-08  6:23       ` Jonas Karlman
  0 siblings, 1 reply; 28+ messages in thread
From: Tomasz Figa @ 2019-10-08  3:29 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Ezequiel Garcia, linux-media, kernel, Nicolas Dufresne,
	linux-rockchip, Heiko Stuebner, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, linux-kernel, Douglas Anderson

Hi Jonas,

On Tue, Oct 8, 2019 at 3:33 AM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> On 2019-10-07 19:45, Ezequiel Garcia wrote:
> > From: Francois Buergisser <fbuergisser@chromium.org>
> >
> > The setting of the motion vectors usage and the setting of motion
> > vectors address are currently done under different conditions.
> >
> > When decoding pre-recorded videos, this results of leaving the motion
> > vectors address unset, resulting in faulty memory accesses. Fix it
> > by using the same condition everywhere, which matches the profiles
> > that support motion vectors.
>
> This does not fully match hantro sdk:
>
>   enable direct MV writing and POC tables for high/main streams.
>   enable it also for any "baseline" stream which have main/high tools enabled.
>
>   (sps->profile_idc > 66 && sps->constrained_set0_flag == 0) ||
>   sps->frame_mbs_only_flag != 1 ||
>   sps->chroma_format_idc != 1 ||
>   sps->scaling_matrix_present_flag != 0 ||
>   pps->entropy_coding_mode_flag != 0 ||
>   pps->weighted_pred_flag != 0 ||
>   pps->weighted_bi_pred_idc != 0 ||
>   pps->transform8x8_flag != 0 ||
>   pps->scaling_matrix_present_flag != 0

Thanks for double checking this. I can confirm that it's what Hantro
SDK does indeed.

However, would a stream with sps->profile_idc <= 66 and those other
conditions met be still a compliant stream?

>
> Above check is used when DIR_MV_BASE should be written.
> And WRITE_MVS_E is set to nal_ref_idc != 0 when above is true.
>
> I think it may be safer to always set DIR_MV_BASE and keep the existing nal_ref_idc check for WRITE_MVS_E.

That might have a performance penalty or some other side effects,
though. Otherwise Hantro SDK wouldn't have enable it conditionally.

> (That is what I did in my "media: hantro: H264 fixes and improvements" series, v2 is incoming)
> Or have you found any video that is having issues in such case?

We've been running the code with sps->profile_idc > 66 in production
for 4 years and haven't seen any reports of a stream that wasn't
decoded correctly.

If we decide to go with a different behavior, I'd suggest thoroughly
verifying the behavior on a big number of streams, including some
performance measurements.

Best regards,
Tomasz

>
> Regards,
> Jonas
>
> >
> > Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
> > Signed-off-by: Francois Buergisser <fbuergisser@chromium.org>
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> > v2:
> > * New patch.
> >
> >  drivers/staging/media/hantro/hantro_g1_h264_dec.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> > index 7ab534936843..c92460407613 100644
> > --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> > +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> > @@ -35,7 +35,7 @@ static void set_params(struct hantro_ctx *ctx)
> >       if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
> >               reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E;
> >       reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E;
> > -     if (dec_param->nal_ref_idc)
> > +     if (sps->profile_idc > 66)
> >               reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E;
> >
> >       if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288
  2019-10-07 17:45 ` [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288 Ezequiel Garcia
@ 2019-10-08  5:27   ` Tomasz Figa
  2019-10-08  6:31     ` Jonas Karlman
  0 siblings, 1 reply; 28+ messages in thread
From: Tomasz Figa @ 2019-10-08  5:27 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: Linux Media Mailing List, kernel, Nicolas Dufresne,
	open list:ARM/Rockchip SoC...,
	Heiko Stuebner, Jonas Karlman, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, Linux Kernel Mailing List,
	Douglas Anderson

Hi Ezequiel, Jonas,

On Tue, Oct 8, 2019 at 2:46 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> From: Jonas Karlman <jonas@kwiboo.se>
>
> TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
> change frmsize max_width/max_height to match TRM.
>
> Fixes: 760327930e10 ("media: hantro: Enable H264 decoding on rk3288")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> v2:
> * No changes.
>
>  drivers/staging/media/hantro/rk3288_vpu_hw.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> index 6bfcc47d1e58..ebb017b8a334 100644
> --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
> +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> @@ -67,10 +67,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
>                 .max_depth = 2,
>                 .frmsize = {
>                         .min_width = 48,
> -                       .max_width = 3840,
> +                       .max_width = 4096,
>                         .step_width = H264_MB_DIM,
>                         .min_height = 48,
> -                       .max_height = 2160,
> +                       .max_height = 2304,

This doesn't match the datasheet I have, which is RK3288 Datasheet Rev
1.4 and which has the values as in current code. What's the one you
got the values from?

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition
  2019-10-08  3:29     ` Tomasz Figa
@ 2019-10-08  6:23       ` Jonas Karlman
  2019-10-08 10:26         ` Tomasz Figa
  0 siblings, 1 reply; 28+ messages in thread
From: Jonas Karlman @ 2019-10-08  6:23 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Ezequiel Garcia, linux-media, kernel, Nicolas Dufresne,
	linux-rockchip, Heiko Stuebner, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, linux-kernel, Douglas Anderson

On 2019-10-08 05:29, Tomasz Figa wrote:
> Hi Jonas,
>
> On Tue, Oct 8, 2019 at 3:33 AM Jonas Karlman <jonas@kwiboo.se> wrote:
>> On 2019-10-07 19:45, Ezequiel Garcia wrote:
>>> From: Francois Buergisser <fbuergisser@chromium.org>
>>>
>>> The setting of the motion vectors usage and the setting of motion
>>> vectors address are currently done under different conditions.
>>>
>>> When decoding pre-recorded videos, this results of leaving the motion
>>> vectors address unset, resulting in faulty memory accesses. Fix it
>>> by using the same condition everywhere, which matches the profiles
>>> that support motion vectors.
>> This does not fully match hantro sdk:
>>
>>   enable direct MV writing and POC tables for high/main streams.
>>   enable it also for any "baseline" stream which have main/high tools enabled.
>>
>>   (sps->profile_idc > 66 && sps->constrained_set0_flag == 0) ||
>>   sps->frame_mbs_only_flag != 1 ||
>>   sps->chroma_format_idc != 1 ||
>>   sps->scaling_matrix_present_flag != 0 ||
>>   pps->entropy_coding_mode_flag != 0 ||
>>   pps->weighted_pred_flag != 0 ||
>>   pps->weighted_bi_pred_idc != 0 ||
>>   pps->transform8x8_flag != 0 ||
>>   pps->scaling_matrix_present_flag != 0
> Thanks for double checking this. I can confirm that it's what Hantro
> SDK does indeed.
>
> However, would a stream with sps->profile_idc <= 66 and those other
> conditions met be still a compliant stream?

You are correct, if a non-compliant video is having decoding problems it should probably be handled
on userspace side (by not reporting baseline profile) and not in kernel.
All my video samples that was having the issue fixed in this patch are now decoded correctly.

>
>> Above check is used when DIR_MV_BASE should be written.
>> And WRITE_MVS_E is set to nal_ref_idc != 0 when above is true.
>>
>> I think it may be safer to always set DIR_MV_BASE and keep the existing nal_ref_idc check for WRITE_MVS_E.
> That might have a performance penalty or some other side effects,
> though. Otherwise Hantro SDK wouldn't have enable it conditionally.
>
>> (That is what I did in my "media: hantro: H264 fixes and improvements" series, v2 is incoming)
>> Or have you found any video that is having issues in such case?
> We've been running the code with sps->profile_idc > 66 in production
> for 4 years and haven't seen any reports of a stream that wasn't
> decoded correctly.
>
> If we decide to go with a different behavior, I'd suggest thoroughly
> verifying the behavior on a big number of streams, including some
> performance measurements.

I agree, I would however suggest to change the if statement to the following (or similar)
as that should be the optimal for performance reasons and match the hantro sdk.

if (sps->profile_idc > 66 && dec_param->nal_ref_idc)

Regards,
Jonas

>
> Best regards,
> Tomasz
>
>> Regards,
>> Jonas
>>
>>> Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
>>> Signed-off-by: Francois Buergisser <fbuergisser@chromium.org>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>> ---
>>> v2:
>>> * New patch.
>>>
>>>  drivers/staging/media/hantro/hantro_g1_h264_dec.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
>>> index 7ab534936843..c92460407613 100644
>>> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
>>> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
>>> @@ -35,7 +35,7 @@ static void set_params(struct hantro_ctx *ctx)
>>>       if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
>>>               reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E;
>>>       reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E;
>>> -     if (dec_param->nal_ref_idc)
>>> +     if (sps->profile_idc > 66)
>>>               reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E;
>>>
>>>       if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288
  2019-10-08  5:27   ` Tomasz Figa
@ 2019-10-08  6:31     ` Jonas Karlman
  2019-10-08 10:42       ` Tomasz Figa
  0 siblings, 1 reply; 28+ messages in thread
From: Jonas Karlman @ 2019-10-08  6:31 UTC (permalink / raw)
  To: Tomasz Figa, Ezequiel Garcia
  Cc: Linux Media Mailing List, kernel, Nicolas Dufresne,
	open list:ARM/Rockchip SoC...,
	Heiko Stuebner, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, Linux Kernel Mailing List,
	Douglas Anderson

On 2019-10-08 07:27, Tomasz Figa wrote:
> Hi Ezequiel, Jonas,
>
> On Tue, Oct 8, 2019 at 2:46 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>> From: Jonas Karlman <jonas@kwiboo.se>
>>
>> TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
>> change frmsize max_width/max_height to match TRM.
>>
>> Fixes: 760327930e10 ("media: hantro: Enable H264 decoding on rk3288")
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>> v2:
>> * No changes.
>>
>>  drivers/staging/media/hantro/rk3288_vpu_hw.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
>> index 6bfcc47d1e58..ebb017b8a334 100644
>> --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
>> +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
>> @@ -67,10 +67,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
>>                 .max_depth = 2,
>>                 .frmsize = {
>>                         .min_width = 48,
>> -                       .max_width = 3840,
>> +                       .max_width = 4096,
>>                         .step_width = H264_MB_DIM,
>>                         .min_height = 48,
>> -                       .max_height = 2160,
>> +                       .max_height = 2304,
> This doesn't match the datasheet I have, which is RK3288 Datasheet Rev
> 1.4 and which has the values as in current code. What's the one you
> got the values from?

The RK3288 TRM vcodec chapter from [1], unknown revision and date, lists 48x48 to 4096x2304 step size 16 pixels under 25.5.1 H.264 decoder.

I can also confirm that one of my test samples (PUPPIES BATH IN 4K) is 4096x2304 and can be decoded after this patch.
However the decoding speed is not optimal at 400Mhz, if I recall correctly you need to set the VPU1 clock to 600Mhz for 4K decoding on RK3288.

I am not sure if I should include a v2 of this patch in my v2 series, as-is this patch do not apply on master (H264_MB_DIM has changed to MB_DIM in master).

[1] http://www.t-firefly.com/download/firefly-rk3288/docs/TRM/rk3288-chapter-25-video-encoder-decoder-unit-(vcodec).pdf

Regards,
Jonas

>
> Best regards,
> Tomasz


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition
  2019-10-08  6:23       ` Jonas Karlman
@ 2019-10-08 10:26         ` Tomasz Figa
  2019-10-08 13:57           ` Jonas Karlman
  0 siblings, 1 reply; 28+ messages in thread
From: Tomasz Figa @ 2019-10-08 10:26 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Ezequiel Garcia, linux-media, kernel, Nicolas Dufresne,
	linux-rockchip, Heiko Stuebner, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, linux-kernel, Douglas Anderson

On Tue, Oct 8, 2019 at 3:23 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> On 2019-10-08 05:29, Tomasz Figa wrote:
> > Hi Jonas,
> >
> > On Tue, Oct 8, 2019 at 3:33 AM Jonas Karlman <jonas@kwiboo.se> wrote:
> >> On 2019-10-07 19:45, Ezequiel Garcia wrote:
> >>> From: Francois Buergisser <fbuergisser@chromium.org>
> >>>
> >>> The setting of the motion vectors usage and the setting of motion
> >>> vectors address are currently done under different conditions.
> >>>
> >>> When decoding pre-recorded videos, this results of leaving the motion
> >>> vectors address unset, resulting in faulty memory accesses. Fix it
> >>> by using the same condition everywhere, which matches the profiles
> >>> that support motion vectors.
> >> This does not fully match hantro sdk:
> >>
> >>   enable direct MV writing and POC tables for high/main streams.
> >>   enable it also for any "baseline" stream which have main/high tools enabled.
> >>
> >>   (sps->profile_idc > 66 && sps->constrained_set0_flag == 0) ||
> >>   sps->frame_mbs_only_flag != 1 ||
> >>   sps->chroma_format_idc != 1 ||
> >>   sps->scaling_matrix_present_flag != 0 ||
> >>   pps->entropy_coding_mode_flag != 0 ||
> >>   pps->weighted_pred_flag != 0 ||
> >>   pps->weighted_bi_pred_idc != 0 ||
> >>   pps->transform8x8_flag != 0 ||
> >>   pps->scaling_matrix_present_flag != 0
> > Thanks for double checking this. I can confirm that it's what Hantro
> > SDK does indeed.
> >
> > However, would a stream with sps->profile_idc <= 66 and those other
> > conditions met be still a compliant stream?
>
> You are correct, if a non-compliant video is having decoding problems it should probably be handled
> on userspace side (by not reporting baseline profile) and not in kernel.
> All my video samples that was having the issue fixed in this patch are now decoded correctly.
>
> >
> >> Above check is used when DIR_MV_BASE should be written.
> >> And WRITE_MVS_E is set to nal_ref_idc != 0 when above is true.
> >>
> >> I think it may be safer to always set DIR_MV_BASE and keep the existing nal_ref_idc check for WRITE_MVS_E.
> > That might have a performance penalty or some other side effects,
> > though. Otherwise Hantro SDK wouldn't have enable it conditionally.
> >
> >> (That is what I did in my "media: hantro: H264 fixes and improvements" series, v2 is incoming)
> >> Or have you found any video that is having issues in such case?
> > We've been running the code with sps->profile_idc > 66 in production
> > for 4 years and haven't seen any reports of a stream that wasn't
> > decoded correctly.
> >
> > If we decide to go with a different behavior, I'd suggest thoroughly
> > verifying the behavior on a big number of streams, including some
> > performance measurements.
>
> I agree, I would however suggest to change the if statement to the following (or similar)
> as that should be the optimal for performance reasons and match the hantro sdk.
>
> if (sps->profile_idc > 66 && dec_param->nal_ref_idc)

Sorry for my ignorance, but could you elaborate on this? What's the
meaning of nal_ref_idc? I don't see it being checked in the Hantro SDK
condition you mentioned earlier.

>
> Regards,
> Jonas
>
> >
> > Best regards,
> > Tomasz
> >
> >> Regards,
> >> Jonas
> >>
> >>> Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
> >>> Signed-off-by: Francois Buergisser <fbuergisser@chromium.org>
> >>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> >>> ---
> >>> v2:
> >>> * New patch.
> >>>
> >>>  drivers/staging/media/hantro/hantro_g1_h264_dec.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> >>> index 7ab534936843..c92460407613 100644
> >>> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> >>> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> >>> @@ -35,7 +35,7 @@ static void set_params(struct hantro_ctx *ctx)
> >>>       if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
> >>>               reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E;
> >>>       reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E;
> >>> -     if (dec_param->nal_ref_idc)
> >>> +     if (sps->profile_idc > 66)
> >>>               reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E;
> >>>
> >>>       if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288
  2019-10-08  6:31     ` Jonas Karlman
@ 2019-10-08 10:42       ` Tomasz Figa
  2019-10-08 13:35         ` Tomasz Figa
  0 siblings, 1 reply; 28+ messages in thread
From: Tomasz Figa @ 2019-10-08 10:42 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Ezequiel Garcia, Linux Media Mailing List, kernel,
	Nicolas Dufresne, open list:ARM/Rockchip SoC...,
	Heiko Stuebner, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, Linux Kernel Mailing List,
	Douglas Anderson

On Tue, Oct 8, 2019 at 3:31 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> On 2019-10-08 07:27, Tomasz Figa wrote:
> > Hi Ezequiel, Jonas,
> >
> > On Tue, Oct 8, 2019 at 2:46 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> >> From: Jonas Karlman <jonas@kwiboo.se>
> >>
> >> TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
> >> change frmsize max_width/max_height to match TRM.
> >>
> >> Fixes: 760327930e10 ("media: hantro: Enable H264 decoding on rk3288")
> >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> >> ---
> >> v2:
> >> * No changes.
> >>
> >>  drivers/staging/media/hantro/rk3288_vpu_hw.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> >> index 6bfcc47d1e58..ebb017b8a334 100644
> >> --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
> >> +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> >> @@ -67,10 +67,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
> >>                 .max_depth = 2,
> >>                 .frmsize = {
> >>                         .min_width = 48,
> >> -                       .max_width = 3840,
> >> +                       .max_width = 4096,
> >>                         .step_width = H264_MB_DIM,
> >>                         .min_height = 48,
> >> -                       .max_height = 2160,
> >> +                       .max_height = 2304,
> > This doesn't match the datasheet I have, which is RK3288 Datasheet Rev
> > 1.4 and which has the values as in current code. What's the one you
> > got the values from?
>
> The RK3288 TRM vcodec chapter from [1], unknown revision and date, lists 48x48 to 4096x2304 step size 16 pixels under 25.5.1 H.264 decoder.
>
> I can also confirm that one of my test samples (PUPPIES BATH IN 4K) is 4096x2304 and can be decoded after this patch.
> However the decoding speed is not optimal at 400Mhz, if I recall correctly you need to set the VPU1 clock to 600Mhz for 4K decoding on RK3288.
>
> I am not sure if I should include a v2 of this patch in my v2 series, as-is this patch do not apply on master (H264_MB_DIM has changed to MB_DIM in master).
>
> [1] http://www.t-firefly.com/download/firefly-rk3288/docs/TRM/rk3288-chapter-25-video-encoder-decoder-unit-(vcodec).pdf

I checked the RK3288 TRM V1.1 too and it refers to 3840x2160@24fps as
the maximum.

As for performance, we've actually been getting around 33 fps at 400
MHz with 3840x2160 on our devices (the old RK3288 Asus Chromebook
Flip).

I guess we might want to check that with Hantro.

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288
  2019-10-08 10:42       ` Tomasz Figa
@ 2019-10-08 13:35         ` Tomasz Figa
  2019-10-08 13:53           ` Tomasz Figa
  0 siblings, 1 reply; 28+ messages in thread
From: Tomasz Figa @ 2019-10-08 13:35 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Ezequiel Garcia, Linux Media Mailing List, kernel,
	Nicolas Dufresne, open list:ARM/Rockchip SoC...,
	Heiko Stuebner, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, Linux Kernel Mailing List,
	Douglas Anderson

On Tue, Oct 8, 2019 at 7:42 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> On Tue, Oct 8, 2019 at 3:31 PM Jonas Karlman <jonas@kwiboo.se> wrote:
> >
> > On 2019-10-08 07:27, Tomasz Figa wrote:
> > > Hi Ezequiel, Jonas,
> > >
> > > On Tue, Oct 8, 2019 at 2:46 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > >> From: Jonas Karlman <jonas@kwiboo.se>
> > >>
> > >> TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
> > >> change frmsize max_width/max_height to match TRM.
> > >>
> > >> Fixes: 760327930e10 ("media: hantro: Enable H264 decoding on rk3288")
> > >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > >> ---
> > >> v2:
> > >> * No changes.
> > >>
> > >>  drivers/staging/media/hantro/rk3288_vpu_hw.c | 4 ++--
> > >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> > >> index 6bfcc47d1e58..ebb017b8a334 100644
> > >> --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
> > >> +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> > >> @@ -67,10 +67,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
> > >>                 .max_depth = 2,
> > >>                 .frmsize = {
> > >>                         .min_width = 48,
> > >> -                       .max_width = 3840,
> > >> +                       .max_width = 4096,
> > >>                         .step_width = H264_MB_DIM,
> > >>                         .min_height = 48,
> > >> -                       .max_height = 2160,
> > >> +                       .max_height = 2304,
> > > This doesn't match the datasheet I have, which is RK3288 Datasheet Rev
> > > 1.4 and which has the values as in current code. What's the one you
> > > got the values from?
> >
> > The RK3288 TRM vcodec chapter from [1], unknown revision and date, lists 48x48 to 4096x2304 step size 16 pixels under 25.5.1 H.264 decoder.
> >
> > I can also confirm that one of my test samples (PUPPIES BATH IN 4K) is 4096x2304 and can be decoded after this patch.
> > However the decoding speed is not optimal at 400Mhz, if I recall correctly you need to set the VPU1 clock to 600Mhz for 4K decoding on RK3288.
> >
> > I am not sure if I should include a v2 of this patch in my v2 series, as-is this patch do not apply on master (H264_MB_DIM has changed to MB_DIM in master).
> >
> > [1] http://www.t-firefly.com/download/firefly-rk3288/docs/TRM/rk3288-chapter-25-video-encoder-decoder-unit-(vcodec).pdf
>
> I checked the RK3288 TRM V1.1 too and it refers to 3840x2160@24fps as
> the maximum.
>
> As for performance, we've actually been getting around 33 fps at 400
> MHz with 3840x2160 on our devices (the old RK3288 Asus Chromebook
> Flip).
>
> I guess we might want to check that with Hantro.

Could you check the value of bits 10:0 in register at 0x0c8? That
should be the maximum supported stream width in the units of 16
pixels.

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288
  2019-10-08 13:35         ` Tomasz Figa
@ 2019-10-08 13:53           ` Tomasz Figa
  2019-10-08 14:12             ` Jonas Karlman
  0 siblings, 1 reply; 28+ messages in thread
From: Tomasz Figa @ 2019-10-08 13:53 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Ezequiel Garcia, Linux Media Mailing List, kernel,
	Nicolas Dufresne, open list:ARM/Rockchip SoC...,
	Heiko Stuebner, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, Linux Kernel Mailing List,
	Douglas Anderson

On Tue, Oct 8, 2019 at 10:35 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> On Tue, Oct 8, 2019 at 7:42 PM Tomasz Figa <tfiga@chromium.org> wrote:
> >
> > On Tue, Oct 8, 2019 at 3:31 PM Jonas Karlman <jonas@kwiboo.se> wrote:
> > >
> > > On 2019-10-08 07:27, Tomasz Figa wrote:
> > > > Hi Ezequiel, Jonas,
> > > >
> > > > On Tue, Oct 8, 2019 at 2:46 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > >> From: Jonas Karlman <jonas@kwiboo.se>
> > > >>
> > > >> TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
> > > >> change frmsize max_width/max_height to match TRM.
> > > >>
> > > >> Fixes: 760327930e10 ("media: hantro: Enable H264 decoding on rk3288")
> > > >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > >> ---
> > > >> v2:
> > > >> * No changes.
> > > >>
> > > >>  drivers/staging/media/hantro/rk3288_vpu_hw.c | 4 ++--
> > > >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >>
> > > >> diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> > > >> index 6bfcc47d1e58..ebb017b8a334 100644
> > > >> --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
> > > >> +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> > > >> @@ -67,10 +67,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
> > > >>                 .max_depth = 2,
> > > >>                 .frmsize = {
> > > >>                         .min_width = 48,
> > > >> -                       .max_width = 3840,
> > > >> +                       .max_width = 4096,
> > > >>                         .step_width = H264_MB_DIM,
> > > >>                         .min_height = 48,
> > > >> -                       .max_height = 2160,
> > > >> +                       .max_height = 2304,
> > > > This doesn't match the datasheet I have, which is RK3288 Datasheet Rev
> > > > 1.4 and which has the values as in current code. What's the one you
> > > > got the values from?
> > >
> > > The RK3288 TRM vcodec chapter from [1], unknown revision and date, lists 48x48 to 4096x2304 step size 16 pixels under 25.5.1 H.264 decoder.
> > >
> > > I can also confirm that one of my test samples (PUPPIES BATH IN 4K) is 4096x2304 and can be decoded after this patch.
> > > However the decoding speed is not optimal at 400Mhz, if I recall correctly you need to set the VPU1 clock to 600Mhz for 4K decoding on RK3288.
> > >
> > > I am not sure if I should include a v2 of this patch in my v2 series, as-is this patch do not apply on master (H264_MB_DIM has changed to MB_DIM in master).
> > >
> > > [1] http://www.t-firefly.com/download/firefly-rk3288/docs/TRM/rk3288-chapter-25-video-encoder-decoder-unit-(vcodec).pdf
> >
> > I checked the RK3288 TRM V1.1 too and it refers to 3840x2160@24fps as
> > the maximum.
> >
> > As for performance, we've actually been getting around 33 fps at 400
> > MHz with 3840x2160 on our devices (the old RK3288 Asus Chromebook
> > Flip).
> >
> > I guess we might want to check that with Hantro.
>
> Could you check the value of bits 10:0 in register at 0x0c8? That
> should be the maximum supported stream width in the units of 16
> pixels.

Correction: The unit is 1 pixel and there are additional 2 most
significant bits at 0x0d8, 15:14.

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition
  2019-10-08 10:26         ` Tomasz Figa
@ 2019-10-08 13:57           ` Jonas Karlman
  2019-10-10  7:36             ` Tomasz Figa
  0 siblings, 1 reply; 28+ messages in thread
From: Jonas Karlman @ 2019-10-08 13:57 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Ezequiel Garcia, linux-media, kernel, Nicolas Dufresne,
	linux-rockchip, Heiko Stuebner, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, linux-kernel, Douglas Anderson

On 2019-10-08 12:26, Tomasz Figa wrote:
> On Tue, Oct 8, 2019 at 3:23 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>> On 2019-10-08 05:29, Tomasz Figa wrote:
>>> Hi Jonas,
>>>
>>> On Tue, Oct 8, 2019 at 3:33 AM Jonas Karlman <jonas@kwiboo.se> wrote:
>>>> On 2019-10-07 19:45, Ezequiel Garcia wrote:
>>>>> From: Francois Buergisser <fbuergisser@chromium.org>
>>>>>
>>>>> The setting of the motion vectors usage and the setting of motion
>>>>> vectors address are currently done under different conditions.
>>>>>
>>>>> When decoding pre-recorded videos, this results of leaving the motion
>>>>> vectors address unset, resulting in faulty memory accesses. Fix it
>>>>> by using the same condition everywhere, which matches the profiles
>>>>> that support motion vectors.
>>>> This does not fully match hantro sdk:
>>>>
>>>>   enable direct MV writing and POC tables for high/main streams.
>>>>   enable it also for any "baseline" stream which have main/high tools enabled.
>>>>
>>>>   (sps->profile_idc > 66 && sps->constrained_set0_flag == 0) ||
>>>>   sps->frame_mbs_only_flag != 1 ||
>>>>   sps->chroma_format_idc != 1 ||
>>>>   sps->scaling_matrix_present_flag != 0 ||
>>>>   pps->entropy_coding_mode_flag != 0 ||
>>>>   pps->weighted_pred_flag != 0 ||
>>>>   pps->weighted_bi_pred_idc != 0 ||
>>>>   pps->transform8x8_flag != 0 ||
>>>>   pps->scaling_matrix_present_flag != 0
>>> Thanks for double checking this. I can confirm that it's what Hantro
>>> SDK does indeed.
>>>
>>> However, would a stream with sps->profile_idc <= 66 and those other
>>> conditions met be still a compliant stream?
>> You are correct, if a non-compliant video is having decoding problems it should probably be handled
>> on userspace side (by not reporting baseline profile) and not in kernel.
>> All my video samples that was having the issue fixed in this patch are now decoded correctly.
>>
>>>> Above check is used when DIR_MV_BASE should be written.
>>>> And WRITE_MVS_E is set to nal_ref_idc != 0 when above is true.
>>>>
>>>> I think it may be safer to always set DIR_MV_BASE and keep the existing nal_ref_idc check for WRITE_MVS_E.
>>> That might have a performance penalty or some other side effects,
>>> though. Otherwise Hantro SDK wouldn't have enable it conditionally.
>>>
>>>> (That is what I did in my "media: hantro: H264 fixes and improvements" series, v2 is incoming)
>>>> Or have you found any video that is having issues in such case?
>>> We've been running the code with sps->profile_idc > 66 in production
>>> for 4 years and haven't seen any reports of a stream that wasn't
>>> decoded correctly.
>>>
>>> If we decide to go with a different behavior, I'd suggest thoroughly
>>> verifying the behavior on a big number of streams, including some
>>> performance measurements.
>> I agree, I would however suggest to change the if statement to the following (or similar)
>> as that should be the optimal for performance reasons and match the hantro sdk.
>>
>> if (sps->profile_idc > 66 && dec_param->nal_ref_idc)
> Sorry for my ignorance, but could you elaborate on this? What's the
> meaning of nal_ref_idc? I don't see it being checked in the Hantro SDK
> condition you mentioned earlier.

My somewhat limited understanding of h264 spec is that nal_ref_idc should be 0 for non-reference field/frame/pictures
and because of this there should not be any need to write motion vector data as the field/frame should not be referenced.

My base for the hantro sdk code is the imx8 imx-vpu-hantro package and it uses (simplified):
  SetDecRegister(h264_regs, HWIF_WRITE_MVS_E, nal_ref_idc != 0)
for MVC there is an additional condition.

Regards,
Jonas

>
>> Regards,
>> Jonas
>>
>>> Best regards,
>>> Tomasz
>>>
>>>> Regards,
>>>> Jonas
>>>>
>>>>> Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
>>>>> Signed-off-by: Francois Buergisser <fbuergisser@chromium.org>
>>>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>>>> ---
>>>>> v2:
>>>>> * New patch.
>>>>>
>>>>>  drivers/staging/media/hantro/hantro_g1_h264_dec.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
>>>>> index 7ab534936843..c92460407613 100644
>>>>> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
>>>>> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
>>>>> @@ -35,7 +35,7 @@ static void set_params(struct hantro_ctx *ctx)
>>>>>       if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
>>>>>               reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E;
>>>>>       reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E;
>>>>> -     if (dec_param->nal_ref_idc)
>>>>> +     if (sps->profile_idc > 66)
>>>>>               reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E;
>>>>>
>>>>>       if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288
  2019-10-08 13:53           ` Tomasz Figa
@ 2019-10-08 14:12             ` Jonas Karlman
  2019-10-08 20:39               ` Jonas Karlman
  2019-10-10  7:23               ` Tomasz Figa
  0 siblings, 2 replies; 28+ messages in thread
From: Jonas Karlman @ 2019-10-08 14:12 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Ezequiel Garcia, Linux Media Mailing List, kernel,
	Nicolas Dufresne, open list:ARM/Rockchip SoC...,
	Heiko Stuebner, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, Linux Kernel Mailing List,
	Douglas Anderson

On 2019-10-08 15:53, Tomasz Figa wrote:
> On Tue, Oct 8, 2019 at 10:35 PM Tomasz Figa <tfiga@chromium.org> wrote:
>> On Tue, Oct 8, 2019 at 7:42 PM Tomasz Figa <tfiga@chromium.org> wrote:
>>> On Tue, Oct 8, 2019 at 3:31 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>>>> On 2019-10-08 07:27, Tomasz Figa wrote:
>>>>> Hi Ezequiel, Jonas,
>>>>>
>>>>> On Tue, Oct 8, 2019 at 2:46 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>>>>>> From: Jonas Karlman <jonas@kwiboo.se>
>>>>>>
>>>>>> TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
>>>>>> change frmsize max_width/max_height to match TRM.
>>>>>>
>>>>>> Fixes: 760327930e10 ("media: hantro: Enable H264 decoding on rk3288")
>>>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>>>> ---
>>>>>> v2:
>>>>>> * No changes.
>>>>>>
>>>>>>  drivers/staging/media/hantro/rk3288_vpu_hw.c | 4 ++--
>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
>>>>>> index 6bfcc47d1e58..ebb017b8a334 100644
>>>>>> --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
>>>>>> +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
>>>>>> @@ -67,10 +67,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
>>>>>>                 .max_depth = 2,
>>>>>>                 .frmsize = {
>>>>>>                         .min_width = 48,
>>>>>> -                       .max_width = 3840,
>>>>>> +                       .max_width = 4096,
>>>>>>                         .step_width = H264_MB_DIM,
>>>>>>                         .min_height = 48,
>>>>>> -                       .max_height = 2160,
>>>>>> +                       .max_height = 2304,
>>>>> This doesn't match the datasheet I have, which is RK3288 Datasheet Rev
>>>>> 1.4 and which has the values as in current code. What's the one you
>>>>> got the values from?
>>>> The RK3288 TRM vcodec chapter from [1], unknown revision and date, lists 48x48 to 4096x2304 step size 16 pixels under 25.5.1 H.264 decoder.
>>>>
>>>> I can also confirm that one of my test samples (PUPPIES BATH IN 4K) is 4096x2304 and can be decoded after this patch.
>>>> However the decoding speed is not optimal at 400Mhz, if I recall correctly you need to set the VPU1 clock to 600Mhz for 4K decoding on RK3288.
>>>>
>>>> I am not sure if I should include a v2 of this patch in my v2 series, as-is this patch do not apply on master (H264_MB_DIM has changed to MB_DIM in master).
>>>>
>>>> [1] http://www.t-firefly.com/download/firefly-rk3288/docs/TRM/rk3288-chapter-25-video-encoder-decoder-unit-(vcodec).pdf
>>> I checked the RK3288 TRM V1.1 too and it refers to 3840x2160@24fps as
>>> the maximum.
>>>
>>> As for performance, we've actually been getting around 33 fps at 400
>>> MHz with 3840x2160 on our devices (the old RK3288 Asus Chromebook
>>> Flip).
>>>
>>> I guess we might want to check that with Hantro.
>> Could you check the value of bits 10:0 in register at 0x0c8? That
>> should be the maximum supported stream width in the units of 16
>> pixels.
> Correction: The unit is 1 pixel and there are additional 2 most
> significant bits at 0x0d8, 15:14.

I will check this later tonight when I have access to my devices.
The PUPPIES BATH IN 4K (4096x2304) sample decoded without issue using rockchip 4.4 BSP kernel and mpp last time I tested.

The vcodec driver in 4.4 BSP kernel use 300/400 Mhz as default clock rate and will change to 600 Mhz when width is over 2560, see [1]:
  raise frequency for resolution larger than 1440p avc

[1] https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/video/rockchip/vcodec/vcodec_service.c#L2551-L2570

Regards,
Jonas

>
> Best regards,
> Tomasz


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288
  2019-10-08 14:12             ` Jonas Karlman
@ 2019-10-08 20:39               ` Jonas Karlman
  2019-10-10  7:27                 ` Tomasz Figa
  2019-10-10  7:23               ` Tomasz Figa
  1 sibling, 1 reply; 28+ messages in thread
From: Jonas Karlman @ 2019-10-08 20:39 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: fbuergisser, kernel, Heiko Stuebner, Alexandre Courbot,
	Linux Kernel Mailing List, Douglas Anderson,
	open list:ARM/Rockchip SoC...,
	Boris Brezillon, Philipp Zabel, Nicolas Dufresne,
	Ezequiel Garcia, Linux Media Mailing List

On 2019-10-08 16:12, Jonas Karlman wrote:
> On 2019-10-08 15:53, Tomasz Figa wrote:
>> On Tue, Oct 8, 2019 at 10:35 PM Tomasz Figa <tfiga@chromium.org> wrote:
>>> On Tue, Oct 8, 2019 at 7:42 PM Tomasz Figa <tfiga@chromium.org> wrote:
>>>> On Tue, Oct 8, 2019 at 3:31 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>>>>> On 2019-10-08 07:27, Tomasz Figa wrote:
>>>>>> Hi Ezequiel, Jonas,
>>>>>>
>>>>>> On Tue, Oct 8, 2019 at 2:46 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>>>>>>> From: Jonas Karlman <jonas@kwiboo.se>
>>>>>>>
>>>>>>> TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
>>>>>>> change frmsize max_width/max_height to match TRM.
>>>>>>>
>>>>>>> Fixes: 760327930e10 ("media: hantro: Enable H264 decoding on rk3288")
>>>>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>>>>> ---
>>>>>>> v2:
>>>>>>> * No changes.
>>>>>>>
>>>>>>>  drivers/staging/media/hantro/rk3288_vpu_hw.c | 4 ++--
>>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
>>>>>>> index 6bfcc47d1e58..ebb017b8a334 100644
>>>>>>> --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
>>>>>>> +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
>>>>>>> @@ -67,10 +67,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
>>>>>>>                 .max_depth = 2,
>>>>>>>                 .frmsize = {
>>>>>>>                         .min_width = 48,
>>>>>>> -                       .max_width = 3840,
>>>>>>> +                       .max_width = 4096,
>>>>>>>                         .step_width = H264_MB_DIM,
>>>>>>>                         .min_height = 48,
>>>>>>> -                       .max_height = 2160,
>>>>>>> +                       .max_height = 2304,
>>>>>> This doesn't match the datasheet I have, which is RK3288 Datasheet Rev
>>>>>> 1.4 and which has the values as in current code. What's the one you
>>>>>> got the values from?
>>>>> The RK3288 TRM vcodec chapter from [1], unknown revision and date, lists 48x48 to 4096x2304 step size 16 pixels under 25.5.1 H.264 decoder.
>>>>>
>>>>> I can also confirm that one of my test samples (PUPPIES BATH IN 4K) is 4096x2304 and can be decoded after this patch.
>>>>> However the decoding speed is not optimal at 400Mhz, if I recall correctly you need to set the VPU1 clock to 600Mhz for 4K decoding on RK3288.
>>>>>
>>>>> I am not sure if I should include a v2 of this patch in my v2 series, as-is this patch do not apply on master (H264_MB_DIM has changed to MB_DIM in master).
>>>>>
>>>>> [1] http://www.t-firefly.com/download/firefly-rk3288/docs/TRM/rk3288-chapter-25-video-encoder-decoder-unit-(vcodec).pdf
>>>> I checked the RK3288 TRM V1.1 too and it refers to 3840x2160@24fps as
>>>> the maximum.
>>>>
>>>> As for performance, we've actually been getting around 33 fps at 400
>>>> MHz with 3840x2160 on our devices (the old RK3288 Asus Chromebook
>>>> Flip).
>>>>
>>>> I guess we might want to check that with Hantro.
>>> Could you check the value of bits 10:0 in register at 0x0c8? That
>>> should be the maximum supported stream width in the units of 16
>>> pixels.
>> Correction: The unit is 1 pixel and there are additional 2 most
>> significant bits at 0x0d8, 15:14.
> I will check this later tonight when I have access to my devices.

My Asus Tinker Board S (RK3288-C) is reporting support for 0x780 / 1920 pixels:

0x000  (0) = 0x67313688
0x0c8 (50) = 0xfbb56f80
0x0d8 (54) = 0xe5da0000

> The PUPPIES BATH IN 4K (4096x2304) sample decoded without issue using rockchip 4.4 BSP kernel and mpp last time I tested.
>
> The vcodec driver in 4.4 BSP kernel use 300/400 Mhz as default clock rate and will change to 600 Mhz when width is over 2560, see [1]:
>   raise frequency for resolution larger than 1440p avc
>
> [1] https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/video/rockchip/vcodec/vcodec_service.c#L2551-L2570
>
> Regards,
> Jonas
>
>> Best regards,
>> Tomasz
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288
  2019-10-08 14:12             ` Jonas Karlman
  2019-10-08 20:39               ` Jonas Karlman
@ 2019-10-10  7:23               ` Tomasz Figa
  2019-10-13 22:10                 ` Nicolas Dufresne
  1 sibling, 1 reply; 28+ messages in thread
From: Tomasz Figa @ 2019-10-10  7:23 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Ezequiel Garcia, Linux Media Mailing List, kernel,
	Nicolas Dufresne, open list:ARM/Rockchip SoC...,
	Heiko Stuebner, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, Linux Kernel Mailing List,
	Douglas Anderson

On Tue, Oct 8, 2019 at 11:12 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> On 2019-10-08 15:53, Tomasz Figa wrote:
> > On Tue, Oct 8, 2019 at 10:35 PM Tomasz Figa <tfiga@chromium.org> wrote:
> >> On Tue, Oct 8, 2019 at 7:42 PM Tomasz Figa <tfiga@chromium.org> wrote:
> >>> On Tue, Oct 8, 2019 at 3:31 PM Jonas Karlman <jonas@kwiboo.se> wrote:
> >>>> On 2019-10-08 07:27, Tomasz Figa wrote:
> >>>>> Hi Ezequiel, Jonas,
> >>>>>
> >>>>> On Tue, Oct 8, 2019 at 2:46 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> >>>>>> From: Jonas Karlman <jonas@kwiboo.se>
> >>>>>>
> >>>>>> TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
> >>>>>> change frmsize max_width/max_height to match TRM.
> >>>>>>
> >>>>>> Fixes: 760327930e10 ("media: hantro: Enable H264 decoding on rk3288")
> >>>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> >>>>>> ---
> >>>>>> v2:
> >>>>>> * No changes.
> >>>>>>
> >>>>>>  drivers/staging/media/hantro/rk3288_vpu_hw.c | 4 ++--
> >>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> >>>>>> index 6bfcc47d1e58..ebb017b8a334 100644
> >>>>>> --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
> >>>>>> +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> >>>>>> @@ -67,10 +67,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
> >>>>>>                 .max_depth = 2,
> >>>>>>                 .frmsize = {
> >>>>>>                         .min_width = 48,
> >>>>>> -                       .max_width = 3840,
> >>>>>> +                       .max_width = 4096,
> >>>>>>                         .step_width = H264_MB_DIM,
> >>>>>>                         .min_height = 48,
> >>>>>> -                       .max_height = 2160,
> >>>>>> +                       .max_height = 2304,
> >>>>> This doesn't match the datasheet I have, which is RK3288 Datasheet Rev
> >>>>> 1.4 and which has the values as in current code. What's the one you
> >>>>> got the values from?
> >>>> The RK3288 TRM vcodec chapter from [1], unknown revision and date, lists 48x48 to 4096x2304 step size 16 pixels under 25.5.1 H.264 decoder.
> >>>>
> >>>> I can also confirm that one of my test samples (PUPPIES BATH IN 4K) is 4096x2304 and can be decoded after this patch.
> >>>> However the decoding speed is not optimal at 400Mhz, if I recall correctly you need to set the VPU1 clock to 600Mhz for 4K decoding on RK3288.
> >>>>
> >>>> I am not sure if I should include a v2 of this patch in my v2 series, as-is this patch do not apply on master (H264_MB_DIM has changed to MB_DIM in master).
> >>>>
> >>>> [1] http://www.t-firefly.com/download/firefly-rk3288/docs/TRM/rk3288-chapter-25-video-encoder-decoder-unit-(vcodec).pdf
> >>> I checked the RK3288 TRM V1.1 too and it refers to 3840x2160@24fps as
> >>> the maximum.
> >>>
> >>> As for performance, we've actually been getting around 33 fps at 400
> >>> MHz with 3840x2160 on our devices (the old RK3288 Asus Chromebook
> >>> Flip).
> >>>
> >>> I guess we might want to check that with Hantro.
> >> Could you check the value of bits 10:0 in register at 0x0c8? That
> >> should be the maximum supported stream width in the units of 16
> >> pixels.
> > Correction: The unit is 1 pixel and there are additional 2 most
> > significant bits at 0x0d8, 15:14.
>
> I will check this later tonight when I have access to my devices.
> The PUPPIES BATH IN 4K (4096x2304) sample decoded without issue using rockchip 4.4 BSP kernel and mpp last time I tested.
>
> The vcodec driver in 4.4 BSP kernel use 300/400 Mhz as default clock rate and will change to 600 Mhz when width is over 2560, see [1]:
>   raise frequency for resolution larger than 1440p avc
>
> [1] https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/video/rockchip/vcodec/vcodec_service.c#L2551-L2570

How comes it works for us well at 400 MHz? Better DRAM? Differences in
how Vcodec BSP handles the hardware that somehow make the decoding
slower?

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288
  2019-10-08 20:39               ` Jonas Karlman
@ 2019-10-10  7:27                 ` Tomasz Figa
  0 siblings, 0 replies; 28+ messages in thread
From: Tomasz Figa @ 2019-10-10  7:27 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: fbuergisser, kernel, Heiko Stuebner, Alexandre Courbot,
	Linux Kernel Mailing List, Douglas Anderson,
	open list:ARM/Rockchip SoC...,
	Boris Brezillon, Philipp Zabel, Nicolas Dufresne,
	Ezequiel Garcia, Linux Media Mailing List

On Wed, Oct 9, 2019 at 5:39 AM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> On 2019-10-08 16:12, Jonas Karlman wrote:
> > On 2019-10-08 15:53, Tomasz Figa wrote:
> >> On Tue, Oct 8, 2019 at 10:35 PM Tomasz Figa <tfiga@chromium.org> wrote:
> >>> On Tue, Oct 8, 2019 at 7:42 PM Tomasz Figa <tfiga@chromium.org> wrote:
> >>>> On Tue, Oct 8, 2019 at 3:31 PM Jonas Karlman <jonas@kwiboo.se> wrote:
> >>>>> On 2019-10-08 07:27, Tomasz Figa wrote:
> >>>>>> Hi Ezequiel, Jonas,
> >>>>>>
> >>>>>> On Tue, Oct 8, 2019 at 2:46 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> >>>>>>> From: Jonas Karlman <jonas@kwiboo.se>
> >>>>>>>
> >>>>>>> TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
> >>>>>>> change frmsize max_width/max_height to match TRM.
> >>>>>>>
> >>>>>>> Fixes: 760327930e10 ("media: hantro: Enable H264 decoding on rk3288")
> >>>>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> >>>>>>> ---
> >>>>>>> v2:
> >>>>>>> * No changes.
> >>>>>>>
> >>>>>>>  drivers/staging/media/hantro/rk3288_vpu_hw.c | 4 ++--
> >>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> >>>>>>> index 6bfcc47d1e58..ebb017b8a334 100644
> >>>>>>> --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
> >>>>>>> +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> >>>>>>> @@ -67,10 +67,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
> >>>>>>>                 .max_depth = 2,
> >>>>>>>                 .frmsize = {
> >>>>>>>                         .min_width = 48,
> >>>>>>> -                       .max_width = 3840,
> >>>>>>> +                       .max_width = 4096,
> >>>>>>>                         .step_width = H264_MB_DIM,
> >>>>>>>                         .min_height = 48,
> >>>>>>> -                       .max_height = 2160,
> >>>>>>> +                       .max_height = 2304,
> >>>>>> This doesn't match the datasheet I have, which is RK3288 Datasheet Rev
> >>>>>> 1.4 and which has the values as in current code. What's the one you
> >>>>>> got the values from?
> >>>>> The RK3288 TRM vcodec chapter from [1], unknown revision and date, lists 48x48 to 4096x2304 step size 16 pixels under 25.5.1 H.264 decoder.
> >>>>>
> >>>>> I can also confirm that one of my test samples (PUPPIES BATH IN 4K) is 4096x2304 and can be decoded after this patch.
> >>>>> However the decoding speed is not optimal at 400Mhz, if I recall correctly you need to set the VPU1 clock to 600Mhz for 4K decoding on RK3288.
> >>>>>
> >>>>> I am not sure if I should include a v2 of this patch in my v2 series, as-is this patch do not apply on master (H264_MB_DIM has changed to MB_DIM in master).
> >>>>>
> >>>>> [1] http://www.t-firefly.com/download/firefly-rk3288/docs/TRM/rk3288-chapter-25-video-encoder-decoder-unit-(vcodec).pdf
> >>>> I checked the RK3288 TRM V1.1 too and it refers to 3840x2160@24fps as
> >>>> the maximum.
> >>>>
> >>>> As for performance, we've actually been getting around 33 fps at 400
> >>>> MHz with 3840x2160 on our devices (the old RK3288 Asus Chromebook
> >>>> Flip).
> >>>>
> >>>> I guess we might want to check that with Hantro.
> >>> Could you check the value of bits 10:0 in register at 0x0c8? That
> >>> should be the maximum supported stream width in the units of 16
> >>> pixels.
> >> Correction: The unit is 1 pixel and there are additional 2 most
> >> significant bits at 0x0d8, 15:14.
> > I will check this later tonight when I have access to my devices.
>
> My Asus Tinker Board S (RK3288-C) is reporting support for 0x780 / 1920 pixels:
>
> 0x000  (0) = 0x67313688
> 0x0c8 (50) = 0xfbb56f80
> 0x0d8 (54) = 0xe5da0000
>

Looks like that register doesn't work very well in Rockchip's
implementation... Thanks for checking anyway.

I guess we can allow 4096x2304 for the time being and see what happens.

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition
  2019-10-08 13:57           ` Jonas Karlman
@ 2019-10-10  7:36             ` Tomasz Figa
  0 siblings, 0 replies; 28+ messages in thread
From: Tomasz Figa @ 2019-10-10  7:36 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Ezequiel Garcia, linux-media, kernel, Nicolas Dufresne,
	linux-rockchip, Heiko Stuebner, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, linux-kernel, Douglas Anderson

On Tue, Oct 8, 2019 at 10:57 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> On 2019-10-08 12:26, Tomasz Figa wrote:
> > On Tue, Oct 8, 2019 at 3:23 PM Jonas Karlman <jonas@kwiboo.se> wrote:
> >> On 2019-10-08 05:29, Tomasz Figa wrote:
> >>> Hi Jonas,
> >>>
> >>> On Tue, Oct 8, 2019 at 3:33 AM Jonas Karlman <jonas@kwiboo.se> wrote:
> >>>> On 2019-10-07 19:45, Ezequiel Garcia wrote:
> >>>>> From: Francois Buergisser <fbuergisser@chromium.org>
> >>>>>
> >>>>> The setting of the motion vectors usage and the setting of motion
> >>>>> vectors address are currently done under different conditions.
> >>>>>
> >>>>> When decoding pre-recorded videos, this results of leaving the motion
> >>>>> vectors address unset, resulting in faulty memory accesses. Fix it
> >>>>> by using the same condition everywhere, which matches the profiles
> >>>>> that support motion vectors.
> >>>> This does not fully match hantro sdk:
> >>>>
> >>>>   enable direct MV writing and POC tables for high/main streams.
> >>>>   enable it also for any "baseline" stream which have main/high tools enabled.
> >>>>
> >>>>   (sps->profile_idc > 66 && sps->constrained_set0_flag == 0) ||
> >>>>   sps->frame_mbs_only_flag != 1 ||
> >>>>   sps->chroma_format_idc != 1 ||
> >>>>   sps->scaling_matrix_present_flag != 0 ||
> >>>>   pps->entropy_coding_mode_flag != 0 ||
> >>>>   pps->weighted_pred_flag != 0 ||
> >>>>   pps->weighted_bi_pred_idc != 0 ||
> >>>>   pps->transform8x8_flag != 0 ||
> >>>>   pps->scaling_matrix_present_flag != 0
> >>> Thanks for double checking this. I can confirm that it's what Hantro
> >>> SDK does indeed.
> >>>
> >>> However, would a stream with sps->profile_idc <= 66 and those other
> >>> conditions met be still a compliant stream?
> >> You are correct, if a non-compliant video is having decoding problems it should probably be handled
> >> on userspace side (by not reporting baseline profile) and not in kernel.
> >> All my video samples that was having the issue fixed in this patch are now decoded correctly.
> >>
> >>>> Above check is used when DIR_MV_BASE should be written.
> >>>> And WRITE_MVS_E is set to nal_ref_idc != 0 when above is true.
> >>>>
> >>>> I think it may be safer to always set DIR_MV_BASE and keep the existing nal_ref_idc check for WRITE_MVS_E.
> >>> That might have a performance penalty or some other side effects,
> >>> though. Otherwise Hantro SDK wouldn't have enable it conditionally.
> >>>
> >>>> (That is what I did in my "media: hantro: H264 fixes and improvements" series, v2 is incoming)
> >>>> Or have you found any video that is having issues in such case?
> >>> We've been running the code with sps->profile_idc > 66 in production
> >>> for 4 years and haven't seen any reports of a stream that wasn't
> >>> decoded correctly.
> >>>
> >>> If we decide to go with a different behavior, I'd suggest thoroughly
> >>> verifying the behavior on a big number of streams, including some
> >>> performance measurements.
> >> I agree, I would however suggest to change the if statement to the following (or similar)
> >> as that should be the optimal for performance reasons and match the hantro sdk.
> >>
> >> if (sps->profile_idc > 66 && dec_param->nal_ref_idc)
> > Sorry for my ignorance, but could you elaborate on this? What's the
> > meaning of nal_ref_idc? I don't see it being checked in the Hantro SDK
> > condition you mentioned earlier.
>
> My somewhat limited understanding of h264 spec is that nal_ref_idc should be 0 for non-reference field/frame/pictures
> and because of this there should not be any need to write motion vector data as the field/frame should not be referenced.
>
> My base for the hantro sdk code is the imx8 imx-vpu-hantro package and it uses (simplified):
>   SetDecRegister(h264_regs, HWIF_WRITE_MVS_E, nal_ref_idc != 0)
> for MVC there is an additional condition.

Aha, completely makes sense. Thanks for clarifying.

Best regards,
Tomasz

>
> Regards,
> Jonas
>
> >
> >> Regards,
> >> Jonas
> >>
> >>> Best regards,
> >>> Tomasz
> >>>
> >>>> Regards,
> >>>> Jonas
> >>>>
> >>>>> Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
> >>>>> Signed-off-by: Francois Buergisser <fbuergisser@chromium.org>
> >>>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> >>>>> ---
> >>>>> v2:
> >>>>> * New patch.
> >>>>>
> >>>>>  drivers/staging/media/hantro/hantro_g1_h264_dec.c | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> >>>>> index 7ab534936843..c92460407613 100644
> >>>>> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> >>>>> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> >>>>> @@ -35,7 +35,7 @@ static void set_params(struct hantro_ctx *ctx)
> >>>>>       if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD)
> >>>>>               reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E;
> >>>>>       reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E;
> >>>>> -     if (dec_param->nal_ref_idc)
> >>>>> +     if (sps->profile_idc > 66)
> >>>>>               reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E;
> >>>>>
> >>>>>       if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) &&
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288
  2019-10-10  7:23               ` Tomasz Figa
@ 2019-10-13 22:10                 ` Nicolas Dufresne
  2019-10-15  3:27                   ` Tomasz Figa
  0 siblings, 1 reply; 28+ messages in thread
From: Nicolas Dufresne @ 2019-10-13 22:10 UTC (permalink / raw)
  To: Tomasz Figa, Jonas Karlman
  Cc: Ezequiel Garcia, Linux Media Mailing List, kernel,
	open list:ARM/Rockchip SoC...,
	Heiko Stuebner, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, Linux Kernel Mailing List,
	Douglas Anderson

[-- Attachment #1: Type: text/plain, Size: 4901 bytes --]

Le jeudi 10 octobre 2019 à 16:23 +0900, Tomasz Figa a écrit :
> On Tue, Oct 8, 2019 at 11:12 PM Jonas Karlman <jonas@kwiboo.se> wrote:
> > On 2019-10-08 15:53, Tomasz Figa wrote:
> > > On Tue, Oct 8, 2019 at 10:35 PM Tomasz Figa <tfiga@chromium.org> wrote:
> > > > On Tue, Oct 8, 2019 at 7:42 PM Tomasz Figa <tfiga@chromium.org> wrote:
> > > > > On Tue, Oct 8, 2019 at 3:31 PM Jonas Karlman <jonas@kwiboo.se> wrote:
> > > > > > On 2019-10-08 07:27, Tomasz Figa wrote:
> > > > > > > Hi Ezequiel, Jonas,
> > > > > > > 
> > > > > > > On Tue, Oct 8, 2019 at 2:46 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > > > > > > From: Jonas Karlman <jonas@kwiboo.se>
> > > > > > > > 
> > > > > > > > TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
> > > > > > > > change frmsize max_width/max_height to match TRM.
> > > > > > > > 
> > > > > > > > Fixes: 760327930e10 ("media: hantro: Enable H264 decoding on rk3288")
> > > > > > > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > > > > > > ---
> > > > > > > > v2:
> > > > > > > > * No changes.
> > > > > > > > 
> > > > > > > >  drivers/staging/media/hantro/rk3288_vpu_hw.c | 4 ++--
> > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> > > > > > > > index 6bfcc47d1e58..ebb017b8a334 100644
> > > > > > > > --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
> > > > > > > > +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> > > > > > > > @@ -67,10 +67,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
> > > > > > > >                 .max_depth = 2,
> > > > > > > >                 .frmsize = {
> > > > > > > >                         .min_width = 48,
> > > > > > > > -                       .max_width = 3840,
> > > > > > > > +                       .max_width = 4096,
> > > > > > > >                         .step_width = H264_MB_DIM,
> > > > > > > >                         .min_height = 48,
> > > > > > > > -                       .max_height = 2160,
> > > > > > > > +                       .max_height = 2304,
> > > > > > > This doesn't match the datasheet I have, which is RK3288 Datasheet Rev
> > > > > > > 1.4 and which has the values as in current code. What's the one you
> > > > > > > got the values from?
> > > > > > The RK3288 TRM vcodec chapter from [1], unknown revision and date, lists 48x48 to 4096x2304 step size 16 pixels under 25.5.1 H.264 decoder.
> > > > > > 
> > > > > > I can also confirm that one of my test samples (PUPPIES BATH IN 4K) is 4096x2304 and can be decoded after this patch.
> > > > > > However the decoding speed is not optimal at 400Mhz, if I recall correctly you need to set the VPU1 clock to 600Mhz for 4K decoding on RK3288.
> > > > > > 
> > > > > > I am not sure if I should include a v2 of this patch in my v2 series, as-is this patch do not apply on master (H264_MB_DIM has changed to MB_DIM in master).
> > > > > > 
> > > > > > [1] http://www.t-firefly.com/download/firefly-rk3288/docs/TRM/rk3288-chapter-25-video-encoder-decoder-unit-(vcodec).pdf
> > > > > I checked the RK3288 TRM V1.1 too and it refers to 3840x2160@24fps as
> > > > > the maximum.
> > > > > 
> > > > > As for performance, we've actually been getting around 33 fps at 400
> > > > > MHz with 3840x2160 on our devices (the old RK3288 Asus Chromebook
> > > > > Flip).
> > > > > 
> > > > > I guess we might want to check that with Hantro.
> > > > Could you check the value of bits 10:0 in register at 0x0c8? That
> > > > should be the maximum supported stream width in the units of 16
> > > > pixels.
> > > Correction: The unit is 1 pixel and there are additional 2 most
> > > significant bits at 0x0d8, 15:14.
> > 
> > I will check this later tonight when I have access to my devices.
> > The PUPPIES BATH IN 4K (4096x2304) sample decoded without issue using rockchip 4.4 BSP kernel and mpp last time I tested.
> > 
> > The vcodec driver in 4.4 BSP kernel use 300/400 Mhz as default clock rate and will change to 600 Mhz when width is over 2560, see [1]:
> >   raise frequency for resolution larger than 1440p avc
> > 
> > [1] https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/video/rockchip/vcodec/vcodec_service.c#L2551-L2570
> 
> How comes it works for us well at 400 MHz? Better DRAM? Differences in
> how Vcodec BSP handles the hardware that somehow make the decoding
> slower?

FWIW, here on the mainline driver, on RK3288, playing a 4K30 sample
(probably the max for this one) get stuck at 20fps with 400MHz. So
600MHz would in theory be perfect to reach 30fps. That being said,
different stream yield different performance with H264 and other
CODECs, so doing a completely objective evaluation is hard.

> 
> Best regards,
> Tomasz

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288
  2019-10-13 22:10                 ` Nicolas Dufresne
@ 2019-10-15  3:27                   ` Tomasz Figa
  0 siblings, 0 replies; 28+ messages in thread
From: Tomasz Figa @ 2019-10-15  3:27 UTC (permalink / raw)
  To: Nicolas Dufresne
  Cc: Jonas Karlman, Ezequiel Garcia, Linux Media Mailing List, kernel,
	open list:ARM/Rockchip SoC...,
	Heiko Stuebner, Philipp Zabel, Boris Brezillon,
	Alexandre Courbot, fbuergisser, Linux Kernel Mailing List,
	Douglas Anderson

On Mon, Oct 14, 2019 at 7:10 AM Nicolas Dufresne
<nicolas.dufresne@collabora.com> wrote:
>
> Le jeudi 10 octobre 2019 à 16:23 +0900, Tomasz Figa a écrit :
> > On Tue, Oct 8, 2019 at 11:12 PM Jonas Karlman <jonas@kwiboo.se> wrote:
> > > On 2019-10-08 15:53, Tomasz Figa wrote:
> > > > On Tue, Oct 8, 2019 at 10:35 PM Tomasz Figa <tfiga@chromium.org> wrote:
> > > > > On Tue, Oct 8, 2019 at 7:42 PM Tomasz Figa <tfiga@chromium.org> wrote:
> > > > > > On Tue, Oct 8, 2019 at 3:31 PM Jonas Karlman <jonas@kwiboo.se> wrote:
> > > > > > > On 2019-10-08 07:27, Tomasz Figa wrote:
> > > > > > > > Hi Ezequiel, Jonas,
> > > > > > > >
> > > > > > > > On Tue, Oct 8, 2019 at 2:46 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > > > > > > > > From: Jonas Karlman <jonas@kwiboo.se>
> > > > > > > > >
> > > > > > > > > TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
> > > > > > > > > change frmsize max_width/max_height to match TRM.
> > > > > > > > >
> > > > > > > > > Fixes: 760327930e10 ("media: hantro: Enable H264 decoding on rk3288")
> > > > > > > > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > > > > > > > ---
> > > > > > > > > v2:
> > > > > > > > > * No changes.
> > > > > > > > >
> > > > > > > > >  drivers/staging/media/hantro/rk3288_vpu_hw.c | 4 ++--
> > > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/staging/media/hantro/rk3288_vpu_hw.c b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> > > > > > > > > index 6bfcc47d1e58..ebb017b8a334 100644
> > > > > > > > > --- a/drivers/staging/media/hantro/rk3288_vpu_hw.c
> > > > > > > > > +++ b/drivers/staging/media/hantro/rk3288_vpu_hw.c
> > > > > > > > > @@ -67,10 +67,10 @@ static const struct hantro_fmt rk3288_vpu_dec_fmts[] = {
> > > > > > > > >                 .max_depth = 2,
> > > > > > > > >                 .frmsize = {
> > > > > > > > >                         .min_width = 48,
> > > > > > > > > -                       .max_width = 3840,
> > > > > > > > > +                       .max_width = 4096,
> > > > > > > > >                         .step_width = H264_MB_DIM,
> > > > > > > > >                         .min_height = 48,
> > > > > > > > > -                       .max_height = 2160,
> > > > > > > > > +                       .max_height = 2304,
> > > > > > > > This doesn't match the datasheet I have, which is RK3288 Datasheet Rev
> > > > > > > > 1.4 and which has the values as in current code. What's the one you
> > > > > > > > got the values from?
> > > > > > > The RK3288 TRM vcodec chapter from [1], unknown revision and date, lists 48x48 to 4096x2304 step size 16 pixels under 25.5.1 H.264 decoder.
> > > > > > >
> > > > > > > I can also confirm that one of my test samples (PUPPIES BATH IN 4K) is 4096x2304 and can be decoded after this patch.
> > > > > > > However the decoding speed is not optimal at 400Mhz, if I recall correctly you need to set the VPU1 clock to 600Mhz for 4K decoding on RK3288.
> > > > > > >
> > > > > > > I am not sure if I should include a v2 of this patch in my v2 series, as-is this patch do not apply on master (H264_MB_DIM has changed to MB_DIM in master).
> > > > > > >
> > > > > > > [1] http://www.t-firefly.com/download/firefly-rk3288/docs/TRM/rk3288-chapter-25-video-encoder-decoder-unit-(vcodec).pdf
> > > > > > I checked the RK3288 TRM V1.1 too and it refers to 3840x2160@24fps as
> > > > > > the maximum.
> > > > > >
> > > > > > As for performance, we've actually been getting around 33 fps at 400
> > > > > > MHz with 3840x2160 on our devices (the old RK3288 Asus Chromebook
> > > > > > Flip).
> > > > > >
> > > > > > I guess we might want to check that with Hantro.
> > > > > Could you check the value of bits 10:0 in register at 0x0c8? That
> > > > > should be the maximum supported stream width in the units of 16
> > > > > pixels.
> > > > Correction: The unit is 1 pixel and there are additional 2 most
> > > > significant bits at 0x0d8, 15:14.
> > >
> > > I will check this later tonight when I have access to my devices.
> > > The PUPPIES BATH IN 4K (4096x2304) sample decoded without issue using rockchip 4.4 BSP kernel and mpp last time I tested.
> > >
> > > The vcodec driver in 4.4 BSP kernel use 300/400 Mhz as default clock rate and will change to 600 Mhz when width is over 2560, see [1]:
> > >   raise frequency for resolution larger than 1440p avc
> > >
> > > [1] https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/video/rockchip/vcodec/vcodec_service.c#L2551-L2570
> >
> > How comes it works for us well at 400 MHz? Better DRAM? Differences in
> > how Vcodec BSP handles the hardware that somehow make the decoding
> > slower?
>
> FWIW, here on the mainline driver, on RK3288, playing a 4K30 sample
> (probably the max for this one) get stuck at 20fps with 400MHz. So
> 600MHz would in theory be perfect to reach 30fps. That being said,
> different stream yield different performance with H264 and other
> CODECs, so doing a completely objective evaluation is hard.

For a fair comparison, we're using the following stream in our 4K
performance test:
http://storage.googleapis.com/chromiumos-test-assets-public/tast/cros/video/perf/h264/2160p_30fps_300frames_20190801.h264

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes
  2019-10-07 17:45 ` [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes Ezequiel Garcia
@ 2019-11-08 10:19   ` Boris Brezillon
  2019-11-08 11:52     ` Tomasz Figa
  2019-11-09 12:25     ` Hans Verkuil
  0 siblings, 2 replies; 28+ messages in thread
From: Boris Brezillon @ 2019-11-08 10:19 UTC (permalink / raw)
  To: Ezequiel Garcia, Tomasz Figa, Hans Verkuil
  Cc: linux-media, kernel, Nicolas Dufresne, linux-rockchip,
	Heiko Stuebner, Jonas Karlman, Philipp Zabel, Alexandre Courbot,
	fbuergisser, linux-kernel, Douglas Anderson

On Mon,  7 Oct 2019 14:45:02 -0300
Ezequiel Garcia <ezequiel@collabora.com> wrote:

> Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders")
> changed the conditions under S_FMT was allowed for OUTPUT
> CAPTURE buffers.
> 
> However, and according to the mem-to-mem stateless decoder specification,
> in order to support dynamic resolution changes, S_FMT should be allowed
> even if OUTPUT buffers have been allocated.
> 
> Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a resolution
> modification, provided the pixel format stays the same.
> 
> Tested on RK3288 platforms using ChromiumOS Video Decode/Encode Accelerator Unittests.
> 
> Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders")
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> --
> v2:
> * Call try_fmt_out before using the format,
>   pointed out by Philipp.
> 
>  drivers/staging/media/hantro/hantro_v4l2.c | 28 +++++++++++++++-------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
> index 3dae52abb96c..586d243cc3cc 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -367,19 +367,26 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
>  {
>  	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
>  	struct hantro_ctx *ctx = fh_to_ctx(priv);
> +	struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
>  	const struct hantro_fmt *formats;
>  	unsigned int num_fmts;
> -	struct vb2_queue *vq;
>  	int ret;
>  
> -	/* Change not allowed if queue is busy. */
> -	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> -	if (vb2_is_busy(vq))
> -		return -EBUSY;
> +	ret = vidioc_try_fmt_out_mplane(file, priv, f);
> +	if (ret)
> +		return ret;
>  
>  	if (!hantro_is_encoder_ctx(ctx)) {
>  		struct vb2_queue *peer_vq;
>  
> +		/*
> +		 * In other to support dynamic resolution change,

		      ^ order

> +		 * the decoder admits a resolution change, as long
> +		 * as the pixelformat remains. Can't be done if streaming.
> +		 */
> +		if (vb2_is_streaming(vq) || (vb2_is_busy(vq) &&
> +		    pix_mp->pixelformat != ctx->src_fmt.pixelformat))
> +			return -EBUSY;

Sorry to chime in only now, but I'm currently looking at the VP9 spec
and it seems the resolution is allowed to change dynamically [1] (I
guess the same applies to VP8). IIU the spec correctly, coded frames
using the new resolution can reference decoded frames using the old
one (can be higher or lower res BTW). If we force a streamoff to change
the resolution (as seems to be the case here), we'll lose those ref
frames (see the hantro_return_bufs() in stop streaming), right?
Hans, Tomasz, any idea how this dynamic resolution change could/should
be supported?

>  		/*
>  		 * Since format change on the OUTPUT queue will reset
>  		 * the CAPTURE queue, we can't allow doing so
> @@ -389,12 +396,15 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
>  					  V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
>  		if (vb2_is_busy(peer_vq))
>  			return -EBUSY;
> +	} else {
> +		/*
> +		 * The encoder doesn't admit a format change if
> +		 * there are OUTPUT buffers allocated.
> +		 */
> +		if (vb2_is_busy(vq))
> +			return -EBUSY;
>  	}
>  
> -	ret = vidioc_try_fmt_out_mplane(file, priv, f);
> -	if (ret)
> -		return ret;
> -
>  	formats = hantro_get_formats(ctx, &num_fmts);
>  	ctx->vpu_src_fmt = hantro_find_format(formats, num_fmts,
>  					      pix_mp->pixelformat);

[1] Section "5.16 Reference frame scaling" of
    https://storage.googleapis.com/downloads.webmproject.org/docs/vp9/vp9-bitstream-specification-v0.6-20160331-draft.pdf

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes
  2019-11-08 10:19   ` Boris Brezillon
@ 2019-11-08 11:52     ` Tomasz Figa
  2019-11-09 12:17       ` Hans Verkuil
  2019-11-09 12:25     ` Hans Verkuil
  1 sibling, 1 reply; 28+ messages in thread
From: Tomasz Figa @ 2019-11-08 11:52 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Ezequiel Garcia, Hans Verkuil, Linux Media Mailing List, kernel,
	Nicolas Dufresne, open list:ARM/Rockchip SoC...,
	Heiko Stuebner, Jonas Karlman, Philipp Zabel, Alexandre Courbot,
	fbuergisser, Linux Kernel Mailing List, Douglas Anderson

On Fri, Nov 8, 2019 at 7:20 PM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Mon,  7 Oct 2019 14:45:02 -0300
> Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> > Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders")
> > changed the conditions under S_FMT was allowed for OUTPUT
> > CAPTURE buffers.
> >
> > However, and according to the mem-to-mem stateless decoder specification,
> > in order to support dynamic resolution changes, S_FMT should be allowed
> > even if OUTPUT buffers have been allocated.
> >
> > Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a resolution
> > modification, provided the pixel format stays the same.
> >
> > Tested on RK3288 platforms using ChromiumOS Video Decode/Encode Accelerator Unittests.
> >
> > Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders")
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > --
> > v2:
> > * Call try_fmt_out before using the format,
> >   pointed out by Philipp.
> >
> >  drivers/staging/media/hantro/hantro_v4l2.c | 28 +++++++++++++++-------
> >  1 file changed, 19 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
> > index 3dae52abb96c..586d243cc3cc 100644
> > --- a/drivers/staging/media/hantro/hantro_v4l2.c
> > +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> > @@ -367,19 +367,26 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
> >  {
> >       struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> >       struct hantro_ctx *ctx = fh_to_ctx(priv);
> > +     struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> >       const struct hantro_fmt *formats;
> >       unsigned int num_fmts;
> > -     struct vb2_queue *vq;
> >       int ret;
> >
> > -     /* Change not allowed if queue is busy. */
> > -     vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> > -     if (vb2_is_busy(vq))
> > -             return -EBUSY;
> > +     ret = vidioc_try_fmt_out_mplane(file, priv, f);
> > +     if (ret)
> > +             return ret;
> >
> >       if (!hantro_is_encoder_ctx(ctx)) {
> >               struct vb2_queue *peer_vq;
> >
> > +             /*
> > +              * In other to support dynamic resolution change,
>
>                       ^ order
>
> > +              * the decoder admits a resolution change, as long
> > +              * as the pixelformat remains. Can't be done if streaming.
> > +              */
> > +             if (vb2_is_streaming(vq) || (vb2_is_busy(vq) &&
> > +                 pix_mp->pixelformat != ctx->src_fmt.pixelformat))
> > +                     return -EBUSY;
>
> Sorry to chime in only now, but I'm currently looking at the VP9 spec
> and it seems the resolution is allowed to change dynamically [1] (I
> guess the same applies to VP8). IIU the spec correctly, coded frames
> using the new resolution can reference decoded frames using the old
> one (can be higher or lower res BTW). If we force a streamoff to change
> the resolution (as seems to be the case here), we'll lose those ref
> frames (see the hantro_return_bufs() in stop streaming), right?
> Hans, Tomasz, any idea how this dynamic resolution change could/should
> be supported?

The same problem applies to stateful decoders as well. This is an
inherent limitation of the current V4L2 API. To handle this kind of
streams we would have to make the format a per-buffer parameter,
rather than per-queue as it is defined currently.

Best regards,
Tomasz

>
> >               /*
> >                * Since format change on the OUTPUT queue will reset
> >                * the CAPTURE queue, we can't allow doing so
> > @@ -389,12 +396,15 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
> >                                         V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> >               if (vb2_is_busy(peer_vq))
> >                       return -EBUSY;
> > +     } else {
> > +             /*
> > +              * The encoder doesn't admit a format change if
> > +              * there are OUTPUT buffers allocated.
> > +              */
> > +             if (vb2_is_busy(vq))
> > +                     return -EBUSY;
> >       }
> >
> > -     ret = vidioc_try_fmt_out_mplane(file, priv, f);
> > -     if (ret)
> > -             return ret;
> > -
> >       formats = hantro_get_formats(ctx, &num_fmts);
> >       ctx->vpu_src_fmt = hantro_find_format(formats, num_fmts,
> >                                             pix_mp->pixelformat);
>
> [1] Section "5.16 Reference frame scaling" of
>     https://storage.googleapis.com/downloads.webmproject.org/docs/vp9/vp9-bitstream-specification-v0.6-20160331-draft.pdf

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes
  2019-11-08 11:52     ` Tomasz Figa
@ 2019-11-09 12:17       ` Hans Verkuil
  0 siblings, 0 replies; 28+ messages in thread
From: Hans Verkuil @ 2019-11-09 12:17 UTC (permalink / raw)
  To: Tomasz Figa, Boris Brezillon
  Cc: Ezequiel Garcia, Linux Media Mailing List, kernel,
	Nicolas Dufresne, open list:ARM/Rockchip SoC...,
	Heiko Stuebner, Jonas Karlman, Philipp Zabel, Alexandre Courbot,
	fbuergisser, Linux Kernel Mailing List, Douglas Anderson

On 11/8/19 12:52 PM, Tomasz Figa wrote:
> On Fri, Nov 8, 2019 at 7:20 PM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
>>
>> On Mon,  7 Oct 2019 14:45:02 -0300
>> Ezequiel Garcia <ezequiel@collabora.com> wrote:
>>
>>> Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders")
>>> changed the conditions under S_FMT was allowed for OUTPUT
>>> CAPTURE buffers.
>>>
>>> However, and according to the mem-to-mem stateless decoder specification,
>>> in order to support dynamic resolution changes, S_FMT should be allowed
>>> even if OUTPUT buffers have been allocated.
>>>
>>> Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a resolution
>>> modification, provided the pixel format stays the same.
>>>
>>> Tested on RK3288 platforms using ChromiumOS Video Decode/Encode Accelerator Unittests.
>>>
>>> Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders")
>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>> --
>>> v2:
>>> * Call try_fmt_out before using the format,
>>>   pointed out by Philipp.
>>>
>>>  drivers/staging/media/hantro/hantro_v4l2.c | 28 +++++++++++++++-------
>>>  1 file changed, 19 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
>>> index 3dae52abb96c..586d243cc3cc 100644
>>> --- a/drivers/staging/media/hantro/hantro_v4l2.c
>>> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
>>> @@ -367,19 +367,26 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
>>>  {
>>>       struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
>>>       struct hantro_ctx *ctx = fh_to_ctx(priv);
>>> +     struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
>>>       const struct hantro_fmt *formats;
>>>       unsigned int num_fmts;
>>> -     struct vb2_queue *vq;
>>>       int ret;
>>>
>>> -     /* Change not allowed if queue is busy. */
>>> -     vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
>>> -     if (vb2_is_busy(vq))
>>> -             return -EBUSY;
>>> +     ret = vidioc_try_fmt_out_mplane(file, priv, f);
>>> +     if (ret)
>>> +             return ret;
>>>
>>>       if (!hantro_is_encoder_ctx(ctx)) {
>>>               struct vb2_queue *peer_vq;
>>>
>>> +             /*
>>> +              * In other to support dynamic resolution change,
>>
>>                       ^ order
>>
>>> +              * the decoder admits a resolution change, as long
>>> +              * as the pixelformat remains. Can't be done if streaming.
>>> +              */
>>> +             if (vb2_is_streaming(vq) || (vb2_is_busy(vq) &&
>>> +                 pix_mp->pixelformat != ctx->src_fmt.pixelformat))
>>> +                     return -EBUSY;
>>
>> Sorry to chime in only now, but I'm currently looking at the VP9 spec
>> and it seems the resolution is allowed to change dynamically [1] (I
>> guess the same applies to VP8). IIU the spec correctly, coded frames
>> using the new resolution can reference decoded frames using the old
>> one (can be higher or lower res BTW). If we force a streamoff to change
>> the resolution (as seems to be the case here), we'll lose those ref
>> frames (see the hantro_return_bufs() in stop streaming), right?
>> Hans, Tomasz, any idea how this dynamic resolution change could/should
>> be supported?
> 
> The same problem applies to stateful decoders as well. This is an
> inherent limitation of the current V4L2 API. To handle this kind of
> streams we would have to make the format a per-buffer parameter,
> rather than per-queue as it is defined currently.

This would be interesting to implement in new streaming ioctls.

There are more reasons besides codec support why you would want that
(i.e. a resolution change on an HDMI input). It's kind of supported
since you can allocate larger buffers than needed for the current format,
but currently there is no way to see when a new resolution is received.

Related to this is the fact that while you can add new buffers on the
fly (CREATE_BUFS), you can't delete unused buffers. Also, the max number
of buffers (32) is too small for some of the more advanced scenarios.

This really needs to be addressed as well in new streaming ioctls.

Regards,

	Hans

> 
> Best regards,
> Tomasz
> 
>>
>>>               /*
>>>                * Since format change on the OUTPUT queue will reset
>>>                * the CAPTURE queue, we can't allow doing so
>>> @@ -389,12 +396,15 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
>>>                                         V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
>>>               if (vb2_is_busy(peer_vq))
>>>                       return -EBUSY;
>>> +     } else {
>>> +             /*
>>> +              * The encoder doesn't admit a format change if
>>> +              * there are OUTPUT buffers allocated.
>>> +              */
>>> +             if (vb2_is_busy(vq))
>>> +                     return -EBUSY;
>>>       }
>>>
>>> -     ret = vidioc_try_fmt_out_mplane(file, priv, f);
>>> -     if (ret)
>>> -             return ret;
>>> -
>>>       formats = hantro_get_formats(ctx, &num_fmts);
>>>       ctx->vpu_src_fmt = hantro_find_format(formats, num_fmts,
>>>                                             pix_mp->pixelformat);
>>
>> [1] Section "5.16 Reference frame scaling" of
>>     https://storage.googleapis.com/downloads.webmproject.org/docs/vp9/vp9-bitstream-specification-v0.6-20160331-draft.pdf


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes
  2019-11-08 10:19   ` Boris Brezillon
  2019-11-08 11:52     ` Tomasz Figa
@ 2019-11-09 12:25     ` Hans Verkuil
  2019-11-09 16:01       ` Ezequiel Garcia
  2019-11-09 19:13       ` Boris Brezillon
  1 sibling, 2 replies; 28+ messages in thread
From: Hans Verkuil @ 2019-11-09 12:25 UTC (permalink / raw)
  To: Boris Brezillon, Ezequiel Garcia, Tomasz Figa
  Cc: linux-media, kernel, Nicolas Dufresne, linux-rockchip,
	Heiko Stuebner, Jonas Karlman, Philipp Zabel, Alexandre Courbot,
	fbuergisser, linux-kernel, Douglas Anderson

On 11/8/19 11:19 AM, Boris Brezillon wrote:
> On Mon,  7 Oct 2019 14:45:02 -0300
> Ezequiel Garcia <ezequiel@collabora.com> wrote:
> 
>> Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders")
>> changed the conditions under S_FMT was allowed for OUTPUT
>> CAPTURE buffers.
>>
>> However, and according to the mem-to-mem stateless decoder specification,
>> in order to support dynamic resolution changes, S_FMT should be allowed
>> even if OUTPUT buffers have been allocated.
>>
>> Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a resolution
>> modification, provided the pixel format stays the same.
>>
>> Tested on RK3288 platforms using ChromiumOS Video Decode/Encode Accelerator Unittests.
>>
>> Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders")
>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>> --
>> v2:
>> * Call try_fmt_out before using the format,
>>   pointed out by Philipp.
>>
>>  drivers/staging/media/hantro/hantro_v4l2.c | 28 +++++++++++++++-------
>>  1 file changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
>> index 3dae52abb96c..586d243cc3cc 100644
>> --- a/drivers/staging/media/hantro/hantro_v4l2.c
>> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
>> @@ -367,19 +367,26 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
>>  {
>>  	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
>>  	struct hantro_ctx *ctx = fh_to_ctx(priv);
>> +	struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
>>  	const struct hantro_fmt *formats;
>>  	unsigned int num_fmts;
>> -	struct vb2_queue *vq;
>>  	int ret;
>>  
>> -	/* Change not allowed if queue is busy. */
>> -	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
>> -	if (vb2_is_busy(vq))
>> -		return -EBUSY;
>> +	ret = vidioc_try_fmt_out_mplane(file, priv, f);
>> +	if (ret)
>> +		return ret;
>>  
>>  	if (!hantro_is_encoder_ctx(ctx)) {
>>  		struct vb2_queue *peer_vq;
>>  
>> +		/*
>> +		 * In other to support dynamic resolution change,
> 
> 		      ^ order
> 
>> +		 * the decoder admits a resolution change, as long
>> +		 * as the pixelformat remains. Can't be done if streaming.
>> +		 */
>> +		if (vb2_is_streaming(vq) || (vb2_is_busy(vq) &&
>> +		    pix_mp->pixelformat != ctx->src_fmt.pixelformat))
>> +			return -EBUSY;
> 
> Sorry to chime in only now, but I'm currently looking at the VP9 spec
> and it seems the resolution is allowed to change dynamically [1] (I
> guess the same applies to VP8). IIU the spec correctly, coded frames
> using the new resolution can reference decoded frames using the old
> one (can be higher or lower res BTW). If we force a streamoff to change
> the resolution (as seems to be the case here), we'll lose those ref
> frames (see the hantro_return_bufs() in stop streaming), right?
> Hans, Tomasz, any idea how this dynamic resolution change could/should
> be supported?

As Tomasz also mentioned, supporting this is much more work, and probably
requires new streaming ioctls.

In the meantime I think this patch is fine (with the typo fixed, I can do
that), so is it OK if I merge this?

Regards,

	Hans

> 
>>  		/*
>>  		 * Since format change on the OUTPUT queue will reset
>>  		 * the CAPTURE queue, we can't allow doing so
>> @@ -389,12 +396,15 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
>>  					  V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
>>  		if (vb2_is_busy(peer_vq))
>>  			return -EBUSY;
>> +	} else {
>> +		/*
>> +		 * The encoder doesn't admit a format change if
>> +		 * there are OUTPUT buffers allocated.
>> +		 */
>> +		if (vb2_is_busy(vq))
>> +			return -EBUSY;
>>  	}
>>  
>> -	ret = vidioc_try_fmt_out_mplane(file, priv, f);
>> -	if (ret)
>> -		return ret;
>> -
>>  	formats = hantro_get_formats(ctx, &num_fmts);
>>  	ctx->vpu_src_fmt = hantro_find_format(formats, num_fmts,
>>  					      pix_mp->pixelformat);
> 
> [1] Section "5.16 Reference frame scaling" of
>     https://storage.googleapis.com/downloads.webmproject.org/docs/vp9/vp9-bitstream-specification-v0.6-20160331-draft.pdf
> 


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes
  2019-11-09 12:25     ` Hans Verkuil
@ 2019-11-09 16:01       ` Ezequiel Garcia
  2019-11-09 19:13       ` Boris Brezillon
  1 sibling, 0 replies; 28+ messages in thread
From: Ezequiel Garcia @ 2019-11-09 16:01 UTC (permalink / raw)
  To: Hans Verkuil, Boris Brezillon, Tomasz Figa
  Cc: linux-media, kernel, Nicolas Dufresne, linux-rockchip,
	Heiko Stuebner, Jonas Karlman, Philipp Zabel, Alexandre Courbot,
	fbuergisser, linux-kernel, Douglas Anderson

Hi Hans,

On Sat, 2019-11-09 at 13:25 +0100, Hans Verkuil wrote:
> On 11/8/19 11:19 AM, Boris Brezillon wrote:
> > On Mon,  7 Oct 2019 14:45:02 -0300
> > Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > 
> > > Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders")
> > > changed the conditions under S_FMT was allowed for OUTPUT
> > > CAPTURE buffers.
> > > 
> > > However, and according to the mem-to-mem stateless decoder specification,
> > > in order to support dynamic resolution changes, S_FMT should be allowed
> > > even if OUTPUT buffers have been allocated.
> > > 
> > > Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a resolution
> > > modification, provided the pixel format stays the same.
> > > 
> > > Tested on RK3288 platforms using ChromiumOS Video Decode/Encode Accelerator Unittests.
> > > 
> > > Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders")
> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > --
> > > v2:
> > > * Call try_fmt_out before using the format,
> > >   pointed out by Philipp.
> > > 
> > >  drivers/staging/media/hantro/hantro_v4l2.c | 28 +++++++++++++++-------
> > >  1 file changed, 19 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
> > > index 3dae52abb96c..586d243cc3cc 100644
> > > --- a/drivers/staging/media/hantro/hantro_v4l2.c
> > > +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> > > @@ -367,19 +367,26 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
> > >  {
> > >  	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > >  	struct hantro_ctx *ctx = fh_to_ctx(priv);
> > > +	struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> > >  	const struct hantro_fmt *formats;
> > >  	unsigned int num_fmts;
> > > -	struct vb2_queue *vq;
> > >  	int ret;
> > >  
> > > -	/* Change not allowed if queue is busy. */
> > > -	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> > > -	if (vb2_is_busy(vq))
> > > -		return -EBUSY;
> > > +	ret = vidioc_try_fmt_out_mplane(file, priv, f);
> > > +	if (ret)
> > > +		return ret;
> > >  
> > >  	if (!hantro_is_encoder_ctx(ctx)) {
> > >  		struct vb2_queue *peer_vq;
> > >  
> > > +		/*
> > > +		 * In other to support dynamic resolution change,
> > 
> > 		      ^ order
> > 
> > > +		 * the decoder admits a resolution change, as long
> > > +		 * as the pixelformat remains. Can't be done if streaming.
> > > +		 */
> > > +		if (vb2_is_streaming(vq) || (vb2_is_busy(vq) &&
> > > +		    pix_mp->pixelformat != ctx->src_fmt.pixelformat))
> > > +			return -EBUSY;
> > 
> > Sorry to chime in only now, but I'm currently looking at the VP9 spec
> > and it seems the resolution is allowed to change dynamically [1] (I
> > guess the same applies to VP8). IIU the spec correctly, coded frames
> > using the new resolution can reference decoded frames using the old
> > one (can be higher or lower res BTW). If we force a streamoff to change
> > the resolution (as seems to be the case here), we'll lose those ref
> > frames (see the hantro_return_bufs() in stop streaming), right?
> > Hans, Tomasz, any idea how this dynamic resolution change could/should
> > be supported?
> 
> As Tomasz also mentioned, supporting this is much more work, and probably
> requires new streaming ioctls.
> 
> In the meantime I think this patch is fine (with the typo fixed, I can do
> that), so is it OK if I merge this?
> 

Yes, please.

I originally posted this as a v5.4 fix, so it would be nice
if we can mark this as stable material.

Thanks,
Ezequiel


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes
  2019-11-09 12:25     ` Hans Verkuil
  2019-11-09 16:01       ` Ezequiel Garcia
@ 2019-11-09 19:13       ` Boris Brezillon
  1 sibling, 0 replies; 28+ messages in thread
From: Boris Brezillon @ 2019-11-09 19:13 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Ezequiel Garcia, Tomasz Figa, linux-media, kernel,
	Nicolas Dufresne, linux-rockchip, Heiko Stuebner, Jonas Karlman,
	Philipp Zabel, Alexandre Courbot, fbuergisser, linux-kernel,
	Douglas Anderson

On Sat, 9 Nov 2019 13:25:18 +0100
Hans Verkuil <hverkuil@xs4all.nl> wrote:

> On 11/8/19 11:19 AM, Boris Brezillon wrote:
> > On Mon,  7 Oct 2019 14:45:02 -0300
> > Ezequiel Garcia <ezequiel@collabora.com> wrote:
> >   
> >> Commit 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders")
> >> changed the conditions under S_FMT was allowed for OUTPUT
> >> CAPTURE buffers.
> >>
> >> However, and according to the mem-to-mem stateless decoder specification,
> >> in order to support dynamic resolution changes, S_FMT should be allowed
> >> even if OUTPUT buffers have been allocated.
> >>
> >> Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a resolution
> >> modification, provided the pixel format stays the same.
> >>
> >> Tested on RK3288 platforms using ChromiumOS Video Decode/Encode Accelerator Unittests.
> >>
> >> Fixes: 953aaa1492c53 ("media: rockchip/vpu: Prepare things to support decoders")
> >> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> >> --
> >> v2:
> >> * Call try_fmt_out before using the format,
> >>   pointed out by Philipp.
> >>
> >>  drivers/staging/media/hantro/hantro_v4l2.c | 28 +++++++++++++++-------
> >>  1 file changed, 19 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
> >> index 3dae52abb96c..586d243cc3cc 100644
> >> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> >> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> >> @@ -367,19 +367,26 @@ vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
> >>  {
> >>  	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> >>  	struct hantro_ctx *ctx = fh_to_ctx(priv);
> >> +	struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> >>  	const struct hantro_fmt *formats;
> >>  	unsigned int num_fmts;
> >> -	struct vb2_queue *vq;
> >>  	int ret;
> >>  
> >> -	/* Change not allowed if queue is busy. */
> >> -	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> >> -	if (vb2_is_busy(vq))
> >> -		return -EBUSY;
> >> +	ret = vidioc_try_fmt_out_mplane(file, priv, f);
> >> +	if (ret)
> >> +		return ret;
> >>  
> >>  	if (!hantro_is_encoder_ctx(ctx)) {
> >>  		struct vb2_queue *peer_vq;
> >>  
> >> +		/*
> >> +		 * In other to support dynamic resolution change,  
> > 
> > 		      ^ order
> >   
> >> +		 * the decoder admits a resolution change, as long
> >> +		 * as the pixelformat remains. Can't be done if streaming.
> >> +		 */
> >> +		if (vb2_is_streaming(vq) || (vb2_is_busy(vq) &&
> >> +		    pix_mp->pixelformat != ctx->src_fmt.pixelformat))
> >> +			return -EBUSY;  
> > 
> > Sorry to chime in only now, but I'm currently looking at the VP9 spec
> > and it seems the resolution is allowed to change dynamically [1] (I
> > guess the same applies to VP8). IIU the spec correctly, coded frames
> > using the new resolution can reference decoded frames using the old
> > one (can be higher or lower res BTW). If we force a streamoff to change
> > the resolution (as seems to be the case here), we'll lose those ref
> > frames (see the hantro_return_bufs() in stop streaming), right?
> > Hans, Tomasz, any idea how this dynamic resolution change could/should
> > be supported?  
> 
> As Tomasz also mentioned, supporting this is much more work, and probably
> requires new streaming ioctls.
> 
> In the meantime I think this patch is fine (with the typo fixed, I can do
> that), so is it OK if I merge this?

Sure, go ahead, here's my

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

in case you haven't applied the patch already.

Oh, BTW, it wasn't clear in my previous reply, but I didn't intend to
block this patch with my VP9 concerns. My only motivation was to start
a discussion on how to solve my specific issue ;-).

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, back to index

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-07 17:45 [PATCH v2 for 5.4 0/4] media: hantro: Collected fixes for v5.4 Ezequiel Garcia
2019-10-07 17:45 ` [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes Ezequiel Garcia
2019-11-08 10:19   ` Boris Brezillon
2019-11-08 11:52     ` Tomasz Figa
2019-11-09 12:17       ` Hans Verkuil
2019-11-09 12:25     ` Hans Verkuil
2019-11-09 16:01       ` Ezequiel Garcia
2019-11-09 19:13       ` Boris Brezillon
2019-10-07 17:45 ` [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288 Ezequiel Garcia
2019-10-08  5:27   ` Tomasz Figa
2019-10-08  6:31     ` Jonas Karlman
2019-10-08 10:42       ` Tomasz Figa
2019-10-08 13:35         ` Tomasz Figa
2019-10-08 13:53           ` Tomasz Figa
2019-10-08 14:12             ` Jonas Karlman
2019-10-08 20:39               ` Jonas Karlman
2019-10-10  7:27                 ` Tomasz Figa
2019-10-10  7:23               ` Tomasz Figa
2019-10-13 22:10                 ` Nicolas Dufresne
2019-10-15  3:27                   ` Tomasz Figa
2019-10-07 17:45 ` [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition Ezequiel Garcia
2019-10-07 18:33   ` Jonas Karlman
2019-10-08  3:29     ` Tomasz Figa
2019-10-08  6:23       ` Jonas Karlman
2019-10-08 10:26         ` Tomasz Figa
2019-10-08 13:57           ` Jonas Karlman
2019-10-10  7:36             ` Tomasz Figa
2019-10-07 17:45 ` [PATCH v2 for 5.4 4/4] media: hantro: Fix picture order count table enable Ezequiel Garcia

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git