[PATCH 0/6] New drm crtc property for varying the Crtc size

[PATCH 0/6] New drm crtc property for varying the Crtc size

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

Added a new drm crtc property which provides control
to vary the Pipe Src or Crtc size. 
With this, User can flip the frame buffers of resolution, which
is different from the currently selected mode. For this Driver will
appropriately enable the Panel fitter internally, for the required
scaling.
For this also removed the check of change in frame buffer pitch,
from the crtc page flip function.  
Added a return type to panel fitter config & crtc retsore mode
functions, so that an error could be returned to User space for an 
invalid configuration. 
Also added Max downscale ratio checks when enbaling Panel fitter.

Akash Goel (6):
  drm/i915: Added a return type for panel fitter config functions
  drm/i915: Added a return type for the restore crtc mode function
  drm/i915: Added Max down-scale ratio checks when enabling Panel fitter
  drm/i915: Added support to allow change in FB pitch across flips
  Documentation/drm: Describing crtc size property
  drm/i915: New drm crtc property for varying the Crtc size

 Documentation/DocBook/drm.tmpl       |  13 +++-
 drivers/gpu/drm/drm_crtc.c           |   7 ++
 drivers/gpu/drm/drm_fb_helper.c      |   7 ++
 drivers/gpu/drm/i915/intel_display.c | 128 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_dp.c      |  45 +++++++++---
 drivers/gpu/drm/i915/intel_drv.h     |  11 ++-
 drivers/gpu/drm/i915/intel_hdmi.c    |  28 ++++++--
 drivers/gpu/drm/i915/intel_lvds.c    |  18 +++--
 drivers/gpu/drm/i915/intel_panel.c   | 123 ++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_sdvo.c    |  57 ++++++++++++++--
 drivers/gpu/drm/i915/intel_sprite.c  |   4 +-
 drivers/gpu/drm/i915/intel_tv.c      |  26 ++++++-
 include/drm/drm_crtc.h               |   7 ++
 13 files changed, 410 insertions(+), 64 deletions(-)

-- 
1.9.2

[PATCH 1/6] drm/i915: Added a return type for panel fitter config functions

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

This patch changes the return type of panel fitter configuration
functions from 'void', so that an error could be returned back to
User space, either during the modeset time or when some property
is being set, if the configuation is not valid.

Signed-off-by: Akash Goel <akash.goel@intel.com>
Signed-off-by: Pallavi G<pallavi.g@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c    | 15 +++++++++------
 drivers/gpu/drm/i915/intel_drv.h   |  4 ++--
 drivers/gpu/drm/i915/intel_lvds.c  | 11 ++++++-----
 drivers/gpu/drm/i915/intel_panel.c | 10 ++++++----
 4 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e5ada4f..3273b77 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -859,12 +859,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
 		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
 				       adjusted_mode);
-		if (!HAS_PCH_SPLIT(dev))
-			intel_gmch_panel_fitting(intel_crtc, pipe_config,
-						 intel_connector->panel.fitting_mode);
-		else
-			intel_pch_panel_fitting(intel_crtc, pipe_config,
-						intel_connector->panel.fitting_mode);
+		if (!HAS_PCH_SPLIT(dev)) {
+			if (!intel_gmch_panel_fitting(intel_crtc, pipe_config,
+						 intel_connector->panel.fitting_mode))
+				return false;
+		} else {
+			if (!intel_pch_panel_fitting(intel_crtc, pipe_config,
+						 intel_connector->panel.fitting_mode))
+				return false;
+		}
 	}
 
 	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3abc915..d1b5ded 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1016,10 +1016,10 @@ int intel_panel_init(struct intel_panel *panel,
 void intel_panel_fini(struct intel_panel *panel);
 void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
 			    struct drm_display_mode *adjusted_mode);
-void intel_pch_panel_fitting(struct intel_crtc *crtc,
+bool intel_pch_panel_fitting(struct intel_crtc *crtc,
 			     struct intel_crtc_config *pipe_config,
 			     int fitting_mode);
-void intel_gmch_panel_fitting(struct intel_crtc *crtc,
+bool intel_gmch_panel_fitting(struct intel_crtc *crtc,
 			      struct intel_crtc_config *pipe_config,
 			      int fitting_mode);
 void intel_panel_set_backlight_acpi(struct intel_connector *connector,
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 1987491..2a53768 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -317,12 +317,13 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
 	if (HAS_PCH_SPLIT(dev)) {
 		pipe_config->has_pch_encoder = true;
 
-		intel_pch_panel_fitting(intel_crtc, pipe_config,
-					intel_connector->panel.fitting_mode);
+		if (!intel_pch_panel_fitting(intel_crtc, pipe_config,
+					intel_connector->panel.fitting_mode))
+			return false;
 	} else {
-		intel_gmch_panel_fitting(intel_crtc, pipe_config,
-					 intel_connector->panel.fitting_mode);
-
+		if (!intel_gmch_panel_fitting(intel_crtc, pipe_config,
+					intel_connector->panel.fitting_mode))
+			return false;
 	}
 
 	/*
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 59b028f..15f2979 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -96,7 +96,7 @@ intel_find_panel_downclock(struct drm_device *dev,
 }
 
 /* adjusted_mode has been preset to be the panel's fixed mode */
-void
+bool
 intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
 			struct intel_crtc_config *pipe_config,
 			int fitting_mode)
@@ -158,13 +158,14 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
 
 	default:
 		WARN(1, "bad panel fit mode: %d\n", fitting_mode);
-		return;
+		return false;
 	}
 
 done:
 	pipe_config->pch_pfit.pos = (x << 16) | y;
 	pipe_config->pch_pfit.size = (width << 16) | height;
 	pipe_config->pch_pfit.enabled = pipe_config->pch_pfit.size != 0;
+	return true;
 }
 
 static void
@@ -300,7 +301,7 @@ static void i9xx_scale_aspect(struct intel_crtc_config *pipe_config,
 	}
 }
 
-void intel_gmch_panel_fitting(struct intel_crtc *intel_crtc,
+bool intel_gmch_panel_fitting(struct intel_crtc *intel_crtc,
 			      struct intel_crtc_config *pipe_config,
 			      int fitting_mode)
 {
@@ -352,7 +353,7 @@ void intel_gmch_panel_fitting(struct intel_crtc *intel_crtc,
 		break;
 	default:
 		WARN(1, "bad panel fit mode: %d\n", fitting_mode);
-		return;
+		return false;
 	}
 
 	/* 965+ wants fuzzy fitting */
@@ -374,6 +375,7 @@ out:
 	pipe_config->gmch_pfit.control = pfit_control;
 	pipe_config->gmch_pfit.pgm_ratios = pfit_pgm_ratios;
 	pipe_config->gmch_pfit.lvds_border_bits = border;
+	return true;
 }
 
 enum drm_connector_status
-- 
1.9.2

[PATCH 2/6] drm/i915: Added a return type for the restore crtc mode function

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

This patch changes the return type of 'crtc_restore_mode'
function from 'void', so that an error could be returned back to
User space, from the set property ioctl call, if the configuation
is not valid.

Signed-off-by: Akash Goel <akash.goel@intel.com>
Signed-off-by: Pallavi G<pallavi.g@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  5 ++--
 drivers/gpu/drm/i915/intel_dp.c      | 30 +++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_hdmi.c    | 28 +++++++++++++++---
 drivers/gpu/drm/i915/intel_lvds.c    |  7 ++++-
 drivers/gpu/drm/i915/intel_sdvo.c    | 57 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_tv.c      | 26 ++++++++++++++--
 7 files changed, 136 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5871efa..47a5424 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10964,9 +10964,10 @@ static int intel_set_mode(struct drm_crtc *crtc,
 	return ret;
 }
 
-void intel_crtc_restore_mode(struct drm_crtc *crtc)
+int intel_crtc_restore_mode(struct drm_crtc *crtc)
 {
-	intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y, crtc->primary->fb);
+	return intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y,
+						crtc->primary->fb);
 }
 
 #undef for_each_intel_crtc_masked
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3273b77..4457a06 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3889,6 +3889,9 @@ intel_dp_set_property(struct drm_connector *connector,
 	struct intel_encoder *intel_encoder = intel_attached_encoder(connector);
 	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
 	int ret;
+	bool old_has_audio = 0, old_auto = 0;
+	uint32_t old_range = 0;
+	int old_force_audio = 0, old_fitting_mode = 0;
 
 	ret = drm_object_property_set_value(&connector->base, property, val);
 	if (ret)
@@ -3901,6 +3904,9 @@ intel_dp_set_property(struct drm_connector *connector,
 		if (i == intel_dp->force_audio)
 			return 0;
 
+		old_force_audio = intel_dp->force_audio;
+		old_has_audio = intel_dp->has_audio;
+
 		intel_dp->force_audio = i;
 
 		if (i == HDMI_AUDIO_AUTO)
@@ -3916,8 +3922,8 @@ intel_dp_set_property(struct drm_connector *connector,
 	}
 
 	if (property == dev_priv->broadcast_rgb_property) {
-		bool old_auto = intel_dp->color_range_auto;
-		uint32_t old_range = intel_dp->color_range;
+		old_auto = intel_dp->color_range_auto;
+		old_range = intel_dp->color_range;
 
 		switch (val) {
 		case INTEL_BROADCAST_RGB_AUTO:
@@ -3953,6 +3959,7 @@ intel_dp_set_property(struct drm_connector *connector,
 			/* the eDP scaling property is not changed */
 			return 0;
 		}
+		old_fitting_mode = intel_connector->panel.fitting_mode;
 		intel_connector->panel.fitting_mode = val;
 
 		goto done;
@@ -3961,9 +3968,22 @@ intel_dp_set_property(struct drm_connector *connector,
 	return -EINVAL;
 
 done:
-	if (intel_encoder->base.crtc)
-		intel_crtc_restore_mode(intel_encoder->base.crtc);
-
+	if (intel_encoder->base.crtc) {
+		ret = intel_crtc_restore_mode(intel_encoder->base.crtc);
+		if (ret) {
+			if (property == dev_priv->force_audio_property) {
+				intel_dp->force_audio = old_force_audio;
+				intel_dp->has_audio = old_has_audio;
+			}
+			else if (property == dev_priv->broadcast_rgb_property) {
+				intel_dp->color_range_auto = old_auto;
+				intel_dp->color_range = old_range;
+			}
+			else if (property == connector->dev->mode_config.scaling_mode_property)
+				intel_connector->panel.fitting_mode = old_fitting_mode;
+		}
+		return ret;
+	}
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d1b5ded..a30abd9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -802,7 +802,7 @@ void intel_frontbuffer_flip(struct drm_device *dev,
 
 void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
 void intel_mark_idle(struct drm_device *dev);
-void intel_crtc_restore_mode(struct drm_crtc *crtc);
+int intel_crtc_restore_mode(struct drm_crtc *crtc);
 void intel_crtc_control(struct drm_crtc *crtc, bool enable);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);
 void intel_encoder_destroy(struct drm_encoder *encoder);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 9169786..a711000 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1074,6 +1074,9 @@ intel_hdmi_set_property(struct drm_connector *connector,
 		hdmi_to_dig_port(intel_hdmi);
 	struct drm_i915_private *dev_priv = connector->dev->dev_private;
 	int ret;
+	enum hdmi_force_audio old_force_audio = 0;
+	bool old_has_audio = 0, old_has_hdmi_sink = 0, old_auto = 0;
+	uint32_t old_range = 0;
 
 	ret = drm_object_property_set_value(&connector->base, property, val);
 	if (ret)
@@ -1086,6 +1089,10 @@ intel_hdmi_set_property(struct drm_connector *connector,
 		if (i == intel_hdmi->force_audio)
 			return 0;
 
+		old_force_audio = intel_hdmi->force_audio;
+		old_has_hdmi_sink = intel_hdmi->has_hdmi_sink;
+		old_has_audio = intel_hdmi->has_audio;
+
 		intel_hdmi->force_audio = i;
 
 		if (i == HDMI_AUDIO_AUTO)
@@ -1101,8 +1108,8 @@ intel_hdmi_set_property(struct drm_connector *connector,
 	}
 
 	if (property == dev_priv->broadcast_rgb_property) {
-		bool old_auto = intel_hdmi->color_range_auto;
-		uint32_t old_range = intel_hdmi->color_range;
+		old_auto = intel_hdmi->color_range_auto;
+		old_range = intel_hdmi->color_range;
 
 		switch (val) {
 		case INTEL_BROADCAST_RGB_AUTO:
@@ -1147,8 +1154,21 @@ intel_hdmi_set_property(struct drm_connector *connector,
 	return -EINVAL;
 
 done:
-	if (intel_dig_port->base.base.crtc)
-		intel_crtc_restore_mode(intel_dig_port->base.base.crtc);
+	if (intel_dig_port->base.base.crtc) {
+		ret = intel_crtc_restore_mode(intel_dig_port->base.base.crtc);
+		if (ret) {
+			if (property == dev_priv->force_audio_property) {
+				intel_hdmi->force_audio = old_force_audio;
+				intel_hdmi->has_hdmi_sink = old_has_hdmi_sink;
+				intel_hdmi->has_audio = old_has_audio;
+			}
+			else if (property == dev_priv->broadcast_rgb_property) {
+				intel_hdmi->color_range_auto = old_auto;
+				intel_hdmi->color_range = old_range;
+			}
+		}
+		return ret;
+	}
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 2a53768..8fd9299 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -493,6 +493,7 @@ static int intel_lvds_set_property(struct drm_connector *connector,
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct drm_device *dev = connector->dev;
+	int old_fitting_mode = 0;
 
 	if (property == dev->mode_config.scaling_mode_property) {
 		struct drm_crtc *crtc;
@@ -506,6 +507,7 @@ static int intel_lvds_set_property(struct drm_connector *connector,
 			/* the LVDS scaling property is not changed */
 			return 0;
 		}
+		old_fitting_mode = intel_connector->panel.fitting_mode;
 		intel_connector->panel.fitting_mode = value;
 
 		crtc = intel_attached_encoder(connector)->base.crtc;
@@ -514,7 +516,10 @@ static int intel_lvds_set_property(struct drm_connector *connector,
 			 * If the CRTC is enabled, the display will be changed
 			 * according to the new panel fitting mode.
 			 */
-			intel_crtc_restore_mode(crtc);
+			int ret = intel_crtc_restore_mode(crtc);
+			if (ret)
+				intel_connector->panel.fitting_mode = old_fitting_mode;
+			return ret;
 		}
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 9350edd..fe9b447 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2074,6 +2074,9 @@ intel_sdvo_set_property(struct drm_connector *connector,
 	uint16_t temp_value;
 	uint8_t cmd;
 	int ret;
+	bool old_has_hdmi_audio = 0, old_auto = 0;
+	uint32_t old_range = 0, old_value = 0;
+	int old_force_audio = 0, old_tv_format_index = 0;
 
 	ret = drm_object_property_set_value(&connector->base, property, val);
 	if (ret)
@@ -2086,6 +2089,9 @@ intel_sdvo_set_property(struct drm_connector *connector,
 		if (i == intel_sdvo_connector->force_audio)
 			return 0;
 
+		old_force_audio = intel_sdvo_connector->force_audio;
+		old_has_hdmi_audio = intel_sdvo->has_hdmi_audio;
+
 		intel_sdvo_connector->force_audio = i;
 
 		if (i == HDMI_AUDIO_AUTO)
@@ -2101,8 +2107,8 @@ intel_sdvo_set_property(struct drm_connector *connector,
 	}
 
 	if (property == dev_priv->broadcast_rgb_property) {
-		bool old_auto = intel_sdvo->color_range_auto;
-		uint32_t old_range = intel_sdvo->color_range;
+		old_auto = intel_sdvo->color_range_auto;
+		old_range = intel_sdvo->color_range;
 
 		switch (val) {
 		case INTEL_BROADCAST_RGB_AUTO:
@@ -2133,6 +2139,7 @@ intel_sdvo_set_property(struct drm_connector *connector,
 	if (intel_sdvo_connector->name == property) { \
 		if (intel_sdvo_connector->cur_##name == temp_value) return 0; \
 		if (intel_sdvo_connector->max_##name < temp_value) return -EINVAL; \
+		old_value = intel_sdvo_connector->cur_##name; \
 		cmd = SDVO_CMD_SET_##NAME; \
 		intel_sdvo_connector->cur_##name = temp_value; \
 		goto set_value; \
@@ -2146,6 +2153,7 @@ intel_sdvo_set_property(struct drm_connector *connector,
 		    intel_sdvo_connector->tv_format_supported[val])
 			return 0;
 
+		old_tv_format_index = intel_sdvo->tv_format_index;
 		intel_sdvo->tv_format_index = intel_sdvo_connector->tv_format_supported[val];
 		goto done;
 	} else if (IS_TV_OR_LVDS(intel_sdvo_connector)) {
@@ -2156,6 +2164,7 @@ intel_sdvo_set_property(struct drm_connector *connector,
 			if (intel_sdvo_connector->left_margin == temp_value)
 				return 0;
 
+			old_value = temp_value;
 			intel_sdvo_connector->left_margin = temp_value;
 			intel_sdvo_connector->right_margin = temp_value;
 			temp_value = intel_sdvo_connector->max_hscan -
@@ -2168,6 +2177,7 @@ intel_sdvo_set_property(struct drm_connector *connector,
 			if (intel_sdvo_connector->right_margin == temp_value)
 				return 0;
 
+			old_value = temp_value;
 			intel_sdvo_connector->left_margin = temp_value;
 			intel_sdvo_connector->right_margin = temp_value;
 			temp_value = intel_sdvo_connector->max_hscan -
@@ -2180,6 +2190,7 @@ intel_sdvo_set_property(struct drm_connector *connector,
 			if (intel_sdvo_connector->top_margin == temp_value)
 				return 0;
 
+			old_value = temp_value;
 			intel_sdvo_connector->top_margin = temp_value;
 			intel_sdvo_connector->bottom_margin = temp_value;
 			temp_value = intel_sdvo_connector->max_vscan -
@@ -2192,6 +2203,7 @@ intel_sdvo_set_property(struct drm_connector *connector,
 			if (intel_sdvo_connector->bottom_margin == temp_value)
 				return 0;
 
+			old_value = temp_value;
 			intel_sdvo_connector->top_margin = temp_value;
 			intel_sdvo_connector->bottom_margin = temp_value;
 			temp_value = intel_sdvo_connector->max_vscan -
@@ -2222,11 +2234,48 @@ set_value:
 
 
 done:
-	if (intel_sdvo->base.base.crtc)
-		intel_crtc_restore_mode(intel_sdvo->base.base.crtc);
+	if (intel_sdvo->base.base.crtc) {
+		int ret = intel_crtc_restore_mode(intel_sdvo->base.base.crtc);
+
+#define UNDO_PROPERTY(name) \
+		else if (intel_sdvo_connector->name == property) \
+			intel_sdvo_connector->cur_##name = old_value;
+
+		if (property == dev_priv->force_audio_property) {
+			intel_sdvo_connector->force_audio = old_force_audio;
+			intel_sdvo->has_hdmi_audio = old_has_hdmi_audio;
+		} else if (property == dev_priv->broadcast_rgb_property) {
+			intel_sdvo->color_range_auto = old_auto;
+			intel_sdvo->color_range = old_range;
+		} else if ((intel_sdvo_connector->left == property) ||
+				   (intel_sdvo_connector->right == property)) {
+			intel_sdvo_connector->left_margin = old_value;
+			intel_sdvo_connector->right_margin = old_value;
+		} else if ((intel_sdvo_connector->top == property) ||
+				   (intel_sdvo_connector->bottom == property)) {
+			intel_sdvo_connector->top_margin = old_value;
+			intel_sdvo_connector->bottom_margin = old_value;
+		} else if (property == intel_sdvo_connector->tv_format)
+			intel_sdvo->tv_format_index = old_tv_format_index;
+		UNDO_PROPERTY(hpos)
+		UNDO_PROPERTY(vpos)
+		UNDO_PROPERTY(saturation)
+		UNDO_PROPERTY(contrast)
+		UNDO_PROPERTY(hue)
+		UNDO_PROPERTY(brightness)
+		UNDO_PROPERTY(sharpness)
+		UNDO_PROPERTY(flicker_filter)
+		UNDO_PROPERTY(flicker_filter_2d)
+		UNDO_PROPERTY(flicker_filter_adaptive)
+		UNDO_PROPERTY(tv_chroma_filter)
+		UNDO_PROPERTY(tv_luma_filter)
+		UNDO_PROPERTY(dot_crawl)
+		return ret;
+	}
 
 	return 0;
 #undef CHECK_PROPERTY
+#undef UNDO_PROPERTY
 }
 
 static const struct drm_connector_funcs intel_sdvo_connector_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 32186a6..83059a8 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1458,6 +1458,8 @@ intel_tv_set_property(struct drm_connector *connector, struct drm_property *prop
 	struct drm_crtc *crtc = intel_tv->base.base.crtc;
 	int ret = 0;
 	bool changed = false;
+	int old_margin = 0;
+	const char *old_format = NULL;
 
 	ret = drm_object_property_set_value(&connector->base, property, val);
 	if (ret < 0)
@@ -1465,18 +1467,22 @@ intel_tv_set_property(struct drm_connector *connector, struct drm_property *prop
 
 	if (property == dev->mode_config.tv_left_margin_property &&
 		intel_tv->margin[TV_MARGIN_LEFT] != val) {
+		old_margin = intel_tv->margin[TV_MARGIN_LEFT];
 		intel_tv->margin[TV_MARGIN_LEFT] = val;
 		changed = true;
 	} else if (property == dev->mode_config.tv_right_margin_property &&
 		intel_tv->margin[TV_MARGIN_RIGHT] != val) {
+		old_margin = intel_tv->margin[TV_MARGIN_RIGHT];
 		intel_tv->margin[TV_MARGIN_RIGHT] = val;
 		changed = true;
 	} else if (property == dev->mode_config.tv_top_margin_property &&
 		intel_tv->margin[TV_MARGIN_TOP] != val) {
+		old_margin = intel_tv->margin[TV_MARGIN_TOP];
 		intel_tv->margin[TV_MARGIN_TOP] = val;
 		changed = true;
 	} else if (property == dev->mode_config.tv_bottom_margin_property &&
 		intel_tv->margin[TV_MARGIN_BOTTOM] != val) {
+		old_margin = intel_tv->margin[TV_MARGIN_BOTTOM];
 		intel_tv->margin[TV_MARGIN_BOTTOM] = val;
 		changed = true;
 	} else if (property == dev->mode_config.tv_mode_property) {
@@ -1487,6 +1493,7 @@ intel_tv_set_property(struct drm_connector *connector, struct drm_property *prop
 		if (!strcmp(intel_tv->tv_format, tv_modes[val].name))
 			goto out;
 
+		old_format = intel_tv->tv_format;
 		intel_tv->tv_format = tv_modes[val].name;
 		changed = true;
 	} else {
@@ -1494,8 +1501,23 @@ intel_tv_set_property(struct drm_connector *connector, struct drm_property *prop
 		goto out;
 	}
 
-	if (changed && crtc)
-		intel_crtc_restore_mode(crtc);
+	if (changed && crtc) {
+		ret =  intel_crtc_restore_mode(crtc);
+		if (ret) {
+			if (property == dev->mode_config.tv_left_margin_property)
+				intel_tv->margin[TV_MARGIN_LEFT] = old_margin;
+			else if (property == dev->mode_config.tv_right_margin_property)
+				intel_tv->margin[TV_MARGIN_RIGHT] = old_margin;
+			else if (property == dev->mode_config.tv_top_margin_property)
+				intel_tv->margin[TV_MARGIN_TOP] = old_margin;
+			else if (property == dev->mode_config.tv_bottom_margin_property)
+				intel_tv->margin[TV_MARGIN_BOTTOM] = old_margin;
+			else if (property == dev->mode_config.tv_mode_property) {
+				intel_tv->tv_format = old_format;
+			}
+		}
+		return ret;
+	}
 out:
 	return ret;
 }
-- 
1.9.2

[PATCH 3/6] drm/i915: Added Max down-scale ratio checks when enabling Panel fitter

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

This patch adds a check on the Max down scale ratio supported by the
Panel fitter. If Source width/height is too big, that the downscale
ratio of more than 1.125 is needed to fit into the Output window,
then that configuration will be rejected.

Signed-off-by: Akash Goel <akash.goel@intel.com>
Signed-off-by: Pallavi G<pallavi.g@intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c | 113 ++++++++++++++++++++++++++++++-------
 1 file changed, 93 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 15f2979..350e94d 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -33,6 +33,22 @@
 #include <linux/moduleparam.h>
 #include "intel_drv.h"
 
+/* Max Downscale ratio of 1.125, expressed in 1.12 fixed point format */
+#define MAX_DOWNSCALE_RATIO  (0x9 << 9)
+
+static inline u32 panel_fitter_scaling(u32 source, u32 target)
+{
+	/*
+	 * Floating point operation is not supported. So the FACTOR
+	 * is defined, which can avoid the floating point computation
+	 * when calculating the panel ratio.
+	 */
+#define ACCURACY 12
+#define FACTOR (1 << ACCURACY)
+	u32 ratio = source * FACTOR / target;
+	return (FACTOR * ratio + FACTOR/2) / FACTOR;
+}
+
 void
 intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
 		       struct drm_display_mode *adjusted_mode)
@@ -103,6 +119,7 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
 {
 	struct drm_display_mode *adjusted_mode;
 	int x, y, width, height;
+	u32 pf_horizontal_ratio, pf_vertical_ratio;
 
 	adjusted_mode = &pipe_config->adjusted_mode;
 
@@ -161,6 +178,19 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
 		return false;
 	}
 
+	pf_horizontal_ratio = panel_fitter_scaling(pipe_config->pipe_src_w,
+				width);
+	pf_vertical_ratio = panel_fitter_scaling(pipe_config->pipe_src_h,
+				height);
+
+	if (pf_horizontal_ratio > MAX_DOWNSCALE_RATIO) {
+		DRM_DEBUG_KMS("Src width is too big to downscale\n");
+		return false;
+	} else if (pf_vertical_ratio > MAX_DOWNSCALE_RATIO) {
+		DRM_DEBUG_KMS("Src height is too big to downscale\n");
+		return false;
+	}
+
 done:
 	pipe_config->pch_pfit.pos = (x << 16) | y;
 	pipe_config->pch_pfit.size = (width << 16) | height;
@@ -211,21 +241,9 @@ centre_vertically(struct drm_display_mode *mode,
 	mode->crtc_vsync_end = mode->crtc_vsync_start + sync_width;
 }
 
-static inline u32 panel_fitter_scaling(u32 source, u32 target)
-{
-	/*
-	 * Floating point operation is not supported. So the FACTOR
-	 * is defined, which can avoid the floating point computation
-	 * when calculating the panel ratio.
-	 */
-#define ACCURACY 12
-#define FACTOR (1 << ACCURACY)
-	u32 ratio = source * FACTOR / target;
-	return (FACTOR * ratio + FACTOR/2) / FACTOR;
-}
-
 static void i965_scale_aspect(struct intel_crtc_config *pipe_config,
-			      u32 *pfit_control)
+			      u32 *pfit_control,
+			      u32 *pf_horizontal_ratio, u32 *pf_vertical_ratio)
 {
 	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
 	u32 scaled_width = adjusted_mode->hdisplay *
@@ -234,19 +252,39 @@ static void i965_scale_aspect(struct intel_crtc_config *pipe_config,
 		adjusted_mode->vdisplay;
 
 	/* 965+ is easy, it does everything in hw */
-	if (scaled_width > scaled_height)
+	if (scaled_width > scaled_height) {
 		*pfit_control |= PFIT_ENABLE |
 			PFIT_SCALING_PILLAR;
-	else if (scaled_width < scaled_height)
+		*pf_horizontal_ratio =
+				panel_fitter_scaling(pipe_config->pipe_src_w,
+				      scaled_height / pipe_config->pipe_src_h);
+		*pf_vertical_ratio = panel_fitter_scaling(pipe_config->pipe_src_h,
+					    adjusted_mode->vdisplay);
+	}
+	else if (scaled_width < scaled_height) {
 		*pfit_control |= PFIT_ENABLE |
 			PFIT_SCALING_LETTER;
-	else if (adjusted_mode->hdisplay != pipe_config->pipe_src_w)
+		*pf_vertical_ratio =
+				panel_fitter_scaling(pipe_config->pipe_src_h,
+				      scaled_width / pipe_config->pipe_src_w);
+		*pf_horizontal_ratio = panel_fitter_scaling(pipe_config->pipe_src_w,
+						adjusted_mode->hdisplay);
+	}
+	else if (adjusted_mode->hdisplay != pipe_config->pipe_src_w) {
 		*pfit_control |= PFIT_ENABLE | PFIT_SCALING_AUTO;
+		*pf_horizontal_ratio =
+				panel_fitter_scaling(pipe_config->pipe_src_w,
+				adjusted_mode->hdisplay);
+		*pf_vertical_ratio =
+				panel_fitter_scaling(pipe_config->pipe_src_h,
+				adjusted_mode->vdisplay);
+	}
 }
 
 static void i9xx_scale_aspect(struct intel_crtc_config *pipe_config,
 			      u32 *pfit_control, u32 *pfit_pgm_ratios,
-			      u32 *border)
+			      u32 *border,
+			      u32 *pf_horizontal_ratio, u32 *pf_vertical_ratio)
 {
 	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
 	u32 scaled_width = adjusted_mode->hdisplay *
@@ -264,11 +302,15 @@ static void i9xx_scale_aspect(struct intel_crtc_config *pipe_config,
 		centre_horizontally(adjusted_mode,
 				    scaled_height /
 				    pipe_config->pipe_src_h);
+		*pf_horizontal_ratio =
+				panel_fitter_scaling(pipe_config->pipe_src_w,
+				    scaled_height / pipe_config->pipe_src_h);
 
 		*border = LVDS_BORDER_ENABLE;
 		if (pipe_config->pipe_src_h != adjusted_mode->vdisplay) {
 			bits = panel_fitter_scaling(pipe_config->pipe_src_h,
 						    adjusted_mode->vdisplay);
+			*pf_vertical_ratio = bits;
 
 			*pfit_pgm_ratios |= (bits << PFIT_HORIZ_SCALE_SHIFT |
 					     bits << PFIT_VERT_SCALE_SHIFT);
@@ -280,11 +322,15 @@ static void i9xx_scale_aspect(struct intel_crtc_config *pipe_config,
 		centre_vertically(adjusted_mode,
 				  scaled_width /
 				  pipe_config->pipe_src_w);
+		*pf_vertical_ratio =
+				panel_fitter_scaling(pipe_config->pipe_src_h,
+				    scaled_width / pipe_config->pipe_src_w);
 
 		*border = LVDS_BORDER_ENABLE;
 		if (pipe_config->pipe_src_w != adjusted_mode->hdisplay) {
 			bits = panel_fitter_scaling(pipe_config->pipe_src_w,
 						    adjusted_mode->hdisplay);
+			*pf_horizontal_ratio = bits;
 
 			*pfit_pgm_ratios |= (bits << PFIT_HORIZ_SCALE_SHIFT |
 					     bits << PFIT_VERT_SCALE_SHIFT);
@@ -298,6 +344,13 @@ static void i9xx_scale_aspect(struct intel_crtc_config *pipe_config,
 				  VERT_AUTO_SCALE | HORIZ_AUTO_SCALE |
 				  VERT_INTERP_BILINEAR |
 				  HORIZ_INTERP_BILINEAR);
+
+		*pf_horizontal_ratio =
+				panel_fitter_scaling(pipe_config->pipe_src_w,
+				adjusted_mode->hdisplay);
+		*pf_vertical_ratio =
+				panel_fitter_scaling(pipe_config->pipe_src_h,
+				adjusted_mode->vdisplay);
 	}
 }
 
@@ -308,6 +361,7 @@ bool intel_gmch_panel_fitting(struct intel_crtc *intel_crtc,
 	struct drm_device *dev = intel_crtc->base.dev;
 	u32 pfit_control = 0, pfit_pgm_ratios = 0, border = 0;
 	struct drm_display_mode *adjusted_mode;
+	u32 pf_horizontal_ratio = 0, pf_vertical_ratio = 0;
 
 	adjusted_mode = &pipe_config->adjusted_mode;
 
@@ -325,20 +379,31 @@ bool intel_gmch_panel_fitting(struct intel_crtc *intel_crtc,
 		centre_horizontally(adjusted_mode, pipe_config->pipe_src_w);
 		centre_vertically(adjusted_mode, pipe_config->pipe_src_h);
 		border = LVDS_BORDER_ENABLE;
+		/* 1:1 scaling */
+		pf_horizontal_ratio = pf_vertical_ratio = 1;
 		break;
 	case DRM_MODE_SCALE_ASPECT:
 		/* Scale but preserve the aspect ratio */
 		if (INTEL_INFO(dev)->gen >= 4)
-			i965_scale_aspect(pipe_config, &pfit_control);
+			i965_scale_aspect(pipe_config, &pfit_control,
+				 &pf_horizontal_ratio, &pf_vertical_ratio);
 		else
 			i9xx_scale_aspect(pipe_config, &pfit_control,
-					  &pfit_pgm_ratios, &border);
+					  &pfit_pgm_ratios, &border,
+					  &pf_horizontal_ratio,
+					  &pf_vertical_ratio);
 		break;
 	case DRM_MODE_SCALE_FULLSCREEN:
 		/*
 		 * Full scaling, even if it changes the aspect ratio.
 		 * Fortunately this is all done for us in hw.
 		 */
+		pf_horizontal_ratio =
+				panel_fitter_scaling(pipe_config->pipe_src_w,
+				adjusted_mode->hdisplay);
+		pf_vertical_ratio =
+				panel_fitter_scaling(pipe_config->pipe_src_h,
+				adjusted_mode->vdisplay);
 		if (pipe_config->pipe_src_h != adjusted_mode->vdisplay ||
 		    pipe_config->pipe_src_w != adjusted_mode->hdisplay) {
 			pfit_control |= PFIT_ENABLE;
@@ -356,6 +421,14 @@ bool intel_gmch_panel_fitting(struct intel_crtc *intel_crtc,
 		return false;
 	}
 
+	if (pf_horizontal_ratio > MAX_DOWNSCALE_RATIO) {
+		DRM_DEBUG_KMS("Src width is too big to downscale\n");
+		return false;
+	} else if (pf_vertical_ratio > MAX_DOWNSCALE_RATIO) {
+		DRM_DEBUG_KMS("Src height is too big to downscale\n");
+		return false;
+	}
+
 	/* 965+ wants fuzzy fitting */
 	/* FIXME: handle multiple panels by failing gracefully */
 	if (INTEL_INFO(dev)->gen >= 4)
-- 
1.9.2

[PATCH 4/6] drm/i915: Added support to allow change in FB pitch across flips

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

This patch removes the check for change in pitch of frame buffer
across page flips. Since there is a support for MMIO based page
flips, we can update the plane registers to update the
FB pitch & accordingly the offsets in LINOFF/TILEOFF registers.
The plane registers are updated atomically, using the mechanism
used for Sprite planes update. Doing so also requires deferring of
MMIO flip, if issued in interrupt context.

Signed-off-by: Akash Goel <akash.goel@intel.com>
Signed-off-by: Pallavi G<pallavi.g@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 51 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h     |  5 ++++
 drivers/gpu/drm/i915/intel_sprite.c  |  4 +--
 3 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 47a5424..a75d1a0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9542,6 +9542,29 @@ static bool use_mmio_flip(struct intel_engine_cs *ring,
 		return ring != obj->ring;
 }
 
+static void intel_mmio_flip_work_fn(struct work_struct *work)
+{
+	struct intel_mmio_flip *mmio_flip =
+		container_of(work, struct intel_mmio_flip, work);
+	struct intel_crtc *intel_crtc =
+		container_of(mmio_flip, struct intel_crtc, mmio_flip);
+	struct drm_device *dev = intel_crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc = &intel_crtc->base;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
+	intel_mark_page_flip_active(intel_crtc);
+
+	dev_priv->display.update_primary_plane(crtc, crtc->primary->fb,
+							crtc->x, crtc->y);
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+}
+
 static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
 {
 	struct drm_device *dev = intel_crtc->base.dev;
@@ -9552,6 +9575,17 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
 	u32 dspcntr;
 	u32 reg;
 
+	/* if we are in interrupt context and there is a need to update
+	   all the plane registers (atomically), then defer the flip to
+	   a process context, as we can then wait for the next vblank boundary */
+	if (intel_crtc->unpin_work->update_all_plane_reg) {
+		if (in_atomic())
+			schedule_work(&intel_crtc->mmio_flip.work);
+		else
+			intel_mmio_flip_work_fn(&intel_crtc->mmio_flip.work);
+		return;
+	}
+
 	intel_mark_page_flip_active(intel_crtc);
 
 	reg = DSPCNTR(intel_crtc->plane);
@@ -9685,6 +9719,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	struct intel_unpin_work *work;
 	struct intel_engine_cs *ring;
 	unsigned long flags;
+	bool update_all_plane_reg = 0;
 	int ret;
 
 	/*
@@ -9706,7 +9741,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (INTEL_INFO(dev)->gen > 3 &&
 	    (fb->offsets[0] != crtc->primary->fb->offsets[0] ||
 	     fb->pitches[0] != crtc->primary->fb->pitches[0]))
-		return -EINVAL;
+		update_all_plane_reg = 1;
 
 	if (i915_terminally_wedged(&dev_priv->gpu_error))
 		goto out_hang;
@@ -9718,6 +9753,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	work->event = event;
 	work->crtc = crtc;
 	work->old_fb_obj = intel_fb_obj(old_fb);
+	work->update_all_plane_reg = update_all_plane_reg;
 	INIT_WORK(&work->work, intel_unpin_work_fn);
 
 	ret = drm_crtc_vblank_get(crtc);
@@ -9785,9 +9821,14 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (use_mmio_flip(ring, obj))
 		ret = intel_queue_mmio_flip(dev, crtc, fb, obj, ring,
 					    page_flip_flags);
-	else
-		ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, ring,
-				page_flip_flags);
+	else {
+		if (update_all_plane_reg)
+			ret = -EINVAL;
+		else
+			ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, ring,
+					page_flip_flags);
+	}
+
 	if (ret)
 		goto cleanup_unpin;
 
@@ -11912,6 +11953,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	intel_crtc->cursor_cntl = ~0;
 	intel_crtc->cursor_size = ~0;
 
+	INIT_WORK(&intel_crtc->mmio_flip.work, intel_mmio_flip_work_fn);
+
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a30abd9..0cb685a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -375,6 +375,7 @@ struct intel_pipe_wm {
 struct intel_mmio_flip {
 	u32 seqno;
 	u32 ring_id;
+	struct work_struct work;
 };
 
 struct intel_crtc {
@@ -659,6 +660,7 @@ struct intel_unpin_work {
 	u32 flip_count;
 	u32 gtt_offset;
 	bool enable_stall_check;
+	bool update_all_plane_reg;
 };
 
 struct intel_set_config {
@@ -848,6 +850,9 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane);
 void intel_finish_page_flip(struct drm_device *dev, int pipe);
 void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
 
+bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count);
+void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
+
 /* shared dpll functions */
 struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
 void assert_shared_dpll(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0bdb00b..5e4a641 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -46,7 +46,7 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
 	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
 }
 
-static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
+bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
 {
 	struct drm_device *dev = crtc->base.dev;
 	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
@@ -112,7 +112,7 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl
 	return true;
 }
 
-static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
+void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
 {
 	struct drm_device *dev = crtc->base.dev;
 	enum pipe pipe = crtc->pipe;
-- 
1.9.2

Re: [PATCH 4/6] drm/i915: Added support to allow change in FB pitch across flips

[Daniel Vetter]

On Thu, Aug 14, 2014 at 02:54:25PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> This patch removes the check for change in pitch of frame buffer
> across page flips. Since there is a support for MMIO based page
> flips, we can update the plane registers to update the
> FB pitch & accordingly the offsets in LINOFF/TILEOFF registers.
> The plane registers are updated atomically, using the mechanism
> used for Sprite planes update. Doing so also requires deferring of
> MMIO flip, if issued in interrupt context.
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> Signed-off-by: Pallavi G<pallavi.g@intel.com>

This looks like a separate patch from the overall work in this series. It
definitely needs an i-g-t testcase, preferrably one to check that the
update is indeed atomic.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 51 +++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h     |  5 ++++
>  drivers/gpu/drm/i915/intel_sprite.c  |  4 +--
>  3 files changed, 54 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 47a5424..a75d1a0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9542,6 +9542,29 @@ static bool use_mmio_flip(struct intel_engine_cs *ring,
>  		return ring != obj->ring;
>  }
>  
> +static void intel_mmio_flip_work_fn(struct work_struct *work)
> +{
> +	struct intel_mmio_flip *mmio_flip =
> +		container_of(work, struct intel_mmio_flip, work);
> +	struct intel_crtc *intel_crtc =
> +		container_of(mmio_flip, struct intel_crtc, mmio_flip);
> +	struct drm_device *dev = intel_crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	u32 start_vbl_count;
> +	bool atomic_update;
> +
> +	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
> +
> +	intel_mark_page_flip_active(intel_crtc);
> +
> +	dev_priv->display.update_primary_plane(crtc, crtc->primary->fb,
> +							crtc->x, crtc->y);
> +
> +	if (atomic_update)
> +		intel_pipe_update_end(intel_crtc, start_vbl_count);
> +}
> +
>  static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
>  {
>  	struct drm_device *dev = intel_crtc->base.dev;
> @@ -9552,6 +9575,17 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
>  	u32 dspcntr;
>  	u32 reg;
>  
> +	/* if we are in interrupt context and there is a need to update
> +	   all the plane registers (atomically), then defer the flip to
> +	   a process context, as we can then wait for the next vblank boundary */
> +	if (intel_crtc->unpin_work->update_all_plane_reg) {
> +		if (in_atomic())
> +			schedule_work(&intel_crtc->mmio_flip.work);
> +		else
> +			intel_mmio_flip_work_fn(&intel_crtc->mmio_flip.work);
> +		return;
> +	}
> +
>  	intel_mark_page_flip_active(intel_crtc);
>  
>  	reg = DSPCNTR(intel_crtc->plane);
> @@ -9685,6 +9719,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	struct intel_unpin_work *work;
>  	struct intel_engine_cs *ring;
>  	unsigned long flags;
> +	bool update_all_plane_reg = 0;
>  	int ret;
>  
>  	/*
> @@ -9706,7 +9741,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	if (INTEL_INFO(dev)->gen > 3 &&
>  	    (fb->offsets[0] != crtc->primary->fb->offsets[0] ||
>  	     fb->pitches[0] != crtc->primary->fb->pitches[0]))
> -		return -EINVAL;
> +		update_all_plane_reg = 1;
>  
>  	if (i915_terminally_wedged(&dev_priv->gpu_error))
>  		goto out_hang;
> @@ -9718,6 +9753,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	work->event = event;
>  	work->crtc = crtc;
>  	work->old_fb_obj = intel_fb_obj(old_fb);
> +	work->update_all_plane_reg = update_all_plane_reg;
>  	INIT_WORK(&work->work, intel_unpin_work_fn);
>  
>  	ret = drm_crtc_vblank_get(crtc);
> @@ -9785,9 +9821,14 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	if (use_mmio_flip(ring, obj))
>  		ret = intel_queue_mmio_flip(dev, crtc, fb, obj, ring,
>  					    page_flip_flags);
> -	else
> -		ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, ring,
> -				page_flip_flags);
> +	else {
> +		if (update_all_plane_reg)
> +			ret = -EINVAL;
> +		else
> +			ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, ring,
> +					page_flip_flags);
> +	}
> +
>  	if (ret)
>  		goto cleanup_unpin;
>  
> @@ -11912,6 +11953,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	intel_crtc->cursor_cntl = ~0;
>  	intel_crtc->cursor_size = ~0;
>  
> +	INIT_WORK(&intel_crtc->mmio_flip.work, intel_mmio_flip_work_fn);
> +
>  	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>  	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
>  	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a30abd9..0cb685a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -375,6 +375,7 @@ struct intel_pipe_wm {
>  struct intel_mmio_flip {
>  	u32 seqno;
>  	u32 ring_id;
> +	struct work_struct work;
>  };
>  
>  struct intel_crtc {
> @@ -659,6 +660,7 @@ struct intel_unpin_work {
>  	u32 flip_count;
>  	u32 gtt_offset;
>  	bool enable_stall_check;
> +	bool update_all_plane_reg;
>  };
>  
>  struct intel_set_config {
> @@ -848,6 +850,9 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane);
>  void intel_finish_page_flip(struct drm_device *dev, int pipe);
>  void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
>  
> +bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count);
> +void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
> +
>  /* shared dpll functions */
>  struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
>  void assert_shared_dpll(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0bdb00b..5e4a641 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -46,7 +46,7 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
>  	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
>  }
>  
> -static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
> +bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
> @@ -112,7 +112,7 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl
>  	return true;
>  }
>  
> -static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
> +void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	enum pipe pipe = crtc->pipe;
> -- 
> 1.9.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

[PATCH 5/6] Documentation/drm: Describing crtc size property

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

Updated drm documentation to include description of
Crtc size property

Signed-off-by: Akash Goel <akash.goel@intel.com>
Signed-off-by: Pallavi G<pallavi.g@intel.com>
---
 Documentation/DocBook/drm.tmpl | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index c11ec6b..7c8441d 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2508,7 +2508,7 @@ void intel_crt_init(struct drm_device *dev)
 	<td valign="top" >Description/Restrictions</td>
 	</tr>
 	<tr>
-	<td rowspan="21" valign="top" >DRM</td>
+	<td rowspan="22" valign="top" >DRM</td>
 	<td rowspan="2" valign="top" >Generic</td>
 	<td valign="top" >“EDID”</td>
 	<td valign="top" >BLOB | IMMUTABLE</td>
@@ -2639,7 +2639,7 @@ void intel_crt_init(struct drm_device *dev)
 	<td valign="top" >TBD</td>
 	</tr>
 	<tr>
-	<td rowspan="3" valign="top" >Optional</td>
+	<td rowspan="4" valign="top" >Optional</td>
 	<td valign="top" >“scaling mode”</td>
 	<td valign="top" >ENUM</td>
 	<td valign="top" >{ "None", "Full", "Center", "Full aspect" }</td>
@@ -2663,6 +2663,15 @@ void intel_crt_init(struct drm_device *dev)
 	<td valign="top" >TBD</td>
 	</tr>
 	<tr>
+	<td valign="top" >“crtc size”</td>
+	<td valign="top" >RANGE</td>
+	<td valign="top" >Min=0, Max=0xFFFFFFFF</td>
+	<td valign="top" >Crtc</td>
+	<td valign="top" >DRM property to change the Crtc size dynamically.
+		Height/Width are in packed(height in lower 16 bits) form so that both
+		can be updated in a single call.</td>
+	</tr>
+	<tr>
 	<td rowspan="21" valign="top" >i915</td>
 	<td rowspan="2" valign="top" >Generic</td>
 	<td valign="top" >"Broadcast RGB"</td>
-- 
1.9.2

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

[PATCH 6/6] drm/i915: New drm crtc property for varying the Crtc size

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

This patch adds a new drm crtc property for varying the Pipe Src size
or the Panel fitter input size. Pipe Src controls the size that is
scaled from.
This will allow to dynamically flip the frame buffers
of different resolutions.
For this Driver internally enables the Panel fitter or Hw scaler
if its a Fixed mode panel & new Pipe Src values differ from the
Pipe timings

Signed-off-by: Akash Goel <akash.goel@intel.com>
Signed-off-by: Pallavi G<pallavi.g@intel.com>
---
 drivers/gpu/drm/drm_crtc.c           |  7 ++++
 drivers/gpu/drm/drm_fb_helper.c      |  7 ++++
 drivers/gpu/drm/i915/intel_display.c | 72 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h               |  7 ++++
 4 files changed, 93 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index f09b752..328efac 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -5169,3 +5169,10 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
 					   supported_rotations);
 }
 EXPORT_SYMBOL(drm_mode_create_rotation_property);
+
+struct drm_property *drm_mode_create_crtc_size_property(struct drm_device *dev)
+{
+	return drm_property_create_range(dev, 0, "crtc size",
+					   0, UINT_MAX);
+}
+EXPORT_SYMBOL(drm_mode_create_crtc_size_property);
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 63d7b8e..6eb1949 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -307,6 +307,13 @@ static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 		struct drm_crtc *crtc = mode_set->crtc;
 		int ret;
 
+		if (dev->mode_config.crtc_size_property) {
+			crtc->crtc_w = crtc->crtc_h = 0;
+			drm_object_property_set_value(&crtc->base,
+				dev->mode_config.crtc_size_property,
+				0);
+		}
+
 		if (crtc->funcs->cursor_set) {
 			ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0);
 			if (ret)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a75d1a0..7c417fc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10183,6 +10183,12 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 	pipe_config->pipe_src_w = pipe_config->requested_mode.crtc_hdisplay;
 	pipe_config->pipe_src_h = pipe_config->requested_mode.crtc_vdisplay;
 
+	/* Set the Pipe Src values as per the crtc size property values */
+	if (crtc->crtc_w && crtc->crtc_h) {
+		pipe_config->pipe_src_w = crtc->crtc_w;
+		pipe_config->pipe_src_h = crtc->crtc_h;
+	}
+
 encoder_retry:
 	/* Ensure the port clock defaults are reset when retrying. */
 	pipe_config->port_clock = 0;
@@ -11438,11 +11444,67 @@ out_config:
 	return ret;
 }
 
+static int intel_crtc_set_property(struct drm_crtc *crtc,
+				    struct drm_property *prop,
+				    uint64_t val)
+{
+	struct drm_device *dev = crtc->dev;
+	int ret = -ENOENT;
+
+	if (prop == dev->mode_config.crtc_size_property) {
+		int old_width, old_height;
+		int new_width = (int)((val >> 16) & 0xffff);
+		int new_height = (int)(val & 0xffff);
+		const struct drm_framebuffer *fb = crtc->primary->fb;
+
+		if ((new_width == crtc->crtc_w) &&
+		    (new_height == crtc->crtc_h))
+			return 0;
+
+		/* Check if property is resetted, so just return */
+		if ((new_width == 0) && (new_height) == 0) {
+			crtc->crtc_w = crtc->crtc_h = 0;
+			/* FIXME, Is modeset required here ?. Actually the
+			   current FB may not be compatible with the mode */
+			return 0;
+		} else if ((new_width == 0) || (new_height == 0))
+			return -EINVAL;
+
+		/* Check if the current FB is compatible with new requested
+		   Pipesrc values by the User */
+		if (new_width > fb->width ||
+			new_height > fb->height ||
+			crtc->x > fb->width - new_width ||
+			crtc->y > fb->height - new_height) {
+			DRM_DEBUG_KMS("New Pipe Src values %dx%d is incompatible with current fb size & viewport %ux%u+%d+%d\n",
+			      new_width, new_height, fb->width, fb->height, crtc->x, crtc->y);
+			return -EINVAL;
+		}
+
+		old_width = crtc->crtc_w;
+		old_height = crtc->crtc_h;
+
+		crtc->crtc_w = new_width;
+		crtc->crtc_h = new_height;
+
+		/* Currently do a modeset always, this will also
+		 * enable & configure the Panel fitter accordingly */
+		ret = intel_crtc_restore_mode(crtc);
+		if (ret) {
+			crtc->crtc_w = old_width;
+			crtc->crtc_h = old_height;
+		}
+	}
+
+	return ret;
+}
+
 static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.gamma_set = intel_crtc_gamma_set,
 	.set_config = intel_crtc_set_config,
 	.destroy = intel_crtc_destroy,
 	.page_flip = intel_crtc_page_flip,
+	.set_property = intel_crtc_set_property,
 };
 
 static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
@@ -11963,6 +12025,16 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
 
 	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
+
+	if (!dev->mode_config.crtc_size_property)
+			dev->mode_config.crtc_size_property =
+				drm_mode_create_crtc_size_property(dev);
+
+	if (dev->mode_config.crtc_size_property)
+			drm_object_attach_property(&intel_crtc->base.base,
+				dev->mode_config.crtc_size_property,
+				0);
+
 	return;
 
 fail:
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 0375d75..b409003 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -354,6 +354,11 @@ struct drm_crtc {
 	bool invert_dimensions;
 
 	int x, y;
+
+	/* Gets set only through the crtc_size property */
+	int crtc_w;
+	int crtc_h;
+
 	const struct drm_crtc_funcs *funcs;
 
 	/* CRTC gamma size for reporting to userspace */
@@ -826,6 +831,7 @@ struct drm_mode_config {
 	struct drm_property *path_property;
 	struct drm_property *plane_type_property;
 	struct drm_property *rotation_property;
+	struct drm_property *crtc_size_property;
 
 	/* DVI-I properties */
 	struct drm_property *dvi_i_subconnector_property;
@@ -1137,6 +1143,7 @@ extern int drm_format_vert_chroma_subsampling(uint32_t format);
 extern const char *drm_get_format_name(uint32_t format);
 extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
 							      unsigned int supported_rotations);
+extern struct drm_property *drm_mode_create_crtc_size_property(struct drm_device *dev);
 extern unsigned int drm_rotation_simplify(unsigned int rotation,
 					  unsigned int supported_rotations);
 
-- 
1.9.2

Re: [PATCH 6/6] drm/i915: New drm crtc property for varying the Crtc size

[Daniel Vetter]

On Thu, Aug 14, 2014 at 02:54:27PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> +		/* Check if the current FB is compatible with new requested
> +		   Pipesrc values by the User */
> +		if (new_width > fb->width ||
> +			new_height > fb->height ||
> +			crtc->x > fb->width - new_width ||
> +			crtc->y > fb->height - new_height) {
> +			DRM_DEBUG_KMS("New Pipe Src values %dx%d is incompatible with current fb size & viewport %ux%u+%d+%d\n",
> +			      new_width, new_height, fb->width, fb->height, crtc->x, crtc->y);
> +			return -EINVAL;
> +		}

I think this part here is the achilles heel of your approach. Right now we
use crtc->mode.hdisplay/vdisplay in a lot of places both in i915 and the
drm core to make sure that the primary plane fb, sprite fb and positioning
and cursor placement all make sense.

If my understanding of the pipe src rectangle is correct (please correct
me if I'm wrong) we should switch _all_ these checks to instead look at
the new crtc_w/h field. Even worse that we need to change drm core code
and as a result of that all drm drivers. Awful lot of code to check, test
and for i915 validate with i-g-t testcases.

Now the solution thus far (for the normal panel fitter operation) is that
the logical input mode for the crtc ends up in crtc->config.mode and as a
copy in crtc->mode. And the actual output mode is in
crtc->config.adjusted_mode.

Our modeset code already takes care of copying crtc->config.mode to
crtc->mode, so we only need to concern ourselfs with crtc->config.mode. If
we'd copy the pipe_src_w/h values back into it the existing code would
still be able to check all the sprite/cursor/fb constraints.

So the flow on a modeset (and that's what we'll end up calling from the
set_property function too) is:

1. Userspace requested mode goes into pipe_config->mode.
2. We make a copy into pipe_config->adjusted_mode and frob it more.
3. crtc compute_config code notices the special pipe src window, and
adjusts pipe_config->mode plus computes the panel fitter configuration.

If all that checks out we continue with the modeset sequence.

4. We store the new pipe_config into crtc->config.
5. Actual hw register writes for the modeset change happens.
6. We copy crtc->config.mode into crtc->mode so that drm core and helper
functions can validate fb/sprite/cursors again.

The result would be that the set_property function here would do _no_
argument checking, but would instead fully rely upon the modeset sequence
to compute the desired configuration. And if it fails it would restore the
old configuration like you already do.

Now test coverage: I think we need a few i-g-ts that provoke this, i.e.
which set the pipe_src != requested mode and then place cursor/sprite/fb
to validate that the checking is still correct.

Aside: In the shiny new world of atomic display updates we always need to
have similar procedures i.e.

1. Store state without much checking.
2. Run full modeset machinery to compute the new resulting state.
3. Check that, and only if that checks out, commit it.

If we duplicate special cases of these checks all over, then we'll have an
unmaintainable mess in shorter order. C.f. compare the discussion about
rotation properties.

Thoughts, better ideas and issues with this plan?

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

Re: [PATCH 6/6] drm/i915: New drm crtc property for varying the Crtc size

[Ville Syrjälä]

On Thu, Aug 14, 2014 at 04:42:01PM +0200, Daniel Vetter wrote:
> On Thu, Aug 14, 2014 at 02:54:27PM +0530, akash.goel@intel.com wrote:
> > From: Akash Goel <akash.goel@intel.com>
> > +		/* Check if the current FB is compatible with new requested
> > +		   Pipesrc values by the User */
> > +		if (new_width > fb->width ||
> > +			new_height > fb->height ||
> > +			crtc->x > fb->width - new_width ||
> > +			crtc->y > fb->height - new_height) {
> > +			DRM_DEBUG_KMS("New Pipe Src values %dx%d is incompatible with current fb size & viewport %ux%u+%d+%d\n",
> > +			      new_width, new_height, fb->width, fb->height, crtc->x, crtc->y);
> > +			return -EINVAL;
> > +		}
> 
> I think this part here is the achilles heel of your approach. Right now we
> use crtc->mode.hdisplay/vdisplay

We don't use it in i915. If we do that's a bug. All the relevant places
should be loooking at pipe_src_{w,h}. In the core the viewport check
should be about the only place that cares about this stuff.

> in a lot of places both in i915 and the
> drm core to make sure that the primary plane fb, sprite fb and positioning
> and cursor placement all make sense.
> 
> If my understanding of the pipe src rectangle is correct (please correct
> me if I'm wrong) we should switch _all_ these checks to instead look at
> the new crtc_w/h field. Even worse that we need to change drm core code
> and as a result of that all drm drivers. Awful lot of code to check, test
> and for i915 validate with i-g-t testcases.
> 
> Now the solution thus far (for the normal panel fitter operation) is that
> the logical input mode for the crtc ends up in crtc->config.mode and as a
> copy in crtc->mode. And the actual output mode is in
> crtc->config.adjusted_mode.
> 
> Our modeset code already takes care of copying crtc->config.mode to
> crtc->mode, so we only need to concern ourselfs with crtc->config.mode. If
> we'd copy the pipe_src_w/h values back into it the existing code would
> still be able to check all the sprite/cursor/fb constraints.
> 
> So the flow on a modeset (and that's what we'll end up calling from the
> set_property function too) is:
> 
> 1. Userspace requested mode goes into pipe_config->mode.
> 2. We make a copy into pipe_config->adjusted_mode and frob it more.
> 3. crtc compute_config code notices the special pipe src window, and
> adjusts pipe_config->mode plus computes the panel fitter configuration.
> 
> If all that checks out we continue with the modeset sequence.
> 
> 4. We store the new pipe_config into crtc->config.
> 5. Actual hw register writes for the modeset change happens.
> 6. We copy crtc->config.mode into crtc->mode so that drm core and helper
> functions can validate fb/sprite/cursors again.

We shouldn't just magically change the user specified mode, we need it
to stay intact for a subsequent modeset so that we can start the
adjusted_mode frobbery fresh next time around. It also seems weird to
report back a different mode to userspace than what the user provided.
What you suggest was exactly the previous approach and I NAKed it.

> The result would be that the set_property function here would do _no_
> argument checking, but would instead fully rely upon the modeset sequence
> to compute the desired configuration.

We don't have sufficient checks in the modeset path. The
drm_crtc_check_viewport() call is in drm_mode_setcrtc() which is too
high up in the stack to affect modesets originating from property
changes. That being said we should definitely use drm_crtc_check_viewport()
here instead of hand rolling the exacty same thing in i915.

And to avoid having to touch too much code, drm_crtc_check_viewport()
should encapsulate the logic to fall back to mode->{h,v}display when
crtc->{w,h} are zero.

> And if it fails it would restore the
> old configuration like you already do.
> 
> Now test coverage: I think we need a few i-g-ts that provoke this, i.e.
> which set the pipe_src != requested mode and then place cursor/sprite/fb
> to validate that the checking is still correct.
> 
> Aside: In the shiny new world of atomic display updates we always need to
> have similar procedures i.e.
> 
> 1. Store state without much checking.
> 2. Run full modeset machinery to compute the new resulting state.
> 3. Check that, and only if that checks out, commit it.
> 
> If we duplicate special cases of these checks all over, then we'll have an
> unmaintainable mess in shorter order. C.f. compare the discussion about
> rotation properties.
> 
> Thoughts, better ideas and issues with this plan?
> 
> Thanks, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 6/6] drm/i915: New drm crtc property for varying the Crtc size

[Daniel Vetter]

On Thu, Aug 14, 2014 at 06:06:58PM +0300, Ville Syrjälä wrote:
> On Thu, Aug 14, 2014 at 04:42:01PM +0200, Daniel Vetter wrote:
> > On Thu, Aug 14, 2014 at 02:54:27PM +0530, akash.goel@intel.com wrote:
> > > From: Akash Goel <akash.goel@intel.com>
> > > +		/* Check if the current FB is compatible with new requested
> > > +		   Pipesrc values by the User */
> > > +		if (new_width > fb->width ||
> > > +			new_height > fb->height ||
> > > +			crtc->x > fb->width - new_width ||
> > > +			crtc->y > fb->height - new_height) {
> > > +			DRM_DEBUG_KMS("New Pipe Src values %dx%d is incompatible with current fb size & viewport %ux%u+%d+%d\n",
> > > +			      new_width, new_height, fb->width, fb->height, crtc->x, crtc->y);
> > > +			return -EINVAL;
> > > +		}
> > 
> > I think this part here is the achilles heel of your approach. Right now we
> > use crtc->mode.hdisplay/vdisplay
> 
> We don't use it in i915. If we do that's a bug. All the relevant places
> should be loooking at pipe_src_{w,h}. In the core the viewport check
> should be about the only place that cares about this stuff.

Well within i915, but not anywhere in the drm core or helper code since
they're simply not aware of pipe_src_w/h. E.g. the plane helper code also
uses crtc->mode.h/vdisplay.

Changing that basic drm assumption is really invasive imo.

> > in a lot of places both in i915 and the
> > drm core to make sure that the primary plane fb, sprite fb and positioning
> > and cursor placement all make sense.
> > 
> > If my understanding of the pipe src rectangle is correct (please correct
> > me if I'm wrong) we should switch _all_ these checks to instead look at
> > the new crtc_w/h field. Even worse that we need to change drm core code
> > and as a result of that all drm drivers. Awful lot of code to check, test
> > and for i915 validate with i-g-t testcases.
> > 
> > Now the solution thus far (for the normal panel fitter operation) is that
> > the logical input mode for the crtc ends up in crtc->config.mode and as a
> > copy in crtc->mode. And the actual output mode is in
> > crtc->config.adjusted_mode.
> > 
> > Our modeset code already takes care of copying crtc->config.mode to
> > crtc->mode, so we only need to concern ourselfs with crtc->config.mode. If
> > we'd copy the pipe_src_w/h values back into it the existing code would
> > still be able to check all the sprite/cursor/fb constraints.
> > 
> > So the flow on a modeset (and that's what we'll end up calling from the
> > set_property function too) is:
> > 
> > 1. Userspace requested mode goes into pipe_config->mode.
> > 2. We make a copy into pipe_config->adjusted_mode and frob it more.
> > 3. crtc compute_config code notices the special pipe src window, and
> > adjusts pipe_config->mode plus computes the panel fitter configuration.
> > 
> > If all that checks out we continue with the modeset sequence.
> > 
> > 4. We store the new pipe_config into crtc->config.
> > 5. Actual hw register writes for the modeset change happens.
> > 6. We copy crtc->config.mode into crtc->mode so that drm core and helper
> > functions can validate fb/sprite/cursors again.
> 
> We shouldn't just magically change the user specified mode, we need it
> to stay intact for a subsequent modeset so that we can start the
> adjusted_mode frobbery fresh next time around. It also seems weird to
> report back a different mode to userspace than what the user provided.
> What you suggest was exactly the previous approach and I NAKed it.

Oh I'm fully aware that it's in the leagues of cross hacks ;-) But doing
the crtc->src_w/h thing correctly rolled out across the entire drm
subsystem will be a big chunk more work than just adding new variables to
struct drm_crtc which are only used in i915.

> > The result would be that the set_property function here would do _no_
> > argument checking, but would instead fully rely upon the modeset sequence
> > to compute the desired configuration.
> 
> We don't have sufficient checks in the modeset path. The
> drm_crtc_check_viewport() call is in drm_mode_setcrtc() which is too
> high up in the stack to affect modesets originating from property
> changes. That being said we should definitely use drm_crtc_check_viewport()
> here instead of hand rolling the exacty same thing in i915.

I guess then it needs to be moved down eventually.

> And to avoid having to touch too much code, drm_crtc_check_viewport()
> should encapsulate the logic to fall back to mode->{h,v}display when
> crtc->{w,h} are zero.

Actually you get to do a full drm audit anyway, since there's more than
check_viewport. They should all switch to crtc->src_w/h, and we should
fill that out by default in the crtc helpers. Imo stuffing magic new stuff
into the core only used by one driver isn't good, if we go this route we
should do it properly.

Or we ditch the struct drm_crtc change and pull it all into i915.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/6] drm/i915: New drm crtc property for varying the Crtc size

[Ville Syrjälä]

On Thu, Aug 14, 2014 at 05:26:27PM +0200, Daniel Vetter wrote:
> On Thu, Aug 14, 2014 at 06:06:58PM +0300, Ville Syrjälä wrote:
> > On Thu, Aug 14, 2014 at 04:42:01PM +0200, Daniel Vetter wrote:
> > > On Thu, Aug 14, 2014 at 02:54:27PM +0530, akash.goel@intel.com wrote:
> > > > From: Akash Goel <akash.goel@intel.com>
> > > > +		/* Check if the current FB is compatible with new requested
> > > > +		   Pipesrc values by the User */
> > > > +		if (new_width > fb->width ||
> > > > +			new_height > fb->height ||
> > > > +			crtc->x > fb->width - new_width ||
> > > > +			crtc->y > fb->height - new_height) {
> > > > +			DRM_DEBUG_KMS("New Pipe Src values %dx%d is incompatible with current fb size & viewport %ux%u+%d+%d\n",
> > > > +			      new_width, new_height, fb->width, fb->height, crtc->x, crtc->y);
> > > > +			return -EINVAL;
> > > > +		}
> > > 
> > > I think this part here is the achilles heel of your approach. Right now we
> > > use crtc->mode.hdisplay/vdisplay
> > 
> > We don't use it in i915. If we do that's a bug. All the relevant places
> > should be loooking at pipe_src_{w,h}. In the core the viewport check
> > should be about the only place that cares about this stuff.
> 
> Well within i915, but not anywhere in the drm core or helper code since
> they're simply not aware of pipe_src_w/h. E.g. the plane helper code also
> uses crtc->mode.h/vdisplay.
> 
> Changing that basic drm assumption is really invasive imo.
> 
> > > in a lot of places both in i915 and the
> > > drm core to make sure that the primary plane fb, sprite fb and positioning
> > > and cursor placement all make sense.
> > > 
> > > If my understanding of the pipe src rectangle is correct (please correct
> > > me if I'm wrong) we should switch _all_ these checks to instead look at
> > > the new crtc_w/h field. Even worse that we need to change drm core code
> > > and as a result of that all drm drivers. Awful lot of code to check, test
> > > and for i915 validate with i-g-t testcases.
> > > 
> > > Now the solution thus far (for the normal panel fitter operation) is that
> > > the logical input mode for the crtc ends up in crtc->config.mode and as a
> > > copy in crtc->mode. And the actual output mode is in
> > > crtc->config.adjusted_mode.
> > > 
> > > Our modeset code already takes care of copying crtc->config.mode to
> > > crtc->mode, so we only need to concern ourselfs with crtc->config.mode. If
> > > we'd copy the pipe_src_w/h values back into it the existing code would
> > > still be able to check all the sprite/cursor/fb constraints.
> > > 
> > > So the flow on a modeset (and that's what we'll end up calling from the
> > > set_property function too) is:
> > > 
> > > 1. Userspace requested mode goes into pipe_config->mode.
> > > 2. We make a copy into pipe_config->adjusted_mode and frob it more.
> > > 3. crtc compute_config code notices the special pipe src window, and
> > > adjusts pipe_config->mode plus computes the panel fitter configuration.
> > > 
> > > If all that checks out we continue with the modeset sequence.
> > > 
> > > 4. We store the new pipe_config into crtc->config.
> > > 5. Actual hw register writes for the modeset change happens.
> > > 6. We copy crtc->config.mode into crtc->mode so that drm core and helper
> > > functions can validate fb/sprite/cursors again.
> > 
> > We shouldn't just magically change the user specified mode, we need it
> > to stay intact for a subsequent modeset so that we can start the
> > adjusted_mode frobbery fresh next time around. It also seems weird to
> > report back a different mode to userspace than what the user provided.
> > What you suggest was exactly the previous approach and I NAKed it.
> 
> Oh I'm fully aware that it's in the leagues of cross hacks ;-) But doing
> the crtc->src_w/h thing correctly rolled out across the entire drm
> subsystem will be a big chunk more work than just adding new variables to
> struct drm_crtc which are only used in i915.
> 
> > > The result would be that the set_property function here would do _no_
> > > argument checking, but would instead fully rely upon the modeset sequence
> > > to compute the desired configuration.
> > 
> > We don't have sufficient checks in the modeset path. The
> > drm_crtc_check_viewport() call is in drm_mode_setcrtc() which is too
> > high up in the stack to affect modesets originating from property
> > changes. That being said we should definitely use drm_crtc_check_viewport()
> > here instead of hand rolling the exacty same thing in i915.
> 
> I guess then it needs to be moved down eventually.
> 
> > And to avoid having to touch too much code, drm_crtc_check_viewport()
> > should encapsulate the logic to fall back to mode->{h,v}display when
> > crtc->{w,h} are zero.
> 
> Actually you get to do a full drm audit anyway, since there's more than
> check_viewport. They should all switch to crtc->src_w/h, and we should
> fill that out by default in the crtc helpers. Imo stuffing magic new stuff
> into the core only used by one driver isn't good, if we go this route we
> should do it properly.
> 
> Or we ditch the struct drm_crtc change and pull it all into i915.

We can't since we need to get past drm_crtc_check_viewport().

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 6/6] drm/i915: New drm crtc property for varying the Crtc size

[Ville Syrjälä]

On Thu, Aug 14, 2014 at 05:26:27PM +0200, Daniel Vetter wrote:
> On Thu, Aug 14, 2014 at 06:06:58PM +0300, Ville Syrjälä wrote:
> > On Thu, Aug 14, 2014 at 04:42:01PM +0200, Daniel Vetter wrote:
> > > On Thu, Aug 14, 2014 at 02:54:27PM +0530, akash.goel@intel.com wrote:
> > > > From: Akash Goel <akash.goel@intel.com>
> > > > +		/* Check if the current FB is compatible with new requested
> > > > +		   Pipesrc values by the User */
> > > > +		if (new_width > fb->width ||
> > > > +			new_height > fb->height ||
> > > > +			crtc->x > fb->width - new_width ||
> > > > +			crtc->y > fb->height - new_height) {
> > > > +			DRM_DEBUG_KMS("New Pipe Src values %dx%d is incompatible with current fb size & viewport %ux%u+%d+%d\n",
> > > > +			      new_width, new_height, fb->width, fb->height, crtc->x, crtc->y);
> > > > +			return -EINVAL;
> > > > +		}
> > > 
> > > I think this part here is the achilles heel of your approach. Right now we
> > > use crtc->mode.hdisplay/vdisplay
> > 
> > We don't use it in i915. If we do that's a bug. All the relevant places
> > should be loooking at pipe_src_{w,h}. In the core the viewport check
> > should be about the only place that cares about this stuff.
> 
> Well within i915, but not anywhere in the drm core or helper code since
> they're simply not aware of pipe_src_w/h. E.g. the plane helper code also
> uses crtc->mode.h/vdisplay.

My quick grep audit tells me the viewport check and
drm_primary_helper_update() are the only places in the core that care.
The latter is rather dubious anyway since the clipping should be done
against the adjusted mode and not the user specified mode, so anyone
using that is already dancing on thin ice.

The other drivers are something I would not touch. Given how many places
we had to frob in i915 I'm somewhat sure I'd not like what I see there
and then any patch I might cook up would be too half assed to satisfy my
quality standards anyway.

As far as always filling the crtc->w,h always goes, I'm not sure that's
the best idea anyway since we still need the "0 is special" handling.
Well, we could fill them out, but then we definitely need a flag or
something to indicate that they came from the mode and not the
properties, so that we know whether we should overwrite them from
with {h,v}display during a subsquent modeset or if they should keep
their current value.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 6/6] drm/i915: New drm crtc property for varying the Crtc size

[Daniel Vetter]

On Thu, Aug 14, 2014 at 5:45 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> My quick grep audit tells me the viewport check and
> drm_primary_helper_update() are the only places in the core that care.
> The latter is rather dubious anyway since the clipping should be done
> against the adjusted mode and not the user specified mode, so anyone
> using that is already dancing on thin ice.

My understanding has always been that the requested mode is what
userspace plans to feed into the pipe, and the adjusted mode is what
actually lands in the sink. Yeah there's some hilarity in the vblank
code which somehow presumes that the vblank counter works with the
adjusted mode because that's what i915 needs.

It's that fundamental assumption that we break by making the pipe_src
stuff official and which is the part that freaks me out a bit.

> The other drivers are something I would not touch. Given how many places
> we had to frob in i915 I'm somewhat sure I'd not like what I see there
> and then any patch I might cook up would be too half assed to satisfy my
> quality standards anyway.

Yeah, other drivers only need to be audited I think once they start
supporting the pipe_src stuff. But I think the core+helpers should be
able to cope properly.

> As far as always filling the crtc->w,h always goes, I'm not sure that's
> the best idea anyway since we still need the "0 is special" handling.
> Well, we could fill them out, but then we definitely need a flag or
> something to indicate that they came from the mode and not the
> properties, so that we know whether we should overwrite them from
> with {h,v}display during a subsquent modeset or if they should keep
> their current value.

Hm, I guess we can keep that implicit meaning, but then we need a
small helper to get at the crtc viewport, e.g. drm_crtc_viewport_rect
or so. That would also be an excellent place to document this trickery
properly.

Oh and: Such drm changes _really_ must be split out into separate prep
patches cc: dri-devel.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: New drm crtc property for varying the Crtc size

[Ville Syrjälä]

On Thu, Aug 14, 2014 at 06:07:44PM +0200, Daniel Vetter wrote:
> On Thu, Aug 14, 2014 at 5:45 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > My quick grep audit tells me the viewport check and
> > drm_primary_helper_update() are the only places in the core that care.
> > The latter is rather dubious anyway since the clipping should be done
> > against the adjusted mode and not the user specified mode, so anyone
> > using that is already dancing on thin ice.
> 
> My understanding has always been that the requested mode is what
> userspace plans to feed into the pipe, and the adjusted mode is what
> actually lands in the sink. Yeah there's some hilarity in the vblank
> code which somehow presumes that the vblank counter works with the
> adjusted mode because that's what i915 needs.

The problem with clipping planes to the user size is that the driver is
free to frob the mode a bit to line it up with hardware constraints. So
what the user requested might be a few pixels off compared to what the
hardware will end up using, and then if you configure the plane
blindly using the coordinates clipped against the user size, the
hardware may get somewhat upset.

> 
> It's that fundamental assumption that we break by making the pipe_src
> stuff official and which is the part that freaks me out a bit.

Yeah there's definitely some dangers with these properties. The biggest
being when one master sets the properties, and then another master which
doesn't know anything about these properties takes over. The result
might be a failed modeset an then the user doesn't see anything.

This is one reason why I'd prefer that we'd maintain the state
per-master. For fbdev the "reset everything to default" trick seems
sufficient but I'm not sure I'd like to that to actual users.

> 
> > The other drivers are something I would not touch. Given how many places
> > we had to frob in i915 I'm somewhat sure I'd not like what I see there
> > and then any patch I might cook up would be too half assed to satisfy my
> > quality standards anyway.
> 
> Yeah, other drivers only need to be audited I think once they start
> supporting the pipe_src stuff. But I think the core+helpers should be
> able to cope properly.
> 
> > As far as always filling the crtc->w,h always goes, I'm not sure that's
> > the best idea anyway since we still need the "0 is special" handling.
> > Well, we could fill them out, but then we definitely need a flag or
> > something to indicate that they came from the mode and not the
> > properties, so that we know whether we should overwrite them from
> > with {h,v}display during a subsquent modeset or if they should keep
> > their current value.
> 
> Hm, I guess we can keep that implicit meaning, but then we need a
> small helper to get at the crtc viewport, e.g. drm_crtc_viewport_rect
> or so. That would also be an excellent place to document this trickery
> properly.

Yeah such small helpers is what I had in mind when suggesting this
originally.

> Oh and: Such drm changes _really_ must be split out into separate prep
> patches cc: dri-devel.

Indeed.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 6/6] drm/i915: New drm crtc property for varying the Crtc size

[Daniel Vetter]

On Thu, Aug 14, 2014 at 6:45 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Thu, Aug 14, 2014 at 06:07:44PM +0200, Daniel Vetter wrote:
>> On Thu, Aug 14, 2014 at 5:45 PM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > My quick grep audit tells me the viewport check and
>> > drm_primary_helper_update() are the only places in the core that care.
>> > The latter is rather dubious anyway since the clipping should be done
>> > against the adjusted mode and not the user specified mode, so anyone
>> > using that is already dancing on thin ice.
>>
>> My understanding has always been that the requested mode is what
>> userspace plans to feed into the pipe, and the adjusted mode is what
>> actually lands in the sink. Yeah there's some hilarity in the vblank
>> code which somehow presumes that the vblank counter works with the
>> adjusted mode because that's what i915 needs.
>
> The problem with clipping planes to the user size is that the driver is
> free to frob the mode a bit to line it up with hardware constraints. So
> what the user requested might be a few pixels off compared to what the
> hardware will end up using, and then if you configure the plane
> blindly using the coordinates clipped against the user size, the
> hardware may get somewhat upset.

Hm, I've thought we didn't do that yet, but only frobbed the adjusted
mode to make it fit our requirements for e.g interlaced stuff. I don't
think it would be good if we start doing that since there's no way for
userspace to be aware of the resulting single-pixel border of garbage.

_If_ we need to do that I think we should frob modes when adding them
to the connector mode list.

>> It's that fundamental assumption that we break by making the pipe_src
>> stuff official and which is the part that freaks me out a bit.
>
> Yeah there's definitely some dangers with these properties. The biggest
> being when one master sets the properties, and then another master which
> doesn't know anything about these properties takes over. The result
> might be a failed modeset an then the user doesn't see anything.
>
> This is one reason why I'd prefer that we'd maintain the state
> per-master. For fbdev the "reset everything to default" trick seems
> sufficient but I'm not sure I'd like to that to actual users.

Yeah, this is unsolved territory. Not sure whether attaching the state
to the master should be helpful, or whether we should care at all. For
a generic distro we should boot-up sane-ish, and userspace can always
grab the full property picture and restore that if all else fails. The
fbcon restoring is really just for developers imo.

>> > The other drivers are something I would not touch. Given how many places
>> > we had to frob in i915 I'm somewhat sure I'd not like what I see there
>> > and then any patch I might cook up would be too half assed to satisfy my
>> > quality standards anyway.
>>
>> Yeah, other drivers only need to be audited I think once they start
>> supporting the pipe_src stuff. But I think the core+helpers should be
>> able to cope properly.
>>
>> > As far as always filling the crtc->w,h always goes, I'm not sure that's
>> > the best idea anyway since we still need the "0 is special" handling.
>> > Well, we could fill them out, but then we definitely need a flag or
>> > something to indicate that they came from the mode and not the
>> > properties, so that we know whether we should overwrite them from
>> > with {h,v}display during a subsquent modeset or if they should keep
>> > their current value.
>>
>> Hm, I guess we can keep that implicit meaning, but then we need a
>> small helper to get at the crtc viewport, e.g. drm_crtc_viewport_rect
>> or so. That would also be an excellent place to document this trickery
>> properly.
>
> Yeah such small helpers is what I had in mind when suggesting this
> originally.
>
>> Oh and: Such drm changes _really_ must be split out into separate prep
>> patches cc: dri-devel.
>
> Indeed.

Sounds like we have a rough plan.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: New drm crtc property for varying the Crtc size

[Ville Syrjälä]

On Thu, Aug 14, 2014 at 07:36:13PM +0200, Daniel Vetter wrote:
> On Thu, Aug 14, 2014 at 6:45 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Aug 14, 2014 at 06:07:44PM +0200, Daniel Vetter wrote:
> >> On Thu, Aug 14, 2014 at 5:45 PM, Ville Syrjälä
> >> <ville.syrjala@linux.intel.com> wrote:
> >> > My quick grep audit tells me the viewport check and
> >> > drm_primary_helper_update() are the only places in the core that care.
> >> > The latter is rather dubious anyway since the clipping should be done
> >> > against the adjusted mode and not the user specified mode, so anyone
> >> > using that is already dancing on thin ice.
> >>
> >> My understanding has always been that the requested mode is what
> >> userspace plans to feed into the pipe, and the adjusted mode is what
> >> actually lands in the sink. Yeah there's some hilarity in the vblank
> >> code which somehow presumes that the vblank counter works with the
> >> adjusted mode because that's what i915 needs.
> >
> > The problem with clipping planes to the user size is that the driver is
> > free to frob the mode a bit to line it up with hardware constraints. So
> > what the user requested might be a few pixels off compared to what the
> > hardware will end up using, and then if you configure the plane
> > blindly using the coordinates clipped against the user size, the
> > hardware may get somewhat upset.
> 
> Hm, I've thought we didn't do that yet, but only frobbed the adjusted
> mode to make it fit our requirements for e.g interlaced stuff. I don't
> think it would be good if we start doing that since there's no way for
> userspace to be aware of the resulting single-pixel border of garbage.
> 
> _If_ we need to do that I think we should frob modes when adding them
> to the connector mode list.

Sure but the user can supply any mode, doesn't have to be on any list.
And the only sane rule for the frobbing would be that you can (slightly)
reduce hdisp/vdisp but never expand them so that there will never be any
extra garbage exposed (and the FB might not be big enough anyway). But
even reducing hdisp/vdisp by one pixel can be enough to anger the
hardware if a plane then extends one pixel into the blanking.

This isn't much of a problem for i915 though. The hardware is generally
good enough to not need it. Double wide and (s)dvo/lvds gang mode are
the only exception that comes to mind. Even there we just need to make
pipe src width even, but still that's something we have to account
when clipping planes.

On older hardware there were generally more restrictions eg. some
legacy baggage from VGA days which required horizontal timings to
be multiples of 8. I also "fondly" remember much more magic timing
restrictions in certain pieces hardware which were something close
to "if (foo*bar % this == that) frob else don't". IMO these kinds of
restrictions are too magic to make rejecting the mode an option,
so frobbing is the lesser of two evils.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 6/6] drm/i915: New drm crtc property for varying the Crtc size

[Daniel Vetter]

On Thu, Aug 14, 2014 at 7:58 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> Sure but the user can supply any mode, doesn't have to be on any list.
> And the only sane rule for the frobbing would be that you can (slightly)
> reduce hdisp/vdisp but never expand them so that there will never be any
> extra garbage exposed (and the FB might not be big enough anyway). But
> even reducing hdisp/vdisp by one pixel can be enough to anger the
> hardware if a plane then extends one pixel into the blanking.
>
> This isn't much of a problem for i915 though. The hardware is generally
> good enough to not need it. Double wide and (s)dvo/lvds gang mode are
> the only exception that comes to mind. Even there we just need to make
> pipe src width even, but still that's something we have to account
> when clipping planes.
>
> On older hardware there were generally more restrictions eg. some
> legacy baggage from VGA days which required horizontal timings to
> be multiples of 8. I also "fondly" remember much more magic timing
> restrictions in certain pieces hardware which were something close
> to "if (foo*bar % this == that) frob else don't". IMO these kinds of
> restrictions are too magic to make rejecting the mode an option,
> so frobbing is the lesser of two evils.

Imo the mode list we provide should be reasonable for everyone, and if
you start to add your own modes then I expect the user to do that
adjusting for us. Nowadays there should be very few cases where we
don't provide decent mode lists and where it's not a super-special
embedded thing where you need to configure everything yourself anyway.
So I don't think we should ever adjust the input region for a crtc.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: New drm crtc property for varying the Crtc size

[Ville Syrjälä]

On Thu, Aug 14, 2014 at 08:33:23PM +0200, Daniel Vetter wrote:
> On Thu, Aug 14, 2014 at 7:58 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > Sure but the user can supply any mode, doesn't have to be on any list.
> > And the only sane rule for the frobbing would be that you can (slightly)
> > reduce hdisp/vdisp but never expand them so that there will never be any
> > extra garbage exposed (and the FB might not be big enough anyway). But
> > even reducing hdisp/vdisp by one pixel can be enough to anger the
> > hardware if a plane then extends one pixel into the blanking.
> >
> > This isn't much of a problem for i915 though. The hardware is generally
> > good enough to not need it. Double wide and (s)dvo/lvds gang mode are
> > the only exception that comes to mind. Even there we just need to make
> > pipe src width even, but still that's something we have to account
> > when clipping planes.
> >
> > On older hardware there were generally more restrictions eg. some
> > legacy baggage from VGA days which required horizontal timings to
> > be multiples of 8. I also "fondly" remember much more magic timing
> > restrictions in certain pieces hardware which were something close
> > to "if (foo*bar % this == that) frob else don't". IMO these kinds of
> > restrictions are too magic to make rejecting the mode an option,
> > so frobbing is the lesser of two evils.
> 
> Imo the mode list we provide should be reasonable for everyone, and if
> you start to add your own modes then I expect the user to do that
> adjusting for us. Nowadays there should be very few cases where we
> don't provide decent mode lists and where it's not a super-special
> embedded thing where you need to configure everything yourself anyway.
> So I don't think we should ever adjust the input region for a crtc.

That's fine for decent hardware. But if/when I write a driver for
old junk I'm probably going to frob no matter what anyone says :)

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 0/6] New drm crtc property for varying the Crtc size

[Damien Lespiau]

Hi,

Do we have some (possiblly CRC based) tests coming for this? it'd be
super good to be able to test the work indepedently of a compositor when
reviewing. At the end of the day, we do really want tests covering that
code.

Thanks,

-- 
Damien

On Thu, Aug 14, 2014 at 02:54:21PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> Added a new drm crtc property which provides control
> to vary the Pipe Src or Crtc size. 
> With this, User can flip the frame buffers of resolution, which
> is different from the currently selected mode. For this Driver will
> appropriately enable the Panel fitter internally, for the required
> scaling.
> For this also removed the check of change in frame buffer pitch,
> from the crtc page flip function.  
> Added a return type to panel fitter config & crtc retsore mode
> functions, so that an error could be returned to User space for an 
> invalid configuration. 
> Also added Max downscale ratio checks when enbaling Panel fitter.
> 
> Akash Goel (6):
>   drm/i915: Added a return type for panel fitter config functions
>   drm/i915: Added a return type for the restore crtc mode function
>   drm/i915: Added Max down-scale ratio checks when enabling Panel fitter
>   drm/i915: Added support to allow change in FB pitch across flips
>   Documentation/drm: Describing crtc size property
>   drm/i915: New drm crtc property for varying the Crtc size
> 
>  Documentation/DocBook/drm.tmpl       |  13 +++-
>  drivers/gpu/drm/drm_crtc.c           |   7 ++
>  drivers/gpu/drm/drm_fb_helper.c      |   7 ++
>  drivers/gpu/drm/i915/intel_display.c | 128 +++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_dp.c      |  45 +++++++++---
>  drivers/gpu/drm/i915/intel_drv.h     |  11 ++-
>  drivers/gpu/drm/i915/intel_hdmi.c    |  28 ++++++--
>  drivers/gpu/drm/i915/intel_lvds.c    |  18 +++--
>  drivers/gpu/drm/i915/intel_panel.c   | 123 ++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_sdvo.c    |  57 ++++++++++++++--
>  drivers/gpu/drm/i915/intel_sprite.c  |   4 +-
>  drivers/gpu/drm/i915/intel_tv.c      |  26 ++++++-
>  include/drm/drm_crtc.h               |   7 ++
>  13 files changed, 410 insertions(+), 64 deletions(-)
> 
> -- 
> 1.9.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx