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;
