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

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

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