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 <seanpaul@xxxxxxxxxxxx> > > > > 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/20190502194956.218441-2-sean@xxxxxxxxxx > > > > Cc: Daniel Vetter <daniel@xxxxxxxx> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx> > > --- > > 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
