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

From: Rob Clark
Date: Mon Mar 23 2015 - 18:34:14 EST

Next message: Jiri Kosina: "Re: [PATCH v3] kprobes: Disable Kprobe when ftrace arming fails"
Previous message: Sanidhya Kashyap: "missing MS_RDONLY check in fsync"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

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-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: Jiri Kosina: "Re: [PATCH v3] kprobes: Disable Kprobe when ftrace arming fails"
Previous message: Sanidhya Kashyap: "missing MS_RDONLY check in fsync"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]