| Message ID | 20170311213536.2226-2-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? -Daniel
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. Noralf. > -Daniel
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. 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
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. 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. > 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.
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
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@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@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. 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.
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@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@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. > > 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
On 16/03/17 12:14 AM, Noralf Trønnes wrote: > > 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 FWIW, the 11-patch series at https://patchwork.freedesktop.org/project/Xorg/patches/?submitter=16295&state=&q=&archive=&delegate= could allow this to work without involving X's PRIME support.
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); /** + * tinydrm_rgb565_buf_copy - Copy RGB565/XRGB8888 to RGB565 clip buffer + * @dst: RGB565 destination buffer + * @fb: DRM framebuffer + * @clip: Clip rectangle area to copy + * @swap: Swap bytes + * + * Returns: + * Zero on success, negative error code on failure. + */ +int tinydrm_rgb565_buf_copy(void *dst, struct drm_framebuffer *fb, + struct drm_clip_rect *clip, bool swap) +{ + struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0); + struct dma_buf_attachment *import_attach = cma_obj->base.import_attach; + struct drm_format_name_buf format_name; + void *src = cma_obj->vaddr; + int ret = 0; + + if (import_attach) { + ret = dma_buf_begin_cpu_access(import_attach->dmabuf, + DMA_FROM_DEVICE); + if (ret) + return ret; + } + + switch (fb->format->format) { + case DRM_FORMAT_RGB565: + if (swap) + tinydrm_swab16(dst, src, fb, clip); + else + tinydrm_memcpy(dst, src, fb, clip); + break; + case DRM_FORMAT_XRGB8888: + tinydrm_xrgb8888_to_rgb565(dst, src, fb, clip, swap); + break; + default: + dev_err_once(fb->dev->dev, "Format is not supported: %s\n", + drm_get_format_name(fb->format->format, + &format_name)); + return -EINVAL; + } + + if (import_attach) + ret = dma_buf_end_cpu_access(import_attach->dmabuf, + DMA_FROM_DEVICE); + return ret; +} +EXPORT_SYMBOL(tinydrm_rgb565_buf_copy); + +/** * tinydrm_of_find_backlight - Find backlight device in device-tree * @dev: Device * diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h index 9b9b6cf..d1f6722 100644 --- a/include/drm/tinydrm/tinydrm-helpers.h +++ b/include/drm/tinydrm/tinydrm-helpers.h @@ -43,6 +43,8 @@ void tinydrm_swab16(u16 *dst, void *vaddr, struct drm_framebuffer *fb, void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr, struct drm_framebuffer *fb, struct drm_clip_rect *clip, bool swap); +int tinydrm_rgb565_buf_copy(void *dst, struct drm_framebuffer *fb, + struct drm_clip_rect *clip, bool swap); struct backlight_device *tinydrm_of_find_backlight(struct device *dev); int tinydrm_enable_backlight(struct backlight_device *backlight);
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(-)