| Message ID | 20230731161258.2987564-5-stanislaw.gruszka@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | accel/ivpu: Refactor driver code and support new hardware | expand |
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. 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? 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. What do you think? Oded, am I misunderstanding your earlier comment? -Jeff
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
On Thu, Aug 03, 2023 at 10:37:37AM +0200, Stanislaw Gruszka wrote: > > 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. To avoid confusion, I'll change the topic and commit massage before applying: accel/ivpu: Extend get_param ioctl to identify capabilities Add DRM_IVPU_PARAM_CAPABILITIES parameters to get_param 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. Regards Stanislaw
On 8/8/2023 2:52 AM, Stanislaw Gruszka wrote: > On Thu, Aug 03, 2023 at 10:37:37AM +0200, Stanislaw Gruszka wrote: >>> 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. > > To avoid confusion, I'll change the topic and commit massage > before applying: > > accel/ivpu: Extend get_param ioctl to identify capabilities > > Add DRM_IVPU_PARAM_CAPABILITIES parameters to get_param 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. This is perhaps slightly better. I didn't find the original one confusing. Seems like no opinions on pushing this up to the framework. You did point out DRM drivers have driver level ones, so carry-on I guess. Seems ok to me. I'd prefer to see some comments in the uapi header describing what the DRM_IVPU_CAP_* values mean. A bit more than "device has metric streamer support" - what is metric streamer, and why might userspace care? However, as a uAPI change, is Oded's Ack not required? I thought that was the rule. -Jeff
Hi On Tue, Aug 08, 2023 at 06:45:51PM -0600, Jeffrey Hugo wrote: > On 8/8/2023 2:52 AM, Stanislaw Gruszka wrote: > > On Thu, Aug 03, 2023 at 10:37:37AM +0200, Stanislaw Gruszka wrote: > > > > 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. > > > > To avoid confusion, I'll change the topic and commit massage > > before applying: > > > > accel/ivpu: Extend get_param ioctl to identify capabilities > > > > Add DRM_IVPU_PARAM_CAPABILITIES parameters to get_param 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. > > This is perhaps slightly better. I didn't find the original one confusing. > > Seems like no opinions on pushing this up to the framework. You did point > out DRM drivers have driver level ones, so carry-on I guess. > > Seems ok to me. I'd prefer to see some comments in the uapi header > describing what the DRM_IVPU_CAP_* values mean. A bit more than "device has > metric streamer support" - what is metric streamer, and why might userspace > care? You have right, this should be documented. I'll send separate patch for this. > However, as a uAPI change, is Oded's Ack not required? I thought that was > the rule. I looked at git log from files in include/uapi/drm/ and seems that individual driver uAPI changes are up to the driver maintainer for drm misc drivers. At least there is no NACK from Oded so far :-) so I'm going to apply this, since want the changes to be merged in 6.6 merge window. Regards Stanislaw
On 8/9/2023 5:24 AM, Stanislaw Gruszka wrote: > Hi > > On Tue, Aug 08, 2023 at 06:45:51PM -0600, Jeffrey Hugo wrote: >> On 8/8/2023 2:52 AM, Stanislaw Gruszka wrote: >>> On Thu, Aug 03, 2023 at 10:37:37AM +0200, Stanislaw Gruszka wrote: >>>>> 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. >>> >>> To avoid confusion, I'll change the topic and commit massage >>> before applying: >>> >>> accel/ivpu: Extend get_param ioctl to identify capabilities >>> >>> Add DRM_IVPU_PARAM_CAPABILITIES parameters to get_param 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. >> >> This is perhaps slightly better. I didn't find the original one confusing. >> >> Seems like no opinions on pushing this up to the framework. You did point >> out DRM drivers have driver level ones, so carry-on I guess. >> >> Seems ok to me. I'd prefer to see some comments in the uapi header >> describing what the DRM_IVPU_CAP_* values mean. A bit more than "device has >> metric streamer support" - what is metric streamer, and why might userspace >> care? > > You have right, this should be documented. I'll send separate patch for > this. Sounds good. If you like, Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com> > >> However, as a uAPI change, is Oded's Ack not required? I thought that was >> the rule. > > I looked at git log from files in include/uapi/drm/ and seems that individual > driver uAPI changes are up to the driver maintainer for drm misc drivers. I was referencing this - "<snip> bugfixes for the qaic driver can be pushed directly by the qaic team. Still with acks/r-b requirements as per usual, and I guess for anything bigger/new uapi an ack from oded is needed." https://lore.kernel.org/dri-devel/ZC13QdSRybIe3nvk@phenom.ffwll.local/ Maybe that is qaic specific rules since we are rather new to the community and still learning. > At least there is no NACK from Oded so far :-) so I'm going to apply this, > since want the changes to be merged in 6.6 merge window. > > Regards > Stanislaw
diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c index fad607dbb2c6..d33eb17007bf 100644 --- a/drivers/accel/ivpu/ivpu_drv.c +++ b/drivers/accel/ivpu/ivpu_drv.c @@ -115,6 +115,22 @@ void ivpu_file_priv_put(struct ivpu_file_priv **link) kref_put(&file_priv->ref, file_priv_release); } +static int ivpu_get_capabilities(struct ivpu_device *vdev, struct drm_ivpu_param *args) +{ + switch (args->index) { + case DRM_IVPU_CAP_METRIC_STREAMER: + args->value = 0; + break; + case DRM_IVPU_CAP_DMA_MEMORY_RANGE: + args->value = 0; + break; + default: + return -EINVAL; + } + + return 0; +} + static int ivpu_get_param_ioctl(struct drm_device *dev, void *data, struct drm_file *file) { struct ivpu_file_priv *file_priv = file->driver_priv; @@ -174,6 +190,9 @@ static int ivpu_get_param_ioctl(struct drm_device *dev, void *data, struct drm_f case DRM_IVPU_PARAM_SKU: args->value = vdev->hw->sku; break; + case DRM_IVPU_PARAM_CAPABILITIES: + ret = ivpu_get_capabilities(vdev, args); + break; default: ret = -EINVAL; break; diff --git a/include/uapi/drm/ivpu_accel.h b/include/uapi/drm/ivpu_accel.h index 839820aed87e..3e99b74eef04 100644 --- a/include/uapi/drm/ivpu_accel.h +++ b/include/uapi/drm/ivpu_accel.h @@ -60,6 +60,7 @@ extern "C" { #define DRM_IVPU_PARAM_UNIQUE_INFERENCE_ID 10 #define DRM_IVPU_PARAM_TILE_CONFIG 11 #define DRM_IVPU_PARAM_SKU 12 +#define DRM_IVPU_PARAM_CAPABILITIES 13 #define DRM_IVPU_PLATFORM_TYPE_SILICON 0 @@ -68,6 +69,9 @@ extern "C" { #define DRM_IVPU_CONTEXT_PRIORITY_FOCUS 2 #define DRM_IVPU_CONTEXT_PRIORITY_REALTIME 3 +#define DRM_IVPU_CAP_METRIC_STREAMER 1 +#define DRM_IVPU_CAP_DMA_MEMORY_RANGE 2 + /** * struct drm_ivpu_param - Get/Set VPU parameters */