[PATCH 00/13] fbdev locking rework and deferred setup, take 2

[PATCH 00/13] fbdev locking rework and deferred setup, take 2

[Daniel Vetter]

Hi all,

Thanks to Liviu's help I realized that I fumbled the locking rework completely.
This one here should be better, but somehow I'm having a real bad day today and
I spent all day typing shit code, and then making it worse.

This here seems to work at first glance, but please test and review carefully.

Thanks a lot.

Cheers, Daniel

Daniel Vetter (9):
  drm/i915: Drop FBDEV #ifdev in mst code
  drm/fb-helper: Push locking in fb_is_bound
  drm/fb-helper: Drop locking from the vsync wait ioctl code
  drm/fb-helper: Push locking into pan_display_atomic|legacy
  drm/fb-helper: Push locking into restore_fbdev_mode_atomic|legacy
  drm/fb-helper: Stop using mode_config.mutex for internals
  drm/fb-helper: Split dpms handling into legacy and atomic paths
  drm/fb-helper: Support deferred setup
  drm/atomic-helper: Realign function parameters

Thierry Reding (4):
  drm/fb-helper: Push down modeset lock into FB helpers
  drm/fb-helper: Add top-level lock
  drm/exynos: Remove custom FB helper deferred setup
  drm/hisilicon: Remove custom FB helper deferred setup

 drivers/gpu/drm/drm_atomic_helper.c             |  24 +-
 drivers/gpu/drm/drm_fb_helper.c                 | 362 +++++++++++++++++-------
 drivers/gpu/drm/drm_vblank.c                    |   2 +-
 drivers/gpu/drm/exynos/exynos_drm_drv.c         |   6 +-
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c       |  26 +-
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c |  21 +-
 drivers/gpu/drm/i915/intel_dp_mst.c             |  43 +--
 drivers/gpu/drm/i915/intel_fbdev.c              |  16 +-
 drivers/gpu/drm/radeon/radeon_dp_mst.c          |   7 -
 include/drm/drm_fb_helper.h                     |  42 ++-
 10 files changed, 345 insertions(+), 204 deletions(-)

-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/13] drm/fb-helper: Push down modeset lock into FB helpers

[Daniel Vetter]

From: Thierry Reding <treding@nvidia.com>

Move the modeset locking from drivers into FB helpers.

v2: Also handle intel_connector_add_to_fbdev.

Tested-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c        | 40 +++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_dp_mst.c    |  6 -----
 drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ------
 3 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 574af01d3ce9..5e73cc9f51e3 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -109,8 +109,8 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
 	for (({ lockdep_assert_held(&(fbh)->dev->mode_config.mutex); }), \
 	     i__ = 0; i__ < (fbh)->connector_count; i__++)
 
-int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
-				    struct drm_connector *connector)
+static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
+					     struct drm_connector *connector)
 {
 	struct drm_fb_helper_connector *fb_conn;
 	struct drm_fb_helper_connector **temp;
@@ -141,8 +141,23 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
 	drm_connector_get(connector);
 	fb_conn->connector = connector;
 	fb_helper->connector_info[fb_helper->connector_count++] = fb_conn;
+
 	return 0;
 }
+
+int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
+				    struct drm_connector *connector)
+{
+	int err;
+
+	mutex_lock(&fb_helper->dev->mode_config.mutex);
+
+	err = __drm_fb_helper_add_one_connector(fb_helper, connector);
+
+	mutex_unlock(&fb_helper->dev->mode_config.mutex);
+
+	return err;
+}
 EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
 
 /**
@@ -172,8 +187,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 	mutex_lock(&dev->mode_config.mutex);
 	drm_connector_list_iter_begin(dev, &conn_iter);
 	drm_for_each_connector_iter(connector, &conn_iter) {
-		ret = drm_fb_helper_add_one_connector(fb_helper, connector);
-
+		ret = __drm_fb_helper_add_one_connector(fb_helper, connector);
 		if (ret)
 			goto fail;
 	}
@@ -198,8 +212,8 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 }
 EXPORT_SYMBOL(drm_fb_helper_single_add_all_connectors);
 
-int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
-				       struct drm_connector *connector)
+static int __drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
+						struct drm_connector *connector)
 {
 	struct drm_fb_helper_connector *fb_helper_connector;
 	int i, j;
@@ -227,6 +241,20 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 
 	return 0;
 }
+
+int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
+				       struct drm_connector *connector)
+{
+	int err;
+
+	mutex_lock(&fb_helper->dev->mode_config.mutex);
+
+	err = __drm_fb_helper_remove_one_connector(fb_helper, connector);
+
+	mutex_unlock(&fb_helper->dev->mode_config.mutex);
+
+	return err;
+}
 EXPORT_SYMBOL(drm_fb_helper_remove_one_connector);
 
 static void drm_fb_helper_save_lut_atomic(struct drm_crtc *crtc, struct drm_fb_helper *helper)
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 2cf046beae0f..5b78165882ce 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -501,11 +501,8 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 static void intel_dp_register_mst_connector(struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
-	struct drm_device *dev = connector->dev;
 
-	drm_modeset_lock_all(dev);
 	intel_connector_add_to_fbdev(intel_connector);
-	drm_modeset_unlock_all(dev);
 
 	drm_connector_register(&intel_connector->base);
 }
@@ -514,15 +511,12 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 					   struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
-	struct drm_device *dev = connector->dev;
 
 	drm_connector_unregister(connector);
 
 	/* need to nuke the connector */
-	drm_modeset_lock_all(dev);
 	intel_connector_remove_from_fbdev(intel_connector);
 	intel_connector->mst_port = NULL;
-	drm_modeset_unlock_all(dev);
 
 	drm_connector_unreference(&intel_connector->base);
 	DRM_DEBUG_KMS("\n");
diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c
index 6598306dca9b..ebdf1b859cb6 100644
--- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
+++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
@@ -300,9 +300,7 @@ static void radeon_dp_register_mst_connector(struct drm_connector *connector)
 	struct drm_device *dev = connector->dev;
 	struct radeon_device *rdev = dev->dev_private;
 
-	drm_modeset_lock_all(dev);
 	radeon_fb_add_connector(rdev, connector);
-	drm_modeset_unlock_all(dev);
 
 	drm_connector_register(connector);
 }
@@ -315,13 +313,8 @@ static void radeon_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	struct radeon_device *rdev = dev->dev_private;
 
 	drm_connector_unregister(connector);
-	/* need to nuke the connector */
-	drm_modeset_lock_all(dev);
-	/* dpms off */
 	radeon_fb_remove_connector(rdev, connector);
-
 	drm_connector_cleanup(connector);
-	drm_modeset_unlock_all(dev);
 
 	kfree(connector);
 	DRM_DEBUG_KMS("\n");
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/13] drm/fb-helper: Push down modeset lock into FB helpers

[Maarten Lankhorst]

Op 27-06-17 om 16:59 schreef Daniel Vetter:
> From: Thierry Reding <treding@nvidia.com>
>
> Move the modeset locking from drivers into FB helpers.
>
> v2: Also handle intel_connector_add_to_fbdev.
>
> Tested-by: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_fb_helper.c        | 40 +++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_dp_mst.c    |  6 -----
>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ------
>  3 files changed, 34 insertions(+), 19 deletions(-)
I know we suck at DP-MST. But I fear the unregister leaves open a race with mst_port being unset without connection_mutex..

best_encoder() and mode_valid() appear to use it. It's probably harmless and I have no good solution, so maybe just annotate it in the patch? Might also affect i915_hpd_poll_init_work, though I think the race in itself is harmless.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/13] drm/fb-helper: Push down modeset lock into FB helpers

[Daniel Vetter]

On Thu, Jun 29, 2017 at 11:10 AM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Op 27-06-17 om 16:59 schreef Daniel Vetter:
>> From: Thierry Reding <treding@nvidia.com>
>>
>> Move the modeset locking from drivers into FB helpers.
>>
>> v2: Also handle intel_connector_add_to_fbdev.
>>
>> Tested-by: John Stultz <john.stultz@linaro.org>
>> Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>  drivers/gpu/drm/drm_fb_helper.c        | 40 +++++++++++++++++++++++++++++-----
>>  drivers/gpu/drm/i915/intel_dp_mst.c    |  6 -----
>>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ------
>>  3 files changed, 34 insertions(+), 19 deletions(-)
> I know we suck at DP-MST. But I fear the unregister leaves open a race with mst_port being unset without connection_mutex..
>
> best_encoder() and mode_valid() appear to use it. It's probably harmless and I have no good solution, so maybe just annotate it in the patch? Might also affect i915_hpd_poll_init_work, though I think the race in itself is harmless.

Hm, but this did race even before that already ... Should I just wrap
the mst_port = NULL in the connection_mutex? With a big warning that
we should have proper refcounting for this instead (both connectors
and all the mst things are refcounted already).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/13] drm/fb-helper: Push down modeset lock into FB helpers

[Maarten Lankhorst]

Op 29-06-17 om 11:23 schreef Daniel Vetter:
> On Thu, Jun 29, 2017 at 11:10 AM, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>> Op 27-06-17 om 16:59 schreef Daniel Vetter:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Move the modeset locking from drivers into FB helpers.
>>>
>>> v2: Also handle intel_connector_add_to_fbdev.
>>>
>>> Tested-by: John Stultz <john.stultz@linaro.org>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> ---
>>>  drivers/gpu/drm/drm_fb_helper.c        | 40 +++++++++++++++++++++++++++++-----
>>>  drivers/gpu/drm/i915/intel_dp_mst.c    |  6 -----
>>>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ------
>>>  3 files changed, 34 insertions(+), 19 deletions(-)
>> I know we suck at DP-MST. But I fear the unregister leaves open a race with mst_port being unset without connection_mutex..
>>
>> best_encoder() and mode_valid() appear to use it. It's probably harmless and I have no good solution, so maybe just annotate it in the patch? Might also affect i915_hpd_poll_init_work, though I think the race in itself is harmless.
> Hm, but this did race even before that already ... Should I just wrap
> the mst_port = NULL in the connection_mutex? With a big warning that
> we should have proper refcounting for this instead (both connectors
> and all the mst things are refcounted already).
> -Daniel

I think leave open the race, the DP-MST modeset code itself doesn't use the connector->mst_port, just the detect callbacks do. In case of nonblocking modeset mst_port might already fall away so it's not like the race is any worse now.

I don't even know how to solve it, except by not unplugging. :)

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/13] drm/fb-helper: Push down modeset lock into FB helpers

[Daniel Vetter]

On Thu, Jun 29, 2017 at 11:33 AM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Op 29-06-17 om 11:23 schreef Daniel Vetter:
>> On Thu, Jun 29, 2017 at 11:10 AM, Maarten Lankhorst
>> <maarten.lankhorst@linux.intel.com> wrote:
>>> Op 27-06-17 om 16:59 schreef Daniel Vetter:
>>>> From: Thierry Reding <treding@nvidia.com>
>>>>
>>>> Move the modeset locking from drivers into FB helpers.
>>>>
>>>> v2: Also handle intel_connector_add_to_fbdev.
>>>>
>>>> Tested-by: John Stultz <john.stultz@linaro.org>
>>>> Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> ---
>>>>  drivers/gpu/drm/drm_fb_helper.c        | 40 +++++++++++++++++++++++++++++-----
>>>>  drivers/gpu/drm/i915/intel_dp_mst.c    |  6 -----
>>>>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ------
>>>>  3 files changed, 34 insertions(+), 19 deletions(-)
>>> I know we suck at DP-MST. But I fear the unregister leaves open a race with mst_port being unset without connection_mutex..
>>>
>>> best_encoder() and mode_valid() appear to use it. It's probably harmless and I have no good solution, so maybe just annotate it in the patch? Might also affect i915_hpd_poll_init_work, though I think the race in itself is harmless.
>> Hm, but this did race even before that already ... Should I just wrap
>> the mst_port = NULL in the connection_mutex? With a big warning that
>> we should have proper refcounting for this instead (both connectors
>> and all the mst things are refcounted already).
>> -Daniel
>
> I think leave open the race, the DP-MST modeset code itself doesn't use the connector->mst_port, just the detect callbacks do. In case of nonblocking modeset mst_port might already fall away so it's not like the race is any worse now.
>
> I don't even know how to solve it, except by not unplugging. :)

Why does grabbing the lock not fix the race? ->detect eventually calls
drm_dp_get_validated_port_ref, which will bail on a zombie mst port..
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/13] drm/fb-helper: Push down modeset lock into FB helpers

[Maarten Lankhorst]

Op 29-06-17 om 11:44 schreef Daniel Vetter:
> On Thu, Jun 29, 2017 at 11:33 AM, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>> Op 29-06-17 om 11:23 schreef Daniel Vetter:
>>> On Thu, Jun 29, 2017 at 11:10 AM, Maarten Lankhorst
>>> <maarten.lankhorst@linux.intel.com> wrote:
>>>> Op 27-06-17 om 16:59 schreef Daniel Vetter:
>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>
>>>>> Move the modeset locking from drivers into FB helpers.
>>>>>
>>>>> v2: Also handle intel_connector_add_to_fbdev.
>>>>>
>>>>> Tested-by: John Stultz <john.stultz@linaro.org>
>>>>> Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>> ---
>>>>>  drivers/gpu/drm/drm_fb_helper.c        | 40 +++++++++++++++++++++++++++++-----
>>>>>  drivers/gpu/drm/i915/intel_dp_mst.c    |  6 -----
>>>>>  drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ------
>>>>>  3 files changed, 34 insertions(+), 19 deletions(-)
>>>> I know we suck at DP-MST. But I fear the unregister leaves open a race with mst_port being unset without connection_mutex..
>>>>
>>>> best_encoder() and mode_valid() appear to use it. It's probably harmless and I have no good solution, so maybe just annotate it in the patch? Might also affect i915_hpd_poll_init_work, though I think the race in itself is harmless.
>>> Hm, but this did race even before that already ... Should I just wrap
>>> the mst_port = NULL in the connection_mutex? With a big warning that
>>> we should have proper refcounting for this instead (both connectors
>>> and all the mst things are refcounted already).
>>> -Daniel
>> I think leave open the race, the DP-MST modeset code itself doesn't use the connector->mst_port, just the detect callbacks do. In case of nonblocking modeset mst_port might already fall away so it's not like the race is any worse now.
>>
>> I don't even know how to solve it, except by not unplugging. :)
> Why does grabbing the lock not fix the race? ->detect eventually calls
> drm_dp_get_validated_port_ref, which will bail on a zombie mst port..
Oh ok. But there will always be a race. If DP-MST fails immediately after drm_dp_get_validated_port_ref,
you'd still not be able to do anything about it. :)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/13] drm/fb-helper: Push down modeset lock into FB helpers

[Daniel Vetter]

On Thu, Jun 29, 2017 at 1:13 PM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Oh ok. But there will always be a race. If DP-MST fails immediately after drm_dp_get_validated_port_ref,
> you'd still not be able to do anything about it. :)

This is just about not oopsing, any transactions to the sink will
still go nowhere an fail.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/13] drm/i915: Drop FBDEV #ifdev in mst code

[Daniel Vetter]

Since

commit a03fdcb1863297481a4b817c2a759cafcbdfa0ae
Author: Archit Taneja <architt@codeaurora.org>
Date:   Wed Aug 5 12:28:57 2015 +0530

    drm: Add top level Kconfig option for DRM fbdev emulation

this is properly handled using dummy functions. This essentially
undoes

commit 7296c849bf2eca2bd7d34a4686a53e3089150ac1
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Tue Jul 22 20:10:28 2014 +1000

    drm/i915: fix build without fbde

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 37 ++++++++++---------------------------
 1 file changed, 10 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 5b78165882ce..c43ed29995b3 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -443,28 +443,6 @@ static bool intel_dp_mst_get_hw_state(struct intel_connector *connector)
 	return false;
 }
 
-static void intel_connector_add_to_fbdev(struct intel_connector *connector)
-{
-#ifdef CONFIG_DRM_FBDEV_EMULATION
-	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-
-	if (dev_priv->fbdev)
-		drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper,
-						&connector->base);
-#endif
-}
-
-static void intel_connector_remove_from_fbdev(struct intel_connector *connector)
-{
-#ifdef CONFIG_DRM_FBDEV_EMULATION
-	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-
-	if (dev_priv->fbdev)
-		drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper,
-						   &connector->base);
-#endif
-}
-
 static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port, const char *pathprop)
 {
 	struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
@@ -500,25 +478,30 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 
 static void intel_dp_register_mst_connector(struct drm_connector *connector)
 {
-	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 
-	intel_connector_add_to_fbdev(intel_connector);
+	if (dev_priv->fbdev)
+		drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper,
+						connector);
 
-	drm_connector_register(&intel_connector->base);
+	drm_connector_register(connector);
 }
 
 static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 					   struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 
 	drm_connector_unregister(connector);
 
 	/* need to nuke the connector */
-	intel_connector_remove_from_fbdev(intel_connector);
+	if (dev_priv->fbdev)
+		drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper,
+						   connector);
 	intel_connector->mst_port = NULL;
 
-	drm_connector_unreference(&intel_connector->base);
+	drm_connector_unreference(connector);
 	DRM_DEBUG_KMS("\n");
 }
 
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/13] drm/fb-helper: Add top-level lock

[Daniel Vetter]

From: Thierry Reding <treding@nvidia.com>

Introduce a new top-level lock for the FB helper code. This will allow
better locking granularity and avoid the need to abuse modeset locking
for this purpose instead.

This patch just adds the new lock everywhere we currently grab
mode_config->mutex (explicitly, or through drm_modeset_lock_all).
Follow-up patches will push the kms locking down into only the places
that need it.

v2:
- use lockdep_assert_held
- use drm_fb_helper_for_each_connector where possible
- use the new top-level lock consistently, i.e. in all the places
  we're currently acquiring mode_config.mutex.
- small polish to the kerneldoc

Tested-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c | 47 +++++++++++++++++++++++++++++++++++++----
 include/drm/drm_fb_helper.h     | 19 ++++++++++++++++-
 2 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 5e73cc9f51e3..a0d4603857d8 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -119,7 +119,8 @@ static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
 	if (!drm_fbdev_emulation)
 		return 0;
 
-	WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
+	lockdep_assert_held(&fb_helper->lock);
+	lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
 
 	count = fb_helper->connector_count + 1;
 
@@ -150,11 +151,13 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
 {
 	int err;
 
+	mutex_lock(&fb_helper->lock);
 	mutex_lock(&fb_helper->dev->mode_config.mutex);
 
 	err = __drm_fb_helper_add_one_connector(fb_helper, connector);
 
 	mutex_unlock(&fb_helper->dev->mode_config.mutex);
+	mutex_unlock(&fb_helper->lock);
 
 	return err;
 }
@@ -184,6 +187,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 	if (!drm_fbdev_emulation)
 		return 0;
 
+	mutex_lock(&fb_helper->lock);
 	mutex_lock(&dev->mode_config.mutex);
 	drm_connector_list_iter_begin(dev, &conn_iter);
 	drm_for_each_connector_iter(connector, &conn_iter) {
@@ -207,6 +211,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 out:
 	drm_connector_list_iter_end(&conn_iter);
 	mutex_unlock(&dev->mode_config.mutex);
+	mutex_unlock(&fb_helper->lock);
 
 	return ret;
 }
@@ -221,9 +226,9 @@ static int __drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 	if (!drm_fbdev_emulation)
 		return 0;
 
-	WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
+	lockdep_assert_held(&fb_helper->lock);
 
-	for (i = 0; i < fb_helper->connector_count; i++) {
+	drm_fb_helper_for_each_connector(fb_helper, i) {
 		if (fb_helper->connector_info[i]->connector == connector)
 			break;
 	}
@@ -247,11 +252,13 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 {
 	int err;
 
+	mutex_lock(&fb_helper->lock);
 	mutex_lock(&fb_helper->dev->mode_config.mutex);
 
 	err = __drm_fb_helper_remove_one_connector(fb_helper, connector);
 
 	mutex_unlock(&fb_helper->dev->mode_config.mutex);
+	mutex_unlock(&fb_helper->lock);
 
 	return err;
 }
@@ -517,16 +524,21 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 	if (!drm_fbdev_emulation)
 		return -ENODEV;
 
+	mutex_lock(&fb_helper->lock);
 	drm_modeset_lock_all(dev);
+
 	ret = restore_fbdev_mode(fb_helper);
 
 	do_delayed = fb_helper->delayed_hotplug;
 	if (do_delayed)
 		fb_helper->delayed_hotplug = false;
+
 	drm_modeset_unlock_all(dev);
+	mutex_unlock(&fb_helper->lock);
 
 	if (do_delayed)
 		drm_fb_helper_hotplug_event(fb_helper);
+
 	return ret;
 }
 EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked);
@@ -576,11 +588,13 @@ static bool drm_fb_helper_force_kernel_mode(void)
 		if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 			continue;
 
+		mutex_lock(&helper->lock);
 		drm_modeset_lock_all(dev);
 		ret = restore_fbdev_mode(helper);
 		if (ret)
 			error = true;
 		drm_modeset_unlock_all(dev);
+		mutex_unlock(&helper->lock);
 	}
 	return error;
 }
@@ -620,9 +634,11 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
 	/*
 	 * For each CRTC in this fb, turn the connectors on/off.
 	 */
+	mutex_lock(&fb_helper->lock);
 	drm_modeset_lock_all(dev);
 	if (!drm_fb_helper_is_bound(fb_helper)) {
 		drm_modeset_unlock_all(dev);
+		mutex_unlock(&fb_helper->lock);
 		return;
 	}
 
@@ -641,6 +657,7 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
 		}
 	}
 	drm_modeset_unlock_all(dev);
+	mutex_unlock(&fb_helper->lock);
 }
 
 /**
@@ -762,6 +779,7 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
 	INIT_WORK(&helper->resume_work, drm_fb_helper_resume_worker);
 	INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work);
 	helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0;
+	mutex_init(&helper->lock);
 	helper->funcs = funcs;
 	helper->dev = dev;
 }
@@ -927,6 +945,7 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 	}
 	mutex_unlock(&kernel_fb_helper_lock);
 
+	mutex_destroy(&fb_helper->lock);
 	drm_fb_helper_crtc_free(fb_helper);
 
 }
@@ -1257,9 +1276,11 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 	if (oops_in_progress)
 		return -EBUSY;
 
+	mutex_lock(&fb_helper->lock);
 	drm_modeset_lock_all(dev);
 	if (!drm_fb_helper_is_bound(fb_helper)) {
 		drm_modeset_unlock_all(dev);
+		mutex_unlock(&fb_helper->lock);
 		return -EBUSY;
 	}
 
@@ -1292,6 +1313,7 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 	}
  out:
 	drm_modeset_unlock_all(dev);
+	mutex_unlock(&fb_helper->lock);
 	return rc;
 }
 EXPORT_SYMBOL(drm_fb_helper_setcmap);
@@ -1314,6 +1336,7 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
 	struct drm_crtc *crtc;
 	int ret = 0;
 
+	mutex_lock(&fb_helper->lock);
 	mutex_lock(&dev->mode_config.mutex);
 	if (!drm_fb_helper_is_bound(fb_helper)) {
 		ret = -EBUSY;
@@ -1360,6 +1383,7 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
 
 unlock:
 	mutex_unlock(&dev->mode_config.mutex);
+	mutex_unlock(&fb_helper->lock);
 	return ret;
 }
 EXPORT_SYMBOL(drm_fb_helper_ioctl);
@@ -1589,9 +1613,11 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 	if (oops_in_progress)
 		return -EBUSY;
 
+	mutex_lock(&fb_helper->lock);
 	drm_modeset_lock_all(dev);
 	if (!drm_fb_helper_is_bound(fb_helper)) {
 		drm_modeset_unlock_all(dev);
+		mutex_unlock(&fb_helper->lock);
 		return -EBUSY;
 	}
 
@@ -1600,6 +1626,7 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 	else
 		ret = pan_display_legacy(var, info);
 	drm_modeset_unlock_all(dev);
+	mutex_unlock(&fb_helper->lock);
 
 	return ret;
 }
@@ -2266,6 +2293,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
 		DRM_DEBUG_KMS("No connectors reported connected with modes\n");
 
 	/* prevent concurrent modification of connector_count by hotplug */
+	lockdep_assert_held(&fb_helper->lock);
 	lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
 
 	crtcs = kcalloc(fb_helper->connector_count,
@@ -2392,12 +2420,14 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 	if (!drm_fbdev_emulation)
 		return 0;
 
+	mutex_lock(&fb_helper->lock);
 	mutex_lock(&dev->mode_config.mutex);
 	drm_setup_crtcs(fb_helper,
 			dev->mode_config.max_width,
 			dev->mode_config.max_height);
 	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
 	mutex_unlock(&dev->mode_config.mutex);
+	mutex_unlock(&fb_helper->lock);
 	if (ret)
 		return ret;
 
@@ -2445,25 +2475,34 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
 int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
+	int err = 0;
 
 	if (!drm_fbdev_emulation)
 		return 0;
 
+	mutex_lock(&fb_helper->lock);
 	mutex_lock(&dev->mode_config.mutex);
+
 	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
 		fb_helper->delayed_hotplug = true;
 		mutex_unlock(&dev->mode_config.mutex);
-		return 0;
+		goto unlock;
 	}
+
 	DRM_DEBUG_KMS("\n");
 
 	drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
 
 	mutex_unlock(&dev->mode_config.mutex);
+	mutex_unlock(&fb_helper->lock);
 
 	drm_fb_helper_set_par(fb_helper->fbdev);
 
 	return 0;
+
+unlock:
+	mutex_unlock(&fb_helper->lock);
+	return err;
 }
 EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
 
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 119e5e4609c7..ea170b96e88d 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -169,7 +169,6 @@ struct drm_fb_helper_connector {
  * @crtc_info: per-CRTC helper state (mode, x/y offset, etc)
  * @connector_count: number of connected connectors
  * @connector_info_alloc_count: size of connector_info
- * @connector_info: array of per-connector information
  * @funcs: driver callbacks for fb helper
  * @fbdev: emulated fbdev device info struct
  * @pseudo_palette: fake palette of 16 colors
@@ -191,6 +190,12 @@ struct drm_fb_helper {
 	struct drm_fb_helper_crtc *crtc_info;
 	int connector_count;
 	int connector_info_alloc_count;
+	/**
+	 * @connector_info:
+	 *
+	 * Array of per-connector information. Do not iterate directly, but use
+	 * drm_fb_helper_for_each_connector.
+	 */
 	struct drm_fb_helper_connector **connector_info;
 	const struct drm_fb_helper_funcs *funcs;
 	struct fb_info *fbdev;
@@ -201,6 +206,18 @@ struct drm_fb_helper {
 	struct work_struct resume_work;
 
 	/**
+	 * @lock:
+	 *
+	 * Top-level FBDEV helper lock. This protects all internal data
+	 * structures and lists, such as @connector_info and @crtc_info.
+	 *
+	 * FIXME: fbdev emulation locking is a mess and long term we want to
+	 * protect all helper internal state with this lock as well as reduce
+	 * core KMS locking as much as possible.
+	 */
+	struct mutex lock;
+
+	/**
 	 * @kernel_fb_list:
 	 *
 	 * Entry on the global kernel_fb_helper_list, used for kgdb entry/exit.
-- 
2.11.0

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

[PATCH 04/13] drm/fb-helper: Push locking in fb_is_bound

[Daniel Vetter]

That function only needs to take the individual crtc locks, not all
the kms locks. Push down the locking and then minimize it.

Cc: John Stultz <john.stultz@linaro.org>
Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a0d4603857d8..14b3f885a01f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -557,10 +557,12 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
 		return false;
 
 	drm_for_each_crtc(crtc, dev) {
+		drm_modeset_lock(&crtc->mutex, NULL);
 		if (crtc->primary->fb)
 			crtcs_bound++;
 		if (crtc->primary->fb == fb_helper->fb)
 			bound++;
+		drm_modeset_unlock(&crtc->mutex);
 	}
 
 	if (bound < crtcs_bound)
@@ -635,13 +637,12 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
 	 * For each CRTC in this fb, turn the connectors on/off.
 	 */
 	mutex_lock(&fb_helper->lock);
-	drm_modeset_lock_all(dev);
 	if (!drm_fb_helper_is_bound(fb_helper)) {
-		drm_modeset_unlock_all(dev);
 		mutex_unlock(&fb_helper->lock);
 		return;
 	}
 
+	drm_modeset_lock_all(dev);
 	for (i = 0; i < fb_helper->crtc_count; i++) {
 		crtc = fb_helper->crtc_info[i].mode_set.crtc;
 
@@ -1277,13 +1278,12 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 		return -EBUSY;
 
 	mutex_lock(&fb_helper->lock);
-	drm_modeset_lock_all(dev);
 	if (!drm_fb_helper_is_bound(fb_helper)) {
-		drm_modeset_unlock_all(dev);
 		mutex_unlock(&fb_helper->lock);
 		return -EBUSY;
 	}
 
+	drm_modeset_lock_all(dev);
 	for (i = 0; i < fb_helper->crtc_count; i++) {
 		crtc = fb_helper->crtc_info[i].mode_set.crtc;
 		crtc_funcs = crtc->helper_private;
@@ -1337,12 +1337,12 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
 	int ret = 0;
 
 	mutex_lock(&fb_helper->lock);
-	mutex_lock(&dev->mode_config.mutex);
 	if (!drm_fb_helper_is_bound(fb_helper)) {
 		ret = -EBUSY;
 		goto unlock;
 	}
 
+	mutex_lock(&dev->mode_config.mutex);
 	switch (cmd) {
 	case FBIO_WAITFORVSYNC:
 		/*
@@ -1614,13 +1614,12 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 		return -EBUSY;
 
 	mutex_lock(&fb_helper->lock);
-	drm_modeset_lock_all(dev);
 	if (!drm_fb_helper_is_bound(fb_helper)) {
-		drm_modeset_unlock_all(dev);
 		mutex_unlock(&fb_helper->lock);
 		return -EBUSY;
 	}
 
+	drm_modeset_lock_all(dev);
 	if (drm_drv_uses_atomic_modeset(dev))
 		ret = pan_display_atomic(var, info);
 	else
@@ -2481,16 +2480,15 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 		return 0;
 
 	mutex_lock(&fb_helper->lock);
-	mutex_lock(&dev->mode_config.mutex);
-
 	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
 		fb_helper->delayed_hotplug = true;
-		mutex_unlock(&dev->mode_config.mutex);
-		goto unlock;
+		mutex_unlock(&fb_helper->lock);
+		return err;
 	}
 
 	DRM_DEBUG_KMS("\n");
 
+	mutex_lock(&dev->mode_config.mutex);
 	drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
 
 	mutex_unlock(&dev->mode_config.mutex);
@@ -2499,10 +2497,6 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 	drm_fb_helper_set_par(fb_helper->fbdev);
 
 	return 0;
-
-unlock:
-	mutex_unlock(&fb_helper->lock);
-	return err;
 }
 EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
 
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/13] drm/fb-helper: Drop locking from the vsync wait ioctl code

[Daniel Vetter]

Like with the drm-native vblank wait ioctl we can entirely rely on the
spinlocks in drm_vblank.c, no need at all to take expensive mutexes.
The only reason we had to take mode_config.mutex was to protect the
fbdev helper's data-structures, but that's now done by
fb_helper->lock.

Cc: John Stultz <john.stultz@linaro.org>
Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 14b3f885a01f..13330c22e6bf 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1342,7 +1342,6 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
 		goto unlock;
 	}
 
-	mutex_lock(&dev->mode_config.mutex);
 	switch (cmd) {
 	case FBIO_WAITFORVSYNC:
 		/*
@@ -1382,7 +1381,6 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
 	}
 
 unlock:
-	mutex_unlock(&dev->mode_config.mutex);
 	mutex_unlock(&fb_helper->lock);
 	return ret;
 }
-- 
2.11.0

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

[PATCH 06/13] drm/fb-helper: Push locking into pan_display_atomic|legacy

[Daniel Vetter]

For the legacy path we'll keep drm_modeset_lock_all, for the atomic
one we drop the use of the magic implicit context and wire it up
properly.

Cc: John Stultz <john.stultz@linaro.org>
Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 13330c22e6bf..5d8bf7dfa618 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1331,7 +1331,6 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
 			unsigned long arg)
 {
 	struct drm_fb_helper *fb_helper = info->par;
-	struct drm_device *dev = fb_helper->dev;
 	struct drm_mode_set *mode_set;
 	struct drm_crtc *crtc;
 	int ret = 0;
@@ -1522,12 +1521,17 @@ static int pan_display_atomic(struct fb_var_screeninfo *var,
 	struct drm_plane *plane;
 	int i, ret;
 	unsigned int plane_mask;
+	struct drm_modeset_acquire_ctx ctx;
+
+	drm_modeset_acquire_init(&ctx, 0);
 
 	state = drm_atomic_state_alloc(dev);
-	if (!state)
-		return -ENOMEM;
+	if (!state) {
+		ret = -ENOMEM;
+		goto out_ctx;
+	}
 
-	state->acquire_ctx = dev->mode_config.acquire_ctx;
+	state->acquire_ctx = &ctx;
 retry:
 	plane_mask = 0;
 	for (i = 0; i < fb_helper->crtc_count; i++) {
@@ -1540,7 +1544,7 @@ static int pan_display_atomic(struct fb_var_screeninfo *var,
 
 		ret = __drm_atomic_helper_set_config(mode_set, state);
 		if (ret != 0)
-			goto fail;
+			goto out_state;
 
 		plane = mode_set->crtc->primary;
 		plane_mask |= (1 << drm_plane_index(plane));
@@ -1549,23 +1553,27 @@ static int pan_display_atomic(struct fb_var_screeninfo *var,
 
 	ret = drm_atomic_commit(state);
 	if (ret != 0)
-		goto fail;
+		goto out_state;
 
 	info->var.xoffset = var->xoffset;
 	info->var.yoffset = var->yoffset;
 
-fail:
+out_state:
 	drm_atomic_clean_old_fb(dev, plane_mask, ret);
 
 	if (ret == -EDEADLK)
 		goto backoff;
 
 	drm_atomic_state_put(state);
+out_ctx:
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
 	return ret;
 
 backoff:
 	drm_atomic_state_clear(state);
-	drm_atomic_legacy_backoff(state);
+	drm_modeset_backoff(&ctx);
 
 	goto retry;
 }
@@ -1578,6 +1586,7 @@ static int pan_display_legacy(struct fb_var_screeninfo *var,
 	int ret = 0;
 	int i;
 
+	drm_modeset_lock_all(fb_helper->dev);
 	for (i = 0; i < fb_helper->crtc_count; i++) {
 		modeset = &fb_helper->crtc_info[i].mode_set;
 
@@ -1592,6 +1601,7 @@ static int pan_display_legacy(struct fb_var_screeninfo *var,
 			}
 		}
 	}
+	drm_modeset_unlock_all(fb_helper->dev);
 
 	return ret;
 }
@@ -1617,12 +1627,10 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 		return -EBUSY;
 	}
 
-	drm_modeset_lock_all(dev);
 	if (drm_drv_uses_atomic_modeset(dev))
 		ret = pan_display_atomic(var, info);
 	else
 		ret = pan_display_legacy(var, info);
-	drm_modeset_unlock_all(dev);
 	mutex_unlock(&fb_helper->lock);
 
 	return ret;
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/13] drm/fb-helper: Push locking into restore_fbdev_mode_atomic|legacy

[Daniel Vetter]

Same game as with the panning function, use drm_modeset_lock_all for
legacy paths, and a proper acquire ctx w/w mutex dance for atomic.

Cc: John Stultz <john.stultz@linaro.org>
Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 48 +++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 5d8bf7dfa618..400bbb07eff2 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -396,12 +396,17 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
 	struct drm_atomic_state *state;
 	int i, ret;
 	unsigned int plane_mask;
+	struct drm_modeset_acquire_ctx ctx;
+
+	drm_modeset_acquire_init(&ctx, 0);
 
 	state = drm_atomic_state_alloc(dev);
-	if (!state)
-		return -ENOMEM;
+	if (!state) {
+		ret = -ENOMEM;
+		goto out_ctx;
+	}
 
-	state->acquire_ctx = dev->mode_config.acquire_ctx;
+	state->acquire_ctx = &ctx;
 retry:
 	plane_mask = 0;
 	drm_for_each_plane(plane, dev) {
@@ -410,7 +415,7 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
 		plane_state = drm_atomic_get_plane_state(state, plane);
 		if (IS_ERR(plane_state)) {
 			ret = PTR_ERR(plane_state);
-			goto fail;
+			goto out_state;
 		}
 
 		plane_state->rotation = DRM_MODE_ROTATE_0;
@@ -424,7 +429,7 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
 
 		ret = __drm_atomic_helper_disable_plane(plane, plane_state);
 		if (ret != 0)
-			goto fail;
+			goto out_state;
 	}
 
 	for (i = 0; i < fb_helper->crtc_count; i++) {
@@ -432,23 +437,27 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
 
 		ret = __drm_atomic_helper_set_config(mode_set, state);
 		if (ret != 0)
-			goto fail;
+			goto out_state;
 	}
 
 	ret = drm_atomic_commit(state);
 
-fail:
+out_state:
 	drm_atomic_clean_old_fb(dev, plane_mask, ret);
 
 	if (ret == -EDEADLK)
 		goto backoff;
 
 	drm_atomic_state_put(state);
+out_ctx:
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
 	return ret;
 
 backoff:
 	drm_atomic_state_clear(state);
-	drm_atomic_legacy_backoff(state);
+	drm_modeset_backoff(&ctx);
 
 	goto retry;
 }
@@ -457,8 +466,9 @@ static int restore_fbdev_mode_legacy(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
 	struct drm_plane *plane;
-	int i;
+	int i, ret = 0;
 
+	drm_modeset_lock_all(fb_helper->dev);
 	drm_for_each_plane(plane, dev) {
 		if (plane->type != DRM_PLANE_TYPE_PRIMARY)
 			drm_plane_force_disable(plane);
@@ -472,32 +482,31 @@ static int restore_fbdev_mode_legacy(struct drm_fb_helper *fb_helper)
 	for (i = 0; i < fb_helper->crtc_count; i++) {
 		struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set;
 		struct drm_crtc *crtc = mode_set->crtc;
-		int ret;
 
 		if (crtc->funcs->cursor_set2) {
 			ret = crtc->funcs->cursor_set2(crtc, NULL, 0, 0, 0, 0, 0);
 			if (ret)
-				return ret;
+				goto out;
 		} else if (crtc->funcs->cursor_set) {
 			ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0);
 			if (ret)
-				return ret;
+				goto out;
 		}
 
 		ret = drm_mode_set_config_internal(mode_set);
 		if (ret)
-			return ret;
+			goto out;
 	}
+out:
+	drm_modeset_unlock_all(fb_helper->dev);
 
-	return 0;
+	return ret;
 }
 
 static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
 
-	drm_warn_on_modeset_not_all_locked(dev);
-
 	if (drm_drv_uses_atomic_modeset(dev))
 		return restore_fbdev_mode_atomic(fb_helper);
 	else
@@ -517,7 +526,6 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
  */
 int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 {
-	struct drm_device *dev = fb_helper->dev;
 	bool do_delayed;
 	int ret;
 
@@ -525,15 +533,11 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 		return -ENODEV;
 
 	mutex_lock(&fb_helper->lock);
-	drm_modeset_lock_all(dev);
-
 	ret = restore_fbdev_mode(fb_helper);
 
 	do_delayed = fb_helper->delayed_hotplug;
 	if (do_delayed)
 		fb_helper->delayed_hotplug = false;
-
-	drm_modeset_unlock_all(dev);
 	mutex_unlock(&fb_helper->lock);
 
 	if (do_delayed)
@@ -591,11 +595,9 @@ static bool drm_fb_helper_force_kernel_mode(void)
 			continue;
 
 		mutex_lock(&helper->lock);
-		drm_modeset_lock_all(dev);
 		ret = restore_fbdev_mode(helper);
 		if (ret)
 			error = true;
-		drm_modeset_unlock_all(dev);
 		mutex_unlock(&helper->lock);
 	}
 	return error;
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/13] drm/fb-helper: Stop using mode_config.mutex for internals

[Daniel Vetter]

Those are now all protected using fb_helper->lock.

v2: We still need to hold mode_config.mutex right around calling
connector->fill_modes.

v3: I forgot to hold mode_config.mutex while looking at
connector->status and the mode list. Also, we need to patch up the
i915 ->initial_config callback to grab the locks it needs to inspect
the modeset state recovered from the fw.

Cc: John Stultz <john.stultz@linaro.org>
Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c    | 33 ++++++++-------------------------
 drivers/gpu/drm/drm_vblank.c       |  2 +-
 drivers/gpu/drm/i915/intel_fbdev.c | 16 ++++++++++++----
 3 files changed, 21 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 400bbb07eff2..59e2916471b2 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -106,7 +106,7 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
  */
 
 #define drm_fb_helper_for_each_connector(fbh, i__) \
-	for (({ lockdep_assert_held(&(fbh)->dev->mode_config.mutex); }), \
+	for (({ lockdep_assert_held(&(fbh)->lock); }), \
 	     i__ = 0; i__ < (fbh)->connector_count; i__++)
 
 static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
@@ -120,7 +120,6 @@ static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
 		return 0;
 
 	lockdep_assert_held(&fb_helper->lock);
-	lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
 
 	count = fb_helper->connector_count + 1;
 
@@ -152,11 +151,7 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
 	int err;
 
 	mutex_lock(&fb_helper->lock);
-	mutex_lock(&fb_helper->dev->mode_config.mutex);
-
 	err = __drm_fb_helper_add_one_connector(fb_helper, connector);
-
-	mutex_unlock(&fb_helper->dev->mode_config.mutex);
 	mutex_unlock(&fb_helper->lock);
 
 	return err;
@@ -188,7 +183,6 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 		return 0;
 
 	mutex_lock(&fb_helper->lock);
-	mutex_lock(&dev->mode_config.mutex);
 	drm_connector_list_iter_begin(dev, &conn_iter);
 	drm_for_each_connector_iter(connector, &conn_iter) {
 		ret = __drm_fb_helper_add_one_connector(fb_helper, connector);
@@ -210,7 +204,6 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 	fb_helper->connector_count = 0;
 out:
 	drm_connector_list_iter_end(&conn_iter);
-	mutex_unlock(&dev->mode_config.mutex);
 	mutex_unlock(&fb_helper->lock);
 
 	return ret;
@@ -253,11 +246,7 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 	int err;
 
 	mutex_lock(&fb_helper->lock);
-	mutex_lock(&fb_helper->dev->mode_config.mutex);
-
 	err = __drm_fb_helper_remove_one_connector(fb_helper, connector);
-
-	mutex_unlock(&fb_helper->dev->mode_config.mutex);
 	mutex_unlock(&fb_helper->lock);
 
 	return err;
@@ -1893,12 +1882,11 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe
 EXPORT_SYMBOL(drm_fb_helper_fill_var);
 
 static int drm_fb_helper_probe_connector_modes(struct drm_fb_helper *fb_helper,
-					       uint32_t maxX,
-					       uint32_t maxY)
+						uint32_t maxX,
+						uint32_t maxY)
 {
 	struct drm_connector *connector;
-	int count = 0;
-	int i;
+	int i, count = 0;
 
 	drm_fb_helper_for_each_connector(fb_helper, i) {
 		connector = fb_helper->connector_info[i]->connector;
@@ -2296,12 +2284,8 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
 	int i;
 
 	DRM_DEBUG_KMS("\n");
-	if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
-		DRM_DEBUG_KMS("No connectors reported connected with modes\n");
-
 	/* prevent concurrent modification of connector_count by hotplug */
 	lockdep_assert_held(&fb_helper->lock);
-	lockdep_assert_held(&fb_helper->dev->mode_config.mutex);
 
 	crtcs = kcalloc(fb_helper->connector_count,
 			sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL);
@@ -2316,7 +2300,10 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
 		goto out;
 	}
 
+	mutex_lock(&fb_helper->dev->mode_config.mutex);
 	drm_enable_connectors(fb_helper, enabled);
+	if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
+		DRM_DEBUG_KMS("No connectors reported connected with modes\n");
 
 	if (!(fb_helper->funcs->initial_config &&
 	      fb_helper->funcs->initial_config(fb_helper, crtcs, modes,
@@ -2337,6 +2324,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
 
 		drm_pick_crtcs(fb_helper, crtcs, modes, 0, width, height);
 	}
+	mutex_unlock(&fb_helper->dev->mode_config.mutex);
 
 	/* need to set the modesets up here for use later */
 	/* fill out the connector<->crtc mappings into the modesets */
@@ -2428,12 +2416,10 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 		return 0;
 
 	mutex_lock(&fb_helper->lock);
-	mutex_lock(&dev->mode_config.mutex);
 	drm_setup_crtcs(fb_helper,
 			dev->mode_config.max_width,
 			dev->mode_config.max_height);
 	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
-	mutex_unlock(&dev->mode_config.mutex);
 	mutex_unlock(&fb_helper->lock);
 	if (ret)
 		return ret;
@@ -2496,10 +2482,7 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 
 	DRM_DEBUG_KMS("\n");
 
-	mutex_lock(&dev->mode_config.mutex);
 	drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
-
-	mutex_unlock(&dev->mode_config.mutex);
 	mutex_unlock(&fb_helper->lock);
 
 	drm_fb_helper_set_par(fb_helper->fbdev);
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 05d043e9219f..8099574c8a11 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -836,7 +836,7 @@ static void send_vblank_event(struct drm_device *dev,
  * NOTE: Drivers using this to send out the &drm_crtc_state.event as part of an
  * atomic commit must ensure that the next vblank happens at exactly the same
  * time as the atomic commit is committed to the hardware. This function itself
- * does **not** protect again the next vblank interrupt racing with either this
+ * does **not** protect against the next vblank interrupt racing with either this
  * function call or the atomic commit operation. A possible sequence could be:
  *
  * 1. Driver commits new hardware state into vblank-synchronized registers.
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 03347c6ae599..460ca0b3fb88 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -352,14 +352,20 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 	unsigned int count = min(fb_helper->connector_count, BITS_PER_LONG);
 	int i, j;
 	bool *save_enabled;
-	bool fallback = true;
+	bool fallback = true, ret = true;
 	int num_connectors_enabled = 0;
 	int num_connectors_detected = 0;
+	struct drm_modeset_acquire_ctx ctx;
 
 	save_enabled = kcalloc(count, sizeof(bool), GFP_KERNEL);
 	if (!save_enabled)
 		return false;
 
+	drm_modeset_acquire_init(&ctx, 0);
+
+	while (drm_modeset_lock_all_ctx(fb_helper->dev, &ctx) != 0)
+		drm_modeset_backoff(&ctx);
+
 	memcpy(save_enabled, enabled, count);
 	mask = GENMASK(count - 1, 0);
 	conn_configured = 0;
@@ -509,12 +515,14 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 bail:
 		DRM_DEBUG_KMS("Not using firmware configuration\n");
 		memcpy(enabled, save_enabled, count);
-		kfree(save_enabled);
-		return false;
+		ret = false;
 	}
 
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
 	kfree(save_enabled);
-	return true;
+	return ret;
 }
 
 static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/13] drm/fb-helper: Split dpms handling into legacy and atomic paths

[Daniel Vetter]

Like with panning and modesetting, and like with those, stick with
simple drm_modeset_locking_all for the legacy path, and the full
atomic dance for atomic drivers.

This means a bit more boilerplate since setting up the atomic state
machinery is rather verbose, but then this is shared code for 30+
drivers or so, so meh.

After this patch there's only the LUT/cmap path which is still using
drm_modeset_lock_all for an atomic driver. But Peter is already
locking into reworking that, so I'll leave that code as-is for now.

Cc: Peter Rosin <peda@axentia.se>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 88 +++++++++++++++++++++++++++++++++++------
 1 file changed, 76 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 59e2916471b2..bbd4c6d0378d 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -616,23 +616,13 @@ static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = {
 static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { };
 #endif
 
-static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
+static void dpms_legacy(struct drm_fb_helper *fb_helper, int dpms_mode)
 {
-	struct drm_fb_helper *fb_helper = info->par;
 	struct drm_device *dev = fb_helper->dev;
 	struct drm_crtc *crtc;
 	struct drm_connector *connector;
 	int i, j;
 
-	/*
-	 * For each CRTC in this fb, turn the connectors on/off.
-	 */
-	mutex_lock(&fb_helper->lock);
-	if (!drm_fb_helper_is_bound(fb_helper)) {
-		mutex_unlock(&fb_helper->lock);
-		return;
-	}
-
 	drm_modeset_lock_all(dev);
 	for (i = 0; i < fb_helper->crtc_count; i++) {
 		crtc = fb_helper->crtc_info[i].mode_set.crtc;
@@ -649,6 +639,81 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
 		}
 	}
 	drm_modeset_unlock_all(dev);
+}
+
+static void dpms_atomic(struct drm_fb_helper *fb_helper, int dpms_mode)
+{
+	struct drm_device *dev = fb_helper->dev;
+	struct drm_atomic_state *state;
+	int i, ret;
+
+	struct drm_modeset_acquire_ctx ctx;
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state) {
+		ret = -ENOMEM;
+		goto out_ctx;
+	}
+
+	state->acquire_ctx = &ctx;
+retry:
+	for (i = 0; i < fb_helper->crtc_count; i++) {
+		struct drm_crtc_state *crtc_state;
+		struct drm_crtc *crtc;
+
+		if (!fb_helper->crtc_info[i].mode_set.mode)
+			continue;
+
+		crtc = fb_helper->crtc_info[i].mode_set.crtc;
+
+		crtc_state = drm_atomic_get_crtc_state(state, crtc);
+		if (IS_ERR(crtc_state)) {
+			ret = PTR_ERR(crtc_state);
+			goto out_state;
+		}
+
+		crtc_state->active = dpms_mode == DRM_MODE_DPMS_ON;
+	}
+
+	ret = drm_atomic_commit(state);
+out_state:
+	if (ret == -EDEADLK)
+		goto backoff;
+
+	drm_atomic_state_put(state);
+out_ctx:
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
+	return;
+
+backoff:
+	drm_atomic_state_clear(state);
+	drm_modeset_backoff(&ctx);
+
+	goto retry;
+
+}
+
+static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+
+	/*
+	 * For each CRTC in this fb, turn the connectors on/off.
+	 */
+	mutex_lock(&fb_helper->lock);
+	if (!drm_fb_helper_is_bound(fb_helper)) {
+		mutex_unlock(&fb_helper->lock);
+		return;
+	}
+
+	if (drm_drv_uses_atomic_modeset(fb_helper->dev))
+		dpms_atomic(fb_helper, dpms_mode);
+	else
+		dpms_legacy(fb_helper, dpms_mode);
 	mutex_unlock(&fb_helper->lock);
 }
 
@@ -2467,7 +2532,6 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
  */
 int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 {
-	struct drm_device *dev = fb_helper->dev;
 	int err = 0;
 
 	if (!drm_fbdev_emulation)
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/13] drm/fb-helper: Split dpms handling into legacy and atomic paths

[Maarten Lankhorst]

Op 27-06-17 om 16:59 schreef Daniel Vetter:
> Like with panning and modesetting, and like with those, stick with
> simple drm_modeset_locking_all for the legacy path, and the full
> atomic dance for atomic drivers.
>
> This means a bit more boilerplate since setting up the atomic state
> machinery is rather verbose, but then this is shared code for 30+
> drivers or so, so meh.
>
> After this patch there's only the LUT/cmap path which is still using
> drm_modeset_lock_all for an atomic driver. But Peter is already
> locking into reworking that, so I'll leave that code as-is for now.
>
> Cc: Peter Rosin <peda@axentia.se>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 88 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 76 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 59e2916471b2..bbd4c6d0378d 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -616,23 +616,13 @@ static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = {
>  static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { };
>  #endif
>  
> -static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
> +static void dpms_legacy(struct drm_fb_helper *fb_helper, int dpms_mode)
>  {
> -	struct drm_fb_helper *fb_helper = info->par;
>  	struct drm_device *dev = fb_helper->dev;
>  	struct drm_crtc *crtc;
>  	struct drm_connector *connector;
>  	int i, j;
>  
> -	/*
> -	 * For each CRTC in this fb, turn the connectors on/off.
> -	 */
> -	mutex_lock(&fb_helper->lock);
> -	if (!drm_fb_helper_is_bound(fb_helper)) {
> -		mutex_unlock(&fb_helper->lock);
> -		return;
> -	}
> -
>  	drm_modeset_lock_all(dev);
>  	for (i = 0; i < fb_helper->crtc_count; i++) {
>  		crtc = fb_helper->crtc_info[i].mode_set.crtc;
> @@ -649,6 +639,81 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
>  		}
>  	}
>  	drm_modeset_unlock_all(dev);
> +}
> +
> +static void dpms_atomic(struct drm_fb_helper *fb_helper, int dpms_mode)
> +{
> +	struct drm_device *dev = fb_helper->dev;
> +	struct drm_atomic_state *state;
> +	int i, ret;
> +
> +	struct drm_modeset_acquire_ctx ctx;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state) {
> +		ret = -ENOMEM;
> +		goto out_ctx;
> +	}
> +
> +	state->acquire_ctx = &ctx;
> +retry:
> +	for (i = 0; i < fb_helper->crtc_count; i++) {
> +		struct drm_crtc_state *crtc_state;
> +		struct drm_crtc *crtc;
> +
> +		if (!fb_helper->crtc_info[i].mode_set.mode)
> +			continue;
> +
> +		crtc = fb_helper->crtc_info[i].mode_set.crtc;
> +
> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +		if (IS_ERR(crtc_state)) {
> +			ret = PTR_ERR(crtc_state);
> +			goto out_state;
> +		}
Hm, maybe remove the early continue, and change this to if (crtc_state->enable) crtc_state->active = ...; ?

I don't know if it matters in practice, but it might be more resilient when crtc state does not match our expected state,
similar to how DPMS on is ignored without CRTC.
> +		crtc_state->active = dpms_mode == DRM_MODE_DPMS_ON;
> +	}
> +
> +	ret = drm_atomic_commit(state);
> +out_state:
> +	if (ret == -EDEADLK)
> +		goto backoff;
> +
> +	drm_atomic_state_put(state);
> +out_ctx:
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +
> +	return;
> +
> +backoff:
> +	drm_atomic_state_clear(state);
> +	drm_modeset_backoff(&ctx);
> +
> +	goto retry;
> +
> +}
> +
> +static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
> +{
> +	struct drm_fb_helper *fb_helper = info->par;
> +
> +	/*
> +	 * For each CRTC in this fb, turn the connectors on/off.
> +	 */
> +	mutex_lock(&fb_helper->lock);
> +	if (!drm_fb_helper_is_bound(fb_helper)) {
> +		mutex_unlock(&fb_helper->lock);
> +		return;
> +	}
> +
> +	if (drm_drv_uses_atomic_modeset(fb_helper->dev))
> +		dpms_atomic(fb_helper, dpms_mode);
> +	else
> +		dpms_legacy(fb_helper, dpms_mode);
>  	mutex_unlock(&fb_helper->lock);
>  }
>  
> @@ -2467,7 +2532,6 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
>   */
>  int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>  {
> -	struct drm_device *dev = fb_helper->dev;
>  	int err = 0;
>  
>  	if (!drm_fbdev_emulation)


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/13] drm/fb-helper: Split dpms handling into legacy and atomic paths

[Daniel Vetter]

On Thu, Jun 29, 2017 at 12:22 PM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
>> +static void dpms_atomic(struct drm_fb_helper *fb_helper, int dpms_mode)
>> +{
>> +     struct drm_device *dev = fb_helper->dev;
>> +     struct drm_atomic_state *state;
>> +     int i, ret;
>> +
>> +     struct drm_modeset_acquire_ctx ctx;
>> +
>> +     drm_modeset_acquire_init(&ctx, 0);
>> +
>> +     state = drm_atomic_state_alloc(dev);
>> +     if (!state) {
>> +             ret = -ENOMEM;
>> +             goto out_ctx;
>> +     }
>> +
>> +     state->acquire_ctx = &ctx;
>> +retry:
>> +     for (i = 0; i < fb_helper->crtc_count; i++) {
>> +             struct drm_crtc_state *crtc_state;
>> +             struct drm_crtc *crtc;
>> +
>> +             if (!fb_helper->crtc_info[i].mode_set.mode)
>> +                     continue;
>> +
>> +             crtc = fb_helper->crtc_info[i].mode_set.crtc;
>> +
>> +             crtc_state = drm_atomic_get_crtc_state(state, crtc);
>> +             if (IS_ERR(crtc_state)) {
>> +                     ret = PTR_ERR(crtc_state);
>> +                     goto out_state;
>> +             }
> Hm, maybe remove the early continue, and change this to if (crtc_state->enable) crtc_state->active = ...; ?
>
> I don't know if it matters in practice, but it might be more resilient when crtc state does not match our expected state,
> similar to how DPMS on is ignored without CRTC.

I just blindly smashed the old fbdev code in with the helper and moved
the locking out. Not sure what would be better here, since the
continue is in a way just part of a non-existent
drm_fb_helper_for_each_active_crtc loop iterator. Not sure it's worth
it to overpolish this code to such an extent :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/13] drm/fb-helper: Split dpms handling into legacy and atomic paths

[Maarten Lankhorst]

Op 29-06-17 om 12:31 schreef Daniel Vetter:
> On Thu, Jun 29, 2017 at 12:22 PM, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>>> +static void dpms_atomic(struct drm_fb_helper *fb_helper, int dpms_mode)
>>> +{
>>> +     struct drm_device *dev = fb_helper->dev;
>>> +     struct drm_atomic_state *state;
>>> +     int i, ret;
>>> +
>>> +     struct drm_modeset_acquire_ctx ctx;
>>> +
>>> +     drm_modeset_acquire_init(&ctx, 0);
>>> +
>>> +     state = drm_atomic_state_alloc(dev);
>>> +     if (!state) {
>>> +             ret = -ENOMEM;
>>> +             goto out_ctx;
>>> +     }
>>> +
>>> +     state->acquire_ctx = &ctx;
>>> +retry:
>>> +     for (i = 0; i < fb_helper->crtc_count; i++) {
>>> +             struct drm_crtc_state *crtc_state;
>>> +             struct drm_crtc *crtc;
>>> +
>>> +             if (!fb_helper->crtc_info[i].mode_set.mode)
>>> +                     continue;
>>> +
>>> +             crtc = fb_helper->crtc_info[i].mode_set.crtc;
>>> +
>>> +             crtc_state = drm_atomic_get_crtc_state(state, crtc);
>>> +             if (IS_ERR(crtc_state)) {
>>> +                     ret = PTR_ERR(crtc_state);
>>> +                     goto out_state;
>>> +             }
>> Hm, maybe remove the early continue, and change this to if (crtc_state->enable) crtc_state->active = ...; ?
>>
>> I don't know if it matters in practice, but it might be more resilient when crtc state does not match our expected state,
>> similar to how DPMS on is ignored without CRTC.
> I just blindly smashed the old fbdev code in with the helper and moved
> the locking out. Not sure what would be better here, since the
> continue is in a way just part of a non-existent
> drm_fb_helper_for_each_active_crtc loop iterator. Not sure it's worth
> it to overpolish this code to such an extent :-)
> -Daniel

Well checking for crtc_state->enable instead of mode_set.mode probably means we're at least bug for bug
compatible vs legacy. It might be better to add a bool active to restore_fbdev_mode_atomic.

What about something like this? The less atomic commits the better. :)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index e49bae10f0ee..2c401fe8531c 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -378,7 +378,7 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_debug_leave);
 
-static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
+static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
 {
 	struct drm_device *dev = fb_helper->dev;
 	struct drm_plane *plane;
@@ -427,6 +427,17 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
 		ret = __drm_atomic_helper_set_config(mode_set, state);
 		if (ret != 0)
 			goto out_state;
+
+		/*
+		 * __drm_atomic_helper_set_config() sets active when a
+		 * mode is set, unconditionally clear it if we force DPMS off
+		 */
+		if (!active) {
+			struct drm_crtc *crtc = mode_set->crtc;
+			struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+			crtc_state->active = false;
+		}
 	}
 
 	ret = drm_atomic_commit(state);
@@ -497,7 +508,7 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 	struct drm_device *dev = fb_helper->dev;
 
 	if (drm_drv_uses_atomic_modeset(dev))
-		return restore_fbdev_mode_atomic(fb_helper);
+		return restore_fbdev_mode_atomic(fb_helper, true);
 	else
 		return restore_fbdev_mode_legacy(fb_helper);
 }
@@ -714,7 +669,7 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
 	}
 
 	if (drm_drv_uses_atomic_modeset(fb_helper->dev))
-		dpms_atomic(fb_helper, dpms_mode);
+		restore_fbdev_mode_atomic(fb_helper, dpms_mode == DRM_MODE_DPMS_ON);
 	else
 		dpms_legacy(fb_helper, dpms_mode);
 	mutex_unlock(&fb_helper->lock);

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

Re: [PATCH 09/13] drm/fb-helper: Split dpms handling into legacy and atomic paths

[Daniel Vetter]

On Thu, Jun 29, 2017 at 12:58 PM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Op 29-06-17 om 12:31 schreef Daniel Vetter:
>> On Thu, Jun 29, 2017 at 12:22 PM, Maarten Lankhorst
>> <maarten.lankhorst@linux.intel.com> wrote:
>>>> +static void dpms_atomic(struct drm_fb_helper *fb_helper, int dpms_mode)
>>>> +{
>>>> +     struct drm_device *dev = fb_helper->dev;
>>>> +     struct drm_atomic_state *state;
>>>> +     int i, ret;
>>>> +
>>>> +     struct drm_modeset_acquire_ctx ctx;
>>>> +
>>>> +     drm_modeset_acquire_init(&ctx, 0);
>>>> +
>>>> +     state = drm_atomic_state_alloc(dev);
>>>> +     if (!state) {
>>>> +             ret = -ENOMEM;
>>>> +             goto out_ctx;
>>>> +     }
>>>> +
>>>> +     state->acquire_ctx = &ctx;
>>>> +retry:
>>>> +     for (i = 0; i < fb_helper->crtc_count; i++) {
>>>> +             struct drm_crtc_state *crtc_state;
>>>> +             struct drm_crtc *crtc;
>>>> +
>>>> +             if (!fb_helper->crtc_info[i].mode_set.mode)
>>>> +                     continue;
>>>> +
>>>> +             crtc = fb_helper->crtc_info[i].mode_set.crtc;
>>>> +
>>>> +             crtc_state = drm_atomic_get_crtc_state(state, crtc);
>>>> +             if (IS_ERR(crtc_state)) {
>>>> +                     ret = PTR_ERR(crtc_state);
>>>> +                     goto out_state;
>>>> +             }
>>> Hm, maybe remove the early continue, and change this to if (crtc_state->enable) crtc_state->active = ...; ?
>>>
>>> I don't know if it matters in practice, but it might be more resilient when crtc state does not match our expected state,
>>> similar to how DPMS on is ignored without CRTC.
>> I just blindly smashed the old fbdev code in with the helper and moved
>> the locking out. Not sure what would be better here, since the
>> continue is in a way just part of a non-existent
>> drm_fb_helper_for_each_active_crtc loop iterator. Not sure it's worth
>> it to overpolish this code to such an extent :-)
>> -Daniel
>
> Well checking for crtc_state->enable instead of mode_set.mode probably means we're at least bug for bug
> compatible vs legacy. It might be better to add a bool active to restore_fbdev_mode_atomic.
>
> What about something like this? The less atomic commits the better. :)

Hm yeah, that sounds like a useful simplification. I don't think we
should do too much into extremes in fbdev and only have 1 atomic
commit, because fbdev fundamentally isn't atomic. But merging them
where it makes sense is reasonable.
-Daniel

>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index e49bae10f0ee..2c401fe8531c 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -378,7 +378,7 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
>  }
>  EXPORT_SYMBOL(drm_fb_helper_debug_leave);
>
> -static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
> +static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
>  {
>         struct drm_device *dev = fb_helper->dev;
>         struct drm_plane *plane;
> @@ -427,6 +427,17 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
>                 ret = __drm_atomic_helper_set_config(mode_set, state);
>                 if (ret != 0)
>                         goto out_state;
> +
> +               /*
> +                * __drm_atomic_helper_set_config() sets active when a
> +                * mode is set, unconditionally clear it if we force DPMS off
> +                */
> +               if (!active) {
> +                       struct drm_crtc *crtc = mode_set->crtc;
> +                       struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +
> +                       crtc_state->active = false;
> +               }
>         }
>
>         ret = drm_atomic_commit(state);
> @@ -497,7 +508,7 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>         struct drm_device *dev = fb_helper->dev;
>
>         if (drm_drv_uses_atomic_modeset(dev))
> -               return restore_fbdev_mode_atomic(fb_helper);
> +               return restore_fbdev_mode_atomic(fb_helper, true);
>         else
>                 return restore_fbdev_mode_legacy(fb_helper);
>  }
> @@ -714,7 +669,7 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
>         }
>
>         if (drm_drv_uses_atomic_modeset(fb_helper->dev))
> -               dpms_atomic(fb_helper, dpms_mode);
> +               restore_fbdev_mode_atomic(fb_helper, dpms_mode == DRM_MODE_DPMS_ON);
>         else
>                 dpms_legacy(fb_helper, dpms_mode);
>         mutex_unlock(&fb_helper->lock);
>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/13] drm/fb-helper: Split dpms handling into legacy and atomic paths

[Maarten Lankhorst]

Op 29-06-17 om 13:00 schreef Daniel Vetter:
> On Thu, Jun 29, 2017 at 12:58 PM, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>> Op 29-06-17 om 12:31 schreef Daniel Vetter:
>>> On Thu, Jun 29, 2017 at 12:22 PM, Maarten Lankhorst
>>> <maarten.lankhorst@linux.intel.com> wrote:
>>>>> +static void dpms_atomic(struct drm_fb_helper *fb_helper, int dpms_mode)
>>>>> +{
>>>>> +     struct drm_device *dev = fb_helper->dev;
>>>>> +     struct drm_atomic_state *state;
>>>>> +     int i, ret;
>>>>> +
>>>>> +     struct drm_modeset_acquire_ctx ctx;
>>>>> +
>>>>> +     drm_modeset_acquire_init(&ctx, 0);
>>>>> +
>>>>> +     state = drm_atomic_state_alloc(dev);
>>>>> +     if (!state) {
>>>>> +             ret = -ENOMEM;
>>>>> +             goto out_ctx;
>>>>> +     }
>>>>> +
>>>>> +     state->acquire_ctx = &ctx;
>>>>> +retry:
>>>>> +     for (i = 0; i < fb_helper->crtc_count; i++) {
>>>>> +             struct drm_crtc_state *crtc_state;
>>>>> +             struct drm_crtc *crtc;
>>>>> +
>>>>> +             if (!fb_helper->crtc_info[i].mode_set.mode)
>>>>> +                     continue;
>>>>> +
>>>>> +             crtc = fb_helper->crtc_info[i].mode_set.crtc;
>>>>> +
>>>>> +             crtc_state = drm_atomic_get_crtc_state(state, crtc);
>>>>> +             if (IS_ERR(crtc_state)) {
>>>>> +                     ret = PTR_ERR(crtc_state);
>>>>> +                     goto out_state;
>>>>> +             }
>>>> Hm, maybe remove the early continue, and change this to if (crtc_state->enable) crtc_state->active = ...; ?
>>>>
>>>> I don't know if it matters in practice, but it might be more resilient when crtc state does not match our expected state,
>>>> similar to how DPMS on is ignored without CRTC.
>>> I just blindly smashed the old fbdev code in with the helper and moved
>>> the locking out. Not sure what would be better here, since the
>>> continue is in a way just part of a non-existent
>>> drm_fb_helper_for_each_active_crtc loop iterator. Not sure it's worth
>>> it to overpolish this code to such an extent :-)
>>> -Daniel
>> Well checking for crtc_state->enable instead of mode_set.mode probably means we're at least bug for bug
>> compatible vs legacy. It might be better to add a bool active to restore_fbdev_mode_atomic.
>>
>> What about something like this? The less atomic commits the better. :)
> Hm yeah, that sounds like a useful simplification. I don't think we
> should do too much into extremes in fbdev and only have 1 atomic
> commit, because fbdev fundamentally isn't atomic. But merging them
> where it makes sense is reasonable.
> -Daniel

Agreed, perhaps with pan_display_atomic too, if we no longer use modeset locks,
then it becomes very simple..
What about getting rid of that one too, in favor of this?

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 0cd2035ac1d1..b4356fdf5e6d 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1557,70 +1557,36 @@ int drm_fb_helper_set_par(struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_set_par);
 
-static int pan_display_atomic(struct fb_var_screeninfo *var,
-			      struct fb_info *info)
+static void pan_set(struct drm_fb_helper *fb_helper, int x, int y)
 {
-	struct drm_fb_helper *fb_helper = info->par;
-	struct drm_device *dev = fb_helper->dev;
-	struct drm_atomic_state *state;
-	struct drm_plane *plane;
-	int i, ret;
-	unsigned int plane_mask;
-	struct drm_modeset_acquire_ctx ctx;
-
-	drm_modeset_acquire_init(&ctx, 0);
-
-	state = drm_atomic_state_alloc(dev);
-	if (!state) {
-		ret = -ENOMEM;
-		goto out_ctx;
-	}
+	int i;
 
-	state->acquire_ctx = &ctx;
-retry:
-	plane_mask = 0;
 	for (i = 0; i < fb_helper->crtc_count; i++) {
 		struct drm_mode_set *mode_set;
 
 		mode_set = &fb_helper->crtc_info[i].mode_set;
 
-		mode_set->x = var->xoffset;
-		mode_set->y = var->yoffset;
-
-		ret = __drm_atomic_helper_set_config(mode_set, state);
-		if (ret != 0)
-			goto out_state;
-
-		plane = mode_set->crtc->primary;
-		plane_mask |= (1 << drm_plane_index(plane));
-		plane->old_fb = plane->fb;
+		mode_set->x = x;
+		mode_set->y = y;
 	}
+}
 
-	ret = drm_atomic_commit(state);
-	if (ret != 0)
-		goto out_state;
-
-	info->var.xoffset = var->xoffset;
-	info->var.yoffset = var->yoffset;
+static int pan_display_atomic(struct fb_var_screeninfo *var,
+			      struct fb_info *info)
+{
+	struct drm_fb_helper *fb_helper = info->par;
+	int ret;
 
-out_state:
-	drm_atomic_clean_old_fb(dev, plane_mask, ret);
+	pan_set(fb_helper, var->xoffset, var->yoffset);
 
-	if (ret == -EDEADLK)
-		goto backoff;
-
-	drm_atomic_state_put(state);
-out_ctx:
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
+	ret = restore_fbdev_mode(fb_helper);
+	if (!ret) {
+		info->var.xoffset = var->xoffset;
+		info->var.yoffset = var->yoffset;
+	} else
+		pan_set(fb_helper, info->var.xoffset, info->var.yoffset);
 
 	return ret;
-
-backoff:
-	drm_atomic_state_clear(state);
-	drm_modeset_backoff(&ctx);
-
-	goto retry;
 }
 
 static int pan_display_legacy(struct fb_var_screeninfo *var,


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/13] drm/fb-helper: Support deferred setup

[Daniel Vetter]

FB helper code falls back to a 1024x768 mode if no outputs are connected
or don't report back any modes upon initialization. This can be annoying
because outputs that are added to FB helper later on can't be used with
FB helper if they don't support a matching mode.

The fallback is in place because VGA connectors can happen to report an
unknown connection status even when they are in fact connected.

Some drivers have custom solutions in place to defer FB helper setup
until at least one output is connected. But the logic behind these
solutions is always the same and there is nothing driver-specific about
it, so a better alterative is to fix the FB helper core and add support
for all drivers automatically.

This patch adds support for deferred FB helper setup. It checks all the
connectors for their connection status, and if all of them report to be
disconnected marks the FB helper as needing deferred setup. Whet setup
is deferred, the FB helper core will automatically retry setup after a
hotplug event, and it will keep trying until it succeeds.

v2: Rebase onto my entirely reworked fbdev helper locking. One big
difference is that this version again drops&reacquires the fbdev lock
(which is now fb_helper->lock, but before this patch series it was
mode_config->mutex), because register_framebuffer must be able to
recurse back into fbdev helper code for the initial screen setup.

v3: __drm_fb_helper_initial_config must hold fb_helper->lock upon
return, I've fumbled that in the deferred setup case (Liviu).

v4: I was blind, redo this all. __drm_fb_helper_initial_config
shouldn't need to reacquire fb_helper->lock, that just confuses
callers. I myself got confused by kernel_fb_helper_lock and somehow
thought it's the same as fb_helper->lock. Tsk.

Also simplify the logic a bit (we don't need two functions to probe
connectors), we can stick much closer to the existing code. And update
some comments I've spotted that are outdated.

Cc: Liviu Dudau <liviu@dudau.co.uk>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Thierry Reding <treding@nvidia.com> (v1)
Tested-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c | 100 ++++++++++++++++++++++++++--------------
 include/drm/drm_fb_helper.h     |  23 +++++++++
 2 files changed, 88 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index bbd4c6d0378d..d833eb2320d1 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -521,6 +521,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 	if (!drm_fbdev_emulation)
 		return -ENODEV;
 
+	if (READ_ONCE(fb_helper->deferred_setup))
+		return 0;
+
 	mutex_lock(&fb_helper->lock);
 	ret = restore_fbdev_mode(fb_helper);
 
@@ -1695,8 +1698,7 @@ EXPORT_SYMBOL(drm_fb_helper_pan_display);
 
 /*
  * Allocates the backing storage and sets up the fbdev info structure through
- * the ->fb_probe callback and then registers the fbdev and sets up the panic
- * notifier.
+ * the ->fb_probe callback.
  */
 static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 					 int preferred_bpp)
@@ -1794,13 +1796,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	}
 
 	if (crtc_count == 0 || sizes.fb_width == -1 || sizes.fb_height == -1) {
-		/*
-		 * hmm everyone went away - assume VGA cable just fell out
-		 * and will come back later.
-		 */
-		DRM_INFO("Cannot find any crtc or sizes - going 1024x768\n");
-		sizes.fb_width = sizes.surface_width = 1024;
-		sizes.fb_height = sizes.surface_height = 768;
+		DRM_INFO("Cannot find any crtc or sizes\n");
+		return -EAGAIN;
 	}
 
 	/* Handle our overallocation */
@@ -2429,6 +2426,57 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
 	kfree(enabled);
 }
 
+/* Note: Drops fb_helper->lock before returning. */
+static int __drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper,
+					  int bpp_sel)
+{
+	struct drm_device *dev = fb_helper->dev;
+	struct fb_info *info;
+	unsigned int width, height;
+	int ret;
+
+	width = dev->mode_config.max_width;
+	height = dev->mode_config.max_height;
+
+	drm_setup_crtcs(fb_helper, width, height);
+	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
+	if (ret < 0) {
+		if (ret == -EAGAIN) {
+			fb_helper->preferred_bpp = bpp_sel;
+			fb_helper->deferred_setup = true;
+		}
+		mutex_unlock(&fb_helper->lock);
+
+		return ret;
+	}
+
+	fb_helper->deferred_setup = false;
+
+	info = fb_helper->fbdev;
+	info->var.pixclock = 0;
+
+	/* Need to drop locks to avoid recursive deadlock in
+	 * register_framebuffer. This is ok because the only thing left to do is
+	 * register the fbdev emulation instance in kernel_fb_helper_list. */
+	mutex_unlock(&fb_helper->lock);
+
+	ret = register_framebuffer(info);
+	if (ret < 0)
+		return ret;
+
+	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
+		 info->node, info->fix.id);
+
+	mutex_lock(&kernel_fb_helper_lock);
+	if (list_empty(&kernel_fb_helper_list))
+		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
+
+	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
+	mutex_unlock(&kernel_fb_helper_lock);
+
+	return 0;
+}
+
 /**
  * drm_fb_helper_initial_config - setup a sane initial connector configuration
  * @fb_helper: fb_helper device struct
@@ -2473,39 +2521,15 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
  */
 int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 {
-	struct drm_device *dev = fb_helper->dev;
-	struct fb_info *info;
 	int ret;
 
 	if (!drm_fbdev_emulation)
 		return 0;
 
 	mutex_lock(&fb_helper->lock);
-	drm_setup_crtcs(fb_helper,
-			dev->mode_config.max_width,
-			dev->mode_config.max_height);
-	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
-	mutex_unlock(&fb_helper->lock);
-	if (ret)
-		return ret;
+	ret = __drm_fb_helper_initial_config(fb_helper, bpp_sel);
 
-	info = fb_helper->fbdev;
-	info->var.pixclock = 0;
-	ret = register_framebuffer(info);
-	if (ret < 0)
-		return ret;
-
-	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
-		 info->node, info->fix.id);
-
-	mutex_lock(&kernel_fb_helper_lock);
-	if (list_empty(&kernel_fb_helper_list))
-		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
-
-	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
-	mutex_unlock(&kernel_fb_helper_lock);
-
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL(drm_fb_helper_initial_config);
 
@@ -2538,6 +2562,12 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 		return 0;
 
 	mutex_lock(&fb_helper->lock);
+	if (fb_helper->deferred_setup) {
+		err = __drm_fb_helper_initial_config(fb_helper,
+						     fb_helper->preferred_bpp);
+		return err;
+	}
+
 	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
 		fb_helper->delayed_hotplug = true;
 		mutex_unlock(&fb_helper->lock);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index ea170b96e88d..a5ea6ffdfecc 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -232,6 +232,29 @@ struct drm_fb_helper {
 	 * needs to be reprobe when fbdev is in control again.
 	 */
 	bool delayed_hotplug;
+
+	/**
+	 * @deferred_setup:
+	 *
+	 * If no outputs are connected (disconnected or unknown) the FB helper
+	 * code will defer setup until at least one of the outputs shows up.
+	 * This field keeps track of the status so that setup can be retried
+	 * at every hotplug event until it succeeds eventually.
+	 *
+	 * Protected by @lock.
+	 */
+	bool deferred_setup;
+
+	/**
+	 * @preferred_bpp:
+	 *
+	 * Temporary storage for the driver's preferred BPP setting passed to
+	 * FB helper initialization. This needs to be tracked so that deferred
+	 * FB helper setup can pass this on.
+	 *
+	 * See also: @deferred_setup
+	 */
+	int preferred_bpp;
 };
 
 /**
-- 
2.11.0

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

[PATCH] drm/fb-helper: Support deferred setup

[Daniel Vetter]

FB helper code falls back to a 1024x768 mode if no outputs are connected
or don't report back any modes upon initialization. This can be annoying
because outputs that are added to FB helper later on can't be used with
FB helper if they don't support a matching mode.

The fallback is in place because VGA connectors can happen to report an
unknown connection status even when they are in fact connected.

Some drivers have custom solutions in place to defer FB helper setup
until at least one output is connected. But the logic behind these
solutions is always the same and there is nothing driver-specific about
it, so a better alterative is to fix the FB helper core and add support
for all drivers automatically.

This patch adds support for deferred FB helper setup. It checks all the
connectors for their connection status, and if all of them report to be
disconnected marks the FB helper as needing deferred setup. Whet setup
is deferred, the FB helper core will automatically retry setup after a
hotplug event, and it will keep trying until it succeeds.

v2: Rebase onto my entirely reworked fbdev helper locking. One big
difference is that this version again drops&reacquires the fbdev lock
(which is now fb_helper->lock, but before this patch series it was
mode_config->mutex), because register_framebuffer must be able to
recurse back into fbdev helper code for the initial screen setup.

v3: __drm_fb_helper_initial_config must hold fb_helper->lock upon
return, I've fumbled that in the deferred setup case (Liviu).

v4: I was blind, redo this all. __drm_fb_helper_initial_config
shouldn't need to reacquire fb_helper->lock, that just confuses
callers. I myself got confused by kernel_fb_helper_lock and somehow
thought it's the same as fb_helper->lock. Tsk.

Also simplify the logic a bit (we don't need two functions to probe
connectors), we can stick much closer to the existing code. And update
some comments I've spotted that are outdated.

v5: Don't pass -EAGAIN to drivers, it's just an internal error code
(Liviu).

Cc: Liviu Dudau <liviu@dudau.co.uk>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Thierry Reding <treding@nvidia.com> (v1)
Tested-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c | 101 ++++++++++++++++++++++++++--------------
 include/drm/drm_fb_helper.h     |  23 +++++++++
 2 files changed, 89 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index bbd4c6d0378d..e49bae10f0ee 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -521,6 +521,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 	if (!drm_fbdev_emulation)
 		return -ENODEV;
 
+	if (READ_ONCE(fb_helper->deferred_setup))
+		return 0;
+
 	mutex_lock(&fb_helper->lock);
 	ret = restore_fbdev_mode(fb_helper);
 
@@ -1695,8 +1698,7 @@ EXPORT_SYMBOL(drm_fb_helper_pan_display);
 
 /*
  * Allocates the backing storage and sets up the fbdev info structure through
- * the ->fb_probe callback and then registers the fbdev and sets up the panic
- * notifier.
+ * the ->fb_probe callback.
  */
 static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 					 int preferred_bpp)
@@ -1794,13 +1796,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	}
 
 	if (crtc_count == 0 || sizes.fb_width == -1 || sizes.fb_height == -1) {
-		/*
-		 * hmm everyone went away - assume VGA cable just fell out
-		 * and will come back later.
-		 */
-		DRM_INFO("Cannot find any crtc or sizes - going 1024x768\n");
-		sizes.fb_width = sizes.surface_width = 1024;
-		sizes.fb_height = sizes.surface_height = 768;
+		DRM_INFO("Cannot find any crtc or sizes\n");
+		return -EAGAIN;
 	}
 
 	/* Handle our overallocation */
@@ -2429,6 +2426,58 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
 	kfree(enabled);
 }
 
+/* Note: Drops fb_helper->lock before returning. */
+static int __drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper,
+					  int bpp_sel)
+{
+	struct drm_device *dev = fb_helper->dev;
+	struct fb_info *info;
+	unsigned int width, height;
+	int ret;
+
+	width = dev->mode_config.max_width;
+	height = dev->mode_config.max_height;
+
+	drm_setup_crtcs(fb_helper, width, height);
+	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
+	if (ret < 0) {
+		if (ret == -EAGAIN) {
+			fb_helper->preferred_bpp = bpp_sel;
+			fb_helper->deferred_setup = true;
+			ret = 0;
+		}
+		mutex_unlock(&fb_helper->lock);
+
+		return ret;
+	}
+
+	fb_helper->deferred_setup = false;
+
+	info = fb_helper->fbdev;
+	info->var.pixclock = 0;
+
+	/* Need to drop locks to avoid recursive deadlock in
+	 * register_framebuffer. This is ok because the only thing left to do is
+	 * register the fbdev emulation instance in kernel_fb_helper_list. */
+	mutex_unlock(&fb_helper->lock);
+
+	ret = register_framebuffer(info);
+	if (ret < 0)
+		return ret;
+
+	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
+		 info->node, info->fix.id);
+
+	mutex_lock(&kernel_fb_helper_lock);
+	if (list_empty(&kernel_fb_helper_list))
+		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
+
+	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
+	mutex_unlock(&kernel_fb_helper_lock);
+
+	return 0;
+}
+
 /**
  * drm_fb_helper_initial_config - setup a sane initial connector configuration
  * @fb_helper: fb_helper device struct
@@ -2473,39 +2522,15 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
  */
 int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
 {
-	struct drm_device *dev = fb_helper->dev;
-	struct fb_info *info;
 	int ret;
 
 	if (!drm_fbdev_emulation)
 		return 0;
 
 	mutex_lock(&fb_helper->lock);
-	drm_setup_crtcs(fb_helper,
-			dev->mode_config.max_width,
-			dev->mode_config.max_height);
-	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
-	mutex_unlock(&fb_helper->lock);
-	if (ret)
-		return ret;
+	ret = __drm_fb_helper_initial_config(fb_helper, bpp_sel);
 
-	info = fb_helper->fbdev;
-	info->var.pixclock = 0;
-	ret = register_framebuffer(info);
-	if (ret < 0)
-		return ret;
-
-	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
-		 info->node, info->fix.id);
-
-	mutex_lock(&kernel_fb_helper_lock);
-	if (list_empty(&kernel_fb_helper_list))
-		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
-
-	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
-	mutex_unlock(&kernel_fb_helper_lock);
-
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL(drm_fb_helper_initial_config);
 
@@ -2538,6 +2563,12 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 		return 0;
 
 	mutex_lock(&fb_helper->lock);
+	if (fb_helper->deferred_setup) {
+		err = __drm_fb_helper_initial_config(fb_helper,
+						     fb_helper->preferred_bpp);
+		return err;
+	}
+
 	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
 		fb_helper->delayed_hotplug = true;
 		mutex_unlock(&fb_helper->lock);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index ea170b96e88d..a5ea6ffdfecc 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -232,6 +232,29 @@ struct drm_fb_helper {
 	 * needs to be reprobe when fbdev is in control again.
 	 */
 	bool delayed_hotplug;
+
+	/**
+	 * @deferred_setup:
+	 *
+	 * If no outputs are connected (disconnected or unknown) the FB helper
+	 * code will defer setup until at least one of the outputs shows up.
+	 * This field keeps track of the status so that setup can be retried
+	 * at every hotplug event until it succeeds eventually.
+	 *
+	 * Protected by @lock.
+	 */
+	bool deferred_setup;
+
+	/**
+	 * @preferred_bpp:
+	 *
+	 * Temporary storage for the driver's preferred BPP setting passed to
+	 * FB helper initialization. This needs to be tracked so that deferred
+	 * FB helper setup can pass this on.
+	 *
+	 * See also: @deferred_setup
+	 */
+	int preferred_bpp;
 };
 
 /**
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/fb-helper: Support deferred setup

[Liviu Dudau]

On Wed, Jun 28, 2017 at 01:32:01PM +0200, Daniel Vetter wrote:
> FB helper code falls back to a 1024x768 mode if no outputs are connected
> or don't report back any modes upon initialization. This can be annoying
> because outputs that are added to FB helper later on can't be used with
> FB helper if they don't support a matching mode.
> 
> The fallback is in place because VGA connectors can happen to report an
> unknown connection status even when they are in fact connected.
> 
> Some drivers have custom solutions in place to defer FB helper setup
> until at least one output is connected. But the logic behind these
> solutions is always the same and there is nothing driver-specific about
> it, so a better alterative is to fix the FB helper core and add support
> for all drivers automatically.
> 
> This patch adds support for deferred FB helper setup. It checks all the
> connectors for their connection status, and if all of them report to be
> disconnected marks the FB helper as needing deferred setup. Whet setup
> is deferred, the FB helper core will automatically retry setup after a
> hotplug event, and it will keep trying until it succeeds.
> 
> v2: Rebase onto my entirely reworked fbdev helper locking. One big
> difference is that this version again drops&reacquires the fbdev lock
> (which is now fb_helper->lock, but before this patch series it was
> mode_config->mutex), because register_framebuffer must be able to
> recurse back into fbdev helper code for the initial screen setup.
> 
> v3: __drm_fb_helper_initial_config must hold fb_helper->lock upon
> return, I've fumbled that in the deferred setup case (Liviu).
> 
> v4: I was blind, redo this all. __drm_fb_helper_initial_config
> shouldn't need to reacquire fb_helper->lock, that just confuses
> callers. I myself got confused by kernel_fb_helper_lock and somehow
> thought it's the same as fb_helper->lock. Tsk.
> 
> Also simplify the logic a bit (we don't need two functions to probe
> connectors), we can stick much closer to the existing code. And update
> some comments I've spotted that are outdated.
> 
> v5: Don't pass -EAGAIN to drivers, it's just an internal error code
> (Liviu).
> 
> Cc: Liviu Dudau <liviu@dudau.co.uk>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Thierry Reding <treding@nvidia.com> (v1)
> Tested-by: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Tested-by: Liviu Dudau <liviu@dudau.co.uk>

I'm going through the debugging on the deferred setup on first monitor connect,
as that seems to not do a full modeset, but that might turn out a bug in my
driver.

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 101 ++++++++++++++++++++++++++--------------
>  include/drm/drm_fb_helper.h     |  23 +++++++++
>  2 files changed, 89 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index bbd4c6d0378d..e49bae10f0ee 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -521,6 +521,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>  	if (!drm_fbdev_emulation)
>  		return -ENODEV;
>  
> +	if (READ_ONCE(fb_helper->deferred_setup))
> +		return 0;
> +
>  	mutex_lock(&fb_helper->lock);
>  	ret = restore_fbdev_mode(fb_helper);
>  
> @@ -1695,8 +1698,7 @@ EXPORT_SYMBOL(drm_fb_helper_pan_display);
>  
>  /*
>   * Allocates the backing storage and sets up the fbdev info structure through
> - * the ->fb_probe callback and then registers the fbdev and sets up the panic
> - * notifier.
> + * the ->fb_probe callback.
>   */
>  static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  					 int preferred_bpp)
> @@ -1794,13 +1796,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  	}
>  
>  	if (crtc_count == 0 || sizes.fb_width == -1 || sizes.fb_height == -1) {
> -		/*
> -		 * hmm everyone went away - assume VGA cable just fell out
> -		 * and will come back later.
> -		 */
> -		DRM_INFO("Cannot find any crtc or sizes - going 1024x768\n");
> -		sizes.fb_width = sizes.surface_width = 1024;
> -		sizes.fb_height = sizes.surface_height = 768;
> +		DRM_INFO("Cannot find any crtc or sizes\n");
> +		return -EAGAIN;
>  	}
>  
>  	/* Handle our overallocation */
> @@ -2429,6 +2426,58 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  	kfree(enabled);
>  }
>  
> +/* Note: Drops fb_helper->lock before returning. */
> +static int __drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper,
> +					  int bpp_sel)
> +{
> +	struct drm_device *dev = fb_helper->dev;
> +	struct fb_info *info;
> +	unsigned int width, height;
> +	int ret;
> +
> +	width = dev->mode_config.max_width;
> +	height = dev->mode_config.max_height;
> +
> +	drm_setup_crtcs(fb_helper, width, height);
> +	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> +	if (ret < 0) {
> +		if (ret == -EAGAIN) {
> +			fb_helper->preferred_bpp = bpp_sel;
> +			fb_helper->deferred_setup = true;
> +			ret = 0;
> +		}
> +		mutex_unlock(&fb_helper->lock);
> +
> +		return ret;
> +	}
> +
> +	fb_helper->deferred_setup = false;
> +
> +	info = fb_helper->fbdev;
> +	info->var.pixclock = 0;
> +
> +	/* Need to drop locks to avoid recursive deadlock in
> +	 * register_framebuffer. This is ok because the only thing left to do is
> +	 * register the fbdev emulation instance in kernel_fb_helper_list. */
> +	mutex_unlock(&fb_helper->lock);
> +
> +	ret = register_framebuffer(info);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
> +		 info->node, info->fix.id);
> +
> +	mutex_lock(&kernel_fb_helper_lock);
> +	if (list_empty(&kernel_fb_helper_list))
> +		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
> +
> +	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> +	mutex_unlock(&kernel_fb_helper_lock);
> +
> +	return 0;
> +}
> +
>  /**
>   * drm_fb_helper_initial_config - setup a sane initial connector configuration
>   * @fb_helper: fb_helper device struct
> @@ -2473,39 +2522,15 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>   */
>  int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
>  {
> -	struct drm_device *dev = fb_helper->dev;
> -	struct fb_info *info;
>  	int ret;
>  
>  	if (!drm_fbdev_emulation)
>  		return 0;
>  
>  	mutex_lock(&fb_helper->lock);
> -	drm_setup_crtcs(fb_helper,
> -			dev->mode_config.max_width,
> -			dev->mode_config.max_height);
> -	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> -	mutex_unlock(&fb_helper->lock);
> -	if (ret)
> -		return ret;
> +	ret = __drm_fb_helper_initial_config(fb_helper, bpp_sel);
>  
> -	info = fb_helper->fbdev;
> -	info->var.pixclock = 0;
> -	ret = register_framebuffer(info);
> -	if (ret < 0)
> -		return ret;
> -
> -	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
> -		 info->node, info->fix.id);
> -
> -	mutex_lock(&kernel_fb_helper_lock);
> -	if (list_empty(&kernel_fb_helper_list))
> -		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
> -
> -	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> -	mutex_unlock(&kernel_fb_helper_lock);
> -
> -	return 0;
> +	return ret;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_initial_config);
>  
> @@ -2538,6 +2563,12 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>  		return 0;
>  
>  	mutex_lock(&fb_helper->lock);
> +	if (fb_helper->deferred_setup) {
> +		err = __drm_fb_helper_initial_config(fb_helper,
> +						     fb_helper->preferred_bpp);
> +		return err;
> +	}
> +

Minor nitpick: specially in drm_fb_helper_initial_config() you can get rid of the ret variable
and use 'return __drm_fb_helper_initial_config()'. Also here, can skip storing the result of
__drm_fb_helper_initial_config() in err.

Best regards,
Liviu


>  	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
>  		fb_helper->delayed_hotplug = true;
>  		mutex_unlock(&fb_helper->lock);
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index ea170b96e88d..a5ea6ffdfecc 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -232,6 +232,29 @@ struct drm_fb_helper {
>  	 * needs to be reprobe when fbdev is in control again.
>  	 */
>  	bool delayed_hotplug;
> +
> +	/**
> +	 * @deferred_setup:
> +	 *
> +	 * If no outputs are connected (disconnected or unknown) the FB helper
> +	 * code will defer setup until at least one of the outputs shows up.
> +	 * This field keeps track of the status so that setup can be retried
> +	 * at every hotplug event until it succeeds eventually.
> +	 *
> +	 * Protected by @lock.
> +	 */
> +	bool deferred_setup;
> +
> +	/**
> +	 * @preferred_bpp:
> +	 *
> +	 * Temporary storage for the driver's preferred BPP setting passed to
> +	 * FB helper initialization. This needs to be tracked so that deferred
> +	 * FB helper setup can pass this on.
> +	 *
> +	 * See also: @deferred_setup
> +	 */
> +	int preferred_bpp;
>  };
>  
>  /**
> -- 
> 2.11.0
> 

-- 
   _
 _|_|_
 ('_')
 (⊃  )⊃
 |_|_|
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/fb-helper: Support deferred setup

[Maarten Lankhorst]

Op 28-06-17 om 13:32 schreef Daniel Vetter:
> FB helper code falls back to a 1024x768 mode if no outputs are connected
> or don't report back any modes upon initialization. This can be annoying
> because outputs that are added to FB helper later on can't be used with
> FB helper if they don't support a matching mode.
>
> The fallback is in place because VGA connectors can happen to report an
> unknown connection status even when they are in fact connected.
>
> Some drivers have custom solutions in place to defer FB helper setup
> until at least one output is connected. But the logic behind these
> solutions is always the same and there is nothing driver-specific about
> it, so a better alterative is to fix the FB helper core and add support
> for all drivers automatically.
>
> This patch adds support for deferred FB helper setup. It checks all the
> connectors for their connection status, and if all of them report to be
> disconnected marks the FB helper as needing deferred setup. Whet setup
> is deferred, the FB helper core will automatically retry setup after a
> hotplug event, and it will keep trying until it succeeds.
>
> v2: Rebase onto my entirely reworked fbdev helper locking. One big
> difference is that this version again drops&reacquires the fbdev lock
> (which is now fb_helper->lock, but before this patch series it was
> mode_config->mutex), because register_framebuffer must be able to
> recurse back into fbdev helper code for the initial screen setup.
>
> v3: __drm_fb_helper_initial_config must hold fb_helper->lock upon
> return, I've fumbled that in the deferred setup case (Liviu).
>
> v4: I was blind, redo this all. __drm_fb_helper_initial_config
> shouldn't need to reacquire fb_helper->lock, that just confuses
> callers. I myself got confused by kernel_fb_helper_lock and somehow
> thought it's the same as fb_helper->lock. Tsk.
>
> Also simplify the logic a bit (we don't need two functions to probe
> connectors), we can stick much closer to the existing code. And update
> some comments I've spotted that are outdated.
>
> v5: Don't pass -EAGAIN to drivers, it's just an internal error code
> (Liviu).
>
> Cc: Liviu Dudau <liviu@dudau.co.uk>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Thierry Reding <treding@nvidia.com> (v1)
> Tested-by: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 101 ++++++++++++++++++++++++++--------------
>  include/drm/drm_fb_helper.h     |  23 +++++++++
>  2 files changed, 89 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index bbd4c6d0378d..e49bae10f0ee 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -521,6 +521,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>  	if (!drm_fbdev_emulation)
>  		return -ENODEV;
>  
> +	if (READ_ONCE(fb_helper->deferred_setup))
> +		return 0;
> +
>  	mutex_lock(&fb_helper->lock);
>  	ret = restore_fbdev_mode(fb_helper);
>  
> @@ -1695,8 +1698,7 @@ EXPORT_SYMBOL(drm_fb_helper_pan_display);
>  
>  /*
>   * Allocates the backing storage and sets up the fbdev info structure through
> - * the ->fb_probe callback and then registers the fbdev and sets up the panic
> - * notifier.
> + * the ->fb_probe callback.
>   */
>  static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  					 int preferred_bpp)
> @@ -1794,13 +1796,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  	}
>  
>  	if (crtc_count == 0 || sizes.fb_width == -1 || sizes.fb_height == -1) {
> -		/*
> -		 * hmm everyone went away - assume VGA cable just fell out
> -		 * and will come back later.
> -		 */
> -		DRM_INFO("Cannot find any crtc or sizes - going 1024x768\n");
> -		sizes.fb_width = sizes.surface_width = 1024;
> -		sizes.fb_height = sizes.surface_height = 768;
> +		DRM_INFO("Cannot find any crtc or sizes\n");
> +		return -EAGAIN;
>  	}
>  
>  	/* Handle our overallocation */
> @@ -2429,6 +2426,58 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  	kfree(enabled);
>  }
>  
> +/* Note: Drops fb_helper->lock before returning. */
> +static int __drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper,
> +					  int bpp_sel)
> +{
> +	struct drm_device *dev = fb_helper->dev;
> +	struct fb_info *info;
> +	unsigned int width, height;
> +	int ret;
> +
> +	width = dev->mode_config.max_width;
> +	height = dev->mode_config.max_height;
> +
> +	drm_setup_crtcs(fb_helper, width, height);
> +	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> +	if (ret < 0) {
> +		if (ret == -EAGAIN) {
> +			fb_helper->preferred_bpp = bpp_sel;
> +			fb_helper->deferred_setup = true;
> +			ret = 0;
> +		}
> +		mutex_unlock(&fb_helper->lock);
> +
> +		return ret;
> +	}
> +
> +	fb_helper->deferred_setup = false;
> +
> +	info = fb_helper->fbdev;
> +	info->var.pixclock = 0;
> +
> +	/* Need to drop locks to avoid recursive deadlock in
> +	 * register_framebuffer. This is ok because the only thing left to do is
> +	 * register the fbdev emulation instance in kernel_fb_helper_list. */
> +	mutex_unlock(&fb_helper->lock);
> +
> +	ret = register_framebuffer(info);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
> +		 info->node, info->fix.id);
> +
> +	mutex_lock(&kernel_fb_helper_lock);
> +	if (list_empty(&kernel_fb_helper_list))
> +		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
> +
> +	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> +	mutex_unlock(&kernel_fb_helper_lock);
> +
> +	return 0;
> +}
> +
>  /**
>   * drm_fb_helper_initial_config - setup a sane initial connector configuration
>   * @fb_helper: fb_helper device struct
> @@ -2473,39 +2522,15 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>   */
>  int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel)
>  {
> -	struct drm_device *dev = fb_helper->dev;
> -	struct fb_info *info;
>  	int ret;
>  
>  	if (!drm_fbdev_emulation)
>  		return 0;
>  
>  	mutex_lock(&fb_helper->lock);
> -	drm_setup_crtcs(fb_helper,
> -			dev->mode_config.max_width,
> -			dev->mode_config.max_height);
> -	ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> -	mutex_unlock(&fb_helper->lock);
> -	if (ret)
> -		return ret;
> +	ret = __drm_fb_helper_initial_config(fb_helper, bpp_sel);
>  
> -	info = fb_helper->fbdev;
> -	info->var.pixclock = 0;
> -	ret = register_framebuffer(info);
> -	if (ret < 0)
> -		return ret;
> -
> -	dev_info(dev->dev, "fb%d: %s frame buffer device\n",
> -		 info->node, info->fix.id);
> -
> -	mutex_lock(&kernel_fb_helper_lock);
> -	if (list_empty(&kernel_fb_helper_list))
> -		register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
> -
> -	list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
> -	mutex_unlock(&kernel_fb_helper_lock);
> -
> -	return 0;
> +	return ret;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_initial_config);
>  
> @@ -2538,6 +2563,12 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>  		return 0;
>  
>  	mutex_lock(&fb_helper->lock);
> +	if (fb_helper->deferred_setup) {
> +		err = __drm_fb_helper_initial_config(fb_helper,
> +						     fb_helper->preferred_bpp);
Maybe rename this function to *_and_unlock? I had to read this code twice to make sure it works as intended. :)
> +		return err;
> +	}
> +
>  	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
>  		fb_helper->delayed_hotplug = true;
>  		mutex_unlock(&fb_helper->lock);
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index ea170b96e88d..a5ea6ffdfecc 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -232,6 +232,29 @@ struct drm_fb_helper {
>  	 * needs to be reprobe when fbdev is in control again.
>  	 */
>  	bool delayed_hotplug;
> +
> +	/**
> +	 * @deferred_setup:
> +	 *
> +	 * If no outputs are connected (disconnected or unknown) the FB helper
> +	 * code will defer setup until at least one of the outputs shows up.
> +	 * This field keeps track of the status so that setup can be retried
> +	 * at every hotplug event until it succeeds eventually.
> +	 *
> +	 * Protected by @lock.
> +	 */
> +	bool deferred_setup;
> +
> +	/**
> +	 * @preferred_bpp:
> +	 *
> +	 * Temporary storage for the driver's preferred BPP setting passed to
> +	 * FB helper initialization. This needs to be tracked so that deferred
> +	 * FB helper setup can pass this on.
> +	 *
> +	 * See also: @deferred_setup
> +	 */
> +	int preferred_bpp;
>  };
>  
>  /**


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/fb-helper: Support deferred setup

[Daniel Vetter]

On Thu, Jun 29, 2017 at 12:59 PM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
>> @@ -2538,6 +2563,12 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>>               return 0;
>>
>>       mutex_lock(&fb_helper->lock);
>> +     if (fb_helper->deferred_setup) {
>> +             err = __drm_fb_helper_initial_config(fb_helper,
>> +                                                  fb_helper->preferred_bpp);
> Maybe rename this function to *_and_unlock? I had to read this code twice to make sure it works as intended. :)

If this is the official suffix for trickery like this I can do - I
already asked on irc what it should be called :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/fb-helper: Restore first connection behaviour on deferred setup

[Liviu Dudau]

Prior to commit b0aa06e9a7fd ("drm/fb-helper: Support deferred setup"),
if no output is connected at framebuffer setup time, we get a default
1024x768 mode that is going to be used when we first connect a monitor.
After the commit, on first connection after deferred setup, we probe
the monitor and get the preferred resolution, but no mode get set
because the drm_fb_helper_hotplug_event() function returns early
when the setup has been deferred. That is different from what happens
on a second re-connect of the monitor, when the native mode get set.

Create a more consistent behaviour by checking in the
drm_fb_helper_hotplug_event() function if the deferred setup is still
active. If not, that means we now have a valid framebuffer that can be
used for setting the correct mode.

Fixes: b0aa06e9a7fd ("drm/fb-helper: Support deferred setup")
Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d833eb2320d1..bb7b44d284ec 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2444,6 +2444,7 @@ static int __drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper,
 		if (ret == -EAGAIN) {
 			fb_helper->preferred_bpp = bpp_sel;
 			fb_helper->deferred_setup = true;
+			ret = 0;
 		}
 		mutex_unlock(&fb_helper->lock);
 
@@ -2565,7 +2566,13 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 	if (fb_helper->deferred_setup) {
 		err = __drm_fb_helper_initial_config(fb_helper,
 						     fb_helper->preferred_bpp);
-		return err;
+		/*
+		 * __drm_fb_helper_initial_config can change deferred_setup,
+		 * if 'false' that means we can go ahead with the rest of
+		 * the setup as normal
+		 */
+		if (fb_helper->deferred_setup)
+			return err;
 	}
 
 	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
-- 
2.13.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/fb-helper: Restore first connection behaviour on deferred setup

[Daniel Vetter]

On Fri, Jun 30, 2017 at 6:51 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> Prior to commit b0aa06e9a7fd ("drm/fb-helper: Support deferred setup"),
> if no output is connected at framebuffer setup time, we get a default
> 1024x768 mode that is going to be used when we first connect a monitor.
> After the commit, on first connection after deferred setup, we probe
> the monitor and get the preferred resolution, but no mode get set
> because the drm_fb_helper_hotplug_event() function returns early
> when the setup has been deferred. That is different from what happens
> on a second re-connect of the monitor, when the native mode get set.
>
> Create a more consistent behaviour by checking in the
> drm_fb_helper_hotplug_event() function if the deferred setup is still
> active. If not, that means we now have a valid framebuffer that can be
> used for setting the correct mode.
>
> Fixes: b0aa06e9a7fd ("drm/fb-helper: Support deferred setup")
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

I thought the analysis over irc was that fbcon picked a different
driver as it's console, and that's why nothing shows up on the malidp
output in the deferred case? That's mildly annoying, but iirc fbcon
has always been rather erratic in multi-gpu setups. Although I thought
that it would by default bind all fbdev drivers as consoles (and then
you need to rebind the right console driver, if the right Kconfig is
enabled, through sysfs).

Either way if the register_framebuffer() call in initial_config isn't
good enough, then we need to add the set_par in initial_config
unconditionally, not just in the deferred probe case. Just disable
fbcon entirely for testing, in that case even without deferred probing
nothing will show up.

I'd say if this is still needed in the single gpu case then we need to
investigate more, but for multi-gpu it is what it is (aka fbcon is not
great).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/fb-helper: Restore first connection behaviour on deferred setup

[Liviu Dudau]

On Fri, Jun 30, 2017 at 08:13:58PM +0200, Daniel Vetter wrote:
> On Fri, Jun 30, 2017 at 6:51 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > Prior to commit b0aa06e9a7fd ("drm/fb-helper: Support deferred setup"),
> > if no output is connected at framebuffer setup time, we get a default
> > 1024x768 mode that is going to be used when we first connect a monitor.
> > After the commit, on first connection after deferred setup, we probe
> > the monitor and get the preferred resolution, but no mode get set
> > because the drm_fb_helper_hotplug_event() function returns early
> > when the setup has been deferred. That is different from what happens
> > on a second re-connect of the monitor, when the native mode get set.
> >
> > Create a more consistent behaviour by checking in the
> > drm_fb_helper_hotplug_event() function if the deferred setup is still
> > active. If not, that means we now have a valid framebuffer that can be
> > used for setting the correct mode.
> >
> > Fixes: b0aa06e9a7fd ("drm/fb-helper: Support deferred setup")
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I thought the analysis over irc was that fbcon picked a different
> driver as it's console, and that's why nothing shows up on the malidp
> output in the deferred case? That's mildly annoying, but iirc fbcon
> has always been rather erratic in multi-gpu setups. Although I thought
> that it would by default bind all fbdev drivers as consoles (and then
> you need to rebind the right console driver, if the right Kconfig is
> enabled, through sysfs).

That's right, fbcon picks a different driver and binds to it. But
before the patch, on the first connection with a monitor that happened
after setup (i.e. when the fbdev emulation mode set the default 1024x768
mode), a full atomic modeset was trigger for the driver that had the
connection and the "card" output was enabled. Now with your patch that
does not happen on the first connection, but it does after I turn off and
back on the monitor. I know that is how most computer problems are solved,
but still ... :)

> 
> Either way if the register_framebuffer() call in initial_config isn't
> good enough, then we need to add the set_par in initial_config
> unconditionally, not just in the deferred probe case. Just disable
> fbcon entirely for testing, in that case even without deferred probing
> nothing will show up.

Actually the un-ballanced call to set_par happens with your patch (it
gets called for all hotplug events that are *not* deferred. I was trying
to balance things and call set_par also for deferred setups where the
__drm_fb_helper_initial_config() call succeeded. And the fact that we do
call set_par in the hotplug path even when the setup is not deferred
tells me that register_framebuffer() is probably not enough.

> 
> I'd say if this is still needed in the single gpu case then we need to
> investigate more, but for multi-gpu it is what it is (aka fbcon is not
> great).

I think single gpu case hides the problem because fbcon forces the right
sequence of calls. And I think the old behaviour was correct, as I was
getting output of the second "card" on hotplug, now I have to turn the
monitor off and on again to get some signal sync.

Best regards,
Liviu

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

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/fb-helper: Restore first connection behaviour on deferred setup

[Daniel Vetter]

On Mon, Jul 03, 2017 at 09:44:50AM +0100, Liviu Dudau wrote:
> On Fri, Jun 30, 2017 at 08:13:58PM +0200, Daniel Vetter wrote:
> > On Fri, Jun 30, 2017 at 6:51 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > > Prior to commit b0aa06e9a7fd ("drm/fb-helper: Support deferred setup"),
> > > if no output is connected at framebuffer setup time, we get a default
> > > 1024x768 mode that is going to be used when we first connect a monitor.
> > > After the commit, on first connection after deferred setup, we probe
> > > the monitor and get the preferred resolution, but no mode get set
> > > because the drm_fb_helper_hotplug_event() function returns early
> > > when the setup has been deferred. That is different from what happens
> > > on a second re-connect of the monitor, when the native mode get set.
> > >
> > > Create a more consistent behaviour by checking in the
> > > drm_fb_helper_hotplug_event() function if the deferred setup is still
> > > active. If not, that means we now have a valid framebuffer that can be
> > > used for setting the correct mode.
> > >
> > > Fixes: b0aa06e9a7fd ("drm/fb-helper: Support deferred setup")
> > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > I thought the analysis over irc was that fbcon picked a different
> > driver as it's console, and that's why nothing shows up on the malidp
> > output in the deferred case? That's mildly annoying, but iirc fbcon
> > has always been rather erratic in multi-gpu setups. Although I thought
> > that it would by default bind all fbdev drivers as consoles (and then
> > you need to rebind the right console driver, if the right Kconfig is
> > enabled, through sysfs).
> 
> That's right, fbcon picks a different driver and binds to it. But
> before the patch, on the first connection with a monitor that happened
> after setup (i.e. when the fbdev emulation mode set the default 1024x768
> mode), a full atomic modeset was trigger for the driver that had the
> connection and the "card" output was enabled. Now with your patch that
> does not happen on the first connection, but it does after I turn off and
> back on the monitor. I know that is how most computer problems are solved,
> but still ... :)
> 
> > 
> > Either way if the register_framebuffer() call in initial_config isn't
> > good enough, then we need to add the set_par in initial_config
> > unconditionally, not just in the deferred probe case. Just disable
> > fbcon entirely for testing, in that case even without deferred probing
> > nothing will show up.
> 
> Actually the un-ballanced call to set_par happens with your patch (it
> gets called for all hotplug events that are *not* deferred. I was trying
> to balance things and call set_par also for deferred setups where the
> __drm_fb_helper_initial_config() call succeeded. And the fact that we do
> call set_par in the hotplug path even when the setup is not deferred
> tells me that register_framebuffer() is probably not enough.

But then you end up doing it twice if that's the fbdev fbcon will bind
too.

And of course we must call set_par to enable the newly hotplugged screen,
it'd be black otherwise. It's just that with just 1 gpu, the
register_framebuffer takes care of the initial config stuff (if you have
fbcon enabled).

If you boot with just 1 gpu, and hotplug that one, it will work correctly.
Multi-gpu + fbcon just don't work in a predictable fashion.

> > I'd say if this is still needed in the single gpu case then we need to
> > investigate more, but for multi-gpu it is what it is (aka fbcon is not
> > great).
> 
> I think single gpu case hides the problem because fbcon forces the right
> sequence of calls. And I think the old behaviour was correct, as I was
> getting output of the second "card" on hotplug, now I have to turn the
> monitor off and on again to get some signal sync.

So the problem is that we rely upon register_framebuffer->fbcon to create
that initial set_par. We could add an unconditional set_par to fix this
bug (the condition really is pre-existing, if you'd boot with 2 gpus and
both connected at start-up the 2nd one would still be off). But that comes
at the cost of forcing everyone to do a 2nd, unecessary modeset right
after boot-up, potentially destroying neat tricks like fastboot.

So we can't just do that either, because that would regress boot-up time.
Now if you have an idea how to fix this without rewriting fbcon+fbdev, we
could try, but I'm not sure that's feasible really ...

After all if you entirely disable fbdev emulation, your screen also stays
off. And maybe you event _want_ it to stay off, even with fbdev emulation
enabled.

So really no idea what exactly we should do here, but adding an
unconditional set_par in the initial_config path isn't the solution I
think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/13] drm/exynos: Remove custom FB helper deferred setup

[Daniel Vetter]

From: Thierry Reding <treding@nvidia.com>

The FB helper core now supports deferred setup, so the driver's custom
implementation can be removed.

v2: Drop NULL check, drm_fb_helper_hotplug_event handles that already.

Cc: Inki Dae <inki.dae@samsung.com>
Cc: Joonyoung Shim <jy0922.shim@samsung.com>
Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c   |  6 ++++--
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 26 +-------------------------
 2 files changed, 5 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 35a8dfc93836..cab9e12d7846 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -395,8 +395,9 @@ static int exynos_drm_bind(struct device *dev)
 	/* init kms poll for handling hpd */
 	drm_kms_helper_poll_init(drm);
 
-	/* force connectors detection */
-	drm_helper_hpd_irq_event(drm);
+	ret = exynos_drm_fbdev_init(drm);
+	if (ret)
+		goto err_cleanup_poll;
 
 	/* register the DRM device */
 	ret = drm_dev_register(drm, 0);
@@ -407,6 +408,7 @@ static int exynos_drm_bind(struct device *dev)
 
 err_cleanup_fbdev:
 	exynos_drm_fbdev_fini(drm);
+err_cleanup_poll:
 	drm_kms_helper_poll_fini(drm);
 	exynos_drm_device_subdrv_remove(drm);
 err_unbind_all:
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index 641531243e04..c3a068409b48 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -183,24 +183,6 @@ static const struct drm_fb_helper_funcs exynos_drm_fb_helper_funcs = {
 	.fb_probe =	exynos_drm_fbdev_create,
 };
 
-static bool exynos_drm_fbdev_is_anything_connected(struct drm_device *dev)
-{
-	struct drm_connector *connector;
-	bool ret = false;
-
-	mutex_lock(&dev->mode_config.mutex);
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		if (connector->status != connector_status_connected)
-			continue;
-
-		ret = true;
-		break;
-	}
-	mutex_unlock(&dev->mode_config.mutex);
-
-	return ret;
-}
-
 int exynos_drm_fbdev_init(struct drm_device *dev)
 {
 	struct exynos_drm_fbdev *fbdev;
@@ -211,9 +193,6 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
 	if (!dev->mode_config.num_crtc || !dev->mode_config.num_connector)
 		return 0;
 
-	if (!exynos_drm_fbdev_is_anything_connected(dev))
-		return 0;
-
 	fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
 	if (!fbdev)
 		return -ENOMEM;
@@ -304,8 +283,5 @@ void exynos_drm_output_poll_changed(struct drm_device *dev)
 	struct exynos_drm_private *private = dev->dev_private;
 	struct drm_fb_helper *fb_helper = private->fb_helper;
 
-	if (fb_helper)
-		drm_fb_helper_hotplug_event(fb_helper);
-	else
-		exynos_drm_fbdev_init(dev);
+	drm_fb_helper_hotplug_event(fb_helper);
 }
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/13] drm/hisilicon: Remove custom FB helper deferred setup

[Daniel Vetter]

From: Thierry Reding <treding@nvidia.com>

The FB helper core now supports deferred setup, so the driver's custom
implementation can be removed.

Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
Cc: Rongrong Zou <zourongrong@gmail.com>
Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
Cc: Chen Feng <puck.chen@hisilicon.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
index 8065d6cb1d7f..da51befa8246 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
@@ -54,14 +54,7 @@ static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
 {
 	struct kirin_drm_private *priv = dev->dev_private;
 
-	if (priv->fbdev) {
-		drm_fbdev_cma_hotplug_event(priv->fbdev);
-	} else {
-		priv->fbdev = drm_fbdev_cma_init(dev, 32,
-						 dev->mode_config.num_connector);
-		if (IS_ERR(priv->fbdev))
-			priv->fbdev = NULL;
-	}
+	drm_fbdev_cma_hotplug_event(priv->fbdev);
 }
 #endif
 
@@ -128,11 +121,19 @@ static int kirin_drm_kms_init(struct drm_device *dev)
 	/* init kms poll for handling hpd */
 	drm_kms_helper_poll_init(dev);
 
-	/* force detection after connectors init */
-	(void)drm_helper_hpd_irq_event(dev);
+	priv->fbdev = drm_fbdev_cma_init(dev, 32,
+					 dev->mode_config.num_connector);
+	if (IS_ERR(priv->fbdev)) {
+		DRM_ERROR("failed to initialize fbdev.\n");
+		ret = PTR_ERR(priv->fbdev);
+		goto err_cleanup_poll;
+	}
 
 	return 0;
 
+err_cleanup_poll:
+	drm_kms_helper_poll_fini(dev);
+	drm_vblank_cleanup(dev);
 err_unbind_all:
 	component_unbind_all(dev->dev, dev);
 err_dc_cleanup:
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/hisilicon: Remove custom FB helper deferred setup

[Daniel Vetter]

From: Thierry Reding <treding@nvidia.com>

The FB helper core now supports deferred setup, so the driver's custom
implementation can be removed.

v2: Dont' resurrect drm_vblank_cleanup.

Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
Cc: Rongrong Zou <zourongrong@gmail.com>
Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
Cc: Chen Feng <puck.chen@hisilicon.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
index 8065d6cb1d7f..1178341c3858 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
@@ -54,14 +54,7 @@ static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
 {
 	struct kirin_drm_private *priv = dev->dev_private;
 
-	if (priv->fbdev) {
-		drm_fbdev_cma_hotplug_event(priv->fbdev);
-	} else {
-		priv->fbdev = drm_fbdev_cma_init(dev, 32,
-						 dev->mode_config.num_connector);
-		if (IS_ERR(priv->fbdev))
-			priv->fbdev = NULL;
-	}
+	drm_fbdev_cma_hotplug_event(priv->fbdev);
 }
 #endif
 
@@ -128,11 +121,18 @@ static int kirin_drm_kms_init(struct drm_device *dev)
 	/* init kms poll for handling hpd */
 	drm_kms_helper_poll_init(dev);
 
-	/* force detection after connectors init */
-	(void)drm_helper_hpd_irq_event(dev);
+	priv->fbdev = drm_fbdev_cma_init(dev, 32,
+					 dev->mode_config.num_connector);
+	if (IS_ERR(priv->fbdev)) {
+		DRM_ERROR("failed to initialize fbdev.\n");
+		ret = PTR_ERR(priv->fbdev);
+		goto err_cleanup_poll;
+	}
 
 	return 0;
 
+err_cleanup_poll:
+	drm_kms_helper_poll_fini(dev);
 err_unbind_all:
 	component_unbind_all(dev->dev, dev);
 err_dc_cleanup:
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 13/13] drm/atomic-helper: Realign function parameters

[Daniel Vetter]

Too jarring.

Fixes: f869a6ecf254 ("drm/atomic: Add target_vblank support in atomic helpers (v2)")
Cc: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 2f269e4267da..5a80b6960ae1 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2964,12 +2964,11 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
 
-static int page_flip_common(
-				struct drm_atomic_state *state,
-				struct drm_crtc *crtc,
-				struct drm_framebuffer *fb,
-				struct drm_pending_vblank_event *event,
-				uint32_t flags)
+static int page_flip_common(struct drm_atomic_state *state,
+			    struct drm_crtc *crtc,
+			    struct drm_framebuffer *fb,
+			    struct drm_pending_vblank_event *event,
+			    uint32_t flags)
 {
 	struct drm_plane *plane = crtc->primary;
 	struct drm_plane_state *plane_state;
@@ -3063,13 +3062,12 @@ EXPORT_SYMBOL(drm_atomic_helper_page_flip);
  * Returns:
  * Returns 0 on success, negative errno numbers on failure.
  */
-int drm_atomic_helper_page_flip_target(
-				struct drm_crtc *crtc,
-				struct drm_framebuffer *fb,
-				struct drm_pending_vblank_event *event,
-				uint32_t flags,
-				uint32_t target,
-				struct drm_modeset_acquire_ctx *ctx)
+int drm_atomic_helper_page_flip_target(struct drm_crtc *crtc,
+				       struct drm_framebuffer *fb,
+				       struct drm_pending_vblank_event *event,
+				       uint32_t flags,
+				       uint32_t target,
+				       struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_plane *plane = crtc->primary;
 	struct drm_atomic_state *state;
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/13] drm/atomic-helper: Realign function parameters

[Deucher, Alexander]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch]
> Sent: Tuesday, June 27, 2017 11:00 AM
> To: DRI Development
> Cc: Intel Graphics Development; Daniel Vetter; Grodzovsky, Andrey;
> Deucher, Alexander; Daniel Vetter
> Subject: [PATCH 13/13] drm/atomic-helper: Realign function parameters
> 
> Too jarring.
> 
> Fixes: f869a6ecf254 ("drm/atomic: Add target_vblank support in atomic
> helpers (v2)")
> Cc: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 2f269e4267da..5a80b6960ae1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2964,12 +2964,11 @@
> drm_atomic_helper_connector_set_property(struct drm_connector
> *connector,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
> 
> -static int page_flip_common(
> -				struct drm_atomic_state *state,
> -				struct drm_crtc *crtc,
> -				struct drm_framebuffer *fb,
> -				struct drm_pending_vblank_event *event,
> -				uint32_t flags)
> +static int page_flip_common(struct drm_atomic_state *state,
> +			    struct drm_crtc *crtc,
> +			    struct drm_framebuffer *fb,
> +			    struct drm_pending_vblank_event *event,
> +			    uint32_t flags)
>  {
>  	struct drm_plane *plane = crtc->primary;
>  	struct drm_plane_state *plane_state;
> @@ -3063,13 +3062,12 @@
> EXPORT_SYMBOL(drm_atomic_helper_page_flip);
>   * Returns:
>   * Returns 0 on success, negative errno numbers on failure.
>   */
> -int drm_atomic_helper_page_flip_target(
> -				struct drm_crtc *crtc,
> -				struct drm_framebuffer *fb,
> -				struct drm_pending_vblank_event *event,
> -				uint32_t flags,
> -				uint32_t target,
> -				struct drm_modeset_acquire_ctx *ctx)
> +int drm_atomic_helper_page_flip_target(struct drm_crtc *crtc,
> +				       struct drm_framebuffer *fb,
> +				       struct drm_pending_vblank_event
> *event,
> +				       uint32_t flags,
> +				       uint32_t target,
> +				       struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_plane *plane = crtc->primary;
>  	struct drm_atomic_state *state;
> --
> 2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/13] drm/atomic-helper: Realign function parameters

[Harry Wentland]

On 2017-06-27 10:59 AM, Daniel Vetter wrote:
> Too jarring.
> 
> Fixes: f869a6ecf254 ("drm/atomic: Add target_vblank support in atomic helpers (v2)")
> Cc: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 2f269e4267da..5a80b6960ae1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2964,12 +2964,11 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
>  
> -static int page_flip_common(
> -				struct drm_atomic_state *state,
> -				struct drm_crtc *crtc,
> -				struct drm_framebuffer *fb,
> -				struct drm_pending_vblank_event *event,
> -				uint32_t flags)
> +static int page_flip_common(struct drm_atomic_state *state,
> +			    struct drm_crtc *crtc,
> +			    struct drm_framebuffer *fb,
> +			    struct drm_pending_vblank_event *event,
> +			    uint32_t flags)
>  {
>  	struct drm_plane *plane = crtc->primary;
>  	struct drm_plane_state *plane_state;
> @@ -3063,13 +3062,12 @@ EXPORT_SYMBOL(drm_atomic_helper_page_flip);
>   * Returns:
>   * Returns 0 on success, negative errno numbers on failure.
>   */
> -int drm_atomic_helper_page_flip_target(
> -				struct drm_crtc *crtc,
> -				struct drm_framebuffer *fb,
> -				struct drm_pending_vblank_event *event,
> -				uint32_t flags,
> -				uint32_t target,
> -				struct drm_modeset_acquire_ctx *ctx)
> +int drm_atomic_helper_page_flip_target(struct drm_crtc *crtc,
> +				       struct drm_framebuffer *fb,
> +				       struct drm_pending_vblank_event *event,
> +				       uint32_t flags,
> +				       uint32_t target,
> +				       struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_plane *plane = crtc->primary;
>  	struct drm_atomic_state *state;
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/13] drm/atomic-helper: Realign function parameters

[Daniel Vetter]

On Tue, Jun 27, 2017 at 11:42:53AM -0400, Harry Wentland wrote:
> On 2017-06-27 10:59 AM, Daniel Vetter wrote:
> > Too jarring.
> > 
> > Fixes: f869a6ecf254 ("drm/atomic: Add target_vblank support in atomic helpers (v2)")
> > Cc: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Applied, thanks for your review.
-Daniel

> 
> Harry
> 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 24 +++++++++++-------------
> >  1 file changed, 11 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 2f269e4267da..5a80b6960ae1 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2964,12 +2964,11 @@ drm_atomic_helper_connector_set_property(struct drm_connector *connector,
> >  }
> >  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
> >  
> > -static int page_flip_common(
> > -				struct drm_atomic_state *state,
> > -				struct drm_crtc *crtc,
> > -				struct drm_framebuffer *fb,
> > -				struct drm_pending_vblank_event *event,
> > -				uint32_t flags)
> > +static int page_flip_common(struct drm_atomic_state *state,
> > +			    struct drm_crtc *crtc,
> > +			    struct drm_framebuffer *fb,
> > +			    struct drm_pending_vblank_event *event,
> > +			    uint32_t flags)
> >  {
> >  	struct drm_plane *plane = crtc->primary;
> >  	struct drm_plane_state *plane_state;
> > @@ -3063,13 +3062,12 @@ EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> >   * Returns:
> >   * Returns 0 on success, negative errno numbers on failure.
> >   */
> > -int drm_atomic_helper_page_flip_target(
> > -				struct drm_crtc *crtc,
> > -				struct drm_framebuffer *fb,
> > -				struct drm_pending_vblank_event *event,
> > -				uint32_t flags,
> > -				uint32_t target,
> > -				struct drm_modeset_acquire_ctx *ctx)
> > +int drm_atomic_helper_page_flip_target(struct drm_crtc *crtc,
> > +				       struct drm_framebuffer *fb,
> > +				       struct drm_pending_vblank_event *event,
> > +				       uint32_t flags,
> > +				       uint32_t target,
> > +				       struct drm_modeset_acquire_ctx *ctx)
> >  {
> >  	struct drm_plane *plane = crtc->primary;
> >  	struct drm_atomic_state *state;
> > 

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

✓ Fi.CI.BAT: success for fbdev locking rework and deferred setup, take 2

[Patchwork]

== Series Details ==

Series: fbdev locking rework and deferred setup, take 2
URL   : https://patchwork.freedesktop.org/series/26437/
State : success

== Summary ==

Series 26437v1 fbdev locking rework and deferred setup, take 2
https://patchwork.freedesktop.org/api/1.0/series/26437/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-7560u) fdo#100125 +1
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-pnv-d510) fdo#101597 +1

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:439s
fi-bdw-gvtdvm    total:279  pass:257  dwarn:8   dfail:0   fail:0   skip:14  time:429s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:354s
fi-bsw-n3050     total:279  pass:242  dwarn:1   dfail:0   fail:0   skip:36  time:528s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:507s
fi-byt-j1900     total:279  pass:253  dwarn:2   dfail:0   fail:0   skip:24  time:494s
fi-byt-n2820     total:279  pass:249  dwarn:2   dfail:0   fail:0   skip:28  time:487s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:598s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:434s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:417s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:419s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:490s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:479s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:470s
fi-kbl-7560u     total:279  pass:268  dwarn:1   dfail:0   fail:0   skip:10  time:577s
fi-kbl-r         total:279  pass:260  dwarn:1   dfail:0   fail:0   skip:18  time:584s
fi-pnv-d510      total:279  pass:222  dwarn:2   dfail:0   fail:0   skip:55  time:559s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:456s
fi-skl-6700hq    total:279  pass:223  dwarn:1   dfail:0   fail:30  skip:24  time:340s
fi-skl-6700k     total:279  pass:257  dwarn:4   dfail:0   fail:0   skip:18  time:466s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:476s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:432s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:541s
fi-snb-2600      total:279  pass:249  dwarn:0   dfail:0   fail:1   skip:29  time:409s

bf26e1dbbba24a7697559f1131d4be99747b7646 drm-tip: 2017y-06m-27d-13h-59m-07s UTC integration manifest
7e59fef drm/atomic-helper: Realign function parameters
c05b19b drm/hisilicon: Remove custom FB helper deferred setup
1547eea drm/exynos: Remove custom FB helper deferred setup
b39637d drm/fb-helper: Support deferred setup
a937020 drm/fb-helper: Split dpms handling into legacy and atomic paths
798bcdf drm/fb-helper: Stop using mode_config.mutex for internals
d472950 drm/fb-helper: Push locking into restore_fbdev_mode_atomic|legacy
4a74480 drm/fb-helper: Push locking into pan_display_atomic|legacy
5fa27ff drm/fb-helper: Drop locking from the vsync wait ioctl code
cc8cb12 drm/fb-helper: Push locking in fb_is_bound
f920b96 drm/fb-helper: Add top-level lock
79896e3 drm/i915: Drop FBDEV #ifdev in mst code
697548f drm/fb-helper: Push down modeset lock into FB helpers

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5050/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/13] fbdev locking rework and deferred setup, take 2

[John Stultz]

On Tue, Jun 27, 2017 at 7:59 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> Thanks to Liviu's help I realized that I fumbled the locking rework completely.
> This one here should be better, but somehow I'm having a real bad day today and
> I spent all day typing shit code, and then making it worse.
>
> This here seems to work at first glance, but please test and review carefully.

After some self-caused testing pain (somehow missed picking one of the
patches in mutt), I managed to get these tested w/ HiKey against
4.12-rc7.

Things seemed to work fairly well.

Though the one quirk I saw was that previously if I booted the device
w/o HDMI plugged in, it would boot up defaulting to 1024x768
resolution, and when I plugged in the HDMI cable, the monitor would
start up and use that resolution.

With your patchset, if I boot without the monitor attached,  it seems
no fb is created, so surfaceflinger crashes repeatedly trying to start
up.  Then when I do plug the montior in, I get the following crash:

[  106.503724] Unable to handle kernel NULL pointer dereference at
virtual address 00000038
[  106.512050] pgd = ffffff8008e89000
[  106.515596] [00000038] *pgd=0000000077ffe003,
*pud=0000000077ffe003, *pmd=0000000000000000
[  106.524050] Internal error: Oops: 96000005 [#1] PREEMPT SMP
[  106.529678] CPU: 2 PID: 1184 Comm: kworker/2:2 Tainted: G        W
     4.12.0-rc7-00093-g42439fe-dirty #3054
[  106.539711] Hardware name: HiKey Development Board (DT)
[  106.544996] Workqueue: events adv7511_hpd_work
[  106.549484] task: ffffffc0753c3e80 task.stack: ffffffc074b0c000
[  106.555449] PC is at drm_sysfs_hotplug_event+0x34/0x58
[  106.560624] LR is at drm_sysfs_hotplug_event+0x34/0x58
[  106.565796] pc : [<ffffff8008536634>] lr : [<ffffff8008536634>]
pstate: 40000145
[  106.573221] sp : ffffffc074b0fd40
[  106.576562] x29: ffffffc074b0fd40 x28: 0000000000000000
[  106.581925] x27: ffffffc005f93d20 x26: ffffffc074d387b8
[  106.587288] x25: ffffffc0753c3e80 x24: ffffffc0747e3258
[  106.592650] x23: 0000000000000000 x22: ffffffc077f44a80
[  106.598012] x21: ffffffc077f46900 x20: ffffffc07441cd00
[  106.603373] x19: 0000000000000000 x18: 0000000000000000
[  106.608733] x17: 0000000000000000 x16: 0000000000000000
[  106.614093] x15: 00000018cc1b97ed x14: 0000000000000000
[  106.619453] x13: 0000000000000000 x12: 0000000000000000
[  106.624813] x11: 00000000000002e5 x10: 0000000000000880
[  106.630173] x9 : ffffffc074b0fa10 x8 : ffffffc0753c4760
[  106.635534] x7 : ffffffc077f3ec80 x6 : ffffffc077f3c090
[  106.640895] x5 : ffffff8008da0568 x4 : 0000000000000000
[  106.646254] x3 : 0000000000000000 x2 : ffffff8008c39d90
[  106.651613] x1 : 0000000000000001 x0 : ffffff8008c02fe8
[  106.656981] Process kworker/2:2 (pid: 1184, stack limit = 0xffffffc074b0c000)
[  106.664152] Stack: (0xffffffc074b0fd40 to 0xffffffc074b10000)
[  106.669940] fd40: ffffffc074b0fd70 ffffff800851d1ec
0000000000000000 ffffff80085c7b04
[  106.677813] fd60: ffffff8008c39d80 0000000000000000
ffffffc074b0fd90 ffffff8008550bd4
[  106.685687] fd80: ffffffc0747e3018 ffffff80080df688
ffffffc074b0fdc0 ffffff80080d0814
[  106.693561] fda0: 0000000000000000 ffffff8008a45eb8
ffffffc074b0fdc0 000000d0080d0808
[  106.701436] fdc0: ffffffc074b0fe00 ffffff80080d0a68
ffffffc07441cd00 ffffffc074a83a00
[  106.709310] fde0: ffffffc077f44a80 ffffffc07441cd30
ffffff8008d46000 ffffffc077f44aa0
[  106.717184] fe00: ffffffc074b0fe60 ffffff80080d6974
ffffffc074d38780 ffffffc074a83a00
[  106.725059] fe20: ffffff8008df9160 ffffffc0753c3e80
ffffff8008bf9528 ffffffc07441cd00
[  106.732929] fe40: ffffff80080d0a20 ffffffc074d387b8
ffffffc005f93d20 0000000000000000
[  106.740801] fe60: 0000000000000000 ffffff8008082ec0
ffffff80080d6878 ffffffc074a83a00
[  106.748671] fe80: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
[  106.756541] fea0: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
[  106.764411] fec0: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
[  106.772281] fee0: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
[  106.780151] ff00: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
[  106.788021] ff20: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
[  106.795889] ff40: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
[  106.803759] ff60: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
[  106.811629] ff80: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
[  106.819475] ffa0: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
[  106.827308] ffc0: 0000000000000000 0000000000000005
0000000000000000 0000000000000000
[  106.835143] ffe0: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
[  106.842974] Call trace:
[  106.845422] Exception stack(0xffffffc074b0fb70 to 0xffffffc074b0fca0)
[  106.851866] fb60:
0000000000000000 0000008000000000
[  106.859701] fb80: ffffffc074b0fd40 ffffff8008536634
ffffffc074b0fba0 ffffff80085b6d88
[  106.867535] fba0: ffffffc074b0fbd0 ffffff8008700f4c
ffffffc0747e2818 0000000000000002
[  106.875370] fbc0: ffffffc0747e28b8 0000000000000002
ffffffc074b0fc20 ffffff80086fd5dc
[  106.883205] fbe0: ffffffc0747e28b8 0000000000000001
0000000000000002 ffffffc074b0fca0
[  106.891039] fc00: ffffffc074b0fc50 ffffff80086fb5c8
ffffff8008c02fe8 0000000000000001
[  106.898872] fc20: ffffff8008c39d90 0000000000000000
0000000000000000 ffffff8008da0568
[  106.906706] fc40: ffffffc077f3c090 ffffffc077f3ec80
ffffffc0753c4760 ffffffc074b0fa10
[  106.914540] fc60: 0000000000000880 00000000000002e5
0000000000000000 0000000000000000
[  106.922374] fc80: 0000000000000000 00000018cc1b97ed
0000000000000000 0000000000000000
[  106.930209] [<ffffff8008536634>] drm_sysfs_hotplug_event+0x34/0x58
[  106.936397] [<ffffff800851d1ec>] drm_kms_helper_hotplug_event+0x14/0x38
[  106.943015] [<ffffff8008550bd4>] adv7511_hpd_work+0x4c/0x60
[  106.948594] [<ffffff80080d0814>] process_one_work+0x114/0x320
[  106.954344] [<ffffff80080d0a68>] worker_thread+0x48/0x428
[  106.959748] [<ffffff80080d6974>] kthread+0xfc/0x128
[  106.964632] [<ffffff8008082ec0>] ret_from_fork+0x10/0x50
[  106.969949] Code: 913fa020 52800021 a9027fa3 97fff898 (f9401e60)
[  106.976112] ---[ end trace dd098614ee129219 ]---
[  107.016693] Kernel panic - not syncing: Fatal exception


I've got to dig a bit more here (probably something is wrong with the
bridge driver), but let me know if the lack of the 1024x768 default fb
is expected.

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

Re: [PATCH 00/13] fbdev locking rework and deferred setup, take 2

[Daniel Vetter]

On Tue, Jun 27, 2017 at 04:02:02PM -0700, John Stultz wrote:
> On Tue, Jun 27, 2017 at 7:59 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > Thanks to Liviu's help I realized that I fumbled the locking rework completely.
> > This one here should be better, but somehow I'm having a real bad day today and
> > I spent all day typing shit code, and then making it worse.
> >
> > This here seems to work at first glance, but please test and review carefully.
> 
> After some self-caused testing pain (somehow missed picking one of the
> patches in mutt), I managed to get these tested w/ HiKey against
> 4.12-rc7.
> 
> Things seemed to work fairly well.
> 
> Though the one quirk I saw was that previously if I booted the device
> w/o HDMI plugged in, it would boot up defaulting to 1024x768
> resolution, and when I plugged in the HDMI cable, the monitor would
> start up and use that resolution.
> 
> With your patchset, if I boot without the monitor attached,  it seems
> no fb is created, so surfaceflinger crashes repeatedly trying to start
> up.  Then when I do plug the montior in, I get the following crash:
> 
> [  106.503724] Unable to handle kernel NULL pointer dereference at
> virtual address 00000038
> [  106.512050] pgd = ffffff8008e89000
> [  106.515596] [00000038] *pgd=0000000077ffe003,
> *pud=0000000077ffe003, *pmd=0000000000000000
> [  106.524050] Internal error: Oops: 96000005 [#1] PREEMPT SMP
> [  106.529678] CPU: 2 PID: 1184 Comm: kworker/2:2 Tainted: G        W
>      4.12.0-rc7-00093-g42439fe-dirty #3054
> [  106.539711] Hardware name: HiKey Development Board (DT)
> [  106.544996] Workqueue: events adv7511_hpd_work
> [  106.549484] task: ffffffc0753c3e80 task.stack: ffffffc074b0c000
> [  106.555449] PC is at drm_sysfs_hotplug_event+0x34/0x58
> [  106.560624] LR is at drm_sysfs_hotplug_event+0x34/0x58
> [  106.565796] pc : [<ffffff8008536634>] lr : [<ffffff8008536634>]
> pstate: 40000145
> [  106.573221] sp : ffffffc074b0fd40
> [  106.576562] x29: ffffffc074b0fd40 x28: 0000000000000000
> [  106.581925] x27: ffffffc005f93d20 x26: ffffffc074d387b8
> [  106.587288] x25: ffffffc0753c3e80 x24: ffffffc0747e3258
> [  106.592650] x23: 0000000000000000 x22: ffffffc077f44a80
> [  106.598012] x21: ffffffc077f46900 x20: ffffffc07441cd00
> [  106.603373] x19: 0000000000000000 x18: 0000000000000000
> [  106.608733] x17: 0000000000000000 x16: 0000000000000000
> [  106.614093] x15: 00000018cc1b97ed x14: 0000000000000000
> [  106.619453] x13: 0000000000000000 x12: 0000000000000000
> [  106.624813] x11: 00000000000002e5 x10: 0000000000000880
> [  106.630173] x9 : ffffffc074b0fa10 x8 : ffffffc0753c4760
> [  106.635534] x7 : ffffffc077f3ec80 x6 : ffffffc077f3c090
> [  106.640895] x5 : ffffff8008da0568 x4 : 0000000000000000
> [  106.646254] x3 : 0000000000000000 x2 : ffffff8008c39d90
> [  106.651613] x1 : 0000000000000001 x0 : ffffff8008c02fe8
> [  106.656981] Process kworker/2:2 (pid: 1184, stack limit = 0xffffffc074b0c000)
> [  106.664152] Stack: (0xffffffc074b0fd40 to 0xffffffc074b10000)
> [  106.669940] fd40: ffffffc074b0fd70 ffffff800851d1ec
> 0000000000000000 ffffff80085c7b04
> [  106.677813] fd60: ffffff8008c39d80 0000000000000000
> ffffffc074b0fd90 ffffff8008550bd4
> [  106.685687] fd80: ffffffc0747e3018 ffffff80080df688
> ffffffc074b0fdc0 ffffff80080d0814
> [  106.693561] fda0: 0000000000000000 ffffff8008a45eb8
> ffffffc074b0fdc0 000000d0080d0808
> [  106.701436] fdc0: ffffffc074b0fe00 ffffff80080d0a68
> ffffffc07441cd00 ffffffc074a83a00
> [  106.709310] fde0: ffffffc077f44a80 ffffffc07441cd30
> ffffff8008d46000 ffffffc077f44aa0
> [  106.717184] fe00: ffffffc074b0fe60 ffffff80080d6974
> ffffffc074d38780 ffffffc074a83a00
> [  106.725059] fe20: ffffff8008df9160 ffffffc0753c3e80
> ffffff8008bf9528 ffffffc07441cd00
> [  106.732929] fe40: ffffff80080d0a20 ffffffc074d387b8
> ffffffc005f93d20 0000000000000000
> [  106.740801] fe60: 0000000000000000 ffffff8008082ec0
> ffffff80080d6878 ffffffc074a83a00
> [  106.748671] fe80: 0000000000000000 0000000000000000
> 0000000000000000 0000000000000000
> [  106.756541] fea0: 0000000000000000 0000000000000000
> 0000000000000000 0000000000000000
> [  106.764411] fec0: 0000000000000000 0000000000000000
> 0000000000000000 0000000000000000
> [  106.772281] fee0: 0000000000000000 0000000000000000
> 0000000000000000 0000000000000000
> [  106.780151] ff00: 0000000000000000 0000000000000000
> 0000000000000000 0000000000000000
> [  106.788021] ff20: 0000000000000000 0000000000000000
> 0000000000000000 0000000000000000
> [  106.795889] ff40: 0000000000000000 0000000000000000
> 0000000000000000 0000000000000000
> [  106.803759] ff60: 0000000000000000 0000000000000000
> 0000000000000000 0000000000000000
> [  106.811629] ff80: 0000000000000000 0000000000000000
> 0000000000000000 0000000000000000
> [  106.819475] ffa0: 0000000000000000 0000000000000000
> 0000000000000000 0000000000000000
> [  106.827308] ffc0: 0000000000000000 0000000000000005
> 0000000000000000 0000000000000000
> [  106.835143] ffe0: 0000000000000000 0000000000000000
> 0000000000000000 0000000000000000
> [  106.842974] Call trace:
> [  106.845422] Exception stack(0xffffffc074b0fb70 to 0xffffffc074b0fca0)
> [  106.851866] fb60:
> 0000000000000000 0000008000000000
> [  106.859701] fb80: ffffffc074b0fd40 ffffff8008536634
> ffffffc074b0fba0 ffffff80085b6d88
> [  106.867535] fba0: ffffffc074b0fbd0 ffffff8008700f4c
> ffffffc0747e2818 0000000000000002
> [  106.875370] fbc0: ffffffc0747e28b8 0000000000000002
> ffffffc074b0fc20 ffffff80086fd5dc
> [  106.883205] fbe0: ffffffc0747e28b8 0000000000000001
> 0000000000000002 ffffffc074b0fca0
> [  106.891039] fc00: ffffffc074b0fc50 ffffff80086fb5c8
> ffffff8008c02fe8 0000000000000001
> [  106.898872] fc20: ffffff8008c39d90 0000000000000000
> 0000000000000000 ffffff8008da0568
> [  106.906706] fc40: ffffffc077f3c090 ffffffc077f3ec80
> ffffffc0753c4760 ffffffc074b0fa10
> [  106.914540] fc60: 0000000000000880 00000000000002e5
> 0000000000000000 0000000000000000
> [  106.922374] fc80: 0000000000000000 00000018cc1b97ed
> 0000000000000000 0000000000000000
> [  106.930209] [<ffffff8008536634>] drm_sysfs_hotplug_event+0x34/0x58
> [  106.936397] [<ffffff800851d1ec>] drm_kms_helper_hotplug_event+0x14/0x38
> [  106.943015] [<ffffff8008550bd4>] adv7511_hpd_work+0x4c/0x60
> [  106.948594] [<ffffff80080d0814>] process_one_work+0x114/0x320
> [  106.954344] [<ffffff80080d0a68>] worker_thread+0x48/0x428
> [  106.959748] [<ffffff80080d6974>] kthread+0xfc/0x128
> [  106.964632] [<ffffff8008082ec0>] ret_from_fork+0x10/0x50
> [  106.969949] Code: 913fa020 52800021 a9027fa3 97fff898 (f9401e60)
> [  106.976112] ---[ end trace dd098614ee129219 ]---
> [  107.016693] Kernel panic - not syncing: Fatal exception
> 
> 
> I've got to dig a bit more here (probably something is wrong with the
> bridge driver), but let me know if the lack of the 1024x768 default fb
> is expected.

That's the entire point of the deferred setup trick - only once you plug
in the first output will we create the fbdev (because the framebuffer for
that stuff is static, because fbdev doesn't allow remapping and moving).
Oops otoh isn't what should happen.

Since I don't do arm, can you pls explain where/how it blew up? Only quick
guess I have is that adv7511->connector.dev is NULL in adv7511_hpd_work,
which would indeed be a bug in your bridge. Or the driver load sequence of
your driver, handling hotplug before the connector is fully registered
isn't a good idea.
-Daniel
-- 
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

✓ Fi.CI.BAT: success for fbdev locking rework and deferred setup, take 2 (rev2)

[Patchwork]

== Series Details ==

Series: fbdev locking rework and deferred setup, take 2 (rev2)
URL   : https://patchwork.freedesktop.org/series/26437/
State : success

== Summary ==

Series 26437v2 fbdev locking rework and deferred setup, take 2
https://patchwork.freedesktop.org/api/1.0/series/26437/revisions/2/mbox/

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:438s
fi-bdw-gvtdvm    total:279  pass:257  dwarn:8   dfail:0   fail:0   skip:14  time:426s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:356s
fi-bsw-n3050     total:279  pass:242  dwarn:1   dfail:0   fail:0   skip:36  time:530s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:505s
fi-byt-j1900     total:279  pass:253  dwarn:2   dfail:0   fail:0   skip:24  time:487s
fi-byt-n2820     total:279  pass:249  dwarn:2   dfail:0   fail:0   skip:28  time:485s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:596s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:434s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:417s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:421s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:495s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:469s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:466s
fi-kbl-7560u     total:279  pass:268  dwarn:1   dfail:0   fail:0   skip:10  time:568s
fi-kbl-r         total:279  pass:260  dwarn:1   dfail:0   fail:0   skip:18  time:580s
fi-pnv-d510      total:279  pass:221  dwarn:3   dfail:0   fail:0   skip:55  time:555s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:450s
fi-skl-6700hq    total:279  pass:223  dwarn:1   dfail:0   fail:30  skip:24  time:341s
fi-skl-6700k     total:279  pass:257  dwarn:4   dfail:0   fail:0   skip:18  time:466s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:474s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:430s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:547s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:415s

51ebcf3056873a715adda8d12449fbaad9b1b9f4 drm-tip: 2017y-06m-27d-20h-39m-13s UTC integration manifest
dc0a528 drm/atomic-helper: Realign function parameters
bd8562e drm/hisilicon: Remove custom FB helper deferred setup
a5fa9bd drm/exynos: Remove custom FB helper deferred setup
d13237b drm/fb-helper: Support deferred setup
b6fd921 drm/fb-helper: Split dpms handling into legacy and atomic paths
85eca32 drm/fb-helper: Stop using mode_config.mutex for internals
9c5a78e drm/fb-helper: Push locking into restore_fbdev_mode_atomic|legacy
4e6d765 drm/fb-helper: Push locking into pan_display_atomic|legacy
3f0dacf drm/fb-helper: Drop locking from the vsync wait ioctl code
a207f8d drm/fb-helper: Push locking in fb_is_bound
9f59a3c drm/fb-helper: Add top-level lock
d53b06d drm/i915: Drop FBDEV #ifdev in mst code
455bdc2 drm/fb-helper: Push down modeset lock into FB helpers

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5055/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for fbdev locking rework and deferred setup, take 2 (rev3)

[Patchwork]

== Series Details ==

Series: fbdev locking rework and deferred setup, take 2 (rev3)
URL   : https://patchwork.freedesktop.org/series/26437/
State : success

== Summary ==

Series 26437v3 fbdev locking rework and deferred setup, take 2
https://patchwork.freedesktop.org/api/1.0/series/26437/revisions/3/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-kbl-7560u) fdo#100125 +1
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                dmesg-warn -> PASS       (fi-pnv-d510) fdo#101597 +1

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:444s
fi-bdw-gvtdvm    total:279  pass:257  dwarn:8   dfail:0   fail:0   skip:14  time:423s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:354s
fi-bsw-n3050     total:279  pass:242  dwarn:1   dfail:0   fail:0   skip:36  time:522s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:504s
fi-byt-j1900     total:279  pass:253  dwarn:2   dfail:0   fail:0   skip:24  time:488s
fi-byt-n2820     total:279  pass:249  dwarn:2   dfail:0   fail:0   skip:28  time:487s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:594s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:432s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:414s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:425s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:502s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:476s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:465s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:570s
fi-kbl-r         total:279  pass:260  dwarn:1   dfail:0   fail:0   skip:18  time:579s
fi-pnv-d510      total:279  pass:222  dwarn:2   dfail:0   fail:0   skip:55  time:558s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:459s
fi-skl-6700hq    total:279  pass:223  dwarn:1   dfail:0   fail:30  skip:24  time:345s
fi-skl-6700k     total:279  pass:257  dwarn:4   dfail:0   fail:0   skip:18  time:466s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:476s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:444s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:548s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:403s

90513552ce32ddf2a4efffa21a5df5f21a425a18 drm-tip: 2017y-06m-28d-11h-26m-16s UTC integration manifest
a712ea9 drm/atomic-helper: Realign function parameters
1ed8ce9 drm/hisilicon: Remove custom FB helper deferred setup
9166d3e drm/exynos: Remove custom FB helper deferred setup
61d4a4b drm/fb-helper: Support deferred setup
4392314 drm/fb-helper: Split dpms handling into legacy and atomic paths
c4cbe02 drm/fb-helper: Stop using mode_config.mutex for internals
61c2425 drm/fb-helper: Push locking into restore_fbdev_mode_atomic|legacy
584a0e9 drm/fb-helper: Push locking into pan_display_atomic|legacy
2a53d27 drm/fb-helper: Drop locking from the vsync wait ioctl code
58f43d8 drm/fb-helper: Push locking in fb_is_bound
11b9905 drm/fb-helper: Add top-level lock
95a9f45 drm/i915: Drop FBDEV #ifdev in mst code
34a5437 drm/fb-helper: Push down modeset lock into FB helpers

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5057/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 00/13] fbdev locking + deferred setup take 3

[Daniel Vetter]

Hi all,

Here's the locking patches respun with Maarten's review. I think that part is
ready for merging. The deferred setup itself needs more thought, since both
Maarten and Liviu are still unhappy with what happens between the deferred setup
and fbcon.

Cheers, Daniel

Daniel Vetter (9):
  drm/i915: Drop FBDEV #ifdev in mst code
  drm/fb-helper: Push locking in fb_is_bound
  drm/fb-helper: Drop locking from the vsync wait ioctl code
  drm/fb-helper: Push locking into pan_display_atomic|legacy
  drm/fb-helper: Push locking into restore_fbdev_mode_atomic|legacy
  drm/fb-helper: Stop using mode_config.mutex for internals
  drm/fb-helper: Split dpms handling into legacy and atomic paths
  drm/fb-helper: Support deferred setup
  drm/i915: Protect against deferred fbdev setup

Thierry Reding (4):
  drm/fb-helper: Push down modeset lock into FB helpers
  drm/fb-helper: Add top-level lock
  drm/exynos: Remove custom FB helper deferred setup
  drm/hisilicon: Remove custom FB helper deferred setup

 drivers/gpu/drm/drm_fb_helper.c                 | 359 ++++++++++++++----------
 drivers/gpu/drm/drm_vblank.c                    |   2 +-
 drivers/gpu/drm/exynos/exynos_drm_drv.c         |   6 +-
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c       |  26 +-
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c |  20 +-
 drivers/gpu/drm/i915/i915_debugfs.c             |   2 +-
 drivers/gpu/drm/i915/i915_drv.h                 |   2 -
 drivers/gpu/drm/i915/intel_dp_mst.c             |  47 +---
 drivers/gpu/drm/i915/intel_fbdev.c              |  16 +-
 drivers/gpu/drm/radeon/radeon_dp_mst.c          |   7 -
 include/drm/drm_fb_helper.h                     |  42 ++-
 11 files changed, 297 insertions(+), 232 deletions(-)

-- 
2.13.2

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

[PATCH 01/13] drm/fb-helper: Push down modeset lock into FB helpers

[Daniel Vetter]

From: Thierry Reding <treding@nvidia.com>

Move the modeset locking from drivers into FB helpers.

v2: Also handle intel_connector_add_to_fbdev.

v3: Prevent race in intel_dp_mst with ->detect (Maarten)

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Tested-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Thierry Reding <treding@nvidia.com> (v1)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c        | 40 +++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_dp_mst.c    |  9 +++-----
 drivers/gpu/drm/radeon/radeon_dp_mst.c |  7 ------
 3 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 574af01d3ce9..5e73cc9f51e3 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -109,8 +109,8 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);
 	for (({ lockdep_assert_held(&(fbh)->dev->mode_config.mutex); }), \
 	     i__ = 0; i__ < (fbh)->connector_count; i__++)
 
-int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
-				    struct drm_connector *connector)
+static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
+					     struct drm_connector *connector)
 {
 	struct drm_fb_helper_connector *fb_conn;
 	struct drm_fb_helper_connector **temp;
@@ -141,8 +141,23 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
 	drm_connector_get(connector);
 	fb_conn->connector = connector;
 	fb_helper->connector_info[fb_helper->connector_count++] = fb_conn;
+
 	return 0;
 }
+
+int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
+				    struct drm_connector *connector)
+{
+	int err;
+
+	mutex_lock(&fb_helper->dev->mode_config.mutex);
+
+	err = __drm_fb_helper_add_one_connector(fb_helper, connector);
+
+	mutex_unlock(&fb_helper->dev->mode_config.mutex);
+
+	return err;
+}
 EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
 
 /**
@@ -172,8 +187,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 	mutex_lock(&dev->mode_config.mutex);
 	drm_connector_list_iter_begin(dev, &conn_iter);
 	drm_for_each_connector_iter(connector, &conn_iter) {
-		ret = drm_fb_helper_add_one_connector(fb_helper, connector);
-
+		ret = __drm_fb_helper_add_one_connector(fb_helper, connector);
 		if (ret)
 			goto fail;
 	}
@@ -198,8 +212,8 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 }
 EXPORT_SYMBOL(drm_fb_helper_single_add_all_connectors);
 
-int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
-				       struct drm_connector *connector)
+static int __drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
+						struct drm_connector *connector)
 {
 	struct drm_fb_helper_connector *fb_helper_connector;
 	int i, j;
@@ -227,6 +241,20 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
 
 	return 0;
 }
+
+int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
+				       struct drm_connector *connector)
+{
+	int err;
+
+	mutex_lock(&fb_helper->dev->mode_config.mutex);
+
+	err = __drm_fb_helper_remove_one_connector(fb_helper, connector);
+
+	mutex_unlock(&fb_helper->dev->mode_config.mutex);
+
+	return err;
+}
 EXPORT_SYMBOL(drm_fb_helper_remove_one_connector);
 
 static void drm_fb_helper_save_lut_atomic(struct drm_crtc *crtc, struct drm_fb_helper *helper)
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 2cf046beae0f..9aa959284497 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -501,11 +501,8 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 static void intel_dp_register_mst_connector(struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
-	struct drm_device *dev = connector->dev;
 
-	drm_modeset_lock_all(dev);
 	intel_connector_add_to_fbdev(intel_connector);
-	drm_modeset_unlock_all(dev);
 
 	drm_connector_register(&intel_connector->base);
 }
@@ -514,15 +511,15 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 					   struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
-	struct drm_device *dev = connector->dev;
 
 	drm_connector_unregister(connector);
 
 	/* need to nuke the connector */
-	drm_modeset_lock_all(dev);
 	intel_connector_remove_from_fbdev(intel_connector);
+	/* prevent race with the check in ->detect */
+	drm_modeset_lock(&connector->dev->mode_config.connection_mutex, NULL);
 	intel_connector->mst_port = NULL;
-	drm_modeset_unlock_all(dev);
+	drm_modeset_unlock(&connector->dev->mode_config.connection_mutex);
 
 	drm_connector_unreference(&intel_connector->base);
 	DRM_DEBUG_KMS("\n");
diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c
index 6598306dca9b..ebdf1b859cb6 100644
--- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
+++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
@@ -300,9 +300,7 @@ static void radeon_dp_register_mst_connector(struct drm_connector *connector)
 	struct drm_device *dev = connector->dev;
 	struct radeon_device *rdev = dev->dev_private;
 
-	drm_modeset_lock_all(dev);
 	radeon_fb_add_connector(rdev, connector);
-	drm_modeset_unlock_all(dev);
 
 	drm_connector_register(connector);
 }
@@ -315,13 +313,8 @@ static void radeon_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	struct radeon_device *rdev = dev->dev_private;
 
 	drm_connector_unregister(connector);
-	/* need to nuke the connector */
-	drm_modeset_lock_all(dev);
-	/* dpms off */
 	radeon_fb_remove_connector(rdev, connector);
-
 	drm_connector_cleanup(connector);
-	drm_modeset_unlock_all(dev);
 
 	kfree(connector);
 	DRM_DEBUG_KMS("\n");
-- 
2.13.2

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