Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753063AbcC2Mwt (ORCPT ); Tue, 29 Mar 2016 08:52:49 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:36690 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752322AbcC2Mwr (ORCPT ); Tue, 29 Mar 2016 08:52:47 -0400 Date: Tue, 29 Mar 2016 14:52:49 +0200 From: Daniel Vetter To: Alexey Brodkin Cc: "daniel@ffwll.ch" , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "dh.herrmann@gmail.com" , "airlied@linux.ie" Subject: Re: [PATCH 2/4 v3] drm: Introduce drm_connector_register_all() helper Message-ID: <20160329125249.GX2510@phenom.ffwll.local> Mail-Followup-To: Alexey Brodkin , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "dh.herrmann@gmail.com" , "airlied@linux.ie" References: <1458722577-20283-1-git-send-email-abrodkin@synopsys.com> <1458722577-20283-3-git-send-email-abrodkin@synopsys.com> <1458740464.3213.9.camel@synopsys.com> <20160329081935.GK2510@phenom.ffwll.local> <1459242758.3021.1.camel@synopsys.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1459242758.3021.1.camel@synopsys.com> X-Operating-System: Linux phenom 4.4.0-1-amd64 User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4940 Lines: 116 On Tue, Mar 29, 2016 at 09:12:38AM +0000, Alexey Brodkin wrote: > Hi Daniel, > > On Tue, 2016-03-29 at 10:19 +0200, Daniel Vetter wrote: > > On Wed, Mar 23, 2016 at 01:41:05PM +0000, Alexey Brodkin wrote: > > > > > > Hi David, > > > > > > On Wed, 2016-03-23 at 12:13 +0100, David Herrmann wrote: > > > > > > > > Hi > > > > > > > > On Wed, Mar 23, 2016 at 9:42 AM, Alexey Brodkin > > > > wrote: > > > > > > > > > > > > > > > As a pair to already existing drm_connector_unregister_all() we're adding > > > > > generic implementation of what is already done in some drivers. > > > > > > > > > > Once this helper is implemented we'll be ready to switch existing > > > > > driver-specific implementations with the generic one. > > > > > > > > > > Signed-off-by: Alexey Brodkin > > > > > Cc: Daniel Vetter > > > > > Cc: David Airlie > > > > > --- > > > > > > > > > > Changes v2 -> v3: > > > > > ?* Updated title with capital after colon > > > > > ?* Simplified failure path with direct and unconditional invocation of > > > > > ???unregister_all() > > > > > ?* Updated kerneldoc description of the drm_connector_register_all() > > > > > > > > > > Changes v1 -> v2: > > > > > ?* Rename drm_connector_unplug_all() to drm_connector_unregister_all() > > > > > ?* Use drm_for_each_connector() instead of list_for_each_entry() > > > > > ?* Updated kerneldoc for drm_dev_register() > > > > > > > > > > ?drivers/gpu/drm/drm_crtc.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > > > > > ?drivers/gpu/drm/drm_drv.c??|??6 +++++- > > > > > ?include/drm/drm_crtc.h?????|??3 ++- > > > > > ?3 files changed, 50 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > > > > index 65488a6..21eea11 100644 > > > > > --- a/drivers/gpu/drm/drm_crtc.c > > > > > +++ b/drivers/gpu/drm/drm_crtc.c > > > > > @@ -1081,6 +1081,49 @@ void drm_connector_unregister(struct drm_connector *connector) > > > > > ?EXPORT_SYMBOL(drm_connector_unregister); > > > > > > > > > > ?/** > > > > > + * drm_connector_register_all - register all connectors > > > > > + * @dev: drm device > > > > > + * > > > > > + * This function registers all connectors in sysfs and other places so that > > > > > + * userspace can start to access them. Drivers can call it after calling > > > > > + * drm_dev_register() to complete the device registration, if they don't call > > > > > + * drm_connector_register() on each connector individually. > > > > > + * > > > > > + * When a device is unplugged and should be removed from userspace access, > > > > > + * call drm_connector_unregister_all(), which is the inverse of this > > > > > + * function. > > > > > + * > > > > > + * Returns: > > > > > + * Zero on success, error code on failure. > > > > > + */ > > > > > +int drm_connector_register_all(struct drm_device *dev) > > > > > +{ > > > > > +???????struct drm_connector *connector; > > > > > +???????int ret; > > > > > + > > > > > +???????mutex_lock(&dev->mode_config.mutex); > > > > > + > > > > > +???????drm_for_each_connector(connector, dev) { > > > > > +???????????????ret = drm_connector_register(connector); > > > > > +???????????????if (ret) { > > > > > +???????????????????????/* > > > > > +????????????????????????* We may safely call unregister_all() here within > > > > > +????????????????????????* area locked with mutex because unregister_all() > > > > > +????????????????????????* doesn't use locks inside (see a comment in that > > > > > +????????????????????????* function). > > > > > +????????????????????????*/ > > > > Ugh? unregister_all() says: > > > > > > > > /* FIXME: taking the mode config mutex ends up in a clash with sysfs */ > > > > > > > > This strongly contradicts your comment. Anyway, regardless how you > > > > want to fix it: You better unlock the mode-config mutex before > > > > returning below. > > > So good catch. > > > But what I really meant since we didn't get any further after registering > > > all "good" connections (see we're not releasing mutex still) the will be > > > no clashes with sysfs. > > > > > > Still I;d like Daniel to comment on that separately. > > I think doing the unregister_all outside of the loop&locked section is > > better for future-proofing. My long-term plan for connector lifetimes and > > the connector list is: > > - refcounting for connectors (Dave has wip patches already). > > - separate lock for the connector list (and only that). > > > > Doing it entirely separate would make things easier and more robust. > > Ok makes sense. > > > I merged patch 1 meanwhile to drm-misc. > > So may I re-send only patches 2-4 then (using "drm-misc" as a base)? Sure. drm-misc is also in linux-next, in case you need other bits to be able to test all your patches. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch