Introduce new mode validation callbacks

Add a new helper to call crtc->mode_valid callback.

Suggested-by Ville Syrjälä <[email protected]>

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_crtc.c          | 22 ++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc_internal.h |  3 +++
 2 files changed, 25 insertions(+)

drivers/gpu/drm/drm_crtc.c +5 -0

index 5af25ce..07ae705 100644
@@ -38,6 +38,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_modeset_lock.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_auth.h>

@@ -741,3 +742,24 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
 
 	return ret;
 }
+
+/**
+ * drm_crtc_mode_valid - call crtc->mode_valid callback, if any.
+ * @crtc: crtc

drivers/gpu/drm/drm_crtc.c +7 -0

+
+	if (!crtc_funcs || !crtc_funcs->mode_valid)
+		return MODE_OK;
+
+	return crtc_funcs->mode_valid(crtc, mode);
+}
+EXPORT_SYMBOL(drm_crtc_mode_valid);

drivers/gpu/drm/drm_crtc_internal.h +3 -0

index d077c54..3800abd 100644
@@ -45,6 +45,9 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
 
 struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc);
 
+enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
+					 const struct drm_display_mode *mode);
+
 /* IOCTLs */
 int drm_mode_getcrtc(struct drm_device *dev,
 		     void *data, struct drm_file *file_priv);

Signature · 1 line

1.9.1

This changes the connector probe helper function to use the new encoder->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 encodersXcrtcs and calls the callback.

If at least a valid encoderXcrtc 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 v1->v2:
- Use new helpers suggested by Ville
- Change documentation (Daniel)

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

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

index 1b0c14a..de47413 100644
@@ -39,6 +39,8 @@
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_edid.h>
 
+#include "drm_crtc_internal.h"
+
 /**
  * DOC: output probing helper overview
  *

@@ -80,6 +82,54 @@
 	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)

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;

@@ -284,7 +334,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 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

@@ -428,8 +482,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

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 description of connector->mode_valid() callback so that it matches the existing behaviour: It is never called in atomic check phase.

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 v1->v2:
- Change description of connector->mode_valid() (Daniel)

 include/drm/drm_bridge.h                 | 20 ++++++++++++++
 include/drm/drm_modeset_helper_vtables.h | 45 ++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

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 +40 -0

index c01c328..eec2c70 100644
@@ -106,6 +106,26 @@ 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

@@ -457,6 +477,26 @@ 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

@@ -795,6 +835,11 @@ struct drm_connector_helper_funcs {
 	 * (which is usually derived from the EDID data block from the sink).

Quoted 42 lines · Jose Abreu

Add a new helper to call crtc->mode_valid callback.

Suggested-by Ville Syrjälä <[email protected]>

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_crtc.c          | 22 ++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc_internal.h |  3 +++
 2 files changed, 25 insertions(+)

drivers/gpu/drm/drm_crtc.c +11 -0

index 5af25ce..07ae705 100644
@@ -38,6 +38,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_modeset_lock.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_auth.h>

@@ -741,3 +742,24 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
 
 	return ret;
 }
+
+/**
+ * drm_crtc_mode_valid - call crtc->mode_valid callback, if any.
+ * @crtc: crtc
+ * @mode: mode to be validated
+ *
+ * If no mode_valid callback is available this will return MODE_OK.
+ *
+ * Returns: drm_mode_status Enum
+ */

drivers/gpu/drm/drm_crtc.c int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj, · line 758

Quoted 4 lines

drivers/gpu/drm/drm_crtc.c +4 -0

+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;

This is clearly a helper func, but you place it into the core and EXPORT_SYMBOL it. Imo this should be entirely internal to the helpers, perhaps just stuff them all into drm_probe_helpers.c? Header file would be drm_crtc_helper_internal.h.

That also means no need for kernel-doc (only the driver api is formally documented) and then these 3 patches are so tiny it's better to squash them into the patch that adds their users.

Thanks, Daniel

Quoted 25 lines

drivers/gpu/drm/drm_crtc.c +7 -0

+
+	if (!crtc_funcs || !crtc_funcs->mode_valid)
+		return MODE_OK;
+
+	return crtc_funcs->mode_valid(crtc, mode);
+}
+EXPORT_SYMBOL(drm_crtc_mode_valid);

drivers/gpu/drm/drm_crtc_internal.h +3 -0

index d077c54..3800abd 100644
@@ -45,6 +45,9 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
 
 struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc);
 
+enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
+					 const struct drm_display_mode *mode);
+
 /* IOCTLs */
 int drm_mode_getcrtc(struct drm_device *dev,
 		     void *data, struct drm_file *file_priv);

Signature · 1 line

1.9.1

Quoted 70 lines · Jose Abreu

This changes the connector probe helper function to use the new encoder->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 encodersXcrtcs and calls the callback.

If at least a valid encoderXcrtc 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 v1->v2:
- Use new helpers suggested by Ville
- Change documentation (Daniel)

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

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

index 1b0c14a..de47413 100644
@@ -39,6 +39,8 @@
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_edid.h>
 
+#include "drm_crtc_internal.h"
+
 /**
  * DOC: output probing helper overview
  *

@@ -80,6 +82,54 @@
 	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) {

drivers/gpu/drm/drm_probe_helper.c line 114

Quoted 6 lines

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

+			/* 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;
+		}

One thing I've forgotten the last time around: Please also check bridge->mode_valid here. The encoder->bridge mapping is fixed.

Otherwise I think this looks good.
-Daniel

Quoted 49 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;

@@ -284,7 +334,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 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

@@ -428,8 +482,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

Quoted 121 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 description of connector->mode_valid() callback so that it matches the existing behaviour: It is never called in atomic check phase.

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 v1->v2:
- Change description of connector->mode_valid() (Daniel)

 include/drm/drm_bridge.h                 | 20 ++++++++++++++
 include/drm/drm_modeset_helper_vtables.h | 45 ++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

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 +40 -0

index c01c328..eec2c70 100644
@@ -106,6 +106,26 @@ 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

@@ -457,6 +477,26 @@ 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

@@ -795,6 +835,11 @@ struct drm_connector_helper_funcs {
 	 * (which is usually derived from the EDID data block from the sink).

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

Quoted 10 lines

include/drm/drm_modeset_helper_vtables.h +5 -0

 	 * 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

Kerneldoc review seems to still be missing. One case that needs to be updated is this note here. But there's a pile of other places where we reference one of the mode_valid or mode_fixup functions, and they should all be updated.

Also, it'd be good to explain what to put into mode_valid and what to put into mode_fixup, for objects which have both. I can help with this, but I think it'd be good if you make a first round, since that might catch some interactions we've missed.

Thanks, Daniel

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 +19 -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;
+

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

+}
+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

Quoted 43 lines · 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 +19 -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;
+

drivers/gpu/drm/drm_bridge.c bool drm_bridge_mode_fixup(struct drm_bridge *bridge, · line 237

Quoted 10 lines

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

+	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);

Looks like it should be pretty trivial to avoid the recursion.

Am I correct in interpreting this that bridges have some kind of a hand rolled linked list implementation? Reusing the standard linked lists would allow you to use list_for_each() etc.

Quoted 24 lines

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

+}
+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

Hi Ville,

Quoted 56 lines · Ville Syrjälä

Quoted 43 lines · 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 +19 -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;
+

drivers/gpu/drm/drm_bridge.c bool drm_bridge_mode_fixup(struct drm_bridge *bridge, · line 237

Quoted 10 lines

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

+	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);

Looks like it should be pretty trivial to avoid the recursion.

Quoted 3 lines

Am I correct in interpreting this that bridges have some kind of a hand rolled linked list implementation? Reusing the standard linked lists would allow you to use list_for_each() etc.

I reused the drm_bridge_mode_fixup but now I see how its done like that: so that the fixup is propagated in the correct order. As for mode_valid we just need to check if ret != MODE_OK then I think we can use the list_for_each_entry(bridge->list).

Best regards,
Jose Miguel Abreu

Quoted 23 lines

Quoted 23 lines

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

+}
+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

Hi Daniel,

Quoted 43 lines · Daniel Vetter

Quoted 42 lines · Jose Abreu

Add a new helper to call crtc->mode_valid callback.

Suggested-by Ville Syrjälä <[email protected]>

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_crtc.c          | 22 ++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc_internal.h |  3 +++
 2 files changed, 25 insertions(+)

drivers/gpu/drm/drm_crtc.c +11 -0

index 5af25ce..07ae705 100644
@@ -38,6 +38,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_modeset_lock.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_auth.h>

@@ -741,3 +742,24 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
 
 	return ret;
 }
+
+/**
+ * drm_crtc_mode_valid - call crtc->mode_valid callback, if any.
+ * @crtc: crtc
+ * @mode: mode to be validated
+ *
+ * If no mode_valid callback is available this will return MODE_OK.
+ *
+ * Returns: drm_mode_status Enum
+ */

Quoted 8 lines

drivers/gpu/drm/drm_crtc.c int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj, · line 758

Quoted 4 lines

drivers/gpu/drm/drm_crtc.c +4 -0

+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;

This is clearly a helper func, but you place it into the core and EXPORT_SYMBOL it. Imo this should be entirely internal to the helpers, perhaps just stuff them all into drm_probe_helpers.c? Header file would be drm_crtc_helper_internal.h.

Yeah, at first I was not planning to export it but then I saw that drm_bridge_mode_fixup() is exported (and is in drm_bridge.c) so it kind of felt right to place this in drm_crtc.c. Anyway, I will move them to drm_probe_helpers.c, indeed there is no point in exporting this.

Quoted 43 lines · 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 +19 -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;
+

drivers/gpu/drm/drm_bridge.c bool drm_bridge_mode_fixup(struct drm_bridge *bridge, · line 237

Quoted 10 lines

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

+	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);

Looks like it should be pretty trivial to avoid the recursion.

Am I correct in interpreting this that bridges have some kind of a hand rolled linked list implementation? Reusing the standard linked lists would allow you to use list_for_each() etc.

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

This calls the connector->mode_valid(), 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 | 75 +++++++++++++++++++++++++++++++++++--
  1 file changed, 72 insertions(+), 3 deletions(-)

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

index 8be9719..19d0ea1 100644
@@ -452,6 +452,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 +529,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

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

  	return mode_fixup(state);
  }
  EXPORT_SYMBOL(drm_atomic_helper_check_modeset);

Quoted 123 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 connector->mode_valid(), 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 | 75 +++++++++++++++++++++++++++++++++++--
  1 file changed, 72 insertions(+), 3 deletions(-)

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

index 8be9719..19d0ea1 100644
@@ -452,6 +452,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 +529,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 +682,10 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
  			return ret;
  	}
+	ret = mode_valid(state);

drivers/gpu/drm/drm_atomic_helper.c static int handle_conflicting_encoders(struct drm_atomic_state *state, · line 688

Quoted 3 lines

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

+	if (ret)
+		return ret;
+

Since we've ensured that the modes won't fail, can mode_fixup() and the mode_fixup() ops for crtc/bridge/encoders be assured to not fail too? This is assuming that all drivers have moved to using the new mode_valid() ops correctly.

Quoted 37 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 description of connector->mode_valid() callback so that it matches the existing behaviour: It is never called in atomic check phase.

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 v1->v2:
- Change description of connector->mode_valid() (Daniel)

 include/drm/drm_bridge.h                 | 20 ++++++++++++++
 include/drm/drm_modeset_helper_vtables.h | 45 +++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

include/drm/drm_bridge.h +0 -0

index fdd82fc..00c6c36 100644
@@ -59,6 +59,26 @@ struct drm_bridge_funcs {
 	void (*detach)(struct drm_bridge *bridge);
 

include/drm/drm_bridge.h struct drm_bridge_funcs {

Quoted 6 lines

include/drm/drm_bridge.h +5 -0

 	/**
+	 * @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 

Quoted 5 lines

include/drm/drm_bridge.h +5 -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.
+	 *

include/drm/drm_bridge.h struct drm_bridge_funcs {

Quoted 8 lines

include/drm/drm_bridge.h +8 -0

+	 * 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 

Quoted 23 lines

include/drm/drm_bridge.h +2 -0

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

(quoted diff) +0 -0

b/include/drm/drm_modeset_helper_vtables.h index c01c328..eec2c70 100644

include/drm/drm_modeset_helper_vtables.h +10 -0

@@ -106,6 +106,26 @@ 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.
+	 *

include/drm/drm_modeset_helper_vtables.h struct drm_crtc_helper_funcs {

Quoted 8 lines

include/drm/drm_modeset_helper_vtables.h +8 -0

+	 * 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 

Quoted 8 lines

include/drm/drm_modeset_helper_vtables.h +2 -0

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

@@ -457,6 +477,26 @@ struct drm_encoder_helper_funcs {
 	void (*dpms)(struct drm_encoder *encoder, int mode);
 

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs {

Quoted 8 lines

include/drm/drm_modeset_helper_vtables.h +7 -0

 	/**
+	 * @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 

include/drm/drm_modeset_helper_vtables.h struct drm_encoder_helper_funcs {

Quoted 11 lines

include/drm/drm_modeset_helper_vtables.h +11 -0

+	 * 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 

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

Quoted 10 lines

include/drm/drm_modeset_helper_vtables.h +3 -0

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

@@ -795,6 +835,11 @@ 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 

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

Quoted 7 lines

include/drm/drm_modeset_helper_vtables.h +4 -0

+	 * 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

Kerneldoc review seems to still be missing. One case that needs to be updated is this note here. But there's a pile of other places where we reference one of the mode_valid or mode_fixup functions, and they should all be updated.

Also, it'd be good to explain what to put into mode_valid and what to put into mode_fixup, for objects which have both. I can help with this, but I think it'd be good if you make a first round, since that might catch some interactions we've missed.

This changes the connector probe helper function to use the new encoder->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 encodersXcrtcs and calls the callback.

If at least a valid encoderXcrtc 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 v1->v2:
- Use new helpers suggested by Ville
- Change documentation (Daniel)

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

(quoted diff) +0 -0

b/drivers/gpu/drm/drm_probe_helper.c index 1b0c14a..de47413 100644

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

@@ -39,6 +39,8 @@
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_edid.h>
 
+#include "drm_crtc_internal.h"
+
 /**
  * DOC: output probing helper overview
  *

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

+{
+	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 */

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

+			 * will not accept the mode anyway. If all encoders
+			 * reject the mode then, at exit, ret will not be
+			 * MODE_OK. */
+			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)
 {

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

 	struct drm_cmdline_mode *cmdline_mode;

@@ -284,7 +334,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 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

}

Quoted 39 lines · Ville Syrjälä

Quoted 38 lines · 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 +6 -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.

Quoted 8 lines

drivers/gpu/drm/drm_bridge.c

Quoted 8 lines

+ *
+ * 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
+ */

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

+enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
+					   const struct drm_display_mode 

Quoted 16 lines

drivers/gpu/drm/drm_bridge.c bool drm_bridge_mode_fixup(struct drm_bridge *bridge, · line 237

Quoted 13 lines

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

+{
+	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);

Looks like it should be pretty trivial to avoid the recursion.

Quoted 3 lines

Am I correct in interpreting this that bridges have some kind of a hand rolled linked list implementation? Reusing the standard linked lists would allow you to use list_for_each() etc.

Yeah it's a hand-rolled list, but current hw also has a bridge nesting depth of 2, so it really doesn't matter. I guess once we have real long chains of bridges we can fix this (and just using list_head sounds like a great idea).

Quoted 144 lines · Archit Taneja

Quoted 143 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 connector->mode_valid(), 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 | 75 +++++++++++++++++++++++++++--
  1 file changed, 72 insertions(+), 3 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/drm_atomic_helper.c index 8be9719..19d0ea1 100644

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

@@ -452,6 +452,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 +529,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

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

@@ -617,6 +682,10 @@ static int handle_conflicting_encoders(struct
 drm_atomic_state *state,
  			return ret;
  	}
 

Quoted 9 lines

drivers/gpu/drm/drm_atomic_helper.c static int handle_conflicting_encoders(struct drm_atomic_state *state, · line 688

Quoted 4 lines

drivers/gpu/drm/drm_atomic_helper.c +4 -0

+	ret = mode_valid(state);
+	if (ret)
+		return ret;
+

Since we've ensured that the modes won't fail, can mode_fixup() and the mode_fixup() ops for crtc/bridge/encoders be assured to not fail too? This is assuming that all drivers have moved to using the new mode_valid() ops correctly.

The entire point of re-checking is that userspace can create its own modes and submit them to the kernel, bypassing the current connector->mode_valid() check. I think almost all drivers get this wrong unfortunately :(

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(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/arc/arcpgu_crtc.c index ad9a959..01cae0a 100644

drivers/gpu/drm/arc/arcpgu_crtc.c +0 -0

@@ -32,6 +32,18 @@
 	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 },

drivers/gpu/drm/arc/arcpgu_crtc.c +10 -15

+{
+	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,
 };

Hi Daniel,

Quoted 40 lines · On Wednesday 10 May 2017 17:14:33 Daniel Vetter wrote:

Quoted 39 lines · Ville Syrjälä

Quoted 38 lines · 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 +6 -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.

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/drm_bridge.c

Quoted 8 lines

+ *
+ * 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
+ */

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

+enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
+					   const struct drm_display_mode

*mode)

Quoted 20 lines

Quoted 19 lines

drivers/gpu/drm/drm_bridge.c bool drm_bridge_mode_fixup(struct drm_bridge *bridge, · line 237

Quoted 13 lines

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

+{
+	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);

Looks like it should be pretty trivial to avoid the recursion.

Am I correct in interpreting this that bridges have some kind of a hand rolled linked list implementation? Reusing the standard linked lists would allow you to use list_for_each() etc.

Quoted 4 lines

Yeah it's a hand-rolled list, but current hw also has a bridge nesting depth of 2, so it really doesn't matter. I guess once we have real long chains of bridges we can fix this (and just using list_head sounds like a great idea).

Even if not really needed right now, it's a pretty easy cleanup, if Jose has time to handle it in v3 of this series let's not postpone it ;-)

Quoted 43 lines · Laurent Pinchart

Quoted 42 lines · On Wednesday 10 May 2017 17:14:33 Daniel Vetter wrote:

Quoted 41 lines · Ville Syrjälä

Quoted 40 lines · 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 +1 -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.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/drm_bridge.c

Quoted 8 lines

+ *
+ * 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
+ */

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

+enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
+					   const struct 

Quoted 28 lines

*mode)

Quoted 24 lines

Quoted 16 lines

drivers/gpu/drm/drm_bridge.c bool drm_bridge_mode_fixup(struct drm_bridge *bridge, · line 237

Quoted 13 lines

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

+{
+	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);

Looks like it should be pretty trivial to avoid the recursion.

Quoted 3 lines

Am I correct in interpreting this that bridges have some kind of a hand rolled linked list implementation? Reusing the standard linked lists would allow you to use list_for_each() etc.

Yeah it's a hand-rolled list, but current hw also has a bridge nesting depth of 2, so it really doesn't matter. I guess once we have real long chains of bridges we can fix this (and just using list_head sounds like a great idea).

Quoted 2 lines

Even if not really needed right now, it's a pretty easy cleanup, if Jose has time to handle it in v3 of this series let's not postpone it ;-)

jfyi, some of the bridge functions call the ops from the last bridge in the chain to first, so we'd need to use list_for_each_entry_prev() (or something like that) for them.

Hi Jose,

Thank you for the patch.

Quoted 46 lines · On Tuesday 09 May 2017 18:00:12 Jose Abreu wrote:

This changes the connector probe helper function to use the new encoder->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 encodersXcrtcs and calls the callback.

If at least a valid encoderXcrtc 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 v1->v2:
- Use new helpers suggested by Ville
- Change documentation (Daniel)

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

(quoted diff) +0 -0

b/drivers/gpu/drm/drm_probe_helper.c index 1b0c14a..de47413 100644

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

@@ -39,6 +39,8 @@
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_edid.h>
 
+#include "drm_crtc_internal.h"
+
 /**
  * DOC: output probing helper overview
  *

@@ -80,6 +82,54 @@
 	return MODE_OK;
 }
 

drivers/gpu/drm/drm_probe_helper.c line 87

Quoted 3 lines

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

+static enum drm_mode_status
+drm_mode_validate_connector(struct drm_connector *connector,
+			    struct drm_display_mode *mode)

This does more than validating the mode against the connector, it validates it against the whole pipeline. I would call the function drm_mode_validate_pipeline() (or any other similar name).

Quoted 13 lines

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

+{
+	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 */

drivers/gpu/drm/drm_probe_helper.c

Quoted 10 lines

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

+	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

Quoted 50 lines

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

+			 * will not accept the mode anyway. If all encoders
+			 * reject the mode then, at exit, ret will not be
+			 * MODE_OK. */
+			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)
 {

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

 	struct drm_cmdline_mode *cmdline_mode;

@@ -284,7 +334,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 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 +0 -0

@@ -428,8 +482,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);
 

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

Quoted 5 lines

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

-		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);

I would reverse the arguments order to match the style of the other validation functions.

Quoted 2 lines

}

Quoted 34 lines · Laurent Pinchart

Quoted 33 lines · On Tuesday 09 May 2017 18:00:12 Jose Abreu wrote:

This changes the connector probe helper function to use the new encoder->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 encodersXcrtcs and calls the callback.

If at least a valid encoderXcrtc 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 v1->v2:
- Use new helpers suggested by Ville
- Change documentation (Daniel)

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

(quoted diff) +0 -0

b/drivers/gpu/drm/drm_probe_helper.c index 1b0c14a..de47413 100644

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

Quoted 7 lines

drivers/gpu/drm/drm_probe_helper.c line 87

Quoted 3 lines

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

+static enum drm_mode_status
+drm_mode_validate_connector(struct drm_connector *connector,
+			    struct drm_display_mode *mode)

This does more than validating the mode against the connector, it validates it against the whole pipeline. I would call the function drm_mode_validate_pipeline() (or any other similar name).

Yeah, in previous version I had something similar but I changed in order to address review comments. I can change again though...

Quoted 47 lines

Quoted 12 lines

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

+{
+	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 */

drivers/gpu/drm/drm_probe_helper.c

Quoted 10 lines

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

+	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

Quoted 22 lines

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

+			 * will not accept the mode anyway. If all encoders
+			 * reject the mode then, at exit, ret will not be
+			 * MODE_OK. */
+			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;
+}

Quoted 14 lines

Quoted 14 lines

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

@@ -428,8 +482,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,

Quoted 4 lines

Quoted 1 line

mode);

I would reverse the arguments order to match the style of the other validation functions.

Hmm, I think it makes more sense to pass connector first and then mode ...

Hi Jose,

Thank you for the patch.

Quoted 31 lines · On Tuesday 09 May 2017 18:00:15 Jose Abreu wrote:

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(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/arc/arcpgu_crtc.c index ad9a959..01cae0a 100644

drivers/gpu/drm/arc/arcpgu_crtc.c +0 -0

@@ -32,6 +32,18 @@
 	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 },
 };
 

drivers/gpu/drm/arc/arcpgu_crtc.c line 42

Quoted 8 lines

drivers/gpu/drm/arc/arcpgu_crtc.c +8 -0

+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;

This isn't anything new introduced by this patch, but shouldn't drivers allow for some margin in clock frequencies ? Surely if the mode requires a 60.000.000 Hz frequency and the hardware can only generate 59.999.999 Hz or 60.000.001 Hz we shouldn't fail. As far as I understand, this is something the mode_fixup() operation is supposed to handle, but the arc driver doesn't implement it.

drivers/gpu/drm/arc/arcpgu_crtc.c line 45

Quoted 3 lines

drivers/gpu/drm/arc/arcpgu_crtc.c +2 -0

+	return true;
+}

Can't you inline this in arc_pgu_crtc_mode_valid() as there's a single caller ?

drivers/gpu/drm/arc/arcpgu_crtc.c static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc) · line 80

Quoted 9 lines

drivers/gpu/drm/arc/arcpgu_crtc.c +2 -0

 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)

Quoted 48 lines

drivers/gpu/drm/arc/arcpgu_crtc.c +10 -15

+{
+	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,
 };