[Intel-gfx] [RFC PATCH 4/7] drm/i915: enable fastboot for everyone
Daniel Vetter
daniel at ffwll.ch
Tue Jun 23 04:50:43 PDT 2015
On Tue, Jun 23, 2015 at 12:38:50PM +0200, Maarten Lankhorst wrote:
> Op 22-06-15 om 17:21 schreef Daniel Vetter:
> >> - /*
> >> - * FIXME: BIOS likes to set up a cloned config with lvds+external
> >> - * screen. Since we don't yet re-compute the pipe config when moving
> >> - * just the lvds port away to another pipe the sw tracking won't match.
> >> - *
> >> - * Proper atomic modesets with recomputed global state will fix this.
> >> - * Until then just don't check gmch state for inherited modes.
> >> - */
> >> if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) {
> > This seems like a separate change - with recomputing state on all pipes
> > unconditionally we should be able to drop this?
> We repurpose inherited mode here to skip some checks on first boot that
> will be patched up on first atomic commit.
Yeah, but since you delete the FIXME comment (I agree that's adressed)
there shouldn't be any reason any more for this. If there still is then we
need a new comment imo. And we do read out pfit state correctly, so this
shouldn't cause any pipe config mismatches on the initial state.
> >> @@ -13147,7 +13122,10 @@ static int __intel_set_mode(struct drm_atomic_state *state)
> >> if (needs_modeset(crtc->state) && crtc->state->active) {
> >> update_scanline_offset(to_intel_crtc(crtc));
> >> dev_priv->display.crtc_enable(crtc);
> >> - }
> >> + } else if (to_intel_crtc_state(crtc_state)->quirks &
> >> + PIPE_CONFIG_QUIRK_INHERITED_MODE)
> >> + /* force hw readout check */
> >> + any_ms = true;
> > Imo this should be a separate patch since forcing a modeset check after
> > the initial takeover is a good thing. Also maybe do a
> > s/any_ms/do_modeset_checks/ for clarity of what this does.
> It's used for other reasons too, like updating power domains when crtc
> states change. So do_modeset_checks would be misleading.
Yeah I got confused reading code since it looks so different. any_ms
totally does not what I thought it does, and the name is good as-is.
> >>
> >> drm_atomic_helper_commit_planes_on_crtc(crtc_state);
> >> }
> >> @@ -13430,8 +13408,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> >> if (ret)
> >> goto out;
> >>
> >> - intel_update_pipe_size(to_intel_crtc(set->crtc));
> >> -
> >> ret = intel_set_mode_checked(state);
> >> if (ret) {
> >> DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
> >> @@ -13718,10 +13694,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
> >> if (!crtc->state->active)
> >> return;
> >>
> >> - if (state->visible)
> >> - /* FIXME: kill this fastboot hack */
> >> - intel_update_pipe_size(intel_crtc);
> >> -
> >> dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y);
> >> }
> >>
> >> @@ -13741,6 +13713,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> >> struct drm_device *dev = crtc->dev;
> >> struct drm_i915_private *dev_priv = dev->dev_private;
> >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >> + struct intel_crtc_state *old_intel_state =
> >> + to_intel_crtc_state(old_crtc_state);
> >>
> >> if (!needs_modeset(crtc->state))
> >> intel_pre_plane_update(intel_crtc);
> >> @@ -13756,7 +13730,10 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> >> intel_pipe_update_start(intel_crtc,
> >> &intel_crtc->atomic.start_vbl_count);
> >>
> >> - if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
> >> + if (!needs_modeset(crtc->state) && crtc->state->active &&
> >> + old_intel_state->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE)
> >> + intel_update_fastboot(intel_crtc, old_intel_state);
> > Nack, fastboot only for the initial mode is not what I want. We should be
> > able to speed up _any_ modeset where we just disable the pfit. Allowing
> > fast pfit removal unconditionally will also allow us to easily test this
> > part of fastboot with an igt, which I'd also like to see.
> You're right, but I think because this needs more infrastructure. I only want to convert fastboot at first.
> For MST we might also need a variant which only performs a pipe modeset, rather than a full modeset.
> Still there are cases where we will need to force a modeset regardless, for example when device frequency
> changes.
>
> In either case this probably requires a lot of code may differ on each platform. :(
This isn't about anything else but fastboot: I just want the pfit-disable
trick to work on _any_ modeset, since if we don't do that then we can't
test it with igt. And that's bad. pfit-disabling really should be
orthogonal to any inherited state cleanup, if it's not the good isn't yet
ready for prime time imo.
> >> + else if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
> >> skl_detach_scalers(intel_crtc);
> > Also I think it'd be clearer if we have state flags (like for the pageflip
> > stuff) in crtc_state to enable these fixup functions on an as-needed
> > basis.
> I think this call should be moved to the plane commit function instead, with a single
> detach_scaler to disable the relevant scaler, if the scaler's not used afterwards.
>
> fastboot updates would disable it for the crtc, eliminating the need to call it separately.
Yeah that makes sense.
>
> >> }
> >>
> >> @@ -15096,6 +15073,114 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
> >> return true;
> >> }
> >>
> >> +static bool
> >> +intel_compare_m_n(unsigned int m, unsigned int n,
> >> + unsigned int m2, unsigned int n2)
> >> +{
> >> + if (m == m2 && n == n2)
> >> + return true;
> >> +
> >> + if (!m || !n || !m2 || !n2)
> >> + return false;
> >> +
> >> + if (m > m2) {
> >> + while (m > m2) {
> >> + m >>= 1;
> >> + n >>= 1;
> >> + }
> >> + } else if (m < m2) {
> >> + while (m < m2) {
> >> + m2 >>= 1;
> >> + n2 >>= 1;
> >> + }
> >> + }
> >> +
> >> + return m == m2 && n == n2;
> >> +}
> >> +
> >> +static bool
> >> +intel_compare_link_m_n(const struct intel_link_m_n *m_n,
> >> + const struct intel_link_m_n *m2_n2)
> >> +{
> >> + if (m_n->tu == m2_n2->tu &&
> >> + intel_compare_m_n(m_n->gmch_m, m_n->gmch_n,
> >> + m2_n2->gmch_m, m2_n2->gmch_n) &&
> >> + intel_compare_m_n(m_n->link_m, m_n->link_n,
> >> + m2_n2->link_m, m2_n2->link_n))
> >> + return true;
> >> +
> >> + return false;
> >> +}
> >> +
> >> +static void intel_fastboot_calc_modeset(struct intel_crtc *crtc,
> >> + struct intel_crtc_state *hw_state,
> >> + struct intel_crtc_state *pipe_config)
> >> +{
> >> + struct drm_device *dev = crtc->base.dev;
> >> + struct intel_crtc_state sw_state;
> >> +
> >> + if (needs_modeset(&pipe_config->base)) {
> >> + DRM_DEBUG_KMS("Skipping fastboot checks, modeset forced\n");
> >> + return;
> >> + }
> >> +
> >> + memset(&sw_state, 0, sizeof(sw_state));
> >> + sw_state.base = pipe_config->base;
> >> + sw_state.scaler_state = pipe_config->scaler_state;
> >> + sw_state.shared_dpll = pipe_config->shared_dpll;
> >> + sw_state.dpll_hw_state = pipe_config->dpll_hw_state;
> >> + sw_state.ddi_pll_sel = pipe_config->ddi_pll_sel;
> >> + intel_modeset_pipe_config(&crtc->base, &sw_state);
> >> + sw_state.quirks = hw_state->quirks & PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS;
> >> +
> >> + if (intel_compare_link_m_n(&sw_state.fdi_m_n, &hw_state->fdi_m_n))
> >> + sw_state.fdi_m_n = hw_state->fdi_m_n;
> >> +
> >> + if (INTEL_INFO(dev)->gen < 8) {
> >> + if (intel_compare_link_m_n(&sw_state.dp_m_n,
> >> + &hw_state->dp_m_n))
> >> + sw_state.dp_m_n = hw_state->dp_m_n;
> >> +
> >> + if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
> >> + &hw_state->dp_m2_n2))
> >> + sw_state.dp_m2_n2 = hw_state->dp_m2_n2;
> >> + } else {
> >> + if (intel_compare_link_m_n(&sw_state.dp_m_n,
> >> + &hw_state->dp_m_n)) {
> >> + sw_state.dp_m_n = hw_state->dp_m_n;
> >> + hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
> >> + } else if (intel_compare_link_m_n(&sw_state.dp_m2_n2,
> >> + &hw_state->dp_m_n)) {
> >> + sw_state.dp_m2_n2 = hw_state->dp_m_n;
> >> +
> >> + hw_state->dp_m_n = sw_state.dp_m_n;
> >> + hw_state->dp_m2_n2 = sw_state.dp_m2_n2;
> >> + }
> >> + }
> > Not too happy with splitting up the modeset state compare code like this.
> > If we add enum mode { EXACT, COMPATIBLE } to intel_pipe_config_compare we
> > could tie things together well and have all the pipe config code grouped
> > in one place. But then the _compare() is a bit misleading since it'd also
> > update the sw_state, so maybe better to call it compare_and_ajust.
> Or pass a bool adjust, since checking state after a modeset needs to match exactly,
> and no adjustment can be done.
Yeah adjust is a better name. The point behind the enum is that if you
have a pile of bool arguments it's hard to know without looking at the
function what's going on. With enums you can go with self-descriptive
names. We're not doing this yet everywhere, but as a rule I'd like to see
it if there's more than one bool in the arg list.
> > The macros would then either copy the hw_state to sw_state (if it's some
> > intermediate state we don't care about) or compare it (with maybe some
> > adjustments made). But I'd really like to have all that compare/adjust
> > logic in one place if possible.
> >
> >> +
> >> + /* Check if we need to force a modeset */
> >> + if (!intel_pipe_config_compare(dev, &sw_state, hw_state, true)) {
> >> + DRM_INFO("[CRTC:%i] sw state incompatible, forcing modeset\n",
> >> + crtc->base.base.id);
> >> + pipe_config->base.mode_changed = true;
> >> + } else {
> >> + DRM_INFO("[CRTC:%i] sw state and hw state fastboot compatible\n",
> >> + crtc->base.base.id);
> >> +
> >> + *pipe_config = sw_state;
> >> + intel_dump_pipe_config(crtc, pipe_config,
> >> + "[new fastboot state]");
> >> +
> >> + /* prevent a modeset by patching up mode struct */
> >> +
> >> + hw_state->base.mode.type = pipe_config->base.mode.type =
> >> + hw_state->base.adjusted_mode.type = pipe_config->base.adjusted_mode.type;
> >> +
> >> + /* clock readout can be approximate, just copy it. */
> >> + hw_state->base.mode.clock = pipe_config->base.mode.clock = pipe_config->base.adjusted_mode.clock;
> > Why do we still need this? If we use intel_pipe_config_compare to compute
> > mode_changed then we hopefully don't need to hack around things any more.
> > I guess this is where we need your plan to split out connector_changed
> > from mode_changed and then replace the mode_changed computed by the
> > helpers by what intel_pipe_config_compare things is justified.
> My display has a slightly different clock and mode->type in edid. Patching this up prevented the modeset.
> There can also be other reasons for modeset, like CDCLK changes.
Yeah I know we need to patch this up, but we should patch up the requested
mode. Instead allowing some slight mismatches in the adjusted mode (and
copying over the dpll state if that's the case) should be all that's
needed.
Maybe helpers should also look at adjusted modes instead of requested
modes? Would be a big change, but I think it makes sense in general -
adjusted_mode is the one the crtc is supposed to output. That would also
help with handling pfit changes correctly in i915.
>
> >> + }
> >> +}
> >> +
> >> static void intel_sanitize_crtc(struct intel_crtc *crtc,
> >> struct intel_crtc_state *pipe_config,
> >> bool force_restore)
> >> @@ -15135,37 +15220,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> >> pipe_config->base.active = !!enable;
> >> }
> >>
> >> - if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE) {
> >> - if (force_restore || i915.fastboot)
> >> - pipe_config->base.mode_changed = true;
> >> - else
> >> - pipe_config->base.active = false;
> >> - }
> >> -
> >> - /* XXX: This is needs to be modified for making fastboot possible
> >> - * by default without requiring a modeset.
> >> - */
> >> - if (hw_state->base.active && pipe_config->base.active) {
> >> - struct intel_crtc_state sw_state;
> >> -
> >> - memset(&sw_state, 0, sizeof(sw_state));
> >> - sw_state.base = pipe_config->base;
> >> - sw_state.scaler_state = pipe_config->scaler_state;
> >> - sw_state.shared_dpll = pipe_config->shared_dpll;
> >> - sw_state.dpll_hw_state = pipe_config->dpll_hw_state;
> >> - sw_state.ddi_pll_sel = pipe_config->ddi_pll_sel;
> >> -
> >> - intel_modeset_pipe_config(&crtc->base, &sw_state);
> >> -
> >> - /* Check if we need to force a modeset */
> >> - if (!intel_pipe_config_compare(dev, &sw_state, hw_state, !i915.fastboot)) {
> >> - DRM_DEBUG_KMS("sw and hw state differ, forcing modeset\n");
> >> - pipe_config->base.mode_changed = true;
> >> - } else {
> >> - *pipe_config = sw_state;
> >> - }
> >> - }
> >> + if (hw_state->quirks & PIPE_CONFIG_QUIRK_WRONG_PLANE)
> >> + pipe_config->base.mode_changed = true;
> >>
> >> + else if (hw_state->base.active && pipe_config->base.active)
> >> + intel_fastboot_calc_modeset(crtc, hw_state, pipe_config);
> >>
> >> if (dev_priv->quirks & QUIRK_PIPEA_FORCE && !hw_state->base.active &&
> >> crtc->pipe == PIPE_A && !pipe_config->base.active) {
> >> @@ -15584,6 +15643,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev, bool force_restore)
> >> crtc_state->planes_changed = false;
> >>
> >> if (crtc->state->enable) {
> >> + memset(&crtc->mode, 0, sizeof(crtc->mode));
> >> intel_mode_from_pipe_config(&crtc->mode,
> >> to_intel_crtc_state(crtc->state));
> >> intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
> >> @@ -15678,9 +15738,12 @@ void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
> >> int j;
> >> struct drm_plane *primary = c->primary;
> >>
> >> + if (!c->state->active)
> >> + continue;
> >> +
> >> obj = intel_fb_obj(primary->state->fb);
> >> if (obj == NULL)
> >> - goto disable;
> >> + continue;
> >>
> >> mutex_lock(&dev->struct_mutex);
> >> ret = intel_pin_and_fence_fb_obj(primary, primary->state->fb,
> >> @@ -15715,6 +15778,8 @@ void intel_modeset_gem_init(struct drm_device *dev, struct drm_atomic_state *sta
> >> */
> >>
> >> disable:
> >> + DRM_DEBUG_KMS("[CRTC:%i] No fb or forced inactive, disabling.\n",
> >> + c->base.id);
> >> crtc_state->active = crtc_state->enable = false;
> >>
> >> ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> >> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> >> index 6ac4990a0c11..2c494d78652a 100644
> >> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> >> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> >> @@ -486,15 +486,10 @@ retry:
> >> * config, not the input mode, which is what crtc->mode
> >> * usually contains. But since our current fastboot
> >> * code puts a mode derived from the post-pfit timings
> >> - * into crtc->mode this works out correctly. We don't
> >> - * use hwmode anywhere right now, so use it for this
> >> - * since the fb helper layer wants a pointer to
> >> - * something we own.
> >> + * into crtc->mode this works out correctly.
> >> */
> >> DRM_DEBUG_KMS("looking for current mode on connector %s\n",
> >> connector->name);
> >> - intel_mode_from_pipe_config(&encoder->crtc->hwmode,
> >> - to_intel_crtc(encoder->crtc)->config);
> >> modes[i] = &encoder->crtc->hwmode;
> >> }
> >> crtcs[i] = new_crtc;
> >> @@ -584,9 +579,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
> >> struct intel_crtc *intel_crtc;
> >> unsigned int max_size = 0;
> >>
> >> - if (!i915.fastboot)
> >> - return false;
> > I think it'd be useful to split the fbdev part of the fastboot removal out
> > into a separate patch. Or do I miss a depency here? Assuming ofc that we
> > enable the pfit trick for modesets in general.
> >
> I think you did miss a dependency. Not competely sure though..
Well yeah that's likely, but since I didn't see it you need to help me out
;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list