Re: [PATCH] drm/msm/a2xx: support loading legacy (iMX) firmware

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 02/01/2023 03:39, Rob Clark wrote:
> On Sun, Jan 1, 2023 at 7:57 AM Dmitry Baryshkov
> <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > Support loading A200 firmware generated from the iMX firmware header
> > files. The firmware lacks protection support, however it allows GPU to
> > function properly while using the firmware files with clear license
> > which allows redistribution.
> >
> > Cc: Jonathan Marek <jonathan@xxxxxxxx>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/msm/adreno/a2xx_gpu.c | 27 +++++++++++++++++++++++----
> >   drivers/gpu/drm/msm/adreno/a2xx_gpu.h |  1 +
> >   2 files changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> > index 6c9a747eb4ad..c67089a7ebc1 100644
> > --- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> > @@ -53,6 +53,8 @@ static void a2xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> >
> >   static bool a2xx_me_init(struct msm_gpu *gpu)
> >   {
> > +       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> > +       struct a2xx_gpu *a2xx_gpu = to_a2xx_gpu(adreno_gpu);
> >          struct msm_ringbuffer *ring = gpu->rb[0];
> >
> >          OUT_PKT3(ring, CP_ME_INIT, 18);
> > @@ -84,15 +86,20 @@ static bool a2xx_me_init(struct msm_gpu *gpu)
> >          /* NQ and External Memory Swap */
> >          OUT_RING(ring, 0x00000000);
> >          /* protected mode error checking (0x1f2 is REG_AXXX_CP_INT_CNTL) */
> > -       OUT_RING(ring, 0x200001f2);
> > +       if (a2xx_gpu->protection_disabled)
> > +               OUT_RING(ring, 0x00000000);
> > +       else
> > +               OUT_RING(ring, 0x200001f2);
> >          /* Disable header dumping and Header dump address */
> >          OUT_RING(ring, 0x00000000);
> >          /* Header dump size */
> >          OUT_RING(ring, 0x00000000);
> >
> > -       /* enable protected mode */
> > -       OUT_PKT3(ring, CP_SET_PROTECTED_MODE, 1);
> > -       OUT_RING(ring, 1);
> > +       if (!a2xx_gpu->protection_disabled) {
> > +               /* enable protected mode */
> > +               OUT_PKT3(ring, CP_SET_PROTECTED_MODE, 1);
> > +               OUT_RING(ring, 1);
> > +       }
> >
> >          adreno_flush(gpu, ring, REG_AXXX_CP_RB_WPTR);
> >          return a2xx_idle(gpu);
> > @@ -101,6 +108,7 @@ static bool a2xx_me_init(struct msm_gpu *gpu)
> >   static int a2xx_hw_init(struct msm_gpu *gpu)
> >   {
> >          struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> > +       struct a2xx_gpu *a2xx_gpu = to_a2xx_gpu(adreno_gpu);
> >          dma_addr_t pt_base, tran_error;
> >          uint32_t *ptr, len;
> >          int i, ret;
> > @@ -221,6 +229,17 @@ static int a2xx_hw_init(struct msm_gpu *gpu)
> >          len = adreno_gpu->fw[ADRENO_FW_PM4]->size / 4;
> >          DBG("loading PM4 ucode version: %x", ptr[1]);
> >
> > +       /*
> > +        * New firmware files seem to have GPU and firmware version in this
> > +        * word (0x20xxxx for A200, 0x220xxx for A220, 0x225xxx for A225).
> > +        * Older firmware files, which lack protection support, have 0 instead.
> > +        */
> > +       if (ptr[1] == 0) {
>
> I don't really have a good enough picture about all the possible fw
> versions floating around out there, esp back to the pre-qc days, to
> know if this is a good enough check.  But I guess we can go with it,
> and in the worst case later add an allowlist table of fw checksums (or
> similar) if this doesn't turn out to be sufficient, so the overall
> approach isn't painting us into a corner.
>
> Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>

For the reference. I have pushed existing redistributable firmware files 
to https://github.com/lumag/yamato-firmware.git (I can move the repo to 
some other location, e.g. to gitlab.fdo.org/msm if that's a better place).

I've also sent a patch to linux-firmware, so at some point we should be 
able to use a200 with the default setup.

> > +               dev_warn(gpu->dev->dev,
> > +                        "Legacy firmware detected, disabling protection support\n");
> > +               a2xx_gpu->protection_disabled = true;
> > +       }
> > +
> >          gpu_write(gpu, REG_AXXX_CP_DEBUG,
> >                          AXXX_CP_DEBUG_MIU_128BIT_WRITE_ENABLE);
> >          gpu_write(gpu, REG_AXXX_CP_ME_RAM_WADDR, 0);
> > diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.h b/drivers/gpu/drm/msm/adreno/a2xx_gpu.h
> > index 02fba2cb8932..161a075f94af 100644
> > --- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.h
> > +++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.h
> > @@ -15,6 +15,7 @@
> >   struct a2xx_gpu {
> >          struct adreno_gpu base;
> >          bool pm_enabled;
> > +       bool protection_disabled;
> >   };
> >   #define to_a2xx_gpu(x) container_of(x, struct a2xx_gpu, base)
> >
> > --
> > 2.39.0

--
With best wishes
Dmitry