Re: [RFC 2/2] [INCOMPLETE] drm/i915: Split up crtc_state into uapi and hw

[Manasi Navare <manasi.d.navare@intel.com>]

Here it looks like, we are changing all the helpers to accept intel_crtc_state as an argument
and then use the intel_crtc_state->hw and ->uapi whereever needed inside the function.

But with seperating the drm_crtc_state now into hw and uapi, wasnt one of the goals
to retain teh helpers taking drm_crtc_state as an argument and not having to change
that all through the i915 driver?

Manasi

On Fri, Jun 14, 2019 at 10:21:15PM +0200, Maarten Lankhorst wrote:
>  Use uapi for drm, and keep the internal state in hw.
>  
>  This patch is incomplete, just want feedback on the direction I'm going.
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  | 13 ++--
>  drivers/gpu/drm/i915/intel_display.c | 88 +++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>  3 files changed, 57 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 1f891b554ead..36fbd408ba81 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -185,12 +185,14 @@ struct drm_crtc_state *
>  intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  {
>  	struct intel_crtc_state *crtc_state;
> +	const struct intel_crtc_state *old_crtc_state = to_intel_crtc_state(crtc->state);
>  
> -	crtc_state = kmemdup(crtc->state, sizeof(*crtc_state), GFP_KERNEL);
> +	crtc_state = kmemdup(old_crtc_state, sizeof(*crtc_state), GFP_KERNEL);
>  	if (!crtc_state)
>  		return NULL;
>  
> -	__drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state->hw);
> +	__drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state->uapi);
> +	memset(&crtc_state->hw, 0, sizeof(crtc_state->hw));
>  
>  	crtc_state->update_pipe = false;
>  	crtc_state->disable_lp_wm = false;
> @@ -203,7 +205,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  	crtc_state->fb_bits = 0;
>  	crtc_state->update_planes = 0;
>  
> -	return &crtc_state->hw;
> +	return &crtc_state->uapi;
>  }
>  
>  /**
> @@ -218,7 +220,10 @@ void
>  intel_crtc_destroy_state(struct drm_crtc *crtc,
>  			 struct drm_crtc_state *state)
>  {
> -	drm_atomic_helper_crtc_destroy_state(crtc, state);
> +	struct intel_crtc_state *crtc_state = to_intel_crtc_state(state);
> +
> +	__drm_atomic_helper_crtc_destroy_state(crtc, &crtc_state->uapi);
> +	kfree(crtc_state);
>  }
>  
>  static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_state,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9aa273fb5415..2de978ab8fc2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -514,9 +514,9 @@ icl_wa_scalerclkgating(struct drm_i915_private *dev_priv, enum pipe pipe,
>  }
>  
>  static bool
> -needs_modeset(const struct drm_crtc_state *state)
> +needs_modeset(const struct intel_crtc_state *state)
>  {
> -	return drm_atomic_crtc_needs_modeset(state);
> +	return drm_atomic_crtc_needs_modeset(&state->hw);
>  }
>  
>  /*
> @@ -5795,7 +5795,7 @@ static bool hsw_pre_update_disable_ips(const struct intel_crtc_state *old_crtc_s
>  	if (!old_crtc_state->ips_enabled)
>  		return false;
>  
> -	if (needs_modeset(&new_crtc_state->hw))
> +	if (needs_modeset(new_crtc_state))
>  		return true;
>  
>  	/*
> @@ -5822,7 +5822,7 @@ static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_s
>  	if (!new_crtc_state->ips_enabled)
>  		return false;
>  
> -	if (needs_modeset(&new_crtc_state->hw))
> +	if (needs_modeset(new_crtc_state))
>  		return true;
>  
>  	/*
> @@ -5899,7 +5899,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>  		intel_fbc_post_update(crtc);
>  
>  		if (new_primary_state->visible &&
> -		    (needs_modeset(&pipe_config->hw) ||
> +		    (needs_modeset(pipe_config) ||
>  		     !old_primary_state->visible))
>  			intel_post_enable_primary(&crtc->base, pipe_config);
>  	}
> @@ -5923,7 +5923,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>  	struct drm_plane *primary = crtc->base.primary;
>  	struct drm_plane_state *old_primary_state =
>  		drm_atomic_get_old_plane_state(old_state, primary);
> -	bool modeset = needs_modeset(&pipe_config->hw);
> +	bool modeset = needs_modeset(pipe_config);
>  	struct intel_atomic_state *old_intel_state =
>  		to_intel_atomic_state(old_state);
>  
> @@ -5983,7 +5983,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>  	 * If we're doing a modeset, we're done.  No need to do any pre-vblank
>  	 * watermark programming here.
>  	 */
> -	if (needs_modeset(&pipe_config->hw))
> +	if (needs_modeset(pipe_config))
>  		return;
>  
>  	/*
> @@ -11338,7 +11338,7 @@ int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_stat
>  	struct intel_plane *plane = to_intel_plane(plane_state->plane);
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	bool mode_changed = needs_modeset(crtc_state);
> +	bool mode_changed = needs_modeset(pipe_config);
>  	bool was_crtc_enabled = old_crtc_state->hw.active;
>  	bool is_crtc_enabled = crtc_state->active;
>  	bool turn_off, turn_on, visible, was_visible;
> @@ -11607,7 +11607,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc_state);
>  	int ret;
> -	bool mode_changed = needs_modeset(crtc_state);
> +	bool mode_changed = needs_modeset(pipe_config);
>  
>  	if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv) &&
>  	    mode_changed && !crtc_state->active)
> @@ -13095,7 +13095,7 @@ intel_modeset_verify_crtc(struct drm_crtc *crtc,
>  			  struct drm_crtc_state *old_state,
>  			  struct drm_crtc_state *new_state)
>  {
> -	if (!needs_modeset(new_state) &&
> +	if (!needs_modeset(to_intel_crtc_state(new_state)) &&
>  	    !to_intel_crtc_state(new_state)->update_pipe)
>  		return;
>  
> @@ -13187,7 +13187,7 @@ static void intel_modeset_clear_plls(struct intel_atomic_state *state)
>  		struct intel_shared_dpll *old_dpll =
>  			old_crtc_state->shared_dpll;
>  
> -		if (!needs_modeset(&new_crtc_state->hw))
> +		if (!needs_modeset(new_crtc_state))
>  			continue;
>  
>  		new_crtc_state->shared_dpll = NULL;
> @@ -13217,7 +13217,7 @@ static int haswell_mode_set_planes_workaround(struct intel_atomic_state *state)
>  	/* look at all crtc's that are going to be enabled in during modeset */
>  	for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
>  		if (!crtc_state->hw.active ||
> -		    !needs_modeset(&crtc_state->hw))
> +		    !needs_modeset(crtc_state))
>  			continue;
>  
>  		if (first_crtc_state) {
> @@ -13242,7 +13242,7 @@ static int haswell_mode_set_planes_workaround(struct intel_atomic_state *state)
>  		crtc_state->hsw_workaround_pipe = INVALID_PIPE;
>  
>  		if (!crtc_state->hw.active ||
> -		    needs_modeset(&crtc_state->hw))
> +		    needs_modeset(crtc_state))
>  			continue;
>  
>  		/* 2 or more enabled crtcs means no need for w/a */
> @@ -13276,32 +13276,32 @@ static int intel_lock_all_pipes(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> -static int intel_modeset_all_pipes(struct drm_atomic_state *state)
> +static int intel_modeset_all_pipes(struct intel_atomic_state *state)
>  {
> -	struct drm_crtc *crtc;
> +	struct intel_crtc *crtc;
>  
>  	/*
>  	 * Add all pipes to the state, and force
>  	 * a modeset on all the active ones.
>  	 */
>  	for_each_crtc(state->dev, crtc) {
> -		struct drm_crtc_state *crtc_state;
> +		struct intel_crtc_state *crtc_state;
>  		int ret;
>  
> -		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +		crtc_state = intel_atomic_get_crtc_state(state, crtc);
>  		if (IS_ERR(crtc_state))
>  			return PTR_ERR(crtc_state);
>  
> -		if (!crtc_state->active || needs_modeset(crtc_state))
> +		if (!crtc_state->hw.active || needs_modeset(crtc_state))
>  			continue;
>  
>  		crtc_state->mode_changed = true;
>  
> -		ret = drm_atomic_add_affected_connectors(state, crtc);
> +		ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base);
>  		if (ret)
>  			return ret;
>  
> -		ret = drm_atomic_add_affected_planes(state, crtc);
> +		ret = drm_atomic_add_affected_planes(&state->base, &crtc->base);
>  		if (ret)
>  			return ret;
>  	}
> @@ -13369,12 +13369,12 @@ static int intel_modeset_checks(struct intel_atomic_state *state)
>  		}
>  
>  		if (is_power_of_2(state->active_crtcs)) {
> -			struct drm_crtc *crtc;
> -			struct drm_crtc_state *crtc_state;
> +			struct intel_crtc *crtc;
> +			struct intel_crtc_state *crtc_state;
>  
>  			pipe = ilog2(state->active_crtcs);
> -			crtc = &intel_get_crtc_for_pipe(dev_priv, pipe)->base;
> -			crtc_state = drm_atomic_get_new_crtc_state(&state->base, crtc);
> +			crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> +			crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>  			if (crtc_state && needs_modeset(crtc_state))
>  				pipe = INVALID_PIPE;
>  		} else {
> @@ -13393,7 +13393,7 @@ static int intel_modeset_checks(struct intel_atomic_state *state)
>  			state->cdclk.pipe = pipe;
>  		} else if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
>  						     &state->cdclk.actual)) {
> -			ret = intel_modeset_all_pipes(&state->base);
> +			ret = intel_modeset_all_pipes(state);
>  			if (ret < 0)
>  				return ret;
>  
> @@ -13451,6 +13451,8 @@ static int intel_atomic_check(struct drm_device *dev,
>  	/* Catch I915_MODE_FLAG_INHERITED */
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
> +		new_intel_crtc_state->hw = new_intel_crtc_state->uapi;
> +
>  		if (new_crtc_state->hw.mode.private_flags !=
>  		    old_crtc_state->hw.mode.private_flags)
>  			new_crtc_state->hw.mode_changed = true;
> @@ -13462,7 +13464,7 @@ static int intel_atomic_check(struct drm_device *dev,
>  
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
> -		if (!needs_modeset(&new_crtc_state->hw))
> +		if (!needs_modeset(new_crtc_state))
>  			continue;
>  
>  		if (!new_crtc_state->hw.enable) {
> @@ -13480,7 +13482,7 @@ static int intel_atomic_check(struct drm_device *dev,
>  			new_crtc_state->update_pipe = true;
>  		}
>  
> -		if (needs_modeset(&new_crtc_state->hw))
> +		if (needs_modeset(new_crtc_state))
>  			any_ms = true;
>  	}
>  
> @@ -13515,12 +13517,12 @@ static int intel_atomic_check(struct drm_device *dev,
>  
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
> -		if (!needs_modeset(&new_crtc_state->hw) &&
> +		if (!needs_modeset(new_crtc_state) &&
>  		    !new_crtc_state->update_pipe)
>  			continue;
>  
>  		intel_dump_pipe_config(new_crtc_state, state,
> -				       needs_modeset(&new_crtc_state->hw) ?
> +				       needs_modeset(new_crtc_state) ?
>  				       "[modeset]" : "[fastset]");
>  	}
>  
> @@ -13567,7 +13569,7 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
> -	bool modeset = needs_modeset(new_crtc_state);
> +	bool modeset = needs_modeset(pipe_config);
>  	struct intel_plane_state *new_plane_state =
>  		intel_atomic_get_new_plane_state(to_intel_atomic_state(state),
>  						 to_intel_plane(crtc->primary));
> @@ -13776,20 +13778,23 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
>  		intel_crtc = to_intel_crtc(crtc);
>  
> -		if (needs_modeset(new_crtc_state) ||
> -		    to_intel_crtc_state(new_crtc_state)->update_pipe) {
> +		/* Copy for drm_atomic_helper_setup_commit() */
> +		new_intel_crtc_state->hw = new_intel_crtc_state->uapi;
> +
> +		if (needs_modeset(new_intel_crtc_state) ||
> +		    new_intel_crtc_state->update_pipe) {
>  
>  			put_domains[intel_crtc->pipe] =
>  				modeset_get_crtc_power_domains(crtc,
>  					new_intel_crtc_state);
>  		}
>  
> -		if (!needs_modeset(new_crtc_state))
> +		if (!needs_modeset(new_intel_crtc_state))
>  			continue;
>  
>  		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
>  
> -		if (old_crtc_state->active) {
> +		if (new_intel_crtc_state->hw.active) {
>  			intel_crtc_disable_planes(intel_state, intel_crtc);
>  
>  			/*
> @@ -13878,8 +13883,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>  		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
>  
> -		if (new_crtc_state->active &&
> -		    !needs_modeset(new_crtc_state) &&
> +		if (new_intel_crtc_state->hw.active &&
> +		    !needs_modeset(new_intel_crtc_state) &&
>  		    (new_intel_crtc_state->hw.color_mgmt_changed ||
>  		     new_intel_crtc_state->update_pipe))
>  			intel_color_load_luts(new_intel_crtc_state);
> @@ -14238,9 +14243,9 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  	int ret;
>  
>  	if (old_obj) {
> -		struct drm_crtc_state *crtc_state =
> -			drm_atomic_get_new_crtc_state(new_state->state,
> -						      plane->state->crtc);
> +		struct intel_crtc_state *crtc_state =
> +			intel_atomic_get_new_crtc_state(intel_state,
> +							plane->state->crtc);
>  
>  		/* Big Hammer, we also need to ensure that any pending
>  		 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
> @@ -14401,7 +14406,7 @@ static void intel_begin_crtc_commit(struct intel_atomic_state *state,
>  		intel_atomic_get_old_crtc_state(state, crtc);
>  	struct intel_crtc_state *new_crtc_state =
>  		intel_atomic_get_new_crtc_state(state, crtc);
> -	bool modeset = needs_modeset(&new_crtc_state->hw);
> +	bool modeset = needs_modeset(new_crtc_state);
>  
>  	/* Perform vblank evasion around commit operation */
>  	intel_pipe_update_start(new_crtc_state);
> @@ -14454,7 +14459,7 @@ static void intel_finish_crtc_commit(struct intel_atomic_state *state,
>  	intel_pipe_update_end(new_crtc_state);
>  
>  	if (new_crtc_state->update_pipe &&
> -	    !needs_modeset(&new_crtc_state->hw) &&
> +	    !needs_modeset(new_crtc_state) &&
>  	    old_crtc_state->hw.mode.private_flags & I915_MODE_FLAG_INHERITED)
>  		intel_crtc_arm_fifo_underrun(crtc, new_crtc_state);
>  }
> @@ -14608,6 +14613,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  		ret = -ENOMEM;
>  		goto out_free;
>  	}
> +	new_crtc_state->hw = new_crtc_state->uapi;
>  
>  	drm_atomic_set_fb_for_plane(new_plane_state, fb);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ec50f9167161..d39ef2443ce9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -713,7 +713,7 @@ enum intel_output_format {
>  };
>  
>  struct intel_crtc_state {
> -	struct drm_crtc_state hw;
> +	struct drm_crtc_state uapi, hw;
>  
>  	/**
>  	 * quirks - bitfield with hw state readout quirks
> -- 
> 2.20.1
> 
> _______________________________________________
> 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