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

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

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