| Message ID | 20170515091136.26307-2-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, On 15-05-2017 10:11, Daniel Vetter wrote: > Brought up by both Laurent and Andrzej when reviewing the new > ->mode_valid hooks. Since mode_fixup is just a simpler version of the > much more generic atomic_check we can't really unify it with > mode_valid. Most drivers should probably switch their current > mode_fixup code to either the new mode_valid or the atomic_check > hooks, but e.g. that doesn't exist yet for bridges, and for CRTCs the > situation is a bit more complicated. Hence there's no clear > equivalence between mode_fixup and mode_valid, even if it looks like > that at first glance. > > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Jose Abreu <Jose.Abreu@synopsys.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Reviewed-by: Jose Abreu <joabreu@synopsys.com> Best regards, Jose Miguel Abreu > --- > include/drm/drm_modeset_helper_vtables.h | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > index c72fca544a41..613b2a602b77 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -156,7 +156,11 @@ struct drm_crtc_helper_funcs { > * allowed. > * > * Atomic drivers which need to inspect and adjust more state should > - * instead use the @atomic_check callback. > + * instead use the @atomic_check callback, but note that they're not > + * perfectly equivalent: @mode_valid is called from > + * drm_atomic_helper_check_modeset(), but @atomic_check is called from > + * drm_atomic_helper_check_planes(), because originally it was meant for > + * plane update checks only.. > * > * Also beware that userspace can request its own custom modes, neither > * core nor helpers filter modes to the list of probe modes reported by > @@ -529,7 +533,9 @@ struct drm_encoder_helper_funcs { > * allowed. > * > * Atomic drivers which need to inspect and adjust more state should > - * instead use the @atomic_check callback. > + * instead use the @atomic_check callback. If @atomic_check is used, > + * this hook isn't called since @atomic_check allows a strict superset > + * of the functionality of @mode_fixup. > * > * Also beware that userspace can request its own custom modes, neither > * core nor helpers filter modes to the list of probe modes reported by > @@ -716,6 +722,11 @@ struct drm_encoder_helper_funcs { > * update the CRTC to match what the encoder needs for the requested > * connector. > * > + * Since this provides a strict superset of the functionality of > + * @mode_fixup (the requested and adjusted modes are both available > + * through the passed in &struct drm_crtc_state) @mode_fixup is not > + * called when @atomic_check is implemented. > + * > * This function is used by the atomic helpers, but it is optional. > * > * NOTE:
On 15.05.2017 11:11, Daniel Vetter wrote: > Brought up by both Laurent and Andrzej when reviewing the new > ->mode_valid hooks. Since mode_fixup is just a simpler version of the > much more generic atomic_check we can't really unify it with > mode_valid. Most drivers should probably switch their current > mode_fixup code to either the new mode_valid or the atomic_check > hooks, but e.g. that doesn't exist yet for bridges, and for CRTCs the > situation is a bit more complicated. Hence there's no clear > equivalence between mode_fixup and mode_valid, even if it looks like > that at first glance. > > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Jose Abreu <Jose.Abreu@synopsys.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > include/drm/drm_modeset_helper_vtables.h | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > index c72fca544a41..613b2a602b77 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -156,7 +156,11 @@ struct drm_crtc_helper_funcs { > * allowed. > * > * Atomic drivers which need to inspect and adjust more state should > - * instead use the @atomic_check callback. > + * instead use the @atomic_check callback, but note that they're not > + * perfectly equivalent: @mode_valid is called from > + * drm_atomic_helper_check_modeset(), but @atomic_check is called from > + * drm_atomic_helper_check_planes(), because originally it was meant for > + * plane update checks only.. Two dots. Beside this: Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> -- Regards Andrzej > * > * Also beware that userspace can request its own custom modes, neither > * core nor helpers filter modes to the list of probe modes reported by > @@ -529,7 +533,9 @@ struct drm_encoder_helper_funcs { > * allowed. > * > * Atomic drivers which need to inspect and adjust more state should > - * instead use the @atomic_check callback. > + * instead use the @atomic_check callback. If @atomic_check is used, > + * this hook isn't called since @atomic_check allows a strict superset > + * of the functionality of @mode_fixup. > * > * Also beware that userspace can request its own custom modes, neither > * core nor helpers filter modes to the list of probe modes reported by > @@ -716,6 +722,11 @@ struct drm_encoder_helper_funcs { > * update the CRTC to match what the encoder needs for the requested > * connector. > * > + * Since this provides a strict superset of the functionality of > + * @mode_fixup (the requested and adjusted modes are both available > + * through the passed in &struct drm_crtc_state) @mode_fixup is not > + * called when @atomic_check is implemented. > + * > * This function is used by the atomic helpers, but it is optional. > * > * NOTE:
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index c72fca544a41..613b2a602b77 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -156,7 +156,11 @@ struct drm_crtc_helper_funcs { * allowed. * * Atomic drivers which need to inspect and adjust more state should - * instead use the @atomic_check callback. + * instead use the @atomic_check callback, but note that they're not + * perfectly equivalent: @mode_valid is called from + * drm_atomic_helper_check_modeset(), but @atomic_check is called from + * drm_atomic_helper_check_planes(), because originally it was meant for + * plane update checks only.. * * Also beware that userspace can request its own custom modes, neither * core nor helpers filter modes to the list of probe modes reported by @@ -529,7 +533,9 @@ struct drm_encoder_helper_funcs { * allowed. * * Atomic drivers which need to inspect and adjust more state should - * instead use the @atomic_check callback. + * instead use the @atomic_check callback. If @atomic_check is used, + * this hook isn't called since @atomic_check allows a strict superset + * of the functionality of @mode_fixup. * * Also beware that userspace can request its own custom modes, neither * core nor helpers filter modes to the list of probe modes reported by @@ -716,6 +722,11 @@ struct drm_encoder_helper_funcs { * update the CRTC to match what the encoder needs for the requested * connector. * + * Since this provides a strict superset of the functionality of + * @mode_fixup (the requested and adjusted modes are both available + * through the passed in &struct drm_crtc_state) @mode_fixup is not + * called when @atomic_check is implemented. + * * This function is used by the atomic helpers, but it is optional. * * NOTE:
Brought up by both Laurent and Andrzej when reviewing the new ->mode_valid hooks. Since mode_fixup is just a simpler version of the much more generic atomic_check we can't really unify it with mode_valid. Most drivers should probably switch their current mode_fixup code to either the new mode_valid or the atomic_check hooks, but e.g. that doesn't exist yet for bridges, and for CRTCs the situation is a bit more complicated. Hence there's no clear equivalence between mode_fixup and mode_valid, even if it looks like that at first glance. Cc: Andrzej Hajda <a.hajda@samsung.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Jose Abreu <Jose.Abreu@synopsys.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- include/drm/drm_modeset_helper_vtables.h | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)