[v5,3/6] drm/i915/display/icl: HW state readout for transcoder port sync config

Commit Message

Navare, Manasi Sept. 27, 2019, 12:11 a.m. UTC

After the state is committed, we readout the HW registers and compare
the HW state with the SW state that we just committed.
For Transcdoer port sync, we add master_transcoder and the
salves bitmask to the crtc_state, hence we need to read those during
the HW state readout to avoid pipe state mismatch.

v6:
* Go through both parts of HW readout (Maarten)
* Add a WARN if the same trans configured as
master and slave (Ville, Maarten)
v5:
* Add return INVALID in defaut case (Maarten)
v4:
* Get power domains in master loop for get_config (Ville)
v3:
* Add TRANSCODER_D (Maarten)
* v3 Reviewed-by: Maarten Lankhorst <[e-mail]>
v2:
* Add Transcoder_D and MISSING_CASE (Maarten)

Cc: Ville Syrjälä <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Cc: Matt Roper <[e-mail]>
Cc: Jani Nikula <[e-mail]>
Signed-off-by: Manasi Navare <[e-mail]>
Reviewed-by: Maarten Lankhorst <[e-mail]>
---
 drivers/gpu/drm/i915/display/intel_display.c | 69 ++++++++++++++++++++
 1 file changed, 69 insertions(+)

Comments

Navare, Manasi Sept. 27, 2019, 9:04 p.m. UTC | #1

Ville, Maarten

In this patch, I added a WARN ON for the case where the same trans could be
configured as master and slave.

Does this look good?

Manasi 


On Thu, Sep 26, 2019 at 05:11:10PM -0700, Manasi Navare wrote:
> After the state is committed, we readout the HW registers and compare
> the HW state with the SW state that we just committed.
> For Transcdoer port sync, we add master_transcoder and the
> salves bitmask to the crtc_state, hence we need to read those during
> the HW state readout to avoid pipe state mismatch.
> 
> v6:
> * Go through both parts of HW readout (Maarten)
> * Add a WARN if the same trans configured as
> master and slave (Ville, Maarten)
> v5:
> * Add return INVALID in defaut case (Maarten)
> v4:
> * Get power domains in master loop for get_config (Ville)
> v3:
> * Add TRANSCODER_D (Maarten)
> * v3 Reviewed-by: Maarten Lankhorst <[e-mail]>
> v2:
> * Add Transcoder_D and MISSING_CASE (Maarten)
> 
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Matt Roper <[e-mail]>
> Cc: Jani Nikula <[e-mail]>
> Signed-off-by: Manasi Navare <[e-mail]>
> Reviewed-by: Maarten Lankhorst <[e-mail]>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 69 ++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index af6b8f10f132..6e4af6ded6f0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -10510,6 +10510,72 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>  	}
>  }
>  
> +static enum transcoder transcoder_master(struct drm_i915_private *dev_priv,
> +					 enum transcoder cpu_transcoder)
> +{
> +	u32 trans_port_sync, master_select;
> +
> +	trans_port_sync = I915_READ(TRANS_DDI_FUNC_CTL2(cpu_transcoder));
> +
> +	if ((trans_port_sync & PORT_SYNC_MODE_ENABLE) == 0)
> +		return INVALID_TRANSCODER;
> +
> +	master_select = trans_port_sync &
> +			PORT_SYNC_MODE_MASTER_SELECT_MASK;
> +	switch (master_select) {
> +	case 1:
> +		return TRANSCODER_A;
> +	case 2:
> +		return TRANSCODER_B;
> +	case 3:
> +		return TRANSCODER_C;
> +	case 4:
> +		return TRANSCODER_D;
> +	default:
> +		MISSING_CASE(master_select);
> +		return INVALID_TRANSCODER;
> +	}
> +}
> +
> +static void icelake_get_trans_port_sync_config(struct intel_crtc *crtc,
> +					       struct intel_crtc_state *pipe_config)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	u32 transcoders;
> +	enum transcoder cpu_transcoder;
> +
> +	pipe_config->master_transcoder = transcoder_master(dev_priv,
> +							   pipe_config->cpu_transcoder);
> +	if (pipe_config->master_transcoder != INVALID_TRANSCODER)
> +		pipe_config->sync_mode_slaves_mask = 0;
> +
> +	transcoders = BIT(TRANSCODER_A) |
> +		BIT(TRANSCODER_B) |
> +		BIT(TRANSCODER_C) |
> +		BIT(TRANSCODER_D);
> +	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> +		enum intel_display_power_domain power_domain;
> +		intel_wakeref_t trans_wakeref;
> +
> +		power_domain = POWER_DOMAIN_TRANSCODER(cpu_transcoder);
> +		trans_wakeref = intel_display_power_get_if_enabled(dev_priv,
> +								   power_domain);
> +
> +		if (!trans_wakeref)
> +			continue;
> +
> +		if (transcoder_master(dev_priv, cpu_transcoder) ==
> +		    pipe_config->cpu_transcoder)
> +			pipe_config->sync_mode_slaves_mask |= BIT(cpu_transcoder);
> +
> +		intel_display_power_put(dev_priv, power_domain, trans_wakeref);
> +	}
> +
> +	WARN_ON(pipe_config->master_transcoder != INVALID_TRANSCODER &&
> +		pipe_config->sync_mode_slaves_mask);
> +}
> +
>  static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  				    struct intel_crtc_state *pipe_config)
>  {
> @@ -10629,6 +10695,9 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  		pipe_config->pixel_multiplier = 1;
>  	}
>  
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		icelake_get_trans_port_sync_config(crtc, pipe_config);
> +
>  out:
>  	for_each_power_domain(power_domain, power_domain_mask)
>  		intel_display_power_put(dev_priv,
> -- 
> 2.19.1
>

Ville Syrjala Sept. 30, 2019, 2:21 p.m. UTC | #2

On Thu, Sep 26, 2019 at 05:11:10PM -0700, Manasi Navare wrote:
> After the state is committed, we readout the HW registers and compare
> the HW state with the SW state that we just committed.
> For Transcdoer port sync, we add master_transcoder and the
> salves bitmask to the crtc_state, hence we need to read those during
> the HW state readout to avoid pipe state mismatch.
> 
> v6:
> * Go through both parts of HW readout (Maarten)
> * Add a WARN if the same trans configured as
> master and slave (Ville, Maarten)
> v5:
> * Add return INVALID in defaut case (Maarten)
> v4:
> * Get power domains in master loop for get_config (Ville)
> v3:
> * Add TRANSCODER_D (Maarten)
> * v3 Reviewed-by: Maarten Lankhorst <[e-mail]>
> v2:
> * Add Transcoder_D and MISSING_CASE (Maarten)
> 
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Matt Roper <[e-mail]>
> Cc: Jani Nikula <[e-mail]>
> Signed-off-by: Manasi Navare <[e-mail]>
> Reviewed-by: Maarten Lankhorst <[e-mail]>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 69 ++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index af6b8f10f132..6e4af6ded6f0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -10510,6 +10510,72 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>  	}
>  }
>  
> +static enum transcoder transcoder_master(struct drm_i915_private *dev_priv,
> +					 enum transcoder cpu_transcoder)
> +{
> +	u32 trans_port_sync, master_select;
> +
> +	trans_port_sync = I915_READ(TRANS_DDI_FUNC_CTL2(cpu_transcoder));
> +
> +	if ((trans_port_sync & PORT_SYNC_MODE_ENABLE) == 0)
> +		return INVALID_TRANSCODER;
> +
> +	master_select = trans_port_sync &
> +			PORT_SYNC_MODE_MASTER_SELECT_MASK;
> +	switch (master_select) {
> +	case 1:
> +		return TRANSCODER_A;
> +	case 2:
> +		return TRANSCODER_B;
> +	case 3:
> +		return TRANSCODER_C;
> +	case 4:
> +		return TRANSCODER_D;

Missing EDP. Also A-D are just master_select-1

> +	default:
> +		MISSING_CASE(master_select);
> +		return INVALID_TRANSCODER;
> +	}
> +}
> +
> +static void icelake_get_trans_port_sync_config(struct intel_crtc *crtc,
> +					       struct intel_crtc_state *pipe_config)

s/pipe_config/crtc_state/

'crtc' argument can be derived so doesn't need to be passed in.

> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	u32 transcoders;
> +	enum transcoder cpu_transcoder;
> +
> +	pipe_config->master_transcoder = transcoder_master(dev_priv,
> +							   pipe_config->cpu_transcoder);
> +	if (pipe_config->master_transcoder != INVALID_TRANSCODER)
> +		pipe_config->sync_mode_slaves_mask = 0;

Zeroing seems redundant.

> +
> +	transcoders = BIT(TRANSCODER_A) |
> +		BIT(TRANSCODER_B) |
> +		BIT(TRANSCODER_C) |
> +		BIT(TRANSCODER_D);
> +	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> +		enum intel_display_power_domain power_domain;
> +		intel_wakeref_t trans_wakeref;
> +
> +		power_domain = POWER_DOMAIN_TRANSCODER(cpu_transcoder);
> +		trans_wakeref = intel_display_power_get_if_enabled(dev_priv,
> +								   power_domain);
> +
> +		if (!trans_wakeref)
> +			continue;
> +
> +		if (transcoder_master(dev_priv, cpu_transcoder) ==
> +		    pipe_config->cpu_transcoder)
> +			pipe_config->sync_mode_slaves_mask |= BIT(cpu_transcoder);
> +
> +		intel_display_power_put(dev_priv, power_domain, trans_wakeref);
> +	}
> +
> +	WARN_ON(pipe_config->master_transcoder != INVALID_TRANSCODER &&
> +		pipe_config->sync_mode_slaves_mask);
> +}
> +
>  static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  				    struct intel_crtc_state *pipe_config)
>  {
> @@ -10629,6 +10695,9 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  		pipe_config->pixel_multiplier = 1;
>  	}
>  
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		icelake_get_trans_port_sync_config(crtc, pipe_config);
> +
>  out:
>  	for_each_power_domain(power_domain, power_domain_mask)
>  		intel_display_power_put(dev_priv,
> -- 
> 2.19.1

Lucas De Marchi Sept. 30, 2019, 7:45 p.m. UTC | #3

On Thu, Sep 26, 2019 at 05:11:10PM -0700, Manasi Navare wrote:
>After the state is committed, we readout the HW registers and compare
>the HW state with the SW state that we just committed.
>For Transcdoer port sync, we add master_transcoder and the
>salves bitmask to the crtc_state, hence we need to read those during
>the HW state readout to avoid pipe state mismatch.
>
>v6:
>* Go through both parts of HW readout (Maarten)
>* Add a WARN if the same trans configured as
>master and slave (Ville, Maarten)
>v5:
>* Add return INVALID in defaut case (Maarten)
>v4:
>* Get power domains in master loop for get_config (Ville)
>v3:
>* Add TRANSCODER_D (Maarten)
>* v3 Reviewed-by: Maarten Lankhorst <[e-mail]>
>v2:
>* Add Transcoder_D and MISSING_CASE (Maarten)
>
>Cc: Ville Syrjälä <[e-mail]>
>Cc: Maarten Lankhorst <[e-mail]>
>Cc: Matt Roper <[e-mail]>
>Cc: Jani Nikula <[e-mail]>
>Signed-off-by: Manasi Navare <[e-mail]>
>Reviewed-by: Maarten Lankhorst <[e-mail]>
>---
> drivers/gpu/drm/i915/display/intel_display.c | 69 ++++++++++++++++++++
> 1 file changed, 69 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>index af6b8f10f132..6e4af6ded6f0 100644
>--- a/drivers/gpu/drm/i915/display/intel_display.c
>+++ b/drivers/gpu/drm/i915/display/intel_display.c
>@@ -10510,6 +10510,72 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
> 	}
> }
>
>+static enum transcoder transcoder_master(struct drm_i915_private *dev_priv,
>+					 enum transcoder cpu_transcoder)

I find this function name misleading since there's no indication it's
actually reading the HW.

Maybe even inline this in the only caller? Or would a `_readout` suffix
make sense?

>+{
>+	u32 trans_port_sync, master_select;
>+
>+	trans_port_sync = I915_READ(TRANS_DDI_FUNC_CTL2(cpu_transcoder));
>+
>+	if ((trans_port_sync & PORT_SYNC_MODE_ENABLE) == 0)
>+		return INVALID_TRANSCODER;
>+
>+	master_select = trans_port_sync &
>+			PORT_SYNC_MODE_MASTER_SELECT_MASK;
>+	switch (master_select) {
>+	case 1:
>+		return TRANSCODER_A;
>+	case 2:
>+		return TRANSCODER_B;
>+	case 3:
>+		return TRANSCODER_C;
>+	case 4:
>+		return TRANSCODER_D;
>+	default:
>+		MISSING_CASE(master_select);
>+		return INVALID_TRANSCODER;
>+	}
>+}
>+
>+static void icelake_get_trans_port_sync_config(struct intel_crtc *crtc,
>+					       struct intel_crtc_state *pipe_config)

>+{
>+	struct drm_device *dev = crtc->base.dev;
>+	struct drm_i915_private *dev_priv = to_i915(dev);
>+	u32 transcoders;
>+	enum transcoder cpu_transcoder;
>+
>+	pipe_config->master_transcoder = transcoder_master(dev_priv,
>+							   pipe_config->cpu_transcoder);
>+	if (pipe_config->master_transcoder != INVALID_TRANSCODER)
>+		pipe_config->sync_mode_slaves_mask = 0;
>+
>+	transcoders = BIT(TRANSCODER_A) |
>+		BIT(TRANSCODER_B) |
>+		BIT(TRANSCODER_C) |
>+		BIT(TRANSCODER_D);
>+	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
>+		enum intel_display_power_domain power_domain;
>+		intel_wakeref_t trans_wakeref;
>+
>+		power_domain = POWER_DOMAIN_TRANSCODER(cpu_transcoder);
>+		trans_wakeref = intel_display_power_get_if_enabled(dev_priv,
>+								   power_domain);
>+
>+		if (!trans_wakeref)
>+			continue;
>+
>+		if (transcoder_master(dev_priv, cpu_transcoder) ==
>+		    pipe_config->cpu_transcoder)
>+			pipe_config->sync_mode_slaves_mask |= BIT(cpu_transcoder);
>+
>+		intel_display_power_put(dev_priv, power_domain, trans_wakeref);
>+	}
>+
>+	WARN_ON(pipe_config->master_transcoder != INVALID_TRANSCODER &&
>+		pipe_config->sync_mode_slaves_mask);
>+}
>+
> static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> 				    struct intel_crtc_state *pipe_config)
> {
>@@ -10629,6 +10695,9 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> 		pipe_config->pixel_multiplier = 1;
> 	}
>
>+	if (INTEL_GEN(dev_priv) >= 11)
>+		icelake_get_trans_port_sync_config(crtc, pipe_config);

Three letters prefix for functions is much more common, so I'd stick
with that. For Ice Lake for example there's only icelake_get_ddi_pll()
as opposed to tons of icl_*() functions.


>+
> out:
> 	for_each_power_domain(power_domain, power_domain_mask)
> 		intel_display_power_put(dev_priv,
>-- 
>2.19.1
>
>_______________________________________________
>Intel-gfx mailing list
>[e-mail]
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Navare, Manasi Oct. 7, 2019, 3:31 a.m. UTC | #4

On Mon, Sep 30, 2019 at 05:21:08PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 26, 2019 at 05:11:10PM -0700, Manasi Navare wrote:
> > After the state is committed, we readout the HW registers and compare
> > the HW state with the SW state that we just committed.
> > For Transcdoer port sync, we add master_transcoder and the
> > salves bitmask to the crtc_state, hence we need to read those during
> > the HW state readout to avoid pipe state mismatch.
> > 
> > v6:
> > * Go through both parts of HW readout (Maarten)
> > * Add a WARN if the same trans configured as
> > master and slave (Ville, Maarten)
> > v5:
> > * Add return INVALID in defaut case (Maarten)
> > v4:
> > * Get power domains in master loop for get_config (Ville)
> > v3:
> > * Add TRANSCODER_D (Maarten)
> > * v3 Reviewed-by: Maarten Lankhorst <[e-mail]>
> > v2:
> > * Add Transcoder_D and MISSING_CASE (Maarten)
> > 
> > Cc: Ville Syrjälä <[e-mail]>
> > Cc: Maarten Lankhorst <[e-mail]>
> > Cc: Matt Roper <[e-mail]>
> > Cc: Jani Nikula <[e-mail]>
> > Signed-off-by: Manasi Navare <[e-mail]>
> > Reviewed-by: Maarten Lankhorst <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 69 ++++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index af6b8f10f132..6e4af6ded6f0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -10510,6 +10510,72 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
> >  	}
> >  }
> >  
> > +static enum transcoder transcoder_master(struct drm_i915_private *dev_priv,
> > +					 enum transcoder cpu_transcoder)
> > +{
> > +	u32 trans_port_sync, master_select;
> > +
> > +	trans_port_sync = I915_READ(TRANS_DDI_FUNC_CTL2(cpu_transcoder));
> > +
> > +	if ((trans_port_sync & PORT_SYNC_MODE_ENABLE) == 0)
> > +		return INVALID_TRANSCODER;
> > +
> > +	master_select = trans_port_sync &
> > +			PORT_SYNC_MODE_MASTER_SELECT_MASK;
> > +	switch (master_select) {
> > +	case 1:
> > +		return TRANSCODER_A;
> > +	case 2:
> > +		return TRANSCODER_B;
> > +	case 3:
> > +		return TRANSCODER_C;
> > +	case 4:
> > +		return TRANSCODER_D;
> 
> Missing EDP. Also A-D are just master_select-1

Yes will just derive it as master_Select -1 and for 0, i will
set it to EDP
That should work right?

> 
> > +	default:
> > +		MISSING_CASE(master_select);
> > +		return INVALID_TRANSCODER;
> > +	}
> > +}
> > +
> > +static void icelake_get_trans_port_sync_config(struct intel_crtc *crtc,
> > +					       struct intel_crtc_state *pipe_config)
> 
> s/pipe_config/crtc_state/
>

Ok
 
> 'crtc' argument can be derived so doesn't need to be passed in.
>

Ok will remove that
 
> > +{
> > +	struct drm_device *dev = crtc->base.dev;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	u32 transcoders;
> > +	enum transcoder cpu_transcoder;
> > +
> > +	pipe_config->master_transcoder = transcoder_master(dev_priv,
> > +							   pipe_config->cpu_transcoder);
> > +	if (pipe_config->master_transcoder != INVALID_TRANSCODER)
> > +		pipe_config->sync_mode_slaves_mask = 0;
> 
> Zeroing seems redundant.

Just for safer side since we want the bitmask to be 0 for slaves
But I guess it will be initialized to 0 anyways right so no need to set it to 0?

MAnasi


> 
> > +
> > +	transcoders = BIT(TRANSCODER_A) |
> > +		BIT(TRANSCODER_B) |
> > +		BIT(TRANSCODER_C) |
> > +		BIT(TRANSCODER_D);
> > +	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> > +		enum intel_display_power_domain power_domain;
> > +		intel_wakeref_t trans_wakeref;
> > +
> > +		power_domain = POWER_DOMAIN_TRANSCODER(cpu_transcoder);
> > +		trans_wakeref = intel_display_power_get_if_enabled(dev_priv,
> > +								   power_domain);
> > +
> > +		if (!trans_wakeref)
> > +			continue;
> > +
> > +		if (transcoder_master(dev_priv, cpu_transcoder) ==
> > +		    pipe_config->cpu_transcoder)
> > +			pipe_config->sync_mode_slaves_mask |= BIT(cpu_transcoder);
> > +
> > +		intel_display_power_put(dev_priv, power_domain, trans_wakeref);
> > +	}
> > +
> > +	WARN_ON(pipe_config->master_transcoder != INVALID_TRANSCODER &&
> > +		pipe_config->sync_mode_slaves_mask);
> > +}
> > +
> >  static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> >  				    struct intel_crtc_state *pipe_config)
> >  {
> > @@ -10629,6 +10695,9 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> >  		pipe_config->pixel_multiplier = 1;
> >  	}
> >  
> > +	if (INTEL_GEN(dev_priv) >= 11)
> > +		icelake_get_trans_port_sync_config(crtc, pipe_config);
> > +
> >  out:
> >  	for_each_power_domain(power_domain, power_domain_mask)
> >  		intel_display_power_put(dev_priv,
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel

Navare, Manasi Oct. 7, 2019, 3:33 a.m. UTC | #5

On Mon, Sep 30, 2019 at 12:45:39PM -0700, Lucas De Marchi wrote:
> On Thu, Sep 26, 2019 at 05:11:10PM -0700, Manasi Navare wrote:
> >After the state is committed, we readout the HW registers and compare
> >the HW state with the SW state that we just committed.
> >For Transcdoer port sync, we add master_transcoder and the
> >salves bitmask to the crtc_state, hence we need to read those during
> >the HW state readout to avoid pipe state mismatch.
> >
> >v6:
> >* Go through both parts of HW readout (Maarten)
> >* Add a WARN if the same trans configured as
> >master and slave (Ville, Maarten)
> >v5:
> >* Add return INVALID in defaut case (Maarten)
> >v4:
> >* Get power domains in master loop for get_config (Ville)
> >v3:
> >* Add TRANSCODER_D (Maarten)
> >* v3 Reviewed-by: Maarten Lankhorst <[e-mail]>
> >v2:
> >* Add Transcoder_D and MISSING_CASE (Maarten)
> >
> >Cc: Ville Syrjälä <[e-mail]>
> >Cc: Maarten Lankhorst <[e-mail]>
> >Cc: Matt Roper <[e-mail]>
> >Cc: Jani Nikula <[e-mail]>
> >Signed-off-by: Manasi Navare <[e-mail]>
> >Reviewed-by: Maarten Lankhorst <[e-mail]>
> >---
> >drivers/gpu/drm/i915/display/intel_display.c | 69 ++++++++++++++++++++
> >1 file changed, 69 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >index af6b8f10f132..6e4af6ded6f0 100644
> >--- a/drivers/gpu/drm/i915/display/intel_display.c
> >+++ b/drivers/gpu/drm/i915/display/intel_display.c
> >@@ -10510,6 +10510,72 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
> >	}
> >}
> >
> >+static enum transcoder transcoder_master(struct drm_i915_private *dev_priv,
> >+					 enum transcoder cpu_transcoder)
> 
> I find this function name misleading since there's no indication it's
> actually reading the HW.
> 
> Maybe even inline this in the only caller? Or would a `_readout` suffix
> make sense?

I think renaming it with a _readout suffix will be good, i will do that

Manasi

> 
> >+{
> >+	u32 trans_port_sync, master_select;
> >+
> >+	trans_port_sync = I915_READ(TRANS_DDI_FUNC_CTL2(cpu_transcoder));
> >+
> >+	if ((trans_port_sync & PORT_SYNC_MODE_ENABLE) == 0)
> >+		return INVALID_TRANSCODER;
> >+
> >+	master_select = trans_port_sync &
> >+			PORT_SYNC_MODE_MASTER_SELECT_MASK;
> >+	switch (master_select) {
> >+	case 1:
> >+		return TRANSCODER_A;
> >+	case 2:
> >+		return TRANSCODER_B;
> >+	case 3:
> >+		return TRANSCODER_C;
> >+	case 4:
> >+		return TRANSCODER_D;
> >+	default:
> >+		MISSING_CASE(master_select);
> >+		return INVALID_TRANSCODER;
> >+	}
> >+}
> >+
> >+static void icelake_get_trans_port_sync_config(struct intel_crtc *crtc,
> >+					       struct intel_crtc_state *pipe_config)
> 
> >+{
> >+	struct drm_device *dev = crtc->base.dev;
> >+	struct drm_i915_private *dev_priv = to_i915(dev);
> >+	u32 transcoders;
> >+	enum transcoder cpu_transcoder;
> >+
> >+	pipe_config->master_transcoder = transcoder_master(dev_priv,
> >+							   pipe_config->cpu_transcoder);
> >+	if (pipe_config->master_transcoder != INVALID_TRANSCODER)
> >+		pipe_config->sync_mode_slaves_mask = 0;
> >+
> >+	transcoders = BIT(TRANSCODER_A) |
> >+		BIT(TRANSCODER_B) |
> >+		BIT(TRANSCODER_C) |
> >+		BIT(TRANSCODER_D);
> >+	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> >+		enum intel_display_power_domain power_domain;
> >+		intel_wakeref_t trans_wakeref;
> >+
> >+		power_domain = POWER_DOMAIN_TRANSCODER(cpu_transcoder);
> >+		trans_wakeref = intel_display_power_get_if_enabled(dev_priv,
> >+								   power_domain);
> >+
> >+		if (!trans_wakeref)
> >+			continue;
> >+
> >+		if (transcoder_master(dev_priv, cpu_transcoder) ==
> >+		    pipe_config->cpu_transcoder)
> >+			pipe_config->sync_mode_slaves_mask |= BIT(cpu_transcoder);
> >+
> >+		intel_display_power_put(dev_priv, power_domain, trans_wakeref);
> >+	}
> >+
> >+	WARN_ON(pipe_config->master_transcoder != INVALID_TRANSCODER &&
> >+		pipe_config->sync_mode_slaves_mask);
> >+}
> >+
> >static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> >				    struct intel_crtc_state *pipe_config)
> >{
> >@@ -10629,6 +10695,9 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> >		pipe_config->pixel_multiplier = 1;
> >	}
> >
> >+	if (INTEL_GEN(dev_priv) >= 11)
> >+		icelake_get_trans_port_sync_config(crtc, pipe_config);
> 
> Three letters prefix for functions is much more common, so I'd stick
> with that. For Ice Lake for example there's only icelake_get_ddi_pll()
> as opposed to tons of icl_*() functions.
> 
> 
> >+
> >out:
> >	for_each_power_domain(power_domain, power_domain_mask)
> >		intel_display_power_put(dev_priv,
> >-- 
> >2.19.1
> >
> >_______________________________________________
> >Intel-gfx mailing list
> >[e-mail]
> >https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index af6b8f10f132..6e4af6ded6f0 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -10510,6 +10510,72 @@  static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
 	}
 }
 
+static enum transcoder transcoder_master(struct drm_i915_private *dev_priv,
+					 enum transcoder cpu_transcoder)
+{
+	u32 trans_port_sync, master_select;
+
+	trans_port_sync = I915_READ(TRANS_DDI_FUNC_CTL2(cpu_transcoder));
+
+	if ((trans_port_sync & PORT_SYNC_MODE_ENABLE) == 0)
+		return INVALID_TRANSCODER;
+
+	master_select = trans_port_sync &
+			PORT_SYNC_MODE_MASTER_SELECT_MASK;
+	switch (master_select) {
+	case 1:
+		return TRANSCODER_A;
+	case 2:
+		return TRANSCODER_B;
+	case 3:
+		return TRANSCODER_C;
+	case 4:
+		return TRANSCODER_D;
+	default:
+		MISSING_CASE(master_select);
+		return INVALID_TRANSCODER;
+	}
+}
+
+static void icelake_get_trans_port_sync_config(struct intel_crtc *crtc,
+					       struct intel_crtc_state *pipe_config)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	u32 transcoders;
+	enum transcoder cpu_transcoder;
+
+	pipe_config->master_transcoder = transcoder_master(dev_priv,
+							   pipe_config->cpu_transcoder);
+	if (pipe_config->master_transcoder != INVALID_TRANSCODER)
+		pipe_config->sync_mode_slaves_mask = 0;
+
+	transcoders = BIT(TRANSCODER_A) |
+		BIT(TRANSCODER_B) |
+		BIT(TRANSCODER_C) |
+		BIT(TRANSCODER_D);
+	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
+		enum intel_display_power_domain power_domain;
+		intel_wakeref_t trans_wakeref;
+
+		power_domain = POWER_DOMAIN_TRANSCODER(cpu_transcoder);
+		trans_wakeref = intel_display_power_get_if_enabled(dev_priv,
+								   power_domain);
+
+		if (!trans_wakeref)
+			continue;
+
+		if (transcoder_master(dev_priv, cpu_transcoder) ==
+		    pipe_config->cpu_transcoder)
+			pipe_config->sync_mode_slaves_mask |= BIT(cpu_transcoder);
+
+		intel_display_power_put(dev_priv, power_domain, trans_wakeref);
+	}
+
+	WARN_ON(pipe_config->master_transcoder != INVALID_TRANSCODER &&
+		pipe_config->sync_mode_slaves_mask);
+}
+
 static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 				    struct intel_crtc_state *pipe_config)
 {
@@ -10629,6 +10695,9 @@  static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 		pipe_config->pixel_multiplier = 1;
 	}
 
+	if (INTEL_GEN(dev_priv) >= 11)
+		icelake_get_trans_port_sync_config(crtc, pipe_config);
+
 out:
 	for_each_power_domain(power_domain, power_domain_mask)
 		intel_display_power_put(dev_priv,