Re: [EXTERNAL] Re: [PATCH v5 03/17] drm/imagination/uapi: Add PowerVR driver UAPI

[Sarah Walker]

On Thu, 2023-08-17 at 19:43 -0500, Faith Ekstrand wrote:
> On Wed, Aug 16, 2023 at 3:26 AM Sarah Walker <sarah.walker@xxxxxxxxxx> wrote:
> > 
> > +/**
> > + * struct drm_pvr_dev_query_runtime_info - Container used to fetch information
> > + * about the graphics runtime.
> > + *
> > + * When fetching this type &struct drm_pvr_ioctl_dev_query_args.type must be set
> > + * to %DRM_PVR_DEV_QUERY_RUNTIME_INFO_GET.
> > + */
> > +struct drm_pvr_dev_query_runtime_info {
> > +       /**
> > +        * @free_list_min_pages: Minimum allowed free list size,
> > +        * in PM physical pages.
> > +        */
> > +       __u64 free_list_min_pages;
> > +
> > +       /**
> > +        * @free_list_max_pages: Maximum allowed free list size,
> > +        * in PM physical pages.
> > +        */
> > +       __u64 free_list_max_pages;
> > +
> > +       /**
> > +        * @common_store_alloc_region_size: Size of the Allocation
> > +        * Region within the Common Store used for coefficient and shared
> > +        * registers, in dwords.
> > +        */
> > +       __u32 common_store_alloc_region_size;
> 
> Any reason why this is in dwords?  It's not really my place to have an opinion but that seems like kind-of a funny unit for the size of an allocation region. Why not just bytes?

This is a holdover from the closed source driver. It can be changed to bytes if
that is particularly desired?

> +/**
> > + * struct drm_pvr_dev_query_quirks - Container used to fetch information about
> > + * hardware fixes for which the device may require support in the user mode
> > + * driver.
> > + *
> > + * When fetching this type &struct drm_pvr_ioctl_dev_query_args.type must be set
> > + * to %DRM_PVR_DEV_QUERY_QUIRKS_GET.
> > + */
> > +struct drm_pvr_dev_query_quirks {
> > +       /**
> > +        * @quirks: A userspace address for the hardware quirks __u32 array.
> > +        *
> > +        * The first @musthave_count items in the list are quirks that the
> > +        * client must support for this device. If userspace does not support
> > +        * all these quirks then functionality is not guaranteed and client
> > +        * initialisation must fail.
> > +        * The remaining quirks in the list affect userspace and the kernel or
> > +        * firmware. They are disabled by default and require userspace to
> > +        * opt-in. The opt-in mechanism depends on the quirk.
> > +        */
> > +       __u64 quirks;
> 
> Where are these quirk IDs defined and where do they come from? If they're effectively coming from hardware, possibly via firmware, that's probably okay.  The important thing is that quirks should only ever get removed for any given piece of hardware otherwise you risk breaking userspace.

Quirks are defined in the firmware header. The actual IDs are from our issue
tracking system; they're shared with the closed source driver. We are aware of
the need to not remove quirks for a given GPU.

> > +/**
> > + * struct drm_pvr_dev_query_enhancements - Container used to fetch information
> > + * about optional enhancements supported by the device that require support in
> > + * the user mode driver.
> > + *
> > + * When fetching this type &struct drm_pvr_ioctl_dev_query_args.type must be set
> > + * to %DRM_PVR_DEV_ENHANCEMENTS_GET.
> > + */
> > +struct drm_pvr_dev_query_enhancements {
> > +       /**
> > +        * @enhancements: A userspace address for the hardware enhancements
> > +        * __u32 array.
> > +        *
> > +        * These enhancements affect userspace and the kernel or firmware. They
> > +        * are disabled by default and require userspace to opt-in. The opt-in
> > +        * mechanism depends on the quirk.
> > +        */
> > +       __u64 enhancements;
> 
> Can you provide some examples of "enhancements"? Not that you need to put it in the docs. I'm just trying to understand what this API is doing so I can better review. Again, where do these come from? Also, how is an enhancement different from a quirk?

Enhancements are comparatively minor improvements in GPU subrevisions that don't
qualify as a full "product feature". A couple of examples would be 35421, which
improves compute thread barrier support, and 42064, which adds mask support for
the pixel backend. As with quirks, enhancements are defined in the firmware
header, with the IDs coming from our issue tracker.

Thanks,
Sarah