Op 26-09-2019 om 05:48 schreef Matt Roper: > On Fri, Sep 20, 2019 at 01:42:23PM +0200, Maarten Lankhorst wrote: >> When the clock is higher than the dotclock, try with 2 pipes enabled. >> If we can enable 2, then we will go into big joiner mode, and steal >> the adjacent crtc. >> >> This only links the crtc's in software, no hardware or plane >> programming is done yet. Blobs are also copied from the master's >> crtc_state, so it doesn't depend at commit time on the other >> crtc_state. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> >> --- >> drivers/gpu/drm/i915/display/intel_atomic.c | 15 +- >> drivers/gpu/drm/i915/display/intel_atomic.h | 3 +- >> drivers/gpu/drm/i915/display/intel_display.c | 197 +++++++++++++++++- >> .../drm/i915/display/intel_display_types.h | 11 +- >> drivers/gpu/drm/i915/display/intel_dp.c | 25 ++- >> 5 files changed, 228 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c >> index c50e0b218bd6..0db04064c86e 100644 >> --- a/drivers/gpu/drm/i915/display/intel_atomic.c >> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c >> @@ -228,25 +228,26 @@ void intel_crtc_free_hw_state(struct intel_crtc_state *crtc_state) >> intel_crtc_put_color_blobs(crtc_state); >> } >> >> -void intel_crtc_copy_color_blobs(struct intel_crtc_state *crtc_state) >> +void intel_crtc_copy_color_blobs(struct intel_crtc_state *crtc_state, >> + const struct intel_crtc_state *from_crtc_state) >> { >> intel_crtc_put_color_blobs(crtc_state); >> >> - if (crtc_state->uapi.degamma_lut) >> + if (from_crtc_state->uapi.degamma_lut) >> crtc_state->hw.degamma_lut = >> - drm_property_blob_get(crtc_state->uapi.degamma_lut); >> + drm_property_blob_get(from_crtc_state->uapi.degamma_lut); >> else >> crtc_state->hw.degamma_lut = NULL; >> >> - if (crtc_state->uapi.gamma_lut) >> + if (from_crtc_state->uapi.gamma_lut) >> crtc_state->hw.gamma_lut = >> - drm_property_blob_get(crtc_state->uapi.gamma_lut); >> + drm_property_blob_get(from_crtc_state->uapi.gamma_lut); >> else >> crtc_state->hw.gamma_lut = NULL; >> >> - if (crtc_state->uapi.ctm) >> + if (from_crtc_state->uapi.ctm) >> crtc_state->hw.ctm = >> - drm_property_blob_get(crtc_state->uapi.ctm); >> + drm_property_blob_get(from_crtc_state->uapi.ctm); >> else >> crtc_state->hw.ctm = NULL; >> } >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h >> index 42be91e0772a..8da84d64aa04 100644 >> --- a/drivers/gpu/drm/i915/display/intel_atomic.h >> +++ b/drivers/gpu/drm/i915/display/intel_atomic.h >> @@ -36,7 +36,8 @@ struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc); >> void intel_crtc_destroy_state(struct drm_crtc *crtc, >> struct drm_crtc_state *state); >> void intel_crtc_free_hw_state(struct intel_crtc_state *crtc_state); >> -void intel_crtc_copy_color_blobs(struct intel_crtc_state *crtc_state); >> +void intel_crtc_copy_color_blobs(struct intel_crtc_state *crtc_state, >> + const struct intel_crtc_state *from_crtc_state); >> struct drm_atomic_state *intel_atomic_state_alloc(struct drm_device *dev); >> void intel_atomic_state_clear(struct drm_atomic_state *state); >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> index ba52a70840fd..143d531c4c81 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -7434,7 +7434,7 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, >> struct intel_crtc_state *pipe_config) >> { >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> - const struct drm_display_mode *adjusted_mode = &pipe_config->hw.adjusted_mode; >> + struct drm_display_mode *adjusted_mode = &pipe_config->hw.adjusted_mode; >> int clock_limit = dev_priv->max_dotclk_freq; >> >> if (INTEL_GEN(dev_priv) < 4) { >> @@ -7451,6 +7451,25 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, >> } >> } >> >> + /* >> + * copy hw mode to transcoder mode. >> + * This matters mostly for big joiner, which splits the mode in half. >> + */ >> + pipe_config->hw.transcoder_mode = pipe_config->hw.adjusted_mode; >> + if (pipe_config->bigjoiner) { >> + /* Make sure the crtc config is halved horizontally */ >> + adjusted_mode->crtc_clock /= 2; >> + adjusted_mode->crtc_hdisplay /= 2; >> + adjusted_mode->crtc_hblank_start /= 2; >> + adjusted_mode->crtc_hblank_end /= 2; >> + adjusted_mode->crtc_hsync_start /= 2; >> + adjusted_mode->crtc_hsync_end /= 2; >> + adjusted_mode->crtc_htotal /= 2; >> + adjusted_mode->crtc_hskew /= 2; > If adjusted_mode is the "pipe mode," does anything other than the size > have meaning or matter at that level? Not that halving the rest of > these hurts anything, it just seems strange to consider them at the pipe > level. Correct. A few places look at crtc_htotal/vtotal, for example pixel rate calculations, watermarks and pipe scaler. transcoder gets the full mode covering both pipes, anything else looks at the reduced mode in order to not break anything. :) >> + >> + pipe_config->pipe_src_w /= 2; >> + } >> + >> if (adjusted_mode->crtc_clock > clock_limit) { >> DRM_DEBUG_KMS("requested pixel clock (%d kHz) too high (max: %d kHz, double wide: %s)\n", >> adjusted_mode->crtc_clock, clock_limit, >> @@ -8133,7 +8152,7 @@ static void intel_set_pipe_timings(const struct intel_crtc_state *crtc_state) > We may want to rename this function to intel_set_transcoder_timings > now that pipe and transcoder are starting to be different. +1, will fix. > > >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> enum pipe pipe = crtc->pipe; >> enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; >> - const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; >> + const struct drm_display_mode *adjusted_mode = &crtc_state->hw.transcoder_mode; >> u32 crtc_vtotal, crtc_vblank_end; >> int vsyncshift = 0; >> >> @@ -11774,6 +11793,8 @@ static int intel_crtc_atomic_check(struct drm_crtc *_crtc, >> to_intel_crtc_state(_crtc_state); >> int ret; >> bool mode_changed = needs_modeset(crtc_state); >> + struct intel_atomic_state *state = >> + to_intel_atomic_state(crtc_state->uapi.state); >> >> if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv) && >> mode_changed && !crtc_state->hw.active) >> @@ -11781,6 +11802,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *_crtc, >> >> if (mode_changed && crtc_state->hw.enable && >> dev_priv->display.crtc_compute_clock && >> + !crtc_state->bigjoiner_slave && >> !WARN_ON(crtc_state->shared_dpll)) { >> ret = dev_priv->display.crtc_compute_clock(crtc, crtc_state); >> if (ret) >> @@ -11796,8 +11818,15 @@ static int intel_crtc_atomic_check(struct drm_crtc *_crtc, >> >> if (mode_changed || crtc_state->update_pipe || >> crtc_state->uapi.color_mgmt_changed) { >> + const struct intel_crtc_state *master_crtc_state = crtc_state; >> + >> + if (crtc_state->bigjoiner_slave) >> + master_crtc_state = >> + intel_atomic_get_new_crtc_state(state, >> + crtc_state->bigjoiner_linked_crtc); >> + >> /* Copy color blobs to hw state */ >> - intel_crtc_copy_color_blobs(crtc_state); >> + intel_crtc_copy_color_blobs(crtc_state, master_crtc_state); >> >> ret = intel_color_check(crtc_state); >> if (ret) >> @@ -12259,7 +12288,48 @@ static void copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state) >> crtc_state->uapi.enable = crtc_state->hw.enable; >> crtc_state->uapi.active = crtc_state->hw.active; >> crtc_state->uapi.mode = crtc_state->hw.mode; >> - crtc_state->uapi.adjusted_mode = crtc_state->hw.adjusted_mode; >> + crtc_state->uapi.adjusted_mode = crtc_state->hw.transcoder_mode; >> +} >> + >> +static int >> +copy_bigjoiner_crtc_state(struct intel_crtc_state *crtc_state, >> + const struct intel_crtc_state *from_crtc_state) >> +{ >> + struct intel_crtc_state *saved_state; >> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); >> + >> + saved_state = kmemdup(from_crtc_state, sizeof(*saved_state), GFP_KERNEL); >> + if (!saved_state) >> + return -ENOMEM; >> + >> + saved_state->uapi = crtc_state->uapi; >> + saved_state->scaler_state = crtc_state->scaler_state; >> + saved_state->shared_dpll = crtc_state->shared_dpll; >> + saved_state->dpll_hw_state = crtc_state->dpll_hw_state; >> + saved_state->crc_enabled = crtc_state->crc_enabled; >> + >> + intel_crtc_free_hw_state(crtc_state); >> + memcpy(crtc_state, saved_state, sizeof(*crtc_state)); >> + kfree(saved_state); >> + >> + /* Re-init hw state */ >> + memset(&crtc_state->hw, 0, sizeof(saved_state->hw)); >> + crtc_state->hw.enable = from_crtc_state->hw.enable; >> + crtc_state->hw.active = from_crtc_state->hw.active; >> + crtc_state->hw.mode = from_crtc_state->hw.mode; >> + crtc_state->hw.adjusted_mode = from_crtc_state->hw.adjusted_mode; >> + >> + /* Some fixups */ >> + crtc_state->uapi.mode_changed = from_crtc_state->uapi.mode_changed; >> + crtc_state->uapi.connectors_changed = from_crtc_state->uapi.connectors_changed; >> + crtc_state->uapi.active_changed = from_crtc_state->uapi.active_changed; >> + crtc_state->nv12_planes = crtc_state->c8_planes = crtc_state->update_planes = 0; >> + crtc_state->bigjoiner_linked_crtc = to_intel_crtc(from_crtc_state->uapi.crtc); >> + crtc_state->bigjoiner_slave = true; >> + crtc_state->cpu_transcoder = (enum transcoder)crtc->pipe; >> + crtc_state->has_audio = false; >> + >> + return 0; >> } >> >> static int >> @@ -12432,7 +12502,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config) >> base_bpp, pipe_config->pipe_bpp, pipe_config->dither); >> >> /* uapi wants a copy of the adjusted_mode for vblank bookkeeping */ >> - pipe_config->uapi.adjusted_mode = pipe_config->hw.adjusted_mode; >> + pipe_config->uapi.adjusted_mode = pipe_config->hw.transcoder_mode; >> >> return 0; >> } >> @@ -13549,6 +13619,109 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta >> new_crtc_state->has_drrs = old_crtc_state->has_drrs; >> } >> >> +static int intel_atomic_check_bigjoiner(struct intel_atomic_state *state) >> +{ >> + struct drm_i915_private *dev_priv = to_i915(state->base.dev); >> + struct intel_crtc_state *old_crtc_state, *new_crtc_state, *slave_crtc_state, *master_crtc_state; >> + struct intel_crtc *crtc, *slave, *master; >> + int i, ret = 0; >> + >> + if (INTEL_GEN(dev_priv) < 11) >> + return 0; >> + >> + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, >> + new_crtc_state, i) { >> + if (!old_crtc_state->bigjoiner_slave) >> + continue; >> + >> + if (crtc->pipe == PIPE_A) { > Rather than a hardcoded PIPE_A check, should we have a bitmask of pipes > that can be big joiner masters somewhere? mask = 0b111 for gen12, but > mask = 0b010 for gen11. Then the allowable slaves > would be mask << 1, which would also ensure gen11 doesn't allow a slave on pipe B. > >> + DRM_ERROR("Bigjoiner slave on pipe A?\n"); > We should probably just use a WARN_ON() in the condition above since > this shouldn't happen unless we screwed up somewhere in the driver. > >> + return -EINVAL; >> + } >> + >> + /* crtc staying in slave mode? */ >> + if (!new_crtc_state->uapi.enable) >> + continue; >> + >> + if (needs_modeset(new_crtc_state) || new_crtc_state->update_pipe) { > Won't needs_modeset() always return true here? I.e., if we were a slave > previously but didn't bail on the uapi.enable check above, then > uapi.enable is flipping from false to true. > > Given that enabling the previously-slave CRTC is only possible if the > previously-master CRTC is also modesetting in a way that won't need a > slave anymore, it seems like the next step would be > > master_crtc_state = intel_get_existing_crtc_state() > if (!master_crtc_state || !needs_modeset(master_crtc_state)) > fail > > otherwise we've now overcommitted our slave CRTC. > >> + master = old_crtc_state->bigjoiner_linked_crtc; >> + master_crtc_state = intel_atomic_get_crtc_state(&state->base, master); >> + if (IS_ERR(master_crtc_state)) >> + return PTR_ERR(master_crtc_state); >> + >> + /* >> + * Force modeset on master, to recalculate bigjoiner >> + * state. >> + * >> + * If master_crtc_state was not part of the atomic commit, >> + * we will fail because the master was not deconfigured, >> + * but at least fail below to unify the checks. >> + */ >> + master_crtc_state->uapi.mode_changed = true; > Should we instead update intel_pipe_config_compare to check bigjoiner > fields and prevent fastsets on states currently using the bigjoiner? > >> + >> + ret = drm_atomic_add_affected_planes(&state->base, &crtc->base); >> + if (ret) >> + return ret; >> + >> + ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base); >> + if (ret) >> + return ret; >> + } >> + } >> + >> + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, >> + new_crtc_state, i) { >> + if (!new_crtc_state->uapi.enable || !new_crtc_state->bigjoiner) { >> + if (!old_crtc_state->bigjoiner) >> + continue; >> + } >> + >> + if (!needs_modeset(new_crtc_state) && !new_crtc_state->update_pipe) >> + continue; >> + >> + if (new_crtc_state->bigjoiner && !new_crtc_state->bigjoiner_slave) { >> + if (1 + crtc->pipe >= INTEL_NUM_PIPES(dev_priv)) { >> + DRM_DEBUG_KMS("Big joiner configuration requires CRTC + 1 to be used, doesn't exist\n"); >> + return -EINVAL; >> + } >> + >> + slave = new_crtc_state->bigjoiner_linked_crtc = >> + intel_get_crtc_for_pipe(dev_priv, crtc->pipe + 1); >> + slave_crtc_state = intel_atomic_get_crtc_state(&state->base, slave); >> + if (IS_ERR(slave_crtc_state)) >> + return PTR_ERR(slave_crtc_state); >> + >> + if (slave_crtc_state->uapi.enable) { >> + DRM_DEBUG_KMS("[CRTC:%d:%s] Big joiner configuration requires this CRTC to be unconfigured\n", >> + slave->base.base.id, slave->base.name); >> + return -EINVAL; >> + } else { >> + DRM_DEBUG_KMS("[CRTC:%d:%s] Used as slave for big joiner\n", >> + slave->base.base.id, slave->base.name); >> + ret = copy_bigjoiner_crtc_state(slave_crtc_state, new_crtc_state); >> + } >> + } else { >> + master = new_crtc_state->bigjoiner_linked_crtc; >> + if (!master) >> + continue; >> + > What about the case of a CRTC that previously needed the bigjoiner but > is performing a modeset such that it no longer does? In that case > new_crtc_state->bigjoiner will be false so we'll end up in this branch. > But then we seem to be trying to grab the ex-master's master which > doesn't make sense for this case since we actually want to grab the > slave and clear it out. > > The code in this branch seems to be trying to operate on a CRTC that was > previously a slave, but assuming an ex-slave is staying disabled > (!new_crtc_state->uapi.enable), then I think we took the 'continue' at > the top of the loop and never make it down here. > >> + master_crtc_state = intel_atomic_get_crtc_state(&state->base, master); >> + if (IS_ERR(master_crtc_state)) >> + return PTR_ERR(master_crtc_state); >> + >> + if (!master_crtc_state->uapi.enable && !new_crtc_state->uapi.enable) { >> + DRM_DEBUG_KMS("[CRTC:%d:%s] Disabling slave from big joiner\n", >> + crtc->base.base.id, crtc->base.name); >> + ret = clear_intel_crtc_state(new_crtc_state); >> + } >> + } >> + if (ret) >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> /** >> * intel_atomic_check - validate state object >> * @dev: drm device >> @@ -13583,7 +13756,10 @@ static int intel_atomic_check(struct drm_device *dev, >> >> if (!new_crtc_state->uapi.enable) { >> any_ms = true; >> - clear_intel_crtc_state(new_crtc_state); >> + >> + /* big joiner slave is cleared in intel_atomic_check_bigjoiner() */ >> + if (!new_crtc_state->bigjoiner_slave) >> + clear_intel_crtc_state(new_crtc_state); >> continue; >> } >> >> @@ -13597,6 +13773,10 @@ static int intel_atomic_check(struct drm_device *dev, >> any_ms = true; >> } >> >> + ret = intel_atomic_check_bigjoiner(state); >> + if (ret) >> + return ret; >> + >> ret = drm_dp_mst_atomic_check(&state->base); >> if (ret) >> goto fail; >> @@ -13747,7 +13927,9 @@ static void intel_update_crtc(struct intel_crtc *crtc, >> >> commit_pipe_config(state, old_crtc_state, new_crtc_state); >> >> - if (INTEL_GEN(dev_priv) >= 9) >> + if (new_crtc_state->bigjoiner) >> + {/* Not supported yet */} >> + else if (INTEL_GEN(dev_priv) >= 9) >> skl_update_planes_on_crtc(state, crtc); >> else >> i9xx_update_planes_on_crtc(state, crtc); >> @@ -16846,7 +17028,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) >> } >> >> intel_bw_crtc_update(bw_state, crtc_state); >> - >> copy_hw_to_uapi_state(crtc_state); >> intel_pipe_config_sanity_check(dev_priv, crtc_state); >> } > Strap whitespace change. > >> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h >> index 394f84e5d583..dd0329a4b9ff 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_types.h >> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h >> @@ -774,7 +774,7 @@ struct intel_crtc_state { >> struct { >> bool active, enable; >> struct drm_property_blob *degamma_lut, *gamma_lut, *ctm; >> - struct drm_display_mode mode, adjusted_mode; >> + struct drm_display_mode mode, adjusted_mode, transcoder_mode; >> } hw; >> >> /** >> @@ -1005,6 +1005,15 @@ struct intel_crtc_state { >> /* enable pipe csc? */ >> bool csc_enable; >> >> + /* enable pipe big joiner? */ >> + bool bigjoiner; >> + >> + /* big joiner slave crtc? */ >> + bool bigjoiner_slave; >> + >> + /* linked crtc for bigjoiner, either slave or master */ >> + struct intel_crtc *bigjoiner_linked_crtc; >> + >> /* Display Stream compression state */ >> struct { >> bool compression_enable; >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c >> index 046e1662d1e3..3d487ce16151 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c >> @@ -2105,6 +2105,19 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp, >> pipe_config->port_clock = intel_dp->common_rates[limits->max_clock]; >> pipe_config->lane_count = limits->max_lane_count; >> >> + if (adjusted_mode->crtc_clock > intel_dp_downstream_max_dotclock(intel_dp, false)) { >> + if (adjusted_mode->crtc_clock > intel_dp_downstream_max_dotclock(intel_dp, true)) { >> + DRM_DEBUG_KMS("Clock rate too high for big joiner\n"); >> + return -EINVAL; >> + } >> + if (intel_dp_is_edp(intel_dp)) { >> + DRM_DEBUG_KMS("Cannot split eDP stream in bigjoiner configuration.\n"); >> + return -EINVAL; >> + } >> + pipe_config->bigjoiner = true; >> + DRM_DEBUG_KMS("Using bigjoiner configuration\n"); >> + } >> + >> if (intel_dp_is_edp(intel_dp)) { >> pipe_config->dsc_params.compressed_bpp = >> min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4, >> @@ -2122,12 +2135,12 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp, >> pipe_config->lane_count, >> adjusted_mode->crtc_clock, >> adjusted_mode->crtc_hdisplay, >> - false); >> + pipe_config->bigjoiner); >> dsc_dp_slice_count = >> intel_dp_dsc_get_slice_count(intel_dp, >> adjusted_mode->crtc_clock, >> adjusted_mode->crtc_hdisplay, >> - false); >> + pipe_config->bigjoiner); >> if (!dsc_max_output_bpp || !dsc_dp_slice_count) { >> DRM_DEBUG_KMS("Compressed BPP/Slice Count not supported\n"); >> return -EINVAL; >> @@ -2142,13 +2155,13 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp, >> * is greater than the maximum Cdclock and if slice count is even >> * then we need to use 2 VDSC instances. >> */ >> - if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq) { >> - if (pipe_config->dsc_params.slice_count > 1) { >> - pipe_config->dsc_params.dsc_split = true; >> - } else { >> + if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq || pipe_config->bigjoiner) { >> + if (pipe_config->dsc_params.slice_count < 2) { >> DRM_DEBUG_KMS("Cannot split stream to use 2 VDSC instances\n"); >> return -EINVAL; >> } >> + >> + pipe_config->dsc_params.dsc_split = true; >> } >> >> ret = intel_dp_compute_dsc_params(intel_dp, pipe_config); >> -- >> 2.20.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx