Re: [RFC] drm/i915: Eliminate devid sprinkle

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

 



Quoting Tvrtko Ursulin (2018-02-22 08:09:07)
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> 
> 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).
> 
>    text    data     bss     dec     hex filename
> 1673630   59691    5064 1738385  1a8691 i915.ko.0
> 1673214   59691    5064 1737969  1a84f1 i915.ko.1
> 
> 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>
> ---
> @@ -2624,38 +2631,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)

Killme. :)

> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index 298f8996cc54..f5c9d29a7471 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -111,10 +111,11 @@ void intel_device_info_dump(const struct intel_device_info *info,
>         struct drm_i915_private *dev_priv =
>                 container_of(info, struct drm_i915_private, 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 (subplatform=%x) gen=%i\n",
>                    INTEL_DEVID(dev_priv),
>                    INTEL_REVID(dev_priv),
>                    intel_platform_name(info->platform),
> +                  info->subplatform_mask,
>                    info->gen);

Worth it.

>  
>         intel_device_info_dump_flags(info, p);
> @@ -458,6 +459,62 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv)
>         return 0;
>  }
>  
> +static 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)) {

> -#define IS_PINEVIEW_G(dev_priv)        (INTEL_DEVID(dev_priv) == 0xa001)
> -#define IS_PINEVIEW_M(dev_priv)        (INTEL_DEVID(dev_priv) == 0xa011)

> +               if (devid == 0xa001)
> +                       info->subplatform_mask = INTEL_SUBPLATFORM_PINEVIEW_G;
> +               else if (devid == 0xa011)
> +                       info->subplatform_mask = INTEL_SUBPLATFORM_PINEVIEW_M;

Ok. Do we use IS_PINEVIEW_G/M? Should just be IS_PINEVIEW() &&
IS_MOBILE() but I guess after this change, IS_PINEVIEW_G/M is even
easier.

> +       } 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;
> +       } else if (IS_BROADWELL(i915)) {

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

> +               if ((devid & 0xf) == 0x6 ||
> +                   (devid & 0xf) == 0xb ||
> +                   (devid & 0xf) == 0xe)
> +                       info->subplatform_mask = INTEL_SUBPLATFORM_ULT;

> -#define IS_BDW_ULX(dev_priv)   (IS_BROADWELL(dev_priv) && \
> -                                (INTEL_DEVID(dev_priv) & 0xf) == 0xe)

> +               /* ULX machines are also considered ULT. */
> +               if ((devid & 0xf) == 0xe)
> +                       info->subplatform_mask |= INTEL_SUBPLATFORM_ULX;

Ok.

> +       } else if (IS_SKYLAKE(i915)) {

> -#define IS_SKL_ULT(dev_priv)   (INTEL_DEVID(dev_priv) == 0x1906 || \
> -                                INTEL_DEVID(dev_priv) == 0x1913 || \
> -                                INTEL_DEVID(dev_priv) == 0x1916 || \
> -                                INTEL_DEVID(dev_priv) == 0x1921 || \
> -                                INTEL_DEVID(dev_priv) == 0x1926)

> +               if (devid == 0x1906 ||
> +                   devid == 0x1913 ||
> +                   devid == 0x1916 ||
> +                   devid == 0x1921 ||
> +                   devid == 0x1926)
> +                       info->subplatform_mask = INTEL_SUBPLATFORM_ULT;

> -#define IS_SKL_ULX(dev_priv)   (INTEL_DEVID(dev_priv) == 0x190E || \
> -                                INTEL_DEVID(dev_priv) == 0x1915 || \
> -                                INTEL_DEVID(dev_priv) == 0x191E)

> +               else if (devid == 0x190E ||
> +                        devid == 0x1915 ||
> +                        devid == 0x191E)
> +                       info->subplatform_mask = INTEL_SUBPLATFORM_ULX;

Ok.

> +       } else if (IS_KABYLAKE(i915)) {

> -#define IS_KBL_ULT(dev_priv)   (INTEL_DEVID(dev_priv) == 0x5906 || \
> -                                INTEL_DEVID(dev_priv) == 0x5913 || \
> -                                INTEL_DEVID(dev_priv) == 0x5916 || \
> -                                INTEL_DEVID(dev_priv) == 0x5921 || \
> -                                INTEL_DEVID(dev_priv) == 0x5926)

> +               if (devid == 0x5906 ||
> +                   devid == 0x5913 ||
> +                   devid == 0x5916 ||
> +                   devid == 0x5921 ||
> +                   devid  == 0x5926)
> +                       info->subplatform_mask = INTEL_SUBPLATFORM_ULT;

> -#define IS_KBL_ULX(dev_priv)   (INTEL_DEVID(dev_priv) == 0x590E || \
> -                                INTEL_DEVID(dev_priv) == 0x5915 || \
> -                                INTEL_DEVID(dev_priv) == 0x591E)

> +               else if (devid == 0x590E ||
> +                        devid == 0x5915 ||
> +                        devid == 0x591E)
> +                       info->subplatform_mask = INTEL_SUBPLATFORM_ULX;

Ok.

> +       } else if (IS_COFFEELAKE(i915)) {

> -#define IS_CFL_ULT(dev_priv)   (IS_COFFEELAKE(dev_priv) && \
> -                                (INTEL_DEVID(dev_priv) & 0x00F0) == 0x00A0)

> +                if ((devid & 0x00F0) == 0x00A0)
> +                       info->subplatform_mask = INTEL_SUBPLATFORM_ULT;

Ok.

> +       } else if (IS_CANNONLAKE(i915)) {

> -#define IS_CNL_WITH_PORT_F(dev_priv)   (IS_CANNONLAKE(dev_priv) && \
> -                                       (INTEL_DEVID(dev_priv) & 0x0004) == 0x0004)

> +               if ((devid & 0x0004) == 0x0004)
> +                       info->subplatform_mask = INTEL_SUBPLATFORM_PORTF;

Odd, but ok. (Wondering if this needs it own bit. It doesn't, ok.)

> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index 71fdfb0451ef..7b6211061fba 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -74,6 +74,20 @@ enum intel_platform {
>         INTEL_MAX_PLATFORMS
>  };
>  
> +/* Subplatform flags share the same namespace per parent platform. */
> +
> +#define INTEL_SUBPLATFORM_BITS (2)

Enough space to do the same for GT (4 bits?) on top?

Looks good to me. I'm moaning at mkwrite_device_info() but that can
stand for now ;)

Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux