[PATCH] media: verisilicon: Final fix for the crash when opening the driver

[Michael Tretter]

On Mon, 01 May 2023 14:50:57 -0400, Nicolas Dufresne wrote:
> Le vendredi 21 avril 2023 à 12:47 +0200, Marek Szyprowski a écrit :
> > ctx->vpu_src_fmt is no more initialized before calling hantro_try_fmt()
> > so checking it led to crash the kernel. Simply use the provided 'fmt' as
> > a format for those checks.
> > 
> > This fixes the following issue observed on Odroid-M1 board:
> > 
> >  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000008
> >  Mem abort info:
> >  ...
> >  Modules linked in: crct10dif_ce hantro_vpu snd_soc_simple_card snd_soc_simple_card_utils v4l2_vp9 v4l2_h264 rockchip_saradc v4l2_mem2mem videobuf2_dma_contig videobuf2_memops rtc_rk808 videobuf2_v4l2 industrialio_triggered_buffer rockchip_thermal dwmac_rk stmmac_platform stmmac videodev kfifo_buf display_connector videobuf2_common pcs_xpcs mc rockchipdrm analogix_dp dw_mipi_dsi dw_hdmi drm_display_helper panfrost drm_shmem_helper gpu_sched ip_tables x_tables ipv6
> >  CPU: 3 PID: 176 Comm: v4l_id Not tainted 6.3.0-rc7-next-20230420 #13481
> >  Hardware name: Hardkernel ODROID-M1 (DT)
> >  pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> >  pc : hantro_try_fmt+0xa0/0x278 [hantro_vpu]
> >  lr : hantro_try_fmt+0x94/0x278 [hantro_vpu]
> >  ...
> >  Call trace:
> >   hantro_try_fmt+0xa0/0x278 [hantro_vpu]
> >   hantro_set_fmt_out+0x3c/0x298 [hantro_vpu]
> >   hantro_reset_raw_fmt+0x98/0x128 [hantro_vpu]
> >   hantro_set_fmt_cap+0x240/0x254 [hantro_vpu]
> >   hantro_reset_encoded_fmt+0x94/0xcc [hantro_vpu]
> >   hantro_reset_fmts+0x18/0x38 [hantro_vpu]
> >   hantro_open+0xd4/0x20c [hantro_vpu]
> >   v4l2_open+0x80/0x120 [videodev]
> >   chrdev_open+0xc0/0x22c
> >   do_dentry_open+0x13c/0x48c
> >   vfs_open+0x2c/0x38
> >   path_openat+0x550/0x934
> >   do_filp_open+0x80/0x12c
> >   do_sys_openat2+0xb4/0x168
> >   __arm64_sys_openat+0x64/0xac
> >   invoke_syscall+0x48/0x114
> >   el0_svc_common+0x100/0x120
> >   do_el0_svc+0x3c/0xa8
> >   el0_svc+0x40/0xa8
> >   el0t_64_sync_handler+0xb8/0xbc
> >   el0t_64_sync+0x190/0x194
> >  Code: 97fc8a7f f940aa80 52864a61 72a686c1 (b9400800) 
> >  ---[ end trace 0000000000000000 ]---
> > 
> > Fixes: db6f68b51e5c ("media: verisilicon: Do not set context src/dst formats in reset functions")
> > Signed-off-by: Marek Szyprowski <m.szyprowski at samsung.com>
> > ---
> > It looks that I've missed the fact that the first fix merged as commit
> > f100ce3bbd6a ("media: verisilicon: Fix crash when probing encoder") did
> > not fix all the issues introduced by db6f68b51e5c ("media: verisilicon:
> > Do not set context src/dst formats in reset functions"). I'm really sorry
> > for that.
> > ---
> >  drivers/media/platform/verisilicon/hantro_v4l2.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
> > index 835518534e3b..618ea23a7d49 100644
> > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> > @@ -313,17 +313,17 @@ static int hantro_try_fmt(const struct hantro_ctx *ctx,
> >  		/* Fill remaining fields */
> >  		v4l2_fill_pixfmt_mp(pix_mp, fmt->fourcc, pix_mp->width,
> >  				    pix_mp->height);
> > -		if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE &&
> > +		if (fmt->fourcc == V4L2_PIX_FMT_H264_SLICE &&
> 
> In this context, we have !coded, so fmt->fourcc will never be any of H264, VP9
> or HEVC, so we'llnever allocate space for motion vector. I'm surprise you are
> getting anything working after that. What this code is trying to achieve is to
> pad the size of raw images depending on the selected coded format (opposite
> queue).

The error happens for the encoder. Thus, it is the OUTPUT device that is
!coded. The OUTPUT device also determines vpu_src_format, which will never be
H264, VP9, or HEVC for an encoder.

The encoder works fine with this patch, but I think it breaks the decoder.

> 
> I think the proper fix is to actually fix the default coded format setup. This
> will ensure that vpu_src_fmt is always valid. The same pointer can be used in
> start-streaming it seems.

Setting a default coded format would be a proper fix. However, this previously
was done in the hantro_reset_*_fmt functions, where it has been removed.

Michael

> 
> >  		    !hantro_needs_postproc(ctx, fmt))
> >  			pix_mp->plane_fmt[0].sizeimage +=
> >  				hantro_h264_mv_size(pix_mp->width,
> >  						    pix_mp->height);
> > -		else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME &&
> > +		else if (fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME &&
> >  			 !hantro_needs_postproc(ctx, fmt))
> >  			pix_mp->plane_fmt[0].sizeimage +=
> >  				hantro_vp9_mv_size(pix_mp->width,
> >  						   pix_mp->height);
> > -		else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE &&
> > +		else if (fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE &&
> >  			 !hantro_needs_postproc(ctx, fmt))
> >  			pix_mp->plane_fmt[0].sizeimage +=
> >  				hantro_hevc_mv_size(pix_mp->width,
>