Re: [Intel-gfx] [PATCH 2/3] drm/i915: Don't enable the cursor on a disable pipe

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

On Thu, Sep 12, 2013 at 09:08:17PM +0100, Chris Wilson wrote:
> On Thu, Sep 12, 2013 at 10:45:42PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On HSW enabling a plane on a disabled pipe may hang the entire system.
> > And there's no good reason for doing it ever, so just don't.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 18043a2..d0137b6 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6793,6 +6793,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> >  	u32 base, pos;
> >  	bool visible;
> >  
> > +	if (!intel_crtc->active)
> > +		return;
> 
> This is misleading since we do expect to call this function whilst
> turning off the crtc. This check makes it appear that such calls might
> be wrong.

crtc->active is true until the pipe has been fully turned off.

> Also the !crtc->enabled following intel_crtc->active makes
> ones question their sanity.

I actually forgot we had that there. If you recall I'm actually
removing it in my cursor visibility fixes. But I didn't realize that
we don't actually clear crtc->config to zero for inactive pipes, so
I actually would have introduced a bug there.

> So I feel this check detracts from readability of the function.

I just wanted something minimal for stable. But since we have the
->enabled check there currently I think this patch could just be
dropped as ->enabled should match ->active outside modeset
operations, and during modeset operations we're covered by Jani's
patch to remove the extra cursor calls.

But I need to amend my earlier visibility fixes to add either ->active
or ->enabled check back. Or I need to make it so that crtc->config
gets cleared for disabled pipes.

-- 
Ville Syrjälä
Intel OTC