mbox series

[v6,0/9] Refactor MTK MDP driver into core/components

Message ID 20210802121215.703023-1-eizan@chromium.org
Headers show
Series Refactor MTK MDP driver into core/components | expand

Message

Eizan Miyamoto Aug. 2, 2021, 12:12 p.m. UTC
This is an update to
https://patchwork.kernel.org/project/linux-mediatek/list/?series=283075
To address some comments and fixes.

This series has been verified to work on 5.13.


Changes in v6:
- Don't propagate errors from clock_on/off as an afterthought.
- Split apart modifying mdp driver to be loadable from mmsys from
  actually loading it from mmsys into two changs to make review easier.
- Update devicetree bindings to reflect no longer needing the
  mediatek,vpu property in the mdp_rdma0 device node.
- Some stylistic cleanups.

Changes in v5:
- rebase and test on 5.13-next @ e2f74b13dbe6

Changes in v4:
- rebase and test on 5.13
- don't depend on https://patchwork.kernel.org/project/linux-mediatek/list/?series=464873

Changes in v3:
- get mdp master from aliases instead of strcmp against of_node->name

Changes in v2:
- rebased onto Linux 5.12
- 100 char line length allowance was utilized in a few places
- Removal of a redundant dev_err() print at the end of
  mtk_mdp_comp_init()
- Instead of printing errors and ignoring them, I've added a patch to
  correctly propagate them.
- Use of C style comments.
- Three additional patches were added to eliminate dependency on the
  mediatek,vpu property inside the mdp_rdma0 device node.

Eizan Miyamoto (9):
  mtk-mdp: propagate errors from clock_on
  mtk-mdp: add driver to probe mdp components
  mtk-mdp: use pm_runtime in MDP component driver
  media: mtk-mdp: don't pm_run_time_get/put for master comp in clock_on
  mtk-mdp: make mdp driver to be loadable by platform_device_register*()
  soc: mediatek: mmsys: instantiate mdp virtual device from mmsys
  media: mtk-mdp: use mdp-rdma0 alias to point to MDP master
  dts: mtk-mdp: remove mediatek,vpu property from primary MDP device
  dt-bindings: mediatek: remove vpu requirement from mtk-mdp

 .../bindings/media/mediatek-mdp.txt           |   3 -
 arch/arm64/boot/dts/mediatek/mt8173.dtsi      |   1 -
 drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 268 +++++++++++++++--
 drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  34 +--
 drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 282 ++++++++++++------
 drivers/media/platform/mtk-mdp/mtk_mdp_core.h |   3 +
 drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c  |   4 +-
 drivers/soc/mediatek/mtk-mmsys.c              |  20 +-
 8 files changed, 470 insertions(+), 145 deletions(-)

Comments

Enric Balletbo Serra Aug. 3, 2021, 10:26 a.m. UTC | #1
Hi Eizan,

Thank you for your patch.

Missatge de Eizan Miyamoto <eizan@chromium.org> del dia dl., 2 d’ag.
2021 a les 14:14:
>

> Without this change, the MDP components are not fully integrated into

> the runtime power management subsystem, and the MDP driver does not

> work.

>

> For each of the component device drivers to be able to call

> pm_runtime_get/put_sync() a pointer to the component's device struct

> had to be added to struct mtk_mdp_comp, set by mtk_mdp_comp_init().

>

> Note that the dev argument to mtk_mdp_comp_clock_on/off() has been

> removed. Those functions used to be called from the "master" mdp driver

> in mtk_mdp_core.c, but the component's device pointer no longer

> corresponds to the mdp master device pointer, which is not the right

> device to pass to pm_runtime_put/get_sync() which we had to add to get

> the driver to work properly.

>

> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>


Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>



> ---

>

> (no changes since v1)

>

>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 24 +++++++++++++++----

>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  6 +++--

>  drivers/media/platform/mtk-mdp/mtk_mdp_core.c |  7 +++---

>  3 files changed, 27 insertions(+), 10 deletions(-)

>

> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c

> index 7a0e3acffab9..472c261b01e8 100644

> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c

> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c

> @@ -12,6 +12,7 @@

>  #include <linux/of_address.h>

>  #include <linux/of_platform.h>

>  #include <soc/mediatek/smi.h>

> +#include <linux/pm_runtime.h>

>

>  #include "mtk_mdp_comp.h"

>  #include "mtk_mdp_core.h"

> @@ -50,14 +51,22 @@ static const struct of_device_id mtk_mdp_comp_driver_dt_match[] = {

>  };

>  MODULE_DEVICE_TABLE(of, mtk_mdp_comp_driver_dt_match);

>

> -int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)

> +int mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp)

>  {

>         int i, err, status;

>

>         if (comp->larb_dev) {

>                 err = mtk_smi_larb_get(comp->larb_dev);

>                 if (err)

> -                       dev_err(dev, "failed to get larb, err %d.\n", err);

> +                       dev_err(comp->dev, "failed to get larb, err %d.\n", err);

> +       }

> +

> +       err = pm_runtime_get_sync(comp->dev);

> +       if (err < 0) {

> +               dev_err(comp->dev,

> +                       "failed to runtime get, err %d.\n",

> +                       err);

> +               return err;

>         }

>

>         for (i = 0; i < ARRAY_SIZE(comp->clk); i++) {

> @@ -66,7 +75,7 @@ int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)

>                 err = clk_prepare_enable(comp->clk[i]);

>                 if (err) {

>                         status = err;

> -                       dev_err(dev, "failed to enable clock, err %d. i:%d\n", err, i);

> +                       dev_err(comp->dev, "failed to enable clock, err %d. i:%d\n", err, i);

>                         goto err_clk_prepare_enable;

>                 }

>         }

> @@ -80,10 +89,12 @@ int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)

>                 clk_disable_unprepare(comp->clk[i]);

>         }

>

> +       pm_runtime_put_sync(comp->dev);

> +

>         return status;

>  }

>

> -void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp)

> +int mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)

>  {

>         int i;

>

> @@ -95,6 +106,8 @@ void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp)

>

>         if (comp->larb_dev)

>                 mtk_smi_larb_put(comp->larb_dev);

> +

> +       return pm_runtime_put_sync(comp->dev);

>  }

>

>  static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *data)

> @@ -103,6 +116,7 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *da

>         struct mtk_mdp_dev *mdp = data;

>

>         mtk_mdp_register_component(mdp, comp);

> +       pm_runtime_enable(dev);

>

>         return 0;

>  }

> @@ -113,6 +127,7 @@ static void mtk_mdp_comp_unbind(struct device *dev, struct device *master,

>         struct mtk_mdp_comp *comp = dev_get_drvdata(dev);

>         struct mtk_mdp_dev *mdp = data;

>

> +       pm_runtime_disable(dev);

>         mtk_mdp_unregister_component(mdp, comp);

>  }

>

> @@ -132,6 +147,7 @@ int mtk_mdp_comp_init(struct mtk_mdp_comp *comp, struct device *dev)

>                  (enum mtk_mdp_comp_type)of_device_get_match_data(dev);

>

>         INIT_LIST_HEAD(&comp->node);

> +       comp->dev = dev;

>

>         for (i = 0; i < ARRAY_SIZE(comp->clk); i++) {

>                 comp->clk[i] = of_clk_get(node, i);

> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h

> index df5fc4c94f90..f2e22e7e7c45 100644

> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h

> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h

> @@ -12,17 +12,19 @@

>   * @node:      list node to track sibing MDP components

>   * @clk:       clocks required for component

>   * @larb_dev:  SMI device required for component

> + * @dev:       component's device

>   */

>  struct mtk_mdp_comp {

>         struct list_head        node;

>         struct clk              *clk[2];

> +       struct device           *dev;

>         struct device           *larb_dev;

>  };

>

>  int mtk_mdp_comp_init(struct mtk_mdp_comp *comp, struct device *dev);

>

> -int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp);

> -void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp);

> +int mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp);

> +int mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp);

>

>  extern struct platform_driver mtk_mdp_component_driver;

>

> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c

> index b813a822439a..714154450981 100644

> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c

> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c

> @@ -58,7 +58,7 @@ static int mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)

>         int err;

>

>         list_for_each_entry(comp_node, &mdp->comp_list, node) {

> -               err = mtk_mdp_comp_clock_on(dev, comp_node);

> +               err = mtk_mdp_comp_clock_on(comp_node);

>                 if (err) {

>                         status = err;

>                         goto err_mtk_mdp_comp_clock_on;

> @@ -69,18 +69,17 @@ static int mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)

>

>  err_mtk_mdp_comp_clock_on:

>         list_for_each_entry_continue_reverse(comp_node, &mdp->comp_list, node)

> -               mtk_mdp_comp_clock_off(dev, comp_node);

> +               mtk_mdp_comp_clock_off(comp_node);

>

>         return status;

>  }

>

>  static void mtk_mdp_clock_off(struct mtk_mdp_dev *mdp)

>  {

> -       struct device *dev = &mdp->pdev->dev;

>         struct mtk_mdp_comp *comp_node;

>

>         list_for_each_entry(comp_node, &mdp->comp_list, node)

> -               mtk_mdp_comp_clock_off(dev, comp_node);

> +               mtk_mdp_comp_clock_off(comp_node);

>  }

>

>  static void mtk_mdp_wdt_worker(struct work_struct *work)

> --

> 2.32.0.554.ge1b32706d8-goog

>

>

> _______________________________________________

> Linux-mediatek mailing list

> Linux-mediatek@lists.infradead.org

> http://lists.infradead.org/mailman/listinfo/linux-mediatek
Enric Balletbo Serra Aug. 3, 2021, 10:26 a.m. UTC | #2
Hi Eizan,

Thank you for your patch.

Missatge de Eizan Miyamoto <eizan@chromium.org> del dia dl., 2 d’ag.
2021 a les 14:13:
>
> Up until this change, many errors were logged but ignored when powering
> on clocks inside mtk_mdp_core. This change tries to do a better job at
> propagating errors back to the power management framework.
>
> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>

Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

> ---
>
> (no changes since v1)
>
>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 25 ++++++++++++-----
>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  2 +-
>  drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 27 ++++++++++++++-----
>  3 files changed, 39 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> index b3426a551bea..76e295c8d9bc 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> @@ -13,10 +13,9 @@
>
>  #include "mtk_mdp_comp.h"
>
> -
> -void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
> +int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
>  {
> -       int i, err;
> +       int i, err, status;
>
>         if (comp->larb_dev) {
>                 err = mtk_smi_larb_get(comp->larb_dev);
> @@ -30,11 +29,23 @@ void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
>                 if (IS_ERR(comp->clk[i]))
>                         continue;
>                 err = clk_prepare_enable(comp->clk[i]);
> -               if (err)
> -                       dev_err(dev,
> -                       "failed to enable clock, err %d. type:%d i:%d\n",
> -                               err, comp->type, i);
> +               if (err) {
> +                       status = err;
> +                       dev_err(dev, "failed to enable clock, err %d. i:%d\n", err, i);
> +                       goto err_clk_prepare_enable;
> +               }
> +       }
> +
> +       return 0;
> +
> +err_clk_prepare_enable:
> +       for (--i; i >= 0; i--) {
> +               if (IS_ERR(comp->clk[i]))
> +                       continue;
> +               clk_disable_unprepare(comp->clk[i]);

nit: We could use bulk functions here, but that's probably a follow-up patch.




>         }
> +
> +       return status;
>  }
>
>  void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp)
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
> index 7897766c96bb..92ab5249bcad 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
> @@ -41,7 +41,7 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,
>                       struct mtk_mdp_comp *comp,
>                       enum mtk_mdp_comp_type comp_type);
>  void mtk_mdp_comp_deinit(struct device *dev, struct mtk_mdp_comp *comp);
> -void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp);
> +int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp);
>  void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp);
>
>
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> index 976aa1f4829b..412bbec0f735 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> @@ -52,13 +52,28 @@ static const struct of_device_id mtk_mdp_of_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, mtk_mdp_of_ids);
>
> -static void mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
> +static int mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
>  {
> -       struct device *dev = &mdp->pdev->dev;
>         struct mtk_mdp_comp *comp_node;
> +       int status;
> +       struct device *dev = &mdp->pdev->dev;
> +       int err;
>
> -       list_for_each_entry(comp_node, &mdp->comp_list, node)
> -               mtk_mdp_comp_clock_on(dev, comp_node);
> +       list_for_each_entry(comp_node, &mdp->comp_list, node) {
> +               err = mtk_mdp_comp_clock_on(dev, comp_node);
> +               if (err) {
> +                       status = err;
> +                       goto err_mtk_mdp_comp_clock_on;
> +               }
> +       }
> +
> +       return 0;
> +
> +err_mtk_mdp_comp_clock_on:
> +       list_for_each_entry_continue_reverse(comp_node, &mdp->comp_list, node)
> +               mtk_mdp_comp_clock_off(dev, comp_node);
> +
> +       return status;
>  }
>
>  static void mtk_mdp_clock_off(struct mtk_mdp_dev *mdp)
> @@ -274,9 +289,7 @@ static int __maybe_unused mtk_mdp_pm_resume(struct device *dev)
>  {
>         struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);
>
> -       mtk_mdp_clock_on(mdp);
> -
> -       return 0;
> +       return mtk_mdp_clock_on(mdp);
>  }
>
>  static int __maybe_unused mtk_mdp_suspend(struct device *dev)
> --
> 2.32.0.554.ge1b32706d8-goog
>
Enric Balletbo Serra Aug. 3, 2021, 10:27 a.m. UTC | #3
Hi Eizan,

Thank you for your patch.

Missatge de Eizan Miyamoto <eizan@chromium.org> del dia dl., 2 d’ag.
2021 a les 14:15:
>
> It is no longer needed by the mtk-mdp driver
>
> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>

Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>


> ---
>
> (no changes since v1)
>
>  Documentation/devicetree/bindings/media/mediatek-mdp.txt | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/mediatek-mdp.txt b/Documentation/devicetree/bindings/media/mediatek-mdp.txt
> index caa24943da33..4c325585f68f 100644
> --- a/Documentation/devicetree/bindings/media/mediatek-mdp.txt
> +++ b/Documentation/devicetree/bindings/media/mediatek-mdp.txt
> @@ -4,8 +4,6 @@ Media Data Path is used for scaling and color space conversion.
>
>  Required properties (controller node):
>  - compatible: "mediatek,mt8173-mdp"
> -- mediatek,vpu: the node of video processor unit, see
> -  Documentation/devicetree/bindings/media/mediatek-vpu.txt for details.
>
>  Required properties (all function blocks, child node):
>  - compatible: Should be one of
> @@ -41,7 +39,6 @@ Example:
>                 power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>;
>                 iommus = <&iommu M4U_PORT_MDP_RDMA0>;
>                 mediatek,larb = <&larb0>;
> -               mediatek,vpu = <&vpu>;
>         };
>
>         mdp_rdma1: rdma@14002000 {
> --
> 2.32.0.554.ge1b32706d8-goog
>
Enric Balletbo Serra Aug. 3, 2021, 10:27 a.m. UTC | #4
Hi Eizan,

Thank you for your patch.

Missatge de Eizan Miyamoto <eizan@chromium.org> del dia dl., 2 d’ag.
2021 a les 14:13:
>

> Rather than hanging the MDP master component driver off of the rdma0

> device, make it possible too create a "virtual" device by registering

> it with platform_device_register_*() to be probed by the mtk_mdp_core

> driver.

>

> Broadly, three interdependent things are done by this change:

> - Make it is possible to search for MDP devices in the device tree

>   through the grandparent device's of_node.

> - v4l-related setup is moved into from the mtk_mdp_core driver to the

>   mtk_mdp_comp driver.

> - Presence of a mediatek,vpu property in an MDP component device node

>   is used to determine what device to use when dispatching DMA ops from

>   the relevant ioctl, and also do V4L2 initialization in this case.

>

> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>

> ---

>

> Changes in v6:

> - Don't propagate errors from clock_on/off as an afterthought.

> - Split apart modifying mdp driver to be loadable from mmsys from

>   actually loading it from mmsys into two changs to make review easier.

> - Update devicetree bindings to reflect no longer needing the

>   mediatek,vpu property in the mdp_rdma0 device node.

> - Some stylistic cleanups.

>

> Changes in v5:

> - rebase and test on 5.13-next @ e2f74b13dbe6

>

> Changes in v4:

> - rebase and test on 5.13

> - don't depend on https://patchwork.kernel.org/project/linux-mediatek/list/?series=464873

>

> Changes in v3:

> - get mdp master from aliases instead of strcmp against of_node->name

>

> Changes in v2:

> - rebased onto Linux 5.12

> - 100 char line length allowance was utilized in a few places

> - Removal of a redundant dev_err() print at the end of

>   mtk_mdp_comp_init()

> - Instead of printing errors and ignoring them, I've added a patch to

>   correctly propagate them.

> - Use of C style comments.

> - Three additional patches were added to eliminate dependency on the

>   mediatek,vpu property inside the mdp_rdma0 device node.

>

>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 51 ++++++++++-----

>  drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 62 ++++++-------------

>  drivers/media/platform/mtk-mdp/mtk_mdp_core.h |  2 +

>  drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c  |  4 +-

>  4 files changed, 60 insertions(+), 59 deletions(-)

>

> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c

> index 7b6c8a3f3455..85ef274841a3 100644

> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c

> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c

> @@ -155,8 +155,45 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *da

>  {

>         struct mtk_mdp_comp *comp = dev_get_drvdata(dev);

>         struct mtk_mdp_dev *mdp = data;

> +       struct device_node *vpu_node;

>

>         mtk_mdp_register_component(mdp, comp);

> +

> +       /*

> +        * If this component has a "mediatek-vpu" property, it is responsible for

> +        * notifying the mdp master driver about it so it can be further initialized

> +        * later.

> +        */

> +       vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);

> +       if (vpu_node) {

> +               int ret;

> +

> +               mdp->vpu_dev = of_find_device_by_node(vpu_node);

> +               if (WARN_ON(!mdp->vpu_dev)) {


This looks a bit excessive IMO, but on the other hand looks like this
is a transitional patch as all this will be removed after some rework
on the latest patch.

> +                       dev_err(dev, "vpu pdev failed\n");


You already did a WARN_ON, this print is not needed. But again, all
this seems to be transitional and is removed later. So it doesn't
really bothers me

Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>



> +                       of_node_put(vpu_node);

> +               }

> +

> +               ret = v4l2_device_register(dev, &mdp->v4l2_dev);

> +               if (ret) {

> +                       dev_err(dev, "Failed to register v4l2 device\n");

> +                       return -EINVAL;

> +               }

> +

> +               ret = vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));

> +               if (ret) {

> +                       dev_err(dev, "Failed to set vb2 dma mag seg size\n");

> +                       return -EINVAL;

> +               }

> +

> +               /*

> +                * presence of the "mediatek,vpu" property in a device node

> +                * indicates that it is the primary MDP rdma device and MDP DMA

> +                * ops should be handled by its DMA callbacks.

> +                */

> +               mdp->rdma_dev = dev;

> +       }

> +

>         pm_runtime_enable(dev);

>

>         return 0;

> @@ -237,23 +274,9 @@ int mtk_mdp_comp_init(struct mtk_mdp_comp *comp, struct device *dev)

>  static int mtk_mdp_comp_probe(struct platform_device *pdev)

>  {

>         struct device *dev = &pdev->dev;

> -       struct device_node *vpu_node;

>         int status;

>         struct mtk_mdp_comp *comp;

>

> -       vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);

> -       if (vpu_node) {

> -               of_node_put(vpu_node);

> -               /*

> -                * The device tree node with a mediatek,vpu property is deemed

> -                * the MDP "master" device, we don't want to add a component

> -                * for it in this function because the initialization for the

> -                * master is done elsewhere.

> -                */

> -               dev_info(dev, "vpu node found, not probing\n");

> -               return -ENODEV;

> -       }

> -

>         comp = devm_kzalloc(dev, sizeof(*comp), GFP_KERNEL);

>         if (!comp)

>                 return -ENOMEM;

> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c

> index a72a9ba41ea6..50eafcc9993d 100644

> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c

> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c

> @@ -159,6 +159,17 @@ static int mtk_mdp_master_bind(struct device *dev)

>                 goto err_component_bind_all;

>         }

>

> +       if (mdp->vpu_dev) {

> +               int ret = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp,

> +                                         VPU_RST_MDP);

> +               if (ret) {

> +                       dev_err(dev, "Failed to register reset handler\n");

> +                       goto err_wdt_reg;

> +               }

> +       } else {

> +               dev_err(dev, "no vpu_dev found\n");

> +       }

> +

>         status = mtk_mdp_register_m2m_device(mdp);

>         if (status) {

>                 dev_err(dev, "Failed to register m2m device: %d\n", status);

> @@ -170,6 +181,8 @@ static int mtk_mdp_master_bind(struct device *dev)

>         return 0;

>

>  err_mtk_mdp_register_m2m_device:

> +

> +err_wdt_reg:

>         component_unbind_all(dev, mdp);

>

>  err_component_bind_all:

> @@ -228,8 +241,13 @@ static int mtk_mdp_probe(struct platform_device *pdev)

>                 of_node_put(node);

>                 parent = dev->of_node;

>                 dev_warn(dev, "device tree is out of date\n");

> -       } else {

> +       } else if (dev->of_node) {

>                 parent = dev->of_node->parent;

> +       } else if (dev->parent) {

> +               /* maybe we were created from a call to platform_device_register_data() */

> +               parent = dev->parent->parent->of_node;

> +       } else {

> +               return -ENODEV;

>         }

>

>         /* Iterate over sibling MDP function blocks */

> @@ -262,16 +280,6 @@ static int mtk_mdp_probe(struct platform_device *pdev)

>                 }

>         }

>

> -       /*

> -        * Create a component for myself so that clocks can be toggled in

> -        * clock_on().

> -        */

> -       ret = mtk_mdp_comp_init(&mdp->comp_self, dev);

> -       if (ret) {

> -               dev_err(dev, "Failed to initialize component\n");

> -               goto err_comp;

> -       }

> -

>         mdp->job_wq = create_singlethread_workqueue(MTK_MDP_MODULE_NAME);

>         if (!mdp->job_wq) {

>                 dev_err(&pdev->dev, "unable to alloc job workqueue\n");

> @@ -287,29 +295,8 @@ static int mtk_mdp_probe(struct platform_device *pdev)

>         }

>         INIT_WORK(&mdp->wdt_work, mtk_mdp_wdt_worker);

>

> -       ret = v4l2_device_register(dev, &mdp->v4l2_dev);

> -       if (ret) {

> -               dev_err(&pdev->dev, "Failed to register v4l2 device\n");

> -               ret = -EINVAL;

> -               goto err_dev_register;

> -       }

> -

> -       mdp->vpu_dev = vpu_get_plat_device(pdev);

> -       ret = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp,

> -                                 VPU_RST_MDP);

> -       if (ret) {

> -               dev_err(&pdev->dev, "Failed to register reset handler\n");

> -               goto err_wdt_reg;

> -       }

> -

>         platform_set_drvdata(pdev, mdp);

>

> -       ret = vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));

> -       if (ret) {

> -               dev_err(&pdev->dev, "Failed to set vb2 dma mag seg size\n");

> -               goto err_set_max_seg_size;

> -       }

> -

>         ret = component_master_add_with_match(dev, &mtk_mdp_com_ops, match);

>         if (ret) {

>                 dev_err(dev, "Component master add failed\n");

> @@ -321,22 +308,12 @@ static int mtk_mdp_probe(struct platform_device *pdev)

>         return 0;

>

>  err_component_master_add:

> -       vb2_dma_contig_clear_max_seg_size(&pdev->dev);

> -

> -err_set_max_seg_size:

> -

> -err_wdt_reg:

> -       v4l2_device_unregister(&mdp->v4l2_dev);

> -

> -err_dev_register:

>         destroy_workqueue(mdp->wdt_wq);

>

>  err_alloc_wdt_wq:

>         destroy_workqueue(mdp->job_wq);

>

>  err_alloc_job_wq:

> -

> -err_comp:

>         dev_dbg(dev, "err %d\n", ret);

>         return ret;

>  }

> @@ -404,7 +381,6 @@ static struct platform_driver mtk_mdp_driver = {

>         .driver = {

>                 .name   = MTK_MDP_MODULE_NAME,

>                 .pm     = &mtk_mdp_pm_ops,

> -               .of_match_table = mtk_mdp_of_ids,

>         }

>  };

>

> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.h b/drivers/media/platform/mtk-mdp/mtk_mdp_core.h

> index 8a52539b15d4..9fcd8b8e7c25 100644

> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.h

> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.h

> @@ -133,6 +133,7 @@ struct mtk_mdp_variant {

>   * struct mtk_mdp_dev - abstraction for image processor entity

>   * @lock:      the mutex protecting this data structure

>   * @vpulock:   the mutex protecting the communication with VPU

> + * @rdma_dev:  device pointer to rdma device for MDP

>   * @pdev:      pointer to the image processor platform device

>   * @variant:   the IP variant information

>   * @id:                image processor device index (0..MTK_MDP_MAX_DEVS)

> @@ -151,6 +152,7 @@ struct mtk_mdp_variant {

>  struct mtk_mdp_dev {

>         struct mutex                    lock;

>         struct mutex                    vpulock;

> +       struct device                   *rdma_dev;

>         struct platform_device          *pdev;

>         struct mtk_mdp_variant          *variant;

>         u16                             id;

> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c

> index f14779e7596e..9834d3bbe851 100644

> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c

> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c

> @@ -929,7 +929,7 @@ static int mtk_mdp_m2m_queue_init(void *priv, struct vb2_queue *src_vq,

>         src_vq->mem_ops = &vb2_dma_contig_memops;

>         src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);

>         src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;

> -       src_vq->dev = &ctx->mdp_dev->pdev->dev;

> +       src_vq->dev = ctx->mdp_dev->rdma_dev;

>         src_vq->lock = &ctx->mdp_dev->lock;

>

>         ret = vb2_queue_init(src_vq);

> @@ -944,7 +944,7 @@ static int mtk_mdp_m2m_queue_init(void *priv, struct vb2_queue *src_vq,

>         dst_vq->mem_ops = &vb2_dma_contig_memops;

>         dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);

>         dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;

> -       dst_vq->dev = &ctx->mdp_dev->pdev->dev;

> +       dst_vq->dev = ctx->mdp_dev->rdma_dev;

>         dst_vq->lock = &ctx->mdp_dev->lock;

>

>         return vb2_queue_init(dst_vq);

> --

> 2.32.0.554.ge1b32706d8-goog

>
Enric Balletbo Serra Aug. 3, 2021, 10:29 a.m. UTC | #5
Hi all,

Missatge de Eizan Miyamoto <eizan@chromium.org> del dia dl., 2 d’ag.
2021 a les 14:12:
>
>
> This is an update to
> https://patchwork.kernel.org/project/linux-mediatek/list/?series=283075
> To address some comments and fixes.
>
> This series has been verified to work on 5.13.
>

The series have also been validated on top of 5.14-rc4 and linux-next
on an Acer Chromebook R 13 without observing any problems and running
some video decoding tests, so, for the full series.

Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

>
> Changes in v6:
> - Don't propagate errors from clock_on/off as an afterthought.
> - Split apart modifying mdp driver to be loadable from mmsys from
>   actually loading it from mmsys into two changs to make review easier.
> - Update devicetree bindings to reflect no longer needing the
>   mediatek,vpu property in the mdp_rdma0 device node.
> - Some stylistic cleanups.
>
> Changes in v5:
> - rebase and test on 5.13-next @ e2f74b13dbe6
>
> Changes in v4:
> - rebase and test on 5.13
> - don't depend on https://patchwork.kernel.org/project/linux-mediatek/list/?series=464873
>
> Changes in v3:
> - get mdp master from aliases instead of strcmp against of_node->name
>
> Changes in v2:
> - rebased onto Linux 5.12
> - 100 char line length allowance was utilized in a few places
> - Removal of a redundant dev_err() print at the end of
>   mtk_mdp_comp_init()
> - Instead of printing errors and ignoring them, I've added a patch to
>   correctly propagate them.
> - Use of C style comments.
> - Three additional patches were added to eliminate dependency on the
>   mediatek,vpu property inside the mdp_rdma0 device node.
>
> Eizan Miyamoto (9):
>   mtk-mdp: propagate errors from clock_on
>   mtk-mdp: add driver to probe mdp components
>   mtk-mdp: use pm_runtime in MDP component driver
>   media: mtk-mdp: don't pm_run_time_get/put for master comp in clock_on
>   mtk-mdp: make mdp driver to be loadable by platform_device_register*()
>   soc: mediatek: mmsys: instantiate mdp virtual device from mmsys
>   media: mtk-mdp: use mdp-rdma0 alias to point to MDP master
>   dts: mtk-mdp: remove mediatek,vpu property from primary MDP device
>   dt-bindings: mediatek: remove vpu requirement from mtk-mdp
>
>  .../bindings/media/mediatek-mdp.txt           |   3 -
>  arch/arm64/boot/dts/mediatek/mt8173.dtsi      |   1 -
>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 268 +++++++++++++++--
>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  34 +--
>  drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 282 ++++++++++++------
>  drivers/media/platform/mtk-mdp/mtk_mdp_core.h |   3 +
>  drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c  |   4 +-
>  drivers/soc/mediatek/mtk-mmsys.c              |  20 +-
>  8 files changed, 470 insertions(+), 145 deletions(-)
>
> --
> 2.32.0.554.ge1b32706d8-goog
>
Dafna Hirschfeld Aug. 5, 2021, 6:06 a.m. UTC | #6
On 02.08.21 14:12, Eizan Miyamoto wrote:
> Up until this change, many errors were logged but ignored when powering
> on clocks inside mtk_mdp_core. This change tries to do a better job at
> propagating errors back to the power management framework.
> 
> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
> ---
> 
> (no changes since v1)
> 
>   drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 25 ++++++++++++-----
>   drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  2 +-
>   drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 27 ++++++++++++++-----
>   3 files changed, 39 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> index b3426a551bea..76e295c8d9bc 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> @@ -13,10 +13,9 @@
>   
>   #include "mtk_mdp_comp.h"
>   
> -
> -void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
> +int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
>   {
> -	int i, err;
> +	int i, err, status;
>   
>   	if (comp->larb_dev) {
>   		err = mtk_smi_larb_get(comp->larb_dev);
> @@ -30,11 +29,23 @@ void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
>   		if (IS_ERR(comp->clk[i]))
>   			continue;
>   		err = clk_prepare_enable(comp->clk[i]);
> -		if (err)
> -			dev_err(dev,
> -			"failed to enable clock, err %d. type:%d i:%d\n",
> -				err, comp->type, i);
> +		if (err) {
> +			status = err;
> +			dev_err(dev, "failed to enable clock, err %d. i:%d\n", err, i);
> +			goto err_clk_prepare_enable;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_clk_prepare_enable:
> +	for (--i; i >= 0; i--) {
> +		if (IS_ERR(comp->clk[i]))
> +			continue;
> +		clk_disable_unprepare(comp->clk[i]);
>   	}
> +
> +	return status;

There is an API function clk_bulk_prepare_enable to prepare and enable an array of clks
so you can just use it.

>   }
>   
>   void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp)
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
> index 7897766c96bb..92ab5249bcad 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
> @@ -41,7 +41,7 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,
>   		      struct mtk_mdp_comp *comp,
>   		      enum mtk_mdp_comp_type comp_type);
>   void mtk_mdp_comp_deinit(struct device *dev, struct mtk_mdp_comp *comp);
> -void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp);
> +int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp);
>   void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp);
>   
>   
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> index 976aa1f4829b..412bbec0f735 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> @@ -52,13 +52,28 @@ static const struct of_device_id mtk_mdp_of_ids[] = {
>   };
>   MODULE_DEVICE_TABLE(of, mtk_mdp_of_ids);
>   
> -static void mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
> +static int mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
>   {
> -	struct device *dev = &mdp->pdev->dev;
>   	struct mtk_mdp_comp *comp_node;
> +	int status;
> +	struct device *dev = &mdp->pdev->dev;
> +	int err;
>   
> -	list_for_each_entry(comp_node, &mdp->comp_list, node)
> -		mtk_mdp_comp_clock_on(dev, comp_node);
> +	list_for_each_entry(comp_node, &mdp->comp_list, node) {
> +		err = mtk_mdp_comp_clock_on(dev, comp_node);
> +		if (err) {
> +			status = err;

You can get rid of the new var 'status' and just return ret in case of error

> +			goto err_mtk_mdp_comp_clock_on;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_mtk_mdp_comp_clock_on:
> +	list_for_each_entry_continue_reverse(comp_node, &mdp->comp_list, node)
> +		mtk_mdp_comp_clock_off(dev, comp_node);
> +
> +	return status;
>   }
>   
>   static void mtk_mdp_clock_off(struct mtk_mdp_dev *mdp)
> @@ -274,9 +289,7 @@ static int __maybe_unused mtk_mdp_pm_resume(struct device *dev)
>   {
>   	struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);
>   
> -	mtk_mdp_clock_on(mdp);
> -
> -	return 0;
> +	return mtk_mdp_clock_on(mdp);
>   }
>   
>   static int __maybe_unused mtk_mdp_suspend(struct device *dev)
>
Rob Herring (Arm) Aug. 6, 2021, 9:47 p.m. UTC | #7
On Mon, 02 Aug 2021 22:12:15 +1000, Eizan Miyamoto wrote:
> It is no longer needed by the mtk-mdp driver
> 
> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  Documentation/devicetree/bindings/media/mediatek-mdp.txt | 3 ---
>  1 file changed, 3 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>
Eizan Miyamoto Aug. 9, 2021, 3:23 a.m. UTC | #8
Hi Dafna, thank you very much for spending time to review the patch,
your time spent is very much appreciated.

On Thu, Aug 5, 2021 at 4:06 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
> > +err_clk_prepare_enable:
> > +     for (--i; i >= 0; i--) {
> > +             if (IS_ERR(comp->clk[i]))
> > +                     continue;
> > +             clk_disable_unprepare(comp->clk[i]);
> >       }
> > +
> > +     return status;
>
> There is an API function clk_bulk_prepare_enable to prepare and enable an array of clks
> so you can just use it.

As per Enric's suggestion earlier in this email thread, are you OK
with me making this change in a follow-up patch, particularly since
the logic as it is was preserved from previous functionality?

> > -static void mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
> > +static int mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
> >   {
> > -     struct device *dev = &mdp->pdev->dev;
> >       struct mtk_mdp_comp *comp_node;
> > +     int status;
> > +     struct device *dev = &mdp->pdev->dev;
> > +     int err;
> >
> > -     list_for_each_entry(comp_node, &mdp->comp_list, node)
> > -             mtk_mdp_comp_clock_on(dev, comp_node);
> > +     list_for_each_entry(comp_node, &mdp->comp_list, node) {
> > +             err = mtk_mdp_comp_clock_on(dev, comp_node);
> > +             if (err) {
> > +                     status = err;
>
> You can get rid of the new var 'status' and just return ret in case of error

This seems like a nit (please let me know if you disagree), and it's
also cleaned up in a follow-on patch in the series ("don't
pm_run_time_get/put for master comp in clock_on"). Is making the
change you are suggesting here something that should require uploading
a new series version?

Eizan
Dafna Hirschfeld Aug. 9, 2021, 7:42 a.m. UTC | #9
Hi,

On 09.08.21 05:23, Eizan Miyamoto wrote:
> Hi Dafna, thank you very much for spending time to review the patch,
> your time spent is very much appreciated.
> 
> On Thu, Aug 5, 2021 at 4:06 PM Dafna Hirschfeld
> <dafna.hirschfeld@collabora.com> wrote:
>>> +err_clk_prepare_enable:
>>> +     for (--i; i >= 0; i--) {
>>> +             if (IS_ERR(comp->clk[i]))
>>> +                     continue;
>>> +             clk_disable_unprepare(comp->clk[i]);
>>>        }
>>> +
>>> +     return status;
>>
>> There is an API function clk_bulk_prepare_enable to prepare and enable an array of clks
>> so you can just use it.
> 
> As per Enric's suggestion earlier in this email thread, are you OK
> with me making this change in a follow-up patch, particularly since
> the logic as it is was preserved from previous functionality?

sure,
I just give suggestions.
A follow-up patch would be nice.

> 
>>> -static void mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
>>> +static int mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
>>>    {
>>> -     struct device *dev = &mdp->pdev->dev;
>>>        struct mtk_mdp_comp *comp_node;
>>> +     int status;
>>> +     struct device *dev = &mdp->pdev->dev;
>>> +     int err;
>>>
>>> -     list_for_each_entry(comp_node, &mdp->comp_list, node)
>>> -             mtk_mdp_comp_clock_on(dev, comp_node);
>>> +     list_for_each_entry(comp_node, &mdp->comp_list, node) {
>>> +             err = mtk_mdp_comp_clock_on(dev, comp_node);
>>> +             if (err) {
>>> +                     status = err;
>>
>> You can get rid of the new var 'status' and just return ret in case of error
> 
> This seems like a nit (please let me know if you disagree), and it's
> also cleaned up in a follow-on patch in the series ("don't
> pm_run_time_get/put for master comp in clock_on"). Is making the
> change you are suggesting here something that should require uploading
> a new series version?

Hi, no, I am fine with it. No need for new version.


Thanks,
Dafna

> 
> Eizan
>
houlong wei Aug. 16, 2021, 3:37 a.m. UTC | #10
On Mon, 2021-08-02 at 20:12 +0800, Eizan Miyamoto wrote:
> Rather than hanging the MDP master component driver off of the rdma0

> device, make it possible too create a "virtual" device by registering

> it with platform_device_register_*() to be probed by the mtk_mdp_core

> driver.

> 

> Broadly, three interdependent things are done by this change:

> - Make it is possible to search for MDP devices in the device tree

>   through the grandparent device's of_node.

> - v4l-related setup is moved into from the mtk_mdp_core driver to the

>   mtk_mdp_comp driver.

> - Presence of a mediatek,vpu property in an MDP component device node

>   is used to determine what device to use when dispatching DMA ops

> from

>   the relevant ioctl, and also do V4L2 initialization in this case.

> 

> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>

> ---

> 

> Changes in v6:

> - Don't propagate errors from clock_on/off as an afterthought.

> - Split apart modifying mdp driver to be loadable from mmsys from

>   actually loading it from mmsys into two changs to make review

> easier.

> - Update devicetree bindings to reflect no longer needing the

>   mediatek,vpu property in the mdp_rdma0 device node.

> - Some stylistic cleanups.

> 

> Changes in v5:

> - rebase and test on 5.13-next @ e2f74b13dbe6

> 

> Changes in v4:

> - rebase and test on 5.13

> - don't depend on 

> https://patchwork.kernel.org/project/linux-mediatek/list/?series=464873

> 

> Changes in v3:

> - get mdp master from aliases instead of strcmp against of_node->name

> 

> Changes in v2:

> - rebased onto Linux 5.12

> - 100 char line length allowance was utilized in a few places

> - Removal of a redundant dev_err() print at the end of

>   mtk_mdp_comp_init()

> - Instead of printing errors and ignoring them, I've added a patch to

>   correctly propagate them.

> - Use of C style comments.

> - Three additional patches were added to eliminate dependency on the

>   mediatek,vpu property inside the mdp_rdma0 device node.

> 

>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 51 ++++++++++-----

>  drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 62 ++++++-----------

> --

>  drivers/media/platform/mtk-mdp/mtk_mdp_core.h |  2 +

>  drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c  |  4 +-

>  4 files changed, 60 insertions(+), 59 deletions(-)

> 

> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c

> b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c

> index 7b6c8a3f3455..85ef274841a3 100644

> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c

> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c

> @@ -155,8 +155,45 @@ static int mtk_mdp_comp_bind(struct device *dev,

> struct device *master, void *da

>  {

>  	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);

>  	struct mtk_mdp_dev *mdp = data;

> +	struct device_node *vpu_node;

>  

>  	mtk_mdp_register_component(mdp, comp);

> +

> +	/*

> +	 * If this component has a "mediatek-vpu" property, it is

> responsible for

> +	 * notifying the mdp master driver about it so it can be

> further initialized

> +	 * later.

> +	 */

> +	vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);

> +	if (vpu_node) {

> +		int ret;

> +

> +		mdp->vpu_dev = of_find_device_by_node(vpu_node);

> +		if (WARN_ON(!mdp->vpu_dev)) {

> +			dev_err(dev, "vpu pdev failed\n");

> +			of_node_put(vpu_node);

> +		}


Maybe 'of_node_put(vpu_node)' should be called when 'mdp->vpu_dev' is
valid.


> +		ret = v4l2_device_register(dev, &mdp->v4l2_dev);

> +		if (ret) {

> +			dev_err(dev, "Failed to register v4l2

> device\n");

> +			return -EINVAL;

> +		}

> +

> +		ret = vb2_dma_contig_set_max_seg_size(dev,

> DMA_BIT_MASK(32));

> +		if (ret) {

> +			dev_err(dev, "Failed to set vb2 dma mag seg

> size\n");

> +			return -EINVAL;

> +		}

> +

> +		/*

> +		 * presence of the "mediatek,vpu" property in a device

> node

> +		 * indicates that it is the primary MDP rdma device and

> MDP DMA

> +		 * ops should be handled by its DMA callbacks.

> +		 */

> +		mdp->rdma_dev = dev;

> +	}

> +

>  	pm_runtime_enable(dev);

>  

>  	return 0;

> @@ -237,23 +274,9 @@ int mtk_mdp_comp_init(struct mtk_mdp_comp *comp,

> struct device *dev)

>  static int mtk_mdp_comp_probe(struct platform_device *pdev)

>  {

>  	struct device *dev = &pdev->dev;

> -	struct device_node *vpu_node;

>  	int status;

>  	struct mtk_mdp_comp *comp;

>  

> -	vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);

> -	if (vpu_node) {

> -		of_node_put(vpu_node);

> -		/*

> -		 * The device tree node with a mediatek,vpu property is

> deemed

> -		 * the MDP "master" device, we don't want to add a

> component

> -		 * for it in this function because the initialization

> for the

> -		 * master is done elsewhere.

> -		 */

> -		dev_info(dev, "vpu node found, not probing\n");

> -		return -ENODEV;

> -	}

> -

>  	comp = devm_kzalloc(dev, sizeof(*comp), GFP_KERNEL);

>  	if (!comp)

>  		return -ENOMEM;

> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c

> b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c

> index a72a9ba41ea6..50eafcc9993d 100644

> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c

> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c

> @@ -159,6 +159,17 @@ static int mtk_mdp_master_bind(struct device

> *dev)

>  		goto err_component_bind_all;

>  	}

>  

> +	if (mdp->vpu_dev) {

> +		int ret = vpu_wdt_reg_handler(mdp->vpu_dev,

> mtk_mdp_reset_handler, mdp,

> +					  VPU_RST_MDP);

> +		if (ret) {

> +			dev_err(dev, "Failed to register reset

> handler\n");

> +			goto err_wdt_reg;

> +		}

> +	} else {

> +		dev_err(dev, "no vpu_dev found\n");

> +	}

> +

>  	status = mtk_mdp_register_m2m_device(mdp);

>  	if (status) {

>  		dev_err(dev, "Failed to register m2m device: %d\n",

> status);

> @@ -170,6 +181,8 @@ static int mtk_mdp_master_bind(struct device

> *dev)

>  	return 0;

>  

>  err_mtk_mdp_register_m2m_device:

> +

> +err_wdt_reg:

>  	component_unbind_all(dev, mdp);

>  

>  err_component_bind_all:

> @@ -228,8 +241,13 @@ static int mtk_mdp_probe(struct platform_device

> *pdev)

>  		of_node_put(node);

>  		parent = dev->of_node;

>  		dev_warn(dev, "device tree is out of date\n");

> -	} else {

> +	} else if (dev->of_node) {

>  		parent = dev->of_node->parent;

> +	} else if (dev->parent) {

> +		/* maybe we were created from a call to

> platform_device_register_data() */

> +		parent = dev->parent->parent->of_node;

> +	} else {

> +		return -ENODEV;

>  	}

>  

>  	/* Iterate over sibling MDP function blocks */

> @@ -262,16 +280,6 @@ static int mtk_mdp_probe(struct platform_device

> *pdev)

>  		}

>  	}

>  

> -	/*

> -	 * Create a component for myself so that clocks can be toggled

> in

> -	 * clock_on().

> -	 */

> -	ret = mtk_mdp_comp_init(&mdp->comp_self, dev);

> -	if (ret) {

> -		dev_err(dev, "Failed to initialize component\n");

> -		goto err_comp;

> -	}

> -

>  	mdp->job_wq =

> create_singlethread_workqueue(MTK_MDP_MODULE_NAME);

>  	if (!mdp->job_wq) {

>  		dev_err(&pdev->dev, "unable to alloc job workqueue\n");

> @@ -287,29 +295,8 @@ static int mtk_mdp_probe(struct platform_device

> *pdev)

>  	}

>  	INIT_WORK(&mdp->wdt_work, mtk_mdp_wdt_worker);

>  

> -	ret = v4l2_device_register(dev, &mdp->v4l2_dev);

> -	if (ret) {

> -		dev_err(&pdev->dev, "Failed to register v4l2

> device\n");

> -		ret = -EINVAL;

> -		goto err_dev_register;

> -	}

> -

> -	mdp->vpu_dev = vpu_get_plat_device(pdev);

> -	ret = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler,

> mdp,

> -				  VPU_RST_MDP);

> -	if (ret) {

> -		dev_err(&pdev->dev, "Failed to register reset

> handler\n");

> -		goto err_wdt_reg;

> -	}

> -

>  	platform_set_drvdata(pdev, mdp);

>  

> -	ret = vb2_dma_contig_set_max_seg_size(&pdev->dev,

> DMA_BIT_MASK(32));

> -	if (ret) {

> -		dev_err(&pdev->dev, "Failed to set vb2 dma mag seg

> size\n");

> -		goto err_set_max_seg_size;

> -	}

> -

>  	ret = component_master_add_with_match(dev, &mtk_mdp_com_ops,

> match);

>  	if (ret) {

>  		dev_err(dev, "Component master add failed\n");

> @@ -321,22 +308,12 @@ static int mtk_mdp_probe(struct platform_device

> *pdev)

>  	return 0;

>  

>  err_component_master_add:

> -	vb2_dma_contig_clear_max_seg_size(&pdev->dev);

> -

> -err_set_max_seg_size:

> -

> -err_wdt_reg:

> -	v4l2_device_unregister(&mdp->v4l2_dev);

> -

> -err_dev_register:

>  	destroy_workqueue(mdp->wdt_wq);

>  

>  err_alloc_wdt_wq:

>  	destroy_workqueue(mdp->job_wq);

>  

>  err_alloc_job_wq:

> -

> -err_comp:

>  	dev_dbg(dev, "err %d\n", ret);

>  	return ret;

>  }

> @@ -404,7 +381,6 @@ static struct platform_driver mtk_mdp_driver = {

>  	.driver = {

>  		.name	= MTK_MDP_MODULE_NAME,

>  		.pm	= &mtk_mdp_pm_ops,

> -		.of_match_table = mtk_mdp_of_ids,

>  	}

>  };

>  

> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.h

> b/drivers/media/platform/mtk-mdp/mtk_mdp_core.h

> index 8a52539b15d4..9fcd8b8e7c25 100644

> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.h

> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.h

> @@ -133,6 +133,7 @@ struct mtk_mdp_variant {

>   * struct mtk_mdp_dev - abstraction for image processor entity

>   * @lock:	the mutex protecting this data structure

>   * @vpulock:	the mutex protecting the communication with VPU

> + * @rdma_dev:  device pointer to rdma device for MDP

>   * @pdev:	pointer to the image processor platform device

>   * @variant:	the IP variant information

>   * @id:		image processor device index

> (0..MTK_MDP_MAX_DEVS)

> @@ -151,6 +152,7 @@ struct mtk_mdp_variant {

>  struct mtk_mdp_dev {

>  	struct mutex			lock;

>  	struct mutex			vpulock;

> +	struct device			*rdma_dev;

>  	struct platform_device		*pdev;

>  	struct mtk_mdp_variant		*variant;

>  	u16				id;

> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c

> b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c

> index f14779e7596e..9834d3bbe851 100644

> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c

> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c

> @@ -929,7 +929,7 @@ static int mtk_mdp_m2m_queue_init(void *priv,

> struct vb2_queue *src_vq,

>  	src_vq->mem_ops = &vb2_dma_contig_memops;

>  	src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);

>  	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;

> -	src_vq->dev = &ctx->mdp_dev->pdev->dev;

> +	src_vq->dev = ctx->mdp_dev->rdma_dev;

>  	src_vq->lock = &ctx->mdp_dev->lock;

>  

>  	ret = vb2_queue_init(src_vq);

> @@ -944,7 +944,7 @@ static int mtk_mdp_m2m_queue_init(void *priv,

> struct vb2_queue *src_vq,

>  	dst_vq->mem_ops = &vb2_dma_contig_memops;

>  	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);

>  	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;

> -	dst_vq->dev = &ctx->mdp_dev->pdev->dev;

> +	dst_vq->dev = ctx->mdp_dev->rdma_dev;

>  	dst_vq->lock = &ctx->mdp_dev->lock;

>  

>  	return vb2_queue_init(dst_vq);

> -- 

> 2.32.0.554.ge1b32706d8-goog

>