| Message ID | 1431354318-11995-8-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 11, 2015 at 04:24:43PM +0200, Maarten Lankhorst wrote: > @@ -14679,7 +14646,8 @@ static bool primary_get_hw_state(struct intel_crtc *crtc) > return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE; > } > > -static void intel_modeset_readout_hw_state(struct drm_device *dev) > +static void intel_modeset_readout_hw_state(struct drm_device *dev, > + unsigned *crtc_mask) Why is crtc_mask a paramter? > { > struct drm_i915_private *dev_priv = dev->dev_private; > enum pipe pipe; > @@ -14688,6 +14656,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > struct intel_connector *connector; > int i; > > + *crtc_mask = 0; > for_each_intel_crtc(dev, crtc) { > struct drm_plane *primary = crtc->base.primary; > struct intel_plane_state *plane_state; > @@ -14696,6 +14665,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > > crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE; > > + if (crtc->active) > + *crtc_mask |= drm_crtc_index(&crtc->base); > + > crtc->active = dev_priv->display.get_pipe_config(crtc, > crtc->config); > > @@ -14778,7 +14750,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, > struct intel_encoder *encoder; > int i; > > - intel_modeset_readout_hw_state(dev); > + unsigned crtc_mask; > + > + intel_modeset_readout_hw_state(dev, &crtc_mask); > > /* > * Now that we have the config, copy it to each CRTC struct > @@ -14837,10 +14811,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, > struct drm_crtc *crtc = > dev_priv->pipe_to_crtc_mapping[pipe]; > > - intel_crtc_restore_mode(crtc); > + if (crtc_mask & (1 << drm_crtc_index(crtc))) > + intel_crtc_restore_mode(crtc); this seems a bit backwards. If we push the crtc loop down into intel_crct_restore_mode (and call it intel_restore_mode or so) then we could reuse the ->enable state from atomic. I.e. /* all the code in intel_crtc_restore_mode except the set_mode * call at the bottom */ for_crc_in_atomic_state(crtc_state) if (crtc_state->enable) intel_set_mode That would look more atomic imo (we'll just need to replace this loop with a drm_atomic_commit once we're there). And contain the restore state tracking to one function instead of spreading it over readout_hw_state. Thoughts? -Daniel > } > - } else { > - intel_modeset_update_staged_output_state(dev); > } > > intel_modeset_check_state(dev); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index f85761494dd1..1e892098eea2 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -515,7 +515,6 @@ struct intel_crtc { > > struct intel_initial_plane_config plane_config; > struct intel_crtc_state *config; > - bool new_enabled; > > /* reset counter value when the last flip was submitted */ > unsigned int reset_counter; > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, May 11, 2015 at 07:33:01PM +0200, Daniel Vetter wrote: > On Mon, May 11, 2015 at 04:24:43PM +0200, Maarten Lankhorst wrote: > > @@ -14679,7 +14646,8 @@ static bool primary_get_hw_state(struct intel_crtc *crtc) > > return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE; > > } > > > > -static void intel_modeset_readout_hw_state(struct drm_device *dev) > > +static void intel_modeset_readout_hw_state(struct drm_device *dev, > > + unsigned *crtc_mask) > > Why is crtc_mask a paramter? > > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > enum pipe pipe; > > @@ -14688,6 +14656,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > > struct intel_connector *connector; > > int i; > > > > + *crtc_mask = 0; > > for_each_intel_crtc(dev, crtc) { > > struct drm_plane *primary = crtc->base.primary; > > struct intel_plane_state *plane_state; > > @@ -14696,6 +14665,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > > > > crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE; > > > > + if (crtc->active) > > + *crtc_mask |= drm_crtc_index(&crtc->base); > > + > > crtc->active = dev_priv->display.get_pipe_config(crtc, > > crtc->config); > > > > @@ -14778,7 +14750,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, > > struct intel_encoder *encoder; > > int i; > > > > - intel_modeset_readout_hw_state(dev); > > + unsigned crtc_mask; > > + > > + intel_modeset_readout_hw_state(dev, &crtc_mask); > > > > /* > > * Now that we have the config, copy it to each CRTC struct > > @@ -14837,10 +14811,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, > > struct drm_crtc *crtc = > > dev_priv->pipe_to_crtc_mapping[pipe]; > > > > - intel_crtc_restore_mode(crtc); > > + if (crtc_mask & (1 << drm_crtc_index(crtc))) > > + intel_crtc_restore_mode(crtc); > > this seems a bit backwards. If we push the crtc loop down into > intel_crct_restore_mode (and call it intel_restore_mode or so) then we > could reuse the ->enable state from atomic. I.e. > > /* all the code in intel_crtc_restore_mode except the set_mode > * call at the bottom */ > > for_crc_in_atomic_state(crtc_state) > if (crtc_state->enable) > intel_set_mode > > That would look more atomic imo (we'll just need to replace this loop with > a drm_atomic_commit once we're there). And contain the restore state > tracking to one function instead of spreading it over readout_hw_state. This doesn't work, and we actually need to pull the atomic state copying out of restore_mode up into readout_hw_state so that we can use it instead of ->new_* pointers and intel_crtc->config. And that needs to happen _before_ remove the new_ pointers ofc. Otherwise (i.e. after this patch) well overwrite object linking in readout_hw_state and then restore_mode will do something silly. Or do I still miss something? -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a9ce827601d8..8c2fb951029b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9733,7 +9733,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, retry: ret = drm_modeset_lock(&config->connection_mutex, ctx); if (ret) - goto fail_unlock; + goto fail; /* * Algorithm gets a little messy: @@ -9751,10 +9751,10 @@ retry: ret = drm_modeset_lock(&crtc->mutex, ctx); if (ret) - goto fail_unlock; + goto fail; ret = drm_modeset_lock(&crtc->primary->mutex, ctx); if (ret) - goto fail_unlock; + goto fail; old->dpms_mode = connector->dpms; old->load_detect_temp = false; @@ -9773,9 +9773,6 @@ retry: continue; if (possible_crtc->state->enable) continue; - /* This can occur when applying the pipe A quirk on resume. */ - if (to_intel_crtc(possible_crtc)->new_enabled) - continue; crtc = possible_crtc; break; @@ -9786,18 +9783,17 @@ retry: */ if (!crtc) { DRM_DEBUG_KMS("no pipe available for load-detect\n"); - goto fail_unlock; + goto fail; } ret = drm_modeset_lock(&crtc->mutex, ctx); if (ret) - goto fail_unlock; + goto fail; ret = drm_modeset_lock(&crtc->primary->mutex, ctx); if (ret) - goto fail_unlock; + goto fail; intel_crtc = to_intel_crtc(crtc); - intel_crtc->new_enabled = true; old->dpms_mode = connector->dpms; old->load_detect_temp = true; old->release_fb = NULL; @@ -9865,9 +9861,7 @@ retry: intel_wait_for_vblank(dev, intel_crtc->pipe); return true; - fail: - intel_crtc->new_enabled = crtc->state->enable; -fail_unlock: +fail: drm_atomic_state_free(state); state = NULL; @@ -9913,8 +9907,6 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, if (IS_ERR(crtc_state)) goto fail; - intel_crtc->new_enabled = false; - connector_state->best_encoder = NULL; connector_state->crtc = NULL; @@ -11052,21 +11044,6 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = { .atomic_flush = intel_finish_crtc_commit, }; -/** - * intel_modeset_update_staged_output_state - * - * Updates the staged output configuration state, e.g. after we've read out the - * current hw state. - */ -static void intel_modeset_update_staged_output_state(struct drm_device *dev) -{ - struct intel_crtc *crtc; - - for_each_intel_crtc(dev, crtc) { - crtc->new_enabled = crtc->base.state->enable; - } -} - /* Transitional helper to copy current connector/encoder state to * connector->state. This is needed so that code that is partially * converted to atomic does the right thing. @@ -11119,10 +11096,6 @@ static void intel_modeset_fixup_state(struct drm_atomic_state *state) crtc->base.enabled = crtc->base.state->enable; crtc->config = to_intel_crtc_state(crtc->base.state); } - - /* Copy the new configuration to the staged state, to keep the few - * pieces of code that haven't been converted yet happy */ - intel_modeset_update_staged_output_state(state->dev); } static void @@ -12415,9 +12388,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) } for_each_intel_crtc(dev, intel_crtc) { - if (intel_crtc->new_enabled == intel_crtc->base.enabled) - continue; - crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); if (IS_ERR(crtc_state)) { DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n", @@ -12426,9 +12396,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) continue; } - crtc_state->base.active = crtc_state->base.enable = - intel_crtc->new_enabled; - if (&intel_crtc->base == crtc) drm_mode_copy(&crtc_state->base.mode, &crtc->mode); } @@ -14679,7 +14646,8 @@ static bool primary_get_hw_state(struct intel_crtc *crtc) return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE; } -static void intel_modeset_readout_hw_state(struct drm_device *dev) +static void intel_modeset_readout_hw_state(struct drm_device *dev, + unsigned *crtc_mask) { struct drm_i915_private *dev_priv = dev->dev_private; enum pipe pipe; @@ -14688,6 +14656,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) struct intel_connector *connector; int i; + *crtc_mask = 0; for_each_intel_crtc(dev, crtc) { struct drm_plane *primary = crtc->base.primary; struct intel_plane_state *plane_state; @@ -14696,6 +14665,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE; + if (crtc->active) + *crtc_mask |= drm_crtc_index(&crtc->base); + crtc->active = dev_priv->display.get_pipe_config(crtc, crtc->config); @@ -14778,7 +14750,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, struct intel_encoder *encoder; int i; - intel_modeset_readout_hw_state(dev); + unsigned crtc_mask; + + intel_modeset_readout_hw_state(dev, &crtc_mask); /* * Now that we have the config, copy it to each CRTC struct @@ -14837,10 +14811,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; - intel_crtc_restore_mode(crtc); + if (crtc_mask & (1 << drm_crtc_index(crtc))) + intel_crtc_restore_mode(crtc); } - } else { - intel_modeset_update_staged_output_state(dev); } intel_modeset_check_state(dev); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index f85761494dd1..1e892098eea2 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -515,7 +515,6 @@ struct intel_crtc { struct intel_initial_plane_config plane_config; struct intel_crtc_state *config; - bool new_enabled; /* reset counter value when the last flip was submitted */ unsigned int reset_counter;
No longer any different from state->enable. v2: Keep track of enabled crtc's for calling intel_crtc_restore_mode. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 63 +++++++++++------------------------- drivers/gpu/drm/i915/intel_drv.h | 1 - 2 files changed, 18 insertions(+), 46 deletions(-)