[PATCH 07/17] drm/xe/oa/uapi: Define and parse OA stream properties

[Umesh Nerlige Ramappa]

On Tue, Dec 19, 2023 at 06:29:56PM +0200, Lionel Landwerlin wrote:
>On 19/12/2023 18:26, Umesh Nerlige Ramappa wrote:
>>On Mon, Dec 18, 2023 at 06:59:57PM -0800, Dixit, Ashutosh wrote:
>>>On Thu, 07 Dec 2023 22:43:19 -0800, Ashutosh Dixit wrote:
>>>>
>>>>+    /**
>>>>+     * @DRM_XE_OA_PROPERTY_OPEN_FLAGS: CLOEXEC and NONBLOCK flags are
>>>>+     * directly applied to returned OA fd. DISABLED opens the 
>>>>OA stream in a
>>>>+     * DISABLED state (see @DRM_XE_PERF_IOCTL_ENABLE).
>>>>+     */
>>>>+    DRM_XE_OA_PROPERTY_OPEN_FLAGS,
>>>>+#define DRM_XE_OA_FLAG_FD_CLOEXEC    (1 << 0)
>>>>+#define DRM_XE_OA_FLAG_FD_NONBLOCK    (1 << 1)
>>>>+#define DRM_XE_OA_FLAG_DISABLED        (1 << 2)
>>>
>>>I am wondering why these flags should be part of this uapi:
>>>
>>>* O_CLOEXEC and O_NONBLOCK can be set on the returned stream fd 
>>>using fcntl
>>> (see man 2 fcntl)
>>
>>I think the O_CLOEXEC was used so that a fork doesn't carry over the 
>>fd to the child. For the OA use case, we want to prevent that.  
>>However, these flags don't really need to be passed separately. They 
>>can be flags in the stream open property.
>>
>>Umesh
>>
>
>You know that the application can set those flags by using the fcntl() 
>syscall?
>
>It doesn't look like it's a useful feature to add in the driver.
>

Right. It does look like it's not needed in the driver.

I just don't know if there was a reason to include it in the same call 
as stream open ioctl. My guess is that we didn't want them to be 
separate calls due the nature of OA use case - privileged and single 
user. The application could just open a stream fd and fork a bunch of 
threads and all threads would have access to the stream fd (even if they 
drop root?).

Or I might be overthinking this. Maybe it's just there in the driver 
because fcntl mentions some races that may/may not apply to our use 
case. In practice, the application will likely call the fcntl right away 
and since OA does not support multiple users, the above concerns are not 
relevant, so fine to do it in fcntl.

Thanks,
Umesh
>
>-Lionel
>
>
>>>* DRM_XE_OA_FLAG_DISABLED can just be a stream open property, 
>>>doesn't need
>>> to be a fd flag.
>>>
>>>Comments?
>>
>>
>>>
>>>Thanks.
>>>-- 
>>>Ashutosh
>
>