[PATCH] accel/ivpu: Add turbo flag to the DRM_IVPU_CMDQ_CREATE ioctl

[Jacek Lawrynowicz]

Hi,

On 6/12/2025 3:42 PM, Jeff Hugo wrote:
> On 6/12/2025 7:31 AM, Falkowski, Maciej wrote:
>> On 6/6/2025 6:30 PM, Jeff Hugo wrote:
>>
>>> On 6/5/2025 10:20 AM, Maciej Falkowski wrote:
>>>> From: Andrzej Kacprowski <Andrzej.Kacprowski at intel.com>
>>>>
>>>> Introduces a new parameter to the DRM_IVPU_CMDQ_CREATE ioctl,
>>>
>>> Introduce
>> Ack, thanks.
>>>
>>>> enabling turbo mode for jobs submitted via the command queue.
>>>> Turbo mode allows jobs to run at higher frequencies,
>>>> potentially improving performance for demanding workloads.
>>>>
>>>> The change also adds the IVPU_TEST_MODE_TURBO_DISABLE flag
>>>
>>> "This change" is redundant. Just start with "Also add the..."
>> Ack, thanks.
>>>
>>>> to allow test mode to explicitly disable turbo mode
>>>> requested by the application.
>>>> The IVPU_TEST_MODE_TURBO mode has been renamed to
>>>> IVPU_TEST_MODE_TURBO_ENABLE for clarity and consistency.
>>>>
>>>> +/* Command queue flags */
>>>> +#define DRM_IVPU_CMDQ_FLAG_TURBO 0x00000001
>>>> +
>>>>   /**
>>>>    * struct drm_ivpu_cmdq_create - Create command queue for job submission
>>>>    */
>>>> @@ -462,6 +465,17 @@ struct drm_ivpu_cmdq_create {
>>>>        * %DRM_IVPU_JOB_PRIORITY_REALTIME
>>>>        */
>>>>       __u32 priority;
>>>> +    /**
>>>> +     * @flags:
>>>> +     *
>>>> +     * Supported flags:
>>>> +     *
>>>> +     * %DRM_IVPU_CMDQ_FLAG_TURBO
>>>> +     *
>>>> +     * Enable low-latency mode for the command queue. The NPU will maximize performance
>>>> +     * when executing jobs from such queue at the cost of increased power usage.
>>>> +     */
>>>> +    __u32 flags;
>>>
>>> This is going to break the struct size on compat.  You probably need a __u32 reserved to maintain 64-bit alignment. 
>>
>> Thank you for suggestion,
>> I think compat is preserved here as u32 imposes 4 byte alignment on 64bit
>> so the alignment is going to be 12 bytes on both 32bit and 64bit, I tested this manually.
>> Please correct me if I am wrong.
> 
> Looks like I'm wrong.  Majority of the structures have 64-bit values, and I didn't clearly see that this specific one is only 32-bit values.
> 
> My initial comment was based on https://docs.kernel.org/process/botching-up-ioctls.html - specifically:
> 
> Pad the entire struct to a multiple of 64-bits if the structure contains 64-bit types - the structure size will otherwise differ on 32-bit versus 64-bit. Having a different structure size hurts when passing arrays of structures to the kernel, or if the kernel checks the structure size, which e.g. the drm core does.
> 
> Ok. This was the only functional comment, and it is resolved. The other two are trivial fixups, so I think with those -
> 
> Reviewed-by: Jeff Hugo <jeff.hugo at oss.qualcomm.com>

Thanks for pointing this out anyway. It turns out we have alignment problems with couple other structures in UAPI.
We will send a separate fix for these.

Regards,
Jacek