Re: [PATCH v9, 06/19] media: mtk-vcodec: Manage multi hardware information

["yunfei.dong@mediatek.com" <yunfei.dong@mediatek.com>]

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@intel.com>
> 
> 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