This series adds support for multi hardware decode into mtk-vcodec, by first adding use
of_platform_populate to manage each hardware information: interrupt, clock, register
bases and power. Secondly add core work queue to deal with core hardware message,
at the same time, add msg queue for different hardware share messages. Lastly, the
architecture of different specs are not the same, using specs type to separate them.
This series has been tested with both MT8183 and MT8173. Decoding was working for both chips.
Patches 1~3 rewrite get register bases and power on/off interface.
Patches 4 build decoder pm file to module.
Patches 5 add to support 8192.
Patch 6 support multi hardware.
Patch 7 separate video encoder and decoder document
Patch 8-17 add interfaces to support core hardware.
Patch 18-19 remove mtk_vcodec_release_dec/enc_pm interfaces.
---
changes compared with v8:
- add new patch 18~19 to remove mtk_vcodec_release_de/enc_pm interfaces.
- fix spelling mistakes for patch 17/19
- fix yaml comments for patch 15/19
Changes compared with v7:
- add new patch 4 to build decoder pm file as module
- add new patch 5 to support 8192
- fix comments for patch 6/17
- change some logic for using work queue instead of create thread for core hardware decode for patch 10/17
- using work queue for hardware decode instead of create thread for patch 13/17
- add returen value for patch 14/17
- fix yaml check fail 15/17
Changes compared with v6:
- Use of_platform_populate to manage multi hardware, not component framework for patch 4/15
- Re-write dtsi document for hardware architecture changed for patch 13/15 -The dtsi will write like below in patch 13/15:
vcodec_dec: vcodec_dec@16000000 {
compatible = "mediatek,mt8192-vcodec-dec";
#address-cells = <2>;
#size-cells = <2>;
ranges;
reg = <0 0x16000000 0 0x1000>; /* VDEC_SYS */
mediatek,scp = <&scp>;
iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>;
dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>;
vcodec_lat {
compatible = "mediatek,mtk-vcodec-lat";
reg = <0 0x16010000 0 0x800>; /* VDEC_MISC */
reg-name = "reg-misc";
interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>;
iommus = <&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD_EXT>,
<&iommu0 M4U_PORT_L5_VDEC_LAT0_VLD2_EXT>,
<&iommu0 M4U_PORT_L5_VDEC_LAT0_AVC_MV_EXT>,
<&iommu0 M4U_PORT_L5_VDEC_LAT0_PRED_RD_EXT>,
<&iommu0 M4U_PORT_L5_VDEC_LAT0_TILE_EXT>,
<&iommu0 M4U_PORT_L5_VDEC_LAT0_WDMA_EXT>,
<&iommu0 M4U_PORT_L5_VDEC_LAT0_RG_CTRL_DMA_EXT>,
<&iommu0 M4U_PORT_L5_VDEC_UFO_ENC_EXT>;
clocks = <&topckgen CLK_TOP_VDEC_SEL>,
<&vdecsys_soc CLK_VDEC_SOC_VDEC>,
<&vdecsys_soc CLK_VDEC_SOC_LAT>,
<&vdecsys_soc CLK_VDEC_SOC_LARB1>,
<&topckgen CLK_TOP_MAINPLL_D4>;
clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-lat",
"vdec-vdec", "vdec-top";
assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>;
assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>;
power-domains = <&spm MT8192_POWER_DOMAIN_VDEC>;
};
vcodec_core {
compatible = "mediatek,mtk-vcodec-core";
reg = <0 0x16025000 0 0x1000>; /* VDEC_CORE_MISC */
reg-names = "reg-misc";
interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>;
iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>,
<&iommu0 M4U_PORT_L4_VDEC_UFO_EXT>,
<&iommu0 M4U_PORT_L4_VDEC_PP_EXT>,
<&iommu0 M4U_PORT_L4_VDEC_PRED_RD_EXT>,
<&iommu0 M4U_PORT_L4_VDEC_PRED_WR_EXT>,
<&iommu0 M4U_PORT_L4_VDEC_PPWRAP_EXT>,
<&iommu0 M4U_PORT_L4_VDEC_TILE_EXT>,
<&iommu0 M4U_PORT_L4_VDEC_VLD_EXT>,
<&iommu0 M4U_PORT_L4_VDEC_VLD2_EXT>,
<&iommu0 M4U_PORT_L4_VDEC_AVC_MV_EXT>,
<&iommu0 M4U_PORT_L4_VDEC_RG_CTRL_DMA_EXT>;
clocks = <&topckgen CLK_TOP_VDEC_SEL>,
<&vdecsys CLK_VDEC_VDEC>,
<&vdecsys CLK_VDEC_LAT>,
<&vdecsys CLK_VDEC_LARB1>,
<&topckgen CLK_TOP_MAINPLL_D4>;
clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-lat",
"vdec-vdec", "vdec-top";
assigned-clocks = <&topckgen CLK_TOP_VDEC_SEL>;
assigned-clock-parents = <&topckgen CLK_TOP_MAINPLL_D4>;
power-domains = <&spm MT8192_POWER_DOMAIN_VDEC2>;
};
};
Changes compared with v5:
- Add decoder hardware block diagram for patch 13/15
Changes compared with v4:
- Fix comments for patch 4/15
>> + if (dev->is_comp_supported) {
>> + ret = mtk_vcodec_init_master(dev);
>> + if (ret < 0)
>> + goto err_component_match;
>> + } else {
>> + platform_set_drvdata(pdev, dev);
>> + }
Fix platform_set_drvdata.
- Fix build error for patch 9/15
- Add depend patch in case of error header file for patch 13/15
Changes compared with v3:
- Fix return value for patch 1/15
- Fix comments for patch 4/15
> Looking up "mediatek,mtk-vcodec-core" to determine if it uses component framwork sounds like...
Add prameter in pdata, for all platform will use compoent after mt8183
>> + if (dev->is_comp_supported) {
>> + ret = mtk_vcodec_init_master(dev);
>> + if (ret < 0)
>> + goto err_component_match;
>> + } else {
>> + platform_set_drvdata(pdev, dev);
>> + }
> + Has asked the same question in [1]. Why it removes the
> +platform_set_drvdata() above? mtk_vcodec_init_master() also calls platform_set_drvdata().
Must call component_master_add_with_match after platform_set_drvdata for component architecture.
- Fix yaml files check fail for patch 5/15
- Fix yaml file check fail for patch 14/15
Changes compared with v1:
- Fix many comments for patch 3/14
- Remove unnecessary code for patch 4/14
- Using enum mtk_vdec_hw_count instead of magic numbers for patch 6/14
- Reconstructed get/put lat buffer for lat and core hardware for patch 7/14
- Using yaml format to instead of txt file for patch 12/14
Yunfei Dong (19):
media: mtk-vcodec: Get numbers of register bases from DT
media: mtk-vcodec: Align vcodec wake up interrupt interface
media: mtk-vcodec: Refactor vcodec pm interface
media: mtk-vcodec: Build decoder pm file as module
media: mtk-vcodec: Support MT8192
media: mtk-vcodec: Manage multi hardware information
dt-bindings: media: mtk-vcodec: Separate video encoder and decoder
dt-bindings
media: mtk-vcodec: Use pure single core for MT8183
media: mtk-vcodec: Add irq interface for multi hardware
media: mtk-vcodec: Add msg queue feature for lat and core architecture
media: mtk-vcodec: Generalize power and clock on/off interfaces
media: mtk-vcodec: Add new interface to lock different hardware
media: mtk-vcodec: Add work queue for core hardware decode
media: mtk-vcodec: Support 34bits dma address for vdec
dt-bindings: media: mtk-vcodec: Adds decoder dt-bindings for mt8192
media: mtk-vcodec: Add core dec and dec end ipi msg
media: mtk-vcodec: Use codec type to separate different hardware
media: mtk-vcodec: Remove mtk_vcodec_release_dec_pm
media: mtk-vcodec: Remove mtk_vcodec_release_enc_pm
.../media/mediatek,vcodec-decoder.yaml | 176 +++++++++++
.../media/mediatek,vcodec-encoder.yaml | 187 ++++++++++++
.../media/mediatek,vcodec-subdev-decoder.yaml | 261 ++++++++++++++++
.../bindings/media/mediatek-vcodec.txt | 131 --------
drivers/media/platform/mtk-vcodec/Makefile | 10 +-
.../platform/mtk-vcodec/mtk_vcodec_dec.c | 4 +-
.../platform/mtk-vcodec/mtk_vcodec_dec.h | 1 +
.../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 216 ++++++++++---
.../platform/mtk-vcodec/mtk_vcodec_dec_hw.c | 169 +++++++++++
.../platform/mtk-vcodec/mtk_vcodec_dec_hw.h | 55 ++++
.../platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 106 +++++--
.../platform/mtk-vcodec/mtk_vcodec_dec_pm.h | 12 +-
.../mtk-vcodec/mtk_vcodec_dec_stateful.c | 2 +
.../mtk-vcodec/mtk_vcodec_dec_stateless.c | 20 ++
.../platform/mtk-vcodec/mtk_vcodec_drv.h | 75 ++++-
.../platform/mtk-vcodec/mtk_vcodec_enc_drv.c | 21 +-
.../platform/mtk-vcodec/mtk_vcodec_enc_pm.c | 10 +-
.../platform/mtk-vcodec/mtk_vcodec_enc_pm.h | 3 +-
.../platform/mtk-vcodec/mtk_vcodec_intr.c | 27 +-
.../platform/mtk-vcodec/mtk_vcodec_intr.h | 4 +-
.../platform/mtk-vcodec/mtk_vcodec_util.c | 87 +++++-
.../platform/mtk-vcodec/mtk_vcodec_util.h | 8 +-
.../platform/mtk-vcodec/vdec/vdec_h264_if.c | 2 +-
.../mtk-vcodec/vdec/vdec_h264_req_if.c | 2 +-
.../platform/mtk-vcodec/vdec/vdec_vp8_if.c | 2 +-
.../platform/mtk-vcodec/vdec/vdec_vp9_if.c | 2 +-
.../media/platform/mtk-vcodec/vdec_drv_if.c | 21 +-
.../media/platform/mtk-vcodec/vdec_ipi_msg.h | 16 +-
.../platform/mtk-vcodec/vdec_msg_queue.c | 286 ++++++++++++++++++
.../platform/mtk-vcodec/vdec_msg_queue.h | 148 +++++++++
.../media/platform/mtk-vcodec/vdec_vpu_if.c | 46 ++-
.../media/platform/mtk-vcodec/vdec_vpu_if.h | 22 ++
.../platform/mtk-vcodec/venc/venc_h264_if.c | 2 +-
.../platform/mtk-vcodec/venc/venc_vp8_if.c | 2 +-
34 files changed, 1840 insertions(+), 296 deletions(-)
create mode 100644 Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
create mode 100644 Documentation/devicetree/bindings/media/mediatek,vcodec-encoder.yaml
create mode 100644 Documentation/devicetree/bindings/media/mediatek,vcodec-subdev-decoder.yaml
delete mode 100644 Documentation/devicetree/bindings/media/mediatek-vcodec.txt
create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h
create mode 100644 drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
create mode 100644 drivers/media/platform/mtk-vcodec/vdec_msg_queue.h
--
2.25.1
There are only two lines in mtk_vcodec_release_dec_pm, using
pm_runtime_disable and put_device instead directly.
Move pm_runtime_enable outside mtk_vcodec_init_dec_pm to symmetry with
pm_runtime_disable, after that, rename mtk_vcodec_init_dec_pm to *_clk since
it only has clock operations now.
Signed-off-by: Yunfei Dong <[email protected]>
Co-developed-by: Yong Wu <[email protected]>
---
new patch to remove: mtk_vcodec_release_dec_pm
---
.../media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 10 +++++++---
drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c | 7 +++++--
drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 11 ++---------
drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h | 3 +--
4 files changed, 15 insertions(+), 16 deletions(-)
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
index affef6c7c8b5..2f3530eeaa6a 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
@@ -11,6 +11,7 @@
#include <linux/module.h>
#include <linux/of_device.h>
#include <linux/of.h>
+#include <linux/pm_runtime.h>
#include <media/v4l2-event.h>
#include <media/v4l2-mem2mem.h>
#include <media/videobuf2-dma-contig.h>
@@ -164,11 +165,12 @@ static int mtk_vcodec_init_dec_params(struct mtk_vcodec_dev *dev)
return ret;
}
- ret = mtk_vcodec_init_dec_pm(pdev, &dev->pm);
+ ret = mtk_vcodec_init_dec_clk(pdev, &dev->pm);
if (ret < 0) {
dev_err(&pdev->dev, "failed to get mt vcodec clock source");
return ret;
}
+ pm_runtime_enable(&pdev->dev);
}
return 0;
@@ -482,7 +484,8 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
if (IS_VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch))
destroy_workqueue(dev->core_workqueue);
err_res:
- mtk_vcodec_release_dec_pm(&dev->pm);
+ pm_runtime_disable(dev->pm.dev);
+ put_device(dev->pm.larbvdec);
err_dec_pm:
mtk_vcodec_fw_release(dev->fw_handler);
return ret;
@@ -526,7 +529,8 @@ static int mtk_vcodec_dec_remove(struct platform_device *pdev)
video_unregister_device(dev->vfd_dec);
v4l2_device_unregister(&dev->v4l2_dev);
- mtk_vcodec_release_dec_pm(&dev->pm);
+ pm_runtime_disable(dev->pm.dev);
+ put_device(dev->pm.larbvdec);
mtk_vcodec_fw_release(dev->fw_handler);
return 0;
}
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
index c3488e3fa6af..5f61e260a218 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
@@ -9,6 +9,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/pm_runtime.h>
#include <linux/slab.h>
#include "mtk_vcodec_drv.h"
@@ -113,9 +114,10 @@ static int mtk_vdec_hw_probe(struct platform_device *pdev)
return -ENOMEM;
subdev_dev->plat_dev = pdev;
- ret = mtk_vcodec_init_dec_pm(pdev, &subdev_dev->pm);
+ ret = mtk_vcodec_init_dec_clk(pdev, &subdev_dev->pm);
if (ret)
return ret;
+ pm_runtime_enable(&pdev->dev);
subdev_dev->reg_base[VDEC_HW_MISC] =
devm_platform_ioremap_resource_byname(pdev, "misc");
@@ -148,7 +150,8 @@ static int mtk_vdec_hw_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, subdev_dev);
return 0;
err:
- mtk_vcodec_release_dec_pm(&subdev_dev->pm);
+ pm_runtime_disable(subdev_dev->pm.dev);
+ put_device(subdev_dev->pm.larbvdec);
return ret;
}
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
index 6435ccb13d37..a54871325325 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
@@ -15,7 +15,7 @@
#include "mtk_vcodec_dec_pm.h"
#include "mtk_vcodec_util.h"
-int mtk_vcodec_init_dec_pm(struct platform_device *pdev,
+int mtk_vcodec_init_dec_clk(struct platform_device *pdev,
struct mtk_vcodec_pm *pm)
{
struct device_node *node;
@@ -79,14 +79,7 @@ int mtk_vcodec_init_dec_pm(struct platform_device *pdev,
put_device(pm->larbvdec);
return ret;
}
-EXPORT_SYMBOL_GPL(mtk_vcodec_init_dec_pm);
-
-void mtk_vcodec_release_dec_pm(struct mtk_vcodec_pm *pm)
-{
- pm_runtime_disable(pm->dev);
- put_device(pm->larbvdec);
-}
-EXPORT_SYMBOL_GPL(mtk_vcodec_release_dec_pm);
+EXPORT_SYMBOL_GPL(mtk_vcodec_init_dec_clk);
int mtk_vcodec_dec_pw_on(struct mtk_vcodec_dev *vdec_dev, int hw_idx)
{
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
index 6ae29fea4e7f..c4121df9764f 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
@@ -9,9 +9,8 @@
#include "mtk_vcodec_drv.h"
-int mtk_vcodec_init_dec_pm(struct platform_device *pdev,
+int mtk_vcodec_init_dec_clk(struct platform_device *pdev,
struct mtk_vcodec_pm *pm);
-void mtk_vcodec_release_dec_pm(struct mtk_vcodec_pm *pm);
int mtk_vcodec_dec_pw_on(struct mtk_vcodec_dev *vdec_dev, int hw_idx);
void mtk_vcodec_dec_pw_off(struct mtk_vcodec_dev *vdec_dev, int hw_idx);
--
2.25.1
Manage each hardware information which includes irq/power/clk.
The hardware includes LAT0, LAT1 and CORE.
Signed-off-by: Yunfei Dong <[email protected]>
Reported-by: kernel test robot <[email protected]>
---
drivers/media/platform/mtk-vcodec/Makefile | 5 +-
.../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 119 +++++++++----
.../platform/mtk-vcodec/mtk_vcodec_dec_hw.c | 166 ++++++++++++++++++
.../platform/mtk-vcodec/mtk_vcodec_dec_hw.h | 51 ++++++
.../mtk-vcodec/mtk_vcodec_dec_stateful.c | 1 +
.../mtk-vcodec/mtk_vcodec_dec_stateless.c | 2 +
.../platform/mtk-vcodec/mtk_vcodec_drv.h | 23 +++
7 files changed, 334 insertions(+), 33 deletions(-)
create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h
diff --git a/drivers/media/platform/mtk-vcodec/Makefile b/drivers/media/platform/mtk-vcodec/Makefile
index 5d36e05535d7..95feb793c287 100644
--- a/drivers/media/platform/mtk-vcodec/Makefile
+++ b/drivers/media/platform/mtk-vcodec/Makefile
@@ -3,7 +3,8 @@
obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC) += mtk-vcodec-dec.o \
mtk-vcodec-enc.o \
mtk-vcodec-common.o \
- mtk-vcodec-dec-common.o
+ mtk-vcodec-dec-common.o \
+ mtk-vcodec-dec-hw.o
mtk-vcodec-dec-y := vdec/vdec_h264_if.o \
vdec/vdec_vp8_if.o \
@@ -16,6 +17,8 @@ mtk-vcodec-dec-y := vdec/vdec_h264_if.o \
mtk_vcodec_dec_stateful.o \
mtk_vcodec_dec_stateless.o \
+mtk-vcodec-dec-hw-y := mtk_vcodec_dec_hw.o
+
mtk-vcodec-dec-common-y := mtk_vcodec_dec_pm.o
mtk-vcodec-enc-y := venc/venc_vp8_if.o \
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
index b7a51e96d4ba..eb2af42aa102 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
@@ -18,19 +18,49 @@
#include "mtk_vcodec_drv.h"
#include "mtk_vcodec_dec.h"
+#include "mtk_vcodec_dec_hw.h"
#include "mtk_vcodec_dec_pm.h"
#include "mtk_vcodec_intr.h"
-#include "mtk_vcodec_util.h"
#include "mtk_vcodec_fw.h"
-#define VDEC_HW_ACTIVE 0x10
-#define VDEC_IRQ_CFG 0x11
-#define VDEC_IRQ_CLR 0x10
-#define VDEC_IRQ_CFG_REG 0xa4
-
module_param(mtk_v4l2_dbg_level, int, 0644);
module_param(mtk_vcodec_dbg, bool, 0644);
+static int mtk_vcodec_subdev_device_check(struct mtk_vcodec_ctx *ctx)
+ {
+ struct mtk_vcodec_dev *vdec_dev = ctx->dev;
+ struct platform_device *pdev = vdec_dev->plat_dev;
+ struct device_node *subdev_node;
+ enum mtk_vdec_hw_id hw_idx;
+ const struct of_device_id *of_id;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(mtk_vdec_hw_match); i++) {
+ of_id = &mtk_vdec_hw_match[i];
+ subdev_node = of_find_compatible_node(NULL, NULL,
+ of_id->compatible);
+ if (!subdev_node)
+ continue;
+
+ if (!of_device_is_available(subdev_node)) {
+ of_node_put(subdev_node);
+ dev_err(&pdev->dev, "Fail to get MMSYS node\n");
+ continue;
+ }
+
+ hw_idx = (enum mtk_vdec_hw_id)(uintptr_t)of_id->data;
+ vdec_dev->subdev_node[hw_idx] = subdev_node;
+
+ if (!test_bit(hw_idx, vdec_dev->hardware_bitmap)) {
+ dev_err(&pdev->dev, "Vdec hw_idx is not ready %d.",
+ hw_idx);
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
static irqreturn_t mtk_vcodec_dec_irq_handler(int irq, void *priv)
{
struct mtk_vcodec_dev *dev = priv;
@@ -95,6 +125,41 @@ static int mtk_vcodec_get_reg_bases(struct mtk_vcodec_dev *dev)
return 0;
}
+static int mtk_vcodec_init_dec_params(struct mtk_vcodec_dev *dev)
+{
+ struct platform_device *pdev = dev->plat_dev;
+ int ret;
+
+ ret = mtk_vcodec_get_reg_bases(dev);
+ if (ret)
+ return ret;
+
+ if (!dev->vdec_pdata->is_subdev_supported) {
+ dev->dec_irq = platform_get_irq(pdev, 0);
+ if (dev->dec_irq < 0) {
+ dev_err(&pdev->dev, "failed to get irq number");
+ return dev->dec_irq;
+ }
+
+ irq_set_status_flags(dev->dec_irq, IRQ_NOAUTOEN);
+ ret = devm_request_irq(&pdev->dev, dev->dec_irq,
+ mtk_vcodec_dec_irq_handler, 0, pdev->name, dev);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to install dev->dec_irq %d (%d)",
+ dev->dec_irq, ret);
+ return ret;
+ }
+
+ ret = mtk_vcodec_init_dec_pm(pdev, &dev->pm);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to get mt vcodec clock source");
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
static int fops_vcodec_open(struct file *file)
{
struct mtk_vcodec_dev *dev = video_drvdata(file);
@@ -116,6 +181,12 @@ static int fops_vcodec_open(struct file *file)
init_waitqueue_head(&ctx->queue);
mutex_init(&ctx->lock);
+ ret = mtk_vcodec_subdev_device_check(ctx);
+ if (ret) {
+ mtk_v4l2_err("Failed to check vdec comp device.");
+ goto err_ctrls_setup;
+ }
+
ctx->type = MTK_INST_DECODER;
ret = dev->vdec_pdata->ctrls_setup(ctx);
if (ret) {
@@ -220,7 +291,6 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
{
struct mtk_vcodec_dev *dev;
struct video_device *vfd_dec;
- struct resource *res;
phandle rproc_phandle;
enum mtk_vcodec_fw_type fw_type;
int ret;
@@ -249,32 +319,10 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
if (IS_ERR(dev->fw_handler))
return PTR_ERR(dev->fw_handler);
- ret = mtk_vcodec_init_dec_pm(dev->plat_dev, &dev->pm);
- if (ret < 0) {
- dev_err(&pdev->dev, "Failed to get mt vcodec clock source");
- goto err_dec_pm;
- }
-
- ret = mtk_vcodec_get_reg_bases(dev);
- if (ret)
- goto err_res;
-
- res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (res == NULL) {
- dev_err(&pdev->dev, "failed to get irq resource");
- ret = -ENOENT;
- goto err_res;
- }
-
- dev->dec_irq = platform_get_irq(pdev, 0);
- irq_set_status_flags(dev->dec_irq, IRQ_NOAUTOEN);
- ret = devm_request_irq(&pdev->dev, dev->dec_irq,
- mtk_vcodec_dec_irq_handler, 0, pdev->name, dev);
+ ret = mtk_vcodec_init_dec_params(dev);
if (ret) {
- dev_err(&pdev->dev, "Failed to install dev->dec_irq %d (%d)",
- dev->dec_irq,
- ret);
- goto err_res;
+ dev_err(&pdev->dev, "Failed to init pm and registers");
+ goto err_dec_pm;
}
mutex_init(&dev->dec_mutex);
@@ -329,6 +377,13 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
goto err_event_workq;
}
+ ret = of_platform_populate(pdev->dev.of_node, NULL, NULL,
+ &pdev->dev);
+ if (ret) {
+ mtk_v4l2_err("Master device of_platform_populate failed.");
+ goto err_event_workq;
+ }
+
if (dev->vdec_pdata->uses_stateless_api) {
dev->mdev_dec.dev = &pdev->dev;
strscpy(dev->mdev_dec.model, MTK_VCODEC_DEC_NAME,
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
new file mode 100644
index 000000000000..745be12548ef
--- /dev/null
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
@@ -0,0 +1,166 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2021 MediaTek Inc.
+ * Author: Yunfei Dong <[email protected]>
+ */
+
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/slab.h>
+
+#include "mtk_vcodec_drv.h"
+#include "mtk_vcodec_dec.h"
+#include "mtk_vcodec_dec_hw.h"
+#include "mtk_vcodec_dec_pm.h"
+#include "mtk_vcodec_intr.h"
+#include "mtk_vcodec_util.h"
+
+const struct of_device_id mtk_vdec_hw_match[] = {
+ {
+ .compatible = "mediatek,mtk-vcodec-lat",
+ .data = (void *)MTK_VDEC_LAT0,
+ },
+ {
+ .compatible = "mediatek,mtk-vcodec-core",
+ .data = (void *)MTK_VDEC_CORE,
+ },
+ {},
+};
+EXPORT_SYMBOL_GPL(mtk_vdec_hw_match);
+
+MODULE_DEVICE_TABLE(of, mtk_vdec_hw_match);
+
+static irqreturn_t mtk_vdec_hw_irq_handler(int irq, void *priv)
+{
+ struct mtk_vdec_hw_dev *dev = priv;
+ struct mtk_vcodec_ctx *ctx;
+ u32 cg_status;
+ unsigned int dec_done_status;
+ void __iomem *vdec_misc_addr = dev->reg_base[VDEC_HW_MISC] +
+ VDEC_IRQ_CFG_REG;
+
+ ctx = mtk_vcodec_get_curr_ctx(dev->main_dev);
+
+ /* check if HW active or not */
+ cg_status = readl(dev->reg_base[VDEC_HW_SYS]);
+ if (cg_status & VDEC_HW_ACTIVE) {
+ mtk_v4l2_err("vdec active is not 0x0 (0x%08x)",
+ cg_status);
+ return IRQ_HANDLED;
+ }
+
+ dec_done_status = readl(vdec_misc_addr);
+ if ((dec_done_status & MTK_VDEC_IRQ_STATUS_DEC_SUCCESS) !=
+ MTK_VDEC_IRQ_STATUS_DEC_SUCCESS)
+ return IRQ_HANDLED;
+
+ /* clear interrupt */
+ writel(dec_done_status | VDEC_IRQ_CFG, vdec_misc_addr);
+ writel(dec_done_status & ~VDEC_IRQ_CLR, vdec_misc_addr);
+
+ wake_up_ctx(ctx, MTK_INST_IRQ_RECEIVED);
+
+ mtk_v4l2_debug(3, "wake up ctx %d, dec_done_status=%x",
+ ctx->id, dec_done_status);
+
+ return IRQ_HANDLED;
+}
+
+static int mtk_vdec_hw_init_irq(struct mtk_vdec_hw_dev *dev)
+{
+ struct platform_device *pdev = dev->plat_dev;
+ int ret;
+
+ dev->dec_irq = platform_get_irq(pdev, 0);
+ if (dev->dec_irq < 0) {
+ dev_err(&pdev->dev, "Failed to get irq resource");
+ return dev->dec_irq;
+ }
+
+ irq_set_status_flags(dev->dec_irq, IRQ_NOAUTOEN);
+ ret = devm_request_irq(&pdev->dev, dev->dec_irq,
+ mtk_vdec_hw_irq_handler, 0, pdev->name, dev);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to install dev->dec_irq %d (%d)",
+ dev->dec_irq, ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int mtk_vdec_hw_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mtk_vdec_hw_dev *subdev_dev;
+ struct mtk_vcodec_dev *main_dev;
+ const struct of_device_id *of_id;
+ int hw_idx;
+ int ret;
+
+ if (!dev->parent)
+ return -EPROBE_DEFER;
+
+ main_dev = dev_get_drvdata(dev->parent);
+ if (!main_dev)
+ return -EPROBE_DEFER;
+
+ subdev_dev = devm_kzalloc(dev, sizeof(*subdev_dev), GFP_KERNEL);
+ if (!subdev_dev)
+ return -ENOMEM;
+
+ subdev_dev->plat_dev = pdev;
+ ret = mtk_vcodec_init_dec_pm(pdev, &subdev_dev->pm);
+ if (ret)
+ return ret;
+
+ subdev_dev->reg_base[VDEC_HW_MISC] =
+ devm_platform_ioremap_resource_byname(pdev, "misc");
+ if (IS_ERR((__force void *)subdev_dev->reg_base[VDEC_HW_MISC])) {
+ ret = PTR_ERR((__force void *)subdev_dev->reg_base[VDEC_HW_MISC]);
+ goto err;
+ }
+
+ ret = mtk_vdec_hw_init_irq(subdev_dev);
+ if (ret)
+ goto err;
+
+ of_id = of_match_device(mtk_vdec_hw_match, dev);
+ if (!of_id) {
+ dev_err(dev, "Can't get vdec comp device id.\n");
+ ret = -EINVAL;
+ goto err;
+ }
+
+ hw_idx = (enum mtk_vdec_hw_id)(uintptr_t)of_id->data;
+ if (hw_idx < MTK_VDEC_HW_MAX) {
+ main_dev->subdev_dev[hw_idx] = subdev_dev;
+ subdev_dev->hw_idx = hw_idx;
+ subdev_dev->main_dev = main_dev;
+ subdev_dev->reg_base[VDEC_HW_SYS] =
+ main_dev->reg_base[VDEC_HW_SYS];
+ set_bit(subdev_dev->hw_idx, main_dev->hardware_bitmap);
+ }
+
+ platform_set_drvdata(pdev, subdev_dev);
+ return 0;
+err:
+ mtk_vcodec_release_dec_pm(&subdev_dev->pm);
+ return ret;
+}
+
+static struct platform_driver mtk_vdec_driver = {
+ .probe = mtk_vdec_hw_probe,
+ .driver = {
+ .name = "mtk-vdec-comp",
+ .of_match_table = mtk_vdec_hw_match,
+ },
+};
+
+module_platform_driver(mtk_vdec_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Mediatek video decoder hardware driver");
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h
new file mode 100644
index 000000000000..f7f36790629d
--- /dev/null
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2021 MediaTek Inc.
+ * Author: Yunfei Dong <[email protected]>
+ */
+
+#ifndef _MTK_VCODEC_DEC_HW_H_
+#define _MTK_VCODEC_DEC_HW_H_
+
+#include <linux/io.h>
+#include <linux/platform_device.h>
+
+#include "mtk_vcodec_drv.h"
+
+#define VDEC_HW_ACTIVE 0x10
+#define VDEC_IRQ_CFG 0x11
+#define VDEC_IRQ_CLR 0x10
+#define VDEC_IRQ_CFG_REG 0xa4
+
+extern const struct of_device_id mtk_vdec_hw_match[MTK_VDEC_HW_MAX];
+
+/**
+ * enum mtk_vdec_hw_reg_idx - subdev hardware register base index
+ */
+enum mtk_vdec_hw_reg_idx {
+ VDEC_HW_SYS,
+ VDEC_HW_MISC,
+ VDEC_HW_MAX
+};
+
+/**
+ * struct mtk_vdec_hw_dev - vdec hardware driver data
+ * @plat_dev: platform device
+ * @main_dev: main device
+ * @reg_base: Mapped address of MTK Vcodec registers.
+ *
+ * @dec_irq: decoder irq resource
+ * @pm: power management control
+ * @hw_idx: each hardware index
+ */
+struct mtk_vdec_hw_dev {
+ struct platform_device *plat_dev;
+ struct mtk_vcodec_dev *main_dev;
+ void __iomem *reg_base[VDEC_HW_MAX];
+
+ int dec_irq;
+ struct mtk_vcodec_pm pm;
+ int hw_idx;
+};
+
+#endif /* _MTK_VCODEC_DEC_HW_H_ */
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateful.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateful.c
index bef49244e61b..c7f9259ad094 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateful.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateful.c
@@ -625,4 +625,5 @@ const struct mtk_vcodec_dec_pdata mtk_vdec_8173_pdata = {
.num_framesizes = NUM_SUPPORTED_FRAMESIZE,
.worker = mtk_vdec_worker,
.flush_decoder = mtk_vdec_flush_decoder,
+ .is_subdev_supported = false,
};
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateless.c
index 26e4d6f4ec04..2d285515b625 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateless.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateless.c
@@ -357,6 +357,7 @@ const struct mtk_vcodec_dec_pdata mtk_vdec_8183_pdata = {
.uses_stateless_api = true,
.worker = mtk_vdec_worker,
.flush_decoder = mtk_vdec_flush_decoder,
+ .is_subdev_supported = false,
};
const struct mtk_vcodec_dec_pdata mtk_lat_sig_core_pdata = {
@@ -373,4 +374,5 @@ const struct mtk_vcodec_dec_pdata mtk_lat_sig_core_pdata = {
.uses_stateless_api = true,
.worker = mtk_vdec_worker,
.flush_decoder = mtk_vdec_flush_decoder,
+ .is_subdev_supported = true,
};
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
index 0fa9d85114b9..b1fc7ebfeb71 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
@@ -93,6 +93,17 @@ enum mtk_fmt_type {
MTK_FMT_FRAME = 2,
};
+/**
+ * struct mtk_vdec_hw_id - Hardware index used to separate
+ * different hardware
+ */
+enum mtk_vdec_hw_id {
+ MTK_VDEC_CORE,
+ MTK_VDEC_LAT0,
+ MTK_VDEC_LAT1,
+ MTK_VDEC_HW_MAX,
+};
+
/*
* struct mtk_video_fmt - Structure used to store information about pixelformats
*/
@@ -332,6 +343,7 @@ enum mtk_chip {
*
* @chip: chip this decoder is compatible with
*
+ * @is_subdev_supported: whether support parent-node architecture(subdev)
* @uses_stateless_api: whether the decoder uses the stateless API with requests
*/
@@ -353,6 +365,7 @@ struct mtk_vcodec_dec_pdata {
enum mtk_chip chip;
+ bool is_subdev_supported;
bool uses_stateless_api;
};
@@ -423,6 +436,11 @@ struct mtk_vcodec_enc_pdata {
* @pm: power management control
* @dec_capability: used to identify decode capability, ex: 4k
* @enc_capability: used to identify encode capability
+ *
+ * @subdev_dev: subdev hardware device
+ * @subdev_node: subdev node
+ *
+ * @hardware_bitmap: used to record hardware is ready or not
*/
struct mtk_vcodec_dev {
struct v4l2_device v4l2_dev;
@@ -460,6 +478,11 @@ struct mtk_vcodec_dev {
struct mtk_vcodec_pm pm;
unsigned int dec_capability;
unsigned int enc_capability;
+
+ void *subdev_dev[MTK_VDEC_HW_MAX];
+ struct device_node *subdev_node[MTK_VDEC_HW_MAX];
+
+ DECLARE_BITMAP(hardware_bitmap, MTK_VDEC_HW_MAX);
};
static inline struct mtk_vcodec_ctx *fh_to_ctx(struct v4l2_fh *fh)
--
2.25.1
On Tue, Nov 09, 2021 at 08:50:17PM +0800, Yunfei Dong wrote:
> Manage each hardware information which includes irq/power/clk.
> The hardware includes LAT0, LAT1 and CORE.
The commit message doesn't explain the code. Could you provide some
explanations about how the async mechanism works? (e.g. A bitmap for
all sub-devices' readiness ...)
> Reported-by: kernel test robot <[email protected]>
Apparently wrong tag.
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> index b7a51e96d4ba..eb2af42aa102 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> @@ -18,19 +18,49 @@
>
> #include "mtk_vcodec_drv.h"
> #include "mtk_vcodec_dec.h"
> +#include "mtk_vcodec_dec_hw.h"
> #include "mtk_vcodec_dec_pm.h"
> #include "mtk_vcodec_intr.h"
> -#include "mtk_vcodec_util.h"
Why does mtk_vcodec_util.h need to remove?
> +static int mtk_vcodec_subdev_device_check(struct mtk_vcodec_ctx *ctx)
> + {
Remove the extra space.
> + struct mtk_vcodec_dev *vdec_dev = ctx->dev;
> + struct platform_device *pdev = vdec_dev->plat_dev;
> + struct device_node *subdev_node;
> + enum mtk_vdec_hw_id hw_idx;
> + const struct of_device_id *of_id;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(mtk_vdec_hw_match); i++) {
> + of_id = &mtk_vdec_hw_match[i];
> + subdev_node = of_find_compatible_node(NULL, NULL,
> + of_id->compatible);
> + if (!subdev_node)
> + continue;
> +
> + if (!of_device_is_available(subdev_node)) {
> + of_node_put(subdev_node);
> + dev_err(&pdev->dev, "Fail to get MMSYS node\n");
I am not sure if the error message makes sense about mentioning MMSYS here.
> + continue;
> + }
> +
> + hw_idx = (enum mtk_vdec_hw_id)(uintptr_t)of_id->data;
Does it really need to cast twice?
> + vdec_dev->subdev_node[hw_idx] = subdev_node;
> +
> + if (!test_bit(hw_idx, vdec_dev->hardware_bitmap)) {
> + dev_err(&pdev->dev, "Vdec hw_idx is not ready %d.",
> + hw_idx);
I would prefer "Vdec %d is not ready\n".
> + return -EINVAL;
-EAGAIN makes more sense.
> + }
> + }
> +
> + return 0;
> +}
Would it need to call of_node_put() in the error handling path?
> +static int mtk_vcodec_init_dec_params(struct mtk_vcodec_dev *dev)
> +{
I would rather not call them "params". They are more like "resources".
> + struct platform_device *pdev = dev->plat_dev;
> + int ret;
> +
> + ret = mtk_vcodec_get_reg_bases(dev);
> + if (ret)
> + return ret;
> +
> + if (!dev->vdec_pdata->is_subdev_supported) {
> + dev->dec_irq = platform_get_irq(pdev, 0);
> + if (dev->dec_irq < 0) {
> + dev_err(&pdev->dev, "failed to get irq number");
> + return dev->dec_irq;
> + }
> +
> + irq_set_status_flags(dev->dec_irq, IRQ_NOAUTOEN);
> + ret = devm_request_irq(&pdev->dev, dev->dec_irq,
> + mtk_vcodec_dec_irq_handler, 0, pdev->name, dev);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to install dev->dec_irq %d (%d)",
> + dev->dec_irq, ret);
> + return ret;
> + }
> +
> + ret = mtk_vcodec_init_dec_pm(pdev, &dev->pm);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to get mt vcodec clock source");
> + return ret;
> + }
> + }
I would prefer:
if (dev->vdec_pdata->is_subdev_supported)
return 0;
And decrease the indent level by 1 for the following blocks.
> @@ -329,6 +377,13 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
> goto err_event_workq;
> }
>
> + ret = of_platform_populate(pdev->dev.of_node, NULL, NULL,
> + &pdev->dev);
> + if (ret) {
> + mtk_v4l2_err("Master device of_platform_populate failed.");
s/Master/Main/
Doesn't it need to reference `is_subdev_supported` before populating?
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
> new file mode 100644
> index 000000000000..745be12548ef
[...]
> +const struct of_device_id mtk_vdec_hw_match[] = {
> + {
> + .compatible = "mediatek,mtk-vcodec-lat",
> + .data = (void *)MTK_VDEC_LAT0,
> + },
> + {
> + .compatible = "mediatek,mtk-vcodec-core",
> + .data = (void *)MTK_VDEC_CORE,
> + },
> + {},
> +};
> +EXPORT_SYMBOL_GPL(mtk_vdec_hw_match);
Introducing new compatible strings. Need a dt-bindings patch for them.
> +static int mtk_vdec_hw_probe(struct platform_device *pdev)
> +{
[...]
> + subdev_dev->reg_base[VDEC_HW_MISC] =
> + devm_platform_ioremap_resource_byname(pdev, "misc");
> + if (IS_ERR((__force void *)subdev_dev->reg_base[VDEC_HW_MISC])) {
> + ret = PTR_ERR((__force void *)subdev_dev->reg_base[VDEC_HW_MISC]);
> + goto err;
> + }
Resource "misc" needs a dt-bindings patch to explain how to use/specify it.
> + hw_idx = (enum mtk_vdec_hw_id)(uintptr_t)of_id->data;
Does it really need to cast twice?
> + if (hw_idx < MTK_VDEC_HW_MAX) {
> + main_dev->subdev_dev[hw_idx] = subdev_dev;
> + subdev_dev->hw_idx = hw_idx;
> + subdev_dev->main_dev = main_dev;
> + subdev_dev->reg_base[VDEC_HW_SYS] =
> + main_dev->reg_base[VDEC_HW_SYS];
> + set_bit(subdev_dev->hw_idx, main_dev->hardware_bitmap);
> + }
mtk_vcodec_subdev_device_check() doesn't check the value of
of_id->data. Does it make more sense to align the implementation? If
hw_idx is equal to or bigger than MTK_VDEC_HW_MAX, shall it print
warning messages for example.
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h
[...]
> +#define VDEC_HW_ACTIVE 0x10
> +#define VDEC_IRQ_CFG 0x11
> +#define VDEC_IRQ_CLR 0x10
> +#define VDEC_IRQ_CFG_REG 0xa4
If only mtk_vcodec_dec_hw.c uses them, could they move to the file?
> @@ -423,6 +436,11 @@ struct mtk_vcodec_enc_pdata {
> * @pm: power management control
> * @dec_capability: used to identify decode capability, ex: 4k
> * @enc_capability: used to identify encode capability
> + *
> + * @subdev_dev: subdev hardware device
> + * @subdev_node: subdev node
> + *
> + * @hardware_bitmap: used to record hardware is ready or not
> */
> struct mtk_vcodec_dev {
> struct v4l2_device v4l2_dev;
> @@ -460,6 +478,11 @@ struct mtk_vcodec_dev {
> struct mtk_vcodec_pm pm;
> unsigned int dec_capability;
> unsigned int enc_capability;
> +
> + void *subdev_dev[MTK_VDEC_HW_MAX];
> + struct device_node *subdev_node[MTK_VDEC_HW_MAX];
> +
> + DECLARE_BITMAP(hardware_bitmap, MTK_VDEC_HW_MAX);
I would prefer to use name `subdev_bitmap`.
Hi Tzung-Bi,
Thanks for your suggestion.
On Wed, 2021-11-10 at 18:30 +0800, Tzung-Bi Shih wrote:
> On Tue, Nov 09, 2021 at 08:50:17PM +0800, Yunfei Dong wrote:
> > Manage each hardware information which includes irq/power/clk.
> > The hardware includes LAT0, LAT1 and CORE.
>
> The commit message doesn't explain the code. Could you provide some
> explanations about how the async mechanism works? (e.g. A bitmap for
> all sub-devices' readiness ...)
>
add more detail description for commit message.
> > Reported-by: kernel test robot <[email protected]>
>
> Apparently wrong tag.
>
Remove
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > index b7a51e96d4ba..eb2af42aa102 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > @@ -18,19 +18,49 @@
> >
> > #include "mtk_vcodec_drv.h"
> > #include "mtk_vcodec_dec.h"
> > +#include "mtk_vcodec_dec_hw.h"
> > #include "mtk_vcodec_dec_pm.h"
> > #include "mtk_vcodec_intr.h"
> > -#include "mtk_vcodec_util.h"
>
> Why does mtk_vcodec_util.h need to remove?
>
Put #include "mtk_vcodec_util.h" in mtk_vcodec_dec_hw.h.
> > +static int mtk_vcodec_subdev_device_check(struct mtk_vcodec_ctx
> > *ctx)
> > + {
>
> Remove the extra space.
>
Fix
> > + struct mtk_vcodec_dev *vdec_dev = ctx->dev;
> > + struct platform_device *pdev = vdec_dev->plat_dev;
> > + struct device_node *subdev_node;
> > + enum mtk_vdec_hw_id hw_idx;
> > + const struct of_device_id *of_id;
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(mtk_vdec_hw_match); i++) {
> > + of_id = &mtk_vdec_hw_match[i];
> > + subdev_node = of_find_compatible_node(NULL, NULL,
> > + of_id->compatible);
> > + if (!subdev_node)
> > + continue;
> > +
> > + if (!of_device_is_available(subdev_node)) {
> > + of_node_put(subdev_node);
> > + dev_err(&pdev->dev, "Fail to get MMSYS
> > node\n");
>
> I am not sure if the error message makes sense about mentioning MMSYS
> here.
>
Fix with subdev node.
> > + continue;
> > + }
> > +
> > + hw_idx = (enum mtk_vdec_hw_id)(uintptr_t)of_id->data;
>
> Does it really need to cast twice?
>
yes, or will build warning.
> > + vdec_dev->subdev_node[hw_idx] = subdev_node;
> > +
> > + if (!test_bit(hw_idx, vdec_dev->hardware_bitmap)) {
> > + dev_err(&pdev->dev, "Vdec hw_idx is not ready
> > %d.",
> > + hw_idx);
>
> I would prefer "Vdec %d is not ready\n".
>
Fix
> > + return -EINVAL;
>
> -EAGAIN makes more sense.
> Fix
> > + }
> > + }
> > +
> > + return 0;
> > +}
>
> Would it need to call of_node_put() in the error handling path?
>
yes, fix it.
> > +static int mtk_vcodec_init_dec_params(struct mtk_vcodec_dev *dev)
> > +{
>
> I would rather not call them "params". They are more like
> "resources".
>
Fix it with resources.
> > + struct platform_device *pdev = dev->plat_dev;
> > + int ret;
> > +
> > + ret = mtk_vcodec_get_reg_bases(dev);
> > + if (ret)
> > + return ret;
> > +
> > + if (!dev->vdec_pdata->is_subdev_supported) {
> > + dev->dec_irq = platform_get_irq(pdev, 0);
> > + if (dev->dec_irq < 0) {
> > + dev_err(&pdev->dev, "failed to get irq
> > number");
> > + return dev->dec_irq;
> > + }
> > +
> > + irq_set_status_flags(dev->dec_irq, IRQ_NOAUTOEN);
> > + ret = devm_request_irq(&pdev->dev, dev->dec_irq,
> > + mtk_vcodec_dec_irq_handler, 0, pdev->name,
> > dev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to install dev-
> > >dec_irq %d (%d)",
> > + dev->dec_irq, ret);
> > + return ret;
> > + }
> > +
> > + ret = mtk_vcodec_init_dec_pm(pdev, &dev->pm);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "failed to get mt vcodec
> > clock source");
> > + return ret;
> > + }
> > + }
>
> I would prefer:
>
> if (dev->vdec_pdata->is_subdev_supported)
> return 0;
>
Fix
> And decrease the indent level by 1 for the following blocks.
>
> > @@ -329,6 +377,13 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > goto err_event_workq;
> > }
> >
> > + ret = of_platform_populate(pdev->dev.of_node, NULL, NULL,
> > + &pdev->dev);
> > + if (ret) {
> > + mtk_v4l2_err("Master device of_platform_populate
> > failed.");
>
> s/Master/Main/
>
> Doesn't it need to reference `is_subdev_supported` before populating?
>
Fix
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
> > new file mode 100644
> > index 000000000000..745be12548ef
>
> [...]
> > +const struct of_device_id mtk_vdec_hw_match[] = {
> > + {
> > + .compatible = "mediatek,mtk-vcodec-lat",
> > + .data = (void *)MTK_VDEC_LAT0,
> > + },
> > + {
> > + .compatible = "mediatek,mtk-vcodec-core",
> > + .data = (void *)MTK_VDEC_CORE,
> > + },
> > + {},
> > +};
> > +EXPORT_SYMBOL_GPL(mtk_vdec_hw_match);
>
> Introducing new compatible strings. Need a dt-bindings patch for
> them.
>
Add in patch 15.
> > +static int mtk_vdec_hw_probe(struct platform_device *pdev)
> > +{
>
> [...]
> > + subdev_dev->reg_base[VDEC_HW_MISC] =
> > + devm_platform_ioremap_resource_byname(pdev, "misc");
> > + if (IS_ERR((__force void *)subdev_dev-
> > >reg_base[VDEC_HW_MISC])) {
> > + ret = PTR_ERR((__force void *)subdev_dev-
> > >reg_base[VDEC_HW_MISC]);
> > + goto err;
> > + }
>
> Resource "misc" needs a dt-bindings patch to explain how to
> use/specify it.
>
Remove.
> > + hw_idx = (enum mtk_vdec_hw_id)(uintptr_t)of_id->data;
>
> Does it really need to cast twice?
>
yes, or will build warning.
> > + if (hw_idx < MTK_VDEC_HW_MAX) {
> > + main_dev->subdev_dev[hw_idx] = subdev_dev;
> > + subdev_dev->hw_idx = hw_idx;
> > + subdev_dev->main_dev = main_dev;
> > + subdev_dev->reg_base[VDEC_HW_SYS] =
> > + main_dev->reg_base[VDEC_HW_SYS];
> > + set_bit(subdev_dev->hw_idx, main_dev-
> > >hardware_bitmap);
> > + }
>
> mtk_vcodec_subdev_device_check() doesn't check the value of
> of_id->data. Does it make more sense to align the
> implementation? If
> hw_idx is equal to or bigger than MTK_VDEC_HW_MAX, shall it print
> warning messages for example.
>
bitmap is used to record whether the hardware index is init or not.
Set the hardware index to 1 in bitmap when init done.
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h
>
> [...]
> > +#define VDEC_HW_ACTIVE 0x10
> > +#define VDEC_IRQ_CFG 0x11
> > +#define VDEC_IRQ_CLR 0x10
> > +#define VDEC_IRQ_CFG_REG 0xa4
>
> If only mtk_vcodec_dec_hw.c uses them, could they move to the file?
>
Also used in mtk_vcodec_dec_drv.c
> > @@ -423,6 +436,11 @@ struct mtk_vcodec_enc_pdata {
> > * @pm: power management control
> > * @dec_capability: used to identify decode capability, ex: 4k
> > * @enc_capability: used to identify encode capability
> > + *
> > + * @subdev_dev: subdev hardware device
> > + * @subdev_node: subdev node
> > + *
> > + * @hardware_bitmap: used to record hardware is ready or not
> > */
> > struct mtk_vcodec_dev {
> > struct v4l2_device v4l2_dev;
> > @@ -460,6 +478,11 @@ struct mtk_vcodec_dev {
> > struct mtk_vcodec_pm pm;
> > unsigned int dec_capability;
> > unsigned int enc_capability;
> > +
> > + void *subdev_dev[MTK_VDEC_HW_MAX];
> > + struct device_node *subdev_node[MTK_VDEC_HW_MAX];
> > +
> > + DECLARE_BITMAP(hardware_bitmap, MTK_VDEC_HW_MAX);
>
> I would prefer to use name `subdev_bitmap`.
Fix
Thanks,
Yunfei Dong
On Thu, Nov 11, 2021 at 11:49 AM [email protected]
<[email protected]> wrote:
>
> Hi Tzung-Bi,
>
> Thanks for your suggestion.
> On Wed, 2021-11-10 at 18:30 +0800, Tzung-Bi Shih wrote:
> > On Tue, Nov 09, 2021 at 08:50:17PM +0800, Yunfei Dong wrote:
> > > Manage each hardware information which includes irq/power/clk.
> > > The hardware includes LAT0, LAT1 and CORE.
> >
> > The commit message doesn't explain the code. Could you provide some
> > explanations about how the async mechanism works? (e.g. A bitmap for
> > all sub-devices' readiness ...)
> >
> add more detail description for commit message.
> > > Reported-by: kernel test robot <[email protected]>
> >
> > Apparently wrong tag.
> >
> Remove
> > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > > index b7a51e96d4ba..eb2af42aa102 100644
> > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > > @@ -18,19 +18,49 @@
> > >
> > > #include "mtk_vcodec_drv.h"
> > > #include "mtk_vcodec_dec.h"
> > > +#include "mtk_vcodec_dec_hw.h"
> > > #include "mtk_vcodec_dec_pm.h"
> > > #include "mtk_vcodec_intr.h"
> > > -#include "mtk_vcodec_util.h"
> >
> > Why does mtk_vcodec_util.h need to remove?
> >
> Put #include "mtk_vcodec_util.h" in mtk_vcodec_dec_hw.h.
If this file directly uses anything from that header file, it should
include it directly, instead of depending on some transitive inclusion.
This would avoid having to deal with breakage if/when the includes from
header files change beneath you.
ChenYu