[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;