[Intel-gfx] [PATCH 11/23] drm/i915: Try to make bigjoiner work in atomic check.
Matt Roper
matthew.d.roper at intel.com
Thu Sep 26 03:48:02 UTC 2019
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 at linux.intel.com>
> ---
> 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.
> +
> + 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.
> 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
More information about the Intel-gfx
mailing list