LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Sean Paul <sean@poorly.run>
To: Andrzej Hajda <a.hajda@samsung.com>
Cc: Sean Paul <sean@poorly.run>,
	dri-devel@lists.freedesktop.org,
	Maxime Ripard <maxime.ripard@bootlin.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	linux-kernel@vger.kernel.org, David Airlie <airlied@linux.ie>,
	Sean Paul <seanpaul@chromium.org>
Subject: Re: [PATCH v4 03/11] drm: Add atomic variants for bridge enable/disable
Date: Tue, 21 May 2019 10:31:48 -0400	[thread overview]
Message-ID: <20190521143148.GB17077@art_vandelay> (raw)
In-Reply-To: <04aef058-fd7f-d37f-de84-6d05a66341bd@samsung.com>

On Tue, May 21, 2019 at 10:58:06AM +0200, Andrzej Hajda wrote:
> On 08.05.2019 18:09, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> >
> > This patch adds atomic variants for all of
> > pre_enable/enable/disable/post_disable bridge functions. These will be
> > called from the appropriate atomic helper functions. If the bridge
> > driver doesn't implement the atomic version of the function, we will
> > fall back to the vanilla implementation.
> >
> > Note that some drivers call drm_bridge_disable directly, and these cases
> > are not covered. It's up to the driver to decide whether to implement
> > both atomic_disable and disable, or if it's not necessary.
> >
> > Changes in v3:
> > - Added to the patchset
> > Changes in v4:
> > - Fix up docbook references (Daniel)
> >
> > Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-4-sean@poorly.run
> >
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> 
> 
> In general patch looks OK, ie. it correctly mimics old behavior with
> added atomic state.
> 
> However I have mixed feelings about it, because of the whole idea that
> order of enabling/disabling bridges in the chain is the same for all
> bridge chains is just incorrect. It depends on link types between
> bridges, there are also multi-in/out bridges which does not fit at all
> in this model. Due to this lack of flexibility developers either
> explicitly avoids drm_bridge->next chaining in favor of custom
> solutions, or worse they forcibly try to fit into this model by either
> violating hw specs, either by introducing hacks into the drivers.
> 
> The simplest solution for this would be to just let parent bridge calls
> children's callbacks explicitly (as exynos_drm_dsi and vc4_dsi do).

Yeah, I completely agree that the bridge chaining is worn out. I also agree that
letting the parent call the child would alleviate some of this. However I do
worry that relying on the driver to order the calls correctly might be too much
to ask. I think the pattern for handling this type of thing is to add bridge
helper functions that allow drivers to override the default behavior when a
tricky situation arises. This would allow us to break down the bridge midlayer
and more tightly integrate it into the atomic world.

> 
> But since it is not a blocker I will left it up to you :) :

That said, agree that it's out of scope for this project. If you like the above,
I can add a TODO item to the list in a follow-up patch.


> 
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Thank you!

Sean

> 
> 
> Regards
> 
> Andrzej
> 
> 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c |   8 +-
> >  drivers/gpu/drm/drm_bridge.c        | 110 ++++++++++++++++++++++++++++
> >  include/drm/drm_bridge.h            | 106 +++++++++++++++++++++++++++
> >  3 files changed, 220 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index ccf01831f265..e8b7187a8494 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -995,7 +995,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> >  		 * Each encoder has at most one connector (since we always steal
> >  		 * it away), so we won't call disable hooks twice.
> >  		 */
> > -		drm_bridge_disable(encoder->bridge);
> > +		drm_atomic_bridge_disable(encoder->bridge, old_state);
> >  
> >  		/* Right function depends upon target state. */
> >  		if (funcs) {
> > @@ -1009,7 +1009,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> >  				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> >  		}
> >  
> > -		drm_bridge_post_disable(encoder->bridge);
> > +		drm_atomic_bridge_post_disable(encoder->bridge, old_state);
> >  	}
> >  
> >  	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> > @@ -1308,7 +1308,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> >  		 * Each encoder has at most one connector (since we always steal
> >  		 * it away), so we won't call enable hooks twice.
> >  		 */
> > -		drm_bridge_pre_enable(encoder->bridge);
> > +		drm_atomic_bridge_pre_enable(encoder->bridge, old_state);
> >  
> >  		if (funcs) {
> >  			if (funcs->atomic_enable)
> > @@ -1319,7 +1319,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> >  				funcs->commit(encoder);
> >  		}
> >  
> > -		drm_bridge_enable(encoder->bridge);
> > +		drm_atomic_bridge_enable(encoder->bridge, old_state);
> >  	}
> >  
> >  	drm_atomic_helper_commit_writebacks(dev, old_state);
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 138b2711d389..cba537c99e43 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -352,6 +352,116 @@ void drm_bridge_enable(struct drm_bridge *bridge)
> >  }
> >  EXPORT_SYMBOL(drm_bridge_enable);
> >  
> > +/**
> > + * drm_atomic_bridge_disable - disables all bridges in the encoder chain
> > + * @bridge: bridge control structure
> > + * @state: atomic state being committed
> > + *
> > + * Calls &drm_bridge_funcs.atomic_disable (falls back on
> > + * &drm_bridge_funcs.disable) op for all the bridges in the encoder chain,
> > + * starting from the last bridge to the first. These are called before calling
> > + * &drm_encoder_helper_funcs.atomic_disable
> > + *
> > + * Note: the bridge passed should be the one closest to the encoder
> > + */
> > +void drm_atomic_bridge_disable(struct drm_bridge *bridge,
> > +			       struct drm_atomic_state *state)
> > +{
> > +	if (!bridge)
> > +		return;
> > +
> > +	drm_atomic_bridge_disable(bridge->next, state);
> > +
> > +	if (bridge->funcs->atomic_disable)
> > +		bridge->funcs->atomic_disable(bridge, state);
> > +	else if (bridge->funcs->disable)
> > +		bridge->funcs->disable(bridge);
> > +}
> > +EXPORT_SYMBOL(drm_atomic_bridge_disable);
> > +
> > +/**
> > + * drm_atomic_bridge_post_disable - cleans up after disabling all bridges in the
> > + *				    encoder chain
> > + * @bridge: bridge control structure
> > + * @state: atomic state being committed
> > + *
> > + * Calls &drm_bridge_funcs.atomic_post_disable (falls back on
> > + * &drm_bridge_funcs.post_disable) op for all the bridges in the encoder chain,
> > + * starting from the first bridge to the last. These are called after completing
> > + * &drm_encoder_helper_funcs.atomic_disable
> > + *
> > + * Note: the bridge passed should be the one closest to the encoder
> > + */
> > +void drm_atomic_bridge_post_disable(struct drm_bridge *bridge,
> > +				    struct drm_atomic_state *state)
> > +{
> > +	if (!bridge)
> > +		return;
> > +
> > +	if (bridge->funcs->atomic_post_disable)
> > +		bridge->funcs->atomic_post_disable(bridge, state);
> > +	else if (bridge->funcs->post_disable)
> > +		bridge->funcs->post_disable(bridge);
> > +
> > +	drm_atomic_bridge_post_disable(bridge->next, state);
> > +}
> > +EXPORT_SYMBOL(drm_atomic_bridge_post_disable);
> > +
> > +/**
> > + * drm_atomic_bridge_pre_enable - prepares for enabling all bridges in the
> > + *				  encoder chain
> > + * @bridge: bridge control structure
> > + * @state: atomic state being committed
> > + *
> > + * Calls &drm_bridge_funcs.atomic_pre_enable (falls back on
> > + * &drm_bridge_funcs.pre_enable) op for all the bridges in the encoder chain,
> > + * starting from the last bridge to the first. These are called before calling
> > + * &drm_encoder_helper_funcs.atomic_enable
> > + *
> > + * Note: the bridge passed should be the one closest to the encoder
> > + */
> > +void drm_atomic_bridge_pre_enable(struct drm_bridge *bridge,
> > +				  struct drm_atomic_state *state)
> > +{
> > +	if (!bridge)
> > +		return;
> > +
> > +	drm_atomic_bridge_pre_enable(bridge->next, state);
> > +
> > +	if (bridge->funcs->atomic_pre_enable)
> > +		bridge->funcs->atomic_pre_enable(bridge, state);
> > +	else if (bridge->funcs->pre_enable)
> > +		bridge->funcs->pre_enable(bridge);
> > +}
> > +EXPORT_SYMBOL(drm_atomic_bridge_pre_enable);
> > +
> > +/**
> > + * drm_atomic_bridge_enable - enables all bridges in the encoder chain
> > + * @bridge: bridge control structure
> > + * @state: atomic state being committed
> > + *
> > + * Calls &drm_bridge_funcs.atomic_enable (falls back on
> > + * &drm_bridge_funcs.enable) op for all the bridges in the encoder chain,
> > + * starting from the first bridge to the last. These are called after completing
> > + * &drm_encoder_helper_funcs.atomic_enable
> > + *
> > + * Note: the bridge passed should be the one closest to the encoder
> > + */
> > +void drm_atomic_bridge_enable(struct drm_bridge *bridge,
> > +			      struct drm_atomic_state *state)
> > +{
> > +	if (!bridge)
> > +		return;
> > +
> > +	if (bridge->funcs->atomic_enable)
> > +		bridge->funcs->atomic_enable(bridge, state);
> > +	else if (bridge->funcs->enable)
> > +		bridge->funcs->enable(bridge);
> > +
> > +	drm_atomic_bridge_enable(bridge->next, state);
> > +}
> > +EXPORT_SYMBOL(drm_atomic_bridge_enable);
> > +
> >  #ifdef CONFIG_OF
> >  /**
> >   * of_drm_find_bridge - find the bridge corresponding to the device node in
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index d4428913a4e1..322801884814 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -237,6 +237,103 @@ struct drm_bridge_funcs {
> >  	 * The enable callback is optional.
> >  	 */
> >  	void (*enable)(struct drm_bridge *bridge);
> > +
> > +	/**
> > +	 * @atomic_pre_enable:
> > +	 *
> > +	 * This callback should enable the bridge. It is called right before
> > +	 * the preceding element in the display pipe is enabled. If the
> > +	 * preceding element is a bridge this means it's called before that
> > +	 * bridge's @atomic_pre_enable or @pre_enable function. If the preceding
> > +	 * element is a &drm_encoder it's called right before the encoder's
> > +	 * &drm_encoder_helper_funcs.atomic_enable hook.
> > +	 *
> > +	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
> > +	 * will not yet be running when this callback is called. The bridge must
> > +	 * not enable the display link feeding the next bridge in the chain (if
> > +	 * there is one) when this callback is called.
> > +	 *
> > +	 * Note that this function will only be invoked in the context of an
> > +	 * atomic commit. It will not be invoked from &drm_bridge_pre_enable. It
> > +	 * would be prudent to also provide an implementation of @pre_enable if
> > +	 * you are expecting driver calls into &drm_bridge_pre_enable.
> > +	 *
> > +	 * The @atomic_pre_enable callback is optional.
> > +	 */
> > +	void (*atomic_pre_enable)(struct drm_bridge *bridge,
> > +				  struct drm_atomic_state *state);
> > +
> > +	/**
> > +	 * @atomic_enable:
> > +	 *
> > +	 * This callback should enable the bridge. It is called right after
> > +	 * the preceding element in the display pipe is enabled. If the
> > +	 * preceding element is a bridge this means it's called after that
> > +	 * bridge's @atomic_enable or @enable function. If the preceding element
> > +	 * is a &drm_encoder it's called right after the encoder's
> > +	 * &drm_encoder_helper_funcs.atomic_enable hook.
> > +	 *
> > +	 * The bridge can assume that the display pipe (i.e. clocks and timing
> > +	 * signals) feeding it is running when this callback is called. This
> > +	 * callback must enable the display link feeding the next bridge in the
> > +	 * chain if there is one.
> > +	 *
> > +	 * Note that this function will only be invoked in the context of an
> > +	 * atomic commit. It will not be invoked from &drm_bridge_enable. It
> > +	 * would be prudent to also provide an implementation of @enable if
> > +	 * you are expecting driver calls into &drm_bridge_enable.
> > +	 *
> > +	 * The enable callback is optional.
> > +	 */
> > +	void (*atomic_enable)(struct drm_bridge *bridge,
> > +			      struct drm_atomic_state *state);
> > +	/**
> > +	 * @atomic_disable:
> > +	 *
> > +	 * This callback should disable the bridge. It is called right before
> > +	 * the preceding element in the display pipe is disabled. If the
> > +	 * preceding element is a bridge this means it's called before that
> > +	 * bridge's @atomic_disable or @disable vfunc. If the preceding element
> > +	 * is a &drm_encoder it's called right before the
> > +	 * &drm_encoder_helper_funcs.atomic_disable hook.
> > +	 *
> > +	 * The bridge can assume that the display pipe (i.e. clocks and timing
> > +	 * signals) feeding it is still running when this callback is called.
> > +	 *
> > +	 * Note that this function will only be invoked in the context of an
> > +	 * atomic commit. It will not be invoked from &drm_bridge_disable. It
> > +	 * would be prudent to also provide an implementation of @disable if
> > +	 * you are expecting driver calls into &drm_bridge_disable.
> > +	 *
> > +	 * The disable callback is optional.
> > +	 */
> > +	void (*atomic_disable)(struct drm_bridge *bridge,
> > +			       struct drm_atomic_state *state);
> > +
> > +	/**
> > +	 * @atomic_post_disable:
> > +	 *
> > +	 * This callback should disable the bridge. It is called right after the
> > +	 * preceding element in the display pipe is disabled. If the preceding
> > +	 * element is a bridge this means it's called after that bridge's
> > +	 * @atomic_post_disable or @post_disable function. If the preceding
> > +	 * element is a &drm_encoder it's called right after the encoder's
> > +	 * &drm_encoder_helper_funcs.atomic_disable hook.
> > +	 *
> > +	 * The bridge must assume that the display pipe (i.e. clocks and timing
> > +	 * signals) feeding it is no longer running when this callback is
> > +	 * called.
> > +	 *
> > +	 * Note that this function will only be invoked in the context of an
> > +	 * atomic commit. It will not be invoked from &drm_bridge_post_disable.
> > +	 * It would be prudent to also provide an implementation of
> > +	 * @post_disable if you are expecting driver calls into
> > +	 * &drm_bridge_post_disable.
> > +	 *
> > +	 * The post_disable callback is optional.
> > +	 */
> > +	void (*atomic_post_disable)(struct drm_bridge *bridge,
> > +				    struct drm_atomic_state *state);
> >  };
> >  
> >  /**
> > @@ -314,6 +411,15 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
> >  void drm_bridge_pre_enable(struct drm_bridge *bridge);
> >  void drm_bridge_enable(struct drm_bridge *bridge);
> >  
> > +void drm_atomic_bridge_disable(struct drm_bridge *bridge,
> > +			       struct drm_atomic_state *state);
> > +void drm_atomic_bridge_post_disable(struct drm_bridge *bridge,
> > +				    struct drm_atomic_state *state);
> > +void drm_atomic_bridge_pre_enable(struct drm_bridge *bridge,
> > +				  struct drm_atomic_state *state);
> > +void drm_atomic_bridge_enable(struct drm_bridge *bridge,
> > +			      struct drm_atomic_state *state);
> > +
> >  #ifdef CONFIG_DRM_PANEL_BRIDGE
> >  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
> >  					u32 connector_type);
> 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

  reply	other threads:[~2019-05-21 14:31 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20190508160920.144739-1-sean@poorly.run>
2019-05-08 16:09 ` [PATCH v4 01/11] drm: Add atomic variants of enable/disable to encoder helper funcs Sean Paul
2019-05-08 16:31   ` Daniel Vetter
2019-05-08 18:18     ` Sean Paul
2019-05-08 16:09 ` [PATCH v4 02/11] drm: Add drm_atomic_get_(old|new-_connector_for_encoder() helpers Sean Paul
2019-05-08 16:33   ` Daniel Vetter
2019-05-12 16:48     ` Laurent Pinchart
2019-05-08 16:09 ` [PATCH v4 03/11] drm: Add atomic variants for bridge enable/disable Sean Paul
2019-05-21  8:58   ` Andrzej Hajda
2019-05-21 14:31     ` Sean Paul [this message]
2019-05-08 16:09 ` [PATCH v4 04/11] drm: Convert connector_helper_funcs->atomic_check to accept drm_atomic_state Sean Paul
2019-05-08 16:09 ` [PATCH v4 05/11] drm: Add helpers to kick off self refresh mode in drivers Sean Paul
2019-05-08 16:35   ` Daniel Vetter
2019-05-08 16:09 ` [PATCH v4 06/11] drm/rockchip: Use dirtyfb helper Sean Paul
2019-05-08 16:09 ` [PATCH v4 07/11] drm/rockchip: Check for fast link training before enabling psr Sean Paul
2019-05-08 16:09 ` [PATCH v4 08/11] drm/rockchip: Use the helpers for PSR Sean Paul
2019-05-08 16:09 ` [PATCH v4 09/11] drm/rockchip: Use vop_win in vop_win_disable instead of vop_win_data Sean Paul
2019-05-08 16:09 ` [PATCH v4 10/11] drm/rockchip: Don't fully disable vop on self refresh Sean Paul
2019-05-08 16:09 ` [PATCH v4 11/11] drm/rockchip: Use drm_atomic_helper_commit_tail_rpm Sean Paul

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190521143148.GB17077@art_vandelay \
    --to=sean@poorly.run \
    --cc=a.hajda@samsung.com \
    --cc=airlied@linux.ie \
    --cc=daniel.vetter@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maxime.ripard@bootlin.com \
    --cc=seanpaul@chromium.org \
    --subject='Re: [PATCH v4 03/11] drm: Add atomic variants for bridge enable/disable' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).