Add a new helper to set variable length values within a bitmap, that can
overflow the borders of a single BITS_PER_LONG container.
This function makes it easier to write values to hardware memory blobs that
do not require alignment.
Add tests to the lib/test_bitmap.c kselftest module to verify proper function.
Signed-off-by: Sebastian Fricke <[email protected]>
---
include/linux/bitmap.h | 40 +++++++++++++++++++++++++++++++++++
lib/test_bitmap.c | 48 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 88 insertions(+)
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 2e6cd5681040..9f8d635b70a9 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -76,6 +76,7 @@ struct device;
* bitmap_to_arr64(buf, src, nbits) Copy nbits from buf to u64[] dst
* bitmap_get_value8(map, start) Get 8bit value from map at start
* bitmap_set_value8(map, value, start) Set 8bit value to map at start
+ * bitmap_set_value(map, value, start, nbits) Set a variable length value to map at start
*
* Note, bitmap_zero() and bitmap_fill() operate over the region of
* unsigned longs, that is, bits behind bitmap till the unsigned long
@@ -573,6 +574,45 @@ static inline void bitmap_set_value8(unsigned long *map, unsigned long value,
map[index] |= value << offset;
}
+/**
+ * bitmap_set_value - set a variable length value within a memory region
+ * @map: address to the bitmap memory region
+ * @value: the variable length value
+ * @start: bit offset of the value
+ * @length: Length of the value
+ */
+static inline void bitmap_set_value(unsigned long *map, unsigned long value,
+ unsigned long start, unsigned char length)
+{
+ size_t index = BIT_WORD(start);
+ unsigned long offset = start % BITS_PER_LONG;
+ int diff_to_max = 0;
+
+ if (!length)
+ return;
+
+
+ if (length < BITS_PER_LONG)
+ value &= (BIT(length) - 1);
+
+ while (length > 0) {
+ diff_to_max = BITS_PER_LONG - offset;
+ map[index] &= ~((BIT(length) - 1) << offset);
+ if (length > diff_to_max) {
+ unsigned long tmp = value & (BIT(diff_to_max) - 1);
+
+ map[index] |= tmp << offset;
+ value >>= diff_to_max;
+ length -= diff_to_max;
+ index += 1;
+ offset = 0;
+ } else {
+ map[index] |= value << offset;
+ length = 0;
+ }
+ }
+}
+
#endif /* __ASSEMBLY__ */
#endif /* __LINUX_BITMAP_H */
diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index d5923a640457..509317ad2f72 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -869,6 +869,53 @@ static void __init test_bitmap_print_buf(void)
}
}
+struct test_bitmap_set_value_sample {
+ unsigned long value[2];
+ unsigned char length[2];
+ unsigned int offset[2];
+ unsigned long expected[2][2];
+ int amount;
+};
+
+static const struct test_bitmap_set_value_sample test_set[] __initconst = {
+ /* Check that multiple values can be chained up */
+ { {10, 20}, {4, 5}, {0, 4}, {{10, 330}}, 2 },
+ /* Check that a value can be set across two BITS_PER_LONG chunks */
+ { {10, 6}, {4, 3}, {0, 63}, {{10, 10}, {0, 3}}, 2 },
+ /* Set a value with length shorter than the given length */
+ { {3, 6}, {4, 10}, {0, 4}, {{3, 99}}, 1 },
+ /* Set a value with length longer than the given length */
+ { {15}, {2}, {0}, {{3}}, 1 },
+ /* Check that values are properly overwritten */
+ { {15, 12}, {4, 4}, {0, 2}, {{15, 51}}, 2 },
+ /* Check that a set without a length doesn't change anything */
+ { {10}, {0}, {0}, {{0}}, 1 },
+};
+
+static void __init test_bitmap_set_value(void)
+{
+ int i, j, k;
+ int correct_tests = 0;
+
+ for (i = 0; i < ARRAY_SIZE(test_set); i++) {
+ const struct test_bitmap_set_value_sample *t = &test_set[i];
+ int test_correct = 1;
+ DECLARE_BITMAP(map, BITS_PER_LONG * 2);
+
+ bitmap_zero(map, BITS_PER_LONG * 2);
+ for (j = 0; j < t->amount; j++) {
+ bitmap_set_value(map, t->value[j], t->offset[j], t->length[j]);
+ for (k = 0; k < 2; k++) {
+ if (expect_eq_uint(map[k], t->expected[k][j]))
+ test_correct = 0;
+ }
+ }
+ if (test_correct)
+ correct_tests += 1;
+ }
+ pr_err("set_value: %d/%ld tests correct\n", correct_tests, ARRAY_SIZE(test_set));
+}
+
static void __init selftest(void)
{
test_zero_clear();
@@ -884,6 +931,7 @@ static void __init selftest(void)
test_for_each_set_clump8();
test_bitmap_cut();
test_bitmap_print_buf();
+ test_bitmap_set_value();
}
KSTM_MODULE_LOADERS(test_bitmap);
--
2.25.1
Provide a callback for codec variant drivers to indicate the correct
output pixel-format after receiving the SPS of the byte-stream, on the
initial s_ctrl ioctl call from user-space.
Signed-off-by: Jonas Karlman <[email protected]>
Signed-off-by: Sebastian Fricke <[email protected]>
---
drivers/staging/media/rkvdec/TODO | 2 ++
drivers/staging/media/rkvdec/rkvdec.c | 45 +++++++++++++++++++++++----
drivers/staging/media/rkvdec/rkvdec.h | 2 ++
3 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/media/rkvdec/TODO b/drivers/staging/media/rkvdec/TODO
index 2c0779383276..44e1e06ee6e4 100644
--- a/drivers/staging/media/rkvdec/TODO
+++ b/drivers/staging/media/rkvdec/TODO
@@ -9,3 +9,5 @@
code in rkvdec_request_validate and cedrus_request_validate.
The helper needs to the driver private data associated with
the videobuf2 queue, from a media request.
+
+* Implement the valid_fmt callback for H264 and VP9
diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 7bab7586918c..2625e0a736f4 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -27,6 +27,23 @@
#include "rkvdec.h"
#include "rkvdec-regs.h"
+/*
+ * Fetch the optimal pixel-format directly from the codec variation. If the
+ * valid_fmt callback is not implemented, then the context variable valid_fmt
+ * will be unset.
+ * When the `valid_fmt` variable is not set, the default pixel_format (the first
+ * entry of the decoded_fmts) is used and `enum_capture_fmt` will return all
+ * available formats for the current coded format.
+ */
+static int rkvdec_get_valid_fmt(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
+{
+ const struct rkvdec_coded_fmt_desc *coded_desc = ctx->coded_fmt_desc;
+
+ if (coded_desc->ops->valid_fmt)
+ return coded_desc->ops->valid_fmt(ctx, ctrl);
+ return 0;
+}
+
static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
{
struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
@@ -35,6 +52,9 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
if (desc->ops->try_ctrl)
return desc->ops->try_ctrl(ctx, ctrl);
+ if (ctx->valid_fmt && ctx->valid_fmt != rkvdec_get_valid_fmt(ctx, ctrl))
+ /* Only allow the current valid format */
+ return -EINVAL;
return 0;
}
@@ -190,6 +210,7 @@ static void rkvdec_reset_decoded_fmt(struct rkvdec_ctx *ctx)
{
struct v4l2_format *f = &ctx->decoded_fmt;
+ ctx->valid_fmt = 0;
rkvdec_reset_fmt(ctx, f, ctx->coded_fmt_desc->decoded_fmts[0]);
f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
v4l2_fill_pixfmt_mp(&f->fmt.pix_mp,
@@ -249,14 +270,17 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
if (WARN_ON(!coded_desc))
return -EINVAL;
- for (i = 0; i < coded_desc->num_decoded_fmts; i++) {
- if (coded_desc->decoded_fmts[i] == pix_mp->pixelformat)
- break;
+ if (ctx->valid_fmt) {
+ pix_mp->pixelformat = ctx->valid_fmt;
+ } else {
+ for (i = 0; i < coded_desc->num_decoded_fmts; i++) {
+ if (coded_desc->decoded_fmts[i] == pix_mp->pixelformat)
+ break;
+ }
+ if (i == coded_desc->num_decoded_fmts)
+ pix_mp->pixelformat = coded_desc->decoded_fmts[0];
}
- if (i == coded_desc->num_decoded_fmts)
- pix_mp->pixelformat = coded_desc->decoded_fmts[0];
-
/* Always apply the frmsize constraint of the coded end. */
pix_mp->width = max(pix_mp->width, ctx->coded_fmt.fmt.pix_mp.width);
pix_mp->height = max(pix_mp->height, ctx->coded_fmt.fmt.pix_mp.height);
@@ -325,6 +349,7 @@ static int rkvdec_s_capture_fmt(struct file *file, void *priv,
return ret;
ctx->decoded_fmt = *f;
+ ctx->valid_fmt = f->fmt.pix_mp.pixelformat;
return 0;
}
@@ -428,6 +453,14 @@ static int rkvdec_enum_capture_fmt(struct file *file, void *priv,
if (WARN_ON(!ctx->coded_fmt_desc))
return -EINVAL;
+ if (ctx->valid_fmt) {
+ if (f->index)
+ return -EINVAL;
+
+ f->pixelformat = ctx->valid_fmt;
+ return 0;
+ }
+
if (f->index >= ctx->coded_fmt_desc->num_decoded_fmts)
return -EINVAL;
diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
index 633335ebb9c4..b9e219438bc9 100644
--- a/drivers/staging/media/rkvdec/rkvdec.h
+++ b/drivers/staging/media/rkvdec/rkvdec.h
@@ -66,6 +66,7 @@ vb2_to_rkvdec_decoded_buf(struct vb2_buffer *buf)
struct rkvdec_coded_fmt_ops {
int (*adjust_fmt)(struct rkvdec_ctx *ctx,
struct v4l2_format *f);
+ u32 (*valid_fmt)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
int (*start)(struct rkvdec_ctx *ctx);
void (*stop)(struct rkvdec_ctx *ctx);
int (*run)(struct rkvdec_ctx *ctx);
@@ -101,6 +102,7 @@ struct rkvdec_ctx {
struct v4l2_fh fh;
struct v4l2_format coded_fmt;
struct v4l2_format decoded_fmt;
+ u32 valid_fmt;
const struct rkvdec_coded_fmt_desc *coded_fmt_desc;
struct v4l2_ctrl_handler ctrl_hdl;
struct rkvdec_dev *dev;
--
2.25.1
Enable user-space to set the SPS of the current byte-stream on the
decoder. This action will trigger the decoder to pick the optimal
pixel-format for the capture queue.
Signed-off-by: Sebastian Fricke <[email protected]>
Signed-off-by: Jonas Karlman <[email protected]>
---
drivers/staging/media/rkvdec/rkvdec.c | 60 ++++++++++++++++++++-------
1 file changed, 46 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 2625e0a736f4..f84579f764ab 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -27,6 +27,17 @@
#include "rkvdec.h"
#include "rkvdec-regs.h"
+static void rkvdec_fill_decoded_pixfmt(struct rkvdec_ctx *ctx,
+ struct v4l2_pix_format_mplane *pix_mp)
+{
+ v4l2_fill_pixfmt_mp(pix_mp, pix_mp->pixelformat,
+ pix_mp->width, pix_mp->height);
+ pix_mp->plane_fmt[0].sizeimage += 128 *
+ DIV_ROUND_UP(pix_mp->width, 16) *
+ DIV_ROUND_UP(pix_mp->height, 16);
+ pix_mp->field = V4L2_FIELD_NONE;
+}
+
/*
* Fetch the optimal pixel-format directly from the codec variation. If the
* valid_fmt callback is not implemented, then the context variable valid_fmt
@@ -44,6 +55,27 @@ static int rkvdec_get_valid_fmt(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
return 0;
}
+static int rkvdec_set_valid_fmt(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
+{
+ struct v4l2_pix_format_mplane *pix_mp;
+
+ switch (ctrl->id) {
+ case V4L2_CID_STATELESS_H264_SPS:
+ case V4L2_CID_STATELESS_VP9_FRAME:
+ case V4L2_CID_STATELESS_HEVC_SPS:
+ ctx->valid_fmt = rkvdec_get_valid_fmt(ctx, ctrl);
+
+ pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
+ pix_mp->pixelformat = ctx->valid_fmt;
+ rkvdec_fill_decoded_pixfmt(ctx, pix_mp);
+ break;
+ default:
+ dev_err(ctx->dev->dev, "Unsupported stateless control ID: %x\n", ctrl->id);
+ return -EINVAL;
+ };
+ return 0;
+}
+
static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
{
struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
@@ -58,8 +90,18 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
return 0;
}
+static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
+
+ if (!ctx->valid_fmt)
+ return rkvdec_set_valid_fmt(ctx, ctrl);
+ return 0;
+}
+
static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
.try_ctrl = rkvdec_try_ctrl,
+ .s_ctrl = rkvdec_s_ctrl,
};
static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
@@ -213,13 +255,9 @@ static void rkvdec_reset_decoded_fmt(struct rkvdec_ctx *ctx)
ctx->valid_fmt = 0;
rkvdec_reset_fmt(ctx, f, ctx->coded_fmt_desc->decoded_fmts[0]);
f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
- v4l2_fill_pixfmt_mp(&f->fmt.pix_mp,
- ctx->coded_fmt_desc->decoded_fmts[0],
- ctx->coded_fmt.fmt.pix_mp.width,
- ctx->coded_fmt.fmt.pix_mp.height);
- f->fmt.pix_mp.plane_fmt[0].sizeimage += 128 *
- DIV_ROUND_UP(f->fmt.pix_mp.width, 16) *
- DIV_ROUND_UP(f->fmt.pix_mp.height, 16);
+ f->fmt.pix_mp.width = ctx->coded_fmt.fmt.pix_mp.width;
+ f->fmt.pix_mp.height = ctx->coded_fmt.fmt.pix_mp.height;
+ rkvdec_fill_decoded_pixfmt(ctx, &f->fmt.pix_mp);
}
static int rkvdec_enum_framesizes(struct file *file, void *priv,
@@ -288,13 +326,7 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
&pix_mp->height,
&coded_desc->frmsize);
- v4l2_fill_pixfmt_mp(pix_mp, pix_mp->pixelformat,
- pix_mp->width, pix_mp->height);
- pix_mp->plane_fmt[0].sizeimage +=
- 128 *
- DIV_ROUND_UP(pix_mp->width, 16) *
- DIV_ROUND_UP(pix_mp->height, 16);
- pix_mp->field = V4L2_FIELD_NONE;
+ rkvdec_fill_decoded_pixfmt(ctx, pix_mp);
return 0;
}
--
2.25.1
On Wed, Jul 13, 2022 at 06:31:59PM +0200, Sebastian Fricke wrote:
> Add a new helper to set variable length values within a bitmap, that can
> overflow the borders of a single BITS_PER_LONG container.
> This function makes it easier to write values to hardware memory blobs that
> do not require alignment.
>
> Add tests to the lib/test_bitmap.c kselftest module to verify proper function.
>
> Signed-off-by: Sebastian Fricke <[email protected]>
> ---
> include/linux/bitmap.h | 40 +++++++++++++++++++++++++++++++++++
> lib/test_bitmap.c | 48 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 88 insertions(+)
>
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index 2e6cd5681040..9f8d635b70a9 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -76,6 +76,7 @@ struct device;
> * bitmap_to_arr64(buf, src, nbits) Copy nbits from buf to u64[] dst
> * bitmap_get_value8(map, start) Get 8bit value from map at start
> * bitmap_set_value8(map, value, start) Set 8bit value to map at start
> + * bitmap_set_value(map, value, start, nbits) Set a variable length value to map at start
> *
> * Note, bitmap_zero() and bitmap_fill() operate over the region of
> * unsigned longs, that is, bits behind bitmap till the unsigned long
> @@ -573,6 +574,45 @@ static inline void bitmap_set_value8(unsigned long *map, unsigned long value,
> map[index] |= value << offset;
> }
>
> +/**
> + * bitmap_set_value - set a variable length value within a memory region
> + * @map: address to the bitmap memory region
> + * @value: the variable length value
> + * @start: bit offset of the value
> + * @length: Length of the value
There's no such thing like a length of value. Data structures and
types have size, and arrays have length.
> + */
> +static inline void bitmap_set_value(unsigned long *map, unsigned long value,
> + unsigned long start, unsigned char length)
> +{
> + size_t index = BIT_WORD(start);
> + unsigned long offset = start % BITS_PER_LONG;
> + int diff_to_max = 0;
> +
> + if (!length)
> + return;
> +
> +
2nd empty line is not needed. Actually, all this chunk is not needed
because 'while (length > 0)' will do the work.
> + if (length < BITS_PER_LONG)
> + value &= (BIT(length) - 1);
> +
> + while (length > 0) {
> + diff_to_max = BITS_PER_LONG - offset;
> + map[index] &= ~((BIT(length) - 1) << offset);
> + if (length > diff_to_max) {
> + unsigned long tmp = value & (BIT(diff_to_max) - 1);
We have GENMASK() for this.
> +
> + map[index] |= tmp << offset;
> + value >>= diff_to_max;
> + length -= diff_to_max;
> + index += 1;
> + offset = 0;
> + } else {
> + map[index] |= value << offset;
> + length = 0;
> + }
> + }
I have a strong feeling that this can be written much simpler...
But anyways, this is not suitable for generic bitmaps because this
bitmap_set_value() is limited with a single words. All bitmap functions
that copy data to/from bitmap are able to work with bigger chunks. (With
the exception of bitmap_{set,get}_value8, which doesn't allow unaligned
accesses.)
What you want is to copy bits to the dst bitmap starting from the offset,
right? It's very similar to what bitmap_set() does, except that it always
'copies' ~0UL.
I'd suggest you to try implementing
bitmap_copy_from(dst, src, dst_off, len)
or even
bitmap_copy_from(dst, dst_off, src, src_off, len)
if you expect that you'll need more flexibility in the future.
This bitmap_copy_from() may be based, for example, on extended version
of __bitmap_set():
void __bitmap_set(unsigned long *dst, unsigned long *src, unsigned int start, int len)
Thanks,
Yury
> +}
> +
> #endif /* __ASSEMBLY__ */
>
> #endif /* __LINUX_BITMAP_H */
> diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
> index d5923a640457..509317ad2f72 100644
> --- a/lib/test_bitmap.c
> +++ b/lib/test_bitmap.c
> @@ -869,6 +869,53 @@ static void __init test_bitmap_print_buf(void)
> }
> }
>
> +struct test_bitmap_set_value_sample {
> + unsigned long value[2];
> + unsigned char length[2];
> + unsigned int offset[2];
> + unsigned long expected[2][2];
> + int amount;
> +};
> +
> +static const struct test_bitmap_set_value_sample test_set[] __initconst = {
> + /* Check that multiple values can be chained up */
> + { {10, 20}, {4, 5}, {0, 4}, {{10, 330}}, 2 },
> + /* Check that a value can be set across two BITS_PER_LONG chunks */
> + { {10, 6}, {4, 3}, {0, 63}, {{10, 10}, {0, 3}}, 2 },
> + /* Set a value with length shorter than the given length */
> + { {3, 6}, {4, 10}, {0, 4}, {{3, 99}}, 1 },
> + /* Set a value with length longer than the given length */
> + { {15}, {2}, {0}, {{3}}, 1 },
> + /* Check that values are properly overwritten */
> + { {15, 12}, {4, 4}, {0, 2}, {{15, 51}}, 2 },
> + /* Check that a set without a length doesn't change anything */
> + { {10}, {0}, {0}, {{0}}, 1 },
> +};
> +
> +static void __init test_bitmap_set_value(void)
> +{
> + int i, j, k;
> + int correct_tests = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(test_set); i++) {
> + const struct test_bitmap_set_value_sample *t = &test_set[i];
> + int test_correct = 1;
> + DECLARE_BITMAP(map, BITS_PER_LONG * 2);
> +
> + bitmap_zero(map, BITS_PER_LONG * 2);
> + for (j = 0; j < t->amount; j++) {
> + bitmap_set_value(map, t->value[j], t->offset[j], t->length[j]);
> + for (k = 0; k < 2; k++) {
> + if (expect_eq_uint(map[k], t->expected[k][j]))
> + test_correct = 0;
> + }
> + }
> + if (test_correct)
> + correct_tests += 1;
> + }
> + pr_err("set_value: %d/%ld tests correct\n", correct_tests, ARRAY_SIZE(test_set));
> +}
> +
> static void __init selftest(void)
> {
> test_zero_clear();
> @@ -884,6 +931,7 @@ static void __init selftest(void)
> test_for_each_set_clump8();
> test_bitmap_cut();
> test_bitmap_print_buf();
> + test_bitmap_set_value();
> }
>
> KSTM_MODULE_LOADERS(test_bitmap);
> --
> 2.25.1
Hi Sebastian,
On Wed, Jul 13, 2022 at 1:33 PM Sebastian Fricke
<[email protected]> wrote:
>
> Enable user-space to set the SPS of the current byte-stream on the
> decoder. This action will trigger the decoder to pick the optimal
> pixel-format for the capture queue.
>
> Signed-off-by: Sebastian Fricke <[email protected]>
> Signed-off-by: Jonas Karlman <[email protected]>
> ---
> drivers/staging/media/rkvdec/rkvdec.c | 60 ++++++++++++++++++++-------
> 1 file changed, 46 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index 2625e0a736f4..f84579f764ab 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -27,6 +27,17 @@
> #include "rkvdec.h"
> #include "rkvdec-regs.h"
>
> +static void rkvdec_fill_decoded_pixfmt(struct rkvdec_ctx *ctx,
> + struct v4l2_pix_format_mplane *pix_mp)
> +{
> + v4l2_fill_pixfmt_mp(pix_mp, pix_mp->pixelformat,
> + pix_mp->width, pix_mp->height);
> + pix_mp->plane_fmt[0].sizeimage += 128 *
> + DIV_ROUND_UP(pix_mp->width, 16) *
> + DIV_ROUND_UP(pix_mp->height, 16);
> + pix_mp->field = V4L2_FIELD_NONE;
> +}
> +
> /*
> * Fetch the optimal pixel-format directly from the codec variation. If the
> * valid_fmt callback is not implemented, then the context variable valid_fmt
> @@ -44,6 +55,27 @@ static int rkvdec_get_valid_fmt(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
> return 0;
> }
>
> +static int rkvdec_set_valid_fmt(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
> +{
> + struct v4l2_pix_format_mplane *pix_mp;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_STATELESS_H264_SPS:
> + case V4L2_CID_STATELESS_VP9_FRAME:
> + case V4L2_CID_STATELESS_HEVC_SPS:
> + ctx->valid_fmt = rkvdec_get_valid_fmt(ctx, ctrl);
> +
> + pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
> + pix_mp->pixelformat = ctx->valid_fmt;
> + rkvdec_fill_decoded_pixfmt(ctx, pix_mp);
> + break;
> + default:
> + dev_err(ctx->dev->dev, "Unsupported stateless control ID: %x\n", ctrl->id);
> + return -EINVAL;
> + };
> + return 0;
> +}
> +
> static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> @@ -58,8 +90,18 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
> return 0;
> }
>
> +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> +
> + if (!ctx->valid_fmt)
> + return rkvdec_set_valid_fmt(ctx, ctrl);
We've discussed this approach with Jonas.
See "[PATCH v2 10/12] media: rkvdec: Lock capture pixel format in
s_ctrl and s_fmt".
https://lore.kernel.org/all/[email protected]/
The outcome of the discussion is:
* setting SPS resets the CAPTURE format (as per the spec).
* the application uses ENUM/TRY/S_FMT to negotiate a format, and uses
SPS to filter formats.
Thanks!
Ezequiel
On Wed, Jul 13, 2022 at 8:56 PM Yury Norov <[email protected]> wrote:
> On Wed, Jul 13, 2022 at 06:31:59PM +0200, Sebastian Fricke wrote:
...
> I'd suggest you to try implementing
> bitmap_copy_from(dst, src, dst_off, len)
> or even
> bitmap_copy_from(dst, dst_off, src, src_off, len)
> if you expect that you'll need more flexibility in the future.
Do you think it would be useful?
We have bitmap_replace() & bitmap_remap(). Wouldn't that be enough?
--
With Best Regards,
Andy Shevchenko
On Wed, Jul 13, 2022 at 9:44 PM Yury Norov <[email protected]> wrote:
> On Wed, Jul 13, 2022 at 09:10:33PM +0200, Andy Shevchenko wrote:
> > On Wed, Jul 13, 2022 at 8:56 PM Yury Norov <[email protected]> wrote:
> > > On Wed, Jul 13, 2022 at 06:31:59PM +0200, Sebastian Fricke wrote:
> >
> > ...
> >
> > > I'd suggest you to try implementing
> > > bitmap_copy_from(dst, src, dst_off, len)
> > > or even
> > > bitmap_copy_from(dst, dst_off, src, src_off, len)
> > > if you expect that you'll need more flexibility in the future.
> >
> > Do you think it would be useful?
> >
> > We have bitmap_replace() & bitmap_remap(). Wouldn't that be enough?
>
> bitmap_replace and bitmap_remap have no an 'offset' parameter.
True.
But then it's a bit too generic to have this src_off, no?
I would rather expect for asymmetrical bitmaps that the other side
will be either one of the fixed width types (it makes sense to have
for 32- or 64-bit arguments.
When you have a source bitmap of x bits and you would like to copy it
into a y-bit one, I would think that either you have a small amount of
bits in x anyway, or x is a full-sized bitmap (same order as y). Also
keep in mind that granularity is long, so less than long it makes no
sense.
bitmap_copy_from_T(unsigned long *map, start, len, T src),
where T is type, start is the offset in map, len is the amount of bits
from src starting from 0. That's what is required in most of the cases
I believe.
--
With Best Regards,
Andy Shevchenko
On Wed, Jul 13, 2022 at 09:10:33PM +0200, Andy Shevchenko wrote:
> On Wed, Jul 13, 2022 at 8:56 PM Yury Norov <[email protected]> wrote:
> > On Wed, Jul 13, 2022 at 06:31:59PM +0200, Sebastian Fricke wrote:
>
> ...
>
> > I'd suggest you to try implementing
> > bitmap_copy_from(dst, src, dst_off, len)
> > or even
> > bitmap_copy_from(dst, dst_off, src, src_off, len)
> > if you expect that you'll need more flexibility in the future.
>
> Do you think it would be useful?
>
> We have bitmap_replace() & bitmap_remap(). Wouldn't that be enough?
bitmap_replace and bitmap_remap have no an 'offset' parameter.
On Wed, Jul 13, 2022 at 10:14:24PM +0200, Andy Shevchenko wrote:
> On Wed, Jul 13, 2022 at 9:44 PM Yury Norov <[email protected]> wrote:
> > On Wed, Jul 13, 2022 at 09:10:33PM +0200, Andy Shevchenko wrote:
> > > On Wed, Jul 13, 2022 at 8:56 PM Yury Norov <[email protected]> wrote:
> > > > On Wed, Jul 13, 2022 at 06:31:59PM +0200, Sebastian Fricke wrote:
> > >
> > > ...
> > >
> > > > I'd suggest you to try implementing
> > > > bitmap_copy_from(dst, src, dst_off, len)
> > > > or even
> > > > bitmap_copy_from(dst, dst_off, src, src_off, len)
> > > > if you expect that you'll need more flexibility in the future.
> > >
> > > Do you think it would be useful?
> > >
> > > We have bitmap_replace() & bitmap_remap(). Wouldn't that be enough?
> >
> > bitmap_replace and bitmap_remap have no an 'offset' parameter.
>
> True.
>
> But then it's a bit too generic to have this src_off, no?
That's why I said:
> > > > if you expect that you'll need more flexibility in the future.
My preferred option is bitmap_copy_from(dst, src, dst_off, len).
> I would rather expect for asymmetrical bitmaps that the other side
> will be either one of the fixed width types (it makes sense to have
> for 32- or 64-bit arguments.
Look at patch #6 - it copies 1,4,5,9,10,32,37... - pretty much a random
number number of bits.
> When you have a source bitmap of x bits and you would like to copy it
> into a y-bit one, I would think that either you have a small amount of
> bits in x anyway, or x is a full-sized bitmap (same order as y).
It sounds like a speculation to me. Why shouldn't we let people to
copy with an offset any number of bits?
> Also
> keep in mind that granularity is long, so less than long it makes no
> sense.
>
> bitmap_copy_from_T(unsigned long *map, start, len, T src),
>
> where T is type, start is the offset in map, len is the amount of bits
> from src starting from 0. That's what is required in most of the cases
> I believe.
But not in Sebastian's case, according to patch #6.
Thanks,
Yury
On Wed, Jul 13, 2022 at 01:42:17PM -0700, Yury Norov wrote:
> On Wed, Jul 13, 2022 at 10:14:24PM +0200, Andy Shevchenko wrote:
> > On Wed, Jul 13, 2022 at 9:44 PM Yury Norov <[email protected]> wrote:
> > > On Wed, Jul 13, 2022 at 09:10:33PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Jul 13, 2022 at 8:56 PM Yury Norov <[email protected]> wrote:
> > > > > On Wed, Jul 13, 2022 at 06:31:59PM +0200, Sebastian Fricke wrote:
...
> > > > > I'd suggest you to try implementing
> > > > > bitmap_copy_from(dst, src, dst_off, len)
> > > > > or even
> > > > > bitmap_copy_from(dst, dst_off, src, src_off, len)
> > > > > if you expect that you'll need more flexibility in the future.
> > > >
> > > > Do you think it would be useful?
> > > >
> > > > We have bitmap_replace() & bitmap_remap(). Wouldn't that be enough?
> > >
> > > bitmap_replace and bitmap_remap have no an 'offset' parameter.
> >
> > True.
> >
> > But then it's a bit too generic to have this src_off, no?
>
> That's why I said:
>
> > > > > if you expect that you'll need more flexibility in the future.
>
> My preferred option is bitmap_copy_from(dst, src, dst_off, len).
>
> > I would rather expect for asymmetrical bitmaps that the other side
> > will be either one of the fixed width types (it makes sense to have
> > for 32- or 64-bit arguments.
>
> Look at patch #6 - it copies 1,4,5,9,10,32,37... - pretty much a random
> number number of bits.
It's too poor randomness, as u64 covers all what in patch 6.
> > When you have a source bitmap of x bits and you would like to copy it
> > into a y-bit one, I would think that either you have a small amount of
> > bits in x anyway, or x is a full-sized bitmap (same order as y).
>
> It sounds like a speculation to me. Why shouldn't we let people to
> copy with an offset any number of bits?
Because it's a common case. You have a value in the register / variable, which
naturally is one of the POD types. Now you want to inject this into bitmap at
the arbitrary offset. Value itself also needs to be variadic size in bits.
Basically what he is trying to achieve is something like bitfield.h API over
bitmaps. Dunno, if actually bitfield.h in the certain driver wouldn't be
enough.
> > Also
> > keep in mind that granularity is long, so less than long it makes no
> > sense.
> >
> > bitmap_copy_from_T(unsigned long *map, start, len, T src),
> >
> > where T is type, start is the offset in map, len is the amount of bits
> > from src starting from 0. That's what is required in most of the cases
> > I believe.
>
> But not in Sebastian's case, according to patch #6.
I think it's a case, see above.
--
With Best Regards,
Andy Shevchenko
Hey Yury and Andy,
thanks for you review and discussion, after some further communication
with Nicolas Dufresne and Andrzej Pietrasiewicz, we came to the
conclusion that I will prepare a more general bit-writer API, which fits
better to the use-case we have on a lot of multimedia hardware.
I'll probably reuse the bitmap API as the backbone of that
implementation and use the `*_set8` to write the content back to memory,
but as we have a few more potential users for such an API, I'll strive
towards creating a new kernel API instead of extending an existing API
that isn't designed for our use-case.
Thanks a lot for your input and ideas.
Greetings,
Sebastian
On 14.07.2022 14:24, Andy Shevchenko wrote:
>On Wed, Jul 13, 2022 at 01:42:17PM -0700, Yury Norov wrote:
>> On Wed, Jul 13, 2022 at 10:14:24PM +0200, Andy Shevchenko wrote:
>> > On Wed, Jul 13, 2022 at 9:44 PM Yury Norov <[email protected]> wrote:
>> > > On Wed, Jul 13, 2022 at 09:10:33PM +0200, Andy Shevchenko wrote:
>> > > > On Wed, Jul 13, 2022 at 8:56 PM Yury Norov <[email protected]> wrote:
>> > > > > On Wed, Jul 13, 2022 at 06:31:59PM +0200, Sebastian Fricke wrote:
>
>...
>
>> > > > > I'd suggest you to try implementing
>> > > > > bitmap_copy_from(dst, src, dst_off, len)
>> > > > > or even
>> > > > > bitmap_copy_from(dst, dst_off, src, src_off, len)
>> > > > > if you expect that you'll need more flexibility in the future.
>> > > >
>> > > > Do you think it would be useful?
>> > > >
>> > > > We have bitmap_replace() & bitmap_remap(). Wouldn't that be enough?
>> > >
>> > > bitmap_replace and bitmap_remap have no an 'offset' parameter.
>> >
>> > True.
>> >
>> > But then it's a bit too generic to have this src_off, no?
>>
>> That's why I said:
>>
>> > > > > if you expect that you'll need more flexibility in the future.
>>
>> My preferred option is bitmap_copy_from(dst, src, dst_off, len).
>>
>> > I would rather expect for asymmetrical bitmaps that the other side
>> > will be either one of the fixed width types (it makes sense to have
>> > for 32- or 64-bit arguments.
>>
>> Look at patch #6 - it copies 1,4,5,9,10,32,37... - pretty much a random
>> number number of bits.
>
>It's too poor randomness, as u64 covers all what in patch 6.
>
>> > When you have a source bitmap of x bits and you would like to copy it
>> > into a y-bit one, I would think that either you have a small amount of
>> > bits in x anyway, or x is a full-sized bitmap (same order as y).
>>
>> It sounds like a speculation to me. Why shouldn't we let people to
>> copy with an offset any number of bits?
>
>Because it's a common case. You have a value in the register / variable, which
>naturally is one of the POD types. Now you want to inject this into bitmap at
>the arbitrary offset. Value itself also needs to be variadic size in bits.
>
>Basically what he is trying to achieve is something like bitfield.h API over
>bitmaps. Dunno, if actually bitfield.h in the certain driver wouldn't be
>enough.
>
>> > Also
>> > keep in mind that granularity is long, so less than long it makes no
>> > sense.
>> >
>> > bitmap_copy_from_T(unsigned long *map, start, len, T src),
>> >
>> > where T is type, start is the offset in map, len is the amount of bits
>> > from src starting from 0. That's what is required in most of the cases
>> > I believe.
>>
>> But not in Sebastian's case, according to patch #6.
>
>I think it's a case, see above.
>
>--
>With Best Regards,
>Andy Shevchenko
>
>