| Message ID | 20190315004235.23711-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Drop platform_mask | expand |
Quoting José Roberto de Souza (2019-03-15 00:42:35) > We don't have any platform that is composed by 2 or more platforms so > we don't need a mask, lets drop it and remove the actual limit of 32 > platforms. gcc doesn't entirely agree, this is a net loss here (i.e. code size increases). -Chris
On 15/03/2019 00:52, Chris Wilson wrote: > Quoting José Roberto de Souza (2019-03-15 00:42:35) >> We don't have any platform that is composed by 2 or more platforms so >> we don't need a mask, lets drop it and remove the actual limit of 32 >> platforms. Platform mask was a nifty trick to compile tests like IS_SKYLAKE || IS_BROADWELL etc into a single conditional. > gcc doesn't entirely agree, this is a net loss here (i.e. code size > increases). Perhaps the size re-gain of dropping the platform mask could be checked against the size gain of making the mask 64 bit. However, one other benefit of the mask will also help with dead code elimination once per SKU build work is resurfaced. For the single selected platforms it doesn't matter, but for a subset it still does I think. So I think we got two questions here - checking between size gains of the two options, and how interesting would multi-platform builds be. One use case for the latter I had in mind was legacy vs execlists driver build but that wasn't based on any formal feature requests as far as I am aware. Regards, Tvrtko
On 15/03/2019 06:56, Tvrtko Ursulin wrote: > > On 15/03/2019 00:52, Chris Wilson wrote: >> Quoting José Roberto de Souza (2019-03-15 00:42:35) >>> We don't have any platform that is composed by 2 or more platforms so >>> we don't need a mask, lets drop it and remove the actual limit of 32 >>> platforms. > > Platform mask was a nifty trick to compile tests like IS_SKYLAKE || > IS_BROADWELL etc into a single conditional. > >> gcc doesn't entirely agree, this is a net loss here (i.e. code size >> increases). > > Perhaps the size re-gain of dropping the platform mask could be checked > against the size gain of making the mask 64 bit. One possible alternative could be splitting the 64-bit platform mask into two 32-bit dwords. Like: u32 platform_mask[2]; #define IS_PLATFORM(p) (platform_mask[p / 32] & BIT(p % 32)) And a bit of jigging the enum space so that we don't end up with something often tested together on the dword boundary. In fact I had a prototype many months ago which went a step further and interleaved platform with gen mask, in some sized chunks, so even IS_PLATFORM || IS_GEN checks could be merged. This included the sub-platform thing as well with the ULT/ULX/LP stuff I think.. maybe I need to dig this out to see how it worked. Regards, Tvrtko
On Fri, Mar 15, 2019 at 12:13 AM Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > > On 15/03/2019 06:56, Tvrtko Ursulin wrote: > > > > On 15/03/2019 00:52, Chris Wilson wrote: > >> Quoting José Roberto de Souza (2019-03-15 00:42:35) > >>> We don't have any platform that is composed by 2 or more platforms so > >>> we don't need a mask, lets drop it and remove the actual limit of 32 > >>> platforms. > > > > Platform mask was a nifty trick to compile tests like IS_SKYLAKE || > > IS_BROADWELL etc into a single conditional. > > > >> gcc doesn't entirely agree, this is a net loss here (i.e. code size > >> increases). > > > > Perhaps the size re-gain of dropping the platform mask could be checked > > against the size gain of making the mask 64 bit. current: text data bss dec hex filename 1836533 40454 4176 1881163 1cb44b drivers/gpu/drm/i915/i915.o.old 64 bit bitmask: 1836757 40454 4176 1881387 1cb52b drivers/gpu/drm/i915/i915.o no platform_mask: 1837591 40454 4176 1882221 1cb86d drivers/gpu/drm/i915/i915.o So current situation to "just use a number" we are talking about 1k here, which for me looks acceptable. Alternative is the 64-bit bitmask, with ~200 bytes. Lucas De Marchi > > One possible alternative could be splitting the 64-bit platform mask > into two 32-bit dwords. Like: > > u32 platform_mask[2]; > > #define IS_PLATFORM(p) (platform_mask[p / 32] & BIT(p % 32)) > > And a bit of jigging the enum space so that we don't end up with > something often tested together on the dword boundary. > > In fact I had a prototype many months ago which went a step further and > interleaved platform with gen mask, in some sized chunks, so even > IS_PLATFORM || IS_GEN checks could be merged. This included the > sub-platform thing as well with the ULT/ULX/LP stuff I think.. maybe I > need to dig this out to see how it worked. > > Regards, > > Tvrtko > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Lucas De Marchi
On 15/03/2019 07:27, Lucas De Marchi wrote: > On Fri, Mar 15, 2019 at 12:13 AM Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: >> >> >> On 15/03/2019 06:56, Tvrtko Ursulin wrote: >>> >>> On 15/03/2019 00:52, Chris Wilson wrote: >>>> Quoting José Roberto de Souza (2019-03-15 00:42:35) >>>>> We don't have any platform that is composed by 2 or more platforms so >>>>> we don't need a mask, lets drop it and remove the actual limit of 32 >>>>> platforms. >>> >>> Platform mask was a nifty trick to compile tests like IS_SKYLAKE || >>> IS_BROADWELL etc into a single conditional. >>> >>>> gcc doesn't entirely agree, this is a net loss here (i.e. code size >>>> increases). >>> >>> Perhaps the size re-gain of dropping the platform mask could be checked >>> against the size gain of making the mask 64 bit. > > current: > text data bss dec hex filename > 1836533 40454 4176 1881163 1cb44b > drivers/gpu/drm/i915/i915.o.old > > 64 bit bitmask: > 1836757 40454 4176 1881387 1cb52b drivers/gpu/drm/i915/i915.o > > no platform_mask: > 1837591 40454 4176 1882221 1cb86d > drivers/gpu/drm/i915/i915.o > > > So current situation to "just use a number" we are talking about 1k > here, which for me looks acceptable. Alternative is the 64-bit > bitmask, with ~200 bytes. Maybe yeah, but there was also the second part about per SKU builds. Let me dig out the sub platform patch and modify it to split out the mask into two dwords and see how that would look. Regards, Tvrtko > > Lucas De Marchi > > > >> >> One possible alternative could be splitting the 64-bit platform mask >> into two 32-bit dwords. Like: >> >> u32 platform_mask[2]; >> >> #define IS_PLATFORM(p) (platform_mask[p / 32] & BIT(p % 32)) >> >> And a bit of jigging the enum space so that we don't end up with >> something often tested together on the dword boundary. >> >> In fact I had a prototype many months ago which went a step further and >> interleaved platform with gen mask, in some sized chunks, so even >> IS_PLATFORM || IS_GEN checks could be merged. This included the >> sub-platform thing as well with the ULT/ULX/LP stuff I think.. maybe I >> need to dig this out to see how it worked. >> >> Regards, >> >> Tvrtko >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Lucas De Marchi >
Em sex, 2019-03-15 às 06:56 +0000, Tvrtko Ursulin escreveu: > On 15/03/2019 00:52, Chris Wilson wrote: > > Quoting José Roberto de Souza (2019-03-15 00:42:35) > > > We don't have any platform that is composed by 2 or more platforms so > > > we don't need a mask, lets drop it and remove the actual limit of 32 > > > platforms. > > Platform mask was a nifty trick to compile tests like IS_SKYLAKE || > IS_BROADWELL etc into a single conditional. > So perhaps the code would benefit from a small comment that says exactly that, so the next person looking at it won't propose its removal again. José, would you volunteer for that patch? > > gcc doesn't entirely agree, this is a net loss here (i.e. code size > > increases). > > Perhaps the size re-gain of dropping the platform mask could be checked > against the size gain of making the mask 64 bit. > > However, one other benefit of the mask will also help with dead code > elimination once per SKU build work is resurfaced. For the single > selected platforms it doesn't matter, but for a subset it still does I > think. > > So I think we got two questions here - checking between size gains of > the two options, and how interesting would multi-platform builds be. > > One use case for the latter I had in mind was legacy vs execlists driver > build but that wasn't based on any formal feature requests as far as I > am aware. > > Regards, > > Tvrtko > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Mar 15, 2019 at 07:13:49AM +0000, Tvrtko Ursulin wrote: > > On 15/03/2019 06:56, Tvrtko Ursulin wrote: > > > > On 15/03/2019 00:52, Chris Wilson wrote: > >> Quoting José Roberto de Souza (2019-03-15 00:42:35) > >>> We don't have any platform that is composed by 2 or more platforms so > >>> we don't need a mask, lets drop it and remove the actual limit of 32 > >>> platforms. > > > > Platform mask was a nifty trick to compile tests like IS_SKYLAKE || > > IS_BROADWELL etc into a single conditional. > > > >> gcc doesn't entirely agree, this is a net loss here (i.e. code size > >> increases). > > > > Perhaps the size re-gain of dropping the platform mask could be checked > > against the size gain of making the mask 64 bit. > > One possible alternative could be splitting the 64-bit platform mask > into two 32-bit dwords. Like: > > u32 platform_mask[2]; > > #define IS_PLATFORM(p) (platform_mask[p / 32] & BIT(p % 32)) This is fast approaching nih bitmap.h territory.
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 0d743907e7bc..60df3e0eb2d1 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1798,8 +1798,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..f9ff055fb9cb 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2281,7 +2281,7 @@ 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) (INTEL_INFO(dev_priv)->platform == (p)) #define IS_I830(dev_priv) IS_PLATFORM(dev_priv, INTEL_I830) #define IS_I845G(dev_priv) IS_PLATFORM(dev_priv, INTEL_I845G) diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c index 3cf697e8f1fa..2b042618e3ca 100644 --- a/drivers/gpu/drm/i915/i915_pci.c +++ b/drivers/gpu/drm/i915/i915_pci.c @@ -32,7 +32,7 @@ #include "i915_globals.h" #include "i915_selftest.h" -#define PLATFORM(x) .platform = (x), .platform_mask = BIT(x) +#define PLATFORM(x) .platform = (x) #define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1) #define I845_PIPE_OFFSETS \ diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h index 047d10bdd455..547439698d98 100644 --- a/drivers/gpu/drm/i915/intel_device_info.h +++ b/drivers/gpu/drm/i915/intel_device_info.h @@ -160,7 +160,6 @@ struct intel_device_info { intel_engine_mask_t engine_mask; /* Engines supported by the HW */ enum intel_platform platform; - u32 platform_mask; enum intel_ppgtt ppgtt; unsigned int page_sizes; /* page sizes supported by the HW */
We don't have any platform that is composed by 2 or more platforms so we don't need a mask, lets drop it and remove the actual limit of 32 platforms. Cc: Lucas De Marchi <lucas.demarchi@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 2 -- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/i915_pci.c | 2 +- drivers/gpu/drm/i915/intel_device_info.h | 1 - 4 files changed, 2 insertions(+), 5 deletions(-)