diff mbox series

[v6,14/18] media: verisilicon: vp9: Use destination buffer height to compute chroma offset

Message ID 20230901124414.48497-15-benjamin.gaignard@collabora.com (mailing list archive)
State New, archived
Headers show
Series Add DELETE_BUF ioctl | expand

Commit Message

Benjamin Gaignard Sept. 1, 2023, 12:44 p.m. UTC
Source and destination buffer height may not be the same because
alignment constraint are different.
Use destination height to compute chroma offset because we target
this buffer as hardware output.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
---
 drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Jernej Škrabec Sept. 10, 2023, 1:21 p.m. UTC | #1
Hi Benjamin!

Dne petek, 01. september 2023 ob 14:44:10 CEST je Benjamin Gaignard 
napisal(a):
> Source and destination buffer height may not be the same because
> alignment constraint are different.
> Use destination height to compute chroma offset because we target
> this buffer as hardware output.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
> ---
>  drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c index
> 6db1c32fce4d..1f3f5e7ce978 100644
> --- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> +++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> @@ -93,9 +93,7 @@ static int start_prepare_run(struct hantro_ctx *ctx, const
> struct v4l2_ctrl_vp9_ static size_t chroma_offset(const struct hantro_ctx
> *ctx,
>  			    const struct v4l2_ctrl_vp9_frame 
*dec_params)
>  {
> -	int bytes_per_pixel = dec_params->bit_depth == 8 ? 1 : 2;
> -
> -	return ctx->src_fmt.width * ctx->src_fmt.height * bytes_per_pixel;
> +	return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth / 
8;

Commit message doesn't mention bit_depth change at all. While I think there is 
no difference between dec_params->bit_depth and ctx->bit_depth, you shouldn't 
just use ordinary division. If bit_depth is 10, it will be rounded down. And 
if you decide to use bit_depth from context, please remove dec_params 
argument.

Best regards,
Jernej

>  }
> 
>  static size_t mv_offset(const struct hantro_ctx *ctx,
Benjamin Gaignard Sept. 11, 2023, 8:55 a.m. UTC | #2
Le 10/09/2023 à 15:21, Jernej Škrabec a écrit :
> Hi Benjamin!
>
> Dne petek, 01. september 2023 ob 14:44:10 CEST je Benjamin Gaignard
> napisal(a):
>> Source and destination buffer height may not be the same because
>> alignment constraint are different.
>> Use destination height to compute chroma offset because we target
>> this buffer as hardware output.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
>> ---
>>   drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
>> b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c index
>> 6db1c32fce4d..1f3f5e7ce978 100644
>> --- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
>> +++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
>> @@ -93,9 +93,7 @@ static int start_prepare_run(struct hantro_ctx *ctx, const
>> struct v4l2_ctrl_vp9_ static size_t chroma_offset(const struct hantro_ctx
>> *ctx,
>>   			    const struct v4l2_ctrl_vp9_frame
> *dec_params)
>>   {
>> -	int bytes_per_pixel = dec_params->bit_depth == 8 ? 1 : 2;
>> -
>> -	return ctx->src_fmt.width * ctx->src_fmt.height * bytes_per_pixel;
>> +	return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth /
> 8;
>
> Commit message doesn't mention bit_depth change at all. While I think there is
> no difference between dec_params->bit_depth and ctx->bit_depth, you shouldn't
> just use ordinary division. If bit_depth is 10, it will be rounded down. And
> if you decide to use bit_depth from context, please remove dec_params
> argument.

I will change this patch and create a helpers function for chroma and motion vectors
offsets that VP9 and HEVC code will use since they are identical.
I don't see issue with the division. If you have in mind a solution please write it
so I could test it.

Regards,
Benjamin

>
> Best regards,
> Jernej
>
>>   }
>>
>>   static size_t mv_offset(const struct hantro_ctx *ctx,
>
>
>
>
Jernej Škrabec Sept. 11, 2023, 4:36 p.m. UTC | #3
Dne ponedeljek, 11. september 2023 ob 10:55:02 CEST je Benjamin Gaignard 
napisal(a):
> Le 10/09/2023 à 15:21, Jernej Škrabec a écrit :
> > Hi Benjamin!
> > 
> > Dne petek, 01. september 2023 ob 14:44:10 CEST je Benjamin Gaignard
> > 
> > napisal(a):
> >> Source and destination buffer height may not be the same because
> >> alignment constraint are different.
> >> Use destination height to compute chroma offset because we target
> >> this buffer as hardware output.
> >> 
> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> >> Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
> >> ---
> >> 
> >>   drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c | 4 +---
> >>   1 file changed, 1 insertion(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> >> b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c index
> >> 6db1c32fce4d..1f3f5e7ce978 100644
> >> --- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> >> +++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> >> @@ -93,9 +93,7 @@ static int start_prepare_run(struct hantro_ctx *ctx,
> >> const struct v4l2_ctrl_vp9_ static size_t chroma_offset(const struct
> >> hantro_ctx *ctx,
> >> 
> >>   			    const struct v4l2_ctrl_vp9_frame
> > 
> > *dec_params)
> > 
> >>   {
> >> 
> >> -	int bytes_per_pixel = dec_params->bit_depth == 8 ? 1 : 2;
> >> -
> >> -	return ctx->src_fmt.width * ctx->src_fmt.height * bytes_per_pixel;
> >> +	return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth /
> > 
> > 8;
> > 
> > Commit message doesn't mention bit_depth change at all. While I think
> > there is no difference between dec_params->bit_depth and ctx->bit_depth,
> > you shouldn't just use ordinary division. If bit_depth is 10, it will be
> > rounded down. And if you decide to use bit_depth from context, please
> > remove dec_params argument.
> 
> I will change this patch and create a helpers function for chroma and motion
> vectors offsets that VP9 and HEVC code will use since they are identical.
> I don't see issue with the division. If you have in mind a solution please
> write it so I could test it.

Solution is same as the code that you removed:
int bytes_per_pixel = dec_params->bit_depth == 8 ? 1 : 2;

Or alternatively:
int bytes_per_pixel = DIV_ROUND_UP(dec_params->bit_depth, 8);

Consider bit_depth being 10. With old code you get 2, with yours you get 1.

Best regards,
Jernej

> 
> Regards,
> Benjamin
> 
> > Best regards,
> > Jernej
> > 
> >>   }
> >>   
> >>   static size_t mv_offset(const struct hantro_ctx *ctx,
Benjamin Gaignard Sept. 12, 2023, 8:41 a.m. UTC | #4
Le 11/09/2023 à 18:36, Jernej Škrabec a écrit :
> Dne ponedeljek, 11. september 2023 ob 10:55:02 CEST je Benjamin Gaignard
> napisal(a):
>> Le 10/09/2023 à 15:21, Jernej Škrabec a écrit :
>>> Hi Benjamin!
>>>
>>> Dne petek, 01. september 2023 ob 14:44:10 CEST je Benjamin Gaignard
>>>
>>> napisal(a):
>>>> Source and destination buffer height may not be the same because
>>>> alignment constraint are different.
>>>> Use destination height to compute chroma offset because we target
>>>> this buffer as hardware output.
>>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>> Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
>>>> ---
>>>>
>>>>    drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c | 4 +---
>>>>    1 file changed, 1 insertion(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
>>>> b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c index
>>>> 6db1c32fce4d..1f3f5e7ce978 100644
>>>> --- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
>>>> +++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
>>>> @@ -93,9 +93,7 @@ static int start_prepare_run(struct hantro_ctx *ctx,
>>>> const struct v4l2_ctrl_vp9_ static size_t chroma_offset(const struct
>>>> hantro_ctx *ctx,
>>>>
>>>>    			    const struct v4l2_ctrl_vp9_frame
>>> *dec_params)
>>>
>>>>    {
>>>>
>>>> -	int bytes_per_pixel = dec_params->bit_depth == 8 ? 1 : 2;
>>>> -
>>>> -	return ctx->src_fmt.width * ctx->src_fmt.height * bytes_per_pixel;
>>>> +	return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth /
>>> 8;
>>>
>>> Commit message doesn't mention bit_depth change at all. While I think
>>> there is no difference between dec_params->bit_depth and ctx->bit_depth,
>>> you shouldn't just use ordinary division. If bit_depth is 10, it will be
>>> rounded down. And if you decide to use bit_depth from context, please
>>> remove dec_params argument.
>> I will change this patch and create a helpers function for chroma and motion
>> vectors offsets that VP9 and HEVC code will use since they are identical.
>> I don't see issue with the division. If you have in mind a solution please
>> write it so I could test it.
> Solution is same as the code that you removed:
> int bytes_per_pixel = dec_params->bit_depth == 8 ? 1 : 2;
>
> Or alternatively:
> int bytes_per_pixel = DIV_ROUND_UP(dec_params->bit_depth, 8);
>
> Consider bit_depth being 10. With old code you get 2, with yours you get 1.

The old code is wrong ;-)
If the format depth is 10 bits per pixel then chroma offset (in bytes) formula is
width * height * 10 / 8 not width * height * 16 / 8.

I have already confirm that with HEVC on the same hardware.

Regards,
Benjamin

>
> Best regards,
> Jernej
>
>> Regards,
>> Benjamin
>>
>>> Best regards,
>>> Jernej
>>>
>>>>    }
>>>>    
>>>>    static size_t mv_offset(const struct hantro_ctx *ctx,
>
>
>
>
Nicolas Dufresne Sept. 12, 2023, 3:26 p.m. UTC | #5
Le mardi 12 septembre 2023 à 10:41 +0200, Benjamin Gaignard a écrit :
> Le 11/09/2023 à 18:36, Jernej Škrabec a écrit :
> > Dne ponedeljek, 11. september 2023 ob 10:55:02 CEST je Benjamin Gaignard
> > napisal(a):
> > > Le 10/09/2023 à 15:21, Jernej Škrabec a écrit :
> > > > Hi Benjamin!
> > > > 
> > > > Dne petek, 01. september 2023 ob 14:44:10 CEST je Benjamin Gaignard
> > > > 
> > > > napisal(a):
> > > > > Source and destination buffer height may not be the same because
> > > > > alignment constraint are different.
> > > > > Use destination height to compute chroma offset because we target
> > > > > this buffer as hardware output.
> > > > > 
> > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > > > > Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
> > > > > ---
> > > > > 
> > > > >    drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c | 4 +---
> > > > >    1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> > > > > b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c index
> > > > > 6db1c32fce4d..1f3f5e7ce978 100644
> > > > > --- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> > > > > +++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> > > > > @@ -93,9 +93,7 @@ static int start_prepare_run(struct hantro_ctx *ctx,
> > > > > const struct v4l2_ctrl_vp9_ static size_t chroma_offset(const struct
> > > > > hantro_ctx *ctx,
> > > > > 
> > > > >    			    const struct v4l2_ctrl_vp9_frame
> > > > *dec_params)
> > > > 
> > > > >    {
> > > > > 
> > > > > -	int bytes_per_pixel = dec_params->bit_depth == 8 ? 1 : 2;
> > > > > -
> > > > > -	return ctx->src_fmt.width * ctx->src_fmt.height * bytes_per_pixel;
> > > > > +	return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth /
> > > > 8;
> > > > 
> > > > Commit message doesn't mention bit_depth change at all. While I think
> > > > there is no difference between dec_params->bit_depth and ctx->bit_depth,
> > > > you shouldn't just use ordinary division. If bit_depth is 10, it will be
> > > > rounded down. And if you decide to use bit_depth from context, please
> > > > remove dec_params argument.
> > > I will change this patch and create a helpers function for chroma and motion
> > > vectors offsets that VP9 and HEVC code will use since they are identical.
> > > I don't see issue with the division. If you have in mind a solution please
> > > write it so I could test it.
> > Solution is same as the code that you removed:
> > int bytes_per_pixel = dec_params->bit_depth == 8 ? 1 : 2;
> > 
> > Or alternatively:
> > int bytes_per_pixel = DIV_ROUND_UP(dec_params->bit_depth, 8);
> > 
> > Consider bit_depth being 10. With old code you get 2, with yours you get 1.
> 
> The old code is wrong ;-)
> If the format depth is 10 bits per pixel then chroma offset (in bytes) formula is
> width * height * 10 / 8 not width * height * 16 / 8.
> 
> I have already confirm that with HEVC on the same hardware.

Just for general interest, this is related to the fact that the reference frame
are not P010 tiled (upstreamed but untested code), but NV15 (packed) tiled. I'm
effectively missing a log of context around this patch though to comment, but
I'd like to underline that v4l2-common have all the information now to deal with
fractional pixel sizes, which gives me the impression this code is duplicating.

Nicolas

> 
> Regards,
> Benjamin
> 
> > 
> > Best regards,
> > Jernej
> > 
> > > Regards,
> > > Benjamin
> > > 
> > > > Best regards,
> > > > Jernej
> > > > 
> > > > >    }
> > > > >    
> > > > >    static size_t mv_offset(const struct hantro_ctx *ctx,
> > 
> > 
> > 
> >
Jernej Škrabec Sept. 12, 2023, 3:51 p.m. UTC | #6
Dne torek, 12. september 2023 ob 10:41:10 CEST je Benjamin Gaignard 
napisal(a):
> Le 11/09/2023 à 18:36, Jernej Škrabec a écrit :
> > Dne ponedeljek, 11. september 2023 ob 10:55:02 CEST je Benjamin Gaignard
> > 
> > napisal(a):
> >> Le 10/09/2023 à 15:21, Jernej Škrabec a écrit :
> >>> Hi Benjamin!
> >>> 
> >>> Dne petek, 01. september 2023 ob 14:44:10 CEST je Benjamin Gaignard
> >>> 
> >>> napisal(a):
> >>>> Source and destination buffer height may not be the same because
> >>>> alignment constraint are different.
> >>>> Use destination height to compute chroma offset because we target
> >>>> this buffer as hardware output.
> >>>> 
> >>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> >>>> Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
> >>>> ---
> >>>> 
> >>>>    drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c | 4 +---
> >>>>    1 file changed, 1 insertion(+), 3 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> >>>> b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c index
> >>>> 6db1c32fce4d..1f3f5e7ce978 100644
> >>>> --- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> >>>> +++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> >>>> @@ -93,9 +93,7 @@ static int start_prepare_run(struct hantro_ctx *ctx,
> >>>> const struct v4l2_ctrl_vp9_ static size_t chroma_offset(const struct
> >>>> hantro_ctx *ctx,
> >>>> 
> >>>>    			    const struct v4l2_ctrl_vp9_frame
> >>> 
> >>> *dec_params)
> >>> 
> >>>>    {
> >>>> 
> >>>> -	int bytes_per_pixel = dec_params->bit_depth == 8 ? 1 : 2;
> >>>> -
> >>>> -	return ctx->src_fmt.width * ctx->src_fmt.height * bytes_per_pixel;
> >>>> +	return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth /
> >>> 
> >>> 8;
> >>> 
> >>> Commit message doesn't mention bit_depth change at all. While I think
> >>> there is no difference between dec_params->bit_depth and ctx->bit_depth,
> >>> you shouldn't just use ordinary division. If bit_depth is 10, it will be
> >>> rounded down. And if you decide to use bit_depth from context, please
> >>> remove dec_params argument.
> >> 
> >> I will change this patch and create a helpers function for chroma and
> >> motion vectors offsets that VP9 and HEVC code will use since they are
> >> identical. I don't see issue with the division. If you have in mind a
> >> solution please write it so I could test it.
> > 
> > Solution is same as the code that you removed:
> > int bytes_per_pixel = dec_params->bit_depth == 8 ? 1 : 2;
> > 
> > Or alternatively:
> > int bytes_per_pixel = DIV_ROUND_UP(dec_params->bit_depth, 8);
> > 
> > Consider bit_depth being 10. With old code you get 2, with yours you get
> > 1.
> 
> The old code is wrong ;-)
> If the format depth is 10 bits per pixel then chroma offset (in bytes)
> formula is width * height * 10 / 8 not width * height * 16 / 8.
> 
> I have already confirm that with HEVC on the same hardware.

Ok, mention of bit_depth issue in commit log would be great. It talks only 
about width and height.

In any case, are width and/or height always dividable by 8?

Best regards,
Jernej

> 
> Regards,
> Benjamin
> 
> > Best regards,
> > Jernej
> > 
> >> Regards,
> >> Benjamin
> >> 
> >>> Best regards,
> >>> Jernej
> >>> 
> >>>>    }
> >>>>    
> >>>>    static size_t mv_offset(const struct hantro_ctx *ctx,
diff mbox series

Patch

diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
index 6db1c32fce4d..1f3f5e7ce978 100644
--- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
+++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
@@ -93,9 +93,7 @@  static int start_prepare_run(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_
 static size_t chroma_offset(const struct hantro_ctx *ctx,
 			    const struct v4l2_ctrl_vp9_frame *dec_params)
 {
-	int bytes_per_pixel = dec_params->bit_depth == 8 ? 1 : 2;
-
-	return ctx->src_fmt.width * ctx->src_fmt.height * bytes_per_pixel;
+	return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth / 8;
 }
 
 static size_t mv_offset(const struct hantro_ctx *ctx,