Re: [CI 06/12] drm/i915: Complete crtc hw/uapi split, v5.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 30, 2019 at 03:26:51PM +0100, Maarten Lankhorst wrote:
> Now that we separated everything into uapi and hw, it's
> time to make the split definitive. Remove the union and
> make a copy of the hw state on modeset and fastset.
> 
> Color blobs are copied in crtc atomic_check(), right
> before color management is checked.
> 
> Changes since v1:
> - Copy all blobs immediately after drm_atomic_helper_check_modeset().
> - Clear crtc_state->hw on disable, instead of using clear_intel_crtc_state().
> Changes since v2:
> - Use intel_crtc_free_hw_state + clear in intel_crtc_disable_noatomic().
> - Make a intel_crtc_prepare_state() function that clears the crtc_state
>   and copies hw members.
> - Remove setting uapi.adjusted_mode, we now have a direct call to
>   drm_calc_timestamping_constants().
> Changes since v3:
> - Rename prefix copy_hw_to_uapi_state() with intel_crtc.
> - Copy color blobs to uapi as well.
> - Add a intel_crtc_copy_uapi_to_hw_state_nomodeset() function for clarity.
> Changes since v4:
> - Copy hw.adjusted_mode back to uapi.adjusted_mode, to shut up
>   the call to drm_calc_timestamping_constants() in
>   drm_atomic_helper_update_legacy_modeset_state().
> - Use drm_property_replace_blob (Ville).
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic.c   | 31 ++++++++
>  drivers/gpu/drm/i915/display/intel_atomic.h   |  2 +
>  drivers/gpu/drm/i915/display/intel_display.c  | 78 +++++++++++++++----
>  .../drm/i915/display/intel_display_types.h    |  9 ++-
>  4 files changed, 103 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index 48964f33c0c1..3301c178da03 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -195,6 +195,14 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  
>  	__drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state->uapi);
>  
> +	/* copy color blobs */
> +	if (crtc_state->hw.degamma_lut)
> +		drm_property_blob_get(crtc_state->hw.degamma_lut);
> +	if (crtc_state->hw.ctm)
> +		drm_property_blob_get(crtc_state->hw.ctm);
> +	if (crtc_state->hw.gamma_lut)
> +		drm_property_blob_get(crtc_state->hw.gamma_lut);
> +
>  	crtc_state->update_pipe = false;
>  	crtc_state->disable_lp_wm = false;
>  	crtc_state->disable_cxsr = false;
> @@ -208,6 +216,28 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  	return &crtc_state->uapi;
>  }
>  
> +static void intel_crtc_put_color_blobs(struct intel_crtc_state *crtc_state)
> +{
> +	drm_property_blob_put(crtc_state->hw.degamma_lut);
> +	drm_property_blob_put(crtc_state->hw.gamma_lut);
> +	drm_property_blob_put(crtc_state->hw.ctm);
> +}
> +
> +void intel_crtc_free_hw_state(struct intel_crtc_state *crtc_state)
> +{
> +	intel_crtc_put_color_blobs(crtc_state);
> +}
> +
> +void intel_crtc_copy_color_blobs(struct intel_crtc_state *crtc_state)
> +{
> +	drm_property_replace_blob(&crtc_state->hw.degamma_lut,
> +				  crtc_state->uapi.degamma_lut);
> +	drm_property_replace_blob(&crtc_state->hw.gamma_lut,
> +				  crtc_state->uapi.gamma_lut);
> +	drm_property_replace_blob(&crtc_state->hw.ctm,
> +				  crtc_state->uapi.ctm);
> +}
> +
>  /**
>   * intel_crtc_destroy_state - destroy crtc state
>   * @crtc: drm crtc
> @@ -223,6 +253,7 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
>  	struct intel_crtc_state *crtc_state = to_intel_crtc_state(state);
>  
>  	__drm_atomic_helper_crtc_destroy_state(&crtc_state->uapi);
> +	intel_crtc_free_hw_state(crtc_state);
>  	kfree(crtc_state);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
> index 49d5cb1b9e0a..7b49623419ba 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> @@ -36,6 +36,8 @@ intel_digital_connector_duplicate_state(struct drm_connector *connector);
>  struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
>  void intel_crtc_destroy_state(struct drm_crtc *crtc,
>  			       struct drm_crtc_state *state);
> +void intel_crtc_free_hw_state(struct intel_crtc_state *crtc_state);
> +void intel_crtc_copy_color_blobs(struct intel_crtc_state *crtc_state);
>  struct drm_atomic_state *intel_atomic_state_alloc(struct drm_device *dev);
>  void intel_atomic_state_clear(struct drm_atomic_state *state);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 66223701ef78..e30f467eec67 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7185,6 +7185,8 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
>  	crtc->enabled = false;
>  	crtc->state->connector_mask = 0;
>  	crtc->state->encoder_mask = 0;
> +	intel_crtc_free_hw_state(crtc_state);
> +	memset(&crtc_state->hw, 0, sizeof(crtc_state->hw));
>  
>  	for_each_encoder_on_crtc(crtc->dev, crtc, encoder)
>  		encoder->base.crtc = NULL;
> @@ -12560,8 +12562,40 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
>  	return ret;
>  }
>  
> +static void
> +intel_crtc_copy_uapi_to_hw_state_nomodeset(struct intel_crtc_state *crtc_state)
> +{
> +	intel_crtc_copy_color_blobs(crtc_state);
> +}
> +
> +static void
> +intel_crtc_copy_uapi_to_hw_state(struct intel_crtc_state *crtc_state)
> +{
> +	crtc_state->hw.enable = crtc_state->uapi.enable;
> +	crtc_state->hw.active = crtc_state->uapi.active;
> +	crtc_state->hw.mode = crtc_state->uapi.mode;
> +	crtc_state->hw.adjusted_mode = crtc_state->uapi.adjusted_mode;
> +	intel_crtc_copy_uapi_to_hw_state_nomodeset(crtc_state);
> +}
> +
> +static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state)
> +{
> +	crtc_state->uapi.enable = crtc_state->hw.enable;
> +	crtc_state->uapi.active = crtc_state->hw.active;
> +	crtc_state->uapi.mode = crtc_state->hw.mode;
> +	crtc_state->uapi.adjusted_mode = crtc_state->hw.adjusted_mode;
> +
> +	/* copy color blobs to uapi */
> +	drm_property_replace_blob(&crtc_state->uapi.degamma_lut,
> +				  crtc_state->hw.degamma_lut);
> +	drm_property_replace_blob(&crtc_state->uapi.gamma_lut,
> +				  crtc_state->hw.gamma_lut);
> +	drm_property_replace_blob(&crtc_state->uapi.ctm,
> +				  crtc_state->hw.ctm);
> +}
> +
>  static int
> -clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> +intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *dev_priv =
>  		to_i915(crtc_state->uapi.crtc->dev);
> @@ -12571,11 +12605,15 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	if (!saved_state)
>  		return -ENOMEM;
>  
> +	/* free the old crtc_state->hw members */
> +	intel_crtc_free_hw_state(crtc_state);
> +
>  	/* FIXME: before the switch to atomic started, a new pipe_config was
>  	 * kzalloc'd. Code that depends on any field being zero should be
>  	 * fixed, so that the crtc_state can be safely duplicated. For now,
>  	 * only fields that are know to not cause problems are preserved. */
>  
> +	saved_state->uapi = crtc_state->uapi;
>  	saved_state->scaler_state = crtc_state->scaler_state;
>  	saved_state->shared_dpll = crtc_state->shared_dpll;
>  	saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> @@ -12593,14 +12631,11 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  		saved_state->sync_mode_slaves_mask =
>  			crtc_state->sync_mode_slaves_mask;
>  
> -	/* Keep base drm_crtc_state intact, only clear our extended struct */
> -	BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> -	BUILD_BUG_ON(offsetof(struct intel_crtc_state, uapi));
> -	BUILD_BUG_ON(offsetof(struct intel_crtc_state, hw));
> -	memcpy(&crtc_state->uapi + 1, &saved_state->uapi + 1,
> -	       sizeof(*crtc_state) - sizeof(crtc_state->uapi));
> -
> +	memcpy(crtc_state, saved_state, sizeof(*crtc_state));
>  	kfree(saved_state);
> +
> +	intel_crtc_copy_uapi_to_hw_state(crtc_state);
> +
>  	return 0;
>  }
>  
> @@ -12616,10 +12651,6 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>  	int i;
>  	bool retry = true;
>  
> -	ret = clear_intel_crtc_state(pipe_config);
> -	if (ret)
> -		return ret;
> -
>  	pipe_config->cpu_transcoder =
>  		(enum transcoder) to_intel_crtc(crtc)->pipe;
>  
> @@ -12747,6 +12778,12 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>  	DRM_DEBUG_KMS("hw max bpp: %i, pipe bpp: %i, dithering: %i\n",
>  		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>  
> +	/*
> +	 * Make drm_calc_timestamping_constants in
> +	 * drm_atomic_helper_update_legacy_modeset_state() happy
> +	 */
> +	pipe_config->uapi.adjusted_mode = pipe_config->hw.adjusted_mode;
> +

Yeah, no choice but do this now. Hopefully we can nuke this soon.

>  	return 0;
>  }
>  
> @@ -13476,6 +13513,8 @@ verify_crtc_state(struct intel_crtc *crtc,
>  
>  	state = old_crtc_state->uapi.state;
>  	__drm_atomic_helper_crtc_destroy_state(&old_crtc_state->uapi);
> +	intel_crtc_free_hw_state(old_crtc_state);
> +
>  	pipe_config = old_crtc_state;
>  	memset(pipe_config, 0, sizeof(*pipe_config));
>  	pipe_config->uapi.crtc = &crtc->base;
> @@ -14010,14 +14049,24 @@ 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))
> +		if (!needs_modeset(new_crtc_state)) {
> +			/* Light copy */
> +			intel_crtc_copy_uapi_to_hw_state_nomodeset(new_crtc_state);
> +
>  			continue;
> +		}
>  
>  		if (!new_crtc_state->uapi.enable) {
> +			intel_crtc_copy_uapi_to_hw_state(new_crtc_state);
> +
>  			any_ms = true;
>  			continue;
>  		}
>  
> +		ret = intel_crtc_prepare_cleared_state(new_crtc_state);
> +		if (ret)
> +			goto fail;
> +
>  		ret = intel_modeset_pipe_config(new_crtc_state);
>  		if (ret)
>  			goto fail;
> @@ -17291,6 +17340,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  			to_intel_crtc_state(crtc->base.state);
>  
>  		__drm_atomic_helper_crtc_destroy_state(&crtc_state->uapi);
> +		intel_crtc_free_hw_state(crtc_state);
>  		memset(crtc_state, 0, sizeof(*crtc_state));
>  		__drm_atomic_helper_crtc_reset(&crtc->base, &crtc_state->uapi);
>  
> @@ -17419,6 +17469,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  			 * set a flag to indicate that a full recalculation is
>  			 * needed on the next commit.
>  			 */
> +			crtc_state->hw.mode = crtc->base.mode;
>  			crtc_state->hw.mode.private_flags = I915_MODE_FLAG_INHERITED;
>  
>  			intel_crtc_compute_pixel_rate(crtc_state);
> @@ -17467,6 +17518,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  
>  		intel_bw_crtc_update(bw_state, crtc_state);
>  
> +		intel_crtc_copy_hw_to_uapi_state(crtc_state);

I pushed the crtc->base.mode nuking, so the readout will need a slight
adjustment. I belive it should just look like:

	pipe_config_to_mode(hw.adjusted_mode);

	hw.mode = hw.adjusted_mode;
	hw.mode.hdisplay = ...;
	hw.mode.vdisplay = ...;
...
	intel_crtc_copy_hw_to_uapi_state();

And we'll need to make intel_crtc_copy_hw_to_uapi_state()
use drm_atomic_set_mode_for_crtc() instead of doing a
straight copy, otherwise the mode_blob will get out of sync.

Otherwise lgtm.

>  		intel_pipe_config_sanity_check(dev_priv, crtc_state);
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index e84343d3bf8d..9319ca682105 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -757,8 +757,6 @@ enum intel_output_format {
>  };
>  
>  struct intel_crtc_state {
> -	union {
> -	struct drm_crtc_state base;
>  	/*
>  	 * uapi (drm) state. This is the software state shown to userspace.
>  	 * In particular, the following members are used for bookkeeping:
> @@ -781,8 +779,11 @@ struct intel_crtc_state {
>  	 *
>  	 * During initial hw readout, they need to be copied to uapi.
>  	 */
> -	struct drm_crtc_state hw;
> -	};
> +	struct {
> +		bool active, enable;
> +		struct drm_property_blob *degamma_lut, *gamma_lut, *ctm;
> +		struct drm_display_mode mode, adjusted_mode;
> +	} hw;
>  
>  	/**
>  	 * quirks - bitfield with hw state readout quirks
> -- 
> 2.23.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx