Re: [Freedreno] [PATCH v3 4/4] drm/msm/dp: Add driver support to utilize drm panel
Hi Stephen,
Will make the changes.
-Original Message-
From: Stephen Boyd
Sent: Thursday, February 10, 2022 6:52 AM
To: Sankeerth Billakanti (QUIC) ; agr...@kernel.org;
airl...@linux.ie; bjorn.anders...@linaro.org; dan...@ffwll.ch;
devicet...@vger.kernel.org; diand...@chromium.org;
dri-de...@lists.freedesktop.org; freedreno@lists.freedesktop.org;
krzysztof.kozlow...@canonical.com; linux-arm-...@vger.kernel.org;
linux-ker...@vger.kernel.org; robdcl...@gmail.com; robh...@kernel.org;
s...@ravnborg.org; seanp...@chromium.org; thierry.red...@gmail.com
Cc: quic_kalyant ; Abhinav Kumar (QUIC)
; Kuogee Hsieh (QUIC) ;
quic_mkrishn ; quic_vproddut
; dmitry.barysh...@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
> ---
>
> 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 ms
Re: [Freedreno] [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
> ---
>
> 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
[Freedreno] [PATCH v3 4/4] drm/msm/dp: Add driver support to utilize drm panel
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
---
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 */
+ 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 */
+ 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;
+
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)
+ 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)
+ return 0;
+
+ dp = to_dp_connector(connector)->dp_display;
+
+ if (mode->clock > EDP_MAX_PIXEL_CLK_KHZ)
+ return MODE_BAD;
+
+ 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,
+
+};
+
/* 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);
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_LANES4
+/* Maximum validated clock */
+#define EDP_MAX_PIXEL_CLK_KHZ 285550
+
enum dp_pm_type {
DP_CORE_PM,
DP_CTRL_PM,
--
2.7.4
