| Message ID | 1494275308-24007-1-git-send-email-jcrouse@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jordan,
[auto build test ERROR on robclark/msm-next]
[also build test ERROR on v4.11 next-20170509]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jordan-Crouse/drm-msm-gpu-Enable-zap-shader-for-A5XX/20170509-043519
base: git://people.freedesktop.org/~robclark/linux msm-next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64
All errors (new ones prefixed by >>):
>> ERROR: "qcom_mdt_load" [drivers/gpu/drm/msm/msm.ko] undefined!
>> ERROR: "qcom_mdt_get_size" [drivers/gpu/drm/msm/msm.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Mon 08 May 13:28 PDT 2017, Jordan Crouse wrote: > The A5XX GPU powers on in "secure" mode. In secure mode the GPU can > only render to buffers that are marked as secure and inaccessible > to the kernel and user through a series of hardware protections. In > practice secure mode is used to draw things like a UI on a secure > video frame. > > In order to switch out of secure mode the GPU executes a special > shader that clears out the GMEM and other sensitve registers and > then writes a register. Because the kernel can't be trusted the > shader binary is signed and verified and programmed by the > secure world. To do this we need to read the MDT header and the > segments from the firmware location and put them in memory and > present them for approval. > > For targets without secure support there is an out: if the > secure world doesn't support secure then there are no hardware > protections and we can freely write the SECVID_TRUST register from > the CPU. We don't have 100% confidence that we can query the > secure capabilities at run time but we have enough calls that > need to go right to give us some confidence that we're at least doing > something useful. > > Of course if we guess wrong you trigger a permissions violation > which usually ends up in a system crash but thats a problem > that shows up immediately. Looks good overall, just some minor nits. > > [v2: use child device per Bjorn] > [v3: use generic MDT loader per Bjorn] These lines should go after the --- below > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > --- > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 174 ++++++++++++++++++++++++++++- > drivers/gpu/drm/msm/adreno/a5xx_gpu.h | 2 + > drivers/gpu/drm/msm/adreno/adreno_device.c | 1 + > drivers/gpu/drm/msm/adreno/adreno_gpu.h | 1 + > 4 files changed, 176 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > index 31a9bce..4545064 100644 > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c > @@ -11,6 +11,12 @@ > * > */ > > +#include <linux/types.h> > +#include <linux/cpumask.h> > +#include <linux/qcom_scm.h> > +#include <linux/dma-mapping.h> > +#include <linux/of_reserved_mem.h> > +#include <linux/soc/qcom/mdt_loader.h> > #include "msm_gem.h" > #include "msm_mmu.h" > #include "a5xx_gpu.h" > @@ -18,6 +24,57 @@ > extern bool hang_debug; > static void a5xx_dump(struct msm_gpu *gpu); > > +#define GPU_PAS_ID 13 > + > +static int zap_shader_load_mdt(struct device *dev, const char *fwname) > +{ > + const struct firmware *fw; > + phys_addr_t mem_phys; > + ssize_t mem_size; > + void *mem_region = NULL; > + int ret; > + > + /* Request the MDT file for the firmware */ > + ret = request_firmware(&fw, fwname, dev); > + if (ret) { > + DRM_DEV_ERROR(dev, "Unable to load %s\n", fwname); > + return ret; > + } > + > + /* Figure out how much memory we need */ > + mem_size = qcom_mdt_get_size(fw); > + if (mem_size < 0) { > + ret = mem_size; > + goto out; > + } > + > + /* Allocate memory for the firmware image */ > + mem_region = dma_alloc_coherent(dev, mem_size, &mem_phys, GFP_KERNEL); Do you ever need to issue an qcom_scm_pas_shutdown() on the GPU? If not you can use dmam_alloc_coherent() here and device_unregister() will free this memory for you. > + if (!mem_region) { > + ret = -ENOMEM; > + goto out; > + } > + > + /* Load the rest of the MDT */ > + ret = qcom_mdt_load(dev, fw, fwname, GPU_PAS_ID, mem_region, mem_phys, > + mem_size); > + if (ret) > + goto out; > + > + /* Send the image to the secure world */ > + ret = qcom_scm_pas_auth_and_reset(GPU_PAS_ID); > + if (ret) > + DRM_DEV_ERROR(dev, "Unable to authorize the image\n"); > + > +out: > + if (ret && mem_region) > + dma_free_coherent(dev, mem_size, mem_region, mem_phys); You're leaking mem_region on success. Based on the fact that you force this memory to live in the peripheral_region I suspect you want to make sure that Linux consider it allocated until you remove the device. > + > + release_firmware(fw); > + > + return ret; > +} > + > static void a5xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit, > struct msm_file_private *ctx) > { > @@ -304,6 +361,97 @@ static int a5xx_ucode_init(struct msm_gpu *gpu) > return 0; > } > > +#define SCM_GPU_ZAP_SHADER_RESUME 0 > + > +static int a5xx_zap_shader_resume(struct msm_gpu *gpu) > +{ > + int ret; > + > + ret = qcom_scm_set_remote_state(SCM_GPU_ZAP_SHADER_RESUME, GPU_PAS_ID); > + if (ret) > + DRM_ERROR("%s: zap-shader resume failed: %d\n", > + gpu->name, ret); > + > + return ret; > +} > + > +/* Set up a child device to "own" the zap shader */ > +static int a5xx_zap_shader_dev_init(struct device *parent, struct device *dev) > +{ > + struct device_node *node; > + int ret; > + > + if (dev->parent) > + return 0; > + > + /* Find the sub-node for the zap shader */ > + node = of_get_child_by_name(parent->of_node, "zap-shader"); > + if (!node) { > + DRM_DEV_ERROR(parent, "zap-shader not found in device tree\n"); > + return -ENODEV; > + } > + > + dev->parent = parent; > + dev->of_node = node; > + dev_set_name(dev, "adreno_zap_shader"); > + > + ret = device_register(dev); > + if (ret) { > + DRM_DEV_ERROR(parent, "Couldn't register zap shader device\n"); > + goto out; > + } > + > + ret = of_reserved_mem_device_init(dev); > + if (!ret) > + return 0; Stick with the idiomatic way of handling failures and use two labels below to handle the two failure cases. > + > + DRM_DEV_ERROR(parent, "Unable to set up the reserved memory\n"); > + device_unregister(dev); > + > +out: > + dev->parent = NULL; > + return ret; > +} > + Regards, Bjorn
On Wed, May 10, 2017 at 7:28 PM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Mon 08 May 13:28 PDT 2017, Jordan Crouse wrote: > >> The A5XX GPU powers on in "secure" mode. In secure mode the GPU can >> only render to buffers that are marked as secure and inaccessible >> to the kernel and user through a series of hardware protections. In >> practice secure mode is used to draw things like a UI on a secure >> video frame. >> >> In order to switch out of secure mode the GPU executes a special >> shader that clears out the GMEM and other sensitve registers and >> then writes a register. Because the kernel can't be trusted the >> shader binary is signed and verified and programmed by the >> secure world. To do this we need to read the MDT header and the >> segments from the firmware location and put them in memory and >> present them for approval. >> >> For targets without secure support there is an out: if the >> secure world doesn't support secure then there are no hardware >> protections and we can freely write the SECVID_TRUST register from >> the CPU. We don't have 100% confidence that we can query the >> secure capabilities at run time but we have enough calls that >> need to go right to give us some confidence that we're at least doing >> something useful. >> >> Of course if we guess wrong you trigger a permissions violation >> which usually ends up in a system crash but thats a problem >> that shows up immediately. > > Looks good overall, just some minor nits. > >> >> [v2: use child device per Bjorn] >> [v3: use generic MDT loader per Bjorn] > > These lines should go after the --- below jfyi, drm prefers these above --- so it is retained in the commit msg. (But yes, for most other subsystems your suggestion would be correct) BR, -R > >> >> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> >> --- >> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 174 ++++++++++++++++++++++++++++- >> drivers/gpu/drm/msm/adreno/a5xx_gpu.h | 2 + >> drivers/gpu/drm/msm/adreno/adreno_device.c | 1 + >> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 1 + >> 4 files changed, 176 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c >> index 31a9bce..4545064 100644 >> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c >> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c >> @@ -11,6 +11,12 @@ >> * >> */ >> >> +#include <linux/types.h> >> +#include <linux/cpumask.h> >> +#include <linux/qcom_scm.h> >> +#include <linux/dma-mapping.h> >> +#include <linux/of_reserved_mem.h> >> +#include <linux/soc/qcom/mdt_loader.h> >> #include "msm_gem.h" >> #include "msm_mmu.h" >> #include "a5xx_gpu.h" >> @@ -18,6 +24,57 @@ >> extern bool hang_debug; >> static void a5xx_dump(struct msm_gpu *gpu); >> >> +#define GPU_PAS_ID 13 >> + >> +static int zap_shader_load_mdt(struct device *dev, const char *fwname) >> +{ >> + const struct firmware *fw; >> + phys_addr_t mem_phys; >> + ssize_t mem_size; >> + void *mem_region = NULL; >> + int ret; >> + >> + /* Request the MDT file for the firmware */ >> + ret = request_firmware(&fw, fwname, dev); >> + if (ret) { >> + DRM_DEV_ERROR(dev, "Unable to load %s\n", fwname); >> + return ret; >> + } >> + >> + /* Figure out how much memory we need */ >> + mem_size = qcom_mdt_get_size(fw); >> + if (mem_size < 0) { >> + ret = mem_size; >> + goto out; >> + } >> + >> + /* Allocate memory for the firmware image */ >> + mem_region = dma_alloc_coherent(dev, mem_size, &mem_phys, GFP_KERNEL); > > Do you ever need to issue an qcom_scm_pas_shutdown() on the GPU? If not > you can use dmam_alloc_coherent() here and device_unregister() will free > this memory for you. > >> + if (!mem_region) { >> + ret = -ENOMEM; >> + goto out; >> + } >> + >> + /* Load the rest of the MDT */ >> + ret = qcom_mdt_load(dev, fw, fwname, GPU_PAS_ID, mem_region, mem_phys, >> + mem_size); >> + if (ret) >> + goto out; >> + >> + /* Send the image to the secure world */ >> + ret = qcom_scm_pas_auth_and_reset(GPU_PAS_ID); >> + if (ret) >> + DRM_DEV_ERROR(dev, "Unable to authorize the image\n"); >> + >> +out: >> + if (ret && mem_region) >> + dma_free_coherent(dev, mem_size, mem_region, mem_phys); > > You're leaking mem_region on success. Based on the fact that you force > this memory to live in the peripheral_region I suspect you want to make > sure that Linux consider it allocated until you remove the device. > >> + >> + release_firmware(fw); >> + >> + return ret; >> +} >> + >> static void a5xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit, >> struct msm_file_private *ctx) >> { >> @@ -304,6 +361,97 @@ static int a5xx_ucode_init(struct msm_gpu *gpu) >> return 0; >> } >> >> +#define SCM_GPU_ZAP_SHADER_RESUME 0 >> + >> +static int a5xx_zap_shader_resume(struct msm_gpu *gpu) >> +{ >> + int ret; >> + >> + ret = qcom_scm_set_remote_state(SCM_GPU_ZAP_SHADER_RESUME, GPU_PAS_ID); >> + if (ret) >> + DRM_ERROR("%s: zap-shader resume failed: %d\n", >> + gpu->name, ret); >> + >> + return ret; >> +} >> + >> +/* Set up a child device to "own" the zap shader */ >> +static int a5xx_zap_shader_dev_init(struct device *parent, struct device *dev) >> +{ >> + struct device_node *node; >> + int ret; >> + >> + if (dev->parent) >> + return 0; >> + >> + /* Find the sub-node for the zap shader */ >> + node = of_get_child_by_name(parent->of_node, "zap-shader"); >> + if (!node) { >> + DRM_DEV_ERROR(parent, "zap-shader not found in device tree\n"); >> + return -ENODEV; >> + } >> + >> + dev->parent = parent; >> + dev->of_node = node; >> + dev_set_name(dev, "adreno_zap_shader"); >> + >> + ret = device_register(dev); >> + if (ret) { >> + DRM_DEV_ERROR(parent, "Couldn't register zap shader device\n"); >> + goto out; >> + } >> + >> + ret = of_reserved_mem_device_init(dev); >> + if (!ret) >> + return 0; > > Stick with the idiomatic way of handling failures and use two labels > below to handle the two failure cases. > >> + >> + DRM_DEV_ERROR(parent, "Unable to set up the reserved memory\n"); >> + device_unregister(dev); >> + >> +out: >> + dev->parent = NULL; >> + return ret; >> +} >> + > > Regards, > Bjorn > _______________________________________________ > Freedreno mailing list > Freedreno@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c index 31a9bce..4545064 100644 --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c @@ -11,6 +11,12 @@ * */ +#include <linux/types.h> +#include <linux/cpumask.h> +#include <linux/qcom_scm.h> +#include <linux/dma-mapping.h> +#include <linux/of_reserved_mem.h> +#include <linux/soc/qcom/mdt_loader.h> #include "msm_gem.h" #include "msm_mmu.h" #include "a5xx_gpu.h" @@ -18,6 +24,57 @@ extern bool hang_debug; static void a5xx_dump(struct msm_gpu *gpu); +#define GPU_PAS_ID 13 + +static int zap_shader_load_mdt(struct device *dev, const char *fwname) +{ + const struct firmware *fw; + phys_addr_t mem_phys; + ssize_t mem_size; + void *mem_region = NULL; + int ret; + + /* Request the MDT file for the firmware */ + ret = request_firmware(&fw, fwname, dev); + if (ret) { + DRM_DEV_ERROR(dev, "Unable to load %s\n", fwname); + return ret; + } + + /* Figure out how much memory we need */ + mem_size = qcom_mdt_get_size(fw); + if (mem_size < 0) { + ret = mem_size; + goto out; + } + + /* Allocate memory for the firmware image */ + mem_region = dma_alloc_coherent(dev, mem_size, &mem_phys, GFP_KERNEL); + if (!mem_region) { + ret = -ENOMEM; + goto out; + } + + /* Load the rest of the MDT */ + ret = qcom_mdt_load(dev, fw, fwname, GPU_PAS_ID, mem_region, mem_phys, + mem_size); + if (ret) + goto out; + + /* Send the image to the secure world */ + ret = qcom_scm_pas_auth_and_reset(GPU_PAS_ID); + if (ret) + DRM_DEV_ERROR(dev, "Unable to authorize the image\n"); + +out: + if (ret && mem_region) + dma_free_coherent(dev, mem_size, mem_region, mem_phys); + + release_firmware(fw); + + return ret; +} + static void a5xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit, struct msm_file_private *ctx) { @@ -304,6 +361,97 @@ static int a5xx_ucode_init(struct msm_gpu *gpu) return 0; } +#define SCM_GPU_ZAP_SHADER_RESUME 0 + +static int a5xx_zap_shader_resume(struct msm_gpu *gpu) +{ + int ret; + + ret = qcom_scm_set_remote_state(SCM_GPU_ZAP_SHADER_RESUME, GPU_PAS_ID); + if (ret) + DRM_ERROR("%s: zap-shader resume failed: %d\n", + gpu->name, ret); + + return ret; +} + +/* Set up a child device to "own" the zap shader */ +static int a5xx_zap_shader_dev_init(struct device *parent, struct device *dev) +{ + struct device_node *node; + int ret; + + if (dev->parent) + return 0; + + /* Find the sub-node for the zap shader */ + node = of_get_child_by_name(parent->of_node, "zap-shader"); + if (!node) { + DRM_DEV_ERROR(parent, "zap-shader not found in device tree\n"); + return -ENODEV; + } + + dev->parent = parent; + dev->of_node = node; + dev_set_name(dev, "adreno_zap_shader"); + + ret = device_register(dev); + if (ret) { + DRM_DEV_ERROR(parent, "Couldn't register zap shader device\n"); + goto out; + } + + ret = of_reserved_mem_device_init(dev); + if (!ret) + return 0; + + DRM_DEV_ERROR(parent, "Unable to set up the reserved memory\n"); + device_unregister(dev); + +out: + dev->parent = NULL; + return ret; +} + +static int a5xx_zap_shader_init(struct msm_gpu *gpu) +{ + static bool loaded; + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); + struct a5xx_gpu *a5xx_gpu = to_a5xx_gpu(adreno_gpu); + struct platform_device *pdev = a5xx_gpu->pdev; + int ret; + + /* + * If the zap shader is already loaded into memory we just need to kick + * the remote processor to reinitialize it + */ + if (loaded) + return a5xx_zap_shader_resume(gpu); + + /* We need SCM to be able to load the firmware */ + if (!qcom_scm_is_available()) { + DRM_DEV_ERROR(&pdev->dev, "SCM is not available\n"); + return -EPROBE_DEFER; + } + + /* Each GPU has a target specific zap shader firmware name to use */ + if (!adreno_gpu->info->zapfw) { + DRM_DEV_ERROR(&pdev->dev, + "Zap shader firmware file not specified for this target\n"); + return -ENODEV; + } + + ret = a5xx_zap_shader_dev_init(&pdev->dev, &a5xx_gpu->zap_dev); + + if (!ret) + ret = zap_shader_load_mdt(&a5xx_gpu->zap_dev, + adreno_gpu->info->zapfw); + + loaded = !ret; + + return ret; +} + #define A5XX_INT_MASK (A5XX_RBBM_INT_0_MASK_RBBM_AHB_ERROR | \ A5XX_RBBM_INT_0_MASK_RBBM_TRANSFER_TIMEOUT | \ A5XX_RBBM_INT_0_MASK_RBBM_ME_MS_TIMEOUT | \ @@ -488,8 +636,27 @@ static int a5xx_hw_init(struct msm_gpu *gpu) return -EINVAL; } - /* Put the GPU into unsecure mode */ - gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0); + /* + * 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. + */ + ret = a5xx_zap_shader_init(gpu); + if (!ret) { + OUT_PKT7(gpu->rb, CP_SET_SECURE_MODE, 1); + OUT_RING(gpu->rb, 0x00000000); + + gpu->funcs->flush(gpu); + if (!gpu->funcs->idle(gpu)) + return -EINVAL; + } else { + /* Print a warning so if we die, we know why */ + dev_warn_once(gpu->dev->dev, + "Zap shader not enabled - using SECVID_TRUST_CNTL instead\n"); + gpu_write(gpu, REG_A5XX_RBBM_SECVID_TRUST_CNTL, 0x0); + } return 0; } @@ -521,6 +688,9 @@ static void a5xx_destroy(struct msm_gpu *gpu) DBG("%s", gpu->name); + if (a5xx_gpu->zap_dev.parent) + device_unregister(&a5xx_gpu->zap_dev); + if (a5xx_gpu->pm4_bo) { if (a5xx_gpu->pm4_iova) msm_gem_put_iova(a5xx_gpu->pm4_bo, gpu->id); diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.h b/drivers/gpu/drm/msm/adreno/a5xx_gpu.h index 1590f84..78408f5 100644 --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.h +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.h @@ -36,6 +36,8 @@ struct a5xx_gpu { uint32_t gpmu_dwords; uint32_t lm_leakage; + + struct device zap_dev; }; #define to_a5xx_gpu(x) container_of(x, struct a5xx_gpu, base) diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c index c0fa5d1..b7bd6d3 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_device.c +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c @@ -86,6 +86,7 @@ ADRENO_QUIRK_FAULT_DETECT_MASK, .init = a5xx_gpu_init, .gpmufw = "a530v3_gpmu.fw2", + .zapfw = "a530_zap.mdt", }, }; diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h index fb4831f..12b1483 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h @@ -77,6 +77,7 @@ struct adreno_info { uint32_t gmem; enum adreno_quirks quirks; struct msm_gpu *(*init)(struct drm_device *dev); + const char *zapfw; }; const struct adreno_info *adreno_info(struct adreno_rev rev);
The A5XX GPU powers on in "secure" mode. In secure mode the GPU can only render to buffers that are marked as secure and inaccessible to the kernel and user through a series of hardware protections. In practice secure mode is used to draw things like a UI on a secure video frame. In order to switch out of secure mode the GPU executes a special shader that clears out the GMEM and other sensitve registers and then writes a register. Because the kernel can't be trusted the shader binary is signed and verified and programmed by the secure world. To do this we need to read the MDT header and the segments from the firmware location and put them in memory and present them for approval. For targets without secure support there is an out: if the secure world doesn't support secure then there are no hardware protections and we can freely write the SECVID_TRUST register from the CPU. We don't have 100% confidence that we can query the secure capabilities at run time but we have enough calls that need to go right to give us some confidence that we're at least doing something useful. Of course if we guess wrong you trigger a permissions violation which usually ends up in a system crash but thats a problem that shows up immediately. [v2: use child device per Bjorn] [v3: use generic MDT loader per Bjorn] Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> --- drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 174 ++++++++++++++++++++++++++++- drivers/gpu/drm/msm/adreno/a5xx_gpu.h | 2 + drivers/gpu/drm/msm/adreno/adreno_device.c | 1 + drivers/gpu/drm/msm/adreno/adreno_gpu.h | 1 + 4 files changed, 176 insertions(+), 2 deletions(-)