[1/5] drm/i915: Initialise g4x watermarks for disabled pipes

Commit Message

Chris Wilson April 6, 2011, 6:59 a.m. UTC

On Tue, 05 Apr 2011 18:02:51 -0700, Keith Packard <[e-mail]> wrote:
> On Tue, 05 Apr 2011 22:12:19 +0100, Chris Wilson <[e-mail]> wrote:
> 
> > Indeed, I started by setting them to zero in the caller. Decided that
> > there was some precedent to use the guard_size as the minimum value for
> > unused planes (and so perhaps the unused planes on the unused pipes) and
> > so it was then natural to do it inside g4x_compute_wm. I guess it all
> > depends on how many FIFOs are split between the pipes. Using guard_size,
> > I believe, should be safest.
> 
> guard_size is probably better than random stack stuff. Any opinion on
> whether this should be done in g4x_update_wm instead of g4x_compute_wm0?

I'd prefer to keep the mucking around with intel_watermak_params in the
one spot. How about:

Comments

Keith Packard April 6, 2011, 7:36 a.m. UTC | #1

On Wed, 06 Apr 2011 07:59:37 +0100, Chris Wilson <[e-mail]> wrote:

> I'd prefer to keep the mucking around with intel_watermak_params in the
> one spot. How about:

My concern is that g4x_compute_wm0 is now different from
ironlake_compute_wm0, which seems like a potential trap for the
unwary.

> -	*plane_wm = entries + display->guard_size;
> +	*plane_wm += entries;
...
> -	*cursor_wm = entries + cursor->guard_size;
> +	*cursor_wm += entries;

Uh, no. If we find out that '0' is the right value for the off-case, I
don't want to discover that we forgot to reset these...

Chris Wilson April 6, 2011, 8:02 a.m. UTC | #2

On Wed, 06 Apr 2011 00:36:50 -0700, Keith Packard <[e-mail]> wrote:
> On Wed, 06 Apr 2011 07:59:37 +0100, Chris Wilson <[e-mail]> wrote:
> 
> > I'd prefer to keep the mucking around with intel_watermak_params in the
> > one spot. How about:
> 
> My concern is that g4x_compute_wm0 is now different from
> ironlake_compute_wm0, which seems like a potential trap for the
> unwary.

A trap that I wrote for myself and fell into. The goal was to reduce the
number of copies of the watermark computation by gradual refactoring.

Looks like we can now indeed merge g4x_compute_wm0 and ironlake_compute_wm0
and ignore the off-values for gen5+.

So fix the use of uninitialised values for -fixes and remove the redundant
copy in -next?
-Chris

Keith Packard April 6, 2011, 3:12 p.m. UTC | #3

On Wed, 06 Apr 2011 09:02:22 +0100, Chris Wilson <[e-mail]> wrote:

> Looks like we can now indeed merge g4x_compute_wm0 and ironlake_compute_wm0
> and ignore the off-values for gen5+.

They do seem surprisingly similar at this point...

> So fix the use of uninitialised values for -fixes and remove the redundant
> copy in -next?

Sounds good.

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f1798f2..dbe11eb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3799,11 +3799,14 @@  static bool g4x_compute_wm0(struct drm_device *dev,
 	int line_time_us, line_count;
 	int entries, tlb_miss;
 
+	/* The FIFO requires a minimum number of entries (the guard size)
+	 * as it switches between planes:
+	 */
+	*cursor_wm = cursor->guard_size;
+	*plane_wm = display->guard_size;
+
 	crtc = intel_get_crtc_for_plane(dev, plane);
-	if (crtc->fb == NULL || !crtc->enabled) {
-		*cursor_wm = *plane_wm = display->guard_size;
+	if (crtc->fb == NULL || !crtc->enabled)
 		return false;
-	}
 
 	htotal = crtc->mode.htotal;
 	hdisplay = crtc->mode.hdisplay;
@@ -3816,7 +3819,7 @@  static bool g4x_compute_wm0(struct drm_device *dev,
 	if (tlb_miss > 0)
 		entries += tlb_miss;
 	entries = DIV_ROUND_UP(entries, display->cacheline_size);
-	*plane_wm = entries + display->guard_size;
+	*plane_wm += entries;
 	if (*plane_wm > (int)display->max_wm)
 		*plane_wm = display->max_wm;
 
@@ -3828,7 +3831,7 @@  static bool g4x_compute_wm0(struct drm_device *dev,
 	if (tlb_miss > 0)
 		entries += tlb_miss;
 	entries = DIV_ROUND_UP(entries, cursor->cacheline_size);
-	*cursor_wm = entries + cursor->guard_size;
+	*cursor_wm += entries;
 	if (*cursor_wm > (int)cursor->max_wm)
 		*cursor_wm = (int)cursor->max_wm;