All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 00/11] drm/fb-helper: Deferred setup support
@ 2017-03-29 14:43 ` Thierry Reding
  2017-03-29 14:43   ` [PATCH v4 01/11] drm/fb-helper: Cleanup checkpatch warnings Thierry Reding
                     ` (12 more replies)
  0 siblings, 13 replies; 19+ messages in thread
From: Thierry Reding @ 2017-03-29 14:43 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: John Stultz, intel-gfx, dri-devel

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 v4:
- use fb_conn for variables of type struct drm_fb_helper_connector *
  for consistency
- make top-level FB helper lock more robust
- improve kerneldoc

Changes in v3:
- fix kerneldoc for top-level FB helper lock
- drop some patches that no longer apply
- add Tested-by from John Stultz
- add cleanup patches

Changes in v2:
- now with locking

Thierry Reding (11):
  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: Make top-level lock more robust
  drm/fb-helper: Support deferred setup
  drm/exynos: Remove custom FB helper deferred setup
  drm/hisilicon: Remove custom FB helper deferred setup
  drm/atmel-hlcdc: Remove unnecessary NULL check
  drm/rockchip: Remove unnecessary NULL check

 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c    |   3 +-
 drivers/gpu/drm/drm_fb_helper.c                 | 376 +++++++++++++++++-------
 drivers/gpu/drm/exynos/exynos_drm_drv.c         |   6 +-
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c       |  23 --
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c |  21 +-
 drivers/gpu/drm/i915/intel_dp_mst.c             |   3 -
 drivers/gpu/drm/radeon/radeon_dp_mst.c          |   7 -
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c      |   4 +-
 include/drm/drm_fb_helper.h                     |  35 +++
 9 files changed, 316 insertions(+), 162 deletions(-)

-- 
2.12.0

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v4 01/11] drm/fb-helper: Cleanup checkpatch warnings
  2017-03-29 14:43 ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Thierry Reding
@ 2017-03-29 14:43   ` Thierry Reding
  2017-03-29 14:43   ` [PATCH v4 02/11] drm/fb-helper: Reshuffle code for subsequent patches Thierry Reding
                     ` (11 subsequent siblings)
  12 siblings, 0 replies; 19+ messages in thread
From: Thierry Reding @ 2017-03-29 14:43 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx, dri-devel

From: Thierry Reding <treding@nvidia.com>

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

Tested-by: John Stultz <john.stultz@linaro.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 54 ++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index e65becd964a1..d06027a1b9fa 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -213,9 +213,9 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 	fb_helper_connector = fb_helper->connector_info[i];
 	drm_connector_put(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);
 
@@ -316,6 +316,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);
@@ -346,7 +347,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)
@@ -378,7 +379,7 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
 			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);
@@ -488,8 +489,10 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
 	struct drm_crtc *crtc;
 	int bound = 0, crtcs_bound = 0;
 
-	/* Sometimes user space wants everything disabled, so don't steal the
-	 * display if there's a master. */
+	/*
+	 * Sometimes user space wants everything disabled, so don't steal the
+	 * display if there's a master.
+	 */
 	if (READ_ONCE(dev->master))
 		return false;
 
@@ -537,6 +540,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");
@@ -870,9 +874,8 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 	mutex_lock(&kernel_fb_helper_lock);
 	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);
-		}
 	}
 	mutex_unlock(&kernel_fb_helper_lock);
 
@@ -1165,6 +1168,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;
 		}
@@ -1447,7 +1451,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)
@@ -1456,7 +1460,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;
@@ -1561,11 +1565,10 @@ 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 */
+	/* if driver picks 8 or 16 by default use that for both depth/bpp */
 	if (preferred_bpp != sizes.surface_bpp)
 		sizes.surface_depth = sizes.surface_bpp = preferred_bpp;
 
@@ -1630,6 +1633,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));
@@ -1645,8 +1649,10 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	}
 
 	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. */
+		/*
+		 * hmm everyone went away - assume VGA cable just fell out
+		 * and will come back later.
+		 */
 		DRM_INFO("Cannot find any crtc or sizes - going 1024x768\n");
 		sizes.fb_width = sizes.surface_width = 1024;
 		sizes.fb_height = sizes.surface_height = 768;
@@ -1703,7 +1709,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);
 
@@ -1725,6 +1730,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;
@@ -2057,13 +2063,15 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
 				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;
 
-			/* find the tile offsets for this pass - need
-			   to find all tiles left and above */
+			/*
+			 * find the tile offsets for this pass - need to find
+			 * all tiles left and above
+			 */
 			drm_get_tile_offsets(fb_helper, modes, offsets,
 					     i, fb_helper_conn->connector->tile_h_loc, fb_helper_conn->connector->tile_v_loc);
 		}
@@ -2147,8 +2155,10 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
 	if (!encoder)
 		goto out;
 
-	/* select a crtc for this connector and then attempt to configure
-	   remaining connectors */
+	/*
+	 * 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];
 
-- 
2.12.0

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 02/11] drm/fb-helper: Reshuffle code for subsequent patches
  2017-03-29 14:43 ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Thierry Reding
  2017-03-29 14:43   ` [PATCH v4 01/11] drm/fb-helper: Cleanup checkpatch warnings Thierry Reding
@ 2017-03-29 14:43   ` Thierry Reding
  2017-03-29 14:43   ` [PATCH v4 03/11] drm/fb-helper: Improve code readability Thierry Reding
                     ` (10 subsequent siblings)
  12 siblings, 0 replies; 19+ messages in thread
From: Thierry Reding @ 2017-03-29 14:43 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: John Stultz, intel-gfx, dri-devel

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.

Tested-by: John Stultz <john.stultz@linaro.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 59 ++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d06027a1b9fa..1581305c1053 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -109,6 +109,35 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
 	for (({ lockdep_assert_held(&(fbh)->dev->mode_config.mutex); }), \
 	     i__ = 0; i__ < (fbh)->connector_count; i__++)
 
+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_get(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
@@ -162,36 +191,6 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 }
 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_get(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.12.0

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 03/11] drm/fb-helper: Improve code readability
  2017-03-29 14:43 ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Thierry Reding
  2017-03-29 14:43   ` [PATCH v4 01/11] drm/fb-helper: Cleanup checkpatch warnings Thierry Reding
  2017-03-29 14:43   ` [PATCH v4 02/11] drm/fb-helper: Reshuffle code for subsequent patches Thierry Reding
@ 2017-03-29 14:43   ` Thierry Reding
  2017-03-29 14:43   ` [PATCH v4 04/11] drm/fb-helper: Push down modeset lock into FB helpers Thierry Reding
                     ` (9 subsequent siblings)
  12 siblings, 0 replies; 19+ messages in thread
From: Thierry Reding @ 2017-03-29 14:43 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx, dri-devel

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.

Tested-by: John Stultz <john.stultz@linaro.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 1581305c1053..673a47445d61 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -109,31 +109,38 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
 	for (({ lockdep_assert_held(&(fbh)->dev->mode_config.mutex); }), \
 	     i__ = 0; i__ < (fbh)->connector_count; i__++)
 
-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 *fb_conn;
 	struct drm_fb_helper_connector **temp;
-	struct drm_fb_helper_connector *fb_helper_connector;
+	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(fb_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)
+	fb_conn = kzalloc(sizeof(*fb_conn), GFP_KERNEL);
+	if (!fb_conn)
 		return -ENOMEM;
 
 	drm_connector_get(connector);
-	fb_helper_connector->connector = connector;
-	fb_helper->connector_info[fb_helper->connector_count++] = fb_helper_connector;
+	fb_conn->connector = connector;
+	fb_helper->connector_info[fb_helper->connector_count++] = fb_conn;
 	return 0;
 }
 EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
-- 
2.12.0

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 04/11] drm/fb-helper: Push down modeset lock into FB helpers
  2017-03-29 14:43 ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Thierry Reding
                     ` (2 preceding siblings ...)
  2017-03-29 14:43   ` [PATCH v4 03/11] drm/fb-helper: Improve code readability Thierry Reding
@ 2017-03-29 14:43   ` Thierry Reding
  2017-03-31 18:28     ` Daniel Vetter
  2017-03-29 14:43   ` [PATCH v4 05/11] drm/fb-helper: Add top-level lock Thierry Reding
                     ` (8 subsequent siblings)
  12 siblings, 1 reply; 19+ messages in thread
From: Thierry Reding @ 2017-03-29 14:43 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: John Stultz, intel-gfx, dri-devel

From: Thierry Reding <treding@nvidia.com>

Move the modeset locking from drivers into FB helpers.

Tested-by: John Stultz <john.stultz@linaro.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_fb_helper.c        | 40 +++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_dp_mst.c    |  3 ---
 drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ------
 3 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 673a47445d61..18105cbe9810 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -109,8 +109,8 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
 	for (({ lockdep_assert_held(&(fbh)->dev->mode_config.mutex); }), \
 	     i__ = 0; i__ < (fbh)->connector_count; i__++)
 
-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 *fb_conn;
 	struct drm_fb_helper_connector **temp;
@@ -141,8 +141,23 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
 	drm_connector_get(connector);
 	fb_conn->connector = connector;
 	fb_helper->connector_info[fb_helper->connector_count++] = fb_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);
 
 /**
@@ -172,8 +187,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 	mutex_lock(&dev->mode_config.mutex);
 	drm_connector_list_iter_begin(dev, &conn_iter);
 	drm_for_each_connector_iter(connector, &conn_iter) {
-		ret = drm_fb_helper_add_one_connector(fb_helper, connector);
-
+		ret = __drm_fb_helper_add_one_connector(fb_helper, connector);
 		if (ret)
 			goto fail;
 	}
@@ -198,8 +212,8 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 }
 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;
@@ -227,6 +241,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 c1f62eb07c07..1e3ee32a9acb 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -484,15 +484,12 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 					   struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
-	struct drm_device *dev = connector->dev;
 
 	drm_connector_unregister(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 6598306dca9b..ebdf1b859cb6 100644
--- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
+++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
@@ -300,9 +300,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);
 }
@@ -315,13 +313,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.12.0

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 05/11] drm/fb-helper: Add top-level lock
  2017-03-29 14:43 ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Thierry Reding
                     ` (3 preceding siblings ...)
  2017-03-29 14:43   ` [PATCH v4 04/11] drm/fb-helper: Push down modeset lock into FB helpers Thierry Reding
@ 2017-03-29 14:43   ` Thierry Reding
  2017-03-29 14:43   ` [PATCH v4 06/11] drm/fb-helper: Make top-level lock more robust Thierry Reding
                     ` (7 subsequent siblings)
  12 siblings, 0 replies; 19+ messages in thread
From: Thierry Reding @ 2017-03-29 14:43 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: John Stultz, intel-gfx, dri-devel

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.

Tested-by: John Stultz <john.stultz@linaro.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 26 +++++++++++++++++++++++++-
 include/drm/drm_fb_helper.h     | 12 ++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 18105cbe9810..860be51d92f6 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -119,6 +119,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;
@@ -150,11 +151,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;
 }
@@ -184,6 +187,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_connector_list_iter_begin(dev, &conn_iter);
 	drm_for_each_connector_iter(connector, &conn_iter) {
@@ -207,6 +211,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 out:
 	drm_connector_list_iter_end(&conn_iter);
 	mutex_unlock(&dev->mode_config.mutex);
+	mutex_unlock(&fb_helper->lock);
 
 	return ret;
 }
@@ -221,6 +226,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++) {
@@ -247,11 +253,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;
 }
@@ -503,16 +511,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);
@@ -748,6 +761,7 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
 	INIT_WORK(&helper->resume_work, drm_fb_helper_resume_worker);
 	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;
 }
@@ -913,6 +927,7 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 	}
 	mutex_unlock(&kernel_fb_helper_lock);
 
+	mutex_destroy(&fb_helper->lock);
 	drm_fb_helper_crtc_free(fb_helper);
 
 }
@@ -2422,25 +2437,34 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
 int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
+	int err = 0;
 
 	if (!drm_fbdev_emulation)
 		return 0;
 
+	mutex_lock(&fb_helper->lock);
 	mutex_lock(&dev->mode_config.mutex);
+
 	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
 		fb_helper->delayed_hotplug = true;
 		mutex_unlock(&dev->mode_config.mutex);
-		return 0;
+		goto unlock;
 	}
+
 	DRM_DEBUG_KMS("\n");
 
 	drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
 
 	mutex_unlock(&dev->mode_config.mutex);
+	mutex_unlock(&fb_helper->lock);
 
 	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 119e5e4609c7..44e2c57a7049 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -201,6 +201,18 @@ struct drm_fb_helper {
 	struct work_struct resume_work;
 
 	/**
+	 * @lock:
+	 *
+	 * Top-level FB helper lock. This protects all internal data structures
+	 * and lists, such as @connector_info and @crtc_info.
+	 *
+	 * FIXME: fbdev emulation locking is a mess and long term we want to
+	 * protect all helper internal state with this lock as well as reduce
+	 * core KMS locking as much as possible.
+	 */
+	struct mutex lock;
+
+	/**
 	 * @kernel_fb_list:
 	 *
 	 * Entry on the global kernel_fb_helper_list, used for kgdb entry/exit.
-- 
2.12.0

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 06/11] drm/fb-helper: Make top-level lock more robust
  2017-03-29 14:43 ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Thierry Reding
                     ` (4 preceding siblings ...)
  2017-03-29 14:43   ` [PATCH v4 05/11] drm/fb-helper: Add top-level lock Thierry Reding
@ 2017-03-29 14:43   ` Thierry Reding
  2017-03-29 14:51     ` Thierry Reding
  2017-04-03  8:40     ` [Intel-gfx] " Daniel Vetter
  2017-03-29 14:43   ` [PATCH v4 07/11] drm/fb-helper: Support deferred setup Thierry Reding
                     ` (6 subsequent siblings)
  12 siblings, 2 replies; 19+ messages in thread
From: Thierry Reding @ 2017-03-29 14:43 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: John Stultz, intel-gfx, dri-devel

From: Thierry Reding <treding@nvidia.com>

The existing drm_fb_helper_hotplug_event() function needs to take the
top-level fb-helper lock. However, the function can also be called from
code that has already taken this lock. Introduce an unlocked variant of
this function that can be used in the latter case.

This function calls drm_fb_helper_restore_fbdev_mode_unlocked(), via
drm_fb_helper_set_par(), so we also need to introduce an unlocked copy
of that to avoid recursive locking issues.

Similarly, the drm_fb_helper_initial_config() function ends up calling
drm_fb_helper_set_par(), via register_framebuffer(), and needs an
unlocked variant to avoid recursive locking.

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

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 860be51d92f6..21a90322531c 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -491,18 +491,10 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 	return 0;
 }
 
-/**
- * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration
- * @fb_helper: fbcon to restore
- *
- * This should be called from driver's drm &drm_driver.lastclose callback
- * when implementing an fbcon on top of kms using this helper. This ensures that
- * the user isn't greeted with a black screen when e.g. X dies.
- *
- * RETURNS:
- * Zero if everything went ok, negative error code otherwise.
- */
-int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
+static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper);
+
+static int
+__drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
 	bool do_delayed;
@@ -511,7 +503,8 @@ 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);
+	WARN_ON(!mutex_is_locked(&fb_helper->lock));
+
 	drm_modeset_lock_all(dev);
 
 	ret = restore_fbdev_mode(fb_helper);
@@ -521,10 +514,31 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 		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);
+		__drm_fb_helper_hotplug_event(fb_helper);
+
+	return ret;
+}
+
+/**
+ * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration
+ * @fb_helper: fbcon to restore
+ *
+ * This should be called from driver's drm &drm_driver.lastclose callback
+ * when implementing an fbcon on top of kms using this helper. This ensures that
+ * the user isn't greeted with a black screen when e.g. X dies.
+ *
+ * RETURNS:
+ * Zero if everything went ok, negative error code otherwise.
+ */
+int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
+{
+	int ret;
+
+	mutex_lock(&fb_helper->lock);
+	ret = __drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
+	mutex_unlock(&fb_helper->lock);
 
 	return ret;
 }
@@ -1486,7 +1500,7 @@ int drm_fb_helper_set_par(struct fb_info *info)
 		return -EINVAL;
 	}
 
-	drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
+	__drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
 
 	return 0;
 }
@@ -2333,6 +2347,46 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
 	kfree(enabled);
 }
 
+static int __drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper,
+					  int bpp_sel)
+{
+	struct drm_device *dev = fb_helper->dev;
+	struct fb_info *info;
+	int ret;
+
+	if (!drm_fbdev_emulation)
+		return 0;
+
+	WARN_ON(!mutex_is_locked(&fb_helper->lock));
+
+	mutex_lock(&dev->mode_config.mutex);
+	drm_setup_crtcs(fb_helper,
+			dev->mode_config.max_width,
+			dev->mode_config.max_height);
+	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
+	mutex_unlock(&dev->mode_config.mutex);
+	if (ret)
+		return ret;
+
+	info = fb_helper->fbdev;
+	info->var.pixclock = 0;
+	ret = register_framebuffer(info);
+	if (ret < 0)
+		return ret;
+
+	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
+		 info->node, info->fix.id);
+
+	mutex_lock(&kernel_fb_helper_lock);
+	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);
+	mutex_unlock(&kernel_fb_helper_lock);
+
+	return 0;
+}
+
 /**
  * drm_fb_helper_initial_config - setup a sane initial connector configuration
  * @fb_helper: fb_helper device struct
@@ -2377,41 +2431,50 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
  */
 int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 {
-	struct drm_device *dev = fb_helper->dev;
-	struct fb_info *info;
 	int ret;
 
+	mutex_lock(&fb_helper->lock);
+	ret = __drm_fb_helper_initial_config(fb_helper, bpp_sel);
+	mutex_unlock(&fb_helper->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_fb_helper_initial_config);
+
+static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
+{
+	struct drm_device *dev = fb_helper->dev;
+	unsigned int width, height;
+
 	if (!drm_fbdev_emulation)
 		return 0;
 
+	WARN_ON(!mutex_is_locked(&fb_helper->lock));
+
 	mutex_lock(&dev->mode_config.mutex);
-	drm_setup_crtcs(fb_helper,
-			dev->mode_config.max_width,
-			dev->mode_config.max_height);
-	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
-	mutex_unlock(&dev->mode_config.mutex);
-	if (ret)
-		return ret;
 
-	info = fb_helper->fbdev;
-	info->var.pixclock = 0;
-	ret = register_framebuffer(info);
-	if (ret < 0)
-		return ret;
+	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
+		fb_helper->delayed_hotplug = true;
+		mutex_unlock(&dev->mode_config.mutex);
+		return 0;
+	}
 
-	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
-		 info->node, info->fix.id);
+	DRM_DEBUG_KMS("\n");
 
-	mutex_lock(&kernel_fb_helper_lock);
-	if (list_empty(&kernel_fb_helper_list))
-		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
+	width = dev->mode_config.max_width;
+	height = dev->mode_config.max_height;
 
-	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
-	mutex_unlock(&kernel_fb_helper_lock);
+	if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
+		DRM_DEBUG_KMS("No connectors reported connected with modes\n");
+
+	drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
+
+	mutex_unlock(&dev->mode_config.mutex);
+
+	drm_fb_helper_set_par(fb_helper->fbdev);
 
 	return 0;
 }
-EXPORT_SYMBOL(drm_fb_helper_initial_config);
 
 /**
  * drm_fb_helper_hotplug_event - respond to a hotplug notification by
@@ -2436,35 +2499,13 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
  */
 int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 {
-	struct drm_device *dev = fb_helper->dev;
-	int err = 0;
-
-	if (!drm_fbdev_emulation)
-		return 0;
+	int ret;
 
 	mutex_lock(&fb_helper->lock);
-	mutex_lock(&dev->mode_config.mutex);
-
-	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
-		fb_helper->delayed_hotplug = true;
-		mutex_unlock(&dev->mode_config.mutex);
-		goto unlock;
-	}
-
-	DRM_DEBUG_KMS("\n");
-
-	drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
-
-	mutex_unlock(&dev->mode_config.mutex);
+	ret = __drm_fb_helper_hotplug_event(fb_helper);
 	mutex_unlock(&fb_helper->lock);
 
-	drm_fb_helper_set_par(fb_helper->fbdev);
-
-	return 0;
-
-unlock:
-	mutex_unlock(&fb_helper->lock);
-	return err;
+	return ret;
 }
 EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
 
-- 
2.12.0

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 07/11] drm/fb-helper: Support deferred setup
  2017-03-29 14:43 ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Thierry Reding
                     ` (5 preceding siblings ...)
  2017-03-29 14:43   ` [PATCH v4 06/11] drm/fb-helper: Make top-level lock more robust Thierry Reding
@ 2017-03-29 14:43   ` Thierry Reding
  2017-03-29 14:43   ` [PATCH v4 08/11] drm/exynos: Remove custom FB helper " Thierry Reding
                     ` (5 subsequent siblings)
  12 siblings, 0 replies; 19+ messages in thread
From: Thierry Reding @ 2017-03-29 14:43 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx, dri-devel

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.

Tested-by: John Stultz <john.stultz@linaro.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 53 +++++++++++++++++++++++++++++++++++++----
 include/drm/drm_fb_helper.h     | 23 ++++++++++++++++++
 2 files changed, 71 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 21a90322531c..e9fe47d218e1 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -505,6 +505,9 @@ __drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 
 	WARN_ON(!mutex_is_locked(&fb_helper->lock));
 
+	if (fb_helper->deferred_setup)
+		return 0;
+
 	drm_modeset_lock_all(dev);
 
 	ret = restore_fbdev_mode(fb_helper);
@@ -1611,6 +1614,23 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 }
 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
@@ -2268,8 +2288,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
 	int i;
 
 	DRM_DEBUG_KMS("\n");
-	if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
-		DRM_DEBUG_KMS("No connectors reported connected with modes\n");
 
 	/* prevent concurrent modification of connector_count by hotplug */
 	lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
@@ -2351,6 +2369,7 @@ static int __drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper,
 					  int bpp_sel)
 {
 	struct drm_device *dev = fb_helper->dev;
+	unsigned int width, height;
 	struct fb_info *info;
 	int ret;
 
@@ -2360,14 +2379,34 @@ static int __drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper,
 	WARN_ON(!mutex_is_locked(&fb_helper->lock));
 
 	mutex_lock(&dev->mode_config.mutex);
-	drm_setup_crtcs(fb_helper,
-			dev->mode_config.max_width,
-			dev->mode_config.max_height);
+
+	width = dev->mode_config.max_width;
+	height = dev->mode_config.max_height;
+
+	if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
+		DRM_DEBUG_KMS("No connectors reported connected with modes\n");
+
+	/*
+	 * 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 = bpp_sel;
+		fb_helper->deferred_setup = true;
+		mutex_unlock(&dev->mode_config.mutex);
+		return 0;
+	}
+
+	drm_setup_crtcs(fb_helper, width, height);
+
 	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
 	mutex_unlock(&dev->mode_config.mutex);
 	if (ret)
 		return ret;
 
+	fb_helper->deferred_setup = false;
+
 	info = fb_helper->fbdev;
 	info->var.pixclock = 0;
 	ret = register_framebuffer(info);
@@ -2451,6 +2490,10 @@ static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 
 	WARN_ON(!mutex_is_locked(&fb_helper->lock));
 
+	if (fb_helper->deferred_setup)
+		return __drm_fb_helper_initial_config(fb_helper,
+						      fb_helper->preferred_bpp);
+
 	mutex_lock(&dev->mode_config.mutex);
 
 	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 44e2c57a7049..b0af21b371ea 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -227,6 +227,29 @@ struct drm_fb_helper {
 	 * needs to be reprobe when fbdev is in control again.
 	 */
 	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.
+	 *
+	 * Protected by @lock.
+	 */
+	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;
 };
 
 /**
-- 
2.12.0

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 08/11] drm/exynos: Remove custom FB helper deferred setup
  2017-03-29 14:43 ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Thierry Reding
                     ` (6 preceding siblings ...)
  2017-03-29 14:43   ` [PATCH v4 07/11] drm/fb-helper: Support deferred setup Thierry Reding
@ 2017-03-29 14:43   ` Thierry Reding
  2017-03-29 17:50     ` Daniel Vetter
  2017-03-29 14:43   ` [PATCH v4 09/11] drm/hisilicon: " Thierry Reding
                     ` (4 subsequent siblings)
  12 siblings, 1 reply; 19+ messages in thread
From: Thierry Reding @ 2017-03-29 14:43 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: John Stultz, intel-gfx, dri-devel

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   |  6 ++++--
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 23 -----------------------
 2 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 09d3c4c3c858..08f9533ddbe8 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -399,8 +399,9 @@ static int exynos_drm_bind(struct device *dev)
 	/* init kms poll for handling hpd */
 	drm_kms_helper_poll_init(drm);
 
-	/* force connectors detection */
-	drm_helper_hpd_irq_event(drm);
+	ret = exynos_drm_fbdev_init(drm);
+	if (ret)
+		goto err_cleanup_poll;
 
 	/* register the DRM device */
 	ret = drm_dev_register(drm, 0);
@@ -411,6 +412,7 @@ static int exynos_drm_bind(struct device *dev)
 
 err_cleanup_fbdev:
 	exynos_drm_fbdev_fini(drm);
+err_cleanup_poll:
 	drm_kms_helper_poll_fini(drm);
 	exynos_drm_device_subdrv_remove(drm);
 err_cleanup_vblank:
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index 641531243e04..e64a1041dd29 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -183,24 +183,6 @@ static const struct drm_fb_helper_funcs exynos_drm_fb_helper_funcs = {
 	.fb_probe =	exynos_drm_fbdev_create,
 };
 
-static bool exynos_drm_fbdev_is_anything_connected(struct drm_device *dev)
-{
-	struct drm_connector *connector;
-	bool ret = false;
-
-	mutex_lock(&dev->mode_config.mutex);
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		if (connector->status != connector_status_connected)
-			continue;
-
-		ret = true;
-		break;
-	}
-	mutex_unlock(&dev->mode_config.mutex);
-
-	return ret;
-}
-
 int exynos_drm_fbdev_init(struct drm_device *dev)
 {
 	struct exynos_drm_fbdev *fbdev;
@@ -211,9 +193,6 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
 	if (!dev->mode_config.num_crtc || !dev->mode_config.num_connector)
 		return 0;
 
-	if (!exynos_drm_fbdev_is_anything_connected(dev))
-		return 0;
-
 	fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
 	if (!fbdev)
 		return -ENOMEM;
@@ -306,6 +285,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.12.0

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 09/11] drm/hisilicon: Remove custom FB helper deferred setup
  2017-03-29 14:43 ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Thierry Reding
                     ` (7 preceding siblings ...)
  2017-03-29 14:43   ` [PATCH v4 08/11] drm/exynos: Remove custom FB helper " Thierry Reding
@ 2017-03-29 14:43   ` Thierry Reding
  2017-03-29 14:44   ` [PATCH v4 10/11] drm/atmel-hlcdc: Remove unnecessary NULL check Thierry Reding
                     ` (3 subsequent siblings)
  12 siblings, 0 replies; 19+ messages in thread
From: Thierry Reding @ 2017-03-29 14:43 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: John Stultz, intel-gfx, dri-devel

From: Thierry Reding <treding@nvidia.com>

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

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
index df4f50713e54..408c7cfc180c 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
@@ -55,14 +55,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_connector);
-		if (IS_ERR(priv->fbdev))
-			priv->fbdev = NULL;
-	}
+	drm_fbdev_cma_hotplug_event(priv->fbdev);
 }
 #endif
 
@@ -129,11 +122,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_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.12.0

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 10/11] drm/atmel-hlcdc: Remove unnecessary NULL check
  2017-03-29 14:43 ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Thierry Reding
                     ` (8 preceding siblings ...)
  2017-03-29 14:43   ` [PATCH v4 09/11] drm/hisilicon: " Thierry Reding
@ 2017-03-29 14:44   ` Thierry Reding
  2017-03-29 14:44   ` [PATCH v4 11/11] drm/rockchip: " Thierry Reding
                     ` (2 subsequent siblings)
  12 siblings, 0 replies; 19+ messages in thread
From: Thierry Reding @ 2017-03-29 14:44 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: John Stultz, intel-gfx, dri-devel

From: Thierry Reding <treding@nvidia.com>

drm_fbdev_cma_hotplug_event() already checks for NULL pointers before
dereferencing, so callers don't need to do that.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index f4a3065f7f51..31443402b9a9 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -450,8 +450,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);
+	drm_fbdev_cma_hotplug_event(dc->fbdev);
 }
 
 struct atmel_hlcdc_dc_commit {
-- 
2.12.0

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 11/11] drm/rockchip: Remove unnecessary NULL check
  2017-03-29 14:43 ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Thierry Reding
                     ` (9 preceding siblings ...)
  2017-03-29 14:44   ` [PATCH v4 10/11] drm/atmel-hlcdc: Remove unnecessary NULL check Thierry Reding
@ 2017-03-29 14:44   ` Thierry Reding
  2017-06-21 13:39     ` Daniel Vetter
  2017-03-29 15:04   ` ✗ Fi.CI.BAT: warning for drm/fb-helper: Deferred setup support (rev3) Patchwork
  2017-03-30 10:21   ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Andrzej Hajda
  12 siblings, 1 reply; 19+ messages in thread
From: Thierry Reding @ 2017-03-29 14:44 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx, dri-devel

From: Thierry Reding <treding@nvidia.com>

The expression &private->fbdev_helper can never be NULL, so the check is
completely unnecessary.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 81f9548672b0..df6bceabeca8 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -168,10 +168,8 @@ rockchip_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
 static void rockchip_drm_output_poll_changed(struct drm_device *dev)
 {
 	struct rockchip_drm_private *private = dev->dev_private;
-	struct drm_fb_helper *fb_helper = &private->fbdev_helper;
 
-	if (fb_helper)
-		drm_fb_helper_hotplug_event(fb_helper);
+	drm_fb_helper_hotplug_event(&private->fbdev_helper);
 }
 
 static void
-- 
2.12.0

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 06/11] drm/fb-helper: Make top-level lock more robust
  2017-03-29 14:43   ` [PATCH v4 06/11] drm/fb-helper: Make top-level lock more robust Thierry Reding
@ 2017-03-29 14:51     ` Thierry Reding
  2017-04-03  8:40     ` [Intel-gfx] " Daniel Vetter
  1 sibling, 0 replies; 19+ messages in thread
From: Thierry Reding @ 2017-03-29 14:51 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx, dri-devel


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

On Wed, Mar 29, 2017 at 04:43:56PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The existing drm_fb_helper_hotplug_event() function needs to take the
> top-level fb-helper lock. However, the function can also be called from
> code that has already taken this lock. Introduce an unlocked variant of
> this function that can be used in the latter case.
> 
> This function calls drm_fb_helper_restore_fbdev_mode_unlocked(), via
> drm_fb_helper_set_par(), so we also need to introduce an unlocked copy
> of that to avoid recursive locking issues.
> 
> Similarly, the drm_fb_helper_initial_config() function ends up calling
> drm_fb_helper_set_par(), via register_framebuffer(), and needs an
> unlocked variant to avoid recursive locking.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 167 +++++++++++++++++++++++++---------------
>  1 file changed, 104 insertions(+), 63 deletions(-)

Note that this could probably be squashed into 05/11, but I left it
separate for easier review since it's the only new patch in the series.

Also I had originally split this into three separate patches, but the
recursive call stack for the drm_fb_helper_hotplug_event() function and
drm_fb_helper_restore_fbdev_mode_unlocked() made it impossible to keep
bisectibility across the series.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* ✗ Fi.CI.BAT: warning for drm/fb-helper: Deferred setup support (rev3)
  2017-03-29 14:43 ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Thierry Reding
                     ` (10 preceding siblings ...)
  2017-03-29 14:44   ` [PATCH v4 11/11] drm/rockchip: " Thierry Reding
@ 2017-03-29 15:04   ` Patchwork
  2017-03-30 10:21   ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Andrzej Hajda
  12 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2017-03-29 15:04 UTC (permalink / raw)
  To: Thierry Reding; +Cc: intel-gfx

== Series Details ==

Series: drm/fb-helper: Deferred setup support (rev3)
URL   : https://patchwork.freedesktop.org/series/8410/
State : warning

== Summary ==

Series 8410v3 drm/fb-helper: Deferred setup support
https://patchwork.freedesktop.org/api/1.0/series/8410/revisions/3/mbox/

Test core_auth:
        Subgroup basic-auth:
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-bxt-j4205)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-kbl-7560u)
                pass       -> DMESG-WARN (fi-bsw-n3050)
                pass       -> DMESG-WARN (fi-ivb-3770)
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-bxt-t5700)
Test core_prop_blob:
        Subgroup basic:
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-bxt-j4205)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-kbl-7560u)
                pass       -> DMESG-WARN (fi-bsw-n3050)
                pass       -> DMESG-WARN (fi-ivb-3770)
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-bxt-t5700)
Test drv_getparams_basic:
        Subgroup basic-eu-total:
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-bxt-j4205)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-kbl-7560u)
                pass       -> DMESG-WARN (fi-bsw-n3050)
                pass       -> DMESG-WARN (fi-ivb-3770)
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-bxt-t5700)
        Subgroup basic-subslice-total:
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-bxt-j4205)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-kbl-7560u)
                pass       -> DMESG-WARN (fi-bsw-n3050)
                pass       -> DMESG-WARN (fi-ivb-3770)
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-bxt-t5700)
Test drv_hangman:
        Subgroup error-state-basic:
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-bxt-j4205)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-kbl-7560u)
                pass       -> DMESG-WARN (fi-bsw-n3050)
                pass       -> DMESG-WARN (fi-ivb-3770)
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-bxt-t5700)
Test gem_basic:
        Subgroup bad-close:
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-bxt-j4205)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-kbl-7560u)
                pass       -> DMESG-WARN (fi-bsw-n3050)
                pass       -> DMESG-WARN (fi-ivb-3770)
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-bxt-t5700)
        Subgroup create-close:
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-bxt-j4205)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-skl-6770hq)
WARNING: Long output truncated

2f9f22b419350cafb06ba7e5342bc461fcb0afca drm-tip: 2017y-03m-29d-12h-52m-56s UTC integration manifest
78b4e48 drm/rockchip: Remove unnecessary NULL check
676c5ac drm/atmel-hlcdc: Remove unnecessary NULL check
f3c07bb drm/hisilicon: Remove custom FB helper deferred setup
994f2b0 drm/exynos: Remove custom FB helper deferred setup
d0764de drm/fb-helper: Support deferred setup
e4f394a drm/fb-helper: Make top-level lock more robust
fd2834b drm/fb-helper: Add top-level lock
8888ce9 drm/fb-helper: Push down modeset lock into FB helpers
181ab83 drm/fb-helper: Improve code readability
0b72cbf drm/fb-helper: Reshuffle code for subsequent patches
03515d3 drm/fb-helper: Cleanup checkpatch warnings

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4343/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 08/11] drm/exynos: Remove custom FB helper deferred setup
  2017-03-29 14:43   ` [PATCH v4 08/11] drm/exynos: Remove custom FB helper " Thierry Reding
@ 2017-03-29 17:50     ` Daniel Vetter
  0 siblings, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2017-03-29 17:50 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Daniel Vetter, intel-gfx, John Stultz, dri-devel

On Wed, Mar 29, 2017 at 04:43:58PM +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   |  6 ++++--
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 23 -----------------------
>  2 files changed, 4 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 09d3c4c3c858..08f9533ddbe8 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -399,8 +399,9 @@ static int exynos_drm_bind(struct device *dev)
>  	/* init kms poll for handling hpd */
>  	drm_kms_helper_poll_init(drm);
>  
> -	/* force connectors detection */
> -	drm_helper_hpd_irq_event(drm);
> +	ret = exynos_drm_fbdev_init(drm);
> +	if (ret)
> +		goto err_cleanup_poll;
>  
>  	/* register the DRM device */
>  	ret = drm_dev_register(drm, 0);
> @@ -411,6 +412,7 @@ static int exynos_drm_bind(struct device *dev)
>  
>  err_cleanup_fbdev:
>  	exynos_drm_fbdev_fini(drm);
> +err_cleanup_poll:
>  	drm_kms_helper_poll_fini(drm);
>  	exynos_drm_device_subdrv_remove(drm);
>  err_cleanup_vblank:
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index 641531243e04..e64a1041dd29 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -183,24 +183,6 @@ static const struct drm_fb_helper_funcs exynos_drm_fb_helper_funcs = {
>  	.fb_probe =	exynos_drm_fbdev_create,
>  };
>  
> -static bool exynos_drm_fbdev_is_anything_connected(struct drm_device *dev)
> -{
> -	struct drm_connector *connector;
> -	bool ret = false;
> -
> -	mutex_lock(&dev->mode_config.mutex);
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> -		if (connector->status != connector_status_connected)
> -			continue;
> -
> -		ret = true;
> -		break;
> -	}
> -	mutex_unlock(&dev->mode_config.mutex);
> -
> -	return ret;
> -}
> -
>  int exynos_drm_fbdev_init(struct drm_device *dev)
>  {
>  	struct exynos_drm_fbdev *fbdev;
> @@ -211,9 +193,6 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
>  	if (!dev->mode_config.num_crtc || !dev->mode_config.num_connector)
>  		return 0;
>  
> -	if (!exynos_drm_fbdev_is_anything_connected(dev))
> -		return 0;
> -
>  	fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
>  	if (!fbdev)
>  		return -ENOMEM;
> @@ -306,6 +285,4 @@ void exynos_drm_output_poll_changed(struct drm_device *dev)
>  
>  	if (fb_helper)
>  		drm_fb_helper_hotplug_event(fb_helper);

Tiny nit: I think you can remove the above NULL check too. With that:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> -	else
> -		exynos_drm_fbdev_init(dev);
>  }
> -- 
> 2.12.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 00/11] drm/fb-helper: Deferred setup support
  2017-03-29 14:43 ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Thierry Reding
                     ` (11 preceding siblings ...)
  2017-03-29 15:04   ` ✗ Fi.CI.BAT: warning for drm/fb-helper: Deferred setup support (rev3) Patchwork
@ 2017-03-30 10:21   ` Andrzej Hajda
  12 siblings, 0 replies; 19+ messages in thread
From: Andrzej Hajda @ 2017-03-30 10:21 UTC (permalink / raw)
  To: Thierry Reding, Daniel Vetter; +Cc: intel-gfx, dri-devel

Hi Thierry,

On 29.03.2017 16:43, Thierry Reding wrote:
> 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.

I have tested it on Exynos (TM2), generally it works but I have observed
two things:
1. Now fbdev appears on only one connected device (in my case, only on
DSI/Panel, not on HDMI/MHL/TV), I do not know if this change is intended
but I have not seen it in commit message.
2. "echo off >/sys/class/drm/card0-*/status" does not work anymore, ie
it succeeds, subsequent calls to modetest shows that connector is
disabled, but panel/TV still displays the image.

Regards
Andrzej

>
> Thierry
>
> Changes in v4:
> - use fb_conn for variables of type struct drm_fb_helper_connector *
>   for consistency
> - make top-level FB helper lock more robust
> - improve kerneldoc
>
> Changes in v3:
> - fix kerneldoc for top-level FB helper lock
> - drop some patches that no longer apply
> - add Tested-by from John Stultz
> - add cleanup patches
>
> Changes in v2:
> - now with locking
>
> Thierry Reding (11):
>   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: Make top-level lock more robust
>   drm/fb-helper: Support deferred setup
>   drm/exynos: Remove custom FB helper deferred setup
>   drm/hisilicon: Remove custom FB helper deferred setup
>   drm/atmel-hlcdc: Remove unnecessary NULL check
>   drm/rockchip: Remove unnecessary NULL check
>
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c    |   3 +-
>  drivers/gpu/drm/drm_fb_helper.c                 | 376 +++++++++++++++++-------
>  drivers/gpu/drm/exynos/exynos_drm_drv.c         |   6 +-
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c       |  23 --
>  drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c |  21 +-
>  drivers/gpu/drm/i915/intel_dp_mst.c             |   3 -
>  drivers/gpu/drm/radeon/radeon_dp_mst.c          |   7 -
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c      |   4 +-
>  include/drm/drm_fb_helper.h                     |  35 +++
>  9 files changed, 316 insertions(+), 162 deletions(-)
>

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 04/11] drm/fb-helper: Push down modeset lock into FB helpers
  2017-03-29 14:43   ` [PATCH v4 04/11] drm/fb-helper: Push down modeset lock into FB helpers Thierry Reding
@ 2017-03-31 18:28     ` Daniel Vetter
  0 siblings, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2017-03-31 18:28 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Daniel Vetter, intel-gfx, John Stultz, dri-devel

On Wed, Mar 29, 2017 at 04:43:54PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Move the modeset locking from drivers into FB helpers.
> 
> Tested-by: John Stultz <john.stultz@linaro.org>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c        | 40 +++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_dp_mst.c    |  3 ---
>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ------
>  3 files changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 673a47445d61..18105cbe9810 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -109,8 +109,8 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
>  	for (({ lockdep_assert_held(&(fbh)->dev->mode_config.mutex); }), \
>  	     i__ = 0; i__ < (fbh)->connector_count; i__++)
>  
> -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 *fb_conn;
>  	struct drm_fb_helper_connector **temp;
> @@ -141,8 +141,23 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
>  	drm_connector_get(connector);
>  	fb_conn->connector = connector;
>  	fb_helper->connector_info[fb_helper->connector_count++] = fb_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);
>  
>  /**
> @@ -172,8 +187,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
>  	mutex_lock(&dev->mode_config.mutex);
>  	drm_connector_list_iter_begin(dev, &conn_iter);
>  	drm_for_each_connector_iter(connector, &conn_iter) {
> -		ret = drm_fb_helper_add_one_connector(fb_helper, connector);
> -
> +		ret = __drm_fb_helper_add_one_connector(fb_helper, connector);
>  		if (ret)
>  			goto fail;
>  	}
> @@ -198,8 +212,8 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
>  }
>  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;
> @@ -227,6 +241,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 c1f62eb07c07..1e3ee32a9acb 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -484,15 +484,12 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>  					   struct drm_connector *connector)
>  {
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
> -	struct drm_device *dev = connector->dev;
>  
>  	drm_connector_unregister(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);

I missed that you missed the add_to_fbdev case :( I merged the first 3
patches to drm-misc.

Thanks, Daniel

>  
>  	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 6598306dca9b..ebdf1b859cb6 100644
> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> @@ -300,9 +300,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);
>  }
> @@ -315,13 +313,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.12.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v4 06/11] drm/fb-helper: Make top-level lock more robust
  2017-03-29 14:43   ` [PATCH v4 06/11] drm/fb-helper: Make top-level lock more robust Thierry Reding
  2017-03-29 14:51     ` Thierry Reding
@ 2017-04-03  8:40     ` Daniel Vetter
  1 sibling, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2017-04-03  8:40 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Daniel Vetter, intel-gfx, dri-devel

On Wed, Mar 29, 2017 at 04:43:56PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The existing drm_fb_helper_hotplug_event() function needs to take the
> top-level fb-helper lock. However, the function can also be called from
> code that has already taken this lock. Introduce an unlocked variant of
> this function that can be used in the latter case.
> 
> This function calls drm_fb_helper_restore_fbdev_mode_unlocked(), via
> drm_fb_helper_set_par(), so we also need to introduce an unlocked copy
> of that to avoid recursive locking issues.
> 
> Similarly, the drm_fb_helper_initial_config() function ends up calling
> drm_fb_helper_set_par(), via register_framebuffer(), and needs an
> unlocked variant to avoid recursive locking.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 167 +++++++++++++++++++++++++---------------
>  1 file changed, 104 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 860be51d92f6..21a90322531c 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -491,18 +491,10 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>  	return 0;
>  }
>  
> -/**
> - * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration
> - * @fb_helper: fbcon to restore
> - *
> - * This should be called from driver's drm &drm_driver.lastclose callback
> - * when implementing an fbcon on top of kms using this helper. This ensures that
> - * the user isn't greeted with a black screen when e.g. X dies.
> - *
> - * RETURNS:
> - * Zero if everything went ok, negative error code otherwise.
> - */
> -int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper);
> +
> +static int
> +__drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>  {
>  	struct drm_device *dev = fb_helper->dev;
>  	bool do_delayed;
> @@ -511,7 +503,8 @@ 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);
> +	WARN_ON(!mutex_is_locked(&fb_helper->lock));

lockdep_assert_held is the new cool.

> +
>  	drm_modeset_lock_all(dev);
>  
>  	ret = restore_fbdev_mode(fb_helper);
> @@ -521,10 +514,31 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>  		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);
> +		__drm_fb_helper_hotplug_event(fb_helper);
> +
> +	return ret;
> +}
> +
> +/**
> + * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration
> + * @fb_helper: fbcon to restore
> + *
> + * This should be called from driver's drm &drm_driver.lastclose callback
> + * when implementing an fbcon on top of kms using this helper. This ensures that
> + * the user isn't greeted with a black screen when e.g. X dies.
> + *
> + * RETURNS:
> + * Zero if everything went ok, negative error code otherwise.
> + */
> +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> +{
> +	int ret;
> +
> +	mutex_lock(&fb_helper->lock);
> +	ret = __drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> +	mutex_unlock(&fb_helper->lock);
>  
>  	return ret;
>  }
> @@ -1486,7 +1500,7 @@ int drm_fb_helper_set_par(struct fb_info *info)
>  		return -EINVAL;
>  	}
>  
> -	drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> +	__drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);

Nah, you need the locking still for when this is called from userspace
through fbdev ioctl.
>  
>  	return 0;
>  }
> @@ -2333,6 +2347,46 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  	kfree(enabled);
>  }
>  
> +static int __drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper,
> +					  int bpp_sel)
> +{
> +	struct drm_device *dev = fb_helper->dev;
> +	struct fb_info *info;
> +	int ret;
> +
> +	if (!drm_fbdev_emulation)
> +		return 0;
> +
> +	WARN_ON(!mutex_is_locked(&fb_helper->lock));
> +
> +	mutex_lock(&dev->mode_config.mutex);
> +	drm_setup_crtcs(fb_helper,
> +			dev->mode_config.max_width,
> +			dev->mode_config.max_height);
> +	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> +	mutex_unlock(&dev->mode_config.mutex);
> +	if (ret)
> +		return ret;
> +
> +	info = fb_helper->fbdev;
> +	info->var.pixclock = 0;
> +	ret = register_framebuffer(info);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
> +		 info->node, info->fix.id);
> +
> +	mutex_lock(&kernel_fb_helper_lock);
> +	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);
> +	mutex_unlock(&kernel_fb_helper_lock);
> +
> +	return 0;
> +}
> +
>  /**
>   * drm_fb_helper_initial_config - setup a sane initial connector configuration
>   * @fb_helper: fb_helper device struct
> @@ -2377,41 +2431,50 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>   */
>  int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
>  {
> -	struct drm_device *dev = fb_helper->dev;
> -	struct fb_info *info;
>  	int ret;
>  
> +	mutex_lock(&fb_helper->lock);
> +	ret = __drm_fb_helper_initial_config(fb_helper, bpp_sel);
> +	mutex_unlock(&fb_helper->lock);

Yeah this won't work because it'll deadlock with the register_framebuffer.
You need to push it down so that it only protects the same critical
section as mode_config.mutex currently ddoes.

> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_fb_helper_initial_config);
> +
> +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
> +{
> +	struct drm_device *dev = fb_helper->dev;
> +	unsigned int width, height;
> +
>  	if (!drm_fbdev_emulation)
>  		return 0;
>  
> +	WARN_ON(!mutex_is_locked(&fb_helper->lock));
> +
>  	mutex_lock(&dev->mode_config.mutex);
> -	drm_setup_crtcs(fb_helper,
> -			dev->mode_config.max_width,
> -			dev->mode_config.max_height);
> -	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> -	mutex_unlock(&dev->mode_config.mutex);
> -	if (ret)
> -		return ret;
>  
> -	info = fb_helper->fbdev;
> -	info->var.pixclock = 0;
> -	ret = register_framebuffer(info);
> -	if (ret < 0)
> -		return ret;
> +	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
> +		fb_helper->delayed_hotplug = true;
> +		mutex_unlock(&dev->mode_config.mutex);
> +		return 0;
> +	}
>  
> -	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
> -		 info->node, info->fix.id);
> +	DRM_DEBUG_KMS("\n");
>  
> -	mutex_lock(&kernel_fb_helper_lock);
> -	if (list_empty(&kernel_fb_helper_list))
> -		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
> +	width = dev->mode_config.max_width;
> +	height = dev->mode_config.max_height;
>  
> -	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> -	mutex_unlock(&kernel_fb_helper_lock);
> +	if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
> +		DRM_DEBUG_KMS("No connectors reported connected with modes\n");
> +
> +	drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
> +
> +	mutex_unlock(&dev->mode_config.mutex);
> +
> +	drm_fb_helper_set_par(fb_helper->fbdev);
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL(drm_fb_helper_initial_config);
>  
>  /**
>   * drm_fb_helper_hotplug_event - respond to a hotplug notification by
> @@ -2436,35 +2499,13 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
>   */
>  int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>  {
> -	struct drm_device *dev = fb_helper->dev;
> -	int err = 0;
> -
> -	if (!drm_fbdev_emulation)
> -		return 0;
> +	int ret;
>  
>  	mutex_lock(&fb_helper->lock);
> -	mutex_lock(&dev->mode_config.mutex);
> -
> -	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
> -		fb_helper->delayed_hotplug = true;
> -		mutex_unlock(&dev->mode_config.mutex);
> -		goto unlock;
> -	}
> -
> -	DRM_DEBUG_KMS("\n");
> -
> -	drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
> -
> -	mutex_unlock(&dev->mode_config.mutex);
> +	ret = __drm_fb_helper_hotplug_event(fb_helper);
>  	mutex_unlock(&fb_helper->lock);

Yeah you can't hold the lock through the entire hotplug_event process,
otherwise the potential register_framebuffer will go boom on a deadlock.
I'll reply to one of the other patches with what I think should be done.
-Daniel

>  
> -	drm_fb_helper_set_par(fb_helper->fbdev);
> -
> -	return 0;
> -
> -unlock:
> -	mutex_unlock(&fb_helper->lock);
> -	return err;
> +	return ret;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>  
> -- 
> 2.12.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 11/11] drm/rockchip: Remove unnecessary NULL check
  2017-03-29 14:44   ` [PATCH v4 11/11] drm/rockchip: " Thierry Reding
@ 2017-06-21 13:39     ` Daniel Vetter
  0 siblings, 0 replies; 19+ messages in thread
From: Daniel Vetter @ 2017-06-21 13:39 UTC (permalink / raw)
  To: Thierry Reding; +Cc: Daniel Vetter, intel-gfx, John Stultz, dri-devel

On Wed, Mar 29, 2017 at 04:44:01PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The expression &private->fbdev_helper can never be NULL, so the check is
> completely unnecessary.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

I just noticed that these two patches at the end of this series don't
depend upon the core rework, so applied them both.
-Daniel

> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index 81f9548672b0..df6bceabeca8 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -168,10 +168,8 @@ rockchip_user_fb_create(struct drm_device *dev, struct drm_file *file_priv,
>  static void rockchip_drm_output_poll_changed(struct drm_device *dev)
>  {
>  	struct rockchip_drm_private *private = dev->dev_private;
> -	struct drm_fb_helper *fb_helper = &private->fbdev_helper;
>  
> -	if (fb_helper)
> -		drm_fb_helper_hotplug_event(fb_helper);
> +	drm_fb_helper_hotplug_event(&private->fbdev_helper);
>  }
>  
>  static void
> -- 
> 2.12.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2017-06-21 13:39 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20170329144416epcas3p471b0fac16a5bb6b51a3abf012c9e9e92@epcas3p4.samsung.com>
2017-03-29 14:43 ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Thierry Reding
2017-03-29 14:43   ` [PATCH v4 01/11] drm/fb-helper: Cleanup checkpatch warnings Thierry Reding
2017-03-29 14:43   ` [PATCH v4 02/11] drm/fb-helper: Reshuffle code for subsequent patches Thierry Reding
2017-03-29 14:43   ` [PATCH v4 03/11] drm/fb-helper: Improve code readability Thierry Reding
2017-03-29 14:43   ` [PATCH v4 04/11] drm/fb-helper: Push down modeset lock into FB helpers Thierry Reding
2017-03-31 18:28     ` Daniel Vetter
2017-03-29 14:43   ` [PATCH v4 05/11] drm/fb-helper: Add top-level lock Thierry Reding
2017-03-29 14:43   ` [PATCH v4 06/11] drm/fb-helper: Make top-level lock more robust Thierry Reding
2017-03-29 14:51     ` Thierry Reding
2017-04-03  8:40     ` [Intel-gfx] " Daniel Vetter
2017-03-29 14:43   ` [PATCH v4 07/11] drm/fb-helper: Support deferred setup Thierry Reding
2017-03-29 14:43   ` [PATCH v4 08/11] drm/exynos: Remove custom FB helper " Thierry Reding
2017-03-29 17:50     ` Daniel Vetter
2017-03-29 14:43   ` [PATCH v4 09/11] drm/hisilicon: " Thierry Reding
2017-03-29 14:44   ` [PATCH v4 10/11] drm/atmel-hlcdc: Remove unnecessary NULL check Thierry Reding
2017-03-29 14:44   ` [PATCH v4 11/11] drm/rockchip: " Thierry Reding
2017-06-21 13:39     ` Daniel Vetter
2017-03-29 15:04   ` ✗ Fi.CI.BAT: warning for drm/fb-helper: Deferred setup support (rev3) Patchwork
2017-03-30 10:21   ` [PATCH v4 00/11] drm/fb-helper: Deferred setup support Andrzej Hajda

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.