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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

-- 
Ville Syrjälä
Intel