diff mbox series

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

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

Commit Message

Sean Paul May 2, 2019, 7:49 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

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/drm_atomic_helper.c      |  6 +++-
 include/drm/drm_modeset_helper_vtables.h | 45 ++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)

Comments

Daniel Vetter May 3, 2019, 7:51 a.m. UTC | #1 
On Thu, May 02, 2019 at 03:49:43PM -0400, Sean Paul wrote:
> 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
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c      |  6 +++-
>  include/drm/drm_modeset_helper_vtables.h | 45 ++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 553415fe8ede..71cc7d6b0644 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1001,6 +1001,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  		if (funcs) {
>  			if (new_conn_state->crtc && funcs->prepare)
>  				funcs->prepare(encoder);
> +			else if (funcs->atomic_disable)
> +				funcs->atomic_disable(encoder, old_state);

I'd put the atomic one first. The reason why ->disable here is later is
that ->disable has some really funky meaning in the old legacy crtc
helpers, and hence ->prepare was preferrable: Legacy ->disable also was
supposed to release resources (something we now do in
->atomic_destroy_state). See the huge NOTE: in the @disable kerneldoc.


>  			else if (funcs->disable)
>  				funcs->disable(encoder);
>  			else if (funcs->dpms)
> @@ -1309,7 +1311,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 8f3602811eb5..de57fb40cb6e 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -675,6 +675,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.
> +	 */

I'd add something like "For atomic drivers also consider @atomic_disable"
to the kerneldoc of @disable (before the NOTE: which is only relevant for
pre-atomic). Same for the enable side.

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

needs to be adjusted here for "takes priority".
> +	 *
> +	 * 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);
> +

With the nits:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  	/**
>  	 * @disable:
>  	 *
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
>
Sean Paul May 3, 2019, 12:34 p.m. UTC | #2 
On Fri, May 03, 2019 at 09:51:30AM +0200, Daniel Vetter wrote:
> On Thu, May 02, 2019 at 03:49:43PM -0400, Sean Paul wrote:
> > 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
> > 
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c      |  6 +++-
> >  include/drm/drm_modeset_helper_vtables.h | 45 ++++++++++++++++++++++++
> >  2 files changed, 50 insertions(+), 1 deletion(-)

/snip

> > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > index 8f3602811eb5..de57fb40cb6e 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -675,6 +675,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.
> > +	 */
> 
> I'd add something like "For atomic drivers also consider @atomic_disable"
> to the kerneldoc of @disable (before the NOTE: which is only relevant for
> pre-atomic). Same for the enable side.
> 
> > +	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.
> 
> needs to be adjusted here for "takes priority".

Can you clarify this comment? I'm a little fuzzy on what it means.



> > +	 *
> > +	 * 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);
> > +
> 
> With the nits:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks!

Sean

> 
> >  	/**
> >  	 * @disable:
> >  	 *
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter May 3, 2019, 2:08 p.m. UTC | #3 
On Fri, May 3, 2019 at 2:34 PM Sean Paul <sean@poorly.run> wrote:
>
> On Fri, May 03, 2019 at 09:51:30AM +0200, Daniel Vetter wrote:
> > On Thu, May 02, 2019 at 03:49:43PM -0400, Sean Paul wrote:
> > > 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
> > >
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c      |  6 +++-
> > >  include/drm/drm_modeset_helper_vtables.h | 45 ++++++++++++++++++++++++
> > >  2 files changed, 50 insertions(+), 1 deletion(-)
>
> /snip
>
> > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > > index 8f3602811eb5..de57fb40cb6e 100644
> > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > @@ -675,6 +675,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.
> > > +    */
> >
> > I'd add something like "For atomic drivers also consider @atomic_disable"
> > to the kerneldoc of @disable (before the NOTE: which is only relevant for
> > pre-atomic). Same for the enable side.
> >
> > > +   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.
> >
> > needs to be adjusted here for "takes priority".
>
> Can you clarify this comment? I'm a little fuzzy on what it means.

Further up I suggest that @atomic_disable should take priority over
all the others (plus explain why @disable is lower than @prepare,
because of the special semantics this has in legacy crtc helpers).
Once you do that you also need to adjust the wording in the kerneldoc
here (same wording as in @atomic_enable sounds good to me), i.e.
explain that @atomic_disable takes priority over all other hooks.
-Daniel

>
>
>
> > > +    *
> > > +    * 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);
> > > +
> >
> > With the nits:
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Thanks!
>
> Sean
>
> >
> > >     /**
> > >      * @disable:
> > >      *
> > > --
> > > Sean Paul, Software Engineer, Google / Chromium OS
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 553415fe8ede..71cc7d6b0644 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1001,6 +1001,8 @@  disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		if (funcs) {
 			if (new_conn_state->crtc && funcs->prepare)
 				funcs->prepare(encoder);
+			else if (funcs->atomic_disable)
+				funcs->atomic_disable(encoder, old_state);
 			else if (funcs->disable)
 				funcs->disable(encoder);
 			else if (funcs->dpms)
@@ -1309,7 +1311,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 8f3602811eb5..de57fb40cb6e 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -675,6 +675,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:
 	 *