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

[PATCH] 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 (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 BITS_TO_LONGS() (which rounds internally), rather
than BIT_WORD(), and comparing with < rather than <=.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 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..17407b2de2bf 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 < BITS_TO_LONGS(host->info->nb_pts); 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 < BITS_TO_LONGS(host->info->nb_pts); ++i) {
 		host1x_sync_writel(host, 0xffffffffu,
 			HOST1X_SYNC_SYNCPT_THRESH_INT_DISABLE(i));
 		host1x_sync_writel(host, 0xffffffffu,
-- 
1.8.1.5

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

[Terje Bergström]

On 01.04.2014 23:10, Stephen Warren wrote:
> diff --git a/drivers/gpu/host1x/hw/intr_hw.c b/drivers/gpu/host1x/hw/intr_hw.c
> index db9017adfe2b..17407b2de2bf 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 < BITS_TO_LONGS(host->info->nb_pts); 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 < BITS_TO_LONGS(host->info->nb_pts); ++i) {
>  		host1x_sync_writel(host, 0xffffffffu,
>  			HOST1X_SYNC_SYNCPT_THRESH_INT_DISABLE(i));
>  		host1x_sync_writel(host, 0xffffffffu,
> 

Wouldn't this break in architectures with 64-bit longs? Probably the
safest way would be to use DIV_ROUND_UP(host->info->nb_pts, 32), because
we know there are 32 bits in each host1x register.

Terje

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

[Thierry Reding]

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

On Thu, Apr 03, 2014 at 11:09:22AM +0300, Terje Bergström wrote:
> On 01.04.2014 23:10, Stephen Warren wrote:
> > diff --git a/drivers/gpu/host1x/hw/intr_hw.c b/drivers/gpu/host1x/hw/intr_hw.c
> > index db9017adfe2b..17407b2de2bf 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 < BITS_TO_LONGS(host->info->nb_pts); 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 < BITS_TO_LONGS(host->info->nb_pts); ++i) {
> >  		host1x_sync_writel(host, 0xffffffffu,
> >  			HOST1X_SYNC_SYNCPT_THRESH_INT_DISABLE(i));
> >  		host1x_sync_writel(host, 0xffffffffu,
> > 
> 
> Wouldn't this break in architectures with 64-bit longs?

Well, BIT_WORD() relies on BITS_PER_LONG too, so the current code is
broken for 64-bit architectures too.

> Probably the safest way would be to use DIV_ROUND_UP(host->info->nb_pts, 32),
> because we know there are 32 bits in each host1x register.

I agree. I hope there aren't any plans on making the registers 64 bits
wide in the future. Although they'd probably still be accessible as 32
bit values even then.

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