Re: [Intel-gfx] [PATCH 45/53] drm/i915/bdw: Do not call intel_runtime_pm_get() in an interrupt
On Sat, Jul 26, 2014 at 11:27:38AM +0100, Chris Wilson wrote: On Wed, Jun 18, 2014 at 10:54:13PM +0200, Daniel Vetter wrote: On Fri, Jun 13, 2014 at 04:38:03PM +0100, oscar.ma...@intel.com wrote: From: Oscar Mateo oscar.ma...@intel.com Or with a spinlock grabbed, because it might sleep, which is not a nice thing to do. Instead, do the runtime_pm get/put together with the create/destroy request, and handle the forcewake get/put directly. Signed-off-by: Oscar Mateo oscar.ma...@intel.com Looks like a fixup that should be squashed into relevant earlier patches. The whole gen6_gt_force_wake_get() calling intel_runtime_pm_get() is broken due to this - we must be able to read registers in atomic context! Please revert c8c8fb33b37766acf6474784b0d5245dab9a1690 force_wake_get can't call runtime_pm_get becuase pm_get can sleep. So if you want to read registers from atomic context you have to have a runtime pm reference from someone else. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 22/40] drm/i915: Add chv port D TX wells
On Fri, Jul 25, 2014 at 04:30:29PM +0300, Imre Deak wrote: On Sat, 2014-06-28 at 02:04 +0300, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com Add the TX wells for port D. The Punit subsystem numbers are a total guess at this time. Also I'm not sure these even exist. Certainly the Punit in current hardware doesn't deal with these. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/i915_reg.h | 4 drivers/gpu/drm/i915/intel_pm.c | 23 +++ 2 files changed, 27 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 3d1fef4..191df9e 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -525,6 +525,10 @@ enum punit_power_well { PUNIT_POWER_WELL_DPIO_RX0 = 10, PUNIT_POWER_WELL_DPIO_RX1 = 11, PUNIT_POWER_WELL_DPIO_CMN_D = 12, + /* FIXME: guesswork below */ + PUNIT_POWER_WELL_DPIO_TX_D_LANES_01 = 13, + PUNIT_POWER_WELL_DPIO_TX_D_LANES_23 = 14, + PUNIT_POWER_WELL_DPIO_RX2 = 15, PUNIT_POWER_WELL_NUM, }; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index cae936c..55f3e6b 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -6540,6 +6540,15 @@ EXPORT_SYMBOL_GPL(i915_release_power_well); BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) | \ BIT(POWER_DOMAIN_INIT)) +#define CHV_DPIO_TX_D_LANES_01_POWER_DOMAINS ( \ + BIT(POWER_DOMAIN_PORT_DDI_D_2_LANES) | \ + BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) | \ + BIT(POWER_DOMAIN_INIT)) + +#define CHV_DPIO_TX_D_LANES_23_POWER_DOMAINS ( \ + BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) | \ Atm, for all other ports we power up all lanes regardless of the actual configuration (until the PHY side setup is proved to work fine). So for consistency I'd do the same here too. With that change: Reviewed-by: Imre Deak imre.d...@intel.com Pulled in all the power well patches Imre reviewed except this one. Thanks, Daniel + BIT(POWER_DOMAIN_INIT)) + static const struct i915_power_well_ops i9xx_always_on_power_well_ops = { .sync_hw = i9xx_always_on_power_well_noop, .enable = i9xx_always_on_power_well_noop, @@ -6757,6 +6766,20 @@ static struct i915_power_well chv_power_wells[] = { .ops = vlv_dpio_power_well_ops, .data = PUNIT_POWER_WELL_DPIO_TX_C_LANES_23, }, + { + .name = dpio-tx-d-01, + .domains = CHV_DPIO_TX_D_LANES_01_POWER_DOMAINS | + CHV_DPIO_TX_D_LANES_23_POWER_DOMAINS, + .ops = vlv_dpio_power_well_ops, + .data = PUNIT_POWER_WELL_DPIO_TX_D_LANES_01, + }, + { + .name = dpio-tx-d-23, + .domains = CHV_DPIO_TX_D_LANES_01_POWER_DOMAINS | + CHV_DPIO_TX_D_LANES_23_POWER_DOMAINS, + .ops = vlv_dpio_power_well_ops, + .data = PUNIT_POWER_WELL_DPIO_TX_D_LANES_23, + }, #endif }; ___ 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915/bdw: BDW Software Turbo
On Fri, Jul 25, 2014 at 05:47:11PM -0700, Sun, Daisy wrote: we have reconsidered good suggestions and evaluated performance and complexity again. Timer Constant callback would continuously wake up CPU and entire package, results in lower CPU and package C-state and shorter battery life, especially for standby time. If you shut down the timer when idle this shouldn't be a concern at all. See the idle infrastructure we have and e.g. the retire work handler. We could even put the software turbo into the retire work handler ... execbuf is a good one, and we had taken it into account too. execbuf can happen much more frequent than flips. Synchronization and calculation overhead were the main reasons that we tried to avoid using too much IA resource to benefit GT. Yeah, running it at each execbuf is going to be too expensive. But with the regular timer there should be no need to also do this in flips - it might badly interfere with the missed-flip boosting patches from Chris even. Here's is a revised version of software turbo for BDW, please take a look and see if there's any concern. Doesn't seem to be attached. For software turbo, it can be tough to find out a perfect solution , may need some trade-off. Well, but we've made already quite some nice improvments with the boosting logic. So I think it can be done, but I agree it's not easy. Revised design: GT busyness will still be calculated when page_flip comes in, then GT frequency will be adjusted accordingly. This point stays the same as previous design. For the cases no flip will happen(server or background task with no display activity) which is a previous concern, set GT frequency to RP0(no turbo algorithm interfered in this case). Implementation details: 1) Driver start with RP0 as GT frequency. 2) When the flip comes, do the regular software turbo busyness calculation. Also set a timer with 250ms; 3) If the flip keep coming in time, keep turbo algorithm, reset timer; 4) When the timer is fired, set RP frequency to RP0 so that the background task will still be taken care of(the RPS boost and idle need to be disabled in this situation). 5)If the flip comes again, go to 2). To recap, For most common cases, GT will run at a desired frequency as a result of software turbo algorithm; For background workloads or no flip environment, GT will be running at RP0 with shorter execution time to extend RC6 and pkg C state residency as long as power is concerned. I'll start with the implementation if all concerns are ironed out. Ah, I expected a patch ;-) Usually code diffs are much more efficient communication as a replacement for when you'd use a whiteboard session with a colocated team. It doesn't need to run nor even compile, just speudo-code illustrating the main integration points. Anyway, see above for my comments: No need to integrate with flips if we do the timer thing correctly. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Rework GPU reset sequence to match driver load thaw
On Fri, Jul 25, 2014 at 06:05:29PM -0700, Ben Widawsky wrote: On Wed, Jul 16, 2014 at 04:05:59PM +0100, alistair.mcau...@intel.com wrote: From: McAulay, Alistair alistair.mcau...@intel.com This patch is to address Daniels concerns over different code during reset: http://lists.freedesktop.org/archives/intel-gfx/2014-June/047758.html The reason for aiming as hard as possible to use the exact same code for driver load, gpu reset and runtime pm/system resume is that we've simply seen too many bugs due to slight variations and unintended omissions. Tested using igt drv_hangman. Signed-off-by: McAulay, Alistair alistair.mcau...@intel.com 2 quick comments before I actually do a real review. 1. Did you actually run this with and without full ppgtt? 2. I don't think this is actually fulfilling what Daniel is requesting, though we can let him comment. Mostly looks like what I think we need. Comments below. 3. Did you reall do #1? Assuming you satisifed #1, can you please post the igt results for the permutations (pre patch w/ and w/o ppgtt; post patch w/ and w/o ppgtt) I really want this data because I spent *a lot* of time with these specific areas in the PPGTT work, and I am somewhat skeptical enough of the code has changed that this will magically work. I also tried the trickiness with the ring handling functions, and never succeeded. Also, with the context stuff, I'm simply not convinced it can magically vanish. If igt looks good, and Daniel agrees that this is what he actually wanted, I will go fishing for corner cases and do the review. I think the hack in ring_begin might explain why it never worked before. But fully agreed, we really need to test this well (and fill gaps if igt misses anything around resets - we don't have any systematic gpu reset coverage anywhere outside of igt). Thanks. --- drivers/gpu/drm/i915/i915_gem.c | 2 - drivers/gpu/drm/i915/i915_gem_context.c | 42 + drivers/gpu/drm/i915/i915_gem_gtt.c | 67 + drivers/gpu/drm/i915/i915_gem_gtt.h | 3 +- drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +- 5 files changed, 14 insertions(+), 104 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index ef047bc..b38e086 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2590,8 +2590,6 @@ void i915_gem_reset(struct drm_device *dev) for_each_ring(ring, dev_priv, i) i915_gem_reset_ring_cleanup(dev_priv, ring); - i915_gem_context_reset(dev); - i915_gem_restore_fences(dev); } diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index de72a28..d96219f 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -372,42 +372,6 @@ err_destroy: return ERR_PTR(ret); } -void i915_gem_context_reset(struct drm_device *dev) -{ - struct drm_i915_private *dev_priv = dev-dev_private; - int i; - - /* Prevent the hardware from restoring the last context (which hung) on -* the next switch */ - for (i = 0; i I915_NUM_RINGS; i++) { - struct intel_engine_cs *ring = dev_priv-ring[i]; - struct intel_context *dctx = ring-default_context; - struct intel_context *lctx = ring-last_context; - - /* Do a fake switch to the default context */ - if (lctx == dctx) - continue; - - if (!lctx) - continue; - - if (dctx-legacy_hw_ctx.rcs_state i == RCS) { - WARN_ON(i915_gem_obj_ggtt_pin(dctx-legacy_hw_ctx.rcs_state, - get_context_alignment(dev), 0)); - /* Fake a finish/inactive */ - dctx-legacy_hw_ctx.rcs_state-base.write_domain = 0; - dctx-legacy_hw_ctx.rcs_state-active = 0; - } - - if (lctx-legacy_hw_ctx.rcs_state i == RCS) - i915_gem_object_ggtt_unpin(lctx-legacy_hw_ctx.rcs_state); - - i915_gem_context_unreference(lctx); - i915_gem_context_reference(dctx); - ring-last_context = dctx; - } -} I don't understand why we no longer need this - after reset we probably have the default context loaded (if we resue the driver load sequence exactly), so I expect that we must reset the software tracking accordingly. - int i915_gem_context_init(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; @@ -498,10 +462,6 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv) ppgtt-enable(ppgtt); } - /* FIXME: We should make this work, even in reset */ - if (i915_reset_in_progress(dev_priv-gpu_error))
Re: [Intel-gfx] [PATCH i-g-t] lib: avoid getopt value conflicts with tests
On 25 July 2014 17:57, Paulo Zanoni przan...@gmail.com wrote: 2014-07-25 13:08 GMT-03:00 Thomas Wood thomas.w...@intel.com: Most tests use a printable character as the value for getopt to return, so avoid conflicts by using non-printing values for the standard options. Instead of this patch, isn't there any way to verify if the tests are using any character that is reserved to these general options? Having -r instead of --run-subtest is quite nice. That said, I'm not against your patch either. The only standard short option is -h for --help, which needs to be fixed in the patch. Adding a warning for conflicting short options and getopt return values could be added, but probably in a separate patch. Signed-off-by: Thomas Wood thomas.w...@intel.com --- lib/igt_core.c | 23 +++ 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/lib/igt_core.c b/lib/igt_core.c index a0c9499..882614a 100644 --- a/lib/igt_core.c +++ b/lib/igt_core.c @@ -218,6 +218,13 @@ int num_test_children; int test_children_sz; bool test_child; +enum { + OPT_LIST_SUBTESTS, + OPT_RUN_SUBTEST, + OPT_DEBUG, + OPT_HELP +}; + __attribute__((format(printf, 1, 2))) static void kmsg(const char *format, ...) #define KERN_INFO 5 @@ -320,10 +327,10 @@ static int common_init(int argc, char **argv, { int c, option_index = 0; static struct option long_options[] = { - {list-subtests, 0, 0, 'l'}, - {run-subtest, 1, 0, 'r'}, - {debug, 0, 0, 'd'}, - {help, 0, 0, 'h'}, + {list-subtests, 0, 0, OPT_LIST_SUBTESTS}, + {run-subtest, 1, 0, OPT_RUN_SUBTEST}, + {debug, 0, 0, OPT_DEBUG}, + {help, 0, 0, OPT_HELP}, }; char *short_opts; struct option *combined_opts; @@ -370,18 +377,18 @@ static int common_init(int argc, char **argv, while ((c = getopt_long(argc, argv, short_opts, combined_opts, option_index)) != -1) { switch(c) { - case 'd': + case OPT_DEBUG: igt_log_level = IGT_LOG_DEBUG; break; - case 'l': + case OPT_LIST_SUBTESTS: if (!run_single_subtest) list_subtests = true; break; - case 'r': + case OPT_RUN_SUBTEST: if (!list_subtests) run_single_subtest = strdup(optarg); break; - case 'h': + case OPT_HELP: print_usage(help_str, false); ret = -1; goto out; -- 1.9.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx - Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] work around warning in i915_gem_gtt
Gcc warns that addr might be used uninitialized. It may not, but I see why gcc gets confused. Additionally, hiding code with side-effects inside WARN_ON() argument seems uncool, so I moved it outside. Signed-off-by: Pavel Machek pa...@ucw.cz diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 8b3cde7..8fcc974 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1448,7 +1448,7 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm, (gen6_gtt_pte_t __iomem *)dev_priv-gtt.gsm + first_entry; int i = 0; struct sg_page_iter sg_iter; - dma_addr_t addr; + dma_addr_t addr = 0; for_each_sg_page(st-sgl, sg_iter, st-nents, 0) { addr = sg_page_iter_dma_address(sg_iter); @@ -1462,9 +1462,10 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm, * of NUMA access patterns. Therefore, even with the way we assume * hardware should work, we must keep this posting read for paranoia. */ - if (i != 0) - WARN_ON(readl(gtt_entries[i-1]) != - vm-pte_encode(addr, level, true)); + if (i != 0) { + unsigned long gtt = readl(gtt_entries[i-1]); + WARN_ON(gtt != vm-pte_encode(addr, level, true)); + } /* This next bit makes the above posting read even more important. We * want to flush the TLBs only after we're certain all the PTE updates -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] drm/i915: CONFIG_DRM_I915_UMS
On Sat, 2014-07-26 at 01:44 +0200, Daniel Vetter wrote: On Fri, Jul 25, 2014 at 2:14 PM, Paul Bolle pebo...@tiscali.nl wrote: Your commit 2225a28fd916 (drm/i915: Ditch UMS config option) is included in today's linux-next (ie, next-20140725). It removes the Kconfig symbol DRM_I915_UMS. It didn't remove the two (negative) checks for CONFIG_DRM_I915_UMS. These checks are superfluous as they now will always evaluate to true. Is the trivial cleanup to remove them already queued somewhere? No, and intentionally. So this was not by mistake, which is good to know. Actually removing the code for user-mode-setting isn't just removing these two blocks, Just to be clear: I'm only suggesting to remove the two lines reading #ifndef CONFIG_DRM_I915_UMS and their corresponding #endif lines. but requires the gutting of roughly 10k lines splattered all over the driver. Essentially all the code that checks for !drm_core_check_feature(DRIVER_MODESET) needs to go. That's not quite as trivial, and before I do that I want to make really sure that really no one misses this option. So probably after 3.17 is out the door for a bit. None of what I brought up is urgent. But I do hope you don't mind me sending a reminder if these few (preprocessor) lines are staying around longer than expected. Paul Bolle ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Introduce FBC False Color for debug purposes.
On Mon, Jul 07, 2014 at 11:42:04AM -0700, Rodrigo Vivi wrote: With this bit enabled, HW changes the color when compressing frames for debug purposes. ALthough the simple way to enable a single bit is over intel_reg_write, this value is overwriten on next update_fbc so depending on the workload it is not possible to set this bit with intel-gpu-tools. So this patch introduces a persistent way to enable false color over debugfs. v2: Use DEFINE_SIMPLE_ATTRIBUTE as Daniel suggested Cc: Daniel Vetter daniel.vet...@ffwll.ch Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com --- drivers/gpu/drm/i915/i915_debugfs.c | 42 + drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 6 ++ 4 files changed, 51 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index c1b88a8..b049fc5 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1510,6 +1510,47 @@ static int i915_fbc_status(struct seq_file *m, void *unused) return 0; } +static int i915_fbc_fc_get(void *data, u64 *val) +{ + struct drm_device *dev = data; + struct drm_i915_private *dev_priv = dev-dev_private; + + if (INTEL_INFO(dev)-gen 5) + return -ENODEV; Did you test this on ILK/SNB? Bspec says the bit is MBZ before IVB. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/40] drm/i915: Align chv rps min/max/rpe values
On Sat, Jul 12, 2014 at 07:16:15PM +0530, Deepak S wrote: On Saturday 28 June 2014 04:33 AM, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com CHV wants even rps opcodes so make sure the min/max/rpe values are also even. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/i915_debugfs.c | 8 drivers/gpu/drm/i915/intel_pm.c | 19 ++- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 415010e..9b01e7c 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -3566,6 +3566,10 @@ i915_max_freq_set(void *data, u64 val) if (IS_VALLEYVIEW(dev)) { val = vlv_freq_opcode(dev_priv, val); + /* CHV needs even encode values */ + if (IS_CHERRYVIEW(dev)) + val = ~1; + hw_max = dev_priv-rps.max_freq; hw_min = dev_priv-rps.min_freq; } else { @@ -3647,6 +3651,10 @@ i915_min_freq_set(void *data, u64 val) if (IS_VALLEYVIEW(dev)) { val = vlv_freq_opcode(dev_priv, val); + /* CHV needs even encode values */ + if (IS_CHERRYVIEW(dev)) + val = ALIGN(val, 2); + hw_max = dev_priv-rps.max_freq; hw_min = dev_priv-rps.min_freq; } else { diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 10c9c02..e3f23c2 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3924,21 +3924,30 @@ static void cherryview_init_gt_powersave(struct drm_device *dev) mutex_lock(dev_priv-rps.hw_lock); dev_priv-rps.max_freq = cherryview_rps_max_freq(dev_priv); + if (WARN_ON_ONCE(dev_priv-rps.max_freq 1)) + dev_priv-rps.max_freq = ~1; Cannot we use ALIGN Here? The idea was to round max freq down and min freq up. Other than this it looks fine Reviewed-by: Deepak S deepa...@linux.intel.com dev_priv-rps.rp0_freq = dev_priv-rps.max_freq; DRM_DEBUG_DRIVER(max GPU freq: %d MHz (%u)\n, vlv_gpu_freq(dev_priv, dev_priv-rps.max_freq), dev_priv-rps.max_freq); - dev_priv-rps.efficient_freq = cherryview_rps_rpe_freq(dev_priv); - DRM_DEBUG_DRIVER(RPe GPU freq: %d MHz (%u)\n, -vlv_gpu_freq(dev_priv, dev_priv-rps.efficient_freq), -dev_priv-rps.efficient_freq); - dev_priv-rps.min_freq = cherryview_rps_min_freq(dev_priv); + if (WARN_ON_ONCE(dev_priv-rps.min_freq 1)) + dev_priv-rps.min_freq = ALIGN(dev_priv-rps.min_freq, 2); DRM_DEBUG_DRIVER(min GPU freq: %d MHz (%u)\n, vlv_gpu_freq(dev_priv, dev_priv-rps.min_freq), dev_priv-rps.min_freq); + dev_priv-rps.efficient_freq = cherryview_rps_rpe_freq(dev_priv); + if (WARN_ON_ONCE(dev_priv-rps.min_freq 1)) + dev_priv-rps.efficient_freq = ~1; + dev_priv-rps.efficient_freq = clamp(dev_priv-rps.efficient_freq, +dev_priv-rps.min_freq, +dev_priv-rps.max_freq); + DRM_DEBUG_DRIVER(RPe GPU freq: %d MHz (%u)\n, +vlv_gpu_freq(dev_priv, dev_priv-rps.efficient_freq), +dev_priv-rps.efficient_freq); + /* Preserve min/max settings in case of re-init */ if (dev_priv-rps.max_freq_softlimit == 0) dev_priv-rps.max_freq_softlimit = dev_priv-rps.max_freq; -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t] lib: check test options for conflicts
Check any test specific options for conflicts with the standard set of options. Signed-off-by: Thomas Wood thomas.w...@intel.com --- lib/igt_core.c | 46 ++ 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/lib/igt_core.c b/lib/igt_core.c index 0e254e3..f4761ca 100644 --- a/lib/igt_core.c +++ b/lib/igt_core.c @@ -325,14 +325,16 @@ static int common_init(int argc, char **argv, const char *help_str, igt_opt_handler_t extra_opt_handler) { - int c, option_index = 0; + int c, option_index = 0, i, x; static struct option long_options[] = { {list-subtests, 0, 0, OPT_LIST_SUBTESTS}, {run-subtest, 1, 0, OPT_RUN_SUBTEST}, {debug, 0, 0, OPT_DEBUG}, {help, 0, 0, OPT_HELP}, + {0, 0, 0, 0} }; char *short_opts; + const char *std_short_opts = h; struct option *combined_opts; int extra_opt_count; int all_opt_count; @@ -356,10 +358,45 @@ static int common_init(int argc, char **argv, /* First calculate space for all passed-in extra long options */ all_opt_count = 0; - while (extra_long_opts extra_long_opts[all_opt_count].name) + while (extra_long_opts extra_long_opts[all_opt_count].name) { + + /* check for conflicts with standard long option values */ + for (i = 0; long_options[i].name; i++) + if (extra_long_opts[all_opt_count].val == long_options[i].val) + igt_warn(Conflicting long option values between --%s and --%s\n, +extra_long_opts[all_opt_count].name, +long_options[i].name); + + /* check for conflicts with short options */ + if (extra_long_opts[all_opt_count].val != ':' +strchr(std_short_opts, extra_long_opts[all_opt_count].val)) { + igt_warn(Conflicting long and short option values between --%s and -%s\n, +extra_long_opts[all_opt_count].name, +long_options[i].name); + } + + all_opt_count++; + } extra_opt_count = all_opt_count; + /* check for conflicts in extra short options*/ + for (i = 0; extra_short_opts extra_short_opts[i]; i++) { + + if (extra_short_opts[i] == ':') + continue; + + /* check for conflicts with standard short options */ + if (strchr(std_short_opts, extra_short_opts[i])) + igt_warn(Conflicting short option: -%c\n, std_short_opts[i]); + + /* check for conflicts with standard long option values */ + for (x = 0; long_options[x].name; x++) + if (long_options[x].val == extra_short_opts[i]) + igt_warn(Conflicting short option and long option value: --%s and -%c\n, +long_options[x].name, extra_short_opts[i]); + } + all_opt_count += ARRAY_SIZE(long_options); combined_opts = malloc(all_opt_count * sizeof(*combined_opts)); @@ -370,8 +407,9 @@ static int common_init(int argc, char **argv, memcpy(combined_opts[extra_opt_count], long_options, ARRAY_SIZE(long_options) * sizeof(*combined_opts)); - ret = asprintf(short_opts, %sh, - extra_short_opts ? extra_short_opts : ); + ret = asprintf(short_opts, %s%s, + extra_short_opts ? extra_short_opts : , + std_short_opts); assert(ret = 0); while ((c = getopt_long(argc, argv, short_opts, combined_opts, -- 1.9.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 17/40] drm/i915: Add chv cmnlane power wells
On Fri, Jul 25, 2014 at 02:55:00PM +0300, Imre Deak wrote: On Sat, 2014-06-28 at 02:04 +0300, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com CHV has two display PHYs so there are also two cmnlane power wells. Add the approriate code to power the wells up/down. Like on VLV we do the cmnreset assert/deassert and the DPLL refclock enabling at approriate times. This code actually works on my bsw. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 89 + 2 files changed, 90 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index d246609..19e68d6 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -512,6 +512,7 @@ enum punit_power_well { PUNIT_POWER_WELL_DPIO_TX_C_LANES_23 = 9, PUNIT_POWER_WELL_DPIO_RX0 = 10, PUNIT_POWER_WELL_DPIO_RX1 = 11, + PUNIT_POWER_WELL_DPIO_CMN_D = 12, PUNIT_POWER_WELL_NUM, }; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index e2b956e..f88490b 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -6200,6 +6200,64 @@ static void vlv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv, vlv_set_power_well(dev_priv, power_well, false); } +static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well) +{ + enum dpio_phy phy; + + WARN_ON_ONCE(power_well-data != PUNIT_POWER_WELL_DPIO_CMN_BC +power_well-data != PUNIT_POWER_WELL_DPIO_CMN_D); + + /* +* Enable the CRI clock source so we can get at the +* display and the reference clock for VGA +* hotplug / manual detection. +*/ + if (power_well-data == PUNIT_POWER_WELL_DPIO_CMN_BC) { + phy = DPIO_PHY0; + I915_WRITE(DPLL(PIPE_B), I915_READ(DPLL(PIPE_B)) | + DPLL_REFA_CLK_ENABLE_VLV); + I915_WRITE(DPLL(PIPE_B), I915_READ(DPLL(PIPE_B)) | + DPLL_REFA_CLK_ENABLE_VLV | DPLL_INTEGRATED_CRI_CLK_VLV); Any reason the two clocks are enabled sequentially? For PHY1 you don't do this.. I think I meant to enable the ref clock for both pipes A and B. So the first rmw should have hit DPLL(PIPE_A). In any case: Reviewed-by: Imre Deak imre.d...@intel.com + } else { + phy = DPIO_PHY1; + I915_WRITE(DPLL(PIPE_C), I915_READ(DPLL(PIPE_C)) | + DPLL_REFA_CLK_ENABLE_VLV | DPLL_INTEGRATED_CRI_CLK_VLV); + } + udelay(1); /* 10ns for cmnreset, 0ns for sidereset */ + vlv_set_power_well(dev_priv, power_well, true); + + /* Poll for phypwrgood signal */ + if (wait_for(I915_READ(DISPLAY_PHY_STATUS) PHY_POWERGOOD(phy), 1)) + DRM_ERROR(Display PHY %d is not power up\n, phy); + + I915_WRITE(DISPLAY_PHY_CONTROL, + PHY_COM_LANE_RESET_DEASSERT(phy, I915_READ(DISPLAY_PHY_CONTROL))); +} + +static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well) +{ + enum dpio_phy phy; + + WARN_ON_ONCE(power_well-data != PUNIT_POWER_WELL_DPIO_CMN_BC +power_well-data != PUNIT_POWER_WELL_DPIO_CMN_D); + + if (power_well-data == PUNIT_POWER_WELL_DPIO_CMN_BC) { + phy = DPIO_PHY0; + assert_pll_disabled(dev_priv, PIPE_A); + assert_pll_disabled(dev_priv, PIPE_B); + } else { + phy = DPIO_PHY1; + assert_pll_disabled(dev_priv, PIPE_C); + } + + I915_WRITE(DISPLAY_PHY_CONTROL, + PHY_COM_LANE_RESET_ASSERT(phy, I915_READ(DISPLAY_PHY_CONTROL))); + + vlv_set_power_well(dev_priv, power_well, false); +} + static void check_power_well_state(struct drm_i915_private *dev_priv, struct i915_power_well *power_well) { @@ -6369,6 +6427,18 @@ EXPORT_SYMBOL_GPL(i915_release_power_well); BIT(POWER_DOMAIN_PORT_DDI_C_4_LANES) | \ BIT(POWER_DOMAIN_INIT)) +#define CHV_DPIO_CMN_BC_POWER_DOMAINS (\ + BIT(POWER_DOMAIN_PORT_DDI_B_2_LANES) | \ + BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) | \ + BIT(POWER_DOMAIN_PORT_DDI_C_2_LANES) | \ + BIT(POWER_DOMAIN_PORT_DDI_C_4_LANES) | \ + BIT(POWER_DOMAIN_INIT)) + +#define CHV_DPIO_CMN_D_POWER_DOMAINS ( \ + BIT(POWER_DOMAIN_PORT_DDI_D_2_LANES) | \ + BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) | \ + BIT(POWER_DOMAIN_INIT)) + static const struct i915_power_well_ops i9xx_always_on_power_well_ops = { .sync_hw =
Re: [Intel-gfx] [PATCH 22/40] drm/i915: Add chv port D TX wells
On Fri, Jul 25, 2014 at 04:30:29PM +0300, Imre Deak wrote: On Sat, 2014-06-28 at 02:04 +0300, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com Add the TX wells for port D. The Punit subsystem numbers are a total guess at this time. Also I'm not sure these even exist. Certainly the Punit in current hardware doesn't deal with these. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/i915_reg.h | 4 drivers/gpu/drm/i915/intel_pm.c | 23 +++ 2 files changed, 27 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 3d1fef4..191df9e 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -525,6 +525,10 @@ enum punit_power_well { PUNIT_POWER_WELL_DPIO_RX0 = 10, PUNIT_POWER_WELL_DPIO_RX1 = 11, PUNIT_POWER_WELL_DPIO_CMN_D = 12, + /* FIXME: guesswork below */ + PUNIT_POWER_WELL_DPIO_TX_D_LANES_01 = 13, + PUNIT_POWER_WELL_DPIO_TX_D_LANES_23 = 14, + PUNIT_POWER_WELL_DPIO_RX2 = 15, PUNIT_POWER_WELL_NUM, }; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index cae936c..55f3e6b 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -6540,6 +6540,15 @@ EXPORT_SYMBOL_GPL(i915_release_power_well); BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) | \ BIT(POWER_DOMAIN_INIT)) +#define CHV_DPIO_TX_D_LANES_01_POWER_DOMAINS ( \ + BIT(POWER_DOMAIN_PORT_DDI_D_2_LANES) | \ + BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) | \ + BIT(POWER_DOMAIN_INIT)) + +#define CHV_DPIO_TX_D_LANES_23_POWER_DOMAINS ( \ + BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) | \ Atm, for all other ports we power up all lanes regardless of the actual configuration (until the PHY side setup is proved to work fine). So for consistency I'd do the same here too. With that change: We do that here too. '.domains = 01 | 23' for both tx-d wells. Or am I missing something? Reviewed-by: Imre Deak imre.d...@intel.com + BIT(POWER_DOMAIN_INIT)) + static const struct i915_power_well_ops i9xx_always_on_power_well_ops = { .sync_hw = i9xx_always_on_power_well_noop, .enable = i9xx_always_on_power_well_noop, @@ -6757,6 +6766,20 @@ static struct i915_power_well chv_power_wells[] = { .ops = vlv_dpio_power_well_ops, .data = PUNIT_POWER_WELL_DPIO_TX_C_LANES_23, }, + { + .name = dpio-tx-d-01, + .domains = CHV_DPIO_TX_D_LANES_01_POWER_DOMAINS | + CHV_DPIO_TX_D_LANES_23_POWER_DOMAINS, + .ops = vlv_dpio_power_well_ops, + .data = PUNIT_POWER_WELL_DPIO_TX_D_LANES_01, + }, + { + .name = dpio-tx-d-23, + .domains = CHV_DPIO_TX_D_LANES_01_POWER_DOMAINS | + CHV_DPIO_TX_D_LANES_23_POWER_DOMAINS, + .ops = vlv_dpio_power_well_ops, + .data = PUNIT_POWER_WELL_DPIO_TX_D_LANES_23, + }, #endif }; -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t] lib: don't abort if forcing the connector state fails
Ensure tests using igt_enable_connectors can still run even if the relevant debugfs files are not available. Signed-off-by: Thomas Wood thomas.w...@intel.com --- lib/igt_kms.c | 19 ++- lib/igt_kms.h | 2 +- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 20370a9..740b5dd 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -429,9 +429,11 @@ static char* get_debugfs_connector_path(int drm_fd, drmModeConnector *connector, * @state: state to force on @connector * * Force the specified state on the specified connector. + * + * Returns: true on success */ -void kmstest_force_connector(int drm_fd, drmModeConnector *connector, enum -kmstest_force_connector_state state) +bool kmstest_force_connector(int drm_fd, drmModeConnector *connector, +enum kmstest_force_connector_state state) { char *path; const char *value; @@ -458,12 +460,15 @@ void kmstest_force_connector(int drm_fd, drmModeConnector *connector, enum debugfs_fd = open(path, O_WRONLY | O_TRUNC); free(path); - igt_assert(debugfs_fd != -1); + if (debugfs_fd == -1) { + return false; + } ret = write(debugfs_fd, value, strlen(value)); close(debugfs_fd); igt_assert(ret != -1); + return (ret == -1) ? false : true; } /** @@ -1509,8 +1514,12 @@ void igt_enable_connectors(void) continue; /* just enable VGA for now */ - if (c-connector_type == DRM_MODE_CONNECTOR_VGA) - kmstest_force_connector(drm_fd, c, FORCE_CONNECTOR_ON); + if (c-connector_type == DRM_MODE_CONNECTOR_VGA) { + if (!kmstest_force_connector(drm_fd, c, FORCE_CONNECTOR_ON)) + igt_info(Unable to force state on %s-%d\n, + kmstest_connector_type_str(c-connector_type), +c-connector_type_id); + } drmModeFreeConnector(c); } diff --git a/lib/igt_kms.h b/lib/igt_kms.h index fb0e66a..08b46ab 100644 --- a/lib/igt_kms.h +++ b/lib/igt_kms.h @@ -130,7 +130,7 @@ int kmstest_get_connector_default_mode(int drm_fd, drmModeConnector *connector, int kmstest_get_connector_config(int drm_fd, uint32_t connector_id, unsigned long crtc_idx_mask, struct kmstest_connector_config *config); -void kmstest_force_connector(int fd, drmModeConnector *connector, +bool kmstest_force_connector(int fd, drmModeConnector *connector, enum kmstest_force_connector_state state); void kmstest_force_edid(int drm_fd, drmModeConnector *connector, const unsigned char *edid, size_t length); -- 1.9.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] work around warning in i915_gem_gtt
On Mon, Jul 28, 2014 at 01:20:58PM +0200, Pavel Machek wrote: Gcc warns that addr might be used uninitialized. It may not, but I see why gcc gets confused. Additionally, hiding code with side-effects inside WARN_ON() argument seems uncool, so I moved it outside. Signed-off-by: Pavel Machek pa...@ucw.cz diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 8b3cde7..8fcc974 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1448,7 +1448,7 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm, (gen6_gtt_pte_t __iomem *)dev_priv-gtt.gsm + first_entry; int i = 0; struct sg_page_iter sg_iter; - dma_addr_t addr; + dma_addr_t addr = 0; I want to have a /* shuts up gcc */ for these kinds of things, where we need to help out the compiler. Added and merged, thanks. -Daniel for_each_sg_page(st-sgl, sg_iter, st-nents, 0) { addr = sg_page_iter_dma_address(sg_iter); @@ -1462,9 +1462,10 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm, * of NUMA access patterns. Therefore, even with the way we assume * hardware should work, we must keep this posting read for paranoia. */ - if (i != 0) - WARN_ON(readl(gtt_entries[i-1]) != - vm-pte_encode(addr, level, true)); + if (i != 0) { + unsigned long gtt = readl(gtt_entries[i-1]); + WARN_ON(gtt != vm-pte_encode(addr, level, true)); + } /* This next bit makes the above posting read even more important. We * want to flush the TLBs only after we're certain all the PTE updates -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ___ dri-devel mailing list dri-de...@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Add rotation_property to mode_config and creating it
On Tue, Jul 15, 2014 at 05:43:37PM +0530, sonika.jin...@intel.com wrote: From: Sonika Jindal sonika.jin...@intel.com v2: Adding creation of rotation_property here. Signed-off-by: Sonika Jindal sonika.jin...@intel.com --- drivers/gpu/drm/drm_crtc.c |3 ++- include/drm/drm_crtc.h |1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 787631e..49c0747 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1299,7 +1299,8 @@ static int drm_mode_create_standard_plane_properties(struct drm_device *dev) type, drm_plane_type_enum_list, ARRAY_SIZE(drm_plane_type_enum_list)); dev-mode_config.plane_type_property = type; - + dev-mode_config.rotation_property = drm_mode_create_rotation_property(dev, + BIT(DRM_ROTATE_0) | BIT(DRM_ROTATE_180)); This might not make sense for other (!i915) hardware. And that's the reason why I had the driver create the property in the first place. I think Daniel was thinking that we might want to expose all the bits regardless of what the hardware supports, but I don't like that idea. There are other properties (eg. alpha blending, csc stuff, etc.) that have the same problem of hardware supporting only a (potentially small) subset of the possible values. I'd rather we didn't make life harder for userspace when the kernel can already report that certain values will never work. return 0; } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index ce6df4a..5545dd3 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -819,6 +819,7 @@ struct drm_mode_config { struct drm_property *dpms_property; struct drm_property *path_property; struct drm_property *plane_type_property; + struct drm_property *rotation_property; /* DVI-I properties */ struct drm_property *dvi_i_subconnector_property; -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] lib: don't abort if forcing the connector state fails
On Mon, Jul 28, 2014 at 04:24:49PM +0100, Thomas Wood wrote: Ensure tests using igt_enable_connectors can still run even if the relevant debugfs files are not available. Signed-off-by: Thomas Wood thomas.w...@intel.com --- lib/igt_kms.c | 19 ++- lib/igt_kms.h | 2 +- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 20370a9..740b5dd 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -429,9 +429,11 @@ static char* get_debugfs_connector_path(int drm_fd, drmModeConnector *connector, * @state: state to force on @connector * * Force the specified state on the specified connector. + * + * Returns: true on success */ -void kmstest_force_connector(int drm_fd, drmModeConnector *connector, enum - kmstest_force_connector_state state) +bool kmstest_force_connector(int drm_fd, drmModeConnector *connector, + enum kmstest_force_connector_state state) { char *path; const char *value; @@ -458,12 +460,15 @@ void kmstest_force_connector(int drm_fd, drmModeConnector *connector, enum debugfs_fd = open(path, O_WRONLY | O_TRUNC); free(path); - igt_assert(debugfs_fd != -1); + if (debugfs_fd == -1) { + return false; + } Aside: We have some neat debugfs helpers in igt_debugfs.c. Might convert over to them while at it. -Daniel ret = write(debugfs_fd, value, strlen(value)); close(debugfs_fd); igt_assert(ret != -1); + return (ret == -1) ? false : true; } /** @@ -1509,8 +1514,12 @@ void igt_enable_connectors(void) continue; /* just enable VGA for now */ - if (c-connector_type == DRM_MODE_CONNECTOR_VGA) - kmstest_force_connector(drm_fd, c, FORCE_CONNECTOR_ON); + if (c-connector_type == DRM_MODE_CONNECTOR_VGA) { + if (!kmstest_force_connector(drm_fd, c, FORCE_CONNECTOR_ON)) + igt_info(Unable to force state on %s-%d\n, + kmstest_connector_type_str(c-connector_type), + c-connector_type_id); + } drmModeFreeConnector(c); } diff --git a/lib/igt_kms.h b/lib/igt_kms.h index fb0e66a..08b46ab 100644 --- a/lib/igt_kms.h +++ b/lib/igt_kms.h @@ -130,7 +130,7 @@ int kmstest_get_connector_default_mode(int drm_fd, drmModeConnector *connector, int kmstest_get_connector_config(int drm_fd, uint32_t connector_id, unsigned long crtc_idx_mask, struct kmstest_connector_config *config); -void kmstest_force_connector(int fd, drmModeConnector *connector, +bool kmstest_force_connector(int fd, drmModeConnector *connector, enum kmstest_force_connector_state state); void kmstest_force_edid(int drm_fd, drmModeConnector *connector, const unsigned char *edid, size_t length); -- 1.9.3 ___ 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: s/seqno/request/ tracking inside objects
On Fri, Jul 25, 2014 at 01:27:00PM +0100, Chris Wilson wrote: At the heart of this change is that the seqno is a too low level of an abstraction to handle the growing complexities of command tracking, both with the introduction of multiple command queues with execbuffer and the potential for reordering with a scheduler. On top of the seqno we have the request. Conceptually this is just a fence, but it also has substantial bookkeeping of its own in order to track the context and batch in flight, for example. It is the central structure upon which we can extend with dependency tracking et al. As regards the objects, they were using the seqno as a simple fence, upon which is check or even wait upon for command completion. This patch exchanges that seqno/ring pair with the request itself. For the majority, lifetime of the request is ordered by how we retire objects then requests. However, both the unlocked waits and probing elsewhere do not tie into the normal request lifetimes and so we need to introduce a kref. Extending the objects to use the request as the fence naturally extends to segregrating read/write fence tracking. This has significance for it reduces the number of semaphores we need to emit, reducing the likelihood of #54226, and improving performance overall. NOTE: this is not against bare drm-intel-nightly and is likely to conflict with execlists... Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Jesse Barnes jbar...@virtuousgeek.org Cc: Daniel Vetter daniel.vet...@ffwll.ch Cc: Oscar Mateo oscar.ma...@intel.com Cc: Brad Volkin bradley.d.vol...@intel.com Ok, read through it and I like overall. Also, right now is the perfect time to merge it since we're right before the merge window. But this here needs to be split up a bit to cut out prep patches. I've noticed a few things in-line, but there's also the mechanical stuff (like dropping the drm_ prefix from requests). -Daniel --- drivers/gpu/drm/i915/i915_debugfs.c | 37 +- drivers/gpu/drm/i915/i915_drv.h | 108 ++-- drivers/gpu/drm/i915/i915_gem.c | 769 --- drivers/gpu/drm/i915/i915_gem_context.c | 19 +- drivers/gpu/drm/i915/i915_gem_exec.c | 10 +- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 37 +- drivers/gpu/drm/i915/i915_gem_render_state.c | 5 +- drivers/gpu/drm/i915/i915_gem_tiling.c | 2 +- drivers/gpu/drm/i915/i915_gpu_error.c| 35 +- drivers/gpu/drm/i915/i915_irq.c | 6 +- drivers/gpu/drm/i915/i915_perf.c | 6 +- drivers/gpu/drm/i915/i915_trace.h| 2 +- drivers/gpu/drm/i915/intel_display.c | 50 +- drivers/gpu/drm/i915/intel_drv.h | 3 +- drivers/gpu/drm/i915/intel_overlay.c | 118 ++-- drivers/gpu/drm/i915/intel_ringbuffer.c | 83 +-- drivers/gpu/drm/i915/intel_ringbuffer.h | 11 +- 17 files changed, 745 insertions(+), 556 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 406e630..676d5f1 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -122,10 +122,11 @@ static inline const char *get_global_flag(struct drm_i915_gem_object *obj) static void describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) { + struct i915_gem_request *rq = i915_gem_object_last_read(obj); struct i915_vma *vma; int pin_count = 0; - seq_printf(m, %pK: %s%s%s %8zdKiB %02x %02x %u %u %u%s%s%s, + seq_printf(m, %pK: %s%s%s %8zdKiB %02x %02x %x %x %x%s%s%s, obj-base, get_pin_flag(obj), get_tiling_flag(obj), @@ -133,9 +134,9 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) obj-base.size / 1024, obj-base.read_domains, obj-base.write_domain, -obj-last_read_seqno, -obj-last_write_seqno, -obj-last_fenced_seqno, +i915_request_seqno(rq), +i915_request_seqno(obj-last_write.request), +i915_request_seqno(obj-last_fence.request), i915_cache_level_str(obj-cache_level), obj-dirty ? dirty : , obj-madv == I915_MADV_DONTNEED ? purgeable : ); @@ -168,8 +169,8 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) *t = '\0'; seq_printf(m, (%s mappable), s); } - if (obj-ring != NULL) - seq_printf(m, (%s), obj-ring-name); + if (rq) + seq_printf(m, (%s), rq-ring-name); if (obj-frontbuffer_bits) seq_printf(m, (frontbuffer: 0x%03x), obj-frontbuffer_bits); } @@ -336,7 +337,7 @@ static int per_file_stats(int id, void *ptr, void *data) if (ppgtt-ctx ppgtt-ctx-file_priv !=
[Intel-gfx] [RFC] Move BDW workarounds to ring init fn
From: Arun Siluvery arun.siluv...@linux.intel.com This patch moves BDW workarounds from init_clock_gating() to render ring init fn otherwise they are lost when gpu is reset. In case of execlists, some of the workarounds modify registers that are part of register state context which doesn't get initialized until init_clock_gating(); this results in default context with incorrect values as it is restored and saved before updated by workarounds. Open issue: For Wa4x4STCOptimizationDisable, we set CACHE_MODE_1[6:6] = 1 At the time when HW contexts are enabled after rings are initialized with default context this workaround is valid but followed by a context switch this is getting reset, please see below log snippet. ... [5.978209] [drm:i915_pages_create_for_stolen] offset=0x0, size=8294400 [5.978213] [drm:intel_alloc_plane_obj] plane fb obj 8801472e [5.978215] [drm:i915_gem_setup_global_gtt] reserving preallocated space: 0 + 7e9000 [5.978216] [drm:i915_gem_setup_global_gtt] clearing unused GTT space: [7e9000, f000] [5.979613] [drm:i915_gem_init] CACHE_MODE_1: 0x0180 [5.981372] [drm:gen8_ppgtt_init] Allocated 4 pages for page directories (0 wasted) [5.981373] [drm:gen8_ppgtt_init] Allocated 2048 pages for page tables (0 wasted) [5.981376] [drm:i915_gem_context_init] HW context support initialized [5.981462] [drm:i915_gem_init_hw] CACHE_MODE_1: 0x0180 [5.981467] [drm:i915_gem_init_rings] CACHE_MODE_1: 0x0180 [5.981704] [drm:bdw_init_workarounds] CACHE_MODE_1: 0x01C0 [5.981716] [drm:init_status_page] bsd ring hws offset: 0x0081e000 [5.981792] [drm:init_status_page] blitter ring hws offset: 0x0083f000 [5.981910] [drm:init_status_page] video enhancement ring hws offset: 0x0086 [5.982001] [drm:i915_gem_init_hw] CACHE_MODE_1: 0x01C0 [5.982104] [drm:i915_gem_context_enable] Switch render ring to default_context [5.982106] [drm:i915_gem_render_state_init] render ring: Render state init [5.982120] [drm:do_switch] render ring, CACHE_MODE_1: 0x01C0, uninitialized: 1 [5.982121] [drm:i915_gem_context_enable] Switch bsd ring to default_context [5.982122] [drm:do_switch] bsd ring, CACHE_MODE_1: 0x01C0, uninitialized: 0 [5.982123] [drm:i915_gem_context_enable] Switch blitter ring to default_context [5.982126] [drm:do_switch] blitter ring, CACHE_MODE_1: 0x01C0, uninitialized: 0 [5.982126] [drm:i915_gem_context_enable] Switch video enhancement ring to default_context [5.982128] [drm:do_switch] video enhancement ring, CACHE_MODE_1: 0x01C0, uninitialized: 0 [5.982133] [drm:i915_gem_init] CACHE_MODE_1: 0x01C0 [5.982258] [drm:intel_init_clock_gating] ... [ 10.037019] [drm:do_switch] blitter ring, CACHE_MODE_1: 0x0180, uninitialized: 0 ... [ 10.488145] [drm:do_switch] render ring, CACHE_MODE_1: 0x0180, uninitialized: 0 ... I am currently testing this with an igt which triggers a gpu reset and compares WA register contents before and after reset but the test fails because of this register hence not sending it now. Please let me know how to keep this WA valid after a context switch. Arun Siluvery (1): drm/i915/bdw: Initialize BDW workarounds in render ring init fn drivers/gpu/drm/i915/i915_debugfs.c | 46 ++ drivers/gpu/drm/i915/intel_pm.c | 59 drivers/gpu/drm/i915/intel_ringbuffer.c | 68 + 3 files changed, 114 insertions(+), 59 deletions(-) -- 1.9.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC] drm/i915/bdw: Initialize BDW workarounds in render ring init fn
From: Arun Siluvery arun.siluv...@linux.intel.com The workarounds at the moment are initialized in init_clock_gating() but they are lost during reset; In case of execlists some workarounds modify registers that are part of register state context, since these are not initialized until init_clock_gating() default context ends up with incorrect values as render context is restored and saved before updated by workarounds hence move them to render ring init fn. This should be ok as these workarounds are not related to display clock gating. Signed-off-by: Arun Siluvery arun.siluv...@linux.intel.com --- drivers/gpu/drm/i915/i915_debugfs.c | 46 ++ drivers/gpu/drm/i915/intel_pm.c | 59 drivers/gpu/drm/i915/intel_ringbuffer.c | 68 + 3 files changed, 114 insertions(+), 59 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 083683c..cf7da30 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2397,20 +2397,65 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused) seq_printf(m, dpll_md: 0x%08x\n, pll-hw_state.dpll_md); seq_printf(m, fp0: 0x%08x\n, pll-hw_state.fp0); seq_printf(m, fp1: 0x%08x\n, pll-hw_state.fp1); seq_printf(m, wrpll: 0x%08x\n, pll-hw_state.wrpll); } drm_modeset_unlock_all(dev); return 0; } +static int i915_workaround_info(struct seq_file *m, void *unused) +{ + struct drm_info_node *node = (struct drm_info_node *) m-private; + struct drm_device *dev = node-minor-dev; + struct drm_i915_private *dev_priv = dev-dev_private; + int ret; + + ret = mutex_lock_interruptible(dev-struct_mutex); + if (ret) + return ret; + + if (IS_BROADWELL(dev)) { + seq_printf(m, GEN8_ROW_CHICKEN:\t0x%08x\n, + I915_READ(GEN8_ROW_CHICKEN)); + seq_printf(m, HALF_SLICE_CHICKEN3:\t0x%08x\n, + I915_READ(HALF_SLICE_CHICKEN3)); + seq_printf(m, GAMTARBMODE:\t0x%08x\n, I915_READ(GAMTARBMODE)); + seq_printf(m, _3D_CHICKEN3:\t0x%08x\n, + I915_READ(_3D_CHICKEN3)); + seq_printf(m, COMMON_SLICE_CHICKEN2:\t0x%08x\n, + I915_READ(COMMON_SLICE_CHICKEN2)); + seq_printf(m, GEN7_HALF_SLICE_CHICKEN1:\t0x%08x\n, + I915_READ(GEN7_HALF_SLICE_CHICKEN1)); + seq_printf(m, GEN7_ROW_CHICKEN2:\t0x%08x\n, + I915_READ(GEN7_ROW_CHICKEN2)); + seq_printf(m, GAM_ECOCHK:\t0x%08x\n, + I915_READ(GAM_ECOCHK)); + seq_printf(m, HDC_CHICKEN0:\t0x%08x\n, + I915_READ(HDC_CHICKEN0)); + seq_printf(m, GEN7_FF_THREAD_MODE:\t0x%08x\n, + I915_READ(GEN7_FF_THREAD_MODE)); + seq_printf(m, GEN8_UCGCTL6:\t0x%08x\n, + I915_READ(GEN8_UCGCTL6)); + seq_printf(m, GEN6_RC_SLEEP_PSMI_CONTROL:\t0x%08x\n, + I915_READ(GEN6_RC_SLEEP_PSMI_CONTROL)); + seq_printf(m, CACHE_MODE_1:\t0x%08x\n, + I915_READ(CACHE_MODE_1)); + } else + DRM_DEBUG_DRIVER(Not available for Gen%d\n, +INTEL_INFO(dev)-gen); + + mutex_unlock(dev-struct_mutex); + return 0; +} + struct pipe_crc_info { const char *name; struct drm_device *dev; enum pipe pipe; }; static int i915_pipe_crc_open(struct inode *inode, struct file *filep) { struct pipe_crc_info *info = inode-i_private; struct drm_i915_private *dev_priv = info-dev-dev_private; @@ -3904,20 +3949,21 @@ static const struct drm_info_list i915_debugfs_list[] = { {i915_ppgtt_info, i915_ppgtt_info, 0}, {i915_llc, i915_llc, 0}, {i915_edp_psr_status, i915_edp_psr_status, 0}, {i915_sink_crc_eDP1, i915_sink_crc, 0}, {i915_energy_uJ, i915_energy_uJ, 0}, {i915_pc8_status, i915_pc8_status, 0}, {i915_power_domain_info, i915_power_domain_info, 0}, {i915_display_info, i915_display_info, 0}, {i915_semaphore_status, i915_semaphore_status, 0}, {i915_shared_dplls_info, i915_shared_dplls_info, 0}, + {i915_workaround_info, i915_workaround_info, 0}, }; #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list) static const struct i915_debugfs_files { const char *name; const struct file_operations *fops; } i915_debugfs_files[] = { {i915_wedged, i915_wedged_fops}, {i915_max_freq, i915_max_freq_fops}, {i915_min_freq, i915_min_freq_fops}, diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
Re: [Intel-gfx] [RFC] drm/i915/bdw: Initialize BDW workarounds in render ring init fn
On Mon, Jul 28, 2014 at 05:31:46PM +0100, arun.siluv...@linux.intel.com wrote: From: Arun Siluvery arun.siluv...@linux.intel.com The workarounds at the moment are initialized in init_clock_gating() but they are lost during reset; In case of execlists some workarounds modify registers that are part of register state context, since these are not initialized until init_clock_gating() default context ends up with incorrect values as render context is restored and saved before updated by workarounds hence move them to render ring init fn. This should be ok as these workarounds are not related to display clock gating. Signed-off-by: Arun Siluvery arun.siluv...@linux.intel.com --- drivers/gpu/drm/i915/i915_debugfs.c | 46 ++ drivers/gpu/drm/i915/intel_pm.c | 59 drivers/gpu/drm/i915/intel_ringbuffer.c | 68 + 3 files changed, 114 insertions(+), 59 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 083683c..cf7da30 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2397,20 +2397,65 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused) seq_printf(m, dpll_md: 0x%08x\n, pll-hw_state.dpll_md); seq_printf(m, fp0: 0x%08x\n, pll-hw_state.fp0); seq_printf(m, fp1: 0x%08x\n, pll-hw_state.fp1); seq_printf(m, wrpll: 0x%08x\n, pll-hw_state.wrpll); } drm_modeset_unlock_all(dev); return 0; } +static int i915_workaround_info(struct seq_file *m, void *unused) +{ + struct drm_info_node *node = (struct drm_info_node *) m-private; + struct drm_device *dev = node-minor-dev; + struct drm_i915_private *dev_priv = dev-dev_private; + int ret; + + ret = mutex_lock_interruptible(dev-struct_mutex); + if (ret) + return ret; + + if (IS_BROADWELL(dev)) { + seq_printf(m, GEN8_ROW_CHICKEN:\t0x%08x\n, +I915_READ(GEN8_ROW_CHICKEN)); + seq_printf(m, HALF_SLICE_CHICKEN3:\t0x%08x\n, +I915_READ(HALF_SLICE_CHICKEN3)); + seq_printf(m, GAMTARBMODE:\t0x%08x\n, I915_READ(GAMTARBMODE)); + seq_printf(m, _3D_CHICKEN3:\t0x%08x\n, +I915_READ(_3D_CHICKEN3)); + seq_printf(m, COMMON_SLICE_CHICKEN2:\t0x%08x\n, +I915_READ(COMMON_SLICE_CHICKEN2)); + seq_printf(m, GEN7_HALF_SLICE_CHICKEN1:\t0x%08x\n, +I915_READ(GEN7_HALF_SLICE_CHICKEN1)); + seq_printf(m, GEN7_ROW_CHICKEN2:\t0x%08x\n, +I915_READ(GEN7_ROW_CHICKEN2)); + seq_printf(m, GAM_ECOCHK:\t0x%08x\n, +I915_READ(GAM_ECOCHK)); + seq_printf(m, HDC_CHICKEN0:\t0x%08x\n, +I915_READ(HDC_CHICKEN0)); + seq_printf(m, GEN7_FF_THREAD_MODE:\t0x%08x\n, +I915_READ(GEN7_FF_THREAD_MODE)); + seq_printf(m, GEN8_UCGCTL6:\t0x%08x\n, +I915_READ(GEN8_UCGCTL6)); + seq_printf(m, GEN6_RC_SLEEP_PSMI_CONTROL:\t0x%08x\n, +I915_READ(GEN6_RC_SLEEP_PSMI_CONTROL)); + seq_printf(m, CACHE_MODE_1:\t0x%08x\n, +I915_READ(CACHE_MODE_1)); + } else + DRM_DEBUG_DRIVER(Not available for Gen%d\n, + INTEL_INFO(dev)-gen); + + mutex_unlock(dev-struct_mutex); + return 0; +} + This smells like a separate patch. But I'm not sure we want at all since intel_reg_read will provide the same information. struct pipe_crc_info { const char *name; struct drm_device *dev; enum pipe pipe; }; static int i915_pipe_crc_open(struct inode *inode, struct file *filep) { struct pipe_crc_info *info = inode-i_private; struct drm_i915_private *dev_priv = info-dev-dev_private; @@ -3904,20 +3949,21 @@ static const struct drm_info_list i915_debugfs_list[] = { {i915_ppgtt_info, i915_ppgtt_info, 0}, {i915_llc, i915_llc, 0}, {i915_edp_psr_status, i915_edp_psr_status, 0}, {i915_sink_crc_eDP1, i915_sink_crc, 0}, {i915_energy_uJ, i915_energy_uJ, 0}, {i915_pc8_status, i915_pc8_status, 0}, {i915_power_domain_info, i915_power_domain_info, 0}, {i915_display_info, i915_display_info, 0}, {i915_semaphore_status, i915_semaphore_status, 0}, {i915_shared_dplls_info, i915_shared_dplls_info, 0}, + {i915_workaround_info, i915_workaround_info, 0}, }; #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list) static const struct i915_debugfs_files { const char *name; const struct file_operations *fops; } i915_debugfs_files[] = { {i915_wedged,
Re: [Intel-gfx] [PATCH] drm/i915: Rework GPU reset sequence to match driver load thaw
Hi Ben / Daniel, I agree that this needs to be properly tested. Are there any particular igt tests you would suggest I use? I've been running: drv_hangman, drv_suspend, gem_hangcheck_forcewake. Also do you have a set of PPGTT Patches that should work with these tests. Michel sent me a set of patches to enable PPGTT, but these 3 tests fail with the patches. Thanks, Alistair. -Original Message- From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Monday, July 28, 2014 10:27 AM To: Ben Widawsky Cc: Mcaulay, Alistair; intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] drm/i915: Rework GPU reset sequence to match driver load thaw On Fri, Jul 25, 2014 at 06:05:29PM -0700, Ben Widawsky wrote: On Wed, Jul 16, 2014 at 04:05:59PM +0100, alistair.mcau...@intel.com wrote: From: McAulay, Alistair alistair.mcau...@intel.com This patch is to address Daniels concerns over different code during reset: http://lists.freedesktop.org/archives/intel-gfx/2014-June/047758.htm l The reason for aiming as hard as possible to use the exact same code for driver load, gpu reset and runtime pm/system resume is that we've simply seen too many bugs due to slight variations and unintended omissions. Tested using igt drv_hangman. Signed-off-by: McAulay, Alistair alistair.mcau...@intel.com 2 quick comments before I actually do a real review. 1. Did you actually run this with and without full ppgtt? 2. I don't think this is actually fulfilling what Daniel is requesting, though we can let him comment. Mostly looks like what I think we need. Comments below. 3. Did you reall do #1? Assuming you satisifed #1, can you please post the igt results for the permutations (pre patch w/ and w/o ppgtt; post patch w/ and w/o ppgtt) I really want this data because I spent *a lot* of time with these specific areas in the PPGTT work, and I am somewhat skeptical enough of the code has changed that this will magically work. I also tried the trickiness with the ring handling functions, and never succeeded. Also, with the context stuff, I'm simply not convinced it can magically vanish. If igt looks good, and Daniel agrees that this is what he actually wanted, I will go fishing for corner cases and do the review. I think the hack in ring_begin might explain why it never worked before. But fully agreed, we really need to test this well (and fill gaps if igt misses anything around resets - we don't have any systematic gpu reset coverage anywhere outside of igt). Thanks. --- drivers/gpu/drm/i915/i915_gem.c | 2 - drivers/gpu/drm/i915/i915_gem_context.c | 42 + drivers/gpu/drm/i915/i915_gem_gtt.c | 67 + drivers/gpu/drm/i915/i915_gem_gtt.h | 3 +- drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +- 5 files changed, 14 insertions(+), 104 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index ef047bc..b38e086 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2590,8 +2590,6 @@ void i915_gem_reset(struct drm_device *dev) for_each_ring(ring, dev_priv, i) i915_gem_reset_ring_cleanup(dev_priv, ring); - i915_gem_context_reset(dev); - i915_gem_restore_fences(dev); } diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index de72a28..d96219f 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -372,42 +372,6 @@ err_destroy: return ERR_PTR(ret); } -void i915_gem_context_reset(struct drm_device *dev) -{ - struct drm_i915_private *dev_priv = dev-dev_private; - int i; - - /* Prevent the hardware from restoring the last context (which hung) on -* the next switch */ - for (i = 0; i I915_NUM_RINGS; i++) { - struct intel_engine_cs *ring = dev_priv-ring[i]; - struct intel_context *dctx = ring-default_context; - struct intel_context *lctx = ring-last_context; - - /* Do a fake switch to the default context */ - if (lctx == dctx) - continue; - - if (!lctx) - continue; - - if (dctx-legacy_hw_ctx.rcs_state i == RCS) { - WARN_ON(i915_gem_obj_ggtt_pin(dctx-legacy_hw_ctx.rcs_state, - get_context_alignment(dev), 0)); - /* Fake a finish/inactive */ - dctx-legacy_hw_ctx.rcs_state-base.write_domain = 0; - dctx-legacy_hw_ctx.rcs_state-active = 0; - } - - if (lctx-legacy_hw_ctx.rcs_state i == RCS) - i915_gem_object_ggtt_unpin(lctx-legacy_hw_ctx.rcs_state); - -
Re: [Intel-gfx] [RFC] Move BDW workarounds to ring init fn
On Mon, Jul 28, 2014 at 05:31:45PM +0100, arun.siluv...@linux.intel.com wrote: From: Arun Siluvery arun.siluv...@linux.intel.com This patch moves BDW workarounds from init_clock_gating() to render ring init fn otherwise they are lost when gpu is reset. In case of execlists, some of the workarounds modify registers that are part of register state context which doesn't get initialized until init_clock_gating(); this results in default context with incorrect values as it is restored and saved before updated by workarounds. I don't think it has to do with execlists. Many of the registers are part of the context image even in ring buffer mode AFAIK. Open issue: For Wa4x4STCOptimizationDisable, we set CACHE_MODE_1[6:6] = 1 At the time when HW contexts are enabled after rings are initialized with default context this workaround is valid but followed by a context switch this is getting reset, please see below log snippet. This is a bit weird. The default context should have restore inhibit==1 so it shouldn't clobber the CACHE_MODE_1 register. There was a specific magic dance you're supposed to do when accessing such registers with mmio, but here we do the write even before the first context switch. Apparently there was some kind of problem with CACHE_MODE_0 on snb too: commit 3a69ddd6f872180b6f61fda87152b37202118fbc Author: Kenneth Graunke kenn...@whitecape.org Date: Fri Apr 27 12:44:41 2012 -0700 drm/i915: Set the Stencil Cache eviction policy to non-LRA mode. but IIRC I wasn't able to reproduce it when I tried. Maybe we need to delay these register writes until we've switched to the default context? ... [5.978209] [drm:i915_pages_create_for_stolen] offset=0x0, size=8294400 [5.978213] [drm:intel_alloc_plane_obj] plane fb obj 8801472e [5.978215] [drm:i915_gem_setup_global_gtt] reserving preallocated space: 0 + 7e9000 [5.978216] [drm:i915_gem_setup_global_gtt] clearing unused GTT space: [7e9000, f000] [5.979613] [drm:i915_gem_init] CACHE_MODE_1: 0x0180 [5.981372] [drm:gen8_ppgtt_init] Allocated 4 pages for page directories (0 wasted) [5.981373] [drm:gen8_ppgtt_init] Allocated 2048 pages for page tables (0 wasted) [5.981376] [drm:i915_gem_context_init] HW context support initialized [5.981462] [drm:i915_gem_init_hw] CACHE_MODE_1: 0x0180 [5.981467] [drm:i915_gem_init_rings] CACHE_MODE_1: 0x0180 [5.981704] [drm:bdw_init_workarounds] CACHE_MODE_1: 0x01C0 [5.981716] [drm:init_status_page] bsd ring hws offset: 0x0081e000 [5.981792] [drm:init_status_page] blitter ring hws offset: 0x0083f000 [5.981910] [drm:init_status_page] video enhancement ring hws offset: 0x0086 [5.982001] [drm:i915_gem_init_hw] CACHE_MODE_1: 0x01C0 [5.982104] [drm:i915_gem_context_enable] Switch render ring to default_context [5.982106] [drm:i915_gem_render_state_init] render ring: Render state init [5.982120] [drm:do_switch] render ring, CACHE_MODE_1: 0x01C0, uninitialized: 1 [5.982121] [drm:i915_gem_context_enable] Switch bsd ring to default_context [5.982122] [drm:do_switch] bsd ring, CACHE_MODE_1: 0x01C0, uninitialized: 0 [5.982123] [drm:i915_gem_context_enable] Switch blitter ring to default_context [5.982126] [drm:do_switch] blitter ring, CACHE_MODE_1: 0x01C0, uninitialized: 0 [5.982126] [drm:i915_gem_context_enable] Switch video enhancement ring to default_context [5.982128] [drm:do_switch] video enhancement ring, CACHE_MODE_1: 0x01C0, uninitialized: 0 [5.982133] [drm:i915_gem_init] CACHE_MODE_1: 0x01C0 [5.982258] [drm:intel_init_clock_gating] ... [ 10.037019] [drm:do_switch] blitter ring, CACHE_MODE_1: 0x0180, uninitialized: 0 ... [ 10.488145] [drm:do_switch] render ring, CACHE_MODE_1: 0x0180, uninitialized: 0 ... I am currently testing this with an igt which triggers a gpu reset and compares WA register contents before and after reset but the test fails because of this register hence not sending it now. Please let me know how to keep this WA valid after a context switch. Arun Siluvery (1): drm/i915/bdw: Initialize BDW workarounds in render ring init fn drivers/gpu/drm/i915/i915_debugfs.c | 46 ++ drivers/gpu/drm/i915/intel_pm.c | 59 drivers/gpu/drm/i915/intel_ringbuffer.c | 68 + 3 files changed, 114 insertions(+), 59 deletions(-) -- 1.9.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.
From: Sagar Kamble sagar.a.kam...@intel.com Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in PM suspend and resume path similar to runtime suspend and resume. Cc: Imre Deak imre.d...@intel.com Cc: Paulo Zanoni paulo.r.zan...@intel.com Cc: Daniel Vetter daniel.vet...@ffwll.ch Cc: Jani Nikula jani.nik...@linux.intel.com Signed-off-by: Sagar Kamble sagar.a.kam...@intel.com --- drivers/gpu/drm/i915/i915_drv.c | 84 drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_drv.h | 7 3 files changed, 85 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 6c4b25c..fdfeccf 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -495,6 +495,26 @@ static int i915_drm_freeze(struct drm_device *dev) struct drm_i915_private *dev_priv = dev-dev_private; struct drm_crtc *crtc; pci_power_t opregion_target_state; + u32 mask; + int err = 0; + + /* Following sequence from vlv_runtime_suspend */ + if (IS_VALLEYVIEW(dev)) { + /* +* Bspec defines the following GT well on flags as debug only, +* so don't treat them as hard failures. +*/ + (void)vlv_wait_for_gt_wells(dev_priv, false); + + mask = VLV_GTLC_RENDER_CTX_EXISTS | VLV_GTLC_MEDIA_CTX_EXISTS; + WARN_ON((I915_READ(VLV_GTLC_WAKE_CTRL) mask) != mask); + + vlv_check_no_gt_access(dev_priv); + + err = vlv_force_gfx_clock(dev_priv, true); + if (err) + goto err1; + } /* ignore lid events during suspend */ mutex_lock(dev_priv-modeset_restore_lock); @@ -542,6 +562,10 @@ static int i915_drm_freeze(struct drm_device *dev) i915_gem_suspend_gtt_mappings(dev); + /* Save Gunit State */ + if (IS_VALLEYVIEW(dev)) + vlv_save_gunit_s0ix_state(dev_priv); + i915_save_state(dev); opregion_target_state = PCI_D3cold; @@ -562,7 +586,27 @@ static int i915_drm_freeze(struct drm_device *dev) intel_display_set_init_power(dev_priv, false); - return 0; + /* Clear Allow Wake Bit so that none of the +* force/demand wake requests +*/ + if (IS_VALLEYVIEW(dev)) { + err = vlv_allow_gt_wake(dev_priv, false); + if (err) + goto err2; + + /* Release graphics clocks */ + vlv_force_gfx_clock(dev_priv, false); + } + return err; +err2: + /* For safety always re-enable waking and disable gfx clock forcing */ + if (IS_VALLEYVIEW(dev)) + vlv_allow_gt_wake(dev_priv, true); +err1: + if (IS_VALLEYVIEW(dev)) + vlv_force_gfx_clock(dev_priv, false); + + return err; } int i915_suspend(struct drm_device *dev, pm_message_t state) @@ -610,6 +654,26 @@ void intel_console_resume(struct work_struct *work) static int i915_drm_thaw_early(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; + int ret = 0; + int err; + + /* +* Following sequence from vlv_runtime_resume. Clock is released +* in i915_drm_thaw. +* If any of the steps fail just try to continue, that's the best we +* can do at this point. Return the first error code (which will also +* leave RPM permanently disabled). +*/ + + if (IS_VALLEYVIEW(dev)) { + ret = vlv_force_gfx_clock(dev_priv, true); + + vlv_restore_gunit_s0ix_state(dev_priv); + + err = vlv_allow_gt_wake(dev_priv, true); + if (!ret) + ret = err; + } if (IS_HASWELL(dev) || IS_BROADWELL(dev)) hsw_disable_pc8(dev_priv); @@ -618,7 +682,7 @@ static int i915_drm_thaw_early(struct drm_device *dev) intel_uncore_sanitize(dev); intel_power_domains_init_hw(dev_priv); - return 0; + return ret; } static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) @@ -695,6 +759,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) intel_opregion_notify_adapter(dev, PCI_D0); + /* Release graphics clocks turned on in thaw_early*/ + vlv_force_gfx_clock(dev_priv, false); + return 0; } @@ -1028,7 +1095,7 @@ static int hsw_runtime_resume(struct drm_i915_private *dev_priv) * a black-box for the driver. Further investigation is needed to reduce the * saved/restored registers even further, by following the same 3 criteria. */ -static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv) +void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv) { struct vlv_s0ix_state *s =
Re: [Intel-gfx] [PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.
On Mon, 2014-07-28 at 23:07 +0530, sagar.a.kam...@intel.com wrote: From: Sagar Kamble sagar.a.kam...@intel.com Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in PM suspend and resume path similar to runtime suspend and resume. Cc: Imre Deak imre.d...@intel.com Cc: Paulo Zanoni paulo.r.zan...@intel.com Cc: Daniel Vetter daniel.vet...@ffwll.ch Cc: Jani Nikula jani.nik...@linux.intel.com Signed-off-by: Sagar Kamble sagar.a.kam...@intel.com --- drivers/gpu/drm/i915/i915_drv.c | 84 drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_drv.h | 7 3 files changed, 85 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 6c4b25c..fdfeccf 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -495,6 +495,26 @@ static int i915_drm_freeze(struct drm_device *dev) struct drm_i915_private *dev_priv = dev-dev_private; struct drm_crtc *crtc; pci_power_t opregion_target_state; + u32 mask; + int err = 0; + + /* Following sequence from vlv_runtime_suspend */ + if (IS_VALLEYVIEW(dev)) { + /* + * Bspec defines the following GT well on flags as debug only, + * so don't treat them as hard failures. + */ + (void)vlv_wait_for_gt_wells(dev_priv, false); + + mask = VLV_GTLC_RENDER_CTX_EXISTS | VLV_GTLC_MEDIA_CTX_EXISTS; + WARN_ON((I915_READ(VLV_GTLC_WAKE_CTRL) mask) != mask); + + vlv_check_no_gt_access(dev_priv); Above piece of code may not be at the right place since during suspend operation there might be workload on wells. Added to make it analogous to runtime suspend. Moving it below gem_suspend will be proper way. Is my understanding correct? + + err = vlv_force_gfx_clock(dev_priv, true); + if (err) + goto err1; + } /* ignore lid events during suspend */ mutex_lock(dev_priv-modeset_restore_lock); @@ -542,6 +562,10 @@ static int i915_drm_freeze(struct drm_device *dev) i915_gem_suspend_gtt_mappings(dev); + /* Save Gunit State */ + if (IS_VALLEYVIEW(dev)) + vlv_save_gunit_s0ix_state(dev_priv); + i915_save_state(dev); opregion_target_state = PCI_D3cold; @@ -562,7 +586,27 @@ static int i915_drm_freeze(struct drm_device *dev) intel_display_set_init_power(dev_priv, false); - return 0; + /* Clear Allow Wake Bit so that none of the + * force/demand wake requests + */ + if (IS_VALLEYVIEW(dev)) { + err = vlv_allow_gt_wake(dev_priv, false); + if (err) + goto err2; + + /* Release graphics clocks */ + vlv_force_gfx_clock(dev_priv, false); + } + return err; +err2: + /* For safety always re-enable waking and disable gfx clock forcing */ + if (IS_VALLEYVIEW(dev)) + vlv_allow_gt_wake(dev_priv, true); +err1: + if (IS_VALLEYVIEW(dev)) + vlv_force_gfx_clock(dev_priv, false); + + return err; } int i915_suspend(struct drm_device *dev, pm_message_t state) @@ -610,6 +654,26 @@ void intel_console_resume(struct work_struct *work) static int i915_drm_thaw_early(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; + int ret = 0; + int err; + + /* + * Following sequence from vlv_runtime_resume. Clock is released + * in i915_drm_thaw. + * If any of the steps fail just try to continue, that's the best we + * can do at this point. Return the first error code (which will also + * leave RPM permanently disabled). + */ + + if (IS_VALLEYVIEW(dev)) { + ret = vlv_force_gfx_clock(dev_priv, true); + + vlv_restore_gunit_s0ix_state(dev_priv); + + err = vlv_allow_gt_wake(dev_priv, true); + if (!ret) + ret = err; + } if (IS_HASWELL(dev) || IS_BROADWELL(dev)) hsw_disable_pc8(dev_priv); @@ -618,7 +682,7 @@ static int i915_drm_thaw_early(struct drm_device *dev) intel_uncore_sanitize(dev); intel_power_domains_init_hw(dev_priv); - return 0; + return ret; } static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) @@ -695,6 +759,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) intel_opregion_notify_adapter(dev, PCI_D0); + /* Release graphics clocks turned on in thaw_early*/ + vlv_force_gfx_clock(dev_priv, false); + return 0; } @@ -1028,7 +1095,7 @@ static int hsw_runtime_resume(struct drm_i915_private *dev_priv) * a black-box for the driver. Further
[Intel-gfx] [PATCH 0/3] Fixes for runtime PM on planes APIs
From: Paulo Zanoni paulo.r.zan...@intel.com Hi This series fixes some bugs that happen when we're runtime suspended and try to use the planes APIs. I also wrote IGT test cases for the bugs, so we will be able to detect future regressions. The controversial part of these patches is that we had previously defined that we wanted to get/put runtime PM in the highest level of the stack, wrapping as much code as possible, but Daniel asked me to only get/put runtime PM around the functions that pin the objects (still on the highest level, but only around the pin functions). This series implements Daniel's suggestions. Thanks, Paulo Paulo Zanoni (3): drm/i915: fix cursor handling when runtime suspended drm/i915: get runtime PM when pinning sprite objects drm/i915: get runtime PM when pinning primary plane objects drivers/gpu/drm/i915/intel_display.c | 9 + drivers/gpu/drm/i915/intel_sprite.c | 3 +++ 2 files changed, 12 insertions(+) -- 2.0.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] tests/pm_rpm: add cursor subtests
From: Paulo Zanoni paulo.r.zan...@intel.com These tests currently trigger WARNs on our Kernel. Let's make sure we fix the bugs and they never come back. v2: Reorganize the code a little bit, and improve the tests. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- tests/pm_rpm.c | 123 - 1 file changed, 113 insertions(+), 10 deletions(-) diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c index 869e6f3..f720f86 100644 --- a/tests/pm_rpm.c +++ b/tests/pm_rpm.c @@ -277,13 +277,15 @@ static uint32_t get_fb(struct mode_set_data *data, int width, int height) igt_assert(false); } -static bool enable_one_screen_with_type(struct mode_set_data *data, - enum screen_type type) +static bool find_connector_for_modeset(struct mode_set_data *data, + enum screen_type type, + uint32_t *connector_id, + drmModeModeInfoPtr *mode) { - uint32_t crtc_id = 0, buffer_id = 0, connector_id = 0; - drmModeModeInfoPtr mode = NULL; - int i, rc; + int i; + *connector_id = 0; + *mode = NULL; for (i = 0; i data-res-count_connectors; i++) { drmModeConnectorPtr c = data-connectors[i]; @@ -296,13 +298,23 @@ static bool enable_one_screen_with_type(struct mode_set_data *data, continue; if (c-connection == DRM_MODE_CONNECTED c-count_modes) { - connector_id = c-connector_id; - mode = c-modes[0]; + *connector_id = c-connector_id; + *mode = c-modes[0]; break; } } - if (connector_id == 0) + return (*connector_id != 0); +} + +static bool enable_one_screen_with_type(struct mode_set_data *data, + enum screen_type type) +{ + uint32_t crtc_id = 0, buffer_id = 0, connector_id; + drmModeModeInfoPtr mode; + int rc; + + if (!find_connector_for_modeset(data, type, connector_id, mode)) return false; crtc_id = data-res-crtcs[0]; @@ -310,8 +322,6 @@ static bool enable_one_screen_with_type(struct mode_set_data *data, igt_assert(crtc_id); igt_assert(buffer_id); - igt_assert(connector_id); - igt_assert(mode); rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0, connector_id, 1, mode); @@ -1407,6 +1417,93 @@ static void dpms_mode_unset_subtest(enum screen_type type) igt_assert(wait_for_suspended()); } +static void fill_igt_fb(struct igt_fb *fb, uint32_t color) +{ + int i; + uint32_t *ptr; + + ptr = gem_mmap__cpu(drm_fd, fb-gem_handle, fb-size, 0); + for (i = 0; i fb-size/sizeof(uint32_t); i++) + ptr[i] = color; + igt_assert(munmap(ptr, fb-size) == 0); +} + +/* At some point, this test triggered WARNs in the Kernel. */ +static void cursor_subtest(bool dpms) +{ + uint32_t crtc_id = 0, connector_id; + drmModeModeInfoPtr mode; + int rc; + struct igt_fb scanout_fb, cursor_fb1, cursor_fb2; + + disable_all_screens(ms_data); + igt_assert(wait_for_suspended()); + + igt_require(find_connector_for_modeset(ms_data, SCREEN_TYPE_ANY, + connector_id, mode)); + + crtc_id = ms_data.res-crtcs[0]; + igt_assert(crtc_id); + + igt_create_fb(drm_fd, mode-hdisplay, mode-vdisplay, + DRM_FORMAT_XRGB, false, scanout_fb); + igt_create_fb(drm_fd, 64, 64, DRM_FORMAT_ARGB, false, cursor_fb1); + igt_create_fb(drm_fd, 64, 64, DRM_FORMAT_ARGB, false, cursor_fb2); + + fill_igt_fb(scanout_fb, 0xFF); + fill_igt_fb(cursor_fb1, 0xFF00); + fill_igt_fb(cursor_fb2, 0xFF00FF00); + + rc = drmModeSetCrtc(drm_fd, crtc_id, scanout_fb.fb_id, 0, 0, + connector_id, 1, mode); + igt_assert(rc == 0); + igt_assert(wait_for_active()); + + rc = drmModeSetCursor(drm_fd, crtc_id, cursor_fb1.gem_handle, + cursor_fb1.width, cursor_fb1.height); + igt_assert(rc == 0); + rc = drmModeMoveCursor(drm_fd, crtc_id, 0, 0); + igt_assert(rc == 0); + igt_assert(wait_for_active()); + + if (dpms) + disable_all_screens_dpms(ms_data); + else + disable_all_screens(ms_data); + igt_assert(wait_for_suspended()); + + /* First, just move the cursor. */ + rc = drmModeMoveCursor(drm_fd, crtc_id, 1, 1); + igt_assert(rc == 0); + igt_assert(wait_for_suspended()); + + /* Then unset it, and set a new one. */ + rc = drmModeSetCursor(drm_fd, crtc_id, 0, 0, 0); + igt_assert(rc == 0); +
[Intel-gfx] [PATCH 1/3] drm/i915: fix cursor handling when runtime suspended
From: Paulo Zanoni paulo.r.zan...@intel.com If we're runtime suspended and try to use the cursor interfaces, we will get a lot of WARNs saying we did the wrong thing. For intel_crtc_update_cursor(), all we need to do is return if the CRTC is not active, since writing the registers won't really have any effect if the screen is not visible, and we will write the registers later when enabling the screen. For intel_crtc_cursor_set_obj(), we just need to wake up the hardware then pinning the display plane. v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel) Testcase: igt/pm_rpm/cursor Testcase: igt/pm_rpm/cursor-dpms Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645 Cc: sta...@vger.kernel.org Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/intel_display.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1edfd1a..f1a9b5c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8144,6 +8144,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, if (base == 0 intel_crtc-cursor_base == 0) return; + if (!intel_crtc-active) + return; + I915_WRITE(CURPOS(pipe), pos); if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev)) @@ -8217,7 +8220,9 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, if (need_vtd_wa(dev)) alignment = 64*1024; + intel_runtime_pm_get(dev_priv); ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL); + intel_runtime_pm_put(dev_priv); if (ret) { DRM_DEBUG_KMS(failed to move cursor bo into the GTT\n); goto fail_locked; -- 2.0.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] tests/pm_rpm: add planes subtests
From: Paulo Zanoni paulo.r.zan...@intel.com Just like the cursor subtests, these also trigger WARNs on the current Kernel. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- tests/pm_rpm.c | 212 - 1 file changed, 211 insertions(+), 1 deletion(-) diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c index f720f86..048d9ad 100644 --- a/tests/pm_rpm.c +++ b/tests/pm_rpm.c @@ -51,6 +51,9 @@ #include igt_kms.h #include igt_debugfs.h +/* One day, this will be on your libdrm. */ +#define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2 + #define MSR_PC8_RES0x630 #define MSR_PC9_RES0x631 #define MSR_PC10_RES 0x632 @@ -72,6 +75,12 @@ enum screen_type { SCREEN_TYPE_ANY, }; +enum plane_type { + PLANE_OVERLAY, + PLANE_PRIMARY, + PLANE_CURSOR, +}; + /* Wait flags */ #define DONT_WAIT 0 #define WAIT_STATUS1 @@ -1504,6 +1513,199 @@ static void cursor_subtest(bool dpms) igt_assert(wait_for_suspended()); } +static enum plane_type get_plane_type(uint32_t plane_id) +{ + drmModeObjectPropertiesPtr props; + int i, j; + enum plane_type type; + bool found = false; + + props = drmModeObjectGetProperties(drm_fd, plane_id, + DRM_MODE_OBJECT_PLANE); + igt_assert(props); + + for (i = 0; i props-count_props !found; i++) { + drmModePropertyPtr prop; + const char *enum_name = NULL; + + prop = drmModeGetProperty(drm_fd, props-props[i]); + igt_assert(prop); + + if (strcmp(prop-name, type) == 0) { + igt_assert(prop-flags DRM_MODE_PROP_ENUM); + igt_assert(props-prop_values[i] prop-count_enums); + + for (j = 0; j prop-count_enums; j++) { + if (prop-enums[j].value == + props-prop_values[i]) { + enum_name = prop-enums[j].name; + break; + } + } + igt_assert(enum_name); + + if (strcmp(enum_name, Overlay) == 0) + type = PLANE_OVERLAY; + else if (strcmp(enum_name, Primary) == 0) + type = PLANE_PRIMARY; + else if (strcmp(enum_name, Cursor) == 0) + type = PLANE_CURSOR; + else + igt_assert(0); + + found = true; + } + + drmModeFreeProperty(prop); + } + igt_assert(found); + + drmModeFreeObjectProperties(props); + return type; +} + +static void test_one_plane(bool dpms, uint32_t plane_id, + enum plane_type plane_type) +{ + int rc; + uint32_t plane_format, plane_w, plane_h; + uint32_t crtc_id, connector_id; + struct igt_fb scanout_fb, plane_fb1, plane_fb2; + drmModeModeInfoPtr mode; + int32_t crtc_x = 0, crtc_y = 0; + + disable_all_screens(ms_data); + igt_assert(wait_for_suspended()); + + igt_require(find_connector_for_modeset(ms_data, SCREEN_TYPE_ANY, + connector_id, mode)); + + crtc_id = ms_data.res-crtcs[0]; + igt_assert(crtc_id); + + igt_create_fb(drm_fd, mode-hdisplay, mode-vdisplay, + DRM_FORMAT_XRGB, false, scanout_fb); + + fill_igt_fb(scanout_fb, 0xFF); + + switch (plane_type) { + case PLANE_OVERLAY: + plane_format = DRM_FORMAT_XRGB; + plane_w = 64; + plane_h = 64; + break; + case PLANE_PRIMARY: + plane_format = DRM_FORMAT_XRGB; + plane_w = mode-hdisplay; + plane_h = mode-vdisplay; + break; + case PLANE_CURSOR: + plane_format = DRM_FORMAT_ARGB; + plane_w = 64; + plane_h = 64; + break; + default: + igt_assert(0); + break; + } + + igt_create_fb(drm_fd, plane_w, plane_h, plane_format, false, + plane_fb1); + igt_create_fb(drm_fd, plane_w, plane_h, plane_format, false, + plane_fb2); + fill_igt_fb(plane_fb1, 0xFF00); + fill_igt_fb(plane_fb2, 0xFF00FF00); + + rc = drmModeSetCrtc(drm_fd, crtc_id, scanout_fb.fb_id, 0, 0, + connector_id, 1, mode); + igt_assert(rc == 0); + igt_assert(wait_for_active()); + + rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0, +0, 0, plane_fb1.width, plane_fb1.height, +0 16, 0 16, plane_fb1.width 16, +
[Intel-gfx] [PATCH 3/3] drm/i915: get runtime PM when pinning primary plane objects
From: Paulo Zanoni paulo.r.zan...@intel.com Otherwise we may get WARNs saying we're writing registers while runtime suspended. Testcase: igt/pm_rpm/universal-planes Testcase: igt/pm_rpm/universal-planes-dpms Cc: sta...@vger.kernel.org Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/intel_display.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f1a9b5c..5948207 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11531,7 +11531,9 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc, INTEL_FRONTBUFFER_PRIMARY(intel_crtc-pipe)); /* Pin and return without programming hardware */ + intel_runtime_pm_get(dev_priv); ret = intel_pin_and_fence_fb_obj(dev, obj, NULL); + intel_runtime_pm_put(dev_priv); mutex_unlock(dev-struct_mutex); return ret; @@ -11553,7 +11555,9 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc, * fail. */ if (plane-fb != fb) { + intel_runtime_pm_get(dev_priv); ret = intel_pin_and_fence_fb_obj(dev, obj, NULL); + intel_runtime_pm_put(dev_priv); if (ret) { mutex_unlock(dev-struct_mutex); return ret; -- 2.0.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/3] drm/i915: get runtime PM when pinning sprite objects
From: Paulo Zanoni paulo.r.zan...@intel.com Otherwise we may get WARNs saying we're writing registers while runtime suspended. Testcase: igt/pm_rpm/legacy-planes Testcase: igt/pm_rpm/legacy-planes-dpms Cc: sta...@vger.kernel.org Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/intel_sprite.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index d34a569..8c5a8f7 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -821,6 +821,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, uint32_t src_w, uint32_t src_h) { struct drm_device *dev = plane-dev; + struct drm_i915_private *dev_priv = dev-dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_plane *intel_plane = to_intel_plane(plane); enum pipe pipe = intel_crtc-pipe; @@ -1009,7 +1010,9 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, * primary plane requires 256KiB alignment with 64 PTE padding, * the sprite planes only require 128KiB alignment and 32 PTE padding. */ + intel_runtime_pm_get(dev_priv); ret = intel_pin_and_fence_fb_obj(dev, obj, NULL); + intel_runtime_pm_put(dev_priv); i915_gem_track_fb(old_obj, obj, INTEL_FRONTBUFFER_SPRITE(pipe)); -- 2.0.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Add rotation_property to mode_config and creating it
On Mon, Jul 28, 2014 at 06:29:41PM +0300, Ville Syrjälä wrote: On Tue, Jul 15, 2014 at 05:43:37PM +0530, sonika.jin...@intel.com wrote: From: Sonika Jindal sonika.jin...@intel.com v2: Adding creation of rotation_property here. Signed-off-by: Sonika Jindal sonika.jin...@intel.com --- drivers/gpu/drm/drm_crtc.c |3 ++- include/drm/drm_crtc.h |1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 787631e..49c0747 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1299,7 +1299,8 @@ static int drm_mode_create_standard_plane_properties(struct drm_device *dev) type, drm_plane_type_enum_list, ARRAY_SIZE(drm_plane_type_enum_list)); dev-mode_config.plane_type_property = type; - + dev-mode_config.rotation_property = drm_mode_create_rotation_property(dev, + BIT(DRM_ROTATE_0) | BIT(DRM_ROTATE_180)); This might not make sense for other (!i915) hardware. And that's the reason why I had the driver create the property in the first place. I think Daniel was thinking that we might want to expose all the bits regardless of what the hardware supports, but I don't like that idea. There are other properties (eg. alpha blending, csc stuff, etc.) that have the same problem of hardware supporting only a (potentially small) subset of the possible values. I'd rather we didn't make life harder for userspace when the kernel can already report that certain values will never work. Well I'd like the property to be in some generic place so that fbcon can unroate and that with the atomic stuff we can have rotation support in the core structures. Which should help with argument checking. But for rotation I don't think we should set it up in generic code, but in i915. So the place where we keep it would be generic, the values would be the sames, but the allowed set would differ depending upon platform or driver. -Daniel return 0; } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index ce6df4a..5545dd3 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -819,6 +819,7 @@ struct drm_mode_config { struct drm_property *dpms_property; struct drm_property *path_property; struct drm_property *plane_type_property; + struct drm_property *rotation_property; /* DVI-I properties */ struct drm_property *dvi_i_subconnector_property; -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/1] drm/i915: Adding Gfx Clock, Wake and Gunit save/restore logic in PM suspend/resume paths.
On Mon, Jul 28, 2014 at 11:07:10PM +0530, sagar.a.kam...@intel.com wrote: From: Sagar Kamble sagar.a.kam...@intel.com Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of gunit registers need to be followed in PM suspend and resume path similar to runtime suspend and resume. Cc: Imre Deak imre.d...@intel.com Cc: Paulo Zanoni paulo.r.zan...@intel.com Cc: Daniel Vetter daniel.vet...@ffwll.ch Cc: Jani Nikula jani.nik...@linux.intel.com Signed-off-by: Sagar Kamble sagar.a.kam...@intel.com Whereever that code currently lives, I want it shared instead of runtime pm and system s/r diverging even more. Runtime pm is fully platform agnostic, so this should be possible without add a single IS_VLV check anywhere in the code. -Daniel --- drivers/gpu/drm/i915/i915_drv.c | 84 drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_drv.h | 7 3 files changed, 85 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 6c4b25c..fdfeccf 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -495,6 +495,26 @@ static int i915_drm_freeze(struct drm_device *dev) struct drm_i915_private *dev_priv = dev-dev_private; struct drm_crtc *crtc; pci_power_t opregion_target_state; + u32 mask; + int err = 0; + + /* Following sequence from vlv_runtime_suspend */ + if (IS_VALLEYVIEW(dev)) { + /* + * Bspec defines the following GT well on flags as debug only, + * so don't treat them as hard failures. + */ + (void)vlv_wait_for_gt_wells(dev_priv, false); + + mask = VLV_GTLC_RENDER_CTX_EXISTS | VLV_GTLC_MEDIA_CTX_EXISTS; + WARN_ON((I915_READ(VLV_GTLC_WAKE_CTRL) mask) != mask); + + vlv_check_no_gt_access(dev_priv); + + err = vlv_force_gfx_clock(dev_priv, true); + if (err) + goto err1; + } /* ignore lid events during suspend */ mutex_lock(dev_priv-modeset_restore_lock); @@ -542,6 +562,10 @@ static int i915_drm_freeze(struct drm_device *dev) i915_gem_suspend_gtt_mappings(dev); + /* Save Gunit State */ + if (IS_VALLEYVIEW(dev)) + vlv_save_gunit_s0ix_state(dev_priv); + i915_save_state(dev); opregion_target_state = PCI_D3cold; @@ -562,7 +586,27 @@ static int i915_drm_freeze(struct drm_device *dev) intel_display_set_init_power(dev_priv, false); - return 0; + /* Clear Allow Wake Bit so that none of the + * force/demand wake requests + */ + if (IS_VALLEYVIEW(dev)) { + err = vlv_allow_gt_wake(dev_priv, false); + if (err) + goto err2; + + /* Release graphics clocks */ + vlv_force_gfx_clock(dev_priv, false); + } + return err; +err2: + /* For safety always re-enable waking and disable gfx clock forcing */ + if (IS_VALLEYVIEW(dev)) + vlv_allow_gt_wake(dev_priv, true); +err1: + if (IS_VALLEYVIEW(dev)) + vlv_force_gfx_clock(dev_priv, false); + + return err; } int i915_suspend(struct drm_device *dev, pm_message_t state) @@ -610,6 +654,26 @@ void intel_console_resume(struct work_struct *work) static int i915_drm_thaw_early(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; + int ret = 0; + int err; + + /* + * Following sequence from vlv_runtime_resume. Clock is released + * in i915_drm_thaw. + * If any of the steps fail just try to continue, that's the best we + * can do at this point. Return the first error code (which will also + * leave RPM permanently disabled). + */ + + if (IS_VALLEYVIEW(dev)) { + ret = vlv_force_gfx_clock(dev_priv, true); + + vlv_restore_gunit_s0ix_state(dev_priv); + + err = vlv_allow_gt_wake(dev_priv, true); + if (!ret) + ret = err; + } if (IS_HASWELL(dev) || IS_BROADWELL(dev)) hsw_disable_pc8(dev_priv); @@ -618,7 +682,7 @@ static int i915_drm_thaw_early(struct drm_device *dev) intel_uncore_sanitize(dev); intel_power_domains_init_hw(dev_priv); - return 0; + return ret; } static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) @@ -695,6 +759,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) intel_opregion_notify_adapter(dev, PCI_D0); + /* Release graphics clocks turned on in thaw_early*/ + vlv_force_gfx_clock(dev_priv, false); + return 0; } @@ -1028,7 +1095,7 @@ static int hsw_runtime_resume(struct drm_i915_private *dev_priv) * a black-box for the driver.
[Intel-gfx] [PATCH] drm/i915: Fix read back of plane stride register
From: Rafael Barbalho rafael.barba...@intel.com According to the specifications bit 6 is actually valid in the stride register. Cc: Jesse Barnes jbar...@virtuousgeek.org Cc: Ville Syrjälä ville.syrj...@linux.intel.com Signed-off-by: Rafael Barbalho rafael.barba...@intel.com --- drivers/gpu/drm/i915/intel_display.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 99eb7ca..52dab31 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6221,7 +6221,7 @@ static void i9xx_get_plane_config(struct intel_crtc *crtc, crtc-base.primary-fb-height = ((val 0) 0xfff) + 1; val = I915_READ(DSPSTRIDE(pipe)); - crtc-base.primary-fb-pitches[0] = val 0xff80; + crtc-base.primary-fb-pitches[0] = val 0xffc0; aligned_height = intel_align_height(dev, crtc-base.primary-fb-height, plane_config-tiled); @@ -7241,7 +7241,7 @@ static void ironlake_get_plane_config(struct intel_crtc *crtc, crtc-base.primary-fb-height = ((val 0) 0xfff) + 1; val = I915_READ(DSPSTRIDE(pipe)); - crtc-base.primary-fb-pitches[0] = val 0xff80; + crtc-base.primary-fb-pitches[0] = val 0xffc0; aligned_height = intel_align_height(dev, crtc-base.primary-fb-height, plane_config-tiled); -- 2.0.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] drm/i915/bdw: Initialize BDW workarounds in render ring init fn
On Mon, Jul 28, 2014 at 08:00:39PM +0300, Ville Syrjälä wrote: On Mon, Jul 28, 2014 at 05:31:46PM +0100, arun.siluv...@linux.intel.com wrote: From: Arun Siluvery arun.siluv...@linux.intel.com The workarounds at the moment are initialized in init_clock_gating() but they are lost during reset; In case of execlists some workarounds modify registers that are part of register state context, since these are not initialized until init_clock_gating() default context ends up with incorrect values as render context is restored and saved before updated by workarounds hence move them to render ring init fn. This should be ok as these workarounds are not related to display clock gating. Signed-off-by: Arun Siluvery arun.siluv...@linux.intel.com --- drivers/gpu/drm/i915/i915_debugfs.c | 46 ++ drivers/gpu/drm/i915/intel_pm.c | 59 drivers/gpu/drm/i915/intel_ringbuffer.c | 68 + 3 files changed, 114 insertions(+), 59 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 083683c..cf7da30 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2397,20 +2397,65 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused) seq_printf(m, dpll_md: 0x%08x\n, pll-hw_state.dpll_md); seq_printf(m, fp0: 0x%08x\n, pll-hw_state.fp0); seq_printf(m, fp1: 0x%08x\n, pll-hw_state.fp1); seq_printf(m, wrpll: 0x%08x\n, pll-hw_state.wrpll); } drm_modeset_unlock_all(dev); return 0; } +static int i915_workaround_info(struct seq_file *m, void *unused) +{ + struct drm_info_node *node = (struct drm_info_node *) m-private; + struct drm_device *dev = node-minor-dev; + struct drm_i915_private *dev_priv = dev-dev_private; + int ret; + + ret = mutex_lock_interruptible(dev-struct_mutex); + if (ret) + return ret; + + if (IS_BROADWELL(dev)) { + seq_printf(m, GEN8_ROW_CHICKEN:\t0x%08x\n, + I915_READ(GEN8_ROW_CHICKEN)); + seq_printf(m, HALF_SLICE_CHICKEN3:\t0x%08x\n, + I915_READ(HALF_SLICE_CHICKEN3)); + seq_printf(m, GAMTARBMODE:\t0x%08x\n, I915_READ(GAMTARBMODE)); + seq_printf(m, _3D_CHICKEN3:\t0x%08x\n, + I915_READ(_3D_CHICKEN3)); + seq_printf(m, COMMON_SLICE_CHICKEN2:\t0x%08x\n, + I915_READ(COMMON_SLICE_CHICKEN2)); + seq_printf(m, GEN7_HALF_SLICE_CHICKEN1:\t0x%08x\n, + I915_READ(GEN7_HALF_SLICE_CHICKEN1)); + seq_printf(m, GEN7_ROW_CHICKEN2:\t0x%08x\n, + I915_READ(GEN7_ROW_CHICKEN2)); + seq_printf(m, GAM_ECOCHK:\t0x%08x\n, + I915_READ(GAM_ECOCHK)); + seq_printf(m, HDC_CHICKEN0:\t0x%08x\n, + I915_READ(HDC_CHICKEN0)); + seq_printf(m, GEN7_FF_THREAD_MODE:\t0x%08x\n, + I915_READ(GEN7_FF_THREAD_MODE)); + seq_printf(m, GEN8_UCGCTL6:\t0x%08x\n, + I915_READ(GEN8_UCGCTL6)); + seq_printf(m, GEN6_RC_SLEEP_PSMI_CONTROL:\t0x%08x\n, + I915_READ(GEN6_RC_SLEEP_PSMI_CONTROL)); + seq_printf(m, CACHE_MODE_1:\t0x%08x\n, + I915_READ(CACHE_MODE_1)); + } else + DRM_DEBUG_DRIVER(Not available for Gen%d\n, +INTEL_INFO(dev)-gen); + + mutex_unlock(dev-struct_mutex); + return 0; +} + This smells like a separate patch. But I'm not sure we want at all since intel_reg_read will provide the same information. Yeah, debugfs files that just do what intel_reg_read does are just an additional maintaince burden. I know that we have a few that dump lots of registers, but most of them dump a lot of other information, too. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] drm/i915/bdw: Initialize BDW workarounds in render ring init fn
On 28/07/2014 20:22, Daniel Vetter wrote: On Mon, Jul 28, 2014 at 08:00:39PM +0300, Ville Syrjälä wrote: On Mon, Jul 28, 2014 at 05:31:46PM +0100, arun.siluv...@linux.intel.com wrote: From: Arun Siluvery arun.siluv...@linux.intel.com The workarounds at the moment are initialized in init_clock_gating() but they are lost during reset; In case of execlists some workarounds modify registers that are part of register state context, since these are not initialized until init_clock_gating() default context ends up with incorrect values as render context is restored and saved before updated by workarounds hence move them to render ring init fn. This should be ok as these workarounds are not related to display clock gating. Signed-off-by: Arun Siluvery arun.siluv...@linux.intel.com --- drivers/gpu/drm/i915/i915_debugfs.c | 46 ++ drivers/gpu/drm/i915/intel_pm.c | 59 drivers/gpu/drm/i915/intel_ringbuffer.c | 68 + 3 files changed, 114 insertions(+), 59 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 083683c..cf7da30 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2397,20 +2397,65 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused) seq_printf(m, dpll_md: 0x%08x\n, pll-hw_state.dpll_md); seq_printf(m, fp0: 0x%08x\n, pll-hw_state.fp0); seq_printf(m, fp1: 0x%08x\n, pll-hw_state.fp1); seq_printf(m, wrpll: 0x%08x\n, pll-hw_state.wrpll); } drm_modeset_unlock_all(dev); return 0; } +static int i915_workaround_info(struct seq_file *m, void *unused) +{ + struct drm_info_node *node = (struct drm_info_node *) m-private; + struct drm_device *dev = node-minor-dev; + struct drm_i915_private *dev_priv = dev-dev_private; + int ret; + + ret = mutex_lock_interruptible(dev-struct_mutex); + if (ret) + return ret; + + if (IS_BROADWELL(dev)) { + seq_printf(m, GEN8_ROW_CHICKEN:\t0x%08x\n, + I915_READ(GEN8_ROW_CHICKEN)); + seq_printf(m, HALF_SLICE_CHICKEN3:\t0x%08x\n, + I915_READ(HALF_SLICE_CHICKEN3)); + seq_printf(m, GAMTARBMODE:\t0x%08x\n, I915_READ(GAMTARBMODE)); + seq_printf(m, _3D_CHICKEN3:\t0x%08x\n, + I915_READ(_3D_CHICKEN3)); + seq_printf(m, COMMON_SLICE_CHICKEN2:\t0x%08x\n, + I915_READ(COMMON_SLICE_CHICKEN2)); + seq_printf(m, GEN7_HALF_SLICE_CHICKEN1:\t0x%08x\n, + I915_READ(GEN7_HALF_SLICE_CHICKEN1)); + seq_printf(m, GEN7_ROW_CHICKEN2:\t0x%08x\n, + I915_READ(GEN7_ROW_CHICKEN2)); + seq_printf(m, GAM_ECOCHK:\t0x%08x\n, + I915_READ(GAM_ECOCHK)); + seq_printf(m, HDC_CHICKEN0:\t0x%08x\n, + I915_READ(HDC_CHICKEN0)); + seq_printf(m, GEN7_FF_THREAD_MODE:\t0x%08x\n, + I915_READ(GEN7_FF_THREAD_MODE)); + seq_printf(m, GEN8_UCGCTL6:\t0x%08x\n, + I915_READ(GEN8_UCGCTL6)); + seq_printf(m, GEN6_RC_SLEEP_PSMI_CONTROL:\t0x%08x\n, + I915_READ(GEN6_RC_SLEEP_PSMI_CONTROL)); + seq_printf(m, CACHE_MODE_1:\t0x%08x\n, + I915_READ(CACHE_MODE_1)); + } else + DRM_DEBUG_DRIVER(Not available for Gen%d\n, +INTEL_INFO(dev)-gen); + + mutex_unlock(dev-struct_mutex); + return 0; +} + This smells like a separate patch. But I'm not sure we want at all since intel_reg_read will provide the same information. Yeah, debugfs files that just do what intel_reg_read does are just an additional maintaince burden. I know that we have a few that dump lots of registers, but most of them dump a lot of other information, too. -Daniel I've added this mainly for testing workarounds which can be extended further as we move WAs for other chipsets but I agree it can be done with intel_reg_read. regards Arun ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: s/seqno/request/ tracking inside objects
On Fri, 25 Jul 2014 13:27:00 +0100 Chris Wilson ch...@chris-wilson.co.uk wrote: @@ -614,12 +615,12 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data) seq_printf(m, Flip pending (waiting for vsync) on pipe %c (plane %c)\n, pipe, plane); } - if (work-ring) + if (work-flip_queued_request) { + struct i915_gem_request *rq = work-flip_queued_request; seq_printf(m, Flip queued on %s at seqno %u, now %u\n, - work-ring-name, - work-flip_queued_seqno, - work-ring-get_seqno(work-ring, true)); - else + rq-ring-name, rq-seqno, + rq-ring-get_seqno(rq-ring, true)); + } else seq_printf(m, Flip not associated I wonder if the overlay, flip queue and unlocked wait changes could be split out somehow; I think they're the trickiest part of the change... It does look like you're doing get/puts in the right places, though I didn't check the error paths (and I'm not familiar at all with the overlay bits, I guess Daniel will have to look at that). with any ring\n); seq_printf(m, Flip queued on frame %d, (was ready on frame %d), now %d\n, work-flip_queued_vblank, @@ -656,7 +657,7 @@ static int i915_gem_request_info(struct seq_file *m, void *data) struct drm_device *dev = node-minor-dev; struct drm_i915_private *dev_priv = dev-dev_private; struct intel_engine_cs *ring; - struct drm_i915_gem_request *gem_request; + struct i915_gem_request *rq; int ret, count, i; ret = mutex_lock_interruptible(dev-struct_mutex); @@ -669,12 +670,10 @@ static int i915_gem_request_info(struct seq_file *m, void *data) continue; seq_printf(m, %s requests:\n, ring-name); - list_for_each_entry(gem_request, - ring-request_list, - list) { + list_for_each_entry(rq, ring-request_list, list) { seq_printf(m, %d @ %d\n, -gem_request-seqno, -(int) (jiffies - gem_request-emitted_jiffies)); +rq-seqno, +(int)(jiffies - rq-emitted_jiffies)); } count++; } Semi gratuitous rename (though I know you renamed the struct to catch all the users). diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 9837b0f..5794d096 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -187,6 +187,7 @@ enum hpd_pin { struct drm_i915_private; struct i915_mm_struct; struct i915_mmu_object; +struct i915_gem_request; enum intel_dpll_id { DPLL_ID_PRIVATE = -1, /* non-shared dpll in use */ @@ -1720,16 +1721,15 @@ struct drm_i915_gem_object { struct drm_mm_node *stolen; struct list_head global_list; - struct list_head ring_list; /** Used in execbuf to temporarily hold a ref */ struct list_head obj_exec_link; /** * This is set if the object is on the active lists (has pending - * rendering and so a non-zero seqno), and is not set if it i s on - * inactive (ready to be unbound) list. + * rendering and so a submitted request), and is not set if it is on + * inactive (ready to be unbound) list. We track activity per engine. */ - unsigned int active:1; + unsigned int active:3; I wonder if it would be better to be explicit and have a num_rings sized array here then. We need 4 bits anyway for the second video ring right? We'd need a wrapper to check for active then though... @@ -1850,7 +1850,9 @@ void i915_gem_track_fb(struct drm_i915_gem_object *old, * sequence-number comparisons on buffer last_rendering_seqnos, and associate * an emission time with seqnos for tracking how far ahead of the GPU we are. */ -struct drm_i915_gem_request { +struct i915_gem_request { + struct kref kref; + /** On Which ring this request was generated */ struct intel_engine_cs *ring; @@ -1878,8 +1880,60 @@ struct drm_i915_gem_request { struct drm_i915_file_private *file_priv; /** file_priv list entry for this request */ struct list_head client_list; + + bool completed:1; }; If Daniel pulls in Greg's tree, the kref could turn into a fence and the completed could be removed. +static inline struct intel_engine_cs *i915_request_ring(struct i915_gem_request *rq) +{ + return rq ? rq-ring : NULL; +} + +static inline int i915_request_ring_id(struct i915_gem_request *rq) +{ + return rq ? rq-ring-id : -1; +} + +static inline u32 i915_request_seqno(struct i915_gem_request *rq) +{ + return rq ? rq-seqno : 0; +} + +/** + * Returns true if seq1
Re: [Intel-gfx] [RFC] drm/i915/bdw: Initialize BDW workarounds in render ring init fn
On 28/07/2014 18:00, Ville Syrjälä wrote: On Mon, Jul 28, 2014 at 05:31:46PM +0100, arun.siluv...@linux.intel.com wrote: From: Arun Siluvery arun.siluv...@linux.intel.com The workarounds at the moment are initialized in init_clock_gating() but they are lost during reset; In case of execlists some workarounds modify registers that are part of register state context, since these are not initialized until init_clock_gating() default context ends up with incorrect values as render context is restored and saved before updated by workarounds hence move them to render ring init fn. This should be ok as these workarounds are not related to display clock gating. Signed-off-by: Arun Siluvery arun.siluv...@linux.intel.com --- drivers/gpu/drm/i915/i915_debugfs.c | 46 ++ drivers/gpu/drm/i915/intel_pm.c | 59 drivers/gpu/drm/i915/intel_ringbuffer.c | 68 + 3 files changed, 114 insertions(+), 59 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 083683c..cf7da30 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2397,20 +2397,65 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused) seq_printf(m, dpll_md: 0x%08x\n, pll-hw_state.dpll_md); seq_printf(m, fp0: 0x%08x\n, pll-hw_state.fp0); seq_printf(m, fp1: 0x%08x\n, pll-hw_state.fp1); seq_printf(m, wrpll: 0x%08x\n, pll-hw_state.wrpll); } drm_modeset_unlock_all(dev); return 0; } +static int i915_workaround_info(struct seq_file *m, void *unused) +{ + struct drm_info_node *node = (struct drm_info_node *) m-private; + struct drm_device *dev = node-minor-dev; + struct drm_i915_private *dev_priv = dev-dev_private; + int ret; + + ret = mutex_lock_interruptible(dev-struct_mutex); + if (ret) + return ret; + + if (IS_BROADWELL(dev)) { + seq_printf(m, GEN8_ROW_CHICKEN:\t0x%08x\n, + I915_READ(GEN8_ROW_CHICKEN)); + seq_printf(m, HALF_SLICE_CHICKEN3:\t0x%08x\n, + I915_READ(HALF_SLICE_CHICKEN3)); + seq_printf(m, GAMTARBMODE:\t0x%08x\n, I915_READ(GAMTARBMODE)); + seq_printf(m, _3D_CHICKEN3:\t0x%08x\n, + I915_READ(_3D_CHICKEN3)); + seq_printf(m, COMMON_SLICE_CHICKEN2:\t0x%08x\n, + I915_READ(COMMON_SLICE_CHICKEN2)); + seq_printf(m, GEN7_HALF_SLICE_CHICKEN1:\t0x%08x\n, + I915_READ(GEN7_HALF_SLICE_CHICKEN1)); + seq_printf(m, GEN7_ROW_CHICKEN2:\t0x%08x\n, + I915_READ(GEN7_ROW_CHICKEN2)); + seq_printf(m, GAM_ECOCHK:\t0x%08x\n, + I915_READ(GAM_ECOCHK)); + seq_printf(m, HDC_CHICKEN0:\t0x%08x\n, + I915_READ(HDC_CHICKEN0)); + seq_printf(m, GEN7_FF_THREAD_MODE:\t0x%08x\n, + I915_READ(GEN7_FF_THREAD_MODE)); + seq_printf(m, GEN8_UCGCTL6:\t0x%08x\n, + I915_READ(GEN8_UCGCTL6)); + seq_printf(m, GEN6_RC_SLEEP_PSMI_CONTROL:\t0x%08x\n, + I915_READ(GEN6_RC_SLEEP_PSMI_CONTROL)); + seq_printf(m, CACHE_MODE_1:\t0x%08x\n, + I915_READ(CACHE_MODE_1)); + } else + DRM_DEBUG_DRIVER(Not available for Gen%d\n, +INTEL_INFO(dev)-gen); + + mutex_unlock(dev-struct_mutex); + return 0; +} + This smells like a separate patch. But I'm not sure we want at all since intel_reg_read will provide the same information. struct pipe_crc_info { const char *name; struct drm_device *dev; enum pipe pipe; }; static int i915_pipe_crc_open(struct inode *inode, struct file *filep) { struct pipe_crc_info *info = inode-i_private; struct drm_i915_private *dev_priv = info-dev-dev_private; @@ -3904,20 +3949,21 @@ static const struct drm_info_list i915_debugfs_list[] = { {i915_ppgtt_info, i915_ppgtt_info, 0}, {i915_llc, i915_llc, 0}, {i915_edp_psr_status, i915_edp_psr_status, 0}, {i915_sink_crc_eDP1, i915_sink_crc, 0}, {i915_energy_uJ, i915_energy_uJ, 0}, {i915_pc8_status, i915_pc8_status, 0}, {i915_power_domain_info, i915_power_domain_info, 0}, {i915_display_info, i915_display_info, 0}, {i915_semaphore_status, i915_semaphore_status, 0}, {i915_shared_dplls_info, i915_shared_dplls_info, 0}, + {i915_workaround_info, i915_workaround_info, 0}, }; #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list) static const struct i915_debugfs_files { const char *name; const struct
Re: [Intel-gfx] [PATCH] mutex: Export an interface to wrap a mutex lock
On Sat, Jul 26, 2014 at 09:01:55AM +0100, Chris Wilson wrote: In i915, we have a big mutex around our device struct - every time before we attempt to communicate with the GPU, we acquire the mutex. This makes it a convenient juncture to place our GPU error handling - before we take the mutex we first check whether the GPU is hung or whether we are in the process of recovering from a GPU hang. So we wrap the call to mutex_lock() alongside our additional error handling routines. The downside of using a wrapper around mutex_lock() is that lockdep and lockstat cannot discriminate the true callers of mutex_lock(). Unless we provide a means for the wrapper to pass that information down. It also appears that i915 is unique in this manner of wrapping mutex_lock(). Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ben Widawsky benjamin.widaw...@intel.com With the one fix inline, take your pick: Reported-and-tested-by: Ben Widawsky b...@bwidawsk.net Reviewed-by: Ben Widawsky b...@bwidawsk.net I find it very strange that i915 is unique here. It makes me feel like we're doin' in wrong. BTW, a cleaned up version of the macro version I sent you might meet with less resistance given that no other driver needs it [currently]. Also, looking at this code now, we could probably replace a bunch of mutex_lock()'s in i915 with mutex_lock_wrapper(..., TASK_KILLABLE, ...) --- drivers/gpu/drm/i915/i915_gem.c | 4 +++- include/linux/mutex.h | 9 + kernel/locking/mutex.c | 36 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index ae3c7b6162f5..888acd01db44 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -233,7 +233,9 @@ int i915_mutex_lock_interruptible(struct drm_device *dev) if (ret) return ret; - ret = mutex_lock_interruptible(dev-struct_mutex); + ret = mutex_lock_wrapper(dev-struct_mutex, + TASK_INTERRUPTIBLE, + _RET_IP_); if (ret) return ret; diff --git a/include/linux/mutex.h b/include/linux/mutex.h index 42aa9b9ecd5f..b88255a390a2 100644 --- a/include/linux/mutex.h +++ b/include/linux/mutex.h @@ -143,10 +143,15 @@ extern int __must_check mutex_lock_interruptible_nested(struct mutex *lock, unsigned int subclass); extern int __must_check mutex_lock_killable_nested(struct mutex *lock, unsigned int subclass); +extern int __must_check mutex_lock_wrapper_nested(struct mutex *lock, + unsigned int subclass, + long state, + unsigned long ip); #define mutex_lock(lock) mutex_lock_nested(lock, 0) #define mutex_lock_interruptible(lock) mutex_lock_interruptible_nested(lock, 0) #define mutex_lock_killable(lock) mutex_lock_killable_nested(lock, 0) +#define mutex_lock_wrapper(lock, state, ip) mutex_lock_killable_nested(lock, 0, state, ip) s/mutex_lock_killable_nested/mutex_lock_wrapper_nested/ #define mutex_lock_nest_lock(lock, nest_lock) \ do { \ @@ -158,10 +163,14 @@ do { \ extern void mutex_lock(struct mutex *lock); extern int __must_check mutex_lock_interruptible(struct mutex *lock); extern int __must_check mutex_lock_killable(struct mutex *lock); +extern int __must_check mutex_lock_wrapper(struct mutex *lock, +long state, +unsigned long ip); # define mutex_lock_nested(lock, subclass) mutex_lock(lock) # define mutex_lock_interruptible_nested(lock, subclass) mutex_lock_interruptible(lock) # define mutex_lock_killable_nested(lock, subclass) mutex_lock_killable(lock) +# define mutex_lock_wrapper_nested(lock, subclass, state, ip) mutex_lock_wrapper(lock, state, ip) # define mutex_lock_nest_lock(lock, nest_lock) mutex_lock(lock) #endif diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c index acca2c1a3c5e..243ee5fc5dc3 100644 --- a/kernel/locking/mutex.c +++ b/kernel/locking/mutex.c @@ -619,6 +619,17 @@ mutex_lock_interruptible_nested(struct mutex *lock, unsigned int subclass) EXPORT_SYMBOL_GPL(mutex_lock_interruptible_nested); +int __sched +mutex_lock_wrapper_nested(struct mutex *lock, unsigned int subclass, + long state, unsigned long ip) +{ + might_sleep(); + return __mutex_lock_common(lock, state, +subclass, NULL, ip, NULL, 0); +} + +EXPORT_SYMBOL_GPL(mutex_lock_wrapper_nested); +
[Intel-gfx] [PATCH 1/2] drm/i915: Collect gtier properly on HSW.
GTIER and DEIER doesn't have same interface on HSW so this or operation makes the information provided useless. Cc: Paulo Zanoni paulo.r.zan...@intel.com Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gpu_error.c | 16 ++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index ef38c3b..ccb97f1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -314,6 +314,7 @@ struct drm_i915_error_state { u32 eir; u32 pgtbl_er; u32 ier; + u32 gtier; u32 ccid; u32 derrmr; u32 forcewake; diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index 0b3f694..372fea3 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -359,6 +359,8 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m, err_printf(m, PCI ID: 0x%04x\n, dev-pdev-device); err_printf(m, EIR: 0x%08x\n, error-eir); err_printf(m, IER: 0x%08x\n, error-ier); + if (IS_HASWELL(dev)) + err_printf(m, GTIER: 0x%08x\n, error-gtier); err_printf(m, PGTBL_ER: 0x%08x\n, error-pgtbl_er); err_printf(m, FORCEWAKE: 0x%08x\n, error-forcewake); err_printf(m, DERRMR: 0x%08x\n, error-derrmr); @@ -1135,13 +1137,15 @@ static void i915_capture_reg_state(struct drm_i915_private *dev_priv, if (HAS_HW_CONTEXTS(dev)) error-ccid = I915_READ(CCID); - if (HAS_PCH_SPLIT(dev)) + if (IS_HASWELL(dev)) { + error-ier = I915_READ(DEIER); + error-gtier = I915_READ(GTIER); + } else if (HAS_PCH_SPLIT(dev)) { error-ier = I915_READ(DEIER) | I915_READ(GTIER); - else { - if (IS_GEN2(dev)) - error-ier = I915_READ16(IER); - else - error-ier = I915_READ(IER); + } else if (IS_GEN2(dev)) { + error-ier = I915_READ16(IER); + } else { + error-ier = I915_READ(IER); } /* 4: Everything else */ -- 1.9.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/3] Fixes for runtime PM on planes APIs
On Mon, Jul 28, 2014 at 03:37:11PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com Hi This series fixes some bugs that happen when we're runtime suspended and try to use the planes APIs. I also wrote IGT test cases for the bugs, so we will be able to detect future regressions. The controversial part of these patches is that we had previously defined that we wanted to get/put runtime PM in the highest level of the stack, wrapping as much code as possible, but Daniel asked me to only get/put runtime PM around the functions that pin the objects (still on the highest level, but only around the pin functions). This series implements Daniel's suggestions. These look pretty straightforward to me, so for all three: Reviewed-by: Matt Roper matthew.d.ro...@intel.com However as a side note on the runtime PM stuff I'll admit that it's not an area I'd previously paid too much attention to and my first reaction looking at the patches was I wonder if intel_runtime_pm_{get,put} could be pushed down into intel_pin_and_fence_fb_obj(). Until I read your cover letter I wasn't aware of the goal of get/put runtime PM in the highest level of the stack, wrapping as much code as possible and I don't think that's really explained anywhere in the code or in the commit log today. Maybe we could add a comment in intel_pm.c explaining that design goal for future contributors/reviewers? Matt Thanks, Paulo Paulo Zanoni (3): drm/i915: fix cursor handling when runtime suspended drm/i915: get runtime PM when pinning sprite objects drm/i915: get runtime PM when pinning primary plane objects drivers/gpu/drm/i915/intel_display.c | 9 + drivers/gpu/drm/i915/intel_sprite.c | 3 +++ 2 files changed, 12 insertions(+) -- 2.0.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Matt Roper Graphics Software Engineer IoTG Platform Enabling Development Intel Corporation (916) 356-2795 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] tests/pm_rpm: add planes subtests
On Mon, Jul 28, 2014 at 03:37:15PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com Just like the cursor subtests, these also trigger WARNs on the current Kernel. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com I feel like a lot of the setup you have to do here is duplicating logic we have in the igt_kms library. Was there functionality lacking from that library that prevented you from using it to write this test? If so, I can look into adding it. Anyway, the test still looks good, just one or two minor comments inline below. --- tests/pm_rpm.c | 212 - 1 file changed, 211 insertions(+), 1 deletion(-) diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c index f720f86..048d9ad 100644 --- a/tests/pm_rpm.c +++ b/tests/pm_rpm.c @@ -51,6 +51,9 @@ #include igt_kms.h #include igt_debugfs.h +/* One day, this will be on your libdrm. */ +#define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2 I think there was just an official release of libdrm today that finally includes this and the plane type enum below...maybe we can just bump the libdrm requirement for igt and stop including these by hand? + #define MSR_PC8_RES 0x630 #define MSR_PC9_RES 0x631 #define MSR_PC10_RES 0x632 @@ -72,6 +75,12 @@ enum screen_type { SCREEN_TYPE_ANY, }; +enum plane_type { + PLANE_OVERLAY, + PLANE_PRIMARY, + PLANE_CURSOR, +}; + /* Wait flags */ #define DONT_WAIT0 #define WAIT_STATUS 1 @@ -1504,6 +1513,199 @@ static void cursor_subtest(bool dpms) igt_assert(wait_for_suspended()); } +static enum plane_type get_plane_type(uint32_t plane_id) +{ + drmModeObjectPropertiesPtr props; + int i, j; + enum plane_type type; + bool found = false; + + props = drmModeObjectGetProperties(drm_fd, plane_id, +DRM_MODE_OBJECT_PLANE); + igt_assert(props); + + for (i = 0; i props-count_props !found; i++) { + drmModePropertyPtr prop; + const char *enum_name = NULL; + + prop = drmModeGetProperty(drm_fd, props-props[i]); + igt_assert(prop); + + if (strcmp(prop-name, type) == 0) { + igt_assert(prop-flags DRM_MODE_PROP_ENUM); + igt_assert(props-prop_values[i] prop-count_enums); + + for (j = 0; j prop-count_enums; j++) { + if (prop-enums[j].value == + props-prop_values[i]) { + enum_name = prop-enums[j].name; + break; + } + } + igt_assert(enum_name); + + if (strcmp(enum_name, Overlay) == 0) + type = PLANE_OVERLAY; + else if (strcmp(enum_name, Primary) == 0) + type = PLANE_PRIMARY; + else if (strcmp(enum_name, Cursor) == 0) + type = PLANE_CURSOR; + else + igt_assert(0); + + found = true; + } + + drmModeFreeProperty(prop); + } + igt_assert(found); + + drmModeFreeObjectProperties(props); + return type; +} + +static void test_one_plane(bool dpms, uint32_t plane_id, +enum plane_type plane_type) +{ + int rc; + uint32_t plane_format, plane_w, plane_h; + uint32_t crtc_id, connector_id; + struct igt_fb scanout_fb, plane_fb1, plane_fb2; + drmModeModeInfoPtr mode; + int32_t crtc_x = 0, crtc_y = 0; + + disable_all_screens(ms_data); + igt_assert(wait_for_suspended()); + + igt_require(find_connector_for_modeset(ms_data, SCREEN_TYPE_ANY, +connector_id, mode)); + + crtc_id = ms_data.res-crtcs[0]; + igt_assert(crtc_id); + + igt_create_fb(drm_fd, mode-hdisplay, mode-vdisplay, + DRM_FORMAT_XRGB, false, scanout_fb); + + fill_igt_fb(scanout_fb, 0xFF); + + switch (plane_type) { + case PLANE_OVERLAY: + plane_format = DRM_FORMAT_XRGB; + plane_w = 64; + plane_h = 64; + break; + case PLANE_PRIMARY: + plane_format = DRM_FORMAT_XRGB; + plane_w = mode-hdisplay; + plane_h = mode-vdisplay; + break; + case PLANE_CURSOR: + plane_format = DRM_FORMAT_ARGB; + plane_w = 64; + plane_h = 64; + break; + default: + igt_assert(0); + break; + } + + igt_create_fb(drm_fd, plane_w, plane_h, plane_format, false, + plane_fb1); + igt_create_fb(drm_fd, plane_w,
Re: [Intel-gfx] [PATCH] drm/i915: Rework GPU reset sequence to match driver load thaw
On Mon, Jul 28, 2014 at 05:12:59PM +, Mcaulay, Alistair wrote: Hi Ben / Daniel, I agree that this needs to be properly tested. Are there any particular igt tests you would suggest I use? I've been running: drv_hangman, drv_suspend, gem_hangcheck_forcewake. I thought IGT had added some random reset stuff in the way that we have for signals. However, it looks like I imagined it. I guess you can add gem_hang, gem_reset_stats, and tests/debugfs_wedged to that list. Daniel can probably provide any I might have missed. The way that igt quiesces everything these days really hurts the ability to test multi-process. If every tests starts off with no work, and running the default context, things are pretty trivial. Similarly, running these tests in isolation, even if it isn't quiescing doesn't help the situation. The way I wrote the code originally was through debugging hangs on a desktop as I developed patches, and not with IGT (though drv_hangman could catch many issues). I'd definitely recommend trying to invoke hangs on a running desktop. I'd advise doing this by modifying mesa to submit a crap batch, I can provide you more details on how to do this if you need it. Also try to disable the quiescing in IGT and run more than these tests in isolation. Also do you have a set of PPGTT Patches that should work with these tests. Michel sent me a set of patches to enable PPGTT, but these 3 tests fail with the patches. I will try to reproduce this on my patch series when I have some time and if nothing else, that should be a good preparation/refresher for the patch review anyway. The patches I wrote shouldn't have touched much on these paths - not sure if Michel changed anything there. With patch on top of what Michel sent you, everything passes? Thanks, Alistair. -Original Message- From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Monday, July 28, 2014 10:27 AM To: Ben Widawsky Cc: Mcaulay, Alistair; intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] drm/i915: Rework GPU reset sequence to match driver load thaw On Fri, Jul 25, 2014 at 06:05:29PM -0700, Ben Widawsky wrote: On Wed, Jul 16, 2014 at 04:05:59PM +0100, alistair.mcau...@intel.com wrote: From: McAulay, Alistair alistair.mcau...@intel.com This patch is to address Daniels concerns over different code during reset: http://lists.freedesktop.org/archives/intel-gfx/2014-June/047758.htm l The reason for aiming as hard as possible to use the exact same code for driver load, gpu reset and runtime pm/system resume is that we've simply seen too many bugs due to slight variations and unintended omissions. Tested using igt drv_hangman. Signed-off-by: McAulay, Alistair alistair.mcau...@intel.com 2 quick comments before I actually do a real review. 1. Did you actually run this with and without full ppgtt? 2. I don't think this is actually fulfilling what Daniel is requesting, though we can let him comment. Mostly looks like what I think we need. Comments below. 3. Did you reall do #1? Assuming you satisifed #1, can you please post the igt results for the permutations (pre patch w/ and w/o ppgtt; post patch w/ and w/o ppgtt) I really want this data because I spent *a lot* of time with these specific areas in the PPGTT work, and I am somewhat skeptical enough of the code has changed that this will magically work. I also tried the trickiness with the ring handling functions, and never succeeded. Also, with the context stuff, I'm simply not convinced it can magically vanish. If igt looks good, and Daniel agrees that this is what he actually wanted, I will go fishing for corner cases and do the review. I think the hack in ring_begin might explain why it never worked before. But fully agreed, we really need to test this well (and fill gaps if igt misses anything around resets - we don't have any systematic gpu reset coverage anywhere outside of igt). Thanks. --- drivers/gpu/drm/i915/i915_gem.c | 2 - drivers/gpu/drm/i915/i915_gem_context.c | 42 + drivers/gpu/drm/i915/i915_gem_gtt.c | 67 + drivers/gpu/drm/i915/i915_gem_gtt.h | 3 +- drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +- 5 files changed, 14 insertions(+), 104 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index ef047bc..b38e086 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2590,8 +2590,6 @@ void i915_gem_reset(struct drm_device *dev) for_each_ring(ring, dev_priv, i) i915_gem_reset_ring_cleanup(dev_priv, ring); - i915_gem_context_reset(dev); - i915_gem_restore_fences(dev); } diff --git a/drivers/gpu/drm/i915/i915_gem_context.c