[PATCH 0/5] drm/tinydrm: Add tinydrm_panel abstraction

[PATCH 0/5] drm/tinydrm: Add tinydrm_panel abstraction

[Noralf Trønnes]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Noralf Trønnes]

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);
-- 
2.10.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[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?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Noralf Trønnes]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Mon, Mar 13, 2017 at 01:30:40PM +0100, Noralf Trønnes wrote:
> 
> Den 12.03.2017 19.00, skrev Daniel Vetter:
> > On Sat, Mar 11, 2017 at 10:35:32PM +0100, Noralf Trønnes wrote:
> > > Add tinydrm_rgb565_buf_copy() function that copies buffer rectangle to
> > > destination buffer and also handles XRGB8888 and byte swap conversions.
> > > Useful for displays that only support RGB565 and can do partial updates.
> > > 
> > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > ---
> > >   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 56 +++++++++++++++++++++++++-
> > >   include/drm/tinydrm/tinydrm-helpers.h          |  2 +
> > >   2 files changed, 56 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > > index d4cda33..e639453 100644
> > > --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > > @@ -7,13 +7,15 @@
> > >    * (at your option) any later version.
> > >    */
> > > -#include <drm/tinydrm/tinydrm.h>
> > > -#include <drm/tinydrm/tinydrm-helpers.h>
> > >   #include <linux/backlight.h>
> > > +#include <linux/dma-buf.h>
> > >   #include <linux/pm.h>
> > >   #include <linux/spi/spi.h>
> > >   #include <linux/swab.h>
> > > +#include <drm/tinydrm/tinydrm.h>
> > > +#include <drm/tinydrm/tinydrm-helpers.h>
> > > +
> > >   static unsigned int spi_max;
> > >   module_param(spi_max, uint, 0400);
> > >   MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size");
> > > @@ -181,6 +183,56 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
> > >   EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
> > So I noticed that we already have the xrgb8888 to rgb565 function, so I'm
> > a bit late on this, but: DRM doesn't do format conversions, with the
> > single exception that the legacy cursor interface is specced to be
> > argb8888.
> > 
> > Imo this should be removed (and preferrably before we ship tinydrm in a
> > stable kernel). Why did you add it?
> 
> I added it from the start because plymouth can only do xrgb8888 and I
> thought that this was probably the format that most apps/libs/tools
> supported, ensuring that tinydrm would work with everything. But I was
> aware that this was the kernel patching up userspace, so I knew that it
> might be shot down.
> 
> But after your comment, I thought that this was in the clear:
> https://lists.freedesktop.org/archives/dri-devel/2017-January/130551.html
> 
> > > +EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
> >
> > I wonder whether the above would make sense in drm core as some kind of fb
> > helpers. But we can do that once there's a clear need.
> 
> I can make a patch that removes this format conversion.

I have no idea what I thought back then :-) But most likely I slightly
misread it as argb8888_to_rgb565 (it works the same really) used for
cursor compat, which is ok-ish.

But then I just looked through all drivers, and I found exactly one driver
which doesn't support XRGB8888, and that was probably an oversight. So
there's some arguments for always supporting that. Otoh if you do buffer
sharing and have a nice hw spi engine, touching a wc buffer with the cpu
is _real_ slow (because of the uncached reads), so we really shouldn't let
userspace stumble over this pitfall by accident. The trouble is that by
exposing both, most userspace will pick XRGB8888, even when they support
RGB565.

And uncached reads are as a rule of thumb 1000x slower, so you don't need
a big panel to feel the pain.

Given that I think we should remove the fake XRGB8888 support.

Wrt plymouth, I'm a bit surprised that one falls over: We have a pile of
chips that (in some circumstances) prefer RGB565 (mostly big resolutions
with little vram), that's controlled by the preferred_bpp parameter of
drm_fb_helper_single_fb_probe(). You seem to have wired that up all
correctly, plymouth still fails?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Noralf Trønnes]

Den 14.03.2017 08.17, skrev Daniel Vetter:
> On Mon, Mar 13, 2017 at 01:30:40PM +0100, Noralf Trønnes wrote:
>> Den 12.03.2017 19.00, skrev Daniel Vetter:
>>> On Sat, Mar 11, 2017 at 10:35:32PM +0100, Noralf Trønnes wrote:
>>>> Add tinydrm_rgb565_buf_copy() function that copies buffer rectangle to
>>>> destination buffer and also handles XRGB8888 and byte swap conversions.
>>>> Useful for displays that only support RGB565 and can do partial updates.
>>>>
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>> ---
>>>>    drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 56 +++++++++++++++++++++++++-
>>>>    include/drm/tinydrm/tinydrm-helpers.h          |  2 +
>>>>    2 files changed, 56 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>>> index d4cda33..e639453 100644
>>>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>>> @@ -7,13 +7,15 @@
>>>>     * (at your option) any later version.
>>>>     */
>>>> -#include <drm/tinydrm/tinydrm.h>
>>>> -#include <drm/tinydrm/tinydrm-helpers.h>
>>>>    #include <linux/backlight.h>
>>>> +#include <linux/dma-buf.h>
>>>>    #include <linux/pm.h>
>>>>    #include <linux/spi/spi.h>
>>>>    #include <linux/swab.h>
>>>> +#include <drm/tinydrm/tinydrm.h>
>>>> +#include <drm/tinydrm/tinydrm-helpers.h>
>>>> +
>>>>    static unsigned int spi_max;
>>>>    module_param(spi_max, uint, 0400);
>>>>    MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size");
>>>> @@ -181,6 +183,56 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
>>>>    EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
>>> So I noticed that we already have the xrgb8888 to rgb565 function, so I'm
>>> a bit late on this, but: DRM doesn't do format conversions, with the
>>> single exception that the legacy cursor interface is specced to be
>>> argb8888.
>>>
>>> Imo this should be removed (and preferrably before we ship tinydrm in a
>>> stable kernel). Why did you add it?
>> I added it from the start because plymouth can only do xrgb8888 and I
>> thought that this was probably the format that most apps/libs/tools
>> supported, ensuring that tinydrm would work with everything. But I was
>> aware that this was the kernel patching up userspace, so I knew that it
>> might be shot down.
>>
>> But after your comment, I thought that this was in the clear:
>> https://lists.freedesktop.org/archives/dri-devel/2017-January/130551.html
>>
>>>> +EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
>>> I wonder whether the above would make sense in drm core as some kind of fb
>>> helpers. But we can do that once there's a clear need.
>> I can make a patch that removes this format conversion.
> I have no idea what I thought back then :-) But most likely I slightly
> misread it as argb8888_to_rgb565 (it works the same really) used for
> cursor compat, which is ok-ish.
>
> But then I just looked through all drivers, and I found exactly one driver
> which doesn't support XRGB8888, and that was probably an oversight. So
> there's some arguments for always supporting that. Otoh if you do buffer
> sharing and have a nice hw spi engine, touching a wc buffer with the cpu
> is _real_ slow (because of the uncached reads), so we really shouldn't let
> userspace stumble over this pitfall by accident. The trouble is that by
> exposing both, most userspace will pick XRGB8888, even when they support
> RGB565.
>
> And uncached reads are as a rule of thumb 1000x slower, so you don't need
> a big panel to feel the pain.
>
> Given that I think we should remove the fake XRGB8888 support.

The Raspberry Pi, which is by far the largest user of these displays,
has a DMA capable SPI controller that can only do 8-bit words.
Since it is little endian, 16-bit RGB565 has to be byte swapped using
a ordinary kmalloc buffer.

Theoretical maximum at 48MHz is:
1 / (320 * 240 * 16 * (1/48000000.0)) = 39.0625Hz

The actual numbers isn't very far off:

$ modetest -M "mi0283qt" -s 25:320x240@RG16 -v
setting mode 320x240-0Hz@RG16 on connectors 25, crtc 27
freq: 39.36Hz
freq: 31.16Hz
freq: 31.21Hz

$ modetest -M "mi0283qt" -s 25:320x240@XR24 -v
setting mode 320x240-0Hz@XR24 on connectors 25, crtc 27
freq: 37.30Hz
freq: 29.76Hz
freq: 29.84Hz


Disabling byte swapping, passing cma_obj->vaddr through to SPI, doesn't
improve the numbers much:

$ modetest -M "mi0283qt" -s 25:320x240@RG16 -v
setting mode 320x240-0Hz@RG16 on connectors 25, crtc 27
freq: 43.90Hz
freq: 33.49Hz
freq: 33.49Hz

The SPI bus is sooo slow that the cpu can jump through all kinds of
hoops without affecting throughput much.

> Wrt plymouth, I'm a bit surprised that one falls over: We have a pile of
> chips that (in some circumstances) prefer RGB565 (mostly big resolutions
> with little vram), that's controlled by the preferred_bpp parameter of
> drm_fb_helper_single_fb_probe(). You seem to have wired that up all
> correctly, plymouth still fails?

I tried to get plymouth to work on the Raspberry Pi, but gave up.
I couldn't get it to use the display.
But here's an extract from the plymouth code:

create_output_buffer():

         if (drmModeAddFB (backend->device_fd, width, height,
                           24, 32, buffer->row_stride, buffer->handle,
                           &buffer->id) != 0) {

bpp=32 and depth=24 -> DRM_FORMAT_XRGB8888

And the has_32bpp_support() function makes it clear that 32bpp is required.

https://cgit.freedesktop.org/plymouth/tree/src/plugins/renderers/drm/plugin.c


Noralf.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

> Theoretical maximum at 48MHz is:
> 1 / (320 * 240 * 16 * (1/48000000.0)) = 39.0625Hz
> 
> The actual numbers isn't very far off:
> 
> $ modetest -M "mi0283qt" -s 25:320x240@RG16 -v
> setting mode 320x240-0Hz@RG16 on connectors 25, crtc 27
> freq: 39.36Hz
> freq: 31.16Hz
> freq: 31.21Hz
> 
> $ modetest -M "mi0283qt" -s 25:320x240@XR24 -v
> setting mode 320x240-0Hz@XR24 on connectors 25, crtc 27
> freq: 37.30Hz
> freq: 29.76Hz
> freq: 29.84Hz
> 
> 
> Disabling byte swapping, passing cma_obj->vaddr through to SPI, doesn't
> improve the numbers much:
> 
> $ modetest -M "mi0283qt" -s 25:320x240@RG16 -v
> setting mode 320x240-0Hz@RG16 on connectors 25, crtc 27
> freq: 43.90Hz
> freq: 33.49Hz
> freq: 33.49Hz
> 
> The SPI bus is sooo slow that the cpu can jump through all kinds of
> hoops without affecting throughput much.

Hey, 4 more frames! But in either case, you'll probably get much faster if
you offload the upload work to an async worker. Converting ->dirty to use
atomic might help a lot here, since we could try to stall only to the
previous frame, and not be entirely synchronous.

> > Wrt plymouth, I'm a bit surprised that one falls over: We have a pile of
> > chips that (in some circumstances) prefer RGB565 (mostly big resolutions
> > with little vram), that's controlled by the preferred_bpp parameter of
> > drm_fb_helper_single_fb_probe(). You seem to have wired that up all
> > correctly, plymouth still fails?
> 
> I tried to get plymouth to work on the Raspberry Pi, but gave up.
> I couldn't get it to use the display.
> But here's an extract from the plymouth code:
> 
> create_output_buffer():
> 
>         if (drmModeAddFB (backend->device_fd, width, height,
>                           24, 32, buffer->row_stride, buffer->handle,
>                           &buffer->id) != 0) {
> 
> bpp=32 and depth=24 -> DRM_FORMAT_XRGB8888
> 
> And the has_32bpp_support() function makes it clear that 32bpp is required.
> 
> https://cgit.freedesktop.org/plymouth/tree/src/plugins/renderers/drm/plugin.c

Blergh. Oh well, I guess we should just accept that userspace developers
are lazy :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Noralf Trønnes]

Den 15.03.2017 13.39, skrev Daniel Vetter:
> On Wed, Mar 15, 2017 at 01:15:37PM +0100, Noralf Trønnes wrote:
>> Den 14.03.2017 08.17, skrev Daniel Vetter:
>>> On Mon, Mar 13, 2017 at 01:30:40PM +0100, Noralf Trønnes wrote:
>>>> Den 12.03.2017 19.00, skrev Daniel Vetter:
>>>>> On Sat, Mar 11, 2017 at 10:35:32PM +0100, Noralf Trønnes wrote:
>>>>>> Add tinydrm_rgb565_buf_copy() function that copies buffer rectangle to
>>>>>> destination buffer and also handles XRGB8888 and byte swap conversions.
>>>>>> Useful for displays that only support RGB565 and can do partial updates.
>>>>>>
>>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>>> ---
>>>>>>     drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 56 +++++++++++++++++++++++++-
>>>>>>     include/drm/tinydrm/tinydrm-helpers.h          |  2 +
>>>>>>     2 files changed, 56 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>>>>> index d4cda33..e639453 100644
>>>>>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>>>>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>>>>> @@ -7,13 +7,15 @@
>>>>>>      * (at your option) any later version.
>>>>>>      */
>>>>>> -#include <drm/tinydrm/tinydrm.h>
>>>>>> -#include <drm/tinydrm/tinydrm-helpers.h>
>>>>>>     #include <linux/backlight.h>
>>>>>> +#include <linux/dma-buf.h>
>>>>>>     #include <linux/pm.h>
>>>>>>     #include <linux/spi/spi.h>
>>>>>>     #include <linux/swab.h>
>>>>>> +#include <drm/tinydrm/tinydrm.h>
>>>>>> +#include <drm/tinydrm/tinydrm-helpers.h>
>>>>>> +
>>>>>>     static unsigned int spi_max;
>>>>>>     module_param(spi_max, uint, 0400);
>>>>>>     MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size");
>>>>>> @@ -181,6 +183,56 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
>>>>>>     EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
>>>>> So I noticed that we already have the xrgb8888 to rgb565 function, so I'm
>>>>> a bit late on this, but: DRM doesn't do format conversions, with the
>>>>> single exception that the legacy cursor interface is specced to be
>>>>> argb8888.
>>>>>
>>>>> Imo this should be removed (and preferrably before we ship tinydrm in a
>>>>> stable kernel). Why did you add it?
>>>> I added it from the start because plymouth can only do xrgb8888 and I
>>>> thought that this was probably the format that most apps/libs/tools
>>>> supported, ensuring that tinydrm would work with everything. But I was
>>>> aware that this was the kernel patching up userspace, so I knew that it
>>>> might be shot down.
>>>>
>>>> But after your comment, I thought that this was in the clear:
>>>> https://lists.freedesktop.org/archives/dri-devel/2017-January/130551.html
>>>>
>>>>>> +EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
>>>>> I wonder whether the above would make sense in drm core as some kind of fb
>>>>> helpers. But we can do that once there's a clear need.
>>>> I can make a patch that removes this format conversion.
>>> I have no idea what I thought back then :-) But most likely I slightly
>>> misread it as argb8888_to_rgb565 (it works the same really) used for
>>> cursor compat, which is ok-ish.
>>>
>>> But then I just looked through all drivers, and I found exactly one driver
>>> which doesn't support XRGB8888, and that was probably an oversight. So
>>> there's some arguments for always supporting that. Otoh if you do buffer
>>> sharing and have a nice hw spi engine, touching a wc buffer with the cpu
>>> is _real_ slow (because of the uncached reads), so we really shouldn't let
>>> userspace stumble over this pitfall by accident. The trouble is that by
>>> exposing both, most userspace will pick XRGB8888, even when they support
>>> RGB565.
>>>
>>> And uncached reads are as a rule of thumb 1000x slower, so you don't need
>>> a big panel to feel the pain.
>>>
>>> Given that I think we should remove the fake XRGB8888 support.
>> The Raspberry Pi, which is by far the largest user of these displays,
>> has a DMA capable SPI controller that can only do 8-bit words.
>> Since it is little endian, 16-bit RGB565 has to be byte swapped using
>> a ordinary kmalloc buffer.
> I always get endianess wrong, but why do we need to swap if the controller
> shuffles 8bit blocks around? If the panel is big endian, then we just need
> to use big endian drm_fourcc (they exist).

But does userspace use it?
A quick google search for DRM_FORMAT_BIG_ENDIAN yielded only weston
which drops the field when looking at the format in libweston/gl-renderer.c.

In fact 3 months ago you said it was broken:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-December/473034.html

 > Trying to fix up everything is probably going to be lots of work, but
 > assuming that everything is broken for big endian is probably correct.
 > -Daniel


>> Theoretical maximum at 48MHz is:
>> 1 / (320 * 240 * 16 * (1/48000000.0)) = 39.0625Hz
>>
>> The actual numbers isn't very far off:
>>
>> $ modetest -M "mi0283qt" -s 25:320x240@RG16 -v
>> setting mode 320x240-0Hz@RG16 on connectors 25, crtc 27
>> freq: 39.36Hz
>> freq: 31.16Hz
>> freq: 31.21Hz
>>
>> $ modetest -M "mi0283qt" -s 25:320x240@XR24 -v
>> setting mode 320x240-0Hz@XR24 on connectors 25, crtc 27
>> freq: 37.30Hz
>> freq: 29.76Hz
>> freq: 29.84Hz
>>
>>
>> Disabling byte swapping, passing cma_obj->vaddr through to SPI, doesn't
>> improve the numbers much:
>>
>> $ modetest -M "mi0283qt" -s 25:320x240@RG16 -v
>> setting mode 320x240-0Hz@RG16 on connectors 25, crtc 27
>> freq: 43.90Hz
>> freq: 33.49Hz
>> freq: 33.49Hz
>>
>> The SPI bus is sooo slow that the cpu can jump through all kinds of
>> hoops without affecting throughput much.
> Hey, 4 more frames! But in either case, you'll probably get much faster if
> you offload the upload work to an async worker. Converting ->dirty to use
> atomic might help a lot here, since we could try to stall only to the
> previous frame, and not be entirely synchronous.

The next step wrt tinydrm and performance is to get it working with
PRIME especially for games. I have only done a test with a hacked
modetest, but failed to get X working with vc4 as the renderer.
https://github.com/anholt/linux/issues/10


>>> Wrt plymouth, I'm a bit surprised that one falls over: We have a pile of
>>> chips that (in some circumstances) prefer RGB565 (mostly big resolutions
>>> with little vram), that's controlled by the preferred_bpp parameter of
>>> drm_fb_helper_single_fb_probe(). You seem to have wired that up all
>>> correctly, plymouth still fails?
>> I tried to get plymouth to work on the Raspberry Pi, but gave up.
>> I couldn't get it to use the display.
>> But here's an extract from the plymouth code:
>>
>> create_output_buffer():
>>
>>          if (drmModeAddFB (backend->device_fd, width, height,
>>                            24, 32, buffer->row_stride, buffer->handle,
>>                            &buffer->id) != 0) {
>>
>> bpp=32 and depth=24 -> DRM_FORMAT_XRGB8888
>>
>> And the has_32bpp_support() function makes it clear that 32bpp is required.
>>
>> https://cgit.freedesktop.org/plymouth/tree/src/plugins/renderers/drm/plugin.c
> Blergh. Oh well, I guess we should just accept that userspace developers
> are lazy :(

So the conversion can stay? :-)

Noralf.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Wed, Mar 15, 2017 at 04:14:49PM +0100, Noralf Trønnes wrote:
> 
> Den 15.03.2017 13.39, skrev Daniel Vetter:
> > On Wed, Mar 15, 2017 at 01:15:37PM +0100, Noralf Trønnes wrote:
> > > Den 14.03.2017 08.17, skrev Daniel Vetter:
> > > > On Mon, Mar 13, 2017 at 01:30:40PM +0100, Noralf Trønnes wrote:
> > > > > Den 12.03.2017 19.00, skrev Daniel Vetter:
> > > > > > On Sat, Mar 11, 2017 at 10:35:32PM +0100, Noralf Trønnes wrote:
> > > > > > > Add tinydrm_rgb565_buf_copy() function that copies buffer rectangle to
> > > > > > > destination buffer and also handles XRGB8888 and byte swap conversions.
> > > > > > > Useful for displays that only support RGB565 and can do partial updates.
> > > > > > > 
> > > > > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > > > > > ---
> > > > > > >     drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 56 +++++++++++++++++++++++++-
> > > > > > >     include/drm/tinydrm/tinydrm-helpers.h          |  2 +
> > > > > > >     2 files changed, 56 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > > > > > > index d4cda33..e639453 100644
> > > > > > > --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > > > > > > +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> > > > > > > @@ -7,13 +7,15 @@
> > > > > > >      * (at your option) any later version.
> > > > > > >      */
> > > > > > > -#include <drm/tinydrm/tinydrm.h>
> > > > > > > -#include <drm/tinydrm/tinydrm-helpers.h>
> > > > > > >     #include <linux/backlight.h>
> > > > > > > +#include <linux/dma-buf.h>
> > > > > > >     #include <linux/pm.h>
> > > > > > >     #include <linux/spi/spi.h>
> > > > > > >     #include <linux/swab.h>
> > > > > > > +#include <drm/tinydrm/tinydrm.h>
> > > > > > > +#include <drm/tinydrm/tinydrm-helpers.h>
> > > > > > > +
> > > > > > >     static unsigned int spi_max;
> > > > > > >     module_param(spi_max, uint, 0400);
> > > > > > >     MODULE_PARM_DESC(spi_max, "Set a lower SPI max transfer size");
> > > > > > > @@ -181,6 +183,56 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
> > > > > > >     EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
> > > > > > So I noticed that we already have the xrgb8888 to rgb565 function, so I'm
> > > > > > a bit late on this, but: DRM doesn't do format conversions, with the
> > > > > > single exception that the legacy cursor interface is specced to be
> > > > > > argb8888.
> > > > > > 
> > > > > > Imo this should be removed (and preferrably before we ship tinydrm in a
> > > > > > stable kernel). Why did you add it?
> > > > > I added it from the start because plymouth can only do xrgb8888 and I
> > > > > thought that this was probably the format that most apps/libs/tools
> > > > > supported, ensuring that tinydrm would work with everything. But I was
> > > > > aware that this was the kernel patching up userspace, so I knew that it
> > > > > might be shot down.
> > > > > 
> > > > > But after your comment, I thought that this was in the clear:
> > > > > https://lists.freedesktop.org/archives/dri-devel/2017-January/130551.html
> > > > > 
> > > > > > > +EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
> > > > > > I wonder whether the above would make sense in drm core as some kind of fb
> > > > > > helpers. But we can do that once there's a clear need.
> > > > > I can make a patch that removes this format conversion.
> > > > I have no idea what I thought back then :-) But most likely I slightly
> > > > misread it as argb8888_to_rgb565 (it works the same really) used for
> > > > cursor compat, which is ok-ish.
> > > > 
> > > > But then I just looked through all drivers, and I found exactly one driver
> > > > which doesn't support XRGB8888, and that was probably an oversight. So
> > > > there's some arguments for always supporting that. Otoh if you do buffer
> > > > sharing and have a nice hw spi engine, touching a wc buffer with the cpu
> > > > is _real_ slow (because of the uncached reads), so we really shouldn't let
> > > > userspace stumble over this pitfall by accident. The trouble is that by
> > > > exposing both, most userspace will pick XRGB8888, even when they support
> > > > RGB565.
> > > > 
> > > > And uncached reads are as a rule of thumb 1000x slower, so you don't need
> > > > a big panel to feel the pain.
> > > > 
> > > > Given that I think we should remove the fake XRGB8888 support.
> > > The Raspberry Pi, which is by far the largest user of these displays,
> > > has a DMA capable SPI controller that can only do 8-bit words.
> > > Since it is little endian, 16-bit RGB565 has to be byte swapped using
> > > a ordinary kmalloc buffer.
> > I always get endianess wrong, but why do we need to swap if the controller
> > shuffles 8bit blocks around? If the panel is big endian, then we just need
> > to use big endian drm_fourcc (they exist).
> 
> But does userspace use it?
> A quick google search for DRM_FORMAT_BIG_ENDIAN yielded only weston
> which drops the field when looking at the format in libweston/gl-renderer.c.
> 
> In fact 3 months ago you said it was broken:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-December/473034.html

Just looking for victims to fix up the entire mess :-)

> > Trying to fix up everything is probably going to be lots of work, but
> > assuming that everything is broken for big endian is probably correct.
> > -Daniel
> 
> 
> > > Theoretical maximum at 48MHz is:
> > > 1 / (320 * 240 * 16 * (1/48000000.0)) = 39.0625Hz
> > > 
> > > The actual numbers isn't very far off:
> > > 
> > > $ modetest -M "mi0283qt" -s 25:320x240@RG16 -v
> > > setting mode 320x240-0Hz@RG16 on connectors 25, crtc 27
> > > freq: 39.36Hz
> > > freq: 31.16Hz
> > > freq: 31.21Hz
> > > 
> > > $ modetest -M "mi0283qt" -s 25:320x240@XR24 -v
> > > setting mode 320x240-0Hz@XR24 on connectors 25, crtc 27
> > > freq: 37.30Hz
> > > freq: 29.76Hz
> > > freq: 29.84Hz
> > > 
> > > 
> > > Disabling byte swapping, passing cma_obj->vaddr through to SPI, doesn't
> > > improve the numbers much:
> > > 
> > > $ modetest -M "mi0283qt" -s 25:320x240@RG16 -v
> > > setting mode 320x240-0Hz@RG16 on connectors 25, crtc 27
> > > freq: 43.90Hz
> > > freq: 33.49Hz
> > > freq: 33.49Hz
> > > 
> > > The SPI bus is sooo slow that the cpu can jump through all kinds of
> > > hoops without affecting throughput much.
> > Hey, 4 more frames! But in either case, you'll probably get much faster if
> > you offload the upload work to an async worker. Converting ->dirty to use
> > atomic might help a lot here, since we could try to stall only to the
> > previous frame, and not be entirely synchronous.
> 
> The next step wrt tinydrm and performance is to get it working with
> PRIME especially for games. I have only done a test with a hacked
> modetest, but failed to get X working with vc4 as the renderer.
> https://github.com/anholt/linux/issues/10
> 
> 
> > > > Wrt plymouth, I'm a bit surprised that one falls over: We have a pile of
> > > > chips that (in some circumstances) prefer RGB565 (mostly big resolutions
> > > > with little vram), that's controlled by the preferred_bpp parameter of
> > > > drm_fb_helper_single_fb_probe(). You seem to have wired that up all
> > > > correctly, plymouth still fails?
> > > I tried to get plymouth to work on the Raspberry Pi, but gave up.
> > > I couldn't get it to use the display.
> > > But here's an extract from the plymouth code:
> > > 
> > > create_output_buffer():
> > > 
> > >          if (drmModeAddFB (backend->device_fd, width, height,
> > >                            24, 32, buffer->row_stride, buffer->handle,
> > >                            &buffer->id) != 0) {
> > > 
> > > bpp=32 and depth=24 -> DRM_FORMAT_XRGB8888
> > > 
> > > And the has_32bpp_support() function makes it clear that 32bpp is required.
> > > 
> > > https://cgit.freedesktop.org/plymouth/tree/src/plugins/renderers/drm/plugin.c
> > Blergh. Oh well, I guess we should just accept that userspace developers
> > are lazy :(
> 
> So the conversion can stay? :-)

Yes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Michel Dänzer]

On 16/03/17 12:14 AM, Noralf Trønnes wrote:
> 
> The next step wrt tinydrm and performance is to get it working with
> PRIME especially for games. I have only done a test with a hacked
> modetest, but failed to get X working with vc4 as the renderer.
> https://github.com/anholt/linux/issues/10

FWIW, the 11-patch series at
https://patchwork.freedesktop.org/project/Xorg/patches/?submitter=16295&state=&q=&archive=&delegate=
could allow this to work without involving X's PRIME support.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] drm/tinydrm: Add tinydrm_panel abstraction

[Noralf Trønnes]

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 &regmap 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 */
-- 
2.10.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] drm/tinydrm/mipi-dbi: Start conversion to tinydrm_panel

[Noralf Trønnes]

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);
-- 
2.10.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/5] drm/tinydrm/mi0283qt: Use tinydrm_panel

[Noralf Trønnes]

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,
-- 
2.10.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/5] drm/tinydrm/mipi-dbi: Clean up after tinydrm_panel conversion

[Noralf Trønnes]

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);
 
-- 
2.10.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] drm/tinydrm: Add tinydrm_panel abstraction

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] drm/tinydrm: Add tinydrm_panel abstraction

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

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

> 
> 2. Add tons more panel drivers. Personally I'd do that first :-)
> 
> Cheers, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] drm/tinydrm: Add tinydrm_panel abstraction

[Noralf Trønnes]

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:
>>
>> 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.
> 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
>
>> 2. Add tons more panel drivers. Personally I'd do that first :-)
>>
>> Cheers, Daniel
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] drm/tinydrm: Add tinydrm_panel abstraction

[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).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] drm/tinydrm: Add tinydrm_panel abstraction

[Noralf Trønnes]

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.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] drm/tinydrm: Add tinydrm_panel abstraction

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

> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] drm/tinydrm: Add tinydrm_panel abstraction

[Noralf Trønnes]

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.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/5] drm/tinydrm: Add tinydrm_panel abstraction

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel