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

[Maarten Lankhorst]

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