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 <msiva...@codeaurora.org>
> Cc: Lina Iyer <il...@codeaurora.org>
> Cc: Bjorn Andersson <bjorn.anders...@linaro.org>
> Cc: Evan Green <evgr...@chromium.org>
> Cc: Jordan Crouse <jcro...@codeaurora.org>
> Cc: Rob Clark <robdcl...@gmail.com>
> Signed-off-by: Stephen Boyd <swb...@chromium.org>
> ---
> 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
