From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v2 07/16] drm/i915: Add vblank based delayed watermark update mechanism Date: Mon, 9 Jun 2014 18:01:35 +0300 Message-ID: <20140609150135.GR27580@intel.com> References: <1400770101-14277-1-git-send-email-ville.syrjala@linux.intel.com> <1400770101-14277-8-git-send-email-ville.syrjala@linux.intel.com> <20140603193229.GY27580@intel.com> <20140604161044.GN7416@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 1E5596E036 for ; Mon, 9 Jun 2014 08:04:15 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140604161044.GN7416@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org 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=E4l=E4 Intel OTC