diff mbox series

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

Message ID HE1PR06MB4011544CF7A6F36EF1CA47EEAC610@HE1PR06MB4011.eurprd06.prod.outlook.com (mailing list archive)
State New, archived
Headers show
Series media: hantro: H264 fixes and improvements | expand

Commit Message

Jonas Karlman Oct. 29, 2019, 1:24 a.m. UTC
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 <jonas@kwiboo.se>
---
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(-)

Comments

Boris Brezillon Oct. 31, 2019, 9:21 a.m. UTC | #1
On Tue, 29 Oct 2019 01:24:50 +0000
Jonas Karlman <jonas@kwiboo.se> 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 <jonas@kwiboo.se>
> ---
> 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);
>
Jonas Karlman Oct. 31, 2019, 10 a.m. UTC | #2
On 2019-10-31 10:21, Boris Brezillon wrote:
> On Tue, 29 Oct 2019 01:24:50 +0000
> Jonas Karlman <jonas@kwiboo.se> 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 <jonas@kwiboo.se>
>> ---
>> 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);
>>  
>>
diff mbox series

Patch

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