Il giorno lun 23 set 2019 alle ore 18:37 Rob Clark
<robdclark@xxxxxxxxx> ha scritto:
>
> On Sat, Sep 21, 2019 at 3:04 AM <kholk11@xxxxxxxxx> wrote:
> >
> > From: "Angelo G. Del Regno" <kholk11@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
>
> [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...
