[PATCH 1/5] drm/i915: preserve SSC if previously set v2

[PATCH 1/5] drm/i915: preserve SSC if previously set v2

[Jesse Barnes]

Some machines may have a broken VBT or no VBT at all, but we still want
to use SSC there.  So check for it and keep it enabled if we see it
already on.  Based on an earlier fix from Kristian.

v2: honor modparam if set too (Daniel)
    read out at init time and store for panel_use_ssc() use (Jesse)

Reported-by: Kristian Høgsberg <hoegsberg@gmail.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 ++
 drivers/gpu/drm/i915/intel_display.c | 11 ++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8631fb3..f57b752 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1404,6 +1404,8 @@ struct drm_i915_private {
 	struct intel_opregion opregion;
 	struct intel_vbt_data vbt;
 
+	bool bios_ssc; /* BIOS had SSC enabled at boot? */
+
 	/* overlay */
 	struct intel_overlay *overlay;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5cbb28..0e8c9bc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5355,7 +5355,7 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
 {
 	if (i915.panel_use_ssc >= 0)
 		return i915.panel_use_ssc != 0;
-	return dev_priv->vbt.lvds_use_ssc
+	return (dev_priv->vbt.lvds_use_ssc || dev_priv->bios_ssc)
 		&& !(dev_priv->quirks & QUIRK_LVDS_SSC_DISABLE);
 }
 
@@ -12478,6 +12478,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 
 void intel_modeset_gem_init(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *c;
 	struct intel_framebuffer *fb;
 
@@ -12485,6 +12486,14 @@ void intel_modeset_gem_init(struct drm_device *dev)
 	intel_init_gt_powersave(dev);
 	mutex_unlock(&dev->struct_mutex);
 
+	/*
+	 * There may be no VBT; and if the BIOS enabled SSC we can
+	 * just keep using it to avoid unnecessary flicker.
+	 */
+	if ((HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) &&
+	    (I915_READ(PCH_DREF_CONTROL) & DREF_SSC1_ENABLE))
+			dev_priv->bios_ssc = true;
+
 	intel_modeset_init_hw(dev);
 
 	intel_setup_overlay(dev);
-- 
1.8.3.2

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

[PATCH 2/5] drm/i915: preserve swizzle settings if necessary v3

[Jesse Barnes]

Some machines (like MBAs) might use a tiled framebuffer but not enable
display swizzling at boot time.  We want to preserve that configuration
if possible to prevent a boot time mode set.  On IVB+ it shouldn't
affect performance anyway since the memory controller does internal
swizzling anyway.

For most other configs we'll be able to enable swizzling at boot time,
since the initial framebuffer won't be tiled, thus we won't see any
corruption when we enable it.

v2: preserve swizzling if BIOS had it set (Daniel)
v3: preserve swizzling only if we inherited a tiled framebuffer (Daniel)
    check display swizzle setting in detect_bit_6_swizzle (Daniel)
    use gen6 as cutoff point (Daniel)

Reported-by: Kristian Høgsberg <hoegsberg@gmail.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.h        |  1 +
 drivers/gpu/drm/i915/i915_gem.c        |  3 +++
 drivers/gpu/drm/i915/i915_gem_tiling.c | 38 +++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_display.c   |  3 +++
 4 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f57b752..f49fdcb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1405,6 +1405,7 @@ struct drm_i915_private {
 	struct intel_vbt_data vbt;
 
 	bool bios_ssc; /* BIOS had SSC enabled at boot? */
+	bool preserve_bios_swizzle;
 
 	/* overlay */
 	struct intel_overlay *overlay;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index bfc7af0..0b168fb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4580,6 +4580,9 @@ void i915_gem_init_swizzling(struct drm_device *dev)
 	    dev_priv->mm.bit_6_swizzle_x == I915_BIT_6_SWIZZLE_NONE)
 		return;
 
+	if (INTEL_INFO(dev)->gen >= 6 && dev_priv->preserve_bios_swizzle)
+		return;
+
 	I915_WRITE(DISP_ARB_CTL, I915_READ(DISP_ARB_CTL) |
 				 DISP_TILE_SURFACE_SWIZZLING);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index cb150e8..73005ad 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -91,26 +91,30 @@ i915_gem_detect_bit_6_swizzle(struct drm_device *dev)
 	uint32_t swizzle_x = I915_BIT_6_SWIZZLE_UNKNOWN;
 	uint32_t swizzle_y = I915_BIT_6_SWIZZLE_UNKNOWN;
 
-	if (IS_VALLEYVIEW(dev)) {
-		swizzle_x = I915_BIT_6_SWIZZLE_NONE;
-		swizzle_y = I915_BIT_6_SWIZZLE_NONE;
-	} else if (INTEL_INFO(dev)->gen >= 6) {
+	if (INTEL_INFO(dev)->gen >= 6) {
 		uint32_t dimm_c0, dimm_c1;
-		dimm_c0 = I915_READ(MAD_DIMM_C0);
-		dimm_c1 = I915_READ(MAD_DIMM_C1);
-		dimm_c0 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK;
-		dimm_c1 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK;
-		/* Enable swizzling when the channels are populated with
-		 * identically sized dimms. We don't need to check the 3rd
-		 * channel because no cpu with gpu attached ships in that
-		 * configuration. Also, swizzling only makes sense for 2
-		 * channels anyway. */
-		if (dimm_c0 == dimm_c1) {
-			swizzle_x = I915_BIT_6_SWIZZLE_9_10;
-			swizzle_y = I915_BIT_6_SWIZZLE_9;
-		} else {
+
+		/* Make sure to honor boot time display configuration */
+		if (!(I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING)) {
 			swizzle_x = I915_BIT_6_SWIZZLE_NONE;
 			swizzle_y = I915_BIT_6_SWIZZLE_NONE;
+		} else {
+			dimm_c0 = I915_READ(MAD_DIMM_C0);
+			dimm_c1 = I915_READ(MAD_DIMM_C1);
+			dimm_c0 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK;
+			dimm_c1 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK;
+			/* Enable swizzling when the channels are populated with
+			 * identically sized dimms. We don't need to check the
+			 * 3rd channel because no cpu with gpu attached ships
+			 * in that configuration. Also, swizzling only makes
+			 * sense for 2 channels anyway. */
+			if (dimm_c0 == dimm_c1) {
+				swizzle_x = I915_BIT_6_SWIZZLE_9_10;
+				swizzle_y = I915_BIT_6_SWIZZLE_9;
+			} else {
+				swizzle_x = I915_BIT_6_SWIZZLE_NONE;
+				swizzle_y = I915_BIT_6_SWIZZLE_NONE;
+			}
 		}
 	} else if (IS_GEN5(dev)) {
 		/* On Ironlake whatever DRAM config, GPU always do
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0e8c9bc..7dbbef7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2361,6 +2361,7 @@ static void intel_find_plane_obj(struct intel_crtc *intel_crtc,
 				 struct intel_plane_config *plane_config)
 {
 	struct drm_device *dev = intel_crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *c;
 	struct intel_crtc *i;
 	struct intel_framebuffer *fb;
@@ -2389,6 +2390,8 @@ static void intel_find_plane_obj(struct intel_crtc *intel_crtc,
 
 		fb = to_intel_framebuffer(c->primary->fb);
 		if (i915_gem_obj_ggtt_offset(fb->obj) == plane_config->base) {
+			if (fb->obj->tiling_mode != I915_TILING_NONE)
+				dev_priv->preserve_bios_swizzle = true;
 			drm_framebuffer_reference(c->primary->fb);
 			intel_crtc->base.primary->fb = c->primary->fb;
 			break;
-- 
1.8.3.2

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

[PATCH 3/5] drm: add drm_mode_same_size function

[Jesse Barnes]

From: Kristian Høgsberg <hoegsberg@gmail.com>

Like mode_equal but w/o the clock checks.  Useful for checking if modes
are close enough to re-use to avoid a boot time mode set for example.

Signed-off-by: Kristian Høgsberg <hoegsberg@gmail.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/drm_modes.c | 8 ++++++++
 include/drm/drm_modes.h     | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index bedf189..92ae7f3 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -905,6 +905,14 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
 }
 EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
 
+bool drm_mode_same_size(const struct drm_display_mode *mode1,
+			const struct drm_display_mode *mode2)
+{
+	return mode1->vdisplay == mode2->vdisplay &&
+		mode1->hdisplay == mode2->hdisplay;
+}
+EXPORT_SYMBOL(drm_mode_same_size);
+
 /**
  * drm_mode_validate_size - make sure modes adhere to size constraints
  * @dev: DRM device
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 91d0582..7272309 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -215,6 +215,8 @@ bool drm_mode_equal(const struct drm_display_mode *mode1,
 		    const struct drm_display_mode *mode2);
 bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
 					const struct drm_display_mode *mode2);
+bool drm_mode_same_size(const struct drm_display_mode *mode1,
+			const struct drm_display_mode *mode2);
 
 /* for use by the crtc helper probe functions */
 void drm_mode_validate_size(struct drm_device *dev,
-- 
1.8.3.2

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

[PATCH 4/5] drm/i915: use current mode if the size matches the preferred mode

[Jesse Barnes]

From: Kristian Høgsberg <hoegsberg@gmail.com>

The BIOS may set a native mode that doesn't quite match the preferred
mode timings.  It should be ok to use however if it uses the same size,
so try to avoid a mode set in that case.

Signed-off-by: Kristian Høgsberg <hoegsberg@gmail.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 47 +++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 088fe93..09819ae 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -379,42 +379,31 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 		/* go for command line mode first */
 		modes[i] = drm_pick_cmdline_mode(fb_conn, width, height);
 
-		/* try for preferred next */
+		/* try for preferred next or match current */
 		if (!modes[i]) {
-			DRM_DEBUG_KMS("looking for preferred mode on connector %s\n",
-				      connector->name);
-			modes[i] = drm_has_preferred_mode(fb_conn, width,
-							  height);
-		}
+			struct drm_display_mode *preferred;
 
-		/* No preferred mode marked by the EDID? Are there any modes? */
-		if (!modes[i] && !list_empty(&connector->modes)) {
-			DRM_DEBUG_KMS("using first mode listed on connector %s\n",
+			DRM_DEBUG_KMS("looking for preferred mode on connector %s\n",
 				      connector->name);
-			modes[i] = list_first_entry(&connector->modes,
-						    struct drm_display_mode,
-						    head);
-		}
+			preferred = drm_has_preferred_mode(fb_conn, width,
+							   height);
 
-		/* last resort: use current mode */
-		if (!modes[i]) {
-			/*
-			 * IMPORTANT: We want to use the adjusted mode (i.e.
-			 * after the panel fitter upscaling) as the initial
-			 * config, not the input mode, which is what crtc->mode
-			 * usually contains. But since our current fastboot
-			 * code puts a mode derived from the post-pfit timings
-			 * into crtc->mode this works out correctly. We don't
-			 * use hwmode anywhere right now, so use it for this
-			 * since the fb helper layer wants a pointer to
-			 * something we own.
-			 */
-			DRM_DEBUG_KMS("looking for current mode on connector %s\n",
-				      connector->name);
 			intel_mode_from_pipe_config(&encoder->crtc->hwmode,
 						    &to_intel_crtc(encoder->crtc)->config);
-			modes[i] = &encoder->crtc->hwmode;
+ 			modes[i] = &encoder->crtc->hwmode;
+
+			if (preferred &&
+			    !drm_mode_same_size(preferred, modes[i])) {
+				DRM_DEBUG_KMS("using preferred mode %s "
+					      "instead of current mode %s "
+					      "on connector %d\n",
+					      preferred->name,
+					      modes[i]->name,
+					      fb_conn->connector->base.id);
+				modes[i] = preferred;
+			}
 		}
+
 		crtcs[i] = new_crtc;
 
 		DRM_DEBUG_KMS("connector %s on pipe %d [CRTC:%d]: %dx%d%s\n",
-- 
1.8.3.2

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

[PATCH 5/5] drm/i915: enable fastboot by default

[Jesse Barnes]

Let them eat mincemeat pie.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_params.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index d05a2af..081ab2f 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -41,7 +41,7 @@ struct i915_params i915 __read_mostly = {
 	.preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
 	.disable_power_well = 1,
 	.enable_ips = 1,
-	.fastboot = 0,
+	.fastboot = 42,
 	.prefault_disable = 0,
 	.reset = true,
 	.invert_brightness = 0,
@@ -132,7 +132,7 @@ MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
 
 module_param_named(fastboot, i915.fastboot, bool, 0600);
 MODULE_PARM_DESC(fastboot,
-	"Try to skip unnecessary mode sets at boot time (default: false)");
+	"Try to skip unnecessary mode sets at boot time (default: true)");
 
 module_param_named(prefault_disable, i915.prefault_disable, bool, 0600);
 MODULE_PARM_DESC(prefault_disable,
-- 
1.8.3.2

Re: [PATCH 1/5] drm/i915: preserve SSC if previously set v2

[Jani Nikula]

On Thu, 05 Jun 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Some machines may have a broken VBT or no VBT at all, but we still want
> to use SSC there.  So check for it and keep it enabled if we see it
> already on.  Based on an earlier fix from Kristian.
>
> v2: honor modparam if set too (Daniel)
>     read out at init time and store for panel_use_ssc() use (Jesse)
>
> Reported-by: Kristian Høgsberg <hoegsberg@gmail.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>  drivers/gpu/drm/i915/intel_display.c | 11 ++++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8631fb3..f57b752 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1404,6 +1404,8 @@ struct drm_i915_private {
>  	struct intel_opregion opregion;
>  	struct intel_vbt_data vbt;
>  
> +	bool bios_ssc; /* BIOS had SSC enabled at boot? */
> +
>  	/* overlay */
>  	struct intel_overlay *overlay;
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5cbb28..0e8c9bc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5355,7 +5355,7 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
>  {
>  	if (i915.panel_use_ssc >= 0)
>  		return i915.panel_use_ssc != 0;
> -	return dev_priv->vbt.lvds_use_ssc
> +	return (dev_priv->vbt.lvds_use_ssc || dev_priv->bios_ssc)
>  		&& !(dev_priv->quirks & QUIRK_LVDS_SSC_DISABLE);
>  }
>  
> @@ -12478,6 +12478,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  
>  void intel_modeset_gem_init(struct drm_device *dev)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *c;
>  	struct intel_framebuffer *fb;
>  
> @@ -12485,6 +12486,14 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  	intel_init_gt_powersave(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  
> +	/*
> +	 * There may be no VBT; and if the BIOS enabled SSC we can
> +	 * just keep using it to avoid unnecessary flicker.
> +	 */
> +	if ((HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) &&
> +	    (I915_READ(PCH_DREF_CONTROL) & DREF_SSC1_ENABLE))
> +			dev_priv->bios_ssc = true;

Do we have any need to differentiate between this and what VBT has? We
could just treat dev_priv->vbt as "VBT or what BIOS did", and do
dev_priv->vbt.lvds_use_ssc = true; here, possibly with a
DRM_DEBUG_KMS. It would simplify code elsewhere and be less error prone.
We already do a similar thing for the eDP bpp (see intel_dp.c, look for
"This is a big fat ugly hack").

BR,
Jani.


> +
>  	intel_modeset_init_hw(dev);
>  
>  	intel_setup_overlay(dev);
> -- 
> 1.8.3.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: preserve SSC if previously set v2

[Chris Wilson]

On Fri, Jun 06, 2014 at 02:04:21PM +0300, Jani Nikula wrote:
> Do we have any need to differentiate between this and what VBT has? We
> could just treat dev_priv->vbt as "VBT or what BIOS did", and do
> dev_priv->vbt.lvds_use_ssc = true; here, possibly with a
> DRM_DEBUG_KMS. It would simplify code elsewhere and be less error prone.
> We already do a similar thing for the eDP bpp (see intel_dp.c, look for
> "This is a big fat ugly hack").

Indeed, that is a nice simplification. Conceptually, at least.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/5] drm/i915: enable fastboot by default

[Jani Nikula]

On Thu, 05 Jun 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Let them eat mincemeat pie.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_params.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index d05a2af..081ab2f 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -41,7 +41,7 @@ struct i915_params i915 __read_mostly = {
>  	.preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
>  	.disable_power_well = 1,
>  	.enable_ips = 1,
> -	.fastboot = 0,
> +	.fastboot = 42,

The answer to the ultimate question of life, the universe, and
everything should simply be "true" here. Personally, I don't think it's
a bad answer.

>  	.prefault_disable = 0,
>  	.reset = true,
>  	.invert_brightness = 0,
> @@ -132,7 +132,7 @@ MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
>  
>  module_param_named(fastboot, i915.fastboot, bool, 0600);

Side note, why do we allow the param to be changed after boot?

BR,
Jani.

>  MODULE_PARM_DESC(fastboot,
> -	"Try to skip unnecessary mode sets at boot time (default: false)");
> +	"Try to skip unnecessary mode sets at boot time (default: true)");
>  
>  module_param_named(prefault_disable, i915.prefault_disable, bool, 0600);
>  MODULE_PARM_DESC(prefault_disable,
> -- 
> 1.8.3.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH 1/5] drm/i915: preserve SSC if previously set v2

[Daniel Vetter]

On Thu, Jun 05, 2014 at 11:24:27AM -0700, Jesse Barnes wrote:
> Some machines may have a broken VBT or no VBT at all, but we still want
> to use SSC there.  So check for it and keep it enabled if we see it
> already on.  Based on an earlier fix from Kristian.
> 
> v2: honor modparam if set too (Daniel)
>     read out at init time and store for panel_use_ssc() use (Jesse)
> 
> Reported-by: Kristian Høgsberg <hoegsberg@gmail.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Ugh.

This means the BIOS idea of when we need SSC and ours are diverging, which
isn't good. And I think we should further lock this down by tracking the
"uses SSC refclk" state in the pipe config, just to make sure we don't
fumble this.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>  drivers/gpu/drm/i915/intel_display.c | 11 ++++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8631fb3..f57b752 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1404,6 +1404,8 @@ struct drm_i915_private {
>  	struct intel_opregion opregion;
>  	struct intel_vbt_data vbt;
>  
> +	bool bios_ssc; /* BIOS had SSC enabled at boot? */
> +
>  	/* overlay */
>  	struct intel_overlay *overlay;
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5cbb28..0e8c9bc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5355,7 +5355,7 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
>  {
>  	if (i915.panel_use_ssc >= 0)
>  		return i915.panel_use_ssc != 0;
> -	return dev_priv->vbt.lvds_use_ssc
> +	return (dev_priv->vbt.lvds_use_ssc || dev_priv->bios_ssc)
>  		&& !(dev_priv->quirks & QUIRK_LVDS_SSC_DISABLE);
>  }
>  
> @@ -12478,6 +12478,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  
>  void intel_modeset_gem_init(struct drm_device *dev)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *c;
>  	struct intel_framebuffer *fb;
>  
> @@ -12485,6 +12486,14 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  	intel_init_gt_powersave(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  
> +	/*
> +	 * There may be no VBT; and if the BIOS enabled SSC we can
> +	 * just keep using it to avoid unnecessary flicker.
> +	 */
> +	if ((HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) &&
> +	    (I915_READ(PCH_DREF_CONTROL) & DREF_SSC1_ENABLE))
> +			dev_priv->bios_ssc = true;
> +
>  	intel_modeset_init_hw(dev);
>  
>  	intel_setup_overlay(dev);
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/5] drm/i915: preserve swizzle settings if necessary v3

[Daniel Vetter]

On Thu, Jun 05, 2014 at 11:24:28AM -0700, Jesse Barnes wrote:
> Some machines (like MBAs) might use a tiled framebuffer but not enable
> display swizzling at boot time.  We want to preserve that configuration
> if possible to prevent a boot time mode set.  On IVB+ it shouldn't
> affect performance anyway since the memory controller does internal
> swizzling anyway.
> 
> For most other configs we'll be able to enable swizzling at boot time,
> since the initial framebuffer won't be tiled, thus we won't see any
> corruption when we enable it.
> 
> v2: preserve swizzling if BIOS had it set (Daniel)
> v3: preserve swizzling only if we inherited a tiled framebuffer (Daniel)
>     check display swizzle setting in detect_bit_6_swizzle (Daniel)
>     use gen6 as cutoff point (Daniel)
> 
> Reported-by: Kristian Høgsberg <hoegsberg@gmail.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.h        |  1 +
>  drivers/gpu/drm/i915/i915_gem.c        |  3 +++
>  drivers/gpu/drm/i915/i915_gem_tiling.c | 38 +++++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_display.c   |  3 +++
>  4 files changed, 28 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f57b752..f49fdcb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1405,6 +1405,7 @@ struct drm_i915_private {
>  	struct intel_vbt_data vbt;
>  
>  	bool bios_ssc; /* BIOS had SSC enabled at boot? */
> +	bool preserve_bios_swizzle;
>  
>  	/* overlay */
>  	struct intel_overlay *overlay;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index bfc7af0..0b168fb 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4580,6 +4580,9 @@ void i915_gem_init_swizzling(struct drm_device *dev)
>  	    dev_priv->mm.bit_6_swizzle_x == I915_BIT_6_SWIZZLE_NONE)
>  		return;
>  
> +	if (INTEL_INFO(dev)->gen >= 6 && dev_priv->preserve_bios_swizzle)
> +		return;
> +

Above two hunks shouldnt be needed if the setup in
i915_gem_detect_bit_6_swizzle works correctly.

>  	I915_WRITE(DISP_ARB_CTL, I915_READ(DISP_ARB_CTL) |
>  				 DISP_TILE_SURFACE_SWIZZLING);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index cb150e8..73005ad 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -91,26 +91,30 @@ i915_gem_detect_bit_6_swizzle(struct drm_device *dev)
>  	uint32_t swizzle_x = I915_BIT_6_SWIZZLE_UNKNOWN;
>  	uint32_t swizzle_y = I915_BIT_6_SWIZZLE_UNKNOWN;
>  
> -	if (IS_VALLEYVIEW(dev)) {
> -		swizzle_x = I915_BIT_6_SWIZZLE_NONE;
> -		swizzle_y = I915_BIT_6_SWIZZLE_NONE;
> -	} else if (INTEL_INFO(dev)->gen >= 6) {
> +	if (INTEL_INFO(dev)->gen >= 6) {
>  		uint32_t dimm_c0, dimm_c1;
> -		dimm_c0 = I915_READ(MAD_DIMM_C0);
> -		dimm_c1 = I915_READ(MAD_DIMM_C1);
> -		dimm_c0 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK;
> -		dimm_c1 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK;
> -		/* Enable swizzling when the channels are populated with
> -		 * identically sized dimms. We don't need to check the 3rd
> -		 * channel because no cpu with gpu attached ships in that
> -		 * configuration. Also, swizzling only makes sense for 2
> -		 * channels anyway. */
> -		if (dimm_c0 == dimm_c1) {
> -			swizzle_x = I915_BIT_6_SWIZZLE_9_10;
> -			swizzle_y = I915_BIT_6_SWIZZLE_9;
> -		} else {
> +
> +		/* Make sure to honor boot time display configuration */
> +		if (!(I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING)) {

Not quite what I had in mind. Imo we need to check for whether any
inherited fbs are tiled and if so also inherit the swizzle setting
unconditionally, whether it is swizzled or unswizzled. See

http://patchwork.freedesktop.org/patch/22204/

Cheers, Daniel

>  			swizzle_x = I915_BIT_6_SWIZZLE_NONE;
>  			swizzle_y = I915_BIT_6_SWIZZLE_NONE;
> +		} else {
> +			dimm_c0 = I915_READ(MAD_DIMM_C0);
> +			dimm_c1 = I915_READ(MAD_DIMM_C1);
> +			dimm_c0 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK;
> +			dimm_c1 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK;
> +			/* Enable swizzling when the channels are populated with
> +			 * identically sized dimms. We don't need to check the
> +			 * 3rd channel because no cpu with gpu attached ships
> +			 * in that configuration. Also, swizzling only makes
> +			 * sense for 2 channels anyway. */
> +			if (dimm_c0 == dimm_c1) {
> +				swizzle_x = I915_BIT_6_SWIZZLE_9_10;
> +				swizzle_y = I915_BIT_6_SWIZZLE_9;
> +			} else {
> +				swizzle_x = I915_BIT_6_SWIZZLE_NONE;
> +				swizzle_y = I915_BIT_6_SWIZZLE_NONE;
> +			}
>  		}
>  	} else if (IS_GEN5(dev)) {
>  		/* On Ironlake whatever DRAM config, GPU always do
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0e8c9bc..7dbbef7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2361,6 +2361,7 @@ static void intel_find_plane_obj(struct intel_crtc *intel_crtc,
>  				 struct intel_plane_config *plane_config)
>  {
>  	struct drm_device *dev = intel_crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *c;
>  	struct intel_crtc *i;
>  	struct intel_framebuffer *fb;
> @@ -2389,6 +2390,8 @@ static void intel_find_plane_obj(struct intel_crtc *intel_crtc,
>  
>  		fb = to_intel_framebuffer(c->primary->fb);
>  		if (i915_gem_obj_ggtt_offset(fb->obj) == plane_config->base) {
> +			if (fb->obj->tiling_mode != I915_TILING_NONE)
> +				dev_priv->preserve_bios_swizzle = true;
>  			drm_framebuffer_reference(c->primary->fb);
>  			intel_crtc->base.primary->fb = c->primary->fb;
>  			break;
> -- 
> 1.8.3.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 4/5] drm/i915: use current mode if the size matches the preferred mode

[Daniel Vetter]

On Thu, Jun 05, 2014 at 11:24:30AM -0700, Jesse Barnes wrote:
> From: Kristian Høgsberg <hoegsberg@gmail.com>
> 
> The BIOS may set a native mode that doesn't quite match the preferred
> mode timings.  It should be ok to use however if it uses the same size,
> so try to avoid a mode set in that case.
> 
> Signed-off-by: Kristian Høgsberg <hoegsberg@gmail.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Not sure we want this since this seems to override the cmdline options to
force a specific edid. Also not sure whether we shouldn't just add this as
the preferred mode when probing (before the preferred mode the vbt/edid
provides ofc).

What exactly is the mismatch here? It could be DRRS or something fancy,
too.

Not sure what to do here really.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 47 +++++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 088fe93..09819ae 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -379,42 +379,31 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>  		/* go for command line mode first */
>  		modes[i] = drm_pick_cmdline_mode(fb_conn, width, height);
>  
> -		/* try for preferred next */
> +		/* try for preferred next or match current */
>  		if (!modes[i]) {
> -			DRM_DEBUG_KMS("looking for preferred mode on connector %s\n",
> -				      connector->name);
> -			modes[i] = drm_has_preferred_mode(fb_conn, width,
> -							  height);
> -		}
> +			struct drm_display_mode *preferred;
>  
> -		/* No preferred mode marked by the EDID? Are there any modes? */
> -		if (!modes[i] && !list_empty(&connector->modes)) {
> -			DRM_DEBUG_KMS("using first mode listed on connector %s\n",
> +			DRM_DEBUG_KMS("looking for preferred mode on connector %s\n",
>  				      connector->name);
> -			modes[i] = list_first_entry(&connector->modes,
> -						    struct drm_display_mode,
> -						    head);
> -		}
> +			preferred = drm_has_preferred_mode(fb_conn, width,
> +							   height);
>  
> -		/* last resort: use current mode */
> -		if (!modes[i]) {
> -			/*
> -			 * IMPORTANT: We want to use the adjusted mode (i.e.
> -			 * after the panel fitter upscaling) as the initial
> -			 * config, not the input mode, which is what crtc->mode
> -			 * usually contains. But since our current fastboot
> -			 * code puts a mode derived from the post-pfit timings
> -			 * into crtc->mode this works out correctly. We don't
> -			 * use hwmode anywhere right now, so use it for this
> -			 * since the fb helper layer wants a pointer to
> -			 * something we own.
> -			 */
> -			DRM_DEBUG_KMS("looking for current mode on connector %s\n",
> -				      connector->name);
>  			intel_mode_from_pipe_config(&encoder->crtc->hwmode,
>  						    &to_intel_crtc(encoder->crtc)->config);
> -			modes[i] = &encoder->crtc->hwmode;
> + 			modes[i] = &encoder->crtc->hwmode;
> +
> +			if (preferred &&
> +			    !drm_mode_same_size(preferred, modes[i])) {
> +				DRM_DEBUG_KMS("using preferred mode %s "
> +					      "instead of current mode %s "
> +					      "on connector %d\n",
> +					      preferred->name,
> +					      modes[i]->name,
> +					      fb_conn->connector->base.id);
> +				modes[i] = preferred;
> +			}
>  		}
> +
>  		crtcs[i] = new_crtc;
>  
>  		DRM_DEBUG_KMS("connector %s on pipe %d [CRTC:%d]: %dx%d%s\n",
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 5/5] drm/i915: enable fastboot by default

[Daniel Vetter]

On Thu, Jun 05, 2014 at 11:24:31AM -0700, Jesse Barnes wrote:
> Let them eat mincemeat pie.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_params.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index d05a2af..081ab2f 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -41,7 +41,7 @@ struct i915_params i915 __read_mostly = {
>  	.preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
>  	.disable_power_well = 1,
>  	.enable_ips = 1,
> -	.fastboot = 0,
> +	.fastboot = 42,
>  	.prefault_disable = 0,
>  	.reset = true,
>  	.invert_brightness = 0,
> @@ -132,7 +132,7 @@ MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
>  
>  module_param_named(fastboot, i915.fastboot, bool, 0600);
>  MODULE_PARM_DESC(fastboot,
> -	"Try to skip unnecessary mode sets at boot time (default: false)");
> +	"Try to skip unnecessary mode sets at boot time (default: true)");

Nah, that wasn't the intention of this option. It was meant as a hack to
experiment around with fastboot and get things going, but imo we need to
really do the full modeset and short-circuit if the state matches.

And there's still a bunch of things we don't track like infoframes which
we either need to fix up (similar to the pfit fixup) or quirk to disallow
fastboot.
-Daniel

>  
>  module_param_named(prefault_disable, i915.prefault_disable, bool, 0600);
>  MODULE_PARM_DESC(prefault_disable,
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/5] drm/i915: enable fastboot by default

[Daniel Vetter]

On Fri, Jun 06, 2014 at 02:12:44PM +0300, Jani Nikula wrote:
> On Thu, 05 Jun 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > Let them eat mincemeat pie.
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_params.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > index d05a2af..081ab2f 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -41,7 +41,7 @@ struct i915_params i915 __read_mostly = {
> >  	.preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
> >  	.disable_power_well = 1,
> >  	.enable_ips = 1,
> > -	.fastboot = 0,
> > +	.fastboot = 42,
> 
> The answer to the ultimate question of life, the universe, and
> everything should simply be "true" here. Personally, I don't think it's
> a bad answer.
> 
> >  	.prefault_disable = 0,
> >  	.reset = true,
> >  	.invert_brightness = 0,
> > @@ -132,7 +132,7 @@ MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
> >  
> >  module_param_named(fastboot, i915.fastboot, bool, 0600);
> 
> Side note, why do we allow the param to be changed after boot?

Because fastboot works for every modeset, or at least should. E.g. if you
go from an upscale panel resolotion to the full one we still want to just
fixup the pfit, even when it's a normal modeset.

So fastboot really is a bit a misnomer, since the leftover bits (after
I've convinced Jesse that the fb takeover should be done unconditionally
really) is just some modeset fastpaths.

Aside: We still have a leak on the inherited fb, see

https://bugs.freedesktop.org/show_bug.cgi?id=77511

-Daniel

> 
> BR,
> Jani.
> 
> >  MODULE_PARM_DESC(fastboot,
> > -	"Try to skip unnecessary mode sets at boot time (default: false)");
> > +	"Try to skip unnecessary mode sets at boot time (default: true)");
> >  
> >  module_param_named(prefault_disable, i915.prefault_disable, bool, 0600);
> >  MODULE_PARM_DESC(prefault_disable,
> > -- 
> > 1.8.3.2
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/5] drm/i915: preserve swizzle settings if necessary v3

[Jesse Barnes]

On Tue, 10 Jun 2014 16:02:51 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Jun 05, 2014 at 11:24:28AM -0700, Jesse Barnes wrote:
> > Some machines (like MBAs) might use a tiled framebuffer but not enable
> > display swizzling at boot time.  We want to preserve that configuration
> > if possible to prevent a boot time mode set.  On IVB+ it shouldn't
> > affect performance anyway since the memory controller does internal
> > swizzling anyway.
> > 
> > For most other configs we'll be able to enable swizzling at boot time,
> > since the initial framebuffer won't be tiled, thus we won't see any
> > corruption when we enable it.
> > 
> > v2: preserve swizzling if BIOS had it set (Daniel)
> > v3: preserve swizzling only if we inherited a tiled framebuffer (Daniel)
> >     check display swizzle setting in detect_bit_6_swizzle (Daniel)
> >     use gen6 as cutoff point (Daniel)
> > 
> > Reported-by: Kristian Høgsberg <hoegsberg@gmail.com>
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h        |  1 +
> >  drivers/gpu/drm/i915/i915_gem.c        |  3 +++
> >  drivers/gpu/drm/i915/i915_gem_tiling.c | 38 +++++++++++++++++++---------------
> >  drivers/gpu/drm/i915/intel_display.c   |  3 +++
> >  4 files changed, 28 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index f57b752..f49fdcb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1405,6 +1405,7 @@ struct drm_i915_private {
> >  	struct intel_vbt_data vbt;
> >  
> >  	bool bios_ssc; /* BIOS had SSC enabled at boot? */
> > +	bool preserve_bios_swizzle;
> >  
> >  	/* overlay */
> >  	struct intel_overlay *overlay;
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index bfc7af0..0b168fb 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4580,6 +4580,9 @@ void i915_gem_init_swizzling(struct drm_device *dev)
> >  	    dev_priv->mm.bit_6_swizzle_x == I915_BIT_6_SWIZZLE_NONE)
> >  		return;
> >  
> > +	if (INTEL_INFO(dev)->gen >= 6 && dev_priv->preserve_bios_swizzle)
> > +		return;
> > +
> 
> Above two hunks shouldnt be needed if the setup in
> i915_gem_detect_bit_6_swizzle works correctly.
> 
> >  	I915_WRITE(DISP_ARB_CTL, I915_READ(DISP_ARB_CTL) |
> >  				 DISP_TILE_SURFACE_SWIZZLING);
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > index cb150e8..73005ad 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > @@ -91,26 +91,30 @@ i915_gem_detect_bit_6_swizzle(struct drm_device *dev)
> >  	uint32_t swizzle_x = I915_BIT_6_SWIZZLE_UNKNOWN;
> >  	uint32_t swizzle_y = I915_BIT_6_SWIZZLE_UNKNOWN;
> >  
> > -	if (IS_VALLEYVIEW(dev)) {
> > -		swizzle_x = I915_BIT_6_SWIZZLE_NONE;
> > -		swizzle_y = I915_BIT_6_SWIZZLE_NONE;
> > -	} else if (INTEL_INFO(dev)->gen >= 6) {
> > +	if (INTEL_INFO(dev)->gen >= 6) {
> >  		uint32_t dimm_c0, dimm_c1;
> > -		dimm_c0 = I915_READ(MAD_DIMM_C0);
> > -		dimm_c1 = I915_READ(MAD_DIMM_C1);
> > -		dimm_c0 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK;
> > -		dimm_c1 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK;
> > -		/* Enable swizzling when the channels are populated with
> > -		 * identically sized dimms. We don't need to check the 3rd
> > -		 * channel because no cpu with gpu attached ships in that
> > -		 * configuration. Also, swizzling only makes sense for 2
> > -		 * channels anyway. */
> > -		if (dimm_c0 == dimm_c1) {
> > -			swizzle_x = I915_BIT_6_SWIZZLE_9_10;
> > -			swizzle_y = I915_BIT_6_SWIZZLE_9;
> > -		} else {
> > +
> > +		/* Make sure to honor boot time display configuration */
> > +		if (!(I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING)) {
> 
> Not quite what I had in mind. Imo we need to check for whether any
> inherited fbs are tiled and if so also inherit the swizzle setting
> unconditionally, whether it is swizzled or unswizzled. See
> 
> http://patchwork.freedesktop.org/patch/22204/

Yes, that's what I do below... I even added it to the changelog:
http://patchwork.freedesktop.org/patch/27223/

Did you miss the later hunk in intel_display.c?

What we try to do here is enable swizzling if possible, which we can do
if no inherited fbs are tiled.

So I think I've done exactly what you repeated above, and documented
it.  So you're going to need to repeat it with different words so I can
understand, if I'm still missing something.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/i915: use current mode if the size matches the preferred mode

[Jesse Barnes]

On Tue, 10 Jun 2014 16:05:36 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Jun 05, 2014 at 11:24:30AM -0700, Jesse Barnes wrote:
> > From: Kristian Høgsberg <hoegsberg@gmail.com>
> > 
> > The BIOS may set a native mode that doesn't quite match the preferred
> > mode timings.  It should be ok to use however if it uses the same size,
> > so try to avoid a mode set in that case.
> > 
> > Signed-off-by: Kristian Høgsberg <hoegsberg@gmail.com>
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Not sure we want this since this seems to override the cmdline options to
> force a specific edid. Also not sure whether we shouldn't just add this as
> the preferred mode when probing (before the preferred mode the vbt/edid
> provides ofc).
> 
> What exactly is the mismatch here? It could be DRRS or something fancy,
> too.
> 
> Not sure what to do here really.

AFAICT it's just slightly different timings for fun.  I don't think
they go low enough to reduce the DP lane count... maybe there's just a
mismatch between their hard coded panel timings and the ones reported
by the EDID.  Not sure which to trust...  Kristian can you post the
timings you see here?  Both the BIOS timings and the EDID ones?

So I'm stuck here too, I think it's a rare case though.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: enable fastboot by default

[Jesse Barnes]

On Tue, 10 Jun 2014 16:07:44 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Jun 05, 2014 at 11:24:31AM -0700, Jesse Barnes wrote:
> > Let them eat mincemeat pie.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_params.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > index d05a2af..081ab2f 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -41,7 +41,7 @@ struct i915_params i915 __read_mostly = {
> >  	.preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
> >  	.disable_power_well = 1,
> >  	.enable_ips = 1,
> > -	.fastboot = 0,
> > +	.fastboot = 42,
> >  	.prefault_disable = 0,
> >  	.reset = true,
> >  	.invert_brightness = 0,
> > @@ -132,7 +132,7 @@ MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
> >  
> >  module_param_named(fastboot, i915.fastboot, bool, 0600);
> >  MODULE_PARM_DESC(fastboot,
> > -	"Try to skip unnecessary mode sets at boot time (default: false)");
> > +	"Try to skip unnecessary mode sets at boot time (default: true)");
> 
> Nah, that wasn't the intention of this option. It was meant as a hack to
> experiment around with fastboot and get things going, but imo we need to
> really do the full modeset and short-circuit if the state matches.
> 
> And there's still a bunch of things we don't track like infoframes which
> we either need to fix up (similar to the pfit fixup) or quirk to disallow
> fastboot.

Hm that contradicts our earlier discussions w/Damien when we decided
the infoframes stuff were too esoteric to matter...

Also do you want the mod_parm_desc updated to be more accurate?  Not
sure what the request is here.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 5/5] drm/i915: enable fastboot by default

[Stéphane Marchesin]

On Tue, Jun 10, 2014 at 10:31 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Tue, 10 Jun 2014 16:07:44 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
>> On Thu, Jun 05, 2014 at 11:24:31AM -0700, Jesse Barnes wrote:
>> > Let them eat mincemeat pie.
>> >
>> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>> > ---
>> >  drivers/gpu/drm/i915/i915_params.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>> > index d05a2af..081ab2f 100644
>> > --- a/drivers/gpu/drm/i915/i915_params.c
>> > +++ b/drivers/gpu/drm/i915/i915_params.c
>> > @@ -41,7 +41,7 @@ struct i915_params i915 __read_mostly = {
>> >     .preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
>> >     .disable_power_well = 1,
>> >     .enable_ips = 1,
>> > -   .fastboot = 0,
>> > +   .fastboot = 42,
>> >     .prefault_disable = 0,
>> >     .reset = true,
>> >     .invert_brightness = 0,
>> > @@ -132,7 +132,7 @@ MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
>> >
>> >  module_param_named(fastboot, i915.fastboot, bool, 0600);
>> >  MODULE_PARM_DESC(fastboot,
>> > -   "Try to skip unnecessary mode sets at boot time (default: false)");
>> > +   "Try to skip unnecessary mode sets at boot time (default: true)");
>>
>> Nah, that wasn't the intention of this option. It was meant as a hack to
>> experiment around with fastboot and get things going, but imo we need to
>> really do the full modeset and short-circuit if the state matches.
>>
>> And there's still a bunch of things we don't track like infoframes which
>> we either need to fix up (similar to the pfit fixup) or quirk to disallow
>> fastboot.
>
> Hm that contradicts our earlier discussions w/Damien when we decided
> the infoframes stuff were too esoteric to matter...

My 2 cents is that I've seen some really bad TVs which didn't work
because infoframes were missing (IIRC it relied on the VIC to detect
the video mode).

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

Re: [Intel-gfx] [PATCH 5/5] drm/i915: enable fastboot by default

[Jesse Barnes]

On Tue, 10 Jun 2014 11:01:06 -0700
Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:

> On Tue, Jun 10, 2014 at 10:31 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Tue, 10 Jun 2014 16:07:44 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> >> On Thu, Jun 05, 2014 at 11:24:31AM -0700, Jesse Barnes wrote:
> >> > Let them eat mincemeat pie.
> >> >
> >> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_params.c | 4 ++--
> >> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> >> > index d05a2af..081ab2f 100644
> >> > --- a/drivers/gpu/drm/i915/i915_params.c
> >> > +++ b/drivers/gpu/drm/i915/i915_params.c
> >> > @@ -41,7 +41,7 @@ struct i915_params i915 __read_mostly = {
> >> >     .preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
> >> >     .disable_power_well = 1,
> >> >     .enable_ips = 1,
> >> > -   .fastboot = 0,
> >> > +   .fastboot = 42,
> >> >     .prefault_disable = 0,
> >> >     .reset = true,
> >> >     .invert_brightness = 0,
> >> > @@ -132,7 +132,7 @@ MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
> >> >
> >> >  module_param_named(fastboot, i915.fastboot, bool, 0600);
> >> >  MODULE_PARM_DESC(fastboot,
> >> > -   "Try to skip unnecessary mode sets at boot time (default: false)");
> >> > +   "Try to skip unnecessary mode sets at boot time (default: true)");
> >>
> >> Nah, that wasn't the intention of this option. It was meant as a hack to
> >> experiment around with fastboot and get things going, but imo we need to
> >> really do the full modeset and short-circuit if the state matches.
> >>
> >> And there's still a bunch of things we don't track like infoframes which
> >> we either need to fix up (similar to the pfit fixup) or quirk to disallow
> >> fastboot.
> >
> > Hm that contradicts our earlier discussions w/Damien when we decided
> > the infoframes stuff were too esoteric to matter...
> 
> My 2 cents is that I've seen some really bad TVs which didn't work
> because infoframes were missing (IIRC it relied on the VIC to detect
> the video mode).

Yeah so we'd still leave them in place in this case, and apply them on
the next mode set as well, but we wouldn't be explicitly cross checking
for them, at least not yet.

It's a good thing to add, I just didn't think it was a blocker based on
our last discussion about this.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm/i915: preserve swizzle settings if necessary v3

[Daniel Vetter]

On Tue, Jun 10, 2014 at 7:27 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Yes, that's what I do below... I even added it to the changelog:
> http://patchwork.freedesktop.org/patch/27223/
>
> Did you miss the later hunk in intel_display.c?
>
> What we try to do here is enable swizzling if possible, which we can do
> if no inherited fbs are tiled.
>
> So I think I've done exactly what you repeated above, and documented
> it.  So you're going to need to repeat it with different words so I can
> understand, if I'm still missing something.

In swizzle_detect:

...

if (GEN6+) {
	if (preserve_bios_swizzle) {
		if (I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING) {
			swizzle_x = I915_BIT_6_SWIZZLE_9_10;
			...
		} else {
			swizzle_x = I915_BIT_6_SWIZZLE_NONE;
			...
		}
	} else {
		/* existing/old logic to decide about swizzling */
	}
}

...

Plus no shortcut in i915_gem_init_swizzling. Personally I'd also just use
a small helper function to compute preserve_bios_swizzle instead of
storing it in dev_priv (since we will only use it at exactly one place),
but that's a pure style preference.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/5] drm/i915: preserve swizzle settings if necessary v3

[Jesse Barnes]

On Tue, 10 Jun 2014 21:33:27 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Jun 10, 2014 at 7:27 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > Yes, that's what I do below... I even added it to the changelog:
> > http://patchwork.freedesktop.org/patch/27223/
> >
> > Did you miss the later hunk in intel_display.c?
> >
> > What we try to do here is enable swizzling if possible, which we can do
> > if no inherited fbs are tiled.
> >
> > So I think I've done exactly what you repeated above, and documented
> > it.  So you're going to need to repeat it with different words so I can
> > understand, if I'm still missing something.
> 
> In swizzle_detect:
> 
> ...
> 
> if (GEN6+) {
> 	if (preserve_bios_swizzle) {
> 		if (I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING) {
> 			swizzle_x = I915_BIT_6_SWIZZLE_9_10;
> 			...
> 		} else {
> 			swizzle_x = I915_BIT_6_SWIZZLE_NONE;
> 			...
> 		}
> 	} else {
> 		/* existing/old logic to decide about swizzling */
> 	}
> }
> 
> ...
> 
> Plus no shortcut in i915_gem_init_swizzling. Personally I'd also just use
> a small helper function to compute preserve_bios_swizzle instead of
> storing it in dev_priv (since we will only use it at exactly one place),
> but that's a pure style preference.

Doesn't this amount to the same thing?  I.e. we enable it if possible,
otherwise just report it as unswizzled?  So you're just wanting a style
change here?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/5] drm/i915: preserve swizzle settings if necessary v3

[Daniel Vetter]

On Tue, Jun 10, 2014 at 12:45:38PM -0700, Jesse Barnes wrote:
> On Tue, 10 Jun 2014 21:33:27 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Tue, Jun 10, 2014 at 7:27 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > Yes, that's what I do below... I even added it to the changelog:
> > > http://patchwork.freedesktop.org/patch/27223/
> > >
> > > Did you miss the later hunk in intel_display.c?
> > >
> > > What we try to do here is enable swizzling if possible, which we can do
> > > if no inherited fbs are tiled.
> > >
> > > So I think I've done exactly what you repeated above, and documented
> > > it.  So you're going to need to repeat it with different words so I can
> > > understand, if I'm still missing something.
> >
> > In swizzle_detect:
> >
> > ...
> >
> > if (GEN6+) {
> > if (preserve_bios_swizzle) {
> > if (I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING) {
> > swizzle_x = I915_BIT_6_SWIZZLE_9_10;
> > ...
> > } else {
> > swizzle_x = I915_BIT_6_SWIZZLE_NONE;
> > ...
> > }
> > } else {
> > /* existing/old logic to decide about swizzling */
> > }
> > }
> >
> > ...
> >
> > Plus no shortcut in i915_gem_init_swizzling. Personally I'd also just use
> > a small helper function to compute preserve_bios_swizzle instead of
> > storing it in dev_priv (since we will only use it at exactly one place),
> > but that's a pure style preference.
>
> Doesn't this amount to the same thing?  I.e. we enable it if possible,
> otherwise just report it as unswizzled?  So you're just wanting a style
> change here?

So presuming I read your code correctly there's two issues:

- The first thing you check in swizzle_detect is the hw swizzle bit in
  DSP_ARB. If that's not set you force unswizzled. Since most BIOS don't
  bother to set this (they use an untiled buffer after all) this means
  you've effectively disabled swizzling on almost all machines. The goal
  of keeping the old logic was that we actually want to enable swizzling
  in some situations.

- If you have a machine which uses tiled framebuffers and enables
  swizzling in the BIOS your code will a) drop the swizzle setup in
  gem_init_hw, breaking resume b) not set the swizzle settings correctly
  in swizzle_detect, breaking swap in/out and pwrite/pread. Not sure such
  a machine exists, but still.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/5] drm/i915: enable fastboot by default

[Daniel Vetter]

On Tue, Jun 10, 2014 at 11:42:37AM -0700, Jesse Barnes wrote:
> On Tue, 10 Jun 2014 11:01:06 -0700
> Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
> 
> > On Tue, Jun 10, 2014 at 10:31 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > On Tue, 10 Jun 2014 16:07:44 +0200
> > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > >> On Thu, Jun 05, 2014 at 11:24:31AM -0700, Jesse Barnes wrote:
> > >> > Let them eat mincemeat pie.
> > >> >
> > >> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > >> > ---
> > >> >  drivers/gpu/drm/i915/i915_params.c | 4 ++--
> > >> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > >> > index d05a2af..081ab2f 100644
> > >> > --- a/drivers/gpu/drm/i915/i915_params.c
> > >> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > >> > @@ -41,7 +41,7 @@ struct i915_params i915 __read_mostly = {
> > >> >     .preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
> > >> >     .disable_power_well = 1,
> > >> >     .enable_ips = 1,
> > >> > -   .fastboot = 0,
> > >> > +   .fastboot = 42,
> > >> >     .prefault_disable = 0,
> > >> >     .reset = true,
> > >> >     .invert_brightness = 0,
> > >> > @@ -132,7 +132,7 @@ MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
> > >> >
> > >> >  module_param_named(fastboot, i915.fastboot, bool, 0600);
> > >> >  MODULE_PARM_DESC(fastboot,
> > >> > -   "Try to skip unnecessary mode sets at boot time (default: false)");
> > >> > +   "Try to skip unnecessary mode sets at boot time (default: true)");
> > >>
> > >> Nah, that wasn't the intention of this option. It was meant as a hack to
> > >> experiment around with fastboot and get things going, but imo we need to
> > >> really do the full modeset and short-circuit if the state matches.
> > >>
> > >> And there's still a bunch of things we don't track like infoframes which
> > >> we either need to fix up (similar to the pfit fixup) or quirk to disallow
> > >> fastboot.
> > >
> > > Hm that contradicts our earlier discussions w/Damien when we decided
> > > the infoframes stuff were too esoteric to matter...

I'm pretty sure I've never claimed that infoframes are too esoteric. Like
Stéphane mentions they're really good at breaking shitty TVs and resulting
in black screens.

> > My 2 cents is that I've seen some really bad TVs which didn't work
> > because infoframes were missing (IIRC it relied on the VIC to detect
> > the video mode).
> 
> Yeah so we'd still leave them in place in this case, and apply them on
> the next mode set as well, but we wouldn't be explicitly cross checking
> for them, at least not yet.
> 
> It's a good thing to add, I just didn't think it was a blocker based on
> our last discussion about this.

Imo we should quirk hdmi and prevent fastboot exactly because infoframes
are such a pain. Relying on the BIOS for the just means we'll loose a lot
of testing coverage on random screens out there, which I very much want to
avoid.

Another piece of state we don't fix up atm is sound. Since my latest
rework this is tracked in the pipe config, so at least we'll catch that.

Also iirc Chris complained about the modeset state checker overhead caused
by the fastboot option since a normal flip done through setCrtc now hits
that unconditionally. If we'd push the fastboot logic into the overall
modeset path we could restrict modeset state checks to only when we need
them.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/5] drm/i915: preserve swizzle settings if necessary v3

[Jesse Barnes]

On Wed, 11 Jun 2014 11:23:26 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Jun 10, 2014 at 12:45:38PM -0700, Jesse Barnes wrote:
> > On Tue, 10 Jun 2014 21:33:27 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > > On Tue, Jun 10, 2014 at 7:27 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > > Yes, that's what I do below... I even added it to the changelog:
> > > > http://patchwork.freedesktop.org/patch/27223/
> > > >
> > > > Did you miss the later hunk in intel_display.c?
> > > >
> > > > What we try to do here is enable swizzling if possible, which we can do
> > > > if no inherited fbs are tiled.
> > > >
> > > > So I think I've done exactly what you repeated above, and documented
> > > > it.  So you're going to need to repeat it with different words so I can
> > > > understand, if I'm still missing something.
> > >
> > > In swizzle_detect:
> > >
> > > ...
> > >
> > > if (GEN6+) {
> > > if (preserve_bios_swizzle) {
> > > if (I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING) {
> > > swizzle_x = I915_BIT_6_SWIZZLE_9_10;
> > > ...
> > > } else {
> > > swizzle_x = I915_BIT_6_SWIZZLE_NONE;
> > > ...
> > > }
> > > } else {
> > > /* existing/old logic to decide about swizzling */
> > > }
> > > }
> > >
> > > ...
> > >
> > > Plus no shortcut in i915_gem_init_swizzling. Personally I'd also just use
> > > a small helper function to compute preserve_bios_swizzle instead of
> > > storing it in dev_priv (since we will only use it at exactly one place),
> > > but that's a pure style preference.
> >
> > Doesn't this amount to the same thing?  I.e. we enable it if possible,
> > otherwise just report it as unswizzled?  So you're just wanting a style
> > change here?
> 
> So presuming I read your code correctly there's two issues:
> 
> - The first thing you check in swizzle_detect is the hw swizzle bit in
>   DSP_ARB. If that's not set you force unswizzled. Since most BIOS don't
>   bother to set this (they use an untiled buffer after all) this means
>   you've effectively disabled swizzling on almost all machines. The goal
>   of keeping the old logic was that we actually want to enable swizzling
>   in some situations.

Ah damn, I had been thinking that bit_6_swizzle was the runtime call,
and that the init_swizzle call would go ahead and set it up properly.
I see that's too late though.

> - If you have a machine which uses tiled framebuffers and enables
>   swizzling in the BIOS your code will a) drop the swizzle setup in
>   gem_init_hw, breaking resume b) not set the swizzle settings correctly
>   in swizzle_detect, breaking swap in/out and pwrite/pread. Not sure such
>   a machine exists, but still.

This would affect krh's MBA, which is why I wanted testing here...
anyway I'll spin a new one and ask krh to test again.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/5] drm/i915: preserve swizzle settings if necessary v3

[Daniel Vetter]

On Wed, Jun 11, 2014 at 5:13 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> - If you have a machine which uses tiled framebuffers and enables
>>   swizzling in the BIOS your code will a) drop the swizzle setup in
>>   gem_init_hw, breaking resume b) not set the swizzle settings correctly
>>   in swizzle_detect, breaking swap in/out and pwrite/pread. Not sure such
>>   a machine exists, but still.
>
> This would affect krh's MBA, which is why I wanted testing here...
> anyway I'll spin a new one and ask krh to test again.

Hm, I've thought the issue with the MBA is that it used tiled fbs, but
non-swizzled. And then a mess ensued when we've enabled it. But yeah,
unfortunately with the new logic we need to retest :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/5] drm/i915: preserve swizzle settings if necessary v3

[Jesse Barnes]

On Wed, 11 Jun 2014 17:39:29 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Jun 11, 2014 at 5:13 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >> - If you have a machine which uses tiled framebuffers and enables
> >>   swizzling in the BIOS your code will a) drop the swizzle setup in
> >>   gem_init_hw, breaking resume b) not set the swizzle settings correctly
> >>   in swizzle_detect, breaking swap in/out and pwrite/pread. Not sure such
> >>   a machine exists, but still.
> >
> > This would affect krh's MBA, which is why I wanted testing here...
> > anyway I'll spin a new one and ask krh to test again.
> 
> Hm, I've thought the issue with the MBA is that it used tiled fbs, but
> non-swizzled. And then a mess ensued when we've enabled it. But yeah,
> unfortunately with the new logic we need to retest :(

Ah yeah I think you're right, either way, need more testing.

Maybe we should have just gone with the first patch to never enable
swizzling based on Art's assertion that it didn't matter.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/5] drm/i915: preserve swizzle settings if necessary v3

[Steve Aarnio]

On 06/11/2014 08:41 AM, Jesse Barnes wrote:
> On Wed, 11 Jun 2014 17:39:29 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
>> On Wed, Jun 11, 2014 at 5:13 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>>>> - If you have a machine which uses tiled framebuffers and enables
>>>>    swizzling in the BIOS your code will a) drop the swizzle setup in
>>>>    gem_init_hw, breaking resume b) not set the swizzle settings correctly
>>>>    in swizzle_detect, breaking swap in/out and pwrite/pread. Not sure such
>>>>    a machine exists, but still.
>>>
>>> This would affect krh's MBA, which is why I wanted testing here...
>>> anyway I'll spin a new one and ask krh to test again.
>>
>> Hm, I've thought the issue with the MBA is that it used tiled fbs, but
>> non-swizzled. And then a mess ensued when we've enabled it. But yeah,
>> unfortunately with the new logic we need to retest :(
>
> Ah yeah I think you're right, either way, need more testing.
>
> Maybe we should have just gone with the first patch to never enable
> swizzling based on Art's assertion that it didn't matter.
>

I hate to jump into the middle of a conversation that may or may not be related 
to a patch I just posted... but...

There was a very long internal discussion that the Windows guys had with H/W. 
For Gen8+ H/W recommends disabling CSX swizzle.  Technically, BDW still supports 
it, but there is a bug _somewhere_ that makes it problematic.  In any case it 
goes away for sure with Gen9+, so disabling on Gen8 doesn't hurt.

According to the other discussion, the H/W guys say that enabling actually hurts 
performance slightly, and the driver should leave the swizzle decisions to the 
memory controller.

Stevo

Re: [Intel-gfx] [PATCH 2/5] drm/i915: preserve swizzle settings if necessary v3

[Daniel Vetter]

On Fri, Jun 27, 2014 at 09:15:25AM -0700, Steve Aarnio wrote:
> On 06/11/2014 08:41 AM, Jesse Barnes wrote:
> >On Wed, 11 Jun 2014 17:39:29 +0200
> >Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> >>On Wed, Jun 11, 2014 at 5:13 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >>>>- If you have a machine which uses tiled framebuffers and enables
> >>>>   swizzling in the BIOS your code will a) drop the swizzle setup in
> >>>>   gem_init_hw, breaking resume b) not set the swizzle settings correctly
> >>>>   in swizzle_detect, breaking swap in/out and pwrite/pread. Not sure such
> >>>>   a machine exists, but still.
> >>>
> >>>This would affect krh's MBA, which is why I wanted testing here...
> >>>anyway I'll spin a new one and ask krh to test again.
> >>
> >>Hm, I've thought the issue with the MBA is that it used tiled fbs, but
> >>non-swizzled. And then a mess ensued when we've enabled it. But yeah,
> >>unfortunately with the new logic we need to retest :(
> >
> >Ah yeah I think you're right, either way, need more testing.
> >
> >Maybe we should have just gone with the first patch to never enable
> >swizzling based on Art's assertion that it didn't matter.
> >
> 
> I hate to jump into the middle of a conversation that may or may not be
> related to a patch I just posted... but...
> 
> There was a very long internal discussion that the Windows guys had with
> H/W. For Gen8+ H/W recommends disabling CSX swizzle.  Technically, BDW still
> supports it, but there is a bug _somewhere_ that makes it problematic.  In
> any case it goes away for sure with Gen9+, so disabling on Gen8 doesn't
> hurt.
> 
> According to the other discussion, the H/W guys say that enabling actually
> hurts performance slightly, and the driver should leave the swizzle
> decisions to the memory controller.

Patch to disable swizzling detection on gen8+ in i915_gem_tiling.c (only
there, imo ok to keep the hw paths around for setting up the registers)
welcome ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch