| Message ID | 20191007174505.10681-3-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: hantro: Collected fixes for v5.4 | expand |
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
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
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
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
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
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
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
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
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
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
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
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, }, },