Introduce new mode validation callbacks

PATCH Superseded Jose Abreu 5 participants 35 messages May 11, 2017 – May 16, 2017

Subsystems (3)

DRM drivers DRM misc DRM drivers for bridge chips

lore

Share & tools

lore

Jose Abreu

author

2017-05-11 09:05 UTC #0 lore raw

v3 cover Introduce new mode validation callbacks

This series is a follow up from the discussion at [1]. We start by introducing crtc->mode_valid(), encoder->mode_valid() and bridge->mode_valid() callbacks which will be used in followup patches and also by cleaning the documentation a little bit.

We proceed by introducing new helpers to call this new callbacks at 2/6.

At 3/6 a helper function is introduced that calls all mode_valid() from a set of bridges.

Next, at 4/6 we modify the connector probe helper so that only modes which are supported by a given bridge+encoder+crtc combination are probbed.

At 5/6 we call all the mode_valid() callbacks for a given pipeline, except the connector->mode_valid one, so that the mode is validated. This is done before calling mode_fixup().

Finally, at 6/6 we use the new crtc->mode_valid() callback in arcpgu and remove the atomic_check() callback.

[1] https://patchwork.kernel.org/patch/9702233/

Jose Abreu (6):
drm: Add crtc/encoder/bridge->mode_valid() callbacks
drm: Add drm_{crtc/encoder/connector}_mode_valid()
drm: Introduce drm_bridge_mode_valid()
drm: Use new mode_valid() helpers in connector probe helper
drm: Use mode_valid() in atomic modeset
drm: arc: Use crtc->mode_valid() callback

Cc Carlos Palminha <[email protected]>

Cc Alexey Brodkin <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Andrzej Hajda <[email protected]>

Cc Archit Taneja <[email protected]>

 drivers/gpu/drm/arc/arcpgu_crtc.c          |  39 ++++++----
 drivers/gpu/drm/drm_atomic_helper.c        |  76 +++++++++++++++++++-
 drivers/gpu/drm/drm_bridge.c               |  33 +++++++++
 drivers/gpu/drm/drm_crtc_helper_internal.h |  13 ++++
 drivers/gpu/drm/drm_probe_helper.c         | 103 ++++++++++++++++++++++++++-
 include/drm/drm_bridge.h                   |  22 ++++++
 include/drm/drm_modeset_helper_vtables.h   | 110 ++++++++++++++++++++++-------
 7 files changed, 348 insertions(+), 48 deletions(-)

Signature · 1 line

1.9.1

Jose Abreu

author

2017-05-11 09:05 UTC #1 lore raw

PATCH v3 1/6 drm: Add crtc/encoder/bridge->mode_valid() callbacks

This adds a new callback to crtc, encoder and bridge helper functions called mode_valid(). This callback shall be implemented if the corresponding component has some sort of restriction in the modes that can be displayed. A NULL callback implicates that the component can display all the modes.

We also change the documentation so that the new and old callbacks are correctly documented.

Only the callbacks were implemented to simplify review process, following patches will make use of them.

Signed-off-by Jose Abreu <[email protected]>

Cc Carlos Palminha <[email protected]>

Cc Alexey Brodkin <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Andrzej Hajda <[email protected]>

Cc Archit Taneja <[email protected]>

---

Changes v2->v3:
- Try to document the callbacks a little bit better and review current documentation (Daniel)
Changes v1->v2:
- Change description of connector->mode_valid() (Daniel)

 include/drm/drm_bridge.h                 |  20 ++++++
 include/drm/drm_modeset_helper_vtables.h | 110 +++++++++++++++++++++++--------
 2 files changed, 103 insertions(+), 27 deletions(-)

include/drm/drm_bridge.h +20 -0

index fdd82fc..00c6c36 100644
@@ -59,6 +59,26 @@ struct drm_bridge_funcs {
 	void (*detach)(struct drm_bridge *bridge);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * bridge. This should be implemented if the bridge has some sort of
+	 * restriction in the modes it can display. For example, a given bridge
+	 * 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This is called at mode probe and at atomic check phase.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc,
+					   const struct drm_display_mode *mode);
+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate and adjust a mode. The paramater

include/drm/drm_modeset_helper_vtables.h +83 ~~-27~~

157 lines not on this page · raw message

Signature · 1 line

1.9.1

Jose Abreu

author

2017-05-11 09:05 UTC #19 lore raw

PATCH v3 2/6 drm: Add drm_{crtc/encoder/connector}_mode_valid()

In reply to Jose Abreu

Add a new helper to call crtc->mode_valid, connector->mode_valid and encoder->mode_valid callbacks.

Signed-off-by Jose Abreu <[email protected]>

Cc Carlos Palminha <[email protected]>

Cc Alexey Brodkin <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Andrzej Hajda <[email protected]>

Cc Archit Taneja <[email protected]>

---

Changes v2->v3:
- Move helpers to drm_probe_helper.c (Daniel)
- Squeeze patches that introduce the helpers into a single one (Daniel)

 drivers/gpu/drm/drm_crtc_helper_internal.h | 13 ++++++++++
 drivers/gpu/drm/drm_probe_helper.c         | 38 ++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

drivers/gpu/drm/drm_crtc_helper_internal.h +13 -0

index 28295e5..97dfe20 100644
@@ -26,7 +26,11 @@
  * implementation details and are not exported to drivers.
  */
 
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
 #include <drm/drm_dp_helper.h>
+#include <drm/drm_encoder.h>
+#include <drm/drm_modes.h>
 
 /* drm_fb_helper.c */
 #ifdef CONFIG_DRM_FBDEV_EMULATION

@@ -62,4 +66,13 @@ static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
 static inline void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux)
 {
 }
+
+/* drm_probe_helper.c */
+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);
+
 #endif

drivers/gpu/drm/drm_probe_helper.c +38 -0

index 1b0c14a..f01abdc 100644
@@ -38,6 +38,9 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_modeset_helper_vtables.h>
+
+#include "drm_crtc_helper_internal.h"
 
 /**
  * DOC: output probing helper overview

@@ -113,6 +116,41 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
 	return 1;
 }
 
+enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
+					 const struct drm_display_mode *mode)
+{
+	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
+
+	if (!crtc_funcs || !crtc_funcs->mode_valid)
+		return MODE_OK;
+
+	return crtc_funcs->mode_valid(crtc, mode);
+}
+
+enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder *encoder,
+					    const struct drm_display_mode *mode)
+{
+	const struct drm_encoder_helper_funcs *encoder_funcs =
+		encoder->helper_private;
+
+	if (!encoder_funcs || !encoder_funcs->mode_valid)
+		return MODE_OK;
+
+	return encoder_funcs->mode_valid(encoder, mode);
+}
+
+enum drm_mode_status drm_connector_mode_valid(struct drm_connector *connector,
+					      struct drm_display_mode *mode)
+{
+	const struct drm_connector_helper_funcs *connector_funcs =
+		connector->helper_private;
+
+	if (!connector_funcs || !connector_funcs->mode_valid)
+		return MODE_OK;
+
+	return connector_funcs->mode_valid(connector, mode);
+}
+
 #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
 /**
  * drm_kms_helper_poll_enable - re-enable output polling.

Signature · 1 line

1.9.1

Jose Abreu

author

2017-05-11 09:05 UTC #21 lore raw

PATCH v3 3/6 drm: Introduce drm_bridge_mode_valid()

In reply to Jose Abreu

Introduce a new helper function which calls mode_valid() callback for all bridges in an encoder chain.

Signed-off-by Jose Abreu <[email protected]>

Cc Carlos Palminha <[email protected]>

Cc Alexey Brodkin <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Andrzej Hajda <[email protected]>

Cc Archit Taneja <[email protected]>

---
 drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h     |  2 ++
 2 files changed, 35 insertions(+)

drivers/gpu/drm/drm_bridge.c +33 -0

index 86a7637..dc8cdfe 100644
@@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
 EXPORT_SYMBOL(drm_bridge_mode_fixup);
 
 /**
+ * drm_bridge_mode_valid - validate the mode against all bridges in the
+ * 			   encoder chain.
+ * @bridge: bridge control structure
+ * @mode: desired mode to be validated
+ *
+ * Calls &drm_bridge_funcs.mode_valid for all the bridges in the encoder
+ * chain, starting from the first bridge to the last. If at least one bridge
+ * does not accept the mode the function returns the error code.
+ *
+ * Note: the bridge passed should be the one closest to the encoder.
+ *
+ * RETURNS:
+ * MODE_OK on success, drm_mode_status Enum error code on failure
+ */
+enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
+					   const struct drm_display_mode *mode)
+{
+	enum drm_mode_status ret = MODE_OK;
+
+	if (!bridge)
+		return ret;
+
+	if (bridge->funcs->mode_valid)
+		ret = bridge->funcs->mode_valid(bridge, mode);
+
+	if (ret != MODE_OK)
+		return ret;
+
+	return drm_bridge_mode_valid(bridge->next, mode);
+}
+EXPORT_SYMBOL(drm_bridge_mode_valid);
+
+/**
  * drm_bridge_disable - disables all bridges in the encoder chain
  * @bridge: bridge control structure
  *

include/drm/drm_bridge.h +2 -0

index 00c6c36..8358eb3 100644
@@ -233,6 +233,8 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
 			const struct drm_display_mode *mode,
 			struct drm_display_mode *adjusted_mode);
+enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
+					   const struct drm_display_mode *mode);
 void drm_bridge_disable(struct drm_bridge *bridge);
 void drm_bridge_post_disable(struct drm_bridge *bridge);
 void drm_bridge_mode_set(struct drm_bridge *bridge,

Signature · 1 line

1.9.1

Jose Abreu

author

2017-05-11 09:06 UTC #22 lore raw

PATCH v3 4/6 drm: Use new mode_valid() helpers in connector probe helper

In reply to Jose Abreu

This changes the connector probe helper function to use the new encoder->mode_valid(), bridge->mode_valid() and crtc->mode_valid() helper callbacks to validate the modes.

The new callbacks are optional so the behaviour remains the same if they are not implemented. If they are, then the code loops through all the connector's encodersXbridgesXcrtcs and calls the callback.

If at least a valid encoderXbridgeXcrtc combination is found which accepts the mode then the function returns MODE_OK.

Signed-off-by Jose Abreu <[email protected]>

Cc Carlos Palminha <[email protected]>

Cc Alexey Brodkin <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Andrzej Hajda <[email protected]>

Cc Archit Taneja <[email protected]>

---

Changes v2->v3:
- Call also bridge->mode_valid (Daniel)
Changes v1->v2:
- Use new helpers suggested by Ville
- Change documentation (Daniel)

 drivers/gpu/drm/drm_probe_helper.c | 65 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 3 deletions(-)

drivers/gpu/drm/drm_probe_helper.c +62 -3

index f01abdc..84d660e 100644
@@ -83,6 +83,61 @@
 	return MODE_OK;
 }
 
+static enum drm_mode_status
+drm_mode_validate_connector(struct drm_connector *connector,
+			    struct drm_display_mode *mode)
+{
+	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 */
+	ret = drm_connector_mode_valid(connector, mode);
+	if (ret != MODE_OK)
+		return ret;
+
+	/* Step 2: Validate against encoders and crtcs */
+	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;
+
+		ret = drm_encoder_mode_valid(encoder, mode);
+		if (ret != 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
+			 * MODE_OK. */
+			continue;
+		}
+
+		ret = drm_bridge_mode_valid(encoder->bridge, mode);
+		if (ret != MODE_OK) {
+			/* There is also no point in continuing for crtc check
+			 * here. */
+			continue;
+		}
+
+		drm_for_each_crtc(crtc, dev) {
+			if (!drm_encoder_crtc_ok(encoder, crtc))
+				continue;
+
+			ret = drm_crtc_mode_valid(crtc, mode);
+			if (ret == 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. */
+				return ret;
+			}
+		}
+	}
+
+	return ret;
+}
+
 static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
 {
 	struct drm_cmdline_mode *cmdline_mode;

@@ -322,7 +377,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
  *    - 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 hardware specific checks
+ *      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
+ *      enforced by the modeset/atomic helpers
  *
  * 5. Any mode whose status is not OK is pruned from the connector's modes list,
  *    accompanied by a debug message indicating the reason for the mode's

@@ -466,8 +525,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);
 	}

Signature · 1 line

1.9.1

Jose Abreu

author

2017-05-11 09:06 UTC #26 lore raw

PATCH v3 5/6 drm: Use mode_valid() in atomic modeset

In reply to Jose Abreu

This patches makes use of the new mode_valid() callbacks introduced previously to validate the full video pipeline when modesetting.

This calls the encoder->mode_valid(), bridge->mode_valid() and crtc->mode_valid() so that we can make sure that the mode will be accepted in every components.

Signed-off-by Jose Abreu <[email protected]>

Cc Carlos Palminha <[email protected]>

Cc Alexey Brodkin <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Andrzej Hajda <[email protected]>

Cc Archit Taneja <[email protected]>

---

Changes v1->v2:
- Removed call to connector->mode_valid (Ville, Daniel)
- Change function name (Ville)
- Use for_each_new_connector_in_state (Ville)
- Do not validate if connector and mode didn't change (Ville)
- Use new helpers to call mode_valid

 drivers/gpu/drm/drm_atomic_helper.c | 76 +++++++++++++++++++++++++++++++++++--
 1 file changed, 73 insertions(+), 3 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +73 -3

index 8be9719..5a3c458 100644
@@ -32,6 +32,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <linux/dma-fence.h>
 
+#include "drm_crtc_helper_internal.h"
 #include "drm_crtc_internal.h"
 
 /**

@@ -452,6 +453,69 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 	return 0;
 }
 
+static enum drm_mode_status mode_valid_path(struct drm_connector *connector,
+					    struct drm_encoder *encoder,
+					    struct drm_crtc *crtc,
+					    struct drm_display_mode *mode)
+{
+	enum drm_mode_status ret;
+
+	ret = drm_encoder_mode_valid(encoder, mode);
+	if (ret != MODE_OK) {
+		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] mode_valid() failed\n",
+				encoder->base.id, encoder->name);
+		return ret;
+	}
+
+	ret = drm_bridge_mode_valid(encoder->bridge, mode);
+	if (ret != MODE_OK) {
+		DRM_DEBUG_ATOMIC("[BRIDGE] mode_valid() failed\n");
+		return ret;
+	}
+
+	ret = drm_crtc_mode_valid(crtc, mode);
+	if (ret != MODE_OK) {
+		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode_valid() failed\n",
+				crtc->base.id, crtc->name);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int
+mode_valid(struct drm_atomic_state *state)
+{
+	struct drm_connector_state *conn_state;
+	struct drm_connector *connector;
+	int i;
+
+	for_each_new_connector_in_state(state, connector, conn_state, i) {
+		struct drm_encoder *encoder = conn_state->best_encoder;
+		struct drm_crtc *crtc = conn_state->crtc;
+		struct drm_crtc_state *crtc_state;
+		enum drm_mode_status mode_status;
+		struct drm_display_mode *mode;
+
+		if (!crtc || !encoder)
+			continue;
+
+		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+		if (!crtc_state)
+			continue;
+		if (!crtc_state->mode_changed && !crtc_state->connectors_changed)
+			continue;
+
+		mode = &crtc_state->mode;
+
+		mode_status = mode_valid_path(connector, encoder, crtc, mode);
+		if (mode_status != MODE_OK)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 /**
  * drm_atomic_helper_check_modeset - validate state object for modeset changes
  * @dev: DRM device

@@ -466,13 +530,15 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
  * 2. &drm_connector_helper_funcs.atomic_check to validate the connector state.
  * 3. If it's determined a modeset is needed then all connectors on the affected crtc
  *    crtc are added and &drm_connector_helper_funcs.atomic_check is run on them.
- * 4. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
- * 5. &drm_encoder_helper_funcs.atomic_check is called to validate any encoder state.
+ * 4. &drm_encoder_helper_funcs.mode_valid, &drm_bridge_funcs.mode_valid and
+ *    &drm_crtc_helper_funcs.mode_valid are called on the affected components.
+ * 5. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
+ * 6. &drm_encoder_helper_funcs.atomic_check is called to validate any encoder state.
  *    This function is only called when the encoder will be part of a configured crtc,
  *    it must not be used for implementing connector property validation.
  *    If this function is NULL, &drm_atomic_encoder_helper_funcs.mode_fixup is called
  *    instead.
- * 6. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints.
+ * 7. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints.
  *
  * &drm_crtc_state.mode_changed is set when the input mode is changed.
  * &drm_crtc_state.connectors_changed is set when a connector is added or

@@ -617,6 +683,10 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 			return ret;
 	}
 
+	ret = mode_valid(state);
+	if (ret)
+		return ret;
+
 	return mode_fixup(state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_check_modeset);

Signature · 1 line

1.9.1

Jose Abreu

author

2017-05-11 09:06 UTC #28 lore raw

PATCH v3 6/6 drm: arc: Use crtc->mode_valid() callback

In reply to Jose Abreu

Now that we have a callback to check if crtc supports a given mode we can use it in arcpgu so that we restrict the number of probbed modes to the ones we can actually display.

This is specially useful because arcpgu crtc is responsible to set a clock value in the commit() stage but unfortunatelly this clock does not support all the needed ranges.

Also, remove the atomic_check() callback as mode_valid() callback will be called before.

Signed-off-by Jose Abreu <[email protected]>

Cc Carlos Palminha <[email protected]>

Cc Alexey Brodkin <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Andrzej Hajda <[email protected]>

Cc Archit Taneja <[email protected]>

---
 drivers/gpu/drm/arc/arcpgu_crtc.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

drivers/gpu/drm/arc/arcpgu_crtc.c +24 ~~-15~~

index ad9a959..01cae0a 100644
@@ -32,6 +32,18 @@
 	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 },
 };
 
+static bool arc_pgu_is_mode_valid(struct arcpgu_drm_private *arcpgu,
+				  const struct drm_display_mode *mode)
+{
+	long rate, clk_rate = mode->clock * 1000;
+
+	rate = clk_round_rate(arcpgu->clk, clk_rate);
+	if (rate != clk_rate)
+		return false;
+
+	return true;
+}
+
 static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
 {
 	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);

@@ -64,6 +76,17 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
 	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 };
 
+enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc,
+					     const struct drm_display_mode *mode)
+{
+	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
+
+	if (!arc_pgu_is_mode_valid(arcpgu, mode))
+		return MODE_NOCLOCK;
+
+	return MODE_OK;
+}
+
 static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
 	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);

@@ -129,20 +152,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
 			      ~ARCPGU_CTRL_ENABLE_MASK);
 }
 
-static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
-				     struct drm_crtc_state *state)
-{
-	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
-	struct drm_display_mode *mode = &state->adjusted_mode;
-	long rate, clk_rate = mode->clock * 1000;
-
-	rate = clk_round_rate(arcpgu->clk, clk_rate);
-	if (rate != clk_rate)
-		return -EINVAL;
-
-	return 0;
-}
-
 static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 				      struct drm_crtc_state *state)
 {

@@ -158,6 +167,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
+	.mode_valid	= arc_pgu_crtc_mode_valid,
 	.mode_set	= drm_helper_crtc_mode_set,
 	.mode_set_base	= drm_helper_crtc_mode_set_base,
 	.mode_set_nofb	= arc_pgu_crtc_mode_set_nofb,

@@ -165,7 +175,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 	.disable	= arc_pgu_crtc_disable,
 	.prepare	= arc_pgu_crtc_disable,
 	.commit		= arc_pgu_crtc_enable,
-	.atomic_check	= arc_pgu_crtc_atomic_check,
 	.atomic_begin	= arc_pgu_crtc_atomic_begin,
 };

Signature · 1 line

1.9.1

Daniel Vetter

2017-05-12 07:00 UTC #2 lore raw

Re: [PATCH v3 1/6] drm: Add crtc/encoder/bridge->mode_valid() callbacks

In reply to Jose Abreu

Quoted 20 lines · Jose Abreu

This adds a new callback to crtc, encoder and bridge helper functions called mode_valid(). This callback shall be implemented if the corresponding component has some sort of restriction in the modes that can be displayed. A NULL callback implicates that the component can display all the modes.
We also change the documentation so that the new and old callbacks are correctly documented.
Only the callbacks were implemented to simplify review process, following patches will make use of them.
Signed-off-by Jose Abreu <[email protected]>
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>

Quoted 5 lines

---
Changes v2->v3:
- Try to document the callbacks a little bit better and review current documentation (Daniel)

Not quite what I had in mind, all the language about "Please be aware that neither core nor helper filter modes" is still there. And that was added to explain the difference between mode_valid and mode_fixup.

Since all the other patches look good I'll take stab at a v3 myself. -Daniel

Quoted 208 lines

Changes v1->v2:
- Change description of connector->mode_valid() (Daniel)

 include/drm/drm_bridge.h                 |  20 ++++++
 include/drm/drm_modeset_helper_vtables.h | 110 +++++++++++++++++++++++--------
 2 files changed, 103 insertions(+), 27 deletions(-)

include/drm/drm_bridge.h +20 -0

index fdd82fc..00c6c36 100644
@@ -59,6 +59,26 @@ struct drm_bridge_funcs {
 	void (*detach)(struct drm_bridge *bridge);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * bridge. This should be implemented if the bridge has some sort of
+	 * restriction in the modes it can display. For example, a given bridge
+	 * 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This is called at mode probe and at atomic check phase.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc,
+					   const struct drm_display_mode *mode);
+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate and adjust a mode. The paramater

include/drm/drm_modeset_helper_vtables.h +83 ~~-27~~

index c01c328..b07b7cd 100644
@@ -106,14 +106,37 @@ struct drm_crtc_helper_funcs {
 	void (*commit)(struct drm_crtc *crtc);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * crtc. This 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This is called at mode probe and at atomic check phase.
+	 *
+	 * 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
-	 * display mode that userspace requested, adjusted_mode is the mode the
-	 * encoders need to be fed with. Note that this is the inverse semantics
-	 * of the meaning for the &drm_encoder and &drm_bridge_funcs.mode_fixup
-	 * vfunc. If the CRTC cannot support the requested conversion from mode
-	 * to adjusted_mode it should reject the modeset.
+	 * This callback is used to do the validation of an adjusted mode in the
+	 * crtc. The parameter mode is the display mode that userspace requested,
+	 * adjusted_mode is the mode the encoders need to be fed with. Note that
+	 * this is the inverse semantics of the meaning for the &drm_encoder and
+	 * &drm_bridge_funcs.mode_fixup vfunc. If the CRTC cannot support the
+	 * requested conversion from mode to adjusted_mode it should reject the
+	 * modeset. Also note that initial validation of a mode supplied by
+	 * userspace should be done in &drm_crtc_helper_funcs.mode_valid and not
+	 * in this callback.
 	 *
 	 * This function is used by both legacy CRTC helpers and atomic helpers.
 	 * With atomic helpers it is optional.

@@ -135,17 +158,19 @@ struct drm_crtc_helper_funcs {
 	 * Also beware that neither core nor helpers filter modes before
 	 * passing them to the driver: While the list of modes that is
 	 * advertised to userspace is filtered using the
-	 * &drm_connector.mode_valid callback, neither the core nor the helpers
-	 * do any filtering on modes passed in from userspace when setting a
-	 * mode. It is therefore possible for userspace to pass in a mode that
-	 * was previously filtered out using &drm_connector.mode_valid or add a
-	 * custom mode that wasn't probed from EDID or similar to begin with.
-	 * Even though this is an advanced feature and rarely used nowadays,
-	 * some users rely on being able to specify modes manually so drivers
-	 * must be prepared to deal with it. Specifically this means that all
-	 * drivers need not only validate modes in &drm_connector.mode_valid but
-	 * also in this or in the &drm_encoder_helper_funcs.mode_fixup callback
-	 * to make sure invalid modes passed in from userspace are rejected.
+	 * &drm_connector_helper_funcs.mode_valid callback, neither the core nor
+	 * the helpers do any filtering on modes passed in from userspace when
+	 * setting a mode. It is therefore possible for userspace to pass in a
+	 * mode that was previously filtered out using
+	 * &drm_connector_helper_funcs.mode_valid or add a custom mode that
+	 * wasn't probed from EDID or similar to begin with. Even though this
+	 * is an advanced feature and rarely used nowadays, some users rely on
+	 * being able to specify modes manually so drivers must be prepared to
+	 * deal with it. Specifically this means that all drivers need not only
+	 * validate modes in &drm_connector.mode_valid but also in this or in
+	 * the &drm_crtc_helper_funcs.mode_valid,
+	 * &drm_encoder_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid
+	 * callbacks, where applicable.
 	 *
 	 * RETURNS:
 	 *

@@ -457,11 +482,31 @@ struct drm_encoder_helper_funcs {
 	void (*dpms)(struct drm_encoder *encoder, int mode);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * encoder. This should be implemented if the encoder has some sort
+	 * of restriction in the modes it can display. For example, a given
+	 * encoder 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This is called at mode probe and at atomic check phase.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc,
+					   const struct drm_display_mode *mode);
+
+	/**
 	 * @mode_fixup:
 	 *
-	 * This callback is used to validate and adjust a mode. The parameter
-	 * mode is the display mode that should be fed to the next element in
-	 * the display chain, either the final &drm_connector or a &drm_bridge.
+	 * This callback is used to adjust a mode. The parameter mode is the
+	 * display mode that should be fed to the next element in the display
+	 * chain, either the final &drm_connector or a &drm_bridge.
 	 * The parameter adjusted_mode is the input mode the encoder requires. It
 	 * can be modified by this callback and does not need to match mode.
 	 *

@@ -484,19 +529,20 @@ struct drm_encoder_helper_funcs {
 	 *
 	 * Also beware that neither core nor helpers filter modes before
 	 * passing them to the driver: While the list of modes that is
-	 * advertised to userspace is filtered using the connector's
+	 * advertised to userspace is filtered using the
 	 * &drm_connector_helper_funcs.mode_valid callback, neither the core nor
 	 * the helpers do any filtering on modes passed in from userspace when
 	 * setting a mode. It is therefore possible for userspace to pass in a
 	 * mode that was previously filtered out using
 	 * &drm_connector_helper_funcs.mode_valid or add a custom mode that
-	 * wasn't probed from EDID or similar to begin with.  Even though this
+	 * wasn't probed from EDID or similar to begin with. Even though this
 	 * is an advanced feature and rarely used nowadays, some users rely on
 	 * being able to specify modes manually so drivers must be prepared to
 	 * deal with it. Specifically this means that all drivers need not only
 	 * validate modes in &drm_connector.mode_valid but also in this or in
-	 * the &drm_crtc_helper_funcs.mode_fixup callback to make sure
-	 * invalid modes passed in from userspace are rejected.
+	 * the &drm_crtc_helper_funcs.mode_valid,
+	 * &drm_encoder_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid
+	 * callbacks, where applicable.
 	 *
 	 * RETURNS:
 	 *

@@ -795,13 +841,23 @@ struct drm_connector_helper_funcs {
 	 * (which is usually derived from the EDID data block from the sink).
 	 * See e.g. drm_helper_probe_single_connector_modes().
 	 *
+	 * This callback is never called in atomic check phase so that userspace
+	 * can override kernel sink checks in case of broken EDID with wrong
+	 * limits from the sink. You can use the remaining mode_valid()
+	 * callbacks to validate the mode against your video path.
+	 *
 	 * NOTE:
 	 *
 	 * This only filters the mode list supplied to userspace in the
-	 * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and
+	 * GETCONNECTOR IOCTL. Userspace is free to create modes of its own and
 	 * ask the kernel to use them. It this case the atomic helpers or legacy
-	 * CRTC helpers will not call this function. Drivers therefore must
-	 * still fully validate any mode passed in in a modeset request.
+	 * CRTC helpers will not call this function but the mode will be
+	 * validated in atomic check phase using the mode_valid() callbacks
+	 * (&drm_crtc_helper_funcs.mode_valid, &drm_encoder_helper_funcs.mode_valid
+	 * and &drm_bridge_funcs.mode_valid).
+	 * Drivers therefore must implement the mode_valid() callbacks if the
+	 * video pipeline has some sort of restrictions in the modes that can
+	 * be displayed.
 	 *
 	 * To avoid races with concurrent connector state updates, the helper
 	 * libraries always call this with the &drm_mode_config.connection_mutex

Signature · 1 line

1.9.1

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter

2017-05-12 07:31 UTC #3 lore raw

PATCH 1/1 drm: Add crtc/encoder/bridge->mode_valid() callbacks

In reply to Jose Abreu

From: Jose Abreu <[email protected]>

We also change the documentation so that the new and old callbacks are correctly documented.

Only the callbacks were implemented to simplify review process, following patches will make use of them.

Changes in v2 from Daniel:
- Update the warning about how modes aren't filtered in atomic_check - the heleprs help out a lot more now.
- Consistenly roll out that warning, crtc/encoder's atomic_check missed it.
- Sprinkle more links all over the place, so it's easier to see where this stuff is used and how the differen hooks are related.
- Note that ->mode_valid is optional everywhere.
- Explain why the connector's mode_valid is special and does _not_ get called in atomic_check.

Signed-off-by Jose Abreu <[email protected]>

Cc Jose Abreu <[email protected]>

Cc Carlos Palminha <[email protected]>

Cc Alexey Brodkin <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Andrzej Hajda <[email protected]>

Cc Archit Taneja <[email protected]>

Signed-off-by Daniel Vetter <[email protected]> (v2)

---
 include/drm/drm_bridge.h                 |  31 +++++++++
 include/drm/drm_modeset_helper_vtables.h | 116 ++++++++++++++++++++++---------
 2 files changed, 114 insertions(+), 33 deletions(-)

include/drm/drm_bridge.h +31 -0

index fdd82fcbf168..f694de756ecf 100644
@@ -59,6 +59,31 @@ struct drm_bridge_funcs {
 	void (*detach)(struct drm_bridge *bridge);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * bridge. This should be implemented if the bridge has some sort of
+	 * restriction in the modes it can display. For example, a given bridge
+	 * 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in
+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc,
+					   const struct drm_display_mode *mode);
+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate and adjust a mode. The paramater

@@ -82,6 +107,12 @@ struct drm_bridge_funcs {
 	 * NOT touch any persistent state (hardware or software) or data
 	 * structures except the passed in @state parameter.
 	 *
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any bridge constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * True if an acceptable configuration is possible, false if the modeset

include/drm/drm_modeset_helper_vtables.h +83 ~~-33~~

161 lines not on this page · raw message

Signature · 6 lines

2.11.0

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter

Reviewed-by on the v3 series

2017-05-12 07:32 UTC #33 lore raw

In reply to Jose Abreu

Quoted 32 lines · Jose Abreu

This series is a follow up from the discussion at [1]. We start by introducing crtc->mode_valid(), encoder->mode_valid() and bridge->mode_valid() callbacks which will be used in followup patches and also by cleaning the documentation a little bit.
We proceed by introducing new helpers to call this new callbacks at 2/6.
At 3/6 a helper function is introduced that calls all mode_valid() from a set of bridges.
Next, at 4/6 we modify the connector probe helper so that only modes which are supported by a given bridge+encoder+crtc combination are probbed.
At 5/6 we call all the mode_valid() callbacks for a given pipeline, except the connector->mode_valid one, so that the mode is validated. This is done before calling mode_fixup().
Finally, at 6/6 we use the new crtc->mode_valid() callback in arcpgu and remove the atomic_check() callback.
[1] https://patchwork.kernel.org/patch/9702233/
Jose Abreu (6):
drm: Add crtc/encoder/bridge->mode_valid() callbacks
drm: Add drm_{crtc/encoder/connector}_mode_valid()
drm: Introduce drm_bridge_mode_valid()
drm: Use new mode_valid() helpers in connector probe helper
drm: Use mode_valid() in atomic modeset
drm: arc: Use crtc->mode_valid() callback

Quoted 7 lines

Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>

Commented with an entire patch on patch 1, patches 2-5 are all

Reviewed-by Daniel Vetter <[email protected]>

But I think some more acks/r-bs would be really good, since this is quite a bit change in the helper infrastructure. But otherwise ready for merging imo. Can you pls also review my proposal for patch 1?

Thanks, Daniel

Quoted 14 lines

 drivers/gpu/drm/arc/arcpgu_crtc.c          |  39 ++++++----
 drivers/gpu/drm/drm_atomic_helper.c        |  76 +++++++++++++++++++-
 drivers/gpu/drm/drm_bridge.c               |  33 +++++++++
 drivers/gpu/drm/drm_crtc_helper_internal.h |  13 ++++
 drivers/gpu/drm/drm_probe_helper.c         | 103 ++++++++++++++++++++++++++-
 include/drm/drm_bridge.h                   |  22 ++++++
 include/drm/drm_modeset_helper_vtables.h   | 110 ++++++++++++++++++++++-------
 7 files changed, 348 insertions(+), 48 deletions(-)

Signature · 1 line

1.9.1

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Laurent Pinchart

reviewer

2017-05-12 11:29 UTC #4 lore raw

Re: [PATCH] drm: Add crtc/encoder/bridge->mode_valid() callbacks

In reply to Daniel Vetter

Hi Daniel,

Quoted 14 lines · On Friday 12 May 2017 09:31:00 Daniel Vetter wrote:

From: Jose Abreu <[email protected]>
This adds a new callback to crtc, encoder and bridge helper functions called mode_valid(). This callback shall be implemented if the corresponding component has some sort of restriction in the modes that can be displayed. A NULL callback implicates that the component can display all the modes.
We also change the documentation so that the new and old callbacks are correctly documented.
Only the callbacks were implemented to simplify review process, following patches will make use of them.

Quoted 10 lines

Changes in v2 from Daniel:
- Update the warning about how modes aren't filtered in atomic_check - the heleprs help out a lot more now.
- Consistenly roll out that warning, crtc/encoder's atomic_check missed it.
- Sprinkle more links all over the place, so it's easier to see where this stuff is used and how the differen hooks are related.
- Note that ->mode_valid is optional everywhere.
- Explain why the connector's mode_valid is special and does _not_ get called in atomic_check.

As commented on v2 (but after you sent this patch), I think we need to document clearly how mode_valid and mode_fixup should interact. It's very confusing for driver authors at the moment.

Quoted 21 lines

Signed-off-by Jose Abreu <[email protected]>
Cc Jose Abreu <[email protected]>
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>
Signed-off-by Daniel Vetter <[email protected]> (v2)
---
 include/drm/drm_bridge.h                 |  31 +++++++++
 include/drm/drm_modeset_helper_vtables.h | 116
++++++++++++++++++++++--------- 2 files changed, 114 insertions(+), 33 deletions(-)
include/drm/drm_bridge.h +0 -0
index fdd82fcbf168..f694de756ecf 100644

include/drm/drm_bridge.h struct drm_bridge_funcs { · line 66

Quoted 8 lines

include/drm/drm_bridge.h +5 -0

@@ -59,6 +59,31 @@ struct drm_bridge_funcs {
 	void (*detach)(struct drm_bridge *bridge);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * bridge. This should be implemented if the bridge has some sort of
+	 * restriction in the modes it can display. For example, a given

bridge

Quoted 8 lines

include/drm/drm_bridge.h +8 -0

+	 * 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in

include/drm/drm_bridge.h struct drm_bridge_funcs {

Quoted 10 lines

include/drm/drm_bridge.h +10 -0

+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc,
+					   const struct drm_display_mode

*mode);

include/drm/drm_bridge.h struct drm_bridge_funcs { · line 110

Quoted 10 lines

include/drm/drm_bridge.h +3 -0

+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate and adjust a mode. The paramater

@@ -82,6 +107,12 @@ struct drm_bridge_funcs {
 	 * NOT touch any persistent state (hardware or software) or data
 	 * structures except the passed in @state parameter.
 	 *
+	 * Also beware that userspace can request its own custom modes,

neither

include/drm/drm_bridge.h struct drm_bridge_funcs { · line 111

Quoted 1 line

include/drm/drm_bridge.h +1 -0

+	 * core nor helpers filter modes to the list of probe modes reported

include/drm/drm_bridge.h struct drm_bridge_funcs { · line 112

Quoted 1 line

include/drm/drm_bridge.h +1 -0

+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To

ensure

include/drm/drm_bridge.h struct drm_bridge_funcs {

Quoted 6 lines

include/drm/drm_bridge.h +3 -0

+	 * that modes are filtered consistently put any bridge constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * True if an acceptable configuration is possible, false if the

modeset

Quoted 22 lines

(quoted diff) +0 -0

b/include/drm/drm_modeset_helper_vtables.h index c01c328f6cc8..91d071ff1232 100644

include/drm/drm_modeset_helper_vtables.h +13 -0

@@ -106,6 +106,31 @@ struct drm_crtc_helper_funcs {
 	void (*commit)(struct drm_crtc *crtc);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * crtc. This 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 131

Quoted 10 lines

include/drm/drm_modeset_helper_vtables.h +10 -0

+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
+					   const struct drm_display_mode

*mode);

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 138

Quoted 13 lines

include/drm/drm_modeset_helper_vtables.h +2 -4

+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate a mode. The parameter mode is the

@@ -132,20 +157,11 @@ struct drm_crtc_helper_funcs {
 	 * Atomic drivers which need to inspect and adjust more state should
 	 * instead use the @atomic_check callback.
 	 *
-	 * Also beware that neither core nor helpers filter modes before
-	 * passing them to the driver: While the list of modes that is
-	 * advertised to userspace is filtered using the
-	 * &drm_connector.mode_valid callback, neither the core nor the

helpers

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs {

Quoted 3 lines

include/drm/drm_modeset_helper_vtables.h +0 -3

-	 * do any filtering on modes passed in from userspace when setting a
-	 * mode. It is therefore possible for userspace to pass in a mode that
-	 * was previously filtered out using &drm_connector.mode_valid or add

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs {

Quoted 5 lines

include/drm/drm_modeset_helper_vtables.h +0 -5

-	 * custom mode that wasn't probed from EDID or similar to begin with.
-	 * Even though this is an advanced feature and rarely used nowadays,
-	 * some users rely on being able to specify modes manually so drivers
-	 * must be prepared to deal with it. Specifically this means that all
-	 * drivers need not only validate modes in &drm_connector.mode_valid

but

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 147

Quoted 1 line

include/drm/drm_modeset_helper_vtables.h +0 -1
-	 * also in this or in the &drm_encoder_helper_funcs.mode_fixup 

callback

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 160

Quoted 2 lines

include/drm/drm_modeset_helper_vtables.h +1 -1

-	 * to make sure invalid modes passed in from userspace are rejected.
+	 * Also beware that userspace can request its own custom modes,

neither

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 161

Quoted 1 line

include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * core nor helpers filter modes to the list of probe modes reported 

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 162

Quoted 1 line

include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To 

ensure

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 357

Quoted 7 lines

include/drm/drm_modeset_helper_vtables.h +2 -0

+	 * that modes are filtered consistently put any CRTC constraints and
+	 * limits checks into @mode_valid.
 	 *
 	 * RETURNS:
 	 *

@@ -341,6 +357,12 @@ struct drm_crtc_helper_funcs {
 	 * state objects passed-in or assembled in the overall

&drm_atomic_state

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs {

Quoted 3 lines

include/drm/drm_modeset_helper_vtables.h +1 -0

 	 * update tracking structure.
 	 *
+	 * Also beware that userspace can request its own custom modes,

neither

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 361

Quoted 1 line

include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * core nor helpers filter modes to the list of probe modes reported 

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 362

Quoted 1 line

include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To 

ensure

Quoted 7 lines

include/drm/drm_modeset_helper_vtables.h +3 -0

+	 * that modes are filtered consistently put any CRTC constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * 0 on success, -EINVAL if the state or the transition can't be

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 488

Quoted 10 lines

include/drm/drm_modeset_helper_vtables.h +7 -0

@@ -457,6 +479,31 @@ struct drm_encoder_helper_funcs {
 	void (*dpms)(struct drm_encoder *encoder, int mode);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * encoder. This should be implemented if the encoder has some sort
+	 * of restriction in the modes it can display. For example, a given
+	 * encoder may be responsible to set a clock value. If the clock can
+	 * not produce all the values for the available modes then this

callback

Quoted 6 lines

include/drm/drm_modeset_helper_vtables.h +6 -0

+	 * can be used to restrict the number of modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs {

Quoted 10 lines

include/drm/drm_modeset_helper_vtables.h +10 -0

+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc,
+					   const struct drm_display_mode

*mode);

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 488

Quoted 13 lines

include/drm/drm_modeset_helper_vtables.h +2 -4

+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate and adjust a mode. The parameter

@@ -482,21 +529,11 @@ struct drm_encoder_helper_funcs {
 	 * Atomic drivers which need to inspect and adjust more state should
 	 * instead use the @atomic_check callback.
 	 *
-	 * Also beware that neither core nor helpers filter modes before
-	 * passing them to the driver: While the list of modes that is
-	 * advertised to userspace is filtered using the connector's
-	 * &drm_connector_helper_funcs.mode_valid callback, neither the core

nor

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs {

Quoted 8 lines

include/drm/drm_modeset_helper_vtables.h +0 -8

-	 * the helpers do any filtering on modes passed in from userspace when
-	 * setting a mode. It is therefore possible for userspace to pass in a
-	 * mode that was previously filtered out using
-	 * &drm_connector_helper_funcs.mode_valid or add a custom mode that
-	 * wasn't probed from EDID or similar to begin with.  Even though this
-	 * is an advanced feature and rarely used nowadays, some users rely on
-	 * being able to specify modes manually so drivers must be prepared to
-	 * deal with it. Specifically this means that all drivers need not

only

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs {

Quoted 4 lines

include/drm/drm_modeset_helper_vtables.h +1 -3

-	 * validate modes in &drm_connector.mode_valid but also in this or in
-	 * the &drm_crtc_helper_funcs.mode_fixup callback to make sure
-	 * invalid modes passed in from userspace are rejected.
+	 * Also beware that userspace can request its own custom modes,

neither

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 533

Quoted 1 line

include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * core nor helpers filter modes to the list of probe modes reported 

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 534

Quoted 1 line

include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To 

ensure

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 535

Quoted 1 line

include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * that modes are filtered consistently put any encoder constraints 

and

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 723

Quoted 6 lines

include/drm/drm_modeset_helper_vtables.h +1 -0

+	 * limits checks into @mode_valid.
 	 *
 	 * RETURNS:
 	 *

@@ -686,6 +723,12 @@ struct drm_encoder_helper_funcs {
 	 * state objects passed-in or assembled in the overall

&drm_atomic_state

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs {

Quoted 3 lines

include/drm/drm_modeset_helper_vtables.h +1 -0

 	 * update tracking structure.
 	 *
+	 * Also beware that userspace can request its own custom modes,

neither

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 727

Quoted 1 line

include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * core nor helpers filter modes to the list of probe modes reported 

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 728

Quoted 1 line

include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To 

ensure

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 729

Quoted 1 line

include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * that modes are filtered consistently put any encoder constraints 

and

Quoted 6 lines

include/drm/drm_modeset_helper_vtables.h +2 -0

+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * 0 on success, -EINVAL if the state or the transition can't be

include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs { · line 801

Quoted 9 lines

include/drm/drm_modeset_helper_vtables.h +2 -1

@@ -795,13 +838,20 @@ struct drm_connector_helper_funcs {
 	 * (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.
+	 *
 	 * NOTE:
 	 *
 	 * This only filters the mode list supplied to userspace in the
-	 * GETCONNECOTR IOCTL. Userspace is free to create modes of its own

and

include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs { · line 802

Quoted 1 line

include/drm/drm_modeset_helper_vtables.h +0 -1
-	 * ask the kernel to use them. It this case the atomic helpers or 

legacy

include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs {

Quoted 3 lines

include/drm/drm_modeset_helper_vtables.h +1 -2

-	 * CRTC helpers will not call this function. Drivers therefore must
-	 * still fully validate any mode passed in in a modeset request.
+	 * GETCONNECTOR IOCTL. Compared to

&drm_encoder_helper_funcs.mode_valid,

include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs {

Quoted 3 lines

include/drm/drm_modeset_helper_vtables.h +3 -0

+	 * &drm_crtc_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid,
+	 * which are also called by the atomic helpers from
+	 * drm_atomic_helper_check_modeset(). This allows userspace to force

and

include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs {

Quoted 4 lines

include/drm/drm_modeset_helper_vtables.h +4 -0

+	 * ignore sink constraint (like the pixel clock limits in the screen's
+	 * EDID), which is useful for e.g. testing, or working around a broken
+	 * EDID. Any source hardware constraint (which always need to be
+	 * enforced) therefore should be checked in one of the above

callbacks,

include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs {

Quoted 4 lines

include/drm/drm_modeset_helper_vtables.h +1 -0

+	 * and not this one here.
 	 *
 	 * To avoid races with concurrent connector state updates, the helper
 	 * libraries always call this with the

&drm_mode_config.connection_mutex

Signature · 8 lines

Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Andrzej Hajda

2017-05-12 12:37 UTC #6 lore raw

Re: [PATCH] drm: Add crtc/encoder/bridge->mode_valid() callbacks

In reply to Daniel Vetter

Quoted 61 lines · Daniel Vetter

From: Jose Abreu <[email protected]>
This adds a new callback to crtc, encoder and bridge helper functions called mode_valid(). This callback shall be implemented if the corresponding component has some sort of restriction in the modes that can be displayed. A NULL callback implicates that the component can display all the modes.
We also change the documentation so that the new and old callbacks are correctly documented.
Only the callbacks were implemented to simplify review process, following patches will make use of them.
Changes in v2 from Daniel:
- Update the warning about how modes aren't filtered in atomic_check - the heleprs help out a lot more now.
- Consistenly roll out that warning, crtc/encoder's atomic_check missed it.
- Sprinkle more links all over the place, so it's easier to see where this stuff is used and how the differen hooks are related.
- Note that ->mode_valid is optional everywhere.
- Explain why the connector's mode_valid is special and does _not_ get called in atomic_check.
Signed-off-by Jose Abreu <[email protected]>
Cc Jose Abreu <[email protected]>
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>
Signed-off-by Daniel Vetter <[email protected]> (v2)
---
 include/drm/drm_bridge.h                 |  31 +++++++++
 include/drm/drm_modeset_helper_vtables.h | 116 ++++++++++++++++++++++---------
 2 files changed, 114 insertions(+), 33 deletions(-)
include/drm/drm_bridge.h +13 -0
index fdd82fcbf168..f694de756ecf 100644
@@ -59,6 +59,31 @@ struct drm_bridge_funcs {
 	void (*detach)(struct drm_bridge *bridge);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * bridge. This should be implemented if the bridge has some sort of
+	 * restriction in the modes it can display. For example, a given bridge
+	 * 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in

include/drm/drm_bridge.h struct drm_bridge_funcs { · line 84

Quoted 10 lines

include/drm/drm_bridge.h +10 -0

+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc,
+					   const struct drm_display_mode *mode);

I have put my concerns here but it touches all mode_valid callbacks. As the callback can be called in mode probe and atomic check it could only inspect common set of properties for both contexts, for example crtc cannot check to which encoder it is connected, am I right? Maybe it would be good to emphasize it here, as it is emphasized in case of mode_fixup callbacks.

Quoted 67 lines

include/drm/drm_bridge.h +8 -0

+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate and adjust a mode. The paramater

@@ -82,6 +107,12 @@ struct drm_bridge_funcs {
 	 * NOT touch any persistent state (hardware or software) or data
 	 * structures except the passed in @state parameter.
 	 *
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any bridge constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * True if an acceptable configuration is possible, false if the modeset

include/drm/drm_modeset_helper_vtables.h +25 -9

index c01c328f6cc8..91d071ff1232 100644
@@ -106,6 +106,31 @@ struct drm_crtc_helper_funcs {
 	void (*commit)(struct drm_crtc *crtc);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * crtc. This 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in
+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * 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

@@ -132,20 +157,11 @@ struct drm_crtc_helper_funcs {
 	 * Atomic drivers which need to inspect and adjust more state should
 	 * instead use the @atomic_check callback.
 	 *
-	 * Also beware that neither core nor helpers filter modes before
-	 * passing them to the driver: While the list of modes that is
-	 * advertised to userspace is filtered using the
-	 * &drm_connector.mode_valid callback, neither the core nor the helpers
-	 * do any filtering on modes passed in from userspace when setting a
-	 * mode. It is therefore possible for userspace to pass in a mode that
-	 * was previously filtered out using &drm_connector.mode_valid or add a
-	 * custom mode that wasn't probed from EDID or similar to begin with.
-	 * Even though this is an advanced feature and rarely used nowadays,

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 164

Quoted 10 lines

include/drm/drm_modeset_helper_vtables.h +5 -5

-	 * some users rely on being able to specify modes manually so drivers
-	 * must be prepared to deal with it. Specifically this means that all
-	 * drivers need not only validate modes in &drm_connector.mode_valid but
-	 * also in this or in the &drm_encoder_helper_funcs.mode_fixup callback
-	 * to make sure invalid modes passed in from userspace are rejected.
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any CRTC constraints and
+	 * limits checks into @mode_valid.

Why we do not make mode_fixup obsolete for atomic drivers as atomic_check can do the same and is more powerful, this way we could avoid the overlapped functionality of mode_valid and mode_fixup.

Quoted 100 lines

include/drm/drm_modeset_helper_vtables.h +44 ~~-18~~

 	 *
 	 * RETURNS:
 	 *

@@ -341,6 +357,12 @@ struct drm_crtc_helper_funcs {
 	 * state objects passed-in or assembled in the overall &drm_atomic_state
 	 * update tracking structure.
 	 *
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any CRTC constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * 0 on success, -EINVAL if the state or the transition can't be

@@ -457,6 +479,31 @@ struct drm_encoder_helper_funcs {
 	void (*dpms)(struct drm_encoder *encoder, int mode);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * encoder. This should be implemented if the encoder has some sort
+	 * of restriction in the modes it can display. For example, a given
+	 * encoder 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in
+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc,
+					   const struct drm_display_mode *mode);
+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate and adjust a mode. The parameter

@@ -482,21 +529,11 @@ struct drm_encoder_helper_funcs {
 	 * Atomic drivers which need to inspect and adjust more state should
 	 * instead use the @atomic_check callback.
 	 *
-	 * Also beware that neither core nor helpers filter modes before
-	 * passing them to the driver: While the list of modes that is
-	 * advertised to userspace is filtered using the connector's
-	 * &drm_connector_helper_funcs.mode_valid callback, neither the core nor
-	 * the helpers do any filtering on modes passed in from userspace when
-	 * setting a mode. It is therefore possible for userspace to pass in a
-	 * mode that was previously filtered out using
-	 * &drm_connector_helper_funcs.mode_valid or add a custom mode that
-	 * wasn't probed from EDID or similar to begin with.  Even though this
-	 * is an advanced feature and rarely used nowadays, some users rely on
-	 * being able to specify modes manually so drivers must be prepared to
-	 * deal with it. Specifically this means that all drivers need not only
-	 * validate modes in &drm_connector.mode_valid but also in this or in
-	 * the &drm_crtc_helper_funcs.mode_fixup callback to make sure
-	 * invalid modes passed in from userspace are rejected.
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any encoder constraints and
+	 * limits checks into @mode_valid.
 	 *
 	 * RETURNS:
 	 *

@@ -686,6 +723,12 @@ struct drm_encoder_helper_funcs {
 	 * state objects passed-in or assembled in the overall &drm_atomic_state
 	 * update tracking structure.
 	 *
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any encoder constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * 0 on success, -EINVAL if the state or the transition can't be

@@ -795,13 +838,20 @@ struct drm_connector_helper_funcs {
 	 * (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.
+	 *
 	 * NOTE:
 	 *
 	 * This only filters the mode list supplied to userspace in the
-	 * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and
-	 * ask the kernel to use them. It this case the atomic helpers or legacy
-	 * CRTC helpers will not call this function. Drivers therefore must

include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs { · line 854

Quoted 10 lines

include/drm/drm_modeset_helper_vtables.h +9 -1

-	 * still fully validate any mode passed in in a modeset request.
+	 * GETCONNECTOR IOCTL. Compared to &drm_encoder_helper_funcs.mode_valid,
+	 * &drm_crtc_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid,
+	 * which are also called by the atomic helpers from
+	 * drm_atomic_helper_check_modeset(). This allows userspace to force and
+	 * ignore sink constraint (like the pixel clock limits in the screen's
+	 * EDID), which is useful for e.g. testing, or working around a broken
+	 * EDID. Any source hardware constraint (which always need to be
+	 * enforced) therefore should be checked in one of the above callbacks,
+	 * and not this one here.

The callback has the same name but different semantic, little bit weird. But do we really need connector::mode_valid then? I mean: are there connectors not bound to bridge/encoder which have their own constraints? If there are no such connectors, we can remove this callback. If they are rare, maybe just adding loop to connector::get_modes would be enough.

Regards
Andrzej

include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs { · line 857

Quoted 3 lines

include/drm/drm_modeset_helper_vtables.h +0 -0

 	 *
 	 * To avoid races with concurrent connector state updates, the helper
 	 * libraries always call this with the &drm_mode_config.connection_mutex

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Andrzej Hajda

2017-05-12 13:37 UTC #34 lore raw

In reply to Daniel Vetter

Quoted 42 lines · Daniel Vetter

Quoted 32 lines · Jose Abreu
This series is a follow up from the discussion at [1]. We start by introducing crtc->mode_valid(), encoder->mode_valid() and bridge->mode_valid() callbacks which will be used in followup patches and also by cleaning the documentation a little bit.
We proceed by introducing new helpers to call this new callbacks at 2/6.
At 3/6 a helper function is introduced that calls all mode_valid() from a set of bridges.
Next, at 4/6 we modify the connector probe helper so that only modes which are supported by a given bridge+encoder+crtc combination are probbed.
At 5/6 we call all the mode_valid() callbacks for a given pipeline, except the connector->mode_valid one, so that the mode is validated. This is done before calling mode_fixup().
Finally, at 6/6 we use the new crtc->mode_valid() callback in arcpgu and remove the atomic_check() callback.
[1] https://patchwork.kernel.org/patch/9702233/
Jose Abreu (6):
drm: Add crtc/encoder/bridge->mode_valid() callbacks
drm: Add drm_{crtc/encoder/connector}_mode_valid()
drm: Introduce drm_bridge_mode_valid()
drm: Use new mode_valid() helpers in connector probe helper
drm: Use mode_valid() in atomic modeset
drm: arc: Use crtc->mode_valid() callback
Quoted 7 lines
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>
Commented with an entire patch on patch 1, patches 2-5 are all

Quoted 7 lines

Reviewed-by Daniel Vetter <[email protected]>
But I think some more acks/r-bs would be really good, since this is quite a bit change in the helper infrastructure. But otherwise ready for merging imo. Can you pls also review my proposal for patch 1?
Thanks, Daniel

As the patchset improves many things, I would like to point here that there are still issues with mode probing at least in case of panels. Panels in general does not provide discrete list of supported modes, but they provide list of supported mode ranges (named display_timings), at the moment this is only addressed in drm_panel_funcs::get_timings, but drm core does not use it at all.
Currently most of the panel drivers advertises only fixed list of arbitrary chosen modes, and if bridge/connector/encoder/crtc does not support it pipeline does not work, even if slightly different configuration could work. Of course there are workarounds but maybe it would be good to replace drm_connector::modes with drm_connector::timings. This way set of valid timings of the whole pipeline will be intersection of sets of valid timings of every component of the pipeline - quite straightforward and simple construct.

As this is just an idea which came to me during patchset review, it is not backed by any code, but maybe it can be interesting for quick brainstorm.

Regards
Andrzej

Quoted 10 lines

 drivers/gpu/drm/arc/arcpgu_crtc.c          |  39 ++++++----
 drivers/gpu/drm/drm_atomic_helper.c        |  76 +++++++++++++++++++-
 drivers/gpu/drm/drm_bridge.c               |  33 +++++++++
 drivers/gpu/drm/drm_crtc_helper_internal.h |  13 ++++
 drivers/gpu/drm/drm_probe_helper.c         | 103 ++++++++++++++++++++++++++-
 include/drm/drm_bridge.h                   |  22 ++++++
 include/drm/drm_modeset_helper_vtables.h   | 110 ++++++++++++++++++++++-------
 7 files changed, 348 insertions(+), 48 deletions(-)

Quoted 4 lines

Quoted 4 lines
Signature · 1 line
1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Jose Abreu

author Reviewed-by on v1 1/1

2017-05-12 15:59 UTC #10 lore raw

Re: [PATCH] drm: Add crtc/encoder/bridge->mode_valid() callbacks

In reply to Daniel Vetter

Hi Daniel,

Quoted 25 lines · Daniel Vetter

From: Jose Abreu <[email protected]>
This adds a new callback to crtc, encoder and bridge helper functions called mode_valid(). This callback shall be implemented if the corresponding component has some sort of restriction in the modes that can be displayed. A NULL callback implicates that the component can display all the modes.
We also change the documentation so that the new and old callbacks are correctly documented.
Only the callbacks were implemented to simplify review process, following patches will make use of them.
Changes in v2 from Daniel:
- Update the warning about how modes aren't filtered in atomic_check - the heleprs help out a lot more now.
- Consistenly roll out that warning, crtc/encoder's atomic_check missed it.
- Sprinkle more links all over the place, so it's easier to see where this stuff is used and how the differen hooks are related.
- Note that ->mode_valid is optional everywhere.
- Explain why the connector's mode_valid is special and does _not_ get called in atomic_check.

Quoted 10 lines

Signed-off-by Jose Abreu <[email protected]>
Cc Jose Abreu <[email protected]>
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>
Signed-off-by Daniel Vetter <[email protected]> (v2)

Thanks! Looks fine by me.

Reviewed-by Jose Abreu <[email protected]>

Best regards,
Jose Miguel Abreu

Quoted 216 lines

---
 include/drm/drm_bridge.h                 |  31 +++++++++
 include/drm/drm_modeset_helper_vtables.h | 116 ++++++++++++++++++++++---------
 2 files changed, 114 insertions(+), 33 deletions(-)

include/drm/drm_bridge.h +31 -0

index fdd82fcbf168..f694de756ecf 100644
@@ -59,6 +59,31 @@ struct drm_bridge_funcs {
 	void (*detach)(struct drm_bridge *bridge);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * bridge. This should be implemented if the bridge has some sort of
+	 * restriction in the modes it can display. For example, a given bridge
+	 * 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in
+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc,
+					   const struct drm_display_mode *mode);
+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate and adjust a mode. The paramater

@@ -82,6 +107,12 @@ struct drm_bridge_funcs {
 	 * NOT touch any persistent state (hardware or software) or data
 	 * structures except the passed in @state parameter.
 	 *
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any bridge constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * True if an acceptable configuration is possible, false if the modeset

include/drm/drm_modeset_helper_vtables.h +76 ~~-33~~

index c01c328f6cc8..91d071ff1232 100644
@@ -106,6 +106,31 @@ struct drm_crtc_helper_funcs {
 	void (*commit)(struct drm_crtc *crtc);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * crtc. This 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in
+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * 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

@@ -132,20 +157,11 @@ struct drm_crtc_helper_funcs {
 	 * Atomic drivers which need to inspect and adjust more state should
 	 * instead use the @atomic_check callback.
 	 *
-	 * Also beware that neither core nor helpers filter modes before
-	 * passing them to the driver: While the list of modes that is
-	 * advertised to userspace is filtered using the
-	 * &drm_connector.mode_valid callback, neither the core nor the helpers
-	 * do any filtering on modes passed in from userspace when setting a
-	 * mode. It is therefore possible for userspace to pass in a mode that
-	 * was previously filtered out using &drm_connector.mode_valid or add a
-	 * custom mode that wasn't probed from EDID or similar to begin with.
-	 * Even though this is an advanced feature and rarely used nowadays,
-	 * some users rely on being able to specify modes manually so drivers
-	 * must be prepared to deal with it. Specifically this means that all
-	 * drivers need not only validate modes in &drm_connector.mode_valid but
-	 * also in this or in the &drm_encoder_helper_funcs.mode_fixup callback
-	 * to make sure invalid modes passed in from userspace are rejected.
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any CRTC constraints and
+	 * limits checks into @mode_valid.
 	 *
 	 * RETURNS:
 	 *

@@ -341,6 +357,12 @@ struct drm_crtc_helper_funcs {
 	 * state objects passed-in or assembled in the overall &drm_atomic_state
 	 * update tracking structure.
 	 *
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any CRTC constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * 0 on success, -EINVAL if the state or the transition can't be

@@ -457,6 +479,31 @@ struct drm_encoder_helper_funcs {
 	void (*dpms)(struct drm_encoder *encoder, int mode);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * encoder. This should be implemented if the encoder has some sort
+	 * of restriction in the modes it can display. For example, a given
+	 * encoder 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in
+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc,
+					   const struct drm_display_mode *mode);
+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate and adjust a mode. The parameter

@@ -482,21 +529,11 @@ struct drm_encoder_helper_funcs {
 	 * Atomic drivers which need to inspect and adjust more state should
 	 * instead use the @atomic_check callback.
 	 *
-	 * Also beware that neither core nor helpers filter modes before
-	 * passing them to the driver: While the list of modes that is
-	 * advertised to userspace is filtered using the connector's
-	 * &drm_connector_helper_funcs.mode_valid callback, neither the core nor
-	 * the helpers do any filtering on modes passed in from userspace when
-	 * setting a mode. It is therefore possible for userspace to pass in a
-	 * mode that was previously filtered out using
-	 * &drm_connector_helper_funcs.mode_valid or add a custom mode that
-	 * wasn't probed from EDID or similar to begin with.  Even though this
-	 * is an advanced feature and rarely used nowadays, some users rely on
-	 * being able to specify modes manually so drivers must be prepared to
-	 * deal with it. Specifically this means that all drivers need not only
-	 * validate modes in &drm_connector.mode_valid but also in this or in
-	 * the &drm_crtc_helper_funcs.mode_fixup callback to make sure
-	 * invalid modes passed in from userspace are rejected.
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any encoder constraints and
+	 * limits checks into @mode_valid.
 	 *
 	 * RETURNS:
 	 *

@@ -686,6 +723,12 @@ struct drm_encoder_helper_funcs {
 	 * state objects passed-in or assembled in the overall &drm_atomic_state
 	 * update tracking structure.
 	 *
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any encoder constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * 0 on success, -EINVAL if the state or the transition can't be

@@ -795,13 +838,20 @@ struct drm_connector_helper_funcs {
 	 * (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.
+	 *
 	 * NOTE:
 	 *
 	 * This only filters the mode list supplied to userspace in the
-	 * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and
-	 * ask the kernel to use them. It this case the atomic helpers or legacy
-	 * CRTC helpers will not call this function. Drivers therefore must
-	 * still fully validate any mode passed in in a modeset request.
+	 * GETCONNECTOR IOCTL. Compared to &drm_encoder_helper_funcs.mode_valid,
+	 * &drm_crtc_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid,

include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs { · line 857

Quoted 10 lines

include/drm/drm_modeset_helper_vtables.h +7 -0

+	 * which are also called by the atomic helpers from
+	 * drm_atomic_helper_check_modeset(). This allows userspace to force and
+	 * ignore sink constraint (like the pixel clock limits in the screen's
+	 * EDID), which is useful for e.g. testing, or working around a broken
+	 * EDID. Any source hardware constraint (which always need to be
+	 * enforced) therefore should be checked in one of the above callbacks,
+	 * and not this one here.
 	 *
 	 * To avoid races with concurrent connector state updates, the helper
 	 * libraries always call this with the &drm_mode_config.connection_mutex

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter

2017-05-15 06:50 UTC #5 lore raw

Re: [PATCH] drm: Add crtc/encoder/bridge->mode_valid() callbacks

In reply to Laurent Pinchart

Quoted 28 lines · Laurent Pinchart

Hi Daniel,
Quoted 14 lines · On Friday 12 May 2017 09:31:00 Daniel Vetter wrote:
From: Jose Abreu <[email protected]>
This adds a new callback to crtc, encoder and bridge helper functions called mode_valid(). This callback shall be implemented if the corresponding component has some sort of restriction in the modes that can be displayed. A NULL callback implicates that the component can display all the modes.
We also change the documentation so that the new and old callbacks are correctly documented.
Only the callbacks were implemented to simplify review process, following patches will make use of them.
Quoted 10 lines
Changes in v2 from Daniel:
- Update the warning about how modes aren't filtered in atomic_check - the heleprs help out a lot more now.
- Consistenly roll out that warning, crtc/encoder's atomic_check missed it.
- Sprinkle more links all over the place, so it's easier to see where this stuff is used and how the differen hooks are related.
- Note that ->mode_valid is optional everywhere.
- Explain why the connector's mode_valid is special and does _not_ get called in atomic_check.

Quoted 3 lines

As commented on v2 (but after you sent this patch), I think we need to document clearly how mode_valid and mode_fixup should interact. It's very confusing for driver authors at the moment.

I tried to do that here, is it not enough? Note that mode_fixup is kinda deprecated, in favour of atomic_check. So the equivalence is not as clear-cut as you seem to think.
-Daniel

Quoted 282 lines

Quoted 22 lines
Signed-off-by Jose Abreu <[email protected]>
Cc Jose Abreu <[email protected]>
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>
Signed-off-by Daniel Vetter <[email protected]> (v2)
---
 include/drm/drm_bridge.h                 |  31 +++++++++
 include/drm/drm_modeset_helper_vtables.h | 116
++++++++++++++++++++++--------- 2 files changed, 114 insertions(+), 33 deletions(-)
include/drm/drm_bridge.h +0 -0
index fdd82fcbf168..f694de756ecf 100644
include/drm/drm_bridge.h struct drm_bridge_funcs { · line 66
Quoted 8 lines
include/drm/drm_bridge.h +5 -0
@@ -59,6 +59,31 @@ struct drm_bridge_funcs {
 	void (*detach)(struct drm_bridge *bridge);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * bridge. This should be implemented if the bridge has some sort of
+	 * restriction in the modes it can display. For example, a given 
bridge
Quoted 8 lines
include/drm/drm_bridge.h +8 -0
+	 * 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in
include/drm/drm_bridge.h struct drm_bridge_funcs {
Quoted 10 lines
include/drm/drm_bridge.h +10 -0
+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc,
+					   const struct drm_display_mode 
*mode);
include/drm/drm_bridge.h struct drm_bridge_funcs { · line 110
Quoted 10 lines
include/drm/drm_bridge.h +3 -0
+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate and adjust a mode. The paramater
@@ -82,6 +107,12 @@ struct drm_bridge_funcs {
 	 * NOT touch any persistent state (hardware or software) or data
 	 * structures except the passed in @state parameter.
 	 *
+	 * Also beware that userspace can request its own custom modes, 
neither
include/drm/drm_bridge.h struct drm_bridge_funcs { · line 111
Quoted 1 line
include/drm/drm_bridge.h +1 -0
+	 * core nor helpers filter modes to the list of probe modes reported 
by
include/drm/drm_bridge.h struct drm_bridge_funcs { · line 112
Quoted 1 line
include/drm/drm_bridge.h +1 -0
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To 
ensure
include/drm/drm_bridge.h struct drm_bridge_funcs {
Quoted 6 lines
include/drm/drm_bridge.h +3 -0
+	 * that modes are filtered consistently put any bridge constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * True if an acceptable configuration is possible, false if the 
modeset
Quoted 22 lines
(quoted diff) +0 -0
b/include/drm/drm_modeset_helper_vtables.h index c01c328f6cc8..91d071ff1232 100644
include/drm/drm_modeset_helper_vtables.h +13 -0
@@ -106,6 +106,31 @@ struct drm_crtc_helper_funcs {
 	void (*commit)(struct drm_crtc *crtc);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * crtc. This 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in
include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 131
Quoted 10 lines
include/drm/drm_modeset_helper_vtables.h +10 -0
+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
+					   const struct drm_display_mode 
*mode);
include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 138
Quoted 13 lines
include/drm/drm_modeset_helper_vtables.h +2 -4
+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate a mode. The parameter mode is the
@@ -132,20 +157,11 @@ struct drm_crtc_helper_funcs {
 	 * Atomic drivers which need to inspect and adjust more state should
 	 * instead use the @atomic_check callback.
 	 *
-	 * Also beware that neither core nor helpers filter modes before
-	 * passing them to the driver: While the list of modes that is
-	 * advertised to userspace is filtered using the
-	 * &drm_connector.mode_valid callback, neither the core nor the 
helpers
include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs {
Quoted 3 lines
include/drm/drm_modeset_helper_vtables.h +0 -3
-	 * do any filtering on modes passed in from userspace when setting a
-	 * mode. It is therefore possible for userspace to pass in a mode that
-	 * was previously filtered out using &drm_connector.mode_valid or add 
a
include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs {
Quoted 5 lines
include/drm/drm_modeset_helper_vtables.h +0 -5
-	 * custom mode that wasn't probed from EDID or similar to begin with.
-	 * Even though this is an advanced feature and rarely used nowadays,
-	 * some users rely on being able to specify modes manually so drivers
-	 * must be prepared to deal with it. Specifically this means that all
-	 * drivers need not only validate modes in &drm_connector.mode_valid 
but
include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 147
Quoted 1 line
include/drm/drm_modeset_helper_vtables.h +0 -1
-	 * also in this or in the &drm_encoder_helper_funcs.mode_fixup 
callback
include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 160
Quoted 2 lines
include/drm/drm_modeset_helper_vtables.h +1 -1
-	 * to make sure invalid modes passed in from userspace are rejected.
+	 * Also beware that userspace can request its own custom modes, 
neither
include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 161
Quoted 1 line
include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * core nor helpers filter modes to the list of probe modes reported 
by
include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 162
Quoted 1 line
include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To 
ensure
include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 357
Quoted 7 lines
include/drm/drm_modeset_helper_vtables.h +2 -0
+	 * that modes are filtered consistently put any CRTC constraints and
+	 * limits checks into @mode_valid.
 	 *
 	 * RETURNS:
 	 *
@@ -341,6 +357,12 @@ struct drm_crtc_helper_funcs {
 	 * state objects passed-in or assembled in the overall 
&drm_atomic_state
include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs {
Quoted 3 lines
include/drm/drm_modeset_helper_vtables.h +1 -0
 	 * update tracking structure.
 	 *
+	 * Also beware that userspace can request its own custom modes, 
neither
include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 361
Quoted 1 line
include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * core nor helpers filter modes to the list of probe modes reported 
by
include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 362
Quoted 1 line
include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To 
ensure
Quoted 7 lines
include/drm/drm_modeset_helper_vtables.h +3 -0
+	 * that modes are filtered consistently put any CRTC constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * 0 on success, -EINVAL if the state or the transition can't be
include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 488
Quoted 10 lines
include/drm/drm_modeset_helper_vtables.h +7 -0
@@ -457,6 +479,31 @@ struct drm_encoder_helper_funcs {
 	void (*dpms)(struct drm_encoder *encoder, int mode);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * encoder. This should be implemented if the encoder has some sort
+	 * of restriction in the modes it can display. For example, a given
+	 * encoder may be responsible to set a clock value. If the clock can
+	 * not produce all the values for the available modes then this 
callback
Quoted 6 lines
include/drm/drm_modeset_helper_vtables.h +6 -0
+	 * can be used to restrict the number of modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in
include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs {
Quoted 10 lines
include/drm/drm_modeset_helper_vtables.h +10 -0
+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc,
+					   const struct drm_display_mode 
*mode);
include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 488
Quoted 13 lines
include/drm/drm_modeset_helper_vtables.h +2 -4
+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate and adjust a mode. The parameter
@@ -482,21 +529,11 @@ struct drm_encoder_helper_funcs {
 	 * Atomic drivers which need to inspect and adjust more state should
 	 * instead use the @atomic_check callback.
 	 *
-	 * Also beware that neither core nor helpers filter modes before
-	 * passing them to the driver: While the list of modes that is
-	 * advertised to userspace is filtered using the connector's
-	 * &drm_connector_helper_funcs.mode_valid callback, neither the core 
nor
include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs {
Quoted 8 lines
include/drm/drm_modeset_helper_vtables.h +0 -8
-	 * the helpers do any filtering on modes passed in from userspace when
-	 * setting a mode. It is therefore possible for userspace to pass in a
-	 * mode that was previously filtered out using
-	 * &drm_connector_helper_funcs.mode_valid or add a custom mode that
-	 * wasn't probed from EDID or similar to begin with.  Even though this
-	 * is an advanced feature and rarely used nowadays, some users rely on
-	 * being able to specify modes manually so drivers must be prepared to
-	 * deal with it. Specifically this means that all drivers need not 
only
include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs {
Quoted 4 lines
include/drm/drm_modeset_helper_vtables.h +1 -3
-	 * validate modes in &drm_connector.mode_valid but also in this or in
-	 * the &drm_crtc_helper_funcs.mode_fixup callback to make sure
-	 * invalid modes passed in from userspace are rejected.
+	 * Also beware that userspace can request its own custom modes, 
neither
include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 533
Quoted 1 line
include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * core nor helpers filter modes to the list of probe modes reported 
by
include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 534
Quoted 1 line
include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To 
ensure
include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 535
Quoted 1 line
include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * that modes are filtered consistently put any encoder constraints 
and
include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 723
Quoted 6 lines
include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * limits checks into @mode_valid.
 	 *
 	 * RETURNS:
 	 *
@@ -686,6 +723,12 @@ struct drm_encoder_helper_funcs {
 	 * state objects passed-in or assembled in the overall 
&drm_atomic_state
include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs {
Quoted 3 lines
include/drm/drm_modeset_helper_vtables.h +1 -0
 	 * update tracking structure.
 	 *
+	 * Also beware that userspace can request its own custom modes, 
neither
include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 727
Quoted 1 line
include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * core nor helpers filter modes to the list of probe modes reported 
by
include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 728
Quoted 1 line
include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To 
ensure
include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 729
Quoted 1 line
include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * that modes are filtered consistently put any encoder constraints 
and
Quoted 6 lines
include/drm/drm_modeset_helper_vtables.h +2 -0
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * 0 on success, -EINVAL if the state or the transition can't be
include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs { · line 801
Quoted 9 lines
include/drm/drm_modeset_helper_vtables.h +2 -1
@@ -795,13 +838,20 @@ struct drm_connector_helper_funcs {
 	 * (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.
+	 *
 	 * NOTE:
 	 *
 	 * This only filters the mode list supplied to userspace in the
-	 * GETCONNECOTR IOCTL. Userspace is free to create modes of its own 
and
include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs { · line 802
Quoted 1 line
include/drm/drm_modeset_helper_vtables.h +0 -1
-	 * ask the kernel to use them. It this case the atomic helpers or 
legacy
include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs {
Quoted 3 lines
include/drm/drm_modeset_helper_vtables.h +1 -2
-	 * CRTC helpers will not call this function. Drivers therefore must
-	 * still fully validate any mode passed in in a modeset request.
+	 * GETCONNECTOR IOCTL. Compared to 
&drm_encoder_helper_funcs.mode_valid,
include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs {
Quoted 3 lines
include/drm/drm_modeset_helper_vtables.h +3 -0
+	 * &drm_crtc_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid,
+	 * which are also called by the atomic helpers from
+	 * drm_atomic_helper_check_modeset(). This allows userspace to force 
and
include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs {
Quoted 4 lines
include/drm/drm_modeset_helper_vtables.h +4 -0
+	 * ignore sink constraint (like the pixel clock limits in the screen's
+	 * EDID), which is useful for e.g. testing, or working around a broken
+	 * EDID. Any source hardware constraint (which always need to be
+	 * enforced) therefore should be checked in one of the above 
callbacks,
include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs {
Quoted 4 lines
include/drm/drm_modeset_helper_vtables.h +1 -0
+	 * and not this one here.
 	 *
 	 * To avoid races with concurrent connector state updates, the helper
 	 * libraries always call this with the 
&drm_mode_config.connection_mutex
Signature · 3 lines
Regards,

Laurent Pinchart

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter

2017-05-15 06:56 UTC #7 lore raw

Re: [PATCH] drm: Add crtc/encoder/bridge->mode_valid() callbacks

In reply to Andrzej Hajda

Quoted 71 lines · Andrzej Hajda

Quoted 70 lines · Daniel Vetter
From: Jose Abreu <[email protected]>
This adds a new callback to crtc, encoder and bridge helper functions called mode_valid(). This callback shall be implemented if the corresponding component has some sort of restriction in the modes that can be displayed. A NULL callback implicates that the component can display all the modes.
We also change the documentation so that the new and old callbacks are correctly documented.
Only the callbacks were implemented to simplify review process, following patches will make use of them.
Changes in v2 from Daniel:
- Update the warning about how modes aren't filtered in atomic_check - the heleprs help out a lot more now.
- Consistenly roll out that warning, crtc/encoder's atomic_check missed it.
- Sprinkle more links all over the place, so it's easier to see where this stuff is used and how the differen hooks are related.
- Note that ->mode_valid is optional everywhere.
- Explain why the connector's mode_valid is special and does _not_ get called in atomic_check.
Signed-off-by Jose Abreu <[email protected]>
Cc Jose Abreu <[email protected]>
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>
Signed-off-by Daniel Vetter <[email protected]> (v2)
---
 include/drm/drm_bridge.h                 |  31 +++++++++
 include/drm/drm_modeset_helper_vtables.h | 116 ++++++++++++++++++++++---------
 2 files changed, 114 insertions(+), 33 deletions(-)
include/drm/drm_bridge.h +22 -0
index fdd82fcbf168..f694de756ecf 100644
@@ -59,6 +59,31 @@ struct drm_bridge_funcs {
 	void (*detach)(struct drm_bridge *bridge);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * bridge. This should be implemented if the bridge has some sort of
+	 * restriction in the modes it can display. For example, a given bridge
+	 * 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in
+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc,

Quoted 8 lines

include/drm/drm_bridge.h struct drm_bridge_funcs { · line 84
Quoted 1 line
include/drm/drm_bridge.h +1 -0
+					   const struct drm_display_mode *mode);
I have put my concerns here but it touches all mode_valid callbacks. As the callback can be called in mode probe and atomic check it could only inspect common set of properties for both contexts, for example crtc cannot check to which encoder it is connected, am I right? Maybe it would be good to emphasize it here, as it is emphasized in case of mode_fixup callbacks.

You can inspect nothing else but the mode here. Looking at anything else is not allowed since this is the generic check which should work for any config. Good question, so I'll augment the docs to address this.

Quoted 73 lines

include/drm/drm_bridge.h +8 -0

+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate and adjust a mode. The paramater

@@ -82,6 +107,12 @@ struct drm_bridge_funcs {
 	 * NOT touch any persistent state (hardware or software) or data
 	 * structures except the passed in @state parameter.
 	 *
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any bridge constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * True if an acceptable configuration is possible, false if the modeset

include/drm/drm_modeset_helper_vtables.h +26 ~~-14~~

index c01c328f6cc8..91d071ff1232 100644
@@ -106,6 +106,31 @@ struct drm_crtc_helper_funcs {
 	void (*commit)(struct drm_crtc *crtc);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * crtc. This 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in
+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * 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

@@ -132,20 +157,11 @@ struct drm_crtc_helper_funcs {
 	 * Atomic drivers which need to inspect and adjust more state should
 	 * instead use the @atomic_check callback.
 	 *
-	 * Also beware that neither core nor helpers filter modes before
-	 * passing them to the driver: While the list of modes that is
-	 * advertised to userspace is filtered using the
-	 * &drm_connector.mode_valid callback, neither the core nor the helpers
-	 * do any filtering on modes passed in from userspace when setting a
-	 * mode. It is therefore possible for userspace to pass in a mode that
-	 * was previously filtered out using &drm_connector.mode_valid or add a
-	 * custom mode that wasn't probed from EDID or similar to begin with.
-	 * Even though this is an advanced feature and rarely used nowadays,
-	 * some users rely on being able to specify modes manually so drivers
-	 * must be prepared to deal with it. Specifically this means that all
-	 * drivers need not only validate modes in &drm_connector.mode_valid but
-	 * also in this or in the &drm_encoder_helper_funcs.mode_fixup callback
-	 * to make sure invalid modes passed in from userspace are rejected.
+	 * Also beware that userspace can request its own custom modes, neither

Quoted 8 lines

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 164
Quoted 4 lines
include/drm/drm_modeset_helper_vtables.h +4 -0
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any CRTC constraints and
+	 * limits checks into @mode_valid.
Why we do not make mode_fixup obsolete for atomic drivers as atomic_check can do the same and is more powerful, this way we could avoid the overlapped functionality of mode_valid and mode_fixup.

What do you mean with "make obselete"? mode_fixup is the compat callback, atomic_check is the shiny new one, so it is already obselete.

Quoted 107 lines

include/drm/drm_modeset_helper_vtables.h +50 ~~-19~~

 	 *
 	 * RETURNS:
 	 *

@@ -341,6 +357,12 @@ struct drm_crtc_helper_funcs {
 	 * state objects passed-in or assembled in the overall &drm_atomic_state
 	 * update tracking structure.
 	 *
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any CRTC constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * 0 on success, -EINVAL if the state or the transition can't be

@@ -457,6 +479,31 @@ struct drm_encoder_helper_funcs {
 	void (*dpms)(struct drm_encoder *encoder, int mode);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * encoder. This should be implemented if the encoder has some sort
+	 * of restriction in the modes it can display. For example, a given
+	 * encoder 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in
+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc,
+					   const struct drm_display_mode *mode);
+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate and adjust a mode. The parameter

@@ -482,21 +529,11 @@ struct drm_encoder_helper_funcs {
 	 * Atomic drivers which need to inspect and adjust more state should
 	 * instead use the @atomic_check callback.
 	 *
-	 * Also beware that neither core nor helpers filter modes before
-	 * passing them to the driver: While the list of modes that is
-	 * advertised to userspace is filtered using the connector's
-	 * &drm_connector_helper_funcs.mode_valid callback, neither the core nor
-	 * the helpers do any filtering on modes passed in from userspace when
-	 * setting a mode. It is therefore possible for userspace to pass in a
-	 * mode that was previously filtered out using
-	 * &drm_connector_helper_funcs.mode_valid or add a custom mode that
-	 * wasn't probed from EDID or similar to begin with.  Even though this
-	 * is an advanced feature and rarely used nowadays, some users rely on
-	 * being able to specify modes manually so drivers must be prepared to
-	 * deal with it. Specifically this means that all drivers need not only
-	 * validate modes in &drm_connector.mode_valid but also in this or in
-	 * the &drm_crtc_helper_funcs.mode_fixup callback to make sure
-	 * invalid modes passed in from userspace are rejected.
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any encoder constraints and
+	 * limits checks into @mode_valid.
 	 *
 	 * RETURNS:
 	 *

@@ -686,6 +723,12 @@ struct drm_encoder_helper_funcs {
 	 * state objects passed-in or assembled in the overall &drm_atomic_state
 	 * update tracking structure.
 	 *
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any encoder constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * 0 on success, -EINVAL if the state or the transition can't be

@@ -795,13 +838,20 @@ struct drm_connector_helper_funcs {
 	 * (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.
+	 *
 	 * NOTE:
 	 *
 	 * This only filters the mode list supplied to userspace in the
-	 * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and
-	 * ask the kernel to use them. It this case the atomic helpers or legacy
-	 * CRTC helpers will not call this function. Drivers therefore must
-	 * still fully validate any mode passed in in a modeset request.
+	 * GETCONNECTOR IOCTL. Compared to &drm_encoder_helper_funcs.mode_valid,
+	 * &drm_crtc_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid,
+	 * which are also called by the atomic helpers from
+	 * drm_atomic_helper_check_modeset(). This allows userspace to force and
+	 * ignore sink constraint (like the pixel clock limits in the screen's
+	 * EDID), which is useful for e.g. testing, or working around a broken

Quoted 8 lines

include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs { · line 854
Quoted 3 lines
include/drm/drm_modeset_helper_vtables.h +3 -0
+	 * EDID. Any source hardware constraint (which always need to be
+	 * enforced) therefore should be checked in one of the above callbacks,
+	 * and not this one here.
The callback has the same name but different semantic, little bit weird. But do we really need connector::mode_valid then? I mean: are there connectors not bound to bridge/encoder which have their own constraints? If there are no such connectors, we can remove this callback.

Yes. And the pixel clock limit is exactly the current real-world use-case:
There are hdmi screens which have a pixel clock limit set, but in reality can take higher modes. There's users out there who want to use these higher modes on these peculiar screens. But for everyone else (and for all other screens), we do need to filter modes correctly (the example here is dual-link vs. single-link limits, which is an interaction between sink and source).

Quoted 2 lines

If they are rare, maybe just adding loop to connector::get_modes would be enough.

What do you mean with "adding a loop"?
-Daniel

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Andrzej Hajda

2017-05-15 08:10 UTC #8 lore raw

Re: [PATCH] drm: Add crtc/encoder/bridge->mode_valid() callbacks

Quoted 163 lines · Daniel Vetter

Quoted 70 lines · Andrzej Hajda
Quoted 69 lines · Daniel Vetter
From: Jose Abreu <[email protected]>
This adds a new callback to crtc, encoder and bridge helper functions called mode_valid(). This callback shall be implemented if the corresponding component has some sort of restriction in the modes that can be displayed. A NULL callback implicates that the component can display all the modes.
We also change the documentation so that the new and old callbacks are correctly documented.
Only the callbacks were implemented to simplify review process, following patches will make use of them.
Changes in v2 from Daniel:
- Update the warning about how modes aren't filtered in atomic_check - the heleprs help out a lot more now.
- Consistenly roll out that warning, crtc/encoder's atomic_check missed it.
- Sprinkle more links all over the place, so it's easier to see where this stuff is used and how the differen hooks are related.
- Note that ->mode_valid is optional everywhere.
- Explain why the connector's mode_valid is special and does _not_ get called in atomic_check.
Signed-off-by Jose Abreu <[email protected]>
Cc Jose Abreu <[email protected]>
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>
Signed-off-by Daniel Vetter <[email protected]> (v2)
---
 include/drm/drm_bridge.h                 |  31 +++++++++
 include/drm/drm_modeset_helper_vtables.h | 116 ++++++++++++++++++++++---------
 2 files changed, 114 insertions(+), 33 deletions(-)
include/drm/drm_bridge.h +21 -0
index fdd82fcbf168..f694de756ecf 100644
@@ -59,6 +59,31 @@ struct drm_bridge_funcs {
 	void (*detach)(struct drm_bridge *bridge);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * bridge. This should be implemented if the bridge has some sort of
+	 * restriction in the modes it can display. For example, a given bridge
+	 * 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in
+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
Quoted 8 lines
include/drm/drm_bridge.h struct drm_bridge_funcs { · line 84
Quoted 2 lines
include/drm/drm_bridge.h +2 -0
+	enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc,
+					   const struct drm_display_mode *mode);
I have put my concerns here but it touches all mode_valid callbacks. As the callback can be called in mode probe and atomic check it could only inspect common set of properties for both contexts, for example crtc cannot check to which encoder it is connected, am I right? Maybe it would be good to emphasize it here, as it is emphasized in case of mode_fixup callbacks.
You can inspect nothing else but the mode here. Looking at anything else is not allowed since this is the generic check which should work for any config. Good question, so I'll augment the docs to address this.
Quoted 72 lines
Quoted 72 lines
include/drm/drm_bridge.h +8 -0
+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate and adjust a mode. The paramater
@@ -82,6 +107,12 @@ struct drm_bridge_funcs {
 	 * NOT touch any persistent state (hardware or software) or data
 	 * structures except the passed in @state parameter.
 	 *
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any bridge constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * True if an acceptable configuration is possible, false if the modeset
include/drm/drm_modeset_helper_vtables.h +25 ~~-14~~
index c01c328f6cc8..91d071ff1232 100644
@@ -106,6 +106,31 @@ struct drm_crtc_helper_funcs {
 	void (*commit)(struct drm_crtc *crtc);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * crtc. This 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in
+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * 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
@@ -132,20 +157,11 @@ struct drm_crtc_helper_funcs {
 	 * Atomic drivers which need to inspect and adjust more state should
 	 * instead use the @atomic_check callback.
 	 *
-	 * Also beware that neither core nor helpers filter modes before
-	 * passing them to the driver: While the list of modes that is
-	 * advertised to userspace is filtered using the
-	 * &drm_connector.mode_valid callback, neither the core nor the helpers
-	 * do any filtering on modes passed in from userspace when setting a
-	 * mode. It is therefore possible for userspace to pass in a mode that
-	 * was previously filtered out using &drm_connector.mode_valid or add a
-	 * custom mode that wasn't probed from EDID or similar to begin with.
-	 * Even though this is an advanced feature and rarely used nowadays,
-	 * some users rely on being able to specify modes manually so drivers
-	 * must be prepared to deal with it. Specifically this means that all
-	 * drivers need not only validate modes in &drm_connector.mode_valid but
-	 * also in this or in the &drm_encoder_helper_funcs.mode_fixup callback
-	 * to make sure invalid modes passed in from userspace are rejected.
Quoted 8 lines
include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 164
Quoted 5 lines
include/drm/drm_modeset_helper_vtables.h +5 -0
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any CRTC constraints and
+	 * limits checks into @mode_valid.
Why we do not make mode_fixup obsolete for atomic drivers as atomic_check can do the same and is more powerful, this way we could avoid the overlapped functionality of mode_valid and mode_fixup.

Quoted 2 lines

What do you mean with "make obselete"? mode_fixup is the compat callback, atomic_check is the shiny new one, so it is already obselete.

Hmm, the docs says 'atomic drivers which need to inspect and adjust more state should instead use the @atomic_check callback', it does not sound like 'mode_fixup is obsolete for atomic drivers, please use atomic_check instead' :)

Quoted 122 lines

Quoted 106 lines

include/drm/drm_modeset_helper_vtables.h +49 ~~-19~~

 	 *
 	 * RETURNS:
 	 *

@@ -341,6 +357,12 @@ struct drm_crtc_helper_funcs {
 	 * state objects passed-in or assembled in the overall &drm_atomic_state
 	 * update tracking structure.
 	 *
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any CRTC constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * 0 on success, -EINVAL if the state or the transition can't be

@@ -457,6 +479,31 @@ struct drm_encoder_helper_funcs {
 	void (*dpms)(struct drm_encoder *encoder, int mode);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * encoder. This should be implemented if the encoder has some sort
+	 * of restriction in the modes it can display. For example, a given
+	 * encoder 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in
+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc,
+					   const struct drm_display_mode *mode);
+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate and adjust a mode. The parameter

@@ -482,21 +529,11 @@ struct drm_encoder_helper_funcs {
 	 * Atomic drivers which need to inspect and adjust more state should
 	 * instead use the @atomic_check callback.
 	 *
-	 * Also beware that neither core nor helpers filter modes before
-	 * passing them to the driver: While the list of modes that is
-	 * advertised to userspace is filtered using the connector's
-	 * &drm_connector_helper_funcs.mode_valid callback, neither the core nor
-	 * the helpers do any filtering on modes passed in from userspace when
-	 * setting a mode. It is therefore possible for userspace to pass in a
-	 * mode that was previously filtered out using
-	 * &drm_connector_helper_funcs.mode_valid or add a custom mode that
-	 * wasn't probed from EDID or similar to begin with.  Even though this
-	 * is an advanced feature and rarely used nowadays, some users rely on
-	 * being able to specify modes manually so drivers must be prepared to
-	 * deal with it. Specifically this means that all drivers need not only
-	 * validate modes in &drm_connector.mode_valid but also in this or in
-	 * the &drm_crtc_helper_funcs.mode_fixup callback to make sure
-	 * invalid modes passed in from userspace are rejected.
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any encoder constraints and
+	 * limits checks into @mode_valid.
 	 *
 	 * RETURNS:
 	 *

@@ -686,6 +723,12 @@ struct drm_encoder_helper_funcs {
 	 * state objects passed-in or assembled in the overall &drm_atomic_state
 	 * update tracking structure.
 	 *
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any encoder constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * 0 on success, -EINVAL if the state or the transition can't be

@@ -795,13 +838,20 @@ struct drm_connector_helper_funcs {
 	 * (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.
+	 *
 	 * NOTE:
 	 *
 	 * This only filters the mode list supplied to userspace in the
-	 * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and
-	 * ask the kernel to use them. It this case the atomic helpers or legacy
-	 * CRTC helpers will not call this function. Drivers therefore must
-	 * still fully validate any mode passed in in a modeset request.
+	 * GETCONNECTOR IOCTL. Compared to &drm_encoder_helper_funcs.mode_valid,
+	 * &drm_crtc_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid,
+	 * which are also called by the atomic helpers from
+	 * drm_atomic_helper_check_modeset(). This allows userspace to force and
+	 * ignore sink constraint (like the pixel clock limits in the screen's

Quoted 8 lines

include/drm/drm_modeset_helper_vtables.h struct drm_connector_helper_funcs { · line 854
Quoted 4 lines
include/drm/drm_modeset_helper_vtables.h +4 -0
+	 * EDID), which is useful for e.g. testing, or working around a broken
+	 * EDID. Any source hardware constraint (which always need to be
+	 * enforced) therefore should be checked in one of the above callbacks,
+	 * and not this one here.
The callback has the same name but different semantic, little bit weird. But do we really need connector::mode_valid then? I mean: are there connectors not bound to bridge/encoder which have their own constraints? If there are no such connectors, we can remove this callback.

Quoted 4 lines

Quoted 2 lines
If they are rare, maybe just adding loop to connector::get_modes would be enough.
What do you mean with "adding a loop"?
-Daniel

Sorry for confusing shortcut, I have just inspected call sites of both callbacks - drm_helper_probe_single_connector_modes, both callbacks are called from this function, so my idea was to move loop 'list_for_each_entry(mode, &connector->modes, head){ ...., ...->mode_valid(...); ....}' from drm_helper_probe_single_connector_modes to .get_modes callback. After looking bit more at this function I have realized that such change is not so obvious, and it is not really clear if the final result will be better :).

Regards
Andrzej

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter

2017-05-15 08:14 UTC #9 lore raw

Re: [PATCH] drm: Add crtc/encoder/bridge->mode_valid() callbacks

Quoted 9 lines · Andrzej Hajda

Sorry for confusing shortcut, I have just inspected call sites of both callbacks - drm_helper_probe_single_connector_modes, both callbacks are called from this function, so my idea was to move loop 'list_for_each_entry(mode, &connector->modes, head){ ...., ...->mode_valid(...); ....}' from drm_helper_probe_single_connector_modes to .get_modes callback. After looking bit more at this function I have realized that such change is not so obvious, and it is not really clear if the final result will be better :).

This would defeat the purpose of ->get_modes, which is to just grab the modes from the sink, without filtering for source constraints, which is done by ->mode_valid. That framework is why we have the probe helpers, if you want to do something else you can always implement ->fill_modes (which is the uapi entry point) yourself. -Daniel

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Andrzej Hajda

Reviewed-by on v3 2/6

2017-05-15 08:27 UTC #20 lore raw

Re: [PATCH v3 2/6] drm: Add drm_{crtc/encoder/connector}_mode_valid()

In reply to Jose Abreu

Quoted 95 lines · Jose Abreu

Add a new helper to call crtc->mode_valid, connector->mode_valid and encoder->mode_valid callbacks.

Signed-off-by Jose Abreu <[email protected]>

Cc Carlos Palminha <[email protected]>

Cc Alexey Brodkin <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Andrzej Hajda <[email protected]>

Cc Archit Taneja <[email protected]>

---
Changes v2->v3:
- Move helpers to drm_probe_helper.c (Daniel)
- Squeeze patches that introduce the helpers into a single one (Daniel)

 drivers/gpu/drm/drm_crtc_helper_internal.h | 13 ++++++++++
 drivers/gpu/drm/drm_probe_helper.c         | 38 ++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

drivers/gpu/drm/drm_crtc_helper_internal.h +13 -0

index 28295e5..97dfe20 100644
@@ -26,7 +26,11 @@
  * implementation details and are not exported to drivers.
  */
 
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
 #include <drm/drm_dp_helper.h>
+#include <drm/drm_encoder.h>
+#include <drm/drm_modes.h>
 
 /* drm_fb_helper.c */
 #ifdef CONFIG_DRM_FBDEV_EMULATION

@@ -62,4 +66,13 @@ static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
 static inline void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux)
 {
 }
+
+/* drm_probe_helper.c */
+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);
+
 #endif

drivers/gpu/drm/drm_probe_helper.c +28 -0

index 1b0c14a..f01abdc 100644
@@ -38,6 +38,9 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_modeset_helper_vtables.h>
+
+#include "drm_crtc_helper_internal.h"
 
 /**
  * DOC: output probing helper overview

@@ -113,6 +116,41 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
 	return 1;
 }
 
+enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
+					 const struct drm_display_mode *mode)
+{
+	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
+
+	if (!crtc_funcs || !crtc_funcs->mode_valid)
+		return MODE_OK;
+
+	return crtc_funcs->mode_valid(crtc, mode);
+}
+
+enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder *encoder,
+					    const struct drm_display_mode *mode)
+{
+	const struct drm_encoder_helper_funcs *encoder_funcs =
+		encoder->helper_private;
+
+	if (!encoder_funcs || !encoder_funcs->mode_valid)
+		return MODE_OK;
+
+	return encoder_funcs->mode_valid(encoder, mode);
+}
+
+enum drm_mode_status drm_connector_mode_valid(struct drm_connector *connector,
+					      struct drm_display_mode *mode)

drivers/gpu/drm/drm_probe_helper.c static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector) · line 153

Quoted 10 lines

drivers/gpu/drm/drm_probe_helper.c +10 -0

+{
+	const struct drm_connector_helper_funcs *connector_funcs =
+		connector->helper_private;
+
+	if (!connector_funcs || !connector_funcs->mode_valid)
+		return MODE_OK;
+
+	return connector_funcs->mode_valid(connector, mode);
+}
+

"Copy/Paste" as the main generic programming technique in C :) I guess clever/scary macro wouldn't be an option.

Anyway:

Reviewed-by Andrzej Hajda <[email protected]>

--
Regards
Andrzej

drivers/gpu/drm/drm_probe_helper.c static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector) · line 156

Quoted 3 lines

drivers/gpu/drm/drm_probe_helper.c +0 -0

 #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
 /**
  * drm_kms_helper_poll_enable - re-enable output polling.

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Andrzej Hajda

Reviewed-by on v3 4/6

2017-05-15 08:39 UTC #23 lore raw

Re: [PATCH v3 4/6] drm: Use new mode_valid() helpers in connector probe helper

In reply to Jose Abreu

Quoted 66 lines · Jose Abreu

This changes the connector probe helper function to use the new encoder->mode_valid(), bridge->mode_valid() and crtc->mode_valid() helper callbacks to validate the modes.
The new callbacks are optional so the behaviour remains the same if they are not implemented. If they are, then the code loops through all the connector's encodersXbridgesXcrtcs and calls the callback.
If at least a valid encoderXbridgeXcrtc combination is found which accepts the mode then the function returns MODE_OK.
Signed-off-by Jose Abreu <[email protected]>
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>
---
Changes v2->v3:
- Call also bridge->mode_valid (Daniel)
Changes v1->v2:
- Use new helpers suggested by Ville
- Change documentation (Daniel)
 drivers/gpu/drm/drm_probe_helper.c | 65 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 3 deletions(-)
drivers/gpu/drm/drm_probe_helper.c +27 -0
index f01abdc..84d660e 100644
@@ -83,6 +83,61 @@
 	return MODE_OK;
 }
 
+static enum drm_mode_status
+drm_mode_validate_connector(struct drm_connector *connector,
+			    struct drm_display_mode *mode)
+{
+	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 */
+	ret = drm_connector_mode_valid(connector, mode);
+	if (ret != MODE_OK)
+		return ret;
+
+	/* Step 2: Validate against encoders and crtcs */
+	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;
+
+		ret = drm_encoder_mode_valid(encoder, mode);
+		if (ret != 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

drivers/gpu/drm/drm_probe_helper.c line 122

Quoted 10 lines

drivers/gpu/drm/drm_probe_helper.c +10 -0

+			 * MODE_OK. */
+			continue;
+		}
+
+		ret = drm_bridge_mode_valid(encoder->bridge, mode);
+		if (ret != MODE_OK) {
+			/* There is also no point in continuing for crtc check
+			 * here. */
+			continue;
+		}

Maybe it is a bikeshedding, but wouldn't be better to call drm_bridge_mode_valid from drm_encoder_mode_valid, in general call all bridge related stuff from corresponding encoder stuff?
This is more question about role of encoder->bridge, should it be treated as encoder's extension, or as 1st class citizen in drm?

Another concern is about order of calls, it is from sink to source, to keep it consistent bridge should be called before encoder, am I right? Beside this:

Reviewed-by Andrzej Hajda <[email protected]>

--
Regards
Andrzej

Quoted 35 lines

drivers/gpu/drm/drm_probe_helper.c +23 -1

+
+		drm_for_each_crtc(crtc, dev) {
+			if (!drm_encoder_crtc_ok(encoder, crtc))
+				continue;
+
+			ret = drm_crtc_mode_valid(crtc, mode);
+			if (ret == 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. */
+				return ret;
+			}
+		}
+	}
+
+	return ret;
+}
+
 static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
 {
 	struct drm_cmdline_mode *cmdline_mode;

@@ -322,7 +377,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
  *    - 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 hardware specific checks
+ *      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
+ *      enforced by the modeset/atomic helpers
  *
  * 5. Any mode whose status is not OK is pruned from the connector's modes list,
  *    accompanied by a debug message indicating the reason for the mode's

drivers/gpu/drm/drm_probe_helper.c int drm_helper_probe_single_connector_modes(struct drm_connector *connector, · line 532

Quoted 10 lines

drivers/gpu/drm/drm_probe_helper.c +2 -2

@@ -466,8 +525,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);
 	}

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Andrzej Hajda

Reviewed-by on v3 5/6

2017-05-15 08:45 UTC #27 lore raw

Re: [PATCH v3 5/6] drm: Use mode_valid() in atomic modeset

In reply to Jose Abreu

Quoted 7 lines · Jose Abreu

This patches makes use of the new mode_valid() callbacks introduced previously to validate the full video pipeline when modesetting.
This calls the encoder->mode_valid(), bridge->mode_valid() and crtc->mode_valid() so that we can make sure that the mode will be accepted in every components.

Quoted 8 lines

Signed-off-by Jose Abreu <[email protected]>
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>

Reviewed-by Andrzej Hajda <[email protected]>

--
Regards
Andrzej

Daniel Vetter

2017-05-15 08:53 UTC #29 lore raw

Re: [PATCH v3 6/6] drm: arc: Use crtc->mode_valid() callback

In reply to Jose Abreu

Quoted 11 lines · Jose Abreu

Now that we have a callback to check if crtc supports a given mode we can use it in arcpgu so that we restrict the number of probbed modes to the ones we can actually display.
This is specially useful because arcpgu crtc is responsible to set a clock value in the commit() stage but unfortunatelly this clock does not support all the needed ranges.
Also, remove the atomic_check() callback as mode_valid() callback will be called before.

Quoted 8 lines

Signed-off-by Jose Abreu <[email protected]>
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>

Btw for justifying your patch series a bit more, would be real sweet to roll ->mode_valid out to more drivers. I see two easy cases:

bridge/analogix-anx78xx.c: has a simple mode_fixup which really is just a mode_valid callback

bridge/synopsys/dw-hdmi.c: simply hand-rolls what you've done here, we could move the connector_mode valid to the bridge and done.

Care to type these 2 patches on top, just to make this a bit more useful and a clearer case for merging?

Thanks, Daniel

Quoted 86 lines

---
 drivers/gpu/drm/arc/arcpgu_crtc.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

drivers/gpu/drm/arc/arcpgu_crtc.c +24 ~~-15~~

index ad9a959..01cae0a 100644
@@ -32,6 +32,18 @@
 	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 },
 };
 
+static bool arc_pgu_is_mode_valid(struct arcpgu_drm_private *arcpgu,
+				  const struct drm_display_mode *mode)
+{
+	long rate, clk_rate = mode->clock * 1000;
+
+	rate = clk_round_rate(arcpgu->clk, clk_rate);
+	if (rate != clk_rate)
+		return false;
+
+	return true;
+}
+
 static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
 {
 	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);

@@ -64,6 +76,17 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
 	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 };
 
+enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc,
+					     const struct drm_display_mode *mode)
+{
+	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
+
+	if (!arc_pgu_is_mode_valid(arcpgu, mode))
+		return MODE_NOCLOCK;
+
+	return MODE_OK;
+}
+
 static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
 	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);

@@ -129,20 +152,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
 			      ~ARCPGU_CTRL_ENABLE_MASK);
 }
 
-static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
-				     struct drm_crtc_state *state)
-{
-	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
-	struct drm_display_mode *mode = &state->adjusted_mode;
-	long rate, clk_rate = mode->clock * 1000;
-
-	rate = clk_round_rate(arcpgu->clk, clk_rate);
-	if (rate != clk_rate)
-		return -EINVAL;
-
-	return 0;
-}
-
 static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 				      struct drm_crtc_state *state)
 {

@@ -158,6 +167,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
+	.mode_valid	= arc_pgu_crtc_mode_valid,
 	.mode_set	= drm_helper_crtc_mode_set,
 	.mode_set_base	= drm_helper_crtc_mode_set_base,
 	.mode_set_nofb	= arc_pgu_crtc_mode_set_nofb,

@@ -165,7 +175,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 	.disable	= arc_pgu_crtc_disable,
 	.prepare	= arc_pgu_crtc_disable,
 	.commit		= arc_pgu_crtc_enable,
-	.atomic_check	= arc_pgu_crtc_atomic_check,
 	.atomic_begin	= arc_pgu_crtc_atomic_begin,
 };

Signature · 1 line

1.9.1

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Andrzej Hajda

Reviewed-by on v3 6/6

2017-05-15 08:54 UTC #32 lore raw

Re: [PATCH v3 6/6] drm: arc: Use crtc->mode_valid() callback

In reply to Jose Abreu

Quoted 11 lines · Jose Abreu

Now that we have a callback to check if crtc supports a given mode we can use it in arcpgu so that we restrict the number of probbed modes to the ones we can actually display.
This is specially useful because arcpgu crtc is responsible to set a clock value in the commit() stage but unfortunatelly this clock does not support all the needed ranges.
Also, remove the atomic_check() callback as mode_valid() callback will be called before.

Quoted 8 lines

Signed-off-by Jose Abreu <[email protected]>
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>

Reviewed-by Andrzej Hajda <[email protected]>

--
Regards
Andrzej

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter

2017-05-15 09:11 UTC #11 lore raw

PATCH 1/2 drm/doc: Document adjusted/request modes a bit better

In reply to Daniel Vetter

Laurent started a massive discussion on IRC about this. Let's try to document common usage a bit better.

v2: Cross-links+typos.

Cc Laurent Pinchart <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Jose Abreu <[email protected]>

Signed-off-by Daniel Vetter <[email protected]>

---
 include/drm/drm_bridge.h                 |  2 +-
 include/drm/drm_crtc.h                   | 28 +++++++++++++++++++++++++---
 include/drm/drm_modeset_helper_vtables.h |  6 ++++--
 3 files changed, 30 insertions(+), 6 deletions(-)

include/drm/drm_bridge.h +1 -1

index f694de756ecf..f3ad38d0d621 100644
@@ -91,7 +91,7 @@ struct drm_bridge_funcs {
 	 * the display chain, either the final &drm_connector or the next
 	 * &drm_bridge. The parameter adjusted_mode is the input mode the bridge
 	 * requires. It can be modified by this callback and does not need to
-	 * match mode.
+	 * match mode. See also &drm_crtc_state.adjusted_mode for more details.
 	 *
 	 * This is the only hook that allows a bridge to reject a modeset. If
 	 * this function passes all other callbacks must succeed for this

include/drm/drm_crtc.h +25 -3

index 06236b002c22..5f5d53973ca5 100644
@@ -90,8 +90,6 @@ struct drm_plane_helper_funcs;
  * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
  * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
  * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
- * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
- * @mode: current mode timings
  * @mode_blob: &drm_property_blob for @mode
  * @state: backpointer to global drm_atomic_state
  *

@@ -131,9 +129,33 @@ struct drm_crtc_state {
 	u32 connector_mask;
 	u32 encoder_mask;
 
-	/* adjusted_mode: for use by helpers and drivers */
+	/**
+	 * @adjusted_mode:
+	 *
+	 * Internal display timings which can be used by the driver to handle
+	 * differences between the mode requested by userspace in @mode and what
+	 * is actually programmed into the hardware. It is purely driver
+	 * implementation defined what exactly this adjusted mode means. Usually
+	 * it is used to store the hardware display timings used between the
+	 * CRTC and encoder blocks.
+	 */
 	struct drm_display_mode adjusted_mode;
 
+	/**
+	 * @mode:
+	 *
+	 * Display timings requested by userspace. The driver should try to
+	 * match the refresh rate as close as possible (but note that it's
+	 * undefined what exactly is close enough, e.g. some of the HDMI modes
+	 * only differ in less than 1% of the refresh rate). The active width
+	 * and height as observed by userspace for positioning planes must match
+	 * exactly.
+	 *
+	 * For external connectors where the sink isn't fixed (like with a
+	 * built-in panel), this mode here should match the physical mode on the
+	 * wire to the last details (i.e. including sync polarities and
+	 * everything).
+	 */
 	struct drm_display_mode mode;
 
 	/* blob property to expose current mode to atomic userspace */

include/drm/drm_modeset_helper_vtables.h +4 -2

index 91d071ff1232..c72fca544a41 100644
@@ -138,7 +138,8 @@ struct drm_crtc_helper_funcs {
 	 * encoders need to be fed with. Note that this is the inverse semantics
 	 * of the meaning for the &drm_encoder and &drm_bridge_funcs.mode_fixup
 	 * vfunc. If the CRTC cannot support the requested conversion from mode
-	 * to adjusted_mode it should reject the modeset.
+	 * to adjusted_mode it should reject the modeset. See also
+	 * &drm_crtc_state.adjusted_mode for more details.
 	 *
 	 * This function is used by both legacy CRTC helpers and atomic helpers.
 	 * With atomic helpers it is optional.

@@ -510,7 +511,8 @@ struct drm_encoder_helper_funcs {
 	 * mode is the display mode that should be fed to the next element in
 	 * the display chain, either the final &drm_connector or a &drm_bridge.
 	 * The parameter adjusted_mode is the input mode the encoder requires. It
-	 * can be modified by this callback and does not need to match mode.
+	 * can be modified by this callback and does not need to match mode. See
+	 * also &drm_crtc_state.adjusted_mode for more details.
 	 *
 	 * This function is used by both legacy CRTC helpers and atomic helpers.
 	 * This hook is optional.

Signature · 6 lines

2.11.0

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter

2017-05-15 09:11 UTC #12 lore raw

PATCH 2/2 drm/doc: Clarify mode_fixup vs. atomic_check a bit more

Brought up by both Laurent and Andrzej when reviewing the new ->mode_valid hooks. Since mode_fixup is just a simpler version of the much more generic atomic_check we can't really unify it with mode_valid. Most drivers should probably switch their current mode_fixup code to either the new mode_valid or the atomic_check hooks, but e.g. that doesn't exist yet for bridges, and for CRTCs the situation is a bit more complicated. Hence there's no clear equivalence between mode_fixup and mode_valid, even if it looks like that at first glance.

Cc Andrzej Hajda <[email protected]>

Cc Laurent Pinchart <[email protected]>

Cc Jose Abreu <[email protected]>

Signed-off-by Daniel Vetter <[email protected]>

---
 include/drm/drm_modeset_helper_vtables.h | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

include/drm/drm_modeset_helper_vtables.h +13 -2

index c72fca544a41..613b2a602b77 100644
@@ -156,7 +156,11 @@ struct drm_crtc_helper_funcs {
 	 * allowed.
 	 *
 	 * Atomic drivers which need to inspect and adjust more state should
-	 * instead use the @atomic_check callback.
+	 * instead use the @atomic_check callback, but note that they're not
+	 * perfectly equivalent: @mode_valid is called from
+	 * drm_atomic_helper_check_modeset(), but @atomic_check is called from
+	 * drm_atomic_helper_check_planes(), because originally it was meant for
+	 * plane update checks only..
 	 *
 	 * Also beware that userspace can request its own custom modes, neither
 	 * core nor helpers filter modes to the list of probe modes reported by

@@ -529,7 +533,9 @@ struct drm_encoder_helper_funcs {
 	 * allowed.
 	 *
 	 * Atomic drivers which need to inspect and adjust more state should
-	 * instead use the @atomic_check callback.
+	 * instead use the @atomic_check callback. If @atomic_check is used,
+	 * this hook isn't called since @atomic_check allows a strict superset
+	 * of the functionality of @mode_fixup.
 	 *
 	 * Also beware that userspace can request its own custom modes, neither
 	 * core nor helpers filter modes to the list of probe modes reported by

@@ -716,6 +722,11 @@ struct drm_encoder_helper_funcs {
 	 * update the CRTC to match what the encoder needs for the requested
 	 * connector.
 	 *
+	 * Since this provides a strict superset of the functionality of
+	 * @mode_fixup (the requested and adjusted modes are both available
+	 * through the passed in &struct drm_crtc_state) @mode_fixup is not
+	 * called when @atomic_check is implemented.
+	 *
 	 * This function is used by the atomic helpers, but it is optional.
 	 *
 	 * NOTE:

Signature · 6 lines

2.11.0

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter

2017-05-15 09:30 UTC #24 lore raw

Re: [PATCH v3 4/6] drm: Use new mode_valid() helpers in connector probe helper

In reply to Andrzej Hajda

Quoted 84 lines · Andrzej Hajda

Quoted 66 lines · Jose Abreu
This changes the connector probe helper function to use the new encoder->mode_valid(), bridge->mode_valid() and crtc->mode_valid() helper callbacks to validate the modes.
The new callbacks are optional so the behaviour remains the same if they are not implemented. If they are, then the code loops through all the connector's encodersXbridgesXcrtcs and calls the callback.
If at least a valid encoderXbridgeXcrtc combination is found which accepts the mode then the function returns MODE_OK.
Signed-off-by Jose Abreu <[email protected]>
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>
---
Changes v2->v3:
- Call also bridge->mode_valid (Daniel)
Changes v1->v2:
- Use new helpers suggested by Ville
- Change documentation (Daniel)
 drivers/gpu/drm/drm_probe_helper.c | 65 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 3 deletions(-)
drivers/gpu/drm/drm_probe_helper.c +27 -0
index f01abdc..84d660e 100644
@@ -83,6 +83,61 @@
 	return MODE_OK;
 }
 
+static enum drm_mode_status
+drm_mode_validate_connector(struct drm_connector *connector,
+			    struct drm_display_mode *mode)
+{
+	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 */
+	ret = drm_connector_mode_valid(connector, mode);
+	if (ret != MODE_OK)
+		return ret;
+
+	/* Step 2: Validate against encoders and crtcs */
+	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;
+
+		ret = drm_encoder_mode_valid(encoder, mode);
+		if (ret != 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
drivers/gpu/drm/drm_probe_helper.c line 122
Quoted 10 lines
drivers/gpu/drm/drm_probe_helper.c +10 -0
+			 * MODE_OK. */
+			continue;
+		}
+
+		ret = drm_bridge_mode_valid(encoder->bridge, mode);
+		if (ret != MODE_OK) {
+			/* There is also no point in continuing for crtc check
+			 * here. */
+			continue;
+		}
Maybe it is a bikeshedding, but wouldn't be better to call drm_bridge_mode_valid from drm_encoder_mode_valid, in general call all bridge related stuff from corresponding encoder stuff?
This is more question about role of encoder->bridge, should it be treated as encoder's extension, or as 1st class citizen in drm?

Quoted 2 lines

Another concern is about order of calls, it is from sink to source, to keep it consistent bridge should be called before encoder, am I right?

For the atomic_check stuff (where we do change the passed-in mode) this would be correct, and calling order and layering would matter. But this just validates the mode in turn with everything, not taking any cross-component constraint or other configuration-dependent constraints into account. Hence it doesn't matter in which order we call stuff.

Note that the passed-in mode is const, so you can't escape. And v3 of patch 1 now has added wording that you're not allowed to look at anything else dynamie either.

Does that address your concern?
-Daniel

Quoted 54 lines

Beside this:

Reviewed-by Andrzej Hajda <[email protected]>

--
Regards
Andrzej

Quoted 45 lines

drivers/gpu/drm/drm_probe_helper.c +25 -3

+
+		drm_for_each_crtc(crtc, dev) {
+			if (!drm_encoder_crtc_ok(encoder, crtc))
+				continue;
+
+			ret = drm_crtc_mode_valid(crtc, mode);
+			if (ret == 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. */
+				return ret;
+			}
+		}
+	}
+
+	return ret;
+}
+
 static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
 {
 	struct drm_cmdline_mode *cmdline_mode;

@@ -322,7 +377,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
  *    - 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 hardware specific checks
+ *      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
+ *      enforced by the modeset/atomic helpers
  *
  * 5. Any mode whose status is not OK is pruned from the connector's modes list,
  *    accompanied by a debug message indicating the reason for the mode's

@@ -466,8 +525,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);
 	}

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter

2017-05-15 09:33 UTC #17 lore raw

PATCH 1/1 drm: Add crtc/encoder/bridge->mode_valid() callbacks

In reply to Daniel Vetter

From: Jose Abreu <[email protected]>

We also change the documentation so that the new and old callbacks are correctly documented.

Only the callbacks were implemented to simplify review process, following patches will make use of them.

v3: Document what can and cannot be checked in mode_valid a bit better (Andrjez). Answer: Only allowed to look at the mode, nothing else.

Cc Laurent Pinchart <[email protected]>

Signed-off-by Jose Abreu <[email protected]>

Cc Jose Abreu <[email protected]>

Cc Carlos Palminha <[email protected]>

Cc Alexey Brodkin <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Dave Airlie <[email protected]>

Cc Andrzej Hajda <[email protected]>

Cc Archit Taneja <[email protected]>

Signed-off-by Daniel Vetter <[email protected]> (v2)

---
 include/drm/drm_bridge.h                 |  40 +++++++++
 include/drm/drm_modeset_helper_vtables.h | 134 +++++++++++++++++++++++--------
 2 files changed, 141 insertions(+), 33 deletions(-)

include/drm/drm_bridge.h +40 -0

index fdd82fcbf168..f703bafc2e30 100644
@@ -59,6 +59,40 @@ struct drm_bridge_funcs {
 	void (*detach)(struct drm_bridge *bridge);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * bridge. This should be implemented if the bridge has some sort of
+	 * restriction in the modes it can display. For example, a given bridge
+	 * 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 modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This hook is used by the probe helpers to filter the mode list in
+	 * drm_helper_probe_single_connector_modes(), and it is used by the
+	 * atomic helpers to validate modes supplied by userspace in
+	 * drm_atomic_helper_check_modeset().
+	 *
+	 * This function is optional.
+	 *
+	 * NOTE:
+	 *
+	 * Since this function is both called from the check phase of an atomic
+	 * commit, and the mode validation in the probe paths it is not allowed
+	 * to look at anything else but the passed-in mode, and validate it
+	 * against configuration-invariant hardward constraints. Any further
+	 * limits which depend upon the configuration can only be checked in
+	 * @mode_fixup.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_bridge *crtc,
+					   const struct drm_display_mode *mode);
+
+	/**
 	 * @mode_fixup:
 	 *
 	 * This callback is used to validate and adjust a mode. The paramater

@@ -82,6 +116,12 @@ struct drm_bridge_funcs {
 	 * NOT touch any persistent state (hardware or software) or data
 	 * structures except the passed in @state parameter.
 	 *
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any bridge constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 *
 	 * True if an acceptable configuration is possible, false if the modeset

include/drm/drm_modeset_helper_vtables.h +101 ~~-33~~

179 lines not on this page · raw message

Signature · 6 lines

2.11.0

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Andrzej Hajda

2017-05-15 09:51 UTC #25 lore raw

Re: [PATCH v3 4/6] drm: Use new mode_valid() helpers in connector probe helper

In reply to Daniel Vetter

Quoted 92 lines · Daniel Vetter

Quoted 83 lines · Andrzej Hajda
Quoted 66 lines · Jose Abreu
This changes the connector probe helper function to use the new encoder->mode_valid(), bridge->mode_valid() and crtc->mode_valid() helper callbacks to validate the modes.
The new callbacks are optional so the behaviour remains the same if they are not implemented. If they are, then the code loops through all the connector's encodersXbridgesXcrtcs and calls the callback.
If at least a valid encoderXbridgeXcrtc combination is found which accepts the mode then the function returns MODE_OK.
Signed-off-by Jose Abreu <[email protected]>
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>
---
Changes v2->v3:
- Call also bridge->mode_valid (Daniel)
Changes v1->v2:
- Use new helpers suggested by Ville
- Change documentation (Daniel)
 drivers/gpu/drm/drm_probe_helper.c | 65 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 3 deletions(-)
drivers/gpu/drm/drm_probe_helper.c +27 -0
index f01abdc..84d660e 100644
@@ -83,6 +83,61 @@
 	return MODE_OK;
 }
 
+static enum drm_mode_status
+drm_mode_validate_connector(struct drm_connector *connector,
+			    struct drm_display_mode *mode)
+{
+	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 */
+	ret = drm_connector_mode_valid(connector, mode);
+	if (ret != MODE_OK)
+		return ret;
+
+	/* Step 2: Validate against encoders and crtcs */
+	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;
+
+		ret = drm_encoder_mode_valid(encoder, mode);
+		if (ret != 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
drivers/gpu/drm/drm_probe_helper.c line 122
Quoted 10 lines
drivers/gpu/drm/drm_probe_helper.c +10 -0
+			 * MODE_OK. */
+			continue;
+		}
+
+		ret = drm_bridge_mode_valid(encoder->bridge, mode);
+		if (ret != MODE_OK) {
+			/* There is also no point in continuing for crtc check
+			 * here. */
+			continue;
+		}
Maybe it is a bikeshedding, but wouldn't be better to call drm_bridge_mode_valid from drm_encoder_mode_valid, in general call all bridge related stuff from corresponding encoder stuff?
This is more question about role of encoder->bridge, should it be treated as encoder's extension, or as 1st class citizen in drm?
Quoted 2 lines
Another concern is about order of calls, it is from sink to source, to keep it consistent bridge should be called before encoder, am I right?
For the atomic_check stuff (where we do change the passed-in mode) this would be correct, and calling order and layering would matter. But this just validates the mode in turn with everything, not taking any cross-component constraint or other configuration-dependent constraints into account. Hence it doesn't matter in which order we call stuff.

Quoted 5 lines

Note that the passed-in mode is const, so you can't escape. And v3 of patch 1 now has added wording that you're not allowed to look at anything else dynamie either.
Does that address your concern?

Yes, I know it practically does not matter. I have mistakenly written "Beside this: R-b", it should be rather "Anyway: R-b" :)

Regards
Andrzej

Quoted 55 lines

-Daniel

Quoted 53 lines

Beside this:

Reviewed-by Andrzej Hajda <[email protected]>

--
Regards
Andrzej

Quoted 45 lines

drivers/gpu/drm/drm_probe_helper.c +25 -3

+
+		drm_for_each_crtc(crtc, dev) {
+			if (!drm_encoder_crtc_ok(encoder, crtc))
+				continue;
+
+			ret = drm_crtc_mode_valid(crtc, mode);
+			if (ret == 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. */
+				return ret;
+			}
+		}
+	}
+
+	return ret;
+}
+
 static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
 {
 	struct drm_cmdline_mode *cmdline_mode;

@@ -322,7 +377,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
  *    - 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 hardware specific checks
+ *      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
+ *      enforced by the modeset/atomic helpers
  *
  * 5. Any mode whose status is not OK is pruned from the connector's modes list,
  *    accompanied by a debug message indicating the reason for the mode's

@@ -466,8 +525,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);
 	}

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter

2017-05-15 15:52 UTC #30 lore raw

Re: [PATCH v3 6/6] drm: arc: Use crtc->mode_valid() callback

In reply to Daniel Vetter

Quoted 24 lines · Daniel Vetter

Quoted 11 lines · Jose Abreu
Now that we have a callback to check if crtc supports a given mode we can use it in arcpgu so that we restrict the number of probbed modes to the ones we can actually display.
This is specially useful because arcpgu crtc is responsible to set a clock value in the commit() stage but unfortunatelly this clock does not support all the needed ranges.
Also, remove the atomic_check() callback as mode_valid() callback will be called before.
Quoted 8 lines
Signed-off-by Jose Abreu <[email protected]>
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>
Btw for justifying your patch series a bit more, would be real sweet to roll ->mode_valid out to more drivers. I see two easy cases:

Quoted 8 lines

bridge/analogix-anx78xx.c: has a simple mode_fixup which really is just a mode_valid callback
bridge/synopsys/dw-hdmi.c: simply hand-rolls what you've done here, we could move the connector_mode valid to the bridge and done.
Care to type these 2 patches on top, just to make this a bit more useful and a clearer case for merging?

Aside: There's a pile more drivers (encoders and crtc drivers) which would similarly benefit and dont really look like they'd be hard to patch up:
- malidp_crtc_mode_fixup
- armada_drm_crtc_mode_fixup (armada isn't atomic, so forget this one)
- atmel_hlcdc_crtc_mode_fixup
- hdmi_mode_fixup in exynos_hdmi.c
- vc4_crtc_mode_fixup and vc4_dpi_encoder_mode_fixup (they both just check for interlaced modes, we could probably dump the same check in connector->mode_valid).

Plus the 2 bridge drivers. Everyone else does either something more complex, or something that's not quite correct.
-Daniel

Quoted 94 lines

Thanks, Daniel

Quoted 86 lines

---
 drivers/gpu/drm/arc/arcpgu_crtc.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

drivers/gpu/drm/arc/arcpgu_crtc.c +24 ~~-15~~

index ad9a959..01cae0a 100644
@@ -32,6 +32,18 @@
 	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 },
 };
 
+static bool arc_pgu_is_mode_valid(struct arcpgu_drm_private *arcpgu,
+				  const struct drm_display_mode *mode)
+{
+	long rate, clk_rate = mode->clock * 1000;
+
+	rate = clk_round_rate(arcpgu->clk, clk_rate);
+	if (rate != clk_rate)
+		return false;
+
+	return true;
+}
+
 static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
 {
 	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);

@@ -64,6 +76,17 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
 	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 };
 
+enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc,
+					     const struct drm_display_mode *mode)
+{
+	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
+
+	if (!arc_pgu_is_mode_valid(arcpgu, mode))
+		return MODE_NOCLOCK;
+
+	return MODE_OK;
+}
+
 static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
 	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);

@@ -129,20 +152,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
 			      ~ARCPGU_CTRL_ENABLE_MASK);
 }
 
-static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
-				     struct drm_crtc_state *state)
-{
-	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
-	struct drm_display_mode *mode = &state->adjusted_mode;
-	long rate, clk_rate = mode->clock * 1000;
-
-	rate = clk_round_rate(arcpgu->clk, clk_rate);
-	if (rate != clk_rate)
-		return -EINVAL;
-
-	return 0;
-}
-
 static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 				      struct drm_crtc_state *state)
 {

@@ -158,6 +167,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
+	.mode_valid	= arc_pgu_crtc_mode_valid,
 	.mode_set	= drm_helper_crtc_mode_set,
 	.mode_set_base	= drm_helper_crtc_mode_set_base,
 	.mode_set_nofb	= arc_pgu_crtc_mode_set_nofb,

@@ -165,7 +175,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 	.disable	= arc_pgu_crtc_disable,
 	.prepare	= arc_pgu_crtc_disable,
 	.commit		= arc_pgu_crtc_enable,
-	.atomic_check	= arc_pgu_crtc_atomic_check,
 	.atomic_begin	= arc_pgu_crtc_atomic_begin,
 };

Signature · 1 line

1.9.1

Signature · 3 lines

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

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Jose Abreu

author

2017-05-15 23:55 UTC #31 lore raw

Re: [PATCH v3 6/6] drm: arc: Use crtc->mode_valid() callback

Hi Daniel,

Quoted 42 lines · Daniel Vetter

Quoted 23 lines · Daniel Vetter
Quoted 11 lines · Jose Abreu
Now that we have a callback to check if crtc supports a given mode we can use it in arcpgu so that we restrict the number of probbed modes to the ones we can actually display.
This is specially useful because arcpgu crtc is responsible to set a clock value in the commit() stage but unfortunatelly this clock does not support all the needed ranges.
Also, remove the atomic_check() callback as mode_valid() callback will be called before.
Quoted 8 lines
Signed-off-by Jose Abreu <[email protected]>
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>
Btw for justifying your patch series a bit more, would be real sweet to roll ->mode_valid out to more drivers. I see two easy cases:
Quoted 8 lines
bridge/analogix-anx78xx.c: has a simple mode_fixup which really is just a mode_valid callback
bridge/synopsys/dw-hdmi.c: simply hand-rolls what you've done here, we could move the connector_mode valid to the bridge and done.
Care to type these 2 patches on top, just to make this a bit more useful and a clearer case for merging?
Aside: There's a pile more drivers (encoders and crtc drivers) which would similarly benefit and dont really look like they'd be hard to patch up:
- malidp_crtc_mode_fixup
- armada_drm_crtc_mode_fixup (armada isn't atomic, so forget this one)
- atmel_hlcdc_crtc_mode_fixup
- hdmi_mode_fixup in exynos_hdmi.c
- vc4_crtc_mode_fixup and vc4_dpi_encoder_mode_fixup (they both just check for interlaced modes, we could probably dump the same check in connector->mode_valid).

Quoted 3 lines

Plus the 2 bridge drivers. Everyone else does either something more complex, or something that's not quite correct.
-Daniel

Sorry for being silent but I am away from office this week. I will try to evaluate and add this in next version. In the meantime I will review the documentation patches you sent. Thanks!

Best regards,
Jose Miguel Abreu

Quoted 93 lines

Thanks, Daniel

Quoted 86 lines

---
 drivers/gpu/drm/arc/arcpgu_crtc.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

drivers/gpu/drm/arc/arcpgu_crtc.c +24 ~~-15~~

index ad9a959..01cae0a 100644
@@ -32,6 +32,18 @@
 	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 },
 };
 
+static bool arc_pgu_is_mode_valid(struct arcpgu_drm_private *arcpgu,
+				  const struct drm_display_mode *mode)
+{
+	long rate, clk_rate = mode->clock * 1000;
+
+	rate = clk_round_rate(arcpgu->clk, clk_rate);
+	if (rate != clk_rate)
+		return false;
+
+	return true;
+}
+
 static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
 {
 	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);

@@ -64,6 +76,17 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
 	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 };
 
+enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc,
+					     const struct drm_display_mode *mode)
+{
+	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
+
+	if (!arc_pgu_is_mode_valid(arcpgu, mode))
+		return MODE_NOCLOCK;
+
+	return MODE_OK;
+}
+
 static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
 	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);

@@ -129,20 +152,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
 			      ~ARCPGU_CTRL_ENABLE_MASK);
 }
 
-static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
-				     struct drm_crtc_state *state)
-{
-	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
-	struct drm_display_mode *mode = &state->adjusted_mode;
-	long rate, clk_rate = mode->clock * 1000;
-
-	rate = clk_round_rate(arcpgu->clk, clk_rate);
-	if (rate != clk_rate)
-		return -EINVAL;
-
-	return 0;
-}
-
 static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 				      struct drm_crtc_state *state)
 {

@@ -158,6 +167,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
+	.mode_valid	= arc_pgu_crtc_mode_valid,
 	.mode_set	= drm_helper_crtc_mode_set,
 	.mode_set_base	= drm_helper_crtc_mode_set_base,
 	.mode_set_nofb	= arc_pgu_crtc_mode_set_nofb,

@@ -165,7 +175,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
 	.disable	= arc_pgu_crtc_disable,
 	.prepare	= arc_pgu_crtc_disable,
 	.commit		= arc_pgu_crtc_enable,
-	.atomic_check	= arc_pgu_crtc_atomic_check,
 	.atomic_begin	= arc_pgu_crtc_atomic_begin,
 };

Signature · 1 line

1.9.1

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
https://urldefense.proofpoint.com/v2/url?u=http-3A__blog.ffwll.ch&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=yaVFU4TjGY0gVF8El1uKcisy6TPsyCl9uN7Wsis-qhY&m=v9tZNfdrkm8mrAlJyEpm71Te7tDReAFXijS23BkDSTo&s=Y6DG3RpFLs0nomQ09FA3hqCqq6MQ3dvG81s_2XlVytM&e=

Jose Abreu

author Reviewed-by on v1 1/2

2017-05-16 02:39 UTC #15 lore raw

Re: [PATCH 1/2] drm/doc: Document adjusted/request modes a bit better

In reply to Daniel Vetter

Hi Daniel,

Quoted 9 lines · Daniel Vetter

Laurent started a massive discussion on IRC about this. Let's try to document common usage a bit better.
v2: Cross-links+typos.
Cc Laurent Pinchart <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Jose Abreu <[email protected]>
Signed-off-by Daniel Vetter <[email protected]>

Reviewed-by Jose Abreu <[email protected]>

Best regards,
Jose Miguel Abreu

Quoted 82 lines

---
 include/drm/drm_bridge.h                 |  2 +-
 include/drm/drm_crtc.h                   | 28 +++++++++++++++++++++++++---
 include/drm/drm_modeset_helper_vtables.h |  6 ++++--
 3 files changed, 30 insertions(+), 6 deletions(-)

include/drm/drm_bridge.h +1 -1

index f694de756ecf..f3ad38d0d621 100644
@@ -91,7 +91,7 @@ struct drm_bridge_funcs {
 	 * the display chain, either the final &drm_connector or the next
 	 * &drm_bridge. The parameter adjusted_mode is the input mode the bridge
 	 * requires. It can be modified by this callback and does not need to
-	 * match mode.
+	 * match mode. See also &drm_crtc_state.adjusted_mode for more details.
 	 *
 	 * This is the only hook that allows a bridge to reject a modeset. If
 	 * this function passes all other callbacks must succeed for this

include/drm/drm_crtc.h +25 -3

index 06236b002c22..5f5d53973ca5 100644
@@ -90,8 +90,6 @@ struct drm_plane_helper_funcs;
  * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
  * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
  * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached encoders
- * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
- * @mode: current mode timings
  * @mode_blob: &drm_property_blob for @mode
  * @state: backpointer to global drm_atomic_state
  *

@@ -131,9 +129,33 @@ struct drm_crtc_state {
 	u32 connector_mask;
 	u32 encoder_mask;
 
-	/* adjusted_mode: for use by helpers and drivers */
+	/**
+	 * @adjusted_mode:
+	 *
+	 * Internal display timings which can be used by the driver to handle
+	 * differences between the mode requested by userspace in @mode and what
+	 * is actually programmed into the hardware. It is purely driver
+	 * implementation defined what exactly this adjusted mode means. Usually
+	 * it is used to store the hardware display timings used between the
+	 * CRTC and encoder blocks.
+	 */
 	struct drm_display_mode adjusted_mode;
 
+	/**
+	 * @mode:
+	 *
+	 * Display timings requested by userspace. The driver should try to
+	 * match the refresh rate as close as possible (but note that it's
+	 * undefined what exactly is close enough, e.g. some of the HDMI modes
+	 * only differ in less than 1% of the refresh rate). The active width
+	 * and height as observed by userspace for positioning planes must match
+	 * exactly.
+	 *
+	 * For external connectors where the sink isn't fixed (like with a
+	 * built-in panel), this mode here should match the physical mode on the
+	 * wire to the last details (i.e. including sync polarities and
+	 * everything).
+	 */
 	struct drm_display_mode mode;
 
 	/* blob property to expose current mode to atomic userspace */

include/drm/drm_modeset_helper_vtables.h +2 -1

index 91d071ff1232..c72fca544a41 100644
@@ -138,7 +138,8 @@ struct drm_crtc_helper_funcs {
 	 * encoders need to be fed with. Note that this is the inverse semantics
 	 * of the meaning for the &drm_encoder and &drm_bridge_funcs.mode_fixup
 	 * vfunc. If the CRTC cannot support the requested conversion from mode
-	 * to adjusted_mode it should reject the modeset.
+	 * to adjusted_mode it should reject the modeset. See also
+	 * &drm_crtc_state.adjusted_mode for more details.
 	 *
 	 * This function is used by both legacy CRTC helpers and atomic helpers.
 	 * With atomic helpers it is optional.

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 518

Quoted 9 lines

include/drm/drm_modeset_helper_vtables.h +2 -1

@@ -510,7 +511,8 @@ struct drm_encoder_helper_funcs {
 	 * mode is the display mode that should be fed to the next element in
 	 * the display chain, either the final &drm_connector or a &drm_bridge.
 	 * The parameter adjusted_mode is the input mode the encoder requires. It
-	 * can be modified by this callback and does not need to match mode.
+	 * can be modified by this callback and does not need to match mode. See
+	 * also &drm_crtc_state.adjusted_mode for more details.
 	 *
 	 * This function is used by both legacy CRTC helpers and atomic helpers.
 	 * This hook is optional.

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Jose Abreu

author Reviewed-by on v1 2/2

2017-05-16 02:41 UTC #13 lore raw

Re: [PATCH 2/2] drm/doc: Clarify mode_fixup vs. atomic_check a bit more

In reply to Daniel Vetter

Hi Daniel,

Quoted 10 lines · Daniel Vetter

Brought up by both Laurent and Andrzej when reviewing the new ->mode_valid hooks. Since mode_fixup is just a simpler version of the much more generic atomic_check we can't really unify it with mode_valid. Most drivers should probably switch their current mode_fixup code to either the new mode_valid or the atomic_check hooks, but e.g. that doesn't exist yet for bridges, and for CRTCs the situation is a bit more complicated. Hence there's no clear equivalence between mode_fixup and mode_valid, even if it looks like that at first glance.

Quoted 4 lines

Cc Andrzej Hajda <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc Jose Abreu <[email protected]>
Signed-off-by Daniel Vetter <[email protected]>

Reviewed-by Jose Abreu <[email protected]>

Best regards,
Jose Miguel Abreu

Quoted 34 lines

---
 include/drm/drm_modeset_helper_vtables.h | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

include/drm/drm_modeset_helper_vtables.h +8 -2

index c72fca544a41..613b2a602b77 100644
@@ -156,7 +156,11 @@ struct drm_crtc_helper_funcs {
 	 * allowed.
 	 *
 	 * Atomic drivers which need to inspect and adjust more state should
-	 * instead use the @atomic_check callback.
+	 * instead use the @atomic_check callback, but note that they're not
+	 * perfectly equivalent: @mode_valid is called from
+	 * drm_atomic_helper_check_modeset(), but @atomic_check is called from
+	 * drm_atomic_helper_check_planes(), because originally it was meant for
+	 * plane update checks only..
 	 *
 	 * Also beware that userspace can request its own custom modes, neither
 	 * core nor helpers filter modes to the list of probe modes reported by

@@ -529,7 +533,9 @@ struct drm_encoder_helper_funcs {
 	 * allowed.
 	 *
 	 * Atomic drivers which need to inspect and adjust more state should
-	 * instead use the @atomic_check callback.
+	 * instead use the @atomic_check callback. If @atomic_check is used,
+	 * this hook isn't called since @atomic_check allows a strict superset
+	 * of the functionality of @mode_fixup.
 	 *
 	 * Also beware that userspace can request its own custom modes, neither
 	 * core nor helpers filter modes to the list of probe modes reported by

@@ -716,6 +722,11 @@ struct drm_encoder_helper_funcs {
 	 * update the CRTC to match what the encoder needs for the requested

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 732

Quoted 10 lines

include/drm/drm_modeset_helper_vtables.h +5 -0

 	 * connector.
 	 *
+	 * Since this provides a strict superset of the functionality of
+	 * @mode_fixup (the requested and adjusted modes are both available
+	 * through the passed in &struct drm_crtc_state) @mode_fixup is not
+	 * called when @atomic_check is implemented.
+	 *
 	 * This function is used by the atomic helpers, but it is optional.
 	 *
 	 * NOTE:

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Andrzej Hajda

Reviewed-by on v1 2/2

2017-05-16 13:14 UTC #14 lore raw

Re: [PATCH 2/2] drm/doc: Clarify mode_fixup vs. atomic_check a bit more

In reply to Daniel Vetter

Quoted 23 lines · Daniel Vetter

Brought up by both Laurent and Andrzej when reviewing the new ->mode_valid hooks. Since mode_fixup is just a simpler version of the much more generic atomic_check we can't really unify it with mode_valid. Most drivers should probably switch their current mode_fixup code to either the new mode_valid or the atomic_check hooks, but e.g. that doesn't exist yet for bridges, and for CRTCs the situation is a bit more complicated. Hence there's no clear equivalence between mode_fixup and mode_valid, even if it looks like that at first glance.
Cc Andrzej Hajda <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc Jose Abreu <[email protected]>
Signed-off-by Daniel Vetter <[email protected]>
---
 include/drm/drm_modeset_helper_vtables.h | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)
include/drm/drm_modeset_helper_vtables.h +0 -0
index c72fca544a41..613b2a602b77 100644

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs { · line 163

Quoted 9 lines

include/drm/drm_modeset_helper_vtables.h +5 -1

@@ -156,7 +156,11 @@ struct drm_crtc_helper_funcs {
 	 * allowed.
 	 *
 	 * Atomic drivers which need to inspect and adjust more state should
-	 * instead use the @atomic_check callback.
+	 * instead use the @atomic_check callback, but note that they're not
+	 * perfectly equivalent: @mode_valid is called from
+	 * drm_atomic_helper_check_modeset(), but @atomic_check is called from
+	 * drm_atomic_helper_check_planes(), because originally it was meant for
+	 * plane update checks only..

Two dots.

Beside this:

Reviewed-by Andrzej Hajda <[email protected]>

--
Regards
Andrzej

Quoted 16 lines

include/drm/drm_modeset_helper_vtables.h +3 -1

 	 *
 	 * Also beware that userspace can request its own custom modes, neither
 	 * core nor helpers filter modes to the list of probe modes reported by

@@ -529,7 +533,9 @@ struct drm_encoder_helper_funcs {
 	 * allowed.
 	 *
 	 * Atomic drivers which need to inspect and adjust more state should
-	 * instead use the @atomic_check callback.
+	 * instead use the @atomic_check callback. If @atomic_check is used,
+	 * this hook isn't called since @atomic_check allows a strict superset
+	 * of the functionality of @mode_fixup.
 	 *
 	 * Also beware that userspace can request its own custom modes, neither
 	 * core nor helpers filter modes to the list of probe modes reported by

@@ -716,6 +722,11 @@ struct drm_encoder_helper_funcs {
 	 * update the CRTC to match what the encoder needs for the requested

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs { · line 732

Quoted 10 lines

include/drm/drm_modeset_helper_vtables.h +5 -0

 	 * connector.
 	 *
+	 * Since this provides a strict superset of the functionality of
+	 * @mode_fixup (the requested and adjusted modes are both available
+	 * through the passed in &struct drm_crtc_state) @mode_fixup is not
+	 * called when @atomic_check is implemented.
+	 *
 	 * This function is used by the atomic helpers, but it is optional.
 	 *
 	 * NOTE:

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Andrzej Hajda

Reviewed-by on v1 1/1

2017-05-16 13:15 UTC #18 lore raw

Re: [PATCH] drm: Add crtc/encoder/bridge->mode_valid() callbacks

In reply to Daniel Vetter

Quoted 28 lines · Daniel Vetter

From: Jose Abreu <[email protected]>
This adds a new callback to crtc, encoder and bridge helper functions called mode_valid(). This callback shall be implemented if the corresponding component has some sort of restriction in the modes that can be displayed. A NULL callback implicates that the component can display all the modes.
We also change the documentation so that the new and old callbacks are correctly documented.
Only the callbacks were implemented to simplify review process, following patches will make use of them.
Changes in v2 from Daniel:
- Update the warning about how modes aren't filtered in atomic_check - the heleprs help out a lot more now.
- Consistenly roll out that warning, crtc/encoder's atomic_check missed it.
- Sprinkle more links all over the place, so it's easier to see where this stuff is used and how the differen hooks are related.
- Note that ->mode_valid is optional everywhere.
- Explain why the connector's mode_valid is special and does _not_ get called in atomic_check.
v3: Document what can and cannot be checked in mode_valid a bit better (Andrjez). Answer: Only allowed to look at the mode, nothing else.

Quoted 11 lines

Cc Laurent Pinchart <[email protected]>
Signed-off-by Jose Abreu <[email protected]>
Cc Jose Abreu <[email protected]>
Cc Carlos Palminha <[email protected]>
Cc Alexey Brodkin <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Andrzej Hajda <[email protected]>
Cc Archit Taneja <[email protected]>
Signed-off-by Daniel Vetter <[email protected]> (v2)

Reviewed-by Andrzej Hajda <[email protected]>

--
Regards
Andrzej
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Andrzej Hajda

Reviewed-by on v1 1/2

2017-05-16 13:17 UTC #16 lore raw

Re: [PATCH 1/2] drm/doc: Document adjusted/request modes a bit better

In reply to Daniel Vetter

Quoted 9 lines · Daniel Vetter

Laurent started a massive discussion on IRC about this. Let's try to document common usage a bit better.
v2: Cross-links+typos.
Cc Laurent Pinchart <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Jose Abreu <[email protected]>
Signed-off-by Daniel Vetter <[email protected]>

Reviewed-by Andrzej Hajda <[email protected]>

--
Regards
Andrzej
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel