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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 16-03-17 om 09:47 schreef 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@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.
We don't run the state comparison until
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx