Re: [PATCH v4 1/2] misc: fastrpc: support complete DMA pool access to the DSP

[Caleb Connolly]

On 30/05/2023 11:52, Srinivas Kandagatla wrote:
> On 11/05/2023 15:11, Dylan Van Assche wrote:
>> To support FastRPC Context Banks which aren't mapped via the SMMU,
>> make the whole reserved memory region available to the DSP to allow
>> access to coherent buffers.
> 
> Mapping the whole region sounds very inefficient, and also possibly
> making the cma region not usable by others.

Probably I'm missing something obvious here... The downstream driver
maps the whole region, what are the advantages to doing it on a
per-allocation basis?

What are the other users?
> 
>>
> 
> AFAIU SDM845 does not have any context banks for SDSP. All new SoCs
> after 865 have moved to having a context bank.
> 
> For such cases (w/o cb) we can make fastrpc_session_alloc use channel
> context device instead of session ctx device. As this is going to be an
> issue when we try to allocate buffers dynamically for that cb.

Right.. This is what patch 2 does, but presumably the ALLOC_DMA_BUF,
MMAP, etc ioctls won't work with the current iteration?

> 
> In the newer platforms (from 865) there is support for iommu and context
> banks on SDSP, so the existing code flow is identical for both ADSP and
> SDSP.
> 
> 
> We should be careful not to break newer platfroms while adding support
> to this.
> 
> Both myself and Ekansh thought about this and see that the better way to
> add support to this is by
> 
> 1. extend fastrpc_session_alloc() to support zero context banks.
> 
> 2. add flags to mark this and allocate meta data using secure allocation
> when its required based on this flag.
> 
> 3.  buffer allocation can either go with 2 or with a new flag coming
> from userspace.

This sounds pretty good to me!
> 
> 
> 
>> This is performed by assigning the memory to the DSP via a hypervisor
>> call to set the correct permissions for the Virtual Machines on the DSP.
>> This is only necessary when a memory region is provided for SLPI DSPs
>> so guard this with a domain ID check.
>>
>> Signed-off-by: Dylan Van Assche <me@xxxxxxxxxxxxxxxxx>
>> Reviewed-by: Caleb Connolly <caleb.connolly@xxxxxxxxxx>
>> ---
>>   drivers/misc/fastrpc.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index f48466960f1b..1ced553ae959 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -2231,6 +2231,8 @@ static int fastrpc_rpmsg_probe(struct
>> rpmsg_device *rpdev)
>>       int i, err, domain_id = -1, vmcount;
>>       const char *domain;
>>       bool secure_dsp;
>> +    struct device_node *rmem_node;
>> +    struct reserved_mem *rmem;
>>       unsigned int vmids[FASTRPC_MAX_VMIDS];
>>         err = of_property_read_string(rdev->of_node, "label", &domain);
>> @@ -2274,6 +2276,19 @@ static int fastrpc_rpmsg_probe(struct
>> rpmsg_device *rpdev)
>>           }
>>       }
>>   +    rmem_node = of_parse_phandle(rdev->of_node, "memory-region", 0);
>> +    if (domain_id == SDSP_DOMAIN_ID && rmem_node) {
>> +        rmem = of_reserved_mem_lookup(rmem_node);
>> +        if (!rmem) {
>> +            err = -EINVAL;
>> +            goto fdev_error;
>> +        }
>> +
>> +        qcom_scm_assign_mem(rmem->base, rmem->size, &data->perms,
>> +                    data->vmperms, data->vmcount);
> 
> vmperms need to be a bit field.
> 
>> +
>> +    }
>> +
>>       secure_dsp = !(of_property_read_bool(rdev->of_node,
>> "qcom,non-secure-domain"));
>>       data->secure = secure_dsp;
>>   
> 
> --srini

-- 
// Caleb (they/them)