Re: [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest

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

On Tue, 22 Mar 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> On 22/03/2022 15:26, Jani Nikula wrote:
>> On Tue, 22 Mar 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>>> On 22/03/2022 14:49, Jani Nikula wrote:
>>>> On Tue, 22 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>>>> On Tue, Mar 22, 2022 at 12:21:59PM +0200, Jani Nikula wrote:
>>>>>> On Mon, 21 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>>>>>>> On Mon, Mar 21, 2022 at 04:34:49PM -0700, Casey Bowman wrote:
>>>>>>>> Wanted to ping this older thread to find out where we stand with this patch,
>>>>>>>> Are we OK with the current state of these changes?
>>>>>>>>
>>>>>>>> With more recent information gathered from feedback on other patches, would
>>>>>>>> we prefer changing this to a more arch-neutral control flow?
>>>>>>>>
>>>>>>>> e.g.
>>>>>>>> #if IS_ENABLED(CONFIG_X86)
>>>>>>>> ...
>>>>>>>> #else
>>>>>>>> ...
>>>>>>>> #endif
>>>>>>>>
>>>>>>>> Would we also prefer this RFC series be merged or would it be preferred to
>>>>>>>> create a new series instead?
>>>>>>>
>>>>>>> for this specific function, that is used in only 2 places I think it's
>>>>>>> ok to do:
>>>>>>>
>>>>>>> 	static inline bool run_as_guest(void)
>>>>>>> 	{
>>>>>>> 	#if IS_ENABLED(CONFIG_X86)
>>>>>>> 		return !hypervisor_is_type(X86_HYPER_NATIVE);
>>>>>>> 	#else	
>>>>>>> 		/* Not supported yet */
>>>>>>> 		return false;	
>>>>>>> 	#endif
>>>>>>> 	}
>>>>>>>
>>>>>>> For PCH it doesn't really matter as we don't execute that function
>>>>>>> for discrete. For intel_vtd_active() I figure anything other than
>>>>>>> x86 would be fine with false here.
>>>>>>>
>>>>>>> Jani, that this look good to you?
>>>>>>
>>>>>> It's more important to me to get this out of i915_drv.h, which is not
>>>>>> supposed to be a collection of random stuff anymore. I've sent patches
>>>>>> to this effect but they've stalled a bit.
>>>>>
>>>>> do you have a patch moving this particular one? got a link?
>>>>
>>>> Yeah, but it was basically shot down by Tvrtko [1], and I stalled there.
>>>>
>>>> I'd just like to get all this cruft out of i915_drv.h. Whenever we have
>>>> a file where the name isn't super specific, we seem to have a tendency
>>>> of turning it into a dumping ground for random crap. So I'd really like
>>>> to move this out of there *before* expanding on it.
>>>
>>> Sounds like we had agreement on what tweaks to make and I conceded to
>>> live for now with the IMO wrongly named intel_vtd_run_as_guest.
>>>
>>> (I mean I really disagree with file name being trumps, which I think
>>> this example illustrates - this is i915 asking whether the kernel is
>>> running as guest so intel_vtd_ prefix is just wrong. Intel VT-d is the
>>> iommu thingy so it makes no sense when called from PCH detection. But I
>>> have no better ideas at the moment. We can call it i915_run_as_guest, to
>>> signify function belongs to i915, but then we lose the first parameter
>>> names the function rule.)
>> 
>> I think the "first parameter names the function" rule has backfired in
>> gem/gt land, because it's pretty difficult to figure out *where* you'd
>> expect to find or place functions.
>
> Hey surely is not that bad. And I am sure if I tried to add some display 
> feature that there's a chance I'd manage to misplace something. :))
>
> No scheme is perfect and there are always edge cases, ambiguities and 
> always work to cleanup further because it all evolved rather than 
> started from scratch. If you know what the function you wrote is about, 
> surely you can place it into a file whose name suggests it is the right 
> area. If you want the example of GT, there is a nice collection of files 
> per functional area.. intel_gt_<suffix>.. interrupts, power management, 
> requests, debugfs, etc.
>
> And if you look for functions you did not write, I certainly suggest 
> using ctags rather than try opening random files. I think driver is just 
> too big for the latter approach.

Obviously I use code tagging and search. The point was more about the
surprise in expecting to find a function in some place, and finding it
in a totally different place. And obviously for finding the right place
for new functions.

BR,
Jani.



>
> Regards,
>
> Tvrtko
>
>
>> BR,
>> Jani.
>> 
>>>
>>> But in any case I don't see that I created any blockers in this thread.
>>> AFAICS just a respin with intel_vtd_active taking struct device is
>>> needed and job done.
>>>
>>> Regards,
>>>
>>> Tvrtko
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center