Re: [PATCH v3 08/22] drm/i915: Zap call to drm_plane_helper_disable.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 28-05-15 om 03:37 schreef Matt Roper:
> On Wed, May 20, 2015 at 03:38:13PM +0200, Maarten Lankhorst wrote:
>> The primary plane can still be configured when crtc is off,
>> furthermore this is also a noop now that affected planes are
>> added on modesets.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> This was added in patch #2.  Can we squash this change up, or was there
> a reason we needed it for the intermediate patches?
>
> Actually, I'm not sure this is really quite a noop yet.
> prepare_plane_fb is never called on the primary plane as far as I can
> see, so I think our frontbuffer tracking and such might get confused.
> In the regular plane update codepath, we have to handle this with a
> special case by doing:
>
>         intel_crtc->atomic.disabled_planes |= (1 << drm_plane_index(plane));
>
> in the intel_plane_atomic_check function.  But from what I can see, we
> bypass that in this codepath.  igt/kms_universal_plane shows that we do
> run into frontbuffer tracking warnings if we don't put an equivalent
> update to the 'disabled_planes' flag here.

Well if this is the case we're really messing up on adding planes.. If 
we're fine with leaving the warning for now I'm fixing it up in series 
3.

Do the below patches fix it?

commit 05fac5b2c29a85492c677e42ec68e94f9011daa2
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Tue May 26 13:00:08 2015 +0200

    drm/i915: update plane state during init
    
    Atomic planes updates rely on having a accurate plane_mask.
    
    Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 558fe51f151e..692cf9092f6a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2645,9 +2645,9 @@ valid_fb:
 		dev_priv->preserve_bios_swizzle = true;
 
 	primary->fb = fb;
-	primary->state->crtc = &intel_crtc->base;
-	primary->crtc = &intel_crtc->base;
+	primary->crtc = primary->state->crtc = &intel_crtc->base;
 	update_state_fb(primary);
+	intel_crtc->base.state->plane_mask |= (1 << drm_plane_index(primary));
 	obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
 }
 
@@ -14959,7 +14959,9 @@ void intel_modeset_gem_init(struct drm_device *dev)
 				  to_intel_crtc(c)->pipe);
 			drm_framebuffer_unreference(c->primary->fb);
 			c->primary->fb = NULL;
+			c->primary->crtc = c->primary->state->crtc = NULL;
 			update_state_fb(c->primary);
+			c->state->plane_mask &= ~(1 << drm_plane_index(c->primary));
 		}
 	}
 


commit b2caffde98d9f7335e774d1859b881b0df8a109a
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Mon May 25 10:25:45 2015 +0200

    drm/i915: Make sure all planes and connectors are added on modeset.
    
    Add missing calls to drm_atomic_add_affected_*. This is needed
    to convert to atomic planes. When converting to atomic all planes
    are needed on modeset. For good measure make sure all connectors
    are added too.
    
    Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5c1e74d1fb1b..558fe51f151e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5628,7 +5628,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	int max_pixclk = intel_mode_max_pixclk(state->dev, state);
-	int cdclk, i;
+	int cdclk, ret = 0;
 
 	if (max_pixclk < 0)
 		return max_pixclk;
@@ -5643,20 +5643,25 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
 
 	/* add all active pipes to the state */
 	for_each_crtc(state->dev, crtc) {
-		if (!crtc->state->active)
-			continue;
-
 		crtc_state = drm_atomic_get_crtc_state(state, crtc);
 		if (IS_ERR(crtc_state))
 			return PTR_ERR(crtc_state);
-	}
 
-	/* disable/enable all currently active pipes while we change cdclk */
-	for_each_crtc_in_state(state, crtc, crtc_state, i)
-		if (crtc_state->active)
-			crtc_state->mode_changed = true;
+		if (!crtc_state->active || needs_modeset(crtc_state))
+			continue;
 
-	return 0;
+		crtc_state->mode_changed = true;
+
+		ret = drm_atomic_add_affected_connectors(state, crtc);
+		if (ret)
+			break;
+
+		ret = drm_atomic_add_affected_planes(state, crtc);
+		if (ret)
+			break;
+	}
+
+	return ret;
 }
 
 static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
@@ -11483,8 +11488,10 @@ encoder_retry:
 
 	/* Check if we need to force a modeset */
 	if (pipe_config->has_audio !=
-	    to_intel_crtc_state(crtc->state)->has_audio)
+	    to_intel_crtc_state(crtc->state)->has_audio) {
 		pipe_config->base.mode_changed = true;
+		ret = drm_atomic_add_affected_planes(state, crtc);
+	}
 
 	/*
 	 * Note we have an issue here with infoframes: current code
@@ -11492,8 +11499,6 @@ encoder_retry:
 	 * requirements.  So here we should be checking for any
 	 * required changes and forcing a mode set.
 	 */
-
-	return 0;
 fail:
 	return ret;
 }

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