[PATCH] drm cleanup patches

[PATCH] drm cleanup patches

[Sascha Hauer]

The following patches contain some fixes and cleanups for the drm
core.

- fix memory holes
- make some initialization / deinitialization more symmetric
- add convenience functions for creating properties
- remove DRM_CONNECTOR_MAX_PROPERTY limitation

All patches tested on a GeForce 6200 LE with the nouveau driver and
a DELL E6220 Laptop using the intel driver.

Please review and consider applying

Sascha

Sascha Hauer (20):
      drm crtc: use drm_mode_destroy instead of kfree in drm_mode_remove
      drm crtc: add forgotten idr cleanup functions
      drm drm_edit: drm modes have to be free with drm_mode_destroy
      drm drm_fb_helper: destroy modes
      drm: add proper return value for drm_mode_crtc_set_gamma_size
      drm fb helper: use drm_helper_connector_dpms to do dpms
      drm fb helper: remove unused variable conn_limit
      drm fb helper: remove unused variable crtc_id
      drm fb_helper: use lists for crtcs.
      drm: remove now unused crtc_count parameter from drm_fb_helper_init
      drm fb helper: add the connectors inside drm_fb_helper_initial_config
      drm crtc_helper: use list_for_each_entry
      drm crtc: Fix locking comments
      drm: add convenience function to create an enum property
      drm: add convenience function to create an range property
      drm: store connector properties in list
      drm: remove checks for same value in set_prop
      drm: do not call drm_connector_property_set_value from drivers
      drm exynos: use drm_fb_helper_set_par directly
      drm: do not set fb_info->pixmap fields

 drivers/gpu/drm/drm_crtc.c                  |  315 +++++++++++++--------------
 drivers/gpu/drm/drm_crtc_helper.c           |   12 +-
 drivers/gpu/drm/drm_edid.c                  |    2 +-
 drivers/gpu/drm/drm_fb_helper.c             |  216 +++++++------------
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c   |   49 +----
 drivers/gpu/drm/gma500/cdv_intel_hdmi.c     |    4 -
 drivers/gpu/drm/gma500/cdv_intel_lvds.c     |   33 +---
 drivers/gpu/drm/gma500/framebuffer.c        |   15 +-
 drivers/gpu/drm/gma500/psb_intel_lvds.c     |   36 +---
 drivers/gpu/drm/gma500/psb_intel_sdvo.c     |   45 +----
 drivers/gpu/drm/i2c/ch7006_drv.c            |    5 +-
 drivers/gpu/drm/i915/intel_dp.c             |   11 -
 drivers/gpu/drm/i915/intel_fb.c             |   11 +-
 drivers/gpu/drm/i915/intel_hdmi.c           |    5 -
 drivers/gpu/drm/i915/intel_modes.c          |   28 +--
 drivers/gpu/drm/i915/intel_sdvo.c           |   35 +---
 drivers/gpu/drm/i915/intel_tv.c             |   31 +---
 drivers/gpu/drm/nouveau/nouveau_connector.c |   32 +--
 drivers/gpu/drm/nouveau/nouveau_display.c   |   20 +-
 drivers/gpu/drm/nouveau/nouveau_fbcon.c     |   11 +-
 drivers/gpu/drm/radeon/radeon_connectors.c  |   24 +--
 drivers/gpu/drm/radeon/radeon_display.c     |   70 ++-----
 drivers/gpu/drm/radeon/radeon_fb.c          |   11 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_fb.c          |   14 --
 drivers/video/nvidia/nvidia.c               |    6 -
 include/drm/drm_crtc.h                      |   24 ++-
 include/drm/drm_fb_helper.h                 |    9 +-
 27 files changed, 356 insertions(+), 718 deletions(-)

[PATCH 01/20] drm crtc: use drm_mode_destroy instead of kfree in drm_mode_remove

[Sascha Hauer]

Modes are created using drm_mode_create which does a
drm_mode_object_get, so use drm_mode_destroy in drm_mode_remove
which does a drm_mode_object_put.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_crtc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 5e818a8..8389fd3 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -442,7 +442,7 @@ void drm_mode_remove(struct drm_connector *connector,
 		     struct drm_display_mode *mode)
 {
 	list_del(&mode->head);
-	kfree(mode);
+	drm_mode_destroy(connector->dev, mode);
 }
 EXPORT_SYMBOL(drm_mode_remove);
 
-- 
1.7.8.3

[PATCH 02/20] drm crtc: add forgotten idr cleanup functions

[Sascha Hauer]

drm_mode_config_init initializes the idr with idr_init, so
add the missing counterparts in drm_mode_config_cleanup.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_crtc.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 8389fd3..33ebe29 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1048,6 +1048,9 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 				 head) {
 		plane->funcs->destroy(plane);
 	}
+
+	idr_remove_all(&dev->mode_config.crtc_idr);
+	idr_destroy(&dev->mode_config.crtc_idr);
 }
 EXPORT_SYMBOL(drm_mode_config_cleanup);
 
-- 
1.7.8.3

[PATCH 03/20] drm drm_edit: drm modes have to be free with drm_mode_destroy

[Sascha Hauer]

to add the missing drm_mode_object_put for that mode.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_edid.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index ece03fc..770d894 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -745,7 +745,7 @@ drm_mode_std(struct drm_connector *connector, struct edid *edid,
 		 */
 		mode = drm_gtf_mode(dev, hsize, vsize, vrefresh_rate, 0, 0);
 		if (drm_mode_hsync(mode) > drm_gtf2_hbreak(edid)) {
-			kfree(mode);
+			drm_mode_destroy(dev, mode);
 			mode = drm_gtf_mode_complex(dev, hsize, vsize,
 						    vrefresh_rate, 0, 0,
 						    drm_gtf2_m(edid),
-- 
1.7.8.3

[PATCH 04/20] drm drm_fb_helper: destroy modes

[Sascha Hauer]

drm_setup_crtcs allocated modes using drm_mode_duplicate. Free
them in drm_fb_helper_crtc_free.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_fb_helper.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index aada26f..77fec5a 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -430,8 +430,11 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
 	for (i = 0; i < helper->connector_count; i++)
 		kfree(helper->connector_info[i]);
 	kfree(helper->connector_info);
-	for (i = 0; i < helper->crtc_count; i++)
+	for (i = 0; i < helper->crtc_count; i++) {
 		kfree(helper->crtc_info[i].mode_set.connectors);
+		if (helper->crtc_info[i].mode_set.mode)
+			drm_mode_destroy(helper->dev, helper->crtc_info[i].mode_set.mode);
+	}
 	kfree(helper->crtc_info);
 }
 
-- 
1.7.8.3

[PATCH 05/20] drm: add proper return value for drm_mode_crtc_set_gamma_size

[Sascha Hauer]

drm_mode_crtc_set_gamma_size returns boolean true for success
and false for failure. This is not very kernel conform, so
change it to return 0 for success and a propert error code
otherwise. Noone checks the return value, so no users have to
be fixed.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_crtc.c |    6 +++---
 include/drm/drm_crtc.h     |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 33ebe29..df6e413 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3024,7 +3024,7 @@ void drm_mode_connector_detach_encoder(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_mode_connector_detach_encoder);
 
-bool drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
+int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 				  int gamma_size)
 {
 	crtc->gamma_size = gamma_size;
@@ -3032,10 +3032,10 @@ bool drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 	crtc->gamma_store = kzalloc(gamma_size * sizeof(uint16_t) * 3, GFP_KERNEL);
 	if (!crtc->gamma_store) {
 		crtc->gamma_size = 0;
-		return false;
+		return -ENOMEM;
 	}
 
-	return true;
+	return 0;
 }
 EXPORT_SYMBOL(drm_mode_crtc_set_gamma_size);
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 4cd4be2..8d593ad 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -919,7 +919,7 @@ extern int drm_mode_connector_attach_encoder(struct drm_connector *connector,
 					     struct drm_encoder *encoder);
 extern void drm_mode_connector_detach_encoder(struct drm_connector *connector,
 					   struct drm_encoder *encoder);
-extern bool drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
+extern int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 					 int gamma_size);
 extern struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
 		uint32_t id, uint32_t type);
-- 
1.7.8.3

[PATCH 06/20] drm fb helper: use drm_helper_connector_dpms to do dpms

[Sascha Hauer]

drm_fb_helper_on|off currently manually searches for encoders
to turn on/off. Make this simpler by using the helper function.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_fb_helper.c |   80 +++++----------------------------------
 1 files changed, 10 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 77fec5a..4fc38a7f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -306,91 +306,31 @@ static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = {
 static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { };
 #endif
 
-static void drm_fb_helper_on(struct fb_info *info)
+static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
 {
 	struct drm_fb_helper *fb_helper = info->par;
 	struct drm_device *dev = fb_helper->dev;
 	struct drm_crtc *crtc;
-	struct drm_crtc_helper_funcs *crtc_funcs;
 	struct drm_connector *connector;
-	struct drm_encoder *encoder;
 	int i, j;
 
 	/*
-	 * For each CRTC in this fb, turn the crtc on then,
-	 * find all associated encoders and turn them on.
+	 * For each CRTC in this fb, turn the connectors on/off.
 	 */
 	mutex_lock(&dev->mode_config.mutex);
 	for (i = 0; i < fb_helper->crtc_count; i++) {
 		crtc = fb_helper->crtc_info[i].mode_set.crtc;
-		crtc_funcs = crtc->helper_private;
 
 		if (!crtc->enabled)
 			continue;
 
-		crtc_funcs->dpms(crtc, DRM_MODE_DPMS_ON);
-
-		/* Walk the connectors & encoders on this fb turning them on */
+		/* Walk the connectors & encoders on this fb turning them on/off */
 		for (j = 0; j < fb_helper->connector_count; j++) {
 			connector = fb_helper->connector_info[j]->connector;
-			connector->dpms = DRM_MODE_DPMS_ON;
+			drm_helper_connector_dpms(connector, dpms_mode);
 			drm_connector_property_set_value(connector,
-							 dev->mode_config.dpms_property,
-							 DRM_MODE_DPMS_ON);
-		}
-		/* Found a CRTC on this fb, now find encoders */
-		list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
-			if (encoder->crtc == crtc) {
-				struct drm_encoder_helper_funcs *encoder_funcs;
-
-				encoder_funcs = encoder->helper_private;
-				encoder_funcs->dpms(encoder, DRM_MODE_DPMS_ON);
-			}
-		}
-	}
-	mutex_unlock(&dev->mode_config.mutex);
-}
-
-static void drm_fb_helper_off(struct fb_info *info, int dpms_mode)
-{
-	struct drm_fb_helper *fb_helper = info->par;
-	struct drm_device *dev = fb_helper->dev;
-	struct drm_crtc *crtc;
-	struct drm_crtc_helper_funcs *crtc_funcs;
-	struct drm_connector *connector;
-	struct drm_encoder *encoder;
-	int i, j;
-
-	/*
-	 * For each CRTC in this fb, find all associated encoders
-	 * and turn them off, then turn off the CRTC.
-	 */
-	mutex_lock(&dev->mode_config.mutex);
-	for (i = 0; i < fb_helper->crtc_count; i++) {
-		crtc = fb_helper->crtc_info[i].mode_set.crtc;
-		crtc_funcs = crtc->helper_private;
-
-		if (!crtc->enabled)
-			continue;
-
-		/* Walk the connectors on this fb and mark them off */
-		for (j = 0; j < fb_helper->connector_count; j++) {
-			connector = fb_helper->connector_info[j]->connector;
-			connector->dpms = dpms_mode;
-			drm_connector_property_set_value(connector,
-							 dev->mode_config.dpms_property,
-							 dpms_mode);
-		}
-		/* Found a CRTC on this fb, now find encoders */
-		list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
-			if (encoder->crtc == crtc) {
-				struct drm_encoder_helper_funcs *encoder_funcs;
-
-				encoder_funcs = encoder->helper_private;
-				encoder_funcs->dpms(encoder, dpms_mode);
-			}
+				dev->mode_config.dpms_property, dpms_mode);
 		}
-		crtc_funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
 	}
 	mutex_unlock(&dev->mode_config.mutex);
 }
@@ -400,23 +340,23 @@ int drm_fb_helper_blank(int blank, struct fb_info *info)
 	switch (blank) {
 	/* Display: On; HSync: On, VSync: On */
 	case FB_BLANK_UNBLANK:
-		drm_fb_helper_on(info);
+		drm_fb_helper_dpms(info, DRM_MODE_DPMS_ON);
 		break;
 	/* Display: Off; HSync: On, VSync: On */
 	case FB_BLANK_NORMAL:
-		drm_fb_helper_off(info, DRM_MODE_DPMS_STANDBY);
+		drm_fb_helper_dpms(info, DRM_MODE_DPMS_STANDBY);
 		break;
 	/* Display: Off; HSync: Off, VSync: On */
 	case FB_BLANK_HSYNC_SUSPEND:
-		drm_fb_helper_off(info, DRM_MODE_DPMS_STANDBY);
+		drm_fb_helper_dpms(info, DRM_MODE_DPMS_STANDBY);
 		break;
 	/* Display: Off; HSync: On, VSync: Off */
 	case FB_BLANK_VSYNC_SUSPEND:
-		drm_fb_helper_off(info, DRM_MODE_DPMS_SUSPEND);
+		drm_fb_helper_dpms(info, DRM_MODE_DPMS_SUSPEND);
 		break;
 	/* Display: Off; HSync: Off, VSync: Off */
 	case FB_BLANK_POWERDOWN:
-		drm_fb_helper_off(info, DRM_MODE_DPMS_OFF);
+		drm_fb_helper_dpms(info, DRM_MODE_DPMS_OFF);
 		break;
 	}
 	return 0;
-- 
1.7.8.3

[PATCH 07/20] drm fb helper: remove unused variable conn_limit

[Sascha Hauer]

conn_limit is set but never used. Remove it from struct
drm_fb_helper.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_fb_helper.c |    2 +-
 include/drm/drm_fb_helper.h     |    1 -
 2 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 4fc38a7f..7b37874 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -421,7 +421,7 @@ int drm_fb_helper_init(struct drm_device *dev,
 		fb_helper->crtc_info[i].mode_set.crtc = crtc;
 		i++;
 	}
-	fb_helper->conn_limit = max_conn_count;
+
 	return 0;
 out_free:
 	drm_fb_helper_crtc_free(fb_helper);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 6e3076a..55e10d6 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -74,7 +74,6 @@ struct drm_fb_helper {
 	int connector_count;
 	struct drm_fb_helper_connector **connector_info;
 	struct drm_fb_helper_funcs *funcs;
-	int conn_limit;
 	struct fb_info *fbdev;
 	u32 pseudo_palette[17];
 	struct list_head kernel_fb_list;
-- 
1.7.8.3

[PATCH 08/20] drm fb helper: remove unused variable crtc_id

[Sascha Hauer]

crtc_id is set but never used, so remove it from struct
drm_fb_helper_crtc.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_fb_helper.c |    1 -
 include/drm/drm_fb_helper.h     |    1 -
 2 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 7b37874..7740dd2 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -417,7 +417,6 @@ int drm_fb_helper_init(struct drm_device *dev,
 
 	i = 0;
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
-		fb_helper->crtc_info[i].crtc_id = crtc->base.id;
 		fb_helper->crtc_info[i].mode_set.crtc = crtc;
 		i++;
 	}
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 55e10d6..5120b01 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -35,7 +35,6 @@ struct drm_fb_helper;
 #include <linux/kgdb.h>
 
 struct drm_fb_helper_crtc {
-	uint32_t crtc_id;
 	struct drm_mode_set mode_set;
 	struct drm_display_mode *desired_mode;
 };
-- 
1.7.8.3

[PATCH 09/20] drm fb_helper: use lists for crtcs.

[Sascha Hauer]

The fb helper uses fixed size arrays for the associated crtcs.
This is an unnecessary limitation, so instead use a list to
store the crtcs and allocate them dynamically.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_fb_helper.c |  129 ++++++++++++++++++++-------------------
 include/drm/drm_fb_helper.h     |    3 +-
 2 files changed, 68 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 7740dd2..f292a78 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -147,15 +147,14 @@ int drm_fb_helper_debug_enter(struct fb_info *info)
 {
 	struct drm_fb_helper *helper = info->par;
 	struct drm_crtc_helper_funcs *funcs;
-	int i;
 
 	if (list_empty(&kernel_fb_helper_list))
 		return false;
 
 	list_for_each_entry(helper, &kernel_fb_helper_list, kernel_fb_list) {
-		for (i = 0; i < helper->crtc_count; i++) {
-			struct drm_mode_set *mode_set =
-				&helper->crtc_info[i].mode_set;
+		struct drm_fb_helper_crtc *helper_crtc;
+		list_for_each_entry(helper_crtc, &helper->crtc_list, list) {
+			struct drm_mode_set *mode_set = &helper_crtc->mode_set;
 
 			if (!mode_set->crtc->enabled)
 				continue;
@@ -194,10 +193,10 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
 	struct drm_crtc *crtc;
 	struct drm_crtc_helper_funcs *funcs;
 	struct drm_framebuffer *fb;
-	int i;
+	struct drm_fb_helper_crtc *helper_crtc;
 
-	for (i = 0; i < helper->crtc_count; i++) {
-		struct drm_mode_set *mode_set = &helper->crtc_info[i].mode_set;
+	list_for_each_entry(helper_crtc, &helper->crtc_list, list) {
+		struct drm_mode_set *mode_set = &helper_crtc->mode_set;
 		crtc = mode_set->crtc;
 		funcs = crtc->helper_private;
 		fb = drm_mode_config_fb(crtc);
@@ -221,10 +220,12 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
 
 bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 {
+	struct drm_fb_helper_crtc *helper_crtc;
 	bool error = false;
-	int i, ret;
-	for (i = 0; i < fb_helper->crtc_count; i++) {
-		struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set;
+	int ret;
+
+	list_for_each_entry(helper_crtc, &fb_helper->crtc_list, list) {
+		struct drm_mode_set *mode_set = &helper_crtc->mode_set;
 		ret = drm_crtc_helper_set_config(mode_set);
 		if (ret)
 			error = true;
@@ -312,14 +313,15 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
 	struct drm_device *dev = fb_helper->dev;
 	struct drm_crtc *crtc;
 	struct drm_connector *connector;
-	int i, j;
+	struct drm_fb_helper_crtc *helper_crtc;
+	int j;
 
 	/*
 	 * For each CRTC in this fb, turn the connectors on/off.
 	 */
 	mutex_lock(&dev->mode_config.mutex);
-	for (i = 0; i < fb_helper->crtc_count; i++) {
-		crtc = fb_helper->crtc_info[i].mode_set.crtc;
+	list_for_each_entry(helper_crtc, &fb_helper->crtc_list, list) {
+		crtc = helper_crtc->mode_set.crtc;
 
 		if (!crtc->enabled)
 			continue;
@@ -365,17 +367,20 @@ EXPORT_SYMBOL(drm_fb_helper_blank);
 
 static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
 {
+	struct drm_fb_helper_crtc *helper_crtc, *tmp;
 	int i;
 
 	for (i = 0; i < helper->connector_count; i++)
 		kfree(helper->connector_info[i]);
+
 	kfree(helper->connector_info);
-	for (i = 0; i < helper->crtc_count; i++) {
-		kfree(helper->crtc_info[i].mode_set.connectors);
-		if (helper->crtc_info[i].mode_set.mode)
-			drm_mode_destroy(helper->dev, helper->crtc_info[i].mode_set.mode);
+
+	list_for_each_entry_safe(helper_crtc, tmp, &helper->crtc_list, list) {
+		kfree(helper_crtc->mode_set.connectors);
+		if (helper_crtc->mode_set.mode)
+			drm_mode_destroy(helper->dev, helper_crtc->mode_set.mode);
+		kfree(helper_crtc);
 	}
-	kfree(helper->crtc_info);
 }
 
 int drm_fb_helper_init(struct drm_device *dev,
@@ -383,42 +388,40 @@ int drm_fb_helper_init(struct drm_device *dev,
 		       int crtc_count, int max_conn_count)
 {
 	struct drm_crtc *crtc;
+	struct drm_fb_helper_crtc *helper_crtc;
 	int ret = 0;
-	int i;
 
 	fb_helper->dev = dev;
 
 	INIT_LIST_HEAD(&fb_helper->kernel_fb_list);
+	INIT_LIST_HEAD(&fb_helper->crtc_list);
 
-	fb_helper->crtc_info = kcalloc(crtc_count, sizeof(struct drm_fb_helper_crtc), GFP_KERNEL);
-	if (!fb_helper->crtc_info)
+	fb_helper->crtc_count = 0;
+	fb_helper->connector_info = kcalloc(dev->mode_config.num_connector,
+			sizeof(struct drm_fb_helper_connector *), GFP_KERNEL);
+	if (!fb_helper->connector_info)
 		return -ENOMEM;
 
-	fb_helper->crtc_count = crtc_count;
-	fb_helper->connector_info = kcalloc(dev->mode_config.num_connector, sizeof(struct drm_fb_helper_connector *), GFP_KERNEL);
-	if (!fb_helper->connector_info) {
-		kfree(fb_helper->crtc_info);
-		return -ENOMEM;
-	}
 	fb_helper->connector_count = 0;
 
-	for (i = 0; i < crtc_count; i++) {
-		fb_helper->crtc_info[i].mode_set.connectors =
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		helper_crtc = kzalloc(sizeof(*helper_crtc), GFP_KERNEL);
+		if (!helper_crtc)
+			goto out_free;
+
+		helper_crtc->mode_set.crtc = crtc;
+		helper_crtc->mode_set.connectors =
 			kcalloc(max_conn_count,
 				sizeof(struct drm_connector *),
 				GFP_KERNEL);
 
-		if (!fb_helper->crtc_info[i].mode_set.connectors) {
+		if (!helper_crtc->mode_set.connectors) {
 			ret = -ENOMEM;
 			goto out_free;
 		}
-		fb_helper->crtc_info[i].mode_set.num_connectors = 0;
-	}
-
-	i = 0;
-	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
-		fb_helper->crtc_info[i].mode_set.crtc = crtc;
-		i++;
+		helper_crtc->mode_set.num_connectors = 0;
+		list_add_tail(&helper_crtc->list, &fb_helper->crtc_list);
+		fb_helper->crtc_count++;
 	}
 
 	return 0;
@@ -515,11 +518,12 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 	struct drm_crtc_helper_funcs *crtc_funcs;
 	u16 *red, *green, *blue, *transp;
 	struct drm_crtc *crtc;
-	int i, j, rc = 0;
+	struct drm_fb_helper_crtc *helper_crtc;
+	int j, rc = 0;
 	int start;
 
-	for (i = 0; i < fb_helper->crtc_count; i++) {
-		crtc = fb_helper->crtc_info[i].mode_set.crtc;
+	list_for_each_entry(helper_crtc, &fb_helper->crtc_list, list) {
+		crtc = helper_crtc->mode_set.crtc;
 		crtc_funcs = crtc->helper_private;
 
 		red = cmap->red;
@@ -642,9 +646,9 @@ int drm_fb_helper_set_par(struct fb_info *info)
 	struct drm_fb_helper *fb_helper = info->par;
 	struct drm_device *dev = fb_helper->dev;
 	struct fb_var_screeninfo *var = &info->var;
+	struct drm_fb_helper_crtc *helper_crtc;
 	struct drm_crtc *crtc;
 	int ret;
-	int i;
 
 	if (var->pixclock != 0) {
 		DRM_ERROR("PIXEL CLOCK SET\n");
@@ -652,9 +656,9 @@ int drm_fb_helper_set_par(struct fb_info *info)
 	}
 
 	mutex_lock(&dev->mode_config.mutex);
-	for (i = 0; i < fb_helper->crtc_count; i++) {
-		crtc = fb_helper->crtc_info[i].mode_set.crtc;
-		ret = crtc->funcs->set_config(&fb_helper->crtc_info[i].mode_set);
+	list_for_each_entry(helper_crtc, &fb_helper->crtc_list, list) {
+		crtc = helper_crtc->mode_set.crtc;
+		ret = crtc->funcs->set_config(&helper_crtc->mode_set);
 		if (ret) {
 			mutex_unlock(&dev->mode_config.mutex);
 			return ret;
@@ -676,15 +680,15 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 	struct drm_fb_helper *fb_helper = info->par;
 	struct drm_device *dev = fb_helper->dev;
 	struct drm_mode_set *modeset;
+	struct drm_fb_helper_crtc *helper_crtc;
 	struct drm_crtc *crtc;
 	int ret = 0;
-	int i;
 
 	mutex_lock(&dev->mode_config.mutex);
-	for (i = 0; i < fb_helper->crtc_count; i++) {
-		crtc = fb_helper->crtc_info[i].mode_set.crtc;
+	list_for_each_entry(helper_crtc, &fb_helper->crtc_list, list) {
+		modeset = &helper_crtc->mode_set;
+		crtc = modeset->crtc;
 
-		modeset = &fb_helper->crtc_info[i].mode_set;
 
 		modeset->x = var->xoffset;
 		modeset->y = var->yoffset;
@@ -705,6 +709,7 @@ EXPORT_SYMBOL(drm_fb_helper_pan_display);
 int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 				  int preferred_bpp)
 {
+	struct drm_fb_helper_crtc *helper_crtc;
 	int new_fb = 0;
 	int crtc_count = 0;
 	int i;
@@ -755,13 +760,13 @@ int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	}
 
 	crtc_count = 0;
-	for (i = 0; i < fb_helper->crtc_count; i++) {
+	list_for_each_entry(helper_crtc, &fb_helper->crtc_list, list) {
 		struct drm_display_mode *desired_mode;
-		desired_mode = fb_helper->crtc_info[i].desired_mode;
+		desired_mode = helper_crtc->desired_mode;
 
 		if (desired_mode) {
 			if (gamma_size == 0)
-				gamma_size = fb_helper->crtc_info[i].mode_set.crtc->gamma_size;
+				gamma_size = helper_crtc->mode_set.crtc->gamma_size;
 			if (desired_mode->hdisplay < sizes.fb_width)
 				sizes.fb_width = desired_mode->hdisplay;
 			if (desired_mode->vdisplay < sizes.fb_height)
@@ -790,16 +795,15 @@ int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	info = fb_helper->fbdev;
 
 	/* set the fb pointer */
-	for (i = 0; i < fb_helper->crtc_count; i++) {
-		fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb;
-	}
+	list_for_each_entry(helper_crtc, &fb_helper->crtc_list, list)
+		helper_crtc->mode_set.fb = fb_helper->fb;
 
 	if (new_fb) {
 		info->var.pixclock = 0;
 		if (register_framebuffer(info) < 0) {
 			return -EINVAL;
 		}
-
+		drm_fb_helper_blank(FB_BLANK_UNBLANK, info);
 		printk(KERN_INFO "fb%d: %s frame buffer device\n", info->node,
 		       info->fix.id);
 
@@ -1184,12 +1188,13 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
 
 	/* select a crtc for this connector and then attempt to configure
 	   remaining connectors */
-	for (c = 0; c < fb_helper->crtc_count; c++) {
-		crtc = &fb_helper->crtc_info[c];
-
+	c = 0;
+	list_for_each_entry(crtc, &fb_helper->crtc_list, list) {
 		if ((encoder->possible_crtcs & (1 << c)) == 0) {
+			c++;
 			continue;
 		}
+		c++;
 
 		for (o = 0; o < n; o++)
 			if (best_crtcs[o] == crtc)
@@ -1224,7 +1229,7 @@ out:
 static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
-	struct drm_fb_helper_crtc **crtcs;
+	struct drm_fb_helper_crtc **crtcs, *helper_crtc;
 	struct drm_display_mode **modes;
 	struct drm_encoder *encoder;
 	struct drm_mode_set *modeset;
@@ -1264,10 +1269,8 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 
 	/* need to set the modesets up here for use later */
 	/* fill out the connector<->crtc mappings into the modesets */
-	for (i = 0; i < fb_helper->crtc_count; i++) {
-		modeset = &fb_helper->crtc_info[i].mode_set;
-		modeset->num_connectors = 0;
-	}
+	list_for_each_entry(helper_crtc, &fb_helper->crtc_list, list)
+		helper_crtc->mode_set.num_connectors = 0;
 
 	for (i = 0; i < fb_helper->connector_count; i++) {
 		struct drm_display_mode *mode = modes[i];
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 5120b01..e1e1c02 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -37,6 +37,7 @@ struct drm_fb_helper;
 struct drm_fb_helper_crtc {
 	struct drm_mode_set mode_set;
 	struct drm_display_mode *desired_mode;
+	struct list_head list;
 };
 
 struct drm_fb_helper_surface_size {
@@ -69,7 +70,7 @@ struct drm_fb_helper {
 	struct drm_device *dev;
 	struct drm_display_mode *mode;
 	int crtc_count;
-	struct drm_fb_helper_crtc *crtc_info;
+	struct list_head crtc_list;
 	int connector_count;
 	struct drm_fb_helper_connector **connector_info;
 	struct drm_fb_helper_funcs *funcs;
-- 
1.7.8.3

Re: [PATCH 09/20] drm fb_helper: use lists for crtcs.

[Dave Airlie]

On Wed, Feb 1, 2012 at 10:38 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> The fb helper uses fixed size arrays for the associated crtcs.
> This is an unnecessary limitation, so instead use a list to
> store the crtcs and allocate them dynamically.

I need more reasons on why this is a unnecessary limitation, for what?

Its a lot less cache friendly to use a linked list here for not much gain,

do you want to attach/detach crtcs from fb at runtime?

Dave.

Re: [PATCH 09/20] drm fb_helper: use lists for crtcs.

[Sascha Hauer]

On Fri, Feb 03, 2012 at 10:04:27AM +0000, Dave Airlie wrote:
> On Wed, Feb 1, 2012 at 10:38 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > The fb helper uses fixed size arrays for the associated crtcs.
> > This is an unnecessary limitation, so instead use a list to
> > store the crtcs and allocate them dynamically.
> 
> I need more reasons on why this is a unnecessary limitation, for what?
> 
> Its a lot less cache friendly to use a linked list here for not much gain,

How often do you change modes? This code is run when the user changes
virtual terminals, which is not very performance critical.

> 
> do you want to attach/detach crtcs from fb at runtime?

I am working on a mid layer to connect simple framebuffer devices to
kms and it would be convenient to just add a crtc to a drm device once
it appears. This works fine for connectors and encoders, why not also
for crtcs?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 09/20] drm fb_helper: use lists for crtcs.

[Dave Airlie]

On Sat, Feb 4, 2012 at 10:47 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Fri, Feb 03, 2012 at 10:04:27AM +0000, Dave Airlie wrote:
>> On Wed, Feb 1, 2012 at 10:38 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> > The fb helper uses fixed size arrays for the associated crtcs.
>> > This is an unnecessary limitation, so instead use a list to
>> > store the crtcs and allocate them dynamically.
>>
>> I need more reasons on why this is a unnecessary limitation, for what?
>>
>> Its a lot less cache friendly to use a linked list here for not much gain,
>
> How often do you change modes? This code is run when the user changes
> virtual terminals, which is not very performance critical.

But why make it worse unless you have a good reason?

> I am working on a mid layer to connect simple framebuffer devices to
> kms and it would be convenient to just add a crtc to a drm device once
> it appears. This works fine for connectors and encoders, why not also
> for crtcs?

I'd like to see the midlayer before changing the core too much, I hate
midlayers as they always cause more pain in the long run than they
solve,
the DRM is one of the worst examples of midlayer design and I'd rather
not propogate it any further.

Hopefully you meant to say you are working on a set of helper
functions that kms drivers for simple framebuffers can use, btw what
is going to count as a simple framebuffer device? 1 crtc/connector?
I've got to write a bunch of "simple" kms drivers but I'd really
rather this developed the other way. Write a drivers for two simple
devices, then carve the common code into helpers.

Dave.

Re: [PATCH 09/20] drm fb_helper: use lists for crtcs.

[Sascha Hauer]

On Sat, Feb 04, 2012 at 11:21:34AM +0000, Dave Airlie wrote:
> On Sat, Feb 4, 2012 at 10:47 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Fri, Feb 03, 2012 at 10:04:27AM +0000, Dave Airlie wrote:
> >> On Wed, Feb 1, 2012 at 10:38 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> > The fb helper uses fixed size arrays for the associated crtcs.
> >> > This is an unnecessary limitation, so instead use a list to
> >> > store the crtcs and allocate them dynamically.
> >>
> >> I need more reasons on why this is a unnecessary limitation, for what?
> >>
> >> Its a lot less cache friendly to use a linked list here for not much gain,
> >
> > How often do you change modes? This code is run when the user changes
> > virtual terminals, which is not very performance critical.
> 
> But why make it worse unless you have a good reason?
> 
> > I am working on a mid layer to connect simple framebuffer devices to
> > kms and it would be convenient to just add a crtc to a drm device once
> > it appears. This works fine for connectors and encoders, why not also
> > for crtcs?
> 
> I'd like to see the midlayer before changing the core too much, I hate
> midlayers as they always cause more pain in the long run than they
> solve,
> the DRM is one of the worst examples of midlayer design and I'd rather
> not propogate it any further.
> 
> Hopefully you meant to say you are working on a set of helper
> functions that kms drivers for simple framebuffers can use

midlayer is a tainted word and I regret calling it like this right
after sending my mail. Let's call it helper then ;)

> , btw what
> is going to count as a simple framebuffer device? 1 crtc/connector?

It's not limited in the count of crtcs/connectors/encoders.

> I've got to write a bunch of "simple" kms drivers but I'd really
> rather this developed the other way. Write a drivers for two simple
> devices, then carve the common code into helpers.

My goal is to write a driver for the i.MX IPU driver. I realized that
I had to duplicate most of the exynos driver. Having written several
framebuffer drivers (for i.MX1, netx, i.MX28) I know that all these
devices are not very different, so my code is written with having
these kind of devices in my mind.

Anyway, my code does not need this particular patch, so we can delay
(or drop) it.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 10/20] drm: remove now unused crtc_count parameter from drm_fb_helper_init

[Sascha Hauer]

As the crtcs are now allocated dynamically we don't need the
crtc_count parameter anymore.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_fb_helper.c           |    2 +-
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |    8 ++------
 drivers/gpu/drm/gma500/framebuffer.c      |    3 +--
 drivers/gpu/drm/i915/intel_fb.c           |    4 +---
 drivers/gpu/drm/nouveau/nouveau_fbcon.c   |    3 +--
 drivers/gpu/drm/radeon/radeon_fb.c        |    4 +---
 include/drm/drm_fb_helper.h               |    3 +--
 7 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index f292a78..231255b 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -385,7 +385,7 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
 
 int drm_fb_helper_init(struct drm_device *dev,
 		       struct drm_fb_helper *fb_helper,
-		       int crtc_count, int max_conn_count)
+		       int max_conn_count)
 {
 	struct drm_crtc *crtc;
 	struct drm_fb_helper_crtc *helper_crtc;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index d7ae29d..f9f8db2 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -294,7 +294,6 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
 	struct exynos_drm_fbdev *fbdev;
 	struct exynos_drm_private *private = dev->dev_private;
 	struct drm_fb_helper *helper;
-	unsigned int num_crtc;
 	int ret;
 
 	DRM_DEBUG_KMS("%s\n", __FILE__);
@@ -311,9 +310,7 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
 	private->fb_helper = helper = &fbdev->drm_fb_helper;
 	helper->funcs = &exynos_drm_fb_helper_funcs;
 
-	num_crtc = dev->mode_config.num_crtc;
-
-	ret = drm_fb_helper_init(dev, helper, num_crtc, MAX_CONNECTOR);
+	ret = drm_fb_helper_init(dev, helper, MAX_CONNECTOR);
 	if (ret < 0) {
 		DRM_ERROR("failed to initialize drm fb helper.\n");
 		goto err_init;
@@ -438,8 +435,7 @@ int exynos_drm_fbdev_reinit(struct drm_device *dev)
 
 		drm_fb_helper_fini(fb_helper);
 
-		ret = drm_fb_helper_init(dev, fb_helper,
-				dev->mode_config.num_crtc, MAX_CONNECTOR);
+		ret = drm_fb_helper_init(dev, fb_helper, MAX_CONNECTOR);
 		if (ret < 0) {
 			DRM_ERROR("failed to initialize drm fb helper\n");
 			return ret;
diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index 830dfdd6b..5eb185d 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -623,8 +623,7 @@ int psb_fbdev_init(struct drm_device *dev)
 	dev_priv->fbdev = fbdev;
 	fbdev->psb_fb_helper.funcs = &psb_fb_helper_funcs;
 
-	drm_fb_helper_init(dev, &fbdev->psb_fb_helper, dev_priv->ops->crtcs,
-							INTELFB_CONN_LIMIT);
+	drm_fb_helper_init(dev, &fbdev->psb_fb_helper, INTELFB_CONN_LIMIT);
 
 	drm_fb_helper_single_add_all_connectors(&fbdev->psb_fb_helper);
 	drm_fb_helper_initial_config(&fbdev->psb_fb_helper, 32);
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 571375a..a96c2ae 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -235,9 +235,7 @@ int intel_fbdev_init(struct drm_device *dev)
 	dev_priv->fbdev = ifbdev;
 	ifbdev->helper.funcs = &intel_fb_helper_funcs;
 
-	ret = drm_fb_helper_init(dev, &ifbdev->helper,
-				 dev_priv->num_pipe,
-				 INTELFB_CONN_LIMIT);
+	ret = drm_fb_helper_init(dev, &ifbdev->helper, INTELFB_CONN_LIMIT);
 	if (ret) {
 		kfree(ifbdev);
 		return ret;
diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
index 9892218..a4cc944 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
@@ -500,8 +500,7 @@ int nouveau_fbcon_init(struct drm_device *dev)
 	dev_priv->nfbdev = nfbdev;
 	nfbdev->helper.funcs = &nouveau_fbcon_helper_funcs;
 
-	ret = drm_fb_helper_init(dev, &nfbdev->helper,
-				 nv_two_heads(dev) ? 2 : 1, 4);
+	ret = drm_fb_helper_init(dev, &nfbdev->helper, 4);
 	if (ret) {
 		kfree(nfbdev);
 		return ret;
diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
index cf2bf35..6d68944 100644
--- a/drivers/gpu/drm/radeon/radeon_fb.c
+++ b/drivers/gpu/drm/radeon/radeon_fb.c
@@ -377,9 +377,7 @@ int radeon_fbdev_init(struct radeon_device *rdev)
 	rdev->mode_info.rfbdev = rfbdev;
 	rfbdev->helper.funcs = &radeon_fb_helper_funcs;
 
-	ret = drm_fb_helper_init(rdev->ddev, &rfbdev->helper,
-				 rdev->num_crtc,
-				 RADEONFB_CONN_LIMIT);
+	ret = drm_fb_helper_init(rdev->ddev, &rfbdev->helper, RADEONFB_CONN_LIMIT);
 	if (ret) {
 		kfree(rfbdev);
 		return ret;
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index e1e1c02..e1afac5 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -87,8 +87,7 @@ int drm_fb_helper_single_fb_probe(struct drm_fb_helper *helper,
 				  int preferred_bpp);
 
 int drm_fb_helper_init(struct drm_device *dev,
-		       struct drm_fb_helper *helper, int crtc_count,
-		       int max_conn);
+		       struct drm_fb_helper *helper, int max_conn);
 void drm_fb_helper_fini(struct drm_fb_helper *helper);
 int drm_fb_helper_blank(int blank, struct fb_info *info);
 int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
-- 
1.7.8.3

[PATCH 11/20] drm fb helper: add the connectors inside drm_fb_helper_initial_config

[Sascha Hauer]

drm_fb_helper_single_add_all_connectors is always called in
conjunction with drm_fb_helper_initial_config, so call
drm_fb_helper_single_add_all_connectors inside
drm_fb_helper_initial_config and make
drm_fb_helper_single_add_all_connectors static.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_fb_helper.c           |    5 +++--
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |   13 -------------
 drivers/gpu/drm/gma500/framebuffer.c      |    1 -
 drivers/gpu/drm/i915/intel_fb.c           |    1 -
 drivers/gpu/drm/nouveau/nouveau_fbcon.c   |    2 --
 drivers/gpu/drm/radeon/radeon_fb.c        |    1 -
 include/drm/drm_fb_helper.h               |    1 -
 7 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 231255b..b54298f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -44,7 +44,7 @@ MODULE_LICENSE("GPL and additional rights");
 static LIST_HEAD(kernel_fb_helper_list);
 
 /* simple single crtc case helper function */
-int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
+static int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
 	struct drm_connector *connector;
@@ -69,7 +69,6 @@ fail:
 	fb_helper->connector_count = 0;
 	return -ENOMEM;
 }
-EXPORT_SYMBOL(drm_fb_helper_single_add_all_connectors);
 
 static int drm_fb_helper_parse_command_line(struct drm_fb_helper *fb_helper)
 {
@@ -1313,6 +1312,8 @@ bool drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 	struct drm_device *dev = fb_helper->dev;
 	int count = 0;
 
+	drm_fb_helper_single_add_all_connectors(fb_helper);
+
 	/* disable all the possible outputs/crtcs before entering KMS mode */
 	drm_helper_disable_unused_functions(fb_helper->dev);
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index f9f8db2..706c906 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -316,13 +316,6 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
 		goto err_init;
 	}
 
-	ret = drm_fb_helper_single_add_all_connectors(helper);
-	if (ret < 0) {
-		DRM_ERROR("failed to register drm_fb_helper_connector.\n");
-		goto err_setup;
-
-	}
-
 	ret = drm_fb_helper_initial_config(helper, PREFERRED_BPP);
 	if (ret < 0) {
 		DRM_ERROR("failed to set up hw configuration.\n");
@@ -444,12 +437,6 @@ int exynos_drm_fbdev_reinit(struct drm_device *dev)
 		if (!list_empty(&temp_list))
 			list_replace(&temp_list, &fb_helper->kernel_fb_list);
 
-		ret = drm_fb_helper_single_add_all_connectors(fb_helper);
-		if (ret < 0) {
-			DRM_ERROR("failed to add fb helper to connectors\n");
-			goto err;
-		}
-
 		ret = drm_fb_helper_initial_config(fb_helper, PREFERRED_BPP);
 		if (ret < 0) {
 			DRM_ERROR("failed to set up hw configuration.\n");
diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index 5eb185d..c7eadaf 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -625,7 +625,6 @@ int psb_fbdev_init(struct drm_device *dev)
 
 	drm_fb_helper_init(dev, &fbdev->psb_fb_helper, INTELFB_CONN_LIMIT);
 
-	drm_fb_helper_single_add_all_connectors(&fbdev->psb_fb_helper);
 	drm_fb_helper_initial_config(&fbdev->psb_fb_helper, 32);
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index a96c2ae..8f81286 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -241,7 +241,6 @@ int intel_fbdev_init(struct drm_device *dev)
 		return ret;
 	}
 
-	drm_fb_helper_single_add_all_connectors(&ifbdev->helper);
 	drm_fb_helper_initial_config(&ifbdev->helper, 32);
 	return 0;
 }
diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
index a4cc944..01061bb 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
@@ -506,8 +506,6 @@ int nouveau_fbcon_init(struct drm_device *dev)
 		return ret;
 	}
 
-	drm_fb_helper_single_add_all_connectors(&nfbdev->helper);
-
 	if (dev_priv->vram_size <= 32 * 1024 * 1024)
 		preferred_bpp = 8;
 	else if (dev_priv->vram_size <= 64 * 1024 * 1024)
diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
index 6d68944..ed5a642 100644
--- a/drivers/gpu/drm/radeon/radeon_fb.c
+++ b/drivers/gpu/drm/radeon/radeon_fb.c
@@ -383,7 +383,6 @@ int radeon_fbdev_init(struct radeon_device *rdev)
 		return ret;
 	}
 
-	drm_fb_helper_single_add_all_connectors(&rfbdev->helper);
 	drm_fb_helper_initial_config(&rfbdev->helper, bpp_sel);
 	return 0;
 }
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index e1afac5..b989958 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -113,7 +113,6 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info);
 
 int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper);
 bool drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel);
-int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper);
 int drm_fb_helper_debug_enter(struct fb_info *info);
 int drm_fb_helper_debug_leave(struct fb_info *info);
 
-- 
1.7.8.3

[PATCH 12/20] drm crtc_helper: use list_for_each_entry

[Sascha Hauer]

list_for_each_entry_safe is for walking a list safe against removal
of entries. Here, no entries are removed, so use list_for_each_entry.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_crtc_helper.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 84a4a80..d761d12 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -44,12 +44,12 @@ module_param_named(poll, drm_kms_helper_poll, bool, 0600);
 static void drm_mode_validate_flag(struct drm_connector *connector,
 				   int flags)
 {
-	struct drm_display_mode *mode, *t;
+	struct drm_display_mode *mode;
 
 	if (flags == (DRM_MODE_FLAG_DBLSCAN | DRM_MODE_FLAG_INTERLACE))
 		return;
 
-	list_for_each_entry_safe(mode, t, &connector->modes, head) {
+	list_for_each_entry(mode, &connector->modes, head) {
 		if ((mode->flags & DRM_MODE_FLAG_INTERLACE) &&
 				!(flags & DRM_MODE_FLAG_INTERLACE))
 			mode->status = MODE_NO_INTERLACE;
@@ -87,7 +87,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 					    uint32_t maxX, uint32_t maxY)
 {
 	struct drm_device *dev = connector->dev;
-	struct drm_display_mode *mode, *t;
+	struct drm_display_mode *mode;
 	struct drm_connector_helper_funcs *connector_funcs =
 		connector->helper_private;
 	int count = 0;
@@ -96,7 +96,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
 			drm_get_connector_name(connector));
 	/* set all modes to the unverified state */
-	list_for_each_entry_safe(mode, t, &connector->modes, head)
+	list_for_each_entry(mode, &connector->modes, head)
 		mode->status = MODE_UNVERIFIED;
 
 	if (connector->force) {
@@ -136,7 +136,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		mode_flags |= DRM_MODE_FLAG_DBLSCAN;
 	drm_mode_validate_flag(connector, mode_flags);
 
-	list_for_each_entry_safe(mode, t, &connector->modes, head) {
+	list_for_each_entry(mode, &connector->modes, head) {
 		if (mode->status == MODE_OK)
 			mode->status = connector_funcs->mode_valid(connector,
 								   mode);
@@ -152,7 +152,7 @@ prune:
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] probed modes :\n", connector->base.id,
 			drm_get_connector_name(connector));
-	list_for_each_entry_safe(mode, t, &connector->modes, head) {
+	list_for_each_entry(mode, &connector->modes, head) {
 		mode->vrefresh = drm_mode_vrefresh(mode);
 
 		drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
-- 
1.7.8.3

[PATCH 13/20] drm crtc: Fix locking comments

[Sascha Hauer]

Several comments above functions say that the caller must hold the
mode_config lock, but the functions take the lock themselves. Fix
the comments.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_crtc.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index df6e413..322bc7b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -454,7 +454,7 @@ EXPORT_SYMBOL(drm_mode_remove);
  * @name: user visible name of the connector
  *
  * LOCKING:
- * Caller must hold @dev's mode_config lock.
+ * Takes mode config lock.
  *
  * Initialises a preallocated connector. Connectors should be
  * subclassed as part of driver connector objects.
@@ -497,7 +497,7 @@ EXPORT_SYMBOL(drm_connector_init);
  * @connector: connector to cleanup
  *
  * LOCKING:
- * Caller must hold @dev's mode_config lock.
+ * Takes mode config lock.
  *
  * Cleans up the connector but doesn't free the object.
  */
@@ -1314,7 +1314,7 @@ out:
  * @arg: arg from ioctl
  *
  * LOCKING:
- * Caller? (FIXME)
+ * Takes mode config lock.
  *
  * Construct a CRTC configuration structure to return to the user.
  *
@@ -1374,7 +1374,7 @@ out:
  * @arg: arg from ioctl
  *
  * LOCKING:
- * Caller? (FIXME)
+ * Takes mode config lock.
  *
  * Construct a connector configuration structure to return to the user.
  *
@@ -1556,6 +1556,9 @@ out:
  * @data: ioctl data
  * @file_priv: DRM file info
  *
+ * LOCKING:
+ * Takes mode config lock.
+ *
  * Return an plane count and set of IDs.
  */
 int drm_mode_getplane_res(struct drm_device *dev, void *data,
@@ -1602,6 +1605,9 @@ out:
  * @data: ioctl data
  * @file_priv: DRM file info
  *
+ * LOCKING:
+ * Takes mode config lock.
+ *
  * Return plane info, including formats supported, gamma size, any
  * current fb, etc.
  */
@@ -1667,6 +1673,9 @@ out:
  * @data: ioctl data*
  * @file_prive: DRM file info
  *
+ * LOCKING:
+ * Takes mode config lock.
+ *
  * Set plane info, including placement, fb, scaling, and other factors.
  * Or pass a NULL fb to disable.
  */
@@ -1797,7 +1806,7 @@ out:
  * @arg: arg from ioctl
  *
  * LOCKING:
- * Caller? (FIXME)
+ * Takes mode config lock.
  *
  * Build a new CRTC configuration based on user request.
  *
@@ -2278,7 +2287,7 @@ out:
  * @arg: arg from ioctl
  *
  * LOCKING:
- * Caller? (FIXME)
+ * Takes mode config lock.
  *
  * Lookup the FB given its ID and return info about it.
  *
-- 
1.7.8.3

[PATCH 14/20] drm: add convenience function to create an enum property

[Sascha Hauer]

Creating an enum property is a common pattern, so create
a convenience function for this and use it where appropriate.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_crtc.c                |  100 +++++++++++++---------------
 drivers/gpu/drm/i915/intel_modes.c        |   28 +++-----
 drivers/gpu/drm/nouveau/nouveau_display.c |   10 ++--
 drivers/gpu/drm/radeon/radeon_display.c   |   43 +++----------
 include/drm/drm_crtc.h                    |   14 +++-
 5 files changed, 83 insertions(+), 112 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 322bc7b..3988c62 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -38,11 +38,6 @@
 #include "drm_edid.h"
 #include "drm_fourcc.h"
 
-struct drm_prop_enum_list {
-	int type;
-	char *name;
-};
-
 /* Avoid boilerplate.  I'm tired of typing. */
 #define DRM_ENUM_NAME_FN(fnname, list)				\
 	char *fnname(int val)					\
@@ -658,7 +653,6 @@ static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
 {
 	struct drm_property *edid;
 	struct drm_property *dpms;
-	int i;
 
 	/*
 	 * Standard properties (apply to all connectors)
@@ -668,11 +662,9 @@ static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
 				   "EDID", 0);
 	dev->mode_config.edid_property = edid;
 
-	dpms = drm_property_create(dev, DRM_MODE_PROP_ENUM,
-				   "DPMS", ARRAY_SIZE(drm_dpms_enum_list));
-	for (i = 0; i < ARRAY_SIZE(drm_dpms_enum_list); i++)
-		drm_property_add_enum(dpms, i, drm_dpms_enum_list[i].type,
-				      drm_dpms_enum_list[i].name);
+	dpms = drm_property_enum_create(dev, 0,
+				   "DPMS", drm_dpms_enum_list,
+				   ARRAY_SIZE(drm_dpms_enum_list));
 	dev->mode_config.dpms_property = dpms;
 
 	return 0;
@@ -688,30 +680,21 @@ int drm_mode_create_dvi_i_properties(struct drm_device *dev)
 {
 	struct drm_property *dvi_i_selector;
 	struct drm_property *dvi_i_subconnector;
-	int i;
 
 	if (dev->mode_config.dvi_i_select_subconnector_property)
 		return 0;
 
 	dvi_i_selector =
-		drm_property_create(dev, DRM_MODE_PROP_ENUM,
+		drm_property_enum_create(dev, 0,
 				    "select subconnector",
+				    drm_dvi_i_select_enum_list,
 				    ARRAY_SIZE(drm_dvi_i_select_enum_list));
-	for (i = 0; i < ARRAY_SIZE(drm_dvi_i_select_enum_list); i++)
-		drm_property_add_enum(dvi_i_selector, i,
-				      drm_dvi_i_select_enum_list[i].type,
-				      drm_dvi_i_select_enum_list[i].name);
 	dev->mode_config.dvi_i_select_subconnector_property = dvi_i_selector;
 
-	dvi_i_subconnector =
-		drm_property_create(dev, DRM_MODE_PROP_ENUM |
-				    DRM_MODE_PROP_IMMUTABLE,
+	dvi_i_subconnector = drm_property_enum_create(dev, DRM_MODE_PROP_IMMUTABLE,
 				    "subconnector",
+				    drm_dvi_i_subconnector_enum_list,
 				    ARRAY_SIZE(drm_dvi_i_subconnector_enum_list));
-	for (i = 0; i < ARRAY_SIZE(drm_dvi_i_subconnector_enum_list); i++)
-		drm_property_add_enum(dvi_i_subconnector, i,
-				      drm_dvi_i_subconnector_enum_list[i].type,
-				      drm_dvi_i_subconnector_enum_list[i].name);
 	dev->mode_config.dvi_i_subconnector_property = dvi_i_subconnector;
 
 	return 0;
@@ -742,23 +725,17 @@ int drm_mode_create_tv_properties(struct drm_device *dev, int num_modes,
 	/*
 	 * Basic connector properties
 	 */
-	tv_selector = drm_property_create(dev, DRM_MODE_PROP_ENUM,
+	tv_selector = drm_property_enum_create(dev, 0,
 					  "select subconnector",
+					  drm_tv_select_enum_list,
 					  ARRAY_SIZE(drm_tv_select_enum_list));
-	for (i = 0; i < ARRAY_SIZE(drm_tv_select_enum_list); i++)
-		drm_property_add_enum(tv_selector, i,
-				      drm_tv_select_enum_list[i].type,
-				      drm_tv_select_enum_list[i].name);
 	dev->mode_config.tv_select_subconnector_property = tv_selector;
 
 	tv_subconnector =
-		drm_property_create(dev, DRM_MODE_PROP_ENUM |
-				    DRM_MODE_PROP_IMMUTABLE, "subconnector",
+		drm_property_enum_create(dev, DRM_MODE_PROP_IMMUTABLE,
+				    "subconnector",
+				    drm_tv_subconnector_enum_list,
 				    ARRAY_SIZE(drm_tv_subconnector_enum_list));
-	for (i = 0; i < ARRAY_SIZE(drm_tv_subconnector_enum_list); i++)
-		drm_property_add_enum(tv_subconnector, i,
-				      drm_tv_subconnector_enum_list[i].type,
-				      drm_tv_subconnector_enum_list[i].name);
 	dev->mode_config.tv_subconnector_property = tv_subconnector;
 
 	/*
@@ -845,18 +822,14 @@ EXPORT_SYMBOL(drm_mode_create_tv_properties);
 int drm_mode_create_scaling_mode_property(struct drm_device *dev)
 {
 	struct drm_property *scaling_mode;
-	int i;
 
 	if (dev->mode_config.scaling_mode_property)
 		return 0;
 
 	scaling_mode =
-		drm_property_create(dev, DRM_MODE_PROP_ENUM, "scaling mode",
+		drm_property_enum_create(dev, 0, "scaling mode",
+				drm_scaling_mode_enum_list,
 				    ARRAY_SIZE(drm_scaling_mode_enum_list));
-	for (i = 0; i < ARRAY_SIZE(drm_scaling_mode_enum_list); i++)
-		drm_property_add_enum(scaling_mode, i,
-				      drm_scaling_mode_enum_list[i].type,
-				      drm_scaling_mode_enum_list[i].name);
 
 	dev->mode_config.scaling_mode_property = scaling_mode;
 
@@ -874,18 +847,14 @@ EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
 int drm_mode_create_dithering_property(struct drm_device *dev)
 {
 	struct drm_property *dithering_mode;
-	int i;
 
 	if (dev->mode_config.dithering_mode_property)
 		return 0;
 
 	dithering_mode =
-		drm_property_create(dev, DRM_MODE_PROP_ENUM, "dithering",
+		drm_property_enum_create(dev, 0, "dithering",
+				drm_dithering_mode_enum_list,
 				    ARRAY_SIZE(drm_dithering_mode_enum_list));
-	for (i = 0; i < ARRAY_SIZE(drm_dithering_mode_enum_list); i++)
-		drm_property_add_enum(dithering_mode, i,
-				      drm_dithering_mode_enum_list[i].type,
-				      drm_dithering_mode_enum_list[i].name);
 	dev->mode_config.dithering_mode_property = dithering_mode;
 
 	return 0;
@@ -902,20 +871,15 @@ EXPORT_SYMBOL(drm_mode_create_dithering_property);
 int drm_mode_create_dirty_info_property(struct drm_device *dev)
 {
 	struct drm_property *dirty_info;
-	int i;
 
 	if (dev->mode_config.dirty_info_property)
 		return 0;
 
 	dirty_info =
-		drm_property_create(dev, DRM_MODE_PROP_ENUM |
-				    DRM_MODE_PROP_IMMUTABLE,
+		drm_property_enum_create(dev, DRM_MODE_PROP_IMMUTABLE,
 				    "dirty",
+				    drm_dirty_info_enum_list,
 				    ARRAY_SIZE(drm_dirty_info_enum_list));
-	for (i = 0; i < ARRAY_SIZE(drm_dirty_info_enum_list); i++)
-		drm_property_add_enum(dirty_info, i,
-				      drm_dirty_info_enum_list[i].type,
-				      drm_dirty_info_enum_list[i].name);
 	dev->mode_config.dirty_info_property = dirty_info;
 
 	return 0;
@@ -2629,6 +2593,34 @@ fail:
 }
 EXPORT_SYMBOL(drm_property_create);
 
+struct drm_property *drm_property_enum_create(struct drm_device *dev, int flags,
+					 const char *name,
+					 const struct drm_prop_enum_list *props,
+					 int num_values)
+{
+	struct drm_property *property;
+	int i, ret;
+
+	flags |= DRM_MODE_PROP_ENUM;
+
+	property = drm_property_create(dev, flags, name, num_values);
+	if (!property)
+		return NULL;
+
+	for (i = 0; i < num_values; i++) {
+		ret = drm_property_add_enum(property, i,
+				      props[i].type,
+				      props[i].name);
+		if (ret) {
+			drm_property_destroy(dev, property);
+			return NULL;
+		}
+	}
+
+	return property;
+}
+EXPORT_SYMBOL(drm_property_enum_create);
+
 int drm_property_add_enum(struct drm_property *property, int index,
 			  uint64_t value, const char *name)
 {
diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c
index be2c6fe..56c2fb1 100644
--- a/drivers/gpu/drm/i915/intel_modes.c
+++ b/drivers/gpu/drm/i915/intel_modes.c
@@ -83,10 +83,10 @@ int intel_ddc_get_modes(struct drm_connector *connector,
 	return ret;
 }
 
-static const char *force_audio_names[] = {
-	"off",
-	"auto",
-	"on",
+static const struct drm_prop_enum_list force_audio_names[] = {
+	{ -1, "off" },
+	{  0, "auto" },
+	{  1, "on" },
 };
 
 void
@@ -95,27 +95,24 @@ intel_attach_force_audio_property(struct drm_connector *connector)
 	struct drm_device *dev = connector->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_property *prop;
-	int i;
 
 	prop = dev_priv->force_audio_property;
 	if (prop == NULL) {
-		prop = drm_property_create(dev, DRM_MODE_PROP_ENUM,
+		prop = drm_property_enum_create(dev, 0,
 					   "audio",
+					   force_audio_names,
 					   ARRAY_SIZE(force_audio_names));
 		if (prop == NULL)
 			return;
 
-		for (i = 0; i < ARRAY_SIZE(force_audio_names); i++)
-			drm_property_add_enum(prop, i, i-1, force_audio_names[i]);
-
 		dev_priv->force_audio_property = prop;
 	}
 	drm_connector_attach_property(connector, prop, 0);
 }
 
-static const char *broadcast_rgb_names[] = {
-	"Full",
-	"Limited 16:235",
+static const struct drm_prop_enum_list broadcast_rgb_names[] = {
+	{ 0, "Full" },
+	{ 1, "Limited 16:235" },
 };
 
 void
@@ -124,19 +121,16 @@ intel_attach_broadcast_rgb_property(struct drm_connector *connector)
 	struct drm_device *dev = connector->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_property *prop;
-	int i;
 
 	prop = dev_priv->broadcast_rgb_property;
 	if (prop == NULL) {
-		prop = drm_property_create(dev, DRM_MODE_PROP_ENUM,
+		prop = drm_property_enum_create(dev, DRM_MODE_PROP_ENUM,
 					   "Broadcast RGB",
+					   broadcast_rgb_names,
 					   ARRAY_SIZE(broadcast_rgb_names));
 		if (prop == NULL)
 			return;
 
-		for (i = 0; i < ARRAY_SIZE(broadcast_rgb_names); i++)
-			drm_property_add_enum(prop, i, i, broadcast_rgb_names[i]);
-
 		dev_priv->broadcast_rgb_property = prop;
 	}
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 3cb52bc..45adade 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -155,20 +155,20 @@ static const struct drm_mode_config_funcs nouveau_mode_config_funcs = {
 };
 
 
-struct drm_prop_enum_list {
+struct nouveau_drm_prop_enum_list {
 	u8 gen_mask;
 	int type;
 	char *name;
 };
 
-static struct drm_prop_enum_list underscan[] = {
+static struct nouveau_drm_prop_enum_list underscan[] = {
 	{ 6, UNDERSCAN_AUTO, "auto" },
 	{ 6, UNDERSCAN_OFF, "off" },
 	{ 6, UNDERSCAN_ON, "on" },
 	{}
 };
 
-static struct drm_prop_enum_list dither_mode[] = {
+static struct nouveau_drm_prop_enum_list dither_mode[] = {
 	{ 7, DITHERING_MODE_AUTO, "auto" },
 	{ 7, DITHERING_MODE_OFF, "off" },
 	{ 1, DITHERING_MODE_ON, "on" },
@@ -178,7 +178,7 @@ static struct drm_prop_enum_list dither_mode[] = {
 	{}
 };
 
-static struct drm_prop_enum_list dither_depth[] = {
+static struct nouveau_drm_prop_enum_list dither_depth[] = {
 	{ 6, DITHERING_DEPTH_AUTO, "auto" },
 	{ 6, DITHERING_DEPTH_6BPC, "6 bpc" },
 	{ 6, DITHERING_DEPTH_8BPC, "8 bpc" },
@@ -186,7 +186,7 @@ static struct drm_prop_enum_list dither_depth[] = {
 };
 
 #define PROP_ENUM(p,gen,n,list) do {                                           \
-	struct drm_prop_enum_list *l = (list);                                 \
+	struct nouveau_drm_prop_enum_list *l = (list);                         \
 	int c = 0;                                                             \
 	while (l->gen_mask) {                                                  \
 		if (l->gen_mask & (1 << (gen)))                                \
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 8c49fef..eba3529 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -1124,11 +1124,6 @@ static const struct drm_mode_config_funcs radeon_mode_funcs = {
 	.output_poll_changed = radeon_output_poll_changed
 };
 
-struct drm_prop_enum_list {
-	int type;
-	char *name;
-};
-
 static struct drm_prop_enum_list radeon_tmds_pll_enum_list[] =
 {	{ 0, "driver" },
 	{ 1, "bios" },
@@ -1153,7 +1148,7 @@ static struct drm_prop_enum_list radeon_underscan_enum_list[] =
 
 static int radeon_modeset_create_props(struct radeon_device *rdev)
 {
-	int i, sz;
+	int sz;
 
 	if (rdev->is_atom_bios) {
 		rdev->mode_info.coherent_mode_property =
@@ -1170,15 +1165,9 @@ static int radeon_modeset_create_props(struct radeon_device *rdev)
 	if (!ASIC_IS_AVIVO(rdev)) {
 		sz = ARRAY_SIZE(radeon_tmds_pll_enum_list);
 		rdev->mode_info.tmds_pll_property =
-			drm_property_create(rdev->ddev,
-					    DRM_MODE_PROP_ENUM,
-					    "tmds_pll", sz);
-		for (i = 0; i < sz; i++) {
-			drm_property_add_enum(rdev->mode_info.tmds_pll_property,
-					      i,
-					      radeon_tmds_pll_enum_list[i].type,
-					      radeon_tmds_pll_enum_list[i].name);
-		}
+			drm_property_enum_create(rdev->ddev, 0,
+					    "tmds_pll",
+					    radeon_tmds_pll_enum_list, sz);
 	}
 
 	rdev->mode_info.load_detect_property =
@@ -1194,27 +1183,15 @@ static int radeon_modeset_create_props(struct radeon_device *rdev)
 
 	sz = ARRAY_SIZE(radeon_tv_std_enum_list);
 	rdev->mode_info.tv_std_property =
-		drm_property_create(rdev->ddev,
-				    DRM_MODE_PROP_ENUM,
-				    "tv standard", sz);
-	for (i = 0; i < sz; i++) {
-		drm_property_add_enum(rdev->mode_info.tv_std_property,
-				      i,
-				      radeon_tv_std_enum_list[i].type,
-				      radeon_tv_std_enum_list[i].name);
-	}
+		drm_property_enum_create(rdev->ddev, 0,
+				    "tv standard",
+				    radeon_tv_std_enum_list, sz);
 
 	sz = ARRAY_SIZE(radeon_underscan_enum_list);
 	rdev->mode_info.underscan_property =
-		drm_property_create(rdev->ddev,
-				    DRM_MODE_PROP_ENUM,
-				    "underscan", sz);
-	for (i = 0; i < sz; i++) {
-		drm_property_add_enum(rdev->mode_info.underscan_property,
-				      i,
-				      radeon_underscan_enum_list[i].type,
-				      radeon_underscan_enum_list[i].name);
-	}
+		drm_property_enum_create(rdev->ddev, 0,
+				    "underscan",
+				    radeon_underscan_enum_list, sz);
 
 	rdev->mode_info.underscan_hborder_property =
 		drm_property_create(rdev->ddev,
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8d593ad..cdbbb40 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -394,7 +394,7 @@ struct drm_crtc {
 	s64 framedur_ns, linedur_ns, pixeldur_ns;
 
 	/* if you are using the helper */
-	void *helper_private;
+	struct drm_crtc_helper_funcs *helper_private;
 };
 
 
@@ -481,7 +481,7 @@ struct drm_encoder {
 
 	struct drm_crtc *crtc;
 	const struct drm_encoder_funcs *funcs;
-	void *helper_private;
+	struct drm_encoder_helper_funcs *helper_private;
 };
 
 enum drm_connector_force {
@@ -573,7 +573,7 @@ struct drm_connector {
 	/* requested DPMS state */
 	int dpms;
 
-	void *helper_private;
+	struct drm_connector_helper_funcs *helper_private;
 
 	/* forced on connector */
 	enum drm_connector_force force;
@@ -807,6 +807,10 @@ struct drm_mode_config {
 #define obj_to_blob(x) container_of(x, struct drm_property_blob, base)
 #define obj_to_plane(x) container_of(x, struct drm_plane, base)
 
+struct drm_prop_enum_list {
+	int type;
+	char *name;
+};
 
 extern void drm_crtc_init(struct drm_device *dev,
 			  struct drm_crtc *crtc,
@@ -904,6 +908,10 @@ extern int drm_connector_attach_property(struct drm_connector *connector,
 				      struct drm_property *property, uint64_t init_val);
 extern struct drm_property *drm_property_create(struct drm_device *dev, int flags,
 						const char *name, int num_values);
+extern struct drm_property *drm_property_enum_create(struct drm_device *dev, int flags,
+					 const char *name,
+					 const struct drm_prop_enum_list *props,
+					 int num_values);
 extern void drm_property_destroy(struct drm_device *dev, struct drm_property *property);
 extern int drm_property_add_enum(struct drm_property *property, int index,
 				 uint64_t value, const char *name);
-- 
1.7.8.3

Re: [PATCH 14/20] drm: add convenience function to create an enum property

[Chris Wilson]

On Wed,  1 Feb 2012 11:38:32 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> Creating an enum property is a common pattern, so create
> a convenience function for this and use it where appropriate.

Similar naming comments apply as for drm_property_create_range. However,
I did spot something anomalous...

> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8d593ad..cdbbb40 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -394,7 +394,7 @@ struct drm_crtc {
>  	s64 framedur_ns, linedur_ns, pixeldur_ns;
>  
>  	/* if you are using the helper */
> -	void *helper_private;
> +	struct drm_crtc_helper_funcs *helper_private;
>  };
>  
>  
> @@ -481,7 +481,7 @@ struct drm_encoder {
>  
>  	struct drm_crtc *crtc;
>  	const struct drm_encoder_funcs *funcs;
> -	void *helper_private;
> +	struct drm_encoder_helper_funcs *helper_private;
>  };
>  
>  enum drm_connector_force {
> @@ -573,7 +573,7 @@ struct drm_connector {
>  	/* requested DPMS state */
>  	int dpms;
>  
> -	void *helper_private;
> +	struct drm_connector_helper_funcs *helper_private;
>  
>  	/* forced on connector */
>  	enum drm_connector_force force;

This is a separate chunk.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/20] drm: add convenience function to create an enum property

[Sascha Hauer]

On Wed, Feb 01, 2012 at 11:48:53AM +0000, Chris Wilson wrote:
> On Wed,  1 Feb 2012 11:38:32 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > Creating an enum property is a common pattern, so create
> > a convenience function for this and use it where appropriate.
> 
> Similar naming comments apply as for drm_property_create_range.

Indeed, drm_property_create_range sounds better. Will change.

> However,
> I did spot something anomalous...
> 
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 8d593ad..cdbbb40 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -394,7 +394,7 @@ struct drm_crtc {
> >  	s64 framedur_ns, linedur_ns, pixeldur_ns;
> >  
> >  	/* if you are using the helper */
> > -	void *helper_private;
> > +	struct drm_crtc_helper_funcs *helper_private;
> >  };
> >  
> >  
> > @@ -481,7 +481,7 @@ struct drm_encoder {
> >  
> >  	struct drm_crtc *crtc;
> >  	const struct drm_encoder_funcs *funcs;
> > -	void *helper_private;
> > +	struct drm_encoder_helper_funcs *helper_private;
> >  };
> >  
> >  enum drm_connector_force {
> > @@ -573,7 +573,7 @@ struct drm_connector {
> >  	/* requested DPMS state */
> >  	int dpms;
> >  
> > -	void *helper_private;
> > +	struct drm_connector_helper_funcs *helper_private;
> >  
> >  	/* forced on connector */
> >  	enum drm_connector_force force;
> 
> This is a separate chunk.

I know I created this chunk but couldn't find it in my commits. You
found it ;). yes, should be a seperate patch.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 14/20] drm: add convenience function to create an enum property

[David Airlie]

----- Original Message -----
> From: "Chris Wilson" <chris@chris-wilson.co.uk>
> To: "Sascha Hauer" <s.hauer@pengutronix.de>, dri-devel@lists.freedesktop.org
> Cc: kernel@pengutronix.de
> Sent: Wednesday, 1 February, 2012 11:48:53 AM
> Subject: Re: [PATCH 14/20] drm: add convenience function to create an enum	property
> 
> On Wed,  1 Feb 2012 11:38:32 +0100, Sascha Hauer
> <s.hauer@pengutronix.de> wrote:
> > Creating an enum property is a common pattern, so create
> > a convenience function for this and use it where appropriate.
> 
> Similar naming comments apply as for drm_property_create_range.
> However,
> I did spot something anomalous...
> 
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 8d593ad..cdbbb40 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -394,7 +394,7 @@ struct drm_crtc {
> >  	s64 framedur_ns, linedur_ns, pixeldur_ns;
> >  
> >  	/* if you are using the helper */
> > -	void *helper_private;
> > +	struct drm_crtc_helper_funcs *helper_private;
> >  };
> >  
> >  
> > @@ -481,7 +481,7 @@ struct drm_encoder {
> >  
> >  	struct drm_crtc *crtc;
> >  	const struct drm_encoder_funcs *funcs;
> > -	void *helper_private;
> > +	struct drm_encoder_helper_funcs *helper_private;
> >  };
> >  
> >  enum drm_connector_force {
> > @@ -573,7 +573,7 @@ struct drm_connector {
> >  	/* requested DPMS state */
> >  	int dpms;
> >  
> > -	void *helper_private;
> > +	struct drm_connector_helper_funcs *helper_private;
> >  
> >  	/* forced on connector */
> >  	enum drm_connector_force force;
> 
> This is a separate chunk.

And totally wrong, using the helper should remain optional, and the helper includes should not be included into the main headers.

Dave.

Re: [PATCH 14/20] drm: add convenience function to create an enum property

[Sascha Hauer]

On Wed, Feb 01, 2012 at 07:55:40AM -0500, David Airlie wrote:
> 
> 
> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > index 8d593ad..cdbbb40 100644
> > > --- a/include/drm/drm_crtc.h
> > > +++ b/include/drm/drm_crtc.h
> > > @@ -394,7 +394,7 @@ struct drm_crtc {
> > >  	s64 framedur_ns, linedur_ns, pixeldur_ns;
> > >  
> > >  	/* if you are using the helper */
> > > -	void *helper_private;
> > > +	struct drm_crtc_helper_funcs *helper_private;
> > >  };
> > >  
> > >  
> > > @@ -481,7 +481,7 @@ struct drm_encoder {
> > >  
> > >  	struct drm_crtc *crtc;
> > >  	const struct drm_encoder_funcs *funcs;
> > > -	void *helper_private;
> > > +	struct drm_encoder_helper_funcs *helper_private;
> > >  };
> > >  
> > >  enum drm_connector_force {
> > > @@ -573,7 +573,7 @@ struct drm_connector {
> > >  	/* requested DPMS state */
> > >  	int dpms;
> > >  
> > > -	void *helper_private;
> > > +	struct drm_connector_helper_funcs *helper_private;
> > >  
> > >  	/* forced on connector */
> > >  	enum drm_connector_force force;
> > 
> > This is a separate chunk.
> 
> And totally wrong, using the helper should remain optional, and the
> helper includes should not be included into the main headers.

The intention was to prevent people from thinking that they
should invent their own set of helper functions. As these
are only pointers we could also add a

struct drm_crtc_helper_funcs;
struct drm_connector_helper_funcs;
struct drm_encoder_helper_funcs;

to drm_crtc.h without having to include the helper includes.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 14/20] drm: add convenience function to create an enum property

[Daniel Vetter]

On Wed, Feb 01, 2012 at 02:05:38PM +0100, Sascha Hauer wrote:
> On Wed, Feb 01, 2012 at 07:55:40AM -0500, David Airlie wrote:
> > 
> > 
> > > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > > index 8d593ad..cdbbb40 100644
> > > > --- a/include/drm/drm_crtc.h
> > > > +++ b/include/drm/drm_crtc.h
> > > > @@ -394,7 +394,7 @@ struct drm_crtc {
> > > >  	s64 framedur_ns, linedur_ns, pixeldur_ns;
> > > >  
> > > >  	/* if you are using the helper */
> > > > -	void *helper_private;
> > > > +	struct drm_crtc_helper_funcs *helper_private;
> > > >  };
> > > >  
> > > >  
> > > > @@ -481,7 +481,7 @@ struct drm_encoder {
> > > >  
> > > >  	struct drm_crtc *crtc;
> > > >  	const struct drm_encoder_funcs *funcs;
> > > > -	void *helper_private;
> > > > +	struct drm_encoder_helper_funcs *helper_private;
> > > >  };
> > > >  
> > > >  enum drm_connector_force {
> > > > @@ -573,7 +573,7 @@ struct drm_connector {
> > > >  	/* requested DPMS state */
> > > >  	int dpms;
> > > >  
> > > > -	void *helper_private;
> > > > +	struct drm_connector_helper_funcs *helper_private;
> > > >  
> > > >  	/* forced on connector */
> > > >  	enum drm_connector_force force;
> > > 
> > > This is a separate chunk.
> > 
> > And totally wrong, using the helper should remain optional, and the
> > helper includes should not be included into the main headers.
> 
> The intention was to prevent people from thinking that they
> should invent their own set of helper functions. As these
> are only pointers we could also add a
> 
> struct drm_crtc_helper_funcs;
> struct drm_connector_helper_funcs;
> struct drm_encoder_helper_funcs;
> 
> to drm_crtc.h without having to include the helper includes.

I like this, it makes it a notch more obvious what these are for. And we
have precedence for slightly violating abstraction in this way: E.g.  the
vfs isn't splattered with void pointers just because you're free to ignore
a lot of stuff (like the pagecache) when implementing an fs ...

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 14/20] drm: add convenience function to create an enum property

[Dave Airlie]

On Wed, Feb 1, 2012 at 1:05 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, Feb 01, 2012 at 07:55:40AM -0500, David Airlie wrote:
>>
>>
>> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> > > index 8d593ad..cdbbb40 100644
>> > > --- a/include/drm/drm_crtc.h
>> > > +++ b/include/drm/drm_crtc.h
>> > > @@ -394,7 +394,7 @@ struct drm_crtc {
>> > >   s64 framedur_ns, linedur_ns, pixeldur_ns;
>> > >
>> > >   /* if you are using the helper */
>> > > - void *helper_private;
>> > > + struct drm_crtc_helper_funcs *helper_private;
>> > >  };
>> > >
>> > >
>> > > @@ -481,7 +481,7 @@ struct drm_encoder {
>> > >
>> > >   struct drm_crtc *crtc;
>> > >   const struct drm_encoder_funcs *funcs;
>> > > - void *helper_private;
>> > > + struct drm_encoder_helper_funcs *helper_private;
>> > >  };
>> > >
>> > >  enum drm_connector_force {
>> > > @@ -573,7 +573,7 @@ struct drm_connector {
>> > >   /* requested DPMS state */
>> > >   int dpms;
>> > >
>> > > - void *helper_private;
>> > > + struct drm_connector_helper_funcs *helper_private;
>> > >
>> > >   /* forced on connector */
>> > >   enum drm_connector_force force;
>> >
>> > This is a separate chunk.
>>
>> And totally wrong, using the helper should remain optional, and the
>> helper includes should not be included into the main headers.
>
> The intention was to prevent people from thinking that they
> should invent their own set of helper functions. As these
> are only pointers we could also add a
>
> struct drm_crtc_helper_funcs;
> struct drm_connector_helper_funcs;
> struct drm_encoder_helper_funcs;
>
> to drm_crtc.h without having to include the helper includes.
>

This might be better, though driver can invent their own helpers if
they need it, its the whole point of using helpers and not forcing
drivers to do stuff one single way.

Dave.

Re: [PATCH 14/20] drm: add convenience function to create an enum property

[Sascha Hauer]

On Fri, Feb 03, 2012 at 10:08:12AM +0000, Dave Airlie wrote:
> On Wed, Feb 1, 2012 at 1:05 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Wed, Feb 01, 2012 at 07:55:40AM -0500, David Airlie wrote:
> >>
> >>
> >> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >> > > index 8d593ad..cdbbb40 100644
> >> > > --- a/include/drm/drm_crtc.h
> >> > > +++ b/include/drm/drm_crtc.h
> >> > > @@ -394,7 +394,7 @@ struct drm_crtc {
> >> > >   s64 framedur_ns, linedur_ns, pixeldur_ns;
> >> > >
> >> > >   /* if you are using the helper */
> >> > > - void *helper_private;
> >> > > + struct drm_crtc_helper_funcs *helper_private;
> >> > >  };
> >> > >
> >> > >
> >> > > @@ -481,7 +481,7 @@ struct drm_encoder {
> >> > >
> >> > >   struct drm_crtc *crtc;
> >> > >   const struct drm_encoder_funcs *funcs;
> >> > > - void *helper_private;
> >> > > + struct drm_encoder_helper_funcs *helper_private;
> >> > >  };
> >> > >
> >> > >  enum drm_connector_force {
> >> > > @@ -573,7 +573,7 @@ struct drm_connector {
> >> > >   /* requested DPMS state */
> >> > >   int dpms;
> >> > >
> >> > > - void *helper_private;
> >> > > + struct drm_connector_helper_funcs *helper_private;
> >> > >
> >> > >   /* forced on connector */
> >> > >   enum drm_connector_force force;
> >> >
> >> > This is a separate chunk.
> >>
> >> And totally wrong, using the helper should remain optional, and the
> >> helper includes should not be included into the main headers.
> >
> > The intention was to prevent people from thinking that they
> > should invent their own set of helper functions. As these
> > are only pointers we could also add a
> >
> > struct drm_crtc_helper_funcs;
> > struct drm_connector_helper_funcs;
> > struct drm_encoder_helper_funcs;
> >
> > to drm_crtc.h without having to include the helper includes.
> >
> 
> This might be better, though driver can invent their own helpers if
> they need it, its the whole point of using helpers and not forcing
> drivers to do stuff one single way.

I hope to get the drivers more uniform. When people want to invent
their own helpers I think the question we have to ask is what is wrong
with the helpers and what is missing and try to fix/implement this
before adding a new set of helpers. If someone really has a good reason
to implement new helpers we can easily revert this change, but we
shouldn't motivate him to do so.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 15/20] drm: add convenience function to create an range property

[Sascha Hauer]

Creating a range property is a common pattern, so create
a convenience function for this and use it where appropriate.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_crtc.c                |   69 ++++++++++++-----------------
 drivers/gpu/drm/gma500/framebuffer.c      |    5 +--
 drivers/gpu/drm/gma500/psb_intel_sdvo.c   |   28 +++---------
 drivers/gpu/drm/i2c/ch7006_drv.c          |    5 +--
 drivers/gpu/drm/i915/intel_sdvo.c         |   30 +++---------
 drivers/gpu/drm/nouveau/nouveau_display.c |   10 +---
 drivers/gpu/drm/radeon/radeon_display.c   |   27 +++---------
 include/drm/drm_crtc.h                    |    3 +
 8 files changed, 56 insertions(+), 121 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 3988c62..1ebcedf 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -742,28 +742,16 @@ int drm_mode_create_tv_properties(struct drm_device *dev, int num_modes,
 	 * Other, TV specific properties: margins & TV modes.
 	 */
 	dev->mode_config.tv_left_margin_property =
-		drm_property_create(dev, DRM_MODE_PROP_RANGE,
-				    "left margin", 2);
-	dev->mode_config.tv_left_margin_property->values[0] = 0;
-	dev->mode_config.tv_left_margin_property->values[1] = 100;
+		drm_property_range_create(dev, 0, "left margin", 0, 100);
 
 	dev->mode_config.tv_right_margin_property =
-		drm_property_create(dev, DRM_MODE_PROP_RANGE,
-				    "right margin", 2);
-	dev->mode_config.tv_right_margin_property->values[0] = 0;
-	dev->mode_config.tv_right_margin_property->values[1] = 100;
+		drm_property_range_create(dev, 0, "right margin", 0, 100);
 
 	dev->mode_config.tv_top_margin_property =
-		drm_property_create(dev, DRM_MODE_PROP_RANGE,
-				    "top margin", 2);
-	dev->mode_config.tv_top_margin_property->values[0] = 0;
-	dev->mode_config.tv_top_margin_property->values[1] = 100;
+		drm_property_range_create(dev, 0, "top margin", 0, 100);
 
 	dev->mode_config.tv_bottom_margin_property =
-		drm_property_create(dev, DRM_MODE_PROP_RANGE,
-				    "bottom margin", 2);
-	dev->mode_config.tv_bottom_margin_property->values[0] = 0;
-	dev->mode_config.tv_bottom_margin_property->values[1] = 100;
+		drm_property_range_create(dev, 0, "bottom margin", 0, 100);
 
 	dev->mode_config.tv_mode_property =
 		drm_property_create(dev, DRM_MODE_PROP_ENUM,
@@ -773,40 +761,22 @@ int drm_mode_create_tv_properties(struct drm_device *dev, int num_modes,
 				      i, modes[i]);
 
 	dev->mode_config.tv_brightness_property =
-		drm_property_create(dev, DRM_MODE_PROP_RANGE,
-				    "brightness", 2);
-	dev->mode_config.tv_brightness_property->values[0] = 0;
-	dev->mode_config.tv_brightness_property->values[1] = 100;
+		drm_property_range_create(dev, 0, "brightness", 0, 100);
 
 	dev->mode_config.tv_contrast_property =
-		drm_property_create(dev, DRM_MODE_PROP_RANGE,
-				    "contrast", 2);
-	dev->mode_config.tv_contrast_property->values[0] = 0;
-	dev->mode_config.tv_contrast_property->values[1] = 100;
+		drm_property_range_create(dev, 0, "contrast", 0, 100);
 
 	dev->mode_config.tv_flicker_reduction_property =
-		drm_property_create(dev, DRM_MODE_PROP_RANGE,
-				    "flicker reduction", 2);
-	dev->mode_config.tv_flicker_reduction_property->values[0] = 0;
-	dev->mode_config.tv_flicker_reduction_property->values[1] = 100;
+		drm_property_range_create(dev, 0, "flicker reduction", 0, 100);
 
 	dev->mode_config.tv_overscan_property =
-		drm_property_create(dev, DRM_MODE_PROP_RANGE,
-				    "overscan", 2);
-	dev->mode_config.tv_overscan_property->values[0] = 0;
-	dev->mode_config.tv_overscan_property->values[1] = 100;
+		drm_property_range_create(dev, 0, "overscan", 0, 100);
 
 	dev->mode_config.tv_saturation_property =
-		drm_property_create(dev, DRM_MODE_PROP_RANGE,
-				    "saturation", 2);
-	dev->mode_config.tv_saturation_property->values[0] = 0;
-	dev->mode_config.tv_saturation_property->values[1] = 100;
+		drm_property_range_create(dev, 0, "saturation", 0, 100);
 
 	dev->mode_config.tv_hue_property =
-		drm_property_create(dev, DRM_MODE_PROP_RANGE,
-				    "hue", 2);
-	dev->mode_config.tv_hue_property->values[0] = 0;
-	dev->mode_config.tv_hue_property->values[1] = 100;
+		drm_property_range_create(dev, 0, "hue", 0, 100);
 
 	return 0;
 }
@@ -2621,6 +2591,25 @@ struct drm_property *drm_property_enum_create(struct drm_device *dev, int flags,
 }
 EXPORT_SYMBOL(drm_property_enum_create);
 
+struct drm_property *drm_property_range_create(struct drm_device *dev, int flags,
+					 const char *name,
+					 uint64_t min, uint64_t max)
+{
+	struct drm_property *property;
+
+	flags |= DRM_MODE_PROP_RANGE;
+
+	property = drm_property_create(dev, flags, name, 2);
+	if (!property)
+		return NULL;
+
+	property->values[0] = min;
+	property->values[1] = max;
+
+	return property;
+}
+EXPORT_SYMBOL(drm_property_range_create);
+
 int drm_property_add_enum(struct drm_property *property, int index,
 			  uint64_t value, const char *name)
 {
diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index c7eadaf..f359620 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -723,10 +723,7 @@ static int psb_create_backlight_property(struct drm_device *dev)
 	if (dev_priv->backlight_property)
 		return 0;
 
-	backlight = drm_property_create(dev, DRM_MODE_PROP_RANGE,
-							"backlight", 2);
-	backlight->values[0] = 0;
-	backlight->values[1] = 100;
+	backlight = drm_property_range_create(dev, 0, "backlight", 0, 100);
 
 	dev_priv->backlight_property = backlight;
 
diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
index 88b4297..63a11dc 100644
--- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c
+++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
@@ -2312,10 +2312,8 @@ static bool psb_intel_sdvo_tv_create_property(struct psb_intel_sdvo *psb_intel_s
 		psb_intel_sdvo_connector->max_##name = data_value[0]; \
 		psb_intel_sdvo_connector->cur_##name = response; \
 		psb_intel_sdvo_connector->name = \
-			drm_property_create(dev, DRM_MODE_PROP_RANGE, #name, 2); \
+			drm_property_range_create(dev, 0, #name, 0, data_value[0]); \
 		if (!psb_intel_sdvo_connector->name) return false; \
-		psb_intel_sdvo_connector->name->values[0] = 0; \
-		psb_intel_sdvo_connector->name->values[1] = data_value[0]; \
 		drm_connector_attach_property(connector, \
 					      psb_intel_sdvo_connector->name, \
 					      psb_intel_sdvo_connector->cur_##name); \
@@ -2349,25 +2347,19 @@ psb_intel_sdvo_create_enhance_property_tv(struct psb_intel_sdvo *psb_intel_sdvo,
 		psb_intel_sdvo_connector->left_margin = data_value[0] - response;
 		psb_intel_sdvo_connector->right_margin = psb_intel_sdvo_connector->left_margin;
 		psb_intel_sdvo_connector->left =
-			drm_property_create(dev, DRM_MODE_PROP_RANGE,
-					    "left_margin", 2);
+			drm_property_range_create(dev, 0, "left_margin", 0, data_value[0]);
 		if (!psb_intel_sdvo_connector->left)
 			return false;
 
-		psb_intel_sdvo_connector->left->values[0] = 0;
-		psb_intel_sdvo_connector->left->values[1] = data_value[0];
 		drm_connector_attach_property(connector,
 					      psb_intel_sdvo_connector->left,
 					      psb_intel_sdvo_connector->left_margin);
 
 		psb_intel_sdvo_connector->right =
-			drm_property_create(dev, DRM_MODE_PROP_RANGE,
-					    "right_margin", 2);
+			drm_property_range_create(dev, 0, "right_margin", 0, data_value[0]);
 		if (!psb_intel_sdvo_connector->right)
 			return false;
 
-		psb_intel_sdvo_connector->right->values[0] = 0;
-		psb_intel_sdvo_connector->right->values[1] = data_value[0];
 		drm_connector_attach_property(connector,
 					      psb_intel_sdvo_connector->right,
 					      psb_intel_sdvo_connector->right_margin);
@@ -2391,25 +2383,19 @@ psb_intel_sdvo_create_enhance_property_tv(struct psb_intel_sdvo *psb_intel_sdvo,
 		psb_intel_sdvo_connector->top_margin = data_value[0] - response;
 		psb_intel_sdvo_connector->bottom_margin = psb_intel_sdvo_connector->top_margin;
 		psb_intel_sdvo_connector->top =
-			drm_property_create(dev, DRM_MODE_PROP_RANGE,
-					    "top_margin", 2);
+			drm_property_range_create(dev, 0, "top_margin", 0, data_value[0]);
 		if (!psb_intel_sdvo_connector->top)
 			return false;
 
-		psb_intel_sdvo_connector->top->values[0] = 0;
-		psb_intel_sdvo_connector->top->values[1] = data_value[0];
 		drm_connector_attach_property(connector,
 					      psb_intel_sdvo_connector->top,
 					      psb_intel_sdvo_connector->top_margin);
 
 		psb_intel_sdvo_connector->bottom =
-			drm_property_create(dev, DRM_MODE_PROP_RANGE,
-					    "bottom_margin", 2);
+			drm_property_range_create(dev, 0, "bottom_margin", 0, data_value[0]);
 		if (!psb_intel_sdvo_connector->bottom)
 			return false;
 
-		psb_intel_sdvo_connector->bottom->values[0] = 0;
-		psb_intel_sdvo_connector->bottom->values[1] = data_value[0];
 		drm_connector_attach_property(connector,
 					      psb_intel_sdvo_connector->bottom,
 					      psb_intel_sdvo_connector->bottom_margin);
@@ -2438,12 +2424,10 @@ psb_intel_sdvo_create_enhance_property_tv(struct psb_intel_sdvo *psb_intel_sdvo,
 		psb_intel_sdvo_connector->max_dot_crawl = 1;
 		psb_intel_sdvo_connector->cur_dot_crawl = response & 0x1;
 		psb_intel_sdvo_connector->dot_crawl =
-			drm_property_create(dev, DRM_MODE_PROP_RANGE, "dot_crawl", 2);
+			drm_property_range_create(dev, 0, "dot_crawl", 0, 1);
 		if (!psb_intel_sdvo_connector->dot_crawl)
 			return false;
 
-		psb_intel_sdvo_connector->dot_crawl->values[0] = 0;
-		psb_intel_sdvo_connector->dot_crawl->values[1] = 1;
 		drm_connector_attach_property(connector,
 					      psb_intel_sdvo_connector->dot_crawl,
 					      psb_intel_sdvo_connector->cur_dot_crawl);
diff --git a/drivers/gpu/drm/i2c/ch7006_drv.c b/drivers/gpu/drm/i2c/ch7006_drv.c
index 07d55df..aa9be26 100644
--- a/drivers/gpu/drm/i2c/ch7006_drv.c
+++ b/drivers/gpu/drm/i2c/ch7006_drv.c
@@ -252,10 +252,7 @@ static int ch7006_encoder_create_resources(struct drm_encoder *encoder,
 
 	drm_mode_create_tv_properties(dev, NUM_TV_NORMS, ch7006_tv_norm_names);
 
-	priv->scale_property = drm_property_create(dev, DRM_MODE_PROP_RANGE,
-						   "scale", 2);
-	priv->scale_property->values[0] = 0;
-	priv->scale_property->values[1] = 2;
+	priv->scale_property = drm_property_range_create(dev, 0, "scale", 0, 2);
 
 	drm_connector_attach_property(connector, conf->tv_select_subconnector_property,
 				      priv->select_subconnector);
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index e334ec3..24d9e62 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2277,10 +2277,8 @@ static bool intel_sdvo_tv_create_property(struct intel_sdvo *intel_sdvo,
 		intel_sdvo_connector->max_##name = data_value[0]; \
 		intel_sdvo_connector->cur_##name = response; \
 		intel_sdvo_connector->name = \
-			drm_property_create(dev, DRM_MODE_PROP_RANGE, #name, 2); \
+			drm_property_range_create(dev, 0, #name, 0, data_value[0]); \
 		if (!intel_sdvo_connector->name) return false; \
-		intel_sdvo_connector->name->values[0] = 0; \
-		intel_sdvo_connector->name->values[1] = data_value[0]; \
 		drm_connector_attach_property(connector, \
 					      intel_sdvo_connector->name, \
 					      intel_sdvo_connector->cur_##name); \
@@ -2314,25 +2312,19 @@ intel_sdvo_create_enhance_property_tv(struct intel_sdvo *intel_sdvo,
 		intel_sdvo_connector->left_margin = data_value[0] - response;
 		intel_sdvo_connector->right_margin = intel_sdvo_connector->left_margin;
 		intel_sdvo_connector->left =
-			drm_property_create(dev, DRM_MODE_PROP_RANGE,
-					    "left_margin", 2);
+			drm_property_range_create(dev, 0, "left_margin", 0, data_value[0]);
 		if (!intel_sdvo_connector->left)
 			return false;
 
-		intel_sdvo_connector->left->values[0] = 0;
-		intel_sdvo_connector->left->values[1] = data_value[0];
 		drm_connector_attach_property(connector,
 					      intel_sdvo_connector->left,
 					      intel_sdvo_connector->left_margin);
 
 		intel_sdvo_connector->right =
-			drm_property_create(dev, DRM_MODE_PROP_RANGE,
-					    "right_margin", 2);
+			drm_property_range_create(dev, 0, "right_margin", 0, data_value[0]);
 		if (!intel_sdvo_connector->right)
 			return false;
 
-		intel_sdvo_connector->right->values[0] = 0;
-		intel_sdvo_connector->right->values[1] = data_value[0];
 		drm_connector_attach_property(connector,
 					      intel_sdvo_connector->right,
 					      intel_sdvo_connector->right_margin);
@@ -2356,25 +2348,21 @@ intel_sdvo_create_enhance_property_tv(struct intel_sdvo *intel_sdvo,
 		intel_sdvo_connector->top_margin = data_value[0] - response;
 		intel_sdvo_connector->bottom_margin = intel_sdvo_connector->top_margin;
 		intel_sdvo_connector->top =
-			drm_property_create(dev, DRM_MODE_PROP_RANGE,
-					    "top_margin", 2);
+			drm_property_range_create(dev, 0,
+					    "top_margin", 0, data_value[0]);
 		if (!intel_sdvo_connector->top)
 			return false;
 
-		intel_sdvo_connector->top->values[0] = 0;
-		intel_sdvo_connector->top->values[1] = data_value[0];
 		drm_connector_attach_property(connector,
 					      intel_sdvo_connector->top,
 					      intel_sdvo_connector->top_margin);
 
 		intel_sdvo_connector->bottom =
-			drm_property_create(dev, DRM_MODE_PROP_RANGE,
-					    "bottom_margin", 2);
+			drm_property_range_create(dev, 0,
+					    "bottom_margin", 0, data_value[0]);
 		if (!intel_sdvo_connector->bottom)
 			return false;
 
-		intel_sdvo_connector->bottom->values[0] = 0;
-		intel_sdvo_connector->bottom->values[1] = data_value[0];
 		drm_connector_attach_property(connector,
 					      intel_sdvo_connector->bottom,
 					      intel_sdvo_connector->bottom_margin);
@@ -2403,12 +2391,10 @@ intel_sdvo_create_enhance_property_tv(struct intel_sdvo *intel_sdvo,
 		intel_sdvo_connector->max_dot_crawl = 1;
 		intel_sdvo_connector->cur_dot_crawl = response & 0x1;
 		intel_sdvo_connector->dot_crawl =
-			drm_property_create(dev, DRM_MODE_PROP_RANGE, "dot_crawl", 2);
+			drm_property_range_create(dev, 0, "dot_crawl", 0, 1);
 		if (!intel_sdvo_connector->dot_crawl)
 			return false;
 
-		intel_sdvo_connector->dot_crawl->values[0] = 0;
-		intel_sdvo_connector->dot_crawl->values[1] = 1;
 		drm_connector_attach_property(connector,
 					      intel_sdvo_connector->dot_crawl,
 					      intel_sdvo_connector->cur_dot_crawl);
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 45adade..a16d171 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -271,16 +271,10 @@ nouveau_display_create(struct drm_device *dev)
 	PROP_ENUM(disp->underscan_property, gen, "underscan", underscan);
 
 	disp->underscan_hborder_property =
-		drm_property_create(dev, DRM_MODE_PROP_RANGE,
-				    "underscan hborder", 2);
-	disp->underscan_hborder_property->values[0] = 0;
-	disp->underscan_hborder_property->values[1] = 128;
+		drm_property_range_create(dev, 0, "underscan hborder", 0, 128);
 
 	disp->underscan_vborder_property =
-		drm_property_create(dev, DRM_MODE_PROP_RANGE,
-				    "underscan vborder", 2);
-	disp->underscan_vborder_property->values[0] = 0;
-	disp->underscan_vborder_property->values[1] = 128;
+		drm_property_range_create(dev, 0, "underscan vborder", 0, 128);
 
 	dev->mode_config.funcs = (void *)&nouveau_mode_config_funcs;
 	dev->mode_config.fb_base = pci_resource_start(dev->pdev, 1);
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index eba3529..fd59af9 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -1152,14 +1152,9 @@ static int radeon_modeset_create_props(struct radeon_device *rdev)
 
 	if (rdev->is_atom_bios) {
 		rdev->mode_info.coherent_mode_property =
-			drm_property_create(rdev->ddev,
-					    DRM_MODE_PROP_RANGE,
-					    "coherent", 2);
+			drm_property_range_create(rdev->ddev, 0 , "coherent", 0, 1);
 		if (!rdev->mode_info.coherent_mode_property)
 			return -ENOMEM;
-
-		rdev->mode_info.coherent_mode_property->values[0] = 0;
-		rdev->mode_info.coherent_mode_property->values[1] = 1;
 	}
 
 	if (!ASIC_IS_AVIVO(rdev)) {
@@ -1171,13 +1166,9 @@ static int radeon_modeset_create_props(struct radeon_device *rdev)
 	}
 
 	rdev->mode_info.load_detect_property =
-		drm_property_create(rdev->ddev,
-				    DRM_MODE_PROP_RANGE,
-				    "load detection", 2);
+		drm_property_range_create(rdev->ddev, 0, "load detection", 0, 1);
 	if (!rdev->mode_info.load_detect_property)
 		return -ENOMEM;
-	rdev->mode_info.load_detect_property->values[0] = 0;
-	rdev->mode_info.load_detect_property->values[1] = 1;
 
 	drm_mode_create_scaling_mode_property(rdev->ddev);
 
@@ -1194,22 +1185,16 @@ static int radeon_modeset_create_props(struct radeon_device *rdev)
 				    radeon_underscan_enum_list, sz);
 
 	rdev->mode_info.underscan_hborder_property =
-		drm_property_create(rdev->ddev,
-					DRM_MODE_PROP_RANGE,
-					"underscan hborder", 2);
+		drm_property_range_create(rdev->ddev, 0,
+					"underscan hborder", 0, 128);
 	if (!rdev->mode_info.underscan_hborder_property)
 		return -ENOMEM;
-	rdev->mode_info.underscan_hborder_property->values[0] = 0;
-	rdev->mode_info.underscan_hborder_property->values[1] = 128;
 
 	rdev->mode_info.underscan_vborder_property =
-		drm_property_create(rdev->ddev,
-					DRM_MODE_PROP_RANGE,
-					"underscan vborder", 2);
+		drm_property_range_create(rdev->ddev, 0,
+					"underscan vborder", 0, 128);
 	if (!rdev->mode_info.underscan_vborder_property)
 		return -ENOMEM;
-	rdev->mode_info.underscan_vborder_property->values[0] = 0;
-	rdev->mode_info.underscan_vborder_property->values[1] = 128;
 
 	return 0;
 }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index cdbbb40..c03ad9a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -912,6 +912,9 @@ extern struct drm_property *drm_property_enum_create(struct drm_device *dev, int
 					 const char *name,
 					 const struct drm_prop_enum_list *props,
 					 int num_values);
+struct drm_property *drm_property_range_create(struct drm_device *dev, int flags,
+					 const char *name,
+					 uint64_t min, uint64_t max);
 extern void drm_property_destroy(struct drm_device *dev, struct drm_property *property);
 extern int drm_property_add_enum(struct drm_property *property, int index,
 				 uint64_t value, const char *name);
-- 
1.7.8.3

Re: [PATCH 15/20] drm: add convenience function to create an range property

[Chris Wilson]

On Wed,  1 Feb 2012 11:38:33 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> Creating a range property is a common pattern, so create
> a convenience function for this and use it where appropriate.

This is pure bikeshedding, but I would prefer
drm_property_create_range as the object being created is the
drm_property and this is just a variation upon the creation theme.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 16/20] drm: store connector properties in list

[Sascha Hauer]

Storing the properties associated to a connector in a
list allows us to drop the current limitation on a
maximum number of properties per connector.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_crtc.c |  109 ++++++++++++++++++++-----------------------
 include/drm/drm_crtc.h     |    5 +-
 2 files changed, 53 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 1ebcedf..b815e69 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -140,6 +140,12 @@ struct drm_conn_prop_enum_list {
 	int count;
 };
 
+struct drm_connector_property {
+	struct drm_property *base;
+	uint64_t val;
+	struct list_head list;
+};
+
 /*
  * Connector and encoder types.
  */
@@ -470,6 +476,7 @@ void drm_connector_init(struct drm_device *dev,
 	INIT_LIST_HEAD(&connector->user_modes);
 	INIT_LIST_HEAD(&connector->probed_modes);
 	INIT_LIST_HEAD(&connector->modes);
+	INIT_LIST_HEAD(&connector->properties);
 	connector->edid_blob_ptr = NULL;
 
 	list_add_tail(&connector->head, &dev->mode_config.connector_list);
@@ -954,6 +961,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 	struct drm_framebuffer *fb, *fbt;
 	struct drm_property *property, *pt;
 	struct drm_plane *plane, *plt;
+	struct drm_connector_property *cprop, *cpt;
 
 	list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list,
 				 head) {
@@ -962,6 +970,10 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 
 	list_for_each_entry_safe(connector, ot,
 				 &dev->mode_config.connector_list, head) {
+		list_for_each_entry_safe(cprop, cpt, &connector->properties, list) {
+			list_del(&cprop->list);
+			kfree(cprop);
+		}
 		connector->funcs->destroy(connector);
 	}
 
@@ -1324,6 +1336,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	struct drm_mode_object *obj;
 	struct drm_connector *connector;
 	struct drm_display_mode *mode;
+	struct drm_connector_property *cprop;
 	int mode_count = 0;
 	int props_count = 0;
 	int encoders_count = 0;
@@ -1353,11 +1366,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	}
 	connector = obj_to_connector(obj);
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_PROPERTY; i++) {
-		if (connector->property_ids[i] != 0) {
-			props_count++;
-		}
-	}
+	list_for_each_entry(cprop, &connector->properties, list)
+		props_count++;
 
 	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
 		if (connector->encoder_ids[i] != 0) {
@@ -1410,21 +1420,17 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 		copied = 0;
 		prop_ptr = (uint32_t __user *)(unsigned long)(out_resp->props_ptr);
 		prop_values = (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr);
-		for (i = 0; i < DRM_CONNECTOR_MAX_PROPERTY; i++) {
-			if (connector->property_ids[i] != 0) {
-				if (put_user(connector->property_ids[i],
-					     prop_ptr + copied)) {
-					ret = -EFAULT;
-					goto out;
-				}
+		list_for_each_entry(cprop, &connector->properties, list) {
+			if (put_user(cprop->base->base.id, prop_ptr + copied)) {
+				ret = -EFAULT;
+				goto out;
+			}
 
-				if (put_user(connector->property_values[i],
-					     prop_values + copied)) {
-					ret = -EFAULT;
-					goto out;
-				}
-				copied++;
+			if (put_user(cprop->val, prop_values + copied)) {
+				ret = -EFAULT;
+				goto out;
 			}
+			copied++;
 		}
 	}
 	out_resp->count_props = props_count;
@@ -2662,18 +2668,16 @@ EXPORT_SYMBOL(drm_property_destroy);
 int drm_connector_attach_property(struct drm_connector *connector,
 			       struct drm_property *property, uint64_t init_val)
 {
-	int i;
+	struct drm_connector_property *cprop;
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_PROPERTY; i++) {
-		if (connector->property_ids[i] == 0) {
-			connector->property_ids[i] = property->base.id;
-			connector->property_values[i] = init_val;
-			break;
-		}
-	}
+	cprop = kzalloc(sizeof(*cprop), GFP_KERNEL);
+	if (!cprop)
+		return -ENOMEM;
+
+	cprop->val = init_val;
+	cprop->base = property;
+	list_add_tail(&cprop->list, &connector->properties);
 
-	if (i == DRM_CONNECTOR_MAX_PROPERTY)
-		return -EINVAL;
 	return 0;
 }
 EXPORT_SYMBOL(drm_connector_attach_property);
@@ -2681,36 +2685,32 @@ EXPORT_SYMBOL(drm_connector_attach_property);
 int drm_connector_property_set_value(struct drm_connector *connector,
 				  struct drm_property *property, uint64_t value)
 {
-	int i;
+	struct drm_connector_property *cprop;
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_PROPERTY; i++) {
-		if (connector->property_ids[i] == property->base.id) {
-			connector->property_values[i] = value;
-			break;
+	list_for_each_entry(cprop, &connector->properties, list) {
+		if (cprop->base->base.id == property->base.id) {
+			cprop->val = value;
+			return 0;
 		}
 	}
 
-	if (i == DRM_CONNECTOR_MAX_PROPERTY)
-		return -EINVAL;
-	return 0;
+	return -EINVAL;
 }
 EXPORT_SYMBOL(drm_connector_property_set_value);
 
 int drm_connector_property_get_value(struct drm_connector *connector,
 				  struct drm_property *property, uint64_t *val)
 {
-	int i;
+	struct drm_connector_property *cprop;
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_PROPERTY; i++) {
-		if (connector->property_ids[i] == property->base.id) {
-			*val = connector->property_values[i];
-			break;
+	list_for_each_entry(cprop, &connector->properties, list) {
+		if (cprop->base->base.id == property->base.id) {
+			*val = cprop->val;
+			return 0;
 		}
 	}
 
-	if (i == DRM_CONNECTOR_MAX_PROPERTY)
-		return -EINVAL;
-	return 0;
+	return -EINVAL;
 }
 EXPORT_SYMBOL(drm_connector_property_get_value);
 
@@ -2916,6 +2916,7 @@ int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
 	struct drm_mode_connector_set_property *out_resp = data;
 	struct drm_mode_object *obj;
 	struct drm_property *property;
+	struct drm_connector_property *cprop;
 	struct drm_connector *connector;
 	int ret = -EINVAL;
 	int i;
@@ -2931,20 +2932,12 @@ int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
 	}
 	connector = obj_to_connector(obj);
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_PROPERTY; i++) {
-		if (connector->property_ids[i] == out_resp->prop_id)
-			break;
-	}
-
-	if (i == DRM_CONNECTOR_MAX_PROPERTY) {
-		goto out;
-	}
-
-	obj = drm_mode_object_find(dev, out_resp->prop_id, DRM_MODE_OBJECT_PROPERTY);
-	if (!obj) {
-		goto out;
-	}
-	property = obj_to_property(obj);
+	list_for_each_entry(cprop, &connector->properties, list)
+		if (cprop->base->base.id == out_resp->prop_id)
+			goto found;
+	goto out;
+found:
+	property = cprop->base;
 
 	if (property->flags & DRM_MODE_PROP_IMMUTABLE)
 		goto out;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c03ad9a..40f2107 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -451,7 +451,6 @@ struct drm_encoder_funcs {
 };
 
 #define DRM_CONNECTOR_MAX_UMODES 16
-#define DRM_CONNECTOR_MAX_PROPERTY 16
 #define DRM_CONNECTOR_LEN 32
 #define DRM_CONNECTOR_MAX_ENCODER 3
 
@@ -565,8 +564,8 @@ struct drm_connector {
 
 	struct list_head user_modes;
 	struct drm_property_blob *edid_blob_ptr;
-	u32 property_ids[DRM_CONNECTOR_MAX_PROPERTY];
-	uint64_t property_values[DRM_CONNECTOR_MAX_PROPERTY];
+
+	struct list_head properties;
 
 	uint8_t polled; /* DRM_CONNECTOR_POLL_* */
 
-- 
1.7.8.3

[PATCH 17/20] drm: remove checks for same value in set_prop

[Sascha Hauer]

The drivers currently check in set_property whether the property is
unchanged. move this check into the core and do not bother the drivers
with checking for unchanged properties.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/drm_crtc.c                  |    5 ++++
 drivers/gpu/drm/gma500/cdv_intel_lvds.c     |   10 --------
 drivers/gpu/drm/gma500/psb_intel_lvds.c     |   11 ---------
 drivers/gpu/drm/gma500/psb_intel_sdvo.c     |   12 ----------
 drivers/gpu/drm/i915/intel_dp.c             |    6 -----
 drivers/gpu/drm/i915/intel_tv.c             |   27 ++++------------------
 drivers/gpu/drm/nouveau/nouveau_connector.c |   32 ++++++++++----------------
 drivers/gpu/drm/radeon/radeon_connectors.c  |   24 ++++++-------------
 8 files changed, 30 insertions(+), 97 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b815e69..e03d4b8 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2961,6 +2961,11 @@ found:
 		}
 	}
 
+	if (cprop->val == out_resp->value) {
+		ret = 0;
+		goto out;
+	}
+
 	/* Do DPMS ourselves */
 	if (property == connector->dev->mode_config.dpms_property) {
 		if (connector->funcs->dpms)
diff --git a/drivers/gpu/drm/gma500/cdv_intel_lvds.c b/drivers/gpu/drm/gma500/cdv_intel_lvds.c
index 50e744b..7569e8e 100644
--- a/drivers/gpu/drm/gma500/cdv_intel_lvds.c
+++ b/drivers/gpu/drm/gma500/cdv_intel_lvds.c
@@ -457,8 +457,6 @@ int cdv_intel_lvds_set_property(struct drm_connector *connector,
 	if (!strcmp(property->name, "scaling mode") && encoder) {
 		struct psb_intel_crtc *crtc =
 					to_psb_intel_crtc(encoder->crtc);
-		uint64_t curValue;
-
 		if (!crtc)
 			return -1;
 
@@ -473,14 +471,6 @@ int cdv_intel_lvds_set_property(struct drm_connector *connector,
 			return -1;
 		}
 
-		if (drm_connector_property_get_value(connector,
-						     property,
-						     &curValue))
-			return -1;
-
-		if (curValue == value)
-			return 0;
-
 		if (drm_connector_property_set_value(connector,
 							property,
 							value))
diff --git a/drivers/gpu/drm/gma500/psb_intel_lvds.c b/drivers/gpu/drm/gma500/psb_intel_lvds.c
index a25e4ca..7c9498ea 100644
--- a/drivers/gpu/drm/gma500/psb_intel_lvds.c
+++ b/drivers/gpu/drm/gma500/psb_intel_lvds.c
@@ -587,8 +587,6 @@ int psb_intel_lvds_set_property(struct drm_connector *connector,
 	if (!strcmp(property->name, "scaling mode")) {
 		struct psb_intel_crtc *crtc =
 					to_psb_intel_crtc(encoder->crtc);
-		uint64_t curval;
-
 		if (!crtc)
 			goto set_prop_error;
 
@@ -603,14 +601,6 @@ int psb_intel_lvds_set_property(struct drm_connector *connector,
 			goto set_prop_error;
 		}
 
-		if (drm_connector_property_get_value(connector,
-						     property,
-						     &curval))
-			goto set_prop_error;
-
-		if (curval == value)
-			goto set_prop_done;
-
 		if (drm_connector_property_set_value(connector,
 							property,
 							value))
@@ -647,7 +637,6 @@ int psb_intel_lvds_set_property(struct drm_connector *connector,
 		hfuncs->dpms(encoder, value);
 	}
 
-set_prop_done:
 	return 0;
 set_prop_error:
 	return -1;
diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
index 63a11dc..b9c6da9 100644
--- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c
+++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
@@ -1751,10 +1751,6 @@ psb_intel_sdvo_set_property(struct drm_connector *connector,
 		if (val >= TV_FORMAT_NUM)
 			return -EINVAL;
 
-		if (psb_intel_sdvo->tv_format_index ==
-		    psb_intel_sdvo_connector->tv_format_supported[val])
-			return 0;
-
 		psb_intel_sdvo->tv_format_index = psb_intel_sdvo_connector->tv_format_supported[val];
 		goto done;
 	} else if (IS_TV_OR_LVDS(psb_intel_sdvo_connector)) {
@@ -1762,8 +1758,6 @@ psb_intel_sdvo_set_property(struct drm_connector *connector,
 		if (psb_intel_sdvo_connector->left == property) {
 			drm_connector_property_set_value(connector,
 							 psb_intel_sdvo_connector->right, val);
-			if (psb_intel_sdvo_connector->left_margin == temp_value)
-				return 0;
 
 			psb_intel_sdvo_connector->left_margin = temp_value;
 			psb_intel_sdvo_connector->right_margin = temp_value;
@@ -1774,8 +1768,6 @@ psb_intel_sdvo_set_property(struct drm_connector *connector,
 		} else if (psb_intel_sdvo_connector->right == property) {
 			drm_connector_property_set_value(connector,
 							 psb_intel_sdvo_connector->left, val);
-			if (psb_intel_sdvo_connector->right_margin == temp_value)
-				return 0;
 
 			psb_intel_sdvo_connector->left_margin = temp_value;
 			psb_intel_sdvo_connector->right_margin = temp_value;
@@ -1786,8 +1778,6 @@ psb_intel_sdvo_set_property(struct drm_connector *connector,
 		} else if (psb_intel_sdvo_connector->top == property) {
 			drm_connector_property_set_value(connector,
 							 psb_intel_sdvo_connector->bottom, val);
-			if (psb_intel_sdvo_connector->top_margin == temp_value)
-				return 0;
 
 			psb_intel_sdvo_connector->top_margin = temp_value;
 			psb_intel_sdvo_connector->bottom_margin = temp_value;
@@ -1798,8 +1788,6 @@ psb_intel_sdvo_set_property(struct drm_connector *connector,
 		} else if (psb_intel_sdvo_connector->bottom == property) {
 			drm_connector_property_set_value(connector,
 							 psb_intel_sdvo_connector->top, val);
-			if (psb_intel_sdvo_connector->bottom_margin == temp_value)
-				return 0;
 
 			psb_intel_sdvo_connector->top_margin = temp_value;
 			psb_intel_sdvo_connector->bottom_margin = temp_value;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index db3b461..0024b59 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2223,9 +2223,6 @@ intel_dp_set_property(struct drm_connector *connector,
 		int i = val;
 		bool has_audio;
 
-		if (i == intel_dp->force_audio)
-			return 0;
-
 		intel_dp->force_audio = i;
 
 		if (i == 0)
@@ -2241,9 +2238,6 @@ intel_dp_set_property(struct drm_connector *connector,
 	}
 
 	if (property == dev_priv->broadcast_rgb_property) {
-		if (val == !!intel_dp->color_range)
-			return 0;
-
 		intel_dp->color_range = val ? DP_COLOR_RANGE_16_235 : 0;
 		goto done;
 	}
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 1571be3..6eb11fe 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1380,44 +1380,27 @@ intel_tv_set_property(struct drm_connector *connector, struct drm_property *prop
 	struct intel_tv *intel_tv = intel_attached_tv(connector);
 	struct drm_crtc *crtc = intel_tv->base.base.crtc;
 	int ret = 0;
-	bool changed = false;
 
 	ret = drm_connector_property_set_value(connector, property, val);
 	if (ret < 0)
 		goto out;
 
-	if (property == dev->mode_config.tv_left_margin_property &&
-		intel_tv->margin[TV_MARGIN_LEFT] != val) {
+	if (property == dev->mode_config.tv_left_margin_property) {
 		intel_tv->margin[TV_MARGIN_LEFT] = val;
-		changed = true;
-	} else if (property == dev->mode_config.tv_right_margin_property &&
-		intel_tv->margin[TV_MARGIN_RIGHT] != val) {
+	} else if (property == dev->mode_config.tv_right_margin_property) {
 		intel_tv->margin[TV_MARGIN_RIGHT] = val;
-		changed = true;
-	} else if (property == dev->mode_config.tv_top_margin_property &&
-		intel_tv->margin[TV_MARGIN_TOP] != val) {
+	} else if (property == dev->mode_config.tv_top_margin_property) {
 		intel_tv->margin[TV_MARGIN_TOP] = val;
-		changed = true;
-	} else if (property == dev->mode_config.tv_bottom_margin_property &&
-		intel_tv->margin[TV_MARGIN_BOTTOM] != val) {
+	} else if (property == dev->mode_config.tv_bottom_margin_property) {
 		intel_tv->margin[TV_MARGIN_BOTTOM] = val;
-		changed = true;
 	} else if (property == dev->mode_config.tv_mode_property) {
-		if (val >= ARRAY_SIZE(tv_modes)) {
-			ret = -EINVAL;
-			goto out;
-		}
-		if (!strcmp(intel_tv->tv_format, tv_modes[val].name))
-			goto out;
-
 		intel_tv->tv_format = tv_modes[val].name;
-		changed = true;
 	} else {
 		ret = -EINVAL;
 		goto out;
 	}
 
-	if (changed && crtc)
+	if (crtc)
 		drm_crtc_helper_set_mode(crtc, &crtc->mode, crtc->x,
 				crtc->y, crtc->fb);
 out:
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index f3ce34b..c149943 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -467,39 +467,31 @@ nouveau_connector_set_property(struct drm_connector *connector,
 
 	/* Underscan */
 	if (property == disp->underscan_property) {
-		if (nv_connector->underscan != value) {
-			nv_connector->underscan = value;
-			if (!nv_crtc || !nv_crtc->set_scale)
-				return 0;
+		nv_connector->underscan = value;
+		if (!nv_crtc || !nv_crtc->set_scale)
+			return 0;
 
-			return nv_crtc->set_scale(nv_crtc, true);
-		}
+		return nv_crtc->set_scale(nv_crtc, true);
 
 		return 0;
 	}
 
 	if (property == disp->underscan_hborder_property) {
-		if (nv_connector->underscan_hborder != value) {
-			nv_connector->underscan_hborder = value;
-			if (!nv_crtc || !nv_crtc->set_scale)
-				return 0;
+		nv_connector->underscan_hborder = value;
+		if (!nv_crtc || !nv_crtc->set_scale)
+			return 0;
 
-			return nv_crtc->set_scale(nv_crtc, true);
-		}
+		return nv_crtc->set_scale(nv_crtc, true);
 
 		return 0;
 	}
 
 	if (property == disp->underscan_vborder_property) {
-		if (nv_connector->underscan_vborder != value) {
-			nv_connector->underscan_vborder = value;
-			if (!nv_crtc || !nv_crtc->set_scale)
-				return 0;
-
-			return nv_crtc->set_scale(nv_crtc, true);
-		}
+		nv_connector->underscan_vborder = value;
+		if (!nv_crtc || !nv_crtc->set_scale)
+			return 0;
 
-		return 0;
+		return nv_crtc->set_scale(nv_crtc, true);
 	}
 
 	/* Dithering */
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
index e7cb3ab..b0d2dcd 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -319,10 +319,8 @@ int radeon_connector_set_property(struct drm_connector *connector, struct drm_pr
 
 		dig = radeon_encoder->enc_priv;
 		new_coherent_mode = val ? true : false;
-		if (dig->coherent_mode != new_coherent_mode) {
-			dig->coherent_mode = new_coherent_mode;
-			radeon_property_change_mode(&radeon_encoder->base);
-		}
+		dig->coherent_mode = new_coherent_mode;
+		radeon_property_change_mode(&radeon_encoder->base);
 	}
 
 	if (property == rdev->mode_info.underscan_property) {
@@ -333,10 +331,8 @@ int radeon_connector_set_property(struct drm_connector *connector, struct drm_pr
 
 		radeon_encoder = to_radeon_encoder(encoder);
 
-		if (radeon_encoder->underscan_type != val) {
-			radeon_encoder->underscan_type = val;
-			radeon_property_change_mode(&radeon_encoder->base);
-		}
+		radeon_encoder->underscan_type = val;
+		radeon_property_change_mode(&radeon_encoder->base);
 	}
 
 	if (property == rdev->mode_info.underscan_hborder_property) {
@@ -347,10 +343,8 @@ int radeon_connector_set_property(struct drm_connector *connector, struct drm_pr
 
 		radeon_encoder = to_radeon_encoder(encoder);
 
-		if (radeon_encoder->underscan_hborder != val) {
-			radeon_encoder->underscan_hborder = val;
-			radeon_property_change_mode(&radeon_encoder->base);
-		}
+		radeon_encoder->underscan_hborder = val;
+		radeon_property_change_mode(&radeon_encoder->base);
 	}
 
 	if (property == rdev->mode_info.underscan_vborder_property) {
@@ -361,10 +355,8 @@ int radeon_connector_set_property(struct drm_connector *connector, struct drm_pr
 
 		radeon_encoder = to_radeon_encoder(encoder);
 
-		if (radeon_encoder->underscan_vborder != val) {
-			radeon_encoder->underscan_vborder = val;
-			radeon_property_change_mode(&radeon_encoder->base);
-		}
+		radeon_encoder->underscan_vborder = val;
+		radeon_property_change_mode(&radeon_encoder->base);
 	}
 
 	if (property == rdev->mode_info.tv_std_property) {
-- 
1.7.8.3

Re: [PATCH 17/20] drm: remove checks for same value in set_prop

[Chris Wilson]

On Wed,  1 Feb 2012 11:38:35 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> The drivers currently check in set_property whether the property is
> unchanged. move this check into the core and do not bother the drivers
> with checking for unchanged properties.

This patch seems to have functional side-effects beyond the description.

For example,
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index db3b461..0024b59 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2223,9 +2223,6 @@ intel_dp_set_property(struct drm_connector *connector,
>  		int i = val;
>  		bool has_audio;
>  
> -		if (i == intel_dp->force_audio)
> -			return 0;
> -

Here we are checking against the current value of the intel_dp, which
may in theory be modified elsewhere as well, and avoiding the modeswitch
if it is unnecessary. 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 17/20] drm: remove checks for same value in set_prop

[Sascha Hauer]

On Wed, Feb 01, 2012 at 11:55:44AM +0000, Chris Wilson wrote:
> On Wed,  1 Feb 2012 11:38:35 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > The drivers currently check in set_property whether the property is
> > unchanged. move this check into the core and do not bother the drivers
> > with checking for unchanged properties.
> 
> This patch seems to have functional side-effects beyond the description.
> 
> For example,
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index db3b461..0024b59 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2223,9 +2223,6 @@ intel_dp_set_property(struct drm_connector *connector,
> >  		int i = val;
> >  		bool has_audio;
> >  
> > -		if (i == intel_dp->force_audio)
> > -			return 0;
> > -
> 
> Here we are checking against the current value of the intel_dp, which
> may in theory be modified elsewhere as well, and avoiding the modeswitch
> if it is unnecessary.

In case of force_audio I just checked that the current value is not
changed elsewhere, but I must admit that I haven't checked this for
all other properties. Would the patch be ok if I review for side
effects?

BTW if someone changes the underlying variable of a property outside
of the .set_property callback there are likely problems elsewhere
because without calling drm_connector_property_set_value the values
of the variable and the property backing store will be inconsistent.
The calls to drm_connector_property_set_value in turn are quite easy
reviewable, those are very few.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 17/20] drm: remove checks for same value in set_prop

[Chris Wilson]

On Wed, 1 Feb 2012 13:13:44 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, Feb 01, 2012 at 11:55:44AM +0000, Chris Wilson wrote:
> > On Wed,  1 Feb 2012 11:38:35 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > The drivers currently check in set_property whether the property is
> > > unchanged. move this check into the core and do not bother the drivers
> > > with checking for unchanged properties.
> > 
> > This patch seems to have functional side-effects beyond the description.
> > 
> > For example,
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index db3b461..0024b59 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -2223,9 +2223,6 @@ intel_dp_set_property(struct drm_connector *connector,
> > >  		int i = val;
> > >  		bool has_audio;
> > >  
> > > -		if (i == intel_dp->force_audio)
> > > -			return 0;
> > > -
> > 
> > Here we are checking against the current value of the intel_dp, which
> > may in theory be modified elsewhere as well, and avoiding the modeswitch
> > if it is unnecessary.
> 
> In case of force_audio I just checked that the current value is not
> changed elsewhere, but I must admit that I haven't checked this for
> all other properties. Would the patch be ok if I review for side
> effects?
> 
> BTW if someone changes the underlying variable of a property outside
> of the .set_property callback there are likely problems elsewhere
> because without calling drm_connector_property_set_value the values
> of the variable and the property backing store will be inconsistent.
> The calls to drm_connector_property_set_value in turn are quite easy
> reviewable, those are very few.

Sure, it just jumped out as being not in the same pattern as the rest.
I'd break out the ones that are different, like intel_dp, into their own
patch so that the above analysis can be recorded along with the change
and that we have something easy to bisect/change-our-minds about later.
;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 18/20] drm: do not call drm_connector_property_set_value from drivers

[Sascha Hauer]

If a property has changed successfully the core will call
drm_connector_property_set_value, so do not duplicate this
call in the drivers.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/gma500/cdv_intel_hdmi.c |    4 ----
 drivers/gpu/drm/gma500/cdv_intel_lvds.c |   23 ++++++-----------------
 drivers/gpu/drm/gma500/psb_intel_lvds.c |   25 +++++++------------------
 drivers/gpu/drm/gma500/psb_intel_sdvo.c |    5 -----
 drivers/gpu/drm/i915/intel_dp.c         |    5 -----
 drivers/gpu/drm/i915/intel_hdmi.c       |    5 -----
 drivers/gpu/drm/i915/intel_sdvo.c       |    5 -----
 drivers/gpu/drm/i915/intel_tv.c         |    4 ----
 8 files changed, 13 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
index de25560..87b7bd6 100644
--- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
+++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
@@ -195,10 +195,6 @@ static int cdv_hdmi_set_property(struct drm_connector *connector,
 		if (curValue == value)
 			return 0;
 
-		if (drm_connector_property_set_value(connector,
-							property, value))
-			return -1;
-
 		centre = (curValue == DRM_MODE_SCALE_NO_SCALE) ||
 			(value == DRM_MODE_SCALE_NO_SCALE);
 
diff --git a/drivers/gpu/drm/gma500/cdv_intel_lvds.c b/drivers/gpu/drm/gma500/cdv_intel_lvds.c
index 7569e8e..aeb9624 100644
--- a/drivers/gpu/drm/gma500/cdv_intel_lvds.c
+++ b/drivers/gpu/drm/gma500/cdv_intel_lvds.c
@@ -471,11 +471,6 @@ int cdv_intel_lvds_set_property(struct drm_connector *connector,
 			return -1;
 		}
 
-		if (drm_connector_property_set_value(connector,
-							property,
-							value))
-			return -1;
-
 		if (crtc->saved_mode.hdisplay != 0 &&
 		    crtc->saved_mode.vdisplay != 0) {
 			if (!drm_crtc_helper_set_mode(encoder->crtc,
@@ -486,20 +481,14 @@ int cdv_intel_lvds_set_property(struct drm_connector *connector,
 				return -1;
 		}
 	} else if (!strcmp(property->name, "backlight") && encoder) {
-		if (drm_connector_property_set_value(connector,
-							property,
-							value))
-			return -1;
-		else {
 #ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
-			struct drm_psb_private *dev_priv =
-						encoder->dev->dev_private;
-			struct backlight_device *bd =
-						dev_priv->backlight_device;
-			bd->props.brightness = value;
-			backlight_update_status(bd);
+		struct drm_psb_private *dev_priv =
+					encoder->dev->dev_private;
+		struct backlight_device *bd =
+					dev_priv->backlight_device;
+		bd->props.brightness = value;
+		backlight_update_status(bd);
 #endif
-		}
 	} else if (!strcmp(property->name, "DPMS") && encoder) {
 		struct drm_encoder_helper_funcs *helpers =
 					encoder->helper_private;
diff --git a/drivers/gpu/drm/gma500/psb_intel_lvds.c b/drivers/gpu/drm/gma500/psb_intel_lvds.c
index 7c9498ea..112d48a 100644
--- a/drivers/gpu/drm/gma500/psb_intel_lvds.c
+++ b/drivers/gpu/drm/gma500/psb_intel_lvds.c
@@ -601,11 +601,6 @@ int psb_intel_lvds_set_property(struct drm_connector *connector,
 			goto set_prop_error;
 		}
 
-		if (drm_connector_property_set_value(connector,
-							property,
-							value))
-			goto set_prop_error;
-
 		if (crtc->saved_mode.hdisplay != 0 &&
 		    crtc->saved_mode.vdisplay != 0) {
 			if (!drm_crtc_helper_set_mode(encoder->crtc,
@@ -616,21 +611,15 @@ int psb_intel_lvds_set_property(struct drm_connector *connector,
 				goto set_prop_error;
 		}
 	} else if (!strcmp(property->name, "backlight")) {
-		if (drm_connector_property_set_value(connector,
-							property,
-							value))
-			goto set_prop_error;
-		else {
 #ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
-			struct drm_psb_private *devp =
-						encoder->dev->dev_private;
-			struct backlight_device *bd = devp->backlight_device;
-			if (bd) {
-				bd->props.brightness = value;
-				backlight_update_status(bd);
-			}
-#endif
+		struct drm_psb_private *devp =
+					encoder->dev->dev_private;
+		struct backlight_device *bd = devp->backlight_device;
+		if (bd) {
+			bd->props.brightness = value;
+			backlight_update_status(bd);
 		}
+#endif
 	} else if (!strcmp(property->name, "DPMS")) {
 		struct drm_encoder_helper_funcs *hfuncs
 						= encoder->helper_private;
diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
index b9c6da9..4396be8 100644
--- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c
+++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
@@ -1703,11 +1703,6 @@ psb_intel_sdvo_set_property(struct drm_connector *connector,
 	struct drm_psb_private *dev_priv = connector->dev->dev_private;
 	uint16_t temp_value;
 	uint8_t cmd;
-	int ret;
-
-	ret = drm_connector_property_set_value(connector, property, val);
-	if (ret)
-		return ret;
 
 	if (property == dev_priv->force_audio_property) {
 		int i = val;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0024b59..32fba41 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2213,11 +2213,6 @@ intel_dp_set_property(struct drm_connector *connector,
 {
 	struct drm_i915_private *dev_priv = connector->dev->dev_private;
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	int ret;
-
-	ret = drm_connector_property_set_value(connector, property, val);
-	if (ret)
-		return ret;
 
 	if (property == dev_priv->force_audio_property) {
 		int i = val;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 64541f7..8218be5 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -395,11 +395,6 @@ intel_hdmi_set_property(struct drm_connector *connector,
 {
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
 	struct drm_i915_private *dev_priv = connector->dev->dev_private;
-	int ret;
-
-	ret = drm_connector_property_set_value(connector, property, val);
-	if (ret)
-		return ret;
 
 	if (property == dev_priv->force_audio_property) {
 		int i = val;
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 24d9e62..7f4e758 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1669,11 +1669,6 @@ intel_sdvo_set_property(struct drm_connector *connector,
 	struct drm_i915_private *dev_priv = connector->dev->dev_private;
 	uint16_t temp_value;
 	uint8_t cmd;
-	int ret;
-
-	ret = drm_connector_property_set_value(connector, property, val);
-	if (ret)
-		return ret;
 
 	if (property == dev_priv->force_audio_property) {
 		int i = val;
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 6eb11fe..5d074f5 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1381,10 +1381,6 @@ intel_tv_set_property(struct drm_connector *connector, struct drm_property *prop
 	struct drm_crtc *crtc = intel_tv->base.base.crtc;
 	int ret = 0;
 
-	ret = drm_connector_property_set_value(connector, property, val);
-	if (ret < 0)
-		goto out;
-
 	if (property == dev->mode_config.tv_left_margin_property) {
 		intel_tv->margin[TV_MARGIN_LEFT] = val;
 	} else if (property == dev->mode_config.tv_right_margin_property) {
-- 
1.7.8.3

[PATCH 19/20] drm exynos: use drm_fb_helper_set_par directly

[Sascha Hauer]

info->fix.visual already is correctly set from drm_fb_helper_fill_fix.
info->fix.line_length is also set from drm_fb_helper_fill_fix,
so drm_fb_helper_set_par directly instead of a custom
exynos_drm_fbdev_set_par.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |   28 +---------------------------
 1 files changed, 1 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index 706c906..e4bb88e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -46,39 +46,13 @@ struct exynos_drm_fbdev {
 	struct exynos_drm_gem_obj	*exynos_gem_obj;
 };
 
-static int exynos_drm_fbdev_set_par(struct fb_info *info)
-{
-	struct fb_var_screeninfo *var = &info->var;
-
-	switch (var->bits_per_pixel) {
-	case 32:
-	case 24:
-	case 18:
-	case 16:
-	case 12:
-		info->fix.visual = FB_VISUAL_TRUECOLOR;
-		break;
-	case 1:
-		info->fix.visual = FB_VISUAL_MONO01;
-		break;
-	default:
-		info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
-		break;
-	}
-
-	info->fix.line_length = (var->xres_virtual * var->bits_per_pixel) / 8;
-
-	return drm_fb_helper_set_par(info);
-}
-
-
 static struct fb_ops exynos_drm_fb_ops = {
 	.owner		= THIS_MODULE,
 	.fb_fillrect	= cfb_fillrect,
 	.fb_copyarea	= cfb_copyarea,
 	.fb_imageblit	= cfb_imageblit,
 	.fb_check_var	= drm_fb_helper_check_var,
-	.fb_set_par	= exynos_drm_fbdev_set_par,
+	.fb_set_par	= drm_fb_helper_set_par,
 	.fb_blank	= drm_fb_helper_blank,
 	.fb_pan_display	= drm_fb_helper_pan_display,
 	.fb_setcmap	= drm_fb_helper_setcmap,
-- 
1.7.8.3

RE: [PATCH 19/20] drm exynos: use drm_fb_helper_set_par directly

[Inki Dae]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> Sent: Wednesday, February 01, 2012 7:39 PM
> To: dri-devel@lists.freedesktop.org
> Cc: kernel@pengutronix.de; Sascha Hauer; Inki Dae
> Subject: [PATCH 19/20] drm exynos: use drm_fb_helper_set_par directly
> 
> info->fix.visual already is correctly set from drm_fb_helper_fill_fix.
> info->fix.line_length is also set from drm_fb_helper_fill_fix,
> so drm_fb_helper_set_par directly instead of a custom
> exynos_drm_fbdev_set_par.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Inki Dae <inki.dae@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c |   28
+------------------------
> ---
>  1 files changed, 1 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index 706c906..e4bb88e 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -46,39 +46,13 @@ struct exynos_drm_fbdev {
>  	struct exynos_drm_gem_obj	*exynos_gem_obj;
>  };
> 
> -static int exynos_drm_fbdev_set_par(struct fb_info *info)
> -{
> -	struct fb_var_screeninfo *var = &info->var;
> -
> -	switch (var->bits_per_pixel) {
> -	case 32:
> -	case 24:
> -	case 18:
> -	case 16:
> -	case 12:
> -		info->fix.visual = FB_VISUAL_TRUECOLOR;
> -		break;
> -	case 1:
> -		info->fix.visual = FB_VISUAL_MONO01;
> -		break;
> -	default:
> -		info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
> -		break;
> -	}
> -
> -	info->fix.line_length = (var->xres_virtual * var->bits_per_pixel) /
> 8;
> -
> -	return drm_fb_helper_set_par(info);
> -}
> -
> -
>  static struct fb_ops exynos_drm_fb_ops = {
>  	.owner		= THIS_MODULE,
>  	.fb_fillrect	= cfb_fillrect,
>  	.fb_copyarea	= cfb_copyarea,
>  	.fb_imageblit	= cfb_imageblit,
>  	.fb_check_var	= drm_fb_helper_check_var,
> -	.fb_set_par	= exynos_drm_fbdev_set_par,
> +	.fb_set_par	= drm_fb_helper_set_par,
>  	.fb_blank	= drm_fb_helper_blank,
>  	.fb_pan_display	= drm_fb_helper_pan_display,
>  	.fb_setcmap	= drm_fb_helper_setcmap,
> --
> 1.7.8.3

Tested-by: Inki Dae <inki.dae@samsung.com>
Thanks.

[PATCH 20/20] drm: do not set fb_info->pixmap fields

[Sascha Hauer]

The drm drivers set the fb_info->pixmap fields without setting
fb_info->pixmap.addr. If this is not set the fb core will overwrite
these all fb_info->pixmap fields anyway, so there is not much point
in setting them in the first place.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/gma500/framebuffer.c    |    6 ------
 drivers/gpu/drm/i915/intel_fb.c         |    6 ------
 drivers/gpu/drm/nouveau/nouveau_fbcon.c |    6 ------
 drivers/gpu/drm/radeon/radeon_fb.c      |    6 ------
 drivers/gpu/drm/vmwgfx/vmwgfx_fb.c      |   14 --------------
 drivers/video/nvidia/nvidia.c           |    6 ------
 6 files changed, 0 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index f359620..9ff4e5d 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -507,12 +507,6 @@ static int psbfb_create(struct psb_fbdev *fbdev,
 	info->fix.mmio_start = pci_resource_start(dev->pdev, 0);
 	info->fix.mmio_len = pci_resource_len(dev->pdev, 0);
 
-	info->pixmap.size = 64 * 1024;
-	info->pixmap.buf_align = 8;
-	info->pixmap.access_align = 32;
-	info->pixmap.flags = FB_PIXMAP_SYSTEM;
-	info->pixmap.scan_align = 1;
-
 	dev_info(dev->dev, "allocated %dx%d fb\n",
 					psbfb->base.width, psbfb->base.height);
 
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 8f81286..e4c7038 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -152,12 +152,6 @@ static int intelfb_create(struct intel_fbdev *ifbdev,
 	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
 	drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
 
-	info->pixmap.size = 64*1024;
-	info->pixmap.buf_align = 8;
-	info->pixmap.access_align = 32;
-	info->pixmap.flags = FB_PIXMAP_SYSTEM;
-	info->pixmap.scan_align = 1;
-
 	DRM_DEBUG_KMS("allocated %dx%d fb: 0x%08x, bo %p\n",
 		      fb->width, fb->height,
 		      obj->gtt_offset, obj);
diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
index 01061bb..43fd21e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
@@ -381,12 +381,6 @@ nouveau_fbcon_create(struct nouveau_fbdev *nfbdev,
 		goto out_unref;
 	}
 
-	info->pixmap.size = 64*1024;
-	info->pixmap.buf_align = 8;
-	info->pixmap.access_align = 32;
-	info->pixmap.flags = FB_PIXMAP_SYSTEM;
-	info->pixmap.scan_align = 1;
-
 	mutex_unlock(&dev->struct_mutex);
 
 	if (dev_priv->channel && !nouveau_nofbaccel) {
diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
index ed5a642..f5b600f 100644
--- a/drivers/gpu/drm/radeon/radeon_fb.c
+++ b/drivers/gpu/drm/radeon/radeon_fb.c
@@ -254,12 +254,6 @@ static int radeonfb_create(struct radeon_fbdev *rfbdev,
 	info->apertures->ranges[0].base = rdev->ddev->mode_config.fb_base;
 	info->apertures->ranges[0].size = rdev->mc.aper_size;
 
-	info->pixmap.size = 64*1024;
-	info->pixmap.buf_align = 8;
-	info->pixmap.access_align = 32;
-	info->pixmap.flags = FB_PIXMAP_SYSTEM;
-	info->pixmap.scan_align = 1;
-
 	if (info->screen_base == NULL) {
 		ret = -ENOSPC;
 		goto out_unref;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
index 34e51a1..34d1e15 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
@@ -515,20 +515,6 @@ int vmw_fb_init(struct vmw_private *vmw_priv)
 	info->var.xres = initial_width;
 	info->var.yres = initial_height;
 
-#if 0
-	info->pixmap.size = 64*1024;
-	info->pixmap.buf_align = 8;
-	info->pixmap.access_align = 32;
-	info->pixmap.flags = FB_PIXMAP_SYSTEM;
-	info->pixmap.scan_align = 1;
-#else
-	info->pixmap.size = 0;
-	info->pixmap.buf_align = 8;
-	info->pixmap.access_align = 32;
-	info->pixmap.flags = FB_PIXMAP_SYSTEM;
-	info->pixmap.scan_align = 1;
-#endif
-
 	info->apertures = alloc_apertures(1);
 	if (!info->apertures) {
 		ret = -ENOMEM;
diff --git a/drivers/video/nvidia/nvidia.c b/drivers/video/nvidia/nvidia.c
index fe13ac5..8e29b57 100644
--- a/drivers/video/nvidia/nvidia.c
+++ b/drivers/video/nvidia/nvidia.c
@@ -1167,12 +1167,6 @@ static int __devinit nvidia_set_fbinfo(struct fb_info *info)
 		((info->var.bits_per_pixel + 7) >> 3);
 	info->var.yres_virtual = info->screen_size / lpitch;
 
-	info->pixmap.scan_align = 4;
-	info->pixmap.buf_align = 4;
-	info->pixmap.access_align = 32;
-	info->pixmap.size = 8 * 1024;
-	info->pixmap.flags = FB_PIXMAP_SYSTEM;
-
 	if (!hwcur)
 	    info->fbops->fb_cursor = NULL;
 
-- 
1.7.8.3

Re: [PATCH 20/20] drm: do not set fb_info->pixmap fields

[Chris Wilson]

On Wed,  1 Feb 2012 11:38:38 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> The drm drivers set the fb_info->pixmap fields without setting
> fb_info->pixmap.addr. If this is not set the fb core will overwrite
> these all fb_info->pixmap fields anyway, so there is not much point
> in setting them in the first place.

Might be worth replacing those blocks with a comment:
  /* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm cleanup patches

[Sascha Hauer]

Hi David,

On Wed, Feb 01, 2012 at 11:38:18AM +0100, Sascha Hauer wrote:
> The following patches contain some fixes and cleanups for the drm
> core.
> 
> - fix memory holes
> - make some initialization / deinitialization more symmetric
> - add convenience functions for creating properties
> - remove DRM_CONNECTOR_MAX_PROPERTY limitation
> 
> All patches tested on a GeForce 6200 LE with the nouveau driver and
> a DELL E6220 Laptop using the intel driver.
> 
> Please review and consider applying

Is the series otherwise series ok? If yes I would integrate the comments
received so far and repost.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] drm cleanup patches

[Dave Airlie]

On Thu, Feb 2, 2012 at 2:13 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> Hi David,
>
> On Wed, Feb 01, 2012 at 11:38:18AM +0100, Sascha Hauer wrote:
>> The following patches contain some fixes and cleanups for the drm
>> core.
>>
>> - fix memory holes
>> - make some initialization / deinitialization more symmetric
>> - add convenience functions for creating properties
>> - remove DRM_CONNECTOR_MAX_PROPERTY limitation
>>
>> All patches tested on a GeForce 6200 LE with the nouveau driver and
>> a DELL E6220 Laptop using the intel driver.
>>
>> Please review and consider applying
>
> Is the series otherwise series ok? If yes I would integrate the comments
> received so far and repost.
>

Hi Sascha,

I've merged some of the easier one into -next, pushed out now.

The rest I've replied to or would like to see review comments addressed.

Thanks,
Dave.