[PATCH V2] gpu: host1x: handle the correct # of syncpt regs

[PATCH V2] gpu: host1x: handle the correct # of syncpt regs

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

BIT_WORD() truncates rather than rounds, so the loops in
syncpt_thresh_isr() and _host1x_intr_disable_all_syncpt_intrs() use <=
rather than < in an attempt to process the correct number of registers
when rounding of the conversion of count of bits to count of words is
necessary. However, when rounding isn't necessary because the value is
already a multiple of the divisor (as is the case for all values of
nb_pts the code actually sees), this causes one too many registers to
be processed.

Solve this by using and explicit DIV_ROUND_UP() call, rather than
BIT_WORD(), and comparing with < rather than <=.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
v2: Use DIV_ROUND_UP rather than BITS_TO_LONGS to avoid problems on 64-bit.
---
 drivers/gpu/host1x/hw/intr_hw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/host1x/hw/intr_hw.c b/drivers/gpu/host1x/hw/intr_hw.c
index db9017adfe2b..498b37e39058 100644
--- a/drivers/gpu/host1x/hw/intr_hw.c
+++ b/drivers/gpu/host1x/hw/intr_hw.c
@@ -47,7 +47,7 @@ static irqreturn_t syncpt_thresh_isr(int irq, void *dev_id)
 	unsigned long reg;
 	int i, id;
 
-	for (i = 0; i <= BIT_WORD(host->info->nb_pts); i++) {
+	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) {
@@ -64,7 +64,7 @@ static void _host1x_intr_disable_all_syncpt_intrs(struct host1x *host)
 {
 	u32 i;
 
-	for (i = 0; i <= BIT_WORD(host->info->nb_pts); ++i) {
+	for (i = 0; i < DIV_ROUND_UP(host->info->nb_pts, 32); ++i) {
 		host1x_sync_writel(host, 0xffffffffu,
 			HOST1X_SYNC_SYNCPT_THRESH_INT_DISABLE(i));
 		host1x_sync_writel(host, 0xffffffffu,
-- 
1.8.1.5

Re: [PATCH V2] gpu: host1x: handle the correct # of syncpt regs

[Thierry Reding]

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

On Fri, Apr 04, 2014 at 04:31:05PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> BIT_WORD() truncates rather than rounds, so the loops in
> syncpt_thresh_isr() and _host1x_intr_disable_all_syncpt_intrs() use <=
> rather than < in an attempt to process the correct number of registers
> when rounding of the conversion of count of bits to count of words is
> necessary. However, when rounding isn't necessary because the value is
> already a multiple of the divisor (as is the case for all values of
> nb_pts the code actually sees), this causes one too many registers to
> be processed.
> 
> Solve this by using and explicit DIV_ROUND_UP() call, rather than
> BIT_WORD(), and comparing with < rather than <=.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> v2: Use DIV_ROUND_UP rather than BITS_TO_LONGS to avoid problems on 64-bit.
> ---
>  drivers/gpu/host1x/hw/intr_hw.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

If I understand correctly there's no immediate need for this to go to
stable kernels, nor for it to be queued for 3.15, right? That is the
potential extra write isn't causing any harm on actual hardware, is it?

In that case I'll queue this up for 3.16.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2] gpu: host1x: handle the correct # of syncpt regs

[Terje Bergström]

On 07.04.2014 11:18, Thierry Reding wrote:
> If I understand correctly there's no immediate need for this to go to
> stable kernels, nor for it to be queued for 3.15, right? That is the
> potential extra write isn't causing any harm on actual hardware, is it?
> 
> In that case I'll queue this up for 3.16.

The reads and writes would get ignored on 32-bit kernel. The change does
fix sync point behavior in 64-bit kernel, so it is fixing a real issue.

Terje

Re: [PATCH V2] gpu: host1x: handle the correct # of syncpt regs

[Thierry Reding]

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

On Mon, Apr 07, 2014 at 11:34:22AM +0300, Terje Bergström wrote:
> On 07.04.2014 11:18, Thierry Reding wrote:
> > If I understand correctly there's no immediate need for this to go to
> > stable kernels, nor for it to be queued for 3.15, right? That is the
> > potential extra write isn't causing any harm on actual hardware, is it?
> > 
> > In that case I'll queue this up for 3.16.
> 
> The reads and writes would get ignored on 32-bit kernel. The change does
> fix sync point behavior in 64-bit kernel, so it is fixing a real issue.

Okay, but given that we don't support any 64 bit Tegra hardware upstream
yet, 3.16 would still be enough, wouldn't it?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2] gpu: host1x: handle the correct # of syncpt regs

[Terje Bergström]

On 07.04.2014 11:41, Thierry Reding wrote:
> On Mon, Apr 07, 2014 at 11:34:22AM +0300, Terje Bergström wrote:
>> On 07.04.2014 11:18, Thierry Reding wrote:
>>> If I understand correctly there's no immediate need for this to go to
>>> stable kernels, nor for it to be queued for 3.15, right? That is the
>>> potential extra write isn't causing any harm on actual hardware, is it?
>>>
>>> In that case I'll queue this up for 3.16.
>>
>> The reads and writes would get ignored on 32-bit kernel. The change does
>> fix sync point behavior in 64-bit kernel, so it is fixing a real issue.
> 
> Okay, but given that we don't support any 64 bit Tegra hardware upstream
> yet, 3.16 would still be enough, wouldn't it?

Sure, sounds good.

Terje

Re: [PATCH V2] gpu: host1x: handle the correct # of syncpt regs

[Stephen Warren]

On 04/07/2014 02:18 AM, Thierry Reding wrote:
> On Fri, Apr 04, 2014 at 04:31:05PM -0600, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> BIT_WORD() truncates rather than rounds, so the loops in
>> syncpt_thresh_isr() and _host1x_intr_disable_all_syncpt_intrs() use <=
>> rather than < in an attempt to process the correct number of registers
>> when rounding of the conversion of count of bits to count of words is
>> necessary. However, when rounding isn't necessary because the value is
>> already a multiple of the divisor (as is the case for all values of
>> nb_pts the code actually sees), this causes one too many registers to
>> be processed.
>>
>> Solve this by using and explicit DIV_ROUND_UP() call, rather than
>> BIT_WORD(), and comparing with < rather than <=.
>>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>> v2: Use DIV_ROUND_UP rather than BITS_TO_LONGS to avoid problems on 64-bit.
>> ---
>>  drivers/gpu/host1x/hw/intr_hw.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> If I understand correctly there's no immediate need for this to go to
> stable kernels, nor for it to be queued for 3.15, right? That is the
> potential extra write isn't causing any harm on actual hardware, is it?
> 
> In that case I'll queue this up for 3.16.

We should definitely apply this, and as far back as the code exists,
since the SW is touching non-existent registers, and that is presumably
undefined behaviour, which could potentially cause hard-to-diagnose bugs.

Besides, I want the mainline kernel to run on our simulator without
having to maintain patches for fixed issues.

Re: [PATCH V2] gpu: host1x: handle the correct # of syncpt regs

[Stephen Warren]

On 04/04/2014 04:31 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> BIT_WORD() truncates rather than rounds, so the loops in
> syncpt_thresh_isr() and _host1x_intr_disable_all_syncpt_intrs() use <=
> rather than < in an attempt to process the correct number of registers
> when rounding of the conversion of count of bits to count of words is
> necessary. However, when rounding isn't necessary because the value is
> already a multiple of the divisor (as is the case for all values of
> nb_pts the code actually sees), this causes one too many registers to
> be processed.
> 
> Solve this by using and explicit DIV_ROUND_UP() call, rather than
> BIT_WORD(), and comparing with < rather than <=.

I don't see this in linux-next yet.

Re: [PATCH V2] gpu: host1x: handle the correct # of syncpt regs

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 949 bytes --]

On Mon, Apr 14, 2014 at 02:53:51PM -0600, Stephen Warren wrote:
> On 04/04/2014 04:31 PM, Stephen Warren wrote:
> > From: Stephen Warren <swarren@nvidia.com>
> > 
> > BIT_WORD() truncates rather than rounds, so the loops in
> > syncpt_thresh_isr() and _host1x_intr_disable_all_syncpt_intrs() use <=
> > rather than < in an attempt to process the correct number of registers
> > when rounding of the conversion of count of bits to count of words is
> > necessary. However, when rounding isn't necessary because the value is
> > already a multiple of the divisor (as is the case for all values of
> > nb_pts the code actually sees), this causes one too many registers to
> > be processed.
> > 
> > Solve this by using and explicit DIV_ROUND_UP() call, rather than
> > BIT_WORD(), and comparing with < rather than <=.
> 
> I don't see this in linux-next yet.

I've queued this locally but haven't pushed anything out yet.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V2] gpu: host1x: handle the correct # of syncpt regs

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1066 bytes --]

On Mon, Apr 14, 2014 at 02:53:51PM -0600, Stephen Warren wrote:
> On 04/04/2014 04:31 PM, Stephen Warren wrote:
> > From: Stephen Warren <swarren@nvidia.com>
> > 
> > BIT_WORD() truncates rather than rounds, so the loops in
> > syncpt_thresh_isr() and _host1x_intr_disable_all_syncpt_intrs() use <=
> > rather than < in an attempt to process the correct number of registers
> > when rounding of the conversion of count of bits to count of words is
> > necessary. However, when rounding isn't necessary because the value is
> > already a multiple of the divisor (as is the case for all values of
> > nb_pts the code actually sees), this causes one too many registers to
> > be processed.
> > 
> > Solve this by using and explicit DIV_ROUND_UP() call, rather than
> > BIT_WORD(), and comparing with < rather than <=.
> 
> I don't see this in linux-next yet.

Just in case you haven't noticed, this this was merged in v3.15-rc2.
I've also Cc'ed stable so that it can be applied as far back as 3.10
when the code it fixes was introduced.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V2] gpu: host1x: handle the correct # of syncpt regs

[Terje Bergström]

On 05.04.2014 01:31, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> diff --git a/drivers/gpu/host1x/hw/intr_hw.c b/drivers/gpu/host1x/hw/intr_hw.c
> index db9017adfe2b..498b37e39058 100644
> --- a/drivers/gpu/host1x/hw/intr_hw.c
> +++ b/drivers/gpu/host1x/hw/intr_hw.c
> @@ -47,7 +47,7 @@ static irqreturn_t syncpt_thresh_isr(int irq, void *dev_id)
>  	unsigned long reg;
>  	int i, id;
>  
> -	for (i = 0; i <= BIT_WORD(host->info->nb_pts); i++) {
> +	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) {
> @@ -64,7 +64,7 @@ static void _host1x_intr_disable_all_syncpt_intrs(struct host1x *host)
>  {
>  	u32 i;
>  
> -	for (i = 0; i <= BIT_WORD(host->info->nb_pts); ++i) {
> +	for (i = 0; i < DIV_ROUND_UP(host->info->nb_pts, 32); ++i) {
>  		host1x_sync_writel(host, 0xffffffffu,
>  			HOST1X_SYNC_SYNCPT_THRESH_INT_DISABLE(i));
>  		host1x_sync_writel(host, 0xffffffffu,
> 

Acked-By: Terje Bergstrom <tbergstrom@nvidia.com>

Terje