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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx