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