Re: [PATCH] drm/msm: gpu: Enable zap shader for A5XX

[Bjorn Andersson <bjorn.andersson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno