On Wed, May 08, 2019 at 12:09:06PM -0400, Sean Paul wrote:
> From: Sean Paul <[email protected]>
>
> 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)
Why no r-b: me or did you not apply all my suggestions? Too lazy to read
it all again :-)
-Daniel
>
> Link to v3: https://patchwork.freedesktop.org/patch/msgid/[email protected]
>
> Cc: Daniel Vetter <[email protected]>
> Cc: Ville Syrj?l? <[email protected]>
> Signed-off-by: Sean Paul <[email protected]>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 8 +++-
> include/drm/drm_modeset_helper_vtables.h | 48 ++++++++++++++++++++++++
> 2 files changed, 54 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 553415fe8ede..ccf01831f265 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -999,7 +999,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);
> @@ -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..aa509c107083 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:
> *
> @@ -691,6 +736,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
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Wed, May 08, 2019 at 06:31:24PM +0200, Daniel Vetter wrote:
> On Wed, May 08, 2019 at 12:09:06PM -0400, Sean Paul wrote:
> > From: Sean Paul <[email protected]>
> >
> > 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)
>
> Why no r-b: me or did you not apply all my suggestions? Too lazy to read
> it all again :-)
Sorry, I was being a bit conservative slapping your R-b on the patches I
changed. I've incorporated all of your suggestions AFAICT, so I've applied R-b
on patches 01/03/04/05 (conveniently the same patch numbers between v3 & v4).
Unless there's other review feedback I'll probably not send those out. I will
send out an update to Laurent's patch with your suggestions and R-b after a bit
of soak time on the list.
Sean
> -Daniel
>
> >
> > Link to v3: https://patchwork.freedesktop.org/patch/msgid/[email protected]
> >
> > Cc: Daniel Vetter <[email protected]>
> > Cc: Ville Syrj?l? <[email protected]>
> > Signed-off-by: Sean Paul <[email protected]>
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 8 +++-
> > include/drm/drm_modeset_helper_vtables.h | 48 ++++++++++++++++++++++++
> > 2 files changed, 54 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 553415fe8ede..ccf01831f265 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -999,7 +999,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);
> > @@ -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..aa509c107083 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:
> > *
> > @@ -691,6 +736,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
> > --
> > Sean Paul, Software Engineer, Google / Chromium OS
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Sean Paul, Software Engineer, Google / Chromium OS