[v3,13/13] media: verisilicon: Conditionnaly ignore native formats
Commit Message
AV1 film grain feature requires to use the postprocessor to produce
valid frames. In such case the driver shouldn't propose native pixels
format but only post-processed pixels format.
Additionally if when setting a control a value could change capture
queue pixels formats it is needed to call hantro_reset_raw_fmt().
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
v3:
- Reset raw pixel formats list when bit depth or film grain feature
values change.
drivers/media/platform/verisilicon/hantro.h | 3 +++
drivers/media/platform/verisilicon/hantro_drv.c | 11 ++++++++++-
.../media/platform/verisilicon/hantro_postproc.c | 4 ++++
drivers/media/platform/verisilicon/hantro_v4l2.c | 16 +++++++++++++++-
drivers/media/platform/verisilicon/hantro_v4l2.h | 1 +
5 files changed, 33 insertions(+), 2 deletions(-)
Comments
Hi Benjamin,
I love your patch! Perhaps something to improve:
[auto build test WARNING on media-tree/master]
[also build test WARNING on rockchip/for-next linus/master v6.2-rc3 next-20230112]
[cannot apply to pza/reset/next pza/imx-drm/next]
[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/dt-bindings-media-rockchip-vpu-Add-rk3588-vpu-compatible/20230112-010155
base: git://linuxtv.org/media_tree.git master
patch link: https://lore.kernel.org/r/20230111165931.753763-14-benjamin.gaignard%40collabora.com
patch subject: [PATCH v3 13/13] media: verisilicon: Conditionnaly ignore native formats
config: hexagon-randconfig-r011-20230110
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 8d9828ef5aa9688500657d36cd2aefbe12bbd162)
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/7d64cd1d88fb823c1b701e83e0f099d9cc725815
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Benjamin-Gaignard/dt-bindings-media-rockchip-vpu-Add-rk3588-vpu-compatible/20230112-010155
git checkout 7d64cd1d88fb823c1b701e83e0f099d9cc725815
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/media/platform/verisilicon/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from drivers/media/platform/verisilicon/hantro_drv.c:23:
In file included from include/media/v4l2-mem2mem.h:16:
In file included from include/media/videobuf2-v4l2.h:16:
In file included from include/media/videobuf2-core.h:18:
In file included from include/linux/dma-buf.h:16:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/media/platform/verisilicon/hantro_drv.c:23:
In file included from include/media/v4l2-mem2mem.h:16:
In file included from include/media/videobuf2-v4l2.h:16:
In file included from include/media/videobuf2-core.h:18:
In file included from include/linux/dma-buf.h:16:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/media/platform/verisilicon/hantro_drv.c:23:
In file included from include/media/v4l2-mem2mem.h:16:
In file included from include/media/videobuf2-v4l2.h:16:
In file included from include/media/videobuf2-core.h:18:
In file included from include/linux/dma-buf.h:16:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
drivers/media/platform/verisilicon/hantro_drv.c:342:3: error: expected expression
int bit_depth = ctrl->p_new.p_av1_sequence->bit_depth;
^
drivers/media/platform/verisilicon/hantro_drv.c:346:26: error: use of undeclared identifier 'bit_depth'
if (ctx->bit_depth != bit_depth)
^
drivers/media/platform/verisilicon/hantro_drv.c:353:25: error: use of undeclared identifier 'bit_depth'
if (ctx->bit_depth != bit_depth || ctx->need_postproc != need_postproc) {
^
drivers/media/platform/verisilicon/hantro_drv.c:354:21: error: use of undeclared identifier 'bit_depth'
ctx->bit_depth = bit_depth;
^
>> drivers/media/platform/verisilicon/hantro_drv.c:343:8: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement]
bool need_postproc = false;
^
drivers/media/platform/verisilicon/hantro_drv.c:1049:46: warning: implicit conversion from 'unsigned long long' to 'unsigned int' changes value from 18446744073709551615 to 4294967295 [-Wconstant-conversion]
vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~~~~
include/linux/dma-mapping.h:76:40: note: expanded from macro 'DMA_BIT_MASK'
#define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
^~~~~
8 warnings and 4 errors generated.
vim +343 drivers/media/platform/verisilicon/hantro_drv.c
332
333 static int hantro_av1_s_ctrl(struct v4l2_ctrl *ctrl)
334 {
335 struct hantro_ctx *ctx;
336
337 ctx = container_of(ctrl->handler,
338 struct hantro_ctx, ctrl_handler);
339
340 switch (ctrl->id) {
341 case V4L2_CID_STATELESS_AV1_SEQUENCE:
342 int bit_depth = ctrl->p_new.p_av1_sequence->bit_depth;
> 343 bool need_postproc = false;
344
345 if (vb2_is_streaming(v4l2_m2m_get_src_vq(ctx->fh.m2m_ctx)))
346 if (ctx->bit_depth != bit_depth)
347 return -EINVAL;
348
349 if (ctrl->p_new.p_av1_sequence->flags
350 & V4L2_AV1_SEQUENCE_FLAG_FILM_GRAIN_PARAMS_PRESENT)
351 need_postproc = true;
352
353 if (ctx->bit_depth != bit_depth || ctx->need_postproc != need_postproc) {
354 ctx->bit_depth = bit_depth;
355 ctx->need_postproc = need_postproc;
356 hantro_reset_raw_fmt(ctx);
357 }
358 break;
359 default:
360 return -EINVAL;
361 }
362
363 return 0;
364 }
365
Typo in Subject line: Conditionnaly -> Conditionally
Regards,
Hans
On 1/11/23 17:59, Benjamin Gaignard wrote:
> AV1 film grain feature requires to use the postprocessor to produce
> valid frames. In such case the driver shouldn't propose native pixels
> format but only post-processed pixels format.
> Additionally if when setting a control a value could change capture
> queue pixels formats it is needed to call hantro_reset_raw_fmt().
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> v3:
> - Reset raw pixel formats list when bit depth or film grain feature
> values change.
>
> drivers/media/platform/verisilicon/hantro.h | 3 +++
> drivers/media/platform/verisilicon/hantro_drv.c | 11 ++++++++++-
> .../media/platform/verisilicon/hantro_postproc.c | 4 ++++
> drivers/media/platform/verisilicon/hantro_v4l2.c | 16 +++++++++++++++-
> drivers/media/platform/verisilicon/hantro_v4l2.h | 1 +
> 5 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro.h b/drivers/media/platform/verisilicon/hantro.h
> index a98cb40a8d3b..7a5357e810fb 100644
> --- a/drivers/media/platform/verisilicon/hantro.h
> +++ b/drivers/media/platform/verisilicon/hantro.h
> @@ -231,6 +231,8 @@ struct hantro_dev {
> * @ctrl_handler: Control handler used to register controls.
> * @jpeg_quality: User-specified JPEG compression quality.
> * @bit_depth: Bit depth of current frame
> + * @need_postproc: Set to true if the bitstream features require to
> + * use the post-processor.
> *
> * @codec_ops: Set of operations related to codec mode.
> * @postproc: Post-processing context.
> @@ -258,6 +260,7 @@ struct hantro_ctx {
> struct v4l2_ctrl_handler ctrl_handler;
> int jpeg_quality;
> int bit_depth;
> + bool need_postproc;
>
> const struct hantro_codec_ops *codec_ops;
> struct hantro_postproc_ctx postproc;
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 4fc6dea16ae6..ef99f0f0fc53 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -340,12 +340,21 @@ static int hantro_av1_s_ctrl(struct v4l2_ctrl *ctrl)
> switch (ctrl->id) {
> case V4L2_CID_STATELESS_AV1_SEQUENCE:
> int bit_depth = ctrl->p_new.p_av1_sequence->bit_depth;
> + bool need_postproc = false;
>
> if (vb2_is_streaming(v4l2_m2m_get_src_vq(ctx->fh.m2m_ctx)))
> if (ctx->bit_depth != bit_depth)
> return -EINVAL;
>
> - ctx->bit_depth = bit_depth;
> + if (ctrl->p_new.p_av1_sequence->flags
> + & V4L2_AV1_SEQUENCE_FLAG_FILM_GRAIN_PARAMS_PRESENT)
> + need_postproc = true;
> +
> + if (ctx->bit_depth != bit_depth || ctx->need_postproc != need_postproc) {
> + ctx->bit_depth = bit_depth;
> + ctx->need_postproc = need_postproc;
> + hantro_reset_raw_fmt(ctx);
> + }
> break;
> default:
> return -EINVAL;
> diff --git a/drivers/media/platform/verisilicon/hantro_postproc.c b/drivers/media/platform/verisilicon/hantro_postproc.c
> index 7dc39519a2ee..293e5612e2ce 100644
> --- a/drivers/media/platform/verisilicon/hantro_postproc.c
> +++ b/drivers/media/platform/verisilicon/hantro_postproc.c
> @@ -57,6 +57,10 @@ bool hantro_needs_postproc(const struct hantro_ctx *ctx,
> {
> if (ctx->is_encoder)
> return false;
> +
> + if (ctx->need_postproc)
> + return true;
> +
> return fmt->postprocessed;
> }
>
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
> index bbe79dbd2cd9..7566fe86f624 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> @@ -38,6 +38,11 @@ hantro_get_formats(const struct hantro_ctx *ctx, unsigned int *num_fmts)
> {
> const struct hantro_fmt *formats;
>
> + if (ctx->need_postproc) {
> + *num_fmts = 0;
> + return NULL;
> + }
> +
> if (ctx->is_encoder) {
> formats = ctx->dev->variant->enc_fmts;
> *num_fmts = ctx->dev->variant->num_enc_fmts;
> @@ -132,6 +137,15 @@ hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream)
> hantro_check_depth_match(ctx, &formats[i]))
> return &formats[i];
> }
> +
> + formats = hantro_get_postproc_formats(ctx, &num_fmts);
> + for (i = 0; i < num_fmts; i++) {
> + if (bitstream == (formats[i].codec_mode !=
> + HANTRO_MODE_NONE) &&
> + hantro_check_depth_match(ctx, &formats[i]))
> + return &formats[i];
> + }
> +
> return NULL;
> }
>
> @@ -404,7 +418,7 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
> hantro_set_fmt_out(ctx, fmt);
> }
>
> -static void
> +void
> hantro_reset_raw_fmt(struct hantro_ctx *ctx)
> {
> const struct hantro_fmt *raw_vpu_fmt;
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.h b/drivers/media/platform/verisilicon/hantro_v4l2.h
> index 64f6f57e9d7a..f642560aed93 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.h
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.h
> @@ -21,6 +21,7 @@
> extern const struct v4l2_ioctl_ops hantro_ioctl_ops;
> extern const struct vb2_ops hantro_queue_ops;
>
> +void hantro_reset_raw_fmt(struct hantro_ctx *ctx);
> void hantro_reset_fmts(struct hantro_ctx *ctx);
> int hantro_get_format_depth(u32 fourcc);
> const struct hantro_fmt *
@@ -231,6 +231,8 @@ struct hantro_dev {
* @ctrl_handler: Control handler used to register controls.
* @jpeg_quality: User-specified JPEG compression quality.
* @bit_depth: Bit depth of current frame
+ * @need_postproc: Set to true if the bitstream features require to
+ * use the post-processor.
*
* @codec_ops: Set of operations related to codec mode.
* @postproc: Post-processing context.
@@ -258,6 +260,7 @@ struct hantro_ctx {
struct v4l2_ctrl_handler ctrl_handler;
int jpeg_quality;
int bit_depth;
+ bool need_postproc;
const struct hantro_codec_ops *codec_ops;
struct hantro_postproc_ctx postproc;
@@ -340,12 +340,21 @@ static int hantro_av1_s_ctrl(struct v4l2_ctrl *ctrl)
switch (ctrl->id) {
case V4L2_CID_STATELESS_AV1_SEQUENCE:
int bit_depth = ctrl->p_new.p_av1_sequence->bit_depth;
+ bool need_postproc = false;
if (vb2_is_streaming(v4l2_m2m_get_src_vq(ctx->fh.m2m_ctx)))
if (ctx->bit_depth != bit_depth)
return -EINVAL;
- ctx->bit_depth = bit_depth;
+ if (ctrl->p_new.p_av1_sequence->flags
+ & V4L2_AV1_SEQUENCE_FLAG_FILM_GRAIN_PARAMS_PRESENT)
+ need_postproc = true;
+
+ if (ctx->bit_depth != bit_depth || ctx->need_postproc != need_postproc) {
+ ctx->bit_depth = bit_depth;
+ ctx->need_postproc = need_postproc;
+ hantro_reset_raw_fmt(ctx);
+ }
break;
default:
return -EINVAL;
@@ -57,6 +57,10 @@ bool hantro_needs_postproc(const struct hantro_ctx *ctx,
{
if (ctx->is_encoder)
return false;
+
+ if (ctx->need_postproc)
+ return true;
+
return fmt->postprocessed;
}
@@ -38,6 +38,11 @@ hantro_get_formats(const struct hantro_ctx *ctx, unsigned int *num_fmts)
{
const struct hantro_fmt *formats;
+ if (ctx->need_postproc) {
+ *num_fmts = 0;
+ return NULL;
+ }
+
if (ctx->is_encoder) {
formats = ctx->dev->variant->enc_fmts;
*num_fmts = ctx->dev->variant->num_enc_fmts;
@@ -132,6 +137,15 @@ hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream)
hantro_check_depth_match(ctx, &formats[i]))
return &formats[i];
}
+
+ formats = hantro_get_postproc_formats(ctx, &num_fmts);
+ for (i = 0; i < num_fmts; i++) {
+ if (bitstream == (formats[i].codec_mode !=
+ HANTRO_MODE_NONE) &&
+ hantro_check_depth_match(ctx, &formats[i]))
+ return &formats[i];
+ }
+
return NULL;
}
@@ -404,7 +418,7 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
hantro_set_fmt_out(ctx, fmt);
}
-static void
+void
hantro_reset_raw_fmt(struct hantro_ctx *ctx)
{
const struct hantro_fmt *raw_vpu_fmt;
@@ -21,6 +21,7 @@
extern const struct v4l2_ioctl_ops hantro_ioctl_ops;
extern const struct vb2_ops hantro_queue_ops;
+void hantro_reset_raw_fmt(struct hantro_ctx *ctx);
void hantro_reset_fmts(struct hantro_ctx *ctx);
int hantro_get_format_depth(u32 fourcc);
const struct hantro_fmt *