From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752048AbdBMKfY (ORCPT ); Mon, 13 Feb 2017 05:35:24 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:49770 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751007AbdBMKfX (ORCPT ); Mon, 13 Feb 2017 05:35:23 -0500 Date: Mon, 13 Feb 2017 11:35:18 +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: <20170213103518.6xck2ag7bwgdjgki@lukather> References: <20170210140605.GC31595@intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="blhnrudixbfo5qiz" Content-Disposition: inline In-Reply-To: <20170210140605.GC31595@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 --blhnrudixbfo5qiz Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Ville, 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 tested on > > the i.MX6. > >=20 > > Code is based on > > https://github.com/Xilinx/linux-xlnx/blob/master/drivers/gpu/drm/xi= linx/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_h= elper.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 *cmap, = 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, unsign= ed 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 want > to do this for all the crtcs. Though what that crtc means for fb is > rather poorly defined. I guess I could just use that index to retrieve only the right CRTC in fb_helper->crtc_info. >=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. How is that suboptimal? As far as I can see, they are both doing the same thing (except one does for all the CRTCs, but I guess that would no longer be needed with the change you suggested above): - drm_crtc_vblank_get - Wait for the vblank count to change - drm_crtc_vblank_put Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --blhnrudixbfo5qiz Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJYoYviAAoJEBx+YmzsjxAgL8kP/iQNzRAh51WdN4CxPwMApOHa LXYGp76ALPHnd9jdcIi1TI/5YNMVm+y+TPINleSfm93JVsTj3ppMRb3hK/FziV6C zMvpwnLcHnU192UDQnE8UP9IYFtH1BezVGg1PzjNkb35pFGdFjgKRyiCntAsVCVu tVmBj8CSUKsXlA7c/v0Z51yvc+8ozwe5edzGVKv3iejd5Utlz7pb0O6fdRDTG4kQ dEZif11PMypI5EkY9A/VwJOIqtG4MliIq+W6ozd82bwI87r6MrZbu4MEm4K0qQjL gwlI5rJebPHXy/giEiGEbhYTMuURdozNxiBGFeM2IGXmjC+FQp4YToJJCLggF67r YR0rmrzSLCEhIb90W3QpLDvWor1QO8hC2MEylN3Ob/T0GiDQDGaWCQxli/eRlrF3 a0OKAz9kUVo2ukobVHhxNnCtdPhaXms++xVGk7DLJqGtO3jo+THahnq9OjAZ6gWY Eqmri6lNRNrkMzl6nMwvI1GoWaa9xZePH4/HsF9PUr2NcJ5KHCZ3PNeHWK7yJDSQ B7TGZDCOMkKuv/QTTKnDjd++AeK2CKBK5y7E3mRNCKYmYEF9Rtd3zpOJG7T/+yG5 R+WzN/6M+L5GumKgDOAo3ZMIFfBZh2j5KTOYtyUeFWSXAHlBirt2rBt2HFN/ZTE7 6WHtfL/QkM/3COWbQ84+ =9Hn3 -----END PGP SIGNATURE----- --blhnrudixbfo5qiz--