| Message ID | 20170512073100.3252-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, On Friday 12 May 2017 09:31:00 Daniel Vetter wrote: > From: Jose Abreu <Jose.Abreu@synopsys.com> > > This adds a new callback to crtc, encoder and bridge helper functions > called mode_valid(). This callback shall be implemented if the > corresponding component has some sort of restriction in the modes > that can be displayed. A NULL callback implicates that the component > can display all the modes. > > We also change the documentation so that the new and old callbacks > are correctly documented. > > Only the callbacks were implemented to simplify review process, > following patches will make use of them. > > Changes in v2 from Daniel: > - Update the warning about how modes aren't filtered in atomic_check - > the heleprs help out a lot more now. > - Consistenly roll out that warning, crtc/encoder's atomic_check > missed it. > - Sprinkle more links all over the place, so it's easier to see where > this stuff is used and how the differen hooks are related. > - Note that ->mode_valid is optional everywhere. > - Explain why the connector's mode_valid is special and does _not_ get > called in atomic_check. As commented on v2 (but after you sent this patch), I think we need to document clearly how mode_valid and mode_fixup should interact. It's very confusing for driver authors at the moment. > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > Cc: Jose Abreu <joabreu@synopsys.com> > Cc: Carlos Palminha <palminha@synopsys.com> > Cc: Alexey Brodkin <abrodkin@synopsys.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Dave Airlie <airlied@linux.ie> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Archit Taneja <architt@codeaurora.org> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> (v2) > --- > include/drm/drm_bridge.h | 31 +++++++++ > include/drm/drm_modeset_helper_vtables.h | 116 > ++++++++++++++++++++++--------- 2 files changed, 114 insertions(+), 33 > deletions(-) > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index fdd82fcbf168..f694de756ecf 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -59,6 +59,31 @@ struct drm_bridge_funcs { > void (*detach)(struct drm_bridge *bridge); > > /** > + * @mode_valid: > + * > + * This callback is used to check if a specific mode is valid in this > + * bridge. This should be implemented if the bridge has some sort of > + * restriction in the modes it can display. For example, a given bridge > + * may be responsible to set a clock value. If the clock can not > + * produce all the values for the available modes then this callback > + * can be used to restrict the number of modes to only the ones that > + * can be displayed. > + * > + * This hook is used by the probe helpers to filter the mode list in > + * drm_helper_probe_single_connector_modes(), and it is used by the > + * atomic helpers to validate modes supplied by userspace in > + * drm_atomic_helper_check_modeset(). > + * > + * This function is optional. > + * > + * RETURNS: > + * > + * drm_mode_status Enum > + */ > + enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc, > + const struct drm_display_mode *mode); > + > + /** > * @mode_fixup: > * > * This callback is used to validate and adjust a mode. The paramater > @@ -82,6 +107,12 @@ struct drm_bridge_funcs { > * NOT touch any persistent state (hardware or software) or data > * structures except the passed in @state parameter. > * > + * Also beware that userspace can request its own custom modes, neither > + * core nor helpers filter modes to the list of probe modes reported by > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > + * that modes are filtered consistently put any bridge constraints and > + * limits checks into @mode_valid. > + * > * RETURNS: > * > * True if an acceptable configuration is possible, false if the modeset > diff --git a/include/drm/drm_modeset_helper_vtables.h > b/include/drm/drm_modeset_helper_vtables.h index c01c328f6cc8..91d071ff1232 > 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -106,6 +106,31 @@ struct drm_crtc_helper_funcs { > void (*commit)(struct drm_crtc *crtc); > > /** > + * @mode_valid: > + * > + * This callback is used to check if a specific mode is valid in this > + * crtc. This should be implemented if the crtc has some sort of > + * restriction in the modes it can display. For example, a given crtc > + * may be responsible to set a clock value. If the clock can not > + * produce all the values for the available modes then this callback > + * can be used to restrict the number of modes to only the ones that > + * can be displayed. > + * > + * This hook is used by the probe helpers to filter the mode list in > + * drm_helper_probe_single_connector_modes(), and it is used by the > + * atomic helpers to validate modes supplied by userspace in > + * drm_atomic_helper_check_modeset(). > + * > + * This function is optional. > + * > + * RETURNS: > + * > + * drm_mode_status Enum > + */ > + enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, > + const struct drm_display_mode *mode); > + > + /** > * @mode_fixup: > * > * This callback is used to validate a mode. The parameter mode is the > @@ -132,20 +157,11 @@ struct drm_crtc_helper_funcs { > * Atomic drivers which need to inspect and adjust more state should > * instead use the @atomic_check callback. > * > - * Also beware that neither core nor helpers filter modes before > - * passing them to the driver: While the list of modes that is > - * advertised to userspace is filtered using the > - * &drm_connector.mode_valid callback, neither the core nor the helpers > - * do any filtering on modes passed in from userspace when setting a > - * mode. It is therefore possible for userspace to pass in a mode that > - * was previously filtered out using &drm_connector.mode_valid or add a > - * custom mode that wasn't probed from EDID or similar to begin with. > - * Even though this is an advanced feature and rarely used nowadays, > - * some users rely on being able to specify modes manually so drivers > - * must be prepared to deal with it. Specifically this means that all > - * drivers need not only validate modes in &drm_connector.mode_valid but > - * also in this or in the &drm_encoder_helper_funcs.mode_fixup callback > - * to make sure invalid modes passed in from userspace are rejected. > + * Also beware that userspace can request its own custom modes, neither > + * core nor helpers filter modes to the list of probe modes reported by > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > + * that modes are filtered consistently put any CRTC constraints and > + * limits checks into @mode_valid. > * > * RETURNS: > * > @@ -341,6 +357,12 @@ struct drm_crtc_helper_funcs { > * state objects passed-in or assembled in the overall &drm_atomic_state > * update tracking structure. > * > + * Also beware that userspace can request its own custom modes, neither > + * core nor helpers filter modes to the list of probe modes reported by > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > + * that modes are filtered consistently put any CRTC constraints and > + * limits checks into @mode_valid. > + * > * RETURNS: > * > * 0 on success, -EINVAL if the state or the transition can't be > @@ -457,6 +479,31 @@ struct drm_encoder_helper_funcs { > void (*dpms)(struct drm_encoder *encoder, int mode); > > /** > + * @mode_valid: > + * > + * This callback is used to check if a specific mode is valid in this > + * encoder. This should be implemented if the encoder has some sort > + * of restriction in the modes it can display. For example, a given > + * encoder may be responsible to set a clock value. If the clock can > + * not produce all the values for the available modes then this callback > + * can be used to restrict the number of modes to only the ones that > + * can be displayed. > + * > + * This hook is used by the probe helpers to filter the mode list in > + * drm_helper_probe_single_connector_modes(), and it is used by the > + * atomic helpers to validate modes supplied by userspace in > + * drm_atomic_helper_check_modeset(). > + * > + * This function is optional. > + * > + * RETURNS: > + * > + * drm_mode_status Enum > + */ > + enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc, > + const struct drm_display_mode *mode); > + > + /** > * @mode_fixup: > * > * This callback is used to validate and adjust a mode. The parameter > @@ -482,21 +529,11 @@ struct drm_encoder_helper_funcs { > * Atomic drivers which need to inspect and adjust more state should > * instead use the @atomic_check callback. > * > - * Also beware that neither core nor helpers filter modes before > - * passing them to the driver: While the list of modes that is > - * advertised to userspace is filtered using the connector's > - * &drm_connector_helper_funcs.mode_valid callback, neither the core nor > - * the helpers do any filtering on modes passed in from userspace when > - * setting a mode. It is therefore possible for userspace to pass in a > - * mode that was previously filtered out using > - * &drm_connector_helper_funcs.mode_valid or add a custom mode that > - * wasn't probed from EDID or similar to begin with. Even though this > - * is an advanced feature and rarely used nowadays, some users rely on > - * being able to specify modes manually so drivers must be prepared to > - * deal with it. Specifically this means that all drivers need not only > - * validate modes in &drm_connector.mode_valid but also in this or in > - * the &drm_crtc_helper_funcs.mode_fixup callback to make sure > - * invalid modes passed in from userspace are rejected. > + * Also beware that userspace can request its own custom modes, neither > + * core nor helpers filter modes to the list of probe modes reported by > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > + * that modes are filtered consistently put any encoder constraints and > + * limits checks into @mode_valid. > * > * RETURNS: > * > @@ -686,6 +723,12 @@ struct drm_encoder_helper_funcs { > * state objects passed-in or assembled in the overall &drm_atomic_state > * update tracking structure. > * > + * Also beware that userspace can request its own custom modes, neither > + * core nor helpers filter modes to the list of probe modes reported by > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > + * that modes are filtered consistently put any encoder constraints and > + * limits checks into @mode_valid. > + * > * RETURNS: > * > * 0 on success, -EINVAL if the state or the transition can't be > @@ -795,13 +838,20 @@ struct drm_connector_helper_funcs { > * (which is usually derived from the EDID data block from the sink). > * See e.g. drm_helper_probe_single_connector_modes(). > * > + * This function is optional. > + * > * NOTE: > * > * This only filters the mode list supplied to userspace in the > - * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and > - * ask the kernel to use them. It this case the atomic helpers or legacy > - * CRTC helpers will not call this function. Drivers therefore must > - * still fully validate any mode passed in in a modeset request. > + * GETCONNECTOR IOCTL. Compared to &drm_encoder_helper_funcs.mode_valid, > + * &drm_crtc_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid, > + * which are also called by the atomic helpers from > + * drm_atomic_helper_check_modeset(). This allows userspace to force and > + * ignore sink constraint (like the pixel clock limits in the screen's > + * EDID), which is useful for e.g. testing, or working around a broken > + * EDID. Any source hardware constraint (which always need to be > + * enforced) therefore should be checked in one of the above callbacks, > + * and not this one here. > * > * To avoid races with concurrent connector state updates, the helper > * libraries always call this with the &drm_mode_config.connection_mutex
On 12.05.2017 09:31, Daniel Vetter wrote: > From: Jose Abreu <Jose.Abreu@synopsys.com> > > This adds a new callback to crtc, encoder and bridge helper functions > called mode_valid(). This callback shall be implemented if the > corresponding component has some sort of restriction in the modes > that can be displayed. A NULL callback implicates that the component > can display all the modes. > > We also change the documentation so that the new and old callbacks > are correctly documented. > > Only the callbacks were implemented to simplify review process, > following patches will make use of them. > > Changes in v2 from Daniel: > - Update the warning about how modes aren't filtered in atomic_check - > the heleprs help out a lot more now. > - Consistenly roll out that warning, crtc/encoder's atomic_check > missed it. > - Sprinkle more links all over the place, so it's easier to see where > this stuff is used and how the differen hooks are related. > - Note that ->mode_valid is optional everywhere. > - Explain why the connector's mode_valid is special and does _not_ get > called in atomic_check. > > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > Cc: Jose Abreu <joabreu@synopsys.com> > Cc: Carlos Palminha <palminha@synopsys.com> > Cc: Alexey Brodkin <abrodkin@synopsys.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Dave Airlie <airlied@linux.ie> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Archit Taneja <architt@codeaurora.org> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> (v2) > --- > include/drm/drm_bridge.h | 31 +++++++++ > include/drm/drm_modeset_helper_vtables.h | 116 ++++++++++++++++++++++--------- > 2 files changed, 114 insertions(+), 33 deletions(-) > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index fdd82fcbf168..f694de756ecf 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -59,6 +59,31 @@ struct drm_bridge_funcs { > void (*detach)(struct drm_bridge *bridge); > > /** > + * @mode_valid: > + * > + * This callback is used to check if a specific mode is valid in this > + * bridge. This should be implemented if the bridge has some sort of > + * restriction in the modes it can display. For example, a given bridge > + * may be responsible to set a clock value. If the clock can not > + * produce all the values for the available modes then this callback > + * can be used to restrict the number of modes to only the ones that > + * can be displayed. > + * > + * This hook is used by the probe helpers to filter the mode list in > + * drm_helper_probe_single_connector_modes(), and it is used by the > + * atomic helpers to validate modes supplied by userspace in > + * drm_atomic_helper_check_modeset(). > + * > + * This function is optional. > + * > + * RETURNS: > + * > + * drm_mode_status Enum > + */ > + enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc, > + const struct drm_display_mode *mode); I have put my concerns here but it touches all mode_valid callbacks. As the callback can be called in mode probe and atomic check it could only inspect common set of properties for both contexts, for example crtc cannot check to which encoder it is connected, am I right? Maybe it would be good to emphasize it here, as it is emphasized in case of mode_fixup callbacks. > + > + /** > * @mode_fixup: > * > * This callback is used to validate and adjust a mode. The paramater > @@ -82,6 +107,12 @@ struct drm_bridge_funcs { > * NOT touch any persistent state (hardware or software) or data > * structures except the passed in @state parameter. > * > + * Also beware that userspace can request its own custom modes, neither > + * core nor helpers filter modes to the list of probe modes reported by > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > + * that modes are filtered consistently put any bridge constraints and > + * limits checks into @mode_valid. > + * > * RETURNS: > * > * True if an acceptable configuration is possible, false if the modeset > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > index c01c328f6cc8..91d071ff1232 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -106,6 +106,31 @@ struct drm_crtc_helper_funcs { > void (*commit)(struct drm_crtc *crtc); > > /** > + * @mode_valid: > + * > + * This callback is used to check if a specific mode is valid in this > + * crtc. This should be implemented if the crtc has some sort of > + * restriction in the modes it can display. For example, a given crtc > + * may be responsible to set a clock value. If the clock can not > + * produce all the values for the available modes then this callback > + * can be used to restrict the number of modes to only the ones that > + * can be displayed. > + * > + * This hook is used by the probe helpers to filter the mode list in > + * drm_helper_probe_single_connector_modes(), and it is used by the > + * atomic helpers to validate modes supplied by userspace in > + * drm_atomic_helper_check_modeset(). > + * > + * This function is optional. > + * > + * RETURNS: > + * > + * drm_mode_status Enum > + */ > + enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, > + const struct drm_display_mode *mode); > + > + /** > * @mode_fixup: > * > * This callback is used to validate a mode. The parameter mode is the > @@ -132,20 +157,11 @@ struct drm_crtc_helper_funcs { > * Atomic drivers which need to inspect and adjust more state should > * instead use the @atomic_check callback. > * > - * Also beware that neither core nor helpers filter modes before > - * passing them to the driver: While the list of modes that is > - * advertised to userspace is filtered using the > - * &drm_connector.mode_valid callback, neither the core nor the helpers > - * do any filtering on modes passed in from userspace when setting a > - * mode. It is therefore possible for userspace to pass in a mode that > - * was previously filtered out using &drm_connector.mode_valid or add a > - * custom mode that wasn't probed from EDID or similar to begin with. > - * Even though this is an advanced feature and rarely used nowadays, > - * some users rely on being able to specify modes manually so drivers > - * must be prepared to deal with it. Specifically this means that all > - * drivers need not only validate modes in &drm_connector.mode_valid but > - * also in this or in the &drm_encoder_helper_funcs.mode_fixup callback > - * to make sure invalid modes passed in from userspace are rejected. > + * Also beware that userspace can request its own custom modes, neither > + * core nor helpers filter modes to the list of probe modes reported by > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > + * that modes are filtered consistently put any CRTC constraints and > + * limits checks into @mode_valid. Why we do not make mode_fixup obsolete for atomic drivers as atomic_check can do the same and is more powerful, this way we could avoid the overlapped functionality of mode_valid and mode_fixup. > * > * RETURNS: > * > @@ -341,6 +357,12 @@ struct drm_crtc_helper_funcs { > * state objects passed-in or assembled in the overall &drm_atomic_state > * update tracking structure. > * > + * Also beware that userspace can request its own custom modes, neither > + * core nor helpers filter modes to the list of probe modes reported by > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > + * that modes are filtered consistently put any CRTC constraints and > + * limits checks into @mode_valid. > + * > * RETURNS: > * > * 0 on success, -EINVAL if the state or the transition can't be > @@ -457,6 +479,31 @@ struct drm_encoder_helper_funcs { > void (*dpms)(struct drm_encoder *encoder, int mode); > > /** > + * @mode_valid: > + * > + * This callback is used to check if a specific mode is valid in this > + * encoder. This should be implemented if the encoder has some sort > + * of restriction in the modes it can display. For example, a given > + * encoder may be responsible to set a clock value. If the clock can > + * not produce all the values for the available modes then this callback > + * can be used to restrict the number of modes to only the ones that > + * can be displayed. > + * > + * This hook is used by the probe helpers to filter the mode list in > + * drm_helper_probe_single_connector_modes(), and it is used by the > + * atomic helpers to validate modes supplied by userspace in > + * drm_atomic_helper_check_modeset(). > + * > + * This function is optional. > + * > + * RETURNS: > + * > + * drm_mode_status Enum > + */ > + enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc, > + const struct drm_display_mode *mode); > + > + /** > * @mode_fixup: > * > * This callback is used to validate and adjust a mode. The parameter > @@ -482,21 +529,11 @@ struct drm_encoder_helper_funcs { > * Atomic drivers which need to inspect and adjust more state should > * instead use the @atomic_check callback. > * > - * Also beware that neither core nor helpers filter modes before > - * passing them to the driver: While the list of modes that is > - * advertised to userspace is filtered using the connector's > - * &drm_connector_helper_funcs.mode_valid callback, neither the core nor > - * the helpers do any filtering on modes passed in from userspace when > - * setting a mode. It is therefore possible for userspace to pass in a > - * mode that was previously filtered out using > - * &drm_connector_helper_funcs.mode_valid or add a custom mode that > - * wasn't probed from EDID or similar to begin with. Even though this > - * is an advanced feature and rarely used nowadays, some users rely on > - * being able to specify modes manually so drivers must be prepared to > - * deal with it. Specifically this means that all drivers need not only > - * validate modes in &drm_connector.mode_valid but also in this or in > - * the &drm_crtc_helper_funcs.mode_fixup callback to make sure > - * invalid modes passed in from userspace are rejected. > + * Also beware that userspace can request its own custom modes, neither > + * core nor helpers filter modes to the list of probe modes reported by > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > + * that modes are filtered consistently put any encoder constraints and > + * limits checks into @mode_valid. > * > * RETURNS: > * > @@ -686,6 +723,12 @@ struct drm_encoder_helper_funcs { > * state objects passed-in or assembled in the overall &drm_atomic_state > * update tracking structure. > * > + * Also beware that userspace can request its own custom modes, neither > + * core nor helpers filter modes to the list of probe modes reported by > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > + * that modes are filtered consistently put any encoder constraints and > + * limits checks into @mode_valid. > + * > * RETURNS: > * > * 0 on success, -EINVAL if the state or the transition can't be > @@ -795,13 +838,20 @@ struct drm_connector_helper_funcs { > * (which is usually derived from the EDID data block from the sink). > * See e.g. drm_helper_probe_single_connector_modes(). > * > + * This function is optional. > + * > * NOTE: > * > * This only filters the mode list supplied to userspace in the > - * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and > - * ask the kernel to use them. It this case the atomic helpers or legacy > - * CRTC helpers will not call this function. Drivers therefore must > - * still fully validate any mode passed in in a modeset request. > + * GETCONNECTOR IOCTL. Compared to &drm_encoder_helper_funcs.mode_valid, > + * &drm_crtc_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid, > + * which are also called by the atomic helpers from > + * drm_atomic_helper_check_modeset(). This allows userspace to force and > + * ignore sink constraint (like the pixel clock limits in the screen's > + * EDID), which is useful for e.g. testing, or working around a broken > + * EDID. Any source hardware constraint (which always need to be > + * enforced) therefore should be checked in one of the above callbacks, > + * and not this one here. The callback has the same name but different semantic, little bit weird. But do we really need connector::mode_valid then? I mean: are there connectors not bound to bridge/encoder which have their own constraints? If there are no such connectors, we can remove this callback. If they are rare, maybe just adding loop to connector::get_modes would be enough. Regards Andrzej > * > * To avoid races with concurrent connector state updates, the helper > * libraries always call this with the &drm_mode_config.connection_mutex
Hi Daniel, On 12-05-2017 08:31, Daniel Vetter wrote: > From: Jose Abreu <Jose.Abreu@synopsys.com> > > This adds a new callback to crtc, encoder and bridge helper functions > called mode_valid(). This callback shall be implemented if the > corresponding component has some sort of restriction in the modes > that can be displayed. A NULL callback implicates that the component > can display all the modes. > > We also change the documentation so that the new and old callbacks > are correctly documented. > > Only the callbacks were implemented to simplify review process, > following patches will make use of them. > > Changes in v2 from Daniel: > - Update the warning about how modes aren't filtered in atomic_check - > the heleprs help out a lot more now. > - Consistenly roll out that warning, crtc/encoder's atomic_check > missed it. > - Sprinkle more links all over the place, so it's easier to see where > this stuff is used and how the differen hooks are related. > - Note that ->mode_valid is optional everywhere. > - Explain why the connector's mode_valid is special and does _not_ get > called in atomic_check. > > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > Cc: Jose Abreu <joabreu@synopsys.com> > Cc: Carlos Palminha <palminha@synopsys.com> > Cc: Alexey Brodkin <abrodkin@synopsys.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Dave Airlie <airlied@linux.ie> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Archit Taneja <architt@codeaurora.org> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> (v2) Thanks! Looks fine by me. Reviewed-by: Jose Abreu <joabreu@synopsys.com> Best regards, Jose Miguel Abreu > --- > include/drm/drm_bridge.h | 31 +++++++++ > include/drm/drm_modeset_helper_vtables.h | 116 ++++++++++++++++++++++--------- > 2 files changed, 114 insertions(+), 33 deletions(-) > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index fdd82fcbf168..f694de756ecf 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -59,6 +59,31 @@ struct drm_bridge_funcs { > void (*detach)(struct drm_bridge *bridge); > > /** > + * @mode_valid: > + * > + * This callback is used to check if a specific mode is valid in this > + * bridge. This should be implemented if the bridge has some sort of > + * restriction in the modes it can display. For example, a given bridge > + * may be responsible to set a clock value. If the clock can not > + * produce all the values for the available modes then this callback > + * can be used to restrict the number of modes to only the ones that > + * can be displayed. > + * > + * This hook is used by the probe helpers to filter the mode list in > + * drm_helper_probe_single_connector_modes(), and it is used by the > + * atomic helpers to validate modes supplied by userspace in > + * drm_atomic_helper_check_modeset(). > + * > + * This function is optional. > + * > + * RETURNS: > + * > + * drm_mode_status Enum > + */ > + enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc, > + const struct drm_display_mode *mode); > + > + /** > * @mode_fixup: > * > * This callback is used to validate and adjust a mode. The paramater > @@ -82,6 +107,12 @@ struct drm_bridge_funcs { > * NOT touch any persistent state (hardware or software) or data > * structures except the passed in @state parameter. > * > + * Also beware that userspace can request its own custom modes, neither > + * core nor helpers filter modes to the list of probe modes reported by > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > + * that modes are filtered consistently put any bridge constraints and > + * limits checks into @mode_valid. > + * > * RETURNS: > * > * True if an acceptable configuration is possible, false if the modeset > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > index c01c328f6cc8..91d071ff1232 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -106,6 +106,31 @@ struct drm_crtc_helper_funcs { > void (*commit)(struct drm_crtc *crtc); > > /** > + * @mode_valid: > + * > + * This callback is used to check if a specific mode is valid in this > + * crtc. This should be implemented if the crtc has some sort of > + * restriction in the modes it can display. For example, a given crtc > + * may be responsible to set a clock value. If the clock can not > + * produce all the values for the available modes then this callback > + * can be used to restrict the number of modes to only the ones that > + * can be displayed. > + * > + * This hook is used by the probe helpers to filter the mode list in > + * drm_helper_probe_single_connector_modes(), and it is used by the > + * atomic helpers to validate modes supplied by userspace in > + * drm_atomic_helper_check_modeset(). > + * > + * This function is optional. > + * > + * RETURNS: > + * > + * drm_mode_status Enum > + */ > + enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, > + const struct drm_display_mode *mode); > + > + /** > * @mode_fixup: > * > * This callback is used to validate a mode. The parameter mode is the > @@ -132,20 +157,11 @@ struct drm_crtc_helper_funcs { > * Atomic drivers which need to inspect and adjust more state should > * instead use the @atomic_check callback. > * > - * Also beware that neither core nor helpers filter modes before > - * passing them to the driver: While the list of modes that is > - * advertised to userspace is filtered using the > - * &drm_connector.mode_valid callback, neither the core nor the helpers > - * do any filtering on modes passed in from userspace when setting a > - * mode. It is therefore possible for userspace to pass in a mode that > - * was previously filtered out using &drm_connector.mode_valid or add a > - * custom mode that wasn't probed from EDID or similar to begin with. > - * Even though this is an advanced feature and rarely used nowadays, > - * some users rely on being able to specify modes manually so drivers > - * must be prepared to deal with it. Specifically this means that all > - * drivers need not only validate modes in &drm_connector.mode_valid but > - * also in this or in the &drm_encoder_helper_funcs.mode_fixup callback > - * to make sure invalid modes passed in from userspace are rejected. > + * Also beware that userspace can request its own custom modes, neither > + * core nor helpers filter modes to the list of probe modes reported by > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > + * that modes are filtered consistently put any CRTC constraints and > + * limits checks into @mode_valid. > * > * RETURNS: > * > @@ -341,6 +357,12 @@ struct drm_crtc_helper_funcs { > * state objects passed-in or assembled in the overall &drm_atomic_state > * update tracking structure. > * > + * Also beware that userspace can request its own custom modes, neither > + * core nor helpers filter modes to the list of probe modes reported by > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > + * that modes are filtered consistently put any CRTC constraints and > + * limits checks into @mode_valid. > + * > * RETURNS: > * > * 0 on success, -EINVAL if the state or the transition can't be > @@ -457,6 +479,31 @@ struct drm_encoder_helper_funcs { > void (*dpms)(struct drm_encoder *encoder, int mode); > > /** > + * @mode_valid: > + * > + * This callback is used to check if a specific mode is valid in this > + * encoder. This should be implemented if the encoder has some sort > + * of restriction in the modes it can display. For example, a given > + * encoder may be responsible to set a clock value. If the clock can > + * not produce all the values for the available modes then this callback > + * can be used to restrict the number of modes to only the ones that > + * can be displayed. > + * > + * This hook is used by the probe helpers to filter the mode list in > + * drm_helper_probe_single_connector_modes(), and it is used by the > + * atomic helpers to validate modes supplied by userspace in > + * drm_atomic_helper_check_modeset(). > + * > + * This function is optional. > + * > + * RETURNS: > + * > + * drm_mode_status Enum > + */ > + enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc, > + const struct drm_display_mode *mode); > + > + /** > * @mode_fixup: > * > * This callback is used to validate and adjust a mode. The parameter > @@ -482,21 +529,11 @@ struct drm_encoder_helper_funcs { > * Atomic drivers which need to inspect and adjust more state should > * instead use the @atomic_check callback. > * > - * Also beware that neither core nor helpers filter modes before > - * passing them to the driver: While the list of modes that is > - * advertised to userspace is filtered using the connector's > - * &drm_connector_helper_funcs.mode_valid callback, neither the core nor > - * the helpers do any filtering on modes passed in from userspace when > - * setting a mode. It is therefore possible for userspace to pass in a > - * mode that was previously filtered out using > - * &drm_connector_helper_funcs.mode_valid or add a custom mode that > - * wasn't probed from EDID or similar to begin with. Even though this > - * is an advanced feature and rarely used nowadays, some users rely on > - * being able to specify modes manually so drivers must be prepared to > - * deal with it. Specifically this means that all drivers need not only > - * validate modes in &drm_connector.mode_valid but also in this or in > - * the &drm_crtc_helper_funcs.mode_fixup callback to make sure > - * invalid modes passed in from userspace are rejected. > + * Also beware that userspace can request its own custom modes, neither > + * core nor helpers filter modes to the list of probe modes reported by > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > + * that modes are filtered consistently put any encoder constraints and > + * limits checks into @mode_valid. > * > * RETURNS: > * > @@ -686,6 +723,12 @@ struct drm_encoder_helper_funcs { > * state objects passed-in or assembled in the overall &drm_atomic_state > * update tracking structure. > * > + * Also beware that userspace can request its own custom modes, neither > + * core nor helpers filter modes to the list of probe modes reported by > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > + * that modes are filtered consistently put any encoder constraints and > + * limits checks into @mode_valid. > + * > * RETURNS: > * > * 0 on success, -EINVAL if the state or the transition can't be > @@ -795,13 +838,20 @@ struct drm_connector_helper_funcs { > * (which is usually derived from the EDID data block from the sink). > * See e.g. drm_helper_probe_single_connector_modes(). > * > + * This function is optional. > + * > * NOTE: > * > * This only filters the mode list supplied to userspace in the > - * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and > - * ask the kernel to use them. It this case the atomic helpers or legacy > - * CRTC helpers will not call this function. Drivers therefore must > - * still fully validate any mode passed in in a modeset request. > + * GETCONNECTOR IOCTL. Compared to &drm_encoder_helper_funcs.mode_valid, > + * &drm_crtc_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid, > + * which are also called by the atomic helpers from > + * drm_atomic_helper_check_modeset(). This allows userspace to force and > + * ignore sink constraint (like the pixel clock limits in the screen's > + * EDID), which is useful for e.g. testing, or working around a broken > + * EDID. Any source hardware constraint (which always need to be > + * enforced) therefore should be checked in one of the above callbacks, > + * and not this one here. > * > * To avoid races with concurrent connector state updates, the helper > * libraries always call this with the &drm_mode_config.connection_mutex
On Fri, May 12, 2017 at 02:29:30PM +0300, Laurent Pinchart wrote: > Hi Daniel, > > On Friday 12 May 2017 09:31:00 Daniel Vetter wrote: > > From: Jose Abreu <Jose.Abreu@synopsys.com> > > > > This adds a new callback to crtc, encoder and bridge helper functions > > called mode_valid(). This callback shall be implemented if the > > corresponding component has some sort of restriction in the modes > > that can be displayed. A NULL callback implicates that the component > > can display all the modes. > > > > We also change the documentation so that the new and old callbacks > > are correctly documented. > > > > Only the callbacks were implemented to simplify review process, > > following patches will make use of them. > > > > Changes in v2 from Daniel: > > - Update the warning about how modes aren't filtered in atomic_check - > > the heleprs help out a lot more now. > > - Consistenly roll out that warning, crtc/encoder's atomic_check > > missed it. > > - Sprinkle more links all over the place, so it's easier to see where > > this stuff is used and how the differen hooks are related. > > - Note that ->mode_valid is optional everywhere. > > - Explain why the connector's mode_valid is special and does _not_ get > > called in atomic_check. > > As commented on v2 (but after you sent this patch), I think we need to > document clearly how mode_valid and mode_fixup should interact. It's very > confusing for driver authors at the moment. I tried to do that here, is it not enough? Note that mode_fixup is kinda deprecated, in favour of atomic_check. So the equivalence is not as clear-cut as you seem to think. -Daniel > > > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > > Cc: Jose Abreu <joabreu@synopsys.com> > > Cc: Carlos Palminha <palminha@synopsys.com> > > Cc: Alexey Brodkin <abrodkin@synopsys.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Dave Airlie <airlied@linux.ie> > > Cc: Andrzej Hajda <a.hajda@samsung.com> > > Cc: Archit Taneja <architt@codeaurora.org> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> (v2) > > --- > > include/drm/drm_bridge.h | 31 +++++++++ > > include/drm/drm_modeset_helper_vtables.h | 116 > > ++++++++++++++++++++++--------- 2 files changed, 114 insertions(+), 33 > > deletions(-) > > > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > > index fdd82fcbf168..f694de756ecf 100644 > > --- a/include/drm/drm_bridge.h > > +++ b/include/drm/drm_bridge.h > > @@ -59,6 +59,31 @@ struct drm_bridge_funcs { > > void (*detach)(struct drm_bridge *bridge); > > > > /** > > + * @mode_valid: > > + * > > + * This callback is used to check if a specific mode is valid in this > > + * bridge. This should be implemented if the bridge has some sort of > > + * restriction in the modes it can display. For example, a given > bridge > > + * may be responsible to set a clock value. If the clock can not > > + * produce all the values for the available modes then this callback > > + * can be used to restrict the number of modes to only the ones that > > + * can be displayed. > > + * > > + * This hook is used by the probe helpers to filter the mode list in > > + * drm_helper_probe_single_connector_modes(), and it is used by the > > + * atomic helpers to validate modes supplied by userspace in > > + * drm_atomic_helper_check_modeset(). > > + * > > + * This function is optional. > > + * > > + * RETURNS: > > + * > > + * drm_mode_status Enum > > + */ > > + enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc, > > + const struct drm_display_mode > *mode); > > + > > + /** > > * @mode_fixup: > > * > > * This callback is used to validate and adjust a mode. The paramater > > @@ -82,6 +107,12 @@ struct drm_bridge_funcs { > > * NOT touch any persistent state (hardware or software) or data > > * structures except the passed in @state parameter. > > * > > + * Also beware that userspace can request its own custom modes, > neither > > + * core nor helpers filter modes to the list of probe modes reported > by > > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To > ensure > > + * that modes are filtered consistently put any bridge constraints and > > + * limits checks into @mode_valid. > > + * > > * RETURNS: > > * > > * True if an acceptable configuration is possible, false if the > modeset > > diff --git a/include/drm/drm_modeset_helper_vtables.h > > b/include/drm/drm_modeset_helper_vtables.h index c01c328f6cc8..91d071ff1232 > > 100644 > > --- a/include/drm/drm_modeset_helper_vtables.h > > +++ b/include/drm/drm_modeset_helper_vtables.h > > @@ -106,6 +106,31 @@ struct drm_crtc_helper_funcs { > > void (*commit)(struct drm_crtc *crtc); > > > > /** > > + * @mode_valid: > > + * > > + * This callback is used to check if a specific mode is valid in this > > + * crtc. This should be implemented if the crtc has some sort of > > + * restriction in the modes it can display. For example, a given crtc > > + * may be responsible to set a clock value. If the clock can not > > + * produce all the values for the available modes then this callback > > + * can be used to restrict the number of modes to only the ones that > > + * can be displayed. > > + * > > + * This hook is used by the probe helpers to filter the mode list in > > + * drm_helper_probe_single_connector_modes(), and it is used by the > > + * atomic helpers to validate modes supplied by userspace in > > + * drm_atomic_helper_check_modeset(). > > + * > > + * This function is optional. > > + * > > + * RETURNS: > > + * > > + * drm_mode_status Enum > > + */ > > + enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, > > + const struct drm_display_mode > *mode); > > + > > + /** > > * @mode_fixup: > > * > > * This callback is used to validate a mode. The parameter mode is the > > @@ -132,20 +157,11 @@ struct drm_crtc_helper_funcs { > > * Atomic drivers which need to inspect and adjust more state should > > * instead use the @atomic_check callback. > > * > > - * Also beware that neither core nor helpers filter modes before > > - * passing them to the driver: While the list of modes that is > > - * advertised to userspace is filtered using the > > - * &drm_connector.mode_valid callback, neither the core nor the > helpers > > - * do any filtering on modes passed in from userspace when setting a > > - * mode. It is therefore possible for userspace to pass in a mode that > > - * was previously filtered out using &drm_connector.mode_valid or add > a > > - * custom mode that wasn't probed from EDID or similar to begin with. > > - * Even though this is an advanced feature and rarely used nowadays, > > - * some users rely on being able to specify modes manually so drivers > > - * must be prepared to deal with it. Specifically this means that all > > - * drivers need not only validate modes in &drm_connector.mode_valid > but > > - * also in this or in the &drm_encoder_helper_funcs.mode_fixup > callback > > - * to make sure invalid modes passed in from userspace are rejected. > > + * Also beware that userspace can request its own custom modes, > neither > > + * core nor helpers filter modes to the list of probe modes reported > by > > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To > ensure > > + * that modes are filtered consistently put any CRTC constraints and > > + * limits checks into @mode_valid. > > * > > * RETURNS: > > * > > @@ -341,6 +357,12 @@ struct drm_crtc_helper_funcs { > > * state objects passed-in or assembled in the overall > &drm_atomic_state > > * update tracking structure. > > * > > + * Also beware that userspace can request its own custom modes, > neither > > + * core nor helpers filter modes to the list of probe modes reported > by > > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To > ensure > > + * that modes are filtered consistently put any CRTC constraints and > > + * limits checks into @mode_valid. > > + * > > * RETURNS: > > * > > * 0 on success, -EINVAL if the state or the transition can't be > > @@ -457,6 +479,31 @@ struct drm_encoder_helper_funcs { > > void (*dpms)(struct drm_encoder *encoder, int mode); > > > > /** > > + * @mode_valid: > > + * > > + * This callback is used to check if a specific mode is valid in this > > + * encoder. This should be implemented if the encoder has some sort > > + * of restriction in the modes it can display. For example, a given > > + * encoder may be responsible to set a clock value. If the clock can > > + * not produce all the values for the available modes then this > callback > > + * can be used to restrict the number of modes to only the ones that > > + * can be displayed. > > + * > > + * This hook is used by the probe helpers to filter the mode list in > > + * drm_helper_probe_single_connector_modes(), and it is used by the > > + * atomic helpers to validate modes supplied by userspace in > > + * drm_atomic_helper_check_modeset(). > > + * > > + * This function is optional. > > + * > > + * RETURNS: > > + * > > + * drm_mode_status Enum > > + */ > > + enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc, > > + const struct drm_display_mode > *mode); > > + > > + /** > > * @mode_fixup: > > * > > * This callback is used to validate and adjust a mode. The parameter > > @@ -482,21 +529,11 @@ struct drm_encoder_helper_funcs { > > * Atomic drivers which need to inspect and adjust more state should > > * instead use the @atomic_check callback. > > * > > - * Also beware that neither core nor helpers filter modes before > > - * passing them to the driver: While the list of modes that is > > - * advertised to userspace is filtered using the connector's > > - * &drm_connector_helper_funcs.mode_valid callback, neither the core > nor > > - * the helpers do any filtering on modes passed in from userspace when > > - * setting a mode. It is therefore possible for userspace to pass in a > > - * mode that was previously filtered out using > > - * &drm_connector_helper_funcs.mode_valid or add a custom mode that > > - * wasn't probed from EDID or similar to begin with. Even though this > > - * is an advanced feature and rarely used nowadays, some users rely on > > - * being able to specify modes manually so drivers must be prepared to > > - * deal with it. Specifically this means that all drivers need not > only > > - * validate modes in &drm_connector.mode_valid but also in this or in > > - * the &drm_crtc_helper_funcs.mode_fixup callback to make sure > > - * invalid modes passed in from userspace are rejected. > > + * Also beware that userspace can request its own custom modes, > neither > > + * core nor helpers filter modes to the list of probe modes reported > by > > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To > ensure > > + * that modes are filtered consistently put any encoder constraints > and > > + * limits checks into @mode_valid. > > * > > * RETURNS: > > * > > @@ -686,6 +723,12 @@ struct drm_encoder_helper_funcs { > > * state objects passed-in or assembled in the overall > &drm_atomic_state > > * update tracking structure. > > * > > + * Also beware that userspace can request its own custom modes, > neither > > + * core nor helpers filter modes to the list of probe modes reported > by > > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To > ensure > > + * that modes are filtered consistently put any encoder constraints > and > > + * limits checks into @mode_valid. > > + * > > * RETURNS: > > * > > * 0 on success, -EINVAL if the state or the transition can't be > > @@ -795,13 +838,20 @@ struct drm_connector_helper_funcs { > > * (which is usually derived from the EDID data block from the sink). > > * See e.g. drm_helper_probe_single_connector_modes(). > > * > > + * This function is optional. > > + * > > * NOTE: > > * > > * This only filters the mode list supplied to userspace in the > > - * GETCONNECOTR IOCTL. Userspace is free to create modes of its own > and > > - * ask the kernel to use them. It this case the atomic helpers or > legacy > > - * CRTC helpers will not call this function. Drivers therefore must > > - * still fully validate any mode passed in in a modeset request. > > + * GETCONNECTOR IOCTL. Compared to > &drm_encoder_helper_funcs.mode_valid, > > + * &drm_crtc_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid, > > + * which are also called by the atomic helpers from > > + * drm_atomic_helper_check_modeset(). This allows userspace to force > and > > + * ignore sink constraint (like the pixel clock limits in the screen's > > + * EDID), which is useful for e.g. testing, or working around a broken > > + * EDID. Any source hardware constraint (which always need to be > > + * enforced) therefore should be checked in one of the above > callbacks, > > + * and not this one here. > > * > > * To avoid races with concurrent connector state updates, the helper > > * libraries always call this with the > &drm_mode_config.connection_mutex > > -- > Regards, > > Laurent Pinchart >
On Fri, May 12, 2017 at 02:37:17PM +0200, Andrzej Hajda wrote: > On 12.05.2017 09:31, Daniel Vetter wrote: > > From: Jose Abreu <Jose.Abreu@synopsys.com> > > > > This adds a new callback to crtc, encoder and bridge helper functions > > called mode_valid(). This callback shall be implemented if the > > corresponding component has some sort of restriction in the modes > > that can be displayed. A NULL callback implicates that the component > > can display all the modes. > > > > We also change the documentation so that the new and old callbacks > > are correctly documented. > > > > Only the callbacks were implemented to simplify review process, > > following patches will make use of them. > > > > Changes in v2 from Daniel: > > - Update the warning about how modes aren't filtered in atomic_check - > > the heleprs help out a lot more now. > > - Consistenly roll out that warning, crtc/encoder's atomic_check > > missed it. > > - Sprinkle more links all over the place, so it's easier to see where > > this stuff is used and how the differen hooks are related. > > - Note that ->mode_valid is optional everywhere. > > - Explain why the connector's mode_valid is special and does _not_ get > > called in atomic_check. > > > > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > > Cc: Jose Abreu <joabreu@synopsys.com> > > Cc: Carlos Palminha <palminha@synopsys.com> > > Cc: Alexey Brodkin <abrodkin@synopsys.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Dave Airlie <airlied@linux.ie> > > Cc: Andrzej Hajda <a.hajda@samsung.com> > > Cc: Archit Taneja <architt@codeaurora.org> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> (v2) > > --- > > include/drm/drm_bridge.h | 31 +++++++++ > > include/drm/drm_modeset_helper_vtables.h | 116 ++++++++++++++++++++++--------- > > 2 files changed, 114 insertions(+), 33 deletions(-) > > > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > > index fdd82fcbf168..f694de756ecf 100644 > > --- a/include/drm/drm_bridge.h > > +++ b/include/drm/drm_bridge.h > > @@ -59,6 +59,31 @@ struct drm_bridge_funcs { > > void (*detach)(struct drm_bridge *bridge); > > > > /** > > + * @mode_valid: > > + * > > + * This callback is used to check if a specific mode is valid in this > > + * bridge. This should be implemented if the bridge has some sort of > > + * restriction in the modes it can display. For example, a given bridge > > + * may be responsible to set a clock value. If the clock can not > > + * produce all the values for the available modes then this callback > > + * can be used to restrict the number of modes to only the ones that > > + * can be displayed. > > + * > > + * This hook is used by the probe helpers to filter the mode list in > > + * drm_helper_probe_single_connector_modes(), and it is used by the > > + * atomic helpers to validate modes supplied by userspace in > > + * drm_atomic_helper_check_modeset(). > > + * > > + * This function is optional. > > + * > > + * RETURNS: > > + * > > + * drm_mode_status Enum > > + */ > > + enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc, > > + const struct drm_display_mode *mode); > > I have put my concerns here but it touches all mode_valid callbacks. > As the callback can be called in mode probe and atomic check it could > only inspect common set of properties for both contexts, for example > crtc cannot check to which encoder it is connected, am I right? > Maybe it would be good to emphasize it here, as it is emphasized in case > of mode_fixup callbacks. You can inspect nothing else but the mode here. Looking at anything else is not allowed since this is the generic check which should work for any config. Good question, so I'll augment the docs to address this. > > + > > + /** > > * @mode_fixup: > > * > > * This callback is used to validate and adjust a mode. The paramater > > @@ -82,6 +107,12 @@ struct drm_bridge_funcs { > > * NOT touch any persistent state (hardware or software) or data > > * structures except the passed in @state parameter. > > * > > + * Also beware that userspace can request its own custom modes, neither > > + * core nor helpers filter modes to the list of probe modes reported by > > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > > + * that modes are filtered consistently put any bridge constraints and > > + * limits checks into @mode_valid. > > + * > > * RETURNS: > > * > > * True if an acceptable configuration is possible, false if the modeset > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > > index c01c328f6cc8..91d071ff1232 100644 > > --- a/include/drm/drm_modeset_helper_vtables.h > > +++ b/include/drm/drm_modeset_helper_vtables.h > > @@ -106,6 +106,31 @@ struct drm_crtc_helper_funcs { > > void (*commit)(struct drm_crtc *crtc); > > > > /** > > + * @mode_valid: > > + * > > + * This callback is used to check if a specific mode is valid in this > > + * crtc. This should be implemented if the crtc has some sort of > > + * restriction in the modes it can display. For example, a given crtc > > + * may be responsible to set a clock value. If the clock can not > > + * produce all the values for the available modes then this callback > > + * can be used to restrict the number of modes to only the ones that > > + * can be displayed. > > + * > > + * This hook is used by the probe helpers to filter the mode list in > > + * drm_helper_probe_single_connector_modes(), and it is used by the > > + * atomic helpers to validate modes supplied by userspace in > > + * drm_atomic_helper_check_modeset(). > > + * > > + * This function is optional. > > + * > > + * RETURNS: > > + * > > + * drm_mode_status Enum > > + */ > > + enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, > > + const struct drm_display_mode *mode); > > + > > + /** > > * @mode_fixup: > > * > > * This callback is used to validate a mode. The parameter mode is the > > @@ -132,20 +157,11 @@ struct drm_crtc_helper_funcs { > > * Atomic drivers which need to inspect and adjust more state should > > * instead use the @atomic_check callback. > > * > > - * Also beware that neither core nor helpers filter modes before > > - * passing them to the driver: While the list of modes that is > > - * advertised to userspace is filtered using the > > - * &drm_connector.mode_valid callback, neither the core nor the helpers > > - * do any filtering on modes passed in from userspace when setting a > > - * mode. It is therefore possible for userspace to pass in a mode that > > - * was previously filtered out using &drm_connector.mode_valid or add a > > - * custom mode that wasn't probed from EDID or similar to begin with. > > - * Even though this is an advanced feature and rarely used nowadays, > > - * some users rely on being able to specify modes manually so drivers > > - * must be prepared to deal with it. Specifically this means that all > > - * drivers need not only validate modes in &drm_connector.mode_valid but > > - * also in this or in the &drm_encoder_helper_funcs.mode_fixup callback > > - * to make sure invalid modes passed in from userspace are rejected. > > + * Also beware that userspace can request its own custom modes, neither > > + * core nor helpers filter modes to the list of probe modes reported by > > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > > + * that modes are filtered consistently put any CRTC constraints and > > + * limits checks into @mode_valid. > > Why we do not make mode_fixup obsolete for atomic drivers as > atomic_check can do the same and is more powerful, this way we could > avoid the overlapped functionality of mode_valid and mode_fixup. What do you mean with "make obselete"? mode_fixup is the compat callback, atomic_check is the shiny new one, so it is already obselete. > > * > > * RETURNS: > > * > > @@ -341,6 +357,12 @@ struct drm_crtc_helper_funcs { > > * state objects passed-in or assembled in the overall &drm_atomic_state > > * update tracking structure. > > * > > + * Also beware that userspace can request its own custom modes, neither > > + * core nor helpers filter modes to the list of probe modes reported by > > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > > + * that modes are filtered consistently put any CRTC constraints and > > + * limits checks into @mode_valid. > > + * > > * RETURNS: > > * > > * 0 on success, -EINVAL if the state or the transition can't be > > @@ -457,6 +479,31 @@ struct drm_encoder_helper_funcs { > > void (*dpms)(struct drm_encoder *encoder, int mode); > > > > /** > > + * @mode_valid: > > + * > > + * This callback is used to check if a specific mode is valid in this > > + * encoder. This should be implemented if the encoder has some sort > > + * of restriction in the modes it can display. For example, a given > > + * encoder may be responsible to set a clock value. If the clock can > > + * not produce all the values for the available modes then this callback > > + * can be used to restrict the number of modes to only the ones that > > + * can be displayed. > > + * > > + * This hook is used by the probe helpers to filter the mode list in > > + * drm_helper_probe_single_connector_modes(), and it is used by the > > + * atomic helpers to validate modes supplied by userspace in > > + * drm_atomic_helper_check_modeset(). > > + * > > + * This function is optional. > > + * > > + * RETURNS: > > + * > > + * drm_mode_status Enum > > + */ > > + enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc, > > + const struct drm_display_mode *mode); > > + > > + /** > > * @mode_fixup: > > * > > * This callback is used to validate and adjust a mode. The parameter > > @@ -482,21 +529,11 @@ struct drm_encoder_helper_funcs { > > * Atomic drivers which need to inspect and adjust more state should > > * instead use the @atomic_check callback. > > * > > - * Also beware that neither core nor helpers filter modes before > > - * passing them to the driver: While the list of modes that is > > - * advertised to userspace is filtered using the connector's > > - * &drm_connector_helper_funcs.mode_valid callback, neither the core nor > > - * the helpers do any filtering on modes passed in from userspace when > > - * setting a mode. It is therefore possible for userspace to pass in a > > - * mode that was previously filtered out using > > - * &drm_connector_helper_funcs.mode_valid or add a custom mode that > > - * wasn't probed from EDID or similar to begin with. Even though this > > - * is an advanced feature and rarely used nowadays, some users rely on > > - * being able to specify modes manually so drivers must be prepared to > > - * deal with it. Specifically this means that all drivers need not only > > - * validate modes in &drm_connector.mode_valid but also in this or in > > - * the &drm_crtc_helper_funcs.mode_fixup callback to make sure > > - * invalid modes passed in from userspace are rejected. > > + * Also beware that userspace can request its own custom modes, neither > > + * core nor helpers filter modes to the list of probe modes reported by > > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > > + * that modes are filtered consistently put any encoder constraints and > > + * limits checks into @mode_valid. > > * > > * RETURNS: > > * > > @@ -686,6 +723,12 @@ struct drm_encoder_helper_funcs { > > * state objects passed-in or assembled in the overall &drm_atomic_state > > * update tracking structure. > > * > > + * Also beware that userspace can request its own custom modes, neither > > + * core nor helpers filter modes to the list of probe modes reported by > > + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure > > + * that modes are filtered consistently put any encoder constraints and > > + * limits checks into @mode_valid. > > + * > > * RETURNS: > > * > > * 0 on success, -EINVAL if the state or the transition can't be > > @@ -795,13 +838,20 @@ struct drm_connector_helper_funcs { > > * (which is usually derived from the EDID data block from the sink). > > * See e.g. drm_helper_probe_single_connector_modes(). > > * > > + * This function is optional. > > + * > > * NOTE: > > * > > * This only filters the mode list supplied to userspace in the > > - * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and > > - * ask the kernel to use them. It this case the atomic helpers or legacy > > - * CRTC helpers will not call this function. Drivers therefore must > > - * still fully validate any mode passed in in a modeset request. > > + * GETCONNECTOR IOCTL. Compared to &drm_encoder_helper_funcs.mode_valid, > > + * &drm_crtc_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid, > > + * which are also called by the atomic helpers from > > + * drm_atomic_helper_check_modeset(). This allows userspace to force and > > + * ignore sink constraint (like the pixel clock limits in the screen's > > + * EDID), which is useful for e.g. testing, or working around a broken > > + * EDID. Any source hardware constraint (which always need to be > > + * enforced) therefore should be checked in one of the above callbacks, > > + * and not this one here. > > The callback has the same name but different semantic, little bit weird. > But do we really need connector::mode_valid then? I mean: are there > connectors not bound to bridge/encoder which have their own constraints? > If there are no such connectors, we can remove this callback. Yes. And the pixel clock limit is exactly the current real-world use-case: There are hdmi screens which have a pixel clock limit set, but in reality can take higher modes. There's users out there who want to use these higher modes on these peculiar screens. But for everyone else (and for all other screens), we do need to filter modes correctly (the example here is dual-link vs. single-link limits, which is an interaction between sink and source). > If they are rare, maybe just adding loop to connector::get_modes would > be enough. What do you mean with "adding a loop"? -Daniel
On 15.05.2017 08:56, Daniel Vetter wrote: > On Fri, May 12, 2017 at 02:37:17PM +0200, Andrzej Hajda wrote: >> On 12.05.2017 09:31, Daniel Vetter wrote: >>> From: Jose Abreu <Jose.Abreu@synopsys.com> >>> >>> This adds a new callback to crtc, encoder and bridge helper functions >>> called mode_valid(). This callback shall be implemented if the >>> corresponding component has some sort of restriction in the modes >>> that can be displayed. A NULL callback implicates that the component >>> can display all the modes. >>> >>> We also change the documentation so that the new and old callbacks >>> are correctly documented. >>> >>> Only the callbacks were implemented to simplify review process, >>> following patches will make use of them. >>> >>> Changes in v2 from Daniel: >>> - Update the warning about how modes aren't filtered in atomic_check - >>> the heleprs help out a lot more now. >>> - Consistenly roll out that warning, crtc/encoder's atomic_check >>> missed it. >>> - Sprinkle more links all over the place, so it's easier to see where >>> this stuff is used and how the differen hooks are related. >>> - Note that ->mode_valid is optional everywhere. >>> - Explain why the connector's mode_valid is special and does _not_ get >>> called in atomic_check. >>> >>> Signed-off-by: Jose Abreu <joabreu@synopsys.com> >>> Cc: Jose Abreu <joabreu@synopsys.com> >>> Cc: Carlos Palminha <palminha@synopsys.com> >>> Cc: Alexey Brodkin <abrodkin@synopsys.com> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Cc: Dave Airlie <airlied@linux.ie> >>> Cc: Andrzej Hajda <a.hajda@samsung.com> >>> Cc: Archit Taneja <architt@codeaurora.org> >>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> (v2) >>> --- >>> include/drm/drm_bridge.h | 31 +++++++++ >>> include/drm/drm_modeset_helper_vtables.h | 116 ++++++++++++++++++++++--------- >>> 2 files changed, 114 insertions(+), 33 deletions(-) >>> >>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h >>> index fdd82fcbf168..f694de756ecf 100644 >>> --- a/include/drm/drm_bridge.h >>> +++ b/include/drm/drm_bridge.h >>> @@ -59,6 +59,31 @@ struct drm_bridge_funcs { >>> void (*detach)(struct drm_bridge *bridge); >>> >>> /** >>> + * @mode_valid: >>> + * >>> + * This callback is used to check if a specific mode is valid in this >>> + * bridge. This should be implemented if the bridge has some sort of >>> + * restriction in the modes it can display. For example, a given bridge >>> + * may be responsible to set a clock value. If the clock can not >>> + * produce all the values for the available modes then this callback >>> + * can be used to restrict the number of modes to only the ones that >>> + * can be displayed. >>> + * >>> + * This hook is used by the probe helpers to filter the mode list in >>> + * drm_helper_probe_single_connector_modes(), and it is used by the >>> + * atomic helpers to validate modes supplied by userspace in >>> + * drm_atomic_helper_check_modeset(). >>> + * >>> + * This function is optional. >>> + * >>> + * RETURNS: >>> + * >>> + * drm_mode_status Enum >>> + */ >>> + enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc, >>> + const struct drm_display_mode *mode); >> I have put my concerns here but it touches all mode_valid callbacks. >> As the callback can be called in mode probe and atomic check it could >> only inspect common set of properties for both contexts, for example >> crtc cannot check to which encoder it is connected, am I right? >> Maybe it would be good to emphasize it here, as it is emphasized in case >> of mode_fixup callbacks. > You can inspect nothing else but the mode here. Looking at anything else > is not allowed since this is the generic check which should work for any > config. Good question, so I'll augment the docs to address this. > >>> + >>> + /** >>> * @mode_fixup: >>> * >>> * This callback is used to validate and adjust a mode. The paramater >>> @@ -82,6 +107,12 @@ struct drm_bridge_funcs { >>> * NOT touch any persistent state (hardware or software) or data >>> * structures except the passed in @state parameter. >>> * >>> + * Also beware that userspace can request its own custom modes, neither >>> + * core nor helpers filter modes to the list of probe modes reported by >>> + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure >>> + * that modes are filtered consistently put any bridge constraints and >>> + * limits checks into @mode_valid. >>> + * >>> * RETURNS: >>> * >>> * True if an acceptable configuration is possible, false if the modeset >>> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h >>> index c01c328f6cc8..91d071ff1232 100644 >>> --- a/include/drm/drm_modeset_helper_vtables.h >>> +++ b/include/drm/drm_modeset_helper_vtables.h >>> @@ -106,6 +106,31 @@ struct drm_crtc_helper_funcs { >>> void (*commit)(struct drm_crtc *crtc); >>> >>> /** >>> + * @mode_valid: >>> + * >>> + * This callback is used to check if a specific mode is valid in this >>> + * crtc. This should be implemented if the crtc has some sort of >>> + * restriction in the modes it can display. For example, a given crtc >>> + * may be responsible to set a clock value. If the clock can not >>> + * produce all the values for the available modes then this callback >>> + * can be used to restrict the number of modes to only the ones that >>> + * can be displayed. >>> + * >>> + * This hook is used by the probe helpers to filter the mode list in >>> + * drm_helper_probe_single_connector_modes(), and it is used by the >>> + * atomic helpers to validate modes supplied by userspace in >>> + * drm_atomic_helper_check_modeset(). >>> + * >>> + * This function is optional. >>> + * >>> + * RETURNS: >>> + * >>> + * drm_mode_status Enum >>> + */ >>> + enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, >>> + const struct drm_display_mode *mode); >>> + >>> + /** >>> * @mode_fixup: >>> * >>> * This callback is used to validate a mode. The parameter mode is the >>> @@ -132,20 +157,11 @@ struct drm_crtc_helper_funcs { >>> * Atomic drivers which need to inspect and adjust more state should >>> * instead use the @atomic_check callback. >>> * >>> - * Also beware that neither core nor helpers filter modes before >>> - * passing them to the driver: While the list of modes that is >>> - * advertised to userspace is filtered using the >>> - * &drm_connector.mode_valid callback, neither the core nor the helpers >>> - * do any filtering on modes passed in from userspace when setting a >>> - * mode. It is therefore possible for userspace to pass in a mode that >>> - * was previously filtered out using &drm_connector.mode_valid or add a >>> - * custom mode that wasn't probed from EDID or similar to begin with. >>> - * Even though this is an advanced feature and rarely used nowadays, >>> - * some users rely on being able to specify modes manually so drivers >>> - * must be prepared to deal with it. Specifically this means that all >>> - * drivers need not only validate modes in &drm_connector.mode_valid but >>> - * also in this or in the &drm_encoder_helper_funcs.mode_fixup callback >>> - * to make sure invalid modes passed in from userspace are rejected. >>> + * Also beware that userspace can request its own custom modes, neither >>> + * core nor helpers filter modes to the list of probe modes reported by >>> + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure >>> + * that modes are filtered consistently put any CRTC constraints and >>> + * limits checks into @mode_valid. >> Why we do not make mode_fixup obsolete for atomic drivers as >> atomic_check can do the same and is more powerful, this way we could >> avoid the overlapped functionality of mode_valid and mode_fixup. > What do you mean with "make obselete"? mode_fixup is the compat callback, > atomic_check is the shiny new one, so it is already obselete. Hmm, the docs says 'atomic drivers which need to inspect and adjust more state should instead use the @atomic_check callback', it does not sound like 'mode_fixup is obsolete for atomic drivers, please use atomic_check instead' :) >>> * >>> * RETURNS: >>> * >>> @@ -341,6 +357,12 @@ struct drm_crtc_helper_funcs { >>> * state objects passed-in or assembled in the overall &drm_atomic_state >>> * update tracking structure. >>> * >>> + * Also beware that userspace can request its own custom modes, neither >>> + * core nor helpers filter modes to the list of probe modes reported by >>> + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure >>> + * that modes are filtered consistently put any CRTC constraints and >>> + * limits checks into @mode_valid. >>> + * >>> * RETURNS: >>> * >>> * 0 on success, -EINVAL if the state or the transition can't be >>> @@ -457,6 +479,31 @@ struct drm_encoder_helper_funcs { >>> void (*dpms)(struct drm_encoder *encoder, int mode); >>> >>> /** >>> + * @mode_valid: >>> + * >>> + * This callback is used to check if a specific mode is valid in this >>> + * encoder. This should be implemented if the encoder has some sort >>> + * of restriction in the modes it can display. For example, a given >>> + * encoder may be responsible to set a clock value. If the clock can >>> + * not produce all the values for the available modes then this callback >>> + * can be used to restrict the number of modes to only the ones that >>> + * can be displayed. >>> + * >>> + * This hook is used by the probe helpers to filter the mode list in >>> + * drm_helper_probe_single_connector_modes(), and it is used by the >>> + * atomic helpers to validate modes supplied by userspace in >>> + * drm_atomic_helper_check_modeset(). >>> + * >>> + * This function is optional. >>> + * >>> + * RETURNS: >>> + * >>> + * drm_mode_status Enum >>> + */ >>> + enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc, >>> + const struct drm_display_mode *mode); >>> + >>> + /** >>> * @mode_fixup: >>> * >>> * This callback is used to validate and adjust a mode. The parameter >>> @@ -482,21 +529,11 @@ struct drm_encoder_helper_funcs { >>> * Atomic drivers which need to inspect and adjust more state should >>> * instead use the @atomic_check callback. >>> * >>> - * Also beware that neither core nor helpers filter modes before >>> - * passing them to the driver: While the list of modes that is >>> - * advertised to userspace is filtered using the connector's >>> - * &drm_connector_helper_funcs.mode_valid callback, neither the core nor >>> - * the helpers do any filtering on modes passed in from userspace when >>> - * setting a mode. It is therefore possible for userspace to pass in a >>> - * mode that was previously filtered out using >>> - * &drm_connector_helper_funcs.mode_valid or add a custom mode that >>> - * wasn't probed from EDID or similar to begin with. Even though this >>> - * is an advanced feature and rarely used nowadays, some users rely on >>> - * being able to specify modes manually so drivers must be prepared to >>> - * deal with it. Specifically this means that all drivers need not only >>> - * validate modes in &drm_connector.mode_valid but also in this or in >>> - * the &drm_crtc_helper_funcs.mode_fixup callback to make sure >>> - * invalid modes passed in from userspace are rejected. >>> + * Also beware that userspace can request its own custom modes, neither >>> + * core nor helpers filter modes to the list of probe modes reported by >>> + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure >>> + * that modes are filtered consistently put any encoder constraints and >>> + * limits checks into @mode_valid. >>> * >>> * RETURNS: >>> * >>> @@ -686,6 +723,12 @@ struct drm_encoder_helper_funcs { >>> * state objects passed-in or assembled in the overall &drm_atomic_state >>> * update tracking structure. >>> * >>> + * Also beware that userspace can request its own custom modes, neither >>> + * core nor helpers filter modes to the list of probe modes reported by >>> + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure >>> + * that modes are filtered consistently put any encoder constraints and >>> + * limits checks into @mode_valid. >>> + * >>> * RETURNS: >>> * >>> * 0 on success, -EINVAL if the state or the transition can't be >>> @@ -795,13 +838,20 @@ struct drm_connector_helper_funcs { >>> * (which is usually derived from the EDID data block from the sink). >>> * See e.g. drm_helper_probe_single_connector_modes(). >>> * >>> + * This function is optional. >>> + * >>> * NOTE: >>> * >>> * This only filters the mode list supplied to userspace in the >>> - * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and >>> - * ask the kernel to use them. It this case the atomic helpers or legacy >>> - * CRTC helpers will not call this function. Drivers therefore must >>> - * still fully validate any mode passed in in a modeset request. >>> + * GETCONNECTOR IOCTL. Compared to &drm_encoder_helper_funcs.mode_valid, >>> + * &drm_crtc_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid, >>> + * which are also called by the atomic helpers from >>> + * drm_atomic_helper_check_modeset(). This allows userspace to force and >>> + * ignore sink constraint (like the pixel clock limits in the screen's >>> + * EDID), which is useful for e.g. testing, or working around a broken >>> + * EDID. Any source hardware constraint (which always need to be >>> + * enforced) therefore should be checked in one of the above callbacks, >>> + * and not this one here. >> The callback has the same name but different semantic, little bit weird. >> But do we really need connector::mode_valid then? I mean: are there >> connectors not bound to bridge/encoder which have their own constraints? >> If there are no such connectors, we can remove this callback. > Yes. And the pixel clock limit is exactly the current real-world use-case: > There are hdmi screens which have a pixel clock limit set, but in reality > can take higher modes. There's users out there who want to use these > higher modes on these peculiar screens. But for everyone else (and for all > other screens), we do need to filter modes correctly (the example here is > dual-link vs. single-link limits, which is an interaction between sink and > source). > >> If they are rare, maybe just adding loop to connector::get_modes would >> be enough. > What do you mean with "adding a loop"? > -Daniel Sorry for confusing shortcut, I have just inspected call sites of both callbacks - drm_helper_probe_single_connector_modes, both callbacks are called from this function, so my idea was to move loop 'list_for_each_entry(mode, &connector->modes, head){ ...., ...->mode_valid(...); ....}' from drm_helper_probe_single_connector_modes to .get_modes callback. After looking bit more at this function I have realized that such change is not so obvious, and it is not really clear if the final result will be better :). Regards Andrzej
On Mon, May 15, 2017 at 10:10 AM, Andrzej Hajda <a.hajda@samsung.com> wrote: > Sorry for confusing shortcut, I have just inspected call sites of both > callbacks - drm_helper_probe_single_connector_modes, both callbacks are > called from this function, so my idea was to move loop > 'list_for_each_entry(mode, &connector->modes, head){ ...., > ...->mode_valid(...); ....}' from > drm_helper_probe_single_connector_modes to .get_modes callback. After > looking bit more at this function I have realized that such change is > not so obvious, and it is not really clear if the final result will be > better :). This would defeat the purpose of ->get_modes, which is to just grab the modes from the sink, without filtering for source constraints, which is done by ->mode_valid. That framework is why we have the probe helpers, if you want to do something else you can always implement ->fill_modes (which is the uapi entry point) yourself. -Daniel
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index fdd82fcbf168..f694de756ecf 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -59,6 +59,31 @@ struct drm_bridge_funcs { void (*detach)(struct drm_bridge *bridge); /** + * @mode_valid: + * + * This callback is used to check if a specific mode is valid in this + * bridge. This should be implemented if the bridge has some sort of + * restriction in the modes it can display. For example, a given bridge + * may be responsible to set a clock value. If the clock can not + * produce all the values for the available modes then this callback + * can be used to restrict the number of modes to only the ones that + * can be displayed. + * + * This hook is used by the probe helpers to filter the mode list in + * drm_helper_probe_single_connector_modes(), and it is used by the + * atomic helpers to validate modes supplied by userspace in + * drm_atomic_helper_check_modeset(). + * + * This function is optional. + * + * RETURNS: + * + * drm_mode_status Enum + */ + enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc, + const struct drm_display_mode *mode); + + /** * @mode_fixup: * * This callback is used to validate and adjust a mode. The paramater @@ -82,6 +107,12 @@ struct drm_bridge_funcs { * NOT touch any persistent state (hardware or software) or data * structures except the passed in @state parameter. * + * Also beware that userspace can request its own custom modes, neither + * core nor helpers filter modes to the list of probe modes reported by + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure + * that modes are filtered consistently put any bridge constraints and + * limits checks into @mode_valid. + * * RETURNS: * * True if an acceptable configuration is possible, false if the modeset diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index c01c328f6cc8..91d071ff1232 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -106,6 +106,31 @@ struct drm_crtc_helper_funcs { void (*commit)(struct drm_crtc *crtc); /** + * @mode_valid: + * + * This callback is used to check if a specific mode is valid in this + * crtc. This should be implemented if the crtc has some sort of + * restriction in the modes it can display. For example, a given crtc + * may be responsible to set a clock value. If the clock can not + * produce all the values for the available modes then this callback + * can be used to restrict the number of modes to only the ones that + * can be displayed. + * + * This hook is used by the probe helpers to filter the mode list in + * drm_helper_probe_single_connector_modes(), and it is used by the + * atomic helpers to validate modes supplied by userspace in + * drm_atomic_helper_check_modeset(). + * + * This function is optional. + * + * RETURNS: + * + * drm_mode_status Enum + */ + enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, + const struct drm_display_mode *mode); + + /** * @mode_fixup: * * This callback is used to validate a mode. The parameter mode is the @@ -132,20 +157,11 @@ struct drm_crtc_helper_funcs { * Atomic drivers which need to inspect and adjust more state should * instead use the @atomic_check callback. * - * Also beware that neither core nor helpers filter modes before - * passing them to the driver: While the list of modes that is - * advertised to userspace is filtered using the - * &drm_connector.mode_valid callback, neither the core nor the helpers - * do any filtering on modes passed in from userspace when setting a - * mode. It is therefore possible for userspace to pass in a mode that - * was previously filtered out using &drm_connector.mode_valid or add a - * custom mode that wasn't probed from EDID or similar to begin with. - * Even though this is an advanced feature and rarely used nowadays, - * some users rely on being able to specify modes manually so drivers - * must be prepared to deal with it. Specifically this means that all - * drivers need not only validate modes in &drm_connector.mode_valid but - * also in this or in the &drm_encoder_helper_funcs.mode_fixup callback - * to make sure invalid modes passed in from userspace are rejected. + * Also beware that userspace can request its own custom modes, neither + * core nor helpers filter modes to the list of probe modes reported by + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure + * that modes are filtered consistently put any CRTC constraints and + * limits checks into @mode_valid. * * RETURNS: * @@ -341,6 +357,12 @@ struct drm_crtc_helper_funcs { * state objects passed-in or assembled in the overall &drm_atomic_state * update tracking structure. * + * Also beware that userspace can request its own custom modes, neither + * core nor helpers filter modes to the list of probe modes reported by + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure + * that modes are filtered consistently put any CRTC constraints and + * limits checks into @mode_valid. + * * RETURNS: * * 0 on success, -EINVAL if the state or the transition can't be @@ -457,6 +479,31 @@ struct drm_encoder_helper_funcs { void (*dpms)(struct drm_encoder *encoder, int mode); /** + * @mode_valid: + * + * This callback is used to check if a specific mode is valid in this + * encoder. This should be implemented if the encoder has some sort + * of restriction in the modes it can display. For example, a given + * encoder may be responsible to set a clock value. If the clock can + * not produce all the values for the available modes then this callback + * can be used to restrict the number of modes to only the ones that + * can be displayed. + * + * This hook is used by the probe helpers to filter the mode list in + * drm_helper_probe_single_connector_modes(), and it is used by the + * atomic helpers to validate modes supplied by userspace in + * drm_atomic_helper_check_modeset(). + * + * This function is optional. + * + * RETURNS: + * + * drm_mode_status Enum + */ + enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc, + const struct drm_display_mode *mode); + + /** * @mode_fixup: * * This callback is used to validate and adjust a mode. The parameter @@ -482,21 +529,11 @@ struct drm_encoder_helper_funcs { * Atomic drivers which need to inspect and adjust more state should * instead use the @atomic_check callback. * - * Also beware that neither core nor helpers filter modes before - * passing them to the driver: While the list of modes that is - * advertised to userspace is filtered using the connector's - * &drm_connector_helper_funcs.mode_valid callback, neither the core nor - * the helpers do any filtering on modes passed in from userspace when - * setting a mode. It is therefore possible for userspace to pass in a - * mode that was previously filtered out using - * &drm_connector_helper_funcs.mode_valid or add a custom mode that - * wasn't probed from EDID or similar to begin with. Even though this - * is an advanced feature and rarely used nowadays, some users rely on - * being able to specify modes manually so drivers must be prepared to - * deal with it. Specifically this means that all drivers need not only - * validate modes in &drm_connector.mode_valid but also in this or in - * the &drm_crtc_helper_funcs.mode_fixup callback to make sure - * invalid modes passed in from userspace are rejected. + * Also beware that userspace can request its own custom modes, neither + * core nor helpers filter modes to the list of probe modes reported by + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure + * that modes are filtered consistently put any encoder constraints and + * limits checks into @mode_valid. * * RETURNS: * @@ -686,6 +723,12 @@ struct drm_encoder_helper_funcs { * state objects passed-in or assembled in the overall &drm_atomic_state * update tracking structure. * + * Also beware that userspace can request its own custom modes, neither + * core nor helpers filter modes to the list of probe modes reported by + * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure + * that modes are filtered consistently put any encoder constraints and + * limits checks into @mode_valid. + * * RETURNS: * * 0 on success, -EINVAL if the state or the transition can't be @@ -795,13 +838,20 @@ struct drm_connector_helper_funcs { * (which is usually derived from the EDID data block from the sink). * See e.g. drm_helper_probe_single_connector_modes(). * + * This function is optional. + * * NOTE: * * This only filters the mode list supplied to userspace in the - * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and - * ask the kernel to use them. It this case the atomic helpers or legacy - * CRTC helpers will not call this function. Drivers therefore must - * still fully validate any mode passed in in a modeset request. + * GETCONNECTOR IOCTL. Compared to &drm_encoder_helper_funcs.mode_valid, + * &drm_crtc_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid, + * which are also called by the atomic helpers from + * drm_atomic_helper_check_modeset(). This allows userspace to force and + * ignore sink constraint (like the pixel clock limits in the screen's + * EDID), which is useful for e.g. testing, or working around a broken + * EDID. Any source hardware constraint (which always need to be + * enforced) therefore should be checked in one of the above callbacks, + * and not this one here. * * To avoid races with concurrent connector state updates, the helper * libraries always call this with the &drm_mode_config.connection_mutex