Re: [PATCH 05/23] drm/i915: Complete sw/hw split

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Sep 25, 2019 at 04:18:09PM +0200, Maarten Lankhorst wrote:
> Op 25-09-2019 om 15:01 schreef Ville Syrjälä:
> > On Fri, Sep 20, 2019 at 01:42:17PM +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.
> > Don't like. IMO there should be one and only one place where copy over
> > the uapi state into the hw state. Otherwise it's a mess because you
> > constantly have to remind yourself which bits of state already got
> > copied and which didn't.
> 
> Oh now I remember, because bigjoiner is assigned later on we can't copy blobs yet
> 
> and bigjoiner was the whole reaon we wanted separate blobs so it didn't have to look at the other crtc_state..
> 
> Would it help if I copy the blobs in intel_color_check() instead?

Can't we just do something like:

drm_atomic_helper_check_modeset();
for_each_crtc_in_state() {
	if (crtc_state->enable)
		copy_uapi_to_hw();
}

compute_config() {
	if (need_bigjoiner) {
		get_other_crtc_state();
		if (other_crtc_state->uapi.enable)
			fail;
		other_hw = current_hw;
		adjust_hw_state_for_bigjoiner(other_hw);
	}
}

?

> 
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >> ---
> >>  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  | 39 +++++++++++++---
> >>  .../drm/i915/display/intel_display_types.h    |  8 ++--
> >>  4 files changed, 85 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> >> index f4440ede95c5..fb550d3cea7f 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;
> >> @@ -209,6 +217,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
> >> @@ -224,6 +267,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 32bbb5bf48f3..e40485a1e503 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,
> >> @@ -7097,6 +7098,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;
> >> @@ -11804,6 +11806,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *_crtc,
> >>  
> >>  	if (mode_changed || crtc_state->update_pipe ||
> >>  	    crtc_state->uapi.color_mgmt_changed) {
> >> +		/* Copy color blobs to hw state */
> >> +		intel_crtc_copy_color_blobs(crtc_state);
> >> +
> >>  		ret = intel_color_check(crtc_state);
> >>  		if (ret)
> >>  			return ret;
> >> @@ -12251,6 +12256,22 @@ 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;
> >> +}
> >> +
> >> +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)
> >>  {
> >> @@ -12267,6 +12288,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;
> >> @@ -12277,11 +12299,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, 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;
> >> @@ -12421,6 +12441,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;
> >>  }
> >>  
> >> @@ -13142,6 +13165,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;
> >> @@ -13568,6 +13593,7 @@ static int intel_atomic_check(struct drm_device *dev,
> >>  
> >>  		if (!new_crtc_state->uapi.enable) {
> >>  			any_ms = true;
> >> +			clear_intel_crtc_state(new_crtc_state);
> >>  			continue;
> >>  		}
> >>  
> >> @@ -16686,6 +16712,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);
> >>  
> >> @@ -16802,6 +16829,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >>  			crtc->base.mode.vdisplay = crtc_state->pipe_src_h;
> >>  			intel_mode_from_pipe_config(&crtc_state->hw.adjusted_mode,
> >>  						    crtc_state);
> >> +			crtc_state->hw.mode = crtc->base.mode;
> >>  			WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
> >>  
> >>  			/*
> >> @@ -16847,6 +16875,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 2c3567081e16..e81b785cc8f2 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> >> @@ -749,7 +749,6 @@ enum intel_output_format {
> >>  };
> >>  
> >>  struct intel_crtc_state {
> >> -	union {
> >>  	/*
> >>  	 * uapi (drm) state. This is the software state shown to userspace.
> >>  	 * In particular, the following members are used for bookkeeping:
> >> @@ -772,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.20.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux