Re: [Intel-gfx] [PATCH 4/4] drm/i915: Introduce concept of a sub-platform
On Wed, 27 Mar 2019, Tvrtko Ursulin wrote: > On 27/03/2019 12:03, Jani Nikula wrote: >> A lot of this could go away if we were able to encode a bunch of the >> flags in a single array similar to pciidtable. > > I don't see how we can encode everything to distinguish a platform in an > unsigned long, not even on 64-bit? Me neither. That's why I'm blabbering here instead of writing a patch. :p BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Introduce concept of a sub-platform
On 27/03/2019 12:03, Jani Nikula wrote: On Wed, 27 Mar 2019, Chris Wilson wrote: Quoting Tvrtko Ursulin (2019-03-27 11:35:41) On 26/03/2019 09:53, Chris Wilson wrote: Quoting Jani Nikula (2019-03-26 09:34:45) Not to block this series, but looking further outside the box... I've still got the constant vs. runtime device info split unfinished. We've got so many things that are mostly constant, but occasionally need changes. And we've got so many things that could be device info flags, but would lead to proliferation of plenty of almost identical device info structures. Like this ULX/ULT and GT number. So I guess I'm wondering if we're doing the right thing by assigning device info pointers to the struct pci_device_id driver_data member in pciidlist[] table. For one thing, that's a whole lot of bits that could be used directly for assigning platform and subplatform, or features. Of course, we'd then need another table besides pciidlist[] to map to the device info, but we're sort of doing some of that with the ULX/ULT parts. I just overall feel that there must be a better way to do all this, and we just haven't figured it out yet, and we're partially putting ourselves into a box we can't break out of. Thoughts? I think intel_device_info is still fundamentally useful. The disadvantage of having the feature discovery separate from use is outweighed by having consistent stanzas for those features - it makes comparing platforms, finding feature sets much easier. (The cost being that with the setting of the feature flag far away from the code using it, people updating the cost are more likely to forget the flag.) One end goal of this madness, is that we can recompile the kernel module to only support a single sku and dce the rest. But what are the diminishing returns here? Without measurement, I'd say a single platform. So that dictates what can be in the static intel_device_info, features that are constant across a whole platform. And as you point out, we don't need a pointer to the device_info itself, just a platform field which is an index into the device_info block, with plenty of room for subplatform flags. While that says how we hook up device_info, I don't think that reflects on the use of feature flags themselves, or our ability to statically determine a reduced feature set. So not a box, just a mere wet paper bag. To check if I follow.. we are talking about potentially abolishing device info in favour of constructing something at probe time, which could potentially have fewer and overall smaller static data portion? Because I don't see how we can eliminate the pciidlist itself, or even shrink it's size? It has to have one entry per device id, just the question for what we use driver_data for? Correct. What I think Jani was suggesting was that instead of encoding the device_info pointer into driver_data, we encode the platform/subplatform id. We then use the platform portion to lookup the device_info, and subplatform to annotate the runtime_info. Static vs runtime I think shouldn't have effect on the per platform builds. As long as all feature tests are done via macros, or small static inlines, code can still be compiled out. I do have a small nagging feeling about this series as well, but I have managed to convince myself it is better than the device id listed in i915_drv.h. I still feel the pain from having the endless chains of || and so welcome the removal of devid from the macros. So don't know.. we can always drop it and just expand platform mask to u64 to solve the immediate need and leave the rest for later. Imo, this series is an improvement, and doesn't prevent us from changing our minds later (although not back to devid macros please!). Okay, let me still ask this: Why do we have gt member in device info, leading to plenty of device info duplication? Why should we add ULT/ULX as separate tables instead of flags in device info where they'd fid perfectly well. This choice is purely arbitrary. The only reason ULT/ULX isn't in device info is because it would lead to so much duplication. Agreed with everything here. Perhaps for instance moving GT into runtime would further improve things. Someone mentioned GT somewhere around here.. So same approach with id tables/macros as with subplatform. If the downside of not being to compile out GT configurations within a single platform is not a concern. A lot of this could go away if we were able to encode a bunch of the flags in a single array similar to pciidtable. I don't see how we can encode everything to distinguish a platform in an unsigned long, not even on 64-bit? (I agree this series, modulo some nitpicks, is an improvement over status quo, I'm just playing the devil's advocate and trying to come up with something better.) In this case an updated version should be in your mailbox. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Introduce concept of a sub-platform
On Wed, 27 Mar 2019, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-03-27 11:35:41) >> >> On 26/03/2019 09:53, Chris Wilson wrote: >> > Quoting Jani Nikula (2019-03-26 09:34:45) >> >> Not to block this series, but looking further outside the box... >> >> >> >> I've still got the constant vs. runtime device info split >> >> unfinished. We've got so many things that are mostly constant, but >> >> occasionally need changes. And we've got so many things that could be >> >> device info flags, but would lead to proliferation of plenty of almost >> >> identical device info structures. Like this ULX/ULT and GT number. >> >> >> >> So I guess I'm wondering if we're doing the right thing by assigning >> >> device info pointers to the struct pci_device_id driver_data member in >> >> pciidlist[] table. >> >> >> >> For one thing, that's a whole lot of bits that could be used directly >> >> for assigning platform and subplatform, or features. >> >> >> >> Of course, we'd then need another table besides pciidlist[] to map to >> >> the device info, but we're sort of doing some of that with the ULX/ULT >> >> parts. >> >> >> >> I just overall feel that there must be a better way to do all this, and >> >> we just haven't figured it out yet, and we're partially putting >> >> ourselves into a box we can't break out of. >> >> >> >> Thoughts? >> > >> > I think intel_device_info is still fundamentally useful. The >> > disadvantage of having the feature discovery separate from use is >> > outweighed by having consistent stanzas for those features - it makes >> > comparing platforms, finding feature sets much easier. (The cost being >> > that with the setting of the feature flag far away from the code using >> > it, people updating the cost are more likely to forget the flag.) >> > >> > One end goal of this madness, is that we can recompile the kernel module >> > to only support a single sku and dce the rest. But what are the >> > diminishing returns here? Without measurement, I'd say a single >> > platform. >> > >> > So that dictates what can be in the static intel_device_info, features >> > that are constant across a whole platform. And as you point out, we >> > don't need a pointer to the device_info itself, just a platform field >> > which is an index into the device_info block, with plenty of room for >> > subplatform flags. >> > >> > While that says how we hook up device_info, I don't think that reflects >> > on the use of feature flags themselves, or our ability to statically >> > determine a reduced feature set. >> > >> > So not a box, just a mere wet paper bag. >> >> To check if I follow.. we are talking about potentially abolishing >> device info in favour of constructing something at probe time, which >> could potentially have fewer and overall smaller static data portion? >> >> Because I don't see how we can eliminate the pciidlist itself, or even >> shrink it's size? It has to have one entry per device id, just the >> question for what we use driver_data for? > > Correct. What I think Jani was suggesting was that instead of encoding > the device_info pointer into driver_data, we encode the > platform/subplatform id. We then use the platform portion to lookup the > device_info, and subplatform to annotate the runtime_info. > >> Static vs runtime I think shouldn't have effect on the per platform >> builds. As long as all feature tests are done via macros, or small >> static inlines, code can still be compiled out. >> >> I do have a small nagging feeling about this series as well, but I have >> managed to convince myself it is better than the device id listed in >> i915_drv.h. > > I still feel the pain from having the endless chains of || and so > welcome the removal of devid from the macros. > >> So don't know.. we can always drop it and just expand >> platform mask to u64 to solve the immediate need and leave the rest for >> later. > > Imo, this series is an improvement, and doesn't prevent us from changing > our minds later (although not back to devid macros please!). Okay, let me still ask this: Why do we have gt member in device info, leading to plenty of device info duplication? Why should we add ULT/ULX as separate tables instead of flags in device info where they'd fid perfectly well. This choice is purely arbitrary. The only reason ULT/ULX isn't in device info is because it would lead to so much duplication. A lot of this could go away if we were able to encode a bunch of the flags in a single array similar to pciidtable. (I agree this series, modulo some nitpicks, is an improvement over status quo, I'm just playing the devil's advocate and trying to come up with something better.) BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Introduce concept of a sub-platform
Quoting Tvrtko Ursulin (2019-03-27 11:35:41) > > On 26/03/2019 09:53, Chris Wilson wrote: > > Quoting Jani Nikula (2019-03-26 09:34:45) > >> Not to block this series, but looking further outside the box... > >> > >> I've still got the constant vs. runtime device info split > >> unfinished. We've got so many things that are mostly constant, but > >> occasionally need changes. And we've got so many things that could be > >> device info flags, but would lead to proliferation of plenty of almost > >> identical device info structures. Like this ULX/ULT and GT number. > >> > >> So I guess I'm wondering if we're doing the right thing by assigning > >> device info pointers to the struct pci_device_id driver_data member in > >> pciidlist[] table. > >> > >> For one thing, that's a whole lot of bits that could be used directly > >> for assigning platform and subplatform, or features. > >> > >> Of course, we'd then need another table besides pciidlist[] to map to > >> the device info, but we're sort of doing some of that with the ULX/ULT > >> parts. > >> > >> I just overall feel that there must be a better way to do all this, and > >> we just haven't figured it out yet, and we're partially putting > >> ourselves into a box we can't break out of. > >> > >> Thoughts? > > > > I think intel_device_info is still fundamentally useful. The > > disadvantage of having the feature discovery separate from use is > > outweighed by having consistent stanzas for those features - it makes > > comparing platforms, finding feature sets much easier. (The cost being > > that with the setting of the feature flag far away from the code using > > it, people updating the cost are more likely to forget the flag.) > > > > One end goal of this madness, is that we can recompile the kernel module > > to only support a single sku and dce the rest. But what are the > > diminishing returns here? Without measurement, I'd say a single > > platform. > > > > So that dictates what can be in the static intel_device_info, features > > that are constant across a whole platform. And as you point out, we > > don't need a pointer to the device_info itself, just a platform field > > which is an index into the device_info block, with plenty of room for > > subplatform flags. > > > > While that says how we hook up device_info, I don't think that reflects > > on the use of feature flags themselves, or our ability to statically > > determine a reduced feature set. > > > > So not a box, just a mere wet paper bag. > > To check if I follow.. we are talking about potentially abolishing > device info in favour of constructing something at probe time, which > could potentially have fewer and overall smaller static data portion? > > Because I don't see how we can eliminate the pciidlist itself, or even > shrink it's size? It has to have one entry per device id, just the > question for what we use driver_data for? Correct. What I think Jani was suggesting was that instead of encoding the device_info pointer into driver_data, we encode the platform/subplatform id. We then use the platform portion to lookup the device_info, and subplatform to annotate the runtime_info. > Static vs runtime I think shouldn't have effect on the per platform > builds. As long as all feature tests are done via macros, or small > static inlines, code can still be compiled out. > > I do have a small nagging feeling about this series as well, but I have > managed to convince myself it is better than the device id listed in > i915_drv.h. I still feel the pain from having the endless chains of || and so welcome the removal of devid from the macros. > So don't know.. we can always drop it and just expand > platform mask to u64 to solve the immediate need and leave the rest for > later. Imo, this series is an improvement, and doesn't prevent us from changing our minds later (although not back to devid macros please!). -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Introduce concept of a sub-platform
On 26/03/2019 08:39, Jani Nikula wrote:
On Tue, 26 Mar 2019, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
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.
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.
v5:
* Fix subplatform check.
* Protect against forgetting to expand subplatform bits.
* Remove platform enum tallying.
* Add subplatform to error state. (Chris)
* Drop macros and just use static inlines.
* Remove redundant IRONLAKE_M. (Ville)
v6:
* Split out Ironlake change.
* Optimize subplatform check.
* Use __always_inline. (Lucas)
* Add platform_mask comment. (Paulo)
* Pass stored runtime info in error capture. (Chris)
v7:
* Rebased for new AML ULX device id.
* Bump platform mask array size for EHL.
* Stop mentioning device ids in intel_device_subplatform_init by using
the trick of splitting macros i915_pciids.h. (Jani)
* AML seems to be either a subplatform of KBL or CFL so express it like
that.
Signed-off-by: Tvrtko Ursulin
Suggested-by: Chris Wilson
Cc: Chris Wilson
Cc: Jani Nikula
Cc: Lucas De Marchi
Cc: Jose Souza
Cc: Ville Syrjälä
Cc: Paulo Zanoni
Reviewed-by: Chris Wilson # v6
---
drivers/gpu/drm/i915/i915_drv.c | 8 +-
drivers/gpu/drm/i915/i915_drv.h | 124 +--
drivers/gpu/drm/i915/i915_gpu_error.c| 3 +
drivers/gpu/drm/i915/i915_pci.c | 2 +-
drivers/gpu/drm/i915/intel_device_info.c | 145 +++
drivers/gpu/drm/i915/intel_device_info.h | 27 -
6 files changed, 267 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5465b99b4392..74255374cc6b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -868,6 +868,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);
@@ -1718,10 +1720,12 @@ 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
(subplatform=0x%x) gen=%i\n",
INTEL_DEVID(dev_priv),
INTEL_REVID(dev_priv),
intel_platform_name(INTEL_INFO(dev_priv)->platform),
+ intel_subplatform(RUNTIME_INFO(dev_priv),
+INTEL_INFO(dev_priv)->platform),
INTEL_GEN(dev_priv));
intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
@@ -1764,8 +1768,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 48c3b139f36f..c29ebfd94065 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2298,7 +2298,68 @@ 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))
+static __always_inline unsigned int
+__platform_mask_index(const struct intel_runtime_info *info,
+ enum intel_platform p)
+{
+ const unsigned int pbits =
+ BITS_PER_TYPE(info->platform_mask[0]) - INTEL_SUBPLATFORM_BITS;
+
+ /* Expand the platform_mask array if this fails. */
+ BUILD_BUG_ON(INTEL_MAX_PLATFORMS >
+
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Introduce concept of a sub-platform
On 26/03/2019 09:53, Chris Wilson wrote: Quoting Jani Nikula (2019-03-26 09:34:45) Not to block this series, but looking further outside the box... I've still got the constant vs. runtime device info split unfinished. We've got so many things that are mostly constant, but occasionally need changes. And we've got so many things that could be device info flags, but would lead to proliferation of plenty of almost identical device info structures. Like this ULX/ULT and GT number. So I guess I'm wondering if we're doing the right thing by assigning device info pointers to the struct pci_device_id driver_data member in pciidlist[] table. For one thing, that's a whole lot of bits that could be used directly for assigning platform and subplatform, or features. Of course, we'd then need another table besides pciidlist[] to map to the device info, but we're sort of doing some of that with the ULX/ULT parts. I just overall feel that there must be a better way to do all this, and we just haven't figured it out yet, and we're partially putting ourselves into a box we can't break out of. Thoughts? I think intel_device_info is still fundamentally useful. The disadvantage of having the feature discovery separate from use is outweighed by having consistent stanzas for those features - it makes comparing platforms, finding feature sets much easier. (The cost being that with the setting of the feature flag far away from the code using it, people updating the cost are more likely to forget the flag.) One end goal of this madness, is that we can recompile the kernel module to only support a single sku and dce the rest. But what are the diminishing returns here? Without measurement, I'd say a single platform. So that dictates what can be in the static intel_device_info, features that are constant across a whole platform. And as you point out, we don't need a pointer to the device_info itself, just a platform field which is an index into the device_info block, with plenty of room for subplatform flags. While that says how we hook up device_info, I don't think that reflects on the use of feature flags themselves, or our ability to statically determine a reduced feature set. So not a box, just a mere wet paper bag. To check if I follow.. we are talking about potentially abolishing device info in favour of constructing something at probe time, which could potentially have fewer and overall smaller static data portion? Because I don't see how we can eliminate the pciidlist itself, or even shrink it's size? It has to have one entry per device id, just the question for what we use driver_data for? Static vs runtime I think shouldn't have effect on the per platform builds. As long as all feature tests are done via macros, or small static inlines, code can still be compiled out. I do have a small nagging feeling about this series as well, but I have managed to convince myself it is better than the device id listed in i915_drv.h. So don't know.. we can always drop it and just expand platform mask to u64 to solve the immediate need and leave the rest for later. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Introduce concept of a sub-platform
Quoting Jani Nikula (2019-03-26 09:34:45) > Not to block this series, but looking further outside the box... > > I've still got the constant vs. runtime device info split > unfinished. We've got so many things that are mostly constant, but > occasionally need changes. And we've got so many things that could be > device info flags, but would lead to proliferation of plenty of almost > identical device info structures. Like this ULX/ULT and GT number. > > So I guess I'm wondering if we're doing the right thing by assigning > device info pointers to the struct pci_device_id driver_data member in > pciidlist[] table. > > For one thing, that's a whole lot of bits that could be used directly > for assigning platform and subplatform, or features. > > Of course, we'd then need another table besides pciidlist[] to map to > the device info, but we're sort of doing some of that with the ULX/ULT > parts. > > I just overall feel that there must be a better way to do all this, and > we just haven't figured it out yet, and we're partially putting > ourselves into a box we can't break out of. > > Thoughts? I think intel_device_info is still fundamentally useful. The disadvantage of having the feature discovery separate from use is outweighed by having consistent stanzas for those features - it makes comparing platforms, finding feature sets much easier. (The cost being that with the setting of the feature flag far away from the code using it, people updating the cost are more likely to forget the flag.) One end goal of this madness, is that we can recompile the kernel module to only support a single sku and dce the rest. But what are the diminishing returns here? Without measurement, I'd say a single platform. So that dictates what can be in the static intel_device_info, features that are constant across a whole platform. And as you point out, we don't need a pointer to the device_info itself, just a platform field which is an index into the device_info block, with plenty of room for subplatform flags. While that says how we hook up device_info, I don't think that reflects on the use of feature flags themselves, or our ability to statically determine a reduced feature set. So not a box, just a mere wet paper bag. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Introduce concept of a sub-platform
On Tue, 26 Mar 2019, Jani Nikula wrote:
> On Tue, 26 Mar 2019, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin
>>
>> 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.
>>
>> 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.
>>
>> v5:
>> * Fix subplatform check.
>> * Protect against forgetting to expand subplatform bits.
>> * Remove platform enum tallying.
>> * Add subplatform to error state. (Chris)
>> * Drop macros and just use static inlines.
>> * Remove redundant IRONLAKE_M. (Ville)
>>
>> v6:
>> * Split out Ironlake change.
>> * Optimize subplatform check.
>> * Use __always_inline. (Lucas)
>> * Add platform_mask comment. (Paulo)
>> * Pass stored runtime info in error capture. (Chris)
>>
>> v7:
>> * Rebased for new AML ULX device id.
>> * Bump platform mask array size for EHL.
>> * Stop mentioning device ids in intel_device_subplatform_init by using
>>the trick of splitting macros i915_pciids.h. (Jani)
>> * AML seems to be either a subplatform of KBL or CFL so express it like
>>that.
>>
>> Signed-off-by: Tvrtko Ursulin
>> Suggested-by: Chris Wilson
>> Cc: Chris Wilson
>> Cc: Jani Nikula
>> Cc: Lucas De Marchi
>> Cc: Jose Souza
>> Cc: Ville Syrjälä
>> Cc: Paulo Zanoni
>> Reviewed-by: Chris Wilson # v6
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 8 +-
>> drivers/gpu/drm/i915/i915_drv.h | 124 +--
>> drivers/gpu/drm/i915/i915_gpu_error.c| 3 +
>> drivers/gpu/drm/i915/i915_pci.c | 2 +-
>> drivers/gpu/drm/i915/intel_device_info.c | 145 +++
>> drivers/gpu/drm/i915/intel_device_info.h | 27 -
>> 6 files changed, 267 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 5465b99b4392..74255374cc6b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -868,6 +868,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);
>> @@ -1718,10 +1720,12 @@ 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
>> (subplatform=0x%x) gen=%i\n",
>> INTEL_DEVID(dev_priv),
>> INTEL_REVID(dev_priv),
>> intel_platform_name(INTEL_INFO(dev_priv)->platform),
>> + intel_subplatform(RUNTIME_INFO(dev_priv),
>> + INTEL_INFO(dev_priv)->platform),
>> INTEL_GEN(dev_priv));
>>
>> intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
>> @@ -1764,8 +1768,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 48c3b139f36f..c29ebfd94065 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2298,7 +2298,68 @@ 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))
>> +static __always_inline unsigned int
>> +__platform_mask_index(const struct intel_runtime_i
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Introduce concept of a sub-platform
On Tue, 26 Mar 2019, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin
>
> 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.
>
> 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.
>
> v5:
> * Fix subplatform check.
> * Protect against forgetting to expand subplatform bits.
> * Remove platform enum tallying.
> * Add subplatform to error state. (Chris)
> * Drop macros and just use static inlines.
> * Remove redundant IRONLAKE_M. (Ville)
>
> v6:
> * Split out Ironlake change.
> * Optimize subplatform check.
> * Use __always_inline. (Lucas)
> * Add platform_mask comment. (Paulo)
> * Pass stored runtime info in error capture. (Chris)
>
> v7:
> * Rebased for new AML ULX device id.
> * Bump platform mask array size for EHL.
> * Stop mentioning device ids in intel_device_subplatform_init by using
>the trick of splitting macros i915_pciids.h. (Jani)
> * AML seems to be either a subplatform of KBL or CFL so express it like
>that.
>
> Signed-off-by: Tvrtko Ursulin
> Suggested-by: Chris Wilson
> Cc: Chris Wilson
> Cc: Jani Nikula
> Cc: Lucas De Marchi
> Cc: Jose Souza
> Cc: Ville Syrjälä
> Cc: Paulo Zanoni
> Reviewed-by: Chris Wilson # v6
> ---
> drivers/gpu/drm/i915/i915_drv.c | 8 +-
> drivers/gpu/drm/i915/i915_drv.h | 124 +--
> drivers/gpu/drm/i915/i915_gpu_error.c| 3 +
> drivers/gpu/drm/i915/i915_pci.c | 2 +-
> drivers/gpu/drm/i915/intel_device_info.c | 145 +++
> drivers/gpu/drm/i915/intel_device_info.h | 27 -
> 6 files changed, 267 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 5465b99b4392..74255374cc6b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -868,6 +868,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);
> @@ -1718,10 +1720,12 @@ 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
> (subplatform=0x%x) gen=%i\n",
> INTEL_DEVID(dev_priv),
> INTEL_REVID(dev_priv),
> intel_platform_name(INTEL_INFO(dev_priv)->platform),
> +intel_subplatform(RUNTIME_INFO(dev_priv),
> + INTEL_INFO(dev_priv)->platform),
> INTEL_GEN(dev_priv));
>
> intel_device_info_dump_flags(INTEL_INFO(dev_priv), &p);
> @@ -1764,8 +1768,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 48c3b139f36f..c29ebfd94065 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2298,7 +2298,68 @@ 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))
> +static __always_inline unsigned int
> +__platform_mask_index(const struct intel_runtime_info *info,
> + enum intel_platform p)
> +{
> + const unsigned int pbits =
> + BITS_PER_TYPE(info->platform_mask[
