[PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss
On Sun, Nov 16, 2014 at 09:18:47AM -0500, Rob Clark wrote:
> On Fri, Nov 14, 2014 at 5:42 PM, Hai Li 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
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:
[PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss
On Thu, Nov 20, 2014 at 3:57 AM, Thierry Reding
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 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=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
[PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss
On Fri, Nov 14, 2014 at 5:42 PM, Hai Li 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
> ---
> 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
> *
> @@ -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;
> +
[PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss
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
---
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
*
@@ -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
