[Intel-gfx] [PATCH v3 07/20] drm/i915: Rework plane readout.

[Daniel Vetter]

On Tue, Jul 14, 2015 at 12:30:38PM +0200, Maarten Lankhorst wrote:
> Op 14-07-15 om 11:53 schreef Daniel Vetter:
> > On Mon, Jul 13, 2015 at 04:30:20PM +0200, Maarten Lankhorst wrote:
> >> All non-primary planes get disabled during hw readout,
> >> this reduces complexity and means not having to do some plane
> >> visibility checks during the first commit.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > I still think calling readout_plane_state from the hw state readout code
> > is the wrong approach. Instead we should consolidate all the plane
> > readout code exclusively into a new intel_plane_readout_hw_state helper
> > which is called only from driver load paths. That should also contain the
> > fb/gem_bo reconstruction loop.
> Is that a nack?

Nope, just a "I think we want to clarify this more". I'd welcome a patch
on top, but can be done as part of converting dpms (since that will result
in lots of cleanups too).

> > For the other 2 users of this code (lid notifiery and resume) we should
> > just force an unconditional plane restore by setting
> > crtc_state->planes_chagned. But I thinkt hat's ok as some follow-up work,
> > once atomic has settled a bit more.
> >
> If you want to force a plane restore, planes_changed won't do what you think it does.
> planes_changed is a flag that says one or more planes were added to the crtc_state.
> 
> You want to do drm_atomic_add_affected_planes for that, and atomic resume would do that
> since it duplicates all plane state.

Ah right, so should be possible to just consolidate the plane readout code
without requiring any changes in the force_restore case.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch