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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to