[PATCH v9, 06/19] media: mtk-vcodec: Manage multi hardware information
Tzung-Bi Shih
tzungbi at google.com
Wed Nov 10 10:30:26 UTC 2021
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 <lkp at intel.com>
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`.
More information about the dri-devel
mailing list