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

[Daniel Vetter]

On Wed, Mar 15, 2017 at 09:23:30AM +0100, Maarten Lankhorst wrote:
> Op 14-03-17 om 07:50 schreef 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.
> In the core resume, all crtc's are disabled on suspend, and are assumed to be off on resume.
> 
> But with i915 hw readout, the hw may also be resumed by bios and by chance the connectors from bios match up to the state we
> want to set, the mode also being the same.
> 
> The only thing different could be (for example) crtc_state->has_audio or lane configuration, the bios might not set the same. We have to
> force a modeset (which might be downgraded to a fastset) in order to notice that this has to be restored as well.

But we compare crtc_states and upgrade them to a modeset already when e.g.
has_audio changed. Or at least we should.

This smells like you're papering over a bug in our atomic_check code
still. The state compare stuff /should/ be able to figure this out. If
it's broken, then we might run into this in normal modesets too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch