Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Himbaechel GUI #1295

Merged
merged 10 commits into from
Nov 21, 2024
Merged

Himbaechel GUI #1295

merged 10 commits into from
Nov 21, 2024

Conversation

mmicko
Copy link
Member

@mmicko mmicko commented Feb 29, 2024

This extends Himbaechel API with additional methods for drawing bels, wires and pips. There is also configuration attribute to control if some of these are used and also do change in coordinate system (y axis).

Was mostly mimic what we do on ECP5 and XO2/3, and was not sure about equivalent for tile_wire (mapped to GfxTileWireId) so used flags for now, since it anyway keeps some additional custom info.

First few commits are mostly cleanup and fixes of some old standing GUI issues.

Would add at least basic things in Xilinx and Gowin when we agree on API level. Please do comment and suggest changes.

@@ -1,5 +1,5 @@
#!/usr/bin/env bash
set -ex
yosys -p "synth_xilinx -flatten -abc9 -nobram -arch xc7 -top top; write_json blinky.json" blinky.v
nextpnr-himbaechel --device xc7a35tcsg324-1 -o xdc=arty.xdc --json blinky.json -o fasm=blinky.fasm --router router2
../../../../../nextpnr-himbaechel --device xc7a35tcsg324-1 -o xdc=arty.xdc --json blinky.json -o fasm=blinky.fasm --router router2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this breaks if someone does an out of tree build

// Graphics
virtual uint32_t gfxAttributes() { return 0; }

virtual void gfxTileBel(std::vector<GraphicElement> &g, int x, int y, int z, int w, int h,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you perhaps think of some slightly more descriptive names for these API functions, and also add some documentation about them, so I can review them better?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, will update these, but probably not today

@@ -102,6 +109,20 @@ struct HimbaechelAPI
virtual bool isClusterStrict(const CellInfo *cell) const;
virtual bool getClusterPlacement(ClusterId cluster, BelId root_bel,
std::vector<std::pair<CellInfo *, BelId>> &placement) const;

// Graphics
virtual uint32_t gfxAttributes() { return 0; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like this function, can you think of a better way here, like perhaps a struct that the architecture configures?

@mmicko
Copy link
Member Author

mmicko commented Mar 19, 2024

@gatecat Documentation is not added yet, but main question I have is if using tilewire as additional data is fine (need to bump db version in that case as well) ? not sure if there is other way we can actually use to determine which wire to draw.


NEXTPNR_NAMESPACE_BEGIN

enum GfxTileWireId
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will need to be in a uarch-specific namespace, otherwise there will be conflicts

@@ -857,3 +865,18 @@ def write_bba(self, filename):
bba.ref('chip_info')
self.serialise(bba)
bba.pop()

def read_gfx_h(self, filename):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is kind of horrid

@@ -144,6 +144,7 @@ class TileWireData:
index: int
name: IdString
wire_type: IdString
tile_wire: int
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be given a name that makes it's very clear that it's gui-specific, otherwise it will be very unclear to anyone looking at the structures

@stacksmith
Copy link

Is this close to merging? I am pretty desperate to see things, and nextpnr-gowin --gui just crashes all the time now.

@@ -229,9 +232,13 @@ def add_bel_pin(self, bel: BelData, pin: str, wire: str, dir: PinType):

def create_wire(self, name: str, type: str="", const_value: str=""):
# Create a new tile wire of a given name and type (optional) in the tile type
gfx_wire_id = 0
if (name) in self.gfx_wire_ids:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are the brackets here for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, that is leftover from past. removing it now

@mmicko mmicko marked this pull request as ready for review November 21, 2024 13:38
@@ -166,6 +167,7 @@ def serialise_lists(self, context: str, bba: BBAWriter):
def serialise(self, context: str, bba: BBAWriter):
bba.u32(self.name.index)
bba.u32(self.wire_type.index)
bba.u32(self.gfx_wire_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a breaking change for existing binaries so it needs a db version bump:

static constexpr int database_version = 4;

and

bba.u32(4) # version

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to go to 5, 4 is the current version on main

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok to rebase it now, so it is easier then to spot things like this ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds like a good plan!

@gatecat gatecat merged commit 5503546 into master Nov 21, 2024
16 checks passed
@gatecat gatecat deleted the micko/himbaechel_gui branch November 21, 2024 14:13
@gatecat
Copy link
Member

gatecat commented Nov 21, 2024

Thanks, sorry for the delay in getting this merged!

@mmicko
Copy link
Member Author

mmicko commented Nov 21, 2024

Thanks and no worries, was busy with other things anyway.

KelvinChung2000 pushed a commit to KelvinChung2000/nextpnr that referenced this pull request Nov 25, 2024
* Extend Himbaechel API with gfx drawing methods

* Add bel drawing in example uarch

* changed API and added tile wire id in db

* extend API so we can distinguish CLK wires

* added bit more wires

* less horrid way of handling gfx ids

* loop wire range

* removed not needed brackets

* bump database version to 5

* Removed not used GfxFlags
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants