[PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.

Using the DRM fb emulation, all the tests from Geert Uytterhoeven's fbtest
(https://git.kernel.org/pub/scm/linux/kernel/git/geert/fbtest.git) passes:

     ./fbtest -f /dev/fb1
    Using drawops cfb32 (32 bpp packed pixels)
    Available visuals:
      Monochrome
      Grayscale 256
      Truecolor 8:8:8:0
    Using visops truecolor
    Running all tests
    test001: PASSED
    test002: PASSED
    test003: PASSED
    test004: PASSED
    test005: PASSED
    test006: PASSED
    test008: PASSED
    test009: PASSED
    test010: PASSED
    Benchmarking... 10x10 squares: 412.99 Mpixels/s
    Benchmarking... 20x20 squares: 857.46 Mpixels/s
    Benchmarking... 50x50 squares: 1593.51 Mpixels/s
    test012: PASSED
    Benchmarking... R5 circles: 237.07 Mpixels/s
    Benchmarking... R10 circles: 501.24 Mpixels/s
    Benchmarking... R25 circles: 947.86 Mpixels/s
    test013: PASSED

Patch #1 adds an I2C connector type since currently there isn't one and
I2C drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.

Patch #2 adds a drm_fb_gray8_to_mono_reversed() DRM format helper since
most DRM/KMS user-space don't support bpp 1 displays, so drivers expose
a common format that's converted to greyscale and then to monochrome.

Patch #3 adds the driver. The name ssd1307 was used instead of ssd130x
(which would be more accurate) to avoid confusion for users who want to
migrate from the existing ssd1307fb fbdev driver.

Patch #4 just adds a MAINTAINERS entry for this new DRM driver.

Best regards,
Javier


Javier Martinez Canillas (4):
  drm: Add I2C connector type
  drm/format-helper: Add drm_fb_gray8_to_mono_reversed()
  drm/tiny: Add driver for Solomon SSD1307 OLED displays
  MAINTAINERS: Add entry for Solomon SSD1307 OLED displays DRM driver

 MAINTAINERS                         |   7 +
 drivers/gpu/drm/drm_connector.c     |   1 +
 drivers/gpu/drm/drm_format_helper.c |  35 +
 drivers/gpu/drm/tiny/Kconfig        |  12 +
 drivers/gpu/drm/tiny/Makefile       |   1 +
 drivers/gpu/drm/tiny/ssd1307.c      | 976 ++++++++++++++++++++++++++++
 include/drm/drm_format_helper.h     |   2 +
 include/uapi/drm/drm_mode.h         |   1 +
 8 files changed, 1035 insertions(+)
 create mode 100644 drivers/gpu/drm/tiny/ssd1307.c

-- 
2.34.1

[PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.

Using the DRM fb emulation, all the tests from Geert Uytterhoeven's fbtest
(https://git.kernel.org/pub/scm/linux/kernel/git/geert/fbtest.git) passes:

     ./fbtest -f /dev/fb1
    Using drawops cfb32 (32 bpp packed pixels)
    Available visuals:
      Monochrome
      Grayscale 256
      Truecolor 8:8:8:0
    Using visops truecolor
    Running all tests
    test001: PASSED
    test002: PASSED
    test003: PASSED
    test004: PASSED
    test005: PASSED
    test006: PASSED
    test008: PASSED
    test009: PASSED
    test010: PASSED
    Benchmarking... 10x10 squares: 412.99 Mpixels/s
    Benchmarking... 20x20 squares: 857.46 Mpixels/s
    Benchmarking... 50x50 squares: 1593.51 Mpixels/s
    test012: PASSED
    Benchmarking... R5 circles: 237.07 Mpixels/s
    Benchmarking... R10 circles: 501.24 Mpixels/s
    Benchmarking... R25 circles: 947.86 Mpixels/s
    test013: PASSED

Patch #1 adds an I2C connector type since currently there isn't one and
I2C drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.

Patch #2 adds a drm_fb_gray8_to_mono_reversed() DRM format helper since
most DRM/KMS user-space don't support bpp 1 displays, so drivers expose
a common format that's converted to greyscale and then to monochrome.

Patch #3 adds the driver. The name ssd1307 was used instead of ssd130x
(which would be more accurate) to avoid confusion for users who want to
migrate from the existing ssd1307fb fbdev driver.

Patch #4 just adds a MAINTAINERS entry for this new DRM driver.

Best regards,
Javier


Javier Martinez Canillas (4):
  drm: Add I2C connector type
  drm/format-helper: Add drm_fb_gray8_to_mono_reversed()
  drm/tiny: Add driver for Solomon SSD1307 OLED displays
  MAINTAINERS: Add entry for Solomon SSD1307 OLED displays DRM driver

 MAINTAINERS                         |   7 +
 drivers/gpu/drm/drm_connector.c     |   1 +
 drivers/gpu/drm/drm_format_helper.c |  35 +
 drivers/gpu/drm/tiny/Kconfig        |  12 +
 drivers/gpu/drm/tiny/Makefile       |   1 +
 drivers/gpu/drm/tiny/ssd1307.c      | 976 ++++++++++++++++++++++++++++
 include/drm/drm_format_helper.h     |   2 +
 include/uapi/drm/drm_mode.h         |   1 +
 8 files changed, 1035 insertions(+)
 create mode 100644 drivers/gpu/drm/tiny/ssd1307.c

-- 
2.34.1

[PATCH 1/4] drm: Add I2C connector type

[Javier Martinez Canillas]

There isn't a connector type for display controllers accesed through I2C,
most drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.

Add an I2C connector type to match the actual connector.

As Noralf Trønnes mentions in commit fc06bf1d76d6 ("drm: Add SPI connector
type"), user-space should be able to cope with a connector type that does
not yet understand.

Tested with `modetest -M ssd1307 -c` and shows the connector as unknown-1.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/drm_connector.c | 1 +
 include/uapi/drm/drm_mode.h     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index a50c82bc2b2f..975a7525a508 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -105,6 +105,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = {
 	{ DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
 	{ DRM_MODE_CONNECTOR_SPI, "SPI" },
 	{ DRM_MODE_CONNECTOR_USB, "USB" },
+	{ DRM_MODE_CONNECTOR_I2C, "I2C" },
 };
 
 void drm_connector_ida_init(void)
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index e1e351682872..d6d6288242db 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -421,6 +421,7 @@ enum drm_mode_subconnector {
 #define DRM_MODE_CONNECTOR_WRITEBACK	18
 #define DRM_MODE_CONNECTOR_SPI		19
 #define DRM_MODE_CONNECTOR_USB		20
+#define DRM_MODE_CONNECTOR_I2C		21
 
 /**
  * struct drm_mode_get_connector - Get connector metadata.
-- 
2.34.1

[PATCH 1/4] drm: Add I2C connector type

[Javier Martinez Canillas]

There isn't a connector type for display controllers accesed through I2C,
most drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.

Add an I2C connector type to match the actual connector.

As Noralf Trønnes mentions in commit fc06bf1d76d6 ("drm: Add SPI connector
type"), user-space should be able to cope with a connector type that does
not yet understand.

Tested with `modetest -M ssd1307 -c` and shows the connector as unknown-1.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/drm_connector.c | 1 +
 include/uapi/drm/drm_mode.h     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index a50c82bc2b2f..975a7525a508 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -105,6 +105,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = {
 	{ DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
 	{ DRM_MODE_CONNECTOR_SPI, "SPI" },
 	{ DRM_MODE_CONNECTOR_USB, "USB" },
+	{ DRM_MODE_CONNECTOR_I2C, "I2C" },
 };
 
 void drm_connector_ida_init(void)
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index e1e351682872..d6d6288242db 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -421,6 +421,7 @@ enum drm_mode_subconnector {
 #define DRM_MODE_CONNECTOR_WRITEBACK	18
 #define DRM_MODE_CONNECTOR_SPI		19
 #define DRM_MODE_CONNECTOR_USB		20
+#define DRM_MODE_CONNECTOR_I2C		21
 
 /**
  * struct drm_mode_get_connector - Get connector metadata.
-- 
2.34.1

[PATCH 2/4] drm/format-helper: Add drm_fb_gray8_to_mono_reversed()

[Javier Martinez Canillas]

Add support to convert 8-bit grayscale to reversed monochrome for drivers
that control monochromatic displays, that only have 1 bit per pixel depth.

This helper function was based on repaper_gray8_to_mono_reversed() from
the drivers/gpu/drm/tiny/repaper.c driver.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/drm_format_helper.c | 35 +++++++++++++++++++++++++++++
 include/drm/drm_format_helper.h     |  2 ++
 2 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index 0f28dd2bdd72..bf477c136082 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
 	return -EINVAL;
 }
 EXPORT_SYMBOL(drm_fb_blit_toio);
+
+/**
+ * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
+ * @dst: reversed monochrome destination buffer
+ * @src: 8-bit grayscale source buffer
+ * @clip: Clip rectangle area to copy
+ *
+ * DRM doesn't have native monochrome or grayscale support.
+ * Such drivers can announce the commonly supported XR24 format to userspace
+ * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
+ * helper function to convert to the native format.
+ */
+void drm_fb_gray8_to_mono_reversed(void *dst, void *src, const struct drm_rect *clip)
+{
+	size_t width = drm_rect_width(clip);
+	size_t height = drm_rect_width(clip);
+
+	u8 *mono = dst, *gray8 = src;
+	unsigned int y, xb, i;
+
+	for (y = 0; y < height; y++)
+		for (xb = 0; xb < width / 8; xb++) {
+			u8 byte = 0x00;
+
+			for (i = 0; i < 8; i++) {
+				int x = xb * 8 + i;
+
+				byte >>= 1;
+				if (gray8[y * width + x] >> 7)
+					byte |= BIT(7);
+			}
+			*mono++ = byte;
+		}
+}
+EXPORT_SYMBOL(drm_fb_gray8_to_mono_reversed);
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index b30ed5de0a33..cd4c8b7c78de 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -43,4 +43,6 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
 		     const void *vmap, const struct drm_framebuffer *fb,
 		     const struct drm_rect *rect);
 
+void drm_fb_gray8_to_mono_reversed(void *dst, void *vaddr, const struct drm_rect *clip);
+
 #endif /* __LINUX_DRM_FORMAT_HELPER_H */
-- 
2.34.1

[PATCH 2/4] drm/format-helper: Add drm_fb_gray8_to_mono_reversed()

[Javier Martinez Canillas]

Add support to convert 8-bit grayscale to reversed monochrome for drivers
that control monochromatic displays, that only have 1 bit per pixel depth.

This helper function was based on repaper_gray8_to_mono_reversed() from
the drivers/gpu/drm/tiny/repaper.c driver.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/drm_format_helper.c | 35 +++++++++++++++++++++++++++++
 include/drm/drm_format_helper.h     |  2 ++
 2 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index 0f28dd2bdd72..bf477c136082 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
 	return -EINVAL;
 }
 EXPORT_SYMBOL(drm_fb_blit_toio);
+
+/**
+ * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
+ * @dst: reversed monochrome destination buffer
+ * @src: 8-bit grayscale source buffer
+ * @clip: Clip rectangle area to copy
+ *
+ * DRM doesn't have native monochrome or grayscale support.
+ * Such drivers can announce the commonly supported XR24 format to userspace
+ * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
+ * helper function to convert to the native format.
+ */
+void drm_fb_gray8_to_mono_reversed(void *dst, void *src, const struct drm_rect *clip)
+{
+	size_t width = drm_rect_width(clip);
+	size_t height = drm_rect_width(clip);
+
+	u8 *mono = dst, *gray8 = src;
+	unsigned int y, xb, i;
+
+	for (y = 0; y < height; y++)
+		for (xb = 0; xb < width / 8; xb++) {
+			u8 byte = 0x00;
+
+			for (i = 0; i < 8; i++) {
+				int x = xb * 8 + i;
+
+				byte >>= 1;
+				if (gray8[y * width + x] >> 7)
+					byte |= BIT(7);
+			}
+			*mono++ = byte;
+		}
+}
+EXPORT_SYMBOL(drm_fb_gray8_to_mono_reversed);
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index b30ed5de0a33..cd4c8b7c78de 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -43,4 +43,6 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
 		     const void *vmap, const struct drm_framebuffer *fb,
 		     const struct drm_rect *rect);
 
+void drm_fb_gray8_to_mono_reversed(void *dst, void *vaddr, const struct drm_rect *clip);
+
 #endif /* __LINUX_DRM_FORMAT_HELPER_H */
-- 
2.34.1

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Simon Ser]

This driver only advertises XRGB8888 in ssd1307_formats. It would be nice to
expose R8 as well so that user-space can directly produce suitable buffers.
It would also be nice to have some kind of preferred format, so that user-space
knows R8 is preferred over XRGB8888.

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Simon Ser]

This driver only advertises XRGB8888 in ssd1307_formats. It would be nice to
expose R8 as well so that user-space can directly produce suitable buffers.
It would also be nice to have some kind of preferred format, so that user-space
knows R8 is preferred over XRGB8888.

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Simon Ser]

On Monday, January 31st, 2022 at 21:36, Simon Ser <contact@emersion.fr> wrote:

> This driver only advertises XRGB8888 in ssd1307_formats. It would be nice to
> expose R8 as well so that user-space can directly produce suitable buffers.
> It would also be nice to have some kind of preferred format, so that user-space
> knows R8 is preferred over XRGB8888.

Hm, since the format used by the hw is actually R1, adding that to drm_fourcc.h
would be even better.

Let me know if you want me to type up any of the user-space bits.

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Simon Ser]

On Monday, January 31st, 2022 at 21:36, Simon Ser <contact@emersion.fr> wrote:

> This driver only advertises XRGB8888 in ssd1307_formats. It would be nice to
> expose R8 as well so that user-space can directly produce suitable buffers.
> It would also be nice to have some kind of preferred format, so that user-space
> knows R8 is preferred over XRGB8888.

Hm, since the format used by the hw is actually R1, adding that to drm_fourcc.h
would be even better.

Let me know if you want me to type up any of the user-space bits.

Re: [PATCH 1/4] drm: Add I2C connector type

[Sam Ravnborg]

On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:
> There isn't a connector type for display controllers accesed through I2C,
> most drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.
> 
> Add an I2C connector type to match the actual connector.
> 
> As Noralf Trønnes mentions in commit fc06bf1d76d6 ("drm: Add SPI connector
> type"), user-space should be able to cope with a connector type that does
> not yet understand.
> 
> Tested with `modetest -M ssd1307 -c` and shows the connector as unknown-1.
I had expected unknown-21??

> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> ---
> 
>  drivers/gpu/drm/drm_connector.c | 1 +
>  include/uapi/drm/drm_mode.h     | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index a50c82bc2b2f..975a7525a508 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -105,6 +105,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = {
>  	{ DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
>  	{ DRM_MODE_CONNECTOR_SPI, "SPI" },
>  	{ DRM_MODE_CONNECTOR_USB, "USB" },
> +	{ DRM_MODE_CONNECTOR_I2C, "I2C" },
>  };
>  
>  void drm_connector_ida_init(void)
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index e1e351682872..d6d6288242db 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -421,6 +421,7 @@ enum drm_mode_subconnector {
>  #define DRM_MODE_CONNECTOR_WRITEBACK	18
>  #define DRM_MODE_CONNECTOR_SPI		19
>  #define DRM_MODE_CONNECTOR_USB		20
> +#define DRM_MODE_CONNECTOR_I2C		21
>  
>  /**
>   * struct drm_mode_get_connector - Get connector metadata.
> -- 
> 2.34.1

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Sam Ravnborg]

Hi Javier,
On Mon, Jan 31, 2022 at 09:12:20PM +0100, Javier Martinez Canillas wrote:
> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.
> 
> Using the DRM fb emulation, all the tests from Geert Uytterhoeven's fbtest
> (https://git.kernel.org/pub/scm/linux/kernel/git/geert/fbtest.git) passes:

Impressed how fast you did this!
Saw the picture you posted a link to on irc - nice.

> Patch #3 adds the driver. The name ssd1307 was used instead of ssd130x
> (which would be more accurate) to avoid confusion for users who want to
> migrate from the existing ssd1307fb fbdev driver.
Looking forward the name ssd130x would make more sense. There is only so
many existing users and a potential of much more new users.
So in my color of the world the naming that benefits the most users
wins.

	Sam

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Simon,

Thanks for your feedback.

On 1/31/22 21:39, Simon Ser wrote:
> On Monday, January 31st, 2022 at 21:36, Simon Ser <contact@emersion.fr> wrote:
> 
>> This driver only advertises XRGB8888 in ssd1307_formats. It would be nice to
>> expose R8 as well so that user-space can directly produce suitable buffers.
>> It would also be nice to have some kind of preferred format, so that user-space
>> knows R8 is preferred over XRGB8888.
> 
> Hm, since the format used by the hw is actually R1, adding that to drm_fourcc.h
> would be even better.
> 

Yes, agreed that would be nice. We discussed this already with Thomas and my
suggestion was to land the driver as is, advertising XRGB8888. Which is also
what the other driver using monochrome does (drivers/gpu/drm/tiny/repaper.c):

https://www.spinics.net/lists/dri-devel/msg331328.html

As a follow-up we can wire up al the needed bits to have a DRM/KMS driver that
could expose a R1 format.

> Let me know if you want me to type up any of the user-space bits.
> 

Thanks! I also could help to add the needed support in the user-space stack.

Best reagards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Simon,

Thanks for your feedback.

On 1/31/22 21:39, Simon Ser wrote:
> On Monday, January 31st, 2022 at 21:36, Simon Ser <contact@emersion.fr> wrote:
> 
>> This driver only advertises XRGB8888 in ssd1307_formats. It would be nice to
>> expose R8 as well so that user-space can directly produce suitable buffers.
>> It would also be nice to have some kind of preferred format, so that user-space
>> knows R8 is preferred over XRGB8888.
> 
> Hm, since the format used by the hw is actually R1, adding that to drm_fourcc.h
> would be even better.
> 

Yes, agreed that would be nice. We discussed this already with Thomas and my
suggestion was to land the driver as is, advertising XRGB8888. Which is also
what the other driver using monochrome does (drivers/gpu/drm/tiny/repaper.c):

https://www.spinics.net/lists/dri-devel/msg331328.html

As a follow-up we can wire up al the needed bits to have a DRM/KMS driver that
could expose a R1 format.

> Let me know if you want me to type up any of the user-space bits.
> 

Thanks! I also could help to add the needed support in the user-space stack.

Best reagards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 1/4] drm: Add I2C connector type

[Javier Martinez Canillas]

On 1/31/22 21:52, Sam Ravnborg wrote:
> On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:
>> There isn't a connector type for display controllers accesed through I2C,
>> most drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.
>>
>> Add an I2C connector type to match the actual connector.
>>
>> As Noralf Trønnes mentions in commit fc06bf1d76d6 ("drm: Add SPI connector
>> type"), user-space should be able to cope with a connector type that does
>> not yet understand.
>>
>> Tested with `modetest -M ssd1307 -c` and shows the connector as unknown-1.
> I had expected unknown-21??
>

As you pointed out in patch 3/4, I forgot to change DRM_MODE_CONNECTOR_Unknown
to DRM_MODE_CONNECTOR_I2C after adding this patch...
 
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>> ---

Thanks!

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 1/4] drm: Add I2C connector type

[Javier Martinez Canillas]

On 1/31/22 21:52, Sam Ravnborg wrote:
> On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:
>> There isn't a connector type for display controllers accesed through I2C,
>> most drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.
>>
>> Add an I2C connector type to match the actual connector.
>>
>> As Noralf Trønnes mentions in commit fc06bf1d76d6 ("drm: Add SPI connector
>> type"), user-space should be able to cope with a connector type that does
>> not yet understand.
>>
>> Tested with `modetest -M ssd1307 -c` and shows the connector as unknown-1.
> I had expected unknown-21??
>

As you pointed out in patch 3/4, I forgot to change DRM_MODE_CONNECTOR_Unknown
to DRM_MODE_CONNECTOR_I2C after adding this patch...
 
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>> ---

Thanks!

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

On 1/31/22 21:56, Sam Ravnborg wrote:
> Hi Javier,
> On Mon, Jan 31, 2022 at 09:12:20PM +0100, Javier Martinez Canillas wrote:
>> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
>> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.
>>
>> Using the DRM fb emulation, all the tests from Geert Uytterhoeven's fbtest
>> (https://git.kernel.org/pub/scm/linux/kernel/git/geert/fbtest.git) passes:
> 
> Impressed how fast you did this!
> Saw the picture you posted a link to on irc - nice.
>

Thanks :)

What's impressive is how many helper functions the DRM core has, so typing a
new DRM driver is something that could be achieved in a few hours. Which was
one of my goals with this experiment, to understand how much effort would be
for a developer with no prior experience with DRM to port a fbdev driver.
 
>> Patch #3 adds the driver. The name ssd1307 was used instead of ssd130x
>> (which would be more accurate) to avoid confusion for users who want to
>> migrate from the existing ssd1307fb fbdev driver.
> Looking forward the name ssd130x would make more sense. There is only so
> many existing users and a potential of much more new users.
> So in my color of the world the naming that benefits the most users
> wins.
>

Agreed. That's also what Andy suggested and makes a lot of sense to me.
 
> 	Sam
> 

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

On 1/31/22 21:56, Sam Ravnborg wrote:
> Hi Javier,
> On Mon, Jan 31, 2022 at 09:12:20PM +0100, Javier Martinez Canillas wrote:
>> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
>> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.
>>
>> Using the DRM fb emulation, all the tests from Geert Uytterhoeven's fbtest
>> (https://git.kernel.org/pub/scm/linux/kernel/git/geert/fbtest.git) passes:
> 
> Impressed how fast you did this!
> Saw the picture you posted a link to on irc - nice.
>

Thanks :)

What's impressive is how many helper functions the DRM core has, so typing a
new DRM driver is something that could be achieved in a few hours. Which was
one of my goals with this experiment, to understand how much effort would be
for a developer with no prior experience with DRM to port a fbdev driver.
 
>> Patch #3 adds the driver. The name ssd1307 was used instead of ssd130x
>> (which would be more accurate) to avoid confusion for users who want to
>> migrate from the existing ssd1307fb fbdev driver.
> Looking forward the name ssd130x would make more sense. There is only so
> many existing users and a potential of much more new users.
> So in my color of the world the naming that benefits the most users
> wins.
>

Agreed. That's also what Andy suggested and makes a lot of sense to me.
 
> 	Sam
> 

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

On Mon, Jan 31, 2022 at 9:39 PM Simon Ser <contact@emersion.fr> wrote:
> On Monday, January 31st, 2022 at 21:36, Simon Ser <contact@emersion.fr> wrote:
>
> > This driver only advertises XRGB8888 in ssd1307_formats. It would be nice to
> > expose R8 as well so that user-space can directly produce suitable buffers.
> > It would also be nice to have some kind of preferred format, so that user-space
> > knows R8 is preferred over XRGB8888.
>
> Hm, since the format used by the hw is actually R1, adding that to drm_fourcc.h
> would be even better.

What's the story with the Rn formats?
The comments say "n bpp Red", while this is a monochrome (even
inverted) display?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

On Mon, Jan 31, 2022 at 9:39 PM Simon Ser <contact@emersion.fr> wrote:
> On Monday, January 31st, 2022 at 21:36, Simon Ser <contact@emersion.fr> wrote:
>
> > This driver only advertises XRGB8888 in ssd1307_formats. It would be nice to
> > expose R8 as well so that user-space can directly produce suitable buffers.
> > It would also be nice to have some kind of preferred format, so that user-space
> > knows R8 is preferred over XRGB8888.
>
> Hm, since the format used by the hw is actually R1, adding that to drm_fourcc.h
> would be even better.

What's the story with the Rn formats?
The comments say "n bpp Red", while this is a monochrome (even
inverted) display?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Simon Ser]

On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> What's the story with the Rn formats?
>
> The comments say "n bpp Red", while this is a monochrome (even
> inverted) display?

I don't think the color matters that much. "Red" was picked just because it was
an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Simon Ser]

On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> What's the story with the Rn formats?
>
> The comments say "n bpp Red", while this is a monochrome (even
> inverted) display?

I don't think the color matters that much. "Red" was picked just because it was
an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Simon,

On Tue, Feb 1, 2022 at 9:34 AM Simon Ser <contact@emersion.fr> wrote:
> On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > What's the story with the Rn formats?
> >
> > The comments say "n bpp Red", while this is a monochrome (even
> > inverted) display?
>
> I don't think the color matters that much. "Red" was picked just because it was
> an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?

I'd expect 8-bit grayscale to be Y8 instead.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Simon,

On Tue, Feb 1, 2022 at 9:34 AM Simon Ser <contact@emersion.fr> wrote:
> On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > What's the story with the Rn formats?
> >
> > The comments say "n bpp Red", while this is a monochrome (even
> > inverted) display?
>
> I don't think the color matters that much. "Red" was picked just because it was
> an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?

I'd expect 8-bit grayscale to be Y8 instead.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Daniel Vetter]

On Tue, Feb 1, 2022 at 9:34 AM Simon Ser <contact@emersion.fr> wrote:
>
> On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> > What's the story with the Rn formats?
> >
> > The comments say "n bpp Red", while this is a monochrome (even
> > inverted) display?
>
> I don't think the color matters that much. "Red" was picked just because it was
> an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?

The red comes from gl, where with shaders it really doesn't matter
what meaning you attach to channels, but really just how many you
have. So 2-channel formats are called RxGx, 3-channel RxGxBx,
4-channel RxGxBxAx and single-channel Rx. And we use drm_fourcc for
interop in general, hence why these exist.

We should probably make a comment that this really isn't a red channel
when used for display it's a greyscale/intensity format. Aside from
that documentation gap I think reusing Rx formats for
greyscale/intensity for display makes perfect sense.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Daniel Vetter]

On Tue, Feb 1, 2022 at 9:34 AM Simon Ser <contact@emersion.fr> wrote:
>
> On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> > What's the story with the Rn formats?
> >
> > The comments say "n bpp Red", while this is a monochrome (even
> > inverted) display?
>
> I don't think the color matters that much. "Red" was picked just because it was
> an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?

The red comes from gl, where with shaders it really doesn't matter
what meaning you attach to channels, but really just how many you
have. So 2-channel formats are called RxGx, 3-channel RxGxBx,
4-channel RxGxBxAx and single-channel Rx. And we use drm_fourcc for
interop in general, hence why these exist.

We should probably make a comment that this really isn't a red channel
when used for display it's a greyscale/intensity format. Aside from
that documentation gap I think reusing Rx formats for
greyscale/intensity for display makes perfect sense.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Javier,

On Mon, Jan 31, 2022 at 9:12 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.

Thanks for your series!

I'll give it a try on an Adafruit FeatherWing 128x32 OLED, connected
to an OrangeCrab ECP5 FPGA board running a 64 MHz VexRiscv RISC-V
softcore.

> Using the DRM fb emulation, all the tests from Geert Uytterhoeven's fbtest
> (https://git.kernel.org/pub/scm/linux/kernel/git/geert/fbtest.git) passes:
>
>      ./fbtest -f /dev/fb1
>     Using drawops cfb32 (32 bpp packed pixels)
>     Available visuals:
>       Monochrome
>       Grayscale 256
>       Truecolor 8:8:8:0

Oh, fake 32-bpp truecolor ;-)

Does it run modetest, too?

I'm trying to get modetest working on my atari DRM driver.
Comparing to the cirrus driver doesn't help much, as modetest doesn't
seem to work with the cirrus driver (modified to not do hardware
access, as I don't have cirrus hardware):

    # modetest -M cirrus -s 31:1024x768-60Hz
    setting mode 1024x768-60.00Hz on connectors 31, crtc 34
    failed to set gamma: Function not implemented

Does there exist another simple test program for showing something
using the DRM API?

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Javier,

On Mon, Jan 31, 2022 at 9:12 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.

Thanks for your series!

I'll give it a try on an Adafruit FeatherWing 128x32 OLED, connected
to an OrangeCrab ECP5 FPGA board running a 64 MHz VexRiscv RISC-V
softcore.

> Using the DRM fb emulation, all the tests from Geert Uytterhoeven's fbtest
> (https://git.kernel.org/pub/scm/linux/kernel/git/geert/fbtest.git) passes:
>
>      ./fbtest -f /dev/fb1
>     Using drawops cfb32 (32 bpp packed pixels)
>     Available visuals:
>       Monochrome
>       Grayscale 256
>       Truecolor 8:8:8:0

Oh, fake 32-bpp truecolor ;-)

Does it run modetest, too?

I'm trying to get modetest working on my atari DRM driver.
Comparing to the cirrus driver doesn't help much, as modetest doesn't
seem to work with the cirrus driver (modified to not do hardware
access, as I don't have cirrus hardware):

    # modetest -M cirrus -s 31:1024x768-60Hz
    setting mode 1024x768-60.00Hz on connectors 31, crtc 34
    failed to set gamma: Function not implemented

Does there exist another simple test program for showing something
using the DRM API?

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Simon Ser]

On Tuesday, February 1st, 2022 at 09:43, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Does there exist another simple test program for showing something
> using the DRM API?

If you're fine with going low-level, there's tentative [1] which can apply an
arbitrary KMS state. See for instance [2] for basic mode-setting.

[1]: https://git.sr.ht/~emersion/tentative
[2]: https://git.sr.ht/~emersion/tentative/tree/master/item/examples/modeset

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Simon Ser]

On Tuesday, February 1st, 2022 at 09:43, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Does there exist another simple test program for showing something
> using the DRM API?

If you're fine with going low-level, there's tentative [1] which can apply an
arbitrary KMS state. See for instance [2] for basic mode-setting.

[1]: https://git.sr.ht/~emersion/tentative
[2]: https://git.sr.ht/~emersion/tentative/tree/master/item/examples/modeset

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Andy Shevchenko]

On Mon, Jan 31, 2022 at 09:56:23PM +0100, Sam Ravnborg wrote:
> On Mon, Jan 31, 2022 at 09:12:20PM +0100, Javier Martinez Canillas wrote:

...

> > Patch #3 adds the driver. The name ssd1307 was used instead of ssd130x
> > (which would be more accurate) to avoid confusion for users who want to
> > migrate from the existing ssd1307fb fbdev driver.
> Looking forward the name ssd130x would make more sense. There is only so
> many existing users and a potential of much more new users.
> So in my color of the world the naming that benefits the most users
> wins.

It depends if the binding is going to be preserved. Also this series doesn't
answer to the question what to do with the old driver.

If you leave it, I would expect the backward compatibility, otherwise the
series misses removal of the old driver.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Andy Shevchenko]

On Mon, Jan 31, 2022 at 09:56:23PM +0100, Sam Ravnborg wrote:
> On Mon, Jan 31, 2022 at 09:12:20PM +0100, Javier Martinez Canillas wrote:

...

> > Patch #3 adds the driver. The name ssd1307 was used instead of ssd130x
> > (which would be more accurate) to avoid confusion for users who want to
> > migrate from the existing ssd1307fb fbdev driver.
> Looking forward the name ssd130x would make more sense. There is only so
> many existing users and a potential of much more new users.
> So in my color of the world the naming that benefits the most users
> wins.

It depends if the binding is going to be preserved. Also this series doesn't
answer to the question what to do with the old driver.

If you leave it, I would expect the backward compatibility, otherwise the
series misses removal of the old driver.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

On 2/1/22 09:38, Daniel Vetter wrote:
> On Tue, Feb 1, 2022 at 9:34 AM Simon Ser <contact@emersion.fr> wrote:
>>
>> On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>
>>> What's the story with the Rn formats?
>>>
>>> The comments say "n bpp Red", while this is a monochrome (even
>>> inverted) display?
>>
>> I don't think the color matters that much. "Red" was picked just because it was
>> an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?
> 
> The red comes from gl, where with shaders it really doesn't matter
> what meaning you attach to channels, but really just how many you
> have. So 2-channel formats are called RxGx, 3-channel RxGxBx,
> 4-channel RxGxBxAx and single-channel Rx. And we use drm_fourcc for
> interop in general, hence why these exist.
> 
> We should probably make a comment that this really isn't a red channel
> when used for display it's a greyscale/intensity format. Aside from
> that documentation gap I think reusing Rx formats for
> greyscale/intensity for display makes perfect sense.
> -Daniel

To sump up the conversation in the #dri-devel channel, these drivers
should support the following formats:

1) Dx (Daniel suggested that for darkness, but inverted mono)
2) Rx (single-channel for grayscale)
3) RxGxBxAx (4-channel fake 32-bpp truecolor)

The format preference will be in that order, so if user-space is able
to use Dx then there won't be a need for any conversion and just the
native format will be used.

If using Rx then only a Rx -> Dx conversion will happen and the last
format will require the less performant RxGxBxAx -> Rx -> Dx path.

But we still need RxGxBxAx as a fallback for compatibility with the
existing user-space, so all this could be done as a follow-up as an
optimization and shouldn't block monochromatic panel drivers IMO.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

On 2/1/22 09:38, Daniel Vetter wrote:
> On Tue, Feb 1, 2022 at 9:34 AM Simon Ser <contact@emersion.fr> wrote:
>>
>> On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>
>>> What's the story with the Rn formats?
>>>
>>> The comments say "n bpp Red", while this is a monochrome (even
>>> inverted) display?
>>
>> I don't think the color matters that much. "Red" was picked just because it was
>> an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?
> 
> The red comes from gl, where with shaders it really doesn't matter
> what meaning you attach to channels, but really just how many you
> have. So 2-channel formats are called RxGx, 3-channel RxGxBx,
> 4-channel RxGxBxAx and single-channel Rx. And we use drm_fourcc for
> interop in general, hence why these exist.
> 
> We should probably make a comment that this really isn't a red channel
> when used for display it's a greyscale/intensity format. Aside from
> that documentation gap I think reusing Rx formats for
> greyscale/intensity for display makes perfect sense.
> -Daniel

To sump up the conversation in the #dri-devel channel, these drivers
should support the following formats:

1) Dx (Daniel suggested that for darkness, but inverted mono)
2) Rx (single-channel for grayscale)
3) RxGxBxAx (4-channel fake 32-bpp truecolor)

The format preference will be in that order, so if user-space is able
to use Dx then there won't be a need for any conversion and just the
native format will be used.

If using Rx then only a Rx -> Dx conversion will happen and the last
format will require the less performant RxGxBxAx -> Rx -> Dx path.

But we still need RxGxBxAx as a fallback for compatibility with the
existing user-space, so all this could be done as a follow-up as an
optimization and shouldn't block monochromatic panel drivers IMO.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 2/4] drm/format-helper: Add drm_fb_gray8_to_mono_reversed()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3413 bytes --]

Hi

Am 31.01.22 um 21:12 schrieb Javier Martinez Canillas:
> Add support to convert 8-bit grayscale to reversed monochrome for drivers
> that control monochromatic displays, that only have 1 bit per pixel depth.
> 
> This helper function was based on repaper_gray8_to_mono_reversed() from
> the drivers/gpu/drm/tiny/repaper.c driver.

You could convert repaper to the new helper.

> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>   drivers/gpu/drm/drm_format_helper.c | 35 +++++++++++++++++++++++++++++
>   include/drm/drm_format_helper.h     |  2 ++
>   2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index 0f28dd2bdd72..bf477c136082 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>   	return -EINVAL;
>   }
>   EXPORT_SYMBOL(drm_fb_blit_toio);
> +
> +/**
> + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
> + * @dst: reversed monochrome destination buffer
> + * @src: 8-bit grayscale source buffer
> + * @clip: Clip rectangle area to copy
> + *
> + * DRM doesn't have native monochrome or grayscale support.
> + * Such drivers can announce the commonly supported XR24 format to userspace
> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
> + * helper function to convert to the native format.
> + */
> +void drm_fb_gray8_to_mono_reversed(void *dst, void *src, const struct drm_rect *clip)

IMHO it would be better to have a function that converts xrgb8888 to 
mono and let it handle the intermediate step of gray8.

> +{
> +	size_t width = drm_rect_width(clip);
> +	size_t height = drm_rect_width(clip);
> +
> +	u8 *mono = dst, *gray8 = src;
> +	unsigned int y, xb, i;
> +
> +	for (y = 0; y < height; y++)
> +		for (xb = 0; xb < width / 8; xb++) {

The inner loop should probably go into a separate helper function. See 
the drm_fb_*_line() helpers in this file

At some point, we's want to have a single blit helper that takes source 
and destination formats/buffers. It would then pick the correct per-line 
helper for the conversion. So yeah, we'd want something composable.

Best regards
Thomas

> +			u8 byte = 0x00;
> +
> +			for (i = 0; i < 8; i++) {
> +				int x = xb * 8 + i;
> +
> +				byte >>= 1;
> +				if (gray8[y * width + x] >> 7)
> +					byte |= BIT(7);
> +			}
> +			*mono++ = byte;
> +		}
> +}
> +EXPORT_SYMBOL(drm_fb_gray8_to_mono_reversed);
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index b30ed5de0a33..cd4c8b7c78de 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -43,4 +43,6 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>   		     const void *vmap, const struct drm_framebuffer *fb,
>   		     const struct drm_rect *rect);
>   
> +void drm_fb_gray8_to_mono_reversed(void *dst, void *vaddr, const struct drm_rect *clip);
> +
>   #endif /* __LINUX_DRM_FORMAT_HELPER_H */

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/4] drm/format-helper: Add drm_fb_gray8_to_mono_reversed()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3413 bytes --]

Hi

Am 31.01.22 um 21:12 schrieb Javier Martinez Canillas:
> Add support to convert 8-bit grayscale to reversed monochrome for drivers
> that control monochromatic displays, that only have 1 bit per pixel depth.
> 
> This helper function was based on repaper_gray8_to_mono_reversed() from
> the drivers/gpu/drm/tiny/repaper.c driver.

You could convert repaper to the new helper.

> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>   drivers/gpu/drm/drm_format_helper.c | 35 +++++++++++++++++++++++++++++
>   include/drm/drm_format_helper.h     |  2 ++
>   2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index 0f28dd2bdd72..bf477c136082 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>   	return -EINVAL;
>   }
>   EXPORT_SYMBOL(drm_fb_blit_toio);
> +
> +/**
> + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
> + * @dst: reversed monochrome destination buffer
> + * @src: 8-bit grayscale source buffer
> + * @clip: Clip rectangle area to copy
> + *
> + * DRM doesn't have native monochrome or grayscale support.
> + * Such drivers can announce the commonly supported XR24 format to userspace
> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
> + * helper function to convert to the native format.
> + */
> +void drm_fb_gray8_to_mono_reversed(void *dst, void *src, const struct drm_rect *clip)

IMHO it would be better to have a function that converts xrgb8888 to 
mono and let it handle the intermediate step of gray8.

> +{
> +	size_t width = drm_rect_width(clip);
> +	size_t height = drm_rect_width(clip);
> +
> +	u8 *mono = dst, *gray8 = src;
> +	unsigned int y, xb, i;
> +
> +	for (y = 0; y < height; y++)
> +		for (xb = 0; xb < width / 8; xb++) {

The inner loop should probably go into a separate helper function. See 
the drm_fb_*_line() helpers in this file

At some point, we's want to have a single blit helper that takes source 
and destination formats/buffers. It would then pick the correct per-line 
helper for the conversion. So yeah, we'd want something composable.

Best regards
Thomas

> +			u8 byte = 0x00;
> +
> +			for (i = 0; i < 8; i++) {
> +				int x = xb * 8 + i;
> +
> +				byte >>= 1;
> +				if (gray8[y * width + x] >> 7)
> +					byte |= BIT(7);
> +			}
> +			*mono++ = byte;
> +		}
> +}
> +EXPORT_SYMBOL(drm_fb_gray8_to_mono_reversed);
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index b30ed5de0a33..cd4c8b7c78de 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -43,4 +43,6 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>   		     const void *vmap, const struct drm_framebuffer *fb,
>   		     const struct drm_rect *rect);
>   
> +void drm_fb_gray8_to_mono_reversed(void *dst, void *vaddr, const struct drm_rect *clip);
> +
>   #endif /* __LINUX_DRM_FORMAT_HELPER_H */

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1328 bytes --]

Hi

Am 01.02.22 um 09:36 schrieb Geert Uytterhoeven:
> Hi Simon,
> 
> On Tue, Feb 1, 2022 at 9:34 AM Simon Ser <contact@emersion.fr> wrote:
>> On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> What's the story with the Rn formats?
>>>
>>> The comments say "n bpp Red", while this is a monochrome (even
>>> inverted) display?
>>
>> I don't think the color matters that much. "Red" was picked just because it was
>> an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?
> 
> I'd expect 8-bit grayscale to be Y8 instead.

I like this naming, but DRM_FORMAT_R8 is uapi already. :/ If anything, 
we could add Yn formats in addition to existing Rn formats.

Best regards
Thomas

> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1328 bytes --]

Hi

Am 01.02.22 um 09:36 schrieb Geert Uytterhoeven:
> Hi Simon,
> 
> On Tue, Feb 1, 2022 at 9:34 AM Simon Ser <contact@emersion.fr> wrote:
>> On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> What's the story with the Rn formats?
>>>
>>> The comments say "n bpp Red", while this is a monochrome (even
>>> inverted) display?
>>
>> I don't think the color matters that much. "Red" was picked just because it was
>> an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?
> 
> I'd expect 8-bit grayscale to be Y8 instead.

I like this naming, but DRM_FORMAT_R8 is uapi already. :/ If anything, 
we could add Yn formats in addition to existing Rn formats.

Best regards
Thomas

> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Simon Ser]

On Tuesday, February 1st, 2022 at 11:08, Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Am 01.02.22 um 09:36 schrieb Geert Uytterhoeven:
>
> > I'd expect 8-bit grayscale to be Y8 instead.
>
> I like this naming, but DRM_FORMAT_R8 is uapi already. :/ If anything,
> we could add Yn formats in addition to existing Rn formats.

Need to be a bit careful, e.g. Y210 exists and isn't a grayscale format.
This could be confusing.

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Simon Ser]

On Tuesday, February 1st, 2022 at 11:08, Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Am 01.02.22 um 09:36 schrieb Geert Uytterhoeven:
>
> > I'd expect 8-bit grayscale to be Y8 instead.
>
> I like this naming, but DRM_FORMAT_R8 is uapi already. :/ If anything,
> we could add Yn formats in addition to existing Rn formats.

Need to be a bit careful, e.g. Y210 exists and isn't a grayscale format.
This could be confusing.

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 870 bytes --]

Hi

Am 01.02.22 um 11:11 schrieb Simon Ser:
> On Tuesday, February 1st, 2022 at 11:08, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 
>> Am 01.02.22 um 09:36 schrieb Geert Uytterhoeven:
>>
>>> I'd expect 8-bit grayscale to be Y8 instead.
>>
>> I like this naming, but DRM_FORMAT_R8 is uapi already. :/ If anything,
>> we could add Yn formats in addition to existing Rn formats.
> 
> Need to be a bit careful, e.g. Y210 exists and isn't a grayscale format.
> This could be confusing.

Well, ok.  How about 'I' as in 'intensity'? There aren't too many 
drivers supporting this yet. So if we want to find a better name, now's 
the time.

Best regards
Thomas


-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 870 bytes --]

Hi

Am 01.02.22 um 11:11 schrieb Simon Ser:
> On Tuesday, February 1st, 2022 at 11:08, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 
>> Am 01.02.22 um 09:36 schrieb Geert Uytterhoeven:
>>
>>> I'd expect 8-bit grayscale to be Y8 instead.
>>
>> I like this naming, but DRM_FORMAT_R8 is uapi already. :/ If anything,
>> we could add Yn formats in addition to existing Rn formats.
> 
> Need to be a bit careful, e.g. Y210 exists and isn't a grayscale format.
> This could be confusing.

Well, ok.  How about 'I' as in 'intensity'? There aren't too many 
drivers supporting this yet. So if we want to find a better name, now's 
the time.

Best regards
Thomas


-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Geert,

On 2/1/22 09:43, Geert Uytterhoeven wrote:
> Hi Javier,
> 
> On Mon, Jan 31, 2022 at 9:12 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
>> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.
> 
> Thanks for your series!
> 
> I'll give it a try on an Adafruit FeatherWing 128x32 OLED, connected
> to an OrangeCrab ECP5 FPGA board running a 64 MHz VexRiscv RISC-V
> softcore.
>

Awesome! let me know if you have any issues. I keep an update-to-date version
at https://github.com/martinezjavier/linux/tree/ssd1307

>> Using the DRM fb emulation, all the tests from Geert Uytterhoeven's fbtest
>> (https://git.kernel.org/pub/scm/linux/kernel/git/geert/fbtest.git) passes:
>>
>>      ./fbtest -f /dev/fb1
>>     Using drawops cfb32 (32 bpp packed pixels)
>>     Available visuals:
>>       Monochrome
>>       Grayscale 256
>>       Truecolor 8:8:8:0
> 
> Oh, fake 32-bpp truecolor ;-)
>

Yes :) that's what the repaper drivers does to have maximum compatibility
with existing user-space and I followed the same.
 
> Does it run modetest, too?
>

It does, yes. And for example `modetest -M ssd1307` will print all the
info about encoders, connectors, CRTs, etc.
 
> I'm trying to get modetest working on my atari DRM driver.
> Comparing to the cirrus driver doesn't help much, as modetest doesn't
> seem to work with the cirrus driver (modified to not do hardware
> access, as I don't have cirrus hardware):
> 
>     # modetest -M cirrus -s 31:1024x768-60Hz
>     setting mode 1024x768-60.00Hz on connectors 31, crtc 34
>     failed to set gamma: Function not implemented
>

# modetest -M ssd1307 -c -s 31:128x64-0.12Hz
...
setting mode 128x64-0.12Hz on connectors 31, crtc 33
failed to set gamma: Function not implemented

this seems to be a bug in modetest. I found a patch posted some time ago
but never landed: https://www.spinics.net/lists/dri-devel/msg251356.html
 
> Does there exist another simple test program for showing something
> using the DRM API?
>

I tested with plymouth and gdm that make use of the DRM API, they do
start and I see something on the screen but don't really handle that
well the fact that's a 128x64 resolution.

I didn't test with more DRM programs because was mostly interested in
making sure that the fbdev emulation was working correctly.

Noticed that Simon shared some simple examples, I'll give them a try. 

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Geert,

On 2/1/22 09:43, Geert Uytterhoeven wrote:
> Hi Javier,
> 
> On Mon, Jan 31, 2022 at 9:12 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
>> SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.
> 
> Thanks for your series!
> 
> I'll give it a try on an Adafruit FeatherWing 128x32 OLED, connected
> to an OrangeCrab ECP5 FPGA board running a 64 MHz VexRiscv RISC-V
> softcore.
>

Awesome! let me know if you have any issues. I keep an update-to-date version
at https://github.com/martinezjavier/linux/tree/ssd1307

>> Using the DRM fb emulation, all the tests from Geert Uytterhoeven's fbtest
>> (https://git.kernel.org/pub/scm/linux/kernel/git/geert/fbtest.git) passes:
>>
>>      ./fbtest -f /dev/fb1
>>     Using drawops cfb32 (32 bpp packed pixels)
>>     Available visuals:
>>       Monochrome
>>       Grayscale 256
>>       Truecolor 8:8:8:0
> 
> Oh, fake 32-bpp truecolor ;-)
>

Yes :) that's what the repaper drivers does to have maximum compatibility
with existing user-space and I followed the same.
 
> Does it run modetest, too?
>

It does, yes. And for example `modetest -M ssd1307` will print all the
info about encoders, connectors, CRTs, etc.
 
> I'm trying to get modetest working on my atari DRM driver.
> Comparing to the cirrus driver doesn't help much, as modetest doesn't
> seem to work with the cirrus driver (modified to not do hardware
> access, as I don't have cirrus hardware):
> 
>     # modetest -M cirrus -s 31:1024x768-60Hz
>     setting mode 1024x768-60.00Hz on connectors 31, crtc 34
>     failed to set gamma: Function not implemented
>

# modetest -M ssd1307 -c -s 31:128x64-0.12Hz
...
setting mode 128x64-0.12Hz on connectors 31, crtc 33
failed to set gamma: Function not implemented

this seems to be a bug in modetest. I found a patch posted some time ago
but never landed: https://www.spinics.net/lists/dri-devel/msg251356.html
 
> Does there exist another simple test program for showing something
> using the DRM API?
>

I tested with plymouth and gdm that make use of the DRM API, they do
start and I see something on the screen but don't really handle that
well the fact that's a 128x64 resolution.

I didn't test with more DRM programs because was mostly interested in
making sure that the fbdev emulation was working correctly.

Noticed that Simon shared some simple examples, I'll give them a try. 

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 5027 bytes --]

On Tue, 1 Feb 2022 10:49:03 +0100
Javier Martinez Canillas <javierm@redhat.com> wrote:

> On 2/1/22 09:38, Daniel Vetter wrote:
> > On Tue, Feb 1, 2022 at 9:34 AM Simon Ser <contact@emersion.fr> wrote:  
> >>
> >> On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>  
> >>> What's the story with the Rn formats?
> >>>
> >>> The comments say "n bpp Red", while this is a monochrome (even
> >>> inverted) display?  
> >>
> >> I don't think the color matters that much. "Red" was picked just because it was
> >> an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?  
> > 
> > The red comes from gl, where with shaders it really doesn't matter
> > what meaning you attach to channels, but really just how many you
> > have. So 2-channel formats are called RxGx, 3-channel RxGxBx,
> > 4-channel RxGxBxAx and single-channel Rx. And we use drm_fourcc for
> > interop in general, hence why these exist.
> > 
> > We should probably make a comment that this really isn't a red channel
> > when used for display it's a greyscale/intensity format. Aside from
> > that documentation gap I think reusing Rx formats for
> > greyscale/intensity for display makes perfect sense.
> > -Daniel  
> 
> To sump up the conversation in the #dri-devel channel, these drivers
> should support the following formats:
> 
> 1) Dx (Daniel suggested that for darkness, but inverted mono)

Did you consider format C1 instead?

To my understanding, the C formats are paletted, which would also fit
very nicely semantically. You have an enumerated list of pixel values
and each of them produces some arbitrary color on screen. This would
fit e.g. blue/white LCD panels nicely.

The little problem there is the palette.

C8 format is traditionally translated to RGB triplets through GAMMA LUT.
Therefore the display itself is still three-channel, it's just the
framebuffer format that is single-channel. But now, we are dealing
with truly paletted displays. Furthermore, the palette is fixed,
ingrained in the panel hardware.

So we would probably need a new KMS property for the fixed palette of
the panel. What would it be called? Would it be a connector property?

The property would be a read-only blob, an array that maps Cx values to
"colors". How do we represent "colors"? How do we accommodate C1, C2,
C4 and C8 with the same blob?

Since the blob is a mapping from color index to "color", and the array
in the blob has N entries, we could simply say that Cx integer value is
the color index. If the Cx you use does not go up to N, then you miss
some colors. If the Cx you use can go higher than N, then Cx values >=
N will clamp to N-1, for example. Of course, if your panel palette has
only 4 entries, you can expose C1 and C2 and have no reason to expose
C4 or C8, avoiding the Cx >= N issue.

How do we define the array contents then, the "colors"... plain old RGB
triplets do not mean much[1], but that would be better than nothing. I
also suppose that people would not be keen on seeing something like CIE
1931 XYZ or Lab values, even though those would probably have the most
useful definition. Coming up with those values properly would require a
colorimeter. As a compromise, maybe we could use an RGB triplet, and
assume sRGB SDR color space and transfer function, just like we do with
all displays whether they are that or not. If someone needs to know
better, then they can profile the display. sRGB triplets would likely
give enough intuition to what color the indices result in, that it
could be used in automated color conversions or quantizations from
larger color spaces like sRGB with some rough degree of color
similarity.

It is a lot of hassle, but it would have a clear benefit: userspace
would know very well how the display behaves (what colors it shows,
roughly), and you could use Cx formats to drive a panel in its "native"
format.

Possible problems are around interactions with the old GAMMA property,
which is traditionally used for the C8 palette. But if you have a
fixed-palette panel, then maybe you wouldn't expose GAMMA property on
the CRTC at all?

I have no idea how this would map to fbdev API though.


[1] https://gitlab.freedesktop.org/pq/color-and-hdr/-/blob/main/doc/pixels_color.md

Thanks,
pq


> 2) Rx (single-channel for grayscale)
> 3) RxGxBxAx (4-channel fake 32-bpp truecolor)
> 
> The format preference will be in that order, so if user-space is able
> to use Dx then there won't be a need for any conversion and just the
> native format will be used.
> 
> If using Rx then only a Rx -> Dx conversion will happen and the last
> format will require the less performant RxGxBxAx -> Rx -> Dx path.
> 
> But we still need RxGxBxAx as a fallback for compatibility with the
> existing user-space, so all this could be done as a follow-up as an
> optimization and shouldn't block monochromatic panel drivers IMO.
> 
> Best regards,


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 5027 bytes --]

On Tue, 1 Feb 2022 10:49:03 +0100
Javier Martinez Canillas <javierm@redhat.com> wrote:

> On 2/1/22 09:38, Daniel Vetter wrote:
> > On Tue, Feb 1, 2022 at 9:34 AM Simon Ser <contact@emersion.fr> wrote:  
> >>
> >> On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>  
> >>> What's the story with the Rn formats?
> >>>
> >>> The comments say "n bpp Red", while this is a monochrome (even
> >>> inverted) display?  
> >>
> >> I don't think the color matters that much. "Red" was picked just because it was
> >> an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?  
> > 
> > The red comes from gl, where with shaders it really doesn't matter
> > what meaning you attach to channels, but really just how many you
> > have. So 2-channel formats are called RxGx, 3-channel RxGxBx,
> > 4-channel RxGxBxAx and single-channel Rx. And we use drm_fourcc for
> > interop in general, hence why these exist.
> > 
> > We should probably make a comment that this really isn't a red channel
> > when used for display it's a greyscale/intensity format. Aside from
> > that documentation gap I think reusing Rx formats for
> > greyscale/intensity for display makes perfect sense.
> > -Daniel  
> 
> To sump up the conversation in the #dri-devel channel, these drivers
> should support the following formats:
> 
> 1) Dx (Daniel suggested that for darkness, but inverted mono)

Did you consider format C1 instead?

To my understanding, the C formats are paletted, which would also fit
very nicely semantically. You have an enumerated list of pixel values
and each of them produces some arbitrary color on screen. This would
fit e.g. blue/white LCD panels nicely.

The little problem there is the palette.

C8 format is traditionally translated to RGB triplets through GAMMA LUT.
Therefore the display itself is still three-channel, it's just the
framebuffer format that is single-channel. But now, we are dealing
with truly paletted displays. Furthermore, the palette is fixed,
ingrained in the panel hardware.

So we would probably need a new KMS property for the fixed palette of
the panel. What would it be called? Would it be a connector property?

The property would be a read-only blob, an array that maps Cx values to
"colors". How do we represent "colors"? How do we accommodate C1, C2,
C4 and C8 with the same blob?

Since the blob is a mapping from color index to "color", and the array
in the blob has N entries, we could simply say that Cx integer value is
the color index. If the Cx you use does not go up to N, then you miss
some colors. If the Cx you use can go higher than N, then Cx values >=
N will clamp to N-1, for example. Of course, if your panel palette has
only 4 entries, you can expose C1 and C2 and have no reason to expose
C4 or C8, avoiding the Cx >= N issue.

How do we define the array contents then, the "colors"... plain old RGB
triplets do not mean much[1], but that would be better than nothing. I
also suppose that people would not be keen on seeing something like CIE
1931 XYZ or Lab values, even though those would probably have the most
useful definition. Coming up with those values properly would require a
colorimeter. As a compromise, maybe we could use an RGB triplet, and
assume sRGB SDR color space and transfer function, just like we do with
all displays whether they are that or not. If someone needs to know
better, then they can profile the display. sRGB triplets would likely
give enough intuition to what color the indices result in, that it
could be used in automated color conversions or quantizations from
larger color spaces like sRGB with some rough degree of color
similarity.

It is a lot of hassle, but it would have a clear benefit: userspace
would know very well how the display behaves (what colors it shows,
roughly), and you could use Cx formats to drive a panel in its "native"
format.

Possible problems are around interactions with the old GAMMA property,
which is traditionally used for the C8 palette. But if you have a
fixed-palette panel, then maybe you wouldn't expose GAMMA property on
the CRTC at all?

I have no idea how this would map to fbdev API though.


[1] https://gitlab.freedesktop.org/pq/color-and-hdr/-/blob/main/doc/pixels_color.md

Thanks,
pq


> 2) Rx (single-channel for grayscale)
> 3) RxGxBxAx (4-channel fake 32-bpp truecolor)
> 
> The format preference will be in that order, so if user-space is able
> to use Dx then there won't be a need for any conversion and just the
> native format will be used.
> 
> If using Rx then only a Rx -> Dx conversion will happen and the last
> format will require the less performant RxGxBxAx -> Rx -> Dx path.
> 
> But we still need RxGxBxAx as a fallback for compatibility with the
> existing user-space, so all this could be done as a follow-up as an
> optimization and shouldn't block monochromatic panel drivers IMO.
> 
> Best regards,


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Pekka,

On Tue, Feb 1, 2022 at 11:42 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Tue, 1 Feb 2022 10:49:03 +0100
> Javier Martinez Canillas <javierm@redhat.com> wrote:
> > On 2/1/22 09:38, Daniel Vetter wrote:
> > > On Tue, Feb 1, 2022 at 9:34 AM Simon Ser <contact@emersion.fr> wrote:
> > >> On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > >>> What's the story with the Rn formats?
> > >>>
> > >>> The comments say "n bpp Red", while this is a monochrome (even
> > >>> inverted) display?
> > >>
> > >> I don't think the color matters that much. "Red" was picked just because it was
> > >> an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?
> > >
> > > The red comes from gl, where with shaders it really doesn't matter
> > > what meaning you attach to channels, but really just how many you
> > > have. So 2-channel formats are called RxGx, 3-channel RxGxBx,
> > > 4-channel RxGxBxAx and single-channel Rx. And we use drm_fourcc for
> > > interop in general, hence why these exist.
> > >
> > > We should probably make a comment that this really isn't a red channel
> > > when used for display it's a greyscale/intensity format. Aside from
> > > that documentation gap I think reusing Rx formats for
> > > greyscale/intensity for display makes perfect sense.
> > > -Daniel
> >
> > To sump up the conversation in the #dri-devel channel, these drivers
> > should support the following formats:
> >
> > 1) Dx (Daniel suggested that for darkness, but inverted mono)
>
> Did you consider format C1 instead?

That would be a 2-color display, which is not necessarily black
and white. Cfr. Amiga or Atari bit planes with bpp=1.
That's why fbdev has separate visuals for monochrome.

> I have no idea how this would map to fbdev API though.

    #define FB_VISUAL_MONO01                0       /* Monochr.
1=Black 0=White */
    #define FB_VISUAL_MONO10                1       /* Monochr.
1=White 0=Black */
    #define FB_VISUAL_TRUECOLOR             2       /* True color   */

The above is RGB (or grayscale, see below).

    #define FB_VISUAL_PSEUDOCOLOR           3       /* Pseudo color
(like atari) */

Palette

    #define FB_VISUAL_DIRECTCOLOR           4       /* Direct color */

Usually used as RGB with gamma correction, but the actual hardware
is more flexible.

    #define FB_VISUAL_STATIC_PSEUDOCOLOR    5       /* Pseudo color readonly */

Fixed palette

And:

    struct fb_var_screeninfo {
            ...
            __u32 grayscale;                /* 0 = color, 1 = grayscale,    */

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Pekka,

On Tue, Feb 1, 2022 at 11:42 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Tue, 1 Feb 2022 10:49:03 +0100
> Javier Martinez Canillas <javierm@redhat.com> wrote:
> > On 2/1/22 09:38, Daniel Vetter wrote:
> > > On Tue, Feb 1, 2022 at 9:34 AM Simon Ser <contact@emersion.fr> wrote:
> > >> On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > >>> What's the story with the Rn formats?
> > >>>
> > >>> The comments say "n bpp Red", while this is a monochrome (even
> > >>> inverted) display?
> > >>
> > >> I don't think the color matters that much. "Red" was picked just because it was
> > >> an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?
> > >
> > > The red comes from gl, where with shaders it really doesn't matter
> > > what meaning you attach to channels, but really just how many you
> > > have. So 2-channel formats are called RxGx, 3-channel RxGxBx,
> > > 4-channel RxGxBxAx and single-channel Rx. And we use drm_fourcc for
> > > interop in general, hence why these exist.
> > >
> > > We should probably make a comment that this really isn't a red channel
> > > when used for display it's a greyscale/intensity format. Aside from
> > > that documentation gap I think reusing Rx formats for
> > > greyscale/intensity for display makes perfect sense.
> > > -Daniel
> >
> > To sump up the conversation in the #dri-devel channel, these drivers
> > should support the following formats:
> >
> > 1) Dx (Daniel suggested that for darkness, but inverted mono)
>
> Did you consider format C1 instead?

That would be a 2-color display, which is not necessarily black
and white. Cfr. Amiga or Atari bit planes with bpp=1.
That's why fbdev has separate visuals for monochrome.

> I have no idea how this would map to fbdev API though.

    #define FB_VISUAL_MONO01                0       /* Monochr.
1=Black 0=White */
    #define FB_VISUAL_MONO10                1       /* Monochr.
1=White 0=Black */
    #define FB_VISUAL_TRUECOLOR             2       /* True color   */

The above is RGB (or grayscale, see below).

    #define FB_VISUAL_PSEUDOCOLOR           3       /* Pseudo color
(like atari) */

Palette

    #define FB_VISUAL_DIRECTCOLOR           4       /* Direct color */

Usually used as RGB with gamma correction, but the actual hardware
is more flexible.

    #define FB_VISUAL_STATIC_PSEUDOCOLOR    5       /* Pseudo color readonly */

Fixed palette

And:

    struct fb_var_screeninfo {
            ...
            __u32 grayscale;                /* 0 = color, 1 = grayscale,    */

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/4] drm/format-helper: Add drm_fb_gray8_to_mono_reversed()

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3068 bytes --]

On Tue, 1 Feb 2022 10:59:43 +0100
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Hi
> 
> Am 31.01.22 um 21:12 schrieb Javier Martinez Canillas:
> > Add support to convert 8-bit grayscale to reversed monochrome for drivers
> > that control monochromatic displays, that only have 1 bit per pixel depth.
> > 
> > This helper function was based on repaper_gray8_to_mono_reversed() from
> > the drivers/gpu/drm/tiny/repaper.c driver.  
> 
> You could convert repaper to the new helper.
> 
> > 
> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> > ---
> > 
> >   drivers/gpu/drm/drm_format_helper.c | 35 +++++++++++++++++++++++++++++
> >   include/drm/drm_format_helper.h     |  2 ++
> >   2 files changed, 37 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> > index 0f28dd2bdd72..bf477c136082 100644
> > --- a/drivers/gpu/drm/drm_format_helper.c
> > +++ b/drivers/gpu/drm/drm_format_helper.c
> > @@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
> >   	return -EINVAL;
> >   }
> >   EXPORT_SYMBOL(drm_fb_blit_toio);
> > +
> > +/**
> > + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
> > + * @dst: reversed monochrome destination buffer
> > + * @src: 8-bit grayscale source buffer
> > + * @clip: Clip rectangle area to copy
> > + *
> > + * DRM doesn't have native monochrome or grayscale support.
> > + * Such drivers can announce the commonly supported XR24 format to userspace
> > + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
> > + * helper function to convert to the native format.
> > + */
> > +void drm_fb_gray8_to_mono_reversed(void *dst, void *src, const struct drm_rect *clip)  
> 
> IMHO it would be better to have a function that converts xrgb8888 to 
> mono and let it handle the intermediate step of gray8.
> 
> > +{
> > +	size_t width = drm_rect_width(clip);
> > +	size_t height = drm_rect_width(clip);
> > +
> > +	u8 *mono = dst, *gray8 = src;
> > +	unsigned int y, xb, i;
> > +
> > +	for (y = 0; y < height; y++)
> > +		for (xb = 0; xb < width / 8; xb++) {  
> 
> The inner loop should probably go into a separate helper function. See 
> the drm_fb_*_line() helpers in this file
> 
> At some point, we's want to have a single blit helper that takes source 
> and destination formats/buffers. It would then pick the correct per-line 
> helper for the conversion. So yeah, we'd want something composable.

Btw. VKMS is going to do blending, and it needs to support various
formats, so the latest patches from Igor probably do something similar.


Thanks,
pq


> 
> Best regards
> Thomas
> 
> > +			u8 byte = 0x00;
> > +
> > +			for (i = 0; i < 8; i++) {
> > +				int x = xb * 8 + i;
> > +
> > +				byte >>= 1;
> > +				if (gray8[y * width + x] >> 7)
> > +					byte |= BIT(7);
> > +			}
> > +			*mono++ = byte;
> > +		}
> > +}
> > +EXPORT_SYMBOL(drm_fb_gray8_to_mono_reversed);

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] drm/format-helper: Add drm_fb_gray8_to_mono_reversed()

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3068 bytes --]

On Tue, 1 Feb 2022 10:59:43 +0100
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Hi
> 
> Am 31.01.22 um 21:12 schrieb Javier Martinez Canillas:
> > Add support to convert 8-bit grayscale to reversed monochrome for drivers
> > that control monochromatic displays, that only have 1 bit per pixel depth.
> > 
> > This helper function was based on repaper_gray8_to_mono_reversed() from
> > the drivers/gpu/drm/tiny/repaper.c driver.  
> 
> You could convert repaper to the new helper.
> 
> > 
> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> > ---
> > 
> >   drivers/gpu/drm/drm_format_helper.c | 35 +++++++++++++++++++++++++++++
> >   include/drm/drm_format_helper.h     |  2 ++
> >   2 files changed, 37 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> > index 0f28dd2bdd72..bf477c136082 100644
> > --- a/drivers/gpu/drm/drm_format_helper.c
> > +++ b/drivers/gpu/drm/drm_format_helper.c
> > @@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
> >   	return -EINVAL;
> >   }
> >   EXPORT_SYMBOL(drm_fb_blit_toio);
> > +
> > +/**
> > + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
> > + * @dst: reversed monochrome destination buffer
> > + * @src: 8-bit grayscale source buffer
> > + * @clip: Clip rectangle area to copy
> > + *
> > + * DRM doesn't have native monochrome or grayscale support.
> > + * Such drivers can announce the commonly supported XR24 format to userspace
> > + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
> > + * helper function to convert to the native format.
> > + */
> > +void drm_fb_gray8_to_mono_reversed(void *dst, void *src, const struct drm_rect *clip)  
> 
> IMHO it would be better to have a function that converts xrgb8888 to 
> mono and let it handle the intermediate step of gray8.
> 
> > +{
> > +	size_t width = drm_rect_width(clip);
> > +	size_t height = drm_rect_width(clip);
> > +
> > +	u8 *mono = dst, *gray8 = src;
> > +	unsigned int y, xb, i;
> > +
> > +	for (y = 0; y < height; y++)
> > +		for (xb = 0; xb < width / 8; xb++) {  
> 
> The inner loop should probably go into a separate helper function. See 
> the drm_fb_*_line() helpers in this file
> 
> At some point, we's want to have a single blit helper that takes source 
> and destination formats/buffers. It would then pick the correct per-line 
> helper for the conversion. So yeah, we'd want something composable.

Btw. VKMS is going to do blending, and it needs to support various
formats, so the latest patches from Igor probably do something similar.


Thanks,
pq


> 
> Best regards
> Thomas
> 
> > +			u8 byte = 0x00;
> > +
> > +			for (i = 0; i < 8; i++) {
> > +				int x = xb * 8 + i;
> > +
> > +				byte >>= 1;
> > +				if (gray8[y * width + x] >> 7)
> > +					byte |= BIT(7);
> > +			}
> > +			*mono++ = byte;
> > +		}
> > +}
> > +EXPORT_SYMBOL(drm_fb_gray8_to_mono_reversed);

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

On 2/1/22 10:37, Andy Shevchenko wrote:
> On Mon, Jan 31, 2022 at 09:56:23PM +0100, Sam Ravnborg wrote:
>> On Mon, Jan 31, 2022 at 09:12:20PM +0100, Javier Martinez Canillas wrote:
> 
> ...
> 
>>> Patch #3 adds the driver. The name ssd1307 was used instead of ssd130x
>>> (which would be more accurate) to avoid confusion for users who want to
>>> migrate from the existing ssd1307fb fbdev driver.
>> Looking forward the name ssd130x would make more sense. There is only so
>> many existing users and a potential of much more new users.
>> So in my color of the world the naming that benefits the most users
>> wins.
> 
> It depends if the binding is going to be preserved. Also this series doesn't
> answer to the question what to do with the old driver.
>

I don't plan to remove the old driver (yet). My goal here is to have an answer
for Fedora users that might complain that we disabled all the fbdev drivers.

So I wanted to understand the effort involved in porting a fbdev driver to DRM.

> If you leave it, I would expect the backward compatibility, otherwise the
> series misses removal of the old driver.
> 

I don't see how those two are correlated. You just need different compatible
strings to match the new and old drivers. That what was usually done for DRM
drivers that were ported. To give an example, the "omapfb" vs "omapdrm".

Since the current binding has a compatible "ssd1305fb-i2c", we could make the
new one "ssd1305drm-i2c" or better, just "ssd1305-i2c".

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

On 2/1/22 10:37, Andy Shevchenko wrote:
> On Mon, Jan 31, 2022 at 09:56:23PM +0100, Sam Ravnborg wrote:
>> On Mon, Jan 31, 2022 at 09:12:20PM +0100, Javier Martinez Canillas wrote:
> 
> ...
> 
>>> Patch #3 adds the driver. The name ssd1307 was used instead of ssd130x
>>> (which would be more accurate) to avoid confusion for users who want to
>>> migrate from the existing ssd1307fb fbdev driver.
>> Looking forward the name ssd130x would make more sense. There is only so
>> many existing users and a potential of much more new users.
>> So in my color of the world the naming that benefits the most users
>> wins.
> 
> It depends if the binding is going to be preserved. Also this series doesn't
> answer to the question what to do with the old driver.
>

I don't plan to remove the old driver (yet). My goal here is to have an answer
for Fedora users that might complain that we disabled all the fbdev drivers.

So I wanted to understand the effort involved in porting a fbdev driver to DRM.

> If you leave it, I would expect the backward compatibility, otherwise the
> series misses removal of the old driver.
> 

I don't see how those two are correlated. You just need different compatible
strings to match the new and old drivers. That what was usually done for DRM
drivers that were ported. To give an example, the "omapfb" vs "omapdrm".

Since the current binding has a compatible "ssd1305fb-i2c", we could make the
new one "ssd1305drm-i2c" or better, just "ssd1305-i2c".

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Javier,

On Tue, Feb 1, 2022 at 12:31 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> On 2/1/22 10:37, Andy Shevchenko wrote:
> > On Mon, Jan 31, 2022 at 09:56:23PM +0100, Sam Ravnborg wrote:
> >> On Mon, Jan 31, 2022 at 09:12:20PM +0100, Javier Martinez Canillas wrote:
> >
> > ...
> >
> >>> Patch #3 adds the driver. The name ssd1307 was used instead of ssd130x
> >>> (which would be more accurate) to avoid confusion for users who want to
> >>> migrate from the existing ssd1307fb fbdev driver.
> >> Looking forward the name ssd130x would make more sense. There is only so
> >> many existing users and a potential of much more new users.
> >> So in my color of the world the naming that benefits the most users
> >> wins.
> >
> > It depends if the binding is going to be preserved. Also this series doesn't
> > answer to the question what to do with the old driver.
> >
>
> I don't plan to remove the old driver (yet). My goal here is to have an answer
> for Fedora users that might complain that we disabled all the fbdev drivers.
>
> So I wanted to understand the effort involved in porting a fbdev driver to DRM.
>
> > If you leave it, I would expect the backward compatibility, otherwise the
> > series misses removal of the old driver.
> >
>
> I don't see how those two are correlated. You just need different compatible
> strings to match the new and old drivers. That what was usually done for DRM
> drivers that were ported. To give an example, the "omapfb" vs "omapdrm".
>
> Since the current binding has a compatible "ssd1305fb-i2c", we could make the
> new one "ssd1305drm-i2c" or better, just "ssd1305-i2c".

DT describes hardware, not software policy.
If the hardware is the same, the DT bindings should stay the same.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Javier,

On Tue, Feb 1, 2022 at 12:31 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> On 2/1/22 10:37, Andy Shevchenko wrote:
> > On Mon, Jan 31, 2022 at 09:56:23PM +0100, Sam Ravnborg wrote:
> >> On Mon, Jan 31, 2022 at 09:12:20PM +0100, Javier Martinez Canillas wrote:
> >
> > ...
> >
> >>> Patch #3 adds the driver. The name ssd1307 was used instead of ssd130x
> >>> (which would be more accurate) to avoid confusion for users who want to
> >>> migrate from the existing ssd1307fb fbdev driver.
> >> Looking forward the name ssd130x would make more sense. There is only so
> >> many existing users and a potential of much more new users.
> >> So in my color of the world the naming that benefits the most users
> >> wins.
> >
> > It depends if the binding is going to be preserved. Also this series doesn't
> > answer to the question what to do with the old driver.
> >
>
> I don't plan to remove the old driver (yet). My goal here is to have an answer
> for Fedora users that might complain that we disabled all the fbdev drivers.
>
> So I wanted to understand the effort involved in porting a fbdev driver to DRM.
>
> > If you leave it, I would expect the backward compatibility, otherwise the
> > series misses removal of the old driver.
> >
>
> I don't see how those two are correlated. You just need different compatible
> strings to match the new and old drivers. That what was usually done for DRM
> drivers that were ported. To give an example, the "omapfb" vs "omapdrm".
>
> Since the current binding has a compatible "ssd1305fb-i2c", we could make the
> new one "ssd1305drm-i2c" or better, just "ssd1305-i2c".

DT describes hardware, not software policy.
If the hardware is the same, the DT bindings should stay the same.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/4] drm/format-helper: Add drm_fb_gray8_to_mono_reversed()

[Javier Martinez Canillas]

Hello Thomas,

Thanks a lot for your feedback.

On 2/1/22 10:59, Thomas Zimmermann wrote:
> Hi
> 
> Am 31.01.22 um 21:12 schrieb Javier Martinez Canillas:
>> Add support to convert 8-bit grayscale to reversed monochrome for drivers
>> that control monochromatic displays, that only have 1 bit per pixel depth.
>>
>> This helper function was based on repaper_gray8_to_mono_reversed() from
>> the drivers/gpu/drm/tiny/repaper.c driver.
> 
> You could convert repaper to the new helper.
>

Yes, I plan to do it but was out of scope for this patch series.

>>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>>
>>   drivers/gpu/drm/drm_format_helper.c | 35 +++++++++++++++++++++++++++++
>>   include/drm/drm_format_helper.h     |  2 ++
>>   2 files changed, 37 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
>> index 0f28dd2bdd72..bf477c136082 100644
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
>> @@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>>   	return -EINVAL;
>>   }
>>   EXPORT_SYMBOL(drm_fb_blit_toio);
>> +
>> +/**
>> + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
>> + * @dst: reversed monochrome destination buffer
>> + * @src: 8-bit grayscale source buffer
>> + * @clip: Clip rectangle area to copy
>> + *
>> + * DRM doesn't have native monochrome or grayscale support.
>> + * Such drivers can announce the commonly supported XR24 format to userspace
>> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
>> + * helper function to convert to the native format.
>> + */
>> +void drm_fb_gray8_to_mono_reversed(void *dst, void *src, const struct drm_rect *clip)
> 
> IMHO it would be better to have a function that converts xrgb8888 to 
> mono and let it handle the intermediate step of gray8.
>

That's a good idea. I'll add that too.

>> +{
>> +	size_t width = drm_rect_width(clip);
>> +	size_t height = drm_rect_width(clip);
>> +
>> +	u8 *mono = dst, *gray8 = src;
>> +	unsigned int y, xb, i;
>> +
>> +	for (y = 0; y < height; y++)
>> +		for (xb = 0; xb < width / 8; xb++) {
> 
> The inner loop should probably go into a separate helper function. See 
> the drm_fb_*_line() helpers in this file
>
> At some point, we's want to have a single blit helper that takes source 
> and destination formats/buffers. It would then pick the correct per-line 
> helper for the conversion. So yeah, we'd want something composable.
>

Agreed. I'll split that into a separate helper function.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 2/4] drm/format-helper: Add drm_fb_gray8_to_mono_reversed()

[Javier Martinez Canillas]

Hello Thomas,

Thanks a lot for your feedback.

On 2/1/22 10:59, Thomas Zimmermann wrote:
> Hi
> 
> Am 31.01.22 um 21:12 schrieb Javier Martinez Canillas:
>> Add support to convert 8-bit grayscale to reversed monochrome for drivers
>> that control monochromatic displays, that only have 1 bit per pixel depth.
>>
>> This helper function was based on repaper_gray8_to_mono_reversed() from
>> the drivers/gpu/drm/tiny/repaper.c driver.
> 
> You could convert repaper to the new helper.
>

Yes, I plan to do it but was out of scope for this patch series.

>>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>>
>>   drivers/gpu/drm/drm_format_helper.c | 35 +++++++++++++++++++++++++++++
>>   include/drm/drm_format_helper.h     |  2 ++
>>   2 files changed, 37 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
>> index 0f28dd2bdd72..bf477c136082 100644
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
>> @@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>>   	return -EINVAL;
>>   }
>>   EXPORT_SYMBOL(drm_fb_blit_toio);
>> +
>> +/**
>> + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
>> + * @dst: reversed monochrome destination buffer
>> + * @src: 8-bit grayscale source buffer
>> + * @clip: Clip rectangle area to copy
>> + *
>> + * DRM doesn't have native monochrome or grayscale support.
>> + * Such drivers can announce the commonly supported XR24 format to userspace
>> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
>> + * helper function to convert to the native format.
>> + */
>> +void drm_fb_gray8_to_mono_reversed(void *dst, void *src, const struct drm_rect *clip)
> 
> IMHO it would be better to have a function that converts xrgb8888 to 
> mono and let it handle the intermediate step of gray8.
>

That's a good idea. I'll add that too.

>> +{
>> +	size_t width = drm_rect_width(clip);
>> +	size_t height = drm_rect_width(clip);
>> +
>> +	u8 *mono = dst, *gray8 = src;
>> +	unsigned int y, xb, i;
>> +
>> +	for (y = 0; y < height; y++)
>> +		for (xb = 0; xb < width / 8; xb++) {
> 
> The inner loop should probably go into a separate helper function. See 
> the drm_fb_*_line() helpers in this file
>
> At some point, we's want to have a single blit helper that takes source 
> and destination formats/buffers. It would then pick the correct per-line 
> helper for the conversion. So yeah, we'd want something composable.
>

Agreed. I'll split that into a separate helper function.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 1/4] drm: Add I2C connector type

[Noralf Trønnes]

Den 31.01.2022 21.52, skrev Sam Ravnborg:
> On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:
>> There isn't a connector type for display controllers accesed through I2C,
>> most drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.
>>
>> Add an I2C connector type to match the actual connector.
>>
>> As Noralf Trønnes mentions in commit fc06bf1d76d6 ("drm: Add SPI connector
>> type"), user-space should be able to cope with a connector type that does
>> not yet understand.
>>

It turned out that I wasn't entirely correct here, mpv didn't cope with
unknown types. In the PR to add support Emil Velikov wondered if libdrm
should handle these connector names:
https://github.com/mpv-player/mpv/pull/8989#issuecomment-879187711

>> Tested with `modetest -M ssd1307 -c` and shows the connector as unknown-1.
> I had expected unknown-21??
> 
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Sam, didn't you and Laurent discuss adding DRM_MODE_CONNECTOR_PANEL for
such a use case?

If someone adds parallel bus support to the MIPI DBI helper, there will
be one more connector type (I wonder what that one will be called).

Noralf.

>> ---
>>
>>  drivers/gpu/drm/drm_connector.c | 1 +
>>  include/uapi/drm/drm_mode.h     | 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index a50c82bc2b2f..975a7525a508 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -105,6 +105,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = {
>>  	{ DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
>>  	{ DRM_MODE_CONNECTOR_SPI, "SPI" },
>>  	{ DRM_MODE_CONNECTOR_USB, "USB" },
>> +	{ DRM_MODE_CONNECTOR_I2C, "I2C" },
>>  };
>>  
>>  void drm_connector_ida_init(void)
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index e1e351682872..d6d6288242db 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -421,6 +421,7 @@ enum drm_mode_subconnector {
>>  #define DRM_MODE_CONNECTOR_WRITEBACK	18
>>  #define DRM_MODE_CONNECTOR_SPI		19
>>  #define DRM_MODE_CONNECTOR_USB		20
>> +#define DRM_MODE_CONNECTOR_I2C		21
>>  
>>  /**
>>   * struct drm_mode_get_connector - Get connector metadata.
>> -- 
>> 2.34.1

Re: [PATCH 1/4] drm: Add I2C connector type

[Noralf Trønnes]

Den 31.01.2022 21.52, skrev Sam Ravnborg:
> On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:
>> There isn't a connector type for display controllers accesed through I2C,
>> most drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.
>>
>> Add an I2C connector type to match the actual connector.
>>
>> As Noralf Trønnes mentions in commit fc06bf1d76d6 ("drm: Add SPI connector
>> type"), user-space should be able to cope with a connector type that does
>> not yet understand.
>>

It turned out that I wasn't entirely correct here, mpv didn't cope with
unknown types. In the PR to add support Emil Velikov wondered if libdrm
should handle these connector names:
https://github.com/mpv-player/mpv/pull/8989#issuecomment-879187711

>> Tested with `modetest -M ssd1307 -c` and shows the connector as unknown-1.
> I had expected unknown-21??
> 
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Sam, didn't you and Laurent discuss adding DRM_MODE_CONNECTOR_PANEL for
such a use case?

If someone adds parallel bus support to the MIPI DBI helper, there will
be one more connector type (I wonder what that one will be called).

Noralf.

>> ---
>>
>>  drivers/gpu/drm/drm_connector.c | 1 +
>>  include/uapi/drm/drm_mode.h     | 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index a50c82bc2b2f..975a7525a508 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -105,6 +105,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = {
>>  	{ DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
>>  	{ DRM_MODE_CONNECTOR_SPI, "SPI" },
>>  	{ DRM_MODE_CONNECTOR_USB, "USB" },
>> +	{ DRM_MODE_CONNECTOR_I2C, "I2C" },
>>  };
>>  
>>  void drm_connector_ida_init(void)
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index e1e351682872..d6d6288242db 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -421,6 +421,7 @@ enum drm_mode_subconnector {
>>  #define DRM_MODE_CONNECTOR_WRITEBACK	18
>>  #define DRM_MODE_CONNECTOR_SPI		19
>>  #define DRM_MODE_CONNECTOR_USB		20
>> +#define DRM_MODE_CONNECTOR_I2C		21
>>  
>>  /**
>>   * struct drm_mode_get_connector - Get connector metadata.
>> -- 
>> 2.34.1

Re: [PATCH 1/4] drm: Add I2C connector type

[Javier Martinez Canillas]

Hello Noralf,

On 2/1/22 13:58, Noralf Trønnes wrote:
> 
> 
> Den 31.01.2022 21.52, skrev Sam Ravnborg:
>> On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:
>>> There isn't a connector type for display controllers accesed through I2C,
>>> most drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.
>>>
>>> Add an I2C connector type to match the actual connector.
>>>
>>> As Noralf Trønnes mentions in commit fc06bf1d76d6 ("drm: Add SPI connector
>>> type"), user-space should be able to cope with a connector type that does
>>> not yet understand.
>>>
> 
> It turned out that I wasn't entirely correct here, mpv didn't cope with
> unknown types. In the PR to add support Emil Velikov wondered if libdrm
> should handle these connector names:
> https://github.com/mpv-player/mpv/pull/8989#issuecomment-879187711
> 

I see, thanks for the information. What should we do then, just use the type
DRM_MODE_CONNECTOR_Unknown then ?

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Geert,

On 2/1/22 12:38, Geert Uytterhoeven wrote:

[snip]

>>
>> Since the current binding has a compatible "ssd1305fb-i2c", we could make the
>> new one "ssd1305drm-i2c" or better, just "ssd1305-i2c".
> 
> DT describes hardware, not software policy.
> If the hardware is the same, the DT bindings should stay the same.
> 

Yes I know that but the thing is that the current binding don't describe
the hardware correctly. For instance, don't use a backlight DT node as a
property of the panel and have this "fb" suffix in the compatible strings.

Having said that, my opinion is that we should just keep with the existing
bindings and make compatible to that even if isn't completely correct.

Since that will ease adoption of the new DRM driver and allow users to use
it without the need to update their DTBs.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Geert,

On 2/1/22 12:38, Geert Uytterhoeven wrote:

[snip]

>>
>> Since the current binding has a compatible "ssd1305fb-i2c", we could make the
>> new one "ssd1305drm-i2c" or better, just "ssd1305-i2c".
> 
> DT describes hardware, not software policy.
> If the hardware is the same, the DT bindings should stay the same.
> 

Yes I know that but the thing is that the current binding don't describe
the hardware correctly. For instance, don't use a backlight DT node as a
property of the panel and have this "fb" suffix in the compatible strings.

Having said that, my opinion is that we should just keep with the existing
bindings and make compatible to that even if isn't completely correct.

Since that will ease adoption of the new DRM driver and allow users to use
it without the need to update their DTBs.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 1/4] drm: Add I2C connector type

[Noralf Trønnes]

Den 01.02.2022 14.06, skrev Javier Martinez Canillas:
> Hello Noralf,
> 
> On 2/1/22 13:58, Noralf Trønnes wrote:
>>
>>
>> Den 31.01.2022 21.52, skrev Sam Ravnborg:
>>> On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:
>>>> There isn't a connector type for display controllers accesed through I2C,
>>>> most drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.
>>>>
>>>> Add an I2C connector type to match the actual connector.
>>>>
>>>> As Noralf Trønnes mentions in commit fc06bf1d76d6 ("drm: Add SPI connector
>>>> type"), user-space should be able to cope with a connector type that does
>>>> not yet understand.
>>>>
>>
>> It turned out that I wasn't entirely correct here, mpv didn't cope with
>> unknown types. In the PR to add support Emil Velikov wondered if libdrm
>> should handle these connector names:
>> https://github.com/mpv-player/mpv/pull/8989#issuecomment-879187711
>>
> 
> I see, thanks for the information. What should we do then, just use the type
> DRM_MODE_CONNECTOR_Unknown then ?
> 

Not really, I just wanted to point out that it could be that not all
userspace will handle an unknown connector type (I just checked the DE's
at the time). I haven't seen any issues after adding the SPI type so it
can't be that many apps that has problems. Adding to that a tiny
monochrome display is limited in which applications it will encounter I
guess :) It was after adding the USB type that I discovered that mpv
didn't work.

Noralf.

Re: [PATCH 1/4] drm: Add I2C connector type

[Simon Ser]

On Tuesday, February 1st, 2022 at 13:58, Noralf Trønnes <noralf@tronnes.org> wrote:

> It turned out that I wasn't entirely correct here, mpv didn't cope with
> unknown types. In the PR to add support Emil Velikov wondered if libdrm
> should handle these connector names:

Opened this MR to try to make things easier for user-space:
https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/222

Re: [PATCH 1/4] drm: Add I2C connector type

[Simon Ser]

On Tuesday, February 1st, 2022 at 13:58, Noralf Trønnes <noralf@tronnes.org> wrote:

> It turned out that I wasn't entirely correct here, mpv didn't cope with
> unknown types. In the PR to add support Emil Velikov wondered if libdrm
> should handle these connector names:

Opened this MR to try to make things easier for user-space:
https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/222

Re: [PATCH 1/4] drm: Add I2C connector type

[Javier Martinez Canillas]

On 2/1/22 14:20, Noralf Trønnes wrote:
> 
> 
> Den 01.02.2022 14.06, skrev Javier Martinez Canillas:
>> Hello Noralf,
>>
>> On 2/1/22 13:58, Noralf Trønnes wrote:
>>>
>>>
>>> Den 31.01.2022 21.52, skrev Sam Ravnborg:
>>>> On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:
>>>>> There isn't a connector type for display controllers accesed through I2C,
>>>>> most drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.
>>>>>
>>>>> Add an I2C connector type to match the actual connector.
>>>>>
>>>>> As Noralf Trønnes mentions in commit fc06bf1d76d6 ("drm: Add SPI connector
>>>>> type"), user-space should be able to cope with a connector type that does
>>>>> not yet understand.
>>>>>
>>>
>>> It turned out that I wasn't entirely correct here, mpv didn't cope with
>>> unknown types. In the PR to add support Emil Velikov wondered if libdrm
>>> should handle these connector names:
>>> https://github.com/mpv-player/mpv/pull/8989#issuecomment-879187711
>>>
>>
>> I see, thanks for the information. What should we do then, just use the type
>> DRM_MODE_CONNECTOR_Unknown then ?
>>
> 
> Not really, I just wanted to point out that it could be that not all
> userspace will handle an unknown connector type (I just checked the DE's
> at the time). I haven't seen any issues after adding the SPI type so it
> can't be that many apps that has problems. Adding to that a tiny
> monochrome display is limited in which applications it will encounter I
> guess :) It was after adding the USB type that I discovered that mpv
> didn't work.
> 

Anything we do for this rather obscure hardware certainly won't be an
issue for most applications :)

But I wasn't sure if your previous comment meant that you were nacking
$subject. Glad that we can go ahead and describe the correct type then.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Javier,

On Tue, Feb 1, 2022 at 2:09 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> On 2/1/22 12:38, Geert Uytterhoeven wrote:
> >> Since the current binding has a compatible "ssd1305fb-i2c", we could make the
> >> new one "ssd1305drm-i2c" or better, just "ssd1305-i2c".
> >
> > DT describes hardware, not software policy.
> > If the hardware is the same, the DT bindings should stay the same.
> >
>
> Yes I know that but the thing is that the current binding don't describe
> the hardware correctly. For instance, don't use a backlight DT node as a
> property of the panel and have this "fb" suffix in the compatible strings.
>
> Having said that, my opinion is that we should just keep with the existing
> bindings and make compatible to that even if isn't completely correct.
>
> Since that will ease adoption of the new DRM driver and allow users to use
> it without the need to update their DTBs.

To me it looks like the pwms property is not related to the backlight
at all, and only needed for some variants?

And the actual backlight code seems to be about internal contrast
adjustment?

So if the pwms usage is OK, what other reasons are there to break
DT compatibility? IMHO just the "fb" suffix is not a good reason.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Javier,

On Tue, Feb 1, 2022 at 2:09 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> On 2/1/22 12:38, Geert Uytterhoeven wrote:
> >> Since the current binding has a compatible "ssd1305fb-i2c", we could make the
> >> new one "ssd1305drm-i2c" or better, just "ssd1305-i2c".
> >
> > DT describes hardware, not software policy.
> > If the hardware is the same, the DT bindings should stay the same.
> >
>
> Yes I know that but the thing is that the current binding don't describe
> the hardware correctly. For instance, don't use a backlight DT node as a
> property of the panel and have this "fb" suffix in the compatible strings.
>
> Having said that, my opinion is that we should just keep with the existing
> bindings and make compatible to that even if isn't completely correct.
>
> Since that will ease adoption of the new DRM driver and allow users to use
> it without the need to update their DTBs.

To me it looks like the pwms property is not related to the backlight
at all, and only needed for some variants?

And the actual backlight code seems to be about internal contrast
adjustment?

So if the pwms usage is OK, what other reasons are there to break
DT compatibility? IMHO just the "fb" suffix is not a good reason.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/4] drm: Add I2C connector type

[Noralf Trønnes]

Den 01.02.2022 14.38, skrev Simon Ser:
> 
> On Tuesday, February 1st, 2022 at 13:58, Noralf Trønnes <noralf@tronnes.org> wrote:
> 
>> It turned out that I wasn't entirely correct here, mpv didn't cope with
>> unknown types. In the PR to add support Emil Velikov wondered if libdrm
>> should handle these connector names:
> 
> Opened this MR to try to make things easier for user-space:
> https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/222

Thanks Simon!

Re: [PATCH 1/4] drm: Add I2C connector type

[Noralf Trønnes]

Den 01.02.2022 14.38, skrev Simon Ser:
> 
> On Tuesday, February 1st, 2022 at 13:58, Noralf Trønnes <noralf@tronnes.org> wrote:
> 
>> It turned out that I wasn't entirely correct here, mpv didn't cope with
>> unknown types. In the PR to add support Emil Velikov wondered if libdrm
>> should handle these connector names:
> 
> Opened this MR to try to make things easier for user-space:
> https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/222

Thanks Simon!

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Geert,

On 2/1/22 15:14, Geert Uytterhoeven wrote:
> Hi Javier,
> 
> On Tue, Feb 1, 2022 at 2:09 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> On 2/1/22 12:38, Geert Uytterhoeven wrote:
>>>> Since the current binding has a compatible "ssd1305fb-i2c", we could make the
>>>> new one "ssd1305drm-i2c" or better, just "ssd1305-i2c".
>>>
>>> DT describes hardware, not software policy.
>>> If the hardware is the same, the DT bindings should stay the same.
>>>
>>
>> Yes I know that but the thing is that the current binding don't describe
>> the hardware correctly. For instance, don't use a backlight DT node as a
>> property of the panel and have this "fb" suffix in the compatible strings.
>>
>> Having said that, my opinion is that we should just keep with the existing
>> bindings and make compatible to that even if isn't completely correct.
>>
>> Since that will ease adoption of the new DRM driver and allow users to use
>> it without the need to update their DTBs.
> 
> To me it looks like the pwms property is not related to the backlight
> at all, and only needed for some variants?
>

I was reading the datasheets of the ssd1305, ssd1306 and ssd1307. Only the
first one mentions anything about a PWM and says:

  In phase 3, the OLED driver switches to use current source to drive the
  OLED pixels and this is the current drive stage. SSD1305 employs PWM
  (Pulse Width Modulation) method to control the brightness of area color
  A, B, C, D color individually. The longer the waveform in current drive
  stage is, the brighter is the pixel and vice versa.

  After finishing phase 3, the driver IC will go back to phase 1 to display
  the next row image data. This threestep cycle is run continuously to refresh
  image display on OLED panel. 

The way I understand this is that the PWM isn't used for the backlight
but instead to power the IC and allow to display the actual pixels ?

And this matches what Maxime mentioned in this patch:

https://linux-arm-kernel.infradead.narkive.com/5i44FnQ8/patch-1-2-video-ssd1307fb-add-support-for-ssd1306-oled-controller

  The Solomon SSD1306 OLED controller is very similar to the SSD1307,
  except for the fact that the power is given through an external PWM for
  the 1307, and while the 1306 can generate its own power without any PWM. 

> And the actual backlight code seems to be about internal contrast
> adjustment?
> 
> So if the pwms usage is OK, what other reasons are there to break
> DT compatibility? IMHO just the "fb" suffix is not a good reason.
>

Absolutely agreed with you on this. It seems we should just use the existing
binding and make the driver compatible with that. The only value is that the
drm_panel infrastructure could be used, but making it backward compatible is
more worthy IMO.

Best regards, -- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Geert,

On 2/1/22 15:14, Geert Uytterhoeven wrote:
> Hi Javier,
> 
> On Tue, Feb 1, 2022 at 2:09 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> On 2/1/22 12:38, Geert Uytterhoeven wrote:
>>>> Since the current binding has a compatible "ssd1305fb-i2c", we could make the
>>>> new one "ssd1305drm-i2c" or better, just "ssd1305-i2c".
>>>
>>> DT describes hardware, not software policy.
>>> If the hardware is the same, the DT bindings should stay the same.
>>>
>>
>> Yes I know that but the thing is that the current binding don't describe
>> the hardware correctly. For instance, don't use a backlight DT node as a
>> property of the panel and have this "fb" suffix in the compatible strings.
>>
>> Having said that, my opinion is that we should just keep with the existing
>> bindings and make compatible to that even if isn't completely correct.
>>
>> Since that will ease adoption of the new DRM driver and allow users to use
>> it without the need to update their DTBs.
> 
> To me it looks like the pwms property is not related to the backlight
> at all, and only needed for some variants?
>

I was reading the datasheets of the ssd1305, ssd1306 and ssd1307. Only the
first one mentions anything about a PWM and says:

  In phase 3, the OLED driver switches to use current source to drive the
  OLED pixels and this is the current drive stage. SSD1305 employs PWM
  (Pulse Width Modulation) method to control the brightness of area color
  A, B, C, D color individually. The longer the waveform in current drive
  stage is, the brighter is the pixel and vice versa.

  After finishing phase 3, the driver IC will go back to phase 1 to display
  the next row image data. This threestep cycle is run continuously to refresh
  image display on OLED panel. 

The way I understand this is that the PWM isn't used for the backlight
but instead to power the IC and allow to display the actual pixels ?

And this matches what Maxime mentioned in this patch:

https://linux-arm-kernel.infradead.narkive.com/5i44FnQ8/patch-1-2-video-ssd1307fb-add-support-for-ssd1306-oled-controller

  The Solomon SSD1306 OLED controller is very similar to the SSD1307,
  except for the fact that the power is given through an external PWM for
  the 1307, and while the 1306 can generate its own power without any PWM. 

> And the actual backlight code seems to be about internal contrast
> adjustment?
> 
> So if the pwms usage is OK, what other reasons are there to break
> DT compatibility? IMHO just the "fb" suffix is not a good reason.
>

Absolutely agreed with you on this. It seems we should just use the existing
binding and make the driver compatible with that. The only value is that the
drm_panel infrastructure could be used, but making it backward compatible is
more worthy IMO.

Best regards, -- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Sam Ravnborg]

Hi Javier,

On Tue, Feb 01, 2022 at 04:03:30PM +0100, Javier Martinez Canillas wrote:
> Hello Geert,
> 
> On 2/1/22 15:14, Geert Uytterhoeven wrote:
> > Hi Javier,
> > 
> > On Tue, Feb 1, 2022 at 2:09 PM Javier Martinez Canillas
> > <javierm@redhat.com> wrote:
> >> On 2/1/22 12:38, Geert Uytterhoeven wrote:
> >>>> Since the current binding has a compatible "ssd1305fb-i2c", we could make the
> >>>> new one "ssd1305drm-i2c" or better, just "ssd1305-i2c".
> >>>
> >>> DT describes hardware, not software policy.
> >>> If the hardware is the same, the DT bindings should stay the same.
Only if the bindings describe the HW in a correct way that is.

> >>>
> >>
> >> Yes I know that but the thing is that the current binding don't describe
> >> the hardware correctly. For instance, don't use a backlight DT node as a
> >> property of the panel and have this "fb" suffix in the compatible strings.
> >>
> >> Having said that, my opinion is that we should just keep with the existing
> >> bindings and make compatible to that even if isn't completely correct.
> >>
> >> Since that will ease adoption of the new DRM driver and allow users to use
> >> it without the need to update their DTBs.
> > 
> > To me it looks like the pwms property is not related to the backlight
> > at all, and only needed for some variants?
> >
> 
> I was reading the datasheets of the ssd1305, ssd1306 and ssd1307. Only the
> first one mentions anything about a PWM and says:
> 
>   In phase 3, the OLED driver switches to use current source to drive the
>   OLED pixels and this is the current drive stage. SSD1305 employs PWM
>   (Pulse Width Modulation) method to control the brightness of area color
>   A, B, C, D color individually. The longer the waveform in current drive
>   stage is, the brighter is the pixel and vice versa.
> 
>   After finishing phase 3, the driver IC will go back to phase 1 to display
>   the next row image data. This threestep cycle is run continuously to refresh
>   image display on OLED panel. 
> 
> The way I understand this is that the PWM isn't used for the backlight
> but instead to power the IC and allow to display the actual pixels ?
> 
> And this matches what Maxime mentioned in this patch:
> 
> https://linux-arm-kernel.infradead.narkive.com/5i44FnQ8/patch-1-2-video-ssd1307fb-add-support-for-ssd1306-oled-controller
> 
>   The Solomon SSD1306 OLED controller is very similar to the SSD1307,
>   except for the fact that the power is given through an external PWM for
>   the 1307, and while the 1306 can generate its own power without any PWM.

I took a look at the datasheets - and all ssd1305, ssd1306 and ssd1307
are the same. They have timing constrains on the Vcc.
The random schematic I found on the net showed me that a PWM was used to
control the Vcc voltage - which again is used to control the brightness.

All the above has nothing to do with backlight - I had this mixed up in
my head.

So my current understanding:
- solomon,ssd1307fb.yaml should NOT include a backlight node - because
  the backlight is something included in the ssd130x device and not
  something separate.
- 1305, 1306, and 1307 (I did not check 1309) all requires a Vcc
  supply that shall be turned on/off according to the datasheet.
  This implies that we need a regulaator for Vcc - and the regulator
  could be a pwm based regulator or something else - the HW do not care.
- But I can see that several design connect Vcc to a fixed voltage,
  so I am not too sure about this part.

I think the correct binding would have

    ssd1307 => regulator => pwm

So the ssd1307 binding references a regulator, and the regulator
may use an pwm or may use something else.

The current binding references a vbat supply - but the datasheet do not
mention any vbat. It is most likely modelling the Vdd supply.

Right now my take is to go the simple route:
- Keep the binding as is and just use the pwm as already implemented
- Likewise keep the backlight as is

Last I recommend to drop the fbdev variant - if the drm driver has any
regressions we can fix them. And I do not see any other way to move
users over. Unless their setup breaks then they do not change.

> 
> > And the actual backlight code seems to be about internal contrast
> > adjustment?
> > 
> > So if the pwms usage is OK, what other reasons are there to break
> > DT compatibility? IMHO just the "fb" suffix is not a good reason.
> >
> 
> Absolutely agreed with you on this. It seems we should just use the existing
> binding and make the driver compatible with that. The only value is that the
> drm_panel infrastructure could be used, but making it backward compatible is
> more worthy IMO.
Using drm_panel here would IMO just complicate things - it is not that
we will see many different panels (I think).

	Sam

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Sam Ravnborg]

Hi Javier,

On Tue, Feb 01, 2022 at 04:03:30PM +0100, Javier Martinez Canillas wrote:
> Hello Geert,
> 
> On 2/1/22 15:14, Geert Uytterhoeven wrote:
> > Hi Javier,
> > 
> > On Tue, Feb 1, 2022 at 2:09 PM Javier Martinez Canillas
> > <javierm@redhat.com> wrote:
> >> On 2/1/22 12:38, Geert Uytterhoeven wrote:
> >>>> Since the current binding has a compatible "ssd1305fb-i2c", we could make the
> >>>> new one "ssd1305drm-i2c" or better, just "ssd1305-i2c".
> >>>
> >>> DT describes hardware, not software policy.
> >>> If the hardware is the same, the DT bindings should stay the same.
Only if the bindings describe the HW in a correct way that is.

> >>>
> >>
> >> Yes I know that but the thing is that the current binding don't describe
> >> the hardware correctly. For instance, don't use a backlight DT node as a
> >> property of the panel and have this "fb" suffix in the compatible strings.
> >>
> >> Having said that, my opinion is that we should just keep with the existing
> >> bindings and make compatible to that even if isn't completely correct.
> >>
> >> Since that will ease adoption of the new DRM driver and allow users to use
> >> it without the need to update their DTBs.
> > 
> > To me it looks like the pwms property is not related to the backlight
> > at all, and only needed for some variants?
> >
> 
> I was reading the datasheets of the ssd1305, ssd1306 and ssd1307. Only the
> first one mentions anything about a PWM and says:
> 
>   In phase 3, the OLED driver switches to use current source to drive the
>   OLED pixels and this is the current drive stage. SSD1305 employs PWM
>   (Pulse Width Modulation) method to control the brightness of area color
>   A, B, C, D color individually. The longer the waveform in current drive
>   stage is, the brighter is the pixel and vice versa.
> 
>   After finishing phase 3, the driver IC will go back to phase 1 to display
>   the next row image data. This threestep cycle is run continuously to refresh
>   image display on OLED panel. 
> 
> The way I understand this is that the PWM isn't used for the backlight
> but instead to power the IC and allow to display the actual pixels ?
> 
> And this matches what Maxime mentioned in this patch:
> 
> https://linux-arm-kernel.infradead.narkive.com/5i44FnQ8/patch-1-2-video-ssd1307fb-add-support-for-ssd1306-oled-controller
> 
>   The Solomon SSD1306 OLED controller is very similar to the SSD1307,
>   except for the fact that the power is given through an external PWM for
>   the 1307, and while the 1306 can generate its own power without any PWM.

I took a look at the datasheets - and all ssd1305, ssd1306 and ssd1307
are the same. They have timing constrains on the Vcc.
The random schematic I found on the net showed me that a PWM was used to
control the Vcc voltage - which again is used to control the brightness.

All the above has nothing to do with backlight - I had this mixed up in
my head.

So my current understanding:
- solomon,ssd1307fb.yaml should NOT include a backlight node - because
  the backlight is something included in the ssd130x device and not
  something separate.
- 1305, 1306, and 1307 (I did not check 1309) all requires a Vcc
  supply that shall be turned on/off according to the datasheet.
  This implies that we need a regulaator for Vcc - and the regulator
  could be a pwm based regulator or something else - the HW do not care.
- But I can see that several design connect Vcc to a fixed voltage,
  so I am not too sure about this part.

I think the correct binding would have

    ssd1307 => regulator => pwm

So the ssd1307 binding references a regulator, and the regulator
may use an pwm or may use something else.

The current binding references a vbat supply - but the datasheet do not
mention any vbat. It is most likely modelling the Vdd supply.

Right now my take is to go the simple route:
- Keep the binding as is and just use the pwm as already implemented
- Likewise keep the backlight as is

Last I recommend to drop the fbdev variant - if the drm driver has any
regressions we can fix them. And I do not see any other way to move
users over. Unless their setup breaks then they do not change.

> 
> > And the actual backlight code seems to be about internal contrast
> > adjustment?
> > 
> > So if the pwms usage is OK, what other reasons are there to break
> > DT compatibility? IMHO just the "fb" suffix is not a good reason.
> >
> 
> Absolutely agreed with you on this. It seems we should just use the existing
> binding and make the driver compatible with that. The only value is that the
> drm_panel infrastructure could be used, but making it backward compatible is
> more worthy IMO.
Using drm_panel here would IMO just complicate things - it is not that
we will see many different panels (I think).

	Sam

Re: [PATCH 1/4] drm: Add I2C connector type

[Sam Ravnborg]

Hi Noralf,

On Tue, Feb 01, 2022 at 01:58:16PM +0100, Noralf Trønnes wrote:
> 
> 
> Den 31.01.2022 21.52, skrev Sam Ravnborg:
> > On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:
> >> There isn't a connector type for display controllers accesed through I2C,
> >> most drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.
> >>
> >> Add an I2C connector type to match the actual connector.
> >>
> >> As Noralf Trønnes mentions in commit fc06bf1d76d6 ("drm: Add SPI connector
> >> type"), user-space should be able to cope with a connector type that does
> >> not yet understand.
> >>
> 
> It turned out that I wasn't entirely correct here, mpv didn't cope with
> unknown types. In the PR to add support Emil Velikov wondered if libdrm
> should handle these connector names:
> https://github.com/mpv-player/mpv/pull/8989#issuecomment-879187711
> 
> >> Tested with `modetest -M ssd1307 -c` and shows the connector as unknown-1.
> > I had expected unknown-21??
> > 
> >>
> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> > Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> Sam, didn't you and Laurent discuss adding DRM_MODE_CONNECTOR_PANEL for
> such a use case?

You have a splendid memory - yes we did. But no conclusions:
https://lore.kernel.org/dri-devel/?q=DRM_MODE_CONNECTOR_PANEL

As I wrote in another part of this thread(s) - typing the patch is easy.
But I do not understand the userspace implications so I need someone
else to say go.

	Sam

Re: [PATCH 1/4] drm: Add I2C connector type

[Simon Ser]

On Tuesday, February 1st, 2022 at 21:57, Sam Ravnborg <sam@ravnborg.org> wrote:

> As I wrote in another part of this thread(s) - typing the patch is easy.
> But I do not understand the userspace implications so I need someone
> else to say go.

User-space shouldn't really use the connector for anything except making it
easier for the user to understand where to plug the display cable. I think a
generic "panel" connector type makes sense.

Re: [PATCH 1/4] drm: Add I2C connector type

[Simon Ser]

On Tuesday, February 1st, 2022 at 21:57, Sam Ravnborg <sam@ravnborg.org> wrote:

> As I wrote in another part of this thread(s) - typing the patch is easy.
> But I do not understand the userspace implications so I need someone
> else to say go.

User-space shouldn't really use the connector for anything except making it
easier for the user to understand where to plug the display cable. I think a
generic "panel" connector type makes sense.

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Sam,

On 2/1/22 21:40, Sam Ravnborg wrote:

[snip]

> 
> I took a look at the datasheets - and all ssd1305, ssd1306 and ssd1307
> are the same. They have timing constrains on the Vcc.
> The random schematic I found on the net showed me that a PWM was used to
> control the Vcc voltage - which again is used to control the brightness.
> 
> All the above has nothing to do with backlight - I had this mixed up in
> my head.
>

Yes, same here. I was leaning towards fixing the DT binding but then due
Geert comment and after reading the datasheets for ssd130{5,6,7} like you
I had the same understanding.

Glad that you agree.

[snip] 

> 
> Last I recommend to drop the fbdev variant - if the drm driver has any
> regressions we can fix them. And I do not see any other way to move
> users over. Unless their setup breaks then they do not change.
>

As I mentioned in this thread I wouldn't propose to drop the fbdev variant.
I prefer to use the carrot and not the stick. Peter Robinson suggested to
make the driver mutually exclusive and add !FB_SSD1307 in the config symbol.

I think that makes sense and will do it in v2.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Sam,

On 2/1/22 21:40, Sam Ravnborg wrote:

[snip]

> 
> I took a look at the datasheets - and all ssd1305, ssd1306 and ssd1307
> are the same. They have timing constrains on the Vcc.
> The random schematic I found on the net showed me that a PWM was used to
> control the Vcc voltage - which again is used to control the brightness.
> 
> All the above has nothing to do with backlight - I had this mixed up in
> my head.
>

Yes, same here. I was leaning towards fixing the DT binding but then due
Geert comment and after reading the datasheets for ssd130{5,6,7} like you
I had the same understanding.

Glad that you agree.

[snip] 

> 
> Last I recommend to drop the fbdev variant - if the drm driver has any
> regressions we can fix them. And I do not see any other way to move
> users over. Unless their setup breaks then they do not change.
>

As I mentioned in this thread I wouldn't propose to drop the fbdev variant.
I prefer to use the carrot and not the stick. Peter Robinson suggested to
make the driver mutually exclusive and add !FB_SSD1307 in the config symbol.

I think that makes sense and will do it in v2.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 1/4] drm: Add I2C connector type

[Javier Martinez Canillas]

On 2/1/22 23:29, Simon Ser wrote:
> On Tuesday, February 1st, 2022 at 21:57, Sam Ravnborg <sam@ravnborg.org> wrote:
> 
>> As I wrote in another part of this thread(s) - typing the patch is easy.
>> But I do not understand the userspace implications so I need someone
>> else to say go.
> 
> User-space shouldn't really use the connector for anything except making it
> easier for the user to understand where to plug the display cable. I think a
> generic "panel" connector type makes sense.
> 

I'll drop this patch from the series. I didn't have all the context and thought
that was an opportunity to add an I2C type, since there was a SPI type already.

But seems to be more controversial than I expected and shouldn't really matter
for a tiny driver like this. I will just go ahead and use the Unknown type.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 1/4] drm: Add I2C connector type

[Javier Martinez Canillas]

On 2/1/22 23:29, Simon Ser wrote:
> On Tuesday, February 1st, 2022 at 21:57, Sam Ravnborg <sam@ravnborg.org> wrote:
> 
>> As I wrote in another part of this thread(s) - typing the patch is easy.
>> But I do not understand the userspace implications so I need someone
>> else to say go.
> 
> User-space shouldn't really use the connector for anything except making it
> easier for the user to understand where to plug the display cable. I think a
> generic "panel" connector type makes sense.
> 

I'll drop this patch from the series. I didn't have all the context and thought
that was an opportunity to add an I2C type, since there was a SPI type already.

But seems to be more controversial than I expected and shouldn't really matter
for a tiny driver like this. I will just go ahead and use the Unknown type.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 1/4] drm: Add I2C connector type

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 651 bytes --]

Hi Noralf,

since you're here, I'll just hijack the discussion to ask something only 
semi-related.

IIRC the gud driver doesn't update the display immediately during atomic 
commits. Instead, it instructs a helper thread to do the update. What's 
the rational behind this design? Is that something we should adopt for 
other drivers that operate over slow buses (USB, I2C, etc)? Would this 
be relevant for the ssd1307 driver?

Best regards
Thomas

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/4] drm: Add I2C connector type

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 651 bytes --]

Hi Noralf,

since you're here, I'll just hijack the discussion to ask something only 
semi-related.

IIRC the gud driver doesn't update the display immediately during atomic 
commits. Instead, it instructs a helper thread to do the update. What's 
the rational behind this design? Is that something we should adopt for 
other drivers that operate over slow buses (USB, I2C, etc)? Would this 
be relevant for the ssd1307 driver?

Best regards
Thomas

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 4782 bytes --]

On Tue, 1 Feb 2022 12:07:07 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Pekka,
> 
> On Tue, Feb 1, 2022 at 11:42 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Tue, 1 Feb 2022 10:49:03 +0100
> > Javier Martinez Canillas <javierm@redhat.com> wrote:  
> > > On 2/1/22 09:38, Daniel Vetter wrote:  
> > > > On Tue, Feb 1, 2022 at 9:34 AM Simon Ser <contact@emersion.fr> wrote:  
> > > >> On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > >>> What's the story with the Rn formats?
> > > >>>
> > > >>> The comments say "n bpp Red", while this is a monochrome (even
> > > >>> inverted) display?  
> > > >>
> > > >> I don't think the color matters that much. "Red" was picked just because it was
> > > >> an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?  
> > > >
> > > > The red comes from gl, where with shaders it really doesn't matter
> > > > what meaning you attach to channels, but really just how many you
> > > > have. So 2-channel formats are called RxGx, 3-channel RxGxBx,
> > > > 4-channel RxGxBxAx and single-channel Rx. And we use drm_fourcc for
> > > > interop in general, hence why these exist.
> > > >
> > > > We should probably make a comment that this really isn't a red channel
> > > > when used for display it's a greyscale/intensity format. Aside from
> > > > that documentation gap I think reusing Rx formats for
> > > > greyscale/intensity for display makes perfect sense.
> > > > -Daniel  
> > >
> > > To sump up the conversation in the #dri-devel channel, these drivers
> > > should support the following formats:
> > >
> > > 1) Dx (Daniel suggested that for darkness, but inverted mono)  
> >
> > Did you consider format C1 instead?  
> 
> That would be a 2-color display, which is not necessarily black
> and white. Cfr. Amiga or Atari bit planes with bpp=1.
> That's why fbdev has separate visuals for monochrome.

Yes, that is exactly what I was aiming at: to draft a plan for panels
that have a fixed and arbitrary palette. From the discussions I
understood that the panel in question here requires somehow reversed
colors ("inverted mono"), which didn't really sound to be like "normal
monochrome".

> > I have no idea how this would map to fbdev API though.  
> 
>     #define FB_VISUAL_MONO01                0       /* Monochr.
> 1=Black 0=White */
>     #define FB_VISUAL_MONO10                1       /* Monochr.
> 1=White 0=Black */
>     #define FB_VISUAL_TRUECOLOR             2       /* True color   */
> 
> The above is RGB (or grayscale, see below).
> 
>     #define FB_VISUAL_PSEUDOCOLOR           3       /* Pseudo color
> (like atari) */
> 
> Palette
> 
>     #define FB_VISUAL_DIRECTCOLOR           4       /* Direct color */
> 
> Usually used as RGB with gamma correction, but the actual hardware
> is more flexible.
> 
>     #define FB_VISUAL_STATIC_PSEUDOCOLOR    5       /* Pseudo color readonly */
> 
> Fixed palette
> 
> And:
> 
>     struct fb_var_screeninfo {
>             ...
>             __u32 grayscale;                /* 0 = color, 1 = grayscale,    */

DRM has pixel formats, but no visuals so far. Maybe it needs to grow
the concept of visuals in some form? However, care should be taken to
not clash with existing colorimetry features. I would hope that the
colorimetry feature set could be extended to cover the above as well.
Well, only if there would be any users for it.

My silly attempt with Cx formats (e.g. DRM_FORMAT_C8) was a stab in that
direction, but maybe not flexible enough for the above.

If on the other hand the panel is "grayscale" but with an arbitrary
color (white, green, orange or other on black), the IRC consensus seems
to be that one should use Rx formats (e.g. DRM_FORMAT_R8) for it,
regardless of the actual color. That would convey that the pixel value
has a monotonic (increasing) mapping to brightness, unlike with
paletted formats. I agree with this, but wonder how reversed brightness
should be dealt with - or just have the driver invert the pixel values
before sending them to display?

Cx formats with a read-only palette could be used to represent
"grayscale" and "reversed grayscale" too, but people seem to think that
is too complicated to analyse and use for KMS userspace.

Other #dri-devel IRC mumblings were about maybe adding a DRM pixel
format for grayscale or intensity or luminance so that one would not
need to use "red" color channel for something that doesn't look red.
That is, do not use Cx formats because those produce completely
arbitrary colors, and do not use Rx formats because the display is not
redscale. Personally I'd be fine with Rx formats.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 4782 bytes --]

On Tue, 1 Feb 2022 12:07:07 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Pekka,
> 
> On Tue, Feb 1, 2022 at 11:42 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Tue, 1 Feb 2022 10:49:03 +0100
> > Javier Martinez Canillas <javierm@redhat.com> wrote:  
> > > On 2/1/22 09:38, Daniel Vetter wrote:  
> > > > On Tue, Feb 1, 2022 at 9:34 AM Simon Ser <contact@emersion.fr> wrote:  
> > > >> On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > >>> What's the story with the Rn formats?
> > > >>>
> > > >>> The comments say "n bpp Red", while this is a monochrome (even
> > > >>> inverted) display?  
> > > >>
> > > >> I don't think the color matters that much. "Red" was picked just because it was
> > > >> an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?  
> > > >
> > > > The red comes from gl, where with shaders it really doesn't matter
> > > > what meaning you attach to channels, but really just how many you
> > > > have. So 2-channel formats are called RxGx, 3-channel RxGxBx,
> > > > 4-channel RxGxBxAx and single-channel Rx. And we use drm_fourcc for
> > > > interop in general, hence why these exist.
> > > >
> > > > We should probably make a comment that this really isn't a red channel
> > > > when used for display it's a greyscale/intensity format. Aside from
> > > > that documentation gap I think reusing Rx formats for
> > > > greyscale/intensity for display makes perfect sense.
> > > > -Daniel  
> > >
> > > To sump up the conversation in the #dri-devel channel, these drivers
> > > should support the following formats:
> > >
> > > 1) Dx (Daniel suggested that for darkness, but inverted mono)  
> >
> > Did you consider format C1 instead?  
> 
> That would be a 2-color display, which is not necessarily black
> and white. Cfr. Amiga or Atari bit planes with bpp=1.
> That's why fbdev has separate visuals for monochrome.

Yes, that is exactly what I was aiming at: to draft a plan for panels
that have a fixed and arbitrary palette. From the discussions I
understood that the panel in question here requires somehow reversed
colors ("inverted mono"), which didn't really sound to be like "normal
monochrome".

> > I have no idea how this would map to fbdev API though.  
> 
>     #define FB_VISUAL_MONO01                0       /* Monochr.
> 1=Black 0=White */
>     #define FB_VISUAL_MONO10                1       /* Monochr.
> 1=White 0=Black */
>     #define FB_VISUAL_TRUECOLOR             2       /* True color   */
> 
> The above is RGB (or grayscale, see below).
> 
>     #define FB_VISUAL_PSEUDOCOLOR           3       /* Pseudo color
> (like atari) */
> 
> Palette
> 
>     #define FB_VISUAL_DIRECTCOLOR           4       /* Direct color */
> 
> Usually used as RGB with gamma correction, but the actual hardware
> is more flexible.
> 
>     #define FB_VISUAL_STATIC_PSEUDOCOLOR    5       /* Pseudo color readonly */
> 
> Fixed palette
> 
> And:
> 
>     struct fb_var_screeninfo {
>             ...
>             __u32 grayscale;                /* 0 = color, 1 = grayscale,    */

DRM has pixel formats, but no visuals so far. Maybe it needs to grow
the concept of visuals in some form? However, care should be taken to
not clash with existing colorimetry features. I would hope that the
colorimetry feature set could be extended to cover the above as well.
Well, only if there would be any users for it.

My silly attempt with Cx formats (e.g. DRM_FORMAT_C8) was a stab in that
direction, but maybe not flexible enough for the above.

If on the other hand the panel is "grayscale" but with an arbitrary
color (white, green, orange or other on black), the IRC consensus seems
to be that one should use Rx formats (e.g. DRM_FORMAT_R8) for it,
regardless of the actual color. That would convey that the pixel value
has a monotonic (increasing) mapping to brightness, unlike with
paletted formats. I agree with this, but wonder how reversed brightness
should be dealt with - or just have the driver invert the pixel values
before sending them to display?

Cx formats with a read-only palette could be used to represent
"grayscale" and "reversed grayscale" too, but people seem to think that
is too complicated to analyse and use for KMS userspace.

Other #dri-devel IRC mumblings were about maybe adding a DRM pixel
format for grayscale or intensity or luminance so that one would not
need to use "red" color channel for something that doesn't look red.
That is, do not use Cx formats because those produce completely
arbitrary colors, and do not use Rx formats because the display is not
redscale. Personally I'd be fine with Rx formats.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] drm: Add I2C connector type

[Noralf Trønnes]

Den 02.02.2022 10.14, skrev Thomas Zimmermann:
> Hi Noralf,
> 
> since you're here, I'll just hijack the discussion to ask something only
> semi-related.
> 
> IIRC the gud driver doesn't update the display immediately during atomic
> commits. Instead, it instructs a helper thread to do the update. What's
> the rational behind this design? Is that something we should adopt for
> other drivers that operate over slow buses (USB, I2C, etc)? Would this
> be relevant for the ssd1307 driver?
> 

Async flushing is only necessary on multi display setups where there's
only one rendering loop for all the displays. I saw what tiny/gm12u320.c
did and Hans gave me the rationale. The SPI drivers run flushing inline.
Info on the gud wiki:
https://github.com/notro/gud/wiki/Linux-Host-Driver#asynchronous-flushing

Noralf.

Re: [PATCH 1/4] drm: Add I2C connector type

[Noralf Trønnes]

Den 02.02.2022 10.14, skrev Thomas Zimmermann:
> Hi Noralf,
> 
> since you're here, I'll just hijack the discussion to ask something only
> semi-related.
> 
> IIRC the gud driver doesn't update the display immediately during atomic
> commits. Instead, it instructs a helper thread to do the update. What's
> the rational behind this design? Is that something we should adopt for
> other drivers that operate over slow buses (USB, I2C, etc)? Would this
> be relevant for the ssd1307 driver?
> 

Async flushing is only necessary on multi display setups where there's
only one rendering loop for all the displays. I saw what tiny/gm12u320.c
did and Hans gave me the rationale. The SPI drivers run flushing inline.
Info on the gud wiki:
https://github.com/notro/gud/wiki/Linux-Host-Driver#asynchronous-flushing

Noralf.

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Pekka,

On Wed, Feb 2, 2022 at 10:20 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Tue, 1 Feb 2022 12:07:07 +0100
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, Feb 1, 2022 at 11:42 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > > On Tue, 1 Feb 2022 10:49:03 +0100
> > > Javier Martinez Canillas <javierm@redhat.com> wrote:
> > > > On 2/1/22 09:38, Daniel Vetter wrote:
> > > > > On Tue, Feb 1, 2022 at 9:34 AM Simon Ser <contact@emersion.fr> wrote:
> > > > >> On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > >>> What's the story with the Rn formats?
> > > > >>>
> > > > >>> The comments say "n bpp Red", while this is a monochrome (even
> > > > >>> inverted) display?
> > > > >>
> > > > >> I don't think the color matters that much. "Red" was picked just because it was
> > > > >> an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?
> > > > >
> > > > > The red comes from gl, where with shaders it really doesn't matter
> > > > > what meaning you attach to channels, but really just how many you
> > > > > have. So 2-channel formats are called RxGx, 3-channel RxGxBx,
> > > > > 4-channel RxGxBxAx and single-channel Rx. And we use drm_fourcc for
> > > > > interop in general, hence why these exist.
> > > > >
> > > > > We should probably make a comment that this really isn't a red channel
> > > > > when used for display it's a greyscale/intensity format. Aside from
> > > > > that documentation gap I think reusing Rx formats for
> > > > > greyscale/intensity for display makes perfect sense.
> > > > > -Daniel
> > > >
> > > > To sump up the conversation in the #dri-devel channel, these drivers
> > > > should support the following formats:
> > > >
> > > > 1) Dx (Daniel suggested that for darkness, but inverted mono)
> > >
> > > Did you consider format C1 instead?
> >
> > That would be a 2-color display, which is not necessarily black
> > and white. Cfr. Amiga or Atari bit planes with bpp=1.
> > That's why fbdev has separate visuals for monochrome.
>
> Yes, that is exactly what I was aiming at: to draft a plan for panels
> that have a fixed and arbitrary palette. From the discussions I
> understood that the panel in question here requires somehow reversed
> colors ("inverted mono"), which didn't really sound to be like "normal
> monochrome".
>
> > > I have no idea how this would map to fbdev API though.
> >
> >     #define FB_VISUAL_MONO01                0       /* Monochr.
> > 1=Black 0=White */
> >     #define FB_VISUAL_MONO10                1       /* Monochr.
> > 1=White 0=Black */
> >     #define FB_VISUAL_TRUECOLOR             2       /* True color   */
> >
> > The above is RGB (or grayscale, see below).
> >
> >     #define FB_VISUAL_PSEUDOCOLOR           3       /* Pseudo color
> > (like atari) */
> >
> > Palette
> >
> >     #define FB_VISUAL_DIRECTCOLOR           4       /* Direct color */
> >
> > Usually used as RGB with gamma correction, but the actual hardware
> > is more flexible.
> >
> >     #define FB_VISUAL_STATIC_PSEUDOCOLOR    5       /* Pseudo color readonly */
> >
> > Fixed palette
> >
> > And:
> >
> >     struct fb_var_screeninfo {
> >             ...
> >             __u32 grayscale;                /* 0 = color, 1 = grayscale,    */
>
> DRM has pixel formats, but no visuals so far. Maybe it needs to grow
> the concept of visuals in some form? However, care should be taken to
> not clash with existing colorimetry features. I would hope that the
> colorimetry feature set could be extended to cover the above as well.
> Well, only if there would be any users for it.

Fbdev has separate (orthogonal) settings for
  1. Frame buffer layout (FB_TYPE_*),
  2. Pixel format (depth and fb_bitfields),
  3. Visual.
DRM combines all of the above in a fourcc value.

Nowadays most frame buffer layouts are packed, so using a shadow
frame buffer to support other layouts is very helpful, as it means
applications no longer have to care about legacy frame buffer layouts.

> My silly attempt with Cx formats (e.g. DRM_FORMAT_C8) was a stab in that
> direction, but maybe not flexible enough for the above.
>
> If on the other hand the panel is "grayscale" but with an arbitrary
> color (white, green, orange or other on black), the IRC consensus seems
> to be that one should use Rx formats (e.g. DRM_FORMAT_R8) for it,
> regardless of the actual color. That would convey that the pixel value
> has a monotonic (increasing) mapping to brightness, unlike with
> paletted formats. I agree with this, but wonder how reversed brightness

Agreed, the only thing that matters is a monotonic mapping, and
whether it's increasing or decreasing.

> should be dealt with - or just have the driver invert the pixel values
> before sending them to display?

That's an option. If the data has to be copied anyway, inversion is
a cheap operation. Else I think you need new fourcc types.

> Cx formats with a read-only palette could be used to represent
> "grayscale" and "reversed grayscale" too, but people seem to think that
> is too complicated to analyse and use for KMS userspace.

Yeah, it's complicated, but rather rare. Most desktop hardware
(even from the nineties ;-) does support a programmable palette.
Exceptions are CGA, the C64 (no Linux support yet ;-), and eInk
displays that support e.g. white, black, and red.

If you do want to support it, perhaps introduce Fx (F = fixed)
fourcc types?

> Other #dri-devel IRC mumblings were about maybe adding a DRM pixel
> format for grayscale or intensity or luminance so that one would not
> need to use "red" color channel for something that doesn't look red.
> That is, do not use Cx formats because those produce completely
> arbitrary colors, and do not use Rx formats because the display is not
> redscale. Personally I'd be fine with Rx formats.

Fine, as said above, monotonic mapping is what matters.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Geert Uytterhoeven]

Hi Pekka,

On Wed, Feb 2, 2022 at 10:20 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Tue, 1 Feb 2022 12:07:07 +0100
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, Feb 1, 2022 at 11:42 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > > On Tue, 1 Feb 2022 10:49:03 +0100
> > > Javier Martinez Canillas <javierm@redhat.com> wrote:
> > > > On 2/1/22 09:38, Daniel Vetter wrote:
> > > > > On Tue, Feb 1, 2022 at 9:34 AM Simon Ser <contact@emersion.fr> wrote:
> > > > >> On Tuesday, February 1st, 2022 at 09:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > >>> What's the story with the Rn formats?
> > > > >>>
> > > > >>> The comments say "n bpp Red", while this is a monochrome (even
> > > > >>> inverted) display?
> > > > >>
> > > > >> I don't think the color matters that much. "Red" was picked just because it was
> > > > >> an arbitrary color, to make the difference with e.g. C8. Or am I mistaken?
> > > > >
> > > > > The red comes from gl, where with shaders it really doesn't matter
> > > > > what meaning you attach to channels, but really just how many you
> > > > > have. So 2-channel formats are called RxGx, 3-channel RxGxBx,
> > > > > 4-channel RxGxBxAx and single-channel Rx. And we use drm_fourcc for
> > > > > interop in general, hence why these exist.
> > > > >
> > > > > We should probably make a comment that this really isn't a red channel
> > > > > when used for display it's a greyscale/intensity format. Aside from
> > > > > that documentation gap I think reusing Rx formats for
> > > > > greyscale/intensity for display makes perfect sense.
> > > > > -Daniel
> > > >
> > > > To sump up the conversation in the #dri-devel channel, these drivers
> > > > should support the following formats:
> > > >
> > > > 1) Dx (Daniel suggested that for darkness, but inverted mono)
> > >
> > > Did you consider format C1 instead?
> >
> > That would be a 2-color display, which is not necessarily black
> > and white. Cfr. Amiga or Atari bit planes with bpp=1.
> > That's why fbdev has separate visuals for monochrome.
>
> Yes, that is exactly what I was aiming at: to draft a plan for panels
> that have a fixed and arbitrary palette. From the discussions I
> understood that the panel in question here requires somehow reversed
> colors ("inverted mono"), which didn't really sound to be like "normal
> monochrome".
>
> > > I have no idea how this would map to fbdev API though.
> >
> >     #define FB_VISUAL_MONO01                0       /* Monochr.
> > 1=Black 0=White */
> >     #define FB_VISUAL_MONO10                1       /* Monochr.
> > 1=White 0=Black */
> >     #define FB_VISUAL_TRUECOLOR             2       /* True color   */
> >
> > The above is RGB (or grayscale, see below).
> >
> >     #define FB_VISUAL_PSEUDOCOLOR           3       /* Pseudo color
> > (like atari) */
> >
> > Palette
> >
> >     #define FB_VISUAL_DIRECTCOLOR           4       /* Direct color */
> >
> > Usually used as RGB with gamma correction, but the actual hardware
> > is more flexible.
> >
> >     #define FB_VISUAL_STATIC_PSEUDOCOLOR    5       /* Pseudo color readonly */
> >
> > Fixed palette
> >
> > And:
> >
> >     struct fb_var_screeninfo {
> >             ...
> >             __u32 grayscale;                /* 0 = color, 1 = grayscale,    */
>
> DRM has pixel formats, but no visuals so far. Maybe it needs to grow
> the concept of visuals in some form? However, care should be taken to
> not clash with existing colorimetry features. I would hope that the
> colorimetry feature set could be extended to cover the above as well.
> Well, only if there would be any users for it.

Fbdev has separate (orthogonal) settings for
  1. Frame buffer layout (FB_TYPE_*),
  2. Pixel format (depth and fb_bitfields),
  3. Visual.
DRM combines all of the above in a fourcc value.

Nowadays most frame buffer layouts are packed, so using a shadow
frame buffer to support other layouts is very helpful, as it means
applications no longer have to care about legacy frame buffer layouts.

> My silly attempt with Cx formats (e.g. DRM_FORMAT_C8) was a stab in that
> direction, but maybe not flexible enough for the above.
>
> If on the other hand the panel is "grayscale" but with an arbitrary
> color (white, green, orange or other on black), the IRC consensus seems
> to be that one should use Rx formats (e.g. DRM_FORMAT_R8) for it,
> regardless of the actual color. That would convey that the pixel value
> has a monotonic (increasing) mapping to brightness, unlike with
> paletted formats. I agree with this, but wonder how reversed brightness

Agreed, the only thing that matters is a monotonic mapping, and
whether it's increasing or decreasing.

> should be dealt with - or just have the driver invert the pixel values
> before sending them to display?

That's an option. If the data has to be copied anyway, inversion is
a cheap operation. Else I think you need new fourcc types.

> Cx formats with a read-only palette could be used to represent
> "grayscale" and "reversed grayscale" too, but people seem to think that
> is too complicated to analyse and use for KMS userspace.

Yeah, it's complicated, but rather rare. Most desktop hardware
(even from the nineties ;-) does support a programmable palette.
Exceptions are CGA, the C64 (no Linux support yet ;-), and eInk
displays that support e.g. white, black, and red.

If you do want to support it, perhaps introduce Fx (F = fixed)
fourcc types?

> Other #dri-devel IRC mumblings were about maybe adding a DRM pixel
> format for grayscale or intensity or luminance so that one would not
> need to use "red" color channel for something that doesn't look red.
> That is, do not use Cx formats because those produce completely
> arbitrary colors, and do not use Rx formats because the display is not
> redscale. Personally I'd be fine with Rx formats.

Fine, as said above, monotonic mapping is what matters.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Andy Shevchenko]

On Wed, Feb 02, 2022 at 09:38:51AM +0100, Javier Martinez Canillas wrote:
> On 2/1/22 21:40, Sam Ravnborg wrote:

...

> Peter Robinson suggested to
> make the driver mutually exclusive and add !FB_SSD1307 in the config symbol.

And how will distros choose "the right" option in this case?
What to do when I wan to see a regression and I want to change drivers w/o
recompilation?

NAK from me to that proposal.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Andy Shevchenko]

On Wed, Feb 02, 2022 at 09:38:51AM +0100, Javier Martinez Canillas wrote:
> On 2/1/22 21:40, Sam Ravnborg wrote:

...

> Peter Robinson suggested to
> make the driver mutually exclusive and add !FB_SSD1307 in the config symbol.

And how will distros choose "the right" option in this case?
What to do when I wan to see a regression and I want to change drivers w/o
recompilation?

NAK from me to that proposal.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Andy,

On 2/2/22 12:06, Andy Shevchenko wrote:
> On Wed, Feb 02, 2022 at 09:38:51AM +0100, Javier Martinez Canillas wrote:
>> On 2/1/22 21:40, Sam Ravnborg wrote:
> 
> ...
> 
>> Peter Robinson suggested to
>> make the driver mutually exclusive and add !FB_SSD1307 in the config symbol.
> 
> And how will distros choose "the right" option in this case?

It depends on the distro. In Fedora we are disabling *all* the fbdev drivers.

> What to do when I wan to see a regression and I want to change drivers w/o
> recompilation?
>

If you want to have the two drivers without recompilation (and same compatible
to match) then how would kmod / udev choose which one to load ? It becomes a
race condition between the two drivers which one probes first.
 
> NAK from me to that proposal.
> 

What's your suggestion then to solve the issue mentioned above ? With my distro
maintainer hat I don't care that much, since the fbdev drivers will be disabled.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

Hello Andy,

On 2/2/22 12:06, Andy Shevchenko wrote:
> On Wed, Feb 02, 2022 at 09:38:51AM +0100, Javier Martinez Canillas wrote:
>> On 2/1/22 21:40, Sam Ravnborg wrote:
> 
> ...
> 
>> Peter Robinson suggested to
>> make the driver mutually exclusive and add !FB_SSD1307 in the config symbol.
> 
> And how will distros choose "the right" option in this case?

It depends on the distro. In Fedora we are disabling *all* the fbdev drivers.

> What to do when I wan to see a regression and I want to change drivers w/o
> recompilation?
>

If you want to have the two drivers without recompilation (and same compatible
to match) then how would kmod / udev choose which one to load ? It becomes a
race condition between the two drivers which one probes first.
 
> NAK from me to that proposal.
> 

What's your suggestion then to solve the issue mentioned above ? With my distro
maintainer hat I don't care that much, since the fbdev drivers will be disabled.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Andy Shevchenko]

On Wed, Feb 02, 2022 at 12:39:29PM +0100, Javier Martinez Canillas wrote:
> On 2/2/22 12:06, Andy Shevchenko wrote:
> > On Wed, Feb 02, 2022 at 09:38:51AM +0100, Javier Martinez Canillas wrote:
> >> On 2/1/22 21:40, Sam Ravnborg wrote:

> > And how will distros choose "the right" option in this case?
> 
> It depends on the distro. In Fedora we are disabling *all* the fbdev drivers.

Yes, and Distro A will think about old driver (because they have customers and
don't want to have a bad user experience) and Distro F will choose a new one.


> > What to do when I wan to see a regression and I want to change drivers w/o
> > recompilation?
> 
> If you want to have the two drivers without recompilation (and same compatible
> to match) then how would kmod / udev choose which one to load ? It becomes a
> race condition between the two drivers which one probes first.

We have a long history in kernel where new drivers came and old faded.
When two or more drivers of the same feature is enabled in the kernel
we may use modprobe facilities to prioritize them (blacklisting).

> > NAK from me to that proposal.
> 
> What's your suggestion then to solve the issue mentioned above ? With my distro
> maintainer hat I don't care that much, since the fbdev drivers will be disabled.

I think both of them can work together. If user doesn't care, the first one wins.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Andy Shevchenko]

On Wed, Feb 02, 2022 at 12:39:29PM +0100, Javier Martinez Canillas wrote:
> On 2/2/22 12:06, Andy Shevchenko wrote:
> > On Wed, Feb 02, 2022 at 09:38:51AM +0100, Javier Martinez Canillas wrote:
> >> On 2/1/22 21:40, Sam Ravnborg wrote:

> > And how will distros choose "the right" option in this case?
> 
> It depends on the distro. In Fedora we are disabling *all* the fbdev drivers.

Yes, and Distro A will think about old driver (because they have customers and
don't want to have a bad user experience) and Distro F will choose a new one.


> > What to do when I wan to see a regression and I want to change drivers w/o
> > recompilation?
> 
> If you want to have the two drivers without recompilation (and same compatible
> to match) then how would kmod / udev choose which one to load ? It becomes a
> race condition between the two drivers which one probes first.

We have a long history in kernel where new drivers came and old faded.
When two or more drivers of the same feature is enabled in the kernel
we may use modprobe facilities to prioritize them (blacklisting).

> > NAK from me to that proposal.
> 
> What's your suggestion then to solve the issue mentioned above ? With my distro
> maintainer hat I don't care that much, since the fbdev drivers will be disabled.

I think both of them can work together. If user doesn't care, the first one wins.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

On 2/2/22 12:50, Andy Shevchenko wrote:

[snip]

>> What's your suggestion then to solve the issue mentioned above ? With my distro
>> maintainer hat I don't care that much, since the fbdev drivers will be disabled.
> 
> I think both of them can work together. If user doesn't care, the first one wins.
> 

I don't think this is a good idea but as mentioned I don't really care that much
since we will disable all fbdev drivers anyway. So I'm happy to allow them both.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Javier Martinez Canillas]

On 2/2/22 12:50, Andy Shevchenko wrote:

[snip]

>> What's your suggestion then to solve the issue mentioned above ? With my distro
>> maintainer hat I don't care that much, since the fbdev drivers will be disabled.
> 
> I think both of them can work together. If user doesn't care, the first one wins.
> 

I don't think this is a good idea but as mentioned I don't really care that much
since we will disable all fbdev drivers anyway. So I'm happy to allow them both.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Andy Shevchenko]

On Wed, Feb 02, 2022 at 12:54:32PM +0100, Javier Martinez Canillas wrote:
> On 2/2/22 12:50, Andy Shevchenko wrote:
> >> What's your suggestion then to solve the issue mentioned above ? With my distro
> >> maintainer hat I don't care that much, since the fbdev drivers will be disabled.
> > 
> > I think both of them can work together. If user doesn't care, the first one wins.
> 
> I don't think this is a good idea but as mentioned I don't really care that much
> since we will disable all fbdev drivers anyway. So I'm happy to allow them both.

Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays

[Andy Shevchenko]

On Wed, Feb 02, 2022 at 12:54:32PM +0100, Javier Martinez Canillas wrote:
> On 2/2/22 12:50, Andy Shevchenko wrote:
> >> What's your suggestion then to solve the issue mentioned above ? With my distro
> >> maintainer hat I don't care that much, since the fbdev drivers will be disabled.
> > 
> > I think both of them can work together. If user doesn't care, the first one wins.
> 
> I don't think this is a good idea but as mentioned I don't really care that much
> since we will disable all fbdev drivers anyway. So I'm happy to allow them both.

Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] drm: Add I2C connector type

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1838 bytes --]

On Wed, 2 Feb 2022 10:45:42 +0100
Noralf Trønnes <noralf@tronnes.org> wrote:

> Den 02.02.2022 10.14, skrev Thomas Zimmermann:
> > Hi Noralf,
> > 
> > since you're here, I'll just hijack the discussion to ask something only
> > semi-related.
> > 
> > IIRC the gud driver doesn't update the display immediately during atomic
> > commits. Instead, it instructs a helper thread to do the update. What's
> > the rational behind this design? Is that something we should adopt for
> > other drivers that operate over slow buses (USB, I2C, etc)? Would this
> > be relevant for the ssd1307 driver?
> >   
> 
> Async flushing is only necessary on multi display setups where there's
> only one rendering loop for all the displays. I saw what tiny/gm12u320.c
> did and Hans gave me the rationale. The SPI drivers run flushing inline.
> Info on the gud wiki:
> https://github.com/notro/gud/wiki/Linux-Host-Driver#asynchronous-flushing

Hi,

please also consider that userspace may throttle to the KMS pageflip
events. If the pageflip event is immediate from submitting a flip, that
could mean userspace will be repainting in a busy-loop, like 1 kHz.
However, I remember something about virtual KMS drivers doing exactly
this, and there being something that tells userspace to throttle itself
instead of depending on pageflip completions. I just forget how that is
supposed to work, and I'm fairly sure that e.g. Weston does not behave
well there.

Unfortunately, the pageflip event is also what synchronises FB usage.
Once flipping in a new FB completed, the old FB is free for re-use.
But, if the kernel is still copying out from the old FB, userspace may
partially overwrite the contents, temporarily leading to an incomplete
or too new image on screen. Do you have anything to prevent that?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] drm: Add I2C connector type

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1838 bytes --]

On Wed, 2 Feb 2022 10:45:42 +0100
Noralf Trønnes <noralf@tronnes.org> wrote:

> Den 02.02.2022 10.14, skrev Thomas Zimmermann:
> > Hi Noralf,
> > 
> > since you're here, I'll just hijack the discussion to ask something only
> > semi-related.
> > 
> > IIRC the gud driver doesn't update the display immediately during atomic
> > commits. Instead, it instructs a helper thread to do the update. What's
> > the rational behind this design? Is that something we should adopt for
> > other drivers that operate over slow buses (USB, I2C, etc)? Would this
> > be relevant for the ssd1307 driver?
> >   
> 
> Async flushing is only necessary on multi display setups where there's
> only one rendering loop for all the displays. I saw what tiny/gm12u320.c
> did and Hans gave me the rationale. The SPI drivers run flushing inline.
> Info on the gud wiki:
> https://github.com/notro/gud/wiki/Linux-Host-Driver#asynchronous-flushing

Hi,

please also consider that userspace may throttle to the KMS pageflip
events. If the pageflip event is immediate from submitting a flip, that
could mean userspace will be repainting in a busy-loop, like 1 kHz.
However, I remember something about virtual KMS drivers doing exactly
this, and there being something that tells userspace to throttle itself
instead of depending on pageflip completions. I just forget how that is
supposed to work, and I'm fairly sure that e.g. Weston does not behave
well there.

Unfortunately, the pageflip event is also what synchronises FB usage.
Once flipping in a new FB completed, the old FB is free for re-use.
But, if the kernel is still copying out from the old FB, userspace may
partially overwrite the contents, temporarily leading to an incomplete
or too new image on screen. Do you have anything to prevent that?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] drm: Add I2C connector type

[Noralf Trønnes]

Den 02.02.2022 16.04, skrev Pekka Paalanen:
> On Wed, 2 Feb 2022 10:45:42 +0100
> Noralf Trønnes <noralf@tronnes.org> wrote:
> 
>> Den 02.02.2022 10.14, skrev Thomas Zimmermann:
>>> Hi Noralf,
>>>
>>> since you're here, I'll just hijack the discussion to ask something only
>>> semi-related.
>>>
>>> IIRC the gud driver doesn't update the display immediately during atomic
>>> commits. Instead, it instructs a helper thread to do the update. What's
>>> the rational behind this design? Is that something we should adopt for
>>> other drivers that operate over slow buses (USB, I2C, etc)? Would this
>>> be relevant for the ssd1307 driver?
>>>   
>>
>> Async flushing is only necessary on multi display setups where there's
>> only one rendering loop for all the displays. I saw what tiny/gm12u320.c
>> did and Hans gave me the rationale. The SPI drivers run flushing inline.
>> Info on the gud wiki:
>> https://github.com/notro/gud/wiki/Linux-Host-Driver#asynchronous-flushing
> 
> Hi,
> 
> please also consider that userspace may throttle to the KMS pageflip
> events. If the pageflip event is immediate from submitting a flip, that
> could mean userspace will be repainting in a busy-loop, like 1 kHz.
> However, I remember something about virtual KMS drivers doing exactly
> this, and there being something that tells userspace to throttle itself
> instead of depending on pageflip completions. I just forget how that is
> supposed to work, and I'm fairly sure that e.g. Weston does not behave
> well there.
> 
> Unfortunately, the pageflip event is also what synchronises FB usage.
> Once flipping in a new FB completed, the old FB is free for re-use.
> But, if the kernel is still copying out from the old FB, userspace may
> partially overwrite the contents, temporarily leading to an incomplete
> or too new image on screen. Do you have anything to prevent that?
> 

Unfortunately not. One solution would be to make a buffer copy during
the flip and do the USB transfer async but I haven't looked into that.
My plan is to wait and see what problems users report back before trying
to fix anything.

Noralf.

Re: [PATCH 1/4] drm: Add I2C connector type

[Noralf Trønnes]

Den 02.02.2022 16.04, skrev Pekka Paalanen:
> On Wed, 2 Feb 2022 10:45:42 +0100
> Noralf Trønnes <noralf@tronnes.org> wrote:
> 
>> Den 02.02.2022 10.14, skrev Thomas Zimmermann:
>>> Hi Noralf,
>>>
>>> since you're here, I'll just hijack the discussion to ask something only
>>> semi-related.
>>>
>>> IIRC the gud driver doesn't update the display immediately during atomic
>>> commits. Instead, it instructs a helper thread to do the update. What's
>>> the rational behind this design? Is that something we should adopt for
>>> other drivers that operate over slow buses (USB, I2C, etc)? Would this
>>> be relevant for the ssd1307 driver?
>>>   
>>
>> Async flushing is only necessary on multi display setups where there's
>> only one rendering loop for all the displays. I saw what tiny/gm12u320.c
>> did and Hans gave me the rationale. The SPI drivers run flushing inline.
>> Info on the gud wiki:
>> https://github.com/notro/gud/wiki/Linux-Host-Driver#asynchronous-flushing
> 
> Hi,
> 
> please also consider that userspace may throttle to the KMS pageflip
> events. If the pageflip event is immediate from submitting a flip, that
> could mean userspace will be repainting in a busy-loop, like 1 kHz.
> However, I remember something about virtual KMS drivers doing exactly
> this, and there being something that tells userspace to throttle itself
> instead of depending on pageflip completions. I just forget how that is
> supposed to work, and I'm fairly sure that e.g. Weston does not behave
> well there.
> 
> Unfortunately, the pageflip event is also what synchronises FB usage.
> Once flipping in a new FB completed, the old FB is free for re-use.
> But, if the kernel is still copying out from the old FB, userspace may
> partially overwrite the contents, temporarily leading to an incomplete
> or too new image on screen. Do you have anything to prevent that?
> 

Unfortunately not. One solution would be to make a buffer copy during
the flip and do the USB transfer async but I haven't looked into that.
My plan is to wait and see what problems users report back before trying
to fix anything.

Noralf.

Re: [PATCH 2/4] drm/format-helper: Add drm_fb_gray8_to_mono_reversed()

[Geert Uytterhoeven]

Hi Javier,

On Mon, Jan 31, 2022 at 9:12 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Add support to convert 8-bit grayscale to reversed monochrome for drivers
> that control monochromatic displays, that only have 1 bit per pixel depth.
>
> This helper function was based on repaper_gray8_to_mono_reversed() from
> the drivers/gpu/drm/tiny/repaper.c driver.
>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>         return -EINVAL;
>  }
>  EXPORT_SYMBOL(drm_fb_blit_toio);
> +
> +/**
> + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
> + * @dst: reversed monochrome destination buffer

What's the meaning of "reversed"?
During the last few days, I've been balancing between (a) "reverse
video" and (b) "reverse bit order", but none of them seems to be true.

(a) The code maps 0-127 to 0 and 8-255 to 1, which just reduces from
    256 to 2 grayscale levels, without inversion. The result is also
    white-on-black on my ssd130x OLED.
(b) On little-endian, the CFB drawops use little-endian bit order,
    which is what ends up in "byte" in the code below.

> + * @src: 8-bit grayscale source buffer
> + * @clip: Clip rectangle area to copy
> + *
> + * DRM doesn't have native monochrome or grayscale support.
> + * Such drivers can announce the commonly supported XR24 format to userspace
> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
> + * helper function to convert to the native format.
> + */
> +void drm_fb_gray8_to_mono_reversed(void *dst, void *src, const struct drm_rect *clip)
> +{
> +       size_t width = drm_rect_width(clip);
> +       size_t height = drm_rect_width(clip);
> +
> +       u8 *mono = dst, *gray8 = src;
> +       unsigned int y, xb, i;
> +
> +       for (y = 0; y < height; y++)
> +               for (xb = 0; xb < width / 8; xb++) {
> +                       u8 byte = 0x00;
> +
> +                       for (i = 0; i < 8; i++) {
> +                               int x = xb * 8 + i;
> +
> +                               byte >>= 1;
> +                               if (gray8[y * width + x] >> 7)
> +                                       byte |= BIT(7);
> +                       }
> +                       *mono++ = byte;
> +               }
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/4] drm/format-helper: Add drm_fb_gray8_to_mono_reversed()

[Geert Uytterhoeven]

Hi Javier,

On Mon, Jan 31, 2022 at 9:12 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Add support to convert 8-bit grayscale to reversed monochrome for drivers
> that control monochromatic displays, that only have 1 bit per pixel depth.
>
> This helper function was based on repaper_gray8_to_mono_reversed() from
> the drivers/gpu/drm/tiny/repaper.c driver.
>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>         return -EINVAL;
>  }
>  EXPORT_SYMBOL(drm_fb_blit_toio);
> +
> +/**
> + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
> + * @dst: reversed monochrome destination buffer

What's the meaning of "reversed"?
During the last few days, I've been balancing between (a) "reverse
video" and (b) "reverse bit order", but none of them seems to be true.

(a) The code maps 0-127 to 0 and 8-255 to 1, which just reduces from
    256 to 2 grayscale levels, without inversion. The result is also
    white-on-black on my ssd130x OLED.
(b) On little-endian, the CFB drawops use little-endian bit order,
    which is what ends up in "byte" in the code below.

> + * @src: 8-bit grayscale source buffer
> + * @clip: Clip rectangle area to copy
> + *
> + * DRM doesn't have native monochrome or grayscale support.
> + * Such drivers can announce the commonly supported XR24 format to userspace
> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
> + * helper function to convert to the native format.
> + */
> +void drm_fb_gray8_to_mono_reversed(void *dst, void *src, const struct drm_rect *clip)
> +{
> +       size_t width = drm_rect_width(clip);
> +       size_t height = drm_rect_width(clip);
> +
> +       u8 *mono = dst, *gray8 = src;
> +       unsigned int y, xb, i;
> +
> +       for (y = 0; y < height; y++)
> +               for (xb = 0; xb < width / 8; xb++) {
> +                       u8 byte = 0x00;
> +
> +                       for (i = 0; i < 8; i++) {
> +                               int x = xb * 8 + i;
> +
> +                               byte >>= 1;
> +                               if (gray8[y * width + x] >> 7)
> +                                       byte |= BIT(7);
> +                       }
> +                       *mono++ = byte;
> +               }
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/4] drm/format-helper: Add drm_fb_gray8_to_mono_reversed()

[Javier Martinez Canillas]

Hello Geert,

On 3/14/22 14:40, Geert Uytterhoeven wrote:
> Hi Javier,
> 
> On Mon, Jan 31, 2022 at 9:12 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> Add support to convert 8-bit grayscale to reversed monochrome for drivers
>> that control monochromatic displays, that only have 1 bit per pixel depth.
>>
>> This helper function was based on repaper_gray8_to_mono_reversed() from
>> the drivers/gpu/drm/tiny/repaper.c driver.
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
>> @@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>>         return -EINVAL;
>>  }
>>  EXPORT_SYMBOL(drm_fb_blit_toio);
>> +
>> +/**
>> + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
>> + * @dst: reversed monochrome destination buffer
> 
> What's the meaning of "reversed"?

Originally I took this helper from the repaper driver and it was called
repaper_gray8_to_mono_reversed(), so the naming just got carried over.

> During the last few days, I've been balancing between (a) "reverse
> video" and (b) "reverse bit order", but none of them seems to be true.
>
> (a) The code maps 0-127 to 0 and 8-255 to 1, which just reduces from
>     256 to 2 grayscale levels, without inversion. The result is also
>     white-on-black on my ssd130x OLED.
> (b) On little-endian, the CFB drawops use little-endian bit order,
>     which is what ends up in "byte" in the code below.
>

As far as I understand is (a) from the options since the repaper display
uses black-on-white. But as you pointed out the ssd130x OLEDs instead do
white-on-black.

I should had probably just name the helper drm_fb_gray8_to_monochrome()
or maybe drm_fb_gray8_to_gray1() ?

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 2/4] drm/format-helper: Add drm_fb_gray8_to_mono_reversed()

[Javier Martinez Canillas]

Hello Geert,

On 3/14/22 14:40, Geert Uytterhoeven wrote:
> Hi Javier,
> 
> On Mon, Jan 31, 2022 at 9:12 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> Add support to convert 8-bit grayscale to reversed monochrome for drivers
>> that control monochromatic displays, that only have 1 bit per pixel depth.
>>
>> This helper function was based on repaper_gray8_to_mono_reversed() from
>> the drivers/gpu/drm/tiny/repaper.c driver.
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
>> @@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>>         return -EINVAL;
>>  }
>>  EXPORT_SYMBOL(drm_fb_blit_toio);
>> +
>> +/**
>> + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
>> + * @dst: reversed monochrome destination buffer
> 
> What's the meaning of "reversed"?

Originally I took this helper from the repaper driver and it was called
repaper_gray8_to_mono_reversed(), so the naming just got carried over.

> During the last few days, I've been balancing between (a) "reverse
> video" and (b) "reverse bit order", but none of them seems to be true.
>
> (a) The code maps 0-127 to 0 and 8-255 to 1, which just reduces from
>     256 to 2 grayscale levels, without inversion. The result is also
>     white-on-black on my ssd130x OLED.
> (b) On little-endian, the CFB drawops use little-endian bit order,
>     which is what ends up in "byte" in the code below.
>

As far as I understand is (a) from the options since the repaper display
uses black-on-white. But as you pointed out the ssd130x OLEDs instead do
white-on-black.

I should had probably just name the helper drm_fb_gray8_to_monochrome()
or maybe drm_fb_gray8_to_gray1() ?

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat