When decoding a 10bits bitstreams HEVC driver should only expose
10bits pixel formats.
To fulfill this requirement it is needed to call hantro_reset_raw_fmt()
when bit depth change and to correctly set match_depth in pixel formats
enumeration.
Fixes: dc39473d0340 ("media: hantro: imx8m: Enable 10bit decoding")
Signed-off-by: Benjamin Gaignard <[email protected]>
---
version 3:
- Propagate hantro_reset_raw_fmt() error.
I hope I have correctly understood Ezekiel's thoughts
in the way I have implemented them.
version 2:
- Remove struct hantro_ctx *ctx variable in hantro_try_ctrl()
because it isn't used anymore.
.../media/platform/verisilicon/hantro_drv.c | 40 +++++++++++++++----
.../media/platform/verisilicon/hantro_v4l2.c | 6 +--
.../media/platform/verisilicon/hantro_v4l2.h | 1 +
.../media/platform/verisilicon/imx8m_vpu_hw.c | 2 +
4 files changed, 38 insertions(+), 11 deletions(-)
diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index 8cb4a68c9119..a713a45c0108 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -251,11 +251,6 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
{
- struct hantro_ctx *ctx;
-
- ctx = container_of(ctrl->handler,
- struct hantro_ctx, ctrl_handler);
-
if (ctrl->id == V4L2_CID_STATELESS_H264_SPS) {
const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
@@ -274,8 +269,6 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
if (sps->bit_depth_luma_minus8 != 0 && sps->bit_depth_luma_minus8 != 2)
/* Only 8-bit and 10-bit are supported */
return -EINVAL;
-
- ctx->bit_depth = sps->bit_depth_luma_minus8 + 8;
} else if (ctrl->id == V4L2_CID_STATELESS_VP9_FRAME) {
const struct v4l2_ctrl_vp9_frame *dec_params = ctrl->p_new.p_vp9_frame;
@@ -286,6 +279,32 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
return 0;
}
+static int hantro_hevc_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct hantro_ctx *ctx;
+
+ ctx = container_of(ctrl->handler,
+ struct hantro_ctx, ctrl_handler);
+
+ vpu_debug(1, "s_ctrl: id = %d, val = %d\n", ctrl->id, ctrl->val);
+
+ switch (ctrl->id) {
+ case V4L2_CID_STATELESS_HEVC_SPS:
+ const struct v4l2_ctrl_hevc_sps *sps = ctrl->p_new.p_hevc_sps;
+ int bit_depth = sps->bit_depth_luma_minus8 + 8;
+
+ if (ctx->bit_depth != bit_depth) {
+ ctx->bit_depth = bit_depth;
+ return hantro_reset_raw_fmt(ctx);
+ }
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int hantro_jpeg_s_ctrl(struct v4l2_ctrl *ctrl)
{
struct hantro_ctx *ctx;
@@ -328,6 +347,11 @@ static const struct v4l2_ctrl_ops hantro_ctrl_ops = {
.try_ctrl = hantro_try_ctrl,
};
+static const struct v4l2_ctrl_ops hantro_hevc_ctrl_ops = {
+ .s_ctrl = hantro_hevc_s_ctrl,
+ .try_ctrl = hantro_try_ctrl,
+};
+
static const struct v4l2_ctrl_ops hantro_jpeg_ctrl_ops = {
.s_ctrl = hantro_jpeg_s_ctrl,
};
@@ -470,7 +494,7 @@ static const struct hantro_ctrl controls[] = {
.codec = HANTRO_HEVC_DECODER,
.cfg = {
.id = V4L2_CID_STATELESS_HEVC_SPS,
- .ops = &hantro_ctrl_ops,
+ .ops = &hantro_hevc_ctrl_ops,
},
}, {
.codec = HANTRO_HEVC_DECODER,
diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
index 2c7a805289e7..cd85877bbbe2 100644
--- a/drivers/media/platform/verisilicon/hantro_v4l2.c
+++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
@@ -398,7 +398,7 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
hantro_set_fmt_out(ctx, fmt);
}
-static void
+int
hantro_reset_raw_fmt(struct hantro_ctx *ctx)
{
const struct hantro_fmt *raw_vpu_fmt;
@@ -420,9 +420,9 @@ hantro_reset_raw_fmt(struct hantro_ctx *ctx)
raw_fmt->width = encoded_fmt->width;
raw_fmt->height = encoded_fmt->height;
if (ctx->is_encoder)
- hantro_set_fmt_out(ctx, raw_fmt);
+ return hantro_set_fmt_out(ctx, raw_fmt);
else
- hantro_set_fmt_cap(ctx, raw_fmt);
+ return hantro_set_fmt_cap(ctx, raw_fmt);
}
void hantro_reset_fmts(struct hantro_ctx *ctx)
diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.h b/drivers/media/platform/verisilicon/hantro_v4l2.h
index 64f6f57e9d7a..cb8e1fe3422d 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;
+int 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 *
diff --git a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
index b390228fd3b4..f850d8bddef6 100644
--- a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
+++ b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
@@ -152,6 +152,7 @@ static const struct hantro_fmt imx8m_vpu_g2_postproc_fmts[] = {
{
.fourcc = V4L2_PIX_FMT_NV12,
.codec_mode = HANTRO_MODE_NONE,
+ .match_depth = true,
.postprocessed = true,
.frmsize = {
.min_width = FMT_MIN_WIDTH,
@@ -165,6 +166,7 @@ static const struct hantro_fmt imx8m_vpu_g2_postproc_fmts[] = {
{
.fourcc = V4L2_PIX_FMT_P010,
.codec_mode = HANTRO_MODE_NONE,
+ .match_depth = true,
.postprocessed = true,
.frmsize = {
.min_width = FMT_MIN_WIDTH,
--
2.34.1
Hi Benjamin,
I love your patch! Yet something to improve:
[auto build test ERROR on media-tree/master]
[also build test ERROR on sailus-media-tree/streams linus/master v6.2-rc5 next-20230120]
[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/media-verisilicon-HEVC-Only-propose-10-bits-compatible-pixels-formats/20230119-164844
base: git://linuxtv.org/media_tree.git master
patch link: https://lore.kernel.org/r/20230119084723.133576-1-benjamin.gaignard%40collabora.com
patch subject: [PATCH v3] media: verisilicon: HEVC: Only propose 10 bits compatible pixels formats
config: arm-randconfig-r022-20230122 (https://download.01.org/0day-ci/archive/20230123/[email protected]/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
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
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/11b698515c987afb9699f60be773dd7c52cea592
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Benjamin-Gaignard/media-verisilicon-HEVC-Only-propose-10-bits-compatible-pixels-formats/20230119-164844
git checkout 11b698515c987afb9699f60be773dd7c52cea592
# 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=arm olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/media/platform/verisilicon/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
In file included from drivers/media/platform/verisilicon/hantro_drv.c:20:
In file included from include/linux/videodev2.h:61:
include/uapi/linux/videodev2.h:1779:2: warning: field within 'struct v4l2_ext_control' is less aligned than 'union v4l2_ext_control::(anonymous at include/uapi/linux/videodev2.h:1779:2)' and is usually due to 'struct v4l2_ext_control' being packed, which can lead to unaligned accesses [-Wunaligned-access]
union {
^
>> drivers/media/platform/verisilicon/hantro_drv.c:293:3: error: expected expression
const struct v4l2_ctrl_hevc_sps *sps = ctrl->p_new.p_hevc_sps;
^
>> drivers/media/platform/verisilicon/hantro_drv.c:294:19: error: use of undeclared identifier 'sps'
int bit_depth = sps->bit_depth_luma_minus8 + 8;
^
drivers/media/platform/verisilicon/hantro_drv.c:294:7: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement]
int bit_depth = sps->bit_depth_luma_minus8 + 8;
^
drivers/media/platform/verisilicon/hantro_drv.c:1004: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))
^~~~~
3 warnings and 2 errors generated.
vim +293 drivers/media/platform/verisilicon/hantro_drv.c
281
282 static int hantro_hevc_s_ctrl(struct v4l2_ctrl *ctrl)
283 {
284 struct hantro_ctx *ctx;
285
286 ctx = container_of(ctrl->handler,
287 struct hantro_ctx, ctrl_handler);
288
289 vpu_debug(1, "s_ctrl: id = %d, val = %d\n", ctrl->id, ctrl->val);
290
291 switch (ctrl->id) {
292 case V4L2_CID_STATELESS_HEVC_SPS:
> 293 const struct v4l2_ctrl_hevc_sps *sps = ctrl->p_new.p_hevc_sps;
> 294 int bit_depth = sps->bit_depth_luma_minus8 + 8;
295
296 if (ctx->bit_depth != bit_depth) {
297 ctx->bit_depth = bit_depth;
298 return hantro_reset_raw_fmt(ctx);
299 }
300 break;
301 default:
302 return -EINVAL;
303 }
304
305 return 0;
306 }
307
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Le jeudi 19 janvier 2023 à 09:47 +0100, Benjamin Gaignard a écrit :
> When decoding a 10bits bitstreams HEVC driver should only expose
> 10bits pixel formats.
> To fulfill this requirement it is needed to call hantro_reset_raw_fmt()
> when bit depth change and to correctly set match_depth in pixel formats
> enumeration.
>
> Fixes: dc39473d0340 ("media: hantro: imx8m: Enable 10bit decoding")
I actually tested v3, not v1:
Tested with 8bit tiled / P010 linear, as that's what we have in GStreamer
upstream and it fixed the badly selected format issue. No fluster hack was used.
Fluster score was: 141 / 147
Tested-by: Nicolas Dufresne <[email protected]>
>
> Signed-off-by: Benjamin Gaignard <[email protected]>
> ---
> version 3:
> - Propagate hantro_reset_raw_fmt() error.
> I hope I have correctly understood Ezekiel's thoughts
> in the way I have implemented them.
>
> version 2:
> - Remove struct hantro_ctx *ctx variable in hantro_try_ctrl()
> because it isn't used anymore.
>
> .../media/platform/verisilicon/hantro_drv.c | 40 +++++++++++++++----
> .../media/platform/verisilicon/hantro_v4l2.c | 6 +--
> .../media/platform/verisilicon/hantro_v4l2.h | 1 +
> .../media/platform/verisilicon/imx8m_vpu_hw.c | 2 +
> 4 files changed, 38 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 8cb4a68c9119..a713a45c0108 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -251,11 +251,6 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
>
> static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
> {
> - struct hantro_ctx *ctx;
> -
> - ctx = container_of(ctrl->handler,
> - struct hantro_ctx, ctrl_handler);
> -
> if (ctrl->id == V4L2_CID_STATELESS_H264_SPS) {
> const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
>
> @@ -274,8 +269,6 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
> if (sps->bit_depth_luma_minus8 != 0 && sps->bit_depth_luma_minus8 != 2)
> /* Only 8-bit and 10-bit are supported */
> return -EINVAL;
> -
> - ctx->bit_depth = sps->bit_depth_luma_minus8 + 8;
> } else if (ctrl->id == V4L2_CID_STATELESS_VP9_FRAME) {
> const struct v4l2_ctrl_vp9_frame *dec_params = ctrl->p_new.p_vp9_frame;
>
> @@ -286,6 +279,32 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
> return 0;
> }
>
> +static int hantro_hevc_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct hantro_ctx *ctx;
> +
> + ctx = container_of(ctrl->handler,
> + struct hantro_ctx, ctrl_handler);
> +
> + vpu_debug(1, "s_ctrl: id = %d, val = %d\n", ctrl->id, ctrl->val);
> +
> + switch (ctrl->id) {
> + case V4L2_CID_STATELESS_HEVC_SPS:
> + const struct v4l2_ctrl_hevc_sps *sps = ctrl->p_new.p_hevc_sps;
> + int bit_depth = sps->bit_depth_luma_minus8 + 8;
> +
> + if (ctx->bit_depth != bit_depth) {
> + ctx->bit_depth = bit_depth;
> + return hantro_reset_raw_fmt(ctx);
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static int hantro_jpeg_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct hantro_ctx *ctx;
> @@ -328,6 +347,11 @@ static const struct v4l2_ctrl_ops hantro_ctrl_ops = {
> .try_ctrl = hantro_try_ctrl,
> };
>
> +static const struct v4l2_ctrl_ops hantro_hevc_ctrl_ops = {
> + .s_ctrl = hantro_hevc_s_ctrl,
> + .try_ctrl = hantro_try_ctrl,
> +};
> +
> static const struct v4l2_ctrl_ops hantro_jpeg_ctrl_ops = {
> .s_ctrl = hantro_jpeg_s_ctrl,
> };
> @@ -470,7 +494,7 @@ static const struct hantro_ctrl controls[] = {
> .codec = HANTRO_HEVC_DECODER,
> .cfg = {
> .id = V4L2_CID_STATELESS_HEVC_SPS,
> - .ops = &hantro_ctrl_ops,
> + .ops = &hantro_hevc_ctrl_ops,
> },
> }, {
> .codec = HANTRO_HEVC_DECODER,
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
> index 2c7a805289e7..cd85877bbbe2 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> @@ -398,7 +398,7 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
> hantro_set_fmt_out(ctx, fmt);
> }
>
> -static void
> +int
> hantro_reset_raw_fmt(struct hantro_ctx *ctx)
> {
> const struct hantro_fmt *raw_vpu_fmt;
> @@ -420,9 +420,9 @@ hantro_reset_raw_fmt(struct hantro_ctx *ctx)
> raw_fmt->width = encoded_fmt->width;
> raw_fmt->height = encoded_fmt->height;
> if (ctx->is_encoder)
> - hantro_set_fmt_out(ctx, raw_fmt);
> + return hantro_set_fmt_out(ctx, raw_fmt);
> else
> - hantro_set_fmt_cap(ctx, raw_fmt);
> + return hantro_set_fmt_cap(ctx, raw_fmt);
> }
>
> void hantro_reset_fmts(struct hantro_ctx *ctx)
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.h b/drivers/media/platform/verisilicon/hantro_v4l2.h
> index 64f6f57e9d7a..cb8e1fe3422d 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;
>
> +int 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 *
> diff --git a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
> index b390228fd3b4..f850d8bddef6 100644
> --- a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
> +++ b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
> @@ -152,6 +152,7 @@ static const struct hantro_fmt imx8m_vpu_g2_postproc_fmts[] = {
> {
> .fourcc = V4L2_PIX_FMT_NV12,
> .codec_mode = HANTRO_MODE_NONE,
> + .match_depth = true,
> .postprocessed = true,
> .frmsize = {
> .min_width = FMT_MIN_WIDTH,
> @@ -165,6 +166,7 @@ static const struct hantro_fmt imx8m_vpu_g2_postproc_fmts[] = {
> {
> .fourcc = V4L2_PIX_FMT_P010,
> .codec_mode = HANTRO_MODE_NONE,
> + .match_depth = true,
> .postprocessed = true,
> .frmsize = {
> .min_width = FMT_MIN_WIDTH,
Le jeudi 19 janvier 2023 à 09:47 +0100, Benjamin Gaignard a écrit :
> When decoding a 10bits bitstreams HEVC driver should only expose
> 10bits pixel formats.
> To fulfill this requirement it is needed to call hantro_reset_raw_fmt()
> when bit depth change and to correctly set match_depth in pixel formats
> enumeration.
>
> Fixes: dc39473d0340 ("media: hantro: imx8m: Enable 10bit decoding")
>
> Signed-off-by: Benjamin Gaignard <[email protected]>
> ---
> version 3:
> - Propagate hantro_reset_raw_fmt() error.
> I hope I have correctly understood Ezekiel's thoughts
> in the way I have implemented them.
>
> version 2:
> - Remove struct hantro_ctx *ctx variable in hantro_try_ctrl()
> because it isn't used anymore.
>
> .../media/platform/verisilicon/hantro_drv.c | 40 +++++++++++++++----
> .../media/platform/verisilicon/hantro_v4l2.c | 6 +--
> .../media/platform/verisilicon/hantro_v4l2.h | 1 +
> .../media/platform/verisilicon/imx8m_vpu_hw.c | 2 +
> 4 files changed, 38 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 8cb4a68c9119..a713a45c0108 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -251,11 +251,6 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
>
> static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
> {
> - struct hantro_ctx *ctx;
> -
> - ctx = container_of(ctrl->handler,
> - struct hantro_ctx, ctrl_handler);
> -
> if (ctrl->id == V4L2_CID_STATELESS_H264_SPS) {
> const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
>
> @@ -274,8 +269,6 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
> if (sps->bit_depth_luma_minus8 != 0 && sps->bit_depth_luma_minus8 != 2)
> /* Only 8-bit and 10-bit are supported */
> return -EINVAL;
> -
> - ctx->bit_depth = sps->bit_depth_luma_minus8 + 8;
> } else if (ctrl->id == V4L2_CID_STATELESS_VP9_FRAME) {
> const struct v4l2_ctrl_vp9_frame *dec_params = ctrl->p_new.p_vp9_frame;
>
> @@ -286,6 +279,32 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
> return 0;
> }
>
> +static int hantro_hevc_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct hantro_ctx *ctx;
> +
> + ctx = container_of(ctrl->handler,
> + struct hantro_ctx, ctrl_handler);
> +
> + vpu_debug(1, "s_ctrl: id = %d, val = %d\n", ctrl->id, ctrl->val);
> +
> + switch (ctrl->id) {
> + case V4L2_CID_STATELESS_HEVC_SPS:
> + const struct v4l2_ctrl_hevc_sps *sps = ctrl->p_new.p_hevc_sps;
> + int bit_depth = sps->bit_depth_luma_minus8 + 8;
> +
> + if (ctx->bit_depth != bit_depth) {
> + ctx->bit_depth = bit_depth;
> + return hantro_reset_raw_fmt(ctx);
While this match 1:1 with the review comment (chain EBUSY), it does not
conceptually solved the problem since we always modify ctx here, and then modify
it futher into hantro_reset_raw_fmt() before we actually check if the queue is
busy. The code needs to be reorganize to check first, and modify after. Its
quite likely that bit_depth should be a parameter to hantro_reset_raw_fmt() so
we don't spread out the logic.
regards,
Nicolas
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static int hantro_jpeg_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct hantro_ctx *ctx;
> @@ -328,6 +347,11 @@ static const struct v4l2_ctrl_ops hantro_ctrl_ops = {
> .try_ctrl = hantro_try_ctrl,
> };
>
> +static const struct v4l2_ctrl_ops hantro_hevc_ctrl_ops = {
> + .s_ctrl = hantro_hevc_s_ctrl,
> + .try_ctrl = hantro_try_ctrl,
> +};
> +
> static const struct v4l2_ctrl_ops hantro_jpeg_ctrl_ops = {
> .s_ctrl = hantro_jpeg_s_ctrl,
> };
> @@ -470,7 +494,7 @@ static const struct hantro_ctrl controls[] = {
> .codec = HANTRO_HEVC_DECODER,
> .cfg = {
> .id = V4L2_CID_STATELESS_HEVC_SPS,
> - .ops = &hantro_ctrl_ops,
> + .ops = &hantro_hevc_ctrl_ops,
> },
> }, {
> .codec = HANTRO_HEVC_DECODER,
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
> index 2c7a805289e7..cd85877bbbe2 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> @@ -398,7 +398,7 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
> hantro_set_fmt_out(ctx, fmt);
> }
>
> -static void
> +int
> hantro_reset_raw_fmt(struct hantro_ctx *ctx)
> {
> const struct hantro_fmt *raw_vpu_fmt;
> @@ -420,9 +420,9 @@ hantro_reset_raw_fmt(struct hantro_ctx *ctx)
> raw_fmt->width = encoded_fmt->width;
> raw_fmt->height = encoded_fmt->height;
> if (ctx->is_encoder)
> - hantro_set_fmt_out(ctx, raw_fmt);
> + return hantro_set_fmt_out(ctx, raw_fmt);
> else
> - hantro_set_fmt_cap(ctx, raw_fmt);
> + return hantro_set_fmt_cap(ctx, raw_fmt);
> }
>
> void hantro_reset_fmts(struct hantro_ctx *ctx)
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.h b/drivers/media/platform/verisilicon/hantro_v4l2.h
> index 64f6f57e9d7a..cb8e1fe3422d 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;
>
> +int 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 *
> diff --git a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
> index b390228fd3b4..f850d8bddef6 100644
> --- a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
> +++ b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
> @@ -152,6 +152,7 @@ static const struct hantro_fmt imx8m_vpu_g2_postproc_fmts[] = {
> {
> .fourcc = V4L2_PIX_FMT_NV12,
> .codec_mode = HANTRO_MODE_NONE,
> + .match_depth = true,
> .postprocessed = true,
> .frmsize = {
> .min_width = FMT_MIN_WIDTH,
> @@ -165,6 +166,7 @@ static const struct hantro_fmt imx8m_vpu_g2_postproc_fmts[] = {
> {
> .fourcc = V4L2_PIX_FMT_P010,
> .codec_mode = HANTRO_MODE_NONE,
> + .match_depth = true,
> .postprocessed = true,
> .frmsize = {
> .min_width = FMT_MIN_WIDTH,
Hi Benjamin,
Thanks for the patch.
On Tue, Jan 24 2023 at 12:04:25 PM -0300, Ezequiel Garcia
<[email protected]> wrote:
>
>
> On Thu, Jan 19 2023 at 09:47:23 AM +0100, Benjamin Gaignard
> <[email protected]> wrote:
>> When decoding a 10bits bitstreams HEVC driver should only expose
>> 10bits pixel formats.
>> To fulfill this requirement it is needed to call
>> hantro_reset_raw_fmt()
>> when bit depth change and to correctly set match_depth in pixel
>> formats
>> enumeration.
>>
>> Fixes: dc39473d0340 ("media: hantro: imx8m: Enable 10bit decoding")
>>
>> Signed-off-by: Benjamin Gaignard <[email protected]>
>>
>> ---
>> version 3:
>> - Propagate hantro_reset_raw_fmt() error.
>> I hope I have correctly understood Ezekiel's thoughts
>> in the way I have implemented them.
>>
>> version 2:
>> - Remove struct hantro_ctx *ctx variable in hantro_try_ctrl()
>> because it isn't used anymore.
>>
>> .../media/platform/verisilicon/hantro_drv.c | 40
>> +++++++++++++++----
>> .../media/platform/verisilicon/hantro_v4l2.c | 6 +--
>> .../media/platform/verisilicon/hantro_v4l2.h | 1 +
>> .../media/platform/verisilicon/imx8m_vpu_hw.c | 2 +
>> 4 files changed, 38 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c
>> b/drivers/media/platform/verisilicon/hantro_drv.c
>> index 8cb4a68c9119..a713a45c0108 100644
>> --- a/drivers/media/platform/verisilicon/hantro_drv.c
>> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
>> @@ -251,11 +251,6 @@ queue_init(void *priv, struct vb2_queue
>> *src_vq, struct vb2_queue *dst_vq)
>>
>> static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
>> {
>> - struct hantro_ctx *ctx;
>> -
>> - ctx = container_of(ctrl->handler,
>> - struct hantro_ctx, ctrl_handler);
>> -
>> if (ctrl->id == V4L2_CID_STATELESS_H264_SPS) {
>> const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
>>
>> @@ -274,8 +269,6 @@ static int hantro_try_ctrl(struct v4l2_ctrl
>> *ctrl)
>> if (sps->bit_depth_luma_minus8 != 0 && sps->bit_depth_luma_minus8
>> != 2)
>> /* Only 8-bit and 10-bit are supported */
>> return -EINVAL;
>> -
>> - ctx->bit_depth = sps->bit_depth_luma_minus8 + 8;
Oh dammit this is wrong! The try_ctrl shouldn't be changing state!
I missed this when I reviewed the patch.
Please keep in mind, a "try" API in V4L2 typically just
validates but shouldn't set state.
I'm sure we will soon have a Hantro Rust driver and we will be able
to enforce state mutation rules :)
>> } else if (ctrl->id == V4L2_CID_STATELESS_VP9_FRAME) {
>> const struct v4l2_ctrl_vp9_frame *dec_params =
>> ctrl->p_new.p_vp9_frame;
>>
>> @@ -286,6 +279,32 @@ static int hantro_try_ctrl(struct v4l2_ctrl
>> *ctrl)
>> return 0;
>> }
>>
>> +static int hantro_hevc_s_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> + struct hantro_ctx *ctx;
>> +
>> + ctx = container_of(ctrl->handler,
>> + struct hantro_ctx, ctrl_handler);
>> +
>> + vpu_debug(1, "s_ctrl: id = %d, val = %d\n", ctrl->id, ctrl->val);
>> +
>> + switch (ctrl->id) {
>> + case V4L2_CID_STATELESS_HEVC_SPS:
>> + const struct v4l2_ctrl_hevc_sps *sps = ctrl->p_new.p_hevc_sps;
>> + int bit_depth = sps->bit_depth_luma_minus8 + 8;
>> +
>> + if (ctx->bit_depth != bit_depth) {
>> + ctx->bit_depth = bit_depth;
>> + return hantro_reset_raw_fmt(ctx);
You cannot simply return hantro_reset_raw() and set the bit_depth
in hantro_ctx.
If hantro_reset_raw fails, you would leave a bad state behind.
>> + }
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int hantro_jpeg_s_ctrl(struct v4l2_ctrl *ctrl)
>> {
>> struct hantro_ctx *ctx;
>> @@ -328,6 +347,11 @@ static const struct v4l2_ctrl_ops
>> hantro_ctrl_ops = {
>> .try_ctrl = hantro_try_ctrl,
>> };
>>
>> +static const struct v4l2_ctrl_ops hantro_hevc_ctrl_ops = {
>> + .s_ctrl = hantro_hevc_s_ctrl,
>> + .try_ctrl = hantro_try_ctrl,
>> +};
>> +
>> static const struct v4l2_ctrl_ops hantro_jpeg_ctrl_ops = {
>> .s_ctrl = hantro_jpeg_s_ctrl,
>> };
>> @@ -470,7 +494,7 @@ static const struct hantro_ctrl controls[] = {
>> .codec = HANTRO_HEVC_DECODER,
>> .cfg = {
>> .id = V4L2_CID_STATELESS_HEVC_SPS,
>> - .ops = &hantro_ctrl_ops,
>> + .ops = &hantro_hevc_ctrl_ops,
>> },
>> }, {
>> .codec = HANTRO_HEVC_DECODER,
>> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c
>> b/drivers/media/platform/verisilicon/hantro_v4l2.c
>> index 2c7a805289e7..cd85877bbbe2 100644
>> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
>> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
>> @@ -398,7 +398,7 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
>> hantro_set_fmt_out(ctx, fmt);
>> }
>>
>> -static void
>> +int
>> hantro_reset_raw_fmt(struct hantro_ctx *ctx)
>> {
>> const struct hantro_fmt *raw_vpu_fmt;
>> @@ -420,9 +420,9 @@ hantro_reset_raw_fmt(struct hantro_ctx *ctx)
>> raw_fmt->width = encoded_fmt->width;
>> raw_fmt->height = encoded_fmt->height;
>> if (ctx->is_encoder)
>> - hantro_set_fmt_out(ctx, raw_fmt);
>> + return hantro_set_fmt_out(ctx, raw_fmt);
>> else
>> - hantro_set_fmt_cap(ctx, raw_fmt);
>> + return hantro_set_fmt_cap(ctx, raw_fmt);
And same here, you cannot simply return from hantro_reset_raw_fmt.
You need to unroll the changes to ctx->vpu_dst_fmt,
and ctx->vpu_src_fmt.
Thanks!
>> }
>>
>> void hantro_reset_fmts(struct hantro_ctx *ctx)
>> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.h
>> b/drivers/media/platform/verisilicon/hantro_v4l2.h
>> index 64f6f57e9d7a..cb8e1fe3422d 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;
>>
>> +int 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 *
>> diff --git a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
>> b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
>> index b390228fd3b4..f850d8bddef6 100644
>> --- a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
>> +++ b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
>> @@ -152,6 +152,7 @@ static const struct hantro_fmt
>> imx8m_vpu_g2_postproc_fmts[] = {
>> {
>> .fourcc = V4L2_PIX_FMT_NV12,
>> .codec_mode = HANTRO_MODE_NONE,
>> + .match_depth = true,
>> .postprocessed = true,
>> .frmsize = {
>> .min_width = FMT_MIN_WIDTH,
>> @@ -165,6 +166,7 @@ static const struct hantro_fmt
>> imx8m_vpu_g2_postproc_fmts[] = {
>> {
>> .fourcc = V4L2_PIX_FMT_P010,
>> .codec_mode = HANTRO_MODE_NONE,
>> + .match_depth = true,
>> .postprocessed = true,
>> .frmsize = {
>> .min_width = FMT_MIN_WIDTH,
>> --
>> 2.34.1
>>