[v5,1/2] media: verisilicon: Do not change context bit depth before validating the format
Commit Message
It is needed to check if the proposed pixels format is valid before
updating context bit depth and other internal states.
Stop using ctx->bit_depth to check format depth match and return
result to the caller.
Fixes: dc39473d0340 ("media: hantro: imx8m: Enable 10bit decoding")
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
version 5:
- Add Review and Fixes tags
.../platform/verisilicon/hantro_postproc.c | 2 +-
.../media/platform/verisilicon/hantro_v4l2.c | 53 +++++++++----------
.../media/platform/verisilicon/hantro_v4l2.h | 3 +-
3 files changed, 28 insertions(+), 30 deletions(-)
Comments
Hi Benjamin,
Thanks for taking care of this.
On Fri, Jan 27 2023 at 10:21:25 AM +0100, Benjamin Gaignard
<benjamin.gaignard@collabora.com> wrote:
> It is needed to check if the proposed pixels format is valid before
> updating context bit depth and other internal states.
> Stop using ctx->bit_depth to check format depth match and return
> result to the caller.
>
> Fixes: dc39473d0340 ("media: hantro: imx8m: Enable 10bit decoding")
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
> version 5:
> - Add Review and Fixes tags
> .../platform/verisilicon/hantro_postproc.c | 2 +-
> .../media/platform/verisilicon/hantro_v4l2.c | 53
> +++++++++----------
> .../media/platform/verisilicon/hantro_v4l2.h | 3 +-
> 3 files changed, 28 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_postproc.c
> b/drivers/media/platform/verisilicon/hantro_postproc.c
> index 09d8cf942689..6437423ccf3a 100644
> --- a/drivers/media/platform/verisilicon/hantro_postproc.c
> +++ b/drivers/media/platform/verisilicon/hantro_postproc.c
> @@ -197,7 +197,7 @@ int hantro_postproc_alloc(struct hantro_ctx *ctx)
> unsigned int i, buf_size;
>
> /* this should always pick native format */
> - fmt = hantro_get_default_fmt(ctx, false);
> + fmt = hantro_get_default_fmt(ctx, false, ctx->bit_depth);
> if (!fmt)
> return -EINVAL;
> v4l2_fill_pixfmt_mp(&pix_mp, fmt->fourcc, ctx->src_fmt.width,
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c
> b/drivers/media/platform/verisilicon/hantro_v4l2.c
> index 2c7a805289e7..2475bc05dee9 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> @@ -76,17 +76,16 @@ int hantro_get_format_depth(u32 fourcc)
> }
>
> static bool
> -hantro_check_depth_match(const struct hantro_ctx *ctx,
> - const struct hantro_fmt *fmt)
> +hantro_check_depth_match(const struct hantro_fmt *fmt, int bit_depth)
> {
> - int fmt_depth, ctx_depth = 8;
> + int fmt_depth, depth = 8;
>
> if (!fmt->match_depth && !fmt->postprocessed)
> return true;
>
> /* 0 means default depth, which is 8 */
> - if (ctx->bit_depth)
> - ctx_depth = ctx->bit_depth;
> + if (bit_depth)
> + depth = bit_depth;
>
> fmt_depth = hantro_get_format_depth(fmt->fourcc);
>
> @@ -95,9 +94,9 @@ hantro_check_depth_match(const struct hantro_ctx
> *ctx,
> * It may be possible to relax that on some HW.
> */
> if (!fmt->match_depth)
> - return fmt_depth <= ctx_depth;
> + return fmt_depth <= depth;
>
> - return fmt_depth == ctx_depth;
> + return fmt_depth == depth;
> }
>
> static const struct hantro_fmt *
> @@ -119,7 +118,7 @@ hantro_find_format(const struct hantro_ctx *ctx,
> u32 fourcc)
> }
>
> const struct hantro_fmt *
> -hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream)
> +hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream,
> int bit_depth)
> {
> const struct hantro_fmt *formats;
> unsigned int i, num_fmts;
> @@ -128,7 +127,7 @@ hantro_get_default_fmt(const struct hantro_ctx
> *ctx, bool bitstream)
> for (i = 0; i < num_fmts; i++) {
> if (bitstream == (formats[i].codec_mode !=
> HANTRO_MODE_NONE) &&
> - hantro_check_depth_match(ctx, &formats[i]))
> + hantro_check_depth_match(&formats[i], bit_depth))
> return &formats[i];
> }
> return NULL;
> @@ -203,7 +202,7 @@ static int vidioc_enum_fmt(struct file *file,
> void *priv,
>
> if (skip_mode_none == mode_none)
> continue;
> - if (!hantro_check_depth_match(ctx, fmt))
> + if (!hantro_check_depth_match(fmt, ctx->bit_depth))
> continue;
> if (j == f->index) {
> f->pixelformat = fmt->fourcc;
> @@ -223,7 +222,7 @@ static int vidioc_enum_fmt(struct file *file,
> void *priv,
> for (i = 0; i < num_fmts; i++) {
> fmt = &formats[i];
>
> - if (!hantro_check_depth_match(ctx, fmt))
> + if (!hantro_check_depth_match(fmt, ctx->bit_depth))
> continue;
> if (j == f->index) {
> f->pixelformat = fmt->fourcc;
> @@ -291,7 +290,7 @@ static int hantro_try_fmt(const struct hantro_ctx
> *ctx,
>
> fmt = hantro_find_format(ctx, pix_mp->pixelformat);
> if (!fmt) {
> - fmt = hantro_get_default_fmt(ctx, coded);
> + fmt = hantro_get_default_fmt(ctx, coded, 0);
> pix_mp->pixelformat = fmt->fourcc;
> }
>
> @@ -379,15 +378,12 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
> const struct hantro_fmt *vpu_fmt;
> struct v4l2_pix_format_mplane *fmt;
>
> - vpu_fmt = hantro_get_default_fmt(ctx, true);
> + vpu_fmt = hantro_get_default_fmt(ctx, true, 0);
>
> - if (ctx->is_encoder) {
> - ctx->vpu_dst_fmt = vpu_fmt;
> + if (ctx->is_encoder)
> fmt = &ctx->dst_fmt;
> - } else {
> - ctx->vpu_src_fmt = vpu_fmt;
> + else
> fmt = &ctx->src_fmt;
> - }
>
> hantro_reset_fmt(fmt, vpu_fmt);
> fmt->width = vpu_fmt->frmsize.min_width;
> @@ -398,20 +394,21 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
> hantro_set_fmt_out(ctx, fmt);
> }
>
> -static void
> -hantro_reset_raw_fmt(struct hantro_ctx *ctx)
> +int
> +hantro_reset_raw_fmt(struct hantro_ctx *ctx, int bit_depth)
Seems the hantro_reset_raw_fmt and hantro_reset_encoded_fmt still need
some work.
> {
> const struct hantro_fmt *raw_vpu_fmt;
> struct v4l2_pix_format_mplane *raw_fmt, *encoded_fmt;
>
> - raw_vpu_fmt = hantro_get_default_fmt(ctx, false);
> + raw_vpu_fmt = hantro_get_default_fmt(ctx, false, bit_depth);
> +
> + if (!raw_vpu_fmt)
> + return -EINVAL;
>
> if (ctx->is_encoder) {
> - ctx->vpu_src_fmt = raw_vpu_fmt;
Removing these unneeded ctx->vpu_src/dst_fmt assignments in
hantro_reset_{}
is correct. The ctx->vpu_src/dst_fmt assignment needs to be done
only in hantro_set_fmt_out/cap.
Please split this change to a separate patch.
> raw_fmt = &ctx->src_fmt;
> encoded_fmt = &ctx->dst_fmt;
> } else {
> - ctx->vpu_dst_fmt = raw_vpu_fmt;
> raw_fmt = &ctx->dst_fmt;
Now here's the evil: raw_fmt = &ctx->dst_fmt means
that raw_fmt is actually the current context dst_fmt.
But see below:
> encoded_fmt = &ctx->src_fmt;
> }
> @@ -420,15 +417,15 @@ 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);
raw_fmt (&ctx->dst_fmt) is passed to hantro_set_fmt_cap.
static int hantro_set_fmt_cap(struct hantro_ctx *ctx,
struct v4l2_pix_format_mplane *pix_mp)
{
...
ctx->dst_fmt = *pix_mp;
In other words:
ctx->dst_fmt = *(&ctx->dst_fmt) !!!
I'm thinking we could introduce another patch (after removing
ctx->vpu_src/dst_fmt)
but before "media: verisilicon: Do not change context bit depth"), to
fix the confusion
and have something like:
static void
hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
{
const struct hantro_fmt *vpu_fmt;
struct v4l2_pix_format_mplane fmt;
vpu_fmt = hantro_get_default_fmt(ctx, true);
hantro_reset_fmt(&fmt, vpu_fmt);
fmt.width = vpu_fmt->frmsize.min_width;
fmt.height = vpu_fmt->frmsize.min_height;
if (ctx->is_encoder)
hantro_set_fmt_cap(ctx, &fmt);
else
hantro_set_fmt_out(ctx, &fmt);
}
So it's clear the ctx format is actually reset to a new format
in each case; and it's similar to how hantro_set_fmt_cap/out are used
by S_FMT.
Does it make any sense?
Thanks,
Ezequiel
> }
>
> void hantro_reset_fmts(struct hantro_ctx *ctx)
> {
> hantro_reset_encoded_fmt(ctx);
> - hantro_reset_raw_fmt(ctx);
> + hantro_reset_raw_fmt(ctx, 0);
> }
>
> static void
> @@ -528,7 +525,7 @@ static int hantro_set_fmt_out(struct hantro_ctx
> *ctx,
> * changes to the raw format.
> */
> if (!ctx->is_encoder)
> - hantro_reset_raw_fmt(ctx);
> + hantro_reset_raw_fmt(ctx,
> hantro_get_format_depth(pix_mp->pixelformat));
>
> /* Colorimetry information are always propagated. */
> ctx->dst_fmt.colorspace = pix_mp->colorspace;
> @@ -591,7 +588,7 @@ static int hantro_set_fmt_cap(struct hantro_ctx
> *ctx,
> * changes to the raw format.
> */
> if (ctx->is_encoder)
> - hantro_reset_raw_fmt(ctx);
> + hantro_reset_raw_fmt(ctx, 0);
>
> /* Colorimetry information are always propagated. */
> ctx->src_fmt.colorspace = pix_mp->colorspace;
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.h
> b/drivers/media/platform/verisilicon/hantro_v4l2.h
> index 64f6f57e9d7a..9ea2fef57dcd 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.h
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.h
> @@ -21,9 +21,10 @@
> 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, int bit_depth);
> void hantro_reset_fmts(struct hantro_ctx *ctx);
> int hantro_get_format_depth(u32 fourcc);
> const struct hantro_fmt *
> -hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream);
> +hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream,
> int bit_depth);
>
> #endif /* HANTRO_V4L2_H_ */
> --
> 2.34.1
>
On Fri, Jan 27 2023 at 10:21:25 AM +0100, Benjamin Gaignard
<benjamin.gaignard@collabora.com> wrote:
> It is needed to check if the proposed pixels format is valid before
> updating context bit depth and other internal states.
> Stop using ctx->bit_depth to check format depth match and return
> result to the caller.
>
> Fixes: dc39473d0340 ("media: hantro: imx8m: Enable 10bit decoding")
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
..
> */
> if (ctx->is_encoder)
> - hantro_reset_raw_fmt(ctx);
> + hantro_reset_raw_fmt(ctx, 0);
Explicit is better than implicit.
Please replace the "0" to imply default, and instead
pass HANTRO_DEFAULT_BIT_DEPTH explicitly.
Thanks!
>
@@ -197,7 +197,7 @@ int hantro_postproc_alloc(struct hantro_ctx *ctx)
unsigned int i, buf_size;
/* this should always pick native format */
- fmt = hantro_get_default_fmt(ctx, false);
+ fmt = hantro_get_default_fmt(ctx, false, ctx->bit_depth);
if (!fmt)
return -EINVAL;
v4l2_fill_pixfmt_mp(&pix_mp, fmt->fourcc, ctx->src_fmt.width,
@@ -76,17 +76,16 @@ int hantro_get_format_depth(u32 fourcc)
}
static bool
-hantro_check_depth_match(const struct hantro_ctx *ctx,
- const struct hantro_fmt *fmt)
+hantro_check_depth_match(const struct hantro_fmt *fmt, int bit_depth)
{
- int fmt_depth, ctx_depth = 8;
+ int fmt_depth, depth = 8;
if (!fmt->match_depth && !fmt->postprocessed)
return true;
/* 0 means default depth, which is 8 */
- if (ctx->bit_depth)
- ctx_depth = ctx->bit_depth;
+ if (bit_depth)
+ depth = bit_depth;
fmt_depth = hantro_get_format_depth(fmt->fourcc);
@@ -95,9 +94,9 @@ hantro_check_depth_match(const struct hantro_ctx *ctx,
* It may be possible to relax that on some HW.
*/
if (!fmt->match_depth)
- return fmt_depth <= ctx_depth;
+ return fmt_depth <= depth;
- return fmt_depth == ctx_depth;
+ return fmt_depth == depth;
}
static const struct hantro_fmt *
@@ -119,7 +118,7 @@ hantro_find_format(const struct hantro_ctx *ctx, u32 fourcc)
}
const struct hantro_fmt *
-hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream)
+hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream, int bit_depth)
{
const struct hantro_fmt *formats;
unsigned int i, num_fmts;
@@ -128,7 +127,7 @@ hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream)
for (i = 0; i < num_fmts; i++) {
if (bitstream == (formats[i].codec_mode !=
HANTRO_MODE_NONE) &&
- hantro_check_depth_match(ctx, &formats[i]))
+ hantro_check_depth_match(&formats[i], bit_depth))
return &formats[i];
}
return NULL;
@@ -203,7 +202,7 @@ static int vidioc_enum_fmt(struct file *file, void *priv,
if (skip_mode_none == mode_none)
continue;
- if (!hantro_check_depth_match(ctx, fmt))
+ if (!hantro_check_depth_match(fmt, ctx->bit_depth))
continue;
if (j == f->index) {
f->pixelformat = fmt->fourcc;
@@ -223,7 +222,7 @@ static int vidioc_enum_fmt(struct file *file, void *priv,
for (i = 0; i < num_fmts; i++) {
fmt = &formats[i];
- if (!hantro_check_depth_match(ctx, fmt))
+ if (!hantro_check_depth_match(fmt, ctx->bit_depth))
continue;
if (j == f->index) {
f->pixelformat = fmt->fourcc;
@@ -291,7 +290,7 @@ static int hantro_try_fmt(const struct hantro_ctx *ctx,
fmt = hantro_find_format(ctx, pix_mp->pixelformat);
if (!fmt) {
- fmt = hantro_get_default_fmt(ctx, coded);
+ fmt = hantro_get_default_fmt(ctx, coded, 0);
pix_mp->pixelformat = fmt->fourcc;
}
@@ -379,15 +378,12 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
const struct hantro_fmt *vpu_fmt;
struct v4l2_pix_format_mplane *fmt;
- vpu_fmt = hantro_get_default_fmt(ctx, true);
+ vpu_fmt = hantro_get_default_fmt(ctx, true, 0);
- if (ctx->is_encoder) {
- ctx->vpu_dst_fmt = vpu_fmt;
+ if (ctx->is_encoder)
fmt = &ctx->dst_fmt;
- } else {
- ctx->vpu_src_fmt = vpu_fmt;
+ else
fmt = &ctx->src_fmt;
- }
hantro_reset_fmt(fmt, vpu_fmt);
fmt->width = vpu_fmt->frmsize.min_width;
@@ -398,20 +394,21 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
hantro_set_fmt_out(ctx, fmt);
}
-static void
-hantro_reset_raw_fmt(struct hantro_ctx *ctx)
+int
+hantro_reset_raw_fmt(struct hantro_ctx *ctx, int bit_depth)
{
const struct hantro_fmt *raw_vpu_fmt;
struct v4l2_pix_format_mplane *raw_fmt, *encoded_fmt;
- raw_vpu_fmt = hantro_get_default_fmt(ctx, false);
+ raw_vpu_fmt = hantro_get_default_fmt(ctx, false, bit_depth);
+
+ if (!raw_vpu_fmt)
+ return -EINVAL;
if (ctx->is_encoder) {
- ctx->vpu_src_fmt = raw_vpu_fmt;
raw_fmt = &ctx->src_fmt;
encoded_fmt = &ctx->dst_fmt;
} else {
- ctx->vpu_dst_fmt = raw_vpu_fmt;
raw_fmt = &ctx->dst_fmt;
encoded_fmt = &ctx->src_fmt;
}
@@ -420,15 +417,15 @@ 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)
{
hantro_reset_encoded_fmt(ctx);
- hantro_reset_raw_fmt(ctx);
+ hantro_reset_raw_fmt(ctx, 0);
}
static void
@@ -528,7 +525,7 @@ static int hantro_set_fmt_out(struct hantro_ctx *ctx,
* changes to the raw format.
*/
if (!ctx->is_encoder)
- hantro_reset_raw_fmt(ctx);
+ hantro_reset_raw_fmt(ctx, hantro_get_format_depth(pix_mp->pixelformat));
/* Colorimetry information are always propagated. */
ctx->dst_fmt.colorspace = pix_mp->colorspace;
@@ -591,7 +588,7 @@ static int hantro_set_fmt_cap(struct hantro_ctx *ctx,
* changes to the raw format.
*/
if (ctx->is_encoder)
- hantro_reset_raw_fmt(ctx);
+ hantro_reset_raw_fmt(ctx, 0);
/* Colorimetry information are always propagated. */
ctx->src_fmt.colorspace = pix_mp->colorspace;
@@ -21,9 +21,10 @@
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, int bit_depth);
void hantro_reset_fmts(struct hantro_ctx *ctx);
int hantro_get_format_depth(u32 fourcc);
const struct hantro_fmt *
-hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream);
+hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream, int bit_depth);
#endif /* HANTRO_V4L2_H_ */