| Message ID | 20240911131349.933814-17-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Ultrajoiner basic functionality series | expand |
On Wed, Sep 11, 2024 at 06:43:46PM +0530, Ankit Nautiyal wrote: > From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > Ultrajoiner case requires special treatment where both reverse and > staight order iteration doesn't work(for instance disabling case requires > order to be: primary master, slaves, secondary master). > > Lets unify our approach by using not only pipe masks for iterating required > pipes based on joiner type used, but also using different "priority" arrays > for each of those. > > v2: Fix checkpatch warnings. (Ankit) > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 19 +++-- > drivers/gpu/drm/i915/display/intel_display.c | 83 ++++++++++++++++---- > drivers/gpu/drm/i915/display/intel_display.h | 7 ++ > drivers/gpu/drm/i915/display/intel_dp_mst.c | 18 +++-- > 4 files changed, 96 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index 00fbe9f8c03a..2c064b6c6d01 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -3116,10 +3116,11 @@ static void intel_ddi_post_disable_hdmi_or_sst(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 *pipe_crtc; > + struct intel_crtc *pipe_crtc; enum pipe pipe; > > - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, > - intel_crtc_joined_pipe_mask(old_crtc_state)) { > + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, > + intel_crtc_joined_pipe_mask(old_crtc_state), > + intel_get_pipe_order_disable(old_crtc_state)) { > const struct intel_crtc_state *old_pipe_crtc_state = > intel_atomic_get_old_crtc_state(state, pipe_crtc); > > @@ -3130,8 +3131,9 @@ static void intel_ddi_post_disable_hdmi_or_sst(struct intel_atomic_state *state, > > intel_ddi_disable_transcoder_func(old_crtc_state); > > - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, > - intel_crtc_joined_pipe_mask(old_crtc_state)) { > + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, > + intel_crtc_joined_pipe_mask(old_crtc_state), > + intel_get_pipe_order_disable(old_crtc_state)) { > const struct intel_crtc_state *old_pipe_crtc_state = > intel_atomic_get_old_crtc_state(state, pipe_crtc); > > @@ -3383,7 +3385,7 @@ static void intel_enable_ddi(struct intel_atomic_state *state, > const struct drm_connector_state *conn_state) > { > struct drm_i915_private *i915 = to_i915(encoder->base.dev); > - struct intel_crtc *pipe_crtc; > + struct intel_crtc *pipe_crtc; enum pipe pipe; > > intel_ddi_enable_transcoder_func(encoder, crtc_state); > > @@ -3394,8 +3396,9 @@ static void intel_enable_ddi(struct intel_atomic_state *state, > > intel_ddi_wait_for_fec_status(encoder, crtc_state, true); > > - for_each_intel_crtc_in_pipe_mask_reverse(&i915->drm, pipe_crtc, > - intel_crtc_joined_pipe_mask(crtc_state)) { > + for_each_intel_crtc_in_mask_priority(i915, pipe_crtc, pipe, > + intel_crtc_joined_pipe_mask(crtc_state), > + intel_get_pipe_order_enable(crtc_state)) { > const struct intel_crtc_state *pipe_crtc_state = > intel_atomic_get_new_crtc_state(state, pipe_crtc); > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index db27850b2c36..27622d51a473 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -1737,6 +1737,50 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta > hsw_set_transconf(crtc_state); > } > > +static > +bool intel_crtc_is_bigjoiner(const struct intel_crtc_state *pipe_config) > +{ > + return hweight8(pipe_config->joiner_pipes) == 2; > +} > + > +const enum pipe *intel_get_pipe_order_enable(const struct intel_crtc_state *crtc_state) > +{ > + static const enum pipe ultrajoiner_pipe_order_enable[I915_MAX_PIPES] = { > + PIPE_B, PIPE_D, PIPE_C, PIPE_A > + }; > + static const enum pipe bigjoiner_pipe_order_enable[I915_MAX_PIPES] = { > + PIPE_B, PIPE_A, PIPE_D, PIPE_C > + }; > + static const enum pipe nojoiner_pipe_order_enable[I915_MAX_PIPES] = { > + PIPE_A, PIPE_B, PIPE_C, PIPE_D > + }; > + > + if (intel_crtc_is_ultrajoiner(crtc_state)) > + return ultrajoiner_pipe_order_enable; > + else if (intel_crtc_is_bigjoiner(crtc_state)) > + return bigjoiner_pipe_order_enable; > + return nojoiner_pipe_order_enable; > +} > + > +const enum pipe *intel_get_pipe_order_disable(const struct intel_crtc_state *crtc_state) > +{ > + static const enum pipe ultrajoiner_pipe_order_disable[I915_MAX_PIPES] = { > + PIPE_A, PIPE_B, PIPE_D, PIPE_C > + }; > + static const enum pipe bigjoiner_pipe_order_disable[I915_MAX_PIPES] = { > + PIPE_A, PIPE_B, PIPE_C, PIPE_D > + }; > + static const enum pipe nojoiner_pipe_order_disable[I915_MAX_PIPES] = { > + PIPE_A, PIPE_B, PIPE_C, PIPE_D > + }; > + > + if (intel_crtc_is_ultrajoiner(crtc_state)) > + return ultrajoiner_pipe_order_disable; > + else if (intel_crtc_is_bigjoiner(crtc_state)) > + return bigjoiner_pipe_order_disable; > + return nojoiner_pipe_order_disable; I don't think we should need all those diffrent order array. Technically one should do. Though having two might make sense. Another problem is the hardcoded pipes. If we eg. get hardware that would support ultrajoiner on pipes B-E in the future this would no longer work. > +} <snip> > +#define for_each_intel_crtc_in_mask_priority(__dev_priv, intel_crtc, __p, __mask, __priolist) \ > + for_each_pipe(__dev_priv, __p) \ > + for_each_if((__mask) & BIT(__priolist[__p])) \ > + for_each_if(intel_crtc = intel_crtc_for_pipe(to_intel_display(&__dev_priv->drm), __priolist[__p])) I think something like: const u8 intel_pipe_order_enable[4] = { 3, 1, 2, 0, }; const u8 intel_pipe_order_disable[4] = { 0, 2, 1, 3, }; #define for_each_intel_crtc_in_pipe_mask_ordered(crtc, pipe_masks, order, i) \ for ((i) = 0; \ (i) < ARRAY_SIZE(order) && \ ((crtc) = intel_crtc_for_pipe(joiner_primary_pipe(pipe_mask) + (order)[(i)]), 1); \ (i)++) \ for_each_if((crtc) && (pipe_mask) & BIT((crtc)->pipe)) would let us avoid that hardcoded pipe stuff, and everything is just based on the relative order between the pipes. The same orders also work for bigjoiner and non-joined cases (it just skips the pipes that are't in the mask). The alternative would be to just use the bigjoiner primary+secondary masks and come up with a a way to iterate two bitmask in either forward or reverse order. Hmm, I suppose one might just combine the bigjoiner primary and secondary masks into one, with one of them shifted up to some high bits, and then iterate the combined bitmask either forward or backward. Something like this should work: #define for_each_crtc_in_masks(crtc, first_pipes, second_pipes, pipes, i) \ for ((i) = 0, (pipes) = (second_pipes) << 16 | (first_pipes); \ (i) < 32 && ((crtc) = intel_crtc_for_pipe((i) & 15), 1); \ (i)++) \ for_each_if((crtc) && (pipes) & BIT(i)) #define for_each_crtc_in_masks_reverse(crtc, first_pipes, second_pipes, pipes, i) \ for ((i) = 31, (pipes) = (first_pipes) << 16 | (second_pipes); \ (i) >= 0 && ((crtc) = intel_crtc_for_pipe((i) & 15), 1); \ (i)--) \ for_each_if((crtc) && (pipes) & BIT(i)) (could reduce the constants a bit given we don't have 16 pipes). And then we'd just use them like so: for_each_crtc_in_masks_reverse(crtc, bigjoiner_secondary_pipes(), bigjoiner_primary_pipes(), pipes, i) enable(); for_each_crtc_in_masks(crtc, bigjoiner_primary_pipes(), bigjoiner_secondary_pipes(), pipes, i) disable(); Would at least be very close to what we already have, just splitting the pipe masks into two essentially. > + > #define for_each_intel_crtc_in_pipe_mask(dev, intel_crtc, pipe_mask) \ > list_for_each_entry(intel_crtc, \ > &(dev)->mode_config.crtc_list, \ > @@ -431,6 +436,8 @@ bool intel_crtc_is_ultrajoiner(const struct intel_crtc_state *crtc_state); > bool intel_crtc_is_ultrajoiner_primary(const struct intel_crtc_state *crtc_state); > u8 intel_crtc_joiner_secondary_pipes(const struct intel_crtc_state *crtc_state); > struct intel_crtc *intel_primary_crtc(const struct intel_crtc_state *crtc_state); > +const enum pipe *intel_get_pipe_order_enable(const struct intel_crtc_state *crtc_state); > +const enum pipe *intel_get_pipe_order_disable(const struct intel_crtc_state *crtc_state); > bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state); > bool intel_pipe_config_compare(const struct intel_crtc_state *current_config, > const struct intel_crtc_state *pipe_config, > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index be79783ce09b..1c87f81568c8 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -1003,7 +1003,7 @@ static void intel_mst_post_disable_dp(struct intel_atomic_state *state, > struct drm_dp_mst_atomic_payload *new_payload = > drm_atomic_get_mst_payload_state(new_mst_state, connector->port); > struct drm_i915_private *dev_priv = to_i915(connector->base.dev); > - struct intel_crtc *pipe_crtc; > + struct intel_crtc *pipe_crtc; enum pipe pipe; > bool last_mst_stream; > > intel_dp->active_mst_links--; > @@ -1012,8 +1012,9 @@ static void intel_mst_post_disable_dp(struct intel_atomic_state *state, > DISPLAY_VER(dev_priv) >= 12 && last_mst_stream && > !intel_dp_mst_is_master_trans(old_crtc_state)); > > - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, > - intel_crtc_joined_pipe_mask(old_crtc_state)) { > + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, > + intel_crtc_joined_pipe_mask(old_crtc_state), > + intel_get_pipe_order_disable(old_crtc_state)) { > const struct intel_crtc_state *old_pipe_crtc_state = > intel_atomic_get_old_crtc_state(state, pipe_crtc); > > @@ -1037,8 +1038,9 @@ static void intel_mst_post_disable_dp(struct intel_atomic_state *state, > > intel_ddi_disable_transcoder_func(old_crtc_state); > > - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, > - intel_crtc_joined_pipe_mask(old_crtc_state)) { > + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, > + intel_crtc_joined_pipe_mask(old_crtc_state), > + intel_get_pipe_order_disable(old_crtc_state)) { > const struct intel_crtc_state *old_pipe_crtc_state = > intel_atomic_get_old_crtc_state(state, pipe_crtc); > > @@ -1257,6 +1259,7 @@ static void intel_mst_enable_dp(struct intel_atomic_state *state, > enum transcoder trans = pipe_config->cpu_transcoder; > bool first_mst_stream = intel_dp->active_mst_links == 1; > struct intel_crtc *pipe_crtc; > + enum pipe pipe; > int ret; > > drm_WARN_ON(&dev_priv->drm, pipe_config->has_pch_encoder); > @@ -1304,8 +1307,9 @@ static void intel_mst_enable_dp(struct intel_atomic_state *state, > > intel_enable_transcoder(pipe_config); > > - for_each_intel_crtc_in_pipe_mask_reverse(&dev_priv->drm, pipe_crtc, > - intel_crtc_joined_pipe_mask(pipe_config)) { > + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, > + intel_crtc_joined_pipe_mask(pipe_config), > + intel_get_pipe_order_enable(pipe_config)) { > const struct intel_crtc_state *pipe_crtc_state = > intel_atomic_get_new_crtc_state(state, pipe_crtc); > > -- > 2.45.2
On 9/12/2024 4:08 AM, Ville Syrjälä wrote: > On Wed, Sep 11, 2024 at 06:43:46PM +0530, Ankit Nautiyal wrote: >> From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> >> >> Ultrajoiner case requires special treatment where both reverse and >> staight order iteration doesn't work(for instance disabling case requires >> order to be: primary master, slaves, secondary master). >> >> Lets unify our approach by using not only pipe masks for iterating required >> pipes based on joiner type used, but also using different "priority" arrays >> for each of those. >> >> v2: Fix checkpatch warnings. (Ankit) >> >> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_ddi.c | 19 +++-- >> drivers/gpu/drm/i915/display/intel_display.c | 83 ++++++++++++++++---- >> drivers/gpu/drm/i915/display/intel_display.h | 7 ++ >> drivers/gpu/drm/i915/display/intel_dp_mst.c | 18 +++-- >> 4 files changed, 96 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c >> index 00fbe9f8c03a..2c064b6c6d01 100644 >> --- a/drivers/gpu/drm/i915/display/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c >> @@ -3116,10 +3116,11 @@ static void intel_ddi_post_disable_hdmi_or_sst(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 *pipe_crtc; >> + struct intel_crtc *pipe_crtc; enum pipe pipe; >> >> - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, >> - intel_crtc_joined_pipe_mask(old_crtc_state)) { >> + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, >> + intel_crtc_joined_pipe_mask(old_crtc_state), >> + intel_get_pipe_order_disable(old_crtc_state)) { >> const struct intel_crtc_state *old_pipe_crtc_state = >> intel_atomic_get_old_crtc_state(state, pipe_crtc); >> >> @@ -3130,8 +3131,9 @@ static void intel_ddi_post_disable_hdmi_or_sst(struct intel_atomic_state *state, >> >> intel_ddi_disable_transcoder_func(old_crtc_state); >> >> - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, >> - intel_crtc_joined_pipe_mask(old_crtc_state)) { >> + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, >> + intel_crtc_joined_pipe_mask(old_crtc_state), >> + intel_get_pipe_order_disable(old_crtc_state)) { >> const struct intel_crtc_state *old_pipe_crtc_state = >> intel_atomic_get_old_crtc_state(state, pipe_crtc); >> >> @@ -3383,7 +3385,7 @@ static void intel_enable_ddi(struct intel_atomic_state *state, >> const struct drm_connector_state *conn_state) >> { >> struct drm_i915_private *i915 = to_i915(encoder->base.dev); >> - struct intel_crtc *pipe_crtc; >> + struct intel_crtc *pipe_crtc; enum pipe pipe; >> >> intel_ddi_enable_transcoder_func(encoder, crtc_state); >> >> @@ -3394,8 +3396,9 @@ static void intel_enable_ddi(struct intel_atomic_state *state, >> >> intel_ddi_wait_for_fec_status(encoder, crtc_state, true); >> >> - for_each_intel_crtc_in_pipe_mask_reverse(&i915->drm, pipe_crtc, >> - intel_crtc_joined_pipe_mask(crtc_state)) { >> + for_each_intel_crtc_in_mask_priority(i915, pipe_crtc, pipe, >> + intel_crtc_joined_pipe_mask(crtc_state), >> + intel_get_pipe_order_enable(crtc_state)) { >> const struct intel_crtc_state *pipe_crtc_state = >> intel_atomic_get_new_crtc_state(state, pipe_crtc); >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> index db27850b2c36..27622d51a473 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -1737,6 +1737,50 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta >> hsw_set_transconf(crtc_state); >> } >> >> +static >> +bool intel_crtc_is_bigjoiner(const struct intel_crtc_state *pipe_config) >> +{ >> + return hweight8(pipe_config->joiner_pipes) == 2; >> +} >> + >> +const enum pipe *intel_get_pipe_order_enable(const struct intel_crtc_state *crtc_state) >> +{ >> + static const enum pipe ultrajoiner_pipe_order_enable[I915_MAX_PIPES] = { >> + PIPE_B, PIPE_D, PIPE_C, PIPE_A >> + }; >> + static const enum pipe bigjoiner_pipe_order_enable[I915_MAX_PIPES] = { >> + PIPE_B, PIPE_A, PIPE_D, PIPE_C >> + }; >> + static const enum pipe nojoiner_pipe_order_enable[I915_MAX_PIPES] = { >> + PIPE_A, PIPE_B, PIPE_C, PIPE_D >> + }; >> + >> + if (intel_crtc_is_ultrajoiner(crtc_state)) >> + return ultrajoiner_pipe_order_enable; >> + else if (intel_crtc_is_bigjoiner(crtc_state)) >> + return bigjoiner_pipe_order_enable; >> + return nojoiner_pipe_order_enable; >> +} >> + >> +const enum pipe *intel_get_pipe_order_disable(const struct intel_crtc_state *crtc_state) >> +{ >> + static const enum pipe ultrajoiner_pipe_order_disable[I915_MAX_PIPES] = { >> + PIPE_A, PIPE_B, PIPE_D, PIPE_C >> + }; >> + static const enum pipe bigjoiner_pipe_order_disable[I915_MAX_PIPES] = { >> + PIPE_A, PIPE_B, PIPE_C, PIPE_D >> + }; >> + static const enum pipe nojoiner_pipe_order_disable[I915_MAX_PIPES] = { >> + PIPE_A, PIPE_B, PIPE_C, PIPE_D >> + }; >> + >> + if (intel_crtc_is_ultrajoiner(crtc_state)) >> + return ultrajoiner_pipe_order_disable; >> + else if (intel_crtc_is_bigjoiner(crtc_state)) >> + return bigjoiner_pipe_order_disable; >> + return nojoiner_pipe_order_disable; > I don't think we should need all those diffrent order array. Technically > one should do. Though having two might make sense. > > Another problem is the hardcoded pipes. If we eg. get hardware that > would support ultrajoiner on pipes B-E in the future this would no > longer work. > >> +} > <snip> >> +#define for_each_intel_crtc_in_mask_priority(__dev_priv, intel_crtc, __p, __mask, __priolist) \ >> + for_each_pipe(__dev_priv, __p) \ >> + for_each_if((__mask) & BIT(__priolist[__p])) \ >> + for_each_if(intel_crtc = intel_crtc_for_pipe(to_intel_display(&__dev_priv->drm), __priolist[__p])) > > I think something like: > > const u8 intel_pipe_order_enable[4] = { > 3, 1, 2, 0, > }; > > const u8 intel_pipe_order_disable[4] = { > 0, 2, 1, 3, > }; > > #define for_each_intel_crtc_in_pipe_mask_ordered(crtc, pipe_masks, order, i) \ > for ((i) = 0; \ > (i) < ARRAY_SIZE(order) && \ > ((crtc) = intel_crtc_for_pipe(joiner_primary_pipe(pipe_mask) + (order)[(i)]), 1); \ > (i)++) \ > for_each_if((crtc) && (pipe_mask) & BIT((crtc)->pipe)) > > would let us avoid that hardcoded pipe stuff, and everything is > just based on the relative order between the pipes. The same orders > also work for bigjoiner and non-joined cases (it just skips the pipes > that are't in the mask). > > > The alternative would be to just use the bigjoiner primary+secondary masks > and come up with a a way to iterate two bitmask in either forward or reverse > order. Hmm, I suppose one might just combine the bigjoiner primary and > secondary masks into one, with one of them shifted up to some high bits, > and then iterate the combined bitmask either forward or backward. > > Something like this should work: > #define for_each_crtc_in_masks(crtc, first_pipes, second_pipes, pipes, i) \ > for ((i) = 0, (pipes) = (second_pipes) << 16 | (first_pipes); \ > (i) < 32 && ((crtc) = intel_crtc_for_pipe((i) & 15), 1); \ > (i)++) \ > for_each_if((crtc) && (pipes) & BIT(i)) > > #define for_each_crtc_in_masks_reverse(crtc, first_pipes, second_pipes, pipes, i) \ > for ((i) = 31, (pipes) = (first_pipes) << 16 | (second_pipes); \ > (i) >= 0 && ((crtc) = intel_crtc_for_pipe((i) & 15), 1); \ > (i)--) \ > for_each_if((crtc) && (pipes) & BIT(i)) > > (could reduce the constants a bit given we don't have 16 pipes). This looks good to me. changed for 4 pipes, as below: #define for_each_crtc_in_masks(crtc, first_pipes, second_pipes, pipes, i) \ for ((i) = 0, (pipes) = (first_pipes) | ((second_pipes) << 4); \ (i) < 8 && ((crtc) = intel_crtc_for_pipe((i & 3)), 1); \ (i)++) \ for_each_if((crtc) && (pipes) & BIT(i)) #define for_each_crtc_in_masks_reverse(crtc, first_pipes, second_pipes, pipes, i) \ for ((i) = 7, (pipes) = (first_pipes) | ((second_pipes) << 4); \ (i) >= 0 && ((crtc) = intel_crtc_for_pipe((i & 3)), 1); \ (i)--) \ for_each_if((crtc) && (pipes) & BIT(i)) But, for non joiner case, when the bigjoiner_primary/secondary_pipes are 0 so pipes will be 0. How to work with non joiner case? what if we do : (pipes) = ((first_pipes) | ((second_pipes) << 4)) ? ((first_pipes) | ((second_pipes) << 4)) : 0xF; \ So if pipes is computed to be 0, we use pipe:0xF Regards, Ankit > > And then we'd just use them like so: > for_each_crtc_in_masks_reverse(crtc, bigjoiner_secondary_pipes(), > bigjoiner_primary_pipes(), pipes, i) > enable(); > > for_each_crtc_in_masks(crtc, bigjoiner_primary_pipes(), > bigjoiner_secondary_pipes(), pipes, i) > disable(); > > Would at least be very close to what we already have, just splitting > the pipe masks into two essentially. > >> + >> #define for_each_intel_crtc_in_pipe_mask(dev, intel_crtc, pipe_mask) \ >> list_for_each_entry(intel_crtc, \ >> &(dev)->mode_config.crtc_list, \ >> @@ -431,6 +436,8 @@ bool intel_crtc_is_ultrajoiner(const struct intel_crtc_state *crtc_state); >> bool intel_crtc_is_ultrajoiner_primary(const struct intel_crtc_state *crtc_state); >> u8 intel_crtc_joiner_secondary_pipes(const struct intel_crtc_state *crtc_state); >> struct intel_crtc *intel_primary_crtc(const struct intel_crtc_state *crtc_state); >> +const enum pipe *intel_get_pipe_order_enable(const struct intel_crtc_state *crtc_state); >> +const enum pipe *intel_get_pipe_order_disable(const struct intel_crtc_state *crtc_state); >> bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state); >> bool intel_pipe_config_compare(const struct intel_crtc_state *current_config, >> const struct intel_crtc_state *pipe_config, >> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c >> index be79783ce09b..1c87f81568c8 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c >> @@ -1003,7 +1003,7 @@ static void intel_mst_post_disable_dp(struct intel_atomic_state *state, >> struct drm_dp_mst_atomic_payload *new_payload = >> drm_atomic_get_mst_payload_state(new_mst_state, connector->port); >> struct drm_i915_private *dev_priv = to_i915(connector->base.dev); >> - struct intel_crtc *pipe_crtc; >> + struct intel_crtc *pipe_crtc; enum pipe pipe; >> bool last_mst_stream; >> >> intel_dp->active_mst_links--; >> @@ -1012,8 +1012,9 @@ static void intel_mst_post_disable_dp(struct intel_atomic_state *state, >> DISPLAY_VER(dev_priv) >= 12 && last_mst_stream && >> !intel_dp_mst_is_master_trans(old_crtc_state)); >> >> - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, >> - intel_crtc_joined_pipe_mask(old_crtc_state)) { >> + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, >> + intel_crtc_joined_pipe_mask(old_crtc_state), >> + intel_get_pipe_order_disable(old_crtc_state)) { >> const struct intel_crtc_state *old_pipe_crtc_state = >> intel_atomic_get_old_crtc_state(state, pipe_crtc); >> >> @@ -1037,8 +1038,9 @@ static void intel_mst_post_disable_dp(struct intel_atomic_state *state, >> >> intel_ddi_disable_transcoder_func(old_crtc_state); >> >> - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, >> - intel_crtc_joined_pipe_mask(old_crtc_state)) { >> + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, >> + intel_crtc_joined_pipe_mask(old_crtc_state), >> + intel_get_pipe_order_disable(old_crtc_state)) { >> const struct intel_crtc_state *old_pipe_crtc_state = >> intel_atomic_get_old_crtc_state(state, pipe_crtc); >> >> @@ -1257,6 +1259,7 @@ static void intel_mst_enable_dp(struct intel_atomic_state *state, >> enum transcoder trans = pipe_config->cpu_transcoder; >> bool first_mst_stream = intel_dp->active_mst_links == 1; >> struct intel_crtc *pipe_crtc; >> + enum pipe pipe; >> int ret; >> >> drm_WARN_ON(&dev_priv->drm, pipe_config->has_pch_encoder); >> @@ -1304,8 +1307,9 @@ static void intel_mst_enable_dp(struct intel_atomic_state *state, >> >> intel_enable_transcoder(pipe_config); >> >> - for_each_intel_crtc_in_pipe_mask_reverse(&dev_priv->drm, pipe_crtc, >> - intel_crtc_joined_pipe_mask(pipe_config)) { >> + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, >> + intel_crtc_joined_pipe_mask(pipe_config), >> + intel_get_pipe_order_enable(pipe_config)) { >> const struct intel_crtc_state *pipe_crtc_state = >> intel_atomic_get_new_crtc_state(state, pipe_crtc); >> >> -- >> 2.45.2
On Mon, Sep 16, 2024 at 01:09:42PM +0530, Nautiyal, Ankit K wrote: > > On 9/12/2024 4:08 AM, Ville Syrjälä wrote: > > On Wed, Sep 11, 2024 at 06:43:46PM +0530, Ankit Nautiyal wrote: > >> From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > >> > >> Ultrajoiner case requires special treatment where both reverse and > >> staight order iteration doesn't work(for instance disabling case requires > >> order to be: primary master, slaves, secondary master). > >> > >> Lets unify our approach by using not only pipe masks for iterating required > >> pipes based on joiner type used, but also using different "priority" arrays > >> for each of those. > >> > >> v2: Fix checkpatch warnings. (Ankit) > >> > >> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > >> --- > >> drivers/gpu/drm/i915/display/intel_ddi.c | 19 +++-- > >> drivers/gpu/drm/i915/display/intel_display.c | 83 ++++++++++++++++---- > >> drivers/gpu/drm/i915/display/intel_display.h | 7 ++ > >> drivers/gpu/drm/i915/display/intel_dp_mst.c | 18 +++-- > >> 4 files changed, 96 insertions(+), 31 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > >> index 00fbe9f8c03a..2c064b6c6d01 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_ddi.c > >> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > >> @@ -3116,10 +3116,11 @@ static void intel_ddi_post_disable_hdmi_or_sst(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 *pipe_crtc; > >> + struct intel_crtc *pipe_crtc; enum pipe pipe; > >> > >> - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, > >> - intel_crtc_joined_pipe_mask(old_crtc_state)) { > >> + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, > >> + intel_crtc_joined_pipe_mask(old_crtc_state), > >> + intel_get_pipe_order_disable(old_crtc_state)) { > >> const struct intel_crtc_state *old_pipe_crtc_state = > >> intel_atomic_get_old_crtc_state(state, pipe_crtc); > >> > >> @@ -3130,8 +3131,9 @@ static void intel_ddi_post_disable_hdmi_or_sst(struct intel_atomic_state *state, > >> > >> intel_ddi_disable_transcoder_func(old_crtc_state); > >> > >> - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, > >> - intel_crtc_joined_pipe_mask(old_crtc_state)) { > >> + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, > >> + intel_crtc_joined_pipe_mask(old_crtc_state), > >> + intel_get_pipe_order_disable(old_crtc_state)) { > >> const struct intel_crtc_state *old_pipe_crtc_state = > >> intel_atomic_get_old_crtc_state(state, pipe_crtc); > >> > >> @@ -3383,7 +3385,7 @@ static void intel_enable_ddi(struct intel_atomic_state *state, > >> const struct drm_connector_state *conn_state) > >> { > >> struct drm_i915_private *i915 = to_i915(encoder->base.dev); > >> - struct intel_crtc *pipe_crtc; > >> + struct intel_crtc *pipe_crtc; enum pipe pipe; > >> > >> intel_ddi_enable_transcoder_func(encoder, crtc_state); > >> > >> @@ -3394,8 +3396,9 @@ static void intel_enable_ddi(struct intel_atomic_state *state, > >> > >> intel_ddi_wait_for_fec_status(encoder, crtc_state, true); > >> > >> - for_each_intel_crtc_in_pipe_mask_reverse(&i915->drm, pipe_crtc, > >> - intel_crtc_joined_pipe_mask(crtc_state)) { > >> + for_each_intel_crtc_in_mask_priority(i915, pipe_crtc, pipe, > >> + intel_crtc_joined_pipe_mask(crtc_state), > >> + intel_get_pipe_order_enable(crtc_state)) { > >> const struct intel_crtc_state *pipe_crtc_state = > >> intel_atomic_get_new_crtc_state(state, pipe_crtc); > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > >> index db27850b2c36..27622d51a473 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_display.c > >> +++ b/drivers/gpu/drm/i915/display/intel_display.c > >> @@ -1737,6 +1737,50 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta > >> hsw_set_transconf(crtc_state); > >> } > >> > >> +static > >> +bool intel_crtc_is_bigjoiner(const struct intel_crtc_state *pipe_config) > >> +{ > >> + return hweight8(pipe_config->joiner_pipes) == 2; > >> +} > >> + > >> +const enum pipe *intel_get_pipe_order_enable(const struct intel_crtc_state *crtc_state) > >> +{ > >> + static const enum pipe ultrajoiner_pipe_order_enable[I915_MAX_PIPES] = { > >> + PIPE_B, PIPE_D, PIPE_C, PIPE_A > >> + }; > >> + static const enum pipe bigjoiner_pipe_order_enable[I915_MAX_PIPES] = { > >> + PIPE_B, PIPE_A, PIPE_D, PIPE_C > >> + }; > >> + static const enum pipe nojoiner_pipe_order_enable[I915_MAX_PIPES] = { > >> + PIPE_A, PIPE_B, PIPE_C, PIPE_D > >> + }; > >> + > >> + if (intel_crtc_is_ultrajoiner(crtc_state)) > >> + return ultrajoiner_pipe_order_enable; > >> + else if (intel_crtc_is_bigjoiner(crtc_state)) > >> + return bigjoiner_pipe_order_enable; > >> + return nojoiner_pipe_order_enable; > >> +} > >> + > >> +const enum pipe *intel_get_pipe_order_disable(const struct intel_crtc_state *crtc_state) > >> +{ > >> + static const enum pipe ultrajoiner_pipe_order_disable[I915_MAX_PIPES] = { > >> + PIPE_A, PIPE_B, PIPE_D, PIPE_C > >> + }; > >> + static const enum pipe bigjoiner_pipe_order_disable[I915_MAX_PIPES] = { > >> + PIPE_A, PIPE_B, PIPE_C, PIPE_D > >> + }; > >> + static const enum pipe nojoiner_pipe_order_disable[I915_MAX_PIPES] = { > >> + PIPE_A, PIPE_B, PIPE_C, PIPE_D > >> + }; > >> + > >> + if (intel_crtc_is_ultrajoiner(crtc_state)) > >> + return ultrajoiner_pipe_order_disable; > >> + else if (intel_crtc_is_bigjoiner(crtc_state)) > >> + return bigjoiner_pipe_order_disable; > >> + return nojoiner_pipe_order_disable; > > I don't think we should need all those diffrent order array. Technically > > one should do. Though having two might make sense. > > > > Another problem is the hardcoded pipes. If we eg. get hardware that > > would support ultrajoiner on pipes B-E in the future this would no > > longer work. > > > >> +} > > <snip> > >> +#define for_each_intel_crtc_in_mask_priority(__dev_priv, intel_crtc, __p, __mask, __priolist) \ > >> + for_each_pipe(__dev_priv, __p) \ > >> + for_each_if((__mask) & BIT(__priolist[__p])) \ > >> + for_each_if(intel_crtc = intel_crtc_for_pipe(to_intel_display(&__dev_priv->drm), __priolist[__p])) > > > > I think something like: > > > > const u8 intel_pipe_order_enable[4] = { > > 3, 1, 2, 0, > > }; > > > > const u8 intel_pipe_order_disable[4] = { > > 0, 2, 1, 3, > > }; > > > > #define for_each_intel_crtc_in_pipe_mask_ordered(crtc, pipe_masks, order, i) \ > > for ((i) = 0; \ > > (i) < ARRAY_SIZE(order) && \ > > ((crtc) = intel_crtc_for_pipe(joiner_primary_pipe(pipe_mask) + (order)[(i)]), 1); \ > > (i)++) \ > > for_each_if((crtc) && (pipe_mask) & BIT((crtc)->pipe)) > > > > would let us avoid that hardcoded pipe stuff, and everything is > > just based on the relative order between the pipes. The same orders > > also work for bigjoiner and non-joined cases (it just skips the pipes > > that are't in the mask). > > > > > > The alternative would be to just use the bigjoiner primary+secondary masks > > and come up with a a way to iterate two bitmask in either forward or reverse > > order. Hmm, I suppose one might just combine the bigjoiner primary and > > secondary masks into one, with one of them shifted up to some high bits, > > and then iterate the combined bitmask either forward or backward. > > > > Something like this should work: > > #define for_each_crtc_in_masks(crtc, first_pipes, second_pipes, pipes, i) \ > > for ((i) = 0, (pipes) = (second_pipes) << 16 | (first_pipes); \ > > (i) < 32 && ((crtc) = intel_crtc_for_pipe((i) & 15), 1); \ > > (i)++) \ > > for_each_if((crtc) && (pipes) & BIT(i)) > > > > #define for_each_crtc_in_masks_reverse(crtc, first_pipes, second_pipes, pipes, i) \ > > for ((i) = 31, (pipes) = (first_pipes) << 16 | (second_pipes); \ > > (i) >= 0 && ((crtc) = intel_crtc_for_pipe((i) & 15), 1); \ > > (i)--) \ > > for_each_if((crtc) && (pipes) & BIT(i)) > > > > (could reduce the constants a bit given we don't have 16 pipes). > > This looks good to me. changed for 4 pipes, as below: > > > #define for_each_crtc_in_masks(crtc, first_pipes, second_pipes, pipes, i) \ > for ((i) = 0, (pipes) = (first_pipes) | ((second_pipes) << 4); \ > (i) < 8 && ((crtc) = intel_crtc_for_pipe((i & 3)), 1); \ We could probably use a single internal define for the magic number to avoid things going out of sync by accident. Hmm, maybe even define it as something like #define _INTEL_MAX_PIPES_POT roundup_power_of_two(I915_MAX_PIPES) ? O, I suppose we don't really need it to be POT, so we could just replace the '&' with '%', and then we can just use I915_MAX_PIPES directly. > (i)++) \ > for_each_if((crtc) && (pipes) & BIT(i)) > > #define for_each_crtc_in_masks_reverse(crtc, first_pipes, second_pipes, > pipes, i) \ > for ((i) = 7, (pipes) = (first_pipes) | ((second_pipes) << 4); \ > (i) >= 0 && ((crtc) = intel_crtc_for_pipe((i & 3)), 1); \ > (i)--) \ > for_each_if((crtc) && (pipes) & BIT(i)) > > But, for non joiner case, when the bigjoiner_primary/secondary_pipes are > 0 so pipes will be 0. Hmm. I think we just need to make bigjoiner_primary_pipes() return BIT(crtc->pipe) for the non-joiner cases. Maybe we should rename these to something like _modeset_{primary,secondary}_pipes() so that people don't get tempted to use them for anything else? And then we could hide all this into something like #define for_each_pipe_crtc_modeset_disable(...) \ for_each_crtc_in_masks(..., _modeset_primary_pipes(), \ _modeset_secondary_pipes(), ...) #define for_each_pipe_crtc_modeset_enable(...) \ for_each_crtc_in_masks_reverse(..., _modeset_secondary_pipes(), \ _modeset_primary_pipes(), ...)
On Mon, Sep 16, 2024 at 05:54:12PM +0300, Ville Syrjälä wrote: > On Mon, Sep 16, 2024 at 01:09:42PM +0530, Nautiyal, Ankit K wrote: > > > > On 9/12/2024 4:08 AM, Ville Syrjälä wrote: > > > On Wed, Sep 11, 2024 at 06:43:46PM +0530, Ankit Nautiyal wrote: > > >> From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > >> > > >> Ultrajoiner case requires special treatment where both reverse and > > >> staight order iteration doesn't work(for instance disabling case requires > > >> order to be: primary master, slaves, secondary master). > > >> > > >> Lets unify our approach by using not only pipe masks for iterating required > > >> pipes based on joiner type used, but also using different "priority" arrays > > >> for each of those. > > >> > > >> v2: Fix checkpatch warnings. (Ankit) > > >> > > >> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > >> --- > > >> drivers/gpu/drm/i915/display/intel_ddi.c | 19 +++-- > > >> drivers/gpu/drm/i915/display/intel_display.c | 83 ++++++++++++++++---- > > >> drivers/gpu/drm/i915/display/intel_display.h | 7 ++ > > >> drivers/gpu/drm/i915/display/intel_dp_mst.c | 18 +++-- > > >> 4 files changed, 96 insertions(+), 31 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > >> index 00fbe9f8c03a..2c064b6c6d01 100644 > > >> --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > >> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > >> @@ -3116,10 +3116,11 @@ static void intel_ddi_post_disable_hdmi_or_sst(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 *pipe_crtc; > > >> + struct intel_crtc *pipe_crtc; enum pipe pipe; > > >> > > >> - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, > > >> - intel_crtc_joined_pipe_mask(old_crtc_state)) { > > >> + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, > > >> + intel_crtc_joined_pipe_mask(old_crtc_state), > > >> + intel_get_pipe_order_disable(old_crtc_state)) { > > >> const struct intel_crtc_state *old_pipe_crtc_state = > > >> intel_atomic_get_old_crtc_state(state, pipe_crtc); > > >> > > >> @@ -3130,8 +3131,9 @@ static void intel_ddi_post_disable_hdmi_or_sst(struct intel_atomic_state *state, > > >> > > >> intel_ddi_disable_transcoder_func(old_crtc_state); > > >> > > >> - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, > > >> - intel_crtc_joined_pipe_mask(old_crtc_state)) { > > >> + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, > > >> + intel_crtc_joined_pipe_mask(old_crtc_state), > > >> + intel_get_pipe_order_disable(old_crtc_state)) { > > >> const struct intel_crtc_state *old_pipe_crtc_state = > > >> intel_atomic_get_old_crtc_state(state, pipe_crtc); > > >> > > >> @@ -3383,7 +3385,7 @@ static void intel_enable_ddi(struct intel_atomic_state *state, > > >> const struct drm_connector_state *conn_state) > > >> { > > >> struct drm_i915_private *i915 = to_i915(encoder->base.dev); > > >> - struct intel_crtc *pipe_crtc; > > >> + struct intel_crtc *pipe_crtc; enum pipe pipe; > > >> > > >> intel_ddi_enable_transcoder_func(encoder, crtc_state); > > >> > > >> @@ -3394,8 +3396,9 @@ static void intel_enable_ddi(struct intel_atomic_state *state, > > >> > > >> intel_ddi_wait_for_fec_status(encoder, crtc_state, true); > > >> > > >> - for_each_intel_crtc_in_pipe_mask_reverse(&i915->drm, pipe_crtc, > > >> - intel_crtc_joined_pipe_mask(crtc_state)) { > > >> + for_each_intel_crtc_in_mask_priority(i915, pipe_crtc, pipe, > > >> + intel_crtc_joined_pipe_mask(crtc_state), > > >> + intel_get_pipe_order_enable(crtc_state)) { > > >> const struct intel_crtc_state *pipe_crtc_state = > > >> intel_atomic_get_new_crtc_state(state, pipe_crtc); > > >> > > >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > >> index db27850b2c36..27622d51a473 100644 > > >> --- a/drivers/gpu/drm/i915/display/intel_display.c > > >> +++ b/drivers/gpu/drm/i915/display/intel_display.c > > >> @@ -1737,6 +1737,50 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta > > >> hsw_set_transconf(crtc_state); > > >> } > > >> > > >> +static > > >> +bool intel_crtc_is_bigjoiner(const struct intel_crtc_state *pipe_config) > > >> +{ > > >> + return hweight8(pipe_config->joiner_pipes) == 2; > > >> +} > > >> + > > >> +const enum pipe *intel_get_pipe_order_enable(const struct intel_crtc_state *crtc_state) > > >> +{ > > >> + static const enum pipe ultrajoiner_pipe_order_enable[I915_MAX_PIPES] = { > > >> + PIPE_B, PIPE_D, PIPE_C, PIPE_A > > >> + }; > > >> + static const enum pipe bigjoiner_pipe_order_enable[I915_MAX_PIPES] = { > > >> + PIPE_B, PIPE_A, PIPE_D, PIPE_C > > >> + }; > > >> + static const enum pipe nojoiner_pipe_order_enable[I915_MAX_PIPES] = { > > >> + PIPE_A, PIPE_B, PIPE_C, PIPE_D > > >> + }; > > >> + > > >> + if (intel_crtc_is_ultrajoiner(crtc_state)) > > >> + return ultrajoiner_pipe_order_enable; > > >> + else if (intel_crtc_is_bigjoiner(crtc_state)) > > >> + return bigjoiner_pipe_order_enable; > > >> + return nojoiner_pipe_order_enable; > > >> +} > > >> + > > >> +const enum pipe *intel_get_pipe_order_disable(const struct intel_crtc_state *crtc_state) > > >> +{ > > >> + static const enum pipe ultrajoiner_pipe_order_disable[I915_MAX_PIPES] = { > > >> + PIPE_A, PIPE_B, PIPE_D, PIPE_C > > >> + }; > > >> + static const enum pipe bigjoiner_pipe_order_disable[I915_MAX_PIPES] = { > > >> + PIPE_A, PIPE_B, PIPE_C, PIPE_D > > >> + }; > > >> + static const enum pipe nojoiner_pipe_order_disable[I915_MAX_PIPES] = { > > >> + PIPE_A, PIPE_B, PIPE_C, PIPE_D > > >> + }; > > >> + > > >> + if (intel_crtc_is_ultrajoiner(crtc_state)) > > >> + return ultrajoiner_pipe_order_disable; > > >> + else if (intel_crtc_is_bigjoiner(crtc_state)) > > >> + return bigjoiner_pipe_order_disable; > > >> + return nojoiner_pipe_order_disable; > > > I don't think we should need all those diffrent order array. Technically > > > one should do. Though having two might make sense. > > > > > > Another problem is the hardcoded pipes. If we eg. get hardware that > > > would support ultrajoiner on pipes B-E in the future this would no > > > longer work. > > > > > >> +} > > > <snip> > > >> +#define for_each_intel_crtc_in_mask_priority(__dev_priv, intel_crtc, __p, __mask, __priolist) \ > > >> + for_each_pipe(__dev_priv, __p) \ > > >> + for_each_if((__mask) & BIT(__priolist[__p])) \ > > >> + for_each_if(intel_crtc = intel_crtc_for_pipe(to_intel_display(&__dev_priv->drm), __priolist[__p])) > > > > > > I think something like: > > > > > > const u8 intel_pipe_order_enable[4] = { > > > 3, 1, 2, 0, > > > }; > > > > > > const u8 intel_pipe_order_disable[4] = { > > > 0, 2, 1, 3, > > > }; > > > > > > #define for_each_intel_crtc_in_pipe_mask_ordered(crtc, pipe_masks, order, i) \ > > > for ((i) = 0; \ > > > (i) < ARRAY_SIZE(order) && \ > > > ((crtc) = intel_crtc_for_pipe(joiner_primary_pipe(pipe_mask) + (order)[(i)]), 1); \ > > > (i)++) \ > > > for_each_if((crtc) && (pipe_mask) & BIT((crtc)->pipe)) > > > > > > would let us avoid that hardcoded pipe stuff, and everything is > > > just based on the relative order between the pipes. The same orders > > > also work for bigjoiner and non-joined cases (it just skips the pipes > > > that are't in the mask). > > > > > > > > > The alternative would be to just use the bigjoiner primary+secondary masks > > > and come up with a a way to iterate two bitmask in either forward or reverse > > > order. Hmm, I suppose one might just combine the bigjoiner primary and > > > secondary masks into one, with one of them shifted up to some high bits, > > > and then iterate the combined bitmask either forward or backward. > > > > > > Something like this should work: > > > #define for_each_crtc_in_masks(crtc, first_pipes, second_pipes, pipes, i) \ > > > for ((i) = 0, (pipes) = (second_pipes) << 16 | (first_pipes); \ > > > (i) < 32 && ((crtc) = intel_crtc_for_pipe((i) & 15), 1); \ > > > (i)++) \ > > > for_each_if((crtc) && (pipes) & BIT(i)) > > > > > > #define for_each_crtc_in_masks_reverse(crtc, first_pipes, second_pipes, pipes, i) \ > > > for ((i) = 31, (pipes) = (first_pipes) << 16 | (second_pipes); \ > > > (i) >= 0 && ((crtc) = intel_crtc_for_pipe((i) & 15), 1); \ > > > (i)--) \ > > > for_each_if((crtc) && (pipes) & BIT(i)) > > > > > > (could reduce the constants a bit given we don't have 16 pipes). > > > > This looks good to me. changed for 4 pipes, as below: > > > > > > #define for_each_crtc_in_masks(crtc, first_pipes, second_pipes, pipes, i) \ > > for ((i) = 0, (pipes) = (first_pipes) | ((second_pipes) << 4); \ > > (i) < 8 && ((crtc) = intel_crtc_for_pipe((i & 3)), 1); \ > > We could probably use a single internal define for the magic > number to avoid things going out of sync by accident. > > Hmm, maybe even define it as something like > #define _INTEL_MAX_PIPES_POT roundup_power_of_two(I915_MAX_PIPES) > ? > > O, I suppose we don't really need it to be POT, so we could > just replace the '&' with '%', and then we can just use > I915_MAX_PIPES directly. > > > (i)++) \ > > for_each_if((crtc) && (pipes) & BIT(i)) > > > > #define for_each_crtc_in_masks_reverse(crtc, first_pipes, second_pipes, > > pipes, i) \ > > for ((i) = 7, (pipes) = (first_pipes) | ((second_pipes) << 4); \ > > (i) >= 0 && ((crtc) = intel_crtc_for_pipe((i & 3)), 1); \ > > (i)--) \ > > for_each_if((crtc) && (pipes) & BIT(i)) > > > > But, for non joiner case, when the bigjoiner_primary/secondary_pipes are > > 0 so pipes will be 0. > > Hmm. I think we just need to make bigjoiner_primary_pipes() > return BIT(crtc->pipe) for the non-joiner cases. > > Maybe we should rename these to something like > _modeset_{primary,secondary}_pipes() so that people > don't get tempted to use them for anything else? > > And then we could hide all this into something like > #define for_each_pipe_crtc_modeset_disable(...) \ > for_each_crtc_in_masks(..., _modeset_primary_pipes(), \ > _modeset_secondary_pipes(), ...) > #define for_each_pipe_crtc_modeset_enable(...) \ > for_each_crtc_in_masks_reverse(..., _modeset_secondary_pipes(), \ > _modeset_primary_pipes(), ...) These last two macros you could already implement right now using the current code, and then we can replace them with the ultrajoiner capable stuff in another patch and not touch any of the actual modeset code anymore.
On 9/16/2024 8:36 PM, Ville Syrjälä wrote: > On Mon, Sep 16, 2024 at 05:54:12PM +0300, Ville Syrjälä wrote: >> On Mon, Sep 16, 2024 at 01:09:42PM +0530, Nautiyal, Ankit K wrote: >>> On 9/12/2024 4:08 AM, Ville Syrjälä wrote: >>>> On Wed, Sep 11, 2024 at 06:43:46PM +0530, Ankit Nautiyal wrote: >>>>> From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> >>>>> >>>>> Ultrajoiner case requires special treatment where both reverse and >>>>> staight order iteration doesn't work(for instance disabling case requires >>>>> order to be: primary master, slaves, secondary master). >>>>> >>>>> Lets unify our approach by using not only pipe masks for iterating required >>>>> pipes based on joiner type used, but also using different "priority" arrays >>>>> for each of those. >>>>> >>>>> v2: Fix checkpatch warnings. (Ankit) >>>>> >>>>> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> >>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/display/intel_ddi.c | 19 +++-- >>>>> drivers/gpu/drm/i915/display/intel_display.c | 83 ++++++++++++++++---- >>>>> drivers/gpu/drm/i915/display/intel_display.h | 7 ++ >>>>> drivers/gpu/drm/i915/display/intel_dp_mst.c | 18 +++-- >>>>> 4 files changed, 96 insertions(+), 31 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c >>>>> index 00fbe9f8c03a..2c064b6c6d01 100644 >>>>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c >>>>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c >>>>> @@ -3116,10 +3116,11 @@ static void intel_ddi_post_disable_hdmi_or_sst(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 *pipe_crtc; >>>>> + struct intel_crtc *pipe_crtc; enum pipe pipe; >>>>> >>>>> - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, >>>>> - intel_crtc_joined_pipe_mask(old_crtc_state)) { >>>>> + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, >>>>> + intel_crtc_joined_pipe_mask(old_crtc_state), >>>>> + intel_get_pipe_order_disable(old_crtc_state)) { >>>>> const struct intel_crtc_state *old_pipe_crtc_state = >>>>> intel_atomic_get_old_crtc_state(state, pipe_crtc); >>>>> >>>>> @@ -3130,8 +3131,9 @@ static void intel_ddi_post_disable_hdmi_or_sst(struct intel_atomic_state *state, >>>>> >>>>> intel_ddi_disable_transcoder_func(old_crtc_state); >>>>> >>>>> - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, >>>>> - intel_crtc_joined_pipe_mask(old_crtc_state)) { >>>>> + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, >>>>> + intel_crtc_joined_pipe_mask(old_crtc_state), >>>>> + intel_get_pipe_order_disable(old_crtc_state)) { >>>>> const struct intel_crtc_state *old_pipe_crtc_state = >>>>> intel_atomic_get_old_crtc_state(state, pipe_crtc); >>>>> >>>>> @@ -3383,7 +3385,7 @@ static void intel_enable_ddi(struct intel_atomic_state *state, >>>>> const struct drm_connector_state *conn_state) >>>>> { >>>>> struct drm_i915_private *i915 = to_i915(encoder->base.dev); >>>>> - struct intel_crtc *pipe_crtc; >>>>> + struct intel_crtc *pipe_crtc; enum pipe pipe; >>>>> >>>>> intel_ddi_enable_transcoder_func(encoder, crtc_state); >>>>> >>>>> @@ -3394,8 +3396,9 @@ static void intel_enable_ddi(struct intel_atomic_state *state, >>>>> >>>>> intel_ddi_wait_for_fec_status(encoder, crtc_state, true); >>>>> >>>>> - for_each_intel_crtc_in_pipe_mask_reverse(&i915->drm, pipe_crtc, >>>>> - intel_crtc_joined_pipe_mask(crtc_state)) { >>>>> + for_each_intel_crtc_in_mask_priority(i915, pipe_crtc, pipe, >>>>> + intel_crtc_joined_pipe_mask(crtc_state), >>>>> + intel_get_pipe_order_enable(crtc_state)) { >>>>> const struct intel_crtc_state *pipe_crtc_state = >>>>> intel_atomic_get_new_crtc_state(state, pipe_crtc); >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >>>>> index db27850b2c36..27622d51a473 100644 >>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c >>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c >>>>> @@ -1737,6 +1737,50 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta >>>>> hsw_set_transconf(crtc_state); >>>>> } >>>>> >>>>> +static >>>>> +bool intel_crtc_is_bigjoiner(const struct intel_crtc_state *pipe_config) >>>>> +{ >>>>> + return hweight8(pipe_config->joiner_pipes) == 2; >>>>> +} >>>>> + >>>>> +const enum pipe *intel_get_pipe_order_enable(const struct intel_crtc_state *crtc_state) >>>>> +{ >>>>> + static const enum pipe ultrajoiner_pipe_order_enable[I915_MAX_PIPES] = { >>>>> + PIPE_B, PIPE_D, PIPE_C, PIPE_A >>>>> + }; >>>>> + static const enum pipe bigjoiner_pipe_order_enable[I915_MAX_PIPES] = { >>>>> + PIPE_B, PIPE_A, PIPE_D, PIPE_C >>>>> + }; >>>>> + static const enum pipe nojoiner_pipe_order_enable[I915_MAX_PIPES] = { >>>>> + PIPE_A, PIPE_B, PIPE_C, PIPE_D >>>>> + }; >>>>> + >>>>> + if (intel_crtc_is_ultrajoiner(crtc_state)) >>>>> + return ultrajoiner_pipe_order_enable; >>>>> + else if (intel_crtc_is_bigjoiner(crtc_state)) >>>>> + return bigjoiner_pipe_order_enable; >>>>> + return nojoiner_pipe_order_enable; >>>>> +} >>>>> + >>>>> +const enum pipe *intel_get_pipe_order_disable(const struct intel_crtc_state *crtc_state) >>>>> +{ >>>>> + static const enum pipe ultrajoiner_pipe_order_disable[I915_MAX_PIPES] = { >>>>> + PIPE_A, PIPE_B, PIPE_D, PIPE_C >>>>> + }; >>>>> + static const enum pipe bigjoiner_pipe_order_disable[I915_MAX_PIPES] = { >>>>> + PIPE_A, PIPE_B, PIPE_C, PIPE_D >>>>> + }; >>>>> + static const enum pipe nojoiner_pipe_order_disable[I915_MAX_PIPES] = { >>>>> + PIPE_A, PIPE_B, PIPE_C, PIPE_D >>>>> + }; >>>>> + >>>>> + if (intel_crtc_is_ultrajoiner(crtc_state)) >>>>> + return ultrajoiner_pipe_order_disable; >>>>> + else if (intel_crtc_is_bigjoiner(crtc_state)) >>>>> + return bigjoiner_pipe_order_disable; >>>>> + return nojoiner_pipe_order_disable; >>>> I don't think we should need all those diffrent order array. Technically >>>> one should do. Though having two might make sense. >>>> >>>> Another problem is the hardcoded pipes. If we eg. get hardware that >>>> would support ultrajoiner on pipes B-E in the future this would no >>>> longer work. >>>> >>>>> +} >>>> <snip> >>>>> +#define for_each_intel_crtc_in_mask_priority(__dev_priv, intel_crtc, __p, __mask, __priolist) \ >>>>> + for_each_pipe(__dev_priv, __p) \ >>>>> + for_each_if((__mask) & BIT(__priolist[__p])) \ >>>>> + for_each_if(intel_crtc = intel_crtc_for_pipe(to_intel_display(&__dev_priv->drm), __priolist[__p])) >>>> I think something like: >>>> >>>> const u8 intel_pipe_order_enable[4] = { >>>> 3, 1, 2, 0, >>>> }; >>>> >>>> const u8 intel_pipe_order_disable[4] = { >>>> 0, 2, 1, 3, >>>> }; I just realized that as per bspec:54142 sequence for ultrajoiner doesnt follow this. its 1, 3, 2, 0 for enabling and 0, 1, 3, 2 for disabling :( Regards, Ankit >>>> >>>> #define for_each_intel_crtc_in_pipe_mask_ordered(crtc, pipe_masks, order, i) \ >>>> for ((i) = 0; \ >>>> (i) < ARRAY_SIZE(order) && \ >>>> ((crtc) = intel_crtc_for_pipe(joiner_primary_pipe(pipe_mask) + (order)[(i)]), 1); \ >>>> (i)++) \ >>>> for_each_if((crtc) && (pipe_mask) & BIT((crtc)->pipe)) >>>> >>>> would let us avoid that hardcoded pipe stuff, and everything is >>>> just based on the relative order between the pipes. The same orders >>>> also work for bigjoiner and non-joined cases (it just skips the pipes >>>> that are't in the mask). >>>> >>>> >>>> The alternative would be to just use the bigjoiner primary+secondary masks >>>> and come up with a a way to iterate two bitmask in either forward or reverse >>>> order. Hmm, I suppose one might just combine the bigjoiner primary and >>>> secondary masks into one, with one of them shifted up to some high bits, >>>> and then iterate the combined bitmask either forward or backward. >>>> >>>> Something like this should work: >>>> #define for_each_crtc_in_masks(crtc, first_pipes, second_pipes, pipes, i) \ >>>> for ((i) = 0, (pipes) = (second_pipes) << 16 | (first_pipes); \ >>>> (i) < 32 && ((crtc) = intel_crtc_for_pipe((i) & 15), 1); \ >>>> (i)++) \ >>>> for_each_if((crtc) && (pipes) & BIT(i)) >>>> >>>> #define for_each_crtc_in_masks_reverse(crtc, first_pipes, second_pipes, pipes, i) \ >>>> for ((i) = 31, (pipes) = (first_pipes) << 16 | (second_pipes); \ >>>> (i) >= 0 && ((crtc) = intel_crtc_for_pipe((i) & 15), 1); \ >>>> (i)--) \ >>>> for_each_if((crtc) && (pipes) & BIT(i)) >>>> >>>> (could reduce the constants a bit given we don't have 16 pipes). >>> This looks good to me. changed for 4 pipes, as below: >>> >>> >>> #define for_each_crtc_in_masks(crtc, first_pipes, second_pipes, pipes, i) \ >>> for ((i) = 0, (pipes) = (first_pipes) | ((second_pipes) << 4); \ >>> (i) < 8 && ((crtc) = intel_crtc_for_pipe((i & 3)), 1); \ >> We could probably use a single internal define for the magic >> number to avoid things going out of sync by accident. >> >> Hmm, maybe even define it as something like >> #define _INTEL_MAX_PIPES_POT roundup_power_of_two(I915_MAX_PIPES) >> ? >> >> O, I suppose we don't really need it to be POT, so we could >> just replace the '&' with '%', and then we can just use >> I915_MAX_PIPES directly. >> >>> (i)++) \ >>> for_each_if((crtc) && (pipes) & BIT(i)) >>> >>> #define for_each_crtc_in_masks_reverse(crtc, first_pipes, second_pipes, >>> pipes, i) \ >>> for ((i) = 7, (pipes) = (first_pipes) | ((second_pipes) << 4); \ >>> (i) >= 0 && ((crtc) = intel_crtc_for_pipe((i & 3)), 1); \ >>> (i)--) \ >>> for_each_if((crtc) && (pipes) & BIT(i)) >>> >>> But, for non joiner case, when the bigjoiner_primary/secondary_pipes are >>> 0 so pipes will be 0. >> Hmm. I think we just need to make bigjoiner_primary_pipes() >> return BIT(crtc->pipe) for the non-joiner cases. >> >> Maybe we should rename these to something like >> _modeset_{primary,secondary}_pipes() so that people >> don't get tempted to use them for anything else? >> >> And then we could hide all this into something like >> #define for_each_pipe_crtc_modeset_disable(...) \ >> for_each_crtc_in_masks(..., _modeset_primary_pipes(), \ >> _modeset_secondary_pipes(), ...) >> #define for_each_pipe_crtc_modeset_enable(...) \ >> for_each_crtc_in_masks_reverse(..., _modeset_secondary_pipes(), \ >> _modeset_primary_pipes(), ...) > These last two macros you could already implement right > now using the current code, and then we can replace them > with the ultrajoiner capable stuff in another patch and > not touch any of the actual modeset code anymore. >
On Tue, Sep 17, 2024 at 02:52:10PM +0530, Nautiyal, Ankit K wrote: > > On 9/16/2024 8:36 PM, Ville Syrjälä wrote: > > On Mon, Sep 16, 2024 at 05:54:12PM +0300, Ville Syrjälä wrote: > >> On Mon, Sep 16, 2024 at 01:09:42PM +0530, Nautiyal, Ankit K wrote: > >>> On 9/12/2024 4:08 AM, Ville Syrjälä wrote: > >>>> On Wed, Sep 11, 2024 at 06:43:46PM +0530, Ankit Nautiyal wrote: > >>>>> From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > >>>>> > >>>>> Ultrajoiner case requires special treatment where both reverse and > >>>>> staight order iteration doesn't work(for instance disabling case requires > >>>>> order to be: primary master, slaves, secondary master). > >>>>> > >>>>> Lets unify our approach by using not only pipe masks for iterating required > >>>>> pipes based on joiner type used, but also using different "priority" arrays > >>>>> for each of those. > >>>>> > >>>>> v2: Fix checkpatch warnings. (Ankit) > >>>>> > >>>>> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > >>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > >>>>> --- > >>>>> drivers/gpu/drm/i915/display/intel_ddi.c | 19 +++-- > >>>>> drivers/gpu/drm/i915/display/intel_display.c | 83 ++++++++++++++++---- > >>>>> drivers/gpu/drm/i915/display/intel_display.h | 7 ++ > >>>>> drivers/gpu/drm/i915/display/intel_dp_mst.c | 18 +++-- > >>>>> 4 files changed, 96 insertions(+), 31 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > >>>>> index 00fbe9f8c03a..2c064b6c6d01 100644 > >>>>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c > >>>>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > >>>>> @@ -3116,10 +3116,11 @@ static void intel_ddi_post_disable_hdmi_or_sst(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 *pipe_crtc; > >>>>> + struct intel_crtc *pipe_crtc; enum pipe pipe; > >>>>> > >>>>> - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, > >>>>> - intel_crtc_joined_pipe_mask(old_crtc_state)) { > >>>>> + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, > >>>>> + intel_crtc_joined_pipe_mask(old_crtc_state), > >>>>> + intel_get_pipe_order_disable(old_crtc_state)) { > >>>>> const struct intel_crtc_state *old_pipe_crtc_state = > >>>>> intel_atomic_get_old_crtc_state(state, pipe_crtc); > >>>>> > >>>>> @@ -3130,8 +3131,9 @@ static void intel_ddi_post_disable_hdmi_or_sst(struct intel_atomic_state *state, > >>>>> > >>>>> intel_ddi_disable_transcoder_func(old_crtc_state); > >>>>> > >>>>> - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, > >>>>> - intel_crtc_joined_pipe_mask(old_crtc_state)) { > >>>>> + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, > >>>>> + intel_crtc_joined_pipe_mask(old_crtc_state), > >>>>> + intel_get_pipe_order_disable(old_crtc_state)) { > >>>>> const struct intel_crtc_state *old_pipe_crtc_state = > >>>>> intel_atomic_get_old_crtc_state(state, pipe_crtc); > >>>>> > >>>>> @@ -3383,7 +3385,7 @@ static void intel_enable_ddi(struct intel_atomic_state *state, > >>>>> const struct drm_connector_state *conn_state) > >>>>> { > >>>>> struct drm_i915_private *i915 = to_i915(encoder->base.dev); > >>>>> - struct intel_crtc *pipe_crtc; > >>>>> + struct intel_crtc *pipe_crtc; enum pipe pipe; > >>>>> > >>>>> intel_ddi_enable_transcoder_func(encoder, crtc_state); > >>>>> > >>>>> @@ -3394,8 +3396,9 @@ static void intel_enable_ddi(struct intel_atomic_state *state, > >>>>> > >>>>> intel_ddi_wait_for_fec_status(encoder, crtc_state, true); > >>>>> > >>>>> - for_each_intel_crtc_in_pipe_mask_reverse(&i915->drm, pipe_crtc, > >>>>> - intel_crtc_joined_pipe_mask(crtc_state)) { > >>>>> + for_each_intel_crtc_in_mask_priority(i915, pipe_crtc, pipe, > >>>>> + intel_crtc_joined_pipe_mask(crtc_state), > >>>>> + intel_get_pipe_order_enable(crtc_state)) { > >>>>> const struct intel_crtc_state *pipe_crtc_state = > >>>>> intel_atomic_get_new_crtc_state(state, pipe_crtc); > >>>>> > >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > >>>>> index db27850b2c36..27622d51a473 100644 > >>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c > >>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c > >>>>> @@ -1737,6 +1737,50 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta > >>>>> hsw_set_transconf(crtc_state); > >>>>> } > >>>>> > >>>>> +static > >>>>> +bool intel_crtc_is_bigjoiner(const struct intel_crtc_state *pipe_config) > >>>>> +{ > >>>>> + return hweight8(pipe_config->joiner_pipes) == 2; > >>>>> +} > >>>>> + > >>>>> +const enum pipe *intel_get_pipe_order_enable(const struct intel_crtc_state *crtc_state) > >>>>> +{ > >>>>> + static const enum pipe ultrajoiner_pipe_order_enable[I915_MAX_PIPES] = { > >>>>> + PIPE_B, PIPE_D, PIPE_C, PIPE_A > >>>>> + }; > >>>>> + static const enum pipe bigjoiner_pipe_order_enable[I915_MAX_PIPES] = { > >>>>> + PIPE_B, PIPE_A, PIPE_D, PIPE_C > >>>>> + }; > >>>>> + static const enum pipe nojoiner_pipe_order_enable[I915_MAX_PIPES] = { > >>>>> + PIPE_A, PIPE_B, PIPE_C, PIPE_D > >>>>> + }; > >>>>> + > >>>>> + if (intel_crtc_is_ultrajoiner(crtc_state)) > >>>>> + return ultrajoiner_pipe_order_enable; > >>>>> + else if (intel_crtc_is_bigjoiner(crtc_state)) > >>>>> + return bigjoiner_pipe_order_enable; > >>>>> + return nojoiner_pipe_order_enable; > >>>>> +} > >>>>> + > >>>>> +const enum pipe *intel_get_pipe_order_disable(const struct intel_crtc_state *crtc_state) > >>>>> +{ > >>>>> + static const enum pipe ultrajoiner_pipe_order_disable[I915_MAX_PIPES] = { > >>>>> + PIPE_A, PIPE_B, PIPE_D, PIPE_C > >>>>> + }; > >>>>> + static const enum pipe bigjoiner_pipe_order_disable[I915_MAX_PIPES] = { > >>>>> + PIPE_A, PIPE_B, PIPE_C, PIPE_D > >>>>> + }; > >>>>> + static const enum pipe nojoiner_pipe_order_disable[I915_MAX_PIPES] = { > >>>>> + PIPE_A, PIPE_B, PIPE_C, PIPE_D > >>>>> + }; > >>>>> + > >>>>> + if (intel_crtc_is_ultrajoiner(crtc_state)) > >>>>> + return ultrajoiner_pipe_order_disable; > >>>>> + else if (intel_crtc_is_bigjoiner(crtc_state)) > >>>>> + return bigjoiner_pipe_order_disable; > >>>>> + return nojoiner_pipe_order_disable; > >>>> I don't think we should need all those diffrent order array. Technically > >>>> one should do. Though having two might make sense. > >>>> > >>>> Another problem is the hardcoded pipes. If we eg. get hardware that > >>>> would support ultrajoiner on pipes B-E in the future this would no > >>>> longer work. > >>>> > >>>>> +} > >>>> <snip> > >>>>> +#define for_each_intel_crtc_in_mask_priority(__dev_priv, intel_crtc, __p, __mask, __priolist) \ > >>>>> + for_each_pipe(__dev_priv, __p) \ > >>>>> + for_each_if((__mask) & BIT(__priolist[__p])) \ > >>>>> + for_each_if(intel_crtc = intel_crtc_for_pipe(to_intel_display(&__dev_priv->drm), __priolist[__p])) > >>>> I think something like: > >>>> > >>>> const u8 intel_pipe_order_enable[4] = { > >>>> 3, 1, 2, 0, > >>>> }; > >>>> > >>>> const u8 intel_pipe_order_disable[4] = { > >>>> 0, 2, 1, 3, > >>>> }; > > > I just realized that as per bspec:54142 sequence for ultrajoiner doesnt > follow this. > > its 1, 3, 2, 0 for enabling and 0, 1, 3, 2 for disabling :( The bigjoiner secondary pipes are listed as one step in the sequence. I believe that means that there is no ordering requirement between them. As for the funny C vs. BD order in the disable sequence, bspec:68847 has it the other way around as ACBD (a more proper mirror image of the enable sequence). So either the hardware really has changed between these two and the order is thus different, or one of them is simply wrong, or the order doesn't really matter here either. I suspect we can just always follow the more sensible order from bspec:68847. Should probably ask for clarification though, and test on actual hardware to confirm ofc.
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 00fbe9f8c03a..2c064b6c6d01 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -3116,10 +3116,11 @@ static void intel_ddi_post_disable_hdmi_or_sst(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 *pipe_crtc; + struct intel_crtc *pipe_crtc; enum pipe pipe; - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, - intel_crtc_joined_pipe_mask(old_crtc_state)) { + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, + intel_crtc_joined_pipe_mask(old_crtc_state), + intel_get_pipe_order_disable(old_crtc_state)) { const struct intel_crtc_state *old_pipe_crtc_state = intel_atomic_get_old_crtc_state(state, pipe_crtc); @@ -3130,8 +3131,9 @@ static void intel_ddi_post_disable_hdmi_or_sst(struct intel_atomic_state *state, intel_ddi_disable_transcoder_func(old_crtc_state); - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, - intel_crtc_joined_pipe_mask(old_crtc_state)) { + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, + intel_crtc_joined_pipe_mask(old_crtc_state), + intel_get_pipe_order_disable(old_crtc_state)) { const struct intel_crtc_state *old_pipe_crtc_state = intel_atomic_get_old_crtc_state(state, pipe_crtc); @@ -3383,7 +3385,7 @@ static void intel_enable_ddi(struct intel_atomic_state *state, const struct drm_connector_state *conn_state) { struct drm_i915_private *i915 = to_i915(encoder->base.dev); - struct intel_crtc *pipe_crtc; + struct intel_crtc *pipe_crtc; enum pipe pipe; intel_ddi_enable_transcoder_func(encoder, crtc_state); @@ -3394,8 +3396,9 @@ static void intel_enable_ddi(struct intel_atomic_state *state, intel_ddi_wait_for_fec_status(encoder, crtc_state, true); - for_each_intel_crtc_in_pipe_mask_reverse(&i915->drm, pipe_crtc, - intel_crtc_joined_pipe_mask(crtc_state)) { + for_each_intel_crtc_in_mask_priority(i915, pipe_crtc, pipe, + intel_crtc_joined_pipe_mask(crtc_state), + intel_get_pipe_order_enable(crtc_state)) { const struct intel_crtc_state *pipe_crtc_state = intel_atomic_get_new_crtc_state(state, pipe_crtc); diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index db27850b2c36..27622d51a473 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -1737,6 +1737,50 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta hsw_set_transconf(crtc_state); } +static +bool intel_crtc_is_bigjoiner(const struct intel_crtc_state *pipe_config) +{ + return hweight8(pipe_config->joiner_pipes) == 2; +} + +const enum pipe *intel_get_pipe_order_enable(const struct intel_crtc_state *crtc_state) +{ + static const enum pipe ultrajoiner_pipe_order_enable[I915_MAX_PIPES] = { + PIPE_B, PIPE_D, PIPE_C, PIPE_A + }; + static const enum pipe bigjoiner_pipe_order_enable[I915_MAX_PIPES] = { + PIPE_B, PIPE_A, PIPE_D, PIPE_C + }; + static const enum pipe nojoiner_pipe_order_enable[I915_MAX_PIPES] = { + PIPE_A, PIPE_B, PIPE_C, PIPE_D + }; + + if (intel_crtc_is_ultrajoiner(crtc_state)) + return ultrajoiner_pipe_order_enable; + else if (intel_crtc_is_bigjoiner(crtc_state)) + return bigjoiner_pipe_order_enable; + return nojoiner_pipe_order_enable; +} + +const enum pipe *intel_get_pipe_order_disable(const struct intel_crtc_state *crtc_state) +{ + static const enum pipe ultrajoiner_pipe_order_disable[I915_MAX_PIPES] = { + PIPE_A, PIPE_B, PIPE_D, PIPE_C + }; + static const enum pipe bigjoiner_pipe_order_disable[I915_MAX_PIPES] = { + PIPE_A, PIPE_B, PIPE_C, PIPE_D + }; + static const enum pipe nojoiner_pipe_order_disable[I915_MAX_PIPES] = { + PIPE_A, PIPE_B, PIPE_C, PIPE_D + }; + + if (intel_crtc_is_ultrajoiner(crtc_state)) + return ultrajoiner_pipe_order_disable; + else if (intel_crtc_is_bigjoiner(crtc_state)) + return bigjoiner_pipe_order_disable; + return nojoiner_pipe_order_disable; +} + static void hsw_crtc_enable(struct intel_atomic_state *state, struct intel_crtc *crtc) { @@ -1745,19 +1789,21 @@ static void hsw_crtc_enable(struct intel_atomic_state *state, intel_atomic_get_new_crtc_state(state, crtc); struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder; - struct intel_crtc *pipe_crtc; + struct intel_crtc *pipe_crtc; enum pipe pipe; if (drm_WARN_ON(&dev_priv->drm, crtc->active)) return; - for_each_intel_crtc_in_pipe_mask_reverse(&dev_priv->drm, pipe_crtc, - intel_crtc_joined_pipe_mask(new_crtc_state)) + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, + intel_crtc_joined_pipe_mask(new_crtc_state), + intel_get_pipe_order_enable(new_crtc_state)) intel_dmc_enable_pipe(dev_priv, pipe_crtc->pipe); intel_encoders_pre_pll_enable(state, crtc); - for_each_intel_crtc_in_pipe_mask_reverse(&dev_priv->drm, pipe_crtc, - intel_crtc_joined_pipe_mask(new_crtc_state)) { + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, + intel_crtc_joined_pipe_mask(new_crtc_state), + intel_get_pipe_order_enable(new_crtc_state)) { const struct intel_crtc_state *pipe_crtc_state = intel_atomic_get_new_crtc_state(state, pipe_crtc); @@ -1767,8 +1813,9 @@ static void hsw_crtc_enable(struct intel_atomic_state *state, intel_encoders_pre_enable(state, crtc); - for_each_intel_crtc_in_pipe_mask_reverse(&dev_priv->drm, pipe_crtc, - intel_crtc_joined_pipe_mask(new_crtc_state)) { + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, + intel_crtc_joined_pipe_mask(new_crtc_state), + intel_get_pipe_order_enable(new_crtc_state)) { const struct intel_crtc_state *pipe_crtc_state = intel_atomic_get_new_crtc_state(state, pipe_crtc); @@ -1786,8 +1833,9 @@ static void hsw_crtc_enable(struct intel_atomic_state *state, if (!transcoder_is_dsi(cpu_transcoder)) hsw_configure_cpu_transcoder(new_crtc_state); - for_each_intel_crtc_in_pipe_mask_reverse(&dev_priv->drm, pipe_crtc, - intel_crtc_joined_pipe_mask(new_crtc_state)) { + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, + intel_crtc_joined_pipe_mask(new_crtc_state), + intel_get_pipe_order_enable(new_crtc_state)) { const struct intel_crtc_state *pipe_crtc_state = intel_atomic_get_new_crtc_state(state, pipe_crtc); @@ -1822,8 +1870,9 @@ static void hsw_crtc_enable(struct intel_atomic_state *state, intel_encoders_enable(state, crtc); - for_each_intel_crtc_in_pipe_mask_reverse(&dev_priv->drm, pipe_crtc, - intel_crtc_joined_pipe_mask(new_crtc_state)) { + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, + intel_crtc_joined_pipe_mask(new_crtc_state), + intel_get_pipe_order_enable(new_crtc_state)) { const struct intel_crtc_state *pipe_crtc_state = intel_atomic_get_new_crtc_state(state, pipe_crtc); enum pipe hsw_workaround_pipe; @@ -1908,7 +1957,7 @@ static void hsw_crtc_disable(struct intel_atomic_state *state, const struct intel_crtc_state *old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); struct drm_i915_private *i915 = to_i915(crtc->base.dev); - struct intel_crtc *pipe_crtc; + struct intel_crtc *pipe_crtc; enum pipe pipe; /* * FIXME collapse everything to one hook. @@ -1917,8 +1966,9 @@ static void hsw_crtc_disable(struct intel_atomic_state *state, intel_encoders_disable(state, crtc); intel_encoders_post_disable(state, crtc); - for_each_intel_crtc_in_pipe_mask(&i915->drm, pipe_crtc, - intel_crtc_joined_pipe_mask(old_crtc_state)) { + for_each_intel_crtc_in_mask_priority(i915, pipe_crtc, pipe, + intel_crtc_joined_pipe_mask(old_crtc_state), + intel_get_pipe_order_disable(old_crtc_state)) { const struct intel_crtc_state *old_pipe_crtc_state = intel_atomic_get_old_crtc_state(state, pipe_crtc); @@ -1927,8 +1977,9 @@ static void hsw_crtc_disable(struct intel_atomic_state *state, intel_encoders_post_pll_disable(state, crtc); - for_each_intel_crtc_in_pipe_mask(&i915->drm, pipe_crtc, - intel_crtc_joined_pipe_mask(old_crtc_state)) + for_each_intel_crtc_in_mask_priority(i915, pipe_crtc, pipe, + intel_crtc_joined_pipe_mask(old_crtc_state), + intel_get_pipe_order_disable(old_crtc_state)) intel_dmc_disable_pipe(i915, pipe_crtc->pipe); } diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h index dbbe23ea14fc..72dc495c645c 100644 --- a/drivers/gpu/drm/i915/display/intel_display.h +++ b/drivers/gpu/drm/i915/display/intel_display.h @@ -274,6 +274,11 @@ enum phy_fia { &(dev)->mode_config.crtc_list, \ base.head) +#define for_each_intel_crtc_in_mask_priority(__dev_priv, intel_crtc, __p, __mask, __priolist) \ + for_each_pipe(__dev_priv, __p) \ + for_each_if((__mask) & BIT(__priolist[__p])) \ + for_each_if(intel_crtc = intel_crtc_for_pipe(to_intel_display(&__dev_priv->drm), __priolist[__p])) + #define for_each_intel_crtc_in_pipe_mask(dev, intel_crtc, pipe_mask) \ list_for_each_entry(intel_crtc, \ &(dev)->mode_config.crtc_list, \ @@ -431,6 +436,8 @@ bool intel_crtc_is_ultrajoiner(const struct intel_crtc_state *crtc_state); bool intel_crtc_is_ultrajoiner_primary(const struct intel_crtc_state *crtc_state); u8 intel_crtc_joiner_secondary_pipes(const struct intel_crtc_state *crtc_state); struct intel_crtc *intel_primary_crtc(const struct intel_crtc_state *crtc_state); +const enum pipe *intel_get_pipe_order_enable(const struct intel_crtc_state *crtc_state); +const enum pipe *intel_get_pipe_order_disable(const struct intel_crtc_state *crtc_state); bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state); bool intel_pipe_config_compare(const struct intel_crtc_state *current_config, const struct intel_crtc_state *pipe_config, diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index be79783ce09b..1c87f81568c8 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -1003,7 +1003,7 @@ static void intel_mst_post_disable_dp(struct intel_atomic_state *state, struct drm_dp_mst_atomic_payload *new_payload = drm_atomic_get_mst_payload_state(new_mst_state, connector->port); struct drm_i915_private *dev_priv = to_i915(connector->base.dev); - struct intel_crtc *pipe_crtc; + struct intel_crtc *pipe_crtc; enum pipe pipe; bool last_mst_stream; intel_dp->active_mst_links--; @@ -1012,8 +1012,9 @@ static void intel_mst_post_disable_dp(struct intel_atomic_state *state, DISPLAY_VER(dev_priv) >= 12 && last_mst_stream && !intel_dp_mst_is_master_trans(old_crtc_state)); - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, - intel_crtc_joined_pipe_mask(old_crtc_state)) { + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, + intel_crtc_joined_pipe_mask(old_crtc_state), + intel_get_pipe_order_disable(old_crtc_state)) { const struct intel_crtc_state *old_pipe_crtc_state = intel_atomic_get_old_crtc_state(state, pipe_crtc); @@ -1037,8 +1038,9 @@ static void intel_mst_post_disable_dp(struct intel_atomic_state *state, intel_ddi_disable_transcoder_func(old_crtc_state); - for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc, - intel_crtc_joined_pipe_mask(old_crtc_state)) { + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, + intel_crtc_joined_pipe_mask(old_crtc_state), + intel_get_pipe_order_disable(old_crtc_state)) { const struct intel_crtc_state *old_pipe_crtc_state = intel_atomic_get_old_crtc_state(state, pipe_crtc); @@ -1257,6 +1259,7 @@ static void intel_mst_enable_dp(struct intel_atomic_state *state, enum transcoder trans = pipe_config->cpu_transcoder; bool first_mst_stream = intel_dp->active_mst_links == 1; struct intel_crtc *pipe_crtc; + enum pipe pipe; int ret; drm_WARN_ON(&dev_priv->drm, pipe_config->has_pch_encoder); @@ -1304,8 +1307,9 @@ static void intel_mst_enable_dp(struct intel_atomic_state *state, intel_enable_transcoder(pipe_config); - for_each_intel_crtc_in_pipe_mask_reverse(&dev_priv->drm, pipe_crtc, - intel_crtc_joined_pipe_mask(pipe_config)) { + for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe, + intel_crtc_joined_pipe_mask(pipe_config), + intel_get_pipe_order_enable(pipe_config)) { const struct intel_crtc_state *pipe_crtc_state = intel_atomic_get_new_crtc_state(state, pipe_crtc);