Re: [Freedreno] [PATCH] drm/msm: gpu: Enable zap shader for A5XX
On Wed, May 10, 2017 at 7:28 PM, Bjorn Andersson
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
>> ---
>> 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
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> #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);
>> + i
Re: [Freedreno] [PATCH] drm/msm: gpu: Enable zap shader for A5XX
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
> ---
> 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
> +#include
> +#include
> +#include
> +#include
> +#include
> #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;
> +
> +
Re: [Freedreno] [PATCH] drm/msm: gpu: Enable zap shader for A5XX
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 infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
