gpu: host1x: Reduce host1x job allocation size

There is 2 set of num_relocs * sizeof(*) array at the end of host1x job.
Only the first is really used and with job->gather_addr_phys pointing
somewhere within the 1st set of reloc physical addresses.
During pin_job, job->gather_addr_phys array is set indirectly by indexing
num_relocs offset of job->reloc_addr_phys.
Correct the total job size, explicily set job->gather_addr_phys array in
2nd for loop of pin_job and record its physical address.

Signed-off-by: David Ung <davidu@nvidia.com>
---
 drivers/gpu/host1x/job.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Ping?

> -----Original Message-----
> From: David Ung [mailto:davidu@nvidia.com]
> Sent: Friday, January 23, 2015 6:36 PM
> To: airlied@linux.ie
> Cc: linux-tegra@vger.kernel.org; thierry.reding@gmail.com;
> swarren@wwwdotorg.org; Arto Merilainen; David Ung
> Subject: [PATCH] gpu: host1x: Reduce host1x job allocation size
> 
> There is 2 set of num_relocs * sizeof(*) array at the end of host1x job.
> Only the first is really used and with job->gather_addr_phys pointing
> somewhere within the 1st set of reloc physical addresses.
> During pin_job, job->gather_addr_phys array is set indirectly by indexing
> num_relocs offset of job->reloc_addr_phys.
> Correct the total job size, explicily set job->gather_addr_phys array in 2nd
> for loop of pin_job and record its physical address.
> 
> Signed-off-by: David Ung <davidu@nvidia.com>
> ---
>  drivers/gpu/host1x/job.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c index
> 63bd63f..37c67d2 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
> @@ -46,8 +46,8 @@ struct host1x_job *host1x_job_alloc(struct
> host1x_channel *ch,
>  		(u64)num_unpins * sizeof(struct host1x_job_unpin_data) +
>  		(u64)num_waitchks * sizeof(struct host1x_waitchk) +
>  		(u64)num_cmdbufs * sizeof(struct host1x_job_gather) +
> -		(u64)num_unpins * sizeof(dma_addr_t) +
> -		(u64)num_unpins * sizeof(u32 *);
> +		(u64)num_relocs * sizeof(dma_addr_t) +
> +		(u64)num_cmdbufs * sizeof(dma_addr_t);
>  	if (total > ULONG_MAX)
>  		return NULL;
> 
> @@ -212,7 +212,8 @@ static unsigned int pin_job(struct host1x_job *job)
>  		if (!phys_addr)
>  			goto unpin;
> 
> -		job->addr_phys[job->num_unpins] = phys_addr;
> +		g->base = phys_addr;
> +		job->gather_addr_phys[i] = phys_addr;
>  		job->unpins[job->num_unpins].bo = g->bo;
>  		job->unpins[job->num_unpins].sgt = sgt;
>  		job->num_unpins++;
> @@ -536,8 +537,6 @@ int host1x_job_pin(struct host1x_job *job, struct
> device *dev)
>  		if (g->handled)
>  			continue;
> 
> -		g->base = job->gather_addr_phys[i];
> -
>  		for (j = i + 1; j < job->num_gathers; j++)
>  			if (job->gathers[j].bo == g->bo)
>  				job->gathers[j].handled = true;
> --
> 1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Apologies for the delayed review.

On Sat, Jan 24, 2015 at 11:35 AM, David Ung <davidu@nvidia.com> wrote:
> There is 2 set of num_relocs * sizeof(*) array at the end of host1x job.
> Only the first is really used and with job->gather_addr_phys pointing
> somewhere within the 1st set of reloc physical addresses.
> During pin_job, job->gather_addr_phys array is set indirectly by indexing
> num_relocs offset of job->reloc_addr_phys.
> Correct the total job size, explicily set job->gather_addr_phys array in
> 2nd for loop of pin_job and record its physical address.
>
> Signed-off-by: David Ung <davidu@nvidia.com>
> ---
>  drivers/gpu/host1x/job.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> index 63bd63f..37c67d2 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
> @@ -46,8 +46,8 @@ struct host1x_job *host1x_job_alloc(struct host1x_channel *ch,
>                 (u64)num_unpins * sizeof(struct host1x_job_unpin_data) +
>                 (u64)num_waitchks * sizeof(struct host1x_waitchk) +
>                 (u64)num_cmdbufs * sizeof(struct host1x_job_gather) +
> -               (u64)num_unpins * sizeof(dma_addr_t) +
> -               (u64)num_unpins * sizeof(u32 *);
> +               (u64)num_relocs * sizeof(dma_addr_t) +
> +               (u64)num_cmdbufs * sizeof(dma_addr_t);

This seems to be correct but I'd like to have confirmation from
Arto/Thierry/Terje in case I overlooked something.

>         if (total > ULONG_MAX)
>                 return NULL;
>
> @@ -212,7 +212,8 @@ static unsigned int pin_job(struct host1x_job *job)
>                 if (!phys_addr)
>                         goto unpin;
>
> -               job->addr_phys[job->num_unpins] = phys_addr;
> +               g->base = phys_addr;
> +               job->gather_addr_phys[i] = phys_addr;

Looking through the driver it is not clear to me why we have both
addr_phys and the gather_addr_phys/reloc_addr_phys couple since we end
up using addr_phys instead. It seems like other replacements like this
one could be done to improve code clarity. Ultimately I don't see any
reason why addr_phys should even exist - maybe see if you could
replace its uses with one of gather_addr_phys or reloc_addr_phys for a
v2 of this patch?

I suspect a v2 should be divided into two patches, one that fixes the
allocation size, the other that removes the addr_phys member.

Also on a related note I suspect host1x_job_alloc() could set
num_relocs and num_waitchk itself instead of relying on the caller to
do so (that would be 3 patches then :)). Someone who understands
host1x better, please contradict me if this is wrong.

>                 job->unpins[job->num_unpins].bo = g->bo;
>                 job->unpins[job->num_unpins].sgt = sgt;
>                 job->num_unpins++;
> @@ -536,8 +537,6 @@ int host1x_job_pin(struct host1x_job *job, struct device *dev)
>                 if (g->handled)
>                         continue;
>
> -               g->base = job->gather_addr_phys[i];
> -
>                 for (j = i + 1; j < job->num_gathers; j++)
>                         if (job->gathers[j].bo == g->bo)
>                                 job->gathers[j].handled = true;
> --
> 1.8.1.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html