From: "Navare, Manasi" <manasi.d.navare@intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v10 06/11] drm/i915: Enable big joiner support in enable and disable sequences.
Date: Fri, 16 Oct 2020 11:17:33 -0700 [thread overview]
Message-ID: <20201016181733.GA4421@labuser-Z97X-UD5H> (raw)
In-Reply-To: <20201016160620.GP6112@intel.com>
On Fri, Oct 16, 2020 at 07:06:20PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 15, 2020 at 09:37:47AM -0700, Navare, Manasi wrote:
> > On Thu, Oct 15, 2020 at 04:07:05PM +0300, Ville Syrjälä wrote:
> > > On Thu, Oct 08, 2020 at 02:45:30PM -0700, Manasi Navare wrote:
> > > > @@ -4504,6 +4514,29 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
> > > > default:
> > > > break;
> > > > }
> > > > +}
> > > > +
> > > > +void intel_ddi_get_config(struct intel_encoder *encoder,
> > > > + struct intel_crtc_state *pipe_config)
> > > > +{
> > > > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > > + enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
> > > > +
> > > > + /* XXX: DSI transcoder paranoia */
> > > > + if (WARN_ON(transcoder_is_dsi(cpu_transcoder)))
> > > > + return;
> > > > +
> > > > + intel_ddi_read_func_ctl(encoder, pipe_config);
> > > > + if (pipe_config->bigjoiner_slave) {
> > > > + /* read out pipe settings from master */
> > > > + enum transcoder save = pipe_config->cpu_transcoder;
> > > > +
> > > > + /* Our own transcoder needs to be disabled when reading it in intel_ddi_read_func_ctl() */
> > > > + WARN_ON(pipe_config->output_types);
> > > > + pipe_config->cpu_transcoder = (enum transcoder)pipe_config->bigjoiner_linked_crtc->pipe;
> > > > + intel_ddi_read_func_ctl(encoder, pipe_config);
> > > > + pipe_config->cpu_transcoder = save;
> > > > + }
> > >
> > > This stuff is rather horrible. Shouldn't both pipes be using the
> > > same transcoder anyway? If so, is this stuff here just because
> > > we've miscalculated cpu_transcoder for the slave?
> >
> > Well honestly @Maarten wrote this and I believe had several iterations of reviews with you.
> > So he can explain better.
> > My understanding so far was that there is no encoder /transcoder for slave and thats
> > why for bigjoiner_slave we actually read stuff from master
> > So why is this wrong?
> >
> > >
> > > >
> > > > pipe_config->has_audio =
> > > > intel_ddi_is_audio_enabled(dev_priv, cpu_transcoder);
> > > > @@ -4529,7 +4562,8 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
> > > > dev_priv->vbt.edp.bpp = pipe_config->pipe_bpp;
> > > > }
> > > >
> > > > - intel_ddi_clock_get(encoder, pipe_config);
> > > > + if (!pipe_config->bigjoiner_slave)
> > > > + intel_ddi_clock_get(encoder, pipe_config);
> > > >
> > > > if (IS_GEN9_LP(dev_priv))
> > > > pipe_config->lane_lat_optim_mask =
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 37b56f4c2401..faf8bf757bed 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -7125,6 +7125,45 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
> > > > intel_de_write(dev_priv, reg, val);
> > > > }
> > > >
> > > > +static void tgl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> > > > + const struct intel_crtc_state *crtc_state)
> > > > +{
> > > > + struct intel_crtc *master = to_intel_crtc(crtc_state->uapi.crtc);
> > > > + struct intel_crtc_state *master_crtc_state;
> > > > + struct drm_connector_state *conn_state;
> > > > + struct drm_connector *conn;
> > > > + struct intel_encoder *encoder = NULL;
> > > > + int i;
> > > > +
> > > > + if (crtc_state->bigjoiner_slave)
> > > > + master = crtc_state->bigjoiner_linked_crtc;
> > > > +
> > > > + master_crtc_state = intel_atomic_get_new_crtc_state(state, master);
> > > > +
> > > > + for_each_new_connector_in_state(&state->base, conn, conn_state, i) {
> > > > + if (conn_state->crtc != &master->base)
> > > > + continue;
> > > > +
> > > > + encoder = to_intel_encoder(conn_state->best_encoder);
> > > > + break;
> > > > + }
> > > > +
> > > > + if (!crtc_state->bigjoiner_slave) {
> > > > + /* need to enable VDSC, which we skipped in pre-enable */
> > > > + intel_dsc_enable(encoder, crtc_state);
> > > > + } else {
> > > > + /*
> > > > + * Enable sequence steps 1-7 on bigjoiner master
> > > > + */
> > > > + intel_encoders_pre_pll_enable(state, master);
> > > > + intel_enable_shared_dpll(master_crtc_state);
> > > > + intel_encoders_pre_enable(state, master);
> > > > +
> > > > + /* and DSC on slave */
> > > > + intel_dsc_enable(NULL, crtc_state);
> > > > + }
> > > > +}
> > > > +
> > > > static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > > struct intel_crtc *crtc)
> > > > {
> > > > @@ -7138,34 +7177,39 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > > if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > > > return;
> > > >
> > > > - intel_encoders_pre_pll_enable(state, crtc);
> > > > -
> > > > - if (new_crtc_state->shared_dpll)
> > > > - intel_enable_shared_dpll(new_crtc_state);
> > > > + if (!new_crtc_state->bigjoiner) {
> > > > + intel_encoders_pre_pll_enable(state, crtc);
> > > >
> > > > - intel_encoders_pre_enable(state, crtc);
> > > > + if (new_crtc_state->shared_dpll)
> > > > + intel_enable_shared_dpll(new_crtc_state);
> > > >
> > > > - if (!transcoder_is_dsi(cpu_transcoder))
> > > > - intel_set_transcoder_timings(new_crtc_state);
> > > > + intel_encoders_pre_enable(state, crtc);
> > > > + } else {
> > > > + tgl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > > > + }
> > > >
> > > > intel_set_pipe_src_size(new_crtc_state);
> > > > + if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > > > + bdw_set_pipemisc(new_crtc_state);
> > > >
> > > > - if (cpu_transcoder != TRANSCODER_EDP &&
> > > > - !transcoder_is_dsi(cpu_transcoder))
> > > > - intel_de_write(dev_priv, PIPE_MULT(cpu_transcoder),
> > > > - new_crtc_state->pixel_multiplier - 1);
> > > > + if (!new_crtc_state->bigjoiner_slave && !transcoder_is_dsi(cpu_transcoder)) {
> > > > + if (!transcoder_is_dsi(cpu_transcoder))
> > > > + intel_set_transcoder_timings(new_crtc_state);
> > > >
> > > > - if (new_crtc_state->has_pch_encoder)
> > > > - intel_cpu_transcoder_set_m_n(new_crtc_state,
> > > > - &new_crtc_state->fdi_m_n, NULL);
> > > > + if (cpu_transcoder != TRANSCODER_EDP &&
> > > > + !transcoder_is_dsi(cpu_transcoder))
> > > > + intel_de_write(dev_priv, PIPE_MULT(cpu_transcoder),
> > > > + new_crtc_state->pixel_multiplier - 1);
> > > > +
> > > > + if (new_crtc_state->has_pch_encoder)
> > > > + intel_cpu_transcoder_set_m_n(new_crtc_state,
> > > > + &new_crtc_state->fdi_m_n, NULL);
> > > >
> > > > - if (!transcoder_is_dsi(cpu_transcoder)) {
> > > > hsw_set_frame_start_delay(new_crtc_state);
> > > > - hsw_set_pipeconf(new_crtc_state);
> > > > }
> > > >
> > > > - if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > > > - bdw_set_pipemisc(new_crtc_state);
> > > > + if (!transcoder_is_dsi(cpu_transcoder))
> > > > + hsw_set_pipeconf(new_crtc_state);
> > > >
> > > > crtc->active = true;
> > > >
> > > > @@ -7201,6 +7245,11 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > > if (INTEL_GEN(dev_priv) >= 11)
> > > > icl_pipe_mbus_enable(crtc);
> > > >
> > > > + if (new_crtc_state->bigjoiner_slave) {
> > > > + trace_intel_pipe_enable(crtc);
> > > > + intel_crtc_vblank_on(new_crtc_state);
> > > > + }
> > > > +
> > > > intel_encoders_enable(state, crtc);
> > > >
> > > > if (psl_clkgate_wa) {
> > > > @@ -7495,6 +7544,9 @@ static u64 get_crtc_power_domains(struct intel_crtc_state *crtc_state)
> > > > if (crtc_state->shared_dpll)
> > > > mask |= BIT_ULL(POWER_DOMAIN_DISPLAY_CORE);
> > > >
> > > > + if (crtc_state->dsc.compression_enable)
> > > > + mask |= BIT_ULL(intel_dsc_power_domain(crtc_state));
> > > > +
> > > > return mask;
> > > > }
> > > >
> > > > @@ -8113,6 +8165,30 @@ static u32 ilk_pipe_pixel_rate(const struct intel_crtc_state *crtc_state)
> > > > pfit_w * pfit_h);
> > > > }
> > > >
> > > > +static void intel_encoder_get_config(struct intel_encoder *encoder,
> > > > + struct intel_crtc_state *crtc_state)
> > > > +{
> > > > + struct drm_display_mode *pipe_mode = &crtc_state->hw.pipe_mode;
> > > > +
> > > > + encoder->get_config(encoder, crtc_state);
> > > > +
> > > > + *pipe_mode = crtc_state->hw.adjusted_mode;
> > > > + if (crtc_state->bigjoiner) {
> > > > + /*
> > > > + * transcoder is programmed to the full mode,
> > > > + * but pipe timings are half of the transcoder mode
> > > > + */
> > > > + pipe_mode->crtc_hdisplay /= 2;
> > > > + pipe_mode->crtc_hblank_start /= 2;
> > > > + pipe_mode->crtc_hblank_end /= 2;
> > > > + pipe_mode->crtc_hsync_start /= 2;
> > > > + pipe_mode->crtc_hsync_end /= 2;
> > > > + pipe_mode->crtc_htotal /= 2;
> > > > + pipe_mode->crtc_hskew /= 2;
> > > > + pipe_mode->crtc_clock /= 2;
> > > > + }
> > > > +}
> > > > +
> > > > static void intel_crtc_compute_pixel_rate(struct intel_crtc_state *crtc_state)
> > > > {
> > > > struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> > > > @@ -9027,20 +9103,22 @@ static void intel_get_pipe_src_size(struct intel_crtc *crtc,
> > > > void intel_mode_from_pipe_config(struct drm_display_mode *mode,
> > > > struct intel_crtc_state *pipe_config)
> > > > {
> > > > - mode->hdisplay = pipe_config->hw.adjusted_mode.crtc_hdisplay;
> > > > - mode->htotal = pipe_config->hw.adjusted_mode.crtc_htotal;
> > > > - mode->hsync_start = pipe_config->hw.adjusted_mode.crtc_hsync_start;
> > > > - mode->hsync_end = pipe_config->hw.adjusted_mode.crtc_hsync_end;
> > > > + struct drm_display_mode *hw_mode = &pipe_config->hw.adjusted_mode;
> > > > +
> > > > + mode->hdisplay = hw_mode->crtc_hdisplay;
> > > > + mode->htotal = hw_mode->crtc_htotal;
> > > > + mode->hsync_start = hw_mode->crtc_hsync_start;
> > > > + mode->hsync_end = hw_mode->crtc_hsync_end;
> > > >
> > > > - mode->vdisplay = pipe_config->hw.adjusted_mode.crtc_vdisplay;
> > > > - mode->vtotal = pipe_config->hw.adjusted_mode.crtc_vtotal;
> > > > - mode->vsync_start = pipe_config->hw.adjusted_mode.crtc_vsync_start;
> > > > - mode->vsync_end = pipe_config->hw.adjusted_mode.crtc_vsync_end;
> > > > + mode->vdisplay = hw_mode->crtc_vdisplay;
> > > > + mode->vtotal = hw_mode->crtc_vtotal;
> > > > + mode->vsync_start = hw_mode->crtc_vsync_start;
> > > > + mode->vsync_end = hw_mode->crtc_vsync_end;
> > > >
> > > > - mode->flags = pipe_config->hw.adjusted_mode.flags;
> > > > + mode->flags = hw_mode->flags;
> > > > mode->type = DRM_MODE_TYPE_DRIVER;
> > > >
> > > > - mode->clock = pipe_config->hw.adjusted_mode.crtc_clock;
> > > > + mode->clock = hw_mode->crtc_clock;
> > > >
> > > > drm_mode_set_name(mode);
> > > > }
> > > > @@ -11207,6 +11285,9 @@ static void hsw_get_ddi_port_state(struct intel_crtc *crtc,
> > > > } else {
> > > > tmp = intel_de_read(dev_priv,
> > > > TRANS_DDI_FUNC_CTL(cpu_transcoder));
> > > > + if (!(tmp & TRANS_DDI_FUNC_ENABLE))
> > > > + return;
> > > > +
> > > > if (INTEL_GEN(dev_priv) >= 12)
> > > > port = TGL_TRANS_DDI_FUNC_CTL_VAL_TO_PORT(tmp);
> > > > else
> > > > @@ -11279,12 +11360,20 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
> > > > drm_WARN_ON(&dev_priv->drm, active);
> > > > active = true;
> > > > }
> > > > + intel_dsc_get_config(pipe_config);
> > > >
> > > > - if (!active)
> > > > - goto out;
> > > > + if (!active) {
> > > > + /* bigjoiner slave doesn't enable transcoder */
> > > > + if (!pipe_config->bigjoiner_slave)
> > > > + goto out;
> > > >
> > > > - if (!transcoder_is_dsi(pipe_config->cpu_transcoder) ||
> > > > - INTEL_GEN(dev_priv) >= 11) {
> > > > + active = true;
> > > > + pipe_config->pixel_multiplier = 1;
> > > > +
> > > > + /* we cannot read out most state, so don't bother.. */
> > > > + pipe_config->quirks |= PIPE_CONFIG_QUIRK_BIGJOINER_SLAVE;
> > > > + } else if (!transcoder_is_dsi(pipe_config->cpu_transcoder) ||
> > > > + INTEL_GEN(dev_priv) >= 11) {
> > > > hsw_get_ddi_port_state(crtc, pipe_config);
> > > > intel_get_transcoder_timings(crtc, pipe_config);
> > > > }
> > > > @@ -11370,8 +11459,11 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
> > > > }
> > > > }
> > > >
> > > > - if (pipe_config->cpu_transcoder != TRANSCODER_EDP &&
> > > > - !transcoder_is_dsi(pipe_config->cpu_transcoder)) {
> > > > + if (pipe_config->bigjoiner_slave) {
> > > > + /* Cannot be read out as a slave, set to 0. */
> > > > + pipe_config->pixel_multiplier = 0;
> > > > + } else if (pipe_config->cpu_transcoder != TRANSCODER_EDP &&
> > > > + !transcoder_is_dsi(pipe_config->cpu_transcoder)) {
> > > > pipe_config->pixel_multiplier =
> > > > intel_de_read(dev_priv,
> > > > PIPE_MULT(pipe_config->cpu_transcoder)) + 1;
> > > > @@ -12386,7 +12478,7 @@ intel_encoder_current_mode(struct intel_encoder *encoder)
> > > > return NULL;
> > > > }
> > > >
> > > > - encoder->get_config(encoder, crtc_state);
> > > > + intel_encoder_get_config(encoder, crtc_state);
> > > >
> > > > intel_mode_from_pipe_config(mode, crtc_state);
> > > >
> > > > @@ -13385,12 +13477,15 @@ intel_crtc_copy_uapi_to_hw_state(struct intel_atomic_state *state,
> > > > intel_crtc_copy_uapi_to_hw_state_nomodeset(state, crtc_state);
> > > > }
> > > >
> > > > -static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state)
> > > > +static void intel_crtc_copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state,
> > > > + struct drm_display_mode *user_mode)
> > > > {
> > > > - crtc_state->uapi.enable = crtc_state->hw.enable;
> > > > - crtc_state->uapi.active = crtc_state->hw.active;
> > > > - drm_WARN_ON(crtc_state->uapi.crtc->dev,
> > > > - drm_atomic_set_mode_for_crtc(&crtc_state->uapi, &crtc_state->hw.mode) < 0);
> > > > + if (!crtc_state->bigjoiner_slave) {
> > > > + crtc_state->uapi.enable = crtc_state->hw.enable;
> > > > + crtc_state->uapi.active = crtc_state->hw.active;
> > > > + drm_WARN_ON(crtc_state->uapi.crtc->dev,
> > > > + drm_atomic_set_mode_for_crtc(&crtc_state->uapi, user_mode) < 0);
> > > > + }
> > > >
> > > > crtc_state->uapi.adjusted_mode = crtc_state->hw.adjusted_mode;
> > > >
> > > > @@ -14027,21 +14122,42 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> > > >
> > > > PIPE_CONF_CHECK_X(output_types);
> > > >
> > > > - PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hdisplay);
> > > > - PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_htotal);
> > > > - PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hblank_start);
> > > > - PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hblank_end);
> > > > - PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hsync_start);
> > > > - PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hsync_end);
> > > > -
> > > > - PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vdisplay);
> > > > - PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vtotal);
> > > > - PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vblank_start);
> > > > - PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vblank_end);
> > > > - PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vsync_start);
> > > > - PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vsync_end);
> > > > -
> > > > - PIPE_CONF_CHECK_I(pixel_multiplier);
> > > > + if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_BIGJOINER_SLAVE)) {
> > > > + /* bigjoiner mode = transcoder mode / 2, for calculations */
> > > > + PIPE_CONF_CHECK_I(hw.pipe_mode.crtc_hdisplay);
> > > > + PIPE_CONF_CHECK_I(hw.pipe_mode.crtc_htotal);
> > > > + PIPE_CONF_CHECK_I(hw.pipe_mode.crtc_vdisplay);
> > > > + PIPE_CONF_CHECK_I(hw.pipe_mode.crtc_vtotal);
> > > > +
> > > > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hdisplay);
> > > > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_htotal);
> > > > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hblank_start);
> > > > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hblank_end);
> > > > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hsync_start);
> > > > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_hsync_end);
> > > > +
> > > > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vdisplay);
> > > > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vtotal);
> > > > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vblank_start);
> > > > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vblank_end);
> > > > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vsync_start);
> > > > + PIPE_CONF_CHECK_I(hw.adjusted_mode.crtc_vsync_end);
> > > > +
> > > > + PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
> > > > + DRM_MODE_FLAG_INTERLACE);
> > > > +
> > > > + if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS)) {
> > > > + PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
> > > > + DRM_MODE_FLAG_PHSYNC);
> > > > + PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
> > > > + DRM_MODE_FLAG_NHSYNC);
> > > > + PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
> > > > + DRM_MODE_FLAG_PVSYNC);
> > > > + PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
> > > > + DRM_MODE_FLAG_NVSYNC);
> > > > + }
> > > > + PIPE_CONF_CHECK_I(pixel_multiplier);
> > > > + }
> > > > PIPE_CONF_CHECK_I(output_format);
> > > > PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
> > > > if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
> > > > @@ -14051,24 +14167,11 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> > > > PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
> > > > PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
> > > > PIPE_CONF_CHECK_BOOL(has_infoframe);
> > > > - PIPE_CONF_CHECK_BOOL(fec_enable);
> > > > + if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_BIGJOINER_SLAVE))
> > > > + PIPE_CONF_CHECK_BOOL(fec_enable);
> > > >
> > > > PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
> > > >
> > > > - PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
> > > > - DRM_MODE_FLAG_INTERLACE);
> > > > -
> > > > - if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS)) {
> > > > - PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
> > > > - DRM_MODE_FLAG_PHSYNC);
> > > > - PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
> > > > - DRM_MODE_FLAG_NHSYNC);
> > > > - PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
> > > > - DRM_MODE_FLAG_PVSYNC);
> > > > - PIPE_CONF_CHECK_FLAGS(hw.adjusted_mode.flags,
> > > > - DRM_MODE_FLAG_NVSYNC);
> > > > - }
> > > > -
> > > > PIPE_CONF_CHECK_X(gmch_pfit.control);
> > > > /* pfit ratios are autocomputed by the hw on gen4+ */
> > > > if (INTEL_GEN(dev_priv) < 4)
> > > > @@ -14094,7 +14197,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> > > > }
> > > >
> > > > PIPE_CONF_CHECK_I(scaler_state.scaler_id);
> > > > - PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate);
> > > > + if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_BIGJOINER_SLAVE))
> > > > + PIPE_CONF_CHECK_CLOCK_FUZZY(pixel_rate);
> > > >
> > > > PIPE_CONF_CHECK_X(gamma_mode);
> > > > if (IS_CHERRYVIEW(dev_priv))
> > > > @@ -14115,48 +14219,51 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> > > > PIPE_CONF_CHECK_BOOL(double_wide);
> > > >
> > > > PIPE_CONF_CHECK_P(shared_dpll);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.dpll_md);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.fp0);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.fp1);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.wrpll);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.spll);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.ctrl1);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr1);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr2);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr0);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.ebb0);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.ebb4);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.pll0);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.pll1);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.pll2);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.pll3);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.pll6);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.pll8);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.pll9);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.pll10);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.pcsdw12);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_refclkin_ctl);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_clktop2_coreclkctl1);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_clktop2_hsclkctl);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_div0);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_div1);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_lf);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_frac_lock);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_ssc);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_bias);
> > > > - PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_tdc_coldst_bias);
> > > > -
> > > > - PIPE_CONF_CHECK_X(dsi_pll.ctrl);
> > > > - PIPE_CONF_CHECK_X(dsi_pll.div);
> > > > -
> > > > - if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5)
> > > > - PIPE_CONF_CHECK_I(pipe_bpp);
> > > > -
> > > > - PIPE_CONF_CHECK_CLOCK_FUZZY(hw.adjusted_mode.crtc_clock);
> > > > - PIPE_CONF_CHECK_CLOCK_FUZZY(port_clock);
> > > > -
> > > > - PIPE_CONF_CHECK_I(min_voltage_level);
> > > > + if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_BIGJOINER_SLAVE)) {
> > >
> > > This looks super sketchy. I don't see why we'd want to skip all this
> > > state checking for bigjoiner.
> >
> > For big joiner slave we do not enable plls and hence it doesnt make sense
> > to compare the hw state of plls for big joiner slave
>
> This is skipping far more than just plls. Also I think the state should
> still match whether it has its own pll or not. I kinda think we should
> have a pll for both pipes actually since that would avoid all this
> special case code that will just end up breaking all the time.
Only the adjusted mode and pipe mode and port clock can be taken out of this condition.
But we cannot have a pll for the slave that contradicts the Bspec modeset enable sequence.
Manasi
>
> >
> > Manasi
> >
> > >
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.dpll_md);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.fp0);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.fp1);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.wrpll);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.spll);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.ctrl1);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr1);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr2);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.cfgcr0);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.ebb0);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.ebb4);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.pll0);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.pll1);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.pll2);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.pll3);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.pll6);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.pll8);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.pll9);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.pll10);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.pcsdw12);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.mg_refclkin_ctl);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.mg_clktop2_coreclkctl1);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.mg_clktop2_hsclkctl);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_div0);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_div1);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_lf);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_frac_lock);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_ssc);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_bias);
> > > > + PIPE_CONF_CHECK_X(dpll_hw_state.mg_pll_tdc_coldst_bias);
> > > > +
> > > > + PIPE_CONF_CHECK_X(dsi_pll.ctrl);
> > > > + PIPE_CONF_CHECK_X(dsi_pll.div);
> > > > +
> > > > + if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5)
> > > > + PIPE_CONF_CHECK_I(pipe_bpp);
> > > > +
> > > > + PIPE_CONF_CHECK_CLOCK_FUZZY(hw.adjusted_mode.crtc_clock);
> > > > + PIPE_CONF_CHECK_CLOCK_FUZZY(hw.pipe_mode.crtc_clock);
> > > > + PIPE_CONF_CHECK_CLOCK_FUZZY(port_clock);
> > > > +
> > > > + PIPE_CONF_CHECK_I(min_voltage_level);
> > > > + }
> > > >
> > > > PIPE_CONF_CHECK_X(infoframes.enable);
> > > > PIPE_CONF_CHECK_X(infoframes.gcp);
> > > --
> > > Ville Syrjälä
> > > Intel
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2020-10-16 18:16 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-08 21:45 [Intel-gfx] [PATCH v10 01/11] HAX to make DSC work on the icelake test system Manasi Navare
2020-10-08 21:45 ` [Intel-gfx] [PATCH v10 02/11] drm/i915/display: Rename pipe_timings to transcoder_timings Manasi Navare
2020-10-08 21:45 ` [Intel-gfx] [PATCH v10 03/11] drm/i915: Add hw.pipe_mode to allow bigjoiner pipe/transcoder split Manasi Navare
2020-10-08 21:45 ` [Intel-gfx] [PATCH v10 04/11] drm/i915/dp: Allow big joiner modes in intel_dp_mode_valid(), v3 Manasi Navare
2020-10-14 11:26 ` Ville Syrjälä
2020-10-14 19:04 ` Navare, Manasi
2020-10-15 11:52 ` Ville Syrjälä
2020-10-15 16:26 ` Navare, Manasi
2020-10-19 16:30 ` Ville Syrjälä
2020-10-19 22:51 ` Navare, Manasi
2020-10-20 18:39 ` Ville Syrjälä
2020-10-20 18:53 ` Navare, Manasi
2020-10-08 21:45 ` [Intel-gfx] [PATCH v10 05/11] drm/i915: Try to make bigjoiner work in atomic check Manasi Navare
2020-10-14 11:33 ` Ville Syrjälä
2020-10-14 19:09 ` Navare, Manasi
2020-10-08 21:45 ` [Intel-gfx] [PATCH v10 06/11] drm/i915: Enable big joiner support in enable and disable sequences Manasi Navare
2020-10-14 11:07 ` Jani Nikula
2020-10-15 13:07 ` Ville Syrjälä
2020-10-15 16:37 ` Navare, Manasi
2020-10-16 16:06 ` Ville Syrjälä
2020-10-16 18:17 ` Navare, Manasi [this message]
2020-10-16 18:50 ` Ville Syrjälä
2020-10-16 19:24 ` Navare, Manasi
2020-10-19 11:34 ` Jani Nikula
2020-10-19 22:53 ` Navare, Manasi
2020-10-08 21:45 ` [Intel-gfx] [PATCH v10 07/11] drm/i915: Make hardware readout work on i915 Manasi Navare
2020-10-19 16:36 ` Ville Syrjälä
2020-10-19 22:45 ` Navare, Manasi
2020-10-20 18:45 ` Ville Syrjälä
2020-10-20 18:57 ` Navare, Manasi
2020-10-20 21:57 ` Navare, Manasi
2020-10-08 21:45 ` [Intel-gfx] [PATCH v10 08/11] drm/i915: Link planes in a bigjoiner configuration, v3 Manasi Navare
2020-10-08 21:45 ` [Intel-gfx] [PATCH v10 09/11] drm/i915: Add bigjoiner aware plane clipping checks Manasi Navare
2020-10-19 16:20 ` Ville Syrjälä
2020-10-19 22:56 ` Navare, Manasi
2020-10-20 18:51 ` Ville Syrjälä
2020-10-08 21:45 ` [Intel-gfx] [PATCH v10 10/11] drm/i915: Ensure correct master/slave enable/disable sequence Manasi Navare
2020-10-19 16:26 ` Ville Syrjälä
2020-10-19 23:05 ` Navare, Manasi
2020-10-08 21:45 ` [Intel-gfx] [PATCH v10 11/11] drm/i915: Add debugfs dumping for bigjoiner, v3 Manasi Navare
2020-10-08 22:15 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [v10,01/11] HAX to make DSC work on the icelake test system Patchwork
2020-10-08 22:17 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2020-10-08 22:33 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-10-09 1:32 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2020-10-13 19:19 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [v10,01/11] HAX to make DSC work on the icelake test system (rev2) Patchwork
2020-10-13 19:21 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2020-10-13 19:45 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-10-14 14:32 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201016181733.GA4421@labuser-Z97X-UD5H \
--to=manasi.d.navare@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox