[PATCH v6 00/10] DSB enablement.

[PATCH v6 00/10] DSB enablement.

[Animesh Manna]

Display State Buffer (DSB) is hardware capability which allows driver
to batch submit HW programming.

As part of initial enablement common api created which currently used
to program gamma lut proramming.

Going forwad DSB support can be added for HDR and flip related operation.

HSDES: 1209978241
BSpec: 32020

v1: Initial version.

v2: Move intel_dsb files under display folder and fixed an issue.

v3: As per review comments from Chris and Jani,
- removed some unwanted code. (Chris)
- Used i915_gem_object_create_internal instead of _shmem. (Chris)
- cmd_buf_tail removed and can be derived through vma object. (Chris)
- Simplified and optimized code few places. (Chris)
- Called dsb-api directly in callsites instead going via I915_WRITE. (Jani)

v4: Addressed review commnets from Shashank.

v5: Addressed review commnets from Shashank and Jani.

v6: Addressed review commnets from Shashank.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Swati Sharma <swati2.sharma@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>

Animesh Manna (10):
  drm/i915/dsb: feature flag added for display state buffer.
  drm/i915/dsb: DSB context creation.
  drm/i915/dsb: single register write function for DSB.
  drm/i915/dsb: Indexed register write function for DSB.
  drm/i915/dsb: Check DSB engine status.
  drm/i915/dsb: functions to enable/disable DSB engine.
  drm/i915/dsb: function to trigger workload execution of DSB.
  drm/i915/dsb: Enable gamma lut programming using DSB.
  drm/i915/dsb: Enable DSB for gen12.
  drm/i915/dsb: Documentation for DSB.

 Documentation/gpu/i915.rst                    |   9 +
 drivers/gpu/drm/i915/Makefile                 |   1 +
 drivers/gpu/drm/i915/display/intel_color.c    |  57 +--
 .../drm/i915/display/intel_display_types.h    |   3 +
 drivers/gpu/drm/i915/display/intel_dsb.c      | 324 ++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dsb.h      |  48 +++
 drivers/gpu/drm/i915/i915_drv.h               |   3 +
 drivers/gpu/drm/i915/i915_pci.c               |   3 +-
 drivers/gpu/drm/i915/i915_reg.h               |  10 +
 drivers/gpu/drm/i915/intel_device_info.h      |   1 +
 10 files changed, 436 insertions(+), 23 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/display/intel_dsb.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_dsb.h

-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 01/10] drm/i915/dsb: feature flag added for display state buffer.

[Animesh Manna]

Display State Buffer(DSB) is a new hardware capability, introduced
in GEN12 display. DSB allows a driver to batch-program display HW
registers.

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          | 2 ++
 drivers/gpu/drm/i915/intel_device_info.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2ea11123e933..6c6af007f29d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1863,6 +1863,8 @@ static inline struct drm_i915_private *pdev_to_i915(struct pci_dev *pdev)
 	(BUILD_BUG_ON_ZERO(!__builtin_constant_p(n)) + \
 	 INTEL_INFO(dev_priv)->gen == (n))
 
+#define HAS_DSB(dev_priv)	(INTEL_INFO(dev_priv)->display.has_dsb)
+
 /*
  * Return true if revision is in range [since,until] inclusive.
  *
diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
index 92e0c2e0954c..e206f298f48e 100644
--- a/drivers/gpu/drm/i915/intel_device_info.h
+++ b/drivers/gpu/drm/i915/intel_device_info.h
@@ -135,6 +135,7 @@ enum intel_ppgtt_type {
 	func(has_csr); \
 	func(has_ddi); \
 	func(has_dp_mst); \
+	func(has_dsb); \
 	func(has_fbc); \
 	func(has_gmch); \
 	func(has_hotplug); \
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 02/10] drm/i915/dsb: DSB context creation.

[Animesh Manna]

This patch adds a function, which will internally get the gem buffer
for DSB engine. The GEM buffer is from global GTT, and is mapped into
CPU domain, contains the data + opcode to be feed to DSB engine.

v1: Initial version.

v2:
- removed some unwanted code. (Chris)
- Used i915_gem_object_create_internal instead of _shmem. (Chris)
- cmd_buf_tail removed and can be derived through vma object. (Chris)

v3: vma realeased if i915_gem_object_pin_map() failed. (Shashank)

v4: for simplification and based on current usage added single dsb
object in intel_crtc. (Shashank)

v5: seting NULL to cmd_buf moved outside of mutex in dsb-put(). (Shashank)

Cc: Imre Deak <imre.deak@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/Makefile                 |  1 +
 .../drm/i915/display/intel_display_types.h    |  3 +
 drivers/gpu/drm/i915/display/intel_dsb.c      | 70 +++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dsb.h      | 30 ++++++++
 drivers/gpu/drm/i915/i915_drv.h               |  1 +
 5 files changed, 105 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/display/intel_dsb.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_dsb.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 658b930d34a8..6313e7b4bd78 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -172,6 +172,7 @@ i915-y += \
 	display/intel_display_power.o \
 	display/intel_dpio_phy.o \
 	display/intel_dpll_mgr.o \
+	display/intel_dsb.o \
 	display/intel_fbc.o \
 	display/intel_fifo_underrun.o \
 	display/intel_frontbuffer.o \
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index d5cc4b810d9e..49c902b00484 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1033,6 +1033,9 @@ struct intel_crtc {
 
 	/* scalers available on this crtc */
 	int num_scalers;
+
+	/* per pipe DSB related info */
+	struct intel_dsb dsb;
 };
 
 struct intel_plane {
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
new file mode 100644
index 000000000000..7c1b1574788c
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2019 Intel Corporation
+ *
+ */
+
+#include "i915_drv.h"
+#include "intel_display_types.h"
+
+#define DSB_BUF_SIZE    (2 * PAGE_SIZE)
+
+struct intel_dsb *
+intel_dsb_get(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *i915 = to_i915(dev);
+	struct drm_i915_gem_object *obj;
+	struct i915_vma *vma;
+	struct intel_dsb *dsb = &crtc->dsb;
+	intel_wakeref_t wakeref;
+
+	if ((!HAS_DSB(i915)) || dsb->cmd_buf)
+		return dsb;
+
+	dsb->id = DSB1;
+	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
+
+	obj = i915_gem_object_create_internal(i915, DSB_BUF_SIZE);
+	if (IS_ERR(obj))
+		goto err;
+
+	mutex_lock(&i915->drm.struct_mutex);
+	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, PIN_MAPPABLE);
+	mutex_unlock(&i915->drm.struct_mutex);
+	if (IS_ERR(vma)) {
+		DRM_ERROR("Vma creation failed\n");
+		i915_gem_object_put(obj);
+		goto err;
+	}
+
+	dsb->cmd_buf = i915_gem_object_pin_map(vma->obj, I915_MAP_WC);
+	if (IS_ERR(dsb->cmd_buf)) {
+		DRM_ERROR("Command buffer creation failed\n");
+		i915_vma_unpin_and_release(&vma, 0);
+		dsb->cmd_buf = NULL;
+		goto err;
+	}
+	dsb->vma = vma;
+
+err:
+	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
+	return dsb;
+}
+
+void intel_dsb_put(struct intel_dsb *dsb)
+{
+	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
+	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
+
+	if (!dsb)
+		return;
+
+	if (dsb->cmd_buf) {
+		mutex_lock(&i915->drm.struct_mutex);
+		i915_gem_object_unpin_map(dsb->vma->obj);
+		i915_vma_unpin_and_release(&dsb->vma, 0);
+		mutex_unlock(&i915->drm.struct_mutex);
+		dsb->cmd_buf = NULL;
+	}
+}
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
new file mode 100644
index 000000000000..27eb68eb5392
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_dsb.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2019 Intel Corporation
+ */
+
+#ifndef _INTEL_DSB_H
+#define _INTEL_DSB_H
+
+struct intel_crtc;
+struct i915_vma;
+
+enum dsb_id {
+	INVALID_DSB = -1,
+	DSB1,
+	DSB2,
+	DSB3,
+	MAX_DSB_PER_PIPE
+};
+
+struct intel_dsb {
+	enum dsb_id id;
+	u32 *cmd_buf;
+	struct i915_vma *vma;
+};
+
+struct intel_dsb *
+intel_dsb_get(struct intel_crtc *crtc);
+void intel_dsb_put(struct intel_dsb *dsb);
+
+#endif
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6c6af007f29d..069a06edb5c6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -67,6 +67,7 @@
 #include "display/intel_display.h"
 #include "display/intel_display_power.h"
 #include "display/intel_dpll_mgr.h"
+#include "display/intel_dsb.h"
 #include "display/intel_frontbuffer.h"
 #include "display/intel_gmbus.h"
 #include "display/intel_opregion.h"
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 03/10] drm/i915/dsb: single register write function for DSB.

[Animesh Manna]

DSB support single register write through opcode 0x1. Generic
api created which accumulate all single register write in a batch
buffer and once DSB is triggered, it will program all the registers
at the same time.

v1: Initial version.
v2: Unused macro removed and cosmetic changes done. (Shashank)
v3: set free_pos to zero in dsb-put() instead dsb-get() and
a cosmetic change. (Shashank)

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 30 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dsb.h |  9 +++++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 7c1b1574788c..e2c383352145 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -9,6 +9,13 @@
 
 #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
 
+/* DSB opcodes. */
+#define DSB_OPCODE_SHIFT		24
+#define DSB_OPCODE_MMIO_WRITE		0x1
+#define DSB_OPCODE_INDEXED_WRITE	0x9
+#define DSB_BYTE_EN			0xF
+#define DSB_BYTE_EN_SHIFT		20
+
 struct intel_dsb *
 intel_dsb_get(struct intel_crtc *crtc)
 {
@@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb)
 		i915_vma_unpin_and_release(&dsb->vma, 0);
 		mutex_unlock(&i915->drm.struct_mutex);
 		dsb->cmd_buf = NULL;
+		dsb->free_pos = 0;
+	}
+}
+
+void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
+{
+	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	u32 *buf = dsb->cmd_buf;
+
+	if (!buf) {
+		I915_WRITE(reg, val);
+		return;
+	}
+
+	if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
+		DRM_DEBUG_KMS("DSB buffer overflow\n");
+		return;
 	}
+
+	buf[dsb->free_pos++] = val;
+	buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE  << DSB_OPCODE_SHIFT) |
+			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
+			       i915_mmio_reg_offset(reg);
 }
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
index 27eb68eb5392..31b87dcfe160 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.h
+++ b/drivers/gpu/drm/i915/display/intel_dsb.h
@@ -6,6 +6,8 @@
 #ifndef _INTEL_DSB_H
 #define _INTEL_DSB_H
 
+#include "i915_reg.h"
+
 struct intel_crtc;
 struct i915_vma;
 
@@ -21,10 +23,17 @@ struct intel_dsb {
 	enum dsb_id id;
 	u32 *cmd_buf;
 	struct i915_vma *vma;
+
+	/*
+	 * free_pos will point the first free entry position
+	 * and help in calculating tail of command buffer.
+	 */
+	int free_pos;
 };
 
 struct intel_dsb *
 intel_dsb_get(struct intel_crtc *crtc);
 void intel_dsb_put(struct intel_dsb *dsb);
+void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
 
 #endif
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 04/10] drm/i915/dsb: Indexed register write function for DSB.

[Animesh Manna]

DSB can program large set of data through indexed register write
(opcode 0x9) in one shot. DSB feature can be used for bulk register
programming e.g. gamma lut programming, HDR meta data programming.

v1: initial version.
v2: simplified code by using ALIGN(). (Chris)
v3: ascii table added as code comment. (Shashank)
v4: cosmetic changes done. (Shashank)

Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 65 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dsb.h |  8 +++
 2 files changed, 73 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index e2c383352145..9e2927f869b9 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -15,6 +15,7 @@
 #define DSB_OPCODE_INDEXED_WRITE	0x9
 #define DSB_BYTE_EN			0xF
 #define DSB_BYTE_EN_SHIFT		20
+#define DSB_REG_VALUE_MASK		0xfffff
 
 struct intel_dsb *
 intel_dsb_get(struct intel_crtc *crtc)
@@ -77,6 +78,70 @@ void intel_dsb_put(struct intel_dsb *dsb)
 	}
 }
 
+void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
+				 u32 val)
+{
+	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	u32 *buf = dsb->cmd_buf;
+	u32 reg_val;
+
+	if (!buf) {
+		I915_WRITE(reg, val);
+		return;
+	}
+
+	if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
+		DRM_DEBUG_KMS("DSB buffer overflow\n");
+		return;
+	}
+
+	/*
+	 * For example the buffer will look like below for 3 dwords for auto
+	 * increment register:
+	 * +--------------------------------------------------------+
+	 * | size = 3 | offset &| value1 | value2 | value3 | zero   |
+	 * |          | opcode  |        |        |        |        |
+	 * +--------------------------------------------------------+
+	 * +          +         +        +        +        +        +
+	 * 0          4         8        12       16       20       24
+	 * Byte
+	 *
+	 * As every instruction is 8 byte aligned the index of dsb instruction
+	 * will start always from even number while dealing with u32 array. If
+	 * we are writing odd no of dwords, Zeros will be added in the end for
+	 * padding.
+	 */
+	reg_val = buf[dsb->ins_start_offset + 1] & DSB_REG_VALUE_MASK;
+	if (reg_val != i915_mmio_reg_offset(reg)) {
+		/* Every instruction should be 8 byte aligned. */
+		dsb->free_pos = ALIGN(dsb->free_pos, 2);
+
+		dsb->ins_start_offset = dsb->free_pos;
+
+		/* Update the size. */
+		buf[dsb->free_pos++] = 1;
+
+		/* Update the opcode and reg. */
+		buf[dsb->free_pos++] = (DSB_OPCODE_INDEXED_WRITE  <<
+					DSB_OPCODE_SHIFT) |
+					i915_mmio_reg_offset(reg);
+
+		/* Update the value. */
+		buf[dsb->free_pos++] = val;
+	} else {
+		/* Update the new value. */
+		buf[dsb->free_pos++] = val;
+
+		/* Update the size. */
+		buf[dsb->ins_start_offset]++;
+	}
+
+	/* if number of data words is odd, then the last dword should be 0.*/
+	if (dsb->free_pos & 0x1)
+		buf[dsb->free_pos] = 0;
+}
+
 void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
 {
 	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
index 31b87dcfe160..9b2522f20bfb 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.h
+++ b/drivers/gpu/drm/i915/display/intel_dsb.h
@@ -29,11 +29,19 @@ struct intel_dsb {
 	 * and help in calculating tail of command buffer.
 	 */
 	int free_pos;
+
+	/*
+	 * ins_start_offset will help to store start address
+	 * of the dsb instuction of auto-increment register.
+	 */
+	u32 ins_start_offset;
 };
 
 struct intel_dsb *
 intel_dsb_get(struct intel_crtc *crtc);
 void intel_dsb_put(struct intel_dsb *dsb);
 void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
+void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
+				 u32 val);
 
 #endif
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 05/10] drm/i915/dsb: Check DSB engine status.

[Animesh Manna]

As per bspec check for DSB status before programming any
of its register. Inline function added to check the dsb status.

Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 9 +++++++++
 drivers/gpu/drm/i915/i915_reg.h          | 7 +++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 9e2927f869b9..b1da2b06263a 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -17,6 +17,15 @@
 #define DSB_BYTE_EN_SHIFT		20
 #define DSB_REG_VALUE_MASK		0xfffff
 
+static inline bool is_dsb_busy(struct intel_dsb *dsb)
+{
+	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+
+	return DSB_STATUS & I915_READ(DSB_CTRL(pipe, dsb->id));
+}
+
 struct intel_dsb *
 intel_dsb_get(struct intel_crtc *crtc)
 {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index bf37ecebc82f..9188a0b53538 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -11683,4 +11683,11 @@ enum skl_power_gate {
 #define PORT_TX_DFLEXDPCSSS(fia)		_MMIO_FIA((fia), 0x00894)
 #define   DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)		(1 << (tc_port))
 
+/* This register controls the Display State Buffer (DSB) engines. */
+#define _DSBSL_INSTANCE_BASE		0x70B00
+#define DSBSL_INSTANCE(pipe, id)	(_DSBSL_INSTANCE_BASE + \
+					 (pipe) * 0x1000 + (id) * 100)
+#define DSB_CTRL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x8)
+#define   DSB_STATUS			(1 << 0)
+
 #endif /* _I915_REG_H_ */
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 06/10] drm/i915/dsb: functions to enable/disable DSB engine.

[Animesh Manna]

DSB will be used for performance improvement for some special scenario.
DSB engine will be enabled based on need and after completion of its work
will be disabled. Api added for enable/disable operation by using DSB_CTRL
register.

v1: Initial version.
v2: POSTING_READ added after writing control register. (Shashank)
v3: cosmetic changes done. (Shashank)

Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 40 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h          |  1 +
 2 files changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index b1da2b06263a..2b0ffc0afb74 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -26,6 +26,46 @@ static inline bool is_dsb_busy(struct intel_dsb *dsb)
 	return DSB_STATUS & I915_READ(DSB_CTRL(pipe, dsb->id));
 }
 
+static inline bool intel_dsb_enable_engine(struct intel_dsb *dsb)
+{
+	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+	u32 dsb_ctrl;
+
+	dsb_ctrl = I915_READ(DSB_CTRL(pipe, dsb->id));
+	if (DSB_STATUS & dsb_ctrl) {
+		DRM_DEBUG_KMS("DSB engine is busy.\n");
+		return false;
+	}
+
+	dsb_ctrl |= DSB_ENABLE;
+	I915_WRITE(DSB_CTRL(pipe, dsb->id), dsb_ctrl);
+
+	POSTING_READ(DSB_CTRL(pipe, dsb->id));
+	return true;
+}
+
+static inline bool intel_dsb_disable_engine(struct intel_dsb *dsb)
+{
+	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+	u32 dsb_ctrl;
+
+	dsb_ctrl = I915_READ(DSB_CTRL(pipe, dsb->id));
+	if (DSB_STATUS & dsb_ctrl) {
+		DRM_DEBUG_KMS("DSB engine is busy.\n");
+		return false;
+	}
+
+	dsb_ctrl &= ~DSB_ENABLE;
+	I915_WRITE(DSB_CTRL(pipe, dsb->id), dsb_ctrl);
+
+	POSTING_READ(DSB_CTRL(pipe, dsb->id));
+	return true;
+}
+
 struct intel_dsb *
 intel_dsb_get(struct intel_crtc *crtc)
 {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9188a0b53538..2dbaa49f5c74 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -11688,6 +11688,7 @@ enum skl_power_gate {
 #define DSBSL_INSTANCE(pipe, id)	(_DSBSL_INSTANCE_BASE + \
 					 (pipe) * 0x1000 + (id) * 100)
 #define DSB_CTRL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x8)
+#define   DSB_ENABLE			(1 << 31)
 #define   DSB_STATUS			(1 << 0)
 
 #endif /* _I915_REG_H_ */
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 07/10] drm/i915/dsb: function to trigger workload execution of DSB.

[Animesh Manna]

Batch buffer will be created through dsb-reg-write function which can have
single/multiple request based on usecase and once the buffer is ready
commit function will trigger the execution of the batch buffer. All
the registers will be updated simultaneously.

v1: Initial version.
v2: Optimized code few places. (Chris)
v3: USed DRM_ERROR for dsb head/tail programming failure. (Shashank)

Cc: Imre Deak <imre.deak@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 42 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dsb.h |  1 +
 drivers/gpu/drm/i915/i915_reg.h          |  2 ++
 3 files changed, 45 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 2b0ffc0afb74..eea86afb0583 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -212,3 +212,45 @@ void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
 			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
 			       i915_mmio_reg_offset(reg);
 }
+
+void intel_dsb_commit(struct intel_dsb *dsb)
+{
+	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	enum pipe pipe = crtc->pipe;
+	u32 tail;
+
+	if (!dsb->free_pos)
+		return;
+
+	if (!intel_dsb_enable_engine(dsb))
+		goto reset;
+
+	if (is_dsb_busy(dsb)) {
+		DRM_ERROR("HEAD_PTR write failed - dsb engine is busy.\n");
+		goto reset;
+	}
+	I915_WRITE(DSB_HEAD(pipe, dsb->id), i915_ggtt_offset(dsb->vma));
+
+	tail = ALIGN(dsb->free_pos * 4, CACHELINE_BYTES);
+	if (tail > dsb->free_pos * 4)
+		memset(&dsb->cmd_buf[dsb->free_pos], 0,
+		       (tail - dsb->free_pos * 4));
+
+	if (is_dsb_busy(dsb)) {
+		DRM_ERROR("TAIL_PTR write failed - dsb engine is busy.\n");
+		goto reset;
+	}
+	DRM_DEBUG_KMS("DSB execution started - head 0x%x, tail 0x%x\n",
+		      i915_ggtt_offset(dsb->vma), tail);
+	I915_WRITE(DSB_TAIL(pipe, dsb->id), i915_ggtt_offset(dsb->vma) + tail);
+	if (wait_for(!is_dsb_busy(dsb), 1)) {
+		DRM_ERROR("Timed out waiting for DSB workload completion.\n");
+		goto reset;
+	}
+
+reset:
+	dsb->free_pos = 0;
+	intel_dsb_disable_engine(dsb);
+}
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
index 9b2522f20bfb..7389c8c5b665 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.h
+++ b/drivers/gpu/drm/i915/display/intel_dsb.h
@@ -43,5 +43,6 @@ void intel_dsb_put(struct intel_dsb *dsb);
 void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
 void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
 				 u32 val);
+void intel_dsb_commit(struct intel_dsb *dsb);
 
 #endif
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2dbaa49f5c74..c77b5066d8dd 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -11687,6 +11687,8 @@ enum skl_power_gate {
 #define _DSBSL_INSTANCE_BASE		0x70B00
 #define DSBSL_INSTANCE(pipe, id)	(_DSBSL_INSTANCE_BASE + \
 					 (pipe) * 0x1000 + (id) * 100)
+#define DSB_HEAD(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x0)
+#define DSB_TAIL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x4)
 #define DSB_CTRL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x8)
 #define   DSB_ENABLE			(1 << 31)
 #define   DSB_STATUS			(1 << 0)
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 08/10] drm/i915/dsb: Enable gamma lut programming using DSB.

[Animesh Manna]

Gamma lut programming can be programmed using DSB
where bulk register programming can be done using indexed
register write which takes number of data and the mmio offset
to be written.

Currently enabled for 12-bit gamma LUT which is enabled by
default and later 8-bit/10-bit will be enabled in future
based on need.

v1: Initial version.
v2: Directly call dsb-api at callsites. (Jani)
v3:
- modified the code as per single dsb instance per crtc. (Shashank)
- Added dsb get/put call in platform specific load_lut hook. (Jani)
- removed dsb pointer from dev_priv. (Jani)
v4: simplified code by dropping ref-count implementation. (Shashank)

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 57 +++++++++++++---------
 1 file changed, 35 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 318308dc136c..b6b9f0e5166b 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -611,12 +611,13 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
 static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	struct intel_dsb *dsb = intel_dsb_get(crtc);
 	enum pipe pipe = crtc->pipe;
 
 	/* Program the max register to clamp values > 1.0. */
-	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
-	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
-	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
+	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
+	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
+	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
 
 	/*
 	 * Program the gc max 2 register to clamp values > 1.0.
@@ -624,9 +625,12 @@ static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
 	 * from 3.0 to 7.0
 	 */
 	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
-		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16);
-		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16);
-		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16);
+		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 0),
+				    1 << 16);
+		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 1),
+				    1 << 16);
+		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 2),
+				    1 << 16);
 	}
 }
 
@@ -787,22 +791,22 @@ icl_load_gcmax(const struct intel_crtc_state *crtc_state,
 	       const struct drm_color_lut *color)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	struct intel_dsb *dsb = intel_dsb_get(crtc);
 	enum pipe pipe = crtc->pipe;
 
 	/* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */
-	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), color->red);
-	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), color->green);
-	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), color->blue);
+	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 0), color->red);
+	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 1), color->green);
+	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 2), color->blue);
 }
 
 static void
 icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
 	const struct drm_color_lut *lut = blob->data;
+	struct intel_dsb *dsb = intel_dsb_get(crtc);
 	enum pipe pipe = crtc->pipe;
 	u32 i;
 
@@ -813,15 +817,16 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
 	 * Superfine segment has 9 entries, corresponding to values
 	 * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
 	 */
-	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
+	intel_dsb_reg_write(dsb, PREC_PAL_MULTI_SEG_INDEX(pipe),
+			    PAL_PREC_AUTO_INCREMENT);
 
 	for (i = 0; i < 9; i++) {
 		const struct drm_color_lut *entry = &lut[i];
 
-		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
-			   ilk_lut_12p4_ldw(entry));
-		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
-			   ilk_lut_12p4_udw(entry));
+		intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe),
+					    ilk_lut_12p4_ldw(entry));
+		intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe),
+					    ilk_lut_12p4_udw(entry));
 	}
 }
 
@@ -829,10 +834,10 @@ static void
 icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
 	const struct drm_color_lut *lut = blob->data;
 	const struct drm_color_lut *entry;
+	struct intel_dsb *dsb = intel_dsb_get(crtc);
 	enum pipe pipe = crtc->pipe;
 	u32 i;
 
@@ -847,11 +852,13 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
 	 * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
 	 * with seg2[0] being unused by the hardware.
 	 */
-	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
+	intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
 	for (i = 1; i < 257; i++) {
 		entry = &lut[i * 8];
-		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
-		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
+		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
+					    ilk_lut_12p4_ldw(entry));
+		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
+					    ilk_lut_12p4_udw(entry));
 	}
 
 	/*
@@ -868,8 +875,10 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
 	 */
 	for (i = 0; i < 256; i++) {
 		entry = &lut[i * 8 * 128];
-		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
-		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
+		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
+					    ilk_lut_12p4_ldw(entry));
+		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
+					    ilk_lut_12p4_udw(entry));
 	}
 
 	/* The last entry in the LUT is to be programmed in GCMAX */
@@ -882,6 +891,7 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
 {
 	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct intel_dsb *dsb = intel_dsb_get(crtc);
 
 	if (crtc_state->base.degamma_lut)
 		glk_load_degamma_lut(crtc_state);
@@ -900,6 +910,9 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
 		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
 		ivb_load_lut_ext_max(crtc);
 	}
+
+	intel_dsb_commit(dsb);
+	intel_dsb_put(dsb);
 }
 
 static u32 chv_cgm_degamma_ldw(const struct drm_color_lut *color)
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 09/10] drm/i915/dsb: Enable DSB for gen12.

[Animesh Manna]

Enabling DSB by setting 1 to has_dsb flag for gen12.

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index b3cc8560696b..1fd2a364891a 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -787,7 +787,8 @@ static const struct intel_device_info intel_elkhartlake_info = {
 		[TRANSCODER_DSI_0] = TRANSCODER_DSI0_OFFSET, \
 		[TRANSCODER_DSI_1] = TRANSCODER_DSI1_OFFSET, \
 	}, \
-	.has_global_mocs = 1
+	.has_global_mocs = 1, \
+	.display.has_dsb = 1
 
 static const struct intel_device_info intel_tigerlake_12_info = {
 	GEN12_FEATURES,
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 10/10] drm/i915/dsb: Documentation for DSB.

[Animesh Manna]

Added docbook info regarding Display State Buffer(DSB) which
is added from gen12 onwards to batch submit display HW programming.

v1: Initial version as RFC.

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 Documentation/gpu/i915.rst               |  9 ++++
 drivers/gpu/drm/i915/display/intel_dsb.c | 68 ++++++++++++++++++++++++
 2 files changed, 77 insertions(+)

diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
index e249ea7b0ec7..465779670fd4 100644
--- a/Documentation/gpu/i915.rst
+++ b/Documentation/gpu/i915.rst
@@ -246,6 +246,15 @@ Display PLLs
 .. kernel-doc:: drivers/gpu/drm/i915/display/intel_dpll_mgr.h
    :internal:
 
+Display State Buffer
+--------------------
+
+.. kernel-doc:: drivers/gpu/drm/i915/display/intel_dsb.c
+   :doc: DSB
+
+.. kernel-doc:: drivers/gpu/drm/i915/display/intel_dsb.c
+   :internal:
+
 Memory Management and Command Submission
 ========================================
 
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index eea86afb0583..909fdb9935e9 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -9,6 +9,23 @@
 
 #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
 
+/**
+ * DOC: DSB
+ *
+ * A DSB (Display State Buffer) is a queue of MMIO instructions in the memory
+ * which can be offloaded to DSB HW in Display Controller. DSB HW is a DMA
+ * engine that can be programmed to download the DSB from memory.
+ * It allows driver to batch submit display HW programming. This helps to
+ * reduce loading time and CPU activity, thereby making the context switch
+ * faster. DSB Support added from Gen12 Intel graphics based platform.
+ *
+ * DSB's can access only the pipe, plane, and transcoder Data Island Packet
+ * registers.
+ *
+ * DSB HW can support only register writes (both indexed and direct MMIO
+ * writes). There are no registers reads possible with DSB HW engine.
+ */
+
 /* DSB opcodes. */
 #define DSB_OPCODE_SHIFT		24
 #define DSB_OPCODE_MMIO_WRITE		0x1
@@ -66,6 +83,17 @@ static inline bool intel_dsb_disable_engine(struct intel_dsb *dsb)
 	return true;
 }
 
+/**
+ * intel_dsb_get() - Allocate dsb context and return a dsb instance.
+ * @crtc: intel_crtc structure to get pipe info.
+ *
+ * This function will give handle of the DSB instance which
+ * user want to operate on.
+ *
+ * Return : address of Intel_dsb instance requested for.
+ * In failure case, the dsb instance will not have any command buffer.
+ */
+
 struct intel_dsb *
 intel_dsb_get(struct intel_crtc *crtc)
 {
@@ -109,6 +137,14 @@ intel_dsb_get(struct intel_crtc *crtc)
 	return dsb;
 }
 
+/**
+ * intel_dsb_put() - To destroy DSB context.
+ * @dsb: intel_dsb structure.
+ *
+ * This function is used to destroy the dsb-context by doing unpin
+ * and release the vma object.
+ */
+
 void intel_dsb_put(struct intel_dsb *dsb)
 {
 	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
@@ -127,6 +163,19 @@ void intel_dsb_put(struct intel_dsb *dsb)
 	}
 }
 
+/**
+ * intel_dsb_indexed_reg_write() -Write to the dsb context for auto
+ * increment register.
+ * @dsb: intel_dsb structure.
+ * @reg: register address.
+ * @val: value.
+ *
+ * This function is used for auto-increment register and intel_dsb_reg_write()
+ * is used for normal register. During command buffer overflow, a warning
+ * is thrown and rest all erroneous condition register programming is done
+ * through mmio write.
+ */
+
 void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
 				 u32 val)
 {
@@ -191,6 +240,18 @@ void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
 		buf[dsb->free_pos] = 0;
 }
 
+/**
+ * intel_dsb_reg_write() -Write to the dsb context for normal
+ * register.
+ * @dsb: intel_dsb structure.
+ * @reg: register address.
+ * @val: value.
+ *
+ * This function is used for writing register-value pair in command
+ * buffer of DSB. During command buffer overflow, a warning
+ * is thrown and rest all erroneous condition register programming is done
+ * through mmio write.
+ */
 void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
 {
 	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
@@ -213,6 +274,13 @@ void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
 			       i915_mmio_reg_offset(reg);
 }
 
+/**
+ * intel_dsb_commit() - Trigger workload execution of DSB.
+ * @dsb: intel_dsb structure.
+ *
+ * This function is used to do actual write to hardware using DSB.
+ * On errors, fall back to MMIO. Also this function help to reset the context.
+ */
 void intel_dsb_commit(struct intel_dsb *dsb)
 {
 	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
-- 
2.22.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for DSB enablement. (rev6)

[Patchwork]

== Series Details ==

Series: DSB enablement. (rev6)
URL   : https://patchwork.freedesktop.org/series/63013/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
9bb8863660ce drm/i915/dsb: feature flag added for display state buffer.
5d5acdf62c4f drm/i915/dsb: DSB context creation.
-:58: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#58: 
new file mode 100644

total: 0 errors, 1 warnings, 0 checks, 123 lines checked
0e680f2801a3 drm/i915/dsb: single register write function for DSB.
eb620a38bef7 drm/i915/dsb: Indexed register write function for DSB.
f4db38476b74 drm/i915/dsb: Check DSB engine status.
e864475a4151 drm/i915/dsb: functions to enable/disable DSB engine.
7e52dbe00344 drm/i915/dsb: function to trigger workload execution of DSB.
4b92d2df1a77 drm/i915/dsb: Enable gamma lut programming using DSB.
1dde13b7849d drm/i915/dsb: Enable DSB for gen12.
29b32060776d drm/i915/dsb: Documentation for DSB.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for DSB enablement. (rev6)

[Patchwork]

== Series Details ==

Series: DSB enablement. (rev6)
URL   : https://patchwork.freedesktop.org/series/63013/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.6.0
Commit: drm/i915/dsb: feature flag added for display state buffer.
Okay!

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for DSB enablement. (rev6)

[Patchwork]

== Series Details ==

Series: DSB enablement. (rev6)
URL   : https://patchwork.freedesktop.org/series/63013/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6874 -> Patchwork_14369
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/

Known issues
------------

  Here are the changes found in Patchwork_14369 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_flink_basic@basic:
    - fi-icl-u3:          [PASS][1] -> [DMESG-WARN][2] ([fdo#107724]) +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/fi-icl-u3/igt@gem_flink_basic@basic.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/fi-icl-u3/igt@gem_flink_basic@basic.html

  * igt@i915_module_load@reload:
    - fi-blb-e6850:       [PASS][3] -> [INCOMPLETE][4] ([fdo#107718])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/fi-blb-e6850/igt@i915_module_load@reload.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/fi-blb-e6850/igt@i915_module_load@reload.html

  * igt@kms_flip@basic-flip-vs-dpms:
    - fi-hsw-4770r:       [PASS][5] -> [DMESG-WARN][6] ([fdo#105602])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/fi-hsw-4770r/igt@kms_flip@basic-flip-vs-dpms.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/fi-hsw-4770r/igt@kms_flip@basic-flip-vs-dpms.html

  
#### Possible fixes ####

  * igt@gem_ctx_switch@legacy-render:
    - fi-bxt-dsi:         [INCOMPLETE][7] ([fdo#103927] / [fdo#111381]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/fi-bxt-dsi/igt@gem_ctx_switch@legacy-render.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/fi-bxt-dsi/igt@gem_ctx_switch@legacy-render.html

  * igt@gem_mmap_gtt@basic-short:
    - fi-icl-u3:          [DMESG-WARN][9] ([fdo#107724]) -> [PASS][10] +1 similar issue
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/fi-icl-u3/igt@gem_mmap_gtt@basic-short.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/fi-icl-u3/igt@gem_mmap_gtt@basic-short.html

  * igt@i915_selftest@live_execlists:
    - fi-skl-gvtdvm:      [DMESG-FAIL][11] ([fdo#111108]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/fi-skl-gvtdvm/igt@i915_selftest@live_execlists.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/fi-skl-gvtdvm/igt@i915_selftest@live_execlists.html

  * igt@i915_selftest@live_gem_contexts:
    - fi-cfl-guc:         [INCOMPLETE][13] ([fdo#111514]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/fi-cfl-guc/igt@i915_selftest@live_gem_contexts.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/fi-cfl-guc/igt@i915_selftest@live_gem_contexts.html

  * igt@i915_selftest@live_hangcheck:
    - {fi-icl-dsi}:       [INCOMPLETE][15] ([fdo#107713] / [fdo#108569]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/fi-icl-dsi/igt@i915_selftest@live_hangcheck.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/fi-icl-dsi/igt@i915_selftest@live_hangcheck.html

  
#### Warnings ####

  * igt@gem_exec_gttfill@basic:
    - fi-apl-guc:         [DMESG-WARN][17] ([fdo#111652 ]) -> [DMESG-WARN][18] ([fdo#109385] / [fdo#111652 ])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/fi-apl-guc/igt@gem_exec_gttfill@basic.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/fi-apl-guc/igt@gem_exec_gttfill@basic.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][19] ([fdo#111407]) -> [FAIL][20] ([fdo#111096])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109385]: https://bugs.freedesktop.org/show_bug.cgi?id=109385
  [fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
  [fdo#111108]: https://bugs.freedesktop.org/show_bug.cgi?id=111108
  [fdo#111381]: https://bugs.freedesktop.org/show_bug.cgi?id=111381
  [fdo#111407]: https://bugs.freedesktop.org/show_bug.cgi?id=111407
  [fdo#111514]: https://bugs.freedesktop.org/show_bug.cgi?id=111514
  [fdo#111652 ]: https://bugs.freedesktop.org/show_bug.cgi?id=111652 


Participating hosts (54 -> 47)
------------------------------

  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-y fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6874 -> Patchwork_14369

  CI-20190529: 20190529
  CI_DRM_6874: f9ee689ff55489da8db3cd5ddad533967c07561f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5178: efb4539494d94f03374874d3b61bd04ef3802aaa @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14369: 29b32060776de0b3d1924464affaf79f781a1271 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

29b32060776d drm/i915/dsb: Documentation for DSB.
1dde13b7849d drm/i915/dsb: Enable DSB for gen12.
4b92d2df1a77 drm/i915/dsb: Enable gamma lut programming using DSB.
7e52dbe00344 drm/i915/dsb: function to trigger workload execution of DSB.
e864475a4151 drm/i915/dsb: functions to enable/disable DSB engine.
f4db38476b74 drm/i915/dsb: Check DSB engine status.
eb620a38bef7 drm/i915/dsb: Indexed register write function for DSB.
0e680f2801a3 drm/i915/dsb: single register write function for DSB.
5d5acdf62c4f drm/i915/dsb: DSB context creation.
9bb8863660ce drm/i915/dsb: feature flag added for display state buffer.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for DSB enablement. (rev6)

[Patchwork]

== Series Details ==

Series: DSB enablement. (rev6)
URL   : https://patchwork.freedesktop.org/series/63013/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6874_full -> Patchwork_14369_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_14369_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_14369_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_14369_full:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_flip@modeset-vs-vblank-race:
    - shard-kbl:          [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-kbl6/igt@kms_flip@modeset-vs-vblank-race.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-kbl2/igt@kms_flip@modeset-vs-vblank-race.html

  

### Piglit changes ###

#### Possible regressions ####

  * spec@!opengl 1.1@copypixels-sync (NEW):
    - pig-hsw-4770r:      NOTRUN -> [FAIL][3]
   [3]: None

  
New tests
---------

  New tests have been introduced between CI_DRM_6874_full and Patchwork_14369_full:

### New Piglit tests (1) ###

  * spec@!opengl 1.1@copypixels-sync:
    - Statuses : 1 fail(s)
    - Exec time: [13.67] s

  

Known issues
------------

  Here are the changes found in Patchwork_14369_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_switch@legacy-render:
    - shard-apl:          [PASS][4] -> [INCOMPLETE][5] ([fdo#103927] / [fdo#111381])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-apl5/igt@gem_ctx_switch@legacy-render.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-apl8/igt@gem_ctx_switch@legacy-render.html

  * igt@gem_exec_schedule@independent-bsd2:
    - shard-iclb:         [PASS][6] -> [SKIP][7] ([fdo#109276]) +17 similar issues
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-iclb2/igt@gem_exec_schedule@independent-bsd2.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-iclb3/igt@gem_exec_schedule@independent-bsd2.html

  * igt@gem_exec_schedule@preempt-queue-bsd:
    - shard-iclb:         [PASS][8] -> [SKIP][9] ([fdo#111325]) +2 similar issues
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-iclb5/igt@gem_exec_schedule@preempt-queue-bsd.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-iclb4/igt@gem_exec_schedule@preempt-queue-bsd.html

  * igt@kms_concurrent@pipe-b:
    - shard-skl:          [PASS][10] -> [DMESG-WARN][11] ([fdo#106107])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-skl4/igt@kms_concurrent@pipe-b.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-skl4/igt@kms_concurrent@pipe-b.html

  * igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-untiled:
    - shard-snb:          [PASS][12] -> [SKIP][13] ([fdo#109271]) +1 similar issue
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-snb5/igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-untiled.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-snb6/igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-untiled.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          [PASS][14] -> [FAIL][15] ([fdo#105363])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-skl4/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-skl4/igt@kms_flip@flip-vs-expired-vblank-interruptible.html

  * igt@kms_flip@flip-vs-fences-interruptible:
    - shard-apl:          [PASS][16] -> [INCOMPLETE][17] ([fdo#103927])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-apl5/igt@kms_flip@flip-vs-fences-interruptible.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-apl4/igt@kms_flip@flip-vs-fences-interruptible.html

  * igt@kms_flip@modeset-vs-vblank-race:
    - shard-hsw:          [PASS][18] -> [DMESG-WARN][19] ([fdo#102614])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-hsw5/igt@kms_flip@modeset-vs-vblank-race.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-hsw5/igt@kms_flip@modeset-vs-vblank-race.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-apl:          [PASS][20] -> [DMESG-WARN][21] ([fdo#108566]) +3 similar issues
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-apl6/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-apl6/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-pwrite:
    - shard-iclb:         [PASS][22] -> [FAIL][23] ([fdo#103167]) +5 similar issues
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-pwrite.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-iclb6/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-rte:
    - shard-iclb:         [PASS][24] -> [FAIL][25] ([fdo#103167] / [fdo#110378])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-iclb6/igt@kms_frontbuffer_tracking@fbcpsr-1p-rte.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-iclb4/igt@kms_frontbuffer_tracking@fbcpsr-1p-rte.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [PASS][26] -> [FAIL][27] ([fdo#108145] / [fdo#110403])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-skl9/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-skl6/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-iclb:         [PASS][28] -> [FAIL][29] ([fdo#103166])
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-iclb8/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-iclb6/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@kms_psr@psr2_cursor_mmap_gtt:
    - shard-iclb:         [PASS][30] -> [SKIP][31] ([fdo#109441])
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-iclb2/igt@kms_psr@psr2_cursor_mmap_gtt.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-iclb3/igt@kms_psr@psr2_cursor_mmap_gtt.html

  
#### Possible fixes ####

  * igt@gem_exec_schedule@preempt-bsd:
    - shard-iclb:         [SKIP][32] ([fdo#111325]) -> [PASS][33] +3 similar issues
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-iclb4/igt@gem_exec_schedule@preempt-bsd.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-iclb6/igt@gem_exec_schedule@preempt-bsd.html

  * igt@kms_cursor_crc@pipe-b-cursor-suspend:
    - shard-apl:          [DMESG-WARN][34] ([fdo#108566]) -> [PASS][35] +2 similar issues
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-apl7/igt@kms_cursor_crc@pipe-b-cursor-suspend.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-apl1/igt@kms_cursor_crc@pipe-b-cursor-suspend.html

  * igt@kms_cursor_legacy@2x-long-flip-vs-cursor-legacy:
    - shard-glk:          [FAIL][36] ([fdo#104873]) -> [PASS][37]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-glk1/igt@kms_cursor_legacy@2x-long-flip-vs-cursor-legacy.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-glk8/igt@kms_cursor_legacy@2x-long-flip-vs-cursor-legacy.html

  * igt@kms_cursor_legacy@cursor-vs-flip-atomic:
    - shard-hsw:          [FAIL][38] ([fdo#103355]) -> [PASS][39]
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-hsw6/igt@kms_cursor_legacy@cursor-vs-flip-atomic.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-hsw5/igt@kms_cursor_legacy@cursor-vs-flip-atomic.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-mmap-cpu:
    - shard-iclb:         [FAIL][40] ([fdo#103167]) -> [PASS][41] +4 similar issues
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-mmap-cpu.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-mmap-cpu.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [FAIL][42] ([fdo#108145]) -> [PASS][43] +1 similar issue
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-skl7/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-skl1/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          [FAIL][44] ([fdo#108145] / [fdo#110403]) -> [PASS][45]
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-skl5/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-skl4/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html

  * igt@kms_psr2_su@frontbuffer:
    - shard-iclb:         [SKIP][46] ([fdo#109642] / [fdo#111068]) -> [PASS][47]
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-iclb7/igt@kms_psr2_su@frontbuffer.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-iclb2/igt@kms_psr2_su@frontbuffer.html

  * igt@kms_psr@psr2_suspend:
    - shard-iclb:         [SKIP][48] ([fdo#109441]) -> [PASS][49] +2 similar issues
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-iclb3/igt@kms_psr@psr2_suspend.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-iclb2/igt@kms_psr@psr2_suspend.html

  * igt@kms_setmode@basic:
    - shard-hsw:          [FAIL][50] ([fdo#99912]) -> [PASS][51]
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-hsw4/igt@kms_setmode@basic.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-hsw7/igt@kms_setmode@basic.html

  * igt@perf@blocking:
    - shard-skl:          [FAIL][52] ([fdo#110728]) -> [PASS][53]
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-skl4/igt@perf@blocking.html
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-skl4/igt@perf@blocking.html

  * igt@prime_busy@hang-bsd2:
    - shard-iclb:         [SKIP][54] ([fdo#109276]) -> [PASS][55] +12 similar issues
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-iclb3/igt@prime_busy@hang-bsd2.html
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-iclb2/igt@prime_busy@hang-bsd2.html

  * igt@prime_busy@wait-hang-blt:
    - shard-hsw:          [INCOMPLETE][56] ([fdo#103540]) -> [PASS][57]
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-hsw4/igt@prime_busy@wait-hang-blt.html
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-hsw4/igt@prime_busy@wait-hang-blt.html

  
#### Warnings ####

  * igt@gem_mocs_settings@mocs-isolation-bsd2:
    - shard-iclb:         [SKIP][58] ([fdo#109276]) -> [FAIL][59] ([fdo#111330])
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-iclb7/igt@gem_mocs_settings@mocs-isolation-bsd2.html
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-iclb1/igt@gem_mocs_settings@mocs-isolation-bsd2.html

  * igt@gem_mocs_settings@mocs-rc6-bsd2:
    - shard-iclb:         [FAIL][60] ([fdo#111330]) -> [SKIP][61] ([fdo#109276])
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-iclb4/igt@gem_mocs_settings@mocs-rc6-bsd2.html
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-iclb8/igt@gem_mocs_settings@mocs-rc6-bsd2.html

  * igt@i915_pm_rpm@modeset-pc8-residency-stress:
    - shard-iclb:         [SKIP][62] ([fdo#109293]) -> [SKIP][63] ([fdo#109293] / [fdo#109506])
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-iclb2/igt@i915_pm_rpm@modeset-pc8-residency-stress.html
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-iclb3/igt@i915_pm_rpm@modeset-pc8-residency-stress.html

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-pri-indfb-draw-mmap-gtt:
    - shard-hsw:          [FAIL][64] ([fdo#108040]) -> [SKIP][65] ([fdo#109271])
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-hsw1/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-pri-indfb-draw-mmap-gtt.html
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-hsw6/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-pri-indfb-draw-mmap-gtt.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes:
    - shard-apl:          [DMESG-WARN][66] ([fdo#108566]) -> [INCOMPLETE][67] ([fdo#103927])
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6874/shard-apl4/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/shard-apl5/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html

  
  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103355]: https://bugs.freedesktop.org/show_bug.cgi?id=103355
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104873]: https://bugs.freedesktop.org/show_bug.cgi?id=104873
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#106107]: https://bugs.freedesktop.org/show_bug.cgi?id=106107
  [fdo#108040]: https://bugs.freedesktop.org/show_bug.cgi?id=108040
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109293]: https://bugs.freedesktop.org/show_bug.cgi?id=109293
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109506]: https://bugs.freedesktop.org/show_bug.cgi?id=109506
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#110378]: https://bugs.freedesktop.org/show_bug.cgi?id=110378
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#110728]: https://bugs.freedesktop.org/show_bug.cgi?id=110728
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111325]: https://bugs.freedesktop.org/show_bug.cgi?id=111325
  [fdo#111330]: https://bugs.freedesktop.org/show_bug.cgi?id=111330
  [fdo#111381]: https://bugs.freedesktop.org/show_bug.cgi?id=111381
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6874 -> Patchwork_14369

  CI-20190529: 20190529
  CI_DRM_6874: f9ee689ff55489da8db3cd5ddad533967c07561f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5178: efb4539494d94f03374874d3b61bd04ef3802aaa @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14369: 29b32060776de0b3d1924464affaf79f781a1271 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14369/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 01/10] drm/i915/dsb: feature flag added for display state buffer.

[Sharma, Shashank]

On 9/12/2019 12:41 AM, Animesh Manna wrote:
> Display State Buffer(DSB) is a new hardware capability, introduced
> in GEN12 display. DSB allows a driver to batch-program display HW
> registers.
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h          | 2 ++
>   drivers/gpu/drm/i915/intel_device_info.h | 1 +
>   2 files changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2ea11123e933..6c6af007f29d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1863,6 +1863,8 @@ static inline struct drm_i915_private *pdev_to_i915(struct pci_dev *pdev)
>   	(BUILD_BUG_ON_ZERO(!__builtin_constant_p(n)) + \
>   	 INTEL_INFO(dev_priv)->gen == (n))
>   
> +#define HAS_DSB(dev_priv)	(INTEL_INFO(dev_priv)->display.has_dsb)
> +
>   /*
>    * Return true if revision is in range [since,until] inclusive.
>    *
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index 92e0c2e0954c..e206f298f48e 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -135,6 +135,7 @@ enum intel_ppgtt_type {
>   	func(has_csr); \
>   	func(has_ddi); \
>   	func(has_dp_mst); \
> +	func(has_dsb); \
>   	func(has_fbc); \
>   	func(has_gmch); \
>   	func(has_hotplug); \

Looks good to me,

Feel free to use: Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

- Shashank

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 02/10] drm/i915/dsb: DSB context creation.

[Sharma, Shashank]

On 9/12/2019 12:41 AM, Animesh Manna wrote:
> This patch adds a function, which will internally get the gem buffer
> for DSB engine. The GEM buffer is from global GTT, and is mapped into
> CPU domain, contains the data + opcode to be feed to DSB engine.
>
> v1: Initial version.
>
> v2:
> - removed some unwanted code. (Chris)
> - Used i915_gem_object_create_internal instead of _shmem. (Chris)
> - cmd_buf_tail removed and can be derived through vma object. (Chris)
>
> v3: vma realeased if i915_gem_object_pin_map() failed. (Shashank)
>
> v4: for simplification and based on current usage added single dsb
> object in intel_crtc. (Shashank)
>
> v5: seting NULL to cmd_buf moved outside of mutex in dsb-put(). (Shashank)
>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>   drivers/gpu/drm/i915/Makefile                 |  1 +
>   .../drm/i915/display/intel_display_types.h    |  3 +
>   drivers/gpu/drm/i915/display/intel_dsb.c      | 70 +++++++++++++++++++
>   drivers/gpu/drm/i915/display/intel_dsb.h      | 30 ++++++++
>   drivers/gpu/drm/i915/i915_drv.h               |  1 +
>   5 files changed, 105 insertions(+)
>   create mode 100644 drivers/gpu/drm/i915/display/intel_dsb.c
>   create mode 100644 drivers/gpu/drm/i915/display/intel_dsb.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 658b930d34a8..6313e7b4bd78 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -172,6 +172,7 @@ i915-y += \
>   	display/intel_display_power.o \
>   	display/intel_dpio_phy.o \
>   	display/intel_dpll_mgr.o \
> +	display/intel_dsb.o \
>   	display/intel_fbc.o \
>   	display/intel_fifo_underrun.o \
>   	display/intel_frontbuffer.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index d5cc4b810d9e..49c902b00484 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1033,6 +1033,9 @@ struct intel_crtc {
>   
>   	/* scalers available on this crtc */
>   	int num_scalers;
> +
> +	/* per pipe DSB related info */
> +	struct intel_dsb dsb;
>   };
>   
>   struct intel_plane {
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> new file mode 100644
> index 000000000000..7c1b1574788c
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2019 Intel Corporation
> + *
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_display_types.h"
> +
> +#define DSB_BUF_SIZE    (2 * PAGE_SIZE)
> +
> +struct intel_dsb *
> +intel_dsb_get(struct intel_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *i915 = to_i915(dev);
> +	struct drm_i915_gem_object *obj;
> +	struct i915_vma *vma;
> +	struct intel_dsb *dsb = &crtc->dsb;
Minor nitpick, if we move this line above next to i915, all the 
initialized variables will be together.
> +	intel_wakeref_t wakeref;
> +
> +	if ((!HAS_DSB(i915)) || dsb->cmd_buf)
> +		return dsb;
> +
> +	dsb->id = DSB1;
> +	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> +
> +	obj = i915_gem_object_create_internal(i915, DSB_BUF_SIZE);
> +	if (IS_ERR(obj))
This is also an error condition, deserves a DRM_ERROR() too.
> +		goto err;
> +
> +	mutex_lock(&i915->drm.struct_mutex);
> +	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, PIN_MAPPABLE);
> +	mutex_unlock(&i915->drm.struct_mutex);
> +	if (IS_ERR(vma)) {
> +		DRM_ERROR("Vma creation failed\n");
> +		i915_gem_object_put(obj);
> +		goto err;
> +	}
> +
> +	dsb->cmd_buf = i915_gem_object_pin_map(vma->obj, I915_MAP_WC);
> +	if (IS_ERR(dsb->cmd_buf)) {
> +		DRM_ERROR("Command buffer creation failed\n");
> +		i915_vma_unpin_and_release(&vma, 0);
> +		dsb->cmd_buf = NULL;
> +		goto err;
> +	}
> +	dsb->vma = vma;
> +
> +err:
> +	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> +	return dsb;
> +}
> +
> +void intel_dsb_put(struct intel_dsb *dsb)
> +{
> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> +
> +	if (!dsb)
> +		return;
> +
> +	if (dsb->cmd_buf) {
> +		mutex_lock(&i915->drm.struct_mutex);
> +		i915_gem_object_unpin_map(dsb->vma->obj);
> +		i915_vma_unpin_and_release(&dsb->vma, 0);
> +		mutex_unlock(&i915->drm.struct_mutex);
> +		dsb->cmd_buf = NULL;
> +	}
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> new file mode 100644
> index 000000000000..27eb68eb5392
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: MIT
> + *
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef _INTEL_DSB_H
> +#define _INTEL_DSB_H
> +
> +struct intel_crtc;
> +struct i915_vma;
> +
> +enum dsb_id {
> +	INVALID_DSB = -1,
> +	DSB1,
> +	DSB2,
> +	DSB3,
> +	MAX_DSB_PER_PIPE
> +};
> +
> +struct intel_dsb {
> +	enum dsb_id id;
> +	u32 *cmd_buf;
> +	struct i915_vma *vma;
> +};
> +
> +struct intel_dsb *
> +intel_dsb_get(struct intel_crtc *crtc);
> +void intel_dsb_put(struct intel_dsb *dsb);
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6c6af007f29d..069a06edb5c6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -67,6 +67,7 @@
>   #include "display/intel_display.h"
>   #include "display/intel_display_power.h"
>   #include "display/intel_dpll_mgr.h"
> +#include "display/intel_dsb.h"
>   #include "display/intel_frontbuffer.h"
>   #include "display/intel_gmbus.h"
>   #include "display/intel_opregion.h"

With the minor comments fixed, please feel free to use:

Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

- Shashank

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 02/10] drm/i915/dsb: DSB context creation.

[Jani Nikula]

On Thu, 12 Sep 2019, Animesh Manna <animesh.manna@intel.com> wrote:
> This patch adds a function, which will internally get the gem buffer
> for DSB engine. The GEM buffer is from global GTT, and is mapped into
> CPU domain, contains the data + opcode to be feed to DSB engine.
>
> v1: Initial version.
>
> v2:
> - removed some unwanted code. (Chris)
> - Used i915_gem_object_create_internal instead of _shmem. (Chris)
> - cmd_buf_tail removed and can be derived through vma object. (Chris)
>
> v3: vma realeased if i915_gem_object_pin_map() failed. (Shashank)
>
> v4: for simplification and based on current usage added single dsb
> object in intel_crtc. (Shashank)
>
> v5: seting NULL to cmd_buf moved outside of mutex in dsb-put(). (Shashank)
>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile                 |  1 +
>  .../drm/i915/display/intel_display_types.h    |  3 +
>  drivers/gpu/drm/i915/display/intel_dsb.c      | 70 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dsb.h      | 30 ++++++++
>  drivers/gpu/drm/i915/i915_drv.h               |  1 +
>  5 files changed, 105 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_dsb.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_dsb.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 658b930d34a8..6313e7b4bd78 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -172,6 +172,7 @@ i915-y += \
>  	display/intel_display_power.o \
>  	display/intel_dpio_phy.o \
>  	display/intel_dpll_mgr.o \
> +	display/intel_dsb.o \
>  	display/intel_fbc.o \
>  	display/intel_fifo_underrun.o \
>  	display/intel_frontbuffer.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index d5cc4b810d9e..49c902b00484 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1033,6 +1033,9 @@ struct intel_crtc {
>  
>  	/* scalers available on this crtc */
>  	int num_scalers;
> +
> +	/* per pipe DSB related info */
> +	struct intel_dsb dsb;
>  };
>  
>  struct intel_plane {
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> new file mode 100644
> index 000000000000..7c1b1574788c
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2019 Intel Corporation
> + *
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_display_types.h"
> +
> +#define DSB_BUF_SIZE    (2 * PAGE_SIZE)
> +
> +struct intel_dsb *
> +intel_dsb_get(struct intel_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *i915 = to_i915(dev);
> +	struct drm_i915_gem_object *obj;
> +	struct i915_vma *vma;
> +	struct intel_dsb *dsb = &crtc->dsb;
> +	intel_wakeref_t wakeref;
> +
> +	if ((!HAS_DSB(i915)) || dsb->cmd_buf)
> +		return dsb;

This might work now, but this is racy.

> +
> +	dsb->id = DSB1;
> +	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> +
> +	obj = i915_gem_object_create_internal(i915, DSB_BUF_SIZE);
> +	if (IS_ERR(obj))
> +		goto err;
> +
> +	mutex_lock(&i915->drm.struct_mutex);
> +	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, PIN_MAPPABLE);
> +	mutex_unlock(&i915->drm.struct_mutex);
> +	if (IS_ERR(vma)) {
> +		DRM_ERROR("Vma creation failed\n");
> +		i915_gem_object_put(obj);
> +		goto err;
> +	}
> +
> +	dsb->cmd_buf = i915_gem_object_pin_map(vma->obj, I915_MAP_WC);
> +	if (IS_ERR(dsb->cmd_buf)) {
> +		DRM_ERROR("Command buffer creation failed\n");
> +		i915_vma_unpin_and_release(&vma, 0);
> +		dsb->cmd_buf = NULL;
> +		goto err;
> +	}
> +	dsb->vma = vma;
> +
> +err:
> +	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> +	return dsb;
> +}
> +
> +void intel_dsb_put(struct intel_dsb *dsb)
> +{
> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> +
> +	if (!dsb)
> +		return;

If dsb == NULL, the above dereference probably oopses before this check.

BR,
Jani.

> +
> +	if (dsb->cmd_buf) {
> +		mutex_lock(&i915->drm.struct_mutex);
> +		i915_gem_object_unpin_map(dsb->vma->obj);
> +		i915_vma_unpin_and_release(&dsb->vma, 0);
> +		mutex_unlock(&i915->drm.struct_mutex);
> +		dsb->cmd_buf = NULL;
> +	}
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> new file mode 100644
> index 000000000000..27eb68eb5392
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: MIT
> + *
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef _INTEL_DSB_H
> +#define _INTEL_DSB_H
> +
> +struct intel_crtc;
> +struct i915_vma;
> +
> +enum dsb_id {
> +	INVALID_DSB = -1,
> +	DSB1,
> +	DSB2,
> +	DSB3,
> +	MAX_DSB_PER_PIPE
> +};
> +
> +struct intel_dsb {
> +	enum dsb_id id;
> +	u32 *cmd_buf;
> +	struct i915_vma *vma;
> +};
> +
> +struct intel_dsb *
> +intel_dsb_get(struct intel_crtc *crtc);
> +void intel_dsb_put(struct intel_dsb *dsb);
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6c6af007f29d..069a06edb5c6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -67,6 +67,7 @@
>  #include "display/intel_display.h"
>  #include "display/intel_display_power.h"
>  #include "display/intel_dpll_mgr.h"
> +#include "display/intel_dsb.h"
>  #include "display/intel_frontbuffer.h"
>  #include "display/intel_gmbus.h"
>  #include "display/intel_opregion.h"

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 03/10] drm/i915/dsb: single register write function for DSB.

[Sharma, Shashank]

On 9/12/2019 12:41 AM, Animesh Manna wrote:
> DSB support single register write through opcode 0x1. Generic
> api created which accumulate all single register write in a batch
> buffer and once DSB is triggered, it will program all the registers
> at the same time.
>
> v1: Initial version.
> v2: Unused macro removed and cosmetic changes done. (Shashank)
> v3: set free_pos to zero in dsb-put() instead dsb-get() and
> a cosmetic change. (Shashank)
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dsb.c | 30 ++++++++++++++++++++++++
>   drivers/gpu/drm/i915/display/intel_dsb.h |  9 +++++++
>   2 files changed, 39 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 7c1b1574788c..e2c383352145 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -9,6 +9,13 @@
>   
>   #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
>   
> +/* DSB opcodes. */
> +#define DSB_OPCODE_SHIFT		24
> +#define DSB_OPCODE_MMIO_WRITE		0x1
> +#define DSB_OPCODE_INDEXED_WRITE	0x9
We are not using this macro here, this should go to the Batch 
/INDEXED_WRITE patch.
> +#define DSB_BYTE_EN			0xF
> +#define DSB_BYTE_EN_SHIFT		20
> +
>   struct intel_dsb *
>   intel_dsb_get(struct intel_crtc *crtc)
>   {
> @@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb)
>   		i915_vma_unpin_and_release(&dsb->vma, 0);
>   		mutex_unlock(&i915->drm.struct_mutex);
>   		dsb->cmd_buf = NULL;
> +		dsb->free_pos = 0;
> +	}
> +}
> +
I hope this addition of braces are due to diff's adjustment.
> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
> +{
> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 *buf = dsb->cmd_buf;
> +
> +	if (!buf) {
> +		I915_WRITE(reg, val);
> +		return;
> +	}
> +
> +	if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
> +		DRM_DEBUG_KMS("DSB buffer overflow\n");
why shouldn't we do a I915_WRITE(reg, val) here too? This is single 
register write, and we can handle this.
> +		return;
>   	}
> +
> +	buf[dsb->free_pos++] = val;
> +	buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE  << DSB_OPCODE_SHIFT) |
> +			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
> +			       i915_mmio_reg_offset(reg);
>   }
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> index 27eb68eb5392..31b87dcfe160 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -6,6 +6,8 @@
>   #ifndef _INTEL_DSB_H
>   #define _INTEL_DSB_H
>   
> +#include "i915_reg.h"
> +
>   struct intel_crtc;
>   struct i915_vma;
>   
> @@ -21,10 +23,17 @@ struct intel_dsb {
>   	enum dsb_id id;
>   	u32 *cmd_buf;
>   	struct i915_vma *vma;
> +
> +	/*
> +	 * free_pos will point the first free entry position
> +	 * and help in calculating tail of command buffer.
> +	 */
> +	int free_pos;
>   };
>   
>   struct intel_dsb *
>   intel_dsb_get(struct intel_crtc *crtc);
>   void intel_dsb_put(struct intel_dsb *dsb);
> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
>   
>   #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 03/10] drm/i915/dsb: single register write function for DSB.

[Jani Nikula]

On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> wrote:
> On 9/12/2019 12:41 AM, Animesh Manna wrote:
>> DSB support single register write through opcode 0x1. Generic
>> api created which accumulate all single register write in a batch
>> buffer and once DSB is triggered, it will program all the registers
>> at the same time.
>>
>> v1: Initial version.
>> v2: Unused macro removed and cosmetic changes done. (Shashank)
>> v3: set free_pos to zero in dsb-put() instead dsb-get() and
>> a cosmetic change. (Shashank)
>>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dsb.c | 30 ++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_dsb.h |  9 +++++++
>>   2 files changed, 39 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
>> index 7c1b1574788c..e2c383352145 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>> @@ -9,6 +9,13 @@
>>   
>>   #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
>>   
>> +/* DSB opcodes. */
>> +#define DSB_OPCODE_SHIFT		24
>> +#define DSB_OPCODE_MMIO_WRITE		0x1
>> +#define DSB_OPCODE_INDEXED_WRITE	0x9
> We are not using this macro here, this should go to the Batch 
> /INDEXED_WRITE patch.
>> +#define DSB_BYTE_EN			0xF
>> +#define DSB_BYTE_EN_SHIFT		20
>> +
>>   struct intel_dsb *
>>   intel_dsb_get(struct intel_crtc *crtc)
>>   {
>> @@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb)
>>   		i915_vma_unpin_and_release(&dsb->vma, 0);
>>   		mutex_unlock(&i915->drm.struct_mutex);
>>   		dsb->cmd_buf = NULL;
>> +		dsb->free_pos = 0;
>> +	}
>> +}
>> +
> I hope this addition of braces are due to diff's adjustment.
>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>> +{
>> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +	u32 *buf = dsb->cmd_buf;
>> +
>> +	if (!buf) {
>> +		I915_WRITE(reg, val);
>> +		return;
>> +	}
>> +
>> +	if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
>> +		DRM_DEBUG_KMS("DSB buffer overflow\n");
> why shouldn't we do a I915_WRITE(reg, val) here too? This is single 
> register write, and we can handle this.

That would assume it's okay to directly mmio write this and the
subsequent values, and write the batch already stored in the buffer
afterwards.

BR,
Jani.

>> +		return;
>>   	}
>> +
>> +	buf[dsb->free_pos++] = val;
>> +	buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE  << DSB_OPCODE_SHIFT) |
>> +			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>> +			       i915_mmio_reg_offset(reg);
>>   }
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
>> index 27eb68eb5392..31b87dcfe160 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>> @@ -6,6 +6,8 @@
>>   #ifndef _INTEL_DSB_H
>>   #define _INTEL_DSB_H
>>   
>> +#include "i915_reg.h"
>> +
>>   struct intel_crtc;
>>   struct i915_vma;
>>   
>> @@ -21,10 +23,17 @@ struct intel_dsb {
>>   	enum dsb_id id;
>>   	u32 *cmd_buf;
>>   	struct i915_vma *vma;
>> +
>> +	/*
>> +	 * free_pos will point the first free entry position
>> +	 * and help in calculating tail of command buffer.
>> +	 */
>> +	int free_pos;
>>   };
>>   
>>   struct intel_dsb *
>>   intel_dsb_get(struct intel_crtc *crtc);
>>   void intel_dsb_put(struct intel_dsb *dsb);
>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
>>   
>>   #endif

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 03/10] drm/i915/dsb: single register write function for DSB.

[Sharma, Shashank]

On 9/12/2019 6:21 PM, Jani Nikula wrote:
> On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> wrote:
>> On 9/12/2019 12:41 AM, Animesh Manna wrote:
>>> DSB support single register write through opcode 0x1. Generic
>>> api created which accumulate all single register write in a batch
>>> buffer and once DSB is triggered, it will program all the registers
>>> at the same time.
>>>
>>> v1: Initial version.
>>> v2: Unused macro removed and cosmetic changes done. (Shashank)
>>> v3: set free_pos to zero in dsb-put() instead dsb-get() and
>>> a cosmetic change. (Shashank)
>>>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/display/intel_dsb.c | 30 ++++++++++++++++++++++++
>>>    drivers/gpu/drm/i915/display/intel_dsb.h |  9 +++++++
>>>    2 files changed, 39 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
>>> index 7c1b1574788c..e2c383352145 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>>> @@ -9,6 +9,13 @@
>>>    
>>>    #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
>>>    
>>> +/* DSB opcodes. */
>>> +#define DSB_OPCODE_SHIFT		24
>>> +#define DSB_OPCODE_MMIO_WRITE		0x1
>>> +#define DSB_OPCODE_INDEXED_WRITE	0x9
>> We are not using this macro here, this should go to the Batch
>> /INDEXED_WRITE patch.
>>> +#define DSB_BYTE_EN			0xF
>>> +#define DSB_BYTE_EN_SHIFT		20
>>> +
>>>    struct intel_dsb *
>>>    intel_dsb_get(struct intel_crtc *crtc)
>>>    {
>>> @@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb)
>>>    		i915_vma_unpin_and_release(&dsb->vma, 0);
>>>    		mutex_unlock(&i915->drm.struct_mutex);
>>>    		dsb->cmd_buf = NULL;
>>> +		dsb->free_pos = 0;
>>> +	}
>>> +}
>>> +
>> I hope this addition of braces are due to diff's adjustment.
>>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>>> +{
>>> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>> +	u32 *buf = dsb->cmd_buf;
>>> +
>>> +	if (!buf) {
>>> +		I915_WRITE(reg, val);
>>> +		return;
>>> +	}
>>> +
>>> +	if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
>>> +		DRM_DEBUG_KMS("DSB buffer overflow\n");
>> why shouldn't we do a I915_WRITE(reg, val) here too? This is single
>> register write, and we can handle this.
> That would assume it's okay to directly mmio write this and the
> subsequent values, and write the batch already stored in the buffer
> afterwards.

This is single write API, so I won't expect this to be called in an 
indexed context, also, we have exceeded the buffer size already, so no 
subsequent DSB write would be possible anyways. But I can still see it 
would be some kind of mess only, so doesn't really matter if we do this 
I915_write or not :).

- Shashank

> BR,
> Jani.
>
>>> +		return;
>>>    	}
>>> +
>>> +	buf[dsb->free_pos++] = val;
>>> +	buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE  << DSB_OPCODE_SHIFT) |
>>> +			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>>> +			       i915_mmio_reg_offset(reg);
>>>    }
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
>>> index 27eb68eb5392..31b87dcfe160 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>>> @@ -6,6 +6,8 @@
>>>    #ifndef _INTEL_DSB_H
>>>    #define _INTEL_DSB_H
>>>    
>>> +#include "i915_reg.h"
>>> +
>>>    struct intel_crtc;
>>>    struct i915_vma;
>>>    
>>> @@ -21,10 +23,17 @@ struct intel_dsb {
>>>    	enum dsb_id id;
>>>    	u32 *cmd_buf;
>>>    	struct i915_vma *vma;
>>> +
>>> +	/*
>>> +	 * free_pos will point the first free entry position
>>> +	 * and help in calculating tail of command buffer.
>>> +	 */
>>> +	int free_pos;
>>>    };
>>>    
>>>    struct intel_dsb *
>>>    intel_dsb_get(struct intel_crtc *crtc);
>>>    void intel_dsb_put(struct intel_dsb *dsb);
>>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
>>>    
>>>    #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 08/10] drm/i915/dsb: Enable gamma lut programming using DSB.

[Jani Nikula]

On Thu, 12 Sep 2019, Animesh Manna <animesh.manna@intel.com> wrote:
> Gamma lut programming can be programmed using DSB
> where bulk register programming can be done using indexed
> register write which takes number of data and the mmio offset
> to be written.
>
> Currently enabled for 12-bit gamma LUT which is enabled by
> default and later 8-bit/10-bit will be enabled in future
> based on need.
>
> v1: Initial version.
> v2: Directly call dsb-api at callsites. (Jani)
> v3:
> - modified the code as per single dsb instance per crtc. (Shashank)
> - Added dsb get/put call in platform specific load_lut hook. (Jani)
> - removed dsb pointer from dev_priv. (Jani)
> v4: simplified code by dropping ref-count implementation. (Shashank)
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 57 +++++++++++++---------
>  1 file changed, 35 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 318308dc136c..b6b9f0e5166b 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -611,12 +611,13 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
>  static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct intel_dsb *dsb = intel_dsb_get(crtc);

When you've done enough kernel programming, you learn to expect get/put
to be paired, and assume it's a bug if this isn't the case.

So yeah, I did say it's okay not to have refcounting at first, *but* the
expectation that get/put go in pairs is so deeply ingrained that I
didn't even think to say you shouldn't have this kind of asymmetry.

So this creates a problem, how do we pass the dsb pointer here, as
without refcounting you can't actually have the put here as well,
because you throw the stuff out before the commit.

Maybe the easy answer is that we should just do the get and put at
intel_crtc_init and intel_crtc_destroy. Or we could do it at atomic
check and free.

Ville, which approach would conflict with your future vblank worker
stuff the least?


BR,
Jani.



>  	enum pipe pipe = crtc->pipe;
>  
>  	/* Program the max register to clamp values > 1.0. */
> -	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
> -	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
> -	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
> +	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
> +	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
> +	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
>  
>  	/*
>  	 * Program the gc max 2 register to clamp values > 1.0.
> @@ -624,9 +625,12 @@ static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
>  	 * from 3.0 to 7.0
>  	 */
>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
> -		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16);
> -		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16);
> -		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16);
> +		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 0),
> +				    1 << 16);
> +		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 1),
> +				    1 << 16);
> +		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 2),
> +				    1 << 16);
>  	}
>  }
>  
> @@ -787,22 +791,22 @@ icl_load_gcmax(const struct intel_crtc_state *crtc_state,
>  	       const struct drm_color_lut *color)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>  	enum pipe pipe = crtc->pipe;
>  
>  	/* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */
> -	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), color->red);
> -	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), color->green);
> -	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), color->blue);
> +	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 0), color->red);
> +	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 1), color->green);
> +	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 2), color->blue);
>  }
>  
>  static void
>  icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>  	const struct drm_color_lut *lut = blob->data;
> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>  	enum pipe pipe = crtc->pipe;
>  	u32 i;
>  
> @@ -813,15 +817,16 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>  	 * Superfine segment has 9 entries, corresponding to values
>  	 * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
>  	 */
> -	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
> +	intel_dsb_reg_write(dsb, PREC_PAL_MULTI_SEG_INDEX(pipe),
> +			    PAL_PREC_AUTO_INCREMENT);
>  
>  	for (i = 0; i < 9; i++) {
>  		const struct drm_color_lut *entry = &lut[i];
>  
> -		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
> -			   ilk_lut_12p4_ldw(entry));
> -		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
> -			   ilk_lut_12p4_udw(entry));
> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe),
> +					    ilk_lut_12p4_ldw(entry));
> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe),
> +					    ilk_lut_12p4_udw(entry));
>  	}
>  }
>  
> @@ -829,10 +834,10 @@ static void
>  icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>  	const struct drm_color_lut *lut = blob->data;
>  	const struct drm_color_lut *entry;
> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>  	enum pipe pipe = crtc->pipe;
>  	u32 i;
>  
> @@ -847,11 +852,13 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>  	 * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
>  	 * with seg2[0] being unused by the hardware.
>  	 */
> -	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
> +	intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>  	for (i = 1; i < 257; i++) {
>  		entry = &lut[i * 8];
> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
> +					    ilk_lut_12p4_ldw(entry));
> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
> +					    ilk_lut_12p4_udw(entry));
>  	}
>  
>  	/*
> @@ -868,8 +875,10 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>  	 */
>  	for (i = 0; i < 256; i++) {
>  		entry = &lut[i * 8 * 128];
> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
> +					    ilk_lut_12p4_ldw(entry));
> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
> +					    ilk_lut_12p4_udw(entry));
>  	}
>  
>  	/* The last entry in the LUT is to be programmed in GCMAX */
> @@ -882,6 +891,7 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>  {
>  	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>  
>  	if (crtc_state->base.degamma_lut)
>  		glk_load_degamma_lut(crtc_state);
> @@ -900,6 +910,9 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>  		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
>  		ivb_load_lut_ext_max(crtc);
>  	}
> +
> +	intel_dsb_commit(dsb);
> +	intel_dsb_put(dsb);
>  }
>  
>  static u32 chv_cgm_degamma_ldw(const struct drm_color_lut *color)

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 03/10] drm/i915/dsb: single register write function for DSB.

[Animesh Manna]

On 9/12/2019 6:30 PM, Sharma, Shashank wrote:
>
> On 9/12/2019 6:21 PM, Jani Nikula wrote:
>> On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> 
>> wrote:
>>> On 9/12/2019 12:41 AM, Animesh Manna wrote:
>>>> DSB support single register write through opcode 0x1. Generic
>>>> api created which accumulate all single register write in a batch
>>>> buffer and once DSB is triggered, it will program all the registers
>>>> at the same time.
>>>>
>>>> v1: Initial version.
>>>> v2: Unused macro removed and cosmetic changes done. (Shashank)
>>>> v3: set free_pos to zero in dsb-put() instead dsb-get() and
>>>> a cosmetic change. (Shashank)
>>>>
>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/display/intel_dsb.c | 30 
>>>> ++++++++++++++++++++++++
>>>>    drivers/gpu/drm/i915/display/intel_dsb.h |  9 +++++++
>>>>    2 files changed, 39 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c 
>>>> b/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> index 7c1b1574788c..e2c383352145 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> @@ -9,6 +9,13 @@
>>>>       #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
>>>>    +/* DSB opcodes. */
>>>> +#define DSB_OPCODE_SHIFT        24
>>>> +#define DSB_OPCODE_MMIO_WRITE        0x1
>>>> +#define DSB_OPCODE_INDEXED_WRITE    0x9
>>> We are not using this macro here, this should go to the Batch
>>> /INDEXED_WRITE patch.
>>>> +#define DSB_BYTE_EN            0xF
>>>> +#define DSB_BYTE_EN_SHIFT        20
>>>> +
>>>>    struct intel_dsb *
>>>>    intel_dsb_get(struct intel_crtc *crtc)
>>>>    {
>>>> @@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb)
>>>>            i915_vma_unpin_and_release(&dsb->vma, 0);
>>>>            mutex_unlock(&i915->drm.struct_mutex);
>>>>            dsb->cmd_buf = NULL;
>>>> +        dsb->free_pos = 0;
>>>> +    }
>>>> +}
>>>> +
>>> I hope this addition of braces are due to diff's adjustment.
>>>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, 
>>>> u32 val)
>>>> +{
>>>> +    struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
>>>> +    struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>> +    u32 *buf = dsb->cmd_buf;
>>>> +
>>>> +    if (!buf) {
>>>> +        I915_WRITE(reg, val);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
>>>> +        DRM_DEBUG_KMS("DSB buffer overflow\n");
>>> why shouldn't we do a I915_WRITE(reg, val) here too? This is single
>>> register write, and we can handle this.
>> That would assume it's okay to directly mmio write this and the
>> subsequent values, and write the batch already stored in the buffer
>> afterwards.
>
> This is single write API, so I won't expect this to be called in an 
> indexed context, also, we have exceeded the buffer size already, so no 
> subsequent DSB write would be possible anyways. But I can still see it 
> would be some kind of mess only, so doesn't really matter if we do 
> this I915_write or not :).

Adding I915_WRITE can be done, but I feel better to break the 
functionality where we have buffer overflow and based on that we can 
fine tune the buffer size.
If a set of register is targetted to write through DSB then some writing 
through MMIO and and rest writing though DSB may not a nice thing.
So added only debug log to capture the issue.

Regards,
Animesh
>
> - Shashank
>
>> BR,
>> Jani.
>>
>>>> +        return;
>>>>        }
>>>> +
>>>> +    buf[dsb->free_pos++] = val;
>>>> +    buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE << 
>>>> DSB_OPCODE_SHIFT) |
>>>> +                   (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>>>> +                   i915_mmio_reg_offset(reg);
>>>>    }
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h 
>>>> b/drivers/gpu/drm/i915/display/intel_dsb.h
>>>> index 27eb68eb5392..31b87dcfe160 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>>>> @@ -6,6 +6,8 @@
>>>>    #ifndef _INTEL_DSB_H
>>>>    #define _INTEL_DSB_H
>>>>    +#include "i915_reg.h"
>>>> +
>>>>    struct intel_crtc;
>>>>    struct i915_vma;
>>>>    @@ -21,10 +23,17 @@ struct intel_dsb {
>>>>        enum dsb_id id;
>>>>        u32 *cmd_buf;
>>>>        struct i915_vma *vma;
>>>> +
>>>> +    /*
>>>> +     * free_pos will point the first free entry position
>>>> +     * and help in calculating tail of command buffer.
>>>> +     */
>>>> +    int free_pos;
>>>>    };
>>>>       struct intel_dsb *
>>>>    intel_dsb_get(struct intel_crtc *crtc);
>>>>    void intel_dsb_put(struct intel_dsb *dsb);
>>>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, 
>>>> u32 val);
>>>>       #endif

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 04/10] drm/i915/dsb: Indexed register write function for DSB.

[Sharma, Shashank]

On 9/12/2019 12:41 AM, Animesh Manna wrote:
> DSB can program large set of data through indexed register write
> (opcode 0x9) in one shot. DSB feature can be used for bulk register
> programming e.g. gamma lut programming, HDR meta data programming.
>
> v1: initial version.
> v2: simplified code by using ALIGN(). (Chris)
> v3: ascii table added as code comment. (Shashank)
> v4: cosmetic changes done. (Shashank)
>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dsb.c | 65 ++++++++++++++++++++++++
>   drivers/gpu/drm/i915/display/intel_dsb.h |  8 +++
>   2 files changed, 73 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index e2c383352145..9e2927f869b9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -15,6 +15,7 @@
>   #define DSB_OPCODE_INDEXED_WRITE	0x9
>   #define DSB_BYTE_EN			0xF
>   #define DSB_BYTE_EN_SHIFT		20
> +#define DSB_REG_VALUE_MASK		0xfffff
>   
>   struct intel_dsb *
>   intel_dsb_get(struct intel_crtc *crtc)
> @@ -77,6 +78,70 @@ void intel_dsb_put(struct intel_dsb *dsb)
>   	}
>   }
>   
> +void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
> +				 u32 val)
> +{
> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 *buf = dsb->cmd_buf;
> +	u32 reg_val;
> +
> +	if (!buf) {
> +		I915_WRITE(reg, val);
> +		return;
> +	}
> +
> +	if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
> +		DRM_DEBUG_KMS("DSB buffer overflow\n");
> +		return;
> +	}
> +
> +	/*
> +	 * For example the buffer will look like below for 3 dwords for auto
> +	 * increment register:
> +	 * +--------------------------------------------------------+
> +	 * | size = 3 | offset &| value1 | value2 | value3 | zero   |
> +	 * |          | opcode  |        |        |        |        |
> +	 * +--------------------------------------------------------+
> +	 * +          +         +        +        +        +        +
> +	 * 0          4         8        12       16       20       24
> +	 * Byte
> +	 *
> +	 * As every instruction is 8 byte aligned the index of dsb instruction
> +	 * will start always from even number while dealing with u32 array. If
> +	 * we are writing odd no of dwords, Zeros will be added in the end for
> +	 * padding.
> +	 */
> +	reg_val = buf[dsb->ins_start_offset + 1] & DSB_REG_VALUE_MASK;
> +	if (reg_val != i915_mmio_reg_offset(reg)) {
> +		/* Every instruction should be 8 byte aligned. */
> +		dsb->free_pos = ALIGN(dsb->free_pos, 2);
> +
> +		dsb->ins_start_offset = dsb->free_pos;
> +
> +		/* Update the size. */
> +		buf[dsb->free_pos++] = 1;
> +
> +		/* Update the opcode and reg. */
> +		buf[dsb->free_pos++] = (DSB_OPCODE_INDEXED_WRITE  <<
> +					DSB_OPCODE_SHIFT) |
> +					i915_mmio_reg_offset(reg);
> +
> +		/* Update the value. */
> +		buf[dsb->free_pos++] = val;
> +	} else {
> +		/* Update the new value. */
> +		buf[dsb->free_pos++] = val;
> +
> +		/* Update the size. */
> +		buf[dsb->ins_start_offset]++;
> +	}
> +
> +	/* if number of data words is odd, then the last dword should be 0.*/
> +	if (dsb->free_pos & 0x1)
> +		buf[dsb->free_pos] = 0;
So we are adding 0 on every even position while writing buffer and 
letting the next write to overwrite it. Can be done in commit() in the 
end too, but I think its more or less same.
> +}
> +
>   void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>   {
>   	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> index 31b87dcfe160..9b2522f20bfb 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -29,11 +29,19 @@ struct intel_dsb {
>   	 * and help in calculating tail of command buffer.
>   	 */
>   	int free_pos;
> +
> +	/*
> +	 * ins_start_offset will help to store start address
> +	 * of the dsb instuction of auto-increment register.
> +	 */
> +	u32 ins_start_offset;
>   };
>   
>   struct intel_dsb *
>   intel_dsb_get(struct intel_crtc *crtc);
>   void intel_dsb_put(struct intel_dsb *dsb);
>   void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
> +void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
> +				 u32 val);
>   
>   #endif

Looks good to me,

Please feel free to use Reviewed-by: Shashank Sharma 
<shashank.sharma@intel.com>

- Shashank
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 03/10] drm/i915/dsb: single register write function for DSB.

[Sharma, Shashank]

On 9/12/2019 6:37 PM, Animesh Manna wrote:
>
>
> On 9/12/2019 6:30 PM, Sharma, Shashank wrote:
>>
>> On 9/12/2019 6:21 PM, Jani Nikula wrote:
>>> On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> 
>>> wrote:
>>>> On 9/12/2019 12:41 AM, Animesh Manna wrote:
>>>>> DSB support single register write through opcode 0x1. Generic
>>>>> api created which accumulate all single register write in a batch
>>>>> buffer and once DSB is triggered, it will program all the registers
>>>>> at the same time.
>>>>>
>>>>> v1: Initial version.
>>>>> v2: Unused macro removed and cosmetic changes done. (Shashank)
>>>>> v3: set free_pos to zero in dsb-put() instead dsb-get() and
>>>>> a cosmetic change. (Shashank)
>>>>>
>>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/display/intel_dsb.c | 30 
>>>>> ++++++++++++++++++++++++
>>>>>    drivers/gpu/drm/i915/display/intel_dsb.h |  9 +++++++
>>>>>    2 files changed, 39 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c 
>>>>> b/drivers/gpu/drm/i915/display/intel_dsb.c
>>>>> index 7c1b1574788c..e2c383352145 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>>>>> @@ -9,6 +9,13 @@
>>>>>       #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
>>>>>    +/* DSB opcodes. */
>>>>> +#define DSB_OPCODE_SHIFT        24
>>>>> +#define DSB_OPCODE_MMIO_WRITE        0x1
>>>>> +#define DSB_OPCODE_INDEXED_WRITE    0x9
>>>> We are not using this macro here, this should go to the Batch
>>>> /INDEXED_WRITE patch.
>>>>> +#define DSB_BYTE_EN            0xF
>>>>> +#define DSB_BYTE_EN_SHIFT        20
>>>>> +
>>>>>    struct intel_dsb *
>>>>>    intel_dsb_get(struct intel_crtc *crtc)
>>>>>    {
>>>>> @@ -66,5 +73,28 @@ void intel_dsb_put(struct intel_dsb *dsb)
>>>>>            i915_vma_unpin_and_release(&dsb->vma, 0);
>>>>>            mutex_unlock(&i915->drm.struct_mutex);
>>>>>            dsb->cmd_buf = NULL;
>>>>> +        dsb->free_pos = 0;
>>>>> +    }
>>>>> +}
>>>>> +
>>>> I hope this addition of braces are due to diff's adjustment.
>>>>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, 
>>>>> u32 val)
>>>>> +{
>>>>> +    struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
>>>>> +    struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>>> +    u32 *buf = dsb->cmd_buf;
>>>>> +
>>>>> +    if (!buf) {
>>>>> +        I915_WRITE(reg, val);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
>>>>> +        DRM_DEBUG_KMS("DSB buffer overflow\n");
>>>> why shouldn't we do a I915_WRITE(reg, val) here too? This is single
>>>> register write, and we can handle this.
>>> That would assume it's okay to directly mmio write this and the
>>> subsequent values, and write the batch already stored in the buffer
>>> afterwards.
>>
>> This is single write API, so I won't expect this to be called in an 
>> indexed context, also, we have exceeded the buffer size already, so 
>> no subsequent DSB write would be possible anyways. But I can still 
>> see it would be some kind of mess only, so doesn't really matter if 
>> we do this I915_write or not :).
>
> Adding I915_WRITE can be done, but I feel better to break the 
> functionality where we have buffer overflow and based on that we can 
> fine tune the buffer size.
> If a set of register is targetted to write through DSB then some 
> writing through MMIO and and rest writing though DSB may not a nice 
> thing.
> So added only debug log to capture the issue.
>
> Regards,
> Animesh

Yeah, broken this way or other, better to warn as soon as possible.

With the above macro comment fixed,

Please feel free to use Reviewed-by: Shashank Sharma 
<shashank.sharma@intel.com>

- Shashank

>
>>
>> - Shashank
>>
>>> BR,
>>> Jani.
>>>
>>>>> +        return;
>>>>>        }
>>>>> +
>>>>> +    buf[dsb->free_pos++] = val;
>>>>> +    buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE << 
>>>>> DSB_OPCODE_SHIFT) |
>>>>> +                   (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>>>>> +                   i915_mmio_reg_offset(reg);
>>>>>    }
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h 
>>>>> b/drivers/gpu/drm/i915/display/intel_dsb.h
>>>>> index 27eb68eb5392..31b87dcfe160 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>>>>> @@ -6,6 +6,8 @@
>>>>>    #ifndef _INTEL_DSB_H
>>>>>    #define _INTEL_DSB_H
>>>>>    +#include "i915_reg.h"
>>>>> +
>>>>>    struct intel_crtc;
>>>>>    struct i915_vma;
>>>>>    @@ -21,10 +23,17 @@ struct intel_dsb {
>>>>>        enum dsb_id id;
>>>>>        u32 *cmd_buf;
>>>>>        struct i915_vma *vma;
>>>>> +
>>>>> +    /*
>>>>> +     * free_pos will point the first free entry position
>>>>> +     * and help in calculating tail of command buffer.
>>>>> +     */
>>>>> +    int free_pos;
>>>>>    };
>>>>>       struct intel_dsb *
>>>>>    intel_dsb_get(struct intel_crtc *crtc);
>>>>>    void intel_dsb_put(struct intel_dsb *dsb);
>>>>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, 
>>>>> u32 val);
>>>>>       #endif
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 05/10] drm/i915/dsb: Check DSB engine status.

[Sharma, Shashank]

On 9/12/2019 12:41 AM, Animesh Manna wrote:
> As per bspec check for DSB status before programming any
> of its register. Inline function added to check the dsb status.
>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dsb.c | 9 +++++++++
>   drivers/gpu/drm/i915/i915_reg.h          | 7 +++++++
>   2 files changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 9e2927f869b9..b1da2b06263a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -17,6 +17,15 @@
>   #define DSB_BYTE_EN_SHIFT		20
>   #define DSB_REG_VALUE_MASK		0xfffff
>   
> +static inline bool is_dsb_busy(struct intel_dsb *dsb)
> +{
> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +
> +	return DSB_STATUS & I915_READ(DSB_CTRL(pipe, dsb->id));
> +}
> +
>   struct intel_dsb *
>   intel_dsb_get(struct intel_crtc *crtc)
>   {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index bf37ecebc82f..9188a0b53538 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -11683,4 +11683,11 @@ enum skl_power_gate {
>   #define PORT_TX_DFLEXDPCSSS(fia)		_MMIO_FIA((fia), 0x00894)
>   #define   DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)		(1 << (tc_port))
>   
> +/* This register controls the Display State Buffer (DSB) engines. */
> +#define _DSBSL_INSTANCE_BASE		0x70B00
> +#define DSBSL_INSTANCE(pipe, id)	(_DSBSL_INSTANCE_BASE + \
> +					 (pipe) * 0x1000 + (id) * 100)
> +#define DSB_CTRL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x8)
> +#define   DSB_STATUS			(1 << 0)
> +
>   #endif /* _I915_REG_H_ */

Looks good to me,

Please feel free to use Reviewed-by: Shashank Sharma 
<shashank.sharma@intel.com>

- Shashank

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 08/10] drm/i915/dsb: Enable gamma lut programming using DSB.

[Animesh Manna]

On 9/12/2019 6:37 PM, Jani Nikula wrote:
> On Thu, 12 Sep 2019, Animesh Manna <animesh.manna@intel.com> wrote:
>> Gamma lut programming can be programmed using DSB
>> where bulk register programming can be done using indexed
>> register write which takes number of data and the mmio offset
>> to be written.
>>
>> Currently enabled for 12-bit gamma LUT which is enabled by
>> default and later 8-bit/10-bit will be enabled in future
>> based on need.
>>
>> v1: Initial version.
>> v2: Directly call dsb-api at callsites. (Jani)
>> v3:
>> - modified the code as per single dsb instance per crtc. (Shashank)
>> - Added dsb get/put call in platform specific load_lut hook. (Jani)
>> - removed dsb pointer from dev_priv. (Jani)
>> v4: simplified code by dropping ref-count implementation. (Shashank)
>>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_color.c | 57 +++++++++++++---------
>>   1 file changed, 35 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
>> index 318308dc136c..b6b9f0e5166b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>> @@ -611,12 +611,13 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
>>   static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
>>   {
>>   	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
> When you've done enough kernel programming, you learn to expect get/put
> to be paired, and assume it's a bug if this isn't the case.
Already have done it in my previous version,
https://patchwork.freedesktop.org/patch/329481/?series=63013&rev=5
https://patchwork.freedesktop.org/patch/329482/?series=63013&rev=5

Can you please suggest if it is ok?

Regards,
Animesh
>
> So yeah, I did say it's okay not to have refcounting at first, *but* the
> expectation that get/put go in pairs is so deeply ingrained that I
> didn't even think to say you shouldn't have this kind of asymmetry.
>
> So this creates a problem, how do we pass the dsb pointer here, as
> without refcounting you can't actually have the put here as well,
> because you throw the stuff out before the commit.
>
> Maybe the easy answer is that we should just do the get and put at
> intel_crtc_init and intel_crtc_destroy. Or we could do it at atomic
> check and free.
>
> Ville, which approach would conflict with your future vblank worker
> stuff the least?
>
>
> BR,
> Jani.
>
>
>
>>   	enum pipe pipe = crtc->pipe;
>>   
>>   	/* Program the max register to clamp values > 1.0. */
>> -	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
>> -	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
>> -	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
>> +	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
>> +	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
>> +	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
>>   
>>   	/*
>>   	 * Program the gc max 2 register to clamp values > 1.0.
>> @@ -624,9 +625,12 @@ static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
>>   	 * from 3.0 to 7.0
>>   	 */
>>   	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
>> -		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16);
>> -		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16);
>> -		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16);
>> +		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 0),
>> +				    1 << 16);
>> +		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 1),
>> +				    1 << 16);
>> +		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 2),
>> +				    1 << 16);
>>   	}
>>   }
>>   
>> @@ -787,22 +791,22 @@ icl_load_gcmax(const struct intel_crtc_state *crtc_state,
>>   	       const struct drm_color_lut *color)
>>   {
>>   	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>>   	enum pipe pipe = crtc->pipe;
>>   
>>   	/* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */
>> -	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), color->red);
>> -	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), color->green);
>> -	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), color->blue);
>> +	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 0), color->red);
>> +	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 1), color->green);
>> +	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 2), color->blue);
>>   }
>>   
>>   static void
>>   icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>>   {
>>   	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>   	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>>   	const struct drm_color_lut *lut = blob->data;
>> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>>   	enum pipe pipe = crtc->pipe;
>>   	u32 i;
>>   
>> @@ -813,15 +817,16 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>>   	 * Superfine segment has 9 entries, corresponding to values
>>   	 * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
>>   	 */
>> -	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>> +	intel_dsb_reg_write(dsb, PREC_PAL_MULTI_SEG_INDEX(pipe),
>> +			    PAL_PREC_AUTO_INCREMENT);
>>   
>>   	for (i = 0; i < 9; i++) {
>>   		const struct drm_color_lut *entry = &lut[i];
>>   
>> -		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
>> -			   ilk_lut_12p4_ldw(entry));
>> -		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
>> -			   ilk_lut_12p4_udw(entry));
>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe),
>> +					    ilk_lut_12p4_ldw(entry));
>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe),
>> +					    ilk_lut_12p4_udw(entry));
>>   	}
>>   }
>>   
>> @@ -829,10 +834,10 @@ static void
>>   icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>>   {
>>   	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>   	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>>   	const struct drm_color_lut *lut = blob->data;
>>   	const struct drm_color_lut *entry;
>> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>>   	enum pipe pipe = crtc->pipe;
>>   	u32 i;
>>   
>> @@ -847,11 +852,13 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>>   	 * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
>>   	 * with seg2[0] being unused by the hardware.
>>   	 */
>> -	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>> +	intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>>   	for (i = 1; i < 257; i++) {
>>   		entry = &lut[i * 8];
>> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
>> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>> +					    ilk_lut_12p4_ldw(entry));
>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>> +					    ilk_lut_12p4_udw(entry));
>>   	}
>>   
>>   	/*
>> @@ -868,8 +875,10 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>>   	 */
>>   	for (i = 0; i < 256; i++) {
>>   		entry = &lut[i * 8 * 128];
>> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
>> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>> +					    ilk_lut_12p4_ldw(entry));
>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>> +					    ilk_lut_12p4_udw(entry));
>>   	}
>>   
>>   	/* The last entry in the LUT is to be programmed in GCMAX */
>> @@ -882,6 +891,7 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>>   {
>>   	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
>>   	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>>   
>>   	if (crtc_state->base.degamma_lut)
>>   		glk_load_degamma_lut(crtc_state);
>> @@ -900,6 +910,9 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>>   		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
>>   		ivb_load_lut_ext_max(crtc);
>>   	}
>> +
>> +	intel_dsb_commit(dsb);
>> +	intel_dsb_put(dsb);
>>   }
>>   
>>   static u32 chv_cgm_degamma_ldw(const struct drm_color_lut *color)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 06/10] drm/i915/dsb: functions to enable/disable DSB engine.

[Sharma, Shashank]

On 9/12/2019 12:41 AM, Animesh Manna wrote:
> DSB will be used for performance improvement for some special scenario.
> DSB engine will be enabled based on need and after completion of its work
> will be disabled. Api added for enable/disable operation by using DSB_CTRL
> register.
>
> v1: Initial version.
> v2: POSTING_READ added after writing control register. (Shashank)
> v3: cosmetic changes done. (Shashank)
>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dsb.c | 40 ++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_reg.h          |  1 +
>   2 files changed, 41 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index b1da2b06263a..2b0ffc0afb74 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -26,6 +26,46 @@ static inline bool is_dsb_busy(struct intel_dsb *dsb)
>   	return DSB_STATUS & I915_READ(DSB_CTRL(pipe, dsb->id));
>   }
>   
> +static inline bool intel_dsb_enable_engine(struct intel_dsb *dsb)
> +{
> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +	u32 dsb_ctrl;
> +
> +	dsb_ctrl = I915_READ(DSB_CTRL(pipe, dsb->id));
> +	if (DSB_STATUS & dsb_ctrl) {
> +		DRM_DEBUG_KMS("DSB engine is busy.\n");
> +		return false;
> +	}
> +
> +	dsb_ctrl |= DSB_ENABLE;
> +	I915_WRITE(DSB_CTRL(pipe, dsb->id), dsb_ctrl);
> +
> +	POSTING_READ(DSB_CTRL(pipe, dsb->id));
> +	return true;
> +}
> +
> +static inline bool intel_dsb_disable_engine(struct intel_dsb *dsb)
> +{
> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +	u32 dsb_ctrl;
> +
> +	dsb_ctrl = I915_READ(DSB_CTRL(pipe, dsb->id));
> +	if (DSB_STATUS & dsb_ctrl) {
> +		DRM_DEBUG_KMS("DSB engine is busy.\n");
> +		return false;
> +	}
> +
> +	dsb_ctrl &= ~DSB_ENABLE;

Do we really need to care about reading the reg val first and then 
disabling it ? I can understand that for enable().

How about this:

if (!dsb_is_busy()) {

     I915_WRITE(DSB_CTRL(pipe, dsb->id), 0);

    POSTING_READ();

    return true;

}

DRM_DEBUG_KMS("DSB engine is busy.\n");
return false;

But this is optional suggestion, you can take a call on this.

> +	I915_WRITE(DSB_CTRL(pipe, dsb->id), dsb_ctrl);
> +
> +	POSTING_READ(DSB_CTRL(pipe, dsb->id));
> +	return true;
> +}
> +
>   struct intel_dsb *
>   intel_dsb_get(struct intel_crtc *crtc)
>   {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9188a0b53538..2dbaa49f5c74 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -11688,6 +11688,7 @@ enum skl_power_gate {
>   #define DSBSL_INSTANCE(pipe, id)	(_DSBSL_INSTANCE_BASE + \
>   					 (pipe) * 0x1000 + (id) * 100)
>   #define DSB_CTRL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x8)
> +#define   DSB_ENABLE			(1 << 31)
>   #define   DSB_STATUS			(1 << 0)
>   

With or without suggested change above: Feel free to use

Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

- Shashank

>   #endif /* _I915_REG_H_ */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 07/10] drm/i915/dsb: function to trigger workload execution of DSB.

[Sharma, Shashank]

On 9/12/2019 12:41 AM, Animesh Manna wrote:
> Batch buffer will be created through dsb-reg-write function which can have
> single/multiple request based on usecase and once the buffer is ready
> commit function will trigger the execution of the batch buffer. All
> the registers will be updated simultaneously.
>
> v1: Initial version.
> v2: Optimized code few places. (Chris)
> v3: USed DRM_ERROR for dsb head/tail programming failure. (Shashank)
>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dsb.c | 42 ++++++++++++++++++++++++
>   drivers/gpu/drm/i915/display/intel_dsb.h |  1 +
>   drivers/gpu/drm/i915/i915_reg.h          |  2 ++
>   3 files changed, 45 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 2b0ffc0afb74..eea86afb0583 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -212,3 +212,45 @@ void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>   			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>   			       i915_mmio_reg_offset(reg);
>   }
> +
> +void intel_dsb_commit(struct intel_dsb *dsb)
> +{
> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	enum pipe pipe = crtc->pipe;
> +	u32 tail;
> +
> +	if (!dsb->free_pos)
> +		return;
> +
> +	if (!intel_dsb_enable_engine(dsb))
> +		goto reset;
> +
> +	if (is_dsb_busy(dsb)) {
> +		DRM_ERROR("HEAD_PTR write failed - dsb engine is busy.\n");
> +		goto reset;
> +	}
> +	I915_WRITE(DSB_HEAD(pipe, dsb->id), i915_ggtt_offset(dsb->vma));
> +
> +	tail = ALIGN(dsb->free_pos * 4, CACHELINE_BYTES);
> +	if (tail > dsb->free_pos * 4)
> +		memset(&dsb->cmd_buf[dsb->free_pos], 0,
> +		       (tail - dsb->free_pos * 4));
> +
> +	if (is_dsb_busy(dsb)) {
> +		DRM_ERROR("TAIL_PTR write failed - dsb engine is busy.\n");
> +		goto reset;
> +	}
> +	DRM_DEBUG_KMS("DSB execution started - head 0x%x, tail 0x%x\n",
> +		      i915_ggtt_offset(dsb->vma), tail);
> +	I915_WRITE(DSB_TAIL(pipe, dsb->id), i915_ggtt_offset(dsb->vma) + tail);
> +	if (wait_for(!is_dsb_busy(dsb), 1)) {
> +		DRM_ERROR("Timed out waiting for DSB workload completion.\n");
> +		goto reset;
> +	}
> +
> +reset:
> +	dsb->free_pos = 0;
> +	intel_dsb_disable_engine(dsb);

I am still not very convince if a commit function should be void, I 
would still want it to return success or failure, so that we would know 
if the last operation was successful or not.

I would wait Jani N to comment here, on what he feels about this.

- Shashank

> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> index 9b2522f20bfb..7389c8c5b665 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -43,5 +43,6 @@ void intel_dsb_put(struct intel_dsb *dsb);
>   void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
>   void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
>   				 u32 val);
> +void intel_dsb_commit(struct intel_dsb *dsb);
>   
>   #endif
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2dbaa49f5c74..c77b5066d8dd 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -11687,6 +11687,8 @@ enum skl_power_gate {
>   #define _DSBSL_INSTANCE_BASE		0x70B00
>   #define DSBSL_INSTANCE(pipe, id)	(_DSBSL_INSTANCE_BASE + \
>   					 (pipe) * 0x1000 + (id) * 100)
> +#define DSB_HEAD(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x0)
> +#define DSB_TAIL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x4)
>   #define DSB_CTRL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x8)
>   #define   DSB_ENABLE			(1 << 31)
>   #define   DSB_STATUS			(1 << 0)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 07/10] drm/i915/dsb: function to trigger workload execution of DSB.

[Jani Nikula]

On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> wrote:
> On 9/12/2019 12:41 AM, Animesh Manna wrote:
>> Batch buffer will be created through dsb-reg-write function which can have
>> single/multiple request based on usecase and once the buffer is ready
>> commit function will trigger the execution of the batch buffer. All
>> the registers will be updated simultaneously.
>>
>> v1: Initial version.
>> v2: Optimized code few places. (Chris)
>> v3: USed DRM_ERROR for dsb head/tail programming failure. (Shashank)
>>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dsb.c | 42 ++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_dsb.h |  1 +
>>   drivers/gpu/drm/i915/i915_reg.h          |  2 ++
>>   3 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
>> index 2b0ffc0afb74..eea86afb0583 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>> @@ -212,3 +212,45 @@ void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>>   			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>>   			       i915_mmio_reg_offset(reg);
>>   }
>> +
>> +void intel_dsb_commit(struct intel_dsb *dsb)
>> +{
>> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
>> +	struct drm_device *dev = crtc->base.dev;
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>> +	enum pipe pipe = crtc->pipe;
>> +	u32 tail;
>> +
>> +	if (!dsb->free_pos)
>> +		return;
>> +
>> +	if (!intel_dsb_enable_engine(dsb))
>> +		goto reset;
>> +
>> +	if (is_dsb_busy(dsb)) {
>> +		DRM_ERROR("HEAD_PTR write failed - dsb engine is busy.\n");
>> +		goto reset;
>> +	}
>> +	I915_WRITE(DSB_HEAD(pipe, dsb->id), i915_ggtt_offset(dsb->vma));
>> +
>> +	tail = ALIGN(dsb->free_pos * 4, CACHELINE_BYTES);
>> +	if (tail > dsb->free_pos * 4)
>> +		memset(&dsb->cmd_buf[dsb->free_pos], 0,
>> +		       (tail - dsb->free_pos * 4));
>> +
>> +	if (is_dsb_busy(dsb)) {
>> +		DRM_ERROR("TAIL_PTR write failed - dsb engine is busy.\n");
>> +		goto reset;
>> +	}
>> +	DRM_DEBUG_KMS("DSB execution started - head 0x%x, tail 0x%x\n",
>> +		      i915_ggtt_offset(dsb->vma), tail);
>> +	I915_WRITE(DSB_TAIL(pipe, dsb->id), i915_ggtt_offset(dsb->vma) + tail);
>> +	if (wait_for(!is_dsb_busy(dsb), 1)) {
>> +		DRM_ERROR("Timed out waiting for DSB workload completion.\n");
>> +		goto reset;
>> +	}
>> +
>> +reset:
>> +	dsb->free_pos = 0;
>> +	intel_dsb_disable_engine(dsb);
>
> I am still not very convince if a commit function should be void, I 
> would still want it to return success or failure, so that we would know 
> if the last operation was successful or not.
>
> I would wait Jani N to comment here, on what he feels about this.

The question becomes, what do you *do* with the return value? If none of
the callers are going to use it, don't return it.

BR,
Jani.

>
> - Shashank
>
>> +}
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
>> index 9b2522f20bfb..7389c8c5b665 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>> @@ -43,5 +43,6 @@ void intel_dsb_put(struct intel_dsb *dsb);
>>   void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
>>   void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
>>   				 u32 val);
>> +void intel_dsb_commit(struct intel_dsb *dsb);
>>   
>>   #endif
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 2dbaa49f5c74..c77b5066d8dd 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -11687,6 +11687,8 @@ enum skl_power_gate {
>>   #define _DSBSL_INSTANCE_BASE		0x70B00
>>   #define DSBSL_INSTANCE(pipe, id)	(_DSBSL_INSTANCE_BASE + \
>>   					 (pipe) * 0x1000 + (id) * 100)
>> +#define DSB_HEAD(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x0)
>> +#define DSB_TAIL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x4)
>>   #define DSB_CTRL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x8)
>>   #define   DSB_ENABLE			(1 << 31)
>>   #define   DSB_STATUS			(1 << 0)

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 07/10] drm/i915/dsb: function to trigger workload execution of DSB.

[Sharma, Shashank]

On 9/12/2019 7:09 PM, Jani Nikula wrote:
> On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> wrote:
>> On 9/12/2019 12:41 AM, Animesh Manna wrote:
>>> Batch buffer will be created through dsb-reg-write function which can have
>>> single/multiple request based on usecase and once the buffer is ready
>>> commit function will trigger the execution of the batch buffer. All
>>> the registers will be updated simultaneously.
>>>
>>> v1: Initial version.
>>> v2: Optimized code few places. (Chris)
>>> v3: USed DRM_ERROR for dsb head/tail programming failure. (Shashank)
>>>
>>> Cc: Imre Deak <imre.deak@intel.com>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/display/intel_dsb.c | 42 ++++++++++++++++++++++++
>>>    drivers/gpu/drm/i915/display/intel_dsb.h |  1 +
>>>    drivers/gpu/drm/i915/i915_reg.h          |  2 ++
>>>    3 files changed, 45 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
>>> index 2b0ffc0afb74..eea86afb0583 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>>> @@ -212,3 +212,45 @@ void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>>>    			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>>>    			       i915_mmio_reg_offset(reg);
>>>    }
>>> +
>>> +void intel_dsb_commit(struct intel_dsb *dsb)
>>> +{
>>> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
>>> +	struct drm_device *dev = crtc->base.dev;
>>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>>> +	enum pipe pipe = crtc->pipe;
>>> +	u32 tail;
>>> +
>>> +	if (!dsb->free_pos)
>>> +		return;
>>> +
>>> +	if (!intel_dsb_enable_engine(dsb))
>>> +		goto reset;
>>> +
>>> +	if (is_dsb_busy(dsb)) {
>>> +		DRM_ERROR("HEAD_PTR write failed - dsb engine is busy.\n");
>>> +		goto reset;
>>> +	}
>>> +	I915_WRITE(DSB_HEAD(pipe, dsb->id), i915_ggtt_offset(dsb->vma));
>>> +
>>> +	tail = ALIGN(dsb->free_pos * 4, CACHELINE_BYTES);
>>> +	if (tail > dsb->free_pos * 4)
>>> +		memset(&dsb->cmd_buf[dsb->free_pos], 0,
>>> +		       (tail - dsb->free_pos * 4));
>>> +
>>> +	if (is_dsb_busy(dsb)) {
>>> +		DRM_ERROR("TAIL_PTR write failed - dsb engine is busy.\n");
>>> +		goto reset;
>>> +	}
>>> +	DRM_DEBUG_KMS("DSB execution started - head 0x%x, tail 0x%x\n",
>>> +		      i915_ggtt_offset(dsb->vma), tail);
>>> +	I915_WRITE(DSB_TAIL(pipe, dsb->id), i915_ggtt_offset(dsb->vma) + tail);
>>> +	if (wait_for(!is_dsb_busy(dsb), 1)) {
>>> +		DRM_ERROR("Timed out waiting for DSB workload completion.\n");
>>> +		goto reset;
>>> +	}
>>> +
>>> +reset:
>>> +	dsb->free_pos = 0;
>>> +	intel_dsb_disable_engine(dsb);
>> I am still not very convince if a commit function should be void, I
>> would still want it to return success or failure, so that we would know
>> if the last operation was successful or not.
>>
>> I would wait Jani N to comment here, on what he feels about this.
> The question becomes, what do you *do* with the return value? If none of
> the callers are going to use it, don't return it.

I was thinking we should check the return value of the DSB commit (if 
not writes), so that we would be aware that the register programming 
failed, and later even can think about a fallback method. Too ambitious ?

- Shashank

> BR,
> Jani.
>
>> - Shashank
>>
>>> +}
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
>>> index 9b2522f20bfb..7389c8c5b665 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>>> @@ -43,5 +43,6 @@ void intel_dsb_put(struct intel_dsb *dsb);
>>>    void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
>>>    void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
>>>    				 u32 val);
>>> +void intel_dsb_commit(struct intel_dsb *dsb);
>>>    
>>>    #endif
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 2dbaa49f5c74..c77b5066d8dd 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -11687,6 +11687,8 @@ enum skl_power_gate {
>>>    #define _DSBSL_INSTANCE_BASE		0x70B00
>>>    #define DSBSL_INSTANCE(pipe, id)	(_DSBSL_INSTANCE_BASE + \
>>>    					 (pipe) * 0x1000 + (id) * 100)
>>> +#define DSB_HEAD(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x0)
>>> +#define DSB_TAIL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x4)
>>>    #define DSB_CTRL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x8)
>>>    #define   DSB_ENABLE			(1 << 31)
>>>    #define   DSB_STATUS			(1 << 0)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 09/10] drm/i915/dsb: Enable DSB for gen12.

[Sharma, Shashank]

On 9/12/2019 12:41 AM, Animesh Manna wrote:
> Enabling DSB by setting 1 to has_dsb flag for gen12.
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_pci.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index b3cc8560696b..1fd2a364891a 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -787,7 +787,8 @@ static const struct intel_device_info intel_elkhartlake_info = {
>   		[TRANSCODER_DSI_0] = TRANSCODER_DSI0_OFFSET, \
>   		[TRANSCODER_DSI_1] = TRANSCODER_DSI1_OFFSET, \
>   	}, \
> -	.has_global_mocs = 1
> +	.has_global_mocs = 1, \
> +	.display.has_dsb = 1

Looks good to me, feel free to use:

Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

- Shashank

>   
>   static const struct intel_device_info intel_tigerlake_12_info = {
>   	GEN12_FEATURES,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 07/10] drm/i915/dsb: function to trigger workload execution of DSB.

[Jani Nikula]

On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> wrote:
> On 9/12/2019 7:09 PM, Jani Nikula wrote:
>> On Thu, 12 Sep 2019, "Sharma, Shashank" <shashank.sharma@intel.com> wrote:
>>> On 9/12/2019 12:41 AM, Animesh Manna wrote:
>>>> Batch buffer will be created through dsb-reg-write function which can have
>>>> single/multiple request based on usecase and once the buffer is ready
>>>> commit function will trigger the execution of the batch buffer. All
>>>> the registers will be updated simultaneously.
>>>>
>>>> v1: Initial version.
>>>> v2: Optimized code few places. (Chris)
>>>> v3: USed DRM_ERROR for dsb head/tail programming failure. (Shashank)
>>>>
>>>> Cc: Imre Deak <imre.deak@intel.com>
>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/display/intel_dsb.c | 42 ++++++++++++++++++++++++
>>>>    drivers/gpu/drm/i915/display/intel_dsb.h |  1 +
>>>>    drivers/gpu/drm/i915/i915_reg.h          |  2 ++
>>>>    3 files changed, 45 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> index 2b0ffc0afb74..eea86afb0583 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> @@ -212,3 +212,45 @@ void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>>>>    			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
>>>>    			       i915_mmio_reg_offset(reg);
>>>>    }
>>>> +
>>>> +void intel_dsb_commit(struct intel_dsb *dsb)
>>>> +{
>>>> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
>>>> +	struct drm_device *dev = crtc->base.dev;
>>>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>>>> +	enum pipe pipe = crtc->pipe;
>>>> +	u32 tail;
>>>> +
>>>> +	if (!dsb->free_pos)
>>>> +		return;
>>>> +
>>>> +	if (!intel_dsb_enable_engine(dsb))
>>>> +		goto reset;
>>>> +
>>>> +	if (is_dsb_busy(dsb)) {
>>>> +		DRM_ERROR("HEAD_PTR write failed - dsb engine is busy.\n");
>>>> +		goto reset;
>>>> +	}
>>>> +	I915_WRITE(DSB_HEAD(pipe, dsb->id), i915_ggtt_offset(dsb->vma));
>>>> +
>>>> +	tail = ALIGN(dsb->free_pos * 4, CACHELINE_BYTES);
>>>> +	if (tail > dsb->free_pos * 4)
>>>> +		memset(&dsb->cmd_buf[dsb->free_pos], 0,
>>>> +		       (tail - dsb->free_pos * 4));
>>>> +
>>>> +	if (is_dsb_busy(dsb)) {
>>>> +		DRM_ERROR("TAIL_PTR write failed - dsb engine is busy.\n");
>>>> +		goto reset;
>>>> +	}
>>>> +	DRM_DEBUG_KMS("DSB execution started - head 0x%x, tail 0x%x\n",
>>>> +		      i915_ggtt_offset(dsb->vma), tail);
>>>> +	I915_WRITE(DSB_TAIL(pipe, dsb->id), i915_ggtt_offset(dsb->vma) + tail);
>>>> +	if (wait_for(!is_dsb_busy(dsb), 1)) {
>>>> +		DRM_ERROR("Timed out waiting for DSB workload completion.\n");
>>>> +		goto reset;
>>>> +	}
>>>> +
>>>> +reset:
>>>> +	dsb->free_pos = 0;
>>>> +	intel_dsb_disable_engine(dsb);
>>> I am still not very convince if a commit function should be void, I
>>> would still want it to return success or failure, so that we would know
>>> if the last operation was successful or not.
>>>
>>> I would wait Jani N to comment here, on what he feels about this.
>> The question becomes, what do you *do* with the return value? If none of
>> the callers are going to use it, don't return it.
>
> I was thinking we should check the return value of the DSB commit (if 
> not writes), so that we would be aware that the register programming 
> failed, and later even can think about a fallback method. Too ambitious ?

Easy to add later if needed, I think.

BR,
Jani.

>
> - Shashank
>
>> BR,
>> Jani.
>>
>>> - Shashank
>>>
>>>> +}
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
>>>> index 9b2522f20bfb..7389c8c5b665 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>>>> @@ -43,5 +43,6 @@ void intel_dsb_put(struct intel_dsb *dsb);
>>>>    void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val);
>>>>    void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
>>>>    				 u32 val);
>>>> +void intel_dsb_commit(struct intel_dsb *dsb);
>>>>    
>>>>    #endif
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>> index 2dbaa49f5c74..c77b5066d8dd 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -11687,6 +11687,8 @@ enum skl_power_gate {
>>>>    #define _DSBSL_INSTANCE_BASE		0x70B00
>>>>    #define DSBSL_INSTANCE(pipe, id)	(_DSBSL_INSTANCE_BASE + \
>>>>    					 (pipe) * 0x1000 + (id) * 100)
>>>> +#define DSB_HEAD(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x0)
>>>> +#define DSB_TAIL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x4)
>>>>    #define DSB_CTRL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) + 0x8)
>>>>    #define   DSB_ENABLE			(1 << 31)
>>>>    #define   DSB_STATUS			(1 << 0)

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 08/10] drm/i915/dsb: Enable gamma lut programming using DSB.

[Jani Nikula]

On Thu, 12 Sep 2019, Animesh Manna <animesh.manna@intel.com> wrote:
> On 9/12/2019 6:37 PM, Jani Nikula wrote:
>> On Thu, 12 Sep 2019, Animesh Manna <animesh.manna@intel.com> wrote:
>>> Gamma lut programming can be programmed using DSB
>>> where bulk register programming can be done using indexed
>>> register write which takes number of data and the mmio offset
>>> to be written.
>>>
>>> Currently enabled for 12-bit gamma LUT which is enabled by
>>> default and later 8-bit/10-bit will be enabled in future
>>> based on need.
>>>
>>> v1: Initial version.
>>> v2: Directly call dsb-api at callsites. (Jani)
>>> v3:
>>> - modified the code as per single dsb instance per crtc. (Shashank)
>>> - Added dsb get/put call in platform specific load_lut hook. (Jani)
>>> - removed dsb pointer from dev_priv. (Jani)
>>> v4: simplified code by dropping ref-count implementation. (Shashank)
>>>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/display/intel_color.c | 57 +++++++++++++---------
>>>   1 file changed, 35 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
>>> index 318308dc136c..b6b9f0e5166b 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>>> @@ -611,12 +611,13 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
>>>   static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
>>>   {
>>>   	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>> When you've done enough kernel programming, you learn to expect get/put
>> to be paired, and assume it's a bug if this isn't the case.
> Already have done it in my previous version,
> https://patchwork.freedesktop.org/patch/329481/?series=63013&rev=5
> https://patchwork.freedesktop.org/patch/329482/?series=63013&rev=5
>
> Can you please suggest if it is ok?

Yes, but you need to get it right. It seems that the inc and dec can get
out of sync. Also no warnings against putting too many times.

BR,
Jani.



>
> Regards,
> Animesh
>>
>> So yeah, I did say it's okay not to have refcounting at first, *but* the
>> expectation that get/put go in pairs is so deeply ingrained that I
>> didn't even think to say you shouldn't have this kind of asymmetry.
>>
>> So this creates a problem, how do we pass the dsb pointer here, as
>> without refcounting you can't actually have the put here as well,
>> because you throw the stuff out before the commit.
>>
>> Maybe the easy answer is that we should just do the get and put at
>> intel_crtc_init and intel_crtc_destroy. Or we could do it at atomic
>> check and free.
>>
>> Ville, which approach would conflict with your future vblank worker
>> stuff the least?
>>
>>
>> BR,
>> Jani.
>>
>>
>>
>>>   	enum pipe pipe = crtc->pipe;
>>>   
>>>   	/* Program the max register to clamp values > 1.0. */
>>> -	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
>>> -	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
>>> -	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
>>> +	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
>>> +	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
>>> +	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
>>>   
>>>   	/*
>>>   	 * Program the gc max 2 register to clamp values > 1.0.
>>> @@ -624,9 +625,12 @@ static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
>>>   	 * from 3.0 to 7.0
>>>   	 */
>>>   	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
>>> -		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16);
>>> -		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16);
>>> -		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16);
>>> +		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 0),
>>> +				    1 << 16);
>>> +		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 1),
>>> +				    1 << 16);
>>> +		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 2),
>>> +				    1 << 16);
>>>   	}
>>>   }
>>>   
>>> @@ -787,22 +791,22 @@ icl_load_gcmax(const struct intel_crtc_state *crtc_state,
>>>   	       const struct drm_color_lut *color)
>>>   {
>>>   	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>>>   	enum pipe pipe = crtc->pipe;
>>>   
>>>   	/* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */
>>> -	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), color->red);
>>> -	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), color->green);
>>> -	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), color->blue);
>>> +	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 0), color->red);
>>> +	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 1), color->green);
>>> +	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 2), color->blue);
>>>   }
>>>   
>>>   static void
>>>   icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>>>   {
>>>   	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>   	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>>>   	const struct drm_color_lut *lut = blob->data;
>>> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>>>   	enum pipe pipe = crtc->pipe;
>>>   	u32 i;
>>>   
>>> @@ -813,15 +817,16 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>>>   	 * Superfine segment has 9 entries, corresponding to values
>>>   	 * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
>>>   	 */
>>> -	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>>> +	intel_dsb_reg_write(dsb, PREC_PAL_MULTI_SEG_INDEX(pipe),
>>> +			    PAL_PREC_AUTO_INCREMENT);
>>>   
>>>   	for (i = 0; i < 9; i++) {
>>>   		const struct drm_color_lut *entry = &lut[i];
>>>   
>>> -		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
>>> -			   ilk_lut_12p4_ldw(entry));
>>> -		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
>>> -			   ilk_lut_12p4_udw(entry));
>>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe),
>>> +					    ilk_lut_12p4_ldw(entry));
>>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe),
>>> +					    ilk_lut_12p4_udw(entry));
>>>   	}
>>>   }
>>>   
>>> @@ -829,10 +834,10 @@ static void
>>>   icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>>>   {
>>>   	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>   	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>>>   	const struct drm_color_lut *lut = blob->data;
>>>   	const struct drm_color_lut *entry;
>>> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>>>   	enum pipe pipe = crtc->pipe;
>>>   	u32 i;
>>>   
>>> @@ -847,11 +852,13 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>>>   	 * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
>>>   	 * with seg2[0] being unused by the hardware.
>>>   	 */
>>> -	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>>> +	intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>>>   	for (i = 1; i < 257; i++) {
>>>   		entry = &lut[i * 8];
>>> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
>>> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
>>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>>> +					    ilk_lut_12p4_ldw(entry));
>>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>>> +					    ilk_lut_12p4_udw(entry));
>>>   	}
>>>   
>>>   	/*
>>> @@ -868,8 +875,10 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>>>   	 */
>>>   	for (i = 0; i < 256; i++) {
>>>   		entry = &lut[i * 8 * 128];
>>> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
>>> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
>>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>>> +					    ilk_lut_12p4_ldw(entry));
>>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>>> +					    ilk_lut_12p4_udw(entry));
>>>   	}
>>>   
>>>   	/* The last entry in the LUT is to be programmed in GCMAX */
>>> @@ -882,6 +891,7 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>>>   {
>>>   	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
>>>   	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>>>   
>>>   	if (crtc_state->base.degamma_lut)
>>>   		glk_load_degamma_lut(crtc_state);
>>> @@ -900,6 +910,9 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>>>   		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
>>>   		ivb_load_lut_ext_max(crtc);
>>>   	}
>>> +
>>> +	intel_dsb_commit(dsb);
>>> +	intel_dsb_put(dsb);
>>>   }
>>>   
>>>   static u32 chv_cgm_degamma_ldw(const struct drm_color_lut *color)
>

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6 08/10] drm/i915/dsb: Enable gamma lut programming using DSB.

[Animesh Manna]

On 9/17/2019 1:00 PM, Jani Nikula wrote:
> On Thu, 12 Sep 2019, Animesh Manna <animesh.manna@intel.com> wrote:
>> On 9/12/2019 6:37 PM, Jani Nikula wrote:
>>> On Thu, 12 Sep 2019, Animesh Manna <animesh.manna@intel.com> wrote:
>>>> Gamma lut programming can be programmed using DSB
>>>> where bulk register programming can be done using indexed
>>>> register write which takes number of data and the mmio offset
>>>> to be written.
>>>>
>>>> Currently enabled for 12-bit gamma LUT which is enabled by
>>>> default and later 8-bit/10-bit will be enabled in future
>>>> based on need.
>>>>
>>>> v1: Initial version.
>>>> v2: Directly call dsb-api at callsites. (Jani)
>>>> v3:
>>>> - modified the code as per single dsb instance per crtc. (Shashank)
>>>> - Added dsb get/put call in platform specific load_lut hook. (Jani)
>>>> - removed dsb pointer from dev_priv. (Jani)
>>>> v4: simplified code by dropping ref-count implementation. (Shashank)
>>>>
>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/display/intel_color.c | 57 +++++++++++++---------
>>>>    1 file changed, 35 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
>>>> index 318308dc136c..b6b9f0e5166b 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>>>> @@ -611,12 +611,13 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
>>>>    static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
>>>>    {
>>>>    	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>>> When you've done enough kernel programming, you learn to expect get/put
>>> to be paired, and assume it's a bug if this isn't the case.
>> Already have done it in my previous version,
>> https://patchwork.freedesktop.org/patch/329481/?series=63013&rev=5
>> https://patchwork.freedesktop.org/patch/329482/?series=63013&rev=5
>>
>> Can you please suggest if it is ok?
> Yes, but you need to get it right. It seems that the inc and dec can get
> out of sync. Also no warnings against putting too many times.

Hi Jani,
Extra put will not decrement as it is under cmd_buf condition check.
Once it is zero cmd_buf will be assigned to NULL.

if (dsb->cmd_buf) {
         atomic_dec(&dsb->refcount);
         if (!atomic_read(&dsb->refcount)) {
             mutex_lock(&i915->drm.struct_mutex);
             i915_gem_object_unpin_map(dsb->vma->obj);
             i915_vma_unpin_and_release(&dsb->vma, 0);
             dsb->cmd_buf = NULL;
             mutex_unlock(&i915->drm.struct_mutex);
         }
}

Do you want me to add some warning if refcount is already zero and user 
called put?
Can you please help me to understand where inc and dec can go out of sync.

Regards,
Animesh

>
> BR,
> Jani.
>
>
>
>> Regards,
>> Animesh
>>> So yeah, I did say it's okay not to have refcounting at first, *but* the
>>> expectation that get/put go in pairs is so deeply ingrained that I
>>> didn't even think to say you shouldn't have this kind of asymmetry.
>>>
>>> So this creates a problem, how do we pass the dsb pointer here, as
>>> without refcounting you can't actually have the put here as well,
>>> because you throw the stuff out before the commit.
>>>
>>> Maybe the easy answer is that we should just do the get and put at
>>> intel_crtc_init and intel_crtc_destroy. Or we could do it at atomic
>>> check and free.
>>>
>>> Ville, which approach would conflict with your future vblank worker
>>> stuff the least?
>>>
>>>
>>> BR,
>>> Jani.
>>>
>>>
>>>
>>>>    	enum pipe pipe = crtc->pipe;
>>>>    
>>>>    	/* Program the max register to clamp values > 1.0. */
>>>> -	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
>>>> -	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
>>>> -	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
>>>> +	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
>>>> +	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
>>>> +	intel_dsb_reg_write(dsb, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
>>>>    
>>>>    	/*
>>>>    	 * Program the gc max 2 register to clamp values > 1.0.
>>>> @@ -624,9 +625,12 @@ static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
>>>>    	 * from 3.0 to 7.0
>>>>    	 */
>>>>    	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
>>>> -		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16);
>>>> -		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16);
>>>> -		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16);
>>>> +		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 0),
>>>> +				    1 << 16);
>>>> +		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 1),
>>>> +				    1 << 16);
>>>> +		intel_dsb_reg_write(dsb, PREC_PAL_EXT2_GC_MAX(pipe, 2),
>>>> +				    1 << 16);
>>>>    	}
>>>>    }
>>>>    
>>>> @@ -787,22 +791,22 @@ icl_load_gcmax(const struct intel_crtc_state *crtc_state,
>>>>    	       const struct drm_color_lut *color)
>>>>    {
>>>>    	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>>> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>>>>    	enum pipe pipe = crtc->pipe;
>>>>    
>>>>    	/* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */
>>>> -	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), color->red);
>>>> -	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), color->green);
>>>> -	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), color->blue);
>>>> +	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 0), color->red);
>>>> +	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 1), color->green);
>>>> +	intel_dsb_reg_write(dsb, PREC_PAL_GC_MAX(pipe, 2), color->blue);
>>>>    }
>>>>    
>>>>    static void
>>>>    icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>>>>    {
>>>>    	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>>> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>>    	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>>>>    	const struct drm_color_lut *lut = blob->data;
>>>> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>>>>    	enum pipe pipe = crtc->pipe;
>>>>    	u32 i;
>>>>    
>>>> @@ -813,15 +817,16 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>>>>    	 * Superfine segment has 9 entries, corresponding to values
>>>>    	 * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
>>>>    	 */
>>>> -	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>>>> +	intel_dsb_reg_write(dsb, PREC_PAL_MULTI_SEG_INDEX(pipe),
>>>> +			    PAL_PREC_AUTO_INCREMENT);
>>>>    
>>>>    	for (i = 0; i < 9; i++) {
>>>>    		const struct drm_color_lut *entry = &lut[i];
>>>>    
>>>> -		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
>>>> -			   ilk_lut_12p4_ldw(entry));
>>>> -		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
>>>> -			   ilk_lut_12p4_udw(entry));
>>>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe),
>>>> +					    ilk_lut_12p4_ldw(entry));
>>>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_MULTI_SEG_DATA(pipe),
>>>> +					    ilk_lut_12p4_udw(entry));
>>>>    	}
>>>>    }
>>>>    
>>>> @@ -829,10 +834,10 @@ static void
>>>>    icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>>>>    {
>>>>    	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>>> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>>    	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>>>>    	const struct drm_color_lut *lut = blob->data;
>>>>    	const struct drm_color_lut *entry;
>>>> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>>>>    	enum pipe pipe = crtc->pipe;
>>>>    	u32 i;
>>>>    
>>>> @@ -847,11 +852,13 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>>>>    	 * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
>>>>    	 * with seg2[0] being unused by the hardware.
>>>>    	 */
>>>> -	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>>>> +	intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>>>>    	for (i = 1; i < 257; i++) {
>>>>    		entry = &lut[i * 8];
>>>> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
>>>> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
>>>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>>>> +					    ilk_lut_12p4_ldw(entry));
>>>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>>>> +					    ilk_lut_12p4_udw(entry));
>>>>    	}
>>>>    
>>>>    	/*
>>>> @@ -868,8 +875,10 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>>>>    	 */
>>>>    	for (i = 0; i < 256; i++) {
>>>>    		entry = &lut[i * 8 * 128];
>>>> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
>>>> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
>>>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>>>> +					    ilk_lut_12p4_ldw(entry));
>>>> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>>>> +					    ilk_lut_12p4_udw(entry));
>>>>    	}
>>>>    
>>>>    	/* The last entry in the LUT is to be programmed in GCMAX */
>>>> @@ -882,6 +891,7 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>>>>    {
>>>>    	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
>>>>    	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>>> +	struct intel_dsb *dsb = intel_dsb_get(crtc);
>>>>    
>>>>    	if (crtc_state->base.degamma_lut)
>>>>    		glk_load_degamma_lut(crtc_state);
>>>> @@ -900,6 +910,9 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>>>>    		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
>>>>    		ivb_load_lut_ext_max(crtc);
>>>>    	}
>>>> +
>>>> +	intel_dsb_commit(dsb);
>>>> +	intel_dsb_put(dsb);
>>>>    }
>>>>    
>>>>    static u32 chv_cgm_degamma_ldw(const struct drm_color_lut *color)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx