[PATCH v3 2/3] drm/i915: New drm crtc property for varying the Pipe Src size

[PATCH v3 2/3] drm/i915: New drm crtc property for varying the Pipe Src 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 (without modeset) the frame buffers
of different resolutions

v2: Added a check to fail the set property call if Panel fitter is
disabled & new PIPESRC programming do not match with PIPE timings.
Removed the pitch mismatch check on frame buffer, when being flipped.
This is currently done only for VLV/HSW.

v3: Modified the check, added in v2, to consider the platforms having
Split PCH.

Testcase: igt/kms_panel_fitter_test

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  6 ++++
 drivers/gpu/drm/i915/intel_display.c | 65 ++++++++++++++++++++++++++++++++++--
 2 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a0d90ef..6f3af15 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1608,6 +1608,12 @@ typedef struct drm_i915_private {
 	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
 
+	/*
+	 * Property to dynamically vary the size of the
+	 * PIPESRC or Panel fitter input size
+	 */
+	struct drm_property *input_size_property;
+
 	uint32_t hw_context_size;
 	struct list_head context_list;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5dfe156..7149123 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8935,8 +8935,18 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	 * Note that pitch changes could also affect these register.
 	 */
 	if (INTEL_INFO(dev)->gen > 3 &&
-	    (fb->offsets[0] != crtc->fb->offsets[0] ||
-	     fb->pitches[0] != crtc->fb->pitches[0]))
+	    (fb->offsets[0] != crtc->fb->offsets[0]))
+		return -EINVAL;
+
+	/*
+	 * Bypassing the fb pitch check for VLV/HSW, as purportedly there
+	 * is a dynamic flip support in VLV/HSW. This will allow to
+	 * flip fbs of different resolutions without doing a modeset.
+	 * TBD, confirm the same for other newer gen platforms also.
+	 */
+	if (INTEL_INFO(dev)->gen > 3 &&
+	    !IS_VALLEYVIEW(dev) && !IS_HASWELL(dev) &&
+	    (fb->pitches[0] != crtc->fb->pitches[0]))
 		return -EINVAL;
 
 	if (i915_terminally_wedged(&dev_priv->gpu_error))
@@ -10434,8 +10444,50 @@ out_config:
 static int intel_crtc_set_property(struct drm_crtc *crtc,
 		struct drm_property *property, uint64_t val)
 {
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int ret = -ENOENT;
 
+	if (property == dev_priv->input_size_property) {
+		int new_width = (int)((val >> 16) & 0xffff);
+		int new_height = (int)(val & 0xffff);
+		const struct drm_display_mode *adjusted_mode =
+				&intel_crtc->config.adjusted_mode;
+
+		if ((new_width == intel_crtc->config.pipe_src_w) &&
+		    (new_height == intel_crtc->config.pipe_src_h))
+			return 0;
+
+		if (((adjusted_mode->crtc_hdisplay != new_width) ||
+		     (adjusted_mode->crtc_vdisplay != new_height)) &&
+		    ((HAS_PCH_SPLIT(dev) &&
+			(!intel_crtc->config.pch_pfit.enabled)) ||
+		     (!HAS_PCH_SPLIT(dev) &&
+			(!intel_crtc->config.gmch_pfit.control)))) {
+			DRM_ERROR("PIPESRC mismatch with Pipe timings & PF is disabled\n");
+			return -EINVAL;
+		}
+
+		intel_crtc->config.pipe_src_w = new_width;
+		intel_crtc->config.pipe_src_h = new_height;
+
+		intel_crtc->config.requested_mode.hdisplay = new_width;
+		intel_crtc->config.requested_mode.vdisplay = new_height;
+
+		crtc->mode.hdisplay = new_width;
+		crtc->mode.vdisplay = new_height;
+
+		/* pipesrc controls the size that is scaled from, which should
+		* always be the user's requested size.
+		*/
+		I915_WRITE(PIPESRC(intel_crtc->pipe),
+			((intel_crtc->config.pipe_src_w - 1) << 16) |
+			 (intel_crtc->config.pipe_src_h - 1));
+
+		return 0;
+	}
+
 	return ret;
 }
 
@@ -10586,6 +10638,15 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
 
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
+
+	if (!dev_priv->input_size_property)
+		dev_priv->input_size_property =
+			drm_property_create_range(dev, 0, "input size", 0, 0xFFFFFFFF);
+
+	if (dev_priv->input_size_property)
+		drm_object_attach_property(&intel_crtc->base.base,
+					   dev_priv->input_size_property,
+					   0);
 }
 
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
-- 
1.8.5.2

[PATCH v4 2/4] drm/i915: New drm crtc property for varying the Pipe Src 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 (without modeset) the frame buffers
of different resolutions

v2: Added a check to fail the set property call if Panel fitter is
disabled & new PIPESRC programming do not match with PIPE timings.
Removed the pitch mismatch check on frame buffer, when being flipped.
This is currently done only for VLV/HSW.

v3: Modified the check, added in v2, to consider the platforms having
Split PCH.

v4: Refactored based on latest codebase.
Used 'UINT_MAX' macro in place of constant.

Testcase: igt/kms_panel_fitter_test

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  6 +++
 drivers/gpu/drm/i915/intel_display.c | 76 +++++++++++++++++++++++++++++++++++-
 2 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7d6acb4..104a232 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1441,6 +1441,12 @@ struct drm_i915_private {
 	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
 
+	/*
+	 * Property to dynamically vary the size of the
+	 * PIPESRC or Panel fitter input size
+	 */
+	struct drm_property *input_size_property;
+
 	uint32_t hw_context_size;
 	struct list_head context_list;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f26be4e..5484ae2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8902,8 +8902,18 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	 * Note that pitch changes could also affect these register.
 	 */
 	if (INTEL_INFO(dev)->gen > 3 &&
-	    (fb->offsets[0] != crtc->primary->fb->offsets[0] ||
-	     fb->pitches[0] != crtc->primary->fb->pitches[0]))
+	    (fb->offsets[0] != crtc->primary->fb->offsets[0]))
+		return -EINVAL;
+
+	/*
+	 * Bypassing the fb pitch check for VLV/HSW, as purportedly there
+	 * is a dynamic flip support in VLV/HSW. This will allow to
+	 * flip fbs of different resolutions without doing a modeset.
+	 * TBD, confirm the same for other newer gen platforms also.
+	 */
+	if (INTEL_INFO(dev)->gen > 3 &&
+	    !IS_VALLEYVIEW(dev) && !IS_HASWELL(dev) &&
+	    (fb->pitches[0] != crtc->primary->fb->pitches[0]))
 		return -EINVAL;
 
 	if (i915_terminally_wedged(&dev_priv->gpu_error))
@@ -10422,11 +10432,71 @@ out_config:
 	return ret;
 }
 
+static void intel_create_crtc_properties(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	if (!dev_priv->input_size_property)
+		dev_priv->input_size_property =
+			drm_property_create_range(dev, 0, "input size",
+						  0, UINT_MAX);
+
+	if (dev_priv->input_size_property)
+		drm_object_attach_property(&intel_crtc->base.base,
+					   dev_priv->input_size_property,
+					   0);
+}
+
 static int intel_crtc_set_property(struct drm_crtc *crtc,
 		struct drm_property *property, uint64_t val)
 {
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int ret = -ENOENT;
 
+	if (property == dev_priv->input_size_property) {
+		int new_width = (int)((val >> 16) & 0xffff);
+		int new_height = (int)(val & 0xffff);
+		const struct drm_display_mode *adjusted_mode =
+				&intel_crtc->config.adjusted_mode;
+
+		if ((new_width == intel_crtc->config.pipe_src_w) &&
+		    (new_height == intel_crtc->config.pipe_src_h))
+			return 0;
+
+		if (((adjusted_mode->crtc_hdisplay != new_width) ||
+		     (adjusted_mode->crtc_vdisplay != new_height)) &&
+		    ((HAS_PCH_SPLIT(dev) &&
+			(!intel_crtc->config.pch_pfit.enabled)) ||
+		     (!HAS_PCH_SPLIT(dev) &&
+			(!intel_crtc->config.gmch_pfit.control)))) {
+			DRM_ERROR("PIPESRC mismatch with Pipe timings &"
+				  "PF is disabled\n");
+			return -EINVAL;
+		}
+
+		intel_crtc->config.pipe_src_w = new_width;
+		intel_crtc->config.pipe_src_h = new_height;
+
+		intel_crtc->config.requested_mode.hdisplay = new_width;
+		intel_crtc->config.requested_mode.vdisplay = new_height;
+
+		crtc->mode.hdisplay = new_width;
+		crtc->mode.vdisplay = new_height;
+
+		/* pipesrc controls the size that is scaled from, which should
+		* always be the user's requested size.
+		*/
+		I915_WRITE(PIPESRC(intel_crtc->pipe),
+			((intel_crtc->config.pipe_src_w - 1) << 16) |
+			 (intel_crtc->config.pipe_src_h - 1));
+
+		return 0;
+	}
+
 	return ret;
 }
 
@@ -10577,6 +10647,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
 
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
+
+	intel_create_crtc_properties(&intel_crtc->base);
 }
 
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
-- 
1.9.2

[PATCH 4/4] drm/i915: Changed the return type from 'void', so as to return an error

[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 the 'border' property
is being set, if the configuation is not valid.
This addresses the review comment from Ville, given on the previous patch.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  5 +++--
 drivers/gpu/drm/i915/intel_dp.c      | 15 +++++++++------
 drivers/gpu/drm/i915/intel_drv.h     |  6 +++---
 drivers/gpu/drm/i915/intel_lvds.c    | 11 ++++++-----
 drivers/gpu/drm/i915/intel_panel.c   | 28 ++++++++++++++++------------
 5 files changed, 37 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8ff99bf..d7303be 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10018,9 +10018,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 b50b170..ad108e2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -778,12 +778,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 f00d075..bed4941 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -704,7 +704,7 @@ void intel_mark_busy(struct drm_device *dev);
 void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 			struct intel_ring_buffer *ring);
 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_update_dpms(struct drm_crtc *crtc);
 void intel_encoder_destroy(struct drm_encoder *encoder);
 void intel_connector_dpms(struct drm_connector *, int mode);
@@ -888,10 +888,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(struct intel_connector *connector, u32 level,
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 1b1541d..e0ef79d 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -307,12 +307,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 3dbb774..748c98c 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -57,7 +57,7 @@ intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
 	drm_mode_set_crtcinfo(adjusted_mode, 0);
 }
 
-void
+bool
 intel_pch_manual_panel_fitting(struct intel_crtc *intel_crtc,
 			struct intel_crtc_config *pipe_config)
 {
@@ -89,7 +89,7 @@ intel_pch_manual_panel_fitting(struct intel_crtc *intel_crtc,
 
 	if ((dst_width == 0) || (dst_height == 0)) {
 		DRM_ERROR("Invalid border size input\n");
-		return;
+		return false;
 	}
 
 	pf_horizontal_ratio = panel_fitter_scaling(src_width, dst_width);
@@ -97,10 +97,10 @@ intel_pch_manual_panel_fitting(struct intel_crtc *intel_crtc,
 
 	if (pf_horizontal_ratio > MAX_DOWNSCALE_RATIO) {
 		DRM_ERROR("width is too small\n");
-		return;
+		return false;
 	} else if (pf_vertical_ratio > MAX_DOWNSCALE_RATIO) {
 		DRM_ERROR("height is too small\n");
-		return;
+		return false;
 	}
 
 	x = intel_crtc->border[PANEL_BORDER_LEFT];
@@ -109,10 +109,11 @@ intel_pch_manual_panel_fitting(struct intel_crtc *intel_crtc,
 	pipe_config->pch_pfit.pos = (x << 16) | y;
 	pipe_config->pch_pfit.size = (dst_width << 16) | dst_height;
 	pipe_config->pch_pfit.enabled = pipe_config->pch_pfit.size != 0;
+	return true;
 }
 
 /* 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)
@@ -180,13 +181,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
@@ -353,7 +355,7 @@ static void i9xx_scale_aspect(struct intel_crtc_config *pipe_config,
 	}
 }
 
-void intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
+bool intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
 				     struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = intel_crtc->base.dev;
@@ -385,7 +387,7 @@ void intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
 
 	if ((dst_width == 0) || (dst_height == 0)) {
 		DRM_ERROR("Invalid border size input\n");
-		return;
+		return false;
 	}
 
 	pf_horizontal_ratio = panel_fitter_scaling(src_width, dst_width);
@@ -393,10 +395,10 @@ void intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
 
 	if (pf_horizontal_ratio > MAX_DOWNSCALE_RATIO) {
 		DRM_ERROR("width is too small\n");
-		return;
+		return false;
 	} else if (pf_vertical_ratio > MAX_DOWNSCALE_RATIO) {
 		DRM_ERROR("height is too small\n");
-		return;
+		return false;
 	}
 
 	if (dst_width != tot_width)
@@ -430,9 +432,10 @@ void intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
 
 	pipe_config->gmch_pfit.control = pfit_control;
 	pipe_config->gmch_pfit.lvds_border_bits = border;
+	return true;
 }
 
-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)
 {
@@ -490,7 +493,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 */
@@ -512,6 +515,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;
 }
 
 static u32 intel_panel_compute_brightness(struct intel_connector *connector,
-- 
1.9.2

Re: [PATCH 4/4] drm/i915: Changed the return type from 'void', so as to return an error

[Ville Syrjälä]

On Sun, Apr 20, 2014 at 04:22:48PM +0530, akash.goel@intel.com wrote:
> 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 the 'border' property
> is being set, if the configuation is not valid.
> This addresses the review comment from Ville, given on the previous patch.
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>

Please reorder the patches so that this patch comes before you add the
manual panel fitting stuff. Otherwise it's hard to evaluate the error
paths in the new code.

> ---
>  drivers/gpu/drm/i915/intel_display.c |  5 +++--
>  drivers/gpu/drm/i915/intel_dp.c      | 15 +++++++++------
>  drivers/gpu/drm/i915/intel_drv.h     |  6 +++---
>  drivers/gpu/drm/i915/intel_lvds.c    | 11 ++++++-----
>  drivers/gpu/drm/i915/intel_panel.c   | 28 ++++++++++++++++------------
>  5 files changed, 37 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8ff99bf..d7303be 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10018,9 +10018,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 b50b170..ad108e2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -778,12 +778,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 f00d075..bed4941 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -704,7 +704,7 @@ void intel_mark_busy(struct drm_device *dev);
>  void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
>  			struct intel_ring_buffer *ring);
>  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_update_dpms(struct drm_crtc *crtc);
>  void intel_encoder_destroy(struct drm_encoder *encoder);
>  void intel_connector_dpms(struct drm_connector *, int mode);
> @@ -888,10 +888,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(struct intel_connector *connector, u32 level,
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 1b1541d..e0ef79d 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -307,12 +307,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 3dbb774..748c98c 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -57,7 +57,7 @@ intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
>  	drm_mode_set_crtcinfo(adjusted_mode, 0);
>  }
>  
> -void
> +bool
>  intel_pch_manual_panel_fitting(struct intel_crtc *intel_crtc,
>  			struct intel_crtc_config *pipe_config)
>  {
> @@ -89,7 +89,7 @@ intel_pch_manual_panel_fitting(struct intel_crtc *intel_crtc,
>  
>  	if ((dst_width == 0) || (dst_height == 0)) {
>  		DRM_ERROR("Invalid border size input\n");
> -		return;
> +		return false;
>  	}
>  
>  	pf_horizontal_ratio = panel_fitter_scaling(src_width, dst_width);
> @@ -97,10 +97,10 @@ intel_pch_manual_panel_fitting(struct intel_crtc *intel_crtc,
>  
>  	if (pf_horizontal_ratio > MAX_DOWNSCALE_RATIO) {
>  		DRM_ERROR("width is too small\n");
> -		return;
> +		return false;
>  	} else if (pf_vertical_ratio > MAX_DOWNSCALE_RATIO) {
>  		DRM_ERROR("height is too small\n");
> -		return;
> +		return false;
>  	}
>  
>  	x = intel_crtc->border[PANEL_BORDER_LEFT];
> @@ -109,10 +109,11 @@ intel_pch_manual_panel_fitting(struct intel_crtc *intel_crtc,
>  	pipe_config->pch_pfit.pos = (x << 16) | y;
>  	pipe_config->pch_pfit.size = (dst_width << 16) | dst_height;
>  	pipe_config->pch_pfit.enabled = pipe_config->pch_pfit.size != 0;
> +	return true;
>  }
>  
>  /* 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)
> @@ -180,13 +181,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
> @@ -353,7 +355,7 @@ static void i9xx_scale_aspect(struct intel_crtc_config *pipe_config,
>  	}
>  }
>  
> -void intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
> +bool intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
>  				     struct intel_crtc_config *pipe_config)
>  {
>  	struct drm_device *dev = intel_crtc->base.dev;
> @@ -385,7 +387,7 @@ void intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
>  
>  	if ((dst_width == 0) || (dst_height == 0)) {
>  		DRM_ERROR("Invalid border size input\n");
> -		return;
> +		return false;
>  	}
>  
>  	pf_horizontal_ratio = panel_fitter_scaling(src_width, dst_width);
> @@ -393,10 +395,10 @@ void intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
>  
>  	if (pf_horizontal_ratio > MAX_DOWNSCALE_RATIO) {
>  		DRM_ERROR("width is too small\n");
> -		return;
> +		return false;
>  	} else if (pf_vertical_ratio > MAX_DOWNSCALE_RATIO) {
>  		DRM_ERROR("height is too small\n");
> -		return;
> +		return false;
>  	}
>  
>  	if (dst_width != tot_width)
> @@ -430,9 +432,10 @@ void intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
>  
>  	pipe_config->gmch_pfit.control = pfit_control;
>  	pipe_config->gmch_pfit.lvds_border_bits = border;
> +	return true;
>  }
>  
> -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)
>  {
> @@ -490,7 +493,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 */
> @@ -512,6 +515,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;
>  }
>  
>  static u32 intel_panel_compute_brightness(struct intel_connector *connector,
> -- 
> 1.9.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 4/4] drm/i915: Changed the return type from 'void', so as to return an error

[Akash Goel]

On Tue, 2014-04-29 at 16:33 +0300, Ville Syrjälä wrote:
> On Sun, Apr 20, 2014 at 04:22:48PM +0530, akash.goel@intel.com wrote:
> > 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 the 'border' property
> > is being set, if the configuation is not valid.
> > This addresses the review comment from Ville, given on the previous patch.
> > 
> > Signed-off-by: Akash Goel <akash.goel@intel.com>
> 
> Please reorder the patches so that this patch comes before you add the
> manual panel fitting stuff. Otherwise it's hard to evaluate the error
> paths in the new code.
> 

Fine, will reorder the patch sequence so that this first comes first.

Best Regards
Akash



> > ---
> >  drivers/gpu/drm/i915/intel_display.c |  5 +++--
> >  drivers/gpu/drm/i915/intel_dp.c      | 15 +++++++++------
> >  drivers/gpu/drm/i915/intel_drv.h     |  6 +++---
> >  drivers/gpu/drm/i915/intel_lvds.c    | 11 ++++++-----
> >  drivers/gpu/drm/i915/intel_panel.c   | 28 ++++++++++++++++------------
> >  5 files changed, 37 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 8ff99bf..d7303be 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10018,9 +10018,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 b50b170..ad108e2 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -778,12 +778,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 f00d075..bed4941 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -704,7 +704,7 @@ void intel_mark_busy(struct drm_device *dev);
> >  void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
> >  			struct intel_ring_buffer *ring);
> >  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_update_dpms(struct drm_crtc *crtc);
> >  void intel_encoder_destroy(struct drm_encoder *encoder);
> >  void intel_connector_dpms(struct drm_connector *, int mode);
> > @@ -888,10 +888,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(struct intel_connector *connector, u32 level,
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index 1b1541d..e0ef79d 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -307,12 +307,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 3dbb774..748c98c 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -57,7 +57,7 @@ intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
> >  	drm_mode_set_crtcinfo(adjusted_mode, 0);
> >  }
> >  
> > -void
> > +bool
> >  intel_pch_manual_panel_fitting(struct intel_crtc *intel_crtc,
> >  			struct intel_crtc_config *pipe_config)
> >  {
> > @@ -89,7 +89,7 @@ intel_pch_manual_panel_fitting(struct intel_crtc *intel_crtc,
> >  
> >  	if ((dst_width == 0) || (dst_height == 0)) {
> >  		DRM_ERROR("Invalid border size input\n");
> > -		return;
> > +		return false;
> >  	}
> >  
> >  	pf_horizontal_ratio = panel_fitter_scaling(src_width, dst_width);
> > @@ -97,10 +97,10 @@ intel_pch_manual_panel_fitting(struct intel_crtc *intel_crtc,
> >  
> >  	if (pf_horizontal_ratio > MAX_DOWNSCALE_RATIO) {
> >  		DRM_ERROR("width is too small\n");
> > -		return;
> > +		return false;
> >  	} else if (pf_vertical_ratio > MAX_DOWNSCALE_RATIO) {
> >  		DRM_ERROR("height is too small\n");
> > -		return;
> > +		return false;
> >  	}
> >  
> >  	x = intel_crtc->border[PANEL_BORDER_LEFT];
> > @@ -109,10 +109,11 @@ intel_pch_manual_panel_fitting(struct intel_crtc *intel_crtc,
> >  	pipe_config->pch_pfit.pos = (x << 16) | y;
> >  	pipe_config->pch_pfit.size = (dst_width << 16) | dst_height;
> >  	pipe_config->pch_pfit.enabled = pipe_config->pch_pfit.size != 0;
> > +	return true;
> >  }
> >  
> >  /* 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)
> > @@ -180,13 +181,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
> > @@ -353,7 +355,7 @@ static void i9xx_scale_aspect(struct intel_crtc_config *pipe_config,
> >  	}
> >  }
> >  
> > -void intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
> > +bool intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
> >  				     struct intel_crtc_config *pipe_config)
> >  {
> >  	struct drm_device *dev = intel_crtc->base.dev;
> > @@ -385,7 +387,7 @@ void intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
> >  
> >  	if ((dst_width == 0) || (dst_height == 0)) {
> >  		DRM_ERROR("Invalid border size input\n");
> > -		return;
> > +		return false;
> >  	}
> >  
> >  	pf_horizontal_ratio = panel_fitter_scaling(src_width, dst_width);
> > @@ -393,10 +395,10 @@ void intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
> >  
> >  	if (pf_horizontal_ratio > MAX_DOWNSCALE_RATIO) {
> >  		DRM_ERROR("width is too small\n");
> > -		return;
> > +		return false;
> >  	} else if (pf_vertical_ratio > MAX_DOWNSCALE_RATIO) {
> >  		DRM_ERROR("height is too small\n");
> > -		return;
> > +		return false;
> >  	}
> >  
> >  	if (dst_width != tot_width)
> > @@ -430,9 +432,10 @@ void intel_gmch_manual_panel_fitting(struct intel_crtc *intel_crtc,
> >  
> >  	pipe_config->gmch_pfit.control = pfit_control;
> >  	pipe_config->gmch_pfit.lvds_border_bits = border;
> > +	return true;
> >  }
> >  
> > -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)
> >  {
> > @@ -490,7 +493,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 */
> > @@ -512,6 +515,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;
> >  }
> >  
> >  static u32 intel_panel_compute_brightness(struct intel_connector *connector,
> > -- 
> > 1.9.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


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

Re: [PATCH v4 2/4] drm/i915: New drm crtc property for varying the Pipe Src size

[Ville Syrjälä]

On Sun, Apr 20, 2014 at 04:14:18PM +0530, akash.goel@intel.com wrote:
> 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 (without modeset) the frame buffers
> of different resolutions
> 
> v2: Added a check to fail the set property call if Panel fitter is
> disabled & new PIPESRC programming do not match with PIPE timings.
> Removed the pitch mismatch check on frame buffer, when being flipped.
> This is currently done only for VLV/HSW.
> 
> v3: Modified the check, added in v2, to consider the platforms having
> Split PCH.
> 
> v4: Refactored based on latest codebase.
> Used 'UINT_MAX' macro in place of constant.
> 
> Testcase: igt/kms_panel_fitter_test
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  6 +++
>  drivers/gpu/drm/i915/intel_display.c | 76 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 80 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7d6acb4..104a232 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1441,6 +1441,12 @@ struct drm_i915_private {
>  	struct drm_property *broadcast_rgb_property;
>  	struct drm_property *force_audio_property;
>  
> +	/*
> +	 * Property to dynamically vary the size of the
> +	 * PIPESRC or Panel fitter input size
> +	 */
> +	struct drm_property *input_size_property;
> +
>  	uint32_t hw_context_size;
>  	struct list_head context_list;
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f26be4e..5484ae2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8902,8 +8902,18 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	 * Note that pitch changes could also affect these register.
>  	 */
>  	if (INTEL_INFO(dev)->gen > 3 &&
> -	    (fb->offsets[0] != crtc->primary->fb->offsets[0] ||
> -	     fb->pitches[0] != crtc->primary->fb->pitches[0]))
> +	    (fb->offsets[0] != crtc->primary->fb->offsets[0]))
> +		return -EINVAL;
> +
> +	/*
> +	 * Bypassing the fb pitch check for VLV/HSW, as purportedly there
> +	 * is a dynamic flip support in VLV/HSW. This will allow to
> +	 * flip fbs of different resolutions without doing a modeset.
> +	 * TBD, confirm the same for other newer gen platforms also.
> +	 */
> +	if (INTEL_INFO(dev)->gen > 3 &&
> +	    !IS_VALLEYVIEW(dev) && !IS_HASWELL(dev) &&
> +	    (fb->pitches[0] != crtc->primary->fb->pitches[0]))

NAK. Please read the comment above the check to understand why we can't
just change the stride here. I guess on HSW+ it might be possible since
it uses the x/y offsets even with linear FBs so stride changes don't
affect the offset. Also such changes sould be in separate patches
anyway.

I guess one option would be update the linear offset with LRI, but
since the offset register gets latched independently of the DSPSURF
write the changes aren't guaranteed to happen atomically. Also on
VLV LRI to display registers is apparently busted.

If you do find a way to make this work on some platforms, then we
need to have a test for it to make sure it does the right thing.

>  		return -EINVAL;
>  
>  	if (i915_terminally_wedged(&dev_priv->gpu_error))
> @@ -10422,11 +10432,71 @@ out_config:
>  	return ret;
>  }
>  
> +static void intel_create_crtc_properties(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +	if (!dev_priv->input_size_property)
> +		dev_priv->input_size_property =
> +			drm_property_create_range(dev, 0, "input size",
> +						  0, UINT_MAX);

I think people would be happier with separate width/height properties
instead of sticking both into one property.

Also this sounds like a generally useful property for hardware any which
has some kind of full crtc scaler. So maybe it should be in drm core.
Also what ever happended to the property documentation effort. Did the
patch get in, or was it rejected, or is it in limbo?

> +
> +	if (dev_priv->input_size_property)
> +		drm_object_attach_property(&intel_crtc->base.base,
> +					   dev_priv->input_size_property,
> +					   0);
> +}
> +
>  static int intel_crtc_set_property(struct drm_crtc *crtc,
>  		struct drm_property *property, uint64_t val)
>  {
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int ret = -ENOENT;
>  
> +	if (property == dev_priv->input_size_property) {
> +		int new_width = (int)((val >> 16) & 0xffff);
> +		int new_height = (int)(val & 0xffff);
> +		const struct drm_display_mode *adjusted_mode =
> +				&intel_crtc->config.adjusted_mode;
> +
> +		if ((new_width == intel_crtc->config.pipe_src_w) &&
> +		    (new_height == intel_crtc->config.pipe_src_h))
> +			return 0;
> +
> +		if (((adjusted_mode->crtc_hdisplay != new_width) ||
> +		     (adjusted_mode->crtc_vdisplay != new_height)) &&
> +		    ((HAS_PCH_SPLIT(dev) &&
> +			(!intel_crtc->config.pch_pfit.enabled)) ||
> +		     (!HAS_PCH_SPLIT(dev) &&
> +			(!intel_crtc->config.gmch_pfit.control)))) {
> +			DRM_ERROR("PIPESRC mismatch with Pipe timings &"
> +				  "PF is disabled\n");
> +			return -EINVAL;

This raises an interesting question. If we can't toggle the pfit on/off
dynamically should we have some way to force the pfit on at modeset
time (even w/ 1:1 scaling) so that it's possibly to change the scaling
dynamically w/o a full modeset.

One idea might be to treat 0 as a special value for this property. If
both width and height are 0 we do the old thing and disable pfit if
the scaling based on user mode vs. adjusted mode is 1:1, but if this
property has anything but 0, always enable pfit. Thoughts anyone?

> +		}
> +
> +		intel_crtc->config.pipe_src_w = new_width;
> +		intel_crtc->config.pipe_src_h = new_height;

This looks like it needs more error checks to make sure the scaling
ratio stays within acceptable limits.

Also we need to recheck primary plane FB size vs. new PIPESRC.

And we need to reclip sprites and cursors.

This is starting to be quite a lot of stuff. So for the initial
implementation maye it's easier to just do a full modeset here. And
once that's done we can figure out how to avoid the modeset. We should
anyway be moving towards a world where we compute the new config,
validate it, and then figure out if we can get there w/o a full modeset.
So maybe we need to start writing the code to just that and not limit
ourselves to implementing special code for specific properties.

> +
> +		intel_crtc->config.requested_mode.hdisplay = new_width;
> +		intel_crtc->config.requested_mode.vdisplay = new_height;
> +
> +		crtc->mode.hdisplay = new_width;
> +		crtc->mode.vdisplay = new_height;

Hmm. This will cause problems if we have to do a modeset w/o a new user
specified mode (eg. atomic modeset where change on another pipe requires
us to perform modeset on this pipe as well).

So I think we need to leave the modes alone. One thing to worry about is
old userspace that doesn't realize that changes in this property would
have ramifications on the crtc viewport. I'm not sure there's anything
sane we can do but let old userspace shoot itself in the foot if it plays
with this property.

So at modeset time we need to populate pipe_src_{w,h} from the user mode
first, and then overwrite those with the values from this property if
the property value != 0. Assuming we go with my "0 is special" plan.

> +
> +		/* pipesrc controls the size that is scaled from, which should
> +		* always be the user's requested size.
> +		*/
> +		I915_WRITE(PIPESRC(intel_crtc->pipe),
> +			((intel_crtc->config.pipe_src_w - 1) << 16) |
> +			 (intel_crtc->config.pipe_src_h - 1));

And do we need to reprogram the pfit on gmch?

> +
> +		return 0;
> +	}
> +
>  	return ret;
>  }
>  
> @@ -10577,6 +10647,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
>  
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
> +
> +	intel_create_crtc_properties(&intel_crtc->base);
>  }
>  
>  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
> -- 
> 1.9.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v4 2/4] drm/i915: New drm crtc property for varying the Pipe Src size

[Daniel Vetter]

On Tue, Apr 29, 2014 at 05:06:59PM +0300, Ville Syrjälä wrote:
> On Sun, Apr 20, 2014 at 04:14:18PM +0530, akash.goel@intel.com wrote:
> > 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 (without modeset) the frame buffers
> > of different resolutions
> > 
> > v2: Added a check to fail the set property call if Panel fitter is
> > disabled & new PIPESRC programming do not match with PIPE timings.
> > Removed the pitch mismatch check on frame buffer, when being flipped.
> > This is currently done only for VLV/HSW.
> > 
> > v3: Modified the check, added in v2, to consider the platforms having
> > Split PCH.
> > 
> > v4: Refactored based on latest codebase.
> > Used 'UINT_MAX' macro in place of constant.
> > 
> > Testcase: igt/kms_panel_fitter_test
> > 
> > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  6 +++
> >  drivers/gpu/drm/i915/intel_display.c | 76 +++++++++++++++++++++++++++++++++++-
> >  2 files changed, 80 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 7d6acb4..104a232 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1441,6 +1441,12 @@ struct drm_i915_private {
> >  	struct drm_property *broadcast_rgb_property;
> >  	struct drm_property *force_audio_property;
> >  
> > +	/*
> > +	 * Property to dynamically vary the size of the
> > +	 * PIPESRC or Panel fitter input size
> > +	 */
> > +	struct drm_property *input_size_property;
> > +
> >  	uint32_t hw_context_size;
> >  	struct list_head context_list;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f26be4e..5484ae2 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8902,8 +8902,18 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >  	 * Note that pitch changes could also affect these register.
> >  	 */
> >  	if (INTEL_INFO(dev)->gen > 3 &&
> > -	    (fb->offsets[0] != crtc->primary->fb->offsets[0] ||
> > -	     fb->pitches[0] != crtc->primary->fb->pitches[0]))
> > +	    (fb->offsets[0] != crtc->primary->fb->offsets[0]))
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * Bypassing the fb pitch check for VLV/HSW, as purportedly there
> > +	 * is a dynamic flip support in VLV/HSW. This will allow to
> > +	 * flip fbs of different resolutions without doing a modeset.
> > +	 * TBD, confirm the same for other newer gen platforms also.
> > +	 */
> > +	if (INTEL_INFO(dev)->gen > 3 &&
> > +	    !IS_VALLEYVIEW(dev) && !IS_HASWELL(dev) &&
> > +	    (fb->pitches[0] != crtc->primary->fb->pitches[0]))
> 
> NAK. Please read the comment above the check to understand why we can't
> just change the stride here. I guess on HSW+ it might be possible since
> it uses the x/y offsets even with linear FBs so stride changes don't
> affect the offset. Also such changes sould be in separate patches
> anyway.
> 
> I guess one option would be update the linear offset with LRI, but
> since the offset register gets latched independently of the DSPSURF
> write the changes aren't guaranteed to happen atomically. Also on
> VLV LRI to display registers is apparently busted.
> 
> If you do find a way to make this work on some platforms, then we
> need to have a test for it to make sure it does the right thing.

Note that even on hsw we update the linear offset if it's out of range for
the x/y tiled offsets. Which can happen with very wide/high virtual
desktops.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v4 2/4] drm/i915: New drm crtc property for varying the Pipe Src size

[Akash Goel]

On Tue, 2014-04-29 at 17:06 +0300, Ville Syrjälä wrote:
> On Sun, Apr 20, 2014 at 04:14:18PM +0530, akash.goel@intel.com wrote:
> > 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 (without modeset) the frame buffers
> > of different resolutions
> > 
> > v2: Added a check to fail the set property call if Panel fitter is
> > disabled & new PIPESRC programming do not match with PIPE timings.
> > Removed the pitch mismatch check on frame buffer, when being flipped.
> > This is currently done only for VLV/HSW.
> > 
> > v3: Modified the check, added in v2, to consider the platforms having
> > Split PCH.
> > 
> > v4: Refactored based on latest codebase.
> > Used 'UINT_MAX' macro in place of constant.
> > 
> > Testcase: igt/kms_panel_fitter_test
> > 
> > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  6 +++
> >  drivers/gpu/drm/i915/intel_display.c | 76 +++++++++++++++++++++++++++++++++++-
> >  2 files changed, 80 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 7d6acb4..104a232 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1441,6 +1441,12 @@ struct drm_i915_private {
> >  	struct drm_property *broadcast_rgb_property;
> >  	struct drm_property *force_audio_property;
> >  
> > +	/*
> > +	 * Property to dynamically vary the size of the
> > +	 * PIPESRC or Panel fitter input size
> > +	 */
> > +	struct drm_property *input_size_property;
> > +
> >  	uint32_t hw_context_size;
> >  	struct list_head context_list;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f26be4e..5484ae2 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8902,8 +8902,18 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >  	 * Note that pitch changes could also affect these register.
> >  	 */
> >  	if (INTEL_INFO(dev)->gen > 3 &&
> > -	    (fb->offsets[0] != crtc->primary->fb->offsets[0] ||
> > -	     fb->pitches[0] != crtc->primary->fb->pitches[0]))
> > +	    (fb->offsets[0] != crtc->primary->fb->offsets[0]))
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * Bypassing the fb pitch check for VLV/HSW, as purportedly there
> > +	 * is a dynamic flip support in VLV/HSW. This will allow to
> > +	 * flip fbs of different resolutions without doing a modeset.
> > +	 * TBD, confirm the same for other newer gen platforms also.
> > +	 */
> > +	if (INTEL_INFO(dev)->gen > 3 &&
> > +	    !IS_VALLEYVIEW(dev) && !IS_HASWELL(dev) &&
> > +	    (fb->pitches[0] != crtc->primary->fb->pitches[0]))
> 
> NAK. Please read the comment above the check to understand why we can't
> just change the stride here. I guess on HSW+ it might be possible since
> it uses the x/y offsets even with linear FBs so stride changes don't
> affect the offset. Also such changes sould be in separate patches
> anyway.
> 

Actually as per the Bspec, probably the pitch/stride can be changed for
synchronous flips. 
As per the description of MI_DISPLAY_FLIP from the Bspec
"15:6 Display Buffer Pitch
For Synchronous Flips and Stereo 3D Flips only, this field specifies the
64-byte aligned pitch of the new display buffer. For Asynchronous Flips,
this parameter is programmed so that all the flips in a flip chain
should maintain the same pitch as programmed with the last synchronous
flip or stereo 3D flip or direct through MMIO.
The Display Buffer Pitch and Tile parameter and Decrypt Request fields
cannot be changed for asynchronous flips (i.e., the new buffer must have
the same pitch/tile format as the previous buffer).
Asynch flips are Supported on X-Tiled Frame buffers only."

> I guess one option would be update the linear offset with LRI, but
> since the offset register gets latched independently of the DSPSURF
> write the changes aren't guaranteed to happen atomically. Also on
> VLV LRI to display registers is apparently busted.
> 
> If you do find a way to make this work on some platforms, then we
> need to have a test for it to make sure it does the right thing.
> 

Actually we have been using MMIO based flips on BYT, instead of command
streamer based flips, so the DSPSTRIDE & DSPTILEOFF registers are being
written along with DSPSURF on every flip. 
As per the description of DSPSTRIDE register in Bspec, 
"This register is updated either through a command packet passed through
the command stream or writes to this register. When it is desired to
update both this and the start register, 
the stride register must be written first because the write to the start
register is the trigger that causes the update of both registers on the
next VBLANK event"
So once we switch to MMIO flip, then it shall be taken care.


> >  		return -EINVAL;
> >  
> >  	if (i915_terminally_wedged(&dev_priv->gpu_error))
> > @@ -10422,11 +10432,71 @@ out_config:
> >  	return ret;
> >  }
> >  
> > +static void intel_create_crtc_properties(struct drm_crtc *crtc)
> > +{
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +
> > +	if (!dev_priv->input_size_property)
> > +		dev_priv->input_size_property =
> > +			drm_property_create_range(dev, 0, "input size",
> > +						  0, UINT_MAX);
> 
> I think people would be happier with separate width/height properties
> instead of sticking both into one property.
> 
> Also this sounds like a generally useful property for hardware any which
> has some kind of full crtc scaler. So maybe it should be in drm core.
> Also what ever happended to the property documentation effort. Did the
> patch get in, or was it rejected, or is it in limbo?
> 
Fine will split this into 2 properties but will there be any limitation
in future if a single property is persisted with to specify both height
& width here.

Sagar has submitted the patch for property documentation and he got the
ACK also from the Laurent Pinchart, but I think Daniel has not done the
same. Please can you kindly once check with Daniel for this.

> > +
> > +	if (dev_priv->input_size_property)
> > +		drm_object_attach_property(&intel_crtc->base.base,
> > +					   dev_priv->input_size_property,
> > +					   0);
> > +}
> > +
> >  static int intel_crtc_set_property(struct drm_crtc *crtc,
> >  		struct drm_property *property, uint64_t val)
> >  {
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	int ret = -ENOENT;
> >  
> > +	if (property == dev_priv->input_size_property) {
> > +		int new_width = (int)((val >> 16) & 0xffff);
> > +		int new_height = (int)(val & 0xffff);
> > +		const struct drm_display_mode *adjusted_mode =
> > +				&intel_crtc->config.adjusted_mode;
> > +
> > +		if ((new_width == intel_crtc->config.pipe_src_w) &&
> > +		    (new_height == intel_crtc->config.pipe_src_h))
> > +			return 0;
> > +
> > +		if (((adjusted_mode->crtc_hdisplay != new_width) ||
> > +		     (adjusted_mode->crtc_vdisplay != new_height)) &&
> > +		    ((HAS_PCH_SPLIT(dev) &&
> > +			(!intel_crtc->config.pch_pfit.enabled)) ||
> > +		     (!HAS_PCH_SPLIT(dev) &&
> > +			(!intel_crtc->config.gmch_pfit.control)))) {
> > +			DRM_ERROR("PIPESRC mismatch with Pipe timings &"
> > +				  "PF is disabled\n");
> > +			return -EINVAL;
> 
> This raises an interesting question. If we can't toggle the pfit on/off
> dynamically should we have some way to force the pfit on at modeset
> time (even w/ 1:1 scaling) so that it's possibly to change the scaling
> dynamically w/o a full modeset.
> 
> One idea might be to treat 0 as a special value for this property. If
> both width and height are 0 we do the old thing and disable pfit if
> the scaling based on user mode vs. adjusted mode is 1:1, but if this
> property has anything but 0, always enable pfit. Thoughts anyone?
> 

Yes in order to be able to dynamically change the resolution of fb's
being flipped (without a modeset) first Panel fitter has to be enabled.

So 0 value of this property will considered as disabled and old
condition of enabling Panel fitter will be followed(requested mode !=
adjusted mode). 
But if this property is set to some value, and if Panel fitter is not
enabled already, it shall be enabled (which will require a modeset).
Can we do it like this when user sets a non zero value for this
property.
1. Check first if Panel fitter is enabled or not, and if not then enable
it forcefully bypassing the original condition.
2. Update the PIPESRC register later on, with the new values, in the
subsequent Page flip ioctl (and not in the set property call itself),
when FB of the new resolution will be passed.
 
> > +		}
> > +
> > +		intel_crtc->config.pipe_src_w = new_width;
> > +		intel_crtc->config.pipe_src_h = new_height;
> 
> This looks like it needs more error checks to make sure the scaling
> ratio stays within acceptable limits.
> 
> Also we need to recheck primary plane FB size vs. new PIPESRC.
> 
On setting this property,  FB's being subsequently flipped on primary
plane shall comply with the new Pipe SRC values.
And check for this, is already done by the drm_crtc_check_viewport
function in page flip ioctl.

> And we need to reclip sprites and cursors.

> 
> This is starting to be quite a lot of stuff. So for the initial
> implementation may be it's easier to just do a full modeset here. And
> once that's done we can figure out how to avoid the modeset. We should
> anyway be moving towards a world where we compute the new config,
> validate it, and then figure out if we can get there w/o a full modeset.
> So maybe we need to start writing the code to just that and not limit
> ourselves to implementing special code for specific properties.
> 

Sorry couldn't fully comprehend your last point here.

> > +
> > +		intel_crtc->config.requested_mode.hdisplay = new_width;
> > +		intel_crtc->config.requested_mode.vdisplay = new_height;
> > +
> > +		crtc->mode.hdisplay = new_width;
> > +		crtc->mode.vdisplay = new_height;
> 
> Hmm. This will cause problems if we have to do a modeset w/o a new user
> specified mode (eg. atomic modeset where change on another pipe requires
> us to perform modeset on this pipe as well).
> 
> So I think we need to leave the modes alone. One thing to worry about is
> old userspace that doesn't realize that changes in this property would
> have ramifications on the crtc viewport. I'm not sure there's anything
> sane we can do but let old userspace shoot itself in the foot if it plays
> with this property.
> 

Again sorry, but couldn't get your old user space point.
Should the 'hdisplay', 'vdisplay' fields in mode & pipe config
structures be also updated here, when this property is set or they shall
be left unchanged ?

> So at modeset time we need to populate pipe_src_{w,h} from the user mode
> first, and then overwrite those with the values from this property if
> the property value != 0. Assuming we go with my "0 is special" plan.
> 

If this property is once set by User, and then if it subsequently does a
modeset, then the 'fb' passed with that modeset call, shall conform to
which resolution, i.e to the mode passed by User or to the height &
width specified by the current value of this property ? Or User 

> > +
> > +		/* pipesrc controls the size that is scaled from, which should
> > +		* always be the user's requested size.
> > +		*/
> > +		I915_WRITE(PIPESRC(intel_crtc->pipe),
> > +			((intel_crtc->config.pipe_src_w - 1) << 16) |
> > +			 (intel_crtc->config.pipe_src_h - 1));
> 
> And do we need to reprogram the pfit on gmch?
> 
> > +
> > +		return 0;
> > +	}
> > +
> >  	return ret;
> >  }
> >  
> > @@ -10577,6 +10647,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> >  	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
> >  
> >  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
> > +
> > +	intel_create_crtc_properties(&intel_crtc->base);
> >  }
> >  
> >  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
> > -- 
> > 1.9.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


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

Re: [PATCH v4 2/4] drm/i915: New drm crtc property for varying the Pipe Src size

[Akash Goel]

Hi Ville,

Please could you respond to my last mail.
This will enable me to make further progress on this patch.

Bes Regards
Akash

On Sun, 2014-05-04 at 15:51 +0530, Akash Goel wrote:
> On Tue, 2014-04-29 at 17:06 +0300, Ville Syrjälä wrote:
> > On Sun, Apr 20, 2014 at 04:14:18PM +0530, akash.goel@intel.com wrote:
> > > 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 (without modeset) the frame buffers
> > > of different resolutions
> > > 
> > > v2: Added a check to fail the set property call if Panel fitter is
> > > disabled & new PIPESRC programming do not match with PIPE timings.
> > > Removed the pitch mismatch check on frame buffer, when being flipped.
> > > This is currently done only for VLV/HSW.
> > > 
> > > v3: Modified the check, added in v2, to consider the platforms having
> > > Split PCH.
> > > 
> > > v4: Refactored based on latest codebase.
> > > Used 'UINT_MAX' macro in place of constant.
> > > 
> > > Testcase: igt/kms_panel_fitter_test
> > > 
> > > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h      |  6 +++
> > >  drivers/gpu/drm/i915/intel_display.c | 76 +++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 80 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 7d6acb4..104a232 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1441,6 +1441,12 @@ struct drm_i915_private {
> > >  	struct drm_property *broadcast_rgb_property;
> > >  	struct drm_property *force_audio_property;
> > >  
> > > +	/*
> > > +	 * Property to dynamically vary the size of the
> > > +	 * PIPESRC or Panel fitter input size
> > > +	 */
> > > +	struct drm_property *input_size_property;
> > > +
> > >  	uint32_t hw_context_size;
> > >  	struct list_head context_list;
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index f26be4e..5484ae2 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -8902,8 +8902,18 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> > >  	 * Note that pitch changes could also affect these register.
> > >  	 */
> > >  	if (INTEL_INFO(dev)->gen > 3 &&
> > > -	    (fb->offsets[0] != crtc->primary->fb->offsets[0] ||
> > > -	     fb->pitches[0] != crtc->primary->fb->pitches[0]))
> > > +	    (fb->offsets[0] != crtc->primary->fb->offsets[0]))
> > > +		return -EINVAL;
> > > +
> > > +	/*
> > > +	 * Bypassing the fb pitch check for VLV/HSW, as purportedly there
> > > +	 * is a dynamic flip support in VLV/HSW. This will allow to
> > > +	 * flip fbs of different resolutions without doing a modeset.
> > > +	 * TBD, confirm the same for other newer gen platforms also.
> > > +	 */
> > > +	if (INTEL_INFO(dev)->gen > 3 &&
> > > +	    !IS_VALLEYVIEW(dev) && !IS_HASWELL(dev) &&
> > > +	    (fb->pitches[0] != crtc->primary->fb->pitches[0]))
> > 
> > NAK. Please read the comment above the check to understand why we can't
> > just change the stride here. I guess on HSW+ it might be possible since
> > it uses the x/y offsets even with linear FBs so stride changes don't
> > affect the offset. Also such changes sould be in separate patches
> > anyway.
> > 
> 
> Actually as per the Bspec, probably the pitch/stride can be changed for
> synchronous flips. 
> As per the description of MI_DISPLAY_FLIP from the Bspec
> "15:6 Display Buffer Pitch
> For Synchronous Flips and Stereo 3D Flips only, this field specifies the
> 64-byte aligned pitch of the new display buffer. For Asynchronous Flips,
> this parameter is programmed so that all the flips in a flip chain
> should maintain the same pitch as programmed with the last synchronous
> flip or stereo 3D flip or direct through MMIO.
> The Display Buffer Pitch and Tile parameter and Decrypt Request fields
> cannot be changed for asynchronous flips (i.e., the new buffer must have
> the same pitch/tile format as the previous buffer).
> Asynch flips are Supported on X-Tiled Frame buffers only."
> 
> > I guess one option would be update the linear offset with LRI, but
> > since the offset register gets latched independently of the DSPSURF
> > write the changes aren't guaranteed to happen atomically. Also on
> > VLV LRI to display registers is apparently busted.
> > 
> > If you do find a way to make this work on some platforms, then we
> > need to have a test for it to make sure it does the right thing.
> > 
> 
> Actually we have been using MMIO based flips on BYT, instead of command
> streamer based flips, so the DSPSTRIDE & DSPTILEOFF registers are being
> written along with DSPSURF on every flip. 
> As per the description of DSPSTRIDE register in Bspec, 
> "This register is updated either through a command packet passed through
> the command stream or writes to this register. When it is desired to
> update both this and the start register, 
> the stride register must be written first because the write to the start
> register is the trigger that causes the update of both registers on the
> next VBLANK event"
> So once we switch to MMIO flip, then it shall be taken care.
> 
> 
> > >  		return -EINVAL;
> > >  
> > >  	if (i915_terminally_wedged(&dev_priv->gpu_error))
> > > @@ -10422,11 +10432,71 @@ out_config:
> > >  	return ret;
> > >  }
> > >  
> > > +static void intel_create_crtc_properties(struct drm_crtc *crtc)
> > > +{
> > > +	struct drm_device *dev = crtc->dev;
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +
> > > +	if (!dev_priv->input_size_property)
> > > +		dev_priv->input_size_property =
> > > +			drm_property_create_range(dev, 0, "input size",
> > > +						  0, UINT_MAX);
> > 
> > I think people would be happier with separate width/height properties
> > instead of sticking both into one property.
> > 
> > Also this sounds like a generally useful property for hardware any which
> > has some kind of full crtc scaler. So maybe it should be in drm core.
> > Also what ever happended to the property documentation effort. Did the
> > patch get in, or was it rejected, or is it in limbo?
> > 
> Fine will split this into 2 properties but will there be any limitation
> in future if a single property is persisted with to specify both height
> & width here.
> 
> Sagar has submitted the patch for property documentation and he got the
> ACK also from the Laurent Pinchart, but I think Daniel has not done the
> same. Please can you kindly once check with Daniel for this.
> 
> > > +
> > > +	if (dev_priv->input_size_property)
> > > +		drm_object_attach_property(&intel_crtc->base.base,
> > > +					   dev_priv->input_size_property,
> > > +					   0);
> > > +}
> > > +
> > >  static int intel_crtc_set_property(struct drm_crtc *crtc,
> > >  		struct drm_property *property, uint64_t val)
> > >  {
> > > +	struct drm_device *dev = crtc->dev;
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > >  	int ret = -ENOENT;
> > >  
> > > +	if (property == dev_priv->input_size_property) {
> > > +		int new_width = (int)((val >> 16) & 0xffff);
> > > +		int new_height = (int)(val & 0xffff);
> > > +		const struct drm_display_mode *adjusted_mode =
> > > +				&intel_crtc->config.adjusted_mode;
> > > +
> > > +		if ((new_width == intel_crtc->config.pipe_src_w) &&
> > > +		    (new_height == intel_crtc->config.pipe_src_h))
> > > +			return 0;
> > > +
> > > +		if (((adjusted_mode->crtc_hdisplay != new_width) ||
> > > +		     (adjusted_mode->crtc_vdisplay != new_height)) &&
> > > +		    ((HAS_PCH_SPLIT(dev) &&
> > > +			(!intel_crtc->config.pch_pfit.enabled)) ||
> > > +		     (!HAS_PCH_SPLIT(dev) &&
> > > +			(!intel_crtc->config.gmch_pfit.control)))) {
> > > +			DRM_ERROR("PIPESRC mismatch with Pipe timings &"
> > > +				  "PF is disabled\n");
> > > +			return -EINVAL;
> > 
> > This raises an interesting question. If we can't toggle the pfit on/off
> > dynamically should we have some way to force the pfit on at modeset
> > time (even w/ 1:1 scaling) so that it's possibly to change the scaling
> > dynamically w/o a full modeset.
> > 
> > One idea might be to treat 0 as a special value for this property. If
> > both width and height are 0 we do the old thing and disable pfit if
> > the scaling based on user mode vs. adjusted mode is 1:1, but if this
> > property has anything but 0, always enable pfit. Thoughts anyone?
> > 
> 
> Yes in order to be able to dynamically change the resolution of fb's
> being flipped (without a modeset) first Panel fitter has to be enabled.
> 
> So 0 value of this property will considered as disabled and old
> condition of enabling Panel fitter will be followed(requested mode !=
> adjusted mode). 
> But if this property is set to some value, and if Panel fitter is not
> enabled already, it shall be enabled (which will require a modeset).
> Can we do it like this when user sets a non zero value for this
> property.
> 1. Check first if Panel fitter is enabled or not, and if not then enable
> it forcefully bypassing the original condition.
> 2. Update the PIPESRC register later on, with the new values, in the
> subsequent Page flip ioctl (and not in the set property call itself),
> when FB of the new resolution will be passed.
>  
> > > +		}
> > > +
> > > +		intel_crtc->config.pipe_src_w = new_width;
> > > +		intel_crtc->config.pipe_src_h = new_height;
> > 
> > This looks like it needs more error checks to make sure the scaling
> > ratio stays within acceptable limits.
> > 
> > Also we need to recheck primary plane FB size vs. new PIPESRC.
> > 
> On setting this property,  FB's being subsequently flipped on primary
> plane shall comply with the new Pipe SRC values.
> And check for this, is already done by the drm_crtc_check_viewport
> function in page flip ioctl.
> 
> > And we need to reclip sprites and cursors.
> 
> > 
> > This is starting to be quite a lot of stuff. So for the initial
> > implementation may be it's easier to just do a full modeset here. And
> > once that's done we can figure out how to avoid the modeset. We should
> > anyway be moving towards a world where we compute the new config,
> > validate it, and then figure out if we can get there w/o a full modeset.
> > So maybe we need to start writing the code to just that and not limit
> > ourselves to implementing special code for specific properties.
> > 
> 
> Sorry couldn't fully comprehend your last point here.
> 
> > > +
> > > +		intel_crtc->config.requested_mode.hdisplay = new_width;
> > > +		intel_crtc->config.requested_mode.vdisplay = new_height;
> > > +
> > > +		crtc->mode.hdisplay = new_width;
> > > +		crtc->mode.vdisplay = new_height;
> > 
> > Hmm. This will cause problems if we have to do a modeset w/o a new user
> > specified mode (eg. atomic modeset where change on another pipe requires
> > us to perform modeset on this pipe as well).
> > 
> > So I think we need to leave the modes alone. One thing to worry about is
> > old userspace that doesn't realize that changes in this property would
> > have ramifications on the crtc viewport. I'm not sure there's anything
> > sane we can do but let old userspace shoot itself in the foot if it plays
> > with this property.
> > 
> 
> Again sorry, but couldn't get your old user space point.
> Should the 'hdisplay', 'vdisplay' fields in mode & pipe config
> structures be also updated here, when this property is set or they shall
> be left unchanged ?
> 
> > So at modeset time we need to populate pipe_src_{w,h} from the user mode
> > first, and then overwrite those with the values from this property if
> > the property value != 0. Assuming we go with my "0 is special" plan.
> > 
> 
> If this property is once set by User, and then if it subsequently does a
> modeset, then the 'fb' passed with that modeset call, shall conform to
> which resolution, i.e to the mode passed by User or to the height &
> width specified by the current value of this property ? Or User 
> 
> > > +
> > > +		/* pipesrc controls the size that is scaled from, which should
> > > +		* always be the user's requested size.
> > > +		*/
> > > +		I915_WRITE(PIPESRC(intel_crtc->pipe),
> > > +			((intel_crtc->config.pipe_src_w - 1) << 16) |
> > > +			 (intel_crtc->config.pipe_src_h - 1));
> > 
> > And do we need to reprogram the pfit on gmch?
> > 
> > > +
> > > +		return 0;
> > > +	}
> > > +
> > >  	return ret;
> > >  }
> > >  
> > > @@ -10577,6 +10647,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> > >  	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
> > >  
> > >  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
> > > +
> > > +	intel_create_crtc_properties(&intel_crtc->base);
> > >  }
> > >  
> > >  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
> > > -- 
> > > 1.9.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> 


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

Re: [PATCH v4 2/4] drm/i915: New drm crtc property for varying the Pipe Src size

[Ville Syrjälä]

On Sun, May 04, 2014 at 03:51:51PM +0530, Akash Goel wrote:
> On Tue, 2014-04-29 at 17:06 +0300, Ville Syrjälä wrote:
> > On Sun, Apr 20, 2014 at 04:14:18PM +0530, akash.goel@intel.com wrote:
> > > 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 (without modeset) the frame buffers
> > > of different resolutions
> > > 
> > > v2: Added a check to fail the set property call if Panel fitter is
> > > disabled & new PIPESRC programming do not match with PIPE timings.
> > > Removed the pitch mismatch check on frame buffer, when being flipped.
> > > This is currently done only for VLV/HSW.
> > > 
> > > v3: Modified the check, added in v2, to consider the platforms having
> > > Split PCH.
> > > 
> > > v4: Refactored based on latest codebase.
> > > Used 'UINT_MAX' macro in place of constant.
> > > 
> > > Testcase: igt/kms_panel_fitter_test
> > > 
> > > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h      |  6 +++
> > >  drivers/gpu/drm/i915/intel_display.c | 76 +++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 80 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 7d6acb4..104a232 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1441,6 +1441,12 @@ struct drm_i915_private {
> > >  	struct drm_property *broadcast_rgb_property;
> > >  	struct drm_property *force_audio_property;
> > >  
> > > +	/*
> > > +	 * Property to dynamically vary the size of the
> > > +	 * PIPESRC or Panel fitter input size
> > > +	 */
> > > +	struct drm_property *input_size_property;
> > > +
> > >  	uint32_t hw_context_size;
> > >  	struct list_head context_list;
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index f26be4e..5484ae2 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -8902,8 +8902,18 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> > >  	 * Note that pitch changes could also affect these register.
> > >  	 */
> > >  	if (INTEL_INFO(dev)->gen > 3 &&
> > > -	    (fb->offsets[0] != crtc->primary->fb->offsets[0] ||
> > > -	     fb->pitches[0] != crtc->primary->fb->pitches[0]))
> > > +	    (fb->offsets[0] != crtc->primary->fb->offsets[0]))
> > > +		return -EINVAL;
> > > +
> > > +	/*
> > > +	 * Bypassing the fb pitch check for VLV/HSW, as purportedly there
> > > +	 * is a dynamic flip support in VLV/HSW. This will allow to
> > > +	 * flip fbs of different resolutions without doing a modeset.
> > > +	 * TBD, confirm the same for other newer gen platforms also.
> > > +	 */
> > > +	if (INTEL_INFO(dev)->gen > 3 &&
> > > +	    !IS_VALLEYVIEW(dev) && !IS_HASWELL(dev) &&
> > > +	    (fb->pitches[0] != crtc->primary->fb->pitches[0]))
> > 
> > NAK. Please read the comment above the check to understand why we can't
> > just change the stride here. I guess on HSW+ it might be possible since
> > it uses the x/y offsets even with linear FBs so stride changes don't
> > affect the offset. Also such changes sould be in separate patches
> > anyway.
> > 
> 
> Actually as per the Bspec, probably the pitch/stride can be changed for
> synchronous flips. 
> As per the description of MI_DISPLAY_FLIP from the Bspec
> "15:6 Display Buffer Pitch
> For Synchronous Flips and Stereo 3D Flips only, this field specifies the
> 64-byte aligned pitch of the new display buffer. For Asynchronous Flips,
> this parameter is programmed so that all the flips in a flip chain
> should maintain the same pitch as programmed with the last synchronous
> flip or stereo 3D flip or direct through MMIO.
> The Display Buffer Pitch and Tile parameter and Decrypt Request fields
> cannot be changed for asynchronous flips (i.e., the new buffer must have
> the same pitch/tile format as the previous buffer).
> Asynch flips are Supported on X-Tiled Frame buffers only."
> 
> > I guess one option would be update the linear offset with LRI, but
> > since the offset register gets latched independently of the DSPSURF
> > write the changes aren't guaranteed to happen atomically. Also on
> > VLV LRI to display registers is apparently busted.
> > 
> > If you do find a way to make this work on some platforms, then we
> > need to have a test for it to make sure it does the right thing.
> > 
> 
> Actually we have been using MMIO based flips on BYT, instead of command
> streamer based flips, so the DSPSTRIDE & DSPTILEOFF registers are being
> written along with DSPSURF on every flip. 
> As per the description of DSPSTRIDE register in Bspec, 
> "This register is updated either through a command packet passed through
> the command stream or writes to this register. When it is desired to
> update both this and the start register, 
> the stride register must be written first because the write to the start
> register is the trigger that causes the update of both registers on the
> next VBLANK event"
> So once we switch to MMIO flip, then it shall be taken care.
> 
> 
> > >  		return -EINVAL;
> > >  
> > >  	if (i915_terminally_wedged(&dev_priv->gpu_error))
> > > @@ -10422,11 +10432,71 @@ out_config:
> > >  	return ret;
> > >  }
> > >  
> > > +static void intel_create_crtc_properties(struct drm_crtc *crtc)
> > > +{
> > > +	struct drm_device *dev = crtc->dev;
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +
> > > +	if (!dev_priv->input_size_property)
> > > +		dev_priv->input_size_property =
> > > +			drm_property_create_range(dev, 0, "input size",
> > > +						  0, UINT_MAX);
> > 
> > I think people would be happier with separate width/height properties
> > instead of sticking both into one property.
> > 
> > Also this sounds like a generally useful property for hardware any which
> > has some kind of full crtc scaler. So maybe it should be in drm core.
> > Also what ever happended to the property documentation effort. Did the
> > patch get in, or was it rejected, or is it in limbo?
> > 
> Fine will split this into 2 properties but will there be any limitation
> in future if a single property is persisted with to specify both height
> & width here.
> 
> Sagar has submitted the patch for property documentation and he got the
> ACK also from the Laurent Pinchart, but I think Daniel has not done the
> same. Please can you kindly once check with Daniel for this.
> 
> > > +
> > > +	if (dev_priv->input_size_property)
> > > +		drm_object_attach_property(&intel_crtc->base.base,
> > > +					   dev_priv->input_size_property,
> > > +					   0);
> > > +}
> > > +
> > >  static int intel_crtc_set_property(struct drm_crtc *crtc,
> > >  		struct drm_property *property, uint64_t val)
> > >  {
> > > +	struct drm_device *dev = crtc->dev;
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > >  	int ret = -ENOENT;
> > >  
> > > +	if (property == dev_priv->input_size_property) {
> > > +		int new_width = (int)((val >> 16) & 0xffff);
> > > +		int new_height = (int)(val & 0xffff);
> > > +		const struct drm_display_mode *adjusted_mode =
> > > +				&intel_crtc->config.adjusted_mode;
> > > +
> > > +		if ((new_width == intel_crtc->config.pipe_src_w) &&
> > > +		    (new_height == intel_crtc->config.pipe_src_h))
> > > +			return 0;
> > > +
> > > +		if (((adjusted_mode->crtc_hdisplay != new_width) ||
> > > +		     (adjusted_mode->crtc_vdisplay != new_height)) &&
> > > +		    ((HAS_PCH_SPLIT(dev) &&
> > > +			(!intel_crtc->config.pch_pfit.enabled)) ||
> > > +		     (!HAS_PCH_SPLIT(dev) &&
> > > +			(!intel_crtc->config.gmch_pfit.control)))) {
> > > +			DRM_ERROR("PIPESRC mismatch with Pipe timings &"
> > > +				  "PF is disabled\n");
> > > +			return -EINVAL;
> > 
> > This raises an interesting question. If we can't toggle the pfit on/off
> > dynamically should we have some way to force the pfit on at modeset
> > time (even w/ 1:1 scaling) so that it's possibly to change the scaling
> > dynamically w/o a full modeset.
> > 
> > One idea might be to treat 0 as a special value for this property. If
> > both width and height are 0 we do the old thing and disable pfit if
> > the scaling based on user mode vs. adjusted mode is 1:1, but if this
> > property has anything but 0, always enable pfit. Thoughts anyone?
> > 
> 
> Yes in order to be able to dynamically change the resolution of fb's
> being flipped (without a modeset) first Panel fitter has to be enabled.
> 
> So 0 value of this property will considered as disabled and old
> condition of enabling Panel fitter will be followed(requested mode !=
> adjusted mode). 
> But if this property is set to some value, and if Panel fitter is not
> enabled already, it shall be enabled (which will require a modeset).
> Can we do it like this when user sets a non zero value for this
> property.
> 1. Check first if Panel fitter is enabled or not, and if not then enable
> it forcefully bypassing the original condition.
> 2. Update the PIPESRC register later on, with the new values, in the
> subsequent Page flip ioctl (and not in the set property call itself),
> when FB of the new resolution will be passed.

No we need to update everything immediately. For doing things the nice
way we're going to need the atomic modeset ioctl, but for now we need
to comply with the restrictions of the old ioctls. We should not add
any kind of limited special purpose atomic update doohickeys in the
meantime.

>  
> > > +		}
> > > +
> > > +		intel_crtc->config.pipe_src_w = new_width;
> > > +		intel_crtc->config.pipe_src_h = new_height;
> > 
> > This looks like it needs more error checks to make sure the scaling
> > ratio stays within acceptable limits.
> > 
> > Also we need to recheck primary plane FB size vs. new PIPESRC.
> > 
> On setting this property,  FB's being subsequently flipped on primary
> plane shall comply with the new Pipe SRC values.
> And check for this, is already done by the drm_crtc_check_viewport
> function in page flip ioctl.

As stated we need to update everything immediately.

> 
> > And we need to reclip sprites and cursors.
> 
> > 
> > This is starting to be quite a lot of stuff. So for the initial
> > implementation may be it's easier to just do a full modeset here. And
> > once that's done we can figure out how to avoid the modeset. We should
> > anyway be moving towards a world where we compute the new config,
> > validate it, and then figure out if we can get there w/o a full modeset.
> > So maybe we need to start writing the code to just that and not limit
> > ourselves to implementing special code for specific properties.
> > 
> 
> Sorry couldn't fully comprehend your last point here.

The point is that we don't want to duplicate the code that does all this
checking, reclipping etc. So ideally our modeset path should look a bit
like this:

compute_config();
if (can't do it)
	return error;
if (need_full_modeset()) {
	modeset();
} else {
	nuclear_flip();
	// ie. reconfigure planes, reprogram pipesrc, etc.
}

We already have something along those lines for turning a setcrtc ioctl
into just a mmio flip instead of a full modeset. But this should be
extended to handle all kinds of interesting stuff.

Just doing a full modeset for changes in the property value is a reasonable
first step IMO. Adding a short circuit codepath to avoid the full modeset
can be done as a followup patch if needed. But going for the full nuclear
flip thing would seem a better goal than spending time on polishing such
special purpose codepaths. But of course if the code can be written in a
way that contributes to the nuclear flip goal then I see no downsides to
it.

> 
> > > +
> > > +		intel_crtc->config.requested_mode.hdisplay = new_width;
> > > +		intel_crtc->config.requested_mode.vdisplay = new_height;
> > > +
> > > +		crtc->mode.hdisplay = new_width;
> > > +		crtc->mode.vdisplay = new_height;
> > 
> > Hmm. This will cause problems if we have to do a modeset w/o a new user
> > specified mode (eg. atomic modeset where change on another pipe requires
> > us to perform modeset on this pipe as well).
> > 
> > So I think we need to leave the modes alone. One thing to worry about is
> > old userspace that doesn't realize that changes in this property would
> > have ramifications on the crtc viewport. I'm not sure there's anything
> > sane we can do but let old userspace shoot itself in the foot if it plays
> > with this property.
> > 
> 
> Again sorry, but couldn't get your old user space point.
> Should the 'hdisplay', 'vdisplay' fields in mode & pipe config
> structures be also updated here, when this property is set or they shall
> be left unchanged ?

Unchanged.

> 
> > So at modeset time we need to populate pipe_src_{w,h} from the user mode
> > first, and then overwrite those with the values from this property if
> > the property value != 0. Assuming we go with my "0 is special" plan.
> > 
> 
> If this property is once set by User, and then if it subsequently does a
> modeset, then the 'fb' passed with that modeset call, shall conform to
> which resolution, i.e to the mode passed by User or to the height &
> width specified by the current value of this property ? Or User 

Well the user can pass any size fb, but we must make sure that it is
big enough to contain the current primary plane size ie. pipesrc since
the primary plane is always fullscreen. But we also need to make sure
the user requested primary plane coordinates are suitable for the new
pipesrc, or the primary plane must be disabled when changing this
property. That kind of stuff is already checked by the primary plane
clipping code.


> 
> > > +
> > > +		/* pipesrc controls the size that is scaled from, which should
> > > +		* always be the user's requested size.
> > > +		*/
> > > +		I915_WRITE(PIPESRC(intel_crtc->pipe),
> > > +			((intel_crtc->config.pipe_src_w - 1) << 16) |
> > > +			 (intel_crtc->config.pipe_src_h - 1));
> > 
> > And do we need to reprogram the pfit on gmch?
> > 
> > > +
> > > +		return 0;
> > > +	}
> > > +
> > >  	return ret;
> > >  }
> > >  
> > > @@ -10577,6 +10647,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> > >  	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
> > >  
> > >  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
> > > +
> > > +	intel_create_crtc_properties(&intel_crtc->base);
> > >  }
> > >  
> > >  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
> > > -- 
> > > 1.9.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> 

-- 
Ville Syrjälä
Intel OTC