Re: [m5-dev] Review Request: ARM: Implement a CLCD Frame buffer

2010-11-09 Thread Gabe Black 

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/294/#review457
---


I assume this is a step towards VNC accessible video output. There are some 
issues, most importantly (for me) the integration of the generic bitmap stuff. 
This would correspond to the integration of generic VNC stuff eventually, and 
that wouldn't do me any good with x86. There are lots of style bugs that need 
to be fixed.


src/dev/arm/RealView.py
http://reviews.m5sim.org/r/294/#comment666

It seems pointless to align this Param with the one below it but none of 
the others. I think aligning them generally is pointless, so you might just 
pull this one in.



src/dev/arm/RealView.py
http://reviews.m5sim.org/r/294/#comment668

Inconsistent alignment



src/dev/arm/RealView.py
http://reviews.m5sim.org/r/294/#comment667

Inconsistent alignment again.



src/dev/arm/amba_device.cc
http://reviews.m5sim.org/r/294/#comment669

Since this occurs twice and is consistent, it should probably be a constant 
with a name. If it's only meaninful locally it would be fine to leave it in the 
.cc.



src/dev/arm/pl111.hh
http://reviews.m5sim.org/r/294/#comment670

I'd like these to be an enum, but more I'd really like these to not look 
like macros. LCD_TIMING0 = LcdTiming0, etc.



src/dev/arm/pl111.hh
http://reviews.m5sim.org/r/294/#comment671

These are types, not values. They should not use all caps.



src/dev/arm/pl111.hh
http://reviews.m5sim.org/r/294/#comment672

You need spaces around the operators.



src/dev/arm/pl111.hh
http://reviews.m5sim.org/r/294/#comment673

ditto



src/dev/arm/pl111.hh
http://reviews.m5sim.org/r/294/#comment674

If this is a generic utility class (and I think it is) it should go in some 
other file for others to use. Maybe in base. You have an errant space after 
class it looks like.



src/dev/arm/pl111.hh
http://reviews.m5sim.org/r/294/#comment675

The formatting of this comment seems fairly mangled. It also looks copied 
from somewhere. What did this come from originally? Do we have license to use 
it?



src/dev/arm/pl111.cc
http://reviews.m5sim.org/r/294/#comment676

These offsets seem to be basically sequential, less their 4 byte size. 
Perhaps this would be better implemented as an array with an index instead of a 
switch?



src/dev/arm/pl111.cc
http://reviews.m5sim.org/r/294/#comment677

Forgive my laziness, but is this normally how DMAs are done? It seems 
fairly manual. Don't we have a base class for this?



src/dev/arm/pl111.cc
http://reviews.m5sim.org/r/294/#comment678

space before {, not after dmaSize. I won't mark all the style problems, but 
there are others.



src/dev/arm/pl111.cc
http://reviews.m5sim.org/r/294/#comment679

{ goes on the line with the if, space after the if, space after the ==.



src/dev/arm/pl111.cc
http://reviews.m5sim.org/r/294/#comment680

tear, not tare. Double buffering doesn't really make sense here since we 
don't have asynchronous screen updates. Just send out the bitmap whenever it's 
done. As a matter of fact that looks like what's already happening. You're 
doing a memcpy and then just immediately using the memcpy. Nothing's going to 
happen to the original in the mean time.



src/dev/arm/pl111.cc
http://reviews.m5sim.org/r/294/#comment681

This needs endianness handling. Also, if the framebuffer doesn't -mean- 
anything as 32 bit integers, ie its not interpretted, just shuttled in from 
writes, out with reads, and dumped to a file, why not just store it as chars in 
the first place?


- Gabe


On 2010-11-08 15:37:49, Ali Saidi wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviews.m5sim.org/r/294/
 ---
 
 (Updated 2010-11-08 15:37:49)
 
 
 Review request for Default.
 
 
 Summary
 ---
 
 ARM: Implement a CLCD Frame buffer
 
 
 Diffs
 -
 
   src/dev/arm/RealView.py f61e079ad05e 
   src/dev/arm/SConscript f61e079ad05e 
   src/dev/arm/amba_device.hh f61e079ad05e 
   src/dev/arm/amba_device.cc f61e079ad05e 
   src/dev/arm/pl111.hh PRE-CREATION 
   src/dev/arm/pl111.cc PRE-CREATION 
 
 Diff: http://reviews.m5sim.org/r/294/diff
 
 
 Testing
 ---
 
 
 Thanks,
 
 Ali
 


___
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev

[m5-dev] Review Request: ARM: Implement a CLCD Frame buffer

2010-11-08 Thread Ali Saidi 

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/294/
---

Review request for Default.


Summary
---

ARM: Implement a CLCD Frame buffer


Diffs
-

  src/dev/arm/RealView.py f61e079ad05e 
  src/dev/arm/SConscript f61e079ad05e 
  src/dev/arm/amba_device.hh f61e079ad05e 
  src/dev/arm/amba_device.cc f61e079ad05e 
  src/dev/arm/pl111.hh PRE-CREATION 
  src/dev/arm/pl111.cc PRE-CREATION 

Diff: http://reviews.m5sim.org/r/294/diff


Testing
---


Thanks,

Ali

___
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev

[m5-dev] Review Request: ARM: Implement a CLCD Frame buffer

2010-09-16 Thread Ali Saidi 

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/246/
---

Review request for Default.


Summary
---

ARM: Implement a CLCD Frame buffer


Diffs
-

  src/dev/arm/RealView.py 37c56be05af0 
  src/dev/arm/SConscript 37c56be05af0 
  src/dev/arm/amba_device.hh 37c56be05af0 
  src/dev/arm/amba_device.cc 37c56be05af0 
  src/dev/arm/pl111.hh PRE-CREATION 
  src/dev/arm/pl111.cc PRE-CREATION 

Diff: http://reviews.m5sim.org/r/246/diff


Testing
---


Thanks,

Ali

___
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev

Re: [m5-dev] Review Request: ARM: Implement a CLCD Frame buffer

2010-09-16 Thread Nathan Binkert 

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/246/#review349
---


Overall, this diff looks fine.  There are numerous little style violations (all 
of which I did not point out).  Many, many lines also seem to exceed 80 columns.


src/dev/arm/amba_device.hh
http://reviews.m5sim.org/r/246/#comment463

Sort #includes



src/dev/arm/amba_device.cc
http://reviews.m5sim.org/r/246/#comment462

full #include path please.  Sort includes.



src/dev/arm/pl111.hh
http://reviews.m5sim.org/r/246/#comment449

Should be _HH__



src/dev/arm/pl111.hh
http://reviews.m5sim.org/r/246/#comment448

Please sort #includes and have system #include separate and at the top



src/dev/arm/pl111.hh
http://reviews.m5sim.org/r/246/#comment450

Brace on new line.



src/dev/arm/pl111.hh
http://reviews.m5sim.org/r/246/#comment451

Brace on new line.

I'd personally prefer to see a separate declaration of Header header; 
somewhere else, though I won't be stubborn.



src/dev/arm/pl111.hh
http://reviews.m5sim.org/r/246/#comment460

Would uint8_t make more sense here?



src/dev/arm/pl111.hh
http://reviews.m5sim.org/r/246/#comment458

Brace on new line.



src/dev/arm/pl111.hh
http://reviews.m5sim.org/r/246/#comment457

If you're going to keep the header here, there should probably be a space 
after the brace to be consistent with other code. (though I'd rather see the 
header somewhere else.



src/dev/arm/pl111.hh
http://reviews.m5sim.org/r/246/#comment456

brace on new line.



src/dev/arm/pl111.hh
http://reviews.m5sim.org/r/246/#comment459

Brace on new line.



src/dev/arm/pl111.hh
http://reviews.m5sim.org/r/246/#comment461

uint8_t?



src/dev/arm/pl111.cc
http://reviews.m5sim.org/r/246/#comment469

space after comma.



src/dev/arm/pl111.cc
http://reviews.m5sim.org/r/246/#comment452

Space after for.



src/dev/arm/pl111.cc
http://reviews.m5sim.org/r/246/#comment453

Space after for



src/dev/arm/pl111.cc
http://reviews.m5sim.org/r/246/#comment454

space after if, no space after (, space before brace



src/dev/arm/pl111.cc
http://reviews.m5sim.org/r/246/#comment455

space after switch



src/dev/arm/pl111.cc
http://reviews.m5sim.org/r/246/#comment466

I personally like this style, but to be consistent, else should be on line 
with brace.



src/dev/arm/pl111.cc
http://reviews.m5sim.org/r/246/#comment467

and here



src/dev/arm/pl111.cc
http://reviews.m5sim.org/r/246/#comment468

and here
and space before brace



src/dev/arm/pl111.cc
http://reviews.m5sim.org/r/246/#comment465

else should be on line with brace.



src/dev/arm/pl111.cc
http://reviews.m5sim.org/r/246/#comment464

space after if, space before brace.


- Nathan


On 2010-09-16 00:47:54, Ali Saidi wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://reviews.m5sim.org/r/246/
 ---
 
 (Updated 2010-09-16 00:47:54)
 
 
 Review request for Default.
 
 
 Summary
 ---
 
 ARM: Implement a CLCD Frame buffer
 
 
 Diffs
 -
 
   src/dev/arm/RealView.py 37c56be05af0 
   src/dev/arm/SConscript 37c56be05af0 
   src/dev/arm/amba_device.hh 37c56be05af0 
   src/dev/arm/amba_device.cc 37c56be05af0 
   src/dev/arm/pl111.hh PRE-CREATION 
   src/dev/arm/pl111.cc PRE-CREATION 
 
 Diff: http://reviews.m5sim.org/r/246/diff
 
 
 Testing
 ---
 
 
 Thanks,
 
 Ali
 


___
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev