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

From: Jonas Karlman <jonas@kwiboo.se>
To: Tomasz Figa <tfiga@chromium.org>,
	Ezequiel Garcia <ezequiel@collabora.com>
Cc: Linux Media Mailing List <linux-media@vger.kernel.org>,
	"kernel@collabora.com" <kernel@collabora.com>,
	Nicolas Dufresne <nicolas.dufresne@collabora.com>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	Heiko Stuebner <heiko@sntech.de>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Boris Brezillon <boris.brezillon@collabora.com>,
	Alexandre Courbot <acourbot@chromium.org>,
	"fbuergisser@chromium.org" <fbuergisser@chromium.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Douglas Anderson <dianders@chromium.org>
Subject: Re: [PATCH v2 for 5.4 2/4] media: hantro: Fix H264 max frmsize supported on RK3288
Date: Tue, 8 Oct 2019 06:31:34 +0000	[thread overview]
Message-ID: <HE1PR06MB4011EC9E93ECBB6773252247AC9A0@HE1PR06MB4011.eurprd06.prod.outlook.com> (raw)
In-Reply-To: <CAAFQd5BNu2ea3ei_imHmEwmdna0+iiSbQSv_SBsdHfP4Uh1h4Q@mail.gmail.com>

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