[PATCH] drm/i915: Drop platform_mask

[PATCH] drm/i915: Drop platform_mask

[José Roberto de Souza]

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

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Drop platform_mask

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Drop platform_mask

[Patchwork]

== Series Details ==

Series: drm/i915: Drop platform_mask
URL   : https://patchwork.freedesktop.org/series/58030/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5750 -> Patchwork_12472
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/58030/revisions/1/mbox/

Known issues
------------

  Here are the changes found in Patchwork_12472 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@cs-gfx:
    - fi-skl-6700k2:      NOTRUN -> SKIP [fdo#109271] +41

  * igt@amdgpu/amd_basic@cs-multi-fence:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +27

  * igt@amdgpu/amd_basic@cs-sdma:
    - fi-cfl-8109u:       NOTRUN -> SKIP [fdo#109271] +37

  * igt@amdgpu/amd_cs_nop@sync-fork-compute0:
    - fi-icl-u3:          NOTRUN -> SKIP [fdo#109315] +17

  * igt@gem_ctx_param@basic:
    - fi-gdg-551:         NOTRUN -> SKIP [fdo#109271] +106

  * igt@gem_exec_basic@basic-bsd2:
    - fi-icl-u3:          NOTRUN -> SKIP [fdo#109276] +7

  * igt@gem_exec_basic@gtt-bsd1:
    - fi-bxt-j4205:       NOTRUN -> SKIP [fdo#109271] +47

  * igt@gem_exec_basic@gtt-bsd2:
    - fi-byt-clapper:     NOTRUN -> SKIP [fdo#109271] +57

  * igt@gem_exec_basic@readonly-bsd:
    - fi-pnv-d510:        NOTRUN -> SKIP [fdo#109271] +76

  * igt@gem_exec_parse@basic-rejected:
    - fi-icl-u3:          NOTRUN -> SKIP [fdo#109289] +1

  * igt@gem_exec_store@basic-bsd1:
    - fi-kbl-r:           NOTRUN -> SKIP [fdo#109271] +41

  * igt@gem_workarounds@basic-read:
    - fi-snb-2600:        NOTRUN -> SKIP [fdo#109271] +57

  * igt@i915_selftest@live_contexts:
    - fi-icl-u3:          NOTRUN -> DMESG-FAIL [fdo#108569]

  * igt@kms_busy@basic-flip-c:
    - fi-gdg-551:         NOTRUN -> SKIP [fdo#109271] / [fdo#109278]
    - fi-pnv-d510:        NOTRUN -> SKIP [fdo#109271] / [fdo#109278]
    - fi-snb-2600:        NOTRUN -> SKIP [fdo#109271] / [fdo#109278]
    - fi-byt-clapper:     NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_chamelium@hdmi-crc-fast:
    - fi-icl-u3:          NOTRUN -> SKIP [fdo#109284] +8

  * igt@kms_force_connector_basic@force-edid:
    - fi-icl-u3:          NOTRUN -> SKIP [fdo#109285] +3

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u3:          NOTRUN -> FAIL [fdo#103167]

  * igt@runner@aborted:
    - fi-bxt-dsi:         NOTRUN -> FAIL [fdo#109516]

  
#### Possible fixes ####

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109294]: https://bugs.freedesktop.org/show_bug.cgi?id=109294
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109516]: https://bugs.freedesktop.org/show_bug.cgi?id=109516
  [fdo#110028]: https://bugs.freedesktop.org/show_bug.cgi?id=110028


Participating hosts (35 -> 35)
------------------------------

  Additional (11): fi-bxt-dsi fi-bxt-j4205 fi-gdg-551 fi-icl-u3 fi-pnv-d510 fi-icl-y fi-cfl-8109u fi-byt-clapper fi-kbl-r fi-skl-6700k2 fi-snb-2600 
  Missing    (11): fi-kbl-soraka fi-hsw-4770r fi-ilk-m540 fi-bsw-n3050 fi-hsw-4200u fi-hsw-peppy fi-byt-squawks fi-icl-u2 fi-apl-guc fi-bsw-kefka fi-bdw-samus 


Build changes
-------------

    * Linux: CI_DRM_5750 -> Patchwork_12472

  CI_DRM_5750: 2b6425f8c26cb2578d408f7c21e7ac92f83c0b4a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4886: de53202ae4b5747b86ccda22986dbeb47f65d732 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12472: b28aedb98717eb3abedec221a3194728c2855915 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

b28aedb98717 drm/i915: Drop platform_mask

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12472/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Drop platform_mask

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Drop platform_mask

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Drop platform_mask

[Lucas De Marchi]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Drop platform_mask

[Tvrtko Ursulin]

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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Drop platform_mask

[Paulo Zanoni]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Drop platform_mask

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Drop platform_mask

[Lucas De Marchi]

On Fri, Mar 15, 2019 at 08:19:58PM +0200, Ville Syrjälä wrote:
>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.

I would be a little less opposed to this whole idea if it was in
bitmap.h indeed.

Lucas De Marchi

>
>-- 
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx