[PATCH RFC 0/4] drm/core: restore suspend/resume calbacks in KMS drm drivers

[PATCH RFC 0/4] drm/core: restore suspend/resume calbacks in KMS drm drivers

[Andrzej Hajda]

The main intent of this patchset is to allow use of suspend/resume drm driver
callbacks in KMS drivers, as these callbacks seems to me the best place
to implement suspend/resume functionality in drm driver.
Implementing this functionality in master component driver PM ops is problematic
as those callbacks can be called asynchronously regardless of state/existence of
drm device, thus it would require additional synchronization mechanism.

Callbacks re-enabling requires small changes in i915 and exynos driver.
The patchset contains also fix of exynos resume callback.

Regards
Andrzej


Andrzej Hajda (4):
  drm/i915: set PM callbacks only if modeset is turned off
  drm/core: re-enable suspend/resume callbacks for KMS drivers
  drm/exynos: remove master component PM callbacks
  drm/exynos: correct connector->dpms field before resuming

 drivers/gpu/drm/drm_sysfs.c             |  2 --
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 37 ++++++---------------------------
 drivers/gpu/drm/i915/i915_drv.c         |  6 ++----
 3 files changed, 8 insertions(+), 37 deletions(-)

-- 
1.9.1

[PATCH RFC 0/4] drm/core: restore suspend/resume calbacks in KMS drm drivers

[Andrzej Hajda]

The main intent of this patchset is to allow use of suspend/resume drm driver
callbacks in KMS drivers, as these callbacks seems to me the best place
to implement suspend/resume functionality in drm driver.
Implementing this functionality in master component driver PM ops is problematic
as those callbacks can be called asynchronously regardless of state/existence of
drm device, thus it would require additional synchronization mechanism.

Callbacks re-enabling requires small changes in i915 and exynos driver.
The patchset contains also fix of exynos resume callback.

Regards
Andrzej


Andrzej Hajda (4):
  drm/i915: set PM callbacks only if modeset is turned off
  drm/core: re-enable suspend/resume callbacks for KMS drivers
  drm/exynos: remove master component PM callbacks
  drm/exynos: correct connector->dpms field before resuming

 drivers/gpu/drm/drm_sysfs.c             |  2 --
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 37 ++++++---------------------------
 drivers/gpu/drm/i915/i915_drv.c         |  6 ++----
 3 files changed, 8 insertions(+), 37 deletions(-)

-- 
1.9.1

[PATCH RFC 1/4] drm/i915: set PM callbacks only if modeset is turned off

[Andrzej Hajda]

Currently suspend and resume callbacks are called only if driver have
modeset feature disabled.
This patch moves the check directly to i915 driver, it will allow to
remove the check from the core in the future.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3870c73..481f62f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1617,10 +1617,6 @@ static struct drm_driver driver = {
 	.postclose = i915_driver_postclose,
 	.set_busid = drm_pci_set_busid,
 
-	/* Used in place of i915_pm_ops for non-DRIVER_MODESET */
-	.suspend = i915_suspend,
-	.resume = i915_resume_legacy,
-
 	.device_is_agp = i915_driver_device_is_agp,
 	.master_create = i915_master_create,
 	.master_destroy = i915_master_destroy,
@@ -1684,6 +1680,8 @@ static int __init i915_init(void)
 
 	if (!(driver.driver_features & DRIVER_MODESET)) {
 		driver.get_vblank_timestamp = NULL;
+		driver.suspend = i915_suspend;
+		driver.resume = i915_resume_legacy;
 #ifndef CONFIG_DRM_I915_UMS
 		/* Silently fail loading to not upset userspace. */
 		DRM_DEBUG_DRIVER("KMS and UMS disabled.\n");
-- 
1.9.1

[PATCH RFC 1/4] drm/i915: set PM callbacks only if modeset is turned off

[Andrzej Hajda]

Currently suspend and resume callbacks are called only if driver have
modeset feature disabled.
This patch moves the check directly to i915 driver, it will allow to
remove the check from the core in the future.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3870c73..481f62f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1617,10 +1617,6 @@ static struct drm_driver driver = {
 	.postclose = i915_driver_postclose,
 	.set_busid = drm_pci_set_busid,
 
-	/* Used in place of i915_pm_ops for non-DRIVER_MODESET */
-	.suspend = i915_suspend,
-	.resume = i915_resume_legacy,
-
 	.device_is_agp = i915_driver_device_is_agp,
 	.master_create = i915_master_create,
 	.master_destroy = i915_master_destroy,
@@ -1684,6 +1680,8 @@ static int __init i915_init(void)
 
 	if (!(driver.driver_features & DRIVER_MODESET)) {
 		driver.get_vblank_timestamp = NULL;
+		driver.suspend = i915_suspend;
+		driver.resume = i915_resume_legacy;
 #ifndef CONFIG_DRM_I915_UMS
 		/* Silently fail loading to not upset userspace. */
 		DRM_DEBUG_DRIVER("KMS and UMS disabled.\n");
-- 
1.9.1

[PATCH RFC 2/4] drm/core: re-enable suspend/resume callbacks for KMS drivers

[Andrzej Hajda]

Implementing suspend/resume functionality in componentized drm drivers using
master component PM callbacks is problematic because those callbacks can be
called asynchronously regardless of existence/state of drm device.
The patch re-enables suspend/resume drm driver callbacks in drivers with
modeset feature enabled. These callback can be used to implement suspend/resume
functionality in more convenient way.

The patch should not affect behavior of existing drm drivers.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/drm_sysfs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index cc3d6d6..206afc4 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -45,7 +45,6 @@ static int __drm_class_suspend(struct device *dev, pm_message_t state)
 		struct drm_device *drm_dev = drm_minor->dev;
 
 		if (drm_minor->type == DRM_MINOR_LEGACY &&
-		    !drm_core_check_feature(drm_dev, DRIVER_MODESET) &&
 		    drm_dev->driver->suspend)
 			return drm_dev->driver->suspend(drm_dev, state);
 	}
@@ -86,7 +85,6 @@ static int drm_class_resume(struct device *dev)
 		struct drm_device *drm_dev = drm_minor->dev;
 
 		if (drm_minor->type == DRM_MINOR_LEGACY &&
-		    !drm_core_check_feature(drm_dev, DRIVER_MODESET) &&
 		    drm_dev->driver->resume)
 			return drm_dev->driver->resume(drm_dev);
 	}
-- 
1.9.1

[PATCH RFC 2/4] drm/core: re-enable suspend/resume callbacks for KMS drivers

[Andrzej Hajda]

Implementing suspend/resume functionality in componentized drm drivers using
master component PM callbacks is problematic because those callbacks can be
called asynchronously regardless of existence/state of drm device.
The patch re-enables suspend/resume drm driver callbacks in drivers with
modeset feature enabled. These callback can be used to implement suspend/resume
functionality in more convenient way.

The patch should not affect behavior of existing drm drivers.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/drm_sysfs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index cc3d6d6..206afc4 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -45,7 +45,6 @@ static int __drm_class_suspend(struct device *dev, pm_message_t state)
 		struct drm_device *drm_dev = drm_minor->dev;
 
 		if (drm_minor->type == DRM_MINOR_LEGACY &&
-		    !drm_core_check_feature(drm_dev, DRIVER_MODESET) &&
 		    drm_dev->driver->suspend)
 			return drm_dev->driver->suspend(drm_dev, state);
 	}
@@ -86,7 +85,6 @@ static int drm_class_resume(struct device *dev)
 		struct drm_device *drm_dev = drm_minor->dev;
 
 		if (drm_minor->type == DRM_MINOR_LEGACY &&
-		    !drm_core_check_feature(drm_dev, DRIVER_MODESET) &&
 		    drm_dev->driver->resume)
 			return drm_dev->driver->resume(drm_dev);
 	}
-- 
1.9.1

[PATCH RFC 3/4] drm/exynos: remove master component PM callbacks

[Andrzej Hajda]

The patch removes master PM callbacks as their functionality
is already duplicated by suspend/resume drm callbacks.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index cf19e60..dca20b15 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -342,34 +342,6 @@ static struct drm_driver exynos_drm_driver = {
 	.minor	= DRIVER_MINOR,
 };
 
-#ifdef CONFIG_PM_SLEEP
-static int exynos_drm_sys_suspend(struct device *dev)
-{
-	struct drm_device *drm_dev = dev_get_drvdata(dev);
-	pm_message_t message;
-
-	if (pm_runtime_suspended(dev) || !drm_dev)
-		return 0;
-
-	message.event = PM_EVENT_SUSPEND;
-	return exynos_drm_suspend(drm_dev, message);
-}
-
-static int exynos_drm_sys_resume(struct device *dev)
-{
-	struct drm_device *drm_dev = dev_get_drvdata(dev);
-
-	if (pm_runtime_suspended(dev) || !drm_dev)
-		return 0;
-
-	return exynos_drm_resume(drm_dev);
-}
-#endif
-
-static const struct dev_pm_ops exynos_drm_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(exynos_drm_sys_suspend, exynos_drm_sys_resume)
-};
-
 int exynos_drm_component_add(struct device *dev,
 				enum exynos_drm_device_type dev_type,
 				enum exynos_drm_output_type out_type)
@@ -634,7 +606,6 @@ static struct platform_driver exynos_drm_platform_driver = {
 	.driver	= {
 		.owner	= THIS_MODULE,
 		.name	= "exynos-drm",
-		.pm	= &exynos_drm_pm_ops,
 	},
 };
 
-- 
1.9.1

[PATCH RFC 3/4] drm/exynos: remove master component PM callbacks

[Andrzej Hajda]

The patch removes master PM callbacks as their functionality
is already duplicated by suspend/resume drm callbacks.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index cf19e60..dca20b15 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -342,34 +342,6 @@ static struct drm_driver exynos_drm_driver = {
 	.minor	= DRIVER_MINOR,
 };
 
-#ifdef CONFIG_PM_SLEEP
-static int exynos_drm_sys_suspend(struct device *dev)
-{
-	struct drm_device *drm_dev = dev_get_drvdata(dev);
-	pm_message_t message;
-
-	if (pm_runtime_suspended(dev) || !drm_dev)
-		return 0;
-
-	message.event = PM_EVENT_SUSPEND;
-	return exynos_drm_suspend(drm_dev, message);
-}
-
-static int exynos_drm_sys_resume(struct device *dev)
-{
-	struct drm_device *drm_dev = dev_get_drvdata(dev);
-
-	if (pm_runtime_suspended(dev) || !drm_dev)
-		return 0;
-
-	return exynos_drm_resume(drm_dev);
-}
-#endif
-
-static const struct dev_pm_ops exynos_drm_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(exynos_drm_sys_suspend, exynos_drm_sys_resume)
-};
-
 int exynos_drm_component_add(struct device *dev,
 				enum exynos_drm_device_type dev_type,
 				enum exynos_drm_output_type out_type)
@@ -634,7 +606,6 @@ static struct platform_driver exynos_drm_platform_driver = {
 	.driver	= {
 		.owner	= THIS_MODULE,
 		.name	= "exynos-drm",
-		.pm	= &exynos_drm_pm_ops,
 	},
 };
 
-- 
1.9.1

[PATCH RFC 4/4] drm/exynos: correct connector->dpms field before resuming

[Andrzej Hajda]

During system suspend after connector switch off its dpms field
is set to connector previous dpms state. To properly resume dpms field
should be set to its actual state (off) before resuming to previous dpms state.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index dca20b15..6746c5a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -191,8 +191,12 @@ static int exynos_drm_resume(struct drm_device *dev)
 
 	drm_modeset_lock_all(dev);
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		if (connector->funcs->dpms)
-			connector->funcs->dpms(connector, connector->dpms);
+		if (connector->funcs->dpms) {
+			int old_dpms = connector->dpms;
+
+			connector->dpms = DRM_MODE_DPMS_OFF;
+			connector->funcs->dpms(connector, old_dpms);
+		}
 	}
 	drm_modeset_unlock_all(dev);
 
-- 
1.9.1

[PATCH RFC 4/4] drm/exynos: correct connector->dpms field before resuming

[Andrzej Hajda]

During system suspend after connector switch off its dpms field
is set to connector previous dpms state. To properly resume dpms field
should be set to its actual state (off) before resuming to previous dpms state.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index dca20b15..6746c5a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -191,8 +191,12 @@ static int exynos_drm_resume(struct drm_device *dev)
 
 	drm_modeset_lock_all(dev);
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		if (connector->funcs->dpms)
-			connector->funcs->dpms(connector, connector->dpms);
+		if (connector->funcs->dpms) {
+			int old_dpms = connector->dpms;
+
+			connector->dpms = DRM_MODE_DPMS_OFF;
+			connector->funcs->dpms(connector, old_dpms);
+		}
 	}
 	drm_modeset_unlock_all(dev);
 
-- 
1.9.1

Re: [PATCH RFC 0/4] drm/core: restore suspend/resume calbacks in KMS drm drivers

[Daniel Vetter]

On Fri, Oct 03, 2014 at 10:24:09AM +0200, Andrzej Hajda wrote:
> The main intent of this patchset is to allow use of suspend/resume drm driver
> callbacks in KMS drivers, as these callbacks seems to me the best place
> to implement suspend/resume functionality in drm driver.
> Implementing this functionality in master component driver PM ops is problematic
> as those callbacks can be called asynchronously regardless of state/existence of
> drm device, thus it would require additional synchronization mechanism.
> 
> Callbacks re-enabling requires small changes in i915 and exynos driver.
> The patchset contains also fix of exynos resume callback.

Nack.

Like completely and totally. The drm core has really no business doing
hardware stuff, which includes runtime pm, system suspend and all that
nonsense. It' an interface between userspace and drivers, with a big
library to back it all up. Everything else just repeats the old midlayer
mistake.

If you driver needs this, do it there. Also, the component framework is
probably the solution you're looking for. And if there are synchronization
issues with that then we need to fix those instead of reinventing yet
another half-assed broken wheel.

Aside: With David Herrmann's latest patches to de-midlayer the drm
init/teardown sequence the driver is in full control of when the drm data
structures get allocate, initialized and registered. If you convert to
that plus the component framework I'm pretty sure your problem is solved.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH RFC 0/4] drm/core: restore suspend/resume calbacks in KMS drm drivers

[Daniel Vetter]

On Fri, Oct 03, 2014 at 10:24:09AM +0200, Andrzej Hajda wrote:
> The main intent of this patchset is to allow use of suspend/resume drm driver
> callbacks in KMS drivers, as these callbacks seems to me the best place
> to implement suspend/resume functionality in drm driver.
> Implementing this functionality in master component driver PM ops is problematic
> as those callbacks can be called asynchronously regardless of state/existence of
> drm device, thus it would require additional synchronization mechanism.
> 
> Callbacks re-enabling requires small changes in i915 and exynos driver.
> The patchset contains also fix of exynos resume callback.

Nack.

Like completely and totally. The drm core has really no business doing
hardware stuff, which includes runtime pm, system suspend and all that
nonsense. It' an interface between userspace and drivers, with a big
library to back it all up. Everything else just repeats the old midlayer
mistake.

If you driver needs this, do it there. Also, the component framework is
probably the solution you're looking for. And if there are synchronization
issues with that then we need to fix those instead of reinventing yet
another half-assed broken wheel.

Aside: With David Herrmann's latest patches to de-midlayer the drm
init/teardown sequence the driver is in full control of when the drm data
structures get allocate, initialized and registered. If you convert to
that plus the component framework I'm pretty sure your problem is solved.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH RFC 0/4] drm/core: restore suspend/resume calbacks in KMS drm drivers

[Andrzej Hajda]

On 10/03/2014 10:31 AM, Daniel Vetter wrote:
> On Fri, Oct 03, 2014 at 10:24:09AM +0200, Andrzej Hajda wrote:
>> The main intent of this patchset is to allow use of suspend/resume drm driver
>> callbacks in KMS drivers, as these callbacks seems to me the best place
>> to implement suspend/resume functionality in drm driver.
>> Implementing this functionality in master component driver PM ops is problematic
>> as those callbacks can be called asynchronously regardless of state/existence of
>> drm device, thus it would require additional synchronization mechanism.
>>
>> Callbacks re-enabling requires small changes in i915 and exynos driver.
>> The patchset contains also fix of exynos resume callback.
> Nack.
>
> Like completely and totally. The drm core has really no business doing
> hardware stuff, which includes runtime pm, system suspend and all that
> nonsense. It' an interface between userspace and drivers, with a big
> library to back it all up. Everything else just repeats the old midlayer
> mistake.

Hmm, I have just tried to reuse the existing infrastructure, I did not see
any sign "do not touch, this is a mistake". Now I see it, thanks :)

>
> If you driver needs this, do it there. Also, the component framework is
> probably the solution you're looking for. And if there are synchronization
> issues with that then we need to fix those instead of reinventing yet
> another half-assed broken wheel.

But this is an issue closely connected with component framework.
Component framework separates master component probe and drm device
initialization. As a result PM ops which are synchronized with probe
(via device_lock)
are no more synchronized with drm initialization which is usually called
from
.bind callback.

>
> Aside: With David Herrmann's latest patches to de-midlayer the drm
> init/teardown sequence the driver is in full control of when the drm data
> structures get allocate, initialized and registered. If you convert to
> that plus the component framework I'm pretty sure your problem is solved.

I will look closer at it but as I described above it is rather matter of
separation
of master component and drm device initialization.

My idea was to avoid creation of new synchronization mechanism and to
reuse the
existing ones which seems to fit perfectly to the scenario, but if there
is big NO for it
another solution should be found.

Anyway I guess the problem exists for all drivers having component
framework and suspend:
exynos, msm and incoming rockchip.


Regards
Andrzej

>
> Thanks, Daniel

Re: [PATCH RFC 0/4] drm/core: restore suspend/resume calbacks in KMS drm drivers

[Andrzej Hajda]

On 10/03/2014 10:31 AM, Daniel Vetter wrote:
> On Fri, Oct 03, 2014 at 10:24:09AM +0200, Andrzej Hajda wrote:
>> The main intent of this patchset is to allow use of suspend/resume drm driver
>> callbacks in KMS drivers, as these callbacks seems to me the best place
>> to implement suspend/resume functionality in drm driver.
>> Implementing this functionality in master component driver PM ops is problematic
>> as those callbacks can be called asynchronously regardless of state/existence of
>> drm device, thus it would require additional synchronization mechanism.
>>
>> Callbacks re-enabling requires small changes in i915 and exynos driver.
>> The patchset contains also fix of exynos resume callback.
> Nack.
>
> Like completely and totally. The drm core has really no business doing
> hardware stuff, which includes runtime pm, system suspend and all that
> nonsense. It' an interface between userspace and drivers, with a big
> library to back it all up. Everything else just repeats the old midlayer
> mistake.

Hmm, I have just tried to reuse the existing infrastructure, I did not see
any sign "do not touch, this is a mistake". Now I see it, thanks :)

>
> If you driver needs this, do it there. Also, the component framework is
> probably the solution you're looking for. And if there are synchronization
> issues with that then we need to fix those instead of reinventing yet
> another half-assed broken wheel.

But this is an issue closely connected with component framework.
Component framework separates master component probe and drm device
initialization. As a result PM ops which are synchronized with probe
(via device_lock)
are no more synchronized with drm initialization which is usually called
from
.bind callback.

>
> Aside: With David Herrmann's latest patches to de-midlayer the drm
> init/teardown sequence the driver is in full control of when the drm data
> structures get allocate, initialized and registered. If you convert to
> that plus the component framework I'm pretty sure your problem is solved.

I will look closer at it but as I described above it is rather matter of
separation
of master component and drm device initialization.

My idea was to avoid creation of new synchronization mechanism and to
reuse the
existing ones which seems to fit perfectly to the scenario, but if there
is big NO for it
another solution should be found.

Anyway I guess the problem exists for all drivers having component
framework and suspend:
exynos, msm and incoming rockchip.


Regards
Andrzej

>
> Thanks, Daniel

Re: [PATCH RFC 0/4] drm/core: restore suspend/resume calbacks in KMS drm drivers

[Daniel Vetter]

On Fri, Oct 3, 2014 at 11:42 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 10/03/2014 10:31 AM, Daniel Vetter wrote:
>> On Fri, Oct 03, 2014 at 10:24:09AM +0200, Andrzej Hajda wrote:
>>> The main intent of this patchset is to allow use of suspend/resume drm driver
>>> callbacks in KMS drivers, as these callbacks seems to me the best place
>>> to implement suspend/resume functionality in drm driver.
>>> Implementing this functionality in master component driver PM ops is problematic
>>> as those callbacks can be called asynchronously regardless of state/existence of
>>> drm device, thus it would require additional synchronization mechanism.
>>>
>>> Callbacks re-enabling requires small changes in i915 and exynos driver.
>>> The patchset contains also fix of exynos resume callback.
>> Nack.
>>
>> Like completely and totally. The drm core has really no business doing
>> hardware stuff, which includes runtime pm, system suspend and all that
>> nonsense. It' an interface between userspace and drivers, with a big
>> library to back it all up. Everything else just repeats the old midlayer
>> mistake.
>
> Hmm, I have just tried to reuse the existing infrastructure, I did not see
> any sign "do not touch, this is a mistake". Now I see it, thanks :)

As a rule of thumb, if you see a !DRIVER_MODESET check anywhere, then
that's a clear sign that you're wandering off the light and into the
dangerous parts of what drm was like age dark ages ;-)

>> If you driver needs this, do it there. Also, the component framework is
>> probably the solution you're looking for. And if there are synchronization
>> issues with that then we need to fix those instead of reinventing yet
>> another half-assed broken wheel.
>
> But this is an issue closely connected with component framework.
> Component framework separates master component probe and drm device
> initialization. As a result PM ops which are synchronized with probe
> (via device_lock)
> are no more synchronized with drm initialization which is usually called
> from
> .bind callback.

Now I'm confused. component->bind and drm initialization is about
driver load, but all this code here is about system suspend/resume
really. So I'm rather confused what problem you actually want to fix
..

>> Aside: With David Herrmann's latest patches to de-midlayer the drm
>> init/teardown sequence the driver is in full control of when the drm data
>> structures get allocate, initialized and registered. If you convert to
>> that plus the component framework I'm pretty sure your problem is solved.
>
> I will look closer at it but as I described above it is rather matter of
> separation
> of master component and drm device initialization.
>
> My idea was to avoid creation of new synchronization mechanism and to
> reuse the
> existing ones which seems to fit perfectly to the scenario, but if there
> is big NO for it
> another solution should be found.
>
> Anyway I guess the problem exists for all drivers having component
> framework and suspend:
> exynos, msm and incoming rockchip.

It sounds like the component framework needs to be teached to work
with system suspend/resume. I guess either we need to have clever
abuse of deferred probing to make sure that the master device is
probed first and so in the correct place in the system/resume
sequence. Or the master framework needs to grow pm_ops itself so that
the state associated with the logical componentized framework can be
saved/restored.

So master pm_ops would save/restore kms state, and all the components
would just save/restore any additional (register, clock, whatever)
state that each componnent driver would need.

But I'm not really an expert here, so adding more people.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH RFC 0/4] drm/core: restore suspend/resume calbacks in KMS drm drivers

[Daniel Vetter]

On Fri, Oct 3, 2014 at 11:42 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 10/03/2014 10:31 AM, Daniel Vetter wrote:
>> On Fri, Oct 03, 2014 at 10:24:09AM +0200, Andrzej Hajda wrote:
>>> The main intent of this patchset is to allow use of suspend/resume drm driver
>>> callbacks in KMS drivers, as these callbacks seems to me the best place
>>> to implement suspend/resume functionality in drm driver.
>>> Implementing this functionality in master component driver PM ops is problematic
>>> as those callbacks can be called asynchronously regardless of state/existence of
>>> drm device, thus it would require additional synchronization mechanism.
>>>
>>> Callbacks re-enabling requires small changes in i915 and exynos driver.
>>> The patchset contains also fix of exynos resume callback.
>> Nack.
>>
>> Like completely and totally. The drm core has really no business doing
>> hardware stuff, which includes runtime pm, system suspend and all that
>> nonsense. It' an interface between userspace and drivers, with a big
>> library to back it all up. Everything else just repeats the old midlayer
>> mistake.
>
> Hmm, I have just tried to reuse the existing infrastructure, I did not see
> any sign "do not touch, this is a mistake". Now I see it, thanks :)

As a rule of thumb, if you see a !DRIVER_MODESET check anywhere, then
that's a clear sign that you're wandering off the light and into the
dangerous parts of what drm was like age dark ages ;-)

>> If you driver needs this, do it there. Also, the component framework is
>> probably the solution you're looking for. And if there are synchronization
>> issues with that then we need to fix those instead of reinventing yet
>> another half-assed broken wheel.
>
> But this is an issue closely connected with component framework.
> Component framework separates master component probe and drm device
> initialization. As a result PM ops which are synchronized with probe
> (via device_lock)
> are no more synchronized with drm initialization which is usually called
> from
> .bind callback.

Now I'm confused. component->bind and drm initialization is about
driver load, but all this code here is about system suspend/resume
really. So I'm rather confused what problem you actually want to fix
..

>> Aside: With David Herrmann's latest patches to de-midlayer the drm
>> init/teardown sequence the driver is in full control of when the drm data
>> structures get allocate, initialized and registered. If you convert to
>> that plus the component framework I'm pretty sure your problem is solved.
>
> I will look closer at it but as I described above it is rather matter of
> separation
> of master component and drm device initialization.
>
> My idea was to avoid creation of new synchronization mechanism and to
> reuse the
> existing ones which seems to fit perfectly to the scenario, but if there
> is big NO for it
> another solution should be found.
>
> Anyway I guess the problem exists for all drivers having component
> framework and suspend:
> exynos, msm and incoming rockchip.

It sounds like the component framework needs to be teached to work
with system suspend/resume. I guess either we need to have clever
abuse of deferred probing to make sure that the master device is
probed first and so in the correct place in the system/resume
sequence. Or the master framework needs to grow pm_ops itself so that
the state associated with the logical componentized framework can be
saved/restored.

So master pm_ops would save/restore kms state, and all the components
would just save/restore any additional (register, clock, whatever)
state that each componnent driver would need.

But I'm not really an expert here, so adding more people.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH RFC 0/4] drm/core: restore suspend/resume calbacks in KMS drm drivers

[Russell King - ARM Linux]

On Fri, Oct 03, 2014 at 01:39:21PM +0200, Daniel Vetter wrote:
> On Fri, Oct 3, 2014 at 11:42 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> > But this is an issue closely connected with component framework.
> > Component framework separates master component probe and drm device
> > initialization. As a result PM ops which are synchronized with probe
> > (via device_lock)
> > are no more synchronized with drm initialization which is usually called
> > from
> > .bind callback.
> 
> Now I'm confused. component->bind and drm initialization is about
> driver load, but all this code here is about system suspend/resume
> really. So I'm rather confused what problem you actually want to fix
> ..

The component *helper* will disconnect the bind of the master device
from its probe if the components aren't already known.  If all the
components are known at the point the master device probes, the
master device lock will be held (since we'll be operating within the
master device's probe function.)

The issue here is that while the master device is being probed, the
per-device lock is held.  Beneath that lock there are locks in the
component helper which will be taken, and thus will end up depending
upon the per-device lock.

This means that we pretty much can not guarantee synchronisation between
PM on the master device and the component helper calling the bind
callback - if we tried to take the per-device lock here, we would either
deadlock, or we would end up with AB-BA lock dependencies.

What we /could/ do is expose lock/unlock functions from the component
helper so that PM implementations can synchronise with binds - or I could
look at whether we could integrate the component helper into the device
model.  I suspect the latter would not be met with enthusiasm as it would
mean adding bloat to the driver core structures, which would not be needed
in 99% of cases.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH RFC 0/4] drm/core: restore suspend/resume calbacks in KMS drm drivers

[Russell King - ARM Linux]

On Fri, Oct 03, 2014 at 01:39:21PM +0200, Daniel Vetter wrote:
> On Fri, Oct 3, 2014 at 11:42 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> > But this is an issue closely connected with component framework.
> > Component framework separates master component probe and drm device
> > initialization. As a result PM ops which are synchronized with probe
> > (via device_lock)
> > are no more synchronized with drm initialization which is usually called
> > from
> > .bind callback.
> 
> Now I'm confused. component->bind and drm initialization is about
> driver load, but all this code here is about system suspend/resume
> really. So I'm rather confused what problem you actually want to fix
> ..

The component *helper* will disconnect the bind of the master device
from its probe if the components aren't already known.  If all the
components are known at the point the master device probes, the
master device lock will be held (since we'll be operating within the
master device's probe function.)

The issue here is that while the master device is being probed, the
per-device lock is held.  Beneath that lock there are locks in the
component helper which will be taken, and thus will end up depending
upon the per-device lock.

This means that we pretty much can not guarantee synchronisation between
PM on the master device and the component helper calling the bind
callback - if we tried to take the per-device lock here, we would either
deadlock, or we would end up with AB-BA lock dependencies.

What we /could/ do is expose lock/unlock functions from the component
helper so that PM implementations can synchronise with binds - or I could
look at whether we could integrate the component helper into the device
model.  I suspect the latter would not be met with enthusiasm as it would
mean adding bloat to the driver core structures, which would not be needed
in 99% of cases.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.