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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
