[v4,6/8] drm/i915: Nuke fbc members from intel_crtc->atomic, v2.

Submitted by Maarten Lankhorst on Feb. 10, 2016, 12:49 p.m.

Details

Message ID 1455108583-29227-7-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show
Series "Kill off intel_crtc->atomic!" ( rev: 8 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Maarten Lankhorst Feb. 10, 2016, 12:49 p.m.
Factor out intel_fbc_supports_rotation and use it in
pre_plane_update as well. This leaves intel_crtc->atomic
empty, so remove it too.

Changes since v1:
- Add a intel_fbc_supports_rotation helper.

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

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 54be8a255f1f..00cb261c6787 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4782,11 +4782,9 @@  static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
 	struct drm_atomic_state *old_state = old_crtc_state->base.state;
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_plane *primary = crtc->base.primary;
 	struct drm_plane_state *old_pri_state =
 		drm_atomic_get_existing_plane_state(old_state, primary);
@@ -4798,22 +4796,19 @@  static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 	if (pipe_config->wm_changed && pipe_config->base.active)
 		intel_update_watermarks(&crtc->base);
 
-	if (atomic->update_fbc)
-		intel_fbc_post_update(crtc);
-
 	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);
 
+		intel_fbc_post_update(crtc);
+
 		if (primary_state->visible &&
 		    (needs_modeset(&pipe_config->base) ||
 		     !old_primary_state->visible))
 			intel_post_enable_primary(&crtc->base);
 	}
-
-	memset(atomic, 0, sizeof(*atomic));
 }
 
 static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
@@ -4821,7 +4816,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;
@@ -4830,17 +4824,17 @@  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->update_fbc)
-		intel_fbc_pre_update(crtc);
-
 	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);
+		bool turn_off = old_primary_state->visible &&
+		    (modeset || !primary_state->visible);
+
+		intel_fbc_pre_update(crtc);
 
-		if (old_primary_state->visible &&
-		    (modeset || !primary_state->visible))
+		if (turn_off)
 			intel_pre_disable_primary(&crtc->base);
 	}
 
@@ -11822,27 +11816,17 @@  int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	if (visible || was_visible)
 		pipe_config->fb_bits |= to_intel_plane(plane)->frontbuffer_bit;
 
-	switch (plane->type) {
-	case DRM_PLANE_TYPE_PRIMARY:
-		intel_crtc->atomic.update_fbc = true;
-
-		break;
-	case DRM_PLANE_TYPE_CURSOR:
-		break;
-	case DRM_PLANE_TYPE_OVERLAY:
-		/*
-		 * WaCxSRDisabledForSpriteScaling:ivb
-		 *
-		 * cstate->update_wm was already set above, so this flag will
-		 * take effect when we commit and program watermarks.
-		 */
-		if (IS_IVYBRIDGE(dev) &&
-		    needs_scaling(to_intel_plane_state(plane_state)) &&
-		    !needs_scaling(old_plane_state))
-			pipe_config->disable_lp_wm = true;
+	/*
+	 * WaCxSRDisabledForSpriteScaling:ivb
+	 *
+	 * cstate->update_wm was already set above, so this flag will
+	 * take effect when we commit and program watermarks.
+	 */
+	if (plane->type == DRM_PLANE_TYPE_OVERLAY && IS_IVYBRIDGE(dev) &&
+	    needs_scaling(to_intel_plane_state(plane_state)) &&
+	    !needs_scaling(old_plane_state))
+		pipe_config->disable_lp_wm = true;
 
-		break;
-	}
 	return 0;
 }
 
@@ -13241,9 +13225,6 @@  static int intel_atomic_check(struct drm_device *dev,
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc_state);
 
-		memset(&to_intel_crtc(crtc)->atomic, 0,
-		       sizeof(struct intel_crtc_atomic_commit));
-
 		/* Catch I915_MODE_FLAG_INHERITED */
 		if (crtc_state->mode.private_flags != crtc->state->mode.private_flags)
 			crtc_state->mode_changed = true;
@@ -13528,12 +13509,13 @@  static int intel_atomic_commit(struct drm_device *dev,
 		if (!modeset)
 			intel_pre_plane_update(to_intel_crtc_state(crtc_state));
 
-		if (crtc->state->active && intel_crtc->atomic.update_fbc)
+		if ((modeset || update_pipe) && crtc->state->active)
 			intel_fbc_enable(intel_crtc);
 
 		if (crtc->state->active &&
-		    (crtc->state->planes_changed || update_pipe))
+		    (crtc->state->planes_changed || update_pipe)) {
 			drm_atomic_helper_commit_planes_on_crtc(crtc_state);
+		}
 
 		if (pipe_config->base.active &&
 		    (pipe_config->wm_changed || pipe_config->disable_cxsr ||
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 803591d1e613..8effb9ece21e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -538,19 +538,6 @@  struct intel_mmio_flip {
 	unsigned int rotation;
 };
 
-/*
- * Tracking of operations that need to be performed at the beginning/end of an
- * atomic commit, outside the atomic section where interrupts are disabled.
- * These are generally operations that grab mutexes or might otherwise sleep
- * and thus can't be run with interrupts disabled.
- */
-struct intel_crtc_atomic_commit {
-	/* Sleepable operations to perform before commit */
-
-	/* Sleepable operations to perform after commit */
-	bool update_fbc;
-};
-
 struct intel_crtc {
 	struct drm_crtc base;
 	enum pipe pipe;
@@ -610,8 +597,6 @@  struct intel_crtc {
 		int scanline_start;
 	} debug;
 
-	struct intel_crtc_atomic_commit atomic;
-
 	/* scalers available on this crtc */
 	int num_scalers;
 

Comments

Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> Factor out intel_fbc_supports_rotation


^ not anymore


>  and use it in

> pre_plane_update as well. This leaves intel_crtc->atomic

> empty, so remove it too.

> 

> Changes since v1:

> - Add a intel_fbc_supports_rotation helper.


Changes since v2:
- No more need for rotation special-casing.

(I suppose you also have to edit the commit title to be v3)

> 

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

> ---

>  drivers/gpu/drm/i915/intel_display.c | 58 +++++++++++++-------------

> ----------

>  drivers/gpu/drm/i915/intel_drv.h     | 15 ----------

>  2 files changed, 20 insertions(+), 53 deletions(-)

> 

> diff --git a/drivers/gpu/drm/i915/intel_display.c

> b/drivers/gpu/drm/i915/intel_display.c

> index 54be8a255f1f..00cb261c6787 100644

> --- a/drivers/gpu/drm/i915/intel_display.c

> +++ b/drivers/gpu/drm/i915/intel_display.c

> @@ -4782,11 +4782,9 @@ static void intel_post_plane_update(struct

> intel_crtc_state *old_crtc_state)

>  {

>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-

> >base.crtc);

>  	struct drm_atomic_state *old_state = old_crtc_state-

> >base.state;

> -	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;

>  	struct intel_crtc_state *pipe_config =

>  		to_intel_crtc_state(crtc->base.state);

>  	struct drm_device *dev = crtc->base.dev;

> -	struct drm_i915_private *dev_priv = dev->dev_private;

>  	struct drm_plane *primary = crtc->base.primary;

>  	struct drm_plane_state *old_pri_state =

>  		drm_atomic_get_existing_plane_state(old_state,

> primary);

> @@ -4798,22 +4796,19 @@ static void intel_post_plane_update(struct

> intel_crtc_state *old_crtc_state)

>  	if (pipe_config->wm_changed && pipe_config->base.active)

>  		intel_update_watermarks(&crtc->base);

>  

> -	if (atomic->update_fbc)

> -		intel_fbc_post_update(crtc);

> -

>  	if (old_pri_state) {


For a code reader that is not very familiar with all the atomic history
and its details, it's not trivial to conclude that "if
(drm_atomic_get_existing_plane_state(primary))", then we're updating
the primary plane on this atomic commit. And before this patch, it's
much much easier to conclude that update_fbc will be true when an
atomic update is touching the primary plane because that's explicitly
stated by the cod.

So although "let's kill this redundant struct" sounds good, it seems to
me that code clarity is going away with this patch, so I wonder if the
benefits of the patch outweigh the downsides. Isn't there something
else we could do about this, such as some renaming, or adding some
function aliases or just extra commenting?

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

>  

> +		intel_fbc_post_update(crtc);

> +

>  		if (primary_state->visible &&

>  		    (needs_modeset(&pipe_config->base) ||

>  		     !old_primary_state->visible))

>  			intel_post_enable_primary(&crtc->base);

>  	}

> -

> -	memset(atomic, 0, sizeof(*atomic));

>  }

>  

>  static void intel_pre_plane_update(struct intel_crtc_state

> *old_crtc_state)

> @@ -4821,7 +4816,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;

> @@ -4830,17 +4824,17 @@ 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->update_fbc)

> -		intel_fbc_pre_update(crtc);

> -

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

> +		bool turn_off = old_primary_state->visible &&

> +		    (modeset || !primary_state->visible); 


Not really related to the patch, but ok to keep since it's trivial...

> +

> +		intel_fbc_pre_update(crtc);

>  

> -		if (old_primary_state->visible &&

> -		    (modeset || !primary_state->visible))

> +		if (turn_off)

>  			intel_pre_disable_primary(&crtc->base);

>  	}

>  

> @@ -11822,27 +11816,17 @@ int intel_plane_atomic_calc_changes(struct

> drm_crtc_state *crtc_state,

>  	if (visible || was_visible)

>  		pipe_config->fb_bits |= to_intel_plane(plane)-

> >frontbuffer_bit;

>  

> -	switch (plane->type) {

> -	case DRM_PLANE_TYPE_PRIMARY:

> -		intel_crtc->atomic.update_fbc = true;

> -

> -		break;

> -	case DRM_PLANE_TYPE_CURSOR:

> -		break;

> -	case DRM_PLANE_TYPE_OVERLAY:

> -		/*

> -		 * WaCxSRDisabledForSpriteScaling:ivb

> -		 *

> -		 * cstate->update_wm was already set above, so this

> flag will

> -		 * take effect when we commit and program

> watermarks.

> -		 */

> -		if (IS_IVYBRIDGE(dev) &&

> -		    needs_scaling(to_intel_plane_state(plane_state))

> &&

> -		    !needs_scaling(old_plane_state))

> -			pipe_config->disable_lp_wm = true;

> +	/*

> +	 * WaCxSRDisabledForSpriteScaling:ivb

> +	 *

> +	 * cstate->update_wm was already set above, so this flag

> will

> +	 * take effect when we commit and program watermarks.

> +	 */

> +	if (plane->type == DRM_PLANE_TYPE_OVERLAY &&

> IS_IVYBRIDGE(dev) &&

> +	    needs_scaling(to_intel_plane_state(plane_state)) &&

> +	    !needs_scaling(old_plane_state))

> +		pipe_config->disable_lp_wm = true;

>  

> -		break;

> -	}

>  	return 0;

>  }

>  

> @@ -13241,9 +13225,6 @@ static int intel_atomic_check(struct

> drm_device *dev,

>  		struct intel_crtc_state *pipe_config =

>  			to_intel_crtc_state(crtc_state);

>  

> -		memset(&to_intel_crtc(crtc)->atomic, 0,

> -		       sizeof(struct intel_crtc_atomic_commit));

> -

>  		/* Catch I915_MODE_FLAG_INHERITED */

>  		if (crtc_state->mode.private_flags != crtc->state-

> >mode.private_flags)

>  			crtc_state->mode_changed = true;

> @@ -13528,12 +13509,13 @@ static int intel_atomic_commit(struct

> drm_device *dev,

>  		if (!modeset)

>  			intel_pre_plane_update(to_intel_crtc_state(c

> rtc_state));

>  

> -		if (crtc->state->active && intel_crtc-

> >atomic.update_fbc)

> +		if ((modeset || update_pipe) && crtc->state->active)


Same here: not easy for me to verify things are the same now. Even
worse: the check for "is this atomic commit touching the primary
plane?" is now written in a completely different way than the one
introduced above. Maybe there's something we could do to make the code
easier to read.


>  			intel_fbc_enable(intel_crtc);

>  

>  		if (crtc->state->active &&

> -		    (crtc->state->planes_changed || update_pipe))

> +		    (crtc->state->planes_changed || update_pipe)) {

>  			drm_atomic_helper_commit_planes_on_crtc(crtc

> _state);

> +		}


???


Besides the points mentioned above, the patch seems to pass the FBC
tests, which is good :)

My suggestions are sorta bikesheds and although I'm not 100% confident,
the patch seems to be correct, so:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


But please consider the readability concerns pointed on this review,
maybe by proposing v4 or a follow-up patch :)

>  

>  		if (pipe_config->base.active &&

>  		    (pipe_config->wm_changed || pipe_config-

> >disable_cxsr ||

> diff --git a/drivers/gpu/drm/i915/intel_drv.h

> b/drivers/gpu/drm/i915/intel_drv.h

> index 803591d1e613..8effb9ece21e 100644

> --- a/drivers/gpu/drm/i915/intel_drv.h

> +++ b/drivers/gpu/drm/i915/intel_drv.h

> @@ -538,19 +538,6 @@ struct intel_mmio_flip {

>  	unsigned int rotation;

>  };

>  

> -/*

> - * Tracking of operations that need to be performed at the

> beginning/end of an

> - * atomic commit, outside the atomic section where interrupts are

> disabled.

> - * These are generally operations that grab mutexes or might

> otherwise sleep

> - * and thus can't be run with interrupts disabled.

> - */

> -struct intel_crtc_atomic_commit {

> -	/* Sleepable operations to perform before commit */

> -

> -	/* Sleepable operations to perform after commit */

> -	bool update_fbc;

> -};

> -

>  struct intel_crtc {

>  	struct drm_crtc base;

>  	enum pipe pipe;

> @@ -610,8 +597,6 @@ struct intel_crtc {

>  		int scanline_start;

>  	} debug;

>  

> -	struct intel_crtc_atomic_commit atomic;

> -

>  	/* scalers available on this crtc */

>  	int num_scalers;

>
Op 12-02-16 om 14:56 schreef Zanoni, Paulo R:
> Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
>> Factor out intel_fbc_supports_rotation
> ^ not anymore
>
>
>>  and use it in
>> pre_plane_update as well. This leaves intel_crtc->atomic
>> empty, so remove it too.
>>
>> Changes since v1:
>> - Add a intel_fbc_supports_rotation helper.
> Changes since v2:
> - No more need for rotation special-casing.
>
> (I suppose you also have to edit the commit title to be v3)
>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 58 +++++++++++++-------------
>> ----------
>>  drivers/gpu/drm/i915/intel_drv.h     | 15 ----------
>>  2 files changed, 20 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 54be8a255f1f..00cb261c6787 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4782,11 +4782,9 @@ static void intel_post_plane_update(struct
>> intel_crtc_state *old_crtc_state)
>>  {
>>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
>>> base.crtc);
>>  	struct drm_atomic_state *old_state = old_crtc_state-
>>> base.state;
>> -	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>>  	struct intel_crtc_state *pipe_config =
>>  		to_intel_crtc_state(crtc->base.state);
>>  	struct drm_device *dev = crtc->base.dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct drm_plane *primary = crtc->base.primary;
>>  	struct drm_plane_state *old_pri_state =
>>  		drm_atomic_get_existing_plane_state(old_state,
>> primary);
>> @@ -4798,22 +4796,19 @@ static void intel_post_plane_update(struct
>> intel_crtc_state *old_crtc_state)
>>  	if (pipe_config->wm_changed && pipe_config->base.active)
>>  		intel_update_watermarks(&crtc->base);
>>  
>> -	if (atomic->update_fbc)
>> -		intel_fbc_post_update(crtc);
>> -
>>  	if (old_pri_state) {
> For a code reader that is not very familiar with all the atomic history
> and its details, it's not trivial to conclude that "if
> (drm_atomic_get_existing_plane_state(primary))", then we're updating
> the primary plane on this atomic commit. And before this patch, it's
> much much easier to conclude that update_fbc will be true when an
> atomic update is touching the primary plane because that's explicitly
> stated by the cod.
>
> So although "let's kill this redundant struct" sounds good, it seems to
> me that code clarity is going away with this patch, so I wonder if the
> benefits of the patch outweigh the downsides. Isn't there something
> else we could do about this, such as some renaming, or adding some
> function aliases or just extra commenting?
>
>>  		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);
>>  
>> +		intel_fbc_post_update(crtc);
>> +
>>  		if (primary_state->visible &&
>>  		    (needs_modeset(&pipe_config->base) ||
>>  		     !old_primary_state->visible))
>>  			intel_post_enable_primary(&crtc->base);
>>  	}
>> -
>> -	memset(atomic, 0, sizeof(*atomic));
>>  }
>>  
>>  static void intel_pre_plane_update(struct intel_crtc_state
>> *old_crtc_state)
>> @@ -4821,7 +4816,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;
>> @@ -4830,17 +4824,17 @@ 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->update_fbc)
>> -		intel_fbc_pre_update(crtc);
>> -
>>  	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);
>> +		bool turn_off = old_primary_state->visible &&
>> +		    (modeset || !primary_state->visible); 
> Not really related to the patch, but ok to keep since it's trivial...
>
>> +
>> +		intel_fbc_pre_update(crtc);
>>  
>> -		if (old_primary_state->visible &&
>> -		    (modeset || !primary_state->visible))
>> +		if (turn_off)
>>  			intel_pre_disable_primary(&crtc->base);
>>  	}
>>  
>> @@ -11822,27 +11816,17 @@ int intel_plane_atomic_calc_changes(struct
>> drm_crtc_state *crtc_state,
>>  	if (visible || was_visible)
>>  		pipe_config->fb_bits |= to_intel_plane(plane)-
>>> frontbuffer_bit;
>>  
>> -	switch (plane->type) {
>> -	case DRM_PLANE_TYPE_PRIMARY:
>> -		intel_crtc->atomic.update_fbc = true;
>> -
>> -		break;
>> -	case DRM_PLANE_TYPE_CURSOR:
>> -		break;
>> -	case DRM_PLANE_TYPE_OVERLAY:
>> -		/*
>> -		 * WaCxSRDisabledForSpriteScaling:ivb
>> -		 *
>> -		 * cstate->update_wm was already set above, so this
>> flag will
>> -		 * take effect when we commit and program
>> watermarks.
>> -		 */
>> -		if (IS_IVYBRIDGE(dev) &&
>> -		    needs_scaling(to_intel_plane_state(plane_state))
>> &&
>> -		    !needs_scaling(old_plane_state))
>> -			pipe_config->disable_lp_wm = true;
>> +	/*
>> +	 * WaCxSRDisabledForSpriteScaling:ivb
>> +	 *
>> +	 * cstate->update_wm was already set above, so this flag
>> will
>> +	 * take effect when we commit and program watermarks.
>> +	 */
>> +	if (plane->type == DRM_PLANE_TYPE_OVERLAY &&
>> IS_IVYBRIDGE(dev) &&
>> +	    needs_scaling(to_intel_plane_state(plane_state)) &&
>> +	    !needs_scaling(old_plane_state))
>> +		pipe_config->disable_lp_wm = true;
>>  
>> -		break;
>> -	}
>>  	return 0;
>>  }
>>  
>> @@ -13241,9 +13225,6 @@ static int intel_atomic_check(struct
>> drm_device *dev,
>>  		struct intel_crtc_state *pipe_config =
>>  			to_intel_crtc_state(crtc_state);
>>  
>> -		memset(&to_intel_crtc(crtc)->atomic, 0,
>> -		       sizeof(struct intel_crtc_atomic_commit));
>> -
>>  		/* Catch I915_MODE_FLAG_INHERITED */
>>  		if (crtc_state->mode.private_flags != crtc->state-
>>> mode.private_flags)
>>  			crtc_state->mode_changed = true;
>> @@ -13528,12 +13509,13 @@ static int intel_atomic_commit(struct
>> drm_device *dev,
>>  		if (!modeset)
>>  			intel_pre_plane_update(to_intel_crtc_state(c
>> rtc_state));
>>  
>> -		if (crtc->state->active && intel_crtc-
>>> atomic.update_fbc)
>> +		if ((modeset || update_pipe) && crtc->state->active)
> Same here: not easy for me to verify things are the same now. Even
> worse: the check for "is this atomic commit touching the primary
> plane?" is now written in a completely different way than the one
> introduced above. Maybe there's something we could do to make the code
> easier to read.
No, this is called now every time when the crtc has a modeset or update_pipe, even with no primary plane.
This is because atomic may set a mode without having a primary plane, and in that case fbc_enable may never be called.
Seemed like a bug in the original code..

~Maarten
Em Seg, 2016-02-15 às 15:31 +0100, Maarten Lankhorst escreveu:
> Op 12-02-16 om 14:56 schreef Zanoni, Paulo R:

> > Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:

> > > Factor out intel_fbc_supports_rotation

> > ^ not anymore

> > 

> > 

> > >  and use it in

> > > pre_plane_update as well. This leaves intel_crtc->atomic

> > > empty, so remove it too.

> > > 

> > > Changes since v1:

> > > - Add a intel_fbc_supports_rotation helper.

> > Changes since v2:

> > - No more need for rotation special-casing.

> > 

> > (I suppose you also have to edit the commit title to be v3)

> > 

> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c

> > > om>

> > > ---

> > >  drivers/gpu/drm/i915/intel_display.c | 58 +++++++++++++---------

> > > ----

> > > ----------

> > >  drivers/gpu/drm/i915/intel_drv.h     | 15 ----------

> > >  2 files changed, 20 insertions(+), 53 deletions(-)

> > > 

> > > diff --git a/drivers/gpu/drm/i915/intel_display.c

> > > b/drivers/gpu/drm/i915/intel_display.c

> > > index 54be8a255f1f..00cb261c6787 100644

> > > --- a/drivers/gpu/drm/i915/intel_display.c

> > > +++ b/drivers/gpu/drm/i915/intel_display.c

> > > @@ -4782,11 +4782,9 @@ static void intel_post_plane_update(struct

> > > intel_crtc_state *old_crtc_state)

> > >  {

> > >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-

> > > > base.crtc);

> > >  	struct drm_atomic_state *old_state = old_crtc_state-

> > > > base.state;

> > > -	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;

> > >  	struct intel_crtc_state *pipe_config =

> > >  		to_intel_crtc_state(crtc->base.state);

> > >  	struct drm_device *dev = crtc->base.dev;

> > > -	struct drm_i915_private *dev_priv = dev->dev_private;

> > >  	struct drm_plane *primary = crtc->base.primary;

> > >  	struct drm_plane_state *old_pri_state =

> > >  		drm_atomic_get_existing_plane_state(old_state,

> > > primary);

> > > @@ -4798,22 +4796,19 @@ static void

> > > intel_post_plane_update(struct

> > > intel_crtc_state *old_crtc_state)

> > >  	if (pipe_config->wm_changed && pipe_config->base.active)

> > >  		intel_update_watermarks(&crtc->base);

> > >  

> > > -	if (atomic->update_fbc)

> > > -		intel_fbc_post_update(crtc);

> > > -

> > >  	if (old_pri_state) {

> > For a code reader that is not very familiar with all the atomic

> > history

> > and its details, it's not trivial to conclude that "if

> > (drm_atomic_get_existing_plane_state(primary))", then we're

> > updating

> > the primary plane on this atomic commit. And before this patch,

> > it's

> > much much easier to conclude that update_fbc will be true when an

> > atomic update is touching the primary plane because that's

> > explicitly

> > stated by the cod.

> > 

> > So although "let's kill this redundant struct" sounds good, it

> > seems to

> > me that code clarity is going away with this patch, so I wonder if

> > the

> > benefits of the patch outweigh the downsides. Isn't there something

> > else we could do about this, such as some renaming, or adding some

> > function aliases or just extra commenting?

> > 

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

> > >  

> > > +		intel_fbc_post_update(crtc);

> > > +

> > >  		if (primary_state->visible &&

> > >  		    (needs_modeset(&pipe_config->base) ||

> > >  		     !old_primary_state->visible))

> > >  			intel_post_enable_primary(&crtc->base);

> > >  	}

> > > -

> > > -	memset(atomic, 0, sizeof(*atomic));

> > >  }

> > >  

> > >  static void intel_pre_plane_update(struct intel_crtc_state

> > > *old_crtc_state)

> > > @@ -4821,7 +4816,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;

> > > @@ -4830,17 +4824,17 @@ 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->update_fbc)

> > > -		intel_fbc_pre_update(crtc);

> > > -

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

> > > +		bool turn_off = old_primary_state->visible &&

> > > +		    (modeset || !primary_state->visible); 

> > Not really related to the patch, but ok to keep since it's

> > trivial...

> > 

> > > +

> > > +		intel_fbc_pre_update(crtc);

> > >  

> > > -		if (old_primary_state->visible &&

> > > -		    (modeset || !primary_state->visible))

> > > +		if (turn_off)

> > >  			intel_pre_disable_primary(&crtc->base);

> > >  	}

> > >  

> > > @@ -11822,27 +11816,17 @@ int

> > > intel_plane_atomic_calc_changes(struct

> > > drm_crtc_state *crtc_state,

> > >  	if (visible || was_visible)

> > >  		pipe_config->fb_bits |= to_intel_plane(plane)-

> > > > frontbuffer_bit;

> > >  

> > > -	switch (plane->type) {

> > > -	case DRM_PLANE_TYPE_PRIMARY:

> > > -		intel_crtc->atomic.update_fbc = true;

> > > -

> > > -		break;

> > > -	case DRM_PLANE_TYPE_CURSOR:

> > > -		break;

> > > -	case DRM_PLANE_TYPE_OVERLAY:

> > > -		/*

> > > -		 * WaCxSRDisabledForSpriteScaling:ivb

> > > -		 *

> > > -		 * cstate->update_wm was already set above, so

> > > this

> > > flag will

> > > -		 * take effect when we commit and program

> > > watermarks.

> > > -		 */

> > > -		if (IS_IVYBRIDGE(dev) &&

> > > -		    needs_scaling(to_intel_plane_state(plane_sta

> > > te))

> > > &&

> > > -		    !needs_scaling(old_plane_state))

> > > -			pipe_config->disable_lp_wm = true;

> > > +	/*

> > > +	 * WaCxSRDisabledForSpriteScaling:ivb

> > > +	 *

> > > +	 * cstate->update_wm was already set above, so this flag

> > > will

> > > +	 * take effect when we commit and program watermarks.

> > > +	 */

> > > +	if (plane->type == DRM_PLANE_TYPE_OVERLAY &&

> > > IS_IVYBRIDGE(dev) &&

> > > +	    needs_scaling(to_intel_plane_state(plane_state)) &&

> > > +	    !needs_scaling(old_plane_state))

> > > +		pipe_config->disable_lp_wm = true;

> > >  

> > > -		break;

> > > -	}

> > >  	return 0;

> > >  }

> > >  

> > > @@ -13241,9 +13225,6 @@ static int intel_atomic_check(struct

> > > drm_device *dev,

> > >  		struct intel_crtc_state *pipe_config =

> > >  			to_intel_crtc_state(crtc_state);

> > >  

> > > -		memset(&to_intel_crtc(crtc)->atomic, 0,

> > > -		       sizeof(struct intel_crtc_atomic_commit));

> > > -

> > >  		/* Catch I915_MODE_FLAG_INHERITED */

> > >  		if (crtc_state->mode.private_flags != crtc-

> > > >state-

> > > > mode.private_flags)

> > >  			crtc_state->mode_changed = true;

> > > @@ -13528,12 +13509,13 @@ static int intel_atomic_commit(struct

> > > drm_device *dev,

> > >  		if (!modeset)

> > >  			intel_pre_plane_update(to_intel_crtc_sta

> > > te(c

> > > rtc_state));

> > >  

> > > -		if (crtc->state->active && intel_crtc-

> > > > atomic.update_fbc)

> > > +		if ((modeset || update_pipe) && crtc->state-

> > > >active)

> > Same here: not easy for me to verify things are the same now. Even

> > worse: the check for "is this atomic commit touching the primary

> > plane?" is now written in a completely different way than the one

> > introduced above. Maybe there's something we could do to make the

> > code

> > easier to read.

> No, this is called now every time when the crtc has a modeset or

> update_pipe, even with no primary plane.

> This is because atomic may set a mode without having a primary plane,

> and in that case fbc_enable may never be called.

> Seemed like a bug in the original code..


I suppose that proves my point that this code with all those similar-
but-different boolean checks is really hard to read :)

FBC can only happen when there's a primary plane, so AFAIU even if we
don't call intel_fbc_enable here because the primary is disable, we'll
end up calling it later when we enable the primary plane.

Anyway, a change in behavior like this (which was supposed to be a bug
fix) should be on its own separate patch, not hidden inside a refactor.

> 

> ~Maarten

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Whenever there's an update to the primary plane,
fbc_pre_update and fbc_post_update are called. Kill off
intel_crtc->atomic.update_fbc and now that intel_crtc->atomic
is empty, kill it off too.

Changes since v1:
- Add a intel_fbc_supports_rotation helper.
Changes sinec v2:
- Remove intel_fbc_supports_rotation_helper.
- Remove unrelated changes.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
This version doesn't change intel_fbc_enable any more!
Em Seg, 2016-02-29 às 10:58 +0100, Maarten Lankhorst escreveu:
> Whenever there's an update to the primary plane,

> fbc_pre_update and fbc_post_update are called. Kill off

> intel_crtc->atomic.update_fbc and now that intel_crtc->atomic

> is empty, kill it off too.

> 

> Changes since v1:

> - Add a intel_fbc_supports_rotation helper.

> Changes sinec v2:

> - Remove intel_fbc_supports_rotation_helper.

> - Remove unrelated changes.

> 

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

> ---

> This version doesn't change intel_fbc_enable any more!


Ok, so I decided to test this.

First of all, patch 05 needs a rebase too. I did my own local rebase
and I suppose I got things right.

Second, after I did actual testing, I discovered that, contrary to my
previous belief, "old_pri_state" will also be valid when we're only
changing the sprite or cursor planes. So if we apply this patch, we'll
be calling pre_update+enable+post_update for every plane/cursor
enable/disable operation, which is not what we want since these are
unnecessary calls, as FBC only deals with the primary plane. I think we
need to fix this.

The tests I've been using to check this were:
- kms_frontbuffer_tracking/fbc-1p-primscrn-cur-indfb-onoff
- kms_frontbuffer_tracking/fbc-1p-primscrn-spr-indfb-onoff


> 

> diff --git a/drivers/gpu/drm/i915/intel_display.c

> b/drivers/gpu/drm/i915/intel_display.c

> index 78f72bef25aa..c8404092aee9 100644

> --- a/drivers/gpu/drm/i915/intel_display.c

> +++ b/drivers/gpu/drm/i915/intel_display.c

> @@ -4789,11 +4789,9 @@ static void intel_post_plane_update(struct

> intel_crtc_state *old_crtc_state)

>  {

>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-

> >base.crtc);

>  	struct drm_atomic_state *old_state = old_crtc_state-

> >base.state;

> -	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;

>  	struct intel_crtc_state *pipe_config =

>  		to_intel_crtc_state(crtc->base.state);

>  	struct drm_device *dev = crtc->base.dev;

> -	struct drm_i915_private *dev_priv = dev->dev_private;

>  	struct drm_plane *primary = crtc->base.primary;

>  	struct drm_plane_state *old_pri_state =

>  		drm_atomic_get_existing_plane_state(old_state,

> primary);

> @@ -4805,22 +4803,19 @@ static void intel_post_plane_update(struct

> intel_crtc_state *old_crtc_state)

>  	if (pipe_config->wm_changed && pipe_config->base.active)

>  		intel_update_watermarks(&crtc->base);

>  

> -	if (atomic->update_fbc)

> -		intel_fbc_post_update(crtc);

> -

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

>  

> +		intel_fbc_post_update(crtc);

> +

>  		if (primary_state->visible &&

>  		    (needs_modeset(&pipe_config->base) ||

>  		     !old_primary_state->visible))

>  			intel_post_enable_primary(&crtc->base);

>  	}

> -

> -	memset(atomic, 0, sizeof(*atomic));

>  }

>  

>  static void intel_pre_plane_update(struct intel_crtc_state

> *old_crtc_state)

> @@ -4828,7 +4823,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;

> @@ -4837,15 +4831,14 @@ 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->update_fbc)

> -		intel_fbc_pre_update(crtc);

> -

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

>  

> +		intel_fbc_pre_update(crtc);

> +

>  		if (old_primary_state->visible &&

>  		    (modeset || !primary_state->visible))

>  			intel_pre_disable_primary(&crtc->base);

> @@ -11877,27 +11870,17 @@ int intel_plane_atomic_calc_changes(struct

> drm_crtc_state *crtc_state,

>  	if (visible || was_visible)

>  		pipe_config->fb_bits |= to_intel_plane(plane)-

> >frontbuffer_bit;

>  

> -	switch (plane->type) {

> -	case DRM_PLANE_TYPE_PRIMARY:

> -		intel_crtc->atomic.update_fbc = true;

> -

> -		break;

> -	case DRM_PLANE_TYPE_CURSOR:

> -		break;

> -	case DRM_PLANE_TYPE_OVERLAY:

> -		/*

> -		 * WaCxSRDisabledForSpriteScaling:ivb

> -		 *

> -		 * cstate->update_wm was already set above, so this

> flag will

> -		 * take effect when we commit and program

> watermarks.

> -		 */

> -		if (IS_IVYBRIDGE(dev) &&

> -		    needs_scaling(to_intel_plane_state(plane_state))

> &&

> -		    !needs_scaling(old_plane_state))

> -			pipe_config->disable_lp_wm = true;

> +	/*

> +	 * WaCxSRDisabledForSpriteScaling:ivb

> +	 *

> +	 * cstate->update_wm was already set above, so this flag

> will

> +	 * take effect when we commit and program watermarks.

> +	 */

> +	if (plane->type == DRM_PLANE_TYPE_OVERLAY &&

> IS_IVYBRIDGE(dev) &&

> +	    needs_scaling(to_intel_plane_state(plane_state)) &&

> +	    !needs_scaling(old_plane_state))

> +		pipe_config->disable_lp_wm = true;

>  

> -		break;

> -	}

>  	return 0;

>  }

>  

> @@ -13296,9 +13279,6 @@ static int intel_atomic_check(struct

> drm_device *dev,

>  		struct intel_crtc_state *pipe_config =

>  			to_intel_crtc_state(crtc_state);

>  

> -		memset(&to_intel_crtc(crtc)->atomic, 0,

> -		       sizeof(struct intel_crtc_atomic_commit));

> -

>  		/* Catch I915_MODE_FLAG_INHERITED */

>  		if (crtc_state->mode.private_flags != crtc->state-

> >mode.private_flags)

>  			crtc_state->mode_changed = true;

> @@ -13606,7 +13586,8 @@ static int intel_atomic_commit(struct

> drm_device *dev,

>  		if (!modeset)

>  			intel_pre_plane_update(to_intel_crtc_state(c

> rtc_state));

>  

> -		if (crtc->state->active && intel_crtc-

> >atomic.update_fbc)

> +		if (crtc->state->active &&

> +		    drm_atomic_get_existing_plane_state(state, crtc-

> >primary))

>  			intel_fbc_enable(intel_crtc);

>  

>  		if (crtc->state->active &&

> diff --git a/drivers/gpu/drm/i915/intel_drv.h

> b/drivers/gpu/drm/i915/intel_drv.h

> index 058bb10e53ae..5e5b64fff30d 100644

> --- a/drivers/gpu/drm/i915/intel_drv.h

> +++ b/drivers/gpu/drm/i915/intel_drv.h

> @@ -538,19 +538,6 @@ struct intel_mmio_flip {

>  	unsigned int rotation;

>  };

>  

> -/*

> - * Tracking of operations that need to be performed at the

> beginning/end of an

> - * atomic commit, outside the atomic section where interrupts are

> disabled.

> - * These are generally operations that grab mutexes or might

> otherwise sleep

> - * and thus can't be run with interrupts disabled.

> - */

> -struct intel_crtc_atomic_commit {

> -	/* Sleepable operations to perform before commit */

> -

> -	/* Sleepable operations to perform after commit */

> -	bool update_fbc;

> -};

> -

>  struct intel_crtc {

>  	struct drm_crtc base;

>  	enum pipe pipe;

> @@ -610,8 +597,6 @@ struct intel_crtc {

>  		int scanline_start;

>  	} debug;

>  

> -	struct intel_crtc_atomic_commit atomic;

> -

>  	/* scalers available on this crtc */

>  	int num_scalers;

>  

> 

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 29-02-16 om 22:06 schreef Zanoni, Paulo R:
> Em Seg, 2016-02-29 às 10:58 +0100, Maarten Lankhorst escreveu:
>> Whenever there's an update to the primary plane,
>> fbc_pre_update and fbc_post_update are called. Kill off
>> intel_crtc->atomic.update_fbc and now that intel_crtc->atomic
>> is empty, kill it off too.
>>
>> Changes since v1:
>> - Add a intel_fbc_supports_rotation helper.
>> Changes sinec v2:
>> - Remove intel_fbc_supports_rotation_helper.
>> - Remove unrelated changes.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> This version doesn't change intel_fbc_enable any more!
> Ok, so I decided to test this.
>
> First of all, patch 05 needs a rebase too. I did my own local rebase
> and I suppose I got things right.
>
> Second, after I did actual testing, I discovered that, contrary to my
> previous belief, "old_pri_state" will also be valid when we're only
> changing the sprite or cursor planes. So if we apply this patch, we'll
> be calling pre_update+enable+post_update for every plane/cursor
> enable/disable operation, which is not what we want since these are
> unnecessary calls, as FBC only deals with the primary plane. I think we
> need to fix this.
This is a bug addressed by the next patch.

drm/i915: Do not compute watermarks on a noop.

I'll send the improved version for that. I didn't do it yet because I was waiting for the ILK wm patch to land.

~Maarten