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()
