| Message ID | 1462885341-30380-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 10 May 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > With this scheme all call sites of IS_SKL_REVID and IS_BXT_REVID > result in a maximum of one conditional jump instruction (was > three before) and overall reduction in code size. IMO we really shouldn't bother with this. The end goal is that we remove all of these when we decide we no longer care about early (non-production) revisions for a platform. For Skylake I'd argue we are beyond this point. BR, Jani. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/i915_dma.c | 11 +++++++++++ > drivers/gpu/drm/i915/i915_drv.h | 24 +++++++++++++++++++----- > 2 files changed, 30 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 46ac1da64a09..1ae812436827 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1072,6 +1072,17 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv, > memcpy(device_info, info, sizeof(dev_priv->info)); > device_info->device_id = dev->pdev->device; > > + /* For unknown revisions mask stays at zero which is correct. */ > + if (dev->pdev->revision < > + sizeof(device_info->revision_mask) * BITS_PER_BYTE) > + device_info->revision_mask = BIT(dev->pdev->revision); > + > + if (IS_SKYLAKE(dev_priv)) > + device_info->skl_rev_mask = ~0; > + > + if (IS_BROXTON(dev_priv)) > + device_info->bxt_rev_mask = ~0; > + > spin_lock_init(&dev_priv->irq_lock); > spin_lock_init(&dev_priv->gpu_error.lock); > mutex_init(&dev_priv->backlight_lock); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 26e7de415966..fbeb68e9ec96 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -757,6 +757,9 @@ struct intel_csr { > struct intel_device_info { > u32 display_mmio_offset; > u16 device_id; > + u8 revision_mask; > + u8 skl_rev_mask; > + u8 bxt_rev_mask; > u8 num_pipes:3; > u8 num_sprites[I915_MAX_PIPES]; > u8 gen; > @@ -2519,14 +2522,23 @@ struct drm_i915_cmd_table { > #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id) > #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision) > > -#define REVID_FOREVER 0xff > +#define REVID_FOREVER 0 > + > /* > * Return true if revision is in range [since,until] inclusive. > * > * Use 0 for open-ended since, and REVID_FOREVER for open-ended until. > */ > -#define IS_REVID(p, since, until) \ > - (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until)) > +#define IS_REVID(p, mask, since, until) ({\ > + unsigned int __s = (since), __e = (until); \ > + BUILD_BUG_ON(!__builtin_constant_p(since)); \ > + BUILD_BUG_ON(!__builtin_constant_p(until)); \ > + BUILD_BUG_ON(since >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \ > + BUILD_BUG_ON(until >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \ > + if ((__e) == REVID_FOREVER) \ > + __e = BITS_PER_LONG - 1; \ > + !!(INTEL_INFO(p)->revision_mask & (mask) & GENMASK((__e), (__s))); \ > +}) > > #define IS_I830(dev) (INTEL_DEVID(dev) == 0x3577) > #define IS_845G(dev) (INTEL_DEVID(dev) == 0x2562) > @@ -2605,14 +2617,16 @@ struct drm_i915_cmd_table { > #define SKL_REVID_E0 0x4 > #define SKL_REVID_F0 0x5 > > -#define IS_SKL_REVID(p, since, until) (IS_SKYLAKE(p) && IS_REVID(p, since, until)) > +#define IS_SKL_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->skl_rev_mask, \ > + since, until) > > #define BXT_REVID_A0 0x0 > #define BXT_REVID_A1 0x1 > #define BXT_REVID_B0 0x3 > #define BXT_REVID_C0 0x9 > > -#define IS_BXT_REVID(p, since, until) (IS_BROXTON(p) && IS_REVID(p, since, until)) > +#define IS_BXT_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->bxt_rev_mask, \ > + since, until) > > /* > * The genX designation typically refers to the render engine, so render
Hi, On 10/05/16 14:33, Jani Nikula wrote: > On Tue, 10 May 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> With this scheme all call sites of IS_SKL_REVID and IS_BXT_REVID >> result in a maximum of one conditional jump instruction (was >> three before) and overall reduction in code size. > > IMO we really shouldn't bother with this. The end goal is that we remove > all of these when we decide we no longer care about early > (non-production) revisions for a platform. For Skylake I'd argue we are > beyond this point. Thought that simply removing everything guarded by IS_SKL_REVID might be possible never crossed my mind. :) Which one is the first production stepping then? Regards, Tvrtko. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/i915_dma.c | 11 +++++++++++ >> drivers/gpu/drm/i915/i915_drv.h | 24 +++++++++++++++++++----- >> 2 files changed, 30 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c >> index 46ac1da64a09..1ae812436827 100644 >> --- a/drivers/gpu/drm/i915/i915_dma.c >> +++ b/drivers/gpu/drm/i915/i915_dma.c >> @@ -1072,6 +1072,17 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv, >> memcpy(device_info, info, sizeof(dev_priv->info)); >> device_info->device_id = dev->pdev->device; >> >> + /* For unknown revisions mask stays at zero which is correct. */ >> + if (dev->pdev->revision < >> + sizeof(device_info->revision_mask) * BITS_PER_BYTE) >> + device_info->revision_mask = BIT(dev->pdev->revision); >> + >> + if (IS_SKYLAKE(dev_priv)) >> + device_info->skl_rev_mask = ~0; >> + >> + if (IS_BROXTON(dev_priv)) >> + device_info->bxt_rev_mask = ~0; >> + >> spin_lock_init(&dev_priv->irq_lock); >> spin_lock_init(&dev_priv->gpu_error.lock); >> mutex_init(&dev_priv->backlight_lock); >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 26e7de415966..fbeb68e9ec96 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -757,6 +757,9 @@ struct intel_csr { >> struct intel_device_info { >> u32 display_mmio_offset; >> u16 device_id; >> + u8 revision_mask; >> + u8 skl_rev_mask; >> + u8 bxt_rev_mask; >> u8 num_pipes:3; >> u8 num_sprites[I915_MAX_PIPES]; >> u8 gen; >> @@ -2519,14 +2522,23 @@ struct drm_i915_cmd_table { >> #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id) >> #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision) >> >> -#define REVID_FOREVER 0xff >> +#define REVID_FOREVER 0 >> + >> /* >> * Return true if revision is in range [since,until] inclusive. >> * >> * Use 0 for open-ended since, and REVID_FOREVER for open-ended until. >> */ >> -#define IS_REVID(p, since, until) \ >> - (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until)) >> +#define IS_REVID(p, mask, since, until) ({\ >> + unsigned int __s = (since), __e = (until); \ >> + BUILD_BUG_ON(!__builtin_constant_p(since)); \ >> + BUILD_BUG_ON(!__builtin_constant_p(until)); \ >> + BUILD_BUG_ON(since >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \ >> + BUILD_BUG_ON(until >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \ >> + if ((__e) == REVID_FOREVER) \ >> + __e = BITS_PER_LONG - 1; \ >> + !!(INTEL_INFO(p)->revision_mask & (mask) & GENMASK((__e), (__s))); \ >> +}) >> >> #define IS_I830(dev) (INTEL_DEVID(dev) == 0x3577) >> #define IS_845G(dev) (INTEL_DEVID(dev) == 0x2562) >> @@ -2605,14 +2617,16 @@ struct drm_i915_cmd_table { >> #define SKL_REVID_E0 0x4 >> #define SKL_REVID_F0 0x5 >> >> -#define IS_SKL_REVID(p, since, until) (IS_SKYLAKE(p) && IS_REVID(p, since, until)) >> +#define IS_SKL_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->skl_rev_mask, \ >> + since, until) >> >> #define BXT_REVID_A0 0x0 >> #define BXT_REVID_A1 0x1 >> #define BXT_REVID_B0 0x3 >> #define BXT_REVID_C0 0x9 >> >> -#define IS_BXT_REVID(p, since, until) (IS_BROXTON(p) && IS_REVID(p, since, until)) >> +#define IS_BXT_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->bxt_rev_mask, \ >> + since, until) >> >> /* >> * The genX designation typically refers to the render engine, so render >
On Tue, 10 May 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > Hi, > > On 10/05/16 14:33, Jani Nikula wrote: >> On Tue, 10 May 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>> With this scheme all call sites of IS_SKL_REVID and IS_BXT_REVID >>> result in a maximum of one conditional jump instruction (was >>> three before) and overall reduction in code size. >> >> IMO we really shouldn't bother with this. The end goal is that we remove >> all of these when we decide we no longer care about early >> (non-production) revisions for a platform. For Skylake I'd argue we are >> beyond this point. > > Thought that simply removing everything guarded by IS_SKL_REVID might be > possible never crossed my mind. :) Which one is the first production > stepping then? None of the ones we list and check for i.e. we should replace all of the IS_SKL_REVID() uses with either 0 or IS_SKYLAKE(), and simplify from there. Maybe we should check the revid once on driver load, and whine about early hardware. We've had such checks in the past on earlier platforms. This might save our precious time, as we shouldn't be debugging issues reported against pre-production hardware. Ville here says dropping the checks is a good time to verify the workarounds we do have in place are correct going forward. BR, Jani. > > Regards, > > Tvrtko. > >>> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Jani Nikula <jani.nikula@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_dma.c | 11 +++++++++++ >>> drivers/gpu/drm/i915/i915_drv.h | 24 +++++++++++++++++++----- >>> 2 files changed, 30 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c >>> index 46ac1da64a09..1ae812436827 100644 >>> --- a/drivers/gpu/drm/i915/i915_dma.c >>> +++ b/drivers/gpu/drm/i915/i915_dma.c >>> @@ -1072,6 +1072,17 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv, >>> memcpy(device_info, info, sizeof(dev_priv->info)); >>> device_info->device_id = dev->pdev->device; >>> >>> + /* For unknown revisions mask stays at zero which is correct. */ >>> + if (dev->pdev->revision < >>> + sizeof(device_info->revision_mask) * BITS_PER_BYTE) >>> + device_info->revision_mask = BIT(dev->pdev->revision); >>> + >>> + if (IS_SKYLAKE(dev_priv)) >>> + device_info->skl_rev_mask = ~0; >>> + >>> + if (IS_BROXTON(dev_priv)) >>> + device_info->bxt_rev_mask = ~0; >>> + >>> spin_lock_init(&dev_priv->irq_lock); >>> spin_lock_init(&dev_priv->gpu_error.lock); >>> mutex_init(&dev_priv->backlight_lock); >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>> index 26e7de415966..fbeb68e9ec96 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.h >>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>> @@ -757,6 +757,9 @@ struct intel_csr { >>> struct intel_device_info { >>> u32 display_mmio_offset; >>> u16 device_id; >>> + u8 revision_mask; >>> + u8 skl_rev_mask; >>> + u8 bxt_rev_mask; >>> u8 num_pipes:3; >>> u8 num_sprites[I915_MAX_PIPES]; >>> u8 gen; >>> @@ -2519,14 +2522,23 @@ struct drm_i915_cmd_table { >>> #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id) >>> #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision) >>> >>> -#define REVID_FOREVER 0xff >>> +#define REVID_FOREVER 0 >>> + >>> /* >>> * Return true if revision is in range [since,until] inclusive. >>> * >>> * Use 0 for open-ended since, and REVID_FOREVER for open-ended until. >>> */ >>> -#define IS_REVID(p, since, until) \ >>> - (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until)) >>> +#define IS_REVID(p, mask, since, until) ({\ >>> + unsigned int __s = (since), __e = (until); \ >>> + BUILD_BUG_ON(!__builtin_constant_p(since)); \ >>> + BUILD_BUG_ON(!__builtin_constant_p(until)); \ >>> + BUILD_BUG_ON(since >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \ >>> + BUILD_BUG_ON(until >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \ >>> + if ((__e) == REVID_FOREVER) \ >>> + __e = BITS_PER_LONG - 1; \ >>> + !!(INTEL_INFO(p)->revision_mask & (mask) & GENMASK((__e), (__s))); \ >>> +}) >>> >>> #define IS_I830(dev) (INTEL_DEVID(dev) == 0x3577) >>> #define IS_845G(dev) (INTEL_DEVID(dev) == 0x2562) >>> @@ -2605,14 +2617,16 @@ struct drm_i915_cmd_table { >>> #define SKL_REVID_E0 0x4 >>> #define SKL_REVID_F0 0x5 >>> >>> -#define IS_SKL_REVID(p, since, until) (IS_SKYLAKE(p) && IS_REVID(p, since, until)) >>> +#define IS_SKL_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->skl_rev_mask, \ >>> + since, until) >>> >>> #define BXT_REVID_A0 0x0 >>> #define BXT_REVID_A1 0x1 >>> #define BXT_REVID_B0 0x3 >>> #define BXT_REVID_C0 0x9 >>> >>> -#define IS_BXT_REVID(p, since, until) (IS_BROXTON(p) && IS_REVID(p, since, until)) >>> +#define IS_BXT_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->bxt_rev_mask, \ >>> + since, until) >>> >>> /* >>> * The genX designation typically refers to the render engine, so render >>
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 46ac1da64a09..1ae812436827 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1072,6 +1072,17 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv, memcpy(device_info, info, sizeof(dev_priv->info)); device_info->device_id = dev->pdev->device; + /* For unknown revisions mask stays at zero which is correct. */ + if (dev->pdev->revision < + sizeof(device_info->revision_mask) * BITS_PER_BYTE) + device_info->revision_mask = BIT(dev->pdev->revision); + + if (IS_SKYLAKE(dev_priv)) + device_info->skl_rev_mask = ~0; + + if (IS_BROXTON(dev_priv)) + device_info->bxt_rev_mask = ~0; + spin_lock_init(&dev_priv->irq_lock); spin_lock_init(&dev_priv->gpu_error.lock); mutex_init(&dev_priv->backlight_lock); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 26e7de415966..fbeb68e9ec96 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -757,6 +757,9 @@ struct intel_csr { struct intel_device_info { u32 display_mmio_offset; u16 device_id; + u8 revision_mask; + u8 skl_rev_mask; + u8 bxt_rev_mask; u8 num_pipes:3; u8 num_sprites[I915_MAX_PIPES]; u8 gen; @@ -2519,14 +2522,23 @@ struct drm_i915_cmd_table { #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id) #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision) -#define REVID_FOREVER 0xff +#define REVID_FOREVER 0 + /* * Return true if revision is in range [since,until] inclusive. * * Use 0 for open-ended since, and REVID_FOREVER for open-ended until. */ -#define IS_REVID(p, since, until) \ - (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until)) +#define IS_REVID(p, mask, since, until) ({\ + unsigned int __s = (since), __e = (until); \ + BUILD_BUG_ON(!__builtin_constant_p(since)); \ + BUILD_BUG_ON(!__builtin_constant_p(until)); \ + BUILD_BUG_ON(since >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \ + BUILD_BUG_ON(until >= sizeof(INTEL_INFO(p)->revision_mask) * BITS_PER_BYTE); \ + if ((__e) == REVID_FOREVER) \ + __e = BITS_PER_LONG - 1; \ + !!(INTEL_INFO(p)->revision_mask & (mask) & GENMASK((__e), (__s))); \ +}) #define IS_I830(dev) (INTEL_DEVID(dev) == 0x3577) #define IS_845G(dev) (INTEL_DEVID(dev) == 0x2562) @@ -2605,14 +2617,16 @@ struct drm_i915_cmd_table { #define SKL_REVID_E0 0x4 #define SKL_REVID_F0 0x5 -#define IS_SKL_REVID(p, since, until) (IS_SKYLAKE(p) && IS_REVID(p, since, until)) +#define IS_SKL_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->skl_rev_mask, \ + since, until) #define BXT_REVID_A0 0x0 #define BXT_REVID_A1 0x1 #define BXT_REVID_B0 0x3 #define BXT_REVID_C0 0x9 -#define IS_BXT_REVID(p, since, until) (IS_BROXTON(p) && IS_REVID(p, since, until)) +#define IS_BXT_REVID(p, since, until) IS_REVID(p, INTEL_INFO(p)->bxt_rev_mask, \ + since, until) /* * The genX designation typically refers to the render engine, so render