diff mbox series

[v2,2/6] drm/ssd130x: Add a per controller family functions table

Message ID 20231012065822.1007930-3-javierm@redhat.com (mailing list archive)
State Superseded
Headers show
Series drm/solomon: Add support for the SSD132x controller family | expand

Commit Message

Javier Martinez Canillas Oct. 12, 2023, 6:58 a.m. UTC 
To allow the driver to decouple the common logic in the function callbacks
from the behaviour that is specific of a given Solomon controller family.

For this, include a chip family to the device info and add fields to the
driver-private device struct, to store the hardware buffer maximum size,
the intermediate buffer maximum size and pixel format, and a set of per
chip family function handlers.

While doing this change, also use drm_format_info_min_pitch() to calculate
the dest_pitch. This avoids to assume a 1 bpp and instead the pixel format
info for the intermediate buffer is used.

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

Changes in v2:
- Squash patch that uses drm_format_info_min_pitch() to calculate dest_pitch
  with the following patch (Geert Uytterhoeven).
- Store ssd13xx_family_funcs[SSD130X_FAMILY] in struct ssd130x_deviceinfo
  (Geert Uytterhoeven).
- Don't mix switch (family_id) and ssd13xx_funcs[family_id] (Geert Uytterhoeven).
- Replace switch (family_id) by an .set_buffer_sizes (Geert Uytterhoeven).
- Move the rect alignment to a per chip family function (Geert Uytterhoeven).

 drivers/gpu/drm/solomon/ssd130x-i2c.c |  1 +
 drivers/gpu/drm/solomon/ssd130x-spi.c |  2 +
 drivers/gpu/drm/solomon/ssd130x.c     | 95 ++++++++++++++++++++-------
 drivers/gpu/drm/solomon/ssd130x.h     | 24 +++++++
 4 files changed, 99 insertions(+), 23 deletions(-)

Comments

Thomas Zimmermann Oct. 12, 2023, 7:39 a.m. UTC | #1 
Hi Javier

Am 12.10.23 um 08:58 schrieb Javier Martinez Canillas:
[...]
>   
> +struct ssd130x_funcs {
> +	int (*init)(struct ssd130x_device *ssd130x);
> +	int (*set_buffer_sizes)(struct ssd130x_device *ssd130x);
> +	void (*align_rect)(struct ssd130x_device *ssd130x, struct drm_rect *rect);
> +	int (*update_rect)(struct ssd130x_device *ssd130x, struct drm_rect *rect,
> +			   u8 *buf, u8 *data_array);
> +	void (*clear_screen)(struct ssd130x_device *ssd130x,
> +			     u8 *data_array);
> +	void (*fmt_convert)(struct iosys_map *dst, const unsigned int *dst_pitch,
> +			    const struct iosys_map *src, const struct drm_framebuffer *fb,
> +			    const struct drm_rect *clip);
> +};
> +

You are reinventing DRM's atomic helpers. I strongly advised against 
doing that, as it often turns out bad. Maybe see my rant at [1] wrt to 
another driver.

It's much better to create a separate mode-setting pipeline for the 
ssd132x series and share the common code among pipelines. Your driver 
will have a clean and readable implementation for each supported 
chipset. Compare an old version of mgag200 [2] with the current driver 
to see the difference.

Best regards
Thomas

[1] 
https://lore.kernel.org/dri-devel/07cc89a5-5200-72e6-f078-694c5820a99a@suse.de/
[2] https://elixir.bootlin.com/linux/v5.5/source/drivers/gpu/drm/mgag200


>   extern const struct ssd130x_deviceinfo ssd130x_variants[];
>   
>   struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap);
Javier Martinez Canillas Oct. 12, 2023, 8:02 a.m. UTC | #2 
Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

Thanks a lot for your feedback.

> Hi Javier
>
> Am 12.10.23 um 08:58 schrieb Javier Martinez Canillas:
> [...]
>>   
>> +struct ssd130x_funcs {
>> +	int (*init)(struct ssd130x_device *ssd130x);
>> +	int (*set_buffer_sizes)(struct ssd130x_device *ssd130x);
>> +	void (*align_rect)(struct ssd130x_device *ssd130x, struct drm_rect *rect);
>> +	int (*update_rect)(struct ssd130x_device *ssd130x, struct drm_rect *rect,
>> +			   u8 *buf, u8 *data_array);
>> +	void (*clear_screen)(struct ssd130x_device *ssd130x,
>> +			     u8 *data_array);
>> +	void (*fmt_convert)(struct iosys_map *dst, const unsigned int *dst_pitch,
>> +			    const struct iosys_map *src, const struct drm_framebuffer *fb,
>> +			    const struct drm_rect *clip);
>> +};
>> +
>
> You are reinventing DRM's atomic helpers. I strongly advised against 
> doing that, as it often turns out bad. Maybe see my rant at [1] wrt to 
> another driver.
>
> It's much better to create a separate mode-setting pipeline for the 
> ssd132x series and share the common code among pipelines. Your driver 
> will have a clean and readable implementation for each supported 
> chipset. Compare an old version of mgag200 [2] with the current driver 
> to see the difference.
>

I see what you mean. The reason why I didn't go that route was to minimize
code duplication, but you are correct that each level of indirection makes
the driver harder to read, to reason about and fragile (modifying a common
callback could have undesired effects on other chip families as you said).

I'll give it a try to what you propose in v3, have separate modesetting
pipeline for SSD130x and SSD132x, even if this could lead to a little more
duplicated code.

> Best regards
> Thomas
>
Thomas Zimmermann Oct. 12, 2023, 8:21 a.m. UTC | #3 
Hi Javier

Am 12.10.23 um 10:02 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
> Hello Thomas,
> 
> Thanks a lot for your feedback.
> 
>> Hi Javier
>>
>> Am 12.10.23 um 08:58 schrieb Javier Martinez Canillas:
>> [...]
>>>    
>>> +struct ssd130x_funcs {
>>> +	int (*init)(struct ssd130x_device *ssd130x);
>>> +	int (*set_buffer_sizes)(struct ssd130x_device *ssd130x);
>>> +	void (*align_rect)(struct ssd130x_device *ssd130x, struct drm_rect *rect);
>>> +	int (*update_rect)(struct ssd130x_device *ssd130x, struct drm_rect *rect,
>>> +			   u8 *buf, u8 *data_array);
>>> +	void (*clear_screen)(struct ssd130x_device *ssd130x,
>>> +			     u8 *data_array);
>>> +	void (*fmt_convert)(struct iosys_map *dst, const unsigned int *dst_pitch,
>>> +			    const struct iosys_map *src, const struct drm_framebuffer *fb,
>>> +			    const struct drm_rect *clip);
>>> +};
>>> +
>>
>> You are reinventing DRM's atomic helpers. I strongly advised against
>> doing that, as it often turns out bad. Maybe see my rant at [1] wrt to
>> another driver.
>>
>> It's much better to create a separate mode-setting pipeline for the
>> ssd132x series and share the common code among pipelines. Your driver
>> will have a clean and readable implementation for each supported
>> chipset. Compare an old version of mgag200 [2] with the current driver
>> to see the difference.
>>
> 
> I see what you mean. The reason why I didn't go that route was to minimize
> code duplication, but you are correct that each level of indirection makes
> the driver harder to read, to reason about and fragile (modifying a common
> callback could have undesired effects on other chip families as you said).
> 
> I'll give it a try to what you propose in v3, have separate modesetting
> pipeline for SSD130x and SSD132x, even if this could lead to a little more
> duplicated code.

Thanks a lot. I know you want to make a reference driver for others to 
study. So I think at least trying the multi-pipeline way is worth it.

 From the mgag200 and ast drivers, I found that the refactoring patch 
sets can be large to the point where one wonders whether it's actually 
worth it. But the end results turned out ok. (ast still needs more work 
though.)

Best regards
Thomas

> 
>> Best regards
>> Thomas
>>
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/solomon/ssd130x-i2c.c b/drivers/gpu/drm/solomon/ssd130x-i2c.c
index b4eb2d64bf6e..8f89b89d553f 100644
--- a/drivers/gpu/drm/solomon/ssd130x-i2c.c
+++ b/drivers/gpu/drm/solomon/ssd130x-i2c.c
@@ -54,6 +54,7 @@  static void ssd130x_i2c_shutdown(struct i2c_client *client)
 }
 
 static const struct of_device_id ssd130x_of_match[] = {
+	/* ssd130x family */
 	{
 		.compatible = "sinowealth,sh1106",
 		.data = &ssd130x_variants[SH1106_ID],
diff --git a/drivers/gpu/drm/solomon/ssd130x-spi.c b/drivers/gpu/drm/solomon/ssd130x-spi.c
index 19ab4942cb33..257819bccbc8 100644
--- a/drivers/gpu/drm/solomon/ssd130x-spi.c
+++ b/drivers/gpu/drm/solomon/ssd130x-spi.c
@@ -108,6 +108,7 @@  static void ssd130x_spi_shutdown(struct spi_device *spi)
 }
 
 static const struct of_device_id ssd130x_of_match[] = {
+	/* ssd130x family */
 	{
 		.compatible = "sinowealth,sh1106",
 		.data = &ssd130x_variants[SH1106_ID],
@@ -142,6 +143,7 @@  MODULE_DEVICE_TABLE(of, ssd130x_of_match);
  * not be needed for this driver to match the registered SPI devices.
  */
 static const struct spi_device_id ssd130x_spi_table[] = {
+	/* ssd130x family */
 	{ "sh1106",  SH1106_ID },
 	{ "ssd1305", SSD1305_ID },
 	{ "ssd1306", SSD1306_ID },
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 2852cddb098b..08885c33e1a6 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -96,6 +96,12 @@ 
 
 #define MAX_CONTRAST 255
 
+enum ssd130x_family_ids {
+	SSD130X_FAMILY
+};
+
+static const struct ssd130x_funcs ssd130x_family_funcs[];
+
 const struct ssd130x_deviceinfo ssd130x_variants[] = {
 	[SH1106_ID] = {
 		.default_vcomh = 0x40,
@@ -104,6 +110,7 @@  const struct ssd130x_deviceinfo ssd130x_variants[] = {
 		.default_width = 132,
 		.default_height = 64,
 		.page_mode_only = 1,
+		.funcs = &ssd130x_family_funcs[SSD130X_FAMILY],
 	},
 	[SSD1305_ID] = {
 		.default_vcomh = 0x34,
@@ -111,6 +118,7 @@  const struct ssd130x_deviceinfo ssd130x_variants[] = {
 		.default_dclk_frq = 7,
 		.default_width = 132,
 		.default_height = 64,
+		.funcs = &ssd130x_family_funcs[SSD130X_FAMILY],
 	},
 	[SSD1306_ID] = {
 		.default_vcomh = 0x20,
@@ -119,6 +127,7 @@  const struct ssd130x_deviceinfo ssd130x_variants[] = {
 		.need_chargepump = 1,
 		.default_width = 128,
 		.default_height = 64,
+		.funcs = &ssd130x_family_funcs[SSD130X_FAMILY],
 	},
 	[SSD1307_ID] = {
 		.default_vcomh = 0x20,
@@ -127,6 +136,7 @@  const struct ssd130x_deviceinfo ssd130x_variants[] = {
 		.need_pwm = 1,
 		.default_width = 128,
 		.default_height = 39,
+		.funcs = &ssd130x_family_funcs[SSD130X_FAMILY],
 	},
 	[SSD1309_ID] = {
 		.default_vcomh = 0x34,
@@ -134,6 +144,7 @@  const struct ssd130x_deviceinfo ssd130x_variants[] = {
 		.default_dclk_frq = 10,
 		.default_width = 128,
 		.default_height = 64,
+		.funcs = &ssd130x_family_funcs[SSD130X_FAMILY],
 	}
 };
 EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X);
@@ -453,6 +464,33 @@  static int ssd130x_init(struct ssd130x_device *ssd130x)
 				 SSD130X_SET_ADDRESS_MODE_HORIZONTAL);
 }
 
+static int ssd130x_set_buffer_sizes(struct ssd130x_device *ssd130x)
+{
+	unsigned int buffer_pitch;
+	unsigned int pages = DIV_ROUND_UP(ssd130x->height, SSD130X_PAGE_HEIGHT);
+
+	ssd130x->data_array_size = ssd130x->width * pages;
+
+	ssd130x->buffer_fi = drm_format_info(DRM_FORMAT_R1);
+
+	if (!ssd130x->buffer_fi)
+		return -EINVAL;
+
+	buffer_pitch = drm_format_info_min_pitch(ssd130x->buffer_fi, 0, ssd130x->width);
+	ssd130x->buffer_size = buffer_pitch * ssd130x->height;
+
+	return 0;
+}
+
+static void ssd130x_align_rect(struct ssd130x_device *ssd130x,
+			       struct drm_rect *rect)
+{
+	/* Align y to display page boundaries */
+	rect->y1 = round_down(rect->y1, SSD130X_PAGE_HEIGHT);
+	rect->y2 = min_t(unsigned int, round_up(rect->y2, SSD130X_PAGE_HEIGHT),
+			 ssd130x->height);
+}
+
 static int ssd130x_update_rect(struct ssd130x_device *ssd130x,
 			       struct drm_rect *rect, u8 *buf,
 			       u8 *data_array)
@@ -600,34 +638,43 @@  static void ssd130x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
 	}
 }
 
+static const struct ssd130x_funcs ssd130x_family_funcs[] = {
+	[SSD130X_FAMILY] = {
+		.init = ssd130x_init,
+		.set_buffer_sizes = ssd130x_set_buffer_sizes,
+		.align_rect = ssd130x_align_rect,
+		.update_rect = ssd130x_update_rect,
+		.clear_screen = ssd130x_clear_screen,
+		.fmt_convert = drm_fb_xrgb8888_to_mono,
+	},
+};
+
 static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb,
 				const struct iosys_map *vmap,
-				struct drm_rect *rect,
-				u8 *buf, u8 *data_array)
+				struct drm_rect *rect, u8 *buf,
+				const struct drm_format_info *fi,
+				u8 *data_array)
 {
 	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
+	const struct ssd130x_funcs *ssd130x_funcs = ssd130x->device_info->funcs;
 	struct iosys_map dst;
 	unsigned int dst_pitch;
 	int ret = 0;
 
-	/* Align y to display page boundaries */
-	rect->y1 = round_down(rect->y1, SSD130X_PAGE_HEIGHT);
-	rect->y2 = min_t(unsigned int, round_up(rect->y2, SSD130X_PAGE_HEIGHT), ssd130x->height);
+	ssd130x_funcs->align_rect(ssd130x, rect);
 
-	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
+	dst_pitch = drm_format_info_min_pitch(fi, 0, drm_rect_width(rect));
 
 	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
 	if (ret)
 		return ret;
 
 	iosys_map_set_vaddr(&dst, buf);
-	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect);
+	ssd130x_funcs->fmt_convert(&dst, &dst_pitch, vmap, fb, rect);
 
 	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 
-	ssd130x_update_rect(ssd130x, rect, buf, data_array);
-
-	return ret;
+	return ssd130x_funcs->update_rect(ssd130x, rect, buf, data_array);
 }
 
 static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
@@ -639,8 +686,6 @@  static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
 	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
 	struct drm_crtc *crtc = plane_state->crtc;
 	struct drm_crtc_state *crtc_state;
-	const struct drm_format_info *fi;
-	unsigned int pitch;
 	int ret;
 
 	if (!crtc)
@@ -654,13 +699,7 @@  static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
 	if (ret)
 		return ret;
 
-	fi = drm_format_info(DRM_FORMAT_R1);
-	if (!fi)
-		return -EINVAL;
-
-	pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
-
-	ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
+	ssd130x_state->buffer = kzalloc(ssd130x->buffer_size, GFP_KERNEL);
 	if (!ssd130x_state->buffer)
 		return -ENOMEM;
 
@@ -679,6 +718,7 @@  static void ssd130x_primary_plane_atomic_update(struct drm_plane *plane,
 	struct drm_framebuffer *fb = plane_state->fb;
 	struct drm_atomic_helper_damage_iter iter;
 	struct drm_device *drm = plane->dev;
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
 	struct drm_rect dst_clip;
 	struct drm_rect damage;
 	int idx;
@@ -695,6 +735,7 @@  static void ssd130x_primary_plane_atomic_update(struct drm_plane *plane,
 
 		ssd130x_fb_blit_rect(fb, &shadow_plane_state->data[0], &dst_clip,
 				     ssd130x_plane_state->buffer,
+				     ssd130x->buffer_fi,
 				     ssd130x_crtc_state->data_array);
 	}
 
@@ -706,6 +747,7 @@  static void ssd130x_primary_plane_atomic_disable(struct drm_plane *plane,
 {
 	struct drm_device *drm = plane->dev;
 	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
+	const struct ssd130x_funcs *ssd130x_funcs = ssd130x->device_info->funcs;
 	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
 	struct drm_crtc_state *crtc_state;
 	struct ssd130x_crtc_state *ssd130x_crtc_state;
@@ -720,7 +762,7 @@  static void ssd130x_primary_plane_atomic_disable(struct drm_plane *plane,
 	if (!drm_dev_enter(drm, &idx))
 		return;
 
-	ssd130x_clear_screen(ssd130x, ssd130x_crtc_state->data_array);
+	ssd130x_funcs->clear_screen(ssd130x, ssd130x_crtc_state->data_array);
 
 	drm_dev_exit(idx);
 }
@@ -814,14 +856,13 @@  static int ssd130x_crtc_atomic_check(struct drm_crtc *crtc,
 	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
 	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 	struct ssd130x_crtc_state *ssd130x_state = to_ssd130x_crtc_state(crtc_state);
-	unsigned int pages = DIV_ROUND_UP(ssd130x->height, SSD130X_PAGE_HEIGHT);
 	int ret;
 
 	ret = drm_crtc_helper_atomic_check(crtc, state);
 	if (ret)
 		return ret;
 
-	ssd130x_state->data_array = kmalloc(ssd130x->width * pages, GFP_KERNEL);
+	ssd130x_state->data_array = kmalloc(ssd130x->data_array_size, GFP_KERNEL);
 	if (!ssd130x_state->data_array)
 		return -ENOMEM;
 
@@ -899,13 +940,14 @@  static void ssd130x_encoder_atomic_enable(struct drm_encoder *encoder,
 {
 	struct drm_device *drm = encoder->dev;
 	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
+	const struct ssd130x_funcs *ssd130x_funcs = ssd130x->device_info->funcs;
 	int ret;
 
 	ret = ssd130x_power_on(ssd130x);
 	if (ret)
 		return;
 
-	ret = ssd130x_init(ssd130x);
+	ret = ssd130x_funcs->init(ssd130x);
 	if (ret)
 		goto power_off;
 
@@ -1191,6 +1233,7 @@  static int ssd130x_get_resources(struct ssd130x_device *ssd130x)
 
 struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
 {
+	const struct ssd130x_funcs *ssd130x_funcs;
 	struct ssd130x_device *ssd130x;
 	struct backlight_device *bl;
 	struct drm_device *drm;
@@ -1213,6 +1256,12 @@  struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
 
 	ssd130x_parse_properties(ssd130x);
 
+	ssd130x_funcs = ssd130x->device_info->funcs;
+
+	ret = ssd130x_funcs->set_buffer_sizes(ssd130x);
+	if (ret)
+		return ERR_PTR(ret);
+
 	ret = ssd130x_get_resources(ssd130x);
 	if (ret)
 		return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h
index bbe374453605..5af251676587 100644
--- a/drivers/gpu/drm/solomon/ssd130x.h
+++ b/drivers/gpu/drm/solomon/ssd130x.h
@@ -20,11 +20,13 @@ 
 #include <drm/drm_plane_helper.h>
 
 #include <linux/regmap.h>
+#include <linux/iosys-map.h>
 
 #define SSD130X_DATA				0x40
 #define SSD130X_COMMAND				0x80
 
 enum ssd130x_variants {
+	/* ssd130x family */
 	SH1106_ID,
 	SSD1305_ID,
 	SSD1306_ID,
@@ -42,6 +44,9 @@  struct ssd130x_deviceinfo {
 	bool need_pwm;
 	bool need_chargepump;
 	bool page_mode_only;
+
+	/* Chip family specific operations */
+	const struct ssd130x_funcs *funcs;
 };
 
 struct ssd130x_device {
@@ -76,6 +81,12 @@  struct ssd130x_device {
 	u32 col_offset;
 	u32 prechargep1;
 	u32 prechargep2;
+	/* HW format buffer size */
+	u32 data_array_size;
+	/* Intermediate buffer size */
+	u32 buffer_size;
+	/* Pixel format info for the intermediate buffer */
+	const struct drm_format_info *buffer_fi;
 
 	struct backlight_device *bl_dev;
 	struct pwm_device *pwm;
@@ -90,6 +101,19 @@  struct ssd130x_device {
 	u8 page_end;
 };
 
+struct ssd130x_funcs {
+	int (*init)(struct ssd130x_device *ssd130x);
+	int (*set_buffer_sizes)(struct ssd130x_device *ssd130x);
+	void (*align_rect)(struct ssd130x_device *ssd130x, struct drm_rect *rect);
+	int (*update_rect)(struct ssd130x_device *ssd130x, struct drm_rect *rect,
+			   u8 *buf, u8 *data_array);
+	void (*clear_screen)(struct ssd130x_device *ssd130x,
+			     u8 *data_array);
+	void (*fmt_convert)(struct iosys_map *dst, const unsigned int *dst_pitch,
+			    const struct iosys_map *src, const struct drm_framebuffer *fb,
+			    const struct drm_rect *clip);
+};
+
 extern const struct ssd130x_deviceinfo ssd130x_variants[];
 
 struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap);