On Fri, Mar 01, 2024 at 05:26:19PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 01, 2024 at 05:17:41PM +0200, Lisovskiy, Stanislav wrote:
> > On Fri, Mar 01, 2024 at 04:40:28PM +0200, Ville Syrjälä wrote:
> > > On Fri, Mar 01, 2024 at 02:29:28PM +0200, Lisovskiy, Stanislav wrote:
> > > > On Fri, Mar 01, 2024 at 12:43:46PM +0200, Ville Syrjälä wrote:
> > > > > On Fri, Mar 01, 2024 at 12:27:18PM +0200, Lisovskiy, Stanislav wrote:
> > > > > > On Fri, Mar 01, 2024 at 12:10:52PM +0200, Ville Syrjälä wrote:
> > > > > > > On Wed, Feb 21, 2024 at 09:20:09PM +0200, Stanislav Lisovskiy
> > > > > > > wrote:
> > > > > > > > Handle only bigjoiner masters in
> > > > > > > > skl_commit_modeset_enables/disables,
> > > > > > > > slave crtcs should be handled by master hooks. Same for
> > > > > > > > encoders.
> > > > > > > > That way we can also remove a bunch of checks like
> > > > > > > > intel_crtc_is_bigjoiner_slave.
> > > > > > > >
> > > > > > > > v2: Get rid of master vs slave checks and separation in crtc
> > > > > > > > enable/disable hooks.
> > > > > > > > Use unified iteration cycle for all of those, while
> > > > > > > > enabling/disabling
> > > > > > > > transcoder only for those pipes where its needed(Ville
> > > > > > > > Syrjälä)
> > > > > > > >
> > > > > > > > v3: Move all the intel_encoder_* calls under transcoder code
> > > > > > > > path(Ville Syrjälä)
> > > > > > > >
> > > > > > > > v4: - Call intel_crtc_vblank_on from hsw_crtc_enable only for
> > > > > > > > non-transcoder path
> > > > > > > > (for master pipe that will be called from
> > > > > > > > intel_encoders_enable/intel_enable_ddi)
> > > > > > > > - Fix stupid mistake with using crtc->pipe for the mask,
> > > > > > > > instead of BIT(crtc->pipe)
> > > > > > > >
> > > > > > > > Signed-off-by: Stanislav Lisovskiy
> > > > > > > > <stanislav.lisovs...@intel.com>
> > > > > > > > ---
> > > > > > > > drivers/gpu/drm/i915/display/intel_ddi.c | 21 +--
> > > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 183
> > > > > > > > ++++++++++++-------
> > > > > > > > drivers/gpu/drm/i915/display/intel_display.h | 6 +
> > > > > > > > 3 files changed, 121 insertions(+), 89 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > > > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > > > > index bea4415902044..6071e9f500871 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > > > > @@ -3100,7 +3100,6 @@ static void intel_ddi_post_disable(struct
> > > > > > > > intel_atomic_state *state,
> > > > > > > > const struct
> > > > > > > > drm_connector_state *old_conn_state)
> > > > > > > > {
> > > > > > > > struct drm_i915_private *dev_priv =
> > > > > > > > to_i915(encoder->base.dev);
> > > > > > > > - struct intel_crtc *slave_crtc;
> > > > > > > >
> > > > > > > > if (!intel_crtc_has_type(old_crtc_state,
> > > > > > > > INTEL_OUTPUT_DP_MST)) {
> > > > > > > > intel_crtc_vblank_off(old_crtc_state);
> > > > > > > > @@ -3117,17 +3116,6 @@ static void
> > > > > > > > intel_ddi_post_disable(struct intel_atomic_state *state,
> > > > > > > > ilk_pfit_disable(old_crtc_state);
> > > > > > > > }
> > > > > > >
> > > > > > > The master pipe stuff is right here ^ ...
> > > > > > >
> > > > > > > >
> > > > > > > > - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm,
> > > > > > > > slave_crtc,
> > > > > > > > -
> > > > > > > > intel_crtc_bigjoiner_slave_pipes(old_crtc_state)) {
> > > > > > > > - const struct intel_crtc_state
> > > > > > > > *old_slave_crtc_state =
> > > > > > > > - intel_atomic_get_old_crtc_state(state,
> > > > > > > > slave_crtc);
> > > > > > > > -
> > > > > > > > - intel_crtc_vblank_off(old_slave_crtc_state);
> > > > > > > > -
> > > > > > > > - intel_dsc_disable(old_slave_crtc_state);
> > > > > > > > - skl_scaler_disable(old_slave_crtc_state);
> > > > > > > > - }
> > > > > > >
> > > > > > > .. but now you're moving the slave pipe stuff somewhere else?
> > > > > > >
> > > > > > > We should be just iterating the pipes here (assuming this
> > > > > > > is the correct spot to do these steps).
> > > > > > >
> > > > > > > > -
> > > > > > > > /*
> > > > > > > > * When called from DP MST code:
> > > > > > > > * - old_conn_state will be NULL
> > > > > > > > @@ -3363,8 +3351,7 @@ static void intel_enable_ddi(struct
> > > > > > > > intel_atomic_state *state,
> > > > > > > > {
> > > > > > > > drm_WARN_ON(state->base.dev,
> > > > > > > > crtc_state->has_pch_encoder);
> > > > > > > >
> > > > > > > > - if (!intel_crtc_is_bigjoiner_slave(crtc_state))
> > > > > > > > - intel_ddi_enable_transcoder_func(encoder,
> > > > > > > > crtc_state);
> > > > > > > > + intel_ddi_enable_transcoder_func(encoder, crtc_state);
> > > > > > > >
> > > > > > > > /* Enable/Disable DP2.0 SDP split config before
> > > > > > > > transcoder */
> > > > > > > > intel_audio_sdp_split_update(crtc_state);
> > > > > > > > @@ -3469,9 +3456,6 @@ void intel_ddi_update_active_dpll(struct
> > > > > > > > intel_atomic_state *state,
> > > > > > > > struct intel_crtc *crtc)
> > > > > > > > {
> > > > > > > > struct drm_i915_private *i915 =
> > > > > > > > to_i915(encoder->base.dev);
> > > > > > > > - struct intel_crtc_state *crtc_state =
> > > > > > > > - intel_atomic_get_new_crtc_state(state, crtc);
> > > > > > > > - struct intel_crtc *slave_crtc;
> > > > > > > > enum phy phy = intel_port_to_phy(i915, encoder->port);
> > > > > > > >
> > > > > > > > /* FIXME: Add MTL pll_mgr */
> > > > > > > > @@ -3479,9 +3463,6 @@ void intel_ddi_update_active_dpll(struct
> > > > > > > > intel_atomic_state *state,
> > > > > > > > return;
> > > > > > > >
> > > > > > > > intel_update_active_dpll(state, crtc, encoder);
> > > > > > > > - for_each_intel_crtc_in_pipe_mask(&i915->drm, slave_crtc,
> > > > > > > > -
> > > > > > > > intel_crtc_bigjoiner_slave_pipes(crtc_state))
> > > > > > > > - intel_update_active_dpll(state, slave_crtc,
> > > > > > > > encoder);
> > > > > > > > }
> > > > > > > >
> > > > > > > > static void
> > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > > index 916c13a149fd5..e1ea53fd6a288 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > > @@ -1631,31 +1631,12 @@ static void
> > > > > > > > hsw_configure_cpu_transcoder(const struct intel_crtc_state
> > > > > > > > *crtc_sta
> > > > > > > > hsw_set_transconf(crtc_state);
> > > > > > > > }
> > > > > > > >
> > > > > > > > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > > > > > > - struct intel_crtc *crtc)
> > > > > > > > +static void hsw_crtc_enable_pre_transcoder(struct
> > > > > > > > intel_atomic_state *state,
> > > > > > > > + struct intel_crtc
> > > > > > > > *crtc)
> > > > > > > > {
> > > > > > > > const struct intel_crtc_state *new_crtc_state =
> > > > > > > > intel_atomic_get_new_crtc_state(state, crtc);
> > > > > > > > struct drm_i915_private *dev_priv =
> > > > > > > > to_i915(crtc->base.dev);
> > > > > > > > - enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > > > > > > > - enum transcoder cpu_transcoder =
> > > > > > > > new_crtc_state->cpu_transcoder;
> > > > > > > > - bool psl_clkgate_wa;
> > > > > > > > -
> > > > > > > > - if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > > > > > > > - return;
> > > > > > > > -
> > > > > > > > - intel_dmc_enable_pipe(dev_priv, crtc->pipe);
> > > > > > > > -
> > > > > > > > - if (!new_crtc_state->bigjoiner_pipes) {
> > > > > > > > - intel_encoders_pre_pll_enable(state, crtc);
> > > > > > > > -
> > > > > > > > - if (new_crtc_state->shared_dpll)
> > > > > > > > -
> > > > > > > > intel_enable_shared_dpll(new_crtc_state);
> > > > > > > > -
> > > > > > > > - intel_encoders_pre_enable(state, crtc);
> > > > > > > > - } else {
> > > > > > > > - icl_ddi_bigjoiner_pre_enable(state,
> > > > > > > > new_crtc_state);
> > > > > > > > - }
> > > > > > > >
> > > > > > > > intel_dsc_enable(new_crtc_state);
> > > > > > > >
> > > > > > > > @@ -1665,19 +1646,17 @@ static void hsw_crtc_enable(struct
> > > > > > > > intel_atomic_state *state,
> > > > > > > > intel_set_pipe_src_size(new_crtc_state);
> > > > > > > > if (DISPLAY_VER(dev_priv) >= 9 ||
> > > > > > > > IS_BROADWELL(dev_priv))
> > > > > > > > bdw_set_pipe_misc(new_crtc_state);
> > > > > > > > +}
> > > > > > > >
> > > > > > > > - if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> > > > > > > > - !transcoder_is_dsi(cpu_transcoder))
> > > > > > > > - hsw_configure_cpu_transcoder(new_crtc_state);
> > > > > > > > +static void hsw_crtc_enable_post_transcoder(struct
> > > > > > > > intel_atomic_state *state,
> > > > > > > > + struct intel_crtc
> > > > > > > > *crtc)
> > > > > > > > +{
> > > > > > > > + const struct intel_crtc_state *new_crtc_state =
> > > > > > > > + intel_atomic_get_new_crtc_state(state, crtc);
> > > > > > > > + struct drm_i915_private *dev_priv =
> > > > > > > > to_i915(crtc->base.dev);
> > > > > > > >
> > > > > > > > crtc->active = true;
> > > > > > > >
> > > > > > > > - /* Display WA #1180: WaDisableScalarClockGating: glk */
> > > > > > > > - psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> > > > > > > > - new_crtc_state->pch_pfit.enabled;
> > > > > > > > - if (psl_clkgate_wa)
> > > > > > > > - glk_pipe_scaler_clock_gating_wa(dev_priv, pipe,
> > > > > > > > true);
> > > > > > > > -
> > > > > > > > if (DISPLAY_VER(dev_priv) >= 9)
> > > > > > > > skl_pfit_enable(new_crtc_state);
> > > > > > > > else
> > > > > > > > @@ -1700,27 +1679,84 @@ static void hsw_crtc_enable(struct
> > > > > > > > intel_atomic_state *state,
> > > > > > > > icl_set_pipe_chicken(new_crtc_state);
> > > > > > > >
> > > > > > > > intel_initial_watermarks(state, crtc);
> > > > > > > > +}
> > > > > > > >
> > > > > > > > - if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > > > > > > > - intel_crtc_vblank_on(new_crtc_state);
> > > > > > > > +static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > > > > > > + struct intel_crtc *crtc)
> > > > > > > > +{
> > > > > > > > + const struct intel_crtc_state *new_crtc_state =
> > > > > > > > + intel_atomic_get_new_crtc_state(state, crtc);
> > > > > > > > + struct drm_i915_private *dev_priv =
> > > > > > > > to_i915(crtc->base.dev);
> > > > > > > > + enum transcoder cpu_transcoder =
> > > > > > > > new_crtc_state->cpu_transcoder;
> > > > > > > > + struct intel_crtc *_crtc;
> > > > > > > > + int slave_pipe_mask =
> > > > > > > > intel_crtc_bigjoiner_slave_pipes(new_crtc_state);
> > > > > > > > + int pipe_mask = slave_pipe_mask | BIT(crtc->pipe);
> > > > > > > > + bool psl_clkgate_wa;
> > > > > > > > + enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > > > > > > >
> > > > > > > > - intel_encoders_enable(state, crtc);
> > > > > > > > + if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > > > > > > > + return;
> > > > > > > >
> > > > > > > > - if (psl_clkgate_wa) {
> > > > > > > > - intel_crtc_wait_for_next_vblank(crtc);
> > > > > > > > - glk_pipe_scaler_clock_gating_wa(dev_priv, pipe,
> > > > > > > > false);
> > > > > > > > - }
> > > > > > > > + /*
> > > > > > > > + * Use reverse iterator to go through slave pipes first.
> > > > > > > > + * TODO: We might need smarter iterator here
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > for_each_intel_crtc_in_pipe_mask_reverse(&dev_priv->drm, _crtc,
> > > > > > > > + pipe_mask) {
> > > > > > > > + const struct intel_crtc_state *_new_crtc_state =
> > > > > > > > + intel_atomic_get_new_crtc_state(state,
> > > > > > > > _crtc);
> > > > > > > > + bool needs_transcoder = ((slave_pipe_mask &
> > > > > > > > BIT(_crtc->pipe)) == 0) &&
> > > > > > > > +
> > > > > > > > !transcoder_is_dsi(cpu_transcoder);
> > > > > > > > +
> > > > > > > > + intel_dmc_enable_pipe(dev_priv, crtc->pipe);
> > > > > > > > +
> > > > > > > > + if (!new_crtc_state->bigjoiner_pipes) {
> > > > > > > > + if (needs_transcoder)
> > > > > > > > +
> > > > > > > > intel_encoders_pre_pll_enable(state, crtc);
> > > > > > > > +
> > > > > > > > + if (new_crtc_state->shared_dpll)
> > > > > > > > +
> > > > > > > > intel_enable_shared_dpll(new_crtc_state);
> > > > > > > > +
> > > > > > > > + if (needs_transcoder)
> > > > > > > > +
> > > > > > > > intel_encoders_pre_enable(state, crtc);
> > > > > > > > + } else {
> > > > > > > > + icl_ddi_bigjoiner_pre_enable(state,
> > > > > > > > new_crtc_state);
> > > > > > > > + }
> > > > > > >
> > > > > > > That mess needs to be eliminated entirely.
> > > > > >
> > > > > > Yeah, was thinking about this too, was just a bit unsure how..
> > > > > >
> > > > > > >
> > > > > > > > +
> > > > > > > > + hsw_crtc_enable_pre_transcoder(state, _crtc);
> > > > > > > > +
> > > > > > > > + if (needs_transcoder)
> > > > > > > > +
> > > > > > > > hsw_configure_cpu_transcoder(_new_crtc_state);
> > > > > > >
> > > > > > > These transcoder things should not be within any pipe loop at all.
> > > > > >
> > > > > > I didn't want to split the loop, which I would have to do otherwise,
> > > > > > but may be it makes sense, since transcoder path is needed only for
> > > > > > master
> > > > > > pipe. However what if _hypothetically_ :) we would have more than
> > > > > > one master
> > > > > > pipe?
> > > > >
> > > > > Doesn't matter how many pipes there are. There is always just one
> > > > > transcoder.
> > > >
> > > > How I see it now, the sequence for example for master hsw_crtc_enable
> > > > is:
> > > >
> > > > pipe1 stuff before transcoder
> > > > (transcoder programming not needed)
> > > > pipe1 stuff after transcoder
> > > >
> > > > pipe0 stuff before transcoder
> > > > transcoder programmed
> > > > pipe0 stuff after transcoder
> > > >
> > > > if we want to have a single unified loop for all pipes, I guess we have
> > > > to
> > > > have a transcoder check inside a loop, because we cant do the
> > > > "pipe0 stuff after transcoder" thing, before the transcoder is
> > > > programmed.
> > > >
> > > > I could of course split it this way:
> > > >
> > > > for (...)
> > > > pipe stuff before transcoder
> > > >
> > > > transcoder programmed
> > > >
> > > > for (...)
> > > > pipe stuff after transcoder
> > > >
> > > > but then the sequence still will be different from original, it will
> > > > look like:
> > > > pipe1 stuff before transcoder
> > > >
> > > > pipe0 stuff before transcoder
> > > >
> > > > transcoder programmed
> > > >
> > > > pipe1 stuff after transcoder
> > > >
> > > > pipe0 stuff after transcoder
> > > >
> > > > which is different from original sequence, because we in fact
> > > > want that:
> > > > program pipe1(slave)
> > > >
> > > > program pipe0 stuff before transcoder(master)
> > > > program transcoder
> > > > program pipe0 stuff after transcoder(master)
> > > >
> > > > So do you think that splitting won't harm or you see some other way to
> > > > do that?
> > >
> > > The current code is mostly nonsense I think. Probably only work
> > > through the power of prayer. I think we need to be able to control
> > > the per-pipe vs. per-transcoder steps more freely to make it actually
> > > correct.
> >
> >
> > Ville, could you communicate to me next time, if you decide to do everything
> > yourself or you see it some other way.
> > Basically I was trying to discuss how you see things here and then it just
> > ends
> > up that way.
> > If you didn't like the whole approach why I hear this only now, was
> > following
> > all your comments and asking questions.
> > You could either communicate your ideas to me or at least communicate that
> > you
> > plan to send own series..
> > Now we had a discussion, some particular things were discussed, then
> > suddenly
> > you say that everything is crap and send own series.
> >
> > Is that really professional team work?
> >
> > Stan
> >
> > >
> > > I fired off a quick attempt at converting the disable side,
> > > since that is a bit more straightforwad. The end result looks
> > > fairly reasonable to me at least.
> > > https://patchwork.freedesktop.org/series/130619/
> >
> > Ville, could you communicate to me next time, if you decide to do everything
>
> I didn't do everything. Just enough to demonstrate the direction
> where I think we should go.
Okay. What I should do now? Am I supposed to to do the enable side or just
review your
changes? Could you please communicate clearly, what is our plan here.
>
> --
> Ville Syrjälä
> Intel
