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

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

On Thu, Oct 31, 2019 at 12:26:04PM +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).
> Changes since v5:
> - Use hw->mode in intel_modeset_readout_hw_state(). (Ville)
> - Copy to uapi.mode using drm_atomic_set_mode_for_crtc(). (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  | 90 +++++++++++++++----
>  .../drm/i915/display/intel_display_types.h    |  9 +-
>  4 files changed, 109 insertions(+), 23 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 4a9dc14f2ee2..373cbbbb57a4 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;
> @@ -12557,8 +12559,41 @@ 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;
> +	WARN_ON(drm_atomic_set_mode_for_crtc(&crtc_state->uapi, &crtc_state->hw.mode) < 0);
> +
> +	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);
> @@ -12568,11 +12603,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;
> @@ -12590,14 +12629,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;
>  }
>  
> @@ -12613,10 +12649,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;
>  
> @@ -12744,6 +12776,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;
> +
>  	return 0;
>  }
>  
> @@ -13473,6 +13511,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;
> @@ -14007,14 +14047,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;
> @@ -17285,6 +17335,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);
>  
> @@ -17396,15 +17447,14 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  		int min_cdclk = 0;
>  
>  		if (crtc_state->hw.active) {
> -			struct drm_display_mode mode;
> +			struct drm_display_mode *mode = &crtc_state->hw.mode;
>  
>  			intel_mode_from_pipe_config(&crtc_state->hw.adjusted_mode,
>  						    crtc_state);
>  
> -			mode = crtc_state->hw.adjusted_mode;
> -			mode.hdisplay = crtc_state->pipe_src_w;
> -			mode.vdisplay = crtc_state->pipe_src_h;
> -			WARN_ON(drm_atomic_set_mode_for_crtc(&crtc_state->uapi, &mode));
> +			*mode = crtc_state->hw.adjusted_mode;
> +			mode->hdisplay = crtc_state->pipe_src_w;
> +			mode->vdisplay = crtc_state->pipe_src_h;
>  
>  			/*
>  			 * The initial mode needs to be set in order to keep
> @@ -17415,11 +17465,13 @@ 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.private_flags = I915_MODE_FLAG_INHERITED;
> +			mode->private_flags = I915_MODE_FLAG_INHERITED;
>  
>  			intel_crtc_compute_pixel_rate(crtc_state);
>  
>  			intel_crtc_update_active_timings(crtc_state);
> +
> +			intel_crtc_copy_hw_to_uapi_state(crtc_state);

Hmm. I wonder if we'd need to do this even if the crtc is not active?
No difference in the driver load readout, but it will have some impact
on resume readout. OTOH we do the commit_duplicated_state() thing so I
guess the old uapi state shouldn't really matter at that point.

Fingers and toes crossed...
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  		}
>  
>  		for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> 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.24.0.rc1
> 
> _______________________________________________
> 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