[Intel-gfx] [PATCH 4/5] drm/i915: Use new atomic iterator macros in display code

[Daniel Vetter]

On Mon, Mar 13, 2017 at 12:08:19PM +0100, Maarten Lankhorst wrote:
> Op 13-03-17 om 11:15 schreef Daniel Vetter:
> > On Thu, Mar 09, 2017 at 03:52:04PM +0100, Maarten Lankhorst wrote:
> >> Add a big fat warning in __intel_display_resume that the old state is
> >> invalid, and use the correct state everywhere.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 161 ++++++++++++++++++-----------------
> > This file is too big. Because it's one big mess this patch here is way too
> > big and one big mess :(
> >
> >>  1 file changed, 82 insertions(+), 79 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 5526a196e8a2..83f86cf44f66 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -3464,7 +3464,12 @@ __intel_display_resume(struct drm_device *dev,
> >>  	if (!state)
> >>  		return 0;
> >>  
> >> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> >> +	/*
> >> +	 * We've duplicated the state, pointers to the old state are invalid.
> >> +	 *
> >> +	 * Don't attempt to use the old state until we commit the duplicated state.
> >> +	 */
> >> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > Just a drive-by comment, but why exactly do we need this hack? We read out
> > full hw state like on boot-up, assuming our commit machinery computes the
> > diff correctly it should noticed that we have to do a full commit here.
> >
> > Hacks like this imo show that we still have a fairly significant design
> > issue in our fastbook code ...
> We read out the full hw state, but we cannot trust the hw state to be correct. In general when we poke a property we set mode_changed,
> and the fastboot code may downgrade it to a pipe update if it's compatible enough. We only check on mode_changed because not all
> planes and connectors may otherwise be part of the atomic state.

drm_atomic.c doesn't set mode_changed anywhere, it's all derived state.
And we call the full atomic commit, which mean we do re-compute all the
derived stuff like mode_changed. I still don't get why we need this.

Iirc on the fastboot side we had some hacks to dirty plane states to force
this stuff, the same hw readout functions should do the trick here too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch