* [PATCH v2 0/5] drm/ssd130x: A few enhancements and cleanups
@ 2023-06-09 17:09 Javier Martinez Canillas
2023-06-09 17:09 ` [PATCH v2 1/5] drm/ssd130x: Make default width and height to be controller dependent Javier Martinez Canillas
` (5 more replies)
0 siblings, 6 replies; 21+ messages in thread
From: Javier Martinez Canillas @ 2023-06-09 17:09 UTC (permalink / raw)
To: linux-kernel
Cc: Geert Uytterhoeven, Thomas Zimmermann, Maxime Ripard,
Javier Martinez Canillas, Conor Dooley, Daniel Vetter,
David Airlie, Krzysztof Kozlowski, Rob Herring, devicetree,
dri-devel
Hello,
While working on adding support for the SSD132X family of 4-bit grayscale
Solomon OLED panel controllers, I noticed a few things in the driver that
can be improved and make extending to support other chip families easier.
I've split the preparatory patches in this series and will post the actual
SSD132X support as a separate patch-set once this one is merged.
Best regards,
Javier
Changes in v2:
- List per controller default width/height values in DT schema (Maxime Ripard).
Javier Martinez Canillas (5):
drm/ssd130x: Make default width and height to be controller dependent
dt-bindings: display: ssd1307fb: Remove default width and height
values
drm/ssd130x: Set the page height value in the device info data
drm/ssd130x: Don't allocate buffers on each plane update
drm/ssd130x: Remove hardcoded bits-per-pixel in ssd130x_buf_alloc()
.../bindings/display/solomon,ssd1307fb.yaml | 28 +++-
drivers/gpu/drm/solomon/ssd130x.c | 124 ++++++++++++------
drivers/gpu/drm/solomon/ssd130x.h | 6 +
3 files changed, 113 insertions(+), 45 deletions(-)
--
2.40.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 1/5] drm/ssd130x: Make default width and height to be controller dependent
2023-06-09 17:09 [PATCH v2 0/5] drm/ssd130x: A few enhancements and cleanups Javier Martinez Canillas
@ 2023-06-09 17:09 ` Javier Martinez Canillas
2023-06-09 17:09 ` [PATCH v2 2/5] dt-bindings: display: ssd1307fb: Remove default width and height values Javier Martinez Canillas
` (4 subsequent siblings)
5 siblings, 0 replies; 21+ messages in thread
From: Javier Martinez Canillas @ 2023-06-09 17:09 UTC (permalink / raw)
To: linux-kernel
Cc: Geert Uytterhoeven, Thomas Zimmermann, Maxime Ripard,
Javier Martinez Canillas, Daniel Vetter, David Airlie, dri-devel
Currently the driver hardcodes the default values to 96x16 pixels but this
default resolution depends on the controller. The datasheets for the chips
describes the following display controller resolutions:
- SH1106: 132 x 64 Dot Matrix OLED/PLED
- SSD1305: 132 x 64 Dot Matrix OLED/PLED
- SSD1306: 128 x 64 Dot Matrix OLED/PLED
- SSD1307: 128 x 39 Dot Matrix OLED/PLED
- SSD1309: 128 x 64 Dot Matrix OLED/PLED
Add this information to the devices' info structures, and use it set as a
default if not defined in DT rather than hardcoding to an arbitrary value.
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
(no changes since v1)
drivers/gpu/drm/solomon/ssd130x.c | 14 ++++++++++++--
drivers/gpu/drm/solomon/ssd130x.h | 2 ++
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 8cbf5aa66e19..a0e5e26c0bc9 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -99,29 +99,39 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
.default_vcomh = 0x40,
.default_dclk_div = 1,
.default_dclk_frq = 5,
+ .default_width = 132,
+ .default_height = 64,
.page_mode_only = 1,
},
[SSD1305_ID] = {
.default_vcomh = 0x34,
.default_dclk_div = 1,
.default_dclk_frq = 7,
+ .default_width = 132,
+ .default_height = 64,
},
[SSD1306_ID] = {
.default_vcomh = 0x20,
.default_dclk_div = 1,
.default_dclk_frq = 8,
.need_chargepump = 1,
+ .default_width = 128,
+ .default_height = 64,
},
[SSD1307_ID] = {
.default_vcomh = 0x20,
.default_dclk_div = 2,
.default_dclk_frq = 12,
.need_pwm = 1,
+ .default_width = 128,
+ .default_height = 39,
},
[SSD1309_ID] = {
.default_vcomh = 0x34,
.default_dclk_div = 1,
.default_dclk_frq = 10,
+ .default_width = 128,
+ .default_height = 64,
}
};
EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X);
@@ -798,10 +808,10 @@ static void ssd130x_parse_properties(struct ssd130x_device *ssd130x)
struct device *dev = ssd130x->dev;
if (device_property_read_u32(dev, "solomon,width", &ssd130x->width))
- ssd130x->width = 96;
+ ssd130x->width = ssd130x->device_info->default_width;
if (device_property_read_u32(dev, "solomon,height", &ssd130x->height))
- ssd130x->height = 16;
+ ssd130x->height = ssd130x->device_info->default_height;
if (device_property_read_u32(dev, "solomon,page-offset", &ssd130x->page_offset))
ssd130x->page_offset = 1;
diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h
index db03ee5db392..a2bc8d75078b 100644
--- a/drivers/gpu/drm/solomon/ssd130x.h
+++ b/drivers/gpu/drm/solomon/ssd130x.h
@@ -37,6 +37,8 @@ struct ssd130x_deviceinfo {
u32 default_vcomh;
u32 default_dclk_div;
u32 default_dclk_frq;
+ u32 default_width;
+ u32 default_height;
int need_pwm;
int need_chargepump;
bool page_mode_only;
--
2.40.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 2/5] dt-bindings: display: ssd1307fb: Remove default width and height values
2023-06-09 17:09 [PATCH v2 0/5] drm/ssd130x: A few enhancements and cleanups Javier Martinez Canillas
2023-06-09 17:09 ` [PATCH v2 1/5] drm/ssd130x: Make default width and height to be controller dependent Javier Martinez Canillas
@ 2023-06-09 17:09 ` Javier Martinez Canillas
2023-06-10 15:10 ` Conor Dooley
2023-06-09 17:09 ` [PATCH v2 3/5] drm/ssd130x: Set the page height value in the device info data Javier Martinez Canillas
` (3 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Javier Martinez Canillas @ 2023-06-09 17:09 UTC (permalink / raw)
To: linux-kernel
Cc: Geert Uytterhoeven, Thomas Zimmermann, Maxime Ripard,
Javier Martinez Canillas, Conor Dooley, Daniel Vetter,
David Airlie, Krzysztof Kozlowski, Rob Herring, devicetree,
dri-devel
A default resolution in the ssd130x driver isn't set to an arbitrary 96x16
anymore. Instead is set to a width and height that's controller dependent.
The datasheets for the chips describes the following display resolutions:
- SH1106: 132 x 64 Dot Matrix OLED/PLED
- SSD1305: 132 x 64 Dot Matrix OLED/PLED
- SSD1306: 128 x 64 Dot Matrix OLED/PLED
- SSD1307: 128 x 39 Dot Matrix OLED/PLED
- SSD1309: 128 x 64 Dot Matrix OLED/PLED
Update DT schema to reflect what the driver does and make its users aware.
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
Changes in v2:
- List per controller default width/height values in DT schema (Maxime Ripard).
.../bindings/display/solomon,ssd1307fb.yaml | 28 ++++++++++++++++---
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
index 94bb5ef567c6..20e2bd15d4d2 100644
--- a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
+++ b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
@@ -49,15 +49,15 @@ properties:
solomon,height:
$ref: /schemas/types.yaml#/definitions/uint32
- default: 16
description:
- Height in pixel of the screen driven by the controller
+ Height in pixel of the screen driven by the controller.
+ The default value is controller-dependent.
solomon,width:
$ref: /schemas/types.yaml#/definitions/uint32
- default: 96
description:
- Width in pixel of the screen driven by the controller
+ Width in pixel of the screen driven by the controller.
+ The default value is controller-dependent.
solomon,page-offset:
$ref: /schemas/types.yaml#/definitions/uint32
@@ -157,6 +157,10 @@ allOf:
const: sinowealth,sh1106
then:
properties:
+ width:
+ default: 132
+ height:
+ default: 64
solomon,dclk-div:
default: 1
solomon,dclk-frq:
@@ -171,6 +175,10 @@ allOf:
- solomon,ssd1305
then:
properties:
+ width:
+ default: 132
+ height:
+ default: 64
solomon,dclk-div:
default: 1
solomon,dclk-frq:
@@ -185,6 +193,10 @@ allOf:
- solomon,ssd1306
then:
properties:
+ width:
+ default: 128
+ height:
+ default: 64
solomon,dclk-div:
default: 1
solomon,dclk-frq:
@@ -199,6 +211,10 @@ allOf:
- solomon,ssd1307
then:
properties:
+ width:
+ default: 128
+ height:
+ default: 39
solomon,dclk-div:
default: 2
solomon,dclk-frq:
@@ -215,6 +231,10 @@ allOf:
- solomon,ssd1309
then:
properties:
+ width:
+ default: 128
+ height:
+ default: 64
solomon,dclk-div:
default: 1
solomon,dclk-frq:
--
2.40.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 3/5] drm/ssd130x: Set the page height value in the device info data
2023-06-09 17:09 [PATCH v2 0/5] drm/ssd130x: A few enhancements and cleanups Javier Martinez Canillas
2023-06-09 17:09 ` [PATCH v2 1/5] drm/ssd130x: Make default width and height to be controller dependent Javier Martinez Canillas
2023-06-09 17:09 ` [PATCH v2 2/5] dt-bindings: display: ssd1307fb: Remove default width and height values Javier Martinez Canillas
@ 2023-06-09 17:09 ` Javier Martinez Canillas
2023-06-09 17:09 ` [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update Javier Martinez Canillas
` (2 subsequent siblings)
5 siblings, 0 replies; 21+ messages in thread
From: Javier Martinez Canillas @ 2023-06-09 17:09 UTC (permalink / raw)
To: linux-kernel
Cc: Geert Uytterhoeven, Thomas Zimmermann, Maxime Ripard,
Javier Martinez Canillas, Daniel Vetter, David Airlie, dri-devel
The driver only supports OLED controllers that have a page height of 8 but
there are devices that have different page heights. So it is better to not
hardcode this value and instead have it as a per controller data value.
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
(no changes since v1)
drivers/gpu/drm/solomon/ssd130x.c | 15 +++++++++++----
drivers/gpu/drm/solomon/ssd130x.h | 1 +
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index a0e5e26c0bc9..5cac1149e34e 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -102,6 +102,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
.default_width = 132,
.default_height = 64,
.page_mode_only = 1,
+ .page_height = 8,
},
[SSD1305_ID] = {
.default_vcomh = 0x34,
@@ -109,6 +110,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
.default_dclk_frq = 7,
.default_width = 132,
.default_height = 64,
+ .page_height = 8,
},
[SSD1306_ID] = {
.default_vcomh = 0x20,
@@ -117,6 +119,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
.need_chargepump = 1,
.default_width = 128,
.default_height = 64,
+ .page_height = 8,
},
[SSD1307_ID] = {
.default_vcomh = 0x20,
@@ -125,6 +128,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
.need_pwm = 1,
.default_width = 128,
.default_height = 39,
+ .page_height = 8,
},
[SSD1309_ID] = {
.default_vcomh = 0x34,
@@ -132,6 +136,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
.default_dclk_frq = 10,
.default_width = 128,
.default_height = 64,
+ .page_height = 8,
}
};
EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X);
@@ -437,7 +442,8 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf,
unsigned int width = drm_rect_width(rect);
unsigned int height = drm_rect_height(rect);
unsigned int line_length = DIV_ROUND_UP(width, 8);
- unsigned int pages = DIV_ROUND_UP(height, 8);
+ unsigned int page_height = ssd130x->device_info->page_height;
+ unsigned int pages = DIV_ROUND_UP(height, page_height);
struct drm_device *drm = &ssd130x->drm;
u32 array_idx = 0;
int ret, i, j, k;
@@ -559,16 +565,17 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
struct drm_rect *rect)
{
struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
+ unsigned int page_height = ssd130x->device_info->page_height;
struct iosys_map dst;
unsigned int dst_pitch;
int ret = 0;
u8 *buf = NULL;
/* Align y to display page boundaries */
- rect->y1 = round_down(rect->y1, 8);
- rect->y2 = min_t(unsigned int, round_up(rect->y2, 8), ssd130x->height);
+ rect->y1 = round_down(rect->y1, page_height);
+ rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height);
- dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
+ dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), page_height);
buf = kcalloc(dst_pitch, drm_rect_height(rect), GFP_KERNEL);
if (!buf)
return -ENOMEM;
diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h
index a2bc8d75078b..87968b3e7fb8 100644
--- a/drivers/gpu/drm/solomon/ssd130x.h
+++ b/drivers/gpu/drm/solomon/ssd130x.h
@@ -39,6 +39,7 @@ struct ssd130x_deviceinfo {
u32 default_dclk_frq;
u32 default_width;
u32 default_height;
+ u32 page_height;
int need_pwm;
int need_chargepump;
bool page_mode_only;
--
2.40.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update
2023-06-09 17:09 [PATCH v2 0/5] drm/ssd130x: A few enhancements and cleanups Javier Martinez Canillas
` (2 preceding siblings ...)
2023-06-09 17:09 ` [PATCH v2 3/5] drm/ssd130x: Set the page height value in the device info data Javier Martinez Canillas
@ 2023-06-09 17:09 ` Javier Martinez Canillas
2023-07-13 12:44 ` Geert Uytterhoeven
2023-06-09 17:09 ` [PATCH v2 5/5] drm/ssd130x: Remove hardcoded bits-per-pixel in ssd130x_buf_alloc() Javier Martinez Canillas
2023-06-15 21:53 ` [PATCH v2 0/5] drm/ssd130x: A few enhancements and cleanups Javier Martinez Canillas
5 siblings, 1 reply; 21+ messages in thread
From: Javier Martinez Canillas @ 2023-06-09 17:09 UTC (permalink / raw)
To: linux-kernel
Cc: Geert Uytterhoeven, Thomas Zimmermann, Maxime Ripard,
Javier Martinez Canillas, Daniel Vetter, David Airlie, dri-devel
The resolutions for these panels are fixed and defined in the Device Tree,
so there's no point to allocate the buffers on each plane update and that
can just be done once.
Let's do the allocation and free on the encoder enable and disable helpers
since that's where others initialization and teardown operations are done.
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
(no changes since v1)
drivers/gpu/drm/solomon/ssd130x.c | 88 +++++++++++++++++++------------
drivers/gpu/drm/solomon/ssd130x.h | 3 ++
2 files changed, 56 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 5cac1149e34e..0be3b476dc60 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -146,6 +146,31 @@ static inline struct ssd130x_device *drm_to_ssd130x(struct drm_device *drm)
return container_of(drm, struct ssd130x_device, drm);
}
+static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x)
+{
+ unsigned int page_height = ssd130x->device_info->page_height;
+ unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
+
+ ssd130x->buffer = kcalloc(DIV_ROUND_UP(ssd130x->width, 8),
+ ssd130x->height, GFP_KERNEL);
+ if (!ssd130x->buffer)
+ return -ENOMEM;
+
+ ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
+ if (!ssd130x->data_array) {
+ kfree(ssd130x->buffer);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static void ssd130x_buf_free(struct ssd130x_device *ssd130x)
+{
+ kfree(ssd130x->data_array);
+ kfree(ssd130x->buffer);
+}
+
/*
* Helper to write data (SSD130X_DATA) to the device.
*/
@@ -434,11 +459,12 @@ static int ssd130x_init(struct ssd130x_device *ssd130x)
SSD130X_SET_ADDRESS_MODE_HORIZONTAL);
}
-static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf,
- struct drm_rect *rect)
+static int ssd130x_update_rect(struct ssd130x_device *ssd130x, struct drm_rect *rect)
{
unsigned int x = rect->x1;
unsigned int y = rect->y1;
+ u8 *buf = ssd130x->buffer;
+ u8 *data_array = ssd130x->data_array;
unsigned int width = drm_rect_width(rect);
unsigned int height = drm_rect_height(rect);
unsigned int line_length = DIV_ROUND_UP(width, 8);
@@ -447,14 +473,9 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf,
struct drm_device *drm = &ssd130x->drm;
u32 array_idx = 0;
int ret, i, j, k;
- u8 *data_array = NULL;
drm_WARN_ONCE(drm, y % 8 != 0, "y must be aligned to screen page\n");
- data_array = kcalloc(width, pages, GFP_KERNEL);
- if (!data_array)
- return -ENOMEM;
-
/*
* The screen is divided in pages, each having a height of 8
* pixels, and the width of the screen. When sending a byte of
@@ -488,11 +509,11 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf,
/* Set address range for horizontal addressing mode */
ret = ssd130x_set_col_range(ssd130x, ssd130x->col_offset + x, width);
if (ret < 0)
- goto out_free;
+ return ret;
ret = ssd130x_set_page_range(ssd130x, ssd130x->page_offset + y / 8, pages);
if (ret < 0)
- goto out_free;
+ return ret;
}
for (i = 0; i < pages; i++) {
@@ -522,11 +543,11 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf,
ssd130x->page_offset + i,
ssd130x->col_offset + x);
if (ret < 0)
- goto out_free;
+ return ret;
ret = ssd130x_write_data(ssd130x, data_array, width);
if (ret < 0)
- goto out_free;
+ return ret;
array_idx = 0;
}
@@ -536,14 +557,11 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf,
if (!ssd130x->page_address_mode)
ret = ssd130x_write_data(ssd130x, data_array, width * pages);
-out_free:
- kfree(data_array);
return ret;
}
static void ssd130x_clear_screen(struct ssd130x_device *ssd130x)
{
- u8 *buf = NULL;
struct drm_rect fullscreen = {
.x1 = 0,
.x2 = ssd130x->width,
@@ -551,14 +569,7 @@ static void ssd130x_clear_screen(struct ssd130x_device *ssd130x)
.y2 = ssd130x->height,
};
- buf = kcalloc(DIV_ROUND_UP(ssd130x->width, 8), ssd130x->height,
- GFP_KERNEL);
- if (!buf)
- return;
-
- ssd130x_update_rect(ssd130x, buf, &fullscreen);
-
- kfree(buf);
+ ssd130x_update_rect(ssd130x, &fullscreen);
}
static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_map *vmap,
@@ -569,30 +580,27 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
struct iosys_map dst;
unsigned int dst_pitch;
int ret = 0;
- u8 *buf = NULL;
+ u8 *buf = ssd130x->buffer;
+
+ if (!buf)
+ return 0;
/* Align y to display page boundaries */
rect->y1 = round_down(rect->y1, page_height);
rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height);
dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), page_height);
- buf = kcalloc(dst_pitch, drm_rect_height(rect), GFP_KERNEL);
- if (!buf)
- return -ENOMEM;
ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
if (ret)
- goto out_free;
+ return ret;
iosys_map_set_vaddr(&dst, buf);
drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect);
drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
- ssd130x_update_rect(ssd130x, buf, rect);
-
-out_free:
- kfree(buf);
+ ssd130x_update_rect(ssd130x, rect);
return ret;
}
@@ -701,14 +709,22 @@ static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
return;
ret = ssd130x_init(ssd130x);
- if (ret) {
- ssd130x_power_off(ssd130x);
- return;
- }
+ if (ret)
+ goto power_off;
+
+ ret = ssd130x_buf_alloc(ssd130x);
+ if (ret)
+ goto power_off;
ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_ON);
backlight_enable(ssd130x->bl_dev);
+
+ return;
+
+power_off:
+ ssd130x_power_off(ssd130x);
+ return;
}
static void ssd130x_encoder_helper_atomic_disable(struct drm_encoder *encoder,
@@ -721,6 +737,8 @@ static void ssd130x_encoder_helper_atomic_disable(struct drm_encoder *encoder,
ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_OFF);
+ ssd130x_buf_free(ssd130x);
+
ssd130x_power_off(ssd130x);
}
diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h
index 87968b3e7fb8..161588b1cc4d 100644
--- a/drivers/gpu/drm/solomon/ssd130x.h
+++ b/drivers/gpu/drm/solomon/ssd130x.h
@@ -89,6 +89,9 @@ struct ssd130x_device {
u8 col_end;
u8 page_start;
u8 page_end;
+
+ u8 *buffer;
+ u8 *data_array;
};
extern const struct ssd130x_deviceinfo ssd130x_variants[];
--
2.40.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 5/5] drm/ssd130x: Remove hardcoded bits-per-pixel in ssd130x_buf_alloc()
2023-06-09 17:09 [PATCH v2 0/5] drm/ssd130x: A few enhancements and cleanups Javier Martinez Canillas
` (3 preceding siblings ...)
2023-06-09 17:09 ` [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update Javier Martinez Canillas
@ 2023-06-09 17:09 ` Javier Martinez Canillas
2023-07-12 10:24 ` Geert Uytterhoeven
2023-06-15 21:53 ` [PATCH v2 0/5] drm/ssd130x: A few enhancements and cleanups Javier Martinez Canillas
5 siblings, 1 reply; 21+ messages in thread
From: Javier Martinez Canillas @ 2023-06-09 17:09 UTC (permalink / raw)
To: linux-kernel
Cc: Geert Uytterhoeven, Thomas Zimmermann, Maxime Ripard,
Javier Martinez Canillas, Daniel Vetter, David Airlie, dri-devel
The driver only supports OLED controllers that have a native DRM_FORMAT_C1
pixel format and that is why it has harcoded a division of the width by 8.
But the driver might be extended to support devices that have a different
pixel format. So it's better to use the struct drm_format_info helpers to
compute the size of the buffer, used to store the pixels in native format.
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
(no changes since v1)
drivers/gpu/drm/solomon/ssd130x.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 0be3b476dc60..b3dc1ca9dc10 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -150,9 +150,16 @@ static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x)
{
unsigned int page_height = ssd130x->device_info->page_height;
unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
+ const struct drm_format_info *fi;
+ unsigned int pitch;
- ssd130x->buffer = kcalloc(DIV_ROUND_UP(ssd130x->width, 8),
- ssd130x->height, GFP_KERNEL);
+ fi = drm_format_info(DRM_FORMAT_C1);
+ if (!fi)
+ return -EINVAL;
+
+ pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
+
+ ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
if (!ssd130x->buffer)
return -ENOMEM;
--
2.40.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/5] dt-bindings: display: ssd1307fb: Remove default width and height values
2023-06-09 17:09 ` [PATCH v2 2/5] dt-bindings: display: ssd1307fb: Remove default width and height values Javier Martinez Canillas
@ 2023-06-10 15:10 ` Conor Dooley
2023-06-10 17:51 ` Javier Martinez Canillas
0 siblings, 1 reply; 21+ messages in thread
From: Conor Dooley @ 2023-06-10 15:10 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: linux-kernel, Geert Uytterhoeven, Thomas Zimmermann,
Maxime Ripard, Conor Dooley, Daniel Vetter, David Airlie,
Krzysztof Kozlowski, Rob Herring, devicetree, dri-devel
[-- Attachment #1: Type: text/plain, Size: 3653 bytes --]
On Fri, Jun 09, 2023 at 07:09:37PM +0200, Javier Martinez Canillas wrote:
> A default resolution in the ssd130x driver isn't set to an arbitrary 96x16
> anymore. Instead is set to a width and height that's controller dependent.
Did that change to the driver not break backwards compatibility with
existing devicetrees that relied on the default values to get 96x16?
Cheers,
Conor.
>
> The datasheets for the chips describes the following display resolutions:
>
> - SH1106: 132 x 64 Dot Matrix OLED/PLED
> - SSD1305: 132 x 64 Dot Matrix OLED/PLED
> - SSD1306: 128 x 64 Dot Matrix OLED/PLED
> - SSD1307: 128 x 39 Dot Matrix OLED/PLED
> - SSD1309: 128 x 64 Dot Matrix OLED/PLED
>
> Update DT schema to reflect what the driver does and make its users aware.
>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>
> Changes in v2:
> - List per controller default width/height values in DT schema (Maxime Ripard).
>
> .../bindings/display/solomon,ssd1307fb.yaml | 28 ++++++++++++++++---
> 1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
> index 94bb5ef567c6..20e2bd15d4d2 100644
> --- a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
> +++ b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
> @@ -49,15 +49,15 @@ properties:
>
> solomon,height:
> $ref: /schemas/types.yaml#/definitions/uint32
> - default: 16
> description:
> - Height in pixel of the screen driven by the controller
> + Height in pixel of the screen driven by the controller.
> + The default value is controller-dependent.
>
> solomon,width:
> $ref: /schemas/types.yaml#/definitions/uint32
> - default: 96
> description:
> - Width in pixel of the screen driven by the controller
> + Width in pixel of the screen driven by the controller.
> + The default value is controller-dependent.
>
> solomon,page-offset:
> $ref: /schemas/types.yaml#/definitions/uint32
> @@ -157,6 +157,10 @@ allOf:
> const: sinowealth,sh1106
> then:
> properties:
> + width:
> + default: 132
> + height:
> + default: 64
> solomon,dclk-div:
> default: 1
> solomon,dclk-frq:
> @@ -171,6 +175,10 @@ allOf:
> - solomon,ssd1305
> then:
> properties:
> + width:
> + default: 132
> + height:
> + default: 64
> solomon,dclk-div:
> default: 1
> solomon,dclk-frq:
> @@ -185,6 +193,10 @@ allOf:
> - solomon,ssd1306
> then:
> properties:
> + width:
> + default: 128
> + height:
> + default: 64
> solomon,dclk-div:
> default: 1
> solomon,dclk-frq:
> @@ -199,6 +211,10 @@ allOf:
> - solomon,ssd1307
> then:
> properties:
> + width:
> + default: 128
> + height:
> + default: 39
> solomon,dclk-div:
> default: 2
> solomon,dclk-frq:
> @@ -215,6 +231,10 @@ allOf:
> - solomon,ssd1309
> then:
> properties:
> + width:
> + default: 128
> + height:
> + default: 64
> solomon,dclk-div:
> default: 1
> solomon,dclk-frq:
> --
> 2.40.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/5] dt-bindings: display: ssd1307fb: Remove default width and height values
2023-06-10 15:10 ` Conor Dooley
@ 2023-06-10 17:51 ` Javier Martinez Canillas
2023-06-10 18:14 ` Conor Dooley
0 siblings, 1 reply; 21+ messages in thread
From: Javier Martinez Canillas @ 2023-06-10 17:51 UTC (permalink / raw)
To: Conor Dooley
Cc: devicetree, Conor Dooley, Thomas Zimmermann, Krzysztof Kozlowski,
linux-kernel, Maxime Ripard, Rob Herring, Geert Uytterhoeven,
dri-devel
Conor Dooley <conor@kernel.org> writes:
> On Fri, Jun 09, 2023 at 07:09:37PM +0200, Javier Martinez Canillas wrote:
>> A default resolution in the ssd130x driver isn't set to an arbitrary 96x16
>> anymore. Instead is set to a width and height that's controller dependent.
>
> Did that change to the driver not break backwards compatibility with
> existing devicetrees that relied on the default values to get 96x16?
>
It would but I don't think it is an issue in pratice. Most users of these
panels use one of the multiple libraries on top of the spidev interface.
For the small userbase that don't, I believe that they will use the rpif
kernel and ssd1306-overlay.dtbo DTB overlay, which defaults to width=128
and height=64 [1]. So those users will have to explicitly set a width and
height for a 96x16 panel anyways.
The intersection of users that have a 96x16 panel, assumed that default
and consider the DTB a stable ABI, and only update their kernel but not
the DTB should be very small IMO.
[1]: https://github.com/raspberrypi/linux/blob/rpi-6.1.y/arch/arm/boot/dts/overlays/ssd1306-overlay.dts
> Cheers,
> Conor.
>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/5] dt-bindings: display: ssd1307fb: Remove default width and height values
2023-06-10 17:51 ` Javier Martinez Canillas
@ 2023-06-10 18:14 ` Conor Dooley
2023-06-10 23:18 ` Javier Martinez Canillas
0 siblings, 1 reply; 21+ messages in thread
From: Conor Dooley @ 2023-06-10 18:14 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: devicetree, Conor Dooley, Thomas Zimmermann, Krzysztof Kozlowski,
linux-kernel, Maxime Ripard, Rob Herring, Geert Uytterhoeven,
dri-devel
[-- Attachment #1: Type: text/plain, Size: 1536 bytes --]
On Sat, Jun 10, 2023 at 07:51:35PM +0200, Javier Martinez Canillas wrote:
> Conor Dooley <conor@kernel.org> writes:
>
> > On Fri, Jun 09, 2023 at 07:09:37PM +0200, Javier Martinez Canillas wrote:
> >> A default resolution in the ssd130x driver isn't set to an arbitrary 96x16
> >> anymore. Instead is set to a width and height that's controller dependent.
> >
> > Did that change to the driver not break backwards compatibility with
> > existing devicetrees that relied on the default values to get 96x16?
> >
>
> It would but I don't think it is an issue in pratice. Most users of these
> panels use one of the multiple libraries on top of the spidev interface.
>
> For the small userbase that don't, I believe that they will use the rpif
> kernel and ssd1306-overlay.dtbo DTB overlay, which defaults to width=128
> and height=64 [1]. So those users will have to explicitly set a width and
> height for a 96x16 panel anyways.
>
> The intersection of users that have a 96x16 panel, assumed that default
> and consider the DTB a stable ABI, and only update their kernel but not
> the DTB should be very small IMO.
It's the adding of new defaults that makes it a bit messier, since you
can't even revert without potentially breaking a newer user. I'd be more
inclined to require the properties, rather change their defaults in the
binding, lest there are people relying on them.
If you and the other knowledgeable folk in the area really do know such
users do not exist then I suppose it is fine to do.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/5] dt-bindings: display: ssd1307fb: Remove default width and height values
2023-06-10 18:14 ` Conor Dooley
@ 2023-06-10 23:18 ` Javier Martinez Canillas
2023-06-12 7:47 ` Thomas Zimmermann
0 siblings, 1 reply; 21+ messages in thread
From: Javier Martinez Canillas @ 2023-06-10 23:18 UTC (permalink / raw)
To: Conor Dooley
Cc: devicetree, Conor Dooley, Thomas Zimmermann, Krzysztof Kozlowski,
linux-kernel, Maxime Ripard, Rob Herring, Geert Uytterhoeven,
dri-devel
Conor Dooley <conor@kernel.org> writes:
> On Sat, Jun 10, 2023 at 07:51:35PM +0200, Javier Martinez Canillas wrote:
>> Conor Dooley <conor@kernel.org> writes:
>>
>> > On Fri, Jun 09, 2023 at 07:09:37PM +0200, Javier Martinez Canillas wrote:
>> >> A default resolution in the ssd130x driver isn't set to an arbitrary 96x16
>> >> anymore. Instead is set to a width and height that's controller dependent.
>> >
>> > Did that change to the driver not break backwards compatibility with
>> > existing devicetrees that relied on the default values to get 96x16?
>> >
>>
>> It would but I don't think it is an issue in pratice. Most users of these
>> panels use one of the multiple libraries on top of the spidev interface.
>>
>> For the small userbase that don't, I believe that they will use the rpif
>> kernel and ssd1306-overlay.dtbo DTB overlay, which defaults to width=128
>> and height=64 [1]. So those users will have to explicitly set a width and
>> height for a 96x16 panel anyways.
>>
>> The intersection of users that have a 96x16 panel, assumed that default
>> and consider the DTB a stable ABI, and only update their kernel but not
>> the DTB should be very small IMO.
>
> It's the adding of new defaults that makes it a bit messier, since you
> can't even revert without potentially breaking a newer user. I'd be more
> inclined to require the properties, rather change their defaults in the
> binding, lest there are people relying on them.
I think that's OK, the old drivers/video/fbdev/ssd1307fb.c fbdev driver
still has the old behaviour so it will only be a problem for users that
want to move to the new ssd130x driver as well.
By looking at the git log history, the 96x16 resolution was chosen when
the driver was merged because Maxime tested it on a CFA10036 board [1]
that has a 96x16 panel that uses an SSD1307 controller.
But as mentioned, that chip can drive up to 128x39 pixels so the maximum
makes more sense as default to me.
[1]: https://www.crystalfontz.com/product/cfa10036
> If you and the other knowledgeable folk in the area really do know such
> users do not exist then I suppose it is fine to do.
I believe is fine, since as explained above that change was only done in
the ssd130x DRM driver, not the ssd1307fb fbdev driver whose default is
still 96x16. Both drivers share the same DT binding scheme, I was asked
to do that to make it as much backward compatible as possible with the
old fbdev driver.
But I will be OK to drop the "solomon,ssd130?fb-i2c" compatible strings
from the DRM driver and only match against the new "solomon,ssd130?-i2c"
compatible strings. And add a different DT binding schema for the ssd130x
driver, if that would mean being able to fix things like the one mentioned
in this patch.
In my opinion, trying to always make the drivers backward compatible with
old DTBs only makes the drivers code more complicated for unclear benefit.
Usually this just ends being code that is neither used nor tested. Because
in practice most people update the DTBs and kernels, instead of trying to
make the DTB a stable ABI like firmware.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/5] dt-bindings: display: ssd1307fb: Remove default width and height values
2023-06-10 23:18 ` Javier Martinez Canillas
@ 2023-06-12 7:47 ` Thomas Zimmermann
2023-06-12 17:44 ` Conor Dooley
0 siblings, 1 reply; 21+ messages in thread
From: Thomas Zimmermann @ 2023-06-12 7:47 UTC (permalink / raw)
To: Javier Martinez Canillas, Conor Dooley
Cc: devicetree, Conor Dooley, linux-kernel, Maxime Ripard,
Rob Herring, Geert Uytterhoeven, dri-devel, Krzysztof Kozlowski
[-- Attachment #1.1: Type: text/plain, Size: 3705 bytes --]
Hi
Am 11.06.23 um 01:18 schrieb Javier Martinez Canillas:
> Conor Dooley <conor@kernel.org> writes:
>
>> On Sat, Jun 10, 2023 at 07:51:35PM +0200, Javier Martinez Canillas wrote:
>>> Conor Dooley <conor@kernel.org> writes:
>>>
>>>> On Fri, Jun 09, 2023 at 07:09:37PM +0200, Javier Martinez Canillas wrote:
>>>>> A default resolution in the ssd130x driver isn't set to an arbitrary 96x16
>>>>> anymore. Instead is set to a width and height that's controller dependent.
>>>>
>>>> Did that change to the driver not break backwards compatibility with
>>>> existing devicetrees that relied on the default values to get 96x16?
>>>>
>>>
>>> It would but I don't think it is an issue in pratice. Most users of these
>>> panels use one of the multiple libraries on top of the spidev interface.
>>>
>>> For the small userbase that don't, I believe that they will use the rpif
>>> kernel and ssd1306-overlay.dtbo DTB overlay, which defaults to width=128
>>> and height=64 [1]. So those users will have to explicitly set a width and
>>> height for a 96x16 panel anyways.
>>>
>>> The intersection of users that have a 96x16 panel, assumed that default
>>> and consider the DTB a stable ABI, and only update their kernel but not
>>> the DTB should be very small IMO.
>>
>> It's the adding of new defaults that makes it a bit messier, since you
>> can't even revert without potentially breaking a newer user. I'd be more
>> inclined to require the properties, rather change their defaults in the
>> binding, lest there are people relying on them.
>
> I think that's OK, the old drivers/video/fbdev/ssd1307fb.c fbdev driver
> still has the old behaviour so it will only be a problem for users that
> want to move to the new ssd130x driver as well.
>
> By looking at the git log history, the 96x16 resolution was chosen when
> the driver was merged because Maxime tested it on a CFA10036 board [1]
> that has a 96x16 panel that uses an SSD1307 controller.
>
> But as mentioned, that chip can drive up to 128x39 pixels so the maximum
> makes more sense as default to me.
>
> [1]: https://www.crystalfontz.com/product/cfa10036
>
>> If you and the other knowledgeable folk in the area really do know such
>> users do not exist then I suppose it is fine to do.
>
> I believe is fine, since as explained above that change was only done in
> the ssd130x DRM driver, not the ssd1307fb fbdev driver whose default is
> still 96x16. Both drivers share the same DT binding scheme, I was asked
> to do that to make it as much backward compatible as possible with the
> old fbdev driver.
>
> But I will be OK to drop the "solomon,ssd130?fb-i2c" compatible strings
> from the DRM driver and only match against the new "solomon,ssd130?-i2c"
> compatible strings. And add a different DT binding schema for the ssd130x
> driver, if that would mean being able to fix things like the one mentioned
> in this patch.
>
> In my opinion, trying to always make the drivers backward compatible with
> old DTBs only makes the drivers code more complicated for unclear benefit.
>
> Usually this just ends being code that is neither used nor tested. Because
> in practice most people update the DTBs and kernels, instead of trying to
> make the DTB a stable ABI like firmware.
>
From my understanding, fixing the resolution is the correct thing to do
here. Userspace needs to be able to handle these differences.
Best regards
Thomas
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/5] dt-bindings: display: ssd1307fb: Remove default width and height values
2023-06-12 7:47 ` Thomas Zimmermann
@ 2023-06-12 17:44 ` Conor Dooley
2023-06-12 18:30 ` Javier Martinez Canillas
0 siblings, 1 reply; 21+ messages in thread
From: Conor Dooley @ 2023-06-12 17:44 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: Javier Martinez Canillas, devicetree, Conor Dooley, linux-kernel,
Maxime Ripard, Rob Herring, Geert Uytterhoeven, dri-devel,
Krzysztof Kozlowski
[-- Attachment #1: Type: text/plain, Size: 1377 bytes --]
On Mon, Jun 12, 2023 at 09:47:12AM +0200, Thomas Zimmermann wrote:
> Am 11.06.23 um 01:18 schrieb Javier Martinez Canillas:
> > But I will be OK to drop the "solomon,ssd130?fb-i2c" compatible strings
> > from the DRM driver and only match against the new "solomon,ssd130?-i2c"
> > compatible strings. And add a different DT binding schema for the ssd130x
> > driver, if that would mean being able to fix things like the one mentioned
> > in this patch.
If there are different compatibles, then it can always be sorted out
later iff it turns out to be a problem, since new devicetrees should not
be using the deprecated compatibles anyway. I didn't realise that those
deprecated compatibles existed, thanks for your patience.
> > In my opinion, trying to always make the drivers backward compatible with
> > old DTBs only makes the drivers code more complicated for unclear benefit.
> >
> > Usually this just ends being code that is neither used nor tested. Because
> > in practice most people update the DTBs and kernels, instead of trying to
> > make the DTB a stable ABI like firmware.
> >
>
> From my understanding, fixing the resolution is the correct thing to do
> here. Userspace needs to be able to handle these differences.
Fixing meaning correcting, or fixing meaning using a fixed resolution?
Not clear to me what you mean, sorry.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/5] dt-bindings: display: ssd1307fb: Remove default width and height values
2023-06-12 17:44 ` Conor Dooley
@ 2023-06-12 18:30 ` Javier Martinez Canillas
0 siblings, 0 replies; 21+ messages in thread
From: Javier Martinez Canillas @ 2023-06-12 18:30 UTC (permalink / raw)
To: Conor Dooley, Thomas Zimmermann
Cc: devicetree, Conor Dooley, linux-kernel, Maxime Ripard,
Rob Herring, Geert Uytterhoeven, dri-devel, Krzysztof Kozlowski
Conor Dooley <conor@kernel.org> writes:
> On Mon, Jun 12, 2023 at 09:47:12AM +0200, Thomas Zimmermann wrote:
>> Am 11.06.23 um 01:18 schrieb Javier Martinez Canillas:
>
>> > But I will be OK to drop the "solomon,ssd130?fb-i2c" compatible strings
>> > from the DRM driver and only match against the new "solomon,ssd130?-i2c"
>> > compatible strings. And add a different DT binding schema for the ssd130x
>> > driver, if that would mean being able to fix things like the one mentioned
>> > in this patch.
>
> If there are different compatibles, then it can always be sorted out
> later iff it turns out to be a problem, since new devicetrees should not
> be using the deprecated compatibles anyway. I didn't realise that those
> deprecated compatibles existed, thanks for your patience.
>
No worries, thanks for raising this question.
>> > In my opinion, trying to always make the drivers backward compatible with
>> > old DTBs only makes the drivers code more complicated for unclear benefit.
>> >
>> > Usually this just ends being code that is neither used nor tested. Because
>> > in practice most people update the DTBs and kernels, instead of trying to
>> > make the DTB a stable ABI like firmware.
>> >
>>
>> From my understanding, fixing the resolution is the correct thing to do
>> here. Userspace needs to be able to handle these differences.
>
> Fixing meaning correcting, or fixing meaning using a fixed resolution?
> Not clear to me what you mean, sorry.
>
Fixing meaning using as a default the correct maximum resolution for each
OLED controller, rather than an arbitrary 96x16 resolution that was added
just to be compatible with the panel that was tested the original driver.
But after talking with Thomas and Maxime about this issue, I realized that
it won't even cause an issue for theoretical users that may be relying on
the previous default.
Changing the default resolution to something smaller could cause an issue
since a user expecting a bigger default would get their display output cut
but changing to something bigger just means user-space being able to write
more pixels than those that will be displayed.
Because there isn't really a "resolution" configured in the chip, but just
how many pixels a particular controller can drive. The new default is the
maximum that each controller supports according to their documentation.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/5] drm/ssd130x: A few enhancements and cleanups
2023-06-09 17:09 [PATCH v2 0/5] drm/ssd130x: A few enhancements and cleanups Javier Martinez Canillas
` (4 preceding siblings ...)
2023-06-09 17:09 ` [PATCH v2 5/5] drm/ssd130x: Remove hardcoded bits-per-pixel in ssd130x_buf_alloc() Javier Martinez Canillas
@ 2023-06-15 21:53 ` Javier Martinez Canillas
5 siblings, 0 replies; 21+ messages in thread
From: Javier Martinez Canillas @ 2023-06-15 21:53 UTC (permalink / raw)
To: linux-kernel
Cc: Geert Uytterhoeven, Thomas Zimmermann, Maxime Ripard,
Conor Dooley, Daniel Vetter, David Airlie, Krzysztof Kozlowski,
Rob Herring, devicetree, dri-devel
Javier Martinez Canillas <javierm@redhat.com> writes:
> Hello,
>
> While working on adding support for the SSD132X family of 4-bit grayscale
> Solomon OLED panel controllers, I noticed a few things in the driver that
> can be improved and make extending to support other chip families easier.
>
> I've split the preparatory patches in this series and will post the actual
> SSD132X support as a separate patch-set once this one is merged.
>
> Best regards,
> Javier
>
> Changes in v2:
> - List per controller default width/height values in DT schema (Maxime Ripard).
>
Pushed to drm-misc (drm-misc-next). Thanks!
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 5/5] drm/ssd130x: Remove hardcoded bits-per-pixel in ssd130x_buf_alloc()
2023-06-09 17:09 ` [PATCH v2 5/5] drm/ssd130x: Remove hardcoded bits-per-pixel in ssd130x_buf_alloc() Javier Martinez Canillas
@ 2023-07-12 10:24 ` Geert Uytterhoeven
0 siblings, 0 replies; 21+ messages in thread
From: Geert Uytterhoeven @ 2023-07-12 10:24 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: linux-kernel, Thomas Zimmermann, Maxime Ripard, Daniel Vetter,
David Airlie, dri-devel
Hi Javier,
Thanks for your patch!
On Fri, Jun 9, 2023 at 7:09 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> The driver only supports OLED controllers that have a native DRM_FORMAT_C1
DRM_FORMAT_R1 (colormap is fixed to white-on-black).
> pixel format and that is why it has harcoded a division of the width by 8.
hardcoded.
> But the driver might be extended to support devices that have a different
> pixel format. So it's better to use the struct drm_format_info helpers to
> compute the size of the buffer, used to store the pixels in native format.
>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -150,9 +150,16 @@ static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x)
> {
> unsigned int page_height = ssd130x->device_info->page_height;
> unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
> + const struct drm_format_info *fi;
> + unsigned int pitch;
>
> - ssd130x->buffer = kcalloc(DIV_ROUND_UP(ssd130x->width, 8),
> - ssd130x->height, GFP_KERNEL);
> + fi = drm_format_info(DRM_FORMAT_C1);
DRM_FORMAT_R1.
> + if (!fi)
> + return -EINVAL;
> +
> + pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
> +
> + ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
> if (!ssd130x->buffer)
> return -ENOMEM;
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update
2023-06-09 17:09 ` [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update Javier Martinez Canillas
@ 2023-07-13 12:44 ` Geert Uytterhoeven
2023-07-13 13:15 ` Javier Martinez Canillas
0 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2023-07-13 12:44 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: linux-kernel, Thomas Zimmermann, Maxime Ripard, Daniel Vetter,
David Airlie, dri-devel
Hi Javier,
On Fri, Jun 9, 2023 at 7:09 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> The resolutions for these panels are fixed and defined in the Device Tree,
> so there's no point to allocate the buffers on each plane update and that
> can just be done once.
>
> Let's do the allocation and free on the encoder enable and disable helpers
> since that's where others initialization and teardown operations are done.
>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>
> (no changes since v1)
Thanks for your patch, which is now commit 49d7d581ceaf4cf8
("drm/ssd130x: Don't allocate buffers on each plane update") in
drm-misc/for-linux-next.
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -701,14 +709,22 @@ static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
> return;
>
> ret = ssd130x_init(ssd130x);
> - if (ret) {
> - ssd130x_power_off(ssd130x);
> - return;
> - }
> + if (ret)
> + goto power_off;
> +
> + ret = ssd130x_buf_alloc(ssd130x);
This appears to be too late, causing a NULL pointer dereference:
[ 59.302761] [<c0303d90>] ssd130x_update_rect.isra.0+0x13c/0x340
[ 59.304231] [<c0304200>]
ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
[ 59.305716] [<c02f8d54>] drm_atomic_helper_commit_planes+0xfc/0x27c
Bailing out from ssd130x_update_rect() when data_array is still NULL
fixes that.
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update
2023-07-13 12:44 ` Geert Uytterhoeven
@ 2023-07-13 13:15 ` Javier Martinez Canillas
2023-07-13 13:25 ` Geert Uytterhoeven
0 siblings, 1 reply; 21+ messages in thread
From: Javier Martinez Canillas @ 2023-07-13 13:15 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-kernel, Thomas Zimmermann, Maxime Ripard, Daniel Vetter,
David Airlie, dri-devel
Geert Uytterhoeven <geert@linux-m68k.org> writes:
Hello Geert,
> Hi Javier,
>
> On Fri, Jun 9, 2023 at 7:09 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> The resolutions for these panels are fixed and defined in the Device Tree,
>> so there's no point to allocate the buffers on each plane update and that
>> can just be done once.
>>
>> Let's do the allocation and free on the encoder enable and disable helpers
>> since that's where others initialization and teardown operations are done.
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>
>> (no changes since v1)
>
> Thanks for your patch, which is now commit 49d7d581ceaf4cf8
> ("drm/ssd130x: Don't allocate buffers on each plane update") in
> drm-misc/for-linux-next.
>
>> --- a/drivers/gpu/drm/solomon/ssd130x.c
>> +++ b/drivers/gpu/drm/solomon/ssd130x.c
>> @@ -701,14 +709,22 @@ static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
>> return;
>>
>> ret = ssd130x_init(ssd130x);
>> - if (ret) {
>> - ssd130x_power_off(ssd130x);
>> - return;
>> - }
>> + if (ret)
>> + goto power_off;
>> +
>> + ret = ssd130x_buf_alloc(ssd130x);
>
> This appears to be too late, causing a NULL pointer dereference:
>
Thanks for reporting this issue.
> [ 59.302761] [<c0303d90>] ssd130x_update_rect.isra.0+0x13c/0x340
> [ 59.304231] [<c0304200>]
> ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
> [ 59.305716] [<c02f8d54>] drm_atomic_helper_commit_planes+0xfc/0x27c
>
I wonder how this could be too late. I thought that the encoder
.atomic_enable callback would be called before any plane .atomic_update.
> Bailing out from ssd130x_update_rect() when data_array is still NULL
> fixes that.
>
Maybe we can add that with a drm_WARN() ? I still want to understand how
a plane update can happen before an encoder enable.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update
2023-07-13 13:15 ` Javier Martinez Canillas
@ 2023-07-13 13:25 ` Geert Uytterhoeven
2023-07-13 14:12 ` Javier Martinez Canillas
0 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2023-07-13 13:25 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: linux-kernel, Thomas Zimmermann, Maxime Ripard, Daniel Vetter,
David Airlie, dri-devel
Hi Javier,
On Thu, Jul 13, 2023 at 3:21 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > On Fri, Jun 9, 2023 at 7:09 PM Javier Martinez Canillas
> > <javierm@redhat.com> wrote:
> >> The resolutions for these panels are fixed and defined in the Device Tree,
> >> so there's no point to allocate the buffers on each plane update and that
> >> can just be done once.
> >>
> >> Let's do the allocation and free on the encoder enable and disable helpers
> >> since that's where others initialization and teardown operations are done.
> >>
> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> >> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> ---
> >>
> >> (no changes since v1)
> >
> > Thanks for your patch, which is now commit 49d7d581ceaf4cf8
> > ("drm/ssd130x: Don't allocate buffers on each plane update") in
> > drm-misc/for-linux-next.
> >
> >> --- a/drivers/gpu/drm/solomon/ssd130x.c
> >> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> >> @@ -701,14 +709,22 @@ static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
> >> return;
> >>
> >> ret = ssd130x_init(ssd130x);
> >> - if (ret) {
> >> - ssd130x_power_off(ssd130x);
> >> - return;
> >> - }
> >> + if (ret)
> >> + goto power_off;
> >> +
> >> + ret = ssd130x_buf_alloc(ssd130x);
> >
> > This appears to be too late, causing a NULL pointer dereference:
> >
>
> Thanks for reporting this issue.
>
> > [ 59.302761] [<c0303d90>] ssd130x_update_rect.isra.0+0x13c/0x340
> > [ 59.304231] [<c0304200>]
> > ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
> > [ 59.305716] [<c02f8d54>] drm_atomic_helper_commit_planes+0xfc/0x27c
> >
>
> I wonder how this could be too late. I thought that the encoder
> .atomic_enable callback would be called before any plane .atomic_update.
>
> > Bailing out from ssd130x_update_rect() when data_array is still NULL
> > fixes that.
> >
>
> Maybe we can add that with a drm_WARN() ? I still want to understand how
> a plane update can happen before an encoder enable.
Full log is:
ssd130x-i2c 0-003c: supply vcc not found, using dummy regulator
[drm] Initialized ssd130x 1.0.0 20220131 for 0-003c on minor 0
ssd130x-i2c 0-003c: [drm] surface width(128), height(32), bpp(1)
and format(R1 little-endian (0x20203152))
Unable to handle kernel NULL pointer dereference at virtual address 00000000
Oops [#1]
CPU: 0 PID: 1 Comm: swapper Not tainted
6.5.0-rc1-orangecrab-02219-g0a529a1e4bf4 #565
epc : ssd130x_update_rect.isra.0+0x13c/0x340
ra : ssd130x_update_rect.isra.0+0x2bc/0x340
epc : c0303d90 ra : c0303f10 sp : c182b5b0
gp : c06d37f0 tp : c1828000 t0 : 00000064
t1 : 00000000 t2 : 00000000 s0 : c182b600
s1 : c2044000 a0 : 00000000 a1 : 00000000
a2 : 00000008 a3 : a040f080 a4 : 00000000
a5 : 00000000 a6 : 00001000 a7 : 00000008
s2 : 00000004 s3 : 00000080 s4 : c2045000
s5 : 00000010 s6 : 00000080 s7 : 00000000
s8 : 00000000 s9 : a040f000 s10: 00000008
s11: 00000000 t3 : 00000153 t4 : c2050ef4
t5 : c20447a0 t6 : 00000080
status: 00000120 badaddr: 00000000 cause: 0000000f
[<c0303d90>] ssd130x_update_rect.isra.0+0x13c/0x340
[<c0304200>] ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
[<c02f8d54>] drm_atomic_helper_commit_planes+0xfc/0x27c
[<c02f9314>] drm_atomic_helper_commit_tail+0x5c/0xb4
[<c02f94fc>] commit_tail+0x190/0x1b8
[<c02f99fc>] drm_atomic_helper_commit+0x194/0x1c0
[<c02c5d00>] drm_atomic_commit+0xa4/0xe4
[<c02cce40>] drm_client_modeset_commit_atomic+0x244/0x278
[<c02ccef0>] drm_client_modeset_commit_locked+0x7c/0x1bc
[<c02cd064>] drm_client_modeset_commit+0x34/0x64
[<c0301a78>] __drm_fb_helper_restore_fbdev_mode_unlocked+0xc4/0xe8
[<c0303424>] drm_fb_helper_set_par+0x38/0x58
[<c027c410>] fbcon_init+0x294/0x534
[<c02af188>] visual_init+0xac/0x114
[<c02b1834>] do_bind_con_driver.isra.0+0x1bc/0x39c
[<c02b2fcc>] do_take_over_console+0x128/0x1b4
[<c027ad24>] do_fbcon_takeover+0x74/0xfc
[<c027e704>] fbcon_fb_registered+0x168/0x1b4
[<c0275c84>] register_framebuffer+0x180/0x238
[<c03017a4>] __drm_fb_helper_initial_config_and_unlock+0x328/0x538
[<c0303844>] drm_fb_helper_initial_config+0x40/0x54
[<c0300818>] drm_fbdev_generic_client_hotplug+0x98/0xdc
[<c0300c5c>] drm_fbdev_generic_setup+0x9c/0x178
[<c0304fa0>] ssd130x_probe+0x5e0/0x788
[<c030522c>] ssd130x_i2c_probe+0x4c/0x70
[<c0358464>] i2c_device_probe+0x120/0x1f0
[<c030e5a4>] really_probe+0xb8/0x30c
[<c030e974>] __driver_probe_device+0x17c/0x1c8
[<c030ea0c>] driver_probe_device+0x4c/0x140
[<c030ebe4>] __device_attach_driver+0xe4/0x150
[<c030ccc4>] bus_for_each_drv+0x8c/0x100
[<c030f034>] __device_attach+0x12c/0x1ac
[<c030f3e0>] device_initial_probe+0x18/0x28
[<c030cf14>] bus_probe_device+0xcc/0xd0
[<c030b274>] device_add+0x5d8/0x7b4
[<c030b474>] device_register+0x24/0x38
[<c0358f24>] i2c_new_client_device+0x1a8/0x2b8
[<c035b960>] of_i2c_register_devices+0xdc/0x164
[<c0359750>] i2c_register_adapter+0x1b8/0x56c
[<c0359eb0>] i2c_add_adapter+0x94/0x100
[<c035d47c>] __i2c_bit_add_bus+0xc0/0x460
[<c035d838>] i2c_bit_add_bus+0x1c/0x2c
[<c035da20>] litex_i2c_probe+0x108/0x164
[<c0310de4>] platform_probe+0x54/0xb0
[<c030e5a4>] really_probe+0xb8/0x30c
[<c030e974>] __driver_probe_device+0x17c/0x1c8
[<c030ea0c>] driver_probe_device+0x4c/0x140
[<c030ed74>] __driver_attach+0x124/0x1f4
[<c030c880>] bus_for_each_dev+0x84/0xf4
[<c030f4f8>] driver_attach+0x28/0x38
[<c030d174>] bus_add_driver+0x120/0x214
[<c030fc90>] driver_register+0x70/0x15c
[<c0311d98>] __platform_driver_register+0x28/0x38
[<c0520958>] litex_i2c_driver_init+0x24/0x34
[<c0507fe8>] do_one_initcall+0x80/0x238
[<c05083d4>] kernel_init_freeable+0x1b4/0x238
[<c04fdd9c>] kernel_init+0x24/0x144
[<c00022d8>] ret_from_fork+0x10/0x24
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update
2023-07-13 13:25 ` Geert Uytterhoeven
@ 2023-07-13 14:12 ` Javier Martinez Canillas
2023-07-13 14:53 ` Geert Uytterhoeven
0 siblings, 1 reply; 21+ messages in thread
From: Javier Martinez Canillas @ 2023-07-13 14:12 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-kernel, Thomas Zimmermann, Maxime Ripard, Daniel Vetter,
David Airlie, dri-devel
Geert Uytterhoeven <geert@linux-m68k.org> writes:
Hello Geert,
> Hi Javier,
>
> On Thu, Jul 13, 2023 at 3:21 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> > On Fri, Jun 9, 2023 at 7:09 PM Javier Martinez Canillas
>> > <javierm@redhat.com> wrote:
>> >> The resolutions for these panels are fixed and defined in the Device Tree,
>> >> so there's no point to allocate the buffers on each plane update and that
>> >> can just be done once.
>> >>
>> >> Let's do the allocation and free on the encoder enable and disable helpers
>> >> since that's where others initialization and teardown operations are done.
>> >>
>> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> >> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>> >> ---
>> >>
>> >> (no changes since v1)
>> >
>> > Thanks for your patch, which is now commit 49d7d581ceaf4cf8
>> > ("drm/ssd130x: Don't allocate buffers on each plane update") in
>> > drm-misc/for-linux-next.
>> >
>> >> --- a/drivers/gpu/drm/solomon/ssd130x.c
>> >> +++ b/drivers/gpu/drm/solomon/ssd130x.c
>> >> @@ -701,14 +709,22 @@ static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
>> >> return;
>> >>
>> >> ret = ssd130x_init(ssd130x);
>> >> - if (ret) {
>> >> - ssd130x_power_off(ssd130x);
>> >> - return;
>> >> - }
>> >> + if (ret)
>> >> + goto power_off;
>> >> +
>> >> + ret = ssd130x_buf_alloc(ssd130x);
>> >
>> > This appears to be too late, causing a NULL pointer dereference:
>> >
>>
>> Thanks for reporting this issue.
>>
>> > [ 59.302761] [<c0303d90>] ssd130x_update_rect.isra.0+0x13c/0x340
>> > [ 59.304231] [<c0304200>]
>> > ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
>> > [ 59.305716] [<c02f8d54>] drm_atomic_helper_commit_planes+0xfc/0x27c
>> >
>>
>> I wonder how this could be too late. I thought that the encoder
>> .atomic_enable callback would be called before any plane .atomic_update.
>>
>> > Bailing out from ssd130x_update_rect() when data_array is still NULL
>> > fixes that.
>> >
>>
>> Maybe we can add that with a drm_WARN() ? I still want to understand how
>> a plane update can happen before an encoder enable.
>
> Full log is:
>
> ssd130x-i2c 0-003c: supply vcc not found, using dummy regulator
> [drm] Initialized ssd130x 1.0.0 20220131 for 0-003c on minor 0
> ssd130x-i2c 0-003c: [drm] surface width(128), height(32), bpp(1)
> and format(R1 little-endian (0x20203152))
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> Oops [#1]
> CPU: 0 PID: 1 Comm: swapper Not tainted
> 6.5.0-rc1-orangecrab-02219-g0a529a1e4bf4 #565
> epc : ssd130x_update_rect.isra.0+0x13c/0x340
> ra : ssd130x_update_rect.isra.0+0x2bc/0x340
> epc : c0303d90 ra : c0303f10 sp : c182b5b0
> gp : c06d37f0 tp : c1828000 t0 : 00000064
> t1 : 00000000 t2 : 00000000 s0 : c182b600
> s1 : c2044000 a0 : 00000000 a1 : 00000000
> a2 : 00000008 a3 : a040f080 a4 : 00000000
> a5 : 00000000 a6 : 00001000 a7 : 00000008
> s2 : 00000004 s3 : 00000080 s4 : c2045000
> s5 : 00000010 s6 : 00000080 s7 : 00000000
> s8 : 00000000 s9 : a040f000 s10: 00000008
> s11: 00000000 t3 : 00000153 t4 : c2050ef4
> t5 : c20447a0 t6 : 00000080
> status: 00000120 badaddr: 00000000 cause: 0000000f
> [<c0303d90>] ssd130x_update_rect.isra.0+0x13c/0x340
> [<c0304200>] ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
> [<c02f8d54>] drm_atomic_helper_commit_planes+0xfc/0x27c
> [<c02f9314>] drm_atomic_helper_commit_tail+0x5c/0xb4
> [<c02f94fc>] commit_tail+0x190/0x1b8
> [<c02f99fc>] drm_atomic_helper_commit+0x194/0x1c0
Thanks for the log, so I think the problem is that the default struct
drm_mode_config_helper_funcs .atomic_commit_tail is
drm_atomic_helper_commit_tail():
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L1710
That helper calls drm_atomic_helper_commit_planes() and attempts to commit
the state for all planes even for CRTC that are not enabled. I see that
there is a drm_atomic_helper_commit_tail_rpm() helper that instead calls:
drm_atomic_helper_commit_planes(dev, old_state, DRM_PLANE_COMMIT_ACTIVE_ONLY),
which I thought that was the default behaviour.
Can you please try the following change [0] ? If that works then I can
propose as a proper patch.
[0]:
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index afb08a8aa9fc..c543caa3ceee 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -795,6 +795,10 @@ static const struct drm_mode_config_funcs ssd130x_mode_config_funcs = {
.atomic_commit = drm_atomic_helper_commit,
};
+static const struct drm_mode_config_helper_funcs ssd130x_mode_config_helpers = {
+ .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+};
+
static const uint32_t ssd130x_formats[] = {
DRM_FORMAT_XRGB8888,
};
@@ -923,6 +927,7 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
drm->mode_config.max_height = max_height;
drm->mode_config.preferred_depth = 24;
drm->mode_config.funcs = &ssd130x_mode_config_funcs;
+ drm->mode_config.helper_private = &ssd130x_mode_config_helpers;
/* Primary plane */
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update
2023-07-13 14:12 ` Javier Martinez Canillas
@ 2023-07-13 14:53 ` Geert Uytterhoeven
2023-07-13 16:34 ` Javier Martinez Canillas
0 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2023-07-13 14:53 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: linux-kernel, Thomas Zimmermann, Maxime Ripard, Daniel Vetter,
David Airlie, dri-devel
Hi Javier,
On Thu, Jul 13, 2023 at 4:13 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > On Thu, Jul 13, 2023 at 3:21 PM Javier Martinez Canillas
> > <javierm@redhat.com> wrote:
> >> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> >> > On Fri, Jun 9, 2023 at 7:09 PM Javier Martinez Canillas
> >> > <javierm@redhat.com> wrote:
> >> >> The resolutions for these panels are fixed and defined in the Device Tree,
> >> >> so there's no point to allocate the buffers on each plane update and that
> >> >> can just be done once.
> >> >>
> >> >> Let's do the allocation and free on the encoder enable and disable helpers
> >> >> since that's where others initialization and teardown operations are done.
> >> >>
> >> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> >> >> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> >> ---
> >> >>
> >> >> (no changes since v1)
> >> >
> >> > Thanks for your patch, which is now commit 49d7d581ceaf4cf8
> >> > ("drm/ssd130x: Don't allocate buffers on each plane update") in
> >> > drm-misc/for-linux-next.
> >> >
> >> >> --- a/drivers/gpu/drm/solomon/ssd130x.c
> >> >> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> >> >> @@ -701,14 +709,22 @@ static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
> >> >> return;
> >> >>
> >> >> ret = ssd130x_init(ssd130x);
> >> >> - if (ret) {
> >> >> - ssd130x_power_off(ssd130x);
> >> >> - return;
> >> >> - }
> >> >> + if (ret)
> >> >> + goto power_off;
> >> >> +
> >> >> + ret = ssd130x_buf_alloc(ssd130x);
> >> >
> >> > This appears to be too late, causing a NULL pointer dereference:
> >> >
> >>
> >> Thanks for reporting this issue.
> >>
> >> > [ 59.302761] [<c0303d90>] ssd130x_update_rect.isra.0+0x13c/0x340
> >> > [ 59.304231] [<c0304200>]
> >> > ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
> >> > [ 59.305716] [<c02f8d54>] drm_atomic_helper_commit_planes+0xfc/0x27c
> >> >
> >>
> >> I wonder how this could be too late. I thought that the encoder
> >> .atomic_enable callback would be called before any plane .atomic_update.
[...]
> Thanks for the log, so I think the problem is that the default struct
> drm_mode_config_helper_funcs .atomic_commit_tail is
> drm_atomic_helper_commit_tail():
>
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L1710
>
> That helper calls drm_atomic_helper_commit_planes() and attempts to commit
> the state for all planes even for CRTC that are not enabled. I see that
> there is a drm_atomic_helper_commit_tail_rpm() helper that instead calls:
>
> drm_atomic_helper_commit_planes(dev, old_state, DRM_PLANE_COMMIT_ACTIVE_ONLY),
> which I thought that was the default behaviour.
>
> Can you please try the following change [0] ? If that works then I can
> propose as a proper patch.
>
> [0]:
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index afb08a8aa9fc..c543caa3ceee 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -795,6 +795,10 @@ static const struct drm_mode_config_funcs ssd130x_mode_config_funcs = {
> .atomic_commit = drm_atomic_helper_commit,
> };
>
> +static const struct drm_mode_config_helper_funcs ssd130x_mode_config_helpers = {
> + .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> +};
> +
> static const uint32_t ssd130x_formats[] = {
> DRM_FORMAT_XRGB8888,
> };
> @@ -923,6 +927,7 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
> drm->mode_config.max_height = max_height;
> drm->mode_config.preferred_depth = 24;
> drm->mode_config.funcs = &ssd130x_mode_config_funcs;
> + drm->mode_config.helper_private = &ssd130x_mode_config_helpers;
>
> /* Primary plane */
>
Thanks, that works!
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update
2023-07-13 14:53 ` Geert Uytterhoeven
@ 2023-07-13 16:34 ` Javier Martinez Canillas
0 siblings, 0 replies; 21+ messages in thread
From: Javier Martinez Canillas @ 2023-07-13 16:34 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-kernel, Thomas Zimmermann, Maxime Ripard, Daniel Vetter,
David Airlie, dri-devel
Geert Uytterhoeven <geert@linux-m68k.org> writes:
Hello Geert,
> Hi Javier,
[...]
>> +static const struct drm_mode_config_helper_funcs ssd130x_mode_config_helpers = {
>> + .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
>> +};
>> +
>> static const uint32_t ssd130x_formats[] = {
>> DRM_FORMAT_XRGB8888,
>> };
>> @@ -923,6 +927,7 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
>> drm->mode_config.max_height = max_height;
>> drm->mode_config.preferred_depth = 24;
>> drm->mode_config.funcs = &ssd130x_mode_config_funcs;
>> + drm->mode_config.helper_private = &ssd130x_mode_config_helpers;
>>
>> /* Primary plane */
>>
>
> Thanks, that works!
>
Thanks for testing! Proper patch sent:
https://lists.freedesktop.org/archives/dri-devel/2023-July/413630.html
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2023-07-13 16:36 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-09 17:09 [PATCH v2 0/5] drm/ssd130x: A few enhancements and cleanups Javier Martinez Canillas
2023-06-09 17:09 ` [PATCH v2 1/5] drm/ssd130x: Make default width and height to be controller dependent Javier Martinez Canillas
2023-06-09 17:09 ` [PATCH v2 2/5] dt-bindings: display: ssd1307fb: Remove default width and height values Javier Martinez Canillas
2023-06-10 15:10 ` Conor Dooley
2023-06-10 17:51 ` Javier Martinez Canillas
2023-06-10 18:14 ` Conor Dooley
2023-06-10 23:18 ` Javier Martinez Canillas
2023-06-12 7:47 ` Thomas Zimmermann
2023-06-12 17:44 ` Conor Dooley
2023-06-12 18:30 ` Javier Martinez Canillas
2023-06-09 17:09 ` [PATCH v2 3/5] drm/ssd130x: Set the page height value in the device info data Javier Martinez Canillas
2023-06-09 17:09 ` [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update Javier Martinez Canillas
2023-07-13 12:44 ` Geert Uytterhoeven
2023-07-13 13:15 ` Javier Martinez Canillas
2023-07-13 13:25 ` Geert Uytterhoeven
2023-07-13 14:12 ` Javier Martinez Canillas
2023-07-13 14:53 ` Geert Uytterhoeven
2023-07-13 16:34 ` Javier Martinez Canillas
2023-06-09 17:09 ` [PATCH v2 5/5] drm/ssd130x: Remove hardcoded bits-per-pixel in ssd130x_buf_alloc() Javier Martinez Canillas
2023-07-12 10:24 ` Geert Uytterhoeven
2023-06-15 21:53 ` [PATCH v2 0/5] drm/ssd130x: A few enhancements and cleanups Javier Martinez Canillas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).