Den 15.03.2017 13.39, skrev Daniel Vetter:
On Wed, Mar 15, 2017 at 01:15:37PM +0100, Noralf Trønnes wrote:
Den 14.03.2017 08.17, skrev Daniel Vetter:
On Mon, Mar 13, 2017 at 01:30:40PM +0100, Noralf Trønnes wrote:
Den 12.03.2017 19.00, skrev Daniel Vetter:
On Sat, Mar 11, 2017 at 10:35:32PM +0100, Noralf Trønnes wrote:
Add tinydrm_rgb565_buf_copy() function that copies buffer rectangle to destination buffer and also handles XRGB8888 and byte swap conversions. Useful for displays that only support RGB565 and can do partial updates.
Signed-off-by: Noralf Trønnes noralf@tronnes.org
drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 56 +++++++++++++++++++++++++- include/drm/tinydrm/tinydrm-helpers.h | 2 + 2 files changed, 56 insertions(+), 2 deletions(-)diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c index d4cda33..e639453 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c @@ -7,13 +7,15 @@ * (at your option) any later version. */ -#include <drm/tinydrm/tinydrm.h> -#include <drm/tinydrm/tinydrm-helpers.h> #include <linux/backlight.h> +#include <linux/dma-buf.h> #include <linux/pm.h> #include <linux/spi/spi.h> #include <linux/swab.h> +#include <drm/tinydrm/tinydrm.h> +#include <drm/tinydrm/tinydrm-helpers.h>
- static unsigned int spi_max; module_param(spi_max, uint, 0400); MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size");
@@ -181,6 +183,56 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr, EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
So I noticed that we already have the xrgb8888 to rgb565 function, so I'm a bit late on this, but: DRM doesn't do format conversions, with the single exception that the legacy cursor interface is specced to be argb8888.
Imo this should be removed (and preferrably before we ship tinydrm in a stable kernel). Why did you add it?
I added it from the start because plymouth can only do xrgb8888 and I thought that this was probably the format that most apps/libs/tools supported, ensuring that tinydrm would work with everything. But I was aware that this was the kernel patching up userspace, so I knew that it might be shot down.
But after your comment, I thought that this was in the clear: https://lists.freedesktop.org/archives/dri-devel/2017-January/130551.html
+EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
I wonder whether the above would make sense in drm core as some kind of fb helpers. But we can do that once there's a clear need.
I can make a patch that removes this format conversion.
I have no idea what I thought back then :-) But most likely I slightly misread it as argb8888_to_rgb565 (it works the same really) used for cursor compat, which is ok-ish.
But then I just looked through all drivers, and I found exactly one driver which doesn't support XRGB8888, and that was probably an oversight. So there's some arguments for always supporting that. Otoh if you do buffer sharing and have a nice hw spi engine, touching a wc buffer with the cpu is _real_ slow (because of the uncached reads), so we really shouldn't let userspace stumble over this pitfall by accident. The trouble is that by exposing both, most userspace will pick XRGB8888, even when they support RGB565.
And uncached reads are as a rule of thumb 1000x slower, so you don't need a big panel to feel the pain.
Given that I think we should remove the fake XRGB8888 support.
The Raspberry Pi, which is by far the largest user of these displays, has a DMA capable SPI controller that can only do 8-bit words. Since it is little endian, 16-bit RGB565 has to be byte swapped using a ordinary kmalloc buffer.
I always get endianess wrong, but why do we need to swap if the controller shuffles 8bit blocks around? If the panel is big endian, then we just need to use big endian drm_fourcc (they exist).
But does userspace use it? A quick google search for DRM_FORMAT_BIG_ENDIAN yielded only weston which drops the field when looking at the format in libweston/gl-renderer.c.
In fact 3 months ago you said it was broken: http://lists.infradead.org/pipermail/linux-arm-kernel/2016-December/473034.h...
Trying to fix up everything is probably going to be lots of work, but assuming that everything is broken for big endian is probably correct. -Daniel
Theoretical maximum at 48MHz is: 1 / (320 * 240 * 16 * (1/48000000.0)) = 39.0625Hz
The actual numbers isn't very far off:
$ modetest -M "mi0283qt" -s 25:320x240@RG16 -v setting mode 320x240-0Hz@RG16 on connectors 25, crtc 27 freq: 39.36Hz freq: 31.16Hz freq: 31.21Hz
$ modetest -M "mi0283qt" -s 25:320x240@XR24 -v setting mode 320x240-0Hz@XR24 on connectors 25, crtc 27 freq: 37.30Hz freq: 29.76Hz freq: 29.84Hz
Disabling byte swapping, passing cma_obj->vaddr through to SPI, doesn't improve the numbers much:
$ modetest -M "mi0283qt" -s 25:320x240@RG16 -v setting mode 320x240-0Hz@RG16 on connectors 25, crtc 27 freq: 43.90Hz freq: 33.49Hz freq: 33.49Hz
The SPI bus is sooo slow that the cpu can jump through all kinds of hoops without affecting throughput much.
Hey, 4 more frames! But in either case, you'll probably get much faster if you offload the upload work to an async worker. Converting ->dirty to use atomic might help a lot here, since we could try to stall only to the previous frame, and not be entirely synchronous.
The next step wrt tinydrm and performance is to get it working with PRIME especially for games. I have only done a test with a hacked modetest, but failed to get X working with vc4 as the renderer. https://github.com/anholt/linux/issues/10
Wrt plymouth, I'm a bit surprised that one falls over: We have a pile of chips that (in some circumstances) prefer RGB565 (mostly big resolutions with little vram), that's controlled by the preferred_bpp parameter of drm_fb_helper_single_fb_probe(). You seem to have wired that up all correctly, plymouth still fails?
I tried to get plymouth to work on the Raspberry Pi, but gave up. I couldn't get it to use the display. But here's an extract from the plymouth code:
create_output_buffer():
if (drmModeAddFB (backend->device_fd, width, height, 24, 32, buffer->row_stride, buffer->handle, &buffer->id) != 0) {bpp=32 and depth=24 -> DRM_FORMAT_XRGB8888
And the has_32bpp_support() function makes it clear that 32bpp is required.
https://cgit.freedesktop.org/plymouth/tree/src/plugins/renderers/drm/plugin....
Blergh. Oh well, I guess we should just accept that userspace developers are lazy :(
So the conversion can stay? :-)
Noralf.