almindor / mipidsi

MIPI Display Serial Interface unified driver
MIT License
125 stars 48 forks source link

Fill solid optimizations per-color #146

Open almindor opened 1 week ago

almindor commented 1 week ago

NOTE: this is an improvement on #145 using "specialization" workaround. This PR does not need any Model changes.

This adds optimized (per color type) fill_solid paths as part of improving on issue https://github.com/almindor/mipidsi/issues/142.

This improved fill_solid fullscreen render speed on a ST7789 using 80Mhz SPI interface with esp32-c6 board running at 160Mhz CPU speed from ~48ms to ~18ms.

NOTE: the buffer size is arbitrarily chosen, bigger ones can get even better results, but I wanted to avoid using too much memory. If batch support is disabled these paths are not used. I hope we can come up with a better story for buffers overall in the future (e.g. wrt. to display-interface as well as this driver).

almindor commented 1 week ago

NOTE: I'd love to make the BUFFER_SIZE constant a generic param but it becomes quite inwieldy in mipidsi. I think we want to move that into display-interface and in fact, allow usage of &mut [u8] buffer provided by the user.x

This way you would instantiate display-interface with a buffer, possibly DMA configured, and both DI and Mipidsi would then use that buffer as needed.

almindor commented 1 week ago

@rfuest I've implemented your suggestions, lmk if this looks good now. Do you know how soon is the next e-g going to be released? It might make sense to wait and simplify this before we go 0.9 but only if it's short term.

rfuest commented 1 week ago

Do you know how soon is the next e-g going to be released? It might make sense to wait and simplify this before we go 0.9 but only if it's short term.

No, I don't know when I'll get around to finishing the new e-g version.

almindor commented 1 week ago

I just noticed that we'll have the same problem with this as we have with set_pixels_from_buffer:

https://github.com/almindor/mipidsi/blob/33a53887daef820395b1cd292eedb2d7621c0bcb/mipidsi/src/lib.rs#L276-L279

I don't have a good solution for this, except for documenting it. Our new internal endianness setting makes the entire display-interface problems even more confusing.

Yes, the DI needs reworking. It's hard to figure out what to do with it though, it seems between e-g, drivers (e.g. mipidsi) and the DI there's a bit of a leaky placement problem of things such as endianness (both storage format and expectations of the display), buffer use and blit optimizations.

Most of the issues seem to boil down to "losing information due to abstractions" in the end (e.g. DI not knowing about continuous pixels but being the place where buffers should probably be kept and used).

Right now though the DI is more of a problem than a helper, it even implicitly does a 64 pixel wide buffer (so not even bytes but depends on format) no matter what if anything but DataFormat::U8 is used.

One odd finding I had was that dynamic dispatch does not seem to impact performance much, I tried @GrantM11235's static dispatch approach with these changes and it actually made the performance worse.

But this all is a bigger discussion, I just don't know what the best place for it would be.

almindor commented 1 week ago

@rfuest should be good to go