Something we've been missing for a while with drivers that support MST
is being able to prune modes that can't be set due to bandwidth
limitations. So, let's go ahead and add that. This also adds a new hook
that was needed, mode_valid_ctx, so that we can grab additional locks as
needed when validating modes.
Lee Shawn C (1):
drm/i915/mst: filter out the display mode exceed sink's capability
Lyude Paul (1):
drm/probe_helper: Add drm_connector_helper_funcs.mode_valid_ctx
drivers/gpu/drm/drm_crtc_helper_internal.h | 6 +-
drivers/gpu/drm/drm_probe_helper.c | 65 ++++++++++++++-------
drivers/gpu/drm/i915/display/intel_dp_mst.c | 39 +++++++++++--
include/drm/drm_modeset_helper_vtables.h | 41 +++++++++++++
4 files changed, 123 insertions(+), 28 deletions(-)
--
2.26.2
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.
Signed-off-by: Lyude Paul <[email protected]>
Cc: Lee Shawn C <[email protected]>
Tested-by: Lee Shawn C <[email protected]>
---
drivers/gpu/drm/drm_crtc_helper_internal.h | 6 +-
drivers/gpu/drm/drm_probe_helper.c | 65 ++++++++++++++--------
include/drm/drm_modeset_helper_vtables.h | 41 ++++++++++++++
3 files changed, 88 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h b/drivers/gpu/drm/drm_crtc_helper_internal.h
index f0a66ef47e5ad..ca767cba6094d 100644
--- a/drivers/gpu/drm/drm_crtc_helper_internal.h
+++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
@@ -73,8 +73,10 @@ 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);
+enum drm_mode_status
+drm_connector_mode_valid(struct drm_connector *connector,
+ struct drm_display_mode *mode,
+ struct drm_modeset_acquire_ctx *ctx);
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 466dfbba82564..3132784736841 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -86,16 +86,17 @@ 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)
{
struct drm_device *dev = connector->dev;
- enum drm_mode_status ret = MODE_OK;
struct drm_encoder *encoder;
+ int ret = MODE_OK;
/* Step 1: Validate against connector */
- ret = drm_connector_mode_valid(connector, mode);
+ ret = drm_connector_mode_valid(connector, mode, ctx);
if (ret != MODE_OK)
return ret;
@@ -196,16 +197,23 @@ 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)
{
const struct drm_connector_helper_funcs *connector_funcs =
connector->helper_private;
- if (!connector_funcs || !connector_funcs->mode_valid)
+ if (!connector_funcs)
return MODE_OK;
- return connector_funcs->mode_valid(connector, mode);
+ if (connector_funcs->mode_valid_ctx)
+ return connector_funcs->mode_valid_ctx(connector, mode, ctx);
+ else if (connector_funcs->mode_valid)
+ return connector_funcs->mode_valid(connector, mode);
+ else
+ return MODE_OK;
}
#define DRM_OUTPUT_POLL_PERIOD (10*HZ)
@@ -375,8 +383,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
@@ -507,22 +516,34 @@ 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;
+
+ 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_size(mode, maxX, maxY);
+ 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_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);
+ if (ret == -EDEADLK) {
+ drm_modeset_backoff(&ctx);
+ goto retry;
+ } else if (WARN_ON_ONCE(ret < 0)) {
+ mode->status = MODE_BAD;
+ } else {
+ mode->status = ret;
+ }
- 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..8f020c3424b2b 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -968,6 +968,47 @@ 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_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 a mode uses 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:
+ *
+ * Either &drm_mode_status.MODE_OK, one of the failure reasons in
+ * &enum drm_mode_status, or -EDEADLK if a deadlock would have
+ * occurred and we need to backoff.
+ *
+ */
+ int (*mode_valid_ctx)(struct drm_connector *connector,
+ struct drm_display_mode *mode,
+ struct drm_modeset_acquire_ctx *ctx);
+
/**
* @best_encoder:
*
--
2.26.2
Sorry for the delay, the review as I promised:
On Tue, May 26, 2020 at 02:23:09PM -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.
>
> Signed-off-by: Lyude Paul <[email protected]>
> Cc: Lee Shawn C <[email protected]>
> Tested-by: Lee Shawn C <[email protected]>
> ---
> drivers/gpu/drm/drm_crtc_helper_internal.h | 6 +-
> drivers/gpu/drm/drm_probe_helper.c | 65 ++++++++++++++--------
> include/drm/drm_modeset_helper_vtables.h | 41 ++++++++++++++
> 3 files changed, 88 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h b/drivers/gpu/drm/drm_crtc_helper_internal.h
> index f0a66ef47e5ad..ca767cba6094d 100644
> --- a/drivers/gpu/drm/drm_crtc_helper_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
> @@ -73,8 +73,10 @@ 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);
> +enum drm_mode_status
> +drm_connector_mode_valid(struct drm_connector *connector,
> + struct drm_display_mode *mode,
> + struct drm_modeset_acquire_ctx *ctx);
>
> 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 466dfbba82564..3132784736841 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -86,16 +86,17 @@ 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)
> {
> struct drm_device *dev = connector->dev;
> - enum drm_mode_status ret = MODE_OK;
> struct drm_encoder *encoder;
> + int ret = MODE_OK;
>
> /* Step 1: Validate against connector */
> - ret = drm_connector_mode_valid(connector, mode);
> + ret = drm_connector_mode_valid(connector, mode, ctx);
> if (ret != MODE_OK)
> return ret;
>
> @@ -196,16 +197,23 @@ 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)
> {
> const struct drm_connector_helper_funcs *connector_funcs =
> connector->helper_private;
>
> - if (!connector_funcs || !connector_funcs->mode_valid)
> + if (!connector_funcs)
> return MODE_OK;
>
> - return connector_funcs->mode_valid(connector, mode);
> + if (connector_funcs->mode_valid_ctx)
> + return connector_funcs->mode_valid_ctx(connector, mode, ctx);
> + else if (connector_funcs->mode_valid)
> + return connector_funcs->mode_valid(connector, mode);
> + else
> + return MODE_OK;
> }
>
> #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
> @@ -375,8 +383,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
> @@ -507,22 +516,34 @@ 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;
> +
> + 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_size(mode, maxX, maxY);
> + 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_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);
> + if (ret == -EDEADLK) {
> + drm_modeset_backoff(&ctx);
> + goto retry;
> + } else if (WARN_ON_ONCE(ret < 0)) {
> + mode->status = MODE_BAD;
> + } else {
> + mode->status = ret;
> + }
>
> - 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..8f020c3424b2b 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -968,6 +968,47 @@ 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_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 a mode uses against the maximum possible
^mode against?
Reviewed-by: Imre Deak <[email protected]>
> + * 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:
> + *
> + * Either &drm_mode_status.MODE_OK, one of the failure reasons in
> + * &enum drm_mode_status, or -EDEADLK if a deadlock would have
> + * occurred and we need to backoff.
> + *
> + */
> + int (*mode_valid_ctx)(struct drm_connector *connector,
> + struct drm_display_mode *mode,
> + struct drm_modeset_acquire_ctx *ctx);
> +
> /**
> * @best_encoder:
> *
> --
> 2.26.2
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
JFYI - found an issue with this patch that wouldn't have shown up on i915, info
down below:
On Wed, 2020-07-08 at 01:40 +0300, Imre Deak wrote:
> Sorry for the delay, the review as I promised:
>
> On Tue, May 26, 2020 at 02:23:09PM -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.
> >
> > Signed-off-by: Lyude Paul <[email protected]>
> > Cc: Lee Shawn C <[email protected]>
> > Tested-by: Lee Shawn C <[email protected]>
> > ---
> > drivers/gpu/drm/drm_crtc_helper_internal.h | 6 +-
> > drivers/gpu/drm/drm_probe_helper.c | 65 ++++++++++++++--------
> > include/drm/drm_modeset_helper_vtables.h | 41 ++++++++++++++
> > 3 files changed, 88 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h
> > b/drivers/gpu/drm/drm_crtc_helper_internal.h
> > index f0a66ef47e5ad..ca767cba6094d 100644
> > --- a/drivers/gpu/drm/drm_crtc_helper_internal.h
> > +++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
> > @@ -73,8 +73,10 @@ 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);
> > +enum drm_mode_status
> > +drm_connector_mode_valid(struct drm_connector *connector,
> > + struct drm_display_mode *mode,
> > + struct drm_modeset_acquire_ctx *ctx);
> >
> > 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 466dfbba82564..3132784736841 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -86,16 +86,17 @@ 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)
> > {
> > struct drm_device *dev = connector->dev;
> > - enum drm_mode_status ret = MODE_OK;
> > struct drm_encoder *encoder;
> > + int ret = MODE_OK;
> >
> > /* Step 1: Validate against connector */
> > - ret = drm_connector_mode_valid(connector, mode);
> > + ret = drm_connector_mode_valid(connector, mode, ctx);
> > if (ret != MODE_OK)
> > return ret;
> >
> > @@ -196,16 +197,23 @@ 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)
> > {
> > const struct drm_connector_helper_funcs *connector_funcs =
> > connector->helper_private;
> >
> > - if (!connector_funcs || !connector_funcs->mode_valid)
> > + if (!connector_funcs)
> > return MODE_OK;
> >
> > - return connector_funcs->mode_valid(connector, mode);
> > + if (connector_funcs->mode_valid_ctx)
> > + return connector_funcs->mode_valid_ctx(connector, mode, ctx);
> > + else if (connector_funcs->mode_valid)
> > + return connector_funcs->mode_valid(connector, mode);
> > + else
> > + return MODE_OK;
> > }
> >
> > #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
> > @@ -375,8 +383,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
> > @@ -507,22 +516,34 @@ 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;
> > +
> > + 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_size(mode, maxX, maxY);
> > + 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_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);
> > + if (ret == -EDEADLK) {
> > + drm_modeset_backoff(&ctx);
> > + goto retry;
> > + } else if (WARN_ON_ONCE(ret < 0)) {
> > + mode->status = MODE_BAD;
This check is wrong actually. We define negative values for drm_mode_status
(MODE_BAD, MODE_ERROR, MODE_STALE) which, at least with how drivers currently
seem to use them, are something we want to treat as not-unexpected errors and
not WARN_ON.
This is a bit annoying because it does mean there's some overlap between
drm_mode_status and some legitimate errno values (EPERM, ENOENT, ESRCH). Luckily
I can't see any reason why drivers would want to return those, but I think we
should probably print a debugging message when we get any errno values just so
developers don't get confused (also going to add a IS_ERR() equivalent for
drm_mode_status, but with a different name)
> > + } else {
> > + mode->status = ret;
> > + }
> >
> > - 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..8f020c3424b2b 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -968,6 +968,47 @@ 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_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 a mode uses against the maximum possible
> ^mode against?
>
> Reviewed-by: Imre Deak <[email protected]>
>
> > + * 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:
> > + *
> > + * Either &drm_mode_status.MODE_OK, one of the failure reasons in
> > + * &enum drm_mode_status, or -EDEADLK if a deadlock would have
> > + * occurred and we need to backoff.
> > + *
> > + */
> > + int (*mode_valid_ctx)(struct drm_connector *connector,
> > + struct drm_display_mode *mode,
> > + struct drm_modeset_acquire_ctx *ctx);
> > +
> > /**
> > * @best_encoder:
> > *
> > --
> > 2.26.2
> >
> > _______________________________________________
> > dri-devel mailing list
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Jul 08, 2020 at 01:25:14PM -0400, Lyude Paul wrote:
>
> JFYI - found an issue with this patch that wouldn't have shown up on i915, info
> down below:
>
> On Wed, 2020-07-08 at 01:40 +0300, Imre Deak wrote:
> > Sorry for the delay, the review as I promised:
> >
> > On Tue, May 26, 2020 at 02:23:09PM -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.
> > >
> > > Signed-off-by: Lyude Paul <[email protected]>
> > > Cc: Lee Shawn C <[email protected]>
> > > Tested-by: Lee Shawn C <[email protected]>
> > > ---
> > > drivers/gpu/drm/drm_crtc_helper_internal.h | 6 +-
> > > drivers/gpu/drm/drm_probe_helper.c | 65 ++++++++++++++--------
> > > include/drm/drm_modeset_helper_vtables.h | 41 ++++++++++++++
> > > 3 files changed, 88 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > b/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > index f0a66ef47e5ad..ca767cba6094d 100644
> > > --- a/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > +++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > @@ -73,8 +73,10 @@ 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);
> > > +enum drm_mode_status
> > > +drm_connector_mode_valid(struct drm_connector *connector,
> > > + struct drm_display_mode *mode,
> > > + struct drm_modeset_acquire_ctx *ctx);
> > >
> > > 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 466dfbba82564..3132784736841 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -86,16 +86,17 @@ 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)
> > > {
> > > struct drm_device *dev = connector->dev;
> > > - enum drm_mode_status ret = MODE_OK;
> > > struct drm_encoder *encoder;
> > > + int ret = MODE_OK;
> > >
> > > /* Step 1: Validate against connector */
> > > - ret = drm_connector_mode_valid(connector, mode);
> > > + ret = drm_connector_mode_valid(connector, mode, ctx);
> > > if (ret != MODE_OK)
> > > return ret;
> > >
> > > @@ -196,16 +197,23 @@ 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)
> > > {
> > > const struct drm_connector_helper_funcs *connector_funcs =
> > > connector->helper_private;
> > >
> > > - if (!connector_funcs || !connector_funcs->mode_valid)
> > > + if (!connector_funcs)
> > > return MODE_OK;
> > >
> > > - return connector_funcs->mode_valid(connector, mode);
> > > + if (connector_funcs->mode_valid_ctx)
> > > + return connector_funcs->mode_valid_ctx(connector, mode, ctx);
> > > + else if (connector_funcs->mode_valid)
> > > + return connector_funcs->mode_valid(connector, mode);
> > > + else
> > > + return MODE_OK;
> > > }
> > >
> > > #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
> > > @@ -375,8 +383,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
> > > @@ -507,22 +516,34 @@ 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;
> > > +
> > > + 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_size(mode, maxX, maxY);
> > > + 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_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);
> > > + if (ret == -EDEADLK) {
> > > + drm_modeset_backoff(&ctx);
> > > + goto retry;
> > > + } else if (WARN_ON_ONCE(ret < 0)) {
> > > + mode->status = MODE_BAD;
>
> This check is wrong actually. We define negative values for drm_mode_status
> (MODE_BAD, MODE_ERROR, MODE_STALE) which, at least with how drivers currently
> seem to use them, are something we want to treat as not-unexpected errors and
> not WARN_ON.
Ah, yes, missed that.
> This is a bit annoying because it does mean there's some overlap between
> drm_mode_status and some legitimate errno values (EPERM, ENOENT, ESRCH). Luckily
> I can't see any reason why drivers would want to return those, but I think we
> should probably print a debugging message when we get any errno values just so
> developers don't get confused (also going to add a IS_ERR() equivalent for
> drm_mode_status, but with a different name)
Maybe I'm missing something, but having negative values in
drm_mode_status doesn't make sense to me, as all the non-zero values
there just mean the mode is incorrect for some reason and should be
pruned.
Would it make sense to better separate the mode-status value and any
error return value from mode_valid_ctx(), by adding a drm_mode_status *
parameter to the hook and drm_mode_validate_pipeline()?
>
> > > + } else {
> > > + mode->status = ret;
> > > + }
> > >
> > > - 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..8f020c3424b2b 100644
> > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > @@ -968,6 +968,47 @@ 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_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 a mode uses against the maximum possible
> > ^mode against?
> >
> > Reviewed-by: Imre Deak <[email protected]>
> >
> > > + * 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:
> > > + *
> > > + * Either &drm_mode_status.MODE_OK, one of the failure reasons in
> > > + * &enum drm_mode_status, or -EDEADLK if a deadlock would have
> > > + * occurred and we need to backoff.
> > > + *
> > > + */
> > > + int (*mode_valid_ctx)(struct drm_connector *connector,
> > > + struct drm_display_mode *mode,
> > > + struct drm_modeset_acquire_ctx *ctx);
> > > +
> > > /**
> > > * @best_encoder:
> > > *
> > > --
> > > 2.26.2
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > [email protected]
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
On Wed, 2020-07-08 at 21:45 +0300, Imre Deak wrote:
> On Wed, Jul 08, 2020 at 01:25:14PM -0400, Lyude Paul wrote:
> > JFYI - found an issue with this patch that wouldn't have shown up on i915,
> > info
> > down below:
> >
> > On Wed, 2020-07-08 at 01:40 +0300, Imre Deak wrote:
> > > Sorry for the delay, the review as I promised:
> > >
> > > On Tue, May 26, 2020 at 02:23:09PM -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.
> > > >
> > > > Signed-off-by: Lyude Paul <[email protected]>
> > > > Cc: Lee Shawn C <[email protected]>
> > > > Tested-by: Lee Shawn C <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/drm_crtc_helper_internal.h | 6 +-
> > > > drivers/gpu/drm/drm_probe_helper.c | 65 ++++++++++++++--------
> > > > include/drm/drm_modeset_helper_vtables.h | 41 ++++++++++++++
> > > > 3 files changed, 88 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > > b/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > > index f0a66ef47e5ad..ca767cba6094d 100644
> > > > --- a/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > > +++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
> > > > @@ -73,8 +73,10 @@ 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);
> > > > +enum drm_mode_status
> > > > +drm_connector_mode_valid(struct drm_connector *connector,
> > > > + struct drm_display_mode *mode,
> > > > + struct drm_modeset_acquire_ctx *ctx);
> > > >
> > > > 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 466dfbba82564..3132784736841 100644
> > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > @@ -86,16 +86,17 @@ 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)
> > > > {
> > > > struct drm_device *dev = connector->dev;
> > > > - enum drm_mode_status ret = MODE_OK;
> > > > struct drm_encoder *encoder;
> > > > + int ret = MODE_OK;
> > > >
> > > > /* Step 1: Validate against connector */
> > > > - ret = drm_connector_mode_valid(connector, mode);
> > > > + ret = drm_connector_mode_valid(connector, mode, ctx);
> > > > if (ret != MODE_OK)
> > > > return ret;
> > > >
> > > > @@ -196,16 +197,23 @@ 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)
> > > > {
> > > > const struct drm_connector_helper_funcs *connector_funcs =
> > > > connector->helper_private;
> > > >
> > > > - if (!connector_funcs || !connector_funcs->mode_valid)
> > > > + if (!connector_funcs)
> > > > return MODE_OK;
> > > >
> > > > - return connector_funcs->mode_valid(connector, mode);
> > > > + if (connector_funcs->mode_valid_ctx)
> > > > + return connector_funcs->mode_valid_ctx(connector, mode,
> > > > ctx);
> > > > + else if (connector_funcs->mode_valid)
> > > > + return connector_funcs->mode_valid(connector, mode);
> > > > + else
> > > > + return MODE_OK;
> > > > }
> > > >
> > > > #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
> > > > @@ -375,8 +383,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
> > > > @@ -507,22 +516,34 @@ 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;
> > > > +
> > > > + 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_size(mode, maxX, maxY);
> > > > + 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_flag(mode, mode_flags);
> > > > + if (mode->status != MODE_OK)
> > > > + continue;
> > > >
> > > > - if (mode->status == MODE_OK)
> > > > - mode->status = drm_mode_validate_pipeline(mode,
> > > > - connec
> > > > tor);
> > > > + ret = drm_mode_validate_pipeline(mode, connector, &ctx);
> > > > + if (ret == -EDEADLK) {
> > > > + drm_modeset_backoff(&ctx);
> > > > + goto retry;
> > > > + } else if (WARN_ON_ONCE(ret < 0)) {
> > > > + mode->status = MODE_BAD;
> >
> > This check is wrong actually. We define negative values for drm_mode_status
> > (MODE_BAD, MODE_ERROR, MODE_STALE) which, at least with how drivers
> > currently
> > seem to use them, are something we want to treat as not-unexpected errors
> > and
> > not WARN_ON.
>
> Ah, yes, missed that.
>
> > This is a bit annoying because it does mean there's some overlap between
> > drm_mode_status and some legitimate errno values (EPERM, ENOENT, ESRCH).
> > Luckily
> > I can't see any reason why drivers would want to return those, but I think
> > we
> > should probably print a debugging message when we get any errno values just
> > so
> > developers don't get confused (also going to add a IS_ERR() equivalent for
> > drm_mode_status, but with a different name)
>
> Maybe I'm missing something, but having negative values in
> drm_mode_status doesn't make sense to me, as all the non-zero values
> there just mean the mode is incorrect for some reason and should be
> pruned.
>
> Would it make sense to better separate the mode-status value and any
> error return value from mode_valid_ctx(), by adding a drm_mode_status *
> parameter to the hook and drm_mode_validate_pipeline()?
yeah-that would probably make more sense. I will go ahead and do that
>
> > > > + } else {
> > > > + mode->status = ret;
> > > > + }
> > > >
> > > > - if (mode->status == MODE_OK)
> > > > - mode->status = drm_mode_validate_ycbcr420(mode,
> > > > - connec
> > > > tor);
> > > > + 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..8f020c3424b2b 100644
> > > > --- a/include/drm/drm_modeset_helper_vtables.h
> > > > +++ b/include/drm/drm_modeset_helper_vtables.h
> > > > @@ -968,6 +968,47 @@ 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_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 a mode uses against the maximum
> > > > possible
> > > ^mode against?
> > >
> > > Reviewed-by: Imre Deak <[email protected]>
> > >
> > > > + * 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:
> > > > + *
> > > > + * Either &drm_mode_status.MODE_OK, one of the failure reasons
> > > > in
> > > > + * &enum drm_mode_status, or -EDEADLK if a deadlock would have
> > > > + * occurred and we need to backoff.
> > > > + *
> > > > + */
> > > > + int (*mode_valid_ctx)(struct drm_connector *connector,
> > > > + struct drm_display_mode *mode,
> > > > + struct drm_modeset_acquire_ctx *ctx);
> > > > +
> > > > /**
> > > > * @best_encoder:
> > > > *
> > > > --
> > > > 2.26.2
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > [email protected]
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel