[3/3] drm/i915: Disable all planes for load detection, v2.
diff mbox

Message ID 20171220093545.613-4-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst Dec. 20, 2017, 9:35 a.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We don't need any active planes during load detection, so just disable
them all. This saves us from having to come up with a suitable
framebuffer. And we also avoid leaving sprite/cursor planes on and
potentially presenting them at a peculiar location during the load
detection.

Changes since v1 (Maarten):
- Add missing call to add_all_affected_planes.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102707
---
 drivers/gpu/drm/i915/intel_display.c | 147 +++++------------------------------
 1 file changed, 18 insertions(+), 129 deletions(-)

Comments

Daniel Vetter Dec. 20, 2017, 10:28 a.m. UTC | #1
On Wed, Dec 20, 2017 at 10:35:45AM +0100, Maarten Lankhorst wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We don't need any active planes during load detection, so just disable
> them all. This saves us from having to come up with a suitable
> framebuffer. And we also avoid leaving sprite/cursor planes on and
> potentially presenting them at a peculiar location during the load
> detection.
> 
> Changes since v1 (Maarten):
> - Add missing call to add_all_affected_planes.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102707

Less code, I like. And I think we have enough load-detect machines (+ plus
the nasty igt to do it anywhere we still have native vga) to have
reasonable ensurance it actually works.

But maybe let's soak this in -next first for a while, then cherry-pick
over after a few weeks once it's solid.

With the missing Fixes: line added.

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

> ---
>  drivers/gpu/drm/i915/intel_display.c | 147 +++++------------------------------
>  1 file changed, 18 insertions(+), 129 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7e833268c9df..ef61f9a75c93 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9689,111 +9689,27 @@ intel_framebuffer_create(struct drm_i915_gem_object *obj,
>  	return ERR_PTR(ret);
>  }
>  
> -static u32
> -intel_framebuffer_pitch_for_width(int width, int bpp)
> -{
> -	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
> -	return ALIGN(pitch, 64);
> -}
> -
> -static u32
> -intel_framebuffer_size_for_mode(const struct drm_display_mode *mode, int bpp)
> -{
> -	u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp);
> -	return PAGE_ALIGN(pitch * mode->vdisplay);
> -}
> -
> -static struct drm_framebuffer *
> -intel_framebuffer_create_for_mode(struct drm_device *dev,
> -				  const struct drm_display_mode *mode,
> -				  int depth, int bpp)
> -{
> -	struct drm_framebuffer *fb;
> -	struct drm_i915_gem_object *obj;
> -	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> -
> -	obj = i915_gem_object_create(to_i915(dev),
> -				    intel_framebuffer_size_for_mode(mode, bpp));
> -	if (IS_ERR(obj))
> -		return ERR_CAST(obj);
> -
> -	mode_cmd.width = mode->hdisplay;
> -	mode_cmd.height = mode->vdisplay;
> -	mode_cmd.pitches[0] = intel_framebuffer_pitch_for_width(mode_cmd.width,
> -								bpp);
> -	mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
> -
> -	fb = intel_framebuffer_create(obj, &mode_cmd);
> -	if (IS_ERR(fb))
> -		i915_gem_object_put(obj);
> -
> -	return fb;
> -}
> -
> -static struct drm_framebuffer *
> -mode_fits_in_fbdev(struct drm_device *dev,
> -		   const struct drm_display_mode *mode)
> -{
> -#ifdef CONFIG_DRM_FBDEV_EMULATION
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct drm_i915_gem_object *obj;
> -	struct drm_framebuffer *fb;
> -
> -	if (!dev_priv->fbdev)
> -		return NULL;
> -
> -	if (!dev_priv->fbdev->fb)
> -		return NULL;
> -
> -	obj = dev_priv->fbdev->fb->obj;
> -	BUG_ON(!obj);
> -
> -	fb = &dev_priv->fbdev->fb->base;
> -	if (fb->pitches[0] < intel_framebuffer_pitch_for_width(mode->hdisplay,
> -							       fb->format->cpp[0] * 8))
> -		return NULL;
> -
> -	if (obj->base.size < mode->vdisplay * fb->pitches[0])
> -		return NULL;
> -
> -	drm_framebuffer_get(fb);
> -	return fb;
> -#else
> -	return NULL;
> -#endif
> -}
> -
> -static int intel_modeset_setup_plane_state(struct drm_atomic_state *state,
> -					   struct drm_crtc *crtc,
> -					   const struct drm_display_mode *mode,
> -					   struct drm_framebuffer *fb,
> -					   int x, int y)
> +static int intel_modeset_disable_planes(struct drm_atomic_state *state,
> +					struct drm_crtc *crtc)
>  {
> +	struct drm_plane *plane;
>  	struct drm_plane_state *plane_state;
> -	int hdisplay, vdisplay;
> -	int ret;
> -
> -	plane_state = drm_atomic_get_plane_state(state, crtc->primary);
> -	if (IS_ERR(plane_state))
> -		return PTR_ERR(plane_state);
> -
> -	if (mode)
> -		drm_mode_get_hv_timing(mode, &hdisplay, &vdisplay);
> -	else
> -		hdisplay = vdisplay = 0;
> +	int ret, i;
>  
> -	ret = drm_atomic_set_crtc_for_plane(plane_state, fb ? crtc : NULL);
> +	ret = drm_atomic_add_affected_planes(state, crtc);
>  	if (ret)
>  		return ret;
> -	drm_atomic_set_fb_for_plane(plane_state, fb);
> -	plane_state->crtc_x = 0;
> -	plane_state->crtc_y = 0;
> -	plane_state->crtc_w = hdisplay;
> -	plane_state->crtc_h = vdisplay;
> -	plane_state->src_x = x << 16;
> -	plane_state->src_y = y << 16;
> -	plane_state->src_w = hdisplay << 16;
> -	plane_state->src_h = vdisplay << 16;
> +
> +	for_each_new_plane_in_state(state, plane, plane_state, i) {
> +		if (plane_state->crtc != crtc)
> +			continue;
> +
> +		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> +		if (ret)
> +			return ret;
> +
> +		drm_atomic_set_fb_for_plane(plane_state, NULL);
> +	}
>  
>  	return 0;
>  }
> @@ -9811,7 +9727,6 @@ int intel_get_load_detect_pipe(struct drm_connector *connector,
>  	struct drm_crtc *crtc = NULL;
>  	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct drm_framebuffer *fb;
>  	struct drm_mode_config *config = &dev->mode_config;
>  	struct drm_atomic_state *state = NULL, *restore_state = NULL;
>  	struct drm_connector_state *connector_state;
> @@ -9879,10 +9794,6 @@ int intel_get_load_detect_pipe(struct drm_connector *connector,
>  found:
>  	intel_crtc = to_intel_crtc(crtc);
>  
> -	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
> -	if (ret)
> -		goto fail;
> -
>  	state = drm_atomic_state_alloc(dev);
>  	restore_state = drm_atomic_state_alloc(dev);
>  	if (!state || !restore_state) {
> @@ -9914,39 +9825,17 @@ int intel_get_load_detect_pipe(struct drm_connector *connector,
>  	if (!mode)
>  		mode = &load_detect_mode;
>  
> -	/* We need a framebuffer large enough to accommodate all accesses
> -	 * that the plane may generate whilst we perform load detection.
> -	 * We can not rely on the fbcon either being present (we get called
> -	 * during its initialisation to detect all boot displays, or it may
> -	 * not even exist) or that it is large enough to satisfy the
> -	 * requested mode.
> -	 */
> -	fb = mode_fits_in_fbdev(dev, mode);
> -	if (fb == NULL) {
> -		DRM_DEBUG_KMS("creating tmp fb for load-detection\n");
> -		fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32);
> -	} else
> -		DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
> -	if (IS_ERR(fb)) {
> -		DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n");
> -		ret = PTR_ERR(fb);
> -		goto fail;
> -	}
> -
> -	ret = intel_modeset_setup_plane_state(state, crtc, mode, fb, 0, 0);
> -	drm_framebuffer_put(fb);
> +	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
>  	if (ret)
>  		goto fail;
>  
> -	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
> +	ret = intel_modeset_disable_planes(state, crtc);
>  	if (ret)
>  		goto fail;
>  
>  	ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(restore_state, connector));
>  	if (!ret)
>  		ret = PTR_ERR_OR_ZERO(drm_atomic_get_crtc_state(restore_state, crtc));
> -	if (!ret)
> -		ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(restore_state, crtc->primary));
>  	if (ret) {
>  		DRM_DEBUG_KMS("Failed to create a copy of old state to restore: %i\n", ret);
>  		goto fail;
> -- 
> 2.15.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Maarten Lankhorst Dec. 21, 2017, 9:58 a.m. UTC | #2
Op 20-12-17 om 11:28 schreef Daniel Vetter:
> On Wed, Dec 20, 2017 at 10:35:45AM +0100, Maarten Lankhorst wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> We don't need any active planes during load detection, so just disable
>> them all. This saves us from having to come up with a suitable
>> framebuffer. And we also avoid leaving sprite/cursor planes on and
>> potentially presenting them at a peculiar location during the load
>> detection.
>>
>> Changes since v1 (Maarten):
>> - Add missing call to add_all_affected_planes.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102707
> Less code, I like. And I think we have enough load-detect machines (+ plus
> the nasty igt to do it anywhere we still have native vga) to have
> reasonable ensurance it actually works.
>
> But maybe let's soak this in -next first for a while, then cherry-pick
> over after a few weeks once it's solid.
>
> With the missing Fixes: line added.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Pushed, didn't add fixes since it's an ancient bug, for a piece of code that changed a lot.
Since in most cases the fbcon fb will be leaked, the impact is just a warning and not even
a real leak. :)

Thanks for review,

~Maarten

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7e833268c9df..ef61f9a75c93 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9689,111 +9689,27 @@  intel_framebuffer_create(struct drm_i915_gem_object *obj,
 	return ERR_PTR(ret);
 }
 
-static u32
-intel_framebuffer_pitch_for_width(int width, int bpp)
-{
-	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
-	return ALIGN(pitch, 64);
-}
-
-static u32
-intel_framebuffer_size_for_mode(const struct drm_display_mode *mode, int bpp)
-{
-	u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp);
-	return PAGE_ALIGN(pitch * mode->vdisplay);
-}
-
-static struct drm_framebuffer *
-intel_framebuffer_create_for_mode(struct drm_device *dev,
-				  const struct drm_display_mode *mode,
-				  int depth, int bpp)
-{
-	struct drm_framebuffer *fb;
-	struct drm_i915_gem_object *obj;
-	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
-
-	obj = i915_gem_object_create(to_i915(dev),
-				    intel_framebuffer_size_for_mode(mode, bpp));
-	if (IS_ERR(obj))
-		return ERR_CAST(obj);
-
-	mode_cmd.width = mode->hdisplay;
-	mode_cmd.height = mode->vdisplay;
-	mode_cmd.pitches[0] = intel_framebuffer_pitch_for_width(mode_cmd.width,
-								bpp);
-	mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
-
-	fb = intel_framebuffer_create(obj, &mode_cmd);
-	if (IS_ERR(fb))
-		i915_gem_object_put(obj);
-
-	return fb;
-}
-
-static struct drm_framebuffer *
-mode_fits_in_fbdev(struct drm_device *dev,
-		   const struct drm_display_mode *mode)
-{
-#ifdef CONFIG_DRM_FBDEV_EMULATION
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct drm_i915_gem_object *obj;
-	struct drm_framebuffer *fb;
-
-	if (!dev_priv->fbdev)
-		return NULL;
-
-	if (!dev_priv->fbdev->fb)
-		return NULL;
-
-	obj = dev_priv->fbdev->fb->obj;
-	BUG_ON(!obj);
-
-	fb = &dev_priv->fbdev->fb->base;
-	if (fb->pitches[0] < intel_framebuffer_pitch_for_width(mode->hdisplay,
-							       fb->format->cpp[0] * 8))
-		return NULL;
-
-	if (obj->base.size < mode->vdisplay * fb->pitches[0])
-		return NULL;
-
-	drm_framebuffer_get(fb);
-	return fb;
-#else
-	return NULL;
-#endif
-}
-
-static int intel_modeset_setup_plane_state(struct drm_atomic_state *state,
-					   struct drm_crtc *crtc,
-					   const struct drm_display_mode *mode,
-					   struct drm_framebuffer *fb,
-					   int x, int y)
+static int intel_modeset_disable_planes(struct drm_atomic_state *state,
+					struct drm_crtc *crtc)
 {
+	struct drm_plane *plane;
 	struct drm_plane_state *plane_state;
-	int hdisplay, vdisplay;
-	int ret;
-
-	plane_state = drm_atomic_get_plane_state(state, crtc->primary);
-	if (IS_ERR(plane_state))
-		return PTR_ERR(plane_state);
-
-	if (mode)
-		drm_mode_get_hv_timing(mode, &hdisplay, &vdisplay);
-	else
-		hdisplay = vdisplay = 0;
+	int ret, i;
 
-	ret = drm_atomic_set_crtc_for_plane(plane_state, fb ? crtc : NULL);
+	ret = drm_atomic_add_affected_planes(state, crtc);
 	if (ret)
 		return ret;
-	drm_atomic_set_fb_for_plane(plane_state, fb);
-	plane_state->crtc_x = 0;
-	plane_state->crtc_y = 0;
-	plane_state->crtc_w = hdisplay;
-	plane_state->crtc_h = vdisplay;
-	plane_state->src_x = x << 16;
-	plane_state->src_y = y << 16;
-	plane_state->src_w = hdisplay << 16;
-	plane_state->src_h = vdisplay << 16;
+
+	for_each_new_plane_in_state(state, plane, plane_state, i) {
+		if (plane_state->crtc != crtc)
+			continue;
+
+		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
+		if (ret)
+			return ret;
+
+		drm_atomic_set_fb_for_plane(plane_state, NULL);
+	}
 
 	return 0;
 }
@@ -9811,7 +9727,6 @@  int intel_get_load_detect_pipe(struct drm_connector *connector,
 	struct drm_crtc *crtc = NULL;
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct drm_framebuffer *fb;
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_atomic_state *state = NULL, *restore_state = NULL;
 	struct drm_connector_state *connector_state;
@@ -9879,10 +9794,6 @@  int intel_get_load_detect_pipe(struct drm_connector *connector,
 found:
 	intel_crtc = to_intel_crtc(crtc);
 
-	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
-	if (ret)
-		goto fail;
-
 	state = drm_atomic_state_alloc(dev);
 	restore_state = drm_atomic_state_alloc(dev);
 	if (!state || !restore_state) {
@@ -9914,39 +9825,17 @@  int intel_get_load_detect_pipe(struct drm_connector *connector,
 	if (!mode)
 		mode = &load_detect_mode;
 
-	/* We need a framebuffer large enough to accommodate all accesses
-	 * that the plane may generate whilst we perform load detection.
-	 * We can not rely on the fbcon either being present (we get called
-	 * during its initialisation to detect all boot displays, or it may
-	 * not even exist) or that it is large enough to satisfy the
-	 * requested mode.
-	 */
-	fb = mode_fits_in_fbdev(dev, mode);
-	if (fb == NULL) {
-		DRM_DEBUG_KMS("creating tmp fb for load-detection\n");
-		fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32);
-	} else
-		DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
-	if (IS_ERR(fb)) {
-		DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n");
-		ret = PTR_ERR(fb);
-		goto fail;
-	}
-
-	ret = intel_modeset_setup_plane_state(state, crtc, mode, fb, 0, 0);
-	drm_framebuffer_put(fb);
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
 	if (ret)
 		goto fail;
 
-	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
+	ret = intel_modeset_disable_planes(state, crtc);
 	if (ret)
 		goto fail;
 
 	ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(restore_state, connector));
 	if (!ret)
 		ret = PTR_ERR_OR_ZERO(drm_atomic_get_crtc_state(restore_state, crtc));
-	if (!ret)
-		ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(restore_state, crtc->primary));
 	if (ret) {
 		DRM_DEBUG_KMS("Failed to create a copy of old state to restore: %i\n", ret);
 		goto fail;