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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux