Re: [PATCH 4/6] accel/ivpu: Add param ioctl to identify capabilities

[Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>]

Hi

On Wed, Aug 02, 2023 at 11:07:09AM -0600, Jeffrey Hugo wrote:
> On 7/31/2023 10:12 AM, Stanislaw Gruszka wrote:
> > Add DRM_IVPU_PARAM_CAPABILITIES ioctl to query driver capabilities.
> > For now use it for identify metric streamer and new dma memory range
> > features. Currently upstream version of intel_vpu does not have those,
> > they will be added it the future.
> 
> Hmm.  So I happened to remember this from Oded in the reviews for the first
> iVPU series -
> 
> "btw, I talked to Daniel about this and he told me this
> major/minor/patchlevel is legacy in drm and should be deprecated, so
> I'm going to send a patch to document that it is deprecated and how to
> use getcap ioctl to find out the features the driver support."
> 
> So, I went to look at DRM_IOCTL_GET_CAP and it feels like something you
> should be using as it fits the purpose I see in this patch, but also I don't
> see how given that it doesn't hook to the driver.

Indeed it does not have such extension. I considered DRM_IOCTL_GET_CAP, but
did not use it because of that. Seems that DRM drivers that need provide
set of capabilities just create own "getcap" ioctl.

> I suspect at some point, QAIC would have its own need for a "getcap" API.
> Feels like something we should have a common entry in uAPI for
> (ACCEL_IOCTL_GET_CAP ? ), but I don't yet see that we'll have a lot a
> commonality of the capabilities across Accel drivers.  I don't think the DRM
> method of globally defined caps is useful for Accel.
> 
> Does it make sense to have a framework level ioctl, but something that calls
> into the drivers and would allow the drivers to advertise custom
> capabilities?

Perhaps such thing would make sense, resembles to me ethtool for network
devices. But not sure, maybe just having separate ioctls per driver and one
common for common features is simpler.

> Seems like we might want to decide this now, because if we define a iVPU
> specific ioctl as proposed here, but then switch to an Accel-wide mechanism
> later, iVPU is going to be stuck supporting both.

For the record, we do not add new ioctl in this patch, we just extend
existing DRM_IOCTL_IVPU_GET_PARAM one.

> What do you think?

My understating was that this is more like design patter, i.e. drivers
that have to advertise supported features should not use minor/major
numbers but "getcap" ioctl. And that not necessary mean we should have
common ioctl for that for all ACCEL/DRM drivers.

Regards
Stanislaw