Message-ID: <20200710221600.GA29318@ideak-desk.fi.intel.com>
Date:   Sat, 11 Jul 2020 01:16:00 +0300
From:   Imre Deak <imre.deak@...el.com>
To:     Lyude Paul <lyude@...hat.com>
Cc:     intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...ux.ie>,
        open list <linux-kernel@...r.kernel.org>,
        Maxime Ripard <mripard@...nel.org>
Subject: Re: [Intel-gfx] [PATCH v3 1/2] drm/probe_helper: Add
 drm_connector_helper_funcs.mode_valid_ctx

On Fri, Jul 10, 2020 at 03:31:43PM -0400, Lyude Paul wrote:
> This is just an atomic version of mode_valid, which is intended to be
> used for situations where a driver might need to check the atomic state
> of objects other than the connector itself. One such example is with
> MST, where the maximum possible bandwidth on a connector can change
> dynamically irregardless of the display configuration.
> 
> Changes since v1:
> * Use new drm logging functions
> * Make some corrections in the mode_valid_ctx kdoc
> * Return error codes or 0 from ->mode_valid_ctx() on fail, and store the
>   drm_mode_status in an additional function parameter
> Changes since v2:
> * Don't accidentally assign ret to mode->status on success, or we'll
>   squash legitimate mode validation results
> * Don't forget to assign MODE_OK to status in drm_connector_mode_valid()
>   if we have no callbacks
> * Drop leftover hunk in drm_modes.h around enum drm_mode_status
> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> Cc: Lee Shawn C <shawn.c.lee@...el.com>

Some nits below, but looks good in either way:

Reviewed-and-tested-by: Imre Deak <imre.deak@...el.com>


> ---
>  drivers/gpu/drm/drm_crtc_helper_internal.h |  7 +-
>  drivers/gpu/drm/drm_probe_helper.c         | 94 ++++++++++++++--------
>  include/drm/drm_modeset_helper_vtables.h   | 42 ++++++++++
>  3 files changed, 109 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h b/drivers/gpu/drm/drm_crtc_helper_internal.h
> index f0a66ef47e5ad..25ce42e799952 100644
> --- a/drivers/gpu/drm/drm_crtc_helper_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
> @@ -73,8 +73,11 @@ enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
>  					 const struct drm_display_mode *mode);
>  enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder *encoder,
>  					    const struct drm_display_mode *mode);
> -enum drm_mode_status drm_connector_mode_valid(struct drm_connector *connector,
> -					      struct drm_display_mode *mode);
> +int
> +drm_connector_mode_valid(struct drm_connector *connector,
> +			 struct drm_display_mode *mode,
> +			 struct drm_modeset_acquire_ctx *ctx,
> +			 enum drm_mode_status *status);
>  
>  struct drm_encoder *
>  drm_connector_get_single_encoder(struct drm_connector *connector);
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index e0ed58d291ed9..f7bd1d35aa805 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -86,17 +86,19 @@ drm_mode_validate_flag(const struct drm_display_mode *mode,
>  	return MODE_OK;
>  }
>  
> -static enum drm_mode_status
> +static int
>  drm_mode_validate_pipeline(struct drm_display_mode *mode,
> -			    struct drm_connector *connector)
> +			   struct drm_connector *connector,
> +			   struct drm_modeset_acquire_ctx *ctx,
> +			   enum drm_mode_status *status)
>  {
>  	struct drm_device *dev = connector->dev;
> -	enum drm_mode_status ret = MODE_OK;
>  	struct drm_encoder *encoder;
> +	int ret;
>  
>  	/* Step 1: Validate against connector */
> -	ret = drm_connector_mode_valid(connector, mode);
> -	if (ret != MODE_OK)
> +	ret = drm_connector_mode_valid(connector, mode, ctx, status);
> +	if (ret || *status != MODE_OK)
>  		return ret;

After this point ret is guaranteed to stay 0, so would be clearer to
just s/return ret/return 0/ later in this func.

>  
>  	/* Step 2: Validate against encoders and crtcs */
> @@ -104,8 +106,8 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,
>  		struct drm_bridge *bridge;
>  		struct drm_crtc *crtc;
>  
> -		ret = drm_encoder_mode_valid(encoder, mode);
> -		if (ret != MODE_OK) {
> +		*status = drm_encoder_mode_valid(encoder, mode);
> +		if (*status != MODE_OK) {
>  			/* No point in continuing for crtc check as this encoder
>  			 * will not accept the mode anyway. If all encoders
>  			 * reject the mode then, at exit, ret will not be
> @@ -114,10 +116,10 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,
>  		}
>  
>  		bridge = drm_bridge_chain_get_first_bridge(encoder);
> -		ret = drm_bridge_chain_mode_valid(bridge,
> -						  &connector->display_info,
> -						  mode);
> -		if (ret != MODE_OK) {
> +		*status = drm_bridge_chain_mode_valid(bridge,
> +						      &connector->display_info,
> +						      mode);
> +		if (*status != MODE_OK) {
>  			/* There is also no point in continuing for crtc check
>  			 * here. */
>  			continue;
> @@ -127,8 +129,8 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,
>  			if (!drm_encoder_crtc_ok(encoder, crtc))
>  				continue;
>  
> -			ret = drm_crtc_mode_valid(crtc, mode);
> -			if (ret == MODE_OK) {
> +			*status = drm_crtc_mode_valid(crtc, mode);
> +			if (*status == MODE_OK) {
>  				/* If we get to this point there is at least
>  				 * one combination of encoder+crtc that works
>  				 * for this mode. Lets return now. */
> @@ -198,16 +200,26 @@ enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder *encoder,
>  	return encoder_funcs->mode_valid(encoder, mode);
>  }
>  
> -enum drm_mode_status drm_connector_mode_valid(struct drm_connector *connector,
> -					      struct drm_display_mode *mode)
> +int
> +drm_connector_mode_valid(struct drm_connector *connector,
> +			 struct drm_display_mode *mode,
> +			 struct drm_modeset_acquire_ctx *ctx,
> +			 enum drm_mode_status *status)
>  {
>  	const struct drm_connector_helper_funcs *connector_funcs =
>  		connector->helper_private;

would look a bit neater by
	int ret = 0;
and

>  
> -	if (!connector_funcs || !connector_funcs->mode_valid)
> -		return MODE_OK;
> +	if (!connector_funcs)
> +		*status = MODE_OK;
> +	else if (connector_funcs->mode_valid_ctx)
> +		return connector_funcs->mode_valid_ctx(connector, mode, ctx,
> +						       status);

		ret = ...
> +	else if (connector_funcs->mode_valid)
> +		*status = connector_funcs->mode_valid(connector, mode);
> +	else
> +		*status = MODE_OK;
>  
> -	return connector_funcs->mode_valid(connector, mode);
> +	return 0;

	return ret;

>  }
>  
>  #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
> @@ -385,8 +397,9 @@ EXPORT_SYMBOL(drm_helper_probe_detect);
>   *      (if specified)
>   *    - drm_mode_validate_flag() checks the modes against basic connector
>   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
> - *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
> - *      driver and/or sink specific checks
> + *    - the optional &drm_connector_helper_funcs.mode_valid or
> + *      &drm_connector_helper_funcs.mode_valid_ctx helpers can perform driver
> + *      and/or sink specific checks
>   *    - the optional &drm_crtc_helper_funcs.mode_valid,
>   *      &drm_bridge_funcs.mode_valid and &drm_encoder_helper_funcs.mode_valid
>   *      helpers can perform driver and/or source specific checks which are also
> @@ -517,22 +530,39 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  		mode_flags |= DRM_MODE_FLAG_3D_MASK;
>  
>  	list_for_each_entry(mode, &connector->modes, head) {
> -		if (mode->status == MODE_OK)
> -			mode->status = drm_mode_validate_driver(dev, mode);
> +		if (mode->status != MODE_OK)
> +			continue;
>  
> -		if (mode->status == MODE_OK)
> -			mode->status = drm_mode_validate_size(mode, maxX, maxY);
> +		mode->status = drm_mode_validate_driver(dev, mode);
> +		if (mode->status != MODE_OK)
> +			continue;
>  
> -		if (mode->status == MODE_OK)
> -			mode->status = drm_mode_validate_flag(mode, mode_flags);
> +		mode->status = drm_mode_validate_size(mode, maxX, maxY);
> +		if (mode->status != MODE_OK)
> +			continue;
> +
> +		mode->status = drm_mode_validate_flag(mode, mode_flags);
> +		if (mode->status != MODE_OK)
> +			continue;
>  
> -		if (mode->status == MODE_OK)
> -			mode->status = drm_mode_validate_pipeline(mode,
> -								  connector);
> +		ret = drm_mode_validate_pipeline(mode, connector, &ctx,
> +						 &mode->status);
> +		if (ret) {
> +			drm_dbg_kms(dev,
> +				    "drm_mode_validate_pipeline failed: %d\n",
> +				    ret);
> +
> +			if (drm_WARN_ON_ONCE(dev, ret != -EDEADLK)) {
> +				mode->status = MODE_ERROR;
> +			} else {
> +				drm_modeset_backoff(&ctx);
> +				goto retry;
> +			}
> +		}
>  
> -		if (mode->status == MODE_OK)
> -			mode->status = drm_mode_validate_ycbcr420(mode,
> -								  connector);
> +		if (mode->status != MODE_OK)
> +			continue;
> +		mode->status = drm_mode_validate_ycbcr420(mode, connector);
>  	}
>  
>  prune:
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 421a30f084631..4efec30f8badc 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -968,6 +968,48 @@ struct drm_connector_helper_funcs {
>  	 */
>  	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
>  					   struct drm_display_mode *mode);
> +
> +	/**
> +	 * @mode_valid_ctx:
> +	 *
> +	 * Callback to validate a mode for a connector, irrespective of the
> +	 * specific display configuration.
> +	 *
> +	 * This callback is used by the probe helpers to filter the mode list
> +	 * (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, and is the atomic version of
> +	 * &drm_connector_helper_funcs.mode_valid.
> +	 *
> +	 * To allow for accessing the atomic state of modesetting objects, the
> +	 * helper libraries always call this with ctx set to a valid context,
> +	 * and &drm_mode_config.connection_mutex will always be locked with
> +	 * the ctx parameter set to @ctx. This allows for taking additional
> +	 * locks as required.
> +	 *
> +	 * Even though additional locks may be acquired, this callback is
> +	 * still expected not to take any constraints into account which would
> +	 * be influenced by the currently set display state - such constraints
> +	 * should be handled in the driver's atomic check. For example, if a
> +	 * connector shares display bandwidth with other connectors then it
> +	 * would be ok to validate the minimum bandwidth requirement of a mode
> +	 * against the maximum possible bandwidth of the connector. But it
> +	 * wouldn't be ok to take the current bandwidth usage of other
> +	 * connectors into account, as this would change depending on the
> +	 * display state.
> +	 *
> +	 * Returns:
> +	 * 0 if &drm_connector_helper_funcs.mode_valid_ctx succeeded and wrote
> +	 * the &enum drm_mode_status value to @status, or a negative error
> +	 * code otherwise.
> +	 *
> +	 */
> +	int (*mode_valid_ctx)(struct drm_connector *connector,
> +			      struct drm_display_mode *mode,
> +			      struct drm_modeset_acquire_ctx *ctx,
> +			      enum drm_mode_status *status);
> +
>  	/**
>  	 * @best_encoder:
>  	 *
> -- 
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Powered by blists - more mailing lists