* [PATCH] drm/i915: Hook the force-restore into the highlevel modeset
@ 2013-06-07 12:47 Chris Wilson
2013-06-07 15:29 ` Daniel Vetter
2013-06-07 19:10 ` Daniel Vetter
0 siblings, 2 replies; 6+ messages in thread
From: Chris Wilson @ 2013-06-07 12:47 UTC (permalink / raw)
To: intel-gfx
On random machines, the BIOS changes the hardware state of the display
when it detects a lid event. In return, we have to then do a forced
restore of the users requested configuration as soon as we handle the
lid event. Currently, this is done by calling the lowlevel CRTC
configuration functions directly with the old state. This results in
a flash on all connected displays. However, since we probe the state of
the hardware left by the BIOS, we can instead hook into the higher level
modeset code which evaluates the minimum changes required to setup the
requested state. This converts the modeset back into a no-op on sane
machines, and eliminates the screen blanking.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=65486
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++--
drivers/gpu/drm/i915/intel_drv.h | 5 +++++
2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ba7e311..5b727a3b2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9632,11 +9632,33 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
* checking (bogus) intermediate states.
*/
for_each_pipe(pipe) {
+ struct drm_mode_set set;
+ struct drm_connector *connector, *connectors[16];
struct drm_crtc *crtc =
dev_priv->pipe_to_crtc_mapping[pipe];
- __intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y,
- crtc->fb);
+ set.crtc = crtc;
+ set.x = crtc->x;
+ set.y = crtc->y;
+ set.mode = crtc->fb ? &crtc->mode : NULL;
+ set.fb = crtc->fb;
+ set.connectors = connectors;
+ set.num_connectors = 0;
+
+ list_for_each_entry(connector,
+ &dev->mode_config.connector_list,
+ head) {
+ if (&intel_attached_crtc(connector)->base != crtc)
+ continue;
+
+ connectors[set.num_connectors++] = connector;
+ if (set.num_connectors == ARRAY_SIZE(connectors))
+ break;
+ }
+
+ if (intel_crtc_set_config(&set))
+ __intel_set_mode(crtc, &crtc->mode,
+ crtc->x, crtc->y, crtc->fb);
}
list_for_each_entry(plane, &dev->mode_config.plane_list, head)
intel_plane_restore(plane);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1f04b7f..ef3f809 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -648,6 +648,11 @@ static inline struct intel_encoder *intel_attached_encoder(struct drm_connector
return to_intel_connector(connector)->encoder;
}
+static inline struct intel_crtc *intel_attached_crtc(struct drm_connector *connector)
+{
+ return to_intel_crtc(to_intel_connector(connector)->encoder->base.crtc);
+}
+
static inline struct intel_digital_port *
enc_to_dig_port(struct drm_encoder *encoder)
{
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Hook the force-restore into the highlevel modeset
2013-06-07 12:47 [PATCH] drm/i915: Hook the force-restore into the highlevel modeset Chris Wilson
@ 2013-06-07 15:29 ` Daniel Vetter
2013-06-07 19:10 ` Daniel Vetter
1 sibling, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2013-06-07 15:29 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Fri, Jun 7, 2013 at 2:47 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On random machines, the BIOS changes the hardware state of the display
> when it detects a lid event. In return, we have to then do a forced
> restore of the users requested configuration as soon as we handle the
> lid event. Currently, this is done by calling the lowlevel CRTC
> configuration functions directly with the old state. This results in
> a flash on all connected displays. However, since we probe the state of
> the hardware left by the BIOS, we can instead hook into the higher level
> modeset code which evaluates the minimum changes required to setup the
> requested state. This converts the modeset back into a no-op on sane
> machines, and eliminates the screen blanking.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=65486
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Hm, my Grand Plan was to actually move into the other direction, i.e.
shovel all the modeset compare logic into the lower levels (that's
what the different pipe masks have been for) and in addition switch
from comparing a few important things like the requested
mode/fb->pixel format to comparing computed pipe configs. That way we
can fully subsume fastboot into any other modeset (and also subsume
such "fixup bios damage" like this one).
Essentially my plan is to pimp intel_modeset_affected_pipes to not
just look at the output routing, but also compare the pipe config.
That means we need to move it past the pipe config compute stage and
also have a crtc->new_config pointer for each crtc (pointing to the
old one for all other crtcs). Then we could drop the HACK at the end
of intel_modeset_affected_pipes and instead just compare pipe configs.
Problem is that we still lack a few essential pieces to not miss a
modeset update. To paper over that we could use the recently added
pipe_config->quirk field and set the PIPE_CONFIG_QUIRK_NO_FASTBOOT
flag in intel_set_mode. The lid updater here only calls
__intel_set_mode so would get the good treatment already. And
intel_pipe_config_compare would also need to grow some smarts (and
maybe a flag parameter to control a few things betweent the fast
modeset compare mode and the check state compare mode.
Anyway, just wanted to dump my ideas about this, your patch here looks
simple enough as a short-term stopgap measure.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++--
> drivers/gpu/drm/i915/intel_drv.h | 5 +++++
> 2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ba7e311..5b727a3b2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9632,11 +9632,33 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> * checking (bogus) intermediate states.
> */
> for_each_pipe(pipe) {
> + struct drm_mode_set set;
> + struct drm_connector *connector, *connectors[16];
> struct drm_crtc *crtc =
> dev_priv->pipe_to_crtc_mapping[pipe];
>
> - __intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y,
> - crtc->fb);
> + set.crtc = crtc;
> + set.x = crtc->x;
> + set.y = crtc->y;
> + set.mode = crtc->fb ? &crtc->mode : NULL;
> + set.fb = crtc->fb;
> + set.connectors = connectors;
> + set.num_connectors = 0;
> +
> + list_for_each_entry(connector,
> + &dev->mode_config.connector_list,
> + head) {
> + if (&intel_attached_crtc(connector)->base != crtc)
> + continue;
> +
> + connectors[set.num_connectors++] = connector;
> + if (set.num_connectors == ARRAY_SIZE(connectors))
> + break;
> + }
> +
> + if (intel_crtc_set_config(&set))
> + __intel_set_mode(crtc, &crtc->mode,
> + crtc->x, crtc->y, crtc->fb);
> }
> list_for_each_entry(plane, &dev->mode_config.plane_list, head)
> intel_plane_restore(plane);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1f04b7f..ef3f809 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -648,6 +648,11 @@ static inline struct intel_encoder *intel_attached_encoder(struct drm_connector
> return to_intel_connector(connector)->encoder;
> }
>
> +static inline struct intel_crtc *intel_attached_crtc(struct drm_connector *connector)
> +{
> + return to_intel_crtc(to_intel_connector(connector)->encoder->base.crtc);
> +}
> +
> static inline struct intel_digital_port *
> enc_to_dig_port(struct drm_encoder *encoder)
> {
> --
> 1.7.10.4
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Hook the force-restore into the highlevel modeset
2013-06-07 12:47 [PATCH] drm/i915: Hook the force-restore into the highlevel modeset Chris Wilson
2013-06-07 15:29 ` Daniel Vetter
@ 2013-06-07 19:10 ` Daniel Vetter
2013-06-07 19:14 ` Chris Wilson
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2013-06-07 19:10 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Fri, Jun 07, 2013 at 01:47:41PM +0100, Chris Wilson wrote:
> On random machines, the BIOS changes the hardware state of the display
> when it detects a lid event. In return, we have to then do a forced
> restore of the users requested configuration as soon as we handle the
> lid event. Currently, this is done by calling the lowlevel CRTC
> configuration functions directly with the old state. This results in
> a flash on all connected displays. However, since we probe the state of
> the hardware left by the BIOS, we can instead hook into the higher level
> modeset code which evaluates the minimum changes required to setup the
> requested state. This converts the modeset back into a no-op on sane
> machines, and eliminates the screen blanking.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=65486
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++--
> drivers/gpu/drm/i915/intel_drv.h | 5 +++++
> 2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ba7e311..5b727a3b2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9632,11 +9632,33 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> * checking (bogus) intermediate states.
> */
> for_each_pipe(pipe) {
> + struct drm_mode_set set;
> + struct drm_connector *connector, *connectors[16];
> struct drm_crtc *crtc =
> dev_priv->pipe_to_crtc_mapping[pipe];
>
> - __intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y,
> - crtc->fb);
> + set.crtc = crtc;
> + set.x = crtc->x;
> + set.y = crtc->y;
> + set.mode = crtc->fb ? &crtc->mode : NULL;
> + set.fb = crtc->fb;
> + set.connectors = connectors;
> + set.num_connectors = 0;
> +
> + list_for_each_entry(connector,
> + &dev->mode_config.connector_list,
> + head) {
> + if (&intel_attached_crtc(connector)->base != crtc)
> + continue;
> +
> + connectors[set.num_connectors++] = connector;
> + if (set.num_connectors == ARRAY_SIZE(connectors))
> + break;
> + }
> +
> + if (intel_crtc_set_config(&set))
> + __intel_set_mode(crtc, &crtc->mode,
> + crtc->x, crtc->y, crtc->fb);
Won't this horribly scream in the modeset state checker again if we've
only partially restored the output state (in case the bios was really
seriuos about killing our config)?
-Daniel
> }
> list_for_each_entry(plane, &dev->mode_config.plane_list, head)
> intel_plane_restore(plane);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1f04b7f..ef3f809 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -648,6 +648,11 @@ static inline struct intel_encoder *intel_attached_encoder(struct drm_connector
> return to_intel_connector(connector)->encoder;
> }
>
> +static inline struct intel_crtc *intel_attached_crtc(struct drm_connector *connector)
> +{
> + return to_intel_crtc(to_intel_connector(connector)->encoder->base.crtc);
> +}
> +
> static inline struct intel_digital_port *
> enc_to_dig_port(struct drm_encoder *encoder)
> {
> --
> 1.7.10.4
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Hook the force-restore into the highlevel modeset
2013-06-07 19:10 ` Daniel Vetter
@ 2013-06-07 19:14 ` Chris Wilson
2013-06-07 19:32 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2013-06-07 19:14 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Fri, Jun 07, 2013 at 09:10:23PM +0200, Daniel Vetter wrote:
> On Fri, Jun 07, 2013 at 01:47:41PM +0100, Chris Wilson wrote:
> > + if (intel_crtc_set_config(&set))
> > + __intel_set_mode(crtc, &crtc->mode,
> > + crtc->x, crtc->y, crtc->fb);
>
> Won't this horribly scream in the modeset state checker again if we've
> only partially restored the output state (in case the bios was really
> seriuos about killing our config)?
I thought that was going to be a useful scream. It should mean that we
have we missing bits in the pipe_config recovery, right?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Hook the force-restore into the highlevel modeset
2013-06-07 19:14 ` Chris Wilson
@ 2013-06-07 19:32 ` Daniel Vetter
2013-06-07 19:50 ` Chris Wilson
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2013-06-07 19:32 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, intel-gfx
On Fri, Jun 7, 2013 at 9:14 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Jun 07, 2013 at 09:10:23PM +0200, Daniel Vetter wrote:
>> On Fri, Jun 07, 2013 at 01:47:41PM +0100, Chris Wilson wrote:
>> > + if (intel_crtc_set_config(&set))
>> > + __intel_set_mode(crtc, &crtc->mode,
>> > + crtc->x, crtc->y, crtc->fb);
>>
>> Won't this horribly scream in the modeset state checker again if we've
>> only partially restored the output state (in case the bios was really
>> seriuos about killing our config)?
>
> I thought that was going to be a useful scream. It should mean that we
> have we missing bits in the pipe_config recovery, right?
Nope, we've had to rework it to only check at the very end of the
force_restore operation, since we still lack an atomic modeset:
commit f30da187cdcd0939288038e11fb3bfbd1b655564
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date: Thu Apr 11 20:22:50 2013 +0200
drm/i915: don't check inconsistent modeset state when force-restoring
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Hook the force-restore into the highlevel modeset
2013-06-07 19:32 ` Daniel Vetter
@ 2013-06-07 19:50 ` Chris Wilson
0 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2013-06-07 19:50 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Fri, Jun 07, 2013 at 09:32:33PM +0200, Daniel Vetter wrote:
> On Fri, Jun 7, 2013 at 9:14 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Fri, Jun 07, 2013 at 09:10:23PM +0200, Daniel Vetter wrote:
> >> On Fri, Jun 07, 2013 at 01:47:41PM +0100, Chris Wilson wrote:
> >> > + if (intel_crtc_set_config(&set))
> >> > + __intel_set_mode(crtc, &crtc->mode,
> >> > + crtc->x, crtc->y, crtc->fb);
> >>
> >> Won't this horribly scream in the modeset state checker again if we've
> >> only partially restored the output state (in case the bios was really
> >> seriuos about killing our config)?
> >
> > I thought that was going to be a useful scream. It should mean that we
> > have we missing bits in the pipe_config recovery, right?
>
> Nope, we've had to rework it to only check at the very end of the
> force_restore operation, since we still lack an atomic modeset:
Well that's a bit naff. Not the simple solution I hoped for. Your turn.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-06-07 19:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-07 12:47 [PATCH] drm/i915: Hook the force-restore into the highlevel modeset Chris Wilson
2013-06-07 15:29 ` Daniel Vetter
2013-06-07 19:10 ` Daniel Vetter
2013-06-07 19:14 ` Chris Wilson
2013-06-07 19:32 ` Daniel Vetter
2013-06-07 19:50 ` Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.