[08/14] drm/i915: Complete crtc hw/uapi split, v2.
diff mbox series

Message ID 20191017132105.15528-8-maarten.lankhorst@linux.intel.com
State New
Headers show
Series
  • [01/14] drm/i915: Rework watermark readout to use plane api
Related show

Commit Message

Maarten Lankhorst Oct. 17, 2019, 1:20 p.m. UTC
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().

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_atomic.c   | 44 ++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_atomic.h   |  2 +
 drivers/gpu/drm/i915/display/intel_display.c  | 45 ++++++++++++++++---
 .../drm/i915/display/intel_display_types.h    |  9 ++--
 4 files changed, 89 insertions(+), 11 deletions(-)

Comments

Ville Syrjälä Oct. 22, 2019, 6:32 p.m. UTC | #1
On Thu, Oct 17, 2019 at 03:20:59PM +0200, 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().
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic.c   | 44 ++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_atomic.h   |  2 +
>  drivers/gpu/drm/i915/display/intel_display.c  | 45 ++++++++++++++++---
>  .../drm/i915/display/intel_display_types.h    |  9 ++--
>  4 files changed, 89 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index 7cf13b9c7d38..266d0ce9d03d 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,41 @@ 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)
> +{
> +	intel_crtc_put_color_blobs(crtc_state);
> +
> +	if (crtc_state->uapi.degamma_lut)
> +		crtc_state->hw.degamma_lut =
> +			drm_property_blob_get(crtc_state->uapi.degamma_lut);
> +	else
> +		crtc_state->hw.degamma_lut = NULL;

Side note: If drm_property_blob_get() would pass NULL through this kind
of stuff would look a lot less annoying.

> +
> +	if (crtc_state->uapi.gamma_lut)
> +		crtc_state->hw.gamma_lut =
> +			drm_property_blob_get(crtc_state->uapi.gamma_lut);
> +	else
> +		crtc_state->hw.gamma_lut = NULL;
> +
> +	if (crtc_state->uapi.ctm)
> +		crtc_state->hw.ctm =
> +			drm_property_blob_get(crtc_state->uapi.ctm);
> +	else
> +		crtc_state->hw.ctm = NULL;
> +}
> +
>  /**
>   * intel_crtc_destroy_state - destroy crtc state
>   * @crtc: drm crtc
> @@ -223,6 +266,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 58065d3161a3..42be91e0772a 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> @@ -35,6 +35,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 06c593d56d92..c009489641bd 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -114,6 +114,7 @@ static const u64 cursor_format_modifiers[] = {
>  	DRM_FORMAT_MOD_INVALID
>  };
>  
> +static void copy_uapi_to_hw_state(struct intel_crtc_state *crtc_state);
>  static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
>  				struct intel_crtc_state *pipe_config);
>  static void ironlake_pch_clock_get(struct intel_crtc *crtc,
> @@ -7041,6 +7042,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
>  	crtc->enabled = false;
>  	crtc->state->connector_mask = 0;
>  	crtc->state->encoder_mask = 0;
> +	copy_uapi_to_hw_state(to_intel_crtc_state(crtc->state));
>  
>  	for_each_encoder_on_crtc(crtc->dev, crtc, encoder)
>  		encoder->base.crtc = NULL;
> @@ -12254,6 +12256,23 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
>  	return ret;
>  }
>  
> +static void 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_color_blobs(crtc_state);
> +}
> +
> +static void 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;
> +}
> +
>  static int
>  clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  {
> @@ -12270,6 +12289,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	 * 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;
> @@ -12280,12 +12300,9 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		saved_state->wm = crtc_state->wm;
>  
> -	/* 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));
> +	intel_crtc_free_hw_state(crtc_state);
> +	memcpy(crtc_state, saved_state, sizeof(*crtc_state));
> +	copy_uapi_to_hw_state(crtc_state);

I still think this stuff is in the wrong place. IMO all the uapi->hw copy
should happen in exactly one place (and that place I believe should be
just after drm_atomic_helper_check_modeset()). The patch changelog
seemed to imply that was what were going for but the code doesn't appear
to agree.

>  
>  	kfree(saved_state);
>  	return 0;
> @@ -12425,6 +12442,9 @@ 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);
>  
> +	/* uapi wants a copy of the adjusted_mode for vblank bookkeeping */
> +	pipe_config->uapi.adjusted_mode = pipe_config->hw.adjusted_mode;

This should no longer be needed?

> +
>  	return 0;
>  }
>  
> @@ -13110,6 +13130,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;
> @@ -13572,11 +13594,17 @@ 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)) {
> +			if (new_crtc_state->uapi.color_mgmt_changed)
> +				intel_crtc_copy_color_blobs(new_crtc_state);
>  			continue;
> +		}
>  
>  		if (!new_crtc_state->uapi.enable) {
>  			any_ms = true;
> +			intel_crtc_free_hw_state(new_crtc_state);
> +			memset(&new_crtc_state->hw, 0,
> +			       sizeof(new_crtc_state->hw));
>  			continue;
>  		}
>  
> @@ -16662,6 +16690,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);
>  
> @@ -16790,6 +16819,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);
> @@ -16820,6 +16850,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  
>  		intel_bw_crtc_update(bw_state, crtc_state);
>  
> +		copy_hw_to_uapi_state(crtc_state);
>  		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 4d85ea5832d7..41d471ab9a64 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -749,8 +749,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:
> @@ -773,8 +771,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
Maarten Lankhorst Oct. 23, 2019, 3:06 p.m. UTC | #2
Op 22-10-2019 om 20:32 schreef Ville Syrjälä:
> On Thu, Oct 17, 2019 at 03:20:59PM +0200, 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().
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_atomic.c   | 44 ++++++++++++++++++
>>  drivers/gpu/drm/i915/display/intel_atomic.h   |  2 +
>>  drivers/gpu/drm/i915/display/intel_display.c  | 45 ++++++++++++++++---
>>  .../drm/i915/display/intel_display_types.h    |  9 ++--
>>  4 files changed, 89 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
>> index 7cf13b9c7d38..266d0ce9d03d 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,41 @@ 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)
>> +{
>> +	intel_crtc_put_color_blobs(crtc_state);
>> +
>> +	if (crtc_state->uapi.degamma_lut)
>> +		crtc_state->hw.degamma_lut =
>> +			drm_property_blob_get(crtc_state->uapi.degamma_lut);
>> +	else
>> +		crtc_state->hw.degamma_lut = NULL;
> Side note: If drm_property_blob_get() would pass NULL through this kind
> of stuff would look a lot less annoying.
>
>> +
>> +	if (crtc_state->uapi.gamma_lut)
>> +		crtc_state->hw.gamma_lut =
>> +			drm_property_blob_get(crtc_state->uapi.gamma_lut);
>> +	else
>> +		crtc_state->hw.gamma_lut = NULL;
>> +
>> +	if (crtc_state->uapi.ctm)
>> +		crtc_state->hw.ctm =
>> +			drm_property_blob_get(crtc_state->uapi.ctm);
>> +	else
>> +		crtc_state->hw.ctm = NULL;
>> +}
>> +
>>  /**
>>   * intel_crtc_destroy_state - destroy crtc state
>>   * @crtc: drm crtc
>> @@ -223,6 +266,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 58065d3161a3..42be91e0772a 100644
>> --- a/drivers/gpu/drm/i915/display/intel_atomic.h
>> +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
>> @@ -35,6 +35,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 06c593d56d92..c009489641bd 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -114,6 +114,7 @@ static const u64 cursor_format_modifiers[] = {
>>  	DRM_FORMAT_MOD_INVALID
>>  };
>>  
>> +static void copy_uapi_to_hw_state(struct intel_crtc_state *crtc_state);
>>  static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
>>  				struct intel_crtc_state *pipe_config);
>>  static void ironlake_pch_clock_get(struct intel_crtc *crtc,
>> @@ -7041,6 +7042,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
>>  	crtc->enabled = false;
>>  	crtc->state->connector_mask = 0;
>>  	crtc->state->encoder_mask = 0;
>> +	copy_uapi_to_hw_state(to_intel_crtc_state(crtc->state));
>>  
>>  	for_each_encoder_on_crtc(crtc->dev, crtc, encoder)
>>  		encoder->base.crtc = NULL;
>> @@ -12254,6 +12256,23 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
>>  	return ret;
>>  }
>>  
>> +static void 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_color_blobs(crtc_state);
>> +}
>> +
>> +static void 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;
>> +}
>> +
>>  static int
>>  clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>>  {
>> @@ -12270,6 +12289,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>>  	 * 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;
>> @@ -12280,12 +12300,9 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>>  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>>  		saved_state->wm = crtc_state->wm;
>>  
>> -	/* 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));
>> +	intel_crtc_free_hw_state(crtc_state);
>> +	memcpy(crtc_state, saved_state, sizeof(*crtc_state));
>> +	copy_uapi_to_hw_state(crtc_state);
> I still think this stuff is in the wrong place. IMO all the uapi->hw copy
> should happen in exactly one place (and that place I believe should be
> just after drm_atomic_helper_check_modeset()). The patch changelog
> seemed to imply that was what were going for but the code doesn't appear
> to agree.
Yeah I will just make a single function on top of there.
>
>>  
>>  	kfree(saved_state);
>>  	return 0;
>> @@ -12425,6 +12442,9 @@ 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);
>>  
>> +	/* uapi wants a copy of the adjusted_mode for vblank bookkeeping */
>> +	pipe_config->uapi.adjusted_mode = pipe_config->hw.adjusted_mode;
> This should no longer be needed?
Yeah, think so. One way to find out. :)
>
>> +
>>  	return 0;
>>  }
>>  
>> @@ -13110,6 +13130,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;
>> @@ -13572,11 +13594,17 @@ 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)) {
>> +			if (new_crtc_state->uapi.color_mgmt_changed)
>> +				intel_crtc_copy_color_blobs(new_crtc_state);
>>  			continue;
>> +		}
>>  
>>  		if (!new_crtc_state->uapi.enable) {
>>  			any_ms = true;
>> +			intel_crtc_free_hw_state(new_crtc_state);
>> +			memset(&new_crtc_state->hw, 0,
>> +			       sizeof(new_crtc_state->hw));
>>  			continue;
>>  		}
>>  
>> @@ -16662,6 +16690,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);
>>  
>> @@ -16790,6 +16819,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);
>> @@ -16820,6 +16850,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>>  
>>  		intel_bw_crtc_update(bw_state, crtc_state);
>>  
>> +		copy_hw_to_uapi_state(crtc_state);
>>  		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 4d85ea5832d7..41d471ab9a64 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -749,8 +749,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:
>> @@ -773,8 +771,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

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index 7cf13b9c7d38..266d0ce9d03d 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,41 @@  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)
+{
+	intel_crtc_put_color_blobs(crtc_state);
+
+	if (crtc_state->uapi.degamma_lut)
+		crtc_state->hw.degamma_lut =
+			drm_property_blob_get(crtc_state->uapi.degamma_lut);
+	else
+		crtc_state->hw.degamma_lut = NULL;
+
+	if (crtc_state->uapi.gamma_lut)
+		crtc_state->hw.gamma_lut =
+			drm_property_blob_get(crtc_state->uapi.gamma_lut);
+	else
+		crtc_state->hw.gamma_lut = NULL;
+
+	if (crtc_state->uapi.ctm)
+		crtc_state->hw.ctm =
+			drm_property_blob_get(crtc_state->uapi.ctm);
+	else
+		crtc_state->hw.ctm = NULL;
+}
+
 /**
  * intel_crtc_destroy_state - destroy crtc state
  * @crtc: drm crtc
@@ -223,6 +266,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 58065d3161a3..42be91e0772a 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.h
+++ b/drivers/gpu/drm/i915/display/intel_atomic.h
@@ -35,6 +35,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 06c593d56d92..c009489641bd 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -114,6 +114,7 @@  static const u64 cursor_format_modifiers[] = {
 	DRM_FORMAT_MOD_INVALID
 };
 
+static void copy_uapi_to_hw_state(struct intel_crtc_state *crtc_state);
 static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
 				struct intel_crtc_state *pipe_config);
 static void ironlake_pch_clock_get(struct intel_crtc *crtc,
@@ -7041,6 +7042,7 @@  static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
 	crtc->enabled = false;
 	crtc->state->connector_mask = 0;
 	crtc->state->encoder_mask = 0;
+	copy_uapi_to_hw_state(to_intel_crtc_state(crtc->state));
 
 	for_each_encoder_on_crtc(crtc->dev, crtc, encoder)
 		encoder->base.crtc = NULL;
@@ -12254,6 +12256,23 @@  static bool check_digital_port_conflicts(struct intel_atomic_state *state)
 	return ret;
 }
 
+static void 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_color_blobs(crtc_state);
+}
+
+static void 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;
+}
+
 static int
 clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 {
@@ -12270,6 +12289,7 @@  clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	 * 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;
@@ -12280,12 +12300,9 @@  clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
 		saved_state->wm = crtc_state->wm;
 
-	/* 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));
+	intel_crtc_free_hw_state(crtc_state);
+	memcpy(crtc_state, saved_state, sizeof(*crtc_state));
+	copy_uapi_to_hw_state(crtc_state);
 
 	kfree(saved_state);
 	return 0;
@@ -12425,6 +12442,9 @@  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);
 
+	/* uapi wants a copy of the adjusted_mode for vblank bookkeeping */
+	pipe_config->uapi.adjusted_mode = pipe_config->hw.adjusted_mode;
+
 	return 0;
 }
 
@@ -13110,6 +13130,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;
@@ -13572,11 +13594,17 @@  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)) {
+			if (new_crtc_state->uapi.color_mgmt_changed)
+				intel_crtc_copy_color_blobs(new_crtc_state);
 			continue;
+		}
 
 		if (!new_crtc_state->uapi.enable) {
 			any_ms = true;
+			intel_crtc_free_hw_state(new_crtc_state);
+			memset(&new_crtc_state->hw, 0,
+			       sizeof(new_crtc_state->hw));
 			continue;
 		}
 
@@ -16662,6 +16690,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);
 
@@ -16790,6 +16819,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);
@@ -16820,6 +16850,7 @@  static void intel_modeset_readout_hw_state(struct drm_device *dev)
 
 		intel_bw_crtc_update(bw_state, crtc_state);
 
+		copy_hw_to_uapi_state(crtc_state);
 		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 4d85ea5832d7..41d471ab9a64 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -749,8 +749,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:
@@ -773,8 +771,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