Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754070AbcCSKC3 (ORCPT ); Sat, 19 Mar 2016 06:02:29 -0400 Received: from mail-wm0-f52.google.com ([74.125.82.52]:38810 "EHLO mail-wm0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752186AbcCSKCQ (ORCPT ); Sat, 19 Mar 2016 06:02:16 -0400 Date: Sat, 19 Mar 2016 11:02:58 +0100 From: Daniel Vetter To: Alexey Brodkin Cc: "daniel@ffwll.ch" , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "airlied@linux.ie" Subject: Re: [PATCH 1/3] drm: introduce drm_connector_plug_all() helper Message-ID: <20160319100258.GP14170@phenom.ffwll.local> Mail-Followup-To: Alexey Brodkin , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "airlied@linux.ie" References: <1458295304-5528-1-git-send-email-abrodkin@synopsys.com> <1458295304-5528-2-git-send-email-abrodkin@synopsys.com> <20160318180609.GM14170@phenom.ffwll.local> <1458338329.2994.8.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: <1458338329.2994.8.camel@synopsys.com> X-Operating-System: Linux phenom 4.3.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: 5326 Lines: 150 On Fri, Mar 18, 2016 at 09:58:49PM +0000, Alexey Brodkin wrote: > Hi Daniel, > > On Fri, 2016-03-18 at 19:06 +0100, Daniel Vetter wrote: > > On Fri, Mar 18, 2016 at 01:01:42PM +0300, Alexey Brodkin wrote: > > > > > > As a pair to already existing drm_connector_unplug_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 generic one. > > > > > > Signed-off-by: Alexey Brodkin > > > Cc: Daniel Vetter > > > Cc: David Airlie > > > --- > > > ?drivers/gpu/drm/drm_crtc.c | 44 +++++++++++++++++++++++++++++++++++++++++++- > > > ?drivers/gpu/drm/drm_drv.c??|??3 ++- > > > ?include/drm/drm_crtc.h?????|??3 ++- > > > ?3 files changed, 47 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > > index 65258ac..ce27420 100644 > > > --- a/drivers/gpu/drm/drm_crtc.c > > > +++ b/drivers/gpu/drm/drm_crtc.c > > > @@ -1080,6 +1080,46 @@ void drm_connector_unregister(struct drm_connector *connector) > > > ?} > > > ?EXPORT_SYMBOL(drm_connector_unregister); > > > ? > > > +/** > > > + * drm_connector_plug_all - register connector userspace interfaces > > > + * @dev: drm device > > > + * > > > + * This function registers all connector userspace interfaces in sysfs. Should > > > + * be call when the device is disconnected, e.g. from an usb driver's > > Still talks about disconnect ;-) Please also mention that this just calls > > drm_connector_register() exactly like this including () to generate a > > kerneldoc hyperlink. > > Well I intentionally left in description of drm_connector_register_all(): > "Should be call when the device is disconnected, e.g. from an usb driver's > ?->connect callback." You use "disconnected" for connecting stuff. That doesn't make sense to me at all - register_all is for when you want to publish something, not for unpublishing when the device disappears. Or maybe this is a case of lost in translation, and you mean something else? -Daniel > I did mean it. Or is this statement is incorrect and example of invocation of > drm_connector_register_all() should be different? Which one works better then? > > > > > > > + * ->connect callback. > > Returns: section is missing, specifying how this can fail. Just copy the > > one from connector_register(). > > Yeah, correct. Blind copy-paste doesn't work equally good always :( > > > > > > > + */ > > > +int drm_connector_plug_all(struct drm_device *dev) > > > +{ > > > + struct drm_connector *connector, *failed; > > > + int ret; > > > + > > > + mutex_lock(&dev->mode_config.mutex); > > > + > > > + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > > for_each_connector here please. And the s/plug/register/ naming discussion > > we've had. > > Ok. > > > > > > > + ret = drm_connector_register(connector); > > > + if (ret) { > > > + failed = connector; > > > + goto err; > > > + } > > > + } > > > + > > > + mutex_unlock(&dev->mode_config.mutex); > > > + > > > + return 0; > > > + > > > +err: > > > + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > > > + if (failed == connector) > > > + break; > > > + > > > + drm_connector_unregister(connector); > > > + } > > > + > > > + mutex_unlock(&dev->mode_config.mutex); > > > + > > > + return ret; > > > +} > > > +EXPORT_SYMBOL(drm_connector_plug_all); > > > ? > > > ?/** > > > ? * drm_connector_unplug_all - unregister connector userspace interfaces > > > @@ -1093,10 +1133,12 @@ void drm_connector_unplug_all(struct drm_device *dev) > > > ?{ > > > ? struct drm_connector *connector; > > > ? > > > - /* FIXME: taking the mode config mutex ends up in a clash with sysfs */ > > > + mutex_lock(&dev->mode_config.mutex); > > You can't drop that FIXME, the bug is still there. > > That's clear given your explanation in the previous email. > > > > > > > + > > > ? list_for_each_entry(connector, &dev->mode_config.connector_list, head) > > > ? drm_connector_unregister(connector); > > > ? > > > + mutex_unlock(&dev->mode_config.mutex); > > > ?} > > > ?EXPORT_SYMBOL(drm_connector_unplug_all); > > > ? > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > > index 167c8d3..4a559c6 100644 > > > --- a/drivers/gpu/drm/drm_drv.c > > > +++ b/drivers/gpu/drm/drm_drv.c > > > @@ -715,7 +715,8 @@ EXPORT_SYMBOL(drm_dev_unref); > > > ? * > > > ? * Register the DRM device @dev with the system, advertise device to user-space > > > ? * and start normal device operation. @dev must be allocated via drm_dev_alloc() > > > - * previously. > > > + * previously and right after drm_dev_register() driver should call > > It'd do 2 sentences here for simplicity, not connect them with and. Also > > "... _the_ driver should ..." > > Ok. > > > > > > > + * drm_connector_plug_all() to register all connectors in sysfs. > > Maybe mention why this is separate: "This is a separate call for backwards > > compatibility with drivers still using the deprecated ->load() callback, > > where connectors are registered from within the ->load() callback." > > Ok. > > -Alexey -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch