On 27/03/2019 12:03, Jani Nikula wrote:
On Wed, 27 Mar 2019, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
