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

[Jani Nikula]

On Wed, 27 Mar 2019, Chris Wilson <chris at chris-wilson.co.uk> 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