Re: [PATCH v2 2/2] soc: qcom: cmd-db: Stop memcpy()ing in cmd_db_read_aux_data()

[Jordan Crouse]

On Mon, Sep 17, 2018 at 01:40:07PM -0700, Stephen Boyd wrote:
> Let's change the function signature to return the pointer to memory or
> an error pointer on failure, and take an argument that lets us return
> the size of the aux data read. This way we can remove the
> cmd_db_read_aux_data_len() API entirely and also get rid of the memcpy
> operation from cmd_db to the caller.
> 
> Cc: Mahesh Sivasubramanian <msivasub@xxxxxxxxxxxxxx>
> Cc: Lina Iyer <ilina@xxxxxxxxxxxxxx>
> Cc: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> Cc: Evan Green <evgreen@xxxxxxxxxxxx>
> Cc: Jordan Crouse <jcrouse@xxxxxxxxxxxxxx>
> Cc: Rob Clark <robdclark@xxxxxxxxx>
> Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 34 ++++++++++-----------
>  drivers/soc/qcom/cmd-db.c             | 43 +++++----------------------
>  include/soc/qcom/cmd-db.h             | 12 ++------
>  3 files changed, 28 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index bbb8126ec5c5..e4387a3fa745 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -860,23 +860,23 @@ static int a6xx_gmu_memory_probe(struct a6xx_gmu *gmu)
>  }
>  
>  /* Get the list of RPMh voltage levels from cmd-db */
> -static int a6xx_gmu_rpmh_arc_cmds(const char *id, void *vals, int size)
> +static const u16 *a6xx_gmu_rpmh_arc_cmds(const char *id, int *size)
>  {
> -	u32 len = cmd_db_read_aux_data_len(id);
> -
> -	if (!len)
> -		return 0;
> -
> -	if (WARN_ON(len > size))
> -		return -EINVAL;
> -
> -	cmd_db_read_aux_data(id, vals, len);
> +	size_t len;
> +	const u16 *vals;
>  
> +	vals = cmd_db_read_aux_data(id, &len);
>  	/*
>  	 * The data comes back as an array of unsigned shorts so adjust the
>  	 * count accordingly
>  	 */
> -	return len >> 1;
> +	len >>= 1;
> +
> +	if (!len || WARN_ON(len > 16))
> +		len = 0;

I don't think we care any more about the size of the array since there isn't any
data being copied.

> +
> +	*size = len;
> +	return vals;
>  }

I'm not sure this function has any value any more. We could just call
cmd_db_read_aux_data() directly for each id and adjust the size on the fly
when we call a6xx_gmu_rpmh_arc_votes_init.

>  /* Return the 'arc-level' for the given frequency */
> @@ -907,8 +907,8 @@ static u32 a6xx_gmu_get_arc_level(struct device *dev, unsigned long freq)
>  
>  static int a6xx_gmu_rpmh_arc_votes_init(struct device *dev, u32 *votes,
>  		unsigned long *freqs, int freqs_count,
> -		u16 *pri, int pri_count,
> -		u16 *sec, int sec_count)
> +		const u16 *pri, int pri_count,
> +		const u16 *sec, int sec_count)
>  {
>  	int i, j;
>  
> @@ -970,14 +970,14 @@ static int a6xx_gmu_rpmh_votes_init(struct a6xx_gmu *gmu)
>  	struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
>  	struct msm_gpu *gpu = &adreno_gpu->base;
>  
> -	u16 gx[16], cx[16], mx[16];
> +	const u16 *gx, *cx, *mx;
>  	u32 gxcount, cxcount, mxcount;
>  	int ret;
>  
>  	/* Get the list of available voltage levels for each component */
> -	gxcount = a6xx_gmu_rpmh_arc_cmds("gfx.lvl", gx, sizeof(gx));
> -	cxcount = a6xx_gmu_rpmh_arc_cmds("cx.lvl", cx, sizeof(cx));
> -	mxcount = a6xx_gmu_rpmh_arc_cmds("mx.lvl", mx, sizeof(mx));
> +	gx = a6xx_gmu_rpmh_arc_cmds("gfx.lvl", &gxcount);
> +	cx = a6xx_gmu_rpmh_arc_cmds("cx.lvl", &cxcount);
> +	mx = a6xx_gmu_rpmh_arc_cmds("mx.lvl", &mxcount);
>  
>  	/* Build the GX votes */
>  	ret = a6xx_gmu_rpmh_arc_votes_init(&gpu->pdev->dev, gmu->gx_arc_votes,
> diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
> index 5c9cc6824891..c701b3b010f1 100644
> --- a/drivers/soc/qcom/cmd-db.c
> +++ b/drivers/soc/qcom/cmd-db.c
> @@ -192,55 +192,28 @@ EXPORT_SYMBOL(cmd_db_read_addr);
>  /**
>   * cmd_db_read_aux_data() - Query command db for aux data.
>   *
> - *  @id: Resource to retrieve AUX Data on.
> - *  @data: Data buffer to copy returned aux data to. Returns size on NULL
> - *  @len: Caller provides size of data buffer passed in.
> + *  @id: Resource to retrieve AUX Data on
> + *  @len: size of data buffer returned

Is the GPU the only consumer of this function?  Would it make more sense to
return the number of entries rather than the size? I would suppose that would
imply that the caller knows the size of the entries but we have to know that
anyway if we're going to walk the list.

>   *
> - *  Return: size of data on success, errno otherwise
> + *  Return: pointer to data on success, error pointer otherwise
>   */
> -int cmd_db_read_aux_data(const char *id, u8 *data, size_t len)
> +const void *cmd_db_read_aux_data(const char *id, size_t *len)
>  {
>  	int ret;
>  	const struct entry_header *ent;
>  	const struct rsc_hdr *rsc_hdr;
> -	u16 ent_len;
> -
> -	if (!data)
> -		return -EINVAL;
>  
>  	ret = cmd_db_get_header(id, &ent, &rsc_hdr);
>  	if (ret)
> -		return ret;
> -
> -	ent_len = le16_to_cpu(ent->len);
> -	if (len < ent_len)
> -		return -EINVAL;
> +		return ERR_PTR(ret);
>  
> -	len = min_t(u16, ent_len, len);
> -	memcpy(data, rsc_offset(rsc_hdr, ent), len);
> +	if (len)
> +		*len = le16_to_cpu(ent->len);
>  
> -	return len;
> +	return rsc_offset(rsc_hdr, ent);
>  }
>  EXPORT_SYMBOL(cmd_db_read_aux_data);

<snip>

Jordan
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project