diff mbox series

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

Message ID 1661830389-22439-3-git-send-email-quic_abhinavk@quicinc.com (mailing list archive)
State New, archived
Headers show
Series Limit pluggable display modes | expand

Commit Message

Abhinav Kumar Aug. 30, 2022, 3:33 a.m. UTC
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(-)

Comments

Stephen Boyd Sept. 8, 2022, 1:06 a.m. UTC | #1
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;
Dmitry Baryshkov Sept. 8, 2022, 2:46 p.m. UTC | #2
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?
Abhinav Kumar Sept. 8, 2022, 6:53 p.m. UTC | #3
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;
Abhinav Kumar Sept. 8, 2022, 7:05 p.m. UTC | #4
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.
diff mbox series

Patch

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)
 {
 }