Re: [PATCH v3 3/3] misc: fastrpc: add support for gdsp remoteproc

[Dmitry Baryshkov]

On Tue, Jun 24, 2025 at 01:58:47PM +0800, Ling Xu wrote:
> 在 6/23/2025 6:28 PM, Konrad Dybcio 写道:
> > On 6/22/25 3:38 PM, Ling Xu wrote:
> >> The fastrpc driver has support for 5 types of remoteprocs. There are
> >> some products which support GDSP remoteprocs. Add changes to support
> >> GDSP remoteprocs.
> > 
> > Commit messages saying "add changes to support xyz" often indicate
> > the problem or the non-obvious solution is not properly described
> > (which is the case here as well)
> > 
> > [...]
> > 
> 
> Okay, I will change to
> "Add related domain IDS to support GDSP remoteprocs."
> 
> >> +static int fastrpc_get_domain_id(const char *domain)
> >> +{
> >> +	if (strncmp(domain, "adsp", 4) == 0)
> > 
> > if (!strncmp(...)) is the common syntax, although it's obviously
> > not functionally different
> > 
> >> +		return ADSP_DOMAIN_ID;
> >> +	else if (strncmp(domain, "cdsp", 4) == 0)
> >> +		return CDSP_DOMAIN_ID;
> >> +	else if (strncmp(domain, "mdsp", 4) == 0)
> >> +		return MDSP_DOMAIN_ID;
> >> +	else if (strncmp(domain, "sdsp", 4) == 0)
> >> +		return SDSP_DOMAIN_ID;
> >> +	else if (strncmp(domain, "gdsp", 4) == 0)
> >> +		return GDSP_DOMAIN_ID;
> > 
> > FWIW, other places call it G*P*DSP
> > 
> In fastrpc, we call it GDSP to match dsp side.
> because in device,the related path for gdsp images are gdsp and gdsp0.
> > [...]
> > 
> >> --- a/include/uapi/misc/fastrpc.h
> >> +++ b/include/uapi/misc/fastrpc.h
> >> @@ -18,6 +18,14 @@
> >>  #define FASTRPC_IOCTL_MEM_UNMAP		_IOWR('R', 11, struct fastrpc_mem_unmap)
> >>  #define FASTRPC_IOCTL_GET_DSP_INFO	_IOWR('R', 13, struct fastrpc_ioctl_capability)
> >>  
> >> +#define ADSP_DOMAIN_ID (0)
> >> +#define MDSP_DOMAIN_ID (1)
> >> +#define SDSP_DOMAIN_ID (2)
> >> +#define CDSP_DOMAIN_ID (3)
> >> +#define GDSP_DOMAIN_ID (4)
> >> +
> >> +#define FASTRPC_DOMAIN_MAX    4
> > 
> > What are these used for now?
> > 
> To get proper domain IDs for fastrpc_rpmsg_probe etc.

These seem to be driver-internal, so they don't need to be exposed to
userspace.

> >>  /**
> >>   * enum fastrpc_map_flags - control flags for mapping memory on DSP user process
> >>   * @FASTRPC_MAP_STATIC: Map memory pages with RW- permission and CACHE WRITEBACK.
> >> @@ -134,10 +142,9 @@ struct fastrpc_mem_unmap {
> >>  };
> >>  
> >>  struct fastrpc_ioctl_capability {
> >> -	__u32 domain;
> >>  	__u32 attribute_id;
> >>  	__u32 capability;   /* dsp capability */
> >> -	__u32 reserved[4];
> >> +	__u32 reserved[5];
> > 
> > This is an ABI break, as the data within structs is well, structured
> 
> this is suggested by Dmitry, I will have a discussion internally.

No, I didn't suggest to break the ABI. I suggested making the domain
field reserved.

> > 
> > Konrad
> 
> -- 
> Thx and BRs,
> Ling Xu
> 

-- 
With best wishes
Dmitry