[Intel-gfx] [PATCH 15/24] drm/i915: Try to make bigjoiner work in atomic check, v2.
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Thu Oct 10 12:42:20 UTC 2019
Op 08-10-2019 om 21:40 schreef Ville Syrjälä:
> On Fri, Oct 04, 2019 at 01:35:05PM +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.
>>
>> Changes since v1:
>> - Rename pipe timings to transcoder timings, as they are now different.
>>
>> 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 | 218 ++++++++++++++++--
>> .../drm/i915/display/intel_display_types.h | 11 +-
>> drivers/gpu/drm/i915/display/intel_dp.c | 25 +-
>> 5 files changed, 234 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
>> index 4783d7ff4fcf..a5b11bd9da68 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 caab8cfddcbd..c2b3c7b6f39b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -123,7 +123,7 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc,
>> static int intel_framebuffer_init(struct intel_framebuffer *ifb,
>> struct drm_i915_gem_object *obj,
>> struct drm_mode_fb_cmd2 *mode_cmd);
>> -static void intel_set_pipe_timings(const struct intel_crtc_state *crtc_state);
>> +static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_state);
>> static void intel_set_pipe_src_size(const struct intel_crtc_state *crtc_state);
>> static void intel_cpu_transcoder_set_m_n(const struct intel_crtc_state *crtc_state,
>> const struct intel_link_m_n *m_n,
>> @@ -6308,7 +6308,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
>> if (intel_crtc_has_dp_encoder(pipe_config))
>> intel_dp_set_m_n(pipe_config, M1_N1);
>>
>> - intel_set_pipe_timings(pipe_config);
>> + intel_set_transcoder_timings(pipe_config);
>> intel_set_pipe_src_size(pipe_config);
>>
>> if (pipe_config->has_pch_encoder) {
>> @@ -6435,7 +6435,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
>> intel_dp_set_m_n(pipe_config, M1_N1);
>>
>> if (!transcoder_is_dsi(cpu_transcoder))
>> - intel_set_pipe_timings(pipe_config);
>> + intel_set_transcoder_timings(pipe_config);
>>
>> intel_set_pipe_src_size(pipe_config);
>>
>> @@ -6838,7 +6838,7 @@ static void valleyview_crtc_enable(struct intel_crtc_state *pipe_config,
>> if (intel_crtc_has_dp_encoder(pipe_config))
>> intel_dp_set_m_n(pipe_config, M1_N1);
>>
>> - intel_set_pipe_timings(pipe_config);
>> + intel_set_transcoder_timings(pipe_config);
>> intel_set_pipe_src_size(pipe_config);
>>
>> if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B) {
>> @@ -6906,7 +6906,7 @@ static void i9xx_crtc_enable(struct intel_crtc_state *pipe_config,
>> if (intel_crtc_has_dp_encoder(pipe_config))
>> intel_dp_set_m_n(pipe_config, M1_N1);
>>
>> - intel_set_pipe_timings(pipe_config);
>> + intel_set_transcoder_timings(pipe_config);
>> intel_set_pipe_src_size(pipe_config);
>>
>> i9xx_set_pipeconf(pipe_config);
>> @@ -7396,7 +7396,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) {
>> @@ -7413,6 +7413,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;
>> +
>> + pipe_config->pipe_src_w /= 2;
>> + }
> Hmm. Oh right, we still need to keep the full timings since
> the joiner is between the pipe and transcoder :/
>
> We probably need a check to make sure the non-halved horiz
> timings are even.
>
> hskew we don't use so don't need to frob that.
>
> I wonder if there are other places that need the full values. Quick
> glance only really reveals encoder and audio code. Most encoder code
> I guess doesn't really matter since bigjoiner is DP only. And the DP
> code itself looks fine since this halving happens after
> .compute_confog(). And the audio code only uses crtc_clock on HDMI
> (not sure it should use it there either, but that's a separate issue).
>
>> +
>> 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,
>> @@ -8114,13 +8133,13 @@ static void i8xx_compute_dpll(struct intel_crtc *crtc,
>> crtc_state->dpll_hw_state.dpll = dpll;
>> }
>>
>> -static void intel_set_pipe_timings(const struct intel_crtc_state *crtc_state)
>> +static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_state)
>> {
>> struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> 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;
>>
>> @@ -8205,8 +8224,8 @@ static bool intel_pipe_is_interlaced(const struct intel_crtc_state *crtc_state)
>> return I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_INTERLACE_MASK;
>> }
>>
>> -static void intel_get_pipe_timings(struct intel_crtc *crtc,
>> - struct intel_crtc_state *pipe_config)
>> +static void intel_get_transcoder_timings(struct intel_crtc *crtc,
>> + struct intel_crtc_state *pipe_config)
> Missing the transcoder_mode readout + adjusted_mode/2 here?
>
>> {
>> struct drm_device *dev = crtc->base.dev;
>> struct drm_i915_private *dev_priv = to_i915(dev);
>> @@ -8809,7 +8828,7 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>> if (INTEL_GEN(dev_priv) < 4)
>> pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
>>
>> - intel_get_pipe_timings(crtc, pipe_config);
>> + intel_get_transcoder_timings(crtc, pipe_config);
>> intel_get_pipe_src_size(crtc, pipe_config);
>>
>> i9xx_get_pfit_config(crtc, pipe_config);
>> @@ -10045,7 +10064,7 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>> pipe_config->pixel_multiplier = 1;
>> }
>>
>> - intel_get_pipe_timings(crtc, pipe_config);
>> + intel_get_transcoder_timings(crtc, pipe_config);
>> intel_get_pipe_src_size(crtc, pipe_config);
>>
>> ironlake_get_pfit_config(crtc, pipe_config);
>> @@ -10445,7 +10464,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>> if (!transcoder_is_dsi(pipe_config->cpu_transcoder) ||
>> INTEL_GEN(dev_priv) >= 11) {
>> haswell_get_ddi_port_state(crtc, pipe_config);
>> - intel_get_pipe_timings(crtc, pipe_config);
>> + intel_get_transcoder_timings(crtc, pipe_config);
>> }
>>
>> intel_get_pipe_src_size(crtc, pipe_config);
>> @@ -11814,6 +11833,7 @@ static int intel_crtc_atomic_check(struct intel_atomic_state *state,
>>
>> 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)
>> @@ -12278,7 +12298,7 @@ static void copy_uapi_to_hw_state(struct intel_crtc_state *crtc_state)
>> crtc_state->hw.active = crtc_state->uapi.active;
>> crtc_state->hw.mode = crtc_state->uapi.mode;
>> crtc_state->hw.adjusted_mode = crtc_state->uapi.adjusted_mode;
>> - intel_crtc_copy_color_blobs(crtc_state);
>> + intel_crtc_copy_color_blobs(crtc_state, crtc_state);
>> }
>>
>> static void copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state)
>> @@ -12286,7 +12306,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
>> @@ -12459,7 +12520,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;
>> }
>> @@ -13612,6 +13673,109 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
>> return 0;
>> }
>>
>> +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)
> I keep thinking that points at the slave. "is_bigjoiner_slave" would be
> a less confusing name perhaps.
>
>> + continue;
>> +
>> + if (crtc->pipe == PIPE_A) {
>> + DRM_ERROR("Bigjoiner slave on pipe A?\n");
>> + return -EINVAL;
>> + }
> Dead code?
>
>> +
>> + /* crtc staying in slave mode? */
>> + if (!new_crtc_state->uapi.enable)
>> + continue;
>> +
>> + if (needs_modeset(new_crtc_state) || new_crtc_state->update_pipe) {
> Are these checks even needed? We've already determined we were a
> slave and the uapi crtc is now enabled, so we have to give the
> pipe back to the uapi user no matter what.
>
>> + 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;
>> +
>> + 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;
> This whole thing feels a bit late to be doing it. Can't we just do this
> stuff before .compute_config()? Then at least in theory the master could
> have a chance to reconfigure itself to not use the bigjoiner? I suppose
> there is nothing it really can do to make the configuration work, but
> I think it would still feel like the more natural order of things.
>
>> + }
>> + }
>> +
>> + 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;
> I'm having a hard time decoding that. What are we trying to do?
Break and merge the bigjoiner slave configs. Although I guess we can probably do this inside the pipe check now. I may have made it too complicated before. :)
>
>> + }
>> +
>> + 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;
>> + }
> I think we should be able to reject such a configuration already
> when we decided that we need to use the bigjoiner. Should probably
> write it with bitmasks to be future proof.
Yeah, see above. I think not future proofing is fine for now. :)
>> +
>> + 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 {
> I guess this means we're always looking to associate the bigjoiner
> master with the uapi crtc? I think in theory we should be able to
> handle the case where the slave is the uapi crtc as well. But maybe
> it doesn't matter so much as userspace probably picks crtcs in order
> anyway. Hmm, maybe it would even be hard to deal with that...
I think just doing the easy thing first makes it less likely to mess up. In theory we can do this, but I think it's not worth handling this special case.
>> + master = new_crtc_state->bigjoiner_linked_crtc;
>> + if (!master)
>> + continue;
>> +
>> + 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);
>> + }
> This too feels like it could be done before we've compute anything.
>
> Something like this is what I was thinking earlier:
>
> detach_joiner() {
> master.bigjoiner = {};
> slave.bigjoiner = {};
> }
>
> setup_state_thing() {
> if (new_crtc_state->uapi.enable) {
> if (is_slave) {
> assert(needs_modeset(new_crtc_state));
> master = get_linked();
> master.modeset = true;
> detach_joiner();
> } else if (is_master) {
> slave = get_linked();
> if (needs_modeset(new_crtc_state)) {
> slave.modeset = true;
> detach_joiner();
> }
> }
> copy_uapi_to_hw(new_crtc_state);
> } else {
> if (is_master) {
> assert(needs_modeset(new_crtc_state));
> slave = get_linked();
> slave.modeset = true;
> detach_joiner();
> }
> if (!is_slave && needs_modeset(new_crtc_state))
> copy_uapi_to_hw(new_crtc_state);
> }
> }
>
> drm_atomic_helper_check_modeset();
> + setup_state_thing();
Yeah, can do it immediately after in intel_crtc_check_fastset(), will fix the series. :)
More information about the Intel-gfx
mailing list