| Message ID | 20240301143600.1334-9-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Bigjoiner stuff | expand |
On Fri, Mar 01, 2024 at 04:36:00PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Reorganize the crtc disable path to only deal with the > master pipes/transcoders in intel_old_crtc_state_disables() > and offload the handling of joined pipes to hsw_crtc_disable(). > This makes the whole thing much more sensible since we can > actually control the order in which we do the per-pipe vs. > per-transcoder modeset steps. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 64 ++++++++++++-------- > 1 file changed, 38 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 1df3923cc30d..07239c1ce9df 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -1793,29 +1793,27 @@ static void hsw_crtc_disable(struct intel_atomic_state *state, > const struct intel_crtc_state *old_master_crtc_state = > intel_atomic_get_old_crtc_state(state, master_crtc); > struct drm_i915_private *i915 = to_i915(master_crtc->base.dev); > + u8 pipe_mask = intel_crtc_joined_pipe_mask(old_master_crtc_state); > + struct intel_crtc *crtc; > > /* > * FIXME collapse everything to one hook. > * Need care with mst->ddi interactions. > */ > - if (!intel_crtc_is_bigjoiner_slave(old_master_crtc_state)) { > - intel_encoders_disable(state, master_crtc); > - intel_encoders_post_disable(state, master_crtc); > - } > - > - intel_disable_shared_dpll(old_master_crtc_state); > + intel_encoders_disable(state, master_crtc); > + intel_encoders_post_disable(state, master_crtc); > > - if (!intel_crtc_is_bigjoiner_slave(old_master_crtc_state)) { > - struct intel_crtc *slave_crtc; > + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) { > + const struct intel_crtc_state *old_crtc_state = > + intel_atomic_get_old_crtc_state(state, crtc); > > - intel_encoders_post_pll_disable(state, master_crtc); > + intel_disable_shared_dpll(old_crtc_state); > + } > > - intel_dmc_disable_pipe(i915, master_crtc->pipe); > + intel_encoders_post_pll_disable(state, master_crtc); > > - for_each_intel_crtc_in_pipe_mask(&i915->drm, slave_crtc, > - intel_crtc_bigjoiner_slave_pipes(old_master_crtc_state)) > - intel_dmc_disable_pipe(i915, slave_crtc->pipe); > - } > + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) > + intel_dmc_disable_pipe(i915, crtc->pipe); > } Okay the only difference from hsw_crtc_disable part from my patch is that I don't have intel_crtc_joined_pipe_mask and encoder calls are outside the pipe loop. Ok. You could of course just communicate this to me, it is quite a small thing to change. And still there is a question about how to handle the crtc enable side, since extracting transcoder programming from the pipe loop, will break the sequence, as I described. Either it is ok that we will partly program slave/master pipe, then program transcoder then again program slave/master pipes or it has to be in a pipe loop. Stan > > static void i9xx_pfit_enable(const struct intel_crtc_state *crtc_state) > @@ -6753,24 +6751,33 @@ static void intel_update_crtc(struct intel_atomic_state *state, > } > > static void intel_old_crtc_state_disables(struct intel_atomic_state *state, > - struct intel_crtc *crtc) > + struct intel_crtc *master_crtc) > { > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > - const struct intel_crtc_state *new_crtc_state = > - intel_atomic_get_new_crtc_state(state, crtc); > + const struct intel_crtc_state *old_master_crtc_state = > + intel_atomic_get_old_crtc_state(state, master_crtc); > + u8 pipe_mask = intel_crtc_joined_pipe_mask(old_master_crtc_state); > + struct intel_crtc *crtc; > > /* > * We need to disable pipe CRC before disabling the pipe, > * or we race against vblank off. > */ > - intel_crtc_disable_pipe_crc(crtc); > + for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, crtc, pipe_mask) > + intel_crtc_disable_pipe_crc(crtc); > > dev_priv->display.funcs.display->crtc_disable(state, crtc); > - crtc->active = false; > - intel_fbc_disable(crtc); > > - if (!new_crtc_state->hw.active) > - intel_initial_watermarks(state, crtc); > + for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, crtc, pipe_mask) { > + const struct intel_crtc_state *new_crtc_state = > + intel_atomic_get_new_crtc_state(state, crtc); > + > + crtc->active = false; > + intel_fbc_disable(crtc); > + > + if (!new_crtc_state->hw.active) > + intel_initial_watermarks(state, crtc); > + } > } > > static void intel_commit_modeset_disables(struct intel_atomic_state *state) > @@ -6810,19 +6817,21 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state) > if ((disable_pipes & BIT(crtc->pipe)) == 0) > continue; > > + if (intel_crtc_is_bigjoiner_slave(old_crtc_state)) > + continue; > + > /* In case of Transcoder port Sync master slave CRTCs can be > * assigned in any order and we need to make sure that > * slave CRTCs are disabled first and then master CRTC since > * Slave vblanks are masked till Master Vblanks. > */ > if (!is_trans_port_sync_slave(old_crtc_state) && > - !intel_dp_mst_is_slave_trans(old_crtc_state) && > - !intel_crtc_is_bigjoiner_slave(old_crtc_state)) > + !intel_dp_mst_is_slave_trans(old_crtc_state)) > continue; > > intel_old_crtc_state_disables(state, crtc); > > - disable_pipes &= ~BIT(crtc->pipe); > + disable_pipes &= ~intel_crtc_joined_pipe_mask(old_crtc_state); > } > > /* Disable everything else left on */ > @@ -6830,9 +6839,12 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state) > if ((disable_pipes & BIT(crtc->pipe)) == 0) > continue; > > + if (intel_crtc_is_bigjoiner_slave(old_crtc_state)) > + continue; > + > intel_old_crtc_state_disables(state, crtc); > > - disable_pipes &= ~BIT(crtc->pipe); > + disable_pipes &= ~intel_crtc_joined_pipe_mask(old_crtc_state); > } > > drm_WARN_ON(&i915->drm, disable_pipes); > -- > 2.43.0 >
On Fri, Mar 01, 2024 at 04:36:00PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Reorganize the crtc disable path to only deal with the > master pipes/transcoders in intel_old_crtc_state_disables() > and offload the handling of joined pipes to hsw_crtc_disable(). > This makes the whole thing much more sensible since we can > actually control the order in which we do the per-pipe vs. > per-transcoder modeset steps. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 64 ++++++++++++-------- > 1 file changed, 38 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 1df3923cc30d..07239c1ce9df 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -1793,29 +1793,27 @@ static void hsw_crtc_disable(struct intel_atomic_state *state, > const struct intel_crtc_state *old_master_crtc_state = > intel_atomic_get_old_crtc_state(state, master_crtc); > struct drm_i915_private *i915 = to_i915(master_crtc->base.dev); > + u8 pipe_mask = intel_crtc_joined_pipe_mask(old_master_crtc_state); > + struct intel_crtc *crtc; > > /* > * FIXME collapse everything to one hook. > * Need care with mst->ddi interactions. > */ > - if (!intel_crtc_is_bigjoiner_slave(old_master_crtc_state)) { > - intel_encoders_disable(state, master_crtc); > - intel_encoders_post_disable(state, master_crtc); > - } > - > - intel_disable_shared_dpll(old_master_crtc_state); > + intel_encoders_disable(state, master_crtc); > + intel_encoders_post_disable(state, master_crtc); > > - if (!intel_crtc_is_bigjoiner_slave(old_master_crtc_state)) { > - struct intel_crtc *slave_crtc; > + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) { > + const struct intel_crtc_state *old_crtc_state = > + intel_atomic_get_old_crtc_state(state, crtc); > > - intel_encoders_post_pll_disable(state, master_crtc); > + intel_disable_shared_dpll(old_crtc_state); > + } > > - intel_dmc_disable_pipe(i915, master_crtc->pipe); > + intel_encoders_post_pll_disable(state, master_crtc); > > - for_each_intel_crtc_in_pipe_mask(&i915->drm, slave_crtc, > - intel_crtc_bigjoiner_slave_pipes(old_master_crtc_state)) > - intel_dmc_disable_pipe(i915, slave_crtc->pipe); > - } > + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) > + intel_dmc_disable_pipe(i915, crtc->pipe); > } > > static void i9xx_pfit_enable(const struct intel_crtc_state *crtc_state) > @@ -6753,24 +6751,33 @@ static void intel_update_crtc(struct intel_atomic_state *state, > } > > static void intel_old_crtc_state_disables(struct intel_atomic_state *state, > - struct intel_crtc *crtc) > + struct intel_crtc *master_crtc) > { > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > - const struct intel_crtc_state *new_crtc_state = > - intel_atomic_get_new_crtc_state(state, crtc); > + const struct intel_crtc_state *old_master_crtc_state = > + intel_atomic_get_old_crtc_state(state, master_crtc); > + u8 pipe_mask = intel_crtc_joined_pipe_mask(old_master_crtc_state); > + struct intel_crtc *crtc; > > /* > * We need to disable pipe CRC before disabling the pipe, > * or we race against vblank off. > */ > - intel_crtc_disable_pipe_crc(crtc); > + for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, crtc, pipe_mask) > + intel_crtc_disable_pipe_crc(crtc); > > dev_priv->display.funcs.display->crtc_disable(state, crtc); > - crtc->active = false; > - intel_fbc_disable(crtc); > > - if (!new_crtc_state->hw.active) > - intel_initial_watermarks(state, crtc); > + for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, crtc, pipe_mask) { > + const struct intel_crtc_state *new_crtc_state = > + intel_atomic_get_new_crtc_state(state, crtc); > + > + crtc->active = false; > + intel_fbc_disable(crtc); > + > + if (!new_crtc_state->hw.active) > + intel_initial_watermarks(state, crtc); > + } > } > > static void intel_commit_modeset_disables(struct intel_atomic_state *state) > @@ -6810,19 +6817,21 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state) > if ((disable_pipes & BIT(crtc->pipe)) == 0) > continue; > > + if (intel_crtc_is_bigjoiner_slave(old_crtc_state)) > + continue; > + > /* In case of Transcoder port Sync master slave CRTCs can be > * assigned in any order and we need to make sure that > * slave CRTCs are disabled first and then master CRTC since > * Slave vblanks are masked till Master Vblanks. > */ > if (!is_trans_port_sync_slave(old_crtc_state) && > - !intel_dp_mst_is_slave_trans(old_crtc_state) && > - !intel_crtc_is_bigjoiner_slave(old_crtc_state)) > + !intel_dp_mst_is_slave_trans(old_crtc_state)) > continue; > > intel_old_crtc_state_disables(state, crtc); > > - disable_pipes &= ~BIT(crtc->pipe); > + disable_pipes &= ~intel_crtc_joined_pipe_mask(old_crtc_state); > } > > /* Disable everything else left on */ > @@ -6830,9 +6839,12 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state) > if ((disable_pipes & BIT(crtc->pipe)) == 0) > continue; > > + if (intel_crtc_is_bigjoiner_slave(old_crtc_state)) > + continue; > + > intel_old_crtc_state_disables(state, crtc); > > - disable_pipes &= ~BIT(crtc->pipe); > + disable_pipes &= ~intel_crtc_joined_pipe_mask(old_crtc_state); Yep btw that one I've also fixed today, based on your comment, also for enable side.. Stan > } > > drm_WARN_ON(&i915->drm, disable_pipes); > -- > 2.43.0 >
On Fri, Mar 01, 2024 at 06:04:27PM +0200, Lisovskiy, Stanislav wrote: > On Fri, Mar 01, 2024 at 04:36:00PM +0200, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Reorganize the crtc disable path to only deal with the > > master pipes/transcoders in intel_old_crtc_state_disables() > > and offload the handling of joined pipes to hsw_crtc_disable(). > > This makes the whole thing much more sensible since we can > > actually control the order in which we do the per-pipe vs. > > per-transcoder modeset steps. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 64 ++++++++++++-------- > > 1 file changed, 38 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index 1df3923cc30d..07239c1ce9df 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -1793,29 +1793,27 @@ static void hsw_crtc_disable(struct intel_atomic_state *state, > > const struct intel_crtc_state *old_master_crtc_state = > > intel_atomic_get_old_crtc_state(state, master_crtc); > > struct drm_i915_private *i915 = to_i915(master_crtc->base.dev); > > + u8 pipe_mask = intel_crtc_joined_pipe_mask(old_master_crtc_state); > > + struct intel_crtc *crtc; > > > > /* > > * FIXME collapse everything to one hook. > > * Need care with mst->ddi interactions. > > */ > > - if (!intel_crtc_is_bigjoiner_slave(old_master_crtc_state)) { > > - intel_encoders_disable(state, master_crtc); > > - intel_encoders_post_disable(state, master_crtc); > > - } > > - > > - intel_disable_shared_dpll(old_master_crtc_state); > > + intel_encoders_disable(state, master_crtc); > > + intel_encoders_post_disable(state, master_crtc); > > > > - if (!intel_crtc_is_bigjoiner_slave(old_master_crtc_state)) { > > - struct intel_crtc *slave_crtc; > > + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) { > > + const struct intel_crtc_state *old_crtc_state = > > + intel_atomic_get_old_crtc_state(state, crtc); > > > > - intel_encoders_post_pll_disable(state, master_crtc); > > + intel_disable_shared_dpll(old_crtc_state); > > + } > > > > - intel_dmc_disable_pipe(i915, master_crtc->pipe); > > + intel_encoders_post_pll_disable(state, master_crtc); > > > > - for_each_intel_crtc_in_pipe_mask(&i915->drm, slave_crtc, > > - intel_crtc_bigjoiner_slave_pipes(old_master_crtc_state)) > > - intel_dmc_disable_pipe(i915, slave_crtc->pipe); > > - } > > + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) > > + intel_dmc_disable_pipe(i915, crtc->pipe); > > } > > Okay the only difference from hsw_crtc_disable part from my patch is that > I don't have intel_crtc_joined_pipe_mask and encoder calls are outside the pipe > loop. Ok. You could of course just communicate this to me, it is quite a small > thing to change. > > And still there is a question about how to handle the crtc enable side, since > extracting transcoder programming from the pipe loop, will break the sequence, > as I described. Either it is ok that we will partly program slave/master pipe, then > program transcoder then again program slave/master pipes or it has to be > in a pipe loop. Transcoder stuff shouldn't be in pipe loops. That's what I've been saying all along.
On Fri, Mar 01, 2024 at 04:36:00PM +0200, Ville Syrjala wrote: > /* > * We need to disable pipe CRC before disabling the pipe, > * or we race against vblank off. > */ > - intel_crtc_disable_pipe_crc(crtc); > + for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, crtc, pipe_mask) > + intel_crtc_disable_pipe_crc(crtc); Oh, and the pipe crc stuff is one thing we probably should spend some brain cells on. The pfit/plane CRCs are per-pipe, so we should either handle it in igt somehow, or we need come up with some kind of scheme to combine the CRCs from all the joined pipes in the kernel so that userspace doesn't have to deal with them.
On Fri, Mar 01, 2024 at 06:10:25PM +0200, Ville Syrjälä wrote: > On Fri, Mar 01, 2024 at 06:04:27PM +0200, Lisovskiy, Stanislav wrote: > > On Fri, Mar 01, 2024 at 04:36:00PM +0200, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Reorganize the crtc disable path to only deal with the > > > master pipes/transcoders in intel_old_crtc_state_disables() > > > and offload the handling of joined pipes to hsw_crtc_disable(). > > > This makes the whole thing much more sensible since we can > > > actually control the order in which we do the per-pipe vs. > > > per-transcoder modeset steps. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 64 ++++++++++++-------- > > > 1 file changed, 38 insertions(+), 26 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > index 1df3923cc30d..07239c1ce9df 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -1793,29 +1793,27 @@ static void hsw_crtc_disable(struct intel_atomic_state *state, > > > const struct intel_crtc_state *old_master_crtc_state = > > > intel_atomic_get_old_crtc_state(state, master_crtc); > > > struct drm_i915_private *i915 = to_i915(master_crtc->base.dev); > > > + u8 pipe_mask = intel_crtc_joined_pipe_mask(old_master_crtc_state); > > > + struct intel_crtc *crtc; > > > > > > /* > > > * FIXME collapse everything to one hook. > > > * Need care with mst->ddi interactions. > > > */ > > > - if (!intel_crtc_is_bigjoiner_slave(old_master_crtc_state)) { > > > - intel_encoders_disable(state, master_crtc); > > > - intel_encoders_post_disable(state, master_crtc); > > > - } > > > - > > > - intel_disable_shared_dpll(old_master_crtc_state); > > > + intel_encoders_disable(state, master_crtc); > > > + intel_encoders_post_disable(state, master_crtc); > > > > > > - if (!intel_crtc_is_bigjoiner_slave(old_master_crtc_state)) { > > > - struct intel_crtc *slave_crtc; > > > + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) { > > > + const struct intel_crtc_state *old_crtc_state = > > > + intel_atomic_get_old_crtc_state(state, crtc); > > > > > > - intel_encoders_post_pll_disable(state, master_crtc); > > > + intel_disable_shared_dpll(old_crtc_state); > > > + } > > > > > > - intel_dmc_disable_pipe(i915, master_crtc->pipe); > > > + intel_encoders_post_pll_disable(state, master_crtc); > > > > > > - for_each_intel_crtc_in_pipe_mask(&i915->drm, slave_crtc, > > > - intel_crtc_bigjoiner_slave_pipes(old_master_crtc_state)) > > > - intel_dmc_disable_pipe(i915, slave_crtc->pipe); > > > - } > > > + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) > > > + intel_dmc_disable_pipe(i915, crtc->pipe); > > > } > > > > Okay the only difference from hsw_crtc_disable part from my patch is that > > I don't have intel_crtc_joined_pipe_mask and encoder calls are outside the pipe > > loop. Ok. You could of course just communicate this to me, it is quite a small > > thing to change. > > > > And still there is a question about how to handle the crtc enable side, since > > extracting transcoder programming from the pipe loop, will break the sequence, > > as I described. Either it is ok that we will partly program slave/master pipe, then > > program transcoder then again program slave/master pipes or it has to be > > in a pipe loop. > > Transcoder stuff shouldn't be in pipe loops. That's what > I've been saying all along. Yep, I realize you kept saying this and I described you the problem what happens if we extract it from there. Either it is ok to have 2 loops and have transcoder programming in between or you first program pipes then program the transcoder - in both cases that would change the sequence of how it is done now. My question was if this is ok or not. Stan > > -- > Ville Syrjälä > Intel
On Fri, Mar 01, 2024 at 06:22:19PM +0200, Lisovskiy, Stanislav wrote: > On Fri, Mar 01, 2024 at 06:10:25PM +0200, Ville Syrjälä wrote: > > On Fri, Mar 01, 2024 at 06:04:27PM +0200, Lisovskiy, Stanislav wrote: > > > On Fri, Mar 01, 2024 at 04:36:00PM +0200, Ville Syrjala wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Reorganize the crtc disable path to only deal with the > > > > master pipes/transcoders in intel_old_crtc_state_disables() > > > > and offload the handling of joined pipes to hsw_crtc_disable(). > > > > This makes the whole thing much more sensible since we can > > > > actually control the order in which we do the per-pipe vs. > > > > per-transcoder modeset steps. > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_display.c | 64 ++++++++++++-------- > > > > 1 file changed, 38 insertions(+), 26 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > index 1df3923cc30d..07239c1ce9df 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > @@ -1793,29 +1793,27 @@ static void hsw_crtc_disable(struct intel_atomic_state *state, > > > > const struct intel_crtc_state *old_master_crtc_state = > > > > intel_atomic_get_old_crtc_state(state, master_crtc); > > > > struct drm_i915_private *i915 = to_i915(master_crtc->base.dev); > > > > + u8 pipe_mask = intel_crtc_joined_pipe_mask(old_master_crtc_state); > > > > + struct intel_crtc *crtc; > > > > > > > > /* > > > > * FIXME collapse everything to one hook. > > > > * Need care with mst->ddi interactions. > > > > */ > > > > - if (!intel_crtc_is_bigjoiner_slave(old_master_crtc_state)) { > > > > - intel_encoders_disable(state, master_crtc); > > > > - intel_encoders_post_disable(state, master_crtc); > > > > - } > > > > - > > > > - intel_disable_shared_dpll(old_master_crtc_state); > > > > + intel_encoders_disable(state, master_crtc); > > > > + intel_encoders_post_disable(state, master_crtc); > > > > > > > > - if (!intel_crtc_is_bigjoiner_slave(old_master_crtc_state)) { > > > > - struct intel_crtc *slave_crtc; > > > > + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) { > > > > + const struct intel_crtc_state *old_crtc_state = > > > > + intel_atomic_get_old_crtc_state(state, crtc); > > > > > > > > - intel_encoders_post_pll_disable(state, master_crtc); > > > > + intel_disable_shared_dpll(old_crtc_state); > > > > + } > > > > > > > > - intel_dmc_disable_pipe(i915, master_crtc->pipe); > > > > + intel_encoders_post_pll_disable(state, master_crtc); > > > > > > > > - for_each_intel_crtc_in_pipe_mask(&i915->drm, slave_crtc, > > > > - intel_crtc_bigjoiner_slave_pipes(old_master_crtc_state)) > > > > - intel_dmc_disable_pipe(i915, slave_crtc->pipe); > > > > - } > > > > + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) > > > > + intel_dmc_disable_pipe(i915, crtc->pipe); > > > > } > > > > > > Okay the only difference from hsw_crtc_disable part from my patch is that > > > I don't have intel_crtc_joined_pipe_mask and encoder calls are outside the pipe > > > loop. Ok. You could of course just communicate this to me, it is quite a small > > > thing to change. > > > > > > And still there is a question about how to handle the crtc enable side, since > > > extracting transcoder programming from the pipe loop, will break the sequence, > > > as I described. Either it is ok that we will partly program slave/master pipe, then > > > program transcoder then again program slave/master pipes or it has to be > > > in a pipe loop. > > > > Transcoder stuff shouldn't be in pipe loops. That's what > > I've been saying all along. > > Yep, I realize you kept saying this and I described you the problem what happens if > we extract it from there. > Either it is ok to have 2 loops and have transcoder programming in between or you > first program pipes then program the transcoder - in both cases that would change > the sequence of how it is done now. > My question was if this is ok or not. Well, that's pretty much it's supposed to be done. As mentioned I think the current code kinda works more by luck. I suppose the only reason it works at all is that we do try to order at least some of the steps via the tricks in icl_ddi_bigjoiner_pre_enable() and the specific ordering of the crtcs from the modeset_enables/disables(). But I'm pretty sure there are some steps that currently get done in different places for the master and slace pipes. And that's not by design. In general it's pretty hard to actually figure out what steps are being done in which order in the current code. The "is it OK?" question I think is best answered by asking the real hardware. If there is some specific ordering requirement that the current code accidentally gets right but the obvious code would somehow get wrong, the hardware should be able to tell us pretty quickly.
On Fri, Mar 01, 2024 at 06:47:54PM +0200, Ville Syrjälä wrote: > On Fri, Mar 01, 2024 at 06:22:19PM +0200, Lisovskiy, Stanislav wrote: > > On Fri, Mar 01, 2024 at 06:10:25PM +0200, Ville Syrjälä wrote: > > > On Fri, Mar 01, 2024 at 06:04:27PM +0200, Lisovskiy, Stanislav wrote: > > > > On Fri, Mar 01, 2024 at 04:36:00PM +0200, Ville Syrjala wrote: > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > Reorganize the crtc disable path to only deal with the > > > > > master pipes/transcoders in intel_old_crtc_state_disables() > > > > > and offload the handling of joined pipes to hsw_crtc_disable(). > > > > > This makes the whole thing much more sensible since we can > > > > > actually control the order in which we do the per-pipe vs. > > > > > per-transcoder modeset steps. > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_display.c | 64 ++++++++++++-------- > > > > > 1 file changed, 38 insertions(+), 26 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > > index 1df3923cc30d..07239c1ce9df 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > @@ -1793,29 +1793,27 @@ static void hsw_crtc_disable(struct intel_atomic_state *state, > > > > > const struct intel_crtc_state *old_master_crtc_state = > > > > > intel_atomic_get_old_crtc_state(state, master_crtc); > > > > > struct drm_i915_private *i915 = to_i915(master_crtc->base.dev); > > > > > + u8 pipe_mask = intel_crtc_joined_pipe_mask(old_master_crtc_state); > > > > > + struct intel_crtc *crtc; > > > > > > > > > > /* > > > > > * FIXME collapse everything to one hook. > > > > > * Need care with mst->ddi interactions. > > > > > */ > > > > > - if (!intel_crtc_is_bigjoiner_slave(old_master_crtc_state)) { > > > > > - intel_encoders_disable(state, master_crtc); > > > > > - intel_encoders_post_disable(state, master_crtc); > > > > > - } > > > > > - > > > > > - intel_disable_shared_dpll(old_master_crtc_state); > > > > > + intel_encoders_disable(state, master_crtc); > > > > > + intel_encoders_post_disable(state, master_crtc); > > > > > > > > > > - if (!intel_crtc_is_bigjoiner_slave(old_master_crtc_state)) { > > > > > - struct intel_crtc *slave_crtc; > > > > > + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) { > > > > > + const struct intel_crtc_state *old_crtc_state = > > > > > + intel_atomic_get_old_crtc_state(state, crtc); > > > > > > > > > > - intel_encoders_post_pll_disable(state, master_crtc); > > > > > + intel_disable_shared_dpll(old_crtc_state); > > > > > + } > > > > > > > > > > - intel_dmc_disable_pipe(i915, master_crtc->pipe); > > > > > + intel_encoders_post_pll_disable(state, master_crtc); > > > > > > > > > > - for_each_intel_crtc_in_pipe_mask(&i915->drm, slave_crtc, > > > > > - intel_crtc_bigjoiner_slave_pipes(old_master_crtc_state)) > > > > > - intel_dmc_disable_pipe(i915, slave_crtc->pipe); > > > > > - } > > > > > + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) > > > > > + intel_dmc_disable_pipe(i915, crtc->pipe); > > > > > } > > > > > > > > Okay the only difference from hsw_crtc_disable part from my patch is that > > > > I don't have intel_crtc_joined_pipe_mask and encoder calls are outside the pipe > > > > loop. Ok. You could of course just communicate this to me, it is quite a small > > > > thing to change. > > > > > > > > And still there is a question about how to handle the crtc enable side, since > > > > extracting transcoder programming from the pipe loop, will break the sequence, > > > > as I described. Either it is ok that we will partly program slave/master pipe, then > > > > program transcoder then again program slave/master pipes or it has to be > > > > in a pipe loop. > > > > > > Transcoder stuff shouldn't be in pipe loops. That's what > > > I've been saying all along. > > > > Yep, I realize you kept saying this and I described you the problem what happens if > > we extract it from there. > > Either it is ok to have 2 loops and have transcoder programming in between or you > > first program pipes then program the transcoder - in both cases that would change > > the sequence of how it is done now. > > My question was if this is ok or not. > > Well, that's pretty much it's supposed to be done. As mentioned > I think the current code kinda works more by luck. > > I suppose the only reason it works at all is that we do try to order > at least some of the steps via the tricks in > icl_ddi_bigjoiner_pre_enable() and the specific ordering of the crtcs > from the modeset_enables/disables(). But I'm pretty sure there are > some steps that currently get done in different places for > the master and slace pipes. And that's not by design. > > In general it's pretty hard to actually figure out what steps are > being done in which order in the current code. > > The "is it OK?" question I think is best answered by asking the > real hardware. If there is some specific ordering requirement > that the current code accidentally gets right but the obvious > code would somehow get wrong, the hardware should be able to > tell us pretty quickly. I think this has to be spec to follow or somekind of agreement, trial and error method doesn't sound like right approach, even if hardware works someway, doesn't mean it is supposed to be like that. That is why I didn't want to change the original sequence of calls, assuming there was some reason behind it. Stan > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 1df3923cc30d..07239c1ce9df 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -1793,29 +1793,27 @@ static void hsw_crtc_disable(struct intel_atomic_state *state, const struct intel_crtc_state *old_master_crtc_state = intel_atomic_get_old_crtc_state(state, master_crtc); struct drm_i915_private *i915 = to_i915(master_crtc->base.dev); + u8 pipe_mask = intel_crtc_joined_pipe_mask(old_master_crtc_state); + struct intel_crtc *crtc; /* * FIXME collapse everything to one hook. * Need care with mst->ddi interactions. */ - if (!intel_crtc_is_bigjoiner_slave(old_master_crtc_state)) { - intel_encoders_disable(state, master_crtc); - intel_encoders_post_disable(state, master_crtc); - } - - intel_disable_shared_dpll(old_master_crtc_state); + intel_encoders_disable(state, master_crtc); + intel_encoders_post_disable(state, master_crtc); - if (!intel_crtc_is_bigjoiner_slave(old_master_crtc_state)) { - struct intel_crtc *slave_crtc; + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) { + const struct intel_crtc_state *old_crtc_state = + intel_atomic_get_old_crtc_state(state, crtc); - intel_encoders_post_pll_disable(state, master_crtc); + intel_disable_shared_dpll(old_crtc_state); + } - intel_dmc_disable_pipe(i915, master_crtc->pipe); + intel_encoders_post_pll_disable(state, master_crtc); - for_each_intel_crtc_in_pipe_mask(&i915->drm, slave_crtc, - intel_crtc_bigjoiner_slave_pipes(old_master_crtc_state)) - intel_dmc_disable_pipe(i915, slave_crtc->pipe); - } + for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, pipe_mask) + intel_dmc_disable_pipe(i915, crtc->pipe); } static void i9xx_pfit_enable(const struct intel_crtc_state *crtc_state) @@ -6753,24 +6751,33 @@ static void intel_update_crtc(struct intel_atomic_state *state, } static void intel_old_crtc_state_disables(struct intel_atomic_state *state, - struct intel_crtc *crtc) + struct intel_crtc *master_crtc) { struct drm_i915_private *dev_priv = to_i915(state->base.dev); - const struct intel_crtc_state *new_crtc_state = - intel_atomic_get_new_crtc_state(state, crtc); + const struct intel_crtc_state *old_master_crtc_state = + intel_atomic_get_old_crtc_state(state, master_crtc); + u8 pipe_mask = intel_crtc_joined_pipe_mask(old_master_crtc_state); + struct intel_crtc *crtc; /* * We need to disable pipe CRC before disabling the pipe, * or we race against vblank off. */ - intel_crtc_disable_pipe_crc(crtc); + for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, crtc, pipe_mask) + intel_crtc_disable_pipe_crc(crtc); dev_priv->display.funcs.display->crtc_disable(state, crtc); - crtc->active = false; - intel_fbc_disable(crtc); - if (!new_crtc_state->hw.active) - intel_initial_watermarks(state, crtc); + for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, crtc, pipe_mask) { + const struct intel_crtc_state *new_crtc_state = + intel_atomic_get_new_crtc_state(state, crtc); + + crtc->active = false; + intel_fbc_disable(crtc); + + if (!new_crtc_state->hw.active) + intel_initial_watermarks(state, crtc); + } } static void intel_commit_modeset_disables(struct intel_atomic_state *state) @@ -6810,19 +6817,21 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state) if ((disable_pipes & BIT(crtc->pipe)) == 0) continue; + if (intel_crtc_is_bigjoiner_slave(old_crtc_state)) + continue; + /* In case of Transcoder port Sync master slave CRTCs can be * assigned in any order and we need to make sure that * slave CRTCs are disabled first and then master CRTC since * Slave vblanks are masked till Master Vblanks. */ if (!is_trans_port_sync_slave(old_crtc_state) && - !intel_dp_mst_is_slave_trans(old_crtc_state) && - !intel_crtc_is_bigjoiner_slave(old_crtc_state)) + !intel_dp_mst_is_slave_trans(old_crtc_state)) continue; intel_old_crtc_state_disables(state, crtc); - disable_pipes &= ~BIT(crtc->pipe); + disable_pipes &= ~intel_crtc_joined_pipe_mask(old_crtc_state); } /* Disable everything else left on */ @@ -6830,9 +6839,12 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state) if ((disable_pipes & BIT(crtc->pipe)) == 0) continue; + if (intel_crtc_is_bigjoiner_slave(old_crtc_state)) + continue; + intel_old_crtc_state_disables(state, crtc); - disable_pipes &= ~BIT(crtc->pipe); + disable_pipes &= ~intel_crtc_joined_pipe_mask(old_crtc_state); } drm_WARN_ON(&i915->drm, disable_pipes);