diff mbox series

[v9,1/6] media: verisilicon: Do not set context src/dst formats in reset functions

Message ID 20230220104849.398203-2-benjamin.gaignard@collabora.com
State New
Headers
Series media: verisilicon: HEVC: fix 10bits handling |

Commit Message

Benjamin Gaignard Feb. 20, 2023, 10:48 a.m. UTC 
  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(-)

Comments

Ezequiel Garcia Feb. 26, 2023, 12:58 p.m. UTC | #1 
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
>
Marek Szyprowski April 12, 2023, 4:14 p.m. UTC | #2 
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
Benjamin Gaignard April 12, 2023, 4:40 p.m. UTC | #3 
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
Marek Szyprowski April 12, 2023, 4:53 p.m. UTC | #4 
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
Shreeya Patel April 26, 2023, 10:19 p.m. UTC | #5 
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;
>   	}
Thorsten Leemhuis May 1, 2023, 7:21 a.m. UTC | #6 
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;
>>       }
> 
>
Benjamin Gaignard May 2, 2023, 6:56 a.m. UTC | #7 
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;
>>>        }
>>
Linux regression tracking (Thorsten Leemhuis) May 2, 2023, 11:12 a.m. UTC | #8 
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.
Shreeya Patel May 2, 2023, 2:02 p.m. UTC | #9 
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 mbox series

Patch

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