On Wed, Jun 04, 2014 at 06:10:44PM +0200, Daniel Vetter wrote: > On Wed, Jun 04, 2014 at 11:01:01AM -0300, Paulo Zanoni wrote: > > > This function is only called at init/resume. It populates the software > > > state with something that matches the current hardware state. I guess > > > a comment explaning the purpose of the function is the best we can do > > > here, or do you have a better idea? > > > > The problem is that we can use the get_hw_state functions not only to > > check driver state a init/resume, but also to do state tracking > > assertions at certain points of the code. Since most (all?) the other > > HW state readout functions don't have side-effects, there's a > > possibility that someone may add code to do HW state assertion at some > > points, and just call these things without realizing the potential > > side effects. A comment would help, but moving the assignment to > > another place would also solve the problem for me. You choose. > > We already do that in other places, e.g. the edp bit depth hacks we have. > But a comment might be justified. > > I haven't looked at the details here, just a high-level comment that we do > play such ugly tricks already. And I agree that it's unexpected. Well IIRC I didn't even call this function get_hw_state() until "someone" asked me to rename it ;) I could rename it back to whatever it was originally. It was never meant to be just a "read hardware state into a temp structure" type of thing since it's been updating intel_crtc->wm.active from the start. With the rename I should be able to drop the ugly underscore from _ilk_wm_get_hw_state(). -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
