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

[Jeff Hugo <jeff.hugo@oss.qualcomm.com>]

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@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@oss.qualcomm.com>