Re: [PATCH v5] drm: Add UAPI for the Asahi driver

[Alyssa Rosenzweig]

> > +/**
> > + * 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@xxxxxxxxxx>

Thanks!