RE: [PATCH v3 4/4] drm/msm/dp: Add driver support to utilize drm panel

["Sankeerth Billakanti (QUIC)" <quic_sbillaka@quicinc.com>]

Hi Stephen,
Will make the changes.

-----Original Message-----
From: Stephen Boyd <swboyd@chromium.org> 
Sent: Thursday, February 10, 2022 6:52 AM
To: Sankeerth Billakanti (QUIC) <quic_sbillaka@quicinc.com>; agross@kernel.org; airlied@linux.ie; bjorn.andersson@linaro.org; daniel@ffwll.ch; devicetree@vger.kernel.org; dianders@chromium.org; dri-devel@lists.freedesktop.org; freedreno@lists.freedesktop.org; krzysztof.kozlowski@canonical.com; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; robdclark@gmail.com; robh+dt@kernel.org; sam@ravnborg.org; seanpaul@chromium.org; thierry.reding@gmail.com
Cc: quic_kalyant <quic_kalyant@quicinc.com>; Abhinav Kumar (QUIC) <quic_abhinavk@quicinc.com>; Kuogee Hsieh (QUIC) <quic_khsieh@quicinc.com>; quic_mkrishn <quic_mkrishn@quicinc.com>; quic_vproddut <quic_vproddut@quicinc.com>; dmitry.baryshkov@linaro.org
Subject: Re: [PATCH v3 4/4] drm/msm/dp: Add driver support to utilize drm panel

Quoting Sankeerth Billakanti (2022-02-09 00:55:32)
> Add support in the DP driver to utilize the custom eDP panels from 
> drm/panels.
>
> An eDP panel is always connected to the platform. So, the eDP 
> connector can be reported as always connected. The display mode will 
> be sourced from the panel. The panel mode will be set after the link 
> training is completed.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> ---
>
> Changes in v3:
>   None
>
>  drivers/gpu/drm/msm/dp/dp_display.c |  8 ++++++
>  drivers/gpu/drm/msm/dp/dp_drm.c     | 54 +++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/msm/dp/dp_parser.h  |  3 +++
>  3 files changed, 60 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 7cc4d21..410fda4 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1513,6 +1513,10 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
>                 return -EINVAL;
>         }
>
> +       /* handle eDP on */

This comment is obvious. Please remove.

> +       if (dp->connector_type == DRM_MODE_CONNECTOR_eDP)
> +               dp_hpd_plug_handle(dp_display, 0);
> +
>         mutex_lock(&dp_display->event_mutex);
>
>         /* stop sentinel checking */
> @@ -1577,6 +1581,10 @@ int msm_dp_display_disable(struct msm_dp *dp, 
> struct drm_encoder *encoder)
>
>         dp_display = container_of(dp, struct dp_display_private, 
> dp_display);
>
> +       /* handle edp off */

This comment is obvious. Please remove.

> +       if (dp->connector_type == DRM_MODE_CONNECTOR_eDP)
> +               dp_hpd_unplug_handle(dp_display, 0);
> +
>         mutex_lock(&dp_display->event_mutex);
>
>         /* stop sentinel checking */
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c 
> b/drivers/gpu/drm/msm/dp/dp_drm.c index d4d360d..12fa8c1 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -39,6 +39,10 @@ static enum drm_connector_status 
> dp_connector_detect(struct drm_connector *conn,
>
>         dp = to_dp_connector(conn)->dp_display;
>
> +       /* eDP is always  connected */
> +       if (dp->connector_type == DRM_MODE_CONNECTOR_eDP)
> +               return connector_status_connected;

Why not implement different connector ops for eDP and then not implement this function at all in that case?

> +
>         DRM_DEBUG_DP("is_connected = %s\n",
>                 (dp->is_connected) ? "true" : "false");
>
> @@ -123,6 +127,35 @@ static enum drm_mode_status dp_connector_mode_valid(
>         return dp_display_validate_mode(dp_disp, mode->clock);  }
>
> +static int edp_connector_get_modes(struct drm_connector *connector) {
> +       struct msm_dp *dp;
> +
> +       if (!connector)

Is this check really necessary? Why doesn't drm do it in higher layers?

> +               return 0;
> +
> +       dp = to_dp_connector(connector)->dp_display;
> +
> +       return drm_bridge_get_modes(dp->panel_bridge, connector); }
> +
> +static enum drm_mode_status edp_connector_mode_valid(
> +               struct drm_connector *connector,
> +               struct drm_display_mode *mode) {
> +       struct msm_dp *dp;
> +
> +       if (!connector)

Is this check really necessary? Why doesn't drm do it in higher layers?

> +               return 0;
> +
> +       dp = to_dp_connector(connector)->dp_display;
> +
> +       if (mode->clock > EDP_MAX_PIXEL_CLK_KHZ)
> +               return MODE_BAD;

Why not return MODE_CLOCK_HIGH?

> +
> +       return MODE_OK;
> +}
> +
>  static const struct drm_connector_funcs dp_connector_funcs = {
>         .detect = dp_connector_detect,
>         .fill_modes = drm_helper_probe_single_connector_modes,
> @@ -137,6 +170,12 @@ static const struct drm_connector_helper_funcs dp_connector_helper_funcs = {
>         .mode_valid = dp_connector_mode_valid,  };
>
> +static const struct drm_connector_helper_funcs edp_connector_helper_funcs = {
> +       .get_modes = edp_connector_get_modes,
> +       .mode_valid = edp_connector_mode_valid,
> +

Why the extra newline?

> +};
> +
>  /* connector initialization */
>  struct drm_connector *dp_drm_connector_init(struct msm_dp 
> *dp_display)  { @@ -160,12 +199,17 @@ struct drm_connector 
> *dp_drm_connector_init(struct msm_dp *dp_display)
>         if (ret)
>                 return ERR_PTR(ret);
>
> -       drm_connector_helper_add(connector, &dp_connector_helper_funcs);
> +       if (dp_display->connector_type == DRM_MODE_CONNECTOR_eDP) {
> +               drm_connector_helper_add(connector,
> +                               &edp_connector_helper_funcs);
> +       } else {
> +               drm_connector_helper_add(connector, 
> + &dp_connector_helper_funcs);
>
> -       /*
> -        * Enable HPD to let hpd event is handled when cable is connected.
> -        */
> -       connector->polled = DRM_CONNECTOR_POLL_HPD;
> +               /*
> +                * Enable HPD to let hpd event is handled when cable is connected.
> +                */
> +               connector->polled = DRM_CONNECTOR_POLL_HPD;
> +       }
>
>         drm_connector_attach_encoder(connector, dp_display->encoder);
>