Re: [PATCH v3 09/12] drm/xe/pxp: Add API to mark a BO as using PXP

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

On Thu, 21 Nov 2024, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote:
> On 11/21/2024 1:57 AM, Jani Nikula wrote:
>> On Wed, 20 Nov 2024, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote:
>>> diff --git a/drivers/gpu/drm/xe/compat-i915-headers/pxp/intel_pxp.h b/drivers/gpu/drm/xe/compat-i915-headers/pxp/intel_pxp.h
>>> index 419e8e926f00..533bc82255b6 100644
>>> --- a/drivers/gpu/drm/xe/compat-i915-headers/pxp/intel_pxp.h
>>> +++ b/drivers/gpu/drm/xe/compat-i915-headers/pxp/intel_pxp.h
>>> @@ -9,6 +9,9 @@
>>>   #include <linux/errno.h>
>>>   #include <linux/types.h>
>>>   
>>> +#include "xe_bo.h"
>>> +#include "xe_pxp.h"
>>> +
>> Can't have this. This will include xe_bo.h and xe_pxp.h from i915
>> display.
>>
>> Basically you can't use gem_to_xe_bo() in static inlines in headers that
>> get included to i915 display. It all needs to stay opaque.
>
> Why would this be included to i915 display? This is the copy of the 
> header used for building the display code with Xe, i915 should use it's 
> own copy (i915/pxp/intel_pxp.h). Several other headers in the 
> compat-i915-headers subfolder include Xe headers. Or is the problem 
> specifically with the BO part of it? Because I can move the code to 
> xe_pxp.c, but I'd still have to include at least xe_pxp.h.

With "i915 display", I refer to the display code that gets built for
both i915 and xe. And regardless of which module the code is being built
for, it should not include details like this. The goal is to make it
more and more independent from the two, eventually spawning into a
dedicated module. So we want to axe includes that lead from i915 display
to either i915 or xe core headers.

BR,
Jani.



>
> Daniele
>
>>
>> BR,
>> Jani.
>>
>>>   struct drm_gem_object;
>>>   struct xe_pxp;
>>>   
>>> @@ -16,7 +19,15 @@ static inline int intel_pxp_key_check(struct xe_pxp *pxp,
>>>   				      struct drm_gem_object *obj,
>>>   				      bool assign)
>>>   {
>>> -	return -ENODEV;
>>> +	/*
>>> +	 * The assign variable is used in i915 to assign the key to the BO at
>>> +	 * first submission time. In Xe the key is instead assigned at BO
>>> +	 * creation time, so the assign variable must always be false.
>>> +	 */
>>> +	if (assign)
>>> +		return -EINVAL;
>>> +
>>> +	return xe_pxp_key_check(pxp, gem_to_xe_bo(obj));
>>>   }
>>>   
>>>   #endif
>

-- 
Jani Nikula, Intel