On Fri, Feb 04, 2022 at 09:20:49AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Currently the bigjoiner state copy logic is kind of
> a byzantine mess.
>
> Clean it up to operate in the following manner during a full
> modeset:
> 1) master uapi -> hw state copy
> 2) master hw -> slave hw state copy
>
> And during a non-modeset update we do:
> 1) master uapi -> hw state light copy
> 2) master hw -> slave hw state light copy
>
> I think that is now easier to reason about since we never do
> any kind of master uapi -> slave hw state copy short circuit
> that could happen previously.
>
> Obviously this does now depend on the master uapi->hw copy
> always happening before the master hw -> slave hw copy, but
> that is guaranteed by the fact that we always add both crtcs
> to the state early, the crtcs are registered in pipe
> order (so the compute_config loop happens in pipe order),
> and the hardware requires the master pipe has to be lower
> than the slave pipe as well. And for good measure we shall
> add a check+WARN for this before doing the bigjoiner crtc
> assignment.
>
> v2: Fix uapi.ctm vs. hw.ctm copy-paste fail
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Looks good to me, jus one question on how we decide what state from orginal slave crtc state to preserve.
But in either case
Reviewed-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_atomic.c | 11 --
> drivers/gpu/drm/i915/display/intel_atomic.h | 2 -
> drivers/gpu/drm/i915/display/intel_display.c | 170 ++++++++++++-------
> 3 files changed, 108 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index 093904065112..e0667d163266 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -281,17 +281,6 @@ 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,
> - const struct intel_crtc_state *from_crtc_state)
> -{
> - drm_property_replace_blob(&crtc_state->hw.degamma_lut,
> - from_crtc_state->uapi.degamma_lut);
> - drm_property_replace_blob(&crtc_state->hw.gamma_lut,
> - from_crtc_state->uapi.gamma_lut);
> - drm_property_replace_blob(&crtc_state->hw.ctm,
> - from_crtc_state->uapi.ctm);
> -}
> -
> /**
> * intel_crtc_destroy_state - destroy crtc state
> * @crtc: drm crtc
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
> index d2700c74c9da..1dc439983dd9 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> @@ -44,8 +44,6 @@ 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,
> - const struct intel_crtc_state *from_crtc_state);
> struct drm_atomic_state *intel_atomic_state_alloc(struct drm_device *dev);
> void intel_atomic_state_free(struct drm_atomic_state *state);
> 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 349cc3807e8b..b391cb98b12f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5818,32 +5818,37 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
>
> static void
> intel_crtc_copy_uapi_to_hw_state_nomodeset(struct intel_atomic_state *state,
> - struct intel_crtc_state *crtc_state)
> + struct intel_crtc *crtc)
> {
> - const struct intel_crtc_state *master_crtc_state;
> - struct intel_crtc *master_crtc;
> + struct intel_crtc_state *crtc_state =
> + intel_atomic_get_new_crtc_state(state, crtc);
>
> - master_crtc = intel_master_crtc(crtc_state);
> - master_crtc_state = intel_atomic_get_new_crtc_state(state, master_crtc);
> + WARN_ON(crtc_state->bigjoiner_slave);
>
> - /* No need to copy state if the master state is unchanged */
> - if (master_crtc_state) {
> - crtc_state->uapi.color_mgmt_changed = master_crtc_state->uapi.color_mgmt_changed;
> - intel_crtc_copy_color_blobs(crtc_state, master_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);
> }
>
> static void
> -intel_crtc_copy_uapi_to_hw_state(struct intel_atomic_state *state,
> - struct intel_crtc_state *crtc_state)
> +intel_crtc_copy_uapi_to_hw_state_modeset(struct intel_atomic_state *state,
> + struct intel_crtc *crtc)
> {
> + struct intel_crtc_state *crtc_state =
> + intel_atomic_get_new_crtc_state(state, crtc);
> +
> + WARN_ON(crtc_state->bigjoiner_slave);
> +
> 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;
> crtc_state->hw.scaling_filter = crtc_state->uapi.scaling_filter;
>
> - intel_crtc_copy_uapi_to_hw_state_nomodeset(state, crtc_state);
> + intel_crtc_copy_uapi_to_hw_state_nomodeset(state, crtc);
> }
>
> static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state)
> @@ -5859,7 +5864,6 @@ static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state
> crtc_state->uapi.adjusted_mode = crtc_state->hw.adjusted_mode;
> crtc_state->uapi.scaling_filter = crtc_state->hw.scaling_filter;
>
> - /* 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,
> @@ -5868,61 +5872,79 @@ static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state
> crtc_state->hw.ctm);
> }
>
> +static void
> +copy_bigjoiner_crtc_state_nomodeset(struct intel_atomic_state *state,
> + struct intel_crtc *slave_crtc)
> +{
> + struct intel_crtc_state *slave_crtc_state =
> + intel_atomic_get_new_crtc_state(state, slave_crtc);
> + struct intel_crtc *master_crtc = intel_master_crtc(slave_crtc_state);
> + const struct intel_crtc_state *master_crtc_state =
> + intel_atomic_get_new_crtc_state(state, master_crtc);
> +
> + drm_property_replace_blob(&slave_crtc_state->hw.degamma_lut,
> + master_crtc_state->hw.degamma_lut);
> + drm_property_replace_blob(&slave_crtc_state->hw.gamma_lut,
> + master_crtc_state->hw.gamma_lut);
> + drm_property_replace_blob(&slave_crtc_state->hw.ctm,
> + master_crtc_state->hw.ctm);
> +
> + slave_crtc_state->uapi.color_mgmt_changed = master_crtc_state->uapi.color_mgmt_changed;
> +}
> +
> static int
> -copy_bigjoiner_crtc_state(struct intel_crtc_state *crtc_state,
> - const struct intel_crtc_state *from_crtc_state)
> +copy_bigjoiner_crtc_state_modeset(struct intel_atomic_state *state,
> + struct intel_crtc *slave_crtc)
> {
> + struct intel_crtc_state *slave_crtc_state =
> + intel_atomic_get_new_crtc_state(state, slave_crtc);
> + struct intel_crtc *master_crtc = intel_master_crtc(slave_crtc_state);
> + const struct intel_crtc_state *master_crtc_state =
> + intel_atomic_get_new_crtc_state(state, master_crtc);
> struct intel_crtc_state *saved_state;
>
> - saved_state = kmemdup(from_crtc_state, sizeof(*saved_state), GFP_KERNEL);
> + saved_state = kmemdup(master_crtc_state, sizeof(*saved_state), GFP_KERNEL);
> if (!saved_state)
> return -ENOMEM;
>
> - 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;
> - saved_state->crc_enabled = crtc_state->crc_enabled;
> + /* preserve some things from the slave's original crtc state */
> + saved_state->uapi = slave_crtc_state->uapi;
> + saved_state->scaler_state = slave_crtc_state->scaler_state;
> + saved_state->shared_dpll = slave_crtc_state->shared_dpll;
> + saved_state->dpll_hw_state = slave_crtc_state->dpll_hw_state;
> + saved_state->crc_enabled = slave_crtc_state->crc_enabled;
Slave crtc state here not set at all , so why do we preserve the things from slave's original crtc state and how we
decide on what all to preserve?
Manasi
>
> - intel_crtc_free_hw_state(crtc_state);
> - memcpy(crtc_state, saved_state, sizeof(*crtc_state));
> + intel_crtc_free_hw_state(slave_crtc_state);
> + memcpy(slave_crtc_state, saved_state, sizeof(*slave_crtc_state));
> kfree(saved_state);
>
> /* Re-init hw state */
> - memset(&crtc_state->hw, 0, sizeof(saved_state->hw));
> - crtc_state->hw.enable = from_crtc_state->hw.enable;
> - crtc_state->hw.active = from_crtc_state->hw.active;
> - crtc_state->hw.mode = from_crtc_state->hw.mode;
> - crtc_state->hw.pipe_mode = from_crtc_state->hw.pipe_mode;
> - crtc_state->hw.adjusted_mode = from_crtc_state->hw.adjusted_mode;
> - crtc_state->hw.scaling_filter = from_crtc_state->hw.scaling_filter;
> + memset(&slave_crtc_state->hw, 0, sizeof(slave_crtc_state->hw));
> + slave_crtc_state->hw.enable = master_crtc_state->hw.enable;
> + slave_crtc_state->hw.active = master_crtc_state->hw.active;
> + slave_crtc_state->hw.mode = master_crtc_state->hw.mode;
> + slave_crtc_state->hw.pipe_mode = master_crtc_state->hw.pipe_mode;
> + slave_crtc_state->hw.adjusted_mode = master_crtc_state->hw.adjusted_mode;
> + slave_crtc_state->hw.scaling_filter = master_crtc_state->hw.scaling_filter;
>
> - drm_property_replace_blob(&crtc_state->hw.degamma_lut,
> - from_crtc_state->hw.degamma_lut);
> - drm_property_replace_blob(&crtc_state->hw.gamma_lut,
> - from_crtc_state->hw.gamma_lut);
> - drm_property_replace_blob(&crtc_state->hw.ctm,
> - from_crtc_state->hw.ctm);
> + copy_bigjoiner_crtc_state_nomodeset(state, slave_crtc);
>
> /* Some fixups */
> - crtc_state->uapi.mode_changed = from_crtc_state->uapi.mode_changed;
> - crtc_state->uapi.connectors_changed = from_crtc_state->uapi.connectors_changed;
> - crtc_state->uapi.active_changed = from_crtc_state->uapi.active_changed;
> - crtc_state->uapi.color_mgmt_changed = from_crtc_state->uapi.color_mgmt_changed;
> - crtc_state->nv12_planes = crtc_state->c8_planes = crtc_state->update_planes = 0;
> - crtc_state->bigjoiner_linked_crtc = to_intel_crtc(from_crtc_state->uapi.crtc);
> - crtc_state->bigjoiner_slave = true;
> - crtc_state->cpu_transcoder = from_crtc_state->cpu_transcoder;
> - crtc_state->has_audio = from_crtc_state->has_audio;
> + slave_crtc_state->uapi.mode_changed = master_crtc_state->uapi.mode_changed;
> + slave_crtc_state->uapi.connectors_changed = master_crtc_state->uapi.connectors_changed;
> + slave_crtc_state->uapi.active_changed = master_crtc_state->uapi.active_changed;
> + slave_crtc_state->cpu_transcoder = master_crtc_state->cpu_transcoder;
> + slave_crtc_state->has_audio = master_crtc_state->has_audio;
>
> return 0;
> }
>
> static int
> intel_crtc_prepare_cleared_state(struct intel_atomic_state *state,
> - struct intel_crtc_state *crtc_state)
> + struct intel_crtc *crtc)
> {
> - struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> + struct intel_crtc_state *crtc_state =
> + intel_atomic_get_new_crtc_state(state, crtc);
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> struct intel_crtc_state *saved_state;
>
> @@ -5952,7 +5974,7 @@ intel_crtc_prepare_cleared_state(struct intel_atomic_state *state,
> memcpy(crtc_state, saved_state, sizeof(*crtc_state));
> kfree(saved_state);
>
> - intel_crtc_copy_uapi_to_hw_state(state, crtc_state);
> + intel_crtc_copy_uapi_to_hw_state_modeset(state, crtc);
>
> return 0;
> }
> @@ -7592,6 +7614,9 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state,
> struct intel_crtc_state *slave_crtc_state;
> struct intel_crtc *slave_crtc;
>
> + WARN_ON(master_crtc_state->bigjoiner_linked_crtc);
> + WARN_ON(master_crtc_state->bigjoiner_slave);
> +
> if (!master_crtc_state->bigjoiner)
> return 0;
>
> @@ -7604,7 +7629,6 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state,
> return -EINVAL;
> }
>
> - master_crtc_state->bigjoiner_linked_crtc = slave_crtc;
> slave_crtc_state = intel_atomic_get_crtc_state(&state->base, slave_crtc);
> if (IS_ERR(slave_crtc_state))
> return PTR_ERR(slave_crtc_state);
> @@ -7613,11 +7637,28 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state,
> if (slave_crtc_state->uapi.enable)
> goto claimed;
>
> + /*
> + * The state copy logic assumes the master crtc gets processed
> + * before the slave crtc during the main compute_config loop.
> + * This works because the crtcs are created in pipe order,
> + * and the hardware requires master pipe < slave pipe as well.
> + * Should that change we need to rethink the logic.
> + */
> + if (WARN_ON(drm_crtc_index(&master_crtc->base) > drm_crtc_index(&slave_crtc->base)))
> + return -EINVAL;
> +
> drm_dbg_kms(&i915->drm,
> - "[CRTC:%d:%s] Used as slave for big joiner\n",
> - slave_crtc->base.base.id, slave_crtc->base.name);
> + "[CRTC:%d:%s] Used as slave for big joiner master [CRTC:%d:%s]\n",
> + slave_crtc->base.base.id, slave_crtc->base.name,
> + master_crtc->base.base.id, master_crtc->base.name);
>
> - return copy_bigjoiner_crtc_state(slave_crtc_state, master_crtc_state);
> + master_crtc_state->bigjoiner_linked_crtc = slave_crtc;
> + master_crtc_state->bigjoiner_slave = false;
> +
> + slave_crtc_state->bigjoiner_linked_crtc = master_crtc;
> + slave_crtc_state->bigjoiner_slave = true;
> +
> + return copy_bigjoiner_crtc_state_modeset(state, slave_crtc);
>
> claimed:
> drm_dbg_kms(&i915->drm,
> @@ -7629,15 +7670,19 @@ static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state,
> }
>
> static void kill_bigjoiner_slave(struct intel_atomic_state *state,
> - struct intel_crtc_state *master_crtc_state)
> + struct intel_crtc *master_crtc)
> {
> + struct intel_crtc_state *master_crtc_state =
> + intel_atomic_get_new_crtc_state(state, master_crtc);
> + struct intel_crtc *slave_crtc = master_crtc_state->bigjoiner_linked_crtc;
> struct intel_crtc_state *slave_crtc_state =
> - intel_atomic_get_new_crtc_state(state, master_crtc_state->bigjoiner_linked_crtc);
> + intel_atomic_get_new_crtc_state(state, slave_crtc);
>
> slave_crtc_state->bigjoiner = master_crtc_state->bigjoiner = false;
> slave_crtc_state->bigjoiner_slave = master_crtc_state->bigjoiner_slave = false;
> slave_crtc_state->bigjoiner_linked_crtc = master_crtc_state->bigjoiner_linked_crtc = NULL;
> - intel_crtc_copy_uapi_to_hw_state(state, slave_crtc_state);
> +
> + intel_crtc_copy_uapi_to_hw_state_modeset(state, slave_crtc);
> }
>
> /**
> @@ -7823,7 +7868,7 @@ static int intel_bigjoiner_add_affected_crtcs(struct intel_atomic_state *state)
> /* Kill old bigjoiner link, we may re-establish afterwards */
> if (intel_crtc_needs_modeset(crtc_state) &&
> crtc_state->bigjoiner && !crtc_state->bigjoiner_slave)
> - kill_bigjoiner_slave(state, crtc_state);
> + kill_bigjoiner_slave(state, crtc);
> }
>
> return 0;
> @@ -7867,21 +7912,22 @@ 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 (!intel_crtc_needs_modeset(new_crtc_state)) {
> - /* Light copy */
> - intel_crtc_copy_uapi_to_hw_state_nomodeset(state, new_crtc_state);
> -
> + if (new_crtc_state->bigjoiner_slave)
> + copy_bigjoiner_crtc_state_nomodeset(state, crtc);
> + else
> + intel_crtc_copy_uapi_to_hw_state_nomodeset(state, crtc);
> continue;
> }
>
> if (!new_crtc_state->uapi.enable) {
> if (!new_crtc_state->bigjoiner_slave) {
> - intel_crtc_copy_uapi_to_hw_state(state, new_crtc_state);
> + intel_crtc_copy_uapi_to_hw_state_modeset(state, crtc);
> any_ms = true;
> }
> continue;
> }
>
> - ret = intel_crtc_prepare_cleared_state(state, new_crtc_state);
> + ret = intel_crtc_prepare_cleared_state(state, crtc);
> if (ret)
> goto fail;
>
> --
> 2.34.1
>
