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@xxxxxxxxxxx>
---
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.html
> 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.c
Blergh. Oh well, I guess we should just accept that userspace developers
are lazy :(
So the conversion can stay? :-)
Noralf.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
