On Fri, May 3, 2019 at 2:34 PM Sean Paul <sean@xxxxxxxxxx> 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@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 > > > > > > 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 | 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@xxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel