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

[Rob Clark <robdclark@gmail.com>]

On Tue, Dec 6, 2016 at 10:35 AM, Jordan Crouse <jcrouse@codeaurora.org> wrote:
> 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 <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?
>
> 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.

also for recovery when I crash the gpu ;-)

BR,
-R