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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Boris,

On Thu, Aug 22, 2019 at 11:00:56AM +0200, Boris Brezillon wrote:
> On Wed, 21 Aug 2019 23:14:56 +0300 Laurent Pinchart 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.

I think consistency would help. Regarding the reset operation, unless
I'm mistaken, it got its name from the fact that it should create a
software state that reflects the current hardware state, in order to
support fastboot (taking over a running display controller started by
the boot loader without causing any glitch). That being said, I'm not
sure reset was the best name even for that :-)

> > > +
> > > +/**
> > > + * 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.

It would help with consistency, but I'll leave that to you.

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux