Re: [PATCH RFC 10/19] drm/bridge: Add a drm_bridge_state object

[Boris Brezillon <boris.brezillon@collabora.com>]

On Wed, 21 Aug 2019 23:14:56 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> > +int
> > +drm_atomic_add_encoder_bridges(struct drm_atomic_state *state,
> > +			       struct drm_encoder *encoder)
> > +{
> > +	struct drm_bridge_state *bridge_state;
> > +	struct drm_bridge *bridge;
> > +
> > +	if (!encoder)
> > +		return 0;
> > +
> > +	DRM_DEBUG_ATOMIC("Adding all bridges for [encoder:%d:%s] to %p\n",
> > +			 encoder->base.id, encoder->name, state);
> > +
> > +	for (bridge = drm_bridge_chain_get_first_bridge(encoder); bridge;
> > +	     bridge = drm_bridge_chain_get_next_bridge(bridge)) {  
> 
> Would a for_each_bridge() macro make sense ? I'd implement it on top of
> list_for_each_entry() to make it simple.

I can add this helper. Note that for_each_bridge() will iterate over
the whole bridge chain, and we might need a for_each_bridge_from() at
some point if we want to iterate over a sub-chain.

> 
> > +		bridge_state = drm_atomic_get_bridge_state(state, bridge);
> > +		if (IS_ERR(bridge_state))
> > +			return PTR_ERR(bridge_state);
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_add_encoder_bridges);

[...]

> >  
> > +/**
> > + * drm_atomic_helper_init_bridge_state() - Initializes a bridge state  
> 
> s/Initializes/Initialize/
> 
> > + * @bridge this state is referring to  
> 
> Did you mean
> 
>  * @bridge: the bridge this state is referring to ?

Yes.

> 
> > + * @state: bridge state to initialize
> > + *
> > + * For now it's just a memset(0) plus a state->bridge assignment. Might
> > + * be extended in the future.
> > + */
> > +void drm_atomic_helper_init_bridge_state(struct drm_bridge *bridge,
> > +					 struct drm_bridge_state *state)
> > +{
> > +	memset(state, 0, sizeof(*state));
> > +	state->bridge = bridge;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_init_bridge_state);  
> 
> Other objects don't export a state init function helper, but state reset
> and state duplicate helpers. Is there a reason to depart from that model
> ? Same for the copy helper below.

I thought those names better reflect what the helpers do.
__drm_atomic_helper_crtc_duplicate_state() for instance, it actually
does not duplicate the state, but just copies current state info into
the new state object which has been allocated by the caller. Same for
the reset helpers, they actually do not reset anything but just
initialize the state to a default. I also try to avoid prefixing
functions with __ when I can (names can be clarified/extended to avoid
conflicts most of the time).
Will switch back to reset/dup names if you prefer.

> 
> > +
> > +/**
> > + * drm_atomic_helper_copy_bridge_state() - Copy the content of a bridge state
> > + * @bridge: bridge the old and new state are referring to
> > + * @old: previous bridge state to copy from
> > + * @new: new bridge state to copy to
> > + *
> > + * Should be used by custom &drm_bridge_funcs.atomic_duplicate() implementation
> > + * to copy the previous state into the new object.
> > + */
> > +void drm_atomic_helper_copy_bridge_state(struct drm_bridge *bridge,
> > +					 const struct drm_bridge_state *old,
> > +					 struct drm_bridge_state *new)
> > +{
> > +	*new = *old;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_copy_bridge_state);
> > +
> > +/**
> > + * drm_atomic_helper_duplicate_bridge_state() - Default duplicate state helper
> > + * @bridge: bridge containing the state to duplicate
> > + *
> > + * Default implementation of &drm_bridge_funcs.atomic_duplicate().
> > + *
> > + * RETURNS:
> > + * a valid state object or NULL if the allocation fails.
> > + */
> > +struct drm_bridge_state *
> > +drm_atomic_helper_duplicate_bridge_state(struct drm_bridge *bridge)
> > +{
> > +	struct drm_bridge_state *old = NULL, *new;
> > +
> > +	new = kzalloc(sizeof(*new), GFP_KERNEL);
> > +	if (!new)
> > +		return NULL;
> > +
> > +	if (bridge->base.state) {
> > +		old = drm_priv_to_bridge_state(bridge->base.state);
> > +		drm_atomic_helper_copy_bridge_state(bridge, old, new);
> > +	} else {
> > +		drm_atomic_helper_init_bridge_state(bridge, new);
> > +	}
> > +
> > +	return new;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_duplicate_bridge_state);
> > +
> > +/**
> > + * drm_atomic_helper_destroy_bridge_state() - Default destroy state helper
> > + * @bridge: the bridge this state refers to
> > + * @state: state object to destroy
> > + *
> > + * Just a simple kfree() for now.
> > + */
> > +void drm_atomic_helper_destroy_bridge_state(struct drm_bridge *bridge,
> > +					    struct drm_bridge_state *state)
> > +{
> > +	kfree(state);
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_destroy_bridge_state);
> > +
> >  #ifdef CONFIG_OF
> >  /**
> >   * of_drm_find_bridge - find the bridge corresponding to the device node in
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index 927e1205d7aa..5ee25fd51e80 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -660,6 +660,9 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state,
> >  	return plane->state;
> >  }
> >  
> > +int __must_check
> > +drm_atomic_add_encoder_bridges(struct drm_atomic_state *state,
> > +                               struct drm_encoder *encoder);
> >  int __must_check
> >  drm_atomic_add_affected_connectors(struct drm_atomic_state *state,
> >  				   struct drm_crtc *crtc);
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index f40181274ed7..8ca25d266d0c 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -25,6 +25,7 @@
> >  
> >  #include <linux/list.h>
> >  #include <linux/ctype.h>
> > +#include <drm/drm_atomic.h>
> >  #include <drm/drm_mode_object.h>
> >  #include <drm/drm_modes.h>
> >  
> > @@ -32,6 +33,23 @@ struct drm_bridge;
> >  struct drm_bridge_timings;
> >  struct drm_panel;
> >  
> > +/**
> > + * struct drm_bridge_state - Atomic bridge state object
> > + * @base: inherit from &drm_private_state
> > + * @bridge: the bridge this state refers to
> > + */
> > +struct drm_bridge_state {
> > +	struct drm_private_state base;
> > +
> > +	struct drm_bridge *bridge;
> > +};
> > +
> > +static inline struct drm_bridge_state *
> > +drm_priv_to_bridge_state(struct drm_private_state *priv)
> > +{
> > +	return container_of(priv, struct drm_bridge_state, base);
> > +}
> > +
> >  /**
> >   * struct drm_bridge_funcs - drm_bridge control functions
> >   */
> > @@ -337,6 +355,30 @@ struct drm_bridge_funcs {
> >  	 */
> >  	void (*atomic_post_disable)(struct drm_bridge *bridge,
> >  				    struct drm_atomic_state *state);
> > +
> > +	/**
> > +	 * @atomic_duplicate_state:
> > +	 *
> > +	 * Duplicate the current bridge state object.
> > +	 *
> > +	 * Note that this function can be called when current bridge state is
> > +	 * NULL. In this case, implementations should either implement HW
> > +	 * readback or initialize the state with sensible default values.  
> 
> Related to the question above, shouldn't we have a reset state operation
> instead ?

I had a version with a reset hook and a helper that was falling back to
->duplicate_state() when not specified, but I decided to drop it to
simplify things (I think most drivers will just use the default
helper, and those that actually want to implement HW readback can
easily do it from their ->duplicate_state() hook by testing
bridge->state value).
I can re-introduce it if you like.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel