Re: [Intel-gfx] [PATCH] drm/i915: Always use kref tracking for all contexts.
On Wed, Apr 09, 2014 at 09:07:36AM +0100, Chris Wilson wrote: If we always initialize kref for the context, even if we are using fake contexts for hangstats when there is no hw support, we can forgo the dance to dereference the ctx-obj and inspect whether we are permitted to use kref inside i915_gem_context_reference() and _unreference(). My ulterior motive here is to improve the debugging of a use-after-free of ctx-obj. This patch avoids the dereference here and instead forces the assertion checks associated with kref. v2: Refactor the fake contexts to being even more like the real contexts, so that there is much less duplicated and special case code. v3: Tweaks. v4: Tweaks, minor. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76671 Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Tested-by: lu hua huax...@intel.com Cc: Ben Widawsky benjamin.widaw...@intel.com Cc: Mika Kuoppala mika.kuopp...@intel.com I couldn't spot any problems, but I got a bit lazier with each review since v2. Reviewed-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_drv.h| 8 +- drivers/gpu/drm/i915/i915_gem.c| 2 +- drivers/gpu/drm/i915/i915_gem_context.c| 234 - drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- 4 files changed, 101 insertions(+), 145 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 533bd8f6a5b1..0557bd92b26b 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2276,20 +2276,18 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file); int i915_gem_context_enable(struct drm_i915_private *dev_priv); void i915_gem_context_close(struct drm_device *dev, struct drm_file *file); int i915_switch_context(struct intel_ring_buffer *ring, - struct drm_file *file, struct i915_hw_context *to); + struct i915_hw_context *to); struct i915_hw_context * i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id); void i915_gem_context_free(struct kref *ctx_ref); static inline void i915_gem_context_reference(struct i915_hw_context *ctx) { - if (ctx-obj HAS_HW_CONTEXTS(ctx-obj-base.dev)) - kref_get(ctx-ref); + kref_get(ctx-ref); } static inline void i915_gem_context_unreference(struct i915_hw_context *ctx) { - if (ctx-obj HAS_HW_CONTEXTS(ctx-obj-base.dev)) - kref_put(ctx-ref, i915_gem_context_free); + kref_put(ctx-ref, i915_gem_context_free); } static inline bool i915_gem_context_is_default(const struct i915_hw_context *c) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index eb5cf077c626..4368437458fd 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2819,7 +2819,7 @@ int i915_gpu_idle(struct drm_device *dev) /* Flush everything onto the inactive list. */ for_each_ring(ring, dev_priv, i) { - ret = i915_switch_context(ring, NULL, ring-default_context); + ret = i915_switch_context(ring, ring-default_context); if (ret) return ret; diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 30b355afb362..f77b4c126465 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -96,9 +96,6 @@ #define GEN6_CONTEXT_ALIGN (6410) #define GEN7_CONTEXT_ALIGN 4096 -static int do_switch(struct intel_ring_buffer *ring, - struct i915_hw_context *to); - static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt) { struct drm_device *dev = ppgtt-base.dev; @@ -185,13 +182,15 @@ void i915_gem_context_free(struct kref *ctx_ref) typeof(*ctx), ref); struct i915_hw_ppgtt *ppgtt = NULL; - /* We refcount even the aliasing PPGTT to keep the code symmetric */ - if (USES_PPGTT(ctx-obj-base.dev)) - ppgtt = ctx_to_ppgtt(ctx); + if (ctx-obj) { + /* We refcount even the aliasing PPGTT to keep the code symmetric */ + if (USES_PPGTT(ctx-obj-base.dev)) + ppgtt = ctx_to_ppgtt(ctx); - /* XXX: Free up the object before tearing down the address space, in - * case we're bound in the PPGTT */ - drm_gem_object_unreference(ctx-obj-base); + /* XXX: Free up the object before tearing down the address space, in + * case we're bound in the PPGTT */ + drm_gem_object_unreference(ctx-obj-base); + } if (ppgtt) kref_put(ppgtt-ref, ppgtt_release); @@ -232,40 +231,40 @@ __create_hw_context(struct drm_device *dev, return ERR_PTR(-ENOMEM); kref_init(ctx-ref); - ctx-obj =
Re: [Intel-gfx] [PATCH] BDW swizzling
On Thu, Apr 10, 2014 at 03:50:35PM -0700, Ben Widawsky wrote: On Thu, Apr 10, 2014 at 06:51:50PM +0100, Damien Lespiau wrote: [snip] Do you know if you have a configuration where we try to swizzle? If yes and tests/gem_tiled_pread is passing that would give us a nice bit of information. (which of course can be tried by the next person with time to do so). If you get it wrong, it looks really obvious. Swizzling is *supposed* to be one of those transparent things (I thought). What follows can be entirely wrong, it's mostly from memory and a brief conversation with Art. There are 3 places that care about swizzling: 1. The memory/DRAM controller 2. The displayer interface to memory 3. The GAM arbiter (generic interface to memory) It may or may not be talking about the same type of swizzling (bit) in all cases. The important thing, and what I have observed, is that the GAM and DE match on how things are swizzled. Otherwise we render/blit to a surface and it gets [de]swizzled when it's displayed. I never measured performance for setting both to 0, instead of 1. The part that's confused me has always been why we are supposed to program it based on #1. The way the DRAM controller decides to lay out the physical rows/banks etc. shouldn't matter as long as everyone goes through the same DRAM controller. It should just be transparent linear RAM. In other words, the comment about how we need to program the swizzle based on the DRAM controller never quite made sense to me. It's also possible if you enable one, you shouldn't/should enable another since compounding swizzling may be self-defeating. Dunno - so maybe your patch helps, maybe it hurts. Art suggested that the swizzling in GAM and DE predate the DRAM swizzler. My bad. I just read the actual patch's commit message. Seems like you knew all this already. Feel free to ignore me. I'll try to read both before responding, next time. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/4] Reduce intel_display.c
On Fri, 11 Apr 2014, Ben Widawsky b...@bwidawsk.net wrote: On Wed, Apr 09, 2014 at 06:44:29PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com Hi We always talk about how intel_display.c is a giant file and how we would like to reduce it, so this is my attempt. Currently the file has 12090 lines, and after my patch series it has 8850 lines. I don't know if right now is the appropriate time to merge patches like this. I don't remember seeing too many patches on the list touching cursor/fdi/eld/pll functions, but I know there is never an appropriate time for huge changes. Also, this change will obviously make the lives of people who backport our patches more complicated. So if we don't want this series at all, feel free to NACK it. I am only responding because it seems nobody else really said much. I never touch this code, and I shouldn't be the authority. I really quickly glanced at the patches. 1. +LOC: It sucks that you ended up adding 220 lines. I assume half of it is the copyright header, but still, considering there are no actual refactors, cleanups, or functional changes - adding lines makes me unhappy. 2. necessary? I personally haven't heard from anyone that we need to shrink intel_display.c (again, I am the furthest from being an expert). I doubt anyone isn't using some form of tags, or grep to navigate anyway. My problem has never been the file size itself, but just the structure of the display code interacting with the core KMS was hard to follow. 3. conflicts: Like you said, it's likely nobody touches this code, but we should keep in mind we do have several people working on older branches, and this kind of thing makes any sort of backport hard. On the other hand: 1. If more than one person finds the results more readable/consumable, I think it's worth it, and probably mostly justifies doing it. You've also shrunk the file by quite a bit, so it's somewhat useful churn. 2. intel_pll.c sounds like a good idea I'm in favour of reducing the size of intel_display.c. I did not look at the patches though, because I've sent a few patches to this effect in the past (limits/pll and quirks at least), but they were stalled because they were in the collision course with the Grand Plans Daniel has. So Daniel, I expect you to chime in on this one too. ;) To reduce the conflict/backport pains, might be good to do this spread out over a few releases instead of everything at once. *shrug*. BR, Jani. I also didn't really know what kind of changes I needed to do to the file headers, so I just copied the header from intel_display.c, kept Eric's name and added a 2014 to Intel's copyright. I am not a lawyer and this may be not the best thing to do, so please tell me the correct approach here :) There are also some things that we might want to migrate from intel_ddi.c to intel_pll.c, but I'll leave this to another patch. Also, feel free to propose better ways to split intel_display.c. Thanks, Paulo Paulo Zanoni (4): drm/i915: extract intel_eld.c from intel_display.c drm/i915: extract intel_cursor.c from intel_display.c drm/i915: extract intel_fdi.c from intel_display.c drm/i915: extract intel_pll.c from intel_display.c drivers/gpu/drm/i915/Makefile|4 + drivers/gpu/drm/i915/intel_cursor.c | 357 drivers/gpu/drm/i915/intel_ddi.c | 142 +- drivers/gpu/drm/i915/intel_display.c | 3622 ++ drivers/gpu/drm/i915/intel_drv.h | 143 +- drivers/gpu/drm/i915/intel_eld.c | 355 drivers/gpu/drm/i915/intel_fdi.c | 959 + drivers/gpu/drm/i915/intel_panel.c | 36 + drivers/gpu/drm/i915/intel_pll.c | 1779 + 9 files changed, 3808 insertions(+), 3589 deletions(-) create mode 100644 drivers/gpu/drm/i915/intel_cursor.c create mode 100644 drivers/gpu/drm/i915/intel_eld.c create mode 100644 drivers/gpu/drm/i915/intel_fdi.c create mode 100644 drivers/gpu/drm/i915/intel_pll.c -- 1.9.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Invalidate our pages under memory pressure
On Tue, Mar 25, 2014 at 01:23:06PM +, Chris Wilson wrote: Try to flush out dirty pages into the swapcache (and from there into the swapfile) when under memory pressure and forced to drop GEM objects from memory. In effect, this should just allow us to discard unused pages for memory reclaim and to start writeback earlier. v2: Hugh Dickins warned that explicitly starting writeback from shrink_slab was prone to deadlocks within shmemfs. Cc: Hugh Dickins hu...@google.com Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Good news! QA have declared that this series really does prevent the random OOM where we have completely unused swap. So all it needs is someone brave enough to review. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Invalidate our pages under memory pressure
On Fri, Apr 11, 2014 at 09:30:20AM +0100, Chris Wilson wrote: On Tue, Mar 25, 2014 at 01:23:06PM +, Chris Wilson wrote: Try to flush out dirty pages into the swapcache (and from there into the swapfile) when under memory pressure and forced to drop GEM objects from memory. In effect, this should just allow us to discard unused pages for memory reclaim and to start writeback earlier. v2: Hugh Dickins warned that explicitly starting writeback from shrink_slab was prone to deadlocks within shmemfs. Cc: Hugh Dickins hu...@google.com Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Good news! QA have declared that this series really does prevent the random OOM where we have completely unused swap. So all it needs is someone brave enough to review. Awesome work. Do we need the additional patch you've just posted to improve the writeback stalling after calling shrinkers too, or is that not required? wrt reviewers I'm poking people to get this done from our side. For merging an ack on the mm pieces would be good so that I can pull it in through drm-intel trees. My plan is to merge it in 3.16 if it all checks out. Cheers, 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 4/4] drm/i915: Invalidate our pages under memory pressure
On Fri, Apr 11, 2014 at 10:38:25AM +0200, Daniel Vetter wrote: On Fri, Apr 11, 2014 at 09:30:20AM +0100, Chris Wilson wrote: On Tue, Mar 25, 2014 at 01:23:06PM +, Chris Wilson wrote: Try to flush out dirty pages into the swapcache (and from there into the swapfile) when under memory pressure and forced to drop GEM objects from memory. In effect, this should just allow us to discard unused pages for memory reclaim and to start writeback earlier. v2: Hugh Dickins warned that explicitly starting writeback from shrink_slab was prone to deadlocks within shmemfs. Cc: Hugh Dickins hu...@google.com Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Good news! QA have declared that this series really does prevent the random OOM where we have completely unused swap. So all it needs is someone brave enough to review. Awesome work. Do we need the additional patch you've just posted to improve the writeback stalling after calling shrinkers too, or is that not required? That looks to be required (or at least I hope it provokes the mm gods into doing something sensible) for a different problem. However, that test is still behaving strangely (inactive_anon =~ 2x shmem, it should be almost equal) as it appears that there is severe overallocation, or a leak. But that we can generate massive amounts of writeback from our shrinker which may not be cleared in time for the allocation to succeed is a problem (addressed by that mm patch). -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages
On Thu, Apr 10, 2014 at 10:12:39AM +, Gupta, Sourab wrote: On Wed, 2014-04-09 at 13:06 +, Daniel Vetter wrote: On Tue, Apr 08, 2014 at 06:53:03AM +, Gupta, Sourab wrote: On Tue, 2014-04-08 at 06:45 +, Chris Wilson wrote: On Tue, Apr 08, 2014 at 04:32:02AM +, Gupta, Sourab wrote: Hi Rodrigo, In this patch, while freeing the purgeable stolen object, the memory node also has to be freed, so as to make space for new object. We need to call drm_mm_remove_node while freeing obj. The below modification patch was floated earlier for this purpose: http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html Right, I have a v2 locally with the fix you identified. -Chris Ok, Thanks Chris. I'd really prefer if someone would pick up all the stolen/create2_ioctl/whatever patches, pack them up into a polished series, add the testcases and submit this all for review and merging. Otherwise this will linger forever and we'll get nowhere. Chris seems swamped with other stuff, so Sourab could you please take a look at this? Please check with your manager that you have sufficient bandwidth to pull this through. I'll be on vacation from next week, so I'll be able to gauge this better after coming back. Nevertheless, I have some questions regarding the expectation of userspace code changes required for these patches (i.e. libdrm changes and igt testcases) 1) For libdrm , I am assuming, a counterpart of drm_intel_gem_bo_alloc_tiled() function would call the create2 ioctl and take in the parameters needed. Should the caching of objects from libdrm need to take care of both the placement domains seperately (as in different sets of bo buckets)? Should libdrm be transparent to all the combinations of different parameters being passed by user or should the prohibited combinations be disallowed from libdrm side? I'm not sure whether we need a cache implemented in libdrm. Since stolen objects are fairly special it's probably easier to just have a simple linear cache tailor-made in the respective UMD. So just exposing create2_ioctl should be good enough. 2) For the igt, since we have a lot of parameters exposed to user, the number of subtests required may be huge and still they may not test out everything. So, Is the expectation here to have exhaustive test cases for all the parameters (tiling/cache/domain/madvise/offset etc.) going in as input to the create2 ioctl? For eg. let us say we are going to check the render copy operation of src and dest bo's. Do we need to provide all possible combinations of different (create2 ioctl) input parameters to these src and dest bo's and then run the render copy test for all these combinations. Any guiding pointers from your side as to how we may go about the igt testcases? At a high-level there's two parts for igt tests: - First is functional tests, where we try to make sure that the feature actually works. I.e. allocate some stolen memory and then do something with it, making sure that data access for the gpu and similar things work. For this we just want some reasonable base coverage so that when we hit a bug somewhere it's easy to extend the testcase to cover that bug with a specific subtest. - Then there's interface testing. kernel/userspace is a trust barrier, so we need to make sure that evil userspace can't make the kernel crash with some crazy invalid combination of flags or operations (like create a stolen object and then try to mmap it). Since this is security relevant and also since we can't ever change established userspace ABI I want full coverage of all cases. But this is just about detecting abuse correctly, no functional tests here. For details see my two blog posts on the topic: http://blog.ffwll.ch/2013/11/botching-up-ioctls.html http://blog.ffwll.ch/2013/11/testing-requirements-for-drmi915.html Cheers, 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/bdw: Use timeout mode for RC6 on bdw
On Thu, Apr 10, 2014 at 08:29:55PM +, O'Rourke, Tom wrote: Higher RC6 residency is observed using timeout mode instead of EI mode. This applies to Broadwell only. The difference is particularly noticeable with video playback. Issue: VIZ-3778 Change-Id: I62bb12e21caf19651034826b45cde7f73a80938d Signed-off-by: Tom O'Rourke Tom.O'rou...@intel.com How recent a nightly branch have you used to obtain these results? Chris just fixed some serious bugs in the gpu booster logic which would have affected all intermediate workloads. -Daniel He must not be using nightly if he has any BDW RC6 residency at all. [TOR:] Ben is correct. I was testing mostly with a kernel for Android. I also tested with Ben's broadwell branch and saw similar improvement. Ok I think it'd be good to have this when we actually merge/enable the final pieces of the bdw rc6 code. Can you please work together with Ben to make sure this patch isn't lost? Or who is shepherding bdw rc6 nowadays? -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/bdw: BDW swizzling in done by the memory controller
On Thu, Apr 10, 2014 at 05:24:08PM +0100, Damien Lespiau wrote: Instead of needing to configure swizzling in 3 units (GAM, GT, DE), the memory controller is in charge of doing them on BDW. As a consequence all those swizzling bits are reserved. As specs put it: Before Gen8, there was a historical configuration control field to swizzle address bit[6] for in X/Y tiling modes. This was set in three different places: TILECTL[1:0], ARB_MODE[5:4], and DISP_ARB_CTL[14:13] For Gen8 the swizzle fields are all reserved, and the CPU's memory controller performs all address swizzling modifications. This also means that user space doesn't have to manually swizzle when accessing tiled buffers from the CPU, and so we always return I915_BIT_6_SWIZZLE_NONE from i915_gem_detect_bit_6_swizzle(), which short-circuits the initialization of the registers mentionned above in i915_gem_init_swizzling(). Signed-off-by: Damien Lespiau damien.lesp...@intel.com Afaik the memory controller has always done the swizzling. What this pile of bits controls is the _additional_ swizzling done to improve the access pattern for 2d data, i.e. everything X and Y tiled. The theory of operation is that the additional swizzling improves access patterns when walking in Y direction on a surface. And when we've last looked at this (well Chris) it seemed to indeed have improved sampler performace. The downside is that it's a bit a pain for userspace since essentially userspace has to deal with 4 tiling formats instead of just 2, but we have all the code for that. So not yet sold on the story here, at least for gen8/bdw. Apparently people haven't screamed about this yet, so it probably works. -Daniel --- drivers/gpu/drm/i915/i915_gem.c| 2 -- drivers/gpu/drm/i915/i915_gem_tiling.c | 10 +- drivers/gpu/drm/i915/i915_reg.h| 1 - 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 85c9cf0..9032c1b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4325,8 +4325,6 @@ void i915_gem_init_swizzling(struct drm_device *dev) I915_WRITE(ARB_MODE, _MASKED_BIT_ENABLE(ARB_MODE_SWIZZLE_SNB)); else if (IS_GEN7(dev)) I915_WRITE(ARB_MODE, _MASKED_BIT_ENABLE(ARB_MODE_SWIZZLE_IVB)); - else if (IS_GEN8(dev)) - I915_WRITE(GAMTARBMODE, _MASKED_BIT_ENABLE(ARB_MODE_SWIZZLE_BDW)); else BUG(); } diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c index cb150e8..a5ddf12 100644 --- a/drivers/gpu/drm/i915/i915_gem_tiling.c +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c @@ -91,7 +91,15 @@ i915_gem_detect_bit_6_swizzle(struct drm_device *dev) uint32_t swizzle_x = I915_BIT_6_SWIZZLE_UNKNOWN; uint32_t swizzle_y = I915_BIT_6_SWIZZLE_UNKNOWN; - if (IS_VALLEYVIEW(dev)) { + if (INTEL_INFO(dev)-gen = 8) { + /* + * On BDW+, the CPU memory controller performs all address + * swizzling modifications. This condition also catches CHV, + * where swizzling is not supported. + */ + swizzle_x = I915_BIT_6_SWIZZLE_NONE; + swizzle_y = I915_BIT_6_SWIZZLE_NONE; + } else if (IS_VALLEYVIEW(dev)) { swizzle_x = I915_BIT_6_SWIZZLE_NONE; swizzle_y = I915_BIT_6_SWIZZLE_NONE; } else if (INTEL_INFO(dev)-gen = 6) { diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 01c05af..faba21b 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -781,7 +781,6 @@ enum punit_power_well { #define ARB_MODE_SWIZZLE_IVB (15) #define GAMTARBMODE 0x04a08 #define ARB_MODE_BWGTLB_DISABLE (19) -#define ARB_MODE_SWIZZLE_BDW (11) #define RENDER_HWS_PGA_GEN7 (0x04080) #define RING_FAULT_REG(ring) (0x4094 + 0x100*(ring)-id) #define RING_FAULT_GTTSEL_MASK (111) -- 1.8.3.1 ___ 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: Add 42ms delay for IPS disable
On Thu, Apr 10, 2014 at 11:38:21PM +, Runyan, Arthur J wrote: Ben explained some of the fine details of the code to me, and I'm happy. Reviewed-by: Art Runyan arthur.j.run...@intel.com From: Ben Widawsky benjamin.widaw...@linux.intel.com This is a requirement added to the spec. This patch will prevent persistent corruption on the display. v2: Make the wait before the vblank wait. (Art) Try to finish early by polling the register s/present/prevent (Chris) Cc: Art Runyan arthur.j.run...@intel.com Signed-off-by: Ben Widawsky b...@bwidawsk.net Queued for -next, thanks for the patch. --- drivers/gpu/drm/i915/intel_display.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7f02444..05c60b1 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3583,10 +3583,13 @@ void hsw_disable_ips(struct intel_crtc *crtc) return; assert_plane_enabled(dev_priv, crtc-plane); -if (IS_BROADWELL(crtc-base.dev)) { +if (IS_BROADWELL(dev)) { mutex_lock(dev_priv-rps.hw_lock); WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL, 0)); mutex_unlock(dev_priv-rps.hw_lock); +/* wait for pcode to finish disabling IPS, which may take up to 42ms */ +if (wait_for((I915_READ(IPS_CTL) IPS_ENABLE) == 0, 42)) +DRM_DEBUG_KMS(Timed out waiting for IPS disable\n); I've upgraded this to an ERROR to make sure we'll get noticed if it happens. -Daniel } else { I915_WRITE(IPS_CTL, 0); POSTING_READ(IPS_CTL); -- 1.9.1 ___ 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/bdw: BDW swizzling in done by the memory controller
On Fri, Apr 11, 2014 at 11:09:03AM +0200, Daniel Vetter wrote: Afaik the memory controller has always done the swizzling. What this pile of bits controls is the _additional_ swizzling done to improve the access pattern for 2d data, i.e. everything X and Y tiled. The theory of operation is that the additional swizzling improves access patterns when walking in Y direction on a surface. And when we've last looked at this (well Chris) it seemed to indeed have improved sampler performace. And recall this was around SNB time. Jesse keeps claiming that IVB+ doesn't benefit, and I'm inclined to believe him. At some point, we should test again. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v9 3/6] drm/i915: Add support for DRRS to switch RR
On Fri, Apr 11, 2014 at 02:48:53PM +0530, Vandana Kannan wrote: On Apr-10-2014 2:28 PM, Daniel Vetter wrote: On Thu, Apr 10, 2014 at 11:43:15AM +0300, Jani Nikula wrote: Reviewed-by: Jani Nikula jani.nik...@intel.com On Sat, 05 Apr 2014, Vandana Kannan vandana.kan...@intel.com wrote: From: Pradeep Bhat pradeep.b...@intel.com This patch computes and stored 2nd M/N/TU for switching to different refresh rate dynamically. PIPECONF_EDP_RR_MODE_SWITCH bit helps toggle between alternate refresh rates programmed in 2nd M/N/TU registers. v2: Daniel's review comments Computing M2/N2 in compute_config and storing it in crtc_config v3: Modified reference to edp_downclock and edp_downclock_avail based on the changes made to move them from dev_private to intel_panel. v4: Modified references to is_drrs_supported based on the changes made to rename it to drrs_support. v5: Jani's review comments Removed superfluous return statements. Changed support for Gen 7 and above. Corrected indentation. Re-structured the code which finds crtc and connector from encoder. Changed some logs to be less verbose. v6: Modifying i915_drrs to include only intel connector as intel_dp can be derived from intel connector when required. v7: As per internal review comments, acquiring mutex just before accessing drrs RR. As per Chris's review comments, added documentation about the use of locking in the function. v8: Incorporated Jani's review comments. Removed reference to edp_downclock. v9: Jani's review comments. Modified comment in set_drrs. Changed index to type edp_drrs_refresh_rate_type. Check if PSR is enabled before setting registers fo DRRS. Signed-off-by: Pradeep Bhat pradeep.b...@intel.com Signed-off-by: Vandana Kannan vandana.kan...@intel.com Cc: Jani Nikula jani.nik...@linux.intel.com Queued for -next, thanks for the patch. One thing that's missing though is the state readout and cross-check support for this new bit of crtc-config data. We need to add that before putting this to real use. -Daniel Hi Daniel, Could you please elaborate on your input above - on the missing code and cross-checking for support part? If you add new state to crtc-config then you need to add the relevan readout code for that state (see the code in check_crtc_state) and ofc also add it to intel_pipe_config_compare. This is a debug feature of our driver to make sure we never lose track of things and thus far has been extremely helpful in catching issues early. -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
[Intel-gfx] [PULL] drm-intel-fixes
Hi Dave, here's the first batch of fixes for 3.15. BR, Jani. The following changes since commit c39b06951f1dc2e384650288676c5b7dcc0ec92c: DRM: armada: fix corruption while loading cursors (2014-04-08 10:51:03 +1000) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/drm-intel-fixes-2014-04-11 for you to fetch changes up to 691e6415c891b8b2b082a120b896b443531c4d45: drm/i915: Always use kref tracking for all contexts. (2014-04-11 13:29:51 +0300) Chris Wilson (1): drm/i915: Always use kref tracking for all contexts. Daniel Vetter (2): drm/mm: Don't WARN if drm_mm_reserve_node drm/i915: Disable self-refresh for untiled fbs on i915gm Jani Nikula (2): drm/i915: check VBT for supported backlight type drm/i915: do not setup backlight if not available according to VBT drivers/gpu/drm/drm_mm.c |2 - drivers/gpu/drm/i915/i915_drv.h|9 +- drivers/gpu/drm/i915/i915_gem.c|2 +- drivers/gpu/drm/i915/i915_gem_context.c| 218 +++- drivers/gpu/drm/i915/i915_gem_execbuffer.c |2 +- drivers/gpu/drm/i915/intel_bios.c | 10 ++ drivers/gpu/drm/i915/intel_bios.h |3 + drivers/gpu/drm/i915/intel_panel.c |5 + drivers/gpu/drm/i915/intel_pm.c| 10 ++ 9 files changed, 122 insertions(+), 139 deletions(-) -- Jani Nikula, Intel Open Source Technology Center pgpN0L1kTQl79.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/bdw: BDW swizzling in done by the memory controller
On Fri, Apr 11, 2014 at 11:09:03AM +0200, Daniel Vetter wrote: So not yet sold on the story here, at least for gen8/bdw. Apparently people haven't screamed about this yet, so it probably works. Having thought about it a bit more, I don't see how the CPU side would know about the tiling layout of the surfaces it accesses, so the remaining options I see: - we still need to swizzle the address on the CPU side - bit 6 swizzling for X/Y tiling is just gone and the optimal use of the RAM is left to the memory controller. If that's the case, we should see things failing soon enough So, until any failure case, meh. Just one thing to remember is that the swizzling bits we set are possibly reserved and may be no-ops. -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Adding debug code for dp_m2_n2 in crtc_config
Adding relevant read out comparison code, in check_crtc_state, for the new member of crtc_config, dp_m2_n2, which was introduced to store link_m_n values for a DP downclock mode (if available). Suggested by Daniel. Signed-off-by: Vandana Kannan vandana.kan...@intel.com Cc: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/intel_display.c | 15 +++ 1 file changed, 15 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1af1d14..36fc034 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9199,6 +9199,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, pipe_config-dp_m_n.gmch_m, pipe_config-dp_m_n.gmch_n, pipe_config-dp_m_n.link_m, pipe_config-dp_m_n.link_n, pipe_config-dp_m_n.tu); + + DRM_DEBUG_KMS(dp: %i, gmch_m2: %u, gmch_n2: %u, link_m2: %u, link_n2: %u, tu2: %u\n, + pipe_config-has_dp_encoder, + pipe_config-dp_m2_n2.gmch_m, + pipe_config-dp_m2_n2.gmch_n, + pipe_config-dp_m2_n2.link_m, + pipe_config-dp_m2_n2.link_n, + pipe_config-dp_m2_n2.tu); + DRM_DEBUG_KMS(requested mode:\n); drm_mode_debug_printmodeline(pipe_config-requested_mode); DRM_DEBUG_KMS(adjusted mode:\n); @@ -9619,6 +9628,12 @@ intel_pipe_config_compare(struct drm_device *dev, PIPE_CONF_CHECK_I(dp_m_n.link_n); PIPE_CONF_CHECK_I(dp_m_n.tu); + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_m); + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_n); + PIPE_CONF_CHECK_I(dp_m2_n2.link_m); + PIPE_CONF_CHECK_I(dp_m2_n2.link_n); + PIPE_CONF_CHECK_I(dp_m2_n2.tu); + PIPE_CONF_CHECK_I(adjusted_mode.crtc_hdisplay); PIPE_CONF_CHECK_I(adjusted_mode.crtc_htotal); PIPE_CONF_CHECK_I(adjusted_mode.crtc_hblank_start); -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v9 3/6] drm/i915: Add support for DRRS to switch RR
On Apr-11-2014 2:56 PM, Daniel Vetter wrote: On Fri, Apr 11, 2014 at 02:48:53PM +0530, Vandana Kannan wrote: On Apr-10-2014 2:28 PM, Daniel Vetter wrote: On Thu, Apr 10, 2014 at 11:43:15AM +0300, Jani Nikula wrote: Reviewed-by: Jani Nikula jani.nik...@intel.com On Sat, 05 Apr 2014, Vandana Kannan vandana.kan...@intel.com wrote: From: Pradeep Bhat pradeep.b...@intel.com This patch computes and stored 2nd M/N/TU for switching to different refresh rate dynamically. PIPECONF_EDP_RR_MODE_SWITCH bit helps toggle between alternate refresh rates programmed in 2nd M/N/TU registers. v2: Daniel's review comments Computing M2/N2 in compute_config and storing it in crtc_config v3: Modified reference to edp_downclock and edp_downclock_avail based on the changes made to move them from dev_private to intel_panel. v4: Modified references to is_drrs_supported based on the changes made to rename it to drrs_support. v5: Jani's review comments Removed superfluous return statements. Changed support for Gen 7 and above. Corrected indentation. Re-structured the code which finds crtc and connector from encoder. Changed some logs to be less verbose. v6: Modifying i915_drrs to include only intel connector as intel_dp can be derived from intel connector when required. v7: As per internal review comments, acquiring mutex just before accessing drrs RR. As per Chris's review comments, added documentation about the use of locking in the function. v8: Incorporated Jani's review comments. Removed reference to edp_downclock. v9: Jani's review comments. Modified comment in set_drrs. Changed index to type edp_drrs_refresh_rate_type. Check if PSR is enabled before setting registers fo DRRS. Signed-off-by: Pradeep Bhat pradeep.b...@intel.com Signed-off-by: Vandana Kannan vandana.kan...@intel.com Cc: Jani Nikula jani.nik...@linux.intel.com Queued for -next, thanks for the patch. One thing that's missing though is the state readout and cross-check support for this new bit of crtc-config data. We need to add that before putting this to real use. -Daniel Hi Daniel, Could you please elaborate on your input above - on the missing code and cross-checking for support part? If you add new state to crtc-config then you need to add the relevan readout code for that state (see the code in check_crtc_state) and ofc also add it to intel_pipe_config_compare. This is a debug feature of our driver to make sure we never lose track of things and thus far has been extremely helpful in catching issues early. -Daniel I have submitted a patch adding this piece of code. Please have a look.. http://lists.freedesktop.org/archives/intel-gfx/2014-April/043569.html - Vandana ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/bdw: BDW swizzling in done by the memory controller
On Fri, Apr 11, 2014 at 1:10 PM, Damien Lespiau damien.lesp...@intel.com wrote: On Fri, Apr 11, 2014 at 11:09:03AM +0200, Daniel Vetter wrote: So not yet sold on the story here, at least for gen8/bdw. Apparently people haven't screamed about this yet, so it probably works. Having thought about it a bit more, I don't see how the CPU side would know about the tiling layout of the surfaces it accesses, so the remaining options I see: - we still need to swizzle the address on the CPU side - bit 6 swizzling for X/Y tiling is just gone and the optimal use of the RAM is left to the memory controller. If that's the case, we should see things failing soon enough So, until any failure case, meh. Just one thing to remember is that the swizzling bits we set are possibly reserved and may be no-ops. That's very easy to figure out. Set the bit in TILE_CTL and GAMT_ARBMODE differently and see what happens. blt vs gtt mmap access would be different. igt has testcases which will catch this. At least on simulation this blew up rather badly ;-) -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 v4 3/3] drm/i915: New drm crtc property for varying the size of borders
On Fri, Apr 11, 2014 at 08:34:08AM +0530, Akash Goel wrote: On Thu, 2014-04-10 at 14:34 +0300, Ville Syrjälä wrote: On Thu, Apr 10, 2014 at 01:13:56PM +0530, Akash Goel wrote: On Tue, 2014-04-08 at 19:28 +0300, Ville Syrjälä wrote: On Wed, Mar 26, 2014 at 09:25:12AM +0530, akash.g...@intel.com wrote: From: Akash Goel akash.g...@intel.com This patch adds a new drm crtc property for varying the size of the horizontal vertical borers of the output/display window. This will control the output of Panel fitter. v2: Added a new check for the invalid border size input v3: Fixed bugs in output window calculation Removed superfluous checks v4: Added the capability to forecfully enable the Panel fitter. The property value is of 64 bits, first 32 bits are used for border dimensions. The 33rd bit can be used to forcefully enable the panel fitter. This is useful for Panels which do not override the User specified Pipe timings. Testcase: igt/kms_panel_fitter_test Signed-off-by: Akash Goel akash.g...@intel.com --- drivers/gpu/drm/i915/i915_drv.h | 7 ++ drivers/gpu/drm/i915/intel_display.c | 39 +++- drivers/gpu/drm/i915/intel_drv.h | 5 + drivers/gpu/drm/i915/intel_panel.c | 176 --- 4 files changed, 211 insertions(+), 16 deletions(-) snip @@ -42,6 +57,60 @@ intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode, drm_mode_set_crtcinfo(adjusted_mode, 0); } +void +intel_pch_manual_panel_fitting(struct intel_crtc *intel_crtc, + struct intel_crtc_config *pipe_config) +{ + struct drm_display_mode *adjusted_mode; + int x, y; + u32 pf_horizontal_ratio, pf_vertical_ratio; + u32 tot_width, tot_height; + u32 src_width, src_height; /* pipesrc.x, pipesrc.y */ + u32 dst_width, dst_height; + + adjusted_mode = pipe_config-adjusted_mode; + + src_width = pipe_config-pipe_src_w; + src_height = pipe_config-pipe_src_h; + + tot_width = adjusted_mode-hdisplay; + tot_height = adjusted_mode-vdisplay; + + /* + * Having non zero borders will reduce the size of 'HACTIVE/VACTIVE' + * region. So (HACTIVE - Left border - Right Border) * + * (VACTIVE - Top Border - Bottom border) will effectively be the + * output rectangle on screen + */ + dst_width = tot_width - + (((intel_crtc-border_size 16) 0x) * 2); + dst_height = tot_height - + ((intel_crtc-border_size 0x) * 2); I'm thinking that we should allow the user to specify each border width individually rather than just assume left==right and top==bottom. Sorry I thought that the Top/Bottom left/Right borders would be symmetric only. I don't see a reason to limit ourselves here. Fine, will extend this property to set each border separately. Can I use the 12 bits for each border value, as that shall be sufficient. Maybe just add separate properties for each border value. We already have similar properties for TV outputs. Tried setting the borders on EDP HDMI panels by manipulating the Pipe timings (through the logic used in 'centre_horizontally' 'centre_vertically' functions), but it didn't work. Is this logic effective for the LVDS panel only ? Could be. Certainly the border enable bit is there only for LVDS. The gmch panel fitter isn't very flexible so it's possible we can't actually make it do many of the things the pch pfit can do. Yes the GMCH panel fitter function is not equally capable as the PCH counterpart. Here except the LVDS panel, it seems that borders cannot be realized on any other panel, just via the manipulation of Pipe timings (the way it can be done in PCH one). For the same reason the 'Center' Panel fitting mode of scaling mode property is not working on VLV (at least for HDMI/EDP panels). What happens if we set up the pfit to use manual scaling ratios but configure both scaling ratios so that scaled image won't fill the active video region in either direction? Does it position the scaled image at coordinates 0,0 and simply scan black the rest of the time after it's run out of source pixel data? Or does it automagically center the image and scan black on both sides? Or does it fail in some way? Already tried that, but in vain. As per the VLV spec, the support for Manual scaling ratio mode itself has been de-featured, so it didn't work at all. So Auto/LetterBox/PillarBox modes are being supported. I see. As a next step tried to manipulate the Pipe timings, so as to produce the borders on 4 sides of the panel.
[Intel-gfx] [Bug] Wrong screen resolution
Description: Whenever I restart the X server there is a small (20%?) chance that the KDE splash picture is not displayed as expected (1280x1024). Instead I see a smaller version of that picture (1024x768?) in the upper left corner of the monitor, but the correct 1280x1024 video mode is used by xorg. Absolutely _nothing_ unusual in system/xorg logs. After that, everything works fine, with one exception: mplayer fullscreen mode is broken. The video is displayed correctly, but the area not used by the video itself is only partially set to black, the upper right and lower right area contain garbage. If the splash screen is displayed correctly, mplayer full screen mode works correctly too. Most of the time another restart of X corrects the problem. Any idea how to debug that situation? Hardware: === AOpen i915GMm-hfs, Pentium-M Dothan, Eizo L557 monitor (1280x1024) Software: == openSuSE, KDE, all recent versions of linux kernel and xorg, last good unknown. cu, Knut ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Bug] Wrong screen resolution
On Fri, Apr 11, 2014 at 01:46:40PM +0200, Knut Petersen wrote: Description: Whenever I restart the X server there is a small (20%?) chance that the KDE splash picture is not displayed as expected (1280x1024). Instead I see a smaller version of that picture (1024x768?) in the upper left corner of the monitor, but the correct 1280x1024 video mode is used by xorg. Absolutely _nothing_ unusual in system/xorg logs. After that, everything works fine, with one exception: mplayer fullscreen mode is broken. The video is displayed correctly, but the area not used by the video itself is only partially set to black, the upper right and lower right area contain garbage. If the splash screen is displayed correctly, mplayer full screen mode works correctly too. Most of the time another restart of X corrects the problem. Any idea how to debug that situation? What exactly does Xorg.0.log and xrandr say? Have you looked at the drm.debug=6 dmesg? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 3/3] kms_universal_plane: Universal plane testing
On Fri, Apr 11, 2014 at 11:22:39AM +0200, Daniel Vetter wrote: On Thu, Apr 10, 2014 at 05:26:25PM -0700, Matt Roper wrote: Add a simple test to exercise universal plane support. Signed-off-by: Matt Roper matthew.d.ro...@intel.com Looks like a good functional test, but I think we need to add a bit more api nastiness still. So no functional tests with CRCs, but just tests to make sure the kernel doesn't fall over. - primary plane set_plane calls vs. legacy setcrtc primary plane updates. We'll very likely have mixed userspace (e.g. boot splash vs. display manager). E.g. disable primary plane (but keep everything working), then setCrtc a new plane framebuffer. - primary plane vs. other ioctls. Might be easier to extend existing tests for this. E.g. doing a pageflip ioctl if the primary plane is off (might need to decide what we really want to do and if we decide that it should enable the primary plane then we need a CRC based test to make sure that the transition is perfect). Or primary plane changes vs. dpms and suspend/resume. For those functional checks based on CRC would be good to make sure we properly restore everything. - Maybe exercise some of the checks in the primary plane helper to make sure they work. In the future we'll probably lift those limitations (not on current hw afaik though), but then we can adjust those tests to skip on these platforms. - Anything else that was pointed out in review or was tricky while developing this stuff. More ideas! My main concern is interactions with existing features when the primary plane is disabled and no other plane/cursor enabled, i.e. when we scan out a solid black. - solid black vs. dpms for all connector/pipe pairings. The modeset sequence is different for different connectors and my gut says not enabling the primary plane might cause trouble. - solid black vs. vblank events. When transition through a no planes situation we need to make sure that vblank events keep on working. A testcase (maybe in combination of some of the dpms and susped/resume case above too) would be good. I'll leave it to you to somewhat sensibly group all these ideas into real testcases ;-) For further inspiration maybe look at the corner cases existing kms_* tests exercise a bit. -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
[Intel-gfx] [PATCH] drm/plane_helper: don't disable plane in destroy function
By the time drm_mode_config_cleanup calls this all the hw state should be cleaned up already - we even have a WARN right before calling plane-destroy callbacks asserting that all framebuffers are gone. So trying to disable things harder is a bit a bug. Caught by Thierry since it resulted in some mode_config.mutex locking backtraces. Cc: Thierry Reding tred...@nvidia.com Cc: Matt Roper matthew.d.ro...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/drm_plane_helper.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index e768d35ff22e..a3c9c6e11ee9 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -255,7 +255,6 @@ EXPORT_SYMBOL(drm_primary_helper_disable); */ void drm_primary_helper_destroy(struct drm_plane *plane) { - plane-funcs-disable_plane(plane); drm_plane_cleanup(plane); kfree(plane); } -- 1.8.5.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes
Please don't drop the mailing list when discussing patches. And nope, conditioning this on gen6+ won't help at all, since I have a gen6 and gen7 machine here which don't have reliable hdmi live status. Afaik that is _really_ broken across the board and going with the fancy invalidation scheme and delayed work items is the way forward. Cheers, Daniel On Fri, Apr 11, 2014 at 2:51 PM, Sharma, Shashank shashank.sha...@intel.com wrote: Hi Daniel, First of all, sorry for the delay in coming up with the new patch set. I got stuck with some commercial projects :) . There were few review comments what you gave for the previous optimization patch, those were: 1. This might break the old HWs. Few of them might not even be HPD capable, so its required to handle them. 2. Do not rely on the live_status check, as that HW is not yet proven. And you recommended to have a WQ, which will invalidate the cached HDMI EDID after a timeout. I have written another patch, which is addressing both of the above comments, but doesn't use a WQ. Before sending this to ML, I wanted to have an opinion from you, if this qualifies the design you were expecting. I will be really glad, if you can spare some time, and just have a top level view on the patch, and give your opinion. If you think that this is still not the way, and making a WQ is a better approach, I would gladly create a patch which applies that. Or if you will prefer to have this reviewed in the mailing list only, please accept my apologies for direct contact, and just let me know so. I will publish this in ML. The patch is (It's also attached): From e7ec4e4827615c0ff3a4ddb020df35fdf1b0b82f Mon Sep 17 00:00:00 2001 From: Shashank Sharma shashank.sha...@intel.com Date: Fri, 11 Apr 2014 18:02:50 +0530 Subject: [PATCH] drm/i915: Cache HDMI EDID for future reference This patch contains following changes: 1.Cache HDMI EDID and optimize detect() calls, which are frequently called from userspace, causing un-necessary EDID reads. 2.This cached EDID will be used for detection of the status of HDMI connector, for Non HPD detect() calls. HPD calls will update cached EDID. 3.This optimization is kept for new HW's (Gen6 and +), so that It will not break old HWs who may not be even HPD capable. Signed-off-by: Shashank Sharma shashank.sha...@intel.com --- drivers/gpu/drm/i915/intel_drv.h |1 + drivers/gpu/drm/i915/intel_hdmi.c | 28 ++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 42762b7..b7911df 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -478,6 +478,7 @@ struct intel_hdmi { const void *frame, ssize_t len); void (*set_infoframes)(struct drm_encoder *encoder, struct drm_display_mode *adjusted_mode); + struct edid *edid; }; #define DP_MAX_DOWNSTREAM_PORTS0x10 diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index b0413e1..33550ca 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -938,7 +938,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) hdmi_to_dig_port(intel_hdmi); struct intel_encoder *intel_encoder = intel_dig_port-base; struct drm_i915_private *dev_priv = dev-dev_private; - struct edid *edid; + struct edid *edid = NULL; enum intel_display_power_domain power_domain; enum drm_connector_status status = connector_status_disconnected; @@ -948,6 +948,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) power_domain = intel_display_port_power_domain(intel_encoder); intel_display_power_get(dev_priv, power_domain); + /* + * Support EDID caching only for new architectures + * Let old architectures probe and force read EDID + */ + if (INTEL_INFO(dev)-gen = 6) { + if (force intel_hdmi-edid + (connector-status != connector_status_unknown)) { + /* Optimize userspace query, read EDID only + in case of a real hot plug */ + DRM_DEBUG_KMS(HDMI %s, intel_hdmi-edid ? + Connected : Disconnected); + return connector-status; + } + } + intel_hdmi-has_hdmi_sink = false; intel_hdmi-has_audio = false; intel_hdmi-rgb_quant_range_selectable = false; @@ -964,10 +979,19 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) intel_hdmi-has_audio = drm_detect_monitor_audio(edid); intel_hdmi-rgb_quant_range_selectable =
[Intel-gfx] [PATCH] tests/gem_error_capture: Initial testcase for error state capture/dump
From: Oscar Mateo oscar.ma...@intel.com Signed-off-by: Oscar Mateo oscar.ma...@intel.com --- tests/.gitignore | 1 + tests/Makefile.sources| 1 + tests/gem_error_capture.c | 230 ++ 3 files changed, 232 insertions(+) create mode 100644 tests/gem_error_capture.c diff --git a/tests/.gitignore b/tests/.gitignore index 146bab0..945574c 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -27,6 +27,7 @@ gem_ctx_create gem_ctx_exec gem_double_irq_loop gem_dummy_reloc_loop +gem_error_capture gem_evict_alignment gem_evict_everything gem_exec_bad_domains diff --git a/tests/Makefile.sources b/tests/Makefile.sources index bf02a48..612beb6 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -24,6 +24,7 @@ TESTS_progs_M = \ gem_ctx_bad_exec \ gem_ctx_exec \ gem_dummy_reloc_loop \ + gem_error_capture \ gem_evict_alignment \ gem_evict_everything \ gem_exec_bad_domains \ diff --git a/tests/gem_error_capture.c b/tests/gem_error_capture.c new file mode 100644 index 000..bbf0f5d --- /dev/null +++ b/tests/gem_error_capture.c @@ -0,0 +1,230 @@ +/* + * Copyright © 2014 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the Software), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + * Authors: + *Oscar Mateo oscar.ma...@intel.com + * + */ + +/* + * Testcase: Check whether basic error state capture/dump mechanism is correctly + * working for all rings. + */ + +#include unistd.h +#include stdlib.h +#include stdint.h +#include stdio.h +#include string.h +#include fcntl.h +#include inttypes.h +#include errno.h +#include sys/stat.h +#include sys/ioctl.h +#include drm.h +#include ioctl_wrappers.h +#include drmtest.h +#include intel_io.h +#include igt_debugfs.h + +#define MAGIC_NUMBER 0x10001 +uint32_t batch[4] = {MI_NOOP, MI_BATCH_BUFFER_END, MAGIC_NUMBER, MAGIC_NUMBER}; + +static void stop_rings(void) +{ + int fd; + static const char buf[] = 0xf; + + fd = igt_debugfs_open(i915_ring_stop, O_WRONLY); + igt_assert(fd = 0); + + igt_assert(write(fd, buf, sizeof(buf)) == sizeof(buf)); + + close(fd); +} + +static bool rings_stopped(void) +{ + int fd; + static char buf[128]; + unsigned long long val; + + fd = igt_debugfs_open(i915_ring_stop, O_RDONLY); + igt_assert(fd = 0); + + igt_assert(read(fd, buf, sizeof(buf)) 0); + close(fd); + + sscanf(buf, %llx, val); + + return (bool)val; +} + +static void clear_error_state(void) +{ + int fd; + static const char buf[] = ; + + fd = igt_debugfs_open(i915_error_state, O_WRONLY); + igt_assert(fd = 0); + + igt_assert(write(fd, buf, sizeof(buf)) == sizeof(buf)); + close(fd); +} + +static void check_bb_contents(char **line, size_t *line_size, + FILE *file, int pos, uint32_t expected_value) +{ + char expected_line[32]; + + igt_assert(getline(line, line_size, file) 0); + + snprintf(expected_line, sizeof(expected_line), %08x : %08x, + 4*pos, expected_value); + + igt_assert(strstr(*line, expected_line)); +} + +static void check_error_state(const char *expected_ring_name, + uint64_t expected_offset) +{ + FILE *file; + int debug_fd; + char *line = NULL; + size_t line_size; + char *dashes = NULL; + char *ring_name = NULL; + uint32_t gtt_offset = 0; + bool matched; + int i; + + debug_fd = igt_debugfs_open(i915_error_state, O_RDONLY); + igt_assert(debug_fd = 0); + file = fdopen(debug_fd, r); + + while (getline(line, line_size, file) 0) { + dashes = strstr(line, ---); + if (dashes) { + ring_name = realloc(ring_name, dashes - line); + strncpy(ring_name, line, dashes - line); +
Re: [Intel-gfx] [PATCH] tests/gem_error_capture: Initial testcase for error state capture/dump
Daniel, is this what you had in mind? -- Oscar P.S. I just re-read the Jira task and realized that I am missing the check that the ring objects contains a reloc with MI_BB_START for your presumed batch object's address. I´ll add this and resubmit. -Original Message- From: Mateo Lozano, Oscar Sent: Friday, April 11, 2014 2:00 PM To: intel-gfx@lists.freedesktop.org Cc: Mateo Lozano, Oscar Subject: [PATCH] tests/gem_error_capture: Initial testcase for error state capture/dump From: Oscar Mateo oscar.ma...@intel.com Signed-off-by: Oscar Mateo oscar.ma...@intel.com --- tests/.gitignore | 1 + tests/Makefile.sources| 1 + tests/gem_error_capture.c | 230 ++ 3 files changed, 232 insertions(+) create mode 100644 tests/gem_error_capture.c diff --git a/tests/.gitignore b/tests/.gitignore index 146bab0..945574c 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -27,6 +27,7 @@ gem_ctx_create gem_ctx_exec gem_double_irq_loop gem_dummy_reloc_loop +gem_error_capture gem_evict_alignment gem_evict_everything gem_exec_bad_domains diff --git a/tests/Makefile.sources b/tests/Makefile.sources index bf02a48..612beb6 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -24,6 +24,7 @@ TESTS_progs_M = \ gem_ctx_bad_exec \ gem_ctx_exec \ gem_dummy_reloc_loop \ + gem_error_capture \ gem_evict_alignment \ gem_evict_everything \ gem_exec_bad_domains \ diff --git a/tests/gem_error_capture.c b/tests/gem_error_capture.c new file mode 100644 index 000..bbf0f5d --- /dev/null +++ b/tests/gem_error_capture.c @@ -0,0 +1,230 @@ +/* + * Copyright © 2014 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person +obtaining a + * copy of this software and associated documentation files (the +Software), + * to deal in the Software without restriction, including without +limitation + * the rights to use, copy, modify, merge, publish, distribute, +sublicense, + * and/or sell copies of the Software, and to permit persons to whom +the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the +next + * paragraph) shall be included in all copies or substantial portions +of the + * Software. + * + * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, +EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT +SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR +OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, +ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +DEALINGS + * IN THE SOFTWARE. + * + * Authors: + *Oscar Mateo oscar.ma...@intel.com + * + */ + +/* + * Testcase: Check whether basic error state capture/dump mechanism is +correctly + * working for all rings. + */ + +#include unistd.h +#include stdlib.h +#include stdint.h +#include stdio.h +#include string.h +#include fcntl.h +#include inttypes.h +#include errno.h +#include sys/stat.h +#include sys/ioctl.h +#include drm.h +#include ioctl_wrappers.h +#include drmtest.h +#include intel_io.h +#include igt_debugfs.h + +#define MAGIC_NUMBER 0x10001 +uint32_t batch[4] = {MI_NOOP, MI_BATCH_BUFFER_END, MAGIC_NUMBER, +MAGIC_NUMBER}; + +static void stop_rings(void) +{ + int fd; + static const char buf[] = 0xf; + + fd = igt_debugfs_open(i915_ring_stop, O_WRONLY); + igt_assert(fd = 0); + + igt_assert(write(fd, buf, sizeof(buf)) == sizeof(buf)); + + close(fd); +} + +static bool rings_stopped(void) +{ + int fd; + static char buf[128]; + unsigned long long val; + + fd = igt_debugfs_open(i915_ring_stop, O_RDONLY); + igt_assert(fd = 0); + + igt_assert(read(fd, buf, sizeof(buf)) 0); + close(fd); + + sscanf(buf, %llx, val); + + return (bool)val; +} + +static void clear_error_state(void) +{ + int fd; + static const char buf[] = ; + + fd = igt_debugfs_open(i915_error_state, O_WRONLY); + igt_assert(fd = 0); + + igt_assert(write(fd, buf, sizeof(buf)) == sizeof(buf)); + close(fd); +} + +static void check_bb_contents(char **line, size_t *line_size, + FILE *file, int pos, uint32_t expected_value) { + char expected_line[32]; + + igt_assert(getline(line, line_size, file) 0); + + snprintf(expected_line, sizeof(expected_line), %08x : %08x, + 4*pos, expected_value); + + igt_assert(strstr(*line, expected_line)); } + +static void check_error_state(const char *expected_ring_name, + uint64_t expected_offset) +{ + FILE *file; + int
Re: [Intel-gfx] [PATCH] drm/i915: get power domain in case the BIOS enabled eDP VDD
On Wed, Apr 09, 2014 at 06:47:33PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com If I unplug the eDP monitor, the BIOS of my machine will enable the VDD bit, then when the driver loads it will think VDD is enabled. It will detect that the eDP is not enabled and return false from intel_edp_init_connector. This will trigger a call to edp_panel_vdd_off_sync(), which trigger a WARN saying that the refcount of the power domain is less than zero. The problem happens because the driver gets a refcount whenever it enables the VDD bit, and puts the refcount whenever it disables the VDD bit. But on this case, the BIOS enabled VDD, so all we do is to call put() without calling get() first, so the code added is there to make sure we always have the get() in case the BIOS enabled the bit. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com Probably also Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76987 Paulo can you please polish the patch as requested by Chris and resubmit? Please also repoke the bug. Thanks, Daniel --- drivers/gpu/drm/i915/intel_dp.c | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index e48d47c..a432904 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3638,7 +3638,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, { struct drm_connector *connector = intel_connector-base; struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); - struct drm_device *dev = intel_dig_port-base.base.dev; + struct intel_encoder *intel_encoder = intel_dig_port-base; + struct drm_device *dev = intel_encoder-base.dev; struct drm_i915_private *dev_priv = dev-dev_private; struct drm_display_mode *fixed_mode = NULL; bool has_dpcd; @@ -3648,6 +3649,14 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, if (!is_edp(intel_dp)) return true; + /* The VDD bit needs a power domain reference, so if the bit is already + * enabled when we boot, grab this reference. */ + if (edp_have_panel_vdd(intel_dp)) { + enum intel_display_power_domain power_domain; + power_domain = intel_display_port_power_domain(intel_encoder); + intel_display_power_get(dev_priv, power_domain); + } + /* Cache DPCD and EDID for edp. */ intel_edp_panel_vdd_on(intel_dp); has_dpcd = intel_dp_get_dpcd(intel_dp); -- 1.9.0 ___ 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 0/2] Optimization on intel HDMI detect and get_modes
Thanks for the comments, Actually, we are not using live_status at all. The check for gen6 is only for EDID caching. So if the HW is = gen6 cache_edid. Else do not cache EDID, so that we will not block any of the old HW, which might not be HPD capable. Does it sound ok now :) ? Regards Shashank -Original Message- From: daniel.vet...@ffwll.ch [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Friday, April 11, 2014 6:28 PM To: Sharma, Shashank Cc: C, Ramalingam; Wang, Quanxian; intel-gfx Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes Please don't drop the mailing list when discussing patches. And nope, conditioning this on gen6+ won't help at all, since I have a gen6 and gen7 machine here which don't have reliable hdmi live status. Afaik that is _really_ broken across the board and going with the fancy invalidation scheme and delayed work items is the way forward. Cheers, Daniel On Fri, Apr 11, 2014 at 2:51 PM, Sharma, Shashank shashank.sha...@intel.com wrote: Hi Daniel, First of all, sorry for the delay in coming up with the new patch set. I got stuck with some commercial projects :) . There were few review comments what you gave for the previous optimization patch, those were: 1. This might break the old HWs. Few of them might not even be HPD capable, so its required to handle them. 2. Do not rely on the live_status check, as that HW is not yet proven. And you recommended to have a WQ, which will invalidate the cached HDMI EDID after a timeout. I have written another patch, which is addressing both of the above comments, but doesn't use a WQ. Before sending this to ML, I wanted to have an opinion from you, if this qualifies the design you were expecting. I will be really glad, if you can spare some time, and just have a top level view on the patch, and give your opinion. If you think that this is still not the way, and making a WQ is a better approach, I would gladly create a patch which applies that. Or if you will prefer to have this reviewed in the mailing list only, please accept my apologies for direct contact, and just let me know so. I will publish this in ML. The patch is (It's also attached): From e7ec4e4827615c0ff3a4ddb020df35fdf1b0b82f Mon Sep 17 00:00:00 2001 From: Shashank Sharma shashank.sha...@intel.com Date: Fri, 11 Apr 2014 18:02:50 +0530 Subject: [PATCH] drm/i915: Cache HDMI EDID for future reference This patch contains following changes: 1.Cache HDMI EDID and optimize detect() calls, which are frequently called from userspace, causing un-necessary EDID reads. 2.This cached EDID will be used for detection of the status of HDMI connector, for Non HPD detect() calls. HPD calls will update cached EDID. 3.This optimization is kept for new HW's (Gen6 and +), so that It will not break old HWs who may not be even HPD capable. Signed-off-by: Shashank Sharma shashank.sha...@intel.com --- drivers/gpu/drm/i915/intel_drv.h |1 + drivers/gpu/drm/i915/intel_hdmi.c | 28 ++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 42762b7..b7911df 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -478,6 +478,7 @@ struct intel_hdmi { const void *frame, ssize_t len); void (*set_infoframes)(struct drm_encoder *encoder, struct drm_display_mode *adjusted_mode); + struct edid *edid; }; #define DP_MAX_DOWNSTREAM_PORTS0x10 diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index b0413e1..33550ca 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -938,7 +938,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) hdmi_to_dig_port(intel_hdmi); struct intel_encoder *intel_encoder = intel_dig_port-base; struct drm_i915_private *dev_priv = dev-dev_private; - struct edid *edid; + struct edid *edid = NULL; enum intel_display_power_domain power_domain; enum drm_connector_status status = connector_status_disconnected; @@ -948,6 +948,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) power_domain = intel_display_port_power_domain(intel_encoder); intel_display_power_get(dev_priv, power_domain); + /* + * Support EDID caching only for new architectures + * Let old architectures probe and force read EDID + */ + if (INTEL_INFO(dev)-gen = 6) { + if (force intel_hdmi-edid + (connector-status != connector_status_unknown)) { + /* Optimize userspace query, read EDID only + in case of a real hot plug */ +
Re: [Intel-gfx] [PATCH] drm/plane_helper: don't disable plane in destroy function
On Fri, Apr 11, 2014 at 02:12:10PM +0200, Daniel Vetter wrote: By the time drm_mode_config_cleanup calls this all the hw state should be cleaned up already - we even have a WARN right before calling plane-destroy callbacks asserting that all framebuffers are gone. So trying to disable things harder is a bit a bug. Caught by Thierry since it resulted in some mode_config.mutex locking backtraces. Cc: Thierry Reding tred...@nvidia.com Cc: Matt Roper matthew.d.ro...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/drm_plane_helper.c | 1 - 1 file changed, 1 deletion(-) Reviewed-by: Thierry Reding tred...@nvidia.com Tested-by: Thierry Reding tred...@nvidia.com pgpWrUUpKUZRg.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Don't complain about stolen conflicts on gen3
Apparently stuff works that way on those machines. I agree with Chris' concern that this is a bit risky but imo worth a shot in -next just for fun. Afaics all these machines have the pci resources allocated like that by the BIOS, so I suspect that it's all ok. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76983 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=71031 Tested-by: lu hua huax...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/i915_gem_stolen.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index 62ef55ba061c..99d147af173a 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -93,7 +93,11 @@ static unsigned long i915_stolen_to_physical(struct drm_device *dev) r = devm_request_mem_region(dev-dev, base + 1, dev_priv-gtt.stolen_size - 1, Graphics Stolen Memory); - if (r == NULL) { + /* +* GEN3 firmware likes to smash pci bridges into the stolen +* range. Apparently this works. +*/ + if (r == NULL !IS_GEN3(dev)) { DRM_ERROR(conflict detected with stolen region: [0x%08x - 0x%08x]\n, base, base + (uint32_t)dev_priv-gtt.stolen_size); base = 0; -- 1.8.5.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Intel-specific primary plane handling (v2)
On Fri, Apr 11, 2014 at 11:34:36AM +0200, Daniel Vetter wrote: On Thu, Apr 10, 2014 at 05:24:36PM -0700, Matt Roper wrote: ... + /* setplane API takes shifted source rectangle values; unshift them */ + src_x = 16; + src_y = 16; + src_w = 16; + src_h = 16; + + /* +* Current hardware can't reposition the primary plane or scale it +* (although this could change in the future). +*/ + drm_rect_intersect(dest, clip); + if (dest.x1 != 0 || dest.y1 != 0 || + dest.x2 != crtc-mode.hdisplay || dest.y2 != crtc-mode.vdisplay) { + DRM_DEBUG_KMS(Primary plane must cover entire CRTC\n); + return -EINVAL; + } + + if (crtc_w != src_w || crtc_h != src_h) { + DRM_DEBUG_KMS(Can't scale primary plane\n); + return -EINVAL; + } Subpixel check seems to be missing. And can't we extract all these checks both here and from the primary plane helper? I guess there'll be other hw which doesn't have scaling primary planes, but which wants to allow primary plane enable/disable. I was a bit unsure about this. At first I thought I needed to check the subpixel part, but the DocBook reference indicates Devices that don't support subpixel plane coordinates can ignore the fractional part. which sounds to me like we're supposed to just silently ignore the subpixel bits on i915 and other devices that don't support it. Which would probably also mean that I should remove the (subpixel bits == 0) test from the primary helper... Matt -- 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
[Intel-gfx] [PATCH] tests/kms_flip_tiling: Fixes
- Wrap up testcase correctly into the magic code block. - Put local variables out of the longjmp danger zone. Cc: Ander Conselvan de Oliveira ander.conselvan.de.olive...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- tests/kms_flip_tiling.c | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/kms_flip_tiling.c b/tests/kms_flip_tiling.c index e70609d52e78..ca20ad96bc35 100644 --- a/tests/kms_flip_tiling.c +++ b/tests/kms_flip_tiling.c @@ -119,11 +119,10 @@ test_flip_changes_tiling(data_t *data, igt_output_t *output) } static data_t data; +igt_output_t *output; igt_main { - igt_output_t *output; - igt_skip_on_simulation(); igt_fixture { @@ -135,10 +134,10 @@ igt_main igt_display_init(data.display, data.drm_fd); } - igt_subtest_f(flip-changes-tiling); - - for_each_connected_output(data.display, output) - test_flip_changes_tiling(data, output); + igt_subtest_f(flip-changes-tiling) { + for_each_connected_output(data.display, output) + test_flip_changes_tiling(data, output); + } igt_fixture { igt_display_fini(data.display); -- 1.8.5.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/plane_helper: don't disable plane in destroy function
On Fri, Apr 11, 2014 at 02:12:10PM +0200, Daniel Vetter wrote: By the time drm_mode_config_cleanup calls this all the hw state should be cleaned up already - we even have a WARN right before calling plane-destroy callbacks asserting that all framebuffers are gone. So trying to disable things harder is a bit a bug. Caught by Thierry since it resulted in some mode_config.mutex locking backtraces. Cc: Thierry Reding tred...@nvidia.com Cc: Matt Roper matthew.d.ro...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch Reviewed-by: Matt Roper matthew.d.ro...@intel.com --- drivers/gpu/drm/drm_plane_helper.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index e768d35ff22e..a3c9c6e11ee9 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -255,7 +255,6 @@ EXPORT_SYMBOL(drm_primary_helper_disable); */ void drm_primary_helper_destroy(struct drm_plane *plane) { - plane-funcs-disable_plane(plane); drm_plane_cleanup(plane); kfree(plane); } -- 1.8.5.2 -- 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 0/2] Optimization on intel HDMI detect and get_modes
On Fri, Apr 11, 2014 at 01:23:43PM +, Sharma, Shashank wrote: Thanks for the comments, Actually, we are not using live_status at all. The check for gen6 is only for EDID caching. So if the HW is = gen6 cache_edid. Else do not cache EDID, so that we will not block any of the old HW, which might not be HPD capable. Oh, I've thought that this is incremental on top of something you already have. Does it sound ok now :) ? No. HPD is _NOT_ I repeat _NOT_ reliably. Not even on gen6+. live status simply reflects the hpd pin, if either doesn't work, then neither does the other one. Nacked-with-prejudice-by: Daniel Vetter daniel.vet...@ffwll.ch Cheers, Daniel Regards Shashank -Original Message- From: daniel.vet...@ffwll.ch [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Friday, April 11, 2014 6:28 PM To: Sharma, Shashank Cc: C, Ramalingam; Wang, Quanxian; intel-gfx Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes Please don't drop the mailing list when discussing patches. And nope, conditioning this on gen6+ won't help at all, since I have a gen6 and gen7 machine here which don't have reliable hdmi live status. Afaik that is _really_ broken across the board and going with the fancy invalidation scheme and delayed work items is the way forward. Cheers, Daniel On Fri, Apr 11, 2014 at 2:51 PM, Sharma, Shashank shashank.sha...@intel.com wrote: Hi Daniel, First of all, sorry for the delay in coming up with the new patch set. I got stuck with some commercial projects :) . There were few review comments what you gave for the previous optimization patch, those were: 1. This might break the old HWs. Few of them might not even be HPD capable, so its required to handle them. 2. Do not rely on the live_status check, as that HW is not yet proven. And you recommended to have a WQ, which will invalidate the cached HDMI EDID after a timeout. I have written another patch, which is addressing both of the above comments, but doesn't use a WQ. Before sending this to ML, I wanted to have an opinion from you, if this qualifies the design you were expecting. I will be really glad, if you can spare some time, and just have a top level view on the patch, and give your opinion. If you think that this is still not the way, and making a WQ is a better approach, I would gladly create a patch which applies that. Or if you will prefer to have this reviewed in the mailing list only, please accept my apologies for direct contact, and just let me know so. I will publish this in ML. The patch is (It's also attached): From e7ec4e4827615c0ff3a4ddb020df35fdf1b0b82f Mon Sep 17 00:00:00 2001 From: Shashank Sharma shashank.sha...@intel.com Date: Fri, 11 Apr 2014 18:02:50 +0530 Subject: [PATCH] drm/i915: Cache HDMI EDID for future reference This patch contains following changes: 1.Cache HDMI EDID and optimize detect() calls, which are frequently called from userspace, causing un-necessary EDID reads. 2.This cached EDID will be used for detection of the status of HDMI connector, for Non HPD detect() calls. HPD calls will update cached EDID. 3.This optimization is kept for new HW's (Gen6 and +), so that It will not break old HWs who may not be even HPD capable. Signed-off-by: Shashank Sharma shashank.sha...@intel.com --- drivers/gpu/drm/i915/intel_drv.h |1 + drivers/gpu/drm/i915/intel_hdmi.c | 28 ++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 42762b7..b7911df 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -478,6 +478,7 @@ struct intel_hdmi { const void *frame, ssize_t len); void (*set_infoframes)(struct drm_encoder *encoder, struct drm_display_mode *adjusted_mode); + struct edid *edid; }; #define DP_MAX_DOWNSTREAM_PORTS0x10 diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index b0413e1..33550ca 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -938,7 +938,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) hdmi_to_dig_port(intel_hdmi); struct intel_encoder *intel_encoder = intel_dig_port-base; struct drm_i915_private *dev_priv = dev-dev_private; - struct edid *edid; + struct edid *edid = NULL; enum intel_display_power_domain power_domain; enum drm_connector_status status = connector_status_disconnected; @@ -948,6 +948,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) power_domain =
Re: [Intel-gfx] [PATCH] drm/i915: Intel-specific primary plane handling (v2)
On Fri, Apr 11, 2014 at 07:17:41AM -0700, Matt Roper wrote: On Fri, Apr 11, 2014 at 11:34:36AM +0200, Daniel Vetter wrote: On Thu, Apr 10, 2014 at 05:24:36PM -0700, Matt Roper wrote: ... + /* setplane API takes shifted source rectangle values; unshift them */ + src_x = 16; + src_y = 16; + src_w = 16; + src_h = 16; + + /* + * Current hardware can't reposition the primary plane or scale it + * (although this could change in the future). + */ + drm_rect_intersect(dest, clip); + if (dest.x1 != 0 || dest.y1 != 0 || + dest.x2 != crtc-mode.hdisplay || dest.y2 != crtc-mode.vdisplay) { + DRM_DEBUG_KMS(Primary plane must cover entire CRTC\n); + return -EINVAL; + } + + if (crtc_w != src_w || crtc_h != src_h) { + DRM_DEBUG_KMS(Can't scale primary plane\n); + return -EINVAL; + } Subpixel check seems to be missing. And can't we extract all these checks both here and from the primary plane helper? I guess there'll be other hw which doesn't have scaling primary planes, but which wants to allow primary plane enable/disable. I was a bit unsure about this. At first I thought I needed to check the subpixel part, but the DocBook reference indicates Devices that don't support subpixel plane coordinates can ignore the fractional part. which sounds to me like we're supposed to just silently ignore the subpixel bits on i915 and other devices that don't support it. Which would probably also mean that I should remove the (subpixel bits == 0) test from the primary helper... Hm ... yeah I guess you're right. For now it probably won't matter too much. -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] tests/gem_error_capture: Initial testcase for error state capture/dump
On Fri, Apr 11, 2014 at 01:09:22PM +, Mateo Lozano, Oscar wrote: Daniel, is this what you had in mind? -- Oscar P.S. I just re-read the Jira task and realized that I am missing the check that the ring objects contains a reloc with MI_BB_START for your presumed batch object's address. I´ll add this and resubmit. Yeah this looks pretty cool. btw when you resend can you simply sign up someone local to your team for the review? Bit easier that way. I've checked your usage of igt_* infrastructure and that looks good. Thanks, Daniel -Original Message- From: Mateo Lozano, Oscar Sent: Friday, April 11, 2014 2:00 PM To: intel-gfx@lists.freedesktop.org Cc: Mateo Lozano, Oscar Subject: [PATCH] tests/gem_error_capture: Initial testcase for error state capture/dump From: Oscar Mateo oscar.ma...@intel.com Signed-off-by: Oscar Mateo oscar.ma...@intel.com --- tests/.gitignore | 1 + tests/Makefile.sources| 1 + tests/gem_error_capture.c | 230 ++ 3 files changed, 232 insertions(+) create mode 100644 tests/gem_error_capture.c diff --git a/tests/.gitignore b/tests/.gitignore index 146bab0..945574c 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -27,6 +27,7 @@ gem_ctx_create gem_ctx_exec gem_double_irq_loop gem_dummy_reloc_loop +gem_error_capture gem_evict_alignment gem_evict_everything gem_exec_bad_domains diff --git a/tests/Makefile.sources b/tests/Makefile.sources index bf02a48..612beb6 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -24,6 +24,7 @@ TESTS_progs_M = \ gem_ctx_bad_exec \ gem_ctx_exec \ gem_dummy_reloc_loop \ + gem_error_capture \ gem_evict_alignment \ gem_evict_everything \ gem_exec_bad_domains \ diff --git a/tests/gem_error_capture.c b/tests/gem_error_capture.c new file mode 100644 index 000..bbf0f5d --- /dev/null +++ b/tests/gem_error_capture.c @@ -0,0 +1,230 @@ +/* + * Copyright © 2014 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person +obtaining a + * copy of this software and associated documentation files (the +Software), + * to deal in the Software without restriction, including without +limitation + * the rights to use, copy, modify, merge, publish, distribute, +sublicense, + * and/or sell copies of the Software, and to permit persons to whom +the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the +next + * paragraph) shall be included in all copies or substantial portions +of the + * Software. + * + * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, +EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT +SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR +OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, +ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +DEALINGS + * IN THE SOFTWARE. + * + * Authors: + *Oscar Mateo oscar.ma...@intel.com + * + */ + +/* + * Testcase: Check whether basic error state capture/dump mechanism is +correctly + * working for all rings. + */ + +#include unistd.h +#include stdlib.h +#include stdint.h +#include stdio.h +#include string.h +#include fcntl.h +#include inttypes.h +#include errno.h +#include sys/stat.h +#include sys/ioctl.h +#include drm.h +#include ioctl_wrappers.h +#include drmtest.h +#include intel_io.h +#include igt_debugfs.h + +#define MAGIC_NUMBER 0x10001 +uint32_t batch[4] = {MI_NOOP, MI_BATCH_BUFFER_END, MAGIC_NUMBER, +MAGIC_NUMBER}; + +static void stop_rings(void) +{ + int fd; + static const char buf[] = 0xf; + + fd = igt_debugfs_open(i915_ring_stop, O_WRONLY); + igt_assert(fd = 0); + + igt_assert(write(fd, buf, sizeof(buf)) == sizeof(buf)); + + close(fd); +} + +static bool rings_stopped(void) +{ + int fd; + static char buf[128]; + unsigned long long val; + + fd = igt_debugfs_open(i915_ring_stop, O_RDONLY); + igt_assert(fd = 0); + + igt_assert(read(fd, buf, sizeof(buf)) 0); + close(fd); + + sscanf(buf, %llx, val); + + return (bool)val; +} + +static void clear_error_state(void) +{ + int fd; + static const char buf[] = ; + + fd = igt_debugfs_open(i915_error_state, O_WRONLY); + igt_assert(fd = 0); + + igt_assert(write(fd, buf, sizeof(buf)) == sizeof(buf)); + close(fd); +} + +static void check_bb_contents(char **line, size_t *line_size, + FILE *file, int pos, uint32_t expected_value) { + char
Re: [Intel-gfx] [PATCH] tests/kms_flip_tiling: Fixes
On Fri, Apr 11, 2014 at 04:17:48PM +0200, Daniel Vetter wrote: - Wrap up testcase correctly into the magic code block. - Put local variables out of the longjmp danger zone. Cc: Ander Conselvan de Oliveira ander.conselvan.de.olive...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch Ander, quick one for process: Please submit igt patches to the mailing list so that they don't get lost in bugzilla. And thanks a lot for the testcase. -Daniel --- tests/kms_flip_tiling.c | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/kms_flip_tiling.c b/tests/kms_flip_tiling.c index e70609d52e78..ca20ad96bc35 100644 --- a/tests/kms_flip_tiling.c +++ b/tests/kms_flip_tiling.c @@ -119,11 +119,10 @@ test_flip_changes_tiling(data_t *data, igt_output_t *output) } static data_t data; +igt_output_t *output; igt_main { - igt_output_t *output; - igt_skip_on_simulation(); igt_fixture { @@ -135,10 +134,10 @@ igt_main igt_display_init(data.display, data.drm_fd); } - igt_subtest_f(flip-changes-tiling); - - for_each_connected_output(data.display, output) - test_flip_changes_tiling(data, output); + igt_subtest_f(flip-changes-tiling) { + for_each_connected_output(data.display, output) + test_flip_changes_tiling(data, output); + } igt_fixture { igt_display_fini(data.display); -- 1.8.5.2 -- 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: Allow user modes to exceed DVI 165MHz limit
Pushed to -fixes, thanks for the patch and Daniel's r-b on IRC. BR, Jani. On Thu, 27 Mar 2014, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com In commit commit 6375b768a9850b6154478993e5fb566fa4614a9c Author: Ville Syrjälä ville.syrj...@linux.intel.com Date: Mon Mar 3 11:33:36 2014 +0200 drm/i915: Reject 165MHz modes w/ DVI monitors the driver started to filter out display modes which exceed the single-link DVI 165Mz dotclock limits when the monitor doesn't report itself as being HDMI compliant. The intent was to filter out all EDID derived modes that require dual-link DVI to operate since we don't support dual-link. However the patch went a bit too far and also causes the driver to reject such modes even when specified by the user. Normally we don't check the sink limitations when setting a mode from the user. This allows the user to specify any mode whether the sink reports to support it or not. This can be useful since often the sinks support more modes than they report in the EDID. So relax the checks a bit, and apply the single-link DVI dotclock limit only when filtering the mode list, and ignore the limit when setting a user specified mode. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=72961 Tested-by: Nicholas Vinson nvin...@comcast.net Cc: sta...@vger.kernel.org Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_hdmi.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index ee3181e..ca5d23d 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -841,11 +841,11 @@ static void intel_disable_hdmi(struct intel_encoder *encoder) } } -static int hdmi_portclock_limit(struct intel_hdmi *hdmi) +static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit) { struct drm_device *dev = intel_hdmi_to_dev(hdmi); - if (!hdmi-has_hdmi_sink || IS_G4X(dev)) + if ((respect_dvi_limit !hdmi-has_hdmi_sink) || IS_G4X(dev)) return 165000; else if (IS_HASWELL(dev) || INTEL_INFO(dev)-gen = 8) return 30; @@ -857,7 +857,8 @@ static enum drm_mode_status intel_hdmi_mode_valid(struct drm_connector *connector, struct drm_display_mode *mode) { - if (mode-clock hdmi_portclock_limit(intel_attached_hdmi(connector))) + if (mode-clock hdmi_portclock_limit(intel_attached_hdmi(connector), +true)) return MODE_CLOCK_HIGH; if (mode-clock 2) return MODE_CLOCK_LOW; @@ -875,7 +876,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder, struct drm_device *dev = encoder-base.dev; struct drm_display_mode *adjusted_mode = pipe_config-adjusted_mode; int clock_12bpc = pipe_config-adjusted_mode.crtc_clock * 3 / 2; - int portclock_limit = hdmi_portclock_limit(intel_hdmi); + int portclock_limit = hdmi_portclock_limit(intel_hdmi, false); int desired_bpp; if (intel_hdmi-color_range_auto) { -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes
Ok, I will change the implementation. Regards Shashank -Original Message- From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Friday, April 11, 2014 7:53 PM To: Sharma, Shashank Cc: Daniel Vetter; C, Ramalingam; Wang, Quanxian; intel-gfx Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes On Fri, Apr 11, 2014 at 01:23:43PM +, Sharma, Shashank wrote: Thanks for the comments, Actually, we are not using live_status at all. The check for gen6 is only for EDID caching. So if the HW is = gen6 cache_edid. Else do not cache EDID, so that we will not block any of the old HW, which might not be HPD capable. Oh, I've thought that this is incremental on top of something you already have. Does it sound ok now :) ? No. HPD is _NOT_ I repeat _NOT_ reliably. Not even on gen6+. live status simply reflects the hpd pin, if either doesn't work, then neither does the other one. Nacked-with-prejudice-by: Daniel Vetter daniel.vet...@ffwll.ch Cheers, Daniel Regards Shashank -Original Message- From: daniel.vet...@ffwll.ch [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Friday, April 11, 2014 6:28 PM To: Sharma, Shashank Cc: C, Ramalingam; Wang, Quanxian; intel-gfx Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes Please don't drop the mailing list when discussing patches. And nope, conditioning this on gen6+ won't help at all, since I have a gen6 and gen7 machine here which don't have reliable hdmi live status. Afaik that is _really_ broken across the board and going with the fancy invalidation scheme and delayed work items is the way forward. Cheers, Daniel On Fri, Apr 11, 2014 at 2:51 PM, Sharma, Shashank shashank.sha...@intel.com wrote: Hi Daniel, First of all, sorry for the delay in coming up with the new patch set. I got stuck with some commercial projects :) . There were few review comments what you gave for the previous optimization patch, those were: 1. This might break the old HWs. Few of them might not even be HPD capable, so its required to handle them. 2. Do not rely on the live_status check, as that HW is not yet proven. And you recommended to have a WQ, which will invalidate the cached HDMI EDID after a timeout. I have written another patch, which is addressing both of the above comments, but doesn't use a WQ. Before sending this to ML, I wanted to have an opinion from you, if this qualifies the design you were expecting. I will be really glad, if you can spare some time, and just have a top level view on the patch, and give your opinion. If you think that this is still not the way, and making a WQ is a better approach, I would gladly create a patch which applies that. Or if you will prefer to have this reviewed in the mailing list only, please accept my apologies for direct contact, and just let me know so. I will publish this in ML. The patch is (It's also attached): From e7ec4e4827615c0ff3a4ddb020df35fdf1b0b82f Mon Sep 17 00:00:00 2001 From: Shashank Sharma shashank.sha...@intel.com Date: Fri, 11 Apr 2014 18:02:50 +0530 Subject: [PATCH] drm/i915: Cache HDMI EDID for future reference This patch contains following changes: 1.Cache HDMI EDID and optimize detect() calls, which are frequently called from userspace, causing un-necessary EDID reads. 2.This cached EDID will be used for detection of the status of HDMI connector, for Non HPD detect() calls. HPD calls will update cached EDID. 3.This optimization is kept for new HW's (Gen6 and +), so that It will not break old HWs who may not be even HPD capable. Signed-off-by: Shashank Sharma shashank.sha...@intel.com --- drivers/gpu/drm/i915/intel_drv.h |1 + drivers/gpu/drm/i915/intel_hdmi.c | 28 ++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 42762b7..b7911df 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -478,6 +478,7 @@ struct intel_hdmi { const void *frame, ssize_t len); void (*set_infoframes)(struct drm_encoder *encoder, struct drm_display_mode *adjusted_mode); + struct edid *edid; }; #define DP_MAX_DOWNSTREAM_PORTS0x10 diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index b0413e1..33550ca 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -938,7 +938,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) hdmi_to_dig_port(intel_hdmi); struct intel_encoder *intel_encoder = intel_dig_port-base; struct drm_i915_private
[Intel-gfx] [PATCH v2] tests/gem_error_capture: Initial testcase for error state capture/dump
From: Oscar Mateo oscar.ma...@intel.com Guarantees that error capture works at a very basic level. v2: Also check that the ring object contains a reloc with MI_BB_START for the presumed batch object's address. Signed-off-by: Oscar Mateo oscar.ma...@intel.com --- tests/.gitignore | 1 + tests/Makefile.sources| 1 + tests/gem_error_capture.c | 269 ++ 3 files changed, 271 insertions(+) create mode 100644 tests/gem_error_capture.c diff --git a/tests/.gitignore b/tests/.gitignore index 146bab0..945574c 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -27,6 +27,7 @@ gem_ctx_create gem_ctx_exec gem_double_irq_loop gem_dummy_reloc_loop +gem_error_capture gem_evict_alignment gem_evict_everything gem_exec_bad_domains diff --git a/tests/Makefile.sources b/tests/Makefile.sources index bf02a48..612beb6 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -24,6 +24,7 @@ TESTS_progs_M = \ gem_ctx_bad_exec \ gem_ctx_exec \ gem_dummy_reloc_loop \ + gem_error_capture \ gem_evict_alignment \ gem_evict_everything \ gem_exec_bad_domains \ diff --git a/tests/gem_error_capture.c b/tests/gem_error_capture.c new file mode 100644 index 000..88845c9 --- /dev/null +++ b/tests/gem_error_capture.c @@ -0,0 +1,269 @@ +/* + * Copyright © 2014 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the Software), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + * Authors: + *Oscar Mateo oscar.ma...@intel.com + * + */ + +/* + * Testcase: Check whether basic error state capture/dump mechanism is correctly + * working for all rings. + */ + +#include unistd.h +#include stdlib.h +#include stdint.h +#include stdio.h +#include string.h +#include fcntl.h +#include inttypes.h +#include errno.h +#include sys/stat.h +#include sys/ioctl.h +#include drm.h +#include ioctl_wrappers.h +#include drmtest.h +#include intel_io.h +#include igt_debugfs.h + +#define MAGIC_NUMBER 0x10001 +uint32_t batch[4] = {MI_NOOP, MI_BATCH_BUFFER_END, MAGIC_NUMBER, MAGIC_NUMBER}; + +static void stop_rings(void) +{ + int fd; + static const char buf[] = 0xf; + + fd = igt_debugfs_open(i915_ring_stop, O_WRONLY); + igt_assert(fd = 0); + + igt_assert(write(fd, buf, sizeof(buf)) == sizeof(buf)); + + close(fd); +} + +static bool rings_stopped(void) +{ + int fd; + static char buf[128]; + unsigned long long val; + + fd = igt_debugfs_open(i915_ring_stop, O_RDONLY); + igt_assert(fd = 0); + + igt_assert(read(fd, buf, sizeof(buf)) 0); + close(fd); + + sscanf(buf, %llx, val); + + return (bool)val; +} + +static void clear_error_state(void) +{ + int fd; + static const char buf[] = ; + + fd = igt_debugfs_open(i915_error_state, O_WRONLY); + igt_assert(fd = 0); + + igt_assert(write(fd, buf, sizeof(buf)) == sizeof(buf)); + close(fd); +} + +static void check_error_state(const char *expected_ring_name, + uint64_t expected_offset) +{ + FILE *file; + int debug_fd; + char *line = NULL; + size_t line_size = 0; + char *dashes = NULL; + char *ring_name = NULL; + int bb_matched = 0; + uint32_t gtt_offset; + char expected_line[32]; + int req_matched = 0; + int requests; + uint32_t seqno, tail; + long jiffies; + int ringbuf_matched = 0; + unsigned int offset, command, expected_addr = 0; + int i, items; + bool bb_ok = false, req_ok = false, ringbuf_ok = false; + + debug_fd = igt_debugfs_open(i915_error_state, O_RDONLY); + igt_assert(debug_fd = 0); + file = fdopen(debug_fd, r); + + while (getline(line, line_size, file) 0) { + dashes = strstr(line, ---); + if (!dashes) + continue; + + ring_name =
[Intel-gfx] [PATCH] drm/i915/vlv: assert and de-assert sideband reset on resume
This is a bit like the CMN reset de-assert we do in DPIO_CTL, except that it resets the whole common lane section of the PHY. This is required on machines where the BIOS doesn't do this for us on resume to properly re-calibrate and get the PHY ready to transmit data. Without this patch, such machines won't resume correctly much of the time, with the symptom being a 'port ready' timeout and/or a link training failure. I'm open to better suggestions on how to do the power well toggle, with the existing code it looks like I'd have to walk through a bunch of power domains looking for a match, then call a generic function which will warn. I'd prefer to just expose the specific domains directly for low level platform code like this. Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org --- drivers/gpu/drm/i915/intel_pm.c |4 ++-- drivers/gpu/drm/i915/intel_uncore.c | 19 +++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fa00185..3afd0bc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv, return true; } -static void vlv_set_power_well(struct drm_i915_private *dev_priv, - struct i915_power_well *power_well, bool enable) +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable) { enum punit_power_well power_well_id = power_well-data; u32 mask; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 2a72bab..f1abd2d 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags); } +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable); + void intel_uncore_early_sanitize(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev) DRM_INFO(Found %zuMB of eLLC\n, dev_priv-ellc_size); } + /* +* From VLV2A0_DP_eDP_HDMI_DPIO_driver_vbios_notes_11.docx: +* Need to assert and de-assert PHY SB reset by gating the common +* lane power, then un-gating it. +* Simply ungating isn't enough to reset the PHY enough to get +* ports and lanes running. +*/ + if (IS_VALLEYVIEW(dev)) { + struct i915_power_well cmn_well = { + .data = PUNIT_POWER_WELL_DPIO_CMN_BC + }; + + vlv_set_power_well(dev_priv, cmn_well, false); + vlv_set_power_well(dev_priv, cmn_well, true); + } + /* clear out old GT FIFO errors */ if (IS_GEN6(dev) || IS_GEN7(dev)) __raw_i915_write32(dev_priv, GTFIFODBG, -- 1.7.9.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/SDVO: For sysfs link put directory and target in correct order
When linking the i2c sysfs file into the connector's directory pass directory and link target in the right order. This code was introduced with: commit 931c1c26983b4f84e33b78579fc8d57e4a14c6b4 Author: Imre Deak imre.d...@intel.com Date: Tue Feb 11 17:12:51 2014 +0200 drm/i915: sdvo: add i2c sysfs symlink to the connector's directory This is the same what we do for DP connectors, so make things more consistent. Signed-off-by: Egbert Eich e...@suse.de --- drivers/gpu/drm/i915/intel_sdvo.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 5043f16..2d4d461 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -2428,8 +2428,8 @@ intel_sdvo_connector_init(struct intel_sdvo_connector *connector, if (ret 0) goto err1; - ret = sysfs_create_link(encoder-ddc.dev.kobj, - drm_connector-kdev-kobj, + ret = sysfs_create_link(drm_connector-kdev-kobj, + encoder-ddc.dev.kobj, encoder-ddc.dev.kobj.name); if (ret 0) goto err2; -- 1.8.4.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] tests/gem_error_capture: Initial testcase for error state capture/dump
On Fri, Apr 11, 2014 at 05:48:12PM +0100, oscar.ma...@intel.com wrote: +static void check_error_state(const char *expected_ring_name, + uint64_t expected_offset) +{ + FILE *file; + int debug_fd; + char *line = NULL; + size_t line_size = 0; + char *dashes = NULL; + char *ring_name = NULL; + int bb_matched = 0; + uint32_t gtt_offset; + char expected_line[32]; + int req_matched = 0; + int requests; + uint32_t seqno, tail; + long jiffies; + int ringbuf_matched = 0; + unsigned int offset, command, expected_addr = 0; + int i, items; + bool bb_ok = false, req_ok = false, ringbuf_ok = false; Most of these are only of use in local scope. + debug_fd = igt_debugfs_open(i915_error_state, O_RDONLY); + igt_assert(debug_fd = 0); + file = fdopen(debug_fd, r); + + while (getline(line, line_size, file) 0) { + dashes = strstr(line, ---); + if (!dashes) + continue; + + ring_name = realloc(ring_name, dashes - line); + strncpy(ring_name, line, dashes - line); + ring_name[dashes - line - 1] = '\0'; + + bb_matched = sscanf(dashes, --- gtt_offset = 0x%08x\n, + gtt_offset); + if (bb_matched == 1) { + igt_assert(strstr(ring_name, expected_ring_name)); + igt_assert(gtt_offset == expected_offset); + + for (i = 0; i sizeof(batch) / 4; i++) { + igt_assert(getline(line, line_size, file) 0); + snprintf(expected_line, sizeof(expected_line), %08x : %08x, + 4*i, batch[i]); + igt_assert(strstr(line, expected_line)); + } + bb_ok = true; + continue; + } + + req_matched = sscanf(dashes, --- %d requests\n, + requests); + if (req_matched == 1) { + igt_assert(strstr(ring_name, expected_ring_name)); + igt_assert(requests == 1); Bad assumption. You could still have the request from gem_quiescent_gpu() which may not have been retired before the error triggers. + + igt_assert(getline(line, line_size, file) 0); + items = sscanf(line, seqno 0x%08x, emitted %ld, tail 0x%08x\n, + seqno, jiffies, tail); + igt_assert(items == 3); Bad. I may change the format. s/may/will/ + req_ok = true; + continue; + } + + ringbuf_matched = sscanf(dashes, --- ringbuffer = 0x%08x\n, + gtt_offset); + if (ringbuf_matched == 1) { + if (!strstr(ring_name, expected_ring_name)) + continue; + igt_assert(req_ok); + + for (i = 0; i tail / 4; i++) { + igt_assert(getline(line, line_size, file) 0); + items = sscanf(line, %08x : %08x\n, + offset, command); + igt_assert(items == 2); + if ((command 0x1F80) == MI_BATCH_BUFFER_START) { + igt_assert(getline(line, line_size, file) 0); + items = sscanf(line, %08x : %08x\n, + offset, expected_addr); + igt_assert(items == 2); + i++; + } + } + igt_assert(expected_addr == expected_offset); Bad. Some gen encode flags into the BB start address. + ringbuf_ok = true; + continue; + } + + if (bb_ok req_ok ringbuf_ok) + break; + } + igt_assert(bb_ok req_ok ringbuf_ok); + + free(line); + free(ring_name); + close(debug_fd); +} -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/vlv: assert and de-assert sideband reset on resume
On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote: This is a bit like the CMN reset de-assert we do in DPIO_CTL, except that it resets the whole common lane section of the PHY. This is required on machines where the BIOS doesn't do this for us on resume to properly re-calibrate and get the PHY ready to transmit data. Without this patch, such machines won't resume correctly much of the time, with the symptom being a 'port ready' timeout and/or a link training failure. I'm open to better suggestions on how to do the power well toggle, with the existing code it looks like I'd have to walk through a bunch of power domains looking for a match, then call a generic function which will warn. I'd prefer to just expose the specific domains directly for low level platform code like this. Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org --- drivers/gpu/drm/i915/intel_pm.c |4 ++-- drivers/gpu/drm/i915/intel_uncore.c | 19 +++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fa00185..3afd0bc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv, return true; } -static void vlv_set_power_well(struct drm_i915_private *dev_priv, -struct i915_power_well *power_well, bool enable) +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable) { enum punit_power_well power_well_id = power_well-data; u32 mask; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 2a72bab..f1abd2d 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags); } +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable); + void intel_uncore_early_sanitize(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev) DRM_INFO(Found %zuMB of eLLC\n, dev_priv-ellc_size); } + /* + * From VLV2A0_DP_eDP_HDMI_DPIO_driver_vbios_notes_11.docx: + * Need to assert and de-assert PHY SB reset by gating the common + * lane power, then un-gating it. + * Simply ungating isn't enough to reset the PHY enough to get + * ports and lanes running. + */ + if (IS_VALLEYVIEW(dev)) { + struct i915_power_well cmn_well = { + .data = PUNIT_POWER_WELL_DPIO_CMN_BC + }; + + vlv_set_power_well(dev_priv, cmn_well, false); + vlv_set_power_well(dev_priv, cmn_well, true); + } Relationship with intel_reset_dpio? Should we move this bit of code over there? I'm lost in this maze of kick-me-harder patches for byt dpio ... -Daniel + /* clear out old GT FIFO errors */ if (IS_GEN6(dev) || IS_GEN7(dev)) __raw_i915_write32(dev_priv, GTFIFODBG, -- 1.7.9.5 ___ 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/vlv: assert and de-assert sideband reset on resume
On Fri, 2014-04-11 at 10:00 -0700, Jesse Barnes wrote: This is a bit like the CMN reset de-assert we do in DPIO_CTL, except that it resets the whole common lane section of the PHY. This is required on machines where the BIOS doesn't do this for us on resume to properly re-calibrate and get the PHY ready to transmit data. Without this patch, such machines won't resume correctly much of the time, with the symptom being a 'port ready' timeout and/or a link training failure. I'm open to better suggestions on how to do the power well toggle, with the existing code it looks like I'd have to walk through a bunch of power domains looking for a match, then call a generic function which will warn. I'd prefer to just expose the specific domains directly for low level platform code like this. The power_well-sync_hw() handler looks like a good place for such things. It will get called from intel_power_domains_init_hw(), which is later than then the uncore sanitize functions, but then again if it's really needed that early then intel_power_domains_init_hw() should be moved earlier too.. --Imre Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org --- drivers/gpu/drm/i915/intel_pm.c |4 ++-- drivers/gpu/drm/i915/intel_uncore.c | 19 +++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fa00185..3afd0bc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv, return true; } -static void vlv_set_power_well(struct drm_i915_private *dev_priv, -struct i915_power_well *power_well, bool enable) +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable) { enum punit_power_well power_well_id = power_well-data; u32 mask; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 2a72bab..f1abd2d 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags); } +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable); + void intel_uncore_early_sanitize(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev) DRM_INFO(Found %zuMB of eLLC\n, dev_priv-ellc_size); } + /* + * From VLV2A0_DP_eDP_HDMI_DPIO_driver_vbios_notes_11.docx: + * Need to assert and de-assert PHY SB reset by gating the common + * lane power, then un-gating it. + * Simply ungating isn't enough to reset the PHY enough to get + * ports and lanes running. + */ + if (IS_VALLEYVIEW(dev)) { + struct i915_power_well cmn_well = { + .data = PUNIT_POWER_WELL_DPIO_CMN_BC + }; + + vlv_set_power_well(dev_priv, cmn_well, false); + vlv_set_power_well(dev_priv, cmn_well, true); + } + /* clear out old GT FIFO errors */ if (IS_GEN6(dev) || IS_GEN7(dev)) __raw_i915_write32(dev_priv, GTFIFODBG, signature.asc Description: This is a digitally signed message part ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/SDVO: For sysfs link put directory and target in correct order
On Fri, 2014-04-11 at 19:07 +0200, Egbert Eich wrote: When linking the i2c sysfs file into the connector's directory pass directory and link target in the right order. This code was introduced with: commit 931c1c26983b4f84e33b78579fc8d57e4a14c6b4 Author: Imre Deak imre.d...@intel.com Date: Tue Feb 11 17:12:51 2014 +0200 drm/i915: sdvo: add i2c sysfs symlink to the connector's directory This is the same what we do for DP connectors, so make things more consistent. Signed-off-by: Egbert Eich e...@suse.de Oops, good catch. The fix looks ok, fwiw: Reviewed-by: Imre Deak imre.d...@intel.com --- drivers/gpu/drm/i915/intel_sdvo.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 5043f16..2d4d461 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -2428,8 +2428,8 @@ intel_sdvo_connector_init(struct intel_sdvo_connector *connector, if (ret 0) goto err1; - ret = sysfs_create_link(encoder-ddc.dev.kobj, - drm_connector-kdev-kobj, + ret = sysfs_create_link(drm_connector-kdev-kobj, + encoder-ddc.dev.kobj, encoder-ddc.dev.kobj.name); if (ret 0) goto err2; signature.asc Description: This is a digitally signed message part ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/vlv: assert and de-assert sideband reset on resume
On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote: This is a bit like the CMN reset de-assert we do in DPIO_CTL, except that it resets the whole common lane section of the PHY. This is required on machines where the BIOS doesn't do this for us on resume to properly re-calibrate and get the PHY ready to transmit data. Without this patch, such machines won't resume correctly much of the time, with the symptom being a 'port ready' timeout and/or a link training failure. I'm open to better suggestions on how to do the power well toggle, with the existing code it looks like I'd have to walk through a bunch of power domains looking for a match, then call a generic function which will warn. I'd prefer to just expose the specific domains directly for low level platform code like this. Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org --- drivers/gpu/drm/i915/intel_pm.c |4 ++-- drivers/gpu/drm/i915/intel_uncore.c | 19 +++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fa00185..3afd0bc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv, return true; } -static void vlv_set_power_well(struct drm_i915_private *dev_priv, -struct i915_power_well *power_well, bool enable) +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable) { enum punit_power_well power_well_id = power_well-data; u32 mask; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 2a72bab..f1abd2d 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags); } +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable); + void intel_uncore_early_sanitize(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev) DRM_INFO(Found %zuMB of eLLC\n, dev_priv-ellc_size); } + /* + * From VLV2A0_DP_eDP_HDMI_DPIO_driver_vbios_notes_11.docx: + * Need to assert and de-assert PHY SB reset by gating the common + * lane power, then un-gating it. + * Simply ungating isn't enough to reset the PHY enough to get + * ports and lanes running. + */ + if (IS_VALLEYVIEW(dev)) { + struct i915_power_well cmn_well = { + .data = PUNIT_POWER_WELL_DPIO_CMN_BC + }; + + vlv_set_power_well(dev_priv, cmn_well, false); + vlv_set_power_well(dev_priv, cmn_well, true); + } Stick this into intel_reset_dpio() instead? And what about fastboot and whatnot? Should we check if the display is already up and running somehow before we go and kill it with this? + /* clear out old GT FIFO errors */ if (IS_GEN6(dev) || IS_GEN7(dev)) __raw_i915_write32(dev_priv, GTFIFODBG, -- 1.7.9.5 ___ 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] drm/i915/vlv: assert and de-assert sideband reset on resume
On Fri, 11 Apr 2014 19:16:32 +0200 Daniel Vetter dan...@ffwll.ch wrote: On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote: This is a bit like the CMN reset de-assert we do in DPIO_CTL, except that it resets the whole common lane section of the PHY. This is required on machines where the BIOS doesn't do this for us on resume to properly re-calibrate and get the PHY ready to transmit data. Without this patch, such machines won't resume correctly much of the time, with the symptom being a 'port ready' timeout and/or a link training failure. I'm open to better suggestions on how to do the power well toggle, with the existing code it looks like I'd have to walk through a bunch of power domains looking for a match, then call a generic function which will warn. I'd prefer to just expose the specific domains directly for low level platform code like this. Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org --- drivers/gpu/drm/i915/intel_pm.c |4 ++-- drivers/gpu/drm/i915/intel_uncore.c | 19 +++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fa00185..3afd0bc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv, return true; } -static void vlv_set_power_well(struct drm_i915_private *dev_priv, - struct i915_power_well *power_well, bool enable) +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable) { enum punit_power_well power_well_id = power_well-data; u32 mask; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 2a72bab..f1abd2d 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags); } +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable); + void intel_uncore_early_sanitize(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev) DRM_INFO(Found %zuMB of eLLC\n, dev_priv-ellc_size); } + /* +* From VLV2A0_DP_eDP_HDMI_DPIO_driver_vbios_notes_11.docx: +* Need to assert and de-assert PHY SB reset by gating the common +* lane power, then un-gating it. +* Simply ungating isn't enough to reset the PHY enough to get +* ports and lanes running. +*/ + if (IS_VALLEYVIEW(dev)) { + struct i915_power_well cmn_well = { + .data = PUNIT_POWER_WELL_DPIO_CMN_BC + }; + + vlv_set_power_well(dev_priv, cmn_well, false); + vlv_set_power_well(dev_priv, cmn_well, true); + } Relationship with intel_reset_dpio? Should we move this bit of code over there? I'm lost in this maze of kick-me-harder patches for byt dpio ... That happens too late. This will clobber register state, whereas the DPIO reset just resets the interface between the phy and the display. -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/vlv: assert and de-assert sideband reset on resume
On Fri, 11 Apr 2014 20:26:24 +0300 Ville Syrjälä ville.syrj...@linux.intel.com wrote: On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote: This is a bit like the CMN reset de-assert we do in DPIO_CTL, except that it resets the whole common lane section of the PHY. This is required on machines where the BIOS doesn't do this for us on resume to properly re-calibrate and get the PHY ready to transmit data. Without this patch, such machines won't resume correctly much of the time, with the symptom being a 'port ready' timeout and/or a link training failure. I'm open to better suggestions on how to do the power well toggle, with the existing code it looks like I'd have to walk through a bunch of power domains looking for a match, then call a generic function which will warn. I'd prefer to just expose the specific domains directly for low level platform code like this. Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org --- drivers/gpu/drm/i915/intel_pm.c |4 ++-- drivers/gpu/drm/i915/intel_uncore.c | 19 +++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fa00185..3afd0bc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv, return true; } -static void vlv_set_power_well(struct drm_i915_private *dev_priv, - struct i915_power_well *power_well, bool enable) +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable) { enum punit_power_well power_well_id = power_well-data; u32 mask; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 2a72bab..f1abd2d 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags); } +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable); + void intel_uncore_early_sanitize(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev) DRM_INFO(Found %zuMB of eLLC\n, dev_priv-ellc_size); } + /* +* From VLV2A0_DP_eDP_HDMI_DPIO_driver_vbios_notes_11.docx: +* Need to assert and de-assert PHY SB reset by gating the common +* lane power, then un-gating it. +* Simply ungating isn't enough to reset the PHY enough to get +* ports and lanes running. +*/ + if (IS_VALLEYVIEW(dev)) { + struct i915_power_well cmn_well = { + .data = PUNIT_POWER_WELL_DPIO_CMN_BC + }; + + vlv_set_power_well(dev_priv, cmn_well, false); + vlv_set_power_well(dev_priv, cmn_well, true); + } Stick this into intel_reset_dpio() instead? And what about fastboot and whatnot? Should we check if the display is already up and running somehow before we go and kill it with this? reset_dpio is too late. But yes, this generally doesn't need to happen on the boot path (well maybe on some platforms) so we should do something conditional there or we'll lose all the display state. -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Intel-specific primary plane handling (v2)
On Fri, Apr 11, 2014 at 11:34:36AM +0200, Daniel Vetter wrote: On Thu, Apr 10, 2014 at 05:24:36PM -0700, Matt Roper wrote: ... Hm, I've thought we could do a simple if (intel_crtc-primary_enabled) call_primary_plane_helper else enable_the_hw_plane But we need to do all the arg checking for the !primary_enabled case :( Anyway more code sharing make me happier. Cheers, Daniel I think the problem here is that the helper has a bunch of tests targetted at the lowest common denominator hardware. Some of the things it rejects are things that our hardware may begin to allow at some point in the future (e.g., primary plane scaling, partial CRTC coverage of primary plane, etc.). We can probably call into the helper today and get the behavior we want, but I'd expect that some of those restrictions will need to be relaxed in the future and then we'll have to switch the code back at that point. Given that we still need to do all this checking in the 'if (!enabled)' case, I don't think it's worth trying to call through the helper for the 'if (enabled)' case (especially since the actual work here after we're done testing is just a couple lines of code)? Matt -- 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] drm/i915/vlv: assert and de-assert sideband reset on resume
On Fri, Apr 11, 2014 at 10:34:19AM -0700, Jesse Barnes wrote: On Fri, 11 Apr 2014 19:16:32 +0200 Daniel Vetter dan...@ffwll.ch wrote: On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote: This is a bit like the CMN reset de-assert we do in DPIO_CTL, except that it resets the whole common lane section of the PHY. This is required on machines where the BIOS doesn't do this for us on resume to properly re-calibrate and get the PHY ready to transmit data. Without this patch, such machines won't resume correctly much of the time, with the symptom being a 'port ready' timeout and/or a link training failure. I'm open to better suggestions on how to do the power well toggle, with the existing code it looks like I'd have to walk through a bunch of power domains looking for a match, then call a generic function which will warn. I'd prefer to just expose the specific domains directly for low level platform code like this. Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org --- drivers/gpu/drm/i915/intel_pm.c |4 ++-- drivers/gpu/drm/i915/intel_uncore.c | 19 +++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fa00185..3afd0bc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv, return true; } -static void vlv_set_power_well(struct drm_i915_private *dev_priv, -struct i915_power_well *power_well, bool enable) +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable) { enum punit_power_well power_well_id = power_well-data; u32 mask; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 2a72bab..f1abd2d 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags); } +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable); + void intel_uncore_early_sanitize(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev) DRM_INFO(Found %zuMB of eLLC\n, dev_priv-ellc_size); } + /* + * From VLV2A0_DP_eDP_HDMI_DPIO_driver_vbios_notes_11.docx: + * Need to assert and de-assert PHY SB reset by gating the common + * lane power, then un-gating it. + * Simply ungating isn't enough to reset the PHY enough to get + * ports and lanes running. + */ + if (IS_VALLEYVIEW(dev)) { + struct i915_power_well cmn_well = { + .data = PUNIT_POWER_WELL_DPIO_CMN_BC + }; + + vlv_set_power_well(dev_priv, cmn_well, false); + vlv_set_power_well(dev_priv, cmn_well, true); + } Relationship with intel_reset_dpio? Should we move this bit of code over there? I'm lost in this maze of kick-me-harder patches for byt dpio ... That happens too late. This will clobber register state, whereas the DPIO reset just resets the interface between the phy and the display. As a clarification to the cmnreset thing, we never actually assert that signal, we just deassert it. The idea being that it should be asserted by default when things get powered on. But I wonder if we should assert it before suspending anyway. Oh and I think if we power gate the cmnlane we would need to assert/deassert cmnreset around it. In some CHV doc I see a note that side reset must be deasserted before cmnreset. The timing diagrams in VLV docs seem to have that order as well. So unless there's some internal logic which hold cmnreset asserted for the required time, we should do it by hand. Oh and there's another intersting looking note: NOTE1 : Common lane reset must not be de-asserted until REFCLK to PLL is enabled by i_pll*refclkbufen and the clock is running and stable I guess we managed to follow that by accident since we enable the refclock for DPLLB for the hotplug workaround. But perhaps we should enable the refclk for all PLLs just to be sure. -- 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] drm/i915/vlv: assert and de-assert sideband reset on resume
On Fri, Apr 11, 2014 at 10:35:40AM -0700, Jesse Barnes wrote: On Fri, 11 Apr 2014 20:26:24 +0300 Ville Syrjälä ville.syrj...@linux.intel.com wrote: On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote: This is a bit like the CMN reset de-assert we do in DPIO_CTL, except that it resets the whole common lane section of the PHY. This is required on machines where the BIOS doesn't do this for us on resume to properly re-calibrate and get the PHY ready to transmit data. Without this patch, such machines won't resume correctly much of the time, with the symptom being a 'port ready' timeout and/or a link training failure. I'm open to better suggestions on how to do the power well toggle, with the existing code it looks like I'd have to walk through a bunch of power domains looking for a match, then call a generic function which will warn. I'd prefer to just expose the specific domains directly for low level platform code like this. Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org --- drivers/gpu/drm/i915/intel_pm.c |4 ++-- drivers/gpu/drm/i915/intel_uncore.c | 19 +++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fa00185..3afd0bc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv, return true; } -static void vlv_set_power_well(struct drm_i915_private *dev_priv, -struct i915_power_well *power_well, bool enable) +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable) { enum punit_power_well power_well_id = power_well-data; u32 mask; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 2a72bab..f1abd2d 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags); } +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable); + void intel_uncore_early_sanitize(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev) DRM_INFO(Found %zuMB of eLLC\n, dev_priv-ellc_size); } + /* + * From VLV2A0_DP_eDP_HDMI_DPIO_driver_vbios_notes_11.docx: + * Need to assert and de-assert PHY SB reset by gating the common + * lane power, then un-gating it. + * Simply ungating isn't enough to reset the PHY enough to get + * ports and lanes running. + */ + if (IS_VALLEYVIEW(dev)) { + struct i915_power_well cmn_well = { + .data = PUNIT_POWER_WELL_DPIO_CMN_BC + }; + + vlv_set_power_well(dev_priv, cmn_well, false); + vlv_set_power_well(dev_priv, cmn_well, true); + } Stick this into intel_reset_dpio() instead? And what about fastboot and whatnot? Should we check if the display is already up and running somehow before we go and kill it with this? reset_dpio is too late. How come? We shouldn't touch the PHY before it. So either reset_dpio is in the wrong place for some reason, or something else gets called too soon. -- 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] drm/i915/vlv: assert and de-assert sideband reset on resume
On Fri, 11 Apr 2014 21:10:21 +0300 Ville Syrjälä ville.syrj...@linux.intel.com wrote: On Fri, Apr 11, 2014 at 10:35:40AM -0700, Jesse Barnes wrote: On Fri, 11 Apr 2014 20:26:24 +0300 Ville Syrjälä ville.syrj...@linux.intel.com wrote: On Fri, Apr 11, 2014 at 10:00:16AM -0700, Jesse Barnes wrote: This is a bit like the CMN reset de-assert we do in DPIO_CTL, except that it resets the whole common lane section of the PHY. This is required on machines where the BIOS doesn't do this for us on resume to properly re-calibrate and get the PHY ready to transmit data. Without this patch, such machines won't resume correctly much of the time, with the symptom being a 'port ready' timeout and/or a link training failure. I'm open to better suggestions on how to do the power well toggle, with the existing code it looks like I'd have to walk through a bunch of power domains looking for a match, then call a generic function which will warn. I'd prefer to just expose the specific domains directly for low level platform code like this. Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org --- drivers/gpu/drm/i915/intel_pm.c |4 ++-- drivers/gpu/drm/i915/intel_uncore.c | 19 +++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fa00185..3afd0bc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5454,8 +5454,8 @@ static bool i9xx_always_on_power_well_enabled(struct drm_i915_private *dev_priv, return true; } -static void vlv_set_power_well(struct drm_i915_private *dev_priv, - struct i915_power_well *power_well, bool enable) +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable) { enum punit_power_well power_well_id = power_well-data; u32 mask; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 2a72bab..f1abd2d 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -363,6 +363,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags); } +void vlv_set_power_well(struct drm_i915_private *dev_priv, + struct i915_power_well *power_well, bool enable); + void intel_uncore_early_sanitize(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; @@ -381,6 +384,22 @@ void intel_uncore_early_sanitize(struct drm_device *dev) DRM_INFO(Found %zuMB of eLLC\n, dev_priv-ellc_size); } + /* +* From VLV2A0_DP_eDP_HDMI_DPIO_driver_vbios_notes_11.docx: +* Need to assert and de-assert PHY SB reset by gating the common +* lane power, then un-gating it. +* Simply ungating isn't enough to reset the PHY enough to get +* ports and lanes running. +*/ + if (IS_VALLEYVIEW(dev)) { + struct i915_power_well cmn_well = { + .data = PUNIT_POWER_WELL_DPIO_CMN_BC + }; + + vlv_set_power_well(dev_priv, cmn_well, false); + vlv_set_power_well(dev_priv, cmn_well, true); + } Stick this into intel_reset_dpio() instead? And what about fastboot and whatnot? Should we check if the display is already up and running somehow before we go and kill it with this? reset_dpio is too late. How come? We shouldn't touch the PHY before it. So either reset_dpio is in the wrong place for some reason, or something else gets called too soon. Oh actually I haven't tested with just the common reset, it may be ok to put that into the DPIO init function. My earlier patch was toggling all the wells, including display, which would obviously clobber things. -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Intel-specific primary plane handling (v2)
On Fri, Apr 11, 2014 at 10:41:56AM -0700, Matt Roper wrote: On Fri, Apr 11, 2014 at 11:34:36AM +0200, Daniel Vetter wrote: On Thu, Apr 10, 2014 at 05:24:36PM -0700, Matt Roper wrote: ... Hm, I've thought we could do a simple if (intel_crtc-primary_enabled) call_primary_plane_helper else enable_the_hw_plane But we need to do all the arg checking for the !primary_enabled case :( Anyway more code sharing make me happier. Cheers, Daniel I think the problem here is that the helper has a bunch of tests targetted at the lowest common denominator hardware. Some of the things it rejects are things that our hardware may begin to allow at some point in the future (e.g., primary plane scaling, partial CRTC coverage of primary plane, etc.). We can probably call into the helper today and get the behavior we want, but I'd expect that some of those restrictions will need to be relaxed in the future and then we'll have to switch the code back at that point. Given that we still need to do all this checking in the 'if (!enabled)' case, I don't think it's worth trying to call through the helper for the 'if (enabled)' case (especially since the actual work here after we're done testing is just a couple lines of code)? Well for that future I simply expect that we'll get a completely new update_plane function. I agree that reusing the helper completely doesn't work really, but sharing the tests would be nice imo. -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
[Intel-gfx] [PATCH] drm/i915: Intel-specific primary plane handling (v3)
Intel hardware allows the primary plane to be disabled independently of the CRTC. Provide custom primary plane handling to allow this. v3: - Provide gen-specific primary plane format lists (suggested by Daniel Vetter). - If the primary plane is already enabled, go ahead and just call the primary plane helper to do the update (suggested by Daniel Vetter). - Don't try to disable the primary plane on destruction; the DRM layer should have already taken care of this for us. v2: - Unpin fb properly on primary plane disable - Provide an Intel-specific set of primary plane formats - Additional sanity checks on setplane (in line with the checks currently being done by the DRM core primary plane helper) Signed-off-by: Matt Roper matthew.d.ro...@intel.com --- drivers/gpu/drm/i915/intel_display.c | 197 ++- drivers/gpu/drm/i915/intel_drv.h | 1 + 2 files changed, 196 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1390ab5..3e4d36a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -39,8 +39,35 @@ #include i915_trace.h #include drm/drm_dp_helper.h #include drm/drm_crtc_helper.h +#include drm/drm_plane_helper.h +#include drm/drm_rect.h #include linux/dma_remapping.h +/* Primary plane formats supported by all gen */ +#define COMMON_PRIMARY_FORMATS \ + DRM_FORMAT_C8, \ + DRM_FORMAT_RGB565, \ + DRM_FORMAT_XRGB, \ + DRM_FORMAT_ARGB + +/* Primary plane formats for gen = 3 */ +const static uint32_t intel_primary_formats_gen3[] = { + COMMON_PRIMARY_FORMATS, + DRM_FORMAT_XRGB1555, + DRM_FORMAT_ARGB1555, +}; + +/* Primary plane formats for gen = 4 */ +const static uint32_t intel_primary_formats_gen4[] = { + COMMON_PRIMARY_FORMATS, \ + DRM_FORMAT_XBGR, + DRM_FORMAT_ABGR, + DRM_FORMAT_XRGB2101010, + DRM_FORMAT_ARGB2101010, + DRM_FORMAT_XBGR2101010, + DRM_FORMAT_ABGR2101010, +}; + static void intel_increase_pllclock(struct drm_crtc *crtc); static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on); @@ -10556,17 +10583,183 @@ static void intel_shared_dpll_init(struct drm_device *dev) BUG_ON(dev_priv-num_shared_dpll I915_NUM_PLLS); } +static int +intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc, +struct drm_framebuffer *fb, int crtc_x, int crtc_y, +unsigned int crtc_w, unsigned int crtc_h, +uint32_t src_x, uint32_t src_y, +uint32_t src_w, uint32_t src_h) +{ + struct drm_device *dev = crtc-dev; + struct drm_i915_private *dev_priv = dev-dev_private; + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct drm_framebuffer *tmpfb; + struct drm_rect dest = { + .x1 = crtc_x, + .y1 = crtc_y, + .x2 = crtc_x + crtc_w, + .y2 = crtc_y + crtc_h, + }; + struct drm_rect clip = { + .x2 = crtc-mode.hdisplay, + .y2 = crtc-mode.vdisplay, + }; + int ret; + + /* +* At the moment we use the same set of setplane restrictions as the +* DRM primary plane helper, so go ahead and just call the helper if +* the primary plane is already enabled. We only need to take special +* action if the primary plane is disabled (something i915 can do but +* the generic helper can't). +*/ + if (intel_crtc-primary_enabled) + return drm_primary_helper_update(plane, crtc, fb, +crtc_x, crtc_y, +crtc_w, crtc_h, +src_x, src_y, +src_w, src_h); + + /* setplane API takes shifted source rectangle values; unshift them */ + src_x = 16; + src_y = 16; + src_w = 16; + src_h = 16; + + /* +* Current hardware can't reposition the primary plane or scale it +* (although this could change in the future). +*/ + drm_rect_intersect(dest, clip); + if (dest.x1 != 0 || dest.y1 != 0 || + dest.x2 != crtc-mode.hdisplay || dest.y2 != crtc-mode.vdisplay) { + DRM_DEBUG_KMS(Primary plane must cover entire CRTC\n); + return -EINVAL; + } + + if (crtc_w != src_w || crtc_h != src_h) { + DRM_DEBUG_KMS(Can't scale primary plane\n); + return -EINVAL; + } + + /* Primary planes are locked to their owning CRTC */ + if (plane-possible_crtcs != drm_crtc_mask(crtc)) { + DRM_DEBUG_KMS(Cannot change primary plane CRTC\n); + return -EINVAL; + } + + /* Framebuffer
[Intel-gfx] [PATCH] drm/i915: Intel-specific primary plane handling (v4)
Intel hardware allows the primary plane to be disabled independently of the CRTC. Provide custom primary plane handling to allow this. v4: - Don't add a primary_plane field to intel_crtc; that was left over from a much earlier iteration of this patch series, but is no longer needed/used now that the DRM core primary plane support has been merged. v3: - Provide gen-specific primary plane format lists (suggested by Daniel Vetter). - If the primary plane is already enabled, go ahead and just call the primary plane helper to do the update (suggested by Daniel Vetter). - Don't try to disable the primary plane on destruction; the DRM layer should have already taken care of this for us. v2: - Unpin fb properly on primary plane disable - Provide an Intel-specific set of primary plane formats - Additional sanity checks on setplane (in line with the checks currently being done by the DRM core primary plane helper) Signed-off-by: Matt Roper matthew.d.ro...@intel.com --- drivers/gpu/drm/i915/intel_display.c | 197 ++- 1 file changed, 195 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1390ab5..3e4d36a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -39,8 +39,35 @@ #include i915_trace.h #include drm/drm_dp_helper.h #include drm/drm_crtc_helper.h +#include drm/drm_plane_helper.h +#include drm/drm_rect.h #include linux/dma_remapping.h +/* Primary plane formats supported by all gen */ +#define COMMON_PRIMARY_FORMATS \ + DRM_FORMAT_C8, \ + DRM_FORMAT_RGB565, \ + DRM_FORMAT_XRGB, \ + DRM_FORMAT_ARGB + +/* Primary plane formats for gen = 3 */ +const static uint32_t intel_primary_formats_gen3[] = { + COMMON_PRIMARY_FORMATS, + DRM_FORMAT_XRGB1555, + DRM_FORMAT_ARGB1555, +}; + +/* Primary plane formats for gen = 4 */ +const static uint32_t intel_primary_formats_gen4[] = { + COMMON_PRIMARY_FORMATS, \ + DRM_FORMAT_XBGR, + DRM_FORMAT_ABGR, + DRM_FORMAT_XRGB2101010, + DRM_FORMAT_ARGB2101010, + DRM_FORMAT_XBGR2101010, + DRM_FORMAT_ABGR2101010, +}; + static void intel_increase_pllclock(struct drm_crtc *crtc); static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on); @@ -10556,17 +10583,183 @@ static void intel_shared_dpll_init(struct drm_device *dev) BUG_ON(dev_priv-num_shared_dpll I915_NUM_PLLS); } +static int +intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc, +struct drm_framebuffer *fb, int crtc_x, int crtc_y, +unsigned int crtc_w, unsigned int crtc_h, +uint32_t src_x, uint32_t src_y, +uint32_t src_w, uint32_t src_h) +{ + struct drm_device *dev = crtc-dev; + struct drm_i915_private *dev_priv = dev-dev_private; + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct drm_framebuffer *tmpfb; + struct drm_rect dest = { + .x1 = crtc_x, + .y1 = crtc_y, + .x2 = crtc_x + crtc_w, + .y2 = crtc_y + crtc_h, + }; + struct drm_rect clip = { + .x2 = crtc-mode.hdisplay, + .y2 = crtc-mode.vdisplay, + }; + int ret; + + /* +* At the moment we use the same set of setplane restrictions as the +* DRM primary plane helper, so go ahead and just call the helper if +* the primary plane is already enabled. We only need to take special +* action if the primary plane is disabled (something i915 can do but +* the generic helper can't). +*/ + if (intel_crtc-primary_enabled) + return drm_primary_helper_update(plane, crtc, fb, +crtc_x, crtc_y, +crtc_w, crtc_h, +src_x, src_y, +src_w, src_h); + + /* setplane API takes shifted source rectangle values; unshift them */ + src_x = 16; + src_y = 16; + src_w = 16; + src_h = 16; + + /* +* Current hardware can't reposition the primary plane or scale it +* (although this could change in the future). +*/ + drm_rect_intersect(dest, clip); + if (dest.x1 != 0 || dest.y1 != 0 || + dest.x2 != crtc-mode.hdisplay || dest.y2 != crtc-mode.vdisplay) { + DRM_DEBUG_KMS(Primary plane must cover entire CRTC\n); + return -EINVAL; + } + + if (crtc_w != src_w || crtc_h != src_h) { + DRM_DEBUG_KMS(Can't scale primary plane\n); + return -EINVAL; + } + + /* Primary planes are locked to their owning CRTC */ + if
[Intel-gfx] [PATCH 2/2] drm/i915: remove misplaced panel wait in DP off code
We do this wait elsewhere, so drop it to speed things up a bit. Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org --- drivers/gpu/drm/i915/intel_dp.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 728a5db..7642415 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2787,7 +2787,6 @@ intel_dp_link_down(struct intel_dp *intel_dp) DP = ~DP_AUDIO_OUTPUT_ENABLE; I915_WRITE(intel_dp-output_reg, DP ~DP_PORT_EN); POSTING_READ(intel_dp-output_reg); - msleep(intel_dp-panel_power_down_delay); } static bool -- 1.8.4.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/i915: remove unexplained vblank wait in the DP off code
I don't think this is necessary; at least it doesn't appear to be on my BYT. Dropping it speeds up our shutdown code a little, in some cases resulting in faster init times. Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org --- drivers/gpu/drm/i915/intel_dp.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index e48d47c..728a5db 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2756,9 +2756,6 @@ intel_dp_link_down(struct intel_dp *intel_dp) } POSTING_READ(intel_dp-output_reg); - /* We don't really know why we're doing this */ - intel_wait_for_vblank(dev, intel_crtc-pipe); - if (HAS_PCH_IBX(dev) I915_READ(intel_dp-output_reg) DP_PIPEB_SELECT) { struct drm_crtc *crtc = intel_dig_port-base.base.crtc; -- 1.8.4.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/bdw: cs-stall before state cache invld w/a
We do this already for previous GENs. I guess we must do it for BDW too according to DOCS. Pipe_control with CS-stall bit set must be issued before a pipe-control command that has the State Cache Invalidate bit set. This does not solve the problem I have unfortunately. I didn't check if this was in Ville's CHV series. If it was, I apologize. NOTE: I tried to use smaller lengths for the command, but nothing made it happy except 6. Cc: Kenneth Graunke kenn...@whitecape.org Cc: Jordan Justen jljus...@gmail.com Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/intel_ringbuffer.c | 19 --- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index a9b04d1..092dea0 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -266,17 +266,25 @@ gen6_render_ring_flush(struct intel_ring_buffer *ring, static int gen7_render_ring_cs_stall_wa(struct intel_ring_buffer *ring) { - int ret; + int ret, size = 4; - ret = intel_ring_begin(ring, 4); + if (IS_BROADWELL(ring-dev)) + size += 2; + + ret = intel_ring_begin(ring, size); if (ret) return ret; - intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(4)); + intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(size)); intel_ring_emit(ring, PIPE_CONTROL_CS_STALL | PIPE_CONTROL_STALL_AT_SCOREBOARD); intel_ring_emit(ring, 0); intel_ring_emit(ring, 0); + if (IS_BROADWELL(ring-dev)) { + intel_ring_emit(ring, 0); + intel_ring_emit(ring, 0); + } + intel_ring_advance(ring); return 0; @@ -389,6 +397,11 @@ gen8_render_ring_flush(struct intel_ring_buffer *ring, flags |= PIPE_CONTROL_STATE_CACHE_INVALIDATE; flags |= PIPE_CONTROL_QW_WRITE; flags |= PIPE_CONTROL_GLOBAL_GTT_IVB; + + /* Workaround: we must issue a pipe_control with CS-stall bit +* set before a pipe_control command that has the state cache +* invalidate bit set. */ + gen7_render_ring_cs_stall_wa(ring); } ret = intel_ring_begin(ring, 6); -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx