Re: [PATCH] drm: omap: fix: Defer probe if an omapdss device requests for it at connect

Re: [PATCH] drm: omap: fix: Defer probe if an omapdss device requests for it at connect

[Archit Taneja]

On Wednesday 18 September 2013 04:38 PM, Archit Taneja wrote:
> Some omapdss panels are connected to outputs/encoders(HDMI/DSI/DPI) that require
> regulators. The output's connect op tries to get a regulator which may not exist
> yet because it might get registered later in the kernel boot.
>
> omapdrm currently ignores those panels which return a non zero value when
> connected. A better approach would be for omapdrm to request for probe
> deferral if a panel's connect op returns -EPROBE_DEFER.
>
> The connecting of panels is moved very early in the the drm device's probe
> before anything else is initialized. When we enter omap_modeset_init(), we have
> a set of panels that have been connected. We now proceed with registering only
> those panels which are already connected.
>
> Checking whether the panel has a driver or whether it has get_timing/read_edid
> ops in omap_modeset_init() are redundant with the new display model. These can
> be removed since a dssdev device will always have a driver associated with it,
> and all dssdev drivers have a get_timings op.
>
> This fixes boot with omapdrm on an omap4 panda ES board. The regulators used by
> HDMI aren't initialized because I2c isn't initialized, I2C isn't initialized
> as it's pins are not configured because pinctrl is yet to probe.

Copying dri-devel list.

>
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
>   drivers/gpu/drm/omapdrm/omap_crtc.c |  5 ++++
>   drivers/gpu/drm/omapdrm/omap_drv.c  | 51 +++++++++++++++++++++----------------
>   drivers/gpu/drm/omapdrm/omap_drv.h  |  1 +
>   3 files changed, 35 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 0fd2eb1..9c01311 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -623,6 +623,11 @@ void omap_crtc_pre_init(void)
>   	dss_install_mgr_ops(&mgr_ops);
>   }
>
> +void omap_crtc_pre_uninit(void)
> +{
> +	dss_uninstall_mgr_ops();
> +}
> +
>   /* initialize crtc */
>   struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>   		struct drm_plane *plane, enum omap_channel channel, int id)
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 2603d90..cbe5d8e 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -87,6 +87,24 @@ static bool channel_used(struct drm_device *dev, enum omap_channel channel)
>   	return false;
>   }
>
> +static int omap_connect_dssdevs(void)
> +{
> +	int r;
> +	struct omap_dss_device *dssdev = NULL;
> +
> +	for_each_dss_dev(dssdev) {
> +		r = dssdev->driver->connect(dssdev);
> +		if (r == -EPROBE_DEFER) {
> +			return r;
> +		} else if (r) {
> +			dev_warn(dssdev->dev, "could not connect display: %s\n",
> +				dssdev->name);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   static int omap_modeset_init(struct drm_device *dev)
>   {
>   	struct omap_drm_private *priv = dev->dev_private;
> @@ -95,9 +113,6 @@ static int omap_modeset_init(struct drm_device *dev)
>   	int num_mgrs = dss_feat_get_num_mgrs();
>   	int num_crtcs;
>   	int i, id = 0;
> -	int r;
> -
> -	omap_crtc_pre_init();
>
>   	drm_mode_config_init(dev);
>
> @@ -119,26 +134,8 @@ static int omap_modeset_init(struct drm_device *dev)
>   		enum omap_channel channel;
>   		struct omap_overlay_manager *mgr;
>
> -		if (!dssdev->driver) {
> -			dev_warn(dev->dev, "%s has no driver.. skipping it\n",
> -					dssdev->name);
> +		if (!omapdss_device_is_connected(dssdev))
>   			continue;
> -		}
> -
> -		if (!(dssdev->driver->get_timings ||
> -					dssdev->driver->read_edid)) {
> -			dev_warn(dev->dev, "%s driver does not support "
> -				"get_timings or read_edid.. skipping it!\n",
> -				dssdev->name);
> -			continue;
> -		}
> -
> -		r = dssdev->driver->connect(dssdev);
> -		if (r) {
> -			dev_err(dev->dev, "could not connect display: %s\n",
> -					dssdev->name);
> -			continue;
> -		}
>
>   		encoder = omap_encoder_init(dev, dssdev);
>
> @@ -656,9 +653,19 @@ static void pdev_shutdown(struct platform_device *device)
>
>   static int pdev_probe(struct platform_device *device)
>   {
> +	int r;
> +
>   	if (omapdss_is_initialized() == false)
>   		return -EPROBE_DEFER;
>
> +	omap_crtc_pre_init();
> +
> +	r = omap_connect_dssdevs();
> +	if (r) {
> +		omap_crtc_pre_uninit();
> +		return r;
> +	}
> +
>   	DBG("%s", device->name);
>   	return drm_platform_init(&omap_drm_driver, device);
>   }
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
> index 30b95b7..cb86cb3 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -158,6 +158,7 @@ enum omap_channel omap_crtc_channel(struct drm_crtc *crtc);
>   int omap_crtc_apply(struct drm_crtc *crtc,
>   		struct omap_drm_apply *apply);
>   void omap_crtc_pre_init(void);
> +void omap_crtc_pre_uninit(void);
>   struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>   		struct drm_plane *plane, enum omap_channel channel, int id);
>
>

Re: [PATCH] drm: omap: fix: Defer probe if an omapdss device requests for it at connect

[Archit Taneja]

On Wednesday 18 September 2013 06:11 PM, Tomi Valkeinen wrote:
> On 18/09/13 14:08, Archit Taneja wrote:
>> Some omapdss panels are connected to outputs/encoders(HDMI/DSI/DPI) that require
>> regulators. The output's connect op tries to get a regulator which may not exist
>> yet because it might get registered later in the kernel boot.
>>
>> omapdrm currently ignores those panels which return a non zero value when
>> connected. A better approach would be for omapdrm to request for probe
>> deferral if a panel's connect op returns -EPROBE_DEFER.
>>
>> The connecting of panels is moved very early in the the drm device's probe
>> before anything else is initialized. When we enter omap_modeset_init(), we have
>> a set of panels that have been connected. We now proceed with registering only
>> those panels which are already connected.
>>
>> Checking whether the panel has a driver or whether it has get_timing/read_edid
>> ops in omap_modeset_init() are redundant with the new display model. These can
>> be removed since a dssdev device will always have a driver associated with it,
>> and all dssdev drivers have a get_timings op.
>>
>> This fixes boot with omapdrm on an omap4 panda ES board. The regulators used by
>> HDMI aren't initialized because I2c isn't initialized, I2C isn't initialized
>> as it's pins are not configured because pinctrl is yet to probe.
>>
>> Signed-off-by: Archit Taneja <archit@ti.com>
>> ---
>>   drivers/gpu/drm/omapdrm/omap_crtc.c |  5 ++++
>>   drivers/gpu/drm/omapdrm/omap_drv.c  | 51 +++++++++++++++++++++----------------
>>   drivers/gpu/drm/omapdrm/omap_drv.h  |  1 +
>>   3 files changed, 35 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> index 0fd2eb1..9c01311 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> @@ -623,6 +623,11 @@ void omap_crtc_pre_init(void)
>>   	dss_install_mgr_ops(&mgr_ops);
>>   }
>>
>> +void omap_crtc_pre_uninit(void)
>> +{
>> +	dss_uninstall_mgr_ops();
>> +}
>> +
>>   /* initialize crtc */
>>   struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>>   		struct drm_plane *plane, enum omap_channel channel, int id)
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
>> index 2603d90..cbe5d8e 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -87,6 +87,24 @@ static bool channel_used(struct drm_device *dev, enum omap_channel channel)
>>   	return false;
>>   }
>>
>> +static int omap_connect_dssdevs(void)
>> +{
>> +	int r;
>> +	struct omap_dss_device *dssdev = NULL;
>> +
>> +	for_each_dss_dev(dssdev) {
>> +		r = dssdev->driver->connect(dssdev);
>> +		if (r == -EPROBE_DEFER) {
>> +			return r;
>> +		} else if (r) {
>> +			dev_warn(dssdev->dev, "could not connect display: %s\n",
>> +				dssdev->name);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>
> There are two issues with this one:
>
> for_each_dss_dev() uses omap_dss_get_next_device(), and that will
> increase the ref count of the current dssdev. If you interrupt the loop,
> the ref count won't be decreased. I have a hunch that we could have
> similar bugs elsewhere too...

You are saying that the ref counts will be correct only if 
for_each_dss_dev() is fully completed?

Maybe we can save the first dssdev which doesn't connect, save that 
dssdev and let the loop continue for the refcount to get decremented again.

Or maybe use omap_dss_get_next_device in a custom loop, which takes care 
of doing a put() for the device before returning.

>
> Second, in case of error, the dssdevs that were already connected should
> be disconnected. Although maybe that's not important, as they'll end up
> being connected again when the omapdrm is probed the next time.

The one's that were already connected won't connect again and return an 
error which isn't EPROBE_DEFER, so that should be okay right?

Archit

Re: [PATCH] drm: omap: fix: Defer probe if an omapdss device requests for it at connect

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 5020 bytes --]

On 18/09/13 16:17, Archit Taneja wrote:
> On Wednesday 18 September 2013 06:11 PM, Tomi Valkeinen wrote:
>> On 18/09/13 14:08, Archit Taneja wrote:
>>> Some omapdss panels are connected to outputs/encoders(HDMI/DSI/DPI)
>>> that require
>>> regulators. The output's connect op tries to get a regulator which
>>> may not exist
>>> yet because it might get registered later in the kernel boot.
>>>
>>> omapdrm currently ignores those panels which return a non zero value
>>> when
>>> connected. A better approach would be for omapdrm to request for probe
>>> deferral if a panel's connect op returns -EPROBE_DEFER.
>>>
>>> The connecting of panels is moved very early in the the drm device's
>>> probe
>>> before anything else is initialized. When we enter
>>> omap_modeset_init(), we have
>>> a set of panels that have been connected. We now proceed with
>>> registering only
>>> those panels which are already connected.
>>>
>>> Checking whether the panel has a driver or whether it has
>>> get_timing/read_edid
>>> ops in omap_modeset_init() are redundant with the new display model.
>>> These can
>>> be removed since a dssdev device will always have a driver associated
>>> with it,
>>> and all dssdev drivers have a get_timings op.
>>>
>>> This fixes boot with omapdrm on an omap4 panda ES board. The
>>> regulators used by
>>> HDMI aren't initialized because I2c isn't initialized, I2C isn't
>>> initialized
>>> as it's pins are not configured because pinctrl is yet to probe.
>>>
>>> Signed-off-by: Archit Taneja <archit@ti.com>
>>> ---
>>>   drivers/gpu/drm/omapdrm/omap_crtc.c |  5 ++++
>>>   drivers/gpu/drm/omapdrm/omap_drv.c  | 51
>>> +++++++++++++++++++++----------------
>>>   drivers/gpu/drm/omapdrm/omap_drv.h  |  1 +
>>>   3 files changed, 35 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
>>> b/drivers/gpu/drm/omapdrm/omap_crtc.c
>>> index 0fd2eb1..9c01311 100644
>>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>>> @@ -623,6 +623,11 @@ void omap_crtc_pre_init(void)
>>>       dss_install_mgr_ops(&mgr_ops);
>>>   }
>>>
>>> +void omap_crtc_pre_uninit(void)
>>> +{
>>> +    dss_uninstall_mgr_ops();
>>> +}
>>> +
>>>   /* initialize crtc */
>>>   struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>>>           struct drm_plane *plane, enum omap_channel channel, int id)
>>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
>>> b/drivers/gpu/drm/omapdrm/omap_drv.c
>>> index 2603d90..cbe5d8e 100644
>>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>>> @@ -87,6 +87,24 @@ static bool channel_used(struct drm_device *dev,
>>> enum omap_channel channel)
>>>       return false;
>>>   }
>>>
>>> +static int omap_connect_dssdevs(void)
>>> +{
>>> +    int r;
>>> +    struct omap_dss_device *dssdev = NULL;
>>> +
>>> +    for_each_dss_dev(dssdev) {
>>> +        r = dssdev->driver->connect(dssdev);
>>> +        if (r == -EPROBE_DEFER) {
>>> +            return r;
>>> +        } else if (r) {
>>> +            dev_warn(dssdev->dev, "could not connect display: %s\n",
>>> +                dssdev->name);
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>
>> There are two issues with this one:
>>
>> for_each_dss_dev() uses omap_dss_get_next_device(), and that will
>> increase the ref count of the current dssdev. If you interrupt the loop,
>> the ref count won't be decreased. I have a hunch that we could have
>> similar bugs elsewhere too...
> 
> You are saying that the ref counts will be correct only if
> for_each_dss_dev() is fully completed?

Right.

> Maybe we can save the first dssdev which doesn't connect, save that
> dssdev and let the loop continue for the refcount to get decremented again.
> 
> Or maybe use omap_dss_get_next_device in a custom loop, which takes care
> of doing a put() for the device before returning.

Well, you can just use omap_dss_put_device(dssdev) before the return.
That should fix it.

Check drivers/video/omap2/dss/display.c:omap_dss_get_next_device().

>> Second, in case of error, the dssdevs that were already connected should
>> be disconnected. Although maybe that's not important, as they'll end up
>> being connected again when the omapdrm is probed the next time.
> 
> The one's that were already connected won't connect again and return an
> error which isn't EPROBE_DEFER, so that should be okay right?

Right, in practice it doesn't matter. The only issue here is that it's
not very nice if you don't clean up after getting an error =). And,
well, with modules there could be issues in some particular cases, where
leaving things connected would prevent unloading of modules.

omapdrm doesn't seem to disconnect even when it's removed normally. I
guess it'd be nicer to have similar disconnect loop as in omapfb, i.e.
just go over all the displays and disconnect them all.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[PATCH 1/2] drm: omap: fix: Defer probe if an omapdss device requests for it at connect

[Archit Taneja]

Some omapdss panels are connected to outputs/encoders(HDMI/DSI/DPI) that require
regulators. The output's connect op tries to get a regulator which may not exist
yet because it might get registered later in the kernel boot.

omapdrm currently ignores those panels which return a non zero value when
connected. A better approach would be for omapdrm to request for probe
deferral if a panel's connect op returns -EPROBE_DEFER.

The connecting of panels is moved very early in the the drm device's probe
before anything else is initialized. When we enter omap_modeset_init(), we have
a set of panels that have been connected. We now proceed with registering only
those panels which are already connected.

Checking whether the panel has a driver or whether it has get_timing/read_edid
ops in omap_modeset_init() are redundant with the new display model. These can
be removed since a dssdev device will always have a driver associated with it,
and all dssdev drivers have a get_timings op.

This fixes boot with omapdrm on an omap4 panda ES board. The regulators used by
HDMI aren't initialized because I2c isn't initialized, I2C isn't initialized
as it's pins are not configured because pinctrl is yet to probe.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c |  5 +++
 drivers/gpu/drm/omapdrm/omap_drv.c  | 64 ++++++++++++++++++++++++-------------
 drivers/gpu/drm/omapdrm/omap_drv.h  |  1 +
 3 files changed, 48 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 0fd2eb1..9c01311 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -623,6 +623,11 @@ void omap_crtc_pre_init(void)
 	dss_install_mgr_ops(&mgr_ops);
 }
 
+void omap_crtc_pre_uninit(void)
+{
+	dss_uninstall_mgr_ops();
+}
+
 /* initialize crtc */
 struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 		struct drm_plane *plane, enum omap_channel channel, int id)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 2603d90..45fbb1c 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -87,6 +87,37 @@ static bool channel_used(struct drm_device *dev, enum omap_channel channel)
 	return false;
 }
 
+static int omap_connect_dssdevs(void)
+{
+	int r;
+	struct omap_dss_device *dssdev = NULL;
+
+	for_each_dss_dev(dssdev) {
+		r = dssdev->driver->connect(dssdev);
+		if (r == -EPROBE_DEFER) {
+			omap_dss_put_device(dssdev);
+			goto cleanup;
+		} else if (r) {
+			dev_warn(dssdev->dev, "could not connect display: %s\n",
+				dssdev->name);
+		}
+	}
+
+	return 0;
+
+cleanup:
+	/*
+	 * if we are deferring probe, we disconnect the devices we previously
+	 * connected
+	 */
+	dssdev = NULL;
+
+	for_each_dss_dev(dssdev)
+		dssdev->driver->disconnect(dssdev);
+
+	return r;
+}
+
 static int omap_modeset_init(struct drm_device *dev)
 {
 	struct omap_drm_private *priv = dev->dev_private;
@@ -95,9 +126,6 @@ static int omap_modeset_init(struct drm_device *dev)
 	int num_mgrs = dss_feat_get_num_mgrs();
 	int num_crtcs;
 	int i, id = 0;
-	int r;
-
-	omap_crtc_pre_init();
 
 	drm_mode_config_init(dev);
 
@@ -119,26 +147,8 @@ static int omap_modeset_init(struct drm_device *dev)
 		enum omap_channel channel;
 		struct omap_overlay_manager *mgr;
 
-		if (!dssdev->driver) {
-			dev_warn(dev->dev, "%s has no driver.. skipping it\n",
-					dssdev->name);
-			continue;
-		}
-
-		if (!(dssdev->driver->get_timings ||
-					dssdev->driver->read_edid)) {
-			dev_warn(dev->dev, "%s driver does not support "
-				"get_timings or read_edid.. skipping it!\n",
-				dssdev->name);
+		if (!omapdss_device_is_connected(dssdev))
 			continue;
-		}
-
-		r = dssdev->driver->connect(dssdev);
-		if (r) {
-			dev_err(dev->dev, "could not connect display: %s\n",
-					dssdev->name);
-			continue;
-		}
 
 		encoder = omap_encoder_init(dev, dssdev);
 
@@ -656,9 +666,19 @@ static void pdev_shutdown(struct platform_device *device)
 
 static int pdev_probe(struct platform_device *device)
 {
+	int r;
+
 	if (omapdss_is_initialized() == false)
 		return -EPROBE_DEFER;
 
+	omap_crtc_pre_init();
+
+	r = omap_connect_dssdevs();
+	if (r) {
+		omap_crtc_pre_uninit();
+		return r;
+	}
+
 	DBG("%s", device->name);
 	return drm_platform_init(&omap_drm_driver, device);
 }
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 30b95b7..cb86cb3 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -158,6 +158,7 @@ enum omap_channel omap_crtc_channel(struct drm_crtc *crtc);
 int omap_crtc_apply(struct drm_crtc *crtc,
 		struct omap_drm_apply *apply);
 void omap_crtc_pre_init(void);
+void omap_crtc_pre_uninit(void);
 struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 		struct drm_plane *plane, enum omap_channel channel, int id);
 
-- 
1.8.1.2

[PATCH 2/2] drm: omap: disconnect devices when omapdrm module is removed

[Archit Taneja]

omapdrm established connections for omap_dss_device devices when probed. It
should also be responsible to disconnect the devices. Keeping the devices
connected can prevent the panel driver modules from unloading, it can also
cause problems when omapdrm is re-inserted.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 45fbb1c..44a1203 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -86,6 +86,13 @@ static bool channel_used(struct drm_device *dev, enum omap_channel channel)
 
 	return false;
 }
+static void omap_disconnect_dssdevs(void)
+{
+	struct omap_dss_device *dssdev = NULL;
+
+	for_each_dss_dev(dssdev)
+		dssdev->driver->disconnect(dssdev);
+}
 
 static int omap_connect_dssdevs(void)
 {
@@ -110,10 +117,7 @@ cleanup:
 	 * if we are deferring probe, we disconnect the devices we previously
 	 * connected
 	 */
-	dssdev = NULL;
-
-	for_each_dss_dev(dssdev)
-		dssdev->driver->disconnect(dssdev);
+	omap_disconnect_dssdevs();
 
 	return r;
 }
@@ -688,6 +692,8 @@ static int pdev_remove(struct platform_device *device)
 	DBG("");
 	drm_platform_exit(&omap_drm_driver, device);
 
+	omap_disconnect_dssdevs();
+
 	platform_driver_unregister(&omap_dmm_driver);
 	return 0;
 }
-- 
1.8.1.2

Re: [PATCH 2/2] drm: omap: disconnect devices when omapdrm module is removed

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1670 bytes --]

On 19/09/13 11:49, Archit Taneja wrote:
> omapdrm established connections for omap_dss_device devices when probed. It
> should also be responsible to disconnect the devices. Keeping the devices
> connected can prevent the panel driver modules from unloading, it can also
> cause problems when omapdrm is re-inserted.
> 
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 45fbb1c..44a1203 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -86,6 +86,13 @@ static bool channel_used(struct drm_device *dev, enum omap_channel channel)
>  
>  	return false;
>  }
> +static void omap_disconnect_dssdevs(void)
> +{
> +	struct omap_dss_device *dssdev = NULL;
> +
> +	for_each_dss_dev(dssdev)
> +		dssdev->driver->disconnect(dssdev);
> +}

With a quick test, it looks like omapdrm leaves the displays enabled
when exiting. This can be fixed by adding to the above loop:

		if (dssdev->state != OMAP_DSS_DISPLAY_DISABLED)
			dssdev->driver->disable(dssdev);

However... omapdrm still crashes when unloading, and it could be somehow
related to leaving something un-removed. Disabling a CRTC should disable
the display, and maybe omapdrm should disable (and clean-up) all CRTCs
on exit, and maybe that would remove the crash, and also fix the issue
of leaving displays enabled.

But I'm just guessing there, I have no idea how the process of unloading
a drm driver goes.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH 2/2] drm: omap: disconnect devices when omapdrm module is removed

[Archit Taneja]

Hi,

On Thursday 19 September 2013 03:38 PM, Tomi Valkeinen wrote:
> On 19/09/13 11:49, Archit Taneja wrote:
>> omapdrm established connections for omap_dss_device devices when probed. It
>> should also be responsible to disconnect the devices. Keeping the devices
>> connected can prevent the panel driver modules from unloading, it can also
>> cause problems when omapdrm is re-inserted.
>>
>> Signed-off-by: Archit Taneja <archit@ti.com>
>> ---
>>   drivers/gpu/drm/omapdrm/omap_drv.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
>> index 45fbb1c..44a1203 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -86,6 +86,13 @@ static bool channel_used(struct drm_device *dev, enum omap_channel channel)
>>
>>   	return false;
>>   }
>> +static void omap_disconnect_dssdevs(void)
>> +{
>> +	struct omap_dss_device *dssdev = NULL;
>> +
>> +	for_each_dss_dev(dssdev)
>> +		dssdev->driver->disconnect(dssdev);
>> +}
>
> With a quick test, it looks like omapdrm leaves the displays enabled
> when exiting. This can be fixed by adding to the above loop:
>
> 		if (dssdev->state != OMAP_DSS_DISPLAY_DISABLED)
> 			dssdev->driver->disable(dssdev);
>
> However... omapdrm still crashes when unloading, and it could be somehow
> related to leaving something un-removed. Disabling a CRTC should disable
> the display, and maybe omapdrm should disable (and clean-up) all CRTCs
> on exit, and maybe that would remove the crash, and also fix the issue
> of leaving displays enabled.
>
> But I'm just guessing there, I have no idea how the process of unloading
> a drm driver goes.

It seems like dev_unload is the place where we should disconnect the 
dssdevs.

omap_modeset_free() calls drm_mode_config_cleanup() which calls omapdrm 
specific destroy funcs for connectors, encoders and crtcs.

I don't think crtcs should disable the device. I think it's the 
encoder(and connector) which is mapped to a display. The omap_encoder's 
destroy op should disable the dssdev device.

I'm not sure about this though. Rob, do you have any comment on this?

Archit

[PATCH v2 0/2] drm: omap: Fix omapdrm probe and module insertion/removal issues

[Archit Taneja]

With the new omapdss device model. The user(omapdrm/omapfb) of a omap_dss_device
has to call connect() to use the device. A connect() call can request to defer
probe if the device(or the previous entities in the chain) have missing
resources like a regulator or an I2C bus.

We make omapdrm defer probe by trying to first connect all panels, and request
for deferral if any panel requests for a defer. This makes sure that all the
panels are set up correctly when we finally proceed with omapdrm initialization.

In order for omapdrm module to be removed successfully, we need to disconnect
the panels which omapdrm connected. Another thing noticed was that omapdrm
insertion followed by some usage results in panels getting enabled.

Trying to remove omapdrm with a panel enabled results in failure while
disconnecting. This leaves omapdss panels in a bad state. I have added an
explicit panel disable in the omap_encoder_destroy() op, I needed some
suggestions on whether there is a better way to do this.

changes in v2:
- Add special case when no panels are available to connect.
- Make sure panels are diabled (and then disconnected) when omapdrm is removed

Archit Taneja (2):
  drm: omap: fix: Defer probe if an omapdss device requests for it at
    connect
  drm: omap: disconnect devices when omapdrm module is removed

 drivers/gpu/drm/omapdrm/omap_crtc.c    |  5 +++
 drivers/gpu/drm/omapdrm/omap_drv.c     | 77 ++++++++++++++++++++++++----------
 drivers/gpu/drm/omapdrm/omap_drv.h     |  1 +
 drivers/gpu/drm/omapdrm/omap_encoder.c |  3 ++
 4 files changed, 64 insertions(+), 22 deletions(-)

-- 
1.8.1.2

[PATCH v2 1/2] drm: omap: fix: Defer probe if an omapdss device requests for it at connect

[Archit Taneja]

Some omapdss panels are connected to outputs/encoders(HDMI/DSI/DPI) that require
regulators. The output's connect op tries to get a regulator which may not exist
yet because it might get registered later in the kernel boot.

omapdrm currently ignores those panels which return a non zero value when
connected. A better approach would be for omapdrm to request for probe deferral
if a panel's connect op returns -EPROBE_DEFER.

A special case has to be considered when no panels are available to connect when
omapdrm probes. In this case too, we defer probe and expect that a panel will be
available to connect the next time.

The connecting of panels is moved very early in the the drm device's probe
before anything else is initialized. When we enter omap_modeset_init(), we have
a set of panels that have been connected. We now proceed with registering only
those panels which are already connected.

Checking whether the panel has a driver or whether it has get_timing/read_edid
ops in omap_modeset_init() are redundant with the new display model. These can
be removed since a dssdev device will always have a driver associated with it,
and all dssdev drivers have a get_timings op.

This fixes boot with omapdrm on an omap4 panda ES board. The regulators used by
HDMI aren't initialized because I2c isn't initialized, I2C isn't initialized
as it's pins are not configured because pinctrl is yet to probe.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c |  5 +++
 drivers/gpu/drm/omapdrm/omap_drv.c  | 70 +++++++++++++++++++++++++------------
 drivers/gpu/drm/omapdrm/omap_drv.h  |  1 +
 3 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 0fd2eb1..9c01311 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -623,6 +623,11 @@ void omap_crtc_pre_init(void)
 	dss_install_mgr_ops(&mgr_ops);
 }
 
+void omap_crtc_pre_uninit(void)
+{
+	dss_uninstall_mgr_ops();
+}
+
 /* initialize crtc */
 struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 		struct drm_plane *plane, enum omap_channel channel, int id)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 2603d90..be3bfe1 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -87,6 +87,43 @@ static bool channel_used(struct drm_device *dev, enum omap_channel channel)
 	return false;
 }
 
+static int omap_connect_dssdevs(void)
+{
+	int r;
+	struct omap_dss_device *dssdev = NULL;
+	bool no_displays = true;
+
+	for_each_dss_dev(dssdev) {
+		r = dssdev->driver->connect(dssdev);
+		if (r == -EPROBE_DEFER) {
+			omap_dss_put_device(dssdev);
+			goto cleanup;
+		} else if (r) {
+			dev_warn(dssdev->dev, "could not connect display: %s\n",
+				dssdev->name);
+		} else {
+			no_displays = false;
+		}
+	}
+
+	if (no_displays)
+		return -EPROBE_DEFER;
+
+	return 0;
+
+cleanup:
+	/*
+	 * if we are deferring probe, we disconnect the devices we previously
+	 * connected
+	 */
+	dssdev = NULL;
+
+	for_each_dss_dev(dssdev)
+		dssdev->driver->disconnect(dssdev);
+
+	return r;
+}
+
 static int omap_modeset_init(struct drm_device *dev)
 {
 	struct omap_drm_private *priv = dev->dev_private;
@@ -95,9 +132,6 @@ static int omap_modeset_init(struct drm_device *dev)
 	int num_mgrs = dss_feat_get_num_mgrs();
 	int num_crtcs;
 	int i, id = 0;
-	int r;
-
-	omap_crtc_pre_init();
 
 	drm_mode_config_init(dev);
 
@@ -119,26 +153,8 @@ static int omap_modeset_init(struct drm_device *dev)
 		enum omap_channel channel;
 		struct omap_overlay_manager *mgr;
 
-		if (!dssdev->driver) {
-			dev_warn(dev->dev, "%s has no driver.. skipping it\n",
-					dssdev->name);
-			continue;
-		}
-
-		if (!(dssdev->driver->get_timings ||
-					dssdev->driver->read_edid)) {
-			dev_warn(dev->dev, "%s driver does not support "
-				"get_timings or read_edid.. skipping it!\n",
-				dssdev->name);
-			continue;
-		}
-
-		r = dssdev->driver->connect(dssdev);
-		if (r) {
-			dev_err(dev->dev, "could not connect display: %s\n",
-					dssdev->name);
+		if (!omapdss_device_is_connected(dssdev))
 			continue;
-		}
 
 		encoder = omap_encoder_init(dev, dssdev);
 
@@ -656,9 +672,19 @@ static void pdev_shutdown(struct platform_device *device)
 
 static int pdev_probe(struct platform_device *device)
 {
+	int r;
+
 	if (omapdss_is_initialized() == false)
 		return -EPROBE_DEFER;
 
+	omap_crtc_pre_init();
+
+	r = omap_connect_dssdevs();
+	if (r) {
+		omap_crtc_pre_uninit();
+		return r;
+	}
+
 	DBG("%s", device->name);
 	return drm_platform_init(&omap_drm_driver, device);
 }
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 30b95b7..cb86cb3 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -158,6 +158,7 @@ enum omap_channel omap_crtc_channel(struct drm_crtc *crtc);
 int omap_crtc_apply(struct drm_crtc *crtc,
 		struct omap_drm_apply *apply);
 void omap_crtc_pre_init(void);
+void omap_crtc_pre_uninit(void);
 struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 		struct drm_plane *plane, enum omap_channel channel, int id);
 
-- 
1.8.1.2

[PATCH v2 2/2] drm: omap: disconnect devices when omapdrm module is removed

[Archit Taneja]

omapdrm establishes connections for omap_dss_device devices when probed. It
should also be responsible to disconnect the devices. Keeping the devices
connected can prevent the panel driver modules from unloading, it also causes
issues when we try to remove or re-insert omapdrm module.

Before omapdrm module is removed, we need to make sure that all the connectors
are disabled. Without this, the omap_dss_devices won't disconnect, and cause
issues when we try to re-insert omapdrm.

For now, disable encoders in omap_encoder_destroy. We are not clear if this is
the best place to disable an encoder. This makes successive module insertions
and removal work unlike before. But it leads to warning backtraces when the
module is removed. The func omap_crtc_destroy() gves a warning because
omap_crtc->apply_irq.registered is still set when we try to disable the crtc. I
haven't figured out a way to prevent this, would appreciate some help here.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c     | 15 +++++++++++----
 drivers/gpu/drm/omapdrm/omap_encoder.c |  3 +++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index be3bfe1..45f7ec6 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -86,6 +86,13 @@ static bool channel_used(struct drm_device *dev, enum omap_channel channel)
 
 	return false;
 }
+static void omap_disconnect_dssdevs(void)
+{
+	struct omap_dss_device *dssdev = NULL;
+
+	for_each_dss_dev(dssdev)
+		dssdev->driver->disconnect(dssdev);
+}
 
 static int omap_connect_dssdevs(void)
 {
@@ -116,10 +123,7 @@ cleanup:
 	 * if we are deferring probe, we disconnect the devices we previously
 	 * connected
 	 */
-	dssdev = NULL;
-
-	for_each_dss_dev(dssdev)
-		dssdev->driver->disconnect(dssdev);
+	omap_disconnect_dssdevs();
 
 	return r;
 }
@@ -694,6 +698,9 @@ static int pdev_remove(struct platform_device *device)
 	DBG("");
 	drm_platform_exit(&omap_drm_driver, device);
 
+	omap_disconnect_dssdevs();
+	omap_crtc_pre_uninit();
+
 	platform_driver_unregister(&omap_dmm_driver);
 	return 0;
 }
diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c
index 6a12e89..5290a88 100644
--- a/drivers/gpu/drm/omapdrm/omap_encoder.c
+++ b/drivers/gpu/drm/omapdrm/omap_encoder.c
@@ -51,6 +51,9 @@ struct omap_dss_device *omap_encoder_get_dssdev(struct drm_encoder *encoder)
 static void omap_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
+
+	omap_encoder_set_enabled(encoder, false);
+
 	drm_encoder_cleanup(encoder);
 	kfree(omap_encoder);
 }
-- 
1.8.1.2

Re: [PATCH v2 0/2] drm: omap: Fix omapdrm probe and module insertion/removal issues

[Archit Taneja]

Hi Rob,

On Monday 07 October 2013 03:08 PM, Archit Taneja wrote:
> With the new omapdss device model. The user(omapdrm/omapfb) of a omap_dss_device
> has to call connect() to use the device. A connect() call can request to defer
> probe if the device(or the previous entities in the chain) have missing
> resources like a regulator or an I2C bus.
>
> We make omapdrm defer probe by trying to first connect all panels, and request
> for deferral if any panel requests for a defer. This makes sure that all the
> panels are set up correctly when we finally proceed with omapdrm initialization.
>
> In order for omapdrm module to be removed successfully, we need to disconnect
> the panels which omapdrm connected. Another thing noticed was that omapdrm
> insertion followed by some usage results in panels getting enabled.
>
> Trying to remove omapdrm with a panel enabled results in failure while
> disconnecting. This leaves omapdss panels in a bad state. I have added an
> explicit panel disable in the omap_encoder_destroy() op, I needed some
> suggestions on whether there is a better way to do this.
>
> changes in v2:
> - Add special case when no panels are available to connect.
> - Make sure panels are diabled (and then disconnected) when omapdrm is removed

omapdrm fails when built-in in 3.12, the first patch fixes this. The 
second one fixes issues seen with successive module insertion and removals.

It'll be great if the first one can at least make it to 3.12. Could you 
have a look at it?

Thanks,
Archit

Re: [PATCH v2 0/2] drm: omap: Fix omapdrm probe and module insertion/removal issues

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1882 bytes --]

On 07/10/13 12:38, Archit Taneja wrote:
> With the new omapdss device model. The user(omapdrm/omapfb) of a omap_dss_device
> has to call connect() to use the device. A connect() call can request to defer
> probe if the device(or the previous entities in the chain) have missing
> resources like a regulator or an I2C bus.
> 
> We make omapdrm defer probe by trying to first connect all panels, and request
> for deferral if any panel requests for a defer. This makes sure that all the
> panels are set up correctly when we finally proceed with omapdrm initialization.
> 
> In order for omapdrm module to be removed successfully, we need to disconnect
> the panels which omapdrm connected. Another thing noticed was that omapdrm
> insertion followed by some usage results in panels getting enabled.
> 
> Trying to remove omapdrm with a panel enabled results in failure while
> disconnecting. This leaves omapdss panels in a bad state. I have added an
> explicit panel disable in the omap_encoder_destroy() op, I needed some
> suggestions on whether there is a better way to do this.
> 
> changes in v2:
> - Add special case when no panels are available to connect.
> - Make sure panels are diabled (and then disconnected) when omapdrm is removed
> 
> Archit Taneja (2):
>   drm: omap: fix: Defer probe if an omapdss device requests for it at
>     connect
>   drm: omap: disconnect devices when omapdrm module is removed
> 
>  drivers/gpu/drm/omapdrm/omap_crtc.c    |  5 +++
>  drivers/gpu/drm/omapdrm/omap_drv.c     | 77 ++++++++++++++++++++++++----------
>  drivers/gpu/drm/omapdrm/omap_drv.h     |  1 +
>  drivers/gpu/drm/omapdrm/omap_encoder.c |  3 ++
>  4 files changed, 64 insertions(+), 22 deletions(-)
> 

Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

If it's not too late, it'd be nice to get these two fixes for 3.12.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]