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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

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.

> 
> 
> Matt
> 
> > 
> > After a modeset disable you should be able to put in any wm value in
> > .crtc_enable because no plane will be active.
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx