The current implementation supports a single DP instance and the DPU code will
only match it against INTF_DP instance 0. These patches extends this to allow
multiple DP instances and support for matching against DP instances beyond 0.
This is based on v4 of Dmitry's work on multiple DSI interfaces:
https://lore.kernel.org/linux-arm-msm/[email protected]/
With that in place add SC8180x DP and eDP controllers.
Bjorn Andersson (5):
drm/msm/dp: Remove global g_dp_display variable
drm/msm/dp: Modify prototype of encoder based API
drm/msm/dp: Support up to 3 DP controllers
dt-bindings: msm/dp: Add SC8180x compatibles
drm/msm/dp: Add sc8180x DP controllers
.../bindings/display/msm/dp-controller.yaml | 2 +
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 60 +++---
.../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 8 +-
drivers/gpu/drm/msm/dp/dp_display.c | 183 +++++++++++++-----
drivers/gpu/drm/msm/msm_drv.h | 33 ++--
6 files changed, 200 insertions(+), 103 deletions(-)
--
2.29.2
Functions in the DisplayPort code that relates to individual instances
(encoders) are passed both the struct msm_dp and the struct drm_encoder. But
in a situation where multiple DP instances would exist this means that
the caller need to resolve which struct msm_dp relates to the struct
drm_encoder at hand.
The information for doing this lookup is available inside the DP driver,
so update the API to take the struct msm_drm_private and the struct
drm_encoder and have the DP code figure out which struct msm_dp the
operation relates to.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 +++++----
drivers/gpu/drm/msm/dp/dp_display.c | 38 +++++++++++++++++----
drivers/gpu/drm/msm/msm_drv.h | 31 +++++++++--------
3 files changed, 56 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1c04b7cce43e..0d64ef0819af 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1002,8 +1002,8 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
trace_dpu_enc_mode_set(DRMID(drm_enc));
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp)
- msm_dp_display_mode_set(priv->dp, drm_enc, mode, adj_mode);
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS)
+ msm_dp_display_mode_set(priv, drm_enc, mode, adj_mode);
list_for_each_entry(conn_iter, connector_list, head)
if (conn_iter->encoder == drm_enc)
@@ -1184,9 +1184,8 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
_dpu_encoder_virt_enable_helper(drm_enc);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
- ret = msm_dp_display_enable(priv->dp,
- drm_enc);
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+ ret = msm_dp_display_enable(priv, drm_enc);
if (ret) {
DPU_ERROR_ENC(dpu_enc, "dp display enable failed: %d\n",
ret);
@@ -1226,8 +1225,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
/* wait for idle */
dpu_encoder_wait_for_event(drm_enc, MSM_ENC_TX_COMPLETE);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
- if (msm_dp_display_pre_disable(priv->dp, drm_enc))
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+ if (msm_dp_display_pre_disable(priv, drm_enc))
DPU_ERROR_ENC(dpu_enc, "dp display push idle failed\n");
}
@@ -1255,8 +1254,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
- if (msm_dp_display_disable(priv->dp, drm_enc))
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+ if (msm_dp_display_disable(priv, drm_enc))
DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
}
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 8696b36d30e4..59ffd6c8f41f 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1432,12 +1432,25 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
return 0;
}
-int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
+static struct msm_dp *msm_dp_from_drm_encoder(struct msm_drm_private *priv,
+ struct drm_encoder *encoder)
+{
+ if (priv->dp && priv->dp->encoder == encoder)
+ return priv->dp;
+
+ return NULL;
+}
+
+int msm_dp_display_enable(struct msm_drm_private *priv, struct drm_encoder *encoder)
{
int rc = 0;
struct dp_display_private *dp_display;
+ struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
u32 state;
+ if (!dp)
+ return -EINVAL;
+
dp_display = container_of(dp, struct dp_display_private, dp_display);
if (!dp_display->dp_mode.drm_mode.clock) {
DRM_ERROR("invalid params\n");
@@ -1489,9 +1502,13 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
return rc;
}
-int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder)
+int msm_dp_display_pre_disable(struct msm_drm_private *priv, struct drm_encoder *encoder)
{
struct dp_display_private *dp_display;
+ struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
+
+ if (!dp)
+ return 0;
dp_display = container_of(dp, struct dp_display_private, dp_display);
@@ -1500,11 +1517,15 @@ int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder)
return 0;
}
-int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
+int msm_dp_display_disable(struct msm_drm_private *priv, struct drm_encoder *encoder)
{
int rc = 0;
u32 state;
struct dp_display_private *dp_display;
+ struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
+
+ if (!dp)
+ return 0;
dp_display = container_of(dp, struct dp_display_private, dp_display);
@@ -1531,11 +1552,16 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
return rc;
}
-void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,
- struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode)
+void msm_dp_display_mode_set(struct msm_drm_private *priv,
+ struct drm_encoder *encoder,
+ struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode)
{
struct dp_display_private *dp_display;
+ struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
+
+ if (!dp)
+ return;
dp_display = container_of(dp, struct dp_display_private, dp_display);
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 9bfd37855969..e9232032b266 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -388,12 +388,13 @@ int __init msm_dp_register(void);
void __exit msm_dp_unregister(void);
int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
struct drm_encoder *encoder);
-int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder);
-int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder);
-int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder);
-void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,
- struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode);
+int msm_dp_display_enable(struct msm_drm_private *priv, struct drm_encoder *encoder);
+int msm_dp_display_disable(struct msm_drm_private *priv, struct drm_encoder *encoder);
+int msm_dp_display_pre_disable(struct msm_drm_private *priv, struct drm_encoder *encoder);
+void msm_dp_display_mode_set(struct msm_drm_private *priv,
+ struct drm_encoder *encoder,
+ struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode);
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);
@@ -413,25 +414,25 @@ static inline int msm_dp_modeset_init(struct msm_dp *dp_display,
{
return -EINVAL;
}
-static inline int msm_dp_display_enable(struct msm_dp *dp,
+static inline int msm_dp_display_enable(struct msm_drm_private *priv,
struct drm_encoder *encoder)
{
return -EINVAL;
}
-static inline int msm_dp_display_disable(struct msm_dp *dp,
- struct drm_encoder *encoder)
+static inline int msm_dp_display_disable(struct msm_drm_private *priv,
+ struct drm_encoder *encoder)
{
return -EINVAL;
}
-static inline int msm_dp_display_pre_disable(struct msm_dp *dp,
- struct drm_encoder *encoder)
+static inline int msm_dp_display_pre_disable(struct msm_drm_private *priv,
+ struct drm_encoder *encoder)
{
return -EINVAL;
}
-static inline void msm_dp_display_mode_set(struct msm_dp *dp,
- struct drm_encoder *encoder,
- struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode)
+static inline void msm_dp_display_mode_set(struct msm_drm_private *priv,
+ struct drm_encoder *encoder,
+ struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode)
{
}
--
2.29.2
As the Qualcomm DisplayPort driver only supports a single instance of
the driver the commonly used struct dp_display is kept in a global
variable. As we introduce additional instances this obviously doesn't
work.
Replace this with a combination of existing references to adjacent
objects and drvdata.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/gpu/drm/msm/dp/dp_display.c | 78 ++++++++++++++---------------
1 file changed, 37 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 70b319a8fe83..8696b36d30e4 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -27,7 +27,6 @@
#include "dp_audio.h"
#include "dp_debug.h"
-static struct msm_dp *g_dp_display;
#define HPD_STRING_SIZE 30
enum {
@@ -122,6 +121,13 @@ static const struct of_device_id dp_dt_match[] = {
{}
};
+static struct dp_display_private *dev_to_dp_display_private(struct device *dev)
+{
+ struct msm_dp *dp = dev_get_drvdata(dev);
+
+ return container_of(dp, struct dp_display_private, dp_display);
+}
+
static int dp_add_event(struct dp_display_private *dp_priv, u32 event,
u32 data, u32 delay)
{
@@ -198,14 +204,16 @@ static int dp_display_bind(struct device *dev, struct device *master,
void *data)
{
int rc = 0;
- struct dp_display_private *dp;
+ struct dp_display_private *dp = dev_to_dp_display_private(dev);
struct drm_device *drm;
struct msm_drm_private *priv;
drm = dev_get_drvdata(master);
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ if (!dp) {
+ DRM_ERROR("DP driver bind failed. Invalid driver data\n");
+ return -EINVAL;
+ }
dp->dp_display.drm_dev = drm;
priv = drm->dev_private;
@@ -240,12 +248,14 @@ static int dp_display_bind(struct device *dev, struct device *master,
static void dp_display_unbind(struct device *dev, struct device *master,
void *data)
{
- struct dp_display_private *dp;
+ struct dp_display_private *dp = dev_to_dp_display_private(dev);
struct drm_device *drm = dev_get_drvdata(master);
struct msm_drm_private *priv = drm->dev_private;
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ if (!dp) {
+ DRM_ERROR("Invalid DP driver data\n");
+ return;
+ }
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
@@ -376,17 +386,14 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
static int dp_display_usbpd_configure_cb(struct device *dev)
{
int rc = 0;
- struct dp_display_private *dp;
+ struct dp_display_private *dp = dev_to_dp_display_private(dev);
- if (!dev) {
- DRM_ERROR("invalid dev\n");
- rc = -EINVAL;
+ if (!dp) {
+ DRM_ERROR("no driver data found\n");
+ rc = -ENODEV;
goto end;
}
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
-
dp_display_host_init(dp, false);
rc = dp_display_process_hpd_high(dp);
@@ -397,17 +404,14 @@ static int dp_display_usbpd_configure_cb(struct device *dev)
static int dp_display_usbpd_disconnect_cb(struct device *dev)
{
int rc = 0;
- struct dp_display_private *dp;
+ struct dp_display_private *dp = dev_to_dp_display_private(dev);
- if (!dev) {
- DRM_ERROR("invalid dev\n");
- rc = -EINVAL;
+ if (!dp) {
+ DRM_ERROR("no driver data found\n");
+ rc = -ENODEV;
return rc;
}
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
-
dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
return rc;
@@ -466,15 +470,15 @@ static int dp_display_usbpd_attention_cb(struct device *dev)
{
int rc = 0;
u32 sink_request;
- struct dp_display_private *dp;
+ struct dp_display_private *dp = dev_to_dp_display_private(dev);
+ struct dp_usbpd *hpd;
- if (!dev) {
- DRM_ERROR("invalid dev\n");
- return -EINVAL;
+ if (!dp) {
+ DRM_ERROR("no driver data found\n");
+ return -ENODEV;
}
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ hpd = dp->usbpd;
/* check for any test request issued by sink */
rc = dp_link_process_request(dp->link);
@@ -638,7 +642,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
/* signal the disconnect event early to ensure proper teardown */
- dp_display_handle_plugged_change(g_dp_display, false);
+ dp_display_handle_plugged_change(&dp->dp_display, false);
/* enable HDP plug interrupt to prepare for next plugin */
dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
@@ -832,9 +836,7 @@ static int dp_display_prepare(struct msm_dp *dp)
static int dp_display_enable(struct dp_display_private *dp, u32 data)
{
int rc = 0;
- struct msm_dp *dp_display;
-
- dp_display = g_dp_display;
+ struct msm_dp *dp_display = &dp->dp_display;
if (dp_display->power_on) {
DRM_DEBUG_DP("Link already setup, return\n");
@@ -869,9 +871,7 @@ static int dp_display_post_enable(struct msm_dp *dp_display)
static int dp_display_disable(struct dp_display_private *dp, u32 data)
{
- struct msm_dp *dp_display;
-
- dp_display = g_dp_display;
+ struct msm_dp *dp_display = &dp->dp_display;
if (!dp_display->power_on)
return 0;
@@ -1229,14 +1229,13 @@ static int dp_display_probe(struct platform_device *pdev)
}
mutex_init(&dp->event_mutex);
- g_dp_display = &dp->dp_display;
/* Store DP audio handle inside DP display */
- g_dp_display->dp_audio = dp->audio;
+ dp->dp_display.dp_audio = dp->audio;
init_completion(&dp->audio_comp);
- platform_set_drvdata(pdev, g_dp_display);
+ platform_set_drvdata(pdev, &dp->dp_display);
rc = component_add(&pdev->dev, &dp_display_comp_ops);
if (rc) {
@@ -1249,10 +1248,7 @@ static int dp_display_probe(struct platform_device *pdev)
static int dp_display_remove(struct platform_device *pdev)
{
- struct dp_display_private *dp;
-
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ struct dp_display_private *dp = platform_get_drvdata(pdev);
dp_display_deinit_sub_modules(dp);
--
2.29.2
Based on the removal of the g_dp_display and the movement of the
priv->dp lookup into the DP code it's now possible to have multiple
DP instances.
In line with the other controllers in the MSM driver, introduce a
per-compatible list of base addresses which is used to resolve the
"instance id" for the given DP controller. This instance id is used as
index in the priv->dp[] array.
Then extend the initialization code to initialize struct drm_encoder for
each of the registered priv->dp[] and update the logic for finding a
struct msm_dp from a struct drm_encoder.
Lastly, bump the number of struct msm_dp instances carries by priv->dp
to 3, the currently known maximum number of controllers found in a
Qualcomm SoC.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 60 +++++++++++--------
.../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 8 ++-
drivers/gpu/drm/msm/dp/dp_display.c | 59 ++++++++++++++++--
drivers/gpu/drm/msm/msm_drv.h | 2 +-
4 files changed, 95 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index f655adbc2421..a793cc8a007e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -188,6 +188,7 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms, struct drm_minor *minor)
struct dentry *entry;
struct drm_device *dev;
struct msm_drm_private *priv;
+ int i;
if (!p)
return -EINVAL;
@@ -203,8 +204,8 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms, struct drm_minor *minor)
dpu_debugfs_vbif_init(dpu_kms, entry);
dpu_debugfs_core_irq_init(dpu_kms, entry);
- if (priv->dp)
- msm_dp_debugfs_init(priv->dp, minor);
+ for (i = 0; i < ARRAY_SIZE(priv->dp); i++)
+ msm_dp_debugfs_init(priv->dp[i], minor);
return dpu_core_perf_debugfs_init(dpu_kms, entry);
}
@@ -545,33 +546,40 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
struct drm_encoder *encoder = NULL;
struct msm_display_info info;
int rc = 0;
+ int i;
- if (!priv->dp)
- return rc;
+ for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+ if (!priv->dp[i])
+ continue;
- encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
- if (IS_ERR(encoder)) {
- DPU_ERROR("encoder init failed for dsi display\n");
- return PTR_ERR(encoder);
- }
+ encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
+ if (IS_ERR(encoder)) {
+ DPU_ERROR("encoder init failed for dsi display\n");
+ return PTR_ERR(encoder);
+ }
- memset(&info, 0, sizeof(info));
- rc = msm_dp_modeset_init(priv->dp, dev, encoder);
- if (rc) {
- DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
- drm_encoder_cleanup(encoder);
- return rc;
- }
+ memset(&info, 0, sizeof(info));
+ rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
+ if (rc) {
+ DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
+ drm_encoder_cleanup(encoder);
+ return rc;
+ }
- priv->encoders[priv->num_encoders++] = encoder;
+ priv->encoders[priv->num_encoders++] = encoder;
+
+ info.num_of_h_tiles = 1;
+ info.h_tile_instance[0] = i;
+ info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
+ info.intf_type = encoder->encoder_type;
+ rc = dpu_encoder_setup(dev, encoder, &info);
+ if (rc) {
+ DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
+ encoder->base.id, rc);
+ return rc;
+ }
+ }
- info.num_of_h_tiles = 1;
- info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
- info.intf_type = encoder->encoder_type;
- rc = dpu_encoder_setup(dev, encoder, &info);
- if (rc)
- DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
- encoder->base.id, rc);
return rc;
}
@@ -792,6 +800,7 @@ static int dpu_irq_postinstall(struct msm_kms *kms)
{
struct msm_drm_private *priv;
struct dpu_kms *dpu_kms = to_dpu_kms(kms);
+ int i;
if (!dpu_kms || !dpu_kms->dev)
return -EINVAL;
@@ -800,7 +809,8 @@ static int dpu_irq_postinstall(struct msm_kms *kms)
if (!priv)
return -EINVAL;
- msm_dp_irq_postinstall(priv->dp);
+ for (i = 0; i < ARRAY_SIZE(priv->dp); i++)
+ msm_dp_irq_postinstall(priv->dp[i]);
return 0;
}
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
index cabe15190ec1..2e1acb1bc390 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -126,8 +126,12 @@ void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
priv = drm_dev->dev_private;
kms = priv->kms;
- if (priv->dp)
- msm_dp_snapshot(disp_state, priv->dp);
+ for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+ if (!priv->dp[i])
+ continue;
+
+ msm_dp_snapshot(disp_state, priv->dp[i]);
+ }
for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
if (!priv->dsi[i])
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 59ffd6c8f41f..92b7646a1bb7 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -79,6 +79,8 @@ struct dp_display_private {
char *name;
int irq;
+ int id;
+
/* state variables */
bool core_initialized;
bool hpd_irq_on;
@@ -116,8 +118,19 @@ struct dp_display_private {
struct dp_audio *audio;
};
+
+struct msm_dp_config {
+ phys_addr_t io_start[3];
+ size_t num_dp;
+};
+
+static const struct msm_dp_config sc7180_dp_cfg = {
+ .io_start = { 0x0ae90000 },
+ .num_dp = 1,
+};
+
static const struct of_device_id dp_dt_match[] = {
- {.compatible = "qcom,sc7180-dp"},
+ { .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
{}
};
@@ -217,7 +230,7 @@ static int dp_display_bind(struct device *dev, struct device *master,
dp->dp_display.drm_dev = drm;
priv = drm->dev_private;
- priv->dp = &(dp->dp_display);
+ priv->dp[dp->id] = &(dp->dp_display);
rc = dp->parser->parse(dp->parser);
if (rc) {
@@ -238,8 +251,11 @@ static int dp_display_bind(struct device *dev, struct device *master,
}
rc = dp_register_audio_driver(dev, dp->audio);
- if (rc)
+ if (rc) {
DRM_ERROR("Audio registration Dp failed\n");
+ goto end;
+ }
+
end:
return rc;
@@ -259,7 +275,7 @@ static void dp_display_unbind(struct device *dev, struct device *master,
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
- priv->dp = NULL;
+ priv->dp[dp->id] = NULL;
}
static const struct component_ops dp_display_comp_ops = {
@@ -1205,6 +1221,26 @@ int dp_display_request_irq(struct msm_dp *dp_display)
return 0;
}
+static int dp_display_get_id(struct platform_device *pdev)
+{
+ const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
+ struct resource *res;
+ int i;
+
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -EINVAL;
+
+ for (i = 0; i < cfg->num_dp; i++) {
+ if (cfg->io_start[i] == res->start)
+ return i;
+ }
+
+ dev_err(&pdev->dev, "unknown displayport instance\n");
+ return -EINVAL;
+}
+
static int dp_display_probe(struct platform_device *pdev)
{
int rc = 0;
@@ -1219,6 +1255,10 @@ static int dp_display_probe(struct platform_device *pdev)
if (!dp)
return -ENOMEM;
+ dp->id = dp_display_get_id(pdev);
+ if (dp->id < 0)
+ return -EINVAL;
+
dp->pdev = pdev;
dp->name = "drm_dp";
@@ -1386,6 +1426,9 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
struct device *dev;
int rc;
+ if (!dp_display)
+ return;
+
dp = container_of(dp_display, struct dp_display_private, dp_display);
dev = &dp->pdev->dev;
@@ -1435,8 +1478,12 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
static struct msm_dp *msm_dp_from_drm_encoder(struct msm_drm_private *priv,
struct drm_encoder *encoder)
{
- if (priv->dp && priv->dp->encoder == encoder)
- return priv->dp;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+ if (priv->dp[i] && priv->dp[i]->encoder == encoder)
+ return priv->dp[i];
+ }
return NULL;
}
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index e9232032b266..62d54ef6c2c4 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -161,7 +161,7 @@ struct msm_drm_private {
/* DSI is shared by mdp4 and mdp5 */
struct msm_dsi *dsi[2];
- struct msm_dp *dp;
+ struct msm_dp *dp[3];
/* when we have more than one 'msm_gpu' these need to be an array: */
struct msm_gpu *gpu;
--
2.29.2
The sc8180x has 2 DP and 1 eDP controllers, add support for these to the
DP driver.
Link: https://lore.kernel.org/linux-arm-msm/[email protected]/
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/gpu/drm/msm/dp/dp_display.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 92b7646a1bb7..c26805cfcdd1 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -129,8 +129,20 @@ static const struct msm_dp_config sc7180_dp_cfg = {
.num_dp = 1,
};
+static const struct msm_dp_config sc8180x_dp_cfg = {
+ .io_start = { 0xae90000, 0xae98000, 0 },
+ .num_dp = 3,
+};
+
+static const struct msm_dp_config sc8180x_edp_cfg = {
+ .io_start = { 0, 0, 0xae9a000 },
+ .num_dp = 3,
+};
+
static const struct of_device_id dp_dt_match[] = {
{ .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
+ { .compatible = "qcom,sc8180x-dp", .data = &sc8180x_dp_cfg },
+ { .compatible = "qcom,sc8180x-edp", .data = &sc8180x_edp_cfg },
{}
};
--
2.29.2
The Qualcomm SC8180x has 2 DP controllers and 1 eDP controller, add
compatibles for these to the msm/dp binding.
Signed-off-by: Bjorn Andersson <[email protected]>
---
.../devicetree/bindings/display/msm/dp-controller.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index a6e41be038fc..c6056e0b0845 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -17,6 +17,8 @@ properties:
compatible:
enum:
- qcom,sc7180-dp
+ - qcom,sc8180x-dp
+ - qcom,sc8180x-edp
reg:
items:
--
2.29.2
The sc8180x has 2 DP and 1 eDP controllers, add support for these to the
DP driver.
Link: https://lore.kernel.org/linux-arm-msm/[email protected]/
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/gpu/drm/msm/dp/dp_display.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 92b7646a1bb7..c26805cfcdd1 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -129,8 +129,20 @@ static const struct msm_dp_config sc7180_dp_cfg = {
.num_dp = 1,
};
+static const struct msm_dp_config sc8180x_dp_cfg = {
+ .io_start = { 0xae90000, 0xae98000, 0 },
+ .num_dp = 3,
+};
+
+static const struct msm_dp_config sc8180x_edp_cfg = {
+ .io_start = { 0, 0, 0xae9a000 },
+ .num_dp = 3,
+};
+
static const struct of_device_id dp_dt_match[] = {
{ .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
+ { .compatible = "qcom,sc8180x-dp", .data = &sc8180x_dp_cfg },
+ { .compatible = "qcom,sc8180x-edp", .data = &sc8180x_edp_cfg },
{}
};
--
2.29.2
Quoting Bjorn Andersson (2021-07-24 21:24:31)
> As the Qualcomm DisplayPort driver only supports a single instance of
> the driver the commonly used struct dp_display is kept in a global
> variable. As we introduce additional instances this obviously doesn't
> work.
>
> Replace this with a combination of existing references to adjacent
> objects and drvdata.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
Thanks for removing the global.
> drivers/gpu/drm/msm/dp/dp_display.c | 78 ++++++++++++++---------------
> 1 file changed, 37 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 70b319a8fe83..8696b36d30e4 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -27,7 +27,6 @@
> #include "dp_audio.h"
> #include "dp_debug.h"
>
> -static struct msm_dp *g_dp_display;
> #define HPD_STRING_SIZE 30
>
> enum {
> @@ -122,6 +121,13 @@ static const struct of_device_id dp_dt_match[] = {
> {}
> };
>
> +static struct dp_display_private *dev_to_dp_display_private(struct device *dev)
> +{
> + struct msm_dp *dp = dev_get_drvdata(dev);
> +
> + return container_of(dp, struct dp_display_private, dp_display);
> +}
> +
> static int dp_add_event(struct dp_display_private *dp_priv, u32 event,
> u32 data, u32 delay)
> {
> @@ -198,14 +204,16 @@ static int dp_display_bind(struct device *dev, struct device *master,
> void *data)
> {
> int rc = 0;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
> struct drm_device *drm;
> struct msm_drm_private *priv;
>
> drm = dev_get_drvdata(master);
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + if (!dp) {
How can it be NULL? dev_to_dp_display_private() returns container_of()
pointer so it doesn't look possible.
> + DRM_ERROR("DP driver bind failed. Invalid driver data\n");
> + return -EINVAL;
> + }
>
> dp->dp_display.drm_dev = drm;
> priv = drm->dev_private;
Quoting Bjorn Andersson (2021-07-24 21:24:33)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 59ffd6c8f41f..92b7646a1bb7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -238,8 +251,11 @@ static int dp_display_bind(struct device *dev, struct device *master,
> }
>
> rc = dp_register_audio_driver(dev, dp->audio);
> - if (rc)
> + if (rc) {
> DRM_ERROR("Audio registration Dp failed\n");
> + goto end;
> + }
> +
>
> end:
> return rc;
This hunk looks useless. Drop it?
> @@ -1205,6 +1221,26 @@ int dp_display_request_irq(struct msm_dp *dp_display)
> return 0;
> }
>
> +static int dp_display_get_id(struct platform_device *pdev)
> +{
> + const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
> + struct resource *res;
> + int i;
> +
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> +
> + for (i = 0; i < cfg->num_dp; i++) {
> + if (cfg->io_start[i] == res->start)
> + return i;
> + }
> +
> + dev_err(&pdev->dev, "unknown displayport instance\n");
> + return -EINVAL;
> +}
> +
> static int dp_display_probe(struct platform_device *pdev)
> {
> int rc = 0;
> @@ -1219,6 +1255,10 @@ static int dp_display_probe(struct platform_device *pdev)
> if (!dp)
> return -ENOMEM;
>
> + dp->id = dp_display_get_id(pdev);
> + if (dp->id < 0)
> + return -EINVAL;
> +
> dp->pdev = pdev;
> dp->name = "drm_dp";
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index e9232032b266..62d54ef6c2c4 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -161,7 +161,7 @@ struct msm_drm_private {
> /* DSI is shared by mdp4 and mdp5 */
> struct msm_dsi *dsi[2];
>
> - struct msm_dp *dp;
> + struct msm_dp *dp[3];
It would be nice to either make this dynamically sized (probably little
gain), somehow make a BUILD_BUG_ON(), or have a WARN_ON if
ARRAY_SIZE(dp) is less than a num_dp so we know somebody messed up.
Quoting Bjorn Andersson (2021-07-24 21:24:35)
> The Qualcomm SC8180x has 2 DP controllers and 1 eDP controller, add
> compatibles for these to the msm/dp binding.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
Reviewed-by: Stephen Boyd <[email protected]>
Quoting Bjorn Andersson (2021-07-24 21:24:36)
> The sc8180x has 2 DP and 1 eDP controllers, add support for these to the
> DP driver.
>
> Link: https://lore.kernel.org/linux-arm-msm/[email protected]/
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 92b7646a1bb7..c26805cfcdd1 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -129,8 +129,20 @@ static const struct msm_dp_config sc7180_dp_cfg = {
> .num_dp = 1,
> };
>
> +static const struct msm_dp_config sc8180x_dp_cfg = {
> + .io_start = { 0xae90000, 0xae98000, 0 },
> + .num_dp = 3,
> +};
> +
> +static const struct msm_dp_config sc8180x_edp_cfg = {
> + .io_start = { 0, 0, 0xae9a000 },
> + .num_dp = 3,
> +};
Can the two structs not be combined into one struct and set as .data for
either compatible?
> +
> static const struct of_device_id dp_dt_match[] = {
> { .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
> + { .compatible = "qcom,sc8180x-dp", .data = &sc8180x_dp_cfg },
> + { .compatible = "qcom,sc8180x-edp", .data = &sc8180x_edp_cfg },
> {}
Quoting Bjorn Andersson (2021-07-24 21:24:32)
> Functions in the DisplayPort code that relates to individual instances
> (encoders) are passed both the struct msm_dp and the struct drm_encoder. But
> in a situation where multiple DP instances would exist this means that
> the caller need to resolve which struct msm_dp relates to the struct
> drm_encoder at hand.
>
> The information for doing this lookup is available inside the DP driver,
> so update the API to take the struct msm_drm_private and the struct
> drm_encoder and have the DP code figure out which struct msm_dp the
> operation relates to.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
Reviewed-by: Stephen Boyd <[email protected]>
On 25/07/2021 07:24, Bjorn Andersson wrote:
> As the Qualcomm DisplayPort driver only supports a single instance of
> the driver the commonly used struct dp_display is kept in a global
> variable. As we introduce additional instances this obviously doesn't
> work.
>
> Replace this with a combination of existing references to adjacent
> objects and drvdata.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 78 ++++++++++++++---------------
> 1 file changed, 37 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 70b319a8fe83..8696b36d30e4 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -27,7 +27,6 @@
> #include "dp_audio.h"
> #include "dp_debug.h"
>
> -static struct msm_dp *g_dp_display;
> #define HPD_STRING_SIZE 30
>
> enum {
> @@ -122,6 +121,13 @@ static const struct of_device_id dp_dt_match[] = {
> {}
> };
>
> +static struct dp_display_private *dev_to_dp_display_private(struct device *dev)
dev_get_dp_display_private() ?
> +{
> + struct msm_dp *dp = dev_get_drvdata(dev);
> +
> + return container_of(dp, struct dp_display_private, dp_display);
> +}
> +
As a matter of preference, it might be cleaner to inline dev_get_drvdata
and then define msm_dp_get_private to convert from msm_dp to
dp_display_private, see below.
> static int dp_add_event(struct dp_display_private *dp_priv, u32 event,
> u32 data, u32 delay)
> {
> @@ -198,14 +204,16 @@ static int dp_display_bind(struct device *dev, struct device *master,
> void *data)
> {
> int rc = 0;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
> struct drm_device *drm;
> struct msm_drm_private *priv;
>
> drm = dev_get_drvdata(master);
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + if (!dp) {
This is not correct, if I'm not mistaken. you wanted to check if dev's
private data is NULL (correct check), but ended up checking whether the
result of container_of is NULL (incorrect).
> + DRM_ERROR("DP driver bind failed. Invalid driver data\n");
> + return -EINVAL;
> + }
>
> dp->dp_display.drm_dev = drm;
> priv = drm->dev_private;
> @@ -240,12 +248,14 @@ static int dp_display_bind(struct device *dev, struct device *master,
> static void dp_display_unbind(struct device *dev, struct device *master,
> void *data)
> {
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
> struct drm_device *drm = dev_get_drvdata(master);
> struct msm_drm_private *priv = drm->dev_private;
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + if (!dp) {
> + DRM_ERROR("Invalid DP driver data\n");
> + return;
> + }
>
> dp_power_client_deinit(dp->power);
> dp_aux_unregister(dp->aux);
> @@ -376,17 +386,14 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
> static int dp_display_usbpd_configure_cb(struct device *dev)
> {
> int rc = 0;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
>
> - if (!dev) {
> - DRM_ERROR("invalid dev\n");
> - rc = -EINVAL;
> + if (!dp) {
> + DRM_ERROR("no driver data found\n");
> + rc = -ENODEV;
> goto end;
> }
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> -
> dp_display_host_init(dp, false);
>
> rc = dp_display_process_hpd_high(dp);
> @@ -397,17 +404,14 @@ static int dp_display_usbpd_configure_cb(struct device *dev)
> static int dp_display_usbpd_disconnect_cb(struct device *dev)
> {
> int rc = 0;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
>
> - if (!dev) {
> - DRM_ERROR("invalid dev\n");
> - rc = -EINVAL;
`!dev` check should remain in place. And dp should be fetched
afterwards. This applies to all the checks in the patch.
> + if (!dp) {
> + DRM_ERROR("no driver data found\n");
> + rc = -ENODEV;
> return rc;
> }
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> -
> dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
>
> return rc;
> @@ -466,15 +470,15 @@ static int dp_display_usbpd_attention_cb(struct device *dev)
> {
> int rc = 0;
> u32 sink_request;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
> + struct dp_usbpd *hpd;
>
> - if (!dev) {
> - DRM_ERROR("invalid dev\n");
> - return -EINVAL;
> + if (!dp) {
> + DRM_ERROR("no driver data found\n");
> + return -ENODEV;
> }
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + hpd = dp->usbpd;
>
> /* check for any test request issued by sink */
> rc = dp_link_process_request(dp->link);
> @@ -638,7 +642,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
> dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
>
> /* signal the disconnect event early to ensure proper teardown */
> - dp_display_handle_plugged_change(g_dp_display, false);
> + dp_display_handle_plugged_change(&dp->dp_display, false);
>
> /* enable HDP plug interrupt to prepare for next plugin */
> dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
> @@ -832,9 +836,7 @@ static int dp_display_prepare(struct msm_dp *dp)
> static int dp_display_enable(struct dp_display_private *dp, u32 data)
> {
> int rc = 0;
> - struct msm_dp *dp_display;
> -
> - dp_display = g_dp_display;
> + struct msm_dp *dp_display = &dp->dp_display;
>
> if (dp_display->power_on) {
> DRM_DEBUG_DP("Link already setup, return\n");
> @@ -869,9 +871,7 @@ static int dp_display_post_enable(struct msm_dp *dp_display)
>
> static int dp_display_disable(struct dp_display_private *dp, u32 data)
> {
> - struct msm_dp *dp_display;
> -
> - dp_display = g_dp_display;
> + struct msm_dp *dp_display = &dp->dp_display;
>
> if (!dp_display->power_on)
> return 0;
> @@ -1229,14 +1229,13 @@ static int dp_display_probe(struct platform_device *pdev)
> }
>
> mutex_init(&dp->event_mutex);
> - g_dp_display = &dp->dp_display;
>
> /* Store DP audio handle inside DP display */
> - g_dp_display->dp_audio = dp->audio;
> + dp->dp_display.dp_audio = dp->audio;
>
> init_completion(&dp->audio_comp);
>
> - platform_set_drvdata(pdev, g_dp_display);
> + platform_set_drvdata(pdev, &dp->dp_display);
>
> rc = component_add(&pdev->dev, &dp_display_comp_ops);
> if (rc) {
> @@ -1249,10 +1248,7 @@ static int dp_display_probe(struct platform_device *pdev)
>
> static int dp_display_remove(struct platform_device *pdev)
> {
> - struct dp_display_private *dp;
> -
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + struct dp_display_private *dp = platform_get_drvdata(pdev);
dev_to_dp_display_private() rather than just get_drvdata?
>
> dp_display_deinit_sub_modules(dp);
>
>
--
With best wishes
Dmitry
On 25/07/2021 07:24, Bjorn Andersson wrote:
> Functions in the DisplayPort code that relates to individual instances
> (encoders) are passed both the struct msm_dp and the struct drm_encoder. But
> in a situation where multiple DP instances would exist this means that
> the caller need to resolve which struct msm_dp relates to the struct
> drm_encoder at hand.
>
> The information for doing this lookup is available inside the DP driver,
> so update the API to take the struct msm_drm_private and the struct
> drm_encoder and have the DP code figure out which struct msm_dp the
> operation relates to.
Initially I thought to propose moving encoder->dp lookup into dpu code
by adding msm_dp_display_get_encoder() function. However as I was
writing that, I remembered that at some point I had to refactor my own
patchset in the way to get rid of calling msm_FOO_get_encoder().
I'd propose simpler solution. In dpu_encoder_setup() you have the DP
index and the encoder. So you can store valid msm_dp pointer in the
dpu_encoder_virt and remove all the lookups. Then you can replace all
priv->dp with bare dpu_enc->dp accesses. Will this work for you?
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 +++++----
> drivers/gpu/drm/msm/dp/dp_display.c | 38 +++++++++++++++++----
> drivers/gpu/drm/msm/msm_drv.h | 31 +++++++++--------
> 3 files changed, 56 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1c04b7cce43e..0d64ef0819af 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1002,8 +1002,8 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>
> trace_dpu_enc_mode_set(DRMID(drm_enc));
>
> - if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp)
> - msm_dp_display_mode_set(priv->dp, drm_enc, mode, adj_mode);
> + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS)
> + msm_dp_display_mode_set(priv, drm_enc, mode, adj_mode);
>
> list_for_each_entry(conn_iter, connector_list, head)
> if (conn_iter->encoder == drm_enc)
> @@ -1184,9 +1184,8 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
>
> _dpu_encoder_virt_enable_helper(drm_enc);
>
> - if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
> - ret = msm_dp_display_enable(priv->dp,
> - drm_enc);
> + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
> + ret = msm_dp_display_enable(priv, drm_enc);
> if (ret) {
> DPU_ERROR_ENC(dpu_enc, "dp display enable failed: %d\n",
> ret);
> @@ -1226,8 +1225,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
> /* wait for idle */
> dpu_encoder_wait_for_event(drm_enc, MSM_ENC_TX_COMPLETE);
>
> - if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
> - if (msm_dp_display_pre_disable(priv->dp, drm_enc))
> + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
> + if (msm_dp_display_pre_disable(priv, drm_enc))
> DPU_ERROR_ENC(dpu_enc, "dp display push idle failed\n");
> }
>
> @@ -1255,8 +1254,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
>
> DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
>
> - if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
> - if (msm_dp_display_disable(priv->dp, drm_enc))
> + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
> + if (msm_dp_display_disable(priv, drm_enc))
> DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
> }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 8696b36d30e4..59ffd6c8f41f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1432,12 +1432,25 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
> return 0;
> }
>
> -int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
> +static struct msm_dp *msm_dp_from_drm_encoder(struct msm_drm_private *priv,
> + struct drm_encoder *encoder)
> +{
> + if (priv->dp && priv->dp->encoder == encoder)
> + return priv->dp;
> +
> + return NULL;
> +}
> +
> +int msm_dp_display_enable(struct msm_drm_private *priv, struct drm_encoder *encoder)
> {
> int rc = 0;
> struct dp_display_private *dp_display;
> + struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
> u32 state;
>
> + if (!dp)
> + return -EINVAL;
> +
> dp_display = container_of(dp, struct dp_display_private, dp_display);
> if (!dp_display->dp_mode.drm_mode.clock) {
> DRM_ERROR("invalid params\n");
> @@ -1489,9 +1502,13 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
> return rc;
> }
>
> -int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder)
> +int msm_dp_display_pre_disable(struct msm_drm_private *priv, struct drm_encoder *encoder)
> {
> struct dp_display_private *dp_display;
> + struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
> +
> + if (!dp)
> + return 0;
>
> dp_display = container_of(dp, struct dp_display_private, dp_display);
>
> @@ -1500,11 +1517,15 @@ int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder)
> return 0;
> }
>
> -int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
> +int msm_dp_display_disable(struct msm_drm_private *priv, struct drm_encoder *encoder)
> {
> int rc = 0;
> u32 state;
> struct dp_display_private *dp_display;
> + struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
> +
> + if (!dp)
> + return 0;
>
> dp_display = container_of(dp, struct dp_display_private, dp_display);
>
> @@ -1531,11 +1552,16 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
> return rc;
> }
>
> -void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,
> - struct drm_display_mode *mode,
> - struct drm_display_mode *adjusted_mode)
> +void msm_dp_display_mode_set(struct msm_drm_private *priv,
> + struct drm_encoder *encoder,
> + struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode)
> {
> struct dp_display_private *dp_display;
> + struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
> +
> + if (!dp)
> + return;
>
> dp_display = container_of(dp, struct dp_display_private, dp_display);
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 9bfd37855969..e9232032b266 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -388,12 +388,13 @@ int __init msm_dp_register(void);
> void __exit msm_dp_unregister(void);
> int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
> struct drm_encoder *encoder);
> -int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder);
> -int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder);
> -int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder);
> -void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,
> - struct drm_display_mode *mode,
> - struct drm_display_mode *adjusted_mode);
> +int msm_dp_display_enable(struct msm_drm_private *priv, struct drm_encoder *encoder);
> +int msm_dp_display_disable(struct msm_drm_private *priv, struct drm_encoder *encoder);
> +int msm_dp_display_pre_disable(struct msm_drm_private *priv, struct drm_encoder *encoder);
> +void msm_dp_display_mode_set(struct msm_drm_private *priv,
> + struct drm_encoder *encoder,
> + struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode);
> 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);
>
> @@ -413,25 +414,25 @@ static inline int msm_dp_modeset_init(struct msm_dp *dp_display,
> {
> return -EINVAL;
> }
> -static inline int msm_dp_display_enable(struct msm_dp *dp,
> +static inline int msm_dp_display_enable(struct msm_drm_private *priv,
> struct drm_encoder *encoder)
> {
> return -EINVAL;
> }
> -static inline int msm_dp_display_disable(struct msm_dp *dp,
> - struct drm_encoder *encoder)
> +static inline int msm_dp_display_disable(struct msm_drm_private *priv,
> + struct drm_encoder *encoder)
> {
> return -EINVAL;
> }
> -static inline int msm_dp_display_pre_disable(struct msm_dp *dp,
> - struct drm_encoder *encoder)
> +static inline int msm_dp_display_pre_disable(struct msm_drm_private *priv,
> + struct drm_encoder *encoder)
> {
> return -EINVAL;
> }
> -static inline void msm_dp_display_mode_set(struct msm_dp *dp,
> - struct drm_encoder *encoder,
> - struct drm_display_mode *mode,
> - struct drm_display_mode *adjusted_mode)
> +static inline void msm_dp_display_mode_set(struct msm_drm_private *priv,
> + struct drm_encoder *encoder,
> + struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode)
> {
> }
>
>
--
With best wishes
Dmitry
On 2021-07-24 21:24, Bjorn Andersson wrote:
> As the Qualcomm DisplayPort driver only supports a single instance of
> the driver the commonly used struct dp_display is kept in a global
> variable. As we introduce additional instances this obviously doesn't
> work.
>
> Replace this with a combination of existing references to adjacent
> objects and drvdata.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 78 ++++++++++++++---------------
> 1 file changed, 37 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 70b319a8fe83..8696b36d30e4 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -27,7 +27,6 @@
> #include "dp_audio.h"
> #include "dp_debug.h"
>
> -static struct msm_dp *g_dp_display;
> #define HPD_STRING_SIZE 30
>
> enum {
> @@ -122,6 +121,13 @@ static const struct of_device_id dp_dt_match[] = {
> {}
> };
>
> +static struct dp_display_private *dev_to_dp_display_private(struct
> device *dev)
> +{
> + struct msm_dp *dp = dev_get_drvdata(dev);
> +
> + return container_of(dp, struct dp_display_private, dp_display);
> +}
> +
> static int dp_add_event(struct dp_display_private *dp_priv, u32 event,
> u32 data, u32 delay)
> {
> @@ -198,14 +204,16 @@ static int dp_display_bind(struct device *dev,
> struct device *master,
> void *data)
> {
> int rc = 0;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
> struct drm_device *drm;
> struct msm_drm_private *priv;
>
> drm = dev_get_drvdata(master);
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + if (!dp) {
> + DRM_ERROR("DP driver bind failed. Invalid driver data\n");
> + return -EINVAL;
> + }
>
> dp->dp_display.drm_dev = drm;
> priv = drm->dev_private;
> @@ -240,12 +248,14 @@ static int dp_display_bind(struct device *dev,
> struct device *master,
> static void dp_display_unbind(struct device *dev, struct device
> *master,
> void *data)
> {
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
> struct drm_device *drm = dev_get_drvdata(master);
> struct msm_drm_private *priv = drm->dev_private;
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + if (!dp) {
> + DRM_ERROR("Invalid DP driver data\n");
> + return;
> + }
>
> dp_power_client_deinit(dp->power);
> dp_aux_unregister(dp->aux);
> @@ -376,17 +386,14 @@ static void dp_display_host_deinit(struct
> dp_display_private *dp)
> static int dp_display_usbpd_configure_cb(struct device *dev)
> {
> int rc = 0;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
>
> - if (!dev) {
> - DRM_ERROR("invalid dev\n");
> - rc = -EINVAL;
> + if (!dp) {
> + DRM_ERROR("no driver data found\n");
> + rc = -ENODEV;
> goto end;
> }
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> -
> dp_display_host_init(dp, false);
>
> rc = dp_display_process_hpd_high(dp);
> @@ -397,17 +404,14 @@ static int dp_display_usbpd_configure_cb(struct
> device *dev)
> static int dp_display_usbpd_disconnect_cb(struct device *dev)
> {
> int rc = 0;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
>
> - if (!dev) {
> - DRM_ERROR("invalid dev\n");
> - rc = -EINVAL;
> + if (!dp) {
> + DRM_ERROR("no driver data found\n");
> + rc = -ENODEV;
> return rc;
> }
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> -
> dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
>
> return rc;
> @@ -466,15 +470,15 @@ static int dp_display_usbpd_attention_cb(struct
> device *dev)
> {
> int rc = 0;
> u32 sink_request;
> - struct dp_display_private *dp;
> + struct dp_display_private *dp = dev_to_dp_display_private(dev);
> + struct dp_usbpd *hpd;
>
> - if (!dev) {
> - DRM_ERROR("invalid dev\n");
> - return -EINVAL;
> + if (!dp) {
> + DRM_ERROR("no driver data found\n");
> + return -ENODEV;
> }
>
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + hpd = dp->usbpd;
hpd is unused here. It was removed with
https://patches.linaro.org/patch/416670/
>
> /* check for any test request issued by sink */
> rc = dp_link_process_request(dp->link);
> @@ -638,7 +642,7 @@ static int dp_hpd_unplug_handle(struct
> dp_display_private *dp, u32 data)
> dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0,
> DP_TIMEOUT_5_SECOND);
>
> /* signal the disconnect event early to ensure proper teardown */
> - dp_display_handle_plugged_change(g_dp_display, false);
> + dp_display_handle_plugged_change(&dp->dp_display, false);
>
> /* enable HDP plug interrupt to prepare for next plugin */
> dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK,
> true);
> @@ -832,9 +836,7 @@ static int dp_display_prepare(struct msm_dp *dp)
> static int dp_display_enable(struct dp_display_private *dp, u32 data)
> {
> int rc = 0;
> - struct msm_dp *dp_display;
> -
> - dp_display = g_dp_display;
> + struct msm_dp *dp_display = &dp->dp_display;
>
> if (dp_display->power_on) {
> DRM_DEBUG_DP("Link already setup, return\n");
> @@ -869,9 +871,7 @@ static int dp_display_post_enable(struct msm_dp
> *dp_display)
>
> static int dp_display_disable(struct dp_display_private *dp, u32 data)
> {
> - struct msm_dp *dp_display;
> -
> - dp_display = g_dp_display;
> + struct msm_dp *dp_display = &dp->dp_display;
>
> if (!dp_display->power_on)
> return 0;
> @@ -1229,14 +1229,13 @@ static int dp_display_probe(struct
> platform_device *pdev)
> }
>
> mutex_init(&dp->event_mutex);
> - g_dp_display = &dp->dp_display;
>
> /* Store DP audio handle inside DP display */
> - g_dp_display->dp_audio = dp->audio;
> + dp->dp_display.dp_audio = dp->audio;
>
> init_completion(&dp->audio_comp);
>
> - platform_set_drvdata(pdev, g_dp_display);
> + platform_set_drvdata(pdev, &dp->dp_display);
>
> rc = component_add(&pdev->dev, &dp_display_comp_ops);
> if (rc) {
> @@ -1249,10 +1248,7 @@ static int dp_display_probe(struct
> platform_device *pdev)
>
> static int dp_display_remove(struct platform_device *pdev)
> {
> - struct dp_display_private *dp;
> -
> - dp = container_of(g_dp_display,
> - struct dp_display_private, dp_display);
> + struct dp_display_private *dp = platform_get_drvdata(pdev);
>
> dp_display_deinit_sub_modules(dp);
Hi Bjorn
On 2021-07-24 21:24, Bjorn Andersson wrote:
> The current implementation supports a single DP instance and the DPU
> code will
> only match it against INTF_DP instance 0. These patches extends this to
> allow
> multiple DP instances and support for matching against DP instances
> beyond 0.
>
> This is based on v4 of Dmitry's work on multiple DSI interfaces:
> https://lore.kernel.org/linux-arm-msm/[email protected]/
>
> With that in place add SC8180x DP and eDP controllers.
Thanks for posting the changes.
I dont have major concerns on the series as such apart from minor
comments which i will post in a day or two
but I will check and get back if this has been validated on sc7280
without any concerns.
One question i had is not directly related to this series but related to
multi-DP in general.
Does audio work fine across both the DPs when both are connected?
The reason I ask this question is that, I dont know how two hdmi-codec
instances are handled today.
So we will register twice with hdmi-codec so there should be two audio
streams.
But I am not sure if this works correctly in todays design with
hdmi-codec.
Any chance you had validated this?
>
> Bjorn Andersson (5):
> drm/msm/dp: Remove global g_dp_display variable
> drm/msm/dp: Modify prototype of encoder based API
> drm/msm/dp: Support up to 3 DP controllers
> dt-bindings: msm/dp: Add SC8180x compatibles
> drm/msm/dp: Add sc8180x DP controllers
>
> .../bindings/display/msm/dp-controller.yaml | 2 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 60 +++---
> .../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 8 +-
> drivers/gpu/drm/msm/dp/dp_display.c | 183 +++++++++++++-----
> drivers/gpu/drm/msm/msm_drv.h | 33 ++--
> 6 files changed, 200 insertions(+), 103 deletions(-)