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@xxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxx>
> ---
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
