LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
To: Eizan Miyamoto <eizan@chromium.org>, Hans Verkuil <hverkuil@xs4all.nl>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	chunkuang.hu@kernel.org, Yong Wu <yong.wu@mediatek.com>,
	wenst@chromium.org, ck.hu@mediatek.com, houlong.wei@mediatek.com,
	Enric Balletbo i Serra <enric.balletbo@collabora.com>,
	yongqiang.niu@mediatek.com,
	Andrew-CT Chen <andrew-ct.chen@mediatek.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Minghsiu Tsai <minghsiu.tsai@mediatek.com>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	linux-media <linux-media@vger.kernel.org>,
	"moderated list:ARM/Mediatek SoC support" 
	<linux-mediatek@lists.infradead.org>
Subject: Re: [PATCH v7 2/7] mtk-mdp: add driver to probe mdp components
Date: Sat, 4 Sep 2021 09:34:11 -0300	[thread overview]
Message-ID: <CAAEAJfDNDXdJFfB-bHhFcqnnKZ0TY--d_wLGddKRymQOLQY4TQ@mail.gmail.com> (raw)
In-Reply-To: <20210825163247.v7.2.Ie6d1e6e39cf9b5d6b2108ae1096af34c3d55880b@changeid>

Hi Eizan,

Sorry for seeing this series so late.

On Wed, 25 Aug 2021 at 03:35, Eizan Miyamoto <eizan@chromium.org> wrote:
>
> Broadly, this patch (1) adds a driver for various MTK MDP components to
> go alongside the main MTK MDP driver, and (2) hooks them all together
> using the component framework.
>
> (1) Up until now, the MTK MDP driver controls 8 devices in the device
> tree on its own. When running tests for the hardware video decoder, we
> found that the iommus and LARBs were not being properly configured.

Why were not being properly configured? What was the problem?
Why not fixing that instead?

Does this mean the driver is currently broken and unusable?

> To
> configure them, a driver for each be added to mtk_mdp_comp so that
> mtk_iommu_add_device() can (eventually) be called from dma_configure()
> inside really_probe().
>
> (2) The integration into the component framework allows us to defer the
> registration with the v4l2 subsystem until all the MDP-related devices
> have been probed, so that the relevant device node does not become
> available until initialization of all the components is complete.
>
> Some notes about how the component framework has been integrated:
>
> - The driver for the rdma0 component serves double duty as the "master"
>   (aggregate) driver as well as a component driver. This is a non-ideal
>   compromise until a better solution is developed. This device is
>   differentiated from the rest by checking for a "mediatek,vpu" property
>   in the device node.
>

As I have stated in Yunfei, I am not convinced you need an async framework
at all. It seems all these devices could have been linked together
in the device tree, and then have a master device to tie them.

I.e. something like

mdp {
  mdp_rdma0 {
  }
  mdp_rsz0 {
  }
  mdp_rsz1 {
  }
}

All this async games seem like making the driver really obfuscated,
which will mean harder to debug and maintain.
I am not sure we want that burden.

Even if we are all fully convinced that you absolutely need
an async framework, then what's wrong with v4l2-async?

I would start by addressing what is wrong with the IOMMUs
in the current design.

Thanks,
Ezequiel

> - The list of mdp components remains hard-coded as mtk_mdp_comp_dt_ids[]
>   in mtk_mdp_core.c, and as mtk_mdp_comp_driver_dt_match[] in
>   mtk_mdp_comp.c. This unfortunate duplication of information is
>   addressed in a following patch in this series.
>
> - The component driver calls component_add() for each device that is
>   probed.
>
> - In mtk_mdp_probe (the "master" device), we scan the device tree for
>   any matching nodes against mtk_mdp_comp_dt_ids, and add component
>   matches for them. The match criteria is a matching device node
>   pointer.
>
> - When the set of components devices that have been probed corresponds
>   with the list that is generated by the "master", the callback to
>   mtk_mdp_master_bind() is made, which then calls the component bind
>   functions.
>
> - Inside mtk_mdp_master_bind(), once all the component bind functions
>   have been called, we can then register our device to the v4l2
>   subsystem.
>
> - The call to pm_runtime_enable() in the master device is called after
>   all the components have been registered by their bind() functions
>   called by mtk_mtp_master_bind(). As a result, the list of components
>   will not change while power management callbacks mtk_mdp_suspend()/
>   resume() are accessing the list of components.
>
> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Reviewed-by: Houlong Wei <houlong.wei@mediatek.com>
> ---
>

  reply	other threads:[~2021-09-04 12:34 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-25  6:33 [PATCH v7 0/7] Refactor MTK MDP driver into core/components Eizan Miyamoto
2021-08-25  6:33 ` [PATCH v7 1/7] mtk-mdp: propagate errors from clock_on Eizan Miyamoto
2021-08-25  6:33 ` [PATCH v7 2/7] mtk-mdp: add driver to probe mdp components Eizan Miyamoto
2021-09-04 12:34   ` Ezequiel Garcia [this message]
2021-09-05 16:23     ` houlong wei
2021-09-05 16:39       ` houlong wei
2021-12-07 12:37       ` Hans Verkuil
2021-12-07 12:46         ` Ezequiel Garcia
2021-12-07 13:02           ` Hans Verkuil
2021-12-08  2:00       ` Yong Wu
2021-08-25  6:33 ` [PATCH v7 3/7] mtk-mdp: use pm_runtime in MDP component driver Eizan Miyamoto
2021-08-25  6:33 ` [PATCH v7 4/7] media: mtk-mdp: don't pm_run_time_get/put for master comp in clock_on Eizan Miyamoto
2021-08-25  6:33 ` [PATCH v7 5/7] mtk-mdp: make mdp driver to be loadable by platform_device_register*() Eizan Miyamoto
2021-08-25  8:18   ` houlong wei
2021-08-25  6:33 ` [PATCH v7 6/7] soc: mediatek: mmsys: instantiate mdp virtual device from mmsys Eizan Miyamoto
2021-08-25  6:33 ` [PATCH v7 7/7] media: mtk-mdp: use mdp-rdma0 alias to point to MDP master Eizan Miyamoto
2021-08-25  8:43   ` houlong wei
2021-08-25  9:07     ` Enric Balletbo i Serra
2021-08-25 15:18       ` houlong wei
2021-08-25 16:54         ` Enric Balletbo i Serra
2021-08-30 17:14           ` houlong wei

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAAEAJfDNDXdJFfB-bHhFcqnnKZ0TY--d_wLGddKRymQOLQY4TQ@mail.gmail.com \
    --to=ezequiel@vanguardiasur.com.ar \
    --cc=andrew-ct.chen@mediatek.com \
    --cc=chunkuang.hu@kernel.org \
    --cc=ck.hu@mediatek.com \
    --cc=eizan@chromium.org \
    --cc=enric.balletbo@collabora.com \
    --cc=houlong.wei@mediatek.com \
    --cc=hverkuil@xs4all.nl \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=mchehab@kernel.org \
    --cc=minghsiu.tsai@mediatek.com \
    --cc=wenst@chromium.org \
    --cc=yong.wu@mediatek.com \
    --cc=yongqiang.niu@mediatek.com \
    --subject='Re: [PATCH v7 2/7] mtk-mdp: add driver to probe mdp components' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).