Hi Stephen,
Will make the changes.
-----Original Message-----
From: Stephen Boyd <swboyd@xxxxxxxxxxxx>
Sent: Thursday, February 10, 2022 6:52 AM
To: Sankeerth Billakanti (QUIC) <quic_sbillaka@xxxxxxxxxxx>; agross@xxxxxxxxxx; airlied@xxxxxxxx; bjorn.andersson@xxxxxxxxxx; daniel@xxxxxxxx; devicetree@xxxxxxxxxxxxxxx; dianders@xxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; freedreno@xxxxxxxxxxxxxxxxxxxxx; krzysztof.kozlowski@xxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; robdclark@xxxxxxxxx; robh+dt@xxxxxxxxxx; sam@xxxxxxxxxxxx; seanpaul@xxxxxxxxxxxx; thierry.reding@xxxxxxxxx
Cc: quic_kalyant <quic_kalyant@xxxxxxxxxxx>; Abhinav Kumar (QUIC) <quic_abhinavk@xxxxxxxxxxx>; Kuogee Hsieh (QUIC) <quic_khsieh@xxxxxxxxxxx>; quic_mkrishn <quic_mkrishn@xxxxxxxxxxx>; quic_vproddut <quic_vproddut@xxxxxxxxxxx>; dmitry.baryshkov@xxxxxxxxxx
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@xxxxxxxxxxx>
> ---
>
> 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);
>
