From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keith Packard Subject: Re: [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types Date: Thu, 06 Jul 2017 08:36:00 -0700 Message-ID: <864lupd1cv.fsf@keithp.com> References: <20170705221013.27940-1-keithp@keithp.com> <20170705221013.27940-3-keithp@keithp.com> <20170706073049.mfmjvujx4szf6qji@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1129285542==" Return-path: Received: from elaine.keithp.com (home.keithp.com [63.227.221.253]) by gabe.freedesktop.org (Postfix) with ESMTP id 78C816E62F for ; Thu, 6 Jul 2017 15:36:02 +0000 (UTC) In-Reply-To: <20170706073049.mfmjvujx4szf6qji@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: Dave Airlie , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org --===============1129285542== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Daniel Vetter writes: > A few nits below, but looks good otherwise. Thanks. >> static struct drm_pending_vblank_event *create_vblank_event( >> - struct drm_device *dev, uint64_t user_data) >> + struct drm_device *dev, struct drm_crtc *crtc, uint64_t user_data) > > Nit: Please also drop the dev argument, we have crtc->dev easily > available. That fits better into my long-term goal of getting rid of the > (dev, pipe) pairs everywhere in the vblank code and fully switching over > to drm_crtc *. As 'dev' isn't used anyways, this seems like a fine plan. >> + switch (e->event.base.type) { >> + case DRM_EVENT_VBLANK: >> + case DRM_EVENT_FLIP_COMPLETE: >> + if (seq) >> + e->event.vbl.sequence =3D (u32) seq; >> + if (now) { >> + e->event.vbl.tv_sec =3D now->tv_sec; >> + e->event.vbl.tv_usec =3D now->tv_nsec / 1000; >> + } >> + break; >> + } > > Not sure why this change? Also prep for the new, presumably extended > events? Seems at least slightly inconsistent with other paths, where we > still unconditionally fill it in. Yes, this prepares for the new events to make that patch smaller. The places where the data are still unconditionally assigned should know that the event in the struct is either a VBLANK or FLIP_COMPLETE. >> + struct drm_crtc *crtc =3D drm_crtc_from_index(dev, pipe); > > This'll oops on ums drivers since kms isn't set up. How about this fix? diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 857b7cf011e1..e39b2bd074e4 100644 =2D-- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1355,7 +1355,6 @@ static int drm_queue_vblank_event(struct drm_device *= dev, unsigned int pipe, union drm_wait_vblank *vblwait, struct drm_file *file_priv) { =2D struct drm_crtc *crtc =3D drm_crtc_from_index(dev, pipe); struct drm_vblank_crtc *vblank =3D &dev->vblank[pipe]; struct drm_pending_vblank_event *e; struct timespec now; @@ -1373,7 +1372,12 @@ static int drm_queue_vblank_event(struct drm_device = *dev, unsigned int pipe, e->event.base.type =3D DRM_EVENT_VBLANK; e->event.base.length =3D sizeof(e->event.vbl); e->event.vbl.user_data =3D vblwait->request.signal; =2D e->event.vbl.crtc_id =3D crtc ? crtc->base.id : 0; + e->event.vbl.crtc_id =3D 0; + if (drm_core_check_feature(dev, DRIVER_MODESET)) { + struct drm_crtc *crtc =3D drm_crtc_from_index(dev, pipe); + if (crtc) + e->event.vbl.crtc_id =3D crtc->base.id; + } =20 spin_lock_irqsave(&dev->event_lock, flags); > Or maybe I shouldn't have told you this and seized this opportunity to > break all the old drivers :-) You now know my evil plan :-) =2D-=20 =2Dkeith --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEw4O3eCVWE9/bQJ2R2yIaaQAAABEFAlleWOAACgkQ2yIaaQAA ABEGKA/8DLxBWi/oVXESyS1ozEtVkRqI2bYB/1HTL/f/vNM4XEBREc6xHvtU4Ciq 9yDBSabFgPOuQOmWXu21XlqGWpH/3u32AasrDhfrrxrcCBtRLcPl9pYxDrmAu90a H+vEBucUKnyFmaAi1ccWMGjElJ9SbnqoR4RLQ1MIREL9u1jIjb3YmPgyC16wS2D7 WRoicz5+KvTck0ulfMCPr3yA0qcyU8mDdkto0NzVB9dPlV25JjTHI0RvNT6QnmNz hGvu5jeozT5TtDdo9DiE4/sxP8JavTSzPsh5glFOh5/oCA9TJ8IhOhYOYiHhEknY xprtI4G0f6MKYdanDlTOA6bZUGtr6QiF/U1Dxhv+5SBaVxqmjjE8S7qpk75mYMPq NbXXVo+CFxwlVDy6pS4H3TiJuYi81O0PRqalJqwwURu/e6yJPVgHmzwsRSTgihXD tTYeptf0/vYi0vYH3RSAFn3KRDjqsF0N0pEEpw9okbmAsqAx7FPj4++ddpXm0fIN yCQhsem2V+uM5o+kV8rH4l7te9c4dA2HkW3ew6BwBhGRXk+KZ4OvYyr6zJzswFc2 9vDPE6bbtZivrfo8S9tWVZwxoeP3mZMmJKFroITMZicl9W6qDsDSwMu0rM3GPtI8 tHVTLH42sJdodjC7hJrJL2wVrAbUC+2pU2DxMYTBnwrvfyLNusE= =X5d7 -----END PGP SIGNATURE----- --=-=-=-- --===============1129285542== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1129285542==--