lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <59f6e802a2fc814587ed7ad6b023cb63.squirrel@www.codeaurora.org>
Date:	Tue, 24 Mar 2015 15:18:57 -0000
From:	jilaiw@...eaurora.org
To:	"Rob Clark" <robdclark@...il.com>
Cc:	"Jilai Wang" <jilaiw@...eaurora.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"linux-arm-msm" <linux-arm-msm@...r.kernel.org>,
	"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/msm: Refactor msm drm driver by introducing
 msm_drm_sub_dev


> On Thu, Mar 12, 2015 at 4:48 PM, Jilai Wang <jilaiw@...eaurora.org> 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@...eaurora.org>
>> ---
>>  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.
>
> 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@...il.com>
>>   *
>> @@ -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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ