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@xxxxxxxxxxxxxxx>
> >>>> ---
> >>>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
