[PATCH 0/5] i915 display refactoring

[PATCH 0/5] i915 display refactoring

[Matt Roper]

This patch set continues the work that Gustavo Padovan (Collabora) started to
refactor the i915 display code in preparation for atomic modeset.  Gustavo's
patches have been updated to work with the latest drm-intel-nightly code and
the review feedback that came in after he got pulled away to work on other
projects has been incorporated.

Gustavo Padovan (3):
  drm: add helper to get crtc timings (v2)
  drm/i915: remove intel_crtc_cursor_set_obj() (v5)
  drm/i915: remove intel_pipe_set_base() (v3)

Matt Roper (2):
  drm: Refactor mode stereo doubling into its own function
  drm/i915: Don't store panning coordinates as 16.16 fixed point

 drivers/gpu/drm/drm_crtc.c           |  22 ++-
 drivers/gpu/drm/drm_modes.c          |  39 ++--
 drivers/gpu/drm/i915/intel_display.c | 349 +++++++++++++----------------------
 include/drm/drm_crtc.h               |   2 +
 include/drm/drm_modes.h              |   1 +
 5 files changed, 172 insertions(+), 241 deletions(-)

-- 
1.8.5.1

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

[PATCH 1/5] drm: Refactor mode stereo doubling into its own function

[Matt Roper]

There are places where we want to perform vertical doubling for stereo
modes without the other adjustments (doublescan, vscan > 1, etc.) that
drm_mode_set_crtcinfo() performs.  Refactor this logic into its own
function so that it can be called directly in such places.

Cc: dri-devel@lists.freedesktop.org
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_modes.c | 39 ++++++++++++++++++++++++++-------------
 include/drm/drm_modes.h     |  1 +
 2 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 6d8b941..8c25de3 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -728,6 +728,30 @@ int drm_mode_vrefresh(const struct drm_display_mode *mode)
 EXPORT_SYMBOL(drm_mode_vrefresh);
 
 /**
+ * drm_mode_stereo_double - Adjust mode timings for stereo modes
+ * @p: mode to adjust vertical timings of
+ *
+ * Performs stereo doubling of mode parameters when required by the stereo
+ * layout.  This may be used directly in places where the additional
+ * adjustments of drm_mode_set_crtcinfo() are undesired.
+ */
+void drm_mode_stereo_double(struct drm_display_mode *p)
+{
+	unsigned int layout = p->flags & DRM_MODE_FLAG_3D_MASK;
+
+	switch (layout) {
+	case DRM_MODE_FLAG_3D_FRAME_PACKING:
+		p->crtc_clock *= 2;
+		p->crtc_vdisplay += p->crtc_vtotal;
+		p->crtc_vsync_start += p->crtc_vtotal;
+		p->crtc_vsync_end += p->crtc_vtotal;
+		p->crtc_vtotal += p->crtc_vtotal;
+		break;
+	}
+}
+EXPORT_SYMBOL(drm_mode_stereo_double);
+
+/**
  * drm_mode_set_crtcinfo - set CRTC modesetting timing parameters
  * @p: mode
  * @adjust_flags: a combination of adjustment flags
@@ -779,19 +803,8 @@ void drm_mode_set_crtcinfo(struct drm_display_mode *p, int adjust_flags)
 		p->crtc_vtotal *= p->vscan;
 	}
 
-	if (adjust_flags & CRTC_STEREO_DOUBLE) {
-		unsigned int layout = p->flags & DRM_MODE_FLAG_3D_MASK;
-
-		switch (layout) {
-		case DRM_MODE_FLAG_3D_FRAME_PACKING:
-			p->crtc_clock *= 2;
-			p->crtc_vdisplay += p->crtc_vtotal;
-			p->crtc_vsync_start += p->crtc_vtotal;
-			p->crtc_vsync_end += p->crtc_vtotal;
-			p->crtc_vtotal += p->crtc_vtotal;
-			break;
-		}
-	}
+	if (adjust_flags & CRTC_STEREO_DOUBLE)
+		drm_mode_stereo_double(p);
 
 	p->crtc_vblank_start = min(p->crtc_vsync_start, p->crtc_vdisplay);
 	p->crtc_vblank_end = max(p->crtc_vsync_end, p->crtc_vtotal);
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 91d0582..2b3d05e 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -205,6 +205,7 @@ void drm_mode_set_name(struct drm_display_mode *mode);
 int drm_mode_hsync(const struct drm_display_mode *mode);
 int drm_mode_vrefresh(const struct drm_display_mode *mode);
 
+void drm_mode_stereo_double(struct drm_display_mode *p);
 void drm_mode_set_crtcinfo(struct drm_display_mode *p,
 			   int adjust_flags);
 void drm_mode_copy(struct drm_display_mode *dst,
-- 
1.8.5.1

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

[PATCH 2/5] drm: add helper to get crtc timings (v2)

[Matt Roper]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

We need to get hdisplay and vdisplay in a few places so create a
helper to make our job easier.

v2 (by Matt): Use new stereo doubling function (suggested by Ville)

Cc: dri-devel@lists.freedesktop.org
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c           | 22 ++++++++++++----------
 drivers/gpu/drm/i915/intel_display.c |  6 +++---
 include/drm/drm_crtc.h               |  2 ++
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 56737e7..64ec23b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2493,6 +2493,17 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
 }
 EXPORT_SYMBOL(drm_mode_set_config_internal);
 
+void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
+			    int *hdisplay, int *vdisplay)
+{
+	struct drm_display_mode adjusted = *mode;
+
+	drm_mode_stereo_double(&adjusted);
+	*hdisplay = adjusted.crtc_hdisplay;
+	*vdisplay = adjusted.crtc_vdisplay;
+}
+EXPORT_SYMBOL(drm_crtc_get_hv_timing);
+
 /**
  * drm_crtc_check_viewport - Checks that a framebuffer is big enough for the
  *     CRTC viewport
@@ -2510,16 +2521,7 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
 {
 	int hdisplay, vdisplay;
 
-	hdisplay = mode->hdisplay;
-	vdisplay = mode->vdisplay;
-
-	if (drm_mode_is_stereo(mode)) {
-		struct drm_display_mode adjusted = *mode;
-
-		drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE);
-		hdisplay = adjusted.crtc_hdisplay;
-		vdisplay = adjusted.crtc_vdisplay;
-	}
+	drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
 
 	if (crtc->invert_dimensions)
 		swap(hdisplay, vdisplay);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 320bf4c..a967623 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10158,9 +10158,9 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 	 * computation to clearly distinguish it from the adjusted mode, which
 	 * can be changed by the connectors in the below retry loop.
 	 */
-	drm_mode_set_crtcinfo(&pipe_config->requested_mode, CRTC_STEREO_DOUBLE);
-	pipe_config->pipe_src_w = pipe_config->requested_mode.crtc_hdisplay;
-	pipe_config->pipe_src_h = pipe_config->requested_mode.crtc_vdisplay;
+	drm_crtc_get_hv_timing(&pipe_config->requested_mode,
+			       &pipe_config->pipe_src_w,
+			       &pipe_config->pipe_src_h);
 
 encoder_retry:
 	/* Ensure the port clock defaults are reset when retrying. */
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 7b28ab0..23236f6 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1138,6 +1138,8 @@ extern int drm_plane_init(struct drm_device *dev,
 extern void drm_plane_cleanup(struct drm_plane *plane);
 extern unsigned int drm_plane_index(struct drm_plane *plane);
 extern void drm_plane_force_disable(struct drm_plane *plane);
+extern void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
+				   int *hdisplay, int *vdisplay);
 extern int drm_crtc_check_viewport(const struct drm_crtc *crtc,
 				   int x, int y,
 				   const struct drm_display_mode *mode,
-- 
1.8.5.1

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

Re: [PATCH 2/5] drm: add helper to get crtc timings (v2)

[Daniel Vetter]

On Mon, Nov 17, 2014 at 06:10:36PM -0800, Matt Roper wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> We need to get hdisplay and vdisplay in a few places so create a
> helper to make our job easier.
> 
> v2 (by Matt): Use new stereo doubling function (suggested by Ville)
> 
> Cc: dri-devel@lists.freedesktop.org
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c           | 22 ++++++++++++----------
>  drivers/gpu/drm/i915/intel_display.c |  6 +++---
>  include/drm/drm_crtc.h               |  2 ++
>  3 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 56737e7..64ec23b 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2493,6 +2493,17 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
>  }
>  EXPORT_SYMBOL(drm_mode_set_config_internal);
>  

Kerneldoc missing.
-Daniel

> +void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
> +			    int *hdisplay, int *vdisplay)
> +{
> +	struct drm_display_mode adjusted = *mode;
> +
> +	drm_mode_stereo_double(&adjusted);
> +	*hdisplay = adjusted.crtc_hdisplay;
> +	*vdisplay = adjusted.crtc_vdisplay;
> +}
> +EXPORT_SYMBOL(drm_crtc_get_hv_timing);
> +
>  /**
>   * drm_crtc_check_viewport - Checks that a framebuffer is big enough for the
>   *     CRTC viewport
> @@ -2510,16 +2521,7 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
>  {
>  	int hdisplay, vdisplay;
>  
> -	hdisplay = mode->hdisplay;
> -	vdisplay = mode->vdisplay;
> -
> -	if (drm_mode_is_stereo(mode)) {
> -		struct drm_display_mode adjusted = *mode;
> -
> -		drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE);
> -		hdisplay = adjusted.crtc_hdisplay;
> -		vdisplay = adjusted.crtc_vdisplay;
> -	}
> +	drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
>  
>  	if (crtc->invert_dimensions)
>  		swap(hdisplay, vdisplay);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 320bf4c..a967623 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10158,9 +10158,9 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  	 * computation to clearly distinguish it from the adjusted mode, which
>  	 * can be changed by the connectors in the below retry loop.
>  	 */
> -	drm_mode_set_crtcinfo(&pipe_config->requested_mode, CRTC_STEREO_DOUBLE);
> -	pipe_config->pipe_src_w = pipe_config->requested_mode.crtc_hdisplay;
> -	pipe_config->pipe_src_h = pipe_config->requested_mode.crtc_vdisplay;
> +	drm_crtc_get_hv_timing(&pipe_config->requested_mode,
> +			       &pipe_config->pipe_src_w,
> +			       &pipe_config->pipe_src_h);
>  
>  encoder_retry:
>  	/* Ensure the port clock defaults are reset when retrying. */
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 7b28ab0..23236f6 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1138,6 +1138,8 @@ extern int drm_plane_init(struct drm_device *dev,
>  extern void drm_plane_cleanup(struct drm_plane *plane);
>  extern unsigned int drm_plane_index(struct drm_plane *plane);
>  extern void drm_plane_force_disable(struct drm_plane *plane);
> +extern void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
> +				   int *hdisplay, int *vdisplay);
>  extern int drm_crtc_check_viewport(const struct drm_crtc *crtc,
>  				   int x, int y,
>  				   const struct drm_display_mode *mode,
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/5] drm: add helper to get crtc timings (v2)

[Jani Nikula]

On Tue, 18 Nov 2014, Matt Roper <matthew.d.roper@intel.com> wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> We need to get hdisplay and vdisplay in a few places so create a
> helper to make our job easier.
>
> v2 (by Matt): Use new stereo doubling function (suggested by Ville)
>
> Cc: dri-devel@lists.freedesktop.org
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c           | 22 ++++++++++++----------
>  drivers/gpu/drm/i915/intel_display.c |  6 +++---
>  include/drm/drm_crtc.h               |  2 ++
>  3 files changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 56737e7..64ec23b 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2493,6 +2493,17 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
>  }
>  EXPORT_SYMBOL(drm_mode_set_config_internal);
>  
> +void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
> +			    int *hdisplay, int *vdisplay)
> +{
> +	struct drm_display_mode adjusted = *mode;

I'd prefer using drm_mode_copy(). It doesn't matter here and now, but if
it starts to matter in the future the problems will be hard to track.

BR,
Jani.


> +
> +	drm_mode_stereo_double(&adjusted);
> +	*hdisplay = adjusted.crtc_hdisplay;
> +	*vdisplay = adjusted.crtc_vdisplay;
> +}
> +EXPORT_SYMBOL(drm_crtc_get_hv_timing);
> +
>  /**
>   * drm_crtc_check_viewport - Checks that a framebuffer is big enough for the
>   *     CRTC viewport
> @@ -2510,16 +2521,7 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
>  {
>  	int hdisplay, vdisplay;
>  
> -	hdisplay = mode->hdisplay;
> -	vdisplay = mode->vdisplay;
> -
> -	if (drm_mode_is_stereo(mode)) {
> -		struct drm_display_mode adjusted = *mode;
> -
> -		drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE);
> -		hdisplay = adjusted.crtc_hdisplay;
> -		vdisplay = adjusted.crtc_vdisplay;
> -	}
> +	drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
>  
>  	if (crtc->invert_dimensions)
>  		swap(hdisplay, vdisplay);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 320bf4c..a967623 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10158,9 +10158,9 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  	 * computation to clearly distinguish it from the adjusted mode, which
>  	 * can be changed by the connectors in the below retry loop.
>  	 */
> -	drm_mode_set_crtcinfo(&pipe_config->requested_mode, CRTC_STEREO_DOUBLE);
> -	pipe_config->pipe_src_w = pipe_config->requested_mode.crtc_hdisplay;
> -	pipe_config->pipe_src_h = pipe_config->requested_mode.crtc_vdisplay;
> +	drm_crtc_get_hv_timing(&pipe_config->requested_mode,
> +			       &pipe_config->pipe_src_w,
> +			       &pipe_config->pipe_src_h);
>  
>  encoder_retry:
>  	/* Ensure the port clock defaults are reset when retrying. */
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 7b28ab0..23236f6 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1138,6 +1138,8 @@ extern int drm_plane_init(struct drm_device *dev,
>  extern void drm_plane_cleanup(struct drm_plane *plane);
>  extern unsigned int drm_plane_index(struct drm_plane *plane);
>  extern void drm_plane_force_disable(struct drm_plane *plane);
> +extern void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
> +				   int *hdisplay, int *vdisplay);
>  extern int drm_crtc_check_viewport(const struct drm_crtc *crtc,
>  				   int x, int y,
>  				   const struct drm_display_mode *mode,
> -- 
> 1.8.5.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[PATCH 2/5] drm: add helper to get crtc timings (v3)

[Matt Roper]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

We need to get hdisplay and vdisplay in a few places so create a
helper to make our job easier.

v2 (by Matt): Use new stereo doubling function (suggested by Ville)

v3 (by Matt):
 - Add missing kerneldoc (Daniel)
 - Use drm_mode_copy() (Jani)

Cc: dri-devel@lists.freedesktop.org
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c           | 32 ++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_display.c |  6 +++---
 include/drm/drm_crtc.h               |  2 ++
 3 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 56737e7..be1a485 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2494,6 +2494,27 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
 EXPORT_SYMBOL(drm_mode_set_config_internal);
 
 /**
+ * drm_crtc_get_hv_timing - Fetches hdisplay/vdisplay for given mode
+ * @mode: mode to query
+ * @hdisplay: hdisplay value to fill in
+ * @vdisplay: vdisplay value to fill in
+ *
+ * The vdisplay value will be doubled if the specified mode is a stereo mode of
+ * the appropriate layout.
+ */
+void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
+			    int *hdisplay, int *vdisplay)
+{
+	struct drm_display_mode adjusted;
+
+	drm_mode_copy(&adjusted, mode);
+	drm_mode_stereo_double(&adjusted);
+	*hdisplay = adjusted.crtc_hdisplay;
+	*vdisplay = adjusted.crtc_vdisplay;
+}
+EXPORT_SYMBOL(drm_crtc_get_hv_timing);
+
+/**
  * drm_crtc_check_viewport - Checks that a framebuffer is big enough for the
  *     CRTC viewport
  * @crtc: CRTC that framebuffer will be displayed on
@@ -2510,16 +2531,7 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
 {
 	int hdisplay, vdisplay;
 
-	hdisplay = mode->hdisplay;
-	vdisplay = mode->vdisplay;
-
-	if (drm_mode_is_stereo(mode)) {
-		struct drm_display_mode adjusted = *mode;
-
-		drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE);
-		hdisplay = adjusted.crtc_hdisplay;
-		vdisplay = adjusted.crtc_vdisplay;
-	}
+	drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
 
 	if (crtc->invert_dimensions)
 		swap(hdisplay, vdisplay);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 320bf4c..a967623 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10158,9 +10158,9 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 	 * computation to clearly distinguish it from the adjusted mode, which
 	 * can be changed by the connectors in the below retry loop.
 	 */
-	drm_mode_set_crtcinfo(&pipe_config->requested_mode, CRTC_STEREO_DOUBLE);
-	pipe_config->pipe_src_w = pipe_config->requested_mode.crtc_hdisplay;
-	pipe_config->pipe_src_h = pipe_config->requested_mode.crtc_vdisplay;
+	drm_crtc_get_hv_timing(&pipe_config->requested_mode,
+			       &pipe_config->pipe_src_w,
+			       &pipe_config->pipe_src_h);
 
 encoder_retry:
 	/* Ensure the port clock defaults are reset when retrying. */
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 7b28ab0..23236f6 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1138,6 +1138,8 @@ extern int drm_plane_init(struct drm_device *dev,
 extern void drm_plane_cleanup(struct drm_plane *plane);
 extern unsigned int drm_plane_index(struct drm_plane *plane);
 extern void drm_plane_force_disable(struct drm_plane *plane);
+extern void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
+				   int *hdisplay, int *vdisplay);
 extern int drm_crtc_check_viewport(const struct drm_crtc *crtc,
 				   int x, int y,
 				   const struct drm_display_mode *mode,
-- 
1.8.5.1

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

Re: [PATCH 2/5] drm: add helper to get crtc timings (v3)

[Ville Syrjälä]

On Tue, Nov 18, 2014 at 09:12:49AM -0800, Matt Roper wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> We need to get hdisplay and vdisplay in a few places so create a
> helper to make our job easier.
> 
> v2 (by Matt): Use new stereo doubling function (suggested by Ville)
> 
> v3 (by Matt):
>  - Add missing kerneldoc (Daniel)
>  - Use drm_mode_copy() (Jani)
> 
> Cc: dri-devel@lists.freedesktop.org
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c           | 32 ++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_display.c |  6 +++---
>  include/drm/drm_crtc.h               |  2 ++
>  3 files changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 56737e7..be1a485 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2494,6 +2494,27 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
>  EXPORT_SYMBOL(drm_mode_set_config_internal);
>  
>  /**
> + * drm_crtc_get_hv_timing - Fetches hdisplay/vdisplay for given mode
> + * @mode: mode to query
> + * @hdisplay: hdisplay value to fill in
> + * @vdisplay: vdisplay value to fill in
> + *
> + * The vdisplay value will be doubled if the specified mode is a stereo mode of
> + * the appropriate layout.
> + */
> +void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
> +			    int *hdisplay, int *vdisplay)
> +{
> +	struct drm_display_mode adjusted;
> +
> +	drm_mode_copy(&adjusted, mode);
> +	drm_mode_stereo_double(&adjusted);

Hmm. The mode may not have the crtc_ timings populated at all here, so
drm_mode_stereo_double() might just double some zeroes/garbage.

I don't recall anymore if I had some clever idea how to do this. I was
hoping we can avoid duplicating the stereo doubling logic in more than
once place, but maybe it's just easier to admit defeat?

I guess the options are:
1) duplciate some stereo doubling logic
2) populate the required crtc_ timings here as well
3) keep using drm_mode_set_crtcinfo() but remove the offending stuff
   from the copied mode so that it doesn't interfere with the results
4) add more flags to drm_mode_set_crtcinfo() that prevent the offending
   stuff from affecting the results

> +	*hdisplay = adjusted.crtc_hdisplay;
> +	*vdisplay = adjusted.crtc_vdisplay;
> +}
> +EXPORT_SYMBOL(drm_crtc_get_hv_timing);
> +
> +/**
>   * drm_crtc_check_viewport - Checks that a framebuffer is big enough for the
>   *     CRTC viewport
>   * @crtc: CRTC that framebuffer will be displayed on
> @@ -2510,16 +2531,7 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
>  {
>  	int hdisplay, vdisplay;
>  
> -	hdisplay = mode->hdisplay;
> -	vdisplay = mode->vdisplay;
> -
> -	if (drm_mode_is_stereo(mode)) {
> -		struct drm_display_mode adjusted = *mode;
> -
> -		drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE);
> -		hdisplay = adjusted.crtc_hdisplay;
> -		vdisplay = adjusted.crtc_vdisplay;
> -	}
> +	drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
>  
>  	if (crtc->invert_dimensions)
>  		swap(hdisplay, vdisplay);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 320bf4c..a967623 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10158,9 +10158,9 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  	 * computation to clearly distinguish it from the adjusted mode, which
>  	 * can be changed by the connectors in the below retry loop.
>  	 */
> -	drm_mode_set_crtcinfo(&pipe_config->requested_mode, CRTC_STEREO_DOUBLE);
> -	pipe_config->pipe_src_w = pipe_config->requested_mode.crtc_hdisplay;
> -	pipe_config->pipe_src_h = pipe_config->requested_mode.crtc_vdisplay;
> +	drm_crtc_get_hv_timing(&pipe_config->requested_mode,
> +			       &pipe_config->pipe_src_w,
> +			       &pipe_config->pipe_src_h);
>  
>  encoder_retry:
>  	/* Ensure the port clock defaults are reset when retrying. */
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 7b28ab0..23236f6 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1138,6 +1138,8 @@ extern int drm_plane_init(struct drm_device *dev,
>  extern void drm_plane_cleanup(struct drm_plane *plane);
>  extern unsigned int drm_plane_index(struct drm_plane *plane);
>  extern void drm_plane_force_disable(struct drm_plane *plane);
> +extern void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
> +				   int *hdisplay, int *vdisplay);
>  extern int drm_crtc_check_viewport(const struct drm_crtc *crtc,
>  				   int x, int y,
>  				   const struct drm_display_mode *mode,
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915: remove intel_crtc_cursor_set_obj() (v5)

[Matt Roper]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Merge it into the plane update_plane() callback and make other
users use the update_plane() functions instead.

The fb != crtc->cursor->fb was already inside intel_crtc_cursor_set_obj()
so we fold intel_crtc_cursor_set_obj() inside intel_commit_cursor_plane()
and merge both paths into one.

v5 (by Matt):
 - Rebase onto latest di-nightly codebase
 - Drop extra unreference call when we fail to pin (Ville)

Reviewed-by(v4): Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 215 ++++++++++++++++-------------------
 1 file changed, 99 insertions(+), 116 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a967623..8dbc0e9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8364,109 +8364,6 @@ static bool cursor_size_ok(struct drm_device *dev,
 	return true;
 }
 
-static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
-				     struct drm_i915_gem_object *obj,
-				     uint32_t width, uint32_t height)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum pipe pipe = intel_crtc->pipe;
-	unsigned old_width;
-	uint32_t addr;
-	int ret;
-
-	/* if we want to turn off the cursor ignore width and height */
-	if (!obj) {
-		DRM_DEBUG_KMS("cursor off\n");
-		addr = 0;
-		mutex_lock(&dev->struct_mutex);
-		goto finish;
-	}
-
-	/* we only need to pin inside GTT if cursor is non-phy */
-	mutex_lock(&dev->struct_mutex);
-	if (!INTEL_INFO(dev)->cursor_needs_physical) {
-		unsigned alignment;
-
-		/*
-		 * Global gtt pte registers are special registers which actually
-		 * forward writes to a chunk of system memory. Which means that
-		 * there is no risk that the register values disappear as soon
-		 * as we call intel_runtime_pm_put(), so it is correct to wrap
-		 * only the pin/unpin/fence and not more.
-		 */
-		intel_runtime_pm_get(dev_priv);
-
-		/* Note that the w/a also requires 2 PTE of padding following
-		 * the bo. We currently fill all unused PTE with the shadow
-		 * page and so we should always have valid PTE following the
-		 * cursor preventing the VT-d warning.
-		 */
-		alignment = 0;
-		if (need_vtd_wa(dev))
-			alignment = 64*1024;
-
-		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
-			intel_runtime_pm_put(dev_priv);
-			goto fail_locked;
-		}
-
-		ret = i915_gem_object_put_fence(obj);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to release fence for cursor");
-			intel_runtime_pm_put(dev_priv);
-			goto fail_unpin;
-		}
-
-		addr = i915_gem_obj_ggtt_offset(obj);
-
-		intel_runtime_pm_put(dev_priv);
-	} else {
-		int align = IS_I830(dev) ? 16 * 1024 : 256;
-		ret = i915_gem_object_attach_phys(obj, align);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to attach phys object\n");
-			goto fail_locked;
-		}
-		addr = obj->phys_handle->busaddr;
-	}
-
- finish:
-	if (intel_crtc->cursor_bo) {
-		if (!INTEL_INFO(dev)->cursor_needs_physical)
-			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
-	}
-
-	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
-			  INTEL_FRONTBUFFER_CURSOR(pipe));
-	mutex_unlock(&dev->struct_mutex);
-
-	old_width = intel_crtc->cursor_width;
-
-	intel_crtc->cursor_addr = addr;
-	intel_crtc->cursor_bo = obj;
-	intel_crtc->cursor_width = width;
-	intel_crtc->cursor_height = height;
-
-	if (intel_crtc->active) {
-		if (old_width != width)
-			intel_update_watermarks(crtc);
-		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
-
-		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
-	}
-
-	return 0;
-fail_unpin:
-	i915_gem_object_unpin_from_display_plane(obj);
-fail_locked:
-	mutex_unlock(&dev->struct_mutex);
-	return ret;
-}
-
 static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
 				 u16 *blue, uint32_t start, uint32_t size)
 {
@@ -11965,7 +11862,8 @@ intel_cursor_plane_disable(struct drm_plane *plane)
 
 	BUG_ON(!plane->crtc);
 
-	return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
+	return plane->funcs->update_plane(plane, plane->crtc, NULL,
+					  0, 0, 0, 0, 0, 0, 0, 0);
 }
 
 static int
@@ -12029,12 +11927,15 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->crtc;
-	struct drm_framebuffer *fb = state->fb;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
-	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-	struct drm_i915_gem_object *obj = intel_fb->obj;
-	int crtc_w, crtc_h;
+	struct drm_i915_gem_object *obj = intel_fb_obj(state->fb);
+	enum pipe pipe = intel_crtc->pipe;
+	unsigned old_width;
+	uint32_t addr;
+	int ret;
 
 	crtc->cursor_x = state->orig_dst.x1;
 	crtc->cursor_y = state->orig_dst.y1;
@@ -12049,18 +11950,100 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	intel_plane->src_h = drm_rect_height(&state->orig_src);
 	intel_plane->obj = obj;
 
-	if (fb != crtc->cursor->fb) {
-		crtc_w = drm_rect_width(&state->orig_dst);
-		crtc_h = drm_rect_height(&state->orig_dst);
-		return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
+	if (intel_crtc->cursor_bo == obj)
+		goto update;
+
+	/* if we want to turn off the cursor ignore width and height */
+	if (!obj) {
+		DRM_DEBUG_KMS("cursor off\n");
+		addr = 0;
+		mutex_lock(&dev->struct_mutex);
+		goto finish;
+	}
+
+	/* we only need to pin inside GTT if cursor is non-phy */
+	mutex_lock(&dev->struct_mutex);
+	if (!INTEL_INFO(dev)->cursor_needs_physical) {
+		unsigned alignment;
+
+		/*
+		 * Global gtt pte registers are special registers which actually
+		 * forward writes to a chunk of system memory. Which means that
+		 * there is no risk that the register values disappear as soon
+		 * as we call intel_runtime_pm_put(), so it is correct to wrap
+		 * only the pin/unpin/fence and not more.
+		 */
+		intel_runtime_pm_get(dev_priv);
+
+		/* Note that the w/a also requires 2 PTE of padding following
+		 * the bo. We currently fill all unused PTE with the shadow
+		 * page and so we should always have valid PTE following the
+		 * cursor preventing the VT-d warning.
+		 */
+		alignment = 0;
+		if (need_vtd_wa(dev))
+			alignment = 64*1024;
+
+		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
+			intel_runtime_pm_put(dev_priv);
+			goto fail_locked;
+		}
+
+		ret = i915_gem_object_put_fence(obj);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to release fence for cursor");
+			intel_runtime_pm_put(dev_priv);
+			goto fail_unpin;
+		}
+
+		addr = i915_gem_obj_ggtt_offset(obj);
+
+		intel_runtime_pm_put(dev_priv);
+
 	} else {
-		intel_crtc_update_cursor(crtc, state->visible);
+		int align = IS_I830(dev) ? 16 * 1024 : 256;
+		ret = i915_gem_object_attach_phys(obj, align);
+		if (ret) {
+			DRM_DEBUG_KMS("failed to attach phys object\n");
+			goto fail_locked;
+		}
+		addr = obj->phys_handle->busaddr;
+	}
 
-		intel_frontbuffer_flip(crtc->dev,
-				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
+finish:
+	if (intel_crtc->cursor_bo) {
+		if (!INTEL_INFO(dev)->cursor_needs_physical)
+			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
+	}
 
-		return 0;
+	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
+			  INTEL_FRONTBUFFER_CURSOR(pipe));
+	mutex_unlock(&dev->struct_mutex);
+
+	intel_crtc->cursor_addr = addr;
+	intel_crtc->cursor_bo = obj;
+update:
+	old_width = intel_crtc->cursor_width;
+
+	intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
+	intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
+
+	if (intel_crtc->active) {
+		if (old_width != intel_crtc->cursor_width)
+			intel_update_watermarks(crtc);
+		intel_crtc_update_cursor(crtc, state->visible);
+
+		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
 	}
+
+	return 0;
+fail_unpin:
+	i915_gem_object_unpin_from_display_plane(obj);
+fail_locked:
+	mutex_unlock(&dev->struct_mutex);
+	return ret;
 }
 
 static int
-- 
1.8.5.1

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

[PATCH 4/5] drm/i915: Don't store panning coordinates as 16.16 fixed point

[Matt Roper]

When using the universal plane interface, the source rectangle
coordinates define the panning offset for the primary plane, which needs
to be stored in crtc->{x,y}.  The original universal plane code
negelected to set these panning offset fields, which was partially
remedied in:

        commit ccc759dc2a0214fd8b65ed4ebe78050874a67f94
        Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
        Date:   Wed Sep 24 14:20:22 2014 -0300

            drm/i915: Merge of visible and !visible paths for primary planes

However the plane source coordinates are provided in 16.16 fixed point
format and the above commit forgot to convert back to integer
coordinates before saving the values.  When we replace
intel_pipe_set_base() with plane->funcs->update_plane() in a future
patch, this bug becomes visible via the set_config entrypoint as well as
update_plane.

Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Testcase: igt/kms_plane
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8dbc0e9..1c1886e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11668,8 +11668,8 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	struct drm_rect *src = &state->src;
 
 	crtc->primary->fb = fb;
-	crtc->x = src->x1;
-	crtc->y = src->y1;
+	crtc->x = src->x1 >> 16;
+	crtc->y = src->y1 >> 16;
 
 	intel_plane->crtc_x = state->orig_dst.x1;
 	intel_plane->crtc_y = state->orig_dst.y1;
-- 
1.8.5.1

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

Re: [PATCH 4/5] drm/i915: Don't store panning coordinates as 16.16 fixed point

[Ville Syrjälä]

On Mon, Nov 17, 2014 at 06:10:38PM -0800, Matt Roper wrote:
> When using the universal plane interface, the source rectangle
> coordinates define the panning offset for the primary plane, which needs
> to be stored in crtc->{x,y}.  The original universal plane code
> negelected to set these panning offset fields, which was partially
> remedied in:
> 
>         commit ccc759dc2a0214fd8b65ed4ebe78050874a67f94
>         Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>         Date:   Wed Sep 24 14:20:22 2014 -0300
> 
>             drm/i915: Merge of visible and !visible paths for primary planes
> 
> However the plane source coordinates are provided in 16.16 fixed point
> format and the above commit forgot to convert back to integer
> coordinates before saving the values.  When we replace
> intel_pipe_set_base() with plane->funcs->update_plane() in a future
> patch, this bug becomes visible via the set_config entrypoint as well as
> update_plane.
> 
> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Testcase: igt/kms_plane
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8dbc0e9..1c1886e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11668,8 +11668,8 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	struct drm_rect *src = &state->src;
>  
>  	crtc->primary->fb = fb;
> -	crtc->x = src->x1;
> -	crtc->y = src->y1;
> +	crtc->x = src->x1 >> 16;
> +	crtc->y = src->y1 >> 16;

Oh I had the notion that we shift these down during the check phase.
But I see we only do that for sprites. We should pick one approach
and use it for all planes. I suppose keeping to the 16.16 format is
the better option in anticipation of the day when we have planes
which can do sub-pixel panning.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	intel_plane->crtc_x = state->orig_dst.x1;
>  	intel_plane->crtc_y = state->orig_dst.y1;
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/i915: remove intel_pipe_set_base() (v3)

[Matt Roper]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

After some refactor intel_primary_plane_setplane() does the same
as intel_pipe_set_base() so we can get rid of it and replace the calls
with intel_primary_plane_setplane().

v2: take Ville's comments:
	- get the right arguments for update_plane()
	- use drm_crtc_get_hv_timing()

v3 (by Matt):
 - Rebase to latest di-nightly codebase
 - Use primary->funcs->update_plane() in __intel_set_mode()
 - Use primary->funcs->disable_plane() in intel_crtc_disable()

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 124 ++++++++---------------------------
 1 file changed, 27 insertions(+), 97 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1c1886e..968ef0b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2890,71 +2890,6 @@ static void intel_update_pipe_size(struct intel_crtc *crtc)
 	crtc->config.pipe_src_h = adjusted_mode->crtc_vdisplay;
 }
 
-static int
-intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
-		    struct drm_framebuffer *fb)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum pipe pipe = intel_crtc->pipe;
-	struct drm_framebuffer *old_fb = crtc->primary->fb;
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_fb);
-	int ret;
-
-	if (intel_crtc_has_pending_flip(crtc)) {
-		DRM_ERROR("pipe is still busy with an old pageflip\n");
-		return -EBUSY;
-	}
-
-	/* no fb bound */
-	if (!fb) {
-		DRM_ERROR("No FB bound\n");
-		return 0;
-	}
-
-	if (intel_crtc->plane > INTEL_INFO(dev)->num_pipes) {
-		DRM_ERROR("no plane for crtc: plane %c, num_pipes %d\n",
-			  plane_name(intel_crtc->plane),
-			  INTEL_INFO(dev)->num_pipes);
-		return -EINVAL;
-	}
-
-	mutex_lock(&dev->struct_mutex);
-	ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, NULL);
-	if (ret == 0)
-		i915_gem_track_fb(old_obj, intel_fb_obj(fb),
-				  INTEL_FRONTBUFFER_PRIMARY(pipe));
-	mutex_unlock(&dev->struct_mutex);
-	if (ret != 0) {
-		DRM_ERROR("pin & fence failed\n");
-		return ret;
-	}
-
-	dev_priv->display.update_primary_plane(crtc, fb, x, y);
-
-	if (intel_crtc->active)
-		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
-
-	crtc->primary->fb = fb;
-	crtc->x = x;
-	crtc->y = y;
-
-	if (old_fb) {
-		if (intel_crtc->active && old_fb != fb)
-			intel_wait_for_vblank(dev, intel_crtc->pipe);
-		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(old_obj);
-		mutex_unlock(&dev->struct_mutex);
-	}
-
-	mutex_lock(&dev->struct_mutex);
-	intel_update_fbc(dev);
-	mutex_unlock(&dev->struct_mutex);
-
-	return 0;
-}
-
 static void intel_fdi_normal_train(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -5267,8 +5202,6 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct drm_connector *connector;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(crtc->primary->fb);
-	enum pipe pipe = to_intel_crtc(crtc)->pipe;
 
 	/* crtc should still be enabled when we disable it. */
 	WARN_ON(!crtc->enabled);
@@ -5277,14 +5210,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc_update_sarea(crtc, false);
 	dev_priv->display.off(crtc);
 
-	if (crtc->primary->fb) {
-		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(old_obj);
-		i915_gem_track_fb(old_obj, NULL,
-				  INTEL_FRONTBUFFER_PRIMARY(pipe));
-		mutex_unlock(&dev->struct_mutex);
-		crtc->primary->fb = NULL;
-	}
+	crtc->primary->funcs->disable_plane(crtc->primary);
 
 	/* Update computed state. */
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
@@ -9578,6 +9504,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	struct drm_framebuffer *old_fb = crtc->primary->fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_plane *primary = crtc->primary;
+	struct intel_plane *intel_plane = to_intel_plane(primary);
 	enum pipe pipe = intel_crtc->pipe;
 	struct intel_unpin_work *work;
 	struct intel_engine_cs *ring;
@@ -9737,7 +9665,15 @@ free_work:
 	if (ret == -EIO) {
 out_hang:
 		intel_crtc_wait_for_pending_flips(crtc);
-		ret = intel_pipe_set_base(crtc, crtc->x, crtc->y, fb);
+		ret = primary->funcs->update_plane(primary, crtc, fb,
+						   intel_plane->crtc_x,
+						   intel_plane->crtc_y,
+						   intel_plane->crtc_h,
+						   intel_plane->crtc_w,
+						   intel_plane->src_x,
+						   intel_plane->src_y,
+						   intel_plane->src_h,
+						   intel_plane->src_w);
 		if (ret == 0 && event) {
 			spin_lock_irq(&dev->event_lock);
 			drm_send_vblank_event(dev, pipe, event);
@@ -10913,26 +10849,15 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	 * on the DPLL.
 	 */
 	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
-		struct drm_framebuffer *old_fb = crtc->primary->fb;
-		struct drm_i915_gem_object *old_obj = intel_fb_obj(old_fb);
-		struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-
-		mutex_lock(&dev->struct_mutex);
-		ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, NULL);
-		if (ret != 0) {
-			DRM_ERROR("pin & fence failed\n");
-			mutex_unlock(&dev->struct_mutex);
-			goto done;
-		}
-		if (old_fb)
-			intel_unpin_fb_obj(old_obj);
-		i915_gem_track_fb(old_obj, obj,
-				  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
-		mutex_unlock(&dev->struct_mutex);
+		struct drm_plane *primary = intel_crtc->base.primary;
+		int vdisplay, hdisplay;
 
-		crtc->primary->fb = fb;
-		crtc->x = x;
-		crtc->y = y;
+		drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
+		ret = primary->funcs->update_plane(primary, &intel_crtc->base,
+						   fb, 0, 0,
+						   hdisplay, vdisplay,
+						   x << 16, y << 16,
+						   hdisplay << 16, vdisplay << 16);
 	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
@@ -11398,11 +11323,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 					   disable_pipes);
 	} else if (config->fb_changed) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
+		struct drm_plane *primary = set->crtc->primary;
+		int vdisplay, hdisplay;
 
 		intel_crtc_wait_for_pending_flips(set->crtc);
 
-		ret = intel_pipe_set_base(set->crtc,
-					  set->x, set->y, set->fb);
+		drm_crtc_get_hv_timing(set->mode, &hdisplay, &vdisplay);
+		ret = primary->funcs->update_plane(primary, set->crtc, set->fb,
+						   0, 0, hdisplay, vdisplay,
+						   set->x << 16, set->y << 16,
+						   hdisplay << 16, vdisplay << 16);
 
 		/*
 		 * We need to make sure the primary plane is re-enabled if it
-- 
1.8.5.1

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

Re: [PATCH 5/5] drm/i915: remove intel_pipe_set_base() (v3)

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform: baseline_drm_intel_nightly_pass_rate->patch_applied_pass_rate
BYT: pass/total=290/291->291/291
PNV: pass/total=356/356->356/356
ILK: pass/total=371/372->371/372
IVB: pass/total=544/546->545/546
SNB: pass/total=424/425->424/425
HSW: pass/total=595/595->595/595
BDW: pass/total=432/435->433/435
-------------------------------------Detailed-------------------------------------
test_platform: test_suite, test_case, result_with_drm_intel_nightly(count, machine_id...)...->result_with_patch_applied(count, machine_id)...
BYT: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(1, M36)PASS(9, M31M36) -> TIMEOUT(3, M31)PASS(1, M31)
ILK: Intel_gpu_tools, igt_kms_flip_bcs-wf_vblank-vs-dpms, PASS(7, M37M26) -> DMESG_WARN(1, M26)PASS(3, M26)
ILK: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(1, M37)PASS(9, M26M37) -> FAIL(3, M26)PASS(1, M26)
IVB: Intel_gpu_tools, igt_kms_3d, DMESG_WARN(3, M4)PASS(1, M21) -> DMESG_WARN(1, M4)PASS(3, M4)
IVB: Intel_gpu_tools, igt_kms_cursor_crc_cursor-256x256-random, DMESG_WARN(1, M21)PASS(9, M21M34M4) -> PASS(4, M4)
IVB: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(1, M21)PASS(9, M21M34M4) -> TIMEOUT(3, M4)PASS(1, M4)
SNB: Intel_gpu_tools, igt_kms_3d, DMESG_WARN(3, M22)PASS(1, M35) -> DMESG_WARN(1, M22)PASS(3, M22)
SNB: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(1, M35)PASS(9, M35M22) -> TIMEOUT(3, M22)PASS(1, M22)
BDW: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, TIMEOUT(1, M28)PASS(9, M30M28) -> TIMEOUT(3, M30)PASS(1, M30)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: remove intel_pipe_set_base() (v3)

[Ville Syrjälä]

On Mon, Nov 17, 2014 at 06:10:39PM -0800, Matt Roper wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> After some refactor intel_primary_plane_setplane() does the same
> as intel_pipe_set_base() so we can get rid of it and replace the calls
> with intel_primary_plane_setplane().
> 
> v2: take Ville's comments:
> 	- get the right arguments for update_plane()
> 	- use drm_crtc_get_hv_timing()
> 
> v3 (by Matt):
>  - Rebase to latest di-nightly codebase
>  - Use primary->funcs->update_plane() in __intel_set_mode()
>  - Use primary->funcs->disable_plane() in intel_crtc_disable()
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 124 ++++++++---------------------------
>  1 file changed, 27 insertions(+), 97 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1c1886e..968ef0b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2890,71 +2890,6 @@ static void intel_update_pipe_size(struct intel_crtc *crtc)
>  	crtc->config.pipe_src_h = adjusted_mode->crtc_vdisplay;
>  }
>  
> -static int
> -intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> -		    struct drm_framebuffer *fb)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	enum pipe pipe = intel_crtc->pipe;
> -	struct drm_framebuffer *old_fb = crtc->primary->fb;
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_fb);
> -	int ret;
> -
> -	if (intel_crtc_has_pending_flip(crtc)) {
> -		DRM_ERROR("pipe is still busy with an old pageflip\n");
> -		return -EBUSY;
> -	}
> -
> -	/* no fb bound */
> -	if (!fb) {
> -		DRM_ERROR("No FB bound\n");
> -		return 0;
> -	}
> -
> -	if (intel_crtc->plane > INTEL_INFO(dev)->num_pipes) {
> -		DRM_ERROR("no plane for crtc: plane %c, num_pipes %d\n",
> -			  plane_name(intel_crtc->plane),
> -			  INTEL_INFO(dev)->num_pipes);
> -		return -EINVAL;
> -	}
> -
> -	mutex_lock(&dev->struct_mutex);
> -	ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, NULL);
> -	if (ret == 0)
> -		i915_gem_track_fb(old_obj, intel_fb_obj(fb),
> -				  INTEL_FRONTBUFFER_PRIMARY(pipe));
> -	mutex_unlock(&dev->struct_mutex);
> -	if (ret != 0) {
> -		DRM_ERROR("pin & fence failed\n");
> -		return ret;
> -	}
> -
> -	dev_priv->display.update_primary_plane(crtc, fb, x, y);
> -
> -	if (intel_crtc->active)
> -		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
> -
> -	crtc->primary->fb = fb;
> -	crtc->x = x;
> -	crtc->y = y;
> -
> -	if (old_fb) {
> -		if (intel_crtc->active && old_fb != fb)
> -			intel_wait_for_vblank(dev, intel_crtc->pipe);
> -		mutex_lock(&dev->struct_mutex);
> -		intel_unpin_fb_obj(old_obj);
> -		mutex_unlock(&dev->struct_mutex);
> -	}
> -
> -	mutex_lock(&dev->struct_mutex);
> -	intel_update_fbc(dev);
> -	mutex_unlock(&dev->struct_mutex);
> -
> -	return 0;
> -}
> -
>  static void intel_fdi_normal_train(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -5267,8 +5202,6 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_connector *connector;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(crtc->primary->fb);
> -	enum pipe pipe = to_intel_crtc(crtc)->pipe;
>  
>  	/* crtc should still be enabled when we disable it. */
>  	WARN_ON(!crtc->enabled);
> @@ -5277,14 +5210,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
>  	intel_crtc_update_sarea(crtc, false);
>  	dev_priv->display.off(crtc);
>  
> -	if (crtc->primary->fb) {
> -		mutex_lock(&dev->struct_mutex);
> -		intel_unpin_fb_obj(old_obj);
> -		i915_gem_track_fb(old_obj, NULL,
> -				  INTEL_FRONTBUFFER_PRIMARY(pipe));
> -		mutex_unlock(&dev->struct_mutex);
> -		crtc->primary->fb = NULL;
> -	}
> +	crtc->primary->funcs->disable_plane(crtc->primary);
>  
>  	/* Update computed state. */
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> @@ -9578,6 +9504,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	struct drm_framebuffer *old_fb = crtc->primary->fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_plane *primary = crtc->primary;
> +	struct intel_plane *intel_plane = to_intel_plane(primary);
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct intel_unpin_work *work;
>  	struct intel_engine_cs *ring;
> @@ -9737,7 +9665,15 @@ free_work:
>  	if (ret == -EIO) {
>  out_hang:
>  		intel_crtc_wait_for_pending_flips(crtc);

This should alread get called from the .update_plane() hook.

> -		ret = intel_pipe_set_base(crtc, crtc->x, crtc->y, fb);
> +		ret = primary->funcs->update_plane(primary, crtc, fb,
> +						   intel_plane->crtc_x,
> +						   intel_plane->crtc_y,
> +						   intel_plane->crtc_h,
> +						   intel_plane->crtc_w,
> +						   intel_plane->src_x,
> +						   intel_plane->src_y,
> +						   intel_plane->src_h,
> +						   intel_plane->src_w);
>  		if (ret == 0 && event) {
>  			spin_lock_irq(&dev->event_lock);
>  			drm_send_vblank_event(dev, pipe, event);
> @@ -10913,26 +10849,15 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	 * on the DPLL.
>  	 */
>  	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
> -		struct drm_framebuffer *old_fb = crtc->primary->fb;
> -		struct drm_i915_gem_object *old_obj = intel_fb_obj(old_fb);
> -		struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -
> -		mutex_lock(&dev->struct_mutex);
> -		ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, NULL);
> -		if (ret != 0) {
> -			DRM_ERROR("pin & fence failed\n");
> -			mutex_unlock(&dev->struct_mutex);
> -			goto done;
> -		}
> -		if (old_fb)
> -			intel_unpin_fb_obj(old_obj);
> -		i915_gem_track_fb(old_obj, obj,
> -				  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
> -		mutex_unlock(&dev->struct_mutex);
> +		struct drm_plane *primary = intel_crtc->base.primary;
> +		int vdisplay, hdisplay;
>  
> -		crtc->primary->fb = fb;
> -		crtc->x = x;
> -		crtc->y = y;
> +		drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
> +		ret = primary->funcs->update_plane(primary, &intel_crtc->base,
> +						   fb, 0, 0,
> +						   hdisplay, vdisplay,
> +						   x << 16, y << 16,
> +						   hdisplay << 16, vdisplay << 16);
>  	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> @@ -11398,11 +11323,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  					   disable_pipes);
>  	} else if (config->fb_changed) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
> +		struct drm_plane *primary = set->crtc->primary;
> +		int vdisplay, hdisplay;
>  
>  		intel_crtc_wait_for_pending_flips(set->crtc);

ditto

Otherwise this looks nice. So assuming it works and we didn't miss anything important,
and we can just sort out the remaining stereo doubling issue, I think this should fix
some of the disappearing primary plane issues we have currently.

I'm not quite sure if we're now close enough to unify all plane handling in
intel_crtc_{enable,disable}_planes(). The trick with those is that they
shoudln't affect the user visible state of the plane, so we need to
first make sure that's kept separate. So I'm thinking we would want to
virtualize the check/prepare/commit hooks and make sure all the user visible
state is pulled out from them into the .update_plane/.disable_plane hooks.
Does that sound like what other people had in mind?

There's also the primary plane handling in the sprite code, but cleaning
that up that may require a bit more work since that's actually an internal
special case of a nuclear flip.

>  
> -		ret = intel_pipe_set_base(set->crtc,
> -					  set->x, set->y, set->fb);
> +		drm_crtc_get_hv_timing(set->mode, &hdisplay, &vdisplay);
> +		ret = primary->funcs->update_plane(primary, set->crtc, set->fb,
> +						   0, 0, hdisplay, vdisplay,
> +						   set->x << 16, set->y << 16,
> +						   hdisplay << 16, vdisplay << 16);
>  
>  		/*
>  		 * We need to make sure the primary plane is re-enabled if it
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: remove intel_pipe_set_base() (v3)

[Jesse Barnes]

On Mon, 17 Nov 2014 18:10:39 -0800
Matt Roper <matthew.d.roper@intel.com> wrote:

> -static int
> -intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> -		    struct drm_framebuffer *fb)
> -{

I'm gonna miss this old function...  But on the plus side I won't
confuse pipe_set_base and set_pipe_base anymore, always got that wrong
when searching.

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