[PATCH v7 3/4] misc: fastrpc: Cleanup the domain names
Ling Xu
quic_lxu5 at quicinc.com
Thu Jul 17 02:23:48 UTC 2025
在 7/17/2025 3:28 AM, Bjorn Andersson 写道:
> On Mon, Jul 14, 2025 at 11:11:32AM +0530, Ling Xu wrote:
>> Currently the domain ids are added for each instance of domains, this is
>> totally not scalable approach.
>
> This sentence only makes sense for people in your team or participants
> of some recent meeting or (private) mail thread of yours. When providing
> you problem description [1], do so in a way that it makes sense to
> people outside that bubble - and yourself next month.
>
> [1] https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
>
>> Clean this mess and create domain ids for
>> only domains not its instances.
>>
>
> Is the "mess" that the domain is part of the ioctl, or is the mess that
> the names of the domains are defined in an array and you prefer them to
> be listed out in code (fastrpc_get_domain_id())?
I already split this to 2 changes in latest patch, mess means we created domain ids
for its instances.
>
>> Co-developed-by: Srinivas Kandagatla <srinivas.kandagatla at linaro.org>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla at linaro.org>
>> Signed-off-by: Ling Xu <quic_lxu5 at quicinc.com>
>> ---
>> drivers/misc/fastrpc.c | 50 ++++++++++++++++---------------------
>> include/uapi/misc/fastrpc.h | 2 +-
>> 2 files changed, 22 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 378923594f02..85b6eb16b616 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -27,8 +27,6 @@
>> #define MDSP_DOMAIN_ID (1)
>> #define SDSP_DOMAIN_ID (2)
>> #define CDSP_DOMAIN_ID (3)
>> -#define CDSP1_DOMAIN_ID (4)
>> -#define FASTRPC_DEV_MAX 5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>> #define FASTRPC_MAX_SESSIONS 14
>> #define FASTRPC_MAX_VMIDS 16
>> #define FASTRPC_ALIGN 128
>> @@ -106,8 +104,6 @@
>>
>> #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>
>> -static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>> - "sdsp", "cdsp", "cdsp1" };
>> struct fastrpc_phy_page {
>> u64 addr; /* physical address */
>> u64 size; /* size of contiguous region */
>> @@ -1723,7 +1719,6 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
>> uint32_t attribute_id = cap->attribute_id;
>> uint32_t *dsp_attributes;
>> unsigned long flags;
>> - uint32_t domain = cap->domain;
>> int err;
>>
>> spin_lock_irqsave(&cctx->lock, flags);
>> @@ -1741,7 +1736,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
>> err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES);
>> if (err == DSP_UNSUPPORTED_API) {
>> dev_info(&cctx->rpdev->dev,
>> - "Warning: DSP capabilities not supported on domain: %d\n", domain);
>> + "Warning: DSP capabilities not supported\n");
>> kfree(dsp_attributes);
>> return -EOPNOTSUPP;
>> } else if (err) {
>> @@ -1769,17 +1764,6 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
>> return -EFAULT;
>>
>> cap.capability = 0;
>> - if (cap.domain >= FASTRPC_DEV_MAX) {
>> - dev_err(&fl->cctx->rpdev->dev, "Error: Invalid domain id:%d, err:%d\n",
>> - cap.domain, err);
>> - return -ECHRNG;
>> - }
>> -
>> - /* Fastrpc Capablities does not support modem domain */
>> - if (cap.domain == MDSP_DOMAIN_ID) {
>> - dev_err(&fl->cctx->rpdev->dev, "Error: modem not supported %d\n", err);
>> - return -ECHRNG;
>> - }
>>
>> if (cap.attribute_id >= FASTRPC_MAX_DSP_ATTRIBUTES) {
>> dev_err(&fl->cctx->rpdev->dev, "Error: invalid attribute: %d, err: %d\n",
>> @@ -2255,6 +2239,20 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
>> return err;
>> }
>>
>> +static int fastrpc_get_domain_id(const char *domain)
>> +{
>> + if (!strncmp(domain, "adsp", 4))
>> + return ADSP_DOMAIN_ID;
>> + else if (!strncmp(domain, "cdsp", 4))
>> + return CDSP_DOMAIN_ID;
>> + else if (!strncmp(domain, "mdsp", 4))
>> + return MDSP_DOMAIN_ID;
>> + else if (!strncmp(domain, "sdsp", 4))
>> + return SDSP_DOMAIN_ID;
>> +
>
> The removed code performs a string compare and you replace this with a
> string prefix compare, but there's no motivation given to why this is
> done.
>
> I'm also wondering why cdsp1 is now in CDSP_DOMAIN_ID, is that
> intentional? Was it wrong before? If so, that change should be done
> alone and with a Fixes:
>
cdsp1 use cdsp0 daemon, they are two instances but one domain.
In kernel, we just care about the domains. we just give a scalable
approach without adding instances for any new dsp every time.
> Regards,
> Bjorn
>
>> + return -EINVAL;
>> +}
>> +
>> static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>> {
>> struct device *rdev = &rpdev->dev;
>> @@ -2272,15 +2270,10 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>> return err;
>> }
>>
>> - for (i = 0; i < FASTRPC_DEV_MAX; i++) {
>> - if (!strcmp(domains[i], domain)) {
>> - domain_id = i;
>> - break;
>> - }
>> - }
>> + domain_id = fastrpc_get_domain_id(domain);
>>
>> if (domain_id < 0) {
>> - dev_info(rdev, "FastRPC Invalid Domain ID %d\n", domain_id);
>> + dev_info(rdev, "FastRPC Domain %s not supported\n", domain);
>> return -EINVAL;
>> }
>>
>> @@ -2330,21 +2323,20 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>> case ADSP_DOMAIN_ID:
>> case MDSP_DOMAIN_ID:
>> case SDSP_DOMAIN_ID:
>> - /* Unsigned PD offloading is only supported on CDSP and CDSP1 */
>> + /* Unsigned PD offloading is only supported on CDSP */
>> data->unsigned_support = false;
>> - err = fastrpc_device_register(rdev, data, secure_dsp, domains[domain_id]);
>> + err = fastrpc_device_register(rdev, data, secure_dsp, domain);
>> if (err)
>> goto err_free_data;
>> break;
>> case CDSP_DOMAIN_ID:
>> - case CDSP1_DOMAIN_ID:
>> data->unsigned_support = true;
>> /* Create both device nodes so that we can allow both Signed and Unsigned PD */
>> - err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
>> + err = fastrpc_device_register(rdev, data, true, domain);
>> if (err)
>> goto err_free_data;
>>
>> - err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
>> + err = fastrpc_device_register(rdev, data, false, domain);
>> if (err)
>> goto err_deregister_fdev;
>> break;
>> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
>> index f33d914d8f46..c6e2925f47e6 100644
>> --- a/include/uapi/misc/fastrpc.h
>> +++ b/include/uapi/misc/fastrpc.h
>> @@ -134,7 +134,7 @@ struct fastrpc_mem_unmap {
>> };
>>
>> struct fastrpc_ioctl_capability {
>> - __u32 domain;
>> + __u32 unused; /* deprecated, ignored by the kernel */
>> __u32 attribute_id;
>> __u32 capability; /* dsp capability */
>> __u32 reserved[4];
>> --
>> 2.34.1
>>
--
Thx and BRs,
Ling Xu
More information about the dri-devel
mailing list