Re: [PATCH v2 1/2] drm: Introduce crtc->mode_valid() callback

[Manasi Navare <manasi.d.navare@intel.com>]

On Fri, Apr 28, 2017 at 02:47:11PM +0100, Jose Abreu wrote:
> Some crtc's may have restrictions in the mode they can display. In
> this patch a new callback (crtc->mode_valid()) is introduced that
> is called at the same stage of connector->mode_valid() callback.
> 
> This shall be implemented if the crtc has some sort of restriction
> so that we don't probe modes that will fail in the commit() stage.
> 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
> probbed modes to only the ones that can be displayed.
> 
> If the crtc does not implement the callback then the behaviour will
> remain the same. Also, for a given set of crtcs that can be bound to
> the connector, if at least one can display the mode then the mode
> will be probbed.
> 
> Signed-off-by: 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>
> ---
> 
> Changes v1->v2:
> 	- Correct indentation
> 	- Change function name to drm_validate_connector
> 	- Move connnector->mode_valid() to new function
> 	- Change crtc->mode_valid() "mode" field to const
> 	- Code reogarnization
> 	- Return earlier if crtc accepts mode
> 
>  drivers/gpu/drm/drm_probe_helper.c       | 50 ++++++++++++++++++++++++++++++--
>  include/drm/drm_modeset_helper_vtables.h | 26 +++++++++++++++++
>  2 files changed, 74 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 1b0c14a..0741f36 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -80,6 +80,52 @@
>  	return MODE_OK;
>  }
>  
> +static enum drm_mode_status
> +drm_mode_validate_connector(struct drm_connector *connector,
> +			    struct drm_display_mode *mode)
> +{
> +	const struct drm_connector_helper_funcs *connector_funcs =
> +		connector->helper_private;
> +	struct drm_device *dev = connector->dev;
> +	uint32_t *ids = connector->encoder_ids;
> +	enum drm_mode_status ret = MODE_OK;
> +	unsigned int i;
> +
> +	/* Step 1: Validate against connector */
> +	if (connector_funcs->mode_valid)
> +		ret = connector_funcs->mode_valid(connector, mode);
> +
> +	if (ret != MODE_OK)
> +		return ret;
> +
> +	/* Step 2: Validate against crtc's */
> +	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> +		struct drm_encoder *encoder = drm_encoder_find(dev, ids[i]);
> +		struct drm_crtc *crtc;
> +
> +		if (!encoder)
> +			continue;
> +
> +		drm_for_each_crtc(crtc, dev) {
> +			const struct drm_crtc_helper_funcs *crtc_funcs;
> +
> +			if (!drm_encoder_crtc_ok(encoder, crtc))
> +				continue;
> +
> +			crtc_funcs = crtc->helper_private;
> +			if (!crtc_funcs || !crtc_funcs->mode_valid)
> +				return MODE_OK; /* crtc accepts everything */
> +
> +			ret = crtc_funcs->mode_valid(crtc, mode);
> +			if (ret == MODE_OK)
> +				return ret;

> +		}
> +	}
> +
> +	/* NOTE: If no crtc or encoder is found then we return MODE_OK */

I think this comment is misleading because here ret is not alwyas MODE_OK.
It will be the return value from crtc_func->mode_valid which could also
be MODE_NOCLOCK

Manasi

> +	return ret;
> +}
> +
>  static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>  {
>  	struct drm_cmdline_mode *cmdline_mode;
> @@ -428,8 +474,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  		if (mode->status == MODE_OK)
>  			mode->status = drm_mode_validate_flag(mode, mode_flags);
>  
> -		if (mode->status == MODE_OK && connector_funcs->mode_valid)
> -			mode->status = connector_funcs->mode_valid(connector,
> +		if (mode->status == MODE_OK)
> +			mode->status = drm_mode_validate_connector(connector,
>  								   mode);
>  	}
>  
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index c01c328..ecb055c 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -106,6 +106,32 @@ struct drm_crtc_helper_funcs {
>  	void (*commit)(struct drm_crtc *crtc);
>  
>  	/**
> +	 * @mode_valid:
> +	 *
> +	 * This callback 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 probbed modes to only the ones
> +	 * that can be displayed.
> +	 *
> +	 * This is directly called at the same stage of connector->mode_valid
> +	 * callback.
> +	 *
> +	 * NOTE:
> +	 *
> +	 * For a given set of crtc's in a drm_device, if at least one does not
> +	 * have the mode_valid callback, or, at least one returns MODE_OK then
> +	 * the mode will be probbed.
> +	 *
> +	 * 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
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel