Re: [Intel-gfx] [PATCH 1/3] drm/i915: Add struct to hold IP version

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 20 Oct 2021, "Souza, Jose" <jose.souza@intel.com> wrote:
> On Wed, 2021-10-20 at 12:47 +0300, Jani Nikula wrote:
>> On Tue, 19 Oct 2021, José Roberto de Souza <jose.souza@intel.com> wrote:
>> > The constant platform display version is not using this new struct but
>> > the runtime variant will definitely use it.
>> 
>> Cc: Some more folks to hijack this thread. Sorry! ;)
>> 
>> We added runtime info to i915, because we had this idea and goal of
>> turning the device info to a truly const pointer to the info structures
>> in i915_pci.c that are stored in rodata. The idea was that we'll have a
>> complete split of mutable and immutable device data, with all the
>> mutable data in runtime info.
>> 
>> Alas, we never got there. More and more data that was mostly const but
>> sometimes needed tweaking kept piling up. mkwrite_device_info() was
>> supposed to be a clue not to modify device info runtime, but instead it
>> proliferated. Now we have places like intel_fbc_init() disabling FBC
>> through that. But most importantly, we have fusing that considerably
>> changes the device info, and the copying all of that data over to
>> runtime info probably isn't worth it.
>> 
>> Should we just acknowledge that the runtime info is useless, and move
>> some of that data to intel_device_info and some of it elsewhere in i915?
>
> With newer platforms getting more and more modular, I believe we will
> need to store even more mutable platform information.
>
> In my opinion a separation of immutable and mutable platform
> information is cleaner and easier to maintain.

Yeah, that's kind of what the original point was with device and runtime
info split. It's just that a lot of the supposedly immutable platform
info has turned into mutable information.

I think either we need to properly follow through with that idea, and
only store a const struct intel_device_info * to the rodata in
i915_pci.c, or just scrap it. None of this "almost immutable" business
that we currently have. "Almost immutable" means "mutable".

The main problem is that we'll still want to have the initial values in
static data. One idea is something like this:

struct intel_device_info {
	const struct intel_runtime_info *runtime_info;
        /* ... */
};

static const struct intel_device_info i965g_info = {
	.runtime_info = &i965g_initial_runtime_info;
        /* ... */
};

And things like .pipe_mask would be part of struct
intel_runtime_info. You'd copy the stuff over from intel_device_info
runtime_info member to i915->__runtime, but i915->__info would be a
const pointer to the device info. You'd never access the runtime_info
member after of intel_device_info after probe.

It's just really painful, for instance because we already have two sets
of flags, display and non-display, and those would be multiplied to
mutable/immutable. And we should probably increase, not decrease, the
split between display and non-display. The macro horror show of
i915_pci.c would just grow worse.


BR,
Jani.



>
>> 
>> 
>> BR,
>> Jani.
>> 
>

-- 
Jani Nikula, Intel Open Source Graphics Center