From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH 10/11] drm: Use vblank timestamps to guesstimate how many vblanks were missed Date: Wed, 30 Sep 2015 16:42:09 +0200 Message-ID: <20150930144209.GA13408@ulmo.nvidia.com> References: <1442259832-23424-1-git-send-email-ville.syrjala@linux.intel.com> <1442259832-23424-11-git-send-email-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0487465526==" Return-path: In-Reply-To: <1442259832-23424-11-git-send-email-ville.syrjala@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: ville.syrjala@linux.intel.com Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============0487465526== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="45Z9DzgjV8m4Oswq" Content-Disposition: inline --45Z9DzgjV8m4Oswq Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Sep 14, 2015 at 10:43:51PM +0300, ville.syrjala@linux.intel.com wro= te: [...] > @@ -167,7 +238,7 @@ static void drm_update_vblank_count(struct drm_device= *dev, unsigned int pipe, > if (!rc) > t_vblank =3D (struct timeval) {0, 0}; > =20 > - store_vblank(dev, pipe, diff, &t_vblank); > + store_vblank(dev, pipe, diff, &t_vblank, cur_vblank); I think clearing the t_vblank is now wrong here. This breaks weston on Tegra (and I think all other drivers that don't implement hardware VBLANK timestamps). The reason is that if ->get_vblank_timestamp() isn't implemented, rc will always be false at this point, hence resulting in a zero VBLANK timestamp being reported to userspace. In fact the comment, reproduced here because it's not in the patch context: /* * Only reinitialize corresponding vblank timestamp if high-precision query * available and didn't fail. Otherwise reinitialize delayed at next vblank * interrupt and assign 0 for now, to mark the vblanktimestamp as invalid. */ is no longer accurate because this function is now called at the VBLANK interrupt. If you want to keep invalidating timestamps for drivers that implement VBLANK timestamp queries I think the condition needs to be changed to something like: if (dev->get_vblank_timestamp && !rc) and the comment updated to reflect the truth. Alternatively this code can be removed, though I guess having the monotonic timestamp fallback would be kind of missing the point. I've tested either change and they both restore weston on Tegra. Thierry --45Z9DzgjV8m4Oswq Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJWC/S/AAoJEN0jrNd/PrOhxdoP/Rx05b0waunWM4SlNKFiSonJ 98yNSM1O7TXqpXNyN/GBkFHv27AQWuyxtow5CxdDqZ1Yn8pfmwEZOmOQivP9R+Vg 9yDEXuXFd8oUEkWaDXpWX8ttg0x9ofbiekyFSbzSlp1y45GdIRBprr3nhmzA0Cio ELOe4EfSHD/+HMo9fv750MZKskAitFdBb+CwHHwHfeuBvh4vCfXQqBh12h5IFkMI zdrbsLk08O8ZRKvpnqO42k+ANyCqzE0E71M4cjUFmEQRpGsAbwtoQpItyc2nLV45 fhdgc1B1MvFKSmghSTNK8bVNk6BBxyGkJbEl8dvg8wtB/6wJkaDgMDDPA6sJgn9m 8mXBKD3uJWRploEIKPHqLplp6VJTz7RCf+vcnfKxvRgBMkUZCKC9tTkv1QrtgTal dskDRg6YSsCzHlOC8VQ1rzFUhxtFmAx9VNbTVnX6SX4gHCQpvLSAqIjOnSXTAL3b 8AGFaTLhATQP2aGnNwv9LCvrNMUekLC7pTN5f5rptFwXFZFbzJBWJEDvGxLdeExf CzDW8sjC/bwtvYrF+bqkG902uHYkNK2JCzVyRyAP+RdknrM2yV/57qWzCJGyiH/Z jQHodOfRuTTRf84ZBYpmyU8Ik+kJ0eo7sm6ra4lsoWgMwBlehSu+HHIT6qpDOkI3 ed5b+FOZ+Z1Y67vFzpqk =h4xq -----END PGP SIGNATURE----- --45Z9DzgjV8m4Oswq-- --===============0487465526== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK --===============0487465526==--