Re: [PATCH v2 07/12] drm/xe/pxp: Add spport for PXP-using queues

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

<snip>


>>>> @@ -343,6 +377,24 @@ static int exec_queue_set_timeslice(struct 
>>>> xe_device *xe, struct xe_exec_queue *
>>>>       return 0;
>>>>   }
>>>>   +static int
>>>> +exec_queue_set_pxp_type(struct xe_device *xe, struct xe_exec_queue 
>>>> *q, u64 value)
>>>> +{
>>>> +    BUILD_BUG_ON(DRM_XE_PXP_TYPE_NONE != 0);
>>> Why a build bug for something that is a simple 'enum { X=0 }'? It's 
>>> not like there is some complex macro calculation that could be 
>>> broken by some seemingly unrelated change.
>>
>> This was more to make sure that the default value for the extension 
>> was 0. Given that this is UAPI and therefore can't change anyway, 
>> I'll drop the BUG_ON
>>
>>>
>>>> +
>>>> +    if (value == DRM_XE_PXP_TYPE_NONE)
>>>> +        return 0;
>>> This doesn't need to shut any existing PXP down? Is it not possible 
>>> to dynamically change the type?
>>
>> No, this can only be set at queue creation time
> Would be good to add a comment about that? Maybe even an assert or 
> something to ensure this is not called post creation?


Missed this comment on my first read through. All extension functions 
are guaranteed to only be called at creation time, so there is no risk 
of this being called later.

Daniele


>
>
>>
>>>
>>>> +
>>>> +    if (!xe_pxp_is_enabled(xe->pxp))
>>>> +        return -ENODEV;
>>>> +
>>>> +    /* we only support HWDRM sessions right now */
>>>> +    if (XE_IOCTL_DBG(xe, value != DRM_XE_PXP_TYPE_HWDRM))
>>>> +        return -EINVAL;
>>>> +
>>>> +    return xe_pxp_exec_queue_set_type(xe->pxp, q, 
>>>> DRM_XE_PXP_TYPE_HWDRM);
>>>> +}
>>>> +
>>>>   typedef int (*xe_exec_queue_set_property_fn)(struct xe_device *xe,
>>>>                            struct xe_exec_queue *q,
>>>>                            u64 value);
>>>> @@ -350,6 +402,7 @@ typedef int 
>>>> (*xe_exec_queue_set_property_fn)(struct xe_device *xe,
>>>>   static const xe_exec_queue_set_property_fn 
>>>> exec_queue_set_property_funcs[] = {
>>>>       [DRM_XE_EXEC_QUEUE_SET_PROPERTY_PRIORITY] = 
>>>> exec_queue_set_priority,
>>>>       [DRM_XE_EXEC_QUEUE_SET_PROPERTY_TIMESLICE] = 
>>>> exec_queue_set_timeslice,
>>>> +    [DRM_XE_EXEC_QUEUE_SET_PROPERTY_PXP_TYPE] = 
>>>> exec_queue_set_pxp_type,
>>>>   };
>>>>     static int exec_queue_user_ext_set_property(struct xe_device *xe,