[Intel-gfx] [PATCH 4/4] drm/i915: Introduce concept of a sub-platform

[Chris Wilson]

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