Re: [PATCH v8 4/4] drm/msm/dp: enable widebus feature for display port

[Stephen Boyd]

Quoting Kuogee Hsieh (2022-02-17 13:36:28)
> Widebus feature will transmit two pixel data per pixel clock to interface.
> This feature now is required to be enabled to easy migrant to higher

s/migrant/migrate/?

> resolution applications in future. However since some legacy chipsets

s/in/in the/

> does not support this feature, this feature is enabled base on chip's

s/does not/don't/

> hardware revision.
>
> changes in v2:
> -- remove compression related code from timing
> -- remove op_info from  struct msm_drm_private
> -- remove unnecessary wide_bus_en variables
> -- pass wide_bus_en into timing configuration by struct msm_dp
>
> Changes in v3:
> -- split patch into 3 patches
> -- enable widebus feature base on chip hardware revision
>
> Changes in v5:
> -- DP_INTF_CONFIG_DATABUS_WIDEN
>
> Changes in v6:
> -- static inline bool msm_dp_wide_bus_enable() in msm_drv.h
>
> Changes in v7:
> -- add Tested-by
>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Tested-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  4 +++-
>  drivers/gpu/drm/msm/dp/dp_catalog.c         | 34 +++++++++++++++++++++++++++--
>  drivers/gpu/drm/msm/dp/dp_catalog.h         |  3 ++-
>  drivers/gpu/drm/msm/dp/dp_ctrl.c            | 13 +++++++----
>  drivers/gpu/drm/msm/dp/dp_ctrl.h            |  1 +
>  drivers/gpu/drm/msm/dp/dp_display.c         | 30 +++++++++++++++++++++++++
>  drivers/gpu/drm/msm/dp/dp_display.h         |  2 ++
>  drivers/gpu/drm/msm/dp/dp_panel.c           |  4 ++--
>  drivers/gpu/drm/msm/dp/dp_panel.h           |  2 +-
>  drivers/gpu/drm/msm/msm_drv.h               |  6 +++++
>  10 files changed, 88 insertions(+), 11 deletions(-)

My mind is blown by the amount of lines that have to change to plumb
through wide_bus_en bool argument. This driver has too many layers.

>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 64f0b26..5c809c6f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -1796,6 +1796,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>         int ret = 0;
>         bool mainlink_ready = false;
>         struct dp_ctrl_private *ctrl;
> +       u32 pixel_rate_orig;

Why u32? Just unsigned long pixel_rate?

>
>         if (!dp_ctrl)
>                 return -EINVAL;
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index e89556ad..bc86c03 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -979,6 +983,7 @@ int dp_display_get_modes(struct msm_dp *dp,
>                 dp->connector, dp_mode);
>         if (dp_mode->drm_mode.clock)
>                 dp->max_pclk_khz = dp_mode->drm_mode.clock;
> +
>         return ret;
>  }
>

This hunk is useless.

> @@ -1451,6 +1456,28 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
>         }
>  }
>
> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display)
> +{
> +       struct dp_display_private *dp;
> +       u32 revision, major, minor;
> +
> +       dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> +       /* for the time being widebus only support on DP */

/* TODO: For the time being only support widebus on DP */

> +       if (dp_display->connector_type  == DRM_MODE_CONNECTOR_DisplayPort) {
> +               revision = dp_catalog_hw_revision(dp->catalog);
> +               major = ((revision >> 28) & 0x0ff);
> +               minor = ((revision >> 16) & 0x0fff);

Use GENMASK() and FIELD_GET()?

> +
> +               DRM_DEBUG_DP("id=%d major=%d minor=%d\n", dp->id, major, minor);
> +
> +               if (major >= 1 && minor >= 2)
> +                       return true;
> +       }
> +
> +       return false;
> +}
> +
>  void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
>  {
>         struct dp_display_private *dp;
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 07f6c41..d11bf5c 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -398,6 +398,7 @@ 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);
>
>  void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
> +bool msm_dp_wide_bus_enable(struct msm_dp *dp_display);

A better name would be msm_dp_wide_bus_available()