Getting the chip name of each platform according to the device
compatible to set different parameter.
Signed-off-by: Yunfei Dong <[email protected]>
---
.../mediatek/vcodec/decoder/mtk_vcodec_dec.c | 24 +----------------
.../vcodec/decoder/mtk_vcodec_dec_drv.c | 26 +++++++++++++++++++
.../vcodec/decoder/mtk_vcodec_dec_drv.h | 17 ++++++++++++
3 files changed, 44 insertions(+), 23 deletions(-)
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
index 91ed576d6821..ba742f0e391d 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
@@ -208,36 +208,14 @@ static int vidioc_vdec_dqbuf(struct file *file, void *priv,
return v4l2_m2m_dqbuf(file, ctx->m2m_ctx, buf);
}
-static int mtk_vcodec_dec_get_chip_name(void *priv)
-{
- struct mtk_vcodec_dec_ctx *ctx = fh_to_dec_ctx(priv);
- struct device *dev = &ctx->dev->plat_dev->dev;
-
- if (of_device_is_compatible(dev->of_node, "mediatek,mt8173-vcodec-dec"))
- return 8173;
- else if (of_device_is_compatible(dev->of_node, "mediatek,mt8183-vcodec-dec"))
- return 8183;
- else if (of_device_is_compatible(dev->of_node, "mediatek,mt8192-vcodec-dec"))
- return 8192;
- else if (of_device_is_compatible(dev->of_node, "mediatek,mt8195-vcodec-dec"))
- return 8195;
- else if (of_device_is_compatible(dev->of_node, "mediatek,mt8186-vcodec-dec"))
- return 8186;
- else if (of_device_is_compatible(dev->of_node, "mediatek,mt8188-vcodec-dec"))
- return 8188;
- else
- return 8173;
-}
-
static int vidioc_vdec_querycap(struct file *file, void *priv,
struct v4l2_capability *cap)
{
struct mtk_vcodec_dec_ctx *ctx = fh_to_dec_ctx(priv);
struct device *dev = &ctx->dev->plat_dev->dev;
- int platform_name = mtk_vcodec_dec_get_chip_name(priv);
strscpy(cap->driver, dev->driver->name, sizeof(cap->driver));
- snprintf(cap->card, sizeof(cap->card), "MT%d video decoder", platform_name);
+ snprintf(cap->card, sizeof(cap->card), "MT%d video decoder", ctx->dev->chip_name);
return 0;
}
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
index 0a89ce452ac3..f47c98faf068 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
@@ -326,6 +326,26 @@ static const struct v4l2_file_operations mtk_vcodec_fops = {
.mmap = v4l2_m2m_fop_mmap,
};
+static void mtk_vcodec_dec_get_chip_name(struct mtk_vcodec_dec_dev *vdec_dev)
+{
+ struct device *dev = &vdec_dev->plat_dev->dev;
+
+ if (of_device_is_compatible(dev->of_node, "mediatek,mt8173-vcodec-dec"))
+ vdec_dev->chip_name = MTK_VDEC_MT8173;
+ else if (of_device_is_compatible(dev->of_node, "mediatek,mt8183-vcodec-dec"))
+ vdec_dev->chip_name = MTK_VDEC_MT8183;
+ else if (of_device_is_compatible(dev->of_node, "mediatek,mt8192-vcodec-dec"))
+ vdec_dev->chip_name = MTK_VDEC_MT8192;
+ else if (of_device_is_compatible(dev->of_node, "mediatek,mt8195-vcodec-dec"))
+ vdec_dev->chip_name = MTK_VDEC_MT8195;
+ else if (of_device_is_compatible(dev->of_node, "mediatek,mt8186-vcodec-dec"))
+ vdec_dev->chip_name = MTK_VDEC_MT8186;
+ else if (of_device_is_compatible(dev->of_node, "mediatek,mt8188-vcodec-dec"))
+ vdec_dev->chip_name = MTK_VDEC_MT8188;
+ else
+ vdec_dev->chip_name = MTK_VDEC_INVAL;
+}
+
static int mtk_vcodec_probe(struct platform_device *pdev)
{
struct mtk_vcodec_dec_dev *dev;
@@ -341,6 +361,12 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
INIT_LIST_HEAD(&dev->ctx_list);
dev->plat_dev = pdev;
+ mtk_vcodec_dec_get_chip_name(dev);
+ if (dev->chip_name == MTK_VDEC_INVAL) {
+ dev_err(&pdev->dev, "Failed to get decoder chip name");
+ return -EINVAL;
+ }
+
dev->vdec_pdata = of_device_get_match_data(&pdev->dev);
if (!of_property_read_u32(pdev->dev.of_node, "mediatek,vpu",
&rproc_phandle)) {
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
index 7e36b2c69b7d..8f228ba9aa47 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
@@ -18,6 +18,19 @@
#define IS_VDEC_LAT_ARCH(hw_arch) ((hw_arch) >= MTK_VDEC_LAT_SINGLE_CORE)
#define IS_VDEC_INNER_RACING(capability) ((capability) & MTK_VCODEC_INNER_RACING)
+/*
+ * enum mtk_vcodec_dec_chip_name - Structure used to separate different platform
+ */
+enum mtk_vcodec_dec_chip_name {
+ MTK_VDEC_INVAL = 0,
+ MTK_VDEC_MT8173 = 8173,
+ MTK_VDEC_MT8183 = 8183,
+ MTK_VDEC_MT8186 = 8186,
+ MTK_VDEC_MT8188 = 8188,
+ MTK_VDEC_MT8192 = 8192,
+ MTK_VDEC_MT8195 = 8195,
+};
+
/*
* enum mtk_vdec_format_types - Structure used to get supported
* format types according to decoder capability
@@ -249,6 +262,8 @@ struct mtk_vcodec_dec_ctx {
* @vdec_racing_info: record register value
* @dec_racing_info_mutex: mutex lock used for inner racing mode
* @dbgfs: debug log related information
+ *
+ * @chip_name: the chip name used to separate different platform
*/
struct mtk_vcodec_dec_dev {
struct v4l2_device v4l2_dev;
@@ -289,6 +304,8 @@ struct mtk_vcodec_dec_dev {
/* Protects access to vdec_racing_info data */
struct mutex dec_racing_info_mutex;
struct mtk_vcodec_dbgfs dbgfs;
+
+ enum mtk_vcodec_dec_chip_name chip_name;
};
static inline struct mtk_vcodec_dec_ctx *fh_to_dec_ctx(struct v4l2_fh *fh)
--
2.18.0
The supported format type of different platforms are not the
same. Need to set the supported profile according to the chip name.
Signed-off-by: Yunfei Dong <[email protected]>
---
.../vcodec/decoder/mtk_vcodec_dec_stateless.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
index 1fdb21dbacb8..84c0bed577ed 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
@@ -559,6 +559,20 @@ static void mtk_vcodec_dec_fill_h264_level(struct v4l2_ctrl_config *cfg,
};
}
+static void mtk_vcodec_dec_fill_h264_profile(struct v4l2_ctrl_config *cfg,
+ struct mtk_vcodec_dec_ctx *ctx)
+{
+ switch (ctx->dev->chip_name) {
+ case MTK_VDEC_MT8188:
+ case MTK_VDEC_MT8195:
+ cfg->max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH_10;
+ break;
+ default:
+ cfg->max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH;
+ break;
+ };
+}
+
static void mtk_vcodec_dec_fill_h265_level(struct v4l2_ctrl_config *cfg,
struct mtk_vcodec_dec_ctx *ctx)
{
@@ -585,6 +599,11 @@ static void mtk_vcodec_dec_reset_controls(struct v4l2_ctrl_config *cfg,
mtk_vcodec_dec_fill_h265_level(cfg, ctx);
mtk_v4l2_vdec_dbg(3, ctx, "h265 supported level: %lld %lld", cfg->max, cfg->def);
break;
+ case V4L2_CID_MPEG_VIDEO_H264_PROFILE:
+ mtk_vcodec_dec_fill_h264_profile(cfg, ctx);
+ mtk_v4l2_vdec_dbg(3, ctx, "h264 supported profile: %lld %lld", cfg->max,
+ cfg->menu_skip_mask);
+ break;
default:
break;
};
--
2.18.0
Il 16/10/23 08:43, Yunfei Dong ha scritto:
> Getting the chip name of each platform according to the device
> compatible to set different parameter.
>
> Signed-off-by: Yunfei Dong <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Hey Yunfei,
Thanks for your patches!
Could you provide a cover-letter for the next version please?
This will help to get a good context of why we need these changes and to
store the changelog in a helpful manner.
Thanks.
On 16.10.2023 14:43, Yunfei Dong wrote:
>Getting the chip name of each platform according to the device
>compatible to set different parameter.
I would reword this commit description slightly, basically what you
change is that you store the chip name in context permanently and that
you utilize a enum to be more descriptive.
So how about:
"""
Store the name of the chip in the context of the driver in order to be
able to choose the correct configuration values for the different codecs.
Use a enum value instead of an integer to store a more descriptive name.
"""
A few more comments below.
>
>Signed-off-by: Yunfei Dong <[email protected]>
>---
> .../mediatek/vcodec/decoder/mtk_vcodec_dec.c | 24 +----------------
> .../vcodec/decoder/mtk_vcodec_dec_drv.c | 26 +++++++++++++++++++
> .../vcodec/decoder/mtk_vcodec_dec_drv.h | 17 ++++++++++++
> 3 files changed, 44 insertions(+), 23 deletions(-)
>
>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
>index 91ed576d6821..ba742f0e391d 100644
>--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
>+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
>@@ -208,36 +208,14 @@ static int vidioc_vdec_dqbuf(struct file *file, void *priv,
> return v4l2_m2m_dqbuf(file, ctx->m2m_ctx, buf);
> }
>
>-static int mtk_vcodec_dec_get_chip_name(void *priv)
>-{
>- struct mtk_vcodec_dec_ctx *ctx = fh_to_dec_ctx(priv);
>- struct device *dev = &ctx->dev->plat_dev->dev;
>-
>- if (of_device_is_compatible(dev->of_node, "mediatek,mt8173-vcodec-dec"))
>- return 8173;
>- else if (of_device_is_compatible(dev->of_node, "mediatek,mt8183-vcodec-dec"))
>- return 8183;
>- else if (of_device_is_compatible(dev->of_node, "mediatek,mt8192-vcodec-dec"))
>- return 8192;
>- else if (of_device_is_compatible(dev->of_node, "mediatek,mt8195-vcodec-dec"))
>- return 8195;
>- else if (of_device_is_compatible(dev->of_node, "mediatek,mt8186-vcodec-dec"))
>- return 8186;
>- else if (of_device_is_compatible(dev->of_node, "mediatek,mt8188-vcodec-dec"))
>- return 8188;
>- else
>- return 8173;
>-}
>-
> static int vidioc_vdec_querycap(struct file *file, void *priv,
> struct v4l2_capability *cap)
> {
> struct mtk_vcodec_dec_ctx *ctx = fh_to_dec_ctx(priv);
> struct device *dev = &ctx->dev->plat_dev->dev;
>- int platform_name = mtk_vcodec_dec_get_chip_name(priv);
>
> strscpy(cap->driver, dev->driver->name, sizeof(cap->driver));
>- snprintf(cap->card, sizeof(cap->card), "MT%d video decoder", platform_name);
>+ snprintf(cap->card, sizeof(cap->card), "MT%d video decoder", ctx->dev->chip_name);
>
> return 0;
> }
>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
>index 0a89ce452ac3..f47c98faf068 100644
>--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
>+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
>@@ -326,6 +326,26 @@ static const struct v4l2_file_operations mtk_vcodec_fops = {
> .mmap = v4l2_m2m_fop_mmap,
> };
>
>+static void mtk_vcodec_dec_get_chip_name(struct mtk_vcodec_dec_dev *vdec_dev)
>+{
>+ struct device *dev = &vdec_dev->plat_dev->dev;
>+
>+ if (of_device_is_compatible(dev->of_node, "mediatek,mt8173-vcodec-dec"))
>+ vdec_dev->chip_name = MTK_VDEC_MT8173;
>+ else if (of_device_is_compatible(dev->of_node, "mediatek,mt8183-vcodec-dec"))
>+ vdec_dev->chip_name = MTK_VDEC_MT8183;
>+ else if (of_device_is_compatible(dev->of_node, "mediatek,mt8192-vcodec-dec"))
>+ vdec_dev->chip_name = MTK_VDEC_MT8192;
>+ else if (of_device_is_compatible(dev->of_node, "mediatek,mt8195-vcodec-dec"))
>+ vdec_dev->chip_name = MTK_VDEC_MT8195;
>+ else if (of_device_is_compatible(dev->of_node, "mediatek,mt8186-vcodec-dec"))
>+ vdec_dev->chip_name = MTK_VDEC_MT8186;
>+ else if (of_device_is_compatible(dev->of_node, "mediatek,mt8188-vcodec-dec"))
>+ vdec_dev->chip_name = MTK_VDEC_MT8188;
>+ else
>+ vdec_dev->chip_name = MTK_VDEC_INVAL;
>+}
>+
> static int mtk_vcodec_probe(struct platform_device *pdev)
> {
> struct mtk_vcodec_dec_dev *dev;
>@@ -341,6 +361,12 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
> INIT_LIST_HEAD(&dev->ctx_list);
> dev->plat_dev = pdev;
>
>+ mtk_vcodec_dec_get_chip_name(dev);
>+ if (dev->chip_name == MTK_VDEC_INVAL) {
>+ dev_err(&pdev->dev, "Failed to get decoder chip name");
>+ return -EINVAL;
>+ }
>+
> dev->vdec_pdata = of_device_get_match_data(&pdev->dev);
> if (!of_property_read_u32(pdev->dev.of_node, "mediatek,vpu",
> &rproc_phandle)) {
>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
>index 7e36b2c69b7d..8f228ba9aa47 100644
>--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
>+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
>@@ -18,6 +18,19 @@
> #define IS_VDEC_LAT_ARCH(hw_arch) ((hw_arch) >= MTK_VDEC_LAT_SINGLE_CORE)
> #define IS_VDEC_INNER_RACING(capability) ((capability) & MTK_VCODEC_INNER_RACING)
>
>+/*
>+ * enum mtk_vcodec_dec_chip_name - Structure used to separate different platform
>+ */
I don't feel like this comment is terribly helpful because it is pretty
clear what the enum is about, I would just drop it.
>+enum mtk_vcodec_dec_chip_name {
>+ MTK_VDEC_INVAL = 0,
>+ MTK_VDEC_MT8173 = 8173,
>+ MTK_VDEC_MT8183 = 8183,
>+ MTK_VDEC_MT8186 = 8186,
>+ MTK_VDEC_MT8188 = 8188,
>+ MTK_VDEC_MT8192 = 8192,
>+ MTK_VDEC_MT8195 = 8195,
>+};
>+
> /*
> * enum mtk_vdec_format_types - Structure used to get supported
> * format types according to decoder capability
>@@ -249,6 +262,8 @@ struct mtk_vcodec_dec_ctx {
> * @vdec_racing_info: record register value
> * @dec_racing_info_mutex: mutex lock used for inner racing mode
> * @dbgfs: debug log related information
>+ *
>+ * @chip_name: the chip name used to separate different platform
I wouldn't repeat chip name in the description and specify more
concretely why we need to separate the platforms.
My suggestion:
* @chip_name: used to distinguish platforms and select the correct codec configuration values.
> */
> struct mtk_vcodec_dec_dev {
> struct v4l2_device v4l2_dev;
>@@ -289,6 +304,8 @@ struct mtk_vcodec_dec_dev {
> /* Protects access to vdec_racing_info data */
> struct mutex dec_racing_info_mutex;
> struct mtk_vcodec_dbgfs dbgfs;
>+
>+ enum mtk_vcodec_dec_chip_name chip_name;
> };
>
> static inline struct mtk_vcodec_dec_ctx *fh_to_dec_ctx(struct v4l2_fh *fh)
Besides those small wording choices, the patch looks good.
So with these issues resolved:
Reviewed-by: Sebastian Fricke <[email protected]>
Regards,
Sebastian
>--
>2.18.0
>
Hey Yunfei,
On 16.10.2023 14:43, Yunfei Dong wrote:
>The supported format type of different platforms are not the
>same. Need to set the supported profile according to the chip name.
I would suggest the following rewording:
Set the maximum H264 codec profile for each platform.
The various mediatek platforms support different profiles for decoding,
the profile of the codec limits the capabilities for decoding.
With that you can add:
Reviewed-by: Sebastian Fricke <[email protected]>
Regards,
Sebastian
>
>Signed-off-by: Yunfei Dong <[email protected]>
>---
> .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>index 1fdb21dbacb8..84c0bed577ed 100644
>--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>@@ -559,6 +559,20 @@ static void mtk_vcodec_dec_fill_h264_level(struct v4l2_ctrl_config *cfg,
> };
> }
>
>+static void mtk_vcodec_dec_fill_h264_profile(struct v4l2_ctrl_config *cfg,
>+ struct mtk_vcodec_dec_ctx *ctx)
>+{
>+ switch (ctx->dev->chip_name) {
>+ case MTK_VDEC_MT8188:
>+ case MTK_VDEC_MT8195:
>+ cfg->max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH_10;
>+ break;
>+ default:
>+ cfg->max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH;
>+ break;
>+ };
>+}
>+
> static void mtk_vcodec_dec_fill_h265_level(struct v4l2_ctrl_config *cfg,
> struct mtk_vcodec_dec_ctx *ctx)
> {
>@@ -585,6 +599,11 @@ static void mtk_vcodec_dec_reset_controls(struct v4l2_ctrl_config *cfg,
> mtk_vcodec_dec_fill_h265_level(cfg, ctx);
> mtk_v4l2_vdec_dbg(3, ctx, "h265 supported level: %lld %lld", cfg->max, cfg->def);
> break;
>+ case V4L2_CID_MPEG_VIDEO_H264_PROFILE:
>+ mtk_vcodec_dec_fill_h264_profile(cfg, ctx);
>+ mtk_v4l2_vdec_dbg(3, ctx, "h264 supported profile: %lld %lld", cfg->max,
>+ cfg->menu_skip_mask);
>+ break;
> default:
> break;
> };
>--
>2.18.0
>
Hey Yunfei,
please replace "Getting" with "Get" in the title and replace "of each"
with "for each".
On 21.10.2023 10:30, Sebastian Fricke wrote:
>Hey Yunfei,
>
>Thanks for your patches!
>
>Could you provide a cover-letter for the next version please?
>This will help to get a good context of why we need these changes and to
>store the changelog in a helpful manner.
>Thanks.
>
>On 16.10.2023 14:43, Yunfei Dong wrote:
>>Getting the chip name of each platform according to the device
>>compatible to set different parameter.
>
>I would reword this commit description slightly, basically what you
>change is that you store the chip name in context permanently and that
>you utilize a enum to be more descriptive.
>
>So how about:
>
>"""
>Store the name of the chip in the context of the driver in order to be
>able to choose the correct configuration values for the different codecs.
>Use a enum value instead of an integer to store a more descriptive name.
>"""
>
>A few more comments below.
>
>>
>>Signed-off-by: Yunfei Dong <[email protected]>
>>---
>>.../mediatek/vcodec/decoder/mtk_vcodec_dec.c | 24 +----------------
>>.../vcodec/decoder/mtk_vcodec_dec_drv.c | 26 +++++++++++++++++++
>>.../vcodec/decoder/mtk_vcodec_dec_drv.h | 17 ++++++++++++
>>3 files changed, 44 insertions(+), 23 deletions(-)
>>
>>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
>>index 91ed576d6821..ba742f0e391d 100644
>>--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
>>+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
>>@@ -208,36 +208,14 @@ static int vidioc_vdec_dqbuf(struct file *file, void *priv,
>> return v4l2_m2m_dqbuf(file, ctx->m2m_ctx, buf);
>>}
>>
>>-static int mtk_vcodec_dec_get_chip_name(void *priv)
>>-{
>>- struct mtk_vcodec_dec_ctx *ctx = fh_to_dec_ctx(priv);
>>- struct device *dev = &ctx->dev->plat_dev->dev;
>>-
>>- if (of_device_is_compatible(dev->of_node, "mediatek,mt8173-vcodec-dec"))
>>- return 8173;
>>- else if (of_device_is_compatible(dev->of_node, "mediatek,mt8183-vcodec-dec"))
>>- return 8183;
>>- else if (of_device_is_compatible(dev->of_node, "mediatek,mt8192-vcodec-dec"))
>>- return 8192;
>>- else if (of_device_is_compatible(dev->of_node, "mediatek,mt8195-vcodec-dec"))
>>- return 8195;
>>- else if (of_device_is_compatible(dev->of_node, "mediatek,mt8186-vcodec-dec"))
>>- return 8186;
>>- else if (of_device_is_compatible(dev->of_node, "mediatek,mt8188-vcodec-dec"))
>>- return 8188;
>>- else
>>- return 8173;
>>-}
>>-
>>static int vidioc_vdec_querycap(struct file *file, void *priv,
>> struct v4l2_capability *cap)
>>{
>> struct mtk_vcodec_dec_ctx *ctx = fh_to_dec_ctx(priv);
>> struct device *dev = &ctx->dev->plat_dev->dev;
>>- int platform_name = mtk_vcodec_dec_get_chip_name(priv);
>>
>> strscpy(cap->driver, dev->driver->name, sizeof(cap->driver));
>>- snprintf(cap->card, sizeof(cap->card), "MT%d video decoder", platform_name);
>>+ snprintf(cap->card, sizeof(cap->card), "MT%d video decoder", ctx->dev->chip_name);
>>
>> return 0;
>>}
>>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
>>index 0a89ce452ac3..f47c98faf068 100644
>>--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
>>+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
>>@@ -326,6 +326,26 @@ static const struct v4l2_file_operations mtk_vcodec_fops = {
>> .mmap = v4l2_m2m_fop_mmap,
>>};
>>
>>+static void mtk_vcodec_dec_get_chip_name(struct mtk_vcodec_dec_dev *vdec_dev)
>>+{
>>+ struct device *dev = &vdec_dev->plat_dev->dev;
>>+
>>+ if (of_device_is_compatible(dev->of_node, "mediatek,mt8173-vcodec-dec"))
>>+ vdec_dev->chip_name = MTK_VDEC_MT8173;
>>+ else if (of_device_is_compatible(dev->of_node, "mediatek,mt8183-vcodec-dec"))
>>+ vdec_dev->chip_name = MTK_VDEC_MT8183;
>>+ else if (of_device_is_compatible(dev->of_node, "mediatek,mt8192-vcodec-dec"))
>>+ vdec_dev->chip_name = MTK_VDEC_MT8192;
>>+ else if (of_device_is_compatible(dev->of_node, "mediatek,mt8195-vcodec-dec"))
>>+ vdec_dev->chip_name = MTK_VDEC_MT8195;
>>+ else if (of_device_is_compatible(dev->of_node, "mediatek,mt8186-vcodec-dec"))
>>+ vdec_dev->chip_name = MTK_VDEC_MT8186;
>>+ else if (of_device_is_compatible(dev->of_node, "mediatek,mt8188-vcodec-dec"))
>>+ vdec_dev->chip_name = MTK_VDEC_MT8188;
>>+ else
>>+ vdec_dev->chip_name = MTK_VDEC_INVAL;
>>+}
>>+
>>static int mtk_vcodec_probe(struct platform_device *pdev)
>>{
>> struct mtk_vcodec_dec_dev *dev;
>>@@ -341,6 +361,12 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
>> INIT_LIST_HEAD(&dev->ctx_list);
>> dev->plat_dev = pdev;
>>
>>+ mtk_vcodec_dec_get_chip_name(dev);
>>+ if (dev->chip_name == MTK_VDEC_INVAL) {
>>+ dev_err(&pdev->dev, "Failed to get decoder chip name");
>>+ return -EINVAL;
>>+ }
>>+
>> dev->vdec_pdata = of_device_get_match_data(&pdev->dev);
>> if (!of_property_read_u32(pdev->dev.of_node, "mediatek,vpu",
>> &rproc_phandle)) {
>>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
>>index 7e36b2c69b7d..8f228ba9aa47 100644
>>--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
>>+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
>>@@ -18,6 +18,19 @@
>>#define IS_VDEC_LAT_ARCH(hw_arch) ((hw_arch) >= MTK_VDEC_LAT_SINGLE_CORE)
>>#define IS_VDEC_INNER_RACING(capability) ((capability) & MTK_VCODEC_INNER_RACING)
>>
>>+/*
>>+ * enum mtk_vcodec_dec_chip_name - Structure used to separate different platform
>>+ */
>
>I don't feel like this comment is terribly helpful because it is pretty
>clear what the enum is about, I would just drop it.
>
>>+enum mtk_vcodec_dec_chip_name {
>>+ MTK_VDEC_INVAL = 0,
>>+ MTK_VDEC_MT8173 = 8173,
>>+ MTK_VDEC_MT8183 = 8183,
>>+ MTK_VDEC_MT8186 = 8186,
>>+ MTK_VDEC_MT8188 = 8188,
>>+ MTK_VDEC_MT8192 = 8192,
>>+ MTK_VDEC_MT8195 = 8195,
>>+};
>>+
>>/*
>> * enum mtk_vdec_format_types - Structure used to get supported
>> * format types according to decoder capability
>>@@ -249,6 +262,8 @@ struct mtk_vcodec_dec_ctx {
>> * @vdec_racing_info: record register value
>> * @dec_racing_info_mutex: mutex lock used for inner racing mode
>> * @dbgfs: debug log related information
>>+ *
>>+ * @chip_name: the chip name used to separate different platform
>
>I wouldn't repeat chip name in the description and specify more
>concretely why we need to separate the platforms.
>
>My suggestion:
>
> * @chip_name: used to distinguish platforms and select the correct codec configuration values.
>
>> */
>>struct mtk_vcodec_dec_dev {
>> struct v4l2_device v4l2_dev;
>>@@ -289,6 +304,8 @@ struct mtk_vcodec_dec_dev {
>> /* Protects access to vdec_racing_info data */
>> struct mutex dec_racing_info_mutex;
>> struct mtk_vcodec_dbgfs dbgfs;
>>+
>>+ enum mtk_vcodec_dec_chip_name chip_name;
>>};
>>
>>static inline struct mtk_vcodec_dec_ctx *fh_to_dec_ctx(struct v4l2_fh *fh)
>
>Besides those small wording choices, the patch looks good.
>
>So with these issues resolved:
>
>Reviewed-by: Sebastian Fricke <[email protected]>
>
>Regards,
>Sebastian
>>--
>>2.18.0
>>
Hey Yunfei,
please replace Setting with Set in the title.
On 21.10.2023 11:29, Sebastian Fricke wrote:
>Hey Yunfei,
>
>On 16.10.2023 14:43, Yunfei Dong wrote:
>>The supported format type of different platforms are not the
>>same. Need to set the supported profile according to the chip name.
>
>I would suggest the following rewording:
>
>Set the maximum H264 codec profile for each platform.
>The various mediatek platforms support different profiles for decoding,
>the profile of the codec limits the capabilities for decoding.
>
>With that you can add:
>Reviewed-by: Sebastian Fricke <[email protected]>
>
>Regards,
>Sebastian
>
>>
>>Signed-off-by: Yunfei Dong <[email protected]>
>>---
>>.../vcodec/decoder/mtk_vcodec_dec_stateless.c | 19 +++++++++++++++++++
>>1 file changed, 19 insertions(+)
>>
>>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>>index 1fdb21dbacb8..84c0bed577ed 100644
>>--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>>+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>>@@ -559,6 +559,20 @@ static void mtk_vcodec_dec_fill_h264_level(struct v4l2_ctrl_config *cfg,
>> };
>>}
>>
>>+static void mtk_vcodec_dec_fill_h264_profile(struct v4l2_ctrl_config *cfg,
>>+ struct mtk_vcodec_dec_ctx *ctx)
>>+{
>>+ switch (ctx->dev->chip_name) {
>>+ case MTK_VDEC_MT8188:
>>+ case MTK_VDEC_MT8195:
>>+ cfg->max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH_10;
>>+ break;
>>+ default:
>>+ cfg->max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH;
>>+ break;
>>+ };
>>+}
>>+
>>static void mtk_vcodec_dec_fill_h265_level(struct v4l2_ctrl_config *cfg,
>> struct mtk_vcodec_dec_ctx *ctx)
>>{
>>@@ -585,6 +599,11 @@ static void mtk_vcodec_dec_reset_controls(struct v4l2_ctrl_config *cfg,
>> mtk_vcodec_dec_fill_h265_level(cfg, ctx);
>> mtk_v4l2_vdec_dbg(3, ctx, "h265 supported level: %lld %lld", cfg->max, cfg->def);
>> break;
>>+ case V4L2_CID_MPEG_VIDEO_H264_PROFILE:
>>+ mtk_vcodec_dec_fill_h264_profile(cfg, ctx);
>>+ mtk_v4l2_vdec_dbg(3, ctx, "h264 supported profile: %lld %lld", cfg->max,
>>+ cfg->menu_skip_mask);
>>+ break;
>> default:
>> break;
>> };
>>--
>>2.18.0
>>
Hi Sebastian,
Thanks for your advice.
Send the patch v2 to fix the below items:
Fix the commit message for some patches.
Fix the level of mt8195 and mt8192.
Best Regards,
Yunfei Dong
On Sat, 2023-10-21 at 10:30 +0200, Sebastian Fricke wrote:
> Hey Yunfei,
>
> Thanks for your patches!
>
> Could you provide a cover-letter for the next version please?
> This will help to get a good context of why we need these changes and
> to
> store the changelog in a helpful manner.
> Thanks.
>
> On 16.10.2023 14:43, Yunfei Dong wrote:
> > Getting the chip name of each platform according to the device
> > compatible to set different parameter.
>
> I would reword this commit description slightly, basically what you
> change is that you store the chip name in context permanently and
> that
> you utilize a enum to be more descriptive.
>
> So how about:
>
> """
> Store the name of the chip in the context of the driver in order to
> be
> able to choose the correct configuration values for the different
> codecs.
> Use a enum value instead of an integer to store a more descriptive
> name.
> """
>
> A few more comments below.
>
> >
> > Signed-off-by: Yunfei Dong <[email protected]>
> > ---
> > .../mediatek/vcodec/decoder/mtk_vcodec_dec.c | 24 +---------------
> > -
> > .../vcodec/decoder/mtk_vcodec_dec_drv.c | 26
> > +++++++++++++++++++
> > .../vcodec/decoder/mtk_vcodec_dec_drv.h | 17 ++++++++++++
> > 3 files changed, 44 insertions(+), 23 deletions(-)
> >
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
> > index 91ed576d6821..ba742f0e391d 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
> > @@ -208,36 +208,14 @@ static int vidioc_vdec_dqbuf(struct file
> > *file, void *priv,
> > return v4l2_m2m_dqbuf(file, ctx->m2m_ctx, buf);
> > }
> >
> > -static int mtk_vcodec_dec_get_chip_name(void *priv)
> > -{
> > - struct mtk_vcodec_dec_ctx *ctx = fh_to_dec_ctx(priv);
> > - struct device *dev = &ctx->dev->plat_dev->dev;
> > -
> > - if (of_device_is_compatible(dev->of_node, "mediatek,mt8173-
> > vcodec-dec"))
> > - return 8173;
> > - else if (of_device_is_compatible(dev->of_node,
> > "mediatek,mt8183-vcodec-dec"))
> > - return 8183;
> > - else if (of_device_is_compatible(dev->of_node,
> > "mediatek,mt8192-vcodec-dec"))
> > - return 8192;
> > - else if (of_device_is_compatible(dev->of_node,
> > "mediatek,mt8195-vcodec-dec"))
> > - return 8195;
> > - else if (of_device_is_compatible(dev->of_node,
> > "mediatek,mt8186-vcodec-dec"))
> > - return 8186;
> > - else if (of_device_is_compatible(dev->of_node,
> > "mediatek,mt8188-vcodec-dec"))
> > - return 8188;
> > - else
> > - return 8173;
> > -}
> > -
> > static int vidioc_vdec_querycap(struct file *file, void *priv,
> > struct v4l2_capability *cap)
> > {
> > struct mtk_vcodec_dec_ctx *ctx = fh_to_dec_ctx(priv);
> > struct device *dev = &ctx->dev->plat_dev->dev;
> > - int platform_name = mtk_vcodec_dec_get_chip_name(priv);
> >
> > strscpy(cap->driver, dev->driver->name, sizeof(cap->driver));
> > - snprintf(cap->card, sizeof(cap->card), "MT%d video decoder",
> > platform_name);
> > + snprintf(cap->card, sizeof(cap->card), "MT%d video decoder",
> > ctx->dev->chip_name);
> >
> > return 0;
> > }
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv
> > .c
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv
> > .c
> > index 0a89ce452ac3..f47c98faf068 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv
> > .c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv
> > .c
> > @@ -326,6 +326,26 @@ static const struct v4l2_file_operations
> > mtk_vcodec_fops = {
> > .mmap = v4l2_m2m_fop_mmap,
> > };
> >
> > +static void mtk_vcodec_dec_get_chip_name(struct mtk_vcodec_dec_dev
> > *vdec_dev)
> > +{
> > + struct device *dev = &vdec_dev->plat_dev->dev;
> > +
> > + if (of_device_is_compatible(dev->of_node, "mediatek,mt8173-
> > vcodec-dec"))
> > + vdec_dev->chip_name = MTK_VDEC_MT8173;
> > + else if (of_device_is_compatible(dev->of_node,
> > "mediatek,mt8183-vcodec-dec"))
> > + vdec_dev->chip_name = MTK_VDEC_MT8183;
> > + else if (of_device_is_compatible(dev->of_node,
> > "mediatek,mt8192-vcodec-dec"))
> > + vdec_dev->chip_name = MTK_VDEC_MT8192;
> > + else if (of_device_is_compatible(dev->of_node,
> > "mediatek,mt8195-vcodec-dec"))
> > + vdec_dev->chip_name = MTK_VDEC_MT8195;
> > + else if (of_device_is_compatible(dev->of_node,
> > "mediatek,mt8186-vcodec-dec"))
> > + vdec_dev->chip_name = MTK_VDEC_MT8186;
> > + else if (of_device_is_compatible(dev->of_node,
> > "mediatek,mt8188-vcodec-dec"))
> > + vdec_dev->chip_name = MTK_VDEC_MT8188;
> > + else
> > + vdec_dev->chip_name = MTK_VDEC_INVAL;
> > +}
> > +
> > static int mtk_vcodec_probe(struct platform_device *pdev)
> > {
> > struct mtk_vcodec_dec_dev *dev;
> > @@ -341,6 +361,12 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > INIT_LIST_HEAD(&dev->ctx_list);
> > dev->plat_dev = pdev;
> >
> > + mtk_vcodec_dec_get_chip_name(dev);
> > + if (dev->chip_name == MTK_VDEC_INVAL) {
> > + dev_err(&pdev->dev, "Failed to get decoder chip name");
> > + return -EINVAL;
> > + }
> > +
> > dev->vdec_pdata = of_device_get_match_data(&pdev->dev);
> > if (!of_property_read_u32(pdev->dev.of_node, "mediatek,vpu",
> > &rproc_phandle)) {
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv
> > .h
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv
> > .h
> > index 7e36b2c69b7d..8f228ba9aa47 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv
> > .h
> > +++
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv
> > .h
> > @@ -18,6 +18,19 @@
> > #define IS_VDEC_LAT_ARCH(hw_arch) ((hw_arch) >=
> > MTK_VDEC_LAT_SINGLE_CORE)
> > #define IS_VDEC_INNER_RACING(capability) ((capability) &
> > MTK_VCODEC_INNER_RACING)
> >
> > +/*
> > + * enum mtk_vcodec_dec_chip_name - Structure used to separate
> > different platform
> > + */
>
> I don't feel like this comment is terribly helpful because it is
> pretty
> clear what the enum is about, I would just drop it.
>
> > +enum mtk_vcodec_dec_chip_name {
> > + MTK_VDEC_INVAL = 0,
> > + MTK_VDEC_MT8173 = 8173,
> > + MTK_VDEC_MT8183 = 8183,
> > + MTK_VDEC_MT8186 = 8186,
> > + MTK_VDEC_MT8188 = 8188,
> > + MTK_VDEC_MT8192 = 8192,
> > + MTK_VDEC_MT8195 = 8195,
> > +};
> > +
> > /*
> > * enum mtk_vdec_format_types - Structure used to get supported
> > * format types according to decoder capability
> > @@ -249,6 +262,8 @@ struct mtk_vcodec_dec_ctx {
> > * @vdec_racing_info: record register value
> > * @dec_racing_info_mutex: mutex lock used for inner racing mode
> > * @dbgfs: debug log related information
> > + *
> > + * @chip_name: the chip name used to separate different platform
>
> I wouldn't repeat chip name in the description and specify more
> concretely why we need to separate the platforms.
>
> My suggestion:
>
> * @chip_name: used to distinguish platforms and select the correct
> codec configuration values.
>
> > */
> > struct mtk_vcodec_dec_dev {
> > struct v4l2_device v4l2_dev;
> > @@ -289,6 +304,8 @@ struct mtk_vcodec_dec_dev {
> > /* Protects access to vdec_racing_info data */
> > struct mutex dec_racing_info_mutex;
> > struct mtk_vcodec_dbgfs dbgfs;
> > +
> > + enum mtk_vcodec_dec_chip_name chip_name;
> > };
> >
> > static inline struct mtk_vcodec_dec_ctx *fh_to_dec_ctx(struct
> > v4l2_fh *fh)
>
> Besides those small wording choices, the patch looks good.
>
> So with these issues resolved:
>
> Reviewed-by: Sebastian Fricke <[email protected]>
>
> Regards,
> Sebastian
> > --
> > 2.18.0
> >