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

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

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?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch