[Intel-gfx] [PATCH v5 3/6] drm/i915/display/icl: HW state readout for transcoder port sync config

[Lucas De Marchi]

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 <maarten.lankhorst at linux.intel.com>
>v2:
>* Add Transcoder_D and MISSING_CASE (Maarten)
>
>Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>Cc: Matt Roper <matthew.d.roper at intel.com>
>Cc: Jani Nikula <jani.nikula at intel.com>
>Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
>Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>---
> 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
>Intel-gfx at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx