Skip to content

Increase firmware call buffer size to 48 bytes#2738

Merged
pelwell merged 1 commit intoraspberrypi:rpi-4.19.yfrom
JamesH65:rpi-4.19.y
Oct 31, 2018
Merged

Increase firmware call buffer size to 48 bytes#2738
pelwell merged 1 commit intoraspberrypi:rpi-4.19.yfrom
JamesH65:rpi-4.19.y

Conversation

@JamesH65
Copy link
Copy Markdown
Contributor

An assumption was made in commit a1547e0 that 32 bytes
would be enough data buffer size for all firmware calls. However,
the axi performance monitor driver uses a call with 44 bytes
(RPI_FIRMWARE_GET_PERIPH_REG) to get the VC registers values.

Increase value to 48 to take this in to account.

Signed-off-by: James Hughes james.hughes@raspberrypi.org

An assumption was made in commit a1547e0 that 32 bytes
would be enough data buffer size for all firmware calls. However,
the axi performance monitor driver uses a call with 44 bytes
(RPI_FIRMWARE_GET_PERIPH_REG) to get the VC registers values.

Increase value to 48 to take this in to account.

Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
@JamesH65
Copy link
Copy Markdown
Contributor Author

@lategoodbye What's the appropriate was to push this upstream? Assuming we are all happy with it here.

@pelwell pelwell merged commit 0297830 into raspberrypi:rpi-4.19.y Oct 31, 2018
@popcornmix
Copy link
Copy Markdown
Collaborator

I guess we'd ideally like this upstream.
@lategoodbye do you think this would be accepted without a driver being upstream that requires it?

@lategoodbye
Copy link
Copy Markdown
Contributor

Let me say, you would get my ack for this change. But i cannot speak for the other kernel hackers. Maybe it would be better to go through to all firmware requests tags and verify that 44 is sufficient for all of them. This had the advantage we wouldn't need to argue about downstream drivers like axi perfmon.

@JamesH65
Copy link
Copy Markdown
Contributor Author

JamesH65 commented Nov 1, 2018

I've had a quick look. It appears that there is one command that requires 136 bytes, get_edid. I have no idea if it is ever used.

@JamesH65
Copy link
Copy Markdown
Contributor Author

JamesH65 commented Nov 1, 2018

And another, one of the palette functions requires 1024bytes which I suspect the devs are not going to be happy having on the stack.

I reckon probably at least 4 or 5 firmware calls have been broken by this upstream change.

@lategoodbye
Copy link
Copy Markdown
Contributor

But the usage of VLAs was already broken before. Looks like this documentation isn't up to date, because it doesn't contain RPI_FIRMWARE_GET_PERIPH_REG.

Maybe we should define a threshold to decide between stack usage and allocation.
@anholt

@JamesH65
Copy link
Copy Markdown
Contributor Author

JamesH65 commented Nov 1, 2018

Althogh that doc is somewhat out of date, it does describe the palette call with 1024 byte length, which had that been considered would have been OK. Not sure how that was missed.

@anholt
Copy link
Copy Markdown
Contributor

anholt commented Nov 1, 2018

Threshold between stack and allocation would be fine, but honestly just allocating always would be fine, too. The next call does a dma_alloc_coherent, which is going to be more expensive than the kmalloc would be, I'm sure.

@anholt
Copy link
Copy Markdown
Contributor

anholt commented Nov 1, 2018

(Also, @JamesH65 -- any chance we could see a native AXI performance driver without the firmware involved?)

@JamesH65
Copy link
Copy Markdown
Contributor Author

JamesH65 commented Nov 2, 2018

Unfortunately, the VC4 AXI performance registers are not available on the ARM memory bus, hence I had to write this mailbox function to grab them (The call can read from other peripherals as well, but is currently limited to just the AXI perf ones).

@pelwell
Copy link
Copy Markdown
Contributor

pelwell commented Nov 2, 2018

Are they DMA-able?

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.

5 participants