* [PATCH v9 1/6] media: verisilicon: Do not set context src/dst formats in reset functions
2023-02-20 10:48 [PATCH v9 0/6] media: verisilicon: HEVC: fix 10bits handling Benjamin Gaignard
@ 2023-02-20 10:48 ` Benjamin Gaignard
2023-02-26 12:58 ` Ezequiel Garcia
` (2 more replies)
2023-02-20 10:48 ` [PATCH v9 2/6] media: verisilicon: Do not use ctx fields as format storage when resetting Benjamin Gaignard
` (4 subsequent siblings)
5 siblings, 3 replies; 21+ messages in thread
From: Benjamin Gaignard @ 2023-02-20 10:48 UTC (permalink / raw)
To: ezequiel, p.zabel, mchehab, shawnguo, s.hauer, kernel, festevam,
linux-imx, hverkuil-cisco, nicolas.dufresne, robert.mader
Cc: linux-media, linux-rockchip, linux-kernel, linux-arm-kernel,
kernel, Benjamin Gaignard
Setting context source and destination formats should only be done
in hantro_set_fmt_out() and hantro_set_fmt_cap() after check that
the targeted queue is not busy.
Remove these calls from hantro_reset_encoded_fmt() and
hantro_reset_raw_fmt() to clean the driver.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
drivers/media/platform/verisilicon/hantro_v4l2.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
index c0d427956210..d8aa42bd4cd4 100644
--- a/drivers/media/platform/verisilicon/hantro_v4l2.c
+++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
@@ -382,13 +382,10 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
vpu_fmt = hantro_get_default_fmt(ctx, true);
- if (ctx->is_encoder) {
- ctx->vpu_dst_fmt = vpu_fmt;
+ if (ctx->is_encoder)
fmt = &ctx->dst_fmt;
- } else {
- ctx->vpu_src_fmt = vpu_fmt;
+ else
fmt = &ctx->src_fmt;
- }
hantro_reset_fmt(fmt, vpu_fmt);
fmt->width = vpu_fmt->frmsize.min_width;
@@ -408,11 +405,9 @@ hantro_reset_raw_fmt(struct hantro_ctx *ctx)
raw_vpu_fmt = hantro_get_default_fmt(ctx, false);
if (ctx->is_encoder) {
- ctx->vpu_src_fmt = raw_vpu_fmt;
raw_fmt = &ctx->src_fmt;
encoded_fmt = &ctx->dst_fmt;
} else {
- ctx->vpu_dst_fmt = raw_vpu_fmt;
raw_fmt = &ctx->dst_fmt;
encoded_fmt = &ctx->src_fmt;
}
--
2.34.1
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 1/6] media: verisilicon: Do not set context src/dst formats in reset functions
2023-02-20 10:48 ` [PATCH v9 1/6] media: verisilicon: Do not set context src/dst formats in reset functions Benjamin Gaignard
@ 2023-02-26 12:58 ` Ezequiel Garcia
[not found] ` <CGME20230412161415eucas1p1536b537c3f866e9820d3bea8bb9ea2d9@eucas1p1.samsung.com>
2023-04-26 22:19 ` Shreeya Patel
2 siblings, 0 replies; 21+ messages in thread
From: Ezequiel Garcia @ 2023-02-26 12:58 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: p.zabel, mchehab, shawnguo, s.hauer, kernel, festevam, linux-imx,
hverkuil-cisco, nicolas.dufresne, robert.mader, linux-media,
linux-rockchip, linux-kernel, linux-arm-kernel, kernel
On Mon, Feb 20 2023 at 11:48:44 AM +0100, Benjamin Gaignard
<benjamin.gaignard@collabora.com> wrote:
> Setting context source and destination formats should only be done
> in hantro_set_fmt_out() and hantro_set_fmt_cap() after check that
> the targeted queue is not busy.
> Remove these calls from hantro_reset_encoded_fmt() and
> hantro_reset_raw_fmt() to clean the driver.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
> drivers/media/platform/verisilicon/hantro_v4l2.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c
> b/drivers/media/platform/verisilicon/hantro_v4l2.c
> index c0d427956210..d8aa42bd4cd4 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> @@ -382,13 +382,10 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
>
> vpu_fmt = hantro_get_default_fmt(ctx, true);
>
> - if (ctx->is_encoder) {
> - ctx->vpu_dst_fmt = vpu_fmt;
> + if (ctx->is_encoder)
> fmt = &ctx->dst_fmt;
> - } else {
> - ctx->vpu_src_fmt = vpu_fmt;
> + else
> fmt = &ctx->src_fmt;
> - }
>
> hantro_reset_fmt(fmt, vpu_fmt);
> fmt->width = vpu_fmt->frmsize.min_width;
> @@ -408,11 +405,9 @@ hantro_reset_raw_fmt(struct hantro_ctx *ctx)
> raw_vpu_fmt = hantro_get_default_fmt(ctx, false);
>
> if (ctx->is_encoder) {
> - ctx->vpu_src_fmt = raw_vpu_fmt;
> raw_fmt = &ctx->src_fmt;
> encoded_fmt = &ctx->dst_fmt;
> } else {
> - ctx->vpu_dst_fmt = raw_vpu_fmt;
> raw_fmt = &ctx->dst_fmt;
> encoded_fmt = &ctx->src_fmt;
> }
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread[parent not found: <CGME20230412161415eucas1p1536b537c3f866e9820d3bea8bb9ea2d9@eucas1p1.samsung.com>]
* Re: [PATCH v9 1/6] media: verisilicon: Do not set context src/dst formats in reset functions
[not found] ` <CGME20230412161415eucas1p1536b537c3f866e9820d3bea8bb9ea2d9@eucas1p1.samsung.com>
@ 2023-04-12 16:14 ` Marek Szyprowski
2023-04-12 16:40 ` Benjamin Gaignard
0 siblings, 1 reply; 21+ messages in thread
From: Marek Szyprowski @ 2023-04-12 16:14 UTC (permalink / raw)
To: Benjamin Gaignard, ezequiel, p.zabel, mchehab, shawnguo, s.hauer,
kernel, festevam, linux-imx, hverkuil-cisco, nicolas.dufresne,
robert.mader
Cc: linux-media, linux-rockchip, linux-kernel, linux-arm-kernel, kernel
Hi,
On 20.02.2023 11:48, Benjamin Gaignard wrote:
> Setting context source and destination formats should only be done
> in hantro_set_fmt_out() and hantro_set_fmt_cap() after check that
> the targeted queue is not busy.
> Remove these calls from hantro_reset_encoded_fmt() and
> hantro_reset_raw_fmt() to clean the driver.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
This patch landed recently in linux-next as commit db6f68b51e5c ("media:
verisilicon: Do not set context src/dst formats in reset functions").
Unfortunately it causes the following regression during Debian boot on
Odroid-M1 board:
--->8---
hantro-vpu fdea0000.video-codec: Adding to iommu group 0
hantro-vpu fdea0000.video-codec: registered rockchip,rk3568-vpu-dec as
/dev/video0
hantro-vpu fdee0000.video-codec: Adding to iommu group 1
hantro-vpu fdee0000.video-codec: registered rockchip,rk3568-vepu-enc as
/dev/video1
Unable to handle kernel NULL pointer dereference at virtual address
0000000000000008
Mem abort info:
ESR = 0x0000000096000004
EC = 0x25: DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
FSC = 0x04: level 0 translation fault
Data abort info:
ISV = 0, ISS = 0x00000004
CM = 0, WnR = 0
user pgtable: 4k pages, 48-bit VAs, pgdp=00000001f446f000
[0000000000000008] pgd=0000000000000000, p4d=0000000000000000
Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
Modules linked in: hantro_vpu v4l2_vp9 v4l2_h264 v4l2_mem2mem
videobuf2_dma_contig snd_soc_simple_card display_connector
snd_soc_simple_card_utils videobuf2_memops crct10dif_ce dwmac_rk
rockchip_thermal videobuf2_v4l2 stmmac_platform rockchip_saradc
industrialio_triggered_buffer kfifo_buf stmmac videodev pcs_xpcs
rtc_rk808 videobuf2_common rockchipdrm panfrost mc drm_shmem_helper
analogix_dp gpu_sched dw_mipi_dsi dw_hdmi drm_display_helper ip_tables
x_tables ipv6
CPU: 3 PID: 171 Comm: v4l_id Not tainted 6.3.0-rc2+ #13478
Hardware name: Hardkernel ODROID-M1 (DT)
pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : hantro_try_fmt+0xb4/0x280 [hantro_vpu]
lr : hantro_try_fmt+0xa8/0x280 [hantro_vpu]
...
Call trace:
hantro_try_fmt+0xb4/0x280 [hantro_vpu]
hantro_set_fmt_out+0x3c/0x278 [hantro_vpu]
hantro_reset_raw_fmt+0x94/0xb4 [hantro_vpu]
hantro_set_fmt_cap+0x23c/0x250 [hantro_vpu]
hantro_reset_fmts+0x94/0xcc [hantro_vpu]
hantro_open+0xd4/0x20c [hantro_vpu]
v4l2_open+0x80/0x120 [videodev]
chrdev_open+0xc0/0x22c
do_dentry_open+0x13c/0x490
vfs_open+0x2c/0x38
path_openat+0x550/0x938
do_filp_open+0x80/0x12c
do_sys_openat2+0xb4/0x16c
__arm64_sys_openat+0x64/0xac
invoke_syscall+0x48/0x114
el0_svc_common.constprop.0+0xfc/0x11c
do_el0_svc+0x38/0xa4
el0_svc+0x48/0xb8
el0t_64_sync_handler+0xb8/0xbc
el0t_64_sync+0x190/0x194
Code: 97fe726c f940aa80 52864a61 72a686c1 (b9400800)
---[ end trace 0000000000000000 ]---
I know that v4l_id tool, which is a part of systemd/udev, is known to
crash badly on various vendor kernels (fixing this would be a really
hard, especially assuming the brokenness of some vendor hacks), but I
hoped that at least it should not be able to crash the mainline kernel.
> ---
> drivers/media/platform/verisilicon/hantro_v4l2.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
> index c0d427956210..d8aa42bd4cd4 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> @@ -382,13 +382,10 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
>
> vpu_fmt = hantro_get_default_fmt(ctx, true);
>
> - if (ctx->is_encoder) {
> - ctx->vpu_dst_fmt = vpu_fmt;
> + if (ctx->is_encoder)
> fmt = &ctx->dst_fmt;
> - } else {
> - ctx->vpu_src_fmt = vpu_fmt;
> + else
> fmt = &ctx->src_fmt;
> - }
>
> hantro_reset_fmt(fmt, vpu_fmt);
> fmt->width = vpu_fmt->frmsize.min_width;
> @@ -408,11 +405,9 @@ hantro_reset_raw_fmt(struct hantro_ctx *ctx)
> raw_vpu_fmt = hantro_get_default_fmt(ctx, false);
>
> if (ctx->is_encoder) {
> - ctx->vpu_src_fmt = raw_vpu_fmt;
> raw_fmt = &ctx->src_fmt;
> encoded_fmt = &ctx->dst_fmt;
> } else {
> - ctx->vpu_dst_fmt = raw_vpu_fmt;
> raw_fmt = &ctx->dst_fmt;
> encoded_fmt = &ctx->src_fmt;
> }
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 1/6] media: verisilicon: Do not set context src/dst formats in reset functions
2023-04-12 16:14 ` Marek Szyprowski
@ 2023-04-12 16:40 ` Benjamin Gaignard
2023-04-12 16:53 ` Marek Szyprowski
0 siblings, 1 reply; 21+ messages in thread
From: Benjamin Gaignard @ 2023-04-12 16:40 UTC (permalink / raw)
To: Marek Szyprowski, ezequiel, p.zabel, mchehab, shawnguo, s.hauer,
kernel, festevam, linux-imx, hverkuil-cisco, nicolas.dufresne,
robert.mader
Cc: linux-media, linux-rockchip, linux-kernel, linux-arm-kernel, kernel
[-- Attachment #1: Type: text/plain, Size: 4877 bytes --]
Le 12/04/2023 à 18:14, Marek Szyprowski a écrit :
> Hi,
>
> On 20.02.2023 11:48, Benjamin Gaignard wrote:
>> Setting context source and destination formats should only be done
>> in hantro_set_fmt_out() and hantro_set_fmt_cap() after check that
>> the targeted queue is not busy.
>> Remove these calls from hantro_reset_encoded_fmt() and
>> hantro_reset_raw_fmt() to clean the driver.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> This patch landed recently in linux-next as commit db6f68b51e5c ("media:
> verisilicon: Do not set context src/dst formats in reset functions").
Hi,
I do not have this board up and running with Hantro encoder but
I think the attached patch may solve the issue.
Could you tell me if it works ?
Regards,
Benjamin
>
> Unfortunately it causes the following regression during Debian boot on
> Odroid-M1 board:
>
> --->8---
>
> hantro-vpu fdea0000.video-codec: Adding to iommu group 0
> hantro-vpu fdea0000.video-codec: registered rockchip,rk3568-vpu-dec as
> /dev/video0
> hantro-vpu fdee0000.video-codec: Adding to iommu group 1
> hantro-vpu fdee0000.video-codec: registered rockchip,rk3568-vepu-enc as
> /dev/video1
> Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000008
> Mem abort info:
> ESR = 0x0000000096000004
> EC = 0x25: DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> FSC = 0x04: level 0 translation fault
> Data abort info:
> ISV = 0, ISS = 0x00000004
> CM = 0, WnR = 0
> user pgtable: 4k pages, 48-bit VAs, pgdp=00000001f446f000
> [0000000000000008] pgd=0000000000000000, p4d=0000000000000000
> Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> Modules linked in: hantro_vpu v4l2_vp9 v4l2_h264 v4l2_mem2mem
> videobuf2_dma_contig snd_soc_simple_card display_connector
> snd_soc_simple_card_utils videobuf2_memops crct10dif_ce dwmac_rk
> rockchip_thermal videobuf2_v4l2 stmmac_platform rockchip_saradc
> industrialio_triggered_buffer kfifo_buf stmmac videodev pcs_xpcs
> rtc_rk808 videobuf2_common rockchipdrm panfrost mc drm_shmem_helper
> analogix_dp gpu_sched dw_mipi_dsi dw_hdmi drm_display_helper ip_tables
> x_tables ipv6
> CPU: 3 PID: 171 Comm: v4l_id Not tainted 6.3.0-rc2+ #13478
> Hardware name: Hardkernel ODROID-M1 (DT)
> pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : hantro_try_fmt+0xb4/0x280 [hantro_vpu]
> lr : hantro_try_fmt+0xa8/0x280 [hantro_vpu]
> ...
> Call trace:
> hantro_try_fmt+0xb4/0x280 [hantro_vpu]
> hantro_set_fmt_out+0x3c/0x278 [hantro_vpu]
> hantro_reset_raw_fmt+0x94/0xb4 [hantro_vpu]
> hantro_set_fmt_cap+0x23c/0x250 [hantro_vpu]
> hantro_reset_fmts+0x94/0xcc [hantro_vpu]
> hantro_open+0xd4/0x20c [hantro_vpu]
> v4l2_open+0x80/0x120 [videodev]
> chrdev_open+0xc0/0x22c
> do_dentry_open+0x13c/0x490
> vfs_open+0x2c/0x38
> path_openat+0x550/0x938
> do_filp_open+0x80/0x12c
> do_sys_openat2+0xb4/0x16c
> __arm64_sys_openat+0x64/0xac
> invoke_syscall+0x48/0x114
> el0_svc_common.constprop.0+0xfc/0x11c
> do_el0_svc+0x38/0xa4
> el0_svc+0x48/0xb8
> el0t_64_sync_handler+0xb8/0xbc
> el0t_64_sync+0x190/0x194
> Code: 97fe726c f940aa80 52864a61 72a686c1 (b9400800)
> ---[ end trace 0000000000000000 ]---
>
> I know that v4l_id tool, which is a part of systemd/udev, is known to
> crash badly on various vendor kernels (fixing this would be a really
> hard, especially assuming the brokenness of some vendor hacks), but I
> hoped that at least it should not be able to crash the mainline kernel.
>
>
>> ---
>> drivers/media/platform/verisilicon/hantro_v4l2.c | 9 ++-------
>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
>> index c0d427956210..d8aa42bd4cd4 100644
>> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
>> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
>> @@ -382,13 +382,10 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
>>
>> vpu_fmt = hantro_get_default_fmt(ctx, true);
>>
>> - if (ctx->is_encoder) {
>> - ctx->vpu_dst_fmt = vpu_fmt;
>> + if (ctx->is_encoder)
>> fmt = &ctx->dst_fmt;
>> - } else {
>> - ctx->vpu_src_fmt = vpu_fmt;
>> + else
>> fmt = &ctx->src_fmt;
>> - }
>>
>> hantro_reset_fmt(fmt, vpu_fmt);
>> fmt->width = vpu_fmt->frmsize.min_width;
>> @@ -408,11 +405,9 @@ hantro_reset_raw_fmt(struct hantro_ctx *ctx)
>> raw_vpu_fmt = hantro_get_default_fmt(ctx, false);
>>
>> if (ctx->is_encoder) {
>> - ctx->vpu_src_fmt = raw_vpu_fmt;
>> raw_fmt = &ctx->src_fmt;
>> encoded_fmt = &ctx->dst_fmt;
>> } else {
>> - ctx->vpu_dst_fmt = raw_vpu_fmt;
>> raw_fmt = &ctx->dst_fmt;
>> encoded_fmt = &ctx->src_fmt;
>> }
> Best regards
[-- Attachment #2: 0001-media-verisilicon-Fix-crash-when-probing-encoder.patch --]
[-- Type: text/x-patch, Size: 1125 bytes --]
From c601ccc9b98a3f735493faf8487dbfa59ec4e0c6 Mon Sep 17 00:00:00 2001
From: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Date: Wed, 12 Apr 2023 18:38:29 +0200
Subject: [PATCH] media: verisilicon: Fix crash when probing encoder
ctx->vpu_dst_fmt is no more initialized before calling hantro_try_fmt()
This led to crash the kernel.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Fixes: db6f68b51e5c ("media: verisilicon: Do not set context src/dst formats in reset functions")
---
drivers/media/platform/verisilicon/hantro_v4l2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
index 8f1414085f47..e8bcb6d669fc 100644
--- a/drivers/media/platform/verisilicon/hantro_v4l2.c
+++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
@@ -297,7 +297,7 @@ static int hantro_try_fmt(const struct hantro_ctx *ctx,
pix_mp->num_planes = 1;
vpu_fmt = fmt;
} else if (ctx->is_encoder) {
- vpu_fmt = ctx->vpu_dst_fmt;
+ vpu_fmt = fmt;
} else {
vpu_fmt = fmt;
/*
--
2.34.1
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 1/6] media: verisilicon: Do not set context src/dst formats in reset functions
2023-04-12 16:40 ` Benjamin Gaignard
@ 2023-04-12 16:53 ` Marek Szyprowski
0 siblings, 0 replies; 21+ messages in thread
From: Marek Szyprowski @ 2023-04-12 16:53 UTC (permalink / raw)
To: Benjamin Gaignard, ezequiel, p.zabel, mchehab, shawnguo, s.hauer,
kernel, festevam, linux-imx, hverkuil-cisco, nicolas.dufresne,
robert.mader
Cc: linux-media, linux-rockchip, linux-kernel, linux-arm-kernel, kernel
Hi,
On 12.04.2023 18:40, Benjamin Gaignard wrote:
>
> Le 12/04/2023 à 18:14, Marek Szyprowski a écrit :
>> Hi,
>>
>> On 20.02.2023 11:48, Benjamin Gaignard wrote:
>>> Setting context source and destination formats should only be done
>>> in hantro_set_fmt_out() and hantro_set_fmt_cap() after check that
>>> the targeted queue is not busy.
>>> Remove these calls from hantro_reset_encoded_fmt() and
>>> hantro_reset_raw_fmt() to clean the driver.
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> This patch landed recently in linux-next as commit db6f68b51e5c ("media:
>> verisilicon: Do not set context src/dst formats in reset functions").
>
> Hi,
>
> I do not have this board up and running with Hantro encoder but
> I think the attached patch may solve the issue.
> Could you tell me if it works ?
Yep, it fixes the issue.
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
It looks that the code could be a bit more cleaned up, as with the
attached patch, there is such construction:
if (coded) {
pix_mp->num_planes = 1;
vpu_fmt = fmt;
} else if (ctx->is_encoder) {
vpu_fmt = fmt;
} else {
vpu_fmt = fmt;
/*
* Width/height on the CAPTURE end of a decoder are
ignored and
* replaced by the OUTPUT ones.
*/
pix_mp->width = ctx->src_fmt.width;
pix_mp->height = ctx->src_fmt.height;
}
Common 'vpu_fmt = fmt' can be moved out of the above if-else block.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v9 1/6] media: verisilicon: Do not set context src/dst formats in reset functions
2023-02-20 10:48 ` [PATCH v9 1/6] media: verisilicon: Do not set context src/dst formats in reset functions Benjamin Gaignard
2023-02-26 12:58 ` Ezequiel Garcia
[not found] ` <CGME20230412161415eucas1p1536b537c3f866e9820d3bea8bb9ea2d9@eucas1p1.samsung.com>
@ 2023-04-26 22:19 ` Shreeya Patel
2023-05-01 7:21 ` Thorsten Leemhuis
2 siblings, 1 reply; 21+ messages in thread
From: Shreeya Patel @ 2023-04-26 22:19 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: linux-media, linux-rockchip, linux-kernel, linux-arm-kernel,
kernel, robert.mader, nicolas.dufresne, ezequiel, festevam,
p.zabel, mchehab, shawnguo, s.hauer, kernel, hverkuil-cisco,
linux-imx, regressions
Hi Benjamin,
On 20/02/23 16:18, Benjamin Gaignard wrote:
> Setting context source and destination formats should only be done
> in hantro_set_fmt_out() and hantro_set_fmt_cap() after check that
> the targeted queue is not busy.
> Remove these calls from hantro_reset_encoded_fmt() and
> hantro_reset_raw_fmt() to clean the driver.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
KernelCI found this patch causes a regression in the
baseline.dmesg.alert test [1] on rk3399-rock-pi-4b [2],
see the bisection report for more details [3].
Let us know if you have any questions.
[1] https://github.com/kernelci/kernelci-core/blob/main/config/rootfs/debos/overlays/baseline/opt/kernelci/dmesg.sh
[2] https://linux.kernelci.org/test/case/id/6442e825f19134d74c2e865d/
[3] https://groups.io/g/kernelci-results/message/40740
Thanks,
Shreeya Patel
#regzbot introduced: db6f68b51e5c
> ---
> drivers/media/platform/verisilicon/hantro_v4l2.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
> index c0d427956210..d8aa42bd4cd4 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> @@ -382,13 +382,10 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
>
> vpu_fmt = hantro_get_default_fmt(ctx, true);
>
> - if (ctx->is_encoder) {
> - ctx->vpu_dst_fmt = vpu_fmt;
> + if (ctx->is_encoder)
> fmt = &ctx->dst_fmt;
> - } else {
> - ctx->vpu_src_fmt = vpu_fmt;
> + else
> fmt = &ctx->src_fmt;
> - }
>
> hantro_reset_fmt(fmt, vpu_fmt);
> fmt->width = vpu_fmt->frmsize.min_width;
> @@ -408,11 +405,9 @@ hantro_reset_raw_fmt(struct hantro_ctx *ctx)
> raw_vpu_fmt = hantro_get_default_fmt(ctx, false);
>
> if (ctx->is_encoder) {
> - ctx->vpu_src_fmt = raw_vpu_fmt;
> raw_fmt = &ctx->src_fmt;
> encoded_fmt = &ctx->dst_fmt;
> } else {
> - ctx->vpu_dst_fmt = raw_vpu_fmt;
> raw_fmt = &ctx->dst_fmt;
> encoded_fmt = &ctx->src_fmt;
> }
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 1/6] media: verisilicon: Do not set context src/dst formats in reset functions
2023-04-26 22:19 ` Shreeya Patel
@ 2023-05-01 7:21 ` Thorsten Leemhuis
2023-05-02 6:56 ` Benjamin Gaignard
0 siblings, 1 reply; 21+ messages in thread
From: Thorsten Leemhuis @ 2023-05-01 7:21 UTC (permalink / raw)
To: Shreeya Patel, Benjamin Gaignard
Cc: linux-media, linux-rockchip, linux-kernel, linux-arm-kernel,
kernel, robert.mader, nicolas.dufresne, ezequiel, festevam,
p.zabel, mchehab, shawnguo, s.hauer, kernel, hverkuil-cisco,
linux-imx, regressions
On 27.04.23 00:19, Shreeya Patel wrote:
> On 20/02/23 16:18, Benjamin Gaignard wrote:
>> Setting context source and destination formats should only be done
>> in hantro_set_fmt_out() and hantro_set_fmt_cap() after check that
>> the targeted queue is not busy.
>> Remove these calls from hantro_reset_encoded_fmt() and
>> hantro_reset_raw_fmt() to clean the driver.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>
> KernelCI found this patch causes a regression in the
> baseline.dmesg.alert test [1] on rk3399-rock-pi-4b [2],
> see the bisection report for more details [3].
>
> Let us know if you have any questions.
>
>
> [1]
> https://github.com/kernelci/kernelci-core/blob/main/config/rootfs/debos/overlays/baseline/opt/kernelci/dmesg.sh
> [2] https://linux.kernelci.org/test/case/id/6442e825f19134d74c2e865d/
> [3] https://groups.io/g/kernelci-results/message/40740
Thx for the report. FWIW, regzbot noticed there is a patch that refers
to the culprit that might have been landed in next after your test ran
for the last time (and meanwhile it was mainlined): f100ce3bbd6 ("media:
verisilicon: Fix crash when probing encoder")
I wonder if that is related or might even fix the issue.
Ciao, Thorsten
>> ---
>> drivers/media/platform/verisilicon/hantro_v4l2.c | 9 ++-------
>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c
>> b/drivers/media/platform/verisilicon/hantro_v4l2.c
>> index c0d427956210..d8aa42bd4cd4 100644
>> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
>> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
>> @@ -382,13 +382,10 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
>> vpu_fmt = hantro_get_default_fmt(ctx, true);
>> - if (ctx->is_encoder) {
>> - ctx->vpu_dst_fmt = vpu_fmt;
>> + if (ctx->is_encoder)
>> fmt = &ctx->dst_fmt;
>> - } else {
>> - ctx->vpu_src_fmt = vpu_fmt;
>> + else
>> fmt = &ctx->src_fmt;
>> - }
>> hantro_reset_fmt(fmt, vpu_fmt);
>> fmt->width = vpu_fmt->frmsize.min_width;
>> @@ -408,11 +405,9 @@ hantro_reset_raw_fmt(struct hantro_ctx *ctx)
>> raw_vpu_fmt = hantro_get_default_fmt(ctx, false);
>> if (ctx->is_encoder) {
>> - ctx->vpu_src_fmt = raw_vpu_fmt;
>> raw_fmt = &ctx->src_fmt;
>> encoded_fmt = &ctx->dst_fmt;
>> } else {
>> - ctx->vpu_dst_fmt = raw_vpu_fmt;
>> raw_fmt = &ctx->dst_fmt;
>> encoded_fmt = &ctx->src_fmt;
>> }
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 1/6] media: verisilicon: Do not set context src/dst formats in reset functions
2023-05-01 7:21 ` Thorsten Leemhuis
@ 2023-05-02 6:56 ` Benjamin Gaignard
2023-05-02 11:12 ` Linux regression tracking (Thorsten Leemhuis)
0 siblings, 1 reply; 21+ messages in thread
From: Benjamin Gaignard @ 2023-05-02 6:56 UTC (permalink / raw)
To: Thorsten Leemhuis, Shreeya Patel
Cc: linux-media, linux-rockchip, linux-kernel, linux-arm-kernel,
kernel, robert.mader, nicolas.dufresne, ezequiel, festevam,
p.zabel, mchehab, shawnguo, s.hauer, kernel, hverkuil-cisco,
linux-imx, regressions
Le 01/05/2023 à 09:21, Thorsten Leemhuis a écrit :
> On 27.04.23 00:19, Shreeya Patel wrote:
>> On 20/02/23 16:18, Benjamin Gaignard wrote:
>>> Setting context source and destination formats should only be done
>>> in hantro_set_fmt_out() and hantro_set_fmt_cap() after check that
>>> the targeted queue is not busy.
>>> Remove these calls from hantro_reset_encoded_fmt() and
>>> hantro_reset_raw_fmt() to clean the driver.
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> KernelCI found this patch causes a regression in the
>> baseline.dmesg.alert test [1] on rk3399-rock-pi-4b [2],
>> see the bisection report for more details [3].
>>
>> Let us know if you have any questions.
>>
>>
>> [1]
>> https://github.com/kernelci/kernelci-core/blob/main/config/rootfs/debos/overlays/baseline/opt/kernelci/dmesg.sh
>> [2] https://linux.kernelci.org/test/case/id/6442e825f19134d74c2e865d/
>> [3] https://groups.io/g/kernelci-results/message/40740
> Thx for the report. FWIW, regzbot noticed there is a patch that refers
> to the culprit that might have been landed in next after your test ran
> for the last time (and meanwhile it was mainlined): f100ce3bbd6 ("media:
> verisilicon: Fix crash when probing encoder")
Yes that patch should fix the probing issue.
Marek is working on an additional one to fix pixel format negotiation
but that doesn't impact the boot.
Regards,
Benjamin
>
> I wonder if that is related or might even fix the issue.
>
> Ciao, Thorsten
>
>>> ---
>>> drivers/media/platform/verisilicon/hantro_v4l2.c | 9 ++-------
>>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c
>>> b/drivers/media/platform/verisilicon/hantro_v4l2.c
>>> index c0d427956210..d8aa42bd4cd4 100644
>>> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
>>> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
>>> @@ -382,13 +382,10 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
>>> vpu_fmt = hantro_get_default_fmt(ctx, true);
>>> - if (ctx->is_encoder) {
>>> - ctx->vpu_dst_fmt = vpu_fmt;
>>> + if (ctx->is_encoder)
>>> fmt = &ctx->dst_fmt;
>>> - } else {
>>> - ctx->vpu_src_fmt = vpu_fmt;
>>> + else
>>> fmt = &ctx->src_fmt;
>>> - }
>>> hantro_reset_fmt(fmt, vpu_fmt);
>>> fmt->width = vpu_fmt->frmsize.min_width;
>>> @@ -408,11 +405,9 @@ hantro_reset_raw_fmt(struct hantro_ctx *ctx)
>>> raw_vpu_fmt = hantro_get_default_fmt(ctx, false);
>>> if (ctx->is_encoder) {
>>> - ctx->vpu_src_fmt = raw_vpu_fmt;
>>> raw_fmt = &ctx->src_fmt;
>>> encoded_fmt = &ctx->dst_fmt;
>>> } else {
>>> - ctx->vpu_dst_fmt = raw_vpu_fmt;
>>> raw_fmt = &ctx->dst_fmt;
>>> encoded_fmt = &ctx->src_fmt;
>>> }
>>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 1/6] media: verisilicon: Do not set context src/dst formats in reset functions
2023-05-02 6:56 ` Benjamin Gaignard
@ 2023-05-02 11:12 ` Linux regression tracking (Thorsten Leemhuis)
2023-05-02 14:02 ` Shreeya Patel
0 siblings, 1 reply; 21+ messages in thread
From: Linux regression tracking (Thorsten Leemhuis) @ 2023-05-02 11:12 UTC (permalink / raw)
To: Benjamin Gaignard, Shreeya Patel
Cc: linux-media, linux-rockchip, linux-kernel, linux-arm-kernel,
kernel, robert.mader, nicolas.dufresne, ezequiel, festevam,
p.zabel, mchehab, shawnguo, s.hauer, kernel, hverkuil-cisco,
linux-imx, regressions
On 02.05.23 08:56, Benjamin Gaignard wrote:
> Le 01/05/2023 à 09:21, Thorsten Leemhuis a écrit :
>> On 27.04.23 00:19, Shreeya Patel wrote:
>>> On 20/02/23 16:18, Benjamin Gaignard wrote:
>>>> Setting context source and destination formats should only be done
>>>> in hantro_set_fmt_out() and hantro_set_fmt_cap() after check that
>>>> the targeted queue is not busy.
>>>> Remove these calls from hantro_reset_encoded_fmt() and
>>>> hantro_reset_raw_fmt() to clean the driver.
>>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>> KernelCI found this patch causes a regression in the
>>> baseline.dmesg.alert test [1] on rk3399-rock-pi-4b [2],
>>> see the bisection report for more details [3].
>>>
>>> Let us know if you have any questions.
>>>
>>> [1]
>>> https://github.com/kernelci/kernelci-core/blob/main/config/rootfs/debos/overlays/baseline/opt/kernelci/dmesg.sh
>>> [2] https://linux.kernelci.org/test/case/id/6442e825f19134d74c2e865d/
>>> [3] https://groups.io/g/kernelci-results/message/40740
>> Thx for the report. FWIW, regzbot noticed there is a patch that refers
>> to the culprit that might have been landed in next after your test ran
>> for the last time (and meanwhile it was mainlined): f100ce3bbd6 ("media:
>> verisilicon: Fix crash when probing encoder")
>
> Yes that patch should fix the probing issue.
> Marek is working on an additional one to fix pixel format negotiation
> but that doesn't impact the boot.
Great, thx for the reply.
Shreeya, normally I believe developers in cases like this and would have
included
#regzbot fix: f100ce3bbd6
in this mail (without the space in front of the #) to mark the
regression as resolved. Would that be okay for you and other kernel.ci
people? Or do you want to confirm this first?
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 1/6] media: verisilicon: Do not set context src/dst formats in reset functions
2023-05-02 11:12 ` Linux regression tracking (Thorsten Leemhuis)
@ 2023-05-02 14:02 ` Shreeya Patel
0 siblings, 0 replies; 21+ messages in thread
From: Shreeya Patel @ 2023-05-02 14:02 UTC (permalink / raw)
To: Linux regressions mailing list, Benjamin Gaignard
Cc: linux-media, linux-rockchip, linux-kernel, linux-arm-kernel,
kernel, robert.mader, nicolas.dufresne, ezequiel, festevam,
p.zabel, mchehab, shawnguo, s.hauer, kernel, hverkuil-cisco,
linux-imx, kernelci, gustavo.padovan, ricardo.canuelo,
Guillaume Charles Tucker, denys.f
Hi Thorsten,
On 02/05/23 16:42, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 02.05.23 08:56, Benjamin Gaignard wrote:
>> Le 01/05/2023 à 09:21, Thorsten Leemhuis a écrit :
>>> On 27.04.23 00:19, Shreeya Patel wrote:
>>>> On 20/02/23 16:18, Benjamin Gaignard wrote:
>>>>> Setting context source and destination formats should only be done
>>>>> in hantro_set_fmt_out() and hantro_set_fmt_cap() after check that
>>>>> the targeted queue is not busy.
>>>>> Remove these calls from hantro_reset_encoded_fmt() and
>>>>> hantro_reset_raw_fmt() to clean the driver.
>>>>>
>>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>> KernelCI found this patch causes a regression in the
>>>> baseline.dmesg.alert test [1] on rk3399-rock-pi-4b [2],
>>>> see the bisection report for more details [3].
>>>>
>>>> Let us know if you have any questions.
>>>>
>>>> [1]
>>>> https://github.com/kernelci/kernelci-core/blob/main/config/rootfs/debos/overlays/baseline/opt/kernelci/dmesg.sh
>>>> [2] https://linux.kernelci.org/test/case/id/6442e825f19134d74c2e865d/
>>>> [3] https://groups.io/g/kernelci-results/message/40740
>>> Thx for the report. FWIW, regzbot noticed there is a patch that refers
>>> to the culprit that might have been landed in next after your test ran
>>> for the last time (and meanwhile it was mainlined): f100ce3bbd6 ("media:
>>> verisilicon: Fix crash when probing encoder")
>> Yes that patch should fix the probing issue.
>> Marek is working on an additional one to fix pixel format negotiation
>> but that doesn't impact the boot.
> Great, thx for the reply.
>
> Shreeya, normally I believe developers in cases like this and would have
> included
>
> #regzbot fix: f100ce3bbd6
>
> in this mail (without the space in front of the #) to mark the
> regression as resolved. Would that be okay for you and other kernel.ci
> people? Or do you want to confirm this first?
I checked the commit pointed out and also double checked with Benjamin
internally so
we are good to mark this as fixed.
#regzbot fix: f100ce3bbd6a
Thanks,
Shreeya Patel
>
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v9 2/6] media: verisilicon: Do not use ctx fields as format storage when resetting
2023-02-20 10:48 [PATCH v9 0/6] media: verisilicon: HEVC: fix 10bits handling Benjamin Gaignard
2023-02-20 10:48 ` [PATCH v9 1/6] media: verisilicon: Do not set context src/dst formats in reset functions Benjamin Gaignard
@ 2023-02-20 10:48 ` Benjamin Gaignard
2023-02-26 12:58 ` Ezequiel Garcia
2023-02-20 10:48 ` [PATCH v9 3/6] media: verisilicon: Do not set ctx->bit_depth in hantro_try_ctrl() Benjamin Gaignard
` (3 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Benjamin Gaignard @ 2023-02-20 10:48 UTC (permalink / raw)
To: ezequiel, p.zabel, mchehab, shawnguo, s.hauer, kernel, festevam,
linux-imx, hverkuil-cisco, nicolas.dufresne, robert.mader
Cc: linux-media, linux-rockchip, linux-kernel, linux-arm-kernel,
kernel, Benjamin Gaignard
Source and destination pixel formats fields of context structure should
not be used as storage when resetting the format.
Use local variables instead and let hantro_set_fmt_out() and
hantro_set_fmt_cap() set them correctly later.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
.../media/platform/verisilicon/hantro_v4l2.c | 40 +++++++++----------
1 file changed, 18 insertions(+), 22 deletions(-)
diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
index d8aa42bd4cd4..d94c99f875c8 100644
--- a/drivers/media/platform/verisilicon/hantro_v4l2.c
+++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
@@ -378,47 +378,43 @@ static void
hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
{
const struct hantro_fmt *vpu_fmt;
- struct v4l2_pix_format_mplane *fmt;
+ struct v4l2_pix_format_mplane fmt;
vpu_fmt = hantro_get_default_fmt(ctx, true);
+ if (!vpu_fmt)
+ return;
+ hantro_reset_fmt(&fmt, vpu_fmt);
+ fmt.width = vpu_fmt->frmsize.min_width;
+ fmt.height = vpu_fmt->frmsize.min_height;
if (ctx->is_encoder)
- fmt = &ctx->dst_fmt;
- else
- fmt = &ctx->src_fmt;
-
- hantro_reset_fmt(fmt, vpu_fmt);
- fmt->width = vpu_fmt->frmsize.min_width;
- fmt->height = vpu_fmt->frmsize.min_height;
- if (ctx->is_encoder)
- hantro_set_fmt_cap(ctx, fmt);
+ hantro_set_fmt_cap(ctx, &fmt);
else
- hantro_set_fmt_out(ctx, fmt);
+ hantro_set_fmt_out(ctx, &fmt);
}
static void
hantro_reset_raw_fmt(struct hantro_ctx *ctx)
{
const struct hantro_fmt *raw_vpu_fmt;
- struct v4l2_pix_format_mplane *raw_fmt, *encoded_fmt;
+ struct v4l2_pix_format_mplane raw_fmt, *encoded_fmt;
raw_vpu_fmt = hantro_get_default_fmt(ctx, false);
+ if (!raw_vpu_fmt)
+ return;
- if (ctx->is_encoder) {
- raw_fmt = &ctx->src_fmt;
+ if (ctx->is_encoder)
encoded_fmt = &ctx->dst_fmt;
- } else {
- raw_fmt = &ctx->dst_fmt;
+ else
encoded_fmt = &ctx->src_fmt;
- }
- hantro_reset_fmt(raw_fmt, raw_vpu_fmt);
- raw_fmt->width = encoded_fmt->width;
- raw_fmt->height = encoded_fmt->height;
+ hantro_reset_fmt(&raw_fmt, raw_vpu_fmt);
+ raw_fmt.width = encoded_fmt->width;
+ raw_fmt.height = encoded_fmt->height;
if (ctx->is_encoder)
- hantro_set_fmt_out(ctx, raw_fmt);
+ hantro_set_fmt_out(ctx, &raw_fmt);
else
- hantro_set_fmt_cap(ctx, raw_fmt);
+ hantro_set_fmt_cap(ctx, &raw_fmt);
}
void hantro_reset_fmts(struct hantro_ctx *ctx)
--
2.34.1
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 2/6] media: verisilicon: Do not use ctx fields as format storage when resetting
2023-02-20 10:48 ` [PATCH v9 2/6] media: verisilicon: Do not use ctx fields as format storage when resetting Benjamin Gaignard
@ 2023-02-26 12:58 ` Ezequiel Garcia
0 siblings, 0 replies; 21+ messages in thread
From: Ezequiel Garcia @ 2023-02-26 12:58 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: p.zabel, mchehab, shawnguo, s.hauer, kernel, festevam, linux-imx,
hverkuil-cisco, nicolas.dufresne, robert.mader, linux-media,
linux-rockchip, linux-kernel, linux-arm-kernel, kernel
On Mon, Feb 20 2023 at 11:48:45 AM +0100, Benjamin Gaignard
<benjamin.gaignard@collabora.com> wrote:
> Source and destination pixel formats fields of context structure
> should
> not be used as storage when resetting the format.
> Use local variables instead and let hantro_set_fmt_out() and
> hantro_set_fmt_cap() set them correctly later.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
> .../media/platform/verisilicon/hantro_v4l2.c | 40
> +++++++++----------
> 1 file changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c
> b/drivers/media/platform/verisilicon/hantro_v4l2.c
> index d8aa42bd4cd4..d94c99f875c8 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> @@ -378,47 +378,43 @@ static void
> hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
> {
> const struct hantro_fmt *vpu_fmt;
> - struct v4l2_pix_format_mplane *fmt;
> + struct v4l2_pix_format_mplane fmt;
>
> vpu_fmt = hantro_get_default_fmt(ctx, true);
> + if (!vpu_fmt)
> + return;
>
> + hantro_reset_fmt(&fmt, vpu_fmt);
> + fmt.width = vpu_fmt->frmsize.min_width;
> + fmt.height = vpu_fmt->frmsize.min_height;
> if (ctx->is_encoder)
> - fmt = &ctx->dst_fmt;
> - else
> - fmt = &ctx->src_fmt;
> -
> - hantro_reset_fmt(fmt, vpu_fmt);
> - fmt->width = vpu_fmt->frmsize.min_width;
> - fmt->height = vpu_fmt->frmsize.min_height;
> - if (ctx->is_encoder)
> - hantro_set_fmt_cap(ctx, fmt);
> + hantro_set_fmt_cap(ctx, &fmt);
> else
> - hantro_set_fmt_out(ctx, fmt);
> + hantro_set_fmt_out(ctx, &fmt);
> }
>
> static void
> hantro_reset_raw_fmt(struct hantro_ctx *ctx)
> {
> const struct hantro_fmt *raw_vpu_fmt;
> - struct v4l2_pix_format_mplane *raw_fmt, *encoded_fmt;
> + struct v4l2_pix_format_mplane raw_fmt, *encoded_fmt;
>
> raw_vpu_fmt = hantro_get_default_fmt(ctx, false);
> + if (!raw_vpu_fmt)
> + return;
>
> - if (ctx->is_encoder) {
> - raw_fmt = &ctx->src_fmt;
> + if (ctx->is_encoder)
> encoded_fmt = &ctx->dst_fmt;
> - } else {
> - raw_fmt = &ctx->dst_fmt;
> + else
> encoded_fmt = &ctx->src_fmt;
> - }
>
> - hantro_reset_fmt(raw_fmt, raw_vpu_fmt);
> - raw_fmt->width = encoded_fmt->width;
> - raw_fmt->height = encoded_fmt->height;
> + hantro_reset_fmt(&raw_fmt, raw_vpu_fmt);
> + raw_fmt.width = encoded_fmt->width;
> + raw_fmt.height = encoded_fmt->height;
> if (ctx->is_encoder)
> - hantro_set_fmt_out(ctx, raw_fmt);
> + hantro_set_fmt_out(ctx, &raw_fmt);
> else
> - hantro_set_fmt_cap(ctx, raw_fmt);
> + hantro_set_fmt_cap(ctx, &raw_fmt);
> }
>
> void hantro_reset_fmts(struct hantro_ctx *ctx)
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v9 3/6] media: verisilicon: Do not set ctx->bit_depth in hantro_try_ctrl()
2023-02-20 10:48 [PATCH v9 0/6] media: verisilicon: HEVC: fix 10bits handling Benjamin Gaignard
2023-02-20 10:48 ` [PATCH v9 1/6] media: verisilicon: Do not set context src/dst formats in reset functions Benjamin Gaignard
2023-02-20 10:48 ` [PATCH v9 2/6] media: verisilicon: Do not use ctx fields as format storage when resetting Benjamin Gaignard
@ 2023-02-20 10:48 ` Benjamin Gaignard
2023-02-26 12:59 ` Ezequiel Garcia
2023-02-20 10:48 ` [PATCH v9 4/6] media: verisilicon: Do not change context bit depth before validating the format Benjamin Gaignard
` (2 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Benjamin Gaignard @ 2023-02-20 10:48 UTC (permalink / raw)
To: ezequiel, p.zabel, mchehab, shawnguo, s.hauer, kernel, festevam,
linux-imx, hverkuil-cisco, nicolas.dufresne, robert.mader
Cc: linux-media, linux-rockchip, linux-kernel, linux-arm-kernel,
kernel, Benjamin Gaignard
In hantro_try_ctrl() we should only check the values inside
control parameters and not set ctx->bit_depth. That must
be done in controls set function.
Create a set control function for hevc where ctx->bit_depth is
set at the right time.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
.../media/platform/verisilicon/hantro_drv.c | 32 ++++++++++++++-----
1 file changed, 24 insertions(+), 8 deletions(-)
diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index b0aeedae7b65..c237253803f4 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -251,11 +251,6 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
{
- struct hantro_ctx *ctx;
-
- ctx = container_of(ctrl->handler,
- struct hantro_ctx, ctrl_handler);
-
if (ctrl->id == V4L2_CID_STATELESS_H264_SPS) {
const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
@@ -274,8 +269,6 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
if (sps->bit_depth_luma_minus8 != 0 && sps->bit_depth_luma_minus8 != 2)
/* Only 8-bit and 10-bit are supported */
return -EINVAL;
-
- ctx->bit_depth = sps->bit_depth_luma_minus8 + 8;
} else if (ctrl->id == V4L2_CID_STATELESS_VP9_FRAME) {
const struct v4l2_ctrl_vp9_frame *dec_params = ctrl->p_new.p_vp9_frame;
@@ -324,6 +317,24 @@ static int hantro_vp9_s_ctrl(struct v4l2_ctrl *ctrl)
return 0;
}
+static int hantro_hevc_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct hantro_ctx *ctx;
+
+ ctx = container_of(ctrl->handler,
+ struct hantro_ctx, ctrl_handler);
+
+ switch (ctrl->id) {
+ case V4L2_CID_STATELESS_HEVC_SPS:
+ ctx->bit_depth = ctrl->p_new.p_hevc_sps->bit_depth_luma_minus8 + 8;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static const struct v4l2_ctrl_ops hantro_ctrl_ops = {
.try_ctrl = hantro_try_ctrl,
};
@@ -336,6 +347,11 @@ static const struct v4l2_ctrl_ops hantro_vp9_ctrl_ops = {
.s_ctrl = hantro_vp9_s_ctrl,
};
+static const struct v4l2_ctrl_ops hantro_hevc_ctrl_ops = {
+ .try_ctrl = hantro_try_ctrl,
+ .s_ctrl = hantro_hevc_s_ctrl,
+};
+
#define HANTRO_JPEG_ACTIVE_MARKERS (V4L2_JPEG_ACTIVE_MARKER_APP0 | \
V4L2_JPEG_ACTIVE_MARKER_COM | \
V4L2_JPEG_ACTIVE_MARKER_DQT | \
@@ -470,7 +486,7 @@ static const struct hantro_ctrl controls[] = {
.codec = HANTRO_HEVC_DECODER,
.cfg = {
.id = V4L2_CID_STATELESS_HEVC_SPS,
- .ops = &hantro_ctrl_ops,
+ .ops = &hantro_hevc_ctrl_ops,
},
}, {
.codec = HANTRO_HEVC_DECODER,
--
2.34.1
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 3/6] media: verisilicon: Do not set ctx->bit_depth in hantro_try_ctrl()
2023-02-20 10:48 ` [PATCH v9 3/6] media: verisilicon: Do not set ctx->bit_depth in hantro_try_ctrl() Benjamin Gaignard
@ 2023-02-26 12:59 ` Ezequiel Garcia
0 siblings, 0 replies; 21+ messages in thread
From: Ezequiel Garcia @ 2023-02-26 12:59 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: p.zabel, mchehab, shawnguo, s.hauer, kernel, festevam, linux-imx,
hverkuil-cisco, nicolas.dufresne, robert.mader, linux-media,
linux-rockchip, linux-kernel, linux-arm-kernel, kernel
On Mon, Feb 20 2023 at 11:48:46 AM +0100, Benjamin Gaignard
<benjamin.gaignard@collabora.com> wrote:
> In hantro_try_ctrl() we should only check the values inside
> control parameters and not set ctx->bit_depth. That must
> be done in controls set function.
> Create a set control function for hevc where ctx->bit_depth is
> set at the right time.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
> .../media/platform/verisilicon/hantro_drv.c | 32
> ++++++++++++++-----
> 1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c
> b/drivers/media/platform/verisilicon/hantro_drv.c
> index b0aeedae7b65..c237253803f4 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -251,11 +251,6 @@ queue_init(void *priv, struct vb2_queue *src_vq,
> struct vb2_queue *dst_vq)
>
> static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
> {
> - struct hantro_ctx *ctx;
> -
> - ctx = container_of(ctrl->handler,
> - struct hantro_ctx, ctrl_handler);
> -
> if (ctrl->id == V4L2_CID_STATELESS_H264_SPS) {
> const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
>
> @@ -274,8 +269,6 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
> if (sps->bit_depth_luma_minus8 != 0 && sps->bit_depth_luma_minus8
> != 2)
> /* Only 8-bit and 10-bit are supported */
> return -EINVAL;
> -
> - ctx->bit_depth = sps->bit_depth_luma_minus8 + 8;
> } else if (ctrl->id == V4L2_CID_STATELESS_VP9_FRAME) {
> const struct v4l2_ctrl_vp9_frame *dec_params =
> ctrl->p_new.p_vp9_frame;
>
> @@ -324,6 +317,24 @@ static int hantro_vp9_s_ctrl(struct v4l2_ctrl
> *ctrl)
> return 0;
> }
>
> +static int hantro_hevc_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct hantro_ctx *ctx;
> +
> + ctx = container_of(ctrl->handler,
> + struct hantro_ctx, ctrl_handler);
> +
> + switch (ctrl->id) {
> + case V4L2_CID_STATELESS_HEVC_SPS:
> + ctx->bit_depth = ctrl->p_new.p_hevc_sps->bit_depth_luma_minus8 + 8;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static const struct v4l2_ctrl_ops hantro_ctrl_ops = {
> .try_ctrl = hantro_try_ctrl,
> };
> @@ -336,6 +347,11 @@ static const struct v4l2_ctrl_ops
> hantro_vp9_ctrl_ops = {
> .s_ctrl = hantro_vp9_s_ctrl,
> };
>
> +static const struct v4l2_ctrl_ops hantro_hevc_ctrl_ops = {
> + .try_ctrl = hantro_try_ctrl,
> + .s_ctrl = hantro_hevc_s_ctrl,
> +};
> +
> #define HANTRO_JPEG_ACTIVE_MARKERS (V4L2_JPEG_ACTIVE_MARKER_APP0 | \
> V4L2_JPEG_ACTIVE_MARKER_COM | \
> V4L2_JPEG_ACTIVE_MARKER_DQT | \
> @@ -470,7 +486,7 @@ static const struct hantro_ctrl controls[] = {
> .codec = HANTRO_HEVC_DECODER,
> .cfg = {
> .id = V4L2_CID_STATELESS_HEVC_SPS,
> - .ops = &hantro_ctrl_ops,
> + .ops = &hantro_hevc_ctrl_ops,
> },
> }, {
> .codec = HANTRO_HEVC_DECODER,
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v9 4/6] media: verisilicon: Do not change context bit depth before validating the format
2023-02-20 10:48 [PATCH v9 0/6] media: verisilicon: HEVC: fix 10bits handling Benjamin Gaignard
` (2 preceding siblings ...)
2023-02-20 10:48 ` [PATCH v9 3/6] media: verisilicon: Do not set ctx->bit_depth in hantro_try_ctrl() Benjamin Gaignard
@ 2023-02-20 10:48 ` Benjamin Gaignard
2023-02-26 12:59 ` Ezequiel Garcia
2023-02-20 10:48 ` [PATCH v9 5/6] media: verisilicon: HEVC: Only propose 10 bits compatible pixels formats Benjamin Gaignard
2023-02-20 10:48 ` [PATCH v9 6/6] media: verisilicon: VP9: " Benjamin Gaignard
5 siblings, 1 reply; 21+ messages in thread
From: Benjamin Gaignard @ 2023-02-20 10:48 UTC (permalink / raw)
To: ezequiel, p.zabel, mchehab, shawnguo, s.hauer, kernel, festevam,
linux-imx, hverkuil-cisco, nicolas.dufresne, robert.mader
Cc: linux-media, linux-rockchip, linux-kernel, linux-arm-kernel,
kernel, Benjamin Gaignard, Nicolas Dufresne
It is needed to check if the proposed pixels format is valid before
updating context bit depth and other internal states.
Stop using ctx->bit_depth to check format depth match and return
result to the caller.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
.../platform/verisilicon/hantro_postproc.c | 2 +-
.../media/platform/verisilicon/hantro_v4l2.c | 51 ++++++++++---------
.../media/platform/verisilicon/hantro_v4l2.h | 3 +-
3 files changed, 30 insertions(+), 26 deletions(-)
diff --git a/drivers/media/platform/verisilicon/hantro_postproc.c b/drivers/media/platform/verisilicon/hantro_postproc.c
index 09d8cf942689..6437423ccf3a 100644
--- a/drivers/media/platform/verisilicon/hantro_postproc.c
+++ b/drivers/media/platform/verisilicon/hantro_postproc.c
@@ -197,7 +197,7 @@ int hantro_postproc_alloc(struct hantro_ctx *ctx)
unsigned int i, buf_size;
/* this should always pick native format */
- fmt = hantro_get_default_fmt(ctx, false);
+ fmt = hantro_get_default_fmt(ctx, false, ctx->bit_depth);
if (!fmt)
return -EINVAL;
v4l2_fill_pixfmt_mp(&pix_mp, fmt->fourcc, ctx->src_fmt.width,
diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
index d94c99f875c8..d238d407f986 100644
--- a/drivers/media/platform/verisilicon/hantro_v4l2.c
+++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
@@ -28,6 +28,8 @@
#include "hantro_hw.h"
#include "hantro_v4l2.h"
+#define HANTRO_DEFAULT_BIT_DEPTH 8
+
static int hantro_set_fmt_out(struct hantro_ctx *ctx,
struct v4l2_pix_format_mplane *pix_mp);
static int hantro_set_fmt_cap(struct hantro_ctx *ctx,
@@ -76,18 +78,13 @@ int hantro_get_format_depth(u32 fourcc)
}
static bool
-hantro_check_depth_match(const struct hantro_ctx *ctx,
- const struct hantro_fmt *fmt)
+hantro_check_depth_match(const struct hantro_fmt *fmt, int bit_depth)
{
- int fmt_depth, ctx_depth = 8;
+ int fmt_depth;
if (!fmt->match_depth && !fmt->postprocessed)
return true;
- /* 0 means default depth, which is 8 */
- if (ctx->bit_depth)
- ctx_depth = ctx->bit_depth;
-
fmt_depth = hantro_get_format_depth(fmt->fourcc);
/*
@@ -95,9 +92,9 @@ hantro_check_depth_match(const struct hantro_ctx *ctx,
* It may be possible to relax that on some HW.
*/
if (!fmt->match_depth)
- return fmt_depth <= ctx_depth;
+ return fmt_depth <= bit_depth;
- return fmt_depth == ctx_depth;
+ return fmt_depth == bit_depth;
}
static const struct hantro_fmt *
@@ -119,7 +116,7 @@ hantro_find_format(const struct hantro_ctx *ctx, u32 fourcc)
}
const struct hantro_fmt *
-hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream)
+hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream, int bit_depth)
{
const struct hantro_fmt *formats;
unsigned int i, num_fmts;
@@ -128,7 +125,7 @@ hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream)
for (i = 0; i < num_fmts; i++) {
if (bitstream == (formats[i].codec_mode !=
HANTRO_MODE_NONE) &&
- hantro_check_depth_match(ctx, &formats[i]))
+ hantro_check_depth_match(&formats[i], bit_depth))
return &formats[i];
}
return NULL;
@@ -204,7 +201,7 @@ static int vidioc_enum_fmt(struct file *file, void *priv,
if (skip_mode_none == mode_none)
continue;
- if (!hantro_check_depth_match(ctx, fmt))
+ if (!hantro_check_depth_match(fmt, ctx->bit_depth))
continue;
if (j == f->index) {
f->pixelformat = fmt->fourcc;
@@ -224,7 +221,7 @@ static int vidioc_enum_fmt(struct file *file, void *priv,
for (i = 0; i < num_fmts; i++) {
fmt = &formats[i];
- if (!hantro_check_depth_match(ctx, fmt))
+ if (!hantro_check_depth_match(fmt, ctx->bit_depth))
continue;
if (j == f->index) {
f->pixelformat = fmt->fourcc;
@@ -292,7 +289,7 @@ static int hantro_try_fmt(const struct hantro_ctx *ctx,
fmt = hantro_find_format(ctx, pix_mp->pixelformat);
if (!fmt) {
- fmt = hantro_get_default_fmt(ctx, coded);
+ fmt = hantro_get_default_fmt(ctx, coded, HANTRO_DEFAULT_BIT_DEPTH);
pix_mp->pixelformat = fmt->fourcc;
}
@@ -380,7 +377,7 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
const struct hantro_fmt *vpu_fmt;
struct v4l2_pix_format_mplane fmt;
- vpu_fmt = hantro_get_default_fmt(ctx, true);
+ vpu_fmt = hantro_get_default_fmt(ctx, true, HANTRO_DEFAULT_BIT_DEPTH);
if (!vpu_fmt)
return;
@@ -393,15 +390,16 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
hantro_set_fmt_out(ctx, &fmt);
}
-static void
-hantro_reset_raw_fmt(struct hantro_ctx *ctx)
+int
+hantro_reset_raw_fmt(struct hantro_ctx *ctx, int bit_depth)
{
const struct hantro_fmt *raw_vpu_fmt;
struct v4l2_pix_format_mplane raw_fmt, *encoded_fmt;
+ int ret;
- raw_vpu_fmt = hantro_get_default_fmt(ctx, false);
+ raw_vpu_fmt = hantro_get_default_fmt(ctx, false, bit_depth);
if (!raw_vpu_fmt)
- return;
+ return -EINVAL;
if (ctx->is_encoder)
encoded_fmt = &ctx->dst_fmt;
@@ -412,15 +410,20 @@ hantro_reset_raw_fmt(struct hantro_ctx *ctx)
raw_fmt.width = encoded_fmt->width;
raw_fmt.height = encoded_fmt->height;
if (ctx->is_encoder)
- hantro_set_fmt_out(ctx, &raw_fmt);
+ ret = hantro_set_fmt_out(ctx, &raw_fmt);
else
- hantro_set_fmt_cap(ctx, &raw_fmt);
+ ret = hantro_set_fmt_cap(ctx, &raw_fmt);
+
+ if (!ret)
+ ctx->bit_depth = bit_depth;
+
+ return ret;
}
void hantro_reset_fmts(struct hantro_ctx *ctx)
{
hantro_reset_encoded_fmt(ctx);
- hantro_reset_raw_fmt(ctx);
+ hantro_reset_raw_fmt(ctx, HANTRO_DEFAULT_BIT_DEPTH);
}
static void
@@ -520,7 +523,7 @@ static int hantro_set_fmt_out(struct hantro_ctx *ctx,
* changes to the raw format.
*/
if (!ctx->is_encoder)
- hantro_reset_raw_fmt(ctx);
+ hantro_reset_raw_fmt(ctx, hantro_get_format_depth(pix_mp->pixelformat));
/* Colorimetry information are always propagated. */
ctx->dst_fmt.colorspace = pix_mp->colorspace;
@@ -583,7 +586,7 @@ static int hantro_set_fmt_cap(struct hantro_ctx *ctx,
* changes to the raw format.
*/
if (ctx->is_encoder)
- hantro_reset_raw_fmt(ctx);
+ hantro_reset_raw_fmt(ctx, HANTRO_DEFAULT_BIT_DEPTH);
/* Colorimetry information are always propagated. */
ctx->src_fmt.colorspace = pix_mp->colorspace;
diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.h b/drivers/media/platform/verisilicon/hantro_v4l2.h
index 64f6f57e9d7a..9ea2fef57dcd 100644
--- a/drivers/media/platform/verisilicon/hantro_v4l2.h
+++ b/drivers/media/platform/verisilicon/hantro_v4l2.h
@@ -21,9 +21,10 @@
extern const struct v4l2_ioctl_ops hantro_ioctl_ops;
extern const struct vb2_ops hantro_queue_ops;
+int hantro_reset_raw_fmt(struct hantro_ctx *ctx, int bit_depth);
void hantro_reset_fmts(struct hantro_ctx *ctx);
int hantro_get_format_depth(u32 fourcc);
const struct hantro_fmt *
-hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream);
+hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream, int bit_depth);
#endif /* HANTRO_V4L2_H_ */
--
2.34.1
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 4/6] media: verisilicon: Do not change context bit depth before validating the format
2023-02-20 10:48 ` [PATCH v9 4/6] media: verisilicon: Do not change context bit depth before validating the format Benjamin Gaignard
@ 2023-02-26 12:59 ` Ezequiel Garcia
0 siblings, 0 replies; 21+ messages in thread
From: Ezequiel Garcia @ 2023-02-26 12:59 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: p.zabel, mchehab, shawnguo, s.hauer, kernel, festevam, linux-imx,
hverkuil-cisco, nicolas.dufresne, robert.mader, linux-media,
linux-rockchip, linux-kernel, linux-arm-kernel, kernel,
Nicolas Dufresne
On Mon, Feb 20 2023 at 11:48:47 AM +0100, Benjamin Gaignard
<benjamin.gaignard@collabora.com> wrote:
> It is needed to check if the proposed pixels format is valid before
> updating context bit depth and other internal states.
> Stop using ctx->bit_depth to check format depth match and return
> result to the caller.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
> .../platform/verisilicon/hantro_postproc.c | 2 +-
> .../media/platform/verisilicon/hantro_v4l2.c | 51
> ++++++++++---------
> .../media/platform/verisilicon/hantro_v4l2.h | 3 +-
> 3 files changed, 30 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_postproc.c
> b/drivers/media/platform/verisilicon/hantro_postproc.c
> index 09d8cf942689..6437423ccf3a 100644
> --- a/drivers/media/platform/verisilicon/hantro_postproc.c
> +++ b/drivers/media/platform/verisilicon/hantro_postproc.c
> @@ -197,7 +197,7 @@ int hantro_postproc_alloc(struct hantro_ctx *ctx)
> unsigned int i, buf_size;
>
> /* this should always pick native format */
> - fmt = hantro_get_default_fmt(ctx, false);
> + fmt = hantro_get_default_fmt(ctx, false, ctx->bit_depth);
> if (!fmt)
> return -EINVAL;
> v4l2_fill_pixfmt_mp(&pix_mp, fmt->fourcc, ctx->src_fmt.width,
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c
> b/drivers/media/platform/verisilicon/hantro_v4l2.c
> index d94c99f875c8..d238d407f986 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> @@ -28,6 +28,8 @@
> #include "hantro_hw.h"
> #include "hantro_v4l2.h"
>
> +#define HANTRO_DEFAULT_BIT_DEPTH 8
> +
> static int hantro_set_fmt_out(struct hantro_ctx *ctx,
> struct v4l2_pix_format_mplane *pix_mp);
> static int hantro_set_fmt_cap(struct hantro_ctx *ctx,
> @@ -76,18 +78,13 @@ int hantro_get_format_depth(u32 fourcc)
> }
>
> static bool
> -hantro_check_depth_match(const struct hantro_ctx *ctx,
> - const struct hantro_fmt *fmt)
> +hantro_check_depth_match(const struct hantro_fmt *fmt, int bit_depth)
> {
> - int fmt_depth, ctx_depth = 8;
> + int fmt_depth;
>
> if (!fmt->match_depth && !fmt->postprocessed)
> return true;
>
> - /* 0 means default depth, which is 8 */
> - if (ctx->bit_depth)
> - ctx_depth = ctx->bit_depth;
> -
> fmt_depth = hantro_get_format_depth(fmt->fourcc);
>
> /*
> @@ -95,9 +92,9 @@ hantro_check_depth_match(const struct hantro_ctx
> *ctx,
> * It may be possible to relax that on some HW.
> */
> if (!fmt->match_depth)
> - return fmt_depth <= ctx_depth;
> + return fmt_depth <= bit_depth;
>
> - return fmt_depth == ctx_depth;
> + return fmt_depth == bit_depth;
> }
>
> static const struct hantro_fmt *
> @@ -119,7 +116,7 @@ hantro_find_format(const struct hantro_ctx *ctx,
> u32 fourcc)
> }
>
> const struct hantro_fmt *
> -hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream)
> +hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream,
> int bit_depth)
> {
> const struct hantro_fmt *formats;
> unsigned int i, num_fmts;
> @@ -128,7 +125,7 @@ hantro_get_default_fmt(const struct hantro_ctx
> *ctx, bool bitstream)
> for (i = 0; i < num_fmts; i++) {
> if (bitstream == (formats[i].codec_mode !=
> HANTRO_MODE_NONE) &&
> - hantro_check_depth_match(ctx, &formats[i]))
> + hantro_check_depth_match(&formats[i], bit_depth))
> return &formats[i];
> }
> return NULL;
> @@ -204,7 +201,7 @@ static int vidioc_enum_fmt(struct file *file,
> void *priv,
>
> if (skip_mode_none == mode_none)
> continue;
> - if (!hantro_check_depth_match(ctx, fmt))
> + if (!hantro_check_depth_match(fmt, ctx->bit_depth))
> continue;
> if (j == f->index) {
> f->pixelformat = fmt->fourcc;
> @@ -224,7 +221,7 @@ static int vidioc_enum_fmt(struct file *file,
> void *priv,
> for (i = 0; i < num_fmts; i++) {
> fmt = &formats[i];
>
> - if (!hantro_check_depth_match(ctx, fmt))
> + if (!hantro_check_depth_match(fmt, ctx->bit_depth))
> continue;
> if (j == f->index) {
> f->pixelformat = fmt->fourcc;
> @@ -292,7 +289,7 @@ static int hantro_try_fmt(const struct hantro_ctx
> *ctx,
>
> fmt = hantro_find_format(ctx, pix_mp->pixelformat);
> if (!fmt) {
> - fmt = hantro_get_default_fmt(ctx, coded);
> + fmt = hantro_get_default_fmt(ctx, coded, HANTRO_DEFAULT_BIT_DEPTH);
> pix_mp->pixelformat = fmt->fourcc;
> }
>
> @@ -380,7 +377,7 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
> const struct hantro_fmt *vpu_fmt;
> struct v4l2_pix_format_mplane fmt;
>
> - vpu_fmt = hantro_get_default_fmt(ctx, true);
> + vpu_fmt = hantro_get_default_fmt(ctx, true,
> HANTRO_DEFAULT_BIT_DEPTH);
> if (!vpu_fmt)
> return;
>
> @@ -393,15 +390,16 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
> hantro_set_fmt_out(ctx, &fmt);
> }
>
> -static void
> -hantro_reset_raw_fmt(struct hantro_ctx *ctx)
> +int
> +hantro_reset_raw_fmt(struct hantro_ctx *ctx, int bit_depth)
> {
> const struct hantro_fmt *raw_vpu_fmt;
> struct v4l2_pix_format_mplane raw_fmt, *encoded_fmt;
> + int ret;
>
> - raw_vpu_fmt = hantro_get_default_fmt(ctx, false);
> + raw_vpu_fmt = hantro_get_default_fmt(ctx, false, bit_depth);
> if (!raw_vpu_fmt)
> - return;
> + return -EINVAL;
>
> if (ctx->is_encoder)
> encoded_fmt = &ctx->dst_fmt;
> @@ -412,15 +410,20 @@ hantro_reset_raw_fmt(struct hantro_ctx *ctx)
> raw_fmt.width = encoded_fmt->width;
> raw_fmt.height = encoded_fmt->height;
> if (ctx->is_encoder)
> - hantro_set_fmt_out(ctx, &raw_fmt);
> + ret = hantro_set_fmt_out(ctx, &raw_fmt);
> else
> - hantro_set_fmt_cap(ctx, &raw_fmt);
> + ret = hantro_set_fmt_cap(ctx, &raw_fmt);
> +
> + if (!ret)
> + ctx->bit_depth = bit_depth;
> +
> + return ret;
> }
>
> void hantro_reset_fmts(struct hantro_ctx *ctx)
> {
> hantro_reset_encoded_fmt(ctx);
> - hantro_reset_raw_fmt(ctx);
> + hantro_reset_raw_fmt(ctx, HANTRO_DEFAULT_BIT_DEPTH);
> }
>
> static void
> @@ -520,7 +523,7 @@ static int hantro_set_fmt_out(struct hantro_ctx
> *ctx,
> * changes to the raw format.
> */
> if (!ctx->is_encoder)
> - hantro_reset_raw_fmt(ctx);
> + hantro_reset_raw_fmt(ctx,
> hantro_get_format_depth(pix_mp->pixelformat));
>
> /* Colorimetry information are always propagated. */
> ctx->dst_fmt.colorspace = pix_mp->colorspace;
> @@ -583,7 +586,7 @@ static int hantro_set_fmt_cap(struct hantro_ctx
> *ctx,
> * changes to the raw format.
> */
> if (ctx->is_encoder)
> - hantro_reset_raw_fmt(ctx);
> + hantro_reset_raw_fmt(ctx, HANTRO_DEFAULT_BIT_DEPTH);
>
> /* Colorimetry information are always propagated. */
> ctx->src_fmt.colorspace = pix_mp->colorspace;
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.h
> b/drivers/media/platform/verisilicon/hantro_v4l2.h
> index 64f6f57e9d7a..9ea2fef57dcd 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.h
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.h
> @@ -21,9 +21,10 @@
> extern const struct v4l2_ioctl_ops hantro_ioctl_ops;
> extern const struct vb2_ops hantro_queue_ops;
>
> +int hantro_reset_raw_fmt(struct hantro_ctx *ctx, int bit_depth);
> void hantro_reset_fmts(struct hantro_ctx *ctx);
> int hantro_get_format_depth(u32 fourcc);
> const struct hantro_fmt *
> -hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream);
> +hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream,
> int bit_depth);
>
> #endif /* HANTRO_V4L2_H_ */
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v9 5/6] media: verisilicon: HEVC: Only propose 10 bits compatible pixels formats
2023-02-20 10:48 [PATCH v9 0/6] media: verisilicon: HEVC: fix 10bits handling Benjamin Gaignard
` (3 preceding siblings ...)
2023-02-20 10:48 ` [PATCH v9 4/6] media: verisilicon: Do not change context bit depth before validating the format Benjamin Gaignard
@ 2023-02-20 10:48 ` Benjamin Gaignard
2023-02-26 12:59 ` Ezequiel Garcia
2023-02-20 10:48 ` [PATCH v9 6/6] media: verisilicon: VP9: " Benjamin Gaignard
5 siblings, 1 reply; 21+ messages in thread
From: Benjamin Gaignard @ 2023-02-20 10:48 UTC (permalink / raw)
To: ezequiel, p.zabel, mchehab, shawnguo, s.hauer, kernel, festevam,
linux-imx, hverkuil-cisco, nicolas.dufresne, robert.mader
Cc: linux-media, linux-rockchip, linux-kernel, linux-arm-kernel,
kernel, Benjamin Gaignard, Nicolas Dufresne
When decoding a 10bits bitstreams HEVC driver should only expose
10bits pixel formats.
To fulfill this requirement it is needed to call hantro_reset_raw_fmt()
when bit depth change and to correctly set match_depth in pixel formats
enumeration.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
version 9:
- Fix brackets
drivers/media/platform/verisilicon/hantro_drv.c | 12 +++++++++---
drivers/media/platform/verisilicon/imx8m_vpu_hw.c | 2 ++
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index c237253803f4..7d452f1afaae 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -325,9 +325,15 @@ static int hantro_hevc_s_ctrl(struct v4l2_ctrl *ctrl)
struct hantro_ctx, ctrl_handler);
switch (ctrl->id) {
- case V4L2_CID_STATELESS_HEVC_SPS:
- ctx->bit_depth = ctrl->p_new.p_hevc_sps->bit_depth_luma_minus8 + 8;
- break;
+ case V4L2_CID_STATELESS_HEVC_SPS: {
+ const struct v4l2_ctrl_hevc_sps *sps = ctrl->p_new.p_hevc_sps;
+ int bit_depth = sps->bit_depth_luma_minus8 + 8;
+
+ if (ctx->bit_depth == bit_depth)
+ return 0;
+
+ return hantro_reset_raw_fmt(ctx, bit_depth);
+ }
default:
return -EINVAL;
}
diff --git a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
index b390228fd3b4..f850d8bddef6 100644
--- a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
+++ b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
@@ -152,6 +152,7 @@ static const struct hantro_fmt imx8m_vpu_g2_postproc_fmts[] = {
{
.fourcc = V4L2_PIX_FMT_NV12,
.codec_mode = HANTRO_MODE_NONE,
+ .match_depth = true,
.postprocessed = true,
.frmsize = {
.min_width = FMT_MIN_WIDTH,
@@ -165,6 +166,7 @@ static const struct hantro_fmt imx8m_vpu_g2_postproc_fmts[] = {
{
.fourcc = V4L2_PIX_FMT_P010,
.codec_mode = HANTRO_MODE_NONE,
+ .match_depth = true,
.postprocessed = true,
.frmsize = {
.min_width = FMT_MIN_WIDTH,
--
2.34.1
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 5/6] media: verisilicon: HEVC: Only propose 10 bits compatible pixels formats
2023-02-20 10:48 ` [PATCH v9 5/6] media: verisilicon: HEVC: Only propose 10 bits compatible pixels formats Benjamin Gaignard
@ 2023-02-26 12:59 ` Ezequiel Garcia
0 siblings, 0 replies; 21+ messages in thread
From: Ezequiel Garcia @ 2023-02-26 12:59 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: p.zabel, mchehab, shawnguo, s.hauer, kernel, festevam, linux-imx,
hverkuil-cisco, nicolas.dufresne, robert.mader, linux-media,
linux-rockchip, linux-kernel, linux-arm-kernel, kernel,
Nicolas Dufresne
On Mon, Feb 20 2023 at 11:48:48 AM +0100, Benjamin Gaignard
<benjamin.gaignard@collabora.com> wrote:
> When decoding a 10bits bitstreams HEVC driver should only expose
> 10bits pixel formats.
> To fulfill this requirement it is needed to call
> hantro_reset_raw_fmt()
> when bit depth change and to correctly set match_depth in pixel
> formats
> enumeration.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
> version 9:
> - Fix brackets
>
> drivers/media/platform/verisilicon/hantro_drv.c | 12 +++++++++---
> drivers/media/platform/verisilicon/imx8m_vpu_hw.c | 2 ++
> 2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c
> b/drivers/media/platform/verisilicon/hantro_drv.c
> index c237253803f4..7d452f1afaae 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -325,9 +325,15 @@ static int hantro_hevc_s_ctrl(struct v4l2_ctrl
> *ctrl)
> struct hantro_ctx, ctrl_handler);
>
> switch (ctrl->id) {
> - case V4L2_CID_STATELESS_HEVC_SPS:
> - ctx->bit_depth = ctrl->p_new.p_hevc_sps->bit_depth_luma_minus8 + 8;
> - break;
> + case V4L2_CID_STATELESS_HEVC_SPS: {
> + const struct v4l2_ctrl_hevc_sps *sps = ctrl->p_new.p_hevc_sps;
> + int bit_depth = sps->bit_depth_luma_minus8 + 8;
> +
> + if (ctx->bit_depth == bit_depth)
> + return 0;
> +
> + return hantro_reset_raw_fmt(ctx, bit_depth);
> + }
> default:
> return -EINVAL;
> }
> diff --git a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
> b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
> index b390228fd3b4..f850d8bddef6 100644
> --- a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
> +++ b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
> @@ -152,6 +152,7 @@ static const struct hantro_fmt
> imx8m_vpu_g2_postproc_fmts[] = {
> {
> .fourcc = V4L2_PIX_FMT_NV12,
> .codec_mode = HANTRO_MODE_NONE,
> + .match_depth = true,
> .postprocessed = true,
> .frmsize = {
> .min_width = FMT_MIN_WIDTH,
> @@ -165,6 +166,7 @@ static const struct hantro_fmt
> imx8m_vpu_g2_postproc_fmts[] = {
> {
> .fourcc = V4L2_PIX_FMT_P010,
> .codec_mode = HANTRO_MODE_NONE,
> + .match_depth = true,
> .postprocessed = true,
> .frmsize = {
> .min_width = FMT_MIN_WIDTH,
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v9 6/6] media: verisilicon: VP9: Only propose 10 bits compatible pixels formats
2023-02-20 10:48 [PATCH v9 0/6] media: verisilicon: HEVC: fix 10bits handling Benjamin Gaignard
` (4 preceding siblings ...)
2023-02-20 10:48 ` [PATCH v9 5/6] media: verisilicon: HEVC: Only propose 10 bits compatible pixels formats Benjamin Gaignard
@ 2023-02-20 10:48 ` Benjamin Gaignard
2023-02-26 13:00 ` Ezequiel Garcia
5 siblings, 1 reply; 21+ messages in thread
From: Benjamin Gaignard @ 2023-02-20 10:48 UTC (permalink / raw)
To: ezequiel, p.zabel, mchehab, shawnguo, s.hauer, kernel, festevam,
linux-imx, hverkuil-cisco, nicolas.dufresne, robert.mader
Cc: linux-media, linux-rockchip, linux-kernel, linux-arm-kernel,
kernel, Benjamin Gaignard
When decoding a 10bits bitstreams VP9 driver should only expose
10bits pixel formats.
To fulfill this requirement it is needed to call hantro_reset_raw_fmt()
when bit depth change and to correctly set match_depth in pixel formats
enumeration.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
version 9:
- Fix brackets
drivers/media/platform/verisilicon/hantro_drv.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index 7d452f1afaae..d20e62c025ae 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -307,9 +307,14 @@ static int hantro_vp9_s_ctrl(struct v4l2_ctrl *ctrl)
struct hantro_ctx, ctrl_handler);
switch (ctrl->id) {
- case V4L2_CID_STATELESS_VP9_FRAME:
- ctx->bit_depth = ctrl->p_new.p_vp9_frame->bit_depth;
- break;
+ case V4L2_CID_STATELESS_VP9_FRAME: {
+ int bit_depth = ctrl->p_new.p_vp9_frame->bit_depth;
+
+ if (ctx->bit_depth == bit_depth)
+ return 0;
+
+ return hantro_reset_raw_fmt(ctx, bit_depth);
+ }
default:
return -EINVAL;
}
--
2.34.1
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v9 6/6] media: verisilicon: VP9: Only propose 10 bits compatible pixels formats
2023-02-20 10:48 ` [PATCH v9 6/6] media: verisilicon: VP9: " Benjamin Gaignard
@ 2023-02-26 13:00 ` Ezequiel Garcia
0 siblings, 0 replies; 21+ messages in thread
From: Ezequiel Garcia @ 2023-02-26 13:00 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: p.zabel, mchehab, shawnguo, s.hauer, kernel, festevam, linux-imx,
hverkuil-cisco, nicolas.dufresne, robert.mader, linux-media,
linux-rockchip, linux-kernel, linux-arm-kernel, kernel
On Mon, Feb 20 2023 at 11:48:49 AM +0100, Benjamin Gaignard
<benjamin.gaignard@collabora.com> wrote:
> When decoding a 10bits bitstreams VP9 driver should only expose
> 10bits pixel formats.
> To fulfill this requirement it is needed to call
> hantro_reset_raw_fmt()
> when bit depth change and to correctly set match_depth in pixel
> formats
> enumeration.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
> version 9:
> - Fix brackets
>
> drivers/media/platform/verisilicon/hantro_drv.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c
> b/drivers/media/platform/verisilicon/hantro_drv.c
> index 7d452f1afaae..d20e62c025ae 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -307,9 +307,14 @@ static int hantro_vp9_s_ctrl(struct v4l2_ctrl
> *ctrl)
> struct hantro_ctx, ctrl_handler);
>
> switch (ctrl->id) {
> - case V4L2_CID_STATELESS_VP9_FRAME:
> - ctx->bit_depth = ctrl->p_new.p_vp9_frame->bit_depth;
> - break;
> + case V4L2_CID_STATELESS_VP9_FRAME: {
> + int bit_depth = ctrl->p_new.p_vp9_frame->bit_depth;
> +
> + if (ctx->bit_depth == bit_depth)
> + return 0;
> +
> + return hantro_reset_raw_fmt(ctx, bit_depth);
> + }
> default:
> return -EINVAL;
> }
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread