Re: [PATCH v3 03/16] media: mtk-vcodec: add SCP firmware ops

From: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
To: Alexandre Courbot <acourbot@chromium.org>
Cc: Tiffany Lin <tiffany.lin@mediatek.com>,
	Andrew-CT Chen <andrew-ct.chen@mediatek.com>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Yunfei Dong <yunfei.dong@mediatek.com>,
	Maoguang Meng <maoguang.meng@mediatek.com>,
	linux-media <linux-media@vger.kernel.org>,
	"moderated list:ARM/Mediatek SoC support" 
	<linux-mediatek@lists.infradead.org>,
	devicetree <devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 03/16] media: mtk-vcodec: add SCP firmware ops
Date: Mon, 27 Jul 2020 11:09:25 -0300	[thread overview]
Message-ID: <CAAEAJfD6tiJKLNoSJbvJkafLrU8s5Y8mZOg038SuH1QNCO-+_g@mail.gmail.com> (raw)
In-Reply-To: <CAPBb6MV=oo-a9POY_qedcJYU_qSJ695FwJthBrp3SQUK+g0JvA@mail.gmail.com>

On Mon, 27 Jul 2020 at 06:06, Alexandre Courbot <acourbot@chromium.org> wrote:
>
> On Thu, Jul 23, 2020 at 6:40 AM Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
> >
> > On Mon, 13 Jul 2020 at 03:09, Alexandre Courbot <acourbot@chromium.org> wrote:
> > >
> > > From: Yunfei Dong <yunfei.dong@mediatek.com>
> > >
> > > Add support for communicating with the SCP firmware, which will be used
> > > by MT8183.
> > >
> > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> > > [acourbot: refactor, cleanup and split]
> > > Co-developed-by: Alexandre Courbot <acourbot@chromium.org>
> > > Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> > > Acked-by: Tiffany Lin <tiffany.lin@mediatek.com>
> > > ---
> > >  drivers/media/platform/Kconfig                |  1 +
> > >  .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c  |  3 +
> > >  .../platform/mtk-vcodec/mtk_vcodec_enc_drv.c  |  3 +
> > >  .../media/platform/mtk-vcodec/mtk_vcodec_fw.c | 56 +++++++++++++++++++
> > >  .../media/platform/mtk-vcodec/mtk_vcodec_fw.h |  2 +
> > >  5 files changed, 65 insertions(+)
> > >
> > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > > index c57ee78fa99d..f0dbe048efea 100644
> > > --- a/drivers/media/platform/Kconfig
> > > +++ b/drivers/media/platform/Kconfig
> > > @@ -256,6 +256,7 @@ config VIDEO_MEDIATEK_VCODEC
> > >         select VIDEOBUF2_DMA_CONTIG
> > >         select V4L2_MEM2MEM_DEV
> > >         select VIDEO_MEDIATEK_VPU
> > > +       select MTK_SCP
> > >         help
> > >             Mediatek video codec driver provides HW capability to
> > >             encode and decode in a range of video formats
> > > 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 4f07a5fcce7f..5b5765b98e57 100644
> > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > > @@ -225,6 +225,9 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
> > >         if (!of_property_read_u32(pdev->dev.of_node, "mediatek,vpu",
> > >                                   &rproc_phandle)) {
> > >                 fw_type = VPU;
> > > +       } else if (!of_property_read_u32(pdev->dev.of_node, "mediatek,scp",
> > > +                                        &rproc_phandle)) {
> > > +               fw_type = SCP;
> > >         } else {
> > >                 mtk_v4l2_err("Could not get vdec IPI device");
> > >                 return -ENODEV;
> > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
> > > index 4340ea10afd0..42530cd01a30 100644
> > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
> > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
> > > @@ -233,6 +233,9 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
> > >         if (!of_property_read_u32(pdev->dev.of_node, "mediatek,vpu",
> > >                                   &rproc_phandle)) {
> > >                 fw_type = VPU;
> > > +       } else if (!of_property_read_u32(pdev->dev.of_node, "mediatek,scp",
> > > +                                        &rproc_phandle)) {
> > > +               fw_type = SCP;
> > >         } else {
> > >                 mtk_v4l2_err("Could not get venc IPI device");
> > >                 return -ENODEV;
> > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> > > index 967bb100a990..f2a62ea62fc6 100644
> > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> > > @@ -19,6 +19,7 @@ struct mtk_vcodec_fw {
> > >         enum mtk_vcodec_fw_type type;
> > >         const struct mtk_vcodec_fw_ops *ops;
> > >         struct platform_device *pdev;
> > > +       struct mtk_scp *scp;
> > >  };
> > >
> > >  static int mtk_vcodec_vpu_load_firmware(struct mtk_vcodec_fw *fw)
> > > @@ -71,6 +72,48 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
> > >         .ipi_send = mtk_vcodec_vpu_ipi_send,
> > >  };
> > >
> > > +static int mtk_vcodec_scp_load_firmware(struct mtk_vcodec_fw *fw)
> > > +{
> > > +       return rproc_boot(scp_get_rproc(fw->scp));
> > > +}
> > > +
> > > +static unsigned int mtk_vcodec_scp_get_vdec_capa(struct mtk_vcodec_fw *fw)
> > > +{
> > > +       return scp_get_vdec_hw_capa(fw->scp);
> > > +}
> > > +
> > > +static unsigned int mtk_vcodec_scp_get_venc_capa(struct mtk_vcodec_fw *fw)
> > > +{
> > > +       return scp_get_venc_hw_capa(fw->scp);
> > > +}
> > > +
> > > +static void *mtk_vcodec_vpu_scp_dm_addr(struct mtk_vcodec_fw *fw,
> > > +                                       u32 dtcm_dmem_addr)
> > > +{
> > > +       return scp_mapping_dm_addr(fw->scp, dtcm_dmem_addr);
> > > +}
> > > +
> > > +static int mtk_vcodec_scp_set_ipi_register(struct mtk_vcodec_fw *fw, int id,
> > > +               mtk_vcodec_ipi_handler handler, const char *name, void *priv)
> > > +{
> > > +       return scp_ipi_register(fw->scp, id, handler, priv);
> > > +}
> > > +
> > > +static int mtk_vcodec_scp_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
> > > +               unsigned int len, unsigned int wait)
> > > +{
> > > +       return scp_ipi_send(fw->scp, id, buf, len, wait);
> > > +}
> > > +
> > > +static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
> > > +       .load_firmware = mtk_vcodec_scp_load_firmware,
> > > +       .get_vdec_capa = mtk_vcodec_scp_get_vdec_capa,
> > > +       .get_venc_capa = mtk_vcodec_scp_get_venc_capa,
> > > +       .map_dm_addr = mtk_vcodec_vpu_scp_dm_addr,
> > > +       .ipi_register = mtk_vcodec_scp_set_ipi_register,
> > > +       .ipi_send = mtk_vcodec_scp_ipi_send,
> > > +};
> > > +
> > >  static void mtk_vcodec_reset_handler(void *priv)
> > >  {
> > >         struct mtk_vcodec_dev *dev = priv;
> > > @@ -94,6 +137,7 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
> > >         const struct mtk_vcodec_fw_ops *ops;
> > >         struct mtk_vcodec_fw *fw;
> > >         struct platform_device *fw_pdev = NULL;
> > > +       struct mtk_scp *scp = NULL;
> > >
> > >         switch (type) {
> > >         case VPU:
> > > @@ -106,6 +150,14 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
> > >                 vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_reset_handler,
> > >                                     dev, rst_id);
> > >                 break;
> > > +       case SCP:
> > > +               ops = &mtk_vcodec_rproc_msg;
> > > +               scp = scp_get(dev->plat_dev);
> > > +               if (!scp) {
> > > +                       mtk_v4l2_err("could not get vdec scp handle");
> > > +                       return ERR_PTR(-EPROBE_DEFER);
> >
> > I suspect the EPROBE_DEFER should be returned by scp_get
> > itself instead.
>
> scp_get() is a function of of mtk_scp remoteproc driver, so even if we
> decide this is desirable (which I am not convinced, as the current
> code leaves the freedom to decide how the absence of SCP should be
> interpreted to the driver) this is beyond the scope of this series.
>

How can the consumer decide if it should defer probing or not?

> >
> > > +               }
> > > +               break;
> > >         default:
> > >                 mtk_v4l2_err("invalid vcodec fw type");
> > >                 return ERR_PTR(-EINVAL);
> > > @@ -118,6 +170,7 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
> > >         fw->type = type;
> > >         fw->ops = ops;
> > >         fw->pdev = fw_pdev;
> > > +       fw->scp = scp;
> > >
> > >         return fw;
> > >  }
> > > @@ -129,6 +182,9 @@ void mtk_vcodec_fw_release(struct mtk_vcodec_fw *fw)
> > >         case VPU:
> > >                 put_device(&fw->pdev->dev);
> > >                 break;
> > > +       case SCP:
> > > +               scp_put(fw->scp);
> >
> > Interestingly scp_put is a wrapper around put_device :-)
> > Perhaps not a reason to violate the layering.
>
> I don't see what is wrong with the current code? If SCP is in use, we
> use SCP functions to manage it. If in the future SCP involves in such
> a way that we need to do more than a put_device(), we are covered. Or
> am I missing something?

Oh, nothing wrong. I just found it interesting that scp_put was
just wrapping put_device. Nothing to fix really.

Thanks!
Ezequiel