Re: [Intel-gfx] [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

2013-05-08 Thread Daniel Vetter 
On Fri, May 3, 2013 at 6:50 PM, Daniel Vetter dan...@ffwll.ch wrote:
 On Fri, May 3, 2013 at 6:28 PM, Chris Wilson ch...@chris-wilson.co.uk wrote:
 Not quite. It was added afterwards, I know for it is one of my mistakes
 that I belatedly recognised as trying to workaround a bug in UXA. And we
 then layered on further bugs to try and patch up the glaring holes then
 introduced.

 I've done some digging around and found some cool stuff, but no change
 which introduce the original bug that we've never consulted dpms state
 in drm_crtc_helper_set_mode but just blindly enabled encoders. Most of
 the commits I've dug out try to paper over that mismatch by updating
 dpms state, too. E.g. bf9dc102 which was then fixed up a bit in
 811aaa55 but I don't see any mention of someone explicitly kill dpms
 checks.

 Of course for DP support we've started to add dpms state checks to our
 dp encoder (since double-enable kills DP). Then duplicated the
 drm_connector-dpms into our own intel_dp-dpms state since the crtc
 helpers liked to frob around with dpms in a rather ill-defined way.
 But that's just hilarity and flailing around on top, nothing that I
 could see fundamentally changed that a full setcrtc implicitly enabled
 the encoders/crtc hw. And since we have our own modeset code that all
 disappeared (since a full modeset simply force-updates the dpms state
 to reflect reality).

 But nowhere have a found a commit to support your claim, just lots of
 flailing around due to the ill-defined semantics of this all. Care to
 do some more digging?

Ping for history digging ... or an ack on the patch ;-)

Alternative ideas are welcome too ofc, but under the assumption that
setcrtc always kinda enabled encoders/connectors implicitly
(neglecting a few funny corner cases every screwed up in different
ways) this hack here seems the most sensibleconsistent option we
have.
-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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

2013-05-03 Thread Imre Deak 
Currently the driver's assumed behavior for a modeset with an attached
FB is that the corresponding connector will be switched to DPMS ON mode
if it happened to be in DPMS OFF (or another power save mode). This
wasn't enforced though if only the FB changed, everything else (format,
connector etc.) remaining the same. In this case we only set the new FB
base and left the connector in the old power save mode.

Fix this by forcing a full modeset whenever there is an attached FB and
any affected connector is in a power save mode.

Signed-off-by: Imre Deak imre.d...@intel.com
---
 drivers/gpu/drm/i915/intel_display.c |   21 -
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 2939524..0aee2ba 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8380,6 +8380,21 @@ static void intel_set_config_restore_state(struct 
drm_device *dev,
}
 }
 
+static bool
+crtc_connector_off(struct drm_crtc *crtc, struct drm_connector *connectors,
+  int num_connectors)
+{
+   int i;
+
+   for (i = 0; i  num_connectors; i++)
+   if (connectors[i].encoder 
+   connectors[i].encoder-crtc == crtc 
+   connectors[i].dpms != DRM_MODE_DPMS_ON)
+   return true;
+
+   return false;
+}
+
 static void
 intel_set_config_compute_mode_changes(struct drm_mode_set *set,
  struct intel_set_config *config)
@@ -8397,8 +8412,12 @@ intel_set_config_compute_mode_changes(struct 
drm_mode_set *set,
} else if (set-fb-pixel_format !=
   set-crtc-fb-pixel_format) {
config-mode_changed = true;
-   } else
+   } else if (crtc_connector_off(set-crtc, *set-connectors,
+ set-num_connectors)) {
+   config-mode_changed = true;
+   } else {
config-fb_changed = true;
+   }
}
 
if (set-fb  (set-x != set-crtc-x || set-y != set-crtc-y))
-- 
1.7.10.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

2013-05-03 Thread Daniel Vetter 
On Fri, May 3, 2013 at 1:22 PM, Imre Deak imre.d...@intel.com wrote:
 Currently the driver's assumed behavior for a modeset with an attached
 FB is that the corresponding connector will be switched to DPMS ON mode
 if it happened to be in DPMS OFF (or another power save mode). This
 wasn't enforced though if only the FB changed, everything else (format,
 connector etc.) remaining the same. In this case we only set the new FB
 base and left the connector in the old power save mode.

 Fix this by forcing a full modeset whenever there is an attached FB and
 any affected connector is in a power save mode.

 Signed-off-by: Imre Deak imre.d...@intel.com


Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61642
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=59834
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=59339

Safe for the first just a few wild guesses. Can you please spam
bugzilla with this patch (there's tons more spurious kms_flip fail
iirc, also a few of the kms_flip jitter reports should be fixed by
now).

Thanks, 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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

2013-05-03 Thread Daniel Vetter 
On Fri, May 3, 2013 at 1:27 PM, Daniel Vetter dan...@ffwll.ch wrote:
 On Fri, May 3, 2013 at 1:22 PM, Imre Deak imre.d...@intel.com wrote:
 Currently the driver's assumed behavior for a modeset with an attached
 FB is that the corresponding connector will be switched to DPMS ON mode
 if it happened to be in DPMS OFF (or another power save mode). This
 wasn't enforced though if only the FB changed, everything else (format,
 connector etc.) remaining the same. In this case we only set the new FB
 base and left the connector in the old power save mode.

 Fix this by forcing a full modeset whenever there is an attached FB and
 any affected connector is in a power save mode.

 Signed-off-by: Imre Deak imre.d...@intel.com


 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61642
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=59834
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=59339

 Safe for the first just a few wild guesses. Can you please spam
 bugzilla with this patch (there's tons more spurious kms_flip fail
 iirc, also a few of the kms_flip jitter reports should be fixed by
 now).

Also, and i-g-t to specifically exercise this would be nice ... Maybe
easiest on top of kms_flip, using dpms off, but then a set_base as an
implicit dpms on.
-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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

2013-05-03 Thread Imre Deak 
On Fri, 2013-05-03 at 13:32 +0200, Daniel Vetter wrote:
 On Fri, May 3, 2013 at 1:27 PM, Daniel Vetter dan...@ffwll.ch wrote:
  On Fri, May 3, 2013 at 1:22 PM, Imre Deak imre.d...@intel.com wrote:
  Currently the driver's assumed behavior for a modeset with an attached
  FB is that the corresponding connector will be switched to DPMS ON mode
  if it happened to be in DPMS OFF (or another power save mode). This
  wasn't enforced though if only the FB changed, everything else (format,
  connector etc.) remaining the same. In this case we only set the new FB
  base and left the connector in the old power save mode.
 
  Fix this by forcing a full modeset whenever there is an attached FB and
  any affected connector is in a power save mode.
 
  Signed-off-by: Imre Deak imre.d...@intel.com
 
 
  Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61642

Chris marked this as a dup for another bug with an other root cause.

  Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=59834

Yes, this is tested against the patch already.

  Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=59339

Ok I'll point them to the fix.

  Safe for the first just a few wild guesses. Can you please spam
  bugzilla with this patch (there's tons more spurious kms_flip fail
  iirc, also a few of the kms_flip jitter reports should be fixed by
  now).
 
 Also, and i-g-t to specifically exercise this would be nice ... Maybe
 easiest on top of kms_flip, using dpms off, but then a set_base as an
 implicit dpms on.

Ok, will put together something.

--Imre

 -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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

2013-05-03 Thread Chris Wilson 
On Fri, May 03, 2013 at 02:22:09PM +0300, Imre Deak wrote:
 Currently the driver's assumed behavior for a modeset with an attached
 FB is that the corresponding connector will be switched to DPMS ON mode
 if it happened to be in DPMS OFF (or another power save mode). This
 wasn't enforced though if only the FB changed, everything else (format,
 connector etc.) remaining the same. In this case we only set the new FB
 base and left the connector in the old power save mode.
 
 Fix this by forcing a full modeset whenever there is an attached FB and
 any affected connector is in a power save mode.
 
 Signed-off-by: Imre Deak imre.d...@intel.com

NAK. Papering over bugs, much?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

2013-05-03 Thread Jani Nikula 
On Fri, 03 May 2013, Imre Deak imre.d...@intel.com wrote:
 Currently the driver's assumed behavior for a modeset with an attached
 FB is that the corresponding connector will be switched to DPMS ON mode
 if it happened to be in DPMS OFF (or another power save mode). This
 wasn't enforced though if only the FB changed, everything else (format,
 connector etc.) remaining the same. In this case we only set the new FB
 base and left the connector in the old power save mode.

 Fix this by forcing a full modeset whenever there is an attached FB and
 any affected connector is in a power save mode.

 Signed-off-by: Imre Deak imre.d...@intel.com
 ---
  drivers/gpu/drm/i915/intel_display.c |   21 -
  1 file changed, 20 insertions(+), 1 deletion(-)

 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index 2939524..0aee2ba 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -8380,6 +8380,21 @@ static void intel_set_config_restore_state(struct 
 drm_device *dev,
   }
  }
  
 +static bool
 +crtc_connector_off(struct drm_crtc *crtc, struct drm_connector *connectors,
 +int num_connectors)
 +{
 + int i;
 +
 + for (i = 0; i  num_connectors; i++)
 + if (connectors[i].encoder 
 + connectors[i].encoder-crtc == crtc 
 + connectors[i].dpms != DRM_MODE_DPMS_ON)
 + return true;
 +
 + return false;
 +}
 +
  static void
  intel_set_config_compute_mode_changes(struct drm_mode_set *set,
 struct intel_set_config *config)
 @@ -8397,8 +8412,12 @@ intel_set_config_compute_mode_changes(struct 
 drm_mode_set *set,
   } else if (set-fb-pixel_format !=
  set-crtc-fb-pixel_format) {
   config-mode_changed = true;
 - } else
 + } else if (crtc_connector_off(set-crtc, *set-connectors,
 +   set-num_connectors)) {

Bikeshed, crtc_connector_off() sounds like disabling something, while
is_crtc_connector_off() would sound like checking the state.

Jani.


 + config-mode_changed = true;
 + } else {
   config-fb_changed = true;
 + }
   }
  
   if (set-fb  (set-x != set-crtc-x || set-y != set-crtc-y))
 -- 
 1.7.10.4

 ___
 Intel-gfx mailing list
 Intel-gfx@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

2013-05-03 Thread Imre Deak 
On Fri, 2013-05-03 at 12:44 +0100, Chris Wilson wrote:
 On Fri, May 03, 2013 at 02:22:09PM +0300, Imre Deak wrote:
  Currently the driver's assumed behavior for a modeset with an attached
  FB is that the corresponding connector will be switched to DPMS ON mode
  if it happened to be in DPMS OFF (or another power save mode). This
  wasn't enforced though if only the FB changed, everything else (format,
  connector etc.) remaining the same. In this case we only set the new FB
  base and left the connector in the old power save mode.
  
  Fix this by forcing a full modeset whenever there is an attached FB and
  any affected connector is in a power save mode.
  
  Signed-off-by: Imre Deak imre.d...@intel.com
 
 NAK. Papering over bugs, much?

Ok. I posted this based on our IRC discussion where we seemed to agree
that DPMS_ON is assumed already; just that it's not done consistently.
That is if user space does a modeset now that ends up in a full modeset
(lets say a new FB with different format) then the kernel will do a
DPMS_ON. But if the new FB happens to be the same format then it won't.
I thought this is inconsistent and should be fixed.

Another way to make it consistent would be then to remove DPMS ON from
the full modeset path..

--Imre


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

2013-05-03 Thread Chris Wilson 
On Fri, May 03, 2013 at 02:55:45PM +0300, Imre Deak wrote:
 On Fri, 2013-05-03 at 12:44 +0100, Chris Wilson wrote:
  On Fri, May 03, 2013 at 02:22:09PM +0300, Imre Deak wrote:
   Currently the driver's assumed behavior for a modeset with an attached
   FB is that the corresponding connector will be switched to DPMS ON mode
   if it happened to be in DPMS OFF (or another power save mode). This
   wasn't enforced though if only the FB changed, everything else (format,
   connector etc.) remaining the same. In this case we only set the new FB
   base and left the connector in the old power save mode.
   
   Fix this by forcing a full modeset whenever there is an attached FB and
   any affected connector is in a power save mode.
   
   Signed-off-by: Imre Deak imre.d...@intel.com
  
  NAK. Papering over bugs, much?
 
 Ok. I posted this based on our IRC discussion where we seemed to agree
 that DPMS_ON is assumed already; just that it's not done consistently.
 That is if user space does a modeset now that ends up in a full modeset
 (lets say a new FB with different format) then the kernel will do a
 DPMS_ON. But if the new FB happens to be the same format then it won't.
 I thought this is inconsistent and should be fixed.
 
 Another way to make it consistent would be then to remove DPMS ON from
 the full modeset path..

Right, we have mentioned in the past that the conflation between DPMS
and modesetting is a more recent bug that is going to cause us even more
trouble later. I am not sure how we can fix that as UXA already makes
many bad assumptions.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

2013-05-03 Thread Imre Deak 
On Fri, 2013-05-03 at 13:22 +0100, Chris Wilson wrote:
 On Fri, May 03, 2013 at 02:55:45PM +0300, Imre Deak wrote:
  On Fri, 2013-05-03 at 12:44 +0100, Chris Wilson wrote:
   On Fri, May 03, 2013 at 02:22:09PM +0300, Imre Deak wrote:
Currently the driver's assumed behavior for a modeset with an attached
FB is that the corresponding connector will be switched to DPMS ON mode
if it happened to be in DPMS OFF (or another power save mode). This
wasn't enforced though if only the FB changed, everything else (format,
connector etc.) remaining the same. In this case we only set the new FB
base and left the connector in the old power save mode.

Fix this by forcing a full modeset whenever there is an attached FB and
any affected connector is in a power save mode.

Signed-off-by: Imre Deak imre.d...@intel.com
   
   NAK. Papering over bugs, much?
  
  Ok. I posted this based on our IRC discussion where we seemed to agree
  that DPMS_ON is assumed already; just that it's not done consistently.
  That is if user space does a modeset now that ends up in a full modeset
  (lets say a new FB with different format) then the kernel will do a
  DPMS_ON. But if the new FB happens to be the same format then it won't.
  I thought this is inconsistent and should be fixed.
  
  Another way to make it consistent would be then to remove DPMS ON from
  the full modeset path..
 
 Right, we have mentioned in the past that the conflation between DPMS
 and modesetting is a more recent bug that is going to cause us even more
 trouble later. I am not sure how we can fix that as UXA already makes
 many bad assumptions.

Ok, so I assume the right approach for user space is to do an explicit
DPMS_ON after modesetting.

--Imre

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

2013-05-03 Thread Daniel Vetter 
On Fri, May 3, 2013 at 2:22 PM, Chris Wilson ch...@chris-wilson.co.uk wrote:
 On Fri, May 03, 2013 at 02:55:45PM +0300, Imre Deak wrote:
 On Fri, 2013-05-03 at 12:44 +0100, Chris Wilson wrote:
  On Fri, May 03, 2013 at 02:22:09PM +0300, Imre Deak wrote:
   Currently the driver's assumed behavior for a modeset with an attached
   FB is that the corresponding connector will be switched to DPMS ON mode
   if it happened to be in DPMS OFF (or another power save mode). This
   wasn't enforced though if only the FB changed, everything else (format,
   connector etc.) remaining the same. In this case we only set the new FB
   base and left the connector in the old power save mode.
  
   Fix this by forcing a full modeset whenever there is an attached FB and
   any affected connector is in a power save mode.
  
   Signed-off-by: Imre Deak imre.d...@intel.com
 
  NAK. Papering over bugs, much?

 Ok. I posted this based on our IRC discussion where we seemed to agree
 that DPMS_ON is assumed already; just that it's not done consistently.
 That is if user space does a modeset now that ends up in a full modeset
 (lets say a new FB with different format) then the kernel will do a
 DPMS_ON. But if the new FB happens to be the same format then it won't.
 I thought this is inconsistent and should be fixed.

 Another way to make it consistent would be then to remove DPMS ON from
 the full modeset path..

 Right, we have mentioned in the past that the conflation between DPMS
 and modesetting is a more recent bug that is going to cause us even more
 trouble later. I am not sure how we can fix that as UXA already makes
 many bad assumptions.

Afaict the conflagration of dpms on after a modeset has been even in
the old crtc helpers. The only difference is that now we explicitly
update the dpms state tracking to avoid double-encoder enables. That
part allowed us to rip out tons of state tracking from tons of places.

So ever since kms happened, we have inconsistent setcrtc semantics.
Stopping to force dpms on will horribly break existing userspace, so
we can't do this. Making sure that the semantics are more consistent
(especially if we further optimize modeset transitions for fastboot)
seems like the lesser of two evils.

We'll get a 2nd chance to get this right the atomic modeset ioctl.
Until then, I want this duct-taped together, and Imre's patch looks
like a viable approach. Working beats ugly semantics here imo.

Cheers, 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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

2013-05-03 Thread Jesse Barnes 
On Fri, 3 May 2013 17:55:31 +0200
Daniel Vetter dan...@ffwll.ch wrote:

 On Fri, May 3, 2013 at 2:22 PM, Chris Wilson ch...@chris-wilson.co.uk wrote:
  On Fri, May 03, 2013 at 02:55:45PM +0300, Imre Deak wrote:
  On Fri, 2013-05-03 at 12:44 +0100, Chris Wilson wrote:
   On Fri, May 03, 2013 at 02:22:09PM +0300, Imre Deak wrote:
Currently the driver's assumed behavior for a modeset with an attached
FB is that the corresponding connector will be switched to DPMS ON mode
if it happened to be in DPMS OFF (or another power save mode). This
wasn't enforced though if only the FB changed, everything else (format,
connector etc.) remaining the same. In this case we only set the new FB
base and left the connector in the old power save mode.
   
Fix this by forcing a full modeset whenever there is an attached FB and
any affected connector is in a power save mode.
   
Signed-off-by: Imre Deak imre.d...@intel.com
  
   NAK. Papering over bugs, much?
 
  Ok. I posted this based on our IRC discussion where we seemed to agree
  that DPMS_ON is assumed already; just that it's not done consistently.
  That is if user space does a modeset now that ends up in a full modeset
  (lets say a new FB with different format) then the kernel will do a
  DPMS_ON. But if the new FB happens to be the same format then it won't.
  I thought this is inconsistent and should be fixed.
 
  Another way to make it consistent would be then to remove DPMS ON from
  the full modeset path..
 
  Right, we have mentioned in the past that the conflation between DPMS
  and modesetting is a more recent bug that is going to cause us even more
  trouble later. I am not sure how we can fix that as UXA already makes
  many bad assumptions.
 
 Afaict the conflagration of dpms on after a modeset has been even in
 the old crtc helpers. The only difference is that now we explicitly
 update the dpms state tracking to avoid double-encoder enables. That
 part allowed us to rip out tons of state tracking from tons of places.
 
 So ever since kms happened, we have inconsistent setcrtc semantics.
 Stopping to force dpms on will horribly break existing userspace, so
 we can't do this. Making sure that the semantics are more consistent
 (especially if we further optimize modeset transitions for fastboot)
 seems like the lesser of two evils.
 
 We'll get a 2nd chance to get this right the atomic modeset ioctl.
 Until then, I want this duct-taped together, and Imre's patch looks
 like a viable approach. Working beats ugly semantics here imo.

Agreed; there's definitely redundancy here, but turning the display on
at mode set time when it was clearly intended and implied by other
usages seems like following the policy of least surprise.

-- 
Jesse Barnes, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

2013-05-03 Thread Chris Wilson 
On Fri, May 03, 2013 at 05:55:31PM +0200, Daniel Vetter wrote:
 On Fri, May 3, 2013 at 2:22 PM, Chris Wilson ch...@chris-wilson.co.uk wrote:
  On Fri, May 03, 2013 at 02:55:45PM +0300, Imre Deak wrote:
  On Fri, 2013-05-03 at 12:44 +0100, Chris Wilson wrote:
   On Fri, May 03, 2013 at 02:22:09PM +0300, Imre Deak wrote:
Currently the driver's assumed behavior for a modeset with an attached
FB is that the corresponding connector will be switched to DPMS ON mode
if it happened to be in DPMS OFF (or another power save mode). This
wasn't enforced though if only the FB changed, everything else (format,
connector etc.) remaining the same. In this case we only set the new FB
base and left the connector in the old power save mode.
   
Fix this by forcing a full modeset whenever there is an attached FB and
any affected connector is in a power save mode.
   
Signed-off-by: Imre Deak imre.d...@intel.com
  
   NAK. Papering over bugs, much?
 
  Ok. I posted this based on our IRC discussion where we seemed to agree
  that DPMS_ON is assumed already; just that it's not done consistently.
  That is if user space does a modeset now that ends up in a full modeset
  (lets say a new FB with different format) then the kernel will do a
  DPMS_ON. But if the new FB happens to be the same format then it won't.
  I thought this is inconsistent and should be fixed.
 
  Another way to make it consistent would be then to remove DPMS ON from
  the full modeset path..
 
  Right, we have mentioned in the past that the conflation between DPMS
  and modesetting is a more recent bug that is going to cause us even more
  trouble later. I am not sure how we can fix that as UXA already makes
  many bad assumptions.
 
 Afaict the conflagration of dpms on after a modeset has been even in
 the old crtc helpers. The only difference is that now we explicitly
 update the dpms state tracking to avoid double-encoder enables. That
 part allowed us to rip out tons of state tracking from tons of places.
 
 So ever since kms happened, we have inconsistent setcrtc semantics.
 Stopping to force dpms on will horribly break existing userspace, so
 we can't do this. Making sure that the semantics are more consistent
 (especially if we further optimize modeset transitions for fastboot)
 seems like the lesser of two evils.

Not quite. It was added afterwards, I know for it is one of my mistakes
that I belatedly recognised as trying to workaround a bug in UXA. And we
then layered on further bugs to try and patch up the glaring holes then
introduced.

 We'll get a 2nd chance to get this right the atomic modeset ioctl.
 Until then, I want this duct-taped together, and Imre's patch looks
 like a viable approach. Working beats ugly semantics here imo.

I think the patch is quite ugly as we do not attempt to avoid the
unnecessary modeset step and so penalise working userspace code.  Now,
if you were first to make intel_modeset_affected_pipes() smarter, such
that the explicit fb set-base optimisation was no longer ever necessary,
things would be more interesting.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

2013-05-03 Thread Daniel Vetter 
On Fri, May 3, 2013 at 6:28 PM, Chris Wilson ch...@chris-wilson.co.uk wrote:
 Not quite. It was added afterwards, I know for it is one of my mistakes
 that I belatedly recognised as trying to workaround a bug in UXA. And we
 then layered on further bugs to try and patch up the glaring holes then
 introduced.

I've done some digging around and found some cool stuff, but no change
which introduce the original bug that we've never consulted dpms state
in drm_crtc_helper_set_mode but just blindly enabled encoders. Most of
the commits I've dug out try to paper over that mismatch by updating
dpms state, too. E.g. bf9dc102 which was then fixed up a bit in
811aaa55 but I don't see any mention of someone explicitly kill dpms
checks.

Of course for DP support we've started to add dpms state checks to our
dp encoder (since double-enable kills DP). Then duplicated the
drm_connector-dpms into our own intel_dp-dpms state since the crtc
helpers liked to frob around with dpms in a rather ill-defined way.
But that's just hilarity and flailing around on top, nothing that I
could see fundamentally changed that a full setcrtc implicitly enabled
the encoders/crtc hw. And since we have our own modeset code that all
disappeared (since a full modeset simply force-updates the dpms state
to reflect reality).

But nowhere have a found a commit to support your claim, just lots of
flailing around due to the ill-defined semantics of this all. Care to
do some more digging?

Thanks, 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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: force full modeset if the connector is in DPMS OFF mode

2013-05-03 Thread Egbert Eich 
On Fri, May 03, 2013 at 02:22:09PM +0300, Imre Deak wrote:
 Currently the driver's assumed behavior for a modeset with an attached
 FB is that the corresponding connector will be switched to DPMS ON mode
 if it happened to be in DPMS OFF (or another power save mode). This
 wasn't enforced though if only the FB changed, everything else (format,
 connector etc.) remaining the same. In this case we only set the new FB
 base and left the connector in the old power save mode.
 
 Fix this by forcing a full modeset whenever there is an attached FB and
 any affected connector is in a power save mode.
 
 Signed-off-by: Imre Deak imre.d...@intel.com

[..]

 @@ -8397,8 +8412,12 @@ intel_set_config_compute_mode_changes(struct 
 drm_mode_set *set,
   } else if (set-fb-pixel_format !=
  set-crtc-fb-pixel_format) {
   config-mode_changed = true;
 - } else
 + } else if (crtc_connector_off(set-crtc, *set-connectors,
 +   set-num_connectors)) {
 + config-mode_changed = true;
 + } else {
   config-fb_changed = true;
 + }
   }
  
   if (set-fb  (set-x != set-crtc-x || set-y != set-crtc-y))

On https://bugs.freedesktop.org/show_bug.cgi?id=64178
I had a similar problem for which I found a very similar 'workaround':

Assuming the Xserver is displaying a mode a on output A, doing a:

xrandr --output A --off
xrandr --output A --mode a

will not light up the screen.

This all sounds quite similar, to the issues describe by this patch,
however the patch as is will not fix this issue: With this setup the
fb does not change. The crtc_connector_off() test however only runs if
set-crtc-fb != set-fb is true.

Thus the connector_off() test needs to happen before:

-   if (set-crtc-fb != set-fb) {
+   if (set-connectors != NULL  
+   connector_off(set-crtc, *set-connectors,
+ set-num_connectors)) {
+   config-mode_changed = true;
+   } else if (set-crtc-fb != set-fb) {

Mind the test for set-connectors != NULL as now the connectors list
may be empty.

Cheers,
Egbert.

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx