[PATCH 0/6] drm: msm: A5XX zap shader

[PATCH 0/6] drm: msm: A5XX zap shader

[Jordan Crouse]

Hey Rob -

Here is the stack of stuff to add the zap shader for msm-next. I left
the code broken out as the first two changes are merged into the device
specific tree and the third one has been seen before so this should
hopefully cut down on confusion.

The following patches are basically the changes that Bjorn requested and a bit
more clean up to rely on the device tree less as is our current plan of action.

I am not at all oppposed to squashing these into one big change or two moderate
changes if it makes life easier.

Jordan Crouse (6):
  drm/msm: Add a quick and dirty PIL loader
  drm/msm: gpu: Use the zap shader on 5XX if we can
  drm/msm: Improve the zap shader
  drm/msm: Create a child device for the zap shader
  drm/msm: Move zap shader firmware name to the device table
  drm/msm: Document the zap-shader subnode for the GPU

 .../devicetree/bindings/display/msm/gpu.txt        |  13 ++
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c              | 249 ++++++++++++++++++++-
 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 +
 5 files changed, 264 insertions(+), 2 deletions(-)

-- 
1.9.1

[PATCH 1/6] drm/msm: Add a quick and dirty PIL loader

[Jordan Crouse]

In order to switch the GPU out of secure mode on most systems we
need to load a zap shader into memory and get it authenticated
and into the secure world.  All the bits and pieces to do
the load are scattered throughout the kernel, but we need to
bring everything together.

Add a semi-custom loader that will read a MDT file and get
it loaded and authenticated through SCM.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 166 ++++++++++++++++++++++++++++++++++
 1 file changed, 166 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index 31a9bce..6c55d24 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/elf.h>
+#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 "msm_gem.h"
 #include "msm_mmu.h"
 #include "a5xx_gpu.h"
@@ -18,6 +24,166 @@
 extern bool hang_debug;
 static void a5xx_dump(struct msm_gpu *gpu);
 
+static inline bool _check_segment(const struct elf32_phdr *phdr)
+{
+	return ((phdr->p_type == PT_LOAD) &&
+		((phdr->p_flags & (7 << 24)) != (2 << 24)) &&
+		phdr->p_memsz);
+}
+
+static int __pil_tz_load_image(struct platform_device *pdev,
+		const struct firmware *mdt, const char *fwname,
+		void *fwptr, size_t fw_size, unsigned long fw_min_addr)
+{
+	char str[64] = { 0 };
+	const struct elf32_hdr *ehdr = (struct elf32_hdr *) mdt->data;
+	const struct elf32_phdr *phdrs = (struct elf32_phdr *) (ehdr + 1);
+	const struct firmware *fw;
+	int i, ret = 0;
+
+	for (i = 0; i < ehdr->e_phnum; i++) {
+		const struct elf32_phdr *phdr = &phdrs[i];
+		size_t offset;
+
+		/* Make sure the segment is loadable */
+		if (!_check_segment(phdr))
+			continue;
+
+		/* Get the offset of the segment within the region */
+		offset = (phdr->p_paddr - fw_min_addr);
+
+		/* Request the file containing the segment */
+		snprintf(str, sizeof(str) - 1, "%s.b%02d", fwname, i);
+
+		ret = request_firmware(&fw, str, &pdev->dev);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to load segment %s\n", str);
+			break;
+		}
+
+		if (offset + fw->size > fw_size) {
+			dev_err(&pdev->dev, "Segment %s is too big\n", str);
+			ret = -EINVAL;
+			release_firmware(fw);
+			break;
+		}
+
+		/* Copy the segment into place */
+		memcpy(fwptr + offset, fw->data, fw->size);
+		release_firmware(fw);
+	}
+
+	return ret;
+}
+
+static int _pil_tz_load_image(struct platform_device *pdev)
+{
+	char str[64] = { 0 };
+	const char *fwname;
+	const struct elf32_hdr *ehdr;
+	const struct elf32_phdr *phdrs;
+	const struct firmware *mdt;
+	phys_addr_t fw_min_addr, fw_max_addr;
+	dma_addr_t fw_phys;
+	size_t fw_size;
+	u32 pas_id;
+	void *ptr;
+	int i, ret;
+
+	if (pdev == NULL)
+		return -ENODEV;
+
+	if (!qcom_scm_is_available()) {
+		dev_err(&pdev->dev, "SCM is not available\n");
+		return -EINVAL;
+	}
+
+	ret = of_reserved_mem_device_init(&pdev->dev);
+
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to set up the reserved memory\n");
+		return ret;
+	}
+
+	/* Get the firmware and PAS id from the device node */
+	if (of_property_read_string(pdev->dev.of_node, "qcom,firmware",
+		&fwname)) {
+		dev_err(&pdev->dev, "Could not read a firmware name\n");
+		return -EINVAL;
+	}
+
+	if (of_property_read_u32(pdev->dev.of_node, "qcom,pas-id", &pas_id)) {
+		dev_err(&pdev->dev, "Could not read the pas ID\n");
+		return -EINVAL;
+	}
+
+	snprintf(str, sizeof(str) - 1, "%s.mdt", fwname);
+
+	/* Request the MDT file for the firmware */
+	ret = request_firmware(&mdt, str, &pdev->dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to load %s\n", str);
+		return ret;
+	}
+
+	ehdr = (struct elf32_hdr *) mdt->data;
+	phdrs = (struct elf32_phdr *) (ehdr + 1);
+
+	/* Get the extents of the firmware image */
+
+	fw_min_addr = (phys_addr_t) ULLONG_MAX;
+	fw_max_addr = 0;
+
+	for (i = 0; i < ehdr->e_phnum; i++) {
+		const struct elf32_phdr *phdr = &phdrs[i];
+
+		if (!_check_segment(phdr))
+			continue;
+
+		fw_min_addr = min_t(phys_addr_t, fw_min_addr, phdr->p_paddr);
+		fw_max_addr = max_t(phys_addr_t, fw_max_addr,
+			PAGE_ALIGN(phdr->p_paddr + phdr->p_memsz));
+	}
+
+	if (fw_min_addr == (phys_addr_t) ULLONG_MAX && fw_max_addr == 0)
+		goto out;
+
+	fw_size = (size_t) (fw_max_addr - fw_min_addr);
+
+	/* Verify the MDT header */
+	ret = qcom_scm_pas_init_image(pas_id, mdt->data, mdt->size);
+	if (ret) {
+		dev_err(&pdev->dev, "Invalid firmware metadata\n");
+		goto out;
+	}
+
+	/* allocate some memory */
+	ptr = dma_alloc_coherent(&pdev->dev, fw_size, &fw_phys, GFP_KERNEL);
+	if (ptr == NULL)
+		goto out;
+
+	/* Set up the newly allocated memory region */
+	ret = qcom_scm_pas_mem_setup(pas_id, fw_phys, fw_size);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to set up firmware memory\n");
+		goto out;
+	}
+
+	ret = __pil_tz_load_image(pdev, mdt, fwname, ptr, fw_size, fw_min_addr);
+	if (!ret) {
+		ret = qcom_scm_pas_auth_and_reset(pas_id);
+		if (ret)
+			dev_err(&pdev->dev, "Unable to authorize the image\n");
+	}
+
+out:
+	if (ret && ptr)
+		dma_free_coherent(&pdev->dev, fw_size, ptr, fw_phys);
+
+	release_firmware(mdt);
+	return ret;
+}
+
 static void a5xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit,
 	struct msm_file_private *ctx)
 {
-- 
1.9.1

Re: [PATCH 1/6] drm/msm: Add a quick and dirty PIL loader

[Bjorn Andersson]

On Wed 12 Apr 14:15 PDT 2017, Jordan Crouse wrote:
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
[..]
> +static int _pil_tz_load_image(struct platform_device *pdev)
> +{

You should be able to replace this with a call into
drivers/soc/qcom/mdt_loader.c (if not please let me know so we can make
that work for you).

> +	char str[64] = { 0 };
> +	const char *fwname;
> +	const struct elf32_hdr *ehdr;
> +	const struct elf32_phdr *phdrs;
> +	const struct firmware *mdt;
> +	phys_addr_t fw_min_addr, fw_max_addr;
> +	dma_addr_t fw_phys;
> +	size_t fw_size;
> +	u32 pas_id;
> +	void *ptr;
> +	int i, ret;
> +
> +	if (pdev == NULL)
> +		return -ENODEV;
> +
> +	if (!qcom_scm_is_available()) {
> +		dev_err(&pdev->dev, "SCM is not available\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = of_reserved_mem_device_init(&pdev->dev);

You're supposed to do this once per device, so although I believe you
only call this function as an extension of the init function it would be
better for maintainability if you move this call to where you create the
device.

> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "Unable to set up the reserved memory\n");
> +		return ret;
> +	}
> +
> +	/* Get the firmware and PAS id from the device node */
> +	if (of_property_read_string(pdev->dev.of_node, "qcom,firmware",
> +		&fwname)) {
> +		dev_err(&pdev->dev, "Could not read a firmware name\n");
> +		return -EINVAL;
> +	}
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "qcom,pas-id", &pas_id)) {
> +		dev_err(&pdev->dev, "Could not read the pas ID\n");
> +		return -EINVAL;
> +	}
> +

As noted in the later patches, there's no point in introducing this code
(that's incompatible with the DT binding) just to remove it again.
Squash in the other code-patches (DT binding should be separate) and
this would be cleaner.

> +	snprintf(str, sizeof(str) - 1, "%s.mdt", fwname);

N.B. snprintf() takes the size of the buffer as second argument and will
make sure there's a NUL at the end, so no -1.

Regards,
Bjorn
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 3/6] drm/msm: Improve the zap shader

[Jordan Crouse]

Simply the code use snprintf correctly and make sure that we memset
the rest of the segment if the memory size in the ELF file is larger
than the file size.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 70 +++++++++++++++++------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index 0e2b00a..0a096f8 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -31,11 +31,11 @@ static inline bool _check_segment(const struct elf32_phdr *phdr)
 		phdr->p_memsz);
 }
 
-static int __pil_tz_load_image(struct platform_device *pdev,
+static int zap_load_segments(struct platform_device *pdev,
 		const struct firmware *mdt, const char *fwname,
 		void *fwptr, size_t fw_size, unsigned long fw_min_addr)
 {
-	char str[64] = { 0 };
+	char filename[64];
 	const struct elf32_hdr *ehdr = (struct elf32_hdr *) mdt->data;
 	const struct elf32_phdr *phdrs = (struct elf32_phdr *) (ehdr + 1);
 	const struct firmware *fw;
@@ -53,16 +53,18 @@ static int __pil_tz_load_image(struct platform_device *pdev,
 		offset = (phdr->p_paddr - fw_min_addr);
 
 		/* Request the file containing the segment */
-		snprintf(str, sizeof(str) - 1, "%s.b%02d", fwname, i);
+		snprintf(filename, sizeof(filename), "%s.b%02d", fwname, i);
 
-		ret = request_firmware(&fw, str, &pdev->dev);
+		ret = request_firmware(&fw, filename, &pdev->dev);
 		if (ret) {
-			dev_err(&pdev->dev, "Failed to load segment %s\n", str);
+			DRM_DEV_ERROR(&pdev->dev, "Failed to load segment %s\n",
+				filename);
 			break;
 		}
 
 		if (offset + fw->size > fw_size) {
-			dev_err(&pdev->dev, "Segment %s is too big\n", str);
+			DRM_DEV_ERROR(&pdev->dev, "Segment %s is too big\n",
+				filename);
 			ret = -EINVAL;
 			release_firmware(fw);
 			break;
@@ -70,15 +72,19 @@ static int __pil_tz_load_image(struct platform_device *pdev,
 
 		/* Copy the segment into place */
 		memcpy(fwptr + offset, fw->data, fw->size);
+
+		if (phdr->p_memsz > phdr->p_filesz)
+			memset(fwptr + fw->size, 0,
+				phdr->p_memsz - phdr->p_filesz);
 		release_firmware(fw);
 	}
 
 	return ret;
 }
 
-static int _pil_tz_load_image(struct platform_device *pdev)
+static int zap_load_mdt(struct platform_device *pdev)
 {
-	char str[64] = { 0 };
+	char filename[64];
 	const char *fwname;
 	const struct elf32_hdr *ehdr;
 	const struct elf32_phdr *phdrs;
@@ -86,7 +92,6 @@ static int _pil_tz_load_image(struct platform_device *pdev)
 	phys_addr_t fw_min_addr, fw_max_addr;
 	dma_addr_t fw_phys;
 	size_t fw_size;
-	u32 pas_id;
 	void *ptr;
 	int i, ret;
 
@@ -94,35 +99,29 @@ static int _pil_tz_load_image(struct platform_device *pdev)
 		return -ENODEV;
 
 	if (!qcom_scm_is_available()) {
-		dev_err(&pdev->dev, "SCM is not available\n");
-		return -EINVAL;
+		DRM_DEV_ERROR(&pdev->dev, "SCM is not available\n");
+		return -EPROBE_DEFER;
 	}
 
 	ret = of_reserved_mem_device_init(&pdev->dev);
-
 	if (ret) {
-		dev_err(&pdev->dev, "Unable to set up the reserved memory\n");
+		DRM_DEV_ERROR(&pdev->dev, "Unable to set up the reserved memory\n");
 		return ret;
 	}
 
 	/* Get the firmware and PAS id from the device node */
 	if (of_property_read_string(pdev->dev.of_node, "qcom,firmware",
 		&fwname)) {
-		dev_err(&pdev->dev, "Could not read a firmware name\n");
-		return -EINVAL;
-	}
-
-	if (of_property_read_u32(pdev->dev.of_node, "qcom,pas-id", &pas_id)) {
-		dev_err(&pdev->dev, "Could not read the pas ID\n");
+		DRM_DEV_ERROR(&pdev->dev, "Could not read a firmware name\n");
 		return -EINVAL;
 	}
 
-	snprintf(str, sizeof(str) - 1, "%s.mdt", fwname);
+	snprintf(filename, sizeof(filename), "%s.mdt", fwname);
 
 	/* Request the MDT file for the firmware */
-	ret = request_firmware(&mdt, str, &pdev->dev);
+	ret = request_firmware(&mdt, filename, &pdev->dev);
 	if (ret) {
-		dev_err(&pdev->dev, "Unable to load %s\n", str);
+		DRM_DEV_ERROR(&pdev->dev, "Unable to load %s\n", filename);
 		return ret;
 	}
 
@@ -151,9 +150,9 @@ static int _pil_tz_load_image(struct platform_device *pdev)
 	fw_size = (size_t) (fw_max_addr - fw_min_addr);
 
 	/* Verify the MDT header */
-	ret = qcom_scm_pas_init_image(pas_id, mdt->data, mdt->size);
+	ret = qcom_scm_pas_init_image(13, mdt->data, mdt->size);
 	if (ret) {
-		dev_err(&pdev->dev, "Invalid firmware metadata\n");
+		DRM_DEV_ERROR(&pdev->dev, "Invalid firmware metadata\n");
 		goto out;
 	}
 
@@ -163,18 +162,19 @@ static int _pil_tz_load_image(struct platform_device *pdev)
 		goto out;
 
 	/* Set up the newly allocated memory region */
-	ret = qcom_scm_pas_mem_setup(pas_id, fw_phys, fw_size);
+	ret = qcom_scm_pas_mem_setup(13, fw_phys, fw_size);
 	if (ret) {
-		dev_err(&pdev->dev, "Unable to set up firmware memory\n");
+		DRM_DEV_ERROR(&pdev->dev, "Unable to set up firmware memory\n");
 		goto out;
 	}
 
-	ret = __pil_tz_load_image(pdev, mdt, fwname, ptr, fw_size, fw_min_addr);
-	if (!ret) {
-		ret = qcom_scm_pas_auth_and_reset(pas_id);
-		if (ret)
-			dev_err(&pdev->dev, "Unable to authorize the image\n");
-	}
+	ret = zap_load_segments(pdev, mdt, fwname, ptr, fw_size, fw_min_addr);
+	if (ret)
+		goto out;
+
+	ret = qcom_scm_pas_auth_and_reset(13);
+	if (ret)
+		DRM_DEV_ERROR(&pdev->dev, "Unable to authorize the image\n");
 
 out:
 	if (ret && ptr)
@@ -502,14 +502,14 @@ static int a5xx_zap_shader_init(struct msm_gpu *gpu)
 	of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
 
 	/* Find the sub-node for the zap shader */
-	node = of_find_node_by_name(pdev->dev.of_node, "qcom,zap-shader");
+	node = of_get_child_by_name(pdev->dev.of_node, "zap-shader");
 	if (!node) {
-		DRM_ERROR("%s: qcom,zap-shader not found in device tree\n",
+		DRM_ERROR("%s: zap-shader not found in device tree\n",
 			gpu->name);
 		return -ENODEV;
 	}
 
-	ret = _pil_tz_load_image(of_find_device_by_node(node));
+	ret = zap_load_mdt(of_find_device_by_node(node));
 	if (ret)
 		DRM_ERROR("%s: Unable to load the zap shader\n",
 			gpu->name);
-- 
1.9.1

Re: [PATCH 3/6] drm/msm: Improve the zap shader

[Bjorn Andersson]

On Wed 12 Apr 14:15 PDT 2017, Jordan Crouse wrote:

> Simply the code use snprintf correctly and make sure that we memset
> the rest of the segment if the memory size in the ELF file is larger
> than the file size.
> 
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>

Please squash this with the prior commits.

> ---
>  drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 70 +++++++++++++++++------------------
>  1 file changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
[..]
> @@ -151,9 +150,9 @@ static int _pil_tz_load_image(struct platform_device *pdev)
>  	fw_size = (size_t) (fw_max_addr - fw_min_addr);
>  
>  	/* Verify the MDT header */
> -	ret = qcom_scm_pas_init_image(pas_id, mdt->data, mdt->size);
> +	ret = qcom_scm_pas_init_image(13, mdt->data, mdt->size);

Please turn 13 into a define.

>  	if (ret) {
> -		dev_err(&pdev->dev, "Invalid firmware metadata\n");
> +		DRM_DEV_ERROR(&pdev->dev, "Invalid firmware metadata\n");
>  		goto out;
>  	}
>  

Regards,
Bjorn

[PATCH 4/6] drm/msm: Create a child device for the zap shader

[Jordan Crouse]

Currently we abuse the platform device engine to create a platform
device for the zap shader subnode so that we can isolate the
reserved memory away from the parent GPU device.

It is much safer to create and register a simple child device and use
that instead.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 77 ++++++++++++++++++++---------------
 drivers/gpu/drm/msm/adreno/a5xx_gpu.h |  2 +
 2 files changed, 46 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index 0a096f8..8678bce 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -31,7 +31,7 @@ static inline bool _check_segment(const struct elf32_phdr *phdr)
 		phdr->p_memsz);
 }
 
-static int zap_load_segments(struct platform_device *pdev,
+static int zap_load_segments(struct device *dev,
 		const struct firmware *mdt, const char *fwname,
 		void *fwptr, size_t fw_size, unsigned long fw_min_addr)
 {
@@ -55,15 +55,15 @@ static int zap_load_segments(struct platform_device *pdev,
 		/* Request the file containing the segment */
 		snprintf(filename, sizeof(filename), "%s.b%02d", fwname, i);
 
-		ret = request_firmware(&fw, filename, &pdev->dev);
+		ret = request_firmware(&fw, filename, dev);
 		if (ret) {
-			DRM_DEV_ERROR(&pdev->dev, "Failed to load segment %s\n",
+			DRM_DEV_ERROR(dev, "Failed to load segment %s\n",
 				filename);
 			break;
 		}
 
 		if (offset + fw->size > fw_size) {
-			DRM_DEV_ERROR(&pdev->dev, "Segment %s is too big\n",
+			DRM_DEV_ERROR(dev, "Segment %s is too big\n",
 				filename);
 			ret = -EINVAL;
 			release_firmware(fw);
@@ -82,7 +82,7 @@ static int zap_load_segments(struct platform_device *pdev,
 	return ret;
 }
 
-static int zap_load_mdt(struct platform_device *pdev)
+static int zap_load_mdt(struct device *dev)
 {
 	char filename[64];
 	const char *fwname;
@@ -92,37 +92,29 @@ static int zap_load_mdt(struct platform_device *pdev)
 	phys_addr_t fw_min_addr, fw_max_addr;
 	dma_addr_t fw_phys;
 	size_t fw_size;
-	void *ptr;
+	void *ptr = NULL;
 	int i, ret;
 
-	if (pdev == NULL)
-		return -ENODEV;
-
-	if (!qcom_scm_is_available()) {
-		DRM_DEV_ERROR(&pdev->dev, "SCM is not available\n");
-		return -EPROBE_DEFER;
-	}
-
-	ret = of_reserved_mem_device_init(&pdev->dev);
+	ret = of_reserved_mem_device_init(dev);
 	if (ret) {
-		DRM_DEV_ERROR(&pdev->dev, "Unable to set up the reserved memory\n");
+		DRM_DEV_ERROR(dev, "Unable to set up the reserved memory\n");
 		return ret;
 	}
 
 	/* Get the firmware and PAS id from the device node */
-	if (of_property_read_string(pdev->dev.of_node, "qcom,firmware",
+	if (of_property_read_string(dev->of_node, "qcom,firmware",
 		&fwname)) {
-		DRM_DEV_ERROR(&pdev->dev, "Could not read a firmware name\n");
+		DRM_DEV_ERROR(dev, "Could not read a firmware name\n");
 		return -EINVAL;
 	}
 
 	snprintf(filename, sizeof(filename), "%s.mdt", fwname);
 
 	/* Request the MDT file for the firmware */
-	ret = request_firmware(&mdt, filename, &pdev->dev);
+	ret = request_firmware(&mdt, filename, dev);
 	if (ret) {
-		DRM_DEV_ERROR(&pdev->dev, "Unable to load %s\n", filename);
-		return ret;
+		DRM_DEV_ERROR(dev, "Unable to load %s\n", filename);
+		goto out;
 	}
 
 	ehdr = (struct elf32_hdr *) mdt->data;
@@ -152,35 +144,36 @@ static int zap_load_mdt(struct platform_device *pdev)
 	/* Verify the MDT header */
 	ret = qcom_scm_pas_init_image(13, mdt->data, mdt->size);
 	if (ret) {
-		DRM_DEV_ERROR(&pdev->dev, "Invalid firmware metadata\n");
+		DRM_DEV_ERROR(dev, "Invalid firmware metadata\n");
 		goto out;
 	}
 
 	/* allocate some memory */
-	ptr = dma_alloc_coherent(&pdev->dev, fw_size, &fw_phys, GFP_KERNEL);
+	ptr = dma_alloc_coherent(dev, fw_size, &fw_phys, GFP_KERNEL);
 	if (ptr == NULL)
 		goto out;
 
 	/* Set up the newly allocated memory region */
 	ret = qcom_scm_pas_mem_setup(13, fw_phys, fw_size);
 	if (ret) {
-		DRM_DEV_ERROR(&pdev->dev, "Unable to set up firmware memory\n");
+		DRM_DEV_ERROR(dev, "Unable to set up firmware memory\n");
 		goto out;
 	}
 
-	ret = zap_load_segments(pdev, mdt, fwname, ptr, fw_size, fw_min_addr);
+	ret = zap_load_segments(dev, mdt, fwname, ptr, fw_size, fw_min_addr);
 	if (ret)
 		goto out;
 
 	ret = qcom_scm_pas_auth_and_reset(13);
 	if (ret)
-		DRM_DEV_ERROR(&pdev->dev, "Unable to authorize the image\n");
+		DRM_DEV_ERROR(dev, "Unable to authorize the image\n");
 
 out:
 	if (ret && ptr)
-		dma_free_coherent(&pdev->dev, fw_size, ptr, fw_phys);
+		dma_free_coherent(dev, fw_size, ptr, fw_phys);
 
 	release_firmware(mdt);
+
 	return ret;
 }
 
@@ -498,8 +491,10 @@ static int a5xx_zap_shader_init(struct msm_gpu *gpu)
 	if (loaded)
 		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);
+	if (!qcom_scm_is_available()) {
+		DRM_DEV_ERROR(&pdev->dev, "SCM is not available\n");
+		return -EPROBE_DEFER;
+	}
 
 	/* Find the sub-node for the zap shader */
 	node = of_get_child_by_name(pdev->dev.of_node, "zap-shader");
@@ -509,10 +504,23 @@ static int a5xx_zap_shader_init(struct msm_gpu *gpu)
 		return -ENODEV;
 	}
 
-	ret = zap_load_mdt(of_find_device_by_node(node));
-	if (ret)
-		DRM_ERROR("%s: Unable to load the zap shader\n",
-			gpu->name);
+	/* Set up the child device to "own" the zap shader */
+	if (!a5xx_gpu->zap_dev.parent) {
+		a5xx_gpu->zap_dev.parent = &pdev->dev;
+		a5xx_gpu->zap_dev.of_node = node;
+		dev_set_name(&a5xx_gpu->zap_dev, "adreno_zap_shader");
+
+		ret = device_register(&a5xx_gpu->zap_dev);
+		if (ret) {
+			DRM_DEV_ERROR(&pdev->dev,
+				"Couldn't register the zap shader device\n");
+
+			a5xx_gpu->zap_dev.parent = NULL;
+			return ret;
+		}
+	}
+
+	ret = zap_load_mdt(&a5xx_gpu->zap_dev);
 
 	loaded = !ret;
 
@@ -755,6 +763,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)
-- 
1.9.1

[PATCH 5/6] drm/msm: Move zap shader firmware name to the device table

[Jordan Crouse]

The zap shader firmware name is not platform specific. Move it
to the device table instead.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c      | 24 ++++++++++++------------
 drivers/gpu/drm/msm/adreno/adreno_device.c |  1 +
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index 8678bce..fc9a81a 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -82,10 +82,9 @@ static int zap_load_segments(struct device *dev,
 	return ret;
 }
 
-static int zap_load_mdt(struct device *dev)
+static int zap_load_mdt(struct device *dev, const char *fwname)
 {
 	char filename[64];
-	const char *fwname;
 	const struct elf32_hdr *ehdr;
 	const struct elf32_phdr *phdrs;
 	const struct firmware *mdt;
@@ -101,13 +100,6 @@ static int zap_load_mdt(struct device *dev)
 		return ret;
 	}
 
-	/* Get the firmware and PAS id from the device node */
-	if (of_property_read_string(dev->of_node, "qcom,firmware",
-		&fwname)) {
-		DRM_DEV_ERROR(dev, "Could not read a firmware name\n");
-		return -EINVAL;
-	}
-
 	snprintf(filename, sizeof(filename), "%s.mdt", fwname);
 
 	/* Request the MDT file for the firmware */
@@ -491,16 +483,24 @@ static int a5xx_zap_shader_init(struct msm_gpu *gpu)
 	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;
+	}
+
 	/* Find the sub-node for the zap shader */
 	node = of_get_child_by_name(pdev->dev.of_node, "zap-shader");
 	if (!node) {
-		DRM_ERROR("%s: zap-shader not found in device tree\n",
-			gpu->name);
+		DRM_DEV_ERROR(&pdev->dev,
+			"zap-shader not found in device tree\n");
 		return -ENODEV;
 	}
 
@@ -520,7 +520,7 @@ static int a5xx_zap_shader_init(struct msm_gpu *gpu)
 		}
 	}
 
-	ret = zap_load_mdt(&a5xx_gpu->zap_dev);
+	ret = zap_load_mdt(&a5xx_gpu->zap_dev, adreno_gpu->info->zapfw);
 
 	loaded = !ret;
 
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index c0fa5d1..6118b10 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",
 	},
 };
 
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);
-- 
1.9.1

Re: [PATCH 5/6] drm/msm: Move zap shader firmware name to the device table

[Bjorn Andersson]

On Wed 12 Apr 14:15 PDT 2017, Jordan Crouse wrote:

> The zap shader firmware name is not platform specific. Move it
> to the device table instead.
> 
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>

This looks good, but it should be squashed with the previous commits.

Regards,
Bjorn

> ---
>  drivers/gpu/drm/msm/adreno/a5xx_gpu.c      | 24 ++++++++++++------------
>  drivers/gpu/drm/msm/adreno/adreno_device.c |  1 +
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  1 +
>  3 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index 8678bce..fc9a81a 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -82,10 +82,9 @@ static int zap_load_segments(struct device *dev,
>  	return ret;
>  }
>  
> -static int zap_load_mdt(struct device *dev)
> +static int zap_load_mdt(struct device *dev, const char *fwname)
>  {
>  	char filename[64];
> -	const char *fwname;
>  	const struct elf32_hdr *ehdr;
>  	const struct elf32_phdr *phdrs;
>  	const struct firmware *mdt;
> @@ -101,13 +100,6 @@ static int zap_load_mdt(struct device *dev)
>  		return ret;
>  	}
>  
> -	/* Get the firmware and PAS id from the device node */
> -	if (of_property_read_string(dev->of_node, "qcom,firmware",
> -		&fwname)) {
> -		DRM_DEV_ERROR(dev, "Could not read a firmware name\n");
> -		return -EINVAL;
> -	}
> -
>  	snprintf(filename, sizeof(filename), "%s.mdt", fwname);
>  
>  	/* Request the MDT file for the firmware */
> @@ -491,16 +483,24 @@ static int a5xx_zap_shader_init(struct msm_gpu *gpu)
>  	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;
> +	}
> +
>  	/* Find the sub-node for the zap shader */
>  	node = of_get_child_by_name(pdev->dev.of_node, "zap-shader");
>  	if (!node) {
> -		DRM_ERROR("%s: zap-shader not found in device tree\n",
> -			gpu->name);
> +		DRM_DEV_ERROR(&pdev->dev,
> +			"zap-shader not found in device tree\n");
>  		return -ENODEV;
>  	}
>  
> @@ -520,7 +520,7 @@ static int a5xx_zap_shader_init(struct msm_gpu *gpu)
>  		}
>  	}
>  
> -	ret = zap_load_mdt(&a5xx_gpu->zap_dev);
> +	ret = zap_load_mdt(&a5xx_gpu->zap_dev, adreno_gpu->info->zapfw);
>  
>  	loaded = !ret;
>  
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index c0fa5d1..6118b10 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",
>  	},
>  };
>  
> 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);
> -- 
> 1.9.1
> 

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

[Jordan Crouse]

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 6c55d24..0e2b00a 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -470,6 +470,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_set_remote_state(0, 13);
+	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)
+		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);
+
+	/* Find the sub-node for the zap shader */
+	node = of_find_node_by_name(pdev->dev.of_node, "qcom,zap-shader");
+	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;
+}
+
 #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 | \
@@ -654,8 +703,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;
 }
-- 
1.9.1

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Bjorn Andersson]

On Wed 12 Apr 14:15 PDT 2017, Jordan Crouse wrote:
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index 6c55d24..0e2b00a 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -470,6 +470,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_set_remote_state(0, 13);

Is 13 here the PAS_ID, or just a coincidence? And what is state 0? Can
we have a define for this?

> +	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)
> +		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);

As this is new code, there's little (no) value in doing this stepwise,
just squash this with the later commits replacing this.

> +
> +	/* Find the sub-node for the zap shader */
> +	node = of_find_node_by_name(pdev->dev.of_node, "qcom,zap-shader");
> +	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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Jordan Crouse]

On Mon, Apr 17, 2017 at 12:58:07PM -0700, Bjorn Andersson wrote:
> On Wed 12 Apr 14:15 PDT 2017, Jordan Crouse wrote:
> > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > index 6c55d24..0e2b00a 100644
> > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > @@ -470,6 +470,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_set_remote_state(0, 13);
> 
> Is 13 here the PAS_ID, or just a coincidence? And what is state 0? Can
> we have a define for this?

I think it is officially a coincidence but probably an informed one. Now that I
think about it I seem to remember Andy asking me to add a define for the 13.
The state is more confusing - because video is the other consumer of this call
and they call 0 to disable and 1 to enable, and GPU calls 0 to enable.  I can
add a #define but it will be just for GPU - maybe something like
REMOTE_STATE_GPU_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)
> > +		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);
> 
> As this is new code, there's little (no) value in doing this stepwise,
> just squash this with the later commits replacing this.

Yeah, I'm thinking I'll do a squash on the next go around unless Rob objects.

Jordan
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Bjorn Andersson]

On Mon 17 Apr 14:51 PDT 2017, Jordan Crouse wrote:

> On Mon, Apr 17, 2017 at 12:58:07PM -0700, Bjorn Andersson wrote:
> > On Wed 12 Apr 14:15 PDT 2017, Jordan Crouse wrote:
> > > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > > index 6c55d24..0e2b00a 100644
> > > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > > @@ -470,6 +470,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_set_remote_state(0, 13);
> > 
> > Is 13 here the PAS_ID, or just a coincidence? And what is state 0? Can
> > we have a define for this?
> 
> I think it is officially a coincidence but probably an informed one. Now that I
> think about it I seem to remember Andy asking me to add a define for the 13.

So far I haven't pushed the pas-id defines to the public scm header, but
we could do that and until we see a discrepancy just use that define
here.

> The state is more confusing - because video is the other consumer of this call
> and they call 0 to disable and 1 to enable, and GPU calls 0 to enable.  I can
> add a #define but it will be just for GPU - maybe something like
> REMOTE_STATE_GPU_RESUME?
> 

As the states aren't globally defined I think it makes sense to just
keep them opaque in the scm-world and have the two(?) states defined
locally here in the adreno driver.

> 
> > > +	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)
> > > +		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);
> > 
> > As this is new code, there's little (no) value in doing this stepwise,
> > just squash this with the later commits replacing this.
> 
> Yeah, I'm thinking I'll do a squash on the next go around unless Rob objects.
> 

Sounds good.

Regards,
Bjorn
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 6/6] drm/msm: Document the zap-shader subnode for the GPU

[Jordan Crouse]

The 'zap-shader' subnode is used to define a phandle for the
PIL memory region that is required to load GPU secure firwmare images
(known as the "zap shader").

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
 Documentation/devicetree/bindings/display/msm/gpu.txt | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/gpu.txt b/Documentation/devicetree/bindings/display/msm/gpu.txt
index 43fac0f..0376b20 100644
--- a/Documentation/devicetree/bindings/display/msm/gpu.txt
+++ b/Documentation/devicetree/bindings/display/msm/gpu.txt
@@ -15,6 +15,15 @@ Required properties:
   * "iface"
   * "mem_iface"
 
+Subnodes:
+- zap-shader:	Defines the memory region used by the zap shader
+		to load the firmware via the PIL loader. This is required
+		for adreno-5xx targets and newer unless the secure mode
+		is specifically disabled on a target.
+
+Required properties for zap-shader:
+- memory-region: Phandle to the PIL reserved memory region.
+
 Example:
 
 / {
@@ -34,5 +43,9 @@ Example:
 		    <&mmcc GFX3D_CLK>,
 		    <&mmcc GFX3D_AHB_CLK>,
 		    <&mmcc MMSS_IMEM_AHB_CLK>;
+
+		zap-shader {
+			memory-region = <&peripheral_reserved>;
+		};
 	};
 };
-- 
1.9.1

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 6/6] drm/msm: Document the zap-shader subnode for the GPU

[Bjorn Andersson]

On Wed 12 Apr 14:15 PDT 2017, Jordan Crouse wrote:

> The 'zap-shader' subnode is used to define a phandle for the
> PIL memory region that is required to load GPU secure firwmare images
> (known as the "zap shader").
> 
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  Documentation/devicetree/bindings/display/msm/gpu.txt | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/gpu.txt b/Documentation/devicetree/bindings/display/msm/gpu.txt
> index 43fac0f..0376b20 100644
> --- a/Documentation/devicetree/bindings/display/msm/gpu.txt
> +++ b/Documentation/devicetree/bindings/display/msm/gpu.txt
> @@ -15,6 +15,15 @@ Required properties:
>    * "iface"
>    * "mem_iface"
>  
> +Subnodes:
> +- zap-shader:	Defines the memory region used by the zap shader
> +		to load the firmware via the PIL loader. This is required
> +		for adreno-5xx targets and newer unless the secure mode
> +		is specifically disabled on a target.
> +
> +Required properties for zap-shader:
> +- memory-region: Phandle to the PIL reserved memory region.
> +
>  Example:
>  
>  / {
> @@ -34,5 +43,9 @@ Example:
>  		    <&mmcc GFX3D_CLK>,
>  		    <&mmcc GFX3D_AHB_CLK>,
>  		    <&mmcc MMSS_IMEM_AHB_CLK>;
> +
> +		zap-shader {
> +			memory-region = <&peripheral_reserved>;
> +		};
>  	};
>  };
> -- 
> 1.9.1
> 

Re: [PATCH 0/6] drm: msm: A5XX zap shader

[Bjorn Andersson]

On Wed 12 Apr 14:15 PDT 2017, Jordan Crouse wrote:

> Hey Rob -
> 
> Here is the stack of stuff to add the zap shader for msm-next. I left
> the code broken out as the first two changes are merged into the device
> specific tree and the third one has been seen before so this should
> hopefully cut down on confusion.
> 
> The following patches are basically the changes that Bjorn requested and a bit
> more clean up to rely on the device tree less as is our current plan of action.
> 
> I am not at all oppposed to squashing these into one big change or two moderate
> changes if it makes life easier.
> 

Keeping patches in a series separate makes sense when they show the
individual steps of a transition and thereby acting as documentation for
future readers of the git log.

But there's little value in documenting the steps that development took,
so please squash the code into commits that will look meaningful in the
git log.


(And then the DT binding patch should be kept separate, for process
reasons)

Regards,
Bjorn

> Jordan Crouse (6):
>   drm/msm: Add a quick and dirty PIL loader
>   drm/msm: gpu: Use the zap shader on 5XX if we can
>   drm/msm: Improve the zap shader
>   drm/msm: Create a child device for the zap shader
>   drm/msm: Move zap shader firmware name to the device table
>   drm/msm: Document the zap-shader subnode for the GPU
> 
>  .../devicetree/bindings/display/msm/gpu.txt        |  13 ++
>  drivers/gpu/drm/msm/adreno/a5xx_gpu.c              | 249 ++++++++++++++++++++-
>  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 +
>  5 files changed, 264 insertions(+), 2 deletions(-)
> 
> -- 
> 1.9.1
> 

Re: [PATCH 0/6] drm: msm: A5XX zap shader

[Archit Taneja]

On 04/13/2017 02:45 AM, Jordan Crouse wrote:
> Hey Rob -
>
> Here is the stack of stuff to add the zap shader for msm-next. I left
> the code broken out as the first two changes are merged into the device
> specific tree and the third one has been seen before so this should
> hopefully cut down on confusion.
>
> The following patches are basically the changes that Bjorn requested and a bit
> more clean up to rely on the device tree less as is our current plan of action.
>
> I am not at all oppposed to squashing these into one big change or two moderate
> changes if it makes life easier.

For the series:

Tested-by: Archit Taneja <architt@codeaurora.org>

Thanks,
Archit

>
> Jordan Crouse (6):
>   drm/msm: Add a quick and dirty PIL loader
>   drm/msm: gpu: Use the zap shader on 5XX if we can
>   drm/msm: Improve the zap shader
>   drm/msm: Create a child device for the zap shader
>   drm/msm: Move zap shader firmware name to the device table
>   drm/msm: Document the zap-shader subnode for the GPU
>
>  .../devicetree/bindings/display/msm/gpu.txt        |  13 ++
>  drivers/gpu/drm/msm/adreno/a5xx_gpu.c              | 249 ++++++++++++++++++++-
>  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 +
>  5 files changed, 264 insertions(+), 2 deletions(-)
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project