Re: [PATCH] drm/i915: Introduce concept of a sub-platform

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Tvrtko Ursulin (2019-03-15 12:26:33)
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> Suggested-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> Cc: Jose Souza <jose.souza@xxxxxxxxx>
> ---
>  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!

>                            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.

> +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);

	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.

>  /**
>   * 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 */

>         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.)

Couldn't spot anything wrong, and so long as subplatform remains clear
in the error state, hint hint, I'm happy.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux