Skip to content

Commit

Permalink
Merge the modeset-rework, basic conversion into drm-intel-next
Browse files Browse the repository at this point in the history
As a quick reference I'll detail the motivation and design of the new code a
bit here (mostly stitched together from patchbomb announcements and commits
introducing the new concepts).

The crtc helper code has the fundamental assumption that encoders and crtcs can
be enabled/disabled in any order, as long as we take care of depencies (which
means that enabled encoders need an enabled crtc to feed them data,
essentially).

Our hw works differently. We already have tons of ugly cases where crtc code
enables encoder hw (or encoder->mode_set enables stuff that should only be
enabled in enocder->commit) to work around these issues. But on the disable
side we can't pull off similar tricks - there we actually need to rework the
modeset sequence that controls all this. And this is also the real motivation
why I've finally undertaken this rewrite: eDP on my shiny new Ivybridge
Ultrabook is broken, and it's broken due to the wrong disable sequence ...

The new code introduces a few interfaces and concepts:

- Add new encoder->enable/disable functions which are directly called from the
crtc->enable/disable function. This ensures that the encoder's can be
enabled/disabled at a very specific in the modeset sequence, controlled by our
platform specific code (instead of the crtc helper code calling them at a time
it deems convenient).

- Rework the dpms code - our code has mostly 1:1 connector:encoder mappings and
does support cloning on only a few encoders, so we can simplify things quite a
bit.

- Also only ever disable/enable the entire output pipeline. This ensures that
we obey the right sequence of enabling/disabling things, trying to be clever
here mostly just complicates the code and results in bugs. For cloneable
encoders this requires a bit of special handling to ensure that outputs can
still be disabled individually, but it simplifies the common case.

- Add infrastructure to read out the current hw state. No amount of careful
ordering will help us if we brick the hw on the initial modeset setup. Which
could happen if we just randomly disable things, oblivious to the state set up
by the bios. Hence we need to be able to read that out. As a benefit, we grow a
few generic functions useful to cross-check our modeset code with actual hw
state.

With all this in place, we can copy&paste the crtc helper code into the
drm/i915 driver and start to rework it:

- As detailed above, the new code only disables/enables an entire output pipe.
As a preparation for global mode-changes (e.g. reassigning shared resources) it
keeps track of which pipes need to be touched by a set of bitmasks.

- To ensure that we correctly disable the current display pipes, we need to
know the currently active connector/encoder/crtc linking. The old crtc helper
simply overwrote these links with the new setup, the new code stages the new
links in ->new_* pointers. Those get commited to the real linking pointers once
the old output configuration has been torn down, before the ->mode_set
callbacks are called.

- Finally the code adds tons of self-consistency checks by employing the new hw
state readout functions to cross-check the actual hw state with what the
datastructure think it should be. These checks are done both after every
modeset and after the hw state has been read out and sanitized at boot/resume
time. All these checks greatly helped in tracking down regressions and bugs in
the new code.

With this new basis, a lot of cleanups and improvements to the code are now
possible (besides the DP fixes that ultimately made me write this), but not yet
done:

- I think we should create struct intel_mode and use it as the adjusted mode
everywhere to store little pieces like needs_tvclock, pipe dithering values or
dp link parameters. That would still be a layering violation, but at least we
wouldn't need to recompute these kinds of things in intel_display.c. Especially
the port bpc computation needed for selecting the pipe bpc and dithering
settings in intel_display.c is rather gross.

- In a related rework we could implement ->mode_valid in terms of ->mode_fixup
in a generic way - I've hunted down too many bugs where ->mode_valid did the
right thing, but ->mode_fixup didn't. Or vice versa, resulting in funny bugs
for user-supplied modes.

- Ditch the idea to rework the hdp handling in the common crtc helper code and
just move things to i915.ko. Which would rid us of the ->detect crtc helper
dependencies.

- LVDS wire pair and pll enabling is all done in the crtc->mode_set function
currently. We should be able to move this to the crtc_enable callbacks (or in
the case of the LVDS wire pair enabling, into some encoder callback).

Last, but not least, this new code should also help in enabling a few neat
features: The hw state readout code prepares (but there are still big pieces
missing) for fastboot, i.e. avoiding the inital modeset at boot-up and just
taking over the configuration left behind by the bios. We also should be able
to extend the configuration checks in the beginning of the modeset sequence and
make better decisions about shared resources (which is the entire point behind
the atomic/global modeset ioctl).

Tested-by: Jani Nikula <jani.nikula@intel.com>
Tested-by: Ben Widawsky <ben@bwidawsk.net>
Tested-by: Damien Lespiau <damien.lespiau@intel.com>
Tested-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
Tested-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
Acked-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Acked-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Tested-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
  • Loading branch information
Daniel Vetter committed Sep 6, 2012
2 parents 8c3f929 + b980514 commit a1ceb67
Show file tree
Hide file tree
Showing 20 changed files with 2,075 additions and 530 deletions.
6 changes: 6 additions & 0 deletions drivers/gpu/drm/i915/dvo.h
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,12 @@ struct intel_dvo_dev_ops {
*/
enum drm_connector_status (*detect)(struct intel_dvo_device *dvo);

/*
* Probe the current hw status, returning true if the connected output
* is active.
*/
bool (*get_hw_state)(struct intel_dvo_device *dev);

/**
* Query the device for the modes it provides.
*
Expand Down
13 changes: 13 additions & 0 deletions drivers/gpu/drm/i915/dvo_ch7017.c
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,18 @@ static void ch7017_dpms(struct intel_dvo_device *dvo, bool enable)
msleep(20);
}

static bool ch7017_get_hw_state(struct intel_dvo_device *dvo)
{
uint8_t val;

ch7017_read(dvo, CH7017_LVDS_POWER_DOWN, &val);

if (val & CH7017_LVDS_POWER_DOWN_EN)
return false;
else
return true;
}

static void ch7017_dump_regs(struct intel_dvo_device *dvo)
{
uint8_t val;
Expand Down Expand Up @@ -396,6 +408,7 @@ struct intel_dvo_dev_ops ch7017_ops = {
.mode_valid = ch7017_mode_valid,
.mode_set = ch7017_mode_set,
.dpms = ch7017_dpms,
.get_hw_state = ch7017_get_hw_state,
.dump_regs = ch7017_dump_regs,
.destroy = ch7017_destroy,
};
13 changes: 13 additions & 0 deletions drivers/gpu/drm/i915/dvo_ch7xxx.c
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,18 @@ static void ch7xxx_dpms(struct intel_dvo_device *dvo, bool enable)
ch7xxx_writeb(dvo, CH7xxx_PM, CH7xxx_PM_FPD);
}

static bool ch7xxx_get_hw_state(struct intel_dvo_device *dvo)
{
u8 val;

ch7xxx_readb(dvo, CH7xxx_PM, &val);

if (val & CH7xxx_PM_FPD)
return false;
else
return true;
}

static void ch7xxx_dump_regs(struct intel_dvo_device *dvo)
{
int i;
Expand Down Expand Up @@ -326,6 +338,7 @@ struct intel_dvo_dev_ops ch7xxx_ops = {
.mode_valid = ch7xxx_mode_valid,
.mode_set = ch7xxx_mode_set,
.dpms = ch7xxx_dpms,
.get_hw_state = ch7xxx_get_hw_state,
.dump_regs = ch7xxx_dump_regs,
.destroy = ch7xxx_destroy,
};
15 changes: 15 additions & 0 deletions drivers/gpu/drm/i915/dvo_ivch.c
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,20 @@ static void ivch_dpms(struct intel_dvo_device *dvo, bool enable)
udelay(16 * 1000);
}

static bool ivch_get_hw_state(struct intel_dvo_device *dvo)
{
uint16_t vr01;

/* Set the new power state of the panel. */
if (!ivch_read(dvo, VR01, &vr01))
return false;

if (vr01 & VR01_LCD_ENABLE)
return true;
else
return false;
}

static void ivch_mode_set(struct intel_dvo_device *dvo,
struct drm_display_mode *mode,
struct drm_display_mode *adjusted_mode)
Expand Down Expand Up @@ -413,6 +427,7 @@ static void ivch_destroy(struct intel_dvo_device *dvo)
struct intel_dvo_dev_ops ivch_ops = {
.init = ivch_init,
.dpms = ivch_dpms,
.get_hw_state = ivch_get_hw_state,
.mode_valid = ivch_mode_valid,
.mode_set = ivch_mode_set,
.detect = ivch_detect,
Expand Down
15 changes: 15 additions & 0 deletions drivers/gpu/drm/i915/dvo_ns2501.c
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,20 @@ static void ns2501_mode_set(struct intel_dvo_device *dvo,
restore_dvo(dvo);
}

/* set the NS2501 power state */
static bool ns2501_get_hw_state(struct intel_dvo_device *dvo)
{
unsigned char ch;

if (!ns2501_readb(dvo, NS2501_REG8, &ch))
return false;

if (ch & NS2501_8_PD)
return true;
else
return false;
}

/* set the NS2501 power state */
static void ns2501_dpms(struct intel_dvo_device *dvo, bool enable)
{
Expand Down Expand Up @@ -568,6 +582,7 @@ struct intel_dvo_dev_ops ns2501_ops = {
.mode_valid = ns2501_mode_valid,
.mode_set = ns2501_mode_set,
.dpms = ns2501_dpms,
.get_hw_state = ns2501_get_hw_state,
.dump_regs = ns2501_dump_regs,
.destroy = ns2501_destroy,
};
16 changes: 16 additions & 0 deletions drivers/gpu/drm/i915/dvo_sil164.c
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,21 @@ static void sil164_dpms(struct intel_dvo_device *dvo, bool enable)
return;
}

static bool sil164_get_hw_state(struct intel_dvo_device *dvo)
{
int ret;
unsigned char ch;

ret = sil164_readb(dvo, SIL164_REG8, &ch);
if (ret == false)
return false;

if (ch & SIL164_8_PD)
return true;
else
return false;
}

static void sil164_dump_regs(struct intel_dvo_device *dvo)
{
uint8_t val;
Expand Down Expand Up @@ -258,6 +273,7 @@ struct intel_dvo_dev_ops sil164_ops = {
.mode_valid = sil164_mode_valid,
.mode_set = sil164_mode_set,
.dpms = sil164_dpms,
.get_hw_state = sil164_get_hw_state,
.dump_regs = sil164_dump_regs,
.destroy = sil164_destroy,
};
14 changes: 14 additions & 0 deletions drivers/gpu/drm/i915/dvo_tfp410.c
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,19 @@ static void tfp410_dpms(struct intel_dvo_device *dvo, bool enable)
tfp410_writeb(dvo, TFP410_CTL_1, ctl1);
}

static bool tfp410_get_hw_state(struct intel_dvo_device *dvo)
{
uint8_t ctl1;

if (!tfp410_readb(dvo, TFP410_CTL_1, &ctl1))
return false;

if (ctl1 & TFP410_CTL_1_PD)
return true;
else
return false;
}

static void tfp410_dump_regs(struct intel_dvo_device *dvo)
{
uint8_t val, val2;
Expand Down Expand Up @@ -299,6 +312,7 @@ struct intel_dvo_dev_ops tfp410_ops = {
.mode_valid = tfp410_mode_valid,
.mode_set = tfp410_mode_set,
.dpms = tfp410_dpms,
.get_hw_state = tfp410_get_hw_state,
.dump_regs = tfp410_dump_regs,
.destroy = tfp410_destroy,
};
9 changes: 4 additions & 5 deletions drivers/gpu/drm/i915/i915_drv.c
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,9 @@ static int i915_drm_freeze(struct drm_device *dev)
"GEM idle failed, resume might fail\n");
return error;
}

intel_modeset_disable(dev);

drm_irq_uninstall(dev);
}

Expand Down Expand Up @@ -543,13 +546,9 @@ static int i915_drm_thaw(struct drm_device *dev)
mutex_unlock(&dev->struct_mutex);

intel_modeset_init_hw(dev);
intel_modeset_setup_hw_state(dev);
drm_mode_config_reset(dev);
drm_irq_install(dev);

/* Resume the modeset for every activated CRTC */
mutex_lock(&dev->mode_config.mutex);
drm_helper_resume_force_mode(dev);
mutex_unlock(&dev->mode_config.mutex);
}

intel_opregion_init(dev);
Expand Down
4 changes: 3 additions & 1 deletion drivers/gpu/drm/i915/i915_drv.h
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,6 @@ struct drm_i915_error_state {
};

struct drm_i915_display_funcs {
void (*dpms)(struct drm_crtc *crtc, int mode);
bool (*fbc_enabled)(struct drm_device *dev);
void (*enable_fbc)(struct drm_crtc *crtc, unsigned long interval);
void (*disable_fbc)(struct drm_device *dev);
Expand All @@ -257,6 +256,8 @@ struct drm_i915_display_funcs {
struct drm_display_mode *adjusted_mode,
int x, int y,
struct drm_framebuffer *old_fb);
void (*crtc_enable)(struct drm_crtc *crtc);
void (*crtc_disable)(struct drm_crtc *crtc);
void (*off)(struct drm_crtc *crtc);
void (*write_eld)(struct drm_connector *connector,
struct drm_crtc *crtc);
Expand Down Expand Up @@ -1550,6 +1551,7 @@ extern void intel_modeset_init(struct drm_device *dev);
extern void intel_modeset_gem_init(struct drm_device *dev);
extern void intel_modeset_cleanup(struct drm_device *dev);
extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state);
extern void intel_modeset_setup_hw_state(struct drm_device *dev);
extern bool intel_fbc_enabled(struct drm_device *dev);
extern void intel_disable_fbc(struct drm_device *dev);
extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
Expand Down
2 changes: 2 additions & 0 deletions drivers/gpu/drm/i915/i915_reg.h
Original file line number Diff line number Diff line change
Expand Up @@ -4037,6 +4037,8 @@
#define PORT_TRANS_C_SEL_CPT (2<<29)
#define PORT_TRANS_SEL_MASK (3<<29)
#define PORT_TRANS_SEL_CPT(pipe) ((pipe) << 29)
#define PORT_TO_PIPE(val) (((val) & (1<<30)) >> 30)
#define PORT_TO_PIPE_CPT(val) (((val) & PORT_TRANS_SEL_MASK) >> 29)

#define TRANS_DP_CTL_A 0xe0300
#define TRANS_DP_CTL_B 0xe1300
Expand Down
Loading

0 comments on commit a1ceb67

Please sign in to comment.