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 > > > > > <[email protected]> > > > > > v2: > > > > > * Add Transcoder_D and MISSING_CASE (Maarten) > > > > > > > > > > Cc: Ville Syrjälä <[email protected]> > > > > > Cc: Maarten Lankhorst <[email protected]> > > > > > Cc: Matt Roper <[email protected]> > > > > > Cc: Jani Nikula <[email protected]> > > > > > Signed-off-by: Manasi Navare <[email protected]> > > > > > --- > > > > > 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 <[email protected]> > > > > > > > > > + 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 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
