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

[Daniel Vetter <daniel@ffwll.ch>]

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