| Message ID | 20240520073839.23881-3-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Ultrajoiner basic functionality series | expand |
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@intel.com> > --- > 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.
On Mon, May 20, 2024 at 09:24:45PM +0300, Ville Syrjälä wrote: > 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@intel.com> > > --- > > 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. Well, here we need to decide whats worse: hardcoded bits/shifts versus harcoded pipes.. I would vote for pipes then, with reasoning that they are at least more obvious and easy to read. It is anyway quite easy to change those here or make it platform based, if needed. > > > +} > > + > > +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. I decided to convert joiner_master_pipe to joiner_master_pipes which should return a mask, instead of a pipe. That approach makes it more generic: for bigjoiner we still get only a single bit set in a mask, however for ultrajoiner case we have now 2 master pipes, so we need a mask here. joiner_primary_master_pipes is indeed supposed to return only a single primary master pipe, however I decided that operating with masks instead of enum, seems more generic and practical approach, for example if we need to get all pipes, which are not primary master, as below. > > > } > > > > 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, For ultrajoiner that is slave pipes + secondary master pipe. I.e it is everything that is below primary master. > returns just all the bigjoiner slave pipes. Which one we want > depends on the use case I guess. So we might need both variants. Yeah, we need both ways: sometimes we need to get all pipes except primary master. And sometimes we need to get only slave pipes in Bigjoiner terminology. There are use cases for both. However definition of slave pipe is a bit tricky here, because technically secondary master pipe is also a slave pipe in relation to primary master pipe. > > > } > > > > -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. Yeah, then joiner_slave_pipes would have to return only slave pipes in bigjoiner terminology. > > 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. I think it would be quite natural to call it a primary master. Initially BSpec called it that way and it sounds logical. I.e now we have not only master/slave hierarchy, but also second level of hierarchy between masters: secondary master and primary master. Other names sound less obvious tbh: i.e "master of masters" :) or "overall master" and etc.. That is why I'm a bit opposed to that Jani says to rename master/slave to primary/secondary - we get a problem with naming for Ultrajoiner then: as we are going to have primary of primary pipe or smth like that. Can't think of anything better than using primary/secondary master. If anyone has better sounding ideas - you are welcome. > - 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. We should just follow a KISS rule here, I guess.. Stan > > -- > Ville Syrjälä > Intel
On Tue, May 21, 2024 at 11:25:31AM +0300, Lisovskiy, Stanislav wrote: > On Mon, May 20, 2024 at 09:24:45PM +0300, Ville Syrjälä wrote: > > 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@intel.com> > > > --- > > > 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. > > Well, here we need to decide whats worse: hardcoded bits/shifts versus harcoded pipes.. > I would vote for pipes then, with reasoning that they are at least more obvious and easy to read. > It is anyway quite easy to change those here or make it platform based, if needed. Hardcoded pipes aren't going to allow us to make the rest of the code generic because the overall master pipe can be anything when ultrajoiner isn't used. Eg. the way we assign the bigjoiner_pipes is by simply setting a some number (either two or four) of consecutive bits in the mask. In order for that to keep working universally these functions must be able to answer questions based on that bitmask, no matter which consecutive set of bits are set. > > > > > > +} > > > + > > > +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. > > I decided to convert joiner_master_pipe to joiner_master_pipes which should return a mask, > instead of a pipe. > That approach makes it more generic: for bigjoiner we still get only a single bit set in a mask, > however for ultrajoiner case we have now 2 master pipes, so we need a mask here. > > joiner_primary_master_pipes is indeed supposed to return only a single primary master pipe, > however I decided that operating with masks instead of enum, seems more generic and practical approach, > for example if we need to get all pipes, which are not primary master, as below. > > > > > > } > > > > > > 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, > > For ultrajoiner that is slave pipes + secondary master pipe. > I.e it is everything that is below primary master. Same for for non-ultrajoiner. The only difference is that there is just the one slave rather than three. But the callers don't need to care about that in general. I suspect there is probably only few uses cases for this: - the master->slave state copying. And there we just want to go through all the slaves, no matter how many there are - during the high level modeset sequence (and probably a few other places as well) we need to simply skip all the slaves, and again it doesn't matter how many there are For the plane updates and such we probably don't really need to care about the master/slave relationships, so the current thing that just iterates all joined pipes will work perfectly fine. And for actual modeset sequencing I suspect we just need the bigjoiner master/slave bitmasks and make sure we iterate through each in turn: enable: 1. for_each_reverse(bigjoiner_slaves) 2. for_each_reverse(bigjoiner_masters) disable: 1. for_each(bigjoiner_masters) 2. for_each(bigjoiner_slaves) > > returns just all the bigjoiner slave pipes. Which one we want > > depends on the use case I guess. So we might need both variants. > > Yeah, we need both ways: sometimes we need to get all pipes except primary master. > And sometimes we need to get only slave pipes in Bigjoiner terminology. > There are use cases for both. > > However definition of slave pipe is a bit tricky here, because technically secondary > master pipe is also a slave pipe in relation to primary master pipe. > > > > > > } > > > > > > -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. > > Yeah, then joiner_slave_pipes would have to return only slave pipes in > bigjoiner terminology. > > > > > 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. > > I think it would be quite natural to call it a primary master. > > Initially BSpec called it that way and it sounds logical. > > I.e now we have not only master/slave hierarchy, but also > second level of hierarchy between masters: secondary master > and primary master. > Other names sound less obvious tbh: i.e "master of masters" :) > or "overall master" and etc.. > > That is why I'm a bit opposed to that Jani says to rename > master/slave to primary/secondary - we get a problem with > naming for Ultrajoiner then: > as we are going to have primary of primary pipe or smth like that. > > Can't think of anything better than using primary/secondary master. > If anyone has better sounding ideas - you are welcome. The bspec rename does this: master -> primary slave -> secondary I do agree that what you're going for here would have been pretty natural way to experss this, but I think that ship sailed when the annoying bspec rename happened. If we now start using those same names to refer to a completely different concept I think the end result will be extremely confusing. IMO we probably need to slighly extend the ultrajoiner and bigjoiners terms to cover all the joiner cases, which would look something like this: - ultrajoiner master/primary = the first pipe in the set - ultrajoiner slaves/secondaries = the rest of the pipes in the set - bigjoiner masters/primaries = first pipe + third pipe + ... in the set - bigjoiner slaves/secondaries = second pipe + fourth pipe + ... in the set Or perhaps we just drop the "ultra" part form the first two and speak of just "joiner" in general when referring to things on that level? Either that or we try to invent some new terminology for these two levels of roles that doesn't conflict badly with the bspec terminology. What that would be I'm not sure. Up/down, heads/tails, noble/peasant, apple/orange? Nothing truly inspiring comes to mind right now.
On Tue, May 21, 2024 at 09:09:16PM +0300, Ville Syrjälä wrote: > On Tue, May 21, 2024 at 11:25:31AM +0300, Lisovskiy, Stanislav wrote: > > On Mon, May 20, 2024 at 09:24:45PM +0300, Ville Syrjälä wrote: > > > 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@intel.com> > > > > --- > > > > 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. > > > > Well, here we need to decide whats worse: hardcoded bits/shifts versus harcoded pipes.. > > I would vote for pipes then, with reasoning that they are at least more obvious and easy to read. > > It is anyway quite easy to change those here or make it platform based, if needed. > > Hardcoded pipes aren't going to allow us to make the rest of the > code generic because the overall master pipe can be anything when > ultrajoiner isn't used. joiner_master_pipes in current revision isn't supposed to calculate master pipe for given crtc_state it just returns overall mask of the pipes, which are allowed to be master. For actual calculation another function is supposed to be used, which is intel_crtc_master_pipe. That will determine, if its ultrajoiner or bigjoiner and return correspondent pipe. It is also based now on assumption that master pipe is always the slave pipe - 1, which I also don't like, if one day BSpec decides to make it possible to have like master to be pipe B and slave to be pipe A, then we are screwed with that approach. Thats why I would prefer overall to set all those relations by some platform based or programmatically set table mappings, however I guess that won't go through reviews :) > > Eg. the way we assign the bigjoiner_pipes is by simply setting a > some number (either two or four) of consecutive bits in the mask. > In order for that to keep working universally these functions must > be able to answer questions based on that bitmask, no matter which > consecutive set of bits are set. You probably mean that stuff: <------>if (intel_dp_need_ultrajoiner(intel_dp, adjusted_mode->crtc_clock)) <------><------>pipe_config->joiner_pipes = GENMASK(crtc->pipe + 3, crtc->pipe); <------>else if (intel_dp_need_bigjoiner(intel_dp, connector, <------><------><------><------><------> adjusted_mode->crtc_hdisplay, <------><------><------><------><------> adjusted_mode->crtc_clock)) <------><------>pipe_config->joiner_pipes = GENMASK(crtc->pipe + 1, crtc->pipe); That one is also about the same, in fact I think we should be able to set this in a more meaningful way - what if some platform will support 3 but not all 4 pipes for joiner, or 5 pipes.. Best approach would be to use something like joiner_master_pipes | joiner_slave_pipes functions there, which return platform based masks of pipes, because this GENMASK is kinda hardcoded and not explicit enough. > > > > > > > > > > +} > > > > + > > > > +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. > > > > I decided to convert joiner_master_pipe to joiner_master_pipes which should return a mask, > > instead of a pipe. > > That approach makes it more generic: for bigjoiner we still get only a single bit set in a mask, > > however for ultrajoiner case we have now 2 master pipes, so we need a mask here. > > > > joiner_primary_master_pipes is indeed supposed to return only a single primary master pipe, > > however I decided that operating with masks instead of enum, seems more generic and practical approach, > > for example if we need to get all pipes, which are not primary master, as below. > > > > > > > > > } > > > > > > > > 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, > > > > For ultrajoiner that is slave pipes + secondary master pipe. > > I.e it is everything that is below primary master. > > Same for for non-ultrajoiner. The only difference is that there is just > the one slave rather than three. But the callers don't need to care > about that in general. There is a difference when we are doing actual register programming, there we need to know exactly which pipes are just slaves, which pipe is secondary or primary master. However for things like state copying, yep we need to treat all except primary master as slaves. > > I suspect there is probably only few uses cases for this: > - the master->slave state copying. And there we just want to > go through all the slaves, no matter how many there are > - during the high level modeset sequence (and probably a few > other places as well) we need to simply skip all the slaves, > and again it doesn't matter how many there are > > For the plane updates and such we probably don't really need to > care about the master/slave relationships, so the current thing > that just iterates all joined pipes will work perfectly fine. > > And for actual modeset sequencing I suspect we just need the > bigjoiner master/slave bitmasks and make sure we iterate through > each in turn: > enable: > 1. for_each_reverse(bigjoiner_slaves) > 2. for_each_reverse(bigjoiner_masters) > disable: > 1. for_each(bigjoiner_masters) > 2. for_each(bigjoiner_slaves) I would just unite those 2 cycles using new macro, I introduced, where we can set explicitly the order in which it should attend each pipe in mask. Also it will be completely transparent from hsw_crtc_enable/hsw_crtc_disable point of view, without even having to mention things like master/slave there. > > > > returns just all the bigjoiner slave pipes. Which one we want > > > depends on the use case I guess. So we might need both variants. > > > > Yeah, we need both ways: sometimes we need to get all pipes except primary master. > > And sometimes we need to get only slave pipes in Bigjoiner terminology. > > There are use cases for both. > > > > However definition of slave pipe is a bit tricky here, because technically secondary > > master pipe is also a slave pipe in relation to primary master pipe. > > > > > > > > > } > > > > > > > > -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. > > > > Yeah, then joiner_slave_pipes would have to return only slave pipes in > > bigjoiner terminology. > > > > > > > > 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. > > > > I think it would be quite natural to call it a primary master. > > > > Initially BSpec called it that way and it sounds logical. > > > > I.e now we have not only master/slave hierarchy, but also > > second level of hierarchy between masters: secondary master > > and primary master. > > Other names sound less obvious tbh: i.e "master of masters" :) > > or "overall master" and etc.. > > > > That is why I'm a bit opposed to that Jani says to rename > > master/slave to primary/secondary - we get a problem with > > naming for Ultrajoiner then: > > as we are going to have primary of primary pipe or smth like that. > > > > Can't think of anything better than using primary/secondary master. > > If anyone has better sounding ideas - you are welcome. > > The bspec rename does this: > master -> primary > slave -> secondary > > I do agree that what you're going for here would have been pretty > natural way to experss this, but I think that ship sailed when > the annoying bspec rename happened. If we now start using those > same names to refer to a completely different concept I think the > end result will be extremely confusing. > > IMO we probably need to slighly extend the ultrajoiner and bigjoiners > terms to cover all the joiner cases, which would look something like this: > - ultrajoiner master/primary = the first pipe in the set > - ultrajoiner slaves/secondaries = the rest of the pipes in the set > - bigjoiner masters/primaries = first pipe + third pipe + ... in the set > - bigjoiner slaves/secondaries = second pipe + fourth pipe + ... in the set > > Or perhaps we just drop the "ultra" part form the first two and > speak of just "joiner" in general when referring to things on > that level? Then we won't be able to distinguish between primary master and secondary master. BSpec offers to call it Ultrajoiner master and Ultrajoiner secondary which is assuming to be Bigjoiner master, however still _quite_ confusing. We can call the primary master to be Ultrajoiner primary and refer to the rest of pipes like bigjoiner primary/secondary. To me it sounds so much more complex compared to primary/secondary master/slave to be honest.. Looks like we are inventing troubles for ourselves. Stan > > Either that or we try to invent some new terminology for these > two levels of roles that doesn't conflict badly with the bspec > terminology. What that would be I'm not sure. Up/down, > heads/tails, noble/peasant, apple/orange? Nothing truly > inspiring comes to mind right now. > > -- > Ville Syrjälä > Intel
On Wed, May 22, 2024 at 11:01:32AM +0300, Lisovskiy, Stanislav wrote: > On Tue, May 21, 2024 at 09:09:16PM +0300, Ville Syrjälä wrote: > > On Tue, May 21, 2024 at 11:25:31AM +0300, Lisovskiy, Stanislav wrote: > > > On Mon, May 20, 2024 at 09:24:45PM +0300, Ville Syrjälä wrote: > > > > 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@intel.com> > > > > > --- > > > > > 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. > > > > > > Well, here we need to decide whats worse: hardcoded bits/shifts versus harcoded pipes.. > > > I would vote for pipes then, with reasoning that they are at least more obvious and easy to read. > > > It is anyway quite easy to change those here or make it platform based, if needed. > > > > Hardcoded pipes aren't going to allow us to make the rest of the > > code generic because the overall master pipe can be anything when > > ultrajoiner isn't used. > > joiner_master_pipes in current revision isn't supposed to calculate > master pipe for given crtc_state it just returns overall mask of the > pipes, which are allowed to be master. Then it seems misnamed, and also why is it taking a crtc_state? > > For actual calculation another function is supposed to be used, which is > intel_crtc_master_pipe. > That will determine, if its ultrajoiner or bigjoiner and return correspondent > pipe. It is also based now on assumption that master pipe is always the > slave pipe - 1, which I also don't like, if one day BSpec decides to make it > possible to have like master to be pipe B and slave to be pipe A, > then we are screwed with that approach. > Thats why I would prefer overall to set all those relations by some platform > based or programmatically set table mappings, however I guess that won't go > through reviews :) > > > > > Eg. the way we assign the bigjoiner_pipes is by simply setting a > > some number (either two or four) of consecutive bits in the mask. > > In order for that to keep working universally these functions must > > be able to answer questions based on that bitmask, no matter which > > consecutive set of bits are set. > > You probably mean that stuff: > > <------>if (intel_dp_need_ultrajoiner(intel_dp, adjusted_mode->crtc_clock)) > <------><------>pipe_config->joiner_pipes = GENMASK(crtc->pipe + 3, crtc->pipe); > <------>else if (intel_dp_need_bigjoiner(intel_dp, connector, > <------><------><------><------><------> adjusted_mode->crtc_hdisplay, > <------><------><------><------><------> adjusted_mode->crtc_clock)) > <------><------>pipe_config->joiner_pipes = GENMASK(crtc->pipe + 1, crtc->pipe); > > That one is also about the same, in fact I think we should be able > to set this in a more meaningful way - what if some platform will support > 3 but not all 4 pipes for joiner, or 5 pipes.. > Best approach would be to use something like joiner_master_pipes | joiner_slave_pipes > functions there, which return platform based masks of pipes, because > this GENMASK is kinda hardcoded and not explicit enough. > > > > > > > > > > > > > > > +} > > > > > + > > > > > +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. > > > > > > I decided to convert joiner_master_pipe to joiner_master_pipes which should return a mask, > > > instead of a pipe. > > > That approach makes it more generic: for bigjoiner we still get only a single bit set in a mask, > > > however for ultrajoiner case we have now 2 master pipes, so we need a mask here. > > > > > > joiner_primary_master_pipes is indeed supposed to return only a single primary master pipe, > > > however I decided that operating with masks instead of enum, seems more generic and practical approach, > > > for example if we need to get all pipes, which are not primary master, as below. > > > > > > > > > > > > } > > > > > > > > > > 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, > > > > > > For ultrajoiner that is slave pipes + secondary master pipe. > > > I.e it is everything that is below primary master. > > > > Same for for non-ultrajoiner. The only difference is that there is just > > the one slave rather than three. But the callers don't need to care > > about that in general. > > There is a difference when we are doing actual register programming, there > we need to know exactly which pipes are just slaves, That would be bigjoiner_slave_pipes() in what I outlined. > which pipe is secondary > or primary master. And that is just the difference between ultrajoiner_master_pipe() vs. bigjoiner_master_pipes() & ~BIT(ultrajoiner_master_pipe()) > However for things like state copying, yep we need to treat all except > primary master as slaves. And that would be just iterating through ultrajoiner_slave_pipes() (or joiner_slave_pipes(), depending on which of naming schemes I listed would be used). > > > > > I suspect there is probably only few uses cases for this: > > - the master->slave state copying. And there we just want to > > go through all the slaves, no matter how many there are > > - during the high level modeset sequence (and probably a few > > other places as well) we need to simply skip all the slaves, > > and again it doesn't matter how many there are > > > > For the plane updates and such we probably don't really need to > > care about the master/slave relationships, so the current thing > > that just iterates all joined pipes will work perfectly fine. > > > > And for actual modeset sequencing I suspect we just need the > > bigjoiner master/slave bitmasks and make sure we iterate through > > each in turn: > > enable: > > 1. for_each_reverse(bigjoiner_slaves) > > 2. for_each_reverse(bigjoiner_masters) > > disable: > > 1. for_each(bigjoiner_masters) > > 2. for_each(bigjoiner_slaves) > > I would just unite those 2 cycles using new macro, I introduced, where > we can set explicitly the order in which it should attend each pipe in mask. > Also it will be completely transparent from hsw_crtc_enable/hsw_crtc_disable > point of view, without even having to mention things like master/slave there. Yeah, a single macro would be nice, assuming we can achive it without too many horrors. But still, the implementation just needs those two masks. > > > > > > > returns just all the bigjoiner slave pipes. Which one we want > > > > depends on the use case I guess. So we might need both variants. > > > > > > Yeah, we need both ways: sometimes we need to get all pipes except primary master. > > > And sometimes we need to get only slave pipes in Bigjoiner terminology. > > > There are use cases for both. > > > > > > However definition of slave pipe is a bit tricky here, because technically secondary > > > master pipe is also a slave pipe in relation to primary master pipe. > > > > > > > > > > > > } > > > > > > > > > > -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. > > > > > > Yeah, then joiner_slave_pipes would have to return only slave pipes in > > > bigjoiner terminology. > > > > > > > > > > > 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. > > > > > > I think it would be quite natural to call it a primary master. > > > > > > Initially BSpec called it that way and it sounds logical. > > > > > > I.e now we have not only master/slave hierarchy, but also > > > second level of hierarchy between masters: secondary master > > > and primary master. > > > Other names sound less obvious tbh: i.e "master of masters" :) > > > or "overall master" and etc.. > > > > > > That is why I'm a bit opposed to that Jani says to rename > > > master/slave to primary/secondary - we get a problem with > > > naming for Ultrajoiner then: > > > as we are going to have primary of primary pipe or smth like that. > > > > > > Can't think of anything better than using primary/secondary master. > > > If anyone has better sounding ideas - you are welcome. > > > > The bspec rename does this: > > master -> primary > > slave -> secondary > > > > I do agree that what you're going for here would have been pretty > > natural way to experss this, but I think that ship sailed when > > the annoying bspec rename happened. If we now start using those > > same names to refer to a completely different concept I think the > > end result will be extremely confusing. > > > > IMO we probably need to slighly extend the ultrajoiner and bigjoiners > > terms to cover all the joiner cases, which would look something like this: > > - ultrajoiner master/primary = the first pipe in the set > > - ultrajoiner slaves/secondaries = the rest of the pipes in the set > > - bigjoiner masters/primaries = first pipe + third pipe + ... in the set > > - bigjoiner slaves/secondaries = second pipe + fourth pipe + ... in the set > > > > Or perhaps we just drop the "ultra" part form the first two and > > speak of just "joiner" in general when referring to things on > > that level? > > Then we won't be able to distinguish between primary master and secondary master. > BSpec offers to call it Ultrajoiner master and Ultrajoiner secondary > which is assuming to be Bigjoiner master, however still _quite_ confusing. > We can call the primary master to be Ultrajoiner primary and refer to the rest > of pipes like bigjoiner primary/secondary. > > To me it sounds so much more complex compared to primary/secondary master/slave > to be honest.. Looks like we are inventing troubles for ourselves. Some people will just keep on complaining about the master/slave terminology, and having to keep responding to that is very tiresome. So we are going to do the rename to align to the new primary/secondary bspec terminology, if only to avoid that annoyance. So to avoid a bit of the confusion I think we can just use the generic term "joiner" at the high level stuff. The first pipe in the set will be the joiner_primary, everyone else is a joiner_secondary. For the low level DSS_CTL register frobbing I think we just need to be able to answer these simple questions: - is ultrajoiner used? - is bigjoiner used? - is uncompressed joiner used? - is this pipe an ultrajoiner primary? - is this pipe a bigjoiner primary? - is this pipe an uncompressed joiner primary? And beyond that we just need to be able to split bigjoiner/uncompressed joiner primaries from bigjoiner/uncompressed joiner secondaries for the purposes of the modeset iterator macros. IMO for that we can keep using the term "bigjoiner" even if it also covers uncompressed joiner as well.
On Wed, May 22, 2024 at 02:40:56PM +0300, Ville Syrjälä wrote: > On Wed, May 22, 2024 at 11:01:32AM +0300, Lisovskiy, Stanislav wrote: > > On Tue, May 21, 2024 at 09:09:16PM +0300, Ville Syrjälä wrote: > > > On Tue, May 21, 2024 at 11:25:31AM +0300, Lisovskiy, Stanislav wrote: > > > > On Mon, May 20, 2024 at 09:24:45PM +0300, Ville Syrjälä wrote: > > > > > 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@intel.com> > > > > > > --- > > > > > > 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. > > > > > > > > Well, here we need to decide whats worse: hardcoded bits/shifts versus harcoded pipes.. > > > > I would vote for pipes then, with reasoning that they are at least more obvious and easy to read. > > > > It is anyway quite easy to change those here or make it platform based, if needed. > > > > > > Hardcoded pipes aren't going to allow us to make the rest of the > > > code generic because the overall master pipe can be anything when > > > ultrajoiner isn't used. > > > > joiner_master_pipes in current revision isn't supposed to calculate > > master pipe for given crtc_state it just returns overall mask of the > > pipes, which are allowed to be master. > > Then it seems misnamed, and also why is it taking a crtc_state? Yeah, crtc_state isn't necessary here, I think. > > > > > For actual calculation another function is supposed to be used, which is > > intel_crtc_master_pipe. > > That will determine, if its ultrajoiner or bigjoiner and return correspondent > > pipe. It is also based now on assumption that master pipe is always the > > slave pipe - 1, which I also don't like, if one day BSpec decides to make it > > possible to have like master to be pipe B and slave to be pipe A, > > then we are screwed with that approach. > > Thats why I would prefer overall to set all those relations by some platform > > based or programmatically set table mappings, however I guess that won't go > > through reviews :) > > > > > > > > Eg. the way we assign the bigjoiner_pipes is by simply setting a > > > some number (either two or four) of consecutive bits in the mask. > > > In order for that to keep working universally these functions must > > > be able to answer questions based on that bitmask, no matter which > > > consecutive set of bits are set. > > > > You probably mean that stuff: > > > > <------>if (intel_dp_need_ultrajoiner(intel_dp, adjusted_mode->crtc_clock)) > > <------><------>pipe_config->joiner_pipes = GENMASK(crtc->pipe + 3, crtc->pipe); > > <------>else if (intel_dp_need_bigjoiner(intel_dp, connector, > > <------><------><------><------><------> adjusted_mode->crtc_hdisplay, > > <------><------><------><------><------> adjusted_mode->crtc_clock)) > > <------><------>pipe_config->joiner_pipes = GENMASK(crtc->pipe + 1, crtc->pipe); > > > > That one is also about the same, in fact I think we should be able > > to set this in a more meaningful way - what if some platform will support > > 3 but not all 4 pipes for joiner, or 5 pipes.. > > Best approach would be to use something like joiner_master_pipes | joiner_slave_pipes > > functions there, which return platform based masks of pipes, because > > this GENMASK is kinda hardcoded and not explicit enough. > > > > > > > > > > > > > > > > > > > > +} > > > > > > + > > > > > > +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. > > > > > > > > I decided to convert joiner_master_pipe to joiner_master_pipes which should return a mask, > > > > instead of a pipe. > > > > That approach makes it more generic: for bigjoiner we still get only a single bit set in a mask, > > > > however for ultrajoiner case we have now 2 master pipes, so we need a mask here. > > > > > > > > joiner_primary_master_pipes is indeed supposed to return only a single primary master pipe, > > > > however I decided that operating with masks instead of enum, seems more generic and practical approach, > > > > for example if we need to get all pipes, which are not primary master, as below. > > > > > > > > > > > > > > > } > > > > > > > > > > > > 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, > > > > > > > > For ultrajoiner that is slave pipes + secondary master pipe. > > > > I.e it is everything that is below primary master. > > > > > > Same for for non-ultrajoiner. The only difference is that there is just > > > the one slave rather than three. But the callers don't need to care > > > about that in general. > > > > There is a difference when we are doing actual register programming, there > > we need to know exactly which pipes are just slaves, > > That would be bigjoiner_slave_pipes() in what I outlined. > > > which pipe is secondary > > or primary master. > > And that is just the difference between ultrajoiner_master_pipe() vs. > bigjoiner_master_pipes() & ~BIT(ultrajoiner_master_pipe()) > > > However for things like state copying, yep we need to treat all except > > primary master as slaves. > > And that would be just iterating through > ultrajoiner_slave_pipes() (or joiner_slave_pipes(), depending > on which of naming schemes I listed would be used). So are we still going to use ultrajoiner/bigjoiner names in functions?.. :( I thought that we are now calling primary master "joiner_primary" and everything else is "joiner_secondary". I would get all masters with joiner_primary_pipes(renamed from current joiner_master_pipes) All bigjoiner slave pipes could be obtained by joiner_secondary_pipes &~ joiner_master_pipes All ultrajoiner slave pipes(including secondary master) would be just joiner_secondary_pipes. And ultrajoiner primary master we shall get by joiner_primary_pipE(returns just pipe enum). That way we get rid of bigjoiner/ultrajoiner naming at least in most places and also using new terminology. Are you ok with that? Stan > > > > > > > > > I suspect there is probably only few uses cases for this: > > > - the master->slave state copying. And there we just want to > > > go through all the slaves, no matter how many there are > > > - during the high level modeset sequence (and probably a few > > > other places as well) we need to simply skip all the slaves, > > > and again it doesn't matter how many there are > > > > > > For the plane updates and such we probably don't really need to > > > care about the master/slave relationships, so the current thing > > > that just iterates all joined pipes will work perfectly fine. > > > > > > And for actual modeset sequencing I suspect we just need the > > > bigjoiner master/slave bitmasks and make sure we iterate through > > > each in turn: > > > enable: > > > 1. for_each_reverse(bigjoiner_slaves) > > > 2. for_each_reverse(bigjoiner_masters) > > > disable: > > > 1. for_each(bigjoiner_masters) > > > 2. for_each(bigjoiner_slaves) > > > > I would just unite those 2 cycles using new macro, I introduced, where > > we can set explicitly the order in which it should attend each pipe in mask. > > Also it will be completely transparent from hsw_crtc_enable/hsw_crtc_disable > > point of view, without even having to mention things like master/slave there. > > Yeah, a single macro would be nice, assuming we can achive it without > too many horrors. But still, the implementation just needs those two > masks. > > > > > > > > > > > returns just all the bigjoiner slave pipes. Which one we want > > > > > depends on the use case I guess. So we might need both variants. > > > > > > > > Yeah, we need both ways: sometimes we need to get all pipes except primary master. > > > > And sometimes we need to get only slave pipes in Bigjoiner terminology. > > > > There are use cases for both. > > > > > > > > However definition of slave pipe is a bit tricky here, because technically secondary > > > > master pipe is also a slave pipe in relation to primary master pipe. > > > > > > > > > > > > > > > } > > > > > > > > > > > > -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. > > > > > > > > Yeah, then joiner_slave_pipes would have to return only slave pipes in > > > > bigjoiner terminology. > > > > > > > > > > > > > > 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. > > > > > > > > I think it would be quite natural to call it a primary master. > > > > > > > > Initially BSpec called it that way and it sounds logical. > > > > > > > > I.e now we have not only master/slave hierarchy, but also > > > > second level of hierarchy between masters: secondary master > > > > and primary master. > > > > Other names sound less obvious tbh: i.e "master of masters" :) > > > > or "overall master" and etc.. > > > > > > > > That is why I'm a bit opposed to that Jani says to rename > > > > master/slave to primary/secondary - we get a problem with > > > > naming for Ultrajoiner then: > > > > as we are going to have primary of primary pipe or smth like that. > > > > > > > > Can't think of anything better than using primary/secondary master. > > > > If anyone has better sounding ideas - you are welcome. > > > > > > The bspec rename does this: > > > master -> primary > > > slave -> secondary > > > > > > I do agree that what you're going for here would have been pretty > > > natural way to experss this, but I think that ship sailed when > > > the annoying bspec rename happened. If we now start using those > > > same names to refer to a completely different concept I think the > > > end result will be extremely confusing. > > > > > > IMO we probably need to slighly extend the ultrajoiner and bigjoiners > > > terms to cover all the joiner cases, which would look something like this: > > > - ultrajoiner master/primary = the first pipe in the set > > > - ultrajoiner slaves/secondaries = the rest of the pipes in the set > > > - bigjoiner masters/primaries = first pipe + third pipe + ... in the set > > > - bigjoiner slaves/secondaries = second pipe + fourth pipe + ... in the set > > > > > > Or perhaps we just drop the "ultra" part form the first two and > > > speak of just "joiner" in general when referring to things on > > > that level? > > > > Then we won't be able to distinguish between primary master and secondary master. > > BSpec offers to call it Ultrajoiner master and Ultrajoiner secondary > > which is assuming to be Bigjoiner master, however still _quite_ confusing. > > We can call the primary master to be Ultrajoiner primary and refer to the rest > > of pipes like bigjoiner primary/secondary. > > > > To me it sounds so much more complex compared to primary/secondary master/slave > > to be honest.. Looks like we are inventing troubles for ourselves. > > Some people will just keep on complaining about the master/slave > terminology, and having to keep responding to that is very tiresome. > So we are going to do the rename to align to the new primary/secondary > bspec terminology, if only to avoid that annoyance. > > > So to avoid a bit of the confusion I think we can just use the > generic term "joiner" at the high level stuff. The first pipe > in the set will be the joiner_primary, everyone else is a > joiner_secondary. > > For the low level DSS_CTL register frobbing I think we > just need to be able to answer these simple questions: > - is ultrajoiner used? > - is bigjoiner used? > - is uncompressed joiner used? > - is this pipe an ultrajoiner primary? > - is this pipe a bigjoiner primary? > - is this pipe an uncompressed joiner primary? > > And beyond that we just need to be able to split > bigjoiner/uncompressed joiner primaries from > bigjoiner/uncompressed joiner secondaries for the > purposes of the modeset iterator macros. IMO for that > we can keep using the term "bigjoiner" even if it also > covers uncompressed joiner as well. > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 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); +} + +static u8 joiner_primary_master_pipes(const struct intel_crtc_state *crtc_state) +{ + return BIT(PIPE_A); } 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; } -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)); +} + +bool intel_crtc_is_bigjoiner_master(const struct intel_crtc_state *crtc_state) +{ + return !intel_crtc_is_bigjoiner_slave(crtc_state); +} + +bool intel_crtc_is_joiner_slave(const struct intel_crtc_state *crtc_state) +{ + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); + + if (intel_is_ultrajoiner(crtc_state)) + return crtc_state->joiner_pipes && + !(BIT(crtc->pipe) & joiner_primary_master_pipes(crtc_state)); + return intel_crtc_is_bigjoiner_slave(crtc_state); } bool intel_crtc_is_joiner_master(const struct intel_crtc_state *crtc_state) { struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); + if (intel_is_ultrajoiner(crtc_state)) + return crtc_state->joiner_pipes && + (BIT(crtc->pipe) & joiner_primary_master_pipes(crtc_state)); + return intel_crtc_is_bigjoiner_master(crtc_state); +} + +bool intel_crtc_is_joiner_primary_master(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_primary_master_pipes(crtc_state)); } static int intel_joiner_num_pipes(const struct intel_crtc_state *crtc_state) @@ -283,12 +315,26 @@ u8 intel_crtc_joined_pipe_mask(const struct intel_crtc_state *crtc_state) return BIT(crtc->pipe) | crtc_state->joiner_pipes; } +enum pipe intel_crtc_master_pipe(const struct intel_crtc_state *crtc_state) +{ + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); + + if (intel_is_ultrajoiner(crtc_state)) { + return ffs(joiner_primary_master_pipes(crtc_state)) - 1; + } else if (intel_is_bigjoiner(crtc_state)) { + return intel_crtc_is_joiner_slave(crtc_state) ? + crtc->pipe - 1 : crtc->pipe; + } else { + return crtc->pipe; + } +} + struct intel_crtc *intel_master_crtc(const struct intel_crtc_state *crtc_state) { struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev); if (intel_crtc_is_joiner_slave(crtc_state)) - return intel_crtc_for_pipe(i915, joiner_master_pipe(crtc_state)); + return intel_crtc_for_pipe(i915, intel_crtc_master_pipe(crtc_state)); else return to_intel_crtc(crtc_state->uapi.crtc); } @@ -2857,7 +2903,7 @@ static void intel_joiner_adjust_pipe_src(struct intel_crtc_state *crtc_state) if (num_pipes < 2) return; - master_pipe = joiner_master_pipe(crtc_state); + master_pipe = intel_crtc_master_pipe(crtc_state); width = drm_rect_width(&crtc_state->pipe_src); drm_rect_translate_to(&crtc_state->pipe_src, @@ -5898,7 +5944,7 @@ static int intel_atomic_check_joiner(struct intel_atomic_state *state, /* sanity check */ if (drm_WARN_ON(&i915->drm, - master_crtc->pipe != joiner_master_pipe(master_crtc_state))) + master_crtc->pipe != intel_crtc_master_pipe(master_crtc_state))) return -EINVAL; if (master_crtc_state->joiner_pipes & ~joiner_pipes(i915)) {
Lets implement or change basic functions required for ultrajoiner support from atomic commit/modesetting point of view. Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 66 +++++++++++++++++--- 1 file changed, 56 insertions(+), 10 deletions(-)