Re: [PATCH 1/5] drm/tinydrm: Add tinydrm_rgb565_buf_copy()

[Daniel Vetter <daniel@ffwll.ch>]

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).

> 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.

> > 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 :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel