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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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