From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751899AbdBOOGq (ORCPT ); Wed, 15 Feb 2017 09:06:46 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:43669 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751710AbdBOOGl (ORCPT ); Wed, 15 Feb 2017 09:06:41 -0500 Date: Wed, 15 Feb 2017 15:06:28 +0100 From: Maxime Ripard To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Cc: Daniel Vetter , David Airlie , Jani Nikula , Sean Paul , Stefan Christ , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC Message-ID: <20170215140628.sl6uenogokxxhrrn@lukather> References: <20170210140605.GC31595@intel.com> <20170213103518.6xck2ag7bwgdjgki@lukather> <20170213144533.GI31595@intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="6di2fybhqpzeqk2k" Content-Disposition: inline In-Reply-To: <20170213144533.GI31595@intel.com> User-Agent: Mutt/1.6.2-neo (2016-08-21) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --6di2fybhqpzeqk2k Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Mon, Feb 13, 2017 at 04:45:33PM +0200, Ville Syrj=E4l=E4 wrote: > On Mon, Feb 13, 2017 at 11:35:18AM +0100, Maxime Ripard wrote: > > Hi Ville, > >=20 > > On Fri, Feb 10, 2017 at 04:06:05PM +0200, Ville Syrj=E4l=E4 wrote: > > > On Thu, Feb 02, 2017 at 11:31:57AM +0100, Maxime Ripard wrote: > > > > From: Stefan Christ > > > >=20 > > > > Implement legacy framebuffer ioctl FBIO_WAITFORVSYNC in the generic > > > > framebuffer emulation driver. Legacy framebuffer users like non kms= /drm > > > > based OpenGL(ES)/EGL implementations may require the ioctl to > > > > synchronize drawing or buffer flip for double buffering. It is test= ed on > > > > the i.MX6. > > > >=20 > > > > Code is based on > > > > https://github.com/Xilinx/linux-xlnx/blob/master/drivers/gpu/dr= m/xilinx/xilinx_drm_fb.c#L196 > > > >=20 > > > > Signed-off-by: Stefan Christ > > > > Signed-off-by: Maxime Ripard > > > > --- > > > > drivers/gpu/drm/drm_fb_helper.c | 55 +++++++++++++++++++++++++++++= +++++- > > > > include/drm/drm_fb_helper.h | 5 ++- > > > > 2 files changed, 59 insertions(+), 1 deletion(-) > > > >=20 > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_= fb_helper.c > > > > index e934b541feea..39a3532e311c 100644 > > > > --- a/drivers/gpu/drm/drm_fb_helper.c > > > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > > > @@ -1234,6 +1234,61 @@ int drm_fb_helper_setcmap(struct fb_cmap *cm= ap, struct fb_info *info) > > > > EXPORT_SYMBOL(drm_fb_helper_setcmap); > > > > =20 > > > > /** > > > > + * drm_fb_helper_ioctl - legacy ioctl implementation > > > > + * @info: fbdev registered by the helper > > > > + * @cmd: ioctl command > > > > + * @arg: ioctl argument > > > > + * > > > > + * A helper to implement the standard fbdev ioctl. Only > > > > + * FBIO_WAITFORVSYNC is implemented for now. > > > > + */ > > > > +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, un= signed long arg) > > > > +{ > > > > + struct drm_fb_helper *fb_helper =3D info->par; > > > > + struct drm_device *dev =3D fb_helper->dev; > > > > + unsigned int i; > > > > + int ret =3D 0; > > > > + > > > > + drm_modeset_lock_all(dev); > > > > + if (!drm_fb_helper_is_bound(fb_helper)) { > > > > + drm_modeset_unlock_all(dev); > > > > + return -EBUSY; > > > > + } > > > > + > > > > + switch (cmd) { > > > > + case FBIO_WAITFORVSYNC: > > > > + for (i =3D 0; i < fb_helper->crtc_count; i++) { > > >=20 > > > FBIO_WAITFORVSYNC takes the crtc as a parmeter, so I'm not sure we wa= nt > > > to do this for all the crtcs. Though what that crtc means for fb is > > > rather poorly defined. > >=20 > > I guess I could just use that index to retrieve only the right CRTC in > > fb_helper->crtc_info. > >=20 > > >=20 > > > > + struct drm_mode_set *mode_set; > > > > + struct drm_crtc *crtc; > > > > + > > > > + mode_set =3D &fb_helper->crtc_info[i].mode_set; > > > > + crtc =3D mode_set->crtc; > > > > + > > > > + /* > > > > + * Only call drm_crtc_wait_one_vblank for crtcs that > > > > + * are currently enabled. > > > > + */ > > > > + if (!crtc->enabled) > > > > + continue; > > > > + > > > > + ret =3D drm_crtc_vblank_get(crtc); > > > > + if (!ret) { > > > > + drm_crtc_wait_one_vblank(crtc); > > > > + drm_crtc_vblank_put(crtc); > > > > + } > > >=20 > > > This looks quite sub-optimal. It should rather do something along the > > > lines of what drm_atomic_helper_wait_for_vblanks() does. > >=20 > > How is that suboptimal? >=20 > You're serializing the waits rather than doing them in parallel. >=20 > Let's look at a simple three crtc example (|=3Dvblank): >=20 > time --> > CRTC 1: | | | | > CRTC 2: | | | | > CRTC 3: | | | | >=20 > Your code waits until here: > | | | > 1 2 3 > ^ >=20 > Optimal code would wait until here: > | > ^ Ah, right. But then, this would happen only if we loop over all the CRTCs, which shouldn't happen in the v2 as you suggested. Out of curiosity, how could we fix this if we were to loop over all the framebuffers? By waiting on any vblank count to change? Thanks, Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --6di2fybhqpzeqk2k Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJYpGBfAAoJEBx+YmzsjxAgT4UP/3dtPUKseU80brHP+eNY06/b 8QtiM+/s1akjOPNbAX8wsgU/NcAAOJI3aouaf1ykZbqqsuczRVsfBXiFgiEhLQZk 4wePkXVP4GZgq8BaES805IkBdodZnbU5NqFc/T0OBXcoc1RpQLK53p92iwFqWI2E BVzOlb0Mfkon6yPw2q6MxUbdFXdHMS2te3kGQS79kFe6XFIhGdt26pIP3RvGkez/ PXtgB1rWaPOCgh0vNxs/CAUlxsrf03H663yduek8VjnVue9Vj9ZOIp6YRMbcRDCt ubaaCEl9yc8VjSaH+RDRy30aZN2uKPvpxLu3Psgba8emVOADBYXUFp4PycR3JqT1 GghJrOSVurhhtGxhdufboBVk2v5sb9jiJEy3idMOVCY50MmhCIPKMyfsXysqpzqp nRyANQuudAKwikNF4+6PFFVQFR8Di5CUtMs0xvX8YdeC5k1GX70bRkOxUhpmdGuv OiLh9n7igzIM5r8apSGmacH5ctnR83KSXHG6s94W+Xw3FsjxyQ5IbrHltM1W0SRC EuztFJCza1oydQXMHVNd7zeU7PVPIiBPjVIqlE7gOqvpzyX1oBEYCQrp6rLPscKF +tdQeRj+8ksjfY+s38cXC082R99atFx7OQx6MxkCJFdeFXusPFzeX3ryzdgyq4dk XmD2EX4GZsXYmDPg7zaY =o+KV -----END PGP SIGNATURE----- --6di2fybhqpzeqk2k--