Re: [Intel-xe] [PATCH 00/17] uAPI Alignment - take 1

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Sep 21, 2023 at 11:06:24AM -0400, Zhang, Carl wrote:
> Hi Rodrigo,
> Have a quick review,  several questions:
> 1. we need add pad in drm_xe_engine_class_instance, in the future, media may need to distinguish slim vdbox and full vdbox , or other special configurations. 
> So, reserve one word  should be suitable for future use. and then the structure length become 8 bytes.

it makes sense. let's do that.

> 
> 2.  no clear description about DRM_XE_DEVICE_QUERY_HWCONFIG, even we know it is in intel_hwconfig_types.h

Yes, it is that.
We also need to make that clear.

> 
> 3.  about query GT, if the there are multiple tile , and it is symmetrical, which one will be MAIN, which is REMOTE?

right now, in an hypothetical scenario where we have 2 tiles with 2 render GTs,
we would have 2 main GTs and 2 remote GTs. Is this a problem?

> 
> 4. no method to query HuC readiness . 

We are already discussing this and the trend is to extend the uc_fw_version that
Jose implemented for guc to also work for huc.

> 
> 5. XE_QUERY_CONFIG_NUM_PARAM is only used to represent the number of query type? And it is actually useless, no one should use it.

I agree. Let's remove it.

> 
> 6.
> +/** struct drm_xe_ext_set_property - XE set property extension */
> +struct drm_xe_ext_set_property { 
> It should be VM set property extension ( XE_VM_EXTENSION_SET_PROPERTY)?

Please take a look to patch 11
https://lore.kernel.org/all/20230920192940.135004-12-rodrigo.vivi@intel.com
It looks like a generic struct for all the possible set_property makes more
sense.

> 
> ps. I still not clear why we need extension for single bind operation. :) 

Every ioctl in xe is extensible by design as documented in the xe_drm.h.
That apparently matches the Vulkan extensibility and the desire is to
maintain that everywhere so we minimize the changes of creating new
versions of the ioctls later.

> may raise more question in following days. 

Thank you so much for all the review and comments so far.

> 
> thanks
> Carl
> 
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Sent: Tuesday, September 19, 2023 10:25 PM
> > To: intel-xe@lists.freedesktop.org
> > Cc: Hazubski, Filip <filip.hazubski@intel.com>; Kopryk, Kamil
> > <kamil.kopryk@intel.com>; Chaberek, Jakub <jakub.chaberek@intel.com>;
> > Dunajski, Bartosz <bartosz.dunajski@intel.com>; Souza, Jose
> > <jose.souza@intel.com>; Yu, Effie <effie.yu@intel.com>; Zhang, Carl
> > <carl.zhang@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Subject: [PATCH 00/17] uAPI Alignment - take 1
> > 
> > As a result of the uAPI review efforts started by Thomas[1], we have identified
> > many updates on our uAPI that would lead to breakage in the compatibility.
> > What it is not acceptable after we are merged upstream. So, let's break it
> > before it is too late, and start upstreaming a good, reliable and clean uapi.
> > 
> > Most of this work on putting these patches together for a single shot was led
> > by Francois.
> > 
> > The IGT counter part of this series is available as well[2].
> > 
> > [1] -
> > https://lore.kernel.org/all/863bebd0c624d6fc2b38c0a06b63e468b4185128.c
> > amel@l\
> > inux.intel.com/
> > [2] - https://lore.kernel.org/all/20230919142000.91363-1-
> > rodrigo.vivi@intel.com
> > 
> > Ashutosh Dixit (1):
> >   drm/xe/uapi: Use common drm_xe_ext_set_property extension
> > 
> > Francois Dugast (4):
> >   drm/xe/uapi: Separate VM_BIND's operation and flag
> >   drm/xe/vm: Remove VM_BIND_OP macro
> >   drm/xe/uapi: Remove MMIO ioctl
> >   drm/xe/uapi: Fix naming of
> > XE_QUERY_CONFIG_MAX_EXEC_QUEUE_PRIORITY
> > 
> > Matthew Brost (4):
> >   drm/xe: Fix xe_exec_queue_is_idle for parallel exec queues
> >   drm/xe: Deprecate XE_EXEC_QUEUE_SET_PROPERTY_COMPUTE_MODE
> >     implementation
> >   drm/xe: Rename exec_queue_kill_compute to
> >     xe_vm_remove_compute_exec_queue
> >   drm/xe: Remove XE_EXEC_QUEUE_SET_PROPERTY_COMPUTE_MODE from
> > uAPI
> > 
> > Rodrigo Vivi (5):
> >   drm/xe: Kill XE_VM_PROPERTY_BIND_OP_ERROR_CAPTURE_ADDRESS
> > extension
> >   drm/xe/uapi: Document drm_xe_query_gt
> >   drm/xe/uapi: Replace useless 'instance' per unique gt_id
> >   drm/xe/uapi: Remove unused field of drm_xe_query_gt
> >   drm/xe/uapi: Rename gts to gt_list
> > 
> > Umesh Nerlige Ramappa (3):
> >   drm/xe: Fix array bounds check for queries
> >   drm/xe: Set the correct type for xe_to_user_engine_class
> >   drm/xe: Correlate engine and cpu timestamps with better accuracy
> > 
> >  drivers/gpu/drm/xe/xe_device.c           |   1 -
> >  drivers/gpu/drm/xe/xe_exec_queue.c       |  99 +++-----
> >  drivers/gpu/drm/xe/xe_exec_queue_types.h |   6 +-
> >  drivers/gpu/drm/xe/xe_gt_types.h         |   2 +-
> >  drivers/gpu/drm/xe/xe_mmio.c             | 102 ---------
> >  drivers/gpu/drm/xe/xe_pci.c              |   4 -
> >  drivers/gpu/drm/xe/xe_query.c            | 187 +++++++++++++--
> >  drivers/gpu/drm/xe/xe_vm.c               | 221 +++++-------------
> >  drivers/gpu/drm/xe/xe_vm.h               |   1 +
> >  include/uapi/drm/xe_drm.h                | 280 ++++++++++++-----------
> >  10 files changed, 402 insertions(+), 501 deletions(-)
> > 
> > --
> > 2.41.0