From: Jani Nikula <jani.nikula@intel.com>
To: Tvrtko Ursulin <tursulin@ursulin.net>, Intel-gfx@lists.freedesktop.org
Subject: Re: [RFC v3] drm/i915: Eliminate devid sprinkle
Date: Mon, 26 Feb 2018 16:00:37 +0200 [thread overview]
Message-ID: <871sh73l6i.fsf@intel.com> (raw)
In-Reply-To: <20180222101504.4374-1-tvrtko.ursulin@linux.intel.com>
On Thu, 22 Feb 2018, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> 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.
>
> text data bss dec hex filename
> 1673630 59691 5064 1738385 1a8691 i915.ko.0
> 1673536 59691 5064 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 <tvrtko.ursulin@intel.com>
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@intel.com>
> ---
> 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_BROADWELL(dev_priv) && \
> (dev_priv)->info.gt == 3)
> -#define IS_HSW_ULT(dev_priv) (IS_HASWELL(dev_priv) && \
> - (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0A00)
> +#define IS_HSW_ULT(dev_priv) IS_SUBPLATFORM(dev_priv, INTEL_HASWELL, ULT)
> #define IS_HSW_GT3(dev_priv) (IS_HASWELL(dev_priv) && \
> (dev_priv)->info.gt == 3)
> /* ULX machines are also considered ULT. */
> -#define IS_HSW_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x0A0E || \
> - INTEL_DEVID(dev_priv) == 0x0A1E)
> -#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)
> -#define IS_SKL_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x190E || \
> - INTEL_DEVID(dev_priv) == 0x1915 || \
> - INTEL_DEVID(dev_priv) == 0x191E)
> -#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)
> -#define IS_KBL_ULX(dev_priv) (INTEL_DEVID(dev_priv) == 0x590E || \
> - INTEL_DEVID(dev_priv) == 0x5915 || \
> - INTEL_DEVID(dev_priv) == 0x591E)
> +#define IS_HSW_ULX(dev_priv) IS_SUBPLATFORM(dev_priv, INTEL_HASWELL, ULX)
> +#define IS_SKL_ULT(dev_priv) IS_SUBPLATFORM(dev_priv, INTEL_SKYLAKE, ULT)
> +#define IS_SKL_ULX(dev_priv) IS_SUBPLATFORM(dev_priv, INTEL_SKYLAKE, ULX)
> +#define IS_KBL_ULT(dev_priv) IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, ULT)
> +#define IS_KBL_ULX(dev_priv) IS_SUBPLATFORM(dev_priv, INTEL_KABYLAKE, ULX)
> #define IS_SKL_GT2(dev_priv) (IS_SKYLAKE(dev_priv) && \
> (dev_priv)->info.gt == 2)
> #define IS_SKL_GT3(dev_priv) (IS_SKYLAKE(dev_priv) && \
> @@ -2666,14 +2655,13 @@ intel_info(const struct drm_i915_private *dev_priv)
> (dev_priv)->info.gt == 2)
> #define IS_KBL_GT3(dev_priv) (IS_KABYLAKE(dev_priv) && \
> (dev_priv)->info.gt == 3)
> -#define IS_CFL_ULT(dev_priv) (IS_COFFEELAKE(dev_priv) && \
> - (INTEL_DEVID(dev_priv) & 0x00F0) == 0x00A0)
> +#define IS_CFL_ULT(dev_priv) IS_SUBPLATFORM(dev_priv, INTEL_COFFEELAKE, ULT)
> #define IS_CFL_GT2(dev_priv) (IS_COFFEELAKE(dev_priv) && \
> (dev_priv)->info.gt == 2)
> #define IS_CFL_GT3(dev_priv) (IS_COFFEELAKE(dev_priv) && \
> (dev_priv)->info.gt == 3)
> -#define IS_CNL_WITH_PORT_F(dev_priv) (IS_CANNONLAKE(dev_priv) && \
> - (INTEL_DEVID(dev_priv) & 0x0004) == 0x0004)
> +#define IS_CNL_WITH_PORT_F(dev_priv) \
> + IS_SUBPLATFORM(dev_priv, INTEL_CANNONLAKE, PORTF)
>
> #define IS_ALPHA_SUPPORT(intel_info) ((intel_info)->is_alpha_support)
>
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 1eaabf28d7b7..9e2967f7c583 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -30,6 +30,7 @@
> #include "i915_selftest.h"
>
> #define PLATFORM(x) .platform = (x), .platform_mask = BIT(x)
> +#define SUBPLATFORM(x) .subplatform_mask = BIT(INTEL_SUBPLATFORM_##x)
> #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1)
>
> #define GEN_DEFAULT_PIPEOFFSETS \
> @@ -234,6 +235,7 @@ static const struct intel_device_info intel_ironlake_d_info = {
> static const struct intel_device_info intel_ironlake_m_info = {
> GEN5_FEATURES,
> PLATFORM(INTEL_IRONLAKE),
> + SUBPLATFORM(IRONLAKE_M),
> .is_mobile = 1, .has_fbc = 1,
> };
>
> @@ -605,6 +607,7 @@ static const struct intel_device_info intel_icelake_11_info = {
>
> #undef GEN
> #undef PLATFORM
> +#undef SUBPLATFORM
>
> /*
> * Make sure any device matches here are from most specific to most
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index 298f8996cc54..23bb6f6f94d9 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);
>
> intel_device_info_dump_flags(info, p);
> @@ -458,6 +459,62 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv)
> return 0;
> }
>
> +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;
> + } else if (IS_BROADWELL(i915)) {
> + if ((devid & 0xf) == 0x6 ||
> + (devid & 0xf) == 0xb ||
> + (devid & 0xf) == 0xe)
> + info->subplatform_mask = INTEL_SUBPLATFORM_ULT;
> + /* ULX machines are also considered ULT. */
> + if ((devid & 0xf) == 0xe)
> + info->subplatform_mask |= INTEL_SUBPLATFORM_ULX;
> + } else if (IS_SKYLAKE(i915)) {
> + if (devid == 0x1906 ||
> + devid == 0x1913 ||
> + devid == 0x1916 ||
> + devid == 0x1921 ||
> + devid == 0x1926)
> + info->subplatform_mask = INTEL_SUBPLATFORM_ULT;
> + else if (devid == 0x190E ||
> + devid == 0x1915 ||
> + devid == 0x191E)
> + info->subplatform_mask = INTEL_SUBPLATFORM_ULX;
> + } else if (IS_KABYLAKE(i915)) {
> + if (devid == 0x5906 ||
> + devid == 0x5913 ||
> + devid == 0x5916 ||
> + devid == 0x5921 ||
> + devid == 0x5926)
> + info->subplatform_mask = INTEL_SUBPLATFORM_ULT;
> + else if (devid == 0x590E ||
> + devid == 0x5915 ||
> + devid == 0x591E)
> + info->subplatform_mask = INTEL_SUBPLATFORM_ULX;
> + } else if (IS_COFFEELAKE(i915)) {
> + if ((devid & 0x00F0) == 0x00A0)
> + info->subplatform_mask = INTEL_SUBPLATFORM_ULT;
> + } else if (IS_CANNONLAKE(i915)) {
> + if ((devid & 0x0004) == 0x0004)
> + info->subplatform_mask = INTEL_SUBPLATFORM_PORTF;
> + }
> +}
> +
> /**
> * intel_device_info_runtime_init - initialize runtime info
> * @info: intel device info struct
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index 71fdfb0451ef..2732c03ad0cb 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -74,6 +74,23 @@ 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 platform.
> + */
> +
> +#define INTEL_SUBPLATFORM_BITS (2)
> +
> +#define INTEL_SUBPLATFORM_IRONLAKE_M (0)
> +
> +#define INTEL_SUBPLATFORM_PINEVIEW_G (0)
> +#define INTEL_SUBPLATFORM_PINEVIEW_M (1)
> +
> +#define INTEL_SUBPLATFORM_ULT (0)
> +#define INTEL_SUBPLATFORM_ULX (1)
> +
> +#define INTEL_SUBPLATFORM_PORTF (0)
> +
> #define DEV_INFO_FOR_EACH_FLAG(func) \
> func(is_mobile); \
> func(is_lp); \
> @@ -135,7 +152,14 @@ struct intel_device_info {
> u8 ring_mask; /* Rings supported by the HW */
>
> enum intel_platform platform;
> - u32 platform_mask;
> +
> + union {
> + u32 platform_subplatform_mask;
> + struct {
> + u32 platform_mask : (32 - INTEL_SUBPLATFORM_BITS);
> + u32 subplatform_mask : INTEL_SUBPLATFORM_BITS;
> + };
> + };
>
> u32 display_mmio_offset;
>
> @@ -178,6 +202,7 @@ static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
>
> const char *intel_platform_name(enum intel_platform platform);
>
> +void intel_device_info_subplatform_init(struct intel_device_info *info);
> void intel_device_info_runtime_init(struct intel_device_info *info);
> void intel_device_info_dump(const struct intel_device_info *info,
> struct drm_printer *p);
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2018-02-26 14:00 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-22 8:09 [RFC] drm/i915: Eliminate devid sprinkle Tvrtko Ursulin
2018-02-22 8:16 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2018-02-22 8:24 ` [RFC] " Chris Wilson
2018-02-22 10:39 ` Tvrtko Ursulin
2018-02-22 8:30 ` ✗ Fi.CI.BAT: warning for " Patchwork
2018-02-22 8:35 ` Chris Wilson
2018-02-22 8:59 ` Tvrtko Ursulin
2018-02-22 9:05 ` [RFC v2] " Tvrtko Ursulin
2018-02-22 10:15 ` [RFC v3] " Tvrtko Ursulin
2018-02-22 10:27 ` Chris Wilson
2018-02-22 10:59 ` Chris Wilson
2018-02-26 14:00 ` Jani Nikula [this message]
2018-02-26 15:53 ` Chris Wilson
2018-02-26 16:08 ` Jani Nikula
2018-02-22 9:25 ` ✗ Fi.CI.BAT: warning for drm/i915: Eliminate devid sprinkle (rev2) Patchwork
2018-02-22 10:47 ` ✗ Fi.CI.BAT: warning for drm/i915: Eliminate devid sprinkle (rev3) Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=871sh73l6i.fsf@intel.com \
--to=jani.nikula@intel.com \
--cc=Intel-gfx@lists.freedesktop.org \
--cc=tursulin@ursulin.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox