| Message ID | 20190922170807.12436-3-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/6] drm/i915/display/icl: Save Master transcoder in slave's crtc_state for Transcoder Port Sync | expand |
Hi Ville, This gave me clean CI results with adding the power well get/put like you suggested, could you please review this patch? Regards Manasi On Sun, Sep 22, 2019 at 10:08:04AM -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. > > v4: > * Get power domains in master loop for get_config (Ville) > v3: > * Add TRANSCODER_D (Maarten) > * v3 Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > v2: > * Add Transcoder_D and MISSING_CASE (Maarten) > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@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 1ae5eafe2892..711987eb4e9e 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -10470,6 +10470,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; > + return; > + } > + > + 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); > + } > +} > + > static bool haswell_get_pipe_config(struct intel_crtc *crtc, > struct intel_crtc_state *pipe_config) > { > @@ -10566,6 +10632,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 >
Op 22-09-2019 om 19:08 schreef Manasi Navare: > 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. > > v4: > * Get power domains in master loop for get_config (Ville) > v3: > * Add TRANSCODER_D (Maarten) > * v3 Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > v2: > * Add Transcoder_D and MISSING_CASE (Maarten) > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@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 1ae5eafe2892..711987eb4e9e 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -10470,6 +10470,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; Could move this return up to default. :) > +} > + > +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; > + return; > + } > + It could still be useful to go through the loop below anyway, in case we messed up. We are reading out from hw after all. And then also add this as a PIPE_CONF_CHECK_X check to pipe_config_compare(). With that fixed and CI happy, Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > + 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); > + } > +} > + > static bool haswell_get_pipe_config(struct intel_crtc *crtc, > struct intel_crtc_state *pipe_config) > { > @@ -10566,6 +10632,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,
On Tue, Sep 24, 2019 at 05:38:00PM +0200, Maarten Lankhorst wrote: > Op 22-09-2019 om 19:08 schreef Manasi Navare: > > 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. > > > > v4: > > * Get power domains in master loop for get_config (Ville) > > v3: > > * Add TRANSCODER_D (Maarten) > > * v3 Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > v2: > > * Add Transcoder_D and MISSING_CASE (Maarten) > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@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 1ae5eafe2892..711987eb4e9e 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -10470,6 +10470,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; > Could move this return up to default. :) Yes will do this > > +} > > + > > +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; > > + return; > > + } > > + > > It could still be useful to go through the loop below anyway, in case we messed up. We are reading out from hw after all. > The loop below will be called always in case of the HW state readout for master, in case of the slave it will execute the firs part, get the master transcoder and return, why do we need to call the loop below for slave? Why cant we just return here as in the code? > And then also add this as a PIPE_CONF_CHECK_X check to pipe_config_compare(). > This is already added in pipe_config_compare() in the patch that adds these two master_trans and slave_bitmask to the crtc state Manasi > With that fixed and CI happy, > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > + 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); > > + } > > +} > > + > > static bool haswell_get_pipe_config(struct intel_crtc *crtc, > > struct intel_crtc_state *pipe_config) > > { > > @@ -10566,6 +10632,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, > >
On Tue, Sep 24, 2019 at 10:59:57AM -0700, Manasi Navare wrote: > On Tue, Sep 24, 2019 at 05:38:00PM +0200, Maarten Lankhorst wrote: > > Op 22-09-2019 om 19:08 schreef Manasi Navare: > > > 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. > > > > > > v4: > > > * Get power domains in master loop for get_config (Ville) > > > v3: > > > * Add TRANSCODER_D (Maarten) > > > * v3 Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > v2: > > > * Add Transcoder_D and MISSING_CASE (Maarten) > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Signed-off-by: Manasi Navare <manasi.d.navare@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 1ae5eafe2892..711987eb4e9e 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -10470,6 +10470,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; > > Could move this return up to default. :) > > Yes will do this > > > > +} > > > + > > > +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; > > > + return; > > > + } > > > + > > > > It could still be useful to go through the loop below anyway, in case we messed up. We are reading out from hw after all. > > > > The loop below will be called always in case of the HW state readout for master, in case of the slave it will execute > the firs part, get the master transcoder and return, why do we need to call the loop below for slave? Why cant we just return here > as in the code? I think Maarten's point was to catch cases where the same transcoder is accidentally configure as both slave and master. > > > And then also add this as a PIPE_CONF_CHECK_X check to pipe_config_compare(). > > > > This is already added in pipe_config_compare() in the patch that adds these two master_trans and slave_bitmask to the crtc state > > Manasi > > > With that fixed and CI happy, > > > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > + 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); > > > + } > > > +} > > > + > > > static bool haswell_get_pipe_config(struct intel_crtc *crtc, > > > struct intel_crtc_state *pipe_config) > > > { > > > @@ -10566,6 +10632,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, > > > >
On Wed, Sep 25, 2019 at 01:08:23PM +0300, Ville Syrjälä wrote: > On Tue, Sep 24, 2019 at 10:59:57AM -0700, Manasi Navare wrote: > > On Tue, Sep 24, 2019 at 05:38:00PM +0200, Maarten Lankhorst wrote: > > > Op 22-09-2019 om 19:08 schreef Manasi Navare: > > > > 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. > > > > > > > > v4: > > > > * Get power domains in master loop for get_config (Ville) > > > > v3: > > > > * Add TRANSCODER_D (Maarten) > > > > * v3 Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > v2: > > > > * Add Transcoder_D and MISSING_CASE (Maarten) > > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@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 1ae5eafe2892..711987eb4e9e 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > @@ -10470,6 +10470,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; > > > Could move this return up to default. :) > > > > Yes will do this > > > > > > +} > > > > + > > > > +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; > > > > + return; > > > > + } > > > > + > > > > > > It could still be useful to go through the loop below anyway, in case we messed up. We are reading out from hw after all. > > > > > > > The loop below will be called always in case of the HW state readout for master, in case of the slave it will execute > > the firs part, get the master transcoder and return, why do we need to call the loop below for slave? Why cant we just return here > > as in the code? > > I think Maarten's point was to catch cases where the same transcoder is > accidentally configure as both slave and master. > But shouldnt we add a warn on for such a case, if we let it go through both the first part and the loop below then it will populate the master_trans and slave_bitmask both for the same crtc which would be wrong How can we flag such a case? Manasi > > > > > And then also add this as a PIPE_CONF_CHECK_X check to pipe_config_compare(). > > > > > > > This is already added in pipe_config_compare() in the patch that adds these two master_trans and slave_bitmask to the crtc state > > > > Manasi > > > > > With that fixed and CI happy, > > > > > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > > > + 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); > > > > + } > > > > +} > > > > + > > > > static bool haswell_get_pipe_config(struct intel_crtc *crtc, > > > > struct intel_crtc_state *pipe_config) > > > > { > > > > @@ -10566,6 +10632,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, > > > > > > > > -- > Ville Syrjälä > Intel
On Wed, Sep 25, 2019 at 11:37:58AM -0700, Manasi Navare wrote: > On Wed, Sep 25, 2019 at 01:08:23PM +0300, Ville Syrjälä wrote: > > On Tue, Sep 24, 2019 at 10:59:57AM -0700, Manasi Navare wrote: > > > On Tue, Sep 24, 2019 at 05:38:00PM +0200, Maarten Lankhorst wrote: > > > > Op 22-09-2019 om 19:08 schreef Manasi Navare: > > > > > 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. > > > > > > > > > > v4: > > > > > * Get power domains in master loop for get_config (Ville) > > > > > v3: > > > > > * Add TRANSCODER_D (Maarten) > > > > > * v3 Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > v2: > > > > > * Add Transcoder_D and MISSING_CASE (Maarten) > > > > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@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 1ae5eafe2892..711987eb4e9e 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > @@ -10470,6 +10470,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; > > > > Could move this return up to default. :) > > > > > > Yes will do this > > > > > > > > +} > > > > > + > > > > > +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; > > > > > + return; > > > > > + } > > > > > + > > > > > > > > It could still be useful to go through the loop below anyway, in case we messed up. We are reading out from hw after all. > > > > > > > > > > The loop below will be called always in case of the HW state readout for master, in case of the slave it will execute > > > the firs part, get the master transcoder and return, why do we need to call the loop below for slave? Why cant we just return here > > > as in the code? > > > > I think Maarten's point was to catch cases where the same transcoder is > > accidentally configure as both slave and master. > > > But shouldnt we add a warn on for such a case, if we let it go through both the first part and the loop below > then it will populate the master_trans and slave_bitmask both for the same crtc which would be wrong > How can we flag such a case? During state readout it'll get flagged by the state checker. For the purposes of the initial readout I guess we could WARN_ON() since it never should happen. > > Manasi > > > > > > > > And then also add this as a PIPE_CONF_CHECK_X check to pipe_config_compare(). > > > > > > > > > > This is already added in pipe_config_compare() in the patch that adds these two master_trans and slave_bitmask to the crtc state > > > > > > Manasi > > > > > > > With that fixed and CI happy, > > > > > > > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > > > > > + 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); > > > > > + } > > > > > +} > > > > > + > > > > > static bool haswell_get_pipe_config(struct intel_crtc *crtc, > > > > > struct intel_crtc_state *pipe_config) > > > > > { > > > > > @@ -10566,6 +10632,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, > > > > > > > > > > > > -- > > Ville Syrjälä > > Intel
On Thu, Sep 26, 2019 at 03:28:44PM +0300, Ville Syrjälä wrote: > On Wed, Sep 25, 2019 at 11:37:58AM -0700, Manasi Navare wrote: > > On Wed, Sep 25, 2019 at 01:08:23PM +0300, Ville Syrjälä wrote: > > > On Tue, Sep 24, 2019 at 10:59:57AM -0700, Manasi Navare wrote: > > > > On Tue, Sep 24, 2019 at 05:38:00PM +0200, Maarten Lankhorst wrote: > > > > > Op 22-09-2019 om 19:08 schreef Manasi Navare: > > > > > > 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. > > > > > > > > > > > > v4: > > > > > > * Get power domains in master loop for get_config (Ville) > > > > > > v3: > > > > > > * Add TRANSCODER_D (Maarten) > > > > > > * v3 Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > > v2: > > > > > > * Add Transcoder_D and MISSING_CASE (Maarten) > > > > > > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@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 1ae5eafe2892..711987eb4e9e 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > @@ -10470,6 +10470,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; > > > > > Could move this return up to default. :) > > > > > > > > Yes will do this > > > > > > > > > > +} > > > > > > + > > > > > > +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; > > > > > > + return; > > > > > > + } > > > > > > + > > > > > > > > > > It could still be useful to go through the loop below anyway, in case we messed up. We are reading out from hw after all. > > > > > > > > > > > > > The loop below will be called always in case of the HW state readout for master, in case of the slave it will execute > > > > the firs part, get the master transcoder and return, why do we need to call the loop below for slave? Why cant we just return here > > > > as in the code? > > > > > > I think Maarten's point was to catch cases where the same transcoder is > > > accidentally configure as both slave and master. > > > > > But shouldnt we add a warn on for such a case, if we let it go through both the first part and the loop below > > then it will populate the master_trans and slave_bitmask both for the same crtc which would be wrong > > How can we flag such a case? > > During state readout it'll get flagged by the state checker. For the > purposes of the initial readout I guess we could WARN_ON() since it > never should happen. > So add a WARN_ON if we get INVALID_TRANSCODER for the master_transcoder in slave loop and then instead of returning just continue to the master part where we get the slave mask, correct? Manasi > > > > Manasi > > > > > > > > > > > And then also add this as a PIPE_CONF_CHECK_X check to pipe_config_compare(). > > > > > > > > > > > > > This is already added in pipe_config_compare() in the patch that adds these two master_trans and slave_bitmask to the crtc state > > > > > > > > Manasi > > > > > > > > > With that fixed and CI happy, > > > > > > > > > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > > > > > > > + 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); > > > > > > + } > > > > > > +} > > > > > > + > > > > > > static bool haswell_get_pipe_config(struct intel_crtc *crtc, > > > > > > struct intel_crtc_state *pipe_config) > > > > > > { > > > > > > @@ -10566,6 +10632,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, > > > > > > > > > > > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 1ae5eafe2892..711987eb4e9e 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -10470,6 +10470,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; + return; + } + + 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); + } +} + static bool haswell_get_pipe_config(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config) { @@ -10566,6 +10632,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,
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. v4: * Get power domains in master loop for get_config (Ville) v3: * Add TRANSCODER_D (Maarten) * v3 Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> v2: * Add Transcoder_D and MISSING_CASE (Maarten) Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 69 ++++++++++++++++++++ 1 file changed, 69 insertions(+)