Message-ID: <20170512070027.s6usptrpltm52fhi@phenom.ffwll.local>
Date:   Fri, 12 May 2017 09:00:27 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Jose Abreu <Jose.Abreu@...opsys.com>
Cc:     dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Carlos Palminha <CARLOS.PALMINHA@...opsys.com>,
        Alexey Brodkin <Alexey.Brodkin@...opsys.com>,
        Ville Syrjälä 
        <ville.syrjala@...ux.intel.com>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Dave Airlie <airlied@...ux.ie>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Archit Taneja <architt@...eaurora.org>
Subject: Re: [PATCH v3 1/6] drm: Add crtc/encoder/bridge->mode_valid()
 callbacks

On Thu, May 11, 2017 at 10:05:57AM +0100, Jose Abreu wrote:
> 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.
> 
> Signed-off-by: Jose Abreu <joabreu@...opsys.com>
> Cc: Carlos Palminha <palminha@...opsys.com>
> Cc: Alexey Brodkin <abrodkin@...opsys.com>
> Cc: Ville Syrjälä <ville.syrjala@...ux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@...ll.ch>
> Cc: Dave Airlie <airlied@...ux.ie>
> Cc: Andrzej Hajda <a.hajda@...sung.com>
> Cc: Archit Taneja <architt@...eaurora.org>
> ---
> 
> Changes v2->v3:
> 	- Try to document the callbacks a little bit better and
> 	review current documentation (Daniel)

Not quite what I had in mind, all the language about "Please be aware that
neither core nor helper filter modes" is still there. And that was added
to explain the difference between mode_valid and mode_fixup.

Since all the other patches look good I'll take  stab at a v3 myself.
-Daniel

> Changes v1->v2:
> 	- Change description of connector->mode_valid() (Daniel)
> 
>  include/drm/drm_bridge.h                 |  20 ++++++
>  include/drm/drm_modeset_helper_vtables.h | 110 +++++++++++++++++++++++--------
>  2 files changed, 103 insertions(+), 27 deletions(-)
> 
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index fdd82fc..00c6c36 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -59,6 +59,26 @@ 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 is called at mode probe and at atomic check phase.
> +	 *
> +	 * 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
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index c01c328..b07b7cd 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -106,14 +106,37 @@ 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 is called at mode probe and at atomic check phase.
> +	 *
> +	 * 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
> -	 * display mode that userspace requested, adjusted_mode is the mode the
> -	 * encoders need to be fed with. Note that this is the inverse semantics
> -	 * of the meaning for the &drm_encoder and &drm_bridge_funcs.mode_fixup
> -	 * vfunc. If the CRTC cannot support the requested conversion from mode
> -	 * to adjusted_mode it should reject the modeset.
> +	 * This callback is used to do the validation of an adjusted mode in the
> +	 * crtc. The parameter mode is the display mode that userspace requested,
> +	 * adjusted_mode is the mode the encoders need to be fed with. Note that
> +	 * this is the inverse semantics of the meaning for the &drm_encoder and
> +	 * &drm_bridge_funcs.mode_fixup vfunc. If the CRTC cannot support the
> +	 * requested conversion from mode to adjusted_mode it should reject the
> +	 * modeset. Also note that initial validation of a mode supplied by
> +	 * userspace should be done in &drm_crtc_helper_funcs.mode_valid and not
> +	 * in this callback.
>  	 *
>  	 * This function is used by both legacy CRTC helpers and atomic helpers.
>  	 * With atomic helpers it is optional.
> @@ -135,17 +158,19 @@ struct drm_crtc_helper_funcs {
>  	 * 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.
> +	 * &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_valid,
> +	 * &drm_encoder_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid
> +	 * callbacks, where applicable.
>  	 *
>  	 * RETURNS:
>  	 *
> @@ -457,11 +482,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 is called at mode probe and at atomic check phase.
> +	 *
> +	 * 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
> -	 * mode is the display mode that should be fed to the next element in
> -	 * the display chain, either the final &drm_connector or a &drm_bridge.
> +	 * This callback is used to adjust a mode. The parameter mode is the
> +	 * display mode that should be fed to the next element in the display
> +	 * chain, either the final &drm_connector or a &drm_bridge.
>  	 * The parameter adjusted_mode is the input mode the encoder requires. It
>  	 * can be modified by this callback and does not need to match mode.
>  	 *
> @@ -484,19 +529,20 @@ struct drm_encoder_helper_funcs {
>  	 *
>  	 * 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
> +	 * advertised to userspace is filtered using the
>  	 * &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
> +	 * 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.
> +	 * the &drm_crtc_helper_funcs.mode_valid,
> +	 * &drm_encoder_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid
> +	 * callbacks, where applicable.
>  	 *
>  	 * RETURNS:
>  	 *
> @@ -795,13 +841,23 @@ 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 callback is never called in atomic check phase so that userspace
> +	 * can override kernel sink checks in case of broken EDID with wrong
> +	 * limits from the sink. You can use the remaining mode_valid()
> +	 * callbacks to validate the mode against your video path.
> +	 *
>  	 * NOTE:
>  	 *
>  	 * This only filters the mode list supplied to userspace in the
> -	 * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and
> +	 * GETCONNECTOR 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.
> +	 * CRTC helpers will not call this function but the mode will be
> +	 * validated in atomic check phase using the mode_valid() callbacks
> +	 * (&drm_crtc_helper_funcs.mode_valid, &drm_encoder_helper_funcs.mode_valid
> +	 * and &drm_bridge_funcs.mode_valid).
> +	 * Drivers therefore must implement the mode_valid() callbacks if the
> +	 * video pipeline has some sort of restrictions in the modes that can
> +	 * be displayed.
>  	 *
>  	 * To avoid races with concurrent connector state updates, the helper
>  	 * libraries always call this with the &drm_mode_config.connection_mutex
> -- 
> 1.9.1
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists