Message ID | 20221219155616.848690-4-benjamin.gaignard@collabora.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | AV1 stateless decoder for RK3588 | expand |
Le lundi 19 décembre 2022 à 16:56 +0100, Benjamin Gaignard a écrit : > Store bit depth information from AV1 sequence control. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > .../media/platform/verisilicon/hantro_drv.c | 26 +++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c > index 4500e1fc0f2c..8e93710dcfed 100644 > --- a/drivers/media/platform/verisilicon/hantro_drv.c > +++ b/drivers/media/platform/verisilicon/hantro_drv.c > @@ -324,6 +324,25 @@ static int hantro_vp9_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: > + ctx->bit_depth = ctrl->p_new.p_av1_sequence->bit_depth; That seems a little be weak, what happens if you change the bit_depth with a non-request s_ctrl while its decoding ? To be this deserve a little bit of protection, a something that validate and copy it at the start of the decoding. p.s. I know, VP9 seems similar, though arguably that was copied from jpeg, for which it seems totally save to change the quality at run-time. > + break; > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > + > static const struct v4l2_ctrl_ops hantro_ctrl_ops = { > .try_ctrl = hantro_try_ctrl, > }; > @@ -336,6 +355,12 @@ static const struct v4l2_ctrl_ops hantro_vp9_ctrl_ops = { > .s_ctrl = hantro_vp9_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 | \ > @@ -513,6 +538,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,
Bonjour Nicolas, On Mon, Dec 19, 2022 at 5:37 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote: > > Le lundi 19 décembre 2022 à 16:56 +0100, Benjamin Gaignard a écrit : > > Store bit depth information from AV1 sequence control. > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > > --- > > .../media/platform/verisilicon/hantro_drv.c | 26 +++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c > > index 4500e1fc0f2c..8e93710dcfed 100644 > > --- a/drivers/media/platform/verisilicon/hantro_drv.c > > +++ b/drivers/media/platform/verisilicon/hantro_drv.c > > @@ -324,6 +324,25 @@ static int hantro_vp9_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: > > + ctx->bit_depth = ctrl->p_new.p_av1_sequence->bit_depth; > > That seems a little be weak, what happens if you change the bit_depth with a > non-request s_ctrl while its decoding ? To be this deserve a little bit of > protection, a something that validate and copy it at the start of the decoding. > Oh, nice catch. We need to return EBUSY, see https://www.kernel.org/doc/html/v5.0/media/uapi/v4l/buffer.html#interactions-between-formats-controls-and-buffers. There's already an API in the V4L2 control framework for drivers to use, see v4l2_ctrl_grab in https://www.kernel.org/doc/html/v5.0/media/kapi/v4l2-controls.html#active-and-grabbed-controls. > p.s. I know, VP9 seems similar, though arguably that was copied from jpeg, for > which it seems totally save to change the quality at run-time. > No, wasn't copied from JPEG :-) I just didn't realize this was an issue, but it is given the bit_depth affects the buffers so you are correct, it needs to be fixed for VP9 too. Thanks! Ezequiel > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > + > > static const struct v4l2_ctrl_ops hantro_ctrl_ops = { > > .try_ctrl = hantro_try_ctrl, > > }; > > @@ -336,6 +355,12 @@ static const struct v4l2_ctrl_ops hantro_vp9_ctrl_ops = { > > .s_ctrl = hantro_vp9_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 | \ > > @@ -513,6 +538,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, >
Hi Benjamin, I love your patch! Yet something to improve: [auto build test ERROR on media-tree/master] [also build test ERROR on rockchip/for-next linus/master v6.1 next-20221219] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/AV1-stateless-decoder-for-RK3588/20221220-000013 base: git://linuxtv.org/media_tree.git master patch link: https://lore.kernel.org/r/20221219155616.848690-4-benjamin.gaignard%40collabora.com patch subject: [PATCH v1 3/9] media: verisilicon: Save bit depth for AV1 decoder config: m68k-allmodconfig compiler: m68k-linux-gcc (GCC) 12.1.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/c29cd881c9b7abf03a59462032928b13ff014c22 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Benjamin-Gaignard/AV1-stateless-decoder-for-RK3588/20221220-000013 git checkout c29cd881c9b7abf03a59462032928b13ff014c22 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/media/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): drivers/media/platform/verisilicon/hantro_drv.c: In function 'hantro_av1_s_ctrl': >> drivers/media/platform/verisilicon/hantro_drv.c:335:14: error: 'V4L2_CID_STATELESS_AV1_SEQUENCE' undeclared (first use in this function); did you mean 'V4L2_CID_STATELESS_MPEG2_SEQUENCE'? 335 | case V4L2_CID_STATELESS_AV1_SEQUENCE: | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | V4L2_CID_STATELESS_MPEG2_SEQUENCE drivers/media/platform/verisilicon/hantro_drv.c:335:14: note: each undeclared identifier is reported only once for each function it appears in >> drivers/media/platform/verisilicon/hantro_drv.c:336:46: error: 'union v4l2_ctrl_ptr' has no member named 'p_av1_sequence'; did you mean 'p_mpeg2_sequence'? 336 | ctx->bit_depth = ctrl->p_new.p_av1_sequence->bit_depth; | ^~~~~~~~~~~~~~ | p_mpeg2_sequence drivers/media/platform/verisilicon/hantro_drv.c: At top level: drivers/media/platform/verisilicon/hantro_drv.c:529:31: error: 'V4L2_CID_STATELESS_AV1_FRAME' undeclared here (not in a function); did you mean 'V4L2_CID_STATELESS_VP9_FRAME'? 529 | .id = V4L2_CID_STATELESS_AV1_FRAME, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ | V4L2_CID_STATELESS_VP9_FRAME drivers/media/platform/verisilicon/hantro_drv.c:534:31: error: 'V4L2_CID_STATELESS_AV1_TILE_GROUP_ENTRY' undeclared here (not in a function) 534 | .id = V4L2_CID_STATELESS_AV1_TILE_GROUP_ENTRY, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/media/platform/verisilicon/hantro_drv.c:535:35: error: 'V4L2_AV1_MAX_TILE_COUNT' undeclared here (not in a function) 535 | .dims = { V4L2_AV1_MAX_TILE_COUNT }, | ^~~~~~~~~~~~~~~~~~~~~~~ drivers/media/platform/verisilicon/hantro_drv.c:540:31: error: 'V4L2_CID_STATELESS_AV1_SEQUENCE' undeclared here (not in a function); did you mean 'V4L2_CID_STATELESS_MPEG2_SEQUENCE'? 540 | .id = V4L2_CID_STATELESS_AV1_SEQUENCE, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | V4L2_CID_STATELESS_MPEG2_SEQUENCE drivers/media/platform/verisilicon/hantro_drv.c:546:31: error: 'V4L2_CID_STATELESS_AV1_FILM_GRAIN' undeclared here (not in a function); did you mean 'V4L2_CID_STATELESS_VP9_FRAME'? 546 | .id = V4L2_CID_STATELESS_AV1_FILM_GRAIN, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | V4L2_CID_STATELESS_VP9_FRAME vim +335 drivers/media/platform/verisilicon/hantro_drv.c 326 327 static int hantro_av1_s_ctrl(struct v4l2_ctrl *ctrl) 328 { 329 struct hantro_ctx *ctx; 330 331 ctx = container_of(ctrl->handler, 332 struct hantro_ctx, ctrl_handler); 333 334 switch (ctrl->id) { > 335 case V4L2_CID_STATELESS_AV1_SEQUENCE: > 336 ctx->bit_depth = ctrl->p_new.p_av1_sequence->bit_depth; 337 break; 338 default: 339 return -EINVAL; 340 } 341 342 return 0; 343 } 344
Le 19/12/2022 à 22:29, Ezequiel Garcia a écrit : > Bonjour Nicolas, > > On Mon, Dec 19, 2022 at 5:37 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote: >> Le lundi 19 décembre 2022 à 16:56 +0100, Benjamin Gaignard a écrit : >>> Store bit depth information from AV1 sequence control. >>> >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >>> --- >>> .../media/platform/verisilicon/hantro_drv.c | 26 +++++++++++++++++++ >>> 1 file changed, 26 insertions(+) >>> >>> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c >>> index 4500e1fc0f2c..8e93710dcfed 100644 >>> --- a/drivers/media/platform/verisilicon/hantro_drv.c >>> +++ b/drivers/media/platform/verisilicon/hantro_drv.c >>> @@ -324,6 +324,25 @@ static int hantro_vp9_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: >>> + ctx->bit_depth = ctrl->p_new.p_av1_sequence->bit_depth; >> That seems a little be weak, what happens if you change the bit_depth with a >> non-request s_ctrl while its decoding ? To be this deserve a little bit of >> protection, a something that validate and copy it at the start of the decoding. >> > Oh, nice catch. We need to return EBUSY, see > https://www.kernel.org/doc/html/v5.0/media/uapi/v4l/buffer.html#interactions-between-formats-controls-and-buffers. > > There's already an API in the V4L2 control framework for drivers to use, > see v4l2_ctrl_grab in > https://www.kernel.org/doc/html/v5.0/media/kapi/v4l2-controls.html#active-and-grabbed-controls. > >> p.s. I know, VP9 seems similar, though arguably that was copied from jpeg, for >> which it seems totally save to change the quality at run-time. >> > No, wasn't copied from JPEG :-) I just didn't realize this was an > issue, but it is > given the bit_depth affects the buffers so you are correct, it needs > to be fixed for VP9 too. I will use v4l2_ctrl_grab() in codecs->ops init() and exit() functions but it will be on patch 7 because it where they appear for this codec. Benjamin > > Thanks! > Ezequiel > >>> + break; >>> + default: >>> + return -EINVAL; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> + >>> static const struct v4l2_ctrl_ops hantro_ctrl_ops = { >>> .try_ctrl = hantro_try_ctrl, >>> }; >>> @@ -336,6 +355,12 @@ static const struct v4l2_ctrl_ops hantro_vp9_ctrl_ops = { >>> .s_ctrl = hantro_vp9_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 | \ >>> @@ -513,6 +538,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_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c index 4500e1fc0f2c..8e93710dcfed 100644 --- a/drivers/media/platform/verisilicon/hantro_drv.c +++ b/drivers/media/platform/verisilicon/hantro_drv.c @@ -324,6 +324,25 @@ static int hantro_vp9_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: + ctx->bit_depth = ctrl->p_new.p_av1_sequence->bit_depth; + break; + default: + return -EINVAL; + } + + return 0; +} + + static const struct v4l2_ctrl_ops hantro_ctrl_ops = { .try_ctrl = hantro_try_ctrl, }; @@ -336,6 +355,12 @@ static const struct v4l2_ctrl_ops hantro_vp9_ctrl_ops = { .s_ctrl = hantro_vp9_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 | \ @@ -513,6 +538,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,
Store bit depth information from AV1 sequence control. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> --- .../media/platform/verisilicon/hantro_drv.c | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+)