[PATCH 1/3] drm/i915: Unconditionally check gmch pfit state

[PATCH 1/3] drm/i915: Unconditionally check gmch pfit state

[Daniel Vetter]

Now that we recompute the pipe config for all CRTCs that have changed
we don't have problems with stale configuration data for the global
pfit and can remove this hack. Yay!

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ad0fc6a9d3c6..e14d62693d76 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12612,21 +12612,11 @@ intel_pipe_config_compare(struct drm_device *dev,
 	PIPE_CONF_CHECK_I(pipe_src_w);
 	PIPE_CONF_CHECK_I(pipe_src_h);
 
-	/*
-	 * FIXME: BIOS likes to set up a cloned config with lvds+external
-	 * screen. Since we don't yet re-compute the pipe config when moving
-	 * just the lvds port away to another pipe the sw tracking won't match.
-	 *
-	 * Proper atomic modesets with recomputed global state will fix this.
-	 * Until then just don't check gmch state for inherited modes.
-	 */
-	if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) {
-		PIPE_CONF_CHECK_I(gmch_pfit.control);
-		/* pfit ratios are autocomputed by the hw on gen4+ */
-		if (INTEL_INFO(dev)->gen < 4)
-			PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios);
-		PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits);
-	}
+	PIPE_CONF_CHECK_I(gmch_pfit.control);
+	/* pfit ratios are autocomputed by the hw on gen4+ */
+	if (INTEL_INFO(dev)->gen < 4)
+		PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios);
+	PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits);
 
 	PIPE_CONF_CHECK_I(pch_pfit.enabled);
 	if (current_config->pch_pfit.enabled) {
-- 
2.1.4

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

[PATCH 2/3] drm/i915: Clarify logic for initial modeset

[Daniel Vetter]

Currently we both set mode->private_flags to some value and also use
the pipe_config quirk. But since the pipe_config quirk isn't tied to
the lifetime of the mode object we need to check both.

Simplify this by only using mode.private_flags and stop using the
INHERITED_MODE quirk. Also for clarity add an explicit #define for
that driver priavete mode flag.

By using crtc_state->mode_changed we can also remove the recalc local
variable.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 34 ++++++++++++----------------------
 drivers/gpu/drm/i915/intel_drv.h     |  4 +++-
 2 files changed, 15 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e14d62693d76..576add2697c1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13162,7 +13162,11 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc_state);
-		bool modeset, recalc = false;
+		bool modeset;
+
+		/* Catch I915_MODE_FLAG_INHERITED */
+		if (crtc_state->mode.private_flags != crtc->state->mode.private_flags)
+			crtc_state->mode_changed = true;
 
 		if (!crtc_state->enable) {
 			if (needs_modeset(crtc_state))
@@ -13171,28 +13175,22 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 		}
 
 		modeset = needs_modeset(crtc_state);
-		/* see comment in intel_modeset_readout_hw_state */
-		if (!modeset && crtc_state->mode_blob != crtc->state->mode_blob &&
-		    pipe_config->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE)
-			recalc = true;
 
-		if (!modeset && !recalc)
+		if (!modeset)
 			continue;
 
-		if (recalc) {
-			ret = drm_atomic_add_affected_connectors(state, crtc);
-			if (ret)
-				return ret;
-		}
+		ret = drm_atomic_add_affected_connectors(state, crtc);
+		if (ret)
+			return ret;
 
 		ret = intel_modeset_pipe_config(crtc, pipe_config);
 		if (ret)
 			return ret;
 
-		if (recalc && (!i915.fastboot ||
+		if (!i915.fastboot ||
 		    !intel_pipe_config_compare(state->dev,
 					to_intel_crtc_state(crtc->state),
-					pipe_config, true))) {
+					pipe_config, true)) {
 			modeset = crtc_state->mode_changed = true;
 
 			ret = drm_atomic_add_affected_planes(state, crtc);
@@ -15437,8 +15435,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 		memset(crtc->config, 0, sizeof(*crtc->config));
 		crtc->config->base.crtc = &crtc->base;
 
-		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
-
 		crtc->active = dev_priv->display.get_pipe_config(crtc,
 								 crtc->config);
 
@@ -15464,17 +15460,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 			 * right now it would cause a double modeset if
 			 * fbdev or userspace chooses a different initial mode.
 			 *
-			 * So to prevent the double modeset, fail the memcmp
-			 * test in drm_atomic_set_mode_for_crtc to get a new
-			 * mode blob, and compare if the mode blob changed
-			 * when the PIPE_CONFIG_QUIRK_INHERITED_MODE quirk is
-			 * set.
-			 *
 			 * If that happens, someone indicated they wanted a
 			 * mode change, which means it's safe to do a full
 			 * recalculation.
 			 */
-			crtc->base.state->mode.private_flags = ~0;
+			crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED;
 		}
 
 		crtc->base.hwmode = crtc->config->base.adjusted_mode;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ced590ba79b4..661967611778 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -317,6 +317,9 @@ struct intel_crtc_scaler_state {
 	int scaler_id;
 };
 
+/* drm_mode->private_flags */
+#define I915_MODE_FLAG_INHERITED 1
+
 struct intel_crtc_state {
 	struct drm_crtc_state base;
 
@@ -329,7 +332,6 @@ struct intel_crtc_state {
 	 * accordingly.
 	 */
 #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync mode.flags */
-#define PIPE_CONFIG_QUIRK_INHERITED_MODE	(1<<1) /* mode inherited from firmware */
 	unsigned long quirks;
 
 	/* Pipe source size (ie. panel fitter input size)
-- 
2.1.4

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

[PATCH 3/3] drm/i915: Invert fastboot check

[Daniel Vetter]

Fastboot should only downgrade a modeset if we have a match, not be
used to upgrade to a full modeset. Otherwise we can only use it in a
very restricted way: Initial modeset when the request mode is the
preferred one of the panel and there's still a pfit active. And that
only works because our mode_from_pipe_config fills in the wrong mode
(it takes the adjusted mode, not the requested one).

But we want fast modesets everywhere even after boot-up (especially
for testing, but not only there). Hence we need to be able to make any
modeset a fast one, which means we need to invert the logic and
optionally downgrade a modeset.

Note that this needs ->connector_changed split out from ->mode_changed
otherwise it's not going to work (because we might loose a modeset
because connectors changed but otherwise the config matches). As soon
as that's merged we can drop the i915.fastboot check from this code.

Also make sure that we don't accidentally clear any_ms and that we add
the planes for any kind of modeset.

Finally rename fastboot to fastset (yeah it's a silly name) since this
really isn't about booting all that much.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 576add2697c1..4c974cc84fbc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13162,7 +13162,6 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc_state);
-		bool modeset;
 
 		/* Catch I915_MODE_FLAG_INHERITED */
 		if (crtc_state->mode.private_flags != crtc->state->mode.private_flags)
@@ -13174,11 +13173,12 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 			continue;
 		}
 
-		modeset = needs_modeset(crtc_state);
-
-		if (!modeset)
+		if (!needs_modeset(crtc_state))
 			continue;
 
+		/* FIXME: For only active_changed we shouldn't need to do any
+		 * state recomputation at all. */
+
 		ret = drm_atomic_add_affected_connectors(state, crtc);
 		if (ret)
 			return ret;
@@ -13187,21 +13187,24 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 		if (ret)
 			return ret;
 
-		if (!i915.fastboot ||
-		    !intel_pipe_config_compare(state->dev,
+		if (i915.fastboot &&
+		    intel_pipe_config_compare(state->dev,
 					to_intel_crtc_state(crtc->state),
 					pipe_config, true)) {
-			modeset = crtc_state->mode_changed = true;
+			crtc_state->mode_changed = false;
+		}
+
+		if (needs_modeset(crtc_state)) {
+			any_ms = true;
 
 			ret = drm_atomic_add_affected_planes(state, crtc);
 			if (ret)
 				return ret;
 		}
 
-		any_ms = modeset;
-		intel_dump_pipe_config(to_intel_crtc(crtc),
-				       pipe_config,
-				       modeset ? "[modeset]" : "[fastboot]");
+		intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
+				       needs_modeset(crtc_state) ?
+				       "[modeset]" : "[fastset]");
 	}
 
 	if (any_ms) {
-- 
2.1.4

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

[PATCH 4/3] drm/i915: Clarify logic for initial modeset

[Maarten Lankhorst]

Hey,

Op 15-07-15 om 14:15 schreef Daniel Vetter:
> Currently we both set mode->private_flags to some value and also use
> the pipe_config quirk. But since the pipe_config quirk isn't tied to
> the lifetime of the mode object we need to check both.
>
> Simplify this by only using mode.private_flags and stop using the
> INHERITED_MODE quirk. Also for clarity add an explicit #define for
> that driver priavete mode flag.
>
> By using crtc_state->mode_changed we can also remove the recalc local
> variable.
Those 3 changes look good to me.
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Any objections against this followup patch?
Or should I wait until I made intel_update_pipe_size atomic?

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 7eff33ff84f6..914679ceb200 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -578,9 +578,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
 	struct intel_crtc *intel_crtc;
 	unsigned int max_size = 0;
 
-	if (!i915.fastboot)
-		return false;
-
 	/* Find the largest fb */
 	for_each_crtc(dev, crtc) {
 		struct drm_i915_gem_object *obj =

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

Re: [PATCH 4/3] drm/i915: Clarify logic for initial modeset

[Daniel Vetter]

On Wed, Jul 15, 2015 at 03:20:34PM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> Op 15-07-15 om 14:15 schreef Daniel Vetter:
> > Currently we both set mode->private_flags to some value and also use
> > the pipe_config quirk. But since the pipe_config quirk isn't tied to
> > the lifetime of the mode object we need to check both.
> >
> > Simplify this by only using mode.private_flags and stop using the
> > INHERITED_MODE quirk. Also for clarity add an explicit #define for
> > that driver priavete mode flag.
> >
> > By using crtc_state->mode_changed we can also remove the recalc local
> > variable.
> Those 3 changes look good to me.
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> Any objections against this followup patch?
> Or should I wait until I made intel_update_pipe_size atomic?
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 7eff33ff84f6..914679ceb200 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -578,9 +578,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
>  	struct intel_crtc *intel_crtc;
>  	unsigned int max_size = 0;
>  
> -	if (!i915.fastboot)
> -		return false;

I tried to digg out from history why we added that check, but never found
it. Original commit says that Jesse added this on his own, but doesn't
explain the reason for that. Reading through the code there doesn't seem
to be anything in there that needs other bits of fastboot (or ever needed
other bits of fastboot). The only effect I can see is that we'll take over
the bpp setting from the firmware, and previously that resulted in
modesets since we used the primary bpp to pick the pipe bpp. But that's
fixed now so no risk for spurious modesets when doing fbcon<->X vt
switches.

Can you please paste the above explanation into a commit message and send
it out with sob and all? I'll pick it up.

Cheers, Daniel
> -
>  	/* Find the largest fb */
>  	for_each_crtc(dev, crtc) {
>  		struct drm_i915_gem_object *obj =
> 

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