Re: [PATCH v9, 06/19] media: mtk-vcodec: Manage multi hardware information
From: yunfei.dong@xxxxxxxxxxxx
Date: Wed Nov 10 2021 - 22:49:40 EST
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 <lkp@xxxxxxxxx>
>
> 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