[Intel-gfx] [PATCH 1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check

[Intel-gfx] [PATCH 1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We may want to not use the DSB even if the platform has one.
So replace the HAS_DSB check in the _put() with a cmd_buf check
that will work in either case.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 9dd18144a664..12776f09f227 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -160,9 +160,8 @@ intel_dsb_get(struct intel_crtc *crtc)
 void intel_dsb_put(struct intel_dsb *dsb)
 {
 	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
-	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
 
-	if (!HAS_DSB(i915))
+	if (!dsb->cmd_buf)
 		return;
 
 	if (WARN_ON(dsb->refcount == 0))
-- 
2.24.1

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

[Intel-gfx] [PATCH 2/9] drm/i915/dsb: Disable DSB until fixed

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Our DSB support is borked. The DSB seems to get stuck and we just
get:
[drm:intel_dsb_commit [i915]] DSB execution started - head 0xa8c000, tail 0x10c0
[drm:intel_dsb_commit [i915]] *ERROR* Timed out waiting for DSB workload completion.
[drm:icl_load_luts [i915]] DSB engine is busy.

Which even seems to result in underruns on some internal stuff.
Let's just disable it all until someone figures out what's wrong
with it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 12776f09f227..a16a78cd0ebe 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -105,7 +105,11 @@ intel_dsb_get(struct intel_crtc *crtc)
 	u32 *buf;
 	intel_wakeref_t wakeref;
 
-	if (!HAS_DSB(i915))
+	/*
+	 * FIXME DSB support is busted. DSB tends to be
+	 * busy when it shouldn't be. Disable until fixed.
+	 */
+	if (true || !HAS_DSB(i915))
 		return dsb;
 
 	if (dsb->refcount++ != 0)
-- 
2.24.1

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

[Intel-gfx] [PATCH 3/9] drm/i915/dsb: Turn the "DSB is busy" into an error

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The DSB should never be busy here or we messed up somewhere else.
Make this a DRM_ERROR().

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index a16a78cd0ebe..5bb4ac1610a5 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -52,7 +52,7 @@ static inline bool intel_dsb_enable_engine(struct intel_dsb *dsb)
 
 	dsb_ctrl = intel_de_read(dev_priv, DSB_CTRL(pipe, dsb->id));
 	if (DSB_STATUS & dsb_ctrl) {
-		DRM_DEBUG_KMS("DSB engine is busy.\n");
+		DRM_ERROR("DSB engine is busy.\n");
 		return false;
 	}
 
-- 
2.24.1

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

[Intel-gfx] [PATCH 4/9] drm/i915/dsb: Stop with the RMW

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We don't want to keep random bits set in DSB_CTRL. Stop the
harmful RMW.

Also flip the reverse & around to appease my ocd.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 5bb4ac1610a5..8b357b35c41f 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -40,7 +40,7 @@ static inline bool is_dsb_busy(struct intel_dsb *dsb)
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum pipe pipe = crtc->pipe;
 
-	return DSB_STATUS & intel_de_read(dev_priv, DSB_CTRL(pipe, dsb->id));
+	return intel_de_read(dev_priv, DSB_CTRL(pipe, dsb->id)) & DSB_STATUS;
 }
 
 static inline bool intel_dsb_enable_engine(struct intel_dsb *dsb)
@@ -48,18 +48,15 @@ static inline bool intel_dsb_enable_engine(struct intel_dsb *dsb)
 	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum pipe pipe = crtc->pipe;
-	u32 dsb_ctrl;
 
-	dsb_ctrl = intel_de_read(dev_priv, DSB_CTRL(pipe, dsb->id));
-	if (DSB_STATUS & dsb_ctrl) {
+	if (is_dsb_busy(dsb)) {
 		DRM_ERROR("DSB engine is busy.\n");
 		return false;
 	}
 
-	dsb_ctrl |= DSB_ENABLE;
-	intel_de_write(dev_priv, DSB_CTRL(pipe, dsb->id), dsb_ctrl);
-
+	intel_de_write(dev_priv, DSB_CTRL(pipe, dsb->id), DSB_ENABLE);
 	intel_de_posting_read(dev_priv, DSB_CTRL(pipe, dsb->id));
+
 	return true;
 }
 
@@ -68,18 +65,15 @@ static inline bool intel_dsb_disable_engine(struct intel_dsb *dsb)
 	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum pipe pipe = crtc->pipe;
-	u32 dsb_ctrl;
 
-	dsb_ctrl = intel_de_read(dev_priv, DSB_CTRL(pipe, dsb->id));
-	if (DSB_STATUS & dsb_ctrl) {
+	if (is_dsb_busy(dsb)) {
 		DRM_DEBUG_KMS("DSB engine is busy.\n");
 		return false;
 	}
 
-	dsb_ctrl &= ~DSB_ENABLE;
-	intel_de_write(dev_priv, DSB_CTRL(pipe, dsb->id), dsb_ctrl);
-
+	intel_de_write(dev_priv, DSB_CTRL(pipe, dsb->id), 0);
 	intel_de_posting_read(dev_priv, DSB_CTRL(pipe, dsb->id));
+
 	return true;
 }
 
-- 
2.24.1

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

[Intel-gfx] [PATCH 5/9] drm/i915/dsb: Unwind on map error

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Clean up properly if we can't map the DSB buffer.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 8b357b35c41f..a46a8f499e0e 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -127,6 +127,9 @@ intel_dsb_get(struct intel_crtc *crtc)
 	buf = i915_gem_object_pin_map(vma->obj, I915_MAP_WC);
 	if (IS_ERR(buf)) {
 		DRM_ERROR("Command buffer creation failed\n");
+		i915_vma_unpin(vma);
+		i915_vma_close(vma);
+		i915_gem_object_put(obj);
 		goto out;
 	}
 
-- 
2.24.1

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

[Intel-gfx] [PATCH 6/9] drm/i915/dsb: Inline DSB_CTRL writes into intel_dsb_commit()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

No point in having these wrappers for a simple DSB_CTRL write.
Inline them into intel_dsb_commit().

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 64 +++++-------------------
 1 file changed, 13 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index a46a8f499e0e..53759bf7a451 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -43,40 +43,6 @@ static inline bool is_dsb_busy(struct intel_dsb *dsb)
 	return intel_de_read(dev_priv, DSB_CTRL(pipe, dsb->id)) & DSB_STATUS;
 }
 
-static inline bool intel_dsb_enable_engine(struct intel_dsb *dsb)
-{
-	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	enum pipe pipe = crtc->pipe;
-
-	if (is_dsb_busy(dsb)) {
-		DRM_ERROR("DSB engine is busy.\n");
-		return false;
-	}
-
-	intel_de_write(dev_priv, DSB_CTRL(pipe, dsb->id), DSB_ENABLE);
-	intel_de_posting_read(dev_priv, DSB_CTRL(pipe, dsb->id));
-
-	return true;
-}
-
-static inline bool intel_dsb_disable_engine(struct intel_dsb *dsb)
-{
-	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	enum pipe pipe = crtc->pipe;
-
-	if (is_dsb_busy(dsb)) {
-		DRM_DEBUG_KMS("DSB engine is busy.\n");
-		return false;
-	}
-
-	intel_de_write(dev_priv, DSB_CTRL(pipe, dsb->id), 0);
-	intel_de_posting_read(dev_priv, DSB_CTRL(pipe, dsb->id));
-
-	return true;
-}
-
 /**
  * intel_dsb_get() - Allocate DSB context and return a DSB instance.
  * @crtc: intel_crtc structure to get pipe info.
@@ -306,36 +272,32 @@ void intel_dsb_commit(struct intel_dsb *dsb)
 	if (!dsb->free_pos)
 		return;
 
-	if (!intel_dsb_enable_engine(dsb))
-		goto reset;
-
-	if (is_dsb_busy(dsb)) {
-		DRM_ERROR("HEAD_PTR write failed - dsb engine is busy.\n");
-		goto reset;
-	}
-	intel_de_write(dev_priv, DSB_HEAD(pipe, dsb->id),
-		       i915_ggtt_offset(dsb->vma));
-
 	tail = ALIGN(dsb->free_pos * 4, CACHELINE_BYTES);
 	if (tail > dsb->free_pos * 4)
 		memset(&dsb->cmd_buf[dsb->free_pos], 0,
 		       (tail - dsb->free_pos * 4));
 
 	if (is_dsb_busy(dsb)) {
-		DRM_ERROR("TAIL_PTR write failed - dsb engine is busy.\n");
+		DRM_ERROR("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);
+
+	intel_de_write(dev_priv, DSB_CTRL(pipe, dsb->id),
+		       DSB_ENABLE);
+	intel_de_write(dev_priv, DSB_HEAD(pipe, dsb->id),
+		       i915_ggtt_offset(dsb->vma));
 	intel_de_write(dev_priv, DSB_TAIL(pipe, dsb->id),
 		       i915_ggtt_offset(dsb->vma) + tail);
-	if (wait_for(!is_dsb_busy(dsb), 1)) {
+
+	DRM_DEBUG_KMS("DSB execution started - head 0x%x, tail 0x%x\n",
+		      i915_ggtt_offset(dsb->vma),
+		      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;
 	dsb->ins_start_offset = 0;
-	intel_dsb_disable_engine(dsb);
+	intel_de_write(dev_priv, DSB_CTRL(pipe, dsb->id), 0);
 }
-- 
2.24.1

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

[Intel-gfx] [PATCH 7/9] drm/i915/dsb: Wait for DSB to idle after disabling it

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

In case DSB was busy before we started it we try to disable it.
Let's try to wait for it to become idle.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 53759bf7a451..45acfd91ee74 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -300,4 +300,7 @@ void intel_dsb_commit(struct intel_dsb *dsb)
 	dsb->free_pos = 0;
 	dsb->ins_start_offset = 0;
 	intel_de_write(dev_priv, DSB_CTRL(pipe, dsb->id), 0);
+
+	if (wait_for(!is_dsb_busy(dsb), 1))
+		DRM_ERROR("Timed out waiting for DSB idle.\n");
 }
-- 
2.24.1

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

[Intel-gfx] [PATCH 8/9] drm/i915/dsb: Introduce intel_dsb_align_tail()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Move the DSB tail cacheline alignment to a helper. No need to pollute
the caller with mundane details like this.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 45acfd91ee74..56790fae6386 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -254,6 +254,18 @@ void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
 			       i915_mmio_reg_offset(reg);
 }
 
+static u32 intel_dsb_align_tail(struct intel_dsb *dsb)
+{
+	u32 tail;
+
+	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));
+
+	return tail;
+}
+
 /**
  * intel_dsb_commit() - Trigger workload execution of DSB.
  * @dsb: intel_dsb structure.
@@ -269,14 +281,10 @@ void intel_dsb_commit(struct intel_dsb *dsb)
 	enum pipe pipe = crtc->pipe;
 	u32 tail;
 
-	if (!dsb->free_pos)
+	tail = intel_dsb_align_tail(dsb);
+	if (tail == 0)
 		return;
 
-	tail = ALIGN(dsb->free_pos * 4, CACHELINE_BYTES);
-	if (tail > dsb->free_pos * 4)
-		memset(&dsb->cmd_buf[dsb->free_pos], 0,
-		       (tail - dsb->free_pos * 4));
-
 	if (is_dsb_busy(dsb)) {
 		DRM_ERROR("DSB engine is busy.\n");
 		goto reset;
-- 
2.24.1

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

[Intel-gfx] [PATCH 9/9] drm/i915/dsb: Nuke the 'dev' variables

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

No point in having these drm_device variables around. Get rid of them.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 56790fae6386..c97b64401c23 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -57,8 +57,7 @@ static inline bool is_dsb_busy(struct intel_dsb *dsb)
 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_private *i915 = to_i915(crtc->base.dev);
 	struct intel_dsb *dsb = &crtc->dsb;
 	struct drm_i915_gem_object *obj;
 	struct i915_vma *vma;
@@ -276,8 +275,7 @@ static u32 intel_dsb_align_tail(struct intel_dsb *dsb)
 void intel_dsb_commit(struct intel_dsb *dsb)
 {
 	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
-	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum pipe pipe = crtc->pipe;
 	u32 tail;
 
-- 
2.24.1

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

Re: [Intel-gfx] [PATCH 6/9] drm/i915/dsb: Inline DSB_CTRL writes into intel_dsb_commit()

[Chris Wilson]

Quoting Ville Syrjala (2020-01-29 18:20:31)
> +       intel_de_write(dev_priv, DSB_CTRL(pipe, dsb->id),
> +                      DSB_ENABLE);
> +       intel_de_write(dev_priv, DSB_HEAD(pipe, dsb->id),
> +                      i915_ggtt_offset(dsb->vma));
>         intel_de_write(dev_priv, DSB_TAIL(pipe, dsb->id),
>                        i915_ggtt_offset(dsb->vma) + tail);

I still say this order looks dodgy for a ringbuffer. Is it all truly
latched by DSB_TAIL or might it start passing on being enabled and HEAD
!= TAIL?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/9] drm/i915/dsb: Inline DSB_CTRL writes into intel_dsb_commit()

[Ville Syrjälä]

On Wed, Jan 29, 2020 at 06:32:04PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2020-01-29 18:20:31)
> > +       intel_de_write(dev_priv, DSB_CTRL(pipe, dsb->id),
> > +                      DSB_ENABLE);
> > +       intel_de_write(dev_priv, DSB_HEAD(pipe, dsb->id),
> > +                      i915_ggtt_offset(dsb->vma));
> >         intel_de_write(dev_priv, DSB_TAIL(pipe, dsb->id),
> >                        i915_ggtt_offset(dsb->vma) + tail);
> 
> I still say this order looks dodgy for a ringbuffer. Is it all truly
> latched by DSB_TAIL or might it start passing on being enabled and HEAD
> != TAIL?

My initial thoughts as well, but indeed it's the tail write that
makes it go (or at least that's what the docs say).

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

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check

[Patchwork]

== Series Details ==

Series: series starting with [1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check
URL   : https://patchwork.freedesktop.org/series/72737/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
9d0a47bda474 drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check
dcc47ed9c2ba drm/i915/dsb: Disable DSB until fixed
-:11: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#11: 
[drm:intel_dsb_commit [i915]] DSB execution started - head 0xa8c000, tail 0x10c0

total: 0 errors, 1 warnings, 0 checks, 12 lines checked
1ae817cd8ab9 drm/i915/dsb: Turn the "DSB is busy" into an error
47c190820a9b drm/i915/dsb: Stop with the RMW
b061a1bc939f drm/i915/dsb: Unwind on map error
89f3182a2632 drm/i915/dsb: Inline DSB_CTRL writes into intel_dsb_commit()
25644f18b6f6 drm/i915/dsb: Wait for DSB to idle after disabling it
30f241242694 drm/i915/dsb: Introduce intel_dsb_align_tail()
da5fb99d67d0 drm/i915/dsb: Nuke the 'dev' variables

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

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check

[Patchwork]

== Series Details ==

Series: series starting with [1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check
URL   : https://patchwork.freedesktop.org/series/72737/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7838 -> Patchwork_16319
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16319/index.html

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@kms_chamelium@dp-edid-read:
    - fi-icl-u2:          [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7838/fi-icl-u2/igt@kms_chamelium@dp-edid-read.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16319/fi-icl-u2/igt@kms_chamelium@dp-edid-read.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_active:
    - fi-whl-u:           [PASS][3] -> [DMESG-FAIL][4] ([i915#666])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7838/fi-whl-u/igt@i915_selftest@live_active.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16319/fi-whl-u/igt@i915_selftest@live_active.html

  
#### Possible fixes ####

  * igt@i915_selftest@live_blt:
    - fi-hsw-4770r:       [DMESG-FAIL][5] ([i915#725]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7838/fi-hsw-4770r/igt@i915_selftest@live_blt.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16319/fi-hsw-4770r/igt@i915_selftest@live_blt.html

  * igt@i915_selftest@live_gem_contexts:
    - fi-cfl-8700k:       [INCOMPLETE][7] ([i915#424]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7838/fi-cfl-8700k/igt@i915_selftest@live_gem_contexts.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16319/fi-cfl-8700k/igt@i915_selftest@live_gem_contexts.html

  * igt@i915_selftest@live_gtt:
    - fi-bdw-5557u:       [TIMEOUT][9] ([fdo#112271]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7838/fi-bdw-5557u/igt@i915_selftest@live_gtt.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16319/fi-bdw-5557u/igt@i915_selftest@live_gtt.html

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
    - fi-icl-dsi:         [DMESG-WARN][11] ([i915#109]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7838/fi-icl-dsi/igt@kms_pipe_crc_basic@hang-read-crc-pipe-a.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16319/fi-icl-dsi/igt@kms_pipe_crc_basic@hang-read-crc-pipe-a.html

  
#### Warnings ####

  * igt@i915_selftest@live_execlists:
    - fi-icl-y:           [INCOMPLETE][13] ([i915#140]) -> [DMESG-FAIL][14] ([fdo#108569])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7838/fi-icl-y/igt@i915_selftest@live_execlists.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16319/fi-icl-y/igt@i915_selftest@live_execlists.html

  
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#112271]: https://bugs.freedesktop.org/show_bug.cgi?id=112271
  [i915#109]: https://gitlab.freedesktop.org/drm/intel/issues/109
  [i915#140]: https://gitlab.freedesktop.org/drm/intel/issues/140
  [i915#424]: https://gitlab.freedesktop.org/drm/intel/issues/424
  [i915#666]: https://gitlab.freedesktop.org/drm/intel/issues/666
  [i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725


Participating hosts (50 -> 46)
------------------------------

  Additional (2): fi-byt-n2820 fi-bwr-2160 
  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-kbl-7560u fi-tgl-y fi-byt-clapper fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7838 -> Patchwork_16319

  CI-20190529: 20190529
  CI_DRM_7838: d3d96beea538c8de906a1c4d7e6793a47d17a471 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5404: 4147bab8e3dcaf11bd657b5fb4c109708e94e60c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16319: da5fb99d67d021173d498e119d0f2c8ca5c8c6e3 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

da5fb99d67d0 drm/i915/dsb: Nuke the 'dev' variables
30f241242694 drm/i915/dsb: Introduce intel_dsb_align_tail()
25644f18b6f6 drm/i915/dsb: Wait for DSB to idle after disabling it
89f3182a2632 drm/i915/dsb: Inline DSB_CTRL writes into intel_dsb_commit()
b061a1bc939f drm/i915/dsb: Unwind on map error
47c190820a9b drm/i915/dsb: Stop with the RMW
1ae817cd8ab9 drm/i915/dsb: Turn the "DSB is busy" into an error
dcc47ed9c2ba drm/i915/dsb: Disable DSB until fixed
9d0a47bda474 drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check

== Logs ==

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

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check (rev2)
URL   : https://patchwork.freedesktop.org/series/72737/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
770bb0a72167 drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check
2bb3151e5050 drm/i915/dsb: Disable DSB until fixed
-:11: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#11: 
[drm:intel_dsb_commit [i915]] DSB execution started - head 0xa8c000, tail 0x10c0

total: 0 errors, 1 warnings, 0 checks, 12 lines checked
cbb3cfcc3bae drm/i915/dsb: Turn the "DSB is busy" into an error
46a21d424063 drm/i915/dsb: Stop with the RMW
84a01d2ee8dc drm/i915/dsb: Unwind on map error
53fed04914b3 drm/i915/dsb: Inline DSB_CTRL writes into intel_dsb_commit()
28b429e1447d drm/i915/dsb: Wait for DSB to idle after disabling it
46032e921202 drm/i915/dsb: Introduce intel_dsb_align_tail()
1406264377b6 drm/i915/dsb: Nuke the 'dev' variables

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

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check (rev2)
URL   : https://patchwork.freedesktop.org/series/72737/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7844 -> Patchwork_16336
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16336/index.html

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live_hangcheck:
    - fi-icl-u3:          [PASS][1] -> [TIMEOUT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7844/fi-icl-u3/igt@i915_selftest@live_hangcheck.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16336/fi-icl-u3/igt@i915_selftest@live_hangcheck.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_blt:
    - fi-hsw-4770:        [PASS][3] -> [DMESG-FAIL][4] ([i915#553] / [i915#725])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7844/fi-hsw-4770/igt@i915_selftest@live_blt.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16336/fi-hsw-4770/igt@i915_selftest@live_blt.html

  * igt@i915_selftest@live_execlists:
    - fi-kbl-8809g:       [PASS][5] -> [INCOMPLETE][6] ([fdo#112175] / [fdo#112259] / [i915#435])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7844/fi-kbl-8809g/igt@i915_selftest@live_execlists.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16336/fi-kbl-8809g/igt@i915_selftest@live_execlists.html

  
#### Possible fixes ####

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-6770hq:      [FAIL][7] ([i915#178]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7844/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16336/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][9] ([fdo#111096] / [i915#323]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7844/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16336/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-hsw-peppy:       [DMESG-WARN][11] ([i915#44]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7844/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16336/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html

  
  [fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
  [fdo#112175]: https://bugs.freedesktop.org/show_bug.cgi?id=112175
  [fdo#112259]: https://bugs.freedesktop.org/show_bug.cgi?id=112259
  [i915#178]: https://gitlab.freedesktop.org/drm/intel/issues/178
  [i915#323]: https://gitlab.freedesktop.org/drm/intel/issues/323
  [i915#435]: https://gitlab.freedesktop.org/drm/intel/issues/435
  [i915#44]: https://gitlab.freedesktop.org/drm/intel/issues/44
  [i915#553]: https://gitlab.freedesktop.org/drm/intel/issues/553
  [i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725


Participating hosts (46 -> 42)
------------------------------

  Additional (7): fi-glk-dsi fi-ilk-650 fi-whl-u fi-ivb-3770 fi-cfl-8109u fi-blb-e6850 fi-kbl-r 
  Missing    (11): fi-icl-1065g7 fi-cml-u2 fi-ilk-m540 fi-hsw-4200u fi-bdw-gvtdvm fi-byt-squawks fi-bsw-cyan fi-bwr-2160 fi-gdg-551 fi-elk-e7500 fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7844 -> Patchwork_16336

  CI-20190529: 20190529
  CI_DRM_7844: 47faa2a989ef89a15089190a5f942a2d2a34fda5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5407: a9d69f51dadbcbc53527671f87572d05c3370cba @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16336: 1406264377b6e5e02a9949673ef4c6b2248f6bd8 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

1406264377b6 drm/i915/dsb: Nuke the 'dev' variables
46032e921202 drm/i915/dsb: Introduce intel_dsb_align_tail()
28b429e1447d drm/i915/dsb: Wait for DSB to idle after disabling it
53fed04914b3 drm/i915/dsb: Inline DSB_CTRL writes into intel_dsb_commit()
84a01d2ee8dc drm/i915/dsb: Unwind on map error
46a21d424063 drm/i915/dsb: Stop with the RMW
cbb3cfcc3bae drm/i915/dsb: Turn the "DSB is busy" into an error
2bb3151e5050 drm/i915/dsb: Disable DSB until fixed
770bb0a72167 drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check

== Logs ==

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

Re: [Intel-gfx] [PATCH 1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check

[Souza, Jose]

On Wed, 2020-01-29 at 20:20 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We may want to not use the DSB even if the platform has one.
> So replace the HAS_DSB check in the _put() with a cmd_buf check
> that will work in either case.

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsb.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 9dd18144a664..12776f09f227 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -160,9 +160,8 @@ intel_dsb_get(struct intel_crtc *crtc)
>  void intel_dsb_put(struct intel_dsb *dsb)
>  {
>  	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc),
> dsb);
> -	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>  
> -	if (!HAS_DSB(i915))
> +	if (!dsb->cmd_buf)
>  		return;
>  
>  	if (WARN_ON(dsb->refcount == 0))
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/9] drm/i915/dsb: Disable DSB until fixed

[Souza, Jose]

On Wed, 2020-01-29 at 20:20 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Our DSB support is borked. The DSB seems to get stuck and we just
> get:
> [drm:intel_dsb_commit [i915]] DSB execution started - head 0xa8c000,
> tail 0x10c0
> [drm:intel_dsb_commit [i915]] *ERROR* Timed out waiting for DSB
> workload completion.
> [drm:icl_load_luts [i915]] DSB engine is busy.
> 
> Which even seems to result in underruns on some internal stuff.
> Let's just disable it all until someone figures out what's wrong
> with it.

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsb.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 12776f09f227..a16a78cd0ebe 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -105,7 +105,11 @@ intel_dsb_get(struct intel_crtc *crtc)
>  	u32 *buf;
>  	intel_wakeref_t wakeref;
>  
> -	if (!HAS_DSB(i915))
> +	/*
> +	 * FIXME DSB support is busted. DSB tends to be
> +	 * busy when it shouldn't be. Disable until fixed.
> +	 */
> +	if (true || !HAS_DSB(i915))
>  		return dsb;
>  
>  	if (dsb->refcount++ != 0)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check

[Manna, Animesh]

On 30-01-2020 23:43, Souza, Jose wrote:
> On Wed, 2020-01-29 at 20:20 +0200, Ville Syrjala wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> We may want to not use the DSB even if the platform has one.
>> So replace the HAS_DSB check in the _put() with a cmd_buf check
>> that will work in either case.
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dsb.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
>> b/drivers/gpu/drm/i915/display/intel_dsb.c
>> index 9dd18144a664..12776f09f227 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>> @@ -160,9 +160,8 @@ intel_dsb_get(struct intel_crtc *crtc)
>>   void intel_dsb_put(struct intel_dsb *dsb)
>>   {
>>   	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc),
>> dsb);
>> -	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>>   
>> -	if (!HAS_DSB(i915))
>> +	if (!dsb->cmd_buf)

Ville and Jose,

Have a concern here. In intel_dsb_get() if get failure during i915_gem_object_create_internal, i915_gem_object_ggtt_pin, i915_gem_object_pin_map then we may not have dsb->cmd_buf.
Then ref-count mechanism will break.
I feel HAS_DSB(i915) check is better than dsb->cmd_buf otherwise need to do some cleanup is intel_dsb_get() as well.

Regards,
Animesh

>>   		return;
>>   
>>   	if (WARN_ON(dsb->refcount == 0))
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check

[Ville Syrjälä]

On Fri, Jan 31, 2020 at 03:04:17PM +0530, Manna, Animesh wrote:
> 
> On 30-01-2020 23:43, Souza, Jose wrote:
> > On Wed, 2020-01-29 at 20:20 +0200, Ville Syrjala wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> We may want to not use the DSB even if the platform has one.
> >> So replace the HAS_DSB check in the _put() with a cmd_buf check
> >> that will work in either case.
> > Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> >
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/display/intel_dsb.c | 3 +--
> >>   1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> >> b/drivers/gpu/drm/i915/display/intel_dsb.c
> >> index 9dd18144a664..12776f09f227 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> >> @@ -160,9 +160,8 @@ intel_dsb_get(struct intel_crtc *crtc)
> >>   void intel_dsb_put(struct intel_dsb *dsb)
> >>   {
> >>   	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc),
> >> dsb);
> >> -	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> >>   
> >> -	if (!HAS_DSB(i915))
> >> +	if (!dsb->cmd_buf)
> 
> Ville and Jose,
> 
> Have a concern here. In intel_dsb_get() if get failure during i915_gem_object_create_internal, i915_gem_object_ggtt_pin, i915_gem_object_pin_map then we may not have dsb->cmd_buf.
> Then ref-count mechanism will break.

Hmm. Yeah. The refcount WARN could easily be fixed by either
decrementung refcount on get() fail or doing the "let's never use
DSB" patch after the refcount inc.

> I feel HAS_DSB(i915) check is better than dsb->cmd_buf otherwise need to do some cleanup is intel_dsb_get() as well.
> 
> Regards,
> Animesh
> 
> >>   		return;
> >>   
> >>   	if (WARN_ON(dsb->refcount == 0))
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [PATCH 1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check

[Manna, Animesh]

On 31-01-2020 17:12, Ville Syrjälä wrote:
> On Fri, Jan 31, 2020 at 03:04:17PM +0530, Manna, Animesh wrote:
>> On 30-01-2020 23:43, Souza, Jose wrote:
>>> On Wed, 2020-01-29 at 20:20 +0200, Ville Syrjala wrote:
>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>
>>>> We may want to not use the DSB even if the platform has one.
>>>> So replace the HAS_DSB check in the _put() with a cmd_buf check
>>>> that will work in either case.
>>> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
>>>
>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/display/intel_dsb.c | 3 +--
>>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> b/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> index 9dd18144a664..12776f09f227 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
>>>> @@ -160,9 +160,8 @@ intel_dsb_get(struct intel_crtc *crtc)
>>>>    void intel_dsb_put(struct intel_dsb *dsb)
>>>>    {
>>>>    	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc),
>>>> dsb);
>>>> -	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>>>>    
>>>> -	if (!HAS_DSB(i915))
>>>> +	if (!dsb->cmd_buf)
>> Ville and Jose,
>>
>> Have a concern here. In intel_dsb_get() if get failure during i915_gem_object_create_internal, i915_gem_object_ggtt_pin, i915_gem_object_pin_map then we may not have dsb->cmd_buf.
>> Then ref-count mechanism will break.
> Hmm. Yeah. The refcount WARN could easily be fixed by either
> decrementung refcount on get() fail or doing the "let's never use
> DSB" patch after the refcount inc.

Hmm, from design point get/put/ref-count mechanism introduced to check dsp-api are used properly or not.
For erroneous case managing ref-count in get() itself void the purpose of put() call.
For example,

intel_dsb_get()
got error from i915_gem_object_create_internal, i915_gem_object_ggtt_pin, i915_gem_object_pin_map
intel_dsb_put
intel_dsb_put
...

Should throw warning but can not if we manage in get() itself.

Regards,
Animesh

>
>> I feel HAS_DSB(i915) check is better than dsb->cmd_buf otherwise need to do some cleanup is intel_dsb_get() as well.
>>
>> Regards,
>> Animesh
>>
>>>>    		return;
>>>>    
>>>>    	if (WARN_ON(dsb->refcount == 0))
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check

[Ville Syrjälä]

On Fri, Jan 31, 2020 at 05:36:15PM +0530, Manna, Animesh wrote:
> 
> On 31-01-2020 17:12, Ville Syrjälä wrote:
> > On Fri, Jan 31, 2020 at 03:04:17PM +0530, Manna, Animesh wrote:
> >> On 30-01-2020 23:43, Souza, Jose wrote:
> >>> On Wed, 2020-01-29 at 20:20 +0200, Ville Syrjala wrote:
> >>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>
> >>>> We may want to not use the DSB even if the platform has one.
> >>>> So replace the HAS_DSB check in the _put() with a cmd_buf check
> >>>> that will work in either case.
> >>> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> >>>
> >>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>> ---
> >>>>    drivers/gpu/drm/i915/display/intel_dsb.c | 3 +--
> >>>>    1 file changed, 1 insertion(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> >>>> b/drivers/gpu/drm/i915/display/intel_dsb.c
> >>>> index 9dd18144a664..12776f09f227 100644
> >>>> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> >>>> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> >>>> @@ -160,9 +160,8 @@ intel_dsb_get(struct intel_crtc *crtc)
> >>>>    void intel_dsb_put(struct intel_dsb *dsb)
> >>>>    {
> >>>>    	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc),
> >>>> dsb);
> >>>> -	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> >>>>    
> >>>> -	if (!HAS_DSB(i915))
> >>>> +	if (!dsb->cmd_buf)
> >> Ville and Jose,
> >>
> >> Have a concern here. In intel_dsb_get() if get failure during i915_gem_object_create_internal, i915_gem_object_ggtt_pin, i915_gem_object_pin_map then we may not have dsb->cmd_buf.
> >> Then ref-count mechanism will break.
> > Hmm. Yeah. The refcount WARN could easily be fixed by either
> > decrementung refcount on get() fail or doing the "let's never use
> > DSB" patch after the refcount inc.
> 
> Hmm, from design point get/put/ref-count mechanism introduced to check dsp-api are used properly or not.
> For erroneous case managing ref-count in get() itself void the purpose of put() call.
> For example,
> 
> intel_dsb_get()
> got error from i915_gem_object_create_internal, i915_gem_object_ggtt_pin, i915_gem_object_pin_map
> intel_dsb_put
> intel_dsb_put
> ...
> 
> Should throw warning but can not if we manage in get() itself.

None of this stuff should really exist in the guts of the dsb code
anyway. It's all just a hack to get the dsb code in without actually
taking advantage of the dsb. The real solution would involve doing the
dsb vs. mmio decision upfront at the start of the atomic commit, and
then using totally different codeepaths for those two cases. No real
need for refcounts in that case. But first we'd need to finish the
vblank workers so we'd have the mmio path sorted out.

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

Re: [Intel-gfx] [PATCH 9/9] drm/i915/dsb: Nuke the 'dev' variables

[Sharma, Swati2]

On 29-Jan-20 11:50 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> No point in having these drm_device variables around. Get rid of them.

Reviewed-by: Swati Sharma <swati2.sharma@intel.com>

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dsb.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 56790fae6386..c97b64401c23 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -57,8 +57,7 @@ static inline bool is_dsb_busy(struct intel_dsb *dsb)
>   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_private *i915 = to_i915(crtc->base.dev);
>   	struct intel_dsb *dsb = &crtc->dsb;
>   	struct drm_i915_gem_object *obj;
>   	struct i915_vma *vma;
> @@ -276,8 +275,7 @@ static u32 intel_dsb_align_tail(struct intel_dsb *dsb)
>   void intel_dsb_commit(struct intel_dsb *dsb)
>   {
>   	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> -	struct drm_device *dev = crtc->base.dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>   	enum pipe pipe = crtc->pipe;
>   	u32 tail;
>   
> 

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

Re: [Intel-gfx] [PATCH 3/9] drm/i915/dsb: Turn the "DSB is busy" into an error

[Sharma, Swati2]

On 29-Jan-20 11:50 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The DSB should never be busy here or we messed up somewhere else.
> Make this a DRM_ERROR().

This is required, at-least to catch the issue in CI.

Reviewed-by: Swati Sharma <swati2.sharma@intel.com>

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dsb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index a16a78cd0ebe..5bb4ac1610a5 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -52,7 +52,7 @@ static inline bool intel_dsb_enable_engine(struct intel_dsb *dsb)
>   
>   	dsb_ctrl = intel_de_read(dev_priv, DSB_CTRL(pipe, dsb->id));
>   	if (DSB_STATUS & dsb_ctrl) {
> -		DRM_DEBUG_KMS("DSB engine is busy.\n");
> +		DRM_ERROR("DSB engine is busy.\n");
>   		return false;
>   	}
>   
> 

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

Re: [Intel-gfx] [PATCH 1/9] drm/i915/dsb: Replace HAS_DSB check with dsb->cmd_buf check

[Sharma, Swati2]

Hi Ville,

Can you please rebase on current drm-tip and submit new revision so that 
new run can be executed? BAT failure was observed with the last revision
submitted.

On 29-Jan-20 11:50 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We may want to not use the DSB even if the platform has one.
> So replace the HAS_DSB check in the _put() with a cmd_buf check
> that will work in either case.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dsb.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 9dd18144a664..12776f09f227 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -160,9 +160,8 @@ intel_dsb_get(struct intel_crtc *crtc)
>   void intel_dsb_put(struct intel_dsb *dsb)
>   {
>   	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> -	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>   
> -	if (!HAS_DSB(i915))
> +	if (!dsb->cmd_buf)
>   		return;
>   
>   	if (WARN_ON(dsb->refcount == 0))
> 

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