On Wed, Mar 15, 2017 at 04:14:49PM +0100, Noralf Trønnes wrote: > > 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 Just looking for victims to fix up the entire mess :-) > > 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? :-) Yes. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
