Re: [Intel-gfx] [PATCH v3 06/10] drm/fb-helper: Support deferred setup
On Tue, Mar 21, 2017 at 09:13:54AM +0100, Thierry Reding wrote:
> From: Thierry Reding
>
> 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.
>
> Tested-by: John Stultz
> Signed-off-by: Thierry Reding
Ok 2nd attempt at making this work, probably easier to go back to v2.
> ---
> drivers/gpu/drm/drm_fb_helper.c | 60
> +
> include/drm/drm_fb_helper.h | 21 +++
> 2 files changed, 76 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 9060adcf7cf8..d4a2c97d8b02 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -511,6 +511,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct
> drm_fb_helper *fb_helper)
> if (!drm_fbdev_emulation)
> return -ENODEV;
>
> + if (fb_helper->deferred_setup)
> + return 0;
Please wrap in READ_ONCE to make it clear we're doing lockless checking
here.
> +
> mutex_lock(&fb_helper->lock);
> drm_modeset_lock_all(dev);
>
> @@ -1597,6 +1600,23 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo
> *var,
> }
> EXPORT_SYMBOL(drm_fb_helper_pan_display);
>
> +static bool drm_fb_helper_maybe_connected(struct drm_fb_helper *helper)
> +{
> + bool connected = false;
> + unsigned int i;
> +
> + for (i = 0; i < helper->connector_count; i++) {
> + struct drm_fb_helper_connector *fb = helper->connector_info[i];
> +
> + if (fb->connector->status != connector_status_disconnected) {
> + connected = true;
> + break;
> + }
> + }
> +
> + return connected;
> +}
> +
> /*
> * 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
> @@ -2254,8 +2274,6 @@ 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->dev->mode_config.mutex);
> @@ -2378,6 +2396,7 @@ 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;
> + unsigned int width, height;
> struct fb_info *info;
> int ret;
>
> @@ -2385,14 +2404,34 @@ int drm_fb_helper_initial_config(struct drm_fb_helper
> *fb_helper, int bpp_sel)
> return 0;
>
From here ...
> mutex_lock(&dev->mode_config.mutex);
> - drm_setup_crtcs(fb_helper,
> - dev->mode_config.max_width,
> - dev->mode_config.max_height);
> +
> + width = dev->mode_config.max_width;
> + height = dev->mode_config.max_height;
> +
> + if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
> + DRM_DEBUG_KMS("No connectors reported connected with modes\n");
> +
> + /*
> + * If everything's disconnected, there's no use in attempting to set
> + * up fbdev.
> + */
> + if (!drm_fb_helper_maybe_connected(fb_helper)) {
> + DRM_INFO("No outputs connected, deferring setup\n");
> + fb_helper->preferred_bpp = bpp_sel;
> + fb_helper->deferred_setup = true;
> + mutex_unlock(&dev->mode_config.mutex);
> + return 0;
> + }
> +
> + drm_setup_crtcs(fb_helper, width, height);
> +
> ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> mutex_unlock(&dev->mode_config.mutex);
> if (ret)
> return ret;
>
> + fb_helper->deferred_setup = false;
T
Re: [Intel-gfx] [PATCH v3 06/10] drm/fb-helper: Support deferred setup
On Wed, Mar 22, 2017 at 10:06 PM, Thierry Reding
wrote:
> On Tue, Mar 21, 2017 at 11:10:22AM +0100, Daniel Vetter wrote:
>> On Tue, Mar 21, 2017 at 09:13:54AM +0100, Thierry Reding wrote:
> [...]
>> > diff --git a/drivers/gpu/drm/drm_fb_helper.c
>> > b/drivers/gpu/drm/drm_fb_helper.c
> [...]
>> > @@ -2437,11 +2476,16 @@ 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;
>> > + unsigned int width, height;
>> > int err = 0;
>> >
>> > if (!drm_fbdev_emulation)
>> > return 0;
>> >
>> > + if (fb_helper->deferred_setup)
>> > + return drm_fb_helper_initial_config(fb_helper,
>> > + fb_helper->preferred_bpp);
>>
>> I think this must be moved under the protection of ->lock, you might race
>> otherwise (e.g. hpd vs. userspace forcing a re-probe, both noticing the
>> change).
>
> I think I had originally put this under the lock only to see that result
> in a deadlock. I can't quite remember why that was, but testing shows
> that this still happens. It's getting rather late, so I'll have to defer
> tracking this down to tomorrow.
initial_config also grabs your new lock, which means you'd need more
static _unlocked variants to make it work. I didn't see any other
obvious problem this would cause.
-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: [Intel-gfx] [PATCH v3 06/10] drm/fb-helper: Support deferred setup
On Tue, Mar 21, 2017 at 11:10:22AM +0100, Daniel Vetter wrote:
> On Tue, Mar 21, 2017 at 09:13:54AM +0100, Thierry Reding wrote:
[...]
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c
> > b/drivers/gpu/drm/drm_fb_helper.c
[...]
> > @@ -2437,11 +2476,16 @@ 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;
> > + unsigned int width, height;
> > int err = 0;
> >
> > if (!drm_fbdev_emulation)
> > return 0;
> >
> > + if (fb_helper->deferred_setup)
> > + return drm_fb_helper_initial_config(fb_helper,
> > + fb_helper->preferred_bpp);
>
> I think this must be moved under the protection of ->lock, you might race
> otherwise (e.g. hpd vs. userspace forcing a re-probe, both noticing the
> change).
I think I had originally put this under the lock only to see that result
in a deadlock. I can't quite remember why that was, but testing shows
that this still happens. It's getting rather late, so I'll have to defer
tracking this down to tomorrow.
Thierry
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 06/10] drm/fb-helper: Support deferred setup
On Tue, Mar 21, 2017 at 09:13:54AM +0100, Thierry Reding wrote:
> From: Thierry Reding
>
> 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.
>
> Tested-by: John Stultz
> Signed-off-by: Thierry Reding
Tiny nitpicks below, with those addressed:
Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_fb_helper.c | 60
> +
> include/drm/drm_fb_helper.h | 21 +++
> 2 files changed, 76 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 9060adcf7cf8..d4a2c97d8b02 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -511,6 +511,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct
> drm_fb_helper *fb_helper)
> if (!drm_fbdev_emulation)
> return -ENODEV;
>
> + if (fb_helper->deferred_setup)
> + return 0;
READ_ONCE or put it under the ->lock protection. Just for clarity.
> +
> mutex_lock(&fb_helper->lock);
> drm_modeset_lock_all(dev);
>
> @@ -1597,6 +1600,23 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo
> *var,
> }
> EXPORT_SYMBOL(drm_fb_helper_pan_display);
>
> +static bool drm_fb_helper_maybe_connected(struct drm_fb_helper *helper)
> +{
> + bool connected = false;
> + unsigned int i;
> +
> + for (i = 0; i < helper->connector_count; i++) {
> + struct drm_fb_helper_connector *fb = helper->connector_info[i];
> +
> + if (fb->connector->status != connector_status_disconnected) {
> + connected = true;
> + break;
> + }
> + }
> +
> + return connected;
> +}
> +
> /*
> * 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
> @@ -2254,8 +2274,6 @@ 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->dev->mode_config.mutex);
> @@ -2378,6 +2396,7 @@ 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;
> + unsigned int width, height;
> struct fb_info *info;
> int ret;
>
> @@ -2385,14 +2404,34 @@ int drm_fb_helper_initial_config(struct drm_fb_helper
> *fb_helper, int bpp_sel)
> return 0;
>
> mutex_lock(&dev->mode_config.mutex);
> - drm_setup_crtcs(fb_helper,
> - dev->mode_config.max_width,
> - dev->mode_config.max_height);
> +
> + width = dev->mode_config.max_width;
> + height = dev->mode_config.max_height;
> +
> + if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
> + DRM_DEBUG_KMS("No connectors reported connected with modes\n");
> +
> + /*
> + * If everything's disconnected, there's no use in attempting to set
> + * up fbdev.
> + */
> + if (!drm_fb_helper_maybe_connected(fb_helper)) {
> + DRM_INFO("No outputs connected, deferring setup\n");
> + fb_helper->preferred_bpp = bpp_sel;
> + fb_helper->deferred_setup = true;
> + mutex_unlock(&dev->mode_config.mutex);
> + return 0;
> + }
> +
> + drm_setup_crtcs(fb_helper, width, height);
> +
> ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
> mutex_unlock(&dev->mode_config.mutex);
> if (ret)
> return ret;
>
> + fb_helper->deferred_setup = false;
> +
> info = fb_hel
[Intel-gfx] [PATCH v3 06/10] drm/fb-helper: Support deferred setup
From: Thierry Reding
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.
Tested-by: John Stultz
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/drm_fb_helper.c | 60 +
include/drm/drm_fb_helper.h | 21 +++
2 files changed, 76 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 9060adcf7cf8..d4a2c97d8b02 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -511,6 +511,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct
drm_fb_helper *fb_helper)
if (!drm_fbdev_emulation)
return -ENODEV;
+ if (fb_helper->deferred_setup)
+ return 0;
+
mutex_lock(&fb_helper->lock);
drm_modeset_lock_all(dev);
@@ -1597,6 +1600,23 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo
*var,
}
EXPORT_SYMBOL(drm_fb_helper_pan_display);
+static bool drm_fb_helper_maybe_connected(struct drm_fb_helper *helper)
+{
+ bool connected = false;
+ unsigned int i;
+
+ for (i = 0; i < helper->connector_count; i++) {
+ struct drm_fb_helper_connector *fb = helper->connector_info[i];
+
+ if (fb->connector->status != connector_status_disconnected) {
+ connected = true;
+ break;
+ }
+ }
+
+ return connected;
+}
+
/*
* 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
@@ -2254,8 +2274,6 @@ 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->dev->mode_config.mutex);
@@ -2378,6 +2396,7 @@ 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;
+ unsigned int width, height;
struct fb_info *info;
int ret;
@@ -2385,14 +2404,34 @@ int drm_fb_helper_initial_config(struct drm_fb_helper
*fb_helper, int bpp_sel)
return 0;
mutex_lock(&dev->mode_config.mutex);
- drm_setup_crtcs(fb_helper,
- dev->mode_config.max_width,
- dev->mode_config.max_height);
+
+ width = dev->mode_config.max_width;
+ height = dev->mode_config.max_height;
+
+ if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0)
+ DRM_DEBUG_KMS("No connectors reported connected with modes\n");
+
+ /*
+* If everything's disconnected, there's no use in attempting to set
+* up fbdev.
+*/
+ if (!drm_fb_helper_maybe_connected(fb_helper)) {
+ DRM_INFO("No outputs connected, deferring setup\n");
+ fb_helper->preferred_bpp = bpp_sel;
+ fb_helper->deferred_setup = true;
+ mutex_unlock(&dev->mode_config.mutex);
+ return 0;
+ }
+
+ drm_setup_crtcs(fb_helper, width, height);
+
ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel);
mutex_unlock(&dev->mode_config.mutex);
if (ret)
return ret;
+ fb_helper->deferred_setup = false;
+
info = fb_helper->fbdev;
info->var.pixclock = 0;
ret = register_framebuffer(info);
@@ -2437,11 +2476,16 @@ 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;
+ unsigned int width, height;
int err = 0;
if (!drm_fbdev_em
