From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 28/71] drm/i915/chv: Added CHV specific register read and write Date: Wed, 9 Apr 2014 16:32:10 +0300 Message-ID: <20140409133210.GO18465@intel.com> References: <1397039349-10639-1-git-send-email-ville.syrjala@linux.intel.com> <1397039349-10639-29-git-send-email-ville.syrjala@linux.intel.com> <20140409131604.GB4573@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id 2BE606EB61 for ; Wed, 9 Apr 2014 06:32:41 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140409131604.GB4573@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, Apr 09, 2014 at 02:16:04PM +0100, Chris Wilson wrote: > On Wed, Apr 09, 2014 at 01:28:26PM +0300, ville.syrjala@linux.intel.com w= rote: > > From: Deepak S > > = > > Support to individually control Media/Render well based on the register= access. > > Add CHV specific write function to habdle difference between registers > > that are sadowed vs those that need forcewake even for writes. > > = > > v2: Drop write FIFO for CHV and add comman well forcewake (Ville) > > = > > Signed-off-by: Deepak S > > [vsyrjala: Move the register range macros into intel_uncore.c] > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > drivers/gpu/drm/i915/intel_uncore.c | 139 ++++++++++++++++++++++++++++= +++++--- > > 1 file changed, 131 insertions(+), 8 deletions(-) > > = > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915= /intel_uncore.c > > index 823d699..8e3c686 100644 > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > @@ -495,6 +495,31 @@ void assert_force_wake_inactive(struct drm_i915_pr= ivate *dev_priv) > > ((reg) >=3D 0x22000 && (reg) < 0x24000) ||\ > > ((reg) >=3D 0x30000 && (reg) < 0x40000)) > > = > > +#define FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg) \ > > + (((reg) >=3D 0x2000 && (reg) < 0x4000) ||\ > > + ((reg) >=3D 0x5000 && (reg) < 0x8000) ||\ > > + ((reg) >=3D 0x8300 && (reg) < 0x8500) ||\ > > + ((reg) >=3D 0xB000 && (reg) < 0xC000) ||\ > > + ((reg) >=3D 0xE000 && (reg) < 0xE800)) > > + > > +#define FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)\ > > + (((reg) >=3D 0x8800 && (reg) < 0x8900) ||\ > > + ((reg) >=3D 0xD000 && (reg) < 0xD800) ||\ > > + ((reg) >=3D 0x12000 && (reg) < 0x14000) ||\ > > + ((reg) >=3D 0x1A000 && (reg) < 0x1C000) ||\ > > + ((reg) >=3D 0x1E800 && (reg) < 0x1EA00) ||\ > > + ((reg) >=3D 0x30000 && (reg) < 0x40000)) > > + > > +#define FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)\ > > + (((reg) >=3D 0x4000 && (reg) < 0x5000) ||\ > > + ((reg) >=3D 0x8000 && (reg) < 0x8300) ||\ > > + ((reg) >=3D 0x8500 && (reg) < 0x8600) ||\ > > + ((reg) >=3D 0x9000 && (reg) < 0xB000) ||\ > > + ((reg) >=3D 0xC000 && (reg) < 0xc800) ||\ > > + ((reg) >=3D 0xF000 && (reg) < 0x10000) ||\ > > + ((reg) >=3D 0x14000 && (reg) < 0x14400) ||\ > > + ((reg) >=3D 0x22000 && (reg) < 0x24000)) > = > You could write this as a single stanza of > = > if (reg < foo) fwengine =3D BAR; = > else if (reg < baz) ... > = > which I think would not only help with us reviewing it, but wouldn't > generate so nearly as bad code. For me that looks harder to read as you can't see each range neatly on one line anymore. But you may be right about getting better code out of it. I must admit I've never checked what kind of code gcc generates for these things. Another idea would be to use a switch statement with case ranges, but that doesn't seem really different to the current approach. -- = Ville Syrj=E4l=E4 Intel OTC