[Freedreno] [PATCH 5/5] drm/msm/adreno: Add support for Adreno 510 GPU

[AngeloGioacchino Del Regno]

Il giorno lun 23 set 2019 alle ore 18:37 Rob Clark
<robdclark at gmail.com> ha scritto:
>
> On Sat, Sep 21, 2019 at 3:04 AM <kholk11 at gmail.com> wrote:
> >
> > From: "Angelo G. Del Regno" <kholk11 at gmail.com>
> >
> > The Adreno 510 GPU is a stripped version of the Adreno 5xx,
> > found in low-end SoCs like 8x56 and 8x76, which has 256K of
> > GMEM, with no GPMU nor ZAP.
> > Also, since the Adreno 5xx part of this driver seems to be
> > developed with high-end Adreno GPUs in mind, and since this
> > is a lower end one, add a comment making clear which GPUs
> > which support is not implemented yet is not using the GPMU
> > related hw init code, so that future developers will not go
> > crazy with that.
> >
> > By the way, the lower end Adreno GPUs with no GPMU are:
> > A505/A506/A510 (no ZAP firmware)
> > A508/A509/A512 (with ZAP firmware)
> >
>
> Hi, thanks for the patch.. one comment below about zap firmware...
> which is not completely to do with this patch, but is my thoughts on
> how we should clean up zap handling
>
> > Signed-off-by: Angelo G. Del Regno <kholk11 at gmail.com>
>
> [snip]
>
> > @@ -679,7 +716,8 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
> >
> >         a5xx_preempt_hw_init(gpu);
> >
> > -       a5xx_gpmu_ucode_init(gpu);
> > +       if (!adreno_is_a510(adreno_gpu))
> > +               a5xx_gpmu_ucode_init(gpu);
> >
> >         ret = a5xx_ucode_init(gpu);
> >         if (ret)
> > @@ -712,12 +750,18 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
> >         }
> >
> >         /*
> > -        * Try to load a zap shader into the secure world. If successful
> > +        * If the chip that we are using does support loading one, then
> > +        * try to load a zap shader into the secure world. If successful
> >          * we can use the CP to switch out of secure mode. If not then we
> >          * have no resource but to try to switch ourselves out manually. If we
> >          * guessed wrong then access to the RBBM_SECVID_TRUST_CNTL register will
> >          * be blocked and a permissions violation will soon follow.
> >          */
> > +       if (adreno_is_a510(adreno_gpu)) {
> > +               gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0);
> > +               goto skip_zap;
> > +       }
>
> This is something we need to cleanup on a6xx as well.  But it is
> actually possible to have the same GPU with and without zap.  We have
> this situation today with sdm845, for example.
>
> What I'd like to do is rather than guess whether we can write
> RBBM_SECVID_TRUST_CNTL or not (since that goes spectacularly wrong
> when we guess incorrectly), is choose based on the presence of the
> zap-shader child node in dtb.  (Currently a6xx tries to choose based
> on whether zap firmware is present.. which we need to fix.)
>
> Originally I was thinking we could keep the zap-shader node in the
> SoC's "core" dtsi (ie. msm8996.dtsi, sdm845.dtsi, etc) and using
> /delete-node/ in per-device dts files for devices without zap.. but
> (AFAIU) the zap shader ends up being signed with a vendor key in most
> cases, meaning that to have a "generic" (not device-specific) distro
> image need to have different zap file names/paths for devices from
> different vendors.  Given this, I think it makes more sense to move
> the zap-shader node into a per-device (or at least, per-vendor) dts
> file, ie. something like:
>
>    /* sdm850-lenovo-yoga-c630.dts: */
>   gpu {
>      zap-shader {
>         memory-region = <&gpu_mem>;
>         zap-prefix = "LENOVO";
>     };
>   };
>
> which would trigger the driver to try to load
> /lib/firmware/qcom/LENOVO/a630_zap.mbn
>
> (I'd like to, at least for devices that have ACPI/SMBIOS tables,
> standardize on using the vendor name from SMBIOS tables as this
> prefix.. so we have a way to construct the firmware path if we
> eventually have ACPI boot support on the aarch64 laptops.)
>
> BR,
> -R
>
> > +
> >         ret = a5xx_zap_shader_init(gpu);
> >         if (!ret) {
> >                 OUT_PKT7(gpu->rb[0], CP_SET_SECURE_MODE, 1);
> > @@ -733,6 +777,7 @@ static int a5xx_hw_init(struct msm_gpu *gpu)
> >                 gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0);
> >         }
> >
> > +skip_zap:
> >         /* Last step - yield the ringbuffer */
> >         a5xx_preempt_start(gpu);
> >
> > @@ -1066,6 +1111,7 @@ static void a5xx_dump(struct msm_gpu *gpu)

Thanks to you for the review.
What I've documented there about the A5xx chips having ZAP and the ones not
having it, came out after a research in the downstream KGSL driver, where qcom
does this distinction based on just the Adreno model, which is the main reason
why I did it like that :)))

I am personally not aware of any A5xx chip having this behavior, so that's why I
didn't even try to think different about this patch. It just seemed to
be the right
thing to do...