From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keith Packard Subject: Re: [PATCH 10/12] Add glamor back into the driver Date: Wed, 30 Jul 2014 23:23:05 -0700 Message-ID: <861tt2dp2u.fsf@hiro.keithp.com> References: <1406243908-1123-1-git-send-email-keithp@keithp.com> <1406243908-1123-11-git-send-email-keithp@keithp.com> <8761iedzxh.fsf@eliezer.anholt.net> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1942887431==" Return-path: In-Reply-To: <8761iedzxh.fsf-omZaPlIz5HhaEpDpdNBo/KxOck334EZe@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: xorg-devel-bounces-go0+a7rfsptAfugRpC6u6w@public.gmane.org Sender: "xorg-devel" To: Eric Anholt , xorg-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org List-Id: intel-gfx@lists.freedesktop.org --===============1942887431== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Eric Anholt writes: > Keith Packard writes: > >> This adds glamor support back into the driver, but instad of going >> through UXA, this uses it directly instead. > > This is hard to read with the conditionalizing all of the UXA code in > the same commit as adding the glamor code. Then there are a bunch of > unrelated whitespace changes, or flattening of a bunch of nested > conditionals. I'm not sure how to make the patch easier to review; I guess I could make UXA conditional first? That would be 'crazy' in that the driver would fail to ever work if you didn't ask for UXA, but might make the patch easier to read? > The only substantive problem I see is intel_glamor_set_pixmap_bo(). > The extra enum and temporary variable introduced here seems pretty > pointless (even if that pattern had happened before). Agreed. The problem is that 'DEFAULT_ACCEL_METHOD' is defined as 'GLAMOR', 'UXA' or 'NONE' by configure.ac. This seemed like the cleanest solution in some ways. I also liked having the accel_type enum *not* define acceleration types which weren't compiled into the driver; that caught a few missing #ifdefs > I don't think this will work -- glamor_egl uses a different fd from > intel->bufmgr, so you're attaching some unrelated BO, if you're even > that lucky. This API uses the same FD as the intel bufmgr. From=20intel_glamor.c: if (!glamor_egl_init(scrn, intel->drmSubFD)) { From=20glamor_egl.c: Bool glamor_egl_init(ScrnInfoPtr scrn, int fd) ... glamor_egl->fd =3D fd; ...=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 glamor_egl->has_gem =3D glamor_egl_check_has_gem(fd); ... Bool glamor_egl_create_textured_pixmap(PixmapPtr pixmap, int handle, int stride) ... if (glamor_egl->has_gem) { if (!glamor_get_flink_name(glamor_egl->fd, handle, &name)) { So, you pass in a GEM handle for the intel driver bufmgr and glamor does the flink to get a global name. We should be using an FD passing mechanism here instead, to avoid creating more global names... > No need to check for initiailization -- double-calling it is safe (as > long as the size is consistent). Thanks. Will fix. >> void >> @@ -258,18 +240,52 @@ intel_glamor_flush(intel_screen_private * intel) >> ScreenPtr screen; >>=20=20 >> screen =3D xf86ScrnToScreen(intel->scrn); >> - if (intel->uxa_flags & UXA_USE_GLAMOR) >> - glamor_block_handler(screen); >> + glamor_block_handler(screen); >> } > > glamor_block_handler() is pretty awfully named. We should do something > about that. Suggestions welcome :-) Thanks for the careful review. =2D-=20 keith.packard-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org --=-=-= Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIVAwUBU9ngytsiGmkAAAARAQg+qQ//Y7qcFdjJTMq+0slCBFVULrs4ehrq7gt1 P5cWUF/VqZMROX6vtUozjKsETgCmxWy1CvlH5p9nUZCKz2d1L0VyMrhNL971+5ku 3X/lWxdPdoN0IWzcIm8v8qXi9/89l2RcX75jHIrMkMEaPYRsFztgkFWVBgrxBdHR FW2r4U2DysjOqjr8h4lSV/q08cSr5C/Es/PDhPRyndb7iHyGZ4QGKD28kr4tcMyh qpwLdvZjNcRlijnlCKclz854rWK6drC0rtdQz1h8wkQT4LVp4ICa2tmZUIiIYsV1 OMLKdOfQe7+Fp2duPHOgQ2dhjtAiNI/wwVb1d3rSe2cN+xFCaroprijV+Gq4ZUKZ N09jXMjf5sZE1Mea2x59bdo2tnp5xjSz7AVuIg4sNii5MX0p/smbRgVq+oG2SafP +F/VGKfjVzWfDyGljKBr6monWnIHHF7EqwVvtcf/WzKZDGLSAm26alWGKCJhtn+g gqAyXG2VH87vIn8SKamDEXbJhvoAjZFxcEM9OXSu7nGFLVd3wJ1IiyF52C/ORb7n h5TSGZhiZIVz2djwcwSaWdQ2cGwROTZJvhG5NaOs6LVe22lKLizi6x4OGfL1UbUz iTgKouQBAqJl2Mpb9DzmX++/sZm+V09dzsjzPsCfvnuK/3ezdT/8A9LQUgw+c6c7 ZlbMDA7IycQ= =wiYU -----END PGP SIGNATURE----- --=-=-=-- --===============1942887431== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel --===============1942887431==--