| Message ID | 20230214140557.537984-1-benjamin.gaignard@collabora.com |
|---|---|
| Headers | show |
| Series | AV1 stateless decoder for RK3588 | expand |
On Tue, Feb 14, 2023 at 11:06 AM Benjamin Gaignard <benjamin.gaignard@collabora.com> wrote: > > The driver supports 8 and 10 bits bitstreams, make sure to discard > other cases. > It could happens that userland test if V4L2_CID_STATELESS_AV1_SEQUENCE > exists without setting bit_depth field in this case use > HANTRO_DEFAULT_BIT_DEPTH value. > This shouldn't happen. If the bit_depth argument in hantro_check_depth_match() can be set unchecked by userspace, we have done something wrong!! Are you sure that userspace can do a S_CTRL with an invalid bit-depth? The try_or_set_cluster() function seems to always call try_ctrl before s_ctrl. Thanks, Ezequiel > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > version 4: > - This patch is the result of squashing "Save bit depth for AV1 decoder" > and "Check AV1 bitstreams bit depth" of version 3 + adapation to > "[PATCH v8 0/6] media: verisilicon: HEVC: fix 10bits handling" series. > > .../media/platform/verisilicon/hantro_drv.c | 36 +++++++++++++++++++ > .../media/platform/verisilicon/hantro_v4l2.c | 4 +++ > 2 files changed, 40 insertions(+) > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c > index bc1a85456142..666cd46902da 100644 > --- a/drivers/media/platform/verisilicon/hantro_drv.c > +++ b/drivers/media/platform/verisilicon/hantro_drv.c > @@ -275,7 +275,13 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl) > /* We only support profile 0 */ > if (dec_params->profile != 0) > return -EINVAL; > + } else if (ctrl->id == V4L2_CID_STATELESS_AV1_SEQUENCE) { > + const struct v4l2_ctrl_av1_sequence *sequence = ctrl->p_new.p_av1_sequence; > + > + if (sequence->bit_depth != 8 && sequence->bit_depth != 10) > + return -EINVAL; > } > + > return 0; > } > > @@ -348,6 +354,30 @@ static int hantro_hevc_s_ctrl(struct v4l2_ctrl *ctrl) > return 0; > } > > +static int hantro_av1_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_AV1_SEQUENCE: > + { > + int bit_depth = ctrl->p_new.p_av1_sequence->bit_depth; > + > + if (ctx->bit_depth == bit_depth) > + return 0; > + > + return hantro_reset_raw_fmt(ctx, bit_depth); > + } > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > static const struct v4l2_ctrl_ops hantro_ctrl_ops = { > .try_ctrl = hantro_try_ctrl, > }; > @@ -365,6 +395,11 @@ static const struct v4l2_ctrl_ops hantro_hevc_ctrl_ops = { > .s_ctrl = hantro_hevc_s_ctrl, > }; > > +static const struct v4l2_ctrl_ops hantro_av1_ctrl_ops = { > + .try_ctrl = hantro_try_ctrl, > + .s_ctrl = hantro_av1_s_ctrl, > +}; > + > #define HANTRO_JPEG_ACTIVE_MARKERS (V4L2_JPEG_ACTIVE_MARKER_APP0 | \ > V4L2_JPEG_ACTIVE_MARKER_COM | \ > V4L2_JPEG_ACTIVE_MARKER_DQT | \ > @@ -542,6 +577,7 @@ static const struct hantro_ctrl controls[] = { > .codec = HANTRO_AV1_DECODER, > .cfg = { > .id = V4L2_CID_STATELESS_AV1_SEQUENCE, > + .ops = &hantro_av1_ctrl_ops, > }, > }, { > .codec = HANTRO_AV1_DECODER, > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c > index 992c5baa929f..7e74e47c9a89 100644 > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c > @@ -86,6 +86,10 @@ hantro_check_depth_match(const struct hantro_fmt *fmt, int bit_depth) > if (!fmt->match_depth && !fmt->postprocessed) > return true; > > + /* 0 means default depth, which is 8 */ > + if (!bit_depth) > + bit_depth = HANTRO_DEFAULT_BIT_DEPTH; > + > fmt_depth = hantro_get_format_depth(fmt->fourcc); > > /* > -- > 2.34.1 >
Hi Nicolas, Benjamin, On Tue, Feb 14, 2023 at 11:06 AM Benjamin Gaignard <benjamin.gaignard@collabora.com> wrote: > > This series implement AV1 stateless decoder for RK3588 SoC. > The hardware support 8 and 10 bits bitstreams up to 7680x4320. > AV1 feature like film grain or scaling are done by the postprocessor. > The driver can produce NV12_4L4, NV12_10LE40_4L4, NV12 and P010 pixels formats. > Even if Rockchip have named the hardware VPU981 it looks like a VC9000 but > with a different registers mapping. > > It is based on Daniel's "[PATCH v6] media: Add AV1 uAPI" patches [1] and my > own series to fix 10 bits handling in verisilicon driver "[PATCH v8 0/6] > media: verisilicon: HEVC: fix 10bits handling" [2]. > > The full branch can be found here: > https://gitlab.collabora.com/linux/for-upstream/-/commits/rk3588_av1_decoder_v4 > > Fluster score is: 200/239 while testing AV1-TEST-VECTORS with GStreamer-AV1-V4L2SL-Gst1.0. > The failing tests are: > - the 2 tests with 2 spatial layers: few errors in luma/chroma values > - tests with resolution < hardware limit (64x64) > - 10bits film grain test: bad macroblocks while decoding, the same 8bits > test is working fine. > I did some review of the commits that affect the generic Hantro driver, it looks quite clean! I'll send some R-b soon. Thanks, Ezequiel > Changes in v4: > - Squash "Save bit depth for AV1 decoder" and "Check AV1 bitstreams bit > depth" patches. > - Double motion vectors buffer size. > - Fix the various errors reported by Hans. > > Changes in v3: > - Fix arrays loops limites. > - Remove unused field. > - Reset raw pixel formats list when bit depth or film grain feature > values change. > - Enable post-processor P010 support > > Changes in v2: > - Remove useless +1 in sbs computation. > - Describe NV12_10LE40_4L4 pixels format. > - Post-processor could generate P010. > - Fix comments done on v1. > - The last patch make sure that only post-processed formats are used when film > grain feature is enabled. > > Benjamin > > [1] https://patchwork.kernel.org/project/linux-media/patch/20230214124254.13356-1-daniel.almeida@collabora.com/ > [2] https://www.spinics.net/lists/linux-media/msg226954.html > > Benjamin Gaignard (11): > dt-bindings: media: rockchip-vpu: Add rk3588 vpu compatible > media: Add NV12_10LE40_4L4 pixel format > media: verisilicon: Get bit depth for V4L2_PIX_FMT_NV12_10LE40_4L4 > media: verisilicon: Add AV1 decoder mode and controls > media: verisilicon: Check AV1 bitstreams bit depth > media: verisilicon: Compute motion vectors size for AV1 frames > media: verisilicon: Add AV1 entropy helpers > media: verisilicon: Add Rockchip AV1 decoder > media: verisilicon: Add film grain feature to AV1 driver > media: verisilicon: Enable AV1 decoder on rk3588 > media: verisilicon: Conditionally ignore native formats > > Nicolas Dufresne (1): > v4l2-common: Add support for fractional bpp > > .../bindings/media/rockchip-vpu.yaml | 1 + > .../media/v4l/pixfmt-yuv-planar.rst | 4 + > drivers/media/platform/verisilicon/Makefile | 3 + > drivers/media/platform/verisilicon/hantro.h | 8 + > .../media/platform/verisilicon/hantro_drv.c | 68 +- > .../media/platform/verisilicon/hantro_hw.h | 102 + > .../platform/verisilicon/hantro_postproc.c | 9 +- > .../media/platform/verisilicon/hantro_v4l2.c | 67 +- > .../media/platform/verisilicon/hantro_v4l2.h | 5 +- > .../verisilicon/rockchip_av1_entropymode.c | 4424 +++++++++++++++++ > .../verisilicon/rockchip_av1_entropymode.h | 272 + > .../verisilicon/rockchip_av1_filmgrain.c | 401 ++ > .../verisilicon/rockchip_av1_filmgrain.h | 36 + > .../verisilicon/rockchip_vpu981_hw_av1_dec.c | 2234 +++++++++ > .../verisilicon/rockchip_vpu981_regs.h | 477 ++ > .../platform/verisilicon/rockchip_vpu_hw.c | 134 + > drivers/media/v4l2-core/v4l2-common.c | 149 +- > drivers/media/v4l2-core/v4l2-ioctl.c | 1 + > include/media/v4l2-common.h | 2 + > include/uapi/linux/videodev2.h | 1 + > 20 files changed, 8301 insertions(+), 97 deletions(-) > create mode 100644 drivers/media/platform/verisilicon/rockchip_av1_entropymode.c > create mode 100644 drivers/media/platform/verisilicon/rockchip_av1_entropymode.h > create mode 100644 drivers/media/platform/verisilicon/rockchip_av1_filmgrain.c > create mode 100644 drivers/media/platform/verisilicon/rockchip_av1_filmgrain.h > create mode 100644 drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c > create mode 100644 drivers/media/platform/verisilicon/rockchip_vpu981_regs.h > > -- > 2.34.1 >
Le 18/02/2023 à 14:11, Ezequiel Garcia a écrit : > On Tue, Feb 14, 2023 at 11:06 AM Benjamin Gaignard > <benjamin.gaignard@collabora.com> wrote: >> The driver supports 8 and 10 bits bitstreams, make sure to discard >> other cases. >> It could happens that userland test if V4L2_CID_STATELESS_AV1_SEQUENCE >> exists without setting bit_depth field in this case use >> HANTRO_DEFAULT_BIT_DEPTH value. >> > This shouldn't happen. > > If the bit_depth argument in hantro_check_depth_match() > can be set unchecked by userspace, we have done something wrong!! > > Are you sure that userspace can do a S_CTRL with an invalid bit-depth? > The try_or_set_cluster() function seems to always call try_ctrl before s_ctrl. I have dump the stack when AV1 sequence bit depth = 0 in s_ctrl. It is happening when opening the driver, v4l2 setup the ctrls by calling __v4l2_ctrl_handler_setup() this led to call hantro_av1_s_ctrl() with empty structure. For other codecs isn't a problem because bit depth is coded with a minus 8 value (ie: 8 bits = 0) while for AV1 it is the real value (ie: 8 bits = 8). [ 88.478751] Hardware name: Radxa Rock 5A Board (DT) [ 88.479184] Call trace: [ 88.479406] dump_backtrace.part.0+0xdc/0xf0 [ 88.479796] show_stack+0x18/0x30 [ 88.480099] dump_stack_lvl+0x68/0x84 [ 88.480431] dump_stack+0x18/0x34 [ 88.480732] hantro_av1_s_ctrl+0x7c/0xcc [hantro_vpu] [ 88.481211] __v4l2_ctrl_handler_setup+0x120/0x154 [ 88.481643] v4l2_ctrl_handler_setup+0x2c/0x50 [ 88.482043] hantro_open+0x138/0x204 [hantro_vpu] [ 88.482490] v4l2_open+0xa8/0x124 [ 88.482794] chrdev_open+0xc0/0x22c [ 88.483114] do_dentry_open+0x13c/0x490 [ 88.483464] vfs_open+0x2c/0x40 [ 88.483749] path_openat+0x878/0xe50 [ 88.484074] do_filp_open+0x80/0x130 [ 88.484399] do_sys_openat2+0xb4/0x170 [ 88.484736] __arm64_sys_openat+0x60/0xb0 [ 88.485097] invoke_syscall+0x48/0x114 [ 88.485437] el0_svc_common.constprop.0+0x44/0xfc [ 88.485860] do_el0_svc+0x3c/0xc0 [ 88.486163] el0_svc+0x2c/0x84 [ 88.486441] el0t_64_sync_handler+0xbc/0x140 [ 88.486826] el0t_64_sync+0x190/0x194 Regards, Benjamin > > Thanks, > Ezequiel > >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> version 4: >> - This patch is the result of squashing "Save bit depth for AV1 decoder" >> and "Check AV1 bitstreams bit depth" of version 3 + adapation to >> "[PATCH v8 0/6] media: verisilicon: HEVC: fix 10bits handling" series. >> >> .../media/platform/verisilicon/hantro_drv.c | 36 +++++++++++++++++++ >> .../media/platform/verisilicon/hantro_v4l2.c | 4 +++ >> 2 files changed, 40 insertions(+) >> >> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c >> index bc1a85456142..666cd46902da 100644 >> --- a/drivers/media/platform/verisilicon/hantro_drv.c >> +++ b/drivers/media/platform/verisilicon/hantro_drv.c >> @@ -275,7 +275,13 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl) >> /* We only support profile 0 */ >> if (dec_params->profile != 0) >> return -EINVAL; >> + } else if (ctrl->id == V4L2_CID_STATELESS_AV1_SEQUENCE) { >> + const struct v4l2_ctrl_av1_sequence *sequence = ctrl->p_new.p_av1_sequence; >> + >> + if (sequence->bit_depth != 8 && sequence->bit_depth != 10) >> + return -EINVAL; >> } >> + >> return 0; >> } >> >> @@ -348,6 +354,30 @@ static int hantro_hevc_s_ctrl(struct v4l2_ctrl *ctrl) >> return 0; >> } >> >> +static int hantro_av1_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_AV1_SEQUENCE: >> + { >> + int bit_depth = ctrl->p_new.p_av1_sequence->bit_depth; >> + >> + if (ctx->bit_depth == bit_depth) >> + return 0; >> + >> + return hantro_reset_raw_fmt(ctx, bit_depth); >> + } >> + default: >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> static const struct v4l2_ctrl_ops hantro_ctrl_ops = { >> .try_ctrl = hantro_try_ctrl, >> }; >> @@ -365,6 +395,11 @@ static const struct v4l2_ctrl_ops hantro_hevc_ctrl_ops = { >> .s_ctrl = hantro_hevc_s_ctrl, >> }; >> >> +static const struct v4l2_ctrl_ops hantro_av1_ctrl_ops = { >> + .try_ctrl = hantro_try_ctrl, >> + .s_ctrl = hantro_av1_s_ctrl, >> +}; >> + >> #define HANTRO_JPEG_ACTIVE_MARKERS (V4L2_JPEG_ACTIVE_MARKER_APP0 | \ >> V4L2_JPEG_ACTIVE_MARKER_COM | \ >> V4L2_JPEG_ACTIVE_MARKER_DQT | \ >> @@ -542,6 +577,7 @@ static const struct hantro_ctrl controls[] = { >> .codec = HANTRO_AV1_DECODER, >> .cfg = { >> .id = V4L2_CID_STATELESS_AV1_SEQUENCE, >> + .ops = &hantro_av1_ctrl_ops, >> }, >> }, { >> .codec = HANTRO_AV1_DECODER, >> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c >> index 992c5baa929f..7e74e47c9a89 100644 >> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c >> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c >> @@ -86,6 +86,10 @@ hantro_check_depth_match(const struct hantro_fmt *fmt, int bit_depth) >> if (!fmt->match_depth && !fmt->postprocessed) >> return true; >> >> + /* 0 means default depth, which is 8 */ >> + if (!bit_depth) >> + bit_depth = HANTRO_DEFAULT_BIT_DEPTH; >> + >> fmt_depth = hantro_get_format_depth(fmt->fourcc); >> >> /* >> -- >> 2.34.1 >>
Le lundi 20 février 2023 à 17:24 +0100, Benjamin Gaignard a écrit : > Le 18/02/2023 à 14:11, Ezequiel Garcia a écrit : > > On Tue, Feb 14, 2023 at 11:06 AM Benjamin Gaignard > > <benjamin.gaignard@collabora.com> wrote: > > > The driver supports 8 and 10 bits bitstreams, make sure to discard > > > other cases. > > > It could happens that userland test if V4L2_CID_STATELESS_AV1_SEQUENCE > > > exists without setting bit_depth field in this case use > > > HANTRO_DEFAULT_BIT_DEPTH value. > > > > > This shouldn't happen. > > > > If the bit_depth argument in hantro_check_depth_match() > > can be set unchecked by userspace, we have done something wrong!! > > > > Are you sure that userspace can do a S_CTRL with an invalid bit-depth? > > The try_or_set_cluster() function seems to always call try_ctrl before s_ctrl. > > I have dump the stack when AV1 sequence bit depth = 0 in s_ctrl. > It is happening when opening the driver, v4l2 setup the ctrls by calling __v4l2_ctrl_handler_setup() > this led to call hantro_av1_s_ctrl() with empty structure. > > For other codecs isn't a problem because bit depth is coded with a minus 8 value (ie: 8 bits = 0) > while for AV1 it is the real value (ie: 8 bits = 8). Shouldn't this be fixed in v4l2-ctrls-core.c / std_init_compound() ? This is what we do for VP9: case V4L2_CTRL_TYPE_VP9_FRAME: p_vp9_frame = p; p_vp9_frame->profile = 0; p_vp9_frame->bit_depth = 8; p_vp9_frame->flags |= V4L2_VP9_FRAME_FLAG_X_SUBSAMPLING | V4L2_VP9_FRAME_FLAG_Y_SUBSAMPLING; break; > > [ 88.478751] Hardware name: Radxa Rock 5A Board (DT) > [ 88.479184] Call trace: > [ 88.479406] dump_backtrace.part.0+0xdc/0xf0 > [ 88.479796] show_stack+0x18/0x30 > [ 88.480099] dump_stack_lvl+0x68/0x84 > [ 88.480431] dump_stack+0x18/0x34 > [ 88.480732] hantro_av1_s_ctrl+0x7c/0xcc [hantro_vpu] > [ 88.481211] __v4l2_ctrl_handler_setup+0x120/0x154 > [ 88.481643] v4l2_ctrl_handler_setup+0x2c/0x50 > [ 88.482043] hantro_open+0x138/0x204 [hantro_vpu] > [ 88.482490] v4l2_open+0xa8/0x124 > [ 88.482794] chrdev_open+0xc0/0x22c > [ 88.483114] do_dentry_open+0x13c/0x490 > [ 88.483464] vfs_open+0x2c/0x40 > [ 88.483749] path_openat+0x878/0xe50 > [ 88.484074] do_filp_open+0x80/0x130 > [ 88.484399] do_sys_openat2+0xb4/0x170 > [ 88.484736] __arm64_sys_openat+0x60/0xb0 > [ 88.485097] invoke_syscall+0x48/0x114 > [ 88.485437] el0_svc_common.constprop.0+0x44/0xfc > [ 88.485860] do_el0_svc+0x3c/0xc0 > [ 88.486163] el0_svc+0x2c/0x84 > [ 88.486441] el0t_64_sync_handler+0xbc/0x140 > [ 88.486826] el0t_64_sync+0x190/0x194 > > Regards, > Benjamin > > > > > Thanks, > > Ezequiel > > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > > > --- > > > version 4: > > > - This patch is the result of squashing "Save bit depth for AV1 decoder" > > > and "Check AV1 bitstreams bit depth" of version 3 + adapation to > > > "[PATCH v8 0/6] media: verisilicon: HEVC: fix 10bits handling" series. > > > > > > .../media/platform/verisilicon/hantro_drv.c | 36 +++++++++++++++++++ > > > .../media/platform/verisilicon/hantro_v4l2.c | 4 +++ > > > 2 files changed, 40 insertions(+) > > > > > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c > > > index bc1a85456142..666cd46902da 100644 > > > --- a/drivers/media/platform/verisilicon/hantro_drv.c > > > +++ b/drivers/media/platform/verisilicon/hantro_drv.c > > > @@ -275,7 +275,13 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl) > > > /* We only support profile 0 */ > > > if (dec_params->profile != 0) > > > return -EINVAL; > > > + } else if (ctrl->id == V4L2_CID_STATELESS_AV1_SEQUENCE) { > > > + const struct v4l2_ctrl_av1_sequence *sequence = ctrl->p_new.p_av1_sequence; > > > + > > > + if (sequence->bit_depth != 8 && sequence->bit_depth != 10) > > > + return -EINVAL; > > > } > > > + > > > return 0; > > > } > > > > > > @@ -348,6 +354,30 @@ static int hantro_hevc_s_ctrl(struct v4l2_ctrl *ctrl) > > > return 0; > > > } > > > > > > +static int hantro_av1_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_AV1_SEQUENCE: > > > + { > > > + int bit_depth = ctrl->p_new.p_av1_sequence->bit_depth; > > > + > > > + if (ctx->bit_depth == bit_depth) > > > + return 0; > > > + > > > + return hantro_reset_raw_fmt(ctx, bit_depth); > > > + } > > > + default: > > > + return -EINVAL; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > static const struct v4l2_ctrl_ops hantro_ctrl_ops = { > > > .try_ctrl = hantro_try_ctrl, > > > }; > > > @@ -365,6 +395,11 @@ static const struct v4l2_ctrl_ops hantro_hevc_ctrl_ops = { > > > .s_ctrl = hantro_hevc_s_ctrl, > > > }; > > > > > > +static const struct v4l2_ctrl_ops hantro_av1_ctrl_ops = { > > > + .try_ctrl = hantro_try_ctrl, > > > + .s_ctrl = hantro_av1_s_ctrl, > > > +}; > > > + > > > #define HANTRO_JPEG_ACTIVE_MARKERS (V4L2_JPEG_ACTIVE_MARKER_APP0 | \ > > > V4L2_JPEG_ACTIVE_MARKER_COM | \ > > > V4L2_JPEG_ACTIVE_MARKER_DQT | \ > > > @@ -542,6 +577,7 @@ static const struct hantro_ctrl controls[] = { > > > .codec = HANTRO_AV1_DECODER, > > > .cfg = { > > > .id = V4L2_CID_STATELESS_AV1_SEQUENCE, > > > + .ops = &hantro_av1_ctrl_ops, > > > }, > > > }, { > > > .codec = HANTRO_AV1_DECODER, > > > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c > > > index 992c5baa929f..7e74e47c9a89 100644 > > > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c > > > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c > > > @@ -86,6 +86,10 @@ hantro_check_depth_match(const struct hantro_fmt *fmt, int bit_depth) > > > if (!fmt->match_depth && !fmt->postprocessed) > > > return true; > > > > > > + /* 0 means default depth, which is 8 */ > > > + if (!bit_depth) > > > + bit_depth = HANTRO_DEFAULT_BIT_DEPTH; > > > + > > > fmt_depth = hantro_get_format_depth(fmt->fourcc); > > > > > > /* > > > -- > > > 2.34.1 > > >
Le 20/02/2023 à 20:00, Nicolas Dufresne a écrit : > Le lundi 20 février 2023 à 17:24 +0100, Benjamin Gaignard a écrit : >> Le 18/02/2023 à 14:11, Ezequiel Garcia a écrit : >>> On Tue, Feb 14, 2023 at 11:06 AM Benjamin Gaignard >>> <benjamin.gaignard@collabora.com> wrote: >>>> The driver supports 8 and 10 bits bitstreams, make sure to discard >>>> other cases. >>>> It could happens that userland test if V4L2_CID_STATELESS_AV1_SEQUENCE >>>> exists without setting bit_depth field in this case use >>>> HANTRO_DEFAULT_BIT_DEPTH value. >>>> >>> This shouldn't happen. >>> >>> If the bit_depth argument in hantro_check_depth_match() >>> can be set unchecked by userspace, we have done something wrong!! >>> >>> Are you sure that userspace can do a S_CTRL with an invalid bit-depth? >>> The try_or_set_cluster() function seems to always call try_ctrl before s_ctrl. >> I have dump the stack when AV1 sequence bit depth = 0 in s_ctrl. >> It is happening when opening the driver, v4l2 setup the ctrls by calling __v4l2_ctrl_handler_setup() >> this led to call hantro_av1_s_ctrl() with empty structure. >> >> For other codecs isn't a problem because bit depth is coded with a minus 8 value (ie: 8 bits = 0) >> while for AV1 it is the real value (ie: 8 bits = 8). > Shouldn't this be fixed in v4l2-ctrls-core.c / std_init_compound() ? This is > what we do for VP9: > > > case V4L2_CTRL_TYPE_VP9_FRAME: > p_vp9_frame = p; > p_vp9_frame->profile = 0; > p_vp9_frame->bit_depth = 8; > p_vp9_frame->flags |= V4L2_VP9_FRAME_FLAG_X_SUBSAMPLING | > V4L2_VP9_FRAME_FLAG_Y_SUBSAMPLING; > break; That is indeed a better solution. I will add in my series has a fix of Daniel's series about AV1 uAPI. Thanks, Benjamin > > >> [ 88.478751] Hardware name: Radxa Rock 5A Board (DT) >> [ 88.479184] Call trace: >> [ 88.479406] dump_backtrace.part.0+0xdc/0xf0 >> [ 88.479796] show_stack+0x18/0x30 >> [ 88.480099] dump_stack_lvl+0x68/0x84 >> [ 88.480431] dump_stack+0x18/0x34 >> [ 88.480732] hantro_av1_s_ctrl+0x7c/0xcc [hantro_vpu] >> [ 88.481211] __v4l2_ctrl_handler_setup+0x120/0x154 >> [ 88.481643] v4l2_ctrl_handler_setup+0x2c/0x50 >> [ 88.482043] hantro_open+0x138/0x204 [hantro_vpu] >> [ 88.482490] v4l2_open+0xa8/0x124 >> [ 88.482794] chrdev_open+0xc0/0x22c >> [ 88.483114] do_dentry_open+0x13c/0x490 >> [ 88.483464] vfs_open+0x2c/0x40 >> [ 88.483749] path_openat+0x878/0xe50 >> [ 88.484074] do_filp_open+0x80/0x130 >> [ 88.484399] do_sys_openat2+0xb4/0x170 >> [ 88.484736] __arm64_sys_openat+0x60/0xb0 >> [ 88.485097] invoke_syscall+0x48/0x114 >> [ 88.485437] el0_svc_common.constprop.0+0x44/0xfc >> [ 88.485860] do_el0_svc+0x3c/0xc0 >> [ 88.486163] el0_svc+0x2c/0x84 >> [ 88.486441] el0t_64_sync_handler+0xbc/0x140 >> [ 88.486826] el0t_64_sync+0x190/0x194 >> >> Regards, >> Benjamin >> >>> Thanks, >>> Ezequiel >>> >>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >>>> --- >>>> version 4: >>>> - This patch is the result of squashing "Save bit depth for AV1 decoder" >>>> and "Check AV1 bitstreams bit depth" of version 3 + adapation to >>>> "[PATCH v8 0/6] media: verisilicon: HEVC: fix 10bits handling" series. >>>> >>>> .../media/platform/verisilicon/hantro_drv.c | 36 +++++++++++++++++++ >>>> .../media/platform/verisilicon/hantro_v4l2.c | 4 +++ >>>> 2 files changed, 40 insertions(+) >>>> >>>> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c >>>> index bc1a85456142..666cd46902da 100644 >>>> --- a/drivers/media/platform/verisilicon/hantro_drv.c >>>> +++ b/drivers/media/platform/verisilicon/hantro_drv.c >>>> @@ -275,7 +275,13 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl) >>>> /* We only support profile 0 */ >>>> if (dec_params->profile != 0) >>>> return -EINVAL; >>>> + } else if (ctrl->id == V4L2_CID_STATELESS_AV1_SEQUENCE) { >>>> + const struct v4l2_ctrl_av1_sequence *sequence = ctrl->p_new.p_av1_sequence; >>>> + >>>> + if (sequence->bit_depth != 8 && sequence->bit_depth != 10) >>>> + return -EINVAL; >>>> } >>>> + >>>> return 0; >>>> } >>>> >>>> @@ -348,6 +354,30 @@ static int hantro_hevc_s_ctrl(struct v4l2_ctrl *ctrl) >>>> return 0; >>>> } >>>> >>>> +static int hantro_av1_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_AV1_SEQUENCE: >>>> + { >>>> + int bit_depth = ctrl->p_new.p_av1_sequence->bit_depth; >>>> + >>>> + if (ctx->bit_depth == bit_depth) >>>> + return 0; >>>> + >>>> + return hantro_reset_raw_fmt(ctx, bit_depth); >>>> + } >>>> + default: >>>> + return -EINVAL; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static const struct v4l2_ctrl_ops hantro_ctrl_ops = { >>>> .try_ctrl = hantro_try_ctrl, >>>> }; >>>> @@ -365,6 +395,11 @@ static const struct v4l2_ctrl_ops hantro_hevc_ctrl_ops = { >>>> .s_ctrl = hantro_hevc_s_ctrl, >>>> }; >>>> >>>> +static const struct v4l2_ctrl_ops hantro_av1_ctrl_ops = { >>>> + .try_ctrl = hantro_try_ctrl, >>>> + .s_ctrl = hantro_av1_s_ctrl, >>>> +}; >>>> + >>>> #define HANTRO_JPEG_ACTIVE_MARKERS (V4L2_JPEG_ACTIVE_MARKER_APP0 | \ >>>> V4L2_JPEG_ACTIVE_MARKER_COM | \ >>>> V4L2_JPEG_ACTIVE_MARKER_DQT | \ >>>> @@ -542,6 +577,7 @@ static const struct hantro_ctrl controls[] = { >>>> .codec = HANTRO_AV1_DECODER, >>>> .cfg = { >>>> .id = V4L2_CID_STATELESS_AV1_SEQUENCE, >>>> + .ops = &hantro_av1_ctrl_ops, >>>> }, >>>> }, { >>>> .codec = HANTRO_AV1_DECODER, >>>> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c >>>> index 992c5baa929f..7e74e47c9a89 100644 >>>> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c >>>> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c >>>> @@ -86,6 +86,10 @@ hantro_check_depth_match(const struct hantro_fmt *fmt, int bit_depth) >>>> if (!fmt->match_depth && !fmt->postprocessed) >>>> return true; >>>> >>>> + /* 0 means default depth, which is 8 */ >>>> + if (!bit_depth) >>>> + bit_depth = HANTRO_DEFAULT_BIT_DEPTH; >>>> + >>>> fmt_depth = hantro_get_format_depth(fmt->fourcc); >>>> >>>> /* >>>> -- >>>> 2.34.1 >>>>