Re: [PATCH 08/14] drm/i915: Complete crtc hw/uapi split, v3.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 25-10-2019 om 12:13 schreef Ville Syrjälä:
> On Fri, Oct 25, 2019 at 11:00:06AM +0200, Maarten Lankhorst wrote:
>> Op 24-10-2019 om 17:21 schreef Ville Syrjälä:
>>> On Thu, Oct 24, 2019 at 02:47: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().
>>>> 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().
>>>>
>>>> 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  | 56 +++++++++++++++----
>>>>  .../drm/i915/display/intel_display_types.h    |  9 +--
>>>>  4 files changed, 95 insertions(+), 16 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)
>>> This is only used in intel_display.c so should perhaps live there?
>>>
>>>> +{
>>>> +	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 11dd7a182543..2dbc1df9505a 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>>> @@ -7110,6 +7110,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;
>>>> @@ -12482,22 +12484,50 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +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_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;
>>> Why are we not copying the color blobs?
>> Hmm, we probably should at least copy the gamma blob, but probably all. :)
>>
>> Missed that because this patch lingered on the ml for so long we added hw readout in the mean time.
>>
>>>> +}
>>>> +
>>>>  static int
>>>> -clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>>>> +intel_crtc_prepare_state(struct intel_crtc_state *crtc_state)
>>> Hmm. I was hoping we could make this even simpler, but maybe I'm a bit
>>> naive. I guess we really do need to consider needs_modeset() so as to
>>> not clobber the already computed hw state when we're not going to take
>>> the full .compute_config() path.
>>>
>>>>  {
>>>>  	struct drm_i915_private *dev_priv =
>>>>  		to_i915(crtc_state->uapi.crtc->dev);
>>>>  	struct intel_crtc_state *saved_state;
>>>>  
>>>> +	if (!needs_modeset(crtc_state)) {
>>>> +		/* Only need to update crtc_state->hw color blobs */
>>> That comment is likely to get out of date. Not sure how to word it in a
>>> nice way though. Essentially we just want to copy the part of the state
>>> that doesn't clobber anything set up by .compute_config() & co.
>>>
>>> It would be nice if we had the same level of abstraction for this case
>>> as for the full modeset case. Eg. intel_crtc_copy_hw_state_non_modeset()
>>> or something. A bit ugly that name though. Might have think of something
>>> better.
>>>
>>> Another thing that's a bit confusing is that we need to do the
>>> intel_crtc_free_hw_state() for the full modeset path, but we don't
>>> need it for the non-modeset path. Would be nice it the two paths
>>> behaved more uniformly. 
>> We'd have to get rid of the crtc_state clearing for that. :(
> I was just thinking we'd have something like:
> crtc_state_free_lite() + crtc_state_copy_lite()
> and
> crtc_state_free_full() + crtc_state_copy_full()
>
> With better names probably :)
>
> Then both paths would do the same kind of free+copy and the reader
> wouldn't have to wonder why they look so different.
>
>>>> +		intel_crtc_copy_color_blobs(crtc_state);
>>>> +		return 0;
>>>> +	}
>>>> +
>>>>  	saved_state = kzalloc(sizeof(*saved_state), GFP_KERNEL);
>>>>  	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;
>>>> @@ -12515,14 +12545,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;
>>>>  }
>>>>  
>>>> @@ -12538,10 +12565,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;
>>>>  
>>>> @@ -13361,6 +13384,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;
>>>> @@ -13823,6 +13848,10 @@ 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) {
>>>> +		ret = intel_crtc_prepare_state(new_crtc_state);
>>>> +		if (ret)
>>>> +			return ret;
>>> I have a feeling this should be its own loop in case we need
>>> to insert global stuff after this.
>>>
>>> For example, I think the port sync stuff should be there but
>>> I can't see it now. Ah, for some reason it's buried deep inside
>>> intel_modeset_pipe_config() which seems wrong. If any (or at
>>> least the master) of the genlocked pipes does a full modeset
>>> we need to do a full modeset on all of them, which should also
>>> mean a full .compute_config() on everything.
>>>
>>> Hmm. And if we do that then combining the uapi->hw copy and
>>> clear_intel_crtc_state() like this is not going to work.
>> Genlock would be messy either way, with fastset vs modeset. It would need its own second loop to handle correctly.
> Hmm. Yeah, this fastset vs. port sync is particularly troublesome
> because we now copy some state back from the old state.
>
> Not really sure what to do about that other than:
> a) don't do fastset with port sync (would be a shame)
> b) redo .compute_config() for all tiles if at least one of
>    them still insists on a full modeset after all have gone
>    through the fastset check (a bit yucky)
> c) copy even more state to make sure it's all totally consistent
>    (should really do this anyway), and then just force a modeset
>    on all tiles if one needs it but without redoing any
>    .compute_config() stuff (ie. do the modeset with the fastset
>    adjusted state)
>
> I maybe like c) best. b) would perhaps allow us to not do any port sync
> things before the .compute_config() loop since we'd redo it anyway if
> anything affected neds a modeset. But having a retry loop would be a
> little annoying. Although we're going to need some kind of retry loop
> for eg. MST bandwidth vs. bpc stuff anyway.

Port sync should probably have its own handling in intel_modeset_checks().

I think it's best to just do a split, perhaps clean up v2 as we no longer pass BAT, and worry about fixing this mess later, else things never move forward.

~Maarten

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