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

[Lisovskiy, Stanislav]

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 at 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