Re: [PATCH 1/3] drm: introduce drm_connector_plug_all() helper

[Daniel Vetter]

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 <abrodkin@xxxxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > > Cc: David Airlie <airlied@xxxxxxxx>
> > > ---
> > >  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