Re: [PATCH] drm/msm: Refactor msm drm driver by introducing msm_drm_sub_dev

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

 



> On Tue, Mar 24, 2015 at 11:18 AM,  <jilaiw@xxxxxxxxxxxxxx> wrote:
>>
>>> On Thu, Mar 12, 2015 at 4:48 PM, Jilai Wang <jilaiw@xxxxxxxxxxxxxx>
>>> wrote:
>>>> Introduce msm_drm_sub_dev for each mdp interface component such as
>>>> HDMI/eDP/DSI to contain common information shared with MDP.
>>>>
>>>> Signed-off-by: Jilai Wang <jilaiw@xxxxxxxxxxxxxx>
>>>> ---
>>>>  drivers/gpu/drm/msm/edp/edp.c           | 18 +++++++++--
>>>>  drivers/gpu/drm/msm/edp/edp.h           |  1 +
>>>>  drivers/gpu/drm/msm/hdmi/hdmi.c         | 22 ++++++++++---
>>>>  drivers/gpu/drm/msm/hdmi/hdmi.h         |  1 +
>>>>  drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c |  3 +-
>>>>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 56
>>>> ++++++++++++++++-----------------
>>>>  drivers/gpu/drm/msm/msm_drv.h           | 23 +++++++++-----
>>>>  7 files changed, 80 insertions(+), 44 deletions(-)
>>>
>>> So a couple comments..
>>>
>>> 1) I kinda prefer having some to_hdmi/to_edp/etc macros rather than
>>> just open coding container_of().. I guess kind of a minor thing, but
>>> it keeps things consistent with how "inheritance" is handled elsewhere
>>> (like to_mdp5_crtc(), etc)
>>>
>>> 2) I'd be a bit more enthused by this when it actually results in a
>>> negative diffstat, rather than a positive one.  Not saying it isn't
>>> something we should do at some point, but at this point I'm leaning
>>> towards rebasing the DSI patcheset to not depend on this for now.
>> Actually most of the negative diffstat is in mdp5_kms.c which moves the
>> modeset_init out of construct encoder. And it is required by DSI change.
>
>
> What I meant is that it adds more lines of code than it removes.. for
> refactoring/cleanup type things, I'd generally prefer things that work
> out the other way around (removing more lines than they add)..
> Anyways, if I drop this patch, I'll rebase the DSI patches.. that is
> fine.
>
> By the time I get DSI also working on mdp4, it might get to the point
> where this sort of change actually removes more lines than it adds, so
> that might be the time to revisit.  We may also be able to simplify it
> a bit..  I'm still thinking about it, I haven't completely decided
> yet.
ok.
>
> BR,
> -R
>
>
>>>
>>> BR,
>>> -R
>>>
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/edp/edp.c
>>>> b/drivers/gpu/drm/msm/edp/edp.c
>>>> index 0940e84..8d8b7e9 100644
>>>> --- a/drivers/gpu/drm/msm/edp/edp.c
>>>> +++ b/drivers/gpu/drm/msm/edp/edp.c
>>>> @@ -14,6 +14,9 @@
>>>>  #include <linux/of_irq.h>
>>>>  #include "edp.h"
>>>>
>>>> +static int msm_edp_modeset_init(struct msm_drm_sub_dev *base,
>>>> +       struct drm_device *dev);
>>>> +
>>>>  static irqreturn_t edp_irq(int irq, void *dev_id)
>>>>  {
>>>>         struct msm_edp *edp = dev_id;
>>>> @@ -63,6 +66,8 @@ static struct msm_edp *edp_init(struct
>>>> platform_device
>>>> *pdev)
>>>>         if (ret)
>>>>                 goto fail;
>>>>
>>>> +       edp->base.modeset_init = msm_edp_modeset_init;
>>>> +
>>>>         return edp;
>>>>
>>>>  fail:
>>>> @@ -82,7 +87,8 @@ static int edp_bind(struct device *dev, struct
>>>> device
>>>> *master, void *data)
>>>>         edp = edp_init(to_platform_device(dev));
>>>>         if (IS_ERR(edp))
>>>>                 return PTR_ERR(edp);
>>>> -       priv->edp = edp;
>>>> +
>>>> +       priv->edp = &edp->base;
>>>>
>>>>         return 0;
>>>>  }
>>>> @@ -144,13 +150,19 @@ void __exit msm_edp_unregister(void)
>>>>  }
>>>>
>>>>  /* Second part of initialization, the drm/kms level modeset_init */
>>>> -int msm_edp_modeset_init(struct msm_edp *edp, struct drm_device *dev,
>>>> -                               struct drm_encoder *encoder)
>>>> +static int msm_edp_modeset_init(struct msm_drm_sub_dev *base,
>>>> +       struct drm_device *dev)
>>>>  {
>>>> +       struct msm_edp *edp = container_of(base, struct msm_edp,
>>>> base);
>>>>         struct platform_device *pdev = edp->pdev;
>>>>         struct msm_drm_private *priv = dev->dev_private;
>>>> +       struct drm_encoder *encoder;
>>>>         int ret;
>>>>
>>>> +       if (WARN_ON(base->num_encoders != 1))
>>>> +               return -EINVAL;
>>>> +
>>>> +       encoder = base->encoders[0];
>>>>         edp->encoder = encoder;
>>>>         edp->dev = dev;
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/edp/edp.h
>>>> b/drivers/gpu/drm/msm/edp/edp.h
>>>> index ba5bedd..00eff68 100644
>>>> --- a/drivers/gpu/drm/msm/edp/edp.h
>>>> +++ b/drivers/gpu/drm/msm/edp/edp.h
>>>> @@ -31,6 +31,7 @@ struct edp_aux;
>>>>  struct edp_phy;
>>>>
>>>>  struct msm_edp {
>>>> +       struct msm_drm_sub_dev base;
>>>>         struct drm_device *dev;
>>>>         struct platform_device *pdev;
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c
>>>> b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>>> index 9a8a825..9e886ec 100644
>>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
>>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>>> @@ -19,6 +19,9 @@
>>>>  #include <linux/of_irq.h>
>>>>  #include "hdmi.h"
>>>>
>>>> +static int hdmi_modeset_init(struct msm_drm_sub_dev *base,
>>>> +               struct drm_device *dev);
>>>> +
>>>>  void hdmi_set_mode(struct hdmi *hdmi, bool power_on)
>>>>  {
>>>>         uint32_t ctrl = 0;
>>>> @@ -197,6 +200,8 @@ static struct hdmi *hdmi_init(struct
>>>> platform_device
>>>> *pdev)
>>>>                 goto fail;
>>>>         }
>>>>
>>>> +       hdmi->base.modeset_init = hdmi_modeset_init;
>>>> +
>>>>         return hdmi;
>>>>
>>>>  fail:
>>>> @@ -214,13 +219,19 @@ fail:
>>>>   * should be handled in hdmi_init() so that failure happens from
>>>>   * hdmi sub-device's probe.
>>>>   */
>>>> -int hdmi_modeset_init(struct hdmi *hdmi,
>>>> -               struct drm_device *dev, struct drm_encoder *encoder)
>>>> +static int hdmi_modeset_init(struct msm_drm_sub_dev *base,
>>>> +               struct drm_device *dev)
>>>>  {
>>>> +       struct hdmi *hdmi = container_of(base, struct hdmi, base);
>>>>         struct msm_drm_private *priv = dev->dev_private;
>>>>         struct platform_device *pdev = hdmi->pdev;
>>>> +       struct drm_encoder *encoder;
>>>>         int ret;
>>>>
>>>> +       if (WARN_ON(base->num_encoders != 1))
>>>> +               return -EINVAL;
>>>> +
>>>> +       encoder = base->encoders[0];
>>>>         hdmi->dev = dev;
>>>>         hdmi->encoder = encoder;
>>>>
>>>> @@ -439,7 +450,8 @@ static int hdmi_bind(struct device *dev, struct
>>>> device *master, void *data)
>>>>         hdmi = hdmi_init(to_platform_device(dev));
>>>>         if (IS_ERR(hdmi))
>>>>                 return PTR_ERR(hdmi);
>>>> -       priv->hdmi = hdmi;
>>>> +
>>>> +       priv->hdmi = &hdmi->base;
>>>>
>>>>         return 0;
>>>>  }
>>>> @@ -449,8 +461,10 @@ static void hdmi_unbind(struct device *dev,
>>>> struct
>>>> device *master,
>>>>  {
>>>>         struct drm_device *drm = dev_get_drvdata(master);
>>>>         struct msm_drm_private *priv = drm->dev_private;
>>>> +
>>>>         if (priv->hdmi) {
>>>> -               hdmi_destroy(priv->hdmi);
>>>> +               struct hdmi *hdmi = container_of(priv->hdmi, struct
>>>> hdmi, base);
>>>> +               hdmi_destroy(hdmi);
>>>>                 priv->hdmi = NULL;
>>>>         }
>>>>  }
>>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h
>>>> b/drivers/gpu/drm/msm/hdmi/hdmi.h
>>>> index 68fdfb3..a1d4595 100644
>>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
>>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
>>>> @@ -38,6 +38,7 @@ struct hdmi_audio {
>>>>  };
>>>>
>>>>  struct hdmi {
>>>> +       struct msm_drm_sub_dev base;
>>>>         struct drm_device *dev;
>>>>         struct platform_device *pdev;
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
>>>> b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
>>>> index 24c38d4..02426ba 100644
>>>> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
>>>> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c
>>>> @@ -370,7 +370,8 @@ static int modeset_init(struct mdp4_kms *mdp4_kms)
>>>>
>>>>         if (priv->hdmi) {
>>>>                 /* Construct bridge/connector for HDMI: */
>>>> -               ret = hdmi_modeset_init(priv->hdmi, dev, encoder);
>>>> +               priv->hdmi->encoders[priv->hdmi->num_encoders++] =
>>>> encoder;
>>>> +               ret = priv->hdmi->modeset_init(priv->hdmi, dev);
>>>>                 if (ret) {
>>>>                         dev_err(dev->dev, "failed to initialize HDMI:
>>>> %d\n", ret);
>>>>                         goto fail;
>>>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>>>> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>>>> index 84168bf..ae336ec 100644
>>>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>>>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>>>> @@ -1,5 +1,5 @@
>>>>  /*
>>>> - * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>>>> + * Copyright (c) 2014-2015, The Linux Foundation. All rights
>>>> reserved.
>>>>   * Copyright (C) 2013 Red Hat
>>>>   * Author: Rob Clark <robdclark@xxxxxxxxx>
>>>>   *
>>>> @@ -166,8 +166,9 @@ int mdp5_enable(struct mdp5_kms *mdp5_kms)
>>>>         return 0;
>>>>  }
>>>>
>>>> -static int construct_encoder(struct mdp5_kms *mdp5_kms,
>>>> -               enum mdp5_intf_type intf_type, int intf_num)
>>>> +static struct drm_encoder *construct_encoder(struct mdp5_kms
>>>> *mdp5_kms,
>>>> +               enum mdp5_intf_type intf_type, int intf_num,
>>>> +               enum mdp5_intf_mode intf_mode)
>>>>  {
>>>>         struct drm_device *dev = mdp5_kms->dev;
>>>>         struct msm_drm_private *priv = dev->dev_private;
>>>> @@ -175,33 +176,19 @@ static int construct_encoder(struct mdp5_kms
>>>> *mdp5_kms,
>>>>         struct mdp5_interface intf = {
>>>>                         .num    = intf_num,
>>>>                         .type   = intf_type,
>>>> -                       .mode   = MDP5_INTF_MODE_NONE,
>>>> +                       .mode   = intf_mode,
>>>>         };
>>>> -       int ret = 0;
>>>>
>>>>         encoder = mdp5_encoder_init(dev, &intf);
>>>>         if (IS_ERR(encoder)) {
>>>> -               ret = PTR_ERR(encoder);
>>>> -               dev_err(dev->dev, "failed to construct encoder: %d\n",
>>>> ret);
>>>> -               return ret;
>>>> +               dev_err(dev->dev, "failed to construct encoder\n");
>>>> +               return encoder;
>>>>         }
>>>>
>>>>         encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
>>>>         priv->encoders[priv->num_encoders++] = encoder;
>>>>
>>>> -       if (intf_type == INTF_HDMI) {
>>>> -               ret = hdmi_modeset_init(priv->hdmi, dev, encoder);
>>>> -               if (ret)
>>>> -                       dev_err(dev->dev, "failed to init HDMI: %d\n",
>>>> ret);
>>>> -
>>>> -       } else if (intf_type == INTF_eDP) {
>>>> -               /* Construct bridge/connector for eDP: */
>>>> -               ret = msm_edp_modeset_init(priv->edp, dev, encoder);
>>>> -               if (ret)
>>>> -                       dev_err(dev->dev, "failed to init eDP: %d\n",
>>>> ret);
>>>> -       }
>>>> -
>>>> -       return ret;
>>>> +       return encoder;
>>>>  }
>>>>
>>>>  static int modeset_init(struct mdp5_kms *mdp5_kms)
>>>> @@ -267,26 +254,39 @@ static int modeset_init(struct mdp5_kms
>>>> *mdp5_kms)
>>>>         /* Construct external display interfaces' encoders: */
>>>>         for (i = 0; i < ARRAY_SIZE(hw_cfg->intfs); i++) {
>>>>                 enum mdp5_intf_type intf_type = hw_cfg->intfs[i];
>>>> +               enum mdp5_intf_mode intf_mode = MDP5_INTF_MODE_NONE;
>>>> +               struct msm_drm_sub_dev *sub_dev;
>>>> +               struct drm_encoder *encoder;
>>>>
>>>>                 switch (intf_type) {
>>>>                 case INTF_DISABLED:
>>>> +                       sub_dev = NULL;
>>>>                         break;
>>>>                 case INTF_eDP:
>>>> -                       if (priv->edp)
>>>> -                               ret = construct_encoder(mdp5_kms,
>>>> INTF_eDP, i);
>>>> +                       sub_dev = priv->edp;
>>>>                         break;
>>>>                 case INTF_HDMI:
>>>> -                       if (priv->hdmi)
>>>> -                               ret = construct_encoder(mdp5_kms,
>>>> INTF_HDMI, i);
>>>> +                       sub_dev = priv->hdmi;
>>>>                         break;
>>>>                 default:
>>>>                         dev_err(dev->dev, "unknown intf: %d\n",
>>>> intf_type);
>>>>                         ret = -EINVAL;
>>>> -                       break;
>>>> +                       goto fail;
>>>>                 }
>>>>
>>>> -               if (ret)
>>>> -                       goto fail;
>>>> +               if (sub_dev) {
>>>> +                       encoder = construct_encoder(mdp5_kms,
>>>> intf_type,
>>>> +                                                       i, intf_mode);
>>>> +                       if (IS_ERR(encoder)) {
>>>> +                               ret = PTR_ERR(encoder);
>>>> +                               goto fail;
>>>> +                       }
>>>> +
>>>> +                       sub_dev->encoders[sub_dev->num_encoders++] =
>>>> encoder;
>>>> +                       ret = sub_dev->modeset_init(sub_dev, dev);
>>>> +                       if (ret)
>>>> +                               goto fail;
>>>> +               }
>>>>         }
>>>>
>>>>         return 0;
>>>> diff --git a/drivers/gpu/drm/msm/msm_drv.h
>>>> b/drivers/gpu/drm/msm/msm_drv.h
>>>> index 9e8d441..7b464db 100644
>>>> --- a/drivers/gpu/drm/msm/msm_drv.h
>>>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>>>> @@ -64,6 +64,19 @@ struct msm_file_private {
>>>>         int dummy;
>>>>  };
>>>>
>>>> +/* A base data structure for all MDP sub devices */
>>>> +struct msm_drm_sub_dev {
>>>> +       /*
>>>> +        * the encoders can be used by sub dev,
>>>> +        * must be set before modeset_init
>>>> +        */
>>>> +       unsigned int num_encoders;
>>>> +       struct drm_encoder *encoders[8];
>>>> +
>>>> +       int (*modeset_init)(struct msm_drm_sub_dev *base,
>>>> +               struct drm_device *dev);
>>>> +};
>>>> +
>>>>  struct msm_drm_private {
>>>>
>>>>         struct msm_kms *kms;
>>>> @@ -74,13 +87,13 @@ struct msm_drm_private {
>>>>         /* possibly this should be in the kms component, but it is
>>>>          * shared by both mdp4 and mdp5..
>>>>          */
>>>> -       struct hdmi *hdmi;
>>>> +       struct msm_drm_sub_dev *hdmi;
>>>>
>>>>         /* eDP is for mdp5 only, but kms has not been created
>>>>          * when edp_bind() and edp_init() are called. Here is the only
>>>>          * place to keep the edp instance.
>>>>          */
>>>> -       struct msm_edp *edp;
>>>> +       struct msm_drm_sub_dev *edp;
>>>>
>>>>         /* when we have more than one 'msm_gpu' these need to be an
>>>> array: */
>>>>         struct msm_gpu *gpu;
>>>> @@ -224,17 +237,11 @@ struct drm_framebuffer
>>>> *msm_framebuffer_create(struct drm_device *dev,
>>>>
>>>>  struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev);
>>>>
>>>> -struct hdmi;
>>>> -int hdmi_modeset_init(struct hdmi *hdmi, struct drm_device *dev,
>>>> -               struct drm_encoder *encoder);
>>>>  void __init hdmi_register(void);
>>>>  void __exit hdmi_unregister(void);
>>>>
>>>> -struct msm_edp;
>>>>  void __init msm_edp_register(void);
>>>>  void __exit msm_edp_unregister(void);
>>>> -int msm_edp_modeset_init(struct msm_edp *edp, struct drm_device *dev,
>>>> -               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,
>>>> a Linux Foundation Collaborative Project
>>>>
>>>
>>
>>
>


--
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