From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com>
Subject: Re: [PATCH v2 3/5] drm/i915: Make sprite updates atomic
Date: Mon, 20 Jan 2014 20:38:13 +0200
Message-ID: <20140120183813.GY9454@intel.com>
References: <1389982146-1460-1-git-send-email-ville.syrjala@linux.intel.com>
	<1389982146-1460-4-git-send-email-ville.syrjala@linux.intel.com>
	<20140120162339.GJ15089@phenom.ffwll.local>
	<20140120165644.GW9454@intel.com>
	<CAKMK7uGAQbxhC49UXqei-Mr9A+Wr281PALr_EbFQ7RumFzT41Q@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Return-path: <intel-gfx-bounces@lists.freedesktop.org>
Received: from mga02.intel.com (mga02.intel.com [134.134.136.20])
	by gabe.freedesktop.org (Postfix) with ESMTP id F3041FA848
	for <intel-gfx@lists.freedesktop.org>;
	Mon, 20 Jan 2014 10:38:33 -0800 (PST)
Content-Disposition: inline
In-Reply-To: <CAKMK7uGAQbxhC49UXqei-Mr9A+Wr281PALr_EbFQ7RumFzT41Q@mail.gmail.com>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/intel-gfx>
List-Post: <mailto:intel-gfx@lists.freedesktop.org>
List-Help: <mailto:intel-gfx-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=subscribe>
Sender: intel-gfx-bounces@lists.freedesktop.org
Errors-To: intel-gfx-bounces@lists.freedesktop.org
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx <intel-gfx@lists.freedesktop.org>
List-Id: intel-gfx@lists.freedesktop.org

On Mon, Jan 20, 2014 at 06:43:42PM +0100, Daniel Vetter wrote:
> On Mon, Jan 20, 2014 at 5:56 PM, Ville Syrj=E4l=E4
> <ville.syrjala@linux.intel.com> wrote:
> >> > +{
> >> > +   struct drm_device *dev =3D crtc->dev;
> >> > +   struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc);
> >> > +   const struct drm_display_mode *mode =3D &intel_crtc->config.adju=
sted_mode;
> >> > +   enum pipe pipe =3D intel_crtc->pipe;
> >> > +   /* FIXME needs to be calibrated sensibly */
> >> > +   unsigned int min =3D mode->crtc_vblank_start - usecs_to_scanline=
s(mode, 100);
> >> > +   unsigned int max =3D mode->crtc_vblank_start - 1;
> >> > +   long timeout =3D msecs_to_jiffies_timeout(1);
> >> > +   unsigned int scanline;
> >> > +
> >> > +   if (WARN_ON(drm_vblank_get(dev, pipe)))
> >> > +           return;
> >> > +
> >> > +   local_irq_disable();
> >> > +
> >> > +   intel_crtc->vbl_received =3D false;
> >
> > Now that you got me thinking about barriers again, I wonder if I should
> > add an explicit compiler barrier here. The intel_get_crtc_scanline() ca=
ll
> > should act as a compiler barrier though, so it shouldn't be needed. So
> > maybe I should add a comment here too?
> =

> This piece of code here was the actual reason I've asked for barrier
> comments ;-) Ofc document the wake_up/wait_even barriers for the irq
> write -> read here is also good, but this write here is imo the
> crucial piece.
> =

> Also I think we should have a check here that the caller is holding
> the crtc lock, to make sure that only one thread is using this
> facility. Oh, and one more while I ponder this: We enable interrupt
> processing before crtcs are fully set up, so chasing the pipe->crtc
> mapping from the irq handling either needs to be done carefully (i.e.
> a small analysis of why we won't ever get an vblank interrupt before
> the crtc is set up) or needs to use something statically allocated in
> dev_priv.

We have the same problem already w/ underrun interrupts at least, no?

-- =

Ville Syrj=E4l=E4
Intel OTC