Re: [PATCH 2/3] Start separating pipe vs transcoder set logic for bigjoiner during modeset
On Fri, Mar 01, 2024 at 05:42:59PM +0200, Lisovskiy, Stanislav wrote:
> 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
> > > > > > > > >
> > > > > > > > > ---
> > > > > > > > > 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(_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,
> > > > > > > > > {
> > > > > > > > >
Re: [PATCH 2/3] Start separating pipe vs transcoder set logic for bigjoiner during modeset
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
> > > > > > > >
> > > > > > > > ---
> > > > > > > > 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(_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,
Re: [PATCH 2/3] Start separating pipe vs transcoder set logic for bigjoiner during modeset
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
> > > > > > > ---
> > > > > > > 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(_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 =
Re: [PATCH 2/3] Start separating pipe vs transcoder set logic for bigjoiner during modeset
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
> > > > > > ---
> > > > > > 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(_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 =
Re: [PATCH 2/3] Start separating pipe vs transcoder set logic for bigjoiner during modeset
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
> > > > > ---
> > > > > 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(_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,
> > > > >
Re: [PATCH 2/3] Start separating pipe vs transcoder set logic for bigjoiner during modeset
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
> > > > ---
> > > > 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(_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(>drm, slave_crtc,
> > > > -
> > > >
Re: [PATCH 2/3] Start separating pipe vs transcoder set logic for bigjoiner during modeset
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
> > > ---
> > > 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(_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(>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
Re: [PATCH 2/3] Start separating pipe vs transcoder set logic for bigjoiner during modeset
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
> > ---
> > 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(_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(>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
Re: [PATCH 2/3] Start separating pipe vs transcoder set logic for bigjoiner during modeset
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
> ---
> 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(_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(>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 =
>
RE: [PATCH 2/3] Start separating pipe vs transcoder set logic for bigjoiner during modeset
> -Original Message-
> From: Lisovskiy, Stanislav
> Sent: Tuesday, February 27, 2024 2:41 PM
> To: Srinivas, Vidya
> Cc: intel-gfx@lists.freedesktop.org; Saarinen, Jani ;
> ville.syrj...@linux.intel.com
> Subject: Re: [PATCH 2/3] Start separating pipe vs transcoder set logic for
> bigjoiner during modeset
>
> On Tue, Feb 27, 2024 at 06:40:23AM +0200, Srinivas, Vidya wrote:
> >
> >
> > > -Original Message-
> > > From: Lisovskiy, Stanislav
> > > Sent: Thursday, February 22, 2024 12:50 AM
> > > To: intel-gfx@lists.freedesktop.org
> > > Cc: Lisovskiy, Stanislav ; Saarinen,
> > > Jani ; ville.syrj...@linux.intel.com;
> > > Srinivas, Vidya
> > > Subject: [PATCH 2/3] Start separating pipe vs transcoder set logic
> > > for bigjoiner during modeset
> > >
> > > 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
> > > ---
> > > 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);
> > > }
> > >
> > > - for_each_intel_crtc_in_pipe_mask(_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);
> > > - }
> > > -
> > > /*
> > >* 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)
> > >
Re: [PATCH 2/3] Start separating pipe vs transcoder set logic for bigjoiner during modeset
On Tue, Feb 27, 2024 at 06:40:23AM +0200, Srinivas, Vidya wrote:
>
>
> > -Original Message-
> > From: Lisovskiy, Stanislav
> > Sent: Thursday, February 22, 2024 12:50 AM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Lisovskiy, Stanislav ; Saarinen, Jani
> > ; ville.syrj...@linux.intel.com; Srinivas, Vidya
> >
> > Subject: [PATCH 2/3] Start separating pipe vs transcoder set logic for
> > bigjoiner
> > during modeset
> >
> > 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
> > ---
> > 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);
> > }
> >
> > - for_each_intel_crtc_in_pipe_mask(_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);
> > - }
> > -
> > /*
> > * 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(>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
RE: [PATCH 2/3] Start separating pipe vs transcoder set logic for bigjoiner during modeset
> -Original Message-
> From: Intel-gfx On Behalf Of
> Srinivas, Vidya
> Sent: Tuesday, February 27, 2024 10:10 AM
> To: Lisovskiy, Stanislav ; intel-
> g...@lists.freedesktop.org
> Cc: Saarinen, Jani ; ville.syrj...@linux.intel.com
> Subject: RE: [PATCH 2/3] Start separating pipe vs transcoder set logic for
> bigjoiner during modeset
>
>
>
> > -Original Message-
> > From: Lisovskiy, Stanislav
> > Sent: Thursday, February 22, 2024 12:50 AM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Lisovskiy, Stanislav ; Saarinen,
> > Jani ; ville.syrj...@linux.intel.com;
> > Srinivas, Vidya
> > Subject: [PATCH 2/3] Start separating pipe vs transcoder set logic for
> > bigjoiner during modeset
> >
> > 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
> > ---
> > 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);
> > }
> >
> > - for_each_intel_crtc_in_pipe_mask(_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);
> > - }
> > -
> > /*
> > * 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
> &
RE: [PATCH 2/3] Start separating pipe vs transcoder set logic for bigjoiner during modeset
> -Original Message-
> From: Lisovskiy, Stanislav
> Sent: Thursday, February 22, 2024 12:50 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Lisovskiy, Stanislav ; Saarinen, Jani
> ; ville.syrj...@linux.intel.com; Srinivas, Vidya
>
> Subject: [PATCH 2/3] Start separating pipe vs transcoder set logic for
> bigjoiner
> during modeset
>
> 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
> ---
> 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);
> }
>
> - for_each_intel_crtc_in_pipe_mask(_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);
> - }
> -
> /*
>* 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(>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,
