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

[Noralf Trønnes]

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.

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.

> 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


Noralf.