On 6/11/2024 14:45, Michal Wajdeczko wrote:
On 11.06.2024 22:32, John Harrison wrote:
On 6/11/2024 07:30, Michal Wajdeczko wrote:
There are many GuC ABI definitions named in the same way by the i915
and Xe drivers, preventing proper generation of the documentation.
Promote GuC ABI definitions to shared location that can be used by
both drivers and can be included in documentation.
I still very strongly feel that this is the wrong place for such
documentation. We do not document any of the hardware registers in the
driver, nor the MI_ instructions, etc. Why is this any different? The
GuC ABI is not under the control of the Linux kernel driver, either i915
or Xe. It is effectively a hardware interface no different to any other
hardware interface. It is already fully documented by the owners of that
interface. Rather than just copying random chunks of that documentation
into the kernel driver, we should just be publishing the official
document itself. Same as we do for the rest of the hardware.
so go publish this official document
My point is that we should be putting effort into making this happen. So
far as I know, I am the only person that ever raises this topic. And I
do keep raising it only to be told it is too low a priority. If more
people actually pushed for it then maybe it would happen.
in the meantime IMO it is useful to show, with really little effort, on
what grounds the communication between i915/Xe and GuC works, so
everyone, not just selected engineers, can understand and review our
implementation and check its correctness
Simply providing a half sentence description of a bitfield definition
does not tell you anything about the correctness or usage of an
interface. Sure, you can tell that it is syntactically correct but the
compiler can do that for you with the appropriate FIELD definitions. It
does not tell you anything about whether you are using the interface
properly or even if this is the proper interface to use. You need the
full descriptive documentation for that. Are you going to copy all of
that into the driver? Then you might as well just publish the document
you are copying from and be done.
furthermore, if you don't like any hw documentation then we should
revisit what is already in gpu/i915 section and also reconsider all our
efforts to document all non-static driver functions, as those functions
are still internal to the driver, not to be used outside
I do not know what you are arguing for here.
Xe in general is being pushed to have much better documentation of the
code than i915 did. But documentation of the driver is a very different
topic to documentation of the hardware.
John.
John.
Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
Cc: Matthew Brost <matthew.brost@xxxxxxxxx>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Cc: John Harrison <John.C.Harrison@xxxxxxxxx>
Michal Wajdeczko (7):
drm/xe/guc: Promote GuC ABI headers to shared location
Documentation/gpu: Separate GuC ABI section
Documentation/gpu: Switch to shared GuC ABI definitions
drm/intel/guc: Update CTB communication ABI
drm/intel/guc: Add new KLV definitions
drm/i915: Use shared GuC ABI definitions
drm/xe: Promote SR-IOV GuC ABI definitions to shared location
Documentation/gpu/drivers.rst | 1 +
Documentation/gpu/guc.rst | 23 ++
Documentation/gpu/i915.rst | 9 -
drivers/gpu/drm/i915/Makefile | 5 +
.../gt/uc/abi/guc_communication_ctb_abi.h | 170 -----------
.../gt/uc/abi/guc_communication_mmio_abi.h | 49 ----
drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h | 112 --------
.../gpu/drm/i915/gt/uc/abi/guc_messages_abi.h | 264 ------------------
.../guc}/abi/guc_actions_sriov_abi.h | 0
.../guc}/abi/guc_communication_ctb_abi.h | 2 +
.../guc}/abi/guc_communication_mmio_abi.h | 0
.../drm/{xe => intel/guc}/abi/guc_klvs_abi.h | 18 +-
.../{xe => intel/guc}/abi/guc_messages_abi.h | 0
.../guc}/abi/guc_relay_actions_abi.h | 0
.../guc}/abi/guc_relay_communication_abi.h | 0
drivers/gpu/drm/xe/Makefile | 5 +
16 files changed, 49 insertions(+), 609 deletions(-)
create mode 100644 Documentation/gpu/guc.rst
delete mode 100644
drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h
delete mode 100644
drivers/gpu/drm/i915/gt/uc/abi/guc_communication_mmio_abi.h
delete mode 100644 drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h
delete mode 100644 drivers/gpu/drm/i915/gt/uc/abi/guc_messages_abi.h
rename drivers/gpu/drm/{xe => intel/guc}/abi/guc_actions_sriov_abi.h
(100%)
rename drivers/gpu/drm/{xe =>
intel/guc}/abi/guc_communication_ctb_abi.h (98%)
rename drivers/gpu/drm/{xe =>
intel/guc}/abi/guc_communication_mmio_abi.h (100%)
rename drivers/gpu/drm/{xe => intel/guc}/abi/guc_klvs_abi.h (97%)
rename drivers/gpu/drm/{xe => intel/guc}/abi/guc_messages_abi.h (100%)
rename drivers/gpu/drm/{xe => intel/guc}/abi/guc_relay_actions_abi.h
(100%)
rename drivers/gpu/drm/{xe =>
intel/guc}/abi/guc_relay_communication_abi.h (100%)
