On Wed, May 10, 2017 at 7:28 PM, Bjorn Andersson
<bjorn.anders...@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 <jcro...@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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Reply via email to