Add support for displays that have a register interface and can be operated using a simple vtable.
I have looked through the staging/fbtft drivers and it seems that, except the MIPI controllers, most if not all controllers are operated through a register. And since most controllers have more than one bus interface option, regmap seems like a good choice to describe the interface (tested[1,2]). MIPI DCS can't be represented using regmap since some commands doesn't have a parameter. That would be like a register without a value, which doesn't make sense.
In my second RFC of tinydrm I used drm_panel to decribe the panels since it was a good match to the fbtft displays. I was then told that drm_panel wasn't supposed to used like that, so I dropped it and have tried to use the drm_simple_display_pipe_funcs vtable directly. This hasn't been all successful, since I ended up using devm_add_action() to power down the controller at the right time. Thierry Reding wasn't happy with this and suggested "to add an explicit callback somewhere". My solution has been to copy the drm_panel_funcs vtable. Since I now have a vtable, I also added a callback to flush the framebuffer. So presumably all the fbtft drivers can now be operated through the tinydrm_panel_funcs vtable.
After having done this the question arises: Why not extend tinydrm_device instead of subclassing it?
The benefit of subclassing is that it keeps the door open for drivers that can use tinydrm_device, but not tinydrm_panel. But I don't know of such a driver now, then again who knows what the future brings. Something that might or might not happen isn't a good reason, so it seems that I want it this way because I just like it. And it's easy to merge the two should it be that no one uses tinydrm_device directly three years down the line. But I'm actually not sure what's best.
To recap:
tinydrm_device - Combines drm_simple_display_pipe with CMA backed framebuffer and fbdev. - Optional pipe setup with a connector with one mode, but the driver can do it's own.
tinydrm_panel - All drm operations are distilled down to tinydrm_panel_funcs. - Some common driver properties
Noralf.
[1] https://github.com/notro/tinydrm/blob/master/tinydrm-ili9325.c [2] https://github.com/notro/tinydrm/blob/master/fb_ili9325.c
Noralf Trønnes (5): drm/tinydrm: Add tinydrm_rgb565_buf_copy() drm/tinydrm: Add tinydrm_panel abstraction drm/tinydrm/mipi-dbi: Start conversion to tinydrm_panel drm/tinydrm/mi0283qt: Use tinydrm_panel drm/tinydrm/mipi-dbi: Clean up after tinydrm_panel conversion
Documentation/gpu/tinydrm.rst | 12 + drivers/gpu/drm/tinydrm/Kconfig | 1 + drivers/gpu/drm/tinydrm/core/Makefile | 2 +- drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 56 ++- drivers/gpu/drm/tinydrm/core/tinydrm-panel.c | 532 +++++++++++++++++++++++++ drivers/gpu/drm/tinydrm/mi0283qt.c | 113 ++---- drivers/gpu/drm/tinydrm/mipi-dbi.c | 246 ++++-------- include/drm/tinydrm/mipi-dbi.h | 35 +- include/drm/tinydrm/tinydrm-helpers.h | 2 + include/drm/tinydrm/tinydrm-panel.h | 153 +++++++ 10 files changed, 867 insertions(+), 285 deletions(-) create mode 100644 drivers/gpu/drm/tinydrm/core/tinydrm-panel.c create mode 100644 include/drm/tinydrm/tinydrm-panel.h
-- 2.10.2
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);
/** + * 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);
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....
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....
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.h...
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....
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.h...
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....
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&... could allow this to work without involving X's PRIME support.
Add support for displays that can be operated using a simple vtable. Geared towards controllers that can represent it's registers using regmap.
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- Documentation/gpu/tinydrm.rst | 12 + drivers/gpu/drm/tinydrm/Kconfig | 1 + drivers/gpu/drm/tinydrm/core/Makefile | 2 +- drivers/gpu/drm/tinydrm/core/tinydrm-panel.c | 532 +++++++++++++++++++++++++++ include/drm/tinydrm/tinydrm-panel.h | 153 ++++++++ 5 files changed, 699 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/tinydrm/core/tinydrm-panel.c create mode 100644 include/drm/tinydrm/tinydrm-panel.h
diff --git a/Documentation/gpu/tinydrm.rst b/Documentation/gpu/tinydrm.rst index a913644..bb78433 100644 --- a/Documentation/gpu/tinydrm.rst +++ b/Documentation/gpu/tinydrm.rst @@ -20,6 +20,18 @@ Core functionality .. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c :export:
+Panel +===== + +.. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-panel.c + :doc: overview + +.. kernel-doc:: include/drm/tinydrm/tinydrm-panel.h + :internal: + +.. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-panel.c + :export: + Additional helpers ==================
diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig index 3504c53..4ea0fbc 100644 --- a/drivers/gpu/drm/tinydrm/Kconfig +++ b/drivers/gpu/drm/tinydrm/Kconfig @@ -1,6 +1,7 @@ menuconfig DRM_TINYDRM tristate "Support for simple displays" depends on DRM + select REGMAP select DRM_KMS_HELPER select DRM_KMS_CMA_HELPER select BACKLIGHT_LCD_SUPPORT diff --git a/drivers/gpu/drm/tinydrm/core/Makefile b/drivers/gpu/drm/tinydrm/core/Makefile index fb221e6..213b479 100644 --- a/drivers/gpu/drm/tinydrm/core/Makefile +++ b/drivers/gpu/drm/tinydrm/core/Makefile @@ -1,3 +1,3 @@ -tinydrm-y := tinydrm-core.o tinydrm-pipe.o tinydrm-helpers.o +tinydrm-y := tinydrm-core.o tinydrm-pipe.o tinydrm-panel.o tinydrm-helpers.o
obj-$(CONFIG_DRM_TINYDRM) += tinydrm.o diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-panel.c b/drivers/gpu/drm/tinydrm/core/tinydrm-panel.c new file mode 100644 index 0000000..f3e5fb1 --- /dev/null +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-panel.c @@ -0,0 +1,532 @@ +/* + * Copyright 2017 Noralf Trønnes + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/debugfs.h> +#include <linux/device.h> +#include <linux/i2c.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/spi/spi.h> + +#include <drm/drm_gem_cma_helper.h> +#include <drm/drm_fb_cma_helper.h> +#include <drm/drm_fourcc.h> +#include <drm/tinydrm/tinydrm-helpers.h> +#include <drm/tinydrm/tinydrm-panel.h> + +/** + * DOC: overview + * + * This library provides helpers for displays/panels that can be operated + * using a simple vtable. + * + * Many controllers are operated through a register making ®map a useful + * abstraction for the register interface code. This helper is geared towards + * such controllers. Often controllers also support more than one bus, and + * should for instance a controller be connected in a non-standard way + * (e.g. memory mapped), then only the regmap needs to be changed. + */ + +static int tinydrm_panel_prepare(struct tinydrm_panel *panel) +{ + if (panel->funcs && panel->funcs->prepare) + return panel->funcs->prepare(panel); + + if (panel->regulator) + return regulator_enable(panel->regulator); + + return 0; +} + +static int tinydrm_panel_enable(struct tinydrm_panel *panel) +{ + if (panel->funcs && panel->funcs->enable) + return panel->funcs->enable(panel); + + return tinydrm_enable_backlight(panel->backlight); +} + +static int tinydrm_panel_disable(struct tinydrm_panel *panel) +{ + if (panel->funcs && panel->funcs->disable) + return panel->funcs->disable(panel); + + return tinydrm_disable_backlight(panel->backlight); +} + +static int tinydrm_panel_unprepare(struct tinydrm_panel *panel) +{ + if (panel->funcs && panel->funcs->unprepare) + return panel->funcs->unprepare(panel); + + if (panel->regulator) + return regulator_disable(panel->regulator); + + return 0; +} + +static void tinydrm_panel_pipe_enable(struct drm_simple_display_pipe *pipe, + struct drm_crtc_state *crtc_state) +{ + struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); + struct tinydrm_panel *panel = tinydrm_to_panel(tdev); + struct drm_framebuffer *fb = pipe->plane.fb; + + panel->enabled = true; + fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0); + tinydrm_panel_enable(panel); +} + +static void tinydrm_panel_pipe_disable(struct drm_simple_display_pipe *pipe) +{ + struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); + struct tinydrm_panel *panel = tinydrm_to_panel(tdev); + + panel->enabled = false; + tinydrm_panel_disable(panel); +} + +static void tinydrm_panel_pipe_update(struct drm_simple_display_pipe *pipe, + struct drm_plane_state *old_state) +{ + struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); + struct tinydrm_panel *panel = tinydrm_to_panel(tdev); + struct drm_framebuffer *fb = pipe->plane.state->fb; + + /* fb is set (not changed) */ + if (fb && !old_state->fb) + tinydrm_panel_prepare(panel); + + tinydrm_display_pipe_update(pipe, old_state); + + /* fb is unset */ + if (!fb) + tinydrm_panel_unprepare(panel); +} + +static const struct drm_simple_display_pipe_funcs tinydrm_panel_pipe_funcs = { + .enable = tinydrm_panel_pipe_enable, + .disable = tinydrm_panel_pipe_disable, + .update = tinydrm_panel_pipe_update, + .prepare_fb = tinydrm_display_pipe_prepare_fb, +}; + +static int tinydrm_panel_fb_dirty(struct drm_framebuffer *fb, + struct drm_file *file_priv, + unsigned int flags, unsigned int color, + struct drm_clip_rect *clips, + unsigned int num_clips) +{ + struct tinydrm_device *tdev = fb->dev->dev_private; + struct tinydrm_panel *panel = tinydrm_to_panel(tdev); + struct drm_clip_rect rect; + int ret = 0; + + if (!panel->funcs || !panel->funcs->flush) + return 0; + + mutex_lock(&tdev->dirty_lock); + + if (!panel->enabled) + goto out_unlock; + + /* fbdev can flush even when we're not interested */ + if (tdev->pipe.plane.fb != fb) + goto out_unlock; + + tinydrm_merge_clips(&rect, clips, num_clips, flags, + fb->width, fb->height); + + ret = panel->funcs->flush(panel, fb, &rect); + +out_unlock: + mutex_unlock(&tdev->dirty_lock); + + if (ret) + dev_err_once(fb->dev->dev, "Failed to update display %d\n", + ret); + + return ret; +} + +static const struct drm_framebuffer_funcs tinydrm_panel_fb_funcs = { + .destroy = drm_fb_cma_destroy, + .create_handle = drm_fb_cma_create_handle, + .dirty = tinydrm_panel_fb_dirty, +}; + +/** + * tinydrm_panel_init - Initialize &tinydrm_panel + * @dev: Parent device + * @panel: &tinydrm_panel structure to initialize + * @funcs: Callbacks for the panel (optional) + * @formats: Array of supported formats (DRM_FORMAT_*). The first format is + * considered the default format and &tinydrm_panel->tx_buf is + * allocated a buffer that can hold a framebuffer with that format. + * @format_count: Number of elements in @formats + * @driver: DRM driver + * @mode: Display mode + * @rotation: Initial rotation in degrees Counter Clock Wise + * + * This function initializes a &tinydrm_panel structure and it's underlying + * @tinydrm_device. It also sets up the display pipeline. + * + * Objects created by this function will be automatically freed on driver + * detach (devres). + * + * Returns: + * Zero on success, negative error code on failure. + */ +int tinydrm_panel_init(struct device *dev, struct tinydrm_panel *panel, + const struct tinydrm_panel_funcs *funcs, + const uint32_t *formats, unsigned int format_count, + struct drm_driver *driver, + const struct drm_display_mode *mode, + unsigned int rotation) +{ + struct tinydrm_device *tdev = &panel->tinydrm; + const struct drm_format_info *format_info; + size_t bufsize; + int ret; + + format_info = drm_format_info(formats[0]); + bufsize = mode->vdisplay * mode->hdisplay * format_info->cpp[0]; + + panel->tx_buf = devm_kmalloc(dev, bufsize, GFP_KERNEL); + if (!panel->tx_buf) + return -ENOMEM; + + ret = devm_tinydrm_init(dev, tdev, &tinydrm_panel_fb_funcs, driver); + if (ret) + return ret; + + ret = tinydrm_display_pipe_init(tdev, &tinydrm_panel_pipe_funcs, + DRM_MODE_CONNECTOR_VIRTUAL, + formats, format_count, mode, + rotation); + if (ret) + return ret; + + tdev->drm->mode_config.preferred_depth = format_info->depth; + + panel->rotation = rotation; + panel->funcs = funcs; + + drm_mode_config_reset(tdev->drm); + + DRM_DEBUG_KMS("preferred_depth=%u, rotation = %u\n", + tdev->drm->mode_config.preferred_depth, rotation); + + return 0; +} +EXPORT_SYMBOL(tinydrm_panel_init); + +/** + * tinydrm_panel_rgb565_buf - Return RGB565 buffer to scanout + * @panel: tinydrm panel + * @fb: DRM framebuffer + * @rect: Clip rectangle area to scanout + * + * This function returns the RGB565 framebuffer rectangle to scanout. + * It converts XRGB8888 to RGB565 if necessary. + * If copying isn't necessary (RGB565 full rect, no swap), then the backing + * CMA buffer is returned. + * + * Returns: + * Buffer to scanout on success, ERR_PTR on failure. + */ +void *tinydrm_panel_rgb565_buf(struct tinydrm_panel *panel, + struct drm_framebuffer *fb, + struct drm_clip_rect *rect) +{ + struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0); + bool swap = panel->swap_bytes; + bool full; + void *buf; + int ret; + + full = (rect->x2 - rect->x1) == fb->width && + (rect->y2 - rect->y1) == fb->height; + + if (panel->always_tx_buf || swap || !full || + fb->format->format == DRM_FORMAT_XRGB8888) { + buf = panel->tx_buf; + ret = tinydrm_rgb565_buf_copy(buf, fb, rect, swap); + if (ret) + return ERR_PTR(ret); + } else { + buf = cma_obj->vaddr; + } + + return buf; +} +EXPORT_SYMBOL(tinydrm_panel_rgb565_buf); + +/** + * tinydrm_panel_pm_suspend - tinydrm_panel PM suspend helper + * @dev: Device + * + * tinydrm_panel drivers can use this in their &device_driver->pm operations. + * Use dev_set_drvdata() or similar to set &tinydrm_panel as driver data. + */ +int tinydrm_panel_pm_suspend(struct device *dev) +{ + struct tinydrm_panel *panel = dev_get_drvdata(dev); + int ret; + + ret = tinydrm_suspend(&panel->tinydrm); + if (ret) + return ret; + + /* fb isn't set to NULL by suspend, do .unprepare() explicitly */ + tinydrm_panel_unprepare(panel); + + return 0; +} +EXPORT_SYMBOL(tinydrm_panel_pm_suspend); + +/** + * tinydrm_panel_pm_resume - tinydrm_panel PM resume helper + * @dev: Device + * + * tinydrm_panel drivers can use this in their &device_driver->pm operations. + * Use dev_set_drvdata() or similar to set &tinydrm_panel as driver data. + */ +int tinydrm_panel_pm_resume(struct device *dev) +{ + struct tinydrm_panel *panel = dev_get_drvdata(dev); + + /* fb is NULL on resume, .prepare() will be called in pipe_update */ + + return tinydrm_resume(&panel->tinydrm); +} +EXPORT_SYMBOL(tinydrm_panel_pm_resume); + +/** + * tinydrm_panel_spi_shutdown - tinydrm_panel SPI shutdown helper + * @spi: SPI device + * + * tinydrm_panel drivers can use this as their shutdown callback to turn off + * the display on machine shutdown and reboot. Use spi_set_drvdata() or + * similar to set &tinydrm_panel as driver data. + */ +void tinydrm_panel_spi_shutdown(struct spi_device *spi) +{ + struct tinydrm_panel *panel = spi_get_drvdata(spi); + + tinydrm_shutdown(&panel->tinydrm); +} +EXPORT_SYMBOL(tinydrm_panel_spi_shutdown); + +/** + * tinydrm_panel_i2c_shutdown - tinydrm_panel I2C shutdown helper + * @i2c: I2C client device + * + * tinydrm_panel drivers can use this as their shutdown callback to turn off + * the display on machine shutdown and reboot. Use i2c_set_clientdata() or + * similar to set &tinydrm_panel as driver data. + */ +void tinydrm_panel_i2c_shutdown(struct i2c_client *i2c) +{ + struct tinydrm_panel *panel = i2c_get_clientdata(i2c); + + tinydrm_shutdown(&panel->tinydrm); +} +EXPORT_SYMBOL(tinydrm_panel_i2c_shutdown); + +/** + * tinydrm_panel_platform_shutdown - tinydrm_panel platform driver shutdown + * helper + * @pdev: Platform device + * + * tinydrm_panel drivers can use this as their shutdown callback to turn off + * the display on machine shutdown and reboot. Use platform_set_drvdata() or + * similar to set &tinydrm_panel as driver data. + */ +void tinydrm_panel_platform_shutdown(struct platform_device *pdev) +{ + struct tinydrm_panel *panel = platform_get_drvdata(pdev); + + tinydrm_shutdown(&panel->tinydrm); +} +EXPORT_SYMBOL(tinydrm_panel_platform_shutdown); + +/** + * tinydrm_regmap_raw_swap_bytes - Does a raw write require swapping bytes? + * @reg: Regmap + * + * If the bus doesn't support the full regwidth, it has to break up the word. + * Additionally if the bus and machine doesn't match endian wise, this requires + * byteswapping the buffer when using regmap_raw_write(). + * + * Returns: + * True if byte swapping is needed, otherwise false + */ +bool tinydrm_regmap_raw_swap_bytes(struct regmap *reg) +{ + int val_bytes = regmap_get_val_bytes(reg); + unsigned int bus_val; + u16 val16 = 0x00ff; + + if (val_bytes == 1) + return false; + + if (WARN_ON_ONCE(val_bytes != 2)) + return false; + + regmap_parse_val(reg, &val16, &bus_val); + + return val16 != bus_val; +} +EXPORT_SYMBOL(tinydrm_regmap_raw_swap_bytes); + +#ifdef CONFIG_DEBUG_FS + +static int +tinydrm_kstrtoul_array_from_user(const char __user *s, size_t count, + unsigned int base, + unsigned long *vals, size_t num_vals) +{ + char *buf, *pos, *token; + int ret, i = 0; + + buf = memdup_user_nul(s, count); + if (IS_ERR(buf)) + return PTR_ERR(buf); + + pos = buf; + while (pos) { + if (i == num_vals) { + ret = -E2BIG; + goto err_free; + } + + token = strsep(&pos, " "); + if (!token) { + ret = -EINVAL; + goto err_free; + } + + ret = kstrtoul(token, base, vals++); + if (ret < 0) + goto err_free; + i++; + } + +err_free: + kfree(buf); + + return ret ? ret : i; +} + +static ssize_t tinydrm_regmap_debugfs_reg_write(struct file *file, + const char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct seq_file *m = file->private_data; + struct regmap *reg = m->private; + unsigned long vals[2]; + int ret; + + ret = tinydrm_kstrtoul_array_from_user(user_buf, count, 16, vals, 2); + if (ret <= 0) + return ret; + + if (ret != 2) + return -EINVAL; + + ret = regmap_write(reg, vals[0], vals[1]); + + return ret < 0 ? ret : count; +} + +static int tinydrm_regmap_debugfs_reg_show(struct seq_file *m, void *d) +{ + struct regmap *reg = m->private; + int max_reg = regmap_get_max_register(reg); + int val_bytes = regmap_get_val_bytes(reg); + unsigned int val; + int regnr, ret; + + for (regnr = 0; regnr < max_reg; regnr++) { + seq_printf(m, "%.*x: ", val_bytes * 2, regnr); + ret = regmap_read(reg, regnr, &val); + if (ret) + seq_puts(m, "XX\n"); + else + seq_printf(m, "%.*x\n", val_bytes * 2, val); + } + + return 0; +} + +static int tinydrm_regmap_debugfs_reg_open(struct inode *inode, + struct file *file) +{ + return single_open(file, tinydrm_regmap_debugfs_reg_show, + inode->i_private); +} + +static const struct file_operations tinydrm_regmap_debugfs_reg_fops = { + .owner = THIS_MODULE, + .open = tinydrm_regmap_debugfs_reg_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, + .write = tinydrm_regmap_debugfs_reg_write, +}; + +static int +tinydrm_regmap_debugfs_init(struct regmap *reg, struct dentry *parent) +{ + umode_t mode = 0200; + + if (regmap_get_max_register(reg)) + mode |= 0444; + + debugfs_create_file("registers", mode, parent, reg, + &tinydrm_regmap_debugfs_reg_fops); + return 0; +} + +static const struct drm_info_list tinydrm_panel_debugfslist[] = { + { "fb", drm_fb_cma_debugfs_show, 0 }, +}; + +/** + * tinydrm_panel_debugfs_init - Create tinydrm panel debugfs entries + * @minor: DRM minor + * + * &tinydrm_panel drivers can use this as their + * &drm_driver->debugfs_init callback. + * + * Returns: + * Zero on success, negative error code on failure. + */ +int tinydrm_panel_debugfs_init(struct drm_minor *minor) +{ + struct tinydrm_device *tdev = minor->dev->dev_private; + struct tinydrm_panel *panel = tinydrm_to_panel(tdev); + struct regmap *reg = panel->reg; + int ret; + + if (reg) { + ret = tinydrm_regmap_debugfs_init(reg, minor->debugfs_root); + if (ret) + return ret; + } + + return drm_debugfs_create_files(tinydrm_panel_debugfslist, + ARRAY_SIZE(tinydrm_panel_debugfslist), + minor->debugfs_root, minor); +} +EXPORT_SYMBOL(tinydrm_panel_debugfs_init); + +#endif diff --git a/include/drm/tinydrm/tinydrm-panel.h b/include/drm/tinydrm/tinydrm-panel.h new file mode 100644 index 0000000..fc4348d --- /dev/null +++ b/include/drm/tinydrm/tinydrm-panel.h @@ -0,0 +1,153 @@ +/* + * Copyright (C) 2017 Noralf Trønnes + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#ifndef __LINUX_TINYDRM_PANEL_H +#define __LINUX_TINYDRM_PANEL_H + +#include <drm/tinydrm/tinydrm.h> + +struct backlight_device; +struct platform_device; +struct tinydrm_panel; +struct spi_device; +struct regulator; +struct gpio_desc; +struct regmap; + +/** + * struct tinydrm_panel_funcs - tinydrm panel functions + * + * All functions are optional. + */ +struct tinydrm_panel_funcs { + /** + * @prepare: + * + * Prepare controller/display. + * + * This function is called before framebuffer flushing starts. + * Drivers can use this callback to power on and configure the + * controller/display. + * If this is not set and &tinydrm_panel->regulator is set, + * the regulator is enabled. + */ + int (*prepare)(struct tinydrm_panel *panel); + + /** + * @enable: + * + * Enable display. + * + * This function is called when the display pipeline is enabled. + * Drivers can use this callback to turn on the display. + * If this is not set and &tinydrm_panel->backlight is set, + * the backlight is turned on. + */ + int (*enable)(struct tinydrm_panel *panel); + + /** + * @disable: + * + * Disable display. + * + * This function is called when the display pipeline is disabled. + * Drivers can use this callback to turn off the display. + * If this is not set and &tinydrm_panel->backlight is set, + * the backlight is turned off. + */ + int (*disable)(struct tinydrm_panel *panel); + + /** + * @unprepare: + * + * Unprepare controller/display. + * + * This function is called when framebuffer is unset on the plane. + * Drivers can use this callback to power down the controller/display. + * If this is not set and &tinydrm_panel->regulator is set, + * the regulator is disabled. + */ + int (*unprepare)(struct tinydrm_panel *panel); + + /** + * @flush: + * + * Flush framebuffer to controller/display. + * + * This function is called when the framebuffer is flushed. This + * happens when userspace calls ioctl DRM_IOCTL_MODE_DIRTYFB, when the + * framebuffer is changed on the plane and when the pipeline is + * enabled. If multiple clip rectangles are passed in, they are merged + * into one rectangle and passed to @flush. No flushing happens + * during the time the pipeline is disabled. + */ + int (*flush)(struct tinydrm_panel *panel, struct drm_framebuffer *fb, + struct drm_clip_rect *rect); +}; + +/** + * struct tinydrm_panel - tinydrm panel device + * @tinydrm: Base &tinydrm_device + * @funcs: tinydrm panel functions (optional) + * @reg: Register map (optional) + * @enabled: Pipeline is enabled + * @tx_buf: Transmit buffer + * @swap_bytes: Swap pixel data bytes + * @always_tx_buf: Always use @tx_buf + * @rotation: Rotation in degrees Counter Clock Wise + * @reset: Optional reset gpio + * @backlight: Optional backlight device + * @regulator: Optional regulator + */ +struct tinydrm_panel { + struct tinydrm_device tinydrm; + const struct tinydrm_panel_funcs *funcs; + struct regmap *reg; + bool enabled; + void *tx_buf; + bool swap_bytes; + bool always_tx_buf; + unsigned int rotation; + struct gpio_desc *reset; + struct backlight_device *backlight; + struct regulator *regulator; +}; + +static inline struct tinydrm_panel * +tinydrm_to_panel(struct tinydrm_device *tdev) +{ + return container_of(tdev, struct tinydrm_panel, tinydrm); +} + +int tinydrm_panel_init(struct device *dev, struct tinydrm_panel *panel, + const struct tinydrm_panel_funcs *funcs, + const uint32_t *formats, unsigned int format_count, + struct drm_driver *driver, + const struct drm_display_mode *mode, + unsigned int rotation); + +void *tinydrm_panel_rgb565_buf(struct tinydrm_panel *panel, + struct drm_framebuffer *fb, + struct drm_clip_rect *rect); + +int tinydrm_panel_pm_suspend(struct device *dev); +int tinydrm_panel_pm_resume(struct device *dev); +void tinydrm_panel_spi_shutdown(struct spi_device *spi); +void tinydrm_panel_i2c_shutdown(struct i2c_client *i2c); +void tinydrm_panel_platform_shutdown(struct platform_device *pdev); + +bool tinydrm_regmap_raw_swap_bytes(struct regmap *reg); + +#ifdef CONFIG_DEBUG_FS +int tinydrm_panel_debugfs_init(struct drm_minor *minor); +#else +#define tinydrm_panel_debugfs_init NULL +#endif + +#endif /* __LINUX_TINYDRM_PANEL_H */
This starts the conversion of basing mipi_dbi on tinydrm_panel. - Add mipi_dbi_flush() and mipi_dbi_panel_disable() - Switch to tinydrm_panel properties
MIPI DCS can't be represented using regmap since some commands doesn't have a parameter. That would be like a register without a value, which doesn't make sense. So the tinydrm_panel->reg property isn't used.
Additionally change to the common header include order.
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/tinydrm/mipi-dbi.c | 95 ++++++++++++++++++++++++++++++++++---- include/drm/tinydrm/mipi-dbi.h | 14 +++++- 2 files changed, 98 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c index 29c0939..2cdd558 100644 --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c @@ -9,14 +9,16 @@ * (at your option) any later version. */
-#include <drm/tinydrm/mipi-dbi.h> -#include <drm/tinydrm/tinydrm-helpers.h> #include <linux/debugfs.h> #include <linux/dma-buf.h> #include <linux/gpio/consumer.h> #include <linux/module.h> #include <linux/regulator/consumer.h> #include <linux/spi/spi.h> + +#include <drm/tinydrm/mipi-dbi.h> +#include <drm/tinydrm/tinydrm-helpers.h> + #include <video/mipi_display.h>
#define MIPI_DBI_MAX_SPI_READ_SPEED 2000000 /* 2MHz */ @@ -283,22 +285,84 @@ void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe, } EXPORT_SYMBOL(mipi_dbi_pipe_enable);
+/** + * mipi_dbi_panel_flush - MIPI DBI panel flush helper + * @panel: tinydrm panel + * @fb: DRM framebuffer + * @rect: Clip rectangle to flush + * + * This function flushes the framebuffer changes to the display/controller. + * Drivers can use this as their &tinydrm_panel_funcs->flush callback. + * + * Returns: + * Zero on success, negative error code on failure. + */ +int mipi_dbi_panel_flush(struct tinydrm_panel *panel, + struct drm_framebuffer *fb, + struct drm_clip_rect *rect) +{ + struct mipi_dbi *mipi = mipi_dbi_from_panel(panel); + void *tr; + + DRM_DEBUG("Flushing [FB:%d] x1=%u, x2=%u, y1=%u, y2=%u, swap=%u\n", + fb->base.id, rect->x1, rect->x2, rect->y1, rect->y2, + panel->swap_bytes); + + tr = tinydrm_panel_rgb565_buf(panel, fb, rect); + if (IS_ERR(tr)) + return PTR_ERR(tr); + + mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS, + (rect->x1 >> 8) & 0xFF, rect->x1 & 0xFF, + (rect->x2 >> 8) & 0xFF, (rect->x2 - 1) & 0xFF); + mipi_dbi_command(mipi, MIPI_DCS_SET_PAGE_ADDRESS, + (rect->y1 >> 8) & 0xFF, rect->y1 & 0xFF, + (rect->y2 >> 8) & 0xFF, (rect->y2 - 1) & 0xFF); + + return mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START, tr, + (rect->x2 - rect->x1) * + (rect->y2 - rect->y1) * 2); +} +EXPORT_SYMBOL(mipi_dbi_panel_flush); + static void mipi_dbi_blank(struct mipi_dbi *mipi) { - struct drm_device *drm = mipi->tinydrm.drm; + struct tinydrm_panel *panel = &mipi->panel; + struct drm_device *drm = panel->tinydrm.drm; u16 height = drm->mode_config.min_height; u16 width = drm->mode_config.min_width; size_t len = width * height * 2;
- memset(mipi->tx_buf, 0, len); + memset(panel->tx_buf, 0, len);
mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS, 0, 0, (width >> 8) & 0xFF, (width - 1) & 0xFF); mipi_dbi_command(mipi, MIPI_DCS_SET_PAGE_ADDRESS, 0, 0, (height >> 8) & 0xFF, (height - 1) & 0xFF); mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START, - (u8 *)mipi->tx_buf, len); + panel->tx_buf, len); +} + +/** + * mipi_dbi_panel_disable - MIPI DBI panel disable helper + * @panel: tinydrm panel + * + * This function disables backlight if present or if not the + * display memory is blanked. Drivers can use this as their + * &tinydrm_panel_funcs->disable callback. + */ +int mipi_dbi_panel_disable(struct tinydrm_panel *panel) +{ + struct mipi_dbi *mipi = mipi_dbi_from_panel(panel); + + if (panel->backlight) + return tinydrm_disable_backlight(panel->backlight); + + mipi_dbi_blank(mipi); + + return 0; } +EXPORT_SYMBOL(mipi_dbi_panel_disable);
/** * mipi_dbi_pipe_disable - MIPI DBI pipe disable helper @@ -356,6 +420,7 @@ int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi, { size_t bufsize = mode->vdisplay * mode->hdisplay * sizeof(u16); struct tinydrm_device *tdev = &mipi->tinydrm; + struct tinydrm_panel *panel = &mipi->panel; int ret;
if (!mipi->command) @@ -388,6 +453,12 @@ int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi, DRM_DEBUG_KMS("preferred_depth=%u, rotation = %u\n", tdev->drm->mode_config.preferred_depth, rotation);
+ /* transitional assignements */ + panel->tinydrm.drm = mipi->tinydrm.drm; + mipi->swap_bytes = panel->swap_bytes; + panel->tx_buf = mipi->tx_buf; + panel->reset = mipi->reset; + return 0; } EXPORT_SYMBOL(mipi_dbi_init); @@ -400,12 +471,14 @@ EXPORT_SYMBOL(mipi_dbi_init); */ void mipi_dbi_hw_reset(struct mipi_dbi *mipi) { - if (!mipi->reset) + struct tinydrm_panel *panel = &mipi->panel; + + if (!panel->reset) return;
- gpiod_set_value_cansleep(mipi->reset, 0); + gpiod_set_value_cansleep(panel->reset, 0); msleep(20); - gpiod_set_value_cansleep(mipi->reset, 1); + gpiod_set_value_cansleep(panel->reset, 1); msleep(120); } EXPORT_SYMBOL(mipi_dbi_hw_reset); @@ -764,7 +837,7 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd, if (ret || !num) return ret;
- if (cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes) + if (cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->panel.swap_bytes) bpw = 16;
gpiod_set_value_cansleep(mipi->dc, 1); @@ -806,6 +879,7 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi, unsigned int rotation) { size_t tx_size = tinydrm_spi_max_transfer_size(spi, 0); + struct tinydrm_panel *panel = &mipi->panel; struct device *dev = &spi->dev; int ret;
@@ -840,8 +914,9 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi, mipi->dc = dc; if (tinydrm_machine_little_endian() && !tinydrm_spi_bpw_supported(spi, 16)) - mipi->swap_bytes = true; + panel->swap_bytes = true; } else { + panel->always_tx_buf = true; mipi->command = mipi_dbi_typec1_command; mipi->tx_buf9_len = tx_size; mipi->tx_buf9 = devm_kmalloc(dev, tx_size, GFP_KERNEL); diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h index d137b16..c2ec7ae 100644 --- a/include/drm/tinydrm/mipi-dbi.h +++ b/include/drm/tinydrm/mipi-dbi.h @@ -12,7 +12,7 @@ #ifndef __LINUX_MIPI_DBI_H #define __LINUX_MIPI_DBI_H
-#include <drm/tinydrm/tinydrm.h> +#include <drm/tinydrm/tinydrm-panel.h>
struct spi_device; struct gpio_desc; @@ -21,6 +21,7 @@ struct regulator; /** * struct mipi_dbi - MIPI DBI controller * @tinydrm: tinydrm base + * @panel: tinydrm panel * @spi: SPI device * @enabled: Pipeline is enabled * @cmdlock: Command lock @@ -39,6 +40,7 @@ struct regulator; */ struct mipi_dbi { struct tinydrm_device tinydrm; + struct tinydrm_panel panel; struct spi_device *spi; bool enabled; struct mutex cmdlock; @@ -61,6 +63,12 @@ mipi_dbi_from_tinydrm(struct tinydrm_device *tdev) return container_of(tdev, struct mipi_dbi, tinydrm); }
+static inline struct mipi_dbi * +mipi_dbi_from_panel(struct tinydrm_panel *panel) +{ + return container_of(panel, struct mipi_dbi, panel); +} + int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi, struct gpio_desc *dc, const struct drm_simple_display_pipe_funcs *pipe_funcs, @@ -71,6 +79,10 @@ int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi, const struct drm_simple_display_pipe_funcs *pipe_funcs, struct drm_driver *driver, const struct drm_display_mode *mode, unsigned int rotation); +int mipi_dbi_panel_flush(struct tinydrm_panel *panel, + struct drm_framebuffer *fb, + struct drm_clip_rect *rect); +int mipi_dbi_panel_disable(struct tinydrm_panel *panel); void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe, struct drm_crtc_state *crtc_state); void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
Convert mi0283qt to use tinydrm_panel. Let mipi-dbi use tinydrm_panel_init().
Additionally change to the common header include order.
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/tinydrm/mi0283qt.c | 113 +++++++++++-------------------------- drivers/gpu/drm/tinydrm/mipi-dbi.c | 51 ++++------------- include/drm/tinydrm/mipi-dbi.h | 4 +- 3 files changed, 45 insertions(+), 123 deletions(-)
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index b29fe86..6e8142a 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -9,27 +9,30 @@ * (at your option) any later version. */
-#include <drm/tinydrm/ili9341.h> -#include <drm/tinydrm/mipi-dbi.h> -#include <drm/tinydrm/tinydrm-helpers.h> #include <linux/delay.h> #include <linux/gpio/consumer.h> #include <linux/module.h> #include <linux/property.h> #include <linux/regulator/consumer.h> #include <linux/spi/spi.h> + +#include <drm/tinydrm/ili9341.h> +#include <drm/tinydrm/mipi-dbi.h> +#include <drm/tinydrm/tinydrm-helpers.h> + #include <video/mipi_display.h>
-static int mi0283qt_init(struct mipi_dbi *mipi) +static int mi0283qt_prepare(struct tinydrm_panel *panel) { - struct tinydrm_device *tdev = &mipi->tinydrm; + struct mipi_dbi *mipi = mipi_dbi_from_panel(panel); + struct tinydrm_device *tdev = &panel->tinydrm; struct device *dev = tdev->drm->dev; u8 addr_mode; int ret;
DRM_DEBUG_KMS("\n");
- ret = regulator_enable(mipi->regulator); + ret = regulator_enable(panel->regulator); if (ret) { dev_err(dev, "Failed to enable regulator %d\n", ret); return ret; @@ -43,7 +46,7 @@ static int mi0283qt_init(struct mipi_dbi *mipi) ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET); if (ret) { dev_err(dev, "Error sending command %d\n", ret); - regulator_disable(mipi->regulator); + regulator_disable(panel->regulator); return ret; }
@@ -68,7 +71,7 @@ static int mi0283qt_init(struct mipi_dbi *mipi) /* Memory Access Control */ mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT, 0x55);
- switch (mipi->rotation) { + switch (panel->rotation) { default: addr_mode = ILI9341_MADCTL_MV | ILI9341_MADCTL_MY | ILI9341_MADCTL_MX; @@ -113,19 +116,10 @@ static int mi0283qt_init(struct mipi_dbi *mipi) return 0; }
-static void mi0283qt_fini(void *data) -{ - struct mipi_dbi *mipi = data; - - DRM_DEBUG_KMS("\n"); - regulator_disable(mipi->regulator); -} - -static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = { - .enable = mipi_dbi_pipe_enable, - .disable = mipi_dbi_pipe_disable, - .update = tinydrm_display_pipe_update, - .prepare_fb = tinydrm_display_pipe_prepare_fb, +static const struct tinydrm_panel_funcs mi0283qt_funcs = { + .prepare = mi0283qt_prepare, + .disable = mipi_dbi_panel_disable, + .flush = mipi_dbi_panel_flush, };
static const struct drm_display_mode mi0283qt_mode = { @@ -161,6 +155,7 @@ static int mi0283qt_probe(struct spi_device *spi) { struct device *dev = &spi->dev; struct tinydrm_device *tdev; + struct tinydrm_panel *panel; struct mipi_dbi *mipi; struct gpio_desc *dc; u32 rotation = 0; @@ -170,10 +165,13 @@ static int mi0283qt_probe(struct spi_device *spi) if (!mipi) return -ENOMEM;
- mipi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); - if (IS_ERR(mipi->reset)) { + panel = &mipi->panel; + tdev = &panel->tinydrm; + + panel->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); + if (IS_ERR(panel->reset)) { dev_err(dev, "Failed to get gpio 'reset'\n"); - return PTR_ERR(mipi->reset); + return PTR_ERR(panel->reset); }
dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW); @@ -182,39 +180,26 @@ static int mi0283qt_probe(struct spi_device *spi) return PTR_ERR(dc); }
- mipi->regulator = devm_regulator_get(dev, "power"); - if (IS_ERR(mipi->regulator)) - return PTR_ERR(mipi->regulator); + panel->regulator = devm_regulator_get(dev, "power"); + if (IS_ERR(panel->regulator)) + return PTR_ERR(panel->regulator);
- mipi->backlight = tinydrm_of_find_backlight(dev); - if (IS_ERR(mipi->backlight)) - return PTR_ERR(mipi->backlight); + panel->backlight = tinydrm_of_find_backlight(dev); + if (IS_ERR(panel->backlight)) + return PTR_ERR(panel->backlight);
device_property_read_u32(dev, "rotation", &rotation);
- ret = mipi_dbi_spi_init(spi, mipi, dc, &mi0283qt_pipe_funcs, + ret = mipi_dbi_spi_init(spi, mipi, dc, &mi0283qt_funcs, &mi0283qt_driver, &mi0283qt_mode, rotation); if (ret) return ret;
- ret = mi0283qt_init(mipi); - if (ret) - return ret; - - /* use devres to fini after drm unregister (drv->remove is before) */ - ret = devm_add_action(dev, mi0283qt_fini, mipi); - if (ret) { - mi0283qt_fini(mipi); - return ret; - } - - tdev = &mipi->tinydrm; - ret = devm_tinydrm_register(tdev); if (ret) return ret;
- spi_set_drvdata(spi, mipi); + spi_set_drvdata(spi, panel);
DRM_DEBUG_DRIVER("Initialized %s:%s @%uMHz on minor %d\n", tdev->drm->driver->name, dev_name(dev), @@ -224,41 +209,9 @@ static int mi0283qt_probe(struct spi_device *spi) return 0; }
-static void mi0283qt_shutdown(struct spi_device *spi) -{ - struct mipi_dbi *mipi = spi_get_drvdata(spi); - - tinydrm_shutdown(&mipi->tinydrm); -} - -static int __maybe_unused mi0283qt_pm_suspend(struct device *dev) -{ - struct mipi_dbi *mipi = dev_get_drvdata(dev); - int ret; - - ret = tinydrm_suspend(&mipi->tinydrm); - if (ret) - return ret; - - mi0283qt_fini(mipi); - - return 0; -} - -static int __maybe_unused mi0283qt_pm_resume(struct device *dev) -{ - struct mipi_dbi *mipi = dev_get_drvdata(dev); - int ret; - - ret = mi0283qt_init(mipi); - if (ret) - return ret; - - return tinydrm_resume(&mipi->tinydrm); -} - static const struct dev_pm_ops mi0283qt_pm_ops = { - SET_SYSTEM_SLEEP_PM_OPS(mi0283qt_pm_suspend, mi0283qt_pm_resume) + SET_SYSTEM_SLEEP_PM_OPS(tinydrm_panel_pm_suspend, + tinydrm_panel_pm_resume) };
static struct spi_driver mi0283qt_spi_driver = { @@ -270,7 +223,7 @@ static struct spi_driver mi0283qt_spi_driver = { }, .id_table = mi0283qt_id, .probe = mi0283qt_probe, - .shutdown = mi0283qt_shutdown, + .shutdown = tinydrm_panel_spi_shutdown, }; module_spi_driver(mi0283qt_spi_driver);
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c index 2cdd558..2f12a9a 100644 --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c @@ -397,7 +397,7 @@ static const uint32_t mipi_dbi_formats[] = { * mipi_dbi_init - MIPI DBI initialization * @dev: Parent device * @mipi: &mipi_dbi structure to initialize - * @pipe_funcs: Display pipe functions + * @funcs: tinydrm panel functions * @driver: DRM driver * @mode: Display mode * @rotation: Initial rotation in degrees Counter Clock Wise @@ -414,52 +414,20 @@ static const uint32_t mipi_dbi_formats[] = { * Zero on success, negative error code on failure. */ int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi, - const struct drm_simple_display_pipe_funcs *pipe_funcs, + const struct tinydrm_panel_funcs *funcs, struct drm_driver *driver, const struct drm_display_mode *mode, unsigned int rotation) { - size_t bufsize = mode->vdisplay * mode->hdisplay * sizeof(u16); - struct tinydrm_device *tdev = &mipi->tinydrm; struct tinydrm_panel *panel = &mipi->panel; - int ret;
if (!mipi->command) return -EINVAL;
mutex_init(&mipi->cmdlock);
- mipi->tx_buf = devm_kmalloc(dev, bufsize, GFP_KERNEL); - if (!mipi->tx_buf) - return -ENOMEM; - - ret = devm_tinydrm_init(dev, tdev, &mipi_dbi_fb_funcs, driver); - if (ret) - return ret; - - /* TODO: Maybe add DRM_MODE_CONNECTOR_SPI */ - ret = tinydrm_display_pipe_init(tdev, pipe_funcs, - DRM_MODE_CONNECTOR_VIRTUAL, - mipi_dbi_formats, - ARRAY_SIZE(mipi_dbi_formats), mode, - rotation); - if (ret) - return ret; - - tdev->drm->mode_config.preferred_depth = 16; - mipi->rotation = rotation; - - drm_mode_config_reset(tdev->drm); - - DRM_DEBUG_KMS("preferred_depth=%u, rotation = %u\n", - tdev->drm->mode_config.preferred_depth, rotation); - - /* transitional assignements */ - panel->tinydrm.drm = mipi->tinydrm.drm; - mipi->swap_bytes = panel->swap_bytes; - panel->tx_buf = mipi->tx_buf; - panel->reset = mipi->reset; - - return 0; + return tinydrm_panel_init(dev, panel, funcs, mipi_dbi_formats, + ARRAY_SIZE(mipi_dbi_formats), + driver, mode, rotation); } EXPORT_SYMBOL(mipi_dbi_init);
@@ -851,7 +819,7 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd, * @spi: SPI device * @dc: D/C gpio (optional) * @mipi: &mipi_dbi structure to initialize - * @pipe_funcs: Display pipe functions + * @funcs: tinydrm panel functions * @driver: DRM driver * @mode: Display mode * @rotation: Initial rotation in degrees Counter Clock Wise @@ -873,7 +841,7 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd, */ int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi, struct gpio_desc *dc, - const struct drm_simple_display_pipe_funcs *pipe_funcs, + const struct tinydrm_panel_funcs *funcs, struct drm_driver *driver, const struct drm_display_mode *mode, unsigned int rotation) @@ -924,7 +892,7 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi, return -ENOMEM; }
- return mipi_dbi_init(dev, mipi, pipe_funcs, driver, mode, rotation); + return mipi_dbi_init(dev, mipi, funcs, driver, mode, rotation); } EXPORT_SYMBOL(mipi_dbi_spi_init);
@@ -1061,7 +1029,8 @@ static const struct drm_info_list mipi_dbi_debugfs_list[] = { int mipi_dbi_debugfs_init(struct drm_minor *minor) { struct tinydrm_device *tdev = minor->dev->dev_private; - struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); + struct tinydrm_panel *panel = tinydrm_to_panel(tdev); + struct mipi_dbi *mipi = mipi_dbi_from_panel(panel); umode_t mode = S_IFREG | S_IWUSR;
if (mipi->read_commands) diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h index c2ec7ae..199f109 100644 --- a/include/drm/tinydrm/mipi-dbi.h +++ b/include/drm/tinydrm/mipi-dbi.h @@ -71,12 +71,12 @@ mipi_dbi_from_panel(struct tinydrm_panel *panel)
int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi, struct gpio_desc *dc, - const struct drm_simple_display_pipe_funcs *pipe_funcs, + const struct tinydrm_panel_funcs *funcs, struct drm_driver *driver, const struct drm_display_mode *mode, unsigned int rotation); int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi, - const struct drm_simple_display_pipe_funcs *pipe_funcs, + const struct tinydrm_panel_funcs *funcs, struct drm_driver *driver, const struct drm_display_mode *mode, unsigned int rotation); int mipi_dbi_panel_flush(struct tinydrm_panel *panel,
Finish conversion to tinydrm_panel by removing unneeded functions and properties.
Signed-off-by: Noralf Trønnes noralf@tronnes.org --- drivers/gpu/drm/tinydrm/mipi-dbi.c | 154 ------------------------------------- include/drm/tinydrm/mipi-dbi.h | 25 ------ 2 files changed, 179 deletions(-)
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c index 2f12a9a..0c29b74 100644 --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c @@ -155,136 +155,6 @@ int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len) } EXPORT_SYMBOL(mipi_dbi_command_buf);
-static int mipi_dbi_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; -} - -static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb, - struct drm_file *file_priv, - unsigned int flags, unsigned int color, - struct drm_clip_rect *clips, - unsigned int num_clips) -{ - struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0); - struct tinydrm_device *tdev = fb->dev->dev_private; - struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); - bool swap = mipi->swap_bytes; - struct drm_clip_rect clip; - int ret = 0; - bool full; - void *tr; - - mutex_lock(&tdev->dirty_lock); - - if (!mipi->enabled) - goto out_unlock; - - /* fbdev can flush even when we're not interested */ - if (tdev->pipe.plane.fb != fb) - goto out_unlock; - - full = tinydrm_merge_clips(&clip, clips, num_clips, flags, - fb->width, fb->height); - - DRM_DEBUG("Flushing [FB:%d] x1=%u, x2=%u, y1=%u, y2=%u\n", fb->base.id, - clip.x1, clip.x2, clip.y1, clip.y2); - - if (!mipi->dc || !full || swap || - fb->format->format == DRM_FORMAT_XRGB8888) { - tr = mipi->tx_buf; - ret = mipi_dbi_buf_copy(mipi->tx_buf, fb, &clip, swap); - if (ret) - goto out_unlock; - } else { - tr = cma_obj->vaddr; - } - - mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS, - (clip.x1 >> 8) & 0xFF, clip.x1 & 0xFF, - (clip.x2 >> 8) & 0xFF, (clip.x2 - 1) & 0xFF); - mipi_dbi_command(mipi, MIPI_DCS_SET_PAGE_ADDRESS, - (clip.y1 >> 8) & 0xFF, clip.y1 & 0xFF, - (clip.y2 >> 8) & 0xFF, (clip.y2 - 1) & 0xFF); - - ret = mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START, tr, - (clip.x2 - clip.x1) * (clip.y2 - clip.y1) * 2); - -out_unlock: - mutex_unlock(&tdev->dirty_lock); - - if (ret) - dev_err_once(fb->dev->dev, "Failed to update display %d\n", - ret); - - return ret; -} - -static const struct drm_framebuffer_funcs mipi_dbi_fb_funcs = { - .destroy = drm_fb_cma_destroy, - .create_handle = drm_fb_cma_create_handle, - .dirty = mipi_dbi_fb_dirty, -}; - -/** - * mipi_dbi_pipe_enable - MIPI DBI pipe enable helper - * @pipe: Display pipe - * @crtc_state: CRTC state - * - * This function enables backlight. Drivers can use this as their - * &drm_simple_display_pipe_funcs->enable callback. - */ -void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe, - struct drm_crtc_state *crtc_state) -{ - struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); - struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); - struct drm_framebuffer *fb = pipe->plane.fb; - - DRM_DEBUG_KMS("\n"); - - mipi->enabled = true; - if (fb) - fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0); - - tinydrm_enable_backlight(mipi->backlight); -} -EXPORT_SYMBOL(mipi_dbi_pipe_enable); - /** * mipi_dbi_panel_flush - MIPI DBI panel flush helper * @panel: tinydrm panel @@ -364,30 +234,6 @@ int mipi_dbi_panel_disable(struct tinydrm_panel *panel) } EXPORT_SYMBOL(mipi_dbi_panel_disable);
-/** - * mipi_dbi_pipe_disable - MIPI DBI pipe disable helper - * @pipe: Display pipe - * - * This function disables backlight if present or if not the - * display memory is blanked. Drivers can use this as their - * &drm_simple_display_pipe_funcs->disable callback. - */ -void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe) -{ - struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); - struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); - - DRM_DEBUG_KMS("\n"); - - mipi->enabled = false; - - if (mipi->backlight) - tinydrm_disable_backlight(mipi->backlight); - else - mipi_dbi_blank(mipi); -} -EXPORT_SYMBOL(mipi_dbi_pipe_disable); - static const uint32_t mipi_dbi_formats[] = { DRM_FORMAT_RGB565, DRM_FORMAT_XRGB8888, diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h index 199f109..71b04ef 100644 --- a/include/drm/tinydrm/mipi-dbi.h +++ b/include/drm/tinydrm/mipi-dbi.h @@ -20,50 +20,28 @@ struct regulator;
/** * struct mipi_dbi - MIPI DBI controller - * @tinydrm: tinydrm base * @panel: tinydrm panel * @spi: SPI device - * @enabled: Pipeline is enabled * @cmdlock: Command lock * @command: Bus specific callback executing commands. * @read_commands: Array of read commands terminated by a zero entry. * Reading is disabled if this is NULL. * @dc: Optional D/C gpio. - * @tx_buf: Buffer used for transfer (copy clip rect area) * @tx_buf9: Buffer used for Option 1 9-bit conversion * @tx_buf9_len: Size of tx_buf9. - * @swap_bytes: Swap bytes in buffer before transfer - * @reset: Optional reset gpio - * @rotation: initial rotation in degrees Counter Clock Wise - * @backlight: backlight device (optional) - * @regulator: power regulator (optional) */ struct mipi_dbi { - struct tinydrm_device tinydrm; struct tinydrm_panel panel; struct spi_device *spi; - bool enabled; struct mutex cmdlock; int (*command)(struct mipi_dbi *mipi, u8 cmd, u8 *param, size_t num); const u8 *read_commands; struct gpio_desc *dc; - u16 *tx_buf; void *tx_buf9; size_t tx_buf9_len; - bool swap_bytes; - struct gpio_desc *reset; - unsigned int rotation; - struct backlight_device *backlight; - struct regulator *regulator; };
static inline struct mipi_dbi * -mipi_dbi_from_tinydrm(struct tinydrm_device *tdev) -{ - return container_of(tdev, struct mipi_dbi, tinydrm); -} - -static inline struct mipi_dbi * mipi_dbi_from_panel(struct tinydrm_panel *panel) { return container_of(panel, struct mipi_dbi, panel); @@ -83,9 +61,6 @@ int mipi_dbi_panel_flush(struct tinydrm_panel *panel, struct drm_framebuffer *fb, struct drm_clip_rect *rect); int mipi_dbi_panel_disable(struct tinydrm_panel *panel); -void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe, - struct drm_crtc_state *crtc_state); -void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe); void mipi_dbi_hw_reset(struct mipi_dbi *mipi); bool mipi_dbi_display_is_on(struct mipi_dbi *mipi);
Hi Noralf,
On Sat, Mar 11, 2017 at 10:35:31PM +0100, Noralf Trønnes wrote:
Add support for displays that have a register interface and can be operated using a simple vtable.
I have looked through the staging/fbtft drivers and it seems that, except the MIPI controllers, most if not all controllers are operated through a register. And since most controllers have more than one bus interface option, regmap seems like a good choice to describe the interface (tested[1,2]). MIPI DCS can't be represented using regmap since some commands doesn't have a parameter. That would be like a register without a value, which doesn't make sense.
In my second RFC of tinydrm I used drm_panel to decribe the panels since it was a good match to the fbtft displays. I was then told that drm_panel wasn't supposed to used like that, so I dropped it and have tried to use the drm_simple_display_pipe_funcs vtable directly. This hasn't been all successful, since I ended up using devm_add_action() to power down the controller at the right time. Thierry Reding wasn't happy with this and suggested "to add an explicit callback somewhere". My solution has been to copy the drm_panel_funcs vtable. Since I now have a vtable, I also added a callback to flush the framebuffer. So presumably all the fbtft drivers can now be operated through the tinydrm_panel_funcs vtable.
Ehrm, what? I admit I didn't follow the discussion in-depth, but if drm_panel can't be used to describe a panel, it's not fit for the job and needs to be extended. Adding even more abstraction, matroschka-style, isn't a solution.
Personally I think tinydrm itself is already a bit much wrapping, but I see that for really simple drivers it has it's uses. But more layers feels like going in the wrong direction.
For the callback you're looking for (i.e. the regulator_disable call) I think the correct place is to enable/disable the regulator in the enable/disable hooks of the drm_simple_display_pipe functions. Or maybe in their equivalent in drm_panel (well, probably pre_enable and post_disable, since I guess you need that regulator to driver anything). Same for _init, if the display is completely off there's no point in keeping the hw running. Enabling/disabling the entire hw is pretty much what ->enable and ->disable are for. This also gives you proper runtime pm for almost for free ...
Also, since the regulator is actually stored in struct mipi_dbi, it's probably best to handle it in the mipi_dbi helpers too. You do that already with the backlight anyway.
I noticed that the version of tinydrm that landed doesn't use drm_panel anymore, I thought that was the case once, and for the version I acked?
After having done this the question arises: Why not extend tinydrm_device instead of subclassing it?
The benefit of subclassing is that it keeps the door open for drivers that can use tinydrm_device, but not tinydrm_panel. But I don't know of such a driver now, then again who knows what the future brings. Something that might or might not happen isn't a good reason, so it seems that I want it this way because I just like it. And it's easy to merge the two should it be that no one uses tinydrm_device directly three years down the line. But I'm actually not sure what's best.
As a rule of thumb, never design code for future use that you don't know yet will happen. No one can reliably predict the future, which means you'll get it wrong, and in the future we'll have to do 2x the work: Once do unto the code resulting from the wrong prediction, then redoing it the way we need to. Not including the on-going burden of maintaining unused functionally.
So let's pls merge first, split later when there's a clean need.
To recap:
tinydrm_device
- Combines drm_simple_display_pipe with CMA backed framebuffer and fbdev.
- Optional pipe setup with a connector with one mode, but the driver can do it's own.
tinydrm_panel
- All drm operations are distilled down to tinydrm_panel_funcs.
- Some common driver properties
So overal sorry I'm shredding this a bit, but I don't see the point. Imo much more useful would be:
1. Extract the non-tinydrm specific helpers from tinydrm and put them into the right places. I think from our discussions this was:
- backlight helpers, probably best to put them into a new drm_backlight.c. This is because drivers/video is defactor unmaintained. We could also move drivers/video/backlight to drivers/gpu/backlight and take it all over within drm-misc, but that's more work.
- spi helpers, probably best put into spi core/helper code. Thierry said the spi maintainer is fast&reactive, so shouldn't be a big issue.
- extract the mipi-dbi helper (well, the non-tinydrm specific parts at least) into a separate helper, like we have for mipi-dsi already.
2. Add tons more panel drivers. Personally I'd do that first :-)
Cheers, Daniel
On Sun, Mar 12, 2017 at 06:50:17PM +0100, Daniel Vetter wrote:
Hi Noralf,
On Sat, Mar 11, 2017 at 10:35:31PM +0100, Noralf Trønnes wrote:
Add support for displays that have a register interface and can be operated using a simple vtable.
I have looked through the staging/fbtft drivers and it seems that, except the MIPI controllers, most if not all controllers are operated through a register. And since most controllers have more than one bus interface option, regmap seems like a good choice to describe the interface (tested[1,2]). MIPI DCS can't be represented using regmap since some commands doesn't have a parameter. That would be like a register without a value, which doesn't make sense.
In my second RFC of tinydrm I used drm_panel to decribe the panels since it was a good match to the fbtft displays. I was then told that drm_panel wasn't supposed to used like that, so I dropped it and have tried to use the drm_simple_display_pipe_funcs vtable directly. This hasn't been all successful, since I ended up using devm_add_action() to power down the controller at the right time. Thierry Reding wasn't happy with this and suggested "to add an explicit callback somewhere". My solution has been to copy the drm_panel_funcs vtable. Since I now have a vtable, I also added a callback to flush the framebuffer. So presumably all the fbtft drivers can now be operated through the tinydrm_panel_funcs vtable.
Ehrm, what? I admit I didn't follow the discussion in-depth, but if drm_panel can't be used to describe a panel, it's not fit for the job and needs to be extended. Adding even more abstraction, matroschka-style, isn't a solution.
Personally I think tinydrm itself is already a bit much wrapping, but I see that for really simple drivers it has it's uses. But more layers feels like going in the wrong direction.
For the callback you're looking for (i.e. the regulator_disable call) I think the correct place is to enable/disable the regulator in the enable/disable hooks of the drm_simple_display_pipe functions. Or maybe in their equivalent in drm_panel (well, probably pre_enable and post_disable, since I guess you need that regulator to driver anything). Same for _init, if the display is completely off there's no point in keeping the hw running. Enabling/disabling the entire hw is pretty much what ->enable and ->disable are for. This also gives you proper runtime pm for almost for free ...
Also, since the regulator is actually stored in struct mipi_dbi, it's probably best to handle it in the mipi_dbi helpers too. You do that already with the backlight anyway.
I noticed that the version of tinydrm that landed doesn't use drm_panel anymore, I thought that was the case once, and for the version I acked?
After having done this the question arises: Why not extend tinydrm_device instead of subclassing it?
The benefit of subclassing is that it keeps the door open for drivers that can use tinydrm_device, but not tinydrm_panel. But I don't know of such a driver now, then again who knows what the future brings. Something that might or might not happen isn't a good reason, so it seems that I want it this way because I just like it. And it's easy to merge the two should it be that no one uses tinydrm_device directly three years down the line. But I'm actually not sure what's best.
As a rule of thumb, never design code for future use that you don't know yet will happen. No one can reliably predict the future, which means you'll get it wrong, and in the future we'll have to do 2x the work: Once do unto the code resulting from the wrong prediction, then redoing it the way we need to. Not including the on-going burden of maintaining unused functionally.
So let's pls merge first, split later when there's a clean need.
To recap:
tinydrm_device
- Combines drm_simple_display_pipe with CMA backed framebuffer and fbdev.
- Optional pipe setup with a connector with one mode, but the driver can do it's own.
tinydrm_panel
- All drm operations are distilled down to tinydrm_panel_funcs.
- Some common driver properties
So overal sorry I'm shredding this a bit, but I don't see the point. Imo much more useful would be:
- Extract the non-tinydrm specific helpers from tinydrm and put them into
the right places. I think from our discussions this was:
backlight helpers, probably best to put them into a new drm_backlight.c. This is because drivers/video is defactor unmaintained. We could also move drivers/video/backlight to drivers/gpu/backlight and take it all over within drm-misc, but that's more work.
spi helpers, probably best put into spi core/helper code. Thierry said the spi maintainer is fast&reactive, so shouldn't be a big issue.
extract the mipi-dbi helper (well, the non-tinydrm specific parts at least) into a separate helper, like we have for mipi-dsi already.
A large chunk of the tinydrm functions should probably be moved into relevant existin drm helpers, e.g.
- tinydrm_lastclose could be drm_fb_helper_lastclose. Only thing we need for that is to store the drm_fb_helper pointer somewhere in drm_device->mode_config. And thenc we could roll that out to all the drivers.
- tinydrm_gem_cma_prime_import_sg_table should probably go into the cma helpers, as a _vmapped variant (since not every driver needs the vmap). And tinydrm_gem_cma_free_object could the be merged into drm_gem_cma_free_object().
- tinydrm_fb_create we could move into drm_simple_pipe, only need to add the fb_create hook there, which would again simplify a bunch of things (since it gives you a one-stop vfunc for simple drivers).
- Quick aside: The unregister devm stuff is kinda getting the lifetimes of a drm_device wrong. Doesn't matter, since everyone else gets it wrong too :-)
- With the fbdev pointer in dev->mode_config we could also make suspend/resume helpers entirely generic, at least if we add a dev->mode_config.suspend_state.
Just a bunch of ideas. If you don't feel like doing those, ok if I add them to todo.rst as tinydrm refactorings? -Daniel
- Add tons more panel drivers. Personally I'd do that first :-)
Cheers, Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Den 12.03.2017 19.55, skrev Daniel Vetter:
On Sun, Mar 12, 2017 at 06:50:17PM +0100, Daniel Vetter wrote:
Hi Noralf,
On Sat, Mar 11, 2017 at 10:35:31PM +0100, Noralf Trønnes wrote:
Add support for displays that have a register interface and can be operated using a simple vtable.
I have looked through the staging/fbtft drivers and it seems that, except the MIPI controllers, most if not all controllers are operated through a register. And since most controllers have more than one bus interface option, regmap seems like a good choice to describe the interface (tested[1,2]). MIPI DCS can't be represented using regmap since some commands doesn't have a parameter. That would be like a register without a value, which doesn't make sense.
In my second RFC of tinydrm I used drm_panel to decribe the panels since it was a good match to the fbtft displays. I was then told that drm_panel wasn't supposed to used like that, so I dropped it and have tried to use the drm_simple_display_pipe_funcs vtable directly. This hasn't been all successful, since I ended up using devm_add_action() to power down the controller at the right time. Thierry Reding wasn't happy with this and suggested "to add an explicit callback somewhere". My solution has been to copy the drm_panel_funcs vtable. Since I now have a vtable, I also added a callback to flush the framebuffer. So presumably all the fbtft drivers can now be operated through the tinydrm_panel_funcs vtable.
Ehrm, what? I admit I didn't follow the discussion in-depth, but if drm_panel can't be used to describe a panel, it's not fit for the job and needs to be extended. Adding even more abstraction, matroschka-style, isn't a solution.
Personally I think tinydrm itself is already a bit much wrapping, but I see that for really simple drivers it has it's uses. But more layers feels like going in the wrong direction.
For the callback you're looking for (i.e. the regulator_disable call) I think the correct place is to enable/disable the regulator in the enable/disable hooks of the drm_simple_display_pipe functions. Or maybe in their equivalent in drm_panel (well, probably pre_enable and post_disable, since I guess you need that regulator to driver anything). Same for _init, if the display is completely off there's no point in keeping the hw running. Enabling/disabling the entire hw is pretty much what ->enable and ->disable are for. This also gives you proper runtime pm for almost for free ...
Also, since the regulator is actually stored in struct mipi_dbi, it's probably best to handle it in the mipi_dbi helpers too. You do that already with the backlight anyway.
I noticed that the version of tinydrm that landed doesn't use drm_panel anymore, I thought that was the case once, and for the version I acked?
After having done this the question arises: Why not extend tinydrm_device instead of subclassing it?
The benefit of subclassing is that it keeps the door open for drivers that can use tinydrm_device, but not tinydrm_panel. But I don't know of such a driver now, then again who knows what the future brings. Something that might or might not happen isn't a good reason, so it seems that I want it this way because I just like it. And it's easy to merge the two should it be that no one uses tinydrm_device directly three years down the line. But I'm actually not sure what's best.
As a rule of thumb, never design code for future use that you don't know yet will happen. No one can reliably predict the future, which means you'll get it wrong, and in the future we'll have to do 2x the work: Once do unto the code resulting from the wrong prediction, then redoing it the way we need to. Not including the on-going burden of maintaining unused functionally.
So let's pls merge first, split later when there's a clean need.
To recap:
tinydrm_device
- Combines drm_simple_display_pipe with CMA backed framebuffer and fbdev.
- Optional pipe setup with a connector with one mode, but the driver can do it's own.
tinydrm_panel
- All drm operations are distilled down to tinydrm_panel_funcs.
- Some common driver properties
So overal sorry I'm shredding this a bit, but I don't see the point. Imo much more useful would be:
- Extract the non-tinydrm specific helpers from tinydrm and put them into
the right places. I think from our discussions this was:
backlight helpers, probably best to put them into a new drm_backlight.c. This is because drivers/video is defactor unmaintained. We could also move drivers/video/backlight to drivers/gpu/backlight and take it all over within drm-misc, but that's more work.
spi helpers, probably best put into spi core/helper code. Thierry said the spi maintainer is fast&reactive, so shouldn't be a big issue.
extract the mipi-dbi helper (well, the non-tinydrm specific parts at least) into a separate helper, like we have for mipi-dsi already.
A large chunk of the tinydrm functions should probably be moved into relevant existin drm helpers, e.g.
tinydrm_lastclose could be drm_fb_helper_lastclose. Only thing we need for that is to store the drm_fb_helper pointer somewhere in drm_device->mode_config. And thenc we could roll that out to all the drivers.
tinydrm_gem_cma_prime_import_sg_table should probably go into the cma helpers, as a _vmapped variant (since not every driver needs the vmap). And tinydrm_gem_cma_free_object could the be merged into drm_gem_cma_free_object().
tinydrm_fb_create we could move into drm_simple_pipe, only need to add the fb_create hook there, which would again simplify a bunch of things (since it gives you a one-stop vfunc for simple drivers).
Quick aside: The unregister devm stuff is kinda getting the lifetimes of a drm_device wrong. Doesn't matter, since everyone else gets it wrong too :-)
With the fbdev pointer in dev->mode_config we could also make suspend/resume helpers entirely generic, at least if we add a dev->mode_config.suspend_state.
Just a bunch of ideas. If you don't feel like doing those, ok if I add them to todo.rst as tinydrm refactorings?
Please add them to todo.rst.
I have a fatigue illness that has been getting worse the last months, which means that I have to cut back on programming to not wreck the rest of my waking hours. Up until now I have primarily needed to keep my physical activity to a minimum, but computer work hasn't been that much affected. But double sadly computer work is now also something that affects my energy levels in a major way. A problem with this illness is that the full effect of energy overuse comes after a few days, making it very difficult to find a balance. And with programming being so much fun it will be a challenge to slow down enough.
So until I find this new energy balance, I don't know how much time I can spend on tinydrm.
Noralf.
-Daniel
- Add tons more panel drivers. Personally I'd do that first :-)
Cheers, Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Sun, Mar 12, 2017 at 06:50:17PM +0100, Daniel Vetter wrote:
Hi Noralf,
On Sat, Mar 11, 2017 at 10:35:31PM +0100, Noralf Trønnes wrote:
Add support for displays that have a register interface and can be operated using a simple vtable.
I have looked through the staging/fbtft drivers and it seems that, except the MIPI controllers, most if not all controllers are operated through a register. And since most controllers have more than one bus interface option, regmap seems like a good choice to describe the interface (tested[1,2]). MIPI DCS can't be represented using regmap since some commands doesn't have a parameter. That would be like a register without a value, which doesn't make sense.
In my second RFC of tinydrm I used drm_panel to decribe the panels since it was a good match to the fbtft displays. I was then told that drm_panel wasn't supposed to used like that, so I dropped it and have tried to use the drm_simple_display_pipe_funcs vtable directly. This hasn't been all successful, since I ended up using devm_add_action() to power down the controller at the right time. Thierry Reding wasn't happy with this and suggested "to add an explicit callback somewhere". My solution has been to copy the drm_panel_funcs vtable. Since I now have a vtable, I also added a callback to flush the framebuffer. So presumably all the fbtft drivers can now be operated through the tinydrm_panel_funcs vtable.
Ehrm, what? I admit I didn't follow the discussion in-depth, but if drm_panel can't be used to describe a panel, it's not fit for the job and needs to be extended. Adding even more abstraction, matroschka-style, isn't a solution.
Personally I think tinydrm itself is already a bit much wrapping, but I see that for really simple drivers it has it's uses. But more layers feels like going in the wrong direction.
For the callback you're looking for (i.e. the regulator_disable call) I think the correct place is to enable/disable the regulator in the enable/disable hooks of the drm_simple_display_pipe functions. Or maybe in their equivalent in drm_panel (well, probably pre_enable and post_disable, since I guess you need that regulator to driver anything). Same for _init, if the display is completely off there's no point in keeping the hw running. Enabling/disabling the entire hw is pretty much what ->enable and ->disable are for. This also gives you proper runtime pm for almost for free ...
Also, since the regulator is actually stored in struct mipi_dbi, it's probably best to handle it in the mipi_dbi helpers too. You do that already with the backlight anyway.
I noticed that the version of tinydrm that landed doesn't use drm_panel anymore, I thought that was the case once, and for the version I acked?
Self-correct, there never was a version with drm_panel. tbh I think that's perfectly fine, tinydrm is aimed at simple panels behind spi/i2c buses (where also the entire video data is uploaded through spi/i2c, not just control information). Not changing anything like I recommend seems like the right action still (well, shuffling the regulator into simple_pipe->enable/disable like I think it should be). -Daniel
Den 12.03.2017 20.16, skrev Daniel Vetter:
On Sun, Mar 12, 2017 at 06:50:17PM +0100, Daniel Vetter wrote:
Hi Noralf,
On Sat, Mar 11, 2017 at 10:35:31PM +0100, Noralf Trønnes wrote:
Add support for displays that have a register interface and can be operated using a simple vtable.
I have looked through the staging/fbtft drivers and it seems that, except the MIPI controllers, most if not all controllers are operated through a register. And since most controllers have more than one bus interface option, regmap seems like a good choice to describe the interface (tested[1,2]). MIPI DCS can't be represented using regmap since some commands doesn't have a parameter. That would be like a register without a value, which doesn't make sense.
In my second RFC of tinydrm I used drm_panel to decribe the panels since it was a good match to the fbtft displays. I was then told that drm_panel wasn't supposed to used like that, so I dropped it and have tried to use the drm_simple_display_pipe_funcs vtable directly. This hasn't been all successful, since I ended up using devm_add_action() to power down the controller at the right time. Thierry Reding wasn't happy with this and suggested "to add an explicit callback somewhere". My solution has been to copy the drm_panel_funcs vtable. Since I now have a vtable, I also added a callback to flush the framebuffer. So presumably all the fbtft drivers can now be operated through the tinydrm_panel_funcs vtable.
Ehrm, what? I admit I didn't follow the discussion in-depth, but if drm_panel can't be used to describe a panel, it's not fit for the job and needs to be extended. Adding even more abstraction, matroschka-style, isn't a solution.
Personally I think tinydrm itself is already a bit much wrapping, but I see that for really simple drivers it has it's uses. But more layers feels like going in the wrong direction.
For the callback you're looking for (i.e. the regulator_disable call) I think the correct place is to enable/disable the regulator in the enable/disable hooks of the drm_simple_display_pipe functions. Or maybe in their equivalent in drm_panel (well, probably pre_enable and post_disable, since I guess you need that regulator to driver anything). Same for _init, if the display is completely off there's no point in keeping the hw running. Enabling/disabling the entire hw is pretty much what ->enable and ->disable are for. This also gives you proper runtime pm for almost for free ...
Also, since the regulator is actually stored in struct mipi_dbi, it's probably best to handle it in the mipi_dbi helpers too. You do that already with the backlight anyway.
I noticed that the version of tinydrm that landed doesn't use drm_panel anymore, I thought that was the case once, and for the version I acked?
Self-correct, there never was a version with drm_panel. tbh I think that's perfectly fine, tinydrm is aimed at simple panels behind spi/i2c buses (where also the entire video data is uploaded through spi/i2c, not just control information). Not changing anything like I recommend seems like the right action still (well, shuffling the regulator into simple_pipe->enable/disable like I think it should be).
I have looked at the emails, and I used drm_panel in the first RFC, but I got the impression that Thierry didn't like it so it was dropped in RFC v2.
The reason for making this patchset was to solve a problem of power management that Thierry pointed out in the mi0283qt driver where I use devm_add_action() to disable the regulator on module/device unload. I haven't found a way to do PM in the simple drm pipeline.
I use drm_simple_display_pipe.enable to enable backlight since it's called after drm_simple_display_pipe.update. If it was called before, then I could use it to prepare the panel/controller. I remember having seen some comments in the atomic code about reordering something to make it match PM better. But if .enable() could be called before .update(), how then do I control backlight?
I need to first power on and configure the controller, then it can receive the initial framebuffer, and lastly the backlight is enabled.
No problem with you shredding this, if I can do this with much less code, then all the better.
Thank you for spending time on this.
Noralf.
On Sun, Mar 12, 2017 at 09:17:00PM +0100, Noralf Trønnes wrote:
Den 12.03.2017 20.16, skrev Daniel Vetter:
On Sun, Mar 12, 2017 at 06:50:17PM +0100, Daniel Vetter wrote:
Hi Noralf,
On Sat, Mar 11, 2017 at 10:35:31PM +0100, Noralf Trønnes wrote:
Add support for displays that have a register interface and can be operated using a simple vtable.
I have looked through the staging/fbtft drivers and it seems that, except the MIPI controllers, most if not all controllers are operated through a register. And since most controllers have more than one bus interface option, regmap seems like a good choice to describe the interface (tested[1,2]). MIPI DCS can't be represented using regmap since some commands doesn't have a parameter. That would be like a register without a value, which doesn't make sense.
In my second RFC of tinydrm I used drm_panel to decribe the panels since it was a good match to the fbtft displays. I was then told that drm_panel wasn't supposed to used like that, so I dropped it and have tried to use the drm_simple_display_pipe_funcs vtable directly. This hasn't been all successful, since I ended up using devm_add_action() to power down the controller at the right time. Thierry Reding wasn't happy with this and suggested "to add an explicit callback somewhere". My solution has been to copy the drm_panel_funcs vtable. Since I now have a vtable, I also added a callback to flush the framebuffer. So presumably all the fbtft drivers can now be operated through the tinydrm_panel_funcs vtable.
Ehrm, what? I admit I didn't follow the discussion in-depth, but if drm_panel can't be used to describe a panel, it's not fit for the job and needs to be extended. Adding even more abstraction, matroschka-style, isn't a solution.
Personally I think tinydrm itself is already a bit much wrapping, but I see that for really simple drivers it has it's uses. But more layers feels like going in the wrong direction.
For the callback you're looking for (i.e. the regulator_disable call) I think the correct place is to enable/disable the regulator in the enable/disable hooks of the drm_simple_display_pipe functions. Or maybe in their equivalent in drm_panel (well, probably pre_enable and post_disable, since I guess you need that regulator to driver anything). Same for _init, if the display is completely off there's no point in keeping the hw running. Enabling/disabling the entire hw is pretty much what ->enable and ->disable are for. This also gives you proper runtime pm for almost for free ...
Also, since the regulator is actually stored in struct mipi_dbi, it's probably best to handle it in the mipi_dbi helpers too. You do that already with the backlight anyway.
I noticed that the version of tinydrm that landed doesn't use drm_panel anymore, I thought that was the case once, and for the version I acked?
Self-correct, there never was a version with drm_panel. tbh I think that's perfectly fine, tinydrm is aimed at simple panels behind spi/i2c buses (where also the entire video data is uploaded through spi/i2c, not just control information). Not changing anything like I recommend seems like the right action still (well, shuffling the regulator into simple_pipe->enable/disable like I think it should be).
I have looked at the emails, and I used drm_panel in the first RFC, but I got the impression that Thierry didn't like it so it was dropped in RFC v2.
Hm, I thought I checked all the old versions of your example tinydrm submissions and didn't find any with drm_panel. Do you have a link to archives? I'd like to read Thierry's aguments, in case I'm oblivious to a bad corner case :-)
The reason for making this patchset was to solve a problem of power management that Thierry pointed out in the mi0283qt driver where I use devm_add_action() to disable the regulator on module/device unload. I haven't found a way to do PM in the simple drm pipeline.
I use drm_simple_display_pipe.enable to enable backlight since it's called after drm_simple_display_pipe.update. If it was called before, then I could use it to prepare the panel/controller. I remember having seen some comments in the atomic code about reordering something to make it match PM better. But if .enable() could be called before .update(), how then do I control backlight?
So what everyone else does is enable the backlight in ->enable (with the screen just displaying black) and updating the screen contents in ->update afterwards. That's what the comment in the docs about reordering stuff to make it better fit with runtime PM.
If you don't like that for tinydrm, you can insert a call to ->update in your ->enable. Slightly redundant, but then enabling a screen is not the fastest thing so not much problem if you're inefficient. And you could still fix that with a special case in ->update, but really not sure this is worth it.
Once the screen is on you just get calls to ->update, so then it doesn't matter anymore.
And with this ordering you should be able to stuff the regulator calls into ->enable. On the disable side the same thing, but inverse ordering.
I need to first power on and configure the controller, then it can receive the initial framebuffer, and lastly the backlight is enabled.
No problem with you shredding this, if I can do this with much less code, then all the better.
Yeah, I think we don't need more abstraction here, with atomic helpers, simple_pipe helpers and tinydrm there's enough.
And even if there is eventually a real gpu reusing these panels in their own IP (but still over an spi/i2c bus ofc), then I think we can always reuse at the drm_device level: The gpu ip exposes the spi/i2c bus (plus all the dt entries needed), and the relevant tinydrm driver binds against that. Compositor ties it all together with buffer sharing.
So going over the top with making code shareable with drm_panel probably won't ever be needed anyway. This might change if there's an spi/dbi encoder chip which does it all in hw ... -Daniel
Den 12.03.2017 21.40, skrev Daniel Vetter:
On Sun, Mar 12, 2017 at 09:17:00PM +0100, Noralf Trønnes wrote:
Den 12.03.2017 20.16, skrev Daniel Vetter:
On Sun, Mar 12, 2017 at 06:50:17PM +0100, Daniel Vetter wrote:
Hi Noralf,
On Sat, Mar 11, 2017 at 10:35:31PM +0100, Noralf Trønnes wrote:
Add support for displays that have a register interface and can be operated using a simple vtable.
I have looked through the staging/fbtft drivers and it seems that, except the MIPI controllers, most if not all controllers are operated through a register. And since most controllers have more than one bus interface option, regmap seems like a good choice to describe the interface (tested[1,2]). MIPI DCS can't be represented using regmap since some commands doesn't have a parameter. That would be like a register without a value, which doesn't make sense.
In my second RFC of tinydrm I used drm_panel to decribe the panels since it was a good match to the fbtft displays. I was then told that drm_panel wasn't supposed to used like that, so I dropped it and have tried to use the drm_simple_display_pipe_funcs vtable directly. This hasn't been all successful, since I ended up using devm_add_action() to power down the controller at the right time. Thierry Reding wasn't happy with this and suggested "to add an explicit callback somewhere". My solution has been to copy the drm_panel_funcs vtable. Since I now have a vtable, I also added a callback to flush the framebuffer. So presumably all the fbtft drivers can now be operated through the tinydrm_panel_funcs vtable.
Ehrm, what? I admit I didn't follow the discussion in-depth, but if drm_panel can't be used to describe a panel, it's not fit for the job and needs to be extended. Adding even more abstraction, matroschka-style, isn't a solution.
Personally I think tinydrm itself is already a bit much wrapping, but I see that for really simple drivers it has it's uses. But more layers feels like going in the wrong direction.
For the callback you're looking for (i.e. the regulator_disable call) I think the correct place is to enable/disable the regulator in the enable/disable hooks of the drm_simple_display_pipe functions. Or maybe in their equivalent in drm_panel (well, probably pre_enable and post_disable, since I guess you need that regulator to driver anything). Same for _init, if the display is completely off there's no point in keeping the hw running. Enabling/disabling the entire hw is pretty much what ->enable and ->disable are for. This also gives you proper runtime pm for almost for free ...
Also, since the regulator is actually stored in struct mipi_dbi, it's probably best to handle it in the mipi_dbi helpers too. You do that already with the backlight anyway.
I noticed that the version of tinydrm that landed doesn't use drm_panel anymore, I thought that was the case once, and for the version I acked?
Self-correct, there never was a version with drm_panel. tbh I think that's perfectly fine, tinydrm is aimed at simple panels behind spi/i2c buses (where also the entire video data is uploaded through spi/i2c, not just control information). Not changing anything like I recommend seems like the right action still (well, shuffling the regulator into simple_pipe->enable/disable like I think it should be).
I have looked at the emails, and I used drm_panel in the first RFC, but I got the impression that Thierry didn't like it so it was dropped in RFC v2.
Hm, I thought I checked all the old versions of your example tinydrm submissions and didn't find any with drm_panel. Do you have a link to archives? I'd like to read Thierry's aguments, in case I'm oblivious to a bad corner case :-)
I used drm_panel in the first tinydrm RFC in March 2016: https://lists.freedesktop.org/archives/dri-devel/2016-March/102903.html
struct tinydrm_device { struct drm_device *base; struct drm_panel panel; ... };
Then you commented: https://lists.freedesktop.org/archives/dri-devel/2016-March/102921.html
In the case of tinydrm I think that means we should have a bunch of new drm helpers, or extensions for existing ones:
<snip>
- A helper to create a simple drm_connector from a drm_panel (the get_modes hooks you have here), maybe also in drm_simple_kms_helper.c.
So I made: [PATCH 4/4] drm/panel: Add helper for simple panel connector https://lists.freedesktop.org/archives/dri-devel/2016-May/106890.html
Thierry replied: https://lists.freedesktop.org/archives/dri-devel/2016-May/107023.html
Okay, that gives me a better understanding of where things are headed. That said, why would these devices even need to deal with drm_panel in the first place? Sounds to me like they are devices on some sort of control bus that you talk to directly. And they will represent essentially the panel with its built-in display controller.
drm_panel on the other hand was designed as an interface between display controllers and panels, with the goal to let any display controller talk to any panel.
While I'm sure you can support these types of simple panels using the drm_panel infrastructure I'm not sure it's necessary, since your driver will always talk to the panel directly, and hence the code to deal with the panel specifics could be part of the display pipe functions.
The reason for making this patchset was to solve a problem of power management that Thierry pointed out in the mi0283qt driver where I use devm_add_action() to disable the regulator on module/device unload. I haven't found a way to do PM in the simple drm pipeline.
I use drm_simple_display_pipe.enable to enable backlight since it's called after drm_simple_display_pipe.update. If it was called before, then I could use it to prepare the panel/controller. I remember having seen some comments in the atomic code about reordering something to make it match PM better. But if .enable() could be called before .update(), how then do I control backlight?
So what everyone else does is enable the backlight in ->enable (with the screen just displaying black) and updating the screen contents in ->update afterwards. That's what the comment in the docs about reordering stuff to make it better fit with runtime PM.
If you don't like that for tinydrm, you can insert a call to ->update in your ->enable. Slightly redundant, but then enabling a screen is not the fastest thing so not much problem if you're inefficient. And you could still fix that with a special case in ->update, but really not sure this is worth it.
Once the screen is on you just get calls to ->update, so then it doesn't matter anymore.
And with this ordering you should be able to stuff the regulator calls into ->enable. On the disable side the same thing, but inverse ordering.
Thanks, I'll try that.
Noralf.
On Mon, Mar 13, 2017 at 04:12:37PM +0100, Noralf Trønnes wrote:
Den 12.03.2017 21.40, skrev Daniel Vetter:
On Sun, Mar 12, 2017 at 09:17:00PM +0100, Noralf Trønnes wrote:
Den 12.03.2017 20.16, skrev Daniel Vetter:
On Sun, Mar 12, 2017 at 06:50:17PM +0100, Daniel Vetter wrote:
Hi Noralf,
On Sat, Mar 11, 2017 at 10:35:31PM +0100, Noralf Trønnes wrote:
Add support for displays that have a register interface and can be operated using a simple vtable.
I have looked through the staging/fbtft drivers and it seems that, except the MIPI controllers, most if not all controllers are operated through a register. And since most controllers have more than one bus interface option, regmap seems like a good choice to describe the interface (tested[1,2]). MIPI DCS can't be represented using regmap since some commands doesn't have a parameter. That would be like a register without a value, which doesn't make sense.
In my second RFC of tinydrm I used drm_panel to decribe the panels since it was a good match to the fbtft displays. I was then told that drm_panel wasn't supposed to used like that, so I dropped it and have tried to use the drm_simple_display_pipe_funcs vtable directly. This hasn't been all successful, since I ended up using devm_add_action() to power down the controller at the right time. Thierry Reding wasn't happy with this and suggested "to add an explicit callback somewhere". My solution has been to copy the drm_panel_funcs vtable. Since I now have a vtable, I also added a callback to flush the framebuffer. So presumably all the fbtft drivers can now be operated through the tinydrm_panel_funcs vtable.
Ehrm, what? I admit I didn't follow the discussion in-depth, but if drm_panel can't be used to describe a panel, it's not fit for the job and needs to be extended. Adding even more abstraction, matroschka-style, isn't a solution.
Personally I think tinydrm itself is already a bit much wrapping, but I see that for really simple drivers it has it's uses. But more layers feels like going in the wrong direction.
For the callback you're looking for (i.e. the regulator_disable call) I think the correct place is to enable/disable the regulator in the enable/disable hooks of the drm_simple_display_pipe functions. Or maybe in their equivalent in drm_panel (well, probably pre_enable and post_disable, since I guess you need that regulator to driver anything). Same for _init, if the display is completely off there's no point in keeping the hw running. Enabling/disabling the entire hw is pretty much what ->enable and ->disable are for. This also gives you proper runtime pm for almost for free ...
Also, since the regulator is actually stored in struct mipi_dbi, it's probably best to handle it in the mipi_dbi helpers too. You do that already with the backlight anyway.
I noticed that the version of tinydrm that landed doesn't use drm_panel anymore, I thought that was the case once, and for the version I acked?
Self-correct, there never was a version with drm_panel. tbh I think that's perfectly fine, tinydrm is aimed at simple panels behind spi/i2c buses (where also the entire video data is uploaded through spi/i2c, not just control information). Not changing anything like I recommend seems like the right action still (well, shuffling the regulator into simple_pipe->enable/disable like I think it should be).
I have looked at the emails, and I used drm_panel in the first RFC, but I got the impression that Thierry didn't like it so it was dropped in RFC v2.
Hm, I thought I checked all the old versions of your example tinydrm submissions and didn't find any with drm_panel. Do you have a link to archives? I'd like to read Thierry's aguments, in case I'm oblivious to a bad corner case :-)
I used drm_panel in the first tinydrm RFC in March 2016: https://lists.freedesktop.org/archives/dri-devel/2016-March/102903.html
struct tinydrm_device { struct drm_device *base; struct drm_panel panel; ... };
Then you commented: https://lists.freedesktop.org/archives/dri-devel/2016-March/102921.html
In the case of tinydrm I think that means we should have a bunch of new drm helpers, or extensions for existing ones:
<snip> > - A helper to create a simple drm_connector from a drm_panel (the > get_modes hooks you have here), maybe also in drm_simple_kms_helper.c.
So I made: [PATCH 4/4] drm/panel: Add helper for simple panel connector https://lists.freedesktop.org/archives/dri-devel/2016-May/106890.html
Thierry replied: https://lists.freedesktop.org/archives/dri-devel/2016-May/107023.html
Ugh, that's bad, review has come full circle :(
Given that I'd say let's not bother more with panel frameworks for now, but just add the simple panel drivers as-is. Once we have someone who wants to reuse a panel (probably a mipi-dbi one), but where the dbi bus isn't exposed to software but behind some fancy hw encoder, then we can ponder whether/how (and probably just for the affected drivers) we should re-introduce drm_panel into tinydrm.
I'll try and collect everything we've discussed here into a tinydrm todo.rst entry. -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Michel Dänzer -
Noralf Trønnes