Re: [PATCH 07/28] drm: Update drm_plane_funcs kerneldoc

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Dec 07, 2015 at 12:46:38PM +0100, Thierry Reding wrote:
> On Fri, Dec 04, 2015 at 09:45:48AM +0100, Daniel Vetter wrote:
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> [...]
> > +	/**
> > +	 * @destroy:
> > +	 *
> > +	 * Clean up CRTC resources. This is only called at driver unload time
> 
> Perhaps drop "only" because there are more than a single potential
> usage?

It's for driver unload only since you can't hotplug planes. The only
hotpluggable thing in drm atm are connectors, and I've added these blurbs
to highlight that.

> 
> > +	/**
> > +	 * @set_property:
> > +	 *
> > +	 * This is the legacy entry point to update a property attach to the
> 
> "attached to"
> 
> > -	/* atomic update handling */
> > +	/**
> > +	 * @atomic_duplicate_state:
> > +	 *
> > +	 * Duplicate the current atomic state for this CRTC and return it.
> > +	 * The core and helpers gurantee that any atomic state duplicated with
> > +	 * this hook and still owned by the caller (i.e. not transferred to the
> > +	 * driver by calling ->atomic_commit() from struct
> > +	 * &drm_mode_config_funcs) will be cleaned up by calling the
> > +	 * @atomic_destroy_state hook in this structure.
> > +	 *
> > +	 * Atomic drivers which don't subclass struct &drm_crtc should use
> > +	 * drm_atomic_helper_crtc_duplicate_state(). Drivers that subclass the
> > +	 * state structure to extend it with driver-private state should use
> > +	 * __drm_atomic_helper_crtc_duplicate_state() to make sure shared state is
> > +	 * duplicated in a consisten fashion across drivers.
> 
> "consistent"
> 
> > +	/**
> > +	 * @atomic_set_property:
> > +	 *
> > +	 * Decode a driver-private property value and store the decoded value
> > +	 * into the passed-in state structure. Since the atomic core decodes all
> > +	 * standardized properties (even for extensions beyond the core set of
> > +	 * properties which might not be implemented by all drivers) this
> > +	 * requires drivers to subclass the state structure.
> > +	 *
> > +	 * Such driver-private properties should really only implemented for
> 
> "be implemented"
> 
> > +	 * This function is called in the state assembly phase of atomic
> > +	 * modesets, which can be aborted for any reason (including on
> > +	 * userspace's request to just check whether a configuration would be
> > +	 * possible). Drivers MUST NOT touch any persistent state (hardware or
> > +	 * software) or data structures except the passed in adjusted_mode
> 
> Copy-paste error? Should be "... the passed in @state parameter."
> 
> > +	 *
> > +	 * Also since userspace controls in which order properties are set this
> > +	 * function must not do any input validation (since the state update is
> > +	 * incompletely and hence likely inconsistent). Instead any such input
> 
> "incomplete"
> 
> > +	 * validation must be done in the various atomic_check callbacks.
> > +	 *
> > +	 * RETURNS:
> > +	 *
> > +	 * 0 if the property has been found, -EINVAL if the property isn't
> > +	 * implemented by the driver (which shouldn't ever happen, the core only
> 
> s/shouldn't ever/should never/?
> 
> > +	 * asks for properties attached to this CRTC). No other
> 
> Seems like you could put more text on the above line.
> 
> > +	 * validation is allowed by the driver. The core checks that the
> > +	 * property value is within the range (integer, valid enum value, ...)
> > +	 * the driver set when registering the property already.
> 
> I'd put the "already" after "The core", otherwise it reads weird in my
> opinion.
> 
> > +	 */
> >  	int (*atomic_set_property)(struct drm_crtc *crtc,
> >  				   struct drm_crtc_state *state,
> >  				   struct drm_property *property,
> >  				   uint64_t val);
> > +	/**
> > +	 * @atomic_get_property:
> > +	 *
> > +	 * Reads out the decoded driver-private property. This is used to
> > +	 * implement the GETCRTC ioctl.
> > +	 *
> > +	 * Do not call this function directly, use
> > +	 * drm_atomic_crtc_get_property() instead.
> > +	 *
> > +	 * This callback is optional if the driver does not support any
> > +	 * driver-private atomic properties.
> > +	 *
> > +	 * RETURNS:
> > +	 *
> > +	 * 0 on success, -EINVAL if ther property isn't implemented by the
> 
> s/ther/the/
> 
> > +	 * driver (which shouldn't ever happen, the core only asks for
> 
> s/shouldn't ever/should never/?
> 
> > +	 * properties attached on this CRTC).
> 
> "attached to"
> 
> > @@ -539,19 +662,142 @@ struct drm_connector_funcs {
> >  	enum drm_connector_status (*detect)(struct drm_connector *connector,
> >  					    bool force);
> >  	int (*fill_modes)(struct drm_connector *connector, uint32_t max_width, uint32_t max_height);
> > +
> > +	/**
> > +	 * @set_property:
> > +	 *
> > +	 * This is the legacy entry point to update a property attach to the
> 
> "attached to"
> 
> > +	/**
> > +	 * @destroy:
> > +	 *
> > +	 * Clean up connector resources. This is only called at driver unload time
> > +	 * through drm_mode_config_cleanup(), but can also be called at runtime
> > +	 * when a connector is being hot-unplugged.
> 
> Again, perhaps drop the "only" because it's inconsistent when followed
> by "but can also".

Yeah here the only is wrong so removed it. Also reworded the 2nd part
slightly to add DP MST as an expample and make it clear that you need a
driver which can hotplug connectors for this.

> Most of the comments on the CRTC helpers do apply to the connector
> helpers as well, so I haven't repeated them.

Yeah, I think I found all 3 instances of each.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel