While my first intention was to just add 10-bit HEVC handling, I noticed
a few format handling issues and a bit of redundancy in some cases. Final
result is that driver now sticks to stateless decoder rules better.
Format handling improvements:
1. Default format selection is now based on HW capabilities. Before, MPEG2
was hardcoded but some Cedrus variants don't actually support it.
2. Controls are registered only if related codec is supported by HW.
3. Untiled output format is preferred, if supported, over tiled one. All
display engine cores support untiled format, but only first generation
supports tiled one.
I hope this makes Cedrus eligible for destaging.
Best regards,
Jernej
Jernej Skrabec (11):
media: cedrus: remove superfluous call
media: cedrus: Add format reset helpers
media: cedrus: use helper to set default formats
media: cedrus: Add helper for checking capabilities
media: cedrus: Filter controls based on capability
media: cedrus: set codec ops immediately
media: cedrus: Remove cedrus_codec enum
media: cedrus: prefer untiled capture format
media: cedrus: initialize controls a bit later
media: cedrus: Adjust buffer size based on control values
media: cedrus: h265: Support decoding 10-bit frames
drivers/staging/media/sunxi/cedrus/cedrus.c | 97 +++++-----
drivers/staging/media/sunxi/cedrus/cedrus.h | 22 +--
.../staging/media/sunxi/cedrus/cedrus_dec.c | 4 +-
.../staging/media/sunxi/cedrus/cedrus_h264.c | 2 +-
.../staging/media/sunxi/cedrus/cedrus_h265.c | 37 +++-
.../staging/media/sunxi/cedrus/cedrus_hw.c | 18 +-
.../staging/media/sunxi/cedrus/cedrus_hw.h | 2 +-
.../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 2 +-
.../staging/media/sunxi/cedrus/cedrus_regs.h | 16 ++
.../staging/media/sunxi/cedrus/cedrus_video.c | 166 ++++++++++--------
.../staging/media/sunxi/cedrus/cedrus_video.h | 2 +
.../staging/media/sunxi/cedrus/cedrus_vp8.c | 2 +-
12 files changed, 225 insertions(+), 145 deletions(-)
--
2.38.1
Because not all Cedrus variants supports all codecs, controls should be
registered only if codec related to individual control is supported by
Cedrus.
Replace codec enum, which is not used at all, with capabilities flags
and register control only if capabilities are met.
Signed-off-by: Jernej Skrabec <[email protected]>
---
drivers/staging/media/sunxi/cedrus/cedrus.c | 45 +++++++++++----------
drivers/staging/media/sunxi/cedrus/cedrus.h | 2 +-
2 files changed, 25 insertions(+), 22 deletions(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 2f284a58d787..023566b02dc5 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -77,56 +77,56 @@ static const struct cedrus_control cedrus_controls[] = {
.cfg = {
.id = V4L2_CID_STATELESS_MPEG2_SEQUENCE,
},
- .codec = CEDRUS_CODEC_MPEG2,
+ .capabilities = CEDRUS_CAPABILITY_MPEG2_DEC,
},
{
.cfg = {
.id = V4L2_CID_STATELESS_MPEG2_PICTURE,
},
- .codec = CEDRUS_CODEC_MPEG2,
+ .capabilities = CEDRUS_CAPABILITY_MPEG2_DEC,
},
{
.cfg = {
.id = V4L2_CID_STATELESS_MPEG2_QUANTISATION,
},
- .codec = CEDRUS_CODEC_MPEG2,
+ .capabilities = CEDRUS_CAPABILITY_MPEG2_DEC,
},
{
.cfg = {
.id = V4L2_CID_STATELESS_H264_DECODE_PARAMS,
},
- .codec = CEDRUS_CODEC_H264,
+ .capabilities = CEDRUS_CAPABILITY_H264_DEC,
},
{
.cfg = {
.id = V4L2_CID_STATELESS_H264_SLICE_PARAMS,
},
- .codec = CEDRUS_CODEC_H264,
+ .capabilities = CEDRUS_CAPABILITY_H264_DEC,
},
{
.cfg = {
.id = V4L2_CID_STATELESS_H264_SPS,
.ops = &cedrus_ctrl_ops,
},
- .codec = CEDRUS_CODEC_H264,
+ .capabilities = CEDRUS_CAPABILITY_H264_DEC,
},
{
.cfg = {
.id = V4L2_CID_STATELESS_H264_PPS,
},
- .codec = CEDRUS_CODEC_H264,
+ .capabilities = CEDRUS_CAPABILITY_H264_DEC,
},
{
.cfg = {
.id = V4L2_CID_STATELESS_H264_SCALING_MATRIX,
},
- .codec = CEDRUS_CODEC_H264,
+ .capabilities = CEDRUS_CAPABILITY_H264_DEC,
},
{
.cfg = {
.id = V4L2_CID_STATELESS_H264_PRED_WEIGHTS,
},
- .codec = CEDRUS_CODEC_H264,
+ .capabilities = CEDRUS_CAPABILITY_H264_DEC,
},
{
.cfg = {
@@ -134,7 +134,7 @@ static const struct cedrus_control cedrus_controls[] = {
.max = V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED,
.def = V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED,
},
- .codec = CEDRUS_CODEC_H264,
+ .capabilities = CEDRUS_CAPABILITY_H264_DEC,
},
{
.cfg = {
@@ -142,7 +142,7 @@ static const struct cedrus_control cedrus_controls[] = {
.max = V4L2_STATELESS_H264_START_CODE_NONE,
.def = V4L2_STATELESS_H264_START_CODE_NONE,
},
- .codec = CEDRUS_CODEC_H264,
+ .capabilities = CEDRUS_CAPABILITY_H264_DEC,
},
/*
* We only expose supported profiles information,
@@ -160,20 +160,20 @@ static const struct cedrus_control cedrus_controls[] = {
.menu_skip_mask =
BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
},
- .codec = CEDRUS_CODEC_H264,
+ .capabilities = CEDRUS_CAPABILITY_H264_DEC,
},
{
.cfg = {
.id = V4L2_CID_STATELESS_HEVC_SPS,
.ops = &cedrus_ctrl_ops,
},
- .codec = CEDRUS_CODEC_H265,
+ .capabilities = CEDRUS_CAPABILITY_H265_DEC,
},
{
.cfg = {
.id = V4L2_CID_STATELESS_HEVC_PPS,
},
- .codec = CEDRUS_CODEC_H265,
+ .capabilities = CEDRUS_CAPABILITY_H265_DEC,
},
{
.cfg = {
@@ -181,13 +181,13 @@ static const struct cedrus_control cedrus_controls[] = {
/* The driver can only handle 1 entry per slice for now */
.dims = { 1 },
},
- .codec = CEDRUS_CODEC_H265,
+ .capabilities = CEDRUS_CAPABILITY_H265_DEC,
},
{
.cfg = {
.id = V4L2_CID_STATELESS_HEVC_SCALING_MATRIX,
},
- .codec = CEDRUS_CODEC_H265,
+ .capabilities = CEDRUS_CAPABILITY_H265_DEC,
},
{
.cfg = {
@@ -197,7 +197,7 @@ static const struct cedrus_control cedrus_controls[] = {
.max = 0xffffffff,
.step = 1,
},
- .codec = CEDRUS_CODEC_H265,
+ .capabilities = CEDRUS_CAPABILITY_H265_DEC,
},
{
.cfg = {
@@ -205,7 +205,7 @@ static const struct cedrus_control cedrus_controls[] = {
.max = V4L2_STATELESS_HEVC_DECODE_MODE_SLICE_BASED,
.def = V4L2_STATELESS_HEVC_DECODE_MODE_SLICE_BASED,
},
- .codec = CEDRUS_CODEC_H265,
+ .capabilities = CEDRUS_CAPABILITY_H265_DEC,
},
{
.cfg = {
@@ -213,19 +213,19 @@ static const struct cedrus_control cedrus_controls[] = {
.max = V4L2_STATELESS_HEVC_START_CODE_NONE,
.def = V4L2_STATELESS_HEVC_START_CODE_NONE,
},
- .codec = CEDRUS_CODEC_H265,
+ .capabilities = CEDRUS_CAPABILITY_H265_DEC,
},
{
.cfg = {
.id = V4L2_CID_STATELESS_VP8_FRAME,
},
- .codec = CEDRUS_CODEC_VP8,
+ .capabilities = CEDRUS_CAPABILITY_VP8_DEC,
},
{
.cfg = {
.id = V4L2_CID_STATELESS_HEVC_DECODE_PARAMS,
},
- .codec = CEDRUS_CODEC_H265,
+ .capabilities = CEDRUS_CAPABILITY_H265_DEC,
},
};
@@ -275,6 +275,9 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct cedrus_ctx *ctx)
return -ENOMEM;
for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
+ if (!cedrus_is_capable(ctx, cedrus_controls[i].capabilities))
+ continue;
+
ctrl = v4l2_ctrl_new_custom(hdl, &cedrus_controls[i].cfg,
NULL);
if (hdl->error) {
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
index 1a98790a99af..7a1619967513 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -57,7 +57,7 @@ enum cedrus_h264_pic_type {
struct cedrus_control {
struct v4l2_ctrl_config cfg;
- enum cedrus_codec codec;
+ unsigned int capabilities;
};
struct cedrus_h264_run {
--
2.38.1
Last user of cedrus_codec enum is cedrus_engine_enable() but this
argument is completely redundant. Same information can be obtained via
source pixel format. Let's remove this argument and enum.
No functional changes intended.
Signed-off-by: Jernej Skrabec <[email protected]>
---
drivers/staging/media/sunxi/cedrus/cedrus.h | 8 --------
drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 2 +-
drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 2 +-
drivers/staging/media/sunxi/cedrus/cedrus_hw.c | 12 ++++++------
drivers/staging/media/sunxi/cedrus/cedrus_hw.h | 2 +-
drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c | 2 +-
drivers/staging/media/sunxi/cedrus/cedrus_vp8.c | 2 +-
7 files changed, 11 insertions(+), 19 deletions(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
index 0b082b1fae22..5904294f3108 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -35,14 +35,6 @@
#define CEDRUS_CAPABILITY_VP8_DEC BIT(4)
#define CEDRUS_CAPABILITY_H265_10_DEC BIT(5)
-enum cedrus_codec {
- CEDRUS_CODEC_MPEG2,
- CEDRUS_CODEC_H264,
- CEDRUS_CODEC_H265,
- CEDRUS_CODEC_VP8,
- CEDRUS_CODEC_LAST,
-};
-
enum cedrus_irq_status {
CEDRUS_IRQ_NONE,
CEDRUS_IRQ_ERROR,
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
index c92dec21c1ac..dfb401df138a 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -518,7 +518,7 @@ static int cedrus_h264_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
struct cedrus_dev *dev = ctx->dev;
int ret;
- cedrus_engine_enable(ctx, CEDRUS_CODEC_H264);
+ cedrus_engine_enable(ctx);
cedrus_write(dev, VE_H264_SDROT_CTRL, 0);
cedrus_write(dev, VE_H264_EXTRA_BUFFER1,
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
index 7a438cd22c34..5d3da50ce46a 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
@@ -471,7 +471,7 @@ static int cedrus_h265_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
}
/* Activate H265 engine. */
- cedrus_engine_enable(ctx, CEDRUS_CODEC_H265);
+ cedrus_engine_enable(ctx);
/* Source offset and length in bits. */
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
index c3387cd1e80f..fa86a658fdc6 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
@@ -31,7 +31,7 @@
#include "cedrus_hw.h"
#include "cedrus_regs.h"
-int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec)
+int cedrus_engine_enable(struct cedrus_ctx *ctx)
{
u32 reg = 0;
@@ -42,18 +42,18 @@ int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec)
reg |= VE_MODE_REC_WR_MODE_2MB;
reg |= VE_MODE_DDR_MODE_BW_128;
- switch (codec) {
- case CEDRUS_CODEC_MPEG2:
+ switch (ctx->src_fmt.pixelformat) {
+ case V4L2_PIX_FMT_MPEG2_SLICE:
reg |= VE_MODE_DEC_MPEG;
break;
/* H.264 and VP8 both use the same decoding mode bit. */
- case CEDRUS_CODEC_H264:
- case CEDRUS_CODEC_VP8:
+ case V4L2_PIX_FMT_H264_SLICE:
+ case V4L2_PIX_FMT_VP8_FRAME:
reg |= VE_MODE_DEC_H264;
break;
- case CEDRUS_CODEC_H265:
+ case V4L2_PIX_FMT_HEVC_SLICE:
reg |= VE_MODE_DEC_H265;
break;
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
index 7c92f00e36da..6f1e701b1ea8 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
@@ -16,7 +16,7 @@
#ifndef _CEDRUS_HW_H_
#define _CEDRUS_HW_H_
-int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec);
+int cedrus_engine_enable(struct cedrus_ctx *ctx);
void cedrus_engine_disable(struct cedrus_dev *dev);
void cedrus_dst_format_set(struct cedrus_dev *dev,
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
index c1128d2cd555..10e98f08aafc 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
@@ -66,7 +66,7 @@ static int cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
quantisation = run->mpeg2.quantisation;
/* Activate MPEG engine. */
- cedrus_engine_enable(ctx, CEDRUS_CODEC_MPEG2);
+ cedrus_engine_enable(ctx);
/* Set intra quantisation matrix. */
matrix = quantisation->intra_quantiser_matrix;
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
index f7714baae37d..969677a3bbf9 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
@@ -662,7 +662,7 @@ static int cedrus_vp8_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
int header_size;
u32 reg;
- cedrus_engine_enable(ctx, CEDRUS_CODEC_VP8);
+ cedrus_engine_enable(ctx);
cedrus_write(dev, VE_H264_CTRL, VE_H264_CTRL_VP8);
--
2.38.1
There is several different Cedrus cores with varying capabilities, so
some operations like listing formats depends on checks if feature is
supported or not.
Currently check for capabilities is only in format enumeration helper,
but it will be used also elsewhere later. Let's convert this check to
helper and while at it, also simplify it. There is no need to check if
capability mask is zero, condition will still work properly.
No functional changes intended.
Signed-off-by: Jernej Skrabec <[email protected]>
---
drivers/staging/media/sunxi/cedrus/cedrus.h | 6 ++++++
drivers/staging/media/sunxi/cedrus/cedrus_video.c | 8 +-------
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
index 9cffaf228422..1a98790a99af 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -268,6 +268,12 @@ vb2_to_cedrus_buffer(const struct vb2_buffer *p)
return vb2_v4l2_to_cedrus_buffer(to_vb2_v4l2_buffer(p));
}
+static inline bool
+cedrus_is_capable(struct cedrus_ctx *ctx, unsigned int capabilities)
+{
+ return (ctx->dev->capabilities & capabilities) == capabilities;
+}
+
void *cedrus_find_control_data(struct cedrus_ctx *ctx, u32 id);
u32 cedrus_get_num_of_controls(struct cedrus_ctx *ctx, u32 id);
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index 27a7120fa6fb..04b7b87ef0b7 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -177,19 +177,13 @@ static int cedrus_enum_fmt(struct file *file, struct v4l2_fmtdesc *f,
u32 direction)
{
struct cedrus_ctx *ctx = cedrus_file2ctx(file);
- struct cedrus_dev *dev = ctx->dev;
- unsigned int capabilities = dev->capabilities;
- struct cedrus_format *fmt;
unsigned int i, index;
/* Index among formats that match the requested direction. */
index = 0;
for (i = 0; i < CEDRUS_FORMATS_COUNT; i++) {
- fmt = &cedrus_formats[i];
-
- if (fmt->capabilities && (fmt->capabilities & capabilities) !=
- fmt->capabilities)
+ if (!cedrus_is_capable(ctx, cedrus_formats[i].capabilities))
continue;
if (!(cedrus_formats[i].directions & direction))
--
2.38.1
While it doesn't matter if controls are initialized before or after
queues and formats from open handler standpoint, initializing them last
helps keeping s_ctrl handler simpler, since everything has already valid
values.
This is just preparation for follow up changes. No functional change is
intended.
Signed-off-by: Jernej Skrabec <[email protected]>
---
drivers/staging/media/sunxi/cedrus/cedrus.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 8cfe47574c39..70b07d8bad2b 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -354,27 +354,27 @@ static int cedrus_open(struct file *file)
file->private_data = &ctx->fh;
ctx->dev = dev;
- ret = cedrus_init_ctrls(dev, ctx);
- if (ret)
- goto err_free;
-
ctx->fh.m2m_ctx = v4l2_m2m_ctx_init(dev->m2m_dev, ctx,
&cedrus_queue_init);
if (IS_ERR(ctx->fh.m2m_ctx)) {
ret = PTR_ERR(ctx->fh.m2m_ctx);
- goto err_ctrls;
+ goto err_free;
}
cedrus_reset_out_format(ctx);
+ ret = cedrus_init_ctrls(dev, ctx);
+ if (ret)
+ goto err_m2m_release;
+
v4l2_fh_add(&ctx->fh);
mutex_unlock(&dev->dev_mutex);
return 0;
-err_ctrls:
- v4l2_ctrl_handler_free(&ctx->hdl);
+err_m2m_release:
+ v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
err_free:
kfree(ctx);
mutex_unlock(&dev->dev_mutex);
--
2.38.1
We'll need codec ops soon after output format is set in following
commits. Let's move current codec setup to set output format callback.
While at it, let's remove one level of indirection by changing
current_codec to point to codec ops structure directly.
Signed-off-by: Jernej Skrabec <[email protected]>
---
drivers/staging/media/sunxi/cedrus/cedrus.c | 5 ---
drivers/staging/media/sunxi/cedrus/cedrus.h | 3 +-
.../staging/media/sunxi/cedrus/cedrus_dec.c | 4 +-
.../staging/media/sunxi/cedrus/cedrus_hw.c | 6 +--
.../staging/media/sunxi/cedrus/cedrus_video.c | 44 ++++++++-----------
5 files changed, 25 insertions(+), 37 deletions(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 023566b02dc5..8cfe47574c39 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -455,11 +455,6 @@ static int cedrus_probe(struct platform_device *pdev)
return ret;
}
- dev->dec_ops[CEDRUS_CODEC_MPEG2] = &cedrus_dec_ops_mpeg2;
- dev->dec_ops[CEDRUS_CODEC_H264] = &cedrus_dec_ops_h264;
- dev->dec_ops[CEDRUS_CODEC_H265] = &cedrus_dec_ops_h265;
- dev->dec_ops[CEDRUS_CODEC_VP8] = &cedrus_dec_ops_vp8;
-
mutex_init(&dev->dev_mutex);
INIT_DELAYED_WORK(&dev->watchdog_work, cedrus_watchdog);
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
index 7a1619967513..0b082b1fae22 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -126,7 +126,7 @@ struct cedrus_ctx {
struct v4l2_pix_format src_fmt;
struct v4l2_pix_format dst_fmt;
- enum cedrus_codec current_codec;
+ struct cedrus_dec_ops *current_codec;
struct v4l2_ctrl_handler hdl;
struct v4l2_ctrl **ctrls;
@@ -185,7 +185,6 @@ struct cedrus_dev {
struct platform_device *pdev;
struct device *dev;
struct v4l2_m2m_dev *m2m_dev;
- struct cedrus_dec_ops *dec_ops[CEDRUS_CODEC_LAST];
/* Device file mutex */
struct mutex dev_mutex;
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
index e7f7602a5ab4..fbbf9e6f0f50 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
@@ -94,7 +94,7 @@ void cedrus_device_run(void *priv)
cedrus_dst_format_set(dev, &ctx->dst_fmt);
- error = dev->dec_ops[ctx->current_codec]->setup(ctx, &run);
+ error = ctx->current_codec->setup(ctx, &run);
if (error)
v4l2_err(&ctx->dev->v4l2_dev,
"Failed to setup decoding job: %d\n", error);
@@ -110,7 +110,7 @@ void cedrus_device_run(void *priv)
schedule_delayed_work(&dev->watchdog_work,
msecs_to_jiffies(2000));
- dev->dec_ops[ctx->current_codec]->trigger(ctx);
+ ctx->current_codec->trigger(ctx);
} else {
v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev,
ctx->fh.m2m_ctx,
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
index a6470a89851e..c3387cd1e80f 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
@@ -132,12 +132,12 @@ static irqreturn_t cedrus_irq(int irq, void *data)
return IRQ_NONE;
}
- status = dev->dec_ops[ctx->current_codec]->irq_status(ctx);
+ status = ctx->current_codec->irq_status(ctx);
if (status == CEDRUS_IRQ_NONE)
return IRQ_NONE;
- dev->dec_ops[ctx->current_codec]->irq_disable(ctx);
- dev->dec_ops[ctx->current_codec]->irq_clear(ctx);
+ ctx->current_codec->irq_disable(ctx);
+ ctx->current_codec->irq_clear(ctx);
if (status == CEDRUS_IRQ_ERROR)
state = VB2_BUF_STATE_ERROR;
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index 04b7b87ef0b7..3591bf9d7d9c 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -335,6 +335,21 @@ static int cedrus_s_fmt_vid_out_p(struct cedrus_ctx *ctx,
break;
}
+ switch (ctx->src_fmt.pixelformat) {
+ case V4L2_PIX_FMT_MPEG2_SLICE:
+ ctx->current_codec = &cedrus_dec_ops_mpeg2;
+ break;
+ case V4L2_PIX_FMT_H264_SLICE:
+ ctx->current_codec = &cedrus_dec_ops_h264;
+ break;
+ case V4L2_PIX_FMT_HEVC_SLICE:
+ ctx->current_codec = &cedrus_dec_ops_h265;
+ break;
+ case V4L2_PIX_FMT_VP8_FRAME:
+ ctx->current_codec = &cedrus_dec_ops_vp8;
+ break;
+ }
+
/* Propagate format information to capture. */
ctx->dst_fmt.colorspace = pix_fmt->colorspace;
ctx->dst_fmt.xfer_func = pix_fmt->xfer_func;
@@ -493,34 +508,13 @@ static int cedrus_start_streaming(struct vb2_queue *vq, unsigned int count)
struct cedrus_dev *dev = ctx->dev;
int ret = 0;
- switch (ctx->src_fmt.pixelformat) {
- case V4L2_PIX_FMT_MPEG2_SLICE:
- ctx->current_codec = CEDRUS_CODEC_MPEG2;
- break;
-
- case V4L2_PIX_FMT_H264_SLICE:
- ctx->current_codec = CEDRUS_CODEC_H264;
- break;
-
- case V4L2_PIX_FMT_HEVC_SLICE:
- ctx->current_codec = CEDRUS_CODEC_H265;
- break;
-
- case V4L2_PIX_FMT_VP8_FRAME:
- ctx->current_codec = CEDRUS_CODEC_VP8;
- break;
-
- default:
- return -EINVAL;
- }
-
if (V4L2_TYPE_IS_OUTPUT(vq->type)) {
ret = pm_runtime_resume_and_get(dev->dev);
if (ret < 0)
goto err_cleanup;
- if (dev->dec_ops[ctx->current_codec]->start) {
- ret = dev->dec_ops[ctx->current_codec]->start(ctx);
+ if (ctx->current_codec->start) {
+ ret = ctx->current_codec->start(ctx);
if (ret)
goto err_pm;
}
@@ -542,8 +536,8 @@ static void cedrus_stop_streaming(struct vb2_queue *vq)
struct cedrus_dev *dev = ctx->dev;
if (V4L2_TYPE_IS_OUTPUT(vq->type)) {
- if (dev->dec_ops[ctx->current_codec]->stop)
- dev->dec_ops[ctx->current_codec]->stop(ctx);
+ if (ctx->current_codec->stop)
+ ctx->current_codec->stop(ctx);
pm_runtime_put(dev->dev);
}
--
2.38.1
Now that set output format helper is available, let's use that for
setting default values. Advantage of this is that values will be always
valid. Current code produced invalid default values for V3s SoC, which
doesn't support MPEG2 decoding.
Signed-off-by: Jernej Skrabec <[email protected]>
---
drivers/staging/media/sunxi/cedrus/cedrus.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 55c54dfdc585..2f284a58d787 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -361,16 +361,8 @@ static int cedrus_open(struct file *file)
ret = PTR_ERR(ctx->fh.m2m_ctx);
goto err_ctrls;
}
- ctx->dst_fmt.pixelformat = V4L2_PIX_FMT_NV12_32L32;
- cedrus_prepare_format(&ctx->dst_fmt);
- ctx->src_fmt.pixelformat = V4L2_PIX_FMT_MPEG2_SLICE;
- /*
- * TILED_NV12 has more strict requirements, so copy the width and
- * height to src_fmt to ensure that is matches the dst_fmt resolution.
- */
- ctx->src_fmt.width = ctx->dst_fmt.width;
- ctx->src_fmt.height = ctx->dst_fmt.height;
- cedrus_prepare_format(&ctx->src_fmt);
+
+ cedrus_reset_out_format(ctx);
v4l2_fh_add(&ctx->fh);
--
2.38.1
By re-arranging try format handlers and set out format handler, we can
easily implement reset out/cap format helpers.
There is only one subtle, but important functional change. Capture
pixel format will be reset each time output format is set. This is
actually expected behaviour per stateless decoder API.
Signed-off-by: Jernej Skrabec <[email protected]>
---
.../staging/media/sunxi/cedrus/cedrus_video.c | 100 +++++++++++-------
.../staging/media/sunxi/cedrus/cedrus_video.h | 2 +
2 files changed, 66 insertions(+), 36 deletions(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index 1c3c1d080d31..27a7120fa6fb 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -241,12 +241,10 @@ static int cedrus_g_fmt_vid_out(struct file *file, void *priv,
return 0;
}
-static int cedrus_try_fmt_vid_cap(struct file *file, void *priv,
- struct v4l2_format *f)
+static int cedrus_try_fmt_vid_cap_p(struct cedrus_ctx *ctx,
+ struct v4l2_pix_format *pix_fmt)
{
- struct cedrus_ctx *ctx = cedrus_file2ctx(file);
struct cedrus_dev *dev = ctx->dev;
- struct v4l2_pix_format *pix_fmt = &f->fmt.pix;
struct cedrus_format *fmt =
cedrus_find_format(pix_fmt->pixelformat, CEDRUS_DECODE_DST,
dev->capabilities);
@@ -262,12 +260,16 @@ static int cedrus_try_fmt_vid_cap(struct file *file, void *priv,
return 0;
}
-static int cedrus_try_fmt_vid_out(struct file *file, void *priv,
+static int cedrus_try_fmt_vid_cap(struct file *file, void *priv,
struct v4l2_format *f)
{
- struct cedrus_ctx *ctx = cedrus_file2ctx(file);
+ return cedrus_try_fmt_vid_cap_p(cedrus_file2ctx(file), &f->fmt.pix);
+}
+
+static int cedrus_try_fmt_vid_out_p(struct cedrus_ctx *ctx,
+ struct v4l2_pix_format *pix_fmt)
+{
struct cedrus_dev *dev = ctx->dev;
- struct v4l2_pix_format *pix_fmt = &f->fmt.pix;
struct cedrus_format *fmt =
cedrus_find_format(pix_fmt->pixelformat, CEDRUS_DECODE_SRC,
dev->capabilities);
@@ -281,6 +283,12 @@ static int cedrus_try_fmt_vid_out(struct file *file, void *priv,
return 0;
}
+static int cedrus_try_fmt_vid_out(struct file *file, void *priv,
+ struct v4l2_format *f)
+{
+ return cedrus_try_fmt_vid_out_p(cedrus_file2ctx(file), &f->fmt.pix);
+}
+
static int cedrus_s_fmt_vid_cap(struct file *file, void *priv,
struct v4l2_format *f)
{
@@ -301,13 +309,60 @@ static int cedrus_s_fmt_vid_cap(struct file *file, void *priv,
return 0;
}
+void cedrus_reset_cap_format(struct cedrus_ctx *ctx)
+{
+ ctx->dst_fmt.pixelformat = 0;
+ cedrus_try_fmt_vid_cap_p(ctx, &ctx->dst_fmt);
+}
+
+static int cedrus_s_fmt_vid_out_p(struct cedrus_ctx *ctx,
+ struct v4l2_pix_format *pix_fmt)
+{
+ struct vb2_queue *vq;
+ int ret;
+
+ ret = cedrus_try_fmt_vid_out_p(ctx, pix_fmt);
+ if (ret)
+ return ret;
+
+ ctx->src_fmt = *pix_fmt;
+
+ vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
+
+ switch (ctx->src_fmt.pixelformat) {
+ case V4L2_PIX_FMT_H264_SLICE:
+ case V4L2_PIX_FMT_HEVC_SLICE:
+ vq->subsystem_flags |=
+ VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
+ break;
+ default:
+ vq->subsystem_flags &=
+ ~VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
+ break;
+ }
+
+ /* Propagate format information to capture. */
+ ctx->dst_fmt.colorspace = pix_fmt->colorspace;
+ ctx->dst_fmt.xfer_func = pix_fmt->xfer_func;
+ ctx->dst_fmt.ycbcr_enc = pix_fmt->ycbcr_enc;
+ ctx->dst_fmt.quantization = pix_fmt->quantization;
+ cedrus_reset_cap_format(ctx);
+
+ return 0;
+}
+
+void cedrus_reset_out_format(struct cedrus_ctx *ctx)
+{
+ ctx->src_fmt.pixelformat = 0;
+ cedrus_s_fmt_vid_out_p(ctx, &ctx->src_fmt);
+}
+
static int cedrus_s_fmt_vid_out(struct file *file, void *priv,
struct v4l2_format *f)
{
struct cedrus_ctx *ctx = cedrus_file2ctx(file);
struct vb2_queue *vq;
struct vb2_queue *peer_vq;
- int ret;
vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
/*
@@ -328,34 +383,7 @@ static int cedrus_s_fmt_vid_out(struct file *file, void *priv,
if (vb2_is_busy(peer_vq))
return -EBUSY;
- ret = cedrus_try_fmt_vid_out(file, priv, f);
- if (ret)
- return ret;
-
- ctx->src_fmt = f->fmt.pix;
-
- switch (ctx->src_fmt.pixelformat) {
- case V4L2_PIX_FMT_H264_SLICE:
- case V4L2_PIX_FMT_HEVC_SLICE:
- vq->subsystem_flags |=
- VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
- break;
- default:
- vq->subsystem_flags &=
- ~VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
- break;
- }
-
- /* Propagate format information to capture. */
- ctx->dst_fmt.colorspace = f->fmt.pix.colorspace;
- ctx->dst_fmt.xfer_func = f->fmt.pix.xfer_func;
- ctx->dst_fmt.ycbcr_enc = f->fmt.pix.ycbcr_enc;
- ctx->dst_fmt.quantization = f->fmt.pix.quantization;
- ctx->dst_fmt.width = ctx->src_fmt.width;
- ctx->dst_fmt.height = ctx->src_fmt.height;
- cedrus_prepare_format(&ctx->dst_fmt);
-
- return 0;
+ return cedrus_s_fmt_vid_out_p(cedrus_file2ctx(file), &f->fmt.pix);
}
const struct v4l2_ioctl_ops cedrus_ioctl_ops = {
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.h b/drivers/staging/media/sunxi/cedrus/cedrus_video.h
index 05050c0a0921..8e1afc16a6a1 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.h
@@ -27,5 +27,7 @@ extern const struct v4l2_ioctl_ops cedrus_ioctl_ops;
int cedrus_queue_init(void *priv, struct vb2_queue *src_vq,
struct vb2_queue *dst_vq);
void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt);
+void cedrus_reset_cap_format(struct cedrus_ctx *ctx);
+void cedrus_reset_out_format(struct cedrus_ctx *ctx);
#endif
--
2.38.1
cedrus_try_fmt_vid_out() is called two times inside
cedrus_s_fmt_vid_out(), but nothing changes between calls which would
influence output format. Remove first call, which was added later.
Signed-off-by: Jernej Skrabec <[email protected]>
---
drivers/staging/media/sunxi/cedrus/cedrus_video.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index 33726175d980..1c3c1d080d31 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -309,10 +309,6 @@ static int cedrus_s_fmt_vid_out(struct file *file, void *priv,
struct vb2_queue *peer_vq;
int ret;
- ret = cedrus_try_fmt_vid_out(file, priv, f);
- if (ret)
- return ret;
-
vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
/*
* In order to support dynamic resolution change,
--
2.38.1
While all generations of display engine on Allwinner SoCs support
untiled format, only first generation supports tiled format. Let's
move untiled format up, so it can be picked before tiled one. If
Cedrus variant doesn't support untiled format, tiled will still be
picked as default format.
Signed-off-by: Jernej Skrabec <[email protected]>
---
drivers/staging/media/sunxi/cedrus/cedrus_video.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index 3591bf9d7d9c..f9f723ea3f79 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -56,13 +56,13 @@ static struct cedrus_format cedrus_formats[] = {
.capabilities = CEDRUS_CAPABILITY_VP8_DEC,
},
{
- .pixelformat = V4L2_PIX_FMT_NV12_32L32,
+ .pixelformat = V4L2_PIX_FMT_NV12,
.directions = CEDRUS_DECODE_DST,
+ .capabilities = CEDRUS_CAPABILITY_UNTILED,
},
{
- .pixelformat = V4L2_PIX_FMT_NV12,
+ .pixelformat = V4L2_PIX_FMT_NV12_32L32,
.directions = CEDRUS_DECODE_DST,
- .capabilities = CEDRUS_CAPABILITY_UNTILED,
},
};
--
2.38.1
In some cases decoding engine needs extra space in capture buffers. This
is the case for decoding 10-bit HEVC frames into 8-bit capture format.
This commit only adds infrastructure for such cases.
Signed-off-by: Jernej Skrabec <[email protected]>
---
drivers/staging/media/sunxi/cedrus/cedrus.c | 14 ++++++++++++++
drivers/staging/media/sunxi/cedrus/cedrus.h | 2 ++
drivers/staging/media/sunxi/cedrus/cedrus_video.c | 4 ++++
3 files changed, 20 insertions(+)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 70b07d8bad2b..fbe3b2e7c1d4 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -68,8 +68,22 @@ static int cedrus_try_ctrl(struct v4l2_ctrl *ctrl)
return 0;
}
+static int cedrus_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct cedrus_ctx *ctx = container_of(ctrl->handler,
+ struct cedrus_ctx, hdl);
+ struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
+ V4L2_BUF_TYPE_VIDEO_CAPTURE);
+
+ if (!vb2_is_busy(vq) && !vb2_is_streaming(vq))
+ cedrus_reset_cap_format(ctx);
+
+ return 0;
+}
+
static const struct v4l2_ctrl_ops cedrus_ctrl_ops = {
.try_ctrl = cedrus_try_ctrl,
+ .s_ctrl = cedrus_s_ctrl,
};
static const struct cedrus_control cedrus_controls[] = {
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
index 5904294f3108..774fe8048ce3 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -162,6 +162,8 @@ struct cedrus_dec_ops {
int (*start)(struct cedrus_ctx *ctx);
void (*stop)(struct cedrus_ctx *ctx);
void (*trigger)(struct cedrus_ctx *ctx);
+ unsigned int (*extra_cap_size)(struct cedrus_ctx *ctx,
+ struct v4l2_pix_format *pix_fmt);
};
struct cedrus_variant {
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index f9f723ea3f79..53e65f74046b 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -251,6 +251,10 @@ static int cedrus_try_fmt_vid_cap_p(struct cedrus_ctx *ctx,
pix_fmt->height = ctx->src_fmt.height;
cedrus_prepare_format(pix_fmt);
+ if (ctx->current_codec->extra_cap_size)
+ pix_fmt->sizeimage +=
+ ctx->current_codec->extra_cap_size(ctx, pix_fmt);
+
return 0;
}
--
2.38.1
On Mon, Oct 24, 2022 at 10:15:08PM +0200, Jernej Skrabec wrote:
> There is several different Cedrus cores with varying capabilities, so
> some operations like listing formats depends on checks if feature is
> supported or not.
>
> Currently check for capabilities is only in format enumeration helper,
> but it will be used also elsewhere later. Let's convert this check to
> helper and while at it, also simplify it. There is no need to check if
> capability mask is zero, condition will still work properly.
Sure. That's true. Out of curiousity, can cedrus_formats[i].capabilities
be zero? Because it feels like that's what should be checked.
regards,
dan carpenter
This patchset has some nice changes and I like how the commit messages
really answered the review questions I had.
regards,
dan carpenter
Dne torek, 25. oktober 2022 ob 08:30:28 CEST je Dan Carpenter napisal(a):
> On Mon, Oct 24, 2022 at 10:15:08PM +0200, Jernej Skrabec wrote:
> > There is several different Cedrus cores with varying capabilities, so
> > some operations like listing formats depends on checks if feature is
> > supported or not.
> >
> > Currently check for capabilities is only in format enumeration helper,
> > but it will be used also elsewhere later. Let's convert this check to
> > helper and while at it, also simplify it. There is no need to check if
> > capability mask is zero, condition will still work properly.
>
> Sure. That's true. Out of curiousity, can cedrus_formats[i].capabilities
> be zero? Because it feels like that's what should be checked.
Yes, it can be. It's the case for V4L2_PIX_FMT_NV12_32L32. All variants
supports it, so there is no special capability needed in order to be listed.
What would you check in such case? Condition still works for this case.
Best regards,
Jernej
Hi Jernej,
On Mon 24 Oct 22, 22:15, Jernej Skrabec wrote:
> While all generations of display engine on Allwinner SoCs support
> untiled format, only first generation supports tiled format. Let's
> move untiled format up, so it can be picked before tiled one. If
> Cedrus variant doesn't support untiled format, tiled will still be
> picked as default format.
Makes sense to me. Of course the order shouldn't matter to smart-enough
userspace but it doesn't hurt to serve the most generic case first.
Acked-by: Paul Kocialkowski <[email protected]>
Cheers,
Paul
> Signed-off-by: Jernej Skrabec <[email protected]>
> ---
> drivers/staging/media/sunxi/cedrus/cedrus_video.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> index 3591bf9d7d9c..f9f723ea3f79 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> @@ -56,13 +56,13 @@ static struct cedrus_format cedrus_formats[] = {
> .capabilities = CEDRUS_CAPABILITY_VP8_DEC,
> },
> {
> - .pixelformat = V4L2_PIX_FMT_NV12_32L32,
> + .pixelformat = V4L2_PIX_FMT_NV12,
> .directions = CEDRUS_DECODE_DST,
> + .capabilities = CEDRUS_CAPABILITY_UNTILED,
> },
> {
> - .pixelformat = V4L2_PIX_FMT_NV12,
> + .pixelformat = V4L2_PIX_FMT_NV12_32L32,
> .directions = CEDRUS_DECODE_DST,
> - .capabilities = CEDRUS_CAPABILITY_UNTILED,
> },
> };
>
> --
> 2.38.1
>
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Hi Jernej,
On Tue 25 Oct 22, 17:17, Jernej Škrabec wrote:
> Dne torek, 25. oktober 2022 ob 08:30:28 CEST je Dan Carpenter napisal(a):
> > On Mon, Oct 24, 2022 at 10:15:08PM +0200, Jernej Skrabec wrote:
> > > There is several different Cedrus cores with varying capabilities, so
> > > some operations like listing formats depends on checks if feature is
> > > supported or not.
> > >
> > > Currently check for capabilities is only in format enumeration helper,
> > > but it will be used also elsewhere later. Let's convert this check to
> > > helper and while at it, also simplify it. There is no need to check if
> > > capability mask is zero, condition will still work properly.
> >
> > Sure. That's true. Out of curiousity, can cedrus_formats[i].capabilities
> > be zero? Because it feels like that's what should be checked.
>
> Yes, it can be. It's the case for V4L2_PIX_FMT_NV12_32L32. All variants
> supports it, so there is no special capability needed in order to be listed.
> What would you check in such case? Condition still works for this case.
I think the problem is that (bits & 0) == 0 is always true.
So if the input caps are 0, we need to make sure to return false.
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Dne torek, 25. oktober 2022 ob 17:05:16 CEST je Paul Kocialkowski napisal(a):
> Hi Jernej,
>
> On Mon 24 Oct 22, 22:15, Jernej Skrabec wrote:
> > While all generations of display engine on Allwinner SoCs support
> > untiled format, only first generation supports tiled format. Let's
> > move untiled format up, so it can be picked before tiled one. If
> > Cedrus variant doesn't support untiled format, tiled will still be
> > picked as default format.
>
> Makes sense to me. Of course the order shouldn't matter to smart-enough
> userspace but it doesn't hurt to serve the most generic case first.
Per Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst driver
should select most appropriate format for current platform by default. Setting
capture format is optional by aforementioned document.
Best regards,
Jernej
>
> Acked-by: Paul Kocialkowski <[email protected]>
>
> Cheers,
>
> Paul
>
> > Signed-off-by: Jernej Skrabec <[email protected]>
> > ---
> >
> > drivers/staging/media/sunxi/cedrus/cedrus_video.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus_video.c index
> > 3591bf9d7d9c..f9f723ea3f79 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> > @@ -56,13 +56,13 @@ static struct cedrus_format cedrus_formats[] = {
> >
> > .capabilities = CEDRUS_CAPABILITY_VP8_DEC,
> >
> > },
> > {
> >
> > - .pixelformat = V4L2_PIX_FMT_NV12_32L32,
> > + .pixelformat = V4L2_PIX_FMT_NV12,
> >
> > .directions = CEDRUS_DECODE_DST,
> >
> > + .capabilities = CEDRUS_CAPABILITY_UNTILED,
> >
> > },
> > {
> >
> > - .pixelformat = V4L2_PIX_FMT_NV12,
> > + .pixelformat = V4L2_PIX_FMT_NV12_32L32,
> >
> > .directions = CEDRUS_DECODE_DST,
> >
> > - .capabilities = CEDRUS_CAPABILITY_UNTILED,
> >
> > },
> >
> > };
Hi Jernej,
On Mon 24 Oct 22, 22:15, Jernej Skrabec wrote:
> We'll need codec ops soon after output format is set in following
> commits. Let's move current codec setup to set output format callback.
> While at it, let's remove one level of indirection by changing
> current_codec to point to codec ops structure directly.
This looks much better, thanks!
Acked-by: Paul Kocialkowski <[email protected]>
Cheers,
Paul
> Signed-off-by: Jernej Skrabec <[email protected]>
> ---
> drivers/staging/media/sunxi/cedrus/cedrus.c | 5 ---
> drivers/staging/media/sunxi/cedrus/cedrus.h | 3 +-
> .../staging/media/sunxi/cedrus/cedrus_dec.c | 4 +-
> .../staging/media/sunxi/cedrus/cedrus_hw.c | 6 +--
> .../staging/media/sunxi/cedrus/cedrus_video.c | 44 ++++++++-----------
> 5 files changed, 25 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
> index 023566b02dc5..8cfe47574c39 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -455,11 +455,6 @@ static int cedrus_probe(struct platform_device *pdev)
> return ret;
> }
>
> - dev->dec_ops[CEDRUS_CODEC_MPEG2] = &cedrus_dec_ops_mpeg2;
> - dev->dec_ops[CEDRUS_CODEC_H264] = &cedrus_dec_ops_h264;
> - dev->dec_ops[CEDRUS_CODEC_H265] = &cedrus_dec_ops_h265;
> - dev->dec_ops[CEDRUS_CODEC_VP8] = &cedrus_dec_ops_vp8;
> -
> mutex_init(&dev->dev_mutex);
>
> INIT_DELAYED_WORK(&dev->watchdog_work, cedrus_watchdog);
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> index 7a1619967513..0b082b1fae22 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -126,7 +126,7 @@ struct cedrus_ctx {
>
> struct v4l2_pix_format src_fmt;
> struct v4l2_pix_format dst_fmt;
> - enum cedrus_codec current_codec;
> + struct cedrus_dec_ops *current_codec;
>
> struct v4l2_ctrl_handler hdl;
> struct v4l2_ctrl **ctrls;
> @@ -185,7 +185,6 @@ struct cedrus_dev {
> struct platform_device *pdev;
> struct device *dev;
> struct v4l2_m2m_dev *m2m_dev;
> - struct cedrus_dec_ops *dec_ops[CEDRUS_CODEC_LAST];
>
> /* Device file mutex */
> struct mutex dev_mutex;
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> index e7f7602a5ab4..fbbf9e6f0f50 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> @@ -94,7 +94,7 @@ void cedrus_device_run(void *priv)
>
> cedrus_dst_format_set(dev, &ctx->dst_fmt);
>
> - error = dev->dec_ops[ctx->current_codec]->setup(ctx, &run);
> + error = ctx->current_codec->setup(ctx, &run);
> if (error)
> v4l2_err(&ctx->dev->v4l2_dev,
> "Failed to setup decoding job: %d\n", error);
> @@ -110,7 +110,7 @@ void cedrus_device_run(void *priv)
> schedule_delayed_work(&dev->watchdog_work,
> msecs_to_jiffies(2000));
>
> - dev->dec_ops[ctx->current_codec]->trigger(ctx);
> + ctx->current_codec->trigger(ctx);
> } else {
> v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev,
> ctx->fh.m2m_ctx,
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> index a6470a89851e..c3387cd1e80f 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> @@ -132,12 +132,12 @@ static irqreturn_t cedrus_irq(int irq, void *data)
> return IRQ_NONE;
> }
>
> - status = dev->dec_ops[ctx->current_codec]->irq_status(ctx);
> + status = ctx->current_codec->irq_status(ctx);
> if (status == CEDRUS_IRQ_NONE)
> return IRQ_NONE;
>
> - dev->dec_ops[ctx->current_codec]->irq_disable(ctx);
> - dev->dec_ops[ctx->current_codec]->irq_clear(ctx);
> + ctx->current_codec->irq_disable(ctx);
> + ctx->current_codec->irq_clear(ctx);
>
> if (status == CEDRUS_IRQ_ERROR)
> state = VB2_BUF_STATE_ERROR;
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> index 04b7b87ef0b7..3591bf9d7d9c 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> @@ -335,6 +335,21 @@ static int cedrus_s_fmt_vid_out_p(struct cedrus_ctx *ctx,
> break;
> }
>
> + switch (ctx->src_fmt.pixelformat) {
> + case V4L2_PIX_FMT_MPEG2_SLICE:
> + ctx->current_codec = &cedrus_dec_ops_mpeg2;
> + break;
> + case V4L2_PIX_FMT_H264_SLICE:
> + ctx->current_codec = &cedrus_dec_ops_h264;
> + break;
> + case V4L2_PIX_FMT_HEVC_SLICE:
> + ctx->current_codec = &cedrus_dec_ops_h265;
> + break;
> + case V4L2_PIX_FMT_VP8_FRAME:
> + ctx->current_codec = &cedrus_dec_ops_vp8;
> + break;
> + }
> +
> /* Propagate format information to capture. */
> ctx->dst_fmt.colorspace = pix_fmt->colorspace;
> ctx->dst_fmt.xfer_func = pix_fmt->xfer_func;
> @@ -493,34 +508,13 @@ static int cedrus_start_streaming(struct vb2_queue *vq, unsigned int count)
> struct cedrus_dev *dev = ctx->dev;
> int ret = 0;
>
> - switch (ctx->src_fmt.pixelformat) {
> - case V4L2_PIX_FMT_MPEG2_SLICE:
> - ctx->current_codec = CEDRUS_CODEC_MPEG2;
> - break;
> -
> - case V4L2_PIX_FMT_H264_SLICE:
> - ctx->current_codec = CEDRUS_CODEC_H264;
> - break;
> -
> - case V4L2_PIX_FMT_HEVC_SLICE:
> - ctx->current_codec = CEDRUS_CODEC_H265;
> - break;
> -
> - case V4L2_PIX_FMT_VP8_FRAME:
> - ctx->current_codec = CEDRUS_CODEC_VP8;
> - break;
> -
> - default:
> - return -EINVAL;
> - }
> -
> if (V4L2_TYPE_IS_OUTPUT(vq->type)) {
> ret = pm_runtime_resume_and_get(dev->dev);
> if (ret < 0)
> goto err_cleanup;
>
> - if (dev->dec_ops[ctx->current_codec]->start) {
> - ret = dev->dec_ops[ctx->current_codec]->start(ctx);
> + if (ctx->current_codec->start) {
> + ret = ctx->current_codec->start(ctx);
> if (ret)
> goto err_pm;
> }
> @@ -542,8 +536,8 @@ static void cedrus_stop_streaming(struct vb2_queue *vq)
> struct cedrus_dev *dev = ctx->dev;
>
> if (V4L2_TYPE_IS_OUTPUT(vq->type)) {
> - if (dev->dec_ops[ctx->current_codec]->stop)
> - dev->dec_ops[ctx->current_codec]->stop(ctx);
> + if (ctx->current_codec->stop)
> + ctx->current_codec->stop(ctx);
>
> pm_runtime_put(dev->dev);
> }
> --
> 2.38.1
>
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
On Tue 25 Oct 22, 17:28, Jernej Škrabec wrote:
> Dne torek, 25. oktober 2022 ob 17:22:59 CEST je Paul Kocialkowski napisal(a):
> > Hi Jernej,
> >
> > On Tue 25 Oct 22, 17:17, Jernej Škrabec wrote:
> > > Dne torek, 25. oktober 2022 ob 08:30:28 CEST je Dan Carpenter napisal(a):
> > > > On Mon, Oct 24, 2022 at 10:15:08PM +0200, Jernej Skrabec wrote:
> > > > > There is several different Cedrus cores with varying capabilities, so
> > > > > some operations like listing formats depends on checks if feature is
> > > > > supported or not.
> > > > >
> > > > > Currently check for capabilities is only in format enumeration helper,
> > > > > but it will be used also elsewhere later. Let's convert this check to
> > > > > helper and while at it, also simplify it. There is no need to check if
> > > > > capability mask is zero, condition will still work properly.
> > > >
> > > > Sure. That's true. Out of curiousity, can
> > > > cedrus_formats[i].capabilities
> > > > be zero? Because it feels like that's what should be checked.
> > >
> > > Yes, it can be. It's the case for V4L2_PIX_FMT_NV12_32L32. All variants
> > > supports it, so there is no special capability needed in order to be
> > > listed. What would you check in such case? Condition still works for this
> > > case.
> > I think the problem is that (bits & 0) == 0 is always true.
> > So if the input caps are 0, we need to make sure to return false.
>
> No. If format (or any other) capabilities are 0, means they are supported by
> all variants and it's expected from cedrus_is_capable() to return true.
Mhh, yeah. Not sure what I was thinking. Sorry for the noise.
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Hi,
On Mon 24 Oct 22, 22:15, Jernej Skrabec wrote:
> Because not all Cedrus variants supports all codecs, controls should be
> registered only if codec related to individual control is supported by
> Cedrus.
>
> Replace codec enum, which is not used at all, with capabilities flags
> and register control only if capabilities are met.
>
> Signed-off-by: Jernej Skrabec <[email protected]>
Acked-by: Paul Kocialkowski <[email protected]>
Cheers,
Paul
> ---
> drivers/staging/media/sunxi/cedrus/cedrus.c | 45 +++++++++++----------
> drivers/staging/media/sunxi/cedrus/cedrus.h | 2 +-
> 2 files changed, 25 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
> index 2f284a58d787..023566b02dc5 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -77,56 +77,56 @@ static const struct cedrus_control cedrus_controls[] = {
> .cfg = {
> .id = V4L2_CID_STATELESS_MPEG2_SEQUENCE,
> },
> - .codec = CEDRUS_CODEC_MPEG2,
> + .capabilities = CEDRUS_CAPABILITY_MPEG2_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_MPEG2_PICTURE,
> },
> - .codec = CEDRUS_CODEC_MPEG2,
> + .capabilities = CEDRUS_CAPABILITY_MPEG2_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_MPEG2_QUANTISATION,
> },
> - .codec = CEDRUS_CODEC_MPEG2,
> + .capabilities = CEDRUS_CAPABILITY_MPEG2_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_H264_DECODE_PARAMS,
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_H264_SLICE_PARAMS,
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_H264_SPS,
> .ops = &cedrus_ctrl_ops,
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_H264_PPS,
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_H264_SCALING_MATRIX,
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_H264_PRED_WEIGHTS,
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> {
> .cfg = {
> @@ -134,7 +134,7 @@ static const struct cedrus_control cedrus_controls[] = {
> .max = V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED,
> .def = V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED,
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> {
> .cfg = {
> @@ -142,7 +142,7 @@ static const struct cedrus_control cedrus_controls[] = {
> .max = V4L2_STATELESS_H264_START_CODE_NONE,
> .def = V4L2_STATELESS_H264_START_CODE_NONE,
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> /*
> * We only expose supported profiles information,
> @@ -160,20 +160,20 @@ static const struct cedrus_control cedrus_controls[] = {
> .menu_skip_mask =
> BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_HEVC_SPS,
> .ops = &cedrus_ctrl_ops,
> },
> - .codec = CEDRUS_CODEC_H265,
> + .capabilities = CEDRUS_CAPABILITY_H265_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_HEVC_PPS,
> },
> - .codec = CEDRUS_CODEC_H265,
> + .capabilities = CEDRUS_CAPABILITY_H265_DEC,
> },
> {
> .cfg = {
> @@ -181,13 +181,13 @@ static const struct cedrus_control cedrus_controls[] = {
> /* The driver can only handle 1 entry per slice for now */
> .dims = { 1 },
> },
> - .codec = CEDRUS_CODEC_H265,
> + .capabilities = CEDRUS_CAPABILITY_H265_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_HEVC_SCALING_MATRIX,
> },
> - .codec = CEDRUS_CODEC_H265,
> + .capabilities = CEDRUS_CAPABILITY_H265_DEC,
> },
> {
> .cfg = {
> @@ -197,7 +197,7 @@ static const struct cedrus_control cedrus_controls[] = {
> .max = 0xffffffff,
> .step = 1,
> },
> - .codec = CEDRUS_CODEC_H265,
> + .capabilities = CEDRUS_CAPABILITY_H265_DEC,
> },
> {
> .cfg = {
> @@ -205,7 +205,7 @@ static const struct cedrus_control cedrus_controls[] = {
> .max = V4L2_STATELESS_HEVC_DECODE_MODE_SLICE_BASED,
> .def = V4L2_STATELESS_HEVC_DECODE_MODE_SLICE_BASED,
> },
> - .codec = CEDRUS_CODEC_H265,
> + .capabilities = CEDRUS_CAPABILITY_H265_DEC,
> },
> {
> .cfg = {
> @@ -213,19 +213,19 @@ static const struct cedrus_control cedrus_controls[] = {
> .max = V4L2_STATELESS_HEVC_START_CODE_NONE,
> .def = V4L2_STATELESS_HEVC_START_CODE_NONE,
> },
> - .codec = CEDRUS_CODEC_H265,
> + .capabilities = CEDRUS_CAPABILITY_H265_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_VP8_FRAME,
> },
> - .codec = CEDRUS_CODEC_VP8,
> + .capabilities = CEDRUS_CAPABILITY_VP8_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_HEVC_DECODE_PARAMS,
> },
> - .codec = CEDRUS_CODEC_H265,
> + .capabilities = CEDRUS_CAPABILITY_H265_DEC,
> },
> };
>
> @@ -275,6 +275,9 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev, struct cedrus_ctx *ctx)
> return -ENOMEM;
>
> for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
> + if (!cedrus_is_capable(ctx, cedrus_controls[i].capabilities))
> + continue;
> +
> ctrl = v4l2_ctrl_new_custom(hdl, &cedrus_controls[i].cfg,
> NULL);
> if (hdl->error) {
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> index 1a98790a99af..7a1619967513 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -57,7 +57,7 @@ enum cedrus_h264_pic_type {
>
> struct cedrus_control {
> struct v4l2_ctrl_config cfg;
> - enum cedrus_codec codec;
> + unsigned int capabilities;
> };
>
> struct cedrus_h264_run {
> --
> 2.38.1
>
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Dne torek, 25. oktober 2022 ob 17:22:59 CEST je Paul Kocialkowski napisal(a):
> Hi Jernej,
>
> On Tue 25 Oct 22, 17:17, Jernej Škrabec wrote:
> > Dne torek, 25. oktober 2022 ob 08:30:28 CEST je Dan Carpenter napisal(a):
> > > On Mon, Oct 24, 2022 at 10:15:08PM +0200, Jernej Skrabec wrote:
> > > > There is several different Cedrus cores with varying capabilities, so
> > > > some operations like listing formats depends on checks if feature is
> > > > supported or not.
> > > >
> > > > Currently check for capabilities is only in format enumeration helper,
> > > > but it will be used also elsewhere later. Let's convert this check to
> > > > helper and while at it, also simplify it. There is no need to check if
> > > > capability mask is zero, condition will still work properly.
> > >
> > > Sure. That's true. Out of curiousity, can
> > > cedrus_formats[i].capabilities
> > > be zero? Because it feels like that's what should be checked.
> >
> > Yes, it can be. It's the case for V4L2_PIX_FMT_NV12_32L32. All variants
> > supports it, so there is no special capability needed in order to be
> > listed. What would you check in such case? Condition still works for this
> > case.
> I think the problem is that (bits & 0) == 0 is always true.
> So if the input caps are 0, we need to make sure to return false.
No. If format (or any other) capabilities are 0, means they are supported by
all variants and it's expected from cedrus_is_capable() to return true.
Regards,
Jernej
Hi Jernej,
On Mon 24 Oct 22, 22:15, Jernej Skrabec wrote:
> Last user of cedrus_codec enum is cedrus_engine_enable() but this
> argument is completely redundant. Same information can be obtained via
> source pixel format. Let's remove this argument and enum.
>
> No functional changes intended.
Looks good to me!
Acked-by: Paul Kocialkowski <[email protected]>
See a suggestion below but out of the scope of this patch.
> Signed-off-by: Jernej Skrabec <[email protected]>
> ---
> drivers/staging/media/sunxi/cedrus/cedrus.h | 8 --------
> drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 2 +-
> drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 2 +-
> drivers/staging/media/sunxi/cedrus/cedrus_hw.c | 12 ++++++------
> drivers/staging/media/sunxi/cedrus/cedrus_hw.h | 2 +-
> drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c | 2 +-
> drivers/staging/media/sunxi/cedrus/cedrus_vp8.c | 2 +-
> 7 files changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> index 0b082b1fae22..5904294f3108 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -35,14 +35,6 @@
> #define CEDRUS_CAPABILITY_VP8_DEC BIT(4)
> #define CEDRUS_CAPABILITY_H265_10_DEC BIT(5)
>
> -enum cedrus_codec {
> - CEDRUS_CODEC_MPEG2,
> - CEDRUS_CODEC_H264,
> - CEDRUS_CODEC_H265,
> - CEDRUS_CODEC_VP8,
> - CEDRUS_CODEC_LAST,
> -};
> -
> enum cedrus_irq_status {
> CEDRUS_IRQ_NONE,
> CEDRUS_IRQ_ERROR,
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> index c92dec21c1ac..dfb401df138a 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> @@ -518,7 +518,7 @@ static int cedrus_h264_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
> struct cedrus_dev *dev = ctx->dev;
> int ret;
>
> - cedrus_engine_enable(ctx, CEDRUS_CODEC_H264);
> + cedrus_engine_enable(ctx);
>
> cedrus_write(dev, VE_H264_SDROT_CTRL, 0);
> cedrus_write(dev, VE_H264_EXTRA_BUFFER1,
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> index 7a438cd22c34..5d3da50ce46a 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> @@ -471,7 +471,7 @@ static int cedrus_h265_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
> }
>
> /* Activate H265 engine. */
> - cedrus_engine_enable(ctx, CEDRUS_CODEC_H265);
> + cedrus_engine_enable(ctx);
>
> /* Source offset and length in bits. */
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> index c3387cd1e80f..fa86a658fdc6 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> @@ -31,7 +31,7 @@
> #include "cedrus_hw.h"
> #include "cedrus_regs.h"
>
> -int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec)
> +int cedrus_engine_enable(struct cedrus_ctx *ctx)
> {
> u32 reg = 0;
>
> @@ -42,18 +42,18 @@ int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec)
> reg |= VE_MODE_REC_WR_MODE_2MB;
> reg |= VE_MODE_DDR_MODE_BW_128;
>
> - switch (codec) {
> - case CEDRUS_CODEC_MPEG2:
> + switch (ctx->src_fmt.pixelformat) {
> + case V4L2_PIX_FMT_MPEG2_SLICE:
> reg |= VE_MODE_DEC_MPEG;
> break;
>
> /* H.264 and VP8 both use the same decoding mode bit. */
> - case CEDRUS_CODEC_H264:
> - case CEDRUS_CODEC_VP8:
> + case V4L2_PIX_FMT_H264_SLICE:
> + case V4L2_PIX_FMT_VP8_FRAME:
> reg |= VE_MODE_DEC_H264;
Could be nice to declare VE_MODE_DEC_VP8 with the same bit or to rename it
VE_MODE_DEC_H264_VP8. There's no particular reason why H264 should prevail
over VP8.
Cheers,
Paul
> break;
>
> - case CEDRUS_CODEC_H265:
> + case V4L2_PIX_FMT_HEVC_SLICE:
> reg |= VE_MODE_DEC_H265;
> break;
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> index 7c92f00e36da..6f1e701b1ea8 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> @@ -16,7 +16,7 @@
> #ifndef _CEDRUS_HW_H_
> #define _CEDRUS_HW_H_
>
> -int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec);
> +int cedrus_engine_enable(struct cedrus_ctx *ctx);
> void cedrus_engine_disable(struct cedrus_dev *dev);
>
> void cedrus_dst_format_set(struct cedrus_dev *dev,
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> index c1128d2cd555..10e98f08aafc 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> @@ -66,7 +66,7 @@ static int cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
> quantisation = run->mpeg2.quantisation;
>
> /* Activate MPEG engine. */
> - cedrus_engine_enable(ctx, CEDRUS_CODEC_MPEG2);
> + cedrus_engine_enable(ctx);
>
> /* Set intra quantisation matrix. */
> matrix = quantisation->intra_quantiser_matrix;
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> index f7714baae37d..969677a3bbf9 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> @@ -662,7 +662,7 @@ static int cedrus_vp8_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
> int header_size;
> u32 reg;
>
> - cedrus_engine_enable(ctx, CEDRUS_CODEC_VP8);
> + cedrus_engine_enable(ctx);
>
> cedrus_write(dev, VE_H264_CTRL, VE_H264_CTRL_VP8);
>
> --
> 2.38.1
>
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Hi Jernej,
On Mon 24 Oct 22, 22:15, Jernej Skrabec wrote:
> cedrus_try_fmt_vid_out() is called two times inside
> cedrus_s_fmt_vid_out(), but nothing changes between calls which would
> influence output format. Remove first call, which was added later.
Thanks for the cleanup!
Acked-by: Paul Kocialkowski <[email protected]>
Cheers,
Paul
> Signed-off-by: Jernej Skrabec <[email protected]>
> ---
> drivers/staging/media/sunxi/cedrus/cedrus_video.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> index 33726175d980..1c3c1d080d31 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> @@ -309,10 +309,6 @@ static int cedrus_s_fmt_vid_out(struct file *file, void *priv,
> struct vb2_queue *peer_vq;
> int ret;
>
> - ret = cedrus_try_fmt_vid_out(file, priv, f);
> - if (ret)
> - return ret;
> -
> vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> /*
> * In order to support dynamic resolution change,
> --
> 2.38.1
>
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Dne ponedeljek, 24. oktober 2022 ob 22:15:09 CET je Jernej Skrabec napisal(a):
> Because not all Cedrus variants supports all codecs, controls should be
> registered only if codec related to individual control is supported by
> Cedrus.
>
> Replace codec enum, which is not used at all, with capabilities flags
> and register control only if capabilities are met.
>
> Signed-off-by: Jernej Skrabec <[email protected]>
> ---
> drivers/staging/media/sunxi/cedrus/cedrus.c | 45 +++++++++++----------
> drivers/staging/media/sunxi/cedrus/cedrus.h | 2 +-
> 2 files changed, 25 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
> b/drivers/staging/media/sunxi/cedrus/cedrus.c index
> 2f284a58d787..023566b02dc5 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -77,56 +77,56 @@ static const struct cedrus_control cedrus_controls[] = {
> .cfg = {
> .id =
V4L2_CID_STATELESS_MPEG2_SEQUENCE,
> },
> - .codec = CEDRUS_CODEC_MPEG2,
> + .capabilities = CEDRUS_CAPABILITY_MPEG2_DEC,
> },
> {
> .cfg = {
> .id =
V4L2_CID_STATELESS_MPEG2_PICTURE,
> },
> - .codec = CEDRUS_CODEC_MPEG2,
> + .capabilities = CEDRUS_CAPABILITY_MPEG2_DEC,
> },
> {
> .cfg = {
> .id =
V4L2_CID_STATELESS_MPEG2_QUANTISATION,
> },
> - .codec = CEDRUS_CODEC_MPEG2,
> + .capabilities = CEDRUS_CAPABILITY_MPEG2_DEC,
> },
> {
> .cfg = {
> .id =
V4L2_CID_STATELESS_H264_DECODE_PARAMS,
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> {
> .cfg = {
> .id =
V4L2_CID_STATELESS_H264_SLICE_PARAMS,
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_H264_SPS,
> .ops = &cedrus_ctrl_ops,
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_H264_PPS,
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> {
> .cfg = {
> .id =
V4L2_CID_STATELESS_H264_SCALING_MATRIX,
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> {
> .cfg = {
> .id =
V4L2_CID_STATELESS_H264_PRED_WEIGHTS,
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> {
> .cfg = {
> @@ -134,7 +134,7 @@ static const struct cedrus_control cedrus_controls[] = {
> .max = V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED,
> .def =
V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED,
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> {
> .cfg = {
> @@ -142,7 +142,7 @@ static const struct cedrus_control cedrus_controls[] = {
> .max = V4L2_STATELESS_H264_START_CODE_NONE,
> .def =
V4L2_STATELESS_H264_START_CODE_NONE,
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> /*
> * We only expose supported profiles information,
> @@ -160,20 +160,20 @@ static const struct cedrus_control cedrus_controls[] =
> { .menu_skip_mask =
>
BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
> },
> - .codec = CEDRUS_CODEC_H264,
> + .capabilities = CEDRUS_CAPABILITY_H264_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_HEVC_SPS,
> .ops = &cedrus_ctrl_ops,
> },
> - .codec = CEDRUS_CODEC_H265,
> + .capabilities = CEDRUS_CAPABILITY_H265_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_HEVC_PPS,
> },
> - .codec = CEDRUS_CODEC_H265,
> + .capabilities = CEDRUS_CAPABILITY_H265_DEC,
> },
> {
> .cfg = {
> @@ -181,13 +181,13 @@ static const struct cedrus_control cedrus_controls[] =
> { /* The driver can only handle 1 entry per slice for now */
> .dims = { 1 },
> },
> - .codec = CEDRUS_CODEC_H265,
> + .capabilities = CEDRUS_CAPABILITY_H265_DEC,
> },
> {
> .cfg = {
> .id =
V4L2_CID_STATELESS_HEVC_SCALING_MATRIX,
> },
> - .codec = CEDRUS_CODEC_H265,
> + .capabilities = CEDRUS_CAPABILITY_H265_DEC,
> },
> {
> .cfg = {
> @@ -197,7 +197,7 @@ static const struct cedrus_control cedrus_controls[] = {
> .max = 0xffffffff,
> .step = 1,
> },
> - .codec = CEDRUS_CODEC_H265,
> + .capabilities = CEDRUS_CAPABILITY_H265_DEC,
> },
> {
> .cfg = {
> @@ -205,7 +205,7 @@ static const struct cedrus_control cedrus_controls[] = {
> .max = V4L2_STATELESS_HEVC_DECODE_MODE_SLICE_BASED,
> .def =
V4L2_STATELESS_HEVC_DECODE_MODE_SLICE_BASED,
> },
> - .codec = CEDRUS_CODEC_H265,
> + .capabilities = CEDRUS_CAPABILITY_H265_DEC,
> },
> {
> .cfg = {
> @@ -213,19 +213,19 @@ static const struct cedrus_control cedrus_controls[] =
> { .max = V4L2_STATELESS_HEVC_START_CODE_NONE,
> .def =
V4L2_STATELESS_HEVC_START_CODE_NONE,
> },
> - .codec = CEDRUS_CODEC_H265,
> + .capabilities = CEDRUS_CAPABILITY_H265_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_VP8_FRAME,
> },
> - .codec = CEDRUS_CODEC_VP8,
> + .capabilities = CEDRUS_CAPABILITY_VP8_DEC,
> },
> {
> .cfg = {
> .id = V4L2_CID_STATELESS_HEVC_DECODE_PARAMS,
> },
> - .codec = CEDRUS_CODEC_H265,
> + .capabilities = CEDRUS_CAPABILITY_H265_DEC,
> },
> };
>
> @@ -275,6 +275,9 @@ static int cedrus_init_ctrls(struct cedrus_dev *dev,
> struct cedrus_ctx *ctx) return -ENOMEM;
>
> for (i = 0; i < CEDRUS_CONTROLS_COUNT; i++) {
> + if (!cedrus_is_capable(ctx,
cedrus_controls[i].capabilities))
> + continue;
While it's not visible from this commit, above filtering causes issues when
searching for controls in ctx->ctrls[]. Now are not packed together and search
functions (cedrus_find_control_data, cedrus_get_num_of_controls) rely on
controls not being null. null control marks end of the array.
I'll fix that in next revision.
Best regards,
Jernej
> +
> ctrl = v4l2_ctrl_new_custom(hdl,
&cedrus_controls[i].cfg,
> NULL);
> if (hdl->error) {
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
> b/drivers/staging/media/sunxi/cedrus/cedrus.h index
> 1a98790a99af..7a1619967513 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -57,7 +57,7 @@ enum cedrus_h264_pic_type {
>
> struct cedrus_control {
> struct v4l2_ctrl_config cfg;
> - enum cedrus_codec codec;
> + unsigned int capabilities;
> };
>
> struct cedrus_h264_run {
> --
> 2.38.1