From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mario Kleiner Subject: Re: [PATCH v2 1/3] drm/i915: Skip register reads in i915_get_crtc_scanoutpos() Date: Thu, 26 Sep 2013 18:12:06 +0200 Message-ID: <52445CD6.1080601@gmail.com> References: <20130923103050.GE8482@nuc-i3427.alporthouse.com> <1379936930-26044-1-git-send-email-ville.syrjala@linux.intel.com> <52424C0C.6000609@tuebingen.mpg.de> <20130925081405.GK4531@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ee0-f46.google.com (mail-ee0-f46.google.com [74.125.83.46]) by gabe.freedesktop.org (Postfix) with ESMTP id 07B83E61AE for ; Thu, 26 Sep 2013 09:12:08 -0700 (PDT) Received: by mail-ee0-f46.google.com with SMTP id c13so656115eek.5 for ; Thu, 26 Sep 2013 09:12:06 -0700 (PDT) In-Reply-To: <20130925081405.GK4531@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Cc: intel-gfx@lists.freedesktop.org, Mario Kleiner List-Id: intel-gfx@lists.freedesktop.org On 25.09.13 10:14, Ville Syrj=E4l=E4 wrote: > On Wed, Sep 25, 2013 at 04:35:56AM +0200, Mario Kleiner wrote: >> On 23.09.13 13:48, ville.syrjala@linux.intel.com wrote: >>> From: Ville Syrj=E4l=E4 >>> >>> We have all the information we need in the mode structure, so going and >>> reading it from the hardware is pointless, and slower. >>> >>> We never populated ->get_vblank_timestamp() in the UMS case, and as that >>> is the only way we'd ever call ->get_scanout_position(), we can >>> completely ignore UMS in i915_get_crtc_scanoutpos(). >>> >>> Also reorganize intel_irq_init() a bit to clarify the KMS vs. UMS >>> situation. >>> >>> v2: Drop UMS code >>> >>> Cc: Mario Kleiner >>> Signed-off-by: Ville Syrj=E4l=E4 >>> --- >>> drivers/gpu/drm/i915/i915_irq.c | 43 ++++++++++++++++---------------= ---------- >>> 1 file changed, 17 insertions(+), 26 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i91= 5_irq.c >>> index b356dc1..058f099 100644 >>> --- a/drivers/gpu/drm/i915/i915_irq.c >>> +++ b/drivers/gpu/drm/i915/i915_irq.c >>> @@ -570,24 +570,29 @@ static u32 gm45_get_vblank_counter(struct drm_dev= ice *dev, int pipe) >>> static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, >>> int *vpos, int *hpos) >>> { >>> - drm_i915_private_t *dev_priv =3D (drm_i915_private_t *) dev->dev_priv= ate; >>> - u32 vbl =3D 0, position =3D 0; >>> + struct drm_i915_private *dev_priv =3D dev->dev_private; >>> + struct drm_crtc *crtc =3D dev_priv->pipe_to_crtc_mapping[pipe]; >>> + struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); >>> + const struct drm_display_mode *mode =3D &intel_crtc->config.adjusted_= mode; >>> + u32 position; >>> int vbl_start, vbl_end, htotal, vtotal; >>> bool in_vbl =3D true; >>> int ret =3D 0; >>> - enum transcoder cpu_transcoder =3D intel_pipe_to_cpu_transcoder(dev_p= riv, >>> - pipe); >>> >>> - if (!i915_pipe_enabled(dev, pipe)) { >>> + if (!intel_crtc->active) { >>> DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled " >>> "pipe %c\n", pipe_name(pipe)); >>> return 0; >>> } >>> >>> - /* Get vtotal. */ >>> - vtotal =3D 1 + ((I915_READ(VTOTAL(cpu_transcoder)) >> 16) & 0x1fff); >>> + htotal =3D mode->crtc_htotal; >>> + vtotal =3D mode->crtc_vtotal; >>> + vbl_start =3D mode->crtc_vblank_start; >>> + vbl_end =3D mode->crtc_vblank_end; >>> >>> - if (INTEL_INFO(dev)->gen >=3D 4) { >>> + ret |=3D DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE; >>> + >>> + if (IS_G4X(dev) || INTEL_INFO(dev)->gen >=3D 5) { >>> /* No obvious pixelcount register. Only query vertical >>> * scanout position from Display scan line register. >>> */ >>> @@ -605,29 +610,16 @@ static int i915_get_crtc_scanoutpos(struct drm_de= vice *dev, int pipe, >>> */ >>> position =3D (I915_READ(PIPEFRAMEPIXEL(pipe)) & PIPE_PIXEL_MASK) >= > PIPE_PIXEL_SHIFT; >>> >>> - htotal =3D 1 + ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) & 0x1fff); >>> *vpos =3D position / htotal; >>> *hpos =3D position - (*vpos * htotal); >>> } >>> >>> - /* Query vblank area. */ >>> - vbl =3D I915_READ(VBLANK(cpu_transcoder)); >>> - >>> - /* Test position against vblank region. */ >>> - vbl_start =3D vbl & 0x1fff; >>> - vbl_end =3D (vbl >> 16) & 0x1fff; >>> - >>> - if ((*vpos < vbl_start) || (*vpos > vbl_end)) >>> - in_vbl =3D false; >>> + in_vbl =3D *vpos >=3D vbl_start && *vpos < vbl_end; >> >> I think this should be a <=3D instead of < in *vpos < vbl_end, if it is >> meant to be equal to the line it replaces (not > is <=3D), unless the >> original comparison was off-by-one? > > Yeah, I think the original was wrong, in more ways than one. It forgot > to add +1 to vbl_start/end, and then it did the comparison wrong as > well. > Ah ok, that's possible. Then you have my blessing :). On the Intel side i only had and have sporadic access to an old Intel = GMA-950 (Gen-3?) when writing that function, so i could only really test = one half of the code-path in that function. Also that card only has a = VGA output, which limits my actual measurements to use of a photo-diode = attached to a CRT monitor. That means i can only verify the accuracy of = timestamping down to about 0.2 msecs variability and 0.5 msecs bias due = to the limitations/noise of the measurement setup (depending how close i = get the photo-diode to the corner of the monitor, how dark it is, etc.). = So i know that the jitter in the timestamps is very low, less than 1 = usec standard deviation iirc, and that the absolute error wrt. reality = is lower than 0.2 msecs, but i wouldn't be able to detect absolute = errors of a few scanlines. -mario >> >> > + in_vbl =3D *vpos >=3D vbl_start && *vpos <=3D vbl_end; >> >> Other than that, it looks good. >> >> Reviewed-by: mario.kleiner.de@gmail.com >> >>> >>> /* Inside "upper part" of vblank area? Apply corrective offset: */ >>> if (in_vbl && (*vpos >=3D vbl_start)) >>> *vpos =3D *vpos - vtotal; >>> >>> - /* Readouts valid? */ >>> - if (vbl > 0) >>> - ret |=3D DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE; >>> - >>> /* In vblank? */ >>> if (in_vbl) >>> ret |=3D DRM_SCANOUTPOS_INVBL; >>> @@ -3148,11 +3140,10 @@ void intel_irq_init(struct drm_device *dev) >>> dev->driver->get_vblank_counter =3D gm45_get_vblank_counter; >>> } >>> >>> - if (drm_core_check_feature(dev, DRIVER_MODESET)) >>> + if (drm_core_check_feature(dev, DRIVER_MODESET)) { >>> dev->driver->get_vblank_timestamp =3D i915_get_vblank_timestamp; >>> - else >>> - dev->driver->get_vblank_timestamp =3D NULL; >>> - dev->driver->get_scanout_position =3D i915_get_crtc_scanoutpos; >>> + dev->driver->get_scanout_position =3D i915_get_crtc_scanoutpos; >>> + } >>> >>> if (IS_VALLEYVIEW(dev)) { >>> dev->driver->irq_handler =3D valleyview_irq_handler; >>> >