Messages in this thread | | | Date | Sat, 21 Oct 2023 10:30:15 +0200 | From | Sebastian Fricke <> | Subject | Re: [PATCH 1/7] media: mediatek: vcodec: Getting the chip name of each platform |
| |
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 <yunfei.dong@mediatek.com> >--- > .../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 <sebastian.fricke@collabora.com>
Regards, Sebastian >-- >2.18.0 >
| |