[PATCH v5 00/10] Introduce new mode validation callbacks

[PATCH v5 00/10] Introduce new mode validation callbacks

[Jose Abreu]

NOTE: In this version I just did a rebase onto today's drm-next and
added the tags we've collected plus the changelog plus the missing
maintainers that I forgot to cc in previous version :/

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. This
patch is available at [2] and the series depends on it.

We proceed by introducing new helpers to call this new callbacks
at 1/10.

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

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

At 4/10 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, from 5/10 to 10/10 we use the new callbacks in drivers that
can implement it.

[1] https://patchwork.kernel.org/patch/9702233/
[2] https://lists.freedesktop.org/archives/dri-devel/2017-May/141761.html

Jose Abreu (10):
  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: arcpgu: Use crtc->mode_valid() callback
  drm/bridge: analogix-anx78xx: Use bridge->mode_valid() callback
  drm/bridge/synopsys: dw-hdmi: Use bridge->mode_valid() callback
  drm/arm: malidp: Use crtc->mode_valid() callback
  drm/atmel-hlcdc: Use crtc->mode_valid() callback
  drm: vc4: Use crtc->mode_valid() and encoder->mode_valid() callbacks

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>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

 drivers/gpu/drm/arc/arcpgu_crtc.c              |  29 ++++---
 drivers/gpu/drm/arm/malidp_crtc.c              |  11 ++-
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c |   9 +--
 drivers/gpu/drm/bridge/analogix-anx78xx.c      |  13 ++-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c      |  40 +++-------
 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             | 105 ++++++++++++++++++++++++-
 drivers/gpu/drm/imx/dw_hdmi-imx.c              |   4 +-
 drivers/gpu/drm/meson/meson_dw_hdmi.c          |   2 +-
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c    |   2 +-
 drivers/gpu/drm/vc4/vc4_crtc.c                 |  13 ++-
 drivers/gpu/drm/vc4/vc4_dpi.c                  |  13 ++-
 include/drm/bridge/dw_hdmi.h                   |   2 +-
 include/drm/drm_bridge.h                       |   2 +
 16 files changed, 280 insertions(+), 87 deletions(-)

-- 
1.9.1

[PATCH v5 01/10] 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>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Dave Airlie <airlied@linux.ie>

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

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
---
 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

Re: [PATCH v5 01/10] drm: Add drm_{crtc/encoder/connector}_mode_valid()

[Daniel Vetter]

On Thu, May 25, 2017 at 03:19:13PM +0100, 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>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Dave Airlie <airlied@linux.ie>
> 
> Changes v2->v3:
> 	- Move helpers to drm_probe_helper.c (Daniel)
> 	- Squeeze patches that introduce helpers into a single
> 	one (Daniel)
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> ---
>  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);
> +

This here is the end of the CONFIG_DRM_DP_AUX_CHARDEV #else block, I've
moved them out to make this compile here.
-Daniel

>  #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
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH v5 01/10] drm: Add drm_{crtc/encoder/connector}_mode_valid()

[Neil Armstrong]

On 05/25/2017 04:19 PM, 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>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Dave Airlie <airlied@linux.ie>
> 
> Changes v2->v3:
> 	- Move helpers to drm_probe_helper.c (Daniel)
> 	- Squeeze patches that introduce helpers into a single
> 	one (Daniel)
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> ---
>  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.
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

[PATCH v5 02/10] 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>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 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

Re: [PATCH v5 02/10] drm: Introduce drm_bridge_mode_valid()

[Archit Taneja]

On 05/25/2017 07:49 PM, Jose Abreu wrote:
> Introduce a new helper function which calls mode_valid() callback
> for all bridges in an encoder chain.
>

Reviewed-by: Archit Taneja <architt@codeaurora.org>

> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  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,
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v5 02/10] drm: Introduce drm_bridge_mode_valid()

[Neil Armstrong]

On 05/25/2017 04:19 PM, Jose Abreu wrote:
> 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>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  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,
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

[PATCH v5 03/10] 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>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Changes v3->v4:
	- Change function name (Laurent)
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 | 67 +++++++++++++++++++++++++++++++++++---
 1 file changed, 63 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index f01abdc..00e6832 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_pipeline(struct drm_display_mode *mode,
+			    struct drm_connector *connector)
+{
+	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,9 +525,9 @@ 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,
-								   mode);
+		if (mode->status == MODE_OK)
+			mode->status = drm_mode_validate_pipeline(mode,
+								  connector);
 	}
 
 prune:
-- 
1.9.1

Re: [PATCH v5 03/10] drm: Use new mode_valid() helpers in connector probe helper

[Neil Armstrong]

On 05/25/2017 04:19 PM, 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>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Changes v3->v4:
> 	- Change function name (Laurent)
> 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 | 67 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 63 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index f01abdc..00e6832 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_pipeline(struct drm_display_mode *mode,
> +			    struct drm_connector *connector)
> +{
> +	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,9 +525,9 @@ 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,
> -								   mode);
> +		if (mode->status == MODE_OK)
> +			mode->status = drm_mode_validate_pipeline(mode,
> +								  connector);
>  	}
>  
>  prune:
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

[PATCH v5 04/10] 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>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Cc: Carlos Palminha <palminha@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>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Changes v1->v2:
	- Removed call to connector->mode_valid (Ville, Daniel)
	- Changed 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

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
---
 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 6426339..21afca4 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

Re: [PATCH v5 04/10] drm: Use mode_valid() in atomic modeset

[Daniel Vetter]

On Thu, May 25, 2017 at 03:19:16PM +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>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Carlos Palminha <palminha@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>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Changes v1->v2:
> 	- Removed call to connector->mode_valid (Ville, Daniel)
> 	- Changed 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
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>

Ok, merged up to this one to drm-misc-next, thanks a lot. I'll leave
merging the conversion patches to respective maintainers.
-Daniel

> ---
>  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 6426339..21afca4 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
> 
> 

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

Re: [PATCH v5 04/10] drm: Use mode_valid() in atomic modeset

[Neil Armstrong]

On 05/25/2017 04:19 PM, 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>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Carlos Palminha <palminha@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>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Changes v1->v2:
> 	- Removed call to connector->mode_valid (Ville, Daniel)
> 	- Changed 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
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> ---
>  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 6426339..21afca4 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);
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

[PATCH v5 05/10] drm: arcpgu: 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>
Reviewed-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Changes v4->v5:
	- Change commit message to "arcpgu" (Alexey)
Changes v3->v4:
	- Do not use aux function (Laurent)

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

diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
index ad9a959..99fbdae 100644
--- a/drivers/gpu/drm/arc/arcpgu_crtc.c
+++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
@@ -64,6 +64,19 @@ 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);
+	long rate, clk_rate = mode->clock * 1000;
+
+	rate = clk_round_rate(arcpgu->clk, clk_rate);
+	if (rate != clk_rate)
+		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 +142,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 +157,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 +165,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 v5 05/10] drm: arcpgu: Use crtc->mode_valid() callback

[Neil Armstrong]

On 05/25/2017 04:19 PM, 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>
> Reviewed-by: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Changes v4->v5:
> 	- Change commit message to "arcpgu" (Alexey)
> Changes v3->v4:
> 	- Do not use aux function (Laurent)
> 
> ---
>  drivers/gpu/drm/arc/arcpgu_crtc.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
> index ad9a959..99fbdae 100644
> --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
> +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
> @@ -64,6 +64,19 @@ 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);
> +	long rate, clk_rate = mode->clock * 1000;
> +
> +	rate = clk_round_rate(arcpgu->clk, clk_rate);
> +	if (rate != clk_rate)
> +		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 +142,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 +157,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 +165,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,
>  };
>  
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH v5 05/10] drm: arcpgu: Use crtc->mode_valid() callback

[Jose Abreu]

Hi Daniel, Alexey,


On 25-05-2017 15:19, 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>
> Reviewed-by: Alexey Brodkin <abrodkin@synopsys.com>

@Daniel: I know that arcpgu is not under drm-misc but could you
apply this into drm-misc-next? Alexey (the arcpgu maintainer)
already gave the reviewed-by and this is also "Reviewed-by: Neil
Armstrong <narmstrong@baylibre.com>". It was also tested-by: me.

@Alexey: Is this ok for you?

I need this because I have a pending patch for accepting clock
variations instead of always failing when clk_round_rate() does
not return the expected rate.

Best regards,
Jose Miguel Abreu

> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Changes v4->v5:
> 	- Change commit message to "arcpgu" (Alexey)
> Changes v3->v4:
> 	- Do not use aux function (Laurent)
>
> ---
>  drivers/gpu/drm/arc/arcpgu_crtc.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
> index ad9a959..99fbdae 100644
> --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
> +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
> @@ -64,6 +64,19 @@ 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);
> +	long rate, clk_rate = mode->clock * 1000;
> +
> +	rate = clk_round_rate(arcpgu->clk, clk_rate);
> +	if (rate != clk_rate)
> +		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 +142,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 +157,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 +165,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,
>  };
>  

Re: [PATCH v5 05/10] drm: arcpgu: Use crtc->mode_valid() callback

[Daniel Vetter]

On Wed, Jun 21, 2017 at 10:38:43AM +0100, Jose Abreu wrote:
> Hi Daniel, Alexey,
> 
> 
> On 25-05-2017 15:19, 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>
> > Reviewed-by: Alexey Brodkin <abrodkin@synopsys.com>
> 
> @Daniel: I know that arcpgu is not under drm-misc but could you
> apply this into drm-misc-next? Alexey (the arcpgu maintainer)
> already gave the reviewed-by and this is also "Reviewed-by: Neil
> Armstrong <narmstrong@baylibre.com>". It was also tested-by: me.
> 
> @Alexey: Is this ok for you?
> 
> I need this because I have a pending patch for accepting clock
> variations instead of always failing when clk_round_rate() does
> not return the expected rate.

Alexey reviewed it already, and Dave acked it for merging through
drm-misc, so applied.

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

[PATCH v5 06/10] drm/bridge: analogix-anx78xx: Use bridge->mode_valid() callback

[Jose Abreu]

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

Also, there is no need to use mode_fixup() callback as mode_valid()
will handle the mode validation.

NOTE: Only compile tested.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: David Airlie <airlied@linux.ie>
---
 drivers/gpu/drm/bridge/analogix-anx78xx.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c
index a2a8236..cf69a1c 100644
--- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
+++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
@@ -1061,18 +1061,17 @@ static int anx78xx_bridge_attach(struct drm_bridge *bridge)
 	return 0;
 }
 
-static bool anx78xx_bridge_mode_fixup(struct drm_bridge *bridge,
-				      const struct drm_display_mode *mode,
-				      struct drm_display_mode *adjusted_mode)
+enum drm_mode_status anx78xx_bridge_mode_valid(struct drm_bridge *bridge,
+					       const struct drm_display_mode *mode)
 {
 	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
-		return false;
+		return MODE_NO_INTERLACE;
 
 	/* Max 1200p at 5.4 Ghz, one lane */
 	if (mode->clock > 154000)
-		return false;
+		return MODE_CLOCK_HIGH;
 
-	return true;
+	return MODE_OK;
 }
 
 static void anx78xx_bridge_disable(struct drm_bridge *bridge)
@@ -1129,7 +1128,7 @@ static void anx78xx_bridge_enable(struct drm_bridge *bridge)
 
 static const struct drm_bridge_funcs anx78xx_bridge_funcs = {
 	.attach = anx78xx_bridge_attach,
-	.mode_fixup = anx78xx_bridge_mode_fixup,
+	.mode_valid = anx78xx_bridge_mode_valid,
 	.disable = anx78xx_bridge_disable,
 	.mode_set = anx78xx_bridge_mode_set,
 	.enable = anx78xx_bridge_enable,
-- 
1.9.1

Re: [PATCH v5 06/10] drm/bridge: analogix-anx78xx: Use bridge->mode_valid() callback

[Archit Taneja]

On 05/25/2017 07:49 PM, Jose Abreu wrote:
> Now that we have a callback to check if bridge supports a given mode
> we can use it in Analogix bridge so that we restrict the number of
> probbed modes to the ones we can actually display.
>
> Also, there is no need to use mode_fixup() callback as mode_valid()
> will handle the mode validation.
>

Reviewed-by: Archit Taneja <architt@codeaurora.org>

> NOTE: Only compile tested.
>
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: David Airlie <airlied@linux.ie>
> ---
>  drivers/gpu/drm/bridge/analogix-anx78xx.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c
> index a2a8236..cf69a1c 100644
> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
> @@ -1061,18 +1061,17 @@ static int anx78xx_bridge_attach(struct drm_bridge *bridge)
>  	return 0;
>  }
>
> -static bool anx78xx_bridge_mode_fixup(struct drm_bridge *bridge,
> -				      const struct drm_display_mode *mode,
> -				      struct drm_display_mode *adjusted_mode)
> +enum drm_mode_status anx78xx_bridge_mode_valid(struct drm_bridge *bridge,
> +					       const struct drm_display_mode *mode)
>  {
>  	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> -		return false;
> +		return MODE_NO_INTERLACE;
>
>  	/* Max 1200p at 5.4 Ghz, one lane */
>  	if (mode->clock > 154000)
> -		return false;
> +		return MODE_CLOCK_HIGH;
>
> -	return true;
> +	return MODE_OK;
>  }
>
>  static void anx78xx_bridge_disable(struct drm_bridge *bridge)
> @@ -1129,7 +1128,7 @@ static void anx78xx_bridge_enable(struct drm_bridge *bridge)
>
>  static const struct drm_bridge_funcs anx78xx_bridge_funcs = {
>  	.attach = anx78xx_bridge_attach,
> -	.mode_fixup = anx78xx_bridge_mode_fixup,
> +	.mode_valid = anx78xx_bridge_mode_valid,
>  	.disable = anx78xx_bridge_disable,
>  	.mode_set = anx78xx_bridge_mode_set,
>  	.enable = anx78xx_bridge_enable,
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v5 06/10] drm/bridge: analogix-anx78xx: Use bridge->mode_valid() callback

[Neil Armstrong]

On 05/25/2017 04:19 PM, Jose Abreu wrote:
> Now that we have a callback to check if bridge supports a given mode
> we can use it in Analogix bridge so that we restrict the number of
> probbed modes to the ones we can actually display.
> 
> Also, there is no need to use mode_fixup() callback as mode_valid()
> will handle the mode validation.
> 
> NOTE: Only compile tested.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: David Airlie <airlied@linux.ie>
> ---
>  drivers/gpu/drm/bridge/analogix-anx78xx.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c
> index a2a8236..cf69a1c 100644
> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
> @@ -1061,18 +1061,17 @@ static int anx78xx_bridge_attach(struct drm_bridge *bridge)
>  	return 0;
>  }
>  
> -static bool anx78xx_bridge_mode_fixup(struct drm_bridge *bridge,
> -				      const struct drm_display_mode *mode,
> -				      struct drm_display_mode *adjusted_mode)
> +enum drm_mode_status anx78xx_bridge_mode_valid(struct drm_bridge *bridge,
> +					       const struct drm_display_mode *mode)
>  {
>  	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> -		return false;
> +		return MODE_NO_INTERLACE;
>  
>  	/* Max 1200p at 5.4 Ghz, one lane */
>  	if (mode->clock > 154000)
> -		return false;
> +		return MODE_CLOCK_HIGH;
>  
> -	return true;
> +	return MODE_OK;
>  }
>  
>  static void anx78xx_bridge_disable(struct drm_bridge *bridge)
> @@ -1129,7 +1128,7 @@ static void anx78xx_bridge_enable(struct drm_bridge *bridge)
>  
>  static const struct drm_bridge_funcs anx78xx_bridge_funcs = {
>  	.attach = anx78xx_bridge_attach,
> -	.mode_fixup = anx78xx_bridge_mode_fixup,
> +	.mode_valid = anx78xx_bridge_mode_valid,
>  	.disable = anx78xx_bridge_disable,
>  	.mode_set = anx78xx_bridge_mode_set,
>  	.enable = anx78xx_bridge_enable,
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH v5 06/10] drm/bridge: analogix-anx78xx: Use bridge->mode_valid() callback

[Archit Taneja]

On 05/30/2017 12:58 PM, Neil Armstrong wrote:
> On 05/25/2017 04:19 PM, Jose Abreu wrote:
>> Now that we have a callback to check if bridge supports a given mode
>> we can use it in Analogix bridge so that we restrict the number of
>> probbed modes to the ones we can actually display.
>>
>> Also, there is no need to use mode_fixup() callback as mode_valid()
>> will handle the mode validation.
>>
>> NOTE: Only compile tested.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Carlos Palminha <palminha@synopsys.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Archit Taneja <architt@codeaurora.org>
>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>> Cc: David Airlie <airlied@linux.ie>
>> ---
>>  drivers/gpu/drm/bridge/analogix-anx78xx.c | 13 ++++++-------
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c
>> index a2a8236..cf69a1c 100644
>> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
>> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
>> @@ -1061,18 +1061,17 @@ static int anx78xx_bridge_attach(struct drm_bridge *bridge)
>>  	return 0;
>>  }
>>
>> -static bool anx78xx_bridge_mode_fixup(struct drm_bridge *bridge,
>> -				      const struct drm_display_mode *mode,
>> -				      struct drm_display_mode *adjusted_mode)
>> +enum drm_mode_status anx78xx_bridge_mode_valid(struct drm_bridge *bridge,
>> +					       const struct drm_display_mode *mode)

Queued to drm-misc after adding static again^ and fixing up 80 line warning.

Thanks,
Archit

>>  {
>>  	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>> -		return false;
>> +		return MODE_NO_INTERLACE;
>>
>>  	/* Max 1200p at 5.4 Ghz, one lane */
>>  	if (mode->clock > 154000)
>> -		return false;
>> +		return MODE_CLOCK_HIGH;
>>
>> -	return true;
>> +	return MODE_OK;
>>  }
>>
>>  static void anx78xx_bridge_disable(struct drm_bridge *bridge)
>> @@ -1129,7 +1128,7 @@ static void anx78xx_bridge_enable(struct drm_bridge *bridge)
>>
>>  static const struct drm_bridge_funcs anx78xx_bridge_funcs = {
>>  	.attach = anx78xx_bridge_attach,
>> -	.mode_fixup = anx78xx_bridge_mode_fixup,
>> +	.mode_valid = anx78xx_bridge_mode_valid,
>>  	.disable = anx78xx_bridge_disable,
>>  	.mode_set = anx78xx_bridge_mode_set,
>>  	.enable = anx78xx_bridge_enable,
>>
>
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v5 07/10] drm/bridge/synopsys: dw-hdmi: Use bridge->mode_valid() callback

[Jose Abreu]

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

Also, there is no need to use mode_fixup() callback as mode_valid()
will handle the mode validation.

NOTE: Only compile tested
NOTE 2: I also had to change the pdata declaration of mode_valid
custom callback so that the passed modes are const. I also changed
in the platforms I found. Not even compiled it though.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Carlo Caione <carlo@caione.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Mark Yao <mark.yao@rock-chips.com>
Cc: Heiko Stuebner <heiko@sntech.de>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c   | 40 +++++++++--------------------
 drivers/gpu/drm/imx/dw_hdmi-imx.c           |  4 +--
 drivers/gpu/drm/meson/meson_dw_hdmi.c       |  2 +-
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
 include/drm/bridge/dw_hdmi.h                |  2 +-
 5 files changed, 17 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 8737de8..038dc43 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -1907,24 +1907,6 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
 	return ret;
 }
 
-static enum drm_mode_status
-dw_hdmi_connector_mode_valid(struct drm_connector *connector,
-			     struct drm_display_mode *mode)
-{
-	struct dw_hdmi *hdmi = container_of(connector,
-					   struct dw_hdmi, connector);
-	enum drm_mode_status mode_status = MODE_OK;
-
-	/* We don't support double-clocked modes */
-	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
-		return MODE_BAD;
-
-	if (hdmi->plat_data->mode_valid)
-		mode_status = hdmi->plat_data->mode_valid(connector, mode);
-
-	return mode_status;
-}
-
 static void dw_hdmi_connector_force(struct drm_connector *connector)
 {
 	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
@@ -1950,7 +1932,6 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
 
 static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = {
 	.get_modes = dw_hdmi_connector_get_modes,
-	.mode_valid = dw_hdmi_connector_mode_valid,
 	.best_encoder = drm_atomic_helper_best_encoder,
 };
 
@@ -1973,18 +1954,21 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge)
 	return 0;
 }
 
-static bool dw_hdmi_bridge_mode_fixup(struct drm_bridge *bridge,
-				      const struct drm_display_mode *orig_mode,
-				      struct drm_display_mode *mode)
+static enum drm_mode_status dw_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+						      const struct drm_display_mode *mode)
 {
 	struct dw_hdmi *hdmi = bridge->driver_private;
 	struct drm_connector *connector = &hdmi->connector;
-	enum drm_mode_status status;
+	enum drm_mode_status mode_status = MODE_OK;
 
-	status = dw_hdmi_connector_mode_valid(connector, mode);
-	if (status != MODE_OK)
-		return false;
-	return true;
+	/* We don't support double-clocked modes */
+	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
+		return MODE_BAD;
+
+	if (hdmi->plat_data->mode_valid)
+		mode_status = hdmi->plat_data->mode_valid(connector, mode);
+
+	return mode_status;
 }
 
 static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge,
@@ -2028,7 +2012,7 @@ static void dw_hdmi_bridge_enable(struct drm_bridge *bridge)
 	.enable = dw_hdmi_bridge_enable,
 	.disable = dw_hdmi_bridge_disable,
 	.mode_set = dw_hdmi_bridge_mode_set,
-	.mode_fixup = dw_hdmi_bridge_mode_fixup,
+	.mode_valid = dw_hdmi_bridge_mode_valid,
 };
 
 static irqreturn_t dw_hdmi_i2c_irq(struct dw_hdmi *hdmi)
diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index f039641..5f561c8 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -148,7 +148,7 @@ static int dw_hdmi_imx_atomic_check(struct drm_encoder *encoder,
 };
 
 static enum drm_mode_status imx6q_hdmi_mode_valid(struct drm_connector *con,
-						  struct drm_display_mode *mode)
+						  const struct drm_display_mode *mode)
 {
 	if (mode->clock < 13500)
 		return MODE_CLOCK_LOW;
@@ -160,7 +160,7 @@ static enum drm_mode_status imx6q_hdmi_mode_valid(struct drm_connector *con,
 }
 
 static enum drm_mode_status imx6dl_hdmi_mode_valid(struct drm_connector *con,
-						   struct drm_display_mode *mode)
+						   const struct drm_display_mode *mode)
 {
 	if (mode->clock < 13500)
 		return MODE_CLOCK_LOW;
diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 7b86eb7..f043904 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -537,7 +537,7 @@ static irqreturn_t dw_hdmi_top_thread_irq(int irq, void *dev_id)
 
 /* TOFIX Enable support for non-vic modes */
 static enum drm_mode_status dw_hdmi_mode_valid(struct drm_connector *connector,
-					       struct drm_display_mode *mode)
+					       const struct drm_display_mode *mode)
 {
 	unsigned int vclk_freq;
 	unsigned int venc_freq;
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 63dab6f..f820848 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -155,7 +155,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
 
 static enum drm_mode_status
 dw_hdmi_rockchip_mode_valid(struct drm_connector *connector,
-			    struct drm_display_mode *mode)
+			    const struct drm_display_mode *mode)
 {
 	const struct dw_hdmi_mpll_config *mpll_cfg = rockchip_mpll_cfg;
 	int pclk = mode->clock * 1000;
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index ed599be..4c8d4c8 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -125,7 +125,7 @@ struct dw_hdmi_phy_ops {
 struct dw_hdmi_plat_data {
 	struct regmap *regm;
 	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
-					   struct drm_display_mode *mode);
+					   const struct drm_display_mode *mode);
 	unsigned long input_bus_format;
 	unsigned long input_bus_encoding;
 
-- 
1.9.1

Re: [PATCH v5 07/10] drm/bridge/synopsys: dw-hdmi: Use bridge->mode_valid() callback

[Neil Armstrong]

On 05/25/2017 04:19 PM, Jose Abreu wrote:
> Now that we have a callback to check if bridge supports a given mode
> we can use it in Synopsys Designware HDMI bridge so that we restrict
> the number of probbed modes to the ones we can actually display.
> 
> Also, there is no need to use mode_fixup() callback as mode_valid()
> will handle the mode validation.
> 
> NOTE: Only compile tested
> NOTE 2: I also had to change the pdata declaration of mode_valid
> custom callback so that the passed modes are const. I also changed
> in the platforms I found. Not even compiled it though.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Carlo Caione <carlo@caione.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Mark Yao <mark.yao@rock-chips.com>
> Cc: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c   | 40 +++++++++--------------------
>  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  4 +--
>  drivers/gpu/drm/meson/meson_dw_hdmi.c       |  2 +-
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
>  include/drm/bridge/dw_hdmi.h                |  2 +-
>  5 files changed, 17 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 8737de8..038dc43 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -1907,24 +1907,6 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
>  	return ret;
>  }
>  
> -static enum drm_mode_status
> -dw_hdmi_connector_mode_valid(struct drm_connector *connector,
> -			     struct drm_display_mode *mode)
> -{
> -	struct dw_hdmi *hdmi = container_of(connector,
> -					   struct dw_hdmi, connector);
> -	enum drm_mode_status mode_status = MODE_OK;
> -
> -	/* We don't support double-clocked modes */
> -	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> -		return MODE_BAD;
> -
> -	if (hdmi->plat_data->mode_valid)
> -		mode_status = hdmi->plat_data->mode_valid(connector, mode);
> -
> -	return mode_status;
> -}
> -
>  static void dw_hdmi_connector_force(struct drm_connector *connector)
>  {
>  	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
> @@ -1950,7 +1932,6 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
>  
>  static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = {
>  	.get_modes = dw_hdmi_connector_get_modes,
> -	.mode_valid = dw_hdmi_connector_mode_valid,
>  	.best_encoder = drm_atomic_helper_best_encoder,
>  };
>  
> @@ -1973,18 +1954,21 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge)
>  	return 0;
>  }
>  
> -static bool dw_hdmi_bridge_mode_fixup(struct drm_bridge *bridge,
> -				      const struct drm_display_mode *orig_mode,
> -				      struct drm_display_mode *mode)
> +static enum drm_mode_status dw_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
> +						      const struct drm_display_mode *mode)
>  {
>  	struct dw_hdmi *hdmi = bridge->driver_private;
>  	struct drm_connector *connector = &hdmi->connector;
> -	enum drm_mode_status status;
> +	enum drm_mode_status mode_status = MODE_OK;
>  
> -	status = dw_hdmi_connector_mode_valid(connector, mode);
> -	if (status != MODE_OK)
> -		return false;
> -	return true;
> +	/* We don't support double-clocked modes */
> +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> +		return MODE_BAD;
> +
> +	if (hdmi->plat_data->mode_valid)
> +		mode_status = hdmi->plat_data->mode_valid(connector, mode);
> +
> +	return mode_status;
>  }
>  
>  static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge,
> @@ -2028,7 +2012,7 @@ static void dw_hdmi_bridge_enable(struct drm_bridge *bridge)
>  	.enable = dw_hdmi_bridge_enable,
>  	.disable = dw_hdmi_bridge_disable,
>  	.mode_set = dw_hdmi_bridge_mode_set,
> -	.mode_fixup = dw_hdmi_bridge_mode_fixup,
> +	.mode_valid = dw_hdmi_bridge_mode_valid,
>  };
>  
>  static irqreturn_t dw_hdmi_i2c_irq(struct dw_hdmi *hdmi)
> diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> index f039641..5f561c8 100644
> --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
> +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> @@ -148,7 +148,7 @@ static int dw_hdmi_imx_atomic_check(struct drm_encoder *encoder,
>  };
>  
>  static enum drm_mode_status imx6q_hdmi_mode_valid(struct drm_connector *con,
> -						  struct drm_display_mode *mode)
> +						  const struct drm_display_mode *mode)
>  {
>  	if (mode->clock < 13500)
>  		return MODE_CLOCK_LOW;
> @@ -160,7 +160,7 @@ static enum drm_mode_status imx6q_hdmi_mode_valid(struct drm_connector *con,
>  }
>  
>  static enum drm_mode_status imx6dl_hdmi_mode_valid(struct drm_connector *con,
> -						   struct drm_display_mode *mode)
> +						   const struct drm_display_mode *mode)
>  {
>  	if (mode->clock < 13500)
>  		return MODE_CLOCK_LOW;
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 7b86eb7..f043904 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -537,7 +537,7 @@ static irqreturn_t dw_hdmi_top_thread_irq(int irq, void *dev_id)
>  
>  /* TOFIX Enable support for non-vic modes */
>  static enum drm_mode_status dw_hdmi_mode_valid(struct drm_connector *connector,
> -					       struct drm_display_mode *mode)
> +					       const struct drm_display_mode *mode)
>  {
>  	unsigned int vclk_freq;
>  	unsigned int venc_freq;
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index 63dab6f..f820848 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -155,7 +155,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>  
>  static enum drm_mode_status
>  dw_hdmi_rockchip_mode_valid(struct drm_connector *connector,
> -			    struct drm_display_mode *mode)
> +			    const struct drm_display_mode *mode)
>  {
>  	const struct dw_hdmi_mpll_config *mpll_cfg = rockchip_mpll_cfg;
>  	int pclk = mode->clock * 1000;
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index ed599be..4c8d4c8 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -125,7 +125,7 @@ struct dw_hdmi_phy_ops {
>  struct dw_hdmi_plat_data {
>  	struct regmap *regm;
>  	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
> -					   struct drm_display_mode *mode);
> +					   const struct drm_display_mode *mode);
>  	unsigned long input_bus_format;
>  	unsigned long input_bus_encoding;
>  
> 


Hi Jose,

Tested-by: Neil Armstrong <narmstrong@baylibre.com>

On Meson GXL, no issues.

Thanks,
Neil

Re: [PATCH v5 07/10] drm/bridge/synopsys: dw-hdmi: Use bridge->mode_valid() callback

[Archit Taneja]

Hi,

On 05/25/2017 07:49 PM, Jose Abreu wrote:
> Now that we have a callback to check if bridge supports a given mode
> we can use it in Synopsys Designware HDMI bridge so that we restrict
> the number of probbed modes to the ones we can actually display.
>
> Also, there is no need to use mode_fixup() callback as mode_valid()
> will handle the mode validation.
>
> NOTE: Only compile tested
> NOTE 2: I also had to change the pdata declaration of mode_valid
> custom callback so that the passed modes are const. I also changed
> in the platforms I found. Not even compiled it though.

This compiles fine and has been tested on Meson by Neil.

Since this also touches rockchip/imx drm driver files, can I get an
OK from the maintainers to pull the changes via drm-misc?

Thanks,
Archit

>
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Carlo Caione <carlo@caione.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Mark Yao <mark.yao@rock-chips.com>
> Cc: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c   | 40 +++++++++--------------------
>  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  4 +--
>  drivers/gpu/drm/meson/meson_dw_hdmi.c       |  2 +-
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
>  include/drm/bridge/dw_hdmi.h                |  2 +-
>  5 files changed, 17 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 8737de8..038dc43 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -1907,24 +1907,6 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
>  	return ret;
>  }
>
> -static enum drm_mode_status
> -dw_hdmi_connector_mode_valid(struct drm_connector *connector,
> -			     struct drm_display_mode *mode)
> -{
> -	struct dw_hdmi *hdmi = container_of(connector,
> -					   struct dw_hdmi, connector);
> -	enum drm_mode_status mode_status = MODE_OK;
> -
> -	/* We don't support double-clocked modes */
> -	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> -		return MODE_BAD;
> -
> -	if (hdmi->plat_data->mode_valid)
> -		mode_status = hdmi->plat_data->mode_valid(connector, mode);
> -
> -	return mode_status;
> -}
> -
>  static void dw_hdmi_connector_force(struct drm_connector *connector)
>  {
>  	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
> @@ -1950,7 +1932,6 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
>
>  static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = {
>  	.get_modes = dw_hdmi_connector_get_modes,
> -	.mode_valid = dw_hdmi_connector_mode_valid,
>  	.best_encoder = drm_atomic_helper_best_encoder,
>  };
>
> @@ -1973,18 +1954,21 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge)
>  	return 0;
>  }
>
> -static bool dw_hdmi_bridge_mode_fixup(struct drm_bridge *bridge,
> -				      const struct drm_display_mode *orig_mode,
> -				      struct drm_display_mode *mode)
> +static enum drm_mode_status dw_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
> +						      const struct drm_display_mode *mode)
>  {
>  	struct dw_hdmi *hdmi = bridge->driver_private;
>  	struct drm_connector *connector = &hdmi->connector;
> -	enum drm_mode_status status;
> +	enum drm_mode_status mode_status = MODE_OK;
>
> -	status = dw_hdmi_connector_mode_valid(connector, mode);
> -	if (status != MODE_OK)
> -		return false;
> -	return true;
> +	/* We don't support double-clocked modes */
> +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> +		return MODE_BAD;
> +
> +	if (hdmi->plat_data->mode_valid)
> +		mode_status = hdmi->plat_data->mode_valid(connector, mode);
> +
> +	return mode_status;
>  }
>
>  static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge,
> @@ -2028,7 +2012,7 @@ static void dw_hdmi_bridge_enable(struct drm_bridge *bridge)
>  	.enable = dw_hdmi_bridge_enable,
>  	.disable = dw_hdmi_bridge_disable,
>  	.mode_set = dw_hdmi_bridge_mode_set,
> -	.mode_fixup = dw_hdmi_bridge_mode_fixup,
> +	.mode_valid = dw_hdmi_bridge_mode_valid,
>  };
>
>  static irqreturn_t dw_hdmi_i2c_irq(struct dw_hdmi *hdmi)
> diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> index f039641..5f561c8 100644
> --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
> +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> @@ -148,7 +148,7 @@ static int dw_hdmi_imx_atomic_check(struct drm_encoder *encoder,
>  };
>
>  static enum drm_mode_status imx6q_hdmi_mode_valid(struct drm_connector *con,
> -						  struct drm_display_mode *mode)
> +						  const struct drm_display_mode *mode)
>  {
>  	if (mode->clock < 13500)
>  		return MODE_CLOCK_LOW;
> @@ -160,7 +160,7 @@ static enum drm_mode_status imx6q_hdmi_mode_valid(struct drm_connector *con,
>  }
>
>  static enum drm_mode_status imx6dl_hdmi_mode_valid(struct drm_connector *con,
> -						   struct drm_display_mode *mode)
> +						   const struct drm_display_mode *mode)
>  {
>  	if (mode->clock < 13500)
>  		return MODE_CLOCK_LOW;
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 7b86eb7..f043904 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -537,7 +537,7 @@ static irqreturn_t dw_hdmi_top_thread_irq(int irq, void *dev_id)
>
>  /* TOFIX Enable support for non-vic modes */
>  static enum drm_mode_status dw_hdmi_mode_valid(struct drm_connector *connector,
> -					       struct drm_display_mode *mode)
> +					       const struct drm_display_mode *mode)
>  {
>  	unsigned int vclk_freq;
>  	unsigned int venc_freq;
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index 63dab6f..f820848 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -155,7 +155,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>
>  static enum drm_mode_status
>  dw_hdmi_rockchip_mode_valid(struct drm_connector *connector,
> -			    struct drm_display_mode *mode)
> +			    const struct drm_display_mode *mode)
>  {
>  	const struct dw_hdmi_mpll_config *mpll_cfg = rockchip_mpll_cfg;
>  	int pclk = mode->clock * 1000;
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index ed599be..4c8d4c8 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -125,7 +125,7 @@ struct dw_hdmi_phy_ops {
>  struct dw_hdmi_plat_data {
>  	struct regmap *regm;
>  	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
> -					   struct drm_display_mode *mode);
> +					   const struct drm_display_mode *mode);
>  	unsigned long input_bus_format;
>  	unsigned long input_bus_encoding;
>
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v5 07/10] drm/bridge/synopsys: dw-hdmi: Use bridge->mode_valid() callback

[Philipp Zabel]

Hi Archit,

On Tue, 2017-05-30 at 15:54 +0530, Archit Taneja wrote:
> Hi,
> 
> On 05/25/2017 07:49 PM, Jose Abreu wrote:
> > Now that we have a callback to check if bridge supports a given mode
> > we can use it in Synopsys Designware HDMI bridge so that we restrict
> > the number of probbed modes to the ones we can actually display.
> >
> > Also, there is no need to use mode_fixup() callback as mode_valid()
> > will handle the mode validation.
> >
> > NOTE: Only compile tested
> > NOTE 2: I also had to change the pdata declaration of mode_valid
> > custom callback so that the passed modes are const. I also changed
> > in the platforms I found. Not even compiled it though.
> 
> This compiles fine and has been tested on Meson by Neil.
> 
> Since this also touches rockchip/imx drm driver files, can I get an
> OK from the maintainers to pull the changes via drm-misc?

Of course,
Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH v5 07/10] drm/bridge/synopsys: dw-hdmi: Use bridge->mode_valid() callback

[Archit Taneja]

On 05/30/2017 03:59 PM, Philipp Zabel wrote:
> Hi Archit,
>
> On Tue, 2017-05-30 at 15:54 +0530, Archit Taneja wrote:
>> Hi,
>>
>> On 05/25/2017 07:49 PM, Jose Abreu wrote:
>>> Now that we have a callback to check if bridge supports a given mode
>>> we can use it in Synopsys Designware HDMI bridge so that we restrict
>>> the number of probbed modes to the ones we can actually display.
>>>
>>> Also, there is no need to use mode_fixup() callback as mode_valid()
>>> will handle the mode validation.
>>>
>>> NOTE: Only compile tested
>>> NOTE 2: I also had to change the pdata declaration of mode_valid
>>> custom callback so that the passed modes are const. I also changed
>>> in the platforms I found. Not even compiled it though.
>>
>> This compiles fine and has been tested on Meson by Neil.
>>
>> Since this also touches rockchip/imx drm driver files, can I get an
>> OK from the maintainers to pull the changes via drm-misc?
>
> Of course,
> Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

Thanks for the Ack.

I didn't get an ack from Rockchip, but still going ahead with
the push since it's been a while and the change is very minor.

Thanks,
Archit

>
> regards
> Philipp
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v5 08/10] drm/arm: malidp: 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 malidp so that we restrict the number of probbed
modes to the ones we can actually display.

Also, remove the mode_fixup() callback as this is no longer needed
because mode_valid() will be called before.

NOTE: Not even compiled tested

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Brian Starkey <brian.starkey@arm.com>
Cc: David Airlie <airlied@linux.ie>
---
 drivers/gpu/drm/arm/malidp_crtc.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
index 9446a67..4bb38a2 100644
--- a/drivers/gpu/drm/arm/malidp_crtc.c
+++ b/drivers/gpu/drm/arm/malidp_crtc.c
@@ -22,9 +22,8 @@
 #include "malidp_drv.h"
 #include "malidp_hw.h"
 
-static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc,
-				   const struct drm_display_mode *mode,
-				   struct drm_display_mode *adjusted_mode)
+static enum drm_mode_status malidp_crtc_mode_valid(struct drm_crtc *crtc,
+						   const struct drm_display_mode *mode)
 {
 	struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
 	struct malidp_hw_device *hwdev = malidp->dev;
@@ -40,11 +39,11 @@ static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc,
 		if (rate != req_rate) {
 			DRM_DEBUG_DRIVER("pxlclk doesn't support %ld Hz\n",
 					 req_rate);
-			return false;
+			return MODE_NOCLOCK;
 		}
 	}
 
-	return true;
+	return MODE_OK;
 }
 
 static void malidp_crtc_enable(struct drm_crtc *crtc)
@@ -408,7 +407,7 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs malidp_crtc_helper_funcs = {
-	.mode_fixup = malidp_crtc_mode_fixup,
+	.mode_valid = malidp_crtc_mode_valid,
 	.enable = malidp_crtc_enable,
 	.disable = malidp_crtc_disable,
 	.atomic_check = malidp_crtc_atomic_check,
-- 
1.9.1

Re: [PATCH v5 08/10] drm/arm: malidp: Use crtc->mode_valid() callback

[Neil Armstrong]

On 05/25/2017 04:19 PM, Jose Abreu wrote:
> Now that we have a callback to check if crtc supports a given mode
> we can use it in malidp so that we restrict the number of probbed
> modes to the ones we can actually display.
> 
> Also, remove the mode_fixup() callback as this is no longer needed
> because mode_valid() will be called before.
> 
> NOTE: Not even compiled tested
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Brian Starkey <brian.starkey@arm.com>
> Cc: David Airlie <airlied@linux.ie>
> ---
>  drivers/gpu/drm/arm/malidp_crtc.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
> index 9446a67..4bb38a2 100644
> --- a/drivers/gpu/drm/arm/malidp_crtc.c
> +++ b/drivers/gpu/drm/arm/malidp_crtc.c
> @@ -22,9 +22,8 @@
>  #include "malidp_drv.h"
>  #include "malidp_hw.h"
>  
> -static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc,
> -				   const struct drm_display_mode *mode,
> -				   struct drm_display_mode *adjusted_mode)
> +static enum drm_mode_status malidp_crtc_mode_valid(struct drm_crtc *crtc,
> +						   const struct drm_display_mode *mode)
>  {
>  	struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
>  	struct malidp_hw_device *hwdev = malidp->dev;
> @@ -40,11 +39,11 @@ static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc,
>  		if (rate != req_rate) {
>  			DRM_DEBUG_DRIVER("pxlclk doesn't support %ld Hz\n",
>  					 req_rate);
> -			return false;
> +			return MODE_NOCLOCK;
>  		}
>  	}
>  
> -	return true;
> +	return MODE_OK;
>  }
>  
>  static void malidp_crtc_enable(struct drm_crtc *crtc)
> @@ -408,7 +407,7 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc,
>  }
>  
>  static const struct drm_crtc_helper_funcs malidp_crtc_helper_funcs = {
> -	.mode_fixup = malidp_crtc_mode_fixup,
> +	.mode_valid = malidp_crtc_mode_valid,
>  	.enable = malidp_crtc_enable,
>  	.disable = malidp_crtc_disable,
>  	.atomic_check = malidp_crtc_atomic_check,
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH v5 08/10] drm/arm: malidp: Use crtc->mode_valid() callback

[Liviu Dudau]

On Tue, May 30, 2017 at 09:29:44AM +0200, Neil Armstrong wrote:
> On 05/25/2017 04:19 PM, Jose Abreu wrote:
> > Now that we have a callback to check if crtc supports a given mode
> > we can use it in malidp so that we restrict the number of probbed
> > modes to the ones we can actually display.
> > 
> > Also, remove the mode_fixup() callback as this is no longer needed
> > because mode_valid() will be called before.
> > 
> > NOTE: Not even compiled tested

I did compile it, even done some testing, but by no means have I managed
to cover all the cases. Looks OK to me.

> > 
> > Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> > Cc: Carlos Palminha <palminha@synopsys.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

> > Cc: Brian Starkey <brian.starkey@arm.com>
> > Cc: David Airlie <airlied@linux.ie>
> > ---
> >  drivers/gpu/drm/arm/malidp_crtc.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
> > index 9446a67..4bb38a2 100644
> > --- a/drivers/gpu/drm/arm/malidp_crtc.c
> > +++ b/drivers/gpu/drm/arm/malidp_crtc.c
> > @@ -22,9 +22,8 @@
> >  #include "malidp_drv.h"
> >  #include "malidp_hw.h"
> >  
> > -static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc,
> > -				   const struct drm_display_mode *mode,
> > -				   struct drm_display_mode *adjusted_mode)
> > +static enum drm_mode_status malidp_crtc_mode_valid(struct drm_crtc *crtc,
> > +						   const struct drm_display_mode *mode)
> >  {
> >  	struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
> >  	struct malidp_hw_device *hwdev = malidp->dev;
> > @@ -40,11 +39,11 @@ static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc,
> >  		if (rate != req_rate) {
> >  			DRM_DEBUG_DRIVER("pxlclk doesn't support %ld Hz\n",
> >  					 req_rate);
> > -			return false;
> > +			return MODE_NOCLOCK;
> >  		}
> >  	}
> >  
> > -	return true;
> > +	return MODE_OK;
> >  }
> >  
> >  static void malidp_crtc_enable(struct drm_crtc *crtc)
> > @@ -408,7 +407,7 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc,
> >  }
> >  
> >  static const struct drm_crtc_helper_funcs malidp_crtc_helper_funcs = {
> > -	.mode_fixup = malidp_crtc_mode_fixup,
> > +	.mode_valid = malidp_crtc_mode_valid,
> >  	.enable = malidp_crtc_enable,
> >  	.disable = malidp_crtc_disable,
> >  	.atomic_check = malidp_crtc_atomic_check,
> > 
> 
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v5 08/10] drm/arm: malidp: Use crtc->mode_valid() callback

[Daniel Vetter]

On Tue, May 30, 2017 at 10:37:29AM +0100, Liviu Dudau wrote:
> On Tue, May 30, 2017 at 09:29:44AM +0200, Neil Armstrong wrote:
> > On 05/25/2017 04:19 PM, Jose Abreu wrote:
> > > Now that we have a callback to check if crtc supports a given mode
> > > we can use it in malidp so that we restrict the number of probbed
> > > modes to the ones we can actually display.
> > > 
> > > Also, remove the mode_fixup() callback as this is no longer needed
> > > because mode_valid() will be called before.
> > > 
> > > NOTE: Not even compiled tested
> 
> I did compile it, even done some testing, but by no means have I managed
> to cover all the cases. Looks OK to me.
> 
> > > 
> > > Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> > > Cc: Carlos Palminha <palminha@synopsys.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Liviu Dudau <liviu.dudau@arm.com>
> 
> Acked-by: Liviu Dudau <liviu.dudau@arm.com>

What does this mean? Do you expect me to merge this through drm-misc? Or
do you plan to merge it through your arm tree (all the required patches
are in drm-misc-next and will be in Dave's tree soonish)?

/me confused.

Thanks, Daniel

> 
> > > Cc: Brian Starkey <brian.starkey@arm.com>
> > > Cc: David Airlie <airlied@linux.ie>
> > > ---
> > >  drivers/gpu/drm/arm/malidp_crtc.c | 11 +++++------
> > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
> > > index 9446a67..4bb38a2 100644
> > > --- a/drivers/gpu/drm/arm/malidp_crtc.c
> > > +++ b/drivers/gpu/drm/arm/malidp_crtc.c
> > > @@ -22,9 +22,8 @@
> > >  #include "malidp_drv.h"
> > >  #include "malidp_hw.h"
> > >  
> > > -static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc,
> > > -				   const struct drm_display_mode *mode,
> > > -				   struct drm_display_mode *adjusted_mode)
> > > +static enum drm_mode_status malidp_crtc_mode_valid(struct drm_crtc *crtc,
> > > +						   const struct drm_display_mode *mode)
> > >  {
> > >  	struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
> > >  	struct malidp_hw_device *hwdev = malidp->dev;
> > > @@ -40,11 +39,11 @@ static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc,
> > >  		if (rate != req_rate) {
> > >  			DRM_DEBUG_DRIVER("pxlclk doesn't support %ld Hz\n",
> > >  					 req_rate);
> > > -			return false;
> > > +			return MODE_NOCLOCK;
> > >  		}
> > >  	}
> > >  
> > > -	return true;
> > > +	return MODE_OK;
> > >  }
> > >  
> > >  static void malidp_crtc_enable(struct drm_crtc *crtc)
> > > @@ -408,7 +407,7 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc,
> > >  }
> > >  
> > >  static const struct drm_crtc_helper_funcs malidp_crtc_helper_funcs = {
> > > -	.mode_fixup = malidp_crtc_mode_fixup,
> > > +	.mode_valid = malidp_crtc_mode_valid,
> > >  	.enable = malidp_crtc_enable,
> > >  	.disable = malidp_crtc_disable,
> > >  	.atomic_check = malidp_crtc_atomic_check,
> > > 
> > 
> > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

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

Re: [PATCH v5 08/10] drm/arm: malidp: Use crtc->mode_valid() callback

[Liviu Dudau]

On Wed, May 31, 2017 at 10:20:04AM +0200, Daniel Vetter wrote:
> On Tue, May 30, 2017 at 10:37:29AM +0100, Liviu Dudau wrote:
> > On Tue, May 30, 2017 at 09:29:44AM +0200, Neil Armstrong wrote:
> > > On 05/25/2017 04:19 PM, Jose Abreu wrote:
> > > > Now that we have a callback to check if crtc supports a given mode
> > > > we can use it in malidp so that we restrict the number of probbed
> > > > modes to the ones we can actually display.
> > > > 
> > > > Also, remove the mode_fixup() callback as this is no longer needed
> > > > because mode_valid() will be called before.
> > > > 
> > > > NOTE: Not even compiled tested
> > 
> > I did compile it, even done some testing, but by no means have I managed
> > to cover all the cases. Looks OK to me.
> > 
> > > > 
> > > > Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> > > > Cc: Carlos Palminha <palminha@synopsys.com>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > 
> > Acked-by: Liviu Dudau <liviu.dudau@arm.com>
> 
> What does this mean? Do you expect me to merge this through drm-misc? Or
> do you plan to merge it through your arm tree (all the required patches
> are in drm-misc-next and will be in Dave's tree soonish)?
> 
> /me confused.

/me too. :) I've only got Cc-ed on one patch, so I'm guessing the whole series is
going to be picked up through drm-misc. For patches that are part of a larger
series (to me) it makes sense to push them through a single channel. But I'm not
the author of the series so I don't know what Jose prefers. If Jose wants this
patch to go through mali-dp tree then I'm happy to pull it, otherwise I can sort out
the conflict(s) before sending a pull request to Dave.

On the larger topic, I'm guessing this is not the first time a series touches multiple
drivers that are not together in a single tree. How was this sorted in the past? Is
there a better way?

Best regards,
Liviu

> 
> Thanks, Daniel
> 
> > 
> > > > Cc: Brian Starkey <brian.starkey@arm.com>
> > > > Cc: David Airlie <airlied@linux.ie>
> > > > ---
> > > >  drivers/gpu/drm/arm/malidp_crtc.c | 11 +++++------
> > > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
> > > > index 9446a67..4bb38a2 100644
> > > > --- a/drivers/gpu/drm/arm/malidp_crtc.c
> > > > +++ b/drivers/gpu/drm/arm/malidp_crtc.c
> > > > @@ -22,9 +22,8 @@
> > > >  #include "malidp_drv.h"
> > > >  #include "malidp_hw.h"
> > > >  
> > > > -static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc,
> > > > -				   const struct drm_display_mode *mode,
> > > > -				   struct drm_display_mode *adjusted_mode)
> > > > +static enum drm_mode_status malidp_crtc_mode_valid(struct drm_crtc *crtc,
> > > > +						   const struct drm_display_mode *mode)
> > > >  {
> > > >  	struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
> > > >  	struct malidp_hw_device *hwdev = malidp->dev;
> > > > @@ -40,11 +39,11 @@ static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc,
> > > >  		if (rate != req_rate) {
> > > >  			DRM_DEBUG_DRIVER("pxlclk doesn't support %ld Hz\n",
> > > >  					 req_rate);
> > > > -			return false;
> > > > +			return MODE_NOCLOCK;
> > > >  		}
> > > >  	}
> > > >  
> > > > -	return true;
> > > > +	return MODE_OK;
> > > >  }
> > > >  
> > > >  static void malidp_crtc_enable(struct drm_crtc *crtc)
> > > > @@ -408,7 +407,7 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc,
> > > >  }
> > > >  
> > > >  static const struct drm_crtc_helper_funcs malidp_crtc_helper_funcs = {
> > > > -	.mode_fixup = malidp_crtc_mode_fixup,
> > > > +	.mode_valid = malidp_crtc_mode_valid,
> > > >  	.enable = malidp_crtc_enable,
> > > >  	.disable = malidp_crtc_disable,
> > > >  	.atomic_check = malidp_crtc_atomic_check,
> > > > 
> > > 
> > > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
> > 
> > -- 
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v5 08/10] drm/arm: malidp: Use crtc->mode_valid() callback

[Daniel Vetter]

On Wed, May 31, 2017 at 12:48 PM, Liviu Dudau <liviu.dudau@arm.com> wrote:
> On Wed, May 31, 2017 at 10:20:04AM +0200, Daniel Vetter wrote:
>> On Tue, May 30, 2017 at 10:37:29AM +0100, Liviu Dudau wrote:
>> > On Tue, May 30, 2017 at 09:29:44AM +0200, Neil Armstrong wrote:
>> > > On 05/25/2017 04:19 PM, Jose Abreu wrote:
>> > > > Now that we have a callback to check if crtc supports a given mode
>> > > > we can use it in malidp so that we restrict the number of probbed
>> > > > modes to the ones we can actually display.
>> > > >
>> > > > Also, remove the mode_fixup() callback as this is no longer needed
>> > > > because mode_valid() will be called before.
>> > > >
>> > > > NOTE: Not even compiled tested
>> >
>> > I did compile it, even done some testing, but by no means have I managed
>> > to cover all the cases. Looks OK to me.
>> >
>> > > >
>> > > > Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> > > > Cc: Carlos Palminha <palminha@synopsys.com>
>> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > > > Cc: Liviu Dudau <liviu.dudau@arm.com>
>> >
>> > Acked-by: Liviu Dudau <liviu.dudau@arm.com>
>>
>> What does this mean? Do you expect me to merge this through drm-misc? Or
>> do you plan to merge it through your arm tree (all the required patches
>> are in drm-misc-next and will be in Dave's tree soonish)?
>>
>> /me confused.
>
> /me too. :) I've only got Cc-ed on one patch, so I'm guessing the whole series is
> going to be picked up through drm-misc. For patches that are part of a larger
> series (to me) it makes sense to push them through a single channel. But I'm not
> the author of the series so I don't know what Jose prefers. If Jose wants this
> patch to go through mali-dp tree then I'm happy to pull it, otherwise I can sort out
> the conflict(s) before sending a pull request to Dave.
>
> On the larger topic, I'm guessing this is not the first time a series touches multiple
> drivers that are not together in a single tree. How was this sorted in the past? Is
> there a better way?

I change my preferred merge strategy depending upon how invasive the
patch is. Since this one here is more complex than a simple refactor,
I prefer it goes in through the right trees. And the required patches
are already in drm-misc-next now, so this should be doable.

For simpler stuff it's often easier to just get it landed through
drm-misc, especially if it's just a dumb patch to e.g. add a new
argument to a function and fill out the default one everywhere. For
those I think it's not even required to get an ack from driver
maintainers, just solid review of the idea&implementation in general.

A bit a grey thing in-between is refactorings that are simple, but
require and audit on each driver, and then a final patch at the end to
remove the old helper functions. My drm_vblank_cleanup removal is such
a case. There I prefer driver maintainers to pick things up
themselves, and 1 kernel release afterwards I'll put the leftover
driver patches + the final cleanup into drm-misc.

Anyway, long story short: Your choice here. I just need to know
whether you'll pick it up or want me to merge it through
drm-misc-next. I think in general it'd be good if maintainers don't
just ack patches, but also state what they expect to happen, e.g. when
I ack something I try to make it clear that I expect this to go in
through a different tree than one I maintain. Otherwise I just pick it
up and merge (and say so).

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v5 08/10] drm/arm: malidp: Use crtc->mode_valid() callback

[Liviu Dudau]

On Wed, May 31, 2017 at 12:56:59PM +0200, Daniel Vetter wrote:
> On Wed, May 31, 2017 at 12:48 PM, Liviu Dudau <liviu.dudau@arm.com> wrote:
> > On Wed, May 31, 2017 at 10:20:04AM +0200, Daniel Vetter wrote:
> >> On Tue, May 30, 2017 at 10:37:29AM +0100, Liviu Dudau wrote:
> >> > On Tue, May 30, 2017 at 09:29:44AM +0200, Neil Armstrong wrote:
> >> > > On 05/25/2017 04:19 PM, Jose Abreu wrote:
> >> > > > Now that we have a callback to check if crtc supports a given mode
> >> > > > we can use it in malidp so that we restrict the number of probbed
> >> > > > modes to the ones we can actually display.
> >> > > >
> >> > > > Also, remove the mode_fixup() callback as this is no longer needed
> >> > > > because mode_valid() will be called before.
> >> > > >
> >> > > > NOTE: Not even compiled tested
> >> >
> >> > I did compile it, even done some testing, but by no means have I managed
> >> > to cover all the cases. Looks OK to me.
> >> >
> >> > > >
> >> > > > Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> >> > > > Cc: Carlos Palminha <palminha@synopsys.com>
> >> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> > > > Cc: Liviu Dudau <liviu.dudau@arm.com>
> >> >
> >> > Acked-by: Liviu Dudau <liviu.dudau@arm.com>
> >>
> >> What does this mean? Do you expect me to merge this through drm-misc? Or
> >> do you plan to merge it through your arm tree (all the required patches
> >> are in drm-misc-next and will be in Dave's tree soonish)?
> >>
> >> /me confused.
> >
> > /me too. :) I've only got Cc-ed on one patch, so I'm guessing the whole series is
> > going to be picked up through drm-misc. For patches that are part of a larger
> > series (to me) it makes sense to push them through a single channel. But I'm not
> > the author of the series so I don't know what Jose prefers. If Jose wants this
> > patch to go through mali-dp tree then I'm happy to pull it, otherwise I can sort out
> > the conflict(s) before sending a pull request to Dave.
> >
> > On the larger topic, I'm guessing this is not the first time a series touches multiple
> > drivers that are not together in a single tree. How was this sorted in the past? Is
> > there a better way?
> 
> I change my preferred merge strategy depending upon how invasive the
> patch is. Since this one here is more complex than a simple refactor,
> I prefer it goes in through the right trees. And the required patches
> are already in drm-misc-next now, so this should be doable.
> 
> For simpler stuff it's often easier to just get it landed through
> drm-misc, especially if it's just a dumb patch to e.g. add a new
> argument to a function and fill out the default one everywhere. For
> those I think it's not even required to get an ack from driver
> maintainers, just solid review of the idea&implementation in general.
> 
> A bit a grey thing in-between is refactorings that are simple, but
> require and audit on each driver, and then a final patch at the end to
> remove the old helper functions. My drm_vblank_cleanup removal is such
> a case. There I prefer driver maintainers to pick things up
> themselves, and 1 kernel release afterwards I'll put the leftover
> driver patches + the final cleanup into drm-misc.
> 
> Anyway, long story short: Your choice here. I just need to know
> whether you'll pick it up or want me to merge it through
> drm-misc-next. I think in general it'd be good if maintainers don't
> just ack patches, but also state what they expect to happen, e.g. when
> I ack something I try to make it clear that I expect this to go in
> through a different tree than one I maintain. Otherwise I just pick it
> up and merge (and say so).

OK, if Jose doesn't like a different approach then I'll pick up this patch.
Then I guess I'll keep an eye on when airlied's git tree and see when drm-misc-next
gets merged before sending my pull request.

And sorry for not stating my follow up action with the Ack, like I've said, I
thought the whole series will be picked up by you based on this reply:

https://lists.freedesktop.org/archives/dri-devel/2017-May/142377.html

Best regards,
Liviu

> 
> Thanks, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

[PATCH v5 09/10] drm/atmel-hlcdc: 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 atmel-hlcdc so that we restrict the number of probbed
modes to the ones we can actually display.

Also, remove the mode_fixup() callback as this is no longer needed
because mode_valid() will be called before.

NOTE: Not even compiled tested

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: David Airlie <airlied@linux.ie>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index 53bfa56..bdfe74e 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -140,13 +140,12 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
 			   cfg);
 }
 
-static bool atmel_hlcdc_crtc_mode_fixup(struct drm_crtc *c,
-					const struct drm_display_mode *mode,
-					struct drm_display_mode *adjusted_mode)
+static enum drm_mode_status atmel_hlcdc_crtc_mode_valid(struct drm_crtc *c,
+							const struct drm_display_mode *mode)
 {
 	struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
 
-	return atmel_hlcdc_dc_mode_valid(crtc->dc, adjusted_mode) == MODE_OK;
+	return atmel_hlcdc_dc_mode_valid(crtc->dc, mode);
 }
 
 static void atmel_hlcdc_crtc_disable(struct drm_crtc *c)
@@ -315,7 +314,7 @@ static void atmel_hlcdc_crtc_atomic_flush(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs lcdc_crtc_helper_funcs = {
-	.mode_fixup = atmel_hlcdc_crtc_mode_fixup,
+	.mode_valid = atmel_hlcdc_crtc_mode_valid,
 	.mode_set = drm_helper_crtc_mode_set,
 	.mode_set_nofb = atmel_hlcdc_crtc_mode_set_nofb,
 	.mode_set_base = drm_helper_crtc_mode_set_base,
-- 
1.9.1

Re: [PATCH v5 09/10] drm/atmel-hlcdc: Use crtc->mode_valid() callback

[Neil Armstrong]

On 05/25/2017 04:19 PM, Jose Abreu wrote:
> Now that we have a callback to check if crtc supports a given mode
> we can use it in atmel-hlcdc so that we restrict the number of probbed
> modes to the ones we can actually display.
> 
> Also, remove the mode_fixup() callback as this is no longer needed
> because mode_valid() will be called before.
> 
> NOTE: Not even compiled tested
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: David Airlie <airlied@linux.ie>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index 53bfa56..bdfe74e 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -140,13 +140,12 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>  			   cfg);
>  }
>  
> -static bool atmel_hlcdc_crtc_mode_fixup(struct drm_crtc *c,
> -					const struct drm_display_mode *mode,
> -					struct drm_display_mode *adjusted_mode)
> +static enum drm_mode_status atmel_hlcdc_crtc_mode_valid(struct drm_crtc *c,
> +							const struct drm_display_mode *mode)
>  {
>  	struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
>  
> -	return atmel_hlcdc_dc_mode_valid(crtc->dc, adjusted_mode) == MODE_OK;
> +	return atmel_hlcdc_dc_mode_valid(crtc->dc, mode);
>  }
>  
>  static void atmel_hlcdc_crtc_disable(struct drm_crtc *c)
> @@ -315,7 +314,7 @@ static void atmel_hlcdc_crtc_atomic_flush(struct drm_crtc *crtc,
>  }
>  
>  static const struct drm_crtc_helper_funcs lcdc_crtc_helper_funcs = {
> -	.mode_fixup = atmel_hlcdc_crtc_mode_fixup,
> +	.mode_valid = atmel_hlcdc_crtc_mode_valid,
>  	.mode_set = drm_helper_crtc_mode_set,
>  	.mode_set_nofb = atmel_hlcdc_crtc_mode_set_nofb,
>  	.mode_set_base = drm_helper_crtc_mode_set_base,
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH v5 09/10] drm/atmel-hlcdc: Use crtc->mode_valid() callback

[Boris Brezillon]

On Thu, 25 May 2017 15:19:21 +0100
Jose Abreu <Jose.Abreu@synopsys.com> wrote:

> Now that we have a callback to check if crtc supports a given mode
> we can use it in atmel-hlcdc so that we restrict the number of probbed
> modes to the ones we can actually display.
> 
> Also, remove the mode_fixup() callback as this is no longer needed
> because mode_valid() will be called before.
> 
> NOTE: Not even compiled tested

Applied to drm-misc-next after fixing a compilation warning and
reformating to checkpatch happy.

Thanks,

Boris

> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: David Airlie <airlied@linux.ie>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index 53bfa56..bdfe74e 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -140,13 +140,12 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>  			   cfg);
>  }
>  
> -static bool atmel_hlcdc_crtc_mode_fixup(struct drm_crtc *c,
> -					const struct drm_display_mode *mode,
> -					struct drm_display_mode *adjusted_mode)
> +static enum drm_mode_status atmel_hlcdc_crtc_mode_valid(struct drm_crtc *c,
> +							const struct drm_display_mode *mode)
>  {
>  	struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
>  
> -	return atmel_hlcdc_dc_mode_valid(crtc->dc, adjusted_mode) == MODE_OK;
> +	return atmel_hlcdc_dc_mode_valid(crtc->dc, mode);
>  }
>  
>  static void atmel_hlcdc_crtc_disable(struct drm_crtc *c)
> @@ -315,7 +314,7 @@ static void atmel_hlcdc_crtc_atomic_flush(struct drm_crtc *crtc,
>  }
>  
>  static const struct drm_crtc_helper_funcs lcdc_crtc_helper_funcs = {
> -	.mode_fixup = atmel_hlcdc_crtc_mode_fixup,
> +	.mode_valid = atmel_hlcdc_crtc_mode_valid,
>  	.mode_set = drm_helper_crtc_mode_set,
>  	.mode_set_nofb = atmel_hlcdc_crtc_mode_set_nofb,
>  	.mode_set_base = drm_helper_crtc_mode_set_base,

[PATCH v5 10/10] drm: vc4: Use crtc->mode_valid() and encoder->mode_valid() callbacks

[Jose Abreu]

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

Also, remove the mode_fixup() calls as these are no longer needed
because mode_valid() will be called before.

NOTE: Not even compiled tested

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Eric Anholt <eric@anholt.net>
Cc: David Airlie <airlied@linux.ie>
---
 drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++-------
 drivers/gpu/drm/vc4/vc4_dpi.c  | 13 ++++++-------
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 1b4dbe9..20703c8 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -542,18 +542,17 @@ static void vc4_crtc_enable(struct drm_crtc *crtc)
 	drm_crtc_vblank_on(crtc);
 }
 
-static bool vc4_crtc_mode_fixup(struct drm_crtc *crtc,
-				const struct drm_display_mode *mode,
-				struct drm_display_mode *adjusted_mode)
+static enum drm_mode_status vc4_crtc_mode_valid(struct drm_crtc *crtc,
+						const struct drm_display_mode *mode)
 {
 	/* Do not allow doublescan modes from user space */
-	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN) {
+	if (mode->flags & DRM_MODE_FLAG_DBLSCAN) {
 		DRM_DEBUG_KMS("[CRTC:%d] Doublescan mode rejected.\n",
 			      crtc->base.id);
-		return false;
+		return MODE_NO_DBLESCAN;
 	}
 
-	return true;
+	return MODE_OK;
 }
 
 static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
@@ -863,7 +862,7 @@ static void vc4_crtc_destroy_state(struct drm_crtc *crtc,
 	.mode_set_nofb = vc4_crtc_mode_set_nofb,
 	.disable = vc4_crtc_disable,
 	.enable = vc4_crtc_enable,
-	.mode_fixup = vc4_crtc_mode_fixup,
+	.mode_valid = vc4_crtc_mode_valid,
 	.atomic_check = vc4_crtc_atomic_check,
 	.atomic_flush = vc4_crtc_atomic_flush,
 };
diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c
index c6d7039..61958ab 100644
--- a/drivers/gpu/drm/vc4/vc4_dpi.c
+++ b/drivers/gpu/drm/vc4/vc4_dpi.c
@@ -330,20 +330,19 @@ static void vc4_dpi_encoder_enable(struct drm_encoder *encoder)
 	}
 }
 
-static bool vc4_dpi_encoder_mode_fixup(struct drm_encoder *encoder,
-				       const struct drm_display_mode *mode,
-				       struct drm_display_mode *adjusted_mode)
+static enum drm_mode_status vc4_dpi_encoder_mode_valid(struct drm_encoder *encoder,
+						       const struct drm_display_mode *mode)
 {
-	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
-		return false;
+	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+		return MODE_NO_INTERLACE;
 
-	return true;
+	return MODE_OK;
 }
 
 static const struct drm_encoder_helper_funcs vc4_dpi_encoder_helper_funcs = {
 	.disable = vc4_dpi_encoder_disable,
 	.enable = vc4_dpi_encoder_enable,
-	.mode_fixup = vc4_dpi_encoder_mode_fixup,
+	.mode_valid = vc4_dpi_encoder_mode_valid,
 };
 
 static const struct of_device_id vc4_dpi_dt_match[] = {
-- 
1.9.1

Re: [PATCH v5 10/10] drm: vc4: Use crtc->mode_valid() and encoder->mode_valid() callbacks

[Neil Armstrong]

On 05/25/2017 04:19 PM, Jose Abreu wrote:
> Now that we have a callback to check if crtc and encoder supports a
> given mode we can use it in vc4 so that we restrict the number of
> probbed modes to the ones we can actually display.
> 
> Also, remove the mode_fixup() calls as these are no longer needed
> because mode_valid() will be called before.
> 
> NOTE: Not even compiled tested
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: David Airlie <airlied@linux.ie>
> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 13 ++++++-------
>  drivers/gpu/drm/vc4/vc4_dpi.c  | 13 ++++++-------
>  2 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 1b4dbe9..20703c8 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -542,18 +542,17 @@ static void vc4_crtc_enable(struct drm_crtc *crtc)
>  	drm_crtc_vblank_on(crtc);
>  }
>  
> -static bool vc4_crtc_mode_fixup(struct drm_crtc *crtc,
> -				const struct drm_display_mode *mode,
> -				struct drm_display_mode *adjusted_mode)
> +static enum drm_mode_status vc4_crtc_mode_valid(struct drm_crtc *crtc,
> +						const struct drm_display_mode *mode)
>  {
>  	/* Do not allow doublescan modes from user space */
> -	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN) {
> +	if (mode->flags & DRM_MODE_FLAG_DBLSCAN) {
>  		DRM_DEBUG_KMS("[CRTC:%d] Doublescan mode rejected.\n",
>  			      crtc->base.id);
> -		return false;
> +		return MODE_NO_DBLESCAN;
>  	}
>  
> -	return true;
> +	return MODE_OK;
>  }
>  
>  static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
> @@ -863,7 +862,7 @@ static void vc4_crtc_destroy_state(struct drm_crtc *crtc,
>  	.mode_set_nofb = vc4_crtc_mode_set_nofb,
>  	.disable = vc4_crtc_disable,
>  	.enable = vc4_crtc_enable,
> -	.mode_fixup = vc4_crtc_mode_fixup,
> +	.mode_valid = vc4_crtc_mode_valid,
>  	.atomic_check = vc4_crtc_atomic_check,
>  	.atomic_flush = vc4_crtc_atomic_flush,
>  };
> diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c
> index c6d7039..61958ab 100644
> --- a/drivers/gpu/drm/vc4/vc4_dpi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dpi.c
> @@ -330,20 +330,19 @@ static void vc4_dpi_encoder_enable(struct drm_encoder *encoder)
>  	}
>  }
>  
> -static bool vc4_dpi_encoder_mode_fixup(struct drm_encoder *encoder,
> -				       const struct drm_display_mode *mode,
> -				       struct drm_display_mode *adjusted_mode)
> +static enum drm_mode_status vc4_dpi_encoder_mode_valid(struct drm_encoder *encoder,
> +						       const struct drm_display_mode *mode)
>  {
> -	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
> -		return false;
> +	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> +		return MODE_NO_INTERLACE;
>  
> -	return true;
> +	return MODE_OK;
>  }
>  
>  static const struct drm_encoder_helper_funcs vc4_dpi_encoder_helper_funcs = {
>  	.disable = vc4_dpi_encoder_disable,
>  	.enable = vc4_dpi_encoder_enable,
> -	.mode_fixup = vc4_dpi_encoder_mode_fixup,
> +	.mode_valid = vc4_dpi_encoder_mode_valid,
>  };
>  
>  static const struct of_device_id vc4_dpi_dt_match[] = {
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH v5 10/10] drm: vc4: Use crtc->mode_valid() and encoder->mode_valid() callbacks

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 462 bytes --]

Jose Abreu <Jose.Abreu@synopsys.com> writes:

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

"probed"

>
> Also, remove the mode_fixup() calls as these are no longer needed
> because mode_valid() will be called before.
>
> NOTE: Not even compiled tested

Compile-tested and Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v5 10/10] drm: vc4: Use crtc->mode_valid() and encoder->mode_valid() callbacks

[Daniel Vetter]

On Fri, Jun 02, 2017 at 01:10:00PM -0700, Eric Anholt wrote:
> Jose Abreu <Jose.Abreu@synopsys.com> writes:
> 
> > Now that we have a callback to check if crtc and encoder supports a
> > given mode we can use it in vc4 so that we restrict the number of
> > probbed modes to the ones we can actually display.
> 
> "probed"
> 
> >
> > Also, remove the mode_fixup() calls as these are no longer needed
> > because mode_valid() will be called before.
> >
> > NOTE: Not even compiled tested
> 
> Compile-tested and Reviewed-by: Eric Anholt <eric@anholt.net>

Pushed to drm-misc-next, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch