On Thu, Oct 21, 2021 at 04:11:26PM +0300, Jani Nikula wrote:
On Wed, 20 Oct 2021, "Souza, Jose" <jose.souza@xxxxxxxxx> wrote:
On Wed, 2021-10-20 at 12:47 +0300, Jani Nikula wrote:
On Tue, 19 Oct 2021, José Roberto de Souza <jose.souza@xxxxxxxxx> 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.
I like this approach. I think the only problem would be that if someone
inadvertently do a i915->__info->runtime_info they will be accessing the
wrong data. So maybe to be clear do
struct intel_device_info {
const void *initial_runtime_info;
/* ... */
};
static const struct intel_device_info i965g_info = {
.initial_runtime_info = &i965g_initial_runtime_info;
/* ... */
};
this would make it opaque and even hint by the name so the developer is
not tempted to add a cast.
Lucas De Marchi
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
