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 <[email protected]>
Cc: Lee Shawn C <[email protected]>
---
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;
/* 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;
- 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);
+ 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;
}
#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
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 <[email protected]>
> Cc: Lee Shawn C <[email protected]>
Some nits below, but looks good in either way:
Reviewed-and-tested-by: Imre Deak <[email protected]>
> ---
> 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx