From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
To: Benjamin Gaignard <benjamin.gaignard@collabora.com>,
ezequiel@vanguardiasur.com.ar, p.zabel@pengutronix.de,
mchehab@kernel.org, robh+dt@kernel.org,
krzysztof.kozlowski+dt@linaro.org, heiko@sntech.de,
hverkuil-cisco@xs4all.nl
Cc: linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, kernel@collabora.com
Subject: Re: [PATCH v5 12/13] media: verisilicon: Conditionally ignore native formats
Date: Tue, 11 Apr 2023 11:25:17 -0400 [thread overview]
Message-ID: <8725099bbc33e4c42ddc20ed73d8e32cc2348663.camel@collabora.com> (raw)
In-Reply-To: <20230330154043.1250736-13-benjamin.gaignard@collabora.com>
Le jeudi 30 mars 2023 à 17:40 +0200, Benjamin Gaignard a écrit :
> 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>
> ---
> drivers/media/platform/verisilicon/hantro.h | 3 +
> .../media/platform/verisilicon/hantro_drv.c | 14 +++--
> .../platform/verisilicon/hantro_postproc.c | 6 +-
> .../media/platform/verisilicon/hantro_v4l2.c | 57 +++++++++++++------
> .../media/platform/verisilicon/hantro_v4l2.h | 5 +-
> 5 files changed, 60 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro.h b/drivers/media/platform/verisilicon/hantro.h
> index a98cb40a8d3b..6523ffb74881 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.
> @@ -261,6 +263,7 @@ struct hantro_ctx {
>
> const struct hantro_codec_ops *codec_ops;
> struct hantro_postproc_ctx postproc;
> + bool need_postproc;
>
> /* Specific for particular codec modes. */
> union {
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index aef1de20fc5e..7dc9e71186b4 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -319,7 +319,7 @@ static int hantro_vp9_s_ctrl(struct v4l2_ctrl *ctrl)
> if (ctx->bit_depth == bit_depth)
> return 0;
>
> - return hantro_reset_raw_fmt(ctx, bit_depth);
> + return hantro_reset_raw_fmt(ctx, bit_depth, false);
nit: My personal reference is to never add a boolean parameter (unless there is
only 1 parameter) to a function, as a reader will need to refer to the
definition to understand what the parameter is about. There is two easy fix:
1. Wrap the function with boolean into a wrapper:
/* The automatic postproc */
hantro_reset_raw_fmt()
/* The one that forces postproc */
hantro_reset_raw_fmt_forcing_postproc()
2. Use alias to true/false that gives it a readable meaning
#define HANTRO_FORCE_POSTPROC true
#define HANTRO_AUTO_POSTPROC false
One can also use an enum.
With or without this change (though I'd really like if it get cleanup later as
follow up patches).
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> }
> default:
> return -EINVAL;
> @@ -343,7 +343,7 @@ static int hantro_hevc_s_ctrl(struct v4l2_ctrl *ctrl)
> if (ctx->bit_depth == bit_depth)
> return 0;
>
> - return hantro_reset_raw_fmt(ctx, bit_depth);
> + return hantro_reset_raw_fmt(ctx, bit_depth, false);
> }
> default:
> return -EINVAL;
> @@ -363,11 +363,17 @@ static int hantro_av1_s_ctrl(struct v4l2_ctrl *ctrl)
> case V4L2_CID_STATELESS_AV1_SEQUENCE:
> {
> int bit_depth = ctrl->p_new.p_av1_sequence->bit_depth;
> + bool need_postproc = false;
>
> - if (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)
> return 0;
>
> - return hantro_reset_raw_fmt(ctx, bit_depth);
> + return hantro_reset_raw_fmt(ctx, bit_depth, need_postproc);
> }
> default:
> return -EINVAL;
> diff --git a/drivers/media/platform/verisilicon/hantro_postproc.c b/drivers/media/platform/verisilicon/hantro_postproc.c
> index bb16af50719d..22efa0347090 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;
> }
>
> @@ -197,7 +201,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, ctx->bit_depth);
> + fmt = hantro_get_default_fmt(ctx, false, ctx->bit_depth, false);
> 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 c218c9781e73..48b37b60c19b 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> @@ -31,15 +31,21 @@
> #define HANTRO_DEFAULT_BIT_DEPTH 8
>
> static int hantro_set_fmt_out(struct hantro_ctx *ctx,
> - struct v4l2_pix_format_mplane *pix_mp);
> + struct v4l2_pix_format_mplane *pix_mp,
> + bool need_postproc);
> static int hantro_set_fmt_cap(struct hantro_ctx *ctx,
> struct v4l2_pix_format_mplane *pix_mp);
>
> static const struct hantro_fmt *
> -hantro_get_formats(const struct hantro_ctx *ctx, unsigned int *num_fmts)
> +hantro_get_formats(const struct hantro_ctx *ctx, unsigned int *num_fmts, bool need_postproc)
> {
> const struct hantro_fmt *formats;
>
> + if (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;
> @@ -108,7 +114,7 @@ hantro_find_format(const struct hantro_ctx *ctx, u32 fourcc)
> const struct hantro_fmt *formats;
> unsigned int i, num_fmts;
>
> - formats = hantro_get_formats(ctx, &num_fmts);
> + formats = hantro_get_formats(ctx, &num_fmts, false);
> for (i = 0; i < num_fmts; i++)
> if (formats[i].fourcc == fourcc)
> return &formats[i];
> @@ -121,18 +127,28 @@ 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, int bit_depth)
> +hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream,
> + int bit_depth, bool need_postproc)
> {
> const struct hantro_fmt *formats;
> unsigned int i, num_fmts;
>
> - formats = hantro_get_formats(ctx, &num_fmts);
> + formats = hantro_get_formats(ctx, &num_fmts, need_postproc);
> + for (i = 0; i < num_fmts; i++) {
> + if (bitstream == (formats[i].codec_mode !=
> + HANTRO_MODE_NONE) &&
> + hantro_check_depth_match(&formats[i], bit_depth))
> + 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(&formats[i], bit_depth))
> return &formats[i];
> }
> +
> return NULL;
> }
>
> @@ -199,7 +215,7 @@ static int vidioc_enum_fmt(struct file *file, void *priv,
> */
> skip_mode_none = capture == ctx->is_encoder;
>
> - formats = hantro_get_formats(ctx, &num_fmts);
> + formats = hantro_get_formats(ctx, &num_fmts, false);
> for (i = 0; i < num_fmts; i++) {
> bool mode_none = formats[i].codec_mode == HANTRO_MODE_NONE;
> fmt = &formats[i];
> @@ -294,7 +310,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, HANTRO_DEFAULT_BIT_DEPTH);
> + fmt = hantro_get_default_fmt(ctx, coded, HANTRO_DEFAULT_BIT_DEPTH, false);
> pix_mp->pixelformat = fmt->fourcc;
> }
>
> @@ -387,7 +403,7 @@ 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_DEFAULT_BIT_DEPTH);
> + vpu_fmt = hantro_get_default_fmt(ctx, true, HANTRO_DEFAULT_BIT_DEPTH, false);
> if (!vpu_fmt)
> return;
>
> @@ -397,17 +413,17 @@ hantro_reset_encoded_fmt(struct hantro_ctx *ctx)
> if (ctx->is_encoder)
> hantro_set_fmt_cap(ctx, &fmt);
> else
> - hantro_set_fmt_out(ctx, &fmt);
> + hantro_set_fmt_out(ctx, &fmt, false);
> }
>
> int
> -hantro_reset_raw_fmt(struct hantro_ctx *ctx, int bit_depth)
> +hantro_reset_raw_fmt(struct hantro_ctx *ctx, int bit_depth, bool need_postproc)
> {
> const struct hantro_fmt *raw_vpu_fmt;
> struct v4l2_pix_format_mplane raw_fmt, *encoded_fmt;
> int ret;
>
> - raw_vpu_fmt = hantro_get_default_fmt(ctx, false, bit_depth);
> + raw_vpu_fmt = hantro_get_default_fmt(ctx, false, bit_depth, need_postproc);
> if (!raw_vpu_fmt)
> return -EINVAL;
>
> @@ -420,12 +436,14 @@ hantro_reset_raw_fmt(struct hantro_ctx *ctx, int bit_depth)
> raw_fmt.width = encoded_fmt->width;
> raw_fmt.height = encoded_fmt->height;
> if (ctx->is_encoder)
> - ret = hantro_set_fmt_out(ctx, &raw_fmt);
> + ret = hantro_set_fmt_out(ctx, &raw_fmt, need_postproc);
> else
> ret = hantro_set_fmt_cap(ctx, &raw_fmt);
>
> - if (!ret)
> + if (!ret) {
> ctx->bit_depth = bit_depth;
> + ctx->need_postproc = need_postproc;
> + }
>
> return ret;
> }
> @@ -433,7 +451,7 @@ hantro_reset_raw_fmt(struct hantro_ctx *ctx, int bit_depth)
> void hantro_reset_fmts(struct hantro_ctx *ctx)
> {
> hantro_reset_encoded_fmt(ctx);
> - hantro_reset_raw_fmt(ctx, HANTRO_DEFAULT_BIT_DEPTH);
> + hantro_reset_raw_fmt(ctx, HANTRO_DEFAULT_BIT_DEPTH, false);
> }
>
> static void
> @@ -480,7 +498,8 @@ hantro_update_requires_hold_capture_buf(struct hantro_ctx *ctx, u32 fourcc)
> }
>
> static int hantro_set_fmt_out(struct hantro_ctx *ctx,
> - struct v4l2_pix_format_mplane *pix_mp)
> + struct v4l2_pix_format_mplane *pix_mp,
> + bool need_postproc)
> {
> struct vb2_queue *vq;
> int ret;
> @@ -533,7 +552,9 @@ 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_get_format_depth(pix_mp->pixelformat));
> + hantro_reset_raw_fmt(ctx,
> + hantro_get_format_depth(pix_mp->pixelformat),
> + need_postproc);
>
> /* Colorimetry information are always propagated. */
> ctx->dst_fmt.colorspace = pix_mp->colorspace;
> @@ -596,7 +617,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_DEFAULT_BIT_DEPTH);
> + hantro_reset_raw_fmt(ctx, HANTRO_DEFAULT_BIT_DEPTH, false);
>
> /* Colorimetry information are always propagated. */
> ctx->src_fmt.colorspace = pix_mp->colorspace;
> @@ -616,7 +637,7 @@ static int hantro_set_fmt_cap(struct hantro_ctx *ctx,
> static int
> vidioc_s_fmt_out_mplane(struct file *file, void *priv, struct v4l2_format *f)
> {
> - return hantro_set_fmt_out(fh_to_ctx(priv), &f->fmt.pix_mp);
> + return hantro_set_fmt_out(fh_to_ctx(priv), &f->fmt.pix_mp, false);
> }
>
> static int
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.h b/drivers/media/platform/verisilicon/hantro_v4l2.h
> index 9ea2fef57dcd..db0ee971e4af 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.h
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.h
> @@ -21,10 +21,11 @@
> 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);
> +int hantro_reset_raw_fmt(struct hantro_ctx *ctx, int bit_depth, bool need_postproc);
> 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, int bit_depth);
> +hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream,
> + int bit_depth, bool need_postproc);
>
> #endif /* HANTRO_V4L2_H_ */
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
next prev parent reply other threads:[~2023-04-11 15:25 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-30 15:40 [PATCH v5 00/13] AV1 stateless decoder for RK3588 Benjamin Gaignard
2023-03-30 15:40 ` [PATCH v5 01/13] dt-bindings: media: rockchip-vpu: Add rk3588 vpu compatible Benjamin Gaignard
2023-03-30 15:40 ` [PATCH v5 02/13] v4l2-common: Add support for fractional bpp Benjamin Gaignard
2023-03-30 15:40 ` [PATCH v5 03/13] media: Add NV12_10LE40_4L4 pixel format Benjamin Gaignard
2023-04-11 15:07 ` Nicolas Dufresne
2023-04-11 16:01 ` Nicolas Dufresne
2023-03-30 15:40 ` [PATCH v5 04/13] media: verisilicon: Get bit depth for V4L2_PIX_FMT_NV12_10LE40_4L4 Benjamin Gaignard
2023-04-11 15:08 ` Nicolas Dufresne
2023-03-30 15:40 ` [PATCH v5 05/13] media: verisilicon: Add AV1 decoder mode and controls Benjamin Gaignard
2023-03-30 15:40 ` [PATCH v5 06/13] media: verisilicon: Check AV1 bitstreams bit depth Benjamin Gaignard
2023-04-11 15:09 ` Nicolas Dufresne
2023-03-30 15:40 ` [PATCH v5 07/13] media: verisilicon: Compute motion vectors size for AV1 frames Benjamin Gaignard
2023-04-11 15:10 ` Nicolas Dufresne
2023-03-30 15:40 ` [PATCH v5 08/13] media: verisilicon: Add AV1 entropy helpers Benjamin Gaignard
2023-04-11 15:13 ` Nicolas Dufresne
2023-03-30 15:40 ` [PATCH v5 09/13] media: verisilicon: Add Rockchip AV1 decoder Benjamin Gaignard
2023-04-11 12:33 ` Hans Verkuil
2023-04-11 13:39 ` Benjamin Gaignard
2023-04-11 15:15 ` Nicolas Dufresne
2023-03-30 15:40 ` [PATCH v5 10/13] media: verisilicon: Add film grain feature to AV1 driver Benjamin Gaignard
2023-04-11 15:17 ` Nicolas Dufresne
2023-03-30 15:40 ` [PATCH v5 11/13] media: verisilicon: Enable AV1 decoder on rk3588 Benjamin Gaignard
2023-04-11 15:18 ` Nicolas Dufresne
2023-03-30 15:40 ` [PATCH v5 12/13] media: verisilicon: Conditionally ignore native formats Benjamin Gaignard
2023-04-11 15:25 ` Nicolas Dufresne [this message]
2023-03-30 15:40 ` [PATCH v5 13/13] media: AV1: Make sure that bit depth in correctly initialize Benjamin Gaignard
2023-04-11 15:26 ` Nicolas Dufresne
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8725099bbc33e4c42ddc20ed73d8e32cc2348663.camel@collabora.com \
--to=nicolas.dufresne@collabora.com \
--cc=benjamin.gaignard@collabora.com \
--cc=devicetree@vger.kernel.org \
--cc=ezequiel@vanguardiasur.com.ar \
--cc=heiko@sntech.de \
--cc=hverkuil-cisco@xs4all.nl \
--cc=kernel@collabora.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=mchehab@kernel.org \
--cc=p.zabel@pengutronix.de \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox