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

[Alexey Brodkin <Alexey.Brodkin@synopsys.com>]

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@synopsys.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: David Airlie <airlied@linux.ie>
> > ---
> >  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."

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