[PATCH v3 00/11] DSB enablement.

[PATCH v3 00/11] 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)  

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 (11):
  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: Register definition of DSB registers.
  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: Documentation for DSB.
  drm/i915/dsb: Enable gamma lut programming using DSB.
  drm/i915/dsb: Enable DSB for gen12.

 Documentation/gpu/i915.rst                    |   9 +
 drivers/gpu/drm/i915/Makefile                 |   1 +
 drivers/gpu/drm/i915/display/intel_color.c    |  64 ++--
 .../drm/i915/display/intel_display_types.h    |   3 +
 drivers/gpu/drm/i915/display/intel_dsb.c      | 326 ++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dsb.h      |  49 +++
 drivers/gpu/drm/i915/i915_drv.h               |   5 +
 drivers/gpu/drm/i915/i915_pci.c               |   3 +-
 drivers/gpu/drm/i915/i915_reg.h               |  15 +
 drivers/gpu/drm/i915/intel_device_info.h      |   1 +
 10 files changed, 452 insertions(+), 24 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 v3 01/11] drm/i915/dsb: feature flag added for display state buffer.

[Animesh Manna]

From gen12 onwards Display State Buffer(DSB) is hardware capability
added which allows driver to batch submit display HW programming.
Feature flag has_dsb added to identify the driver/platform support
at runtime.

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/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 b42651a387d9..d32cfdb78b74 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1856,6 +1856,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 v3 02/11] drm/i915/dsb: DSB context creation.

[Animesh Manna]

The function will internally get the gem buffer from global GTT
which is mapped in cpu domain to feed the data + opcode for 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)

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>
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      | 83 +++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dsb.h      | 31 +++++++
 drivers/gpu/drm/i915/i915_drv.h               |  1 +
 5 files changed, 119 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 96514dcc7812..0ab3516b0044 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1026,6 +1026,9 @@ struct intel_crtc {
 
 	/* scalers available on this crtc */
 	int num_scalers;
+
+	/* per pipe DSB related info */
+	struct intel_dsb dsb[MAX_DSB_PER_PIPE];
 };
 
 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..a2845df90573
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -0,0 +1,83 @@
+// 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;
+	intel_wakeref_t wakeref;
+	int i;
+
+	for (i = 0; i < MAX_DSB_PER_PIPE; i++)
+		if (!crtc->dsb[i].cmd_buf)
+			break;
+
+	WARN_ON(i >= MAX_DSB_PER_PIPE);
+
+	dsb = &crtc->dsb[i];
+	dsb->id = i;
+	dsb->crtc = crtc;
+	if (!HAS_DSB(i915))
+		return dsb;
+
+	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_DEBUG_KMS("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_DEBUG_KMS("Command buffer creation failed.\n");
+		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;
+	struct drm_i915_private *i915;
+	struct i915_vma *vma;
+
+	if (!dsb)
+		return;
+
+	crtc = dsb->crtc;
+	i915 = to_i915(crtc->base.dev);
+
+	if (dsb->cmd_buf) {
+		vma = dsb->vma;
+		mutex_lock(&i915->drm.struct_mutex);
+		i915_gem_object_unpin_map(vma->obj);
+		i915_vma_unpin_and_release(&vma, 0);
+		dsb->cmd_buf = NULL;
+		mutex_unlock(&i915->drm.struct_mutex);
+	}
+}
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..4a4091cadc1e
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_dsb.h
@@ -0,0 +1,31 @@
+/* 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 {
+	struct intel_crtc *crtc;
+	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 d32cfdb78b74..dd6cfb89b830 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 v3 03/11] 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.

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 | 36 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dsb.h |  9 ++++++
 2 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 a2845df90573..df288446caeb 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -9,6 +9,20 @@
 
 #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
 
+/* DSB opcodes. */
+#define DSB_OPCODE_SHIFT		24
+#define DSB_OPCODE_NOOP			0x0
+#define DSB_OPCODE_MMIO_WRITE		0x1
+#define DSB_OPCODE_WAIT_FOR_US		0x2
+#define DSB_OPCODE_WAIT_FOR_LINES	0x3
+#define DSB_OPCODE_WAIT_FOR_VBLANK	0x4
+#define DSB_OPCODE_WAIT_FOR_SL_IN	0x5
+#define DSB_OPCODE_WAIT_FOR_SL_OUT	0x6
+#define DSB_OPCODE_GENERATE_INT		0x7
+#define DSB_OPCODE_INDEXED_WRITE	0x9
+#define DSB_OPCODE_POLL			0xA
+#define DSB_BYTE_EN			(0xf << 20)
+
 struct intel_dsb *
 intel_dsb_get(struct intel_crtc *crtc)
 {
@@ -81,3 +95,25 @@ void intel_dsb_put(struct intel_dsb *dsb)
 		mutex_unlock(&i915->drm.struct_mutex);
 	}
 }
+
+void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
+{
+	struct intel_crtc *crtc = dsb->crtc;
+	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 |
+				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 4a4091cadc1e..1b33ab118640 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;
 
@@ -22,10 +24,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 cmd_buf_tail.
+	 */
+	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 v3 04/11] 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. Will be using for bulk register programming
e.g. gamma lut programming, HDR meta data programming.

v1: Initial version.

v2: simplified code by using ALIGN(). (Chris)

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 | 48 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dsb.h |  8 ++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index df288446caeb..520f2bbcc8ae 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -22,6 +22,7 @@
 #define DSB_OPCODE_INDEXED_WRITE	0x9
 #define DSB_OPCODE_POLL			0xA
 #define DSB_BYTE_EN			(0xf << 20)
+#define DSB_REG_VALUE_MASK		0xfffff
 
 struct intel_dsb *
 intel_dsb_get(struct intel_crtc *crtc)
@@ -96,6 +97,53 @@ 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 = dsb->crtc;
+	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;
+	}
+
+	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);
+
+		/* Update the size. */
+		dsb->ins_start_offset = dsb->free_pos;
+		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 = dsb->crtc;
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
index 1b33ab118640..c848747f52d9 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.h
+++ b/drivers/gpu/drm/i915/display/intel_dsb.h
@@ -30,11 +30,19 @@ struct intel_dsb {
 	 * and help in calculating cmd_buf_tail.
 	 */
 	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 v3 05/11] drm/i915/dsb: Register definition of DSB registers.

[Animesh Manna]

Added key register definitions of DSB.

dsb-ctrl register is required to enable dsb-engine.

head-ptr register hold the head of buffer address from where the
execution will start.

Programming tail-ptr register is a trigger point to start execution.

Cc: Uma Shankar <uma.shankar@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/i915_reg.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 02e1ef10c47e..71c6c2380443 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -11564,4 +11564,19 @@ 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_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_BUFFER_REITERATE		(1 << 29)
+#define   DSB_WAIT_FOR_VBLANK		(1 << 28)
+#define   DSB_WAIT_FOR_LINE_IN_RANGE	(1 << 27)
+#define   DSB_HALT			(1 << 16)
+#define   DSB_NON_POSTED_ENABLE		(1 << 8)
+#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 v3 06/11] 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>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 520f2bbcc8ae..d36ee8244427 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -24,6 +24,15 @@
 #define DSB_BYTE_EN			(0xf << 20)
 #define DSB_REG_VALUE_MASK		0xfffff
 
+static inline bool is_dsb_busy(struct intel_dsb *dsb)
+{
+	struct intel_crtc *crtc = dsb->crtc;
+	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)
 {
-- 
2.22.0

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

[PATCH v3 07/11] 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.

Cc: Michel Thierry <michel.thierry@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 | 40 ++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index d36ee8244427..2d6e78868f2d 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -33,6 +33,46 @@ static inline bool is_dsb_busy(struct intel_dsb *dsb)
 	return DSB_STATUS & I915_READ(DSB_CTRL(pipe, dsb->id));
 }
 
+static bool intel_dsb_enable_engine(struct intel_dsb *dsb)
+{
+	struct intel_crtc *crtc = dsb->crtc;
+	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);
+
+	return true;
+}
+
+static bool intel_dsb_disable_engine(struct intel_dsb *dsb)
+{
+	struct intel_crtc *crtc = dsb->crtc;
+	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);
+
+	return true;
+}
+
 struct intel_dsb *
 intel_dsb_get(struct intel_crtc *crtc)
 {
-- 
2.22.0

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

[PATCH v3 08/11] 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)

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 | 42 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dsb.h |  1 +
 2 files changed, 43 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 2d6e78868f2d..bc1734072f34 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -214,3 +214,45 @@ void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
 				DSB_OPCODE_SHIFT) | DSB_BYTE_EN |
 				i915_mmio_reg_offset(reg);
 }
+
+void intel_dsb_commit(struct intel_dsb *dsb)
+{
+	struct intel_crtc *crtc = dsb->crtc;
+	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_DEBUG_KMS("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_DEBUG_KMS("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 c848747f52d9..f5d1f1bedc12 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.h
+++ b/drivers/gpu/drm/i915/display/intel_dsb.h
@@ -44,5 +44,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
-- 
2.22.0

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

[PATCH v3 09/11] 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>
Acked-by: Rodrigo Vivi <rodrigo.vivi@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 3415255ad3dc..38e31223a24c 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 bc1734072f34..501ff7f5db8c 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_NOOP			0x0
@@ -73,6 +90,17 @@ static 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)
 {
@@ -124,6 +152,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;
@@ -146,6 +182,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)
 {
@@ -193,6 +242,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 = dsb->crtc;
@@ -215,6 +276,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 = dsb->crtc;
-- 
2.22.0

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

[PATCH v3 10/11] 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.

v1: Initial version.
v2: Directly call dsb-api at callsites. (Jani)

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_color.c | 64 ++++++++++++++--------
 drivers/gpu/drm/i915/i915_drv.h            |  2 +
 2 files changed, 43 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 71a0201437a9..e4090d8e4547 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -589,34 +589,38 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
 	const struct drm_color_lut *lut = blob->data;
 	int i, lut_size = drm_color_lut_size(blob);
 	enum pipe pipe = crtc->pipe;
+	struct intel_dsb *dsb = dev_priv->dsb;
 
-	I915_WRITE(PREC_PAL_INDEX(pipe), prec_index |
-		   PAL_PREC_AUTO_INCREMENT);
+	intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe),
+			    prec_index | PAL_PREC_AUTO_INCREMENT);
 
 	for (i = 0; i < hw_lut_size; i++) {
 		/* We discard half the user entries in split gamma mode */
 		const struct drm_color_lut *entry =
 			&lut[i * (lut_size - 1) / (hw_lut_size - 1)];
 
-		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(entry));
+		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
+					    ilk_lut_10(entry));
 	}
 
 	/*
 	 * Reset the index, otherwise it prevents the legacy palette to be
 	 * written properly.
 	 */
-	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
+	intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), 0);
 }
 
 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 = dev_priv->dsb;
 	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 +628,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);
 	}
 }
 
@@ -788,12 +795,13 @@ icl_load_gcmax(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);
+	struct intel_dsb *dsb = dev_priv->dsb;
 	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
@@ -803,6 +811,7 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
 	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 = dev_priv->dsb;
 	enum pipe pipe = crtc->pipe;
 	u32 i;
 
@@ -813,15 +822,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));
 	}
 }
 
@@ -833,6 +843,7 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
 	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 = dev_priv->dsb;
 	enum pipe pipe = crtc->pipe;
 	u32 i;
 
@@ -847,11 +858,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 +881,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 */
@@ -980,7 +995,10 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
 
+	dev_priv->dsb = intel_dsb_get(to_intel_crtc(crtc_state->base.crtc));
 	dev_priv->display.load_luts(crtc_state);
+	intel_dsb_commit(dev_priv->dsb);
+	intel_dsb_put(dev_priv->dsb);
 }
 
 void intel_color_commit(const struct intel_crtc_state *crtc_state)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index dd6cfb89b830..ace81ccbd766 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1759,6 +1759,8 @@ struct drm_i915_private {
 	/* Mutex to protect the above hdcp component related values. */
 	struct mutex hdcp_comp_mutex;
 
+	struct intel_dsb *dsb;
+
 	/*
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 	 * will be rejected. Instead look for a better place.
-- 
2.22.0

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

[PATCH v3 11/11] 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>
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 1974e4c78a43..613210385fb5 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

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

[Patchwork]

== Series Details ==

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

== Summary ==

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

total: 0 errors, 1 warnings, 0 checks, 137 lines checked
c0e83ac07f1b drm/i915/dsb: single register write function for DSB.
3eb1547fb3a4 drm/i915/dsb: Indexed register write function for DSB.
c22d5381cd39 drm/i915/dsb: Register definition of DSB registers.
593376017970 drm/i915/dsb: Check DSB engine status.
4072b9dacf94 drm/i915/dsb: functions to enable/disable DSB engine.
ce14930553ee drm/i915/dsb: function to trigger workload execution of DSB.
93b7b00afa8d drm/i915/dsb: Documentation for DSB.
e72f429fc2b0 drm/i915/dsb: Enable gamma lut programming using DSB.
3f02cf4823cd drm/i915/dsb: Enable DSB for gen12.

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

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

[Patchwork]

== Series Details ==

Series: DSB enablement. (rev3)
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. (rev3)

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_6794 -> Patchwork_14206
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_requests:
    - fi-byt-j1900:       [PASS][1] -> [INCOMPLETE][2] ([fdo#102657])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/fi-byt-j1900/igt@i915_selftest@live_requests.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/fi-byt-j1900/igt@i915_selftest@live_requests.html

  
#### Possible fixes ####

  * igt@kms_chamelium@dp-crc-fast:
    - fi-kbl-7500u:       [FAIL][3] ([fdo#109483] / [fdo#109635 ]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html

  
#### Warnings ####

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-icl-u2:          [FAIL][5] ([fdo#111407]) -> [FAIL][6] ([fdo#109483])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html
    - fi-kbl-7500u:       [FAIL][7] ([fdo#111407]) -> [FAIL][8] ([fdo#111096])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
  [fdo#102657]: https://bugs.freedesktop.org/show_bug.cgi?id=102657
  [fdo#109483]: https://bugs.freedesktop.org/show_bug.cgi?id=109483
  [fdo#109635 ]: https://bugs.freedesktop.org/show_bug.cgi?id=109635 
  [fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
  [fdo#111407]: https://bugs.freedesktop.org/show_bug.cgi?id=111407


Participating hosts (51 -> 43)
------------------------------

  Additional (2): fi-hsw-peppy fi-gdg-551 
  Missing    (10): fi-kbl-soraka fi-ilk-m540 fi-tgl-u fi-bsw-n3050 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_6794 -> Patchwork_14206

  CI-20190529: 20190529
  CI_DRM_6794: a1a45a21f6fef00f6150dc151c23555aa851bf74 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5150: a4e8217bcdfef9bb523f26a9084bbf615a6e8abb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14206: 3f02cf4823cdf5cf42a742179b460719c83960b6 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

3f02cf4823cd drm/i915/dsb: Enable DSB for gen12.
e72f429fc2b0 drm/i915/dsb: Enable gamma lut programming using DSB.
93b7b00afa8d drm/i915/dsb: Documentation for DSB.
ce14930553ee drm/i915/dsb: function to trigger workload execution of DSB.
4072b9dacf94 drm/i915/dsb: functions to enable/disable DSB engine.
593376017970 drm/i915/dsb: Check DSB engine status.
c22d5381cd39 drm/i915/dsb: Register definition of DSB registers.
3eb1547fb3a4 drm/i915/dsb: Indexed register write function for DSB.
c0e83ac07f1b drm/i915/dsb: single register write function for DSB.
3b8233fa3574 drm/i915/dsb: DSB context creation.
da32d51f5b17 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_14206/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sharma, Shashank]

Hello Animesh

On 8/28/2019 12:40 AM, Animesh Manna wrote:
>  From gen12 onwards Display State Buffer(DSB) is hardware capability
> added which allows driver to batch submit display HW programming.

Let's rephrase this line a bit, how about:

DSB is a new hardware capability, introduced in GEN12 display.

It allows a driver to batch-program display HW registers.

> Feature flag has_dsb added to identify the driver/platform support
> at runtime.
>
> 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/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 b42651a387d9..d32cfdb78b74 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1856,6 +1856,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); \
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 02/11] drm/i915/dsb: DSB context creation.

[Sharma, Shashank]

On 8/28/2019 12:40 AM, Animesh Manna wrote:
> The function will internally get the gem buffer from global GTT
This patch adds a function, which will internally get the gem buffer for 
DSB engine.
> which is mapped in cpu domain to feed the data + opcode for DSB engine.
This GEM buffer is from global GTT, and is mapped into CPU domain, 
contains the data + opcode to be fed 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)
>
> 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>
> 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      | 83 +++++++++++++++++++
>   drivers/gpu/drm/i915/display/intel_dsb.h      | 31 +++++++
>   drivers/gpu/drm/i915/i915_drv.h               |  1 +
>   5 files changed, 119 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 96514dcc7812..0ab3516b0044 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1026,6 +1026,9 @@ struct intel_crtc {
>   
>   	/* scalers available on this crtc */
>   	int num_scalers;
> +
> +	/* per pipe DSB related info */
> +	struct intel_dsb dsb[MAX_DSB_PER_PIPE];
>   };
>   
>   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..a2845df90573
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2019 Intel Corporation
> + *
> + */
> +
Any particular reason we don't have the traditional license here, like 
other files ?
> +#include "../i915_drv.h"
This is not the way you should include this header. Take example from 
other files in display folder.
> +#include "intel_display_types.h"
> +
> +#define DSB_BUF_SIZE    (2 * PAGE_SIZE)
Any particular reason for this 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;
> +	intel_wakeref_t wakeref;
> +	int i;
> +
> +	for (i = 0; i < MAX_DSB_PER_PIPE; i++)
> +		if (!crtc->dsb[i].cmd_buf)
> +			break;
> +
> +	WARN_ON(i >= MAX_DSB_PER_PIPE);

Its not possible to have i > MAX_DSB_PER_PIPE as per above logic, so it 
should be WARN_ON(i == MAX_DSB_PER_PIPE);

Also, shouldn't we stop operation here (along with the warning) as 
clearly the cmd_buf and dsb we are going to get, is garbage ? On 
negative testing this may cause kernel panic also.

> +
> +	dsb = &crtc->dsb[i];
> +	dsb->id = i;
> +	dsb->crtc = crtc;
> +	if (!HAS_DSB(i915))
> +		return dsb;
This check should be the first line in this function. I am not sure why 
do we want to extract dsb ptr if the platform doesn't even support DSB.
> +
> +	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_DEBUG_KMS("Vma creation failed.\n");
> +		i915_gem_object_put(obj);
Shouldn't gem_object_put() be inside mutex ?
> +		goto err;
> +	}
> +
> +	dsb->cmd_buf = i915_gem_object_pin_map(vma->obj, I915_MAP_WC);
> +	if (IS_ERR(dsb->cmd_buf)) {
> +		DRM_DEBUG_KMS("Command buffer creation failed.\n");
> +		dsb->cmd_buf = NULL;
Why don't we have a i915_gem_object_put(obj) here ?
> +		goto err;
> +	}
> +	dsb->vma = vma;
> +
> +err:
I think we should have a i915_gem_object_put(obj) for all error cases here.
> +	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> +	return dsb;
Again, we are returning a dsb ptr in all cases. As this patch doesn't 
have the caller function, I can't understand why are we returning the 
same ptr in both success or failure cases, but I would prefer a 
dsb_get() function which returns NULL on failure, dsb* on success.
> +}
> +
> +void intel_dsb_put(struct intel_dsb *dsb)
> +{
> +	struct intel_crtc *crtc;
> +	struct drm_i915_private *i915;
> +	struct i915_vma *vma;
> +
> +	if (!dsb)
> +		return;
So the get() API returns a non-NULL pointer in both success and failure 
cases, but put() can have a NULL ptr as input. Looks a bit asymmetrical  
design. Also, we should add API documentation on top of the function 
now, as we need to understand what can be the return values of the 
function.
> +
> +	crtc = dsb->crtc;
> +	i915 = to_i915(crtc->base.dev);
> +
> +	if (dsb->cmd_buf) {
> +		vma = dsb->vma;
> +		mutex_lock(&i915->drm.struct_mutex);
> +		i915_gem_object_unpin_map(vma->obj);
> +		i915_vma_unpin_and_release(&vma, 0);
> +		dsb->cmd_buf = NULL;
> +		mutex_unlock(&i915->drm.struct_mutex);
> +	}
> +}
> 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..4a4091cadc1e
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: MIT
> + *
> + * Copyright © 2019 Intel Corporation
Same as above for license.
> + */
> +
> +#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
The Bspec says there are 3 DSB DMA engines per pipe, and we have defined 
MAX_DSB_PER_PIPE = 3 (and the count starts from 0), which means we 
should be running our loops always < MAX_DSB_PER_PIPE. I would prefer 
this to be either INVALID_DSB = 0 and DSB3 = MAX_DSB = 3, but this is 
just a soft requirement, you can pick or ignore.
> +};
> +
> +struct intel_dsb {
> +	struct intel_crtc *crtc;
> +	enum dsb_id id;
Note that your INVALID_DSB id is -1 not 0. This means all the intel_dsb 
structures will be initialized with id=DSB1 not INVALID_DSB.
> +	u32 *cmd_buf;
> +	struct i915_vma *vma;
> +};
> +
This could also be the place to add doc about the get/put functions.
> +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 d32cfdb78b74..dd6cfb89b830 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"

No external element is using get() / put() functions yet, this change 
should go into a patch, which is using the functions.

- Shashank

>   #include "display/intel_frontbuffer.h"
>   #include "display/intel_gmbus.h"
>   #include "display/intel_opregion.h"
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sharma, Shashank]

On 8/28/2019 12:40 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.
>
> 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 | 36 ++++++++++++++++++++++++
>   drivers/gpu/drm/i915/display/intel_dsb.h |  9 ++++++
>   2 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 a2845df90573..df288446caeb 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -9,6 +9,20 @@
>   
>   #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
>   
> +/* DSB opcodes. */
> +#define DSB_OPCODE_SHIFT		24
> +#define DSB_OPCODE_NOOP			0x0
> +#define DSB_OPCODE_MMIO_WRITE		0x1
> +#define DSB_OPCODE_WAIT_FOR_US		0x2
> +#define DSB_OPCODE_WAIT_FOR_LINES	0x3
> +#define DSB_OPCODE_WAIT_FOR_VBLANK	0x4
> +#define DSB_OPCODE_WAIT_FOR_SL_IN	0x5
May be SL_IR (to indicate in range)
> +#define DSB_OPCODE_WAIT_FOR_SL_OUT	0x6
May be SL_OOR (out of range) or _OR
> +#define DSB_OPCODE_GENERATE_INT		0x7
> +#define DSB_OPCODE_INDEXED_WRITE	0x9
> +#define DSB_OPCODE_POLL			0xA
> +#define DSB_BYTE_EN			(0xf << 20)

But its a nitpick but as we are going by shifts for OPCODE, may be would look consistent to create 2 different macros for enable also.

#define DSB_BYTE_EN			0xF

#define DSB_BYTE_EN_SHIFT		20

   

> +

This patch is using only 3 of the defined commands: 
DSB_OPCODE_MMIO_WRITE, DSB_OPCODE_SHIFT, DSB_BYTE_EN

All other definitions are unused. Please add them only in the patch 
where they are being used.

>   struct intel_dsb *
>   intel_dsb_get(struct intel_crtc *crtc)
>   {
> @@ -81,3 +95,25 @@ void intel_dsb_put(struct intel_dsb *dsb)
>   		mutex_unlock(&i915->drm.struct_mutex);
>   	}
>   }
> +
> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
> +{
> +	struct intel_crtc *crtc = dsb->crtc;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 *buf = dsb->cmd_buf;
> +
Do we need a HAS_DSB() check here ? Or would it be taken care in the 
caller ?
> +	if (!buf) {
> +		I915_WRITE(reg, val);

We can debate on if this is a good idea to fallback to I915_write() if 
DSB is not available. May be we should think about why are we seeing 
this condition (!buf), and that can be checked only when I see the caller.

For now, lets assume we will fallback to I915_WRITE();

> +		return;
> +	}
> +
> +	if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
> +		DRM_DEBUG_KMS("DSB buffer overflow.\n");
Wouldn't it make sense to do a I915_WRITE(reg, val) here also, like above ?
> +		return;
> +	}
> +
> +	buf[dsb->free_pos++] = val;
> +	buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE  <<
> +				DSB_OPCODE_SHIFT) | DSB_BYTE_EN |
> +				i915_mmio_reg_offset(reg);
Once the write is done, are we planning to reset this free_pos ? May be 
in some upcoming patch in the series ?
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> index 4a4091cadc1e..1b33ab118640 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;
>   
> @@ -22,10 +24,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 cmd_buf_tail.
> +	 */
> +	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 v3 04/11] drm/i915/dsb: Indexed register write function for DSB.

[Sharma, Shashank]

On 8/28/2019 12:40 AM, Animesh Manna wrote:
> DSB can program large set of data through indexed register write
> (opcode 0x9) in one shot. Will be using for bulk register programming
Reshuffle-> This feature can be used for bulk register programming e.g.
> e.g. gamma lut programming, HDR meta data programming.
>
> v1: Initial version.
>
> v2: simplified code by using ALIGN(). (Chris)
>
> 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 | 48 ++++++++++++++++++++++++
>   drivers/gpu/drm/i915/display/intel_dsb.h |  8 ++++
>   2 files changed, 56 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index df288446caeb..520f2bbcc8ae 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -22,6 +22,7 @@
>   #define DSB_OPCODE_INDEXED_WRITE	0x9
>   #define DSB_OPCODE_POLL			0xA
>   #define DSB_BYTE_EN			(0xf << 20)
> +#define DSB_REG_VALUE_MASK		0xfffff
>   
>   struct intel_dsb *
>   intel_dsb_get(struct intel_crtc *crtc)
> @@ -96,6 +97,53 @@ void intel_dsb_put(struct intel_dsb *dsb)
>   	}
>   }
>   
> +void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
> +				 u32 val)
We might need one space here, to get this aligned to start of the line 
above (or is this due to my mail client ?).
> +{
> +	struct intel_crtc *crtc = dsb->crtc;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 *buf = dsb->cmd_buf;
> +	u32 reg_val;
> +
- Do we need a HAS_DSB check at start or the caller will take care of it ?
> +	if (!buf) {
> +		I915_WRITE(reg, val);
I am under the assumption that indexed reg write is to write multiple 
registers, and 'reg' is the base value of the register set. I dont think 
it makes sense to write a single register here in case of DSB failure ?
> +		return;
> +	}
> +
> +	if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
> +		DRM_DEBUG_KMS("DSB buffer overflow.\n");
> +		return;
> +	}
> +	reg_val = buf[dsb->ins_start_offset + 1] & DSB_REG_VALUE_MASK;

Why do we have this +1 here ? on the very first run (when 
ins_start_offset is 0), this will fetch reg_val = buf[1]; Is this what 
we want to do ?

> +	if (reg_val != i915_mmio_reg_offset(reg)) {
> +		/* Every instruction should be 8 byte aligned. */
> +		dsb->free_pos = ALIGN(dsb->free_pos, 2);
The comment says that you want the position to be 8 bytes align, but the 
factor sent to the macro is 2 ?
> +
> +		/* Update the size. */
> +		dsb->ins_start_offset = dsb->free_pos;
This comment is irrelevant, you are not updating the size, you are 
caching the base memory location.
> +		buf[dsb->free_pos++] = 1;

What does this indicate ? What is 1 ?

It would be great if you can add an aski graph/table and explain what 
does this DSB command buffer actually contains as per your design.

> +
> +		/* 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]++;
> +	}

The code is looking unnecessarily complicated as ins_start_offset and 
free_pos are being used for multiple reasons. I guess you can use some 
local variables like:

u32 base = count = ALIGN();

  {

     buf [count++] = size;

    buf [count++] = command;

   buf [count++] = value;

}

dsb->start_offset = base;

dsb->free_pos = count;

> +
> +	/* 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 = dsb->crtc;
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> index 1b33ab118640..c848747f52d9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -30,11 +30,19 @@ struct intel_dsb {
>   	 * and help in calculating cmd_buf_tail.
>   	 */
>   	int free_pos;
> +
> +	/*
> +	 * ins_start_offset will help to store start address
> +	 * of the dsb instuction of auto-increment register.
> +	 */
> +	u32 ins_start_offset;
How about the variable to be named as base_offset ? Also, do we need to 
keep this saved, even when the writing is done ?
>   };
>   
>   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);

Cross check the alignment of second line.

- Shashank

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

Re: [PATCH v3 05/11] drm/i915/dsb: Register definition of DSB registers.

[Sharma, Shashank]

On 8/28/2019 12:40 AM, Animesh Manna wrote:
> Added key register definitions of DSB.
>
> dsb-ctrl register is required to enable dsb-engine.
>
> head-ptr register hold the head of buffer address from where the
> execution will start.
>
> Programming tail-ptr register is a trigger point to start execution.
>
> Cc: Uma Shankar <uma.shankar@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/i915_reg.h | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 02e1ef10c47e..71c6c2380443 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -11564,4 +11564,19 @@ 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_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_BUFFER_REITERATE		(1 << 29)
> +#define   DSB_WAIT_FOR_VBLANK		(1 << 28)
> +#define   DSB_WAIT_FOR_LINE_IN_RANGE	(1 << 27)
> +#define   DSB_HALT			(1 << 16)
> +#define   DSB_NON_POSTED_ENABLE		(1 << 8)
> +#define   DSB_STATUS			(1 << 0)
> +

Again, this patch is just adding the header definitions, please merge 
this patch in the patch where these definitions are being used.

- Shashank

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

Re: [PATCH v3 07/11] drm/i915/dsb: functions to enable/disable DSB engine.

[Sharma, Shashank]

On 8/28/2019 12:40 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.
>
> Cc: Michel Thierry <michel.thierry@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 | 40 ++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index d36ee8244427..2d6e78868f2d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -33,6 +33,46 @@ static inline bool is_dsb_busy(struct intel_dsb *dsb)
>   	return DSB_STATUS & I915_READ(DSB_CTRL(pipe, dsb->id));
>   }
>   
> +static bool intel_dsb_enable_engine(struct intel_dsb *dsb)
> +{
> +	struct intel_crtc *crtc = dsb->crtc;
> +	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);
readback DSB status and confirm if that's really enabled ?
> +
> +	return true;
> +}
> +
> +static bool intel_dsb_disable_engine(struct intel_dsb *dsb)
> +{
> +	struct intel_crtc *crtc = dsb->crtc;
> +	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);
> +

Same as above.

- Shashank

> +	return true;
> +}
> +
>   struct intel_dsb *
>   intel_dsb_get(struct intel_crtc *crtc)
>   {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 08/11] drm/i915/dsb: function to trigger workload execution of DSB.

[Sharma, Shashank]

On 8/28/2019 12:40 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)
>
> 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 | 42 ++++++++++++++++++++++++
>   drivers/gpu/drm/i915/display/intel_dsb.h |  1 +
>   2 files changed, 43 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 2d6e78868f2d..bc1734072f34 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -214,3 +214,45 @@ void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>   				DSB_OPCODE_SHIFT) | DSB_BYTE_EN |
>   				i915_mmio_reg_offset(reg);
>   }
> +
> +void intel_dsb_commit(struct intel_dsb *dsb)
> +{
> +	struct intel_crtc *crtc = dsb->crtc;
> +	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;
How would the user know if DSB commit failed, and it has to retry ? 
should we add a return value ?
> +
> +	if (!intel_dsb_enable_engine(dsb))
> +		goto reset;
> +
> +	if (is_dsb_busy(dsb)) {
> +		DRM_DEBUG_KMS("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_DEBUG_KMS("TAIL_PTR write failed - dsb engine is busy.\n");
> +		goto reset;
> +	}
a space here will look good, and aligned to above.
> +	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)) {
This time limit of 1 is bspec recommended, or its by experience ?
> +		DRM_ERROR("Timed out waiting for DSB workload completion.\n");
> +		goto reset;
> +	}
> +
> +reset:
> +	dsb->free_pos = 0;
reset is just doing dsb->free_pos = 0; is that enough ? I think we need 
to do a complete reset of the buffer memset(dsb->buffer, 0) or something 
similar, else batch buffer will contain garbage values from previous 
weites.
> +	intel_dsb_disable_engine(dsb);
We still want to return a -ve value to indicate the write failed.
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> index c848747f52d9..f5d1f1bedc12 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -44,5 +44,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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 09/11] drm/i915/dsb: Documentation for DSB.

[Sharma, Shashank]

So here is the documentation, this resolves atleast one review comment 
from me :)

- Shashank

On 8/28/2019 12:40 AM, Animesh Manna wrote:
> 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>
> Acked-by: Rodrigo Vivi <rodrigo.vivi@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 3415255ad3dc..38e31223a24c 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 bc1734072f34..501ff7f5db8c 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_NOOP			0x0
> @@ -73,6 +90,17 @@ static 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)
>   {
> @@ -124,6 +152,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;
> @@ -146,6 +182,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)
>   {
> @@ -193,6 +242,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 = dsb->crtc;
> @@ -215,6 +276,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 = dsb->crtc;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sharma, Shashank]

On 8/28/2019 12:40 AM, Animesh Manna 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.
>
> v1: Initial version.
> v2: Directly call dsb-api at callsites. (Jani)
>
> 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_color.c | 64 ++++++++++++++--------
>   drivers/gpu/drm/i915/i915_drv.h            |  2 +
>   2 files changed, 43 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 71a0201437a9..e4090d8e4547 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -589,34 +589,38 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
>   	const struct drm_color_lut *lut = blob->data;
>   	int i, lut_size = drm_color_lut_size(blob);
>   	enum pipe pipe = crtc->pipe;
> +	struct intel_dsb *dsb = dev_priv->dsb;
>   
> -	I915_WRITE(PREC_PAL_INDEX(pipe), prec_index |
> -		   PAL_PREC_AUTO_INCREMENT);
I dont see any gen check in any of the functions before calling dsb() 
functions.
> +	intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe),
> +			    prec_index | PAL_PREC_AUTO_INCREMENT);
>   
>   	for (i = 0; i < hw_lut_size; i++) {
>   		/* We discard half the user entries in split gamma mode */
>   		const struct drm_color_lut *entry =
>   			&lut[i * (lut_size - 1) / (hw_lut_size - 1)];
>   
> -		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(entry));
> +		intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
> +					    ilk_lut_10(entry));
>   	}
>   
>   	/*
>   	 * Reset the index, otherwise it prevents the legacy palette to be
>   	 * written properly.
>   	 */
> -	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
> +	intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), 0);
>   }
>   
>   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 = dev_priv->dsb;
>   	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 +628,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);
>   	}
>   }
>   
> @@ -788,12 +795,13 @@ icl_load_gcmax(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);
> +	struct intel_dsb *dsb = dev_priv->dsb;
>   	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
> @@ -803,6 +811,7 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>   	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 = dev_priv->dsb;
>   	enum pipe pipe = crtc->pipe;
>   	u32 i;
>   
> @@ -813,15 +822,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));
>   	}
>   }
>   
> @@ -833,6 +843,7 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>   	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 = dev_priv->dsb;
>   	enum pipe pipe = crtc->pipe;
>   	u32 i;
>   
> @@ -847,11 +858,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 +881,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 */
> @@ -980,7 +995,10 @@ void intel_color_load_luts(const struct intel_crtc_state *crtc_state)
>   {
>   	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>   
> +	dev_priv->dsb = intel_dsb_get(to_intel_crtc(crtc_state->base.crtc));
>   	dev_priv->display.load_luts(crtc_state);
> +	intel_dsb_commit(dev_priv->dsb);
> +	intel_dsb_put(dev_priv->dsb);
>   }
>   
>   void intel_color_commit(const struct intel_crtc_state *crtc_state)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index dd6cfb89b830..ace81ccbd766 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1759,6 +1759,8 @@ struct drm_i915_private {
>   	/* Mutex to protect the above hdcp component related values. */
>   	struct mutex hdcp_comp_mutex;
>   
> +	struct intel_dsb *dsb;
> +
>   	/*
>   	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
>   	 * will be rejected. Instead look for a better place.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Animesh Manna]

On 8/28/2019 7:31 PM, Sharma, Shashank wrote:
> Hello Animesh
>
> On 8/28/2019 12:40 AM, Animesh Manna wrote:
>>  From gen12 onwards Display State Buffer(DSB) is hardware capability
>> added which allows driver to batch submit display HW programming.
>
> Let's rephrase this line a bit, how about:
>
> DSB is a new hardware capability, introduced in GEN12 display.
>
> It allows a driver to batch-program display HW registers.

ok.

>
>
>> Feature flag has_dsb added to identify the driver/platform support
>> at runtime.
>>
>> 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/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 b42651a387d9..d32cfdb78b74 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1856,6 +1856,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); \

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

✓ Fi.CI.IGT: success for DSB enablement. (rev3)

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_6794_full -> Patchwork_14206_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_reloc@basic-cpu-gtt-active:
    - shard-skl:          [PASS][1] -> [DMESG-WARN][2] ([fdo#106107])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-skl6/igt@gem_exec_reloc@basic-cpu-gtt-active.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-skl9/igt@gem_exec_reloc@basic-cpu-gtt-active.html

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

  * igt@gem_workarounds@suspend-resume-context:
    - shard-apl:          [PASS][5] -> [DMESG-WARN][6] ([fdo#108566]) +4 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-apl3/igt@gem_workarounds@suspend-resume-context.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-apl7/igt@gem_workarounds@suspend-resume-context.html

  * igt@kms_cursor_crc@pipe-a-cursor-128x42-onscreen:
    - shard-kbl:          [PASS][7] -> [FAIL][8] ([fdo#103232])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-kbl1/igt@kms_cursor_crc@pipe-a-cursor-128x42-onscreen.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-kbl4/igt@kms_cursor_crc@pipe-a-cursor-128x42-onscreen.html

  * igt@kms_flip@basic-flip-vs-modeset:
    - shard-apl:          [PASS][9] -> [INCOMPLETE][10] ([fdo#103927])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-apl1/igt@kms_flip@basic-flip-vs-modeset.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-apl5/igt@kms_flip@basic-flip-vs-modeset.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-snb:          [PASS][11] -> [INCOMPLETE][12] ([fdo#105411])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-snb1/igt@kms_flip@flip-vs-suspend-interruptible.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-snb1/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-stridechange:
    - shard-hsw:          [PASS][13] -> [INCOMPLETE][14] ([fdo#103540]) +1 similar issue
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-hsw6/igt@kms_frontbuffer_tracking@fbc-stridechange.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-hsw7/igt@kms_frontbuffer_tracking@fbc-stridechange.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-wc:
    - shard-skl:          [PASS][15] -> [FAIL][16] ([fdo#103167]) +2 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-skl1/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-wc.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-skl10/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-wc.html

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

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
    - shard-skl:          [PASS][19] -> [INCOMPLETE][20] ([fdo#104108])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-skl1/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-skl10/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html

  * igt@kms_psr2_su@frontbuffer:
    - shard-iclb:         [PASS][21] -> [SKIP][22] ([fdo#109642] / [fdo#111068])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-iclb2/igt@kms_psr2_su@frontbuffer.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-iclb4/igt@kms_psr2_su@frontbuffer.html

  * igt@kms_psr@psr2_sprite_plane_move:
    - shard-iclb:         [PASS][23] -> [SKIP][24] ([fdo#109441]) +2 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-iclb2/igt@kms_psr@psr2_sprite_plane_move.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-iclb3/igt@kms_psr@psr2_sprite_plane_move.html

  * igt@prime_vgem@fence-wait-bsd2:
    - shard-iclb:         [PASS][25] -> [SKIP][26] ([fdo#109276]) +20 similar issues
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-iclb2/igt@prime_vgem@fence-wait-bsd2.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-iclb6/igt@prime_vgem@fence-wait-bsd2.html

  
#### Possible fixes ####

  * igt@gem_ctx_shared@exec-single-timeline-bsd:
    - shard-iclb:         [SKIP][27] ([fdo#110841]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-iclb4/igt@gem_ctx_shared@exec-single-timeline-bsd.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-iclb3/igt@gem_ctx_shared@exec-single-timeline-bsd.html

  * igt@gem_eio@reset-stress:
    - shard-snb:          [FAIL][29] ([fdo#109661]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-snb5/igt@gem_eio@reset-stress.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-snb7/igt@gem_eio@reset-stress.html

  * igt@gem_exec_schedule@preempt-contexts-bsd2:
    - shard-iclb:         [SKIP][31] ([fdo#109276]) -> [PASS][32] +17 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-iclb6/igt@gem_exec_schedule@preempt-contexts-bsd2.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-iclb4/igt@gem_exec_schedule@preempt-contexts-bsd2.html

  * igt@gem_exec_schedule@reorder-wide-bsd:
    - shard-iclb:         [SKIP][33] ([fdo#111325]) -> [PASS][34] +9 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-iclb1/igt@gem_exec_schedule@reorder-wide-bsd.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-iclb7/igt@gem_exec_schedule@reorder-wide-bsd.html

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - shard-apl:          [DMESG-WARN][35] ([fdo#108566]) -> [PASS][36] +3 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-apl7/igt@i915_suspend@fence-restore-tiled2untiled.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-apl4/igt@i915_suspend@fence-restore-tiled2untiled.html

  * igt@kms_color@pipe-b-ctm-blue-to-red:
    - shard-skl:          [FAIL][37] ([fdo#107201]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-skl3/igt@kms_color@pipe-b-ctm-blue-to-red.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-skl7/igt@kms_color@pipe-b-ctm-blue-to-red.html

  * igt@kms_cursor_crc@pipe-a-cursor-128x42-random:
    - shard-skl:          [FAIL][39] ([fdo#103232]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-skl7/igt@kms_cursor_crc@pipe-a-cursor-128x42-random.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-skl9/igt@kms_cursor_crc@pipe-a-cursor-128x42-random.html

  * igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic:
    - shard-glk:          [FAIL][41] ([fdo#104873]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-glk9/igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-glk7/igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic.html

  * igt@kms_cursor_legacy@cursor-vs-flip-toggle:
    - shard-hsw:          [FAIL][43] ([fdo#103355]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-hsw6/igt@kms_cursor_legacy@cursor-vs-flip-toggle.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-hsw1/igt@kms_cursor_legacy@cursor-vs-flip-toggle.html

  * igt@kms_cursor_legacy@flip-vs-cursor-varying-size:
    - shard-skl:          [FAIL][45] ([fdo#102670] / [fdo#106081]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-skl2/igt@kms_cursor_legacy@flip-vs-cursor-varying-size.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-skl7/igt@kms_cursor_legacy@flip-vs-cursor-varying-size.html

  * igt@kms_frontbuffer_tracking@fbc-indfb-scaledprimary:
    - shard-iclb:         [FAIL][47] ([fdo#103167]) -> [PASS][48] +2 similar issues
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-iclb4/igt@kms_frontbuffer_tracking@fbc-indfb-scaledprimary.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-iclb3/igt@kms_frontbuffer_tracking@fbc-indfb-scaledprimary.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-iclb:         [FAIL][49] ([fdo#103166]) -> [PASS][50] +1 similar issue
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-iclb5/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-iclb2/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@kms_psr@psr2_sprite_mmap_cpu:
    - shard-iclb:         [SKIP][51] ([fdo#109441]) -> [PASS][52] +1 similar issue
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-iclb5/igt@kms_psr@psr2_sprite_mmap_cpu.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-iclb2/igt@kms_psr@psr2_sprite_mmap_cpu.html

  * igt@kms_setmode@basic:
    - shard-apl:          [FAIL][53] ([fdo#99912]) -> [PASS][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-apl5/igt@kms_setmode@basic.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-apl1/igt@kms_setmode@basic.html
    - shard-hsw:          [FAIL][55] ([fdo#99912]) -> [PASS][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-hsw4/igt@kms_setmode@basic.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-hsw7/igt@kms_setmode@basic.html

  * igt@perf@blocking:
    - shard-skl:          [FAIL][57] ([fdo#110728]) -> [PASS][58] +1 similar issue
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6794/shard-skl2/igt@perf@blocking.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14206/shard-skl7/igt@perf@blocking.html

  
#### Warnings ####

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

  
  [fdo#102670]: https://bugs.freedesktop.org/show_bug.cgi?id=102670
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [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#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#104873]: https://bugs.freedesktop.org/show_bug.cgi?id=104873
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#106081]: https://bugs.freedesktop.org/show_bug.cgi?id=106081
  [fdo#106107]: https://bugs.freedesktop.org/show_bug.cgi?id=106107
  [fdo#107201]: https://bugs.freedesktop.org/show_bug.cgi?id=107201
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#109661]: https://bugs.freedesktop.org/show_bug.cgi?id=109661
  [fdo#110728]: https://bugs.freedesktop.org/show_bug.cgi?id=110728
  [fdo#110841]: https://bugs.freedesktop.org/show_bug.cgi?id=110841
  [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#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_6794 -> Patchwork_14206

  CI-20190529: 20190529
  CI_DRM_6794: a1a45a21f6fef00f6150dc151c23555aa851bf74 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5150: a4e8217bcdfef9bb523f26a9084bbf615a6e8abb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_14206: 3f02cf4823cdf5cf42a742179b460719c83960b6 @ 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_14206/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 02/11] drm/i915/dsb: DSB context creation.

[Animesh Manna]

Hi,


On 8/28/2019 8:09 PM, Sharma, Shashank wrote:
>
> On 8/28/2019 12:40 AM, Animesh Manna wrote:
>> The function will internally get the gem buffer from global GTT
> This patch adds a function, which will internally get the gem buffer 
> for DSB engine.
>> which is mapped in cpu domain to feed the data + opcode for DSB engine.
> This GEM buffer is from global GTT, and is mapped into CPU domain, 
> contains the data + opcode to be fed to DSB engine.

ok.

>>
>> 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)
>>
>> 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>
>> 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      | 83 +++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_dsb.h      | 31 +++++++
>>   drivers/gpu/drm/i915/i915_drv.h               |  1 +
>>   5 files changed, 119 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 96514dcc7812..0ab3516b0044 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -1026,6 +1026,9 @@ struct intel_crtc {
>>         /* scalers available on this crtc */
>>       int num_scalers;
>> +
>> +    /* per pipe DSB related info */
>> +    struct intel_dsb dsb[MAX_DSB_PER_PIPE];
>>   };
>>     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..a2845df90573
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>> @@ -0,0 +1,83 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2019 Intel Corporation
>> + *
>> + */
>> +
> Any particular reason we don't have the traditional license here, like 
> other files ?

Current trend I hope to use SPDX header otherwise checkpatch is throwing 
warning/error.

>> +#include "../i915_drv.h"
> This is not the way you should include this header. Take example from 
> other files in display folder.
#include "i915_drv.h"
rt?
>
>> +#include "intel_display_types.h"
>> +
>> +#define DSB_BUF_SIZE    (2 * PAGE_SIZE)
> Any particular reason for this size ?

one page is not sufficient and do not want to use too many pages.

>
>> +
>> +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;
>> +    intel_wakeref_t wakeref;
>> +    int i;
>> +
>> +    for (i = 0; i < MAX_DSB_PER_PIPE; i++)
>> +        if (!crtc->dsb[i].cmd_buf)
>> +            break;
>> +
>> +    WARN_ON(i >= MAX_DSB_PER_PIPE);
>
> Its not possible to have i > MAX_DSB_PER_PIPE as per above logic, so 
> it should be WARN_ON(i == MAX_DSB_PER_PIPE);

ok.

>
> Also, shouldn't we stop operation here (along with the warning) as 
> clearly the cmd_buf and dsb we are going to get, is garbage ? On 
> negative testing this may cause kernel panic also.

I feel for erroneous case, as DSB is not handled properly, better to 
halt the kernel.
Good idea to return NULL, will do it.

>
>> +
>> +    dsb = &crtc->dsb[i];
>> +    dsb->id = i;
>> +    dsb->crtc = crtc;
>> +    if (!HAS_DSB(i915))
>> +        return dsb;
> This check should be the first line in this function. I am not sure 
> why do we want to extract dsb ptr if the platform doesn't even support 
> DSB.

As per agreed design with Jani we want to replace I915_WRITE with 
dsp-write api for all platform and for older platform will fallback to 
i915_WRITE call.

>> +
>> +    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_DEBUG_KMS("Vma creation failed.\n");
>> +        i915_gem_object_put(obj);
> Shouldn't gem_object_put() be inside mutex ?

Tried to refer existing code to understand the need, may not need it.

>> +        goto err;
>> +    }
>> +
>> +    dsb->cmd_buf = i915_gem_object_pin_map(vma->obj, I915_MAP_WC);
>> +    if (IS_ERR(dsb->cmd_buf)) {
>> +        DRM_DEBUG_KMS("Command buffer creation failed.\n");
>> +        dsb->cmd_buf = NULL;
> Why don't we have a i915_gem_object_put(obj) here ?

Will add in err-section.
>
>> +        goto err;
>> +    }
>> +    dsb->vma = vma;
>> +
>> +err:
> I think we should have a i915_gem_object_put(obj) for all error cases 
> here.

Ok.

>> + intel_runtime_pm_put(&i915->runtime_pm, wakeref);
>> +    return dsb;
> Again, we are returning a dsb ptr in all cases. As this patch doesn't 
> have the caller function, I can't understand why are we returning the 
> same ptr in both success or failure cases, but I would prefer a 
> dsb_get() function which returns NULL on failure, dsb* on success.

Explained above.

>> +}
>> +
>> +void intel_dsb_put(struct intel_dsb *dsb)
>> +{
>> +    struct intel_crtc *crtc;
>> +    struct drm_i915_private *i915;
>> +    struct i915_vma *vma;
>> +
>> +    if (!dsb)
>> +        return;
> So the get() API returns a non-NULL pointer in both success and 
> failure cases, but put() can have a NULL ptr as input. Looks a bit 
> asymmetrical  design. Also, we should add API documentation on top of 
> the function now, as we need to understand what can be the return 
> values of the function.

As discussed will send NULL if user ask for DSB after reaching 
MAX_DSB_PER_PIPE, we can keep the null check.

>> +
>> +    crtc = dsb->crtc;
>> +    i915 = to_i915(crtc->base.dev);
>> +
>> +    if (dsb->cmd_buf) {
>> +        vma = dsb->vma;
>> +        mutex_lock(&i915->drm.struct_mutex);
>> +        i915_gem_object_unpin_map(vma->obj);
>> +        i915_vma_unpin_and_release(&vma, 0);
>> +        dsb->cmd_buf = NULL;
>> +        mutex_unlock(&i915->drm.struct_mutex);
>> +    }
>> +}
>> 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..4a4091cadc1e
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>> @@ -0,0 +1,31 @@
>> +/* SPDX-License-Identifier: MIT
>> + *
>> + * Copyright © 2019 Intel Corporation
> Same as above for license.

Explained above.

>> + */
>> +
>> +#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
> The Bspec says there are 3 DSB DMA engines per pipe, and we have 
> defined MAX_DSB_PER_PIPE = 3 (and the count starts from 0), which 
> means we should be running our loops always < MAX_DSB_PER_PIPE. I 
> would prefer this to be either INVALID_DSB = 0 and DSB3 = MAX_DSB = 3, 
> but this is just a soft requirement, you can pick or ignore.

dsb_id is used to internally to access DSB registers.

>> +};
>> +
>> +struct intel_dsb {
>> +    struct intel_crtc *crtc;
>> +    enum dsb_id id;
> Note that your INVALID_DSB id is -1 not 0. This means all the 
> intel_dsb structures will be initialized with id=DSB1 not INVALID_DSB.

I feel it is similar like enum used for pipe, port etc also not 
expecting any invalid usage.

>> +    u32 *cmd_buf;
>> +    struct i915_vma *vma;
>> +};
>> +
> This could also be the place to add doc about the get/put functions.

Initially added per function basis and as per previous feedback created 
a separate patch only for docbook.

>> +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 d32cfdb78b74..dd6cfb89b830 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"
>
> No external element is using get() / put() functions yet, this change 
> should go into a patch, which is using the functions.

Understood your concern, the change will be little big having all 
dsp-api and its usage together, so splited as per different dsp-api and 
later a separate patch is created to enable gamma through DSB.

Regards,
Animesh

>
> - Shashank
>
>>   #include "display/intel_frontbuffer.h"
>>   #include "display/intel_gmbus.h"
>>   #include "display/intel_opregion.h"

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

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

[Animesh Manna]

Hi,


On 8/28/2019 8:46 PM, Sharma, Shashank wrote:
>
> On 8/28/2019 12:40 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.
>>
>> 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 | 36 ++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_dsb.h |  9 ++++++
>>   2 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 a2845df90573..df288446caeb 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>> @@ -9,6 +9,20 @@
>>     #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
>>   +/* DSB opcodes. */
>> +#define DSB_OPCODE_SHIFT        24
>> +#define DSB_OPCODE_NOOP            0x0
>> +#define DSB_OPCODE_MMIO_WRITE        0x1
>> +#define DSB_OPCODE_WAIT_FOR_US        0x2
>> +#define DSB_OPCODE_WAIT_FOR_LINES    0x3
>> +#define DSB_OPCODE_WAIT_FOR_VBLANK    0x4
>> +#define DSB_OPCODE_WAIT_FOR_SL_IN    0x5
> May be SL_IR (to indicate in range)
>> +#define DSB_OPCODE_WAIT_FOR_SL_OUT    0x6
> May be SL_OOR (out of range) or _OR
>> +#define DSB_OPCODE_GENERATE_INT 0x7
>> +#define DSB_OPCODE_INDEXED_WRITE    0x9
>> +#define DSB_OPCODE_POLL            0xA
>> +#define DSB_BYTE_EN            (0xf << 20)
>
> But its a nitpick but as we are going by shifts for OPCODE, may be 
> would look consistent to create 2 different macros for enable also.
>
> #define DSB_BYTE_EN            0xF
>
> #define DSB_BYTE_EN_SHIFT        20

Ok.

>
>
>> +
>
> This patch is using only 3 of the defined commands: 
> DSB_OPCODE_MMIO_WRITE, DSB_OPCODE_SHIFT, DSB_BYTE_EN
>
> All other definitions are unused. Please add them only in the patch 
> where they are being used.

Ok.

>
>>   struct intel_dsb *
>>   intel_dsb_get(struct intel_crtc *crtc)
>>   {
>> @@ -81,3 +95,25 @@ void intel_dsb_put(struct intel_dsb *dsb)
>>           mutex_unlock(&i915->drm.struct_mutex);
>>       }
>>   }
>> +
>> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 
>> val)
>> +{
>> +    struct intel_crtc *crtc = dsb->crtc;
>> +    struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +    u32 *buf = dsb->cmd_buf;
>> +
> Do we need a HAS_DSB() check here ? Or would it be taken care in the 
> caller ?

After passing from HAS_DSB() check will get the dsb->cmd_buf, so having 
non-null cmd_buf implies HAS_DSB is passed.

>> +    if (!buf) {
>> +        I915_WRITE(reg, val);
>
> We can debate on if this is a good idea to fallback to I915_write() if 
> DSB is not available. May be we should think about why are we seeing 
> this condition (!buf), and that can be checked only when I see the 
> caller.
>
> For now, lets assume we will fallback to I915_WRITE();

Having (!buf) check will help to identify platform which are not having 
DSB support.
As per agreed design with Jani we want to replace I915_WRITE with 
dsp-write api for all platform and for older platform will fallback to 
i915_WRITE call.

>
>> +        return;
>> +    }
>> +
>> +    if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
>> +        DRM_DEBUG_KMS("DSB buffer overflow.\n");
> Wouldn't it make sense to do a I915_WRITE(reg, val) here also, like 
> above ?

Wanted to be capture this error and don't want to pass through fallback 
mechanism.
Based on need will fine tune DSB buffer size in future if needed.

>
>> +        return;
>> +    }
>> +
>> +    buf[dsb->free_pos++] = val;
>> +    buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE  <<
>> +                DSB_OPCODE_SHIFT) | DSB_BYTE_EN |
>> +                i915_mmio_reg_offset(reg);
> Once the write is done, are we planning to reset this free_pos ? May 
> be in some upcoming patch in the series ?

Yes, after commit will be done.

Regards,
Animesh

>> +}
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h 
>> b/drivers/gpu/drm/i915/display/intel_dsb.h
>> index 4a4091cadc1e..1b33ab118640 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;
>>   @@ -22,10 +24,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 cmd_buf_tail.
>> +     */
>> +    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 v3 04/11] drm/i915/dsb: Indexed register write function for DSB.

[Animesh Manna]

Hi,


On 8/28/2019 10:16 PM, Sharma, Shashank wrote:
>
> On 8/28/2019 12:40 AM, Animesh Manna wrote:
>> DSB can program large set of data through indexed register write
>> (opcode 0x9) in one shot. Will be using for bulk register programming
> Reshuffle-> This feature can be used for bulk register programming e.g.

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

Hope it is fine now?

>
>> e.g. gamma lut programming, HDR meta data programming.
>>
>> v1: Initial version.
>>
>> v2: simplified code by using ALIGN(). (Chris)
>>
>> 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 | 48 ++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_dsb.h |  8 ++++
>>   2 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c 
>> b/drivers/gpu/drm/i915/display/intel_dsb.c
>> index df288446caeb..520f2bbcc8ae 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>> @@ -22,6 +22,7 @@
>>   #define DSB_OPCODE_INDEXED_WRITE    0x9
>>   #define DSB_OPCODE_POLL            0xA
>>   #define DSB_BYTE_EN            (0xf << 20)
>> +#define DSB_REG_VALUE_MASK        0xfffff
>>     struct intel_dsb *
>>   intel_dsb_get(struct intel_crtc *crtc)
>> @@ -96,6 +97,53 @@ void intel_dsb_put(struct intel_dsb *dsb)
>>       }
>>   }
>>   +void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t 
>> reg,
>> +                 u32 val)
> We might need one space here, to get this aligned to start of the line 
> above (or is this due to my mail client ?).

After applying the patch looks ok maybe some editor issue.

>> +{
>> +    struct intel_crtc *crtc = dsb->crtc;
>> +    struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +    u32 *buf = dsb->cmd_buf;
>> +    u32 reg_val;
>> +
> - Do we need a HAS_DSB check at start or the caller will take care of 
> it ?

As explained before non-null buf implies HAS_DSB check is passed.

>> +    if (!buf) {
>> +        I915_WRITE(reg, val);
> I am under the assumption that indexed reg write is to write multiple 
> registers, and 'reg' is the base value of the register set. I dont 
> think it makes sense to write a single register here in case of DSB 
> failure ?

No, here plan is to replace I915_WRITE with dsb-write and for that 
I915_WRITE is needed for platforms which does not support.

>> +        return;
>> +    }
>> +
>> +    if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
>> +        DRM_DEBUG_KMS("DSB buffer overflow.\n");
>> +        return;
>> +    }
>> +    reg_val = buf[dsb->ins_start_offset + 1] & DSB_REG_VALUE_MASK;
>
> Why do we have this +1 here ? on the very first run (when 
> ins_start_offset is 0), this will fetch reg_val = buf[1]; Is this what 
> we want to do ?

Yes reg offset is stored in buf[x+1] position and buf[x] is for size, 
where x is starting index of indexed-write-dsb-instruction.

>
>> +    if (reg_val != i915_mmio_reg_offset(reg)) {
>> +        /* Every instruction should be 8 byte aligned. */
>> +        dsb->free_pos = ALIGN(dsb->free_pos, 2);
> The comment says that you want the position to be 8 bytes align, but 
> the factor sent to the macro is 2 ?

Yes dsb->free_pos is a index of 4 byte array. The new instruction can 
start from 0, 2, 4, 6 ... etc.

>> +
>> +        /* Update the size. */
>> +        dsb->ins_start_offset = dsb->free_pos;
> This comment is irrelevant, you are not updating the size, you are 
> caching the base memory location.

Yes .. will fix.

>> +        buf[dsb->free_pos++] = 1;
>
> What does this indicate ? What is 1 ?
>
> It would be great if you can add an aski graph/table and explain what 
> does this DSB command buffer actually contains as per your design.

Yes, as per offline discussion will add in code-comment with ascii 
graph/table.

>
>
>> +
>> +        /* 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]++;
>> +    }
>
> The code is looking unnecessarily complicated as ins_start_offset and 
> free_pos are being used for multiple reasons. I guess you can use some 
> local variables like:
>
> u32 base = count = ALIGN();
>
>  {
>
>     buf [count++] = size;
>
>    buf [count++] = command;
>
>   buf [count++] = value;
>
> }
>
> dsb->start_offset = base;
>
> dsb->free_pos = count;

Here the whole buffer not getting in one shot.

>
>> +
>> +    /* 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 = dsb->crtc;
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h 
>> b/drivers/gpu/drm/i915/display/intel_dsb.h
>> index 1b33ab118640..c848747f52d9 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
>> @@ -30,11 +30,19 @@ struct intel_dsb {
>>        * and help in calculating cmd_buf_tail.
>>        */
>>       int free_pos;
>> +
>> +    /*
>> +     * ins_start_offset will help to store start address
>> +     * of the dsb instuction of auto-increment register.
>> +     */
>> +    u32 ins_start_offset;
> How about the variable to be named as base_offset ? Also, do we need 
> to keep this saved, even when the writing is done ?

Yes it is needed to compare the register value of the next write. As 
agreed offline we can have ins_start_offset as variable-name.

>>   };
>>     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);
>
> Cross check the alignment of second line.

Yes looks fine after applying the patch.

Regards,
Animesh

>
> - Shashank
>
>>     #endif

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

Re: [PATCH v3 05/11] drm/i915/dsb: Register definition of DSB registers.

[Animesh Manna]

Hi,


On 8/28/2019 10:32 PM, Sharma, Shashank wrote:
>
> On 8/28/2019 12:40 AM, Animesh Manna wrote:
>> Added key register definitions of DSB.
>>
>> dsb-ctrl register is required to enable dsb-engine.
>>
>> head-ptr register hold the head of buffer address from where the
>> execution will start.
>>
>> Programming tail-ptr register is a trigger point to start execution.
>>
>> Cc: Uma Shankar <uma.shankar@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/i915_reg.h | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 02e1ef10c47e..71c6c2380443 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -11564,4 +11564,19 @@ 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_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_BUFFER_REITERATE        (1 << 29)
>> +#define   DSB_WAIT_FOR_VBLANK        (1 << 28)
>> +#define   DSB_WAIT_FOR_LINE_IN_RANGE    (1 << 27)
>> +#define   DSB_HALT            (1 << 16)
>> +#define   DSB_NON_POSTED_ENABLE        (1 << 8)
>> +#define   DSB_STATUS            (1 << 0)
>> +
>
> Again, this patch is just adding the header definitions, please merge 
> this patch in the patch where these definitions are being used.

Ok.

Regards,
Animesh
>
> - Shashank
>
>>   #endif /* _I915_REG_H_ */

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

Re: [PATCH v3 07/11] drm/i915/dsb: functions to enable/disable DSB engine.

[Animesh Manna]

Hi,


On 8/28/2019 10:37 PM, Sharma, Shashank wrote:
>
> On 8/28/2019 12:40 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.
>>
>> Cc: Michel Thierry <michel.thierry@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 | 40 ++++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c 
>> b/drivers/gpu/drm/i915/display/intel_dsb.c
>> index d36ee8244427..2d6e78868f2d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>> @@ -33,6 +33,46 @@ static inline bool is_dsb_busy(struct intel_dsb *dsb)
>>       return DSB_STATUS & I915_READ(DSB_CTRL(pipe, dsb->id));
>>   }
>>   +static bool intel_dsb_enable_engine(struct intel_dsb *dsb)
>> +{
>> +    struct intel_crtc *crtc = dsb->crtc;
>> +    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);
> readback DSB status and confirm if that's really enabled ?

Ok.

>> +
>> +    return true;
>> +}
>> +
>> +static bool intel_dsb_disable_engine(struct intel_dsb *dsb)
>> +{
>> +    struct intel_crtc *crtc = dsb->crtc;
>> +    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);
>> +
>
> Same as above.

Ok.

Regards,
Animesh
>
> - Shashank
>
>> +    return true;
>> +}
>> +
>>   struct intel_dsb *
>>   intel_dsb_get(struct intel_crtc *crtc)
>>   {

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

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

[Animesh Manna]

Hi,


On 8/28/2019 11:45 PM, Sharma, Shashank wrote:
>
> On 8/28/2019 12:40 AM, Animesh Manna 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.
>>
>> v1: Initial version.
>> v2: Directly call dsb-api at callsites. (Jani)
>>
>> 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_color.c | 64 ++++++++++++++--------
>>   drivers/gpu/drm/i915/i915_drv.h            |  2 +
>>   2 files changed, 43 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c 
>> b/drivers/gpu/drm/i915/display/intel_color.c
>> index 71a0201437a9..e4090d8e4547 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>> @@ -589,34 +589,38 @@ static void bdw_load_lut_10(struct intel_crtc 
>> *crtc,
>>       const struct drm_color_lut *lut = blob->data;
>>       int i, lut_size = drm_color_lut_size(blob);
>>       enum pipe pipe = crtc->pipe;
>> +    struct intel_dsb *dsb = dev_priv->dsb;
>>   -    I915_WRITE(PREC_PAL_INDEX(pipe), prec_index |
>> -           PAL_PREC_AUTO_INCREMENT);
> I dont see any gen check in any of the functions before calling dsb() 
> functions.

Explained before, as per agreed design with Jani the plan here is to 
replace I915_WRITE call with dsb-write
and for platforms which are not supporting DSB will fallback into MMIO 
write.

Regards,
Animesh

>> +    intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe),
>> +                prec_index | PAL_PREC_AUTO_INCREMENT);
>>         for (i = 0; i < hw_lut_size; i++) {
>>           /* We discard half the user entries in split gamma mode */
>>           const struct drm_color_lut *entry =
>>               &lut[i * (lut_size - 1) / (hw_lut_size - 1)];
>>   -        I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(entry));
>> +        intel_dsb_indexed_reg_write(dsb, PREC_PAL_DATA(pipe),
>> +                        ilk_lut_10(entry));
>>       }
>>         /*
>>        * Reset the index, otherwise it prevents the legacy palette to be
>>        * written properly.
>>        */
>> -    I915_WRITE(PREC_PAL_INDEX(pipe), 0);
>> +    intel_dsb_reg_write(dsb, PREC_PAL_INDEX(pipe), 0);
>>   }
>>     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 = dev_priv->dsb;
>>       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 +628,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);
>>       }
>>   }
>>   @@ -788,12 +795,13 @@ icl_load_gcmax(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);
>> +    struct intel_dsb *dsb = dev_priv->dsb;
>>       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
>> @@ -803,6 +811,7 @@ icl_program_gamma_superfine_segment(const struct 
>> intel_crtc_state *crtc_state)
>>       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 = dev_priv->dsb;
>>       enum pipe pipe = crtc->pipe;
>>       u32 i;
>>   @@ -813,15 +822,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));
>>       }
>>   }
>>   @@ -833,6 +843,7 @@ icl_program_gamma_multi_segment(const struct 
>> intel_crtc_state *crtc_state)
>>       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 = dev_priv->dsb;
>>       enum pipe pipe = crtc->pipe;
>>       u32 i;
>>   @@ -847,11 +858,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 +881,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 */
>> @@ -980,7 +995,10 @@ void intel_color_load_luts(const struct 
>> intel_crtc_state *crtc_state)
>>   {
>>       struct drm_i915_private *dev_priv = 
>> to_i915(crtc_state->base.crtc->dev);
>>   +    dev_priv->dsb = 
>> intel_dsb_get(to_intel_crtc(crtc_state->base.crtc));
>>       dev_priv->display.load_luts(crtc_state);
>> +    intel_dsb_commit(dev_priv->dsb);
>> +    intel_dsb_put(dev_priv->dsb);
>>   }
>>     void intel_color_commit(const struct intel_crtc_state *crtc_state)
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index dd6cfb89b830..ace81ccbd766 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1759,6 +1759,8 @@ struct drm_i915_private {
>>       /* Mutex to protect the above hdcp component related values. */
>>       struct mutex hdcp_comp_mutex;
>>   +    struct intel_dsb *dsb;
>> +
>>       /*
>>        * NOTE: This is the dri1/ums dungeon, don't add stuff here. 
>> Your patch
>>        * will be rejected. Instead look for a better place.

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