[RFC 1/3] drm/i915: remove !enabled handling from commit primary plane step

[RFC 1/3] drm/i915: remove !enabled handling from commit primary plane step

[Gustavo Padovan]

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

The !crtc->enabled case will now be handled by the !visible code,
since the handling is basically the same.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5279b99..2ccf7c0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11837,32 +11837,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	struct drm_rect *src = &state->src;
 	int ret;
 
-	/*
-	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
-	 * updating the fb pointer, and returning without touching the
-	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
-	 * turn on the display with all planes setup as desired.
-	 */
-	if (!crtc->enabled) {
-		mutex_lock(&dev->struct_mutex);
-
-		/*
-		 * If we already called setplane while the crtc was disabled,
-		 * we may have an fb pinned; unpin it.
-		 */
-		if (plane->fb)
-			intel_unpin_fb_obj(old_obj);
-
-		i915_gem_track_fb(old_obj, obj,
-				  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
-
-		/* Pin and return without programming hardware */
-		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
-		mutex_unlock(&dev->struct_mutex);
-
-		return ret;
-	}
-
 	intel_crtc_wait_for_pending_flips(crtc);
 
 	/*
-- 
1.9.3

[RFC 2/3] drm/i915: create intel_update_pipe_size()

[Gustavo Padovan]

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

Factor out a piece of code from intel_pipe_set_base() that updates
the pipe size and adjust fitter.

This will help refactor the update primary plane path.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 71 +++++++++++++++++++++---------------
 1 file changed, 41 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2ccf7c0..e7e7184 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2779,6 +2779,46 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
 	return pending;
 }
 
+static void intel_update_pipe_size(struct drm_crtc *crtc)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	const struct drm_display_mode *adjusted_mode;
+
+	if (!i915.fastboot)
+		return;
+
+	/*
+	 * Update pipe size and adjust fitter if needed: the reason for this is
+	 * that in compute_mode_changes we check the native mode (not the pfit
+	 * mode) to see if we can flip rather than do a full mode set. In the
+	 * fastboot case, we'll flip, but if we don't update the pipesrc and
+	 * pfit state, we'll end up with a big fb scanned out into the wrong
+	 * sized surface.
+	 *
+	 * To fix this properly, we need to hoist the checks up into
+	 * compute_mode_changes (or above), check the actual pfit state and
+	 * whether the platform allows pfit disable with pipe active, and only
+	 * then update the pipesrc and pfit state, even on the flip path.
+	 */
+
+	adjusted_mode = &intel_crtc->config.adjusted_mode;
+
+	I915_WRITE(PIPESRC(intel_crtc->pipe),
+		   ((adjusted_mode->crtc_hdisplay - 1) << 16) |
+		   (adjusted_mode->crtc_vdisplay - 1));
+	if (!intel_crtc->config.pch_pfit.enabled &&
+	    (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||
+	     intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
+		I915_WRITE(PF_CTL(intel_crtc->pipe), 0);
+		I915_WRITE(PF_WIN_POS(intel_crtc->pipe), 0);
+		I915_WRITE(PF_WIN_SZ(intel_crtc->pipe), 0);
+	}
+	intel_crtc->config.pipe_src_w = adjusted_mode->crtc_hdisplay;
+	intel_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)
@@ -2821,36 +2861,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		return ret;
 	}
 
-	/*
-	 * Update pipe size and adjust fitter if needed: the reason for this is
-	 * that in compute_mode_changes we check the native mode (not the pfit
-	 * mode) to see if we can flip rather than do a full mode set. In the
-	 * fastboot case, we'll flip, but if we don't update the pipesrc and
-	 * pfit state, we'll end up with a big fb scanned out into the wrong
-	 * sized surface.
-	 *
-	 * To fix this properly, we need to hoist the checks up into
-	 * compute_mode_changes (or above), check the actual pfit state and
-	 * whether the platform allows pfit disable with pipe active, and only
-	 * then update the pipesrc and pfit state, even on the flip path.
-	 */
-	if (i915.fastboot) {
-		const struct drm_display_mode *adjusted_mode =
-			&intel_crtc->config.adjusted_mode;
-
-		I915_WRITE(PIPESRC(intel_crtc->pipe),
-			   ((adjusted_mode->crtc_hdisplay - 1) << 16) |
-			   (adjusted_mode->crtc_vdisplay - 1));
-		if (!intel_crtc->config.pch_pfit.enabled &&
-		    (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||
-		     intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
-			I915_WRITE(PF_CTL(intel_crtc->pipe), 0);
-			I915_WRITE(PF_WIN_POS(intel_crtc->pipe), 0);
-			I915_WRITE(PF_WIN_SZ(intel_crtc->pipe), 0);
-		}
-		intel_crtc->config.pipe_src_w = adjusted_mode->crtc_hdisplay;
-		intel_crtc->config.pipe_src_h = adjusted_mode->crtc_vdisplay;
-	}
+	intel_update_pipe_size(crtc);
 
 	dev_priv->display.update_primary_plane(crtc, fb, x, y);
 
-- 
1.9.3

[RFC 3/3] drm/i915: Merge of visible and !visible paths for primary planes

[Gustavo Padovan]

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

Fold intel_pipe_set_base() in the update primary plane path merging
pieces of code that are common to both paths.

Basically the the pin/unpin procedures are the same for both paths
and some checks can also be shared (some of the were moved to the
check() stage)

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 100 ++++++++++++++++++++++++-----------
 1 file changed, 69 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e7e7184..175a284 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11821,16 +11821,36 @@ intel_check_primary_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_device *dev = crtc->dev;
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
+	int ret;
 
-	return drm_plane_helper_check_update(plane, crtc, fb,
+	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;
+
+	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;
+	}
+
+	/* no fb bound */
+	if (state->visible && !fb) {
+		DRM_ERROR("No FB bound\n");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static int
@@ -11842,14 +11862,34 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	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 = 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;
-	int ret;
+	int ret = 0;
 
 	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;
+	}
+
+	mutex_lock(&dev->struct_mutex);
+	if (plane->fb != fb) {
+		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+		if (ret == 0)
+			i915_gem_track_fb(old_obj, obj,
+					  INTEL_FRONTBUFFER_PRIMARY(pipe));
+	}
+	mutex_unlock(&dev->struct_mutex);
+	if (ret != 0) {
+		DRM_ERROR("pin & fence failed\n");
+		return ret;
+	}
+
 	/*
 	 * If clipping results in a non-visible primary plane, we'll disable
 	 * the primary plane.  Note that this is a bit different than what
@@ -11857,33 +11897,9 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	 * because plane->fb still gets set and pinned.
 	 */
 	if (!state->visible) {
-		mutex_lock(&dev->struct_mutex);
-
-		/*
-		 * Try to pin the new fb first so that we can bail out if we
-		 * fail.
-		 */
-		if (plane->fb != fb) {
-			ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
-			if (ret) {
-				mutex_unlock(&dev->struct_mutex);
-				return ret;
-			}
-		}
-
-		i915_gem_track_fb(old_obj, obj,
-				  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
-
 		if (intel_crtc->primary_enabled)
 			intel_disable_primary_hw_plane(plane, crtc);
 
-
-		if (plane->fb != fb)
-			if (plane->fb)
-				intel_unpin_fb_obj(old_obj);
-
-		mutex_unlock(&dev->struct_mutex);
-
 	} else {
 		if (intel_crtc && intel_crtc->active &&
 		    intel_crtc->primary_enabled) {
@@ -11903,12 +11919,34 @@ intel_commit_primary_plane(struct drm_plane *plane,
 				intel_disable_fbc(dev);
 			}
 		}
-		ret = intel_pipe_set_base(crtc, src->x1, src->y1, fb);
-		if (ret)
-			return ret;
 
-		if (!intel_crtc->primary_enabled)
-			intel_enable_primary_hw_plane(plane, crtc);
+		intel_update_pipe_size(crtc);
+
+		intel_crtc->primary_enabled = true;
+
+		dev_priv->display.update_primary_plane(crtc, fb, src->x1, src->y1);
+
+		mutex_lock(&dev->struct_mutex);
+		intel_update_fbc(dev);
+		mutex_unlock(&dev->struct_mutex);
+	}
+
+	crtc->primary->fb = fb;
+	crtc->x = src->x1;
+	crtc->y = src->y1;
+
+	if (intel_crtc->active)
+		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
+
+	if (old_fb) {
+		if (intel_crtc->active && old_fb != fb)
+			intel_wait_for_vblank(dev, intel_crtc->pipe);
+
+		if (old_fb != fb) {
+			mutex_lock(&dev->struct_mutex);
+			intel_unpin_fb_obj(old_obj);
+			mutex_unlock(&dev->struct_mutex);
+		}
 	}
 
 	intel_plane->crtc_x = state->orig_dst.x1;
-- 
1.9.3

Re: [RFC 2/3] drm/i915: create intel_update_pipe_size()

[Ville Syrjälä]

On Tue, Sep 09, 2014 at 11:43:20AM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Factor out a piece of code from intel_pipe_set_base() that updates
> the pipe size and adjust fitter.
> 
> This will help refactor the update primary plane path.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 71 +++++++++++++++++++++---------------
>  1 file changed, 41 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2ccf7c0..e7e7184 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2779,6 +2779,46 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
>  	return pending;
>  }
>  
> +static void intel_update_pipe_size(struct drm_crtc *crtc)

These days we usually prefer to pass intel_crtc instead of drm_crtc. You
can still call it 'crtc' since that's shorter and because we don't need
anything from drm_crtc in this function there won't be any confusion
between the two.

Otherwise the patch looks good.

> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	const struct drm_display_mode *adjusted_mode;
> +
> +	if (!i915.fastboot)
> +		return;
> +
> +	/*
> +	 * Update pipe size and adjust fitter if needed: the reason for this is
> +	 * that in compute_mode_changes we check the native mode (not the pfit
> +	 * mode) to see if we can flip rather than do a full mode set. In the
> +	 * fastboot case, we'll flip, but if we don't update the pipesrc and
> +	 * pfit state, we'll end up with a big fb scanned out into the wrong
> +	 * sized surface.
> +	 *
> +	 * To fix this properly, we need to hoist the checks up into
> +	 * compute_mode_changes (or above), check the actual pfit state and
> +	 * whether the platform allows pfit disable with pipe active, and only
> +	 * then update the pipesrc and pfit state, even on the flip path.
> +	 */
> +
> +	adjusted_mode = &intel_crtc->config.adjusted_mode;
> +
> +	I915_WRITE(PIPESRC(intel_crtc->pipe),
> +		   ((adjusted_mode->crtc_hdisplay - 1) << 16) |
> +		   (adjusted_mode->crtc_vdisplay - 1));
> +	if (!intel_crtc->config.pch_pfit.enabled &&
> +	    (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||
> +	     intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
> +		I915_WRITE(PF_CTL(intel_crtc->pipe), 0);
> +		I915_WRITE(PF_WIN_POS(intel_crtc->pipe), 0);
> +		I915_WRITE(PF_WIN_SZ(intel_crtc->pipe), 0);
> +	}
> +	intel_crtc->config.pipe_src_w = adjusted_mode->crtc_hdisplay;
> +	intel_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)
> @@ -2821,36 +2861,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  		return ret;
>  	}
>  
> -	/*
> -	 * Update pipe size and adjust fitter if needed: the reason for this is
> -	 * that in compute_mode_changes we check the native mode (not the pfit
> -	 * mode) to see if we can flip rather than do a full mode set. In the
> -	 * fastboot case, we'll flip, but if we don't update the pipesrc and
> -	 * pfit state, we'll end up with a big fb scanned out into the wrong
> -	 * sized surface.
> -	 *
> -	 * To fix this properly, we need to hoist the checks up into
> -	 * compute_mode_changes (or above), check the actual pfit state and
> -	 * whether the platform allows pfit disable with pipe active, and only
> -	 * then update the pipesrc and pfit state, even on the flip path.
> -	 */
> -	if (i915.fastboot) {
> -		const struct drm_display_mode *adjusted_mode =
> -			&intel_crtc->config.adjusted_mode;
> -
> -		I915_WRITE(PIPESRC(intel_crtc->pipe),
> -			   ((adjusted_mode->crtc_hdisplay - 1) << 16) |
> -			   (adjusted_mode->crtc_vdisplay - 1));
> -		if (!intel_crtc->config.pch_pfit.enabled &&
> -		    (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||
> -		     intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
> -			I915_WRITE(PF_CTL(intel_crtc->pipe), 0);
> -			I915_WRITE(PF_WIN_POS(intel_crtc->pipe), 0);
> -			I915_WRITE(PF_WIN_SZ(intel_crtc->pipe), 0);
> -		}
> -		intel_crtc->config.pipe_src_w = adjusted_mode->crtc_hdisplay;
> -		intel_crtc->config.pipe_src_h = adjusted_mode->crtc_vdisplay;
> -	}
> +	intel_update_pipe_size(crtc);
>  
>  	dev_priv->display.update_primary_plane(crtc, fb, x, y);
>  
> -- 
> 1.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Re: [RFC 1/3] drm/i915: remove !enabled handling from commit primary plane step

[Ville Syrjälä]

On Tue, Sep 09, 2014 at 11:43:19AM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> The !crtc->enabled case will now be handled by the !visible code,
> since the handling is basically the same.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 26 --------------------------
>  1 file changed, 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5279b99..2ccf7c0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11837,32 +11837,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	struct drm_rect *src = &state->src;
>  	int ret;
>  
> -	/*
> -	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
> -	 * updating the fb pointer, and returning without touching the
> -	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
> -	 * turn on the display with all planes setup as desired.
> -	 */
> -	if (!crtc->enabled) {
> -		mutex_lock(&dev->struct_mutex);
> -
> -		/*
> -		 * If we already called setplane while the crtc was disabled,
> -		 * we may have an fb pinned; unpin it.
> -		 */
> -		if (plane->fb)
> -			intel_unpin_fb_obj(old_obj);
> -
> -		i915_gem_track_fb(old_obj, obj,
> -				  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
> -
> -		/* Pin and return without programming hardware */
> -		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> -		mutex_unlock(&dev->struct_mutex);
> -
> -		return ret;
> -	}
> -
>  	intel_crtc_wait_for_pending_flips(crtc);

Yeah this should work just fine.

One difference between the code paths is the intel_crtc_wait_for_pending_flips()
call, but since the crtc isn't enabled there can't be any pending flip.

The other difference is the pin vs. unpin order, but that shouldn't matter
unless there's tons of other stuff pinned as well. It's not worth optimizing
setplane calls on disabled CRTCs too much IMO.

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

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

-- 
Ville Syrjälä
Intel OTC

Re: [RFC 3/3] drm/i915: Merge of visible and !visible paths for primary planes

[Ville Syrjälä]

On Tue, Sep 09, 2014 at 11:43:21AM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Fold intel_pipe_set_base() in the update primary plane path merging
> pieces of code that are common to both paths.
> 
> Basically the the pin/unpin procedures are the same for both paths
> and some checks can also be shared (some of the were moved to the
> check() stage)
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 100 ++++++++++++++++++++++++-----------
>  1 file changed, 69 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e7e7184..175a284 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11821,16 +11821,36 @@ intel_check_primary_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
>  {
>  	struct drm_crtc *crtc = state->crtc;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_device *dev = crtc->dev;
>  	struct drm_framebuffer *fb = state->fb;
>  	struct drm_rect *dest = &state->dst;
>  	struct drm_rect *src = &state->src;
>  	const struct drm_rect *clip = &state->clip;
> +	int ret;
>  
> -	return drm_plane_helper_check_update(plane, crtc, fb,
> +	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;
> +
> +	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;
> +	}

I think we can just kill this plane check. It serves no purpose IMO.

> +
> +	/* no fb bound */
> +	if (state->visible && !fb) {
> +		DRM_ERROR("No FB bound\n");
> +		return -EINVAL;
> +	}

Hmm. This one may be needed still in cases where the BIOS fb takeover
fails. Otherwise it shouldn't happen.

> +
> +	return 0;
>  }
>  
>  static int
> @@ -11842,14 +11862,34 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	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 = 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;
> -	int ret;
> +	int ret = 0;
>  
>  	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;
> +	}

Yeah I guess we can keep this sanity check here.

> +
> +	mutex_lock(&dev->struct_mutex);
> +	if (plane->fb != fb) {
> +		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> +		if (ret == 0)
> +			i915_gem_track_fb(old_obj, obj,
> +					  INTEL_FRONTBUFFER_PRIMARY(pipe));
> +	}
> +	mutex_unlock(&dev->struct_mutex);

Could move the locking within the 'if (plane->fb != fb)' block since the
lock just protects the lower level stuff and not the plane->fb pointer.

Hmm. I started to wonder if we can do the pin/unpin conditionally, but
it must be fine, otherwise we already have some kind of problem with 
the pin count going astray.

I guess we could apply this small optimization to the sprite code as
well. There we currently do the pin/unpin unconditionally.

> +	if (ret != 0) {
> +		DRM_ERROR("pin & fence failed\n");
> +		return ret;
> +	}

Hmm. I wonder if this error is user triggerable on some platforms.
Should probably kill it or make it DRM_DEBUG_KMS or something. 

> +
>  	/*
>  	 * If clipping results in a non-visible primary plane, we'll disable
>  	 * the primary plane.  Note that this is a bit different than what
> @@ -11857,33 +11897,9 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	 * because plane->fb still gets set and pinned.
>  	 */
>  	if (!state->visible) {
> -		mutex_lock(&dev->struct_mutex);
> -
> -		/*
> -		 * Try to pin the new fb first so that we can bail out if we
> -		 * fail.
> -		 */
> -		if (plane->fb != fb) {
> -			ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> -			if (ret) {
> -				mutex_unlock(&dev->struct_mutex);
> -				return ret;
> -			}
> -		}
> -
> -		i915_gem_track_fb(old_obj, obj,
> -				  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
> -
>  		if (intel_crtc->primary_enabled)
>  			intel_disable_primary_hw_plane(plane, crtc);
>  
> -
> -		if (plane->fb != fb)
> -			if (plane->fb)
> -				intel_unpin_fb_obj(old_obj);
> -
> -		mutex_unlock(&dev->struct_mutex);
> -
>  	} else {
>  		if (intel_crtc && intel_crtc->active &&
>  		    intel_crtc->primary_enabled) {
> @@ -11903,12 +11919,34 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  				intel_disable_fbc(dev);
>  			}
>  		}
> -		ret = intel_pipe_set_base(crtc, src->x1, src->y1, fb);
> -		if (ret)
> -			return ret;
>  
> -		if (!intel_crtc->primary_enabled)
> -			intel_enable_primary_hw_plane(plane, crtc);
> +		intel_update_pipe_size(crtc);

I was going to say we probably don't want to call this here, but we do
in fact call it already so this doesn't change the behaviour. Which is
good since I don't want to think about this fastboot hack at all ;)

Maybe slap a comment on top of it. Eg.:
/* FIXME kill this fastboot hack */

> +
> +		intel_crtc->primary_enabled = true;
> +
> +		dev_priv->display.update_primary_plane(crtc, fb, src->x1, src->y1);
> +
> +		mutex_lock(&dev->struct_mutex);
> +		intel_update_fbc(dev);
> +		mutex_unlock(&dev->struct_mutex);

intel_update_fbc() must be called only after primary->fb has been
updated. Just after the intel_frontbuffer_flip() call is probably
the best place for it.

Otherwise this patch is looking good I think.

> +	}
> +
> +	crtc->primary->fb = fb;
> +	crtc->x = src->x1;
> +	crtc->y = src->y1;
> +
> +	if (intel_crtc->active)
> +		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
> +
> +	if (old_fb) {
> +		if (intel_crtc->active && old_fb != fb)
> +			intel_wait_for_vblank(dev, intel_crtc->pipe);
> +
> +		if (old_fb != fb) {
> +			mutex_lock(&dev->struct_mutex);
> +			intel_unpin_fb_obj(old_obj);
> +			mutex_unlock(&dev->struct_mutex);
> +		}
>  	}
>  	intel_plane->crtc_x = state->orig_dst.x1;
> -- 
> 1.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Re: [RFC 2/3] drm/i915: create intel_update_pipe_size()

[Gustavo Padovan]

2014-09-09 Ville Syrjälä <ville.syrjala@linux.intel.com>:

> On Tue, Sep 09, 2014 at 11:43:20AM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Factor out a piece of code from intel_pipe_set_base() that updates
> > the pipe size and adjust fitter.
> > 
> > This will help refactor the update primary plane path.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 71 +++++++++++++++++++++---------------
> >  1 file changed, 41 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 2ccf7c0..e7e7184 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2779,6 +2779,46 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> >  	return pending;
> >  }
> >  
> > +static void intel_update_pipe_size(struct drm_crtc *crtc)
> 
> These days we usually prefer to pass intel_crtc instead of drm_crtc. You
> can still call it 'crtc' since that's shorter and because we don't need
> anything from drm_crtc in this function there won't be any confusion
> between the two.

Actually we need the drm_crtc 3 times in this function, that is why I left it
as an argument. We could just do the other way around and get it from
&intel_crtc->base.

	Gustavo

Re: [RFC 2/3] drm/i915: create intel_update_pipe_size()

[Daniel Vetter]

On Tue, Sep 09, 2014 at 02:43:14PM -0300, Gustavo Padovan wrote:
> 2014-09-09 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> 
> > On Tue, Sep 09, 2014 at 11:43:20AM -0300, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > 
> > > Factor out a piece of code from intel_pipe_set_base() that updates
> > > the pipe size and adjust fitter.
> > > 
> > > This will help refactor the update primary plane path.
> > > 
> > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 71 +++++++++++++++++++++---------------
> > >  1 file changed, 41 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 2ccf7c0..e7e7184 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2779,6 +2779,46 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> > >  	return pending;
> > >  }
> > >  
> > > +static void intel_update_pipe_size(struct drm_crtc *crtc)
> > 
> > These days we usually prefer to pass intel_crtc instead of drm_crtc. You
> > can still call it 'crtc' since that's shorter and because we don't need
> > anything from drm_crtc in this function there won't be any confusion
> > between the two.
> 
> Actually we need the drm_crtc 3 times in this function, that is why I left it
> as an argument. We could just do the other way around and get it from
> &intel_crtc->base.

Yeah I prefer we use the intel_foo types internally without an intel_
prefix in the local variable name. Adding the foo->base. prefix isn't
really much longer than just foo.

Imo upcasting should only be done in interface hooks where we have a
reason for the paramater to have the more generic type, everywhere else it
just looks a bit brittle. And yes I know that we're not there at all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [RFC 1/3] drm/i915: remove !enabled handling from commit primary plane step

[Daniel Vetter]

On Tue, Sep 09, 2014 at 06:58:47PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 09, 2014 at 11:43:19AM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > The !crtc->enabled case will now be handled by the !visible code,
> > since the handling is basically the same.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 26 --------------------------
> >  1 file changed, 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 5279b99..2ccf7c0 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11837,32 +11837,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
> >  	struct drm_rect *src = &state->src;
> >  	int ret;
> >  
> > -	/*
> > -	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
> > -	 * updating the fb pointer, and returning without touching the
> > -	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
> > -	 * turn on the display with all planes setup as desired.
> > -	 */
> > -	if (!crtc->enabled) {
> > -		mutex_lock(&dev->struct_mutex);
> > -
> > -		/*
> > -		 * If we already called setplane while the crtc was disabled,
> > -		 * we may have an fb pinned; unpin it.
> > -		 */
> > -		if (plane->fb)
> > -			intel_unpin_fb_obj(old_obj);
> > -
> > -		i915_gem_track_fb(old_obj, obj,
> > -				  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
> > -
> > -		/* Pin and return without programming hardware */
> > -		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> > -		mutex_unlock(&dev->struct_mutex);
> > -
> > -		return ret;
> > -	}
> > -
> >  	intel_crtc_wait_for_pending_flips(crtc);
> 
> Yeah this should work just fine.
> 
> One difference between the code paths is the intel_crtc_wait_for_pending_flips()
> call, but since the crtc isn't enabled there can't be any pending flip.
> 
> The other difference is the pin vs. unpin order, but that shouldn't matter
> unless there's tons of other stuff pinned as well. It's not worth optimizing
> setplane calls on disabled CRTCs too much IMO.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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