[PATCH 1/4] drm/amdgpu: Introduce gfx software ring(v4)

[PATCH 1/4] drm/amdgpu: Introduce gfx software ring(v4)

[jiadong.zhu]

From: "Jiadong.Zhu" <Jiadong.Zhu@amd.com>

The software ring is created to support priority
context while there is only one hardware queue
for gfx.

Every software rings has its fence driver and could
be used as an ordinary ring for the gpu_scheduler.
Multiple software rings are binded to a real ring
with the ring muxer. The packages committed on the
software ring are copied to the real ring.

v2: use array to store software ring entry.
v3: remove unnecessary prints.
v4: remove amdgpu_ring_sw_init/fini functions,
using gtt for sw ring buffer for later dma copy
optimization.

Signed-off-by: Jiadong.Zhu <Jiadong.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/Makefile          |   3 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h      |   3 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h     |   3 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c | 182 +++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h |  67 +++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c  |  60 ++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h  |  43 +++++
 7 files changed, 360 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
 create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
 create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
 create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
index 3e0e2eb7e235..85224bc81ce5 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -58,7 +58,8 @@ amdgpu-y += amdgpu_device.o amdgpu_kms.o \
 	amdgpu_vm_sdma.o amdgpu_discovery.o amdgpu_ras_eeprom.o amdgpu_nbio.o \
 	amdgpu_umc.o smu_v11_0_i2c.o amdgpu_fru_eeprom.o amdgpu_rap.o \
 	amdgpu_fw_attestation.o amdgpu_securedisplay.o \
-	amdgpu_eeprom.o amdgpu_mca.o amdgpu_psp_ta.o amdgpu_lsdma.o
+	amdgpu_eeprom.o amdgpu_mca.o amdgpu_psp_ta.o amdgpu_lsdma.o \
+	amdgpu_sw_ring.o amdgpu_ring_mux.o
 
 amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
index 53526ffb2ce1..0de8e3cd0f1c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
@@ -33,6 +33,7 @@
 #include "amdgpu_imu.h"
 #include "soc15.h"
 #include "amdgpu_ras.h"
+#include "amdgpu_ring_mux.h"
 
 /* GFX current status */
 #define AMDGPU_GFX_NORMAL_MODE			0x00000000L
@@ -346,6 +347,8 @@ struct amdgpu_gfx {
 	struct amdgpu_gfx_ras		*ras;
 
 	bool				is_poweron;
+
+	struct amdgpu_ring_mux			muxer;
 };
 
 #define amdgpu_gfx_get_gpu_clock_counter(adev) (adev)->gfx.funcs->get_gpu_clock_counter((adev))
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index 7d89a52091c0..fe33a683bfba 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -278,6 +278,9 @@ struct amdgpu_ring {
 	bool			is_mes_queue;
 	uint32_t		hw_queue_id;
 	struct amdgpu_mes_ctx_data *mes_ctx;
+
+	bool			is_sw_ring;
+
 };
 
 #define amdgpu_ring_parse_cs(r, p, job, ib) ((r)->funcs->parse_cs((p), (job), (ib)))
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
new file mode 100644
index 000000000000..652a6d3e0ec3
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
@@ -0,0 +1,182 @@
+/*
+ * Copyright 2022 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+#include <drm/drm_print.h>
+
+#include "amdgpu_ring_mux.h"
+#include "amdgpu_ring.h"
+
+#define AMDGPU_MUX_RESUBMIT_JIFFIES_TIMEOUT (HZ/2)
+
+static int copy_pkt_from_sw_ring(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring,
+	u64 s_begin, u64 s_end);
+
+int amdgpu_ring_mux_init(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring)
+{
+	mux->real_ring = ring;
+	memset(mux->ring_entries, 0, sizeof(mux->ring_entries));
+	mux->num_ring_entries = 0;
+	spin_lock_init(&mux->lock);
+	return 0;
+}
+
+void amdgpu_ring_mux_fini(struct amdgpu_ring_mux *mux)
+{
+	memset(mux->ring_entries, 0, sizeof(mux->ring_entries));
+	mux->num_ring_entries = 0;
+}
+
+int amdgpu_ring_mux_add_sw_ring(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring)
+{
+	struct amdgpu_mux_entry *e;
+
+	if (mux->num_ring_entries == AMDGPU_MAX_GFX_RINGS) {
+		DRM_ERROR("adding sw ring exceeds max gfx num\n");
+		return -ENOMEM;
+	}
+
+	e = &mux->ring_entries[mux->num_ring_entries++];
+
+	e->ring = ring;
+	e->start_ptr_in_hw_ring = 0;
+	e->end_ptr_in_hw_ring = 0;
+	e->sw_cptr = 0;
+	e->sw_rptr = 0;
+	e->sw_wptr = 0;
+
+	return 0;
+}
+
+static struct amdgpu_mux_entry *amdgpu_get_sw_entry(struct amdgpu_ring_mux *mux,
+				struct amdgpu_ring *ring)
+{
+	struct amdgpu_mux_entry *e;
+	int i;
+
+	e = NULL;
+	for (i = 0; i < mux->num_ring_entries; i++) {
+		if (mux->ring_entries[i].ring == ring) {
+			e = &mux->ring_entries[i];
+			break;
+		}
+	}
+
+	return e;
+}
+
+void amdgpu_ring_set_wptr_to_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring, u64 wptr)
+{
+	struct amdgpu_mux_entry *e;
+
+	e = amdgpu_get_sw_entry(mux, ring);
+	if (!e) {
+		DRM_ERROR("cannot find entry for sw ring\n");
+		return;
+	}
+
+	spin_lock(&mux->lock);
+	e->sw_cptr = e->sw_wptr;
+	e->sw_wptr = wptr;
+	e->start_ptr_in_hw_ring = mux->real_ring->wptr;
+
+	if (copy_pkt_from_sw_ring(mux, ring, e->sw_cptr, wptr) == 0) {
+		e->end_ptr_in_hw_ring = mux->real_ring->wptr;
+		amdgpu_ring_commit(mux->real_ring);
+	}
+
+	spin_unlock(&mux->lock);
+}
+
+u64 amdgpu_ring_get_wptr_from_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring)
+{
+	struct amdgpu_mux_entry *e;
+
+	e = amdgpu_get_sw_entry(mux, ring);
+	if (!e) {
+		DRM_ERROR("cannot find entry for sw ring\n");
+		return 0;
+	}
+
+	return e->sw_wptr;
+}
+
+u64 amdgpu_ring_get_rptr_from_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring)
+{
+	struct amdgpu_mux_entry *e;
+	u64 r_rptr, offset, start, end;
+
+	e = amdgpu_get_sw_entry(mux, ring);
+	if (!e) {
+		DRM_ERROR("no sw entry found!\n");
+		return 0;
+	}
+
+	r_rptr = amdgpu_ring_get_rptr(mux->real_ring);
+
+	start = e->start_ptr_in_hw_ring & mux->real_ring->buf_mask;
+	end = e->end_ptr_in_hw_ring & mux->real_ring->buf_mask;
+	if (start > end) {
+		if (r_rptr <= end)
+			r_rptr += mux->real_ring->ring_size >> 2;
+		end += mux->real_ring->ring_size >> 2;
+	}
+
+	if (r_rptr <= end && r_rptr >= start) {
+		offset = r_rptr - start;
+		e->sw_rptr = (e->sw_cptr + offset) & ring->buf_mask;
+	} else if (r_rptr < start) {
+		e->sw_rptr = e->sw_cptr;
+	} else {
+		e->sw_rptr = e->sw_wptr;
+	}
+
+	return e->sw_rptr;
+}
+
+/*copy packages on sw ring range[begin, end) */
+static int copy_pkt_from_sw_ring(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring,
+	u64 s_begin, u64 s_end)
+{
+	u64 begin, end, r_begin, r_end;
+	struct amdgpu_ring *real_ring = mux->real_ring;
+
+	begin = s_begin & ring->buf_mask;
+	end = s_end & ring->buf_mask;
+
+	r_begin = real_ring->wptr & real_ring->buf_mask;
+	if (begin == end)
+		return -ERANGE;
+	if (begin > end) {
+		amdgpu_ring_alloc(real_ring, (ring->ring_size >> 2) + end - begin);
+		amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[begin],
+			(ring->ring_size >> 2) - begin);
+		amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[0], end);
+	} else {
+		amdgpu_ring_alloc(real_ring, end - begin);
+		amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[begin], end - begin);
+	}
+
+	r_end = real_ring->wptr & real_ring->buf_mask;
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
new file mode 100644
index 000000000000..d058c43bb063
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
@@ -0,0 +1,67 @@
+/*
+ * Copyright 2022 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+#ifndef __AMDGPU_RING_MUX__
+#define __AMDGPU_RING_MUX__
+
+#include <linux/timer.h>
+#include <linux/spinlock.h>
+#include "amdgpu_ring.h"
+
+struct amdgpu_ring;
+/*
+ * start_ptr_in_hw_ring - last copied start loc on hw ring
+ * end_ptr_in_hw_ring - last copied end loc on hw ring
+ *sw_cptr -the begin of copy ptr in sw ring
+ *sw_rptr; the read ptr in sw ring
+ *sw_wptr; the write ptr in sw ring
+ */
+struct amdgpu_mux_entry {
+	struct amdgpu_ring	*ring;
+	u64 start_ptr_in_hw_ring;
+	u64 end_ptr_in_hw_ring;
+
+	u64 sw_cptr;
+	u64 sw_rptr;
+	u64 sw_wptr;
+};
+
+struct amdgpu_ring_mux {
+	struct amdgpu_ring *real_ring;
+
+	struct amdgpu_mux_entry ring_entries[AMDGPU_MAX_GFX_RINGS];
+
+	unsigned num_ring_entries;
+
+	spinlock_t			lock;
+
+};
+
+int amdgpu_ring_mux_init(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring);
+void amdgpu_ring_mux_fini(struct amdgpu_ring_mux *mux);
+int amdgpu_ring_mux_add_sw_ring(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring);
+void amdgpu_ring_set_wptr_to_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring, u64 wptr);
+u64 amdgpu_ring_get_wptr_from_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring);
+u64 amdgpu_ring_get_rptr_from_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring);
+
+#endif
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
new file mode 100644
index 000000000000..7a59e3fbb970
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
@@ -0,0 +1,60 @@
+/*
+ * Copyright 2022 Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
+ * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+ * USE OR OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ */
+
+#include "amdgpu_sw_ring.h"
+#include "amdgpu_ring_mux.h"
+
+u64 amdgpu_sw_ring_get_rptr_gfx(struct amdgpu_ring *ring)
+{
+	struct amdgpu_device *adev = ring->adev;
+	struct amdgpu_ring_mux *mux = &adev->gfx.muxer;
+
+	BUG_ON(!ring->is_sw_ring);
+	return amdgpu_ring_get_rptr_from_mux(mux, ring);
+}
+
+u64 amdgpu_sw_ring_get_wptr_gfx(struct amdgpu_ring *ring)
+{
+	struct amdgpu_device *adev = ring->adev;
+	struct amdgpu_ring_mux *mux = &adev->gfx.muxer;
+
+	BUG_ON(!ring->is_sw_ring);
+	return amdgpu_ring_get_wptr_from_mux(mux, ring);
+}
+
+void amdgpu_sw_ring_set_wptr_gfx(struct amdgpu_ring *ring)
+{
+	BUG_ON(!ring->is_sw_ring);
+}
+
+void amdgpu_sw_ring_commit(struct amdgpu_ring *ring)
+{
+	struct amdgpu_device *adev = ring->adev;
+	struct amdgpu_ring_mux *mux = &adev->gfx.muxer;
+
+	BUG_ON(!ring->is_sw_ring);
+	amdgpu_ring_set_wptr_to_mux(mux, ring, ring->wptr);
+}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h
new file mode 100644
index 000000000000..0a2e66318f3f
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h
@@ -0,0 +1,43 @@
+/*
+ * Copyright 2012 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+#include <drm/amdgpu_drm.h>
+#include <drm/gpu_scheduler.h>
+#include <drm/drm_print.h>
+
+#include "amdgpu_irq.h"
+#include "amdgpu_ring.h"
+#include "amdgpu.h"
+
+#ifndef __AMDGPU_SWRING_H__
+#define __AMDGPU_SWRING_H__
+
+u64 amdgpu_sw_ring_get_rptr_gfx(struct amdgpu_ring *ring);
+u64 amdgpu_sw_ring_get_wptr_gfx(struct amdgpu_ring *ring);
+void amdgpu_sw_ring_set_wptr_gfx(struct amdgpu_ring *ring);
+void amdgpu_sw_ring_commit(struct amdgpu_ring *ring);
+
+void amdgpu_sw_ring_ib_begin(struct amdgpu_ring *ring);
+void amdgpu_sw_ring_ib_end(struct amdgpu_ring *ring);
+
+#endif
-- 
2.25.1

Re: [PATCH 1/4] drm/amdgpu: Introduce gfx software ring(v4)

[Christian König]

Am 13.09.22 um 11:05 schrieb jiadong.zhu@amd.com:
> From: "Jiadong.Zhu" <Jiadong.Zhu@amd.com>
>
> The software ring is created to support priority
> context while there is only one hardware queue
> for gfx.
>
> Every software rings has its fence driver and could
> be used as an ordinary ring for the gpu_scheduler.
> Multiple software rings are binded to a real ring
> with the ring muxer. The packages committed on the
> software ring are copied to the real ring.
>
> v2: use array to store software ring entry.
> v3: remove unnecessary prints.
> v4: remove amdgpu_ring_sw_init/fini functions,
> using gtt for sw ring buffer for later dma copy
> optimization.
>
> Signed-off-by: Jiadong.Zhu <Jiadong.Zhu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/Makefile          |   3 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h      |   3 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h     |   3 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c | 182 +++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h |  67 +++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c  |  60 ++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h  |  43 +++++
>   7 files changed, 360 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
>   create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
> index 3e0e2eb7e235..85224bc81ce5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -58,7 +58,8 @@ amdgpu-y += amdgpu_device.o amdgpu_kms.o \
>   	amdgpu_vm_sdma.o amdgpu_discovery.o amdgpu_ras_eeprom.o amdgpu_nbio.o \
>   	amdgpu_umc.o smu_v11_0_i2c.o amdgpu_fru_eeprom.o amdgpu_rap.o \
>   	amdgpu_fw_attestation.o amdgpu_securedisplay.o \
> -	amdgpu_eeprom.o amdgpu_mca.o amdgpu_psp_ta.o amdgpu_lsdma.o
> +	amdgpu_eeprom.o amdgpu_mca.o amdgpu_psp_ta.o amdgpu_lsdma.o \
> +	amdgpu_sw_ring.o amdgpu_ring_mux.o
>   
>   amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> index 53526ffb2ce1..0de8e3cd0f1c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> @@ -33,6 +33,7 @@
>   #include "amdgpu_imu.h"
>   #include "soc15.h"
>   #include "amdgpu_ras.h"
> +#include "amdgpu_ring_mux.h"
>   
>   /* GFX current status */
>   #define AMDGPU_GFX_NORMAL_MODE			0x00000000L
> @@ -346,6 +347,8 @@ struct amdgpu_gfx {
>   	struct amdgpu_gfx_ras		*ras;
>   
>   	bool				is_poweron;
> +
> +	struct amdgpu_ring_mux			muxer;
>   };
>   
>   #define amdgpu_gfx_get_gpu_clock_counter(adev) (adev)->gfx.funcs->get_gpu_clock_counter((adev))
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index 7d89a52091c0..fe33a683bfba 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -278,6 +278,9 @@ struct amdgpu_ring {
>   	bool			is_mes_queue;
>   	uint32_t		hw_queue_id;
>   	struct amdgpu_mes_ctx_data *mes_ctx;
> +
> +	bool			is_sw_ring;
> +
>   };
>   
>   #define amdgpu_ring_parse_cs(r, p, job, ib) ((r)->funcs->parse_cs((p), (job), (ib)))
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
> new file mode 100644
> index 000000000000..652a6d3e0ec3
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
> @@ -0,0 +1,182 @@
> +/*
> + * Copyright 2022 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#include <drm/drm_print.h>
> +
> +#include "amdgpu_ring_mux.h"
> +#include "amdgpu_ring.h"
> +
> +#define AMDGPU_MUX_RESUBMIT_JIFFIES_TIMEOUT (HZ/2)
> +
> +static int copy_pkt_from_sw_ring(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring,
> +	u64 s_begin, u64 s_end);
> +
> +int amdgpu_ring_mux_init(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring)
> +{
> +	mux->real_ring = ring;
> +	memset(mux->ring_entries, 0, sizeof(mux->ring_entries));
> +	mux->num_ring_entries = 0;
> +	spin_lock_init(&mux->lock);
> +	return 0;
> +}
> +
> +void amdgpu_ring_mux_fini(struct amdgpu_ring_mux *mux)
> +{
> +	memset(mux->ring_entries, 0, sizeof(mux->ring_entries));
> +	mux->num_ring_entries = 0;
> +}
> +
> +int amdgpu_ring_mux_add_sw_ring(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring)
> +{
> +	struct amdgpu_mux_entry *e;
> +
> +	if (mux->num_ring_entries == AMDGPU_MAX_GFX_RINGS) {
> +		DRM_ERROR("adding sw ring exceeds max gfx num\n");
> +		return -ENOMEM;
> +	}
> +
> +	e = &mux->ring_entries[mux->num_ring_entries++];
> +
> +	e->ring = ring;
> +	e->start_ptr_in_hw_ring = 0;
> +	e->end_ptr_in_hw_ring = 0;
> +	e->sw_cptr = 0;
> +	e->sw_rptr = 0;
> +	e->sw_wptr = 0;
> +
> +	return 0;
> +}
> +
> +static struct amdgpu_mux_entry *amdgpu_get_sw_entry(struct amdgpu_ring_mux *mux,
> +				struct amdgpu_ring *ring)
> +{
> +	struct amdgpu_mux_entry *e;
> +	int i;
> +
> +	e = NULL;
> +	for (i = 0; i < mux->num_ring_entries; i++) {
> +		if (mux->ring_entries[i].ring == ring) {
> +			e = &mux->ring_entries[i];
> +			break;
> +		}
> +	}
> +
> +	return e;
> +}
> +
> +void amdgpu_ring_set_wptr_to_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring, u64 wptr)
> +{
> +	struct amdgpu_mux_entry *e;
> +
> +	e = amdgpu_get_sw_entry(mux, ring);
> +	if (!e) {
> +		DRM_ERROR("cannot find entry for sw ring\n");
> +		return;
> +	}
> +
> +	spin_lock(&mux->lock);
> +	e->sw_cptr = e->sw_wptr;
> +	e->sw_wptr = wptr;
> +	e->start_ptr_in_hw_ring = mux->real_ring->wptr;
> +
> +	if (copy_pkt_from_sw_ring(mux, ring, e->sw_cptr, wptr) == 0) {
> +		e->end_ptr_in_hw_ring = mux->real_ring->wptr;
> +		amdgpu_ring_commit(mux->real_ring);
> +	}
> +
> +	spin_unlock(&mux->lock);
> +}
> +
> +u64 amdgpu_ring_get_wptr_from_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring)
> +{
> +	struct amdgpu_mux_entry *e;
> +
> +	e = amdgpu_get_sw_entry(mux, ring);
> +	if (!e) {
> +		DRM_ERROR("cannot find entry for sw ring\n");
> +		return 0;
> +	}
> +
> +	return e->sw_wptr;
> +}
> +
> +u64 amdgpu_ring_get_rptr_from_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring)
> +{
> +	struct amdgpu_mux_entry *e;
> +	u64 r_rptr, offset, start, end;
> +
> +	e = amdgpu_get_sw_entry(mux, ring);
> +	if (!e) {
> +		DRM_ERROR("no sw entry found!\n");
> +		return 0;
> +	}
> +
> +	r_rptr = amdgpu_ring_get_rptr(mux->real_ring);
> +
> +	start = e->start_ptr_in_hw_ring & mux->real_ring->buf_mask;
> +	end = e->end_ptr_in_hw_ring & mux->real_ring->buf_mask;
> +	if (start > end) {
> +		if (r_rptr <= end)
> +			r_rptr += mux->real_ring->ring_size >> 2;
> +		end += mux->real_ring->ring_size >> 2;
> +	}
> +
> +	if (r_rptr <= end && r_rptr >= start) {
> +		offset = r_rptr - start;
> +		e->sw_rptr = (e->sw_cptr + offset) & ring->buf_mask;
> +	} else if (r_rptr < start) {
> +		e->sw_rptr = e->sw_cptr;
> +	} else {
> +		e->sw_rptr = e->sw_wptr;
> +	}
> +
> +	return e->sw_rptr;
> +}
> +
> +/*copy packages on sw ring range[begin, end) */
> +static int copy_pkt_from_sw_ring(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring,

Drop the return value here. There is no error handling which would make 
sense here.

> +	u64 s_begin, u64 s_end)
> +{
> +	u64 begin, end, r_begin, r_end;
> +	struct amdgpu_ring *real_ring = mux->real_ring;
> +
> +	begin = s_begin & ring->buf_mask;
> +	end = s_end & ring->buf_mask;
> +
> +	r_begin = real_ring->wptr & real_ring->buf_mask;
> +	if (begin == end)
> +		return -ERANGE;

So this could either mean we should copy nothing or everything. I think 
the best approach would be to print an error into the sytem log if that 
every happens.

> +	if (begin > end) {
> +		amdgpu_ring_alloc(real_ring, (ring->ring_size >> 2) + end - begin);
> +		amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[begin],
> +			(ring->ring_size >> 2) - begin);
> +		amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[0], end);
> +	} else {
> +		amdgpu_ring_alloc(real_ring, end - begin);
> +		amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[begin], end - begin);
> +	}
> +
> +	r_end = real_ring->wptr & real_ring->buf_mask;
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
> new file mode 100644
> index 000000000000..d058c43bb063
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
> @@ -0,0 +1,67 @@
> +/*
> + * Copyright 2022 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#ifndef __AMDGPU_RING_MUX__
> +#define __AMDGPU_RING_MUX__
> +
> +#include <linux/timer.h>
> +#include <linux/spinlock.h>
> +#include "amdgpu_ring.h"
> +
> +struct amdgpu_ring;
> +/*
> + * start_ptr_in_hw_ring - last copied start loc on hw ring
> + * end_ptr_in_hw_ring - last copied end loc on hw ring
> + *sw_cptr -the begin of copy ptr in sw ring
> + *sw_rptr; the read ptr in sw ring
> + *sw_wptr; the write ptr in sw ring
> + */
> +struct amdgpu_mux_entry {
> +	struct amdgpu_ring	*ring;
> +	u64 start_ptr_in_hw_ring;
> +	u64 end_ptr_in_hw_ring;
> +
> +	u64 sw_cptr;
> +	u64 sw_rptr;
> +	u64 sw_wptr;
> +};
> +
> +struct amdgpu_ring_mux {
> +	struct amdgpu_ring *real_ring;
> +
> +	struct amdgpu_mux_entry ring_entries[AMDGPU_MAX_GFX_RINGS];
> +
> +	unsigned num_ring_entries;
> +
> +	spinlock_t			lock;
> +
> +};
> +
> +int amdgpu_ring_mux_init(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring);
> +void amdgpu_ring_mux_fini(struct amdgpu_ring_mux *mux);
> +int amdgpu_ring_mux_add_sw_ring(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring);
> +void amdgpu_ring_set_wptr_to_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring, u64 wptr);
> +u64 amdgpu_ring_get_wptr_from_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring);
> +u64 amdgpu_ring_get_rptr_from_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring);
> +
> +#endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
> new file mode 100644
> index 000000000000..7a59e3fbb970
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
> @@ -0,0 +1,60 @@
> +/*
> + * Copyright 2022 Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sub license, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + */
> +
> +#include "amdgpu_sw_ring.h"
> +#include "amdgpu_ring_mux.h"
> +
> +u64 amdgpu_sw_ring_get_rptr_gfx(struct amdgpu_ring *ring)
> +{
> +	struct amdgpu_device *adev = ring->adev;
> +	struct amdgpu_ring_mux *mux = &adev->gfx.muxer;
> +
> +	BUG_ON(!ring->is_sw_ring);

Never ever add a BUG_ON() if you don't prevent further corruption or 
make an imminent crash more obvious to track down with it.

This here should be a WARN_ON instead.

> +	return amdgpu_ring_get_rptr_from_mux(mux, ring);
> +}
> +
> +u64 amdgpu_sw_ring_get_wptr_gfx(struct amdgpu_ring *ring)
> +{
> +	struct amdgpu_device *adev = ring->adev;
> +	struct amdgpu_ring_mux *mux = &adev->gfx.muxer;
> +
> +	BUG_ON(!ring->is_sw_ring);
> +	return amdgpu_ring_get_wptr_from_mux(mux, ring);
> +}
> +
> +void amdgpu_sw_ring_set_wptr_gfx(struct amdgpu_ring *ring)
> +{
> +	BUG_ON(!ring->is_sw_ring);
> +}
> +
> +void amdgpu_sw_ring_commit(struct amdgpu_ring *ring)
> +{
> +	struct amdgpu_device *adev = ring->adev;
> +	struct amdgpu_ring_mux *mux = &adev->gfx.muxer;
> +
> +	BUG_ON(!ring->is_sw_ring);
> +	amdgpu_ring_set_wptr_to_mux(mux, ring, ring->wptr);
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h
> new file mode 100644
> index 000000000000..0a2e66318f3f
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h
> @@ -0,0 +1,43 @@
> +/*
> + * Copyright 2012 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#include <drm/amdgpu_drm.h>
> +#include <drm/gpu_scheduler.h>
> +#include <drm/drm_print.h>
> +
> +#include "amdgpu_irq.h"
> +#include "amdgpu_ring.h"
> +#include "amdgpu.h"
> +
> +#ifndef __AMDGPU_SWRING_H__
> +#define __AMDGPU_SWRING_H__
> +
> +u64 amdgpu_sw_ring_get_rptr_gfx(struct amdgpu_ring *ring);
> +u64 amdgpu_sw_ring_get_wptr_gfx(struct amdgpu_ring *ring);
> +void amdgpu_sw_ring_set_wptr_gfx(struct amdgpu_ring *ring);
> +void amdgpu_sw_ring_commit(struct amdgpu_ring *ring);
> +
> +void amdgpu_sw_ring_ib_begin(struct amdgpu_ring *ring);
> +void amdgpu_sw_ring_ib_end(struct amdgpu_ring *ring);
> +
> +#endif

Re: [PATCH 1/4] drm/amdgpu: Introduce gfx software ring(v4)

[Luben Tuikov]

It's customary to add a Cc: tag in the commit message on subsequent revisions
of a patch, once a person has reviewed and commented on it--Christian, Andrey, me,
so that subsequent patches' emails go out to those people automatically via a CC.

Inlined:

On 2022-09-13 05:05, jiadong.zhu@amd.com wrote:
> From: "Jiadong.Zhu" <Jiadong.Zhu@amd.com>
> 
> The software ring is created to support priority
> context while there is only one hardware queue
> for gfx.
> 
> Every software rings has its fence driver and could

"ring", singular.

> be used as an ordinary ring for the gpu_scheduler.

"GPU scheduler".

> Multiple software rings are binded to a real ring

The past tense of "bind" is "bound", not "binded".

> with the ring muxer. The packages committed on the
> software ring are copied to the real ring.

Use 79 column wrap setting in your editor, not 50 or 60.
Wrap at 79.

> 
> v2: use array to store software ring entry.
> v3: remove unnecessary prints.
> v4: remove amdgpu_ring_sw_init/fini functions,
> using gtt for sw ring buffer for later dma copy
> optimization.
> 
> Signed-off-by: Jiadong.Zhu <Jiadong.Zhu@amd.com>

Add Cc: tags before the Signed-off-by line.

> ---
>  drivers/gpu/drm/amd/amdgpu/Makefile          |   3 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h      |   3 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h     |   3 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c | 182 +++++++++++++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h |  67 +++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c  |  60 ++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h  |  43 +++++
>  7 files changed, 360 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
>  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
>  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
>  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
> index 3e0e2eb7e235..85224bc81ce5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -58,7 +58,8 @@ amdgpu-y += amdgpu_device.o amdgpu_kms.o \
>  	amdgpu_vm_sdma.o amdgpu_discovery.o amdgpu_ras_eeprom.o amdgpu_nbio.o \
>  	amdgpu_umc.o smu_v11_0_i2c.o amdgpu_fru_eeprom.o amdgpu_rap.o \
>  	amdgpu_fw_attestation.o amdgpu_securedisplay.o \
> -	amdgpu_eeprom.o amdgpu_mca.o amdgpu_psp_ta.o amdgpu_lsdma.o
> +	amdgpu_eeprom.o amdgpu_mca.o amdgpu_psp_ta.o amdgpu_lsdma.o \
> +	amdgpu_sw_ring.o amdgpu_ring_mux.o
>  
>  amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> index 53526ffb2ce1..0de8e3cd0f1c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> @@ -33,6 +33,7 @@
>  #include "amdgpu_imu.h"
>  #include "soc15.h"
>  #include "amdgpu_ras.h"
> +#include "amdgpu_ring_mux.h"
>  
>  /* GFX current status */
>  #define AMDGPU_GFX_NORMAL_MODE			0x00000000L
> @@ -346,6 +347,8 @@ struct amdgpu_gfx {
>  	struct amdgpu_gfx_ras		*ras;
>  
>  	bool				is_poweron;
> +
> +	struct amdgpu_ring_mux			muxer;

It doesn't align, possibly because TAB chars were used between "bool" and "is_poweron",
and because TAB chars were used between "struct amdgpu_ring_mux" and "muxer".

>  };
>  
>  #define amdgpu_gfx_get_gpu_clock_counter(adev) (adev)->gfx.funcs->get_gpu_clock_counter((adev))
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index 7d89a52091c0..fe33a683bfba 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -278,6 +278,9 @@ struct amdgpu_ring {
>  	bool			is_mes_queue;
>  	uint32_t		hw_queue_id;
>  	struct amdgpu_mes_ctx_data *mes_ctx;
> +
> +	bool			is_sw_ring;

Use spaces to align "is_sw_ring", not TAB chars, as I mentioned in my review of v3 of this patch.

> +
>  };
>  
>  #define amdgpu_ring_parse_cs(r, p, job, ib) ((r)->funcs->parse_cs((p), (job), (ib)))
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
> new file mode 100644
> index 000000000000..652a6d3e0ec3
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
> @@ -0,0 +1,182 @@
> +/*
> + * Copyright 2022 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#include <drm/drm_print.h>
> +
> +#include "amdgpu_ring_mux.h"
> +#include "amdgpu_ring.h"
> +
> +#define AMDGPU_MUX_RESUBMIT_JIFFIES_TIMEOUT (HZ/2)
> +
> +static int copy_pkt_from_sw_ring(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring,
> +	u64 s_begin, u64 s_end);
> +
> +int amdgpu_ring_mux_init(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring)
> +{
> +	mux->real_ring = ring;
> +	memset(mux->ring_entries, 0, sizeof(mux->ring_entries));
> +	mux->num_ring_entries = 0;
> +	spin_lock_init(&mux->lock);
> +	return 0;

Get rid of the "return 0;" here and make this function "void".

> +}
> +
> +void amdgpu_ring_mux_fini(struct amdgpu_ring_mux *mux)
> +{
> +	memset(mux->ring_entries, 0, sizeof(mux->ring_entries));
> +	mux->num_ring_entries = 0;
> +}
> +
> +int amdgpu_ring_mux_add_sw_ring(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring)
> +{
> +	struct amdgpu_mux_entry *e;
> +
> +	if (mux->num_ring_entries == AMDGPU_MAX_GFX_RINGS) {
> +		DRM_ERROR("adding sw ring exceeds max gfx num\n");
> +		return -ENOMEM;

Copied from my v3 review:

You can't return here -ENOMEM, as it is not a real out of memory condition.
Maybe EINVAL or something like that, but not ENOMEM.

Also, under what circumstances would we get to this condition here?
Are such circumstances valid?

And if so, then when this is returned, what happens?
Does the driver die?

I feel we shouldn't ever have this here--it should've been
calculated correctly to never have fallen in this/such a circumstance like that here.

v4: Perhaps make it dynamic? So that it works for any ASIC in the future
without the need to modify AMDGPU_MAX_GFX_RINGS, or modify the driver.

> +	}
> +
> +	e = &mux->ring_entries[mux->num_ring_entries++];

This seems hackish here because you're not saving the increment
of mux->num_ring_entries. Also rename the array to "ring_entry",
so that "ring_entry[i]" means "ring entry at index i".

Perhaps keep a "ring_entry_size" member variable in "mux" to hold the size of "ring_entry[]", and
use "num_ring_entries" to hold the dynamic (populated) size of "ring_entry[]",
so that the _next_ available index is:

if (mux->num_ring_entries >= mux->ring_entry_size) {
    Complain;
    return -ENOENT;
}

e = &mux->ring_entry[mux->num_ring_entries];
mux->num_ring_entries += 1;

e->ring = ...

> +
> +	e->ring = ring;
> +	e->start_ptr_in_hw_ring = 0;
> +	e->end_ptr_in_hw_ring = 0;
> +	e->sw_cptr = 0;
> +	e->sw_rptr = 0;
> +	e->sw_wptr = 0;

If struct amdgpu_gfx is kzalloc-ed, then this is all zeroed here anyway. Something to think about.

> +
> +	return 0;
> +}
> +
> +static struct amdgpu_mux_entry *amdgpu_get_sw_entry(struct amdgpu_ring_mux *mux,
> +				struct amdgpu_ring *ring)
> +{
> +	struct amdgpu_mux_entry *e;
> +	int i;
> +
> +	e = NULL;
> +	for (i = 0; i < mux->num_ring_entries; i++) {
> +		if (mux->ring_entries[i].ring == ring) {
> +			e = &mux->ring_entries[i];
> +			break;
> +		}
> +	}
> +
> +	return e;
> +}

This is a bit naive, as every time we want to do an operation on a mapped ring,
we do an O(n) search, as shown in the three functions below.

Perhaps add an index variable to struct amdgpu_ring to tell you this mapping,
in O(1) time complexity, and eliminate this function.

> +
> +void amdgpu_ring_set_wptr_to_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring, u64 wptr)
> +{
> +	struct amdgpu_mux_entry *e;
> +
> +	e = amdgpu_get_sw_entry(mux, ring);
> +	if (!e) {
> +		DRM_ERROR("cannot find entry for sw ring\n");
> +		return;
> +	}
> +
> +	spin_lock(&mux->lock);
> +	e->sw_cptr = e->sw_wptr;
> +	e->sw_wptr = wptr;
> +	e->start_ptr_in_hw_ring = mux->real_ring->wptr;
> +
> +	if (copy_pkt_from_sw_ring(mux, ring, e->sw_cptr, wptr) == 0) {
> +		e->end_ptr_in_hw_ring = mux->real_ring->wptr;
> +		amdgpu_ring_commit(mux->real_ring);
> +	}
> +
> +	spin_unlock(&mux->lock);
> +}
> +
> +u64 amdgpu_ring_get_wptr_from_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring)
> +{
> +	struct amdgpu_mux_entry *e;
> +
> +	e = amdgpu_get_sw_entry(mux, ring);
> +	if (!e) {
> +		DRM_ERROR("cannot find entry for sw ring\n");
> +		return 0;
> +	}
> +
> +	return e->sw_wptr;
> +}
> +
> +u64 amdgpu_ring_get_rptr_from_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring)

So, between the function names of "amdgpu_ring_get_rptr_from_mux()" and "amdgpu_ring_get_wptr_from_mux()",
they are 96.6(6)% _the_same_ name to a human. How about "amdgpu_ring_get_readp_from_mux()" and
"amdgpu_ring_get_writep_from_mux()"?

> +{
> +	struct amdgpu_mux_entry *e;
> +	u64 r_rptr, offset, start, end;

"r_rptr" is not a very descriptive name. How about "readp"? It is more descriptive to a human.
They are all "ptrs", but what is more descriptive is whether it is a "read" or "write", etc.,
so "readp" is more descriptive.

> +
> +	e = amdgpu_get_sw_entry(mux, ring);
> +	if (!e) {
> +		DRM_ERROR("no sw entry found!\n");
> +		return 0;
> +	}
> +
> +	r_rptr = amdgpu_ring_get_rptr(mux->real_ring);
> +
> +	start = e->start_ptr_in_hw_ring & mux->real_ring->buf_mask;
> +	end = e->end_ptr_in_hw_ring & mux->real_ring->buf_mask;
> +	if (start > end) {
> +		if (r_rptr <= end)
> +			r_rptr += mux->real_ring->ring_size >> 2;
> +		end += mux->real_ring->ring_size >> 2;
> +	}
> +
> +	if (r_rptr <= end && r_rptr >= start) {
> +		offset = r_rptr - start;
> +		e->sw_rptr = (e->sw_cptr + offset) & ring->buf_mask;
> +	} else if (r_rptr < start) {
> +		e->sw_rptr = e->sw_cptr;
> +	} else {
> +		e->sw_rptr = e->sw_wptr;
> +	}
> +
> +	return e->sw_rptr;
> +}
> +
> +/*copy packages on sw ring range[begin, end) */
> +static int copy_pkt_from_sw_ring(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring,
> +	u64 s_begin, u64 s_end)
> +{
> +	u64 begin, end, r_begin, r_end;
> +	struct amdgpu_ring *real_ring = mux->real_ring;
> +
> +	begin = s_begin & ring->buf_mask;
> +	end = s_end & ring->buf_mask;
> +
> +	r_begin = real_ring->wptr & real_ring->buf_mask;
> +	if (begin == end)
> +		return -ERANGE;
> +	if (begin > end) {
> +		amdgpu_ring_alloc(real_ring, (ring->ring_size >> 2) + end - begin);
> +		amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[begin],
> +			(ring->ring_size >> 2) - begin);
> +		amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[0], end);
> +	} else {
> +		amdgpu_ring_alloc(real_ring, end - begin);
> +		amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[begin], end - begin);
> +	}
> +
> +	r_end = real_ring->wptr & real_ring->buf_mask;
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
> new file mode 100644
> index 000000000000..d058c43bb063
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
> @@ -0,0 +1,67 @@
> +/*
> + * Copyright 2022 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#ifndef __AMDGPU_RING_MUX__
> +#define __AMDGPU_RING_MUX__
> +
> +#include <linux/timer.h>
> +#include <linux/spinlock.h>
> +#include "amdgpu_ring.h"
> +
> +struct amdgpu_ring;
> +/*
> + * start_ptr_in_hw_ring - last copied start loc on hw ring
> + * end_ptr_in_hw_ring - last copied end loc on hw ring
> + *sw_cptr -the begin of copy ptr in sw ring
> + *sw_rptr; the read ptr in sw ring
> + *sw_wptr; the write ptr in sw ring
> + */
> +struct amdgpu_mux_entry {
> +	struct amdgpu_ring	*ring;

Don't use TAB chars to align--use space characters.

> +	u64 start_ptr_in_hw_ring;
> +	u64 end_ptr_in_hw_ring;
> +
> +	u64 sw_cptr;
> +	u64 sw_rptr;
> +	u64 sw_wptr;

Align all members to "*ring".

> +};
> +
> +struct amdgpu_ring_mux {
> +	struct amdgpu_ring *real_ring;
> +
> +	struct amdgpu_mux_entry ring_entries[AMDGPU_MAX_GFX_RINGS];
> +
> +	unsigned num_ring_entries;
> +
> +	spinlock_t			lock;

Don't use TAB char to align. Also note that TAB char in the kernel is 8 chars long, not 2, 3 or 4.

Regards,
Luben

> +
> +};
> +
> +int amdgpu_ring_mux_init(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring);
> +void amdgpu_ring_mux_fini(struct amdgpu_ring_mux *mux);
> +int amdgpu_ring_mux_add_sw_ring(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring);
> +void amdgpu_ring_set_wptr_to_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring, u64 wptr);
> +u64 amdgpu_ring_get_wptr_from_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring);
> +u64 amdgpu_ring_get_rptr_from_mux(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring);
> +
> +#endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
> new file mode 100644
> index 000000000000..7a59e3fbb970
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
> @@ -0,0 +1,60 @@
> +/*
> + * Copyright 2022 Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sub license, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + */
> +
> +#include "amdgpu_sw_ring.h"
> +#include "amdgpu_ring_mux.h"
> +
> +u64 amdgpu_sw_ring_get_rptr_gfx(struct amdgpu_ring *ring)
> +{
> +	struct amdgpu_device *adev = ring->adev;
> +	struct amdgpu_ring_mux *mux = &adev->gfx.muxer;
> +
> +	BUG_ON(!ring->is_sw_ring);
> +	return amdgpu_ring_get_rptr_from_mux(mux, ring);
> +}
> +
> +u64 amdgpu_sw_ring_get_wptr_gfx(struct amdgpu_ring *ring)
> +{
> +	struct amdgpu_device *adev = ring->adev;
> +	struct amdgpu_ring_mux *mux = &adev->gfx.muxer;
> +
> +	BUG_ON(!ring->is_sw_ring);
> +	return amdgpu_ring_get_wptr_from_mux(mux, ring);
> +}
> +
> +void amdgpu_sw_ring_set_wptr_gfx(struct amdgpu_ring *ring)
> +{
> +	BUG_ON(!ring->is_sw_ring);
> +}
> +
> +void amdgpu_sw_ring_commit(struct amdgpu_ring *ring)
> +{
> +	struct amdgpu_device *adev = ring->adev;
> +	struct amdgpu_ring_mux *mux = &adev->gfx.muxer;
> +
> +	BUG_ON(!ring->is_sw_ring);
> +	amdgpu_ring_set_wptr_to_mux(mux, ring, ring->wptr);
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h
> new file mode 100644
> index 000000000000..0a2e66318f3f
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h
> @@ -0,0 +1,43 @@
> +/*
> + * Copyright 2012 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#include <drm/amdgpu_drm.h>
> +#include <drm/gpu_scheduler.h>
> +#include <drm/drm_print.h>
> +
> +#include "amdgpu_irq.h"
> +#include "amdgpu_ring.h"
> +#include "amdgpu.h"
> +
> +#ifndef __AMDGPU_SWRING_H__
> +#define __AMDGPU_SWRING_H__
> +
> +u64 amdgpu_sw_ring_get_rptr_gfx(struct amdgpu_ring *ring);
> +u64 amdgpu_sw_ring_get_wptr_gfx(struct amdgpu_ring *ring);
> +void amdgpu_sw_ring_set_wptr_gfx(struct amdgpu_ring *ring);
> +void amdgpu_sw_ring_commit(struct amdgpu_ring *ring);
> +
> +void amdgpu_sw_ring_ib_begin(struct amdgpu_ring *ring);
> +void amdgpu_sw_ring_ib_end(struct amdgpu_ring *ring);
> +
> +#endif

RE: [PATCH 1/4] drm/amdgpu: Introduce gfx software ring(v4)

[Zhu, Jiadong]

[AMD Official Use Only - General]

>So, between the function names of "amdgpu_ring_get_rptr_from_mux()" and "amdgpu_ring_get_wptr_from_mux()",
>they are 96.6(6)% _the_same_ name to a human. How about "amdgpu_ring_get_readp_from_mux()" and "amdgpu_ring_get_writep_from_mux()"?

I agree with the similarity between rptr and wptr. But the function named amdgpu_ring_get_rptr_from_mux  is aligned with amdgpu_ring_get_rptr for the same style, easier to understand.
I would rather use it currently until we replace rptr with writep in all the other places, shall we?

Thanks,
Jiadong

-----Original Message-----
From: Tuikov, Luben <Luben.Tuikov@amd.com>
Sent: Thursday, September 15, 2022 12:49 AM
To: Zhu, Jiadong <Jiadong.Zhu@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Huang, Ray <Ray.Huang@amd.com>; Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
Subject: Re: [PATCH 1/4] drm/amdgpu: Introduce gfx software ring(v4)

It's customary to add a Cc: tag in the commit message on subsequent revisions of a patch, once a person has reviewed and commented on it--Christian, Andrey, me, so that subsequent patches' emails go out to those people automatically via a CC.

Inlined:

On 2022-09-13 05:05, jiadong.zhu@amd.com wrote:
> From: "Jiadong.Zhu" <Jiadong.Zhu@amd.com>
>
> The software ring is created to support priority context while there
> is only one hardware queue for gfx.
>
> Every software rings has its fence driver and could

"ring", singular.

> be used as an ordinary ring for the gpu_scheduler.

"GPU scheduler".

> Multiple software rings are binded to a real ring

The past tense of "bind" is "bound", not "binded".

> with the ring muxer. The packages committed on the software ring are
> copied to the real ring.

Use 79 column wrap setting in your editor, not 50 or 60.
Wrap at 79.

>
> v2: use array to store software ring entry.
> v3: remove unnecessary prints.
> v4: remove amdgpu_ring_sw_init/fini functions, using gtt for sw ring
> buffer for later dma copy optimization.
>
> Signed-off-by: Jiadong.Zhu <Jiadong.Zhu@amd.com>

Add Cc: tags before the Signed-off-by line.

> ---
>  drivers/gpu/drm/amd/amdgpu/Makefile          |   3 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h      |   3 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h     |   3 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c | 182
> +++++++++++++++++++  drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h |
> 67 +++++++  drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c  |  60 ++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h  |  43 +++++
>  7 files changed, 360 insertions(+), 1 deletion(-)  create mode 100644
> drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
>  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
>  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
>  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile
> b/drivers/gpu/drm/amd/amdgpu/Makefile
> index 3e0e2eb7e235..85224bc81ce5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -58,7 +58,8 @@ amdgpu-y += amdgpu_device.o amdgpu_kms.o \
>       amdgpu_vm_sdma.o amdgpu_discovery.o amdgpu_ras_eeprom.o amdgpu_nbio.o \
>       amdgpu_umc.o smu_v11_0_i2c.o amdgpu_fru_eeprom.o amdgpu_rap.o \
>       amdgpu_fw_attestation.o amdgpu_securedisplay.o \
> -     amdgpu_eeprom.o amdgpu_mca.o amdgpu_psp_ta.o amdgpu_lsdma.o
> +     amdgpu_eeprom.o amdgpu_mca.o amdgpu_psp_ta.o amdgpu_lsdma.o \
> +     amdgpu_sw_ring.o amdgpu_ring_mux.o
>
>  amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> index 53526ffb2ce1..0de8e3cd0f1c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> @@ -33,6 +33,7 @@
>  #include "amdgpu_imu.h"
>  #include "soc15.h"
>  #include "amdgpu_ras.h"
> +#include "amdgpu_ring_mux.h"
>
>  /* GFX current status */
>  #define AMDGPU_GFX_NORMAL_MODE                       0x00000000L
> @@ -346,6 +347,8 @@ struct amdgpu_gfx {
>       struct amdgpu_gfx_ras           *ras;
>
>       bool                            is_poweron;
> +
> +     struct amdgpu_ring_mux                  muxer;

It doesn't align, possibly because TAB chars were used between "bool" and "is_poweron", and because TAB chars were used between "struct amdgpu_ring_mux" and "muxer".

>  };
>
>  #define amdgpu_gfx_get_gpu_clock_counter(adev)
> (adev)->gfx.funcs->get_gpu_clock_counter((adev))
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index 7d89a52091c0..fe33a683bfba 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -278,6 +278,9 @@ struct amdgpu_ring {
>       bool                    is_mes_queue;
>       uint32_t                hw_queue_id;
>       struct amdgpu_mes_ctx_data *mes_ctx;
> +
> +     bool                    is_sw_ring;

Use spaces to align "is_sw_ring", not TAB chars, as I mentioned in my review of v3 of this patch.

> +
>  };
>
>  #define amdgpu_ring_parse_cs(r, p, job, ib)
> ((r)->funcs->parse_cs((p), (job), (ib))) diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
> new file mode 100644
> index 000000000000..652a6d3e0ec3
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
> @@ -0,0 +1,182 @@
> +/*
> + * Copyright 2022 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person
> +obtaining a
> + * copy of this software and associated documentation files (the
> +"Software"),
> + * to deal in the Software without restriction, including without
> +limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> +sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> +the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> +included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> +EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> +MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
> +SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
> +DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> +OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
> +OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#include <drm/drm_print.h>
> +
> +#include "amdgpu_ring_mux.h"
> +#include "amdgpu_ring.h"
> +
> +#define AMDGPU_MUX_RESUBMIT_JIFFIES_TIMEOUT (HZ/2)
> +
> +static int copy_pkt_from_sw_ring(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring,
> +     u64 s_begin, u64 s_end);
> +
> +int amdgpu_ring_mux_init(struct amdgpu_ring_mux *mux, struct
> +amdgpu_ring *ring) {
> +     mux->real_ring = ring;
> +     memset(mux->ring_entries, 0, sizeof(mux->ring_entries));
> +     mux->num_ring_entries = 0;
> +     spin_lock_init(&mux->lock);
> +     return 0;

Get rid of the "return 0;" here and make this function "void".

> +}
> +
> +void amdgpu_ring_mux_fini(struct amdgpu_ring_mux *mux) {
> +     memset(mux->ring_entries, 0, sizeof(mux->ring_entries));
> +     mux->num_ring_entries = 0;
> +}
> +
> +int amdgpu_ring_mux_add_sw_ring(struct amdgpu_ring_mux *mux, struct
> +amdgpu_ring *ring) {
> +     struct amdgpu_mux_entry *e;
> +
> +     if (mux->num_ring_entries == AMDGPU_MAX_GFX_RINGS) {
> +             DRM_ERROR("adding sw ring exceeds max gfx num\n");
> +             return -ENOMEM;

Copied from my v3 review:

You can't return here -ENOMEM, as it is not a real out of memory condition.
Maybe EINVAL or something like that, but not ENOMEM.

Also, under what circumstances would we get to this condition here?
Are such circumstances valid?

And if so, then when this is returned, what happens?
Does the driver die?

I feel we shouldn't ever have this here--it should've been calculated correctly to never have fallen in this/such a circumstance like that here.

v4: Perhaps make it dynamic? So that it works for any ASIC in the future without the need to modify AMDGPU_MAX_GFX_RINGS, or modify the driver.

> +     }
> +
> +     e = &mux->ring_entries[mux->num_ring_entries++];

This seems hackish here because you're not saving the increment of mux->num_ring_entries. Also rename the array to "ring_entry", so that "ring_entry[i]" means "ring entry at index i".

Perhaps keep a "ring_entry_size" member variable in "mux" to hold the size of "ring_entry[]", and use "num_ring_entries" to hold the dynamic (populated) size of "ring_entry[]", so that the _next_ available index is:

if (mux->num_ring_entries >= mux->ring_entry_size) {
    Complain;
    return -ENOENT;
}

e = &mux->ring_entry[mux->num_ring_entries];
mux->num_ring_entries += 1;

e->ring = ...

> +
> +     e->ring = ring;
> +     e->start_ptr_in_hw_ring = 0;
> +     e->end_ptr_in_hw_ring = 0;
> +     e->sw_cptr = 0;
> +     e->sw_rptr = 0;
> +     e->sw_wptr = 0;

If struct amdgpu_gfx is kzalloc-ed, then this is all zeroed here anyway. Something to think about.

> +
> +     return 0;
> +}
> +
> +static struct amdgpu_mux_entry *amdgpu_get_sw_entry(struct amdgpu_ring_mux *mux,
> +                             struct amdgpu_ring *ring)
> +{
> +     struct amdgpu_mux_entry *e;
> +     int i;
> +
> +     e = NULL;
> +     for (i = 0; i < mux->num_ring_entries; i++) {
> +             if (mux->ring_entries[i].ring == ring) {
> +                     e = &mux->ring_entries[i];
> +                     break;
> +             }
> +     }
> +
> +     return e;
> +}

This is a bit naive, as every time we want to do an operation on a mapped ring, we do an O(n) search, as shown in the three functions below.

Perhaps add an index variable to struct amdgpu_ring to tell you this mapping, in O(1) time complexity, and eliminate this function.

> +
> +void amdgpu_ring_set_wptr_to_mux(struct amdgpu_ring_mux *mux, struct
> +amdgpu_ring *ring, u64 wptr) {
> +     struct amdgpu_mux_entry *e;
> +
> +     e = amdgpu_get_sw_entry(mux, ring);
> +     if (!e) {
> +             DRM_ERROR("cannot find entry for sw ring\n");
> +             return;
> +     }
> +
> +     spin_lock(&mux->lock);
> +     e->sw_cptr = e->sw_wptr;
> +     e->sw_wptr = wptr;
> +     e->start_ptr_in_hw_ring = mux->real_ring->wptr;
> +
> +     if (copy_pkt_from_sw_ring(mux, ring, e->sw_cptr, wptr) == 0) {
> +             e->end_ptr_in_hw_ring = mux->real_ring->wptr;
> +             amdgpu_ring_commit(mux->real_ring);
> +     }
> +
> +     spin_unlock(&mux->lock);
> +}
> +
> +u64 amdgpu_ring_get_wptr_from_mux(struct amdgpu_ring_mux *mux, struct
> +amdgpu_ring *ring) {
> +     struct amdgpu_mux_entry *e;
> +
> +     e = amdgpu_get_sw_entry(mux, ring);
> +     if (!e) {
> +             DRM_ERROR("cannot find entry for sw ring\n");
> +             return 0;
> +     }
> +
> +     return e->sw_wptr;
> +}
> +
> +u64 amdgpu_ring_get_rptr_from_mux(struct amdgpu_ring_mux *mux, struct
> +amdgpu_ring *ring)

So, between the function names of "amdgpu_ring_get_rptr_from_mux()" and "amdgpu_ring_get_wptr_from_mux()",
they are 96.6(6)% _the_same_ name to a human. How about "amdgpu_ring_get_readp_from_mux()" and "amdgpu_ring_get_writep_from_mux()"?

> +{
> +     struct amdgpu_mux_entry *e;
> +     u64 r_rptr, offset, start, end;

"r_rptr" is not a very descriptive name. How about "readp"? It is more descriptive to a human.
They are all "ptrs", but what is more descriptive is whether it is a "read" or "write", etc., so "readp" is more descriptive.

> +
> +     e = amdgpu_get_sw_entry(mux, ring);
> +     if (!e) {
> +             DRM_ERROR("no sw entry found!\n");
> +             return 0;
> +     }
> +
> +     r_rptr = amdgpu_ring_get_rptr(mux->real_ring);
> +
> +     start = e->start_ptr_in_hw_ring & mux->real_ring->buf_mask;
> +     end = e->end_ptr_in_hw_ring & mux->real_ring->buf_mask;
> +     if (start > end) {
> +             if (r_rptr <= end)
> +                     r_rptr += mux->real_ring->ring_size >> 2;
> +             end += mux->real_ring->ring_size >> 2;
> +     }
> +
> +     if (r_rptr <= end && r_rptr >= start) {
> +             offset = r_rptr - start;
> +             e->sw_rptr = (e->sw_cptr + offset) & ring->buf_mask;
> +     } else if (r_rptr < start) {
> +             e->sw_rptr = e->sw_cptr;
> +     } else {
> +             e->sw_rptr = e->sw_wptr;
> +     }
> +
> +     return e->sw_rptr;
> +}
> +
> +/*copy packages on sw ring range[begin, end) */ static int
> +copy_pkt_from_sw_ring(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring,
> +     u64 s_begin, u64 s_end)
> +{
> +     u64 begin, end, r_begin, r_end;
> +     struct amdgpu_ring *real_ring = mux->real_ring;
> +
> +     begin = s_begin & ring->buf_mask;
> +     end = s_end & ring->buf_mask;
> +
> +     r_begin = real_ring->wptr & real_ring->buf_mask;
> +     if (begin == end)
> +             return -ERANGE;
> +     if (begin > end) {
> +             amdgpu_ring_alloc(real_ring, (ring->ring_size >> 2) + end - begin);
> +             amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[begin],
> +                     (ring->ring_size >> 2) - begin);
> +             amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[0], end);
> +     } else {
> +             amdgpu_ring_alloc(real_ring, end - begin);
> +             amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[begin], end - begin);
> +     }
> +
> +     r_end = real_ring->wptr & real_ring->buf_mask;
> +
> +     return 0;
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
> new file mode 100644
> index 000000000000..d058c43bb063
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
> @@ -0,0 +1,67 @@
> +/*
> + * Copyright 2022 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person
> +obtaining a
> + * copy of this software and associated documentation files (the
> +"Software"),
> + * to deal in the Software without restriction, including without
> +limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> +sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> +the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> +included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> +EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> +MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
> +SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
> +DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> +OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
> +OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#ifndef __AMDGPU_RING_MUX__
> +#define __AMDGPU_RING_MUX__
> +
> +#include <linux/timer.h>
> +#include <linux/spinlock.h>
> +#include "amdgpu_ring.h"
> +
> +struct amdgpu_ring;
> +/*
> + * start_ptr_in_hw_ring - last copied start loc on hw ring
> + * end_ptr_in_hw_ring - last copied end loc on hw ring  *sw_cptr -the
> +begin of copy ptr in sw ring  *sw_rptr; the read ptr in sw ring
> +*sw_wptr; the write ptr in sw ring  */ struct amdgpu_mux_entry {
> +     struct amdgpu_ring      *ring;

Don't use TAB chars to align--use space characters.

> +     u64 start_ptr_in_hw_ring;
> +     u64 end_ptr_in_hw_ring;
> +
> +     u64 sw_cptr;
> +     u64 sw_rptr;
> +     u64 sw_wptr;

Align all members to "*ring".

> +};
> +
> +struct amdgpu_ring_mux {
> +     struct amdgpu_ring *real_ring;
> +
> +     struct amdgpu_mux_entry ring_entries[AMDGPU_MAX_GFX_RINGS];
> +
> +     unsigned num_ring_entries;
> +
> +     spinlock_t                      lock;

Don't use TAB char to align. Also note that TAB char in the kernel is 8 chars long, not 2, 3 or 4.

Regards,
Luben

> +
> +};
> +
> +int amdgpu_ring_mux_init(struct amdgpu_ring_mux *mux, struct
> +amdgpu_ring *ring); void amdgpu_ring_mux_fini(struct amdgpu_ring_mux
> +*mux); int amdgpu_ring_mux_add_sw_ring(struct amdgpu_ring_mux *mux,
> +struct amdgpu_ring *ring); void amdgpu_ring_set_wptr_to_mux(struct
> +amdgpu_ring_mux *mux, struct amdgpu_ring *ring, u64 wptr);
> +u64 amdgpu_ring_get_wptr_from_mux(struct amdgpu_ring_mux *mux, struct
> +amdgpu_ring *ring);
> +u64 amdgpu_ring_get_rptr_from_mux(struct amdgpu_ring_mux *mux, struct
> +amdgpu_ring *ring);
> +
> +#endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
> new file mode 100644
> index 000000000000..7a59e3fbb970
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
> @@ -0,0 +1,60 @@
> +/*
> + * Copyright 2022 Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person
> +obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction,
> +including
> + * without limitation the rights to use, copy, modify, merge,
> +publish,
> + * distribute, sub license, and/or sell copies of the Software, and
> +to
> + * permit persons to whom the Software is furnished to do so, subject
> +to
> + * the following conditions:
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> +EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> +MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT
> +SHALL
> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR
> +ANY CLAIM,
> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT
> +OR
> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE
> +OR THE
> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * The above copyright notice and this permission notice (including
> +the
> + * next paragraph) shall be included in all copies or substantial
> +portions
> + * of the Software.
> + *
> + */
> +
> +#include "amdgpu_sw_ring.h"
> +#include "amdgpu_ring_mux.h"
> +
> +u64 amdgpu_sw_ring_get_rptr_gfx(struct amdgpu_ring *ring) {
> +     struct amdgpu_device *adev = ring->adev;
> +     struct amdgpu_ring_mux *mux = &adev->gfx.muxer;
> +
> +     BUG_ON(!ring->is_sw_ring);
> +     return amdgpu_ring_get_rptr_from_mux(mux, ring); }
> +
> +u64 amdgpu_sw_ring_get_wptr_gfx(struct amdgpu_ring *ring) {
> +     struct amdgpu_device *adev = ring->adev;
> +     struct amdgpu_ring_mux *mux = &adev->gfx.muxer;
> +
> +     BUG_ON(!ring->is_sw_ring);
> +     return amdgpu_ring_get_wptr_from_mux(mux, ring); }
> +
> +void amdgpu_sw_ring_set_wptr_gfx(struct amdgpu_ring *ring) {
> +     BUG_ON(!ring->is_sw_ring);
> +}
> +
> +void amdgpu_sw_ring_commit(struct amdgpu_ring *ring) {
> +     struct amdgpu_device *adev = ring->adev;
> +     struct amdgpu_ring_mux *mux = &adev->gfx.muxer;
> +
> +     BUG_ON(!ring->is_sw_ring);
> +     amdgpu_ring_set_wptr_to_mux(mux, ring, ring->wptr); }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h
> new file mode 100644
> index 000000000000..0a2e66318f3f
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h
> @@ -0,0 +1,43 @@
> +/*
> + * Copyright 2012 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person
> +obtaining a
> + * copy of this software and associated documentation files (the
> +"Software"),
> + * to deal in the Software without restriction, including without
> +limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> +sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> +the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> +included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> +EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> +MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
> +SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
> +DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> +OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
> +OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#include <drm/amdgpu_drm.h>
> +#include <drm/gpu_scheduler.h>
> +#include <drm/drm_print.h>
> +
> +#include "amdgpu_irq.h"
> +#include "amdgpu_ring.h"
> +#include "amdgpu.h"
> +
> +#ifndef __AMDGPU_SWRING_H__
> +#define __AMDGPU_SWRING_H__
> +
> +u64 amdgpu_sw_ring_get_rptr_gfx(struct amdgpu_ring *ring);
> +u64 amdgpu_sw_ring_get_wptr_gfx(struct amdgpu_ring *ring); void
> +amdgpu_sw_ring_set_wptr_gfx(struct amdgpu_ring *ring); void
> +amdgpu_sw_ring_commit(struct amdgpu_ring *ring);
> +
> +void amdgpu_sw_ring_ib_begin(struct amdgpu_ring *ring); void
> +amdgpu_sw_ring_ib_end(struct amdgpu_ring *ring);
> +
> +#endif

Re: [PATCH 1/4] drm/amdgpu: Introduce gfx software ring(v4)

[Luben Tuikov]

Hi,

You can still use "writep" and "readp" in the function name. Make code sustainable for the next programmer in the future.

Regards,
Luben

On 2022-09-15 01:30, Zhu, Jiadong wrote:
> [AMD Official Use Only - General]
> 
>> So, between the function names of "amdgpu_ring_get_rptr_from_mux()" and "amdgpu_ring_get_wptr_from_mux()",
>> they are 96.6(6)% _the_same_ name to a human. How about "amdgpu_ring_get_readp_from_mux()" and "amdgpu_ring_get_writep_from_mux()"?
> 
> I agree with the similarity between rptr and wptr. But the function named amdgpu_ring_get_rptr_from_mux  is aligned with amdgpu_ring_get_rptr for the same style, easier to understand.
> I would rather use it currently until we replace rptr with writep in all the other places, shall we?
> 
> Thanks,
> Jiadong
> 
> -----Original Message-----
> From: Tuikov, Luben <Luben.Tuikov@amd.com>
> Sent: Thursday, September 15, 2022 12:49 AM
> To: Zhu, Jiadong <Jiadong.Zhu@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Huang, Ray <Ray.Huang@amd.com>; Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
> Subject: Re: [PATCH 1/4] drm/amdgpu: Introduce gfx software ring(v4)
> 
> It's customary to add a Cc: tag in the commit message on subsequent revisions of a patch, once a person has reviewed and commented on it--Christian, Andrey, me, so that subsequent patches' emails go out to those people automatically via a CC.
> 
> Inlined:
> 
> On 2022-09-13 05:05, jiadong.zhu@amd.com wrote:
>> From: "Jiadong.Zhu" <Jiadong.Zhu@amd.com>
>>
>> The software ring is created to support priority context while there
>> is only one hardware queue for gfx.
>>
>> Every software rings has its fence driver and could
> 
> "ring", singular.
> 
>> be used as an ordinary ring for the gpu_scheduler.
> 
> "GPU scheduler".
> 
>> Multiple software rings are binded to a real ring
> 
> The past tense of "bind" is "bound", not "binded".
> 
>> with the ring muxer. The packages committed on the software ring are
>> copied to the real ring.
> 
> Use 79 column wrap setting in your editor, not 50 or 60.
> Wrap at 79.
> 
>>
>> v2: use array to store software ring entry.
>> v3: remove unnecessary prints.
>> v4: remove amdgpu_ring_sw_init/fini functions, using gtt for sw ring
>> buffer for later dma copy optimization.
>>
>> Signed-off-by: Jiadong.Zhu <Jiadong.Zhu@amd.com>
> 
> Add Cc: tags before the Signed-off-by line.
> 
>> ---
>>  drivers/gpu/drm/amd/amdgpu/Makefile          |   3 +-
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h      |   3 +
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h     |   3 +
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c | 182
>> +++++++++++++++++++  drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h |
>> 67 +++++++  drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c  |  60 ++++++
>> drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h  |  43 +++++
>>  7 files changed, 360 insertions(+), 1 deletion(-)  create mode 100644
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
>>  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
>>  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
>>  create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile
>> b/drivers/gpu/drm/amd/amdgpu/Makefile
>> index 3e0e2eb7e235..85224bc81ce5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
>> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
>> @@ -58,7 +58,8 @@ amdgpu-y += amdgpu_device.o amdgpu_kms.o \
>>       amdgpu_vm_sdma.o amdgpu_discovery.o amdgpu_ras_eeprom.o amdgpu_nbio.o \
>>       amdgpu_umc.o smu_v11_0_i2c.o amdgpu_fru_eeprom.o amdgpu_rap.o \
>>       amdgpu_fw_attestation.o amdgpu_securedisplay.o \
>> -     amdgpu_eeprom.o amdgpu_mca.o amdgpu_psp_ta.o amdgpu_lsdma.o
>> +     amdgpu_eeprom.o amdgpu_mca.o amdgpu_psp_ta.o amdgpu_lsdma.o \
>> +     amdgpu_sw_ring.o amdgpu_ring_mux.o
>>
>>  amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> index 53526ffb2ce1..0de8e3cd0f1c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> @@ -33,6 +33,7 @@
>>  #include "amdgpu_imu.h"
>>  #include "soc15.h"
>>  #include "amdgpu_ras.h"
>> +#include "amdgpu_ring_mux.h"
>>
>>  /* GFX current status */
>>  #define AMDGPU_GFX_NORMAL_MODE                       0x00000000L
>> @@ -346,6 +347,8 @@ struct amdgpu_gfx {
>>       struct amdgpu_gfx_ras           *ras;
>>
>>       bool                            is_poweron;
>> +
>> +     struct amdgpu_ring_mux                  muxer;
> 
> It doesn't align, possibly because TAB chars were used between "bool" and "is_poweron", and because TAB chars were used between "struct amdgpu_ring_mux" and "muxer".
> 
>>  };
>>
>>  #define amdgpu_gfx_get_gpu_clock_counter(adev)
>> (adev)->gfx.funcs->get_gpu_clock_counter((adev))
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> index 7d89a52091c0..fe33a683bfba 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> @@ -278,6 +278,9 @@ struct amdgpu_ring {
>>       bool                    is_mes_queue;
>>       uint32_t                hw_queue_id;
>>       struct amdgpu_mes_ctx_data *mes_ctx;
>> +
>> +     bool                    is_sw_ring;
> 
> Use spaces to align "is_sw_ring", not TAB chars, as I mentioned in my review of v3 of this patch.
> 
>> +
>>  };
>>
>>  #define amdgpu_ring_parse_cs(r, p, job, ib)
>> ((r)->funcs->parse_cs((p), (job), (ib))) diff --git
>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
>> new file mode 100644
>> index 000000000000..652a6d3e0ec3
>> --- /dev/null
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
>> @@ -0,0 +1,182 @@
>> +/*
>> + * Copyright 2022 Advanced Micro Devices, Inc.
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> +obtaining a
>> + * copy of this software and associated documentation files (the
>> +"Software"),
>> + * to deal in the Software without restriction, including without
>> +limitation
>> + * the rights to use, copy, modify, merge, publish, distribute,
>> +sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom
>> +the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be
>> +included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> +EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> +MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
>> +SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
>> +DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
>> +OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
>> +OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +#include <drm/drm_print.h>
>> +
>> +#include "amdgpu_ring_mux.h"
>> +#include "amdgpu_ring.h"
>> +
>> +#define AMDGPU_MUX_RESUBMIT_JIFFIES_TIMEOUT (HZ/2)
>> +
>> +static int copy_pkt_from_sw_ring(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring,
>> +     u64 s_begin, u64 s_end);
>> +
>> +int amdgpu_ring_mux_init(struct amdgpu_ring_mux *mux, struct
>> +amdgpu_ring *ring) {
>> +     mux->real_ring = ring;
>> +     memset(mux->ring_entries, 0, sizeof(mux->ring_entries));
>> +     mux->num_ring_entries = 0;
>> +     spin_lock_init(&mux->lock);
>> +     return 0;
> 
> Get rid of the "return 0;" here and make this function "void".
> 
>> +}
>> +
>> +void amdgpu_ring_mux_fini(struct amdgpu_ring_mux *mux) {
>> +     memset(mux->ring_entries, 0, sizeof(mux->ring_entries));
>> +     mux->num_ring_entries = 0;
>> +}
>> +
>> +int amdgpu_ring_mux_add_sw_ring(struct amdgpu_ring_mux *mux, struct
>> +amdgpu_ring *ring) {
>> +     struct amdgpu_mux_entry *e;
>> +
>> +     if (mux->num_ring_entries == AMDGPU_MAX_GFX_RINGS) {
>> +             DRM_ERROR("adding sw ring exceeds max gfx num\n");
>> +             return -ENOMEM;
> 
> Copied from my v3 review:
> 
> You can't return here -ENOMEM, as it is not a real out of memory condition.
> Maybe EINVAL or something like that, but not ENOMEM.
> 
> Also, under what circumstances would we get to this condition here?
> Are such circumstances valid?
> 
> And if so, then when this is returned, what happens?
> Does the driver die?
> 
> I feel we shouldn't ever have this here--it should've been calculated correctly to never have fallen in this/such a circumstance like that here.
> 
> v4: Perhaps make it dynamic? So that it works for any ASIC in the future without the need to modify AMDGPU_MAX_GFX_RINGS, or modify the driver.
> 
>> +     }
>> +
>> +     e = &mux->ring_entries[mux->num_ring_entries++];
> 
> This seems hackish here because you're not saving the increment of mux->num_ring_entries. Also rename the array to "ring_entry", so that "ring_entry[i]" means "ring entry at index i".
> 
> Perhaps keep a "ring_entry_size" member variable in "mux" to hold the size of "ring_entry[]", and use "num_ring_entries" to hold the dynamic (populated) size of "ring_entry[]", so that the _next_ available index is:
> 
> if (mux->num_ring_entries >= mux->ring_entry_size) {
>     Complain;
>     return -ENOENT;
> }
> 
> e = &mux->ring_entry[mux->num_ring_entries];
> mux->num_ring_entries += 1;
> 
> e->ring = ...
> 
>> +
>> +     e->ring = ring;
>> +     e->start_ptr_in_hw_ring = 0;
>> +     e->end_ptr_in_hw_ring = 0;
>> +     e->sw_cptr = 0;
>> +     e->sw_rptr = 0;
>> +     e->sw_wptr = 0;
> 
> If struct amdgpu_gfx is kzalloc-ed, then this is all zeroed here anyway. Something to think about.
> 
>> +
>> +     return 0;
>> +}
>> +
>> +static struct amdgpu_mux_entry *amdgpu_get_sw_entry(struct amdgpu_ring_mux *mux,
>> +                             struct amdgpu_ring *ring)
>> +{
>> +     struct amdgpu_mux_entry *e;
>> +     int i;
>> +
>> +     e = NULL;
>> +     for (i = 0; i < mux->num_ring_entries; i++) {
>> +             if (mux->ring_entries[i].ring == ring) {
>> +                     e = &mux->ring_entries[i];
>> +                     break;
>> +             }
>> +     }
>> +
>> +     return e;
>> +}
> 
> This is a bit naive, as every time we want to do an operation on a mapped ring, we do an O(n) search, as shown in the three functions below.
> 
> Perhaps add an index variable to struct amdgpu_ring to tell you this mapping, in O(1) time complexity, and eliminate this function.
> 
>> +
>> +void amdgpu_ring_set_wptr_to_mux(struct amdgpu_ring_mux *mux, struct
>> +amdgpu_ring *ring, u64 wptr) {
>> +     struct amdgpu_mux_entry *e;
>> +
>> +     e = amdgpu_get_sw_entry(mux, ring);
>> +     if (!e) {
>> +             DRM_ERROR("cannot find entry for sw ring\n");
>> +             return;
>> +     }
>> +
>> +     spin_lock(&mux->lock);
>> +     e->sw_cptr = e->sw_wptr;
>> +     e->sw_wptr = wptr;
>> +     e->start_ptr_in_hw_ring = mux->real_ring->wptr;
>> +
>> +     if (copy_pkt_from_sw_ring(mux, ring, e->sw_cptr, wptr) == 0) {
>> +             e->end_ptr_in_hw_ring = mux->real_ring->wptr;
>> +             amdgpu_ring_commit(mux->real_ring);
>> +     }
>> +
>> +     spin_unlock(&mux->lock);
>> +}
>> +
>> +u64 amdgpu_ring_get_wptr_from_mux(struct amdgpu_ring_mux *mux, struct
>> +amdgpu_ring *ring) {
>> +     struct amdgpu_mux_entry *e;
>> +
>> +     e = amdgpu_get_sw_entry(mux, ring);
>> +     if (!e) {
>> +             DRM_ERROR("cannot find entry for sw ring\n");
>> +             return 0;
>> +     }
>> +
>> +     return e->sw_wptr;
>> +}
>> +
>> +u64 amdgpu_ring_get_rptr_from_mux(struct amdgpu_ring_mux *mux, struct
>> +amdgpu_ring *ring)
> 
> So, between the function names of "amdgpu_ring_get_rptr_from_mux()" and "amdgpu_ring_get_wptr_from_mux()",
> they are 96.6(6)% _the_same_ name to a human. How about "amdgpu_ring_get_readp_from_mux()" and "amdgpu_ring_get_writep_from_mux()"?
> 
>> +{
>> +     struct amdgpu_mux_entry *e;
>> +     u64 r_rptr, offset, start, end;
> 
> "r_rptr" is not a very descriptive name. How about "readp"? It is more descriptive to a human.
> They are all "ptrs", but what is more descriptive is whether it is a "read" or "write", etc., so "readp" is more descriptive.
> 
>> +
>> +     e = amdgpu_get_sw_entry(mux, ring);
>> +     if (!e) {
>> +             DRM_ERROR("no sw entry found!\n");
>> +             return 0;
>> +     }
>> +
>> +     r_rptr = amdgpu_ring_get_rptr(mux->real_ring);
>> +
>> +     start = e->start_ptr_in_hw_ring & mux->real_ring->buf_mask;
>> +     end = e->end_ptr_in_hw_ring & mux->real_ring->buf_mask;
>> +     if (start > end) {
>> +             if (r_rptr <= end)
>> +                     r_rptr += mux->real_ring->ring_size >> 2;
>> +             end += mux->real_ring->ring_size >> 2;
>> +     }
>> +
>> +     if (r_rptr <= end && r_rptr >= start) {
>> +             offset = r_rptr - start;
>> +             e->sw_rptr = (e->sw_cptr + offset) & ring->buf_mask;
>> +     } else if (r_rptr < start) {
>> +             e->sw_rptr = e->sw_cptr;
>> +     } else {
>> +             e->sw_rptr = e->sw_wptr;
>> +     }
>> +
>> +     return e->sw_rptr;
>> +}
>> +
>> +/*copy packages on sw ring range[begin, end) */ static int
>> +copy_pkt_from_sw_ring(struct amdgpu_ring_mux *mux, struct amdgpu_ring *ring,
>> +     u64 s_begin, u64 s_end)
>> +{
>> +     u64 begin, end, r_begin, r_end;
>> +     struct amdgpu_ring *real_ring = mux->real_ring;
>> +
>> +     begin = s_begin & ring->buf_mask;
>> +     end = s_end & ring->buf_mask;
>> +
>> +     r_begin = real_ring->wptr & real_ring->buf_mask;
>> +     if (begin == end)
>> +             return -ERANGE;
>> +     if (begin > end) {
>> +             amdgpu_ring_alloc(real_ring, (ring->ring_size >> 2) + end - begin);
>> +             amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[begin],
>> +                     (ring->ring_size >> 2) - begin);
>> +             amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[0], end);
>> +     } else {
>> +             amdgpu_ring_alloc(real_ring, end - begin);
>> +             amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[begin], end - begin);
>> +     }
>> +
>> +     r_end = real_ring->wptr & real_ring->buf_mask;
>> +
>> +     return 0;
>> +}
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
>> new file mode 100644
>> index 000000000000..d058c43bb063
>> --- /dev/null
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.h
>> @@ -0,0 +1,67 @@
>> +/*
>> + * Copyright 2022 Advanced Micro Devices, Inc.
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> +obtaining a
>> + * copy of this software and associated documentation files (the
>> +"Software"),
>> + * to deal in the Software without restriction, including without
>> +limitation
>> + * the rights to use, copy, modify, merge, publish, distribute,
>> +sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom
>> +the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be
>> +included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> +EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> +MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
>> +SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
>> +DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
>> +OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
>> +OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +#ifndef __AMDGPU_RING_MUX__
>> +#define __AMDGPU_RING_MUX__
>> +
>> +#include <linux/timer.h>
>> +#include <linux/spinlock.h>
>> +#include "amdgpu_ring.h"
>> +
>> +struct amdgpu_ring;
>> +/*
>> + * start_ptr_in_hw_ring - last copied start loc on hw ring
>> + * end_ptr_in_hw_ring - last copied end loc on hw ring  *sw_cptr -the
>> +begin of copy ptr in sw ring  *sw_rptr; the read ptr in sw ring
>> +*sw_wptr; the write ptr in sw ring  */ struct amdgpu_mux_entry {
>> +     struct amdgpu_ring      *ring;
> 
> Don't use TAB chars to align--use space characters.
> 
>> +     u64 start_ptr_in_hw_ring;
>> +     u64 end_ptr_in_hw_ring;
>> +
>> +     u64 sw_cptr;
>> +     u64 sw_rptr;
>> +     u64 sw_wptr;
> 
> Align all members to "*ring".
> 
>> +};
>> +
>> +struct amdgpu_ring_mux {
>> +     struct amdgpu_ring *real_ring;
>> +
>> +     struct amdgpu_mux_entry ring_entries[AMDGPU_MAX_GFX_RINGS];
>> +
>> +     unsigned num_ring_entries;
>> +
>> +     spinlock_t                      lock;
> 
> Don't use TAB char to align. Also note that TAB char in the kernel is 8 chars long, not 2, 3 or 4.
> 
> Regards,
> Luben
> 
>> +
>> +};
>> +
>> +int amdgpu_ring_mux_init(struct amdgpu_ring_mux *mux, struct
>> +amdgpu_ring *ring); void amdgpu_ring_mux_fini(struct amdgpu_ring_mux
>> +*mux); int amdgpu_ring_mux_add_sw_ring(struct amdgpu_ring_mux *mux,
>> +struct amdgpu_ring *ring); void amdgpu_ring_set_wptr_to_mux(struct
>> +amdgpu_ring_mux *mux, struct amdgpu_ring *ring, u64 wptr);
>> +u64 amdgpu_ring_get_wptr_from_mux(struct amdgpu_ring_mux *mux, struct
>> +amdgpu_ring *ring);
>> +u64 amdgpu_ring_get_rptr_from_mux(struct amdgpu_ring_mux *mux, struct
>> +amdgpu_ring *ring);
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
>> new file mode 100644
>> index 000000000000..7a59e3fbb970
>> --- /dev/null
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.c
>> @@ -0,0 +1,60 @@
>> +/*
>> + * Copyright 2022 Advanced Micro Devices, Inc.
>> + * All Rights Reserved.
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> +obtaining a
>> + * copy of this software and associated documentation files (the
>> + * "Software"), to deal in the Software without restriction,
>> +including
>> + * without limitation the rights to use, copy, modify, merge,
>> +publish,
>> + * distribute, sub license, and/or sell copies of the Software, and
>> +to
>> + * permit persons to whom the Software is furnished to do so, subject
>> +to
>> + * the following conditions:
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> +EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> +MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT
>> +SHALL
>> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR
>> +ANY CLAIM,
>> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT
>> +OR
>> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE
>> +OR THE
>> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
>> + *
>> + * The above copyright notice and this permission notice (including
>> +the
>> + * next paragraph) shall be included in all copies or substantial
>> +portions
>> + * of the Software.
>> + *
>> + */
>> +
>> +#include "amdgpu_sw_ring.h"
>> +#include "amdgpu_ring_mux.h"
>> +
>> +u64 amdgpu_sw_ring_get_rptr_gfx(struct amdgpu_ring *ring) {
>> +     struct amdgpu_device *adev = ring->adev;
>> +     struct amdgpu_ring_mux *mux = &adev->gfx.muxer;
>> +
>> +     BUG_ON(!ring->is_sw_ring);
>> +     return amdgpu_ring_get_rptr_from_mux(mux, ring); }
>> +
>> +u64 amdgpu_sw_ring_get_wptr_gfx(struct amdgpu_ring *ring) {
>> +     struct amdgpu_device *adev = ring->adev;
>> +     struct amdgpu_ring_mux *mux = &adev->gfx.muxer;
>> +
>> +     BUG_ON(!ring->is_sw_ring);
>> +     return amdgpu_ring_get_wptr_from_mux(mux, ring); }
>> +
>> +void amdgpu_sw_ring_set_wptr_gfx(struct amdgpu_ring *ring) {
>> +     BUG_ON(!ring->is_sw_ring);
>> +}
>> +
>> +void amdgpu_sw_ring_commit(struct amdgpu_ring *ring) {
>> +     struct amdgpu_device *adev = ring->adev;
>> +     struct amdgpu_ring_mux *mux = &adev->gfx.muxer;
>> +
>> +     BUG_ON(!ring->is_sw_ring);
>> +     amdgpu_ring_set_wptr_to_mux(mux, ring, ring->wptr); }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h
>> new file mode 100644
>> index 000000000000..0a2e66318f3f
>> --- /dev/null
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sw_ring.h
>> @@ -0,0 +1,43 @@
>> +/*
>> + * Copyright 2012 Advanced Micro Devices, Inc.
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> +obtaining a
>> + * copy of this software and associated documentation files (the
>> +"Software"),
>> + * to deal in the Software without restriction, including without
>> +limitation
>> + * the rights to use, copy, modify, merge, publish, distribute,
>> +sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom
>> +the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be
>> +included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> +EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> +MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
>> +SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,
>> +DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
>> +OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
>> +OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +#include <drm/amdgpu_drm.h>
>> +#include <drm/gpu_scheduler.h>
>> +#include <drm/drm_print.h>
>> +
>> +#include "amdgpu_irq.h"
>> +#include "amdgpu_ring.h"
>> +#include "amdgpu.h"
>> +
>> +#ifndef __AMDGPU_SWRING_H__
>> +#define __AMDGPU_SWRING_H__
>> +
>> +u64 amdgpu_sw_ring_get_rptr_gfx(struct amdgpu_ring *ring);
>> +u64 amdgpu_sw_ring_get_wptr_gfx(struct amdgpu_ring *ring); void
>> +amdgpu_sw_ring_set_wptr_gfx(struct amdgpu_ring *ring); void
>> +amdgpu_sw_ring_commit(struct amdgpu_ring *ring);
>> +
>> +void amdgpu_sw_ring_ib_begin(struct amdgpu_ring *ring); void
>> +amdgpu_sw_ring_ib_end(struct amdgpu_ring *ring);
>> +
>> +#endif

Regards,
-- 
Luben