> > +/**
> > + * struct drm_asahi_params_global - Global parameters.
> > + *
> > + * This struct may be queried by drm_asahi_get_params.
> > + */
> > +struct drm_asahi_params_global {
> > + /** @features: Feature bits from drm_asahi_feature */
> > + __u64 features;
> > +
> > + /** @gpu_generation: GPU generation, e.g. 13 for G13G */
> > + __u32 gpu_generation;
> > +
> > + /** @gpu_variant: GPU variant as a character, e.g. 'G' for G13G */
> > + __u32 gpu_variant;
>
> nit: the example can avoid the duplication of 'G' with "e.g. 'C' for
> G13C"
done in v6, thanks.
> > +/**
> > + * struct drm_asahi_get_params - Arguments passed to
> > DRM_IOCTL_ASAHI_GET_PARAMS
> > + */
> > +struct drm_asahi_get_params {
> > + /** @param_group: Parameter group to fetch (MBZ) */
> > + __u32 param_group;
> > +
> > + /** @pad: MBZ */
> > + __u32 pad;
> > +
> > + /** @pointer: User pointer to write parameter struct */
> > + __u64 pointer;
> > +
> > + /** @size: Size of user buffer, max size supported on return */
> > + __u64 size;
>
> The comment is misleading in the case of newer / extended kernel which
> supports a larger size than supplied. You could change it to "size
> written on return" or clarify that the value on return will not exceed
> the input value.
fixed.
> > +struct drm_asahi_vm_create {
> > + /**
> > + * @kernel_start: Start of the kernel-reserved address range. See
> > + * drm_asahi_params_global::vm_kernel_min_size.
> > + *
> > + * Both @kernel_start and @kernel_end must be within the range of
> > + * valid VAs given by drm_asahi_params_global::vm_user_start and
> > + * drm_asahi_params_global::vm_user_end. The size of the kernel range
>
> This reads a little strange. Would it make sense to rename
> drm_asahi_params_global's
> vm_user_start and vm_user_end to vm_start/vm_end?
Good point, renamed.
> > + /**
> > + * @bind: Union holding the bind request.
> > + *
> > + * This union is named to make the Rust bindings nicer to work with.
> > + */
>
> This comment could use a short justification why this union does not
> defeat extensibility after the initial statement that "structures should
> not contain unions"
Added.
> > + /**
> > + * @syncs: An optional array of drm_asahi_sync. First @in_sync_count
> > + * in-syncs then @out_sync_count out-syncs.
> > + */
> > + __u64 syncs;
>
> Would it make sense to explictly state that this is a pointer?
Done.
> Reviewed-by: Janne Grunau <j...@jannau.net>
Thanks!
