[PATCH v2 for 5.4 0/4] media: hantro: Collected fixes for v5.4

[PATCH v2 for 5.4 0/4] media: hantro: Collected fixes for v5.4

[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

[PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes

[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

[PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288

[Ezequiel Garcia]

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

[PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition

[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

[PATCH v2 for 5.4 4/4] media: hantro: Fix picture order count table enable

[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

Re: [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition

[Jonas Karlman]

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) &&

Re: [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition

[Tomasz Figa]

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) &&
>

Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288

[Tomasz Figa]

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

Re: [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition

[Jonas Karlman]

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) &&

Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288

[Jonas Karlman]

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

Re: [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition

[Tomasz Figa]

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) &&
>

Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288

[Tomasz Figa]

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

Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288

[Tomasz Figa]

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

Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288

[Tomasz Figa]

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

Re: [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition

[Jonas Karlman]

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) &&

Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288

[Jonas Karlman]

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

Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288

[Jonas Karlman]

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

Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288

[Tomasz Figa]

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

Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288

[Tomasz Figa]

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

Re: [PATCH v2 for 5.4 3/4] media: hantro: Fix motion vectors usage condition

[Tomasz Figa]

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) &&
>

Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288

[Nicolas Dufresne]

[-- 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 --]

Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288

[Tomasz Figa]

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

Re: [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes

[Boris Brezillon]

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

Re: [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes

[Tomasz Figa]

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

Re: [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes

[Hans Verkuil]

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

Re: [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes

[Hans Verkuil]

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
> 

Re: [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes

[Ezequiel Garcia]

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

Re: [PATCH v2 for 5.4 1/4] media: hantro: Fix s_fmt for dynamic resolution changes

[Boris Brezillon]

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 ;-).