2019-10-29 09:20:53

by Jonas Karlman

Subject: [PATCH v2 06/10] media: hantro: Use capture buffer width and height for H264 decoding

Calculations for motion vector buffer offset is based on width and height
from the configured capture format, lets use the same values for macroblock
width and height hw regs.

Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
Signed-off-by: Jonas Karlman <[email protected]>
---
Changes in v2:
- new patch split from "media: hantro: Fix H264 motion vector buffer offset"
---
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 71bf162eaf73..eeed11366135 100644
--- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
+++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
@@ -51,8 +51,8 @@ static void set_params(struct hantro_ctx *ctx)
vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL0);

/* Decoder control register 1. */
- reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(sps->pic_width_in_mbs_minus1 + 1) |
- G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(sps->pic_height_in_map_units_minus1 + 1) |
+ reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(MB_WIDTH(ctx->dst_fmt.width)) |
+ G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->dst_fmt.height)) |
G1_REG_DEC_CTRL1_REF_FRAMES(sps->max_num_ref_frames);
vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL1);

--
2.17.1

2019-10-31 09:22:44

by Boris Brezillon

Subject: Re: [PATCH v2 06/10] media: hantro: Use capture buffer width and height for H264 decoding

On Tue, 29 Oct 2019 01:24:50 +0000
Jonas Karlman <[email protected]> wrote:

> Calculations for motion vector buffer offset is based on width and height
> from the configured capture format, lets use the same values for macroblock
> width and height hw regs.
>
> Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
> Signed-off-by: Jonas Karlman <[email protected]>
> ---
> Changes in v2:
> - new patch split from "media: hantro: Fix H264 motion vector buffer offset"
> ---
> 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 71bf162eaf73..eeed11366135 100644
> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
> @@ -51,8 +51,8 @@ static void set_params(struct hantro_ctx *ctx)
> vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL0);
>
> /* Decoder control register 1. */
> - reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(sps->pic_width_in_mbs_minus1 + 1) |
> - G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(sps->pic_height_in_map_units_minus1 + 1) |
> + reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(MB_WIDTH(ctx->dst_fmt.width)) |
> + G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->dst_fmt.height)) |

I'm just curious, do they differ in practice? Also not sure what's been
decided for the "G1 post-proc", but if the dst/capture format res set
by the user is the scaled res (PP can scale IIRC), then you probably
shouldn't use those values here.

> G1_REG_DEC_CTRL1_REF_FRAMES(sps->max_num_ref_frames);
> vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL1);
>

2019-10-31 10:03:12

by Jonas Karlman

Subject: Re: [PATCH v2 06/10] media: hantro: Use capture buffer width and height for H264 decoding

On 2019-10-31 10:21, Boris Brezillon wrote:
> On Tue, 29 Oct 2019 01:24:50 +0000
> Jonas Karlman <[email protected]> wrote:
>
>> Calculations for motion vector buffer offset is based on width and height
>> from the configured capture format, lets use the same values for macroblock
>> width and height hw regs.
>>
>> Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1")
>> Signed-off-by: Jonas Karlman <[email protected]>
>> ---
>> Changes in v2:
>> - new patch split from "media: hantro: Fix H264 motion vector buffer offset"
>> ---
>> 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 71bf162eaf73..eeed11366135 100644
>> --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c
>> +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c
>> @@ -51,8 +51,8 @@ static void set_params(struct hantro_ctx *ctx)
>> vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL0);
>>
>> /* Decoder control register 1. */
>> - reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(sps->pic_width_in_mbs_minus1 + 1) |
>> - G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(sps->pic_height_in_map_units_minus1 + 1) |
>> + reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(MB_WIDTH(ctx->dst_fmt.width)) |
>> + G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->dst_fmt.height)) |
> I'm just curious, do they differ in practice? Also not sure what's been
> decided for the "G1 post-proc", but if the dst/capture format res set
> by the user is the scaled res (PP can scale IIRC), then you probably
> shouldn't use those values here.

You are correct, I wanted to use the same source for both size and offsets, looking at this again
both here and where is it used for offsets this probably need to change.

Do you think we can use src_fmt.width/height for size and offsets? It looks like that is what cedrus is using.

Regards,
Jonas

>
>> G1_REG_DEC_CTRL1_REF_FRAMES(sps->max_num_ref_frames);
>> vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL1);
>>
>>