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

[Lucas De Marchi]

On Thu, Oct 21, 2021 at 04:11:26PM +0300, Jani Nikula wrote:
>On Wed, 20 Oct 2021, "Souza, Jose" <jose.souza at 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 at 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.


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