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

Wed Mar 15 15:14:49 UTC 2017

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

 > 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? :-)

Noralf.