From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [Intel-gfx] [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe Date: Fri, 13 Sep 2013 10:40:16 +0300 Message-ID: <20130913074015.GH20128@intel.com> References: <1379015143-13822-1-git-send-email-ville.syrjala@linux.intel.com> <1379015143-13822-4-git-send-email-ville.syrjala@linux.intel.com> <20130912201356.GC14235@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: <20130912201356.GC14235@nuc-i3427.alporthouse.com> Sender: stable-owner@vger.kernel.org To: Chris Wilson , intel-gfx@lists.freedesktop.org, stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org 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.c= om wrote: > > From: Ville Syrj=E4l=E4 > >=20 > > On HSW enabling a plane on a disabled pipe may hang the entire syst= em. > > And there's no good reason for doing it ever, so just don't. > >=20 > > Cc: stable@vger.kernel.org > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > drivers/gpu/drm/i915/intel_sprite.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > >=20 > > 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, str= uct drm_crtc *crtc, > > .y2 =3D crtc_y + crtc_h, > > }; > > const struct drm_rect clip =3D { > > - .x2 =3D crtc->mode.hdisplay, > > - .y2 =3D crtc->mode.vdisplay, > > + .x2 =3D intel_crtc->active ? crtc->mode.hdisplay : 0, > > + .y2 =3D intel_crtc->active ? crtc->mode.vdisplay : 0, > > }; >=20 > Too much magic that looks like it would have interesting effects late= r > in the function. This function should only be called on an active CRT= C, > so declare it so: >=20 > 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? Especiall= y 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 wh= en the pipe is off outside modeset operations. During modeset even if we a= dd 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 ;) --=20 Ville Syrj=E4l=E4 Intel OTC