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
