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

[Nicolas Dufresne]

Le jeudi 10 octobre 2019 Ã 16:23 +0900, Tomasz Figa a Ãcrit :
> On Tue, Oct 8, 2019 at 11:12 PM Jonas Karlman <jonas@xxxxxxxxx> wrote:
> > On 2019-10-08 15:53, Tomasz Figa wrote:
> > > On Tue, Oct 8, 2019 at 10:35 PM Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote:
> > > > On Tue, Oct 8, 2019 at 7:42 PM Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote:
> > > > > On Tue, Oct 8, 2019 at 3:31 PM Jonas Karlman <jonas@xxxxxxxxx> 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@xxxxxxxxxxxxx> wrote:
> > > > > > > > From: Jonas Karlman <jonas@xxxxxxxxx>
> > > > > > > > 
> > > > > > > > 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@xxxxxxxxx>
> > > > > > > > ---
> > > > > > > > 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: signature.asc
Description: This is a digitally signed message part