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

[Jani Nikula]

On Fri, 22 Nov 2024, Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com> wrote:
> On 11/22/2024 7:06 AM, Jani Nikula wrote:
>> On Thu, 21 Nov 2024, Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com> wrote:
>>> 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.
>> Why not make xe_pxp_key_check() accept struct drm_gem_object * instead
>> of struct xe_bo *? Basically all the interfaces to/from display will
>> need to use i915/xe independent types anyway.
>
> I can do that, but I'd still need to include xe_pxp.h to bring in 
> xe_pxp_key_check(), so there is still going to be an include that goes 
> from display into Xe. If that's ok for now, I'll go that way.

It's better. As long as it doesn't bring in a ton of dependencies.

BR,
Jani.



>
> Daniele
>
>>
>> See ad36a322619c ("drm/i915/display: convert skl_universal_plane.c to
>> struct drm_gem_object") which converts i915 pxp. And i915 display gets
>> to use either opaque types or generic drm types.
>>
>> BR,
>> Jani.
>>
>>
>>
>>> 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
>

-- 
Jani Nikula, Intel