RE: [PATCH 2/2] v4l: s5p-mfc: Limit enum_fmt to output formats of current version
Hi Pawel,
From: Pawel Osciak [mailto:posc...@chromium.org]
Sent: Tuesday, May 20, 2014 3:47 AM
Hi Kamil,
I like the solution as well. Two suggestions to consider below.
On Fri, May 16, 2014 at 9:03 PM, Kamil Debski k.deb...@samsung.com
wrote:
MFC versions support a different set of formats, this specially
applies to the raw YUV formats. This patch changes enum_fmt, so that
it only reports formats that are supported by the used MFC version.
Signed-off-by: Kamil Debski k.deb...@samsung.com
---
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index 9370c34..d5efb10 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -223,6 +223,7 @@ struct s5p_mfc_buf_align { struct
s5p_mfc_variant
{
unsigned int version;
unsigned int port_num;
+ u32 version_bit;
struct s5p_mfc_buf_size *buf_size;
struct s5p_mfc_buf_align *buf_align;
char*fw_name;
@@ -666,6 +667,7 @@ struct s5p_mfc_fmt {
u32 codec_mode;
enum s5p_mfc_fmt_type type;
u32 num_planes;
+ u32 versions;
};
/**
@@ -705,4 +707,9 @@ void set_work_bit_irqsave(struct s5p_mfc_ctx
*ctx);
#define IS_MFCV6_PLUS(dev) (dev-variant-version = 0x60 ? 1 :
0)
#define IS_MFCV7_PLUS(dev) (dev-variant-version = 0x70 ? 1 :
0)
+#define MFC_V5 BIT(0)
+#define MFC_V6 BIT(1)
+#define MFC_V7 BIT(2)
These may be confusing. I'd suggest at least suffixing those macros
with _BIT.
Or better yet, please make this into an enum and also make
variant-versions of size BITS_TO_LONGS() with max enum value.
I think I'll stick with adding the _BIT suffix.
/* Get format */
@@ -384,11 +402,9 @@ static int vidioc_try_fmt(struct file *file,
void *priv, struct v4l2_format *f)
mfc_err(Unknown codec\n);
return -EINVAL;
}
- if (!IS_MFCV6_PLUS(dev)) {
- if (fmt-fourcc == V4L2_PIX_FMT_VP8) {
- mfc_err(Not supported format.\n);
- return -EINVAL;
- }
+ if ((dev-variant-version_bit fmt-versions) == 0)
{
+ mfc_err(Unsupported format by this MFC
version.\n);
+ return -EINVAL;
What do you think of moving this check to find_format()? You wouldn't
have to duplicate it across enum_fmt and try_fmt then...
Find_format is used as a helper for try_fmt and is not used by enum_fmt.
Enum_fmt does also some other checks and operates iterates the formats
array directly, so I think the change included in this patch is ok.
Best wishes,
--
Kamil Debski
Samsung RD Institute Poland
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] v4l: s5p-mfc: Limit enum_fmt to output formats of current version
Hi Kamil,
On 16/05/14 14:03, Kamil Debski wrote:
MFC versions support a different set of formats, this specially applies
to the raw YUV formats. This patch changes enum_fmt, so that it only
reports formats that are supported by the used MFC version.
Signed-off-by: Kamil Debski k.deb...@samsung.com
The patch looks good to me. It seems a nice and clean solution
to the problem.
Acked-by: Sylwester Nawrocki s.nawro...@samsung.com
Regards,
Sylwester
---
drivers/media/platform/s5p-mfc/s5p_mfc.c|3 ++
drivers/media/platform/s5p-mfc/s5p_mfc_common.h |7
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c| 49 +-
drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 50
---
4 files changed, 67 insertions(+), 42 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 2ab90dd..06d2678 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -1339,6 +1339,7 @@ struct s5p_mfc_buf_align mfc_buf_align_v5 = {
static struct s5p_mfc_variant mfc_drvdata_v5 = {
.version= MFC_VERSION,
+ .version_bit= MFC_V5,
.port_num = MFC_NUM_PORTS,
.buf_size = buf_size_v5,
.buf_align = mfc_buf_align_v5,
@@ -1365,6 +1366,7 @@ struct s5p_mfc_buf_align mfc_buf_align_v6 = {
static struct s5p_mfc_variant mfc_drvdata_v6 = {
.version= MFC_VERSION_V6,
+ .version_bit= MFC_V6,
.port_num = MFC_NUM_PORTS_V6,
.buf_size = buf_size_v6,
.buf_align = mfc_buf_align_v6,
@@ -1391,6 +1393,7 @@ struct s5p_mfc_buf_align mfc_buf_align_v7 = {
static struct s5p_mfc_variant mfc_drvdata_v7 = {
.version= MFC_VERSION_V7,
+ .version_bit= MFC_V7,
.port_num = MFC_NUM_PORTS_V7,
.buf_size = buf_size_v7,
.buf_align = mfc_buf_align_v7,
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index 9370c34..d5efb10 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -223,6 +223,7 @@ struct s5p_mfc_buf_align {
struct s5p_mfc_variant {
unsigned int version;
unsigned int port_num;
+ u32 version_bit;
struct s5p_mfc_buf_size *buf_size;
struct s5p_mfc_buf_align *buf_align;
char*fw_name;
@@ -666,6 +667,7 @@ struct s5p_mfc_fmt {
u32 codec_mode;
enum s5p_mfc_fmt_type type;
u32 num_planes;
+ u32 versions;
};
/**
@@ -705,4 +707,9 @@ void set_work_bit_irqsave(struct s5p_mfc_ctx *ctx);
#define IS_MFCV6_PLUS(dev) (dev-variant-version = 0x60 ? 1 : 0)
#define IS_MFCV7_PLUS(dev) (dev-variant-version = 0x70 ? 1 : 0)
+#define MFC_V5 BIT(0)
+#define MFC_V6 BIT(1)
+#define MFC_V7 BIT(2)
+
+
#endif /* S5P_MFC_COMMON_H_ */
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index ac43a4a..a1fee39 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -39,6 +39,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_NONE,
.type = MFC_FMT_RAW,
.num_planes = 2,
+ .versions = MFC_V6 | MFC_V7,
},
{
.name = 4:2:0 2 Planes 64x32 Tiles,
@@ -46,6 +47,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_NONE,
.type = MFC_FMT_RAW,
.num_planes = 2,
+ .versions = MFC_V5,
},
{
.name = 4:2:0 2 Planes Y/CbCr,
@@ -53,6 +55,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_NONE,
.type = MFC_FMT_RAW,
.num_planes = 2,
+ .versions = MFC_V6 | MFC_V7,
},
{
.name = 4:2:0 2 Planes Y/CrCb,
@@ -60,6 +63,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_NONE,
.type = MFC_FMT_RAW,
.num_planes = 2,
+ .versions = MFC_V6 | MFC_V7,
},
{
.name = H264 Encoded Stream,
@@ -67,6 +71,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_H264_DEC,
.type = MFC_FMT_DEC,
.num_planes = 1,
+ .versions = MFC_V5 | MFC_V6 | MFC_V7,
},
{
.name = H264/MVC Encoded Stream,
@@ -74,6 +79,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_H264_MVC_DEC,
Re: [PATCH 2/2] v4l: s5p-mfc: Limit enum_fmt to output formats of current version
Hi Kamil,
Only a small correction.
On Fri, May 16, 2014 at 5:33 PM, Kamil Debski k.deb...@samsung.com wrote:
MFC versions support a different set of formats, this specially applies
to the raw YUV formats. This patch changes enum_fmt, so that it only
reports formats that are supported by the used MFC version.
Signed-off-by: Kamil Debski k.deb...@samsung.com
---
drivers/media/platform/s5p-mfc/s5p_mfc.c|3 ++
drivers/media/platform/s5p-mfc/s5p_mfc_common.h |7
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c| 49 +-
drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 50
---
4 files changed, 67 insertions(+), 42 deletions(-)
[snip]
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index d09c2e1..1ddd152 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -42,6 +42,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_NONE,
.type = MFC_FMT_RAW,
.num_planes = 2,
+ .versions = MFC_V6 | MFC_V7,
},
{
.name = 4:2:0 2 Planes 64x32 Tiles,
@@ -49,6 +50,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_NONE,
.type = MFC_FMT_RAW,
.num_planes = 2,
+ .versions = MFC_V5,
},
{
.name = 4:2:0 2 Planes Y/CbCr,
@@ -56,6 +58,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_NONE,
.type = MFC_FMT_RAW,
.num_planes = 2,
+ .versions = MFC_V5 | MFC_V6 | MFC_V7,
},
{
.name = 4:2:0 2 Planes Y/CrCb,
@@ -63,6 +66,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_NONE,
.type = MFC_FMT_RAW,
.num_planes = 2,
+ .versions = MFC_V5 | MFC_V6 | MFC_V7,
},
{
.name = H264 Encoded Stream,
@@ -70,6 +74,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_H264_ENC,
.type = MFC_FMT_ENC,
.num_planes = 1,
+ .versions = MFC_V5 | MFC_V6 | MFC_V7,
},
{
.name = MPEG4 Encoded Stream,
@@ -77,6 +82,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_MPEG4_ENC,
.type = MFC_FMT_ENC,
.num_planes = 1,
+ .versions = MFC_V5 | MFC_V6 | MFC_V7,
},
{
.name = H263 Encoded Stream,
@@ -84,6 +90,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_H263_ENC,
.type = MFC_FMT_ENC,
.num_planes = 1,
+ .versions = MFC_V5 | MFC_V6 | MFC_V7,
},
{
.name = VP8 Encoded Stream,
@@ -91,6 +98,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_VP8_ENC,
.type = MFC_FMT_ENC,
.num_planes = 1,
+ .versions = MFC_V6 | MFC_V7,
VP8 encodig is supported only from v7 onwards.
So you can remove MFC_V6 from this.
Otherwise looks good to me.
Regards
Arun
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] v4l: s5p-mfc: Limit enum_fmt to output formats of current version
Hi Kamil,
I like the solution as well. Two suggestions to consider below.
On Fri, May 16, 2014 at 9:03 PM, Kamil Debski k.deb...@samsung.com wrote:
MFC versions support a different set of formats, this specially applies
to the raw YUV formats. This patch changes enum_fmt, so that it only
reports formats that are supported by the used MFC version.
Signed-off-by: Kamil Debski k.deb...@samsung.com
---
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index 9370c34..d5efb10 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -223,6 +223,7 @@ struct s5p_mfc_buf_align {
struct s5p_mfc_variant {
unsigned int version;
unsigned int port_num;
+ u32 version_bit;
struct s5p_mfc_buf_size *buf_size;
struct s5p_mfc_buf_align *buf_align;
char*fw_name;
@@ -666,6 +667,7 @@ struct s5p_mfc_fmt {
u32 codec_mode;
enum s5p_mfc_fmt_type type;
u32 num_planes;
+ u32 versions;
};
/**
@@ -705,4 +707,9 @@ void set_work_bit_irqsave(struct s5p_mfc_ctx *ctx);
#define IS_MFCV6_PLUS(dev) (dev-variant-version = 0x60 ? 1 : 0)
#define IS_MFCV7_PLUS(dev) (dev-variant-version = 0x70 ? 1 : 0)
+#define MFC_V5 BIT(0)
+#define MFC_V6 BIT(1)
+#define MFC_V7 BIT(2)
These may be confusing. I'd suggest at least suffixing those macros with _BIT.
Or better yet, please make this into an enum and also make
variant-versions of size BITS_TO_LONGS() with max enum value.
/* Get format */
@@ -384,11 +402,9 @@ static int vidioc_try_fmt(struct file *file, void *priv,
struct v4l2_format *f)
mfc_err(Unknown codec\n);
return -EINVAL;
}
- if (!IS_MFCV6_PLUS(dev)) {
- if (fmt-fourcc == V4L2_PIX_FMT_VP8) {
- mfc_err(Not supported format.\n);
- return -EINVAL;
- }
+ if ((dev-variant-version_bit fmt-versions) == 0) {
+ mfc_err(Unsupported format by this MFC version.\n);
+ return -EINVAL;
What do you think of moving this check to find_format()? You wouldn't
have to duplicate it across enum_fmt and try_fmt then...
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] v4l: s5p-mfc: Limit enum_fmt to output formats of current version
MFC versions support a different set of formats, this specially applies
to the raw YUV formats. This patch changes enum_fmt, so that it only
reports formats that are supported by the used MFC version.
Signed-off-by: Kamil Debski k.deb...@samsung.com
---
drivers/media/platform/s5p-mfc/s5p_mfc.c|3 ++
drivers/media/platform/s5p-mfc/s5p_mfc_common.h |7
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c| 49 +-
drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 50 ---
4 files changed, 67 insertions(+), 42 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 2ab90dd..06d2678 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -1339,6 +1339,7 @@ struct s5p_mfc_buf_align mfc_buf_align_v5 = {
static struct s5p_mfc_variant mfc_drvdata_v5 = {
.version= MFC_VERSION,
+ .version_bit= MFC_V5,
.port_num = MFC_NUM_PORTS,
.buf_size = buf_size_v5,
.buf_align = mfc_buf_align_v5,
@@ -1365,6 +1366,7 @@ struct s5p_mfc_buf_align mfc_buf_align_v6 = {
static struct s5p_mfc_variant mfc_drvdata_v6 = {
.version= MFC_VERSION_V6,
+ .version_bit= MFC_V6,
.port_num = MFC_NUM_PORTS_V6,
.buf_size = buf_size_v6,
.buf_align = mfc_buf_align_v6,
@@ -1391,6 +1393,7 @@ struct s5p_mfc_buf_align mfc_buf_align_v7 = {
static struct s5p_mfc_variant mfc_drvdata_v7 = {
.version= MFC_VERSION_V7,
+ .version_bit= MFC_V7,
.port_num = MFC_NUM_PORTS_V7,
.buf_size = buf_size_v7,
.buf_align = mfc_buf_align_v7,
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index 9370c34..d5efb10 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -223,6 +223,7 @@ struct s5p_mfc_buf_align {
struct s5p_mfc_variant {
unsigned int version;
unsigned int port_num;
+ u32 version_bit;
struct s5p_mfc_buf_size *buf_size;
struct s5p_mfc_buf_align *buf_align;
char*fw_name;
@@ -666,6 +667,7 @@ struct s5p_mfc_fmt {
u32 codec_mode;
enum s5p_mfc_fmt_type type;
u32 num_planes;
+ u32 versions;
};
/**
@@ -705,4 +707,9 @@ void set_work_bit_irqsave(struct s5p_mfc_ctx *ctx);
#define IS_MFCV6_PLUS(dev) (dev-variant-version = 0x60 ? 1 : 0)
#define IS_MFCV7_PLUS(dev) (dev-variant-version = 0x70 ? 1 : 0)
+#define MFC_V5 BIT(0)
+#define MFC_V6 BIT(1)
+#define MFC_V7 BIT(2)
+
+
#endif /* S5P_MFC_COMMON_H_ */
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index ac43a4a..a1fee39 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -39,6 +39,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_NONE,
.type = MFC_FMT_RAW,
.num_planes = 2,
+ .versions = MFC_V6 | MFC_V7,
},
{
.name = 4:2:0 2 Planes 64x32 Tiles,
@@ -46,6 +47,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_NONE,
.type = MFC_FMT_RAW,
.num_planes = 2,
+ .versions = MFC_V5,
},
{
.name = 4:2:0 2 Planes Y/CbCr,
@@ -53,6 +55,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_NONE,
.type = MFC_FMT_RAW,
.num_planes = 2,
+ .versions = MFC_V6 | MFC_V7,
},
{
.name = 4:2:0 2 Planes Y/CrCb,
@@ -60,6 +63,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_NONE,
.type = MFC_FMT_RAW,
.num_planes = 2,
+ .versions = MFC_V6 | MFC_V7,
},
{
.name = H264 Encoded Stream,
@@ -67,6 +71,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_H264_DEC,
.type = MFC_FMT_DEC,
.num_planes = 1,
+ .versions = MFC_V5 | MFC_V6 | MFC_V7,
},
{
.name = H264/MVC Encoded Stream,
@@ -74,6 +79,7 @@ static struct s5p_mfc_fmt formats[] = {
.codec_mode = S5P_MFC_CODEC_H264_MVC_DEC,
.type = MFC_FMT_DEC,
.num_planes = 1,
+ .versions = MFC_V6 | MFC_V7,
},
{
.name = H263 Encoded Stream,
@@ -81,6 +87,7 @@ static
