* [PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss
@ 2014-11-14 22:42 Hai Li
2014-11-14 22:42 ` [PATCH 2/2] drm/msm: Decouple hdmi driver from mdp driver Hai Li
2014-11-16 14:18 ` Rob Clark
0 siblings, 2 replies; 8+ messages in thread
From: Hai Li @ 2014-11-14 22:42 UTC (permalink / raw)
To: dri-devel; +Cc: linux-arm-msm, linux-kernel, robdclark, Hai Li
All the sub-systems in mdss share the same irq. This change provides
the sub-systems with the interfaces to register/unregister their own
irq handlers.
With this change, struct mdp5_kms does not have to keep the hdmi or
edp context.
Signed-off-by: Hai Li <hali@codeaurora.org>
---
drivers/gpu/drm/msm/hdmi/hdmi.c | 12 +++-
drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c | 107 ++++++++++++++++++++++++++++++--
drivers/gpu/drm/msm/msm_drv.h | 19 +++++-
3 files changed, 130 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 9d00dcb..aaf5e2b 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -39,7 +39,7 @@ void hdmi_set_mode(struct hdmi *hdmi, bool power_on)
power_on ? "Enable" : "Disable", ctrl);
}
-irqreturn_t hdmi_irq(int irq, void *dev_id)
+static irqreturn_t hdmi_irq(int irq, void *dev_id)
{
struct hdmi *hdmi = dev_id;
@@ -59,6 +59,9 @@ void hdmi_destroy(struct kref *kref)
struct hdmi *hdmi = container_of(kref, struct hdmi, refcount);
struct hdmi_phy *phy = hdmi->phy;
+ if (hdmi->config->shared_irq)
+ msm_shared_irq_unregister(MSM_SUBSYS_HDMI);
+
if (phy)
phy->funcs->destroy(phy);
@@ -221,6 +224,13 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
hdmi->irq, ret);
goto fail;
}
+ } else {
+ ret = msm_shared_irq_register(MSM_SUBSYS_HDMI, hdmi_irq, hdmi);
+ if (ret < 0) {
+ dev_err(dev->dev, "failed to register shared IRQ: %d\n",
+ ret);
+ goto fail;
+ }
}
encoder->bridge = hdmi->bridge;
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
index f2b985b..2973c1c 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
@@ -1,4 +1,5 @@
/*
+ * Copyright (c) 2014, The Linux Foundation. All rights reserved.
* Copyright (C) 2013 Red Hat
* Author: Rob Clark <robdclark@gmail.com>
*
@@ -19,6 +20,75 @@
#include "msm_drv.h"
#include "mdp5_kms.h"
+struct msm_subsys_shared_irq {
+ u32 mask;
+ u32 count;
+
+ /* Filled by sub system */
+ irqreturn_t (*handler)(int irq, void *dev_id);
+ void *data;
+};
+
+static struct msm_subsys_shared_irq msm_shared_irqs[MSM_SUBSYS_COUNT] = {
+ [MSM_SUBSYS_MDP] = {.mask = MDP5_HW_INTR_STATUS_INTR_MDP,
+ .count = 0},
+ [MSM_SUBSYS_DSI_0] = {.mask = MDP5_HW_INTR_STATUS_INTR_DSI0,
+ .count = 0},
+ [MSM_SUBSYS_DSI_1] = {.mask = MDP5_HW_INTR_STATUS_INTR_DSI1,
+ .count = 0},
+ [MSM_SUBSYS_HDMI] = {.mask = MDP5_HW_INTR_STATUS_INTR_HDMI,
+ .count = 0},
+ [MSM_SUBSYS_EDP] = {.mask = MDP5_HW_INTR_STATUS_INTR_EDP,
+ .count = 0},
+};
+
+static irqreturn_t mdp5_irq_mdp(int irq, void *dev_id);
+
+int msm_shared_irq_register(enum msm_sub_system sys_id,
+ irqreturn_t (*handler)(int irq, void *dev_id), void *data)
+{
+ if (sys_id >= MSM_SUBSYS_COUNT) {
+ DRM_ERROR("Invalid sys_id %d", sys_id);
+ return -EINVAL;
+ }
+
+ if (msm_shared_irqs[sys_id].handler != NULL) {
+ DRM_ERROR("sys %d irq already registered", sys_id);
+ return -EBUSY;
+ }
+
+ msm_shared_irqs[sys_id].data = data;
+ msm_shared_irqs[sys_id].handler = handler;
+
+ return 0;
+}
+
+/*
+ * This function should be called after the interrupt
+ * on the sub-system is disabled.
+ */
+int msm_shared_irq_unregister(enum msm_sub_system sys_id)
+{
+ if (sys_id >= MSM_SUBSYS_COUNT) {
+ DRM_ERROR("Invalid sys_id %d", sys_id);
+ return -EINVAL;
+ }
+
+ msm_shared_irqs[sys_id].handler = NULL;
+ msm_shared_irqs[sys_id].data = NULL;
+
+ /*
+ * Make sure irq_handler and data is invalid.
+ * Then we only need to wait until the last pending interrupt is done.
+ */
+ wmb();
+
+ while (msm_shared_irqs[sys_id].count & 0x1)
+ usleep_range(100, 1000);
+
+ return 0;
+}
+
void mdp5_set_irqmask(struct mdp_kms *mdp_kms, uint32_t irqmask)
{
mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_INTR_EN, irqmask);
@@ -47,6 +117,9 @@ int mdp5_irq_postinstall(struct msm_kms *kms)
MDP5_IRQ_INTF2_UNDER_RUN |
MDP5_IRQ_INTF3_UNDER_RUN;
+ /* Register mdp irq to mdss */
+ msm_shared_irq_register(MSM_SUBSYS_MDP, mdp5_irq_mdp, mdp_kms);
+
mdp_irq_register(mdp_kms, error_handler);
return 0;
@@ -56,10 +129,15 @@ void mdp5_irq_uninstall(struct msm_kms *kms)
{
struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
mdp5_write(mdp5_kms, REG_MDP5_INTR_EN, 0x00000000);
+
+ /* Make sure interrupt is disabled before remove irq. */
+ wmb();
+ msm_shared_irq_unregister(MSM_SUBSYS_MDP);
}
-static void mdp5_irq_mdp(struct mdp_kms *mdp_kms)
+static irqreturn_t mdp5_irq_mdp(int irq, void *dev_id)
{
+ struct mdp_kms *mdp_kms = (struct mdp_kms *)dev_id;
struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
struct drm_device *dev = mdp5_kms->dev;
struct msm_drm_private *priv = dev->dev_private;
@@ -76,23 +154,40 @@ static void mdp5_irq_mdp(struct mdp_kms *mdp_kms)
for (id = 0; id < priv->num_crtcs; id++)
if (status & mdp5_crtc_vblank(priv->crtcs[id]))
drm_handle_vblank(dev, id);
+
+ return IRQ_HANDLED;
}
irqreturn_t mdp5_irq(struct msm_kms *kms)
{
struct mdp_kms *mdp_kms = to_mdp_kms(kms);
struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
+ struct msm_subsys_shared_irq *irq;
uint32_t intr;
+ int i;
intr = mdp5_read(mdp5_kms, REG_MDP5_HW_INTR_STATUS);
VERB("intr=%08x", intr);
- if (intr & MDP5_HW_INTR_STATUS_INTR_MDP)
- mdp5_irq_mdp(mdp_kms);
-
- if (intr & MDP5_HW_INTR_STATUS_INTR_HDMI)
- hdmi_irq(0, mdp5_kms->hdmi);
+ for (i = 0; i < MSM_SUBSYS_COUNT; i++) {
+ irq = &msm_shared_irqs[i];
+ if (intr & irq->mask) {
+ irq->count++;
+
+ /*
+ * These 2 wmb() ensure count is odd number
+ * during handler is running.
+ */
+ wmb();
+ if ((irq->handler != NULL) && (irq->data != NULL))
+ irq->handler(0, irq->data);
+
+ /* Make sure count increments after handler is done */
+ wmb();
+ irq->count++;
+ }
+ }
return IRQ_HANDLED;
}
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 67f9d0a..718ac55 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -127,6 +127,16 @@ struct msm_drm_private {
} vram;
};
+/* For mdp5 only */
+enum msm_sub_system {
+ MSM_SUBSYS_MDP = 0,
+ MSM_SUBSYS_DSI_0,
+ MSM_SUBSYS_DSI_1,
+ MSM_SUBSYS_HDMI,
+ MSM_SUBSYS_EDP,
+ MSM_SUBSYS_COUNT
+};
+
struct msm_format {
uint32_t pixel_format;
};
@@ -145,6 +155,14 @@ void __msm_fence_worker(struct work_struct *work);
(_cb)->func = _func; \
} while (0)
+/*
+ * For mdp5 only, callers should call these 2 functions
+ * only if the irqs are shared with others.
+ */
+int msm_shared_irq_register(enum msm_sub_system sys_id,
+ irqreturn_t (*handler)(int irq, void *dev_id), void *data);
+int msm_shared_irq_unregister(enum msm_sub_system sys_id);
+
int msm_register_mmu(struct drm_device *dev, struct msm_mmu *mmu);
int msm_wait_fence_interruptable(struct drm_device *dev, uint32_t fence,
@@ -203,7 +221,6 @@ struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev);
struct hdmi;
struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder);
-irqreturn_t hdmi_irq(int irq, void *dev_id);
void __init hdmi_register(void);
void __exit hdmi_unregister(void);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] drm/msm: Decouple hdmi driver from mdp driver
2014-11-14 22:42 [PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss Hai Li
@ 2014-11-14 22:42 ` Hai Li
2014-11-16 14:35 ` Rob Clark
2014-11-16 14:18 ` Rob Clark
1 sibling, 1 reply; 8+ messages in thread
From: Hai Li @ 2014-11-14 22:42 UTC (permalink / raw)
To: dri-devel; +Cc: linux-arm-msm, linux-kernel, robdclark, Hai Li
This change is to remove the hdmi structure from mdp kms data structure.
To do this, the initialization flow is re-arranged.
- hdmi_init is moved from modeset_init to hdmi_bind.
- hdmi_destroy is called by hdmi_unbind and the use of kref is abandoned.
- A new interface hdmi_set_encoder is introduced to establish the links
between hdmi connector, encoder and bridge.
Signed-off-by: Hai Li <hali@codeaurora.org>
---
drivers/gpu/drm/msm/hdmi/hdmi.c | 61 +++++++++++++++++++++++--------
drivers/gpu/drm/msm/hdmi/hdmi.h | 14 -------
drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 3 +-
drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 6 +--
drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 14 ++++---
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 38 ++++++++++---------
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 2 -
drivers/gpu/drm/msm/msm_drv.c | 9 ++++-
drivers/gpu/drm/msm/msm_drv.h | 6 ++-
9 files changed, 88 insertions(+), 65 deletions(-)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index aaf5e2b..2d2551f 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -1,4 +1,5 @@
/*
+ * Copyright (c) 2014, The Linux Foundation. All rights reserved.
* Copyright (C) 2013 Red Hat
* Author: Rob Clark <robdclark@gmail.com>
*
@@ -17,6 +18,29 @@
#include "hdmi.h"
+int hdmi_set_encoder(struct platform_device *pdev,
+ struct drm_encoder *encoder)
+{
+ struct hdmi *hdmi = platform_get_drvdata(pdev);
+
+ if (!encoder && !hdmi->encoder) {
+ pr_err("%s:wrong encoder status,encoder=%p,hdmi->encoder=%p\n",
+ __func__, encoder, hdmi->encoder);
+ return -EINVAL;
+ }
+
+ /* Each connector has only one available encoder for now. */
+ hdmi->connector->encoder_ids[0] = 0;
+ hdmi->connector->encoder = NULL;
+ if (encoder) {
+ drm_mode_connector_attach_encoder(hdmi->connector, encoder);
+ encoder->bridge = hdmi->bridge;
+ }
+ hdmi->encoder = encoder;
+
+ return 0;
+}
+
void hdmi_set_mode(struct hdmi *hdmi, bool power_on)
{
uint32_t ctrl = 0;
@@ -54,10 +78,15 @@ static irqreturn_t hdmi_irq(int irq, void *dev_id)
return IRQ_HANDLED;
}
-void hdmi_destroy(struct kref *kref)
+static void hdmi_destroy(struct platform_device *pdev)
{
- struct hdmi *hdmi = container_of(kref, struct hdmi, refcount);
- struct hdmi_phy *phy = hdmi->phy;
+ struct hdmi *hdmi = platform_get_drvdata(pdev);
+ struct hdmi_phy *phy;
+
+ if (!hdmi)
+ return;
+
+ phy = hdmi->phy;
if (hdmi->config->shared_irq)
msm_shared_irq_unregister(MSM_SUBSYS_HDMI);
@@ -68,15 +97,14 @@ void hdmi_destroy(struct kref *kref)
if (hdmi->i2c)
hdmi_i2c_destroy(hdmi->i2c);
- platform_set_drvdata(hdmi->pdev, NULL);
+ platform_set_drvdata(pdev, NULL);
}
/* initialize connector */
-struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
+static int hdmi_init(struct platform_device *pdev, struct drm_device *dev)
{
struct hdmi *hdmi = NULL;
struct msm_drm_private *priv = dev->dev_private;
- struct platform_device *pdev = priv->hdmi_pdev;
struct hdmi_platform_config *config;
int i, ret;
@@ -94,12 +122,11 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
goto fail;
}
- kref_init(&hdmi->refcount);
+ platform_set_drvdata(pdev, hdmi);
hdmi->dev = dev;
hdmi->pdev = pdev;
hdmi->config = config;
- hdmi->encoder = encoder;
hdmi_audio_infoframe_init(&hdmi->audio.infoframe);
@@ -233,14 +260,10 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
}
}
- encoder->bridge = hdmi->bridge;
-
priv->bridges[priv->num_bridges++] = hdmi->bridge;
priv->connectors[priv->num_connectors++] = hdmi->connector;
- platform_set_drvdata(pdev, hdmi);
-
- return hdmi;
+ return 0;
fail:
if (hdmi) {
@@ -249,10 +272,10 @@ fail:
hdmi->bridge->funcs->destroy(hdmi->bridge);
if (hdmi->connector)
hdmi->connector->funcs->destroy(hdmi->connector);
- hdmi_destroy(&hdmi->refcount);
+ hdmi_destroy(pdev);
}
- return ERR_PTR(ret);
+ return ret;
}
/*
@@ -289,6 +312,7 @@ static int get_gpio(struct device *dev, struct device_node *of_node, const char
static int hdmi_bind(struct device *dev, struct device *master, void *data)
{
static struct hdmi_platform_config config = {};
+ int ret;
#ifdef CONFIG_OF
struct device_node *of_node = dev->of_node;
@@ -379,6 +403,12 @@ static int hdmi_bind(struct device *dev, struct device *master, void *data)
}
#endif
dev->platform_data = &config;
+
+ ret = hdmi_init(to_platform_device(dev), dev_get_drvdata(master));
+ if (ret) {
+ dev_err(dev, "%s: hdmi_init fail, %d\n", __func__, ret);
+ return ret;
+ }
set_hdmi_pdev(dev_get_drvdata(master), to_platform_device(dev));
return 0;
}
@@ -387,6 +417,7 @@ static void hdmi_unbind(struct device *dev, struct device *master,
void *data)
{
set_hdmi_pdev(dev_get_drvdata(master), NULL);
+ hdmi_destroy(to_platform_device(dev));
}
static const struct component_ops hdmi_ops = {
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index b981995..a78dd8d 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -38,8 +38,6 @@ struct hdmi_audio {
};
struct hdmi {
- struct kref refcount;
-
struct drm_device *dev;
struct platform_device *pdev;
@@ -103,7 +101,6 @@ struct hdmi_platform_config {
};
void hdmi_set_mode(struct hdmi *hdmi, bool power_on);
-void hdmi_destroy(struct kref *kref);
static inline void hdmi_write(struct hdmi *hdmi, u32 reg, u32 data)
{
@@ -115,17 +112,6 @@ static inline u32 hdmi_read(struct hdmi *hdmi, u32 reg)
return msm_readl(hdmi->mmio + reg);
}
-static inline struct hdmi * hdmi_reference(struct hdmi *hdmi)
-{
- kref_get(&hdmi->refcount);
- return hdmi;
-}
-
-static inline void hdmi_unreference(struct hdmi *hdmi)
-{
- kref_put(&hdmi->refcount, hdmi_destroy);
-}
-
/*
* The phy appears to be different, for example between 8960 and 8x60,
* so split the phy related functions out and load the correct one at
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index f6cf745..6902ad6 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -26,7 +26,6 @@ struct hdmi_bridge {
static void hdmi_bridge_destroy(struct drm_bridge *bridge)
{
struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
- hdmi_unreference(hdmi_bridge->hdmi);
drm_bridge_cleanup(bridge);
kfree(hdmi_bridge);
}
@@ -218,7 +217,7 @@ struct drm_bridge *hdmi_bridge_init(struct hdmi *hdmi)
goto fail;
}
- hdmi_bridge->hdmi = hdmi_reference(hdmi);
+ hdmi_bridge->hdmi = hdmi;
bridge = &hdmi_bridge->base;
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
index 4aca2a3..f5da877 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
@@ -330,8 +330,6 @@ static void hdmi_connector_destroy(struct drm_connector *connector)
drm_connector_unregister(connector);
drm_connector_cleanup(connector);
- hdmi_unreference(hdmi_connector->hdmi);
-
kfree(hdmi_connector);
}
@@ -422,7 +420,7 @@ struct drm_connector *hdmi_connector_init(struct hdmi *hdmi)
goto fail;
}
- hdmi_connector->hdmi = hdmi_reference(hdmi);
+ hdmi_connector->hdmi = hdmi;
INIT_WORK(&hdmi_connector->hpd_work, hotplug_work);
connector = &hdmi_connector->base;
@@ -445,8 +443,6 @@ struct drm_connector *hdmi_connector_init(struct hdmi *hdmi)
goto fail;
}
- drm_mode_connector_attach_encoder(connector, hdmi->encoder);
-
return connector;
fail:
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
index 79d804e..b0b6dee 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
@@ -228,7 +228,6 @@ static int modeset_init(struct mdp4_kms *mdp4_kms)
struct drm_encoder *encoder;
struct drm_connector *connector;
struct drm_panel *panel;
- struct hdmi *hdmi;
int ret;
/* construct non-private planes: */
@@ -326,15 +325,18 @@ static int modeset_init(struct mdp4_kms *mdp4_kms)
priv->crtcs[priv->num_crtcs++] = crtc;
priv->encoders[priv->num_encoders++] = encoder;
- hdmi = hdmi_init(dev, encoder);
- if (IS_ERR(hdmi)) {
- ret = PTR_ERR(hdmi);
- dev_err(dev->dev, "failed to initialize HDMI: %d\n", ret);
- goto fail;
+ ret = hdmi_set_encoder(priv->hdmi_pdev, encoder);
+ if (ret) {
+ dev_err(dev->dev, "failed to set encoder\n");
+ goto fail1;
}
return 0;
+fail1:
+ priv->encoders[--priv->num_encoders] = NULL;
+ if (encoder)
+ encoder->funcs->destroy(encoder);
fail:
return ret;
}
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
index 31a2c63..1bb3a28 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
@@ -302,14 +302,6 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
priv->crtcs[priv->num_crtcs++] = crtc;
}
- /* Construct encoder for HDMI: */
- encoder = mdp5_encoder_init(dev, 3, INTF_HDMI);
- if (IS_ERR(encoder)) {
- dev_err(dev->dev, "failed to construct encoder\n");
- ret = PTR_ERR(encoder);
- goto fail;
- }
-
/* NOTE: the vsync and error irq's are actually associated with
* the INTF/encoder.. the easiest way to deal with this (ie. what
* we do now) is assume a fixed relationship between crtc's and
@@ -318,21 +310,33 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
* care of error and vblank irq's that the crtc has registered,
* and also update user-requested vblank_mask.
*/
- encoder->possible_crtcs = BIT(0);
- mdp5_crtc_set_intf(priv->crtcs[0], 3, INTF_HDMI);
+ if (priv->hdmi_pdev) {
+ /* Construct encoder for HDMI: */
+ encoder = mdp5_encoder_init(dev, 3, INTF_HDMI);
+ if (IS_ERR(encoder)) {
+ dev_err(dev->dev, "failed to construct encoder\n");
+ ret = PTR_ERR(encoder);
+ goto fail;
+ }
- priv->encoders[priv->num_encoders++] = encoder;
+ encoder->possible_crtcs = BIT(0);
+ mdp5_crtc_set_intf(priv->crtcs[0], 3, INTF_HDMI);
- /* Construct bridge/connector for HDMI: */
- mdp5_kms->hdmi = hdmi_init(dev, encoder);
- if (IS_ERR(mdp5_kms->hdmi)) {
- ret = PTR_ERR(mdp5_kms->hdmi);
- dev_err(dev->dev, "failed to initialize HDMI: %d\n", ret);
- goto fail;
+ priv->encoders[priv->num_encoders++] = encoder;
+
+ ret = hdmi_set_encoder(priv->hdmi_pdev, encoder);
+ if (ret)
+ goto fail1;
}
return 0;
+fail1:
+ if (priv->hdmi_pdev) {
+ priv->encoders[--priv->num_encoders] = NULL;
+ if (encoder)
+ encoder->funcs->destroy(encoder);
+ }
fail:
return ret;
}
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
index 5bf340d..c91101d 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
@@ -71,8 +71,6 @@ struct mdp5_kms {
struct clk *lut_clk;
struct clk *vsync_clk;
- struct hdmi *hdmi;
-
struct mdp_irq error_handler;
};
#define to_mdp5_kms(x) container_of(x, struct mdp5_kms, base)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index b67ef59..c27cb9a 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -147,7 +147,10 @@ static int msm_unload(struct drm_device *dev)
priv->vram.paddr, &attrs);
}
- component_unbind_all(dev->dev, dev);
+ if (priv->component_bound) {
+ component_unbind_all(dev->dev, dev);
+ priv->component_bound = false;
+ }
dev->dev_private = NULL;
@@ -237,7 +240,9 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
/* Bind all our sub-components: */
ret = component_bind_all(dev->dev, dev);
if (ret)
- return ret;
+ goto fail;
+
+ priv->component_bound = true;
switch (get_mdp_ver(pdev)) {
case 4:
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 718ac55..76cac63 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -125,6 +125,8 @@ struct msm_drm_private {
*/
struct drm_mm mm;
} vram;
+
+ bool component_bound;
};
/* For mdp5 only */
@@ -219,10 +221,10 @@ struct drm_framebuffer *msm_framebuffer_create(struct drm_device *dev,
struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev);
-struct hdmi;
-struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder);
void __init hdmi_register(void);
void __exit hdmi_unregister(void);
+int hdmi_set_encoder(struct platform_device *pdev,
+ struct drm_encoder *encoder);
#ifdef CONFIG_DEBUG_FS
void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss
2014-11-14 22:42 [PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss Hai Li
@ 2014-11-16 14:18 ` Rob Clark
2014-11-16 14:18 ` Rob Clark
1 sibling, 0 replies; 8+ messages in thread
From: Rob Clark @ 2014-11-16 14:18 UTC (permalink / raw)
To: Hai Li; +Cc: linux-arm-msm, Linux Kernel Mailing List, dri-devel
On Fri, Nov 14, 2014 at 5:42 PM, Hai Li <hali@codeaurora.org> wrote:
> All the sub-systems in mdss share the same irq. This change provides
> the sub-systems with the interfaces to register/unregister their own
> irq handlers.
>
> With this change, struct mdp5_kms does not have to keep the hdmi or
> edp context.
>
So, I think the point of this is to better deal w/ different hw
variants which do or do-not have hdmi, edp, dsi, etc..
But, just playing devil's advocate here, it seems like it would be
simpler to instead just do something like:
if (priv->hdmi && (intr & MDP5_HW_INTR_STATUS_INTR_HDMI))
hdmi_irq(0, priv->hdmi);
if (priv->edp && (intr & MDP5_HW_INTR_STATUS_INTR_EDP))
edp_irq(0, priv->edp);
... etc ...
It is a little less elegant. But it is also less lines of code. I
guess there will be plenty of necessary complexity as we add support
for all mdp5 features. So avoiding some unnecessary complexity might
be a good thing in the long run.
If we really did want to make it more dynamic, we could always extend
'struct mdp_irq' to take both an irq mask and an initiator id, I
suppose. I'm not sure if that is overkill. AFAICT we really only
have 5 different subsystems to dispatch to (mdp5 itself, and
dsi0/dsi1/hdmi/edp), so simply adding some if-not-null checks in
mdp5_irq() seems pretty reasonable to me.
BR,
-R
> Signed-off-by: Hai Li <hali@codeaurora.org>
> ---
> drivers/gpu/drm/msm/hdmi/hdmi.c | 12 +++-
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c | 107 ++++++++++++++++++++++++++++++--
> drivers/gpu/drm/msm/msm_drv.h | 19 +++++-
> 3 files changed, 130 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 9d00dcb..aaf5e2b 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -39,7 +39,7 @@ void hdmi_set_mode(struct hdmi *hdmi, bool power_on)
> power_on ? "Enable" : "Disable", ctrl);
> }
>
> -irqreturn_t hdmi_irq(int irq, void *dev_id)
> +static irqreturn_t hdmi_irq(int irq, void *dev_id)
> {
> struct hdmi *hdmi = dev_id;
>
> @@ -59,6 +59,9 @@ void hdmi_destroy(struct kref *kref)
> struct hdmi *hdmi = container_of(kref, struct hdmi, refcount);
> struct hdmi_phy *phy = hdmi->phy;
>
> + if (hdmi->config->shared_irq)
> + msm_shared_irq_unregister(MSM_SUBSYS_HDMI);
> +
> if (phy)
> phy->funcs->destroy(phy);
>
> @@ -221,6 +224,13 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
> hdmi->irq, ret);
> goto fail;
> }
> + } else {
> + ret = msm_shared_irq_register(MSM_SUBSYS_HDMI, hdmi_irq, hdmi);
> + if (ret < 0) {
> + dev_err(dev->dev, "failed to register shared IRQ: %d\n",
> + ret);
> + goto fail;
> + }
> }
>
> encoder->bridge = hdmi->bridge;
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
> index f2b985b..2973c1c 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
> @@ -1,4 +1,5 @@
> /*
> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> * Copyright (C) 2013 Red Hat
> * Author: Rob Clark <robdclark@gmail.com>
> *
> @@ -19,6 +20,75 @@
> #include "msm_drv.h"
> #include "mdp5_kms.h"
>
> +struct msm_subsys_shared_irq {
> + u32 mask;
> + u32 count;
> +
> + /* Filled by sub system */
> + irqreturn_t (*handler)(int irq, void *dev_id);
> + void *data;
> +};
> +
> +static struct msm_subsys_shared_irq msm_shared_irqs[MSM_SUBSYS_COUNT] = {
> + [MSM_SUBSYS_MDP] = {.mask = MDP5_HW_INTR_STATUS_INTR_MDP,
> + .count = 0},
> + [MSM_SUBSYS_DSI_0] = {.mask = MDP5_HW_INTR_STATUS_INTR_DSI0,
> + .count = 0},
> + [MSM_SUBSYS_DSI_1] = {.mask = MDP5_HW_INTR_STATUS_INTR_DSI1,
> + .count = 0},
> + [MSM_SUBSYS_HDMI] = {.mask = MDP5_HW_INTR_STATUS_INTR_HDMI,
> + .count = 0},
> + [MSM_SUBSYS_EDP] = {.mask = MDP5_HW_INTR_STATUS_INTR_EDP,
> + .count = 0},
> +};
> +
> +static irqreturn_t mdp5_irq_mdp(int irq, void *dev_id);
> +
> +int msm_shared_irq_register(enum msm_sub_system sys_id,
> + irqreturn_t (*handler)(int irq, void *dev_id), void *data)
> +{
> + if (sys_id >= MSM_SUBSYS_COUNT) {
> + DRM_ERROR("Invalid sys_id %d", sys_id);
> + return -EINVAL;
> + }
> +
> + if (msm_shared_irqs[sys_id].handler != NULL) {
> + DRM_ERROR("sys %d irq already registered", sys_id);
> + return -EBUSY;
> + }
> +
> + msm_shared_irqs[sys_id].data = data;
> + msm_shared_irqs[sys_id].handler = handler;
> +
> + return 0;
> +}
> +
> +/*
> + * This function should be called after the interrupt
> + * on the sub-system is disabled.
> + */
> +int msm_shared_irq_unregister(enum msm_sub_system sys_id)
> +{
> + if (sys_id >= MSM_SUBSYS_COUNT) {
> + DRM_ERROR("Invalid sys_id %d", sys_id);
> + return -EINVAL;
> + }
> +
> + msm_shared_irqs[sys_id].handler = NULL;
> + msm_shared_irqs[sys_id].data = NULL;
> +
> + /*
> + * Make sure irq_handler and data is invalid.
> + * Then we only need to wait until the last pending interrupt is done.
> + */
> + wmb();
> +
> + while (msm_shared_irqs[sys_id].count & 0x1)
> + usleep_range(100, 1000);
> +
> + return 0;
> +}
> +
> void mdp5_set_irqmask(struct mdp_kms *mdp_kms, uint32_t irqmask)
> {
> mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_INTR_EN, irqmask);
> @@ -47,6 +117,9 @@ int mdp5_irq_postinstall(struct msm_kms *kms)
> MDP5_IRQ_INTF2_UNDER_RUN |
> MDP5_IRQ_INTF3_UNDER_RUN;
>
> + /* Register mdp irq to mdss */
> + msm_shared_irq_register(MSM_SUBSYS_MDP, mdp5_irq_mdp, mdp_kms);
> +
> mdp_irq_register(mdp_kms, error_handler);
>
> return 0;
> @@ -56,10 +129,15 @@ void mdp5_irq_uninstall(struct msm_kms *kms)
> {
> struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
> mdp5_write(mdp5_kms, REG_MDP5_INTR_EN, 0x00000000);
> +
> + /* Make sure interrupt is disabled before remove irq. */
> + wmb();
> + msm_shared_irq_unregister(MSM_SUBSYS_MDP);
> }
>
> -static void mdp5_irq_mdp(struct mdp_kms *mdp_kms)
> +static irqreturn_t mdp5_irq_mdp(int irq, void *dev_id)
> {
> + struct mdp_kms *mdp_kms = (struct mdp_kms *)dev_id;
> struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
> struct drm_device *dev = mdp5_kms->dev;
> struct msm_drm_private *priv = dev->dev_private;
> @@ -76,23 +154,40 @@ static void mdp5_irq_mdp(struct mdp_kms *mdp_kms)
> for (id = 0; id < priv->num_crtcs; id++)
> if (status & mdp5_crtc_vblank(priv->crtcs[id]))
> drm_handle_vblank(dev, id);
> +
> + return IRQ_HANDLED;
> }
>
> irqreturn_t mdp5_irq(struct msm_kms *kms)
> {
> struct mdp_kms *mdp_kms = to_mdp_kms(kms);
> struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
> + struct msm_subsys_shared_irq *irq;
> uint32_t intr;
> + int i;
>
> intr = mdp5_read(mdp5_kms, REG_MDP5_HW_INTR_STATUS);
>
> VERB("intr=%08x", intr);
>
> - if (intr & MDP5_HW_INTR_STATUS_INTR_MDP)
> - mdp5_irq_mdp(mdp_kms);
> -
> - if (intr & MDP5_HW_INTR_STATUS_INTR_HDMI)
> - hdmi_irq(0, mdp5_kms->hdmi);
> + for (i = 0; i < MSM_SUBSYS_COUNT; i++) {
> + irq = &msm_shared_irqs[i];
> + if (intr & irq->mask) {
> + irq->count++;
> +
> + /*
> + * These 2 wmb() ensure count is odd number
> + * during handler is running.
> + */
> + wmb();
> + if ((irq->handler != NULL) && (irq->data != NULL))
> + irq->handler(0, irq->data);
> +
> + /* Make sure count increments after handler is done */
> + wmb();
> + irq->count++;
> + }
> + }
>
> return IRQ_HANDLED;
> }
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 67f9d0a..718ac55 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -127,6 +127,16 @@ struct msm_drm_private {
> } vram;
> };
>
> +/* For mdp5 only */
> +enum msm_sub_system {
> + MSM_SUBSYS_MDP = 0,
> + MSM_SUBSYS_DSI_0,
> + MSM_SUBSYS_DSI_1,
> + MSM_SUBSYS_HDMI,
> + MSM_SUBSYS_EDP,
> + MSM_SUBSYS_COUNT
> +};
> +
> struct msm_format {
> uint32_t pixel_format;
> };
> @@ -145,6 +155,14 @@ void __msm_fence_worker(struct work_struct *work);
> (_cb)->func = _func; \
> } while (0)
>
> +/*
> + * For mdp5 only, callers should call these 2 functions
> + * only if the irqs are shared with others.
> + */
> +int msm_shared_irq_register(enum msm_sub_system sys_id,
> + irqreturn_t (*handler)(int irq, void *dev_id), void *data);
> +int msm_shared_irq_unregister(enum msm_sub_system sys_id);
> +
> int msm_register_mmu(struct drm_device *dev, struct msm_mmu *mmu);
>
> int msm_wait_fence_interruptable(struct drm_device *dev, uint32_t fence,
> @@ -203,7 +221,6 @@ struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev);
>
> struct hdmi;
> struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder);
> -irqreturn_t hdmi_irq(int irq, void *dev_id);
> void __init hdmi_register(void);
> void __exit hdmi_unregister(void);
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss
@ 2014-11-16 14:18 ` Rob Clark
0 siblings, 0 replies; 8+ messages in thread
From: Rob Clark @ 2014-11-16 14:18 UTC (permalink / raw)
To: Hai Li; +Cc: dri-devel, linux-arm-msm, Linux Kernel Mailing List
On Fri, Nov 14, 2014 at 5:42 PM, Hai Li <hali@codeaurora.org> wrote:
> All the sub-systems in mdss share the same irq. This change provides
> the sub-systems with the interfaces to register/unregister their own
> irq handlers.
>
> With this change, struct mdp5_kms does not have to keep the hdmi or
> edp context.
>
So, I think the point of this is to better deal w/ different hw
variants which do or do-not have hdmi, edp, dsi, etc..
But, just playing devil's advocate here, it seems like it would be
simpler to instead just do something like:
if (priv->hdmi && (intr & MDP5_HW_INTR_STATUS_INTR_HDMI))
hdmi_irq(0, priv->hdmi);
if (priv->edp && (intr & MDP5_HW_INTR_STATUS_INTR_EDP))
edp_irq(0, priv->edp);
... etc ...
It is a little less elegant. But it is also less lines of code. I
guess there will be plenty of necessary complexity as we add support
for all mdp5 features. So avoiding some unnecessary complexity might
be a good thing in the long run.
If we really did want to make it more dynamic, we could always extend
'struct mdp_irq' to take both an irq mask and an initiator id, I
suppose. I'm not sure if that is overkill. AFAICT we really only
have 5 different subsystems to dispatch to (mdp5 itself, and
dsi0/dsi1/hdmi/edp), so simply adding some if-not-null checks in
mdp5_irq() seems pretty reasonable to me.
BR,
-R
> Signed-off-by: Hai Li <hali@codeaurora.org>
> ---
> drivers/gpu/drm/msm/hdmi/hdmi.c | 12 +++-
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c | 107 ++++++++++++++++++++++++++++++--
> drivers/gpu/drm/msm/msm_drv.h | 19 +++++-
> 3 files changed, 130 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 9d00dcb..aaf5e2b 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -39,7 +39,7 @@ void hdmi_set_mode(struct hdmi *hdmi, bool power_on)
> power_on ? "Enable" : "Disable", ctrl);
> }
>
> -irqreturn_t hdmi_irq(int irq, void *dev_id)
> +static irqreturn_t hdmi_irq(int irq, void *dev_id)
> {
> struct hdmi *hdmi = dev_id;
>
> @@ -59,6 +59,9 @@ void hdmi_destroy(struct kref *kref)
> struct hdmi *hdmi = container_of(kref, struct hdmi, refcount);
> struct hdmi_phy *phy = hdmi->phy;
>
> + if (hdmi->config->shared_irq)
> + msm_shared_irq_unregister(MSM_SUBSYS_HDMI);
> +
> if (phy)
> phy->funcs->destroy(phy);
>
> @@ -221,6 +224,13 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
> hdmi->irq, ret);
> goto fail;
> }
> + } else {
> + ret = msm_shared_irq_register(MSM_SUBSYS_HDMI, hdmi_irq, hdmi);
> + if (ret < 0) {
> + dev_err(dev->dev, "failed to register shared IRQ: %d\n",
> + ret);
> + goto fail;
> + }
> }
>
> encoder->bridge = hdmi->bridge;
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
> index f2b985b..2973c1c 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_irq.c
> @@ -1,4 +1,5 @@
> /*
> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> * Copyright (C) 2013 Red Hat
> * Author: Rob Clark <robdclark@gmail.com>
> *
> @@ -19,6 +20,75 @@
> #include "msm_drv.h"
> #include "mdp5_kms.h"
>
> +struct msm_subsys_shared_irq {
> + u32 mask;
> + u32 count;
> +
> + /* Filled by sub system */
> + irqreturn_t (*handler)(int irq, void *dev_id);
> + void *data;
> +};
> +
> +static struct msm_subsys_shared_irq msm_shared_irqs[MSM_SUBSYS_COUNT] = {
> + [MSM_SUBSYS_MDP] = {.mask = MDP5_HW_INTR_STATUS_INTR_MDP,
> + .count = 0},
> + [MSM_SUBSYS_DSI_0] = {.mask = MDP5_HW_INTR_STATUS_INTR_DSI0,
> + .count = 0},
> + [MSM_SUBSYS_DSI_1] = {.mask = MDP5_HW_INTR_STATUS_INTR_DSI1,
> + .count = 0},
> + [MSM_SUBSYS_HDMI] = {.mask = MDP5_HW_INTR_STATUS_INTR_HDMI,
> + .count = 0},
> + [MSM_SUBSYS_EDP] = {.mask = MDP5_HW_INTR_STATUS_INTR_EDP,
> + .count = 0},
> +};
> +
> +static irqreturn_t mdp5_irq_mdp(int irq, void *dev_id);
> +
> +int msm_shared_irq_register(enum msm_sub_system sys_id,
> + irqreturn_t (*handler)(int irq, void *dev_id), void *data)
> +{
> + if (sys_id >= MSM_SUBSYS_COUNT) {
> + DRM_ERROR("Invalid sys_id %d", sys_id);
> + return -EINVAL;
> + }
> +
> + if (msm_shared_irqs[sys_id].handler != NULL) {
> + DRM_ERROR("sys %d irq already registered", sys_id);
> + return -EBUSY;
> + }
> +
> + msm_shared_irqs[sys_id].data = data;
> + msm_shared_irqs[sys_id].handler = handler;
> +
> + return 0;
> +}
> +
> +/*
> + * This function should be called after the interrupt
> + * on the sub-system is disabled.
> + */
> +int msm_shared_irq_unregister(enum msm_sub_system sys_id)
> +{
> + if (sys_id >= MSM_SUBSYS_COUNT) {
> + DRM_ERROR("Invalid sys_id %d", sys_id);
> + return -EINVAL;
> + }
> +
> + msm_shared_irqs[sys_id].handler = NULL;
> + msm_shared_irqs[sys_id].data = NULL;
> +
> + /*
> + * Make sure irq_handler and data is invalid.
> + * Then we only need to wait until the last pending interrupt is done.
> + */
> + wmb();
> +
> + while (msm_shared_irqs[sys_id].count & 0x1)
> + usleep_range(100, 1000);
> +
> + return 0;
> +}
> +
> void mdp5_set_irqmask(struct mdp_kms *mdp_kms, uint32_t irqmask)
> {
> mdp5_write(to_mdp5_kms(mdp_kms), REG_MDP5_INTR_EN, irqmask);
> @@ -47,6 +117,9 @@ int mdp5_irq_postinstall(struct msm_kms *kms)
> MDP5_IRQ_INTF2_UNDER_RUN |
> MDP5_IRQ_INTF3_UNDER_RUN;
>
> + /* Register mdp irq to mdss */
> + msm_shared_irq_register(MSM_SUBSYS_MDP, mdp5_irq_mdp, mdp_kms);
> +
> mdp_irq_register(mdp_kms, error_handler);
>
> return 0;
> @@ -56,10 +129,15 @@ void mdp5_irq_uninstall(struct msm_kms *kms)
> {
> struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
> mdp5_write(mdp5_kms, REG_MDP5_INTR_EN, 0x00000000);
> +
> + /* Make sure interrupt is disabled before remove irq. */
> + wmb();
> + msm_shared_irq_unregister(MSM_SUBSYS_MDP);
> }
>
> -static void mdp5_irq_mdp(struct mdp_kms *mdp_kms)
> +static irqreturn_t mdp5_irq_mdp(int irq, void *dev_id)
> {
> + struct mdp_kms *mdp_kms = (struct mdp_kms *)dev_id;
> struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
> struct drm_device *dev = mdp5_kms->dev;
> struct msm_drm_private *priv = dev->dev_private;
> @@ -76,23 +154,40 @@ static void mdp5_irq_mdp(struct mdp_kms *mdp_kms)
> for (id = 0; id < priv->num_crtcs; id++)
> if (status & mdp5_crtc_vblank(priv->crtcs[id]))
> drm_handle_vblank(dev, id);
> +
> + return IRQ_HANDLED;
> }
>
> irqreturn_t mdp5_irq(struct msm_kms *kms)
> {
> struct mdp_kms *mdp_kms = to_mdp_kms(kms);
> struct mdp5_kms *mdp5_kms = to_mdp5_kms(mdp_kms);
> + struct msm_subsys_shared_irq *irq;
> uint32_t intr;
> + int i;
>
> intr = mdp5_read(mdp5_kms, REG_MDP5_HW_INTR_STATUS);
>
> VERB("intr=%08x", intr);
>
> - if (intr & MDP5_HW_INTR_STATUS_INTR_MDP)
> - mdp5_irq_mdp(mdp_kms);
> -
> - if (intr & MDP5_HW_INTR_STATUS_INTR_HDMI)
> - hdmi_irq(0, mdp5_kms->hdmi);
> + for (i = 0; i < MSM_SUBSYS_COUNT; i++) {
> + irq = &msm_shared_irqs[i];
> + if (intr & irq->mask) {
> + irq->count++;
> +
> + /*
> + * These 2 wmb() ensure count is odd number
> + * during handler is running.
> + */
> + wmb();
> + if ((irq->handler != NULL) && (irq->data != NULL))
> + irq->handler(0, irq->data);
> +
> + /* Make sure count increments after handler is done */
> + wmb();
> + irq->count++;
> + }
> + }
>
> return IRQ_HANDLED;
> }
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 67f9d0a..718ac55 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -127,6 +127,16 @@ struct msm_drm_private {
> } vram;
> };
>
> +/* For mdp5 only */
> +enum msm_sub_system {
> + MSM_SUBSYS_MDP = 0,
> + MSM_SUBSYS_DSI_0,
> + MSM_SUBSYS_DSI_1,
> + MSM_SUBSYS_HDMI,
> + MSM_SUBSYS_EDP,
> + MSM_SUBSYS_COUNT
> +};
> +
> struct msm_format {
> uint32_t pixel_format;
> };
> @@ -145,6 +155,14 @@ void __msm_fence_worker(struct work_struct *work);
> (_cb)->func = _func; \
> } while (0)
>
> +/*
> + * For mdp5 only, callers should call these 2 functions
> + * only if the irqs are shared with others.
> + */
> +int msm_shared_irq_register(enum msm_sub_system sys_id,
> + irqreturn_t (*handler)(int irq, void *dev_id), void *data);
> +int msm_shared_irq_unregister(enum msm_sub_system sys_id);
> +
> int msm_register_mmu(struct drm_device *dev, struct msm_mmu *mmu);
>
> int msm_wait_fence_interruptable(struct drm_device *dev, uint32_t fence,
> @@ -203,7 +221,6 @@ struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev);
>
> struct hdmi;
> struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder);
> -irqreturn_t hdmi_irq(int irq, void *dev_id);
> void __init hdmi_register(void);
> void __exit hdmi_unregister(void);
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] drm/msm: Decouple hdmi driver from mdp driver
2014-11-14 22:42 ` [PATCH 2/2] drm/msm: Decouple hdmi driver from mdp driver Hai Li
@ 2014-11-16 14:35 ` Rob Clark
0 siblings, 0 replies; 8+ messages in thread
From: Rob Clark @ 2014-11-16 14:35 UTC (permalink / raw)
To: Hai Li; +Cc: dri-devel, linux-arm-msm, Linux Kernel Mailing List
On Fri, Nov 14, 2014 at 5:42 PM, Hai Li <hali@codeaurora.org> wrote:
> This change is to remove the hdmi structure from mdp kms data structure.
>
> To do this, the initialization flow is re-arranged.
> - hdmi_init is moved from modeset_init to hdmi_bind.
> - hdmi_destroy is called by hdmi_unbind and the use of kref is abandoned.
> - A new interface hdmi_set_encoder is introduced to establish the links
> between hdmi connector, encoder and bridge.
>
this looks like it is going to conflict somewhat with:
http://cgit.freedesktop.org/~robclark/linux/commit/?h=msm-next&id=3b3a2964a37887ced36261b4b5622ab6137a860c
I had already refactored the hdmi init to better handle probe-defer
(and move all the devm_get's into hdmi_bind()). It is somewhat
similar to what you are doing here.. hdmi_init() is called at
hdmi_bind() time, and then hdmi_modeset_init() is called at toplevel
drm device modeset_init() time to construct the kms modeset object
(connector/bridge) and connect up with encoder.
I didn't remove the kref yet, although it probably could/should be
removed. It doesn't really protect us if the hdmi device is removed
before the toplevel drm device (since all the devm resources would be
dropped). But AFAIU the component framework stuff should protect us
from that. The kref is just left over from before component framework
because I forgot to remove it ;-)
BR,
-R
> Signed-off-by: Hai Li <hali@codeaurora.org>
> ---
> drivers/gpu/drm/msm/hdmi/hdmi.c | 61 +++++++++++++++++++++++--------
> drivers/gpu/drm/msm/hdmi/hdmi.h | 14 -------
> drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 3 +-
> drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 6 +--
> drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 14 ++++---
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 38 ++++++++++---------
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 2 -
> drivers/gpu/drm/msm/msm_drv.c | 9 ++++-
> drivers/gpu/drm/msm/msm_drv.h | 6 ++-
> 9 files changed, 88 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index aaf5e2b..2d2551f 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -1,4 +1,5 @@
> /*
> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> * Copyright (C) 2013 Red Hat
> * Author: Rob Clark <robdclark@gmail.com>
> *
> @@ -17,6 +18,29 @@
>
> #include "hdmi.h"
>
> +int hdmi_set_encoder(struct platform_device *pdev,
> + struct drm_encoder *encoder)
> +{
> + struct hdmi *hdmi = platform_get_drvdata(pdev);
> +
> + if (!encoder && !hdmi->encoder) {
> + pr_err("%s:wrong encoder status,encoder=%p,hdmi->encoder=%p\n",
> + __func__, encoder, hdmi->encoder);
> + return -EINVAL;
> + }
> +
> + /* Each connector has only one available encoder for now. */
> + hdmi->connector->encoder_ids[0] = 0;
> + hdmi->connector->encoder = NULL;
> + if (encoder) {
> + drm_mode_connector_attach_encoder(hdmi->connector, encoder);
> + encoder->bridge = hdmi->bridge;
> + }
> + hdmi->encoder = encoder;
> +
> + return 0;
> +}
> +
> void hdmi_set_mode(struct hdmi *hdmi, bool power_on)
> {
> uint32_t ctrl = 0;
> @@ -54,10 +78,15 @@ static irqreturn_t hdmi_irq(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> -void hdmi_destroy(struct kref *kref)
> +static void hdmi_destroy(struct platform_device *pdev)
> {
> - struct hdmi *hdmi = container_of(kref, struct hdmi, refcount);
> - struct hdmi_phy *phy = hdmi->phy;
> + struct hdmi *hdmi = platform_get_drvdata(pdev);
> + struct hdmi_phy *phy;
> +
> + if (!hdmi)
> + return;
> +
> + phy = hdmi->phy;
>
> if (hdmi->config->shared_irq)
> msm_shared_irq_unregister(MSM_SUBSYS_HDMI);
> @@ -68,15 +97,14 @@ void hdmi_destroy(struct kref *kref)
> if (hdmi->i2c)
> hdmi_i2c_destroy(hdmi->i2c);
>
> - platform_set_drvdata(hdmi->pdev, NULL);
> + platform_set_drvdata(pdev, NULL);
> }
>
> /* initialize connector */
> -struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
> +static int hdmi_init(struct platform_device *pdev, struct drm_device *dev)
> {
> struct hdmi *hdmi = NULL;
> struct msm_drm_private *priv = dev->dev_private;
> - struct platform_device *pdev = priv->hdmi_pdev;
> struct hdmi_platform_config *config;
> int i, ret;
>
> @@ -94,12 +122,11 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
> goto fail;
> }
>
> - kref_init(&hdmi->refcount);
> + platform_set_drvdata(pdev, hdmi);
>
> hdmi->dev = dev;
> hdmi->pdev = pdev;
> hdmi->config = config;
> - hdmi->encoder = encoder;
>
> hdmi_audio_infoframe_init(&hdmi->audio.infoframe);
>
> @@ -233,14 +260,10 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)
> }
> }
>
> - encoder->bridge = hdmi->bridge;
> -
> priv->bridges[priv->num_bridges++] = hdmi->bridge;
> priv->connectors[priv->num_connectors++] = hdmi->connector;
>
> - platform_set_drvdata(pdev, hdmi);
> -
> - return hdmi;
> + return 0;
>
> fail:
> if (hdmi) {
> @@ -249,10 +272,10 @@ fail:
> hdmi->bridge->funcs->destroy(hdmi->bridge);
> if (hdmi->connector)
> hdmi->connector->funcs->destroy(hdmi->connector);
> - hdmi_destroy(&hdmi->refcount);
> + hdmi_destroy(pdev);
> }
>
> - return ERR_PTR(ret);
> + return ret;
> }
>
> /*
> @@ -289,6 +312,7 @@ static int get_gpio(struct device *dev, struct device_node *of_node, const char
> static int hdmi_bind(struct device *dev, struct device *master, void *data)
> {
> static struct hdmi_platform_config config = {};
> + int ret;
> #ifdef CONFIG_OF
> struct device_node *of_node = dev->of_node;
>
> @@ -379,6 +403,12 @@ static int hdmi_bind(struct device *dev, struct device *master, void *data)
> }
> #endif
> dev->platform_data = &config;
> +
> + ret = hdmi_init(to_platform_device(dev), dev_get_drvdata(master));
> + if (ret) {
> + dev_err(dev, "%s: hdmi_init fail, %d\n", __func__, ret);
> + return ret;
> + }
> set_hdmi_pdev(dev_get_drvdata(master), to_platform_device(dev));
> return 0;
> }
> @@ -387,6 +417,7 @@ static void hdmi_unbind(struct device *dev, struct device *master,
> void *data)
> {
> set_hdmi_pdev(dev_get_drvdata(master), NULL);
> + hdmi_destroy(to_platform_device(dev));
> }
>
> static const struct component_ops hdmi_ops = {
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> index b981995..a78dd8d 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> @@ -38,8 +38,6 @@ struct hdmi_audio {
> };
>
> struct hdmi {
> - struct kref refcount;
> -
> struct drm_device *dev;
> struct platform_device *pdev;
>
> @@ -103,7 +101,6 @@ struct hdmi_platform_config {
> };
>
> void hdmi_set_mode(struct hdmi *hdmi, bool power_on);
> -void hdmi_destroy(struct kref *kref);
>
> static inline void hdmi_write(struct hdmi *hdmi, u32 reg, u32 data)
> {
> @@ -115,17 +112,6 @@ static inline u32 hdmi_read(struct hdmi *hdmi, u32 reg)
> return msm_readl(hdmi->mmio + reg);
> }
>
> -static inline struct hdmi * hdmi_reference(struct hdmi *hdmi)
> -{
> - kref_get(&hdmi->refcount);
> - return hdmi;
> -}
> -
> -static inline void hdmi_unreference(struct hdmi *hdmi)
> -{
> - kref_put(&hdmi->refcount, hdmi_destroy);
> -}
> -
> /*
> * The phy appears to be different, for example between 8960 and 8x60,
> * so split the phy related functions out and load the correct one at
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index f6cf745..6902ad6 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -26,7 +26,6 @@ struct hdmi_bridge {
> static void hdmi_bridge_destroy(struct drm_bridge *bridge)
> {
> struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
> - hdmi_unreference(hdmi_bridge->hdmi);
> drm_bridge_cleanup(bridge);
> kfree(hdmi_bridge);
> }
> @@ -218,7 +217,7 @@ struct drm_bridge *hdmi_bridge_init(struct hdmi *hdmi)
> goto fail;
> }
>
> - hdmi_bridge->hdmi = hdmi_reference(hdmi);
> + hdmi_bridge->hdmi = hdmi;
>
> bridge = &hdmi_bridge->base;
>
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> index 4aca2a3..f5da877 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> @@ -330,8 +330,6 @@ static void hdmi_connector_destroy(struct drm_connector *connector)
> drm_connector_unregister(connector);
> drm_connector_cleanup(connector);
>
> - hdmi_unreference(hdmi_connector->hdmi);
> -
> kfree(hdmi_connector);
> }
>
> @@ -422,7 +420,7 @@ struct drm_connector *hdmi_connector_init(struct hdmi *hdmi)
> goto fail;
> }
>
> - hdmi_connector->hdmi = hdmi_reference(hdmi);
> + hdmi_connector->hdmi = hdmi;
> INIT_WORK(&hdmi_connector->hpd_work, hotplug_work);
>
> connector = &hdmi_connector->base;
> @@ -445,8 +443,6 @@ struct drm_connector *hdmi_connector_init(struct hdmi *hdmi)
> goto fail;
> }
>
> - drm_mode_connector_attach_encoder(connector, hdmi->encoder);
> -
> return connector;
>
> fail:
> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
> index 79d804e..b0b6dee 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
> @@ -228,7 +228,6 @@ static int modeset_init(struct mdp4_kms *mdp4_kms)
> struct drm_encoder *encoder;
> struct drm_connector *connector;
> struct drm_panel *panel;
> - struct hdmi *hdmi;
> int ret;
>
> /* construct non-private planes: */
> @@ -326,15 +325,18 @@ static int modeset_init(struct mdp4_kms *mdp4_kms)
> priv->crtcs[priv->num_crtcs++] = crtc;
> priv->encoders[priv->num_encoders++] = encoder;
>
> - hdmi = hdmi_init(dev, encoder);
> - if (IS_ERR(hdmi)) {
> - ret = PTR_ERR(hdmi);
> - dev_err(dev->dev, "failed to initialize HDMI: %d\n", ret);
> - goto fail;
> + ret = hdmi_set_encoder(priv->hdmi_pdev, encoder);
> + if (ret) {
> + dev_err(dev->dev, "failed to set encoder\n");
> + goto fail1;
> }
>
> return 0;
>
> +fail1:
> + priv->encoders[--priv->num_encoders] = NULL;
> + if (encoder)
> + encoder->funcs->destroy(encoder);
> fail:
> return ret;
> }
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> index 31a2c63..1bb3a28 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> @@ -302,14 +302,6 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
> priv->crtcs[priv->num_crtcs++] = crtc;
> }
>
> - /* Construct encoder for HDMI: */
> - encoder = mdp5_encoder_init(dev, 3, INTF_HDMI);
> - if (IS_ERR(encoder)) {
> - dev_err(dev->dev, "failed to construct encoder\n");
> - ret = PTR_ERR(encoder);
> - goto fail;
> - }
> -
> /* NOTE: the vsync and error irq's are actually associated with
> * the INTF/encoder.. the easiest way to deal with this (ie. what
> * we do now) is assume a fixed relationship between crtc's and
> @@ -318,21 +310,33 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
> * care of error and vblank irq's that the crtc has registered,
> * and also update user-requested vblank_mask.
> */
> - encoder->possible_crtcs = BIT(0);
> - mdp5_crtc_set_intf(priv->crtcs[0], 3, INTF_HDMI);
> + if (priv->hdmi_pdev) {
> + /* Construct encoder for HDMI: */
> + encoder = mdp5_encoder_init(dev, 3, INTF_HDMI);
> + if (IS_ERR(encoder)) {
> + dev_err(dev->dev, "failed to construct encoder\n");
> + ret = PTR_ERR(encoder);
> + goto fail;
> + }
>
> - priv->encoders[priv->num_encoders++] = encoder;
> + encoder->possible_crtcs = BIT(0);
> + mdp5_crtc_set_intf(priv->crtcs[0], 3, INTF_HDMI);
>
> - /* Construct bridge/connector for HDMI: */
> - mdp5_kms->hdmi = hdmi_init(dev, encoder);
> - if (IS_ERR(mdp5_kms->hdmi)) {
> - ret = PTR_ERR(mdp5_kms->hdmi);
> - dev_err(dev->dev, "failed to initialize HDMI: %d\n", ret);
> - goto fail;
> + priv->encoders[priv->num_encoders++] = encoder;
> +
> + ret = hdmi_set_encoder(priv->hdmi_pdev, encoder);
> + if (ret)
> + goto fail1;
> }
>
> return 0;
>
> +fail1:
> + if (priv->hdmi_pdev) {
> + priv->encoders[--priv->num_encoders] = NULL;
> + if (encoder)
> + encoder->funcs->destroy(encoder);
> + }
> fail:
> return ret;
> }
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> index 5bf340d..c91101d 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> @@ -71,8 +71,6 @@ struct mdp5_kms {
> struct clk *lut_clk;
> struct clk *vsync_clk;
>
> - struct hdmi *hdmi;
> -
> struct mdp_irq error_handler;
> };
> #define to_mdp5_kms(x) container_of(x, struct mdp5_kms, base)
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index b67ef59..c27cb9a 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -147,7 +147,10 @@ static int msm_unload(struct drm_device *dev)
> priv->vram.paddr, &attrs);
> }
>
> - component_unbind_all(dev->dev, dev);
> + if (priv->component_bound) {
> + component_unbind_all(dev->dev, dev);
> + priv->component_bound = false;
> + }
>
> dev->dev_private = NULL;
>
> @@ -237,7 +240,9 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
> /* Bind all our sub-components: */
> ret = component_bind_all(dev->dev, dev);
> if (ret)
> - return ret;
> + goto fail;
> +
> + priv->component_bound = true;
>
> switch (get_mdp_ver(pdev)) {
> case 4:
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 718ac55..76cac63 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -125,6 +125,8 @@ struct msm_drm_private {
> */
> struct drm_mm mm;
> } vram;
> +
> + bool component_bound;
> };
>
> /* For mdp5 only */
> @@ -219,10 +221,10 @@ struct drm_framebuffer *msm_framebuffer_create(struct drm_device *dev,
>
> struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev);
>
> -struct hdmi;
> -struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder);
> void __init hdmi_register(void);
> void __exit hdmi_unregister(void);
> +int hdmi_set_encoder(struct platform_device *pdev,
> + struct drm_encoder *encoder);
>
> #ifdef CONFIG_DEBUG_FS
> void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss
2014-11-16 14:18 ` Rob Clark
@ 2014-11-20 8:57 ` Thierry Reding
-1 siblings, 0 replies; 8+ messages in thread
From: Thierry Reding @ 2014-11-20 8:57 UTC (permalink / raw)
To: Rob Clark; +Cc: linux-arm-msm, Hai Li, Linux Kernel Mailing List, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 1812 bytes --]
On Sun, Nov 16, 2014 at 09:18:47AM -0500, Rob Clark wrote:
> On Fri, Nov 14, 2014 at 5:42 PM, Hai Li <hali@codeaurora.org> wrote:
> > All the sub-systems in mdss share the same irq. This change provides
> > the sub-systems with the interfaces to register/unregister their own
> > irq handlers.
> >
> > With this change, struct mdp5_kms does not have to keep the hdmi or
> > edp context.
> >
>
> So, I think the point of this is to better deal w/ different hw
> variants which do or do-not have hdmi, edp, dsi, etc..
>
> But, just playing devil's advocate here, it seems like it would be
> simpler to instead just do something like:
>
> if (priv->hdmi && (intr & MDP5_HW_INTR_STATUS_INTR_HDMI))
> hdmi_irq(0, priv->hdmi);
>
> if (priv->edp && (intr & MDP5_HW_INTR_STATUS_INTR_EDP))
> edp_irq(0, priv->edp);
>
> ... etc ...
>
> It is a little less elegant. But it is also less lines of code. I
> guess there will be plenty of necessary complexity as we add support
> for all mdp5 features. So avoiding some unnecessary complexity might
> be a good thing in the long run.
>
> If we really did want to make it more dynamic, we could always extend
> 'struct mdp_irq' to take both an irq mask and an initiator id, I
> suppose. I'm not sure if that is overkill. AFAICT we really only
> have 5 different subsystems to dispatch to (mdp5 itself, and
> dsi0/dsi1/hdmi/edp), so simply adding some if-not-null checks in
> mdp5_irq() seems pretty reasonable to me.
To me this just seems like a regular interrupt multiplexer, so
implementing it as an irq_chip would be the most appropriate. That way
you get all the goodness of a well-tested code base for free and you can
simply pass in the request_{threaded_,}irq()'s dev parameter.
Thierry
[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss
@ 2014-11-20 8:57 ` Thierry Reding
0 siblings, 0 replies; 8+ messages in thread
From: Thierry Reding @ 2014-11-20 8:57 UTC (permalink / raw)
To: Rob Clark; +Cc: Hai Li, linux-arm-msm, Linux Kernel Mailing List, dri-devel
[-- Attachment #1: Type: text/plain, Size: 1812 bytes --]
On Sun, Nov 16, 2014 at 09:18:47AM -0500, Rob Clark wrote:
> On Fri, Nov 14, 2014 at 5:42 PM, Hai Li <hali@codeaurora.org> wrote:
> > All the sub-systems in mdss share the same irq. This change provides
> > the sub-systems with the interfaces to register/unregister their own
> > irq handlers.
> >
> > With this change, struct mdp5_kms does not have to keep the hdmi or
> > edp context.
> >
>
> So, I think the point of this is to better deal w/ different hw
> variants which do or do-not have hdmi, edp, dsi, etc..
>
> But, just playing devil's advocate here, it seems like it would be
> simpler to instead just do something like:
>
> if (priv->hdmi && (intr & MDP5_HW_INTR_STATUS_INTR_HDMI))
> hdmi_irq(0, priv->hdmi);
>
> if (priv->edp && (intr & MDP5_HW_INTR_STATUS_INTR_EDP))
> edp_irq(0, priv->edp);
>
> ... etc ...
>
> It is a little less elegant. But it is also less lines of code. I
> guess there will be plenty of necessary complexity as we add support
> for all mdp5 features. So avoiding some unnecessary complexity might
> be a good thing in the long run.
>
> If we really did want to make it more dynamic, we could always extend
> 'struct mdp_irq' to take both an irq mask and an initiator id, I
> suppose. I'm not sure if that is overkill. AFAICT we really only
> have 5 different subsystems to dispatch to (mdp5 itself, and
> dsi0/dsi1/hdmi/edp), so simply adding some if-not-null checks in
> mdp5_irq() seems pretty reasonable to me.
To me this just seems like a regular interrupt multiplexer, so
implementing it as an irq_chip would be the most appropriate. That way
you get all the goodness of a well-tested code base for free and you can
simply pass in the request_{threaded_,}irq()'s dev parameter.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss
2014-11-20 8:57 ` Thierry Reding
(?)
@ 2014-11-20 14:47 ` Rob Clark
-1 siblings, 0 replies; 8+ messages in thread
From: Rob Clark @ 2014-11-20 14:47 UTC (permalink / raw)
To: Thierry Reding
Cc: Hai Li, linux-arm-msm, Linux Kernel Mailing List, dri-devel
On Thu, Nov 20, 2014 at 3:57 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Sun, Nov 16, 2014 at 09:18:47AM -0500, Rob Clark wrote:
>> On Fri, Nov 14, 2014 at 5:42 PM, Hai Li <hali@codeaurora.org> wrote:
>> > All the sub-systems in mdss share the same irq. This change provides
>> > the sub-systems with the interfaces to register/unregister their own
>> > irq handlers.
>> >
>> > With this change, struct mdp5_kms does not have to keep the hdmi or
>> > edp context.
>> >
>>
>> So, I think the point of this is to better deal w/ different hw
>> variants which do or do-not have hdmi, edp, dsi, etc..
>>
>> But, just playing devil's advocate here, it seems like it would be
>> simpler to instead just do something like:
>>
>> if (priv->hdmi && (intr & MDP5_HW_INTR_STATUS_INTR_HDMI))
>> hdmi_irq(0, priv->hdmi);
>>
>> if (priv->edp && (intr & MDP5_HW_INTR_STATUS_INTR_EDP))
>> edp_irq(0, priv->edp);
>>
>> ... etc ...
>>
>> It is a little less elegant. But it is also less lines of code. I
>> guess there will be plenty of necessary complexity as we add support
>> for all mdp5 features. So avoiding some unnecessary complexity might
>> be a good thing in the long run.
>>
>> If we really did want to make it more dynamic, we could always extend
>> 'struct mdp_irq' to take both an irq mask and an initiator id, I
>> suppose. I'm not sure if that is overkill. AFAICT we really only
>> have 5 different subsystems to dispatch to (mdp5 itself, and
>> dsi0/dsi1/hdmi/edp), so simply adding some if-not-null checks in
>> mdp5_irq() seems pretty reasonable to me.
>
> To me this just seems like a regular interrupt multiplexer, so
> implementing it as an irq_chip would be the most appropriate. That way
> you get all the goodness of a well-tested code base for free and you can
> simply pass in the request_{threaded_,}irq()'s dev parameter.
yup, that is what I did here:
http://cgit.freedesktop.org/~robclark/linux/commit/?h=msm-next&id=d9a7093329225ae29bae370823af13290b133a7e
there is a bit of awkwardness related to threaded handlers.. for now
I don't need threaded handlers so the solution was to not use them
> Thierry
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-11-20 14:47 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-14 22:42 [PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss Hai Li
2014-11-14 22:42 ` [PATCH 2/2] drm/msm: Decouple hdmi driver from mdp driver Hai Li
2014-11-16 14:35 ` Rob Clark
2014-11-16 14:18 ` [PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss Rob Clark
2014-11-16 14:18 ` Rob Clark
2014-11-20 8:57 ` Thierry Reding
2014-11-20 8:57 ` Thierry Reding
2014-11-20 14:47 ` Rob Clark
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.