[PATCH v3 0/6] Introduce new mode validation callbacks

[PATCH v3 0/6] 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 and also by cleaning the documentation a little bit.

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

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

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

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

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

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

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

Cc: Carlos Palminha <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        |  76 +++++++++++++++++++-
 drivers/gpu/drm/drm_bridge.c               |  33 +++++++++
 drivers/gpu/drm/drm_crtc_helper_internal.h |  13 ++++
 drivers/gpu/drm/drm_probe_helper.c         | 103 ++++++++++++++++++++++++++-
 include/drm/drm_bridge.h                   |  22 ++++++
 include/drm/drm_modeset_helper_vtables.h   | 110 ++++++++++++++++++++++-------
 7 files changed, 348 insertions(+), 48 deletions(-)

-- 
1.9.1

[PATCH v3 1/6] 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.

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

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

Signed-off-by: Jose Abreu <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>
---

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

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

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..b07b7cd 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -106,14 +106,37 @@ struct drm_crtc_helper_funcs {
 	void (*commit)(struct drm_crtc *crtc);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * crtc. This should be implemented if the crtc has some sort of
+	 * restriction in the modes it can display. For example, a given crtc
+	 * may be responsible to set a clock value. If the clock can not
+	 * produce all the values for the available modes then this callback
+	 * can be used to restrict the number of modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This is called at mode probe and at atomic check phase.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
+					   const struct drm_display_mode *mode);
+
+	/**
 	 * @mode_fixup:
 	 *
-	 * This callback is used to validate a mode. The parameter mode is the
-	 * display mode that userspace requested, adjusted_mode is the mode the
-	 * encoders need to be fed with. Note that this is the inverse semantics
-	 * of the meaning for the &drm_encoder and &drm_bridge_funcs.mode_fixup
-	 * vfunc. If the CRTC cannot support the requested conversion from mode
-	 * to adjusted_mode it should reject the modeset.
+	 * This callback is used to do the validation of an adjusted mode in the
+	 * crtc. The parameter mode is the display mode that userspace requested,
+	 * adjusted_mode is the mode the encoders need to be fed with. Note that
+	 * this is the inverse semantics of the meaning for the &drm_encoder and
+	 * &drm_bridge_funcs.mode_fixup vfunc. If the CRTC cannot support the
+	 * requested conversion from mode to adjusted_mode it should reject the
+	 * modeset. Also note that initial validation of a mode supplied by
+	 * userspace should be done in &drm_crtc_helper_funcs.mode_valid and not
+	 * in this callback.
 	 *
 	 * This function is used by both legacy CRTC helpers and atomic helpers.
 	 * With atomic helpers it is optional.
@@ -135,17 +158,19 @@ struct drm_crtc_helper_funcs {
 	 * Also beware that neither core nor helpers filter modes before
 	 * passing them to the driver: While the list of modes that is
 	 * advertised to userspace is filtered using the
-	 * &drm_connector.mode_valid callback, neither the core nor the helpers
-	 * do any filtering on modes passed in from userspace when setting a
-	 * mode. It is therefore possible for userspace to pass in a mode that
-	 * was previously filtered out using &drm_connector.mode_valid or add a
-	 * custom mode that wasn't probed from EDID or similar to begin with.
-	 * Even though this is an advanced feature and rarely used nowadays,
-	 * some users rely on being able to specify modes manually so drivers
-	 * must be prepared to deal with it. Specifically this means that all
-	 * drivers need not only validate modes in &drm_connector.mode_valid but
-	 * also in this or in the &drm_encoder_helper_funcs.mode_fixup callback
-	 * to make sure invalid modes passed in from userspace are rejected.
+	 * &drm_connector_helper_funcs.mode_valid callback, neither the core nor
+	 * the helpers do any filtering on modes passed in from userspace when
+	 * setting a mode. It is therefore possible for userspace to pass in a
+	 * mode that was previously filtered out using
+	 * &drm_connector_helper_funcs.mode_valid or add a custom mode that
+	 * wasn't probed from EDID or similar to begin with. Even though this
+	 * is an advanced feature and rarely used nowadays, some users rely on
+	 * being able to specify modes manually so drivers must be prepared to
+	 * deal with it. Specifically this means that all drivers need not only
+	 * validate modes in &drm_connector.mode_valid but also in this or in
+	 * the &drm_crtc_helper_funcs.mode_valid,
+	 * &drm_encoder_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid
+	 * callbacks, where applicable.
 	 *
 	 * RETURNS:
 	 *
@@ -457,11 +482,31 @@ struct drm_encoder_helper_funcs {
 	void (*dpms)(struct drm_encoder *encoder, int mode);
 
 	/**
+	 * @mode_valid:
+	 *
+	 * This callback is used to check if a specific mode is valid in this
+	 * encoder. This should be implemented if the encoder has some sort
+	 * of restriction in the modes it can display. For example, a given
+	 * encoder may be responsible to set a clock value. If the clock can
+	 * not produce all the values for the available modes then this callback
+	 * can be used to restrict the number of modes to only the ones that
+	 * can be displayed.
+	 *
+	 * This is called at mode probe and at atomic check phase.
+	 *
+	 * RETURNS:
+	 *
+	 * drm_mode_status Enum
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc,
+					   const struct drm_display_mode *mode);
+
+	/**
 	 * @mode_fixup:
 	 *
-	 * This callback is used to validate and adjust a mode. The parameter
-	 * mode is the display mode that should be fed to the next element in
-	 * the display chain, either the final &drm_connector or a &drm_bridge.
+	 * This callback is used to adjust a mode. The parameter mode is the
+	 * display mode that should be fed to the next element in the display
+	 * chain, either the final &drm_connector or a &drm_bridge.
 	 * The parameter adjusted_mode is the input mode the encoder requires. It
 	 * can be modified by this callback and does not need to match mode.
 	 *
@@ -484,19 +529,20 @@ struct drm_encoder_helper_funcs {
 	 *
 	 * Also beware that neither core nor helpers filter modes before
 	 * passing them to the driver: While the list of modes that is
-	 * advertised to userspace is filtered using the connector's
+	 * advertised to userspace is filtered using the
 	 * &drm_connector_helper_funcs.mode_valid callback, neither the core nor
 	 * the helpers do any filtering on modes passed in from userspace when
 	 * setting a mode. It is therefore possible for userspace to pass in a
 	 * mode that was previously filtered out using
 	 * &drm_connector_helper_funcs.mode_valid or add a custom mode that
-	 * wasn't probed from EDID or similar to begin with.  Even though this
+	 * wasn't probed from EDID or similar to begin with. Even though this
 	 * is an advanced feature and rarely used nowadays, some users rely on
 	 * being able to specify modes manually so drivers must be prepared to
 	 * deal with it. Specifically this means that all drivers need not only
 	 * validate modes in &drm_connector.mode_valid but also in this or in
-	 * the &drm_crtc_helper_funcs.mode_fixup callback to make sure
-	 * invalid modes passed in from userspace are rejected.
+	 * the &drm_crtc_helper_funcs.mode_valid,
+	 * &drm_encoder_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid
+	 * callbacks, where applicable.
 	 *
 	 * RETURNS:
 	 *
@@ -795,13 +841,23 @@ struct drm_connector_helper_funcs {
 	 * (which is usually derived from the EDID data block from the sink).
 	 * See e.g. drm_helper_probe_single_connector_modes().
 	 *
+	 * This callback is never called in atomic check phase so that userspace
+	 * can override kernel sink checks in case of broken EDID with wrong
+	 * limits from the sink. You can use the remaining mode_valid()
+	 * callbacks to validate the mode against your video path.
+	 *
 	 * NOTE:
 	 *
 	 * This only filters the mode list supplied to userspace in the
-	 * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and
+	 * GETCONNECTOR IOCTL. Userspace is free to create modes of its own and
 	 * ask the kernel to use them. It this case the atomic helpers or legacy
-	 * CRTC helpers will not call this function. Drivers therefore must
-	 * still fully validate any mode passed in in a modeset request.
+	 * CRTC helpers will not call this function but the mode will be
+	 * validated in atomic check phase using the mode_valid() callbacks
+	 * (&drm_crtc_helper_funcs.mode_valid, &drm_encoder_helper_funcs.mode_valid
+	 * and &drm_bridge_funcs.mode_valid).
+	 * Drivers therefore must implement the mode_valid() callbacks if the
+	 * video pipeline has some sort of restrictions in the modes that can
+	 * be displayed.
 	 *
 	 * To avoid races with concurrent connector state updates, the helper
 	 * libraries always call this with the &drm_mode_config.connection_mutex
-- 
1.9.1

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

[Jose Abreu]

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

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
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>
---

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

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

diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h b/drivers/gpu/drm/drm_crtc_helper_internal.h
index 28295e5..97dfe20 100644
--- a/drivers/gpu/drm/drm_crtc_helper_internal.h
+++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
@@ -26,7 +26,11 @@
  * implementation details and are not exported to drivers.
  */
 
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
 #include <drm/drm_dp_helper.h>
+#include <drm/drm_encoder.h>
+#include <drm/drm_modes.h>
 
 /* drm_fb_helper.c */
 #ifdef CONFIG_DRM_FBDEV_EMULATION
@@ -62,4 +66,13 @@ static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
 static inline void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux)
 {
 }
+
+/* drm_probe_helper.c */
+enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
+					 const struct drm_display_mode *mode);
+enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder *encoder,
+					    const struct drm_display_mode *mode);
+enum drm_mode_status drm_connector_mode_valid(struct drm_connector *connector,
+					      struct drm_display_mode *mode);
+
 #endif
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 1b0c14a..f01abdc 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -38,6 +38,9 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_modeset_helper_vtables.h>
+
+#include "drm_crtc_helper_internal.h"
 
 /**
  * DOC: output probing helper overview
@@ -113,6 +116,41 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
 	return 1;
 }
 
+enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
+					 const struct drm_display_mode *mode)
+{
+	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
+
+	if (!crtc_funcs || !crtc_funcs->mode_valid)
+		return MODE_OK;
+
+	return crtc_funcs->mode_valid(crtc, mode);
+}
+
+enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder *encoder,
+					    const struct drm_display_mode *mode)
+{
+	const struct drm_encoder_helper_funcs *encoder_funcs =
+		encoder->helper_private;
+
+	if (!encoder_funcs || !encoder_funcs->mode_valid)
+		return MODE_OK;
+
+	return encoder_funcs->mode_valid(encoder, mode);
+}
+
+enum drm_mode_status drm_connector_mode_valid(struct drm_connector *connector,
+					      struct drm_display_mode *mode)
+{
+	const struct drm_connector_helper_funcs *connector_funcs =
+		connector->helper_private;
+
+	if (!connector_funcs || !connector_funcs->mode_valid)
+		return MODE_OK;
+
+	return connector_funcs->mode_valid(connector, mode);
+}
+
 #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
 /**
  * drm_kms_helper_poll_enable - re-enable output polling.
-- 
1.9.1

[PATCH v3 3/6] 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 v3 4/6] 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(), bridge->mode_valid() and crtc->mode_valid()
helper callbacks to validate the modes.

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

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

Signed-off-by: Jose Abreu <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>
---

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

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

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index f01abdc..84d660e 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -83,6 +83,61 @@
 	return MODE_OK;
 }
 
+static enum drm_mode_status
+drm_mode_validate_connector(struct drm_connector *connector,
+			    struct drm_display_mode *mode)
+{
+	struct drm_device *dev = connector->dev;
+	uint32_t *ids = connector->encoder_ids;
+	enum drm_mode_status ret = MODE_OK;
+	unsigned int i;
+
+	/* Step 1: Validate against connector */
+	ret = drm_connector_mode_valid(connector, mode);
+	if (ret != MODE_OK)
+		return ret;
+
+	/* Step 2: Validate against encoders and crtcs */
+	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
+		struct drm_encoder *encoder = drm_encoder_find(dev, ids[i]);
+		struct drm_crtc *crtc;
+
+		if (!encoder)
+			continue;
+
+		ret = drm_encoder_mode_valid(encoder, mode);
+		if (ret != MODE_OK) {
+			/* No point in continuing for crtc check as this encoder
+			 * will not accept the mode anyway. If all encoders
+			 * reject the mode then, at exit, ret will not be
+			 * MODE_OK. */
+			continue;
+		}
+
+		ret = drm_bridge_mode_valid(encoder->bridge, mode);
+		if (ret != MODE_OK) {
+			/* There is also no point in continuing for crtc check
+			 * here. */
+			continue;
+		}
+
+		drm_for_each_crtc(crtc, dev) {
+			if (!drm_encoder_crtc_ok(encoder, crtc))
+				continue;
+
+			ret = drm_crtc_mode_valid(crtc, mode);
+			if (ret == MODE_OK) {
+				/* If we get to this point there is at least
+				 * one combination of encoder+crtc that works
+				 * for this mode. Lets return now. */
+				return ret;
+			}
+		}
+	}
+
+	return ret;
+}
+
 static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
 {
 	struct drm_cmdline_mode *cmdline_mode;
@@ -322,7 +377,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
  *    - drm_mode_validate_flag() checks the modes against basic connector
  *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
  *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
- *      driver and/or hardware specific checks
+ *      driver and/or sink specific checks
+ *    - the optional &drm_crtc_helper_funcs.mode_valid,
+ *      &drm_bridge_funcs.mode_valid and &drm_encoder_helper_funcs.mode_valid
+ *      helpers can perform driver and/or source specific checks which are also
+ *      enforced by the modeset/atomic helpers
  *
  * 5. Any mode whose status is not OK is pruned from the connector's modes list,
  *    accompanied by a debug message indicating the reason for the mode's
@@ -466,8 +525,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		if (mode->status == MODE_OK)
 			mode->status = drm_mode_validate_flag(mode, mode_flags);
 
-		if (mode->status == MODE_OK && connector_funcs->mode_valid)
-			mode->status = connector_funcs->mode_valid(connector,
+		if (mode->status == MODE_OK)
+			mode->status = drm_mode_validate_connector(connector,
 								   mode);
 	}
 
-- 
1.9.1

[PATCH v3 5/6] 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 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>
---

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

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

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 8be9719..5a3c458 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -32,6 +32,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <linux/dma-fence.h>
 
+#include "drm_crtc_helper_internal.h"
 #include "drm_crtc_internal.h"
 
 /**
@@ -452,6 +453,69 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 	return 0;
 }
 
+static enum drm_mode_status mode_valid_path(struct drm_connector *connector,
+					    struct drm_encoder *encoder,
+					    struct drm_crtc *crtc,
+					    struct drm_display_mode *mode)
+{
+	enum drm_mode_status ret;
+
+	ret = drm_encoder_mode_valid(encoder, mode);
+	if (ret != MODE_OK) {
+		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] mode_valid() failed\n",
+				encoder->base.id, encoder->name);
+		return ret;
+	}
+
+	ret = drm_bridge_mode_valid(encoder->bridge, mode);
+	if (ret != MODE_OK) {
+		DRM_DEBUG_ATOMIC("[BRIDGE] mode_valid() failed\n");
+		return ret;
+	}
+
+	ret = drm_crtc_mode_valid(crtc, mode);
+	if (ret != MODE_OK) {
+		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode_valid() failed\n",
+				crtc->base.id, crtc->name);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int
+mode_valid(struct drm_atomic_state *state)
+{
+	struct drm_connector_state *conn_state;
+	struct drm_connector *connector;
+	int i;
+
+	for_each_new_connector_in_state(state, connector, conn_state, i) {
+		struct drm_encoder *encoder = conn_state->best_encoder;
+		struct drm_crtc *crtc = conn_state->crtc;
+		struct drm_crtc_state *crtc_state;
+		enum drm_mode_status mode_status;
+		struct drm_display_mode *mode;
+
+		if (!crtc || !encoder)
+			continue;
+
+		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+		if (!crtc_state)
+			continue;
+		if (!crtc_state->mode_changed && !crtc_state->connectors_changed)
+			continue;
+
+		mode = &crtc_state->mode;
+
+		mode_status = mode_valid_path(connector, encoder, crtc, mode);
+		if (mode_status != MODE_OK)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 /**
  * drm_atomic_helper_check_modeset - validate state object for modeset changes
  * @dev: DRM device
@@ -466,13 +530,15 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
  * 2. &drm_connector_helper_funcs.atomic_check to validate the connector state.
  * 3. If it's determined a modeset is needed then all connectors on the affected crtc
  *    crtc are added and &drm_connector_helper_funcs.atomic_check is run on them.
- * 4. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
- * 5. &drm_encoder_helper_funcs.atomic_check is called to validate any encoder state.
+ * 4. &drm_encoder_helper_funcs.mode_valid, &drm_bridge_funcs.mode_valid and
+ *    &drm_crtc_helper_funcs.mode_valid are called on the affected components.
+ * 5. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
+ * 6. &drm_encoder_helper_funcs.atomic_check is called to validate any encoder state.
  *    This function is only called when the encoder will be part of a configured crtc,
  *    it must not be used for implementing connector property validation.
  *    If this function is NULL, &drm_atomic_encoder_helper_funcs.mode_fixup is called
  *    instead.
- * 6. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints.
+ * 7. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints.
  *
  * &drm_crtc_state.mode_changed is set when the input mode is changed.
  * &drm_crtc_state.connectors_changed is set when a connector is added or
@@ -617,6 +683,10 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
 			return ret;
 	}
 
+	ret = mode_valid(state);
+	if (ret)
+		return ret;
+
 	return mode_fixup(state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
-- 
1.9.1

[PATCH v3 6/6] 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 v3 1/6] drm: Add crtc/encoder/bridge->mode_valid() callbacks

[Daniel Vetter]

On Thu, May 11, 2017 at 10:05:57AM +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.
> 
> We also change the documentation so that the new and old callbacks
> are correctly documented.
> 
> Only the callbacks were implemented to simplify review process,
> following patches will make use of them.
> 
> Signed-off-by: Jose Abreu <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>
> ---
> 
> Changes v2->v3:
> 	- Try to document the callbacks a little bit better and
> 	review current documentation (Daniel)

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

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

> Changes v1->v2:
> 	- Change description of connector->mode_valid() (Daniel)
> 
>  include/drm/drm_bridge.h                 |  20 ++++++
>  include/drm/drm_modeset_helper_vtables.h | 110 +++++++++++++++++++++++--------
>  2 files changed, 103 insertions(+), 27 deletions(-)
> 
> 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..b07b7cd 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -106,14 +106,37 @@ struct drm_crtc_helper_funcs {
>  	void (*commit)(struct drm_crtc *crtc);
>  
>  	/**
> +	 * @mode_valid:
> +	 *
> +	 * This callback is used to check if a specific mode is valid in this
> +	 * crtc. This should be implemented if the crtc has some sort of
> +	 * restriction in the modes it can display. For example, a given crtc
> +	 * may be responsible to set a clock value. If the clock can not
> +	 * produce all the values for the available modes then this callback
> +	 * can be used to restrict the number of modes to only the ones that
> +	 * can be displayed.
> +	 *
> +	 * This is called at mode probe and at atomic check phase.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * drm_mode_status Enum
> +	 */
> +	enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
> +					   const struct drm_display_mode *mode);
> +
> +	/**
>  	 * @mode_fixup:
>  	 *
> -	 * This callback is used to validate a mode. The parameter mode is the
> -	 * display mode that userspace requested, adjusted_mode is the mode the
> -	 * encoders need to be fed with. Note that this is the inverse semantics
> -	 * of the meaning for the &drm_encoder and &drm_bridge_funcs.mode_fixup
> -	 * vfunc. If the CRTC cannot support the requested conversion from mode
> -	 * to adjusted_mode it should reject the modeset.
> +	 * This callback is used to do the validation of an adjusted mode in the
> +	 * crtc. The parameter mode is the display mode that userspace requested,
> +	 * adjusted_mode is the mode the encoders need to be fed with. Note that
> +	 * this is the inverse semantics of the meaning for the &drm_encoder and
> +	 * &drm_bridge_funcs.mode_fixup vfunc. If the CRTC cannot support the
> +	 * requested conversion from mode to adjusted_mode it should reject the
> +	 * modeset. Also note that initial validation of a mode supplied by
> +	 * userspace should be done in &drm_crtc_helper_funcs.mode_valid and not
> +	 * in this callback.
>  	 *
>  	 * This function is used by both legacy CRTC helpers and atomic helpers.
>  	 * With atomic helpers it is optional.
> @@ -135,17 +158,19 @@ struct drm_crtc_helper_funcs {
>  	 * Also beware that neither core nor helpers filter modes before
>  	 * passing them to the driver: While the list of modes that is
>  	 * advertised to userspace is filtered using the
> -	 * &drm_connector.mode_valid callback, neither the core nor the helpers
> -	 * do any filtering on modes passed in from userspace when setting a
> -	 * mode. It is therefore possible for userspace to pass in a mode that
> -	 * was previously filtered out using &drm_connector.mode_valid or add a
> -	 * custom mode that wasn't probed from EDID or similar to begin with.
> -	 * Even though this is an advanced feature and rarely used nowadays,
> -	 * some users rely on being able to specify modes manually so drivers
> -	 * must be prepared to deal with it. Specifically this means that all
> -	 * drivers need not only validate modes in &drm_connector.mode_valid but
> -	 * also in this or in the &drm_encoder_helper_funcs.mode_fixup callback
> -	 * to make sure invalid modes passed in from userspace are rejected.
> +	 * &drm_connector_helper_funcs.mode_valid callback, neither the core nor
> +	 * the helpers do any filtering on modes passed in from userspace when
> +	 * setting a mode. It is therefore possible for userspace to pass in a
> +	 * mode that was previously filtered out using
> +	 * &drm_connector_helper_funcs.mode_valid or add a custom mode that
> +	 * wasn't probed from EDID or similar to begin with. Even though this
> +	 * is an advanced feature and rarely used nowadays, some users rely on
> +	 * being able to specify modes manually so drivers must be prepared to
> +	 * deal with it. Specifically this means that all drivers need not only
> +	 * validate modes in &drm_connector.mode_valid but also in this or in
> +	 * the &drm_crtc_helper_funcs.mode_valid,
> +	 * &drm_encoder_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid
> +	 * callbacks, where applicable.
>  	 *
>  	 * RETURNS:
>  	 *
> @@ -457,11 +482,31 @@ struct drm_encoder_helper_funcs {
>  	void (*dpms)(struct drm_encoder *encoder, int mode);
>  
>  	/**
> +	 * @mode_valid:
> +	 *
> +	 * This callback is used to check if a specific mode is valid in this
> +	 * encoder. This should be implemented if the encoder has some sort
> +	 * of restriction in the modes it can display. For example, a given
> +	 * encoder may be responsible to set a clock value. If the clock can
> +	 * not produce all the values for the available modes then this callback
> +	 * can be used to restrict the number of modes to only the ones that
> +	 * can be displayed.
> +	 *
> +	 * This is called at mode probe and at atomic check phase.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * drm_mode_status Enum
> +	 */
> +	enum drm_mode_status (*mode_valid)(struct drm_encoder *crtc,
> +					   const struct drm_display_mode *mode);
> +
> +	/**
>  	 * @mode_fixup:
>  	 *
> -	 * This callback is used to validate and adjust a mode. The parameter
> -	 * mode is the display mode that should be fed to the next element in
> -	 * the display chain, either the final &drm_connector or a &drm_bridge.
> +	 * This callback is used to adjust a mode. The parameter mode is the
> +	 * display mode that should be fed to the next element in the display
> +	 * chain, either the final &drm_connector or a &drm_bridge.
>  	 * The parameter adjusted_mode is the input mode the encoder requires. It
>  	 * can be modified by this callback and does not need to match mode.
>  	 *
> @@ -484,19 +529,20 @@ struct drm_encoder_helper_funcs {
>  	 *
>  	 * Also beware that neither core nor helpers filter modes before
>  	 * passing them to the driver: While the list of modes that is
> -	 * advertised to userspace is filtered using the connector's
> +	 * advertised to userspace is filtered using the
>  	 * &drm_connector_helper_funcs.mode_valid callback, neither the core nor
>  	 * the helpers do any filtering on modes passed in from userspace when
>  	 * setting a mode. It is therefore possible for userspace to pass in a
>  	 * mode that was previously filtered out using
>  	 * &drm_connector_helper_funcs.mode_valid or add a custom mode that
> -	 * wasn't probed from EDID or similar to begin with.  Even though this
> +	 * wasn't probed from EDID or similar to begin with. Even though this
>  	 * is an advanced feature and rarely used nowadays, some users rely on
>  	 * being able to specify modes manually so drivers must be prepared to
>  	 * deal with it. Specifically this means that all drivers need not only
>  	 * validate modes in &drm_connector.mode_valid but also in this or in
> -	 * the &drm_crtc_helper_funcs.mode_fixup callback to make sure
> -	 * invalid modes passed in from userspace are rejected.
> +	 * the &drm_crtc_helper_funcs.mode_valid,
> +	 * &drm_encoder_helper_funcs.mode_valid and &drm_bridge_funcs.mode_valid
> +	 * callbacks, where applicable.
>  	 *
>  	 * RETURNS:
>  	 *
> @@ -795,13 +841,23 @@ struct drm_connector_helper_funcs {
>  	 * (which is usually derived from the EDID data block from the sink).
>  	 * See e.g. drm_helper_probe_single_connector_modes().
>  	 *
> +	 * This callback is never called in atomic check phase so that userspace
> +	 * can override kernel sink checks in case of broken EDID with wrong
> +	 * limits from the sink. You can use the remaining mode_valid()
> +	 * callbacks to validate the mode against your video path.
> +	 *
>  	 * NOTE:
>  	 *
>  	 * This only filters the mode list supplied to userspace in the
> -	 * GETCONNECOTR IOCTL. Userspace is free to create modes of its own and
> +	 * GETCONNECTOR IOCTL. Userspace is free to create modes of its own and
>  	 * ask the kernel to use them. It this case the atomic helpers or legacy
> -	 * CRTC helpers will not call this function. Drivers therefore must
> -	 * still fully validate any mode passed in in a modeset request.
> +	 * CRTC helpers will not call this function but the mode will be
> +	 * validated in atomic check phase using the mode_valid() callbacks
> +	 * (&drm_crtc_helper_funcs.mode_valid, &drm_encoder_helper_funcs.mode_valid
> +	 * and &drm_bridge_funcs.mode_valid).
> +	 * Drivers therefore must implement the mode_valid() callbacks if the
> +	 * video pipeline has some sort of restrictions in the modes that can
> +	 * be displayed.
>  	 *
>  	 * To avoid races with concurrent connector state updates, the helper
>  	 * libraries always call this with the &drm_mode_config.connection_mutex
> -- 
> 1.9.1
> 
> 

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

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

[Daniel Vetter]

On Thu, May 11, 2017 at 10:05:56AM +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 and also by cleaning the documentation a little bit.
> 
> We proceed by introducing new helpers to call this new callbacks
> at 2/6.
> 
> At 3/6 a helper function is introduced that calls all mode_valid()
> from a set of bridges.
> 
> Next, at 4/6 we modify the connector probe helper so that only modes
> which are supported by a given bridge+encoder+crtc combination are
> probbed.
> 
> At 5/6 we call all the mode_valid() callbacks for a given pipeline,
> except the connector->mode_valid one, so that the mode is validated.
> This is done before calling mode_fixup().
> 
> Finally, at 6/6 we use the new crtc->mode_valid() callback in arcpgu
> and remove the atomic_check() callback.
> 
> [1] https://patchwork.kernel.org/patch/9702233/
> 
> Jose Abreu (6):
>   drm: Add crtc/encoder/bridge->mode_valid() callbacks
>   drm: Add drm_{crtc/encoder/connector}_mode_valid()
>   drm: Introduce drm_bridge_mode_valid()
>   drm: Use new mode_valid() helpers in connector probe helper
>   drm: Use mode_valid() in atomic modeset
>   drm: arc: Use crtc->mode_valid() callback
> 
> Cc: Carlos Palminha <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>

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

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

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

Thanks, Daniel

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

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

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

[Andrzej Hajda]

On 12.05.2017 09:32, Daniel Vetter wrote:
> On Thu, May 11, 2017 at 10:05:56AM +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 and also by cleaning the documentation a little bit.
>>
>> We proceed by introducing new helpers to call this new callbacks
>> at 2/6.
>>
>> At 3/6 a helper function is introduced that calls all mode_valid()
>> from a set of bridges.
>>
>> Next, at 4/6 we modify the connector probe helper so that only modes
>> which are supported by a given bridge+encoder+crtc combination are
>> probbed.
>>
>> At 5/6 we call all the mode_valid() callbacks for a given pipeline,
>> except the connector->mode_valid one, so that the mode is validated.
>> This is done before calling mode_fixup().
>>
>> Finally, at 6/6 we use the new crtc->mode_valid() callback in arcpgu
>> and remove the atomic_check() callback.
>>
>> [1] https://patchwork.kernel.org/patch/9702233/
>>
>> Jose Abreu (6):
>>   drm: Add crtc/encoder/bridge->mode_valid() callbacks
>>   drm: Add drm_{crtc/encoder/connector}_mode_valid()
>>   drm: Introduce drm_bridge_mode_valid()
>>   drm: Use new mode_valid() helpers in connector probe helper
>>   drm: Use mode_valid() in atomic modeset
>>   drm: arc: Use crtc->mode_valid() callback
>>
>> Cc: Carlos Palminha <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>
> Commented with an entire patch on patch 1, patches 2-5 are all
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> But I think some more acks/r-bs would be really good, since this is quite
> a bit change in the helper infrastructure. But otherwise ready for merging
> imo. Can you pls also review my proposal for patch 1?
>
> Thanks, Daniel

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

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

Regards
Andrzej


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

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

[Andrzej Hajda]

On 11.05.2017 11:05, Jose Abreu wrote:
> Add a new helper to call crtc->mode_valid, connector->mode_valid
> and encoder->mode_valid callbacks.
>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 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>
> ---
> Changes v2->v3:
> 	- Move helpers to drm_probe_helper.c (Daniel)
> 	- Squeeze patches that introduce the helpers into a single
> 	one (Daniel)
>
>  drivers/gpu/drm/drm_crtc_helper_internal.h | 13 ++++++++++
>  drivers/gpu/drm/drm_probe_helper.c         | 38 ++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h b/drivers/gpu/drm/drm_crtc_helper_internal.h
> index 28295e5..97dfe20 100644
> --- a/drivers/gpu/drm/drm_crtc_helper_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
> @@ -26,7 +26,11 @@
>   * implementation details and are not exported to drivers.
>   */
>  
> +#include <drm/drm_connector.h>
> +#include <drm/drm_crtc.h>
>  #include <drm/drm_dp_helper.h>
> +#include <drm/drm_encoder.h>
> +#include <drm/drm_modes.h>
>  
>  /* drm_fb_helper.c */
>  #ifdef CONFIG_DRM_FBDEV_EMULATION
> @@ -62,4 +66,13 @@ static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
>  static inline void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux)
>  {
>  }
> +
> +/* drm_probe_helper.c */
> +enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
> +					 const struct drm_display_mode *mode);
> +enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder *encoder,
> +					    const struct drm_display_mode *mode);
> +enum drm_mode_status drm_connector_mode_valid(struct drm_connector *connector,
> +					      struct drm_display_mode *mode);
> +
>  #endif
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 1b0c14a..f01abdc 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -38,6 +38,9 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_modeset_helper_vtables.h>
> +
> +#include "drm_crtc_helper_internal.h"
>  
>  /**
>   * DOC: output probing helper overview
> @@ -113,6 +116,41 @@ static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>  	return 1;
>  }
>  
> +enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
> +					 const struct drm_display_mode *mode)
> +{
> +	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
> +
> +	if (!crtc_funcs || !crtc_funcs->mode_valid)
> +		return MODE_OK;
> +
> +	return crtc_funcs->mode_valid(crtc, mode);
> +}
> +
> +enum drm_mode_status drm_encoder_mode_valid(struct drm_encoder *encoder,
> +					    const struct drm_display_mode *mode)
> +{
> +	const struct drm_encoder_helper_funcs *encoder_funcs =
> +		encoder->helper_private;
> +
> +	if (!encoder_funcs || !encoder_funcs->mode_valid)
> +		return MODE_OK;
> +
> +	return encoder_funcs->mode_valid(encoder, mode);
> +}
> +
> +enum drm_mode_status drm_connector_mode_valid(struct drm_connector *connector,
> +					      struct drm_display_mode *mode)
> +{
> +	const struct drm_connector_helper_funcs *connector_funcs =
> +		connector->helper_private;
> +
> +	if (!connector_funcs || !connector_funcs->mode_valid)
> +		return MODE_OK;
> +
> +	return connector_funcs->mode_valid(connector, mode);
> +}
> +

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

Anyway:
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

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

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

[Andrzej Hajda]

On 11.05.2017 11:06, Jose Abreu wrote:
> This changes the connector probe helper function to use the new
> encoder->mode_valid(), bridge->mode_valid() and crtc->mode_valid()
> helper callbacks to validate the modes.
>
> The new callbacks are optional so the behaviour remains the same
> if they are not implemented. If they are, then the code loops
> through all the connector's encodersXbridgesXcrtcs and calls the
> callback.
>
> If at least a valid encoderXbridgeXcrtc combination is found which
> accepts the mode then the function returns MODE_OK.
>
> Signed-off-by: Jose Abreu <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>
> ---
>
> Changes v2->v3:
> 	- Call also bridge->mode_valid (Daniel)
> Changes v1->v2:
> 	- Use new helpers suggested by Ville
> 	- Change documentation (Daniel)
>
>  drivers/gpu/drm/drm_probe_helper.c | 65 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 62 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index f01abdc..84d660e 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -83,6 +83,61 @@
>  	return MODE_OK;
>  }
>  
> +static enum drm_mode_status
> +drm_mode_validate_connector(struct drm_connector *connector,
> +			    struct drm_display_mode *mode)
> +{
> +	struct drm_device *dev = connector->dev;
> +	uint32_t *ids = connector->encoder_ids;
> +	enum drm_mode_status ret = MODE_OK;
> +	unsigned int i;
> +
> +	/* Step 1: Validate against connector */
> +	ret = drm_connector_mode_valid(connector, mode);
> +	if (ret != MODE_OK)
> +		return ret;
> +
> +	/* Step 2: Validate against encoders and crtcs */
> +	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> +		struct drm_encoder *encoder = drm_encoder_find(dev, ids[i]);
> +		struct drm_crtc *crtc;
> +
> +		if (!encoder)
> +			continue;
> +
> +		ret = drm_encoder_mode_valid(encoder, mode);
> +		if (ret != MODE_OK) {
> +			/* No point in continuing for crtc check as this encoder
> +			 * will not accept the mode anyway. If all encoders
> +			 * reject the mode then, at exit, ret will not be
> +			 * MODE_OK. */
> +			continue;
> +		}
> +
> +		ret = drm_bridge_mode_valid(encoder->bridge, mode);
> +		if (ret != MODE_OK) {
> +			/* There is also no point in continuing for crtc check
> +			 * here. */
> +			continue;
> +		}

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

Another concern is about order of calls, it is from sink to source, to
keep it consistent bridge should be called before encoder, am I right?
Beside this:
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

> +
> +		drm_for_each_crtc(crtc, dev) {
> +			if (!drm_encoder_crtc_ok(encoder, crtc))
> +				continue;
> +
> +			ret = drm_crtc_mode_valid(crtc, mode);
> +			if (ret == MODE_OK) {
> +				/* If we get to this point there is at least
> +				 * one combination of encoder+crtc that works
> +				 * for this mode. Lets return now. */
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>  {
>  	struct drm_cmdline_mode *cmdline_mode;
> @@ -322,7 +377,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>   *    - drm_mode_validate_flag() checks the modes against basic connector
>   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
>   *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
> - *      driver and/or hardware specific checks
> + *      driver and/or sink specific checks
> + *    - the optional &drm_crtc_helper_funcs.mode_valid,
> + *      &drm_bridge_funcs.mode_valid and &drm_encoder_helper_funcs.mode_valid
> + *      helpers can perform driver and/or source specific checks which are also
> + *      enforced by the modeset/atomic helpers
>   *
>   * 5. Any mode whose status is not OK is pruned from the connector's modes list,
>   *    accompanied by a debug message indicating the reason for the mode's
> @@ -466,8 +525,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  		if (mode->status == MODE_OK)
>  			mode->status = drm_mode_validate_flag(mode, mode_flags);
>  
> -		if (mode->status == MODE_OK && connector_funcs->mode_valid)
> -			mode->status = connector_funcs->mode_valid(connector,
> +		if (mode->status == MODE_OK)
> +			mode->status = drm_mode_validate_connector(connector,
>  								   mode);
>  	}
>  

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

[Andrzej Hajda]

On 11.05.2017 11:06, 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 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>

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

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

[Daniel Vetter]

On Thu, May 11, 2017 at 10:06:02AM +0100, 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 <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>

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

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

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

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

Thanks, Daniel

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

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

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

[Andrzej Hajda]

On 11.05.2017 11:06, 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 <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>

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

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

[Daniel Vetter]

On Mon, May 15, 2017 at 10:39:35AM +0200, Andrzej Hajda wrote:
> On 11.05.2017 11:06, Jose Abreu wrote:
> > This changes the connector probe helper function to use the new
> > encoder->mode_valid(), bridge->mode_valid() and crtc->mode_valid()
> > helper callbacks to validate the modes.
> >
> > The new callbacks are optional so the behaviour remains the same
> > if they are not implemented. If they are, then the code loops
> > through all the connector's encodersXbridgesXcrtcs and calls the
> > callback.
> >
> > If at least a valid encoderXbridgeXcrtc combination is found which
> > accepts the mode then the function returns MODE_OK.
> >
> > Signed-off-by: Jose Abreu <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>
> > ---
> >
> > Changes v2->v3:
> > 	- Call also bridge->mode_valid (Daniel)
> > Changes v1->v2:
> > 	- Use new helpers suggested by Ville
> > 	- Change documentation (Daniel)
> >
> >  drivers/gpu/drm/drm_probe_helper.c | 65 ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 62 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index f01abdc..84d660e 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -83,6 +83,61 @@
> >  	return MODE_OK;
> >  }
> >  
> > +static enum drm_mode_status
> > +drm_mode_validate_connector(struct drm_connector *connector,
> > +			    struct drm_display_mode *mode)
> > +{
> > +	struct drm_device *dev = connector->dev;
> > +	uint32_t *ids = connector->encoder_ids;
> > +	enum drm_mode_status ret = MODE_OK;
> > +	unsigned int i;
> > +
> > +	/* Step 1: Validate against connector */
> > +	ret = drm_connector_mode_valid(connector, mode);
> > +	if (ret != MODE_OK)
> > +		return ret;
> > +
> > +	/* Step 2: Validate against encoders and crtcs */
> > +	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > +		struct drm_encoder *encoder = drm_encoder_find(dev, ids[i]);
> > +		struct drm_crtc *crtc;
> > +
> > +		if (!encoder)
> > +			continue;
> > +
> > +		ret = drm_encoder_mode_valid(encoder, mode);
> > +		if (ret != MODE_OK) {
> > +			/* No point in continuing for crtc check as this encoder
> > +			 * will not accept the mode anyway. If all encoders
> > +			 * reject the mode then, at exit, ret will not be
> > +			 * MODE_OK. */
> > +			continue;
> > +		}
> > +
> > +		ret = drm_bridge_mode_valid(encoder->bridge, mode);
> > +		if (ret != MODE_OK) {
> > +			/* There is also no point in continuing for crtc check
> > +			 * here. */
> > +			continue;
> > +		}
> 
> Maybe it is a bikeshedding, but wouldn't be better to call
> drm_bridge_mode_valid from drm_encoder_mode_valid, in general call all
> bridge related stuff from corresponding encoder stuff?
> This is more question about role of encoder->bridge, should it be
> treated as encoder's extension, or as 1st class citizen in drm?
> 
> Another concern is about order of calls, it is from sink to source, to
> keep it consistent bridge should be called before encoder, am I right?

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

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

Does that address your concern?
-Daniel

> Beside this:
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> 
>  --
> Regards
> Andrzej
> 
> > +
> > +		drm_for_each_crtc(crtc, dev) {
> > +			if (!drm_encoder_crtc_ok(encoder, crtc))
> > +				continue;
> > +
> > +			ret = drm_crtc_mode_valid(crtc, mode);
> > +			if (ret == MODE_OK) {
> > +				/* If we get to this point there is at least
> > +				 * one combination of encoder+crtc that works
> > +				 * for this mode. Lets return now. */
> > +				return ret;
> > +			}
> > +		}
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
> >  {
> >  	struct drm_cmdline_mode *cmdline_mode;
> > @@ -322,7 +377,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
> >   *    - drm_mode_validate_flag() checks the modes against basic connector
> >   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
> >   *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
> > - *      driver and/or hardware specific checks
> > + *      driver and/or sink specific checks
> > + *    - the optional &drm_crtc_helper_funcs.mode_valid,
> > + *      &drm_bridge_funcs.mode_valid and &drm_encoder_helper_funcs.mode_valid
> > + *      helpers can perform driver and/or source specific checks which are also
> > + *      enforced by the modeset/atomic helpers
> >   *
> >   * 5. Any mode whose status is not OK is pruned from the connector's modes list,
> >   *    accompanied by a debug message indicating the reason for the mode's
> > @@ -466,8 +525,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> >  		if (mode->status == MODE_OK)
> >  			mode->status = drm_mode_validate_flag(mode, mode_flags);
> >  
> > -		if (mode->status == MODE_OK && connector_funcs->mode_valid)
> > -			mode->status = connector_funcs->mode_valid(connector,
> > +		if (mode->status == MODE_OK)
> > +			mode->status = drm_mode_validate_connector(connector,
> >  								   mode);
> >  	}
> >  
> 
> 

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

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

[Andrzej Hajda]

On 15.05.2017 11:30, Daniel Vetter wrote:
> On Mon, May 15, 2017 at 10:39:35AM +0200, Andrzej Hajda wrote:
>> On 11.05.2017 11:06, Jose Abreu wrote:
>>> This changes the connector probe helper function to use the new
>>> encoder->mode_valid(), bridge->mode_valid() and crtc->mode_valid()
>>> helper callbacks to validate the modes.
>>>
>>> The new callbacks are optional so the behaviour remains the same
>>> if they are not implemented. If they are, then the code loops
>>> through all the connector's encodersXbridgesXcrtcs and calls the
>>> callback.
>>>
>>> If at least a valid encoderXbridgeXcrtc combination is found which
>>> accepts the mode then the function returns MODE_OK.
>>>
>>> Signed-off-by: Jose Abreu <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>
>>> ---
>>>
>>> Changes v2->v3:
>>> 	- Call also bridge->mode_valid (Daniel)
>>> Changes v1->v2:
>>> 	- Use new helpers suggested by Ville
>>> 	- Change documentation (Daniel)
>>>
>>>  drivers/gpu/drm/drm_probe_helper.c | 65 ++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 62 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>>> index f01abdc..84d660e 100644
>>> --- a/drivers/gpu/drm/drm_probe_helper.c
>>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>>> @@ -83,6 +83,61 @@
>>>  	return MODE_OK;
>>>  }
>>>  
>>> +static enum drm_mode_status
>>> +drm_mode_validate_connector(struct drm_connector *connector,
>>> +			    struct drm_display_mode *mode)
>>> +{
>>> +	struct drm_device *dev = connector->dev;
>>> +	uint32_t *ids = connector->encoder_ids;
>>> +	enum drm_mode_status ret = MODE_OK;
>>> +	unsigned int i;
>>> +
>>> +	/* Step 1: Validate against connector */
>>> +	ret = drm_connector_mode_valid(connector, mode);
>>> +	if (ret != MODE_OK)
>>> +		return ret;
>>> +
>>> +	/* Step 2: Validate against encoders and crtcs */
>>> +	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
>>> +		struct drm_encoder *encoder = drm_encoder_find(dev, ids[i]);
>>> +		struct drm_crtc *crtc;
>>> +
>>> +		if (!encoder)
>>> +			continue;
>>> +
>>> +		ret = drm_encoder_mode_valid(encoder, mode);
>>> +		if (ret != MODE_OK) {
>>> +			/* No point in continuing for crtc check as this encoder
>>> +			 * will not accept the mode anyway. If all encoders
>>> +			 * reject the mode then, at exit, ret will not be
>>> +			 * MODE_OK. */
>>> +			continue;
>>> +		}
>>> +
>>> +		ret = drm_bridge_mode_valid(encoder->bridge, mode);
>>> +		if (ret != MODE_OK) {
>>> +			/* There is also no point in continuing for crtc check
>>> +			 * here. */
>>> +			continue;
>>> +		}
>> Maybe it is a bikeshedding, but wouldn't be better to call
>> drm_bridge_mode_valid from drm_encoder_mode_valid, in general call all
>> bridge related stuff from corresponding encoder stuff?
>> This is more question about role of encoder->bridge, should it be
>> treated as encoder's extension, or as 1st class citizen in drm?
>>
>> Another concern is about order of calls, it is from sink to source, to
>> keep it consistent bridge should be called before encoder, am I right?
> For the atomic_check stuff (where we do change the passed-in mode) this
> would be correct, and calling order and layering would matter. But this
> just validates the mode in turn with everything, not taking any
> cross-component constraint or other configuration-dependent constraints
> into account. Hence it doesn't matter in which order we call stuff.
>
> Note that the passed-in mode is const, so you can't escape. And v3 of
> patch 1 now has added wording that you're not allowed to look at anything
> else dynamie either.
>
> Does that address your concern?

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

Regards
Andrzej

> -Daniel
>
>> Beside this:
>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>>
>>  --
>> Regards
>> Andrzej
>>
>>> +
>>> +		drm_for_each_crtc(crtc, dev) {
>>> +			if (!drm_encoder_crtc_ok(encoder, crtc))
>>> +				continue;
>>> +
>>> +			ret = drm_crtc_mode_valid(crtc, mode);
>>> +			if (ret == MODE_OK) {
>>> +				/* If we get to this point there is at least
>>> +				 * one combination of encoder+crtc that works
>>> +				 * for this mode. Lets return now. */
>>> +				return ret;
>>> +			}
>>> +		}
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>  static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
>>>  {
>>>  	struct drm_cmdline_mode *cmdline_mode;
>>> @@ -322,7 +377,11 @@ void drm_kms_helper_poll_enable(struct drm_device *dev)
>>>   *    - drm_mode_validate_flag() checks the modes against basic connector
>>>   *      capabilities (interlace_allowed,doublescan_allowed,stereo_allowed)
>>>   *    - the optional &drm_connector_helper_funcs.mode_valid helper can perform
>>> - *      driver and/or hardware specific checks
>>> + *      driver and/or sink specific checks
>>> + *    - the optional &drm_crtc_helper_funcs.mode_valid,
>>> + *      &drm_bridge_funcs.mode_valid and &drm_encoder_helper_funcs.mode_valid
>>> + *      helpers can perform driver and/or source specific checks which are also
>>> + *      enforced by the modeset/atomic helpers
>>>   *
>>>   * 5. Any mode whose status is not OK is pruned from the connector's modes list,
>>>   *    accompanied by a debug message indicating the reason for the mode's
>>> @@ -466,8 +525,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>>>  		if (mode->status == MODE_OK)
>>>  			mode->status = drm_mode_validate_flag(mode, mode_flags);
>>>  
>>> -		if (mode->status == MODE_OK && connector_funcs->mode_valid)
>>> -			mode->status = connector_funcs->mode_valid(connector,
>>> +		if (mode->status == MODE_OK)
>>> +			mode->status = drm_mode_validate_connector(connector,
>>>  								   mode);
>>>  	}
>>>  
>>

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

[Daniel Vetter]

On Mon, May 15, 2017 at 10:53:25AM +0200, Daniel Vetter wrote:
> On Thu, May 11, 2017 at 10:06:02AM +0100, 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 <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>
> 
> Btw for justifying your patch series a bit more, would be real sweet to
> roll ->mode_valid out to more drivers. I see two easy cases:
> 
> bridge/analogix-anx78xx.c: has a simple mode_fixup which really is just a
> mode_valid callback
> 
> bridge/synopsys/dw-hdmi.c: simply hand-rolls what you've done here, we
> could move the connector_mode valid to the bridge and done.
> 
> Care to type these 2 patches on top, just to make this a bit more useful
> and a clearer case for merging?

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

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

> 
> Thanks, Daniel
> 
> > ---
> >  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
> > 
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

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

[Jose Abreu]

Hi Daniel,


On 15-05-2017 16:52, Daniel Vetter wrote:
> On Mon, May 15, 2017 at 10:53:25AM +0200, Daniel Vetter wrote:
>> On Thu, May 11, 2017 at 10:06:02AM +0100, 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 <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>
>> Btw for justifying your patch series a bit more, would be real sweet to
>> roll ->mode_valid out to more drivers. I see two easy cases:
>>
>> bridge/analogix-anx78xx.c: has a simple mode_fixup which really is just a
>> mode_valid callback
>>
>> bridge/synopsys/dw-hdmi.c: simply hand-rolls what you've done here, we
>> could move the connector_mode valid to the bridge and done.
>>
>> Care to type these 2 patches on top, just to make this a bit more useful
>> and a clearer case for merging?
> Aside: There's a pile more drivers (encoders and crtc drivers) which would
> similarly benefit and dont really look like they'd be hard to patch up:
> - malidp_crtc_mode_fixup
> - armada_drm_crtc_mode_fixup (armada isn't atomic, so forget this one)
> - atmel_hlcdc_crtc_mode_fixup
> - hdmi_mode_fixup in exynos_hdmi.c
> - vc4_crtc_mode_fixup and vc4_dpi_encoder_mode_fixup (they both just check
>   for interlaced modes, we could probably dump the same check in
>   connector->mode_valid).
>
> Plus the 2 bridge drivers. Everyone else does either something more
> complex, or something that's not quite correct.
> -Daniel

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

Best regards,
Jose Miguel Abreu

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