Re: [PATCH 2/6] drm/i915: Always enable the cursor right after the primary plane

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jun 06, 2013 at 06:30:52PM +0200, Egbert Eich wrote:
> On Tue, May 21, 2013 at 05:17:27PM +0200, Egbert Eich wrote:
> > On Wed, May 08, 2013 at 12:50:24PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Follow the same sequence when enabling the cursor plane during
> > > modeset. No point in doing this stuff in different order on different
> > > generations.
> > > 
> > > This should also avoid a needless wait for vblank for the g4x cursor
> > > workaround when the cursor gets enabled anyway.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 10 ++++------
> > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > > 
> > > @@ -3727,6 +3725,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
> > >  
> > >  	intel_enable_pipe(dev_priv, pipe, false);
> > >  	intel_enable_plane(dev_priv, plane, pipe);
> > > +	intel_crtc_update_cursor(crtc, true);
> > >  	if (IS_G4X(dev))
> > >  		g4x_fixup_plane(dev_priv, pipe);
> > >  
> > 
> > As discussed on IRC this may interfere with 
> > 
> > commit 61bc95c1fbbb6a08b55bbe161fdf1ea5493fc595
> > Author: Egbert Eich <eich@suse.com>
> > Date:   Mon Mar 4 09:24:38 2013 -0500
> > 
> >     DRM/i915: On G45 enable cursor plane briefly after enabling the display plane.
> > 
> > described in https://bugs.freedesktop.org/show_bug.cgi?id=61457
> > 
> 
> Today I had the chance to test this. First I tried
> if I can still reproduce the blank with this patch
> added when I disable my voodoo g4x_fixup_plane():
> It turned out it still happens however very rarely
> (like 1 out of 20 tries). When I reenabled my voodoo
> the issue still occurred.
> I had to switch two lines around, ie:
> 
>  	intel_enable_plane(dev_priv, plane, pipe);
>  	if (IS_G4X(dev))
>  		g4x_fixup_plane(dev_priv, pipe);
> +	intel_crtc_update_cursor(crtc, true);
> 
> to avoid the blank screen issue - which is it didn't
> happen in ~75 tries.
> 
> With this change:
> 
> Acked-by: Egbert Eich <eich@suse.com>

Hmm. That means the cursor itself isn't perhaps really relevant, and
it's maybe more about the self refresh.

BTW did you ever try w/ just the self refresh disable but w/o the cursor trick?

We seem to enable self refresh already before we enable any planes (in
intel_update_watermarks()). So maybe something like this would work:

i9xx_crtc_enable()
 ...
 intel_update_watermarks()
 ...
 <SR off>
 intel_enable_pipe()
 intel_enable_plane()
 <enable other planes>
 [ maybe intel_wait_for_vblank(), but we seem to have too many of those already... ]
 <SR back on>

BTW the spec lists an SR workaround, which we don't follow apparently.
It's only relevant for switching between 1 and 2 pipes though. But maybe
there's a more general issue enabling even 1 pipe w/ SR enabled...

I suspect I need to dig more into the g4x watermark/SR stuff at some
point, so this is good practice for me :) I just wish I was able to
reproduce the bug. Maybe I need to find a ctg instead of the elk I
have...

-- 
Ville Syrjälä
Intel OTC