From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754118AbdBITGR (ORCPT ); Thu, 9 Feb 2017 14:06:17 -0500 Received: from mail-wm0-f65.google.com ([74.125.82.65]:36204 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752301AbdBITGM (ORCPT ); Thu, 9 Feb 2017 14:06:12 -0500 Date: Thu, 9 Feb 2017 20:06:08 +0100 From: Daniel Vetter To: Daniel Stone Cc: Maxime Ripard , Daniel Vetter , David Airlie , Jani Nikula , Sean Paul , Stefan Christ , Linux Kernel Mailing List , dri-devel Subject: Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC Message-ID: <20170209190608.lrogobvi4bhg2blp@phenom.ffwll.local> Mail-Followup-To: Daniel Stone , Maxime Ripard , Daniel Vetter , David Airlie , Jani Nikula , Sean Paul , Stefan Christ , Linux Kernel Mailing List , dri-devel References: <20170209170111.mmp7bpjy6qfl5kow@phenom.ffwll.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux phenom 4.8.0-1-amd64 User-Agent: NeoMutt/20161126 (1.7.1) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Feb 09, 2017 at 05:38:26PM +0000, Daniel Stone wrote: > Hi, > > On 9 February 2017 at 17:01, Daniel Vetter wrote: > > On Thu, Feb 02, 2017 at 11:31:57AM +0100, Maxime Ripard wrote: > >> +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg) > >> +{ > >> + struct drm_fb_helper *fb_helper = info->par; > >> + struct drm_device *dev = fb_helper->dev; > >> + unsigned int i; > >> + int ret = 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 = 0; i < fb_helper->crtc_count; i++) { > >> + struct drm_mode_set *mode_set; > >> + struct drm_crtc *crtc; > >> + > >> + mode_set = &fb_helper->crtc_info[i].mode_set; > >> + crtc = mode_set->crtc; > >> + > >> + /* > >> + * Only call drm_crtc_wait_one_vblank for crtcs that > >> + * are currently enabled. > >> + */ > >> + if (!crtc->enabled) > >> + continue; > > > > This needs locking > > More locking than the drm_modeset_lock_all() above the switch? Ouch. :) Oh, I didn't spot that one. Well, that one should be removed imo, we dont want any vblank wait path to hold these locks, it looks bad. Otoh we still need to grab the dev->mode_config.mutex, because that's also the fbdev lock. There's a plan somewhere to give the fbdev helpers their own lock, so switching to just mode_config.mutex + the changes I suggestd would be good. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch