Page MenuHomeFreeBSD

efifb: add a tunable to select the framebuffer cache attribute
ClosedPublic

Authored by kevans on Nov 7 2018, 9:50 AM.
Tags
None
Referenced Files
F81969640: D17884.diff
Sat, Dec 14, 7:51 AM
Unknown Object (File)
Sun, Nov 17, 7:38 PM
Unknown Object (File)
Apr 7 2024, 1:51 AM
Unknown Object (File)
Apr 2 2024, 12:44 AM
Unknown Object (File)
Mar 30 2024, 7:52 AM
Unknown Object (File)
Dec 20 2023, 1:33 AM
Unknown Object (File)
Nov 30 2023, 3:26 PM
Unknown Object (File)
Nov 14 2023, 10:01 PM

Details

Summary

Mapping the framebuffer with WC (Write Combined) memory type can, in
practice, cause some memory transactions to be rate-limited at a
fraction of the fb write rate. WC allows one core to queue up many
globally visible write transactions, and in the process some unrelated
transactions may end up having to wait for all of the queued up PCI
writes to be flushed.

Add an hw.efifb.cache_attr tunable to allow mapping the framebuffer as
uncacheable instead. We should likely be taking a more careful approach
of checking the memory map to determine which cacheability attributes
are feasible, but the knob lets us use our historically functional
behavior while offering a convenient way to switch on a stock kernel.

The only valid values for hw.efifb.cache_attr at this time are "uc" and
"wc".

Original patch by Marc De La Gueronniere <mdelagueronniere@verisign.com>

MFC after: 1 week
Sponsored by: Verisign, Inc.
Sponsored by: Klara, Inc.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 20678
Build 20093: arc lint + arc unit

Event Timeline

I think what we want to do is have a loader tunable to control this. We probably want to default to WC still, but a loader tunable makes it easy to toggle this for cases that need UC instead.

I agree with John here. It's likely OK, but there's been little to no testing, nor any presentation of benchmark results to show this is a good fix, nor comparisons to what others, eg Linux, do here.
I'd like to see this be at least a loader tunable.

In D17884#869536, @imp wrote:

I agree with John here. It's likely OK, but there's been little to no testing, nor any presentation of benchmark results to show this is a good fix, nor comparisons to what others, eg Linux, do here.
I'd like to see this be at least a loader tunable.

I actually think both this patch and the patched code are technically wrong. Linux searches for a matching segment in the memory map and checks the cacheability attributes on it before determining that it can do either WC/UC in the first place, which... actually makes a lot of sense.

We can probably shelve that for now and add a tunable, though I note that "little to no testing" isn't really accurate -- Verisign's had this deployed for a number of years at this point with no problem, though I don't really have a good idea of how diverse their hardware set is.

In D17884#869536, @imp wrote:

I agree with John here. It's likely OK, but there's been little to no testing, nor any presentation of benchmark results to show this is a good fix, nor comparisons to what others, eg Linux, do here.
I'd like to see this be at least a loader tunable.

I actually think both this patch and the patched code are technically wrong. Linux searches for a matching segment in the memory map and checks the cacheability attributes on it before determining that it can do either WC/UC in the first place, which... actually makes a lot of sense.

Indeed, that makes a lot more sense...

We can probably shelve that for now and add a tunable, though I note that "little to no testing" isn't really accurate -- Verisign's had this deployed for a number of years at this point with no problem, though I don't really have a good idea of how diverse their hardware set is.

Fair points, both... And re-reading it, it sounds crankier than I'd wanted it to sound... I just meant to say no characterization of what hardware that this was needed for, or what it worked with was offered.

kevans retitled this revision from Mapping the EFI frame buffer with the WriteCombined memory type can cause packet loss to efifb: add a tunable to select the framebuffer cache attribute.
kevans edited the summary of this revision. (Show Details)

Switch to a hw.efifb.cache_attr tunable to select between UNCACHEABLE and WRITE_COMBINING, note that we're probably subtly wrong and have room for future improvement.

As you say, this is imperfect, but it does move the state of the hard forward.

This revision is now accepted and ready to land.Feb 28 2023, 3:36 AM