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

[Jonas Karlman <jonas@kwiboo.se>]

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