Re: [PATCH 1/2] drm/i915: Add some essential functionality for joiners

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

On Tue, Sep 17, 2024 at 01:53:57PM +0530, Ankit Nautiyal wrote:
> From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> 
> In most of the cases we now try to avoid mentioning things like
> "bigjoiner" or "ultrajoiner" trying to unify the API and refer
> mostly to all this functionality as "joiner".
> In majority cases that should be way to go.
> However in some cases we still need to distinguish between
> bigjoiner primaries and secondaries(such as DSC register programming).
> 
> Create correspondent helper functions and start using them,
> in order be prepared for adding ultrajoiner functionality.
> 
> v2: Fixed checkpatch warnings (Ankit)
> v3: Introduce ultrajoiner helpers in next patch.
> v4: Streamline the helpers and add few more. (Ville)
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 41 ++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.h |  2 +
>  drivers/gpu/drm/i915/display/intel_vdsc.c    |  4 +-
>  3 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index d4a371edfcdd..deacdf82e143 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -254,6 +254,47 @@ static enum pipe joiner_primary_pipe(const struct intel_crtc_state *crtc_state)
>  	return ffs(crtc_state->joiner_pipes) - 1;
>  }
>  
> +static bool is_bigjoiner(const struct intel_crtc_state *crtc_state)
> +{
> +	return hweight8(crtc_state->joiner_pipes) >= 2;
> +}
> +
> +static u8 bigjoiner_primary_pipes(const struct intel_crtc_state *crtc_state)
> +{
> +	if (!is_bigjoiner(crtc_state))
> +		return 0;
> +
> +	return crtc_state->joiner_pipes & (0b01010101 << joiner_primary_pipe(crtc_state));
> +}
> +
> +static unsigned int bigjoiner_secondary_pipes(const struct intel_crtc_state *crtc_state)
> +{
> +	if (!is_bigjoiner(crtc_state))
> +		return 0;
> +
> +	return crtc_state->joiner_pipes & (0b10101010 << joiner_primary_pipe(crtc_state));
> +}
> +
> +bool intel_crtc_is_bigjoiner_primary(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +
> +	if (!is_bigjoiner(crtc_state))
> +		return false;
> +
> +	return BIT(crtc->pipe) & bigjoiner_primary_pipes(crtc_state);
> +}
> +
> +bool intel_crtc_is_bigjoiner_secondary(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +
> +	if (!is_bigjoiner(crtc_state))
> +		return false;
> +
> +	return BIT(crtc->pipe) & bigjoiner_secondary_pipes(crtc_state);
> +}

We probably want a comment around these somewhere indicating that
(despite the name) these apply to both bigjoiner and uncompressed
joiner.

Unless someone can come up with a better term to use instead of
"bigjoiner" in these? I'm drawing a blank myself.

Anyways, with some clarification added this is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  u8 intel_crtc_joiner_secondary_pipes(const struct intel_crtc_state *crtc_state)
>  {
>  	if (crtc_state->joiner_pipes)
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index 7ca26e5cb20e..894f58ead279 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -426,6 +426,8 @@ bool is_trans_port_sync_master(const struct intel_crtc_state *state);
>  u8 intel_crtc_joined_pipe_mask(const struct intel_crtc_state *crtc_state);
>  bool intel_crtc_is_joiner_secondary(const struct intel_crtc_state *crtc_state);
>  bool intel_crtc_is_joiner_primary(const struct intel_crtc_state *crtc_state);
> +bool intel_crtc_is_bigjoiner_primary(const struct intel_crtc_state *crtc_state);
> +bool intel_crtc_is_bigjoiner_secondary(const struct intel_crtc_state *crtc_state);
>  u8 intel_crtc_joiner_secondary_pipes(const struct intel_crtc_state *crtc_state);
>  struct intel_crtc *intel_primary_crtc(const struct intel_crtc_state *crtc_state);
>  bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index 2e849b015e74..8158e3702ed5 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -742,7 +742,7 @@ void intel_uncompressed_joiner_enable(const struct intel_crtc_state *crtc_state)
>  	u32 dss_ctl1_val = 0;
>  
>  	if (crtc_state->joiner_pipes && !crtc_state->dsc.compression_enable) {
> -		if (intel_crtc_is_joiner_secondary(crtc_state))
> +		if (intel_crtc_is_bigjoiner_secondary(crtc_state))
>  			dss_ctl1_val |= UNCOMPRESSED_JOINER_SECONDARY;
>  		else
>  			dss_ctl1_val |= UNCOMPRESSED_JOINER_PRIMARY;
> @@ -771,7 +771,7 @@ void intel_dsc_enable(const struct intel_crtc_state *crtc_state)
>  	}
>  	if (crtc_state->joiner_pipes) {
>  		dss_ctl1_val |= BIG_JOINER_ENABLE;
> -		if (!intel_crtc_is_joiner_secondary(crtc_state))
> +		if (intel_crtc_is_bigjoiner_primary(crtc_state))
>  			dss_ctl1_val |= PRIMARY_BIG_JOINER_ENABLE;
>  	}
>  	intel_de_write(dev_priv, dss_ctl1_reg(crtc, crtc_state->cpu_transcoder), dss_ctl1_val);
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel