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

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

[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.deak@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

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

[Daniel Vetter]

On Fri, May 3, 2013 at 1:22 PM, Imre Deak <imre.deak@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.deak@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

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

[Daniel Vetter]

On Fri, May 3, 2013 at 1:27 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, May 3, 2013 at 1:22 PM, Imre Deak <imre.deak@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.deak@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

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

[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 <daniel@ffwll.ch> wrote:
> > On Fri, May 3, 2013 at 1:22 PM, Imre Deak <imre.deak@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.deak@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

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

[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.deak@intel.com>

NAK. Papering over bugs, much?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jani Nikula]

On Fri, 03 May 2013, Imre Deak <imre.deak@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.deak@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

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

[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.deak@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

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

[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.deak@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

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

[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.deak@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

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

[Daniel Vetter]

On Fri, May 3, 2013 at 2:22 PM, Chris Wilson <chris@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.deak@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

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

[Jesse Barnes]

On Fri, 3 May 2013 17:55:31 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, May 3, 2013 at 2:22 PM, Chris Wilson <chris@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.deak@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

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

[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 <chris@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.deak@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

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

[Daniel Vetter]

On Fri, May 3, 2013 at 6:28 PM, Chris Wilson <chris@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

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

[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.deak@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.

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

[Egbert Eich]

From: Imre Deak <imre.deak@intel.com>

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.

V_2: Run the test for encoders in power save mode outside the the
test for fb change: user space may have just disabled the encoders
but left everything else in place. Make sure the connector list is
not empty before running this test.

Signed-off-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/i915/intel_display.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2939524..992d469 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
+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)
@@ -8387,7 +8402,11 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
 
 	/* We should be able to check here if the fb has the same properties
 	 * and then just flip_or_move it */
-	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) {
 		/* If we have no fb then treat it as a full mode set */
 		if (set->crtc->fb == NULL) {
 			DRM_DEBUG_KMS("crtc has no fb, full mode set\n");
@@ -8397,8 +8416,9 @@ 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 {
 			config->fb_changed = true;
+		}
 	}
 
 	if (set->fb && (set->x != set->crtc->x || set->y != set->crtc->y))
-- 
1.8.1.4

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

[Daniel Vetter]

On Fri, May 3, 2013 at 6:50 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, May 3, 2013 at 6:28 PM, Chris Wilson <chris@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 sensible&consistent option we
have.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Jesse Barnes]

On Fri,  3 May 2013 19:44:07 +0200
Egbert Eich <eich@suse.de> wrote:

> From: Imre Deak <imre.deak@intel.com>
> 
> 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.
> 
> V_2: Run the test for encoders in power save mode outside the the
> test for fb change: user space may have just disabled the encoders
> but left everything else in place. Make sure the connector list is
> not empty before running this test.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2939524..992d469 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
> +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)
> @@ -8387,7 +8402,11 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
>  
>  	/* We should be able to check here if the fb has the same properties
>  	 * and then just flip_or_move it */
> -	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) {
>  		/* If we have no fb then treat it as a full mode set */
>  		if (set->crtc->fb == NULL) {
>  			DRM_DEBUG_KMS("crtc has no fb, full mode set\n");
> @@ -8397,8 +8416,9 @@ 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 {
>  			config->fb_changed = true;
> +		}
>  	}
>  
>  	if (set->fb && (set->x != set->crtc->x || set->y != set->crtc->y))

Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Daniel Vetter]

On Mon, May 20, 2013 at 02:13:22PM -0700, Jesse Barnes wrote:
> On Fri,  3 May 2013 19:44:07 +0200
> Egbert Eich <eich@suse.de> wrote:
> 
> > From: Imre Deak <imre.deak@intel.com>
> > 
> > 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.
> > 
> > V_2: Run the test for encoders in power save mode outside the the
> > test for fb change: user space may have just disabled the encoders
> > but left everything else in place. Make sure the connector list is
> > not empty before running this test.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Egbert Eich <eich@suse.de>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 24 ++++++++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 2939524..992d469 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
> > +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)
> > @@ -8387,7 +8402,11 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
> >  
> >  	/* We should be able to check here if the fb has the same properties
> >  	 * and then just flip_or_move it */
> > -	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) {
> >  		/* If we have no fb then treat it as a full mode set */
> >  		if (set->crtc->fb == NULL) {
> >  			DRM_DEBUG_KMS("crtc has no fb, full mode set\n");
> > @@ -8397,8 +8416,9 @@ 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 {
> >  			config->fb_changed = true;
> > +		}
> >  	}
> >  
> >  	if (set->fb && (set->x != set->crtc->x || set->y != set->crtc->y))
> 
> Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Picked up for -fixes, thanks for the patch. We can bikeshed this later on,
but this patch here at least should patch the worst holes. And maybe we
come up with saner semantics for the new atomic modeset stuff ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch