Re: [Intel-gfx] [PATCH v3 02/11] drm/i915/dsb: DSB context creation.
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
Cc: Michel Thierry
Cc: Jani Nikula
Cc: Rodrigo Vivi
Signed-off-by: Animesh Manna
---
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 ..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 = >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(>runtime_pm);
+
+obj = i915_gem_object_create_internal(i915, DSB_BUF_SIZE);
+if (IS_ERR(obj))
+goto err;
+
+mutex_lock(>drm.struct_mutex);
+vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, PIN_MAPPABLE);
+mutex_unlock(>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)) {
+
Re: [Intel-gfx] [PATCH v3 02/11] drm/i915/dsb: DSB context creation.
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
Cc: Michel Thierry
Cc: Jani Nikula
Cc: Rodrigo Vivi
Signed-off-by: Animesh Manna
---
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 ..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 = >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(>runtime_pm);
+
+ obj = i915_gem_object_create_internal(i915, DSB_BUF_SIZE);
+ if (IS_ERR(obj))
+ goto err;
+
+ mutex_lock(>drm.struct_mutex);
+ vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, PIN_MAPPABLE);
+ mutex_unlock(>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(>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
