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
--
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
