[PATCH v2 00/14] Kill off intel_crtc->atomic, rework.

[PATCH v2 00/14] Kill off intel_crtc->atomic, rework.

[Maarten Lankhorst]

After fixing up some earlier patches it turns out I had to rework
the rest and improved the individual patches. There were some changes
in upstream, which meant I had to redo most of them.

The order of patches is slightly changed, the update watermark comes before
killing off wait_vblank because it has a dependency on it.

Maarten Lankhorst (14):
  drm/i915: Use passed plane state for sprite planes, v3.
  drm/i915: Extend DSL readout fix to BDW and SKL.
  drm/i915: Do not acquire crtc state to check clock during modeset, v2.
  drm/i915: Handle cdclk limits on broadwell.
  drm/i915/bxt: Use the bypass frequency if there are no active pipes.
  drm/i915: Update watermark related members in the crtc_state, v2.
  drm/i915: Kill off intel_crtc->atomic.wait_vblank.
  drm/i915: Remove intel_crtc->atomic.disable_ips.
  drm/i915: Remove atomic.pre_disable_primary.
  drm/i915: Remove update_sprite_watermarks.
  drm/i915: Remove some post-commit members from intel_crtc->atomic.
  drm/i915: Nuke fbc members from intel_crtc->atomic.
  drm/i915/skl: Update watermarks before the crtc is disabled.
  drm/i915/skl: Do not allow scaling when crtc is disabled.

 drivers/gpu/drm/i915/i915_drv.h      |   5 +
 drivers/gpu/drm/i915/i915_irq.c      |   2 +-
 drivers/gpu/drm/i915/intel_atomic.c  |   6 +-
 drivers/gpu/drm/i915/intel_display.c | 459 ++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h     |  47 ++--
 drivers/gpu/drm/i915/intel_sprite.c  | 120 ++++-----
 6 files changed, 358 insertions(+), 281 deletions(-)

-- 
2.1.0

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

[PATCH v2 01/14] drm/i915: Use passed plane state for sprite planes, v3.

[Maarten Lankhorst]

Don't use plane->state directly, use the pointer from commit_plane.

Changes since v1:
- Fix uses of plane->state->rotation and color key to use the passed state too.
- Only pass crtc_state and plane_state to update_plane.
Changes since v2:
- Rebased.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h    |  10 +--
 drivers/gpu/drm/i915/intel_sprite.c | 120 +++++++++++++++++++-----------------
 2 files changed, 67 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d1a6071afabe..16d4627364c5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -647,16 +647,12 @@ struct intel_plane {
 	/*
 	 * NOTE: Do not place new plane state fields here (e.g., when adding
 	 * new plane properties).  New runtime state should now be placed in
-	 * the intel_plane_state structure and accessed via drm_plane->state.
+	 * the intel_plane_state structure and accessed via plane_state.
 	 */
 
 	void (*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 x, uint32_t y,
-			     uint32_t src_w, uint32_t src_h);
+			     struct intel_crtc_state *crtc_state,
+			     struct intel_plane_state *plane_state);
 	void (*disable_plane)(struct drm_plane *plane,
 			      struct drm_crtc *crtc);
 	int (*check_plane)(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 4276c135b9f2..6d3047f9c80f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -178,28 +178,32 @@ void intel_pipe_update_end(struct intel_crtc *crtc)
 }
 
 static void
-skl_update_plane(struct drm_plane *drm_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 x, uint32_t y,
-		 uint32_t src_w, uint32_t src_h)
+skl_update_plane(struct drm_plane *drm_plane,
+		 struct intel_crtc_state *crtc_state,
+		 struct intel_plane_state *plane_state)
 {
 	struct drm_device *dev = drm_plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
+	struct drm_framebuffer *fb = plane_state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
 	u32 plane_ctl, stride_div, stride;
-	const struct drm_intel_sprite_colorkey *key =
-		&to_intel_plane_state(drm_plane->state)->ckey;
+	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
 	unsigned long surf_addr;
 	u32 tile_height, plane_offset, plane_size;
 	unsigned int rotation;
 	int x_offset, y_offset;
-	struct intel_crtc_state *crtc_state = to_intel_crtc(crtc)->config;
-	int scaler_id;
+
+	int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
+	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
+	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
+	uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
+	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
+	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
+	const struct intel_scaler *scaler =
+		&crtc_state->scaler_state.scalers[plane_state->scaler_id];
 
 	plane_ctl = PLANE_CTL_ENABLE |
 		PLANE_CTL_PIPE_GAMMA_ENABLE |
@@ -208,14 +212,12 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
 	plane_ctl |= skl_plane_ctl_tiling(fb->modifier[0]);
 
-	rotation = drm_plane->state->rotation;
+	rotation = plane_state->base.rotation;
 	plane_ctl |= skl_plane_ctl_rotation(rotation);
 
 	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
 					       fb->pixel_format);
 
-	scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id;
-
 	/* Sizes are 0 based */
 	src_w--;
 	src_h--;
@@ -256,13 +258,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
 
 	/* program plane scaler */
-	if (scaler_id >= 0) {
+	if (plane_state->scaler_id >= 0) {
 		uint32_t ps_ctrl = 0;
+		int scaler_id = plane_state->scaler_id;
 
 		DRM_DEBUG_KMS("plane = %d PS_PLANE_SEL(plane) = 0x%x\n", plane,
 			PS_PLANE_SEL(plane));
-		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(plane) |
-			crtc_state->scaler_state.scalers[scaler_id].mode;
+		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(plane) | scaler->mode;
 		I915_WRITE(SKL_PS_CTRL(pipe, scaler_id), ps_ctrl);
 		I915_WRITE(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
 		I915_WRITE(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
@@ -334,24 +336,28 @@ chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
 }
 
 static void
-vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
-		 struct drm_framebuffer *fb,
-		 int crtc_x, int crtc_y,
-		 unsigned int crtc_w, unsigned int crtc_h,
-		 uint32_t x, uint32_t y,
-		 uint32_t src_w, uint32_t src_h)
+vlv_update_plane(struct drm_plane *dplane,
+		 struct intel_crtc_state *crtc_state,
+		 struct intel_plane_state *plane_state)
 {
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct drm_framebuffer *fb = plane_state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
 	u32 sprctl;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
-	const struct drm_intel_sprite_colorkey *key =
-		&to_intel_plane_state(dplane->state)->ckey;
+	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
+
+	int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
+	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
+	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
+	uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
+	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
+	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
 
 	sprctl = SP_ENABLE;
 
@@ -421,7 +427,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 							fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
-	if (dplane->state->rotation == BIT(DRM_ROTATE_180)) {
+	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
 		sprctl |= SP_ROTATE_180;
 
 		x += src_w;
@@ -474,23 +480,27 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 }
 
 static void
-ivb_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 x, uint32_t y,
-		 uint32_t src_w, uint32_t src_h)
+ivb_update_plane(struct drm_plane *plane,
+		 struct intel_crtc_state *crtc_state,
+		 struct intel_plane_state *plane_state)
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_framebuffer *fb = plane_state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	enum pipe pipe = intel_plane->pipe;
 	u32 sprctl, sprscale = 0;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
-	const struct drm_intel_sprite_colorkey *key =
-		&to_intel_plane_state(plane->state)->ckey;
+	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
+
+	int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
+	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
+	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
+	uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
+	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
+	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
 
 	sprctl = SPRITE_ENABLE;
 
@@ -550,7 +560,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
-	if (plane->state->rotation == BIT(DRM_ROTATE_180)) {
+	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
 		sprctl |= SPRITE_ROTATE_180;
 
 		/* HSW and BDW does this automagically in hardware */
@@ -612,23 +622,27 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 }
 
 static void
-ilk_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 x, uint32_t y,
-		 uint32_t src_w, uint32_t src_h)
+ilk_update_plane(struct drm_plane *plane,
+		 struct intel_crtc_state *crtc_state,
+		 struct intel_plane_state *plane_state)
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_framebuffer *fb = plane_state->base.fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	int pipe = intel_plane->pipe;
 	unsigned long dvssurf_offset, linear_offset;
 	u32 dvscntr, dvsscale;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
-	const struct drm_intel_sprite_colorkey *key =
-		&to_intel_plane_state(plane->state)->ckey;
+	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
+
+	int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
+	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
+	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
+	uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
+	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
+	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
 
 	dvscntr = DVS_ENABLE;
 
@@ -684,7 +698,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= dvssurf_offset;
 
-	if (plane->state->rotation == BIT(DRM_ROTATE_180)) {
+	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
 		dvscntr |= DVS_ROTATE_180;
 
 		x += src_w;
@@ -917,23 +931,17 @@ static void
 intel_commit_sprite_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
-	struct drm_crtc *crtc = state->base.crtc;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
-	struct drm_framebuffer *fb = state->base.fb;
-
-	crtc = crtc ? crtc : plane->crtc;
 
 	if (state->visible) {
-		intel_plane->update_plane(plane, crtc, fb,
-					  state->dst.x1, state->dst.y1,
-					  drm_rect_width(&state->dst),
-					  drm_rect_height(&state->dst),
-					  state->src.x1 >> 16,
-					  state->src.y1 >> 16,
-					  drm_rect_width(&state->src) >> 16,
-					  drm_rect_height(&state->src) >> 16);
+		struct intel_crtc_state *crtc_state =
+			to_intel_crtc(state->base.crtc)->config;
+
+		intel_plane->update_plane(plane, crtc_state, state);
 	} else {
-		intel_plane->disable_plane(plane, crtc);
+		struct drm_crtc *crtc = state->base.crtc;
+
+		intel_plane->disable_plane(plane, crtc ?: plane->crtc);
 	}
 }
 
-- 
2.1.0

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

Re: [PATCH v2 01/14] drm/i915: Use passed plane state for sprite planes, v3.

[Ville Syrjälä]

On Tue, Nov 03, 2015 at 08:31:40AM +0100, Maarten Lankhorst wrote:
> Don't use plane->state directly, use the pointer from commit_plane.
> 
> Changes since v1:
> - Fix uses of plane->state->rotation and color key to use the passed state too.
> - Only pass crtc_state and plane_state to update_plane.
> Changes since v2:
> - Rebased.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h    |  10 +--
>  drivers/gpu/drm/i915/intel_sprite.c | 120 +++++++++++++++++++-----------------
>  2 files changed, 67 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d1a6071afabe..16d4627364c5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -647,16 +647,12 @@ struct intel_plane {
>  	/*
>  	 * NOTE: Do not place new plane state fields here (e.g., when adding
>  	 * new plane properties).  New runtime state should now be placed in
> -	 * the intel_plane_state structure and accessed via drm_plane->state.
> +	 * the intel_plane_state structure and accessed via plane_state.
>  	 */
>  
>  	void (*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 x, uint32_t y,
> -			     uint32_t src_w, uint32_t src_h);
> +			     struct intel_crtc_state *crtc_state,
> +			     struct intel_plane_state *plane_state);

These should be const.

>  	void (*disable_plane)(struct drm_plane *plane,
>  			      struct drm_crtc *crtc);
>  	int (*check_plane)(struct drm_plane *plane,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 4276c135b9f2..6d3047f9c80f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -178,28 +178,32 @@ void intel_pipe_update_end(struct intel_crtc *crtc)
>  }
>  
>  static void
> -skl_update_plane(struct drm_plane *drm_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 x, uint32_t y,
> -		 uint32_t src_w, uint32_t src_h)
> +skl_update_plane(struct drm_plane *drm_plane,
> +		 struct intel_crtc_state *crtc_state,
> +		 struct intel_plane_state *plane_state)
>  {
>  	struct drm_device *dev = drm_plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
> +	struct drm_framebuffer *fb = plane_state->base.fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
>  	u32 plane_ctl, stride_div, stride;
> -	const struct drm_intel_sprite_colorkey *key =
> -		&to_intel_plane_state(drm_plane->state)->ckey;
> +	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>  	unsigned long surf_addr;
>  	u32 tile_height, plane_offset, plane_size;
>  	unsigned int rotation;
>  	int x_offset, y_offset;
> -	struct intel_crtc_state *crtc_state = to_intel_crtc(crtc)->config;
> -	int scaler_id;
> +

What's with the extra newline?

> +	int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
> +	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
> +	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
> +	uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
> +	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
> +	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
> +	const struct intel_scaler *scaler =
> +		&crtc_state->scaler_state.scalers[plane_state->scaler_id];

This looks messy. I would at least get rid of the "assign two things on
one line thing".

>  
>  	plane_ctl = PLANE_CTL_ENABLE |
>  		PLANE_CTL_PIPE_GAMMA_ENABLE |
> @@ -208,14 +212,12 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
>  	plane_ctl |= skl_plane_ctl_tiling(fb->modifier[0]);
>  
> -	rotation = drm_plane->state->rotation;
> +	rotation = plane_state->base.rotation;
>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
>  	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
>  					       fb->pixel_format);
>  
> -	scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id;
> -
>  	/* Sizes are 0 based */
>  	src_w--;
>  	src_h--;
> @@ -256,13 +258,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
>  
>  	/* program plane scaler */
> -	if (scaler_id >= 0) {
> +	if (plane_state->scaler_id >= 0) {
>  		uint32_t ps_ctrl = 0;
> +		int scaler_id = plane_state->scaler_id;
>  
>  		DRM_DEBUG_KMS("plane = %d PS_PLANE_SEL(plane) = 0x%x\n", plane,
>  			PS_PLANE_SEL(plane));
> -		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(plane) |
> -			crtc_state->scaler_state.scalers[scaler_id].mode;
> +		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(plane) | scaler->mode;
>  		I915_WRITE(SKL_PS_CTRL(pipe, scaler_id), ps_ctrl);
>  		I915_WRITE(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
>  		I915_WRITE(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
> @@ -334,24 +336,28 @@ chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
>  }
>  
>  static void
> -vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> -		 struct drm_framebuffer *fb,
> -		 int crtc_x, int crtc_y,
> -		 unsigned int crtc_w, unsigned int crtc_h,
> -		 uint32_t x, uint32_t y,
> -		 uint32_t src_w, uint32_t src_h)
> +vlv_update_plane(struct drm_plane *dplane,
> +		 struct intel_crtc_state *crtc_state,
> +		 struct intel_plane_state *plane_state)
>  {
>  	struct drm_device *dev = dplane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> +	struct drm_framebuffer *fb = plane_state->base.fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	int pipe = intel_plane->pipe;
>  	int plane = intel_plane->plane;
>  	u32 sprctl;
>  	unsigned long sprsurf_offset, linear_offset;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> -	const struct drm_intel_sprite_colorkey *key =
> -		&to_intel_plane_state(dplane->state)->ckey;
> +	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> +
> +	int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
> +	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
> +	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
> +	uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
> +	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
> +	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
>  
>  	sprctl = SP_ENABLE;
>  
> @@ -421,7 +427,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  							fb->pitches[0]);
>  	linear_offset -= sprsurf_offset;
>  
> -	if (dplane->state->rotation == BIT(DRM_ROTATE_180)) {
> +	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
>  		sprctl |= SP_ROTATE_180;
>  
>  		x += src_w;
> @@ -474,23 +480,27 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  }
>  
>  static void
> -ivb_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 x, uint32_t y,
> -		 uint32_t src_w, uint32_t src_h)
> +ivb_update_plane(struct drm_plane *plane,
> +		 struct intel_crtc_state *crtc_state,
> +		 struct intel_plane_state *plane_state)
>  {
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct drm_framebuffer *fb = plane_state->base.fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	enum pipe pipe = intel_plane->pipe;
>  	u32 sprctl, sprscale = 0;
>  	unsigned long sprsurf_offset, linear_offset;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> -	const struct drm_intel_sprite_colorkey *key =
> -		&to_intel_plane_state(plane->state)->ckey;
> +	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> +
> +	int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
> +	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
> +	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
> +	uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
> +	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
> +	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
>  
>  	sprctl = SPRITE_ENABLE;
>  
> @@ -550,7 +560,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  					       pixel_size, fb->pitches[0]);
>  	linear_offset -= sprsurf_offset;
>  
> -	if (plane->state->rotation == BIT(DRM_ROTATE_180)) {
> +	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
>  		sprctl |= SPRITE_ROTATE_180;
>  
>  		/* HSW and BDW does this automagically in hardware */
> @@ -612,23 +622,27 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  }
>  
>  static void
> -ilk_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 x, uint32_t y,
> -		 uint32_t src_w, uint32_t src_h)
> +ilk_update_plane(struct drm_plane *plane,
> +		 struct intel_crtc_state *crtc_state,
> +		 struct intel_plane_state *plane_state)
>  {
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct drm_framebuffer *fb = plane_state->base.fb;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	int pipe = intel_plane->pipe;
>  	unsigned long dvssurf_offset, linear_offset;
>  	u32 dvscntr, dvsscale;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> -	const struct drm_intel_sprite_colorkey *key =
> -		&to_intel_plane_state(plane->state)->ckey;
> +	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> +
> +	int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
> +	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
> +	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
> +	uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
> +	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
> +	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
>  
>  	dvscntr = DVS_ENABLE;
>  
> @@ -684,7 +698,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  					       pixel_size, fb->pitches[0]);
>  	linear_offset -= dvssurf_offset;
>  
> -	if (plane->state->rotation == BIT(DRM_ROTATE_180)) {
> +	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
>  		dvscntr |= DVS_ROTATE_180;
>  
>  		x += src_w;
> @@ -917,23 +931,17 @@ static void
>  intel_commit_sprite_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)

I wonder why that isn't const...

>  {
> -	struct drm_crtc *crtc = state->base.crtc;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	struct drm_framebuffer *fb = state->base.fb;
> -
> -	crtc = crtc ? crtc : plane->crtc;
>  
>  	if (state->visible) {
> -		intel_plane->update_plane(plane, crtc, fb,
> -					  state->dst.x1, state->dst.y1,
> -					  drm_rect_width(&state->dst),
> -					  drm_rect_height(&state->dst),
> -					  state->src.x1 >> 16,
> -					  state->src.y1 >> 16,
> -					  drm_rect_width(&state->src) >> 16,
> -					  drm_rect_height(&state->src) >> 16);
> +		struct intel_crtc_state *crtc_state =
> +			to_intel_crtc(state->base.crtc)->config;
> +
> +		intel_plane->update_plane(plane, crtc_state, state);
>  	} else {
> -		intel_plane->disable_plane(plane, crtc);
> +		struct drm_crtc *crtc = state->base.crtc;
> +
> +		intel_plane->disable_plane(plane, crtc ?: plane->crtc);
>  	}
>  }
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 01/14] drm/i915: Use passed plane state for sprite planes, v3.

[Maarten Lankhorst]

Op 03-11-15 om 10:22 schreef Ville Syrjälä:
> On Tue, Nov 03, 2015 at 08:31:40AM +0100, Maarten Lankhorst wrote:
>> Don't use plane->state directly, use the pointer from commit_plane.
>>
>> Changes since v1:
>> - Fix uses of plane->state->rotation and color key to use the passed state too.
>> - Only pass crtc_state and plane_state to update_plane.
>> Changes since v2:
>> - Rebased.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_drv.h    |  10 +--
>>  drivers/gpu/drm/i915/intel_sprite.c | 120 +++++++++++++++++++-----------------
>>  2 files changed, 67 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index d1a6071afabe..16d4627364c5 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -647,16 +647,12 @@ struct intel_plane {
>>  	/*
>>  	 * NOTE: Do not place new plane state fields here (e.g., when adding
>>  	 * new plane properties).  New runtime state should now be placed in
>> -	 * the intel_plane_state structure and accessed via drm_plane->state.
>> +	 * the intel_plane_state structure and accessed via plane_state.
>>  	 */
>>  
>>  	void (*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 x, uint32_t y,
>> -			     uint32_t src_w, uint32_t src_h);
>> +			     struct intel_crtc_state *crtc_state,
>> +			     struct intel_plane_state *plane_state);
> These should be const.
>
>>  	void (*disable_plane)(struct drm_plane *plane,
>>  			      struct drm_crtc *crtc);
>>  	int (*check_plane)(struct drm_plane *plane,
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index 4276c135b9f2..6d3047f9c80f 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -178,28 +178,32 @@ void intel_pipe_update_end(struct intel_crtc *crtc)
>>  }
>>  
>>  static void
>> -skl_update_plane(struct drm_plane *drm_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 x, uint32_t y,
>> -		 uint32_t src_w, uint32_t src_h)
>> +skl_update_plane(struct drm_plane *drm_plane,
>> +		 struct intel_crtc_state *crtc_state,
>> +		 struct intel_plane_state *plane_state)
>>  {
>>  	struct drm_device *dev = drm_plane->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
>> +	struct drm_framebuffer *fb = plane_state->base.fb;
>>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>>  	const int pipe = intel_plane->pipe;
>>  	const int plane = intel_plane->plane + 1;
>>  	u32 plane_ctl, stride_div, stride;
>> -	const struct drm_intel_sprite_colorkey *key =
>> -		&to_intel_plane_state(drm_plane->state)->ckey;
>> +	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>>  	unsigned long surf_addr;
>>  	u32 tile_height, plane_offset, plane_size;
>>  	unsigned int rotation;
>>  	int x_offset, y_offset;
>> -	struct intel_crtc_state *crtc_state = to_intel_crtc(crtc)->config;
>> -	int scaler_id;
>> +
> What's with the extra newline?
>
>> +	int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
>> +	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
>> +	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
>> +	uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
>> +	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
>> +	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
>> +	const struct intel_scaler *scaler =
>> +		&crtc_state->scaler_state.scalers[plane_state->scaler_id];
> This looks messy. I would at least get rid of the "assign two things on
> one line thing".
>
>>  
>>  	plane_ctl = PLANE_CTL_ENABLE |
>>  		PLANE_CTL_PIPE_GAMMA_ENABLE |
>> @@ -208,14 +212,12 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>>  	plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
>>  	plane_ctl |= skl_plane_ctl_tiling(fb->modifier[0]);
>>  
>> -	rotation = drm_plane->state->rotation;
>> +	rotation = plane_state->base.rotation;
>>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>>  
>>  	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
>>  					       fb->pixel_format);
>>  
>> -	scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id;
>> -
>>  	/* Sizes are 0 based */
>>  	src_w--;
>>  	src_h--;
>> @@ -256,13 +258,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>>  	I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
>>  
>>  	/* program plane scaler */
>> -	if (scaler_id >= 0) {
>> +	if (plane_state->scaler_id >= 0) {
>>  		uint32_t ps_ctrl = 0;
>> +		int scaler_id = plane_state->scaler_id;
>>  
>>  		DRM_DEBUG_KMS("plane = %d PS_PLANE_SEL(plane) = 0x%x\n", plane,
>>  			PS_PLANE_SEL(plane));
>> -		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(plane) |
>> -			crtc_state->scaler_state.scalers[scaler_id].mode;
>> +		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(plane) | scaler->mode;
>>  		I915_WRITE(SKL_PS_CTRL(pipe, scaler_id), ps_ctrl);
>>  		I915_WRITE(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
>>  		I915_WRITE(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
>> @@ -334,24 +336,28 @@ chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
>>  }
>>  
>>  static void
>> -vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>> -		 struct drm_framebuffer *fb,
>> -		 int crtc_x, int crtc_y,
>> -		 unsigned int crtc_w, unsigned int crtc_h,
>> -		 uint32_t x, uint32_t y,
>> -		 uint32_t src_w, uint32_t src_h)
>> +vlv_update_plane(struct drm_plane *dplane,
>> +		 struct intel_crtc_state *crtc_state,
>> +		 struct intel_plane_state *plane_state)
>>  {
>>  	struct drm_device *dev = dplane->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
>> +	struct drm_framebuffer *fb = plane_state->base.fb;
>>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>>  	int pipe = intel_plane->pipe;
>>  	int plane = intel_plane->plane;
>>  	u32 sprctl;
>>  	unsigned long sprsurf_offset, linear_offset;
>>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>> -	const struct drm_intel_sprite_colorkey *key =
>> -		&to_intel_plane_state(dplane->state)->ckey;
>> +	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>> +
>> +	int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
>> +	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
>> +	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
>> +	uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
>> +	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
>> +	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
>>  
>>  	sprctl = SP_ENABLE;
>>  
>> @@ -421,7 +427,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>>  							fb->pitches[0]);
>>  	linear_offset -= sprsurf_offset;
>>  
>> -	if (dplane->state->rotation == BIT(DRM_ROTATE_180)) {
>> +	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
>>  		sprctl |= SP_ROTATE_180;
>>  
>>  		x += src_w;
>> @@ -474,23 +480,27 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>>  }
>>  
>>  static void
>> -ivb_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 x, uint32_t y,
>> -		 uint32_t src_w, uint32_t src_h)
>> +ivb_update_plane(struct drm_plane *plane,
>> +		 struct intel_crtc_state *crtc_state,
>> +		 struct intel_plane_state *plane_state)
>>  {
>>  	struct drm_device *dev = plane->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>> +	struct drm_framebuffer *fb = plane_state->base.fb;
>>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>>  	enum pipe pipe = intel_plane->pipe;
>>  	u32 sprctl, sprscale = 0;
>>  	unsigned long sprsurf_offset, linear_offset;
>>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>> -	const struct drm_intel_sprite_colorkey *key =
>> -		&to_intel_plane_state(plane->state)->ckey;
>> +	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>> +
>> +	int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
>> +	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
>> +	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
>> +	uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
>> +	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
>> +	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
>>  
>>  	sprctl = SPRITE_ENABLE;
>>  
>> @@ -550,7 +560,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>>  					       pixel_size, fb->pitches[0]);
>>  	linear_offset -= sprsurf_offset;
>>  
>> -	if (plane->state->rotation == BIT(DRM_ROTATE_180)) {
>> +	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
>>  		sprctl |= SPRITE_ROTATE_180;
>>  
>>  		/* HSW and BDW does this automagically in hardware */
>> @@ -612,23 +622,27 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>>  }
>>  
>>  static void
>> -ilk_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 x, uint32_t y,
>> -		 uint32_t src_w, uint32_t src_h)
>> +ilk_update_plane(struct drm_plane *plane,
>> +		 struct intel_crtc_state *crtc_state,
>> +		 struct intel_plane_state *plane_state)
>>  {
>>  	struct drm_device *dev = plane->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>> +	struct drm_framebuffer *fb = plane_state->base.fb;
>>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>>  	int pipe = intel_plane->pipe;
>>  	unsigned long dvssurf_offset, linear_offset;
>>  	u32 dvscntr, dvsscale;
>>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>> -	const struct drm_intel_sprite_colorkey *key =
>> -		&to_intel_plane_state(plane->state)->ckey;
>> +	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>> +
>> +	int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
>> +	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
>> +	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
>> +	uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
>> +	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
>> +	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
>>  
>>  	dvscntr = DVS_ENABLE;
>>  
>> @@ -684,7 +698,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>>  					       pixel_size, fb->pitches[0]);
>>  	linear_offset -= dvssurf_offset;
>>  
>> -	if (plane->state->rotation == BIT(DRM_ROTATE_180)) {
>> +	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
>>  		dvscntr |= DVS_ROTATE_180;
>>  
>>  		x += src_w;
>> @@ -917,23 +931,17 @@ static void
>>  intel_commit_sprite_plane(struct drm_plane *plane,
>>  			  struct intel_plane_state *state)
> I wonder why that isn't const...
>
I'm guessing because until recently crtc_state wasn't really that const. :)

I agree we should constify it, but shouldn't that be in a followup patch for readability?

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

[PATCH v2 02/14] drm/i915: Extend DSL readout fix to BDW and SKL.

[Maarten Lankhorst]

Those platforms have the same bug as haswell, and the same fix applies to them.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91579
---
 drivers/gpu/drm/i915/i915_irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6e0a5683bbdc..825114af9c56 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -747,7 +747,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
 	 * problem.  We may need to extend this to include other platforms,
 	 * but so far testing only shows the problem on HSW.
 	 */
-	if (IS_HASWELL(dev) && !position) {
+	if (HAS_DDI(dev) && !position) {
 		int i, temp;
 
 		for (i = 0; i < 100; i++) {
-- 
2.1.0

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

Re: [PATCH v2 02/14] drm/i915: Extend DSL readout fix to BDW and SKL.

[Ville Syrjälä]

On Tue, Nov 03, 2015 at 08:31:41AM +0100, Maarten Lankhorst wrote:
> Those platforms have the same bug as haswell, and the same fix applies to them.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91579

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

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6e0a5683bbdc..825114af9c56 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -747,7 +747,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>  	 * problem.  We may need to extend this to include other platforms,
>  	 * but so far testing only shows the problem on HSW.
>  	 */
> -	if (IS_HASWELL(dev) && !position) {
> +	if (HAS_DDI(dev) && !position) {
>  		int i, temp;
>  
>  		for (i = 0; i < 100; i++) {
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 02/14] drm/i915: Extend DSL readout fix to BDW and SKL.

[Jani Nikula]

On Tue, 03 Nov 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Nov 03, 2015 at 08:31:41AM +0100, Maarten Lankhorst wrote:
>> Those platforms have the same bug as haswell, and the same fix applies to them.

How about Broxton? IS_DDI matches that.

Jani.

>> 
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91579
>
> Sigh. 
> Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 6e0a5683bbdc..825114af9c56 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -747,7 +747,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>>  	 * problem.  We may need to extend this to include other platforms,
>>  	 * but so far testing only shows the problem on HSW.
>>  	 */
>> -	if (IS_HASWELL(dev) && !position) {
>> +	if (HAS_DDI(dev) && !position) {
>>  		int i, temp;
>>  
>>  		for (i = 0; i < 100; i++) {
>> -- 
>> 2.1.0
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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 v2 02/14] drm/i915: Extend DSL readout fix to BDW and SKL.

[Maarten Lankhorst]

Hey,

Op 03-11-15 om 12:32 schreef Jani Nikula:
> On Tue, 03 Nov 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> On Tue, Nov 03, 2015 at 08:31:41AM +0100, Maarten Lankhorst wrote:
>>> Those platforms have the same bug as haswell, and the same fix applies to them.
> How about Broxton? IS_DDI matches that.
>
> Jani.
>
Judging from irc it's very likely it suffers from the same problem, but it would be nice if we had someone who could confirm. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 02/14] drm/i915: Extend DSL readout fix to BDW and SKL.

[Jesse Barnes]

On 11/03/2015 04:44 AM, Maarten Lankhorst wrote:
> Hey,
> 
> Op 03-11-15 om 12:32 schreef Jani Nikula:
>> On Tue, 03 Nov 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>>> On Tue, Nov 03, 2015 at 08:31:41AM +0100, Maarten Lankhorst wrote:
>>>> Those platforms have the same bug as haswell, and the same fix applies to them.
>> How about Broxton? IS_DDI matches that.
>>
>> Jani.
>>
> Judging from irc it's very likely it suffers from the same problem, but it would be nice if we had someone who could confirm. :)

It won't hurt (much) if we apply this workaround and it doesn't affect
BXT, so I think we may as well apply given what we know of BXT's lineage.

Jesse

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

Re: [PATCH v2 02/14] drm/i915: Extend DSL readout fix to BDW and SKL.

[Jani Nikula]

On Thu, 05 Nov 2015, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On 11/03/2015 04:44 AM, Maarten Lankhorst wrote:
>> Hey,
>> 
>> Op 03-11-15 om 12:32 schreef Jani Nikula:
>>> On Tue, 03 Nov 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>>>> On Tue, Nov 03, 2015 at 08:31:41AM +0100, Maarten Lankhorst wrote:
>>>>> Those platforms have the same bug as haswell, and the same fix applies to them.
>>> How about Broxton? IS_DDI matches that.
>>>
>>> Jani.
>>>
>> Judging from irc it's very likely it suffers from the same problem, but it would be nice if we had someone who could confirm. :)
>
> It won't hurt (much) if we apply this workaround and it doesn't affect
> BXT, so I think we may as well apply given what we know of BXT's lineage.

Pushed this one patch to drm-intel-next-fixes, with cc: stable v4.3.

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

[PATCH v2 03/14] drm/i915: Do not acquire crtc state to check clock during modeset, v2.

[Maarten Lankhorst]

Parallel modesets are still not allowed, but this will allow updating
a different crtc during a modeset if the clock is not changed.

Additionally when all pipes are DPMS off the cdclk will be lowered
to the minimum allowed.

Changes since v1:
- Add dev_priv->active_crtcs for tracking which crtcs are active.
- Rename min_cdclk to min_pixclk and move to dev_priv.
- Add a active_crtcs mask which is updated atomically.
- Add intel_atomic_state->modeset which is set on modesets.
- Commit new pixclk/active_crtcs right after state swap.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   5 ++
 drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
 drivers/gpu/drm/i915/intel_display.c | 160 +++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |   7 +-
 4 files changed, 125 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 20cd6d8bb083..4a1afcfe904e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1842,8 +1842,13 @@ struct drm_i915_private {
 	struct intel_pipe_crc pipe_crc[I915_MAX_PIPES];
 #endif
 
+	/* dpll and cdclk state is protected by connection_mutex */
 	int num_shared_dpll;
 	struct intel_shared_dpll shared_dplls[I915_NUM_PLLS];
+
+	unsigned int active_crtcs;
+	unsigned int min_pixclk[I915_MAX_PIPES];
+
 	int dpio_phy_iosf_port[I915_NUM_PHYS_VLV];
 
 	struct i915_workarounds workarounds;
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 643f342de33b..c4eadbc928b7 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -306,5 +306,5 @@ void intel_atomic_state_clear(struct drm_atomic_state *s)
 {
 	struct intel_atomic_state *state = to_intel_atomic_state(s);
 	drm_atomic_state_default_clear(&state->base);
-	state->dpll_set = false;
+	state->dpll_set = state->modeset = false;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7b3cfb64e7f6..68f1e0f9d50d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5229,6 +5229,7 @@ static void modeset_put_power_domains(struct drm_i915_private *dev_priv,
 
 static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
 {
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long put_domains[I915_MAX_PIPES] = {};
@@ -5237,13 +5238,18 @@ static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
 	int i;
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (needs_modeset(crtc->state))
-			put_domains[to_intel_crtc(crtc)->pipe] =
-				modeset_get_crtc_power_domains(crtc);
+		struct intel_crtc *intel_crtc =
+			to_intel_crtc(crtc);
+		enum pipe pipe = intel_crtc->pipe;
+
+		if (!needs_modeset(crtc->state))
+			continue;
+
+		put_domains[pipe] = modeset_get_crtc_power_domains(crtc);
 	}
 
 	if (dev_priv->display.modeset_commit_cdclk) {
-		unsigned int cdclk = to_intel_atomic_state(state)->cdclk;
+		unsigned int cdclk = intel_state->cdclk;
 
 		if (cdclk != dev_priv->cdclk_freq &&
 		    !WARN_ON(!state->allow_modeset))
@@ -5930,23 +5936,32 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
 static int intel_mode_max_pixclk(struct drm_device *dev,
 				 struct drm_atomic_state *state)
 {
-	struct intel_crtc *intel_crtc;
-	struct intel_crtc_state *crtc_state;
-	int max_pixclk = 0;
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	unsigned max_pixel_rate = 0, i;
+	enum pipe pipe;
 
-	for_each_intel_crtc(dev, intel_crtc) {
-		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
+	memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
+	       sizeof(intel_state->min_pixclk));
 
-		if (!crtc_state->base.enable)
-			continue;
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		int pixclk = 0;
+
+		if (crtc_state->enable)
+			pixclk = crtc_state->adjusted_mode.crtc_clock;
 
-		max_pixclk = max(max_pixclk,
-				 crtc_state->base.adjusted_mode.crtc_clock);
+		intel_state->min_pixclk[i] = pixclk;
 	}
 
-	return max_pixclk;
+	if (!intel_state->active_crtcs)
+		return 0;
+
+	for_each_pipe(dev_priv, pipe)
+		max_pixel_rate = max(intel_state->min_pixclk[pipe], max_pixel_rate);
+
+	return max_pixel_rate;
 }
 
 static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
@@ -6247,6 +6262,9 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
 	for_each_power_domain(domain, domains)
 		intel_display_power_put(dev_priv, domain);
 	intel_crtc->enabled_power_domains = 0;
+
+	dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe);
+	dev_priv->min_pixclk[intel_crtc->pipe] = 0;
 }
 
 /*
@@ -9482,29 +9500,39 @@ static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state)
 /* compute the max rate for new configuration */
 static int ilk_max_pixel_rate(struct drm_atomic_state *state)
 {
-	struct intel_crtc *intel_crtc;
-	struct intel_crtc_state *crtc_state;
-	int max_pixel_rate = 0;
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct drm_i915_private *dev_priv = state->dev->dev_private;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	unsigned max_pixel_rate = 0, i;
+	enum pipe pipe;
 
-	for_each_intel_crtc(state->dev, intel_crtc) {
-		int pixel_rate;
+	memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
+	       sizeof(intel_state->min_pixclk));
 
-		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		int pixclk = 0;
 
-		if (!crtc_state->base.enable)
-			continue;
+		if (crtc_state->enable) {
+			struct intel_crtc_state *intel_crtc_state =
+				to_intel_crtc_state(crtc_state);
 
-		pixel_rate = ilk_pipe_pixel_rate(crtc_state);
+			pixclk = ilk_pipe_pixel_rate(intel_crtc_state);
 
-		/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
-		if (IS_BROADWELL(state->dev) && crtc_state->ips_enabled)
-			pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
+			/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
+			if (IS_BROADWELL(dev_priv) && intel_crtc_state->ips_enabled)
+				pixclk = DIV_ROUND_UP(pixclk * 100, 95);
+		}
 
-		max_pixel_rate = max(max_pixel_rate, pixel_rate);
+		intel_state->min_pixclk[i] = pixclk;
 	}
 
+	if (!intel_state->active_crtcs)
+		return 0;
+
+	for_each_pipe(dev_priv, pipe)
+		max_pixel_rate = max(intel_state->min_pixclk[pipe], max_pixel_rate);
+
 	return max_pixel_rate;
 }
 
@@ -12998,15 +13026,27 @@ static int intel_modeset_all_pipes(struct drm_atomic_state *state)
 
 static int intel_modeset_checks(struct drm_atomic_state *state)
 {
-	struct drm_device *dev = state->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int ret;
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct drm_i915_private *dev_priv = state->dev->dev_private;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	int ret = 0, i;
 
 	if (!check_digital_port_conflicts(state)) {
 		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
 		return -EINVAL;
 	}
 
+	intel_state->modeset = true;
+	intel_state->active_crtcs = dev_priv->active_crtcs;
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (crtc_state->active)
+			intel_state->active_crtcs |= 1 << i;
+		else
+			intel_state->active_crtcs &= ~(1 << i);
+	}
+
 	/*
 	 * See if the config requires any additional preparation, e.g.
 	 * to adjust global state with pipes off.  We need to do this
@@ -13030,7 +13070,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
 
 	intel_modeset_clear_plls(state);
 
-	if (IS_HASWELL(dev))
+	if (IS_HASWELL(dev_priv))
 		return haswell_mode_set_planes_workaround(state);
 
 	return 0;
@@ -13244,12 +13284,13 @@ static int intel_atomic_commit(struct drm_device *dev,
 			       struct drm_atomic_state *state,
 			       bool async)
 {
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc *crtc;
 	int ret = 0;
 	int i;
-	bool any_ms = false;
+	bool hw_check = intel_state->modeset;
 
 	ret = intel_atomic_prepare_commit(dev, state, async);
 	if (ret) {
@@ -13260,13 +13301,18 @@ static int intel_atomic_commit(struct drm_device *dev,
 	drm_atomic_helper_swap_state(dev, state);
 	dev_priv->wm.config = to_intel_atomic_state(state)->wm_config;
 
+	if (intel_state->modeset) {
+		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
+		       sizeof(intel_state->min_pixclk));
+		dev_priv->active_crtcs = intel_state->active_crtcs;
+	}
+
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
 		if (!needs_modeset(crtc->state))
 			continue;
 
-		any_ms = true;
 		intel_pre_plane_update(intel_crtc);
 
 		if (crtc_state->active) {
@@ -13281,7 +13327,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	 * update the the output configuration. */
 	intel_modeset_update_crtc_state(state);
 
-	if (any_ms) {
+	if (intel_state->modeset) {
 		intel_shared_dpll_commit(state);
 
 		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
@@ -13305,7 +13351,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 			put_domains = modeset_get_crtc_power_domains(crtc);
 
 			/* make sure intel_modeset_check_state runs */
-			any_ms = true;
+			hw_check = true;
 		}
 
 		if (!modeset)
@@ -13329,7 +13375,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	drm_atomic_helper_cleanup_planes(dev, state);
 	mutex_unlock(&dev->struct_mutex);
 
-	if (any_ms)
+	if (hw_check)
 		intel_modeset_check_state(dev, state);
 
 	drm_atomic_state_free(state);
@@ -15331,16 +15377,36 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 	struct intel_connector *connector;
 	int i;
 
+	dev_priv->active_crtcs = 0;
+
 	for_each_intel_crtc(dev, crtc) {
-		__drm_atomic_helper_crtc_destroy_state(&crtc->base, crtc->base.state);
-		memset(crtc->config, 0, sizeof(*crtc->config));
-		crtc->config->base.crtc = &crtc->base;
+		struct intel_crtc_state *crtc_state = crtc->config;
+		int pixclk = 0;
 
-		crtc->active = dev_priv->display.get_pipe_config(crtc,
-								 crtc->config);
+		__drm_atomic_helper_crtc_destroy_state(&crtc->base, &crtc_state->base);
+		memset(crtc_state, 0, sizeof(*crtc_state));
+		crtc_state->base.crtc = &crtc->base;
 
-		crtc->base.state->active = crtc->active;
-		crtc->base.enabled = crtc->active;
+		crtc_state->base.active = crtc_state->base.enable =
+			dev_priv->display.get_pipe_config(crtc, crtc_state);
+
+		crtc->base.enabled = crtc_state->base.enable;
+		crtc->active = crtc_state->base.active;
+
+		if (crtc_state->base.active) {
+			dev_priv->active_crtcs |= 1 << crtc->pipe;
+
+			if (IS_BROADWELL(dev_priv)) {
+				pixclk = ilk_pipe_pixel_rate(crtc_state);
+
+				/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
+				if (crtc_state->ips_enabled)
+					pixclk = DIV_ROUND_UP(pixclk * 100, 95);
+			} else if (IS_BROXTON(dev_priv) || IS_VALLEYVIEW(dev_priv))
+				pixclk = crtc_state->base.adjusted_mode.crtc_clock;
+		}
+
+		dev_priv->min_pixclk[crtc->pipe] = pixclk;
 
 		readout_plane_state(crtc);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 16d4627364c5..3b03074813e4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -248,7 +248,12 @@ struct intel_atomic_state {
 	struct drm_atomic_state base;
 
 	unsigned int cdclk;
-	bool dpll_set;
+
+	bool dpll_set, modeset;
+
+	unsigned int active_crtcs;
+	unsigned int min_pixclk[I915_MAX_PIPES];
+
 	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
 	struct intel_wm_config wm_config;
 };
-- 
2.1.0

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

Re: [PATCH v2 03/14] drm/i915: Do not acquire crtc state to check clock during modeset, v2.

[Maarten Lankhorst]

Op 03-11-15 om 08:31 schreef Maarten Lankhorst:
> Parallel modesets are still not allowed, but this will allow updating
> a different crtc during a modeset if the clock is not changed.
>
> Additionally when all pipes are DPMS off the cdclk will be lowered
> to the minimum allowed.
>
> Changes since v1:
> - Add dev_priv->active_crtcs for tracking which crtcs are active.
> - Rename min_cdclk to min_pixclk and move to dev_priv.
> - Add a active_crtcs mask which is updated atomically.
> - Add intel_atomic_state->modeset which is set on modesets.
> - Commit new pixclk/active_crtcs right after state swap.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   5 ++
>  drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
>  drivers/gpu/drm/i915/intel_display.c | 160 +++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_drv.h     |   7 +-
>  4 files changed, 125 insertions(+), 49 deletions(-)
>
As I mentioned in discussion of 14/14, patch 3 4 and 5 should be dropped from the series.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 04/14] drm/i915: Handle cdclk limits on broadwell.

[Maarten Lankhorst]

As the comment indicates this can only fail gracefully when
called from compute_config. Fortunately this is now what's happening,
so the fixme can be removed and the DRM_ERROR downgraded.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 68f1e0f9d50d..1fcb5a379e98 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9632,14 +9632,10 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
 	else
 		cdclk = 337500;
 
-	/*
-	 * FIXME move the cdclk caclulation to
-	 * compute_config() so we can fail gracegully.
-	 */
 	if (cdclk > dev_priv->max_cdclk_freq) {
-		DRM_ERROR("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
-			  cdclk, dev_priv->max_cdclk_freq);
-		cdclk = dev_priv->max_cdclk_freq;
+		DRM_INFO("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
+			 cdclk, dev_priv->max_cdclk_freq);
+		return -EINVAL;
 	}
 
 	to_intel_atomic_state(state)->cdclk = cdclk;
-- 
2.1.0

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

[PATCH v2 05/14] drm/i915/bxt: Use the bypass frequency if there are no active pipes.

[Maarten Lankhorst]

Now that pixel clock is set to 0 when there are no active pipes it's
easy to set the bypass frequency for this case.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1fcb5a379e98..54e4f04bb427 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5917,7 +5917,6 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
 	/*
 	 * FIXME:
 	 * - remove the guardband, it's not needed on BXT
-	 * - set 19.2MHz bypass frequency if there are no active pipes
 	 */
 	if (max_pixclk > 576000*9/10)
 		return 624000;
@@ -5927,8 +5926,10 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
 		return 384000;
 	else if (max_pixclk > 144000*9/10)
 		return 288000;
-	else
+	else if (max_pixclk)
 		return 144000;
+	else
+		return 19200;
 }
 
 /* Compute the max pixel clock for new configuration. Uses atomic state if
-- 
2.1.0

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

[PATCH v2 06/14] drm/i915: Update watermark related members in the crtc_state, v2.

[Maarten Lankhorst]

This removes another couple of hacks from intel_crtc->atomic, and
creates proper atomic state for it.

Changes since v1:
- Rebase on top of wm changes.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  |  2 ++
 drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_drv.h     |  6 +++---
 3 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index c4eadbc928b7..3e390db9d22b 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -95,6 +95,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 
 	crtc_state->update_pipe = false;
 	crtc_state->disable_lp_wm = false;
+	crtc_state->visible_changed = false;
+	crtc_state->wm_changed = false;
 
 	return &crtc_state->base;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 54e4f04bb427..356e3a9e1741 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4753,6 +4753,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 static void intel_post_plane_update(struct intel_crtc *crtc)
 {
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
+	struct intel_crtc_state *pipe_config =
+		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -4761,10 +4763,9 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 
 	intel_frontbuffer_flip(dev, atomic->fb_bits);
 
-	if (atomic->disable_cxsr)
-		crtc->wm.cxsr_allowed = true;
+	crtc->wm.cxsr_allowed = true;
 
-	if (crtc->atomic.update_wm_post)
+	if (pipe_config->wm_changed)
 		intel_update_watermarks(&crtc->base);
 
 	if (atomic->update_fbc)
@@ -4781,6 +4782,8 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
+	struct intel_crtc_state *pipe_config =
+		to_intel_crtc_state(crtc->base.state);
 
 	if (atomic->disable_fbc)
 		intel_fbc_disable_crtc(crtc);
@@ -4791,10 +4794,13 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 	if (atomic->pre_disable_primary)
 		intel_pre_disable_primary(&crtc->base);
 
-	if (atomic->disable_cxsr) {
+	if (pipe_config->visible_changed) {
 		crtc->wm.cxsr_allowed = false;
 		intel_set_memory_cxsr(dev_priv, false);
 	}
+
+	if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed)
+		intel_update_watermarks(&crtc->base);
 }
 
 static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
@@ -11617,6 +11623,7 @@ static bool needs_scaling(struct intel_plane_state *state)
 int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 				    struct drm_plane_state *plane_state)
 {
+	struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state);
 	struct drm_crtc *crtc = crtc_state->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_plane *plane = plane_state->plane;
@@ -11663,25 +11670,18 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 			 plane->base.id, was_visible, visible,
 			 turn_off, turn_on, mode_changed);
 
-	if (turn_on) {
-		intel_crtc->atomic.update_wm_pre = true;
-		/* must disable cxsr around plane enable/disable */
-		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
-			intel_crtc->atomic.disable_cxsr = true;
-			/* to potentially re-enable cxsr */
-			intel_crtc->atomic.wait_vblank = true;
-			intel_crtc->atomic.update_wm_post = true;
-		}
-	} else if (turn_off) {
-		intel_crtc->atomic.update_wm_post = true;
+	if (turn_on || turn_off) {
+		pipe_config->wm_changed = true;
+
 		/* must disable cxsr around plane enable/disable */
 		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
+			pipe_config->visible_changed = true;
 			if (is_crtc_enabled)
 				intel_crtc->atomic.wait_vblank = true;
-			intel_crtc->atomic.disable_cxsr = true;
 		}
-	} else if (intel_wm_need_update(plane, plane_state)) {
-		intel_crtc->atomic.update_wm_pre = true;
+	} else if ((was_visible || visible) &&
+		   intel_wm_need_update(plane, plane_state)) {
+		pipe_config->wm_changed = true;
 	}
 
 	if (visible || was_visible)
@@ -11826,7 +11826,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 	}
 
 	if (mode_changed && !crtc_state->active)
-		intel_crtc->atomic.update_wm_post = true;
+		pipe_config->wm_changed = true;
 
 	if (mode_changed && crtc_state->enable &&
 	    dev_priv->display.crtc_compute_clock &&
@@ -13735,9 +13735,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 		to_intel_crtc_state(old_crtc_state);
 	bool modeset = needs_modeset(crtc->state);
 
-	if (intel_crtc->atomic.update_wm_pre)
-		intel_update_watermarks(crtc);
-
 	/* Perform vblank evasion around commit operation */
 	intel_pipe_update_start(intel_crtc);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3b03074813e4..59367453d24b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -372,7 +372,9 @@ struct intel_crtc_state {
 #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync mode.flags */
 	unsigned long quirks;
 
-	bool update_pipe;
+	bool update_pipe; /* can a fast modeset be performed? */
+	bool visible_changed; /* plane visibility changed */
+	bool wm_changed; /* wm changed */
 
 	/* Pipe source size (ie. panel fitter input size)
 	 * All planes will be positioned inside this space,
@@ -535,9 +537,7 @@ struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
 	bool disable_fbc;
 	bool disable_ips;
-	bool disable_cxsr;
 	bool pre_disable_primary;
-	bool update_wm_pre, update_wm_post;
 
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
-- 
2.1.0

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

Re: [PATCH v2 06/14] drm/i915: Update watermark related members in the crtc_state, v2.

[Ander Conselvan De Oliveira]

On Tue, 2015-11-03 at 08:31 +0100, Maarten Lankhorst wrote:
> This removes another couple of hacks from intel_crtc->atomic, and
> creates proper atomic state for it.

Perhaps you could be more verbose about the hacks being removed.

> Changes since v1:
> - Rebase on top of wm changes.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |  2 ++
>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++------------------
> -
>  drivers/gpu/drm/i915/intel_drv.h     |  6 +++---
>  3 files changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index c4eadbc928b7..3e390db9d22b 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -95,6 +95,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  
>  	crtc_state->update_pipe = false;
>  	crtc_state->disable_lp_wm = false;
> +	crtc_state->visible_changed = false;
> +	crtc_state->wm_changed = false;
>  
>  	return &crtc_state->base;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 54e4f04bb427..356e3a9e1741 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4753,6 +4753,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
>  static void intel_post_plane_update(struct intel_crtc *crtc)
>  {
>  	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
> +	struct intel_crtc_state *pipe_config =
> +		to_intel_crtc_state(crtc->base.state);
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> @@ -4761,10 +4763,9 @@ static void intel_post_plane_update(struct intel_crtc
> *crtc)
>  
>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>  
> -	if (atomic->disable_cxsr)
> -		crtc->wm.cxsr_allowed = true;
> +	crtc->wm.cxsr_allowed = true;
>  
> -	if (crtc->atomic.update_wm_post)
> +	if (pipe_config->wm_changed)
>  		intel_update_watermarks(&crtc->base);
>  
>  	if (atomic->update_fbc)
> @@ -4781,6 +4782,8 @@ static void intel_pre_plane_update(struct intel_crtc
> *crtc)
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
> +	struct intel_crtc_state *pipe_config =
> +		to_intel_crtc_state(crtc->base.state);
>  
>  	if (atomic->disable_fbc)
>  		intel_fbc_disable_crtc(crtc);
> @@ -4791,10 +4794,13 @@ static void intel_pre_plane_update(struct intel_crtc
> *crtc)
>  	if (atomic->pre_disable_primary)
>  		intel_pre_disable_primary(&crtc->base);
>  
> -	if (atomic->disable_cxsr) {
> +	if (pipe_config->visible_changed) {
>  		crtc->wm.cxsr_allowed = false;
>  		intel_set_memory_cxsr(dev_priv, false);
>  	}
> +
> +	if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed)
> +		intel_update_watermarks(&crtc->base);
>  }
>  
>  static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned
> plane_mask)
> @@ -11617,6 +11623,7 @@ static bool needs_scaling(struct intel_plane_state
> *state)
>  int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  				    struct drm_plane_state *plane_state)
>  {
> +	struct intel_crtc_state *pipe_config =
> to_intel_crtc_state(crtc_state);
>  	struct drm_crtc *crtc = crtc_state->crtc;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_plane *plane = plane_state->plane;
> @@ -11663,25 +11670,18 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  			 plane->base.id, was_visible, visible,
>  			 turn_off, turn_on, mode_changed);
>  
> -	if (turn_on) {
> -		intel_crtc->atomic.update_wm_pre = true;
> -		/* must disable cxsr around plane enable/disable */
> -		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> -			intel_crtc->atomic.disable_cxsr = true;
> -			/* to potentially re-enable cxsr */
> -			intel_crtc->atomic.wait_vblank = true;
> -			intel_crtc->atomic.update_wm_post = true;
> -		}
> -	} else if (turn_off) {
> -		intel_crtc->atomic.update_wm_post = true;
> +	if (turn_on || turn_off) {
> +		pipe_config->wm_changed = true;
> +
>  		/* must disable cxsr around plane enable/disable */
>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> +			pipe_config->visible_changed = true;
>  			if (is_crtc_enabled)
>  				intel_crtc->atomic.wait_vblank = true;
> -			intel_crtc->atomic.disable_cxsr = true;
>  		}

I'm a bit confused as to what the value of visible_changed is supposed to be. In
the comment in intel_drv.h you wrote "plane visibility changed". But that isn't
set if the cursor plane visibility changed. If the value of this is only
relevant for cxsr, maybe it make sense to still call it disable_cxsr?

Ander

> -	} else if (intel_wm_need_update(plane, plane_state)) {
> -		intel_crtc->atomic.update_wm_pre = true;
> +	} else if ((was_visible || visible) &&
> +		   intel_wm_need_update(plane, plane_state)) {
> +		pipe_config->wm_changed = true;
>  	}
>  
>  	if (visible || was_visible)
> @@ -11826,7 +11826,7 @@ static int intel_crtc_atomic_check(struct drm_crtc
> *crtc,
>  	}
>  
>  	if (mode_changed && !crtc_state->active)
> -		intel_crtc->atomic.update_wm_post = true;
> +		pipe_config->wm_changed = true;
>  
>  	if (mode_changed && crtc_state->enable &&
>  	    dev_priv->display.crtc_compute_clock &&
> @@ -13735,9 +13735,6 @@ static void intel_begin_crtc_commit(struct drm_crtc
> *crtc,
>  		to_intel_crtc_state(old_crtc_state);
>  	bool modeset = needs_modeset(crtc->state);
>  
> -	if (intel_crtc->atomic.update_wm_pre)
> -		intel_update_watermarks(crtc);
> -
>  	/* Perform vblank evasion around commit operation */
>  	intel_pipe_update_start(intel_crtc);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 3b03074813e4..59367453d24b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -372,7 +372,9 @@ struct intel_crtc_state {
>  #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync
> mode.flags */
>  	unsigned long quirks;
>  
> -	bool update_pipe;
> +	bool update_pipe; /* can a fast modeset be performed? */
> +	bool visible_changed; /* plane visibility changed */
> +	bool wm_changed; /* wm changed */
>  
>  	/* Pipe source size (ie. panel fitter input size)
>  	 * All planes will be positioned inside this space,
> @@ -535,9 +537,7 @@ struct intel_crtc_atomic_commit {
>  	/* Sleepable operations to perform before commit */
>  	bool disable_fbc;
>  	bool disable_ips;
> -	bool disable_cxsr;
>  	bool pre_disable_primary;
> -	bool update_wm_pre, update_wm_post;
>  
>  	/* Sleepable operations to perform after commit */
>  	unsigned fb_bits;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 06/14] drm/i915: Update watermark related members in the crtc_state, v2.

[Maarten Lankhorst]

Op 09-11-15 om 15:48 schreef Ander Conselvan De Oliveira:
> On Tue, 2015-11-03 at 08:31 +0100, Maarten Lankhorst wrote:
>> This removes another couple of hacks from intel_crtc->atomic, and
>> creates proper atomic state for it.
> Perhaps you could be more verbose about the hacks being removed.
Right!
>> Changes since v1:
>> - Rebase on top of wm changes.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c  |  2 ++
>>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++------------------
>> -
>>  drivers/gpu/drm/i915/intel_drv.h     |  6 +++---
>>  3 files changed, 24 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index c4eadbc928b7..3e390db9d22b 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -95,6 +95,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>>  
>>  	crtc_state->update_pipe = false;
>>  	crtc_state->disable_lp_wm = false;
>> +	crtc_state->visible_changed = false;
>> +	crtc_state->wm_changed = false;
>>  
>>  	return &crtc_state->base;
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 54e4f04bb427..356e3a9e1741 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4753,6 +4753,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
>>  static void intel_post_plane_update(struct intel_crtc *crtc)
>>  {
>>  	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>> +	struct intel_crtc_state *pipe_config =
>> +		to_intel_crtc_state(crtc->base.state);
>>  	struct drm_device *dev = crtc->base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  
>> @@ -4761,10 +4763,9 @@ static void intel_post_plane_update(struct intel_crtc
>> *crtc)
>>  
>>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>>  
>> -	if (atomic->disable_cxsr)
>> -		crtc->wm.cxsr_allowed = true;
>> +	crtc->wm.cxsr_allowed = true;
>>  
>> -	if (crtc->atomic.update_wm_post)
>> +	if (pipe_config->wm_changed)
>>  		intel_update_watermarks(&crtc->base);
>>  
>>  	if (atomic->update_fbc)
>> @@ -4781,6 +4782,8 @@ static void intel_pre_plane_update(struct intel_crtc
>> *crtc)
>>  	struct drm_device *dev = crtc->base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>> +	struct intel_crtc_state *pipe_config =
>> +		to_intel_crtc_state(crtc->base.state);
>>  
>>  	if (atomic->disable_fbc)
>>  		intel_fbc_disable_crtc(crtc);
>> @@ -4791,10 +4794,13 @@ static void intel_pre_plane_update(struct intel_crtc
>> *crtc)
>>  	if (atomic->pre_disable_primary)
>>  		intel_pre_disable_primary(&crtc->base);
>>  
>> -	if (atomic->disable_cxsr) {
>> +	if (pipe_config->visible_changed) {
>>  		crtc->wm.cxsr_allowed = false;
>>  		intel_set_memory_cxsr(dev_priv, false);
>>  	}
>> +
>> +	if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed)
>> +		intel_update_watermarks(&crtc->base);
>>  }
>>  
>>  static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned
>> plane_mask)
>> @@ -11617,6 +11623,7 @@ static bool needs_scaling(struct intel_plane_state
>> *state)
>>  int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>>  				    struct drm_plane_state *plane_state)
>>  {
>> +	struct intel_crtc_state *pipe_config =
>> to_intel_crtc_state(crtc_state);
>>  	struct drm_crtc *crtc = crtc_state->crtc;
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  	struct drm_plane *plane = plane_state->plane;
>> @@ -11663,25 +11670,18 @@ int intel_plane_atomic_calc_changes(struct
>> drm_crtc_state *crtc_state,
>>  			 plane->base.id, was_visible, visible,
>>  			 turn_off, turn_on, mode_changed);
>>  
>> -	if (turn_on) {
>> -		intel_crtc->atomic.update_wm_pre = true;
>> -		/* must disable cxsr around plane enable/disable */
>> -		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>> -			intel_crtc->atomic.disable_cxsr = true;
>> -			/* to potentially re-enable cxsr */
>> -			intel_crtc->atomic.wait_vblank = true;
>> -			intel_crtc->atomic.update_wm_post = true;
>> -		}
>> -	} else if (turn_off) {
>> -		intel_crtc->atomic.update_wm_post = true;
>> +	if (turn_on || turn_off) {
>> +		pipe_config->wm_changed = true;
>> +
>>  		/* must disable cxsr around plane enable/disable */
>>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>> +			pipe_config->visible_changed = true;
>>  			if (is_crtc_enabled)
>>  				intel_crtc->atomic.wait_vblank = true;
>> -			intel_crtc->atomic.disable_cxsr = true;
>>  		}
> I'm a bit confused as to what the value of visible_changed is supposed to be. In
> the comment in intel_drv.h you wrote "plane visibility changed". But that isn't
> set if the cursor plane visibility changed. If the value of this is only
> relevant for cxsr, maybe it make sense to still call it disable_cxsr?
>
Would it be terrible if I were to disable CXSR for cursor changes too?
Else I'll rename it to disable_cxsr.

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

Re: [PATCH v2 06/14] drm/i915: Update watermark related members in the crtc_state, v2.

[Ville Syrjälä]

On Mon, Nov 09, 2015 at 05:10:40PM +0100, Maarten Lankhorst wrote:
> Op 09-11-15 om 15:48 schreef Ander Conselvan De Oliveira:
> > On Tue, 2015-11-03 at 08:31 +0100, Maarten Lankhorst wrote:
> >> This removes another couple of hacks from intel_crtc->atomic, and
> >> creates proper atomic state for it.
> > Perhaps you could be more verbose about the hacks being removed.
> Right!
> >> Changes since v1:
> >> - Rebase on top of wm changes.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_atomic.c  |  2 ++
> >>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++------------------
> >> -
> >>  drivers/gpu/drm/i915/intel_drv.h     |  6 +++---
> >>  3 files changed, 24 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> >> b/drivers/gpu/drm/i915/intel_atomic.c
> >> index c4eadbc928b7..3e390db9d22b 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >> @@ -95,6 +95,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
> >>  
> >>  	crtc_state->update_pipe = false;
> >>  	crtc_state->disable_lp_wm = false;
> >> +	crtc_state->visible_changed = false;
> >> +	crtc_state->wm_changed = false;
> >>  
> >>  	return &crtc_state->base;
> >>  }
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c
> >> b/drivers/gpu/drm/i915/intel_display.c
> >> index 54e4f04bb427..356e3a9e1741 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4753,6 +4753,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
> >>  static void intel_post_plane_update(struct intel_crtc *crtc)
> >>  {
> >>  	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
> >> +	struct intel_crtc_state *pipe_config =
> >> +		to_intel_crtc_state(crtc->base.state);
> >>  	struct drm_device *dev = crtc->base.dev;
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  
> >> @@ -4761,10 +4763,9 @@ static void intel_post_plane_update(struct intel_crtc
> >> *crtc)
> >>  
> >>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
> >>  
> >> -	if (atomic->disable_cxsr)
> >> -		crtc->wm.cxsr_allowed = true;
> >> +	crtc->wm.cxsr_allowed = true;
> >>  
> >> -	if (crtc->atomic.update_wm_post)
> >> +	if (pipe_config->wm_changed)
> >>  		intel_update_watermarks(&crtc->base);
> >>  
> >>  	if (atomic->update_fbc)
> >> @@ -4781,6 +4782,8 @@ static void intel_pre_plane_update(struct intel_crtc
> >> *crtc)
> >>  	struct drm_device *dev = crtc->base.dev;
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
> >> +	struct intel_crtc_state *pipe_config =
> >> +		to_intel_crtc_state(crtc->base.state);
> >>  
> >>  	if (atomic->disable_fbc)
> >>  		intel_fbc_disable_crtc(crtc);
> >> @@ -4791,10 +4794,13 @@ static void intel_pre_plane_update(struct intel_crtc
> >> *crtc)
> >>  	if (atomic->pre_disable_primary)
> >>  		intel_pre_disable_primary(&crtc->base);
> >>  
> >> -	if (atomic->disable_cxsr) {
> >> +	if (pipe_config->visible_changed) {
> >>  		crtc->wm.cxsr_allowed = false;
> >>  		intel_set_memory_cxsr(dev_priv, false);
> >>  	}
> >> +
> >> +	if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed)
> >> +		intel_update_watermarks(&crtc->base);
> >>  }
> >>  
> >>  static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned
> >> plane_mask)
> >> @@ -11617,6 +11623,7 @@ static bool needs_scaling(struct intel_plane_state
> >> *state)
> >>  int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> >>  				    struct drm_plane_state *plane_state)
> >>  {
> >> +	struct intel_crtc_state *pipe_config =
> >> to_intel_crtc_state(crtc_state);
> >>  	struct drm_crtc *crtc = crtc_state->crtc;
> >>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >>  	struct drm_plane *plane = plane_state->plane;
> >> @@ -11663,25 +11670,18 @@ int intel_plane_atomic_calc_changes(struct
> >> drm_crtc_state *crtc_state,
> >>  			 plane->base.id, was_visible, visible,
> >>  			 turn_off, turn_on, mode_changed);
> >>  
> >> -	if (turn_on) {
> >> -		intel_crtc->atomic.update_wm_pre = true;
> >> -		/* must disable cxsr around plane enable/disable */
> >> -		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> >> -			intel_crtc->atomic.disable_cxsr = true;
> >> -			/* to potentially re-enable cxsr */
> >> -			intel_crtc->atomic.wait_vblank = true;
> >> -			intel_crtc->atomic.update_wm_post = true;
> >> -		}
> >> -	} else if (turn_off) {
> >> -		intel_crtc->atomic.update_wm_post = true;
> >> +	if (turn_on || turn_off) {
> >> +		pipe_config->wm_changed = true;
> >> +
> >>  		/* must disable cxsr around plane enable/disable */
> >>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> >> +			pipe_config->visible_changed = true;
> >>  			if (is_crtc_enabled)
> >>  				intel_crtc->atomic.wait_vblank = true;
> >> -			intel_crtc->atomic.disable_cxsr = true;
> >>  		}
> > I'm a bit confused as to what the value of visible_changed is supposed to be. In
> > the comment in intel_drv.h you wrote "plane visibility changed". But that isn't
> > set if the cursor plane visibility changed. If the value of this is only
> > relevant for cxsr, maybe it make sense to still call it disable_cxsr?
> >
> Would it be terrible if I were to disable CXSR for cursor changes too?

Yes, we need vblank waits around cxsr updates.

> Else I'll rename it to disable_cxsr.
> 
> ~Maarten

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

[PATCH v2 07/14] drm/i915: Kill off intel_crtc->atomic.wait_vblank.

[Maarten Lankhorst]

By handling this after the atomic helper waits for vblanks there will
be one less wait for vblank in the atomic path.

Also get rid of the double wait_for_vblank on broadwell, looks like
it's a bug doing the vblank wait twice.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  |  1 +
 drivers/gpu/drm/i915/intel_display.c | 95 +++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 3 files changed, 64 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 3e390db9d22b..2ba0cd698f9b 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->disable_lp_wm = false;
 	crtc_state->visible_changed = false;
 	crtc_state->wm_changed = false;
+	crtc_state->fb_changed = false;
 
 	return &crtc_state->base;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 356e3a9e1741..2708899d9767 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4669,14 +4669,6 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 
 	/*
-	 * BDW signals flip done immediately if the plane
-	 * is disabled, even if the plane enable is already
-	 * armed to occur at the next vblank :(
-	 */
-	if (IS_BROADWELL(dev))
-		intel_wait_for_vblank(dev, pipe);
-
-	/*
 	 * FIXME IPS should be fine as long as one plane is
 	 * enabled, but in practice it seems to have problems
 	 * when going from primary only to sprite only and vice
@@ -4758,9 +4750,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (atomic->wait_vblank)
-		intel_wait_for_vblank(dev, crtc->pipe);
-
 	intel_frontbuffer_flip(dev, atomic->fb_bits);
 
 	crtc->wm.cxsr_allowed = true;
@@ -11660,6 +11649,9 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	if (!was_visible && !visible)
 		return 0;
 
+	if (fb != old_plane_state->base.fb)
+		pipe_config->fb_changed = true;
+
 	turn_off = was_visible && (!visible || mode_changed);
 	turn_on = visible && (!was_visible || mode_changed);
 
@@ -11676,8 +11668,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		/* must disable cxsr around plane enable/disable */
 		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
 			pipe_config->visible_changed = true;
-			if (is_crtc_enabled)
-				intel_crtc->atomic.wait_vblank = true;
 		}
 	} else if ((was_visible || visible) &&
 		   intel_wm_need_update(plane, plane_state)) {
@@ -11724,14 +11714,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		    plane_state->rotation != BIT(DRM_ROTATE_0))
 			intel_crtc->atomic.disable_fbc = true;
 
-		/*
-		 * BDW signals flip done immediately if the plane
-		 * is disabled, even if the plane enable is already
-		 * armed to occur at the next vblank :(
-		 */
-		if (turn_on && IS_BROADWELL(dev))
-			intel_crtc->atomic.wait_vblank = true;
-
 		intel_crtc->atomic.update_fbc |= visible || mode_changed;
 		break;
 	case DRM_PLANE_TYPE_CURSOR:
@@ -11746,12 +11728,10 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		if (IS_IVYBRIDGE(dev) &&
 		    needs_scaling(to_intel_plane_state(plane_state)) &&
 		    !needs_scaling(old_plane_state)) {
-			to_intel_crtc_state(crtc_state)->disable_lp_wm = true;
-		} else if (turn_off && !mode_changed) {
-			intel_crtc->atomic.wait_vblank = true;
+			pipe_config->disable_lp_wm = true;
+		} else if (turn_off && !mode_changed)
 			intel_crtc->atomic.update_sprite_watermarks |=
 				1 << i;
-		}
 
 		break;
 	}
@@ -13261,6 +13241,48 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
 	return ret;
 }
 
+static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
+					  struct drm_i915_private *dev_priv,
+					  unsigned crtc_mask)
+{
+	unsigned last_vblank_count[I915_MAX_PIPES];
+	enum pipe pipe;
+	int ret;
+
+	if (!crtc_mask)
+		return;
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		ret = drm_crtc_vblank_get(crtc);
+		if (ret != 0) {
+			crtc_mask &= ~(1 << pipe);
+			continue;
+		}
+
+		last_vblank_count[pipe] = drm_crtc_vblank_count(crtc);
+	}
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		wait_event_timeout(dev->vblank[pipe].queue,
+				last_vblank_count[pipe] !=
+					drm_crtc_vblank_count(crtc),
+				msecs_to_jiffies(50));
+
+		drm_crtc_vblank_put(crtc);
+	}
+}
+
+
 /**
  * intel_atomic_commit - commit validated state object
  * @dev: DRM device
@@ -13283,11 +13305,11 @@ static int intel_atomic_commit(struct drm_device *dev,
 {
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool hw_check = intel_state->modeset;
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc *crtc;
-	int ret = 0;
-	int i;
-	bool hw_check = intel_state->modeset;
+	int ret = 0, i;
+	unsigned crtc_vblank_mask = 0;
 
 	ret = intel_atomic_prepare_commit(dev, state, async);
 	if (ret) {
@@ -13335,8 +13357,9 @@ static int intel_atomic_commit(struct drm_device *dev,
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 		bool modeset = needs_modeset(crtc->state);
-		bool update_pipe = !modeset &&
-			to_intel_crtc_state(crtc->state)->update_pipe;
+		struct intel_crtc_state *pipe_config =
+			to_intel_crtc_state(crtc->state);
+		bool update_pipe = !modeset && pipe_config->update_pipe;
 		unsigned long put_domains = 0;
 
 		if (modeset && crtc->state->active) {
@@ -13358,15 +13381,21 @@ static int intel_atomic_commit(struct drm_device *dev,
 		    (crtc->state->planes_changed || update_pipe))
 			drm_atomic_helper_commit_planes_on_crtc(crtc_state);
 
+		if (pipe_config->base.active &&
+		    (pipe_config->wm_changed || pipe_config->visible_changed ||
+		     pipe_config->fb_changed))
+			crtc_vblank_mask |= 1 << i;
+
 		if (put_domains)
 			modeset_put_power_domains(dev_priv, put_domains);
-
-		intel_post_plane_update(intel_crtc);
 	}
 
 	/* FIXME: add subpixel order */
 
-	drm_atomic_helper_wait_for_vblanks(dev, state);
+	intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i)
+		intel_post_plane_update(to_intel_crtc(crtc));
 
 	mutex_lock(&dev->struct_mutex);
 	drm_atomic_helper_cleanup_planes(dev, state);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 59367453d24b..6f99c7398af3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -375,6 +375,7 @@ struct intel_crtc_state {
 	bool update_pipe; /* can a fast modeset be performed? */
 	bool visible_changed; /* plane visibility changed */
 	bool wm_changed; /* wm changed */
+	bool fb_changed; /* fb on any of the planes is changed */
 
 	/* Pipe source size (ie. panel fitter input size)
 	 * All planes will be positioned inside this space,
@@ -541,7 +542,6 @@ struct intel_crtc_atomic_commit {
 
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
-	bool wait_vblank;
 	bool update_fbc;
 	bool post_enable_primary;
 	unsigned update_sprite_watermarks;
-- 
2.1.0

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

Re: [PATCH v2 07/14] drm/i915: Kill off intel_crtc->atomic.wait_vblank.

[Ander Conselvan De Oliveira]

On Tue, 2015-11-03 at 08:31 +0100, Maarten Lankhorst wrote:
> By handling this after the atomic helper waits for vblanks there will
> be one less wait for vblank in the atomic path.

I found this a bit confusing. Sounds like you are referring to calling
intel_post_enable_update() after the call to drm_atomic_helper_wait_for_vblanks(
), except that move wasn't mentioned before. Also, the patch replaces that call
with our own wait for vblank function, so it is probably good to mention why in
the commit message.

> 
> Also get rid of the double wait_for_vblank on broadwell, looks like
> it's a bug doing the vblank wait twice.

Separate patch with more details in the commit message.

Ander

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 95 +++++++++++++++++++++++------------
> -
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>  3 files changed, 64 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index 3e390db9d22b..2ba0cd698f9b 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  	crtc_state->disable_lp_wm = false;
>  	crtc_state->visible_changed = false;
>  	crtc_state->wm_changed = false;
> +	crtc_state->fb_changed = false;
>  
>  	return &crtc_state->base;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 356e3a9e1741..2708899d9767 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4669,14 +4669,6 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  
>  	/*
> -	 * BDW signals flip done immediately if the plane
> -	 * is disabled, even if the plane enable is already
> -	 * armed to occur at the next vblank :(
> -	 */
> -	if (IS_BROADWELL(dev))
> -		intel_wait_for_vblank(dev, pipe);
> -
> -	/*
>  	 * FIXME IPS should be fine as long as one plane is
>  	 * enabled, but in practice it seems to have problems
>  	 * when going from primary only to sprite only and vice
> @@ -4758,9 +4750,6 @@ static void intel_post_plane_update(struct intel_crtc
> *crtc)
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (atomic->wait_vblank)
> -		intel_wait_for_vblank(dev, crtc->pipe);
> -
>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>  
>  	crtc->wm.cxsr_allowed = true;
> @@ -11660,6 +11649,9 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  	if (!was_visible && !visible)
>  		return 0;
>  
> +	if (fb != old_plane_state->base.fb)
> +		pipe_config->fb_changed = true;
> +
>  	turn_off = was_visible && (!visible || mode_changed);
>  	turn_on = visible && (!was_visible || mode_changed);
>  
> @@ -11676,8 +11668,6 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  		/* must disable cxsr around plane enable/disable */
>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>  			pipe_config->visible_changed = true;
> -			if (is_crtc_enabled)
> -				intel_crtc->atomic.wait_vblank = true;
>  		}
>  	} else if ((was_visible || visible) &&
>  		   intel_wm_need_update(plane, plane_state)) {
> @@ -11724,14 +11714,6 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  		    plane_state->rotation != BIT(DRM_ROTATE_0))
>  			intel_crtc->atomic.disable_fbc = true;
>  
> -		/*
> -		 * BDW signals flip done immediately if the plane
> -		 * is disabled, even if the plane enable is already
> -		 * armed to occur at the next vblank :(
> -		 */
> -		if (turn_on && IS_BROADWELL(dev))
> -			intel_crtc->atomic.wait_vblank = true;
> -
>  		intel_crtc->atomic.update_fbc |= visible || mode_changed;
>  		break;
>  	case DRM_PLANE_TYPE_CURSOR:
> @@ -11746,12 +11728,10 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  		if (IS_IVYBRIDGE(dev) &&
>  		    needs_scaling(to_intel_plane_state(plane_state)) &&
>  		    !needs_scaling(old_plane_state)) {
> -			to_intel_crtc_state(crtc_state)->disable_lp_wm =
> true;
> -		} else if (turn_off && !mode_changed) {
> -			intel_crtc->atomic.wait_vblank = true;
> +			pipe_config->disable_lp_wm = true;
> +		} else if (turn_off && !mode_changed)
>  			intel_crtc->atomic.update_sprite_watermarks |=
>  				1 << i;
> -		}
>  
>  		break;
>  	}
> @@ -13261,6 +13241,48 @@ static int intel_atomic_prepare_commit(struct
> drm_device *dev,
>  	return ret;
>  }
>  
> +static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
> +					  struct drm_i915_private *dev_priv,
> +					  unsigned crtc_mask)
> +{
> +	unsigned last_vblank_count[I915_MAX_PIPES];
> +	enum pipe pipe;
> +	int ret;
> +
> +	if (!crtc_mask)
> +		return;
> +
> +	for_each_pipe(dev_priv, pipe) {
> +		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> +
> +		if (!((1 << pipe) & crtc_mask))
> +			continue;
> +
> +		ret = drm_crtc_vblank_get(crtc);
> +		if (ret != 0) {
> +			crtc_mask &= ~(1 << pipe);
> +			continue;
> +		}
> +
> +		last_vblank_count[pipe] = drm_crtc_vblank_count(crtc);
> +	}
> +
> +	for_each_pipe(dev_priv, pipe) {
> +		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> +
> +		if (!((1 << pipe) & crtc_mask))
> +			continue;
> +
> +		wait_event_timeout(dev->vblank[pipe].queue,
> +				last_vblank_count[pipe] !=
> +					drm_crtc_vblank_count(crtc),
> +				msecs_to_jiffies(50));
> +
> +		drm_crtc_vblank_put(crtc);
> +	}
> +}
> +
> +
>  /**
>   * intel_atomic_commit - commit validated state object
>   * @dev: DRM device
> @@ -13283,11 +13305,11 @@ static int intel_atomic_commit(struct drm_device
> *dev,
>  {
>  	struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	bool hw_check = intel_state->modeset;
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_crtc *crtc;
> -	int ret = 0;
> -	int i;
> -	bool hw_check = intel_state->modeset;
> +	int ret = 0, i;
> +	unsigned crtc_vblank_mask = 0;
>  
>  	ret = intel_atomic_prepare_commit(dev, state, async);
>  	if (ret) {
> @@ -13335,8 +13357,9 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  		bool modeset = needs_modeset(crtc->state);
> -		bool update_pipe = !modeset &&
> -			to_intel_crtc_state(crtc->state)->update_pipe;
> +		struct intel_crtc_state *pipe_config =
> +			to_intel_crtc_state(crtc->state);
> +		bool update_pipe = !modeset && pipe_config->update_pipe;
>  		unsigned long put_domains = 0;
>  
>  		if (modeset && crtc->state->active) {
> @@ -13358,15 +13381,21 @@ static int intel_atomic_commit(struct drm_device
> *dev,
>  		    (crtc->state->planes_changed || update_pipe))
>  			drm_atomic_helper_commit_planes_on_crtc(crtc_state);
>  
> +		if (pipe_config->base.active &&
> +		    (pipe_config->wm_changed || pipe_config->visible_changed
> ||
> +		     pipe_config->fb_changed))
> +			crtc_vblank_mask |= 1 << i;
> +
>  		if (put_domains)
>  			modeset_put_power_domains(dev_priv, put_domains);
> -
> -		intel_post_plane_update(intel_crtc);
>  	}
>  
>  	/* FIXME: add subpixel order */
>  
> -	drm_atomic_helper_wait_for_vblanks(dev, state);
> +	intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i)
> +		intel_post_plane_update(to_intel_crtc(crtc));
>  
>  	mutex_lock(&dev->struct_mutex);
>  	drm_atomic_helper_cleanup_planes(dev, state);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 59367453d24b..6f99c7398af3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -375,6 +375,7 @@ struct intel_crtc_state {
>  	bool update_pipe; /* can a fast modeset be performed? */
>  	bool visible_changed; /* plane visibility changed */
>  	bool wm_changed; /* wm changed */
> +	bool fb_changed; /* fb on any of the planes is changed */
>  
>  	/* Pipe source size (ie. panel fitter input size)
>  	 * All planes will be positioned inside this space,
> @@ -541,7 +542,6 @@ struct intel_crtc_atomic_commit {
>  
>  	/* Sleepable operations to perform after commit */
>  	unsigned fb_bits;
> -	bool wait_vblank;
>  	bool update_fbc;
>  	bool post_enable_primary;
>  	unsigned update_sprite_watermarks;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 08/14] drm/i915: Remove intel_crtc->atomic.disable_ips.

[Maarten Lankhorst]

This is already handled in pre_disable_primary, disabling it twice is useless.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 +---------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 2 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2708899d9767..58074f4adca2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4777,9 +4777,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 	if (atomic->disable_fbc)
 		intel_fbc_disable_crtc(crtc);
 
-	if (crtc->atomic.disable_ips)
-		hsw_disable_ips(crtc);
-
 	if (atomic->pre_disable_primary)
 		intel_pre_disable_primary(&crtc->base);
 
@@ -11683,19 +11680,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		intel_crtc->atomic.pre_disable_primary = turn_off;
 		intel_crtc->atomic.post_enable_primary = turn_on;
 
-		if (turn_off) {
-			/*
-			 * FIXME: Actually if we will still have any other
-			 * plane enabled on the pipe we could let IPS enabled
-			 * still, but for now lets consider that when we make
-			 * primary invisible by setting DSPCNTR to 0 on
-			 * update_primary_plane function IPS needs to be
-			 * disable.
-			 */
-			intel_crtc->atomic.disable_ips = true;
-
+		if (turn_off)
 			intel_crtc->atomic.disable_fbc = true;
-		}
 
 		/*
 		 * FBC does not work on some platforms for rotated
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6f99c7398af3..ce5f10fc92aa 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -537,7 +537,6 @@ struct intel_mmio_flip {
 struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
 	bool disable_fbc;
-	bool disable_ips;
 	bool pre_disable_primary;
 
 	/* Sleepable operations to perform after commit */
-- 
2.1.0

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

Re: [PATCH v2 08/14] drm/i915: Remove intel_crtc->atomic.disable_ips.

[Ander Conselvan De Oliveira]

On Tue, 2015-11-03 at 08:31 +0100, Maarten Lankhorst wrote:
> This is already handled in pre_disable_primary, disabling it twice is useless.

The atomic.disable_ips field was added in

commit 066cf55b9ce35f1f90dde9fcec01431a9243a949
Author: Rodrigo Vivi <rodrigo.vivi@intel.com>
Date:   Fri Jun 26 13:55:54 2015 -0700

    drm/i915: Fix IPS related flicker

at which point intel_pre_disable_primary() already called hsw_disable_ips().
This patch is an exact revert of that patch, so it would be good to mention what
changed in between that makes it unnecessary. I suppose it is the fact that
everything goes through the atomic path now.

Ander

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 16 +---------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  2 files changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 2708899d9767..58074f4adca2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4777,9 +4777,6 @@ static void intel_pre_plane_update(struct intel_crtc
> *crtc)
>  	if (atomic->disable_fbc)
>  		intel_fbc_disable_crtc(crtc);
>  
> -	if (crtc->atomic.disable_ips)
> -		hsw_disable_ips(crtc);
> -
>  	if (atomic->pre_disable_primary)
>  		intel_pre_disable_primary(&crtc->base);
>  
> @@ -11683,19 +11680,8 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  		intel_crtc->atomic.pre_disable_primary = turn_off;
>  		intel_crtc->atomic.post_enable_primary = turn_on;
>  
> -		if (turn_off) {
> -			/*
> -			 * FIXME: Actually if we will still have any other
> -			 * plane enabled on the pipe we could let IPS enabled
> -			 * still, but for now lets consider that when we make
> -			 * primary invisible by setting DSPCNTR to 0 on
> -			 * update_primary_plane function IPS needs to be
> -			 * disable.
> -			 */
> -			intel_crtc->atomic.disable_ips = true;
> -
> +		if (turn_off)
>  			intel_crtc->atomic.disable_fbc = true;
> -		}
>  
>  		/*
>  		 * FBC does not work on some platforms for rotated
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 6f99c7398af3..ce5f10fc92aa 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -537,7 +537,6 @@ struct intel_mmio_flip {
>  struct intel_crtc_atomic_commit {
>  	/* Sleepable operations to perform before commit */
>  	bool disable_fbc;
> -	bool disable_ips;
>  	bool pre_disable_primary;
>  
>  	/* Sleepable operations to perform after commit */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 09/14] drm/i915: Remove atomic.pre_disable_primary.

[Maarten Lankhorst]

This can be derived from the atomic state in pre_plane_update,
which makes it more clear when it's supposed to be called.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 58074f4adca2..b1d6ce80daaf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4766,26 +4766,40 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 	memset(atomic, 0, sizeof(*atomic));
 }
 
-static void intel_pre_plane_update(struct intel_crtc *crtc)
+static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 {
+	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
+	struct drm_atomic_state *old_state = old_crtc_state->base.state;
+	struct drm_plane *primary = crtc->base.primary;
+	struct drm_plane_state *old_pri_state =
+		drm_atomic_get_existing_plane_state(old_state, primary);
+	bool modeset = needs_modeset(&pipe_config->base);
 
 	if (atomic->disable_fbc)
 		intel_fbc_disable_crtc(crtc);
 
-	if (atomic->pre_disable_primary)
-		intel_pre_disable_primary(&crtc->base);
+	if (old_pri_state) {
+		struct intel_plane_state *primary_state =
+			to_intel_plane_state(primary->state);
+		struct intel_plane_state *old_primary_state =
+			to_intel_plane_state(old_pri_state);
+
+		if (old_primary_state->visible &&
+		    (modeset || !primary_state->visible))
+			intel_pre_disable_primary(&crtc->base);
+	}
 
 	if (pipe_config->visible_changed) {
 		crtc->wm.cxsr_allowed = false;
 		intel_set_memory_cxsr(dev_priv, false);
 	}
 
-	if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed)
+	if (!modeset && pipe_config->wm_changed)
 		intel_update_watermarks(&crtc->base);
 }
 
@@ -11677,7 +11691,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 
 	switch (plane->type) {
 	case DRM_PLANE_TYPE_PRIMARY:
-		intel_crtc->atomic.pre_disable_primary = turn_off;
 		intel_crtc->atomic.post_enable_primary = turn_on;
 
 		if (turn_off)
@@ -13318,7 +13331,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 		if (!needs_modeset(crtc->state))
 			continue;
 
-		intel_pre_plane_update(intel_crtc);
+		intel_pre_plane_update(to_intel_crtc_state(crtc_state));
 
 		if (crtc_state->active) {
 			intel_crtc_disable_planes(crtc, crtc_state->plane_mask);
@@ -13341,7 +13354,6 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 		bool modeset = needs_modeset(crtc->state);
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc->state);
@@ -13361,7 +13373,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 		}
 
 		if (!modeset)
-			intel_pre_plane_update(intel_crtc);
+			intel_pre_plane_update(to_intel_crtc_state(crtc_state));
 
 		if (crtc->state->active &&
 		    (crtc->state->planes_changed || update_pipe))
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ce5f10fc92aa..8d86627069e5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -537,7 +537,6 @@ struct intel_mmio_flip {
 struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
 	bool disable_fbc;
-	bool pre_disable_primary;
 
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
-- 
2.1.0

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

Re: [PATCH v2 09/14] drm/i915: Remove atomic.pre_disable_primary.

[Ander Conselvan De Oliveira]

On Tue, 2015-11-03 at 08:31 +0100, Maarten Lankhorst wrote:
> This can be derived from the atomic state in pre_plane_update,
> which makes it more clear when it's supposed to be called.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  2 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 58074f4adca2..b1d6ce80daaf 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4766,26 +4766,40 @@ static void intel_post_plane_update(struct intel_crtc
> *crtc)
>  	memset(atomic, 0, sizeof(*atomic));
>  }
>  
> -static void intel_pre_plane_update(struct intel_crtc *crtc)
> +static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc->base.state);
> +	struct drm_atomic_state *old_state = old_crtc_state->base.state;
> +	struct drm_plane *primary = crtc->base.primary;
> +	struct drm_plane_state *old_pri_state =
> +		drm_atomic_get_existing_plane_state(old_state, primary);
> +	bool modeset = needs_modeset(&pipe_config->base);
>  
>  	if (atomic->disable_fbc)
>  		intel_fbc_disable_crtc(crtc);
>  
> -	if (atomic->pre_disable_primary)
> -		intel_pre_disable_primary(&crtc->base);
> +	if (old_pri_state) {
> +		struct intel_plane_state *primary_state =
> +			to_intel_plane_state(primary->state);
> +		struct intel_plane_state *old_primary_state =
> +			to_intel_plane_state(old_pri_state);

Hmm, old_primary_state is the intel_state cast of old_pri_state. Not very
descriptive. Maybe we could have a intel_atomic_get_existing_plane_state()
helper that does the cast for us. Seems like that's the end goal almost every
time we call drm_atomic_get_existing_plane_state() anyway. But I guess that can
be done later, so

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

> +
> +		if (old_primary_state->visible &&
> +		    (modeset || !primary_state->visible))
> +			intel_pre_disable_primary(&crtc->base);
> +	}
>  
>  	if (pipe_config->visible_changed) {
>  		crtc->wm.cxsr_allowed = false;
>  		intel_set_memory_cxsr(dev_priv, false);
>  	}
>  
> -	if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed)
> +	if (!modeset && pipe_config->wm_changed)
>  		intel_update_watermarks(&crtc->base);
>  }
>  
> @@ -11677,7 +11691,6 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  
>  	switch (plane->type) {
>  	case DRM_PLANE_TYPE_PRIMARY:
> -		intel_crtc->atomic.pre_disable_primary = turn_off;
>  		intel_crtc->atomic.post_enable_primary = turn_on;
>  
>  		if (turn_off)
> @@ -13318,7 +13331,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		if (!needs_modeset(crtc->state))
>  			continue;
>  
> -		intel_pre_plane_update(intel_crtc);
> +		intel_pre_plane_update(to_intel_crtc_state(crtc_state));
>  
>  		if (crtc_state->active) {
>  			intel_crtc_disable_planes(crtc, crtc_state
> ->plane_mask);
> @@ -13341,7 +13354,6 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up.
> */
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  		bool modeset = needs_modeset(crtc->state);
>  		struct intel_crtc_state *pipe_config =
>  			to_intel_crtc_state(crtc->state);
> @@ -13361,7 +13373,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		}
>  
>  		if (!modeset)
> -			intel_pre_plane_update(intel_crtc);
> +			intel_pre_plane_update(to_intel_crtc_state(crtc_state
> ));
>  
>  		if (crtc->state->active &&
>  		    (crtc->state->planes_changed || update_pipe))
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index ce5f10fc92aa..8d86627069e5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -537,7 +537,6 @@ struct intel_mmio_flip {
>  struct intel_crtc_atomic_commit {
>  	/* Sleepable operations to perform before commit */
>  	bool disable_fbc;
> -	bool pre_disable_primary;
>  
>  	/* Sleepable operations to perform after commit */
>  	unsigned fb_bits;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 10/14] drm/i915: Remove update_sprite_watermarks.

[Maarten Lankhorst]

Commit 791a32be6eb2 ("drm/i915: Drop intel_update_sprite_watermarks")
removes the use of this variable, but forgot to remove it.

Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 +-----
 drivers/gpu/drm/i915/intel_drv.h     | 1 -
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b1d6ce80daaf..77fe1d75404b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11632,7 +11632,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	struct intel_plane_state *old_plane_state =
 		to_intel_plane_state(plane->state);
 	int idx = intel_crtc->base.base.id, ret;
-	int i = drm_plane_index(plane);
 	bool mode_changed = needs_modeset(crtc_state);
 	bool was_crtc_enabled = crtc->state->active;
 	bool is_crtc_enabled = crtc_state->active;
@@ -11726,11 +11725,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		 */
 		if (IS_IVYBRIDGE(dev) &&
 		    needs_scaling(to_intel_plane_state(plane_state)) &&
-		    !needs_scaling(old_plane_state)) {
+		    !needs_scaling(old_plane_state))
 			pipe_config->disable_lp_wm = true;
-		} else if (turn_off && !mode_changed)
-			intel_crtc->atomic.update_sprite_watermarks |=
-				1 << i;
 
 		break;
 	}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8d86627069e5..dc024c27ca1e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -542,7 +542,6 @@ struct intel_crtc_atomic_commit {
 	unsigned fb_bits;
 	bool update_fbc;
 	bool post_enable_primary;
-	unsigned update_sprite_watermarks;
 };
 
 struct intel_crtc {
-- 
2.1.0

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

Re: [PATCH v2 10/14] drm/i915: Remove update_sprite_watermarks.

[Matt Roper]

On Tue, Nov 03, 2015 at 08:31:49AM +0100, Maarten Lankhorst wrote:
> Commit 791a32be6eb2 ("drm/i915: Drop intel_update_sprite_watermarks")
> removes the use of this variable, but forgot to remove it.
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 +-----
>  drivers/gpu/drm/i915/intel_drv.h     | 1 -
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b1d6ce80daaf..77fe1d75404b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11632,7 +11632,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  	struct intel_plane_state *old_plane_state =
>  		to_intel_plane_state(plane->state);
>  	int idx = intel_crtc->base.base.id, ret;
> -	int i = drm_plane_index(plane);
>  	bool mode_changed = needs_modeset(crtc_state);
>  	bool was_crtc_enabled = crtc->state->active;
>  	bool is_crtc_enabled = crtc_state->active;
> @@ -11726,11 +11725,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  		 */
>  		if (IS_IVYBRIDGE(dev) &&
>  		    needs_scaling(to_intel_plane_state(plane_state)) &&
> -		    !needs_scaling(old_plane_state)) {
> +		    !needs_scaling(old_plane_state))
>  			pipe_config->disable_lp_wm = true;
> -		} else if (turn_off && !mode_changed)
> -			intel_crtc->atomic.update_sprite_watermarks |=
> -				1 << i;
>  
>  		break;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8d86627069e5..dc024c27ca1e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -542,7 +542,6 @@ struct intel_crtc_atomic_commit {
>  	unsigned fb_bits;
>  	bool update_fbc;
>  	bool post_enable_primary;
> -	unsigned update_sprite_watermarks;
>  };
>  
>  struct intel_crtc {
> -- 
> 2.1.0
> 

-- 
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 v2 11/14] drm/i915: Remove some post-commit members from intel_crtc->atomic.

[Maarten Lankhorst]

fb_bits is useful to have in the crtc_state for cs flips later on,
so keep it alive there. The other stuff can be calculated in
post_plane_update, and aren't useful elsewhere.

Currently there's a loop in post_plane_update, this should disappear
with the changes to atomic wm's. At that point only updates to the
primary plane are important.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  |  1 +
 drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_drv.h     |  3 +--
 3 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 2ba0cd698f9b..d890f5f63240 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -98,6 +98,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->visible_changed = false;
 	crtc_state->wm_changed = false;
 	crtc_state->fb_changed = false;
+	crtc_state->fb_bits = 0;
 
 	return &crtc_state->base;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 77fe1d75404b..c42e87c62133 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4742,15 +4742,20 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 	hsw_disable_ips(intel_crtc);
 }
 
-static void intel_post_plane_update(struct intel_crtc *crtc)
+static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 {
+	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
+	struct drm_atomic_state *old_state = old_crtc_state->base.state;
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_plane *primary = crtc->base.primary;
+	struct drm_plane_state *old_pri_state =
+		drm_atomic_get_existing_plane_state(old_state, primary);
 
-	intel_frontbuffer_flip(dev, atomic->fb_bits);
+	intel_frontbuffer_flip(dev, pipe_config->fb_bits);
 
 	crtc->wm.cxsr_allowed = true;
 
@@ -4760,8 +4765,17 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 	if (atomic->update_fbc)
 		intel_fbc_update(dev_priv);
 
-	if (atomic->post_enable_primary)
-		intel_post_enable_primary(&crtc->base);
+	if (old_pri_state) {
+		struct intel_plane_state *primary_state =
+			to_intel_plane_state(primary->state);
+		struct intel_plane_state *old_primary_state =
+			to_intel_plane_state(old_pri_state);
+
+		if (primary_state->visible &&
+		    (needs_modeset(&pipe_config->base) ||
+		     !old_primary_state->visible))
+			intel_post_enable_primary(&crtc->base);
+	}
 
 	memset(atomic, 0, sizeof(*atomic));
 }
@@ -11685,13 +11699,10 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	}
 
 	if (visible || was_visible)
-		intel_crtc->atomic.fb_bits |=
-			to_intel_plane(plane)->frontbuffer_bit;
+		pipe_config->fb_bits |= to_intel_plane(plane)->frontbuffer_bit;
 
 	switch (plane->type) {
 	case DRM_PLANE_TYPE_PRIMARY:
-		intel_crtc->atomic.post_enable_primary = turn_on;
-
 		if (turn_off)
 			intel_crtc->atomic.disable_fbc = true;
 
@@ -13389,7 +13400,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i)
-		intel_post_plane_update(to_intel_crtc(crtc));
+		intel_post_plane_update(to_intel_crtc_state(crtc_state));
 
 	mutex_lock(&dev->struct_mutex);
 	drm_atomic_helper_cleanup_planes(dev, state);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index dc024c27ca1e..aceeeccec09b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -372,6 +372,7 @@ struct intel_crtc_state {
 #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync mode.flags */
 	unsigned long quirks;
 
+	unsigned fb_bits; /* framebuffers to flip */
 	bool update_pipe; /* can a fast modeset be performed? */
 	bool visible_changed; /* plane visibility changed */
 	bool wm_changed; /* wm changed */
@@ -539,9 +540,7 @@ struct intel_crtc_atomic_commit {
 	bool disable_fbc;
 
 	/* Sleepable operations to perform after commit */
-	unsigned fb_bits;
 	bool update_fbc;
-	bool post_enable_primary;
 };
 
 struct intel_crtc {
-- 
2.1.0

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

Re: [PATCH v2 11/14] drm/i915: Remove some post-commit members from intel_crtc->atomic.

[Ander Conselvan De Oliveira]

On Tue, 2015-11-03 at 08:31 +0100, Maarten Lankhorst wrote:
> fb_bits is useful to have in the crtc_state for cs flips later on,

Why? When is later on?

> so keep it alive there. The other stuff can be calculated in
> post_plane_update, and aren't useful elsewhere.
> 
> Currently there's a loop in post_plane_update, this should disappear
> with the changes to atomic wm's. At that point only updates to the
> primary plane are important.

Looks like the loop is gone in v2, but the commit message wasn't updated.

Ander

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +--
>  3 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index 2ba0cd698f9b..d890f5f63240 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -98,6 +98,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  	crtc_state->visible_changed = false;
>  	crtc_state->wm_changed = false;
>  	crtc_state->fb_changed = false;
> +	crtc_state->fb_bits = 0;
>  
>  	return &crtc_state->base;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 77fe1d75404b..c42e87c62133 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4742,15 +4742,20 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
>  	hsw_disable_ips(intel_crtc);
>  }
>  
> -static void intel_post_plane_update(struct intel_crtc *crtc)
> +static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
> +	struct drm_atomic_state *old_state = old_crtc_state->base.state;
>  	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc->base.state);
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_plane *primary = crtc->base.primary;
> +	struct drm_plane_state *old_pri_state =
> +		drm_atomic_get_existing_plane_state(old_state, primary);
>  
> -	intel_frontbuffer_flip(dev, atomic->fb_bits);
> +	intel_frontbuffer_flip(dev, pipe_config->fb_bits);
>  
>  	crtc->wm.cxsr_allowed = true;
>  
> @@ -4760,8 +4765,17 @@ static void intel_post_plane_update(struct intel_crtc
> *crtc)
>  	if (atomic->update_fbc)
>  		intel_fbc_update(dev_priv);
>  
> -	if (atomic->post_enable_primary)
> -		intel_post_enable_primary(&crtc->base);
> +	if (old_pri_state) {
> +		struct intel_plane_state *primary_state =
> +			to_intel_plane_state(primary->state);
> +		struct intel_plane_state *old_primary_state =
> +			to_intel_plane_state(old_pri_state);
> +
> +		if (primary_state->visible &&
> +		    (needs_modeset(&pipe_config->base) ||
> +		     !old_primary_state->visible))
> +			intel_post_enable_primary(&crtc->base);
> +	}
>  
>  	memset(atomic, 0, sizeof(*atomic));
>  }
> @@ -11685,13 +11699,10 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  	}
>  
>  	if (visible || was_visible)
> -		intel_crtc->atomic.fb_bits |=
> -			to_intel_plane(plane)->frontbuffer_bit;
> +		pipe_config->fb_bits |= to_intel_plane(plane)
> ->frontbuffer_bit;
>  
>  	switch (plane->type) {
>  	case DRM_PLANE_TYPE_PRIMARY:
> -		intel_crtc->atomic.post_enable_primary = turn_on;
> -
>  		if (turn_off)
>  			intel_crtc->atomic.disable_fbc = true;
>  
> @@ -13389,7 +13400,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i)
> -		intel_post_plane_update(to_intel_crtc(crtc));
> +		intel_post_plane_update(to_intel_crtc_state(crtc_state));
>  
>  	mutex_lock(&dev->struct_mutex);
>  	drm_atomic_helper_cleanup_planes(dev, state);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index dc024c27ca1e..aceeeccec09b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -372,6 +372,7 @@ struct intel_crtc_state {
>  #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync
> mode.flags */
>  	unsigned long quirks;
>  
> +	unsigned fb_bits; /* framebuffers to flip */
>  	bool update_pipe; /* can a fast modeset be performed? */
>  	bool visible_changed; /* plane visibility changed */
>  	bool wm_changed; /* wm changed */
> @@ -539,9 +540,7 @@ struct intel_crtc_atomic_commit {
>  	bool disable_fbc;
>  
>  	/* Sleepable operations to perform after commit */
> -	unsigned fb_bits;
>  	bool update_fbc;
> -	bool post_enable_primary;
>  };
>  
>  struct intel_crtc {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 12/14] drm/i915: Nuke fbc members from intel_crtc->atomic.

[Maarten Lankhorst]

This leaves intel_crtc->atomic empty, so zap it as well.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 84 ++++++++++++++----------------------
 drivers/gpu/drm/i915/intel_drv.h     | 16 -------
 2 files changed, 33 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c42e87c62133..ecd3169b45ef 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4746,7 +4746,6 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
 	struct drm_atomic_state *old_state = old_crtc_state->base.state;
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
@@ -4762,22 +4761,20 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 	if (pipe_config->wm_changed)
 		intel_update_watermarks(&crtc->base);
 
-	if (atomic->update_fbc)
-		intel_fbc_update(dev_priv);
-
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			to_intel_plane_state(primary->state);
 		struct intel_plane_state *old_primary_state =
 			to_intel_plane_state(old_pri_state);
 
+		if (primary_state->visible)
+			intel_fbc_update(dev_priv);
+
 		if (primary_state->visible &&
 		    (needs_modeset(&pipe_config->base) ||
 		     !old_primary_state->visible))
 			intel_post_enable_primary(&crtc->base);
 	}
-
-	memset(atomic, 0, sizeof(*atomic));
 }
 
 static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
@@ -4785,7 +4782,6 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_atomic_state *old_state = old_crtc_state->base.state;
@@ -4794,9 +4790,6 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 		drm_atomic_get_existing_plane_state(old_state, primary);
 	bool modeset = needs_modeset(&pipe_config->base);
 
-	if (atomic->disable_fbc)
-		intel_fbc_disable_crtc(crtc);
-
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			to_intel_plane_state(primary->state);
@@ -4804,8 +4797,27 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 			to_intel_plane_state(old_pri_state);
 
 		if (old_primary_state->visible &&
-		    (modeset || !primary_state->visible))
+		    (modeset || !primary_state->visible)) {
+			intel_fbc_disable_crtc(crtc);
+
 			intel_pre_disable_primary(&crtc->base);
+		} else if (primary_state->visible &&
+			 INTEL_INFO(dev_priv)->gen <= 4 && !IS_G4X(dev_priv) &&
+			 dev_priv->fbc.crtc == crtc &&
+			 primary_state->base.rotation != BIT(DRM_ROTATE_0)) {
+			/*
+			 * FBC does not work on some platforms for rotated
+			 * planes, so disable it when rotation is not 0 and
+			 * update it when rotation is set back to 0.
+			 *
+			 * FIXME: This is redundant with the fbc update done in
+			 * the primary plane enable function except that that
+			 * one is done too late. We eventually need to unify
+			 * this.
+			 */
+
+			intel_fbc_disable_crtc(crtc);
+		}
 	}
 
 	if (pipe_config->visible_changed) {
@@ -11642,7 +11654,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_plane *plane = plane_state->plane;
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane_state *old_plane_state =
 		to_intel_plane_state(plane->state);
 	int idx = intel_crtc->base.base.id, ret;
@@ -11701,46 +11712,17 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	if (visible || was_visible)
 		pipe_config->fb_bits |= to_intel_plane(plane)->frontbuffer_bit;
 
-	switch (plane->type) {
-	case DRM_PLANE_TYPE_PRIMARY:
-		if (turn_off)
-			intel_crtc->atomic.disable_fbc = true;
-
-		/*
-		 * FBC does not work on some platforms for rotated
-		 * planes, so disable it when rotation is not 0 and
-		 * update it when rotation is set back to 0.
-		 *
-		 * FIXME: This is redundant with the fbc update done in
-		 * the primary plane enable function except that that
-		 * one is done too late. We eventually need to unify
-		 * this.
-		 */
-
-		if (visible &&
-		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
-		    dev_priv->fbc.crtc == intel_crtc &&
-		    plane_state->rotation != BIT(DRM_ROTATE_0))
-			intel_crtc->atomic.disable_fbc = true;
-
-		intel_crtc->atomic.update_fbc |= visible || mode_changed;
-		break;
-	case DRM_PLANE_TYPE_CURSOR:
-		break;
-	case DRM_PLANE_TYPE_OVERLAY:
-		/*
-		 * WaCxSRDisabledForSpriteScaling:ivb
-		 *
-		 * cstate->update_wm was already set above, so this flag will
-		 * take effect when we commit and program watermarks.
-		 */
-		if (IS_IVYBRIDGE(dev) &&
-		    needs_scaling(to_intel_plane_state(plane_state)) &&
-		    !needs_scaling(old_plane_state))
-			pipe_config->disable_lp_wm = true;
+	/*
+	 * WaCxSRDisabledForSpriteScaling:ivb
+	 *
+	 * cstate->update_wm was already set above, so this flag will
+	 * take effect when we commit and program watermarks.
+	 */
+	if (plane->type == DRM_PLANE_TYPE_OVERLAY && IS_IVYBRIDGE(dev) &&
+	    needs_scaling(to_intel_plane_state(plane_state)) &&
+	    !needs_scaling(old_plane_state))
+		pipe_config->disable_lp_wm = true;
 
-		break;
-	}
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index aceeeccec09b..e2261e22642d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -529,20 +529,6 @@ struct intel_mmio_flip {
 	unsigned int rotation;
 };
 
-/*
- * Tracking of operations that need to be performed at the beginning/end of an
- * atomic commit, outside the atomic section where interrupts are disabled.
- * These are generally operations that grab mutexes or might otherwise sleep
- * and thus can't be run with interrupts disabled.
- */
-struct intel_crtc_atomic_commit {
-	/* Sleepable operations to perform before commit */
-	bool disable_fbc;
-
-	/* Sleepable operations to perform after commit */
-	bool update_fbc;
-};
-
 struct intel_crtc {
 	struct drm_crtc base;
 	enum pipe pipe;
@@ -603,8 +589,6 @@ struct intel_crtc {
 		int scanline_start;
 	} debug;
 
-	struct intel_crtc_atomic_commit atomic;
-
 	/* scalers available on this crtc */
 	int num_scalers;
 
-- 
2.1.0

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

Re: [PATCH v2 12/14] drm/i915: Nuke fbc members from intel_crtc->atomic.

[Ander Conselvan De Oliveira]

On Tue, 2015-11-03 at 08:31 +0100, Maarten Lankhorst wrote:
> This leaves intel_crtc->atomic empty, so zap it as well.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 84 ++++++++++++++---------------------
> -
>  drivers/gpu/drm/i915/intel_drv.h     | 16 -------
>  2 files changed, 33 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index c42e87c62133..ecd3169b45ef 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4746,7 +4746,6 @@ static void intel_post_plane_update(struct
> intel_crtc_state *old_crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
>  	struct drm_atomic_state *old_state = old_crtc_state->base.state;
> -	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc->base.state);
>  	struct drm_device *dev = crtc->base.dev;
> @@ -4762,22 +4761,20 @@ static void intel_post_plane_update(struct
> intel_crtc_state *old_crtc_state)
>  	if (pipe_config->wm_changed)
>  		intel_update_watermarks(&crtc->base);
>  
> -	if (atomic->update_fbc)
> -		intel_fbc_update(dev_priv);
> -
>  	if (old_pri_state) {
>  		struct intel_plane_state *primary_state =
>  			to_intel_plane_state(primary->state);
>  		struct intel_plane_state *old_primary_state =
>  			to_intel_plane_state(old_pri_state);
>  
> +		if (primary_state->visible)
> +			intel_fbc_update(dev_priv);
> +
>  		if (primary_state->visible &&
>  		    (needs_modeset(&pipe_config->base) ||
>  		     !old_primary_state->visible))
>  			intel_post_enable_primary(&crtc->base);
>  	}
> -
> -	memset(atomic, 0, sizeof(*atomic));
>  }
>  
>  static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
> @@ -4785,7 +4782,6 @@ static void intel_pre_plane_update(struct
> intel_crtc_state *old_crtc_state)
>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc->base.state);
>  	struct drm_atomic_state *old_state = old_crtc_state->base.state;
> @@ -4794,9 +4790,6 @@ static void intel_pre_plane_update(struct
> intel_crtc_state *old_crtc_state)
>  		drm_atomic_get_existing_plane_state(old_state, primary);
>  	bool modeset = needs_modeset(&pipe_config->base);
>  
> -	if (atomic->disable_fbc)
> -		intel_fbc_disable_crtc(crtc);
> -
>  	if (old_pri_state) {
>  		struct intel_plane_state *primary_state =
>  			to_intel_plane_state(primary->state);
> @@ -4804,8 +4797,27 @@ static void intel_pre_plane_update(struct
> intel_crtc_state *old_crtc_state)
>  			to_intel_plane_state(old_pri_state);
>  
>  		if (old_primary_state->visible &&
> -		    (modeset || !primary_state->visible))
> +		    (modeset || !primary_state->visible)) {
> +			intel_fbc_disable_crtc(crtc);
> +
>  			intel_pre_disable_primary(&crtc->base);
> +		} else if (primary_state->visible &&
> +			 INTEL_INFO(dev_priv)->gen <= 4 && !IS_G4X(dev_priv)
> &&
> +			 dev_priv->fbc.crtc == crtc &&
> +			 primary_state->base.rotation != BIT(DRM_ROTATE_0)) {
> +			/*
> +			 * FBC does not work on some platforms for rotated
> +			 * planes, so disable it when rotation is not 0 and
> +			 * update it when rotation is set back to 0.

The comment gets a bit confusing now, since the update call is done in
intel_post_plane_update() it is a bit difficult to make the connection.

> +			 *
> +			 * FIXME: This is redundant with the fbc update done
> in
> +			 * the primary plane enable function except that that
> +			 * one is done too late. We eventually need to unify
> +			 * this.
> +			 */
> +
> +			intel_fbc_disable_crtc(crtc);
> +		}

I think having all this fbc logic here makes this function hard to read. I'd
suggest moving the fbc rotation to a bool function, and then this could be
written like this:

	bool turn_off = old_primary_state->visible &&
		(modeset || !primary_state->visible);

	if (turn_off || !fbc_supports_rotation(...))
		intel_fbc_disable_crtc(crtc);

	if (turn_off)
		intel_pre_disable_primary(...);

Would look a bit cleaner, IMO.

Ander

>  	}
>  
>  	if (pipe_config->visible_changed) {
> @@ -11642,7 +11654,6 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_plane *plane = plane_state->plane;
>  	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane_state *old_plane_state =
>  		to_intel_plane_state(plane->state);
>  	int idx = intel_crtc->base.base.id, ret;
> @@ -11701,46 +11712,17 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  	if (visible || was_visible)
>  		pipe_config->fb_bits |= to_intel_plane(plane)
> ->frontbuffer_bit;
>  
> -	switch (plane->type) {
> -	case DRM_PLANE_TYPE_PRIMARY:
> -		if (turn_off)
> -			intel_crtc->atomic.disable_fbc = true;
> -
> -		/*
> -		 * FBC does not work on some platforms for rotated
> -		 * planes, so disable it when rotation is not 0 and
> -		 * update it when rotation is set back to 0.
> -		 *
> -		 * FIXME: This is redundant with the fbc update done in
> -		 * the primary plane enable function except that that
> -		 * one is done too late. We eventually need to unify
> -		 * this.
> -		 */
> -
> -		if (visible &&
> -		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> -		    dev_priv->fbc.crtc == intel_crtc &&
> -		    plane_state->rotation != BIT(DRM_ROTATE_0))
> -			intel_crtc->atomic.disable_fbc = true;
> -
> -		intel_crtc->atomic.update_fbc |= visible || mode_changed;
> -		break;
> -	case DRM_PLANE_TYPE_CURSOR:
> -		break;
> -	case DRM_PLANE_TYPE_OVERLAY:
> -		/*
> -		 * WaCxSRDisabledForSpriteScaling:ivb
> -		 *
> -		 * cstate->update_wm was already set above, so this flag will
> -		 * take effect when we commit and program watermarks.
> -		 */
> -		if (IS_IVYBRIDGE(dev) &&
> -		    needs_scaling(to_intel_plane_state(plane_state)) &&
> -		    !needs_scaling(old_plane_state))
> -			pipe_config->disable_lp_wm = true;
> +	/*
> +	 * WaCxSRDisabledForSpriteScaling:ivb
> +	 *
> +	 * cstate->update_wm was already set above, so this flag will
> +	 * take effect when we commit and program watermarks.
> +	 */
> +	if (plane->type == DRM_PLANE_TYPE_OVERLAY && IS_IVYBRIDGE(dev) &&
> +	    needs_scaling(to_intel_plane_state(plane_state)) &&
> +	    !needs_scaling(old_plane_state))
> +		pipe_config->disable_lp_wm = true;
>  
> -		break;
> -	}
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index aceeeccec09b..e2261e22642d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -529,20 +529,6 @@ struct intel_mmio_flip {
>  	unsigned int rotation;
>  };
>  
> -/*
> - * Tracking of operations that need to be performed at the beginning/end of
> an
> - * atomic commit, outside the atomic section where interrupts are disabled.
> - * These are generally operations that grab mutexes or might otherwise sleep
> - * and thus can't be run with interrupts disabled.
> - */
> -struct intel_crtc_atomic_commit {
> -	/* Sleepable operations to perform before commit */
> -	bool disable_fbc;
> -
> -	/* Sleepable operations to perform after commit */
> -	bool update_fbc;
> -};
> -
>  struct intel_crtc {
>  	struct drm_crtc base;
>  	enum pipe pipe;
> @@ -603,8 +589,6 @@ struct intel_crtc {
>  		int scanline_start;
>  	} debug;
>  
> -	struct intel_crtc_atomic_commit atomic;
> -
>  	/* scalers available on this crtc */
>  	int num_scalers;
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 13/14] drm/i915/skl: Update watermarks before the crtc is disabled.

[Maarten Lankhorst]

On skylake some of the registers are only writable when the correct
power wells are enabled. Because of this watermarks have to be updated
before the crtc turns off, or you get unclaimed register read and write
warnings.

This patch needs to be modified slightly to apply to -fixes.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92181
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: stable@vger.kernel.org
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ecd3169b45ef..304e1028c9a4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4758,7 +4758,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 
 	crtc->wm.cxsr_allowed = true;
 
-	if (pipe_config->wm_changed)
+	if (pipe_config->wm_changed && pipe_config->base.active)
 		intel_update_watermarks(&crtc->base);
 
 	if (old_pri_state) {
@@ -13327,6 +13327,9 @@ static int intel_atomic_commit(struct drm_device *dev,
 			dev_priv->display.crtc_disable(crtc);
 			intel_crtc->active = false;
 			intel_disable_shared_dpll(intel_crtc);
+
+			if (!crtc->state->active)
+				intel_update_watermarks(crtc);
 		}
 	}
 
-- 
2.1.0

[PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Maarten Lankhorst]

This fixes a warning when the crtc is turned off. In that case fb
will be NULL, and crtc_clock will be 0. Because the crtc is no longer
active this is not a bug, and shouldn't trigger the WARN_ON.

Also remove handling a null crtc_state, with all transitional helpers
gone this can no longer happen.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 304e1028c9a4..7e2caeef9a11 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13671,7 +13671,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state
 	struct drm_i915_private *dev_priv;
 	int crtc_clock, cdclk;
 
-	if (!intel_crtc || !crtc_state)
+	if (!intel_crtc || !crtc_state->base.active)
 		return DRM_PLANE_HELPER_NO_SCALING;
 
 	dev = intel_crtc->base.dev;
-- 
2.1.0

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

Re: [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Ville Syrjälä]

On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote:
> This fixes a warning when the crtc is turned off. In that case fb
> will be NULL, and crtc_clock will be 0. Because the crtc is no longer
> active this is not a bug, and shouldn't trigger the WARN_ON.

Mm. We want to do scaling checks and whatnot during DPMS. So this should
check .enabled, no?

> Also remove handling a null crtc_state, with all transitional helpers
> gone this can no longer happen.

What about the !intel_crtc check, how is that supposed to happen?

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 304e1028c9a4..7e2caeef9a11 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13671,7 +13671,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state
>  	struct drm_i915_private *dev_priv;
>  	int crtc_clock, cdclk;
>  
> -	if (!intel_crtc || !crtc_state)
> +	if (!intel_crtc || !crtc_state->base.active)
>  		return DRM_PLANE_HELPER_NO_SCALING;
>  
>  	dev = intel_crtc->base.dev;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Maarten Lankhorst]

Op 03-11-15 om 10:09 schreef Ville Syrjälä:
> On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote:
>> This fixes a warning when the crtc is turned off. In that case fb
>> will be NULL, and crtc_clock will be 0. Because the crtc is no longer
>> active this is not a bug, and shouldn't trigger the WARN_ON.
> Mm. We want to do scaling checks and whatnot during DPMS. So this should
> check .enabled, no?
Not sure what the right decision would be here..

     * skl max scale is lower of:
     *    close to 3 but not 3, -1 is for that purpose
     *            or
     *    cdclk/crtc_clock

So when multiple pipes are enabled potentially 3x scaling is allowed, but if you dpms them all off
cdclk might get set to 0. This means a previous valid amount of scaling might suddenly become invalid.

Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting.

This probably needs to be addressed in patch 3/14 then, so that one needs more love.

I will resend patch 3, 4, 5 and 14 as a separate series. The other patches from this series will still work with some easily fixed rejects.
>> Also remove handling a null crtc_state, with all transitional helpers
>> gone this can no longer happen.
> What about the !intel_crtc check, how is that supposed to happen?
When fb == NULL. :)
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 304e1028c9a4..7e2caeef9a11 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13671,7 +13671,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state
>>  	struct drm_i915_private *dev_priv;
>>  	int crtc_clock, cdclk;
>>  
>> -	if (!intel_crtc || !crtc_state)
>> +	if (!intel_crtc || !crtc_state->base.active)
>>  		return DRM_PLANE_HELPER_NO_SCALING;
>>  
>>  	dev = intel_crtc->base.dev;
>> -- 
>> 2.1.0
>>
>> _______________________________________________
>> 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 v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Ville Syrjälä]

On Tue, Nov 03, 2015 at 11:06:53AM +0100, Maarten Lankhorst wrote:
> Op 03-11-15 om 10:09 schreef Ville Syrjälä:
> > On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote:
> >> This fixes a warning when the crtc is turned off. In that case fb
> >> will be NULL, and crtc_clock will be 0. Because the crtc is no longer
> >> active this is not a bug, and shouldn't trigger the WARN_ON.
> > Mm. We want to do scaling checks and whatnot during DPMS. So this should
> > check .enabled, no?
> Not sure what the right decision would be here..
> 
>      * skl max scale is lower of:
>      *    close to 3 but not 3, -1 is for that purpose
>      *            or
>      *    cdclk/crtc_clock
> 
> So when multiple pipes are enabled potentially 3x scaling is allowed, but if you dpms them all off
> cdclk might get set to 0. This means a previous valid amount of scaling might suddenly become invalid.
> 
> Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting.

I think we should keep around the min required cdclk in the crtc state.
And we recalculate that one every time anything changes. Then we can
just compare it against the current cdclk after plane/crtc states have
otherwise been computed to see if we need to change the current cdclk.

> 
> This probably needs to be addressed in patch 3/14 then, so that one needs more love.
> 
> I will resend patch 3, 4, 5 and 14 as a separate series. The other patches from this series will still work with some easily fixed rejects.
> >> Also remove handling a null crtc_state, with all transitional helpers
> >> gone this can no longer happen.
> > What about the !intel_crtc check, how is that supposed to happen?
> When fb == NULL. :)
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 304e1028c9a4..7e2caeef9a11 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -13671,7 +13671,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state
> >>  	struct drm_i915_private *dev_priv;
> >>  	int crtc_clock, cdclk;
> >>  
> >> -	if (!intel_crtc || !crtc_state)
> >> +	if (!intel_crtc || !crtc_state->base.active)
> >>  		return DRM_PLANE_HELPER_NO_SCALING;
> >>  
> >>  	dev = intel_crtc->base.dev;
> >> -- 
> >> 2.1.0
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Maarten Lankhorst]

Op 03-11-15 om 11:40 schreef Ville Syrjälä:
> On Tue, Nov 03, 2015 at 11:06:53AM +0100, Maarten Lankhorst wrote:
>> Op 03-11-15 om 10:09 schreef Ville Syrjälä:
>>> On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote:
>>>> This fixes a warning when the crtc is turned off. In that case fb
>>>> will be NULL, and crtc_clock will be 0. Because the crtc is no longer
>>>> active this is not a bug, and shouldn't trigger the WARN_ON.
>>> Mm. We want to do scaling checks and whatnot during DPMS. So this should
>>> check .enabled, no?
>> Not sure what the right decision would be here..
>>
>>      * skl max scale is lower of:
>>      *    close to 3 but not 3, -1 is for that purpose
>>      *            or
>>      *    cdclk/crtc_clock
>>
>> So when multiple pipes are enabled potentially 3x scaling is allowed, but if you dpms them all off
>> cdclk might get set to 0. This means a previous valid amount of scaling might suddenly become invalid.
>>
>> Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting.
> I think we should keep around the min required cdclk in the crtc state.
> And we recalculate that one every time anything changes. Then we can
> just compare it against the current cdclk after plane/crtc states have
> otherwise been computed to see if we need to change the current cdclk.
>
What would the use be here? In that case the above formula reduces to 1<<16.
If you want to treat dpms off the same as dpms on then you need the separate cdclk's..
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Ville Syrjälä]

On Tue, Nov 03, 2015 at 01:00:32PM +0100, Maarten Lankhorst wrote:
> Op 03-11-15 om 11:40 schreef Ville Syrjälä:
> > On Tue, Nov 03, 2015 at 11:06:53AM +0100, Maarten Lankhorst wrote:
> >> Op 03-11-15 om 10:09 schreef Ville Syrjälä:
> >>> On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote:
> >>>> This fixes a warning when the crtc is turned off. In that case fb
> >>>> will be NULL, and crtc_clock will be 0. Because the crtc is no longer
> >>>> active this is not a bug, and shouldn't trigger the WARN_ON.
> >>> Mm. We want to do scaling checks and whatnot during DPMS. So this should
> >>> check .enabled, no?
> >> Not sure what the right decision would be here..
> >>
> >>      * skl max scale is lower of:
> >>      *    close to 3 but not 3, -1 is for that purpose
> >>      *            or
> >>      *    cdclk/crtc_clock
> >>
> >> So when multiple pipes are enabled potentially 3x scaling is allowed, but if you dpms them all off
> >> cdclk might get set to 0. This means a previous valid amount of scaling might suddenly become invalid.
> >>
> >> Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting.
> > I think we should keep around the min required cdclk in the crtc state.
> > And we recalculate that one every time anything changes. Then we can
> > just compare it against the current cdclk after plane/crtc states have
> > otherwise been computed to see if we need to change the current cdclk.
> >
> What would the use be here? In that case the above formula reduces to 1<<16.
> If you want to treat dpms off the same as dpms on then you need the separate cdclk's..

I don't understand what you're saying. We need to calculate the required
cdclk based on dpms on. And that is what we must check against the
hardware limit. If we then want to optimize the dpms off case we can
just do 'cdclk = state->active ? state->cdclk : 0'. We probably want to
do that when all pipes are in dpms off. But if we have some pipes in
dpms off and some in dpms on, we may want to skip the 'active' check
for all pipes (ie. behave as all pipes are dpms on), just to avoid flicker
when some of the remaining pipes transition from dpms off to dpms on.

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

Re: [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Maarten Lankhorst]

Op 03-11-15 om 14:11 schreef Ville Syrjälä:
> On Tue, Nov 03, 2015 at 01:00:32PM +0100, Maarten Lankhorst wrote:
>> Op 03-11-15 om 11:40 schreef Ville Syrjälä:
>>> On Tue, Nov 03, 2015 at 11:06:53AM +0100, Maarten Lankhorst wrote:
>>>> Op 03-11-15 om 10:09 schreef Ville Syrjälä:
>>>>> On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote:
>>>>>> This fixes a warning when the crtc is turned off. In that case fb
>>>>>> will be NULL, and crtc_clock will be 0. Because the crtc is no longer
>>>>>> active this is not a bug, and shouldn't trigger the WARN_ON.
>>>>> Mm. We want to do scaling checks and whatnot during DPMS. So this should
>>>>> check .enabled, no?
>>>> Not sure what the right decision would be here..
>>>>
>>>>      * skl max scale is lower of:
>>>>      *    close to 3 but not 3, -1 is for that purpose
>>>>      *            or
>>>>      *    cdclk/crtc_clock
>>>>
>>>> So when multiple pipes are enabled potentially 3x scaling is allowed, but if you dpms them all off
>>>> cdclk might get set to 0. This means a previous valid amount of scaling might suddenly become invalid.
>>>>
>>>> Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting.
>>> I think we should keep around the min required cdclk in the crtc state.
>>> And we recalculate that one every time anything changes. Then we can
>>> just compare it against the current cdclk after plane/crtc states have
>>> otherwise been computed to see if we need to change the current cdclk.
>>>
>> What would the use be here? In that case the above formula reduces to 1<<16.
>> If you want to treat dpms off the same as dpms on then you need the separate cdclk's..
> I don't understand what you're saying. We need to calculate the required
> cdclk based on dpms on. And that is what we must check against the
> hardware limit. If we then want to optimize the dpms off case we can
> just do 'cdclk = state->active ? state->cdclk : 0'. We probably want to
> do that when all pipes are in dpms off. But if we have some pipes in
> dpms off and some in dpms on, we may want to skip the 'active' check
> for all pipes (ie. behave as all pipes are dpms on), just to avoid flicker
> when some of the remaining pipes transition from dpms off to dpms on.
Yeah but this would break things, or we'd need to keep 2 cdclk freqs globally.
One with the real frequency, the other one used for calculations in atomic.
And only when all crtc's are disabled they would differ.

In any case, when a plane is DPMS off this code won't be useful, visible = false
because the rectangle will get clipped to (0,0)x(0,0).

Saying no scaling is possible would be fine here I think, without side effects.

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

Re: [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Ville Syrjälä]

On Tue, Nov 17, 2015 at 02:59:35PM +0100, Maarten Lankhorst wrote:
> Op 03-11-15 om 14:11 schreef Ville Syrjälä:
> > On Tue, Nov 03, 2015 at 01:00:32PM +0100, Maarten Lankhorst wrote:
> >> Op 03-11-15 om 11:40 schreef Ville Syrjälä:
> >>> On Tue, Nov 03, 2015 at 11:06:53AM +0100, Maarten Lankhorst wrote:
> >>>> Op 03-11-15 om 10:09 schreef Ville Syrjälä:
> >>>>> On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote:
> >>>>>> This fixes a warning when the crtc is turned off. In that case fb
> >>>>>> will be NULL, and crtc_clock will be 0. Because the crtc is no longer
> >>>>>> active this is not a bug, and shouldn't trigger the WARN_ON.
> >>>>> Mm. We want to do scaling checks and whatnot during DPMS. So this should
> >>>>> check .enabled, no?
> >>>> Not sure what the right decision would be here..
> >>>>
> >>>>      * skl max scale is lower of:
> >>>>      *    close to 3 but not 3, -1 is for that purpose
> >>>>      *            or
> >>>>      *    cdclk/crtc_clock
> >>>>
> >>>> So when multiple pipes are enabled potentially 3x scaling is allowed, but if you dpms them all off
> >>>> cdclk might get set to 0. This means a previous valid amount of scaling might suddenly become invalid.
> >>>>
> >>>> Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting.
> >>> I think we should keep around the min required cdclk in the crtc state.
> >>> And we recalculate that one every time anything changes. Then we can
> >>> just compare it against the current cdclk after plane/crtc states have
> >>> otherwise been computed to see if we need to change the current cdclk.
> >>>
> >> What would the use be here? In that case the above formula reduces to 1<<16.
> >> If you want to treat dpms off the same as dpms on then you need the separate cdclk's..
> > I don't understand what you're saying. We need to calculate the required
> > cdclk based on dpms on. And that is what we must check against the
> > hardware limit. If we then want to optimize the dpms off case we can
> > just do 'cdclk = state->active ? state->cdclk : 0'. We probably want to
> > do that when all pipes are in dpms off. But if we have some pipes in
> > dpms off and some in dpms on, we may want to skip the 'active' check
> > for all pipes (ie. behave as all pipes are dpms on), just to avoid flicker
> > when some of the remaining pipes transition from dpms off to dpms on.
> Yeah but this would break things, or we'd need to keep 2 cdclk freqs globally.
> One with the real frequency, the other one used for calculations in atomic.
> And only when all crtc's are disabled they would differ.
> 
> In any case, when a plane is DPMS off this code won't be useful, visible = false
> because the rectangle will get clipped to (0,0)x(0,0).

And that must be changed.

> 
> Saying no scaling is possible would be fine here I think, without side effects.

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