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;
