[PATCH 0/3] drm/msm: Remove drm_driver load/unload ops

[PATCH 0/3] drm/msm: Remove drm_driver load/unload ops

[Archit Taneja]

Registering the drm_device using the drm_driver load/unload ops is
deprecated since it is prone to race conditions.

The second and third patches removes the usage of these ops. The first
patch prevents warnings when we try to remove the drm/msm kernel module.

Archit Taneja (3):
  drm/msm/hdmi: Prevent gpio_free related kernel warnings
  drm/msm: Centralize connector registration/unregistration
  drm/msm: Drop load/unload drm_driver ops

 drivers/gpu/drm/msm/dsi/dsi_manager.c              |  27 ++---
 drivers/gpu/drm/msm/edp/edp_connector.c            |  20 +--
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c          |  36 +++---
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c |  16 +--
 drivers/gpu/drm/msm/msm_drv.c                      | 134 +++++++++++++--------
 5 files changed, 113 insertions(+), 120 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 1/3] drm/msm/hdmi: Prevent gpio_free related kernel warnings

[Archit Taneja]

Calling the legacy gpio_free on an invalid GPIO (a GPIO numbered -1)
results in kernel warnings. This causes a lot of backtraces when
we try to unload the drm/msm module.

Call gpio_free only on valid GPIOs.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
index 26129bf..ce86117 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
@@ -112,13 +112,16 @@ static int gpio_config(struct hdmi *hdmi, bool on)
 		for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
 			struct hdmi_gpio_data gpio = config->gpios[i];
 
-			if (gpio.output) {
-				int value = gpio.value ? 0 : 1;
+			if (gpio.num != -1) {
+				if (gpio.output) {
+					int value = gpio.value ? 0 : 1;
 
-				gpio_set_value_cansleep(gpio.num, value);
-			}
+					gpio_set_value_cansleep(gpio.num,
+								value);
+				}
 
-			gpio_free(gpio.num);
+				gpio_free(gpio.num);
+			}
 		};
 
 		DBG("gpio off");
@@ -126,8 +129,10 @@ static int gpio_config(struct hdmi *hdmi, bool on)
 
 	return 0;
 err:
-	while (i--)
-		gpio_free(config->gpios[i].num);
+	while (i--) {
+		if (config->gpios[i].num != -1)
+			gpio_free(config->gpios[i].num);
+	}
 
 	return ret;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 1/3] drm/msm/hdmi: Prevent gpio_free related kernel warnings

[Bjorn Andersson]

On Tue 19 Apr 03:56 PDT 2016, Archit Taneja wrote:

> Calling the legacy gpio_free on an invalid GPIO (a GPIO numbered -1)
> results in kernel warnings. This causes a lot of backtraces when
> we try to unload the drm/msm module.
> 
> Call gpio_free only on valid GPIOs.
> 
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> index 26129bf..ce86117 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> @@ -112,13 +112,16 @@ static int gpio_config(struct hdmi *hdmi, bool on)
>  		for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
>  			struct hdmi_gpio_data gpio = config->gpios[i];
>  
> -			if (gpio.output) {
> -				int value = gpio.value ? 0 : 1;
> +			if (gpio.num != -1) {
> +				if (gpio.output) {
> +					int value = gpio.value ? 0 : 1;
>  
> -				gpio_set_value_cansleep(gpio.num, value);
> -			}
> +					gpio_set_value_cansleep(gpio.num,
> +								value);
> +				}
>  
> -			gpio_free(gpio.num);
> +				gpio_free(gpio.num);
> +			}
>  		};
>  
>  		DBG("gpio off");
> @@ -126,8 +129,10 @@ static int gpio_config(struct hdmi *hdmi, bool on)
>  
>  	return 0;
>  err:
> -	while (i--)
> -		gpio_free(config->gpios[i].num);
> +	while (i--) {
> +		if (config->gpios[i].num != -1)
> +			gpio_free(config->gpios[i].num);
> +	}
>  
>  	return ret;
>  }

The patch in itself looks good, but the bigger picture does not.

The ddc and hdp should be muxed to the hdmi block, so they should not
operated as gpios.

The mux seems more of a gpio so it should be made more explicit - i.e.
actually support muxing (if that's needed) rather than just setting hard
coded values.

And you should not gpio_request/free the gpio upon every usage, request
it during "probe time" and release it during "remove".

Regards,
Bjorn

Re: [PATCH 1/3] drm/msm/hdmi: Prevent gpio_free related kernel warnings

[Rob Clark]

[-- Attachment #1.1: Type: text/plain, Size: 3171 bytes --]

On Apr 19, 2016 11:50, "Bjorn Andersson" <bjorn.andersson@linaro.org> wrote:
>
> On Tue 19 Apr 03:56 PDT 2016, Archit Taneja wrote:
>
> > Calling the legacy gpio_free on an invalid GPIO (a GPIO numbered -1)
> > results in kernel warnings. This causes a lot of backtraces when
> > we try to unload the drm/msm module.
> >
> > Call gpio_free only on valid GPIOs.
> >
> > Signed-off-by: Archit Taneja <architt@codeaurora.org>
> > ---
> >  drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 19 ++++++++++++-------
> >  1 file changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> > index 26129bf..ce86117 100644
> > --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> > @@ -112,13 +112,16 @@ static int gpio_config(struct hdmi *hdmi, bool on)
> >               for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
> >                       struct hdmi_gpio_data gpio = config->gpios[i];
> >
> > -                     if (gpio.output) {
> > -                             int value = gpio.value ? 0 : 1;
> > +                     if (gpio.num != -1) {
> > +                             if (gpio.output) {
> > +                                     int value = gpio.value ? 0 : 1;
> >
> > -                             gpio_set_value_cansleep(gpio.num, value);
> > -                     }
> > +                                     gpio_set_value_cansleep(gpio.num,
> > +                                                             value);
> > +                             }
> >
> > -                     gpio_free(gpio.num);
> > +                             gpio_free(gpio.num);
> > +                     }
> >               };
> >
> >               DBG("gpio off");
> > @@ -126,8 +129,10 @@ static int gpio_config(struct hdmi *hdmi, bool on)
> >
> >       return 0;
> >  err:
> > -     while (i--)
> > -             gpio_free(config->gpios[i].num);
> > +     while (i--) {
> > +             if (config->gpios[i].num != -1)
> > +                     gpio_free(config->gpios[i].num);
> > +     }
> >
> >       return ret;
> >  }
>
> The patch in itself looks good, but the bigger picture does not.
>
> The ddc and hdp should be muxed to the hdmi block, so they should not
> operated as gpios.
>
> The mux seems more of a gpio so it should be made more explicit - i.e.
> actually support muxing (if that's needed) rather than just setting hard
> coded values.

Note that at least on some devices, hpd was unreliable without using a
combination of gpio and denounced hpd signal from HDMI block...

Not sure what sort of MUX it is but it seemed possible (and necessary) to
use both at same time..

Please be sure to test lots of devices and monitors if you are going to
change this ;-)

BR,
-R

> And you should not gpio_request/free the gpio upon every usage, request
> it during "probe time" and release it during "remove".
>
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm"
in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #1.2: Type: text/html, Size: 4563 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/msm/hdmi: Prevent gpio_free related kernel warnings

[Bjorn Andersson]

On Tue 19 Apr 10:44 PDT 2016, Rob Clark wrote:

>    On Apr 19, 2016 11:50, "Bjorn Andersson" <bjorn.andersson@linaro.org>
>    wrote:
>    >
>    > On Tue 19 Apr 03:56 PDT 2016, Archit Taneja wrote:
>    >
[..]
>    >
>    > The patch in itself looks good, but the bigger picture does not.
>    >
>    > The ddc and hdp should be muxed to the hdmi block, so they should not
>    > operated as gpios.
>    >
>    > The mux seems more of a gpio so it should be made more explicit - i.e.
>    > actually support muxing (if that's needed) rather than just setting hard
>    > coded values.
> 
>    Note that at least on some devices, hpd was unreliable without using a
>    combination of gpio and denounced hpd signal from HDMI block...
> 

Right, I do think it makes sense to keep the detect-gpio.

>    Not sure what sort of MUX it is but it seemed possible (and necessary) to
>    use both at same time..
> 

The "liquid" devices seems to use it, but I haven't managed to figure
out for what.

>    Please be sure to test lots of devices and monitors if you are going to
>    change this ;-)
> 

Scary...


But I do believe that we should stop requesting/freeing the gpios every
time we try to enable/disable them and in the longer run we should drop
the ddc pins, make the hpd an explicit standalone gpiod handle and
figure out what the muxing is all about (and make that standalone as
well).


The patch solves the immediate symptom, but I think we should give it an
overhaul later. If nothing else just to not to carry the downstream
legacy of always gpio_requesting non-gpio pins.

Regards,
Bjorn

Re: [PATCH 1/3] drm/msm/hdmi: Prevent gpio_free related kernel warnings

[Archit Taneja]

Hi Bjorn, Rob,

On 04/19/2016 11:14 PM, Rob Clark wrote:
>
> On Apr 19, 2016 11:50, "Bjorn Andersson" <bjorn.andersson@linaro.org
> <mailto:bjorn.andersson@linaro.org>> wrote:
>  >
>  > On Tue 19 Apr 03:56 PDT 2016, Archit Taneja wrote:
>  >
>  > > Calling the legacy gpio_free on an invalid GPIO (a GPIO numbered -1)
>  > > results in kernel warnings. This causes a lot of backtraces when
>  > > we try to unload the drm/msm module.
>  > >
>  > > Call gpio_free only on valid GPIOs.
>  > >
>  > > Signed-off-by: Archit Taneja <architt@codeaurora.org
> <mailto:architt@codeaurora.org>>
>  > > ---
>  > >  drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 19 ++++++++++++-------
>  > >  1 file changed, 12 insertions(+), 7 deletions(-)
>  > >
>  > > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
>  > > index 26129bf..ce86117 100644
>  > > --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
>  > > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
>  > > @@ -112,13 +112,16 @@ static int gpio_config(struct hdmi *hdmi,
> bool on)
>  > >               for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
>  > >                       struct hdmi_gpio_data gpio = config->gpios[i];
>  > >
>  > > -                     if (gpio.output) {
>  > > -                             int value = gpio.value ? 0 : 1;
>  > > +                     if (gpio.num != -1) {
>  > > +                             if (gpio.output) {
>  > > +                                     int value = gpio.value ? 0 : 1;
>  > >
>  > > -                             gpio_set_value_cansleep(gpio.num, value);
>  > > -                     }
>  > > +                                     gpio_set_value_cansleep(gpio.num,
>  > > +                                                             value);
>  > > +                             }
>  > >
>  > > -                     gpio_free(gpio.num);
>  > > +                             gpio_free(gpio.num);
>  > > +                     }
>  > >               };
>  > >
>  > >               DBG("gpio off");
>  > > @@ -126,8 +129,10 @@ static int gpio_config(struct hdmi *hdmi, bool on)
>  > >
>  > >       return 0;
>  > >  err:
>  > > -     while (i--)
>  > > -             gpio_free(config->gpios[i].num);
>  > > +     while (i--) {
>  > > +             if (config->gpios[i].num != -1)
>  > > +                     gpio_free(config->gpios[i].num);
>  > > +     }
>  > >
>  > >       return ret;
>  > >  }
>  >
>  > The patch in itself looks good, but the bigger picture does not.
>  >
>  > The ddc and hdp should be muxed to the hdmi block, so they should not
>  > operated as gpios.

I agree on the DDC part. I am not sure why they are passed as GPIOs to
the driver. For boards that do this (i.e, 1fc6410), the pinctrl tlmm
node sets the GPIOs function as "hdmi". Does that mean the the
gpio_set_value() in the driver are simply ignored?

I'll test and drop these if things work fine without them.

>  >
>  > The mux seems more of a gpio so it should be made more explicit - i.e.
>  > actually support muxing (if that's needed) rather than just setting hard
>  > coded values.

I have to study the schematics a bit on what the MUX actually does.

>
> Note that at least on some devices, hpd was unreliable without using a
> combination of gpio and denounced hpd signal from HDMI block...

Hmm, one thing I don't get is that the pinctrl DT node for the HPD pin
explicitly assigns its function as "hdmi". If that's the case, how does
it even work as a gpio?

>
> Not sure what sort of MUX it is but it seemed possible (and necessary)
> to use both at same time..
>
> Please be sure to test lots of devices and monitors if you are going to
> change this ;-)

For sure!

>
> BR,
> -R
>
>  > And you should not gpio_request/free the gpio upon every usage, request
>  > it during "probe time" and release it during "remove".

The hpd_enable/disable functions actually get called only during probe
and remove. We don't enable/disable HPD sensing at other points in the
driver. So, I guess it should be okay for now.

Thanks,
Archit

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/msm: Centralize connector registration/unregistration

[Archit Taneja]

Move the drm_connector registration from the encoder(HDMI/DSI etc) drivers
to the msm platform driver. This will simplify the task of ensuring that
the connectors are registered only after the drm_device itself is
registered.

The connectors' destroy ops are made to use kzalloc instead of
devm_kzalloc to ensure that that the connectors can be successfully
unregistered when the msm driver module is removed. The memory for the
connectors is unallocated when drm_mode_config_cleanup() is called
during either during an error or during driver remove.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c              | 27 ++++++++--------------
 drivers/gpu/drm/msm/edp/edp_connector.c            | 20 ++++------------
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c          | 17 +++-----------
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c | 16 ++-----------
 drivers/gpu/drm/msm/msm_drv.c                      | 15 ++++++++++++
 5 files changed, 33 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 58ba7ec..c8d1f19 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -198,9 +198,13 @@ static enum drm_connector_status dsi_mgr_connector_detect(
 
 static void dsi_mgr_connector_destroy(struct drm_connector *connector)
 {
+	struct dsi_connector *dsi_connector = to_dsi_connector(connector);
+
 	DBG("");
-	drm_connector_unregister(connector);
+
 	drm_connector_cleanup(connector);
+
+	kfree(dsi_connector);
 }
 
 static void dsi_dual_connector_fix_modes(struct drm_connector *connector)
@@ -538,12 +542,9 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id)
 	struct dsi_connector *dsi_connector;
 	int ret, i;
 
-	dsi_connector = devm_kzalloc(msm_dsi->dev->dev,
-				sizeof(*dsi_connector), GFP_KERNEL);
-	if (!dsi_connector) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	dsi_connector = kzalloc(sizeof(*dsi_connector), GFP_KERNEL);
+	if (!dsi_connector)
+		return ERR_PTR(-ENOMEM);
 
 	dsi_connector->id = id;
 
@@ -552,7 +553,7 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id)
 	ret = drm_connector_init(msm_dsi->dev, connector,
 			&dsi_mgr_connector_funcs, DRM_MODE_CONNECTOR_DSI);
 	if (ret)
-		goto fail;
+		return ERR_PTR(ret);
 
 	drm_connector_helper_add(connector, &dsi_mgr_conn_helper_funcs);
 
@@ -565,21 +566,11 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id)
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
 
-	ret = drm_connector_register(connector);
-	if (ret)
-		goto fail;
-
 	for (i = 0; i < MSM_DSI_ENCODER_NUM; i++)
 		drm_mode_connector_attach_encoder(connector,
 						msm_dsi->encoders[i]);
 
 	return connector;
-
-fail:
-	if (connector)
-		dsi_mgr_connector_destroy(connector);
-
-	return ERR_PTR(ret);
 }
 
 /* initialize bridge */
diff --git a/drivers/gpu/drm/msm/edp/edp_connector.c b/drivers/gpu/drm/msm/edp/edp_connector.c
index b4d1b46..72360cd 100644
--- a/drivers/gpu/drm/msm/edp/edp_connector.c
+++ b/drivers/gpu/drm/msm/edp/edp_connector.c
@@ -37,7 +37,7 @@ static void edp_connector_destroy(struct drm_connector *connector)
 	struct edp_connector *edp_connector = to_edp_connector(connector);
 
 	DBG("");
-	drm_connector_unregister(connector);
+
 	drm_connector_cleanup(connector);
 
 	kfree(edp_connector);
@@ -124,10 +124,8 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)
 	int ret;
 
 	edp_connector = kzalloc(sizeof(*edp_connector), GFP_KERNEL);
-	if (!edp_connector) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	if (!edp_connector)
+		return ERR_PTR(-ENOMEM);
 
 	edp_connector->edp = edp;
 
@@ -136,7 +134,7 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)
 	ret = drm_connector_init(edp->dev, connector, &edp_connector_funcs,
 			DRM_MODE_CONNECTOR_eDP);
 	if (ret)
-		goto fail;
+		return ERR_PTR(ret);
 
 	drm_connector_helper_add(connector, &edp_connector_helper_funcs);
 
@@ -147,17 +145,7 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)
 	connector->interlace_allowed = false;
 	connector->doublescan_allowed = false;
 
-	ret = drm_connector_register(connector);
-	if (ret)
-		goto fail;
-
 	drm_mode_connector_attach_encoder(connector, edp->encoder);
 
 	return connector;
-
-fail:
-	if (connector)
-		edp_connector_destroy(connector);
-
-	return ERR_PTR(ret);
 }
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
index ce86117..9cc84ce 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
@@ -346,7 +346,6 @@ static void hdmi_connector_destroy(struct drm_connector *connector)
 
 	hdp_disable(hdmi_connector);
 
-	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 
 	kfree(hdmi_connector);
@@ -438,10 +437,8 @@ struct drm_connector *msm_hdmi_connector_init(struct hdmi *hdmi)
 	int ret;
 
 	hdmi_connector = kzalloc(sizeof(*hdmi_connector), GFP_KERNEL);
-	if (!hdmi_connector) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	if (!hdmi_connector)
+		return ERR_PTR(-ENOMEM);
 
 	hdmi_connector->hdmi = hdmi;
 	INIT_WORK(&hdmi_connector->hpd_work, msm_hdmi_hotplug_work);
@@ -458,21 +455,13 @@ struct drm_connector *msm_hdmi_connector_init(struct hdmi *hdmi)
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
 
-	drm_connector_register(connector);
-
 	ret = hpd_enable(hdmi_connector);
 	if (ret) {
 		dev_err(&hdmi->pdev->dev, "failed to enable HPD: %d\n", ret);
-		goto fail;
+		return ERR_PTR(ret);
 	}
 
 	drm_mode_connector_attach_encoder(connector, hdmi->encoder);
 
 	return connector;
-
-fail:
-	if (connector)
-		hdmi_connector_destroy(connector);
-
-	return ERR_PTR(ret);
 }
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
index e73e174..2648cd7 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
@@ -48,7 +48,6 @@ static void mdp4_lvds_connector_destroy(struct drm_connector *connector)
 	struct mdp4_lvds_connector *mdp4_lvds_connector =
 			to_mdp4_lvds_connector(connector);
 
-	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 
 	kfree(mdp4_lvds_connector);
@@ -121,13 +120,10 @@ struct drm_connector *mdp4_lvds_connector_init(struct drm_device *dev,
 {
 	struct drm_connector *connector = NULL;
 	struct mdp4_lvds_connector *mdp4_lvds_connector;
-	int ret;
 
 	mdp4_lvds_connector = kzalloc(sizeof(*mdp4_lvds_connector), GFP_KERNEL);
-	if (!mdp4_lvds_connector) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	if (!mdp4_lvds_connector)
+		return ERR_PTR(-ENOMEM);
 
 	mdp4_lvds_connector->encoder = encoder;
 	mdp4_lvds_connector->panel_node = panel_node;
@@ -143,15 +139,7 @@ struct drm_connector *mdp4_lvds_connector_init(struct drm_device *dev,
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
 
-	drm_connector_register(connector);
-
 	drm_mode_connector_attach_encoder(connector, encoder);
 
 	return connector;
-
-fail:
-	if (connector)
-		mdp4_lvds_connector_destroy(connector);
-
-	return ERR_PTR(ret);
 }
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index c03b967..47f40d9 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -197,6 +197,8 @@ static int msm_unload(struct drm_device *dev)
 
 	drm_kms_helper_poll_fini(dev);
 
+	drm_connector_unregister_all(dev);
+
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	if (fbdev && priv->fbdev)
 		msm_fbdev_free(dev);
@@ -326,6 +328,7 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
 	struct platform_device *pdev = dev->platformdev;
 	struct msm_drm_private *priv;
 	struct msm_kms *kms;
+	struct drm_connector *connector;
 	int ret;
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
@@ -410,6 +413,18 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
 		goto fail;
 	}
 
+	mutex_lock(&dev->mode_config.mutex);
+
+	drm_for_each_connector(connector, dev) {
+		ret = drm_connector_register(connector);
+		if (ret) {
+			mutex_unlock(&dev->mode_config.mutex);
+			goto fail;
+		}
+	}
+
+	mutex_unlock(&dev->mode_config.mutex);
+
 	drm_mode_config_reset(dev);
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/3] drm/msm: Centralize connector registration/unregistration

[Daniel Vetter]

On Tue, Apr 19, 2016 at 04:26:35PM +0530, Archit Taneja wrote:
> Move the drm_connector registration from the encoder(HDMI/DSI etc) drivers
> to the msm platform driver. This will simplify the task of ensuring that
> the connectors are registered only after the drm_device itself is
> registered.
> 
> The connectors' destroy ops are made to use kzalloc instead of
> devm_kzalloc to ensure that that the connectors can be successfully
> unregistered when the msm driver module is removed. The memory for the
> connectors is unallocated when drm_mode_config_cleanup() is called
> during either during an error or during driver remove.
> 
> Signed-off-by: Archit Taneja <architt@codeaurora.org>

There's an in-flight patch series to add a common
connector_(un)register_all(). Would be good to pick that up and rebase on
top.

The patch series is from Alexey Brodkin <Alexey.Brodkin@synopsys.com>.
-Daniel

> ---
>  drivers/gpu/drm/msm/dsi/dsi_manager.c              | 27 ++++++++--------------
>  drivers/gpu/drm/msm/edp/edp_connector.c            | 20 ++++------------
>  drivers/gpu/drm/msm/hdmi/hdmi_connector.c          | 17 +++-----------
>  drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c | 16 ++-----------
>  drivers/gpu/drm/msm/msm_drv.c                      | 15 ++++++++++++
>  5 files changed, 33 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index 58ba7ec..c8d1f19 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -198,9 +198,13 @@ static enum drm_connector_status dsi_mgr_connector_detect(
>  
>  static void dsi_mgr_connector_destroy(struct drm_connector *connector)
>  {
> +	struct dsi_connector *dsi_connector = to_dsi_connector(connector);
> +
>  	DBG("");
> -	drm_connector_unregister(connector);
> +
>  	drm_connector_cleanup(connector);
> +
> +	kfree(dsi_connector);
>  }
>  
>  static void dsi_dual_connector_fix_modes(struct drm_connector *connector)
> @@ -538,12 +542,9 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id)
>  	struct dsi_connector *dsi_connector;
>  	int ret, i;
>  
> -	dsi_connector = devm_kzalloc(msm_dsi->dev->dev,
> -				sizeof(*dsi_connector), GFP_KERNEL);
> -	if (!dsi_connector) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> +	dsi_connector = kzalloc(sizeof(*dsi_connector), GFP_KERNEL);
> +	if (!dsi_connector)
> +		return ERR_PTR(-ENOMEM);
>  
>  	dsi_connector->id = id;
>  
> @@ -552,7 +553,7 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id)
>  	ret = drm_connector_init(msm_dsi->dev, connector,
>  			&dsi_mgr_connector_funcs, DRM_MODE_CONNECTOR_DSI);
>  	if (ret)
> -		goto fail;
> +		return ERR_PTR(ret);
>  
>  	drm_connector_helper_add(connector, &dsi_mgr_conn_helper_funcs);
>  
> @@ -565,21 +566,11 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id)
>  	connector->interlace_allowed = 0;
>  	connector->doublescan_allowed = 0;
>  
> -	ret = drm_connector_register(connector);
> -	if (ret)
> -		goto fail;
> -
>  	for (i = 0; i < MSM_DSI_ENCODER_NUM; i++)
>  		drm_mode_connector_attach_encoder(connector,
>  						msm_dsi->encoders[i]);
>  
>  	return connector;
> -
> -fail:
> -	if (connector)
> -		dsi_mgr_connector_destroy(connector);
> -
> -	return ERR_PTR(ret);
>  }
>  
>  /* initialize bridge */
> diff --git a/drivers/gpu/drm/msm/edp/edp_connector.c b/drivers/gpu/drm/msm/edp/edp_connector.c
> index b4d1b46..72360cd 100644
> --- a/drivers/gpu/drm/msm/edp/edp_connector.c
> +++ b/drivers/gpu/drm/msm/edp/edp_connector.c
> @@ -37,7 +37,7 @@ static void edp_connector_destroy(struct drm_connector *connector)
>  	struct edp_connector *edp_connector = to_edp_connector(connector);
>  
>  	DBG("");
> -	drm_connector_unregister(connector);
> +
>  	drm_connector_cleanup(connector);
>  
>  	kfree(edp_connector);
> @@ -124,10 +124,8 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)
>  	int ret;
>  
>  	edp_connector = kzalloc(sizeof(*edp_connector), GFP_KERNEL);
> -	if (!edp_connector) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> +	if (!edp_connector)
> +		return ERR_PTR(-ENOMEM);
>  
>  	edp_connector->edp = edp;
>  
> @@ -136,7 +134,7 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)
>  	ret = drm_connector_init(edp->dev, connector, &edp_connector_funcs,
>  			DRM_MODE_CONNECTOR_eDP);
>  	if (ret)
> -		goto fail;
> +		return ERR_PTR(ret);
>  
>  	drm_connector_helper_add(connector, &edp_connector_helper_funcs);
>  
> @@ -147,17 +145,7 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)
>  	connector->interlace_allowed = false;
>  	connector->doublescan_allowed = false;
>  
> -	ret = drm_connector_register(connector);
> -	if (ret)
> -		goto fail;
> -
>  	drm_mode_connector_attach_encoder(connector, edp->encoder);
>  
>  	return connector;
> -
> -fail:
> -	if (connector)
> -		edp_connector_destroy(connector);
> -
> -	return ERR_PTR(ret);
>  }
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> index ce86117..9cc84ce 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> @@ -346,7 +346,6 @@ static void hdmi_connector_destroy(struct drm_connector *connector)
>  
>  	hdp_disable(hdmi_connector);
>  
> -	drm_connector_unregister(connector);
>  	drm_connector_cleanup(connector);
>  
>  	kfree(hdmi_connector);
> @@ -438,10 +437,8 @@ struct drm_connector *msm_hdmi_connector_init(struct hdmi *hdmi)
>  	int ret;
>  
>  	hdmi_connector = kzalloc(sizeof(*hdmi_connector), GFP_KERNEL);
> -	if (!hdmi_connector) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> +	if (!hdmi_connector)
> +		return ERR_PTR(-ENOMEM);
>  
>  	hdmi_connector->hdmi = hdmi;
>  	INIT_WORK(&hdmi_connector->hpd_work, msm_hdmi_hotplug_work);
> @@ -458,21 +455,13 @@ struct drm_connector *msm_hdmi_connector_init(struct hdmi *hdmi)
>  	connector->interlace_allowed = 0;
>  	connector->doublescan_allowed = 0;
>  
> -	drm_connector_register(connector);
> -
>  	ret = hpd_enable(hdmi_connector);
>  	if (ret) {
>  		dev_err(&hdmi->pdev->dev, "failed to enable HPD: %d\n", ret);
> -		goto fail;
> +		return ERR_PTR(ret);
>  	}
>  
>  	drm_mode_connector_attach_encoder(connector, hdmi->encoder);
>  
>  	return connector;
> -
> -fail:
> -	if (connector)
> -		hdmi_connector_destroy(connector);
> -
> -	return ERR_PTR(ret);
>  }
> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
> index e73e174..2648cd7 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
> @@ -48,7 +48,6 @@ static void mdp4_lvds_connector_destroy(struct drm_connector *connector)
>  	struct mdp4_lvds_connector *mdp4_lvds_connector =
>  			to_mdp4_lvds_connector(connector);
>  
> -	drm_connector_unregister(connector);
>  	drm_connector_cleanup(connector);
>  
>  	kfree(mdp4_lvds_connector);
> @@ -121,13 +120,10 @@ struct drm_connector *mdp4_lvds_connector_init(struct drm_device *dev,
>  {
>  	struct drm_connector *connector = NULL;
>  	struct mdp4_lvds_connector *mdp4_lvds_connector;
> -	int ret;
>  
>  	mdp4_lvds_connector = kzalloc(sizeof(*mdp4_lvds_connector), GFP_KERNEL);
> -	if (!mdp4_lvds_connector) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> +	if (!mdp4_lvds_connector)
> +		return ERR_PTR(-ENOMEM);
>  
>  	mdp4_lvds_connector->encoder = encoder;
>  	mdp4_lvds_connector->panel_node = panel_node;
> @@ -143,15 +139,7 @@ struct drm_connector *mdp4_lvds_connector_init(struct drm_device *dev,
>  	connector->interlace_allowed = 0;
>  	connector->doublescan_allowed = 0;
>  
> -	drm_connector_register(connector);
> -
>  	drm_mode_connector_attach_encoder(connector, encoder);
>  
>  	return connector;
> -
> -fail:
> -	if (connector)
> -		mdp4_lvds_connector_destroy(connector);
> -
> -	return ERR_PTR(ret);
>  }
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index c03b967..47f40d9 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -197,6 +197,8 @@ static int msm_unload(struct drm_device *dev)
>  
>  	drm_kms_helper_poll_fini(dev);
>  
> +	drm_connector_unregister_all(dev);
> +
>  #ifdef CONFIG_DRM_FBDEV_EMULATION
>  	if (fbdev && priv->fbdev)
>  		msm_fbdev_free(dev);
> @@ -326,6 +328,7 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
>  	struct platform_device *pdev = dev->platformdev;
>  	struct msm_drm_private *priv;
>  	struct msm_kms *kms;
> +	struct drm_connector *connector;
>  	int ret;
>  
>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> @@ -410,6 +413,18 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
>  		goto fail;
>  	}
>  
> +	mutex_lock(&dev->mode_config.mutex);
> +
> +	drm_for_each_connector(connector, dev) {
> +		ret = drm_connector_register(connector);
> +		if (ret) {
> +			mutex_unlock(&dev->mode_config.mutex);
> +			goto fail;
> +		}
> +	}
> +
> +	mutex_unlock(&dev->mode_config.mutex);
> +
>  	drm_mode_config_reset(dev);
>  
>  #ifdef CONFIG_DRM_FBDEV_EMULATION
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/msm: Centralize connector registration/unregistration

[Archit Taneja]

On 04/19/2016 05:40 PM, Daniel Vetter wrote:
> On Tue, Apr 19, 2016 at 04:26:35PM +0530, Archit Taneja wrote:
>> Move the drm_connector registration from the encoder(HDMI/DSI etc) drivers
>> to the msm platform driver. This will simplify the task of ensuring that
>> the connectors are registered only after the drm_device itself is
>> registered.
>>
>> The connectors' destroy ops are made to use kzalloc instead of
>> devm_kzalloc to ensure that that the connectors can be successfully
>> unregistered when the msm driver module is removed. The memory for the
>> connectors is unallocated when drm_mode_config_cleanup() is called
>> during either during an error or during driver remove.
>>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>
> There's an in-flight patch series to add a common
> connector_(un)register_all(). Would be good to pick that up and rebase on
> top.
>
> The patch series is from Alexey Brodkin <Alexey.Brodkin@synopsys.com>.

Cool. I'll do that. Thanks for the tip.

Archit

> -Daniel
>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi_manager.c              | 27 ++++++++--------------
>>   drivers/gpu/drm/msm/edp/edp_connector.c            | 20 ++++------------
>>   drivers/gpu/drm/msm/hdmi/hdmi_connector.c          | 17 +++-----------
>>   drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c | 16 ++-----------
>>   drivers/gpu/drm/msm/msm_drv.c                      | 15 ++++++++++++
>>   5 files changed, 33 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>> index 58ba7ec..c8d1f19 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>> @@ -198,9 +198,13 @@ static enum drm_connector_status dsi_mgr_connector_detect(
>>
>>   static void dsi_mgr_connector_destroy(struct drm_connector *connector)
>>   {
>> +	struct dsi_connector *dsi_connector = to_dsi_connector(connector);
>> +
>>   	DBG("");
>> -	drm_connector_unregister(connector);
>> +
>>   	drm_connector_cleanup(connector);
>> +
>> +	kfree(dsi_connector);
>>   }
>>
>>   static void dsi_dual_connector_fix_modes(struct drm_connector *connector)
>> @@ -538,12 +542,9 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id)
>>   	struct dsi_connector *dsi_connector;
>>   	int ret, i;
>>
>> -	dsi_connector = devm_kzalloc(msm_dsi->dev->dev,
>> -				sizeof(*dsi_connector), GFP_KERNEL);
>> -	if (!dsi_connector) {
>> -		ret = -ENOMEM;
>> -		goto fail;
>> -	}
>> +	dsi_connector = kzalloc(sizeof(*dsi_connector), GFP_KERNEL);
>> +	if (!dsi_connector)
>> +		return ERR_PTR(-ENOMEM);
>>
>>   	dsi_connector->id = id;
>>
>> @@ -552,7 +553,7 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id)
>>   	ret = drm_connector_init(msm_dsi->dev, connector,
>>   			&dsi_mgr_connector_funcs, DRM_MODE_CONNECTOR_DSI);
>>   	if (ret)
>> -		goto fail;
>> +		return ERR_PTR(ret);
>>
>>   	drm_connector_helper_add(connector, &dsi_mgr_conn_helper_funcs);
>>
>> @@ -565,21 +566,11 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id)
>>   	connector->interlace_allowed = 0;
>>   	connector->doublescan_allowed = 0;
>>
>> -	ret = drm_connector_register(connector);
>> -	if (ret)
>> -		goto fail;
>> -
>>   	for (i = 0; i < MSM_DSI_ENCODER_NUM; i++)
>>   		drm_mode_connector_attach_encoder(connector,
>>   						msm_dsi->encoders[i]);
>>
>>   	return connector;
>> -
>> -fail:
>> -	if (connector)
>> -		dsi_mgr_connector_destroy(connector);
>> -
>> -	return ERR_PTR(ret);
>>   }
>>
>>   /* initialize bridge */
>> diff --git a/drivers/gpu/drm/msm/edp/edp_connector.c b/drivers/gpu/drm/msm/edp/edp_connector.c
>> index b4d1b46..72360cd 100644
>> --- a/drivers/gpu/drm/msm/edp/edp_connector.c
>> +++ b/drivers/gpu/drm/msm/edp/edp_connector.c
>> @@ -37,7 +37,7 @@ static void edp_connector_destroy(struct drm_connector *connector)
>>   	struct edp_connector *edp_connector = to_edp_connector(connector);
>>
>>   	DBG("");
>> -	drm_connector_unregister(connector);
>> +
>>   	drm_connector_cleanup(connector);
>>
>>   	kfree(edp_connector);
>> @@ -124,10 +124,8 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)
>>   	int ret;
>>
>>   	edp_connector = kzalloc(sizeof(*edp_connector), GFP_KERNEL);
>> -	if (!edp_connector) {
>> -		ret = -ENOMEM;
>> -		goto fail;
>> -	}
>> +	if (!edp_connector)
>> +		return ERR_PTR(-ENOMEM);
>>
>>   	edp_connector->edp = edp;
>>
>> @@ -136,7 +134,7 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)
>>   	ret = drm_connector_init(edp->dev, connector, &edp_connector_funcs,
>>   			DRM_MODE_CONNECTOR_eDP);
>>   	if (ret)
>> -		goto fail;
>> +		return ERR_PTR(ret);
>>
>>   	drm_connector_helper_add(connector, &edp_connector_helper_funcs);
>>
>> @@ -147,17 +145,7 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)
>>   	connector->interlace_allowed = false;
>>   	connector->doublescan_allowed = false;
>>
>> -	ret = drm_connector_register(connector);
>> -	if (ret)
>> -		goto fail;
>> -
>>   	drm_mode_connector_attach_encoder(connector, edp->encoder);
>>
>>   	return connector;
>> -
>> -fail:
>> -	if (connector)
>> -		edp_connector_destroy(connector);
>> -
>> -	return ERR_PTR(ret);
>>   }
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
>> index ce86117..9cc84ce 100644
>> --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
>> @@ -346,7 +346,6 @@ static void hdmi_connector_destroy(struct drm_connector *connector)
>>
>>   	hdp_disable(hdmi_connector);
>>
>> -	drm_connector_unregister(connector);
>>   	drm_connector_cleanup(connector);
>>
>>   	kfree(hdmi_connector);
>> @@ -438,10 +437,8 @@ struct drm_connector *msm_hdmi_connector_init(struct hdmi *hdmi)
>>   	int ret;
>>
>>   	hdmi_connector = kzalloc(sizeof(*hdmi_connector), GFP_KERNEL);
>> -	if (!hdmi_connector) {
>> -		ret = -ENOMEM;
>> -		goto fail;
>> -	}
>> +	if (!hdmi_connector)
>> +		return ERR_PTR(-ENOMEM);
>>
>>   	hdmi_connector->hdmi = hdmi;
>>   	INIT_WORK(&hdmi_connector->hpd_work, msm_hdmi_hotplug_work);
>> @@ -458,21 +455,13 @@ struct drm_connector *msm_hdmi_connector_init(struct hdmi *hdmi)
>>   	connector->interlace_allowed = 0;
>>   	connector->doublescan_allowed = 0;
>>
>> -	drm_connector_register(connector);
>> -
>>   	ret = hpd_enable(hdmi_connector);
>>   	if (ret) {
>>   		dev_err(&hdmi->pdev->dev, "failed to enable HPD: %d\n", ret);
>> -		goto fail;
>> +		return ERR_PTR(ret);
>>   	}
>>
>>   	drm_mode_connector_attach_encoder(connector, hdmi->encoder);
>>
>>   	return connector;
>> -
>> -fail:
>> -	if (connector)
>> -		hdmi_connector_destroy(connector);
>> -
>> -	return ERR_PTR(ret);
>>   }
>> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
>> index e73e174..2648cd7 100644
>> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
>> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
>> @@ -48,7 +48,6 @@ static void mdp4_lvds_connector_destroy(struct drm_connector *connector)
>>   	struct mdp4_lvds_connector *mdp4_lvds_connector =
>>   			to_mdp4_lvds_connector(connector);
>>
>> -	drm_connector_unregister(connector);
>>   	drm_connector_cleanup(connector);
>>
>>   	kfree(mdp4_lvds_connector);
>> @@ -121,13 +120,10 @@ struct drm_connector *mdp4_lvds_connector_init(struct drm_device *dev,
>>   {
>>   	struct drm_connector *connector = NULL;
>>   	struct mdp4_lvds_connector *mdp4_lvds_connector;
>> -	int ret;
>>
>>   	mdp4_lvds_connector = kzalloc(sizeof(*mdp4_lvds_connector), GFP_KERNEL);
>> -	if (!mdp4_lvds_connector) {
>> -		ret = -ENOMEM;
>> -		goto fail;
>> -	}
>> +	if (!mdp4_lvds_connector)
>> +		return ERR_PTR(-ENOMEM);
>>
>>   	mdp4_lvds_connector->encoder = encoder;
>>   	mdp4_lvds_connector->panel_node = panel_node;
>> @@ -143,15 +139,7 @@ struct drm_connector *mdp4_lvds_connector_init(struct drm_device *dev,
>>   	connector->interlace_allowed = 0;
>>   	connector->doublescan_allowed = 0;
>>
>> -	drm_connector_register(connector);
>> -
>>   	drm_mode_connector_attach_encoder(connector, encoder);
>>
>>   	return connector;
>> -
>> -fail:
>> -	if (connector)
>> -		mdp4_lvds_connector_destroy(connector);
>> -
>> -	return ERR_PTR(ret);
>>   }
>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>> index c03b967..47f40d9 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.c
>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>> @@ -197,6 +197,8 @@ static int msm_unload(struct drm_device *dev)
>>
>>   	drm_kms_helper_poll_fini(dev);
>>
>> +	drm_connector_unregister_all(dev);
>> +
>>   #ifdef CONFIG_DRM_FBDEV_EMULATION
>>   	if (fbdev && priv->fbdev)
>>   		msm_fbdev_free(dev);
>> @@ -326,6 +328,7 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
>>   	struct platform_device *pdev = dev->platformdev;
>>   	struct msm_drm_private *priv;
>>   	struct msm_kms *kms;
>> +	struct drm_connector *connector;
>>   	int ret;
>>
>>   	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> @@ -410,6 +413,18 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
>>   		goto fail;
>>   	}
>>
>> +	mutex_lock(&dev->mode_config.mutex);
>> +
>> +	drm_for_each_connector(connector, dev) {
>> +		ret = drm_connector_register(connector);
>> +		if (ret) {
>> +			mutex_unlock(&dev->mode_config.mutex);
>> +			goto fail;
>> +		}
>> +	}
>> +
>> +	mutex_unlock(&dev->mode_config.mutex);
>> +
>>   	drm_mode_config_reset(dev);
>>
>>   #ifdef CONFIG_DRM_FBDEV_EMULATION
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> hosted by The Linux Foundation
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation

[PATCH 3/3] drm/msm: Drop load/unload drm_driver ops

[Archit Taneja]

The load/unload drm_driver ops are deprecated. They should be removed as
they result in creation of devices visible to userspace even before
the drm_device is registered.

Drop these ops and use drm_dev_alloc/register and drm_dev_unregister/unref
to explicitly create and destroy the drm device in the msm platform
driver's bind and unbind ops. With this in use, the drm connectors are
only registered once the drm_device is registered.

It also fixes the issue of stray debugfs files after the msm module is
removed. With this, all the debugfs files are removed, and allows
successive module insertions/removals.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 129 ++++++++++++++++++++++++------------------
 1 file changed, 73 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 47f40d9..9db4d5c 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -173,13 +173,11 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
 	return 0;
 }
 
-/*
- * DRM operations:
- */
-
-static int msm_unload(struct drm_device *dev)
+static int msm_drm_uninit(struct device *dev)
 {
-	struct msm_drm_private *priv = dev->dev_private;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct drm_device *ddev = platform_get_drvdata(pdev);
+	struct msm_drm_private *priv = ddev->dev_private;
 	struct msm_kms *kms = priv->kms;
 	struct msm_gpu *gpu = priv->gpu;
 	struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl;
@@ -195,33 +193,34 @@ static int msm_unload(struct drm_device *dev)
 		kfree(vbl_ev);
 	}
 
-	drm_kms_helper_poll_fini(dev);
+	drm_kms_helper_poll_fini(ddev);
+
+	drm_connector_unregister_all(ddev);
 
-	drm_connector_unregister_all(dev);
+	drm_dev_unregister(ddev);
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	if (fbdev && priv->fbdev)
-		msm_fbdev_free(dev);
+		msm_fbdev_free(ddev);
 #endif
-	drm_mode_config_cleanup(dev);
-	drm_vblank_cleanup(dev);
+	drm_mode_config_cleanup(ddev);
 
-	pm_runtime_get_sync(dev->dev);
-	drm_irq_uninstall(dev);
-	pm_runtime_put_sync(dev->dev);
+	pm_runtime_get_sync(dev);
+	drm_irq_uninstall(ddev);
+	pm_runtime_put_sync(dev);
 
 	flush_workqueue(priv->wq);
 	destroy_workqueue(priv->wq);
 
 	if (kms) {
-		pm_runtime_disable(dev->dev);
+		pm_runtime_disable(dev);
 		kms->funcs->destroy(kms);
 	}
 
 	if (gpu) {
-		mutex_lock(&dev->struct_mutex);
+		mutex_lock(&ddev->struct_mutex);
 		gpu->funcs->pm_suspend(gpu);
-		mutex_unlock(&dev->struct_mutex);
+		mutex_unlock(&ddev->struct_mutex);
 		gpu->funcs->destroy(gpu);
 	}
 
@@ -229,13 +228,14 @@ static int msm_unload(struct drm_device *dev)
 		DEFINE_DMA_ATTRS(attrs);
 		dma_set_attr(DMA_ATTR_NO_KERNEL_MAPPING, &attrs);
 		drm_mm_takedown(&priv->vram.mm);
-		dma_free_attrs(dev->dev, priv->vram.size, NULL,
-				priv->vram.paddr, &attrs);
+		dma_free_attrs(dev, priv->vram.size, NULL,
+			       priv->vram.paddr, &attrs);
 	}
 
-	component_unbind_all(dev->dev, dev);
+	component_unbind_all(dev, ddev);
 
-	dev->dev_private = NULL;
+	ddev->dev_private = NULL;
+	drm_dev_unref(ddev);
 
 	kfree(priv);
 
@@ -323,21 +323,31 @@ static int msm_init_vram(struct drm_device *dev)
 	return ret;
 }
 
-static int msm_load(struct drm_device *dev, unsigned long flags)
+static int msm_drm_init(struct device *dev, struct drm_driver *drv)
 {
-	struct platform_device *pdev = dev->platformdev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct drm_device *ddev;
 	struct msm_drm_private *priv;
 	struct msm_kms *kms;
 	struct drm_connector *connector;
 	int ret;
 
+	ddev = drm_dev_alloc(drv, dev);
+	if (!ddev) {
+		dev_err(dev, "failed to allocate drm_device\n");
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(pdev, ddev);
+	ddev->platformdev = pdev;
+
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
-		dev_err(dev->dev, "failed to allocate private data\n");
+		drm_dev_unref(ddev);
 		return -ENOMEM;
 	}
 
-	dev->dev_private = priv;
+	ddev->dev_private = priv;
 
 	priv->wq = alloc_ordered_workqueue("msm", 0);
 	init_waitqueue_head(&priv->fence_event);
@@ -349,25 +359,26 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
 	INIT_WORK(&priv->vblank_ctrl.work, vblank_ctrl_worker);
 	spin_lock_init(&priv->vblank_ctrl.lock);
 
-	drm_mode_config_init(dev);
-
-	platform_set_drvdata(pdev, dev);
+	drm_mode_config_init(ddev);
 
 	/* Bind all our sub-components: */
-	ret = component_bind_all(dev->dev, dev);
-	if (ret)
+	ret = component_bind_all(dev, ddev);
+	if (ret) {
+		kfree(priv);
+		drm_dev_unref(ddev);
 		return ret;
+	}
 
-	ret = msm_init_vram(dev);
+	ret = msm_init_vram(ddev);
 	if (ret)
 		goto fail;
 
 	switch (get_mdp_ver(pdev)) {
 	case 4:
-		kms = mdp4_kms_init(dev);
+		kms = mdp4_kms_init(ddev);
 		break;
 	case 5:
-		kms = mdp5_kms_init(dev);
+		kms = mdp5_kms_init(ddev);
 		break;
 	default:
 		kms = ERR_PTR(-ENODEV);
@@ -381,7 +392,7 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
 		 * and (for example) use dmabuf/prime to share buffers with
 		 * imx drm driver on iMX5
 		 */
-		dev_err(dev->dev, "failed to load kms\n");
+		dev_err(dev, "failed to load kms\n");
 		ret = PTR_ERR(kms);
 		goto fail;
 	}
@@ -389,62 +400,70 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
 	priv->kms = kms;
 
 	if (kms) {
-		pm_runtime_enable(dev->dev);
+		pm_runtime_enable(dev);
 		ret = kms->funcs->hw_init(kms);
 		if (ret) {
-			dev_err(dev->dev, "kms hw init failed: %d\n", ret);
+			dev_err(dev, "kms hw init failed: %d\n", ret);
 			goto fail;
 		}
 	}
 
-	dev->mode_config.funcs = &mode_config_funcs;
+	ddev->mode_config.funcs = &mode_config_funcs;
 
-	ret = drm_vblank_init(dev, priv->num_crtcs);
+	ret = drm_vblank_init(ddev, priv->num_crtcs);
 	if (ret < 0) {
-		dev_err(dev->dev, "failed to initialize vblank\n");
+		dev_err(dev, "failed to initialize vblank\n");
 		goto fail;
 	}
 
-	pm_runtime_get_sync(dev->dev);
-	ret = drm_irq_install(dev, platform_get_irq(dev->platformdev, 0));
-	pm_runtime_put_sync(dev->dev);
+	pm_runtime_get_sync(dev);
+	ret = drm_irq_install(ddev, platform_get_irq(pdev, 0));
+	pm_runtime_put_sync(dev);
 	if (ret < 0) {
-		dev_err(dev->dev, "failed to install IRQ handler\n");
+		dev_err(dev, "failed to install IRQ handler\n");
 		goto fail;
 	}
 
-	mutex_lock(&dev->mode_config.mutex);
+	ret = drm_dev_register(ddev, 0);
+	if (ret)
+		goto fail;
+
+	mutex_lock(&ddev->mode_config.mutex);
 
-	drm_for_each_connector(connector, dev) {
+	drm_for_each_connector(connector, ddev) {
 		ret = drm_connector_register(connector);
 		if (ret) {
-			mutex_unlock(&dev->mode_config.mutex);
+			mutex_unlock(&ddev->mode_config.mutex);
 			goto fail;
 		}
 	}
 
-	mutex_unlock(&dev->mode_config.mutex);
+	mutex_unlock(&ddev->mode_config.mutex);
 
-	drm_mode_config_reset(dev);
+	drm_mode_config_reset(ddev);
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	if (fbdev)
-		priv->fbdev = msm_fbdev_init(dev);
+		priv->fbdev = msm_fbdev_init(ddev);
 #endif
 
-	ret = msm_debugfs_late_init(dev);
+	ret = msm_debugfs_late_init(ddev);
 	if (ret)
 		goto fail;
 
-	drm_kms_helper_poll_init(dev);
+	drm_kms_helper_poll_init(ddev);
 
 	return 0;
 
 fail:
-	msm_unload(dev);
+	msm_drm_uninit(dev);
 	return ret;
 }
 
+/*
+ * DRM operations:
+ */
+
 static void load_gpu(struct drm_device *dev)
 {
 	static DEFINE_MUTEX(init_lock);
@@ -967,8 +986,6 @@ static struct drm_driver msm_driver = {
 				DRIVER_RENDER |
 				DRIVER_ATOMIC |
 				DRIVER_MODESET,
-	.load               = msm_load,
-	.unload             = msm_unload,
 	.open               = msm_open,
 	.preclose           = msm_preclose,
 	.lastclose          = msm_lastclose,
@@ -1068,12 +1085,12 @@ static int add_components(struct device *dev, struct component_match **matchptr,
 
 static int msm_drm_bind(struct device *dev)
 {
-	return drm_platform_init(&msm_driver, to_platform_device(dev));
+	return msm_drm_init(dev, &msm_driver);
 }
 
 static void msm_drm_unbind(struct device *dev)
 {
-	drm_put_dev(platform_get_drvdata(to_platform_device(dev)));
+	msm_drm_uninit(dev);
 }
 
 static const struct component_master_ops msm_drm_ops = {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v2 0/3] drm/msm: Remove drm_driver load/unload ops

[Archit Taneja]

Registering the drm_device using the drm_driver load/unload ops is
deprecated since it is prone to race conditions.

The second and third patches removes the usage of these ops. The first
patch prevents warnings when we try to remove the drm/msm kernel module.

Changes in v2:
- Use the recently created drm_connector_register_all and
  drm_connector_unregister_all helpers instead of writing it ourselves

Archit Taneja (3):
  drm/msm/hdmi: Prevent gpio_free related kernel warnings
  drm/msm: Centralize connector registration/unregistration
  drm/msm: Drop load/unload drm_driver ops

 drivers/gpu/drm/msm/dsi/dsi_manager.c              |  27 ++---
 drivers/gpu/drm/msm/edp/edp_connector.c            |  20 +---
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c          |  36 +++---
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c |  16 +--
 drivers/gpu/drm/msm/msm_drv.c                      | 127 ++++++++++++---------
 5 files changed, 106 insertions(+), 120 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v2 1/3] drm/msm/hdmi: Prevent gpio_free related kernel warnings

[Archit Taneja]

Calling the legacy gpio_free on an invalid GPIO (a GPIO numbered -1)
results in kernel warnings. This causes a lot of backtraces when
we try to unload the drm/msm module.

Call gpio_free only on valid GPIOs.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
index 26129bf..ce86117 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
@@ -112,13 +112,16 @@ static int gpio_config(struct hdmi *hdmi, bool on)
 		for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
 			struct hdmi_gpio_data gpio = config->gpios[i];
 
-			if (gpio.output) {
-				int value = gpio.value ? 0 : 1;
+			if (gpio.num != -1) {
+				if (gpio.output) {
+					int value = gpio.value ? 0 : 1;
 
-				gpio_set_value_cansleep(gpio.num, value);
-			}
+					gpio_set_value_cansleep(gpio.num,
+								value);
+				}
 
-			gpio_free(gpio.num);
+				gpio_free(gpio.num);
+			}
 		};
 
 		DBG("gpio off");
@@ -126,8 +129,10 @@ static int gpio_config(struct hdmi *hdmi, bool on)
 
 	return 0;
 err:
-	while (i--)
-		gpio_free(config->gpios[i].num);
+	while (i--) {
+		if (config->gpios[i].num != -1)
+			gpio_free(config->gpios[i].num);
+	}
 
 	return ret;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 1/3] drm/msm/hdmi: Prevent gpio_free related kernel warnings

[twp]

On 2016-04-25 03:16, Archit Taneja wrote:
> Calling the legacy gpio_free on an invalid GPIO (a GPIO numbered -1)
> results in kernel warnings. This causes a lot of backtraces when
> we try to unload the drm/msm module.
> 
> Call gpio_free only on valid GPIOs.
> 
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> index 26129bf..ce86117 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> @@ -112,13 +112,16 @@ static int gpio_config(struct hdmi *hdmi, bool 
> on)
>  		for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
>  			struct hdmi_gpio_data gpio = config->gpios[i];
> 
> -			if (gpio.output) {
> -				int value = gpio.value ? 0 : 1;
> +			if (gpio.num != -1) {
> +				if (gpio.output) {
> +					int value = gpio.value ? 0 : 1;
> 
> -				gpio_set_value_cansleep(gpio.num, value);
> -			}
> +					gpio_set_value_cansleep(gpio.num,
> +								value);
> +				}
> 
> -			gpio_free(gpio.num);
> +				gpio_free(gpio.num);
> +			}

Can you do something like:

if (gpio.num == -1)
     continue;

instead? That would avoid the additional indentation (and increase 
readability).

Thomas

Re: [PATCH v2 1/3] drm/msm/hdmi: Prevent gpio_free related kernel warnings

[Archit Taneja]

On 04/25/2016 11:18 PM, twp@codeaurora.org wrote:
> On 2016-04-25 03:16, Archit Taneja wrote:
>> Calling the legacy gpio_free on an invalid GPIO (a GPIO numbered -1)
>> results in kernel warnings. This causes a lot of backtraces when
>> we try to unload the drm/msm module.
>>
>> Call gpio_free only on valid GPIOs.
>>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 19 ++++++++++++-------
>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
>> b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
>> index 26129bf..ce86117 100644
>> --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
>> @@ -112,13 +112,16 @@ static int gpio_config(struct hdmi *hdmi, bool on)
>>          for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
>>              struct hdmi_gpio_data gpio = config->gpios[i];
>>
>> -            if (gpio.output) {
>> -                int value = gpio.value ? 0 : 1;
>> +            if (gpio.num != -1) {
>> +                if (gpio.output) {
>> +                    int value = gpio.value ? 0 : 1;
>>
>> -                gpio_set_value_cansleep(gpio.num, value);
>> -            }
>> +                    gpio_set_value_cansleep(gpio.num,
>> +                                value);
>> +                }
>>
>> -            gpio_free(gpio.num);
>> +                gpio_free(gpio.num);
>> +            }
>
> Can you do something like:
>
> if (gpio.num == -1)
>      continue;
>
> instead? That would avoid the additional indentation (and increase
> readability).

Yes, that should make things cleaner. I'll make this change.

Thanks,
Archit

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation

[PATCH v2 2/3] drm/msm: Centralize connector registration/unregistration

[Archit Taneja]

Move the drm_connector registration from the encoder(HDMI/DSI etc) drivers
to the msm platform driver. This will simplify the task of ensuring that
the connectors are registered only after the drm_device itself is
registered.

The connectors' destroy ops are made to use kzalloc instead of
devm_kzalloc to ensure that that the connectors can be successfully
unregistered when the msm driver module is removed. The memory for the
connectors is unallocated when drm_mode_config_cleanup() is called
during either during an error or during driver remove.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c              | 27 ++++++++--------------
 drivers/gpu/drm/msm/edp/edp_connector.c            | 20 ++++------------
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c          | 17 +++-----------
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c | 16 ++-----------
 drivers/gpu/drm/msm/msm_drv.c                      |  8 +++++++
 5 files changed, 26 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 58ba7ec..c8d1f19 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -198,9 +198,13 @@ static enum drm_connector_status dsi_mgr_connector_detect(
 
 static void dsi_mgr_connector_destroy(struct drm_connector *connector)
 {
+	struct dsi_connector *dsi_connector = to_dsi_connector(connector);
+
 	DBG("");
-	drm_connector_unregister(connector);
+
 	drm_connector_cleanup(connector);
+
+	kfree(dsi_connector);
 }
 
 static void dsi_dual_connector_fix_modes(struct drm_connector *connector)
@@ -538,12 +542,9 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id)
 	struct dsi_connector *dsi_connector;
 	int ret, i;
 
-	dsi_connector = devm_kzalloc(msm_dsi->dev->dev,
-				sizeof(*dsi_connector), GFP_KERNEL);
-	if (!dsi_connector) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	dsi_connector = kzalloc(sizeof(*dsi_connector), GFP_KERNEL);
+	if (!dsi_connector)
+		return ERR_PTR(-ENOMEM);
 
 	dsi_connector->id = id;
 
@@ -552,7 +553,7 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id)
 	ret = drm_connector_init(msm_dsi->dev, connector,
 			&dsi_mgr_connector_funcs, DRM_MODE_CONNECTOR_DSI);
 	if (ret)
-		goto fail;
+		return ERR_PTR(ret);
 
 	drm_connector_helper_add(connector, &dsi_mgr_conn_helper_funcs);
 
@@ -565,21 +566,11 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id)
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
 
-	ret = drm_connector_register(connector);
-	if (ret)
-		goto fail;
-
 	for (i = 0; i < MSM_DSI_ENCODER_NUM; i++)
 		drm_mode_connector_attach_encoder(connector,
 						msm_dsi->encoders[i]);
 
 	return connector;
-
-fail:
-	if (connector)
-		dsi_mgr_connector_destroy(connector);
-
-	return ERR_PTR(ret);
 }
 
 /* initialize bridge */
diff --git a/drivers/gpu/drm/msm/edp/edp_connector.c b/drivers/gpu/drm/msm/edp/edp_connector.c
index b4d1b46..72360cd 100644
--- a/drivers/gpu/drm/msm/edp/edp_connector.c
+++ b/drivers/gpu/drm/msm/edp/edp_connector.c
@@ -37,7 +37,7 @@ static void edp_connector_destroy(struct drm_connector *connector)
 	struct edp_connector *edp_connector = to_edp_connector(connector);
 
 	DBG("");
-	drm_connector_unregister(connector);
+
 	drm_connector_cleanup(connector);
 
 	kfree(edp_connector);
@@ -124,10 +124,8 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)
 	int ret;
 
 	edp_connector = kzalloc(sizeof(*edp_connector), GFP_KERNEL);
-	if (!edp_connector) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	if (!edp_connector)
+		return ERR_PTR(-ENOMEM);
 
 	edp_connector->edp = edp;
 
@@ -136,7 +134,7 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)
 	ret = drm_connector_init(edp->dev, connector, &edp_connector_funcs,
 			DRM_MODE_CONNECTOR_eDP);
 	if (ret)
-		goto fail;
+		return ERR_PTR(ret);
 
 	drm_connector_helper_add(connector, &edp_connector_helper_funcs);
 
@@ -147,17 +145,7 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)
 	connector->interlace_allowed = false;
 	connector->doublescan_allowed = false;
 
-	ret = drm_connector_register(connector);
-	if (ret)
-		goto fail;
-
 	drm_mode_connector_attach_encoder(connector, edp->encoder);
 
 	return connector;
-
-fail:
-	if (connector)
-		edp_connector_destroy(connector);
-
-	return ERR_PTR(ret);
 }
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
index ce86117..9cc84ce 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
@@ -346,7 +346,6 @@ static void hdmi_connector_destroy(struct drm_connector *connector)
 
 	hdp_disable(hdmi_connector);
 
-	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 
 	kfree(hdmi_connector);
@@ -438,10 +437,8 @@ struct drm_connector *msm_hdmi_connector_init(struct hdmi *hdmi)
 	int ret;
 
 	hdmi_connector = kzalloc(sizeof(*hdmi_connector), GFP_KERNEL);
-	if (!hdmi_connector) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	if (!hdmi_connector)
+		return ERR_PTR(-ENOMEM);
 
 	hdmi_connector->hdmi = hdmi;
 	INIT_WORK(&hdmi_connector->hpd_work, msm_hdmi_hotplug_work);
@@ -458,21 +455,13 @@ struct drm_connector *msm_hdmi_connector_init(struct hdmi *hdmi)
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
 
-	drm_connector_register(connector);
-
 	ret = hpd_enable(hdmi_connector);
 	if (ret) {
 		dev_err(&hdmi->pdev->dev, "failed to enable HPD: %d\n", ret);
-		goto fail;
+		return ERR_PTR(ret);
 	}
 
 	drm_mode_connector_attach_encoder(connector, hdmi->encoder);
 
 	return connector;
-
-fail:
-	if (connector)
-		hdmi_connector_destroy(connector);
-
-	return ERR_PTR(ret);
 }
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
index e73e174..2648cd7 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
@@ -48,7 +48,6 @@ static void mdp4_lvds_connector_destroy(struct drm_connector *connector)
 	struct mdp4_lvds_connector *mdp4_lvds_connector =
 			to_mdp4_lvds_connector(connector);
 
-	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 
 	kfree(mdp4_lvds_connector);
@@ -121,13 +120,10 @@ struct drm_connector *mdp4_lvds_connector_init(struct drm_device *dev,
 {
 	struct drm_connector *connector = NULL;
 	struct mdp4_lvds_connector *mdp4_lvds_connector;
-	int ret;
 
 	mdp4_lvds_connector = kzalloc(sizeof(*mdp4_lvds_connector), GFP_KERNEL);
-	if (!mdp4_lvds_connector) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	if (!mdp4_lvds_connector)
+		return ERR_PTR(-ENOMEM);
 
 	mdp4_lvds_connector->encoder = encoder;
 	mdp4_lvds_connector->panel_node = panel_node;
@@ -143,15 +139,7 @@ struct drm_connector *mdp4_lvds_connector_init(struct drm_device *dev,
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
 
-	drm_connector_register(connector);
-
 	drm_mode_connector_attach_encoder(connector, encoder);
 
 	return connector;
-
-fail:
-	if (connector)
-		mdp4_lvds_connector_destroy(connector);
-
-	return ERR_PTR(ret);
 }
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index c03b967..8c5b257 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -197,6 +197,8 @@ static int msm_unload(struct drm_device *dev)
 
 	drm_kms_helper_poll_fini(dev);
 
+	drm_connector_unregister_all(dev);
+
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	if (fbdev && priv->fbdev)
 		msm_fbdev_free(dev);
@@ -410,6 +412,12 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
 		goto fail;
 	}
 
+	ret = drm_connector_register_all(dev);
+	if (ret) {
+		dev_err(dev->dev, "failed to register connectors\n");
+		goto fail;
+	}
+
 	drm_mode_config_reset(dev);
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v2 3/3] drm/msm: Drop load/unload drm_driver ops

[Archit Taneja]

The load/unload drm_driver ops are deprecated. They should be removed as
they result in creation of devices visible to userspace even before
the drm_device is registered.

Drop these ops and use drm_dev_alloc/register and drm_dev_unregister/unref
to explicitly create and destroy the drm device in the msm platform
driver's bind and unbind ops. With this in use, the drm connectors are
only registered once the drm_device is registered.

It also fixes the issue of stray debugfs files after the msm module is
removed. With this, all the debugfs files are removed, and allows
successive module insertions/removals.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 125 ++++++++++++++++++++++++------------------
 1 file changed, 71 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 8c5b257..a6db6e6 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -173,13 +173,11 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
 	return 0;
 }
 
-/*
- * DRM operations:
- */
-
-static int msm_unload(struct drm_device *dev)
+static int msm_drm_uninit(struct device *dev)
 {
-	struct msm_drm_private *priv = dev->dev_private;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct drm_device *ddev = platform_get_drvdata(pdev);
+	struct msm_drm_private *priv = ddev->dev_private;
 	struct msm_kms *kms = priv->kms;
 	struct msm_gpu *gpu = priv->gpu;
 	struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl;
@@ -195,33 +193,34 @@ static int msm_unload(struct drm_device *dev)
 		kfree(vbl_ev);
 	}
 
-	drm_kms_helper_poll_fini(dev);
+	drm_kms_helper_poll_fini(ddev);
+
+	drm_connector_unregister_all(ddev);
 
-	drm_connector_unregister_all(dev);
+	drm_dev_unregister(ddev);
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	if (fbdev && priv->fbdev)
-		msm_fbdev_free(dev);
+		msm_fbdev_free(ddev);
 #endif
-	drm_mode_config_cleanup(dev);
-	drm_vblank_cleanup(dev);
+	drm_mode_config_cleanup(ddev);
 
-	pm_runtime_get_sync(dev->dev);
-	drm_irq_uninstall(dev);
-	pm_runtime_put_sync(dev->dev);
+	pm_runtime_get_sync(dev);
+	drm_irq_uninstall(ddev);
+	pm_runtime_put_sync(dev);
 
 	flush_workqueue(priv->wq);
 	destroy_workqueue(priv->wq);
 
 	if (kms) {
-		pm_runtime_disable(dev->dev);
+		pm_runtime_disable(dev);
 		kms->funcs->destroy(kms);
 	}
 
 	if (gpu) {
-		mutex_lock(&dev->struct_mutex);
+		mutex_lock(&ddev->struct_mutex);
 		gpu->funcs->pm_suspend(gpu);
-		mutex_unlock(&dev->struct_mutex);
+		mutex_unlock(&ddev->struct_mutex);
 		gpu->funcs->destroy(gpu);
 	}
 
@@ -229,13 +228,14 @@ static int msm_unload(struct drm_device *dev)
 		DEFINE_DMA_ATTRS(attrs);
 		dma_set_attr(DMA_ATTR_NO_KERNEL_MAPPING, &attrs);
 		drm_mm_takedown(&priv->vram.mm);
-		dma_free_attrs(dev->dev, priv->vram.size, NULL,
-				priv->vram.paddr, &attrs);
+		dma_free_attrs(dev, priv->vram.size, NULL,
+			       priv->vram.paddr, &attrs);
 	}
 
-	component_unbind_all(dev->dev, dev);
+	component_unbind_all(dev, ddev);
 
-	dev->dev_private = NULL;
+	ddev->dev_private = NULL;
+	drm_dev_unref(ddev);
 
 	kfree(priv);
 
@@ -323,20 +323,30 @@ static int msm_init_vram(struct drm_device *dev)
 	return ret;
 }
 
-static int msm_load(struct drm_device *dev, unsigned long flags)
+static int msm_drm_init(struct device *dev, struct drm_driver *drv)
 {
-	struct platform_device *pdev = dev->platformdev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct drm_device *ddev;
 	struct msm_drm_private *priv;
 	struct msm_kms *kms;
 	int ret;
 
+	ddev = drm_dev_alloc(drv, dev);
+	if (!ddev) {
+		dev_err(dev, "failed to allocate drm_device\n");
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(pdev, ddev);
+	ddev->platformdev = pdev;
+
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
-		dev_err(dev->dev, "failed to allocate private data\n");
+		drm_dev_unref(ddev);
 		return -ENOMEM;
 	}
 
-	dev->dev_private = priv;
+	ddev->dev_private = priv;
 
 	priv->wq = alloc_ordered_workqueue("msm", 0);
 	init_waitqueue_head(&priv->fence_event);
@@ -348,25 +358,26 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
 	INIT_WORK(&priv->vblank_ctrl.work, vblank_ctrl_worker);
 	spin_lock_init(&priv->vblank_ctrl.lock);
 
-	drm_mode_config_init(dev);
-
-	platform_set_drvdata(pdev, dev);
+	drm_mode_config_init(ddev);
 
 	/* Bind all our sub-components: */
-	ret = component_bind_all(dev->dev, dev);
-	if (ret)
+	ret = component_bind_all(dev, ddev);
+	if (ret) {
+		kfree(priv);
+		drm_dev_unref(ddev);
 		return ret;
+	}
 
-	ret = msm_init_vram(dev);
+	ret = msm_init_vram(ddev);
 	if (ret)
 		goto fail;
 
 	switch (get_mdp_ver(pdev)) {
 	case 4:
-		kms = mdp4_kms_init(dev);
+		kms = mdp4_kms_init(ddev);
 		break;
 	case 5:
-		kms = mdp5_kms_init(dev);
+		kms = mdp5_kms_init(ddev);
 		break;
 	default:
 		kms = ERR_PTR(-ENODEV);
@@ -380,7 +391,7 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
 		 * and (for example) use dmabuf/prime to share buffers with
 		 * imx drm driver on iMX5
 		 */
-		dev_err(dev->dev, "failed to load kms\n");
+		dev_err(dev, "failed to load kms\n");
 		ret = PTR_ERR(kms);
 		goto fail;
 	}
@@ -388,56 +399,64 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
 	priv->kms = kms;
 
 	if (kms) {
-		pm_runtime_enable(dev->dev);
+		pm_runtime_enable(dev);
 		ret = kms->funcs->hw_init(kms);
 		if (ret) {
-			dev_err(dev->dev, "kms hw init failed: %d\n", ret);
+			dev_err(dev, "kms hw init failed: %d\n", ret);
 			goto fail;
 		}
 	}
 
-	dev->mode_config.funcs = &mode_config_funcs;
+	ddev->mode_config.funcs = &mode_config_funcs;
 
-	ret = drm_vblank_init(dev, priv->num_crtcs);
+	ret = drm_vblank_init(ddev, priv->num_crtcs);
 	if (ret < 0) {
-		dev_err(dev->dev, "failed to initialize vblank\n");
+		dev_err(dev, "failed to initialize vblank\n");
 		goto fail;
 	}
 
-	pm_runtime_get_sync(dev->dev);
-	ret = drm_irq_install(dev, platform_get_irq(dev->platformdev, 0));
-	pm_runtime_put_sync(dev->dev);
+	pm_runtime_get_sync(dev);
+	ret = drm_irq_install(ddev, platform_get_irq(pdev, 0));
+	pm_runtime_put_sync(dev);
 	if (ret < 0) {
-		dev_err(dev->dev, "failed to install IRQ handler\n");
+		dev_err(dev, "failed to install IRQ handler\n");
 		goto fail;
 	}
 
-	ret = drm_connector_register_all(dev);
+	ret = drm_dev_register(ddev, 0);
+	if (ret)
+		goto fail;
+
+	ret = drm_connector_register_all(ddev);
 	if (ret) {
-		dev_err(dev->dev, "failed to register connectors\n");
+		dev_err(dev, "failed to register connectors\n");
 		goto fail;
 	}
 
-	drm_mode_config_reset(dev);
+	drm_mode_config_reset(ddev);
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	if (fbdev)
-		priv->fbdev = msm_fbdev_init(dev);
+		priv->fbdev = msm_fbdev_init(ddev);
 #endif
 
-	ret = msm_debugfs_late_init(dev);
+	ret = msm_debugfs_late_init(ddev);
 	if (ret)
 		goto fail;
 
-	drm_kms_helper_poll_init(dev);
+	drm_kms_helper_poll_init(ddev);
 
 	return 0;
 
 fail:
-	msm_unload(dev);
+	msm_drm_uninit(dev);
 	return ret;
 }
 
+/*
+ * DRM operations:
+ */
+
 static void load_gpu(struct drm_device *dev)
 {
 	static DEFINE_MUTEX(init_lock);
@@ -960,8 +979,6 @@ static struct drm_driver msm_driver = {
 				DRIVER_RENDER |
 				DRIVER_ATOMIC |
 				DRIVER_MODESET,
-	.load               = msm_load,
-	.unload             = msm_unload,
 	.open               = msm_open,
 	.preclose           = msm_preclose,
 	.lastclose          = msm_lastclose,
@@ -1061,12 +1078,12 @@ static int add_components(struct device *dev, struct component_match **matchptr,
 
 static int msm_drm_bind(struct device *dev)
 {
-	return drm_platform_init(&msm_driver, to_platform_device(dev));
+	return msm_drm_init(dev, &msm_driver);
 }
 
 static void msm_drm_unbind(struct device *dev)
 {
-	drm_put_dev(platform_get_drvdata(to_platform_device(dev)));
+	msm_drm_uninit(dev);
 }
 
 static const struct component_master_ops msm_drm_ops = {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v3 0/3] drm/msm: Remove drm_driver load/unload ops

[Archit Taneja]

Registering the drm_device using the drm_driver load/unload ops is
deprecated since it is prone to race conditions.

The second and third patches removes the usage of these ops. The first
patch prevents warnings when we try to remove the drm/msm kernel module.

Changes in v3:
- Minor change in the gpio_free warning removal patch to improve
  indentation

Changes in v2:
- Use the recently created drm_connector_register_all and
  drm_connector_unregister_all helpers instead of writing it ourselves

Archit Taneja (3):
  drm/msm/hdmi: Prevent gpio_free related kernel warnings
  drm/msm: Centralize connector registration/unregistration
  drm/msm: Drop load/unload drm_driver ops

 drivers/gpu/drm/msm/dsi/dsi_manager.c              |  27 ++---
 drivers/gpu/drm/msm/edp/edp_connector.c            |  20 +---
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c          |  26 ++---
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c |  16 +--
 drivers/gpu/drm/msm/msm_drv.c                      | 127 ++++++++++++---------
 5 files changed, 101 insertions(+), 115 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v3 1/3] drm/msm/hdmi: Prevent gpio_free related kernel warnings

[Archit Taneja]

Calling the legacy gpio_free on an invalid GPIO (a GPIO numbered -1)
results in kernel warnings. This causes a lot of backtraces when
we try to unload the drm/msm module.

Call gpio_free only on valid GPIOs.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
index 26129bf..e350b2e 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
@@ -112,6 +112,9 @@ static int gpio_config(struct hdmi *hdmi, bool on)
 		for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
 			struct hdmi_gpio_data gpio = config->gpios[i];
 
+			if (gpio.num == -1)
+				continue;
+
 			if (gpio.output) {
 				int value = gpio.value ? 0 : 1;
 
@@ -126,8 +129,10 @@ static int gpio_config(struct hdmi *hdmi, bool on)
 
 	return 0;
 err:
-	while (i--)
-		gpio_free(config->gpios[i].num);
+	while (i--) {
+		if (config->gpios[i].num != -1)
+			gpio_free(config->gpios[i].num);
+	}
 
 	return ret;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 2/3] drm/msm: Centralize connector registration/unregistration

[Archit Taneja]

Move the drm_connector registration from the encoder(HDMI/DSI etc) drivers
to the msm platform driver. This will simplify the task of ensuring that
the connectors are registered only after the drm_device itself is
registered.

The connectors' destroy ops are made to use kzalloc instead of
devm_kzalloc to ensure that that the connectors can be successfully
unregistered when the msm driver module is removed. The memory for the
connectors is unallocated when drm_mode_config_cleanup() is called
during either during an error or during driver remove.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c              | 27 ++++++++--------------
 drivers/gpu/drm/msm/edp/edp_connector.c            | 20 ++++------------
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c          | 17 +++-----------
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c | 16 ++-----------
 drivers/gpu/drm/msm/msm_drv.c                      |  8 +++++++
 5 files changed, 26 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 58ba7ec..c8d1f19 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -198,9 +198,13 @@ static enum drm_connector_status dsi_mgr_connector_detect(
 
 static void dsi_mgr_connector_destroy(struct drm_connector *connector)
 {
+	struct dsi_connector *dsi_connector = to_dsi_connector(connector);
+
 	DBG("");
-	drm_connector_unregister(connector);
+
 	drm_connector_cleanup(connector);
+
+	kfree(dsi_connector);
 }
 
 static void dsi_dual_connector_fix_modes(struct drm_connector *connector)
@@ -538,12 +542,9 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id)
 	struct dsi_connector *dsi_connector;
 	int ret, i;
 
-	dsi_connector = devm_kzalloc(msm_dsi->dev->dev,
-				sizeof(*dsi_connector), GFP_KERNEL);
-	if (!dsi_connector) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	dsi_connector = kzalloc(sizeof(*dsi_connector), GFP_KERNEL);
+	if (!dsi_connector)
+		return ERR_PTR(-ENOMEM);
 
 	dsi_connector->id = id;
 
@@ -552,7 +553,7 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id)
 	ret = drm_connector_init(msm_dsi->dev, connector,
 			&dsi_mgr_connector_funcs, DRM_MODE_CONNECTOR_DSI);
 	if (ret)
-		goto fail;
+		return ERR_PTR(ret);
 
 	drm_connector_helper_add(connector, &dsi_mgr_conn_helper_funcs);
 
@@ -565,21 +566,11 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id)
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
 
-	ret = drm_connector_register(connector);
-	if (ret)
-		goto fail;
-
 	for (i = 0; i < MSM_DSI_ENCODER_NUM; i++)
 		drm_mode_connector_attach_encoder(connector,
 						msm_dsi->encoders[i]);
 
 	return connector;
-
-fail:
-	if (connector)
-		dsi_mgr_connector_destroy(connector);
-
-	return ERR_PTR(ret);
 }
 
 /* initialize bridge */
diff --git a/drivers/gpu/drm/msm/edp/edp_connector.c b/drivers/gpu/drm/msm/edp/edp_connector.c
index b4d1b46..72360cd 100644
--- a/drivers/gpu/drm/msm/edp/edp_connector.c
+++ b/drivers/gpu/drm/msm/edp/edp_connector.c
@@ -37,7 +37,7 @@ static void edp_connector_destroy(struct drm_connector *connector)
 	struct edp_connector *edp_connector = to_edp_connector(connector);
 
 	DBG("");
-	drm_connector_unregister(connector);
+
 	drm_connector_cleanup(connector);
 
 	kfree(edp_connector);
@@ -124,10 +124,8 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)
 	int ret;
 
 	edp_connector = kzalloc(sizeof(*edp_connector), GFP_KERNEL);
-	if (!edp_connector) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	if (!edp_connector)
+		return ERR_PTR(-ENOMEM);
 
 	edp_connector->edp = edp;
 
@@ -136,7 +134,7 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)
 	ret = drm_connector_init(edp->dev, connector, &edp_connector_funcs,
 			DRM_MODE_CONNECTOR_eDP);
 	if (ret)
-		goto fail;
+		return ERR_PTR(ret);
 
 	drm_connector_helper_add(connector, &edp_connector_helper_funcs);
 
@@ -147,17 +145,7 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)
 	connector->interlace_allowed = false;
 	connector->doublescan_allowed = false;
 
-	ret = drm_connector_register(connector);
-	if (ret)
-		goto fail;
-
 	drm_mode_connector_attach_encoder(connector, edp->encoder);
 
 	return connector;
-
-fail:
-	if (connector)
-		edp_connector_destroy(connector);
-
-	return ERR_PTR(ret);
 }
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
index e350b2e..b15d726 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
@@ -346,7 +346,6 @@ static void hdmi_connector_destroy(struct drm_connector *connector)
 
 	hdp_disable(hdmi_connector);
 
-	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 
 	kfree(hdmi_connector);
@@ -438,10 +437,8 @@ struct drm_connector *msm_hdmi_connector_init(struct hdmi *hdmi)
 	int ret;
 
 	hdmi_connector = kzalloc(sizeof(*hdmi_connector), GFP_KERNEL);
-	if (!hdmi_connector) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	if (!hdmi_connector)
+		return ERR_PTR(-ENOMEM);
 
 	hdmi_connector->hdmi = hdmi;
 	INIT_WORK(&hdmi_connector->hpd_work, msm_hdmi_hotplug_work);
@@ -458,21 +455,13 @@ struct drm_connector *msm_hdmi_connector_init(struct hdmi *hdmi)
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
 
-	drm_connector_register(connector);
-
 	ret = hpd_enable(hdmi_connector);
 	if (ret) {
 		dev_err(&hdmi->pdev->dev, "failed to enable HPD: %d\n", ret);
-		goto fail;
+		return ERR_PTR(ret);
 	}
 
 	drm_mode_connector_attach_encoder(connector, hdmi->encoder);
 
 	return connector;
-
-fail:
-	if (connector)
-		hdmi_connector_destroy(connector);
-
-	return ERR_PTR(ret);
 }
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
index e73e174..2648cd7 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
@@ -48,7 +48,6 @@ static void mdp4_lvds_connector_destroy(struct drm_connector *connector)
 	struct mdp4_lvds_connector *mdp4_lvds_connector =
 			to_mdp4_lvds_connector(connector);
 
-	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 
 	kfree(mdp4_lvds_connector);
@@ -121,13 +120,10 @@ struct drm_connector *mdp4_lvds_connector_init(struct drm_device *dev,
 {
 	struct drm_connector *connector = NULL;
 	struct mdp4_lvds_connector *mdp4_lvds_connector;
-	int ret;
 
 	mdp4_lvds_connector = kzalloc(sizeof(*mdp4_lvds_connector), GFP_KERNEL);
-	if (!mdp4_lvds_connector) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	if (!mdp4_lvds_connector)
+		return ERR_PTR(-ENOMEM);
 
 	mdp4_lvds_connector->encoder = encoder;
 	mdp4_lvds_connector->panel_node = panel_node;
@@ -143,15 +139,7 @@ struct drm_connector *mdp4_lvds_connector_init(struct drm_device *dev,
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
 
-	drm_connector_register(connector);
-
 	drm_mode_connector_attach_encoder(connector, encoder);
 
 	return connector;
-
-fail:
-	if (connector)
-		mdp4_lvds_connector_destroy(connector);
-
-	return ERR_PTR(ret);
 }
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index c03b967..8c5b257 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -197,6 +197,8 @@ static int msm_unload(struct drm_device *dev)
 
 	drm_kms_helper_poll_fini(dev);
 
+	drm_connector_unregister_all(dev);
+
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	if (fbdev && priv->fbdev)
 		msm_fbdev_free(dev);
@@ -410,6 +412,12 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
 		goto fail;
 	}
 
+	ret = drm_connector_register_all(dev);
+	if (ret) {
+		dev_err(dev->dev, "failed to register connectors\n");
+		goto fail;
+	}
+
 	drm_mode_config_reset(dev);
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 3/3] drm/msm: Drop load/unload drm_driver ops

[Archit Taneja]

The load/unload drm_driver ops are deprecated. They should be removed as
they result in creation of devices visible to userspace even before
the drm_device is registered.

Drop these ops and use drm_dev_alloc/register and drm_dev_unregister/unref
to explicitly create and destroy the drm device in the msm platform
driver's bind and unbind ops. With this in use, the drm connectors are
only registered once the drm_device is registered.

It also fixes the issue of stray debugfs files after the msm module is
removed. With this, all the debugfs files are removed, and allows
successive module insertions/removals.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 125 ++++++++++++++++++++++++------------------
 1 file changed, 71 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 8c5b257..a6db6e6 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -173,13 +173,11 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
 	return 0;
 }
 
-/*
- * DRM operations:
- */
-
-static int msm_unload(struct drm_device *dev)
+static int msm_drm_uninit(struct device *dev)
 {
-	struct msm_drm_private *priv = dev->dev_private;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct drm_device *ddev = platform_get_drvdata(pdev);
+	struct msm_drm_private *priv = ddev->dev_private;
 	struct msm_kms *kms = priv->kms;
 	struct msm_gpu *gpu = priv->gpu;
 	struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl;
@@ -195,33 +193,34 @@ static int msm_unload(struct drm_device *dev)
 		kfree(vbl_ev);
 	}
 
-	drm_kms_helper_poll_fini(dev);
+	drm_kms_helper_poll_fini(ddev);
+
+	drm_connector_unregister_all(ddev);
 
-	drm_connector_unregister_all(dev);
+	drm_dev_unregister(ddev);
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	if (fbdev && priv->fbdev)
-		msm_fbdev_free(dev);
+		msm_fbdev_free(ddev);
 #endif
-	drm_mode_config_cleanup(dev);
-	drm_vblank_cleanup(dev);
+	drm_mode_config_cleanup(ddev);
 
-	pm_runtime_get_sync(dev->dev);
-	drm_irq_uninstall(dev);
-	pm_runtime_put_sync(dev->dev);
+	pm_runtime_get_sync(dev);
+	drm_irq_uninstall(ddev);
+	pm_runtime_put_sync(dev);
 
 	flush_workqueue(priv->wq);
 	destroy_workqueue(priv->wq);
 
 	if (kms) {
-		pm_runtime_disable(dev->dev);
+		pm_runtime_disable(dev);
 		kms->funcs->destroy(kms);
 	}
 
 	if (gpu) {
-		mutex_lock(&dev->struct_mutex);
+		mutex_lock(&ddev->struct_mutex);
 		gpu->funcs->pm_suspend(gpu);
-		mutex_unlock(&dev->struct_mutex);
+		mutex_unlock(&ddev->struct_mutex);
 		gpu->funcs->destroy(gpu);
 	}
 
@@ -229,13 +228,14 @@ static int msm_unload(struct drm_device *dev)
 		DEFINE_DMA_ATTRS(attrs);
 		dma_set_attr(DMA_ATTR_NO_KERNEL_MAPPING, &attrs);
 		drm_mm_takedown(&priv->vram.mm);
-		dma_free_attrs(dev->dev, priv->vram.size, NULL,
-				priv->vram.paddr, &attrs);
+		dma_free_attrs(dev, priv->vram.size, NULL,
+			       priv->vram.paddr, &attrs);
 	}
 
-	component_unbind_all(dev->dev, dev);
+	component_unbind_all(dev, ddev);
 
-	dev->dev_private = NULL;
+	ddev->dev_private = NULL;
+	drm_dev_unref(ddev);
 
 	kfree(priv);
 
@@ -323,20 +323,30 @@ static int msm_init_vram(struct drm_device *dev)
 	return ret;
 }
 
-static int msm_load(struct drm_device *dev, unsigned long flags)
+static int msm_drm_init(struct device *dev, struct drm_driver *drv)
 {
-	struct platform_device *pdev = dev->platformdev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct drm_device *ddev;
 	struct msm_drm_private *priv;
 	struct msm_kms *kms;
 	int ret;
 
+	ddev = drm_dev_alloc(drv, dev);
+	if (!ddev) {
+		dev_err(dev, "failed to allocate drm_device\n");
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(pdev, ddev);
+	ddev->platformdev = pdev;
+
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
-		dev_err(dev->dev, "failed to allocate private data\n");
+		drm_dev_unref(ddev);
 		return -ENOMEM;
 	}
 
-	dev->dev_private = priv;
+	ddev->dev_private = priv;
 
 	priv->wq = alloc_ordered_workqueue("msm", 0);
 	init_waitqueue_head(&priv->fence_event);
@@ -348,25 +358,26 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
 	INIT_WORK(&priv->vblank_ctrl.work, vblank_ctrl_worker);
 	spin_lock_init(&priv->vblank_ctrl.lock);
 
-	drm_mode_config_init(dev);
-
-	platform_set_drvdata(pdev, dev);
+	drm_mode_config_init(ddev);
 
 	/* Bind all our sub-components: */
-	ret = component_bind_all(dev->dev, dev);
-	if (ret)
+	ret = component_bind_all(dev, ddev);
+	if (ret) {
+		kfree(priv);
+		drm_dev_unref(ddev);
 		return ret;
+	}
 
-	ret = msm_init_vram(dev);
+	ret = msm_init_vram(ddev);
 	if (ret)
 		goto fail;
 
 	switch (get_mdp_ver(pdev)) {
 	case 4:
-		kms = mdp4_kms_init(dev);
+		kms = mdp4_kms_init(ddev);
 		break;
 	case 5:
-		kms = mdp5_kms_init(dev);
+		kms = mdp5_kms_init(ddev);
 		break;
 	default:
 		kms = ERR_PTR(-ENODEV);
@@ -380,7 +391,7 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
 		 * and (for example) use dmabuf/prime to share buffers with
 		 * imx drm driver on iMX5
 		 */
-		dev_err(dev->dev, "failed to load kms\n");
+		dev_err(dev, "failed to load kms\n");
 		ret = PTR_ERR(kms);
 		goto fail;
 	}
@@ -388,56 +399,64 @@ static int msm_load(struct drm_device *dev, unsigned long flags)
 	priv->kms = kms;
 
 	if (kms) {
-		pm_runtime_enable(dev->dev);
+		pm_runtime_enable(dev);
 		ret = kms->funcs->hw_init(kms);
 		if (ret) {
-			dev_err(dev->dev, "kms hw init failed: %d\n", ret);
+			dev_err(dev, "kms hw init failed: %d\n", ret);
 			goto fail;
 		}
 	}
 
-	dev->mode_config.funcs = &mode_config_funcs;
+	ddev->mode_config.funcs = &mode_config_funcs;
 
-	ret = drm_vblank_init(dev, priv->num_crtcs);
+	ret = drm_vblank_init(ddev, priv->num_crtcs);
 	if (ret < 0) {
-		dev_err(dev->dev, "failed to initialize vblank\n");
+		dev_err(dev, "failed to initialize vblank\n");
 		goto fail;
 	}
 
-	pm_runtime_get_sync(dev->dev);
-	ret = drm_irq_install(dev, platform_get_irq(dev->platformdev, 0));
-	pm_runtime_put_sync(dev->dev);
+	pm_runtime_get_sync(dev);
+	ret = drm_irq_install(ddev, platform_get_irq(pdev, 0));
+	pm_runtime_put_sync(dev);
 	if (ret < 0) {
-		dev_err(dev->dev, "failed to install IRQ handler\n");
+		dev_err(dev, "failed to install IRQ handler\n");
 		goto fail;
 	}
 
-	ret = drm_connector_register_all(dev);
+	ret = drm_dev_register(ddev, 0);
+	if (ret)
+		goto fail;
+
+	ret = drm_connector_register_all(ddev);
 	if (ret) {
-		dev_err(dev->dev, "failed to register connectors\n");
+		dev_err(dev, "failed to register connectors\n");
 		goto fail;
 	}
 
-	drm_mode_config_reset(dev);
+	drm_mode_config_reset(ddev);
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	if (fbdev)
-		priv->fbdev = msm_fbdev_init(dev);
+		priv->fbdev = msm_fbdev_init(ddev);
 #endif
 
-	ret = msm_debugfs_late_init(dev);
+	ret = msm_debugfs_late_init(ddev);
 	if (ret)
 		goto fail;
 
-	drm_kms_helper_poll_init(dev);
+	drm_kms_helper_poll_init(ddev);
 
 	return 0;
 
 fail:
-	msm_unload(dev);
+	msm_drm_uninit(dev);
 	return ret;
 }
 
+/*
+ * DRM operations:
+ */
+
 static void load_gpu(struct drm_device *dev)
 {
 	static DEFINE_MUTEX(init_lock);
@@ -960,8 +979,6 @@ static struct drm_driver msm_driver = {
 				DRIVER_RENDER |
 				DRIVER_ATOMIC |
 				DRIVER_MODESET,
-	.load               = msm_load,
-	.unload             = msm_unload,
 	.open               = msm_open,
 	.preclose           = msm_preclose,
 	.lastclose          = msm_lastclose,
@@ -1061,12 +1078,12 @@ static int add_components(struct device *dev, struct component_match **matchptr,
 
 static int msm_drm_bind(struct device *dev)
 {
-	return drm_platform_init(&msm_driver, to_platform_device(dev));
+	return msm_drm_init(dev, &msm_driver);
 }
 
 static void msm_drm_unbind(struct device *dev)
 {
-	drm_put_dev(platform_get_drvdata(to_platform_device(dev)));
+	msm_drm_uninit(dev);
 }
 
 static const struct component_master_ops msm_drm_ops = {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation