Le mardi 14 juin 2022 à 18:46 +0200, Benjamin Gaignard a écrit :

Le 14/06/2022 à 18:23, Nicolas Dufresne a écrit :

quoted

Le mardi 14 juin 2022 à 17:47 +0200, Hans Verkuil a écrit :

quoted

On 6/14/22 17:43, Nicolas Dufresne wrote:

quoted

Le mardi 14 juin 2022 à 15:58 +0200, Hans Verkuil a écrit :

quoted

On 6/14/22 10:36, Benjamin Gaignard wrote:

quoted

The number of bits to skip in the slice header can be computed
in the driver by using sps, pps and decode_params information.
This allow to remove Hantro dedicated control.

allow -> makes it possible

quoted

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
  drivers/staging/media/hantro/hantro_drv.c     | 36 -----------
  .../staging/media/hantro/hantro_g2_hevc_dec.c | 62 ++++++++++++++++++-
  include/media/hevc-ctrls.h                    | 13 ----
  3 files changed, 61 insertions(+), 50 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 536c8c374952..5aac3a090480 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c

@@ -304,26 +304,6 @@ static int hantro_jpeg_s_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_HANTRO_HEVC_SLICE_HEADER_SKIP:
-		ctx->hevc_dec.ctrls.hevc_hdr_skip_length = ctrl->val;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
  static const struct v4l2_ctrl_ops hantro_ctrl_ops = {
  	.try_ctrl = hantro_try_ctrl,
  };

@@ -332,10 +312,6 @@ static const struct v4l2_ctrl_ops hantro_jpeg_ctrl_ops = {
  	.s_ctrl = hantro_jpeg_s_ctrl,
  };
  
-static const struct v4l2_ctrl_ops hantro_hevc_ctrl_ops = {
-	.s_ctrl = hantro_hevc_s_ctrl,
-};
-
  #define HANTRO_JPEG_ACTIVE_MARKERS	(V4L2_JPEG_ACTIVE_MARKER_APP0 | \
  					 V4L2_JPEG_ACTIVE_MARKER_COM | \
  					 V4L2_JPEG_ACTIVE_MARKER_DQT | \

@@ -487,18 +463,6 @@ static const struct hantro_ctrl controls[] = {
  		.cfg = {
  			.id = V4L2_CID_STATELESS_HEVC_SCALING_MATRIX,
  		},
-	}, {
-		.codec = HANTRO_HEVC_DECODER,
-		.cfg = {
-			.id = V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP,
-			.name = "Hantro HEVC slice header skip bytes",
-			.type = V4L2_CTRL_TYPE_INTEGER,
-			.min = 0,
-			.def = 0,
-			.max = 0x100,
-			.step = 1,
-			.ops = &hantro_hevc_ctrl_ops,
-		},
  	}, {
  		.codec = HANTRO_VP9_DECODER,
  		.cfg = {

diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
index d28653d04d20..3be8d6e60bf0 100644
--- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
+++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c

@@ -117,6 +117,66 @@ static void prepare_tile_info_buffer(struct hantro_ctx *ctx)
  		vpu_debug(1, "%s: no chroma!\n", __func__);
  }
  
+static unsigned int ceil_log2(unsigned int v)
+{
+	/* Compute Ceil(Log2(v))
+	 * Derived from branchless code for integer log2(v) from:
+	 * <http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog>
+	 */
+	unsigned int r, shift;
+
+	v--;
+	r = (v > 0xFFFF) << 4;
+	v >>= r;
+	shift = (v > 0xFF) << 3;
+	v >>= shift;
+	r |= shift;
+	shift = (v > 0xF) << 2;
+	v >>= shift;
+	r |= shift;
+	shift = (v > 0x3) << 1;
+	v >>= shift;
+	r |= shift;
+	r |= (v >> 1);
+
+	return r + 1;
+}

Isn't this identical to fls(v - 1)? See also lib/math/reciprocal_div.c
where this is used.

Thanks for pointing this out, I was wondering if there was an equivalent, and
never knew there was a relation between log2() and the "last set bit". Not sure
about the -1 here though, can you extend ?

Based on how lib/math/reciprocal_div.c did it. Also, the ceil_log2 function
starts with v--, while fls doesn't. That said, it's wise to verify that that
is correct.

Just for the reference, from lib/math/reciprocal_div.c:

	/* ceil(log2(d)) */
	l = fls(d - 1);

Perhaps fls() return position starting from 1 rather then 0 ?

I have compare the results of ceil_log2(d) vs fls(d-1)
for values between 2 (because sps->num_short_term_ref_pic_sets > 1) and 16 (the maximum number of frames)
they are always the same so I will remove ceil_log2() and use fls().

I've also check on my side few samples, and it indeed seems to be exactly the
same.

Regards,
Benjamin

quoted

Nicolas

quoted

Regards,

	Hans

quoted

quoted

Regards,

	Hans

quoted

+
+static int compute_header_skip_lenght(struct hantro_ctx *ctx)
+{
+	const struct hantro_hevc_dec_ctrls *ctrls = &ctx->hevc_dec.ctrls;
+	const struct v4l2_ctrl_hevc_decode_params *decode_params = ctrls->decode_params;
+	const struct v4l2_ctrl_hevc_sps *sps = ctrls->sps;
+	const struct v4l2_ctrl_hevc_pps *pps = ctrls->pps;
+	int skip = 0;
+
+	if (pps->flags & V4L2_HEVC_PPS_FLAG_OUTPUT_FLAG_PRESENT)
+		/* size of pic_output_flag */
+		skip++;
+
+	if (sps->flags & V4L2_HEVC_SPS_FLAG_SEPARATE_COLOUR_PLANE)
+		/* size of pic_order_cnt_lsb */
+		skip += 2;
+
+	if (!(decode_params->flags & V4L2_HEVC_DECODE_PARAM_FLAG_IDR_PIC)) {
+		/* size of pic_order_cnt_lsb */
+		skip += sps->log2_max_pic_order_cnt_lsb_minus4 + 4;
+
+		/* size of short_term_ref_pic_set_sps_flag */
+		skip++;
+
+		if (decode_params->short_term_ref_pic_set_size)
+			/* size of st_ref_pic_set( num_short_term_ref_pic_sets ) */
+			skip += decode_params->short_term_ref_pic_set_size;
+		else if (sps->num_short_term_ref_pic_sets > 1)
+			skip += ceil_log2(sps->num_short_term_ref_pic_sets);
+
+		skip += decode_params->long_term_ref_pic_set_size;
+	}
+
+	return skip;
+}
+
  static void set_params(struct hantro_ctx *ctx)
  {
  	const struct hantro_hevc_dec_ctrls *ctrls = &ctx->hevc_dec.ctrls;

@@ -134,7 +194,7 @@ static void set_params(struct hantro_ctx *ctx)
  
  	hantro_reg_write(vpu, &g2_output_8_bits, 0);
  
-	hantro_reg_write(vpu, &g2_hdr_skip_length, ctrls->hevc_hdr_skip_length);
+	hantro_reg_write(vpu, &g2_hdr_skip_length, compute_header_skip_lenght(ctx));
  
  	min_log2_cb_size = sps->log2_min_luma_coding_block_size_minus3 + 3;
  	max_log2_ctb_size = min_log2_cb_size + sps->log2_diff_max_min_luma_coding_block_size;

diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
index d6cb3779d190..efc0412ac41e 100644
--- a/include/media/hevc-ctrls.h
+++ b/include/media/hevc-ctrls.h

@@ -467,17 +467,4 @@ struct v4l2_ctrl_hevc_scaling_matrix {
  	__u8	scaling_list_dc_coef_32x32[2];
  };
  
-/*  MPEG-class control IDs specific to the Hantro driver as defined by V4L2 */
-#define V4L2_CID_CODEC_HANTRO_BASE				(V4L2_CTRL_CLASS_CODEC | 0x1200)
-/*
- * V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP -
- * the number of data (in bits) to skip in the
- * slice segment header.
- * If non-IDR, the bits to be skipped go from syntax element "pic_output_flag"
- * to before syntax element "slice_temporal_mvp_enabled_flag".
- * If IDR, the skipped bits are just "pic_output_flag"
- * (separate_colour_plane_flag is not supported).
- */
-#define V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP	(V4L2_CID_CODEC_HANTRO_BASE + 0)
-
  #endif

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel