[PATCH v2 22/22] gpu: host1x: At first try a non-blocking allocation for the gather copy

[Dmitry Osipenko]

On 14.06.2017 10:56, Erik Faye-Lund wrote:
> On Wed, Jun 14, 2017 at 1:16 AM, Dmitry Osipenko <digetx at gmail.com> wrote:
>> The blocking gather copy allocation is a major performance downside of the
>> Host1x firewall, it may take hundreds milliseconds which is unacceptable
>> for the real-time graphics operations. Let's try a non-blocking allocation
>> first as a least invasive solution, it makes opentegra (Xorg driver)
>> performance indistinguishable with/without the firewall.
>>
>> Signed-off-by: Dmitry Osipenko <digetx at gmail.com>
>> ---
>>
>> The other much more invasive solution could be: to have a memory pool
>> reserved for the gather copy and performing the firewall checks (and the
>> gather copying) on the actual job submission to the CDMA, not on the job
>> allocation like it is done now. This solution reduces the overhead of a
>> gather copy allocations.
>>
>>  drivers/gpu/host1x/job.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
>> index fc194c676d91..ed0575f14093 100644
>> --- a/drivers/gpu/host1x/job.c
>> +++ b/drivers/gpu/host1x/job.c
>> @@ -594,12 +594,20 @@ static inline int copy_gathers(struct host1x_job *job, struct device *dev)
>>                 size += g->words * sizeof(u32);
>>         }
>>
>> +       /*
>> +        * Try a non-blocking allocation from a higher priority pools first,
>> +        * as awaiting for the allocation here is a major performance hit.
>> +        */
>>         job->gather_copy_mapped = dma_alloc_wc(dev, size, &job->gather_copy,
>> -                                              GFP_KERNEL);
>> -       if (!job->gather_copy_mapped) {
>> -               job->gather_copy_mapped = NULL;
>> +                                              GFP_NOWAIT);
>> +
>> +       /* the higher priority allocation failed, try the generic-blocking */
>> +       if (!job->gather_copy_mapped)
>> +               job->gather_copy_mapped = dma_alloc_wc(dev, size,
>> +                                                      &job->gather_copy,
>> +                                                      GFP_KERNEL);
>> +       if (!job->gather_copy_mapped)
>>                 return -ENOMEM;
>> -       }
>>
>>         job->gather_copy_size = size;
> 
> Can't we just have a static fixed-size buffer, and validate chunks at
> the time? Or why can't we validate directly on the mapped pointers?
> 
> It feels pretty silly to have to allocate memory in the first place here...
> 

We can't validate the mapped pointers because userspace may write to the buffers
memory at any time. Maybe it should be possible to write-protect cmdbuffers
memory for the time of its submission and execution, but I'm not sure whether
it's worth the effort. The current-proposed solution is efficient and least
invasive.

-- 
Dmitry