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

[Rob Clark]

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)