[PATCH 0/5] Introduce new mode validation callbacks

[PATCH 0/5] Introduce new mode validation callbacks

[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.

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

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

At 4/5 we call all the mode_valid() callbacks for a given pipeline
so that the mode is validated. This is done before calling mode_fixup().

Finally, at 5/5 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 (5):
  drm: Add crtc/encoder/bridge->mode_valid() callbacks
  drm: Use new mode_valid() helpers in connector probe helper
  drm: Introduce drm_bridge_mode_valid()
  drm: Use mode_valid() in atomic modeset
  drm: arc: Use crtc->mode_valid() callback

Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Archit Taneja <architt@codeaurora.org>

 drivers/gpu/drm/arc/arcpgu_crtc.c        | 39 ++++++++------
 drivers/gpu/drm/drm_atomic_helper.c      | 92 ++++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_bridge.c             | 33 ++++++++++++
 drivers/gpu/drm/drm_probe_helper.c       | 71 ++++++++++++++++++++++--
 include/drm/drm_bridge.h                 | 22 ++++++++
 include/drm/drm_modeset_helper_vtables.h | 40 ++++++++++++++
 6 files changed, 275 insertions(+), 22 deletions(-)

-- 
1.9.1

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

[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.

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

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Archit Taneja <architt@codeaurora.org>
---
 include/drm/drm_bridge.h                 | 20 ++++++++++++++++
 include/drm/drm_modeset_helper_vtables.h | 40 ++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index fdd82fc..00c6c36 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -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
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index c01c328..bb04688 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -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
-- 
1.9.1

[PATCH 2/5] drm: Use new mode_valid() helpers in connector probe helper

[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 <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/drm_probe_helper.c | 71 +++++++++++++++++++++++++++++++++++---
 1 file changed, 67 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 1b0c14a..bf19f82 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -80,6 +80,67 @@
 	return MODE_OK;
 }
 
+static enum drm_mode_status
+drm_mode_validate_connector(struct drm_connector *connector,
+			    struct drm_display_mode *mode)
+{
+	const struct drm_connector_helper_funcs *connector_funcs =
+		connector->helper_private;
+	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 */
+	if (connector_funcs && connector_funcs->mode_valid)
+		ret = connector_funcs->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]);
+		const struct drm_encoder_helper_funcs *encoder_funcs;
+		struct drm_crtc *crtc;
+
+		if (!encoder)
+			continue;
+
+		encoder_funcs = encoder->helper_private;
+		if (encoder_funcs && encoder_funcs->mode_valid)
+			ret = encoder_funcs->mode_valid(encoder, mode);
+		else
+			ret = MODE_OK; /* encoder accepts everything */
+
+		/* 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. */
+		if (ret != MODE_OK)
+			continue;
+
+		drm_for_each_crtc(crtc, dev) {
+			const struct drm_crtc_helper_funcs *crtc_funcs;
+
+			if (!drm_encoder_crtc_ok(encoder, crtc))
+				continue;
+
+			crtc_funcs = crtc->helper_private;
+			if (!crtc_funcs || !crtc_funcs->mode_valid)
+				return MODE_OK; /* crtc accepts everything */
+
+			ret = crtc_funcs->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;
@@ -283,8 +344,10 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
  *      (if specified)
  *    - drm_mode_validate_flag() checks the modes against basic connector
  *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
- *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
- *      driver and/or hardware specific checks
+ *    - the optional &drm_connector_helper_funcs.mode_valid,
+ *    	&drm_crtc_helper_funcs.mode_valid and
+ *    	&drm_encoder_helper_funcs.mode_valid helpers can perform driver and/or
+ *    	hardware specific checks
  *
  * 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 +491,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);
 	}
 
-- 
1.9.1

[PATCH 3/5] drm: Introduce drm_bridge_mode_valid()

[Jose Abreu]

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

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h     |  2 ++
 2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 86a7637..dc8cdfe 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -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
  *
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 00c6c36..8358eb3 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -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,
-- 
1.9.1

[PATCH 4/5] drm: Use mode_valid() in atomic modeset

[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 <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 92 +++++++++++++++++++++++++++++++++++--
 1 file changed, 89 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 8be9719..6eb305d 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -452,6 +452,85 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 	return 0;
 }
 
+static enum drm_mode_status mode_valid_pipe(struct drm_connector *connector,
+					    struct drm_encoder *encoder,
+					    struct drm_crtc *crtc,
+					    struct drm_display_mode *mode)
+{
+	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
+	const struct drm_connector_helper_funcs *connector_funcs =
+		connector->helper_private;
+	const struct drm_encoder_helper_funcs *encoder_funcs =
+		encoder->helper_private;
+	enum drm_mode_status ret = MODE_OK;
+
+	if (connector_funcs && connector_funcs->mode_valid)
+		ret = connector_funcs->mode_valid(connector, mode);
+
+	if (ret != MODE_OK) {
+		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] mode_valid() failed\n",
+				connector->base.id, connector->name);
+		return ret;
+	}
+
+	if (encoder_funcs && encoder_funcs->mode_valid)
+		ret = encoder_funcs->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;
+	}
+
+	if (crtc_funcs && crtc_funcs->mode_valid)
+		ret = crtc_funcs->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_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;
+
+		mode = &crtc_state->mode;
+
+		mode_status = mode_valid_pipe(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 +545,16 @@ 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_connector_helper_funcs.mode_valid, &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 +699,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);
-- 
1.9.1

[PATCH 5/5] drm: arc: Use crtc->mode_valid() callback

[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 <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/arc/arcpgu_crtc.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
index ad9a959..01cae0a 100644
--- a/drivers/gpu/drm/arc/arcpgu_crtc.c
+++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
@@ -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,
 };
 
-- 
1.9.1

Re: [PATCH 2/5] drm: Use new mode_valid() helpers in connector probe helper

[Ville Syrjälä]

On Thu, May 04, 2017 at 03:11:39PM +0100, 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 <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> ---
>  drivers/gpu/drm/drm_probe_helper.c | 71 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 67 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 1b0c14a..bf19f82 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -80,6 +80,67 @@
>  	return MODE_OK;
>  }
>  
> +static enum drm_mode_status
> +drm_mode_validate_connector(struct drm_connector *connector,
> +			    struct drm_display_mode *mode)
> +{
> +	const struct drm_connector_helper_funcs *connector_funcs =
> +		connector->helper_private;
> +	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 */
> +	if (connector_funcs && connector_funcs->mode_valid)
> +		ret = connector_funcs->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]);
> +		const struct drm_encoder_helper_funcs *encoder_funcs;
> +		struct drm_crtc *crtc;
> +
> +		if (!encoder)
> +			continue;
> +
> +		encoder_funcs = encoder->helper_private;
> +		if (encoder_funcs && encoder_funcs->mode_valid)
> +			ret = encoder_funcs->mode_valid(encoder, mode);
> +		else
> +			ret = MODE_OK; /* encoder accepts everything */

Since you already added the drm_bridge_mode_valid() helper, maybe add one
for connectors, encoders and crtcs as well. Might keep the logic in this
function a bit more clear on account of not having to check for the
presence of the vfunc. Right now all three cases look slightly
different from one another.

> +
> +		/* 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. */
> +		if (ret != MODE_OK)
> +			continue;
> +
> +		drm_for_each_crtc(crtc, dev) {
> +			const struct drm_crtc_helper_funcs *crtc_funcs;
> +
> +			if (!drm_encoder_crtc_ok(encoder, crtc))
> +				continue;
> +
> +			crtc_funcs = crtc->helper_private;
> +			if (!crtc_funcs || !crtc_funcs->mode_valid)
> +				return MODE_OK; /* crtc accepts everything */
> +
> +			ret = crtc_funcs->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;
> @@ -283,8 +344,10 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>   *      (if specified)
>   *    - drm_mode_validate_flag() checks the modes against basic connector
>   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
> - *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
> - *      driver and/or hardware specific checks
> + *    - the optional &drm_connector_helper_funcs.mode_valid,
> + *    	&drm_crtc_helper_funcs.mode_valid and
> + *    	&drm_encoder_helper_funcs.mode_valid helpers can perform driver and/or
> + *    	hardware specific checks
>   *
>   * 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 +491,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);
>  	}
>  
> -- 
> 1.9.1
> 

-- 
Ville Syrjälä
Intel OTC

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

[Ville Syrjälä]

On Thu, May 04, 2017 at 03:11:41PM +0100, Jose Abreu wrote:
> 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 <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 92 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 89 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 8be9719..6eb305d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -452,6 +452,85 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
>  	return 0;
>  }
>  
> +static enum drm_mode_status mode_valid_pipe(struct drm_connector *connector,

The name feels off. Pipe means crtc for i915, and for drm_irq.c as well.

> +					    struct drm_encoder *encoder,
> +					    struct drm_crtc *crtc,
> +					    struct drm_display_mode *mode)
> +{
> +	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
> +	const struct drm_connector_helper_funcs *connector_funcs =
> +		connector->helper_private;
> +	const struct drm_encoder_helper_funcs *encoder_funcs =
> +		encoder->helper_private;
> +	enum drm_mode_status ret = MODE_OK;
> +
> +	if (connector_funcs && connector_funcs->mode_valid)
> +		ret = connector_funcs->mode_valid(connector, mode);

As I mentioned earlier, this would break i915. We either can't call the 
connector hook at all here, or we have to make it somehow opt-in if some
drivers really want to do it.

> +
> +	if (ret != MODE_OK) {
> +		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] mode_valid() failed\n",
> +				connector->base.id, connector->name);
> +		return ret;
> +	}
> +
> +	if (encoder_funcs && encoder_funcs->mode_valid)
> +		ret = encoder_funcs->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;
> +	}
> +
> +	if (crtc_funcs && crtc_funcs->mode_valid)
> +		ret = crtc_funcs->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_connector_in_state(state, connector, conn_state, i) {

for_each_new...

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


Maybe?

if (!mode_changed && !connectors_changed)
	continue;

> +
> +		mode = &crtc_state->mode;

Seems like a fairly pointless variable.

> +
> +		mode_status = mode_valid_pipe(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 +545,16 @@ 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_connector_helper_funcs.mode_valid, &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 +699,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);
> -- 
> 1.9.1
> 

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/5] drm: Use new mode_valid() helpers in connector probe helper

[Jose Abreu]

Hi Ville,


On 04-05-2017 15:32, Ville Syrjälä wrote:
> On Thu, May 04, 2017 at 03:11:39PM +0100, 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 <joabreu@synopsys.com>
>> Cc: Carlos Palminha <palminha@synopsys.com>
>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Dave Airlie <airlied@linux.ie>
>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>> Cc: Archit Taneja <architt@codeaurora.org>
>> ---
>>  drivers/gpu/drm/drm_probe_helper.c | 71 +++++++++++++++++++++++++++++++++++---
>>  1 file changed, 67 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index 1b0c14a..bf19f82 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -80,6 +80,67 @@
>>  	return MODE_OK;
>>  }
>>  
>> +static enum drm_mode_status
>> +drm_mode_validate_connector(struct drm_connector *connector,
>> +			    struct drm_display_mode *mode)
>> +{
>> +	const struct drm_connector_helper_funcs *connector_funcs =
>> +		connector->helper_private;
>> +	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 */
>> +	if (connector_funcs && connector_funcs->mode_valid)
>> +		ret = connector_funcs->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]);
>> +		const struct drm_encoder_helper_funcs *encoder_funcs;
>> +		struct drm_crtc *crtc;
>> +
>> +		if (!encoder)
>> +			continue;
>> +
>> +		encoder_funcs = encoder->helper_private;
>> +		if (encoder_funcs && encoder_funcs->mode_valid)
>> +			ret = encoder_funcs->mode_valid(encoder, mode);
>> +		else
>> +			ret = MODE_OK; /* encoder accepts everything */
> Since you already added the drm_bridge_mode_valid() helper, maybe add one
> for connectors, encoders and crtcs as well. Might keep the logic in this
> function a bit more clear on account of not having to check for the
> presence of the vfunc. Right now all three cases look slightly
> different from one another.

Ok, will add in next version. Thanks!

Best regards,
Jose Miguel Abreu

>
>> +
>> +		/* 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. */
>> +		if (ret != MODE_OK)
>> +			continue;
>> +
>> +		drm_for_each_crtc(crtc, dev) {
>> +			const struct drm_crtc_helper_funcs *crtc_funcs;
>> +
>> +			if (!drm_encoder_crtc_ok(encoder, crtc))
>> +				continue;
>> +
>> +			crtc_funcs = crtc->helper_private;
>> +			if (!crtc_funcs || !crtc_funcs->mode_valid)
>> +				return MODE_OK; /* crtc accepts everything */
>> +
>> +			ret = crtc_funcs->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;
>> @@ -283,8 +344,10 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>>   *      (if specified)
>>   *    - drm_mode_validate_flag() checks the modes against basic connector
>>   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
>> - *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
>> - *      driver and/or hardware specific checks
>> + *    - the optional &drm_connector_helper_funcs.mode_valid,
>> + *    	&drm_crtc_helper_funcs.mode_valid and
>> + *    	&drm_encoder_helper_funcs.mode_valid helpers can perform driver and/or
>> + *    	hardware specific checks
>>   *
>>   * 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 +491,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);
>>  	}
>>  
>> -- 
>> 1.9.1
>>

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

[Jose Abreu]

Hi Ville,


On 04-05-2017 15:40, Ville Syrjälä wrote:
> On Thu, May 04, 2017 at 03:11:41PM +0100, Jose Abreu wrote:
>> 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 <joabreu@synopsys.com>
>> Cc: Carlos Palminha <palminha@synopsys.com>
>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Dave Airlie <airlied@linux.ie>
>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>> Cc: Archit Taneja <architt@codeaurora.org>
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c | 92 +++++++++++++++++++++++++++++++++++--
>>  1 file changed, 89 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 8be9719..6eb305d 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -452,6 +452,85 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
>>  	return 0;
>>  }
>>  
>> +static enum drm_mode_status mode_valid_pipe(struct drm_connector *connector,
> The name feels off. Pipe means crtc for i915, and for drm_irq.c as well.

Hmmm, mode_valid_path, mode_valid_components ?

>
>> +					    struct drm_encoder *encoder,
>> +					    struct drm_crtc *crtc,
>> +					    struct drm_display_mode *mode)
>> +{
>> +	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
>> +	const struct drm_connector_helper_funcs *connector_funcs =
>> +		connector->helper_private;
>> +	const struct drm_encoder_helper_funcs *encoder_funcs =
>> +		encoder->helper_private;
>> +	enum drm_mode_status ret = MODE_OK;
>> +
>> +	if (connector_funcs && connector_funcs->mode_valid)
>> +		ret = connector_funcs->mode_valid(connector, mode);
> As I mentioned earlier, this would break i915. We either can't call the 
> connector hook at all here, or we have to make it somehow opt-in if some
> drivers really want to do it.

Ok. You said you let users exceed the limits, but why doesn't it
fail in atomic_check and will fail in mode_valid? I guess I can
remove it, but this can lead to lots of confusion, i.e. with this
series the mode_valid callbacks for all components are called, if
I remove just one call the whole point will fall apart.

Can we think about something else ? I think making it opt-in is
not ideal, if the helper is there then it should be used, but if
thats the best solution then shall we add a flag which will call
or not *all* the mode_valid callbacks in this stage
(drm_device.allow_modevalid_call, ...) ?

>
>> +
>> +	if (ret != MODE_OK) {
>> +		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] mode_valid() failed\n",
>> +				connector->base.id, connector->name);
>> +		return ret;
>> +	}
>> +
>> +	if (encoder_funcs && encoder_funcs->mode_valid)
>> +		ret = encoder_funcs->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;
>> +	}
>> +
>> +	if (crtc_funcs && crtc_funcs->mode_valid)
>> +		ret = crtc_funcs->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_connector_in_state(state, connector, conn_state, i) {
> for_each_new...

Ok.

>
>> +		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;
>
> Maybe?
>
> if (!mode_changed && !connectors_changed)
> 	continue;

Yes, no point in checking if the mode and connector didn't change.

>
>> +
>> +		mode = &crtc_state->mode;
> Seems like a fairly pointless variable.

Yeah, this was to avoid the next function call to exceed 80
chars, I don't really like line breaks :) Thanks for the review!

Best regards,
Jose Miguel Abreu

>
>> +
>> +		mode_status = mode_valid_pipe(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 +545,16 @@ 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_connector_helper_funcs.mode_valid, &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 +699,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);
>> -- 
>> 1.9.1
>>

Re: [PATCH 2/5] drm: Use new mode_valid() helpers in connector probe helper

[Daniel Vetter]

On Thu, May 04, 2017 at 03:11:39PM +0100, 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 <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>

A few comments below.
-Daniel

> ---
>  drivers/gpu/drm/drm_probe_helper.c | 71 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 67 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 1b0c14a..bf19f82 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -80,6 +80,67 @@
>  	return MODE_OK;
>  }
>  
> +static enum drm_mode_status
> +drm_mode_validate_connector(struct drm_connector *connector,
> +			    struct drm_display_mode *mode)
> +{
> +	const struct drm_connector_helper_funcs *connector_funcs =
> +		connector->helper_private;
> +	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 */
> +	if (connector_funcs && connector_funcs->mode_valid)
> +		ret = connector_funcs->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]);
> +		const struct drm_encoder_helper_funcs *encoder_funcs;
> +		struct drm_crtc *crtc;
> +
> +		if (!encoder)
> +			continue;
> +
> +		encoder_funcs = encoder->helper_private;
> +		if (encoder_funcs && encoder_funcs->mode_valid)
> +			ret = encoder_funcs->mode_valid(encoder, mode);
> +		else
> +			ret = MODE_OK; /* encoder accepts everything */
> +
> +		/* 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. */
> +		if (ret != MODE_OK)
> +			continue;

I think we should validate encoders against connector->possible_ids here.
Might be that not many drivers fill this out correctly, and in case fixing
that is much work, perhaps as a follow-up. But would be good to at least
help look down that part of uapi a bit more.

This isn't checked within atomic and legacy helpers since we assume that
->best_encoder respectively ->atomic_best_encoder gives us a valid encoder
...

> +
> +		drm_for_each_crtc(crtc, dev) {
> +			const struct drm_crtc_helper_funcs *crtc_funcs;
> +
> +			if (!drm_encoder_crtc_ok(encoder, crtc))
> +				continue;

We check this one here already in both atomic and legacy helpers, so all
drivers should get this right.

> +
> +			crtc_funcs = crtc->helper_private;
> +			if (!crtc_funcs || !crtc_funcs->mode_valid)
> +				return MODE_OK; /* crtc accepts everything */
> +
> +			ret = crtc_funcs->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;
> @@ -283,8 +344,10 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>   *      (if specified)
>   *    - drm_mode_validate_flag() checks the modes against basic connector
>   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
> - *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
> - *      driver and/or hardware specific checks
> + *    - the optional &drm_connector_helper_funcs.mode_valid,
> + *    	&drm_crtc_helper_funcs.mode_valid and
> + *    	&drm_encoder_helper_funcs.mode_valid helpers can perform driver and/or
> + *    	hardware specific checks

I'd leave 2 bullets for connector->mode_valid (sink specific checks) and
everything else (source 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 +491,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);
>  	}
>  
> -- 
> 1.9.1
> 
> 

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

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

[Daniel Vetter]

On Thu, May 04, 2017 at 03:11:38PM +0100, Jose Abreu wrote:
> 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.
> 
> Only the callbacks were implemented to simplify review process,
> following patches will make use of them.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> ---
>  include/drm/drm_bridge.h                 | 20 ++++++++++++++++
>  include/drm/drm_modeset_helper_vtables.h | 40 ++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)

I think we should also update the kernel-doc for connector->mode_valid,
explaining why it is not called in the atomic check phase, but only at
mode probe time: To allow userspace to override the kernel's sink
checks, in case of broken EDID with wrong limits from the sink.

Otherwise lgtm.
-Daniel

> 
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index fdd82fc..00c6c36 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -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
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index c01c328..bb04688 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -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
> -- 
> 1.9.1
> 
> 

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

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

[Daniel Vetter]

On Thu, May 04, 2017 at 04:13:51PM +0100, Jose Abreu wrote:
> On 04-05-2017 15:40, Ville Syrjälä wrote:
> > On Thu, May 04, 2017 at 03:11:41PM +0100, Jose Abreu wrote:
> >> +					    struct drm_encoder *encoder,
> >> +					    struct drm_crtc *crtc,
> >> +					    struct drm_display_mode *mode)
> >> +{
> >> +	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
> >> +	const struct drm_connector_helper_funcs *connector_funcs =
> >> +		connector->helper_private;
> >> +	const struct drm_encoder_helper_funcs *encoder_funcs =
> >> +		encoder->helper_private;
> >> +	enum drm_mode_status ret = MODE_OK;
> >> +
> >> +	if (connector_funcs && connector_funcs->mode_valid)
> >> +		ret = connector_funcs->mode_valid(connector, mode);
> > As I mentioned earlier, this would break i915. We either can't call the 
> > connector hook at all here, or we have to make it somehow opt-in if some
> > drivers really want to do it.
> 
> Ok. You said you let users exceed the limits, but why doesn't it
> fail in atomic_check and will fail in mode_valid? I guess I can
> remove it, but this can lead to lots of confusion, i.e. with this
> series the mode_valid callbacks for all components are called, if
> I remove just one call the whole point will fall apart.
> 
> Can we think about something else ? I think making it opt-in is
> not ideal, if the helper is there then it should be used, but if
> thats the best solution then shall we add a flag which will call
> or not *all* the mode_valid callbacks in this stage
> (drm_device.allow_modevalid_call, ...) ?

This is a general issue, not a driver opt-in. With your new callbacks
ideally we'll have:
- all the source checks (clock limits, size limits) are in the
  crtc/encoder/bridge callbacks
- only the sink limit checks (derived from edid or DP aux) are in the
  connector callback

Letting userspace overwrite these seems like a reasonable idea that we
should support in general I think. See also my comment on patch 1 for the
connector's mode_valid kerneldoc.

For arcpgu that might mean that you need to move part of the connector's
mode_valid checks into the encoder, but since all the encoder modeset is
in there already, that seems like a good cleanup of the drm modeset
framework.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/5] Introduce new mode validation callbacks

[Daniel Vetter]

On Thu, May 04, 2017 at 03:11:37PM +0100, Jose Abreu wrote:
> 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.
> 
> Next, at 2/5 we modify the connector probe helper so that only modes
> which are supported by a given encoder+crtc combination are probbed.
> 
> At 3/5 a helper function is introduced that calls all mode_valid()
> from a set of bridges.
> 
> At 4/5 we call all the mode_valid() callbacks for a given pipeline
> so that the mode is validated. This is done before calling mode_fixup().
> 
> Finally, at 5/5 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 (5):
>   drm: Add crtc/encoder/bridge->mode_valid() callbacks
>   drm: Use new mode_valid() helpers in connector probe helper
>   drm: Introduce drm_bridge_mode_valid()
>   drm: Use mode_valid() in atomic modeset
>   drm: arc: Use crtc->mode_valid() callback
> 
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>

Looks nice. A few small things from Ville and me, but with those addressed
this looks all ready for merging and seems like a good improvement to the
modeset framework.

Ok, one more patch at the end is needed: We need to review and upate the
kernel-doc for all the existing hooks, and add cross-links between
mode_valid and atomic_check respectively mode_fixup.

Some more drivers using this would be sweet too, but not a blocker.

Thanks, Daniel
> 
>  drivers/gpu/drm/arc/arcpgu_crtc.c        | 39 ++++++++------
>  drivers/gpu/drm/drm_atomic_helper.c      | 92 ++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/drm_bridge.c             | 33 ++++++++++++
>  drivers/gpu/drm/drm_probe_helper.c       | 71 ++++++++++++++++++++++--
>  include/drm/drm_bridge.h                 | 22 ++++++++
>  include/drm/drm_modeset_helper_vtables.h | 40 ++++++++++++++
>  6 files changed, 275 insertions(+), 22 deletions(-)
> 
> -- 
> 1.9.1
> 
> 

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

Re: [PATCH 2/5] drm: Use new mode_valid() helpers in connector probe helper

[Jose Abreu]

Hi Daniel,


On 08-05-2017 08:50, Daniel Vetter wrote:
> On Thu, May 04, 2017 at 03:11:39PM +0100, 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 <joabreu@synopsys.com>
>> Cc: Carlos Palminha <palminha@synopsys.com>
>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Dave Airlie <airlied@linux.ie>
>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>> Cc: Archit Taneja <architt@codeaurora.org>
> A few comments below.
> -Daniel

Thanks for the review!

>
>> ---
>>  drivers/gpu/drm/drm_probe_helper.c | 71 +++++++++++++++++++++++++++++++++++---
>>  1 file changed, 67 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index 1b0c14a..bf19f82 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -80,6 +80,67 @@
>>  	return MODE_OK;
>>  }
>>  
>> +static enum drm_mode_status
>> +drm_mode_validate_connector(struct drm_connector *connector,
>> +			    struct drm_display_mode *mode)
>> +{
>> +	const struct drm_connector_helper_funcs *connector_funcs =
>> +		connector->helper_private;
>> +	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 */
>> +	if (connector_funcs && connector_funcs->mode_valid)
>> +		ret = connector_funcs->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]);
>> +		const struct drm_encoder_helper_funcs *encoder_funcs;
>> +		struct drm_crtc *crtc;
>> +
>> +		if (!encoder)
>> +			continue;
>> +
>> +		encoder_funcs = encoder->helper_private;
>> +		if (encoder_funcs && encoder_funcs->mode_valid)
>> +			ret = encoder_funcs->mode_valid(encoder, mode);
>> +		else
>> +			ret = MODE_OK; /* encoder accepts everything */
>> +
>> +		/* 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. */
>> +		if (ret != MODE_OK)
>> +			continue;
> I think we should validate encoders against connector->possible_ids here.
> Might be that not many drivers fill this out correctly, and in case fixing
> that is much work, perhaps as a follow-up. But would be good to at least
> help look down that part of uapi a bit more.

I'm sorry but I'm not following you here (I think I need more
coffee :)).

What do you mean by connector->possible_ids ? Is this something
new ? Because I didn't find anything in my tree and I'm based on
today's drm-next from Dave.

>
> This isn't checked within atomic and legacy helpers since we assume that
> ->best_encoder respectively ->atomic_best_encoder gives us a valid encoder
> ...
>
>> +
>> +		drm_for_each_crtc(crtc, dev) {
>> +			const struct drm_crtc_helper_funcs *crtc_funcs;
>> +
>> +			if (!drm_encoder_crtc_ok(encoder, crtc))
>> +				continue;
> We check this one here already in both atomic and legacy helpers, so all
> drivers should get this right.

But drm_for_each_crtc() iterates over all crtc from a device and
some crtcs may only be used by specific encoders (by setting
encoder->possible_crtcs), right ? We need to iterate only over
the encoder's crtc we want to validate.

>
>> +
>> +			crtc_funcs = crtc->helper_private;
>> +			if (!crtc_funcs || !crtc_funcs->mode_valid)
>> +				return MODE_OK; /* crtc accepts everything */
>> +
>> +			ret = crtc_funcs->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;
>> @@ -283,8 +344,10 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>>   *      (if specified)
>>   *    - drm_mode_validate_flag() checks the modes against basic connector
>>   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
>> - *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
>> - *      driver and/or hardware specific checks
>> + *    - the optional &drm_connector_helper_funcs.mode_valid,
>> + *    	&drm_crtc_helper_funcs.mode_valid and
>> + *    	&drm_encoder_helper_funcs.mode_valid helpers can perform driver and/or
>> + *    	hardware specific checks
> I'd leave 2 bullets for connector->mode_valid (sink specific checks) and
> everything else (source checks, which are also enforced by the
> modeset/atomic helpers).

Ok, thanks!

Best regards,
Jose Miguel Abreu

>>   *
>>   * 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 +491,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);
>>  	}
>>  
>> -- 
>> 1.9.1
>>
>>

Re: [PATCH 2/5] drm: Use new mode_valid() helpers in connector probe helper

[Daniel Vetter]

On Mon, May 08, 2017 at 11:13:37AM +0100, Jose Abreu wrote:
> Hi Daniel,
> 
> 
> On 08-05-2017 08:50, Daniel Vetter wrote:
> > On Thu, May 04, 2017 at 03:11:39PM +0100, 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 <joabreu@synopsys.com>
> >> Cc: Carlos Palminha <palminha@synopsys.com>
> >> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Cc: Dave Airlie <airlied@linux.ie>
> >> Cc: Andrzej Hajda <a.hajda@samsung.com>
> >> Cc: Archit Taneja <architt@codeaurora.org>
> > A few comments below.
> > -Daniel
> 
> Thanks for the review!
> 
> >
> >> ---
> >>  drivers/gpu/drm/drm_probe_helper.c | 71 +++++++++++++++++++++++++++++++++++---
> >>  1 file changed, 67 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> >> index 1b0c14a..bf19f82 100644
> >> --- a/drivers/gpu/drm/drm_probe_helper.c
> >> +++ b/drivers/gpu/drm/drm_probe_helper.c
> >> @@ -80,6 +80,67 @@
> >>  	return MODE_OK;
> >>  }
> >>  
> >> +static enum drm_mode_status
> >> +drm_mode_validate_connector(struct drm_connector *connector,
> >> +			    struct drm_display_mode *mode)
> >> +{
> >> +	const struct drm_connector_helper_funcs *connector_funcs =
> >> +		connector->helper_private;
> >> +	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 */
> >> +	if (connector_funcs && connector_funcs->mode_valid)
> >> +		ret = connector_funcs->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]);
> >> +		const struct drm_encoder_helper_funcs *encoder_funcs;
> >> +		struct drm_crtc *crtc;
> >> +
> >> +		if (!encoder)
> >> +			continue;
> >> +
> >> +		encoder_funcs = encoder->helper_private;
> >> +		if (encoder_funcs && encoder_funcs->mode_valid)
> >> +			ret = encoder_funcs->mode_valid(encoder, mode);
> >> +		else
> >> +			ret = MODE_OK; /* encoder accepts everything */
> >> +
> >> +		/* 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. */
> >> +		if (ret != MODE_OK)
> >> +			continue;
> > I think we should validate encoders against connector->possible_ids here.
> > Might be that not many drivers fill this out correctly, and in case fixing
> > that is much work, perhaps as a follow-up. But would be good to at least
> > help look down that part of uapi a bit more.
> 
> I'm sorry but I'm not following you here (I think I need more
> coffee :)).
> 
> What do you mean by connector->possible_ids ? Is this something
> new ? Because I didn't find anything in my tree and I'm based on
> today's drm-next from Dave.

Oops, I guess I was on an old tree or whatever by accident. I meant
drm_connector->encoder_ids, that limits the encoders you can use on a
crtc. For many drivers there's only one.

> > This isn't checked within atomic and legacy helpers since we assume that
> > ->best_encoder respectively ->atomic_best_encoder gives us a valid encoder
> > ...
> >
> >> +
> >> +		drm_for_each_crtc(crtc, dev) {
> >> +			const struct drm_crtc_helper_funcs *crtc_funcs;
> >> +
> >> +			if (!drm_encoder_crtc_ok(encoder, crtc))
> >> +				continue;
> > We check this one here already in both atomic and legacy helpers, so all
> > drivers should get this right.
> 
> But drm_for_each_crtc() iterates over all crtc from a device and
> some crtcs may only be used by specific encoders (by setting
> encoder->possible_crtcs), right ? We need to iterate only over
> the encoder's crtc we want to validate.

This was a comment about ->encoder_ids, since we don't validate that in
the atomic helpers (but instead rely on ->(atomic_)best_encoder to give us
the right encoder) validating here in this function might be a problem and
uncover driver bugs. But for drm_encoder_crtc_ok there's no such risk, so
this is safe.

I was simply dumping my thoughts while reviewing, the code is all good :-)

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