[PATCH] drm/i915: Avoid flicker with horizontal panning on 830GM

[PATCH] drm/i915: Avoid flicker with horizontal panning on 830GM

[Thomas Richter]

Dear intel-gfx developers,

When panning is enabled on the 830GM, horizontal panning creates a lot 
of flickering on specific pixel positions.
After testing, I found that the reason for this is that panning works by 
altering the frame origin pointer, which,
however, has certain alignment restrictions. If the pointer is not 
aligned correctly, the screen starts to flicker
as, probably, DMA fails.

The following patch against drm/i915/intel_display.c fixes the issue by 
ensuring correct alignment. As result,
horizontal panning works correctly, but is a bit "jumpy". Unclear 
whether the problem affects any other
chipset revisions, thus the patch is currently only enabled for rev.2.

Greetings,
     Thomas


diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index bcb62fe..8304e30 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1987,8 +1987,13 @@ static int i9xx_update_plane(struct drm_crtc 
*crtc, struct drm_framebuffer *fb,

         I915_WRITE(reg, dspcntr);

-       linear_offset = y * fb->pitches[0] + x * (fb->bits_per_pixel / 8);
-
+       if (INTEL_INFO(dev)->gen > 2) {
+         linear_offset = y * fb->pitches[0] + x * (fb->bits_per_pixel / 8);
+       } else {
+         /* align the linear offset to 64 pixel boundaries */
+         linear_offset = y * fb->pitches[0] + (x & -32) * 
(fb->bits_per_pixel / 8);
+       }
+
         if (INTEL_INFO(dev)->gen >= 4) {
                 intel_crtc->dspaddr_offset =
                         intel_gen4_compute_page_offset(&x, &y, 
obj->tiling_mode,

Re: [PATCH] drm/i915: Avoid flicker with horizontal panning on 830GM

[Daniel Vetter]

On Sun, Sep 01, 2013 at 07:01:49PM +0200, Thomas Richter wrote:
> Dear intel-gfx developers,
> 
> When panning is enabled on the 830GM, horizontal panning creates a
> lot of flickering on specific pixel positions.
> After testing, I found that the reason for this is that panning
> works by altering the frame origin pointer, which,
> however, has certain alignment restrictions. If the pointer is not
> aligned correctly, the screen starts to flicker
> as, probably, DMA fails.
> 
> The following patch against drm/i915/intel_display.c fixes the issue
> by ensuring correct alignment. As result,
> horizontal panning works correctly, but is a bit "jumpy". Unclear
> whether the problem affects any other
> chipset revisions, thus the patch is currently only enabled for rev.2.

I've just looked at the docs and they only mention that the base address
must be pixel aligned. But it could very well be that the watermarks are a
bit off for your chipset. The below quick hack should test this theory.
-Daniel


diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index dfdc7ad..990b1f4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1673,7 +1673,7 @@ static void i830_update_wm(struct drm_device *dev)
 				       dev_priv->display.get_fifo_size(dev, 0),
 				       4, latency_ns);
 	fwater_lo = I915_READ(FW_BLC) & ~0xfff;
-	fwater_lo |= (3<<8) | planea_wm;
+	fwater_lo |= (3<<8) | 0;
 
 	DRM_DEBUG_KMS("Setting FIFO watermarks - A: %d\n", planea_wm);
 
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Avoid flicker with horizontal panning on 830GM

[Thomas Richter]

Hi Daniel,

> I've just looked at the docs and they only mention that the base address
> must be pixel aligned. But it could very well be that the watermarks are a
> bit off for your chipset. The below quick hack should test this theory.
> -Daniel
>
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index dfdc7ad..990b1f4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1673,7 +1673,7 @@ static void i830_update_wm(struct drm_device *dev)
>   				       dev_priv->display.get_fifo_size(dev, 0),
>   				       4, latency_ns);
>   	fwater_lo = I915_READ(FW_BLC)&  ~0xfff;
> -	fwater_lo |= (3<<8) | planea_wm;
> +	fwater_lo |= (3<<8) | 0;
>
>   	DRM_DEBUG_KMS("Setting FIFO watermarks - A: %d\n", planea_wm);
>
Checked with the above modifications. Unfortunately, the result is 
negative. With the above modifications and my changes commented out, the 
screen flickers in normal state (without panning) but in a different 
way: With the above enabled, you get a rather irregular almost 
pseudo-random flicker, and not the 60/30Hz flicker I see when panning 
horizontally. If I add horizontal panning, then I also get this 
irregular flicker, except when scrolling to the "forbidden positions" at 
which I get the regular hi-frequency flicker again.

However, now that I checked closer, I found that my patch has also a 
drawback, namely the hardware scroll position and the requested scroll 
position disagrees, i.e. the mouse pointer is not exactly where it 
should be, i.e. the mouse pointer hot-spot is off.

Is there a way to indicate the calling method what the actual 
panning/scroll position is if it is different from the requested position?

Is there a different method to scroll the screen than to adjust the 
screen origin? Old hardware had not only a screen pointer, but also a 
pixel-offset (horizontal scroll) register. Is there something like this 
on the 830M to work around the observed trouble?

Greetings,
     Thomas

PS: A closer inspection shows that the screen flickers if the panning 
position x has the property that (x mod 16) != 0. Strange enough.

Re: [PATCH] drm/i915: Avoid flicker with horizontal panning on 830GM

[Ville Syrjälä]

On Mon, Sep 02, 2013 at 09:10:22AM +0200, Daniel Vetter wrote:
> On Sun, Sep 01, 2013 at 07:01:49PM +0200, Thomas Richter wrote:
> > Dear intel-gfx developers,
> > 
> > When panning is enabled on the 830GM, horizontal panning creates a
> > lot of flickering on specific pixel positions.
> > After testing, I found that the reason for this is that panning
> > works by altering the frame origin pointer, which,
> > however, has certain alignment restrictions. If the pointer is not
> > aligned correctly, the screen starts to flicker
> > as, probably, DMA fails.
> > 
> > The following patch against drm/i915/intel_display.c fixes the issue
> > by ensuring correct alignment. As result,
> > horizontal panning works correctly, but is a bit "jumpy". Unclear
> > whether the problem affects any other
> > chipset revisions, thus the patch is currently only enabled for rev.2.
> 
> I've just looked at the docs and they only mention that the base address
> must be pixel aligned.

I first thought this might be some issue w/ double wide mode (if it's
enabled, not sure) but I don't see any relevant restrictions for double
wide. But that investigation spurred me to write pipe config support for
double wide mode. I'll post those after a quick smoke test.

BTW my 855 isn't affected by this issue. First I though I was seeing
something similar but it turns out I had set the fb size too big, so the
ddx had to resort to copying.

Another idea that I had was that I think 830 has a bit of wonkiness w/
the double buffered registers (latching happens regardless of DSPADDR
writes, and reads use the the active set of registers). But I don't
think that can explain your issue especially since DSPCNTR is the only
register we read in set_base.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Avoid flicker with horizontal panning on 830GM

[Daniel Vetter]

On Mon, Sep 02, 2013 at 03:58:59PM +0200, Thomas Richter wrote:
> Hi Daniel,
> 
> >I've just looked at the docs and they only mention that the base address
> >must be pixel aligned. But it could very well be that the watermarks are a
> >bit off for your chipset. The below quick hack should test this theory.
> >-Daniel
> >
> >
> >diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >index dfdc7ad..990b1f4 100644
> >--- a/drivers/gpu/drm/i915/intel_pm.c
> >+++ b/drivers/gpu/drm/i915/intel_pm.c
> >@@ -1673,7 +1673,7 @@ static void i830_update_wm(struct drm_device *dev)
> >  				       dev_priv->display.get_fifo_size(dev, 0),
> >  				       4, latency_ns);
> >  	fwater_lo = I915_READ(FW_BLC)&  ~0xfff;
> >-	fwater_lo |= (3<<8) | planea_wm;
> >+	fwater_lo |= (3<<8) | 0;
> >
> >  	DRM_DEBUG_KMS("Setting FIFO watermarks - A: %d\n", planea_wm);
> >
> Checked with the above modifications. Unfortunately, the result is
> negative. With the above modifications and my changes commented out,
> the screen flickers in normal state (without panning) but in a
> different way: With the above enabled, you get a rather irregular
> almost pseudo-random flicker, and not the 60/30Hz flicker I see when
> panning horizontally. If I add horizontal panning, then I also get
> this irregular flicker, except when scrolling to the "forbidden
> positions" at which I get the regular hi-frequency flicker again.

Hm, I've probably botched the watermarks again. Can you please retest with
the below diff?

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index dfdc7ad..b667ff0 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1064,6 +1064,8 @@ static unsigned long intel_calculate_wm(unsigned long clock_in_khz,
 
 	DRM_DEBUG_KMS("FIFO watermark level: %ld\n", wm_size);
 
+	wm_size = 0;
+
 	/* Don't promote wm_size to unsigned... */
 	if (wm_size > (long)wm->max_wm)
 		wm_size = wm->max_wm;

> However, now that I checked closer, I found that my patch has also a
> drawback, namely the hardware scroll position and the requested
> scroll position disagrees, i.e. the mouse pointer is not exactly
> where it should be, i.e. the mouse pointer hot-spot is off.
> 
> Is there a way to indicate the calling method what the actual
> panning/scroll position is if it is different from the requested
> position?
> 
> Is there a different method to scroll the screen than to adjust the
> screen origin? Old hardware had not only a screen pointer, but also
> a pixel-offset (horizontal scroll) register. Is there something like
> this on the 830M to work around the observed trouble?

The kms api doesn't really allow for such information to be passed around.
And since current userspace expects this to Just Work we need to be rather
careful with such hacks. Since the docs don't mention any such requirement
I think we should try a bit and figure out whether changing the watermarks
fixes this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Avoid flicker with horizontal panning on 830GM

[Ville Syrjälä]

On Mon, Sep 02, 2013 at 03:58:59PM +0200, Thomas Richter wrote:
> Hi Daniel,
> 
> > I've just looked at the docs and they only mention that the base address
> > must be pixel aligned. But it could very well be that the watermarks are a
> > bit off for your chipset. The below quick hack should test this theory.
> > -Daniel
> >
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index dfdc7ad..990b1f4 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -1673,7 +1673,7 @@ static void i830_update_wm(struct drm_device *dev)
> >   				       dev_priv->display.get_fifo_size(dev, 0),
> >   				       4, latency_ns);
> >   	fwater_lo = I915_READ(FW_BLC)&  ~0xfff;
> > -	fwater_lo |= (3<<8) | planea_wm;
> > +	fwater_lo |= (3<<8) | 0;
> >
> >   	DRM_DEBUG_KMS("Setting FIFO watermarks - A: %d\n", planea_wm);
> >
> Checked with the above modifications. Unfortunately, the result is 
> negative. With the above modifications and my changes commented out, the 
> screen flickers in normal state (without panning) but in a different 
> way: With the above enabled, you get a rather irregular almost 
> pseudo-random flicker, and not the 60/30Hz flicker I see when panning 
> horizontally. If I add horizontal panning, then I also get this 
> irregular flicker, except when scrolling to the "forbidden positions" at 
> which I get the regular hi-frequency flicker again.
> 
> However, now that I checked closer, I found that my patch has also a 
> drawback, namely the hardware scroll position and the requested scroll 
> position disagrees, i.e. the mouse pointer is not exactly where it 
> should be, i.e. the mouse pointer hot-spot is off.

I suppose you could fix up the cursor position in the kernel to take into
account the adjustment. But that's a bit iffy in my opinion since that
makes big assumptions about what the user is intending to do with the the
cursor. Ie. the user could just want to use the cursor to show some overlay
at a fixed position on the display.

> Is there a way to indicate the calling method what the actual 
> panning/scroll position is if it is different from the requested position?

I guess you could update the crtc->x/y info and user space could then
read them out after each setcrtc ioctl. Or you could simply hardcode the
knowledge of this restriction to the ddx.

Or maybe you could just return an error for setcrtc when the alignment
is off. Not sure how the userspace will react to that though. It might
still set the cursor to the wrong position.

> 
> Is there a different method to scroll the screen than to adjust the 
> screen origin? Old hardware had not only a screen pointer, but also a 
> pixel-offset (horizontal scroll) register. Is there something like this 
> on the 830M to work around the observed trouble?

DSPADDR is the only register for this purpose.

> 
> Greetings,
>      Thomas
> 
> PS: A closer inspection shows that the screen flickers if the panning 
> position x has the property that (x mod 16) != 0. Strange enough.

Does it depend on the bpp?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Avoid flicker with horizontal panning on 830GM

[Thomas Richter]

On 02.09.2013 16:18, Daniel Vetter wrote:
>>
>> Checked with the above modifications. Unfortunately, the result is
>> negative. With the above modifications and my changes commented out,
>> the screen flickers in normal state (without panning) but in a
>> different way: With the above enabled, you get a rather irregular
>> almost pseudo-random flicker, and not the 60/30Hz flicker I see when
>> panning horizontally. If I add horizontal panning, then I also get
>> this irregular flicker, except when scrolling to the "forbidden
>> positions" at which I get the regular hi-frequency flicker again.
> Hm, I've probably botched the watermarks again. Can you please retest with
> the below diff?
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index dfdc7ad..b667ff0 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1064,6 +1064,8 @@ static unsigned long intel_calculate_wm(unsigned long clock_in_khz,
>
>   	DRM_DEBUG_KMS("FIFO watermark level: %ld\n", wm_size);
>
> +	wm_size = 0;
> +
>   	/* Don't promote wm_size to unsigned... */
>   	if (wm_size>  (long)wm->max_wm)
>   		wm_size = wm->max_wm;

Not much difference, probably in the type of flicker. Very unstable, 
pseudo-random flicker at offset = 0, which gets a bit better for 
somewhat larger positions, but still the high-frequency flicker at 
forbidden positions. The flicker type definitely depends on the scroll 
position, but the high-freq flicker at (x & 16) != 0 does not go away 
neither did it change in any way. )-: Only the flicker frequency at 
positions (x & 16) == 0 did change, and then depends on (x & 15).

> The kms api doesn't really allow for such information to be passed around.
> And since current userspace expects this to Just Work we need to be rather
> careful with such hacks. Since the docs don't mention any such requirement
> I think we should try a bit and figure out whether changing the watermarks
> fixes this.
Is there anything I could do to confirm that the high-freq flicker is 
really due to watermarks? Since the above modifications do not really 
seem to work. It might also be that the amount of prefetch the FIFO 
requires depends on the memory alignment as in "prefetch must occur 
starting from first 256-bit aligned position" or something?

Greetings,
     Thomas

Re: [PATCH] drm/i915: Avoid flicker with horizontal panning on 830GM

[Thomas Richter]

Am 02.09.2013 16:18, schrieb Daniel Vetter:
> Hm, I've probably botched the watermarks again. Can you please retest with
> the below diff?
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index dfdc7ad..b667ff0 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1064,6 +1064,8 @@ static unsigned long intel_calculate_wm(unsigned long clock_in_khz,
>
>   	DRM_DEBUG_KMS("FIFO watermark level: %ld\n", wm_size);
>
> +	wm_size = 0;
> +
>   	/* Don't promote wm_size to unsigned... */
>   	if (wm_size>  (long)wm->max_wm)
>   		wm_size = wm->max_wm;
As this didn't work either, could you point me to a documentation of the 
830M so I could have a look myself? Unfortunately, I did not find 
anything appropriate, only an overall description of the chipset that 
lists all the RAM/addressing related registers and a high-level 
description of the gpu features, but nothing detailed on the 2D graphics 
and VGA-related registers.

Thanks,
     Thomas

Re: [PATCH] drm/i915: Avoid flicker with horizontal panning on 830GM

[Daniel Vetter]

On Thu, Sep 5, 2013 at 4:51 PM, Thomas Richter <thor@math.tu-berlin.de> wrote:
> Am 02.09.2013 16:18, schrieb Daniel Vetter:
>>
>> Hm, I've probably botched the watermarks again. Can you please retest with
>> the below diff?
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index dfdc7ad..b667ff0 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>
>> @@ -1064,6 +1064,8 @@ static unsigned long intel_calculate_wm(unsigned
>> long clock_in_khz,
>>
>>         DRM_DEBUG_KMS("FIFO watermark level: %ld\n", wm_size);
>>
>> +       wm_size = 0;
>> +
>>         /* Don't promote wm_size to unsigned... */
>>         if (wm_size>  (long)wm->max_wm)
>>                 wm_size = wm->max_wm;
>
> As this didn't work either, could you point me to a documentation of the
> 830M so I could have a look myself? Unfortunately, I did not find anything
> appropriate, only an overall description of the chipset that lists all the
> RAM/addressing related registers and a high-level description of the gpu
> features, but nothing detailed on the 2D graphics and VGA-related registers.

Documentation for i830M was unfortunately never published :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch