lkml.org 
[lkml]   [2021]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v6 7/9] media: mtk-mdp: use mdp-rdma0 alias to point to MDP master
    From
    Date
    On Mon, 2021-08-16 at 11:00 +0800, houlong wei wrote:
    > On Mon, 2021-08-02 at 20:12 +0800, Eizan Miyamoto wrote:
    > > ... Instead of depending on the presence of a mediatek,vpu property
    > > in
    > > the device node.
    > >
    > > That property was originally added to link to the vpu node so that
    > > the
    > > mtk_mdp_core driver could pass the right device to
    > > vpu_wdt_reg_handler(). However in a previous patch in this series,
    > > the driver has been modified to search the device tree for that
    > > node
    > > instead.
    > >
    > > That property was also used to indicate the primary MDP device, so
    > > that
    > > it can be passed to the V4L2 subsystem as well as register it to be
    > > used when setting up queues in the open() callback for the
    > > filesystem
    > > device node that is created. In this case, assuming that the
    > > primary
    > > MDP device is the one with a specific alias seems useable because
    > > the
    > > alternative is to add a property to the device tree which doesn't
    > > actually represent any facet of hardware (i.e., this being the
    > > primary
    > > MDP device is a software decision). In other words, this solution
    > > is
    > > equally as arbitrary, but at least it doesn't add a property to a
    > > device node where said property is unrelated to the hardware
    > > present.
    > >
    > > Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
    > > ---
    > >
    > > (no changes since v1)
    > >
    > > drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 56 +++++++++++++--
    > > ----
    > > drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 36 ++++++++----
    > > 2 files changed, 64 insertions(+), 28 deletions(-)
    > >
    > > diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
    > > b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
    > > index 85ef274841a3..9527649de98e 100644
    > > --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
    > > +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
    > > @@ -151,29 +151,50 @@ void mtk_mdp_comp_clock_off(struct
    > > mtk_mdp_comp
    > > *comp)
    > > mtk_smi_larb_put(comp->larb_dev);
    > > }
    > >
    > > -static int mtk_mdp_comp_bind(struct device *dev, struct device
    > > *master, void *data)
    > > +/*
    > > + * The MDP master device node is identified by the device tree
    > > alias
    > > + * "mdp-rdma0".
    > > + */
    > > +static bool is_mdp_master(struct device *dev)
    > > +{
    > > + struct device_node *aliases, *mdp_rdma0_node;
    > > + const char *mdp_rdma0_path;
    > > +
    > > + if (!dev->of_node)
    > > + return false;
    > > +
    > > + aliases = of_find_node_by_path("/aliases");
    > > + if (!aliases) {
    > > + dev_err(dev, "no aliases found for mdp-rdma0");
    > > + return false;
    > > + }
    > > +
    > > + mdp_rdma0_path = of_get_property(aliases, "mdp-rdma0", NULL);
    > > + if (!mdp_rdma0_path) {
    > > + dev_err(dev, "get mdp-rdma0 property of /aliases
    > > failed");
    > > + return false;
    > > + }
    > > +
    > > + mdp_rdma0_node = of_find_node_by_path(mdp_rdma0_path);
    > > + if (!mdp_rdma0_node) {
    > > + dev_err(dev, "path resolution failed for %s",
    > > mdp_rdma0_path);
    > > + return false;
    > > + }
    > > +
    >
    > Hi Eizan,
    >
    > "mdp-rdma0" may be not the only one master device node. In fact,
    > there
    > are 2 "mdp-rdma" in mt8173. You can see "mdp_rdma1" via below link.
    >
    >
    https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v5.13.11#n1016
    > If we add "mediatek,mt8173-mdp" to "mdp_rdma1" like below, we will
    > have
    > one more V4L2 video devie node.
    >
    > mdp_rdma1: rdma@14002000 {
    > compatible = "mediatek,mt8173-mdp-rdma",
    > "mediatek,mt8173-mdp";
    > ...
    > }
    >
    > We should consider the case that there are more than one "MDP_RDMA"
    > components.
    >
    > Regards,
    > Houlong
    >
    > > + return dev->of_node == mdp_rdma0_node;
    > > +}
    > > +
    > > +static int mtk_mdp_comp_bind(struct device *dev, struct device
    > > *master,
    > > + void *data)
    > > {
    > > 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) {
    > > + if (is_mdp_master(dev)) {
    > > 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);
    > > - }
    > > -
    > > ret = v4l2_device_register(dev, &mdp->v4l2_dev);
    > > if (ret) {
    > > dev_err(dev, "Failed to register v4l2
    > > device\n");
    > > @@ -187,9 +208,8 @@ static int mtk_mdp_comp_bind(struct device
    > > *dev,
    > > struct device *master, void *da
    > > }
    > >
    > > /*
    > > - * 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 DMA ops will be handled by the DMA callbacks
    > > associated with this
    > > + * device;
    > > */
    > > mdp->rdma_dev = dev;
    > > }
    > > diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
    > > b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
    > > index 50eafcc9993d..6a775463399c 100644
    > > --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
    > > +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
    > > @@ -150,8 +150,9 @@ static void release_of(struct device *dev, void
    > > *data)
    > >
    > > static int mtk_mdp_master_bind(struct device *dev)
    > > {
    > > - int status;
    > > struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);
    > > + struct device_node *vpu_node;
    > > + int status;
    > >
    > > status = component_bind_all(dev, mdp);
    > > if (status) {
    > > @@ -159,15 +160,30 @@ 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");
    > > + if (mdp->rdma_dev == NULL) {
    > > + dev_err(dev, "Primary MDP device not found");
    > > + status = -ENODEV;
    > > + goto err_component_bind_all;
    > > + }
    > > +
    > > + vpu_node = of_find_node_by_name(NULL, "vpu");
    > > + if (!vpu_node) {
    > > + dev_err(dev, "unable to find vpu node");
    > > + status = -ENODEV;
    > > + goto err_wdt_reg;
    > > + }

    Hi Eizan,

    Is your removing "mediatek,vpu" necessary for this series "Refactor MTK
    MDP driver into core/components" ?

    In some MediaTek projects (not upstream yet), the device-tree node name
    "vpu" may be changed to something like "vpu0", "vpu1" or other name,
    which depends on the implementation of "mtk-vpu" driver. We can specify
    the different phandle to "mediatek,vpu" in .dtsi file. If we use
    of_find_node_by_name() to get the vpu_node, we have to modify the name
    string in different project.
    If the answer of my previous questions is "No", I prefer to slow down
    the modification of removing "mediatek,vpu".

    Sorry for late reply.

    Regards,
    Houlong

    > > +
    > > + mdp->vpu_dev = of_find_device_by_node(vpu_node);
    > > + if (!mdp->vpu_dev) {
    > > + dev_err(dev, "unable to find vpu device");
    > > + status = -ENODEV;
    > > + goto err_wdt_reg;
    > > + }
    > > +
    > > + status = vpu_wdt_reg_handler(mdp->vpu_dev,
    > > mtk_mdp_reset_handler, mdp, VPU_RST_MDP);
    > > + if (status) {
    > > + dev_err(dev, "Failed to register reset handler\n");
    > > + goto err_wdt_reg;
    > > }
    > >
    > > status = mtk_mdp_register_m2m_device(mdp);
    > > --
    > > 2.32.0.554.ge1b32706d8-goog
    > >
    >
    >
    \
     
     \ /
      Last update: 2021-08-16 06:54    [W:3.370 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site