Re: [Intel-gfx] [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe

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

On Thu, Sep 12, 2013 at 09:13:56PM +0100, Chris Wilson wrote:
> On Thu, Sep 12, 2013 at 10:45:43PM +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_sprite.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index d9c7a66..4f11eb1 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -652,8 +652,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  		.y2 = crtc_y + crtc_h,
> >  	};
> >  	const struct drm_rect clip = {
> > -		.x2 = crtc->mode.hdisplay,
> > -		.y2 = crtc->mode.vdisplay,
> > +		.x2 = intel_crtc->active ? crtc->mode.hdisplay : 0,
> > +		.y2 = intel_crtc->active ? crtc->mode.vdisplay : 0,
> >  	};
> 
> Too much magic that looks like it would have interesting effects later
> in the function. This function should only be called on an active CRTC,
> so declare it so:
> 
>   if (WARN_ON(!intel_crtc->active))
>     return -EMONKEY;

I'm actually perfectly happy to let users set up planes on a disabled
pipe. We allow it for cursors already, so why not all planes? Especially
as cursors will be just drm_planes in the future.

And actually we do have a PIPECONF_ENABLED check in there that I was
going to remove but forgot. That already prevents setting up a plane when
the pipe is off outside modeset operations. During modeset even if we add
an ->active check we might be left with a race window when the pipe is not
fully up and running yet. I think to catch those we just need to slap
asserts at the appropriate place in modeset.

So I gues we don't really need this patch in stable either. Hmm. fixing
bugs is easier than I thought ;)

-- 
Ville Syrjälä
Intel OTC