Re: [Intel-gfx] [RFC v3] drm/i915: Eliminate devid sprinkle
On Mon, 26 Feb 2018, Chris Wilson wrote: > Quoting Jani Nikula (2018-02-26 14:00:37) >> On Thu, 22 Feb 2018, Tvrtko Ursulin wrote: >> > From: Tvrtko Ursulin >> > >> > Introduce subplatform mask to eliminate throughout the code devid checking >> > sprinkle, mostly courtesy of IS_*_UL[TX] macros. >> > >> > Subplatform mask initialization is moved either to static tables (Ironlake >> > M) or runtime device info init (Pineview, Haswell, Broadwell, Skylake, >> > Kabylake, Coffeelake and Cannonlake). >> >> I thought Chris had the goal of separating runtime and static init, and >> I very much agreed with that idea. Throw away the mkwrite stuff. This >> patch seems to be at odds with that goal by tying a runtime init into >> the same mask with statically initialized platform mask. > > Yes. > > In the extreme version of single platform LTO, we would bake one > device-info stanza for every subplatform. That may be a little overkill > (or rather too complicated for the user to know and too fine-grained to > be useful). So the middle ground is that we have subplatform in the > runtime_info, and the compiler has to do two loads. We can still benefit > from using BIT() though. Well, let's not conflate platform and subplatform into the same mask in the same info for the short-term benefit, then? And with that, I think we could go towards: #define IS_BWD_ULT(dev_priv) (IS_BROADWELL(dev_priv) && IS_ULT(dev_priv)) where the IS_ULT would just check the subplatform mask. I am not convinced it's worth overloading the subplatform bits. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC v3] drm/i915: Eliminate devid sprinkle
Quoting Jani Nikula (2018-02-26 14:00:37) > On Thu, 22 Feb 2018, Tvrtko Ursulin wrote: > > From: Tvrtko Ursulin > > > > Introduce subplatform mask to eliminate throughout the code devid checking > > sprinkle, mostly courtesy of IS_*_UL[TX] macros. > > > > Subplatform mask initialization is moved either to static tables (Ironlake > > M) or runtime device info init (Pineview, Haswell, Broadwell, Skylake, > > Kabylake, Coffeelake and Cannonlake). > > I thought Chris had the goal of separating runtime and static init, and > I very much agreed with that idea. Throw away the mkwrite stuff. This > patch seems to be at odds with that goal by tying a runtime init into > the same mask with statically initialized platform mask. Yes. In the extreme version of single platform LTO, we would bake one device-info stanza for every subplatform. That may be a little overkill (or rather too complicated for the user to know and too fine-grained to be useful). So the middle ground is that we have subplatform in the runtime_info, and the compiler has to do two loads. We can still benefit from using BIT() though. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC v3] drm/i915: Eliminate devid sprinkle
On Thu, 22 Feb 2018, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin > > Introduce subplatform mask to eliminate throughout the code devid checking > sprinkle, mostly courtesy of IS_*_UL[TX] macros. > > Subplatform mask initialization is moved either to static tables (Ironlake > M) or runtime device info init (Pineview, Haswell, Broadwell, Skylake, > Kabylake, Coffeelake and Cannonlake). I thought Chris had the goal of separating runtime and static init, and I very much agreed with that idea. Throw away the mkwrite stuff. This patch seems to be at odds with that goal by tying a runtime init into the same mask with statically initialized platform mask. I could bikeshed some details, but that's by far the biggest concern I have that should be resolved first. BR, Jani. > >textdata bss dec hex filename > 1673630 596915064 1738385 1a8691 i915.ko.0 > 1673536 596915064 1738291 1a8633 i915.ko.1 > > v2: Fixed IS_SUBPLATFORM. Updated commit msg. > v3: Chris was right, there is an ordering problem. > > Signed-off-by: Tvrtko Ursulin > Suggested-by: Chris Wilson > Cc: Chris Wilson > Cc: Jani Nikula > --- > drivers/gpu/drm/i915/i915_drv.c | 5 ++- > drivers/gpu/drm/i915/i915_drv.h | 58 +-- > drivers/gpu/drm/i915/i915_pci.c | 3 ++ > drivers/gpu/drm/i915/intel_device_info.c | 59 > +++- > drivers/gpu/drm/i915/intel_device_info.h | 27 ++- > 5 files changed, 114 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index aaa861b51024..f6c2e67257c9 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -904,8 +904,11 @@ static int i915_driver_init_early(struct > drm_i915_private *dev_priv, > memcpy(device_info, match_info, sizeof(*device_info)); > device_info->device_id = dev_priv->drm.pdev->device; > > + intel_device_info_subplatform_init(device_info); > + > BUILD_BUG_ON(INTEL_MAX_PLATFORMS > > - sizeof(device_info->platform_mask) * BITS_PER_BYTE); > + sizeof(device_info->platform_subplatform_mask) * > + BITS_PER_BYTE - INTEL_SUBPLATFORM_BITS); > BUG_ON(device_info->gen > sizeof(device_info->gen_mask) * > BITS_PER_BYTE); > spin_lock_init(&dev_priv->irq_lock); > spin_lock_init(&dev_priv->gpu_error.lock); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 82a106b1bdbc..808d957ce9ba 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2587,7 +2587,11 @@ intel_info(const struct drm_i915_private *dev_priv) > #define IS_REVID(p, since, until) \ > (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until)) > > -#define IS_PLATFORM(dev_priv, p) ((dev_priv)->info.platform_mask & BIT(p)) > +#define IS_PLATFORM(dev_priv, p) ((dev_priv)->info.platform_subplatform_mask > & BIT(p)) > +#define IS_SUBPLATFORM(dev_priv, p, s) \ > + (IS_PLATFORM(dev_priv, p) && \ > + ((dev_priv)->info.platform_subplatform_mask & \ > + BIT(32 - INTEL_SUBPLATFORM_BITS + INTEL_SUBPLATFORM_##s))) > > #define IS_I830(dev_priv)IS_PLATFORM(dev_priv, INTEL_I830) > #define IS_I845G(dev_priv) IS_PLATFORM(dev_priv, INTEL_I845G) > @@ -2602,11 +2606,15 @@ intel_info(const struct drm_i915_private *dev_priv) > #define IS_G45(dev_priv) IS_PLATFORM(dev_priv, INTEL_G45) > #define IS_GM45(dev_priv)IS_PLATFORM(dev_priv, INTEL_GM45) > #define IS_G4X(dev_priv) (IS_G45(dev_priv) || IS_GM45(dev_priv)) > -#define IS_PINEVIEW_G(dev_priv) (INTEL_DEVID(dev_priv) == 0xa001) > -#define IS_PINEVIEW_M(dev_priv) (INTEL_DEVID(dev_priv) == 0xa011) > +#define IS_PINEVIEW_G(dev_priv) \ > + IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, PINEVIEW_G) > +#define IS_PINEVIEW_M(dev_priv) \ > + IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, PINEVIEW_M) > #define IS_PINEVIEW(dev_priv)IS_PLATFORM(dev_priv, INTEL_PINEVIEW) > #define IS_G33(dev_priv) IS_PLATFORM(dev_priv, INTEL_G33) > -#define IS_IRONLAKE_M(dev_priv) (INTEL_DEVID(dev_priv) == 0x0046) > +#define IS_IRONLAKE(dev_priv)IS_PLATFORM(dev_priv, INTEL_IRONLAKE) > +#define IS_IRONLAKE_M(dev_priv) \ > + IS_SUBPLATFORM(dev_priv, INTEL_IRONLAKE, IRONLAKE_M) > #define IS_IVYBRIDGE(dev_priv) IS_PLATFORM(dev_priv, INTEL_IVYBRIDGE) > #define IS_IVB_GT1(dev_priv) (IS_IVYBRIDGE(dev_priv) && \ >(dev_priv)->info.gt == 1) > @@ -2624,38 +2632,19 @@ intel_info(const struct drm_i915_private *dev_priv) > #define IS_MOBILE(dev_priv) ((dev_priv)->info.is_mobile) > #define IS_HSW_EARLY_SDV(dev_priv) (IS_HASWELL(dev_priv) && \ > (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0C00) > -#define IS_BDW_ULT(dev_priv) (IS_BROADWELL(dev_priv) && \ > - ((INTEL_DEVID(dev_priv) &
Re: [Intel-gfx] [RFC v3] drm/i915: Eliminate devid sprinkle
Quoting Tvrtko Ursulin (2018-02-22 10:15:04)
> +void intel_device_info_subplatform_init(struct intel_device_info *info)
> +{
> + struct drm_i915_private *i915 =
> + container_of(info, struct drm_i915_private, info);
> + u16 devid = INTEL_DEVID(i915);
> +
> + if (IS_PINEVIEW(i915)) {
> + if (devid == 0xa001)
> + info->subplatform_mask = INTEL_SUBPLATFORM_PINEVIEW_G;
> + else if (devid == 0xa011)
> + info->subplatform_mask = INTEL_SUBPLATFORM_PINEVIEW_M;
> + } else if (IS_HASWELL(i915)) {
> + if ((devid & 0xFF00) == 0x0A00)
> + info->subplatform_mask = INTEL_SUBPLATFORM_ULT;
> + /* ULX machines are also considered ULT. */
> + if (devid == 0x0A0E || devid == 0x0A1E)
> + info->subplatform_mask |= INTEL_SUBPLATFORM_ULX;
Ah. Needs BIT(ULT) | BIT(ULX).
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC v3] drm/i915: Eliminate devid sprinkle
Quoting Tvrtko Ursulin (2018-02-22 10:15:04) > +#define INTEL_SUBPLATFORM_IRONLAKE_M (0) > + > +#define INTEL_SUBPLATFORM_PINEVIEW_G (0) > +#define INTEL_SUBPLATFORM_PINEVIEW_M (1) Looking at these, we can reduce these to IS_MOBILE. Clearer before or after this conversion? Pretty orthogonal I think. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC v3] drm/i915: Eliminate devid sprinkle
From: Tvrtko Ursulin Introduce subplatform mask to eliminate throughout the code devid checking sprinkle, mostly courtesy of IS_*_UL[TX] macros. Subplatform mask initialization is moved either to static tables (Ironlake M) or runtime device info init (Pineview, Haswell, Broadwell, Skylake, Kabylake, Coffeelake and Cannonlake). textdata bss dec hex filename 1673630 596915064 1738385 1a8691 i915.ko.0 1673536 596915064 1738291 1a8633 i915.ko.1 v2: Fixed IS_SUBPLATFORM. Updated commit msg. v3: Chris was right, there is an ordering problem. Signed-off-by: Tvrtko Ursulin Suggested-by: Chris Wilson Cc: Chris Wilson Cc: Jani Nikula --- drivers/gpu/drm/i915/i915_drv.c | 5 ++- drivers/gpu/drm/i915/i915_drv.h | 58 +-- drivers/gpu/drm/i915/i915_pci.c | 3 ++ drivers/gpu/drm/i915/intel_device_info.c | 59 +++- drivers/gpu/drm/i915/intel_device_info.h | 27 ++- 5 files changed, 114 insertions(+), 38 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index aaa861b51024..f6c2e67257c9 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -904,8 +904,11 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv, memcpy(device_info, match_info, sizeof(*device_info)); device_info->device_id = dev_priv->drm.pdev->device; + intel_device_info_subplatform_init(device_info); + BUILD_BUG_ON(INTEL_MAX_PLATFORMS > -sizeof(device_info->platform_mask) * BITS_PER_BYTE); +sizeof(device_info->platform_subplatform_mask) * +BITS_PER_BYTE - INTEL_SUBPLATFORM_BITS); BUG_ON(device_info->gen > sizeof(device_info->gen_mask) * BITS_PER_BYTE); spin_lock_init(&dev_priv->irq_lock); spin_lock_init(&dev_priv->gpu_error.lock); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 82a106b1bdbc..808d957ce9ba 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2587,7 +2587,11 @@ intel_info(const struct drm_i915_private *dev_priv) #define IS_REVID(p, since, until) \ (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until)) -#define IS_PLATFORM(dev_priv, p) ((dev_priv)->info.platform_mask & BIT(p)) +#define IS_PLATFORM(dev_priv, p) ((dev_priv)->info.platform_subplatform_mask & BIT(p)) +#define IS_SUBPLATFORM(dev_priv, p, s) \ + (IS_PLATFORM(dev_priv, p) && \ +((dev_priv)->info.platform_subplatform_mask & \ +BIT(32 - INTEL_SUBPLATFORM_BITS + INTEL_SUBPLATFORM_##s))) #define IS_I830(dev_priv) IS_PLATFORM(dev_priv, INTEL_I830) #define IS_I845G(dev_priv) IS_PLATFORM(dev_priv, INTEL_I845G) @@ -2602,11 +2606,15 @@ intel_info(const struct drm_i915_private *dev_priv) #define IS_G45(dev_priv) IS_PLATFORM(dev_priv, INTEL_G45) #define IS_GM45(dev_priv) IS_PLATFORM(dev_priv, INTEL_GM45) #define IS_G4X(dev_priv) (IS_G45(dev_priv) || IS_GM45(dev_priv)) -#define IS_PINEVIEW_G(dev_priv)(INTEL_DEVID(dev_priv) == 0xa001) -#define IS_PINEVIEW_M(dev_priv)(INTEL_DEVID(dev_priv) == 0xa011) +#define IS_PINEVIEW_G(dev_priv)\ + IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, PINEVIEW_G) +#define IS_PINEVIEW_M(dev_priv)\ + IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, PINEVIEW_M) #define IS_PINEVIEW(dev_priv) IS_PLATFORM(dev_priv, INTEL_PINEVIEW) #define IS_G33(dev_priv) IS_PLATFORM(dev_priv, INTEL_G33) -#define IS_IRONLAKE_M(dev_priv)(INTEL_DEVID(dev_priv) == 0x0046) +#define IS_IRONLAKE(dev_priv) IS_PLATFORM(dev_priv, INTEL_IRONLAKE) +#define IS_IRONLAKE_M(dev_priv)\ + IS_SUBPLATFORM(dev_priv, INTEL_IRONLAKE, IRONLAKE_M) #define IS_IVYBRIDGE(dev_priv) IS_PLATFORM(dev_priv, INTEL_IVYBRIDGE) #define IS_IVB_GT1(dev_priv) (IS_IVYBRIDGE(dev_priv) && \ (dev_priv)->info.gt == 1) @@ -2624,38 +2632,19 @@ intel_info(const struct drm_i915_private *dev_priv) #define IS_MOBILE(dev_priv)((dev_priv)->info.is_mobile) #define IS_HSW_EARLY_SDV(dev_priv) (IS_HASWELL(dev_priv) && \ (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0C00) -#define IS_BDW_ULT(dev_priv) (IS_BROADWELL(dev_priv) && \ -((INTEL_DEVID(dev_priv) & 0xf) == 0x6 || \ -(INTEL_DEVID(dev_priv) & 0xf) == 0xb || \ -(INTEL_DEVID(dev_priv) & 0xf) == 0xe)) -/* ULX machines are also considered ULT. */ -#define IS_BDW_ULX(dev_priv) (IS_BROADWELL(dev_priv) && \ -(INTEL_DEVID(dev_priv) & 0xf) == 0xe) +#define IS_BDW_ULT(dev_priv) IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, ULT) +#define IS_BDW_ULX(dev_priv) IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, ULX) #define IS_BDW_GT3(dev_priv) (IS_BROADWEL
