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

[Daniele Ceraolo Spurio]

On 11/21/2024 12:03 PM, Jani Nikula wrote:
> On Thu, 21 Nov 2024, Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com> wrote:
>> On 11/21/2024 1:57 AM, Jani Nikula wrote:
>>> On Wed, 20 Nov 2024, Daniele Ceraolo Spurio <daniele.ceraolospurio at 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.

Ok that makes sense, but I'm not sure if it can be easily avoided here. 
The PXP key is managed outside of the display code, so we do need a 
callback into Xe or i915 to get that info. The only way I can see that 
being doable without the different headers is if we pass a function 
pointer at display init time and keep that stored inside the 
intel_display structure. That to me sounds like something that should be 
introduced more structurally for multiple callbacks into i915/xe instead 
of just for PXP, but I can do it if that's the preference.

thanks,
Daniele

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