Message-ID: <CAHCN7xKtD+ROTM5_bCr0EgC+vyoAAgEW11SkCvpPQ5wHO3bWBA@mail.gmail.com>
Date:   Tue, 7 Dec 2021 08:07:27 -0600
From:   Adam Ford <aford173@...il.com>
To:     Lucas Stach <l.stach@...gutronix.de>
Cc:     Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
        linux-media <linux-media@...r.kernel.org>,
        Chris Healy <cphealy@...il.com>,
        Benjamin Gaignard <benjamin.gaignard@...labora.com>,
        Hans Verkuil <hverkuil@...all.nl>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "open list:HANTRO VPU CODEC DRIVER" 
        <linux-rockchip@...ts.infradead.org>,
        devicetree <devicetree@...r.kernel.org>,
        arm-soc <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:STAGING SUBSYSTEM" <linux-staging@...ts.linux.dev>
Subject: Re: [RFC V2 5/6] media: hantro: split i.MX8MQ G1 and G2 code

On Tue, Dec 7, 2021 at 7:56 AM Lucas Stach <l.stach@...gutronix.de> wrote:
>
> Am Dienstag, dem 07.12.2021 um 07:50 -0600 schrieb Adam Ford:
> > On Tue, Dec 7, 2021 at 7:41 AM Lucas Stach <l.stach@...gutronix.de> wrote:
> > >
> > > Am Dienstag, dem 07.12.2021 um 07:24 -0600 schrieb Adam Ford:
> > > > On Tue, Dec 7, 2021 at 6:39 AM Ezequiel Garcia
> > > > <ezequiel@...guardiasur.com.ar> wrote:
> > > > >
> > > > > Hi Adam,
> > > > >
> > > > > Thanks for the good work! This is looking quite promising.
> > > > >
> > > > > On Mon, Dec 06, 2021 at 07:54:44PM -0600, Adam Ford wrote:
> > > > > > The VPU in the i.MX8MQ is really the combination of Hantro G1 and
> > > > > > Hantro G2. With the updated vpu-blk-ctrl, the power domains system
> > > > > > can enable and disable them separately as well as pull them out of
> > > > > > reset. This simplifies the code and lets them run independently.
> > > > > >
> > > > > > Signed-off-by: Adam Ford <aford173@...il.com>
> > > > > >
> > > > > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > > > > > index ab2467998d29..d803252a5aba 100644
> > > > > > --- a/drivers/staging/media/hantro/hantro_drv.c
> > > > > > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > > > > > @@ -608,8 +608,8 @@ static const struct of_device_id of_hantro_match[] = {
> > > > > >       { .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, },
> > > > > >  #endif
> > > > > >  #ifdef CONFIG_VIDEO_HANTRO_IMX8M
> > > > > > -     { .compatible = "nxp,imx8mq-vpu", .data = &imx8mq_vpu_variant, },
> > > > > > -     { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant },
> > > > > > +     { .compatible = "nxp,imx8mq-vpu-g1", .data = &imx8mq_vpu_g1_variant, },
> > > > >
> > > > > I think it's important to clarify that you are breaking support
> > > > > for the previous device-tree binding. Not only because of the compatible
> > > > > string change, but because the binding is now quite different.
> > > > >
> > > > > Note that in the past Benjamin tried to avoid this.
> > > > > IIRC, his proposal was backwards compatible.
> > > >
> > > > I was looking at how to do this, but the stand-alone vpu bindings did
> > > > the reset and the fuses manually, it causes issues.  When the block
> > > > control is enabled those memory locations for the resets and fuses are
> > > > assigned to the blk-ctrl driver, so attempting to access them from a
> > > > different driver was a violation.
> > > >
> > > > When I started looking at this, the stand-alone VPU was trying to be
> > > > both G1 and G2, but when I was testing it before I made changes, I
> > > > didn't see the G2 function at all. When I was done the G2 seemed to
> > > > work, so it seems like this is an improvement.  If you want me to keep
> > > > the previous compatible flag, I could rename the nxp,imx8mq-vpu-g1
> > > > back to nxp,imx8mq-vpu and remove the reset/fuse controls, but I'd
> > > > have to add the blk-ctrl reference, so it seemed to me like a better
> > > > alternative to deprecate the old binding/driver and replace it with
> > > > the new one because of the significant changes.  Since I'd like to
> > > > rebase the i.MX8M Mini I did on this work, it seemed weird to have
> > > > nxp,imx8mq-vpu, nxp,imx8mq-vpu-g2, nxp,imx8mm-vpu-g1, and
> > > > nxp,imx8mm-vpu-g2 where the only one without a Gx name was the
> > > > original 8MQ binding but limited to G1 functionality and the G2
> > > > stripped out.
> > >
> > > I would very much appreciate if we could keep the driver code for the
> > > old binding. It does not need to have any new additional functionality,
> > > but just keep the existing G1 h.264 decode when booted on a old DT with
> > > the old VPU description and no blk-ctrl, so we don't regress
> > > functionality when a new kernel is booted with a old DT.
> > >
> > > New functionality with the G2 can depend on the new VPU binding and the
> > > blk-ctrl driver.
> >
> > How does that work when both the original VPU and the blk-ctrl attempt
> > to manipulate the reset and clock lines?  The original binding for the
> > vpu was assigned:
> >
> > reg = <0x38300000 0x10000>,
> >           <0x38310000 0x10000>,
> >           <0x38320000 0x10000>;
> > reg-names = "g1", "g2", "ctrl";
> >
> > If G2 is going to run from 38310000 and vpu-blk-ctrl run from
> > 38320000, they'll collide.
> >
> It's not going to work, but it also doesn't have to. Either you have a
> old DT where the VPU driver will poke the blk-ctrl registers, but no
> blk-ctrl driver, or you have a new DT where the VPU driver leaves the
> blk-ctrl region alone and the blk-ctrl driver needs to handle it.
>
> Just don't support mixing the old VPU DT binding with the new blk-ctrl
> way of doing things. The only thing that needs to keep working is a
> unchannged old DT, where the VPU uses the old binding, but no blk-ctrl
> is present as a separate node.

I think I understand.  I'll leave the old code for the old binding in
the driver and add the new code for the new bindings with blk-ctrl.
Once the device tree is migrated to the new bindings, the old one
becomes harmless, but still works with old device trees lacking the
blk-ctrl.  That makes sense.  In my head, I wasn't thinking about
mixing older device trees.

adam
>
> Regards,
> Lucas
> >
> > >
> > > Regards,
> > > Lucas
> > >
> > > > >
> > > > > If this is unavoidable, due to how the blk-ctrl is handled, then that's
> > > > > fine. Given it's a staging driver, we can still play these games.
> > > > >
> > > > > Having said that, let's please make this very clear in the commit
> > > > > description, to it's clear for developers forward-porting their kernels.
> > > > > This applies not only to this commit, but to all commits that affect
> > > > > the binding.
> > > >
> > > > When I post the formal patch, I will add a note that this explicitly
> > > > deprecates the old bindings due to the fact that the G1 and G2 are
> > > > separate IP blocks in the reference manual, and blk-ctrl is a
> > > > completely separate driver altogether.
> > > >
> > > > >
> > > > > Thanks!
> > > > > Ezequiel
> > > > >
> > > > > > +     { .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant, },
> > > > > >  #endif
> > > > > >  #ifdef CONFIG_VIDEO_HANTRO_SAMA5D4
> > > > > >       { .compatible = "microchip,sama5d4-vdec", .data = &sama5d4_vdec_variant, },
> > > > > > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> > > > > > index cff817ca8d22..122b83a16663 100644
> > > > > > --- a/drivers/staging/media/hantro/hantro_hw.h
> > > > > > +++ b/drivers/staging/media/hantro/hantro_hw.h
> > > > > > @@ -299,8 +299,8 @@ enum hantro_enc_fmt {
> > > > > >       ROCKCHIP_VPU_ENC_FMT_UYVY422 = 3,
> > > > > >  };
> > > > > >
> > > > > > +extern const struct hantro_variant imx8mq_vpu_g1_variant;
> > > > > >  extern const struct hantro_variant imx8mq_vpu_g2_variant;
> > > > > > -extern const struct hantro_variant imx8mq_vpu_variant;
> > > > > >  extern const struct hantro_variant px30_vpu_variant;
> > > > > >  extern const struct hantro_variant rk3036_vpu_variant;
> > > > > >  extern const struct hantro_variant rk3066_vpu_variant;
> > > > > > diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > > > > index 1a43f6fceef9..c9f6e8472258 100644
> > > > > > --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > > > > +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > > > > @@ -13,67 +13,6 @@
> > > > > >  #include "hantro_g1_regs.h"
> > > > > >  #include "hantro_g2_regs.h"
> > > > > >
> > > > > > -#define CTRL_SOFT_RESET              0x00
> > > > > > -#define RESET_G1             BIT(1)
> > > > > > -#define RESET_G2             BIT(0)
> > > > > > -
> > > > > > -#define CTRL_CLOCK_ENABLE    0x04
> > > > > > -#define CLOCK_G1             BIT(1)
> > > > > > -#define CLOCK_G2             BIT(0)
> > > > > > -
> > > > > > -#define CTRL_G1_DEC_FUSE     0x08
> > > > > > -#define CTRL_G1_PP_FUSE              0x0c
> > > > > > -#define CTRL_G2_DEC_FUSE     0x10
> > > > > > -
> > > > > > -static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits)
> > > > > > -{
> > > > > > -     u32 val;
> > > > > > -
> > > > > > -     /* Assert */
> > > > > > -     val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> > > > > > -     val &= ~reset_bits;
> > > > > > -     writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> > > > > > -
> > > > > > -     udelay(2);
> > > > > > -
> > > > > > -     /* Release */
> > > > > > -     val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> > > > > > -     val |= reset_bits;
> > > > > > -     writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> > > > > > -}
> > > > > > -
> > > > > > -static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits)
> > > > > > -{
> > > > > > -     u32 val;
> > > > > > -
> > > > > > -     val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> > > > > > -     val |= clock_bits;
> > > > > > -     writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> > > > > > -}
> > > > > > -
> > > > > > -static int imx8mq_runtime_resume(struct hantro_dev *vpu)
> > > > > > -{
> > > > > > -     int ret;
> > > > > > -
> > > > > > -     ret = clk_bulk_prepare_enable(vpu->variant->num_clocks, vpu->clocks);
> > > > > > -     if (ret) {
> > > > > > -             dev_err(vpu->dev, "Failed to enable clocks\n");
> > > > > > -             return ret;
> > > > > > -     }
> > > > > > -
> > > > > > -     imx8m_soft_reset(vpu, RESET_G1 | RESET_G2);
> > > > > > -     imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2);
> > > > > > -
> > > > > > -     /* Set values of the fuse registers */
> > > > > > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G1_DEC_FUSE);
> > > > > > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G1_PP_FUSE);
> > > > > > -     writel(0xffffffff, vpu->ctrl_base + CTRL_G2_DEC_FUSE);
> > > > > > -
> > > > > > -     clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks);
> > > > > > -
> > > > > > -     return 0;
> > > > > > -}
> > > > > > -
> > > > > >  /*
> > > > > >   * Supported formats.
> > > > > >   */
> > > > > > @@ -209,27 +148,6 @@ static irqreturn_t imx8m_vpu_g2_irq(int irq, void *dev_id)
> > > > > >       return IRQ_HANDLED;
> > > > > >  }
> > > > > >
> > > > > > -static int imx8mq_vpu_hw_init(struct hantro_dev *vpu)
> > > > > > -{
> > > > > > -     vpu->ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1];
> > > > > > -
> > > > > > -     return 0;
> > > > > > -}
> > > > > > -
> > > > > > -static void imx8m_vpu_g1_reset(struct hantro_ctx *ctx)
> > > > > > -{
> > > > > > -     struct hantro_dev *vpu = ctx->dev;
> > > > > > -
> > > > > > -     imx8m_soft_reset(vpu, RESET_G1);
> > > > > > -}
> > > > > > -
> > > > > > -static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
> > > > > > -{
> > > > > > -     struct hantro_dev *vpu = ctx->dev;
> > > > > > -
> > > > > > -     imx8m_soft_reset(vpu, RESET_G2);
> > > > > > -}
> > > > > > -
> > > > > >  /*
> > > > > >   * Supported codec ops.
> > > > > >   */
> > > > > > @@ -237,19 +155,16 @@ static void imx8m_vpu_g2_reset(struct hantro_ctx *ctx)
> > > > > >  static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
> > > > > >       [HANTRO_MODE_MPEG2_DEC] = {
> > > > > >               .run = hantro_g1_mpeg2_dec_run,
> > > > > > -             .reset = imx8m_vpu_g1_reset,
> > > > > >               .init = hantro_mpeg2_dec_init,
> > > > > >               .exit = hantro_mpeg2_dec_exit,
> > > > > >       },
> > > > > >       [HANTRO_MODE_VP8_DEC] = {
> > > > > >               .run = hantro_g1_vp8_dec_run,
> > > > > > -             .reset = imx8m_vpu_g1_reset,
> > > > > >               .init = hantro_vp8_dec_init,
> > > > > >               .exit = hantro_vp8_dec_exit,
> > > > > >       },
> > > > > >       [HANTRO_MODE_H264_DEC] = {
> > > > > >               .run = hantro_g1_h264_dec_run,
> > > > > > -             .reset = imx8m_vpu_g1_reset,
> > > > > >               .init = hantro_h264_dec_init,
> > > > > >               .exit = hantro_h264_dec_exit,
> > > > > >       },
> > > > > > @@ -258,14 +173,12 @@ static const struct hantro_codec_ops imx8mq_vpu_codec_ops[] = {
> > > > > >  static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = {
> > > > > >       [HANTRO_MODE_HEVC_DEC] = {
> > > > > >               .run = hantro_g2_hevc_dec_run,
> > > > > > -             .reset = imx8m_vpu_g2_reset,
> > > > > >               .init = hantro_hevc_dec_init,
> > > > > >               .exit = hantro_hevc_dec_exit,
> > > > > >       },
> > > > > >       [HANTRO_MODE_VP9_DEC] = {
> > > > > >               .run = hantro_g2_vp9_dec_run,
> > > > > >               .done = hantro_g2_vp9_dec_done,
> > > > > > -             .reset = imx8m_vpu_g2_reset,
> > > > > >               .init = hantro_vp9_dec_init,
> > > > > >               .exit = hantro_vp9_dec_exit,
> > > > > >       },
> > > > > > @@ -275,7 +188,7 @@ static const struct hantro_codec_ops imx8mq_vpu_g2_codec_ops[] = {
> > > > > >   * VPU variants.
> > > > > >   */
> > > > > >
> > > > > > -static const struct hantro_irq imx8mq_irqs[] = {
> > > > > > +static const struct hantro_irq imx8mq_g1_irqs[] = {
> > > > > >       { "g1", imx8m_vpu_g1_irq },
> > > > > >  };
> > > > > >
> > > > > > @@ -283,10 +196,12 @@ static const struct hantro_irq imx8mq_g2_irqs[] = {
> > > > > >       { "g2", imx8m_vpu_g2_irq },
> > > > > >  };
> > > > > >
> > > > > > -static const char * const imx8mq_clk_names[] = { "g1", "g2", "bus" };
> > > > > > -static const char * const imx8mq_reg_names[] = { "g1", "g2", "ctrl" };
> > > > > > +static const char * const imx8mq_g1_clk_names[] = { "g1" };
> > > > > > +static const char * const imx8mq_g1_reg_names[] = { "g1" };
> > > > > > +static const char * const imx8mq_g2_clk_names[] = { "g2" };
> > > > > > +static const char * const imx8mq_g2_reg_names[] = { "g2" };
> > > > > >
> > > > > > -const struct hantro_variant imx8mq_vpu_variant = {
> > > > > > +const struct hantro_variant imx8mq_vpu_g1_variant = {
> > > > > >       .dec_fmts = imx8m_vpu_dec_fmts,
> > > > > >       .num_dec_fmts = ARRAY_SIZE(imx8m_vpu_dec_fmts),
> > > > > >       .postproc_fmts = imx8m_vpu_postproc_fmts,
> > > > > > @@ -295,14 +210,12 @@ const struct hantro_variant imx8mq_vpu_variant = {
> > > > > >       .codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER |
> > > > > >                HANTRO_H264_DECODER,
> > > > > >       .codec_ops = imx8mq_vpu_codec_ops,
> > > > > > -     .init = imx8mq_vpu_hw_init,
> > > > > > -     .runtime_resume = imx8mq_runtime_resume,
> > > > > > -     .irqs = imx8mq_irqs,
> > > > > > -     .num_irqs = ARRAY_SIZE(imx8mq_irqs),
> > > > > > -     .clk_names = imx8mq_clk_names,
> > > > > > -     .num_clocks = ARRAY_SIZE(imx8mq_clk_names),
> > > > > > -     .reg_names = imx8mq_reg_names,
> > > > > > -     .num_regs = ARRAY_SIZE(imx8mq_reg_names)
> > > > > > +     .irqs = imx8mq_g1_irqs,
> > > > > > +     .num_irqs = ARRAY_SIZE(imx8mq_g1_irqs),
> > > > > > +     .clk_names = imx8mq_g1_clk_names,
> > > > > > +     .num_clocks = ARRAY_SIZE(imx8mq_g1_clk_names),
> > > > > > +     .reg_names = imx8mq_g1_reg_names,
> > > > > > +     .num_regs = ARRAY_SIZE(imx8mq_g1_reg_names),
> > > > > >  };
> > > > > >
> > > > > >  const struct hantro_variant imx8mq_vpu_g2_variant = {
> > > > > > @@ -314,10 +227,10 @@ const struct hantro_variant imx8mq_vpu_g2_variant = {
> > > > > >       .postproc_ops = &hantro_g2_postproc_ops,
> > > > > >       .codec = HANTRO_HEVC_DECODER | HANTRO_VP9_DECODER,
> > > > > >       .codec_ops = imx8mq_vpu_g2_codec_ops,
> > > > > > -     .init = imx8mq_vpu_hw_init,
> > > > > > -     .runtime_resume = imx8mq_runtime_resume,
> > > > > >       .irqs = imx8mq_g2_irqs,
> > > > > >       .num_irqs = ARRAY_SIZE(imx8mq_g2_irqs),
> > > > > > -     .clk_names = imx8mq_clk_names,
> > > > > > -     .num_clocks = ARRAY_SIZE(imx8mq_clk_names),
> > > > > > +     .clk_names = imx8mq_g2_clk_names,
> > > > > > +     .num_clocks = ARRAY_SIZE(imx8mq_g2_clk_names),
> > > > > > +     .reg_names = imx8mq_g2_reg_names,
> > > > > > +     .num_regs = ARRAY_SIZE(imx8mq_g2_reg_names),
> > > > > >  };
> > > > > > --
> > > > > > 2.32.0
> > > > > >
> > >
> > >
>
>

Powered by blists - more mailing lists