Re: [PATCH 2/3] drm/i915: Promote IS_BROADWELL to a simple macro

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/3] drm/i915: Promote IS_BROADWELL to a simple macro
Date: Fri, 6 May 2016 15:51:11 +0100	[thread overview]
Message-ID: <20160506145111.GH30822@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <572CADB5.900@linux.intel.com>

On Fri, May 06, 2016 at 03:44:05PM +0100, Tvrtko Ursulin wrote:
> 
> On 06/05/16 15:29, Ville Syrjälä wrote:
> >On Fri, May 06, 2016 at 03:18:16PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 06/05/16 15:00, Ville Syrjälä wrote:
> >>>On Fri, May 06, 2016 at 02:43:50PM +0100, Tvrtko Ursulin wrote:
> >>>>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>
> >>>>If we allow it a dedicated flag in dev_priv we enable the
> >>>>compiler to nicely optimize conditions like IS_HASSWELL ||
> >>>>IS_BROADWELL.
> >>>>
> >>>>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>---
> >>>>   drivers/gpu/drm/i915/i915_drv.c | 4 ++++
> >>>>   drivers/gpu/drm/i915/i915_drv.h | 3 ++-
> >>>>   2 files changed, 6 insertions(+), 1 deletion(-)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >>>>index 9fd221c97275..da6532da44e3 100644
> >>>>--- a/drivers/gpu/drm/i915/i915_drv.c
> >>>>+++ b/drivers/gpu/drm/i915/i915_drv.c
> >>>>@@ -300,22 +300,26 @@ static const struct intel_device_info intel_haswell_m_info = {
> >>>>   static const struct intel_device_info intel_broadwell_d_info = {
> >>>>   	BDW_FEATURES,
> >>>>   	.gen = 8,
> >>>>+	.is_broadwell = 1,
> >>>>   };
> >>>>
> >>>>   static const struct intel_device_info intel_broadwell_m_info = {
> >>>>   	BDW_FEATURES,
> >>>>   	.gen = 8, .is_mobile = 1,
> >>>>+	.is_broadwell = 1,
> >>>>   };
> >>>>
> >>>>   static const struct intel_device_info intel_broadwell_gt3d_info = {
> >>>>   	BDW_FEATURES,
> >>>>   	.gen = 8,
> >>>>+	.is_broadwell = 1,
> >>>>   	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
> >>>>   };
> >>>>
> >>>>   static const struct intel_device_info intel_broadwell_gt3m_info = {
> >>>>   	BDW_FEATURES,
> >>>>   	.gen = 8, .is_mobile = 1,
> >>>>+	.is_broadwell = 1,
> >>>>   	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
> >>>>   };
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>>>index c6351016eaf0..459561991081 100644
> >>>>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>>>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>>>@@ -735,6 +735,7 @@ struct intel_csr {
> >>>>   	func(is_valleyview) sep \
> >>>>   	func(is_cherryview) sep \
> >>>>   	func(is_haswell) sep \
> >>>>+	func(is_broadwell) sep \
> >>>>   	func(is_skylake) sep \
> >>>>   	func(is_broxton) sep \
> >>>>   	func(is_kabylake) sep \
> >>>
> >>>We could just replace all of these with a single enum and save a bunch
> >>>of space.
> >>
> >>Bitfield is really nice here because compiler can then merge tests like
> >>IS_HASWELL || IS_BROADWELL into a single test instruction.
> >
> >Most if not all of those are in modeset code so I'm not entirely
> >convinced micro-optimizing that is all that useful.
> 
> Maybe not, but IS_BROADWELL was just tasteless and it is better to
> save instructions across everything than a bit in dev_priv.
> 
> >Bunch of times I've just wished I could write 'whatever >= G4X' instead
> >of 'IS_G4X || gen >= 5' simply for readability.
> 
> I don't even know what G4X is so can't really follow what you mean.
> Is it like Gen4.5, or Gen5-- ? So I can't figure out what kind of
> scheme you propose.

It's gen4.5. A quick refresh of the "eccentric" gen4.

The problem we always run into with a single gen field is that the GPU,
display engine and various bits and pieces have had a divergenet
evolution, e.g. Valleyview has a gen7 gpu with a gen4 display block.

We keep planning to separate the two, and that is mostly done today
through the distinction of IS_GENx() and IS_BROADWELL() etc, and we are
often better served by adding a specific feature flag rather than depend
upon a version field.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx