On Mon, Dec 05, 2016 at 11:57:12AM -0800, Bjorn Andersson wrote:
> 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 <jcro...@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?

This is for resume - if we suspend and resume the device without losing state
the secure zone we can't load it again, so we have to call a different
operation to "resume" it. This will be much more heavily used when we have more
aggressive power management.

> > +           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.

As far as I know, of_platform_populate() just fleshed out the platform devices
for the sub-nodes. We are not creating a new platform driver or doing any sort
of probe code, we're just setting up the useful memory to attach the
memory-region to. As far as I can tell, using of_platform_populate() +
of_find_device_by_node() does a lot of the heavy lifting of what you
describe.

> > +
> > +   /* 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.

Okay, can do.

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

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

Reply via email to