diff mbox series

[v4,5/5] drm/msm/dp: Add driver support to utilize drm panel

Message ID 1644494255-6632-6-git-send-email-quic_sbillaka@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Add support for the eDP panel on sc7280 CRD | expand

Commit Message

Sankeerth Billakanti (QUIC) Feb. 10, 2022, 11:57 a.m. UTC
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 v4:
  - Remove obvious comments
  - Define separate connector_ops for eDP
  - Remove unnecessary checks

Changes in v3:
  None

 drivers/gpu/drm/msm/dp/dp_display.c |  6 ++++
 drivers/gpu/drm/msm/dp/dp_drm.c     | 62 +++++++++++++++++++++++++++++++------
 drivers/gpu/drm/msm/dp/dp_parser.h  |  3 ++
 3 files changed, 61 insertions(+), 10 deletions(-)

Comments

Doug Anderson Feb. 19, 2022, 12:44 a.m. UTC | #1
Hi,

On Thu, Feb 10, 2022 at 3:58 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> 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 v4:
>   - Remove obvious comments
>   - Define separate connector_ops for eDP
>   - Remove unnecessary checks
>
> Changes in v3:
>   None
>
>  drivers/gpu/drm/msm/dp/dp_display.c |  6 ++++
>  drivers/gpu/drm/msm/dp/dp_drm.c     | 62 +++++++++++++++++++++++++++++++------
>  drivers/gpu/drm/msm/dp/dp_parser.h  |  3 ++
>  3 files changed, 61 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 7cc4d21..5d314e6 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1513,6 +1513,9 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
>                 return -EINVAL;
>         }
>
> +       if (dp->connector_type == DRM_MODE_CONNECTOR_eDP)
> +               dp_hpd_plug_handle(dp_display, 0);

I'm really not so sure here. You're just totally ignoring the HPD
signal here which isn't right at all. The HPD signal is important for
knowing if an edp panel is ready yet so you can't just ignore it. The
only way this could work is if something else turns the panel on w/
plenty of time before your code runs so it has had time to get
ready...

It feels like we just need to work to get this all plumbed up properly
with the right power sequencing. That'll also allow us to enable the
generic edp-panel stuff...


> +static int edp_connector_get_modes(struct drm_connector *connector)
> +{
> +       struct msm_dp *dp;
> +
> +       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)
> +{
> +       if (mode->clock > EDP_MAX_PIXEL_CLK_KHZ)
> +               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,
> @@ -132,11 +151,24 @@ static const struct drm_connector_funcs dp_connector_funcs = {
>         .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  };
>
> +static const struct drm_connector_funcs edp_connector_funcs = {
> +       .fill_modes = drm_helper_probe_single_connector_modes,
> +       .destroy = drm_connector_cleanup,
> +       .reset = drm_atomic_helper_connector_reset,
> +       .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
>  static const struct drm_connector_helper_funcs dp_connector_helper_funcs = {
>         .get_modes = dp_connector_get_modes,
>         .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,
> +};
> +
>  /* connector initialization */
>  struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display)
>  {
> @@ -154,18 +186,28 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display)
>
>         connector = &dp_connector->base;
>
> -       ret = drm_connector_init(dp_display->drm_dev, connector,
> -                       &dp_connector_funcs,
> -                       dp_display->connector_type);
> -       if (ret)
> -               return ERR_PTR(ret);
> +       if (dp_display->connector_type == DRM_MODE_CONNECTOR_eDP) {
> +               ret = drm_connector_init(dp_display->drm_dev, connector,
> +                               &edp_connector_funcs, DRM_MODE_CONNECTOR_eDP);
> +               if (ret)
> +                       return ERR_PTR(ret);
> +
> +               drm_connector_helper_add(connector,
> +                               &edp_connector_helper_funcs);
> +       } else {
> +               ret = drm_connector_init(dp_display->drm_dev, connector,
> +                               &dp_connector_funcs,
> +                               DRM_MODE_CONNECTOR_DisplayPort);
> +               if (ret)
> +                       return ERR_PTR(ret);
>
> -       drm_connector_helper_add(connector, &dp_connector_helper_funcs);
> +               drm_connector_helper_add(connector, &dp_connector_helper_funcs);

This is probably not the correct way to do this. Drivers like this
should really be moving _away_ from creating their own connectors. The
idea is that you should just be creating bridges and then someone
creates a "bridge connector" that implements the connector functions
atop the bridge.

This is what Dmitry is working on [1]. Speaking of which, he really
ought to be CCed on all your patches.


> -       /*
> -        * 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);
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index 3172da0..58c4f27 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -17,6 +17,9 @@
>  #define DP_MAX_PIXEL_CLK_KHZ   675000
>  #define DP_MAX_NUM_DP_LANES    4
>
> +/* Maximum validated clock */
> +#define EDP_MAX_PIXEL_CLK_KHZ  285550

As discussed out-of-band, this isn't my favorite define. The datasheet
for sc7280, which is what you're testing on / targeting, claims to
support higher rates. Other users of this driver also ought to support
higher rates. It might be OK short term, but it's definitely not a
good long term solution.

[1] https://lore.kernel.org/all/20220211224006.1797846-5-dmitry.baryshkov@linaro.org/
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 7cc4d21..5d314e6 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1513,6 +1513,9 @@  int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
 		return -EINVAL;
 	}
 
+	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 +1580,9 @@  int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
 
 	dp_display = container_of(dp, struct dp_display_private, dp_display);
 
+	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..2436329 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -123,6 +123,25 @@  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;
+
+	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)
+{
+	if (mode->clock > EDP_MAX_PIXEL_CLK_KHZ)
+		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,
@@ -132,11 +151,24 @@  static const struct drm_connector_funcs dp_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
+static const struct drm_connector_funcs edp_connector_funcs = {
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
 static const struct drm_connector_helper_funcs dp_connector_helper_funcs = {
 	.get_modes = dp_connector_get_modes,
 	.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,
+};
+
 /* connector initialization */
 struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display)
 {
@@ -154,18 +186,28 @@  struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display)
 
 	connector = &dp_connector->base;
 
-	ret = drm_connector_init(dp_display->drm_dev, connector,
-			&dp_connector_funcs,
-			dp_display->connector_type);
-	if (ret)
-		return ERR_PTR(ret);
+	if (dp_display->connector_type == DRM_MODE_CONNECTOR_eDP) {
+		ret = drm_connector_init(dp_display->drm_dev, connector,
+				&edp_connector_funcs, DRM_MODE_CONNECTOR_eDP);
+		if (ret)
+			return ERR_PTR(ret);
+
+		drm_connector_helper_add(connector,
+				&edp_connector_helper_funcs);
+	} else {
+		ret = drm_connector_init(dp_display->drm_dev, connector,
+				&dp_connector_funcs,
+				DRM_MODE_CONNECTOR_DisplayPort);
+		if (ret)
+			return ERR_PTR(ret);
 
-	drm_connector_helper_add(connector, &dp_connector_helper_funcs);
+		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);
 
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
index 3172da0..58c4f27 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -17,6 +17,9 @@ 
 #define DP_MAX_PIXEL_CLK_KHZ	675000
 #define DP_MAX_NUM_DP_LANES	4
 
+/* Maximum validated clock */
+#define EDP_MAX_PIXEL_CLK_KHZ	285550
+
 enum dp_pm_type {
 	DP_CORE_PM,
 	DP_CTRL_PM,