| Message ID | 20230220104849.398203-2-benjamin.gaignard@collabora.com |
|---|---|
| State | Accepted |
| Commit | db6f68b51e5c7b432185c6d872d03901c07136d2 |
| Headers | show |
| Series | media: verisilicon: HEVC: fix 10bits handling | expand |
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 >
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
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
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
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; > }
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; >> } > >
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; >>> } >>
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.
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.
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; }
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(-)