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

[Daniel Vetter]

On Wed, Apr 26, 2017 at 11:48:34AM +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@xxxxxxxxxxxx>
> Cc: Carlos Palminha <palminha@xxxxxxxxxxxx>
> Cc: Alexey Brodkin <abrodkin@xxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Dave Airlie <airlied@xxxxxxxx>

Not sure this is useful, since you still have to duplicate the exact same
check into your ->mode_fixup hook. That seems to make things even more
confusing.

Also this doesn't update the various kerneldoc comments. For the existing
hooks. Since this topic causes so much confusion, I don't think a
half-solution will help, but has some good potential to make things worse.
-Daniel

> ---
>  drivers/gpu/drm/drm_probe_helper.c       | 44 ++++++++++++++++++++++++++++++++
>  include/drm/drm_modeset_helper_vtables.h | 26 +++++++++++++++++++
>  2 files changed, 70 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 1b0c14a..61eac30 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -80,6 +80,46 @@
>  	return MODE_OK;
>  }
>  
> +static enum drm_mode_status drm_mode_validate_connector_crtc(
> +		struct drm_connector *connector,
> +		struct drm_display_mode *mode)
> +{
> +	const struct drm_crtc_helper_funcs *crtc_funcs = NULL;
> +	enum drm_mode_status mode_status = MODE_ERROR;
> +	struct drm_device *dev = connector->dev;
> +	struct drm_encoder *encoder;
> +	struct drm_crtc *crtc;
> +	bool callback_found = false;
> +	int i;
> +
> +	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> +		encoder = drm_encoder_find(dev, connector->encoder_ids[i]);
> +
> +		if (!encoder)
> +			continue;
> +
> +		drm_for_each_crtc(crtc, dev) {
> +			crtc_funcs = crtc->helper_private;
> +
> +			if (!drm_encoder_crtc_ok(encoder, crtc))
> +				continue;
> +			if (!crtc_funcs || !crtc_funcs->mode_valid)
> +				continue;
> +
> +			/* MODE_OK=0 and default mode_status=MODE_ERROR=-1
> +			 * so if at least one crtc accepts the mode we get
> +			 * MODE_OK */
> +			mode_status &= crtc_funcs->mode_valid(crtc, mode);
> +			callback_found |= true;
> +		}
> +	}
> +
> +	/* We can reach here without calling mode_valid if there is no
> +	 * registered crtc with this callback, lets return MODE_OK in this
> +	 * case */
> +	return callback_found ? mode_status : MODE_OK;
> +}
> +
>  static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>  {
>  	struct drm_cmdline_mode *cmdline_mode;
> @@ -431,6 +471,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  		if (mode->status == MODE_OK && connector_funcs->mode_valid)
>  			mode->status = connector_funcs->mode_valid(connector,
>  								   mode);
> +
> +		if (mode->status == MODE_OK)
> +			mode->status = drm_mode_validate_connector_crtc(
> +					connector, mode);
>  	}
>  
>  prune:
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index c01c328..59fffba 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

&drm_connector_helper_funcs.mode_valid to make a proper link. Please also
check the output of make htmldocs and make sure it looks pretty.

> +	 * 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,
> +			struct drm_display_mode *mode);
> +
> +	/**
>  	 * @mode_fixup:
>  	 *
>  	 * This callback is used to validate a mode. The parameter mode is the
> -- 
> 1.9.1
> 
> 

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