Re: [PATCH 2/5] drm/i915: Implement basic functions for ultrajoiner support

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

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