From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v2 1/3] drm/i915: Skip register reads in i915_get_crtc_scanoutpos() Date: Wed, 25 Sep 2013 11:14:05 +0300 Message-ID: <20130925081405.GK4531@intel.com> References: <20130923103050.GE8482@nuc-i3427.alporthouse.com> <1379936930-26044-1-git-send-email-ville.syrjala@linux.intel.com> <52424C0C.6000609@tuebingen.mpg.de> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id BBA5EE5F44 for ; Wed, 25 Sep 2013 01:14:08 -0700 (PDT) Content-Disposition: inline In-Reply-To: <52424C0C.6000609@tuebingen.mpg.de> 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 To: Mario Kleiner Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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. > = > > + 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; > > -- = Ville Syrj=E4l=E4 Intel OTC