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

[Souza, Jose]

On Mon, 2021-10-25 at 12:04 +0300, Jani Nikula wrote:
> On Fri, 22 Oct 2021, Lucas De Marchi <lucas.demarchi at 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 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.
> 
> 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.

Just to be sure, this discussion that Jani started can be build on top of this patch right?

Will try to get someone to review the remaining patch of this series to merge it.

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