[PATCH v2 0/4] drm/probe-helper, modes: Helpers for single-mode displays

[PATCH v2 0/4] drm/probe-helper, modes: Helpers for single-mode displays

[Thomas Zimmermann]

This patchset moves code from simpledrm to probe and display-mode
helpers. The new functions will be useful for the upcoming driver
for PowerPC displays. [1] Where possible, existing drivers are
being converted to use them.

v2:
	* replace 'static' and 'hw' naming with 'fixed'
	* use u32 for 4CC codes (Sam)
	* print a warning if not all formats can be used (Sam)
	* comment fixes (Sam)

[1] https://patchwork.freedesktop.org/series/106538/

Thomas Zimmermann (4):
  drm/probe-helper: Add drm_connector_helper_get_modes_fixed()
  drm/probe-helper: Add drm_crtc_helper_mode_valid_fixed()
  drm/modes: Add initializer macro DRM_MODE_INIT()
  drm/format-helper: Add drm_fb_build_fourcc_list() helper

 drivers/gpu/drm/drm_format_helper.c          | 103 +++++++++++++++++++
 drivers/gpu/drm/drm_mipi_dbi.c               |  38 +++----
 drivers/gpu/drm/drm_probe_helper.c           |  65 ++++++++++++
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c |   1 +
 drivers/gpu/drm/tiny/hx8357d.c               |   1 +
 drivers/gpu/drm/tiny/ili9163.c               |   1 +
 drivers/gpu/drm/tiny/ili9341.c               |   1 +
 drivers/gpu/drm/tiny/ili9486.c               |   1 +
 drivers/gpu/drm/tiny/mi0283qt.c              |   1 +
 drivers/gpu/drm/tiny/panel-mipi-dbi.c        |   1 +
 drivers/gpu/drm/tiny/repaper.c               |  26 ++---
 drivers/gpu/drm/tiny/simpledrm.c             |  96 +++--------------
 drivers/gpu/drm/tiny/st7735r.c               |   1 +
 include/drm/drm_format_helper.h              |  11 +-
 include/drm/drm_mipi_dbi.h                   |   2 +
 include/drm/drm_modes.h                      |  35 ++++++-
 include/drm/drm_probe_helper.h               |   9 +-
 17 files changed, 271 insertions(+), 122 deletions(-)

-- 
2.37.1

[PATCH v2 1/4] drm/probe-helper: Add drm_connector_helper_get_modes_fixed()

[Thomas Zimmermann]

Add drm_connector_helper_get_modes_fixed(), which duplicates a single
display mode for a connector. Convert drivers.

v2:
	* rename 'static' and 'hw' to 'fixed' everywhere
	* fix typo 'there' to 'their' (Sam)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/drm_mipi_dbi.c     | 20 +--------------
 drivers/gpu/drm/drm_probe_helper.c | 40 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/tiny/repaper.c     | 16 +-----------
 drivers/gpu/drm/tiny/simpledrm.c   | 18 +-------------
 include/drm/drm_probe_helper.h     |  3 +++
 5 files changed, 46 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index 84abc3920b57..de2a5be67415 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -415,26 +415,8 @@ EXPORT_SYMBOL(mipi_dbi_pipe_disable);
 static int mipi_dbi_connector_get_modes(struct drm_connector *connector)
 {
 	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(connector->dev);
-	struct drm_display_mode *mode;
 
-	mode = drm_mode_duplicate(connector->dev, &dbidev->mode);
-	if (!mode) {
-		DRM_ERROR("Failed to duplicate mode\n");
-		return 0;
-	}
-
-	if (mode->name[0] == '\0')
-		drm_mode_set_name(mode);
-
-	mode->type |= DRM_MODE_TYPE_PREFERRED;
-	drm_mode_probed_add(connector, mode);
-
-	if (mode->width_mm) {
-		connector->display_info.width_mm = mode->width_mm;
-		connector->display_info.height_mm = mode->height_mm;
-	}
-
-	return 1;
+	return drm_connector_helper_get_modes_fixed(connector, &dbidev->mode);
 }
 
 static const struct drm_connector_helper_funcs mipi_dbi_connector_hfuncs = {
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index bb427c5a4f1f..818150a1b3b0 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -1050,6 +1050,46 @@ int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_connector_helper_get_modes_from_ddc);
 
+/**
+ * drm_connector_helper_get_modes_fixed - Duplicates a display mode for a connector
+ * @connector: the connector
+ * @fixed_mode: the display hardware's mode
+ *
+ * This function duplicates a display modes for a connector. Drivers for hardware
+ * that only supports a single fixed mode can use this function in their connector's
+ * get_modes helper.
+ *
+ * Returns:
+ * The number of created modes.
+ */
+int drm_connector_helper_get_modes_fixed(struct drm_connector *connector,
+					 const struct drm_display_mode *fixed_mode)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(dev, fixed_mode);
+	if (!mode) {
+		drm_err(dev, "Failed to duplicate mode " DRM_MODE_FMT "\n",
+			DRM_MODE_ARG(fixed_mode));
+		return 0;
+	}
+
+	if (mode->name[0] == '\0')
+		drm_mode_set_name(mode);
+
+	mode->type |= DRM_MODE_TYPE_PREFERRED;
+	drm_mode_probed_add(connector, mode);
+
+	if (mode->width_mm)
+		connector->display_info.width_mm = mode->width_mm;
+	if (mode->height_mm)
+		connector->display_info.height_mm = mode->height_mm;
+
+	return 1;
+}
+EXPORT_SYMBOL(drm_connector_helper_get_modes_fixed);
+
 /**
  * drm_connector_helper_get_modes - Read EDID and update connector.
  * @connector: The connector
diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
index c4c1be3ac0b8..01fbd00411fc 100644
--- a/drivers/gpu/drm/tiny/repaper.c
+++ b/drivers/gpu/drm/tiny/repaper.c
@@ -839,22 +839,8 @@ static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
 static int repaper_connector_get_modes(struct drm_connector *connector)
 {
 	struct repaper_epd *epd = drm_to_epd(connector->dev);
-	struct drm_display_mode *mode;
 
-	mode = drm_mode_duplicate(connector->dev, epd->mode);
-	if (!mode) {
-		DRM_ERROR("Failed to duplicate mode\n");
-		return 0;
-	}
-
-	drm_mode_set_name(mode);
-	mode->type |= DRM_MODE_TYPE_PREFERRED;
-	drm_mode_probed_add(connector, mode);
-
-	connector->display_info.width_mm = mode->width_mm;
-	connector->display_info.height_mm = mode->height_mm;
-
-	return 1;
+	return drm_connector_helper_get_modes_fixed(connector, epd->mode);
 }
 
 static const struct drm_connector_helper_funcs repaper_connector_hfuncs = {
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index a81f91814595..c79576844ec0 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -620,24 +620,8 @@ static const struct drm_encoder_funcs simpledrm_encoder_funcs = {
 static int simpledrm_connector_helper_get_modes(struct drm_connector *connector)
 {
 	struct simpledrm_device *sdev = simpledrm_device_of_dev(connector->dev);
-	struct drm_display_mode *mode;
 
-	mode = drm_mode_duplicate(connector->dev, &sdev->mode);
-	if (!mode)
-		return 0;
-
-	if (mode->name[0] == '\0')
-		drm_mode_set_name(mode);
-
-	mode->type |= DRM_MODE_TYPE_PREFERRED;
-	drm_mode_probed_add(connector, mode);
-
-	if (mode->width_mm)
-		connector->display_info.width_mm = mode->width_mm;
-	if (mode->height_mm)
-		connector->display_info.height_mm = mode->height_mm;
-
-	return 1;
+	return drm_connector_helper_get_modes_fixed(connector, &sdev->mode);
 }
 
 static const struct drm_connector_helper_funcs simpledrm_connector_helper_funcs = {
diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
index 8075e02aa865..d261838a7790 100644
--- a/include/drm/drm_probe_helper.h
+++ b/include/drm/drm_probe_helper.h
@@ -7,6 +7,7 @@
 
 struct drm_connector;
 struct drm_device;
+struct drm_display_mode;
 struct drm_modeset_acquire_ctx;
 
 int drm_helper_probe_single_connector_modes(struct drm_connector
@@ -27,6 +28,8 @@ void drm_kms_helper_poll_enable(struct drm_device *dev);
 bool drm_kms_helper_is_poll_worker(void);
 
 int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector);
+int drm_connector_helper_get_modes_fixed(struct drm_connector *connector,
+					 const struct drm_display_mode *fixed_mode);
 int drm_connector_helper_get_modes(struct drm_connector *connector);
 
 #endif
-- 
2.37.1

[PATCH v2 2/4] drm/probe-helper: Add drm_crtc_helper_mode_valid_fixed()

[Thomas Zimmermann]

Add drm_crtc_helper_mode_valid_fixed(), which validates a given mode
against a display hardware's mode. Convert simpledrm and use it in a
few other drivers with static modes.

v2:
	* rename 'static' and 'hw' to 'fixed' everywhere

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/drm_mipi_dbi.c               | 18 ++++++++++++++
 drivers/gpu/drm/drm_probe_helper.c           | 25 ++++++++++++++++++++
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c |  1 +
 drivers/gpu/drm/tiny/hx8357d.c               |  1 +
 drivers/gpu/drm/tiny/ili9163.c               |  1 +
 drivers/gpu/drm/tiny/ili9341.c               |  1 +
 drivers/gpu/drm/tiny/ili9486.c               |  1 +
 drivers/gpu/drm/tiny/mi0283qt.c              |  1 +
 drivers/gpu/drm/tiny/panel-mipi-dbi.c        |  1 +
 drivers/gpu/drm/tiny/repaper.c               | 10 ++++++++
 drivers/gpu/drm/tiny/simpledrm.c             | 10 +-------
 drivers/gpu/drm/tiny/st7735r.c               |  1 +
 include/drm/drm_mipi_dbi.h                   |  2 ++
 include/drm/drm_probe_helper.h               |  8 +++++--
 14 files changed, 70 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index de2a5be67415..a6ac56580876 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -309,6 +309,24 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
 	drm_dev_exit(idx);
 }
 
+/**
+ * mipi_dbi_pipe_mode_valid - MIPI DBI mode-valid helper
+ * @pipe: Simple display pipe
+ * @mode: The mode to test
+ *
+ * This function validates a given display mode against the MIPI DBI's hardware
+ * display. Drivers can use this as their &drm_simple_display_pipe_funcs->mode_valid
+ * callback.
+ */
+enum drm_mode_status mipi_dbi_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
+					      const struct drm_display_mode *mode)
+{
+	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
+
+	return drm_crtc_helper_mode_valid_fixed(&pipe->crtc, mode, &dbidev->mode);
+}
+EXPORT_SYMBOL(mipi_dbi_pipe_mode_valid);
+
 /**
  * mipi_dbi_pipe_update - Display pipe update helper
  * @pipe: Simple display pipe
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 818150a1b3b0..8956b367e12f 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -1014,6 +1014,31 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_helper_hpd_irq_event);
 
+/**
+ * drm_crtc_helper_mode_valid_fixed - Validates a display mode
+ * @crtc: the crtc
+ * @mode: the mode to validate
+ * @fixed_mode: the display hardware's mode
+ *
+ * Returns:
+ * MODE_OK on success, or another mode-status code otherwise.
+ */
+enum drm_mode_status drm_crtc_helper_mode_valid_fixed(struct drm_crtc *crtc,
+						      const struct drm_display_mode *mode,
+						      const struct drm_display_mode *fixed_mode)
+{
+
+	if (mode->hdisplay != fixed_mode->hdisplay && mode->vdisplay != fixed_mode->vdisplay)
+		return MODE_ONE_SIZE;
+	else if (mode->hdisplay != fixed_mode->hdisplay)
+		return MODE_ONE_WIDTH;
+	else if (mode->vdisplay != fixed_mode->vdisplay)
+		return MODE_ONE_HEIGHT;
+
+	return MODE_OK;
+}
+EXPORT_SYMBOL(drm_crtc_helper_mode_valid_fixed);
+
 /**
  * drm_connector_helper_get_modes_from_ddc - Updates the connector's EDID
  *                                           property from the connector's
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
index 7da09e34385d..39dc40cf681f 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -576,6 +576,7 @@ static void ili9341_dbi_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs ili9341_dbi_funcs = {
+	.mode_valid = mipi_dbi_pipe_mode_valid,
 	.enable = ili9341_dbi_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = mipi_dbi_pipe_update,
diff --git a/drivers/gpu/drm/tiny/hx8357d.c b/drivers/gpu/drm/tiny/hx8357d.c
index 57f229a785bf..48c24aa8c28a 100644
--- a/drivers/gpu/drm/tiny/hx8357d.c
+++ b/drivers/gpu/drm/tiny/hx8357d.c
@@ -181,6 +181,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = {
+	.mode_valid = mipi_dbi_pipe_mode_valid,
 	.enable = yx240qv29_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = mipi_dbi_pipe_update,
diff --git a/drivers/gpu/drm/tiny/ili9163.c b/drivers/gpu/drm/tiny/ili9163.c
index 86439e50e304..9a1a5943bee0 100644
--- a/drivers/gpu/drm/tiny/ili9163.c
+++ b/drivers/gpu/drm/tiny/ili9163.c
@@ -100,6 +100,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs ili9163_pipe_funcs = {
+	.mode_valid = mipi_dbi_pipe_mode_valid,
 	.enable = yx240qv29_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = mipi_dbi_pipe_update,
diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c
index b8826a0b086b..69b265e78096 100644
--- a/drivers/gpu/drm/tiny/ili9341.c
+++ b/drivers/gpu/drm/tiny/ili9341.c
@@ -137,6 +137,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = {
+	.mode_valid = mipi_dbi_pipe_mode_valid,
 	.enable = yx240qv29_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = mipi_dbi_pipe_update,
diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c
index a5b433a8e0d8..c80028bb1d11 100644
--- a/drivers/gpu/drm/tiny/ili9486.c
+++ b/drivers/gpu/drm/tiny/ili9486.c
@@ -150,6 +150,7 @@ static void waveshare_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs waveshare_pipe_funcs = {
+	.mode_valid = mipi_dbi_pipe_mode_valid,
 	.enable = waveshare_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = mipi_dbi_pipe_update,
diff --git a/drivers/gpu/drm/tiny/mi0283qt.c b/drivers/gpu/drm/tiny/mi0283qt.c
index 27f1bd4da2f4..bc522fb3d94d 100644
--- a/drivers/gpu/drm/tiny/mi0283qt.c
+++ b/drivers/gpu/drm/tiny/mi0283qt.c
@@ -141,6 +141,7 @@ static void mi0283qt_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
+	.mode_valid = mipi_dbi_pipe_mode_valid,
 	.enable = mi0283qt_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = mipi_dbi_pipe_update,
diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
index a76fefa8adbc..955a61d628e7 100644
--- a/drivers/gpu/drm/tiny/panel-mipi-dbi.c
+++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
@@ -212,6 +212,7 @@ static void panel_mipi_dbi_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs panel_mipi_dbi_pipe_funcs = {
+	.mode_valid = mipi_dbi_pipe_mode_valid,
 	.enable = panel_mipi_dbi_enable,
 	.disable = mipi_dbi_pipe_disable,
 	.update = mipi_dbi_pipe_update,
diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
index 01fbd00411fc..e62f4d16b2c6 100644
--- a/drivers/gpu/drm/tiny/repaper.c
+++ b/drivers/gpu/drm/tiny/repaper.c
@@ -621,6 +621,15 @@ static void power_off(struct repaper_epd *epd)
 	gpiod_set_value_cansleep(epd->discharge, 0);
 }
 
+static enum drm_mode_status repaper_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
+						    const struct drm_display_mode *mode)
+{
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct repaper_epd *epd = drm_to_epd(crtc->dev);
+
+	return drm_crtc_helper_mode_valid_fixed(crtc, mode, epd->mode);
+}
+
 static void repaper_pipe_enable(struct drm_simple_display_pipe *pipe,
 				struct drm_crtc_state *crtc_state,
 				struct drm_plane_state *plane_state)
@@ -831,6 +840,7 @@ static void repaper_pipe_update(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
+	.mode_valid = repaper_pipe_mode_valid,
 	.enable = repaper_pipe_enable,
 	.disable = repaper_pipe_disable,
 	.update = repaper_pipe_update,
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index c79576844ec0..5bed4d564104 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -570,15 +570,7 @@ static enum drm_mode_status simpledrm_crtc_helper_mode_valid(struct drm_crtc *cr
 {
 	struct simpledrm_device *sdev = simpledrm_device_of_dev(crtc->dev);
 
-	if (mode->hdisplay != sdev->mode.hdisplay &&
-	    mode->vdisplay != sdev->mode.vdisplay)
-		return MODE_ONE_SIZE;
-	else if (mode->hdisplay != sdev->mode.hdisplay)
-		return MODE_ONE_WIDTH;
-	else if (mode->vdisplay != sdev->mode.vdisplay)
-		return MODE_ONE_HEIGHT;
-
-	return MODE_OK;
+	return drm_crtc_helper_mode_valid_fixed(crtc, mode, &sdev->mode);
 }
 
 static int simpledrm_crtc_helper_atomic_check(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c
index d2042a0f02dd..c36ba08acda1 100644
--- a/drivers/gpu/drm/tiny/st7735r.c
+++ b/drivers/gpu/drm/tiny/st7735r.c
@@ -133,6 +133,7 @@ static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs st7735r_pipe_funcs = {
+	.mode_valid	= mipi_dbi_pipe_mode_valid,
 	.enable		= st7735r_pipe_enable,
 	.disable	= mipi_dbi_pipe_disable,
 	.update		= mipi_dbi_pipe_update,
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index dad2f187b64b..14eaecb1825c 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -155,6 +155,8 @@ int mipi_dbi_dev_init_with_formats(struct mipi_dbi_dev *dbidev,
 int mipi_dbi_dev_init(struct mipi_dbi_dev *dbidev,
 		      const struct drm_simple_display_pipe_funcs *funcs,
 		      const struct drm_display_mode *mode, unsigned int rotation);
+enum drm_mode_status mipi_dbi_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
+					      const struct drm_display_mode *mode);
 void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
 			  struct drm_plane_state *old_state);
 void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
index d261838a7790..5880daa14624 100644
--- a/include/drm/drm_probe_helper.h
+++ b/include/drm/drm_probe_helper.h
@@ -3,11 +3,11 @@
 #ifndef __DRM_PROBE_HELPER_H__
 #define __DRM_PROBE_HELPER_H__
 
-#include <linux/types.h>
+#include <drm/drm_modes.h>
 
 struct drm_connector;
+struct drm_crtc;
 struct drm_device;
-struct drm_display_mode;
 struct drm_modeset_acquire_ctx;
 
 int drm_helper_probe_single_connector_modes(struct drm_connector
@@ -27,6 +27,10 @@ void drm_kms_helper_poll_disable(struct drm_device *dev);
 void drm_kms_helper_poll_enable(struct drm_device *dev);
 bool drm_kms_helper_is_poll_worker(void);
 
+enum drm_mode_status drm_crtc_helper_mode_valid_fixed(struct drm_crtc *crtc,
+						      const struct drm_display_mode *mode,
+						      const struct drm_display_mode *fixed_mode);
+
 int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector);
 int drm_connector_helper_get_modes_fixed(struct drm_connector *connector,
 					 const struct drm_display_mode *fixed_mode);
-- 
2.37.1

[PATCH v2 3/4] drm/modes: Add initializer macro DRM_MODE_INIT()

[Thomas Zimmermann]

The macro DRM_MODE_INIT() initializes an instance of
struct drm_display_mode with typical parameters. Convert simpledrm
and also update the macro DRM_SIMPLE_MODE().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/tiny/simpledrm.c | 23 ++++++++-------------
 include/drm/drm_modes.h          | 35 +++++++++++++++++++++++++++-----
 2 files changed, 39 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 5bed4d564104..404290760c60 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -30,16 +30,6 @@
 #define DRIVER_MAJOR	1
 #define DRIVER_MINOR	0
 
-/*
- * Assume a monitor resolution of 96 dpi to
- * get a somewhat reasonable screen size.
- */
-#define RES_MM(d)	\
-	(((d) * 254ul) / (96ul * 10ul))
-
-#define SIMPLEDRM_MODE(hd, vd)	\
-	DRM_SIMPLE_MODE(hd, vd, RES_MM(hd), RES_MM(vd))
-
 /*
  * Helpers for simplefb
  */
@@ -641,10 +631,15 @@ static const struct drm_mode_config_funcs simpledrm_mode_config_funcs = {
 static struct drm_display_mode simpledrm_mode(unsigned int width,
 					      unsigned int height)
 {
-	struct drm_display_mode mode = { SIMPLEDRM_MODE(width, height) };
-
-	mode.clock = mode.hdisplay * mode.vdisplay * 60 / 1000 /* kHz */;
-	drm_mode_set_name(&mode);
+	/*
+	 * Assume a monitor resolution of 96 dpi to
+	 * get a somewhat reasonable screen size.
+	 */
+	const struct drm_display_mode mode = {
+		DRM_MODE_INIT(60, width, height,
+			      DRM_MODE_RES_MM(width, 96ul),
+			      DRM_MODE_RES_MM(height, 96ul))
+	};
 
 	return mode;
 }
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index a80ae9639e96..bb932806f029 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -138,6 +138,35 @@ enum drm_mode_status {
 	.vsync_start = (vss), .vsync_end = (vse), .vtotal = (vt), \
 	.vscan = (vs), .flags = (f)
 
+/**
+ * DRM_MODE_RES_MM - Calculates the display size from resolution and DPI
+ * @res: The resolution in pixel
+ * @dpi: The number of dots per inch
+ */
+#define DRM_MODE_RES_MM(res, dpi)	\
+	(((res) * 254ul) / ((dpi) * 10ul))
+
+#define __DRM_MODE_INIT(pix, hd, vd, hd_mm, vd_mm) \
+	.type = DRM_MODE_TYPE_DRIVER, .clock = (pix), \
+	.hdisplay = (hd), .hsync_start = (hd), .hsync_end = (hd), \
+	.htotal = (hd), .vdisplay = (vd), .vsync_start = (vd), \
+	.vsync_end = (vd), .vtotal = (vd), .width_mm = (hd_mm), \
+	.height_mm = (vd_mm)
+
+/**
+ * DRM_SIMPLE_MODE_INIT - Initialize display mode
+ * @hz: Vertical refresh rate in Hertz
+ * @hd: Horizontal resolution, width
+ * @vd: Vertical resolution, height
+ * @hd_mm: Display width in millimeters
+ * @vd_mm: Display height in millimeters
+ *
+ * This macro initializes a &drm_display_mode that contains information about
+ * refresh rate, resolution and physical size.
+ */
+#define DRM_MODE_INIT(hz, hd, vd, hd_mm, vd_mm) \
+	__DRM_MODE_INIT((hd) * (vd) * (hz) / 1000 /* kHz */, hd, vd, hd_mm, vd_mm)
+
 /**
  * DRM_SIMPLE_MODE - Simple display mode
  * @hd: Horizontal resolution, width
@@ -149,11 +178,7 @@ enum drm_mode_status {
  * resolution and physical size.
  */
 #define DRM_SIMPLE_MODE(hd, vd, hd_mm, vd_mm) \
-	.type = DRM_MODE_TYPE_DRIVER, .clock = 1 /* pass validation */, \
-	.hdisplay = (hd), .hsync_start = (hd), .hsync_end = (hd), \
-	.htotal = (hd), .vdisplay = (vd), .vsync_start = (vd), \
-	.vsync_end = (vd), .vtotal = (vd), .width_mm = (hd_mm), \
-	.height_mm = (vd_mm)
+	__DRM_MODE_INIT(1 /* pass validation */, hd, vd, hd_mm, vd_mm)
 
 #define CRTC_INTERLACE_HALVE_V	(1 << 0) /* halve V values for interlacing */
 #define CRTC_STEREO_DOUBLE	(1 << 1) /* adjust timings for stereo modes */
-- 
2.37.1

[PATCH v2 4/4] drm/format-helper: Add drm_fb_build_fourcc_list() helper

[Thomas Zimmermann]

Add drm_fb_build_fourcc_list() function that builds a list of supported
formats from native and emulated ones. Helpful for all drivers that do
format conversion as part of their plane updates. Update current caller.

v2:
	* use u32 instead of uint32_t (Sam)
	* print a warning if output array is too small (Sam)
	* comment fixes (Sam)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/drm_format_helper.c | 103 ++++++++++++++++++++++++++++
 drivers/gpu/drm/tiny/simpledrm.c    |  47 ++-----------
 include/drm/drm_format_helper.h     |  11 ++-
 3 files changed, 118 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index 56642816fdff..fe1db7dbda3f 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -793,3 +793,106 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
 	kfree(src32);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono);
+
+static bool is_listed_fourcc(const uint32_t *fourccs, size_t nfourccs, uint32_t fourcc)
+{
+	const uint32_t *fourccs_end = fourccs + nfourccs;
+
+	while (fourccs < fourccs_end) {
+		if (*fourccs == fourcc)
+			return true;
+		++fourccs;
+	}
+	return false;
+}
+
+/**
+ * drm_fb_build_fourcc_list - Filters a list of supported color formats against
+ *                            the device's native formats
+ * @dev: DRM device
+ * @native_fourccs: 4CC codes of natively supported color formats
+ * @native_nfourccs: The number of entries in @native_fourccs
+ * @extra_fourccs: 4CC codes of additionally supported color formats
+ * @extra_nfourccs: The number of entries in @extra_fourccs
+ * @fourccs_out: Returns 4CC codes of supported color formats
+ * @nfourccs_out: The number of available entries in @fourccs_out
+ *
+ * This function create a list of supported color format from natively
+ * supported formats and the emulated formats.
+ * At a minimum, most userspace programs expect at least support for
+ * XRGB8888 on the primary plane. Devices that have to emulate the
+ * format, and possibly others, can use drm_fb_build_fourcc_list() to
+ * create a list of supported color formats. The returned list can
+ * be handed over to drm_universal_plane_init() et al. Native formats
+ * will go before emulated formats. Other heuristics might be applied
+ * to optimize the order. Formats near the beginning of the list are
+ * usually preferred over formats near the end of the list.
+ *
+ * Returns:
+ * The number of color-formats 4CC codes returned in @fourccs_out.
+ */
+size_t drm_fb_build_fourcc_list(struct drm_device *dev,
+				const u32 *native_fourccs, size_t native_nfourccs,
+				const u32 *extra_fourccs, size_t extra_nfourccs,
+				u32 *fourccs_out, size_t nfourccs_out)
+{
+	u32 *fourccs = fourccs_out;
+	const u32 *fourccs_end = fourccs_out + nfourccs_out;
+	bool found_native = false;
+	size_t nfourccs, i;
+
+	/*
+	 * The device's native formats go first.
+	 */
+
+	nfourccs = min_t(size_t, native_nfourccs, nfourccs_out);
+
+	for (i = 0; i < nfourccs; ++i) {
+		u32 fourcc = native_fourccs[i];
+
+		drm_dbg_kms(dev, "adding native format %p4cc\n", &fourcc);
+
+		if (!found_native)
+			found_native = is_listed_fourcc(extra_fourccs, extra_nfourccs, fourcc);
+		*fourccs = fourcc;
+		++fourccs;
+	}
+
+	/*
+	 * The plane's atomic_update helper converts the framebuffer's color format
+	 * to a native format when copying to device memory.
+	 *
+	 * If there is not a single format supported by both, device and
+	 * driver, the native formats are likely not supported by the conversion
+	 * helpers. Therefore *only* support the native formats and add a
+	 * conversion helper ASAP.
+	 */
+	if (!found_native) {
+		drm_warn(dev, "Format conversion helpers required to add extra formats.\n");
+		goto out;
+	}
+
+	/*
+	 * The extra formats, emulated by the driver, go second.
+	 */
+
+	nfourccs = min_t(size_t, extra_nfourccs, fourccs_end - fourccs);
+
+	for (i = 0; i < nfourccs; ++i) {
+		u32 fourcc = extra_fourccs[i];
+
+		if (is_listed_fourcc(native_fourccs, native_nfourccs, fourcc))
+			continue; /* native formats already went first */
+		*fourccs = fourcc;
+		++fourccs;
+	}
+
+	if (nfourccs < extra_nfourccs) {
+		drm_warn(dev, "Format buffer too small. %zu trailing formats dropped.\n",
+			 extra_nfourccs - nfourccs);
+	}
+
+out:
+	return fourccs - fourccs_out;
+}
+EXPORT_SYMBOL(drm_fb_build_fourcc_list);
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 404290760c60..777ccd250871 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -644,45 +644,6 @@ static struct drm_display_mode simpledrm_mode(unsigned int width,
 	return mode;
 }
 
-static const uint32_t *simpledrm_device_formats(struct simpledrm_device *sdev,
-						size_t *nformats_out)
-{
-	struct drm_device *dev = &sdev->dev;
-	size_t i;
-
-	if (sdev->nformats)
-		goto out; /* don't rebuild list on recurring calls */
-
-	/* native format goes first */
-	sdev->formats[0] = sdev->format->format;
-	sdev->nformats = 1;
-
-	/* default formats go second */
-	for (i = 0; i < ARRAY_SIZE(simpledrm_primary_plane_formats); ++i) {
-		if (simpledrm_primary_plane_formats[i] == sdev->format->format)
-			continue; /* native format already went first */
-		sdev->formats[sdev->nformats] = simpledrm_primary_plane_formats[i];
-		sdev->nformats++;
-	}
-
-	/*
-	 * TODO: The simpledrm driver converts framebuffers to the native
-	 * format when copying them to device memory. If there are more
-	 * formats listed than supported by the driver, the native format
-	 * is not supported by the conversion helpers. Therefore *only*
-	 * support the native format and add a conversion helper ASAP.
-	 */
-	if (drm_WARN_ONCE(dev, i != sdev->nformats,
-			  "format conversion helpers required for %p4cc",
-			  &sdev->format->format)) {
-		sdev->nformats = 1;
-	}
-
-out:
-	*nformats_out = sdev->nformats;
-	return sdev->formats;
-}
-
 static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 							struct platform_device *pdev)
 {
@@ -699,7 +660,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
 	unsigned long max_width, max_height;
-	const uint32_t *formats;
 	size_t nformats;
 	int ret;
 
@@ -811,11 +771,14 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 
 	/* Primary plane */
 
-	formats = simpledrm_device_formats(sdev, &nformats);
+	nformats = drm_fb_build_fourcc_list(dev, &format->format, 1,
+					    simpledrm_primary_plane_formats,
+					    ARRAY_SIZE(simpledrm_primary_plane_formats),
+					    sdev->formats, ARRAY_SIZE(sdev->formats));
 
 	primary_plane = &sdev->primary_plane;
 	ret = drm_universal_plane_init(dev, primary_plane, 0, &simpledrm_primary_plane_funcs,
-				       formats, nformats,
+				       sdev->formats, nformats,
 				       simpledrm_primary_plane_format_modifiers,
 				       DRM_PLANE_TYPE_PRIMARY, NULL);
 	if (ret)
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index caa181194335..eb5c98cf82b8 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -6,11 +6,15 @@
 #ifndef __LINUX_DRM_FORMAT_HELPER_H
 #define __LINUX_DRM_FORMAT_HELPER_H
 
-struct iosys_map;
+#include <linux/types.h>
+
+struct drm_device;
 struct drm_format_info;
 struct drm_framebuffer;
 struct drm_rect;
 
+struct iosys_map;
+
 unsigned int drm_fb_clip_offset(unsigned int pitch, const struct drm_format_info *format,
 				const struct drm_rect *clip);
 
@@ -44,4 +48,9 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
 			     const struct iosys_map *src, const struct drm_framebuffer *fb,
 			     const struct drm_rect *clip);
 
+size_t drm_fb_build_fourcc_list(struct drm_device *dev,
+				const u32 *native_fourccs, size_t native_nfourccs,
+				const u32 *extra_fourccs, size_t extra_nfourccs,
+				u32 *fourccs_out, size_t nfourccs_out);
+
 #endif /* __LINUX_DRM_FORMAT_HELPER_H */
-- 
2.37.1

Re: [PATCH v2 4/4] drm/format-helper: Add drm_fb_build_fourcc_list() helper

[Sam Ravnborg]

Hi Thomas,

On Tue, Aug 16, 2022 at 03:48:53PM +0200, Thomas Zimmermann wrote:
> Add drm_fb_build_fourcc_list() function that builds a list of supported
> formats from native and emulated ones. Helpful for all drivers that do
> format conversion as part of their plane updates. Update current caller.
> 
> v2:
> 	* use u32 instead of uint32_t (Sam)
> 	* print a warning if output array is too small (Sam)
> 	* comment fixes (Sam)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> ---
>  drivers/gpu/drm/drm_format_helper.c | 103 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/tiny/simpledrm.c    |  47 ++-----------
>  include/drm/drm_format_helper.h     |  11 ++-
>  3 files changed, 118 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index 56642816fdff..fe1db7dbda3f 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -793,3 +793,106 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
>  	kfree(src32);
>  }
>  EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono);
> +
> +static bool is_listed_fourcc(const uint32_t *fourccs, size_t nfourccs, uint32_t fourcc)
> +{
> +	const uint32_t *fourccs_end = fourccs + nfourccs;
> +
> +	while (fourccs < fourccs_end) {
> +		if (*fourccs == fourcc)
> +			return true;
> +		++fourccs;
> +	}
> +	return false;
> +}
> +
> +/**
> + * drm_fb_build_fourcc_list - Filters a list of supported color formats against
> + *                            the device's native formats
> + * @dev: DRM device
> + * @native_fourccs: 4CC codes of natively supported color formats
> + * @native_nfourccs: The number of entries in @native_fourccs
> + * @extra_fourccs: 4CC codes of additionally supported color formats
> + * @extra_nfourccs: The number of entries in @extra_fourccs
> + * @fourccs_out: Returns 4CC codes of supported color formats
> + * @nfourccs_out: The number of available entries in @fourccs_out
> + *
> + * This function create a list of supported color format from natively
> + * supported formats and the emulated formats.
> + * At a minimum, most userspace programs expect at least support for
> + * XRGB8888 on the primary plane. Devices that have to emulate the
> + * format, and possibly others, can use drm_fb_build_fourcc_list() to
> + * create a list of supported color formats. The returned list can
> + * be handed over to drm_universal_plane_init() et al. Native formats
> + * will go before emulated formats. Other heuristics might be applied
> + * to optimize the order. Formats near the beginning of the list are
> + * usually preferred over formats near the end of the list.
> + *
> + * Returns:
> + * The number of color-formats 4CC codes returned in @fourccs_out.
> + */
> +size_t drm_fb_build_fourcc_list(struct drm_device *dev,
> +				const u32 *native_fourccs, size_t native_nfourccs,
> +				const u32 *extra_fourccs, size_t extra_nfourccs,
> +				u32 *fourccs_out, size_t nfourccs_out)
> +{
> +	u32 *fourccs = fourccs_out;
> +	const u32 *fourccs_end = fourccs_out + nfourccs_out;
> +	bool found_native = false;
> +	size_t nfourccs, i;
> +
> +	/*
> +	 * The device's native formats go first.
> +	 */
> +
> +	nfourccs = min_t(size_t, native_nfourccs, nfourccs_out);
If nfourccs < nfourccs_out then there is not enough room for the
formats. I know this is unlikely, but anyway.. Maybe warn here too?

I would use min() here, as the types are all compatible, so no need to
cast to size_t.

> +
> +	for (i = 0; i < nfourccs; ++i) {
> +		u32 fourcc = native_fourccs[i];
> +
> +		drm_dbg_kms(dev, "adding native format %p4cc\n", &fourcc);
> +
> +		if (!found_native)
> +			found_native = is_listed_fourcc(extra_fourccs, extra_nfourccs, fourcc);
> +		*fourccs = fourcc;
> +		++fourccs;
> +	}
> +
> +	/*
> +	 * The plane's atomic_update helper converts the framebuffer's color format
> +	 * to a native format when copying to device memory.
> +	 *
> +	 * If there is not a single format supported by both, device and
> +	 * driver, the native formats are likely not supported by the conversion
> +	 * helpers. Therefore *only* support the native formats and add a
> +	 * conversion helper ASAP.
> +	 */
> +	if (!found_native) {
> +		drm_warn(dev, "Format conversion helpers required to add extra formats.\n");
> +		goto out;
> +	}
> +
> +	/*
> +	 * The extra formats, emulated by the driver, go second.
> +	 */
> +
> +	nfourccs = min_t(size_t, extra_nfourccs, fourccs_end - fourccs);
fourccs_end is a pointer which is subtracted with fourccs * sizeof(fourccs_end)
So the result is a pointer. I do not think this does what you try to do.
(Or I need more coffee?)


Using min() should give a nice warning I think - so drop min_t to avoid
the cast.

> +
> +	for (i = 0; i < nfourccs; ++i) {
> +		u32 fourcc = extra_fourccs[i];
> +
> +		if (is_listed_fourcc(native_fourccs, native_nfourccs, fourcc))
> +			continue; /* native formats already went first */
> +		*fourccs = fourcc;
> +		++fourccs;
> +	}
> +
> +	if (nfourccs < extra_nfourccs) {
> +		drm_warn(dev, "Format buffer too small. %zu trailing formats dropped.\n",
> +			 extra_nfourccs - nfourccs);
> +	}
Move this up right after the min()?

	Sam
> +
> +out:
> +	return fourccs - fourccs_out;
> +}
> +EXPORT_SYMBOL(drm_fb_build_fourcc_list);
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 404290760c60..777ccd250871 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -644,45 +644,6 @@ static struct drm_display_mode simpledrm_mode(unsigned int width,
>  	return mode;
>  }
>  
> -static const uint32_t *simpledrm_device_formats(struct simpledrm_device *sdev,
> -						size_t *nformats_out)
> -{
> -	struct drm_device *dev = &sdev->dev;
> -	size_t i;
> -
> -	if (sdev->nformats)
> -		goto out; /* don't rebuild list on recurring calls */
> -
> -	/* native format goes first */
> -	sdev->formats[0] = sdev->format->format;
> -	sdev->nformats = 1;
> -
> -	/* default formats go second */
> -	for (i = 0; i < ARRAY_SIZE(simpledrm_primary_plane_formats); ++i) {
> -		if (simpledrm_primary_plane_formats[i] == sdev->format->format)
> -			continue; /* native format already went first */
> -		sdev->formats[sdev->nformats] = simpledrm_primary_plane_formats[i];
> -		sdev->nformats++;
> -	}
> -
> -	/*
> -	 * TODO: The simpledrm driver converts framebuffers to the native
> -	 * format when copying them to device memory. If there are more
> -	 * formats listed than supported by the driver, the native format
> -	 * is not supported by the conversion helpers. Therefore *only*
> -	 * support the native format and add a conversion helper ASAP.
> -	 */
> -	if (drm_WARN_ONCE(dev, i != sdev->nformats,
> -			  "format conversion helpers required for %p4cc",
> -			  &sdev->format->format)) {
> -		sdev->nformats = 1;
> -	}
> -
> -out:
> -	*nformats_out = sdev->nformats;
> -	return sdev->formats;
> -}
> -
>  static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  							struct platform_device *pdev)
>  {
> @@ -699,7 +660,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  	struct drm_encoder *encoder;
>  	struct drm_connector *connector;
>  	unsigned long max_width, max_height;
> -	const uint32_t *formats;
>  	size_t nformats;
>  	int ret;
>  
> @@ -811,11 +771,14 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  
>  	/* Primary plane */
>  
> -	formats = simpledrm_device_formats(sdev, &nformats);
> +	nformats = drm_fb_build_fourcc_list(dev, &format->format, 1,
> +					    simpledrm_primary_plane_formats,
> +					    ARRAY_SIZE(simpledrm_primary_plane_formats),
> +					    sdev->formats, ARRAY_SIZE(sdev->formats));
>  
>  	primary_plane = &sdev->primary_plane;
>  	ret = drm_universal_plane_init(dev, primary_plane, 0, &simpledrm_primary_plane_funcs,
> -				       formats, nformats,
> +				       sdev->formats, nformats,
>  				       simpledrm_primary_plane_format_modifiers,
>  				       DRM_PLANE_TYPE_PRIMARY, NULL);
>  	if (ret)
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index caa181194335..eb5c98cf82b8 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -6,11 +6,15 @@
>  #ifndef __LINUX_DRM_FORMAT_HELPER_H
>  #define __LINUX_DRM_FORMAT_HELPER_H
>  
> -struct iosys_map;
> +#include <linux/types.h>
> +
> +struct drm_device;
>  struct drm_format_info;
>  struct drm_framebuffer;
>  struct drm_rect;
>  
> +struct iosys_map;
> +
>  unsigned int drm_fb_clip_offset(unsigned int pitch, const struct drm_format_info *format,
>  				const struct drm_rect *clip);
>  
> @@ -44,4 +48,9 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
>  			     const struct iosys_map *src, const struct drm_framebuffer *fb,
>  			     const struct drm_rect *clip);
>  
> +size_t drm_fb_build_fourcc_list(struct drm_device *dev,
> +				const u32 *native_fourccs, size_t native_nfourccs,
> +				const u32 *extra_fourccs, size_t extra_nfourccs,
> +				u32 *fourccs_out, size_t nfourccs_out);
> +
>  #endif /* __LINUX_DRM_FORMAT_HELPER_H */
> -- 
> 2.37.1

Re: [PATCH v2 4/4] drm/format-helper: Add drm_fb_build_fourcc_list() helper

[Thomas Zimmermann]

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

Hi Sam

Am 16.08.22 um 19:22 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Tue, Aug 16, 2022 at 03:48:53PM +0200, Thomas Zimmermann wrote:
>> Add drm_fb_build_fourcc_list() function that builds a list of supported
>> formats from native and emulated ones. Helpful for all drivers that do
>> format conversion as part of their plane updates. Update current caller.
>>
>> v2:
>> 	* use u32 instead of uint32_t (Sam)
>> 	* print a warning if output array is too small (Sam)
>> 	* comment fixes (Sam)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>> ---
>>   drivers/gpu/drm/drm_format_helper.c | 103 ++++++++++++++++++++++++++++
>>   drivers/gpu/drm/tiny/simpledrm.c    |  47 ++-----------
>>   include/drm/drm_format_helper.h     |  11 ++-
>>   3 files changed, 118 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
>> index 56642816fdff..fe1db7dbda3f 100644
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
>> @@ -793,3 +793,106 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
>>   	kfree(src32);
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono);
>> +
>> +static bool is_listed_fourcc(const uint32_t *fourccs, size_t nfourccs, uint32_t fourcc)
>> +{
>> +	const uint32_t *fourccs_end = fourccs + nfourccs;
>> +
>> +	while (fourccs < fourccs_end) {
>> +		if (*fourccs == fourcc)
>> +			return true;
>> +		++fourccs;
>> +	}
>> +	return false;
>> +}
>> +
>> +/**
>> + * drm_fb_build_fourcc_list - Filters a list of supported color formats against
>> + *                            the device's native formats
>> + * @dev: DRM device
>> + * @native_fourccs: 4CC codes of natively supported color formats
>> + * @native_nfourccs: The number of entries in @native_fourccs
>> + * @extra_fourccs: 4CC codes of additionally supported color formats
>> + * @extra_nfourccs: The number of entries in @extra_fourccs
>> + * @fourccs_out: Returns 4CC codes of supported color formats
>> + * @nfourccs_out: The number of available entries in @fourccs_out
>> + *
>> + * This function create a list of supported color format from natively
>> + * supported formats and the emulated formats.
>> + * At a minimum, most userspace programs expect at least support for
>> + * XRGB8888 on the primary plane. Devices that have to emulate the
>> + * format, and possibly others, can use drm_fb_build_fourcc_list() to
>> + * create a list of supported color formats. The returned list can
>> + * be handed over to drm_universal_plane_init() et al. Native formats
>> + * will go before emulated formats. Other heuristics might be applied
>> + * to optimize the order. Formats near the beginning of the list are
>> + * usually preferred over formats near the end of the list.
>> + *
>> + * Returns:
>> + * The number of color-formats 4CC codes returned in @fourccs_out.
>> + */
>> +size_t drm_fb_build_fourcc_list(struct drm_device *dev,
>> +				const u32 *native_fourccs, size_t native_nfourccs,
>> +				const u32 *extra_fourccs, size_t extra_nfourccs,
>> +				u32 *fourccs_out, size_t nfourccs_out)
>> +{
>> +	u32 *fourccs = fourccs_out;
>> +	const u32 *fourccs_end = fourccs_out + nfourccs_out;
>> +	bool found_native = false;
>> +	size_t nfourccs, i;
>> +
>> +	/*
>> +	 * The device's native formats go first.
>> +	 */
>> +
>> +	nfourccs = min_t(size_t, native_nfourccs, nfourccs_out);
> If nfourccs < nfourccs_out then there is not enough room for the
> formats. I know this is unlikely, but anyway.. Maybe warn here too?

We need to take all filtering into account before warning. See my 
comment below.

> 
> I would use min() here, as the types are all compatible, so no need to
> cast to size_t.
> 
>> +
>> +	for (i = 0; i < nfourccs; ++i) {
>> +		u32 fourcc = native_fourccs[i];
>> +
>> +		drm_dbg_kms(dev, "adding native format %p4cc\n", &fourcc);
>> +
>> +		if (!found_native)
>> +			found_native = is_listed_fourcc(extra_fourccs, extra_nfourccs, fourcc);
>> +		*fourccs = fourcc;
>> +		++fourccs;
>> +	}
>> +
>> +	/*
>> +	 * The plane's atomic_update helper converts the framebuffer's color format
>> +	 * to a native format when copying to device memory.
>> +	 *
>> +	 * If there is not a single format supported by both, device and
>> +	 * driver, the native formats are likely not supported by the conversion
>> +	 * helpers. Therefore *only* support the native formats and add a
>> +	 * conversion helper ASAP.
>> +	 */
>> +	if (!found_native) {
>> +		drm_warn(dev, "Format conversion helpers required to add extra formats.\n");
>> +		goto out;
>> +	}
>> +
>> +	/*
>> +	 * The extra formats, emulated by the driver, go second.
>> +	 */
>> +
>> +	nfourccs = min_t(size_t, extra_nfourccs, fourccs_end - fourccs);
> fourccs_end is a pointer which is subtracted with fourccs * sizeof(fourccs_end)
> So the result is a pointer. I do not think this does what you try to do.
> (Or I need more coffee?)

I'm not sure I understand. (fourccs_end - fourccs) returns the number 
entries available at fourccs. It's not a pointer, but ptrdiff_t.

> 
> 
> Using min() should give a nice warning I think - so drop min_t to avoid
> the cast.

ptrdiff_t is signed, so it can generate a warning when compared to 
extra_nfourccs' size_t.

> 
>> +
>> +	for (i = 0; i < nfourccs; ++i) {
>> +		u32 fourcc = extra_fourccs[i];
>> +
>> +		if (is_listed_fourcc(native_fourccs, native_nfourccs, fourcc))
>> +			continue; /* native formats already went first */
>> +		*fourccs = fourcc;
>> +		++fourccs;
>> +	}
>> +
>> +	if (nfourccs < extra_nfourccs) {
>> +		drm_warn(dev, "Format buffer too small. %zu trailing formats dropped.\n",
>> +			 extra_nfourccs - nfourccs);
>> +	}
> Move this up right after the min()?

It can happen that native and extra 4CCs overlap with duplicates. There 
is a continue statement in the loop, which filters out duplicates.

We really should run all loops over the input and then look at the 
resulting output. We need to do this at the end, but the condition is 
not quite correct yet. I'll send out a v3 with a better test.

Best regards
Thomas

> 
> 	Sam
>> +
>> +out:
>> +	return fourccs - fourccs_out;
>> +}
>> +EXPORT_SYMBOL(drm_fb_build_fourcc_list);
>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
>> index 404290760c60..777ccd250871 100644
>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>> @@ -644,45 +644,6 @@ static struct drm_display_mode simpledrm_mode(unsigned int width,
>>   	return mode;
>>   }
>>   
>> -static const uint32_t *simpledrm_device_formats(struct simpledrm_device *sdev,
>> -						size_t *nformats_out)
>> -{
>> -	struct drm_device *dev = &sdev->dev;
>> -	size_t i;
>> -
>> -	if (sdev->nformats)
>> -		goto out; /* don't rebuild list on recurring calls */
>> -
>> -	/* native format goes first */
>> -	sdev->formats[0] = sdev->format->format;
>> -	sdev->nformats = 1;
>> -
>> -	/* default formats go second */
>> -	for (i = 0; i < ARRAY_SIZE(simpledrm_primary_plane_formats); ++i) {
>> -		if (simpledrm_primary_plane_formats[i] == sdev->format->format)
>> -			continue; /* native format already went first */
>> -		sdev->formats[sdev->nformats] = simpledrm_primary_plane_formats[i];
>> -		sdev->nformats++;
>> -	}
>> -
>> -	/*
>> -	 * TODO: The simpledrm driver converts framebuffers to the native
>> -	 * format when copying them to device memory. If there are more
>> -	 * formats listed than supported by the driver, the native format
>> -	 * is not supported by the conversion helpers. Therefore *only*
>> -	 * support the native format and add a conversion helper ASAP.
>> -	 */
>> -	if (drm_WARN_ONCE(dev, i != sdev->nformats,
>> -			  "format conversion helpers required for %p4cc",
>> -			  &sdev->format->format)) {
>> -		sdev->nformats = 1;
>> -	}
>> -
>> -out:
>> -	*nformats_out = sdev->nformats;
>> -	return sdev->formats;
>> -}
>> -
>>   static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>   							struct platform_device *pdev)
>>   {
>> @@ -699,7 +660,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>   	struct drm_encoder *encoder;
>>   	struct drm_connector *connector;
>>   	unsigned long max_width, max_height;
>> -	const uint32_t *formats;
>>   	size_t nformats;
>>   	int ret;
>>   
>> @@ -811,11 +771,14 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>   
>>   	/* Primary plane */
>>   
>> -	formats = simpledrm_device_formats(sdev, &nformats);
>> +	nformats = drm_fb_build_fourcc_list(dev, &format->format, 1,
>> +					    simpledrm_primary_plane_formats,
>> +					    ARRAY_SIZE(simpledrm_primary_plane_formats),
>> +					    sdev->formats, ARRAY_SIZE(sdev->formats));
>>   
>>   	primary_plane = &sdev->primary_plane;
>>   	ret = drm_universal_plane_init(dev, primary_plane, 0, &simpledrm_primary_plane_funcs,
>> -				       formats, nformats,
>> +				       sdev->formats, nformats,
>>   				       simpledrm_primary_plane_format_modifiers,
>>   				       DRM_PLANE_TYPE_PRIMARY, NULL);
>>   	if (ret)
>> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
>> index caa181194335..eb5c98cf82b8 100644
>> --- a/include/drm/drm_format_helper.h
>> +++ b/include/drm/drm_format_helper.h
>> @@ -6,11 +6,15 @@
>>   #ifndef __LINUX_DRM_FORMAT_HELPER_H
>>   #define __LINUX_DRM_FORMAT_HELPER_H
>>   
>> -struct iosys_map;
>> +#include <linux/types.h>
>> +
>> +struct drm_device;
>>   struct drm_format_info;
>>   struct drm_framebuffer;
>>   struct drm_rect;
>>   
>> +struct iosys_map;
>> +
>>   unsigned int drm_fb_clip_offset(unsigned int pitch, const struct drm_format_info *format,
>>   				const struct drm_rect *clip);
>>   
>> @@ -44,4 +48,9 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
>>   			     const struct iosys_map *src, const struct drm_framebuffer *fb,
>>   			     const struct drm_rect *clip);
>>   
>> +size_t drm_fb_build_fourcc_list(struct drm_device *dev,
>> +				const u32 *native_fourccs, size_t native_nfourccs,
>> +				const u32 *extra_fourccs, size_t extra_nfourccs,
>> +				u32 *fourccs_out, size_t nfourccs_out);
>> +
>>   #endif /* __LINUX_DRM_FORMAT_HELPER_H */
>> -- 
>> 2.37.1

-- 
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 v2 3/4] drm/modes: Add initializer macro DRM_MODE_INIT()

[kernel test robot]

Hi Thomas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on next-20220819]
[cannot apply to linus/master v6.0-rc1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/drm-probe-helper-modes-Helpers-for-single-mode-displays/20220816-215250
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
reproduce:
        # https://github.com/intel-lab-lkp/linux/commit/25770aac56aeff6f55419ea4316406ddbb29385f
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Thomas-Zimmermann/drm-probe-helper-modes-Helpers-for-single-mode-displays/20220816-215250
        git checkout 25770aac56aeff6f55419ea4316406ddbb29385f
        make menuconfig
        # enable CONFIG_COMPILE_TEST, CONFIG_WARN_MISSING_DOCUMENTS, CONFIG_WARN_ABI_ERRORS
        make htmldocs

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> ./include/drm/drm_modes.h:168: warning: expecting prototype for DRM_SIMPLE_MODE_INIT(). Prototype was for DRM_MODE_INIT() instead

vim +168 ./include/drm/drm_modes.h

   133	
   134	#define DRM_MODE(nm, t, c, hd, hss, hse, ht, hsk, vd, vss, vse, vt, vs, f) \
   135		.name = nm, .status = 0, .type = (t), .clock = (c), \
   136		.hdisplay = (hd), .hsync_start = (hss), .hsync_end = (hse), \
   137		.htotal = (ht), .hskew = (hsk), .vdisplay = (vd), \
   138		.vsync_start = (vss), .vsync_end = (vse), .vtotal = (vt), \
   139		.vscan = (vs), .flags = (f)
   140	
   141	/**
   142	 * DRM_MODE_RES_MM - Calculates the display size from resolution and DPI
   143	 * @res: The resolution in pixel
   144	 * @dpi: The number of dots per inch
   145	 */
   146	#define DRM_MODE_RES_MM(res, dpi)	\
   147		(((res) * 254ul) / ((dpi) * 10ul))
   148	
   149	#define __DRM_MODE_INIT(pix, hd, vd, hd_mm, vd_mm) \
   150		.type = DRM_MODE_TYPE_DRIVER, .clock = (pix), \
   151		.hdisplay = (hd), .hsync_start = (hd), .hsync_end = (hd), \
   152		.htotal = (hd), .vdisplay = (vd), .vsync_start = (vd), \
   153		.vsync_end = (vd), .vtotal = (vd), .width_mm = (hd_mm), \
   154		.height_mm = (vd_mm)
   155	
   156	/**
   157	 * DRM_SIMPLE_MODE_INIT - Initialize display mode
   158	 * @hz: Vertical refresh rate in Hertz
   159	 * @hd: Horizontal resolution, width
   160	 * @vd: Vertical resolution, height
   161	 * @hd_mm: Display width in millimeters
   162	 * @vd_mm: Display height in millimeters
   163	 *
   164	 * This macro initializes a &drm_display_mode that contains information about
   165	 * refresh rate, resolution and physical size.
   166	 */
   167	#define DRM_MODE_INIT(hz, hd, vd, hd_mm, vd_mm) \
 > 168		__DRM_MODE_INIT((hd) * (vd) * (hz) / 1000 /* kHz */, hd, vd, hd_mm, vd_mm)
   169	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp