2013/10/15 Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>: > On Tue, 15 Oct 2013 17:47:20 -0300 > Paulo Zanoni <przanoni@xxxxxxxxx> wrote: > >> 2013/10/15 Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>: >> > On Tue, 15 Oct 2013 16:54:00 -0300 >> > Paulo Zanoni <przanoni@xxxxxxxxx> wrote: >> > >> >> 2013/10/14 Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>: >> >> > When accessing the display regs for hw state readout or cross check, we >> >> > need to make sure the power well is enabled so we can read valid >> >> > register state. >> >> >> >> On the current code (HSW) we already check for the power wells in the >> >> HW state readout code: if the power well is disabled, then transcoders >> >> A/B/C are disabled. The current code takes care to not touch registers >> >> on a disabled power well. >> > >> > Yeah and we should probably preserve that behavior, for the readout >> > code at least. >> > >> >> > + intel_display_power_put(dev, POWER_DOMAIN_PIPE(0)); >> >> > + >> >> >> >> Why is this here? It certainly deserves a comment in the code. >> > >> > It probably needs to be removed. The refcounting for the initial load >> > is still screwy due to the unconditional enable later on. >> > >> >> So now HSW uses global_resources while VLV uses crtc enable/disable. I >> >> really think both platforms should try to do the same thing. >> > >> > Definitely agreed. >> > >> >> By the way, at least on Haswell, if we do an equivalent change we'll >> >> have problems, because when you disable the power well at >> >> crtc_disable, then everything you did at crtc_mode_set will be undone, >> >> and it won't be redone at crtc_enable. When you reenable the power >> >> well, the registers go back to their default values, not the values >> >> that were previously there. Did you check if VLV behaves the same? >> > >> > No that's taken into account here. In __intel_set_mode we take a >> > private ref on the appropriate power well so that we'll preserve state >> > until we do the first crtc_enable. From then on, the ref is tracked >> > there and we drop the private one in __intel_set_mode >> >> Yeah, but then after all this is done, at some point we'll call >> crtc_disable, then we'll put the last ref back and then the power well >> will be disabled. Then the user moves the mouse and we call >> crtc_enable, so we enable the power well, all its registers to back to >> their reset state, then crtc_enable sets some of the registers, but >> that won't really be enough since no one called crtc_mode_set again, >> and all the state set by crtc_mode_set (not touched by crtc_enable) is >> back to the default values. No? What am I missing? > > That's where the save/restore state of the later patch comes in. We > need that if we're going to support DPMS and runtime suspend w/o a mode > set. Oh... I wasn't even thinking about it since it's on a later patch. But makes sense. But instead of the save/restore thing, shouldn't we just move all the code that touches registers from mode_set to crtc_enable? IMHO it's the shiny solution here. And anything else that we may need to save/restore should be moved to crtc enable/disable and be saved in "struct intel_crtc" every time it is touched. I really think that removing stuff from the save/restore code is the way to go. Also, with my suggestion, crtc_enable will fully implement the "mode set sequence" from BSpec, so people like the Audio guys will have an easier time understanding our code :) > > -- > Jesse Barnes, Intel Open Source Technology Center -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
