[Intel-gfx] [RFC 1/3] drm/i915: Roll intel_crtc->atomic into intel_crtc_state

[Ville Syrjälä]

On Wed, Sep 02, 2015 at 07:15:25AM +0200, Maarten Lankhorst wrote:
> Op 01-09-15 om 17:48 schreef Ville Syrjälä:
> > On Tue, Sep 01, 2015 at 08:30:05AM -0700, Matt Roper wrote:
> >> On Tue, Sep 01, 2015 at 07:24:19AM +0200, Maarten Lankhorst wrote:
> >>> Op 29-08-15 om 01:57 schreef Matt Roper:
> >>>> Way back at the beginning of i915's atomic conversion I added
> >>>> intel_crtc->atomic as a temporary dumping ground for "stuff to do
> >>>> outside vblank evasion" flags since CRTC states weren't properly wired
> >>>> up and tracked at that time.  We've had proper CRTC state tracking for a
> >>>> while now, so there's really no reason for this hack to continue to
> >>>> exist.  Moving forward we want to store intermediate crtc/plane state
> >>>> data for modesets in addition to the final state, so moving these fields
> >>>> into the proper state object allows us to properly compute them for both
> >>>> the intermediate and final state.
> >>>>
> >>>> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> >>>> ---
> >>> Can I shoot this patch down? It's better to add a field 'wm_changed'
> >>> to the crtc_state, which gets reset to false for each crtc_state
> >>> duplication. It's needed for checking if a cs pageflip can be done for
> >>> atomic. It would remove the duplication of some checks there.
> >>>
> >>> The other atomic state members will die soon. I already have some
> >>> patches to achieve that. :)
> >>>
> >>> I'm not sure if an intermediate state is a good idea. Any code that
> >>> disables a crtc should only be looking at the old state.
> >>> pre_plane_update runs all stuff in preparation for disabling planes,
> >>> while post_plane_update runs everything needed for enabling planes. So
> >>> no need to split it up I think, maybe put in some intermediate
> >>> watermarks in intel_atomic_state, but no need for a full crtc_state.
> >> Well, the intermediate state stuff was requested by Ville in response to
> >> my watermark series, so I posted these patches as an RFC to make sure I
> >> was understanding what he was looking for properly.
> >>
> >> Ville, can you comment?
> > My opinion is that the current "disable is special" way of doing things
> > is quite horrible. For one it makes it really hard to reason about what
> > happens to a plane or crtc during the modeset. It's not just off->on,
> > on->off, or same->same, but can be on->off->on. With the intermediate
> > state in place, there can only be one transition, so really easy to
> > think about what's going on.
> pre_plane_update deals with all stuff related to disabling planes, while post_plane_update deals with changes after enabling.
> 
> If the crtc goes from on -> off only you could just hammer in the final values after the disable.
> 
> While for off->on or on->off->on you can put in the final values in .crtc_enable before lighting the pipe. I don't see why wm's would need more transitions.

One special case after another. Yuck. Not to mention that the plane
disable isn't even atomic in the current code, which can look ugly.

> > It'll also mean don't have to sprinkle silly wm update calls all over
> > the modeset path. They will just get updated in response to the plane
> > state changes. Same for IPS/FBC etc.
> IPS and FBC are already calculated correctly in response to modesets.

Correctly perhaps, but not in an obvious way.

-- 
Ville Syrjälä
Intel OTC