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

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

On Fri, 22 Oct 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Thu, Oct 21, 2021 at 04:11:26PM +0300, Jani Nikula wrote:
>>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.
>
>
> 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.

I think that's all fairly straightforward. Any ideas on how to do the
flags split cleanly, though? I already dislike the
DEV_INFO_FOR_EACH_FLAG() and DEV_INFO_DISPLAY_FOR_EACH_FLAG() split.

BR,
Jani.



>
> 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

-- 
Jani Nikula, Intel Open Source Graphics Center