From 11ee9615f9bbc9c0c2dbd9f5eb275459b76f032a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= Date: Thu, 28 May 2015 18:32:36 +0300 Subject: [PATCH 1/7] drm/i915: Don't skip request retirement if the active list is empty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apparently we can have requests even if though the active list is empty, so do the request retirement regardless of whether there's anything on the active list. The way it happened here is that during suspend intel_ring_idle() notices the olr hanging around and then proceeds to get rid of it by adding a request. However since there was nothing on the active lists i915_gem_retire_requests() didn't clean those up, and so the idle work never runs, and we leave the GPU "busy" during suspend resulting in a WARN later. Signed-off-by: Ville Syrjälä Reviewed-by: Chris Wilson Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/i915_gem.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index be35f0486202..c35e0352b77f 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2813,9 +2813,6 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring) { WARN_ON(i915_verify_lists(ring->dev)); - if (list_empty(&ring->active_list)) - return; - /* Retire requests first as we use it above for the early return. * If we retire requests last, we may use a later seqno and so clear * the requests lists without clearing the active list, leading to From 8a389cac1fd7dd6411c7e586ab098938a75870e0 Mon Sep 17 00:00:00 2001 From: Francisco Jerez Date: Fri, 29 May 2015 16:44:13 +0300 Subject: [PATCH 2/7] drm/i915: Fix command parser to validate multiple register access with the same command. Until now the software command checker assumed that commands could read or write at most a single register per packet. This is not necessarily the case, MI_LOAD_REGISTER_IMM expects a variable-length list of offset/value pairs and writes them in sequence. The previous code would only check whether the first entry was valid, effectively allowing userspace to write unrestricted registers of the MMIO space by sending a multi-register write with a legal first register, with potential security implications on Gen6 and 7 hardware. Fix it by extending the drm_i915_cmd_descriptor table to represent multi-register access and making validate_cmd() iterate for all register offsets present in the command packet. Signed-off-by: Francisco Jerez Reviewed-by: Zhigang Gong Signed-off-by: Daniel Vetter Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/i915_cmd_parser.c | 74 +++++++++++++++----------- drivers/gpu/drm/i915/i915_drv.h | 5 ++ 2 files changed, 48 insertions(+), 31 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c index 9605ff8f2fcd..5fc49bbcdb9d 100644 --- a/drivers/gpu/drm/i915/i915_cmd_parser.c +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c @@ -123,7 +123,7 @@ static const struct drm_i915_cmd_descriptor common_cmds[] = { CMD( MI_SEMAPHORE_MBOX, SMI, !F, 0xFF, R ), CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF, R ), CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, W, - .reg = { .offset = 1, .mask = 0x007FFFFC } ), + .reg = { .offset = 1, .mask = 0x007FFFFC, .step = 2 } ), CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF, W | B, .reg = { .offset = 1, .mask = 0x007FFFFC }, .bits = {{ @@ -934,7 +934,7 @@ bool i915_needs_cmd_parser(struct intel_engine_cs *ring) static bool check_cmd(const struct intel_engine_cs *ring, const struct drm_i915_cmd_descriptor *desc, - const u32 *cmd, + const u32 *cmd, u32 length, const bool is_master, bool *oacontrol_set) { @@ -950,38 +950,49 @@ static bool check_cmd(const struct intel_engine_cs *ring, } if (desc->flags & CMD_DESC_REGISTER) { - u32 reg_addr = cmd[desc->reg.offset] & desc->reg.mask; - /* - * OACONTROL requires some special handling for writes. We - * want to make sure that any batch which enables OA also - * disables it before the end of the batch. The goal is to - * prevent one process from snooping on the perf data from - * another process. To do that, we need to check the value - * that will be written to the register. Hence, limit - * OACONTROL writes to only MI_LOAD_REGISTER_IMM commands. + * Get the distance between individual register offset + * fields if the command can perform more than one + * access at a time. */ - if (reg_addr == OACONTROL) { - if (desc->cmd.value == MI_LOAD_REGISTER_MEM) { - DRM_DEBUG_DRIVER("CMD: Rejected LRM to OACONTROL\n"); - return false; + const u32 step = desc->reg.step ? desc->reg.step : length; + u32 offset; + + for (offset = desc->reg.offset; offset < length; + offset += step) { + const u32 reg_addr = cmd[offset] & desc->reg.mask; + + /* + * OACONTROL requires some special handling for + * writes. We want to make sure that any batch which + * enables OA also disables it before the end of the + * batch. The goal is to prevent one process from + * snooping on the perf data from another process. To do + * that, we need to check the value that will be written + * to the register. Hence, limit OACONTROL writes to + * only MI_LOAD_REGISTER_IMM commands. + */ + if (reg_addr == OACONTROL) { + if (desc->cmd.value == MI_LOAD_REGISTER_MEM) { + DRM_DEBUG_DRIVER("CMD: Rejected LRM to OACONTROL\n"); + return false; + } + + if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1)) + *oacontrol_set = (cmd[offset + 1] != 0); } - if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1)) - *oacontrol_set = (cmd[2] != 0); - } - - if (!valid_reg(ring->reg_table, - ring->reg_count, reg_addr)) { - if (!is_master || - !valid_reg(ring->master_reg_table, - ring->master_reg_count, - reg_addr)) { - DRM_DEBUG_DRIVER("CMD: Rejected register 0x%08X in command: 0x%08X (ring=%d)\n", - reg_addr, - *cmd, - ring->id); - return false; + if (!valid_reg(ring->reg_table, + ring->reg_count, reg_addr)) { + if (!is_master || + !valid_reg(ring->master_reg_table, + ring->master_reg_count, + reg_addr)) { + DRM_DEBUG_DRIVER("CMD: Rejected register 0x%08X in command: 0x%08X (ring=%d)\n", + reg_addr, *cmd, + ring->id); + return false; + } } } } @@ -1105,7 +1116,8 @@ int i915_parse_cmds(struct intel_engine_cs *ring, break; } - if (!check_cmd(ring, desc, cmd, is_master, &oacontrol_set)) { + if (!check_cmd(ring, desc, cmd, length, is_master, + &oacontrol_set)) { ret = -EINVAL; break; } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 72f5a3f9dbf2..542fac628b28 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2300,10 +2300,15 @@ struct drm_i915_cmd_descriptor { * Describes where to find a register address in the command to check * against the ring's register whitelist. Only valid if flags has the * CMD_DESC_REGISTER bit set. + * + * A non-zero step value implies that the command may access multiple + * registers in sequence (e.g. LRI), in that case step gives the + * distance in dwords between individual offset fields. */ struct { u32 offset; u32 mask; + u32 step; } reg; #define MAX_CMD_DESC_BITMASKS 3 From c1091b25f547d45ffdcb443a3790aed652fb9b48 Mon Sep 17 00:00:00 2001 From: Francisco Jerez Date: Fri, 29 May 2015 16:44:14 +0300 Subject: [PATCH 3/7] drm/i915: Extend the parser to check register writes against a mask/value pair. In some cases it might be unnecessary or dangerous to give userspace the right to write arbitrary values to some register, even though it might be desirable to give it control of some of its bits. This patch extends the register whitelist entries to contain a mask/value pair in addition to the register offset. For registers with non-zero mask, any LRM writes and LRI writes where the bits of the immediate given by the mask don't match the specified value will be rejected. This will be used in my next patch to grant userspace partial write access to some sensitive registers. Signed-off-by: Francisco Jerez Reviewed-by: Zhigang Gong Signed-off-by: Daniel Vetter Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/i915_cmd_parser.c | 138 ++++++++++++++++-------- drivers/gpu/drm/i915/intel_ringbuffer.h | 5 +- 2 files changed, 96 insertions(+), 47 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c index 5fc49bbcdb9d..cafa3e2f16fc 100644 --- a/drivers/gpu/drm/i915/i915_cmd_parser.c +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c @@ -395,16 +395,38 @@ static const struct drm_i915_cmd_table hsw_blt_ring_cmds[] = { /* * Register whitelists, sorted by increasing register offset. + */ + +/* + * An individual whitelist entry granting access to register addr. If + * mask is non-zero the argument of immediate register writes will be + * AND-ed with mask, and the command will be rejected if the result + * doesn't match value. + * + * Registers with non-zero mask are only allowed to be written using + * LRI. + */ +struct drm_i915_reg_descriptor { + u32 addr; + u32 mask; + u32 value; +}; + +/* Convenience macro for adding 32-bit registers. */ +#define REG32(address, ...) \ + { .addr = address, __VA_ARGS__ } + +/* + * Convenience macro for adding 64-bit registers. * * Some registers that userspace accesses are 64 bits. The register * access commands only allow 32-bit accesses. Hence, we have to include * entries for both halves of the 64-bit registers. */ +#define REG64(addr) \ + REG32(addr), REG32(addr + sizeof(u32)) -/* Convenience macro for adding 64-bit registers */ -#define REG64(addr) (addr), (addr + sizeof(u32)) - -static const u32 gen7_render_regs[] = { +static const struct drm_i915_reg_descriptor gen7_render_regs[] = { REG64(GPGPU_THREADS_DISPATCHED), REG64(HS_INVOCATION_COUNT), REG64(DS_INVOCATION_COUNT), @@ -417,15 +439,15 @@ static const u32 gen7_render_regs[] = { REG64(CL_PRIMITIVES_COUNT), REG64(PS_INVOCATION_COUNT), REG64(PS_DEPTH_COUNT), - OACONTROL, /* Only allowed for LRI and SRM. See below. */ + REG32(OACONTROL), /* Only allowed for LRI and SRM. See below. */ REG64(MI_PREDICATE_SRC0), REG64(MI_PREDICATE_SRC1), - GEN7_3DPRIM_END_OFFSET, - GEN7_3DPRIM_START_VERTEX, - GEN7_3DPRIM_VERTEX_COUNT, - GEN7_3DPRIM_INSTANCE_COUNT, - GEN7_3DPRIM_START_INSTANCE, - GEN7_3DPRIM_BASE_VERTEX, + REG32(GEN7_3DPRIM_END_OFFSET), + REG32(GEN7_3DPRIM_START_VERTEX), + REG32(GEN7_3DPRIM_VERTEX_COUNT), + REG32(GEN7_3DPRIM_INSTANCE_COUNT), + REG32(GEN7_3DPRIM_START_INSTANCE), + REG32(GEN7_3DPRIM_BASE_VERTEX), REG64(GEN7_SO_NUM_PRIMS_WRITTEN(0)), REG64(GEN7_SO_NUM_PRIMS_WRITTEN(1)), REG64(GEN7_SO_NUM_PRIMS_WRITTEN(2)), @@ -434,33 +456,34 @@ static const u32 gen7_render_regs[] = { REG64(GEN7_SO_PRIM_STORAGE_NEEDED(1)), REG64(GEN7_SO_PRIM_STORAGE_NEEDED(2)), REG64(GEN7_SO_PRIM_STORAGE_NEEDED(3)), - GEN7_SO_WRITE_OFFSET(0), - GEN7_SO_WRITE_OFFSET(1), - GEN7_SO_WRITE_OFFSET(2), - GEN7_SO_WRITE_OFFSET(3), - GEN7_L3SQCREG1, - GEN7_L3CNTLREG2, - GEN7_L3CNTLREG3, + REG32(GEN7_SO_WRITE_OFFSET(0)), + REG32(GEN7_SO_WRITE_OFFSET(1)), + REG32(GEN7_SO_WRITE_OFFSET(2)), + REG32(GEN7_SO_WRITE_OFFSET(3)), + REG32(GEN7_L3SQCREG1), + REG32(GEN7_L3CNTLREG2), + REG32(GEN7_L3CNTLREG3), }; -static const u32 gen7_blt_regs[] = { - BCS_SWCTRL, +static const struct drm_i915_reg_descriptor gen7_blt_regs[] = { + REG32(BCS_SWCTRL), }; -static const u32 ivb_master_regs[] = { - FORCEWAKE_MT, - DERRMR, - GEN7_PIPE_DE_LOAD_SL(PIPE_A), - GEN7_PIPE_DE_LOAD_SL(PIPE_B), - GEN7_PIPE_DE_LOAD_SL(PIPE_C), +static const struct drm_i915_reg_descriptor ivb_master_regs[] = { + REG32(FORCEWAKE_MT), + REG32(DERRMR), + REG32(GEN7_PIPE_DE_LOAD_SL(PIPE_A)), + REG32(GEN7_PIPE_DE_LOAD_SL(PIPE_B)), + REG32(GEN7_PIPE_DE_LOAD_SL(PIPE_C)), }; -static const u32 hsw_master_regs[] = { - FORCEWAKE_MT, - DERRMR, +static const struct drm_i915_reg_descriptor hsw_master_regs[] = { + REG32(FORCEWAKE_MT), + REG32(DERRMR), }; #undef REG64 +#undef REG32 static u32 gen7_render_get_cmd_length_mask(u32 cmd_header) { @@ -550,14 +573,16 @@ static bool validate_cmds_sorted(struct intel_engine_cs *ring, return ret; } -static bool check_sorted(int ring_id, const u32 *reg_table, int reg_count) +static bool check_sorted(int ring_id, + const struct drm_i915_reg_descriptor *reg_table, + int reg_count) { int i; u32 previous = 0; bool ret = true; for (i = 0; i < reg_count; i++) { - u32 curr = reg_table[i]; + u32 curr = reg_table[i].addr; if (curr < previous) { DRM_ERROR("CMD: table not sorted ring=%d entry=%d reg=0x%08X prev=0x%08X\n", @@ -804,18 +829,20 @@ find_cmd(struct intel_engine_cs *ring, return default_desc; } -static bool valid_reg(const u32 *table, int count, u32 addr) +static const struct drm_i915_reg_descriptor * +find_reg(const struct drm_i915_reg_descriptor *table, + int count, u32 addr) { - if (table && count != 0) { + if (table) { int i; for (i = 0; i < count; i++) { - if (table[i] == addr) - return true; + if (table[i].addr == addr) + return &table[i]; } } - return false; + return NULL; } static u32 *vmap_batch(struct drm_i915_gem_object *obj, @@ -961,6 +988,20 @@ static bool check_cmd(const struct intel_engine_cs *ring, for (offset = desc->reg.offset; offset < length; offset += step) { const u32 reg_addr = cmd[offset] & desc->reg.mask; + const struct drm_i915_reg_descriptor *reg = + find_reg(ring->reg_table, ring->reg_count, + reg_addr); + + if (!reg && is_master) + reg = find_reg(ring->master_reg_table, + ring->master_reg_count, + reg_addr); + + if (!reg) { + DRM_DEBUG_DRIVER("CMD: Rejected register 0x%08X in command: 0x%08X (ring=%d)\n", + reg_addr, *cmd, ring->id); + return false; + } /* * OACONTROL requires some special handling for @@ -982,15 +1023,22 @@ static bool check_cmd(const struct intel_engine_cs *ring, *oacontrol_set = (cmd[offset + 1] != 0); } - if (!valid_reg(ring->reg_table, - ring->reg_count, reg_addr)) { - if (!is_master || - !valid_reg(ring->master_reg_table, - ring->master_reg_count, - reg_addr)) { - DRM_DEBUG_DRIVER("CMD: Rejected register 0x%08X in command: 0x%08X (ring=%d)\n", - reg_addr, *cmd, - ring->id); + /* + * Check the value written to the register against the + * allowed mask/value pair given in the whitelist entry. + */ + if (reg->mask) { + if (desc->cmd.value == MI_LOAD_REGISTER_MEM) { + DRM_DEBUG_DRIVER("CMD: Rejected LRM to masked register 0x%08X\n", + reg_addr); + return false; + } + + if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1) && + (offset + 2 > length || + (cmd[offset + 1] & reg->mask) != reg->value)) { + DRM_DEBUG_DRIVER("CMD: Rejected LRI to masked register 0x%08X\n", + reg_addr); return false; } } diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 39f6dfc0ee54..e539314ae87e 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -118,6 +118,7 @@ struct intel_ringbuffer { }; struct intel_context; +struct drm_i915_reg_descriptor; struct intel_engine_cs { const char *name; @@ -300,14 +301,14 @@ struct intel_engine_cs { /* * Table of registers allowed in commands that read/write registers. */ - const u32 *reg_table; + const struct drm_i915_reg_descriptor *reg_table; int reg_count; /* * Table of registers allowed in commands that read/write registers, but * only from the DRM master. */ - const u32 *master_reg_table; + const struct drm_i915_reg_descriptor *master_reg_table; int master_reg_count; /* From 41d232b7c891fcecb358784645633850936a95c7 Mon Sep 17 00:00:00 2001 From: Francisco Jerez Date: Fri, 29 May 2015 16:44:15 +0300 Subject: [PATCH 4/7] drm/i915: Add SCRATCH1 and ROW_CHICKEN3 to the register whitelist. Only bit 27 of SCRATCH1 and bit 6 of ROW_CHICKEN3 are allowed to be set because of security-sensitive bits we don't want userspace to mess with. On HSW hardware the whitelisted bits control whether atomic read-modify-write operations are performed on L3 or on GTI, and when set to L3 (which can be 10x-30x better performing than on GTI, depending on the application) require great care to avoid a system hang, so we currently program them to be handled on GTI by default. Beignet can immediately start taking advantage of this change to enable L3 atomics. Mesa should eventually switch to L3 atomics too, but a number of non-trivial changes are still required so it will continue using GTI atomics for now. Signed-off-by: Francisco Jerez Reviewed-by: Zhigang Gong Signed-off-by: Daniel Vetter Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/i915_cmd_parser.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c index cafa3e2f16fc..306d9e4e5cf3 100644 --- a/drivers/gpu/drm/i915/i915_cmd_parser.c +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c @@ -463,6 +463,13 @@ static const struct drm_i915_reg_descriptor gen7_render_regs[] = { REG32(GEN7_L3SQCREG1), REG32(GEN7_L3CNTLREG2), REG32(GEN7_L3CNTLREG3), + REG32(HSW_SCRATCH1, + .mask = ~HSW_SCRATCH1_L3_DATA_ATOMICS_DISABLE, + .value = 0), + REG32(HSW_ROW_CHICKEN3, + .mask = ~(HSW_ROW_CHICKEN3_L3_GLOBAL_ATOMICS_DISABLE << 16 | + HSW_ROW_CHICKEN3_L3_GLOBAL_ATOMICS_DISABLE), + .value = 0), }; static const struct drm_i915_reg_descriptor gen7_blt_regs[] = { From e62d8dc0aa9f7493bcee3cbc4bf666a23a2d2f17 Mon Sep 17 00:00:00 2001 From: Ander Conselvan de Oliveira Date: Tue, 16 Jun 2015 11:49:43 +0300 Subject: [PATCH 5/7] drm/i915: Don't check modeset state in the hw state force restore path Since the force restore logic will restore the CRTCs state one at a time, it is possible that the state will be inconsistent until the whole operation finishes. A call to intel_modeset_check_state() is done once it's over, so don't check the state multiple times in between. This regression was introduced in: commit 7f27126ea3db6ade886f18fd39caf0ff0cd1d37f Author: Jesse Barnes Date: Wed Nov 5 14:26:06 2014 -0800 drm/i915: factor out compute_config from __intel_set_mode v3 v2: Rename check parameter to force_restore. (Matt) Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=94431 Cc: Jesse Barnes Signed-off-by: Ander Conselvan de Oliveira Reviewed-by: Matt Roper Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 4e3f302d86f7..bcf1041b5a1a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -87,7 +87,8 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config); static int intel_set_mode(struct drm_crtc *crtc, - struct drm_atomic_state *state); + struct drm_atomic_state *state, + bool force_restore); static int intel_framebuffer_init(struct drm_device *dev, struct intel_framebuffer *ifb, struct drm_mode_fb_cmd2 *mode_cmd, @@ -10096,7 +10097,7 @@ retry: drm_mode_copy(&crtc_state->base.mode, mode); - if (intel_set_mode(crtc, state)) { + if (intel_set_mode(crtc, state, true)) { DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n"); if (old->release_fb) old->release_fb->funcs->destroy(old->release_fb); @@ -10170,7 +10171,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, if (ret) goto fail; - ret = intel_set_mode(crtc, state); + ret = intel_set_mode(crtc, state, true); if (ret) goto fail; @@ -12646,20 +12647,22 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, } static int intel_set_mode_with_config(struct drm_crtc *crtc, - struct intel_crtc_state *pipe_config) + struct intel_crtc_state *pipe_config, + bool force_restore) { int ret; ret = __intel_set_mode(crtc, pipe_config); - if (ret == 0) + if (ret == 0 && force_restore) intel_modeset_check_state(crtc->dev); return ret; } static int intel_set_mode(struct drm_crtc *crtc, - struct drm_atomic_state *state) + struct drm_atomic_state *state, + bool force_restore) { struct intel_crtc_state *pipe_config; int ret = 0; @@ -12670,7 +12673,7 @@ static int intel_set_mode(struct drm_crtc *crtc, goto out; } - ret = intel_set_mode_with_config(crtc, pipe_config); + ret = intel_set_mode_with_config(crtc, pipe_config, force_restore); if (ret) goto out; @@ -12747,7 +12750,7 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) intel_modeset_setup_plane_state(state, crtc, &crtc->mode, crtc->primary->fb, crtc->x, crtc->y); - ret = intel_set_mode(crtc, state); + ret = intel_set_mode(crtc, state, false); if (ret) drm_atomic_state_free(state); } @@ -12947,7 +12950,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set) primary_plane_was_visible = primary_plane_visible(set->crtc); - ret = intel_set_mode_with_config(set->crtc, pipe_config); + ret = intel_set_mode_with_config(set->crtc, pipe_config, true); if (ret == 0 && pipe_config->base.enable && From 0d26fb891a119d78d8e4b345cfd0be0f2112cb7a Mon Sep 17 00:00:00 2001 From: Ander Conselvan de Oliveira Date: Tue, 16 Jun 2015 11:49:44 +0300 Subject: [PATCH 6/7] drm/i915: Don't update staged config during force restore modesets The force restore path relies on the staged config to preserve the configuration used before a suspend/resume cycle. The update done to it in intel_modeset_fixup_state() would cause that information to be lost after the first modeset, making it impossible to restore the modes for pipes B and C. References: https://bugs.freedesktop.org/show_bug.cgi?id=90468 Signed-off-by: Ander Conselvan de Oliveira Reviewed-by: Matt Roper Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/intel_display.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index bcf1041b5a1a..ff7175b6a013 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11386,10 +11386,6 @@ static void intel_modeset_fixup_state(struct drm_atomic_state *state) crtc->base.enabled = crtc->base.state->enable; crtc->config = to_intel_crtc_state(crtc->base.state); } - - /* Copy the new configuration to the staged state, to keep the few - * pieces of code that haven't been converted yet happy */ - intel_modeset_update_staged_output_state(state->dev); } static void @@ -12654,8 +12650,10 @@ static int intel_set_mode_with_config(struct drm_crtc *crtc, ret = __intel_set_mode(crtc, pipe_config); - if (ret == 0 && force_restore) + if (ret == 0 && force_restore) { + intel_modeset_update_staged_output_state(crtc->dev); intel_modeset_check_state(crtc->dev); + } return ret; } From 4ed9fb371ccdfe465bd3bbb69e4cad5243e6c4e2 Mon Sep 17 00:00:00 2001 From: Ander Conselvan de Oliveira Date: Tue, 16 Jun 2015 11:49:45 +0300 Subject: [PATCH 7/7] drm/i915: Don't set enabled value of all CRTCs when restoring the mode The code in intel_crtc_restore_mode() sets the enabled value of all the CRTCs when restoring the mode after a suspend/resume cycle. When more than one CRTC is enabled, that causes drm_atomic_helper_check_modeset() to fail if there is more than one pipe enabled, since all but one CRTC has valid connector data. Instead, set only the enabled value for the CRTC passed as an argument. v2: Don't leak atomic state. (Matt) Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90468 References: https://bugs.freedesktop.org/show_bug.cgi?id=90396 Signed-off-by: Ander Conselvan de Oliveira Reviewed-by: Matt Roper --- drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++----------------- 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ff7175b6a013..e047105837c9 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12683,7 +12683,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; struct drm_atomic_state *state; - struct intel_crtc *intel_crtc; struct intel_encoder *encoder; struct intel_connector *connector; struct drm_connector_state *connector_state; @@ -12726,25 +12725,19 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) } } - for_each_intel_crtc(dev, intel_crtc) { - if (intel_crtc->new_enabled == intel_crtc->base.enabled) - continue; - - crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); - if (IS_ERR(crtc_state)) { - DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n", - intel_crtc->base.base.id, - PTR_ERR(crtc_state)); - continue; - } - - crtc_state->base.active = crtc_state->base.enable = - intel_crtc->new_enabled; - - if (&intel_crtc->base == crtc) - drm_mode_copy(&crtc_state->base.mode, &crtc->mode); + crtc_state = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc)); + if (IS_ERR(crtc_state)) { + DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n", + crtc->base.id, PTR_ERR(crtc_state)); + drm_atomic_state_free(state); + return; } + crtc_state->base.active = crtc_state->base.enable = + to_intel_crtc(crtc)->new_enabled; + + drm_mode_copy(&crtc_state->base.mode, &crtc->mode); + intel_modeset_setup_plane_state(state, crtc, &crtc->mode, crtc->primary->fb, crtc->x, crtc->y);