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

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

<snip>
>
>>
>>>
>>>> + *  - %DRM_XE_EXEC_QUEUE_SET_PROPERTY_PXP_TYPE - set the type of 
>>>> PXP session
>>>> + *    this queue will be used with. Valid values are listed in enum
>>>> + *    drm_xe_pxp_session_type. %DRM_XE_PXP_TYPE_NONE is the 
>>>> default behavior, so
>>>> + *    there is no need to explicitly set that. When a queue of type
>>>> + *    %DRM_XE_PXP_TYPE_HWDRM is created, the PXP default HWDRM 
>>>> session
>>>> + *    (%XE_PXP_HWDRM_DEFAULT_SESSION) will be started, if isn't 
>>>> already running.
>>>> + *    Given that going into a power-saving state kills PXP HWDRM 
>>>> sessions,
>>>> + *    runtime PM will be blocked while queues of this type are alive.
>>>> + *    All PXP queues will be killed if a PXP invalidation event 
>>>> occurs.
>>> Seems odd to say 'values are listed in ...' and then go on to 
>>> describe each type and provide extra information about them. Seems 
>>> like the extra details should be part of the enum documentation 
>>> instead of here?
>>
>> This is documentation specific to how this ioctl handles those 
>> values, so it belongs here. The 'values are listed in ...' sentence 
>> was about being future proof, in case we update the enum in the 
>> future and don't need to add any extra explanation here.
>>
> That is an argument for having a single point of documentation and 
> that point being the point of definition. Then, if new values are 
> added it is immediately obvious what documentation needs to be updated.

Still not convinced. Having the ioctl-specific info in the enum 
definition would mean having to list the behavior each enum value has 
for each ioctl that uses it; IMO it's cleaner to have them in the ioctl 
documentation itself so it's easy to make clear which special behavior 
applies to which ioctl. Also, when I said not adding any extra 
explanation here for future extensions I was not referring to a mistake; 
for example, compute sessions (which we don't currently support) don't 
have any of the extra requirements that HWDRM sessions have, so we 
wouldn't need to add any extra explanation to this ioctl if we added 
support for those.

Daniele

>
> John.
>
>> Daniele
>>
>>>
>>> John.
>>>
>>>> + *
>>>>    * The example below shows how to use @drm_xe_exec_queue_create 
>>>> to create
>>>>    * a simple exec_queue (no parallel submission) of class
>>>>    * &DRM_XE_ENGINE_CLASS_RENDER.
>>>> @@ -1108,7 +1126,7 @@ struct drm_xe_exec_queue_create {
>>>>   #define DRM_XE_EXEC_QUEUE_EXTENSION_SET_PROPERTY        0
>>>>   #define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_PRIORITY        0
>>>>   #define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_TIMESLICE 1
>>>> -
>>>> +#define   DRM_XE_EXEC_QUEUE_SET_PROPERTY_PXP_TYPE        2
>>>>       /** @extensions: Pointer to the first extension struct, if 
>>>> any */
>>>>       __u64 extensions;
>>>>   @@ -1694,6 +1712,26 @@ struct drm_xe_oa_stream_info {
>>>>       __u64 reserved[3];
>>>>   };
>>>>   +/**
>>>> + * enum drm_xe_pxp_session_type - Supported PXP session types.
>>>> + *
>>>> + * We currently only support HWDRM sessions, which are used for 
>>>> protected
>>>> + * content that ends up being displayed, but the HW supports 
>>>> multiple types, so
>>>> + * we might extend support in the future.
>>>> + */
>>>> +enum drm_xe_pxp_session_type {
>>>> +    /** @DRM_XE_PXP_TYPE_NONE: PXP not used */
>>>> +    DRM_XE_PXP_TYPE_NONE = 0,
>>>> +    /**
>>>> +     * @DRM_XE_PXP_TYPE_HWDRM: HWDRM sessions are used for content 
>>>> that ends
>>>> +     * up on the display.
>>>> +     */
>>>> +    DRM_XE_PXP_TYPE_HWDRM = 1,
>>>> +};
>>>> +
>>>> +/* ID of the protected content session managed by Xe when PXP is 
>>>> active */
>>>> +#define DRM_XE_PXP_HWDRM_DEFAULT_SESSION 0xf
>>>> +
>>>>   #if defined(__cplusplus)
>>>>   }
>>>>   #endif
>>
>