diff mbox series

[v5,01/11] drm: Add atomic variants of enable/disable to encoder helper funcs

Message ID 20190611160844.257498-2-sean@poorly.run (mailing list archive)
State New, archived
Headers show
Series drm: Add self refresh helpers | expand

Commit Message

Sean Paul June 11, 2019, 4:08 p.m. UTC 
From: Sean Paul <seanpaul@chromium.org>

This patch adds atomic_enable and atomic_disable callbacks to the
encoder helpers. This will allow encoders to make informed decisions in
their start-up/shutdown based on the committed state.

Aside from the new hooks, this patch also introduces the new signature
for .atomic_* functions going forward. Instead of passing object state
(well, encoders don't have atomic state, but let's ignore that), we pass
the entire atomic state so the driver can inspect more than what's
happening locally.

This is particularly important for the upcoming self refresh helpers.

Changes in v3:
- Added patch to the set
Changes in v4:
- Move atomic_disable above prepare (Daniel)
- Add breadcrumb to .enable() docbook (Daniel)
Changes in v5:
- None

Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-2-sean@poorly.run
Link to v4: https://patchwork.freedesktop.org/patch/msgid/20190508160920.144739-2-sean@poorly.run

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/drm_atomic_helper.c      |  8 +++-
 include/drm/drm_modeset_helper_vtables.h | 48 ++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 2 deletions(-)

Comments

Sam Ravnborg June 11, 2019, 6:53 p.m. UTC | #1 
Hi Sean.

Nits below.

>  
> +	/**
> +	 * @atomic_disable:
> +	 *
...
> +	 *
> +	 * This callback is a variant of @disable that provides the atomic state
> +	 * to the driver. It takes priority over @disable during atomic commits.
> +	 *
> +	 * This hook is used only by atomic helpers. Atomic drivers don't need
> +	 * to implement it if there's no need to disable anything at the encoder
> +	 * level. To ensure that runtime PM handling (using either DPMS or the
> +	 * new "ACTIVE" property) works @atomic_disable must be the inverse of
> +	 * @atomic_enable.
> +	 */
> +	void (*atomic_disable)(struct drm_encoder *encoder,
> +			       struct drm_atomic_state *state);



> +
> +	/**
> +	 * @atomic_enable:
> +	 *
...
> +	 *
> +	 * This callback is a variant of @enable that provides the atomic state
> +	 * to the driver. It is called in place of @enable during atomic
> +	 * commits.

The wording in this paragrap is not the same as the similar paragraph
above.

One says "it takes priority over"
Another says "called in place of"

Maybe be a bit more explicit and say that "if atomic_{dis,en}able is
define then {dis,en}able is not called?


> +	 *
> +	 * This hook is used only by atomic helpers, for symmetry with @disable.
I do not get the "for symmetry with @disable."?

> +	 * Atomic drivers don't need to implement it if there's no need to
> +	 * enable anything at the encoder level. To ensure that runtime PM
> +	 * handling (using either DPMS or the new "ACTIVE" property) works
> +	 * @enable must be the inverse of @disable for atomic drivers.
Did you want to say "@atomic_enable must be the inverse of @atomic_disable for atomic drivers."?

> +	 */
> +	void (*atomic_enable)(struct drm_encoder *encoder,
> +			      struct drm_atomic_state *state);
> +
>  	/**
>  	 * @disable:
>  	 *
> @@ -695,6 +740,9 @@ struct drm_encoder_helper_funcs {
>  	 * handling (using either DPMS or the new "ACTIVE" property) works
>  	 * @disable must be the inverse of @enable for atomic drivers.
>  	 *
> +	 * For atomic drivers also consider @atomic_disable and save yourself
> +	 * from having to read the NOTE below!
Maybe, if this is so, say that atomic drivers shall alwyas use the
atomic_* variants?
And then add a TODO entry to make it so for the other atomic drivers?
> +	 *
>  	 * NOTE:
>  	 *
>  	 * With legacy CRTC helpers there's a big semantic difference between


	Sam
Sean Paul June 11, 2019, 8:22 p.m. UTC | #2 
On Tue, Jun 11, 2019 at 08:53:52PM +0200, Sam Ravnborg wrote:
> Hi Sean.
> 
> Nits below.
> 
> >  
> > +	/**
> > +	 * @atomic_disable:
> > +	 *
> ...
> > +	 *
> > +	 * This callback is a variant of @disable that provides the atomic state
> > +	 * to the driver. It takes priority over @disable during atomic commits.
> > +	 *
> > +	 * This hook is used only by atomic helpers. Atomic drivers don't need
> > +	 * to implement it if there's no need to disable anything at the encoder
> > +	 * level. To ensure that runtime PM handling (using either DPMS or the
> > +	 * new "ACTIVE" property) works @atomic_disable must be the inverse of
> > +	 * @atomic_enable.
> > +	 */
> > +	void (*atomic_disable)(struct drm_encoder *encoder,
> > +			       struct drm_atomic_state *state);
> 
> 
> 
> > +
> > +	/**
> > +	 * @atomic_enable:
> > +	 *
> ...
> > +	 *
> > +	 * This callback is a variant of @enable that provides the atomic state
> > +	 * to the driver. It is called in place of @enable during atomic
> > +	 * commits.
> 
> The wording in this paragrap is not the same as the similar paragraph
> above.
> 
> One says "it takes priority over"
> Another says "called in place of"
> 
> Maybe be a bit more explicit and say that "if atomic_{dis,en}able is
> define then {dis,en}able is not called?

Sure, can make that change

> 
> 
> > +	 *
> > +	 * This hook is used only by atomic helpers, for symmetry with @disable.
> I do not get the "for symmetry with @disable."?

Copypasta from @enable kerneldoc. I'll change this up and submit a new patch to
fix up @enable to match.

> 
> > +	 * Atomic drivers don't need to implement it if there's no need to
> > +	 * enable anything at the encoder level. To ensure that runtime PM
> > +	 * handling (using either DPMS or the new "ACTIVE" property) works
> > +	 * @enable must be the inverse of @disable for atomic drivers.
> Did you want to say "@atomic_enable must be the inverse of @atomic_disable for atomic drivers."?

Yes I did. I'll also clean up some of the surrounding text in the next version.

> 
> > +	 */
> > +	void (*atomic_enable)(struct drm_encoder *encoder,
> > +			      struct drm_atomic_state *state);
> > +
> >  	/**
> >  	 * @disable:
> >  	 *
> > @@ -695,6 +740,9 @@ struct drm_encoder_helper_funcs {
> >  	 * handling (using either DPMS or the new "ACTIVE" property) works
> >  	 * @disable must be the inverse of @enable for atomic drivers.
> >  	 *
> > +	 * For atomic drivers also consider @atomic_disable and save yourself
> > +	 * from having to read the NOTE below!
> Maybe, if this is so, say that atomic drivers shall alwyas use the
> atomic_* variants?
> And then add a TODO entry to make it so for the other atomic drivers?

I don't think that's quite what we want. For atomic drivers it might be
preferable to use enable and disable if they don't need to inspect state. 

The NOTE only applies to drivers using the legacy helpers, which isn't really
applicable for anyone making the decision between disable and atomic_disable.

Sean

> > +	 *
> >  	 * NOTE:
> >  	 *
> >  	 * With legacy CRTC helpers there's a big semantic difference between
> 
> 
> 	Sam
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 0fc63d6822451..62e29b5ebb6e3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1002,7 +1002,9 @@  disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 		/* Right function depends upon target state. */
 		if (funcs) {
-			if (new_conn_state->crtc && funcs->prepare)
+			if (funcs->atomic_disable)
+				funcs->atomic_disable(encoder, old_state);
+			else if (new_conn_state->crtc && funcs->prepare)
 				funcs->prepare(encoder);
 			else if (funcs->disable)
 				funcs->disable(encoder);
@@ -1311,7 +1313,9 @@  void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 		drm_bridge_pre_enable(encoder->bridge);
 
 		if (funcs) {
-			if (funcs->enable)
+			if (funcs->atomic_enable)
+				funcs->atomic_enable(encoder, old_state);
+			else if (funcs->enable)
 				funcs->enable(encoder);
 			else if (funcs->commit)
 				funcs->commit(encoder);
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index f9c94c2a13646..0701b3a3108e5 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -679,6 +679,51 @@  struct drm_encoder_helper_funcs {
 	enum drm_connector_status (*detect)(struct drm_encoder *encoder,
 					    struct drm_connector *connector);
 
+	/**
+	 * @atomic_disable:
+	 *
+	 * This callback should be used to disable the encoder. With the atomic
+	 * drivers it is called before this encoder's CRTC has been shut off
+	 * using their own &drm_crtc_helper_funcs.atomic_disable hook. If that
+	 * sequence is too simple drivers can just add their own driver private
+	 * encoder hooks and call them from CRTC's callback by looping over all
+	 * encoders connected to it using for_each_encoder_on_crtc().
+	 *
+	 * This callback is a variant of @disable that provides the atomic state
+	 * to the driver. It takes priority over @disable during atomic commits.
+	 *
+	 * This hook is used only by atomic helpers. Atomic drivers don't need
+	 * to implement it if there's no need to disable anything at the encoder
+	 * level. To ensure that runtime PM handling (using either DPMS or the
+	 * new "ACTIVE" property) works @atomic_disable must be the inverse of
+	 * @atomic_enable.
+	 */
+	void (*atomic_disable)(struct drm_encoder *encoder,
+			       struct drm_atomic_state *state);
+
+	/**
+	 * @atomic_enable:
+	 *
+	 * This callback should be used to enable the encoder. It is called
+	 * after this encoder's CRTC has been enabled using their own
+	 * &drm_crtc_helper_funcs.atomic_enable hook. If that sequence is
+	 * too simple drivers can just add their own driver private encoder
+	 * hooks and call them from CRTC's callback by looping over all encoders
+	 * connected to it using for_each_encoder_on_crtc().
+	 *
+	 * This callback is a variant of @enable that provides the atomic state
+	 * to the driver. It is called in place of @enable during atomic
+	 * commits.
+	 *
+	 * This hook is used only by atomic helpers, for symmetry with @disable.
+	 * Atomic drivers don't need to implement it if there's no need to
+	 * enable anything at the encoder level. To ensure that runtime PM
+	 * handling (using either DPMS or the new "ACTIVE" property) works
+	 * @enable must be the inverse of @disable for atomic drivers.
+	 */
+	void (*atomic_enable)(struct drm_encoder *encoder,
+			      struct drm_atomic_state *state);
+
 	/**
 	 * @disable:
 	 *
@@ -695,6 +740,9 @@  struct drm_encoder_helper_funcs {
 	 * handling (using either DPMS or the new "ACTIVE" property) works
 	 * @disable must be the inverse of @enable for atomic drivers.
 	 *
+	 * For atomic drivers also consider @atomic_disable and save yourself
+	 * from having to read the NOTE below!
+	 *
 	 * NOTE:
 	 *
 	 * With legacy CRTC helpers there's a big semantic difference between