[PATCH 0/9] i915 display refactoring (v3)

[PATCH 0/9] i915 display refactoring (v3)

[Matt Roper]

Another iteration of the i915 display refactoring work, with a few additional
patches added to the end which should help simplify the transition to the
atomic plane helpers.

The previous version of this patch series was posted here:
  http://lists.freedesktop.org/archives/intel-gfx/2014-November/055794.html

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

Matt Roper (6):
  drm/i915: Introduce intel_prepare_cursor_plane()
  drm/i915: Make intel_plane_state subclass drm_plane_state
  drm/i915: Consolidate plane 'prepare' functions
  drm/i915: Consolidate plane 'cleanup' operations
  drm/i915: Consolidate top-level .update_plane() handlers
  drm/i915: Make all plane disables use 'update_plane'

 drivers/gpu/drm/drm_crtc.c           |  32 +-
 drivers/gpu/drm/drm_modes.c          |  24 +-
 drivers/gpu/drm/i915/intel_display.c | 604 ++++++++++++++---------------------
 drivers/gpu/drm/i915/intel_drv.h     |  18 +-
 drivers/gpu/drm/i915/intel_sprite.c  | 179 ++---------
 include/drm/drm_crtc.h               |   2 +
 include/drm/drm_modes.h              |   3 +
 7 files changed, 314 insertions(+), 548 deletions(-)

-- 
1.8.5.1

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

[PATCH 1/9] drm: add helper to get crtc timings (v4)

[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)

v4 (by Matt):
 - Drop stereo doubling function again; add 'stereo only' flag
   to drm_mode_set_crtcinfo() instead (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           | 32 ++++++++++++++++++++++----------
 drivers/gpu/drm/drm_modes.c          | 24 ++++++++++++++----------
 drivers/gpu/drm/i915/intel_display.c |  6 +++---
 include/drm/drm_crtc.h               |  2 ++
 include/drm/drm_modes.h              |  3 +++
 5 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 589a921..f06f1b4 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2499,6 +2499,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_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE_ONLY);
+	*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
@@ -2515,16 +2536,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/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 6d8b941..fd5479b 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -765,18 +765,22 @@ void drm_mode_set_crtcinfo(struct drm_display_mode *p, int adjust_flags)
 		}
 	}
 
-	if (p->flags & DRM_MODE_FLAG_DBLSCAN) {
-		p->crtc_vdisplay *= 2;
-		p->crtc_vsync_start *= 2;
-		p->crtc_vsync_end *= 2;
-		p->crtc_vtotal *= 2;
+	if (!(adjust_flags & CRTC_NO_DBLSCAN)) {
+		if (p->flags & DRM_MODE_FLAG_DBLSCAN) {
+			p->crtc_vdisplay *= 2;
+			p->crtc_vsync_start *= 2;
+			p->crtc_vsync_end *= 2;
+			p->crtc_vtotal *= 2;
+		}
 	}
 
-	if (p->vscan > 1) {
-		p->crtc_vdisplay *= p->vscan;
-		p->crtc_vsync_start *= p->vscan;
-		p->crtc_vsync_end *= p->vscan;
-		p->crtc_vtotal *= p->vscan;
+	if (!(adjust_flags & CRTC_NO_VSCAN)) {
+		if (p->vscan > 1) {
+			p->crtc_vdisplay *= p->vscan;
+			p->crtc_vsync_start *= p->vscan;
+			p->crtc_vsync_end *= p->vscan;
+			p->crtc_vtotal *= p->vscan;
+		}
 	}
 
 	if (adjust_flags & CRTC_STEREO_DOUBLE) {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a3d2a44..b87aeab 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10205,9 +10205,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 b459e8f..6e46c5d 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1140,6 +1140,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,
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 91d0582..8f17811 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -90,6 +90,9 @@ enum drm_mode_status {
 
 #define CRTC_INTERLACE_HALVE_V	(1 << 0) /* halve V values for interlacing */
 #define CRTC_STEREO_DOUBLE	(1 << 1) /* adjust timings for stereo modes */
+#define CRTC_NO_DBLSCAN		(1 << 2) /* don't adjust doublescan */
+#define CRTC_NO_VSCAN		(1 << 3) /* don't adjust doublescan */
+#define CRTC_STEREO_DOUBLE_ONLY	(CRTC_NO_DBLSCAN | CRTC_NO_VSCAN)
 
 #define DRM_MODE_FLAG_3D_MAX	DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF
 
-- 
1.8.5.1

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

Re: [Intel-gfx] [PATCH 1/9] drm: add helper to get crtc timings (v4)

[Ander Conselvan de Oliveira]

On 11/24/2014 09:52 PM, 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)
>
> v4 (by Matt):
>   - Drop stereo doubling function again; add 'stereo only' flag
>     to drm_mode_set_crtcinfo() instead (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           | 32 ++++++++++++++++++++++----------
>   drivers/gpu/drm/drm_modes.c          | 24 ++++++++++++++----------
>   drivers/gpu/drm/i915/intel_display.c |  6 +++---
>   include/drm/drm_crtc.h               |  2 ++
>   include/drm/drm_modes.h              |  3 +++
>   5 files changed, 44 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 589a921..f06f1b4 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2499,6 +2499,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_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE_ONLY);
> +	*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
> @@ -2515,16 +2536,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);

This changes the behavior slightly since before, for stereo modes, 
dbl_scan and vscan would affect hdisplay and vdisplay. If I understand 
correctly the old behavior is a bug, but it would be good to state that 
the change is intentional.

>
>   	if (crtc->invert_dimensions)
>   		swap(hdisplay, vdisplay);
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 6d8b941..fd5479b 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -765,18 +765,22 @@ void drm_mode_set_crtcinfo(struct drm_display_mode *p, int adjust_flags)
>   		}
>   	}
>
> -	if (p->flags & DRM_MODE_FLAG_DBLSCAN) {
> -		p->crtc_vdisplay *= 2;
> -		p->crtc_vsync_start *= 2;
> -		p->crtc_vsync_end *= 2;
> -		p->crtc_vtotal *= 2;
> +	if (!(adjust_flags & CRTC_NO_DBLSCAN)) {
> +		if (p->flags & DRM_MODE_FLAG_DBLSCAN) {
> +			p->crtc_vdisplay *= 2;
> +			p->crtc_vsync_start *= 2;
> +			p->crtc_vsync_end *= 2;
> +			p->crtc_vtotal *= 2;
> +		}
>   	}
>
> -	if (p->vscan > 1) {
> -		p->crtc_vdisplay *= p->vscan;
> -		p->crtc_vsync_start *= p->vscan;
> -		p->crtc_vsync_end *= p->vscan;
> -		p->crtc_vtotal *= p->vscan;
> +	if (!(adjust_flags & CRTC_NO_VSCAN)) {
> +		if (p->vscan > 1) {
> +			p->crtc_vdisplay *= p->vscan;
> +			p->crtc_vsync_start *= p->vscan;
> +			p->crtc_vsync_end *= p->vscan;
> +			p->crtc_vtotal *= p->vscan;
> +		}
>   	}
>
>   	if (adjust_flags & CRTC_STEREO_DOUBLE) {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a3d2a44..b87aeab 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10205,9 +10205,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);

Same thing here.


>   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 b459e8f..6e46c5d 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1140,6 +1140,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,
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index 91d0582..8f17811 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -90,6 +90,9 @@ enum drm_mode_status {
>
>   #define CRTC_INTERLACE_HALVE_V	(1 << 0) /* halve V values for interlacing */
>   #define CRTC_STEREO_DOUBLE	(1 << 1) /* adjust timings for stereo modes */
> +#define CRTC_NO_DBLSCAN		(1 << 2) /* don't adjust doublescan */
> +#define CRTC_NO_VSCAN		(1 << 3) /* don't adjust doublescan */
> +#define CRTC_STEREO_DOUBLE_ONLY	(CRTC_NO_DBLSCAN | CRTC_NO_VSCAN)

The kernel doc of drm_mode_set_crtcinfo() has a brief explanation of 
what the flags above do. I think it would be proper to update that with 
the new additions.

Cheers,
Ander

>
>   #define DRM_MODE_FLAG_3D_MAX	DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF
>
>

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

[PATCH 2/9] 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 b87aeab..81894be 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8353,109 +8353,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 = to_i915(dev);
-	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)
 {
@@ -12014,7 +11911,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
@@ -12078,12 +11976,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;
@@ -12098,18 +11999,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 3/9] drm/i915: remove intel_pipe_set_base() (v4)

[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()

v4 (by Matt):
 - Drop redundant calls to intel_crtc_wait_for_pending_flips() before
   calling update_plane() (Ville)

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Acked-and-mourned-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c | 129 ++++++++---------------------------
 1 file changed, 28 insertions(+), 101 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 81894be..17788f8 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;
@@ -5248,8 +5183,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);
@@ -5257,14 +5190,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	dev_priv->display.crtc_disable(crtc);
 	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) {
@@ -9621,6 +9547,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;
@@ -9779,8 +9707,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);
@@ -10962,26 +10897,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);
+		struct drm_plane *primary = intel_crtc->base.primary;
+		int vdisplay, hdisplay;
 
-		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);
-
-		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. */
@@ -11447,11 +11371,14 @@ 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);
-
-		intel_crtc_wait_for_pending_flips(set->crtc);
-
-		ret = intel_pipe_set_base(set->crtc,
-					  set->x, set->y, set->fb);
+		struct drm_plane *primary = set->crtc->primary;
+		int vdisplay, hdisplay;
+
+		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 3/9] drm/i915: remove intel_pipe_set_base() (v4)

[Ander Conselvan de Oliveira]

Reviewed-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

On 11/24/2014 09:53 PM, 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()
> 
> v4 (by Matt):
>   - Drop redundant calls to intel_crtc_wait_for_pending_flips() before
>     calling update_plane() (Ville)
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Acked-and-mourned-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 129 ++++++++---------------------------
>   1 file changed, 28 insertions(+), 101 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 81894be..17788f8 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;
> @@ -5248,8 +5183,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);
> @@ -5257,14 +5190,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
>   	dev_priv->display.crtc_disable(crtc);
>   	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) {
> @@ -9621,6 +9547,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;
> @@ -9779,8 +9707,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);
> @@ -10962,26 +10897,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);
> +		struct drm_plane *primary = intel_crtc->base.primary;
> +		int vdisplay, hdisplay;
>   
> -		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);
> -
> -		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. */
> @@ -11447,11 +11371,14 @@ 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);
> -
> -		intel_crtc_wait_for_pending_flips(set->crtc);
> -
> -		ret = intel_pipe_set_base(set->crtc,
> -					  set->x, set->y, set->fb);
> +		struct drm_plane *primary = set->crtc->primary;
> +		int vdisplay, hdisplay;
> +
> +		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
> 

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

[PATCH 4/9] drm/i915: Introduce intel_prepare_cursor_plane()

[Matt Roper]

Primary and sprite planes have already been refactored to include a
'prepare' step which handles all the commit-time operations that could
fail (i.e., pinning buffers and such).  Refactor the cursor commit in a
similar manner.

For simplicity and consistency with other plane types, we also switch to
using intel_pin_and_fence_fb_obj() to perform our pinning for
non-physical cursors.  This will allow us to more easily migrate the
code into the atomic 'begin' handler in a plane-agnostic manner in a
future patchset.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 114 ++++++++++++++---------------------
 1 file changed, 44 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 17788f8..ff94e43 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11899,19 +11899,51 @@ intel_check_cursor_plane(struct drm_plane *plane,
 }
 
 static int
+intel_prepare_cursor_plane(struct drm_plane *plane,
+			   struct intel_plane_state *state)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_framebuffer *fb = state->fb;
+	struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
+	enum pipe pipe = intel_crtc->pipe;
+	int ret = 0;
+
+	if (old_obj != obj) {
+		/* we only need to pin inside GTT if cursor is non-phy */
+		mutex_lock(&dev->struct_mutex);
+		if (!INTEL_INFO(dev)->cursor_needs_physical) {
+			if (obj)
+				ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
+			if (ret == 0)
+				i915_gem_track_fb(intel_crtc->cursor_bo, obj,
+						  INTEL_FRONTBUFFER_CURSOR(pipe));
+		} else {
+			int align = IS_I830(dev) ? 16 * 1024 : 256;
+			if (obj)
+				ret = i915_gem_object_attach_phys(obj, align);
+			if (ret)
+				DRM_DEBUG_KMS("failed to attach phys object\n");
+		}
+		mutex_unlock(&dev->struct_mutex);
+	}
+
+	return ret;
+}
+
+static void
 intel_commit_cursor_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->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);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	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;
@@ -11929,75 +11961,18 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	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");
+	if (!obj)
 		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;
-		}
-
+	else if (!INTEL_INFO(dev)->cursor_needs_physical)
 		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;
-		}
+	else
 		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);
-
 	intel_crtc->cursor_addr = addr;
 	intel_crtc->cursor_bo = obj;
 update:
@@ -12013,13 +11988,6 @@ update:
 
 		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
@@ -12060,7 +12028,13 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
-	return intel_commit_cursor_plane(plane, &state);
+	ret = intel_prepare_cursor_plane(plane, &state);
+	if (ret)
+		return ret;
+
+	intel_commit_cursor_plane(plane, &state);
+
+	return 0;
 }
 
 static const struct drm_plane_funcs intel_cursor_plane_funcs = {
-- 
1.8.5.1

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

Re: [PATCH 4/9] drm/i915: Introduce intel_prepare_cursor_plane()

[Ander Conselvan de Oliveira]

On 11/24/2014 09:53 PM, Matt Roper wrote:
> Primary and sprite planes have already been refactored to include a
> 'prepare' step which handles all the commit-time operations that could
> fail (i.e., pinning buffers and such).  Refactor the cursor commit in a
> similar manner.
>
> For simplicity and consistency with other plane types, we also switch to
> using intel_pin_and_fence_fb_obj() to perform our pinning for
> non-physical cursors.  This will allow us to more easily migrate the
> code into the atomic 'begin' handler in a plane-agnostic manner in a
> future patchset.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 114 ++++++++++++++---------------------
>   1 file changed, 44 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 17788f8..ff94e43 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11899,19 +11899,51 @@ intel_check_cursor_plane(struct drm_plane *plane,
>   }
>
>   static int
> +intel_prepare_cursor_plane(struct drm_plane *plane,
> +			   struct intel_plane_state *state)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_framebuffer *fb = state->fb;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> +	enum pipe pipe = intel_crtc->pipe;
> +	int ret = 0;
> +
> +	if (old_obj != obj) {
> +		/* we only need to pin inside GTT if cursor is non-phy */
> +		mutex_lock(&dev->struct_mutex);
> +		if (!INTEL_INFO(dev)->cursor_needs_physical) {
> +			if (obj)
> +				ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> +			if (ret == 0)
> +				i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> +						  INTEL_FRONTBUFFER_CURSOR(pipe));

Shouldn't the frontbuffer bits be updated in the else case too? At least 
they were, prior to this patch.

> +		} else {
> +			int align = IS_I830(dev) ? 16 * 1024 : 256;
> +			if (obj)
> +				ret = i915_gem_object_attach_phys(obj, align);
> +			if (ret)
> +				DRM_DEBUG_KMS("failed to attach phys object\n");
> +		}
> +		mutex_unlock(&dev->struct_mutex);
> +	}
> +
> +	return ret;
> +}
> +
> +static void
>   intel_commit_cursor_plane(struct drm_plane *plane,
>   			  struct intel_plane_state *state)
>   {
>   	struct drm_crtc *crtc = state->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);
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	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;
> @@ -11929,75 +11961,18 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   	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");
> +	if (!obj)
>   		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;
> -		}
> -
> +	else if (!INTEL_INFO(dev)->cursor_needs_physical)
>   		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;
> -		}
> +	else
>   		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);

This is now being called without holding dev->struct_mutex. I don't know 
if that is necessary or not.


Ander

>   	}
>
> -	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:
> @@ -12013,13 +11988,6 @@ update:
>
>   		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
> @@ -12060,7 +12028,13 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
>   	if (ret)
>   		return ret;
>
> -	return intel_commit_cursor_plane(plane, &state);
> +	ret = intel_prepare_cursor_plane(plane, &state);
> +	if (ret)
> +		return ret;
> +
> +	intel_commit_cursor_plane(plane, &state);
> +
> +	return 0;
>   }
>
>   static const struct drm_plane_funcs intel_cursor_plane_funcs = {
>

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

Re: [PATCH 4/9] drm/i915: Introduce intel_prepare_cursor_plane()

[Ander Conselvan de Oliveira]

On 11/28/2014 02:15 PM, Ander Conselvan de Oliveira wrote:
> On 11/24/2014 09:53 PM, Matt Roper wrote:
>> Primary and sprite planes have already been refactored to include a
>> 'prepare' step which handles all the commit-time operations that could
>> fail (i.e., pinning buffers and such).  Refactor the cursor commit in a
>> similar manner.
>>
>> For simplicity and consistency with other plane types, we also switch to
>> using intel_pin_and_fence_fb_obj() to perform our pinning for
>> non-physical cursors.  This will allow us to more easily migrate the
>> code into the atomic 'begin' handler in a plane-agnostic manner in a
>> future patchset.
>>
>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 114
>> ++++++++++++++---------------------
>>   1 file changed, 44 insertions(+), 70 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 17788f8..ff94e43 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11899,19 +11899,51 @@ intel_check_cursor_plane(struct drm_plane
>> *plane,
>>   }
>>
>>   static int
>> +intel_prepare_cursor_plane(struct drm_plane *plane,
>> +               struct intel_plane_state *state)
>> +{
>> +    struct drm_device *dev = plane->dev;
>> +    struct drm_framebuffer *fb = state->fb;
>> +    struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
>> +    struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>> +    struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
>> +    enum pipe pipe = intel_crtc->pipe;
>> +    int ret = 0;
>> +
>> +    if (old_obj != obj) {
>> +        /* we only need to pin inside GTT if cursor is non-phy */
>> +        mutex_lock(&dev->struct_mutex);
>> +        if (!INTEL_INFO(dev)->cursor_needs_physical) {
>> +            if (obj)
>> +                ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
>> +            if (ret == 0)
>> +                i915_gem_track_fb(intel_crtc->cursor_bo, obj,
>> +                          INTEL_FRONTBUFFER_CURSOR(pipe));
>
> Shouldn't the frontbuffer bits be updated in the else case too? At least
> they were, prior to this patch.
>
>> +        } else {
>> +            int align = IS_I830(dev) ? 16 * 1024 : 256;
>> +            if (obj)
>> +                ret = i915_gem_object_attach_phys(obj, align);
>> +            if (ret)
>> +                DRM_DEBUG_KMS("failed to attach phys object\n");
>> +        }
>> +        mutex_unlock(&dev->struct_mutex);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static void
>>   intel_commit_cursor_plane(struct drm_plane *plane,
>>                 struct intel_plane_state *state)
>>   {
>>       struct drm_crtc *crtc = state->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);
>>       struct intel_plane *intel_plane = to_intel_plane(plane);
>>       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;
>> @@ -11929,75 +11961,18 @@ intel_commit_cursor_plane(struct drm_plane
>> *plane,
>>       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");
>> +    if (!obj)
>>           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;
>> -        }
>> -
>> +    else if (!INTEL_INFO(dev)->cursor_needs_physical)
>>           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;
>> -        }
>> +    else
>>           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);
>
> This is now being called without holding dev->struct_mutex. I don't know
> if that is necessary or not.

While looking at patch 7 in the series, I noticed that this call is 
replaced by a call to intel_unpin_fb_obj() eventually, and that one 
actually needs dev->struct_mutex to be held. The difference is that the 
latter call also deals with fence registers. Since 
intel_pin_and_fence_fb_obj() might setup fence registers I think we 
should use  intel_unpin_fb_obj() here for symmetry, even though a fence 
register won't actually be set up because the cursor bo is never tiled.

Ander

>>       }
>>
>> -    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:
>> @@ -12013,13 +11988,6 @@ update:
>>
>>           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
>> @@ -12060,7 +12028,13 @@ intel_cursor_plane_update(struct drm_plane
>> *plane, struct drm_crtc *crtc,
>>       if (ret)
>>           return ret;
>>
>> -    return intel_commit_cursor_plane(plane, &state);
>> +    ret = intel_prepare_cursor_plane(plane, &state);
>> +    if (ret)
>> +        return ret;
>> +
>> +    intel_commit_cursor_plane(plane, &state);
>> +
>> +    return 0;
>>   }
>>
>>   static const struct drm_plane_funcs intel_cursor_plane_funcs = {
>>
>

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

Re: [PATCH 4/9] drm/i915: Introduce intel_prepare_cursor_plane()

[Daniel Vetter]

On Fri, Nov 28, 2014 at 02:15:08PM +0200, Ander Conselvan de Oliveira wrote:
> On 11/24/2014 09:53 PM, Matt Roper wrote:
> >Primary and sprite planes have already been refactored to include a
> >'prepare' step which handles all the commit-time operations that could
> >fail (i.e., pinning buffers and such).  Refactor the cursor commit in a
> >similar manner.
> >
> >For simplicity and consistency with other plane types, we also switch to
> >using intel_pin_and_fence_fb_obj() to perform our pinning for
> >non-physical cursors.  This will allow us to more easily migrate the
> >code into the atomic 'begin' handler in a plane-agnostic manner in a
> >future patchset.
> >
> >Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >---
> >  drivers/gpu/drm/i915/intel_display.c | 114 ++++++++++++++---------------------
> >  1 file changed, 44 insertions(+), 70 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >index 17788f8..ff94e43 100644
> >--- a/drivers/gpu/drm/i915/intel_display.c
> >+++ b/drivers/gpu/drm/i915/intel_display.c
> >@@ -11899,19 +11899,51 @@ intel_check_cursor_plane(struct drm_plane *plane,
> >  }
> >
> >  static int
> >+intel_prepare_cursor_plane(struct drm_plane *plane,
> >+			   struct intel_plane_state *state)
> >+{
> >+	struct drm_device *dev = plane->dev;
> >+	struct drm_framebuffer *fb = state->fb;
> >+	struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
> >+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> >+	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> >+	enum pipe pipe = intel_crtc->pipe;
> >+	int ret = 0;
> >+
> >+	if (old_obj != obj) {
> >+		/* we only need to pin inside GTT if cursor is non-phy */
> >+		mutex_lock(&dev->struct_mutex);
> >+		if (!INTEL_INFO(dev)->cursor_needs_physical) {
> >+			if (obj)
> >+				ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> >+			if (ret == 0)
> >+				i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> >+						  INTEL_FRONTBUFFER_CURSOR(pipe));
> 
> Shouldn't the frontbuffer bits be updated in the else case too? At least
> they were, prior to this patch.

gem_track_fb must be called always when you exchange plane buffer (even
when the plane is disabled). It won't hurt too badly since the affected
platforms don't support fbc/psr or anything which needs frontbuffer
tracking. Well more correctly the hw can do fbc, but I don't think we'll
ever enable it. But that's aside, still a bug.

> >+		} else {
> >+			int align = IS_I830(dev) ? 16 * 1024 : 256;
> >+			if (obj)
> >+				ret = i915_gem_object_attach_phys(obj, align);
> >+			if (ret)
> >+				DRM_DEBUG_KMS("failed to attach phys object\n");
> >+		}
> >+		mutex_unlock(&dev->struct_mutex);
> >+	}
> >+
> >+	return ret;
> >+}
> >+
> >+static void
> >  intel_commit_cursor_plane(struct drm_plane *plane,
> >  			  struct intel_plane_state *state)
> >  {
> >  	struct drm_crtc *crtc = state->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);
> >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> >  	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;
> >@@ -11929,75 +11961,18 @@ intel_commit_cursor_plane(struct drm_plane *plane,
> >  	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");
> >+	if (!obj)
> >  		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;
> >-		}
> >-
> >+	else if (!INTEL_INFO(dev)->cursor_needs_physical)
> >  		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;
> >-		}
> >+	else
> >  		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);
> 
> This is now being called without holding dev->struct_mutex. I don't know if
> that is necessary or not.

unpin requires dev->struct_mutex. Same for gem_track_fb actually.
-Daniel

> 
> 
> Ander
> 
> >  	}
> >
> >-	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:
> >@@ -12013,13 +11988,6 @@ update:
> >
> >  		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
> >@@ -12060,7 +12028,13 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
> >  	if (ret)
> >  		return ret;
> >
> >-	return intel_commit_cursor_plane(plane, &state);
> >+	ret = intel_prepare_cursor_plane(plane, &state);
> >+	if (ret)
> >+		return ret;
> >+
> >+	intel_commit_cursor_plane(plane, &state);
> >+
> >+	return 0;
> >  }
> >
> >  static const struct drm_plane_funcs intel_cursor_plane_funcs = {
> >
> 
> _______________________________________________
> 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 4/9] drm/i915: Introduce intel_prepare_cursor_plane()

[Ander Conselvan de Oliveira]

On 11/24/2014 09:53 PM, Matt Roper wrote:
> Primary and sprite planes have already been refactored to include a
> 'prepare' step which handles all the commit-time operations that could
> fail (i.e., pinning buffers and such).  Refactor the cursor commit in a
> similar manner.
>
> For simplicity and consistency with other plane types, we also switch to
> using intel_pin_and_fence_fb_obj() to perform our pinning for
> non-physical cursors.  This will allow us to more easily migrate the
> code into the atomic 'begin' handler in a plane-agnostic manner in a
> future patchset.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 114 ++++++++++++++---------------------
>   1 file changed, 44 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 17788f8..ff94e43 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11899,19 +11899,51 @@ intel_check_cursor_plane(struct drm_plane *plane,
>   }
>
>   static int
> +intel_prepare_cursor_plane(struct drm_plane *plane,
> +			   struct intel_plane_state *state)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_framebuffer *fb = state->fb;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);

I don't see where plane->fb is being updated. I think this needs to be 
done in intel_commit_cursor_plane().

Ander

> +	enum pipe pipe = intel_crtc->pipe;
> +	int ret = 0;
> +
> +	if (old_obj != obj) {
> +		/* we only need to pin inside GTT if cursor is non-phy */
> +		mutex_lock(&dev->struct_mutex);
> +		if (!INTEL_INFO(dev)->cursor_needs_physical) {
> +			if (obj)
> +				ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> +			if (ret == 0)
> +				i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> +						  INTEL_FRONTBUFFER_CURSOR(pipe));
> +		} else {
> +			int align = IS_I830(dev) ? 16 * 1024 : 256;
> +			if (obj)
> +				ret = i915_gem_object_attach_phys(obj, align);
> +			if (ret)
> +				DRM_DEBUG_KMS("failed to attach phys object\n");
> +		}
> +		mutex_unlock(&dev->struct_mutex);
> +	}
> +
> +	return ret;
> +}
> +
> +static void
>   intel_commit_cursor_plane(struct drm_plane *plane,
>   			  struct intel_plane_state *state)
>   {
>   	struct drm_crtc *crtc = state->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);
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	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;
> @@ -11929,75 +11961,18 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   	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");
> +	if (!obj)
>   		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;
> -		}
> -
> +	else if (!INTEL_INFO(dev)->cursor_needs_physical)
>   		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;
> -		}
> +	else
>   		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);
> -
>   	intel_crtc->cursor_addr = addr;
>   	intel_crtc->cursor_bo = obj;
>   update:
> @@ -12013,13 +11988,6 @@ update:
>
>   		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
> @@ -12060,7 +12028,13 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
>   	if (ret)
>   		return ret;
>
> -	return intel_commit_cursor_plane(plane, &state);
> +	ret = intel_prepare_cursor_plane(plane, &state);
> +	if (ret)
> +		return ret;
> +
> +	intel_commit_cursor_plane(plane, &state);
> +
> +	return 0;
>   }
>
>   static const struct drm_plane_funcs intel_cursor_plane_funcs = {
>

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

[PATCH 5/9] drm/i915: Make intel_plane_state subclass drm_plane_state

[Matt Roper]

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 34 +++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_drv.h     |  3 +--
 drivers/gpu/drm/i915/intel_sprite.c  | 16 ++++++++--------
 3 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ff94e43..217d1e3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11578,8 +11578,8 @@ static int
 intel_check_primary_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
-	struct drm_crtc *crtc = state->crtc;
-	struct drm_framebuffer *fb = state->fb;
+	struct drm_crtc *crtc = state->base.crtc;
+	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
@@ -11595,8 +11595,8 @@ static int
 intel_prepare_primary_plane(struct drm_plane *plane,
 			    struct intel_plane_state *state)
 {
-	struct drm_crtc *crtc = state->crtc;
-	struct drm_framebuffer *fb = state->fb;
+	struct drm_crtc *crtc = state->base.crtc;
+	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
@@ -11631,9 +11631,9 @@ static void
 intel_commit_primary_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_crtc *crtc = state->base.crtc;
+	struct drm_framebuffer *fb = state->base.fb;
+	struct drm_device *dev = plane->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;
@@ -11732,8 +11732,8 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int ret;
 
-	state.crtc = crtc;
-	state.fb = fb;
+	state.base.crtc = crtc;
+	state.base.fb = fb;
 
 	/* sample coordinates in 16.16 fixed point */
 	state.src.x1 = src_x;
@@ -11846,9 +11846,9 @@ static int
 intel_check_cursor_plane(struct drm_plane *plane,
 			 struct intel_plane_state *state)
 {
-	struct drm_crtc *crtc = state->crtc;
+	struct drm_crtc *crtc = state->base.crtc;
 	struct drm_device *dev = crtc->dev;
-	struct drm_framebuffer *fb = state->fb;
+	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
@@ -11903,8 +11903,8 @@ intel_prepare_cursor_plane(struct drm_plane *plane,
 			   struct intel_plane_state *state)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_framebuffer *fb = state->fb;
-	struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
+	struct drm_framebuffer *fb = state->base.fb;
+	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
 	enum pipe pipe = intel_crtc->pipe;
@@ -11936,11 +11936,11 @@ static void
 intel_commit_cursor_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
-	struct drm_crtc *crtc = state->crtc;
+	struct drm_crtc *crtc = state->base.crtc;
 	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
-	struct drm_i915_gem_object *obj = intel_fb_obj(state->fb);
+	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
 	enum pipe pipe = intel_crtc->pipe;
 	unsigned old_width;
 	uint32_t addr;
@@ -12001,8 +12001,8 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct intel_plane_state state;
 	int ret;
 
-	state.crtc = crtc;
-	state.fb = fb;
+	state.base.crtc = crtc;
+	state.base.fb = fb;
 
 	/* sample coordinates in 16.16 fixed point */
 	state.src.x1 = src_x;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 44b153c5..a6eafb4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -244,8 +244,7 @@ typedef struct dpll {
 } intel_clock_t;
 
 struct intel_plane_state {
-	struct drm_crtc *crtc;
-	struct drm_framebuffer *fb;
+	struct drm_plane_state base;
 	struct drm_rect src;
 	struct drm_rect dst;
 	struct drm_rect clip;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 7d9c340..fc96d13 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1096,9 +1096,9 @@ static int
 intel_check_sprite_plane(struct drm_plane *plane,
 			 struct intel_plane_state *state)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
+	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
-	struct drm_framebuffer *fb = state->fb;
+	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	int crtc_x, crtc_y;
 	unsigned int crtc_w, crtc_h;
@@ -1262,11 +1262,11 @@ intel_prepare_sprite_plane(struct drm_plane *plane,
 			   struct intel_plane_state *state)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_crtc *crtc = state->crtc;
+	struct drm_crtc *crtc = state->base.crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	enum pipe pipe = intel_crtc->pipe;
-	struct drm_framebuffer *fb = state->fb;
+	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct drm_i915_gem_object *old_obj = intel_plane->obj;
 	int ret;
@@ -1297,11 +1297,11 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_crtc *crtc = state->crtc;
+	struct drm_crtc *crtc = state->base.crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	enum pipe pipe = intel_crtc->pipe;
-	struct drm_framebuffer *fb = state->fb;
+	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct drm_i915_gem_object *old_obj = intel_plane->obj;
 	int crtc_x, crtc_y;
@@ -1391,8 +1391,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int ret;
 
-	state.crtc = crtc;
-	state.fb = fb;
+	state.base.crtc = crtc;
+	state.base.fb = fb;
 
 	/* sample coordinates in 16.16 fixed point */
 	state.src.x1 = src_x;
-- 
1.8.5.1

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

[PATCH 6/9] drm/i915: Consolidate plane 'prepare' functions

[Matt Roper]

The 'prepare' step for all types of planes are pretty similar;
consolidate the three 'prepare' functions into a single function.  This
paves the way for future integration with the atomic plane handlers.

Note that we pull the 'wait for pending flips' functionality out of the
primary plane's prepare step and place it directly in the 'setplane'
code.  When we move to the atomic plane handlers, this code will be in
the 'atomic begin' step.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 167 +++++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_sprite.c  |  44 ++-------
 3 files changed, 99 insertions(+), 114 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 217d1e3..50d4299 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11574,6 +11574,66 @@ disable_unpin:
 	return 0;
 }
 
+/**
+ * intel_prepare_plane_fb - Prepare fb for usage on plane
+ * @plane: drm plane to prepare for
+ * @fb: framebuffer to prepare for presentation
+ *
+ * Prepares a framebuffer for usage on a display plane.  Generally this
+ * involves pinning the underlying object and updating the frontbuffer tracking
+ * bits.  Some older platforms need special physical address handling for
+ * cursor planes.
+ *
+ * Returns 0 on success, negative error code on failure.
+ */
+int
+intel_prepare_plane_fb(struct drm_plane *plane,
+		       struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = plane->dev;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	enum pipe pipe = intel_plane->pipe;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
+	unsigned frontbuffer_bits = 0;
+	int ret;
+
+	if (WARN_ON(fb == plane->fb || !obj))
+		return 0;
+
+	switch (plane->type) {
+	case DRM_PLANE_TYPE_PRIMARY:
+		frontbuffer_bits = INTEL_FRONTBUFFER_PRIMARY(pipe);
+		break;
+	case DRM_PLANE_TYPE_CURSOR:
+		frontbuffer_bits = INTEL_FRONTBUFFER_CURSOR(pipe);
+		break;
+	case DRM_PLANE_TYPE_OVERLAY:
+		frontbuffer_bits = INTEL_FRONTBUFFER_SPRITE(pipe);
+		break;
+	}
+
+	mutex_lock(&dev->struct_mutex);
+
+	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
+	    INTEL_INFO(dev)->cursor_needs_physical) {
+		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");
+	} else {
+		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
+		if (ret == 0)
+			i915_gem_track_fb(old_obj, obj, frontbuffer_bits);
+		else
+			DRM_DEBUG_KMS("pin & fence failed\n");
+	}
+
+	mutex_unlock(&dev->struct_mutex);
+
+	return ret;
+}
+
 static int
 intel_check_primary_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
@@ -11591,42 +11651,6 @@ intel_check_primary_plane(struct drm_plane *plane,
 					     false, true, &state->visible);
 }
 
-static int
-intel_prepare_primary_plane(struct drm_plane *plane,
-			    struct intel_plane_state *state)
-{
-	struct drm_crtc *crtc = state->base.crtc;
-	struct drm_framebuffer *fb = state->base.fb;
-	struct drm_device *dev = crtc->dev;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum pipe pipe = intel_crtc->pipe;
-	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
-	int ret;
-
-	intel_crtc_wait_for_pending_flips(crtc);
-
-	if (intel_crtc_has_pending_flip(crtc)) {
-		DRM_ERROR("pipe is still busy with an old pageflip\n");
-		return -EBUSY;
-	}
-
-	if (old_obj != obj) {
-		mutex_lock(&dev->struct_mutex);
-		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
-		if (ret == 0)
-			i915_gem_track_fb(old_obj, obj,
-					  INTEL_FRONTBUFFER_PRIMARY(pipe));
-		mutex_unlock(&dev->struct_mutex);
-		if (ret != 0) {
-			DRM_DEBUG_KMS("pin & fence failed\n");
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
 static void
 intel_commit_primary_plane(struct drm_plane *plane,
 			   struct intel_plane_state *state)
@@ -11728,6 +11752,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 			     uint32_t src_x, uint32_t src_y,
 			     uint32_t src_w, uint32_t src_h)
 {
+	struct drm_framebuffer *old_fb = plane->fb;
 	struct intel_plane_state state;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int ret;
@@ -11759,9 +11784,18 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
-	ret = intel_prepare_primary_plane(plane, &state);
-	if (ret)
-		return ret;
+	intel_crtc_wait_for_pending_flips(crtc);
+
+	if (intel_crtc_has_pending_flip(crtc)) {
+		DRM_ERROR("pipe is still busy with an old pageflip\n");
+		return -EBUSY;
+	}
+
+	if (fb != old_fb && fb) {
+		ret = intel_prepare_plane_fb(plane, fb);
+		if (ret)
+			return ret;
+	}
 
 	intel_commit_primary_plane(plane, &state);
 
@@ -11898,40 +11932,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	return ret;
 }
 
-static int
-intel_prepare_cursor_plane(struct drm_plane *plane,
-			   struct intel_plane_state *state)
-{
-	struct drm_device *dev = plane->dev;
-	struct drm_framebuffer *fb = state->base.fb;
-	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
-	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
-	enum pipe pipe = intel_crtc->pipe;
-	int ret = 0;
-
-	if (old_obj != obj) {
-		/* we only need to pin inside GTT if cursor is non-phy */
-		mutex_lock(&dev->struct_mutex);
-		if (!INTEL_INFO(dev)->cursor_needs_physical) {
-			if (obj)
-				ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
-			if (ret == 0)
-				i915_gem_track_fb(intel_crtc->cursor_bo, obj,
-						  INTEL_FRONTBUFFER_CURSOR(pipe));
-		} else {
-			int align = IS_I830(dev) ? 16 * 1024 : 256;
-			if (obj)
-				ret = i915_gem_object_attach_phys(obj, align);
-			if (ret)
-				DRM_DEBUG_KMS("failed to attach phys object\n");
-		}
-		mutex_unlock(&dev->struct_mutex);
-	}
-
-	return ret;
-}
-
 static void
 intel_commit_cursor_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
@@ -11941,6 +11941,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
+	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
 	enum pipe pipe = intel_crtc->pipe;
 	unsigned old_width;
 	uint32_t addr;
@@ -11961,6 +11962,17 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	if (intel_crtc->cursor_bo == obj)
 		goto update;
 
+	/*
+	 * 'prepare' is only called when fb != NULL; we still need to update
+	 * frontbuffer tracking for the 'disable' case here.
+	 */
+	if (!obj) {
+		mutex_lock(&dev->struct_mutex);
+		i915_gem_track_fb(old_obj, NULL,
+				  INTEL_FRONTBUFFER_CURSOR(pipe));
+		mutex_unlock(&dev->struct_mutex);
+	}
+
 	if (!obj)
 		addr = 0;
 	else if (!INTEL_INFO(dev)->cursor_needs_physical)
@@ -11997,6 +12009,7 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 			  uint32_t src_x, uint32_t src_y,
 			  uint32_t src_w, uint32_t src_h)
 {
+	struct drm_framebuffer *old_fb = plane->fb;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane_state state;
 	int ret;
@@ -12028,9 +12041,11 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
-	ret = intel_prepare_cursor_plane(plane, &state);
-	if (ret)
-		return ret;
+	if (fb != old_fb && fb) {
+		ret = intel_prepare_plane_fb(plane, fb);
+		if (ret)
+			return ret;
+	}
 
 	intel_commit_cursor_plane(plane, &state);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a6eafb4..e9f4dc1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -924,6 +924,8 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane);
 void intel_finish_page_flip(struct drm_device *dev, int pipe);
 void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
 void intel_check_page_flip(struct drm_device *dev, int pipe);
+int intel_prepare_plane_fb(struct drm_plane *plane,
+			   struct drm_framebuffer *fb);
 
 /* shared dpll functions */
 struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index fc96d13..5d8c2e0 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1257,41 +1257,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	return 0;
 }
 
-static int
-intel_prepare_sprite_plane(struct drm_plane *plane,
-			   struct intel_plane_state *state)
-{
-	struct drm_device *dev = plane->dev;
-	struct drm_crtc *crtc = state->base.crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-	enum pipe pipe = intel_crtc->pipe;
-	struct drm_framebuffer *fb = state->base.fb;
-	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_plane->obj;
-	int ret;
-
-	if (old_obj != obj) {
-		mutex_lock(&dev->struct_mutex);
-
-		/* Note that this will apply the VT-d workaround for scanouts,
-		 * which is more restrictive than required for sprites. (The
-		 * primary plane requires 256KiB alignment with 64 PTE padding,
-		 * the sprite planes only require 128KiB alignment and 32 PTE
-		 * padding.
-		 */
-		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
-		if (ret == 0)
-			i915_gem_track_fb(old_obj, obj,
-					  INTEL_FRONTBUFFER_SPRITE(pipe));
-		mutex_unlock(&dev->struct_mutex);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-
 static void
 intel_commit_sprite_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
@@ -1387,6 +1352,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		   uint32_t src_x, uint32_t src_y,
 		   uint32_t src_w, uint32_t src_h)
 {
+	struct drm_framebuffer *old_fb = plane->fb;
 	struct intel_plane_state state;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int ret;
@@ -1417,9 +1383,11 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
-	ret = intel_prepare_sprite_plane(plane, &state);
-	if (ret)
-		return ret;
+	if (fb != old_fb && fb) {
+		ret = intel_prepare_plane_fb(plane, fb);
+		if (ret)
+			return ret;
+	}
 
 	intel_commit_sprite_plane(plane, &state);
 	return 0;
-- 
1.8.5.1

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

Re: [PATCH 6/9] drm/i915: Consolidate plane 'prepare' functions

[Ander Conselvan de Oliveira]

On 11/24/2014 09:53 PM, Matt Roper wrote:
> The 'prepare' step for all types of planes are pretty similar;
> consolidate the three 'prepare' functions into a single function.  This
> paves the way for future integration with the atomic plane handlers.
>
> Note that we pull the 'wait for pending flips' functionality out of the
> primary plane's prepare step and place it directly in the 'setplane'
> code.  When we move to the atomic plane handlers, this code will be in
> the 'atomic begin' step.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 167 +++++++++++++++++++----------------
>   drivers/gpu/drm/i915/intel_drv.h     |   2 +
>   drivers/gpu/drm/i915/intel_sprite.c  |  44 ++-------
>   3 files changed, 99 insertions(+), 114 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 217d1e3..50d4299 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11574,6 +11574,66 @@ disable_unpin:
>   	return 0;
>   }
>
> +/**
> + * intel_prepare_plane_fb - Prepare fb for usage on plane
> + * @plane: drm plane to prepare for
> + * @fb: framebuffer to prepare for presentation
> + *
> + * Prepares a framebuffer for usage on a display plane.  Generally this
> + * involves pinning the underlying object and updating the frontbuffer tracking
> + * bits.  Some older platforms need special physical address handling for
> + * cursor planes.
> + *
> + * Returns 0 on success, negative error code on failure.
> + */
> +int
> +intel_prepare_plane_fb(struct drm_plane *plane,
> +		       struct drm_framebuffer *fb)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	enum pipe pipe = intel_plane->pipe;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> +	unsigned frontbuffer_bits = 0;
> +	int ret;
> +
> +	if (WARN_ON(fb == plane->fb || !obj))
> +		return 0;
> +
> +	switch (plane->type) {
> +	case DRM_PLANE_TYPE_PRIMARY:
> +		frontbuffer_bits = INTEL_FRONTBUFFER_PRIMARY(pipe);
> +		break;
> +	case DRM_PLANE_TYPE_CURSOR:
> +		frontbuffer_bits = INTEL_FRONTBUFFER_CURSOR(pipe);
> +		break;
> +	case DRM_PLANE_TYPE_OVERLAY:
> +		frontbuffer_bits = INTEL_FRONTBUFFER_SPRITE(pipe);
> +		break;
> +	}
> +
> +	mutex_lock(&dev->struct_mutex);
> +
> +	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
> +	    INTEL_INFO(dev)->cursor_needs_physical) {
> +		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");
> +	} else {
> +		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> +		if (ret == 0)
> +			i915_gem_track_fb(old_obj, obj, frontbuffer_bits);
> +		else
> +			DRM_DEBUG_KMS("pin & fence failed\n");
> +	}

As commented on a previous patch, shouldn't i915_gem_track_fb() be 
called on the INTEL_INFO(dev)->cursor_needs_physical case too?

I can't see anything else wrong with the patch.

Ander

> +
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return ret;
> +}
> +
>   static int
>   intel_check_primary_plane(struct drm_plane *plane,
>   			  struct intel_plane_state *state)
> @@ -11591,42 +11651,6 @@ intel_check_primary_plane(struct drm_plane *plane,
>   					     false, true, &state->visible);
>   }
>
> -static int
> -intel_prepare_primary_plane(struct drm_plane *plane,
> -			    struct intel_plane_state *state)
> -{
> -	struct drm_crtc *crtc = state->base.crtc;
> -	struct drm_framebuffer *fb = state->base.fb;
> -	struct drm_device *dev = crtc->dev;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	enum pipe pipe = intel_crtc->pipe;
> -	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> -	int ret;
> -
> -	intel_crtc_wait_for_pending_flips(crtc);
> -
> -	if (intel_crtc_has_pending_flip(crtc)) {
> -		DRM_ERROR("pipe is still busy with an old pageflip\n");
> -		return -EBUSY;
> -	}
> -
> -	if (old_obj != obj) {
> -		mutex_lock(&dev->struct_mutex);
> -		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> -		if (ret == 0)
> -			i915_gem_track_fb(old_obj, obj,
> -					  INTEL_FRONTBUFFER_PRIMARY(pipe));
> -		mutex_unlock(&dev->struct_mutex);
> -		if (ret != 0) {
> -			DRM_DEBUG_KMS("pin & fence failed\n");
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>   static void
>   intel_commit_primary_plane(struct drm_plane *plane,
>   			   struct intel_plane_state *state)
> @@ -11728,6 +11752,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>   			     uint32_t src_x, uint32_t src_y,
>   			     uint32_t src_w, uint32_t src_h)
>   {
> +	struct drm_framebuffer *old_fb = plane->fb;
>   	struct intel_plane_state state;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	int ret;
> @@ -11759,9 +11784,18 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>   	if (ret)
>   		return ret;
>
> -	ret = intel_prepare_primary_plane(plane, &state);
> -	if (ret)
> -		return ret;
> +	intel_crtc_wait_for_pending_flips(crtc);
> +
> +	if (intel_crtc_has_pending_flip(crtc)) {
> +		DRM_ERROR("pipe is still busy with an old pageflip\n");
> +		return -EBUSY;
> +	}
> +
> +	if (fb != old_fb && fb) {
> +		ret = intel_prepare_plane_fb(plane, fb);
> +		if (ret)
> +			return ret;
> +	}
>
>   	intel_commit_primary_plane(plane, &state);
>
> @@ -11898,40 +11932,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
>   	return ret;
>   }
>
> -static int
> -intel_prepare_cursor_plane(struct drm_plane *plane,
> -			   struct intel_plane_state *state)
> -{
> -	struct drm_device *dev = plane->dev;
> -	struct drm_framebuffer *fb = state->base.fb;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
> -	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
> -	enum pipe pipe = intel_crtc->pipe;
> -	int ret = 0;
> -
> -	if (old_obj != obj) {
> -		/* we only need to pin inside GTT if cursor is non-phy */
> -		mutex_lock(&dev->struct_mutex);
> -		if (!INTEL_INFO(dev)->cursor_needs_physical) {
> -			if (obj)
> -				ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> -			if (ret == 0)
> -				i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> -						  INTEL_FRONTBUFFER_CURSOR(pipe));
> -		} else {
> -			int align = IS_I830(dev) ? 16 * 1024 : 256;
> -			if (obj)
> -				ret = i915_gem_object_attach_phys(obj, align);
> -			if (ret)
> -				DRM_DEBUG_KMS("failed to attach phys object\n");
> -		}
> -		mutex_unlock(&dev->struct_mutex);
> -	}
> -
> -	return ret;
> -}
> -
>   static void
>   intel_commit_cursor_plane(struct drm_plane *plane,
>   			  struct intel_plane_state *state)
> @@ -11941,6 +11941,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
> +	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
>   	enum pipe pipe = intel_crtc->pipe;
>   	unsigned old_width;
>   	uint32_t addr;
> @@ -11961,6 +11962,17 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   	if (intel_crtc->cursor_bo == obj)
>   		goto update;
>
> +	/*
> +	 * 'prepare' is only called when fb != NULL; we still need to update
> +	 * frontbuffer tracking for the 'disable' case here.
> +	 */
> +	if (!obj) {
> +		mutex_lock(&dev->struct_mutex);
> +		i915_gem_track_fb(old_obj, NULL,
> +				  INTEL_FRONTBUFFER_CURSOR(pipe));
> +		mutex_unlock(&dev->struct_mutex);
> +	}
> +
>   	if (!obj)
>   		addr = 0;
>   	else if (!INTEL_INFO(dev)->cursor_needs_physical)
> @@ -11997,6 +12009,7 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
>   			  uint32_t src_x, uint32_t src_y,
>   			  uint32_t src_w, uint32_t src_h)
>   {
> +	struct drm_framebuffer *old_fb = plane->fb;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	struct intel_plane_state state;
>   	int ret;
> @@ -12028,9 +12041,11 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
>   	if (ret)
>   		return ret;
>
> -	ret = intel_prepare_cursor_plane(plane, &state);
> -	if (ret)
> -		return ret;
> +	if (fb != old_fb && fb) {
> +		ret = intel_prepare_plane_fb(plane, fb);
> +		if (ret)
> +			return ret;
> +	}
>
>   	intel_commit_cursor_plane(plane, &state);
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a6eafb4..e9f4dc1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -924,6 +924,8 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane);
>   void intel_finish_page_flip(struct drm_device *dev, int pipe);
>   void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
>   void intel_check_page_flip(struct drm_device *dev, int pipe);
> +int intel_prepare_plane_fb(struct drm_plane *plane,
> +			   struct drm_framebuffer *fb);
>
>   /* shared dpll functions */
>   struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index fc96d13..5d8c2e0 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1257,41 +1257,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
>   	return 0;
>   }
>
> -static int
> -intel_prepare_sprite_plane(struct drm_plane *plane,
> -			   struct intel_plane_state *state)
> -{
> -	struct drm_device *dev = plane->dev;
> -	struct drm_crtc *crtc = state->base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	enum pipe pipe = intel_crtc->pipe;
> -	struct drm_framebuffer *fb = state->base.fb;
> -	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_plane->obj;
> -	int ret;
> -
> -	if (old_obj != obj) {
> -		mutex_lock(&dev->struct_mutex);
> -
> -		/* Note that this will apply the VT-d workaround for scanouts,
> -		 * which is more restrictive than required for sprites. (The
> -		 * primary plane requires 256KiB alignment with 64 PTE padding,
> -		 * the sprite planes only require 128KiB alignment and 32 PTE
> -		 * padding.
> -		 */
> -		ret = intel_pin_and_fence_fb_obj(plane, fb, NULL);
> -		if (ret == 0)
> -			i915_gem_track_fb(old_obj, obj,
> -					  INTEL_FRONTBUFFER_SPRITE(pipe));
> -		mutex_unlock(&dev->struct_mutex);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -
>   static void
>   intel_commit_sprite_plane(struct drm_plane *plane,
>   			  struct intel_plane_state *state)
> @@ -1387,6 +1352,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   		   uint32_t src_x, uint32_t src_y,
>   		   uint32_t src_w, uint32_t src_h)
>   {
> +	struct drm_framebuffer *old_fb = plane->fb;
>   	struct intel_plane_state state;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	int ret;
> @@ -1417,9 +1383,11 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   	if (ret)
>   		return ret;
>
> -	ret = intel_prepare_sprite_plane(plane, &state);
> -	if (ret)
> -		return ret;
> +	if (fb != old_fb && fb) {
> +		ret = intel_prepare_plane_fb(plane, fb);
> +		if (ret)
> +			return ret;
> +	}
>
>   	intel_commit_sprite_plane(plane, &state);
>   	return 0;
>

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

Re: [PATCH 6/9] drm/i915: Consolidate plane 'prepare' functions

[Ander Conselvan de Oliveira]

On 11/24/2014 09:53 PM, Matt Roper wrote:
> The 'prepare' step for all types of planes are pretty similar;
> consolidate the three 'prepare' functions into a single function.  This
> paves the way for future integration with the atomic plane handlers.
>
> Note that we pull the 'wait for pending flips' functionality out of the
> primary plane's prepare step and place it directly in the 'setplane'
> code.  When we move to the atomic plane handlers, this code will be in
> the 'atomic begin' step.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 167 +++++++++++++++++++----------------
>   drivers/gpu/drm/i915/intel_drv.h     |   2 +
>   drivers/gpu/drm/i915/intel_sprite.c  |  44 ++-------
>   3 files changed, 99 insertions(+), 114 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 217d1e3..50d4299 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11574,6 +11574,66 @@ disable_unpin:
>   	return 0;
>   }
>
> +/**
> + * intel_prepare_plane_fb - Prepare fb for usage on plane
> + * @plane: drm plane to prepare for
> + * @fb: framebuffer to prepare for presentation
> + *
> + * Prepares a framebuffer for usage on a display plane.  Generally this
> + * involves pinning the underlying object and updating the frontbuffer tracking
> + * bits.  Some older platforms need special physical address handling for
> + * cursor planes.
> + *
> + * Returns 0 on success, negative error code on failure.
> + */
> +int
> +intel_prepare_plane_fb(struct drm_plane *plane,
> +		       struct drm_framebuffer *fb)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	enum pipe pipe = intel_plane->pipe;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);

[...]

> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index fc96d13..5d8c2e0 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1257,41 +1257,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
>   	return 0;
>   }
>
> -static int
> -intel_prepare_sprite_plane(struct drm_plane *plane,
> -			   struct intel_plane_state *state)
> -{
> -	struct drm_device *dev = plane->dev;
> -	struct drm_crtc *crtc = state->base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	enum pipe pipe = intel_crtc->pipe;
> -	struct drm_framebuffer *fb = state->base.fb;
> -	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_plane->obj;

This used to look at intel_plane->obj, but the new unified prepare 
function uses the value of intel_plane->base.fb, which is not updated in 
intel_commit_sprite_plane().

Ander

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

Re: [PATCH 6/9] drm/i915: Consolidate plane 'prepare' functions

[Matt Roper]

On Mon, Dec 01, 2014 at 10:29:04AM +0200, Ander Conselvan de Oliveira wrote:
> On 11/24/2014 09:53 PM, Matt Roper wrote:
...
> >-static int
> >-intel_prepare_sprite_plane(struct drm_plane *plane,
> >-			   struct intel_plane_state *state)
> >-{
> >-	struct drm_device *dev = plane->dev;
> >-	struct drm_crtc *crtc = state->base.crtc;
> >-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >-	struct intel_plane *intel_plane = to_intel_plane(plane);
> >-	enum pipe pipe = intel_crtc->pipe;
> >-	struct drm_framebuffer *fb = state->base.fb;
> >-	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> >-	struct drm_i915_gem_object *old_obj = intel_plane->obj;
> 
> This used to look at intel_plane->obj, but the new unified prepare
> function uses the value of intel_plane->base.fb, which is not
> updated in intel_commit_sprite_plane().
> 
> Ander

I think this should be okay (same for the cursor case you noted in a
previous patch) because the DRM core updates drm_plane->fb for us in
__setplane_internal() after the driver's update handler succeeds.
Updating drm_plane->fb inside the driver's commit function should only
be necessary if we then use that new value before returning back to the
DRM core.  In the case you note above, we're looking at the value which
was updated by the DRM core following the previous sprite update.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/9] drm/i915: Consolidate plane 'cleanup' operations

[Matt Roper]

All plane update functions need to unpin the old framebuffer when
flipping to a new one.  Pull this logic into a separate function to ease
the integration with atomic plane helpers.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 drivers/gpu/drm/i915/intel_sprite.c  | 27 ++++++-----------
 3 files changed, 52 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 50d4299..20890a9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11634,6 +11634,31 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	return ret;
 }
 
+/**
+ * intel_cleanup_plane_fb - Cleans up an fb after plane use
+ * @plane: drm plane to clean up for
+ * @fb: old framebuffer that was on plane
+ *
+ * Cleans up a framebuffer that has just been removed from a plane.
+ */
+void
+intel_cleanup_plane_fb(struct drm_plane *plane,
+		       struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+
+	if (WARN_ON(!obj))
+		return;
+
+	if (plane->type != DRM_PLANE_TYPE_CURSOR ||
+	    !INTEL_INFO(dev)->cursor_needs_physical) {
+		mutex_lock(&dev->struct_mutex);
+		intel_unpin_fb_obj(obj);
+		mutex_unlock(&dev->struct_mutex);
+	}
+}
+
 static int
 intel_check_primary_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
@@ -11661,9 +11686,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	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 = plane->fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_rect *src = &state->src;
 
@@ -11734,15 +11757,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
 		intel_update_fbc(dev);
 		mutex_unlock(&dev->struct_mutex);
 	}
-
-	if (old_fb && old_fb != fb) {
-		if (intel_crtc->active)
-			intel_wait_for_vblank(dev, intel_crtc->pipe);
-
-		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(old_obj);
-		mutex_unlock(&dev->struct_mutex);
-	}
 }
 
 static int
@@ -11752,6 +11766,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 			     uint32_t src_x, uint32_t src_y,
 			     uint32_t src_w, uint32_t src_h)
 {
+	struct drm_device *dev = plane->dev;
 	struct drm_framebuffer *old_fb = plane->fb;
 	struct intel_plane_state state;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -11799,6 +11814,13 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	intel_commit_primary_plane(plane, &state);
 
+	if (fb != old_fb) {
+		if (intel_crtc->active)
+			intel_wait_for_vblank(dev, intel_crtc->pipe);
+		if (old_fb)
+			intel_cleanup_plane_fb(plane, old_fb);
+	}
+
 	return 0;
 }
 
@@ -11980,11 +12002,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	else
 		addr = obj->phys_handle->busaddr;
 
-	if (intel_crtc->cursor_bo) {
-		if (!INTEL_INFO(dev)->cursor_needs_physical)
-			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
-	}
-
 	intel_crtc->cursor_addr = addr;
 	intel_crtc->cursor_bo = obj;
 update:
@@ -12009,6 +12026,7 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 			  uint32_t src_x, uint32_t src_y,
 			  uint32_t src_w, uint32_t src_h)
 {
+	struct drm_device *dev = plane->dev;
 	struct drm_framebuffer *old_fb = plane->fb;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane_state state;
@@ -12049,6 +12067,13 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	intel_commit_cursor_plane(plane, &state);
 
+	if (fb != old_fb) {
+		if (intel_crtc->active)
+			intel_wait_for_vblank(dev, intel_crtc->pipe);
+		if (old_fb)
+			intel_cleanup_plane_fb(plane, old_fb);
+	}
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e9f4dc1..299fc4f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -926,6 +926,8 @@ void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
 void intel_check_page_flip(struct drm_device *dev, int pipe);
 int intel_prepare_plane_fb(struct drm_plane *plane,
 			   struct drm_framebuffer *fb);
+void intel_cleanup_plane_fb(struct drm_plane *plane,
+			    struct drm_framebuffer *fb);
 
 /* shared dpll functions */
 struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 5d8c2e0..152a32d 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1268,7 +1268,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_plane->obj;
 	int crtc_x, crtc_y;
 	unsigned int crtc_w, crtc_h;
 	uint32_t src_x, src_y, src_w, src_h;
@@ -1326,23 +1325,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 		if (!primary_was_enabled && primary_enabled)
 			intel_post_enable_primary(crtc);
 	}
-
-	/* Unpin old obj after new one is active to avoid ugliness */
-	if (old_obj && old_obj != obj) {
-
-		/*
-		 * It's fairly common to simply update the position of
-		 * an existing object.  In that case, we don't need to
-		 * wait for vblank to avoid ugliness, we only need to
-		 * do the pin & ref bookkeeping.
-		 */
-		if (intel_crtc->active)
-			intel_wait_for_vblank(dev, intel_crtc->pipe);
-
-		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(old_obj);
-		mutex_unlock(&dev->struct_mutex);
-	}
 }
 
 static int
@@ -1352,6 +1334,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		   uint32_t src_x, uint32_t src_y,
 		   uint32_t src_w, uint32_t src_h)
 {
+	struct drm_device *dev = plane->dev;
 	struct drm_framebuffer *old_fb = plane->fb;
 	struct intel_plane_state state;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -1390,6 +1373,14 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	}
 
 	intel_commit_sprite_plane(plane, &state);
+
+	if (fb != old_fb) {
+		if (intel_crtc->active)
+			intel_wait_for_vblank(dev, intel_crtc->pipe);
+		if (old_fb)
+			intel_cleanup_plane_fb(plane, old_fb);
+	}
+
 	return 0;
 }
 
-- 
1.8.5.1

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

Re: [PATCH 7/9] drm/i915: Consolidate plane 'cleanup' operations

[Ander Conselvan de Oliveira]

On 11/24/2014 09:53 PM, Matt Roper wrote:
> All plane update functions need to unpin the old framebuffer when
> flipping to a new one.  Pull this logic into a separate function to ease
> the integration with atomic plane helpers.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++++++++++++++++----------
>   drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>   drivers/gpu/drm/i915/intel_sprite.c  | 27 ++++++-----------
>   3 files changed, 52 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 50d4299..20890a9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11634,6 +11634,31 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>   	return ret;
>   }
>
> +/**
> + * intel_cleanup_plane_fb - Cleans up an fb after plane use
> + * @plane: drm plane to clean up for
> + * @fb: old framebuffer that was on plane
> + *
> + * Cleans up a framebuffer that has just been removed from a plane.
> + */
> +void
> +intel_cleanup_plane_fb(struct drm_plane *plane,
> +		       struct drm_framebuffer *fb)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +
> +	if (WARN_ON(!obj))
> +		return;
> +
> +	if (plane->type != DRM_PLANE_TYPE_CURSOR ||
> +	    !INTEL_INFO(dev)->cursor_needs_physical) {
> +		mutex_lock(&dev->struct_mutex);
> +		intel_unpin_fb_obj(obj);
> +		mutex_unlock(&dev->struct_mutex);
> +	}
> +}
> +
>   static int
>   intel_check_primary_plane(struct drm_plane *plane,
>   			  struct intel_plane_state *state)
> @@ -11661,9 +11686,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
>   	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 = plane->fb;
>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_rect *src = &state->src;
>
> @@ -11734,15 +11757,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
>   		intel_update_fbc(dev);
>   		mutex_unlock(&dev->struct_mutex);
>   	}
> -
> -	if (old_fb && old_fb != fb) {
> -		if (intel_crtc->active)
> -			intel_wait_for_vblank(dev, intel_crtc->pipe);
> -
> -		mutex_lock(&dev->struct_mutex);
> -		intel_unpin_fb_obj(old_obj);
> -		mutex_unlock(&dev->struct_mutex);
> -	}
>   }
>
>   static int
> @@ -11752,6 +11766,7 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>   			     uint32_t src_x, uint32_t src_y,
>   			     uint32_t src_w, uint32_t src_h)
>   {
> +	struct drm_device *dev = plane->dev;
>   	struct drm_framebuffer *old_fb = plane->fb;
>   	struct intel_plane_state state;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -11799,6 +11814,13 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>
>   	intel_commit_primary_plane(plane, &state);
>
> +	if (fb != old_fb) {
> +		if (intel_crtc->active)
> +			intel_wait_for_vblank(dev, intel_crtc->pipe);

So now we wait for vblank even if there's no fb to unpin?

Ander


> +		if (old_fb)
> +			intel_cleanup_plane_fb(plane, old_fb);
> +	}
> +
>   	return 0;
>   }
>
> @@ -11980,11 +12002,6 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>   	else
>   		addr = obj->phys_handle->busaddr;
>
> -	if (intel_crtc->cursor_bo) {
> -		if (!INTEL_INFO(dev)->cursor_needs_physical)
> -			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> -	}
> -
>   	intel_crtc->cursor_addr = addr;
>   	intel_crtc->cursor_bo = obj;
>   update:
> @@ -12009,6 +12026,7 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
>   			  uint32_t src_x, uint32_t src_y,
>   			  uint32_t src_w, uint32_t src_h)
>   {
> +	struct drm_device *dev = plane->dev;
>   	struct drm_framebuffer *old_fb = plane->fb;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   	struct intel_plane_state state;
> @@ -12049,6 +12067,13 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
>
>   	intel_commit_cursor_plane(plane, &state);
>
> +	if (fb != old_fb) {
> +		if (intel_crtc->active)
> +			intel_wait_for_vblank(dev, intel_crtc->pipe);
> +		if (old_fb)
> +			intel_cleanup_plane_fb(plane, old_fb);
> +	}
> +
>   	return 0;
>   }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e9f4dc1..299fc4f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -926,6 +926,8 @@ void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
>   void intel_check_page_flip(struct drm_device *dev, int pipe);
>   int intel_prepare_plane_fb(struct drm_plane *plane,
>   			   struct drm_framebuffer *fb);
> +void intel_cleanup_plane_fb(struct drm_plane *plane,
> +			    struct drm_framebuffer *fb);
>
>   /* shared dpll functions */
>   struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 5d8c2e0..152a32d 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1268,7 +1268,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   	enum pipe pipe = intel_crtc->pipe;
>   	struct drm_framebuffer *fb = state->base.fb;
>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_plane->obj;
>   	int crtc_x, crtc_y;
>   	unsigned int crtc_w, crtc_h;
>   	uint32_t src_x, src_y, src_w, src_h;
> @@ -1326,23 +1325,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   		if (!primary_was_enabled && primary_enabled)
>   			intel_post_enable_primary(crtc);
>   	}
> -
> -	/* Unpin old obj after new one is active to avoid ugliness */
> -	if (old_obj && old_obj != obj) {
> -
> -		/*
> -		 * It's fairly common to simply update the position of
> -		 * an existing object.  In that case, we don't need to
> -		 * wait for vblank to avoid ugliness, we only need to
> -		 * do the pin & ref bookkeeping.
> -		 */
> -		if (intel_crtc->active)
> -			intel_wait_for_vblank(dev, intel_crtc->pipe);
> -
> -		mutex_lock(&dev->struct_mutex);
> -		intel_unpin_fb_obj(old_obj);
> -		mutex_unlock(&dev->struct_mutex);
> -	}
>   }
>
>   static int
> @@ -1352,6 +1334,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   		   uint32_t src_x, uint32_t src_y,
>   		   uint32_t src_w, uint32_t src_h)
>   {
> +	struct drm_device *dev = plane->dev;
>   	struct drm_framebuffer *old_fb = plane->fb;
>   	struct intel_plane_state state;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -1390,6 +1373,14 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   	}
>
>   	intel_commit_sprite_plane(plane, &state);
> +
> +	if (fb != old_fb) {
> +		if (intel_crtc->active)
> +			intel_wait_for_vblank(dev, intel_crtc->pipe);
> +		if (old_fb)
> +			intel_cleanup_plane_fb(plane, old_fb);
> +	}
> +
>   	return 0;
>   }
>
>

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

[PATCH 8/9] drm/i915: Consolidate top-level .update_plane() handlers

[Matt Roper]

Our .update_plane() handlers do the same check/prepare/commit/cleanup
steps regardless of plane type.  Consolidate them all into a single
function that calls check/commit through a vtable.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 113 ++++++++++-------------------------
 drivers/gpu/drm/i915/intel_drv.h     |   9 +++
 drivers/gpu/drm/i915/intel_sprite.c  |  59 +-----------------
 3 files changed, 44 insertions(+), 137 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 20890a9..dd10dc6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11668,12 +11668,23 @@ intel_check_primary_plane(struct drm_plane *plane,
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
+	int ret;
+
+	ret = drm_plane_helper_check_update(plane, crtc, fb,
+					    src, dest, clip,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    false, true, &state->visible);
+	if (ret)
+		return ret;
+
+	intel_crtc_wait_for_pending_flips(crtc);
+	if (intel_crtc_has_pending_flip(crtc)) {
+		DRM_ERROR("pipe is still busy with an old pageflip\n");
+		return -EBUSY;
+	}
 
-	return drm_plane_helper_check_update(plane, crtc, fb,
-					     src, dest, clip,
-					     DRM_PLANE_HELPER_NO_SCALING,
-					     DRM_PLANE_HELPER_NO_SCALING,
-					     false, true, &state->visible);
+	return 0;
 }
 
 static void
@@ -11759,16 +11770,17 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	}
 }
 
-static int
-intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
-			     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
-			     unsigned int crtc_w, unsigned int crtc_h,
-			     uint32_t src_x, uint32_t src_y,
-			     uint32_t src_w, uint32_t src_h)
+int
+intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
+		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+		   unsigned int crtc_w, unsigned int crtc_h,
+		   uint32_t src_x, uint32_t src_y,
+		   uint32_t src_w, uint32_t src_h)
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_framebuffer *old_fb = plane->fb;
 	struct intel_plane_state state;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int ret;
 
@@ -11795,24 +11807,17 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	state.orig_src = state.src;
 	state.orig_dst = state.dst;
 
-	ret = intel_check_primary_plane(plane, &state);
+	ret = intel_plane->check_plane(plane, &state);
 	if (ret)
 		return ret;
 
-	intel_crtc_wait_for_pending_flips(crtc);
-
-	if (intel_crtc_has_pending_flip(crtc)) {
-		DRM_ERROR("pipe is still busy with an old pageflip\n");
-		return -EBUSY;
-	}
-
 	if (fb != old_fb && fb) {
 		ret = intel_prepare_plane_fb(plane, fb);
 		if (ret)
 			return ret;
 	}
 
-	intel_commit_primary_plane(plane, &state);
+	intel_plane->commit_plane(plane, &state);
 
 	if (fb != old_fb) {
 		if (intel_crtc->active)
@@ -11821,6 +11826,8 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 			intel_cleanup_plane_fb(plane, old_fb);
 	}
 
+	plane->fb = fb;
+
 	return 0;
 }
 
@@ -11833,7 +11840,7 @@ static void intel_plane_destroy(struct drm_plane *plane)
 }
 
 static const struct drm_plane_funcs intel_primary_plane_funcs = {
-	.update_plane = intel_primary_plane_setplane,
+	.update_plane = intel_update_plane,
 	.disable_plane = intel_primary_plane_disable,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property
@@ -11855,6 +11862,8 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 	primary->pipe = pipe;
 	primary->plane = pipe;
 	primary->rotation = BIT(DRM_ROTATE_0);
+	primary->check_plane = intel_check_primary_plane;
+	primary->commit_plane = intel_commit_primary_plane;
 	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
 		primary->plane = !pipe;
 
@@ -12019,66 +12028,8 @@ update:
 	}
 }
 
-static int
-intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
-			  struct drm_framebuffer *fb, int crtc_x, int crtc_y,
-			  unsigned int crtc_w, unsigned int crtc_h,
-			  uint32_t src_x, uint32_t src_y,
-			  uint32_t src_w, uint32_t src_h)
-{
-	struct drm_device *dev = plane->dev;
-	struct drm_framebuffer *old_fb = plane->fb;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_plane_state state;
-	int ret;
-
-	state.base.crtc = crtc;
-	state.base.fb = fb;
-
-	/* sample coordinates in 16.16 fixed point */
-	state.src.x1 = src_x;
-	state.src.x2 = src_x + src_w;
-	state.src.y1 = src_y;
-	state.src.y2 = src_y + src_h;
-
-	/* integer pixels */
-	state.dst.x1 = crtc_x;
-	state.dst.x2 = crtc_x + crtc_w;
-	state.dst.y1 = crtc_y;
-	state.dst.y2 = crtc_y + crtc_h;
-
-	state.clip.x1 = 0;
-	state.clip.y1 = 0;
-	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
-	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
-
-	state.orig_src = state.src;
-	state.orig_dst = state.dst;
-
-	ret = intel_check_cursor_plane(plane, &state);
-	if (ret)
-		return ret;
-
-	if (fb != old_fb && fb) {
-		ret = intel_prepare_plane_fb(plane, fb);
-		if (ret)
-			return ret;
-	}
-
-	intel_commit_cursor_plane(plane, &state);
-
-	if (fb != old_fb) {
-		if (intel_crtc->active)
-			intel_wait_for_vblank(dev, intel_crtc->pipe);
-		if (old_fb)
-			intel_cleanup_plane_fb(plane, old_fb);
-	}
-
-	return 0;
-}
-
 static const struct drm_plane_funcs intel_cursor_plane_funcs = {
-	.update_plane = intel_cursor_plane_update,
+	.update_plane = intel_update_plane,
 	.disable_plane = intel_cursor_plane_disable,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
@@ -12098,6 +12049,8 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 	cursor->pipe = pipe;
 	cursor->plane = pipe;
 	cursor->rotation = BIT(DRM_ROTATE_0);
+	cursor->check_plane = intel_check_cursor_plane;
+	cursor->commit_plane = intel_commit_cursor_plane;
 
 	drm_universal_plane_init(dev, &cursor->base, 0,
 				 &intel_cursor_plane_funcs,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 299fc4f..5f7a071 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -509,6 +509,10 @@ struct intel_plane {
 			     uint32_t src_w, uint32_t src_h);
 	void (*disable_plane)(struct drm_plane *plane,
 			      struct drm_crtc *crtc);
+	int (*check_plane)(struct drm_plane *plane,
+			   struct intel_plane_state *state);
+	void (*commit_plane)(struct drm_plane *plane,
+			     struct intel_plane_state *state);
 	int (*update_colorkey)(struct drm_plane *plane,
 			       struct drm_intel_sprite_colorkey *key);
 	void (*get_colorkey)(struct drm_plane *plane,
@@ -1012,6 +1016,11 @@ void intel_dp_hot_plug(struct intel_encoder *intel_encoder);
 void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv);
 uint32_t intel_dp_pack_aux(const uint8_t *src, int src_bytes);
 void intel_dp_unpack_aux(uint32_t src, uint8_t *dst, int dst_bytes);
+int intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
+		       struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+		       unsigned int crtc_w, unsigned int crtc_h,
+		       uint32_t src_x, uint32_t src_y,
+		       uint32_t src_w, uint32_t src_h);
 
 /* intel_dp_mst.c */
 int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 152a32d..bfd5270 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1328,63 +1328,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 }
 
 static int
-intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
-		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
-		   unsigned int crtc_w, unsigned int crtc_h,
-		   uint32_t src_x, uint32_t src_y,
-		   uint32_t src_w, uint32_t src_h)
-{
-	struct drm_device *dev = plane->dev;
-	struct drm_framebuffer *old_fb = plane->fb;
-	struct intel_plane_state state;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int ret;
-
-	state.base.crtc = crtc;
-	state.base.fb = fb;
-
-	/* sample coordinates in 16.16 fixed point */
-	state.src.x1 = src_x;
-	state.src.x2 = src_x + src_w;
-	state.src.y1 = src_y;
-	state.src.y2 = src_y + src_h;
-
-	/* integer pixels */
-	state.dst.x1 = crtc_x;
-	state.dst.x2 = crtc_x + crtc_w;
-	state.dst.y1 = crtc_y;
-	state.dst.y2 = crtc_y + crtc_h;
-
-	state.clip.x1 = 0;
-	state.clip.y1 = 0;
-	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
-	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
-	state.orig_src = state.src;
-	state.orig_dst = state.dst;
-
-	ret = intel_check_sprite_plane(plane, &state);
-	if (ret)
-		return ret;
-
-	if (fb != old_fb && fb) {
-		ret = intel_prepare_plane_fb(plane, fb);
-		if (ret)
-			return ret;
-	}
-
-	intel_commit_sprite_plane(plane, &state);
-
-	if (fb != old_fb) {
-		if (intel_crtc->active)
-			intel_wait_for_vblank(dev, intel_crtc->pipe);
-		if (old_fb)
-			intel_cleanup_plane_fb(plane, old_fb);
-	}
-
-	return 0;
-}
-
-static int
 intel_disable_plane(struct drm_plane *plane)
 {
 	struct drm_device *dev = plane->dev;
@@ -1679,6 +1622,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 	intel_plane->pipe = pipe;
 	intel_plane->plane = plane;
 	intel_plane->rotation = BIT(DRM_ROTATE_0);
+	intel_plane->check_plane = intel_check_sprite_plane;
+	intel_plane->commit_plane = intel_commit_sprite_plane;
 	possible_crtcs = (1 << pipe);
 	ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
 				       &intel_plane_funcs,
-- 
1.8.5.1

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

[PATCH 9/9] drm/i915: Make all plane disables use 'update_plane'

[Matt Roper]

If we extend the commit_plane handlers for each plane type to be able to
handle fb=0, then we can easily implement plane disable via the
update_plane handler.  The cursor plane already works this way, and this
is the direction we need to go to integrate with the atomic plane
handler.  We can now kill off the type-specific disable functions, as
well as the redundant intel_plane_disable() (not to be confused with
intel_disable_plane()).

Note that prepare_plane_fb() only gets called as part of update_plane
when fb!=NULL (by design, to match the semantics of the atomic plane
helpers); this means that our commit_plane handlers need to handle the
frontbuffer tracking for the disable case, even though they don't handle
it for normal updates.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 109 +++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_drv.h     |   2 +-
 drivers/gpu/drm/i915/intel_sprite.c  |  71 ++++++-----------------
 3 files changed, 67 insertions(+), 115 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index dd10dc6..fd1eaf5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3996,7 +3996,7 @@ static void intel_disable_planes(struct drm_crtc *crtc)
 	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
 		intel_plane = to_intel_plane(plane);
 		if (intel_plane->pipe == pipe)
-			intel_plane_disable(&intel_plane->base);
+			plane->funcs->disable_plane(plane);
 	}
 }
 
@@ -11535,45 +11535,6 @@ static void intel_shared_dpll_init(struct drm_device *dev)
 	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
 }
 
-static int
-intel_primary_plane_disable(struct drm_plane *plane)
-{
-	struct drm_device *dev = plane->dev;
-	struct intel_crtc *intel_crtc;
-
-	if (!plane->fb)
-		return 0;
-
-	BUG_ON(!plane->crtc);
-
-	intel_crtc = to_intel_crtc(plane->crtc);
-
-	/*
-	 * Even though we checked plane->fb above, it's still possible that
-	 * the primary plane has been implicitly disabled because the crtc
-	 * coordinates given weren't visible, or because we detected
-	 * that it was 100% covered by a sprite plane.  Or, the CRTC may be
-	 * off and we've set a fb, but haven't actually turned on the CRTC yet.
-	 * In either case, we need to unpin the FB and let the fb pointer get
-	 * updated, but otherwise we don't need to touch the hardware.
-	 */
-	if (!intel_crtc->primary_enabled)
-		goto disable_unpin;
-
-	intel_crtc_wait_for_pending_flips(plane->crtc);
-	intel_disable_primary_hw_plane(plane, plane->crtc);
-
-disable_unpin:
-	mutex_lock(&dev->struct_mutex);
-	i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
-			  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
-	intel_unpin_fb_obj(intel_fb_obj(plane->fb));
-	mutex_unlock(&dev->struct_mutex);
-	plane->fb = NULL;
-
-	return 0;
-}
-
 /**
  * intel_prepare_plane_fb - Prepare fb for usage on plane
  * @plane: drm plane to prepare for
@@ -11678,10 +11639,12 @@ intel_check_primary_plane(struct drm_plane *plane,
 	if (ret)
 		return ret;
 
-	intel_crtc_wait_for_pending_flips(crtc);
-	if (intel_crtc_has_pending_flip(crtc)) {
-		DRM_ERROR("pipe is still busy with an old pageflip\n");
-		return -EBUSY;
+	if (plane->crtc) {
+		intel_crtc_wait_for_pending_flips(plane->crtc);
+		if (intel_crtc_has_pending_flip(plane->crtc)) {
+			DRM_ERROR("pipe is still busy with an old pageflip\n");
+			return -EBUSY;
+		}
 	}
 
 	return 0;
@@ -11696,12 +11659,30 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	struct drm_device *dev = plane->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_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_rect *src = &state->src;
+	enum pipe pipe = intel_plane->pipe;
 
-	crtc->primary->fb = fb;
+	if (!fb) {
+		/*
+		 * 'prepare' is never called when plane is being disabled, so
+		 * we need to handle frontbuffer tracking here
+		 */
+		mutex_lock(&dev->struct_mutex);
+		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
+				  INTEL_FRONTBUFFER_PRIMARY(pipe));
+		mutex_unlock(&dev->struct_mutex);
+
+		/*
+		 * crtc may be passed as NULL when disabling, so set it to
+		 * the proper value now
+		 */
+		crtc = plane->crtc;
+		intel_crtc = to_intel_crtc(crtc);
+	}
+
+	plane->fb = fb;
 	crtc->x = src->x1 >> 16;
 	crtc->y = src->y1 >> 16;
 
@@ -11759,7 +11740,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
 			 * explicitly disables the plane by passing fb=0
 			 * because plane->fb still gets set and pinned.
 			 */
-			intel_disable_primary_hw_plane(plane, crtc);
+			intel_disable_primary_hw_plane(plane, plane->crtc);
 		}
 
 		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
@@ -11831,6 +11812,24 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	return 0;
 }
 
+/**
+ * intel_disable_plane - disable a plane
+ * @plane: plane to disable
+ *
+ * General disable handler for all plane types.
+ */
+int
+intel_disable_plane(struct drm_plane *plane)
+{
+	if (!plane->fb)
+		return 0;
+
+	BUG_ON(!plane->crtc);
+
+	return plane->funcs->update_plane(plane, plane->crtc, NULL,
+					  0, 0, 0, 0, 0, 0, 0, 0);
+}
+
 /* Common destruction function for both primary and cursor planes */
 static void intel_plane_destroy(struct drm_plane *plane)
 {
@@ -11841,7 +11840,7 @@ static void intel_plane_destroy(struct drm_plane *plane)
 
 static const struct drm_plane_funcs intel_primary_plane_funcs = {
 	.update_plane = intel_update_plane,
-	.disable_plane = intel_primary_plane_disable,
+	.disable_plane = intel_disable_plane,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property
 };
@@ -11896,18 +11895,6 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 }
 
 static int
-intel_cursor_plane_disable(struct drm_plane *plane)
-{
-	if (!plane->fb)
-		return 0;
-
-	BUG_ON(!plane->crtc);
-
-	return plane->funcs->update_plane(plane, plane->crtc, NULL,
-					  0, 0, 0, 0, 0, 0, 0, 0);
-}
-
-static int
 intel_check_cursor_plane(struct drm_plane *plane,
 			 struct intel_plane_state *state)
 {
@@ -12030,7 +12017,7 @@ update:
 
 static const struct drm_plane_funcs intel_cursor_plane_funcs = {
 	.update_plane = intel_update_plane,
-	.disable_plane = intel_cursor_plane_disable,
+	.disable_plane = intel_disable_plane,
 	.destroy = intel_plane_destroy,
 	.set_property = intel_plane_set_property,
 };
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5f7a071..da4da2c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1021,6 +1021,7 @@ int intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		       unsigned int crtc_w, unsigned int crtc_h,
 		       uint32_t src_x, uint32_t src_y,
 		       uint32_t src_w, uint32_t src_h);
+int intel_disable_plane(struct drm_plane *plane);
 
 /* intel_dp_mst.c */
 int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
@@ -1201,7 +1202,6 @@ int intel_plane_set_property(struct drm_plane *plane,
 			     struct drm_property *prop,
 			     uint64_t val);
 int intel_plane_restore(struct drm_plane *plane);
-void intel_plane_disable(struct drm_plane *plane);
 int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
 int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index bfd5270..bc5834b 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1109,7 +1109,12 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	const struct drm_rect *clip = &state->clip;
 	int hscale, vscale;
 	int max_scale, min_scale;
-	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	int pixel_size;
+
+	if (!fb) {
+		state->visible = false;
+		return 0;
+	}
 
 	/* Don't modify another pipe's plane */
 	if (intel_plane->pipe != intel_crtc->pipe) {
@@ -1232,6 +1237,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 		if (src_w < 3 || src_h < 3)
 			state->visible = false;
 
+		pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
 		width_bytes = ((src_x * pixel_size) & 63) +
 					src_w * pixel_size;
 
@@ -1276,6 +1282,17 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	bool primary_enabled;
 
 	/*
+	 * 'prepare' is never called when plane is being disabled, so we need
+	 * to handle frontbuffer tracking here
+	 */
+	if (!fb) {
+		mutex_lock(&dev->struct_mutex);
+		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
+				  INTEL_FRONTBUFFER_SPRITE(pipe));
+		mutex_unlock(&dev->struct_mutex);
+	}
+
+	/*
 	 * If the sprite is completely covering the primary plane,
 	 * we can disable the primary and save power.
 	 */
@@ -1327,50 +1344,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	}
 }
 
-static int
-intel_disable_plane(struct drm_plane *plane)
-{
-	struct drm_device *dev = plane->dev;
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-	struct intel_crtc *intel_crtc;
-	enum pipe pipe;
-
-	if (!plane->fb)
-		return 0;
-
-	if (WARN_ON(!plane->crtc))
-		return -EINVAL;
-
-	intel_crtc = to_intel_crtc(plane->crtc);
-	pipe = intel_crtc->pipe;
-
-	if (intel_crtc->active) {
-		bool primary_was_enabled = intel_crtc->primary_enabled;
-
-		intel_crtc->primary_enabled = true;
-
-		intel_plane->disable_plane(plane, plane->crtc);
-
-		if (!primary_was_enabled && intel_crtc->primary_enabled)
-			intel_post_enable_primary(plane->crtc);
-	}
-
-	if (intel_plane->obj) {
-		if (intel_crtc->active)
-			intel_wait_for_vblank(dev, intel_plane->pipe);
-
-		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(intel_plane->obj);
-		i915_gem_track_fb(intel_plane->obj, NULL,
-				  INTEL_FRONTBUFFER_SPRITE(pipe));
-		mutex_unlock(&dev->struct_mutex);
-
-		intel_plane->obj = NULL;
-	}
-
-	return 0;
-}
-
 static void intel_destroy_plane(struct drm_plane *plane)
 {
 	struct intel_plane *intel_plane = to_intel_plane(plane);
@@ -1478,14 +1451,6 @@ int intel_plane_restore(struct drm_plane *plane)
 				  intel_plane->src_w, intel_plane->src_h);
 }
 
-void intel_plane_disable(struct drm_plane *plane)
-{
-	if (!plane->crtc || !plane->fb)
-		return;
-
-	intel_disable_plane(plane);
-}
-
 static const struct drm_plane_funcs intel_plane_funcs = {
 	.update_plane = intel_update_plane,
 	.disable_plane = intel_disable_plane,
-- 
1.8.5.1

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

Re: [PATCH 9/9] drm/i915: Make all plane disables use

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -1              367/367              366/367
ILK                 -5              375/375              370/375
SNB                 -1              450/450              449/450
IVB                 -4              503/503              499/503
BYT                                  289/289              289/289
HSW                 -3              567/567              564/567
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gen3_mixed_blits      PASS(6, M23)      CRASH(1, M23)
 ILK  igt_gem_reset_stats_close-pending-fork-render      TIMEOUT(11, M37M26)PASS(1, M26)      TIMEOUT(1, M26)
 ILK  igt_kms_3d      DMESG_WARN(1, M26)PASS(3, M37M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_render_direct-render      PASS(4, M37M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_rcs-flip-vs-modeset      DMESG_WARN(2, M26)PASS(1, M37)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_vblank-vs-hang      TIMEOUT(11, M37M26)PASS(1, M26)      TIMEOUT(1, M26)
*SNB  igt_kms_3d      PASS(2, M22M35)      DMESG_WARN(1, M35)
 IVB  igt_gem_bad_reloc_negative-reloc      NSPT(14, M34M21M4)PASS(1, M21)      NSPT(1, M21)
 IVB  igt_gem_bad_reloc_negative-reloc-lut      NSPT(3, M21M34M4)PASS(15, M21M34M4)      NSPT(1, M21)
*IVB  igt_kms_3d      PASS(2, M21)      DMESG_WARN(1, M21)
*IVB  igt_kms_cursor_crc_cursor-64x64-offscreen      PASS(2, M21)      DMESG_WARN(1, M21)
 HSW  igt_gem_bad_reloc_negative-reloc-lut      NSPT(24, M40M20M19)PASS(1, M20)      NSPT(1, M19)
*HSW  igt_kms_rotation_crc_primary-rotation      PASS(23, M20M40M19)      DMESG_WARN(1, M19)
*HSW  igt_pm_rc6_residency_rc6-accuracy      PASS(25, M20M40M19)      FAIL(1, M19)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 9/9] drm/i915: Make all plane disables use

[Damien Lespiau]

Hi Shuang,

The threading is still broken in some MUAs. I believe we also need the
References: header in addition to in-reply-to: to solve that.

HTH,

-- 
Damien

On Mon, Nov 24, 2014 at 11:39:48PM -0800, shuang.he@intel.com wrote:
> Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
> -------------------------------------Summary-------------------------------------
> Platform          Delta          drm-intel-nightly          Series Applied
> PNV                 -1              367/367              366/367
> ILK                 -5              375/375              370/375
> SNB                 -1              450/450              449/450
> IVB                 -4              503/503              499/503
> BYT                                  289/289              289/289
> HSW                 -3              567/567              564/567
> BDW                                  417/417              417/417
> -------------------------------------Detailed-------------------------------------
> Platform  Test                                drm-intel-nightly          Series Applied
> *PNV  igt_gen3_mixed_blits      PASS(6, M23)      CRASH(1, M23)
>  ILK  igt_gem_reset_stats_close-pending-fork-render      TIMEOUT(11, M37M26)PASS(1, M26)      TIMEOUT(1, M26)
>  ILK  igt_kms_3d      DMESG_WARN(1, M26)PASS(3, M37M26)      DMESG_WARN(1, M26)
> *ILK  igt_kms_render_direct-render      PASS(4, M37M26)      DMESG_WARN(1, M26)
>  ILK  igt_kms_flip_rcs-flip-vs-modeset      DMESG_WARN(2, M26)PASS(1, M37)      DMESG_WARN(1, M26)
>  ILK  igt_kms_flip_vblank-vs-hang      TIMEOUT(11, M37M26)PASS(1, M26)      TIMEOUT(1, M26)
> *SNB  igt_kms_3d      PASS(2, M22M35)      DMESG_WARN(1, M35)
>  IVB  igt_gem_bad_reloc_negative-reloc      NSPT(14, M34M21M4)PASS(1, M21)      NSPT(1, M21)
>  IVB  igt_gem_bad_reloc_negative-reloc-lut      NSPT(3, M21M34M4)PASS(15, M21M34M4)      NSPT(1, M21)
> *IVB  igt_kms_3d      PASS(2, M21)      DMESG_WARN(1, M21)
> *IVB  igt_kms_cursor_crc_cursor-64x64-offscreen      PASS(2, M21)      DMESG_WARN(1, M21)
>  HSW  igt_gem_bad_reloc_negative-reloc-lut      NSPT(24, M40M20M19)PASS(1, M20)      NSPT(1, M19)
> *HSW  igt_kms_rotation_crc_primary-rotation      PASS(23, M20M40M19)      DMESG_WARN(1, M19)
> *HSW  igt_pm_rc6_residency_rc6-accuracy      PASS(25, M20M40M19)      FAIL(1, M19)
> Note: You need to pay more attention to line start with '*'
> _______________________________________________
> 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 9/9] drm/i915: Make all plane disables use

[He, Shuang]

No problem, I will add it

Thanks
	--Shuang

> -----Original Message-----
> From: Lespiau, Damien
> Sent: Thursday, November 27, 2014 11:58 PM
> To: He, Shuang
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 9/9] drm/i915: Make all plane disables use
> 
> Hi Shuang,
> 
> The threading is still broken in some MUAs. I believe we also need the
> References: header in addition to in-reply-to: to solve that.
> 
> HTH,
> 
> --
> Damien
> 
> On Mon, Nov 24, 2014 at 11:39:48PM -0800, shuang.he@intel.com wrote:
> > Tested-By: PRC QA PRTS (Patch Regression Test System Contact:
> shuang.he@intel.com)
> > -------------------------------------Summary-------------------------------------
> > Platform          Delta          drm-intel-nightly          Series
> Applied
> > PNV                 -1              367/367
> 366/367
> > ILK                 -5              375/375              370/375
> > SNB                 -1              450/450
> 449/450
> > IVB                 -4              503/503
> 499/503
> > BYT                                  289/289
> 289/289
> > HSW                 -3              567/567
> 564/567
> > BDW                                  417/417
> 417/417
> > -------------------------------------Detailed-------------------------------------
> > Platform  Test                                drm-intel-nightly
> Series Applied
> > *PNV  igt_gen3_mixed_blits      PASS(6, M23)      CRASH(1, M23)
> >  ILK  igt_gem_reset_stats_close-pending-fork-render      TIMEOUT(11,
> M37M26)PASS(1, M26)      TIMEOUT(1, M26)
> >  ILK  igt_kms_3d      DMESG_WARN(1, M26)PASS(3, M37M26)
> DMESG_WARN(1, M26)
> > *ILK  igt_kms_render_direct-render      PASS(4, M37M26)
> DMESG_WARN(1, M26)
> >  ILK  igt_kms_flip_rcs-flip-vs-modeset      DMESG_WARN(2, M26)PASS(1,
> M37)      DMESG_WARN(1, M26)
> >  ILK  igt_kms_flip_vblank-vs-hang      TIMEOUT(11, M37M26)PASS(1,
> M26)      TIMEOUT(1, M26)
> > *SNB  igt_kms_3d      PASS(2, M22M35)      DMESG_WARN(1, M35)
> >  IVB  igt_gem_bad_reloc_negative-reloc      NSPT(14,
> M34M21M4)PASS(1, M21)      NSPT(1, M21)
> >  IVB  igt_gem_bad_reloc_negative-reloc-lut      NSPT(3,
> M21M34M4)PASS(15, M21M34M4)      NSPT(1, M21)
> > *IVB  igt_kms_3d      PASS(2, M21)      DMESG_WARN(1, M21)
> > *IVB  igt_kms_cursor_crc_cursor-64x64-offscreen      PASS(2, M21)
> DMESG_WARN(1, M21)
> >  HSW  igt_gem_bad_reloc_negative-reloc-lut      NSPT(24,
> M40M20M19)PASS(1, M20)      NSPT(1, M19)
> > *HSW  igt_kms_rotation_crc_primary-rotation      PASS(23,
> M20M40M19)      DMESG_WARN(1, M19)
> > *HSW  igt_pm_rc6_residency_rc6-accuracy      PASS(25, M20M40M19)
> FAIL(1, M19)
> > Note: You need to pay more attention to line start with '*'
> > _______________________________________________
> > 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 9/9] drm/i915: Make all plane disables use

[Damien Lespiau]

Hi,

So References: is now in the new replies, but that's still not enough to
make threading work in gmail.

Maybe truncating the subject is the cause then? Headers can be
multi-lines and you're only taking the first line. For instance:

Subject: [Intel-gfx] [PATCH] drm/i915: Disable FBC if primary plane is
	rotated on Haswell

The Subject header carries over a second line, while your script only
takes the first. Can we try to fix that and see if it then threads
properly?

Thanks,

-- 
Damien

On Fri, Nov 28, 2014 at 12:53:41AM +0000, He, Shuang wrote:
> No problem, I will add it
> 
> Thanks
> 	--Shuang
> 
> > -----Original Message-----
> > From: Lespiau, Damien
> > Sent: Thursday, November 27, 2014 11:58 PM
> > To: He, Shuang
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 9/9] drm/i915: Make all plane disables use
> > 
> > Hi Shuang,
> > 
> > The threading is still broken in some MUAs. I believe we also need the
> > References: header in addition to in-reply-to: to solve that.
> > 
> > HTH,
> > 
> > --
> > Damien
> > 
> > On Mon, Nov 24, 2014 at 11:39:48PM -0800, shuang.he@intel.com wrote:
> > > Tested-By: PRC QA PRTS (Patch Regression Test System Contact:
> > shuang.he@intel.com)
> > > -------------------------------------Summary-------------------------------------
> > > Platform          Delta          drm-intel-nightly          Series
> > Applied
> > > PNV                 -1              367/367
> > 366/367
> > > ILK                 -5              375/375              370/375
> > > SNB                 -1              450/450
> > 449/450
> > > IVB                 -4              503/503
> > 499/503
> > > BYT                                  289/289
> > 289/289
> > > HSW                 -3              567/567
> > 564/567
> > > BDW                                  417/417
> > 417/417
> > > -------------------------------------Detailed-------------------------------------
> > > Platform  Test                                drm-intel-nightly
> > Series Applied
> > > *PNV  igt_gen3_mixed_blits      PASS(6, M23)      CRASH(1, M23)
> > >  ILK  igt_gem_reset_stats_close-pending-fork-render      TIMEOUT(11,
> > M37M26)PASS(1, M26)      TIMEOUT(1, M26)
> > >  ILK  igt_kms_3d      DMESG_WARN(1, M26)PASS(3, M37M26)
> > DMESG_WARN(1, M26)
> > > *ILK  igt_kms_render_direct-render      PASS(4, M37M26)
> > DMESG_WARN(1, M26)
> > >  ILK  igt_kms_flip_rcs-flip-vs-modeset      DMESG_WARN(2, M26)PASS(1,
> > M37)      DMESG_WARN(1, M26)
> > >  ILK  igt_kms_flip_vblank-vs-hang      TIMEOUT(11, M37M26)PASS(1,
> > M26)      TIMEOUT(1, M26)
> > > *SNB  igt_kms_3d      PASS(2, M22M35)      DMESG_WARN(1, M35)
> > >  IVB  igt_gem_bad_reloc_negative-reloc      NSPT(14,
> > M34M21M4)PASS(1, M21)      NSPT(1, M21)
> > >  IVB  igt_gem_bad_reloc_negative-reloc-lut      NSPT(3,
> > M21M34M4)PASS(15, M21M34M4)      NSPT(1, M21)
> > > *IVB  igt_kms_3d      PASS(2, M21)      DMESG_WARN(1, M21)
> > > *IVB  igt_kms_cursor_crc_cursor-64x64-offscreen      PASS(2, M21)
> > DMESG_WARN(1, M21)
> > >  HSW  igt_gem_bad_reloc_negative-reloc-lut      NSPT(24,
> > M40M20M19)PASS(1, M20)      NSPT(1, M19)
> > > *HSW  igt_kms_rotation_crc_primary-rotation      PASS(23,
> > M20M40M19)      DMESG_WARN(1, M19)
> > > *HSW  igt_pm_rc6_residency_rc6-accuracy      PASS(25, M20M40M19)
> > FAIL(1, M19)
> > > Note: You need to pay more attention to line start with '*'
> > > _______________________________________________
> > > 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 9/9] drm/i915: Make all plane disables use

[Jani Nikula]

On Fri, 28 Nov 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> Hi,
>
> So References: is now in the new replies, but that's still not enough to
> make threading work in gmail.
>
> Maybe truncating the subject is the cause then? Headers can be
> multi-lines and you're only taking the first line. For instance:
>
> Subject: [Intel-gfx] [PATCH] drm/i915: Disable FBC if primary plane is
> 	rotated on Haswell
>
> The Subject header carries over a second line, while your script only
> takes the first. Can we try to fix that and see if it then threads
> properly?

Arguably gmail is broken in this sense, but the subject changing is the
likely culprit here.

http://mid.gmane.org/87fvd83ooi.fsf@intel.com


BR,
Jani.


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

Re: [PATCH 9/9] drm/i915: Make all plane disables use 'update_plane'

[Ander Conselvan de Oliveira]

On 11/24/2014 09:53 PM, Matt Roper wrote:
> If we extend the commit_plane handlers for each plane type to be able to
> handle fb=0, then we can easily implement plane disable via the
> update_plane handler.  The cursor plane already works this way, and this
> is the direction we need to go to integrate with the atomic plane
> handler.  We can now kill off the type-specific disable functions, as
> well as the redundant intel_plane_disable() (not to be confused with
> intel_disable_plane()).
>
> Note that prepare_plane_fb() only gets called as part of update_plane
> when fb!=NULL (by design, to match the semantics of the atomic plane
> helpers); this means that our commit_plane handlers need to handle the
> frontbuffer tracking for the disable case, even though they don't handle
> it for normal updates.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 109 +++++++++++++++--------------------
>   drivers/gpu/drm/i915/intel_drv.h     |   2 +-
>   drivers/gpu/drm/i915/intel_sprite.c  |  71 ++++++-----------------
>   3 files changed, 67 insertions(+), 115 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index dd10dc6..fd1eaf5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3996,7 +3996,7 @@ static void intel_disable_planes(struct drm_crtc *crtc)
>   	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
>   		intel_plane = to_intel_plane(plane);
>   		if (intel_plane->pipe == pipe)
> -			intel_plane_disable(&intel_plane->base);
> +			plane->funcs->disable_plane(plane);
>   	}
>   }
>
> @@ -11535,45 +11535,6 @@ static void intel_shared_dpll_init(struct drm_device *dev)
>   	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
>   }
>
> -static int
> -intel_primary_plane_disable(struct drm_plane *plane)
> -{
> -	struct drm_device *dev = plane->dev;
> -	struct intel_crtc *intel_crtc;
> -
> -	if (!plane->fb)
> -		return 0;
> -
> -	BUG_ON(!plane->crtc);
> -
> -	intel_crtc = to_intel_crtc(plane->crtc);
> -
> -	/*
> -	 * Even though we checked plane->fb above, it's still possible that
> -	 * the primary plane has been implicitly disabled because the crtc
> -	 * coordinates given weren't visible, or because we detected
> -	 * that it was 100% covered by a sprite plane.  Or, the CRTC may be
> -	 * off and we've set a fb, but haven't actually turned on the CRTC yet.
> -	 * In either case, we need to unpin the FB and let the fb pointer get
> -	 * updated, but otherwise we don't need to touch the hardware.
> -	 */
> -	if (!intel_crtc->primary_enabled)
> -		goto disable_unpin;
> -
> -	intel_crtc_wait_for_pending_flips(plane->crtc);
> -	intel_disable_primary_hw_plane(plane, plane->crtc);
> -
> -disable_unpin:
> -	mutex_lock(&dev->struct_mutex);
> -	i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
> -			  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
> -	intel_unpin_fb_obj(intel_fb_obj(plane->fb));
> -	mutex_unlock(&dev->struct_mutex);
> -	plane->fb = NULL;
> -
> -	return 0;
> -}
> -
>   /**
>    * intel_prepare_plane_fb - Prepare fb for usage on plane
>    * @plane: drm plane to prepare for
> @@ -11678,10 +11639,12 @@ intel_check_primary_plane(struct drm_plane *plane,
>   	if (ret)
>   		return ret;
>
> -	intel_crtc_wait_for_pending_flips(crtc);
> -	if (intel_crtc_has_pending_flip(crtc)) {
> -		DRM_ERROR("pipe is still busy with an old pageflip\n");
> -		return -EBUSY;
> +	if (plane->crtc) {
> +		intel_crtc_wait_for_pending_flips(plane->crtc);
> +		if (intel_crtc_has_pending_flip(plane->crtc)) {
> +			DRM_ERROR("pipe is still busy with an old pageflip\n");
> +			return -EBUSY;
> +		}
>   	}
>
>   	return 0;
> @@ -11696,12 +11659,30 @@ intel_commit_primary_plane(struct drm_plane *plane,
>   	struct drm_device *dev = plane->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_i915_gem_object *obj = intel_fb_obj(fb);
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_rect *src = &state->src;
> +	enum pipe pipe = intel_plane->pipe;
>
> -	crtc->primary->fb = fb;
> +	if (!fb) {
> +		/*
> +		 * 'prepare' is never called when plane is being disabled, so
> +		 * we need to handle frontbuffer tracking here
> +		 */
> +		mutex_lock(&dev->struct_mutex);
> +		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
> +				  INTEL_FRONTBUFFER_PRIMARY(pipe));
> +		mutex_unlock(&dev->struct_mutex);
> +
> +		/*
> +		 * crtc may be passed as NULL when disabling, so set it to
> +		 * the proper value now
> +		 */
> +		crtc = plane->crtc;
> +		intel_crtc = to_intel_crtc(crtc);
> +	}
> +
> +	plane->fb = fb;
>   	crtc->x = src->x1 >> 16;
>   	crtc->y = src->y1 >> 16;
>
> @@ -11759,7 +11740,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
>   			 * explicitly disables the plane by passing fb=0
>   			 * because plane->fb still gets set and pinned.
>   			 */
> -			intel_disable_primary_hw_plane(plane, crtc);
> +			intel_disable_primary_hw_plane(plane, plane->crtc);

I can't see why this hunk is necessary, since you set crtc = plane->crtc 
above for the !fb case. Or am I missing something?

>   		}
>
>   		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
> @@ -11831,6 +11812,24 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   	return 0;
>   }
>
> +/**
> + * intel_disable_plane - disable a plane
> + * @plane: plane to disable
> + *
> + * General disable handler for all plane types.
> + */
> +int
> +intel_disable_plane(struct drm_plane *plane)
> +{
> +	if (!plane->fb)
> +		return 0;
> +
> +	BUG_ON(!plane->crtc);

WARN_ON? Maybe not since intel_update_plane will oops. I'm not sure 
what's the guideline here.

Anyway, the series look pretty good overall.

Cheers,
Ander

> +
> +	return plane->funcs->update_plane(plane, plane->crtc, NULL,
> +					  0, 0, 0, 0, 0, 0, 0, 0);
> +}
> +
>   /* Common destruction function for both primary and cursor planes */
>   static void intel_plane_destroy(struct drm_plane *plane)
>   {
> @@ -11841,7 +11840,7 @@ static void intel_plane_destroy(struct drm_plane *plane)
>
>   static const struct drm_plane_funcs intel_primary_plane_funcs = {
>   	.update_plane = intel_update_plane,
> -	.disable_plane = intel_primary_plane_disable,
> +	.disable_plane = intel_disable_plane,
>   	.destroy = intel_plane_destroy,
>   	.set_property = intel_plane_set_property
>   };
> @@ -11896,18 +11895,6 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>   }
>
>   static int
> -intel_cursor_plane_disable(struct drm_plane *plane)
> -{
> -	if (!plane->fb)
> -		return 0;
> -
> -	BUG_ON(!plane->crtc);
> -
> -	return plane->funcs->update_plane(plane, plane->crtc, NULL,
> -					  0, 0, 0, 0, 0, 0, 0, 0);
> -}
> -
> -static int
>   intel_check_cursor_plane(struct drm_plane *plane,
>   			 struct intel_plane_state *state)
>   {
> @@ -12030,7 +12017,7 @@ update:
>
>   static const struct drm_plane_funcs intel_cursor_plane_funcs = {
>   	.update_plane = intel_update_plane,
> -	.disable_plane = intel_cursor_plane_disable,
> +	.disable_plane = intel_disable_plane,
>   	.destroy = intel_plane_destroy,
>   	.set_property = intel_plane_set_property,
>   };
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5f7a071..da4da2c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1021,6 +1021,7 @@ int intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   		       unsigned int crtc_w, unsigned int crtc_h,
>   		       uint32_t src_x, uint32_t src_y,
>   		       uint32_t src_w, uint32_t src_h);
> +int intel_disable_plane(struct drm_plane *plane);
>
>   /* intel_dp_mst.c */
>   int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
> @@ -1201,7 +1202,6 @@ int intel_plane_set_property(struct drm_plane *plane,
>   			     struct drm_property *prop,
>   			     uint64_t val);
>   int intel_plane_restore(struct drm_plane *plane);
> -void intel_plane_disable(struct drm_plane *plane);
>   int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>   			      struct drm_file *file_priv);
>   int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index bfd5270..bc5834b 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1109,7 +1109,12 @@ intel_check_sprite_plane(struct drm_plane *plane,
>   	const struct drm_rect *clip = &state->clip;
>   	int hscale, vscale;
>   	int max_scale, min_scale;
> -	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> +	int pixel_size;
> +
> +	if (!fb) {
> +		state->visible = false;
> +		return 0;
> +	}
>
>   	/* Don't modify another pipe's plane */
>   	if (intel_plane->pipe != intel_crtc->pipe) {
> @@ -1232,6 +1237,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>   		if (src_w < 3 || src_h < 3)
>   			state->visible = false;
>
> +		pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>   		width_bytes = ((src_x * pixel_size) & 63) +
>   					src_w * pixel_size;
>
> @@ -1276,6 +1282,17 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   	bool primary_enabled;
>
>   	/*
> +	 * 'prepare' is never called when plane is being disabled, so we need
> +	 * to handle frontbuffer tracking here
> +	 */
> +	if (!fb) {
> +		mutex_lock(&dev->struct_mutex);
> +		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
> +				  INTEL_FRONTBUFFER_SPRITE(pipe));
> +		mutex_unlock(&dev->struct_mutex);
> +	}
> +
> +	/*
>   	 * If the sprite is completely covering the primary plane,
>   	 * we can disable the primary and save power.
>   	 */
> @@ -1327,50 +1344,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   	}
>   }
>
> -static int
> -intel_disable_plane(struct drm_plane *plane)
> -{
> -	struct drm_device *dev = plane->dev;
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	struct intel_crtc *intel_crtc;
> -	enum pipe pipe;
> -
> -	if (!plane->fb)
> -		return 0;
> -
> -	if (WARN_ON(!plane->crtc))
> -		return -EINVAL;
> -
> -	intel_crtc = to_intel_crtc(plane->crtc);
> -	pipe = intel_crtc->pipe;
> -
> -	if (intel_crtc->active) {
> -		bool primary_was_enabled = intel_crtc->primary_enabled;
> -
> -		intel_crtc->primary_enabled = true;
> -
> -		intel_plane->disable_plane(plane, plane->crtc);
> -
> -		if (!primary_was_enabled && intel_crtc->primary_enabled)
> -			intel_post_enable_primary(plane->crtc);
> -	}
> -
> -	if (intel_plane->obj) {
> -		if (intel_crtc->active)
> -			intel_wait_for_vblank(dev, intel_plane->pipe);
> -
> -		mutex_lock(&dev->struct_mutex);
> -		intel_unpin_fb_obj(intel_plane->obj);
> -		i915_gem_track_fb(intel_plane->obj, NULL,
> -				  INTEL_FRONTBUFFER_SPRITE(pipe));
> -		mutex_unlock(&dev->struct_mutex);
> -
> -		intel_plane->obj = NULL;
> -	}
> -
> -	return 0;
> -}
> -
>   static void intel_destroy_plane(struct drm_plane *plane)
>   {
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
> @@ -1478,14 +1451,6 @@ int intel_plane_restore(struct drm_plane *plane)
>   				  intel_plane->src_w, intel_plane->src_h);
>   }
>
> -void intel_plane_disable(struct drm_plane *plane)
> -{
> -	if (!plane->crtc || !plane->fb)
> -		return;
> -
> -	intel_disable_plane(plane);
> -}
> -
>   static const struct drm_plane_funcs intel_plane_funcs = {
>   	.update_plane = intel_update_plane,
>   	.disable_plane = intel_disable_plane,
>

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

Re: [PATCH 9/9] drm/i915: Make all plane disables use 'update_plane'

[Daniel Vetter]

On Mon, Dec 01, 2014 at 04:04:18PM +0200, Ander Conselvan de Oliveira wrote:
> On 11/24/2014 09:53 PM, Matt Roper wrote:
> >+/**
> >+ * intel_disable_plane - disable a plane
> >+ * @plane: plane to disable
> >+ *
> >+ * General disable handler for all plane types.
> >+ */
> >+int
> >+intel_disable_plane(struct drm_plane *plane)
> >+{
> >+	if (!plane->fb)
> >+		return 0;
> >+
> >+	BUG_ON(!plane->crtc);
> 
> WARN_ON? Maybe not since intel_update_plane will oops. I'm not sure what's
> the guideline here.

I'd go with a if (WARN_ON) return -EINVAL; or so. We've had a bunch of fun
cases when we've done the original hw state readout code where code
fizzled out on an Oops because the fb wasn't reconstructed. And doing that
at boot-up while holding console_lock is really bad for debugging.

But yeah in general keeping the BUG_ON if you'll oops otherwise anyway is
the correct thing to do.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx