[PATCH 13/19] drm: do not steal the display if we have a master

[PATCH 13/19] drm: do not steal the display if we have a master

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Sometimes we want to disable all the screens on a system, because that
will allow the graphics card to be put into low-power states. The
problem is that, for example, while all screens are disabled, if we
get a hotplug interrupt, fbcon will decide to set a mode instead of
keeping everything disabled, which will remove us from our low power
states.

Let's assume that if there's a DRM master, it will be able to do
whatever is appropriate when we get the hotplug.

This problem can be reproduced by the runtime PM test program from
intel-gpu-tools: we disable all the screens so the graphics device can
be put into D3, then something triggers a hotplug interrupt, fbcon
sets a mode and breaks our test suite. The problem can be reproduced
more easily by the "i2c" subtest.

Other approaches considered for the problem:
    - Return "false" if "bound == 0" and the caller of
      drm_fb_helper_is_bound is a hotplug handler. This would break
      the case where the machine boots with no outputs connected, then
      the user plugs a monitor.
    - Add a new IOCTL to force fbcon to not set modes. This would keep
      all the current applications behaving the same, but adding a new
      IOCTL is not always the greatest idea.
    - Return false only if "dev->primary->master && bound == 0". This
      was my first implementation, but Chris suggested we should do
      the check irrespective of the "bound" variable.

Thanks to Daniel Vetter for the investigation, ideas and the
implementation of the hotplug alternative.

v2: - Do the check first, irrespective of "bound".
    - Cc dri-devel

Cc: dri-devel@lists.freedesktop.org
Credits-to: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 0a19401..98a0363 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -359,6 +359,11 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
 	struct drm_crtc *crtc;
 	int bound = 0, crtcs_bound = 0;
 
+	/* Sometimes user space wants everything disabled, so don't steal the
+	 * display if there's a master. */
+	if (dev->primary->master)
+		return false;
+
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
 		if (crtc->fb)
 			crtcs_bound++;
@@ -368,6 +373,7 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
 
 	if (bound < crtcs_bound)
 		return false;
+
 	return true;
 }
 
-- 
1.8.3.1

Re: [PATCH 13/19] drm: do not steal the display if we have a master

[Daniel Vetter]

On Wed, Nov 27, 2013 at 06:24:08PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Sometimes we want to disable all the screens on a system, because that
> will allow the graphics card to be put into low-power states. The
> problem is that, for example, while all screens are disabled, if we
> get a hotplug interrupt, fbcon will decide to set a mode instead of
> keeping everything disabled, which will remove us from our low power
> states.
> 
> Let's assume that if there's a DRM master, it will be able to do
> whatever is appropriate when we get the hotplug.
> 
> This problem can be reproduced by the runtime PM test program from
> intel-gpu-tools: we disable all the screens so the graphics device can
> be put into D3, then something triggers a hotplug interrupt, fbcon
> sets a mode and breaks our test suite. The problem can be reproduced
> more easily by the "i2c" subtest.
> 
> Other approaches considered for the problem:
>     - Return "false" if "bound == 0" and the caller of
>       drm_fb_helper_is_bound is a hotplug handler. This would break
>       the case where the machine boots with no outputs connected, then
>       the user plugs a monitor.
>     - Add a new IOCTL to force fbcon to not set modes. This would keep
>       all the current applications behaving the same, but adding a new
>       IOCTL is not always the greatest idea.
>     - Return false only if "dev->primary->master && bound == 0". This
>       was my first implementation, but Chris suggested we should do
>       the check irrespective of the "bound" variable.
> 
> Thanks to Daniel Vetter for the investigation, ideas and the
> implementation of the hotplug alternative.
> 
> v2: - Do the check first, irrespective of "bound".
>     - Cc dri-devel
> 
> Cc: dri-devel@lists.freedesktop.org
> Credits-to: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0a19401..98a0363 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -359,6 +359,11 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
>  	struct drm_crtc *crtc;
>  	int bound = 0, crtcs_bound = 0;
>  
> +	/* Sometimes user space wants everything disabled, so don't steal the
> +	 * display if there's a master. */
> +	if (dev->primary->master)
> +		return false;
> +
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>  		if (crtc->fb)
>  			crtcs_bound++;
> @@ -368,6 +373,7 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
>  
>  	if (bound < crtcs_bound)
>  		return false;
> +
>  	return true;
>  }
>  
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 13/19] drm: do not steal the display if we have a master

[David Herrmann]

Hi

On Wed, Nov 27, 2013 at 9:24 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Sometimes we want to disable all the screens on a system, because that
> will allow the graphics card to be put into low-power states. The
> problem is that, for example, while all screens are disabled, if we
> get a hotplug interrupt, fbcon will decide to set a mode instead of
> keeping everything disabled, which will remove us from our low power
> states.
>
> Let's assume that if there's a DRM master, it will be able to do
> whatever is appropriate when we get the hotplug.
>
> This problem can be reproduced by the runtime PM test program from
> intel-gpu-tools: we disable all the screens so the graphics device can
> be put into D3, then something triggers a hotplug interrupt, fbcon
> sets a mode and breaks our test suite. The problem can be reproduced
> more easily by the "i2c" subtest.
>
> Other approaches considered for the problem:
>     - Return "false" if "bound == 0" and the caller of
>       drm_fb_helper_is_bound is a hotplug handler. This would break
>       the case where the machine boots with no outputs connected, then
>       the user plugs a monitor.
>     - Add a new IOCTL to force fbcon to not set modes. This would keep
>       all the current applications behaving the same, but adding a new
>       IOCTL is not always the greatest idea.
>     - Return false only if "dev->primary->master && bound == 0". This
>       was my first implementation, but Chris suggested we should do
>       the check irrespective of the "bound" variable.
>
> Thanks to Daniel Vetter for the investigation, ideas and the
> implementation of the hotplug alternative.
>
> v2: - Do the check first, irrespective of "bound".
>     - Cc dri-devel

Thank god all that will get dropped with CONFIG_FB=n. Anyhow:

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> Cc: dri-devel@lists.freedesktop.org
> Credits-to: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0a19401..98a0363 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -359,6 +359,11 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
>         struct drm_crtc *crtc;
>         int bound = 0, crtcs_bound = 0;
>
> +       /* Sometimes user space wants everything disabled, so don't steal the
> +        * display if there's a master. */
> +       if (dev->primary->master)
> +               return false;
> +
>         list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>                 if (crtc->fb)
>                         crtcs_bound++;
> @@ -368,6 +373,7 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
>
>         if (bound < crtcs_bound)
>                 return false;
> +
>         return true;
>  }
>
> --
> 1.8.3.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel