From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Don't enable the cursor on a disable pipe Date: Fri, 13 Sep 2013 10:28:34 +0300 Message-ID: <20130913072833.GF20128@intel.com> References: <1379015143-13822-1-git-send-email-ville.syrjala@linux.intel.com> <1379015143-13822-3-git-send-email-ville.syrjala@linux.intel.com> <20130912200817.GA14235@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: <20130912200817.GA14235@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:08:17PM +0100, Chris Wilson wrote: > On Thu, Sep 12, 2013 at 10:45:42PM +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_display.c | 3 +++ > > 1 file changed, 3 insertions(+) > >=20 > > 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 d= rm_crtc *crtc, > > u32 base, pos; > > bool visible; > > =20 > > + if (!intel_crtc->active) > > + return; >=20 > This is misleading since we do expect to call this function whilst > turning off the crtc. This check makes it appear that such calls migh= t > 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. --=20 Ville Syrj=E4l=E4 Intel OTC