Re: [PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss

From: Rob Clark
Date: Sun Nov 16 2014 - 09:19:43 EST

Next message: SF Markus Elfring: "Re: [Cocci] [PATCH 1/1] kprobes: Deletion of an unnecessary check before the function call "module_put""
Previous message: Gloria C. Mackenzie: "[no subject]"
Next in thread: Thierry Reding: "Re: [PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Nov 14, 2014 at 5:42 PM, Hai Li <hali@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> *
> @@ -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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: SF Markus Elfring: "Re: [Cocci] [PATCH 1/1] kprobes: Deletion of an unnecessary check before the function call "module_put""
Previous message: Gloria C. Mackenzie: "[no subject]"
Next in thread: Thierry Reding: "Re: [PATCH 1/2] drm/msm: Register irq handler for each sub-system in mdss"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]