Patchwork [v2] offb: make the screen properties endian safe

login
register
mail settings
Submitter Cédric Le Goater
Date Oct. 31, 2013, 9:36 a.m.
Message ID <1383212192-3622-1-git-send-email-clg@fr.ibm.com>
Download mbox | patch
Permalink /patch/287441/
State Changes Requested
Headers show

Comments

Cédric Le Goater - Oct. 31, 2013, 9:36 a.m.
The "screen" properties : depth, width, height, linebytes need
to be converted to the host endian order when read from the device
tree.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---

Changes in v2:

 - replaced be32_to_cpu() by be32_to_cpup() 
 - fixed setcolreg ops 

 drivers/video/offb.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Benjamin Herrenschmidt - Nov. 20, 2013, 10:50 p.m.
On Thu, 2013-10-31 at 10:36 +0100, Cédric Le Goater wrote:
> The "screen" properties : depth, width, height, linebytes need
> to be converted to the host endian order when read from the device
> tree.
> 
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---

Did you actually test that ? IE, using emulated VGA in qemu for
example ?

I'm asking because there are a few interesting nits here...

 - fbdev *generally* assume native endian framebuffer, but of course
under qemu today, the adapter will use a big endian frame buffer
aperture. You can compile in support for foreign endian but I don't know
how that actually works.

 - The setcolreg fix ... the "value" is used 2 lines above your endian
swap, is that correct ?

Cheers
Ben.


> Changes in v2:
> 
>  - replaced be32_to_cpu() by be32_to_cpup() 
>  - fixed setcolreg ops 
> 
>  drivers/video/offb.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/video/offb.c b/drivers/video/offb.c
> index 0c4f343..68e8415 100644
> --- a/drivers/video/offb.c
> +++ b/drivers/video/offb.c
> @@ -120,7 +120,7 @@ static int offb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
>  			mask <<= info->var.transp.offset;
>  			value |= mask;
>  		}
> -		pal[regno] = value;
> +		pal[regno] = cpu_to_be32(value);
>  		return 0;
>  	}
>  
> @@ -536,7 +536,7 @@ static void __init offb_init_nodriver(struct device_node *dp, int no_real_node)
>  	unsigned int flags, rsize, addr_prop = 0;
>  	unsigned long max_size = 0;
>  	u64 rstart, address = OF_BAD_ADDR;
> -	const u32 *pp, *addrp, *up;
> +	const __be32 *pp, *addrp, *up;
>  	u64 asize;
>  	int foreign_endian = 0;
>  
> @@ -552,25 +552,25 @@ static void __init offb_init_nodriver(struct device_node *dp, int no_real_node)
>  	if (pp == NULL)
>  		pp = of_get_property(dp, "depth", &len);
>  	if (pp && len == sizeof(u32))
> -		depth = *pp;
> +		depth = be32_to_cpup(pp);
>  
>  	pp = of_get_property(dp, "linux,bootx-width", &len);
>  	if (pp == NULL)
>  		pp = of_get_property(dp, "width", &len);
>  	if (pp && len == sizeof(u32))
> -		width = *pp;
> +		width = be32_to_cpup(pp);
>  
>  	pp = of_get_property(dp, "linux,bootx-height", &len);
>  	if (pp == NULL)
>  		pp = of_get_property(dp, "height", &len);
>  	if (pp && len == sizeof(u32))
> -		height = *pp;
> +		height = be32_to_cpup(pp);
>  
>  	pp = of_get_property(dp, "linux,bootx-linebytes", &len);
>  	if (pp == NULL)
>  		pp = of_get_property(dp, "linebytes", &len);
>  	if (pp && len == sizeof(u32) && (*pp != 0xffffffffu))
> -		pitch = *pp;
> +		pitch = be32_to_cpup(pp);
>  	else
>  		pitch = width * ((depth + 7) / 8);
>
Cédric Le Goater - Nov. 21, 2013, 3:45 p.m.
Hi,

On 11/20/2013 11:50 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2013-10-31 at 10:36 +0100, Cédric Le Goater wrote:
>> The "screen" properties : depth, width, height, linebytes need
>> to be converted to the host endian order when read from the device
>> tree.
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> ---
> 
> Did you actually test that ? IE, using emulated VGA in qemu for
> example ?

Yes, that's how I did. I ran LE and BE guests with "qemu -vga std" and 
different depths.

> I'm asking because there are a few interesting nits here...
> 
>  - fbdev *generally* assume native endian framebuffer, but of course
> under qemu today, the adapter will use a big endian frame buffer
> aperture. You can compile in support for foreign endian but I don't know
> how that actually works.

OK. I will see how I can extend the tests. But, are you suggesting I should
be using the foreign endian framework for the frame buffer ? 
 
>  - The setcolreg fix ... the "value" is used 2 lines above your endian
> swap, is that correct ?

The variables used to calculate "value" are in host endian order. It should
be fine.

Thanks,

C.

 
> Cheers
> Ben.
> 
> 
>> Changes in v2:
>>
>>  - replaced be32_to_cpu() by be32_to_cpup() 
>>  - fixed setcolreg ops 
>>
>>  drivers/video/offb.c |   12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/video/offb.c b/drivers/video/offb.c
>> index 0c4f343..68e8415 100644
>> --- a/drivers/video/offb.c
>> +++ b/drivers/video/offb.c
>> @@ -120,7 +120,7 @@ static int offb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
>>  			mask <<= info->var.transp.offset;
>>  			value |= mask;
>>  		}
>> -		pal[regno] = value;
>> +		pal[regno] = cpu_to_be32(value);
>>  		return 0;
>>  	}
>>  
>> @@ -536,7 +536,7 @@ static void __init offb_init_nodriver(struct device_node *dp, int no_real_node)
>>  	unsigned int flags, rsize, addr_prop = 0;
>>  	unsigned long max_size = 0;
>>  	u64 rstart, address = OF_BAD_ADDR;
>> -	const u32 *pp, *addrp, *up;
>> +	const __be32 *pp, *addrp, *up;
>>  	u64 asize;
>>  	int foreign_endian = 0;
>>  
>> @@ -552,25 +552,25 @@ static void __init offb_init_nodriver(struct device_node *dp, int no_real_node)
>>  	if (pp == NULL)
>>  		pp = of_get_property(dp, "depth", &len);
>>  	if (pp && len == sizeof(u32))
>> -		depth = *pp;
>> +		depth = be32_to_cpup(pp);
>>  
>>  	pp = of_get_property(dp, "linux,bootx-width", &len);
>>  	if (pp == NULL)
>>  		pp = of_get_property(dp, "width", &len);
>>  	if (pp && len == sizeof(u32))
>> -		width = *pp;
>> +		width = be32_to_cpup(pp);
>>  
>>  	pp = of_get_property(dp, "linux,bootx-height", &len);
>>  	if (pp == NULL)
>>  		pp = of_get_property(dp, "height", &len);
>>  	if (pp && len == sizeof(u32))
>> -		height = *pp;
>> +		height = be32_to_cpup(pp);
>>  
>>  	pp = of_get_property(dp, "linux,bootx-linebytes", &len);
>>  	if (pp == NULL)
>>  		pp = of_get_property(dp, "linebytes", &len);
>>  	if (pp && len == sizeof(u32) && (*pp != 0xffffffffu))
>> -		pitch = *pp;
>> +		pitch = be32_to_cpup(pp);
>>  	else
>>  		pitch = width * ((depth + 7) / 8);
>>  
> 
>
Benjamin Herrenschmidt - Nov. 21, 2013, 7:57 p.m.
On Thu, 2013-11-21 at 16:45 +0100, Cedric Le Goater wrote:
> >  - fbdev *generally* assume native endian framebuffer, but of course
> > under qemu today, the adapter will use a big endian frame buffer
> > aperture. You can compile in support for foreign endian but I don't know
> > how that actually works.
> 
> OK. I will see how I can extend the tests. But, are you suggesting I should
> be using the foreign endian framework for the frame buffer ? 

Well, if it works ... did you try 16 and 32bpp ?

Cheers,
Ben.
Cédric Le Goater - Nov. 23, 2013, 5:38 p.m.
On 11/21/2013 08:57 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2013-11-21 at 16:45 +0100, Cedric Le Goater wrote:
>>>  - fbdev *generally* assume native endian framebuffer, but of course
>>> under qemu today, the adapter will use a big endian frame buffer
>>> aperture. You can compile in support for foreign endian but I don't know
>>> how that actually works.
>>
>> OK. I will see how I can extend the tests. But, are you suggesting I should
>> be using the foreign endian framework for the frame buffer ? 
> 
> Well, if it works ... did you try 16 and 32bpp ?

So, 32bpp "works" but 16 is broken ... I guess my palette fix is just a lucky
hack and I need to dig deeper in fb code to have a better understanding of
the color map. 

I should have provided you two patches in the first place. Do you want the 
device tree data fixes for the frame buffer screen properties ? It helps to 
have a display for little endian guests even if the colors are wrong.

Thanks,

C.
Benjamin Herrenschmidt - Nov. 23, 2013, 9:04 p.m.
On Sat, 2013-11-23 at 18:38 +0100, Cedric Le Goater wrote:
> So, 32bpp "works" but 16 is broken ... I guess my palette fix is just a lucky
> hack and I need to dig deeper in fb code to have a better understanding of
> the color map. 
> 
> I should have provided you two patches in the first place. Do you want the 
> device tree data fixes for the frame buffer screen properties ? It helps to 
> have a display for little endian guests even if the colors are wrong.

Before you pull your hair out, check if it works in BE :-)

Cheers,
Ben.
Cédric Le Goater - Dec. 4, 2013, 4:47 p.m.
On 11/23/2013 10:04 PM, Benjamin Herrenschmidt wrote:
> On Sat, 2013-11-23 at 18:38 +0100, Cedric Le Goater wrote:
>> So, 32bpp "works" but 16 is broken ... I guess my palette fix is just a lucky
>> hack and I need to dig deeper in fb code to have a better understanding of
>> the color map. 
>>
>> I should have provided you two patches in the first place. Do you want the 
>> device tree data fixes for the frame buffer screen properties ? It helps to 
>> have a display for little endian guests even if the colors are wrong.
> 
> Before you pull your hair out, check if it works in BE :-)

I spent some more time on this topic and I have a few more patches to 
send you.

The first patch is straight forward. It fixes host endian order issues 
in the offb code when accessing the OF device tree properties. 

The following patch is more of an attempt to fix the pseudo-palette 
entries on little endian. Hopefully this is the good direction. If not, 
I still have some hair to pull out :)

Cheers,

C.

Patch

diff --git a/drivers/video/offb.c b/drivers/video/offb.c
index 0c4f343..68e8415 100644
--- a/drivers/video/offb.c
+++ b/drivers/video/offb.c
@@ -120,7 +120,7 @@  static int offb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
 			mask <<= info->var.transp.offset;
 			value |= mask;
 		}
-		pal[regno] = value;
+		pal[regno] = cpu_to_be32(value);
 		return 0;
 	}
 
@@ -536,7 +536,7 @@  static void __init offb_init_nodriver(struct device_node *dp, int no_real_node)
 	unsigned int flags, rsize, addr_prop = 0;
 	unsigned long max_size = 0;
 	u64 rstart, address = OF_BAD_ADDR;
-	const u32 *pp, *addrp, *up;
+	const __be32 *pp, *addrp, *up;
 	u64 asize;
 	int foreign_endian = 0;
 
@@ -552,25 +552,25 @@  static void __init offb_init_nodriver(struct device_node *dp, int no_real_node)
 	if (pp == NULL)
 		pp = of_get_property(dp, "depth", &len);
 	if (pp && len == sizeof(u32))
-		depth = *pp;
+		depth = be32_to_cpup(pp);
 
 	pp = of_get_property(dp, "linux,bootx-width", &len);
 	if (pp == NULL)
 		pp = of_get_property(dp, "width", &len);
 	if (pp && len == sizeof(u32))
-		width = *pp;
+		width = be32_to_cpup(pp);
 
 	pp = of_get_property(dp, "linux,bootx-height", &len);
 	if (pp == NULL)
 		pp = of_get_property(dp, "height", &len);
 	if (pp && len == sizeof(u32))
-		height = *pp;
+		height = be32_to_cpup(pp);
 
 	pp = of_get_property(dp, "linux,bootx-linebytes", &len);
 	if (pp == NULL)
 		pp = of_get_property(dp, "linebytes", &len);
 	if (pp && len == sizeof(u32) && (*pp != 0xffffffffu))
-		pitch = *pp;
+		pitch = be32_to_cpup(pp);
 	else
 		pitch = width * ((depth + 7) / 8);