On Mon, May 20, 2024 at 10:38:36AM +0300, Stanislav Lisovskiy wrote: > Lets implement or change basic functions required for ultrajoiner > support from atomic commit/modesetting point of view. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx> > --- > drivers/gpu/drm/i915/display/intel_display.c | 66 +++++++++++++++++--- > 1 file changed, 56 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index c74721188e59..c390b79a43d6 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -242,33 +242,65 @@ is_trans_port_sync_mode(const struct intel_crtc_state *crtc_state) > is_trans_port_sync_slave(crtc_state); > } > > -static enum pipe joiner_master_pipe(const struct intel_crtc_state *crtc_state) > +static u8 joiner_master_pipes(const struct intel_crtc_state *crtc_state) > { > - return ffs(crtc_state->joiner_pipes) - 1; > + return BIT(PIPE_A) | BIT(PIPE_C); Not a fan of the hardcoded pipes. We could just do something like joiner_pipes & ((BIT(2) | BIT(0)) << joiner_master_pipe()) or some variant of that. > +} > + > +static u8 joiner_primary_master_pipes(const struct intel_crtc_state *crtc_state) > +{ > + return BIT(PIPE_A); This is just the joiner_master_pipe() we already have. > } > > u8 intel_crtc_joiner_slave_pipes(const struct intel_crtc_state *crtc_state) > { > - if (crtc_state->joiner_pipes) > - return crtc_state->joiner_pipes & ~BIT(joiner_master_pipe(crtc_state)); > + if (intel_is_ultrajoiner(crtc_state)) > + return crtc_state->joiner_pipes & ~joiner_primary_master_pipes(crtc_state); > + else if (intel_is_bigjoiner(crtc_state)) > + return crtc_state->joiner_pipes & ~joiner_master_pipes(crtc_state); > else > return 0; I don't see why this should make any distinction between bigjoiner and ultrajoiner. Either it returns everything that isn't the overall master, or it returns just all the bigjoiner slave pipes. Which one we want depends on the use case I guess. So we might need both variants. > } > > -bool intel_crtc_is_joiner_slave(const struct intel_crtc_state *crtc_state) > +bool intel_crtc_is_bigjoiner_slave(const struct intel_crtc_state *crtc_state) > { > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > return crtc_state->joiner_pipes && > - crtc->pipe != joiner_master_pipe(crtc_state); > + !(BIT(crtc->pipe) & joiner_master_pipes(crtc_state)); I'd probably add a joiner_slave_pipes() so that the logic is less convoluted. But I think first we need a solid agreement on the terminology, and stick to it consistently. Perhaps we need names for? - the single master within the overall set of joined pipes (be it ultrajoiner master or the bigjoiner/uncompressed joiner master when ultrajoiner isn't used). Just call this joiner_master perhaps? Or perhaps just call it ultrajoiner_master but document that it is valid to use it also for the non-ultrajoiner cases. - every other pipe in the set, ie. the inverse of above Should be just {ultra,}joiner_slaves to match the above I guess? Do we actually even need this? Not sure. And the for the modeset sequencing we would perhaps need: - all bigjoiner masters within the entire set of joined pipes - all bigjoiner slaves within the entire set of joined pipes (inverse of the above) The one slight snag here is that the "bigjoiner" name is a bit incorrect for uncompressed joiner, but unless we want to come up with some other name for these then I guess we'll just have to live with it. The other option is we try to come up with some generic names for the two levels of pipe roles. -- Ville Syrjälä Intel