[PATCH 1/5] drm/tinydrm: Add tinydrm_rgb565_buf_copy()
Daniel Vetter
daniel at ffwll.ch
Wed Mar 15 16:38:00 UTC 2017
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 at 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.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 at RG16 -v
> > > setting mode 320x240-0Hz at RG16 on connectors 25, crtc 27
> > > freq: 39.36Hz
> > > freq: 31.16Hz
> > > freq: 31.21Hz
> > >
> > > $ modetest -M "mi0283qt" -s 25:320x240 at XR24 -v
> > > setting mode 320x240-0Hz at 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 at RG16 -v
> > > setting mode 320x240-0Hz at 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
More information about the dri-devel
mailing list