| Message ID | 20220614083614.240641-15-benjamin.gaignard@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Move HEVC stateless controls out of staging | expand |
On 6/14/22 10:36, Benjamin Gaignard wrote: > 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 > > 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. Regards, Hans > + > +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
Le mardi 14 juin 2022 à 15:58 +0200, Hans Verkuil a écrit : > On 6/14/22 10:36, Benjamin Gaignard wrote: > > 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 > > > > > 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 ? > > Regards, > > Hans > > > + > > +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 >
On 6/14/22 17:43, Nicolas Dufresne wrote: > Le mardi 14 juin 2022 à 15:58 +0200, Hans Verkuil a écrit : >> On 6/14/22 10:36, Benjamin Gaignard wrote: >>> 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 >> >>> >>> 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. Regards, Hans > >> >> Regards, >> >> Hans >> >>> + >>> +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 >> >
Le mardi 14 juin 2022 à 17:47 +0200, Hans Verkuil a écrit : > > On 6/14/22 17:43, Nicolas Dufresne wrote: > > Le mardi 14 juin 2022 à 15:58 +0200, Hans Verkuil a écrit : > > > On 6/14/22 10:36, Benjamin Gaignard wrote: > > > > 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 > > > > > > > > > > > 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 ? Nicolas > > Regards, > > Hans > > > > > > > > > Regards, > > > > > > Hans > > > > > > > + > > > > +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 > > > > >
Le 14/06/2022 à 18:23, Nicolas Dufresne a écrit : > Le mardi 14 juin 2022 à 17:47 +0200, Hans Verkuil a écrit : >> On 6/14/22 17:43, Nicolas Dufresne wrote: >>> Le mardi 14 juin 2022 à 15:58 +0200, Hans Verkuil a écrit : >>>> On 6/14/22 10:36, Benjamin Gaignard wrote: >>>>> 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 >>>> >>>>> 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(). Regards, Benjamin > > Nicolas > >> Regards, >> >> Hans >> >>>> Regards, >>>> >>>> Hans >>>> >>>>> + >>>>> +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
Le mardi 14 juin 2022 à 18:46 +0200, Benjamin Gaignard a écrit : > Le 14/06/2022 à 18:23, Nicolas Dufresne a écrit : > > Le mardi 14 juin 2022 à 17:47 +0200, Hans Verkuil a écrit : > > > On 6/14/22 17:43, Nicolas Dufresne wrote: > > > > Le mardi 14 juin 2022 à 15:58 +0200, Hans Verkuil a écrit : > > > > > On 6/14/22 10:36, Benjamin Gaignard wrote: > > > > > > 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 > > > > > > > > > > > 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 > > > > > Nicolas > > > > > Regards, > > > > > > Hans > > > > > > > > Regards, > > > > > > > > > > Hans > > > > > > > > > > > + > > > > > > +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
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; +} + +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
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. 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(-)