[PATCH 1/2] gpu: host1x: Don't fail on NULL bo physical address

[PATCH 1/2] gpu: host1x: Don't fail on NULL bo physical address

[Mikko Perttunen]

Pinning a Host1x BO currently cannot fail and zero is a valid address
for a BO when IOMMU is enabled. To avoid false errors remove checks
for NULL BO physical addresses.

Fixes: 404bfb78daf3 ("gpu: host1x: Add IOMMU support")
Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/gpu/host1x/job.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index bee504406cfc..db509ab8874e 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -197,10 +197,6 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 		}
 
 		phys_addr = host1x_bo_pin(reloc->target.bo, &sgt);
-		if (!phys_addr) {
-			err = -EINVAL;
-			goto unpin;
-		}
 
 		job->addr_phys[job->num_unpins] = phys_addr;
 		job->unpins[job->num_unpins].bo = reloc->target.bo;
@@ -225,10 +221,6 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 		}
 
 		phys_addr = host1x_bo_pin(g->bo, &sgt);
-		if (!phys_addr) {
-			err = -EINVAL;
-			goto unpin;
-		}
 
 		if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && host->domain) {
 			for_each_sg(sgt->sgl, sg, sgt->nents, j)
-- 
2.13.3

[PATCH 2/2] gpu: host1x: Fix bitshift/mask multipliers

[Mikko Perttunen]

Some parts of Host1x uses BIT_WORD/BIT_MASK/BITS_PER_LONG to calculate
register or field offsets. This worked fine on ARMv7, but now that
BITS_PER_LONG is 64 but our registers are still 32-bit things are
broken.

Fix by replacing..
- BIT_WORD with (x / 32)
- BIT_MASK with BIT(x % 32)
- BITS_PER_LONG with 32

Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/gpu/host1x/hw/intr_hw.c   | 24 ++++++++++++------------
 drivers/gpu/host1x/hw/syncpt_hw.c |  2 +-
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/host1x/hw/intr_hw.c b/drivers/gpu/host1x/hw/intr_hw.c
index dacb8009a605..37ebb51703fa 100644
--- a/drivers/gpu/host1x/hw/intr_hw.c
+++ b/drivers/gpu/host1x/hw/intr_hw.c
@@ -33,10 +33,10 @@ static void host1x_intr_syncpt_handle(struct host1x_syncpt *syncpt)
 	unsigned int id = syncpt->id;
 	struct host1x *host = syncpt->host;
 
-	host1x_sync_writel(host, BIT_MASK(id),
-		HOST1X_SYNC_SYNCPT_THRESH_INT_DISABLE(BIT_WORD(id)));
-	host1x_sync_writel(host, BIT_MASK(id),
-		HOST1X_SYNC_SYNCPT_THRESH_CPU0_INT_STATUS(BIT_WORD(id)));
+	host1x_sync_writel(host, BIT(id % 32),
+		HOST1X_SYNC_SYNCPT_THRESH_INT_DISABLE(id / 32));
+	host1x_sync_writel(host, BIT(id % 32),
+		HOST1X_SYNC_SYNCPT_THRESH_CPU0_INT_STATUS(id / 32));
 
 	schedule_work(&syncpt->intr.work);
 }
@@ -50,9 +50,9 @@ static irqreturn_t syncpt_thresh_isr(int irq, void *dev_id)
 	for (i = 0; i < DIV_ROUND_UP(host->info->nb_pts, 32); i++) {
 		reg = host1x_sync_readl(host,
 			HOST1X_SYNC_SYNCPT_THRESH_CPU0_INT_STATUS(i));
-		for_each_set_bit(id, &reg, BITS_PER_LONG) {
+		for_each_set_bit(id, &reg, 32) {
 			struct host1x_syncpt *syncpt =
-				host->syncpt + (i * BITS_PER_LONG + id);
+				host->syncpt + (i * 32 + id);
 			host1x_intr_syncpt_handle(syncpt);
 		}
 	}
@@ -117,17 +117,17 @@ static void _host1x_intr_set_syncpt_threshold(struct host1x *host,
 static void _host1x_intr_enable_syncpt_intr(struct host1x *host,
 					    unsigned int id)
 {
-	host1x_sync_writel(host, BIT_MASK(id),
-		HOST1X_SYNC_SYNCPT_THRESH_INT_ENABLE_CPU0(BIT_WORD(id)));
+	host1x_sync_writel(host, BIT(id % 32),
+		HOST1X_SYNC_SYNCPT_THRESH_INT_ENABLE_CPU0(id / 32));
 }
 
 static void _host1x_intr_disable_syncpt_intr(struct host1x *host,
 					     unsigned int id)
 {
-	host1x_sync_writel(host, BIT_MASK(id),
-		HOST1X_SYNC_SYNCPT_THRESH_INT_DISABLE(BIT_WORD(id)));
-	host1x_sync_writel(host, BIT_MASK(id),
-		HOST1X_SYNC_SYNCPT_THRESH_CPU0_INT_STATUS(BIT_WORD(id)));
+	host1x_sync_writel(host, BIT(id % 32),
+		HOST1X_SYNC_SYNCPT_THRESH_INT_DISABLE(id / 32));
+	host1x_sync_writel(host, BIT(id % 32),
+		HOST1X_SYNC_SYNCPT_THRESH_CPU0_INT_STATUS(id / 32));
 }
 
 static int _host1x_free_syncpt_irq(struct host1x *host)
diff --git a/drivers/gpu/host1x/hw/syncpt_hw.c b/drivers/gpu/host1x/hw/syncpt_hw.c
index c93f74fcce72..7b0270d60742 100644
--- a/drivers/gpu/host1x/hw/syncpt_hw.c
+++ b/drivers/gpu/host1x/hw/syncpt_hw.c
@@ -89,7 +89,7 @@ static int syncpt_cpu_incr(struct host1x_syncpt *sp)
 	    host1x_syncpt_idle(sp))
 		return -EINVAL;
 
-	host1x_sync_writel(host, BIT_MASK(sp->id),
+	host1x_sync_writel(host, BIT(sp->id % 32),
 			   HOST1X_SYNC_SYNCPT_CPU_INCR(reg_offset));
 	wmb();
 
-- 
2.13.3

Re: [PATCH 2/2] gpu: host1x: Fix bitshift/mask multipliers

[Dmitry Osipenko]

On 02.08.2017 12:55, Mikko Perttunen wrote:
> Some parts of Host1x uses BIT_WORD/BIT_MASK/BITS_PER_LONG to calculate
> register or field offsets. This worked fine on ARMv7, but now that
> BITS_PER_LONG is 64 but our registers are still 32-bit things are
> broken.
> 
> Fix by replacing..
> - BIT_WORD with (x / 32)
> - BIT_MASK with BIT(x % 32)
> - BITS_PER_LONG with 32
> 
> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/gpu/host1x/hw/intr_hw.c   | 24 ++++++++++++------------
>  drivers/gpu/host1x/hw/syncpt_hw.c |  2 +-
>  2 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/host1x/hw/intr_hw.c b/drivers/gpu/host1x/hw/intr_hw.c
> index dacb8009a605..37ebb51703fa 100644
> --- a/drivers/gpu/host1x/hw/intr_hw.c
> +++ b/drivers/gpu/host1x/hw/intr_hw.c
> @@ -33,10 +33,10 @@ static void host1x_intr_syncpt_handle(struct host1x_syncpt *syncpt)
>  	unsigned int id = syncpt->id;
>  	struct host1x *host = syncpt->host;
>  
> -	host1x_sync_writel(host, BIT_MASK(id),
> -		HOST1X_SYNC_SYNCPT_THRESH_INT_DISABLE(BIT_WORD(id)));
> -	host1x_sync_writel(host, BIT_MASK(id),
> -		HOST1X_SYNC_SYNCPT_THRESH_CPU0_INT_STATUS(BIT_WORD(id)));
> +	host1x_sync_writel(host, BIT(id % 32),
> +		HOST1X_SYNC_SYNCPT_THRESH_INT_DISABLE(id / 32));
> +	host1x_sync_writel(host, BIT(id % 32),
> +		HOST1X_SYNC_SYNCPT_THRESH_CPU0_INT_STATUS(id / 32));
>  
>  	schedule_work(&syncpt->intr.work);
>  }
> @@ -50,9 +50,9 @@ static irqreturn_t syncpt_thresh_isr(int irq, void *dev_id)
>  	for (i = 0; i < DIV_ROUND_UP(host->info->nb_pts, 32); i++) {
>  		reg = host1x_sync_readl(host,
>  			HOST1X_SYNC_SYNCPT_THRESH_CPU0_INT_STATUS(i));
> -		for_each_set_bit(id, &reg, BITS_PER_LONG) {
> +		for_each_set_bit(id, &reg, 32) {
>  			struct host1x_syncpt *syncpt =
> -				host->syncpt + (i * BITS_PER_LONG + id);
> +				host->syncpt + (i * 32 + id);
>  			host1x_intr_syncpt_handle(syncpt);
>  		}
>  	}
> @@ -117,17 +117,17 @@ static void _host1x_intr_set_syncpt_threshold(struct host1x *host,
>  static void _host1x_intr_enable_syncpt_intr(struct host1x *host,
>  					    unsigned int id)
>  {
> -	host1x_sync_writel(host, BIT_MASK(id),
> -		HOST1X_SYNC_SYNCPT_THRESH_INT_ENABLE_CPU0(BIT_WORD(id)));
> +	host1x_sync_writel(host, BIT(id % 32),
> +		HOST1X_SYNC_SYNCPT_THRESH_INT_ENABLE_CPU0(id / 32));
>  }
>  
>  static void _host1x_intr_disable_syncpt_intr(struct host1x *host,
>  					     unsigned int id)
>  {
> -	host1x_sync_writel(host, BIT_MASK(id),
> -		HOST1X_SYNC_SYNCPT_THRESH_INT_DISABLE(BIT_WORD(id)));
> -	host1x_sync_writel(host, BIT_MASK(id),
> -		HOST1X_SYNC_SYNCPT_THRESH_CPU0_INT_STATUS(BIT_WORD(id)));
> +	host1x_sync_writel(host, BIT(id % 32),
> +		HOST1X_SYNC_SYNCPT_THRESH_INT_DISABLE(id / 32));
> +	host1x_sync_writel(host, BIT(id % 32),
> +		HOST1X_SYNC_SYNCPT_THRESH_CPU0_INT_STATUS(id / 32));
>  }
>  
>  static int _host1x_free_syncpt_irq(struct host1x *host)
> diff --git a/drivers/gpu/host1x/hw/syncpt_hw.c b/drivers/gpu/host1x/hw/syncpt_hw.c
> index c93f74fcce72..7b0270d60742 100644
> --- a/drivers/gpu/host1x/hw/syncpt_hw.c
> +++ b/drivers/gpu/host1x/hw/syncpt_hw.c
> @@ -89,7 +89,7 @@ static int syncpt_cpu_incr(struct host1x_syncpt *sp)
>  	    host1x_syncpt_idle(sp))
>  		return -EINVAL;
>  
> -	host1x_sync_writel(host, BIT_MASK(sp->id),
> +	host1x_sync_writel(host, BIT(sp->id % 32),
>  			   HOST1X_SYNC_SYNCPT_CPU_INCR(reg_offset));
>  	wmb();
>  
> 

Reviewed-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Tested-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
Dmitry

Re: [PATCH 2/2] gpu: host1x: Fix bitshift/mask multipliers

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 700 bytes --]

On Wed, Aug 02, 2017 at 12:55:05PM +0300, Mikko Perttunen wrote:
> Some parts of Host1x uses BIT_WORD/BIT_MASK/BITS_PER_LONG to calculate
> register or field offsets. This worked fine on ARMv7, but now that
> BITS_PER_LONG is 64 but our registers are still 32-bit things are
> broken.
> 
> Fix by replacing..
> - BIT_WORD with (x / 32)
> - BIT_MASK with BIT(x % 32)
> - BITS_PER_LONG with 32
> 
> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/gpu/host1x/hw/intr_hw.c   | 24 ++++++++++++------------
>  drivers/gpu/host1x/hw/syncpt_hw.c |  2 +-
>  2 files changed, 13 insertions(+), 13 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] gpu: host1x: Don't fail on NULL bo physical address

[Dmitry Osipenko]

On 02.08.2017 12:55, Mikko Perttunen wrote:
> Pinning a Host1x BO currently cannot fail and zero is a valid address
> for a BO when IOMMU is enabled. To avoid false errors remove checks
> for NULL BO physical addresses.
> 
> Fixes: 404bfb78daf3 ("gpu: host1x: Add IOMMU support")
> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/gpu/host1x/job.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> index bee504406cfc..db509ab8874e 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
> @@ -197,10 +197,6 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
>  		}
>  
>  		phys_addr = host1x_bo_pin(reloc->target.bo, &sgt);
> -		if (!phys_addr) {
> -			err = -EINVAL;
> -			goto unpin;
> -		}
>  
>  		job->addr_phys[job->num_unpins] = phys_addr;
>  		job->unpins[job->num_unpins].bo = reloc->target.bo;
> @@ -225,10 +221,6 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
>  		}
>  
>  		phys_addr = host1x_bo_pin(g->bo, &sgt);
> -		if (!phys_addr) {
> -			err = -EINVAL;
> -			goto unpin;
> -		}
>  
>  		if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && host->domain) {
>  			for_each_sg(sgt->sgl, sg, sgt->nents, j)
> 

I think 'Fixes' tag isn't really needed for this patch since it's not a bug fix,
but a cleanup.

Reviewed-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
Dmitry

Re: [PATCH 1/2] gpu: host1x: Don't fail on NULL bo physical address

[Mikko Perttunen]

On 08/02/2017 02:06 PM, Dmitry Osipenko wrote:
> On 02.08.2017 12:55, Mikko Perttunen wrote:
>> Pinning a Host1x BO currently cannot fail and zero is a valid address
>> for a BO when IOMMU is enabled. To avoid false errors remove checks
>> for NULL BO physical addresses.
>>
>> Fixes: 404bfb78daf3 ("gpu: host1x: Add IOMMU support")
>> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>   drivers/gpu/host1x/job.c | 8 --------
>>   1 file changed, 8 deletions(-)
>>
>> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
>> index bee504406cfc..db509ab8874e 100644
>> --- a/drivers/gpu/host1x/job.c
>> +++ b/drivers/gpu/host1x/job.c
>> @@ -197,10 +197,6 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
>>   		}
>>   
>>   		phys_addr = host1x_bo_pin(reloc->target.bo, &sgt);
>> -		if (!phys_addr) {
>> -			err = -EINVAL;
>> -			goto unpin;
>> -		}
>>   
>>   		job->addr_phys[job->num_unpins] = phys_addr;
>>   		job->unpins[job->num_unpins].bo = reloc->target.bo;
>> @@ -225,10 +221,6 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
>>   		}
>>   
>>   		phys_addr = host1x_bo_pin(g->bo, &sgt);
>> -		if (!phys_addr) {
>> -			err = -EINVAL;
>> -			goto unpin;
>> -		}
>>   
>>   		if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && host->domain) {
>>   			for_each_sg(sgt->sgl, sg, sgt->nents, j)
>>
> 
> I think 'Fixes' tag isn't really needed for this patch since it's not a bug fix,
> but a cleanup.
> 
> Reviewed-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 

Actually without this, host1x_test fails for me on the first try on TX1 
:) So it's a bugfix.

Thanks for the reviews and tests.

Mikko

Re: [PATCH 1/2] gpu: host1x: Don't fail on NULL bo physical address

[Dmitry Osipenko]

On 02.08.2017 14:16, Mikko Perttunen wrote:
> On 08/02/2017 02:06 PM, Dmitry Osipenko wrote:
>> On 02.08.2017 12:55, Mikko Perttunen wrote:
>>> Pinning a Host1x BO currently cannot fail and zero is a valid address
>>> for a BO when IOMMU is enabled. To avoid false errors remove checks
>>> for NULL BO physical addresses.
>>>
>>> Fixes: 404bfb78daf3 ("gpu: host1x: Add IOMMU support")
>>> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>   drivers/gpu/host1x/job.c | 8 --------
>>>   1 file changed, 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
>>> index bee504406cfc..db509ab8874e 100644
>>> --- a/drivers/gpu/host1x/job.c
>>> +++ b/drivers/gpu/host1x/job.c
>>> @@ -197,10 +197,6 @@ static unsigned int pin_job(struct host1x *host, struct
>>> host1x_job *job)
>>>           }
>>>             phys_addr = host1x_bo_pin(reloc->target.bo, &sgt);
>>> -        if (!phys_addr) {
>>> -            err = -EINVAL;
>>> -            goto unpin;
>>> -        }
>>>             job->addr_phys[job->num_unpins] = phys_addr;
>>>           job->unpins[job->num_unpins].bo = reloc->target.bo;
>>> @@ -225,10 +221,6 @@ static unsigned int pin_job(struct host1x *host, struct
>>> host1x_job *job)
>>>           }
>>>             phys_addr = host1x_bo_pin(g->bo, &sgt);
>>> -        if (!phys_addr) {
>>> -            err = -EINVAL;
>>> -            goto unpin;
>>> -        }
>>>             if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && host->domain) {
>>>               for_each_sg(sgt->sgl, sg, sgt->nents, j)
>>>
>>
>> I think 'Fixes' tag isn't really needed for this patch since it's not a bug fix,
>> but a cleanup.
>>
>> Reviewed-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
> 
> Actually without this, host1x_test fails for me on the first try on TX1 :) So
> it's a bugfix.

Right, my bad.

> 
> Thanks for the reviews and tests.
> 

I haven't tested this patch, only the the second because I've tested it on top
of grate's kernel which has a conflicting patch being applied.

-- 
Dmitry

Re: [PATCH 1/2] gpu: host1x: Don't fail on NULL bo physical address

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 509 bytes --]

On Wed, Aug 02, 2017 at 12:55:04PM +0300, Mikko Perttunen wrote:
> Pinning a Host1x BO currently cannot fail and zero is a valid address
> for a BO when IOMMU is enabled. To avoid false errors remove checks
> for NULL BO physical addresses.
> 
> Fixes: 404bfb78daf3 ("gpu: host1x: Add IOMMU support")
> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/gpu/host1x/job.c | 8 --------
>  1 file changed, 8 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]