[PATCH] Pipe-A underrun workaround for i830 chipset.

[PATCH] Pipe-A underrun workaround for i830 chipset.

[Thomas Richter]

[-- Attachment #1: Type: text/plain, Size: 365 bytes --]

Dear Daniel, dear intel-experts,

please find a patch for the dreadful pipe-A underruns on the i830 
attached that works at least in the linear framebuffer mode. As far as 
the tiling mode is concerned, I'm still scratching my head about it. It 
as at this time unclear how precisely this works, but I'll keep looking.
At least this is something.

So long,
	Thomas

[-- Attachment #2: 0011-Added-a-workaround-for-pipe-A-underruns-on-i830-with.patch --]
[-- Type: text/x-patch, Size: 3245 bytes --]

>From ba72c1e506bb162f8ac595af94cc6ed850439883 Mon Sep 17 00:00:00 2001
From: Thomas Richter <thor@math.tu-berlin.de>
Date: Thu, 14 Nov 2013 18:16:14 +0100
Subject: [PATCH 11/11] Added a workaround for pipe-A underruns on i830 with
 panning.


Signed-off-by: Thomas Richter <thor@math.tu-berlin.de>
---
 drivers/gpu/drm/i915/intel_display.c |   61 ++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 971f6b9..21895b8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2002,6 +2002,64 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
 	}
 }
 
+/*
+** Update the linear frame pointer for i830 based devices. Due to some
+** unknown hardware feature, updating the frame pointer may cause
+** the unified FIFO of these chips run dry, then causing a flicker
+** and a potential shutdown of the video overlay, or worse.
+** thor, 14.11.2013
+*/
+static void i830_update_plane_offset(struct drm_crtc *crtc,
+				     struct drm_framebuffer *fb,
+				     unsigned long linear_offset)
+{
+	struct drm_i915_gem_object *obj;
+	struct intel_framebuffer *intel_fb;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	unsigned long planeadr, oldadr;
+	int plane = intel_crtc->plane;
+	u32 reg = DSPCNTR(plane);
+
+	intel_fb = to_intel_framebuffer(fb);
+	obj = intel_fb->obj;
+
+	planeadr = i915_gem_obj_ggtt_offset(obj) + linear_offset;
+	DRM_DEBUG_KMS("Plane address is 0x%lx", planeadr);
+	/* I830 panning flicker work around.
+	** Only for non-LVDS output, only for i830.
+	** Tiling mode is still not supported.
+	*/
+	if (obj->tiling_mode != I915_TILING_NONE) {
+		if ((planeadr & 0x40)) {
+			DRM_DEBUG_KMS("Detected potential flicker");
+			DRM_DEBUG_KMS("No workaround for tiling mode");
+			DRM_DEBUG_KMS("Use a linear frame buffer");
+		}
+	} else {
+		switch (fb->pixel_format) {
+		case DRM_FORMAT_XRGB1555:
+		case DRM_FORMAT_ARGB1555:
+		case DRM_FORMAT_RGB565:
+		case DRM_FORMAT_XRGB8888:
+		case DRM_FORMAT_ARGB8888:
+		case DRM_FORMAT_XBGR8888:
+		case DRM_FORMAT_ABGR8888:
+			oldadr = I915_READ(DSPADDR(plane));
+			if (((oldadr ^ planeadr) & 0x40) &&
+			    (planeadr & 0xc0) == 0xc0) {
+				I915_WRITE(DSPADDR(plane),
+					   planeadr & (~(0x80)));
+				POSTING_READ(reg);
+				intel_wait_for_vblank(dev, intel_crtc->pipe);
+			}
+			break;
+		}
+	}
+	I915_WRITE(DSPADDR(plane), planeadr);
+}
+
 static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 			     int x, int y)
 {
@@ -2095,6 +2153,9 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 				     i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset);
 		I915_WRITE(DSPTILEOFF(plane), (y << 16) | x);
 		I915_WRITE(DSPLINOFF(plane), linear_offset);
+	} else if (INTEL_INFO(dev)->gen == 2 && IS_I830(dev) &&
+		   !intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
+		i830_update_plane_offset(crtc, fb, linear_offset);
 	} else
 		I915_WRITE(DSPADDR(plane), i915_gem_obj_ggtt_offset(obj) + linear_offset);
 	POSTING_READ(reg);
-- 
1.7.10.4


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] Pipe-A underrun workaround for i830 chipset.

[Daniel Vetter]

On Thu, Nov 14, 2013 at 06:17:03PM +0100, Thomas Richter wrote:
> Dear Daniel, dear intel-experts,
> 
> please find a patch for the dreadful pipe-A underruns on the i830
> attached that works at least in the linear framebuffer mode. As far
> as the tiling mode is concerned, I'm still scratching my head about
> it. It as at this time unclear how precisely this works, but I'll
> keep looking.
> At least this is something.

A few bikesheds below. Once the polish is done I'll merge it.
-Daniel

> 
> So long,
> 	Thomas

> From ba72c1e506bb162f8ac595af94cc6ed850439883 Mon Sep 17 00:00:00 2001
> From: Thomas Richter <thor@math.tu-berlin.de>
> Date: Thu, 14 Nov 2013 18:16:14 +0100
> Subject: [PATCH 11/11] Added a workaround for pipe-A underruns on i830 with
>  panning.
> 

A bit commit message summarizing what has been done thus far would be
good.

Also please keep here a patch reflog, iirc we're at v3 or so. E.g.

v3: Extract workaround into i830_update_plane_offset function.

> 
> Signed-off-by: Thomas Richter <thor@math.tu-berlin.de>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   61 ++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 971f6b9..21895b8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2002,6 +2002,64 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
>  	}
>  }
>  
> +/*
> +** Update the linear frame pointer for i830 based devices. Due to some
> +** unknown hardware feature, updating the frame pointer may cause
> +** the unified FIFO of these chips run dry, then causing a flicker
> +** and a potential shutdown of the video overlay, or worse.
> +** thor, 14.11.2013
> +*/

Multi-line comments should be like this:

/*
 * text ...
 */

with the text flown to align to 80 chars. vim will do this for you.

> +static void i830_update_plane_offset(struct drm_crtc *crtc,
> +				     struct drm_framebuffer *fb,
> +				     unsigned long linear_offset)
> +{
> +	struct drm_i915_gem_object *obj;
> +	struct intel_framebuffer *intel_fb;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	unsigned long planeadr, oldadr;
> +	int plane = intel_crtc->plane;
> +	u32 reg = DSPCNTR(plane);
> +
> +	intel_fb = to_intel_framebuffer(fb);
> +	obj = intel_fb->obj;
> +
> +	planeadr = i915_gem_obj_ggtt_offset(obj) + linear_offset;
> +	DRM_DEBUG_KMS("Plane address is 0x%lx", planeadr);

I'd ditch this debug output due to dmesg spam. I guess it was useful for
debugging, but should go away for upstream.

> +	/* I830 panning flicker work around.
> +	** Only for non-LVDS output, only for i830.
> +	** Tiling mode is still not supported.
> +	*/
> +	if (obj->tiling_mode != I915_TILING_NONE) {
> +		if ((planeadr & 0x40)) {

We tend to use decimal numbers for limits like these.

> +			DRM_DEBUG_KMS("Detected potential flicker");
> +			DRM_DEBUG_KMS("No workaround for tiling mode");
> +			DRM_DEBUG_KMS("Use a linear frame buffer");
> +		}

I'd just ditch this for now, no point spamming dmesg with that much noise
if we don't have a fix.

> +	} else {
> +		switch (fb->pixel_format) {
> +		case DRM_FORMAT_XRGB1555:
> +		case DRM_FORMAT_ARGB1555:
> +		case DRM_FORMAT_RGB565:
> +		case DRM_FORMAT_XRGB8888:
> +		case DRM_FORMAT_ARGB8888:
> +		case DRM_FORMAT_XBGR8888:
> +		case DRM_FORMAT_ABGR8888:

The switch here can be killed - higher levels already check for valid
pixel formats.

> +			oldadr = I915_READ(DSPADDR(plane));
> +			if (((oldadr ^ planeadr) & 0x40) &&
> +			    (planeadr & 0xc0) == 0xc0) {
> +				I915_WRITE(DSPADDR(plane),
> +					   planeadr & (~(0x80)));
> +				POSTING_READ(reg);
> +				intel_wait_for_vblank(dev, intel_crtc->pipe);
> +			}
> +			break;
> +		}
> +	}
> +	I915_WRITE(DSPADDR(plane), planeadr);
> +}
> +
>  static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  			     int x, int y)
>  {
> @@ -2095,6 +2153,9 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  				     i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset);
>  		I915_WRITE(DSPTILEOFF(plane), (y << 16) | x);
>  		I915_WRITE(DSPLINOFF(plane), linear_offset);
> +	} else if (INTEL_INFO(dev)->gen == 2 && IS_I830(dev) &&
> +		   !intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {

IS_I830(dev) is good enough - the gen2 check is redundant and i830M
doesn't have an LVDS encoder (lvds is only possible with the DVO encoder).

> +		i830_update_plane_offset(crtc, fb, linear_offset);
>  	} else
>  		I915_WRITE(DSPADDR(plane), i915_gem_obj_ggtt_offset(obj) + linear_offset);
>  	POSTING_READ(reg);
> -- 
> 1.7.10.4
> 


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] Pipe-A underrun workaround for i830 chipset.

[Thomas Richter]

Hi Daniel,

a couple of comments on your comments. (-;

> with the text flown to align to 80 chars. vim will do this for you.

..which I ain't using. "Using" is not a word that goes along well with 
"vim". (-; But no, let's not start an editor war here.

>> +	if (obj->tiling_mode != I915_TILING_NONE) {
>> +		if ((planeadr & 0x40)) {
>
> We tend to use decimal numbers for limits like these.

That's actually a bitmask to check for alignment. (-; In principle, the 
whole "if" can go because it's currently only a dummy with the real code 
still missing once I understand what's going on in the tiled mode. Or at 
least, once I find a way to fiddle around the pipe underflow.

>> +		switch (fb->pixel_format) {
>> +		case DRM_FORMAT_XRGB1555:
>> +		case DRM_FORMAT_ARGB1555:
>> +		case DRM_FORMAT_RGB565:
>> +		case DRM_FORMAT_XRGB8888:
>> +		case DRM_FORMAT_ARGB8888:
>> +		case DRM_FORMAT_XBGR8888:
>> +		case DRM_FORMAT_ABGR8888:
>
> The switch here can be killed - higher levels already check for valid
> pixel formats.

Validity is not quite the issue here. The issue is that the workaround 
depends on the bytes per pixel. The formats above are all 24 and 16 bit 
modes for which the workaround, well, works. 8bpp and 30bpp modes are 
also currently unsupported. If you prefer, I could also compute a bpp 
value at the caller and pass it along.

> IS_I830(dev) is good enough - the gen2 check is redundant and i830M
> doesn't have an LVDS encoder (lvds is only possible with the DVO encoder).

Ok.

Greetings,
	Thomas