[PATCH v2 0/9] drm/fb-helper: Deferred setup support

[PATCH v2 0/9] drm/fb-helper: Deferred setup support

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This set of patches adds support for deferring FB helper setup, which is
useful to obtain a sane configuration even when no outputs are available
during probe.

One example is HDMI, where fbdev will currently fallback to a 1024x786
resolution if no monitor is connected, and will then forever stay that
way. With these patches, the FB helpers will take note that it doesn't
make sense to setup fbdev yet and will defer until a monitor is
connected, at which point the preferred mode will be selected.

Thierry

Changes in v2:
- now with locking

Thierry Reding (9):
  drm/fb-helper: Cleanup checkpatch warnings
  drm/fb-helper: Reshuffle code for subsequent patches
  drm/fb-helper: Improve code readability
  drm/fb-helper: Push down modeset lock into FB helpers
  drm/fb-helper: Add top-level lock
  drm/fb-helper: Support deferred setup
  drm/atmel-hlcdc: Remove custom FB helper deferred setup
  drm/exynos: Remove custom FB helper deferred setup
  drm/hisilicon: Remove custom FB helper deferred setup

 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c    |  26 +--
 drivers/gpu/drm/drm_fb_helper.c                 | 217 +++++++++++++++++-------
 drivers/gpu/drm/exynos/exynos_drm_drv.c         |   8 +-
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c       |   2 -
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c |  22 +--
 drivers/gpu/drm/i915/intel_dp_mst.c             |   4 -
 drivers/gpu/drm/radeon/radeon_dp_mst.c          |   7 -
 include/drm/drm_fb_helper.h                     |  26 +++
 8 files changed, 217 insertions(+), 95 deletions(-)

-- 
2.8.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/9] drm/fb-helper: Cleanup checkpatch warnings

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Fix up a couple of checkpatch warnings, such as whitespace or coding
style issues.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 7c2eb75db60f..7945620e7439 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -190,9 +190,9 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 	fb_helper_connector = fb_helper->connector_info[i];
 	drm_connector_unreference(fb_helper_connector->connector);
 
-	for (j = i + 1; j < fb_helper->connector_count; j++) {
+	for (j = i + 1; j < fb_helper->connector_count; j++)
 		fb_helper->connector_info[j - 1] = fb_helper->connector_info[j];
-	}
+
 	fb_helper->connector_count--;
 	kfree(fb_helper_connector);
 
@@ -290,6 +290,7 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
 
 	for (i = 0; i < helper->crtc_count; i++) {
 		struct drm_mode_set *mode_set = &helper->crtc_info[i].mode_set;
+
 		crtc = mode_set->crtc;
 		funcs = crtc->helper_private;
 		fb = drm_mode_config_fb(crtc);
@@ -317,7 +318,7 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
 	struct drm_plane *plane;
 	struct drm_atomic_state *state;
 	int i, ret;
-	unsigned plane_mask;
+	unsigned int plane_mask;
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state)
@@ -349,7 +350,7 @@ retry:
 			goto fail;
 	}
 
-	for(i = 0; i < fb_helper->crtc_count; i++) {
+	for (i = 0; i < fb_helper->crtc_count; i++) {
 		struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set;
 
 		ret = __drm_atomic_helper_set_config(mode_set, state);
@@ -511,6 +512,7 @@ static bool drm_fb_helper_force_kernel_mode(void)
 static void drm_fb_helper_restore_work_fn(struct work_struct *ignored)
 {
 	bool ret;
+
 	ret = drm_fb_helper_force_kernel_mode();
 	if (ret == true)
 		DRM_ERROR("Failed to restore crtc configuration\n");
@@ -812,9 +814,8 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 
 	if (!list_empty(&fb_helper->kernel_fb_list)) {
 		list_del(&fb_helper->kernel_fb_list);
-		if (list_empty(&kernel_fb_helper_list)) {
+		if (list_empty(&kernel_fb_helper_list))
 			unregister_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
-		}
 	}
 
 	drm_fb_helper_crtc_free(fb_helper);
@@ -1059,6 +1060,7 @@ static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green,
 			(blue << info->var.blue.offset);
 		if (info->var.transp.length > 0) {
 			u32 mask = (1 << info->var.transp.length) - 1;
+
 			mask <<= info->var.transp.offset;
 			value |= mask;
 		}
@@ -1087,6 +1089,7 @@ static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green,
 		if (fb->depth == 16) {
 			u16 r, g, b;
 			int i;
+
 			if (regno < 32) {
 				for (i = 0; i < 8; i++)
 					fb_helper->funcs->gamma_set(crtc, red,
@@ -1298,7 +1301,7 @@ static int pan_display_atomic(struct fb_var_screeninfo *var,
 	struct drm_atomic_state *state;
 	struct drm_plane *plane;
 	int i, ret;
-	unsigned plane_mask;
+	unsigned int plane_mask;
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state)
@@ -1307,7 +1310,7 @@ static int pan_display_atomic(struct fb_var_screeninfo *var,
 	state->acquire_ctx = dev->mode_config.acquire_ctx;
 retry:
 	plane_mask = 0;
-	for(i = 0; i < fb_helper->crtc_count; i++) {
+	for (i = 0; i < fb_helper->crtc_count; i++) {
 		struct drm_mode_set *mode_set;
 
 		mode_set = &fb_helper->crtc_info[i].mode_set;
@@ -1416,8 +1419,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	memset(&sizes, 0, sizeof(struct drm_fb_helper_surface_size));
 	sizes.surface_depth = 24;
 	sizes.surface_bpp = 32;
-	sizes.fb_width = (unsigned)-1;
-	sizes.fb_height = (unsigned)-1;
+	sizes.fb_width = (u32)-1;
+	sizes.fb_height = (u32)-1;
 
 	/* if driver picks 8 or 16 by default use that
 	   for both depth/bpp */
@@ -1485,6 +1488,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 
 		for (j = 0; j < mode_set->num_connectors; j++) {
 			struct drm_connector *connector = mode_set->connectors[j];
+
 			if (connector->has_tile) {
 				lasth = (connector->tile_h_loc == (connector->num_h_tile - 1));
 				lastv = (connector->tile_v_loc == (connector->num_v_tile - 1));
@@ -1533,9 +1537,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n",
 			info->node, info->fix.id);
 
-	if (list_empty(&kernel_fb_helper_list)) {
+	if (list_empty(&kernel_fb_helper_list))
 		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
-	}
 
 	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
 
@@ -1570,7 +1573,6 @@ void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
 	info->fix.accel = FB_ACCEL_NONE;
 
 	info->fix.line_length = pitch;
-	return;
 }
 EXPORT_SYMBOL(drm_fb_helper_fill_fix);
 
@@ -1592,6 +1594,7 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
 			    uint32_t fb_width, uint32_t fb_height)
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
+
 	info->pseudo_palette = fb_helper->pseudo_palette;
 	info->var.xres_virtual = fb->width;
 	info->var.yres_virtual = fb->height;
@@ -1902,6 +1905,7 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
 	int i;
 	uint64_t conn_configured = 0, mask;
 	int tile_pass = 0;
+
 	mask = (1 << fb_helper->connector_count) - 1;
 retry:
 	for (i = 0; i < fb_helper->connector_count; i++) {
@@ -1925,7 +1929,7 @@ retry:
 				continue;
 
 		} else {
-			if (fb_helper_conn->connector->tile_h_loc != tile_pass -1 &&
+			if (fb_helper_conn->connector->tile_h_loc != tile_pass - 1 &&
 			    fb_helper_conn->connector->tile_v_loc != tile_pass - 1)
 			/* if this tile_pass doesn't cover any of the tiles - keep going */
 				continue;
@@ -2105,6 +2109,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 		struct drm_display_mode *mode = modes[i];
 		struct drm_fb_helper_crtc *fb_crtc = crtcs[i];
 		struct drm_fb_offset *offset = &offsets[i];
+
 		modeset = &fb_crtc->mode_set;
 
 		if (mode && fb_crtc) {
-- 
2.8.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/9] drm/fb-helper: Reshuffle code for subsequent patches

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

An unlocked version of the drm_fb_helper_add_one_connector() function
will be added in a subsequent patch. Reshuffle the code separately to
make the diff more readable later on.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 60 ++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 7945620e7439..301644d12013 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -95,6 +95,36 @@ static LIST_HEAD(kernel_fb_helper_list);
  * mmap page writes.
  */
 
+int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_connector *connector)
+{
+	struct drm_fb_helper_connector **temp;
+	struct drm_fb_helper_connector *fb_helper_connector;
+
+	if (!drm_fbdev_emulation)
+		return 0;
+
+	WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
+	if (fb_helper->connector_count + 1 > fb_helper->connector_info_alloc_count) {
+		temp = krealloc(fb_helper->connector_info, sizeof(struct drm_fb_helper_connector *) * (fb_helper->connector_count + 1), GFP_KERNEL);
+		if (!temp)
+			return -ENOMEM;
+
+		fb_helper->connector_info_alloc_count = fb_helper->connector_count + 1;
+		fb_helper->connector_info = temp;
+	}
+
+
+	fb_helper_connector = kzalloc(sizeof(struct drm_fb_helper_connector), GFP_KERNEL);
+	if (!fb_helper_connector)
+		return -ENOMEM;
+
+	drm_connector_reference(connector);
+	fb_helper_connector->connector = connector;
+	fb_helper->connector_info[fb_helper->connector_count++] = fb_helper_connector;
+	return 0;
+}
+EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
+
 /**
  * drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev
  * 					       emulation helper
@@ -139,36 +169,6 @@ fail:
 }
 EXPORT_SYMBOL(drm_fb_helper_single_add_all_connectors);
 
-int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_connector *connector)
-{
-	struct drm_fb_helper_connector **temp;
-	struct drm_fb_helper_connector *fb_helper_connector;
-
-	if (!drm_fbdev_emulation)
-		return 0;
-
-	WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
-	if (fb_helper->connector_count + 1 > fb_helper->connector_info_alloc_count) {
-		temp = krealloc(fb_helper->connector_info, sizeof(struct drm_fb_helper_connector *) * (fb_helper->connector_count + 1), GFP_KERNEL);
-		if (!temp)
-			return -ENOMEM;
-
-		fb_helper->connector_info_alloc_count = fb_helper->connector_count + 1;
-		fb_helper->connector_info = temp;
-	}
-
-
-	fb_helper_connector = kzalloc(sizeof(struct drm_fb_helper_connector), GFP_KERNEL);
-	if (!fb_helper_connector)
-		return -ENOMEM;
-
-	drm_connector_reference(connector);
-	fb_helper_connector->connector = connector;
-	fb_helper->connector_info[fb_helper->connector_count++] = fb_helper_connector;
-	return 0;
-}
-EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
-
 int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 				       struct drm_connector *connector)
 {
-- 
2.8.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/9] drm/fb-helper: Improve code readability

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Add a couple of temporary variables and use shorter names for existing
variables in drm_fb_helper_add_one_connector() for better readability.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 301644d12013..9afd4208596f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -95,32 +95,38 @@ static LIST_HEAD(kernel_fb_helper_list);
  * mmap page writes.
  */
 
-int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_connector *connector)
+int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
+				    struct drm_connector *connector)
 {
 	struct drm_fb_helper_connector **temp;
-	struct drm_fb_helper_connector *fb_helper_connector;
+	struct drm_fb_helper_connector *conn;
+	unsigned int count;
 
 	if (!drm_fbdev_emulation)
 		return 0;
 
 	WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
-	if (fb_helper->connector_count + 1 > fb_helper->connector_info_alloc_count) {
-		temp = krealloc(fb_helper->connector_info, sizeof(struct drm_fb_helper_connector *) * (fb_helper->connector_count + 1), GFP_KERNEL);
+
+	count = fb_helper->connector_count + 1;
+
+	if (count > fb_helper->connector_info_alloc_count) {
+		size_t size = count * sizeof(conn);
+
+		temp = krealloc(fb_helper->connector_info, size, GFP_KERNEL);
 		if (!temp)
 			return -ENOMEM;
 
-		fb_helper->connector_info_alloc_count = fb_helper->connector_count + 1;
+		fb_helper->connector_info_alloc_count = count;
 		fb_helper->connector_info = temp;
 	}
 
-
-	fb_helper_connector = kzalloc(sizeof(struct drm_fb_helper_connector), GFP_KERNEL);
-	if (!fb_helper_connector)
+	conn = kzalloc(sizeof(*conn), GFP_KERNEL);
+	if (!conn)
 		return -ENOMEM;
 
 	drm_connector_reference(connector);
-	fb_helper_connector->connector = connector;
-	fb_helper->connector_info[fb_helper->connector_count++] = fb_helper_connector;
+	conn->connector = connector;
+	fb_helper->connector_info[fb_helper->connector_count++] = conn;
 	return 0;
 }
 EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
-- 
2.8.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/9] drm/fb-helper: Push down modeset lock into FB helpers

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Move the modeset locking from drivers into FB helpers.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_fb_helper.c        | 59 +++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_dp_mst.c    |  4 ---
 drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ----
 3 files changed, 44 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 9afd4208596f..252c7557bdb5 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -95,8 +95,8 @@ static LIST_HEAD(kernel_fb_helper_list);
  * mmap page writes.
  */
 
-int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
-				    struct drm_connector *connector)
+static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
+					     struct drm_connector *connector)
 {
 	struct drm_fb_helper_connector **temp;
 	struct drm_fb_helper_connector *conn;
@@ -129,6 +129,20 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
 	fb_helper->connector_info[fb_helper->connector_count++] = conn;
 	return 0;
 }
+
+int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
+				    struct drm_connector *connector)
+{
+	int err;
+
+	mutex_lock(&fb_helper->dev->mode_config.mutex);
+
+	err = __drm_fb_helper_add_one_connector(fb_helper, connector);
+
+	mutex_unlock(&fb_helper->dev->mode_config.mutex);
+
+	return err;
+}
 EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
 
 /**
@@ -155,28 +169,28 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 		return 0;
 
 	mutex_lock(&dev->mode_config.mutex);
+
 	drm_for_each_connector(connector, dev) {
-		ret = drm_fb_helper_add_one_connector(fb_helper, connector);
+		ret = __drm_fb_helper_add_one_connector(fb_helper, connector);
+		if (ret) {
+			for (i = 0; i < fb_helper->connector_count; i++) {
+				kfree(fb_helper->connector_info[i]);
+				fb_helper->connector_info[i] = NULL;
+			}
 
-		if (ret)
-			goto fail;
-	}
-	mutex_unlock(&dev->mode_config.mutex);
-	return 0;
-fail:
-	for (i = 0; i < fb_helper->connector_count; i++) {
-		kfree(fb_helper->connector_info[i]);
-		fb_helper->connector_info[i] = NULL;
+			fb_helper->connector_count = 0;
+			break;
+		}
 	}
-	fb_helper->connector_count = 0;
+
 	mutex_unlock(&dev->mode_config.mutex);
 
 	return ret;
 }
 EXPORT_SYMBOL(drm_fb_helper_single_add_all_connectors);
 
-int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
-				       struct drm_connector *connector)
+static int __drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
+						struct drm_connector *connector)
 {
 	struct drm_fb_helper_connector *fb_helper_connector;
 	int i, j;
@@ -193,6 +207,7 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 
 	if (i == fb_helper->connector_count)
 		return -EINVAL;
+
 	fb_helper_connector = fb_helper->connector_info[i];
 	drm_connector_unreference(fb_helper_connector->connector);
 
@@ -204,6 +219,20 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 
 	return 0;
 }
+
+int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
+				       struct drm_connector *connector)
+{
+	int err;
+
+	mutex_lock(&fb_helper->dev->mode_config.mutex);
+
+	err = __drm_fb_helper_remove_one_connector(fb_helper, connector);
+
+	mutex_unlock(&fb_helper->dev->mode_config.mutex);
+
+	return err;
+}
 EXPORT_SYMBOL(drm_fb_helper_remove_one_connector);
 
 static void drm_fb_helper_save_lut_atomic(struct drm_crtc *crtc, struct drm_fb_helper *helper)
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 7a34090cef34..47c3980cb3b9 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -477,9 +477,7 @@ static void intel_dp_register_mst_connector(struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct drm_device *dev = connector->dev;
-	drm_modeset_lock_all(dev);
 	intel_connector_add_to_fbdev(intel_connector);
-	drm_modeset_unlock_all(dev);
 	drm_connector_register(&intel_connector->base);
 }
 
@@ -492,10 +490,8 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	intel_connector->unregister(intel_connector);
 
 	/* need to nuke the connector */
-	drm_modeset_lock_all(dev);
 	intel_connector_remove_from_fbdev(intel_connector);
 	intel_connector->mst_port = NULL;
-	drm_modeset_unlock_all(dev);
 
 	drm_connector_unreference(&intel_connector->base);
 	DRM_DEBUG_KMS("\n");
diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c
index de504ea29c06..a4ccaa5af104 100644
--- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
+++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
@@ -299,9 +299,7 @@ static void radeon_dp_register_mst_connector(struct drm_connector *connector)
 	struct drm_device *dev = connector->dev;
 	struct radeon_device *rdev = dev->dev_private;
 
-	drm_modeset_lock_all(dev);
 	radeon_fb_add_connector(rdev, connector);
-	drm_modeset_unlock_all(dev);
 
 	drm_connector_register(connector);
 }
@@ -314,13 +312,8 @@ static void radeon_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	struct radeon_device *rdev = dev->dev_private;
 
 	drm_connector_unregister(connector);
-	/* need to nuke the connector */
-	drm_modeset_lock_all(dev);
-	/* dpms off */
 	radeon_fb_remove_connector(rdev, connector);
-
 	drm_connector_cleanup(connector);
-	drm_modeset_unlock_all(dev);
 
 	kfree(connector);
 	DRM_DEBUG_KMS("\n");
-- 
2.8.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/9] drm/fb-helper: Push down modeset lock into FB helpers

[Inki Dae]

Hi Thierry,

2016년 06월 08일 00:26에 Thierry Reding 이(가) 쓴 글:
> From: Thierry Reding <treding@nvidia.com>
> 
> Move the modeset locking from drivers into FB helpers.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c        | 59 +++++++++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_dp_mst.c    |  4 ---
>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ----
>  3 files changed, 44 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 9afd4208596f..252c7557bdb5 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -95,8 +95,8 @@ static LIST_HEAD(kernel_fb_helper_list);
>   * mmap page writes.
>   */
>  
> -int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
> -				    struct drm_connector *connector)
> +static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
> +					     struct drm_connector *connector)
>  {
>  	struct drm_fb_helper_connector **temp;
>  	struct drm_fb_helper_connector *conn;
> @@ -129,6 +129,20 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
>  	fb_helper->connector_info[fb_helper->connector_count++] = conn;
>  	return 0;
>  }
> +
> +int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
> +				    struct drm_connector *connector)
> +{
> +	int err;
> +
> +	mutex_lock(&fb_helper->dev->mode_config.mutex);
> +
> +	err = __drm_fb_helper_add_one_connector(fb_helper, connector);

I think you don't need to make __drm_fb_helper_add_on_connector function but just you can implement the codes here instead. And the use of prefix '__' thing would not good.
And my concern about this is that other drivers may need to call this function after taking a same mutex lock. Can you assure anyone not to call this function with mutex lock?

If there is such case then,

some_function()
{
	mutex_lock();
	...
	mutex_unlock();
	drm_fb_helper_add_one_connector();
	...
}

So in this case, other users should consider to make sure to unlock before calling this function. That would be now really what you want.

Thanks,
Inki Dae

> +
> +	mutex_unlock(&fb_helper->dev->mode_config.mutex);
> +
> +	return err;
> +}
>  EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
>  
>  /**
> @@ -155,28 +169,28 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
>  		return 0;
>  
>  	mutex_lock(&dev->mode_config.mutex);
> +
>  	drm_for_each_connector(connector, dev) {
> -		ret = drm_fb_helper_add_one_connector(fb_helper, connector);
> +		ret = __drm_fb_helper_add_one_connector(fb_helper, connector);
> +		if (ret) {
> +			for (i = 0; i < fb_helper->connector_count; i++) {
> +				kfree(fb_helper->connector_info[i]);
> +				fb_helper->connector_info[i] = NULL;
> +			}

I think all resources should be released by someone who allocated the resources in case of failure. I mean that if some routine of __drm_fb_helper_add_on_connector function failed than the function make sure to release the resources allocated. I know this is really not what you did but original code did. So how about cleanning up this thing? 

Thanks,
Inki Dae

>  
> -		if (ret)
> -			goto fail;
> -	}
> -	mutex_unlock(&dev->mode_config.mutex);
> -	return 0;
> -fail:
> -	for (i = 0; i < fb_helper->connector_count; i++) {
> -		kfree(fb_helper->connector_info[i]);
> -		fb_helper->connector_info[i] = NULL;
> +			fb_helper->connector_count = 0;
> +			break;
> +		}
>  	}
> -	fb_helper->connector_count = 0;
> +
>  	mutex_unlock(&dev->mode_config.mutex);
>  
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_single_add_all_connectors);
>  
> -int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
> -				       struct drm_connector *connector)
> +static int __drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
> +						struct drm_connector *connector)
>  {
>  	struct drm_fb_helper_connector *fb_helper_connector;
>  	int i, j;
> @@ -193,6 +207,7 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  
>  	if (i == fb_helper->connector_count)
>  		return -EINVAL;
> +
>  	fb_helper_connector = fb_helper->connector_info[i];
>  	drm_connector_unreference(fb_helper_connector->connector);
>  
> @@ -204,6 +219,20 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  
>  	return 0;
>  }
> +
> +int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
> +				       struct drm_connector *connector)
> +{
> +	int err;
> +
> +	mutex_lock(&fb_helper->dev->mode_config.mutex);
> +
> +	err = __drm_fb_helper_remove_one_connector(fb_helper, connector);
> +
> +	mutex_unlock(&fb_helper->dev->mode_config.mutex);
> +
> +	return err;
> +}
>  EXPORT_SYMBOL(drm_fb_helper_remove_one_connector);
>  
>  static void drm_fb_helper_save_lut_atomic(struct drm_crtc *crtc, struct drm_fb_helper *helper)
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 7a34090cef34..47c3980cb3b9 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -477,9 +477,7 @@ static void intel_dp_register_mst_connector(struct drm_connector *connector)
>  {
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
>  	struct drm_device *dev = connector->dev;
> -	drm_modeset_lock_all(dev);
>  	intel_connector_add_to_fbdev(intel_connector);
> -	drm_modeset_unlock_all(dev);
>  	drm_connector_register(&intel_connector->base);
>  }
>  
> @@ -492,10 +490,8 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>  	intel_connector->unregister(intel_connector);
>  
>  	/* need to nuke the connector */
> -	drm_modeset_lock_all(dev);
>  	intel_connector_remove_from_fbdev(intel_connector);
>  	intel_connector->mst_port = NULL;
> -	drm_modeset_unlock_all(dev);
>  
>  	drm_connector_unreference(&intel_connector->base);
>  	DRM_DEBUG_KMS("\n");
> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> index de504ea29c06..a4ccaa5af104 100644
> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> @@ -299,9 +299,7 @@ static void radeon_dp_register_mst_connector(struct drm_connector *connector)
>  	struct drm_device *dev = connector->dev;
>  	struct radeon_device *rdev = dev->dev_private;
>  
> -	drm_modeset_lock_all(dev);
>  	radeon_fb_add_connector(rdev, connector);
> -	drm_modeset_unlock_all(dev);
>  
>  	drm_connector_register(connector);
>  }
> @@ -314,13 +312,8 @@ static void radeon_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>  	struct radeon_device *rdev = dev->dev_private;
>  
>  	drm_connector_unregister(connector);
> -	/* need to nuke the connector */
> -	drm_modeset_lock_all(dev);
> -	/* dpms off */
>  	radeon_fb_remove_connector(rdev, connector);
> -
>  	drm_connector_cleanup(connector);
> -	drm_modeset_unlock_all(dev);
>  
>  	kfree(connector);
>  	DRM_DEBUG_KMS("\n");
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/9] drm/fb-helper: Add top-level lock

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Introduce a new top-level lock for the FB helper code. This will allow
better locking granularity and avoid the need to abuse modeset locking
for this purpose instead.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 27 ++++++++++++++++++++++++++-
 include/drm/drm_fb_helper.h     |  5 +++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 252c7557bdb5..f7722bcb0064 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -105,6 +105,7 @@ static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
 	if (!drm_fbdev_emulation)
 		return 0;
 
+	WARN_ON(!mutex_is_locked(&fb_helper->lock));
 	WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
 
 	count = fb_helper->connector_count + 1;
@@ -127,6 +128,7 @@ static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
 	drm_connector_reference(connector);
 	conn->connector = connector;
 	fb_helper->connector_info[fb_helper->connector_count++] = conn;
+
 	return 0;
 }
 
@@ -135,11 +137,13 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
 {
 	int err;
 
+	mutex_lock(&fb_helper->lock);
 	mutex_lock(&fb_helper->dev->mode_config.mutex);
 
 	err = __drm_fb_helper_add_one_connector(fb_helper, connector);
 
 	mutex_unlock(&fb_helper->dev->mode_config.mutex);
+	mutex_unlock(&fb_helper->lock);
 
 	return err;
 }
@@ -168,6 +172,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 	if (!drm_fbdev_emulation)
 		return 0;
 
+	mutex_lock(&fb_helper->lock);
 	mutex_lock(&dev->mode_config.mutex);
 
 	drm_for_each_connector(connector, dev) {
@@ -184,6 +189,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 	}
 
 	mutex_unlock(&dev->mode_config.mutex);
+	mutex_unlock(&fb_helper->lock);
 
 	return ret;
 }
@@ -198,6 +204,7 @@ static int __drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 	if (!drm_fbdev_emulation)
 		return 0;
 
+	WARN_ON(!mutex_is_locked(&fb_helper->lock));
 	WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
 
 	for (i = 0; i < fb_helper->connector_count; i++) {
@@ -225,11 +232,13 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 {
 	int err;
 
+	mutex_lock(&fb_helper->lock);
 	mutex_lock(&fb_helper->dev->mode_config.mutex);
 
 	err = __drm_fb_helper_remove_one_connector(fb_helper, connector);
 
 	mutex_unlock(&fb_helper->dev->mode_config.mutex);
+	mutex_unlock(&fb_helper->lock);
 
 	return err;
 }
@@ -478,16 +487,21 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 	if (!drm_fbdev_emulation)
 		return -ENODEV;
 
+	mutex_lock(&fb_helper->lock);
 	drm_modeset_lock_all(dev);
+
 	ret = restore_fbdev_mode(fb_helper);
 
 	do_delayed = fb_helper->delayed_hotplug;
 	if (do_delayed)
 		fb_helper->delayed_hotplug = false;
+
 	drm_modeset_unlock_all(dev);
+	mutex_unlock(&fb_helper->lock);
 
 	if (do_delayed)
 		drm_fb_helper_hotplug_event(fb_helper);
+
 	return ret;
 }
 EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked);
@@ -688,6 +702,7 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
 	spin_lock_init(&helper->dirty_lock);
 	INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work);
 	helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0;
+	mutex_init(&helper->lock);
 	helper->funcs = funcs;
 	helper->dev = dev;
 }
@@ -853,6 +868,7 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 			unregister_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 	}
 
+	mutex_destroy(&fb_helper->lock);
 	drm_fb_helper_crtc_free(fb_helper);
 
 }
@@ -2273,16 +2289,20 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
 	u32 max_width, max_height;
+	int err = 0;
 
 	if (!drm_fbdev_emulation)
 		return 0;
 
+	mutex_lock(&fb_helper->lock);
 	mutex_lock(&fb_helper->dev->mode_config.mutex);
+
 	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
 		fb_helper->delayed_hotplug = true;
 		mutex_unlock(&fb_helper->dev->mode_config.mutex);
-		return 0;
+		goto unlock;
 	}
+
 	DRM_DEBUG_KMS("\n");
 
 	max_width = fb_helper->fb->width;
@@ -2290,6 +2310,7 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 
 	drm_fb_helper_probe_connector_modes(fb_helper, max_width, max_height);
 	mutex_unlock(&fb_helper->dev->mode_config.mutex);
+	mutex_unlock(&fb_helper->lock);
 
 	drm_modeset_lock_all(dev);
 	drm_setup_crtcs(fb_helper);
@@ -2297,6 +2318,10 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 	drm_fb_helper_set_par(fb_helper->fbdev);
 
 	return 0;
+
+unlock:
+	mutex_unlock(&fb_helper->lock);
+	return err;
 }
 EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
 
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 5b4aa35026a3..7739be08ebca 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -198,6 +198,11 @@ struct drm_fb_helper {
 	struct work_struct dirty_work;
 
 	/**
+	 * Top-level FB helper lock.
+	 */
+	struct mutex lock;
+
+	/**
 	 * @kernel_fb_list:
 	 *
 	 * Entry on the global kernel_fb_helper_list, used for kgdb entry/exit.
-- 
2.8.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 5/9] drm/fb-helper: Add top-level lock

[Daniel Vetter]

On Tue, Jun 07, 2016 at 05:26:21PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Introduce a new top-level lock for the FB helper code. This will allow
> better locking granularity and avoid the need to abuse modeset locking
> for this purpose instead.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

I think this is now too much locking, and ime too much locking only leads
to massive confusion. It'd be great if we can clean this up (maybe in a
follow-up patch, maybe in this one here) and restrict the core locks to
just where we need them: So all the places that look at probe state need
dev->mode_config.mutex, all the places that change modeset state need the
modeset locks. But e.g. add/remove_one_connector only need the new
fb_helper->lock I think. In the future we might grabbing the other locks
even down into the legacy/atomic fbdev implementations.

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 27 ++++++++++++++++++++++++++-
>  include/drm/drm_fb_helper.h     |  5 +++++
>  2 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 252c7557bdb5..f7722bcb0064 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -105,6 +105,7 @@ static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
>  	if (!drm_fbdev_emulation)
>  		return 0;
>  
> +	WARN_ON(!mutex_is_locked(&fb_helper->lock));
>  	WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
>  
>  	count = fb_helper->connector_count + 1;
> @@ -127,6 +128,7 @@ static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
>  	drm_connector_reference(connector);
>  	conn->connector = connector;
>  	fb_helper->connector_info[fb_helper->connector_count++] = conn;
> +
>  	return 0;
>  }
>  
> @@ -135,11 +137,13 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
>  {
>  	int err;
>  
> +	mutex_lock(&fb_helper->lock);
>  	mutex_lock(&fb_helper->dev->mode_config.mutex);
>  
>  	err = __drm_fb_helper_add_one_connector(fb_helper, connector);
>  
>  	mutex_unlock(&fb_helper->dev->mode_config.mutex);
> +	mutex_unlock(&fb_helper->lock);
>  
>  	return err;
>  }
> @@ -168,6 +172,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
>  	if (!drm_fbdev_emulation)
>  		return 0;
>  
> +	mutex_lock(&fb_helper->lock);
>  	mutex_lock(&dev->mode_config.mutex);
>  
>  	drm_for_each_connector(connector, dev) {
> @@ -184,6 +189,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
>  	}
>  
>  	mutex_unlock(&dev->mode_config.mutex);
> +	mutex_unlock(&fb_helper->lock);
>  
>  	return ret;
>  }
> @@ -198,6 +204,7 @@ static int __drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  	if (!drm_fbdev_emulation)
>  		return 0;
>  
> +	WARN_ON(!mutex_is_locked(&fb_helper->lock));
>  	WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
>  
>  	for (i = 0; i < fb_helper->connector_count; i++) {
> @@ -225,11 +232,13 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
>  {
>  	int err;
>  
> +	mutex_lock(&fb_helper->lock);
>  	mutex_lock(&fb_helper->dev->mode_config.mutex);
>  
>  	err = __drm_fb_helper_remove_one_connector(fb_helper, connector);
>  
>  	mutex_unlock(&fb_helper->dev->mode_config.mutex);
> +	mutex_unlock(&fb_helper->lock);
>  
>  	return err;
>  }
> @@ -478,16 +487,21 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>  	if (!drm_fbdev_emulation)
>  		return -ENODEV;
>  
> +	mutex_lock(&fb_helper->lock);
>  	drm_modeset_lock_all(dev);
> +
>  	ret = restore_fbdev_mode(fb_helper);
>  
>  	do_delayed = fb_helper->delayed_hotplug;
>  	if (do_delayed)
>  		fb_helper->delayed_hotplug = false;
> +
>  	drm_modeset_unlock_all(dev);
> +	mutex_unlock(&fb_helper->lock);
>  
>  	if (do_delayed)
>  		drm_fb_helper_hotplug_event(fb_helper);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked);
> @@ -688,6 +702,7 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
>  	spin_lock_init(&helper->dirty_lock);
>  	INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work);
>  	helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0;
> +	mutex_init(&helper->lock);
>  	helper->funcs = funcs;
>  	helper->dev = dev;
>  }
> @@ -853,6 +868,7 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
>  			unregister_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
>  	}
>  
> +	mutex_destroy(&fb_helper->lock);
>  	drm_fb_helper_crtc_free(fb_helper);
>  
>  }
> @@ -2273,16 +2289,20 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>  {
>  	struct drm_device *dev = fb_helper->dev;
>  	u32 max_width, max_height;
> +	int err = 0;
>  
>  	if (!drm_fbdev_emulation)
>  		return 0;
>  
> +	mutex_lock(&fb_helper->lock);
>  	mutex_lock(&fb_helper->dev->mode_config.mutex);
> +
>  	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
>  		fb_helper->delayed_hotplug = true;
>  		mutex_unlock(&fb_helper->dev->mode_config.mutex);
> -		return 0;
> +		goto unlock;
>  	}
> +
>  	DRM_DEBUG_KMS("\n");
>  
>  	max_width = fb_helper->fb->width;
> @@ -2290,6 +2310,7 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>  
>  	drm_fb_helper_probe_connector_modes(fb_helper, max_width, max_height);
>  	mutex_unlock(&fb_helper->dev->mode_config.mutex);
> +	mutex_unlock(&fb_helper->lock);
>  
>  	drm_modeset_lock_all(dev);
>  	drm_setup_crtcs(fb_helper);
> @@ -2297,6 +2318,10 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>  	drm_fb_helper_set_par(fb_helper->fbdev);
>  
>  	return 0;
> +
> +unlock:
> +	mutex_unlock(&fb_helper->lock);
> +	return err;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>  
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 5b4aa35026a3..7739be08ebca 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -198,6 +198,11 @@ struct drm_fb_helper {
>  	struct work_struct dirty_work;
>  
>  	/**
> +	 * Top-level FB helper lock.
> +	 */

need to add @lock: in the above comment, otherwise kerneldoc will still
complain about the lack of comment for this new member.
-Daniel

> +	struct mutex lock;
> +
> +	/**
>  	 * @kernel_fb_list:
>  	 *
>  	 * Entry on the global kernel_fb_helper_list, used for kgdb entry/exit.
> -- 
> 2.8.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 6/9] drm/fb-helper: Support deferred setup

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

FB helper code falls back to a 1024x768 mode if no outputs are connected
or don't report back any modes upon initialization. This can be annoying
because outputs that are added to FB helper later on can't be used with
FB helper if they don't support a matching mode.

The fallback is in place because VGA connectors can happen to report an
unknown connection status even when they are in fact connected.

Some drivers have custom solutions in place to defer FB helper setup
until at least one output is connected. But the logic behind these
solutions is always the same and there is nothing driver-specific about
it, so a better alterative is to fix the FB helper core and add support
for all drivers automatically.

This patch adds support for deferred FB helper setup. It checks all the
connectors for their connection status, and if all of them report to be
disconnected marks the FB helper as needing deferred setup. Whet setup
is deferred, the FB helper core will automatically retry setup after a
hotplug event, and it will keep trying until it succeeds.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 36 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_fb_helper.h     | 21 +++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index f7722bcb0064..0911b10c711c 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -487,6 +487,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 	if (!drm_fbdev_emulation)
 		return -ENODEV;
 
+	if (fb_helper->deferred_setup)
+		return 0;
+
 	mutex_lock(&fb_helper->lock);
 	drm_modeset_lock_all(dev);
 
@@ -1452,6 +1455,23 @@ unlock:
 }
 EXPORT_SYMBOL(drm_fb_helper_pan_display);
 
+static bool drm_fb_helper_maybe_connected(struct drm_fb_helper *helper)
+{
+	bool connected = false;
+	unsigned int i;
+
+	for (i = 0; i < helper->connector_count; i++) {
+		struct drm_fb_helper_connector *fb = helper->connector_info[i];
+
+		if (fb->connector->status != connector_status_disconnected) {
+			connected = true;
+			break;
+		}
+	}
+
+	return connected;
+}
+
 /*
  * Allocates the backing storage and sets up the fbdev info structure through
  * the ->fb_probe callback and then registers the fbdev and sets up the panic
@@ -1554,6 +1574,17 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 			sizes.fb_height = min_t(u32, desired_mode->vdisplay + y, sizes.fb_height);
 	}
 
+	/*
+	 * If everything's disconnected, there's no use in attempting to set
+	 * up fbdev.
+	 */
+	if (!drm_fb_helper_maybe_connected(fb_helper)) {
+		DRM_INFO("No outputs connected, deferring setup\n");
+		fb_helper->preferred_bpp = preferred_bpp;
+		fb_helper->deferred_setup = true;
+		return 0;
+	}
+
 	if (crtc_count == 0 || sizes.fb_width == -1 || sizes.fb_height == -1) {
 		/* hmm everyone went away - assume VGA cable just fell out
 		   and will come back later. */
@@ -1593,6 +1624,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 
 	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
 
+	fb_helper->deferred_setup = false;
 	return 0;
 }
 
@@ -2294,6 +2326,10 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 	if (!drm_fbdev_emulation)
 		return 0;
 
+	if (fb_helper->deferred_setup)
+		return drm_fb_helper_initial_config(fb_helper,
+						    fb_helper->preferred_bpp);
+
 	mutex_lock(&fb_helper->lock);
 	mutex_lock(&fb_helper->dev->mode_config.mutex);
 
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 7739be08ebca..dac155ad33f6 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -219,6 +219,27 @@ struct drm_fb_helper {
 	bool delayed_hotplug;
 
 	/**
+	 * @deferred_setup:
+	 *
+	 * If no outputs are connected (disconnected or unknown) the FB helper
+	 * code will defer setup until at least one of the outputs shows up.
+	 * This field keeps track of the status so that setup can be retried
+	 * at every hotplug event until it succeeds eventually.
+	 */
+	bool deferred_setup;
+
+	/**
+	 * @preferred_bpp:
+	 *
+	 * Temporary storage for the driver's preferred BPP setting passed to
+	 * FB helper initialization. This needs to be tracked so that deferred
+	 * FB helper setup can pass this on.
+	 *
+	 * See also: @deferred_setup
+	 */
+	int preferred_bpp;
+
+	/**
 	 * @atomic:
 	 *
 	 * Use atomic updates for restore_fbdev_mode(), etc.  This defaults to
-- 
2.8.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 7/9] drm/atmel-hlcdc: Remove custom FB helper deferred setup

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The FB helper core now supports deferred setup, so the driver's custom
implementation can be removed.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 8ded7645747e..a8a855910885 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -431,15 +431,7 @@ static void atmel_hlcdc_fb_output_poll_changed(struct drm_device *dev)
 {
 	struct atmel_hlcdc_dc *dc = dev->dev_private;
 
-	if (dc->fbdev) {
-		drm_fbdev_cma_hotplug_event(dc->fbdev);
-	} else {
-		dc->fbdev = drm_fbdev_cma_init(dev, 24,
-				dev->mode_config.num_crtc,
-				dev->mode_config.num_connector);
-		if (IS_ERR(dc->fbdev))
-			dc->fbdev = NULL;
-	}
+	drm_fbdev_cma_hotplug_event(dc->fbdev);
 }
 
 struct atmel_hlcdc_dc_commit {
@@ -654,11 +646,23 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
 
 	drm_kms_helper_poll_init(dev);
 
-	/* force connectors detection */
-	drm_helper_hpd_irq_event(dev);
+	dc->fbdev = drm_fbdev_cma_init(dev, 24, dev->mode_config.num_crtc,
+				       dev->mode_config.num_connector);
+	if (IS_ERR(dc->fbdev)) {
+		dev_err(dev->dev, "failed to setup fbdev\n");
+		ret = PTR_ERR(dc->fbdev);
+		goto err_cleanup_poll;
+	}
 
 	return 0;
 
+err_cleanup_poll:
+	drm_kms_helper_poll_fini(dev);
+
+	pm_runtime_get_sync(dev->dev);
+	drm_irq_uninstall(dev);
+	pm_runtime_put_sync(dev->dev);
+
 err_periph_clk_disable:
 	pm_runtime_disable(dev->dev);
 	clk_disable_unprepare(dc->hlcdc->periph_clk);
-- 
2.8.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 8/9] drm/exynos: Remove custom FB helper deferred setup

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The FB helper core now supports deferred setup, so the driver's custom
implementation can be removed.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c   | 8 ++++++--
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 2 --
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 2dd820e23b0c..259c2585c703 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -215,11 +215,15 @@ static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
 	/* init kms poll for handling hpd */
 	drm_kms_helper_poll_init(dev);
 
-	/* force connectors detection */
-	drm_helper_hpd_irq_event(dev);
+	ret = exynos_drm_fbdev_init(dev);
+	if (ret)
+		goto err_cleanup_poll;
 
 	return 0;
 
+err_cleanup_poll:
+	drm_kms_helper_poll_fini(dev);
+	exynos_drm_device_subdrv_remove(dev);
 err_cleanup_vblank:
 	drm_vblank_cleanup(dev);
 err_unbind_all:
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index 67dcd6831291..83a277946200 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -319,6 +319,4 @@ void exynos_drm_output_poll_changed(struct drm_device *dev)
 
 	if (fb_helper)
 		drm_fb_helper_hotplug_event(fb_helper);
-	else
-		exynos_drm_fbdev_init(dev);
 }
-- 
2.8.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 8/9] drm/exynos: Remove custom FB helper deferred setup

[Daniel Vetter]

On Tue, Jun 07, 2016 at 05:26:24PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The FB helper core now supports deferred setup, so the driver's custom
> implementation can be removed.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c   | 8 ++++++--
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 2 --
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 2dd820e23b0c..259c2585c703 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -215,11 +215,15 @@ static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
>  	/* init kms poll for handling hpd */
>  	drm_kms_helper_poll_init(dev);
>  
> -	/* force connectors detection */
> -	drm_helper_hpd_irq_event(dev);
> +	ret = exynos_drm_fbdev_init(dev);
> +	if (ret)
> +		goto err_cleanup_poll;
>  
>  	return 0;
>  
> +err_cleanup_poll:
> +	drm_kms_helper_poll_fini(dev);
> +	exynos_drm_device_subdrv_remove(dev);
>  err_cleanup_vblank:
>  	drm_vblank_cleanup(dev);
>  err_unbind_all:
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index 67dcd6831291..83a277946200 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -319,6 +319,4 @@ void exynos_drm_output_poll_changed(struct drm_device *dev)
>  
>  	if (fb_helper)
>  		drm_fb_helper_hotplug_event(fb_helper);

I think you can drop that if now too, since we always init the structure
(just not register the fbdev instance itself).
-Daniel

> -	else
> -		exynos_drm_fbdev_init(dev);
>  }
> -- 
> 2.8.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 9/9] drm/hisilicon: Remove custom FB helper deferred setup

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The FB helper core now supports deferred setup, so the driver's custom
implementation can be removed.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
index 3f94785fbcca..0e0bd77e4499 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
@@ -54,15 +54,7 @@ static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
 {
 	struct kirin_drm_private *priv = dev->dev_private;
 
-	if (priv->fbdev) {
-		drm_fbdev_cma_hotplug_event(priv->fbdev);
-	} else {
-		priv->fbdev = drm_fbdev_cma_init(dev, 32,
-				dev->mode_config.num_crtc,
-				dev->mode_config.num_connector);
-		if (IS_ERR(priv->fbdev))
-			priv->fbdev = NULL;
-	}
+	drm_fbdev_cma_hotplug_event(priv->fbdev);
 }
 #endif
 
@@ -129,11 +121,19 @@ static int kirin_drm_kms_init(struct drm_device *dev)
 	/* init kms poll for handling hpd */
 	drm_kms_helper_poll_init(dev);
 
-	/* force detection after connectors init */
-	(void)drm_helper_hpd_irq_event(dev);
+	priv->fbdev = drm_fbdev_cma_init(dev, 32, dev->mode_config.num_crtc,
+					 dev->mode_config.num_connector);
+	if (IS_ERR(priv->fbdev)) {
+		DRM_ERROR("failed to initialize fbdev.\n");
+		ret = PTR_ERR(priv->fbdev);
+		goto err_cleanup_poll;
+	}
 
 	return 0;
 
+err_cleanup_poll:
+	drm_kms_helper_poll_fini(dev);
+	drm_vblank_cleanup(dev);
 err_unbind_all:
 	component_unbind_all(dev->dev, dev);
 err_dc_cleanup:
-- 
2.8.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Ro.CI.BAT: failure for drm/fb-helper: Deferred setup support

[Patchwork]

== Series Details ==

Series: drm/fb-helper: Deferred setup support
URL   : https://patchwork.freedesktop.org/series/8410/
State : failure

== Summary ==

Applying: drm/fb-helper: Cleanup checkpatch warnings
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/drm_fb_helper.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/drm_fb_helper.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/drm_fb_helper.c
error: Failed to merge in the changes.
Patch failed at 0001 drm/fb-helper: Cleanup checkpatch warnings
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx