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.kacprow...@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.h...@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
