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.d...@intel.com>
Cc: Michel Thierry <michel.thie...@intel.com>
Cc: Jani Nikula <jani.nik...@intel.com>
Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
Signed-off-by: Animesh Manna <animesh.ma...@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
