tridentfb: fix hang on Blade3D with CONFIG_CC_OPTIMIZE_FOR_SIZE

PATCH RESEND In review Ondrej Zary 2 participants 12 messages Aug 24, 2015 – Sep 24, 2015

Subsystems (2)

Framebuffer layer Framebuffer core

lore

Share & tools

lore

Ondrej Zary

author

2015-08-24 19:37 UTC #0 lore raw

PATCH RESEND 1/4 tridentfb: fix hang on Blade3D with CONFIG_CC_OPTIMIZE_FOR_SIZE

When the kernel is compiled with -Os (CONFIG_CC_OPTIMIZE_FOR_SIZE), tridentfb hangs the machine upon load with Blade3D cards unless acceleration is disabled.

This is caused by memcpy() which copies data byte-by-byte (rep movsb) when compiled with -Os. The card does not like that - it requires 32-bit access.

Use iowrite_32() instead.

Signed-off-by Ondrej Zary <[email protected]>

Acked-by Krzysztof Helt <[email protected]>

---
 drivers/video/fbdev/tridentfb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/video/fbdev/tridentfb.c +1 -1

index 7ed9a22..7429713 100644
@@ -226,7 +226,7 @@ static void blade_image_blit(struct tridentfb_par *par, const char *data,
 	writemmr(par, DST1, point(x, y));
 	writemmr(par, DST2, point(x + w - 1, y + h - 1));
 
-	memcpy(par->io_virt + 0x10000, data, 4 * size);
+	iowrite32_rep(par->io_virt + 0x10000, data, size);
 }
 
 static void blade_copy_rect(struct tridentfb_par *par,

Signature · 1 line

Ondrej Zary

Ondrej Zary

author

2015-08-24 19:37 UTC #1 lore raw

PATCH RESEND 2/4 tridentfb: Fix set_lwidth on TGUI9440 and CYBER9320

According to X.Org driver, chips older than TGUI9660 have only 1 width bit in AddColReg. Touching the 2nd one causes I2C/DDC to fail on TGUI9440.

Set only 1 bit of width in AddColReg on TGUI9440 and CYBER9320.

Signed-off-by Ondrej Zary <[email protected]>

---
 drivers/video/fbdev/tridentfb.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/video/fbdev/tridentfb.c +8 -2

index 7429713..01b43e9 100644
@@ -673,8 +673,14 @@ static int get_nativex(struct tridentfb_par *par)
 static inline void set_lwidth(struct tridentfb_par *par, int width)
 {
 	write3X4(par, VGA_CRTC_OFFSET, width & 0xFF);
-	write3X4(par, AddColReg,
-		 (read3X4(par, AddColReg) & 0xCF) | ((width & 0x300) >> 4));
+	/* chips older than TGUI9660 have only 1 width bit in AddColReg */
+	/* touching the other one breaks I2C/DDC */
+	if (par->chip_id = TGUI9440 || par->chip_id = CYBER9320)
+		write3X4(par, AddColReg,
+		     (read3X4(par, AddColReg) & 0xEF) | ((width & 0x100) >> 4));
+	else
+		write3X4(par, AddColReg,
+		     (read3X4(par, AddColReg) & 0xCF) | ((width & 0x300) >> 4));
 }
 
 /* For resolutions smaller than FP resolution stretch */

Signature · 1 line

Ondrej Zary

Tomi Valkeinen

2015-09-24 11:07 UTC #2 lore raw

Re: [PATCH 2/4] [resend #2] tridentfb: Fix set_lwidth on TGUI9440 and CYBER9320

Quoted 22 lines · Ondrej Zary

According to X.Org driver, chips older than TGUI9660 have only 1 width bit in AddColReg. Touching the 2nd one causes I2C/DDC to fail on TGUI9440.
Set only 1 bit of width in AddColReg on TGUI9440 and CYBER9320.
Signed-off-by Ondrej Zary <[email protected]>
---
 drivers/video/fbdev/tridentfb.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/video/fbdev/tridentfb.c +1 -2
index 7429713..01b43e9 100644
@@ -673,8 +673,14 @@ static int get_nativex(struct tridentfb_par *par)
 static inline void set_lwidth(struct tridentfb_par *par, int width)
 {
 	write3X4(par, VGA_CRTC_OFFSET, width & 0xFF);
-	write3X4(par, AddColReg,
-		 (read3X4(par, AddColReg) & 0xCF) | ((width & 0x300) >> 4));
+	/* chips older than TGUI9660 have only 1 width bit in AddColReg */

drivers/video/fbdev/tridentfb.c static int get_nativex(struct tridentfb_par *par) · line 686

Quoted 10 lines

drivers/video/fbdev/tridentfb.c +7 -0

+	/* touching the other one breaks I2C/DDC */
+	if (par->chip_id == TGUI9440 || par->chip_id == CYBER9320)
+		write3X4(par, AddColReg,
+		     (read3X4(par, AddColReg) & 0xEF) | ((width & 0x100) >> 4));
+	else
+		write3X4(par, AddColReg,
+		     (read3X4(par, AddColReg) & 0xCF) | ((width & 0x300) >> 4));
 }
 
 /* For resolutions smaller than FP resolution stretch */

Thanks, queued for 4.3 fixes.

Tomi

Ondrej Zary

author

2015-08-24 19:37 UTC #3 lore raw

PATCH RESEND 3/4 fb_ddc: Allow I2C adapters without SCL read capability

In reply to Ondrej Zary

i2c-algo-bit allows I2C adapters without SCL read capability to work but fb_ddc_read fails to work on them.

Fix fb_ddc_read to work with I2C adapters not capable of reading SCL.

Signed-off-by Ondrej Zary <[email protected]>

Acked-by Krzysztof Helt <[email protected]>

---
 drivers/video/fbdev/core/fb_ddc.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

drivers/video/fbdev/core/fb_ddc.c +5 -3

index 94322cc..22c694a 100644
@@ -69,10 +69,11 @@ unsigned char *fb_ddc_read(struct i2c_adapter *adapter)
 		algo_data->setscl(algo_data->data, 1);
 		for (j = 0; j < 5; j++) {
 			msleep(10);
-			if (algo_data->getscl(algo_data->data))
+			if (algo_data->getscl &&
+			    algo_data->getscl(algo_data->data))
 				break;
 		}
-		if (j = 5)
+		if (algo_data->getscl && j = 5)
 			continue;
 
 		algo_data->setsda(algo_data->data, 0);

@@ -91,7 +92,8 @@ unsigned char *fb_ddc_read(struct i2c_adapter *adapter)
 		algo_data->setscl(algo_data->data, 1);
 		for (j = 0; j < 10; j++) {
 			msleep(10);
-			if (algo_data->getscl(algo_data->data))
+			if (algo_data->getscl &&
+			    algo_data->getscl(algo_data->data))
 				break;
 		}

Signature · 1 line

Ondrej Zary

Tomi Valkeinen

2015-09-02 11:33 UTC #4 lore raw

Re: [PATCH 3/4] [resend #2] fb_ddc: Allow I2C adapters without SCL read capability

Quoted 30 lines · Ondrej Zary

i2c-algo-bit allows I2C adapters without SCL read capability to work but fb_ddc_read fails to work on them.
Fix fb_ddc_read to work with I2C adapters not capable of reading SCL.
Signed-off-by Ondrej Zary <[email protected]>
Acked-by Krzysztof Helt <[email protected]>
---
 drivers/video/fbdev/core/fb_ddc.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/video/fbdev/core/fb_ddc.c +3 -2
index 94322cc..22c694a 100644
@@ -69,10 +69,11 @@ unsigned char *fb_ddc_read(struct i2c_adapter *adapter)
 		algo_data->setscl(algo_data->data, 1);
 		for (j = 0; j < 5; j++) {
 			msleep(10);
-			if (algo_data->getscl(algo_data->data))
+			if (algo_data->getscl &&
+			    algo_data->getscl(algo_data->data))
 				break;
 		}
-		if (j == 5)
+		if (algo_data->getscl && j == 5)
 			continue;
 
 		algo_data->setsda(algo_data->data, 0);

drivers/video/fbdev/core/fb_ddc.c unsigned char *fb_ddc_read(struct i2c_adapter *adapter) · line 98

Quoted 8 lines

drivers/video/fbdev/core/fb_ddc.c +2 -1

@@ -91,7 +92,8 @@ unsigned char *fb_ddc_read(struct i2c_adapter *adapter)
 		algo_data->setscl(algo_data->data, 1);
 		for (j = 0; j < 10; j++) {
 			msleep(10);
-			if (algo_data->getscl(algo_data->data))
+			if (algo_data->getscl &&
+			    algo_data->getscl(algo_data->data))
 				break;
 		}

Aren't both of those loops pointless if there's no getscl?

Tomi

Ondrej Zary

author

2015-09-02 12:04 UTC #5 lore raw

Re: [PATCH 3/4] [resend #2] fb_ddc: Allow I2C adapters without SCL read capability

Quoted 29 lines · Tomi Valkeinen

Quoted 28 lines · Ondrej Zary
i2c-algo-bit allows I2C adapters without SCL read capability to work but fb_ddc_read fails to work on them.
Fix fb_ddc_read to work with I2C adapters not capable of reading SCL.
Signed-off-by Ondrej Zary <[email protected]>
Acked-by Krzysztof Helt <[email protected]>
---
 drivers/video/fbdev/core/fb_ddc.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
(quoted diff) +0 -0
b/drivers/video/fbdev/core/fb_ddc.c index 94322cc..22c694a 100644
drivers/video/fbdev/core/fb_ddc.c +3 -2
@@ -69,10 +69,11 @@ unsigned char *fb_ddc_read(struct i2c_adapter
 *adapter) algo_data->setscl(algo_data->data, 1);
 		for (j = 0; j < 5; j++) {
 			msleep(10);
-			if (algo_data->getscl(algo_data->data))
+			if (algo_data->getscl &&
+			    algo_data->getscl(algo_data->data))
 				break;
 		}
-		if (j = 5)
+		if (algo_data->getscl && j = 5)
 			continue;
 

Quoted 12 lines

drivers/video/fbdev/core/fb_ddc.c unsigned char *fb_ddc_read(struct i2c_adapter *adapter) · line 98
Quoted 10 lines
drivers/video/fbdev/core/fb_ddc.c +2 -1
 		algo_data->setsda(algo_data->data, 0);
@@ -91,7 +92,8 @@ unsigned char *fb_ddc_read(struct i2c_adapter *adapter)
 		algo_data->setscl(algo_data->data, 1);
 		for (j = 0; j < 10; j++) {
 			msleep(10);
-			if (algo_data->getscl(algo_data->data))
+			if (algo_data->getscl &&
+			    algo_data->getscl(algo_data->data))
 				break;
 		}
Aren't both of those loops pointless if there's no getscl?

They're reduced to delays - don't know how much critical they are.

Signature · 1 line

Ondrej Zary

Tomi Valkeinen

2015-09-24 11:04 UTC #6 lore raw

Re: [PATCH 3/4] [resend #2] fb_ddc: Allow I2C adapters without SCL read capability

Quoted 43 lines · Ondrej Zary

Quoted 29 lines · Tomi Valkeinen
Quoted 28 lines · Ondrej Zary
i2c-algo-bit allows I2C adapters without SCL read capability to work but fb_ddc_read fails to work on them.
Fix fb_ddc_read to work with I2C adapters not capable of reading SCL.
Signed-off-by Ondrej Zary <[email protected]>
Acked-by Krzysztof Helt <[email protected]>
---
 drivers/video/fbdev/core/fb_ddc.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
(quoted diff) +0 -0
b/drivers/video/fbdev/core/fb_ddc.c index 94322cc..22c694a 100644
drivers/video/fbdev/core/fb_ddc.c +3 -2
@@ -69,10 +69,11 @@ unsigned char *fb_ddc_read(struct i2c_adapter
 *adapter) algo_data->setscl(algo_data->data, 1);
 		for (j = 0; j < 5; j++) {
 			msleep(10);
-			if (algo_data->getscl(algo_data->data))
+			if (algo_data->getscl &&
+			    algo_data->getscl(algo_data->data))
 				break;
 		}
-		if (j == 5)
+		if (algo_data->getscl && j == 5)
 			continue;
 
Quoted 12 lines
drivers/video/fbdev/core/fb_ddc.c unsigned char *fb_ddc_read(struct i2c_adapter *adapter) · line 98
Quoted 10 lines
drivers/video/fbdev/core/fb_ddc.c +2 -1
 		algo_data->setsda(algo_data->data, 0);
@@ -91,7 +92,8 @@ unsigned char *fb_ddc_read(struct i2c_adapter *adapter)
 		algo_data->setscl(algo_data->data, 1);
 		for (j = 0; j < 10; j++) {
 			msleep(10);
-			if (algo_data->getscl(algo_data->data))
+			if (algo_data->getscl &&
+			    algo_data->getscl(algo_data->data))
 				break;
 		}
Aren't both of those loops pointless if there's no getscl?

Quoted 1 line

They're reduced to delays - don't know how much critical they are.

I think the code gets somewhat confusing the way you did it.

Maybe instead something like:

if (algo_data->getscl) {
	the current loop & delays here;
} else {
	msleep(25); // 25 was just a guess...
}

Tomi

Ondrej Zary

author

2015-08-24 19:37 UTC #7 lore raw

PATCH RESEND 4/4 tridentfb: Add DDC support

In reply to Ondrej Zary

Add DDC support for Trident cards.

Tested on TGUI9440, TGUI9680, 3DImage 9750, Blade3D 9880 and Blade XP.

Signed-off-by Ondrej Zary <[email protected]>

---
 drivers/video/fbdev/Kconfig     |    9 ++
 drivers/video/fbdev/tridentfb.c |  192 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 196 insertions(+), 5 deletions(-)

drivers/video/fbdev/Kconfig +9 -0

index 2d98de5..e2531b8 100644
@@ -1680,6 +1680,15 @@ config FB_TRIDENT
 	  To compile this driver as a module, choose M here: the
 	  module will be called tridentfb.
 
+config FB_TRIDENT_DDC
+	bool "DDC for Trident support"
+	depends on FB_TRIDENT
+	select FB_DDC
+	select FB_MODE_HELPERS
+	default y
+	help
+	  Say Y here if you want DDC support for your Trident graphics card.
+
 config FB_ARK
 	tristate "ARK 2000PV support"
 	depends on FB && PCI

drivers/video/fbdev/tridentfb.c +187 -5

247 lines not on this page · raw message

Signature · 1 line

Ondrej Zary

Tomi Valkeinen

2015-09-02 11:30 UTC #8 lore raw

Re: [PATCH 4/4] [resend #2] tridentfb: Add DDC support

Hi,

Quoted 17 lines · Ondrej Zary

Add DDC support for Trident cards.
Tested on TGUI9440, TGUI9680, 3DImage 9750, Blade3D 9880 and Blade XP.
Signed-off-by Ondrej Zary <[email protected]>
---
 drivers/video/fbdev/Kconfig     |    9 ++
 drivers/video/fbdev/tridentfb.c |  192 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 196 insertions(+), 5 deletions(-)
drivers/video/fbdev/Kconfig +0 -0
index 2d98de5..e2531b8 100644
@@ -1680,6 +1680,15 @@ config FB_TRIDENT
 	  To compile this driver as a module, choose M here: the
 	  module will be called tridentfb.

drivers/video/fbdev/Kconfig config FB_TRIDENT · line 1691

Quoted 10 lines

drivers/video/fbdev/Kconfig +9 -0

 
+config FB_TRIDENT_DDC
+	bool "DDC for Trident support"
+	depends on FB_TRIDENT
+	select FB_DDC
+	select FB_MODE_HELPERS
+	default y
+	help
+	  Say Y here if you want DDC support for your Trident graphics card.
+

Why would somebody not want this enabled? Is there some drawback if it's enabled?

Tomi

Ondrej Zary

author

2015-09-02 12:04 UTC #9 lore raw

Re: [PATCH 4/4] [resend #2] tridentfb: Add DDC support

Quoted 22 lines · Tomi Valkeinen

Hi,
Quoted 19 lines · Ondrej Zary
Add DDC support for Trident cards.
Tested on TGUI9440, TGUI9680, 3DImage 9750, Blade3D 9880 and Blade XP.
Signed-off-by Ondrej Zary <[email protected]>
---
 drivers/video/fbdev/Kconfig     |    9 ++
 drivers/video/fbdev/tridentfb.c |  192
++++++++++++++++++++++++++++++++++++++- 2 files changed, 196 insertions(+), 5 deletions(-)
drivers/video/fbdev/Kconfig +0 -0
index 2d98de5..e2531b8 100644
@@ -1680,6 +1680,15 @@ config FB_TRIDENT
 	  To compile this driver as a module, choose M here: the
 	  module will be called tridentfb.
 

Quoted 12 lines

drivers/video/fbdev/Kconfig config FB_TRIDENT · line 1691
Quoted 9 lines
drivers/video/fbdev/Kconfig +9 -0
+config FB_TRIDENT_DDC
+	bool "DDC for Trident support"
+	depends on FB_TRIDENT
+	select FB_DDC
+	select FB_MODE_HELPERS
+	default y
+	help
+	  Say Y here if you want DDC support for your Trident graphics card.
+
Why would somebody not want this enabled? Is there some drawback if it's enabled?

It's probably an useless config option but many fbdev drivers have it. I have no problem removing it.

Signature · 1 line

Ondrej Zary

Tomi Valkeinen

2015-09-24 11:00 UTC #10 lore raw

Re: [PATCH 4/4] [resend #2] tridentfb: Add DDC support

Quoted 13 lines · Ondrej Zary

Quoted 4 lines
Quoted 4 lines
drivers/video/fbdev/Kconfig +4 -0
+config FB_TRIDENT_DDC
+	bool "DDC for Trident support"
+	depends on FB_TRIDENT
+	select FB_DDC
Quoted 8 lines
drivers/video/fbdev/Kconfig config FB_TRIDENT · line 1691
Quoted 5 lines
drivers/video/fbdev/Kconfig +5 -0
+	select FB_MODE_HELPERS
+	default y
+	help
+	  Say Y here if you want DDC support for your Trident graphics card.
+
Why would somebody not want this enabled? Is there some drawback if it's enabled?

Quoted 2 lines

It's probably an useless config option but many fbdev drivers have it. I have no problem removing it.

Yes, I'd rather see it removed, if no one sees any use for the option.

Tomi

Tomi Valkeinen

2015-09-24 11:06 UTC #11 lore raw

In reply to Ondrej Zary

Quoted 20 lines · Ondrej Zary

When the kernel is compiled with -Os (CONFIG_CC_OPTIMIZE_FOR_SIZE), tridentfb hangs the machine upon load with Blade3D cards unless acceleration is disabled.
This is caused by memcpy() which copies data byte-by-byte (rep movsb) when compiled with -Os. The card does not like that - it requires 32-bit access.
Use iowrite_32() instead.
Signed-off-by Ondrej Zary <[email protected]>
Acked-by Krzysztof Helt <[email protected]>
---
 drivers/video/fbdev/tridentfb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/video/fbdev/tridentfb.c +0 -0
index 7ed9a22..7429713 100644

drivers/video/fbdev/tridentfb.c static void blade_image_blit(struct tridentfb_par *par, const char *data, · line 232

Quoted 8 lines

drivers/video/fbdev/tridentfb.c +1 -1

@@ -226,7 +226,7 @@ static void blade_image_blit(struct tridentfb_par *par, const char *data,
 	writemmr(par, DST1, point(x, y));
 	writemmr(par, DST2, point(x + w - 1, y + h - 1));
 
-	memcpy(par->io_virt + 0x10000, data, 4 * size);
+	iowrite32_rep(par->io_virt + 0x10000, data, size);
 }
 
 static void blade_copy_rect(struct tridentfb_par *par,

Thanks, queued for 4.3 fixes.

Tomi