Re: [PATCH 12/12] drm/msm: gpu: Use the zap shader on 5XX if we can

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Mon 28 Nov 11:28 PST 2016, 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.
> 
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 72 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index eefe197..a7a58ec 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -469,6 +469,55 @@ static int a5xx_ucode_init(struct msm_gpu *gpu)
>  	return 0;
>  }
>  
> +static int a5xx_zap_shader_resume(struct msm_gpu *gpu)
> +{
> +	int ret;
> +
> +	ret = qcom_scm_gpu_zap_resume();
> +	if (ret)
> +		DRM_ERROR("%s: zap-shader resume failed: %d\n",
> +			gpu->name, ret);
> +
> +	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;
> +	struct device_node *node;
> +	int ret;
> +
> +	/*
> +	 * If the zap shader is already loaded into memory we just need to kick
> +	 * the remote processor to reinitialize it
> +	 */
> +	if (loaded)

Why is this handling needed? Why can init be called multiple times?

> +		return a5xx_zap_shader_resume(gpu);
> +
> +	/* Populate the sub-nodes if they haven't already been done */
> +	of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);

I haven't been able to find the qcom,zap-shader platform driver, but I
presume you have something like:

adreno {
	qcom,zap-shader {
		compatible = "qcom,zap-shader";

		firmware = "zapfw";
		memory-region = <&zap_region>;
	};
};

I presume this is done to not "taint" the adreno device's with the zap
memory region, but I don't think you should (ab)use a platform driver
for this.

You should rather add a struct device zap_dev to your adreno context, do
minimal initialization (name and a parent I think is enough), call
device_register(&zap_dev);, of_reserved_mem_device_init() and then use
that for your dma allocation.

This saves you from creating a platform_driver, instantiating a
platform_device and the worry of the race between the creation of that
device and the of_find_device_by_node() below.

> +
> +	/* Find the sub-node for the zap shader */
> +	node = of_find_node_by_name(pdev->dev.of_node, "qcom,zap-shader");

If you're looking for immediate children use of_get_child_by_name()

And no "qcom," in node names please.

> +	if (!node) {
> +		DRM_ERROR("%s: qcom,zap-shader not found in device tree\n",
> +			gpu->name);
> +		return -ENODEV;
> +	}
> +
> +	ret = _pil_tz_load_image(of_find_device_by_node(node));
> +	if (ret)
> +		DRM_ERROR("%s: Unable to load the zap shader\n",
> +			gpu->name);
> +
> +	loaded = !ret;
> +
> +	return ret;
> +}

Regards,
Bjorn