[PATCH 00/11] Kill off intel_crtc->atomic!

[PATCH 00/11] Kill off intel_crtc->atomic!

[Maarten Lankhorst]

While converting page flip to atomic I've noticed it was easier to kill
off intel_crtc->atomic first. This can be done by adding fb_bits,
visible_changed, wm_changed to the crtc_state and deriving some
primary state during pre/post_plane_update.

This will probably conflict with the atomic wm work, and may conflict slightly
with the fbc rework, but those conflicts shouldn't be too hard to fix up.

After fixing that I've ran some tests and noticed a few SKL bugs, related to
WM's and scaling, so I've fixed those up at the end.

Maarten Lankhorst (11):
  drm/i915: Use passed plane state for sprite planes.
  drm/i915: Do not acquire crtc state to check clock during modeset.
  drm/i915: Kill off intel_crtc->atomic.wait_vblank.
  drm/i915: Update watermark related members in the crtc_state.
  drm/i915: Remove intel_crtc->atomic.disable_ips.
  drm/i915: Remove atomic.pre_disable_primary.
  drm/i915: Remove some post-commit members from intel_crtc->atomic.
  drm/i915: Nuke fbc members from intel_crtc->atomic.
  drm/i915/skl: Prevent unclaimed register writes on skylake.
  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/intel_atomic.c  |   3 +
 drivers/gpu/drm/i915/intel_display.c | 319 +++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_drv.h     |  46 ++---
 drivers/gpu/drm/i915/intel_pm.c      |   5 +
 drivers/gpu/drm/i915/intel_sprite.c  |  67 +++++---
 5 files changed, 224 insertions(+), 216 deletions(-)

-- 
2.1.0

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

[PATCH 01/11] drm/i915: Use passed plane state for sprite planes.

[Maarten Lankhorst]

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

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

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4e35557bbd41..51722e657b91 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -631,16 +631,16 @@ 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,
+			     struct intel_plane_state *plane_state,
 			     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);
+			     uint32_t src_w, uint32_t src_h,
+			     struct intel_scaler *scaler);
 	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 2551335ada04..599d001b1415 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -178,16 +178,19 @@ 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,
+skl_update_plane(struct drm_plane *drm_plane,
+		 struct intel_plane_state *plane_state,
 		 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)
+		 uint32_t src_w, uint32_t src_h,
+		 struct intel_scaler *scaler)
 {
 	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_crtc *crtc = plane_state->base.crtc;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
@@ -199,8 +202,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	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;
 
 	plane_ctl = PLANE_CTL_ENABLE |
 		PLANE_CTL_PIPE_GAMMA_ENABLE |
@@ -219,8 +220,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	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--;
@@ -261,13 +260,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 (scaler) {
 		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);
@@ -341,16 +340,18 @@ 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,
+vlv_update_plane(struct drm_plane *dplane,
+		 struct intel_plane_state *plane_state,
 		 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)
+		 uint32_t src_w, uint32_t src_h,
+		 struct intel_scaler *scaler)
 {
 	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;
@@ -481,16 +482,19 @@ 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,
+ivb_update_plane(struct drm_plane *plane,
+		 struct intel_plane_state *plane_state,
 		 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)
+		 uint32_t src_w, uint32_t src_h,
+		 struct intel_scaler *scaler)
 {
 	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_crtc *crtc = plane_state->base.crtc;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	enum pipe pipe = intel_plane->pipe;
 	u32 sprctl, sprscale = 0;
@@ -623,16 +627,19 @@ 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,
+ilk_update_plane(struct drm_plane *plane,
+		 struct intel_plane_state *plane_state,
 		 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)
+		 uint32_t src_w, uint32_t src_h,
+		 struct intel_scaler *scaler)
 {
 	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_crtc *crtc = plane_state->base.crtc;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	int pipe = intel_plane->pipe;
 	unsigned long dvssurf_offset, linear_offset;
@@ -932,23 +939,31 @@ 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,
+		struct intel_scaler *scaler = NULL;
+
+		if (state->scaler_id >= 0) {
+			struct intel_crtc_state *crtc_state =
+				to_intel_crtc(state->base.crtc)->config;
+
+			scaler = &crtc_state->scaler_state.scalers[state->scaler_id];
+		}
+
+		intel_plane->update_plane(plane, state,
 					  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);
+					  drm_rect_height(&state->src) >> 16,
+					  scaler);
 	} 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 01/11] drm/i915: Use passed plane state for sprite planes.

[Daniel Vetter]

On Thu, Oct 22, 2015 at 01:56:26PM +0200, Maarten Lankhorst wrote:
> Don't use plane->state directly, use the pointer from commit_plane.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h    |  8 ++---
>  drivers/gpu/drm/i915/intel_sprite.c | 67 +++++++++++++++++++++++--------------
>  2 files changed, 45 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4e35557bbd41..51722e657b91 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -631,16 +631,16 @@ 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,
> +			     struct intel_plane_state *plane_state,
>  			     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);
> +			     uint32_t src_w, uint32_t src_h,
> +			     struct intel_scaler *scaler);

This is an ugly hack imo. Better to add the scaler pointer to the plane
state, set it in compute_config once we're done and not pass it around
like this. Or we also pass around a pointer to the right crtc_state.

Also, if we pass around plane_state I think all the crtc_x/y/w/h and x/y
and src_x/y/w/h should disappear, for a really clean vfunc interface.
-Daniel

>  	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 2551335ada04..599d001b1415 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -178,16 +178,19 @@ 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,
> +skl_update_plane(struct drm_plane *drm_plane,
> +		 struct intel_plane_state *plane_state,
>  		 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)
> +		 uint32_t src_w, uint32_t src_h,
> +		 struct intel_scaler *scaler)
>  {
>  	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_crtc *crtc = plane_state->base.crtc;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
> @@ -199,8 +202,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	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;
>  
>  	plane_ctl = PLANE_CTL_ENABLE |
>  		PLANE_CTL_PIPE_GAMMA_ENABLE |
> @@ -219,8 +220,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	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--;
> @@ -261,13 +260,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 (scaler) {
>  		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);
> @@ -341,16 +340,18 @@ 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,
> +vlv_update_plane(struct drm_plane *dplane,
> +		 struct intel_plane_state *plane_state,
>  		 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)
> +		 uint32_t src_w, uint32_t src_h,
> +		 struct intel_scaler *scaler)
>  {
>  	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;
> @@ -481,16 +482,19 @@ 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,
> +ivb_update_plane(struct drm_plane *plane,
> +		 struct intel_plane_state *plane_state,
>  		 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)
> +		 uint32_t src_w, uint32_t src_h,
> +		 struct intel_scaler *scaler)
>  {
>  	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_crtc *crtc = plane_state->base.crtc;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	enum pipe pipe = intel_plane->pipe;
>  	u32 sprctl, sprscale = 0;
> @@ -623,16 +627,19 @@ 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,
> +ilk_update_plane(struct drm_plane *plane,
> +		 struct intel_plane_state *plane_state,
>  		 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)
> +		 uint32_t src_w, uint32_t src_h,
> +		 struct intel_scaler *scaler)
>  {
>  	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_crtc *crtc = plane_state->base.crtc;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	int pipe = intel_plane->pipe;
>  	unsigned long dvssurf_offset, linear_offset;
> @@ -932,23 +939,31 @@ 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,
> +		struct intel_scaler *scaler = NULL;
> +
> +		if (state->scaler_id >= 0) {
> +			struct intel_crtc_state *crtc_state =
> +				to_intel_crtc(state->base.crtc)->config;
> +
> +			scaler = &crtc_state->scaler_state.scalers[state->scaler_id];
> +		}
> +
> +		intel_plane->update_plane(plane, state,
>  					  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);
> +					  drm_rect_height(&state->src) >> 16,
> +					  scaler);
>  	} 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/11] drm/i915: Use passed plane state for sprite planes.

[Maarten Lankhorst]

Op 22-10-15 om 14:58 schreef Daniel Vetter:
> On Thu, Oct 22, 2015 at 01:56:26PM +0200, Maarten Lankhorst wrote:
>> Don't use plane->state directly, use the pointer from commit_plane.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_drv.h    |  8 ++---
>>  drivers/gpu/drm/i915/intel_sprite.c | 67 +++++++++++++++++++++++--------------
>>  2 files changed, 45 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 4e35557bbd41..51722e657b91 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -631,16 +631,16 @@ 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,
>> +			     struct intel_plane_state *plane_state,
>>  			     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);
>> +			     uint32_t src_w, uint32_t src_h,
>> +			     struct intel_scaler *scaler);
> This is an ugly hack imo. Better to add the scaler pointer to the plane
> state, set it in compute_config once we're done and not pass it around
> like this. Or we also pass around a pointer to the right crtc_state.
This won't be valid if the crtc_state is updated after the page flip, so I don't think it's a
good idea to add it to the state. But we could pass the crtc_state here and nuke the rest.

> Also, if we pass around plane_state I think all the crtc_x/y/w/h and x/y
> and src_x/y/w/h should disappear, for a really clean vfunc interface.
Agreed.

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

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

[Maarten Lankhorst]

Op 22-10-15 om 14:58 schreef Daniel Vetter:
> On Thu, Oct 22, 2015 at 01:56:26PM +0200, Maarten Lankhorst wrote:
>> Don't use plane->state directly, use the pointer from commit_plane.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_drv.h    |  8 ++---
>>  drivers/gpu/drm/i915/intel_sprite.c | 67 +++++++++++++++++++++++--------------
>>  2 files changed, 45 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 4e35557bbd41..51722e657b91 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -631,16 +631,16 @@ 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,
>> +			     struct intel_plane_state *plane_state,
>>  			     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);
>> +			     uint32_t src_w, uint32_t src_h,
>> +			     struct intel_scaler *scaler);
> This is an ugly hack imo. Better to add the scaler pointer to the plane
> state, set it in compute_config once we're done and not pass it around
> like this. Or we also pass around a pointer to the right crtc_state.
>
> Also, if we pass around plane_state I think all the crtc_x/y/w/h and x/y
> and src_x/y/w/h should disappear, for a really clean vfunc interface.

How about this? With --scissors

Maybe the comment in intel_drv.h can be removed now too, since
everything is passed through plane_state now.

----->8-------
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.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4e35557bbd41..682f70f95e17 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -631,16 +631,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 2551335ada04..1811b8bcae94 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -178,29 +178,34 @@ 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;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
-	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;
+
+	struct drm_crtc *crtc = crtc_state->base.crtc;
+	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 |
@@ -209,7 +214,7 @@ 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);
 
 	intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h,
@@ -219,8 +224,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	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--;
@@ -261,13 +264,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);
@@ -341,24 +344,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;
 
@@ -428,7 +435,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;
@@ -481,23 +488,28 @@ 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;
+
+	struct drm_crtc *crtc = crtc_state->base.crtc;
+	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;
 
@@ -561,7 +573,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 */
@@ -623,23 +635,28 @@ 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;
+
+	struct drm_crtc *crtc = crtc_state->base.crtc;
+	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;
 
@@ -699,7 +716,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;
@@ -932,23 +949,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);
 	}
 }
 

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

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

[Daniel Vetter]

On Mon, Oct 26, 2015 at 10:30:22AM +0100, Maarten Lankhorst wrote:
> Op 22-10-15 om 14:58 schreef Daniel Vetter:
> > On Thu, Oct 22, 2015 at 01:56:26PM +0200, Maarten Lankhorst wrote:
> >> Don't use plane->state directly, use the pointer from commit_plane.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_drv.h    |  8 ++---
> >>  drivers/gpu/drm/i915/intel_sprite.c | 67 +++++++++++++++++++++++--------------
> >>  2 files changed, 45 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 4e35557bbd41..51722e657b91 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -631,16 +631,16 @@ 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,
> >> +			     struct intel_plane_state *plane_state,
> >>  			     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);
> >> +			     uint32_t src_w, uint32_t src_h,
> >> +			     struct intel_scaler *scaler);
> > This is an ugly hack imo. Better to add the scaler pointer to the plane
> > state, set it in compute_config once we're done and not pass it around
> > like this. Or we also pass around a pointer to the right crtc_state.
> >
> > Also, if we pass around plane_state I think all the crtc_x/y/w/h and x/y
> > and src_x/y/w/h should disappear, for a really clean vfunc interface.
> 
> How about this? With --scissors
> 
> Maybe the comment in intel_drv.h can be removed now too, since
> everything is passed through plane_state now.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Somewhen we need to unconfuse our mess of drm_rect vs. explicit
coordinates ...
-Daniel


> ----->8-------
> 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.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4e35557bbd41..682f70f95e17 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -631,16 +631,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 2551335ada04..1811b8bcae94 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -178,29 +178,34 @@ 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;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> -	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;
> +
> +	struct drm_crtc *crtc = crtc_state->base.crtc;
> +	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 |
> @@ -209,7 +214,7 @@ 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);
>  
>  	intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h,
> @@ -219,8 +224,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	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--;
> @@ -261,13 +264,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);
> @@ -341,24 +344,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;
>  
> @@ -428,7 +435,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;
> @@ -481,23 +488,28 @@ 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;
> +
> +	struct drm_crtc *crtc = crtc_state->base.crtc;
> +	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;
>  
> @@ -561,7 +573,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 */
> @@ -623,23 +635,28 @@ 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;
> +
> +	struct drm_crtc *crtc = crtc_state->base.crtc;
> +	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;
>  
> @@ -699,7 +716,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;
> @@ -932,23 +949,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);
>  	}
>  }
>  
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/11] drm/i915: Do not acquire crtc state to check clock during modeset.

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

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 022e628b8520..051a1e2b1c55 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5237,6 +5237,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] = {};
@@ -5245,13 +5246,20 @@ 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;
+
+		intel_crtc->min_cdclk = intel_state->pipe_cdclk[pipe];
+
+		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))
@@ -5919,7 +5927,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;
@@ -5929,8 +5936,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
@@ -5939,22 +5948,31 @@ 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;
+	int max_pixel_rate = 0;
+	bool any_active = false;
 
-	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);
+	for_each_intel_crtc(state->dev, intel_crtc) {
+		struct drm_crtc_state *drm_crtc_state;
+		struct drm_crtc *crtc = &intel_crtc->base;
+		int pixclk = 0;
 
-		if (!crtc_state->base.enable)
-			continue;
+		drm_crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
+		if (!drm_crtc_state) {
+			any_active |= intel_crtc->active;
+			pixclk = intel_crtc->min_cdclk;
+		} else if (drm_crtc_state->enable) {
+			struct intel_crtc_state *crtc_state =
+				to_intel_crtc_state(drm_crtc_state);
+
+			any_active |= drm_crtc_state->active;
+			pixclk = crtc_state->base.adjusted_mode.crtc_clock;
+		}
 
-		max_pixclk = max(max_pixclk,
-				 crtc_state->base.adjusted_mode.crtc_clock);
+		to_intel_atomic_state(state)->pipe_cdclk[intel_crtc->pipe] = pixclk;
+		max_pixel_rate = max(max_pixel_rate, pixclk);
 	}
 
-	return max_pixclk;
+	return any_active ? max_pixel_rate : 0;
 }
 
 static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
@@ -9491,29 +9509,35 @@ static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state)
 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;
+	bool any_active = false;
 
 	for_each_intel_crtc(state->dev, intel_crtc) {
-		int pixel_rate;
-
-		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
+		struct drm_crtc_state *drm_crtc_state;
+		struct drm_crtc *crtc = &intel_crtc->base;
+		int pixel_rate = 0;
 
-		if (!crtc_state->base.enable)
-			continue;
+		drm_crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
+		if (!drm_crtc_state) {
+			any_active |= intel_crtc->active;
+			pixel_rate = intel_crtc->min_cdclk;
+		} else if (drm_crtc_state->enable) {
+			struct intel_crtc_state *crtc_state =
+				to_intel_crtc_state(drm_crtc_state);
 
-		pixel_rate = ilk_pipe_pixel_rate(crtc_state);
+			any_active |= drm_crtc_state->active;
+			pixel_rate = ilk_pipe_pixel_rate(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(state->dev) && crtc_state->ips_enabled)
+				pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
+		}
 
+		to_intel_atomic_state(state)->pipe_cdclk[intel_crtc->pipe] = pixel_rate;
 		max_pixel_rate = max(max_pixel_rate, pixel_rate);
 	}
 
-	return max_pixel_rate;
+	return any_active ? max_pixel_rate : 0;
 }
 
 static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
@@ -9612,14 +9636,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;
+		return -EINVAL;
 	}
 
 	to_intel_atomic_state(state)->cdclk = cdclk;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 51722e657b91..54a2c0da2ece 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -250,6 +250,7 @@ struct intel_atomic_state {
 	unsigned int cdclk;
 	bool dpll_set;
 	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
+	unsigned int pipe_cdclk[I915_MAX_PIPES];
 };
 
 struct intel_plane_state {
@@ -536,8 +537,16 @@ struct intel_crtc {
 	 * Whether the crtc and the connected output pipeline is active. Implies
 	 * that crtc->enabled is set, i.e. the current mode configuration has
 	 * some outputs connected to this crtc.
+	 *
+	 * Protected by crtc.mutex and connection_mutex
 	 */
 	bool active;
+
+	/* Minimum cdclk required for this pipe,
+	 * protected by crtc.mutex and connection_mutex.
+	 */
+	unsigned int min_cdclk;
+
 	unsigned long enabled_power_domains;
 	bool lowfreq_avail;
 	struct intel_overlay *overlay;
-- 
2.1.0

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

Re: [PATCH 02/11] drm/i915: Do not acquire crtc state to check clock during modeset.

[Daniel Vetter]

On Thu, Oct 22, 2015 at 01:56:27PM +0200, Maarten Lankhorst wrote:
> 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.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 90 ++++++++++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_drv.h     |  9 ++++
>  2 files changed, 64 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 022e628b8520..051a1e2b1c55 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5237,6 +5237,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] = {};
> @@ -5245,13 +5246,20 @@ 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;
> +
> +		intel_crtc->min_cdclk = intel_state->pipe_cdclk[pipe];
> +
> +		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))
> @@ -5919,7 +5927,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;
> @@ -5929,8 +5936,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
> @@ -5939,22 +5948,31 @@ 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;
> +	int max_pixel_rate = 0;
> +	bool any_active = false;
>  
> -	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);
> +	for_each_intel_crtc(state->dev, intel_crtc) {
> +		struct drm_crtc_state *drm_crtc_state;
> +		struct drm_crtc *crtc = &intel_crtc->base;
> +		int pixclk = 0;
>  
> -		if (!crtc_state->base.enable)
> -			continue;
> +		drm_crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
> +		if (!drm_crtc_state) {
> +			any_active |= intel_crtc->active;
> +			pixclk = intel_crtc->min_cdclk;
> +		} else if (drm_crtc_state->enable) {
> +			struct intel_crtc_state *crtc_state =
> +				to_intel_crtc_state(drm_crtc_state);
> +
> +			any_active |= drm_crtc_state->active;
> +			pixclk = crtc_state->base.adjusted_mode.crtc_clock;
> +		}
>  
> -		max_pixclk = max(max_pixclk,
> -				 crtc_state->base.adjusted_mode.crtc_clock);
> +		to_intel_atomic_state(state)->pipe_cdclk[intel_crtc->pipe] = pixclk;
> +		max_pixel_rate = max(max_pixel_rate, pixclk);
>  	}
>  
> -	return max_pixclk;
> +	return any_active ? max_pixel_rate : 0;
>  }
>  
>  static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
> @@ -9491,29 +9509,35 @@ static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state)
>  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;
> +	bool any_active = false;
>  
>  	for_each_intel_crtc(state->dev, intel_crtc) {
> -		int pixel_rate;
> -
> -		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> -		if (IS_ERR(crtc_state))
> -			return PTR_ERR(crtc_state);
> +		struct drm_crtc_state *drm_crtc_state;
> +		struct drm_crtc *crtc = &intel_crtc->base;
> +		int pixel_rate = 0;
>  
> -		if (!crtc_state->base.enable)
> -			continue;
> +		drm_crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
> +		if (!drm_crtc_state) {
> +			any_active |= intel_crtc->active;
> +			pixel_rate = intel_crtc->min_cdclk;
> +		} else if (drm_crtc_state->enable) {
> +			struct intel_crtc_state *crtc_state =
> +				to_intel_crtc_state(drm_crtc_state);
>  
> -		pixel_rate = ilk_pipe_pixel_rate(crtc_state);
> +			any_active |= drm_crtc_state->active;
> +			pixel_rate = ilk_pipe_pixel_rate(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(state->dev) && crtc_state->ips_enabled)
> +				pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> +		}
>  
> +		to_intel_atomic_state(state)->pipe_cdclk[intel_crtc->pipe] = pixel_rate;
>  		max_pixel_rate = max(max_pixel_rate, pixel_rate);
>  	}
>  
> -	return max_pixel_rate;
> +	return any_active ? max_pixel_rate : 0;
>  }
>  
>  static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
> @@ -9612,14 +9636,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;
> +		return -EINVAL;
>  	}
>  
>  	to_intel_atomic_state(state)->cdclk = cdclk;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 51722e657b91..54a2c0da2ece 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -250,6 +250,7 @@ struct intel_atomic_state {
>  	unsigned int cdclk;
>  	bool dpll_set;
>  	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
> +	unsigned int pipe_cdclk[I915_MAX_PIPES];
>  };
>  
>  struct intel_plane_state {
> @@ -536,8 +537,16 @@ struct intel_crtc {
>  	 * Whether the crtc and the connected output pipeline is active. Implies
>  	 * that crtc->enabled is set, i.e. the current mode configuration has
>  	 * some outputs connected to this crtc.
> +	 *
> +	 * Protected by crtc.mutex and connection_mutex

This needs to be clarified to mean: read-protected by connection_mutex,
writers additionally must hold crtc->mutex. Same below.

Thinking about this a bit more the usual design we do is put the state
copies not into the parent object with funky rules, but instead into the
state of the higher-level objects. So for example the read-only
plane_active state is in crtc_state->plane_mask.

Similarly in this case here I think it would be cleaner design to have a
min_cdclk array in intel_state, setting it to 0 if the pipe is totally
off. That way locking rules are 100% clear: If you need to change it, you
need to copy the global state and acquire connection_mutex. That
automatically gives you read-access to all crtc's min_cdclk.

That way we don't play tricks with get_existing_state, don't add more
depencies on the legacy-encumbered intel_crtc->active and it's just
generally more awesome.

Thoughts?
-Daniel

>  	 */
>  	bool active;
> +
> +	/* Minimum cdclk required for this pipe,
> +	 * protected by crtc.mutex and connection_mutex.
> +	 */
> +	unsigned int min_cdclk;
> +
>  	unsigned long enabled_power_domains;
>  	bool lowfreq_avail;
>  	struct intel_overlay *overlay;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/11] drm/i915: Do not acquire crtc state to check clock during modeset.

[Maarten Lankhorst]

Op 22-10-15 om 15:08 schreef Daniel Vetter:
> On Thu, Oct 22, 2015 at 01:56:27PM +0200, Maarten Lankhorst wrote:
>> 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.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 90 ++++++++++++++++++++++--------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  9 ++++
>>  2 files changed, 64 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 022e628b8520..051a1e2b1c55 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5237,6 +5237,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] = {};
>> @@ -5245,13 +5246,20 @@ 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;
>> +
>> +		intel_crtc->min_cdclk = intel_state->pipe_cdclk[pipe];
>> +
>> +		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))
>> @@ -5919,7 +5927,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;
>> @@ -5929,8 +5936,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
>> @@ -5939,22 +5948,31 @@ 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;
>> +	int max_pixel_rate = 0;
>> +	bool any_active = false;
>>  
>> -	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);
>> +	for_each_intel_crtc(state->dev, intel_crtc) {
>> +		struct drm_crtc_state *drm_crtc_state;
>> +		struct drm_crtc *crtc = &intel_crtc->base;
>> +		int pixclk = 0;
>>  
>> -		if (!crtc_state->base.enable)
>> -			continue;
>> +		drm_crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
>> +		if (!drm_crtc_state) {
>> +			any_active |= intel_crtc->active;
>> +			pixclk = intel_crtc->min_cdclk;
>> +		} else if (drm_crtc_state->enable) {
>> +			struct intel_crtc_state *crtc_state =
>> +				to_intel_crtc_state(drm_crtc_state);
>> +
>> +			any_active |= drm_crtc_state->active;
>> +			pixclk = crtc_state->base.adjusted_mode.crtc_clock;
>> +		}
>>  
>> -		max_pixclk = max(max_pixclk,
>> -				 crtc_state->base.adjusted_mode.crtc_clock);
>> +		to_intel_atomic_state(state)->pipe_cdclk[intel_crtc->pipe] = pixclk;
>> +		max_pixel_rate = max(max_pixel_rate, pixclk);
>>  	}
>>  
>> -	return max_pixclk;
>> +	return any_active ? max_pixel_rate : 0;
>>  }
>>  
>>  static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
>> @@ -9491,29 +9509,35 @@ static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state)
>>  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;
>> +	bool any_active = false;
>>  
>>  	for_each_intel_crtc(state->dev, intel_crtc) {
>> -		int pixel_rate;
>> -
>> -		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
>> -		if (IS_ERR(crtc_state))
>> -			return PTR_ERR(crtc_state);
>> +		struct drm_crtc_state *drm_crtc_state;
>> +		struct drm_crtc *crtc = &intel_crtc->base;
>> +		int pixel_rate = 0;
>>  
>> -		if (!crtc_state->base.enable)
>> -			continue;
>> +		drm_crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
>> +		if (!drm_crtc_state) {
>> +			any_active |= intel_crtc->active;
>> +			pixel_rate = intel_crtc->min_cdclk;
>> +		} else if (drm_crtc_state->enable) {
>> +			struct intel_crtc_state *crtc_state =
>> +				to_intel_crtc_state(drm_crtc_state);
>>  
>> -		pixel_rate = ilk_pipe_pixel_rate(crtc_state);
>> +			any_active |= drm_crtc_state->active;
>> +			pixel_rate = ilk_pipe_pixel_rate(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(state->dev) && crtc_state->ips_enabled)
>> +				pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
>> +		}
>>  
>> +		to_intel_atomic_state(state)->pipe_cdclk[intel_crtc->pipe] = pixel_rate;
>>  		max_pixel_rate = max(max_pixel_rate, pixel_rate);
>>  	}
>>  
>> -	return max_pixel_rate;
>> +	return any_active ? max_pixel_rate : 0;
>>  }
>>  
>>  static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
>> @@ -9612,14 +9636,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;
>> +		return -EINVAL;
>>  	}
>>  
>>  	to_intel_atomic_state(state)->cdclk = cdclk;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 51722e657b91..54a2c0da2ece 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -250,6 +250,7 @@ struct intel_atomic_state {
>>  	unsigned int cdclk;
>>  	bool dpll_set;
>>  	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
>> +	unsigned int pipe_cdclk[I915_MAX_PIPES];
>>  };
>>  
>>  struct intel_plane_state {
>> @@ -536,8 +537,16 @@ struct intel_crtc {
>>  	 * Whether the crtc and the connected output pipeline is active. Implies
>>  	 * that crtc->enabled is set, i.e. the current mode configuration has
>>  	 * some outputs connected to this crtc.
>> +	 *
>> +	 * Protected by crtc.mutex and connection_mutex
> This needs to be clarified to mean: read-protected by connection_mutex,
> writers additionally must hold crtc->mutex. Same below.
>
> Thinking about this a bit more the usual design we do is put the state
> copies not into the parent object with funky rules, but instead into the
> state of the higher-level objects. So for example the read-only
> plane_active state is in crtc_state->plane_mask.
>
> Similarly in this case here I think it would be cleaner design to have a
> min_cdclk array in intel_state, setting it to 0 if the pipe is totally
> off. That way locking rules are 100% clear: If you need to change it, you
> need to copy the global state and acquire connection_mutex. That
> automatically gives you read-access to all crtc's min_cdclk.
Sorry this is the only case where a dependency on intel_crtc->active is valid.
fbc and watermark users should stop looking at it. (intel_crtc_active)

2 crtc's, #1 requires clock at 100, #2 requires clock at 200.
You do a DPMS mode off on #2, and if you don't track crtc_clock you would set the display clock to 100 and force a modeset on #1 too.
If you immediately do dpms off on #1 afterwards it means that the user would see #1 being disabled, re-enabled and disabled again..

> That way we don't play tricks with get_existing_state, don't add more
> depencies on the legacy-encumbered intel_crtc->active and it's just
> generally more awesome.
The tricks are needed to allow a modeset to run parallel to any page flips.
Though perhaps it would be better to add a struct to dev_priv that contains
min cdclk and a flag to see if the crtc is active or not.

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

Re: [PATCH 02/11] drm/i915: Do not acquire crtc state to check clock during modeset.

[Daniel Vetter]

On Thu, Oct 22, 2015 at 03:42:58PM +0200, Maarten Lankhorst wrote:
> Op 22-10-15 om 15:08 schreef Daniel Vetter:
> > On Thu, Oct 22, 2015 at 01:56:27PM +0200, Maarten Lankhorst wrote:
> >> 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.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 90 ++++++++++++++++++++++--------------
> >>  drivers/gpu/drm/i915/intel_drv.h     |  9 ++++
> >>  2 files changed, 64 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 022e628b8520..051a1e2b1c55 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -5237,6 +5237,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] = {};
> >> @@ -5245,13 +5246,20 @@ 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;
> >> +
> >> +		intel_crtc->min_cdclk = intel_state->pipe_cdclk[pipe];
> >> +
> >> +		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))
> >> @@ -5919,7 +5927,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;
> >> @@ -5929,8 +5936,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
> >> @@ -5939,22 +5948,31 @@ 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;
> >> +	int max_pixel_rate = 0;
> >> +	bool any_active = false;
> >>  
> >> -	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);
> >> +	for_each_intel_crtc(state->dev, intel_crtc) {
> >> +		struct drm_crtc_state *drm_crtc_state;
> >> +		struct drm_crtc *crtc = &intel_crtc->base;
> >> +		int pixclk = 0;
> >>  
> >> -		if (!crtc_state->base.enable)
> >> -			continue;
> >> +		drm_crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
> >> +		if (!drm_crtc_state) {
> >> +			any_active |= intel_crtc->active;
> >> +			pixclk = intel_crtc->min_cdclk;
> >> +		} else if (drm_crtc_state->enable) {
> >> +			struct intel_crtc_state *crtc_state =
> >> +				to_intel_crtc_state(drm_crtc_state);
> >> +
> >> +			any_active |= drm_crtc_state->active;
> >> +			pixclk = crtc_state->base.adjusted_mode.crtc_clock;
> >> +		}
> >>  
> >> -		max_pixclk = max(max_pixclk,
> >> -				 crtc_state->base.adjusted_mode.crtc_clock);
> >> +		to_intel_atomic_state(state)->pipe_cdclk[intel_crtc->pipe] = pixclk;
> >> +		max_pixel_rate = max(max_pixel_rate, pixclk);
> >>  	}
> >>  
> >> -	return max_pixclk;
> >> +	return any_active ? max_pixel_rate : 0;
> >>  }
> >>  
> >>  static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
> >> @@ -9491,29 +9509,35 @@ static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> >>  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;
> >> +	bool any_active = false;
> >>  
> >>  	for_each_intel_crtc(state->dev, intel_crtc) {
> >> -		int pixel_rate;
> >> -
> >> -		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> >> -		if (IS_ERR(crtc_state))
> >> -			return PTR_ERR(crtc_state);
> >> +		struct drm_crtc_state *drm_crtc_state;
> >> +		struct drm_crtc *crtc = &intel_crtc->base;
> >> +		int pixel_rate = 0;
> >>  
> >> -		if (!crtc_state->base.enable)
> >> -			continue;
> >> +		drm_crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
> >> +		if (!drm_crtc_state) {
> >> +			any_active |= intel_crtc->active;
> >> +			pixel_rate = intel_crtc->min_cdclk;
> >> +		} else if (drm_crtc_state->enable) {
> >> +			struct intel_crtc_state *crtc_state =
> >> +				to_intel_crtc_state(drm_crtc_state);
> >>  
> >> -		pixel_rate = ilk_pipe_pixel_rate(crtc_state);
> >> +			any_active |= drm_crtc_state->active;
> >> +			pixel_rate = ilk_pipe_pixel_rate(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(state->dev) && crtc_state->ips_enabled)
> >> +				pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> >> +		}
> >>  
> >> +		to_intel_atomic_state(state)->pipe_cdclk[intel_crtc->pipe] = pixel_rate;
> >>  		max_pixel_rate = max(max_pixel_rate, pixel_rate);
> >>  	}
> >>  
> >> -	return max_pixel_rate;
> >> +	return any_active ? max_pixel_rate : 0;
> >>  }
> >>  
> >>  static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
> >> @@ -9612,14 +9636,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;
> >> +		return -EINVAL;
> >>  	}
> >>  
> >>  	to_intel_atomic_state(state)->cdclk = cdclk;
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 51722e657b91..54a2c0da2ece 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -250,6 +250,7 @@ struct intel_atomic_state {
> >>  	unsigned int cdclk;
> >>  	bool dpll_set;
> >>  	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
> >> +	unsigned int pipe_cdclk[I915_MAX_PIPES];
> >>  };
> >>  
> >>  struct intel_plane_state {
> >> @@ -536,8 +537,16 @@ struct intel_crtc {
> >>  	 * Whether the crtc and the connected output pipeline is active. Implies
> >>  	 * that crtc->enabled is set, i.e. the current mode configuration has
> >>  	 * some outputs connected to this crtc.
> >> +	 *
> >> +	 * Protected by crtc.mutex and connection_mutex
> > This needs to be clarified to mean: read-protected by connection_mutex,
> > writers additionally must hold crtc->mutex. Same below.
> >
> > Thinking about this a bit more the usual design we do is put the state
> > copies not into the parent object with funky rules, but instead into the
> > state of the higher-level objects. So for example the read-only
> > plane_active state is in crtc_state->plane_mask.
> >
> > Similarly in this case here I think it would be cleaner design to have a
> > min_cdclk array in intel_state, setting it to 0 if the pipe is totally
> > off. That way locking rules are 100% clear: If you need to change it, you
> > need to copy the global state and acquire connection_mutex. That
> > automatically gives you read-access to all crtc's min_cdclk.
> Sorry this is the only case where a dependency on intel_crtc->active is valid.
> fbc and watermark users should stop looking at it. (intel_crtc_active)
> 
> 2 crtc's, #1 requires clock at 100, #2 requires clock at 200.
> You do a DPMS mode off on #2, and if you don't track crtc_clock you would set the display clock to 100 and force a modeset on #1 too.
> If you immediately do dpms off on #1 afterwards it means that the user would see #1 being disabled, re-enabled and disabled again..

Well I assumed min_cdclk != 0 iff active. But the point here isn't to
outright remove the depency upon the active state (I agree we need that to
avoid needless flickering when only changing active state), but to remove
the specific locking restrictions on stuff in intel_crtc. Instead I think
it's better to have this stored in the global state, which naturally gives
us exactly the locking you want, without having to explicitly document it
(and look it up all the time).

> > That way we don't play tricks with get_existing_state, don't add more
> > depencies on the legacy-encumbered intel_crtc->active and it's just
> > generally more awesome.
> The tricks are needed to allow a modeset to run parallel to any page flips.
> Though perhaps it would be better to add a struct to dev_priv that contains
> min cdclk and a flag to see if the crtc is active or not.

Yup, I get the trick. I'm just not happy with the implementation, it looks
way too much like legacy-modesest-lets-track-stuff-in-objects instead of
proper a atomic state based design.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/11] drm/i915: Do not acquire crtc state to check clock during modeset.

[Ville Syrjälä]

On Thu, Oct 22, 2015 at 01:56:27PM +0200, Maarten Lankhorst wrote:
> 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.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 90 ++++++++++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_drv.h     |  9 ++++
>  2 files changed, 64 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 022e628b8520..051a1e2b1c55 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5237,6 +5237,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] = {};
> @@ -5245,13 +5246,20 @@ 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;
> +
> +		intel_crtc->min_cdclk = intel_state->pipe_cdclk[pipe];
> +
> +		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))
> @@ -5919,7 +5927,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;
> @@ -5929,8 +5936,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;

This stuff ought to be a separate patch.

>  }
>  
>  /* Compute the max pixel clock for new configuration. Uses atomic state if
> @@ -5939,22 +5948,31 @@ 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;
> +	int max_pixel_rate = 0;
> +	bool any_active = false;
>  
> -	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);
> +	for_each_intel_crtc(state->dev, intel_crtc) {
> +		struct drm_crtc_state *drm_crtc_state;
> +		struct drm_crtc *crtc = &intel_crtc->base;
> +		int pixclk = 0;
>  
> -		if (!crtc_state->base.enable)
> -			continue;
> +		drm_crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);

Just a random idea here, but would be nicer if we wrap this into
something that returns the intel_crtc_state instead. Avoid having
to do to_intel_crtc_state() in the caller.

Generally I think we should only pass around the intel_foo_state stuff,
except when it gets passed to some core/helper stuff.

> +		if (!drm_crtc_state) {
> +			any_active |= intel_crtc->active;
> +			pixclk = intel_crtc->min_cdclk;
> +		} else if (drm_crtc_state->enable) {
> +			struct intel_crtc_state *crtc_state =
> +				to_intel_crtc_state(drm_crtc_state);
> +
> +			any_active |= drm_crtc_state->active;
> +			pixclk = crtc_state->base.adjusted_mode.crtc_clock;
> +		}
>  
> -		max_pixclk = max(max_pixclk,
> -				 crtc_state->base.adjusted_mode.crtc_clock);
> +		to_intel_atomic_state(state)->pipe_cdclk[intel_crtc->pipe] = pixclk;
> +		max_pixel_rate = max(max_pixel_rate, pixclk);
>  	}
>  
> -	return max_pixclk;
> +	return any_active ? max_pixel_rate : 0;
>  }
>  
>  static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
> @@ -9491,29 +9509,35 @@ static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state)
>  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;
> +	bool any_active = false;
>  
>  	for_each_intel_crtc(state->dev, intel_crtc) {
> -		int pixel_rate;
> -
> -		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> -		if (IS_ERR(crtc_state))
> -			return PTR_ERR(crtc_state);
> +		struct drm_crtc_state *drm_crtc_state;
> +		struct drm_crtc *crtc = &intel_crtc->base;
> +		int pixel_rate = 0;
>  
> -		if (!crtc_state->base.enable)
> -			continue;
> +		drm_crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
> +		if (!drm_crtc_state) {
> +			any_active |= intel_crtc->active;
> +			pixel_rate = intel_crtc->min_cdclk;
> +		} else if (drm_crtc_state->enable) {
> +			struct intel_crtc_state *crtc_state =
> +				to_intel_crtc_state(drm_crtc_state);
>  
> -		pixel_rate = ilk_pipe_pixel_rate(crtc_state);
> +			any_active |= drm_crtc_state->active;
> +			pixel_rate = ilk_pipe_pixel_rate(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(state->dev) && crtc_state->ips_enabled)
> +				pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> +		}
>  
> +		to_intel_atomic_state(state)->pipe_cdclk[intel_crtc->pipe] = pixel_rate;
>  		max_pixel_rate = max(max_pixel_rate, pixel_rate);
>  	}
>  
> -	return max_pixel_rate;
> +	return any_active ? max_pixel_rate : 0;
>  }
>  
>  static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
> @@ -9612,14 +9636,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;
> +		return -EINVAL;
>  	}
>  
>  	to_intel_atomic_state(state)->cdclk = cdclk;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 51722e657b91..54a2c0da2ece 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -250,6 +250,7 @@ struct intel_atomic_state {
>  	unsigned int cdclk;
>  	bool dpll_set;
>  	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
> +	unsigned int pipe_cdclk[I915_MAX_PIPES];
>  };
>  
>  struct intel_plane_state {
> @@ -536,8 +537,16 @@ struct intel_crtc {
>  	 * Whether the crtc and the connected output pipeline is active. Implies
>  	 * that crtc->enabled is set, i.e. the current mode configuration has
>  	 * some outputs connected to this crtc.
> +	 *
> +	 * Protected by crtc.mutex and connection_mutex
>  	 */
>  	bool active;
> +
> +	/* Minimum cdclk required for this pipe,
> +	 * protected by crtc.mutex and connection_mutex.
> +	 */
> +	unsigned int min_cdclk;
> +
>  	unsigned long enabled_power_domains;
>  	bool lowfreq_avail;
>  	struct intel_overlay *overlay;
> -- 
> 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

[PATCH 1/3] 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 59c94929f25b..37eef86e4a0a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1837,8 +1837,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 25a891aa3824..cb8fcf4fe7f2 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -305,5 +305,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 022e628b8520..a0508d5e9e13 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5237,6 +5237,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] = {};
@@ -5245,13 +5246,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))
@@ -5938,23 +5944,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;
 
-		max_pixclk = max(max_pixclk,
-				 crtc_state->base.adjusted_mode.crtc_clock);
+		if (crtc_state->enable)
+			pixclk = crtc_state->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)
@@ -6255,6 +6270,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;
 }
 
 /*
@@ -9490,29 +9508,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;
 }
 
@@ -12977,15 +13005,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
@@ -13009,7 +13049,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;
@@ -13179,12 +13219,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) {
@@ -13194,13 +13235,18 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	drm_atomic_helper_swap_state(dev, state);
 
+	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) {
@@ -13215,7 +13261,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);
@@ -13239,7 +13285,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)
@@ -13263,7 +13309,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);
@@ -15263,16 +15309,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 682f70f95e17..578afc7ac4bd 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];
 };
 
-- 
2.1.0

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

[PATCH 2/3] 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 a0508d5e9e13..e9a94ddcba73 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9640,14 +9640,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 3/3] 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 e9a94ddcba73..ddac097cd08a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5925,7 +5925,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;
@@ -5935,8 +5934,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

Re: [PATCH 3/3] drm/i915/bxt: Use the bypass frequency if there are no active pipes.

[Ville Syrjälä]

On Tue, Oct 27, 2015 at 02:26:33PM +0100, Maarten Lankhorst wrote:
> 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 e9a94ddcba73..ddac097cd08a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5925,7 +5925,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;
> @@ -5935,8 +5934,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;

I think we want to test this on actual hardware before putting it in.
Also we might want to do something similar on other platforms too.

>  }
>  
>  /* Compute the max pixel clock for new configuration. Uses atomic state if
> -- 
> 2.1.0

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

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

[Ville Syrjälä]

On Tue, Oct 27, 2015 at 02:26:31PM +0100, Maarten Lankhorst wrote:
> 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 59c94929f25b..37eef86e4a0a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1837,8 +1837,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 25a891aa3824..cb8fcf4fe7f2 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -305,5 +305,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 022e628b8520..a0508d5e9e13 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5237,6 +5237,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] = {};
> @@ -5245,13 +5246,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))
> @@ -5938,23 +5944,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;
>  
> -		max_pixclk = max(max_pixclk,
> -				 crtc_state->base.adjusted_mode.crtc_clock);
> +		if (crtc_state->enable)
> +			pixclk = crtc_state->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)
> @@ -6255,6 +6270,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;
>  }
>  
>  /*
> @@ -9490,29 +9508,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);

Could you avoid the reindent and renames please?

>  
> -		/* 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;
>  }
>  
> @@ -12977,15 +13005,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
> @@ -13009,7 +13049,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;
> @@ -13179,12 +13219,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) {
> @@ -13194,13 +13235,18 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  	drm_atomic_helper_swap_state(dev, state);
>  
> +	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) {
> @@ -13215,7 +13261,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);
> @@ -13239,7 +13285,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)
> @@ -13263,7 +13309,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);
> @@ -15263,16 +15309,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 682f70f95e17..578afc7ac4bd 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];
>  };
>  
> -- 
> 2.1.0

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

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

[Maarten Lankhorst]

Op 27-10-15 om 14:29 schreef Ville Syrjälä:
> On Tue, Oct 27, 2015 at 02:26:31PM +0100, Maarten Lankhorst wrote:
>> 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 59c94929f25b..37eef86e4a0a 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1837,8 +1837,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 25a891aa3824..cb8fcf4fe7f2 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -305,5 +305,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 022e628b8520..a0508d5e9e13 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5237,6 +5237,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] = {};
>> @@ -5245,13 +5246,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))
>> @@ -5938,23 +5944,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;
>>  
>> -		max_pixclk = max(max_pixclk,
>> -				 crtc_state->base.adjusted_mode.crtc_clock);
>> +		if (crtc_state->enable)
>> +			pixclk = crtc_state->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)
>> @@ -6255,6 +6270,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;
>>  }
>>  
>>  /*
>> @@ -9490,29 +9508,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);
> Could you avoid the reindent and renames please?
>
The pixclk has to be set even when enabled = false. This way the names for ilk_max_pixel_rate and intel_mode_max_pixclk are identical.

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

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

[Ville Syrjälä]

On Tue, Oct 27, 2015 at 02:41:16PM +0100, Maarten Lankhorst wrote:
> Op 27-10-15 om 14:29 schreef Ville Syrjälä:
> > On Tue, Oct 27, 2015 at 02:26:31PM +0100, Maarten Lankhorst wrote:
> >> 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 59c94929f25b..37eef86e4a0a 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -1837,8 +1837,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 25a891aa3824..cb8fcf4fe7f2 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >> @@ -305,5 +305,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 022e628b8520..a0508d5e9e13 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -5237,6 +5237,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] = {};
> >> @@ -5245,13 +5246,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))
> >> @@ -5938,23 +5944,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;
> >>  
> >> -		max_pixclk = max(max_pixclk,
> >> -				 crtc_state->base.adjusted_mode.crtc_clock);
> >> +		if (crtc_state->enable)
> >> +			pixclk = crtc_state->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)
> >> @@ -6255,6 +6270,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;
> >>  }
> >>  
> >>  /*
> >> @@ -9490,29 +9508,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);
> > Could you avoid the reindent and renames please?
> >
> The pixclk has to be set even when enabled = false. This way the names for ilk_max_pixel_rate and intel_mode_max_pixclk are identical.

I don't follow.

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

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

[Maarten Lankhorst]

Op 27-10-15 om 14:49 schreef Ville Syrjälä:
> On Tue, Oct 27, 2015 at 02:41:16PM +0100, Maarten Lankhorst wrote:
>> Op 27-10-15 om 14:29 schreef Ville Syrjälä:
>>> On Tue, Oct 27, 2015 at 02:26:31PM +0100, Maarten Lankhorst wrote:
>>>> 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 59c94929f25b..37eef86e4a0a 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1837,8 +1837,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 25a891aa3824..cb8fcf4fe7f2 100644
>>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>>>> @@ -305,5 +305,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 022e628b8520..a0508d5e9e13 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -5237,6 +5237,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] = {};
>>>> @@ -5245,13 +5246,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))
>>>> @@ -5938,23 +5944,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;
>>>>  
>>>> -		max_pixclk = max(max_pixclk,
>>>> -				 crtc_state->base.adjusted_mode.crtc_clock);
>>>> +		if (crtc_state->enable)
>>>> +			pixclk = crtc_state->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)
>>>> @@ -6255,6 +6270,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;
>>>>  }
>>>>  
>>>>  /*
>>>> @@ -9490,29 +9508,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);
>>> Could you avoid the reindent and renames please?
>>>
>> The pixclk has to be set even when enabled = false. This way the names for ilk_max_pixel_rate and intel_mode_max_pixclk are identical.
> I don't follow.
The old code was a noop when crtc_state->enable = false, new code needs to set min_pixclk[pipe] to 0 in that case.

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

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

[Ville Syrjälä]

On Tue, Oct 27, 2015 at 02:51:16PM +0100, Maarten Lankhorst wrote:
> Op 27-10-15 om 14:49 schreef Ville Syrjälä:
> > On Tue, Oct 27, 2015 at 02:41:16PM +0100, Maarten Lankhorst wrote:
> >> Op 27-10-15 om 14:29 schreef Ville Syrjälä:
> >>> On Tue, Oct 27, 2015 at 02:26:31PM +0100, Maarten Lankhorst wrote:
> >>>> 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 59c94929f25b..37eef86e4a0a 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_drv.h
> >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >>>> @@ -1837,8 +1837,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 25a891aa3824..cb8fcf4fe7f2 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >>>> @@ -305,5 +305,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 022e628b8520..a0508d5e9e13 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>> @@ -5237,6 +5237,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] = {};
> >>>> @@ -5245,13 +5246,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))
> >>>> @@ -5938,23 +5944,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;
> >>>>  
> >>>> -		max_pixclk = max(max_pixclk,
> >>>> -				 crtc_state->base.adjusted_mode.crtc_clock);
> >>>> +		if (crtc_state->enable)
> >>>> +			pixclk = crtc_state->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)
> >>>> @@ -6255,6 +6270,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;
> >>>>  }
> >>>>  
> >>>>  /*
> >>>> @@ -9490,29 +9508,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);
> >>> Could you avoid the reindent and renames please?
> >>>
> >> The pixclk has to be set even when enabled = false. This way the names for ilk_max_pixel_rate and intel_mode_max_pixclk are identical.
> > I don't follow.
> The old code was a noop when crtc_state->enable = false, new code needs to set min_pixclk[pipe] to 0 in that case.

That doesn't explain the rename or reindent.

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

[PATCH 03/11] 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_display.c | 28 +++-------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 2 files changed, 3 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 051a1e2b1c55..b8e1a5471bed 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4672,14 +4672,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
@@ -4760,9 +4752,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_plane *plane;
 
-	if (atomic->wait_vblank)
-		intel_wait_for_vblank(dev, crtc->pipe);
-
 	intel_frontbuffer_flip(dev, atomic->fb_bits);
 
 	if (atomic->disable_cxsr)
@@ -11661,15 +11650,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		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;
 		/* must disable cxsr around plane enable/disable */
 		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
-			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)) {
@@ -11716,21 +11702,12 @@ 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:
 		break;
 	case DRM_PLANE_TYPE_OVERLAY:
 		if (turn_off && !mode_changed) {
-			intel_crtc->atomic.wait_vblank = true;
 			intel_crtc->atomic.update_sprite_watermarks |=
 				1 << i;
 		}
@@ -13271,14 +13248,15 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 		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);
 
+	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);
 	mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 54a2c0da2ece..4b6bb1adfddd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -522,7 +522,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 03/11] drm/i915: Kill off intel_crtc->atomic.wait_vblank.

[Daniel Vetter]

On Thu, Oct 22, 2015 at 01:56:28PM +0200, 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.
> 
> 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>

Problem is that the next plane update should be possible right after this
one happens, which is the potentially racing with the post_plane_update
code. What's your plan to prevent this?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 28 +++-------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  2 files changed, 3 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 051a1e2b1c55..b8e1a5471bed 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4672,14 +4672,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
> @@ -4760,9 +4752,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_plane *plane;
>  
> -	if (atomic->wait_vblank)
> -		intel_wait_for_vblank(dev, crtc->pipe);
> -
>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>  
>  	if (atomic->disable_cxsr)
> @@ -11661,15 +11650,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  		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;
>  		/* must disable cxsr around plane enable/disable */
>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> -			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)) {
> @@ -11716,21 +11702,12 @@ 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:
>  		break;
>  	case DRM_PLANE_TYPE_OVERLAY:
>  		if (turn_off && !mode_changed) {
> -			intel_crtc->atomic.wait_vblank = true;
>  			intel_crtc->atomic.update_sprite_watermarks |=
>  				1 << i;
>  		}
> @@ -13271,14 +13248,15 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  		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);
>  
> +	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);
>  	mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 54a2c0da2ece..4b6bb1adfddd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -522,7 +522,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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/11] drm/i915: Kill off intel_crtc->atomic.wait_vblank.

[Ville Syrjälä]

On Thu, Oct 22, 2015 at 01:56:28PM +0200, 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.
> 
> 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_display.c | 28 +++-------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  2 files changed, 3 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 051a1e2b1c55..b8e1a5471bed 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4672,14 +4672,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);

The right fix for this crap is to not use the flip done interrupt. But
apparently no one wants to fix that.

So now I'm worried that we now depend on the atomic helper doing
synchornous vblank waits here. Are we expecting those vblank waits to
remain there? I would have expected to get rid of all synchronois vblank
waits in plane codepaths (apart from ones for hw workarounds).

Also does the helper actually wait for vblanks when the plane gets
enabled w/o the framebuffer actually changing?

> -
> -	/*
>  	 * 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
> @@ -4760,9 +4752,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_plane *plane;
>  
> -	if (atomic->wait_vblank)
> -		intel_wait_for_vblank(dev, crtc->pipe);
> -
>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>  
>  	if (atomic->disable_cxsr)
> @@ -11661,15 +11650,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>  			intel_crtc->atomic.disable_cxsr = true;
>  			/* to potentially re-enable cxsr */
> -			intel_crtc->atomic.wait_vblank = true;

What's there to make sure cxsr doesn't get enabled/disabled at the wrong
time now? I guess this is the same question as for the BDW w/a, ie. does
the plane getting enabled/disabled (or rather becoming
visible/invisible) always imply a vblank wait?

>  			intel_crtc->atomic.update_wm_post = true;
>  		}
>  	} else if (turn_off) {
>  		intel_crtc->atomic.update_wm_post = true;
>  		/* must disable cxsr around plane enable/disable */
>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> -			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)) {
> @@ -11716,21 +11702,12 @@ 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:
>  		break;
>  	case DRM_PLANE_TYPE_OVERLAY:
>  		if (turn_off && !mode_changed) {
> -			intel_crtc->atomic.wait_vblank = true;
>  			intel_crtc->atomic.update_sprite_watermarks |=
>  				1 << i;
>  		}
> @@ -13271,14 +13248,15 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  		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);
>  
> +	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);
>  	mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 54a2c0da2ece..4b6bb1adfddd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -522,7 +522,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

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

Re: [PATCH 03/11] drm/i915: Kill off intel_crtc->atomic.wait_vblank.

[Maarten Lankhorst]

Op 22-10-15 om 15:30 schreef Ville Syrjälä:
> On Thu, Oct 22, 2015 at 01:56:28PM +0200, 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.
>>
>> 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_display.c | 28 +++-------------------------
>>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>>  2 files changed, 3 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 051a1e2b1c55..b8e1a5471bed 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4672,14 +4672,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);
> The right fix for this crap is to not use the flip done interrupt. But
> apparently no one wants to fix that.
This is fixed later on when converting page_flip to atomic. I've modified
page_flip_finished to ignore flips when visible_changed happens on broadwell.
> So now I'm worried that we now depend on the atomic helper doing
> synchornous vblank waits here. Are we expecting those vblank waits to
> remain there? I would have expected to get rid of all synchronois vblank
> waits in plane codepaths (apart from ones for hw workarounds).
In my unify page flip and atomic series I first add all post_plane_update and
unpinning to unpin_work_fn.

After that I have a patch to convert the last part of atomic after wait_for_vblanks
to schedule unpin_work_fn, next I'll get more bold and zap the wait_for_vblanks and run it async.
It depends on work->can_async_unpin, if it's false it has to wait for everything to finish before queueing the next flip,
so it will finish before next one starts.

> Also does the helper actually wait for vblanks when the plane gets
> enabled w/o the framebuffer actually changing?
afaict it does, as long as the crtc is enabled.

I'm guessing that drm_atomic_helper_wait_for_vblanks should be fixed to look at crtc->state->active instead of crtc->state->enable..

>> -
>> -	/*
>>  	 * 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
>> @@ -4760,9 +4752,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct drm_plane *plane;
>>  
>> -	if (atomic->wait_vblank)
>> -		intel_wait_for_vblank(dev, crtc->pipe);
>> -
>>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>>  
>>  	if (atomic->disable_cxsr)
>> @@ -11661,15 +11650,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>>  			intel_crtc->atomic.disable_cxsr = true;
>>  			/* to potentially re-enable cxsr */
>> -			intel_crtc->atomic.wait_vblank = true;
> What's there to make sure cxsr doesn't get enabled/disabled at the wrong
> time now? I guess this is the same question as for the BDW w/a, ie. does
> the plane getting enabled/disabled (or rather becoming
> visible/invisible) always imply a vblank wait?
crtc mutex lock and in the future atomic wm's..
>>  			intel_crtc->atomic.update_wm_post = true;
>>  		}
>>  	} else if (turn_off) {
>>  		intel_crtc->atomic.update_wm_post = true;
>>  		/* must disable cxsr around plane enable/disable */
>>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>> -			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)) {
>> @@ -11716,21 +11702,12 @@ 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:
>>  		break;
>>  	case DRM_PLANE_TYPE_OVERLAY:
>>  		if (turn_off && !mode_changed) {
>> -			intel_crtc->atomic.wait_vblank = true;
>>  			intel_crtc->atomic.update_sprite_watermarks |=
>>  				1 << i;
>>  		}
>> @@ -13271,14 +13248,15 @@ static int intel_atomic_commit(struct drm_device *dev,
>>  
>>  		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);
>>  
>> +	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);
>>  	mutex_unlock(&dev->struct_mutex);
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 54a2c0da2ece..4b6bb1adfddd 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -522,7 +522,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

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

Re: [PATCH 03/11] drm/i915: Kill off intel_crtc->atomic.wait_vblank.

[Ville Syrjälä]

On Thu, Oct 22, 2015 at 04:50:05PM +0200, Maarten Lankhorst wrote:
> Op 22-10-15 om 15:30 schreef Ville Syrjälä:
> > On Thu, Oct 22, 2015 at 01:56:28PM +0200, 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.
> >>
> >> 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_display.c | 28 +++-------------------------
> >>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
> >>  2 files changed, 3 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 051a1e2b1c55..b8e1a5471bed 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4672,14 +4672,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);
> > The right fix for this crap is to not use the flip done interrupt. But
> > apparently no one wants to fix that.
> This is fixed later on when converting page_flip to atomic. I've modified
> page_flip_finished to ignore flips when visible_changed happens on broadwell.

Sounds like another hack.

> > So now I'm worried that we now depend on the atomic helper doing
> > synchornous vblank waits here. Are we expecting those vblank waits to
> > remain there? I would have expected to get rid of all synchronois vblank
> > waits in plane codepaths (apart from ones for hw workarounds).
> In my unify page flip and atomic series I first add all post_plane_update and
> unpinning to unpin_work_fn.
> 
> After that I have a patch to convert the last part of atomic after wait_for_vblanks
> to schedule unpin_work_fn, next I'll get more bold and zap the wait_for_vblanks and run it async.
> It depends on work->can_async_unpin, if it's false it has to wait for everything to finish before queueing the next flip,
> so it will finish before next one starts.
> 
> > Also does the helper actually wait for vblanks when the plane gets
> > enabled w/o the framebuffer actually changing?
> afaict it does, as long as the crtc is enabled.a

Where is that code? All I see is a old_fb != new_fb check.

> 
> I'm guessing that drm_atomic_helper_wait_for_vblanks should be fixed to look at crtc->state->active instead of crtc->state->enable..
> 
> >> -
> >> -	/*
> >>  	 * 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
> >> @@ -4760,9 +4752,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  	struct drm_plane *plane;
> >>  
> >> -	if (atomic->wait_vblank)
> >> -		intel_wait_for_vblank(dev, crtc->pipe);
> >> -
> >>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
> >>  
> >>  	if (atomic->disable_cxsr)
> >> @@ -11661,15 +11650,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> >>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> >>  			intel_crtc->atomic.disable_cxsr = true;
> >>  			/* to potentially re-enable cxsr */
> >> -			intel_crtc->atomic.wait_vblank = true;
> > What's there to make sure cxsr doesn't get enabled/disabled at the wrong
> > time now? I guess this is the same question as for the BDW w/a, ie. does
> > the plane getting enabled/disabled (or rather becoming
> > visible/invisible) always imply a vblank wait?
> crtc mutex lock and in the future atomic wm's..
> >>  			intel_crtc->atomic.update_wm_post = true;
> >>  		}
> >>  	} else if (turn_off) {
> >>  		intel_crtc->atomic.update_wm_post = true;
> >>  		/* must disable cxsr around plane enable/disable */
> >>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> >> -			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)) {
> >> @@ -11716,21 +11702,12 @@ 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:
> >>  		break;
> >>  	case DRM_PLANE_TYPE_OVERLAY:
> >>  		if (turn_off && !mode_changed) {
> >> -			intel_crtc->atomic.wait_vblank = true;
> >>  			intel_crtc->atomic.update_sprite_watermarks |=
> >>  				1 << i;
> >>  		}
> >> @@ -13271,14 +13248,15 @@ static int intel_atomic_commit(struct drm_device *dev,
> >>  
> >>  		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);
> >>  
> >> +	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);
> >>  	mutex_unlock(&dev->struct_mutex);
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 54a2c0da2ece..4b6bb1adfddd 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -522,7 +522,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

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

Re: [PATCH 03/11] drm/i915: Kill off intel_crtc->atomic.wait_vblank.

[Maarten Lankhorst]

Op 22-10-15 om 17:15 schreef Ville Syrjälä:
> On Thu, Oct 22, 2015 at 04:50:05PM +0200, Maarten Lankhorst wrote:
>> Op 22-10-15 om 15:30 schreef Ville Syrjälä:
>>> On Thu, Oct 22, 2015 at 01:56:28PM +0200, 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.
>>>>
>>>> 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_display.c | 28 +++-------------------------
>>>>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>>>>  2 files changed, 3 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 051a1e2b1c55..b8e1a5471bed 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -4672,14 +4672,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);
>>> The right fix for this crap is to not use the flip done interrupt. But
>>> apparently no one wants to fix that.
>> This is fixed later on when converting page_flip to atomic. I've modified
>> page_flip_finished to ignore flips when visible_changed happens on broadwell.
> Sounds like another hack.
It's not a hack when it's a workaround for buggy hw. ;-)
>
>>> So now I'm worried that we now depend on the atomic helper doing
>>> synchornous vblank waits here. Are we expecting those vblank waits to
>>> remain there? I would have expected to get rid of all synchronois vblank
>>> waits in plane codepaths (apart from ones for hw workarounds).
>> In my unify page flip and atomic series I first add all post_plane_update and
>> unpinning to unpin_work_fn.
>>
>> After that I have a patch to convert the last part of atomic after wait_for_vblanks
>> to schedule unpin_work_fn, next I'll get more bold and zap the wait_for_vblanks and run it async.
>> It depends on work->can_async_unpin, if it's false it has to wait for everything to finish before queueing the next flip,
>> so it will finish before next one starts.
>>
>>> Also does the helper actually wait for vblanks when the plane gets
>>> enabled w/o the framebuffer actually changing?
>> afaict it does, as long as the crtc is enabled.a
> Where is that code? All I see is a old_fb != new_fb check.
Oh indeed, I missed that one. That would make it unreliable to use though. :-/

I think drm_atomic_helper_wait_for_vblanks should be fixed to look at crtc->state->active and maybe take a force flag to force a vblank wait..

Daniel would you be ok with such a patch?

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

Re: [PATCH 03/11] drm/i915: Kill off intel_crtc->atomic.wait_vblank.

[Daniel Vetter]

On Mon, Oct 26, 2015 at 09:31:48AM +0100, Maarten Lankhorst wrote:
> Op 22-10-15 om 17:15 schreef Ville Syrjälä:
> > On Thu, Oct 22, 2015 at 04:50:05PM +0200, Maarten Lankhorst wrote:
> >> Op 22-10-15 om 15:30 schreef Ville Syrjälä:
> >>> On Thu, Oct 22, 2015 at 01:56:28PM +0200, 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.
> >>>>
> >>>> 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_display.c | 28 +++-------------------------
> >>>>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
> >>>>  2 files changed, 3 insertions(+), 26 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>> index 051a1e2b1c55..b8e1a5471bed 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>> @@ -4672,14 +4672,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);
> >>> The right fix for this crap is to not use the flip done interrupt. But
> >>> apparently no one wants to fix that.
> >> This is fixed later on when converting page_flip to atomic. I've modified
> >> page_flip_finished to ignore flips when visible_changed happens on broadwell.
> > Sounds like another hack.
> It's not a hack when it's a workaround for buggy hw. ;-)
> >
> >>> So now I'm worried that we now depend on the atomic helper doing
> >>> synchornous vblank waits here. Are we expecting those vblank waits to
> >>> remain there? I would have expected to get rid of all synchronois vblank
> >>> waits in plane codepaths (apart from ones for hw workarounds).
> >> In my unify page flip and atomic series I first add all post_plane_update and
> >> unpinning to unpin_work_fn.
> >>
> >> After that I have a patch to convert the last part of atomic after wait_for_vblanks
> >> to schedule unpin_work_fn, next I'll get more bold and zap the wait_for_vblanks and run it async.
> >> It depends on work->can_async_unpin, if it's false it has to wait for everything to finish before queueing the next flip,
> >> so it will finish before next one starts.
> >>
> >>> Also does the helper actually wait for vblanks when the plane gets
> >>> enabled w/o the framebuffer actually changing?
> >> afaict it does, as long as the crtc is enabled.a
> > Where is that code? All I see is a old_fb != new_fb check.
> Oh indeed, I missed that one. That would make it unreliable to use though. :-/
> 
> I think drm_atomic_helper_wait_for_vblanks should be fixed to look at crtc->state->active and maybe take a force flag to force a vblank wait..
> 
> Daniel would you be ok with such a patch?

The helper vblank wait is just to avoid tearing, which can't happen if the
fb doesn't change.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/11] drm/i915: Update watermark related members in the crtc_state.

[Maarten Lankhorst]

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

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 | 49 +++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_drv.h     |  6 ++---
 3 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 25a891aa3824..9d69cf25d7c5 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -94,6 +94,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	__drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state->base);
 
 	crtc_state->update_pipe = 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 b8e1a5471bed..5466a0b28f9d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4748,16 +4748,17 @@ 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;
 	struct drm_plane *plane;
 
 	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)
@@ -4778,6 +4779,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);
@@ -4788,10 +4791,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)
@@ -11597,6 +11603,7 @@ static bool intel_wm_need_update(struct drm_plane *plane,
 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;
@@ -11637,6 +11644,17 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	turn_off = was_visible && (!visible || mode_changed);
 	turn_on = visible && (!was_visible || mode_changed);
 
+	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;
+	} else if ((was_visible || visible) &&
+		   intel_wm_need_update(plane, plane_state)) {
+		pipe_config->wm_changed = true;
+	}
+
 	DRM_DEBUG_ATOMIC("[CRTC:%i] has [PLANE:%i] with fb %i\n", idx,
 			 plane->base.id, fb ? fb->base.id : -1);
 
@@ -11644,24 +11662,6 @@ 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.update_wm_post = true;
-		}
-	} else if (turn_off) {
-		intel_crtc->atomic.update_wm_post = true;
-		/* must disable cxsr around plane enable/disable */
-		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
-			intel_crtc->atomic.disable_cxsr = true;
-		}
-	} else if (intel_wm_need_update(plane, plane_state)) {
-		intel_crtc->atomic.update_wm_pre = true;
-	}
-
 	if (visible || was_visible)
 		intel_crtc->atomic.fb_bits |=
 			to_intel_plane(plane)->frontbuffer_bit;
@@ -11783,7 +11783,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 &&
@@ -13622,9 +13622,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 4b6bb1adfddd..a6b85655d717 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -352,7 +352,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,
@@ -516,9 +518,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

[PATCH 05/11] 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 5466a0b28f9d..d24fde6d1e2d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4785,9 +4785,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);
 
@@ -11671,19 +11668,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 a6b85655d717..0f745a1477b9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -517,7 +517,6 @@ struct skl_pipe_wm {
 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

[PATCH 06/11] 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 d24fde6d1e2d..317ff405aca9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4774,26 +4774,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);
 }
 
@@ -11665,7 +11679,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)
@@ -13184,7 +13197,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 			continue;
 
 		any_ms = true;
-		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);
@@ -13207,7 +13220,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);
 		bool update_pipe = !modeset &&
 			to_intel_crtc_state(crtc->state)->update_pipe;
@@ -13226,7 +13238,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 0f745a1477b9..e9e485336c90 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -517,7 +517,6 @@ struct skl_pipe_wm {
 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

[PATCH 07/11] 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 | 54 ++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h     |  4 +--
 3 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 9d69cf25d7c5..05456d90d6c7 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -96,6 +96,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->update_pipe = false;
 	crtc_state->visible_changed = false;
 	crtc_state->wm_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 317ff405aca9..1cd79d9b3da1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4745,16 +4745,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_state *old_plane_state;
 	struct drm_plane *plane;
+	int i;
 
-	intel_frontbuffer_flip(dev, atomic->fb_bits);
+	intel_frontbuffer_flip(dev, pipe_config->fb_bits);
 
 	crtc->wm.cxsr_allowed = true;
 
@@ -4764,12 +4768,34 @@ 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);
+	for_each_plane_in_state(old_state, plane, old_plane_state, i) {
+		struct intel_plane_state *plane_state =
+			to_intel_plane_state(plane->state);
+
+		if (old_plane_state->crtc != &crtc->base &&
+		    plane_state->base.crtc != &crtc->base)
+			continue;
+
+		switch (plane->type) {
+		case DRM_PLANE_TYPE_PRIMARY:
+			if (plane_state->visible &&
+			    (needs_modeset(&pipe_config->base) ||
+			     !to_intel_plane_state(old_plane_state)->visible))
+				intel_post_enable_primary(&crtc->base);
+			break;
+
+		case DRM_PLANE_TYPE_CURSOR:
+			break;
 
-	drm_for_each_plane_mask(plane, dev, atomic->update_sprite_watermarks)
-		intel_update_sprite_watermarks(plane, &crtc->base,
-					       0, 0, 0, false, false);
+		case DRM_PLANE_TYPE_OVERLAY:
+			if (to_intel_plane_state(old_plane_state)->visible &&
+			    !needs_modeset(&pipe_config->base) &&
+			    !plane_state->visible)
+				intel_update_sprite_watermarks(plane, &crtc->base,
+							       0, 0, 0, false, false);
+			break;
+		}
+	}
 
 	memset(atomic, 0, sizeof(*atomic));
 }
@@ -11623,7 +11649,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;
@@ -11674,13 +11699,10 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 			 turn_off, turn_on, mode_changed);
 
 	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;
 
@@ -11704,12 +11726,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		intel_crtc->atomic.update_fbc |= visible || mode_changed;
 		break;
 	case DRM_PLANE_TYPE_CURSOR:
-		break;
 	case DRM_PLANE_TYPE_OVERLAY:
-		if (turn_off && !mode_changed) {
-			intel_crtc->atomic.update_sprite_watermarks |=
-				1 << i;
-		}
+		break;
 	}
 	return 0;
 }
@@ -13253,7 +13271,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	drm_atomic_helper_wait_for_vblanks(dev, state);
 
 	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 e9e485336c90..75cfd8c7b981 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -352,6 +352,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 */
@@ -519,10 +520,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;
-	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

[PATCH 08/11] 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 | 63 +++++++++++++-----------------------
 drivers/gpu/drm/i915/intel_drv.h     | 16 ---------
 2 files changed, 23 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1cd79d9b3da1..6f92ccf2461c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4749,7 +4749,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;
@@ -4765,9 +4764,6 @@ 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);
-
 	for_each_plane_in_state(old_state, plane, old_plane_state, i) {
 		struct intel_plane_state *plane_state =
 			to_intel_plane_state(plane->state);
@@ -4782,6 +4778,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 			    (needs_modeset(&pipe_config->base) ||
 			     !to_intel_plane_state(old_plane_state)->visible))
 				intel_post_enable_primary(&crtc->base);
+
+			if (plane_state->visible)
+				intel_fbc_update(dev_priv);
 			break;
 
 		case DRM_PLANE_TYPE_CURSOR:
@@ -4796,8 +4795,6 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 			break;
 		}
 	}
-
-	memset(atomic, 0, sizeof(*atomic));
 }
 
 static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
@@ -4805,7 +4802,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;
@@ -4814,9 +4810,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);
@@ -4824,8 +4817,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) {
@@ -11645,7 +11657,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,34 +11712,6 @@ 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:
-	case DRM_PLANE_TYPE_OVERLAY:
-		break;
-	}
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 75cfd8c7b981..40011f637980 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -509,20 +509,6 @@ struct skl_pipe_wm {
 	uint32_t linetime;
 };
 
-/*
- * 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;
@@ -590,8 +576,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

[PATCH 09/11] drm/i915/skl: Prevent unclaimed register writes on skylake.

[Maarten Lankhorst]

I'm getting unclaimed register writes when checking the WM registers
after the crtc is disabled. So I would imagine those are guarded by
the crtc power well. Fix this by not reading out wm state when the
power well is off.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4c1c1bb96a9e..fbc10331055e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2833,7 +2833,12 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 	int plane;
 	u32 val;
 
+	memset(ddb, 0, sizeof(*ddb));
+
 	for_each_pipe(dev_priv, pipe) {
+		if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PIPE(pipe)))
+			continue;
+
 		for_each_plane(dev_priv, pipe, plane) {
 			val = I915_READ(PLANE_BUF_CFG(pipe, plane));
 			skl_ddb_entry_init_from_hw(&ddb->plane[pipe][plane],
-- 
2.1.0

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

Re: [Intel-gfx] [PATCH 09/11] drm/i915/skl: Prevent unclaimed register writes on skylake.

[Daniel Vetter]

On Thu, Oct 22, 2015 at 01:56:34PM +0200, Maarten Lankhorst wrote:
> I'm getting unclaimed register writes when checking the WM registers
> after the crtc is disabled. So I would imagine those are guarded by
> the crtc power well. Fix this by not reading out wm state when the
> power well is off.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92181
> Cc: stable@vger.kernel.org

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Jani, one for you.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4c1c1bb96a9e..fbc10331055e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2833,7 +2833,12 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  	int plane;
>  	u32 val;
>  
> +	memset(ddb, 0, sizeof(*ddb));
> +
>  	for_each_pipe(dev_priv, pipe) {
> +		if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PIPE(pipe)))
> +			continue;
> +
>  		for_each_plane(dev_priv, pipe, plane) {
>  			val = I915_READ(PLANE_BUF_CFG(pipe, plane));
>  			skl_ddb_entry_init_from_hw(&ddb->plane[pipe][plane],
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 09/11] drm/i915/skl: Prevent unclaimed register writes on skylake.

[Jani Nikula]

On Thu, 22 Oct 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Oct 22, 2015 at 01:56:34PM +0200, Maarten Lankhorst wrote:
>> I'm getting unclaimed register writes when checking the WM registers
>> after the crtc is disabled. So I would imagine those are guarded by
>> the crtc power well. Fix this by not reading out wm state when the
>> power well is off.
>> 
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92181
>> Cc: stable@vger.kernel.org
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Jani, one for you.

This one pushed to drm-intel-fixes, thanks for the patch and review.

I dropped cc: stable because this will still make it to v4.3, and we're
not backporting SKL fixes beyond that.

BR,
Jani.


> -Daniel
>
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 4c1c1bb96a9e..fbc10331055e 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -2833,7 +2833,12 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>>  	int plane;
>>  	u32 val;
>>  
>> +	memset(ddb, 0, sizeof(*ddb));
>> +
>>  	for_each_pipe(dev_priv, pipe) {
>> +		if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PIPE(pipe)))
>> +			continue;
>> +
>>  		for_each_plane(dev_priv, pipe, plane) {
>>  			val = I915_READ(PLANE_BUF_CFG(pipe, plane));
>>  			skl_ddb_entry_init_from_hw(&ddb->plane[pipe][plane],
>> -- 
>> 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

Re: [Intel-gfx] [PATCH 09/11] drm/i915/skl: Prevent unclaimed register writes on skylake.

[Jani Nikula]

On Fri, 23 Oct 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Thu, 22 Oct 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Thu, Oct 22, 2015 at 01:56:34PM +0200, Maarten Lankhorst wrote:
>>> I'm getting unclaimed register writes when checking the WM registers
>>> after the crtc is disabled. So I would imagine those are guarded by
>>> the crtc power well. Fix this by not reading out wm state when the
>>> power well is off.
>>> 
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92181
>>> Cc: stable@vger.kernel.org
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Jani, one for you.
>
> This one pushed to drm-intel-fixes, thanks for the patch and review.
>
> I dropped cc: stable because this will still make it to v4.3, and we're
> not backporting SKL fixes beyond that.

This didn't make it to v4.3 after all. Dropped from drm-intel-fixes,
pushed to drm-intel-next-fixes, and marked cc: stable for v4.3.

BR,
Jani.


>
> BR,
> Jani.
>
>
>> -Daniel
>>
>>> ---
>>>  drivers/gpu/drm/i915/intel_pm.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>> 
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index 4c1c1bb96a9e..fbc10331055e 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -2833,7 +2833,12 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>>>  	int plane;
>>>  	u32 val;
>>>  
>>> +	memset(ddb, 0, sizeof(*ddb));
>>> +
>>>  	for_each_pipe(dev_priv, pipe) {
>>> +		if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PIPE(pipe)))
>>> +			continue;
>>> +
>>>  		for_each_plane(dev_priv, pipe, plane) {
>>>  			val = I915_READ(PLANE_BUF_CFG(pipe, plane));
>>>  			skl_ddb_entry_init_from_hw(&ddb->plane[pipe][plane],
>>> -- 
>>> 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

[PATCH 10/11] 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 6f92ccf2461c..26931d8eb7ce 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4761,7 +4761,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);
 
 	for_each_plane_in_state(old_state, plane, old_plane_state, i) {
@@ -13205,6 +13205,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

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

Re: [Intel-gfx] [PATCH 10/11] drm/i915/skl: Update watermarks before the crtc is disabled.

[Daniel Vetter]

On Thu, Oct 22, 2015 at 01:56:35PM +0200, Maarten Lankhorst wrote:
> 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 6f92ccf2461c..26931d8eb7ce 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4761,7 +4761,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);

Calling post_plane_commit seems like a bug of the higher-level functions.
If we can't fix that quickly I think we should have at least an early exit
at the top, with a FIXME comment.

If the platform/feature-specific commit hooks have to care about
state->active, then the higher level functions imo haven't done their jobs
properly.

>  
>  	for_each_plane_in_state(old_state, plane, old_plane_state, i) {
> @@ -13205,6 +13205,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);

Does this ever do anything at all? We just killed the crtc completely
above, those watermark writes will get cleansed as soon as the power well
goes down.
-Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 11/11] 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 26931d8eb7ce..6d19723419fb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13541,7 +13541,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 11/11] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Daniel Vetter]

On Thu, Oct 22, 2015 at 01:56:36PM +0200, 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.
> 
> 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 26931d8eb7ce..6d19723419fb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13541,7 +13541,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)

Never check for ->active in atomic_check functions, it might cause dpms on
to fail, which should never happen. We probably should check for
crtc_state->enabled instead.

Otoh why are we even calling the compute config stuff when the crtc is
off. Imo better to cut this out somewhere in the top level functions.
-Daniel

>  		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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx