[RFC PATCH 0/3] Limit pluggable display modes

[RFC PATCH 0/3] Limit pluggable display modes

[Abhinav Kumar]

As reported on https://gitlab.freedesktop.org/drm/msm/-/issues/17, currently
there is no mechanism to limit the display output to the pluggable displays
and it lets users connect any monitor on any chipset based device.

This can lead to undefined behavior because lets say the display
advertises an unsupported pixel clock as its preferred resolution, then
the end-user can experience undefined behavior such as black screen,
crash or an underrun.

The capabilities of every chipset are advertised in the product
specification for both on-device displays and pluggable displays.

Documents such as [1], [2] and [3] can easily be found on the vendor's
website which advertise the max resolution support for that chipset.

Utilize this information to filter out the resolutions which have
pixel clock more than the supported limits.

This change only addresses pluggable displays because the underlying
assumption is that for the built-in displays, the display being chosen
for the product will be done so after referring to the advertised limits.

For calculating the pixel clock, the value has been taken from the CEA
speficiation if the resolution is a CEA one and from the CVT specification
for non-CEA.

This change has only been compile tested so far to get a general feedback
first and once it takes a final shape, will validate on whatever devices I have
and will appreciate help from others who have devices which I dont.

[1]: https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd7c.pdf
[2]: https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd7c_gen2.pdf
[3]: https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd8cx_gen2.pdf

Abhinav Kumar (3):
  drm/msm/dpu: add max external pixel clock for all targets
  drm/msm: filter out modes for DSI bridge having unsupported clock
  drm/msm: filter out modes for DP/eDP bridge having unsupported clock

 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c |  8 ++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  2 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c        | 18 ++++++++----
 drivers/gpu/drm/msm/dp/dp_display.c            | 16 +++++++++--
 drivers/gpu/drm/msm/dp/dp_parser.h             |  1 -
 drivers/gpu/drm/msm/dsi/dsi.c                  |  5 ++++
 drivers/gpu/drm/msm/dsi/dsi.h                  |  6 ++--
 drivers/gpu/drm/msm/dsi/dsi_host.c             | 40 ++++++++++++++++++++++----
 drivers/gpu/drm/msm/dsi/dsi_manager.c          |  2 +-
 drivers/gpu/drm/msm/msm_drv.h                  |  9 ++++--
 10 files changed, 88 insertions(+), 19 deletions(-)

-- 
2.7.4

[RFC PATCH 1/3] drm/msm/dpu: add max external pixel clock for all targets

[Abhinav Kumar]

Add maximum external pixel clock for all targets based on
the advertised limits for each of them.

The pixel clock has been calculated from the timings mentioned
in the CEA specification for CEA modes and according to
the VESA CVT standard for the others.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 8 ++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 27f029fdc682..b04d219ac380 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -277,6 +277,7 @@ static const struct dpu_caps msm8998_dpu_caps = {
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 	.max_hdeci_exp = MAX_HORZ_DECIMATION,
 	.max_vdeci_exp = MAX_VERT_DECIMATION,
+	.max_ext_pclk = 594000,
 };
 
 static const struct dpu_caps qcm2290_dpu_caps = {
@@ -288,6 +289,7 @@ static const struct dpu_caps qcm2290_dpu_caps = {
 	.has_idle_pc = true,
 	.max_linewidth = 2160,
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
+	.max_ext_pclk = 101250,
 };
 
 static const struct dpu_caps sdm845_dpu_caps = {
@@ -304,6 +306,7 @@ static const struct dpu_caps sdm845_dpu_caps = {
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 	.max_hdeci_exp = MAX_HORZ_DECIMATION,
 	.max_vdeci_exp = MAX_VERT_DECIMATION,
+	.max_ext_pclk = 594000,
 };
 
 static const struct dpu_caps sc7180_dpu_caps = {
@@ -316,6 +319,7 @@ static const struct dpu_caps sc7180_dpu_caps = {
 	.has_idle_pc = true,
 	.max_linewidth = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
+	.max_ext_pclk = 312250,
 };
 
 static const struct dpu_caps sm8150_dpu_caps = {
@@ -332,6 +336,7 @@ static const struct dpu_caps sm8150_dpu_caps = {
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 	.max_hdeci_exp = MAX_HORZ_DECIMATION,
 	.max_vdeci_exp = MAX_VERT_DECIMATION,
+	.max_ext_pclk = 594000,
 };
 
 static const struct dpu_caps sc8180x_dpu_caps = {
@@ -348,6 +353,7 @@ static const struct dpu_caps sc8180x_dpu_caps = {
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 	.max_hdeci_exp = MAX_HORZ_DECIMATION,
 	.max_vdeci_exp = MAX_VERT_DECIMATION,
+	.max_ext_pclk = 594000,
 };
 
 static const struct dpu_caps sm8250_dpu_caps = {
@@ -362,6 +368,7 @@ static const struct dpu_caps sm8250_dpu_caps = {
 	.has_3d_merge = true,
 	.max_linewidth = 4096,
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
+	.max_ext_pclk = 594000,
 };
 
 static const struct dpu_caps sc7280_dpu_caps = {
@@ -374,6 +381,7 @@ static const struct dpu_caps sc7280_dpu_caps = {
 	.has_idle_pc = true,
 	.max_linewidth = 2400,
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
+	.max_ext_pclk = 312250,
 };
 
 static const struct dpu_mdp_cfg msm8998_mdp[] = {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index 38aa38ab1568..35cab76d9530 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -370,6 +370,7 @@ struct dpu_rotation_cfg {
 
 /**
  * struct dpu_caps - define DPU capabilities
+ * @max_ext_pclk       max pixel clock of the external display
  * @max_mixer_width    max layer mixer line width support.
  * @max_mixer_blendstages max layer mixer blend stages or
  *                       supported z order
@@ -386,6 +387,7 @@ struct dpu_rotation_cfg {
  * @max_vdeci_exp      max vertical decimation supported (max is 2^value)
  */
 struct dpu_caps {
+	int max_ext_pclk;
 	u32 max_mixer_width;
 	u32 max_mixer_blendstages;
 	u32 qseed_type;
-- 
2.7.4

[RFC PATCH 2/3] drm/msm: filter out modes for DSI bridge having unsupported clock

[Abhinav Kumar]

DSI interface used with a bridge chip connected to an external
display is subject to the same pixel clock limits as one
which is natively pluggable like DisplayPort.

Hence filter out DSI modes having an unsupported pixel clock
if its connected to a bridge which is pluggable.

Ideally, this can be accommodated into msm_dsi_modeset_init()
by passing an extra parameter but this will also affect non-dpu
targets. Till we add the same logic for non-dpu chipsets, lets
have this as a separate call.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 11 +++++++--
 drivers/gpu/drm/msm/dsi/dsi.c           |  5 +++++
 drivers/gpu/drm/msm/dsi/dsi.h           |  6 +++--
 drivers/gpu/drm/msm/dsi/dsi_host.c      | 40 ++++++++++++++++++++++++++++-----
 drivers/gpu/drm/msm/dsi/dsi_manager.c   |  2 +-
 drivers/gpu/drm/msm/msm_drv.h           |  4 ++++
 6 files changed, 58 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 7a5fabc0fd4f..e6f7e07fd2a6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -539,7 +539,8 @@ static void dpu_kms_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
 
 static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 				    struct msm_drm_private *priv,
-				    struct dpu_kms *dpu_kms)
+				    struct dpu_kms *dpu_kms,
+				    int max_ext_pclk)
 {
 	struct drm_encoder *encoder = NULL;
 	struct msm_display_info info;
@@ -582,6 +583,8 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 			break;
 		}
 
+		msm_dsi_set_max_extpclk(priv->dsi[i], max_ext_pclk);
+
 		info.h_tile_instance[info.num_of_h_tiles++] = i;
 		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
 
@@ -595,6 +598,8 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 				break;
 			}
 
+			msm_dsi_set_max_extpclk(priv->dsi[i], max_ext_pclk);
+
 			info.h_tile_instance[info.num_of_h_tiles++] = other;
 		}
 
@@ -702,7 +707,9 @@ static int _dpu_kms_setup_displays(struct drm_device *dev,
 	int rc = 0;
 	int i;
 
-	rc = _dpu_kms_initialize_dsi(dev, priv, dpu_kms);
+	int max_ext_pclk = dpu_kms->catalog->caps->max_ext_pclk;
+
+	rc = _dpu_kms_initialize_dsi(dev, priv, dpu_kms, max_ext_pclk);
 	if (rc) {
 		DPU_ERROR("initialize_dsi failed, rc = %d\n", rc);
 		return rc;
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 39bbabb5daf6..3a06a157d1b1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -265,6 +265,11 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 	return ret;
 }
 
+void msm_dsi_set_max_extpclk(struct msm_dsi *msm_dsi, int max_ext_pclk)
+{
+	 msm_dsi_host_set_max_extpclk(msm_dsi->host, max_ext_pclk);
+}
+
 void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi)
 {
 	msm_dsi_host_snapshot(disp_state, msm_dsi->host);
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 2a96b4fe7839..1be4ebb0f9c8 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -93,8 +93,9 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
 int msm_dsi_host_power_off(struct mipi_dsi_host *host);
 int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host,
 				  const struct drm_display_mode *mode);
-enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
-					    const struct drm_display_mode *mode);
+enum drm_mode_status msm_dsi_host_mode_valid(struct mipi_dsi_host *host,
+		const struct drm_display_mode *mode,
+		struct drm_bridge *ext_bridge);
 unsigned long msm_dsi_host_get_mode_flags(struct mipi_dsi_host *host);
 int msm_dsi_host_register(struct mipi_dsi_host *host);
 void msm_dsi_host_unregister(struct mipi_dsi_host *host);
@@ -109,6 +110,7 @@ void msm_dsi_host_get_phy_clk_req(struct mipi_dsi_host *host,
 void msm_dsi_host_destroy(struct mipi_dsi_host *host);
 int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
 					struct drm_device *dev);
+void msm_dsi_host_set_max_extpclk(struct mipi_dsi_host *host, int max_ext_pclk);
 int msm_dsi_host_init(struct msm_dsi *msm_dsi);
 int msm_dsi_runtime_suspend(struct device *dev);
 int msm_dsi_runtime_resume(struct device *dev);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 57a4c0fa614b..4428a6a66ee1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -172,6 +172,9 @@ struct msm_dsi_host {
 	int dlane_swap;
 	int num_data_lanes;
 
+	/* max pixel clock when used with a bridge chip */
+	int max_ext_pclk;
+
 	/* from phy DT */
 	bool cphy_mode;
 
@@ -2076,6 +2079,13 @@ int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
 	return 0;
 }
 
+void msm_dsi_host_set_max_extpclk(struct mipi_dsi_host *host, int max_ext_pclk)
+{
+	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
+
+	msm_host->max_ext_pclk = max_ext_pclk;
+}
+
 int msm_dsi_host_register(struct mipi_dsi_host *host)
 {
 	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
@@ -2548,17 +2558,14 @@ int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host,
 	return 0;
 }
 
-enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
-					    const struct drm_display_mode *mode)
+static enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
+		const struct drm_display_mode *mode)
 {
 	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
 	struct drm_dsc_config *dsc = msm_host->dsc;
 	int pic_width = mode->hdisplay;
 	int pic_height = mode->vdisplay;
 
-	if (!msm_host->dsc)
-		return MODE_OK;
-
 	if (pic_width % dsc->slice_width) {
 		pr_err("DSI: pic_width %d has to be multiple of slice %d\n",
 		       pic_width, dsc->slice_width);
@@ -2574,6 +2581,29 @@ enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
 	return MODE_OK;
 }
 
+enum drm_mode_status msm_dsi_host_mode_valid(struct mipi_dsi_host *host,
+					    const struct drm_display_mode *mode,
+					    struct drm_bridge *ext_bridge)
+{
+	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
+
+	/* TODO: external bridge chip with DSI having DSC */
+	if (msm_host->dsc)
+		return msm_dsi_host_check_dsc(host, mode);
+
+	/* TODO: add same logic for non-dpu targets */
+	if (!msm_host->max_ext_pclk)
+		return MODE_OK;
+
+	if (ext_bridge) {
+		if (ext_bridge->ops & DRM_BRIDGE_OP_HPD)
+			if (mode->clock > msm_host->max_ext_pclk)
+				return MODE_CLOCK_HIGH;
+	}
+
+	return MODE_OK;
+}
+
 unsigned long msm_dsi_host_get_mode_flags(struct mipi_dsi_host *host)
 {
 	return to_msm_dsi_host(host)->mode_flags;
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 3a1417397283..1543a0e07d5a 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -451,7 +451,7 @@ static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
 	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
 	struct mipi_dsi_host *host = msm_dsi->host;
 
-	return msm_dsi_host_check_dsc(host, mode);
+	return msm_dsi_host_mode_valid(host, mode, msm_dsi->external_bridge);
 }
 
 static const struct drm_bridge_funcs dsi_mgr_bridge_funcs = {
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index ea80846e7ac3..44d882b04327 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -281,6 +281,7 @@ void __init msm_dsi_register(void);
 void __exit msm_dsi_unregister(void);
 int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 			 struct drm_encoder *encoder);
+void msm_dsi_set_max_extpclk(struct msm_dsi *msm_dsi, int max_ext_pclk);
 void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi);
 bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
@@ -299,6 +300,9 @@ static inline int msm_dsi_modeset_init(struct msm_dsi *msm_dsi,
 {
 	return -EINVAL;
 }
+static inline void msm_dsi_set_max_extpclk(struct msm_dsi *msm_dsi, int max_ext_pclk)
+{
+}
 static inline void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi)
 {
 }
-- 
2.7.4

[RFC PATCH 3/3] drm/msm: filter out modes for DP/eDP bridge having unsupported clock

[Abhinav Kumar]

Filter out DP/eDP modes having an unsupported pixel clock by
replacing the current hard-coded limit with the per chipset advertised
value.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c |  7 ++++---
 drivers/gpu/drm/msm/dp/dp_display.c     | 16 +++++++++++++---
 drivers/gpu/drm/msm/dp/dp_parser.h      |  1 -
 drivers/gpu/drm/msm/msm_drv.h           |  5 +++--
 4 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index e6f7e07fd2a6..7857ce58b615 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -614,7 +614,8 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 
 static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 					    struct msm_drm_private *priv,
-					    struct dpu_kms *dpu_kms)
+					    struct dpu_kms *dpu_kms,
+					    int max_ext_pclk)
 {
 	struct drm_encoder *encoder = NULL;
 	struct msm_display_info info;
@@ -632,7 +633,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 		}
 
 		memset(&info, 0, sizeof(info));
-		rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
+		rc = msm_dp_modeset_init(priv->dp[i], dev, encoder, max_ext_pclk);
 		if (rc) {
 			DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
 			drm_encoder_cleanup(encoder);
@@ -715,7 +716,7 @@ static int _dpu_kms_setup_displays(struct drm_device *dev,
 		return rc;
 	}
 
-	rc = _dpu_kms_initialize_displayport(dev, priv, dpu_kms);
+	rc = _dpu_kms_initialize_displayport(dev, priv, dpu_kms, max_ext_pclk);
 	if (rc) {
 		DPU_ERROR("initialize_DP failed, rc = %d\n", rc);
 		return rc;
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index bfd0aeff3f0d..8b91d8adf921 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -117,6 +117,7 @@ struct dp_display_private {
 
 	bool wide_bus_en;
 
+	int max_ext_pclk;
 	struct dp_audio *audio;
 };
 
@@ -986,8 +987,15 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
 	if (dp->is_edp)
 		return MODE_OK;
 
-	if (mode->clock > DP_MAX_PIXEL_CLK_KHZ)
-		return MODE_CLOCK_HIGH;
+	/*
+	 * If DP/eDP supports HPD natively or through a bridge, need to make
+	 * sure that we filter out the modes with pixel clock higher than the
+	 * chipset capabilities
+	 */
+	if ((bridge->ops & DRM_BRIDGE_OP_HPD) ||
+			(dp->next_bridge && (dp->next_bridge->ops & DRM_BRIDGE_OP_HPD)))
+		if (mode->clock > dp_display->max_ext_pclk)
+			return MODE_CLOCK_HIGH;
 
 	dp_display = container_of(dp, struct dp_display_private, dp_display);
 	link_info = &dp_display->panel->link_info;
@@ -1587,7 +1595,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
 }
 
 int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
-			struct drm_encoder *encoder)
+			struct drm_encoder *encoder, int max_ext_pclk)
 {
 	struct msm_drm_private *priv;
 	struct dp_display_private *dp_priv;
@@ -1599,6 +1607,8 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 	priv = dev->dev_private;
 	dp_display->drm_dev = dev;
 
+	dp_priv->max_ext_pclk = max_ext_pclk;
+
 	dp_priv = container_of(dp_display, struct dp_display_private, dp_display);
 
 	ret = dp_display_request_irq(dp_display);
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
index 866c1a82bf1a..c94b793027a2 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -13,7 +13,6 @@
 #include "msm_drv.h"
 
 #define DP_LABEL "MDSS DP DISPLAY"
-#define DP_MAX_PIXEL_CLK_KHZ	675000
 #define DP_MAX_NUM_DP_LANES	4
 
 enum dp_pm_type {
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 44d882b04327..39e8cdde6152 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -329,7 +329,7 @@ static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_
 int __init msm_dp_register(void);
 void __exit msm_dp_unregister(void);
 int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
-			 struct drm_encoder *encoder);
+			 struct drm_encoder *encoder, int max_ext_pclk);
 void msm_dp_irq_postinstall(struct msm_dp *dp_display);
 void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);
 
@@ -346,7 +346,8 @@ static inline void __exit msm_dp_unregister(void)
 }
 static inline int msm_dp_modeset_init(struct msm_dp *dp_display,
 				       struct drm_device *dev,
-				       struct drm_encoder *encoder)
+				       struct drm_encoder *encoder,
+				       int max_ext_pclk)
 {
 	return -EINVAL;
 }
-- 
2.7.4

Re: [RFC PATCH 2/3] drm/msm: filter out modes for DSI bridge having unsupported clock

[Stephen Boyd]

Quoting Abhinav Kumar (2022-08-29 20:33:08)
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
> index 39bbabb5daf6..3a06a157d1b1 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> @@ -265,6 +265,11 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
>         return ret;
>  }
>
> +void msm_dsi_set_max_extpclk(struct msm_dsi *msm_dsi, int max_ext_pclk)

Do we really need a 'setter' API for something like this? Why can't we
directly access the constant value for the max clk in the function that
uses it to limit modes?

> +{
> +        msm_dsi_host_set_max_extpclk(msm_dsi->host, max_ext_pclk);
> +}
> +
>  void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi)
>  {
>         msm_dsi_host_snapshot(disp_state, msm_dsi->host);
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index 2a96b4fe7839..1be4ebb0f9c8 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -93,8 +93,9 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
>  int msm_dsi_host_power_off(struct mipi_dsi_host *host);
>  int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host,
>                                   const struct drm_display_mode *mode);
> -enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
> -                                           const struct drm_display_mode *mode);
> +enum drm_mode_status msm_dsi_host_mode_valid(struct mipi_dsi_host *host,
> +               const struct drm_display_mode *mode,
> +               struct drm_bridge *ext_bridge);
>  unsigned long msm_dsi_host_get_mode_flags(struct mipi_dsi_host *host);
>  int msm_dsi_host_register(struct mipi_dsi_host *host);
>  void msm_dsi_host_unregister(struct mipi_dsi_host *host);
> @@ -109,6 +110,7 @@ void msm_dsi_host_get_phy_clk_req(struct mipi_dsi_host *host,
>  void msm_dsi_host_destroy(struct mipi_dsi_host *host);
>  int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
>                                         struct drm_device *dev);
> +void msm_dsi_host_set_max_extpclk(struct mipi_dsi_host *host, int max_ext_pclk);
>  int msm_dsi_host_init(struct msm_dsi *msm_dsi);
>  int msm_dsi_runtime_suspend(struct device *dev);
>  int msm_dsi_runtime_resume(struct device *dev);
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 57a4c0fa614b..4428a6a66ee1 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -172,6 +172,9 @@ struct msm_dsi_host {
>         int dlane_swap;
>         int num_data_lanes;
>
> +       /* max pixel clock when used with a bridge chip */
> +       int max_ext_pclk;

Will pixel clock be negative? What units is this in? Hz?

> +
>         /* from phy DT */
>         bool cphy_mode;
>
> @@ -2076,6 +2079,13 @@ int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
>         return 0;
>  }
>
> +void msm_dsi_host_set_max_extpclk(struct mipi_dsi_host *host, int max_ext_pclk)
> +{
> +       struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
> +
> +       msm_host->max_ext_pclk = max_ext_pclk;
> +}
> +
>  int msm_dsi_host_register(struct mipi_dsi_host *host)
>  {
>         struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
> @@ -2548,17 +2558,14 @@ int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host,
>         return 0;
>  }
>
> -enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
> -                                           const struct drm_display_mode *mode)
> +static enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
> +               const struct drm_display_mode *mode)
>  {
>         struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>         struct drm_dsc_config *dsc = msm_host->dsc;
>         int pic_width = mode->hdisplay;
>         int pic_height = mode->vdisplay;
>
> -       if (!msm_host->dsc)
> -               return MODE_OK;
> -
>         if (pic_width % dsc->slice_width) {
>                 pr_err("DSI: pic_width %d has to be multiple of slice %d\n",
>                        pic_width, dsc->slice_width);
> @@ -2574,6 +2581,29 @@ enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
>         return MODE_OK;
>  }
>
> +enum drm_mode_status msm_dsi_host_mode_valid(struct mipi_dsi_host *host,
> +                                           const struct drm_display_mode *mode,
> +                                           struct drm_bridge *ext_bridge)
> +{
> +       struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
> +
> +       /* TODO: external bridge chip with DSI having DSC */
> +       if (msm_host->dsc)
> +               return msm_dsi_host_check_dsc(host, mode);
> +
> +       /* TODO: add same logic for non-dpu targets */
> +       if (!msm_host->max_ext_pclk)
> +               return MODE_OK;
> +
> +       if (ext_bridge) {
> +               if (ext_bridge->ops & DRM_BRIDGE_OP_HPD)

Nitpick: Collapse conditions

	if (ext_bridge && (ext_bridge->ops & DRM_BRIDGE_OP_HPD))

Also, what does HPD have to do with this?

> +                       if (mode->clock > msm_host->max_ext_pclk)
> +                               return MODE_CLOCK_HIGH;
> +       }
> +
> +       return MODE_OK;
> +}
> +
>  unsigned long msm_dsi_host_get_mode_flags(struct mipi_dsi_host *host)
>  {
>         return to_msm_dsi_host(host)->mode_flags;

Re: [RFC PATCH 3/3] drm/msm: filter out modes for DP/eDP bridge having unsupported clock

[Stephen Boyd]

Quoting Abhinav Kumar (2022-08-29 20:33:09)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index bfd0aeff3f0d..8b91d8adf921 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -117,6 +117,7 @@ struct dp_display_private {
>
>         bool wide_bus_en;
>
> +       int max_ext_pclk;

Same signed comment as before.

>         struct dp_audio *audio;
>  };
>
> @@ -986,8 +987,15 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
>         if (dp->is_edp)
>                 return MODE_OK;
>
> -       if (mode->clock > DP_MAX_PIXEL_CLK_KHZ)
> -               return MODE_CLOCK_HIGH;
> +       /*
> +        * If DP/eDP supports HPD natively or through a bridge, need to make
> +        * sure that we filter out the modes with pixel clock higher than the
> +        * chipset capabilities
> +        */
> +       if ((bridge->ops & DRM_BRIDGE_OP_HPD) ||
> +                       (dp->next_bridge && (dp->next_bridge->ops & DRM_BRIDGE_OP_HPD)))

Doesn't drm_bridge_chain_mode_valid() already run the mode_valid bridge
function for all bridges in the chain? I don't understand why we need to
peek at the bridge or the next bridge and only filter in that case. Why
can't we always filter modes here? Do we want to allow higher pixel clks
than the chip supports?

> +               if (mode->clock > dp_display->max_ext_pclk)
> +                       return MODE_CLOCK_HIGH;
>

Re: [RFC PATCH 2/3] drm/msm: filter out modes for DSI bridge having unsupported clock

[Dmitry Baryshkov]

On 30/08/2022 06:33, Abhinav Kumar wrote:
> DSI interface used with a bridge chip connected to an external
> display is subject to the same pixel clock limits as one
> which is natively pluggable like DisplayPort.
> 
> Hence filter out DSI modes having an unsupported pixel clock
> if its connected to a bridge which is pluggable.
> 
> Ideally, this can be accommodated into msm_dsi_modeset_init()
> by passing an extra parameter but this will also affect non-dpu
> targets. Till we add the same logic for non-dpu chipsets, lets
> have this as a separate call.

I think this makes DP/DSI depend too much on the DPU and DPU internals. 
Can we instead use clk_round_rate() in the .mode_valid in DSI/DP/HDMI 
drivers in order to check that the requested rate can be achieved?

-- 
With best wishes
Dmitry

Re: [RFC PATCH 0/3] Limit pluggable display modes

[Dmitry Baryshkov]

On 30/08/2022 06:33, Abhinav Kumar wrote:
> As reported on https://gitlab.freedesktop.org/drm/msm/-/issues/17, currently
> there is no mechanism to limit the display output to the pluggable displays
> and it lets users connect any monitor on any chipset based device.
> 
> This can lead to undefined behavior because lets say the display
> advertises an unsupported pixel clock as its preferred resolution, then
> the end-user can experience undefined behavior such as black screen,
> crash or an underrun.
> 
> The capabilities of every chipset are advertised in the product
> specification for both on-device displays and pluggable displays.

After discussing this privately, it was agreed that there are two kinds 
of issues here:
  - filtering the modes which can not be handled by the DPU/DSI/DP 
hardware pieces themselves

  - filtering the modes which can not be handled by the external 
limitations (e.g. the bridge not being able to drive this mode, the pcb 
routing of data lanes being unable to sustain the rate, the connector 
being the limit, etc).

Krzysztof, I'd like to ask your advice if adding a properly like 
`max-link-frequency' sounds like a good idea? The video-interfaces.yaml 
bindings already has the `link-frequencies' property, but it is an array 
of discrete frequencies supported by the link in the graph. In our case 
the list of frequencies is more or less continuous, with max (and min) 
values. Also, can it be added to the final device in the chain (e.g. 
hdmi/dp/vga connectors) or should it be added to the endpoint graph nodes?

> Documents such as [1], [2] and [3] can easily be found on the vendor's
> website which advertise the max resolution support for that chipset.
> 
> Utilize this information to filter out the resolutions which have
> pixel clock more than the supported limits.
> 
> This change only addresses pluggable displays because the underlying
> assumption is that for the built-in displays, the display being chosen
> for the product will be done so after referring to the advertised limits.
> 
> For calculating the pixel clock, the value has been taken from the CEA
> speficiation if the resolution is a CEA one and from the CVT specification
> for non-CEA.
> 
> This change has only been compile tested so far to get a general feedback
> first and once it takes a final shape, will validate on whatever devices I have
> and will appreciate help from others who have devices which I dont.
> 
> [1]: https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd7c.pdf
> [2]: https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd7c_gen2.pdf
> [3]: https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd8cx_gen2.pdf
> 
> Abhinav Kumar (3):
>    drm/msm/dpu: add max external pixel clock for all targets
>    drm/msm: filter out modes for DSI bridge having unsupported clock
>    drm/msm: filter out modes for DP/eDP bridge having unsupported clock
> 
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c |  8 ++++++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  2 ++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c        | 18 ++++++++----
>   drivers/gpu/drm/msm/dp/dp_display.c            | 16 +++++++++--
>   drivers/gpu/drm/msm/dp/dp_parser.h             |  1 -
>   drivers/gpu/drm/msm/dsi/dsi.c                  |  5 ++++
>   drivers/gpu/drm/msm/dsi/dsi.h                  |  6 ++--
>   drivers/gpu/drm/msm/dsi/dsi_host.c             | 40 ++++++++++++++++++++++----
>   drivers/gpu/drm/msm/dsi/dsi_manager.c          |  2 +-
>   drivers/gpu/drm/msm/msm_drv.h                  |  9 ++++--
>   10 files changed, 88 insertions(+), 19 deletions(-)
> 

-- 
With best wishes
Dmitry

Re: [RFC PATCH 2/3] drm/msm: filter out modes for DSI bridge having unsupported clock

[Abhinav Kumar]

Hi Stephen

On 9/7/2022 6:06 PM, Stephen Boyd wrote:
> Quoting Abhinav Kumar (2022-08-29 20:33:08)
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
>> index 39bbabb5daf6..3a06a157d1b1 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
>> @@ -265,6 +265,11 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
>>          return ret;
>>   }
>>
>> +void msm_dsi_set_max_extpclk(struct msm_dsi *msm_dsi, int max_ext_pclk)
> 
> Do we really need a 'setter' API for something like this? Why can't we
> directly access the constant value for the max clk in the function that
> uses it to limit modes?

That constant value is part of the DPU catalog. the value needs to be 
passed from the DPU to DSI/DP for it to use. Hence the setter API.

In this RFC atleast, this is being modeled as a DPU catalog entry.

> 
>> +{
>> +        msm_dsi_host_set_max_extpclk(msm_dsi->host, max_ext_pclk);
>> +}
>> +
>>   void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi)
>>   {
>>          msm_dsi_host_snapshot(disp_state, msm_dsi->host);
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
>> index 2a96b4fe7839..1be4ebb0f9c8 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>> @@ -93,8 +93,9 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
>>   int msm_dsi_host_power_off(struct mipi_dsi_host *host);
>>   int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host,
>>                                    const struct drm_display_mode *mode);
>> -enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
>> -                                           const struct drm_display_mode *mode);
>> +enum drm_mode_status msm_dsi_host_mode_valid(struct mipi_dsi_host *host,
>> +               const struct drm_display_mode *mode,
>> +               struct drm_bridge *ext_bridge);
>>   unsigned long msm_dsi_host_get_mode_flags(struct mipi_dsi_host *host);
>>   int msm_dsi_host_register(struct mipi_dsi_host *host);
>>   void msm_dsi_host_unregister(struct mipi_dsi_host *host);
>> @@ -109,6 +110,7 @@ void msm_dsi_host_get_phy_clk_req(struct mipi_dsi_host *host,
>>   void msm_dsi_host_destroy(struct mipi_dsi_host *host);
>>   int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
>>                                          struct drm_device *dev);
>> +void msm_dsi_host_set_max_extpclk(struct mipi_dsi_host *host, int max_ext_pclk);
>>   int msm_dsi_host_init(struct msm_dsi *msm_dsi);
>>   int msm_dsi_runtime_suspend(struct device *dev);
>>   int msm_dsi_runtime_resume(struct device *dev);
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 57a4c0fa614b..4428a6a66ee1 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -172,6 +172,9 @@ struct msm_dsi_host {
>>          int dlane_swap;
>>          int num_data_lanes;
>>
>> +       /* max pixel clock when used with a bridge chip */
>> +       int max_ext_pclk;
> 
> Will pixel clock be negative? What units is this in? Hz?

This is in Khz. It cannot be negative. I was also thinking of an 
unsigned int for this but the drm_display_mode's clock is an int so i 
also kept it like this.

227 struct drm_display_mode {
228 	/**
229 	 * @clock:
230 	 *
231 	 * Pixel clock in kHz.
232 	 */
233 	int clock;		/* in kHz */
234 	u16 hdisplay;


> 
>> +
>>          /* from phy DT */
>>          bool cphy_mode;
>>
>> @@ -2076,6 +2079,13 @@ int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
>>          return 0;
>>   }
>>
>> +void msm_dsi_host_set_max_extpclk(struct mipi_dsi_host *host, int max_ext_pclk)
>> +{
>> +       struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>> +
>> +       msm_host->max_ext_pclk = max_ext_pclk;
>> +}
>> +
>>   int msm_dsi_host_register(struct mipi_dsi_host *host)
>>   {
>>          struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>> @@ -2548,17 +2558,14 @@ int msm_dsi_host_set_display_mode(struct mipi_dsi_host *host,
>>          return 0;
>>   }
>>
>> -enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
>> -                                           const struct drm_display_mode *mode)
>> +static enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
>> +               const struct drm_display_mode *mode)
>>   {
>>          struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>>          struct drm_dsc_config *dsc = msm_host->dsc;
>>          int pic_width = mode->hdisplay;
>>          int pic_height = mode->vdisplay;
>>
>> -       if (!msm_host->dsc)
>> -               return MODE_OK;
>> -
>>          if (pic_width % dsc->slice_width) {
>>                  pr_err("DSI: pic_width %d has to be multiple of slice %d\n",
>>                         pic_width, dsc->slice_width);
>> @@ -2574,6 +2581,29 @@ enum drm_mode_status msm_dsi_host_check_dsc(struct mipi_dsi_host *host,
>>          return MODE_OK;
>>   }
>>
>> +enum drm_mode_status msm_dsi_host_mode_valid(struct mipi_dsi_host *host,
>> +                                           const struct drm_display_mode *mode,
>> +                                           struct drm_bridge *ext_bridge)
>> +{
>> +       struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>> +
>> +       /* TODO: external bridge chip with DSI having DSC */
>> +       if (msm_host->dsc)
>> +               return msm_dsi_host_check_dsc(host, mode);
>> +
>> +       /* TODO: add same logic for non-dpu targets */
>> +       if (!msm_host->max_ext_pclk)
>> +               return MODE_OK;
>> +
>> +       if (ext_bridge) {
>> +               if (ext_bridge->ops & DRM_BRIDGE_OP_HPD)
> 
> Nitpick: Collapse conditions
> 
> 	if (ext_bridge && (ext_bridge->ops & DRM_BRIDGE_OP_HPD))

Ack
> 
> Also, what does HPD have to do with this?

The documents referenced in the cover letter define the limits for 
built-in and external displays.

This series is targetted only for external displays with an underlying 
assumption that built-in displays are chosen at design time of the 
product and the product spec should be kept in mind while choosing them.

But for external ( pluggable ) displays, this is not true as the 
consumer can plug-in any monitor.

Now, there is no rule that DSI cannot be used as the external display 
with a DSI to HDMI or DSI to DP bridge chip.

In those cases, we need to check if the ext_bridge has HPD support and 
if so use this filtering of modes.

After discussing with Dmitry, I do agree though that instead of checking 
the next bridge, I should be checking the last bridge in the chain instead.

So when i do push the next version, I should change this to check if the 
last bridge has HPD support.

> 
>> +                       if (mode->clock > msm_host->max_ext_pclk)
>> +                               return MODE_CLOCK_HIGH;
>> +       }
>> +
>> +       return MODE_OK;
>> +}
>> +
>>   unsigned long msm_dsi_host_get_mode_flags(struct mipi_dsi_host *host)
>>   {
>>          return to_msm_dsi_host(host)->mode_flags;

Re: [RFC PATCH 3/3] drm/msm: filter out modes for DP/eDP bridge having unsupported clock

[Abhinav Kumar]

On 9/7/2022 6:12 PM, Stephen Boyd wrote:
> Quoting Abhinav Kumar (2022-08-29 20:33:09)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index bfd0aeff3f0d..8b91d8adf921 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -117,6 +117,7 @@ struct dp_display_private {
>>
>>          bool wide_bus_en;
>>
>> +       int max_ext_pclk;
> 
> Same signed comment as before.

This is in Khz. It cannot be negative. I was also thinking of an 
unsigned int for this but the drm_display_mode's clock is an int so i 
also kept it like this.

227 struct drm_display_mode {
228     /**
229      * @clock:
230      *
231      * Pixel clock in kHz.
232      */
233     int clock;        /* in kHz */
234     u16 hdisplay;
> 
>>          struct dp_audio *audio;
>>   };
>>
>> @@ -986,8 +987,15 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
>>          if (dp->is_edp)
>>                  return MODE_OK;
>>
>> -       if (mode->clock > DP_MAX_PIXEL_CLK_KHZ)
>> -               return MODE_CLOCK_HIGH;
>> +       /*
>> +        * If DP/eDP supports HPD natively or through a bridge, need to make
>> +        * sure that we filter out the modes with pixel clock higher than the
>> +        * chipset capabilities
>> +        */
>> +       if ((bridge->ops & DRM_BRIDGE_OP_HPD) ||
>> +                       (dp->next_bridge && (dp->next_bridge->ops & DRM_BRIDGE_OP_HPD)))
> 
> Doesn't drm_bridge_chain_mode_valid() already run the mode_valid bridge
> function for all bridges in the chain? I don't understand why we need to
> peek at the bridge or the next bridge and only filter in that case. Why
> can't we always filter modes here? Do we want to allow higher pixel clks
> than the chip supports?

The next bridge does not know about the max capability of the previous 
bridge.

If the DP is used as a built-in display, we dont want this filter.

If the DP is used as the external display either directly or with the 
next/last bridge as the pluggable one, then we want to apply this filter.

The reason we cant always filter modes here is that lets say the DP is 
being used as a built-in display, then this filter is not needed.

Now coming to the HPD part of the next bridge, its not necessary that we 
use the DP bridge's HPD. We could be using the external bridge's HPD.

Like the DSI patch, I should change this to check the last bridge's HPD 
bridge op.

> 
>> +               if (mode->clock > dp_display->max_ext_pclk)
>> +                       return MODE_CLOCK_HIGH;
>>

Re: [RFC PATCH 2/3] drm/msm: filter out modes for DSI bridge having unsupported clock

[Abhinav Kumar]

Hi Dmitry

On 9/8/2022 7:46 AM, Dmitry Baryshkov wrote:
> On 30/08/2022 06:33, Abhinav Kumar wrote:
>> DSI interface used with a bridge chip connected to an external
>> display is subject to the same pixel clock limits as one
>> which is natively pluggable like DisplayPort.
>>
>> Hence filter out DSI modes having an unsupported pixel clock
>> if its connected to a bridge which is pluggable.
>>
>> Ideally, this can be accommodated into msm_dsi_modeset_init()
>> by passing an extra parameter but this will also affect non-dpu
>> targets. Till we add the same logic for non-dpu chipsets, lets
>> have this as a separate call.
> 
> I think this makes DP/DSI depend too much on the DPU and DPU internals. 
> Can we instead use clk_round_rate() in the .mode_valid in DSI/DP/HDMI 
> drivers in order to check that the requested rate can be achieved?
> 

Just to update here what we discussed offline.
Even if we do implement the clk_round_rate(), for the pixel clk it will 
trickle down to the PLL's limits.

This is within the PLL's limits so it wont effectively filter out the 4k 
mode.

Re: [RFC PATCH 0/3] Limit pluggable display modes

[Krzysztof Kozlowski]

On 08/09/2022 20:05, Dmitry Baryshkov wrote:
> 
> On 30/08/2022 06:33, Abhinav Kumar wrote:
>> As reported on https://gitlab.freedesktop.org/drm/msm/-/issues/17, currently
>> there is no mechanism to limit the display output to the pluggable displays
>> and it lets users connect any monitor on any chipset based device.
>>
>> This can lead to undefined behavior because lets say the display
>> advertises an unsupported pixel clock as its preferred resolution, then
>> the end-user can experience undefined behavior such as black screen,
>> crash or an underrun.
>>
>> The capabilities of every chipset are advertised in the product
>> specification for both on-device displays and pluggable displays.
> 
> After discussing this privately, it was agreed that there are two kinds 
> of issues here:
>   - filtering the modes which can not be handled by the DPU/DSI/DP 
> hardware pieces themselves
> 
>   - filtering the modes which can not be handled by the external 
> limitations (e.g. the bridge not being able to drive this mode, the pcb 
> routing of data lanes being unable to sustain the rate, the connector 
> being the limit, etc).
> 
> Krzysztof, I'd like to ask your advice if adding a properly like 
> `max-link-frequency' sounds like a good idea? The video-interfaces.yaml 
> bindings already has the `link-frequencies' property, but it is an array 
> of discrete frequencies supported by the link in the graph. In our case 
> the list of frequencies is more or less continuous, with max (and min) 

I would just use existing link-frequencies for min-max. Your binding
would need to clarify the difference against video-interfaces.

> values. Also, can it be added to the final device in the chain (e.g. 
> hdmi/dp/vga connectors) or should it be added to the endpoint graph nodes?

The same as existing usage of video-interfaces, so the endpoints?


Best regards,
Krzysztof