Paul Menzel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38647 )
Change subject: drivers/intel/gma/edid: Use byte swap function ......................................................................
drivers/intel/gma/edid: Use byte swap function
Instead of open coding the byte swapping (endianess), use the built-in.
The compiler can use the instruction `bswap` now. As EDID data is not big, the difference in execution time won’t be measurable.
With gcc (Debian 9.2.1-25) 9.2.1 20200123:
.LBE128: .LBE127: - .loc 1 94 3 is_stmt 1 discriminator 3 view .LVU157 - addl $4, %ebp - .loc 1 94 15 is_stmt 0 discriminator 3 view .LVU158 - movb %al, -4(%ebp) - .loc 1 95 3 is_stmt 1 discriminator 3 view .LVU159 - .loc 1 96 28 is_stmt 0 discriminator 3 view .LVU160 - movl %eax, %edx - .loc 1 95 19 discriminator 3 view .LVU161 - movb %ah, -3(%ebp) - .loc 1 96 3 is_stmt 1 discriminator 3 view .LVU162 - .loc 1 96 28 is_stmt 0 discriminator 3 view .LVU163 - shrl $16, %edx - .loc 1 96 19 discriminator 3 view .LVU164 - movb %dl, -2(%ebp) - .loc 1 97 3 is_stmt 1 discriminator 3 view .LVU165 - .loc 1 97 35 is_stmt 0 discriminator 3 view .LVU166 - shrl $24, %eax + .loc 1 96 3 is_stmt 1 discriminator 3 view .LVU159 + .loc 1 96 15 is_stmt 0 discriminator 3 view .LVU160 + movl 76(%esp), %edx + .loc 1 96 17 discriminator 3 view .LVU161 + bswap %eax
Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836 Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M src/drivers/intel/gma/edid.c 1 file changed, 3 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38647/1
diff --git a/src/drivers/intel/gma/edid.c b/src/drivers/intel/gma/edid.c index 6885f98..5f1cfe8 100644 --- a/src/drivers/intel/gma/edid.c +++ b/src/drivers/intel/gma/edid.c @@ -15,6 +15,7 @@ #include <device/mmio.h> #include <console/console.h> #include <delay.h> +#include <swab.h> #include <timer.h>
#include "i915_reg.h" @@ -91,10 +92,8 @@ u32 reg32; wait_rdy(mmio); reg32 = read32(GMBUS3_ADDR); - edid[4 * i] = reg32 & 0xff; - edid[4 * i + 1] = (reg32 >> 8) & 0xff; - edid[4 * i + 2] = (reg32 >> 16) & 0xff; - edid[4 * i + 3] = (reg32 >> 24) & 0xff; + /* Writes 4 * i to (4 * i) + 3 */ + edid[4 * i] = swab32(reg32); } wait_rdy(mmio); write32(GMBUS1_ADDR, GMBUS_SW_RDY
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )
Change subject: drivers/intel/gma/edid: Use byte swap function ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... File src/drivers/intel/gma/edid.c:
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... PS1, Line 94: reg32 This variable shouldn't be necessary anymore.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )
Change subject: drivers/intel/gma/edid: Use byte swap function ......................................................................
Patch Set 1: Code-Review-1
That looks wrong. edid is just 1 byte, so you drop 3 bytes on implicitly casting it.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )
Change subject: drivers/intel/gma/edid: Use byte swap function ......................................................................
Patch Set 1:
(1 comment)
Patch Set 1: Code-Review-1
That looks wrong. edid is just 1 byte, so you drop 3 bytes on implicitly casting it.
What do you mean?
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... File src/drivers/intel/gma/edid.c:
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... PS1, Line 94: reg32
This variable shouldn't be necessary anymore.
Do you mean `swab32(read32(GMBUS3_ADDR)`?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )
Change subject: drivers/intel/gma/edid: Use byte swap function ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... File src/drivers/intel/gma/edid.c:
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... PS1, Line 96: edid[4 * i] = swab32(reg32); I’d think, that all four bytes are written. But I still need to test it.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )
Change subject: drivers/intel/gma/edid: Use byte swap function ......................................................................
Patch Set 1: Code-Review-1
(3 comments)
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... File src/drivers/intel/gma/edid.c:
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... PS1, Line 94: reg32
Do you mean `swab32(read32(GMBUS3_ADDR)`?
Yes, but I think it won't work.
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... PS1, Line 96: edid[4 * i] = swab32(reg32);
I’d think, that all four bytes are written. But I still need to test it.
Well, if you looked at the disassembly right before this, can't you just check if the code will write the 32-bit value or will just cast it?
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... PS1, Line 96: edid Call me cursed, buuut... What if you do a pointer cast and use an u32* here? This would make the byteswap unnecessary.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )
Change subject: drivers/intel/gma/edid: Use byte swap function ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... File src/drivers/intel/gma/edid.c:
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... PS1, Line 96: edid
Call me cursed, buuut... […]
I am not so good in assembly. Can you take a look please?
``` .LBE128: .LBE127: - .loc 1 94 3 is_stmt 1 discriminator 3 view .LVU157 - addl $4, %ebp - .loc 1 94 15 is_stmt 0 discriminator 3 view .LVU158 - movb %al, -4(%ebp) - .loc 1 95 3 is_stmt 1 discriminator 3 view .LVU159 - .loc 1 96 28 is_stmt 0 discriminator 3 view .LVU160 - movl %eax, %edx - .loc 1 95 19 discriminator 3 view .LVU161 - movb %ah, -3(%ebp) - .loc 1 96 3 is_stmt 1 discriminator 3 view .LVU162 - .loc 1 96 28 is_stmt 0 discriminator 3 view .LVU163 - shrl $16, %edx - .loc 1 96 19 discriminator 3 view .LVU164 - movb %dl, -2(%ebp) - .loc 1 97 3 is_stmt 1 discriminator 3 view .LVU165 - .loc 1 97 35 is_stmt 0 discriminator 3 view .LVU166 - shrl $24, %eax + .loc 1 96 3 is_stmt 1 discriminator 3 view .LVU159 + .loc 1 96 15 is_stmt 0 discriminator 3 view .LVU160 + movl 76(%esp), %edx + .loc 1 96 17 discriminator 3 view .LVU161 + bswap %eax .LVL45: - .loc 1 97 19 discriminator 3 view .LVU167 - movb %al, -1(%ebp) + .loc 1 96 15 discriminator 3 view .LVU162 + movb %al, (%edx,%ebp,4) ```
I guess the `movb` is the interesting part?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )
Change subject: drivers/intel/gma/edid: Use byte swap function ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... File src/drivers/intel/gma/edid.c:
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... PS1, Line 96: edid
I am not so good in assembly. Can you take a look please? […]
Indeed, this won't work.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )
Change subject: drivers/intel/gma/edid: Use byte swap function ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... File src/drivers/intel/gma/edid.c:
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... PS1, Line 94: edid[4 * i] = reg32 & 0xff; : edid[4 * i + 1] = (reg32 >> 8) & 0xff; : edid[4 * i + 2] = (reg32 >> 16) & 0xff; : edid[4 * i + 3] = (reg32 >> 24) & 0xff; This looks like little-endian encoding, and we are in drivers/intel/. I don't think there is anything to swap. It's also hard to argue about endianness here, as this is just copying data from hardware to a stream (just leaving the bytes in the order they were found should work).
I think the best way to express this would be to use a host-specific read and write:
((uint32_t *)edid)[i] = read32(GMBUS3_ADDR);
or
write32(edid + 4 * i, read32(GMBUS3_ADDR));
The former doesn't look too nice, the latter would (unnecessarily) treat the RAM where `edid` resides like MMIO...
Or maybe just use memcpy()? ;)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )
Change subject: drivers/intel/gma/edid: Use byte swap function ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... File src/drivers/intel/gma/edid.c:
https://review.coreboot.org/c/coreboot/+/38647/1/src/drivers/intel/gma/edid.... PS1, Line 96: edid
Call me cursed, buuut... What if you do a pointer cast and use an u32* here? This would make the byteswap unnecessary.
If the type is char or void, then aliasing to a different type does NOT result in nasal demons (undefined behavior, after which anything could happen like demons coming out of everyone's noses). So, it's an option.
Hello Patrick Rudolph, Angel Pons, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38647
to look at the new patch set (#2).
Change subject: drivers/intel/gma/edid: Directly assign 32-bit value ......................................................................
drivers/intel/gma/edid: Directly assign 32-bit value
Instead of manually assigning one byte at a time, write the four bytes at once.
With gcc (Debian 9.2.1-25) 9.2.1 20200123 the assembly differences below are made, showing that some instructions are saved.
movl 12(%esi), %eax .LVL44: - .loc 2 34 9 discriminator 3 view .LVU156 -.LBE128: -.LBE127: - .loc 1 94 3 is_stmt 1 discriminator 3 view .LVU157 - addl $4, %ebp - .loc 1 94 15 is_stmt 0 discriminator 3 view .LVU158 - movb %al, -4(%ebp) - .loc 1 95 3 is_stmt 1 discriminator 3 view .LVU159 - .loc 1 96 28 is_stmt 0 discriminator 3 view .LVU160 - movl %eax, %edx - .loc 1 95 19 discriminator 3 view .LVU161 - movb %ah, -3(%ebp) - .loc 1 96 3 is_stmt 1 discriminator 3 view .LVU162 - .loc 1 96 28 is_stmt 0 discriminator 3 view .LVU163 - shrl $16, %edx - .loc 1 96 19 discriminator 3 view .LVU164 - movb %dl, -2(%ebp) - .loc 1 97 3 is_stmt 1 discriminator 3 view .LVU165 - .loc 1 97 35 is_stmt 0 discriminator 3 view .LVU166 - shrl $24, %eax -.LVL45: - .loc 1 97 19 discriminator 3 view .LVU167 - movb %al, -1(%ebp) -.LBE129: - .loc 1 90 33 is_stmt 1 discriminator 3 view .LVU168 + .loc 2 34 9 discriminator 3 view .LVU155 + addl $16, %edi +.LBE125: +.LBE124: + .loc 1 94 29 discriminator 3 view .LVU156 + movl %eax, -16(%edi) + .loc 1 91 33 is_stmt 1 discriminator 3 view .LVU157 jmp .L12
Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836 Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M src/drivers/intel/gma/edid.c 1 file changed, 3 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38647/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )
Change subject: drivers/intel/gma/edid: Directly assign 32-bit value ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38647/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38647/2//COMMIT_MSG@13 PS2, Line 13: are made, showing that some instructions are saved. Why not use the reference toolchain?
https://review.coreboot.org/c/coreboot/+/38647/2//COMMIT_MSG@49 PS2, Line 49: jmp .L12 The diff doesn't provide much value, imho. If you want to keep it, it would be much more readable with the comments removed, just `objdump -d` should do it.
https://review.coreboot.org/c/coreboot/+/38647/2/src/drivers/intel/gma/edid.... File src/drivers/intel/gma/edid.c:
https://review.coreboot.org/c/coreboot/+/38647/2/src/drivers/intel/gma/edid.... PS2, Line 18: #include <swab.h> Not used anymore.
https://review.coreboot.org/c/coreboot/+/38647/2/src/drivers/intel/gma/edid.... PS2, Line 93: /* Only works on LE systems, writes 4 * i to (4 * i) + 3 */ It's much more complicated. I would have said the old code only works with LE.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38647 )
Change subject: drivers/intel/gma/edid: Directly assign 32-bit value ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38647/3/src/drivers/intel/gma/edid.... File src/drivers/intel/gma/edid.c:
https://review.coreboot.org/c/coreboot/+/38647/3/src/drivers/intel/gma/edid.... PS3, Line 82: edid ((uint32_t*)edid)[i] ?
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38647
to look at the new patch set (#4).
Change subject: drivers/intel/gma/edid: Directly assign 32-bit value ......................................................................
drivers/intel/gma/edid: Directly assign 32-bit value
Instead of manually assigning one byte at a time, write the four bytes at once.
With gcc (Debian 9.2.1-25) 9.2.1 20200123 the assembly differences below are made, showing that some instructions are saved.
movl 12(%esi), %eax .LVL44: - .loc 2 34 9 discriminator 3 view .LVU156 -.LBE128: -.LBE127: - .loc 1 94 3 is_stmt 1 discriminator 3 view .LVU157 - addl $4, %ebp - .loc 1 94 15 is_stmt 0 discriminator 3 view .LVU158 - movb %al, -4(%ebp) - .loc 1 95 3 is_stmt 1 discriminator 3 view .LVU159 - .loc 1 96 28 is_stmt 0 discriminator 3 view .LVU160 - movl %eax, %edx - .loc 1 95 19 discriminator 3 view .LVU161 - movb %ah, -3(%ebp) - .loc 1 96 3 is_stmt 1 discriminator 3 view .LVU162 - .loc 1 96 28 is_stmt 0 discriminator 3 view .LVU163 - shrl $16, %edx - .loc 1 96 19 discriminator 3 view .LVU164 - movb %dl, -2(%ebp) - .loc 1 97 3 is_stmt 1 discriminator 3 view .LVU165 - .loc 1 97 35 is_stmt 0 discriminator 3 view .LVU166 - shrl $24, %eax -.LVL45: - .loc 1 97 19 discriminator 3 view .LVU167 - movb %al, -1(%ebp) -.LBE129: - .loc 1 90 33 is_stmt 1 discriminator 3 view .LVU168 + .loc 2 34 9 discriminator 3 view .LVU155 + addl $16, %edi +.LBE125: +.LBE124: + .loc 1 94 29 discriminator 3 view .LVU156 + movl %eax, -16(%edi) + .loc 1 91 33 is_stmt 1 discriminator 3 view .LVU157 jmp .L12
Change-Id: I818318361a83b2b3f0c6d8637dd3b6917590c836 Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M src/drivers/intel/gma/edid.c 1 file changed, 3 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38647/4
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38647?usp=email )
Change subject: drivers/intel/gma/edid: Directly assign 32-bit value ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Angel Pons (Code Review) -
Martin L Roth (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)