[Intel-gfx] [PATCH] drm/i915: Introduce concept of a sub-platform
Chris Wilson
chris at chris-wilson.co.uk
Fri Mar 15 13:42:34 UTC 2019
Quoting Tvrtko Ursulin (2019-03-15 13:32:54)
>
> On 15/03/2019 13:16, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-03-15 12:26:33)
> >> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>
> >> Concept of a sub-platform already exist in our code (like ULX and ULT
> >> platform variants and similar),implemented via the macros which check a
> >> list of device ids to determine a match.
> >>
> >> With this patch we consolidate device ids checking into a single function
> >> called during early driver load.
> >>
> >> A few low bits in the platform mask are reserved for sub-platform
> >> identification and defined as a per-platform namespace.
> >>
> >> At the same time it future proofs the platform_mask handling by preparing
> >> the code for easy extending, and tidies the very verbose WARN strings
> >> generated when IS_PLATFORM macros are embedded into a WARN type
> >> statements.
> >>
> >> The approach is also beneficial to driver size, with an combined shrink of
> >> code and strings of around 1.7 kiB.
> >>
> >> v2: Fixed IS_SUBPLATFORM. Updated commit msg.
> >> v3: Chris was right, there is an ordering problem.
> >>
> >> v4:
> >> * Catch-up with new sub-platforms.
> >> * Rebase for RUNTIME_INFO.
> >> * Drop subplatform mask union tricks and convert platform_mask to an
> >> array for extensibility.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
> >> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >> Cc: Jani Nikula <jani.nikula at intel.com>
> >> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> >> Cc: Jose Souza <jose.souza at intel.com>
> >> ---
> >> drivers/gpu/drm/i915/i915_drv.c | 7 +-
> >> drivers/gpu/drm/i915/i915_drv.h | 110 +++++++++++++++--------
> >> drivers/gpu/drm/i915/i915_pci.c | 2 +-
> >> drivers/gpu/drm/i915/intel_device_info.c | 79 ++++++++++++++++
> >> drivers/gpu/drm/i915/intel_device_info.h | 28 +++++-
> >> 5 files changed, 179 insertions(+), 47 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> index 0d743907e7bc..3218350cd225 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -863,6 +863,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv)
> >> if (i915_inject_load_failure())
> >> return -ENODEV;
> >>
> >> + intel_device_info_subplatform_init(dev_priv);
> >> +
> >> spin_lock_init(&dev_priv->irq_lock);
> >> spin_lock_init(&dev_priv->gpu_error.lock);
> >> mutex_init(&dev_priv->backlight_lock);
> >> @@ -1752,10 +1754,11 @@ static void i915_welcome_messages(struct drm_i915_private *dev_priv)
> >> if (drm_debug & DRM_UT_DRIVER) {
> >> struct drm_printer p = drm_debug_printer("i915 device info:");
> >>
> >> - drm_printf(&p, "pciid=0x%04x rev=0x%02x platform=%s gen=%i\n",
> >> + drm_printf(&p, "pciid=0x%04x rev=0x%02x platform=%s (%x) gen=%i\n",
> >> INTEL_DEVID(dev_priv),
> >> INTEL_REVID(dev_priv),
> >> intel_platform_name(INTEL_INFO(dev_priv)->platform),
> >> + RUNTIME_INFO(dev_priv)->platform_mask[INTEL_INFO(dev_priv)->platform / (BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - INTEL_SUBPLATFORM_BITS)],
> >
> > I hope that's a one-off!
>
> It's indeed horrible. I was thinking of adding a helper but decided to
> wait for some general acks first.
>
> >> INTEL_GEN(dev_priv));
> >>
> >> intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
> >> @@ -1798,8 +1801,6 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent)
> >> memcpy(device_info, match_info, sizeof(*device_info));
> >> RUNTIME_INFO(i915)->device_id = pdev->device;
> >>
> >> - BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
> >> - BITS_PER_TYPE(device_info->platform_mask));
> >> BUG_ON(device_info->gen > BITS_PER_TYPE(device_info->gen_mask));
> >>
> >> return i915;
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index dccb6006aabf..34282cf66cb0 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -2281,7 +2281,46 @@ static inline unsigned int i915_sg_segment_size(void)
> >> #define IS_REVID(p, since, until) \
> >> (INTEL_REVID(p) >= (since) && INTEL_REVID(p) <= (until))
> >>
> >> -#define IS_PLATFORM(dev_priv, p) (INTEL_INFO(dev_priv)->platform_mask & BIT(p))
> >> +#define __IS_PLATFORM(dev_priv, p) \
> >> +({ \
> >> + const unsigned int pbits__ = \
> >> + BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
> >> + INTEL_SUBPLATFORM_BITS; \
> >> + const unsigned int pi__ = (p) / pbits__; \
> >> + const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
> >
> > Oh, p is a compile time constant, so these can all be evaluated at
> > compile time. So not a worry.
> >
> >> +\
> >> + BUILD_BUG_ON(!__builtin_constant_p(p)); \
> >> +\
> >> + (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & BIT(pb__)); \
> >> +})
> >> +
> >> +#define __IS_SUBPLATFORM(dev_priv, p, s) \
> >> +({ \
> >> + const unsigned int pbits__ = \
> >> + BITS_PER_TYPE(RUNTIME_INFO(dev_priv)->platform_mask[0]) - \
> >> + INTEL_SUBPLATFORM_BITS; \
> >> + const unsigned int pi__ = (p) / pbits__; \
> >> + const unsigned int pb__ = (p) % pbits__ + INTEL_SUBPLATFORM_BITS; \
> >> +\
> >> + BUILD_BUG_ON(!__builtin_constant_p(p)); \
> >> + BUILD_BUG_ON(!__builtin_constant_p(s)); \
> >> + BUILD_BUG_ON((s) >= INTEL_SUBPLATFORM_BITS); \
> >> +\
> >> + (RUNTIME_INFO(dev_priv)->platform_mask[pi__] & (BIT(pb__) | BIT(s))); \
> >> +})
> >> +
> >> +static inline bool
> >> +IS_PLATFORM(const struct drm_i915_private *i915, enum intel_platform p)
> >> +{
> >> + return __IS_PLATFORM(i915, p);
> >> +}
> >> +
> >> +static inline bool
> >> +IS_SUBPLATFORM(const struct drm_i915_private *i915,
> >> + enum intel_platform p, unsigned int s)
> >> +{
> >> + return __IS_SUBPLATFORM(i915, p, s);
> >> +}
> >
> > Ok, that all makes sense as a custom bitmap.
>
> Hm head scratch.. why I needed macro and a static inline? I'll check if
> I can get away with only the latter. This fiddling was all about keeping
> WARN_ON strings readable (and short!).
>
> >
> >> +void intel_device_info_subplatform_init(struct drm_i915_private *i915)
> >> +{
> >> + const unsigned int pbits =
> >> + BITS_PER_TYPE(RUNTIME_INFO(i915)->platform_mask[0]) -
> >> + INTEL_SUBPLATFORM_BITS;
> >> + const unsigned int pi = INTEL_INFO(i915)->platform / pbits;
> >> + const unsigned int pb =
> >> + INTEL_INFO(i915)->platform % pbits + INTEL_SUBPLATFORM_BITS;
> >> + struct intel_runtime_info *info = RUNTIME_INFO(i915);
> >> + u16 devid = INTEL_DEVID(i915);
> >> +
> >> + BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
> >> + pbits * ARRAY_SIZE(RUNTIME_INFO(i915)->platform_mask));
> >> +
> >> + info->platform_mask[pi] = BIT(pb);
> >> +
> >> + if (IS_PINEVIEW(i915)) {
> >> + if (devid == 0xa001)
> >> + info->platform_mask[pi] |=
> >> + BIT(INTEL_SUBPLATFORM_PINEVIEW_G);
> >> + else if (devid == 0xa011)
> >> + info->platform_mask[pi] |=
> >> + BIT(INTEL_SUBPLATFORM_PINEVIEW_M);
> >
> > if (IS_PINEVIEW(i915)) {
> > int subplatform = 0;
> >
> > if (devid == 0xa001)
> > subplatform = INTEL_SUBPLATFORM_PINEVIEW_G;
> > else if (devid == 0xa001)
> > subplatform = INTEL_SUBPLATFORM_PINEVIEW_M;
> > else
> > MISSING_CASE(devid);
>
> With MISSING_CASE you are talking specifically for Pineview since it
> only has a total of two ids? You expect more parts to ship? :))
>
> Otherwise the logic here is only to mention ids which are special, not
> all platform knows of. So as a general pattern MISSING_CASE wouldn't work.
Hmm. I worried about forgetting to add subplatform fields in future.
would
int subplatform = 0;
switch (devid) {
case BORING_A:
...
case BORING_Z:
break;
case SUB_AA:
...
case SUB_AZ:
subplatform = A;
break;
case SUB_BA:
...
case SUB_BZ:
subplatform = B;
break;
default:
CI_MISSING_CASE(devid);
break;
}
compile away the jumptable for the defaults when !CI?
However, that does imply that we see an example of every subplatform in
CI. Which, off course, we do!
> > info->platform_mask[pi] |= BIT(subplatform);
> > WARN_ON(!IS_SUBPLATFORM(i915, INTEL_PLATFORM_PINEVIEW, subplatform));
> > }
> >
> > So we catch missing ids, and validate subplatform against
> > SUBPLATFORM_BITS.
>
> Not so straightforward with the ULT and ULX bunch since some of those
> are both.
>
> What I have added locally however is this:
>
> GEM_BUG_ON(mask & ~INTEL_SUBPLATFORM_BITS);
>
> RUNTIME_INFO(i915)->platform_mask[pi] |= mask;
>
> Moved the mask assignment to end so that I can check subplatform did not
> overflow the allocated space.
Fair enough.
> >> /**
> >> * intel_device_info_runtime_init - initialize runtime info
> >> * @dev_priv: the i915 device
> >> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> >> index 047d10bdd455..b03fbd2e451a 100644
> >> --- a/drivers/gpu/drm/i915/intel_device_info.h
> >> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> >> @@ -44,7 +44,7 @@ enum intel_platform {
> >> INTEL_I915G,
> >> INTEL_I915GM,
> >> INTEL_I945G,
> >> - INTEL_I945GM,
> >> + INTEL_I945GM = 8,
> >> INTEL_G33,
> >> INTEL_PINEVIEW,
> >> /* gen4 */
> >> @@ -55,7 +55,7 @@ enum intel_platform {
> >> /* gen5 */
> >> INTEL_IRONLAKE,
> >> /* gen6 */
> >> - INTEL_SANDYBRIDGE,
> >> + INTEL_SANDYBRIDGE = 16,
> >> /* gen7 */
> >> INTEL_IVYBRIDGE,
> >> INTEL_VALLEYVIEW,
> >> @@ -66,7 +66,7 @@ enum intel_platform {
> >> /* gen9 */
> >> INTEL_SKYLAKE,
> >> INTEL_BROXTON,
> >> - INTEL_KABYLAKE,
> >> + INTEL_KABYLAKE = 24,
> >
> > Looks like you are just keeping a tally, and no I have no idea how to
> > add a comment to make that clear.
> >
> > /* tally */
> > /* tally so far */
>
> Yeah.. I kept re-counting to see how it will look when the array is
> expanded, whether I should reserve more suplatfoms bits right now for
> cut-off to fall somewhere reasonable.
>
> I can remove these markers, they are not that useful.
>
> >
> >> INTEL_GEMINILAKE,
> >> INTEL_COFFEELAKE,
> >> /* gen10 */
> >> @@ -76,6 +76,24 @@ enum intel_platform {
> >> INTEL_MAX_PLATFORMS
> >> };
> >>
> >> +/*
> >> + * Subplatform bits share the same namespace per parent platform. In other words
> >> + * it is fine for the same bit to be used on multiple parent platforms.
> >> + */
> >> +
> >> +#define INTEL_SUBPLATFORM_BITS (3)
> >
> >> +#define INTEL_SUBPLATFORM_IRONLAKE_M (0)
> >
> > I can't believe you haven't done i852/i855! (Or whatever that variant
> > was called.)
>
> IS_I85X? I don't see any sub-platforms there.
The gpu is slightly different; but nothing so far for us to do.
Basically, in my grand plan, where there is just one header that defines
all known pci-ids, features, and marketing strings we would need to
differentiate.
> > Couldn't spot anything wrong, and so long as subplatform remains clear
> > in the error state, hint hint, I'm happy.
>
> Like how? With a string like platform names? Would be possible but is it
> really needed on top of devid?
Because I have every devid memorized!
I'd take the subplatform %x; that's enough to see a pattern or to
investigate if might be subplatform specific.
-Chris
More information about the Intel-gfx
mailing list