Re: [Intel-gfx] Video freezing after activating XScreensaver
On Wed, 2015-10-14 at 14:55 +0200, Daniel Vetter wrote: > On Tue, Oct 13, 2015 at 02:51:25PM -0500, Chris wrote: > > On Tue, 2015-10-13 at 17:30 +0200, Daniel Vetter wrote: > > > On Tue, Oct 13, 2015 at 08:56:52AM -0500, Chris wrote: > > > > This is an issue that has been ongoing for over 1 1/2 years now. I have > > > > tried kernel after kernel and whether it's a Ubuntu kernel such as the > > > > one in my sig or a kernel from here - > > > > http://kernel.ubuntu.com/~kernel-ppa/mainline/drm-intel-next/ the video > > > > will intermittently freeze. Running XScreensaver causes it to freeze > > > > when switching between the different screen savers showing a black > > > > screen and the mouse cursor. The cursor can be moved and will switch > > > > between the arrow and the hand tool if hovering over a clickable area on > > > > the desktop even though it can't be seen. Giving an error output in my > > > > syslog of > > > > > > > > kernel: [578631.820045] [drm:i915_hangcheck_elapsed [i915]] *ERROR* > > > > Hangcheck timer elapsed... render ring idle > > > > > > Known problem and we never managed to fix it. The first time it happens a > > > workaround should kick in, which should prevent any future freezes (until > > > you reboot at least). > > > > > > If freezes still happen after that there's another bug on top. > > > -Daniel > > > > > Thanks for the reply Daniel, and there entails the problem. Once the > > video freezes you have no idea if the 'workaround' kicked in or not. I > > can SSH in via my tablet since all the system is still active. That's > > the only way I can effectively capture any log files and reboot. The > > 'Hangcheck...' error only seems to appear if I start XScreensaver > > running at other times the video will freeze with no errors noted at > > all. > > In that case it seems unrelated to the error in dmesg. No idea what's > happening on your system - hangcheck is rather reliable at figuring out > there's a stuck gpu somewhere. If it doesn't fire the gpu should be all > fine and the stall happens somewhere else. > -Daniel > > I can trace the 'Hangcheck...' error back to starting with Ubuntu kernel 3.13.0-36.63-generic. I have almost 10k hourly syslog snapshots and from what I can see kernel 3.13.0-35.62 generic seemed to work fine. It's hard to say when the freeze started without the 'Hangcheck..' error as the Ubuntu kernel folks have had me trying different kernels. Something else I've noticed now that I'm using kernel 4.2.3-040203-generic is that when lowering a window such as this post the screen will flicker then stop and at times will do it continuously. I've tried to capture a screen shot of this but it stops when I bring up Gnome screen shot. However, each time this happens this is noted in my syslog - Oct 14 09:27:46 localhost kernel: [133491.924244] [drm:drm_mode_addfb2] [FB:75] Oct 14 09:27:46 localhost kernel: [133491.941992] [drm:drm_mode_addfb2] [FB:76] Oct 14 09:28:54 localhost kernel: [133560.356750] [drm:drm_mode_addfb2] [FB:57] Oct 14 09:29:47 localhost kernel: [133612.644738] [drm:drm_mode_addfb2] [FB:59] Anyway, thanks for the reply Daniel. Hopefully some day I'll be able to go more than a few days without a video freeze. > > > > > > > > I was able to reproduce this yesterday by starting XScreensaver at > > > > 9:43am and at 13:44pm the video froze giving the above error. At other > > > > times when XScreensaver is not running the video will still randomly > > > > freeze however there is no specific time frame for this to happen. I've > > > > seen it go 2 days then freeze and have also seen it go 16 days then > > > > freeze. As I've said previously all background operations continue to > > > > run as just the video is frozen. > > > > > > > > I have had the random freeze happen when running this kernelkernel > > > > 4.0.0-997-generic #201503310205 SMP Tue Mar > > > > 31 02:07:04 UTC 2015 which Chris Wilson from this list had me run kernel > > > > kernel 4.0.0-997-generic #201503310205 SMP Tue Mar > > > > 31 02:07:04 UTC 2015 however when I attempted to file a bug report at > > > > Ubuntu Launchpad it was not accepted so the Ubuntu kernel folks had me > > > > switch to kernel 4.2.3-040203-generic #201510030832 SMP Sat Oct 3 > > > > 12:34:31 UTC 2015 which although is an upstream kernel is what they > > > > wanted me to run. The bug report that I've filed on Launchpad is > > > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1497627 > > > > > > > > I've attempted to boot into the following Ubuntu upstream kernels: > > > > > > > > v4.3-rc5-unstable - Boot process would begin but would not complete > > > > v4.3-rc4-unstable - same as above > > > > v4.3-rc3-unstable - same as above > > > > v4.3-rc2-unstable - Same as above > > > > v4.3-rc1-unstable - Same as above > > > > > > > > I also attempted last night to boot into > > > > > > > > v4.3-rc5-unstable from > > > > http://kernel.ubuntu.com/~kernel-ppa/mainline/?C=N;O=D which gave the > > > > same
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly
Hi, On 14/10/15 15:51, Daniel Vetter wrote: The rotated view depends upon the rotation paramters, but thus far we didn't bother checking for those. This seems to have been an issue ever since this was introduce in commit fe14d5f4e5468c5b80a24f1a64abcbe116143670 Author: Tvrtko UrsulinDate: Wed Dec 10 17:27:58 2014 + drm/i915: Infrastructure for supporting different GGTT views per object But userspace is allowed to reuse framebuffer backing storage with different framebuffers with different pixel formats/stride/whatever. And e.g. SNA indeed does this. Hence we must check for all the paramters to match, not just that it's rotated. v2: intel_plane_obj_offset also needs to construct the full view, to avoid fallout since they don't fully match. Cc: Tvrtko Ursulin Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem_gtt.h | 2 +- drivers/gpu/drm/i915/intel_display.c | 10 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 2e1f6493c9e7..8a36f4fcc676 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -551,7 +551,7 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, if (a->type != b->type) return false; - if (a->type == I915_GGTT_VIEW_PARTIAL) + if (a->type != I915_GGTT_VIEW_NORMAL) return !memcmp(>params, >params, sizeof(a->params)); return true; } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 57459fedf216..2a5987ce576c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2894,16 +2894,16 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane, struct drm_i915_gem_object *obj, unsigned int plane) { - const struct i915_ggtt_view *view = _ggtt_view_normal; + struct i915_ggtt_view view; struct i915_vma *vma; unsigned char *offset; - if (intel_rotation_90_or_270(intel_plane->base.state->rotation)) - view = _ggtt_view_rotated; + intel_fill_fb_ggtt_view(, intel_plane->base.fb, + intel_plane->base.state); - vma = i915_gem_obj_to_ggtt_view(obj, view); + vma = i915_gem_obj_to_ggtt_view(obj, ); if (WARN(!vma, "ggtt vma for display object not found! (view=%u)\n", - view->type)) + view.type)) return -1; offset = (unsigned char *)vma->node.start; As we discussed on IRC I had wrong assumptions when developing this. Luckily SNA is not using hardware 90/270 yet so we are safe there. And Android probably doesn't reuse the fb obj or it would have been reported. But I'll check. So thanks for the cleanup! For all three: Reviewed-by: Tvrtko Ursulin Just a shame this means so much more computation in intel_plane_obj_offset, really highlights that vma should be stored in the state, if it is possible. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly
On Wed, Oct 14, 2015 at 04:08:25PM +0100, Tvrtko Ursulin wrote: > > Hi, > > On 14/10/15 15:51, Daniel Vetter wrote: > >The rotated view depends upon the rotation paramters, but thus far we > >didn't bother checking for those. This seems to have been an issue > >ever since this was introduce in > > > >commit fe14d5f4e5468c5b80a24f1a64abcbe116143670 > >Author: Tvrtko Ursulin> >Date: Wed Dec 10 17:27:58 2014 + > > > > drm/i915: Infrastructure for supporting different GGTT views per object > > > >But userspace is allowed to reuse framebuffer backing storage with > >different framebuffers with different pixel formats/stride/whatever. > >And e.g. SNA indeed does this. Hence we must check for all the > >paramters to match, not just that it's rotated. > > > >v2: intel_plane_obj_offset also needs to construct the full view, to > >avoid fallout since they don't fully match. > > > >Cc: Tvrtko Ursulin > >Signed-off-by: Daniel Vetter > >--- > > drivers/gpu/drm/i915/i915_gem_gtt.h | 2 +- > > drivers/gpu/drm/i915/intel_display.c | 10 +- > > 2 files changed, 6 insertions(+), 6 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h > >b/drivers/gpu/drm/i915/i915_gem_gtt.h > >index 2e1f6493c9e7..8a36f4fcc676 100644 > >--- a/drivers/gpu/drm/i915/i915_gem_gtt.h > >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > >@@ -551,7 +551,7 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, > > > > if (a->type != b->type) > > return false; > >-if (a->type == I915_GGTT_VIEW_PARTIAL) > >+if (a->type != I915_GGTT_VIEW_NORMAL) > > return !memcmp(>params, >params, sizeof(a->params)); > > return true; > > } > >diff --git a/drivers/gpu/drm/i915/intel_display.c > >b/drivers/gpu/drm/i915/intel_display.c > >index 57459fedf216..2a5987ce576c 100644 > >--- a/drivers/gpu/drm/i915/intel_display.c > >+++ b/drivers/gpu/drm/i915/intel_display.c > >@@ -2894,16 +2894,16 @@ unsigned long intel_plane_obj_offset(struct > >intel_plane *intel_plane, > > struct drm_i915_gem_object *obj, > > unsigned int plane) > > { > >-const struct i915_ggtt_view *view = _ggtt_view_normal; > >+struct i915_ggtt_view view; > > struct i915_vma *vma; > > unsigned char *offset; > > > >-if (intel_rotation_90_or_270(intel_plane->base.state->rotation)) > >-view = _ggtt_view_rotated; > >+intel_fill_fb_ggtt_view(, intel_plane->base.fb, > >+intel_plane->base.state); > > > >-vma = i915_gem_obj_to_ggtt_view(obj, view); > >+vma = i915_gem_obj_to_ggtt_view(obj, ); > > if (WARN(!vma, "ggtt vma for display object not found! (view=%u)\n", > >-view->type)) > >+view.type)) > > return -1; > > > > offset = (unsigned char *)vma->node.start; > > > > As we discussed on IRC I had wrong assumptions when developing this. > Luckily SNA is not using hardware 90/270 yet so we are safe there. > And Android probably doesn't reuse the fb obj or it would have been > reported. But I'll check. > > So thanks for the cleanup! For all three: > > Reviewed-by: Tvrtko Ursulin > > Just a shame this means so much more computation in > intel_plane_obj_offset, really highlights that vma should be stored > in the state, if it is possible. On your todo list is reviewing the patches that eliminate intel_plane_obj_offset. :-p -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/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly
On 14/10/15 16:35, Chris Wilson wrote: On Wed, Oct 14, 2015 at 04:08:25PM +0100, Tvrtko Ursulin wrote: Hi, On 14/10/15 15:51, Daniel Vetter wrote: The rotated view depends upon the rotation paramters, but thus far we didn't bother checking for those. This seems to have been an issue ever since this was introduce in commit fe14d5f4e5468c5b80a24f1a64abcbe116143670 Author: Tvrtko UrsulinDate: Wed Dec 10 17:27:58 2014 + drm/i915: Infrastructure for supporting different GGTT views per object But userspace is allowed to reuse framebuffer backing storage with different framebuffers with different pixel formats/stride/whatever. And e.g. SNA indeed does this. Hence we must check for all the paramters to match, not just that it's rotated. v2: intel_plane_obj_offset also needs to construct the full view, to avoid fallout since they don't fully match. Cc: Tvrtko Ursulin Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem_gtt.h | 2 +- drivers/gpu/drm/i915/intel_display.c | 10 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 2e1f6493c9e7..8a36f4fcc676 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -551,7 +551,7 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, if (a->type != b->type) return false; - if (a->type == I915_GGTT_VIEW_PARTIAL) + if (a->type != I915_GGTT_VIEW_NORMAL) return !memcmp(>params, >params, sizeof(a->params)); return true; } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 57459fedf216..2a5987ce576c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2894,16 +2894,16 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane, struct drm_i915_gem_object *obj, unsigned int plane) { - const struct i915_ggtt_view *view = _ggtt_view_normal; + struct i915_ggtt_view view; struct i915_vma *vma; unsigned char *offset; - if (intel_rotation_90_or_270(intel_plane->base.state->rotation)) - view = _ggtt_view_rotated; + intel_fill_fb_ggtt_view(, intel_plane->base.fb, + intel_plane->base.state); - vma = i915_gem_obj_to_ggtt_view(obj, view); + vma = i915_gem_obj_to_ggtt_view(obj, ); if (WARN(!vma, "ggtt vma for display object not found! (view=%u)\n", - view->type)) + view.type)) return -1; offset = (unsigned char *)vma->node.start; As we discussed on IRC I had wrong assumptions when developing this. Luckily SNA is not using hardware 90/270 yet so we are safe there. *by default (since it requires Y-tiling of the frontbuffer and that makes other accesses very slow and needing workarounds to prevent overall degredation, for some workloads in particular). Even if compiled to use Y tiling it still goes back to X tiled when 90/270 rotation is attempted. I reported that here: https://bugs.freedesktop.org/show_bug.cgi?id=91562#c14 Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly
On Wed, Oct 14, 2015 at 04:08:25PM +0100, Tvrtko Ursulin wrote: > > Hi, > > On 14/10/15 15:51, Daniel Vetter wrote: > >The rotated view depends upon the rotation paramters, but thus far we > >didn't bother checking for those. This seems to have been an issue > >ever since this was introduce in > > > >commit fe14d5f4e5468c5b80a24f1a64abcbe116143670 > >Author: Tvrtko Ursulin> >Date: Wed Dec 10 17:27:58 2014 + > > > > drm/i915: Infrastructure for supporting different GGTT views per object > > > >But userspace is allowed to reuse framebuffer backing storage with > >different framebuffers with different pixel formats/stride/whatever. > >And e.g. SNA indeed does this. Hence we must check for all the > >paramters to match, not just that it's rotated. > > > >v2: intel_plane_obj_offset also needs to construct the full view, to > >avoid fallout since they don't fully match. > > > >Cc: Tvrtko Ursulin > >Signed-off-by: Daniel Vetter > >--- > > drivers/gpu/drm/i915/i915_gem_gtt.h | 2 +- > > drivers/gpu/drm/i915/intel_display.c | 10 +- > > 2 files changed, 6 insertions(+), 6 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h > >b/drivers/gpu/drm/i915/i915_gem_gtt.h > >index 2e1f6493c9e7..8a36f4fcc676 100644 > >--- a/drivers/gpu/drm/i915/i915_gem_gtt.h > >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > >@@ -551,7 +551,7 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, > > > > if (a->type != b->type) > > return false; > >-if (a->type == I915_GGTT_VIEW_PARTIAL) > >+if (a->type != I915_GGTT_VIEW_NORMAL) > > return !memcmp(>params, >params, sizeof(a->params)); > > return true; > > } > >diff --git a/drivers/gpu/drm/i915/intel_display.c > >b/drivers/gpu/drm/i915/intel_display.c > >index 57459fedf216..2a5987ce576c 100644 > >--- a/drivers/gpu/drm/i915/intel_display.c > >+++ b/drivers/gpu/drm/i915/intel_display.c > >@@ -2894,16 +2894,16 @@ unsigned long intel_plane_obj_offset(struct > >intel_plane *intel_plane, > > struct drm_i915_gem_object *obj, > > unsigned int plane) > > { > >-const struct i915_ggtt_view *view = _ggtt_view_normal; > >+struct i915_ggtt_view view; > > struct i915_vma *vma; > > unsigned char *offset; > > > >-if (intel_rotation_90_or_270(intel_plane->base.state->rotation)) > >-view = _ggtt_view_rotated; > >+intel_fill_fb_ggtt_view(, intel_plane->base.fb, > >+intel_plane->base.state); > > > >-vma = i915_gem_obj_to_ggtt_view(obj, view); > >+vma = i915_gem_obj_to_ggtt_view(obj, ); > > if (WARN(!vma, "ggtt vma for display object not found! (view=%u)\n", > >-view->type)) > >+view.type)) > > return -1; > > > > offset = (unsigned char *)vma->node.start; > > > > As we discussed on IRC I had wrong assumptions when developing this. > Luckily SNA is not using hardware 90/270 yet so we are safe there. *by default (since it requires Y-tiling of the frontbuffer and that makes other accesses very slow and needing workarounds to prevent overall degredation, for some workloads in particular). -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] drm/i915: Hold dev->event_lock whilst inspecting intel_crtc->unpin_work
On Tue, Oct 13, 2015 at 12:23:38PM +0300, Ville Syrjälä wrote: > On Sat, Oct 10, 2015 at 10:44:32AM +0100, Chris Wilson wrote: > > We should serialise access to the intel_crtc->unpin_work through the > > dev->event_lock spinlock. It should not be possible for it to disappear > > without severe error as the mmio_flip worker has not tagged the > > unpin_work pending flip-completion. Similarly if the error exists, just > > taking the unpin_work whilst holding the spinlock and then using it > > unserialised just masks the race. (It is supposed to be valid as the > > unpin_work exists until the flip completion interrupt which should not > > fire until we flush the mmio writes to update the display base which is > > the last time we access the unpin_work from the kthread.) > > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=92335 > > Signed-off-by: Chris Wilson> > So not sure what's going on yet? After a couple of nights sleeping on it, not a clue. Either I've missed something that allows unpin_work to silenty disappear before we mark work->pending as pending (let alone complete) or the oops is from another racy pointer dereference. I'm going to guess the latter and see if there are any candidates (here, I can believe that in the middle of programming the flip we get an interrupt that causes the unpin work to disappear). -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 1/4] drm/i915: Unify execlist and legacy request life-cycles
On 13/10/15 12:36, Chris Wilson wrote: On Tue, Oct 13, 2015 at 01:29:56PM +0200, Daniel Vetter wrote: On Fri, Oct 09, 2015 at 06:23:50PM +0100, Chris Wilson wrote: On Fri, Oct 09, 2015 at 07:18:21PM +0200, Daniel Vetter wrote: On Fri, Oct 09, 2015 at 10:45:35AM +0100, Chris Wilson wrote: On Fri, Oct 09, 2015 at 11:15:08AM +0200, Daniel Vetter wrote: My idea was to create a new request for 3. which gets signalled by the scheduler in intel_lrc_irq_handler. My idea was that we'd only create these when a ctx switch might occur to avoid overhead, but I guess if we just outright delay all requests a notch if need that might work too. But I'm really not sure on the implications of that (i.e. does the hardware really unlod the ctx if it's idle?), and whether that would fly still with the scheduler. But figuring this one out here seems to be the cornestone of this reorg. Without it we can't just throw contexts onto the active list. (Let me see if I understand it correctly) Basically the problem is that we can't trust the context object to be synchronized until after the status interrupt. The way we handled that for legacy is to track the currently bound context and keep the vma->pin_count asserted until the request containing the switch away. Doing the same for execlists would trivially fix the issue and if done smartly allows us to share more code (been there, done that). That satisfies me for keeping requests as a basic fence in the GPU timeline and should keep everyone happy that the context can't vanish until after it is complete. The only caveat is that we cannot evict the most recent context. For legacy, we do a switch back to the always pinned default context. For execlists we don't, but it still means we should only have one context which cannot be evicted (like legacy). But it does leave us with the issue that i915_gpu_idle() returns early and i915_gem_context_fini() must keep the explicit gpu reset to be absolutely sure that the pending context writes are completed before the final context is unbound. Yes, and that was what I originally had in mind. Meanwhile the scheduler (will) happen and that means we won't have FIFO ordering. Which means when we switch contexts (as opposed to just adding more to the ringbuffer of the current one) we won't have any idea which context will be the next one. Which also means we don't know which request to pick to retire the old context. Hence why I think we need to be better. But the scheduler does - it is also in charge of making sure the retirement queue is in order. The essence is that we only actually pin engine->last_context, which is chosen as we submit stuff to the hw. Well I'm not sure how much it will reorder, but I'd expect it wants to reorder stuff pretty freely. And as soon as it reorders context (ofc they can't depend on each another) then the legacy hw ctx tracking won't work. I think at least ... Not the way it is written today, but the principle behind it still stands. The last_context submitted to the hardware is pinned until a new one is submitted (such that it remains bound in the GGTT until after the context switch is complete due to the active reference). Instead of doing the context tracking at the start of the execbuffer, the context tracking needs to be pushed down to the submission backend/middleman. -Chris Does anyone actually know what guarantees (if any) the GPU provides w.r.t access to context images vs. USER_INTERRUPTs and CSB-updated interrupts? Does 'active->idle' really mean that the context has been fully updated in memory (and can therefore be unmapped), or just that the engine has stopped processing (but the context might not be saved until it's known that it isn't going to be reactivated). For example, it could implement this: (End of last batch in current context) 1. Update seqno 2. Generate USER_INTERRUPT 3. Engine finishes work (HEAD == TAIL and no further contexts queued in ELSP) 4. Save all per-context registers to context image 5. Flush to memory and invalidate 6. Update CSB 7. Flush to memory 8. Generate CSB-update interrupt. (New batch in same context submitted via ELSP) 9. Reload entire context image from memory 10. Update CSB 11. Generate CSB-update interrupt Or this: 1. Update seqno 2. Generate USER_INTERRUPT 3. Engine finishes work (HEAD == TAIL and no further contexts queued in ELSP) 4. Update CSB 5. Generate CSB-update interrupt. (New batch in DIFFERENT context submitted via ELSP) 6. Save all per-context registers to old context image 7. Load entire context image from new image 8. Update CSB 9. Generate CSB-update interrupt The former is synchronous and relatively easy to model, the latter is more like the way legacy mode works. Any various other
[Intel-gfx] [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly
The rotated view depends upon the rotation paramters, but thus far we didn't bother checking for those. This seems to have been an issue ever since this was introduce in commit fe14d5f4e5468c5b80a24f1a64abcbe116143670 Author: Tvrtko UrsulinDate: Wed Dec 10 17:27:58 2014 + drm/i915: Infrastructure for supporting different GGTT views per object But userspace is allowed to reuse framebuffer backing storage with different framebuffers with different pixel formats/stride/whatever. And e.g. SNA indeed does this. Hence we must check for all the paramters to match, not just that it's rotated. v2: intel_plane_obj_offset also needs to construct the full view, to avoid fallout since they don't fully match. Cc: Tvrtko Ursulin Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem_gtt.h | 2 +- drivers/gpu/drm/i915/intel_display.c | 10 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 2e1f6493c9e7..8a36f4fcc676 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -551,7 +551,7 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, if (a->type != b->type) return false; - if (a->type == I915_GGTT_VIEW_PARTIAL) + if (a->type != I915_GGTT_VIEW_NORMAL) return !memcmp(>params, >params, sizeof(a->params)); return true; } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 57459fedf216..2a5987ce576c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2894,16 +2894,16 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane, struct drm_i915_gem_object *obj, unsigned int plane) { - const struct i915_ggtt_view *view = _ggtt_view_normal; + struct i915_ggtt_view view; struct i915_vma *vma; unsigned char *offset; - if (intel_rotation_90_or_270(intel_plane->base.state->rotation)) - view = _ggtt_view_rotated; + intel_fill_fb_ggtt_view(, intel_plane->base.fb, + intel_plane->base.state); - vma = i915_gem_obj_to_ggtt_view(obj, view); + vma = i915_gem_obj_to_ggtt_view(obj, ); if (WARN(!vma, "ggtt vma for display object not found! (view=%u)\n", - view->type)) + view.type)) return -1; offset = (unsigned char *)vma->node.start; -- 2.5.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/3] drm/i915: Stuff rotation params into view union
We don't need 2 separate unions. Note that this was done intentinoally Author: Joonas LahtinenDate: Wed May 6 14:35:38 2015 +0300 drm/i915: Add a partial GGTT view type on Tvrtko's request, but without a clear justification. Rotated views are also not checking for matching paramters in i915_ggtt_view_equal, which seems like a bug. But this patch here doesn't change that. Cc: Tvrtko Ursulin Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++-- drivers/gpu/drm/i915/i915_gem_gtt.h | 5 + drivers/gpu/drm/i915/intel_display.c | 4 ++-- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index bead61e0de1b..15dfefd58d08 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3283,7 +3283,7 @@ static struct sg_table * intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view, struct drm_i915_gem_object *obj) { - struct intel_rotation_info *rot_info = _view->rotation_info; + struct intel_rotation_info *rot_info = _view->params.rotation_info; unsigned int size_pages = rot_info->size >> PAGE_SHIFT; unsigned int size_pages_uv; struct sg_page_iter sg_iter; @@ -3515,7 +3515,7 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj, if (view->type == I915_GGTT_VIEW_NORMAL) { return obj->base.size; } else if (view->type == I915_GGTT_VIEW_ROTATED) { - return view->rotation_info.size; + return view->params.rotation_info.size; } else if (view->type == I915_GGTT_VIEW_PARTIAL) { return view->params.partial.size << PAGE_SHIFT; } else { diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 9fbb07d6eaad..2e1f6493c9e7 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -156,13 +156,10 @@ struct i915_ggtt_view { u64 offset; unsigned int size; } partial; + struct intel_rotation_info rotation_info; } params; struct sg_table *pages; - - union { - struct intel_rotation_info rotation_info; - }; }; extern const struct i915_ggtt_view i915_ggtt_view_normal; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2c9d822b29b6..57459fedf216 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2247,7 +2247,7 @@ static void intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, const struct drm_plane_state *plane_state) { - struct intel_rotation_info *info = >rotation_info; + struct intel_rotation_info *info = >params.rotation_info; unsigned int tile_height, tile_pitch; *view = i915_ggtt_view_normal; @@ -2909,7 +2909,7 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane, offset = (unsigned char *)vma->node.start; if (plane == 1) { - offset += vma->ggtt_view.rotation_info.uv_start_page * + offset += vma->ggtt_view.params.rotation_info.uv_start_page * PAGE_SIZE; } -- 2.5.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/i915: Drop return value from intel_fill_fb_ggtt_view
It can't fail and there's even a WARN_ON suggesting that if it would, it would be a disaster. Correct this to make things less confusing. Cc: Tvrtko UrsulinSigned-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_display.c | 16 +--- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 184725770ae7..2c9d822b29b6 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2243,7 +2243,7 @@ intel_fb_align_height(struct drm_device *dev, unsigned int height, fb_format_modifier, 0)); } -static int +static void intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, const struct drm_plane_state *plane_state) { @@ -2253,10 +2253,10 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, *view = i915_ggtt_view_normal; if (!plane_state) - return 0; + return; if (!intel_rotation_90_or_270(plane_state->rotation)) - return 0; + return; *view = i915_ggtt_view_rotated; @@ -2283,8 +2283,6 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, info->size_uv = info->width_pages_uv * info->height_pages_uv * PAGE_SIZE; } - - return 0; } static unsigned int intel_linear_alignment(struct drm_i915_private *dev_priv) @@ -2340,9 +2338,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, return -EINVAL; } - ret = intel_fill_fb_ggtt_view(, fb, plane_state); - if (ret) - return ret; + intel_fill_fb_ggtt_view(, fb, plane_state); /* Note that the w/a also requires 64 PTE of padding following the * bo. We currently fill all unused PTE with the shadow page and so @@ -2406,12 +2402,10 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb, { struct drm_i915_gem_object *obj = intel_fb_obj(fb); struct i915_ggtt_view view; - int ret; WARN_ON(!mutex_is_locked(>base.dev->struct_mutex)); - ret = intel_fill_fb_ggtt_view(, fb, plane_state); - WARN_ONCE(ret, "Couldn't get view from plane state!"); + intel_fill_fb_ggtt_view(, fb, plane_state); i915_gem_object_unpin_fence(obj); i915_gem_object_unpin_from_display_plane(obj, ); -- 2.5.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly
On Wed, Oct 14, 2015 at 04:55:47PM +0100, Tvrtko Ursulin wrote: > > On 14/10/15 16:35, Chris Wilson wrote: > >On Wed, Oct 14, 2015 at 04:08:25PM +0100, Tvrtko Ursulin wrote: > >> > >>Hi, > >> > >>On 14/10/15 15:51, Daniel Vetter wrote: > >>>The rotated view depends upon the rotation paramters, but thus far we > >>>didn't bother checking for those. This seems to have been an issue > >>>ever since this was introduce in > >>> > >>>commit fe14d5f4e5468c5b80a24f1a64abcbe116143670 > >>>Author: Tvrtko Ursulin> >>>Date: Wed Dec 10 17:27:58 2014 + > >>> > >>> drm/i915: Infrastructure for supporting different GGTT views per > >>> object > >>> > >>>But userspace is allowed to reuse framebuffer backing storage with > >>>different framebuffers with different pixel formats/stride/whatever. > >>>And e.g. SNA indeed does this. Hence we must check for all the > >>>paramters to match, not just that it's rotated. > >>> > >>>v2: intel_plane_obj_offset also needs to construct the full view, to > >>>avoid fallout since they don't fully match. > >>> > >>>Cc: Tvrtko Ursulin > >>>Signed-off-by: Daniel Vetter > >>>--- > >>> drivers/gpu/drm/i915/i915_gem_gtt.h | 2 +- > >>> drivers/gpu/drm/i915/intel_display.c | 10 +- > >>> 2 files changed, 6 insertions(+), 6 deletions(-) > >>> > >>>diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h > >>>b/drivers/gpu/drm/i915/i915_gem_gtt.h > >>>index 2e1f6493c9e7..8a36f4fcc676 100644 > >>>--- a/drivers/gpu/drm/i915/i915_gem_gtt.h > >>>+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > >>>@@ -551,7 +551,7 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, > >>> > >>> if (a->type != b->type) > >>> return false; > >>>- if (a->type == I915_GGTT_VIEW_PARTIAL) > >>>+ if (a->type != I915_GGTT_VIEW_NORMAL) > >>> return !memcmp(>params, >params, sizeof(a->params)); > >>> return true; > >>> } > >>>diff --git a/drivers/gpu/drm/i915/intel_display.c > >>>b/drivers/gpu/drm/i915/intel_display.c > >>>index 57459fedf216..2a5987ce576c 100644 > >>>--- a/drivers/gpu/drm/i915/intel_display.c > >>>+++ b/drivers/gpu/drm/i915/intel_display.c > >>>@@ -2894,16 +2894,16 @@ unsigned long intel_plane_obj_offset(struct > >>>intel_plane *intel_plane, > >>>struct drm_i915_gem_object *obj, > >>>unsigned int plane) > >>> { > >>>- const struct i915_ggtt_view *view = _ggtt_view_normal; > >>>+ struct i915_ggtt_view view; > >>> struct i915_vma *vma; > >>> unsigned char *offset; > >>> > >>>- if (intel_rotation_90_or_270(intel_plane->base.state->rotation)) > >>>- view = _ggtt_view_rotated; > >>>+ intel_fill_fb_ggtt_view(, intel_plane->base.fb, > >>>+ intel_plane->base.state); > >>> > >>>- vma = i915_gem_obj_to_ggtt_view(obj, view); > >>>+ vma = i915_gem_obj_to_ggtt_view(obj, ); > >>> if (WARN(!vma, "ggtt vma for display object not found! (view=%u)\n", > >>>- view->type)) > >>>+ view.type)) > >>> return -1; > >>> > >>> offset = (unsigned char *)vma->node.start; > >>> > >> > >>As we discussed on IRC I had wrong assumptions when developing this. > >>Luckily SNA is not using hardware 90/270 yet so we are safe there. > > > >*by default (since it requires Y-tiling of the frontbuffer and that > >makes other accesses very slow and needing workarounds to prevent > >overall degredation, for some workloads in particular). > > Even if compiled to use Y tiling it still goes back to X tiled when > 90/270 rotation is attempted. I reported that here: > https://bugs.freedesktop.org/show_bug.cgi?id=91562#c14 Which says the kernel rejected the setcrtc with the primary rotation property set to 2 (either ROTATE_90 or 270, I forget which). -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/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly
On 14/10/15 16:33, Chris Wilson wrote: On Wed, Oct 14, 2015 at 04:08:25PM +0100, Tvrtko Ursulin wrote: Hi, On 14/10/15 15:51, Daniel Vetter wrote: The rotated view depends upon the rotation paramters, but thus far we didn't bother checking for those. This seems to have been an issue ever since this was introduce in commit fe14d5f4e5468c5b80a24f1a64abcbe116143670 Author: Tvrtko UrsulinDate: Wed Dec 10 17:27:58 2014 + drm/i915: Infrastructure for supporting different GGTT views per object But userspace is allowed to reuse framebuffer backing storage with different framebuffers with different pixel formats/stride/whatever. And e.g. SNA indeed does this. Hence we must check for all the paramters to match, not just that it's rotated. v2: intel_plane_obj_offset also needs to construct the full view, to avoid fallout since they don't fully match. Cc: Tvrtko Ursulin Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem_gtt.h | 2 +- drivers/gpu/drm/i915/intel_display.c | 10 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 2e1f6493c9e7..8a36f4fcc676 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -551,7 +551,7 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, if (a->type != b->type) return false; - if (a->type == I915_GGTT_VIEW_PARTIAL) + if (a->type != I915_GGTT_VIEW_NORMAL) return !memcmp(>params, >params, sizeof(a->params)); return true; } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 57459fedf216..2a5987ce576c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2894,16 +2894,16 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane, struct drm_i915_gem_object *obj, unsigned int plane) { - const struct i915_ggtt_view *view = _ggtt_view_normal; + struct i915_ggtt_view view; struct i915_vma *vma; unsigned char *offset; - if (intel_rotation_90_or_270(intel_plane->base.state->rotation)) - view = _ggtt_view_rotated; + intel_fill_fb_ggtt_view(, intel_plane->base.fb, + intel_plane->base.state); - vma = i915_gem_obj_to_ggtt_view(obj, view); + vma = i915_gem_obj_to_ggtt_view(obj, ); if (WARN(!vma, "ggtt vma for display object not found! (view=%u)\n", - view->type)) + view.type)) return -1; offset = (unsigned char *)vma->node.start; As we discussed on IRC I had wrong assumptions when developing this. Luckily SNA is not using hardware 90/270 yet so we are safe there. And Android probably doesn't reuse the fb obj or it would have been reported. But I'll check. So thanks for the cleanup! For all three: Reviewed-by: Tvrtko Ursulin Just a shame this means so much more computation in intel_plane_obj_offset, really highlights that vma should be stored in the state, if it is possible. On your todo list is reviewing the patches that eliminate intel_plane_obj_offset. :-p Have I missed something? I thought I reviewed what you sent so far. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw
On Wed, Oct 14, 2015 at 04:58:55PM +0300, Ander Conselvan De Oliveira wrote: > On Wed, 2015-10-14 at 14:44 +0200, Daniel Vetter wrote: > > On Wed, Oct 14, 2015 at 11:21:46AM +0300, Ander Conselvan De Oliveira wrote: > > > On Tue, 2015-10-13 at 16:08 +0200, Daniel Vetter wrote: > > > > On Tue, Oct 13, 2015 at 04:00:37PM +0200, Maarten Lankhorst wrote: > > > > > Op 13-10-15 om 15:58 schreef Daniel Vetter: > > > > > > On Tue, Oct 13, 2015 at 03:43:28PM +0200, Maarten Lankhorst wrote: > > > > > > > Op 13-10-15 om 15:35 schreef Daniel Vetter: > > > > > > > > On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst > > > > > > > > wrote: > > > > > > > > > Op 23-09-15 om 17:34 schreef Gabriel Feceoru: > > > > > > > > > > Using 2 connectors (DVI and VGA) will cause wrpll to be set > > > > > > > > > > for > > > > > > > > > > INTEL_OUTPUT_HDMI but never reset if switching to > > > > > > > > > > INTEL_OUTPUT_VGA > > > > > > > > > > > > > > > > > > > > Supresses errors like these: > > > > > > > > > > [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in > > > > > > > > > > dpll_hw_state.wrpll > > > > > > > > > > > > > > > > > > > Looks like a good idea to always zero it. > > > > > > > > Except that we still have a bunch of cases where we recompute > > > > > > > > clock state > > > > > > > > but only partially. Can we just move them all up into a common > > > > > > > > place > > > > > > > > please? That would also catch cases where we simply forget to > > > > > > > > fill this > > > > > > > > out at all. > > > > > > > > > > > > > > > > One case I noticed is edp in skl_ddi_pll_select, but there's > > > > > > > > probably > > > > > > > > more. > > > > > > > > > > > > > > > Something like below, with all the memsets for dpll_hw_state > > > > > > > removed? > > > > > > I think this will blow up since we recompute clock state only when > > > > > > needs_modeset is true. So needs a bit more intelligence in deciding > > > > > > when > > > > > > to clear it I think. > > > > > Oops you're right. Maybe intel_modeset_clear_plls because that's > > > > > where all the clock state > > > > > belongs? > > > > > > > > Yeah that might be an even better place, in the loop after the continue; > > > > statement. > > > > > > The reason I didn't put the memset there in the first place was the way > > > we calculate plls for DP > > > with DDI platforms. In that case, ddi_pll_sel is setup from the > > > encoder_config instead of > > > compute_clock, so a memset ends up clearing the new pll config. > > > > Hm, I forgot about this split totally. And there seems to be a giant mess > > going on here: > > > > In our top-level intel_atomic_check we have 4 parts to compute state: > > 1. drm_atomic_helper_check_modeset > > 2. intel_modeset_pipe_config > > 3. intel_modeset_checks > > 4. drm_atomic_helper_check_planes > > > > We recalculate clocks (by calling dev_priv->display.crtc_compute_clock) > > in 1., way ahead of anything else in intel_crtc_atomic_check. That looks > > very suspcious since it means only very later on (in the loop that does > > 2.) do we even decide whether we need to do a full modeset or not. > > > > So what I had in mind is that we clear clocks in > > intel_modeset_pipe_config, before we call any of the callbacks. That makes > > sure that when we decided to do a modeset, we do recompute the clocks > > correctly. > > I had a suspicion this would interact badly with how we "cancel" the modeset > if the pipe config > didn't changed, just after the call to intel_modeset_pipe_config(). It turns > out there's an issue > there already. > > There are two possibilities for the dpll_hw_state value after the new > pipe_config is calculated. It > may have the new values already for DP in HSW/BDW and eDP in SKL or it may > still have the old value. > In the latter case the new value is only calculated in .crtc_clock(), after > we already compared the > old and new configs and may have decided to skip the modeset. > > But doing the memset() in intel_modeset_pipe_config() would be find as long > as we don't change our > minds about doing a modeset later. It's more annoying since my analysis is all wrong: intel_crtc_atomic_check is called from drm_atomic_helper_check_planes, i.e. step 4 not step 1. It'll all work out I think if we memset it in intel_modeset_pipe_config. The caveat is that we need to move the clock recomputation into intel_modeset_pipe_config too (which is better, since then we'll have more accurate state to decided whether we'll fastboot or not). And then intel_modeset_clear_plls would really just update the global pll setup (and would be really good to rename it to intel_modeset_compute_shared_dpll or whatever). Thoughts? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Unify execlist and legacy request life-cycles
On 14/10/2015 15:42, Dave Gordon wrote: On 13/10/15 12:36, Chris Wilson wrote: On Tue, Oct 13, 2015 at 01:29:56PM +0200, Daniel Vetter wrote: On Fri, Oct 09, 2015 at 06:23:50PM +0100, Chris Wilson wrote: On Fri, Oct 09, 2015 at 07:18:21PM +0200, Daniel Vetter wrote: On Fri, Oct 09, 2015 at 10:45:35AM +0100, Chris Wilson wrote: On Fri, Oct 09, 2015 at 11:15:08AM +0200, Daniel Vetter wrote: My idea was to create a new request for 3. which gets signalled by the scheduler in intel_lrc_irq_handler. My idea was that we'd only create these when a ctx switch might occur to avoid overhead, but I guess if we just outright delay all requests a notch if need that might work too. But I'm really not sure on the implications of that (i.e. does the hardware really unlod the ctx if it's idle?), and whether that would fly still with the scheduler. But figuring this one out here seems to be the cornestone of this reorg. Without it we can't just throw contexts onto the active list. (Let me see if I understand it correctly) Basically the problem is that we can't trust the context object to be synchronized until after the status interrupt. The way we handled that for legacy is to track the currently bound context and keep the vma->pin_count asserted until the request containing the switch away. Doing the same for execlists would trivially fix the issue and if done smartly allows us to share more code (been there, done that). That satisfies me for keeping requests as a basic fence in the GPU timeline and should keep everyone happy that the context can't vanish until after it is complete. The only caveat is that we cannot evict the most recent context. For legacy, we do a switch back to the always pinned default context. For execlists we don't, but it still means we should only have one context which cannot be evicted (like legacy). But it does leave us with the issue that i915_gpu_idle() returns early and i915_gem_context_fini() must keep the explicit gpu reset to be absolutely sure that the pending context writes are completed before the final context is unbound. Yes, and that was what I originally had in mind. Meanwhile the scheduler (will) happen and that means we won't have FIFO ordering. Which means when we switch contexts (as opposed to just adding more to the ringbuffer of the current one) we won't have any idea which context will be the next one. Which also means we don't know which request to pick to retire the old context. Hence why I think we need to be better. But the scheduler does - it is also in charge of making sure the retirement queue is in order. The essence is that we only actually pin engine->last_context, which is chosen as we submit stuff to the hw. Well I'm not sure how much it will reorder, but I'd expect it wants to reorder stuff pretty freely. And as soon as it reorders context (ofc they can't depend on each another) then the legacy hw ctx tracking won't work. I think at least ... Not the way it is written today, but the principle behind it still stands. The last_context submitted to the hardware is pinned until a new one is submitted (such that it remains bound in the GGTT until after the context switch is complete due to the active reference). Instead of doing the context tracking at the start of the execbuffer, the context tracking needs to be pushed down to the submission backend/middleman. -Chris Does anyone actually know what guarantees (if any) the GPU provides w.r.t access to context images vs. USER_INTERRUPTs and CSB-updated interrupts? Does 'active->idle' really mean that the context has been fully updated in memory (and can therefore be unmapped), or just that the engine has stopped processing (but the context might not be saved until it's known that it isn't going to be reactivated). For example, it could implement this: (End of last batch in current context) 1. Update seqno 2. Generate USER_INTERRUPT 3. Engine finishes work (HEAD == TAIL and no further contexts queued in ELSP) 4. Save all per-context registers to context image 5. Flush to memory and invalidate 6. Update CSB 7. Flush to memory 8. Generate CSB-update interrupt. (New batch in same context submitted via ELSP) 9. Reload entire context image from memory 10. Update CSB 11. Generate CSB-update interrupt Or this: 1. Update seqno 2. Generate USER_INTERRUPT 3. Engine finishes work (HEAD == TAIL and no further contexts queued in ELSP) 4. Update CSB 5. Generate CSB-update interrupt. (New batch in DIFFERENT context submitted via ELSP) 6. Save all per-context registers to old context image 7. Load entire context image from new image 8. Update CSB 9. Generate CSB-update interrupt The former is synchronous and relatively easy to model, the latter is more like the way
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly
On Wed, Oct 14, 2015 at 04:56:44PM +0100, Tvrtko Ursulin wrote: > > On 14/10/15 16:33, Chris Wilson wrote: > >On Wed, Oct 14, 2015 at 04:08:25PM +0100, Tvrtko Ursulin wrote: > >> > >>Hi, > >> > >>On 14/10/15 15:51, Daniel Vetter wrote: > >>>The rotated view depends upon the rotation paramters, but thus far we > >>>didn't bother checking for those. This seems to have been an issue > >>>ever since this was introduce in > >>> > >>>commit fe14d5f4e5468c5b80a24f1a64abcbe116143670 > >>>Author: Tvrtko Ursulin> >>>Date: Wed Dec 10 17:27:58 2014 + > >>> > >>> drm/i915: Infrastructure for supporting different GGTT views per > >>> object > >>> > >>>But userspace is allowed to reuse framebuffer backing storage with > >>>different framebuffers with different pixel formats/stride/whatever. > >>>And e.g. SNA indeed does this. Hence we must check for all the > >>>paramters to match, not just that it's rotated. > >>> > >>>v2: intel_plane_obj_offset also needs to construct the full view, to > >>>avoid fallout since they don't fully match. > >>> > >>>Cc: Tvrtko Ursulin > >>>Signed-off-by: Daniel Vetter > >>>--- > >>> drivers/gpu/drm/i915/i915_gem_gtt.h | 2 +- > >>> drivers/gpu/drm/i915/intel_display.c | 10 +- > >>> 2 files changed, 6 insertions(+), 6 deletions(-) > >>> > >>>diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h > >>>b/drivers/gpu/drm/i915/i915_gem_gtt.h > >>>index 2e1f6493c9e7..8a36f4fcc676 100644 > >>>--- a/drivers/gpu/drm/i915/i915_gem_gtt.h > >>>+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > >>>@@ -551,7 +551,7 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, > >>> > >>> if (a->type != b->type) > >>> return false; > >>>- if (a->type == I915_GGTT_VIEW_PARTIAL) > >>>+ if (a->type != I915_GGTT_VIEW_NORMAL) > >>> return !memcmp(>params, >params, sizeof(a->params)); > >>> return true; > >>> } > >>>diff --git a/drivers/gpu/drm/i915/intel_display.c > >>>b/drivers/gpu/drm/i915/intel_display.c > >>>index 57459fedf216..2a5987ce576c 100644 > >>>--- a/drivers/gpu/drm/i915/intel_display.c > >>>+++ b/drivers/gpu/drm/i915/intel_display.c > >>>@@ -2894,16 +2894,16 @@ unsigned long intel_plane_obj_offset(struct > >>>intel_plane *intel_plane, > >>>struct drm_i915_gem_object *obj, > >>>unsigned int plane) > >>> { > >>>- const struct i915_ggtt_view *view = _ggtt_view_normal; > >>>+ struct i915_ggtt_view view; > >>> struct i915_vma *vma; > >>> unsigned char *offset; > >>> > >>>- if (intel_rotation_90_or_270(intel_plane->base.state->rotation)) > >>>- view = _ggtt_view_rotated; > >>>+ intel_fill_fb_ggtt_view(, intel_plane->base.fb, > >>>+ intel_plane->base.state); > >>> > >>>- vma = i915_gem_obj_to_ggtt_view(obj, view); > >>>+ vma = i915_gem_obj_to_ggtt_view(obj, ); > >>> if (WARN(!vma, "ggtt vma for display object not found! (view=%u)\n", > >>>- view->type)) > >>>+ view.type)) > >>> return -1; > >>> > >>> offset = (unsigned char *)vma->node.start; > >>> > >> > >>As we discussed on IRC I had wrong assumptions when developing this. > >>Luckily SNA is not using hardware 90/270 yet so we are safe there. > >>And Android probably doesn't reuse the fb obj or it would have been > >>reported. But I'll check. > >> > >>So thanks for the cleanup! For all three: > >> > >>Reviewed-by: Tvrtko Ursulin > >> > >>Just a shame this means so much more computation in > >>intel_plane_obj_offset, really highlights that vma should be stored > >>in the state, if it is possible. > > > >On your todo list is reviewing the patches that eliminate > >intel_plane_obj_offset. > >:-p > > Have I missed something? I thought I reviewed what you sent so far. It's the next step of the vma rewrite which was in the previous pile. Once we are using the VMA as the pin/unpin cookie, storiing it in the plane->state and using the VMA directly is a natural consequence. It just requires playing along with atomic. -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 18/22] drm/i915: Make sure fb offset is (macro)pixel aligned
On Wed, Oct 14, 2015 at 07:29:10PM +0300, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä> > We convert the fb->offsets[] into x/y offsets, so they must be > (macro)pixel aligned. > > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_display.c | 36 > +--- > 1 file changed, 33 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index ce346cfe..70e6e27 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14284,6 +14284,36 @@ u32 intel_fb_pitch_limit(struct drm_device *dev, > uint64_t fb_modifier, > } > } > > +static int intel_fb_check_offsets(const struct drm_mode_fb_cmd2 *mode_cmd) > +{ > + uint32_t format = mode_cmd->pixel_format; > + int i; > + > + for (i = 0; i < drm_format_num_planes(format); i++) { > + unsigned int cpp; > + > + switch (format) { > + case DRM_FORMAT_YUYV: > + case DRM_FORMAT_UYVY: > + case DRM_FORMAT_YVYU: > + case DRM_FORMAT_VYUY: > + cpp = 4; > + break; > + default: > + cpp = drm_format_plane_cpp(format, i); > + break; Might be worth it to extract this into a drm helper. gallium also has the concept of a pixel block size, and it's _really_ nice imo. -Daniel > + } > + > + if (mode_cmd->offsets[i] % cpp) { > + DRM_DEBUG("fb plane %d offset 0x%08x not (macro)pixel > aligned\n", > + i, mode_cmd->offsets[i]); > + return -EINVAL; > + } > + } > + > + return 0; > +} > + > static int intel_framebuffer_init(struct drm_device *dev, > struct intel_framebuffer *intel_fb, > struct drm_mode_fb_cmd2 *mode_cmd, > @@ -14410,9 +14440,9 @@ static int intel_framebuffer_init(struct drm_device > *dev, > return -EINVAL; > } > > - /* FIXME need to adjust LINOFF/TILEOFF accordingly. */ > - if (mode_cmd->offsets[0] != 0) > - return -EINVAL; > + ret = intel_fb_check_offsets(mode_cmd); > + if (ret) > + return ret; > > aligned_height = intel_fb_align_height(dev, mode_cmd->height, > mode_cmd->pixel_format, > -- > 2.4.9 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v2] tests/core_prop_blob: Fix core_prop_blob for android
core_prop_blob was using ioctls not in the android kernel. Added a igt_require_propblob() function and local defines/structures so the test will compile and skip on kernels where the feature is unsupported. v2: moved igt_require_propblob() to core_prop_blob.c (Daniel Vetter) Moved gem_blt.c to a seperate patch (Thomas Wood) Signed-off-by: Derek Morton--- tests/core_prop_blob.c | 71 -- 1 file changed, 52 insertions(+), 19 deletions(-) diff --git a/tests/core_prop_blob.c b/tests/core_prop_blob.c index d704158..4dc6d7d 100644 --- a/tests/core_prop_blob.c +++ b/tests/core_prop_blob.c @@ -25,18 +25,35 @@ * Daniel Stone */ +#include "igt.h" #include #include #include #include -#include "drmtest.h" -#include "igt_debugfs.h" -#include "igt_kms.h" -#include "igt_aux.h" - IGT_TEST_DESCRIPTION("Tests behaviour of mass-data 'blob' properties."); +struct local_drm_mode_get_blob { + uint32_t blob_id; + uint32_t length; + uint64_t data; +}; +struct local_drm_mode_create_blob { + uint64_t data; + uint32_t length; + uint32_t blob_id; +}; +struct local_drm_mode_destroy_blob { + uint32_t blob_id; +}; + +#define LOCAL_DRM_IOCTL_MODE_GETPROPBLOB DRM_IOWR(0xAC, \ + struct local_drm_mode_get_blob) +#define LOCAL_DRM_IOCTL_MODE_CREATEPROPBLOBDRM_IOWR(0xBD, \ + struct local_drm_mode_create_blob) +#define LOCAL_DRM_IOCTL_MODE_DESTROYPROPBLOB DRM_IOWR(0xBE, \ + struct local_drm_mode_destroy_blob) + static const struct drm_mode_modeinfo test_mode_valid = { .clock = 1234, .hdisplay = 640, @@ -61,22 +78,35 @@ static const struct drm_mode_modeinfo test_mode_valid = { return errno; \ } +static void igt_require_propblob(int fd) +{ + struct local_drm_mode_create_blob c; + struct local_drm_mode_destroy_blob d; + uint32_t blob_data; + c.data = _data; + c.length = sizeof(blob_data); + + igt_require(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_CREATEPROPBLOB, ) == 0); + d.blob_id = c.blob_id; + igt_require(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_DESTROYPROPBLOB, ) == 0); +} + static int validate_prop(int fd, uint32_t prop_id) { - struct drm_mode_get_blob get; + struct local_drm_mode_get_blob get; struct drm_mode_modeinfo ret_mode; get.blob_id = prop_id; get.length = 0; get.data = (uintptr_t) 0; - ioctl_or_ret_errno(fd, DRM_IOCTL_MODE_GETPROPBLOB, ); + ioctl_or_ret_errno(fd, LOCAL_DRM_IOCTL_MODE_GETPROPBLOB, ); if (get.length != sizeof(test_mode_valid)) return ENOMEM; get.data = (uintptr_t) _mode; - ioctl_or_ret_errno(fd, DRM_IOCTL_MODE_GETPROPBLOB, ); + ioctl_or_ret_errno(fd, LOCAL_DRM_IOCTL_MODE_GETPROPBLOB, ); if (memcmp(_mode, _mode_valid, sizeof(test_mode_valid)) != 0) return EINVAL; @@ -87,12 +117,12 @@ validate_prop(int fd, uint32_t prop_id) static uint32_t create_prop(int fd) { - struct drm_mode_create_blob create; + struct local_drm_mode_create_blob create; create.length = sizeof(test_mode_valid); create.data = (uintptr_t) _mode_valid; - do_ioctl(fd, DRM_IOCTL_MODE_CREATEPROPBLOB, ); + do_ioctl(fd, LOCAL_DRM_IOCTL_MODE_CREATEPROPBLOB, ); igt_assert_neq_u32(create.blob_id, 0); return create.blob_id; @@ -101,10 +131,10 @@ create_prop(int fd) static int destroy_prop(int fd, uint32_t prop_id) { - struct drm_mode_destroy_blob destroy; + struct local_drm_mode_destroy_blob destroy; destroy.blob_id = prop_id; - ioctl_or_ret_errno(fd, DRM_IOCTL_MODE_DESTROYPROPBLOB, ); + ioctl_or_ret_errno(fd, LOCAL_DRM_IOCTL_MODE_DESTROYPROPBLOB, ); return 0; } @@ -112,8 +142,8 @@ destroy_prop(int fd, uint32_t prop_id) static void test_validate(int fd) { - struct drm_mode_create_blob create; - struct drm_mode_get_blob get; + struct local_drm_mode_create_blob create; + struct local_drm_mode_get_blob get; char too_small[32]; uint32_t prop_id; @@ -122,24 +152,24 @@ test_validate(int fd) /* Outlandish size. */ create.length = 0x1; create.data = (uintptr_t) _small; - do_ioctl_err(fd, DRM_IOCTL_MODE_CREATEPROPBLOB, , EFAULT); + do_ioctl_err(fd, LOCAL_DRM_IOCTL_MODE_CREATEPROPBLOB, , EFAULT); /* Outlandish address. */ create.length = sizeof(test_mode_valid); create.data = (uintptr_t) 0xdeadbeee; - do_ioctl_err(fd, DRM_IOCTL_MODE_CREATEPROPBLOB, , EFAULT); + do_ioctl_err(fd, LOCAL_DRM_IOCTL_MODE_CREATEPROPBLOB, , EFAULT); /* When we pass an incorrect size, the kernel should correct us. */ prop_id =
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly
On Wed, Oct 14, 2015 at 04:51:06PM +0200, Daniel Vetter wrote: > The rotated view depends upon the rotation paramters, but thus far we > didn't bother checking for those. This seems to have been an issue > ever since this was introduce in > > commit fe14d5f4e5468c5b80a24f1a64abcbe116143670 > Author: Tvrtko Ursulin> Date: Wed Dec 10 17:27:58 2014 + > > drm/i915: Infrastructure for supporting different GGTT views per object > > But userspace is allowed to reuse framebuffer backing storage with > different framebuffers with different pixel formats/stride/whatever. > And e.g. SNA indeed does this. Hence we must check for all the > paramters to match, not just that it's rotated. > > v2: intel_plane_obj_offset also needs to construct the full view, to > avoid fallout since they don't fully match. > > Cc: Tvrtko Ursulin > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/i915_gem_gtt.h | 2 +- > drivers/gpu/drm/i915/intel_display.c | 10 +- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h > b/drivers/gpu/drm/i915/i915_gem_gtt.h > index 2e1f6493c9e7..8a36f4fcc676 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -551,7 +551,7 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, > > if (a->type != b->type) > return false; > - if (a->type == I915_GGTT_VIEW_PARTIAL) > + if (a->type != I915_GGTT_VIEW_NORMAL) > return !memcmp(>params, >params, sizeof(a->params)); Again I have basically the same patch, except in mine the union is anoymous so mine needs to handle partial and rotated sepeartely. > return true; > } > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 57459fedf216..2a5987ce576c 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2894,16 +2894,16 @@ unsigned long intel_plane_obj_offset(struct > intel_plane *intel_plane, >struct drm_i915_gem_object *obj, >unsigned int plane) > { > - const struct i915_ggtt_view *view = _ggtt_view_normal; > + struct i915_ggtt_view view; > struct i915_vma *vma; > unsigned char *offset; > > - if (intel_rotation_90_or_270(intel_plane->base.state->rotation)) > - view = _ggtt_view_rotated; > + intel_fill_fb_ggtt_view(, intel_plane->base.fb, > + intel_plane->base.state); > > - vma = i915_gem_obj_to_ggtt_view(obj, view); > + vma = i915_gem_obj_to_ggtt_view(obj, ); > if (WARN(!vma, "ggtt vma for display object not found! (view=%u)\n", > - view->type)) > + view.type)) > return -1; > > offset = (unsigned char *)vma->node.start; > -- > 2.5.1 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] core_prop_blob ioctl_wrappers: Fix new tests/benchmarks for android
>Hi, > >On 13 October 2015 at 16:35, Daniel Vetterwrote: >>> +void igt_require_propblob(int fd) >>> +{ >>> + struct local_drm_mode_create_blob c; >>> + struct local_drm_mode_destroy_blob d; >>> + uint32_t blob_data; >>> + c.data = _data; >>> + c.length = sizeof(blob_data); >>> + >>> + igt_require(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_CREATEPROPBLOB, ) == >>> 0); >>> + d.blob_id = c.blob_id; >>> + igt_require(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_DESTROYPROPBLOB, >>> +) == 0); } >> >> If you want to do this in the library the usual way is to wrap the >> ioctls as functions and put the relevant -ENOTTY check in there as an >> igt_require, followed by an igt_assert for anything else that might >> have gone wrong. >> >> I'd just keep this in the test as a static function though, since then >> you don't have to write api docs ;-) > >I have no real opinion either way; apologies for breaking the build! >Is Android up-to-date with libdrm, or? If it's not, I suspect kms_atomic.c >should just be excluded from the Android build altogether. Hi, The kms_* tests are dependant on cairo which is not in the android build by default. Personally I have never tried getting cairo to run on android so have never built the kms_* tests for android. The version of libdrm used in android will match the kernel version being used in the particular android project. (So it would not be the latest version that goes with drm-nightly) You aren't the 1st person to break the android build (and won't be the last either) :-) //Derek > >Cheers, >Daniel > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 19/22] drm/i915: Don't leak framebuffer_references if drm_framebuffer_init() fails
From: Ville SyrjäläDon't increment obj->framebuffer_references until we know we actually managed to create the framebuffer. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 70e6e27..0cfedf9 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14453,7 +14453,6 @@ static int intel_framebuffer_init(struct drm_device *dev, drm_helper_mode_fill_fb_struct(_fb->base, mode_cmd); intel_fb->obj = obj; - intel_fb->obj->framebuffer_references++; ret = drm_framebuffer_init(dev, _fb->base, _fb_funcs); if (ret) { @@ -14461,6 +14460,8 @@ static int intel_framebuffer_init(struct drm_device *dev, return ret; } + intel_fb->obj->framebuffer_references++; + return 0; } -- 2.4.9 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 09/22] drm/i915: Refactor intel_surf_alignment()
From: Ville SyrjäläPull the code to determine the surface alignment for both linear and tiled surfaces into a separate function intel_surf_alignment(). This will be used not only for the vma alignment but actually aligning the plane SURF once SKL+ starts using intel_compute_page_offset() (since SKL+ needs >4K alignment for tiled surfaces too). Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 45 +--- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index bd55d06..5f3abce 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2321,7 +2321,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, return 0; } -static unsigned int intel_linear_alignment(struct drm_i915_private *dev_priv) +static unsigned int intel_linear_alignment(const struct drm_i915_private *dev_priv) { if (INTEL_INFO(dev_priv)->gen >= 9) return 256 * 1024; @@ -2334,6 +2334,25 @@ static unsigned int intel_linear_alignment(struct drm_i915_private *dev_priv) return 0; } +static unsigned int intel_surf_alignment(const struct drm_i915_private *dev_priv, +uint64_t fb_modifier) +{ + switch (fb_modifier) { + case DRM_FORMAT_MOD_NONE: + return intel_linear_alignment(dev_priv); + case I915_FORMAT_MOD_X_TILED: + if (INTEL_INFO(dev_priv)->gen >= 9) + return 256 * 1024; + return 0; + case I915_FORMAT_MOD_Y_TILED: + case I915_FORMAT_MOD_Yf_TILED: + return 1 * 1024 * 1024; + default: + MISSING_CASE(fb_modifier); + return 0; + } +} + int intel_pin_and_fence_fb_obj(struct drm_plane *plane, struct drm_framebuffer *fb, @@ -2350,29 +2369,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, WARN_ON(!mutex_is_locked(>struct_mutex)); - switch (fb->modifier[0]) { - case DRM_FORMAT_MOD_NONE: - alignment = intel_linear_alignment(dev_priv); - break; - case I915_FORMAT_MOD_X_TILED: - if (INTEL_INFO(dev)->gen >= 9) - alignment = 256 * 1024; - else { - /* pin() will align the object as required by fence */ - alignment = 0; - } - break; - case I915_FORMAT_MOD_Y_TILED: - case I915_FORMAT_MOD_Yf_TILED: - if (WARN_ONCE(INTEL_INFO(dev)->gen < 9, - "Y tiling bo slipped through, driver bug!\n")) - return -EINVAL; - alignment = 1 * 1024 * 1024; - break; - default: - MISSING_CASE(fb->modifier[0]); - return -EINVAL; - } + alignment = intel_surf_alignment(dev_priv, fb->modifier[0]); ret = intel_fill_fb_ggtt_view(, fb, plane_state); if (ret) -- 2.4.9 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 21/22] drm/i915: Rewrite fb rotation GTT handling
From: Ville SyrjäläRedo the fb rotation handling in order to: - eliminate the NV12 special casing - handle fb->offsets[] properly - make the rotation handling reasier for the plane code To achieve these goals we reduce intel_rotation_info to only contain (for each plane) the rotated view width,height,stride in tile units, and the page offset into the object where the plane starts. Each plane is handled exactly the same way, no special casing for NV12 or other formats. We then store the computed rotation_info under intel_framebuffer so that we don't have to recompute it again. To handle fb->offsets[] we treat them as a linear offsets and convert them to x/y offsets from the start of the relevant GTT mapping (either normal or rotated). We store the x/y offsets under intel_framebuffer, and for some extra convenience we also store the rotated pitch (ie. tile aligned plane height). So for each plane we have the normal x/y offsets, rotated x/y offsets, and the rotated pitch. The normal pitch is available already in fb->pitches[]. While we're gathering up all that extra information, we can also easily compute the storage requirements for the framebuffer, so that we can check that the object is big enough to hold it. When it comes time to deal with the plane source coordinates, we first rotate the clipped src coordinates to match the relevant GTT view orientation, then add to them the fb x/y offsets. Next we compute the aligned surface page offset, and as a result we're left with some residual x/y offsets. Finally, if required by the hardware, we convert the remaining x/y offsets into a linear offset. For gen2/3 we simply skip computing the final page offset, and just convert the src+fb x/y offsets directly into a linear offset since that's what the hardware wants. After this all platforms, incluing SKL+, compute these things in exactly the same way (excluding alignemnt differences). Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_gem_gtt.c | 66 ++--- drivers/gpu/drm/i915/i915_gem_gtt.h | 14 +- drivers/gpu/drm/i915/intel_display.c | 452 --- drivers/gpu/drm/i915/intel_drv.h | 32 ++- drivers/gpu/drm/i915/intel_sprite.c | 104 5 files changed, 407 insertions(+), 261 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index bb95b86..98aee6c 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3255,11 +3255,6 @@ rotate_pages(const dma_addr_t *in, unsigned int offset, unsigned int column, row; unsigned int src_idx; - if (!sg) { - st->nents = 0; - sg = st->sgl; - } - for (column = 0; column < width; column++) { src_idx = stride * (height - 1) + column; for (row = 0; row < height; row++) { @@ -3280,16 +3275,14 @@ rotate_pages(const dma_addr_t *in, unsigned int offset, } static struct sg_table * -intel_rotate_fb_obj_pages(struct intel_rotation_info *rot_info, +intel_rotate_fb_obj_pages(const struct intel_rotation_info *info, struct drm_i915_gem_object *obj) { - unsigned int size_pages = rot_info->size >> PAGE_SHIFT; - unsigned int size_pages_uv; + unsigned int size = intel_rotation_info_size(info); struct sg_page_iter sg_iter; unsigned long i; dma_addr_t *page_addr_list; struct sg_table *st; - unsigned int uv_start_page; struct scatterlist *sg; int ret = -ENOMEM; @@ -3299,57 +3292,32 @@ intel_rotate_fb_obj_pages(struct intel_rotation_info *rot_info, if (!page_addr_list) return ERR_PTR(ret); - /* Account for UV plane with NV12. */ - if (rot_info->pixel_format == DRM_FORMAT_NV12) - size_pages_uv = rot_info->size_uv >> PAGE_SHIFT; - else - size_pages_uv = 0; - /* Allocate target SG list. */ st = kmalloc(sizeof(*st), GFP_KERNEL); if (!st) goto err_st_alloc; - ret = sg_alloc_table(st, size_pages + size_pages_uv, GFP_KERNEL); + ret = sg_alloc_table(st, size, GFP_KERNEL); if (ret) goto err_sg_alloc; /* Populate source page list from the object. */ i = 0; for_each_sg_page(obj->pages->sgl, _iter, obj->pages->nents, 0) { - page_addr_list[i] = sg_page_iter_dma_address(_iter); - i++; + page_addr_list[i++] = sg_page_iter_dma_address(_iter); } - /* Rotate the pages. */ - sg = rotate_pages(page_addr_list, 0, -rot_info->width_pages, rot_info->height_pages, -rot_info->width_pages, -st, NULL); - - /* Append the UV plane if NV12. */ - if (rot_info->pixel_format == DRM_FORMAT_NV12) { -
[Intel-gfx] [PATCH 20/22] drm/i915: Pass drm_frambuffer to intel_compute_page_offset()
From: Ville Syrjäläintel_compute_page_offsets() gets passed a bunch of the framebuffer metadate sepearately. Just pass the framebuffer itself to make life simpler for the caller, and make it less likely they would make a mistake in the order of the arguments (as most as just unsigned ints and such). We still pass the pitch explicitly since for 90/270 degree rotation the caller has to pass in the right thing. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 15 +++ drivers/gpu/drm/i915/intel_drv.h | 6 ++ drivers/gpu/drm/i915/intel_sprite.c | 9 +++-- 3 files changed, 12 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 0cfedf9..028dc4a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2515,13 +2515,14 @@ static void intel_adjust_page_offset(int *x, int *y, * to be already rotated to match the rotated GTT view, and * pitch is the tile_height aligned framebuffer height. */ -unsigned long intel_compute_page_offset(struct drm_i915_private *dev_priv, - int *x, int *y, - uint64_t fb_modifier, - unsigned int cpp, +unsigned long intel_compute_page_offset(int *x, int *y, + const struct drm_framebuffer *fb, int plane, unsigned int pitch, unsigned int rotation) { + const struct drm_i915_private *dev_priv = to_i915(fb->dev); + uint64_t fb_modifier = fb->modifier[plane]; + unsigned int cpp = drm_format_plane_cpp(fb->pixel_format, plane); unsigned int offset, alignment; alignment = intel_surf_alignment(dev_priv, fb_modifier); @@ -2842,8 +2843,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, if (INTEL_INFO(dev)->gen >= 4) { intel_crtc->dspaddr_offset = - intel_compute_page_offset(dev_priv, , , - fb->modifier[0], pixel_size, + intel_compute_page_offset(, , fb, 0, fb->pitches[0], rotation); linear_offset -= intel_crtc->dspaddr_offset; } else { @@ -2948,8 +2948,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, linear_offset = y * fb->pitches[0] + x * pixel_size; intel_crtc->dspaddr_offset = - intel_compute_page_offset(dev_priv, , , - fb->modifier[0], pixel_size, + intel_compute_page_offset(, , fb, 0, fb->pitches[0], rotation); linear_offset -= intel_crtc->dspaddr_offset; if (rotation == BIT(DRM_ROTATE_180)) { diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index b0d92e0..36d049d 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1134,10 +1134,8 @@ void assert_fdi_rx_pll(struct drm_i915_private *dev_priv, void assert_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, bool state); #define assert_pipe_enabled(d, p) assert_pipe(d, p, true) #define assert_pipe_disabled(d, p) assert_pipe(d, p, false) -unsigned long intel_compute_page_offset(struct drm_i915_private *dev_priv, - int *x, int *y, - uint64_t fb_modifier, - unsigned int cpp, +unsigned long intel_compute_page_offset(int *x, int *y, + const struct drm_framebuffer *fb, int plane, unsigned int pitch, unsigned int rotation); void intel_prepare_reset(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 8eaebce..828c3eb 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -421,8 +421,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, crtc_h--; linear_offset = y * fb->pitches[0] + x * pixel_size; - sprsurf_offset = intel_compute_page_offset(dev_priv, , , - fb->modifier[0], pixel_size, + sprsurf_offset = intel_compute_page_offset(, , fb, 0, fb->pitches[0], rotation); linear_offset -= sprsurf_offset; @@ -554,8 +553,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, sprscale = SPRITE_SCALE_ENABLE | (src_w << 16) | src_h; linear_offset = y * fb->pitches[0] + x * pixel_size; -
[Intel-gfx] [PATCH 22/22] drm/i915: Don't pass pitch to intel_compute_page_offset()
From: Ville Syrjäläintel_compute_page_offset() can dig up the correct pitch from the fb itself, no need for the caller to pass it in. A bit of extra care is needed for the lower level _intel_compute_page_offset() since that one gets called before the rotated pitch under intel_fb is populated. Note that we don't actually call it with anything but DRM_ROTATE_0 there so we wouldn't actually look up the rotated pitch there, but still, leave the pitch as something the caller has to pass to _intel_compute_page_offset() as an indicator that something is a bit special. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 32 +--- drivers/gpu/drm/i915/intel_drv.h | 1 - drivers/gpu/drm/i915/intel_sprite.c | 26 +++--- 3 files changed, 28 insertions(+), 31 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 8c7dc03..15dbe48 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2604,11 +2604,16 @@ static unsigned int _intel_compute_page_offset(const struct drm_i915_private *de unsigned int intel_compute_page_offset(int *x, int *y, const struct drm_framebuffer *fb, int plane, - unsigned int pitch, unsigned int rotation) { const struct drm_i915_private *dev_priv = to_i915(fb->dev); unsigned int alignment = intel_surf_alignment(dev_priv, fb->modifier[plane]); + unsigned int pitch; + + if (intel_rotation_90_or_270(rotation)) + pitch = to_intel_framebuffer(fb)->plane[plane].rotated.pitch; + else + pitch = fb->pitches[plane]; return _intel_compute_page_offset(dev_priv, x, y, fb, plane, pitch, rotation, alignment ? (alignment - 1) : 0); @@ -3026,8 +3031,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, if (INTEL_INFO(dev)->gen >= 4) intel_crtc->dspaddr_offset = - intel_compute_page_offset(, , fb, 0, - fb->pitches[0], rotation); + intel_compute_page_offset(, , fb, 0, rotation); if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180)) { dspcntr |= DISPPLANE_ROTATE_180; @@ -3129,8 +3133,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, intel_add_fb_offsets(, , fb, 0, rotation); intel_crtc->dspaddr_offset = - intel_compute_page_offset(, , fb, 0, - fb->pitches[0], rotation); + intel_compute_page_offset(, , fb, 0, rotation); if (rotation == BIT(DRM_ROTATE_180)) { dspcntr |= DISPPLANE_ROTATE_180; @@ -3308,7 +3311,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, struct intel_framebuffer *intel_fb; bool visible = to_intel_plane_state(plane->state)->visible; int pipe = intel_crtc->pipe; - u32 plane_ctl, stride_div, stride; + u32 plane_ctl, stride; unsigned int rotation; unsigned long surf_addr; struct intel_crtc_state *crtc_state = intel_crtc->config; @@ -3365,18 +3368,17 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, src_w = drm_rect_width(); src_h = drm_rect_height(); - stride_div = intel_tile_height(dev_priv, fb->modifier[0], - pixel_size); - stride = intel_fb->plane[0].rotated.pitch; + stride = intel_fb->plane[0].rotated.pitch / + intel_tile_height(dev_priv, fb->modifier[0], + pixel_size); } else { - stride_div = intel_fb_stride_alignment(dev_priv, fb->modifier[0], - fb->pixel_format); - stride = fb->pitches[0]; + stride = fb->pitches[0] / + intel_fb_stride_alignment(dev_priv, fb->modifier[0], + fb->pixel_format); } intel_add_fb_offsets(, , fb, 0, rotation); - surf_addr = intel_compute_page_offset(, , fb, 0, - stride, rotation); + surf_addr = intel_compute_page_offset(, , fb, 0, rotation); /* Sizes are 0 based */ src_w--; @@ -3389,7 +3391,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl); I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x); - I915_WRITE(PLANE_STRIDE(pipe, 0), stride / stride_div); + I915_WRITE(PLANE_STRIDE(pipe, 0),
[Intel-gfx] [PATCH 16/22] drm/i915: Pass stride to rotate_pages()
From: Ville SyrjäläPass stride in addition to width and height to rotate_pages(). For now width and stride are the same, but once framebuffer offsets enter the scene that may no longer be the case. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_gem_gtt.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 365a8c6..73ca67d 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3249,6 +3249,7 @@ i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj, static struct scatterlist * rotate_pages(const dma_addr_t *in, unsigned int offset, unsigned int width, unsigned int height, +unsigned int stride, struct sg_table *st, struct scatterlist *sg) { unsigned int column, row; @@ -3260,7 +3261,7 @@ rotate_pages(const dma_addr_t *in, unsigned int offset, } for (column = 0; column < width; column++) { - src_idx = width * (height - 1) + column; + src_idx = stride * (height - 1) + column; for (row = 0; row < height; row++) { st->nents++; /* We don't need the pages, but need to initialize @@ -3271,7 +3272,7 @@ rotate_pages(const dma_addr_t *in, unsigned int offset, sg_dma_address(sg) = in[offset + src_idx]; sg_dma_len(sg) = PAGE_SIZE; sg = sg_next(sg); - src_idx -= width; + src_idx -= stride; } } @@ -3324,6 +3325,7 @@ intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view, /* Rotate the pages. */ sg = rotate_pages(page_addr_list, 0, rot_info->width_pages, rot_info->height_pages, +rot_info->width_pages, st, NULL); /* Append the UV plane if NV12. */ @@ -3339,6 +3341,7 @@ intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view, rotate_pages(page_addr_list, uv_start_page, rot_info->width_pages_uv, rot_info->height_pages_uv, +rot_info->width_pages_uv, st, sg); } -- 2.4.9 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 15/22] drm/i915: Pass the dma_addr_t array as const to rotate_pages()
From: Ville Syrjälärotate_pages() doesn't modify the passed in dma addresses, so make them const. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index e0baff2..365a8c6 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3247,7 +3247,7 @@ i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj, } static struct scatterlist * -rotate_pages(dma_addr_t *in, unsigned int offset, +rotate_pages(const dma_addr_t *in, unsigned int offset, unsigned int width, unsigned int height, struct sg_table *st, struct scatterlist *sg) { -- 2.4.9 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/22] drm/i915: Redo intel_tile_height() as intel_tile_size() / intel_tile_width()
From: Ville SyrjäläI find more usual to think about tile widths than heights, so changing the intel_tile_height() to calculate the tile height as tile_size/tile_width is easier than the opposite to the poor brain. v2: Reorder arguments for consistency Constify dev_priv arguments Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 88 drivers/gpu/drm/i915/intel_drv.h | 5 +- drivers/gpu/drm/i915/intel_sprite.c | 4 +- 3 files changed, 34 insertions(+), 63 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index c31fe47..d028326 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2214,6 +2214,11 @@ static bool need_vtd_wa(struct drm_device *dev) return false; } +static unsigned int intel_tile_size(const struct drm_i915_private *dev_priv) +{ + return IS_GEN2(dev_priv) ? 2048 : 4096; +} + static unsigned int intel_tile_width(const struct drm_i915_private *dev_priv, uint64_t fb_modifier, unsigned int cpp) { @@ -2249,67 +2254,34 @@ static unsigned int intel_tile_width(const struct drm_i915_private *dev_priv, } } -unsigned int -intel_tile_height(struct drm_device *dev, uint32_t pixel_format, - uint64_t fb_format_modifier, unsigned int plane) +unsigned int intel_tile_height(const struct drm_i915_private *dev_priv, + uint64_t fb_modifier, unsigned int cpp) { - unsigned int tile_height; - uint32_t pixel_bytes; - - switch (fb_format_modifier) { - case DRM_FORMAT_MOD_NONE: - tile_height = 1; - break; - case I915_FORMAT_MOD_X_TILED: - tile_height = IS_GEN2(dev) ? 16 : 8; - break; - case I915_FORMAT_MOD_Y_TILED: - tile_height = 32; - break; - case I915_FORMAT_MOD_Yf_TILED: - pixel_bytes = drm_format_plane_cpp(pixel_format, plane); - switch (pixel_bytes) { - default: - case 1: - tile_height = 64; - break; - case 2: - case 4: - tile_height = 32; - break; - case 8: - tile_height = 16; - break; - case 16: - WARN_ONCE(1, - "128-bit pixels are not supported for display!"); - tile_height = 16; - break; - } - break; - default: - MISSING_CASE(fb_format_modifier); - tile_height = 1; - break; - } - - return tile_height; + if (fb_modifier == DRM_FORMAT_MOD_NONE) + return 1; + else + return intel_tile_size(dev_priv) / + intel_tile_width(dev_priv, fb_modifier, cpp); } unsigned int intel_fb_align_height(struct drm_device *dev, unsigned int height, - uint32_t pixel_format, uint64_t fb_format_modifier) + uint32_t pixel_format, uint64_t fb_modifier) { - return ALIGN(height, intel_tile_height(dev, pixel_format, - fb_format_modifier, 0)); + unsigned int cpp = drm_format_plane_cpp(pixel_format, 0); + unsigned int tile_height = intel_tile_height(to_i915(dev), fb_modifier, cpp); + + return ALIGN(height, tile_height); } static int intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, const struct drm_plane_state *plane_state) { + struct drm_i915_private *dev_priv = to_i915(fb->dev); struct intel_rotation_info *info = >rotation_info; unsigned int tile_height, tile_pitch; + unsigned int cpp = drm_format_plane_cpp(fb->pixel_format, 0); *view = i915_ggtt_view_normal; @@ -2327,22 +2299,19 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, info->uv_offset = fb->offsets[1]; info->fb_modifier = fb->modifier[0]; - tile_height = intel_tile_height(fb->dev, fb->pixel_format, - fb->modifier[0], 0); + tile_height = intel_tile_height(dev_priv, fb->modifier[0], cpp); tile_pitch = PAGE_SIZE / tile_height; info->width_pages = DIV_ROUND_UP(fb->pitches[0], tile_pitch); info->height_pages = DIV_ROUND_UP(fb->height, tile_height); info->size = info->width_pages * info->height_pages * PAGE_SIZE; if (info->pixel_format == DRM_FORMAT_NV12) { - tile_height = intel_tile_height(fb->dev, fb->pixel_format, -
[Intel-gfx] [PATCH 14/22] drm/i915: Don't treat differently sized rotated views as equal
From: Ville SyrjäläIn case we have multiple different rotated views into the same object, each one may need its own vma due to being of different sizes. So don't treat all rotated views as equal. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index caa182f..68de734 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -553,6 +553,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, if (a->type != b->type) return false; + if (a->type == I915_GGTT_VIEW_ROTATED) + return !memcmp(>rotated, >rotated, sizeof(a->rotated)); if (a->type == I915_GGTT_VIEW_PARTIAL) return !memcmp(>partial, >partial, sizeof(a->partial)); return true; -- 2.4.9 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 08/22] drm/i915: Pass 90/270 vs. 0/180 rotation info for intel_gen4_compute_page_offset()
From: Ville SyrjäläThe page aligned surface address calculation needs to know which way things are rotated. The contract now says that the caller must pass the rotate x/y coordinates, as well as the tile_height aligned stride in the tile_height direction. This will make it fairly simple to deal with 90/270 degree rotation on SKL+ where we have to deal with the rotated view into the GTT. v2: Pass rotation instead of bool even thoughwe only care about 0/180 vs. 90/270 Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 62 ++-- drivers/gpu/drm/i915/intel_drv.h | 3 +- drivers/gpu/drm/i915/intel_sprite.c | 15 + 3 files changed, 64 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 75be66b..bd55d06 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2451,13 +2451,50 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb, i915_gem_object_unpin_from_display_plane(obj, ); } -/* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel - * is assumed to be a power-of-two. */ +/* + * Return the tile dimensions in pixel units matching + * the specified rotation angle. + */ +static void intel_rotate_tile_dims(unsigned int *tile_width, + unsigned int *tile_height, + unsigned int *pitch, + unsigned int cpp, + unsigned int rotation) +{ + if (intel_rotation_90_or_270(rotation)) { + WARN_ON(*pitch % *tile_height); + + /* pixel units please */ + *tile_width /= cpp; + + /* +* Coordinate space is rotated, orient +* our tile dimensions the same way +*/ + swap(*tile_width, *tile_height); + } else { + WARN_ON(*pitch % *tile_width); + + /* pixel units please */ + *tile_width /= cpp; + *pitch /= cpp; + } +} + +/* + * Computes the linear offset to the base tile and adjusts + * x, y. bytes per pixel is assumed to be a power-of-two. + * + * In the 90/270 rotated case, x and y are assumed + * to be already rotated to match the rotated GTT view, and + * pitch is the tile_height aligned framebuffer height. + */ unsigned long intel_compute_page_offset(struct drm_i915_private *dev_priv, int *x, int *y, uint64_t fb_modifier, unsigned int cpp, - unsigned int pitch) + unsigned int pitch, + unsigned int rotation) { if (fb_modifier != DRM_FORMAT_MOD_NONE) { unsigned int tile_size, tile_width, tile_height; @@ -2467,13 +2504,16 @@ unsigned long intel_compute_page_offset(struct drm_i915_private *dev_priv, tile_width = intel_tile_width(dev_priv, fb_modifier, cpp); tile_height = tile_size / tile_width; + intel_rotate_tile_dims(_width, _height, + , cpp, rotation); + tile_rows = *y / tile_height; *y %= tile_height; - tiles = *x / (tile_width/cpp); - *x %= tile_width/cpp; + tiles = *x / tile_width; + *x %= tile_width; - return tile_rows * pitch * tile_height + tiles * tile_size; + return (tile_rows * (pitch / tile_width) + tiles) * tile_size; } else { unsigned int alignment = intel_linear_alignment(dev_priv) - 1; unsigned int offset; @@ -2685,6 +2725,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, bool visible = to_intel_plane_state(primary->state)->visible; struct drm_i915_gem_object *obj; int plane = intel_crtc->plane; + unsigned int rotation; unsigned long linear_offset; u32 dspcntr; u32 reg = DSPCNTR(plane); @@ -2704,6 +2745,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, if (WARN_ON(obj == NULL)) return; + rotation = crtc->primary->state->rotation; pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); dspcntr = DISPPLANE_GAMMA_ENABLE; @@ -2768,7 +2810,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, intel_crtc->dspaddr_offset = intel_compute_page_offset(dev_priv, , , fb->modifier[0], pixel_size, - fb->pitches[0]); +
[Intel-gfx] [PATCH 07/22] drm/i915: s/intel_gen4_compute_page_offset/intel_compute_page_offset/
From: Ville SyrjäläSince intel_gen4_compute_page_offset() can now handle tiling formats all the way down to gen2, rename it to intel_compute_page_offset(). Not that we actually use it on gen2/3 since there's no DSPSURF etc. registers which would take a page aligned address. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 24 +++- drivers/gpu/drm/i915/intel_drv.h | 10 +- drivers/gpu/drm/i915/intel_sprite.c | 22 +- 3 files changed, 25 insertions(+), 31 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 509fcac..75be66b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2453,11 +2453,11 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb, /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel * is assumed to be a power-of-two. */ -unsigned long intel_gen4_compute_page_offset(struct drm_i915_private *dev_priv, -int *x, int *y, -uint64_t fb_modifier, -unsigned int cpp, -unsigned int pitch) +unsigned long intel_compute_page_offset(struct drm_i915_private *dev_priv, + int *x, int *y, + uint64_t fb_modifier, + unsigned int cpp, + unsigned int pitch) { if (fb_modifier != DRM_FORMAT_MOD_NONE) { unsigned int tile_size, tile_width, tile_height; @@ -2766,10 +2766,9 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, if (INTEL_INFO(dev)->gen >= 4) { intel_crtc->dspaddr_offset = - intel_gen4_compute_page_offset(dev_priv, - , , fb->modifier[0], - pixel_size, - fb->pitches[0]); + intel_compute_page_offset(dev_priv, , , + fb->modifier[0], pixel_size, + fb->pitches[0]); linear_offset -= intel_crtc->dspaddr_offset; } else { intel_crtc->dspaddr_offset = linear_offset; @@ -2871,10 +2870,9 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, linear_offset = y * fb->pitches[0] + x * pixel_size; intel_crtc->dspaddr_offset = - intel_gen4_compute_page_offset(dev_priv, - , , fb->modifier[0], - pixel_size, - fb->pitches[0]); + intel_compute_page_offset(dev_priv, , , + fb->modifier[0], pixel_size, + fb->pitches[0]); linear_offset -= intel_crtc->dspaddr_offset; if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180)) { dspcntr |= DISPPLANE_ROTATE_180; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 429f744..a12ac95 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1135,11 +1135,11 @@ void assert_fdi_rx_pll(struct drm_i915_private *dev_priv, void assert_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, bool state); #define assert_pipe_enabled(d, p) assert_pipe(d, p, true) #define assert_pipe_disabled(d, p) assert_pipe(d, p, false) -unsigned long intel_gen4_compute_page_offset(struct drm_i915_private *dev_priv, -int *x, int *y, -uint64_t fb_modifier, -unsigned int cpp, -unsigned int pitch); +unsigned long intel_compute_page_offset(struct drm_i915_private *dev_priv, + int *x, int *y, + uint64_t fb_modifier, + unsigned int cpp, + unsigned int pitch); void intel_prepare_reset(struct drm_device *dev); void intel_finish_reset(struct drm_device *dev); void hsw_enable_pc8(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 4a1a5f4..6614adb 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -420,11 +420,9 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, crtc_h--;
[Intel-gfx] [PATCH 00/22] drm/i915: Handle fb->offsets[] and rewrite fb rotation handling to be more generic
From: Ville SyrjäläSo while reviewing the NV12 stuff it became clear to me no one had really given fb->offsets[] handling any serious thought. So this patch series aims to fix that. We now treat fb->offsets[] as a linear offset always. One clear benefit over treating it as a linear offset as opposed to a raw byte offset is that we don't have to think about the layout of bytes within the tile at all. The series also generalizes the page rotation to be format agnostic, the caller just specifies the desired geometry in pages for each plane, and the rotation code builds up the sg. The intel_rotation_info then just contains the minimal amount of information needed to do the page rotation. SKL+ also gets changed to use the compute_page_offset stuff so that the plane SURF register will contain the closes (properly aligned) page boundary, and the x/y offsets deal with whatever is left over. The plane code for the other platforms also gets simpler in the end I think. Also the 90/270 rotation handling becomes rather trivial for the plane code. I should still write some decent tests to exercise fb->offsets[]. Series available here: git://github.com/vsyrjala/linux.git fb_offsets Ville Syrjälä (22): drm: Add drm_format_plane_width() and drm_format_plane_height() drm/i915: Pass modifier instead of tiling_mode to gen4_compute_page_offset() drm/i915: Factor out intel_tile_width() drm/i915: Redo intel_tile_height() as intel_tile_size() / intel_tile_width() drm/i915: change intel_fill_fb_ggtt_view() to use the real tile size drm/i915: Use intel_tile_{size,width,height}() in intel_gen4_compute_page_offset() drm/i915: s/intel_gen4_compute_page_offset/intel_compute_page_offset/ drm/i915: Pass 90/270 vs. 0/180 rotation info for intel_gen4_compute_page_offset() drm/i915: Refactor intel_surf_alignment() drm/i915: Support for extra alignment for tiled surfaces drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj() drm/i915: Set i915_ggtt_view_normal type explicitly drm/i915: Move the partial and rotated view data into the same union drm/i915: Don't treat differently sized rotated views as equal drm/i915: Pass the dma_addr_t array as const to rotate_pages() drm/i915: Pass stride to rotate_pages() drm/i915: Pass rotation_info to intel_rotate_fb_obj_pages() drm/i915: Make sure fb offset is (macro)pixel aligned drm/i915: Don't leak framebuffer_references if drm_framebuffer_init() fails drm/i915: Pass drm_frambuffer to intel_compute_page_offset() drm/i915: Rewrite fb rotation GTT handling drm/i915: Don't pass pitch to intel_compute_page_offset() drivers/gpu/drm/i915/i915_gem.c | 10 +- drivers/gpu/drm/i915/i915_gem_gtt.c | 88 ++-- drivers/gpu/drm/i915/i915_gem_gtt.h | 27 +- drivers/gpu/drm/i915/intel_display.c | 792 +++ drivers/gpu/drm/i915/intel_drv.h | 46 +- drivers/gpu/drm/i915/intel_fbdev.c | 2 +- drivers/gpu/drm/i915/intel_sprite.c | 128 +++--- include/drm/drm_crtc.h | 12 + 8 files changed, 681 insertions(+), 424 deletions(-) -- 2.4.9 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 03/22] drm/i915: Factor out intel_tile_width()
From: Ville SyrjäläPull the tile width calculations from intel_fb_stride_alignment() into a new function intel_tile_width(). Also take the opportunity to pass aroun dev_priv instead of dev to intel_fb_stride_alignment(). v2: Reorder argumnents to be more consistent with other functions Change intel_fb_stride_alignment() to accept dev_priv instead of dev Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 76 +++- drivers/gpu/drm/i915/intel_drv.h | 4 +- drivers/gpu/drm/i915/intel_sprite.c | 2 +- 3 files changed, 51 insertions(+), 31 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 6add8d1..c31fe47 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2214,6 +2214,41 @@ static bool need_vtd_wa(struct drm_device *dev) return false; } +static unsigned int intel_tile_width(const struct drm_i915_private *dev_priv, +uint64_t fb_modifier, unsigned int cpp) +{ + switch (fb_modifier) { + case DRM_FORMAT_MOD_NONE: + return cpp; + case I915_FORMAT_MOD_X_TILED: + return IS_GEN2(dev_priv) ? 128 : 512; + case I915_FORMAT_MOD_Y_TILED: + /* No need to check for old gens and Y tiling since this is +* about the display engine and those will be blocked before +* we get here. +*/ + return 128; + case I915_FORMAT_MOD_Yf_TILED: + switch (cpp) { + case 1: + return 64; + case 2: + case 4: + return 128; + case 8: + case 16: + return 256; + default: + MISSING_CASE(cpp); + return cpp; + } + break; + default: + MISSING_CASE(fb_modifier); + return cpp; + } +} + unsigned int intel_tile_height(struct drm_device *dev, uint32_t pixel_format, uint64_t fb_format_modifier, unsigned int plane) @@ -2895,37 +2930,20 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, POSTING_READ(reg); } -u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier, - uint32_t pixel_format) +u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv, + uint64_t fb_modifier, uint32_t pixel_format) { - u32 bits_per_pixel = drm_format_plane_cpp(pixel_format, 0) * 8; - /* * The stride is either expressed as a multiple of 64 bytes * chunks for linear buffers or in number of tiles for tiled * buffers. */ - switch (fb_modifier) { - case DRM_FORMAT_MOD_NONE: - return 64; - case I915_FORMAT_MOD_X_TILED: - if (INTEL_INFO(dev)->gen == 2) - return 128; - return 512; - case I915_FORMAT_MOD_Y_TILED: - /* No need to check for old gens and Y tiling since this is -* about the display engine and those will be blocked before -* we get here. -*/ - return 128; - case I915_FORMAT_MOD_Yf_TILED: - if (bits_per_pixel == 8) - return 64; - else - return 128; - default: - MISSING_CASE(fb_modifier); + if (fb_modifier == DRM_FORMAT_MOD_NONE) { return 64; + } else { + unsigned int cpp = drm_format_plane_cpp(pixel_format, 0); + + return intel_tile_width(dev_priv, fb_modifier, cpp); } } @@ -3106,7 +3124,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, plane_ctl |= skl_plane_ctl_rotation(rotation); obj = intel_fb_obj(fb); - stride_div = intel_fb_stride_alignment(dev, fb->modifier[0], + stride_div = intel_fb_stride_alignment(dev_priv, fb->modifier[0], fb->pixel_format); surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj, 0); @@ -9066,7 +9084,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc, fb->width = ((val >> 0) & 0x1fff) + 1; val = I915_READ(PLANE_STRIDE(pipe, 0)); - stride_mult = intel_fb_stride_alignment(dev, fb->modifier[0], + stride_mult = intel_fb_stride_alignment(dev_priv, fb->modifier[0], fb->pixel_format); fb->pitches[0] = (val & 0x3ff) * stride_mult; @@ -11137,7 +11155,7 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc, * linear buffers or in number of tiles for
[Intel-gfx] [PATCH 06/22] drm/i915: Use intel_tile_{size, width, height}() in intel_gen4_compute_page_offset()
From: Ville SyrjäläMake intel_gen4_compute_page_offset() ready for other tiling formats besied X-tile by getting the tile dimensions through intel_tile_{size,width,height}(). Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 15 ++- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index c8b2907..509fcac 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2460,15 +2460,20 @@ unsigned long intel_gen4_compute_page_offset(struct drm_i915_private *dev_priv, unsigned int pitch) { if (fb_modifier != DRM_FORMAT_MOD_NONE) { + unsigned int tile_size, tile_width, tile_height; unsigned int tile_rows, tiles; - tile_rows = *y / 8; - *y %= 8; + tile_size = intel_tile_size(dev_priv); + tile_width = intel_tile_width(dev_priv, fb_modifier, cpp); + tile_height = tile_size / tile_width; + + tile_rows = *y / tile_height; + *y %= tile_height; - tiles = *x / (512/cpp); - *x %= 512/cpp; + tiles = *x / (tile_width/cpp); + *x %= tile_width/cpp; - return tile_rows * pitch * 8 + tiles * 4096; + return tile_rows * pitch * tile_height + tiles * tile_size; } else { unsigned int alignment = intel_linear_alignment(dev_priv) - 1; unsigned int offset; -- 2.4.9 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 05/22] drm/i915: change intel_fill_fb_ggtt_view() to use the real tile size
From: Ville SyrjäläUse the actual tile size as to compute stuff in intel_fill_fb_ggtt_view() instead of assuming it's PAGE_SIZE. I suppose it doesn't matter since we don't use the results on gen2 platforms where the tile size is 2k. v2: Update due to CbCr plane Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 24 ++-- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d028326..c8b2907 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2280,8 +2280,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, { struct drm_i915_private *dev_priv = to_i915(fb->dev); struct intel_rotation_info *info = >rotation_info; - unsigned int tile_height, tile_pitch; - unsigned int cpp = drm_format_plane_cpp(fb->pixel_format, 0); + unsigned int tile_size, tile_width, tile_height, cpp; *view = i915_ggtt_view_normal; @@ -2299,19 +2298,24 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, info->uv_offset = fb->offsets[1]; info->fb_modifier = fb->modifier[0]; - tile_height = intel_tile_height(dev_priv, fb->modifier[0], cpp); - tile_pitch = PAGE_SIZE / tile_height; - info->width_pages = DIV_ROUND_UP(fb->pitches[0], tile_pitch); + tile_size = intel_tile_size(dev_priv); + + cpp = drm_format_plane_cpp(fb->pixel_format, 0); + tile_width = intel_tile_width(dev_priv, cpp, fb->modifier[0]); + tile_height = tile_size / tile_width; + + info->width_pages = DIV_ROUND_UP(fb->pitches[0], tile_width); info->height_pages = DIV_ROUND_UP(fb->height, tile_height); - info->size = info->width_pages * info->height_pages * PAGE_SIZE; + info->size = info->width_pages * info->height_pages * tile_size; if (info->pixel_format == DRM_FORMAT_NV12) { cpp = drm_format_plane_cpp(fb->pixel_format, 1); - tile_height = intel_tile_height(dev_priv, fb->modifier[1], cpp); - tile_pitch = PAGE_SIZE / tile_height; - info->width_pages_uv = DIV_ROUND_UP(fb->pitches[1], tile_pitch); + tile_width = intel_tile_width(dev_priv, fb->modifier[1], cpp); + tile_height = tile_size / tile_width; + + info->width_pages_uv = DIV_ROUND_UP(fb->pitches[1], tile_width); info->height_pages_uv = DIV_ROUND_UP(fb->height / 2, tile_height); - info->size_uv = info->width_pages_uv * info->height_pages_uv * PAGE_SIZE; + info->size_uv = info->width_pages_uv * info->height_pages_uv * tile_size; } return 0; -- 2.4.9 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()
From: Ville Syrjäläintel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird rotation (to find the right GTT view for it), so no need to pass all kinds of plane stuff. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 39 drivers/gpu/drm/i915/intel_drv.h | 5 ++--- drivers/gpu/drm/i915/intel_fbdev.c | 2 +- 3 files changed, 20 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 85e1473..80e9f2e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2275,8 +2275,9 @@ intel_fb_align_height(struct drm_device *dev, unsigned int height, } static int -intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, - const struct drm_plane_state *plane_state) +intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, + const struct drm_framebuffer *fb, + unsigned int rotation) { struct drm_i915_private *dev_priv = to_i915(fb->dev); struct intel_rotation_info *info = >rotation_info; @@ -2284,10 +2285,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, *view = i915_ggtt_view_normal; - if (!plane_state) - return 0; - - if (!intel_rotation_90_or_270(plane_state->rotation)) + if (!intel_rotation_90_or_270(rotation)) return 0; *view = i915_ggtt_view_rotated; @@ -2354,9 +2352,8 @@ static unsigned int intel_surf_alignment(const struct drm_i915_private *dev_priv } int -intel_pin_and_fence_fb_obj(struct drm_plane *plane, - struct drm_framebuffer *fb, - const struct drm_plane_state *plane_state, +intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, + unsigned int rotation, struct intel_engine_cs *pipelined, struct drm_i915_gem_request **pipelined_request) { @@ -2371,7 +2368,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane, alignment = intel_surf_alignment(dev_priv, fb->modifier[0]); - ret = intel_fill_fb_ggtt_view(, fb, plane_state); + ret = intel_fill_fb_ggtt_view(, fb, rotation); if (ret) return ret; @@ -2432,8 +2429,7 @@ err_interruptible: return ret; } -static void intel_unpin_fb_obj(struct drm_framebuffer *fb, - const struct drm_plane_state *plane_state) +static void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation) { struct drm_i915_gem_object *obj = intel_fb_obj(fb); struct i915_ggtt_view view; @@ -2441,7 +2437,7 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb, WARN_ON(!mutex_is_locked(>base.dev->struct_mutex)); - ret = intel_fill_fb_ggtt_view(, fb, plane_state); + ret = intel_fill_fb_ggtt_view(, fb, rotation); WARN_ONCE(ret, "Couldn't get view from plane state!"); i915_gem_object_unpin_fence(obj); @@ -10780,7 +10776,7 @@ static void intel_unpin_work_fn(struct work_struct *__work) struct drm_plane *primary = crtc->base.primary; mutex_lock(>struct_mutex); - intel_unpin_fb_obj(work->old_fb, primary->state); + intel_unpin_fb_obj(work->old_fb, primary->state->rotation); drm_gem_object_unreference(>pending_flip_obj->base); if (work->flip_queued_req) @@ -11521,8 +11517,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, * synchronisation, so all we want here is to pin the framebuffer * into the display plane and skip any waits. */ - ret = intel_pin_and_fence_fb_obj(crtc->primary, fb, -crtc->primary->state, + ret = intel_pin_and_fence_fb_obj(fb, primary->state->rotation, mmio_flip ? i915_gem_request_get_ring(obj->last_write_req) : ring, ); if (ret) goto cleanup_pending; @@ -11573,7 +11568,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, return 0; cleanup_unpin: - intel_unpin_fb_obj(fb, crtc->primary->state); + intel_unpin_fb_obj(fb, crtc->primary->state->rotation); cleanup_pending: if (request) i915_gem_request_cancel(request); @@ -13457,7 +13452,8 @@ intel_prepare_plane_fb(struct drm_plane *plane, if (ret) DRM_DEBUG_KMS("failed to attach phys object\n"); } else { - ret = intel_pin_and_fence_fb_obj(plane, fb, new_state, NULL, NULL); + ret = intel_pin_and_fence_fb_obj(fb, new_state->rotation, +NULL, NULL); } if (ret == 0) @@ -13488,7 +13484,7
[Intel-gfx] [PATCH 13/22] drm/i915: Move the partial and rotated view data into the same union
From: Ville SyrjäläEach gtt view only has a single type, and so don't need to contain data for both rotated and partial views. So we can move the data into the same union to avoid wasting space. Also rename 'rotation_info' to 'rotated' to match the view type exactly, this should avoid confusion which union members is valid for each view type. Also make the union anonymous to avoid the .params. stuff all over. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_gem.c | 10 +- drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++-- drivers/gpu/drm/i915/i915_gem_gtt.h | 11 --- drivers/gpu/drm/i915/intel_display.c | 4 ++-- 4 files changed, 17 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index e57061a..b93ed18 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1772,12 +1772,12 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf) memset(, 0, sizeof(view)); view.type = I915_GGTT_VIEW_PARTIAL; - view.params.partial.offset = rounddown(page_offset, chunk_size); - view.params.partial.size = + view.partial.offset = rounddown(page_offset, chunk_size); + view.partial.size = min_t(unsigned int, chunk_size, (vma->vm_end - vma->vm_start)/PAGE_SIZE - - view.params.partial.offset); + view.partial.offset); } /* Now pin it into the GTT if needed */ @@ -1805,10 +1805,10 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf) * having accessed it before (at this partials' range). */ unsigned long base = vma->vm_start + -(view.params.partial.offset << PAGE_SHIFT); +(view.partial.offset << PAGE_SHIFT); unsigned int i; - for (i = 0; i < view.params.partial.size; i++) { + for (i = 0; i < view.partial.size; i++) { ret = vm_insert_pfn(vma, base + i * PAGE_SIZE, pfn + i); if (ret) break; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 71acc71..e0baff2 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3282,7 +3282,7 @@ static struct sg_table * intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view, struct drm_i915_gem_object *obj) { - struct intel_rotation_info *rot_info = _view->rotation_info; + struct intel_rotation_info *rot_info = _view->rotated; unsigned int size_pages = rot_info->size >> PAGE_SHIFT; unsigned int size_pages_uv; struct sg_page_iter sg_iter; @@ -3380,16 +3380,16 @@ intel_partial_pages(const struct i915_ggtt_view *view, if (!st) goto err_st_alloc; - ret = sg_alloc_table(st, view->params.partial.size, GFP_KERNEL); + ret = sg_alloc_table(st, view->partial.size, GFP_KERNEL); if (ret) goto err_sg_alloc; sg = st->sgl; st->nents = 0; for_each_sg_page(obj->pages->sgl, _sg_iter, obj->pages->nents, - view->params.partial.offset) + view->partial.offset) { - if (st->nents >= view->params.partial.size) + if (st->nents >= view->partial.size) break; sg_set_page(sg, NULL, PAGE_SIZE, 0); @@ -3514,9 +3514,9 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj, if (view->type == I915_GGTT_VIEW_NORMAL) { return obj->base.size; } else if (view->type == I915_GGTT_VIEW_ROTATED) { - return view->rotation_info.size; + return view->rotated.size; } else if (view->type == I915_GGTT_VIEW_PARTIAL) { - return view->params.partial.size << PAGE_SHIFT; + return view->partial.size << PAGE_SHIFT; } else { WARN_ONCE(1, "GGTT view %u not implemented!\n", view->type); return obj->base.size; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index a216397..caa182f 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -151,17 +151,14 @@ struct intel_rotation_info { struct i915_ggtt_view { enum i915_ggtt_view_type type; + struct sg_table *pages; + union { + struct intel_rotation_info rotated; struct { u64 offset; unsigned int size; } partial; - } params; -
[Intel-gfx] [PATCH 10/22] drm/i915: Support for extra alignment for tiled surfaces
From: Ville SyrjäläSKL+ needs >4K alignment for tiled surfaces, so make intel_compute_page_offset() handle it. The way we do it is first we compute the closest tile boundary as before, and then figure out how many tiles we need to go to reach the desired alignment. The difference in the offset is then added into the x/y offsets. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 51 1 file changed, 46 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5f3abce..85e1473 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2479,6 +2479,39 @@ static void intel_rotate_tile_dims(unsigned int *tile_width, } /* + * Adjust the page offset by moving the difference into + * the x/y offsets. + * + * Input tile dimensions and pitch must already be + * rotated to match x and y, and in pixel units. + * intel_rotate_tile_dims() gives exactly that. + */ +static void intel_adjust_page_offset(int *x, int *y, +unsigned int tile_width, +unsigned int tile_height, +unsigned int tile_size, +unsigned int pitch, +unsigned int old_offset, +unsigned int new_offset) +{ + unsigned int tiles; + + WARN_ON(old_offset & (tile_size - 1)); + WARN_ON(new_offset & (tile_size - 1)); + WARN_ON(new_offset > old_offset); + + tiles = (old_offset - new_offset) / tile_size; + if (tiles == 0) + return; + + /* pitch in tiles */ + pitch /= tile_width; + + *y += tiles / pitch * tile_height; + *x += tiles % pitch * tile_width; +} + +/* * Computes the linear offset to the base tile and adjusts * x, y. bytes per pixel is assumed to be a power-of-two. * @@ -2493,6 +2526,12 @@ unsigned long intel_compute_page_offset(struct drm_i915_private *dev_priv, unsigned int pitch, unsigned int rotation) { + unsigned int offset, alignment; + + alignment = intel_surf_alignment(dev_priv, fb_modifier); + if (alignment) + alignment--; + if (fb_modifier != DRM_FORMAT_MOD_NONE) { unsigned int tile_size, tile_width, tile_height; unsigned int tile_rows, tiles; @@ -2510,16 +2549,18 @@ unsigned long intel_compute_page_offset(struct drm_i915_private *dev_priv, tiles = *x / tile_width; *x %= tile_width; - return (tile_rows * (pitch / tile_width) + tiles) * tile_size; - } else { - unsigned int alignment = intel_linear_alignment(dev_priv) - 1; - unsigned int offset; + offset = (tile_rows * (pitch / tile_width) + tiles) * tile_size; + intel_adjust_page_offset(x, y, tile_width, tile_height, +tile_size, pitch, +offset, offset & ~alignment); + } else { offset = *y * pitch + *x * cpp; *y = (offset & alignment) / pitch; *x = ((offset & alignment) - *y * pitch) / cpp; - return offset & ~alignment; } + + return offset & ~alignment; } static int i9xx_format_to_fourcc(int format) -- 2.4.9 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 17/22] drm/i915: Pass rotation_info to intel_rotate_fb_obj_pages()
From: Ville Syrjäläintel_rotate_fb_obj_pages() doens't need the entire gtt view, just the rotation info suffices. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_gem_gtt.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 73ca67d..bb95b86 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3280,10 +3280,9 @@ rotate_pages(const dma_addr_t *in, unsigned int offset, } static struct sg_table * -intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view, +intel_rotate_fb_obj_pages(struct intel_rotation_info *rot_info, struct drm_i915_gem_object *obj) { - struct intel_rotation_info *rot_info = _view->rotated; unsigned int size_pages = rot_info->size >> PAGE_SHIFT; unsigned int size_pages_uv; struct sg_page_iter sg_iter; @@ -3423,7 +3422,7 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma) vma->ggtt_view.pages = vma->obj->pages; else if (vma->ggtt_view.type == I915_GGTT_VIEW_ROTATED) vma->ggtt_view.pages = - intel_rotate_fb_obj_pages(>ggtt_view, vma->obj); + intel_rotate_fb_obj_pages(>ggtt_view.rotated, vma->obj); else if (vma->ggtt_view.type == I915_GGTT_VIEW_PARTIAL) vma->ggtt_view.pages = intel_partial_pages(>ggtt_view, vma->obj); -- 2.4.9 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 18/22] drm/i915: Make sure fb offset is (macro)pixel aligned
From: Ville SyrjäläWe convert the fb->offsets[] into x/y offsets, so they must be (macro)pixel aligned. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 36 +--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ce346cfe..70e6e27 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14284,6 +14284,36 @@ u32 intel_fb_pitch_limit(struct drm_device *dev, uint64_t fb_modifier, } } +static int intel_fb_check_offsets(const struct drm_mode_fb_cmd2 *mode_cmd) +{ + uint32_t format = mode_cmd->pixel_format; + int i; + + for (i = 0; i < drm_format_num_planes(format); i++) { + unsigned int cpp; + + switch (format) { + case DRM_FORMAT_YUYV: + case DRM_FORMAT_UYVY: + case DRM_FORMAT_YVYU: + case DRM_FORMAT_VYUY: + cpp = 4; + break; + default: + cpp = drm_format_plane_cpp(format, i); + break; + } + + if (mode_cmd->offsets[i] % cpp) { + DRM_DEBUG("fb plane %d offset 0x%08x not (macro)pixel aligned\n", + i, mode_cmd->offsets[i]); + return -EINVAL; + } + } + + return 0; +} + static int intel_framebuffer_init(struct drm_device *dev, struct intel_framebuffer *intel_fb, struct drm_mode_fb_cmd2 *mode_cmd, @@ -14410,9 +14440,9 @@ static int intel_framebuffer_init(struct drm_device *dev, return -EINVAL; } - /* FIXME need to adjust LINOFF/TILEOFF accordingly. */ - if (mode_cmd->offsets[0] != 0) - return -EINVAL; + ret = intel_fb_check_offsets(mode_cmd); + if (ret) + return ret; aligned_height = intel_fb_align_height(dev, mode_cmd->height, mode_cmd->pixel_format, -- 2.4.9 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 12/22] drm/i915: Set i915_ggtt_view_normal type explicitly
From: Ville SyrjäläJust for clarity set the type for i915_ggtt_view_normal explicitly. While at it fix the indentation fail for i915_ggtt_view_rotated. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_gem_gtt.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 620d57e..71acc71 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -95,9 +95,11 @@ static int i915_get_ggtt_vma_pages(struct i915_vma *vma); -const struct i915_ggtt_view i915_ggtt_view_normal; +const struct i915_ggtt_view i915_ggtt_view_normal = { + .type = I915_GGTT_VIEW_NORMAL, +}; const struct i915_ggtt_view i915_ggtt_view_rotated = { -.type = I915_GGTT_VIEW_ROTATED + .type = I915_GGTT_VIEW_ROTATED, }; static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) -- 2.4.9 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t] benchmarks/gem_blt: Include igt.h in gem_blt.c
To fix a build error on android Signed-off-by: Derek Morton--- benchmarks/gem_blt.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/benchmarks/gem_blt.c b/benchmarks/gem_blt.c index 181a5f1..8ab5302 100644 --- a/benchmarks/gem_blt.c +++ b/benchmarks/gem_blt.c @@ -25,6 +25,7 @@ * */ +#include "igt.h" #include #include #include @@ -39,9 +40,6 @@ #include #include "drm.h" -#include "ioctl_wrappers.h" -#include "drmtest.h" -#include "intel_chipset.h" #define LOCAL_I915_EXEC_NO_RELOC (1<<11) #define LOCAL_I915_EXEC_HANDLE_LUT (1<<12) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/22] drm/i915: Handle fb->offsets[] and rewrite fb rotation handling to be more generic
On Wed, Oct 14, 2015 at 07:28:52PM +0300, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä> > So while reviewing the NV12 stuff it became clear to me no one > had really given fb->offsets[] handling any serious thought. > So this patch series aims to fix that. We now treat fb->offsets[] > as a linear offset always. One clear benefit over treating it as > a linear offset as opposed to a raw byte offset is that we don't > have to think about the layout of bytes within the tile at all. > > The series also generalizes the page rotation to be format agnostic, > the caller just specifies the desired geometry in pages for each > plane, and the rotation code builds up the sg. The intel_rotation_info > then just contains the minimal amount of information needed to > do the page rotation. > > SKL+ also gets changed to use the compute_page_offset stuff so that > the plane SURF register will contain the closes (properly aligned) > page boundary, and the x/y offsets deal with whatever is left over. > The plane code for the other platforms also gets simpler in the end > I think. Also the 90/270 rotation handling becomes rather trivial > for the plane code. > > I should still write some decent tests to exercise fb->offsets[]. > > Series available here: > git://github.com/vsyrjala/linux.git fb_offsets As mentioned on irc patches 13&14 need to me unified with my 3 patch series, but otherwise lgtm overall. -Daniel > > Ville Syrjälä (22): > drm: Add drm_format_plane_width() and drm_format_plane_height() > drm/i915: Pass modifier instead of tiling_mode to > gen4_compute_page_offset() > drm/i915: Factor out intel_tile_width() > drm/i915: Redo intel_tile_height() as intel_tile_size() / > intel_tile_width() > drm/i915: change intel_fill_fb_ggtt_view() to use the real tile size > drm/i915: Use intel_tile_{size,width,height}() in > intel_gen4_compute_page_offset() > drm/i915: s/intel_gen4_compute_page_offset/intel_compute_page_offset/ > drm/i915: Pass 90/270 vs. 0/180 rotation info for > intel_gen4_compute_page_offset() > drm/i915: Refactor intel_surf_alignment() > drm/i915: Support for extra alignment for tiled surfaces > drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj() > drm/i915: Set i915_ggtt_view_normal type explicitly > drm/i915: Move the partial and rotated view data into the same union > drm/i915: Don't treat differently sized rotated views as equal > drm/i915: Pass the dma_addr_t array as const to rotate_pages() > drm/i915: Pass stride to rotate_pages() > drm/i915: Pass rotation_info to intel_rotate_fb_obj_pages() > drm/i915: Make sure fb offset is (macro)pixel aligned > drm/i915: Don't leak framebuffer_references if drm_framebuffer_init() > fails > drm/i915: Pass drm_frambuffer to intel_compute_page_offset() > drm/i915: Rewrite fb rotation GTT handling > drm/i915: Don't pass pitch to intel_compute_page_offset() > > drivers/gpu/drm/i915/i915_gem.c | 10 +- > drivers/gpu/drm/i915/i915_gem_gtt.c | 88 ++-- > drivers/gpu/drm/i915/i915_gem_gtt.h | 27 +- > drivers/gpu/drm/i915/intel_display.c | 792 > +++ > drivers/gpu/drm/i915/intel_drv.h | 46 +- > drivers/gpu/drm/i915/intel_fbdev.c | 2 +- > drivers/gpu/drm/i915/intel_sprite.c | 128 +++--- > include/drm/drm_crtc.h | 12 + > 8 files changed, 681 insertions(+), 424 deletions(-) > > -- > 2.4.9 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [BXT DSI timing fixes v1 3/3] drm/i915/bxt: Fixed dsi enc disable and blank at bootup
>-Original Message- >From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter >Sent: Wednesday, October 14, 2015 6:20 PM >To: Shankar, Uma >Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org; Kumar, Shobhit >Subject: Re: [Intel-gfx] [BXT DSI timing fixes v1 3/3] drm/i915/bxt: Fixed dsi >enc >disable and blank at bootup > >On Wed, Oct 14, 2015 at 10:20:32AM +, Shankar, Uma wrote: >> >> >> >-Original Message- >> >From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of >> >Daniel Vetter >> >Sent: Tuesday, October 13, 2015 9:17 PM >> >To: Shankar, Uma >> >Cc: intel-gfx@lists.freedesktop.org; Kumar, Shobhit >> >Subject: Re: [Intel-gfx] [BXT DSI timing fixes v1 3/3] drm/i915/bxt: >> >Fixed dsi enc disable and blank at bootup >> > >> >On Mon, Oct 12, 2015 at 10:55:03PM +0530, Uma Shankar wrote: >> >> During bootup, mipi display was blanking in BXT. This is because >> >> during driver load, though encoder, connector were active but crtc >> >> returned inactive. This caused sanitize function to disable the DSI >> >> panel. In AOS, this is fine since HWC will do a modeset and crtc, >> >> connector, encoder mapping will be restored. But in Charging OS, no >> >> modeset is called, it just calls DPMS ON/OFF. Hence display doesn't >> >> come in COS. This is needed even for seamless booting to Android >> >> without a >> >blank. >> >> >> >> This is fine on BYT/CHT since transcoder is common b/w all encoders. >> >> But for BXT, there is a separate mipi transcoder. Hence this needs >> >> special handling for BXT DSI. >> >> >> >> Signed-off-by: Uma Shankar>> >> --- >> >> drivers/gpu/drm/i915/i915_drv.h |3 +++ >> >> drivers/gpu/drm/i915/intel_display.c | 27 +++ >> >> 2 files changed, 26 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> >> b/drivers/gpu/drm/i915/i915_drv.h index bf14096..ae790ec 100644 >> >> --- a/drivers/gpu/drm/i915/i915_drv.h >> >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> >> @@ -1948,6 +1948,9 @@ struct drm_i915_private { >> >> /* perform PHY state sanity checks? */ >> >> bool chv_phy_assert[2]; >> >> >> >> + /* To check if DSI is initializing at bootup */ >> >> + bool dsi_enumerating; >> > >> >See my other comment, you're working around the broken design of >> >patch 2 here. Special casing fastboot is a big no-no which needs some >> >really good reasons. An example is the special edp clock readout code >> >in the encoder callbacks we have for ilk-ivb cpu edp. >> >-Daniel >> > >> >> I agree this is not a clean solution but was not getting any better >> ideas. As part of driver load and initialization, readout_state >> returns encoder, connector as active but all crtc return inactive. >> This make sanitize encoder to consider this as inconsistent hw state >> and it goes ahead and disables encoder, thereby disabling the BIOS/GOP >> Initialization. >> >> After this only way to recover is a modeset which doesn't come in >> cases like Charging OS in Android. Also this will create issues in >> having a seamless boot without a blank (a must have for Android devices). >> >> For EDP case, we can read DDI_FUNC_CTL and get a pipe number and match >> it with crtc->pipe to detect EDP is on that pipe and map it to crtc. >> >> In DSI there is no such mechanism to detect this. Can you suggest some >> pointers as to how to approach this issue. > >MIPI_CTRL(port) has BXT_PIPE_SELECT_A/C bits. Which btw we don't ever seem >to check anywhere in intel_dsi_compute_config, which is a pretty stellar bug >that running kms_flip even once should have caught. >-Daniel >-- These fields were removed from the port control register in the bspec updates. These fields remained in the code and should be cleaned up and removed. As per latest bspec status, there is no pipe info in the port control for BXT DSI. I am not sure, we could add something like below in compute_config to update transcoder info: if (IS_BROXTON(dev)) { + if (intel_dsi->ports & (1 << PORT_A)) + config->cpu_transcoder = TRANSCODER_MIPI_A; + else + config->cpu_transcoder = TRANSCODER_MIPI_C; + } Thoughts ? Regards, Uma Shankar >Daniel Vetter >Software Engineer, Intel Corporation >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: respect previous reg values on primary plane disable
On Wed, Oct 14, 2015 at 10:58:29AM +0300, Jani Nikula wrote: > On Wed, 14 Oct 2015, Kevin Strasserwrote: > > On HSW the crc differs between black and disabled primary planes, causing an > > assert to fail in the kms_universal_plane test. It seems that things like > > gamma > > correction are causing the black primary plane case to result in a brighter > > color than the disabled primary plane case. > > > > Only toggle the enable bit instead of clearing the control register, making > > the > > disable path more similar to that of the sprite plane. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 > > Testcase: igt/kms_universal_plane > > Signed-off-by: Kevin Strasser > > Cc: sta...@vger.kernel.org # v3.18 > Fixes: fdd508a64192 ("drm/i915: Call .update_primary_plane in intel_{enable, > disable}_primary_hw_plane()") > > How about i9xx_update_primary_plane, modified in the same commit above, > and skylake_update_primary_plane, added in > > commit 6156a45602f990cdb140025a3ced96e6695980cf > Author: Chandra Konduru > Date: Mon Apr 27 13:48:39 2015 -0700 > > drm/i915: skylake primary plane scaling using shared scalers Afaict HSW is the only platform that behaves in this way. I will follow up with a separate patch if needed. Thanks, Kevin ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [DMC_REDESIGN_V2 03/14] drm/i915/bxt: release rpm reference if csr firmware failed to load.
On 9/30/2015 7:54 PM, Imre Deak wrote: On ke, 2015-08-26 at 16:58 +0530, Animesh Manna wrote: Note that for bxt without dmc, display engine can go to lowest possible state (dc9), so releasing the rpm reference. Cc: Daniel VetterCc: Damien Lespiau Cc: Imre Deak Cc: Sunil Kamath Signed-off-by: Animesh Manna --- drivers/gpu/drm/i915/intel_csr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index 75a775b..be388da 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -392,7 +392,7 @@ static void finish_csr_load(const struct firmware *fw, void *context) DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path); out: - if (fw_loaded) + if (fw_loaded || IS_BROXTON(dev)) intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); I don't think this is needed. We disable all display side runtime power management if the firmware is not available. So no need to special case this either imo. In display off case for broxton platform we can directly goto dc9 state for which dmc is not needed. So, don't want to block runtime power management from display side. -Animesh else intel_csr_load_status_set(dev_priv, FW_FAILED); ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] 8086:0f31_freezes_totally
On Wed, 14 Oct 2015, Jani Nikulawrote: > On Tue, 13 Oct 2015, Chris Rainey wrote: >> [1.] One line summary of the problem: 8086:0f31_freezes_totally >> >> [2.] Full description of the problem/report: Total lockup(cannot even >> switch to console via Alt-F1, F2, etc.). Bug is most easily >> reproducible via Chromium browser when opening multiple >> background-tabs(middle-button(wheel) mouse-clicks) and when using >> ALT-TAB to switch between browser and Xterms, etc. Very difficult to >> pin due to randomness(i.e. system may run for 30-seconds, 30-minutes >> or 30-hours before freeze). However -- it rarely runs stable for more >> that 24-48/hrs without a lockup. > > Please file a bug at [1] running drm-intel-nightly. Ermh, I meant to say, and attach dmesg from boot with drm.debug=14 module parameter set. J. > > BR, > Jani. > > > [1] https://bugs.freedesktop.org/enter_bug.cgi?product=DRI=DRM/Intel > > > -- > Jani Nikula, Intel Open Source Technology Center -- 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] 8086:0f31_freezes_totally
On Tue, 13 Oct 2015, Chris Raineywrote: > [1.] One line summary of the problem: 8086:0f31_freezes_totally > > [2.] Full description of the problem/report: Total lockup(cannot even > switch to console via Alt-F1, F2, etc.). Bug is most easily > reproducible via Chromium browser when opening multiple > background-tabs(middle-button(wheel) mouse-clicks) and when using > ALT-TAB to switch between browser and Xterms, etc. Very difficult to > pin due to randomness(i.e. system may run for 30-seconds, 30-minutes > or 30-hours before freeze). However -- it rarely runs stable for more > that 24-48/hrs without a lockup. Please file a bug at [1] running drm-intel-nightly. BR, Jani. [1] https://bugs.freedesktop.org/enter_bug.cgi?product=DRI=DRM/Intel -- 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] v4.3-rc4: i915: ThinkPad Yoga 12: *ERROR* The master control interrupt lied (SDE)! [regression]
On Wed, 14 Oct 2015, "Miramontes Caton, Jairo Daniel"wrote: > Created bug in fdo bugzilla to keep track of this regression: > https://bugs.freedesktop.org/show_bug.cgi?id=92454 Please disregard that, there's already a bug report at https://bugs.freedesktop.org/show_bug.cgi?id=92084. BR, Jani. -- 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 09/20] i915: switch from acpi_os_ioremap to memremap
On Tue, Oct 13, 2015 at 09:24:26AM -0700, Dan Williams wrote: > On Tue, Oct 13, 2015 at 1:24 AM, Daniel Vetterwrote: > > On Mon, Oct 12, 2015 at 09:12:57PM +, Williams, Dan J wrote: > >> On Mon, 2015-10-12 at 09:01 +0200, Daniel Vetter wrote: > >> > On Fri, Oct 09, 2015 at 06:16:25PM -0400, Dan Williams wrote: > >> > > i915 expects the OpRegion to be cached (i.e. not __iomem), so > >> > > explicitly > >> > > map it with memremap rather than the implied cache setting of > >> > > acpi_os_ioremap(). > >> > > > >> > > Cc: Daniel Vetter > >> > > Cc: Jani Nikula > >> > > Cc: intel-gfx@lists.freedesktop.org > >> > > Cc: David Airlie > >> > > Cc: dri-de...@lists.freedesktop.org > >> > > Signed-off-by: Dan Williams > >> > > >> > Assuming you've run sparse over this to make sure you've caught them all, > >> > and with the nit below addressed this is > >> > > >> > Reviewed-by: Daniel Vetter > >> > >> Indeed, re-running sparse again found a few conversions of ioread* I > >> missed as well as moving the force casting out of validate_vbt() to > >> find_vbt(). > >> > >> > Feel free to pull v2 into whatever tree you think it's suitable for (but > >> > you can also resend and I'll pick it up). > >> > >> Please pick up v2 below. > > > > Queued for -next, thanks for the patch. Aside: Attached or separate mail > > seems easier, somehow git apply-mbox can't auto-eat this for of patch. > > -Daniel > > > > "git am --scissors" should detect the "8<---" cut line. TIL, works nice indeed. Thanks for the hint. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation 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: respect previous reg values on primary plane disable
On Wed, 14 Oct 2015, Jani Nikulawrote: > On Wed, 14 Oct 2015, Kevin Strasser wrote: >> On HSW the crc differs between black and disabled primary planes, causing an >> assert to fail in the kms_universal_plane test. It seems that things like >> gamma >> correction are causing the black primary plane case to result in a brighter >> color than the disabled primary plane case. >> >> Only toggle the enable bit instead of clearing the control register, making >> the >> disable path more similar to that of the sprite plane. >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 >> Testcase: igt/kms_universal_plane >> Signed-off-by: Kevin Strasser > > Cc: sta...@vger.kernel.org # v3.18 > Fixes: fdd508a64192 ("drm/i915: Call .update_primary_plane in intel_{enable, > disable}_primary_hw_plane()") > > How about i9xx_update_primary_plane, modified in the same commit above, > and skylake_update_primary_plane, added in > > commit 6156a45602f990cdb140025a3ced96e6695980cf > Author: Chandra Konduru > Date: Mon Apr 27 13:48:39 2015 -0700 > > drm/i915: skylake primary plane scaling using shared scalers PS. If these need fixing too, please keep i9xx/ironlake fix in one commit, and skylake in another, as they're destined to be backported to different kernels. BR, Jani. > > BR, > Jani. > >> --- >> drivers/gpu/drm/i915/intel_display.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c >> b/drivers/gpu/drm/i915/intel_display.c >> index cddb0c6..b6164d8e 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -2829,7 +2829,7 @@ static void ironlake_update_primary_plane(struct >> drm_crtc *crtc, >> int pixel_size; >> >> if (!visible || !fb) { >> -I915_WRITE(reg, 0); >> +I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); >> I915_WRITE(DSPSURF(plane), 0); >> POSTING_READ(reg); >> return; >> -- >> 1.9.1 >> >> ___ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center -- 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] drm/i915: respect previous reg values on primary plane disable
On Wed, 14 Oct 2015, Kevin Strasserwrote: > On HSW the crc differs between black and disabled primary planes, causing an > assert to fail in the kms_universal_plane test. It seems that things like > gamma > correction are causing the black primary plane case to result in a brighter > color than the disabled primary plane case. > > Only toggle the enable bit instead of clearing the control register, making > the > disable path more similar to that of the sprite plane. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 > Testcase: igt/kms_universal_plane > Signed-off-by: Kevin Strasser Cc: sta...@vger.kernel.org # v3.18 Fixes: fdd508a64192 ("drm/i915: Call .update_primary_plane in intel_{enable, disable}_primary_hw_plane()") How about i9xx_update_primary_plane, modified in the same commit above, and skylake_update_primary_plane, added in commit 6156a45602f990cdb140025a3ced96e6695980cf Author: Chandra Konduru Date: Mon Apr 27 13:48:39 2015 -0700 drm/i915: skylake primary plane scaling using shared scalers BR, Jani. > --- > drivers/gpu/drm/i915/intel_display.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index cddb0c6..b6164d8e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2829,7 +2829,7 @@ static void ironlake_update_primary_plane(struct > drm_crtc *crtc, > int pixel_size; > > if (!visible || !fb) { > - I915_WRITE(reg, 0); > + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); > I915_WRITE(DSPSURF(plane), 0); > POSTING_READ(reg); > return; > -- > 1.9.1 > > ___ > 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] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw
On Tue, 2015-10-13 at 16:08 +0200, Daniel Vetter wrote: > On Tue, Oct 13, 2015 at 04:00:37PM +0200, Maarten Lankhorst wrote: > > Op 13-10-15 om 15:58 schreef Daniel Vetter: > > > On Tue, Oct 13, 2015 at 03:43:28PM +0200, Maarten Lankhorst wrote: > > > > Op 13-10-15 om 15:35 schreef Daniel Vetter: > > > > > On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst wrote: > > > > > > Op 23-09-15 om 17:34 schreef Gabriel Feceoru: > > > > > > > Using 2 connectors (DVI and VGA) will cause wrpll to be set for > > > > > > > INTEL_OUTPUT_HDMI but never reset if switching to INTEL_OUTPUT_VGA > > > > > > > > > > > > > > Supresses errors like these: > > > > > > > [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in > > > > > > > dpll_hw_state.wrpll > > > > > > > > > > > > > Looks like a good idea to always zero it. > > > > > Except that we still have a bunch of cases where we recompute clock > > > > > state > > > > > but only partially. Can we just move them all up into a common place > > > > > please? That would also catch cases where we simply forget to fill > > > > > this > > > > > out at all. > > > > > > > > > > One case I noticed is edp in skl_ddi_pll_select, but there's probably > > > > > more. > > > > > > > > > Something like below, with all the memsets for dpll_hw_state removed? > > > I think this will blow up since we recompute clock state only when > > > needs_modeset is true. So needs a bit more intelligence in deciding when > > > to clear it I think. > > Oops you're right. Maybe intel_modeset_clear_plls because that's where all > > the clock state > > belongs? > > Yeah that might be an even better place, in the loop after the continue; > statement. The reason I didn't put the memset there in the first place was the way we calculate plls for DP with DDI platforms. In that case, ddi_pll_sel is setup from the encoder_config instead of compute_clock, so a memset ends up clearing the new pll config. Ander ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: force link training when requested by Sink
Compliance test 4.3.1.11 requires source to perform link training always if the automated test requests for it. This patch enforces this requirement. Signed-off-by: Sivakumar ThulasimaniSigned-off-by: Shubhangi Shrivastava --- drivers/gpu/drm/i915/intel_dp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 7bc75ef..3c7fc58 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4466,7 +4466,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); } - if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { + /* if link training is requested we should perform it always */ + if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) || + (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) { DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", intel_encoder->base.name); intel_dp_start_link_train(intel_dp); -- 2.6.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/2] Enforcing link training as per compliance test requirement
This patchset performs link training whenever it is requested by Panel as stated in compliance test requirement. This requires clearing the automated test data during every short pulse or long pulse instead of clearing it only during the automated requests. Shubhangi Shrivastava (2): drm/i915: Cleanup test data during long/short hotplug drm/i915: force link training when requested by Sink drivers/gpu/drm/i915/intel_dp.c | 25 - 1 file changed, 16 insertions(+), 9 deletions(-) -- 2.6.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/i915: Cleanup test data during long/short hotplug
Automated test data that is updated when a test is requested is not cleared till next automated test request is recevied which can cause various problems. This patch fixes this by clearing this during the next short pulse and on hot unplug. For example, when TEST_LINK_TRAINING is requested it is updated to appropriate variable inside intel_dp_handle_test_request but is also cleared only inside the same function. if the next short pulse does not have the AUTOMATED_TEST_REQUEST bits set the variable will not be cleared resulting in carrying incorrect test status in local variables. Signed-off-by: Sivakumar ThulasimaniSigned-off-by: Shubhangi Shrivastava --- drivers/gpu/drm/i915/intel_dp.c | 21 + 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 18bcfbe..7bc75ef 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4317,13 +4317,6 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) uint8_t rxdata = 0; int status = 0; - intel_dp->compliance_test_active = 0; - intel_dp->compliance_test_type = 0; - intel_dp->compliance_test_data = 0; - - intel_dp->aux.i2c_nack_count = 0; - intel_dp->aux.i2c_defer_count = 0; - status = drm_dp_dpcd_read(_dp->aux, DP_TEST_REQUEST, , 1); if (status <= 0) { DRM_DEBUG_KMS("Could not read test request from sink\n"); @@ -4439,6 +4432,10 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) WARN_ON(!drm_modeset_is_locked(>mode_config.connection_mutex)); + intel_dp->compliance_test_active = 0; + intel_dp->compliance_test_type = 0; + intel_dp->compliance_test_data = 0; + if (!intel_encoder->base.crtc) return; @@ -4817,8 +4814,16 @@ intel_dp_detect(struct drm_connector *connector, bool force) status = ironlake_dp_detect(intel_dp); else status = g4x_dp_detect(intel_dp); - if (status != connector_status_connected) + if (status != connector_status_connected) { + intel_dp->compliance_test_active = 0; + intel_dp->compliance_test_type = 0; + intel_dp->compliance_test_data = 0; + + intel_dp->aux.i2c_nack_count = 0; + intel_dp->aux.i2c_defer_count = 0; + goto out; + } intel_dp_probe_oui(intel_dp); -- 2.6.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation
On Tue, Oct 13, 2015 at 06:04:40PM +0300, Ville Syrjälä wrote: > > + mutex_lock(>struct_mutex); > > + > > I'm thinking we won't even need the lock here anymore. But maybe I'm > missing something. Yeah, not so much here atm, but there is a bug here where we don't take an explicit pin on the VMA we setup for the fbdev which requires the lock. -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] [DMC_REDESIGN_V2 03/14] drm/i915/bxt: release rpm reference if csr firmware failed to load.
On Wed, 14 Oct 2015, Animesh Mannawrote: > On 9/30/2015 7:54 PM, Imre Deak wrote: >> On ke, 2015-08-26 at 16:58 +0530, Animesh Manna wrote: >>> Note that for bxt without dmc, display engine can go to lowest >>> possible state (dc9), so releasing the rpm reference. >>> >>> Cc: Daniel Vetter >>> Cc: Damien Lespiau >>> Cc: Imre Deak >>> Cc: Sunil Kamath >>> Signed-off-by: Animesh Manna >>> --- >>> drivers/gpu/drm/i915/intel_csr.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_csr.c >>> b/drivers/gpu/drm/i915/intel_csr.c >>> index 75a775b..be388da 100644 >>> --- a/drivers/gpu/drm/i915/intel_csr.c >>> +++ b/drivers/gpu/drm/i915/intel_csr.c >>> @@ -392,7 +392,7 @@ static void finish_csr_load(const struct firmware *fw, >>> void *context) >>> >>> DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path); >>> out: >>> - if (fw_loaded) >>> + if (fw_loaded || IS_BROXTON(dev)) >>> intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); >> I don't think this is needed. We disable all display side runtime power >> management if the firmware is not available. So no need to special case >> this either imo. > In display off case for broxton platform we can directly goto dc9 state > for which dmc is not needed. > So, don't want to block runtime power management from display side. Your patch doesn't set load status to failed on bxt. There's another version from Sagar that does [1]. BR, Jani. [1] http://mid.gmane.org/1444754985-15734-1-git-send-email-sagar.a.kam...@intel.com > > -Animesh >> >>> else >>> intel_csr_load_status_set(dev_priv, FW_FAILED); >> > > ___ > 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] [DMC_REDESIGN_V2 03/14] drm/i915/bxt: release rpm reference if csr firmware failed to load.
On Wed, 2015-10-14 at 11:59 +0530, Animesh Manna wrote: > > On 9/30/2015 7:54 PM, Imre Deak wrote: > > On ke, 2015-08-26 at 16:58 +0530, Animesh Manna wrote: > >> Note that for bxt without dmc, display engine can go to lowest > >> possible state (dc9), so releasing the rpm reference. > >> > >> Cc: Daniel Vetter> >> Cc: Damien Lespiau > >> Cc: Imre Deak > >> Cc: Sunil Kamath > >> Signed-off-by: Animesh Manna > >> --- > >> drivers/gpu/drm/i915/intel_csr.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_csr.c > >> b/drivers/gpu/drm/i915/intel_csr.c > >> index 75a775b..be388da 100644 > >> --- a/drivers/gpu/drm/i915/intel_csr.c > >> +++ b/drivers/gpu/drm/i915/intel_csr.c > >> @@ -392,7 +392,7 @@ static void finish_csr_load(const struct firmware *fw, > >> void *context) > >> > >>DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path); > >> out: > >> - if (fw_loaded) > >> + if (fw_loaded || IS_BROXTON(dev)) > >>intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); > > I don't think this is needed. We disable all display side runtime power > > management if the firmware is not available. So no need to special case > > this either imo. > In display off case for broxton platform we can directly goto dc9 state > for which dmc is not needed. > So, don't want to block runtime power management from display side. We could also runtime suspend on Skylake without the DMC firmware. We made the decision that we won't allow that, for no other reason than to simplify the code. Doing that we don't need to check for the firmware loaded status for instance to decide whether to enable DC5/6 or not (which can be racy). So why would we now add the complexity for one platform when we decidedly removed it from another? --Imre > > -Animesh > > > >>else > >>intel_csr_load_status_set(dev_priv, FW_FAILED); > > > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [v4.3/-fixes PATCH/BACKPORT 0/4] plane related backports to -fixes
Op 14-10-15 om 11:06 schreef Jani Nikula: > I'm proposing to backport these to v4.3/-fixes, objections or acks? > > BR, > Jani. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=91910#c4 > > > Jani Nikula (1): > Revert "drm/i915: Add primary plane to mask if it's visible" > > Maarten Lankhorst (1): > drm/i915: Add primary plane to mask if it's visible > > Ville Syrjälä (2): > drm/i915: Assign hwmode after encoder state readout > drm/i915: Move sprite/cursor plane disable to intel_sanitize_crtc() > > drivers/gpu/drm/i915/intel_display.c | 106 > +-- > 1 file changed, 53 insertions(+), 53 deletions(-) > Acking all. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [v4.3/-fixes PATCH/BACKPORT 4/4] drm/i915: Add primary plane to mask if it's visible
From: Maarten LankhorstThis fixes the warnings like "plane A assertion failure, should be disabled but not" that on the initial modeset during boot. This can happen if the primary plane is enabled by the firmware, but inheriting it fails because the DMAR is active or for other reasons. Most likely caused by commit 36750f284b3a4f19b304fda1bb7d6e9e1275ea8d Author: Maarten Lankhorst Date: Mon Jun 1 12:49:54 2015 +0200 drm/i915: update plane state during init This is a new version of commit 721a09f7393de6c28a07516dccd654c6e995944a Author: Maarten Lankhorst Date: Tue Sep 15 14:28:54 2015 +0200 drm/i915: Add primary plane to mask if it's visible That was reverted. Reported-by: Andreas Reis Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91429 Reported-and-tested-by: Emil Renner Berthing Tested-by: Andreas Reis Signed-off-by: Daniel Vetter Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/intel_display.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ac407e3e1edd..b2270d576979 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15101,11 +15101,15 @@ static bool primary_get_hw_state(struct intel_plane *plane) /* FIXME read out full plane state for all planes */ static void readout_plane_state(struct intel_crtc *crtc) { + struct drm_plane *primary = crtc->base.primary; struct intel_plane_state *plane_state = - to_intel_plane_state(crtc->base.primary->state); + to_intel_plane_state(primary->state); plane_state->visible = - primary_get_hw_state(to_intel_plane(crtc->base.primary)); + primary_get_hw_state(to_intel_plane(primary)); + + if (plane_state->visible) + crtc->base.state->plane_mask |= 1 << drm_plane_index(primary); } static void intel_modeset_readout_hw_state(struct drm_device *dev) -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [v4.3/-fixes PATCH/BACKPORT 0/4] plane related backports to -fixes
I'm proposing to backport these to v4.3/-fixes, objections or acks? BR, Jani. References: https://bugs.freedesktop.org/show_bug.cgi?id=91910#c4 Jani Nikula (1): Revert "drm/i915: Add primary plane to mask if it's visible" Maarten Lankhorst (1): drm/i915: Add primary plane to mask if it's visible Ville Syrjälä (2): drm/i915: Assign hwmode after encoder state readout drm/i915: Move sprite/cursor plane disable to intel_sanitize_crtc() drivers/gpu/drm/i915/intel_display.c | 106 +-- 1 file changed, 53 insertions(+), 53 deletions(-) -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [v4.3/-fixes PATCH/BACKPORT 2/4] drm/i915: Assign hwmode after encoder state readout
From: Ville SyrjäläThe dotclock is often calculated in encoder .get_config(), so we shouldn't copy the adjusted_mode to hwmode until we have read out the dotclock. Gets rid of some warnings like these: [drm:drm_calc_timestamping_constants [drm]] *ERROR* crtc 21: Can't calculate constants, dotclock = 0! [drm:i915_get_vblank_timestamp] crtc 0 is disabled v2: Steal Maarten's idea to move crtc->mode etc. assignment too Cc: Maarten Lankhorst Cc: Patrik Jakobsson Signed-off-by: Ville Syrjälä Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91428 Reviewed-by: Patrik Jakobsson Signed-off-by: Daniel Vetter Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/intel_display.c | 57 +++- 1 file changed, 30 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7704315e067f..ed87a7e4c32a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15132,33 +15132,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active; - memset(>base.mode, 0, sizeof(crtc->base.mode)); - if (crtc->base.state->active) { - intel_mode_from_pipe_config(>base.mode, crtc->config); - intel_mode_from_pipe_config(>base.state->adjusted_mode, crtc->config); - WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, >base.mode)); - - /* -* The initial mode needs to be set in order to keep -* the atomic core happy. It wants a valid mode if the -* crtc's enabled, so we do the above call. -* -* At this point some state updated by the connectors -* in their ->detect() callback has not run yet, so -* no recalculation can be done yet. -* -* Even if we could do a recalculation and modeset -* right now it would cause a double modeset if -* fbdev or userspace chooses a different initial mode. -* -* If that happens, someone indicated they wanted a -* mode change, which means it's safe to do a full -* recalculation. -*/ - crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED; - } - - crtc->base.hwmode = crtc->config->base.adjusted_mode; readout_plane_state(crtc, to_intel_crtc_state(crtc->base.state)); DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n", @@ -15218,6 +15191,36 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) connector->base.name, connector->base.encoder ? "enabled" : "disabled"); } + + for_each_intel_crtc(dev, crtc) { + crtc->base.hwmode = crtc->config->base.adjusted_mode; + + memset(>base.mode, 0, sizeof(crtc->base.mode)); + if (crtc->base.state->active) { + intel_mode_from_pipe_config(>base.mode, crtc->config); + intel_mode_from_pipe_config(>base.state->adjusted_mode, crtc->config); + WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, >base.mode)); + + /* +* The initial mode needs to be set in order to keep +* the atomic core happy. It wants a valid mode if the +* crtc's enabled, so we do the above call. +* +* At this point some state updated by the connectors +* in their ->detect() callback has not run yet, so +* no recalculation can be done yet. +* +* Even if we could do a recalculation and modeset +* right now it would cause a double modeset if +* fbdev or userspace chooses a different initial mode. +* +* If that happens, someone indicated they wanted a +* mode change, which means it's safe to do a full +* recalculation. +*/ + crtc->base.state->mode.private_flags = I915_MODE_FLAG_INHERITED; + } + } } /* Scan out the
[Intel-gfx] [v4.3/-fixes PATCH/BACKPORT 1/4] Revert "drm/i915: Add primary plane to mask if it's visible"
This reverts commit 721a09f7393de6c28a07516dccd654c6e995944a. There is nothing wrong with the commit per se. We had two versions of the commit, one in -next headed for v4.4 and this one for v4.3. Turns out we'll need to backport more fixes from -next, and they conflict with the v4.3 version. It gets messy. It will be easiest to revert this one, and backport all the relevant commits from -next without modifications; they apply cleanly after this revert. Requested-by: Joseph YasiReferences: https://bugs.freedesktop.org/show_bug.cgi?id=91910#c4 Cc: Maarten Lankhorst Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/intel_display.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2bf248b04542..7704315e067f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15101,12 +15101,9 @@ static void readout_plane_state(struct intel_crtc *crtc, plane_state = to_intel_plane_state(p->base.state); - if (p->base.type == DRM_PLANE_TYPE_PRIMARY) { + if (p->base.type == DRM_PLANE_TYPE_PRIMARY) plane_state->visible = primary_get_hw_state(crtc); - if (plane_state->visible) - crtc->base.state->plane_mask |= - 1 << drm_plane_index(>base); - } else { + else { if (active) p->disable_plane(>base, >base); -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [v4.3/-fixes PATCH/BACKPORT 3/4] drm/i915: Move sprite/cursor plane disable to intel_sanitize_crtc()
From: Ville SyrjäläMove the sprite/cursor plane disabling to occur in intel_sanitize_crtc() where it belongs instead of doing it in intel_modeset_readout_hw_state(). The plane disabling was first added in 4cf0ebbd4fafbdf8e6431dbb315e5511c3efdc3b drm/i915: Rework plane readout. I got the idea from some patches from Partik and/or Maarten but those moved also the plane state readout to intel_sanitize_crtc() which isn't quite right in my opinion. Cc: Maarten Lankhorst Cc: Patrik Jakobsson Signed-off-by: Ville Syrjälä References: https://bugs.freedesktop.org/show_bug.cgi?id=91910 Reviewed-by: Patrik Jakobsson Signed-off-by: Daniel Vetter Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/intel_display.c | 44 1 file changed, 20 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ed87a7e4c32a..ac407e3e1edd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14911,9 +14911,19 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) /* restore vblank interrupts to correct state */ drm_crtc_vblank_reset(>base); if (crtc->active) { + struct intel_plane *plane; + drm_calc_timestamping_constants(>base, >base.hwmode); update_scanline_offset(crtc); drm_crtc_vblank_on(>base); + + /* Disable everything but the primary plane */ + for_each_intel_plane_on_crtc(dev, crtc, plane) { + if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) + continue; + + plane->disable_plane(>base, >base); + } } /* We need to sanitize the plane -> pipe mapping first because this will @@ -15081,35 +15091,21 @@ void i915_redisable_vga(struct drm_device *dev) i915_redisable_vga_power_on(dev); } -static bool primary_get_hw_state(struct intel_crtc *crtc) +static bool primary_get_hw_state(struct intel_plane *plane) { - struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); - return !!(I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE); + return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE; } -static void readout_plane_state(struct intel_crtc *crtc, - struct intel_crtc_state *crtc_state) +/* FIXME read out full plane state for all planes */ +static void readout_plane_state(struct intel_crtc *crtc) { - struct intel_plane *p; - struct intel_plane_state *plane_state; - bool active = crtc_state->base.active; + struct intel_plane_state *plane_state = + to_intel_plane_state(crtc->base.primary->state); - for_each_intel_plane(crtc->base.dev, p) { - if (crtc->pipe != p->pipe) - continue; - - plane_state = to_intel_plane_state(p->base.state); - - if (p->base.type == DRM_PLANE_TYPE_PRIMARY) - plane_state->visible = primary_get_hw_state(crtc); - else { - if (active) - p->disable_plane(>base, >base); - - plane_state->visible = false; - } - } + plane_state->visible = + primary_get_hw_state(to_intel_plane(crtc->base.primary)); } static void intel_modeset_readout_hw_state(struct drm_device *dev) @@ -15132,7 +15128,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active; - readout_plane_state(crtc, to_intel_crtc_state(crtc->base.state)); + readout_plane_state(crtc); DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n", crtc->base.base.id, -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Log correct start and length in pte map trace
On Tue, 13 Oct 2015, Michel Thierrywrote: > The PTE_map trace added in commit 4c06ec8d13d2 ("drm/i915/gen8: Add > dynamic page trace events") was using the full start and length values, > instead of the page directory ones. > > Since this is just a trace, I don't think it requires cc'ing stable. Especially not since 4c06ec8d13d2 is not even in Linus' tree yet. BR, Jani. > > Cc: Akash Goel > Signed-off-by: Michel Thierry > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > b/drivers/gpu/drm/i915/i915_gem_gtt.c > index e81990d..642fe87 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -1305,8 +1305,8 @@ static int gen8_alloc_va_range_3lvl(struct > i915_address_space *vm, > page_directory[pde] = gen8_pde_encode(px_dma(pt), > I915_CACHE_LLC); > trace_i915_page_table_entry_map(>base, pde, pt, > - gen8_pte_index(start), > - gen8_pte_count(start, > length), > + > gen8_pte_index(pd_start), > + > gen8_pte_count(pd_start, pd_len), > GEN8_PTES); > > /* NB: We haven't yet mapped ptes to pages. At this > -- > 2.6.0 > > ___ > 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] drm/i915: respect previous reg values on primary plane disable
On Wed, Oct 14, 2015 at 10:48:52PM +0300, Ville Syrjälä wrote: > On Wed, Oct 14, 2015 at 11:59:59AM -0700, Kevin Strasser wrote: [...] > > Just to level set, these cases will produce different CRCs on HSW: > > 1. Primary plane disabled, gamma correction disabled > > 2. Primary plane disabled, gamma correction enabled > > > > Case 2 is visibly brighter than case 1 and looks more like the enabled black > > primary plane case. > > Ugh. That's weird. I thought data not going through any plane would > bypass the gamma too. > > > The purpose of this patch is to get the behavior of a > > disabled primary plane to match that of an enabled black plane, just as it > > does > > on non-HSW platforms. > > Does it? I just tried it on IVB, and behaves just like you said. So not > sure how far back this goes. Ah, so this test case is failing on IVB too? Are there any reporting tools we can look at to find out what tests are passing for each platform? > And now I'm really wondering about platforms where the primary > plane need not be fullscreen (gen2/3 and chv). > > I tried this on SKL too, but that confused me even more. The data not > going through any plane seems to be gamma corrected regardless of any > plane control bits, so that's good. However the legacy palette seems > all fubar. Black input apparently doesn't map to palette entry 0. > I wonder if you're seeing this on HSW too, or is your palette entry 0 > supposed to be non-black? I also tried on BDW and it is passing the test with and without my patch applied. I'm not really sure what 'palette entry 0' you are looking for. Could you point me to where in the code I can find out? > Looks like quite a bit more testing is needed to get to the bottom of > this. Agreed, this does seem to extend beyond HSW. For now do you think my patch is the right approach at least for HSW alone? Thanks, Kevin ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: respect previous reg values on primary plane disable
On Wed, Oct 14, 2015 at 03:22:23PM +0300, Ville Syrjälä wrote: > On Wed, Oct 14, 2015 at 01:12:27PM +0100, Chris Wilson wrote: > > On Wed, Oct 14, 2015 at 03:07:41PM +0300, Ville Syrjälä wrote: > > > On Tue, Oct 13, 2015 at 02:24:41PM -0700, Kevin Strasser wrote: [...] > > > > - I915_WRITE(reg, 0); > > > > + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); > > > > > > Eh, what now? We've been trying to eliminate these nasty RMWs. > > > > > > Are you saying that if we disabled the plane, but leave the "pass plane > > > data through gamma" it still affects the output for any pixel "covered" > > > by the disabled plane? > > > > What I thought was being said was that if a plane is set to black (but > > with gamma enabled on the pipe) then a different CRC is produced > > compared to when the pipe is completely disabled (no plane at all). It > > sounded to me like a test case failure. > > In that case I don't understand how the patch is supposed to help. > > But yeah, tests like these should really set up an identity gamma > and pipe csc matrix. > > Also we should grow some properties to control whether the plane > data passes through the gamma/csc or not. Those could then be used > to achieeve the same effect. Just to level set, these cases will produce different CRCs on HSW: 1. Primary plane disabled, gamma correction disabled 2. Primary plane disabled, gamma correction enabled Case 2 is visibly brighter than case 1 and looks more like the enabled black primary plane case. The purpose of this patch is to get the behavior of a disabled primary plane to match that of an enabled black plane, just as it does on non-HSW platforms. Understood, RMWs are inappropriate here. I'll rework the patch to explicitly enable the bits that are needed. Thanks, Kevin ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: respect previous reg values on primary plane disable
On Wed, Oct 14, 2015 at 8:44 PM, Kevin Strasserwrote: > On Wed, Oct 14, 2015 at 10:58:29AM +0300, Jani Nikula wrote: >> On Wed, 14 Oct 2015, Kevin Strasser wrote: >> > On HSW the crc differs between black and disabled primary planes, causing >> > an >> > assert to fail in the kms_universal_plane test. It seems that things like >> > gamma >> > correction are causing the black primary plane case to result in a brighter >> > color than the disabled primary plane case. >> > >> > Only toggle the enable bit instead of clearing the control register, >> > making the >> > disable path more similar to that of the sprite plane. >> > >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 >> > Testcase: igt/kms_universal_plane >> > Signed-off-by: Kevin Strasser >> >> Cc: sta...@vger.kernel.org # v3.18 >> Fixes: fdd508a64192 ("drm/i915: Call .update_primary_plane in intel_{enable, >> disable}_primary_hw_plane()") >> >> How about i9xx_update_primary_plane, modified in the same commit above, >> and skylake_update_primary_plane, added in >> >> commit 6156a45602f990cdb140025a3ced96e6695980cf >> Author: Chandra Konduru >> Date: Mon Apr 27 13:48:39 2015 -0700 >> >> drm/i915: skylake primary plane scaling using shared scalers > > Afaict HSW is the only platform that behaves in this way. I will follow up > with > a separate patch if needed. Are you sure this is specific to hsw and not an artifact of e.g. the hdmi color compression we do? That would apply the same on bdw, but you need the same screen plugged in. -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: respect previous reg values on primary plane disable
On Wed, Oct 14, 2015 at 11:59:59AM -0700, Kevin Strasser wrote: > On Wed, Oct 14, 2015 at 03:22:23PM +0300, Ville Syrjälä wrote: > > On Wed, Oct 14, 2015 at 01:12:27PM +0100, Chris Wilson wrote: > > > On Wed, Oct 14, 2015 at 03:07:41PM +0300, Ville Syrjälä wrote: > > > > On Tue, Oct 13, 2015 at 02:24:41PM -0700, Kevin Strasser wrote: > [...] > > > > > - I915_WRITE(reg, 0); > > > > > + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); > > > > > > > > Eh, what now? We've been trying to eliminate these nasty RMWs. > > > > > > > > Are you saying that if we disabled the plane, but leave the "pass plane > > > > data through gamma" it still affects the output for any pixel "covered" > > > > by the disabled plane? > > > > > > What I thought was being said was that if a plane is set to black (but > > > with gamma enabled on the pipe) then a different CRC is produced > > > compared to when the pipe is completely disabled (no plane at all). It > > > sounded to me like a test case failure. > > > > In that case I don't understand how the patch is supposed to help. > > > > But yeah, tests like these should really set up an identity gamma > > and pipe csc matrix. > > > > Also we should grow some properties to control whether the plane > > data passes through the gamma/csc or not. Those could then be used > > to achieeve the same effect. > > Just to level set, these cases will produce different CRCs on HSW: > 1. Primary plane disabled, gamma correction disabled > 2. Primary plane disabled, gamma correction enabled > > Case 2 is visibly brighter than case 1 and looks more like the enabled black > primary plane case. Ugh. That's weird. I thought data not going through any plane would bypass the gamma too. > The purpose of this patch is to get the behavior of a > disabled primary plane to match that of an enabled black plane, just as it > does > on non-HSW platforms. Does it? I just tried it on IVB, and behaves just like you said. So not sure how far back this goes. And now I'm really wondering about platforms where the primary plane need not be fullscreen (gen2/3 and chv). I tried this on SKL too, but that confused me even more. The data not going through any plane seems to be gamma corrected regardless of any plane control bits, so that's good. However the legacy palette seems all fubar. Black input apparently doesn't map to palette entry 0. I wonder if you're seeing this on HSW too, or is your palette entry 0 supposed to be non-black? Looks like quite a bit more testing is needed to get to the bottom of this. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] drm/i915/hsw: keep gamma and CSC enabled for primary plane disable
On HSW the crc differs between black and disabled primary planes, causing an assert to fail in the kms_universal_plane test. It seems that gamma correction and color space conversion are causing the black primary plane case to result in a brighter color than the disabled primary plane case. Keep gamma and CSC bits enabled for plane disable path on HSW. v2: Avoid use of RMW Keep path unchanged for non-HSW users Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 Testcase: igt/kms_universal_plane/universal-plane-pipe-A-functional Signed-off-by: Kevin Strasser--- drivers/gpu/drm/i915/intel_display.c | 16 ++-- 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d37b7a1..4a04e8b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2798,8 +2798,17 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, u32 reg = DSPCNTR(plane); int pixel_size; + dspcntr = DISPPLANE_GAMMA_ENABLE; + + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) + dspcntr |= DISPPLANE_PIPE_CSC_ENABLE; + if (!visible || !fb) { - I915_WRITE(reg, 0); + if (IS_HASWELL(dev)) { + I915_WRITE(reg, dspcntr); + } else { + I915_WRITE(reg, 0); + } I915_WRITE(DSPSURF(plane), 0); POSTING_READ(reg); return; @@ -2811,13 +2820,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); - dspcntr = DISPPLANE_GAMMA_ENABLE; - dspcntr |= DISPLAY_PLANE_ENABLE; - if (IS_HASWELL(dev) || IS_BROADWELL(dev)) - dspcntr |= DISPPLANE_PIPE_CSC_ENABLE; - switch (fb->pixel_format) { case DRM_FORMAT_C8: dspcntr |= DISPPLANE_8BPP; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 04/10] iommu/vt-d: Assume BIOS lies about ATSR for integrated gfx
On Tue, 2015-10-13 at 22:34 +0100, David Woodhouse wrote: > If the device itself reports ATS in its PCIe capabilities, but the BIOS > neglects to provide an ATSR structure to indicate that the root port can > also cope, then assume the latter is lying. Except, of course, that integrated devices aren't behind a root port. I think this should fix it the correct way? diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c index 4f2eab6..70a031d 100644 --- a/drivers/iommu/intel-iommu.c +++ b/drivers/iommu/intel-iommu.c @@ -4280,14 +4280,17 @@ int dmar_find_matched_atsr_unit(struct pci_dev *dev) dev = pci_physfn(dev); for (bus = dev->bus; bus; bus = bus->parent) { bridge = bus->self; - if (!bridge || !pci_is_pcie(bridge) || + /* If it's an integrated device, allow ATS */ + if (!bridge) + return 1; + /* Connected via non-PCIe: no ATS */ + if (!pci_is_pcie(bridge) || pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE) return 0; + /* If we found the root port, look it up in the ATSR */ if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) break; } - if (!bridge) - return 0; rcu_read_lock(); list_for_each_entry_rcu(atsru, _atsr_units, list) { -- David WoodhouseOpen Source Technology Centre david.woodho...@intel.com Intel Corporation smime.p7s Description: S/MIME cryptographic signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFCv2 DP-typeC 1/6] drm/i915/dp: Reuse encoder if it is already available
We do not need to loop through crtc_state to get the encoder if we already have a valid one available. Signed-off-by: Durgadoss R--- drivers/gpu/drm/i915/intel_ddi.c | 11 --- drivers/gpu/drm/i915/intel_display.c | 2 +- drivers/gpu/drm/i915/intel_drv.h | 3 ++- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index b25e99a..9098c12 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1762,11 +1762,16 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc, * function should be folded into compute_config() eventually. */ bool intel_ddi_pll_select(struct intel_crtc *intel_crtc, - struct intel_crtc_state *crtc_state) + struct intel_crtc_state *crtc_state, + struct intel_encoder *valid_encoder) { struct drm_device *dev = intel_crtc->base.dev; - struct intel_encoder *intel_encoder = - intel_ddi_get_crtc_new_encoder(crtc_state); + struct intel_encoder *intel_encoder; + + if (valid_encoder) + intel_encoder = valid_encoder; + else + intel_encoder = intel_ddi_get_crtc_new_encoder(crtc_state); if (IS_SKYLAKE(dev)) return skl_ddi_pll_select(intel_crtc, crtc_state, diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index cddb0c6..8ae6d7b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9661,7 +9661,7 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state) static int haswell_crtc_compute_clock(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) { - if (!intel_ddi_pll_select(crtc, crtc_state)) + if (!intel_ddi_pll_select(crtc, crtc_state, NULL)) return -EINVAL; crtc->lowfreq_avail = false; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 91b6b40..0822ab6 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -989,7 +989,8 @@ void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv, void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc); void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc); bool intel_ddi_pll_select(struct intel_crtc *crtc, - struct intel_crtc_state *crtc_state); + struct intel_crtc_state *crtc_state, + struct intel_encoder *encoder); void intel_ddi_set_pipe_settings(struct drm_crtc *crtc); void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder); bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector); -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFCv2 DP-typeC 3/6] drm/i915/dp: Abstract all get_ddi_pll methods
This patch wraps the get_ddi_pll() methods for SKL/BXT/HSW+ with a common intel_get_ddi_pll() method, and exports it, so that it can be shared by other users also. Signed-off-by: Durgadoss R--- drivers/gpu/drm/i915/intel_display.c | 18 -- drivers/gpu/drm/i915/intel_drv.h | 2 ++ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5c2b4ce..e3f1201 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9738,6 +9738,17 @@ static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv, } } +void intel_get_ddi_pll(struct drm_i915_private *dev_priv, enum port port, + struct intel_crtc_state *pipe_config) +{ + if (IS_SKYLAKE(dev_priv)) + skylake_get_ddi_pll(dev_priv, port, pipe_config); + else if (IS_BROXTON(dev_priv)) + bxt_get_ddi_pll(dev_priv, port, pipe_config); + else + haswell_get_ddi_pll(dev_priv, port, pipe_config); +} + static void haswell_get_ddi_port_state(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config) { @@ -9751,12 +9762,7 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc, port = (tmp & TRANS_DDI_PORT_MASK) >> TRANS_DDI_PORT_SHIFT; - if (IS_SKYLAKE(dev)) - skylake_get_ddi_pll(dev_priv, port, pipe_config); - else if (IS_BROXTON(dev)) - bxt_get_ddi_pll(dev_priv, port, pipe_config); - else - haswell_get_ddi_pll(dev_priv, port, pipe_config); + intel_get_ddi_pll(dev_priv, port, pipe_config); if (pipe_config->shared_dpll >= 0) { pll = _priv->shared_dplls[pipe_config->shared_dpll]; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index cbcfa14..ee36ddf 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -991,6 +991,8 @@ void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc); bool intel_ddi_pll_select(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state, struct intel_encoder *encoder, bool find_dpll); +void intel_get_ddi_pll(struct drm_i915_private *dev_priv, enum port port, + struct intel_crtc_state *pipe_config); void intel_ddi_set_pipe_settings(struct drm_crtc *crtc); void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder); bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector); -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFCv2 DP-typeC 0/6] Add support for USB typeC based DP
This is an RFC series to start the review/discussion on approach to support USB type C based DP panel. To support USB type C alternate DP mode, the display driver needs to know the number of lanes required by the DP panel as well as number of lanes that can be supported by the type-C cable. Sometimes, the type-C cable may limit the bandwidth even if Panel can support more lanes. The goal is to find out the number of lanes which can be supported using a particular cable so that we can cap 'max_available_lanes' to that number during modeset. The BXT patches are based on 4.2-rc2 and tested on BXT A1. CHV implementation is tested on 3.18. Both are tested only in non-MST mode. The 1/6 to 4/6 are refactoring/exporting functions required to do upfront link train for DDI platforms from intel_ddi.c. Patch 5/6 is upfront implementation for BXT and 6/6 is for CHV. Brief summary of the approach taken: --- 1.As soon as DP-hotplug is detected, driver starts link training with highest number of lanes/bandwidth possible. If it fails, driver retries link training with lane/2 for same bandwidth. We continue this procedure until we find a working configuration of lane/bandwidth values. This 'number of lanes' is then set as the 'max_available_lanes', so that the following intel_dp_compute_config() during modeset picks it up as max_lane_count (instead of 4 always, from DPCD). 2.Since we do only link training on hotplug, only the port and its PLLs are enabled/disabled without touching pipe/ planes etc. 3.For scenarios where we boot with DP connected (along with an LFP like MIPI/eDP) we disable the crtc and then start link training, since BIOS brings up DP. The crtc is enabled back during subsequent modeset. This needs some changes for latest -nightly branch since we do not have intel_crtc_control() anymore. 4.Since we are doing the link training on hotplug (as opposed to during modeset) we named the function '{chv/bxt/*}_upfront_link_train'. We can also think of a virtual func for this, inside intel_encoder. As per Daniel's suggestion on RFCv1: * Added the last patch 6/6 that has implementation for CHV * Made intel_dp_upfront_link_train as common for all platforms and added a intel_ddi_upfront_* for all DDI platforms. Currently, have restricted it to only for SKL/BXT. * Moved the upfront code for DDI platforms into intel_ddi.c from display.c, since that aligned better with other ddi* functions. * Kept the CHV implementation in display.c as of now since we are using some pll functions defined in display.c We can discuss and finalize an appropriate place for this and then refactor/export required functions. Durgadoss R (6): drm/i915/dp: Reuse encoder if it is already available drm/i915/dp: Reuse shared DPLL if it exists already drm/i915/dp: Abstract all get_ddi_pll methods drm/i915/dp: Export enable/disable_shared_dpll methods drm/i915/dp: Enable Upfront link training for typeC DP support on BXT drm/i915/dp: Enable Upfront link training for typeC DP support on CHV drivers/gpu/drm/i915/intel_ddi.c | 231 ++- drivers/gpu/drm/i915/intel_display.c | 159 ++-- drivers/gpu/drm/i915/intel_dp.c | 43 ++- drivers/gpu/drm/i915/intel_drv.h | 11 +- 4 files changed, 401 insertions(+), 43 deletions(-) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFCv2 DP-typeC 5/6] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT
To support USB type C alternate DP mode, the display driver needs to know the number of lanes required by the DP panel as well as number of lanes that can be supported by the type-C cable. Sometimes, the type-C cable may limit the bandwidth even if Panel can support more lanes. To address these scenarios, the display driver will start link training with max lanes, and if that fails, the driver falls back to x2 lanes; and repeats this procedure for all bandwidth/lane configurations. * Since link training is done before modeset only the port (and not pipe/planes) and its associated PLLs are enabled. * Once link training is done, the port and its PLLs are disabled; so that the subsequent modeset is not aware of these changes. * On DP hotplug: Directly start link training on the crtc associated with the DP encoder. * On Connected boot scenarios: When booted with an LFP and a DP, typically, BIOS brings up DP. In these cases, we disable the crtc first and then start upfront link training. The crtc is re-enabled as part of a subsequent modeset. * For BXT, ddi->enable/disable for DP only enable/disable audio codec and hence are not included in upfront link training sequence. * As of now, this is tested only on BXT A1 platform, on kernel 4.2-rc2. Signed-off-by: Durgadoss R--- drivers/gpu/drm/i915/intel_ddi.c | 152 +++ drivers/gpu/drm/i915/intel_dp.c | 41 ++- drivers/gpu/drm/i915/intel_drv.h | 2 + 3 files changed, 194 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 8e4ea36..b3a9bff 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -3209,6 +3209,158 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port) return connector; } +bool intel_ddi_upfront_link_train(struct drm_device *dev, + struct intel_dp *intel_dp, struct intel_crtc *crtc) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); + struct intel_connector *connector = intel_dp->attached_connector; + struct intel_encoder *tmp_encoder, *encoder = connector->encoder; + struct intel_shared_dpll *pll; + struct intel_crtc *tmp_crtc; + struct drm_crtc *tmp_drm_crtc; + uint8_t tmp_lane_count, tmp_link_bw; + bool ret, found, valid_crtc = false; + + /* For now, we have only SKL and BXT supporting type-C */ + if (!IS_BROXTON(dev) || !IS_SKYLAKE(dev)) + return true; + + if (!connector || !encoder) { + DRM_DEBUG_KMS("dp connector/encoder is NULL\n"); + return false; + } + + /* If we already have a crtc, start link training directly */ + if (crtc) { + valid_crtc = true; + goto start_link_train; + } + + /* Find an unused crtc and use it for link training */ + for_each_intel_crtc(dev, crtc) { + if (intel_crtc_active(>base)) + continue; + + /* Make sure the new crtc will work with the encoder */ + if (drm_encoder_crtc_ok(>base, >base)) { + found = true; + + /* Save the existing values */ + tmp_encoder = connector->new_encoder; + tmp_crtc = encoder->new_crtc; + tmp_drm_crtc = encoder->base.crtc; + + connector->new_encoder = encoder; + encoder->new_crtc = crtc; + encoder->base.crtc = >base; + + break; + } + } + + if (!found) { + DRM_ERROR("Could not find crtc for upfront link training\n"); + return false; + } + +start_link_train: + DRM_DEBUG_KMS("upfront link train on pipe:%c\n", pipe_name(crtc->pipe)); + found = false; + + /* Save the existing lane_count and link_bw values */ + tmp_lane_count = intel_dp->lane_count; + tmp_link_bw = intel_dp->link_bw; + + /* Initialize with Max Link rate & lane count supported by panel */ + intel_dp->link_bw = intel_dp->dpcd[DP_MAX_LINK_RATE]; + intel_dp->lane_count = intel_dp->dpcd[DP_MAX_LANE_COUNT] & + DP_MAX_LANE_COUNT_MASK; + + /* Selects the shared DPLL to use for this port */ + intel_get_ddi_pll(dev_priv, dig_port->port, crtc->config); + pll = intel_crtc_to_shared_dpll(crtc); + if (!pll) { + DRM_ERROR("Could not get shared DPLL\n"); + goto exit; + } + + do { + /* Find port clock from link_bw */ + crtc->config->port_clock = + drm_dp_bw_code_to_link_rate(intel_dp->link_bw); + + ret = intel_ddi_pll_select(crtc,
[Intel-gfx] [RFCv2 DP-typeC 4/6] drm/i915/dp: Export enable/disable_shared_dpll methods
This patch exports the intel_{enable/disable}_shared_dpll methods so that they can be called from other files also. Subsequent patches need to call this from intel_ddi.c Signed-off-by: Durgadoss R--- drivers/gpu/drm/i915/intel_display.c | 4 ++-- drivers/gpu/drm/i915/intel_drv.h | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e3f1201..a2ec8cb 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1910,7 +1910,7 @@ static void intel_prepare_shared_dpll(struct intel_crtc *crtc) * The PCH PLL needs to be enabled before the PCH transcoder, since it * drives the transcoder clock. */ -static void intel_enable_shared_dpll(struct intel_crtc *crtc) +void intel_enable_shared_dpll(struct intel_crtc *crtc) { struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; @@ -1940,7 +1940,7 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc) pll->on = true; } -static void intel_disable_shared_dpll(struct intel_crtc *crtc) +void intel_disable_shared_dpll(struct intel_crtc *crtc) { struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index ee36ddf..5bcdd37 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1116,6 +1116,8 @@ void intel_create_rotation_property(struct drm_device *dev, /* shared dpll functions */ struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc); +void intel_enable_shared_dpll(struct intel_crtc *crtc); +void intel_disable_shared_dpll(struct intel_crtc *crtc); void assert_shared_dpll(struct drm_i915_private *dev_priv, struct intel_shared_dpll *pll, bool state); -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFCv2 DP-typeC 2/6] drm/i915/dp: Reuse shared DPLL if it exists already
Do not call intel_get_shared_dpll() if there exists a valid shared DPLL already. Signed-off-by: Durgadoss R--- drivers/gpu/drm/i915/intel_ddi.c | 70 drivers/gpu/drm/i915/intel_display.c | 2 +- drivers/gpu/drm/i915/intel_drv.h | 2 +- 3 files changed, 42 insertions(+), 32 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 9098c12..8e4ea36 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1258,7 +1258,8 @@ hsw_ddi_calculate_wrpll(int clock /* in Hz */, static bool hsw_ddi_pll_select(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state, - struct intel_encoder *intel_encoder) + struct intel_encoder *intel_encoder, + bool find_dpll) { int clock = crtc_state->port_clock; @@ -1278,14 +1279,16 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc, crtc_state->dpll_hw_state.wrpll = val; - pll = intel_get_shared_dpll(intel_crtc, crtc_state); - if (pll == NULL) { - DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", -pipe_name(intel_crtc->pipe)); - return false; - } + if (find_dpll) { + pll = intel_get_shared_dpll(intel_crtc, crtc_state); + if (pll == NULL) { + DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", +pipe_name(intel_crtc->pipe)); + return false; + } - crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id); + crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id); + } } return true; @@ -1540,7 +1543,8 @@ skip_remaining_dividers: static bool skl_ddi_pll_select(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state, - struct intel_encoder *intel_encoder) + struct intel_encoder *intel_encoder, + bool find_dpll) { struct intel_shared_dpll *pll; uint32_t ctrl1, cfgcr1, cfgcr2; @@ -1594,15 +1598,17 @@ skl_ddi_pll_select(struct intel_crtc *intel_crtc, crtc_state->dpll_hw_state.cfgcr1 = cfgcr1; crtc_state->dpll_hw_state.cfgcr2 = cfgcr2; - pll = intel_get_shared_dpll(intel_crtc, crtc_state); - if (pll == NULL) { - DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", -pipe_name(intel_crtc->pipe)); - return false; - } + if (find_dpll) { + pll = intel_get_shared_dpll(intel_crtc, crtc_state); + if (pll == NULL) { + DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", +pipe_name(intel_crtc->pipe)); + return false; + } - /* shared DPLL id 0 is DPLL 1 */ - crtc_state->ddi_pll_sel = pll->id + 1; + /* shared DPLL id 0 is DPLL 1 */ + crtc_state->ddi_pll_sel = pll->id + 1; + } return true; } @@ -1632,7 +1638,8 @@ static const struct bxt_clk_div bxt_dp_clk_val[] = { static bool bxt_ddi_pll_select(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state, - struct intel_encoder *intel_encoder) + struct intel_encoder *intel_encoder, + bool find_pll) { struct intel_shared_dpll *pll; struct bxt_clk_div clk_div = {0}; @@ -1741,15 +1748,17 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc, crtc_state->dpll_hw_state.pcsdw12 = LANESTAGGER_STRAP_OVRD | lanestagger; - pll = intel_get_shared_dpll(intel_crtc, crtc_state); - if (pll == NULL) { - DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", - pipe_name(intel_crtc->pipe)); - return false; - } + if (find_pll) { + pll = intel_get_shared_dpll(intel_crtc, crtc_state); + if (pll == NULL) { + DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", + pipe_name(intel_crtc->pipe)); + return false; + } - /* shared DPLL id 0 is DPLL A */ - crtc_state->ddi_pll_sel = pll->id; + /* shared DPLL id 0 is DPLL A */ + crtc_state->ddi_pll_sel = pll->id; + } return true; } @@ -1763,7 +1772,8 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc, */ bool intel_ddi_pll_select(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state, - struct
[Intel-gfx] [RFCv2 DP-typeC 6/6] drm/i915/dp: Enable Upfront link training for typeC DP support on CHV
To support USB type C alternate DP mode, the display driver needs to know the number of lanes required by DP panel as well as number of lanes that can be supported by the type-C cable. Sometimes, the type-C cable may limit the bandwidth even if Panel can support more lanes. To address these scenarios, the display driver will start link training with max lanes, and if the link training fails, the driver then falls back to x2 lanes. * Since link training is done before modeset, planes are not enabled. Only encoder and the its associated PLLs are enabled. * Once link training is done, the encoder and its PLLs are disabled; so that the subsequent modeset is not aware of these changes. * As of now, this is tested only on CHV. Signed-off-by: Durgadoss R--- drivers/gpu/drm/i915/intel_display.c | 135 +++ drivers/gpu/drm/i915/intel_dp.c | 2 + drivers/gpu/drm/i915/intel_drv.h | 2 + 3 files changed, 139 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a2ec8cb..17b3c7a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15751,3 +15751,138 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file) spin_unlock_irq(>event_lock); } } + +bool chv_upfront_link_train(struct drm_device *dev, + struct intel_dp *intel_dp, struct intel_crtc *crtc) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_connector *connector = intel_dp->attached_connector; + struct intel_encoder *tmp_encoder, *encoder = connector->encoder; + struct intel_crtc *tmp_crtc; + struct drm_crtc *tmp_drm_crtc; + bool found = false, valid_crtc = false; + uint8_t tmp_lane_count, tmp_link_bw; + + if (!connector || !encoder) { + DRM_DEBUG_KMS("dp connector/encoder is NULL\n"); + return false; + } + + /* If we already have a crtc, start link training directly */ + if (crtc) { + valid_crtc = true; + goto start_link_train; + } + + /* Find an unused crtc and use it for link training */ + for_each_intel_crtc(dev, crtc) { + if (intel_crtc_active(>base)) + continue; + + /* Make sure the new crtc will work with the encoder */ + if (drm_encoder_crtc_ok(>base, >base)) { + /* Save the existing values */ + tmp_encoder = connector->new_encoder; + tmp_crtc = encoder->new_crtc; + tmp_drm_crtc = encoder->base.crtc; + + connector->new_encoder = encoder; + encoder->new_crtc = crtc; + encoder->base.crtc = >base; + + found = true; + break; + } + } + + if (!found) { + DRM_ERROR("Could not find crtc for upfront link training\n"); + return false; + } + +start_link_train: + + DRM_DEBUG_KMS("upfront link training on pipe:%c\n", + pipe_name(crtc->pipe)); + found = false; + + /* Save the existing lane_count and link_bw values */ + tmp_lane_count = intel_dp->lane_count; + tmp_link_bw = intel_dp->link_bw; + + /* Initialize with Max Link rate & lane count supported by panel */ + intel_dp->link_bw = intel_dp->dpcd[DP_MAX_LINK_RATE]; + intel_dp->lane_count = intel_dp->dpcd[DP_MAX_LANE_COUNT] & + DP_MAX_LANE_COUNT_MASK; + + /* CHV does not support HBR2 */ + if (intel_dp->link_bw == DP_LINK_BW_5_4) + intel_dp->link_bw = DP_LINK_BW_2_7; + + do { + /* Find port clock from link_bw */ + crtc->config.port_clock = + drm_dp_bw_code_to_link_rate(intel_dp->link_bw); + + /* Enable PLL followed by port */ + intel_dp_set_clock(encoder, >config, intel_dp->link_bw); + chv_update_pll(crtc); + encoder->pre_pll_enable(encoder); + chv_enable_pll(crtc); + encoder->pre_enable(encoder); + + /* Check if link training passed; if so update lane count */ + /* TODO: Newer code has this variable as 'train_set_valid */ + if (intel_dp->has_fast_link_train) { + intel_dp->dpcd[DP_MAX_LANE_COUNT] &= + ~DP_MAX_LANE_COUNT_MASK; + intel_dp->dpcd[DP_MAX_LANE_COUNT] |= + intel_dp->lane_count & DP_MAX_LANE_COUNT_MASK; + + found = true; + } + + /* Reset encoder for next retry or for clean up */ +
Re: [Intel-gfx] [PATCH i-g-t] core_prop_blob ioctl_wrappers: Fix new tests/benchmarks for android
On 13 October 2015 at 16:35, Daniel Vetterwrote: > On Tue, Oct 13, 2015 at 04:16:18PM +0100, Derek Morton wrote: >> Changes since #1b492e311 have broken the Android build. This patch >> fixes the build for Android. >> >> core_prop_blob was using ioctls not in the android kernel. Added a >> igt_require_propblob() function and local defines/structures so the >> test will compile and skip on kernels where the feature is unsupported. >> >> gem_blt - included igt.h >> >> Signed-off-by: Derek Morton >> --- >> benchmarks/gem_blt.c | 4 +--- >> lib/ioctl_wrappers.c | 13 + >> lib/ioctl_wrappers.h | 22 ++ >> tests/core_prop_blob.c | 37 ++--- >> 4 files changed, 54 insertions(+), 22 deletions(-) >> >> diff --git a/benchmarks/gem_blt.c b/benchmarks/gem_blt.c >> index 181a5f1..8ab5302 100644 >> --- a/benchmarks/gem_blt.c >> +++ b/benchmarks/gem_blt.c The gem_blt changes are unrelated to the rest of the patch, so need to be applied separately. >> @@ -25,6 +25,7 @@ >> * >> */ >> >> +#include "igt.h" >> #include >> #include >> #include >> @@ -39,9 +40,6 @@ >> #include >> >> #include "drm.h" >> -#include "ioctl_wrappers.h" >> -#include "drmtest.h" >> -#include "intel_chipset.h" >> >> #define LOCAL_I915_EXEC_NO_RELOC (1<<11) >> #define LOCAL_I915_EXEC_HANDLE_LUT (1<<12) >> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c >> index 80e1ec6..cf34f25 100644 >> --- a/lib/ioctl_wrappers.c >> +++ b/lib/ioctl_wrappers.c >> @@ -1219,6 +1219,19 @@ void igt_require_fb_modifiers(int fd) >> igt_require(has_modifiers); >> } >> >> +void igt_require_propblob(int fd) >> +{ >> + struct local_drm_mode_create_blob c; >> + struct local_drm_mode_destroy_blob d; >> + uint32_t blob_data; >> + c.data = _data; >> + c.length = sizeof(blob_data); >> + >> + igt_require(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_CREATEPROPBLOB, ) == >> 0); >> + d.blob_id = c.blob_id; >> + igt_require(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_DESTROYPROPBLOB, ) == >> 0); >> +} > > If you want to do this in the library the usual way is to wrap the ioctls > as functions and put the relevant -ENOTTY check in there as an > igt_require, followed by an igt_assert for anything else that might have > gone wrong. > > I'd just keep this in the test as a static function though, since then you > don't have to write api docs ;-) It's also unlikely to be used elsewhere at the moment, so a local define and static function in the test is fine. > -Daniel > >> + >> int __kms_addfb(int fd, uint32_t handle, uint32_t width, uint32_t height, >> uint32_t stride, uint32_t pixel_format, uint64_t modifier, >> uint32_t flags, uint32_t *buf_id) >> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h >> index f4deca6..aeb224c 100644 >> --- a/lib/ioctl_wrappers.h >> +++ b/lib/ioctl_wrappers.h >> @@ -149,6 +149,20 @@ struct local_drm_mode_fb_cmd2 { >> uint64_t modifier[4]; >> }; >> >> +struct local_drm_mode_get_blob { >> + uint32_t blob_id; >> + uint32_t length; >> + uint64_t data; >> +}; >> +struct local_drm_mode_create_blob { >> + uint64_t data; >> + uint32_t length; >> + uint32_t blob_id; >> +}; >> +struct local_drm_mode_destroy_blob { >> + uint32_t blob_id; >> +}; >> + >> #define LOCAL_DRM_MODE_FB_MODIFIERS (1<<1) >> >> #define LOCAL_DRM_FORMAT_MOD_VENDOR_INTEL0x01 >> @@ -165,9 +179,17 @@ struct local_drm_mode_fb_cmd2 { >> #define LOCAL_DRM_IOCTL_MODE_ADDFB2 DRM_IOWR(0xB8, \ >>struct local_drm_mode_fb_cmd2) >> >> +#define LOCAL_DRM_IOCTL_MODE_GETPROPBLOB DRM_IOWR(0xAC, \ >> + struct local_drm_mode_get_blob) >> +#define LOCAL_DRM_IOCTL_MODE_CREATEPROPBLOB DRM_IOWR(0xBD, \ >> + struct >> local_drm_mode_create_blob) >> +#define LOCAL_DRM_IOCTL_MODE_DESTROYPROPBLOB DRM_IOWR(0xBE, \ >> + struct >> local_drm_mode_destroy_blob) >> + >> #define LOCAL_DRM_CAP_ADDFB2_MODIFIERS 0x10 >> >> void igt_require_fb_modifiers(int fd); >> +void igt_require_propblob(int fd); >> >> /** >> * __kms_addfb: >> diff --git a/tests/core_prop_blob.c b/tests/core_prop_blob.c >> index d704158..ff56482 100644 >> --- a/tests/core_prop_blob.c >> +++ b/tests/core_prop_blob.c >> @@ -25,16 +25,12 @@ >> * Daniel Stone >> */ >> >> +#include "igt.h" >> #include >> #include >> #include >> #include >> >> -#include "drmtest.h" >> -#include "igt_debugfs.h" >> -#include "igt_kms.h" >> -#include "igt_aux.h" >> - >> IGT_TEST_DESCRIPTION("Tests behaviour of mass-data 'blob' properties."); >> >> static const struct drm_mode_modeinfo test_mode_valid = { >> @@ -64,19 +60,19 @@ static const struct drm_mode_modeinfo test_mode_valid = { >> static int >>
Re: [Intel-gfx] [BXT DSI timing fixes v1 3/3] drm/i915/bxt: Fixed dsi enc disable and blank at bootup
>-Original Message- >From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter >Sent: Tuesday, October 13, 2015 9:17 PM >To: Shankar, Uma >Cc: intel-gfx@lists.freedesktop.org; Kumar, Shobhit >Subject: Re: [Intel-gfx] [BXT DSI timing fixes v1 3/3] drm/i915/bxt: Fixed dsi >enc >disable and blank at bootup > >On Mon, Oct 12, 2015 at 10:55:03PM +0530, Uma Shankar wrote: >> During bootup, mipi display was blanking in BXT. This is because >> during driver load, though encoder, connector were active but crtc >> returned inactive. This caused sanitize function to disable the DSI >> panel. In AOS, this is fine since HWC will do a modeset and crtc, >> connector, encoder mapping will be restored. But in Charging OS, no >> modeset is called, it just calls DPMS ON/OFF. Hence display doesn't >> come in COS. This is needed even for seamless booting to Android without a >blank. >> >> This is fine on BYT/CHT since transcoder is common b/w all encoders. >> But for BXT, there is a separate mipi transcoder. Hence this needs >> special handling for BXT DSI. >> >> Signed-off-by: Uma Shankar>> --- >> drivers/gpu/drm/i915/i915_drv.h |3 +++ >> drivers/gpu/drm/i915/intel_display.c | 27 +++ >> 2 files changed, 26 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h index bf14096..ae790ec 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1948,6 +1948,9 @@ struct drm_i915_private { >> /* perform PHY state sanity checks? */ >> bool chv_phy_assert[2]; >> >> +/* To check if DSI is initializing at bootup */ >> +bool dsi_enumerating; > >See my other comment, you're working around the broken design of patch 2 >here. Special casing fastboot is a big no-no which needs some really good >reasons. An example is the special edp clock readout code in the encoder >callbacks we have for ilk-ivb cpu edp. >-Daniel > I agree this is not a clean solution but was not getting any better ideas. As part of driver load and initialization, readout_state returns encoder, connector as active but all crtc return inactive. This make sanitize encoder to consider this as inconsistent hw state and it goes ahead and disables encoder, thereby disabling the BIOS/GOP Initialization. After this only way to recover is a modeset which doesn't come in cases like Charging OS in Android. Also this will create issues in having a seamless boot without a blank (a must have for Android devices). For EDP case, we can read DDI_FUNC_CTL and get a pipe number and match it with crtc->pipe to detect EDP is on that pipe and map it to crtc. In DSI there is no such mechanism to detect this. Can you suggest some pointers as to how to approach this issue. Thanks & Regards, Uma Shankar >> + >> /* >> * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch >> * will be rejected. Instead look for a better place. >> diff --git a/drivers/gpu/drm/i915/intel_display.c >> b/drivers/gpu/drm/i915/intel_display.c >> index 2717823..fe4f4f3 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -7711,6 +7711,9 @@ static void intel_get_pipe_timings(struct intel_crtc >*crtc, >> bool is_dsi = intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI); >> uint32_t tmp; >> >> +if (dev_priv->dsi_enumerating && dev_priv->vbt.has_mipi) >> +is_dsi = true; >> + >> tmp = I915_READ(HTOTAL(cpu_transcoder)); >> pipe_config->base.adjusted_mode.crtc_hdisplay = (tmp & 0x) + 1; >> pipe_config->base.adjusted_mode.crtc_htotal = ((tmp >> 16) & 0x) >> + 1; @@ -9825,10 +9828,20 @@ static bool haswell_get_pipe_config(struct >intel_crtc *crtc, >> is_dsi = intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI); >> >> /* >> - * Check if encoder is enabled or not >> + * Check if encoder is enabled or not. >> * Separate implementation for DSI and DDI encoders. >> + * For first time during driver init, encoder association >> + * would not have happened and this function will return >> + * false. This will cause encoder to be disabled causing >> + * a blank, till user space does a modeset. In order to avoid >> + * this, if DSI is enabled in VBT, for first iteration, this >> + * will return true since BIOS would have initialized MIPI. >> + * This is needed for seamless booting without blanking and >> + * for Charging OS scenario. Since DSI is the first display in >> + * setup_outputs, hence first crtc will be associated with mipi >> + * display >> */ >> -if (is_dsi) { >> +if (is_dsi || (dev_priv->dsi_enumerating && dev_priv->vbt.has_mipi)) >> +{ >> struct intel_encoder *encoder; >> >> for_each_encoder_on_crtc(dev, >base, encoder) { @@ - >9852,8 >> +9865,11 @@ static bool
Re: [Intel-gfx] [PATCH v2] drm/i915: Drop i915_gem_obj_is_pinned() from set-cache-level
On Tue, Oct 13, 2015 at 03:52:57PM +0200, Daniel Vetter wrote: > On Fri, Oct 09, 2015 at 02:43:21PM +0100, Tvrtko Ursulin wrote: > > > > On 09/10/15 14:11, Chris Wilson wrote: > > >Since the remove of the pin-ioctl, we only care about not changing the > > >cache level on buffers pinned to the hardware as indicated by > > >obj->pin_display. By knowing that only objects pinned to the hardware > > >will have an elevated vma->pin_count, so we can coallesce many of the > > >linear walks over the obj->vma_list. > > > > > >v2: Try and retrospectively add comments explaining the steps in > > >rebinding the active VMA. > > > > > >Signed-off-by: Chris Wilson> > >Cc: Tvrtko Ursulin > > > > Very nice! > > > > Reviewed-by: Tvrtko Ursulin > > Queued for -next, thanks for the patch. Note you also want drm/i915: Move the mb() following release-mmap into release-mmap http://patchwork.freedesktop.org/patch/61128/ to answer the question you posed in 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] drm/i915: Move the mb() following release-mmap into release-mmap
On Tue, Oct 06, 2015 at 03:40:02PM +0100, Tvrtko Ursulin wrote: > > > Hi, > > On 06/10/15 12:58, Chris Wilson wrote: > >As paranoia, we want to ensure that the CPU's PTEs have been revoked for > >the object before we return from i915_gem_release_mmap(). This allows us > >to rely on there being no outstanding memory accesses and guarantees > >serialisation of the code against concurrent access just by calling > >i915_gem_release_mmap(). > > > >Signed-off-by: Chris Wilson> >--- > > drivers/gpu/drm/i915/i915_gem.c | 13 ++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem.c > >b/drivers/gpu/drm/i915/i915_gem.c > >index 2b8ed7a2faab..642644f12295 100644 > >--- a/drivers/gpu/drm/i915/i915_gem.c > >+++ b/drivers/gpu/drm/i915/i915_gem.c > >@@ -1877,11 +1877,21 @@ out: > > void > > i915_gem_release_mmap(struct drm_i915_gem_object *obj) > > { > >+/* Serialisation between user GTT access and our code depends upon > >+ * revoking the CPU's PTE whilst the mutex is held. The next user > >+ * pagefault then has to wait until we release the mutex. > >+ */ > >+lockdep_assert_held(>base.dev->struct_mutex); > >+ > > if (!obj->fault_mappable) > > return; > > > > drm_vma_node_unmap(>base.vma_node, > >obj->base.dev->anon_inode->i_mapping); > >+ > >+/* Ensure that the CPU's PTE are revoked before we return */ > >+mb(); > >+ > > smp_mb() or smp_wmb() would not suffice? Is it needed on uniprocessor? Correct, smp_mb() would not suffice as we are serialised accessing through a mmio channel with the PTE writes. A wmb() may suffice though, but that actually changed code :) -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 1/2] drm/i915: Call encoder hotplug for init and resume cases
Had some discussion with Daniel on IRC about how we can fix the deadlock. We couldn't decide upon any solution but I can give it a try. He suggested to have a work function for ->hot_plug which will get the power domain lock and not allow ->hot_plug if the power_domain is shut. Then when it calls hpt_init again from power_domain get, ->hot_plug gets called. But this looks racy, we can try this out. If there are any other suggestions, please let me know. I will work on this after 26th Oct. Regards, Sonika -Original Message- From: Sharma, Shashank Sent: Monday, October 12, 2015 5:54 PM To: Ville Syrjälä; Vetter, Daniel Cc: intel-gfx@lists.freedesktop.org; Mukherjee, Indranil; Jindal, Sonika Subject: RE: [Intel-gfx] [PATCH 1/2] drm/i915: Call encoder hotplug for init and resume cases We were debugging this issue, and we could find the root cause: In function: Intel_hpd_init() | encoder->hotplug() | display_power_get() | Intel_powe_well_enable() | power_well->ops->enable() | chv_pipe_power_well_enable() | vlv_display_power_well_init() | intel_hdp_init() This function ends up calling intel_hpd_init, Which is causing the mutex deadlock due to recursion, as we are calling encoder->hotplug() from hpd_init(). Intel_hpd_init() | encoder->hotplug() | display_power_get() There are two solutions here: - remove hpd_init() from get_calls - remove encoder->hotplug() function call from hpd_init() and put it in some other place. We have added this function from encoder_init() for android trees, and it works well there. Regards Shashank -Original Message- From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of Ville Syrjälä Sent: Thursday, October 08, 2015 7:06 PM To: Jindal, Sonika Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Call encoder hotplug for init and resume cases On Mon, Oct 05, 2015 at 04:43:14PM +0530, Sonika Jindal wrote: > For all the encoders, call the hot_plug if it is registered. > This is required for connected boot and resume cases to generate fake > hpd resulting in reading of edid. > Removing the initial sdvo hot_plug call too so that it will be called > just once from this loop. > > Signed-off-by: Sonika Jindal> --- > drivers/gpu/drm/i915/intel_hotplug.c | 11 +++ > drivers/gpu/drm/i915/intel_sdvo.c|1 - > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c > b/drivers/gpu/drm/i915/intel_hotplug.c > index 53c0173..eac4757 100644 > --- a/drivers/gpu/drm/i915/intel_hotplug.c > +++ b/drivers/gpu/drm/i915/intel_hotplug.c > @@ -458,6 +458,7 @@ void intel_hpd_init(struct drm_i915_private > *dev_priv) { > struct drm_device *dev = dev_priv->dev; > struct drm_mode_config *mode_config = >mode_config; > + struct intel_encoder *encoder; > struct drm_connector *connector; > int i; > > @@ -482,6 +483,16 @@ void intel_hpd_init(struct drm_i915_private *dev_priv) > if (dev_priv->display.hpd_irq_setup) > dev_priv->display.hpd_irq_setup(dev); > spin_unlock_irq(_priv->irq_lock); > + > + /* > + * Connected boot / resume scenarios can't generate new hot plug. > + * So, probe it manually. > + */ > + list_for_each_entry(encoder, >mode_config.encoder_list, > + base.head) { > + if (encoder->hot_plug) > + encoder->hot_plug(encoder); > + } This breaks the world on CHV [ 3187.575198] [drm:intel_hdmi_hot_plug] Live status not up! [ 3187.585154] = [ 3187.595010] [ INFO: possible recursive locking detected ] [ 3187.604685] 4.3.0-rc4-bsw+ #2488 Tainted: G U W [ 3187.614366] - [ 3187.623892] Xorg/32212 is trying to acquire lock: [ 3187.632635] (_domains->lock){+.+...}, at: [] intel_display_power_get+0x38/0xcb [i915] [ 3187.647492] [ 3187.647492] but task is already holding lock: [ 3187.661054] (_domains->lock){+.+...}, at: [] intel_display_power_get+0x38/0xcb [i915] [ 3187.675960] [ 3187.675960] other info that might help us debug this: [ 3187.690459] Possible unsafe locking scenario: [ 3187.690459] [ 3187.704224]CPU0 [ 3187.710485] [ 3187.716711] lock(_domains->lock); [ 3187.724718] lock(_domains->lock); [ 3187.732663] [ 3187.732663] *** DEADLOCK *** [ 3187.732663] [ 3187.749460] May be due to missing lock nesting notation [ 3187.749460] [ 3187.763833] 5 locks held by Xorg/32212: [ 3187.771523] #0: (drm_global_mutex){+.+.+.}, at: [] drm_release+0x3b/0x49b [drm] [ 3187.785216] #1: (>mode_config.mutex){+.+.+.}, at: [] drm_modeset_lock_all+0x54/0xcd [drm] [ 3187.800437] #2: (crtc_ww_class_acquire){+.+.+.}, at: [] drm_modeset_lock_all+0x5e/0xcd [drm] [ 3187.815488] #3:
Re: [Intel-gfx] [PATCH] drm/i915: respect previous reg values on primary plane disable
On Wed, Oct 14, 2015 at 01:12:27PM +0100, Chris Wilson wrote: > On Wed, Oct 14, 2015 at 03:07:41PM +0300, Ville Syrjälä wrote: > > On Tue, Oct 13, 2015 at 02:24:41PM -0700, Kevin Strasser wrote: > > > On HSW the crc differs between black and disabled primary planes, causing > > > an > > > assert to fail in the kms_universal_plane test. It seems that things like > > > gamma > > > correction are causing the black primary plane case to result in a > > > brighter > > > color than the disabled primary plane case. > > > > > > Only toggle the enable bit instead of clearing the control register, > > > making the > > > disable path more similar to that of the sprite plane. > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 > > > Testcase: igt/kms_universal_plane > > > Signed-off-by: Kevin Strasser> > > --- > > > drivers/gpu/drm/i915/intel_display.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index cddb0c6..b6164d8e 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -2829,7 +2829,7 @@ static void ironlake_update_primary_plane(struct > > > drm_crtc *crtc, > > > int pixel_size; > > > > > > if (!visible || !fb) { > > > - I915_WRITE(reg, 0); > > > + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); > > > > Eh, what now? We've been trying to eliminate these nasty RMWs. > > > > Are you saying that if we disabled the plane, but leave the "pass plane > > data through gamma" it still affects the output for any pixel "covered" > > by the disabled plane? > > What I thought was being said was that if a plane is set to black (but > with gamma enabled on the pipe) then a different CRC is produced > compared to when the pipe is completely disabled (no plane at all). It > sounded to me like a test case failure. In that case I don't understand how the patch is supposed to help. But yeah, tests like these should really set up an identity gamma and pipe csc matrix. Also we should grow some properties to control whether the plane data passes through the gamma/csc or not. Those could then be used to achieeve the same effect. -- 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] [BXT DSI timing fixes v1 3/3] drm/i915/bxt: Fixed dsi enc disable and blank at bootup
On Wed, Oct 14, 2015 at 10:20:32AM +, Shankar, Uma wrote: > > > >-Original Message- > >From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel > >Vetter > >Sent: Tuesday, October 13, 2015 9:17 PM > >To: Shankar, Uma > >Cc: intel-gfx@lists.freedesktop.org; Kumar, Shobhit > >Subject: Re: [Intel-gfx] [BXT DSI timing fixes v1 3/3] drm/i915/bxt: Fixed > >dsi enc > >disable and blank at bootup > > > >On Mon, Oct 12, 2015 at 10:55:03PM +0530, Uma Shankar wrote: > >> During bootup, mipi display was blanking in BXT. This is because > >> during driver load, though encoder, connector were active but crtc > >> returned inactive. This caused sanitize function to disable the DSI > >> panel. In AOS, this is fine since HWC will do a modeset and crtc, > >> connector, encoder mapping will be restored. But in Charging OS, no > >> modeset is called, it just calls DPMS ON/OFF. Hence display doesn't > >> come in COS. This is needed even for seamless booting to Android without a > >blank. > >> > >> This is fine on BYT/CHT since transcoder is common b/w all encoders. > >> But for BXT, there is a separate mipi transcoder. Hence this needs > >> special handling for BXT DSI. > >> > >> Signed-off-by: Uma Shankar> >> --- > >> drivers/gpu/drm/i915/i915_drv.h |3 +++ > >> drivers/gpu/drm/i915/intel_display.c | 27 +++ > >> 2 files changed, 26 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h > >> b/drivers/gpu/drm/i915/i915_drv.h index bf14096..ae790ec 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.h > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > >> @@ -1948,6 +1948,9 @@ struct drm_i915_private { > >>/* perform PHY state sanity checks? */ > >>bool chv_phy_assert[2]; > >> > >> + /* To check if DSI is initializing at bootup */ > >> + bool dsi_enumerating; > > > >See my other comment, you're working around the broken design of patch 2 > >here. Special casing fastboot is a big no-no which needs some really good > >reasons. An example is the special edp clock readout code in the encoder > >callbacks we have for ilk-ivb cpu edp. > >-Daniel > > > > I agree this is not a clean solution but was not getting any better > ideas. As part of driver load and initialization, readout_state returns > encoder, connector as active but all crtc return inactive. This make > sanitize encoder to consider this as inconsistent hw state and it goes > ahead and disables encoder, thereby disabling the BIOS/GOP > Initialization. > > After this only way to recover is a modeset which doesn't come in cases > like Charging OS in Android. Also this will create issues in having a > seamless boot without a blank (a must have for Android devices). > > For EDP case, we can read DDI_FUNC_CTL and get a pipe number and match > it with crtc->pipe to detect EDP is on that pipe and map it to crtc. > > In DSI there is no such mechanism to detect this. Can you suggest some > pointers as to how to approach this issue. MIPI_CTRL(port) has BXT_PIPE_SELECT_A/C bits. Which btw we don't ever seem to check anywhere in intel_dsi_compute_config, which is a pretty stellar bug that running kms_flip even once should have caught. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation 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: respect previous reg values on primary plane disable
On Tue, Oct 13, 2015 at 02:24:41PM -0700, Kevin Strasser wrote: > On HSW the crc differs between black and disabled primary planes, causing an > assert to fail in the kms_universal_plane test. It seems that things like > gamma > correction are causing the black primary plane case to result in a brighter > color than the disabled primary plane case. > > Only toggle the enable bit instead of clearing the control register, making > the > disable path more similar to that of the sprite plane. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 > Testcase: igt/kms_universal_plane > Signed-off-by: Kevin Strasser> --- > drivers/gpu/drm/i915/intel_display.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index cddb0c6..b6164d8e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2829,7 +2829,7 @@ static void ironlake_update_primary_plane(struct > drm_crtc *crtc, > int pixel_size; > > if (!visible || !fb) { > - I915_WRITE(reg, 0); > + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); Eh, what now? We've been trying to eliminate these nasty RMWs. Are you saying that if we disabled the plane, but leave the "pass plane data through gamma" it still affects the output for any pixel "covered" by the disabled plane? > I915_WRITE(DSPSURF(plane), 0); > POSTING_READ(reg); > return; > -- > 1.9.1 > > ___ > 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: respect previous reg values on primary plane disable
On Wed, Oct 14, 2015 at 03:07:41PM +0300, Ville Syrjälä wrote: > On Tue, Oct 13, 2015 at 02:24:41PM -0700, Kevin Strasser wrote: > > On HSW the crc differs between black and disabled primary planes, causing an > > assert to fail in the kms_universal_plane test. It seems that things like > > gamma > > correction are causing the black primary plane case to result in a brighter > > color than the disabled primary plane case. > > > > Only toggle the enable bit instead of clearing the control register, making > > the > > disable path more similar to that of the sprite plane. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 > > Testcase: igt/kms_universal_plane > > Signed-off-by: Kevin Strasser> > --- > > drivers/gpu/drm/i915/intel_display.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index cddb0c6..b6164d8e 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2829,7 +2829,7 @@ static void ironlake_update_primary_plane(struct > > drm_crtc *crtc, > > int pixel_size; > > > > if (!visible || !fb) { > > - I915_WRITE(reg, 0); > > + I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); > > Eh, what now? We've been trying to eliminate these nasty RMWs. > > Are you saying that if we disabled the plane, but leave the "pass plane > data through gamma" it still affects the output for any pixel "covered" > by the disabled plane? What I thought was being said was that if a plane is set to black (but with gamma enabled on the pipe) then a different CRC is produced compared to when the pipe is completely disabled (no plane at all). It sounded to me like a test case failure. -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] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw
On Wed, Oct 14, 2015 at 11:21:46AM +0300, Ander Conselvan De Oliveira wrote: > On Tue, 2015-10-13 at 16:08 +0200, Daniel Vetter wrote: > > On Tue, Oct 13, 2015 at 04:00:37PM +0200, Maarten Lankhorst wrote: > > > Op 13-10-15 om 15:58 schreef Daniel Vetter: > > > > On Tue, Oct 13, 2015 at 03:43:28PM +0200, Maarten Lankhorst wrote: > > > > > Op 13-10-15 om 15:35 schreef Daniel Vetter: > > > > > > On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst wrote: > > > > > > > Op 23-09-15 om 17:34 schreef Gabriel Feceoru: > > > > > > > > Using 2 connectors (DVI and VGA) will cause wrpll to be set for > > > > > > > > INTEL_OUTPUT_HDMI but never reset if switching to > > > > > > > > INTEL_OUTPUT_VGA > > > > > > > > > > > > > > > > Supresses errors like these: > > > > > > > > [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in > > > > > > > > dpll_hw_state.wrpll > > > > > > > > > > > > > > > Looks like a good idea to always zero it. > > > > > > Except that we still have a bunch of cases where we recompute clock > > > > > > state > > > > > > but only partially. Can we just move them all up into a common place > > > > > > please? That would also catch cases where we simply forget to fill > > > > > > this > > > > > > out at all. > > > > > > > > > > > > One case I noticed is edp in skl_ddi_pll_select, but there's > > > > > > probably > > > > > > more. > > > > > > > > > > > Something like below, with all the memsets for dpll_hw_state removed? > > > > I think this will blow up since we recompute clock state only when > > > > needs_modeset is true. So needs a bit more intelligence in deciding when > > > > to clear it I think. > > > Oops you're right. Maybe intel_modeset_clear_plls because that's where > > > all the clock state > > > belongs? > > > > Yeah that might be an even better place, in the loop after the continue; > > statement. > > The reason I didn't put the memset there in the first place was the way we > calculate plls for DP > with DDI platforms. In that case, ddi_pll_sel is setup from the > encoder_config instead of > compute_clock, so a memset ends up clearing the new pll config. Hm, I forgot about this split totally. And there seems to be a giant mess going on here: In our top-level intel_atomic_check we have 4 parts to compute state: 1. drm_atomic_helper_check_modeset 2. intel_modeset_pipe_config 3. intel_modeset_checks 4. drm_atomic_helper_check_planes We recalculate clocks (by calling dev_priv->display.crtc_compute_clock) in 1., way ahead of anything else in intel_crtc_atomic_check. That looks very suspcious since it means only very later on (in the loop that does 2.) do we even decide whether we need to do a full modeset or not. So what I had in mind is that we clear clocks in intel_modeset_pipe_config, before we call any of the callbacks. That makes sure that when we decided to do a modeset, we do recompute the clocks correctly. First step would be to order 1 and 2 correctly I guess. Next up is 3, that one just has a confusing name - it doesn't clear PLL state, but updates the global pll state if necessary. So should perhaps be renamed to intel_modeset_compute_shared_dpll or similar. Related, there's also the scaler code in step 1, which is definitely bonkers since that's done before we recompute the config in step 3. But that's an unrelated issue really. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation 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 3/8] drm/i915: Cope with request list state change during error state capture
On 13/10/2015 12:39, Daniel Vetter wrote: On Fri, Oct 09, 2015 at 12:25:26PM +0100, Tomas Elf wrote: On 09/10/2015 08:48, Chris Wilson wrote: On Thu, Oct 08, 2015 at 07:31:35PM +0100, Tomas Elf wrote: Since we're not synchronizing the ring request list during error state capture the request list state might change between the time the corresponding error request list was allocated and dimensioned to the time when the ring request list is actually captured into the error state. If this happens, throw a WARNING and do early exit and be aware that the captured error state might not be fully reliable. Signed-off-by: Tomas Elf--- drivers/gpu/drm/i915/i915_gpu_error.c | 13 + 1 file changed, 13 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index 32c1799..cc75ca4 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -1071,6 +1071,19 @@ static void i915_gem_record_rings(struct drm_device *dev, list_for_each_entry_safe(request, tmpreq, >request_list, list) { struct drm_i915_error_request *erq; + if (WARN_ON(!request || count >= error->ring[i].num_requests)) { Request cannot be null, count can legitmately be more, the WARN on is inappropriate. Again, I sent several patches over the past couple of years to fix this. -Chris Ok, so having the driver request list change grow in between the point where we allocate and set up the error state request list to the same size as the driver request list (since that's what count being larger than the list size implies) is legitimate? Traversing into unallocated memory seems pretty dodgy to me but if you say so. We still need to handle it ofc, but just not WARN on this condition since it can happen. -Daniel With the RCU discussion ongoing I guess we should we just drop this patch? I agree that what I've been seeing looks like a side-effect of concurrent memory deallocation. Whatever solution you reach should make this patch pointless. Thanks, Tomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [v4.3/-fixes PATCH/BACKPORT 0/4] plane related backports to -fixes
On Wed, 14 Oct 2015, Maarten Lankhorstwrote: > Op 14-10-15 om 11:06 schreef Jani Nikula: >> I'm proposing to backport these to v4.3/-fixes, objections or acks? >> >> BR, >> Jani. >> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=91910#c4 >> >> >> Jani Nikula (1): >> Revert "drm/i915: Add primary plane to mask if it's visible" >> >> Maarten Lankhorst (1): >> drm/i915: Add primary plane to mask if it's visible >> >> Ville Syrjälä (2): >> drm/i915: Assign hwmode after encoder state readout >> drm/i915: Move sprite/cursor plane disable to intel_sanitize_crtc() >> >> drivers/gpu/drm/i915/intel_display.c | 106 >> +-- >> 1 file changed, 53 insertions(+), 53 deletions(-) >> > Acking all. Thanks, pushed all to drm-intel-fixes. BR, Jani. -- 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 3/8] drm/i915: Cope with request list state change during error state capture
On Wed, Oct 14, 2015 at 12:46:51PM +0100, Tomas Elf wrote: > On 13/10/2015 12:39, Daniel Vetter wrote: > >On Fri, Oct 09, 2015 at 12:25:26PM +0100, Tomas Elf wrote: > >>On 09/10/2015 08:48, Chris Wilson wrote: > >>>On Thu, Oct 08, 2015 at 07:31:35PM +0100, Tomas Elf wrote: > Since we're not synchronizing the ring request list during error state > capture > the request list state might change between the time the corresponding > error > request list was allocated and dimensioned to the time when the ring > request > list is actually captured into the error state. If this happens, throw a > WARNING and do early exit and be aware that the captured error state > might not > be fully reliable. > > Signed-off-by: Tomas Elf> --- > drivers/gpu/drm/i915/i915_gpu_error.c | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c > b/drivers/gpu/drm/i915/i915_gpu_error.c > index 32c1799..cc75ca4 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -1071,6 +1071,19 @@ static void i915_gem_record_rings(struct > drm_device *dev, > list_for_each_entry_safe(request, tmpreq, > >request_list, list) { > struct drm_i915_error_request *erq; > > + if (WARN_ON(!request || count >= > error->ring[i].num_requests)) { > >>> > >>>Request cannot be null, count can legitmately be more, the WARN on is > >>>inappropriate. Again, I sent several patches over the past couple of > >>>years to fix this. > >>>-Chris > >>> > >> > >>Ok, so having the driver request list change grow in between the point where > >>we allocate and set up the error state request list to the same size as the > >>driver request list (since that's what count being larger than the list size > >>implies) is legitimate? Traversing into unallocated memory seems pretty > >>dodgy to me but if you say so. > > > >We still need to handle it ofc, but just not WARN on this condition since > >it can happen. > >-Daniel > > > > With the RCU discussion ongoing I guess we should we just drop this patch? I > agree that what I've been seeing looks like a side-effect of concurrent > memory deallocation. Whatever solution you reach should make this patch > pointless. Depending upon the solution we pick we might still need it, since if we only make sure we don't chase freed memory but still allow the list itself to get modified (which would be the case with kfree_rcu), then you need it. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation 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/bxt: Fix wrongly placed ')' in I915_READ()
On Wed, 14 Oct 2015, Jani Nikulawrote: > On Wed, 14 Oct 2015, Dave Gordon wrote: >> On 17/09/15 14:20, Damien Lespiau wrote: >>> Not the first time! not the last time? >>> >>> There is a possibility to use gcc 5's -Wbool-compare to try and compare >>> (reg) in those macros to a constant and gcc will warn that the >>> comparison between a boolean expression and a constant is always either >>> true or false. Maybe. >> >> Since boolean true (1) cannot be a valid argument to this macro, it >> could contain a compile-time check that the parameter is not 1; if >> boolean false (0) happens not to be a valid register address (BSpec says >> MMIO 0 is reserved) the macro could check that the argument is neither >> of these values, and the compiler might then detect that all possible >> paths lead to a compile-time error. Something like this? > > The past errors have also been of the form > > if (I915_READ(SOME_REG(pipe) & MASK) == val) > > which isn't caught by your check. See [1]. > > > BR, > Jani. > > > [1] > http://mid.gmane.org/1442595836-23981-1-git-send-email-ville.syrj...@linux.intel.com > > > > > >> >> #define I915_READ(reg) \ >> ({ \ >> if (__builtin_constant_p(reg)) { \ Also, what Damien fixed wouldn't have been caught because "BXT_PORT_PCS_DW12_LN23(port) != hw_state->pcsdw12" is not a builtin constant. Daniel, I don't think this is interim solution material, sorry. BR, Jani. >> BUILD_BUG_ON((reg) == false); \ >> BUILD_BUG_ON((reg) == true); \ >> } \ >> dev_priv->uncore.funcs.mmio_readl(dev_priv, (reg), true); \ >> }) >> >> Interestingly, that reported three errors, all in intel_dsi.c where the >> port-selection macros use 0s to fill in dummy elements when less than 3 >> ports are being used. >> >> In function ‘intel_dsi_get_hw_state’ >> In function ‘intel_dsi_port_disable’ >> In function ‘intel_dsi_port_enable’ >> >> Other than that, there weren't any cases where a bool constant was being >> passed to this specific macro (as of today). >> >> .Dave. >> >>> Cc: Imre Deak >>> Signed-off-by: Damien Lespiau >>> --- >>> drivers/gpu/drm/i915/intel_ddi.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c >>> b/drivers/gpu/drm/i915/intel_ddi.c >>> index 4823184..5b600bf 100644 >>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>> @@ -2882,7 +2882,7 @@ static bool bxt_ddi_pll_get_hw_state(struct >>> drm_i915_private *dev_priv, >>> * here just read out lanes 0/1 and output a note if lanes 2/3 differ. >>> */ >>> hw_state->pcsdw12 = I915_READ(BXT_PORT_PCS_DW12_LN01(port)); >>> - if (I915_READ(BXT_PORT_PCS_DW12_LN23(port) != hw_state->pcsdw12)) >>> + if (I915_READ(BXT_PORT_PCS_DW12_LN23(port)) != hw_state->pcsdw12) >>> DRM_DEBUG_DRIVER("lane stagger config different for lane 01 >>> (%08x) and 23 (%08x)\n", >>> hw_state->pcsdw12, >>> I915_READ(BXT_PORT_PCS_DW12_LN23(port))); >> >> >> ___ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center -- 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] [RFCv2 DP-typeC 6/6] drm/i915/dp: Enable Upfront link training for typeC DP support on CHV
Hi Durgadoss, [auto build test WARNING on drm-intel/for-linux-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base] url: https://github.com/0day-ci/linux/commits/Durgadoss-R/Add-support-for-USB-typeC-based-DP/20151014-193613 config: x86_64-randconfig-s5-10142016 (attached as .config) reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): drivers/gpu/drm/i915/intel_display.c: In function 'chv_upfront_link_train': drivers/gpu/drm/i915/intel_display.c:15741:27: error: 'struct intel_connector' has no member named 'new_encoder' tmp_encoder = connector->new_encoder; ^ drivers/gpu/drm/i915/intel_display.c:15742:22: error: 'struct intel_encoder' has no member named 'new_crtc' tmp_crtc = encoder->new_crtc; ^ drivers/gpu/drm/i915/intel_display.c:15745:13: error: 'struct intel_connector' has no member named 'new_encoder' connector->new_encoder = encoder; ^ drivers/gpu/drm/i915/intel_display.c:15746:11: error: 'struct intel_encoder' has no member named 'new_crtc' encoder->new_crtc = crtc; ^ drivers/gpu/drm/i915/intel_display.c:15767:24: error: 'struct intel_dp' has no member named 'link_bw' tmp_link_bw = intel_dp->link_bw; ^ drivers/gpu/drm/i915/intel_display.c:15770:10: error: 'struct intel_dp' has no member named 'link_bw' intel_dp->link_bw = intel_dp->dpcd[DP_MAX_LINK_RATE]; ^ In file included from include/uapi/linux/stddef.h:1:0, from include/linux/stddef.h:4, from include/uapi/linux/posix_types.h:4, from include/uapi/linux/types.h:13, from include/linux/types.h:5, from include/linux/list.h:4, from include/linux/dmi.h:4, from drivers/gpu/drm/i915/intel_display.c:27: drivers/gpu/drm/i915/intel_display.c:15775:14: error: 'struct intel_dp' has no member named 'link_bw' if (intel_dp->link_bw == DP_LINK_BW_5_4) ^ include/linux/compiler.h:147:28: note: in definition of macro '__trace_if' if (__builtin_constant_p((cond)) ? !!(cond) : \ ^ >> drivers/gpu/drm/i915/intel_display.c:15775:2: note: in expansion of macro >> 'if' if (intel_dp->link_bw == DP_LINK_BW_5_4) ^ drivers/gpu/drm/i915/intel_display.c:15775:14: error: 'struct intel_dp' has no member named 'link_bw' if (intel_dp->link_bw == DP_LINK_BW_5_4) ^ include/linux/compiler.h:147:40: note: in definition of macro '__trace_if' if (__builtin_constant_p((cond)) ? !!(cond) : \ ^ >> drivers/gpu/drm/i915/intel_display.c:15775:2: note: in expansion of macro >> 'if' if (intel_dp->link_bw == DP_LINK_BW_5_4) ^ drivers/gpu/drm/i915/intel_display.c:15775:14: error: 'struct intel_dp' has no member named 'link_bw' if (intel_dp->link_bw == DP_LINK_BW_5_4) ^ include/linux/compiler.h:158:16: note: in definition of macro '__trace_if' __r = !!(cond); \ ^ >> drivers/gpu/drm/i915/intel_display.c:15775:2: note: in expansion of macro >> 'if' if (intel_dp->link_bw == DP_LINK_BW_5_4) ^ drivers/gpu/drm/i915/intel_display.c:15776:11: error: 'struct intel_dp' has no member named 'link_bw' intel_dp->link_bw = DP_LINK_BW_2_7; ^ drivers/gpu/drm/i915/intel_display.c:15780:15: error: request for member 'port_clock' in something not a structure or union crtc->config.port_clock = ^ drivers/gpu/drm/i915/intel_display.c:15781:41: error: 'struct intel_dp' has no member named 'link_bw' drm_dp_bw_code_to_link_rate(intel_dp->link_bw); ^ drivers/gpu/drm/i915/intel_display.c:15784:3: error: implicit declaration of function 'intel_dp_set_clock' [-Werror=implicit-function-declaration] intel_dp_set_clock(encoder, >config, intel_dp->link_bw); ^ drivers/gpu/drm/i915/intel_display.c:15784:54: error: 'struct intel_dp' has no member named 'link_bw' intel_dp_set_clock(encoder, >config, intel_dp->link_bw); ^ drivers/gpu/drm/i915/intel_display.c:15785:3: error: implicit declaration of function 'chv_update_pll' [-Werror=implicit-function-declaration] chv_update_pll(crtc); ^ drivers/gpu/drm/i915/intel_display.c:15787:3: error: too few arguments to function 'chv_enable_pll' chv_enable_pll(crtc); ^ drivers/gpu/drm/i915/intel_display.c:1636:13: note: declared here static void chv_enable_p
[Intel-gfx] [PATCH 02/22] drm/i915: Pass modifier instead of tiling_mode to gen4_compute_page_offset()
From: Ville SyrjäläIn preparation for handling more than X tiling, pass the fb modifier to gen4_compute_page_offset() instead of the obj->tiling_mode. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 8 drivers/gpu/drm/i915/intel_drv.h | 4 ++-- drivers/gpu/drm/i915/intel_sprite.c | 6 +++--- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index c28fb6a..6add8d1 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2447,11 +2447,11 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb, * is assumed to be a power-of-two. */ unsigned long intel_gen4_compute_page_offset(struct drm_i915_private *dev_priv, int *x, int *y, -unsigned int tiling_mode, +uint64_t fb_modifier, unsigned int cpp, unsigned int pitch) { - if (tiling_mode != I915_TILING_NONE) { + if (fb_modifier != DRM_FORMAT_MOD_NONE) { unsigned int tile_rows, tiles; tile_rows = *y / 8; @@ -2754,7 +2754,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc, if (INTEL_INFO(dev)->gen >= 4) { intel_crtc->dspaddr_offset = intel_gen4_compute_page_offset(dev_priv, - , , obj->tiling_mode, + , , fb->modifier[0], pixel_size, fb->pitches[0]); linear_offset -= intel_crtc->dspaddr_offset; @@ -2859,7 +2859,7 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, linear_offset = y * fb->pitches[0] + x * pixel_size; intel_crtc->dspaddr_offset = intel_gen4_compute_page_offset(dev_priv, - , , obj->tiling_mode, + , , fb->modifier[0], pixel_size, fb->pitches[0]); linear_offset -= intel_crtc->dspaddr_offset; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0598932..1152566 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1138,8 +1138,8 @@ void assert_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, bool state); #define assert_pipe_disabled(d, p) assert_pipe(d, p, false) unsigned long intel_gen4_compute_page_offset(struct drm_i915_private *dev_priv, int *x, int *y, -unsigned int tiling_mode, -unsigned int bpp, +uint64_t fb_modifier, +unsigned int cpp, unsigned int pitch); void intel_prepare_reset(struct drm_device *dev); void intel_finish_reset(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index b229c67..90e27c8 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -422,7 +422,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, linear_offset = y * fb->pitches[0] + x * pixel_size; sprsurf_offset = intel_gen4_compute_page_offset(dev_priv, , , - obj->tiling_mode, + fb->modifier[0], pixel_size, fb->pitches[0]); linear_offset -= sprsurf_offset; @@ -556,7 +556,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, linear_offset = y * fb->pitches[0] + x * pixel_size; sprsurf_offset = intel_gen4_compute_page_offset(dev_priv, - , , obj->tiling_mode, + , , fb->modifier[0], pixel_size, fb->pitches[0]); linear_offset -= sprsurf_offset; @@ -694,7 +694,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, linear_offset = y * fb->pitches[0] + x * pixel_size; dvssurf_offset = intel_gen4_compute_page_offset(dev_priv, -
[Intel-gfx] [PATCH 01/22] drm: Add drm_format_plane_width() and drm_format_plane_height()
From: Ville SyrjäläAdd a few helpers to get the dimensions of the chroma plane(s). Cc: dri-de...@lists.freedesktop.org Signed-off-by: Ville Syrjälä --- include/drm/drm_crtc.h | 12 1 file changed, 12 insertions(+) diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 33ddedd..317baf9 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1497,6 +1497,18 @@ extern int drm_format_num_planes(uint32_t format); extern int drm_format_plane_cpp(uint32_t format, int plane); extern int drm_format_horz_chroma_subsampling(uint32_t format); extern int drm_format_vert_chroma_subsampling(uint32_t format); +static inline int drm_format_plane_width(int width, uint32_t format, int plane) +{ + if (plane == 0) + return width; + return width / drm_format_horz_chroma_subsampling(format); +} +static inline int drm_format_plane_height(int height, uint32_t format, int plane) +{ + if (plane == 0) + return height; + return height / drm_format_vert_chroma_subsampling(format); +} extern const char *drm_get_format_name(uint32_t format); extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, unsigned int supported_rotations); -- 2.4.9 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/i915: Stuff rotation params into view union
On Wed, Oct 14, 2015 at 04:51:05PM +0200, Daniel Vetter wrote: > We don't need 2 separate unions. > > Note that this was done intentinoally > > Author: Joonas Lahtinen> Date: Wed May 6 14:35:38 2015 +0300 > > drm/i915: Add a partial GGTT view type > > on Tvrtko's request, but without a clear justification. Rotated views > are also not checking for matching paramters in i915_ggtt_view_equal, > which seems like a bug. But this patch here doesn't change that. > > Cc: Tvrtko Ursulin > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++-- > drivers/gpu/drm/i915/i915_gem_gtt.h | 5 + > drivers/gpu/drm/i915/intel_display.c | 4 ++-- > 3 files changed, 5 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > b/drivers/gpu/drm/i915/i915_gem_gtt.c > index bead61e0de1b..15dfefd58d08 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -3283,7 +3283,7 @@ static struct sg_table * > intel_rotate_fb_obj_pages(struct i915_ggtt_view *ggtt_view, > struct drm_i915_gem_object *obj) > { > - struct intel_rotation_info *rot_info = _view->rotation_info; > + struct intel_rotation_info *rot_info = _view->params.rotation_info; > unsigned int size_pages = rot_info->size >> PAGE_SHIFT; > unsigned int size_pages_uv; > struct sg_page_iter sg_iter; > @@ -3515,7 +3515,7 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj, > if (view->type == I915_GGTT_VIEW_NORMAL) { > return obj->base.size; > } else if (view->type == I915_GGTT_VIEW_ROTATED) { > - return view->rotation_info.size; > + return view->params.rotation_info.size; > } else if (view->type == I915_GGTT_VIEW_PARTIAL) { > return view->params.partial.size << PAGE_SHIFT; > } else { > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h > b/drivers/gpu/drm/i915/i915_gem_gtt.h > index 9fbb07d6eaad..2e1f6493c9e7 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -156,13 +156,10 @@ struct i915_ggtt_view { > u64 offset; > unsigned int size; > } partial; > + struct intel_rotation_info rotation_info; > } params; > > struct sg_table *pages; > - > - union { > - struct intel_rotation_info rotation_info; > - }; So, I basically just sent the same patch, except I made the union anonymous, and I renamed 'rotation_info' to 'rotated' to match the type. > }; > > extern const struct i915_ggtt_view i915_ggtt_view_normal; > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 2c9d822b29b6..57459fedf216 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2247,7 +2247,7 @@ static void > intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer > *fb, > const struct drm_plane_state *plane_state) > { > - struct intel_rotation_info *info = >rotation_info; > + struct intel_rotation_info *info = >params.rotation_info; > unsigned int tile_height, tile_pitch; > > *view = i915_ggtt_view_normal; > @@ -2909,7 +2909,7 @@ unsigned long intel_plane_obj_offset(struct intel_plane > *intel_plane, > offset = (unsigned char *)vma->node.start; > > if (plane == 1) { > - offset += vma->ggtt_view.rotation_info.uv_start_page * > + offset += vma->ggtt_view.params.rotation_info.uv_start_page * > PAGE_SIZE; > } > > -- > 2.5.1 > > ___ > 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] [BXT DSI timing fixes v1 3/3] drm/i915/bxt: Fixed dsi enc disable and blank at bootup
On Wed, Oct 14, 2015 at 04:37:27PM +, Shankar, Uma wrote: > > > >-Original Message- > >From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel > >Vetter > >Sent: Wednesday, October 14, 2015 6:20 PM > >To: Shankar, Uma > >Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org; Kumar, Shobhit > >Subject: Re: [Intel-gfx] [BXT DSI timing fixes v1 3/3] drm/i915/bxt: Fixed > >dsi enc > >disable and blank at bootup > > > >On Wed, Oct 14, 2015 at 10:20:32AM +, Shankar, Uma wrote: > >> > >> > >> >-Original Message- > >> >From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of > >> >Daniel Vetter > >> >Sent: Tuesday, October 13, 2015 9:17 PM > >> >To: Shankar, Uma > >> >Cc: intel-gfx@lists.freedesktop.org; Kumar, Shobhit > >> >Subject: Re: [Intel-gfx] [BXT DSI timing fixes v1 3/3] drm/i915/bxt: > >> >Fixed dsi enc disable and blank at bootup > >> > > >> >On Mon, Oct 12, 2015 at 10:55:03PM +0530, Uma Shankar wrote: > >> >> During bootup, mipi display was blanking in BXT. This is because > >> >> during driver load, though encoder, connector were active but crtc > >> >> returned inactive. This caused sanitize function to disable the DSI > >> >> panel. In AOS, this is fine since HWC will do a modeset and crtc, > >> >> connector, encoder mapping will be restored. But in Charging OS, no > >> >> modeset is called, it just calls DPMS ON/OFF. Hence display doesn't > >> >> come in COS. This is needed even for seamless booting to Android > >> >> without a > >> >blank. > >> >> > >> >> This is fine on BYT/CHT since transcoder is common b/w all encoders. > >> >> But for BXT, there is a separate mipi transcoder. Hence this needs > >> >> special handling for BXT DSI. > >> >> > >> >> Signed-off-by: Uma Shankar> >> >> --- > >> >> drivers/gpu/drm/i915/i915_drv.h |3 +++ > >> >> drivers/gpu/drm/i915/intel_display.c | 27 +++ > >> >> 2 files changed, 26 insertions(+), 4 deletions(-) > >> >> > >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h > >> >> b/drivers/gpu/drm/i915/i915_drv.h index bf14096..ae790ec 100644 > >> >> --- a/drivers/gpu/drm/i915/i915_drv.h > >> >> +++ b/drivers/gpu/drm/i915/i915_drv.h > >> >> @@ -1948,6 +1948,9 @@ struct drm_i915_private { > >> >> /* perform PHY state sanity checks? */ > >> >> bool chv_phy_assert[2]; > >> >> > >> >> + /* To check if DSI is initializing at bootup */ > >> >> + bool dsi_enumerating; > >> > > >> >See my other comment, you're working around the broken design of > >> >patch 2 here. Special casing fastboot is a big no-no which needs some > >> >really good reasons. An example is the special edp clock readout code > >> >in the encoder callbacks we have for ilk-ivb cpu edp. > >> >-Daniel > >> > > >> > >> I agree this is not a clean solution but was not getting any better > >> ideas. As part of driver load and initialization, readout_state > >> returns encoder, connector as active but all crtc return inactive. > >> This make sanitize encoder to consider this as inconsistent hw state > >> and it goes ahead and disables encoder, thereby disabling the BIOS/GOP > >> Initialization. > >> > >> After this only way to recover is a modeset which doesn't come in > >> cases like Charging OS in Android. Also this will create issues in > >> having a seamless boot without a blank (a must have for Android devices). > >> > >> For EDP case, we can read DDI_FUNC_CTL and get a pipe number and match > >> it with crtc->pipe to detect EDP is on that pipe and map it to crtc. > >> > >> In DSI there is no such mechanism to detect this. Can you suggest some > >> pointers as to how to approach this issue. > > > >MIPI_CTRL(port) has BXT_PIPE_SELECT_A/C bits. Which btw we don't ever seem > >to check anywhere in intel_dsi_compute_config, which is a pretty stellar bug > >that running kms_flip even once should have caught. > >-Daniel > >-- > > These fields were removed from the port control register in the bspec > updates. These fields remained in the code > and should be cleaned up and removed. As per latest bspec status, there is no > pipe info in the port control for BXT DSI. > > I am not sure, we could add something like below in compute_config to update > transcoder info: > > if (IS_BROXTON(dev)) { > + if (intel_dsi->ports & (1 << PORT_A)) > + config->cpu_transcoder = TRANSCODER_MIPI_A; > + else > + config->cpu_transcoder = TRANSCODER_MIPI_C; > + } > > Thoughts ? That doesn't fix the hw state readout really - we need to fix that to reflect reality. If dsi is enabled at boot then the corresponding crtc should state that it's active. So on bxt we also need to check the dsi port registers besides all the other transcoders. Maybe it makes sense to extend the cpu_transcoder enum to include MIPI_A/C, but I'm not sure about that. But the bug here is in haswell_get_pipe_config. And probably we should split
Re: [Intel-gfx] Video freezing after activating XScreensaver
On Tue, Oct 13, 2015 at 02:51:25PM -0500, Chris wrote: > On Tue, 2015-10-13 at 17:30 +0200, Daniel Vetter wrote: > > On Tue, Oct 13, 2015 at 08:56:52AM -0500, Chris wrote: > > > This is an issue that has been ongoing for over 1 1/2 years now. I have > > > tried kernel after kernel and whether it's a Ubuntu kernel such as the > > > one in my sig or a kernel from here - > > > http://kernel.ubuntu.com/~kernel-ppa/mainline/drm-intel-next/ the video > > > will intermittently freeze. Running XScreensaver causes it to freeze > > > when switching between the different screen savers showing a black > > > screen and the mouse cursor. The cursor can be moved and will switch > > > between the arrow and the hand tool if hovering over a clickable area on > > > the desktop even though it can't be seen. Giving an error output in my > > > syslog of > > > > > > kernel: [578631.820045] [drm:i915_hangcheck_elapsed [i915]] *ERROR* > > > Hangcheck timer elapsed... render ring idle > > > > Known problem and we never managed to fix it. The first time it happens a > > workaround should kick in, which should prevent any future freezes (until > > you reboot at least). > > > > If freezes still happen after that there's another bug on top. > > -Daniel > > > Thanks for the reply Daniel, and there entails the problem. Once the > video freezes you have no idea if the 'workaround' kicked in or not. I > can SSH in via my tablet since all the system is still active. That's > the only way I can effectively capture any log files and reboot. The > 'Hangcheck...' error only seems to appear if I start XScreensaver > running at other times the video will freeze with no errors noted at > all. In that case it seems unrelated to the error in dmesg. No idea what's happening on your system - hangcheck is rather reliable at figuring out there's a stuck gpu somewhere. If it doesn't fire the gpu should be all fine and the stall happens somewhere else. -Daniel > > > > > > > I was able to reproduce this yesterday by starting XScreensaver at > > > 9:43am and at 13:44pm the video froze giving the above error. At other > > > times when XScreensaver is not running the video will still randomly > > > freeze however there is no specific time frame for this to happen. I've > > > seen it go 2 days then freeze and have also seen it go 16 days then > > > freeze. As I've said previously all background operations continue to > > > run as just the video is frozen. > > > > > > I have had the random freeze happen when running this kernelkernel > > > 4.0.0-997-generic #201503310205 SMP Tue Mar > > > 31 02:07:04 UTC 2015 which Chris Wilson from this list had me run kernel > > > kernel 4.0.0-997-generic #201503310205 SMP Tue Mar > > > 31 02:07:04 UTC 2015 however when I attempted to file a bug report at > > > Ubuntu Launchpad it was not accepted so the Ubuntu kernel folks had me > > > switch to kernel 4.2.3-040203-generic #201510030832 SMP Sat Oct 3 > > > 12:34:31 UTC 2015 which although is an upstream kernel is what they > > > wanted me to run. The bug report that I've filed on Launchpad is > > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1497627 > > > > > > I've attempted to boot into the following Ubuntu upstream kernels: > > > > > > v4.3-rc5-unstable - Boot process would begin but would not complete > > > v4.3-rc4-unstable - same as above > > > v4.3-rc3-unstable - same as above > > > v4.3-rc2-unstable - Same as above > > > v4.3-rc1-unstable - Same as above > > > > > > I also attempted last night to boot into > > > > > > v4.3-rc5-unstable from > > > http://kernel.ubuntu.com/~kernel-ppa/mainline/?C=N;O=D which gave the > > > same results. The boot process would begin but then go to just a black > > > screen without completing. > > > > > > I have either filed or commented on the following bugs also: > > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=75394 > > > https://bugs.freedesktop.org/show_bug.cgi?id=89290 > > > https://bugs.freedesktop.org/show_bug.cgi?id=91495 > > > > > > System info is: > > > > > > Dell Optiplex 780 > > > Bios A15 > > > Ubuntu 14.04.3 LTS > > > Gnome 3.12.2 > > > > > > Am I still the only one who is experiencing this issue? > > > -- > > > Chris > > > KeyID 0xE372A7DA98E6705C > > > 31.11°N 97.89°W (Elev. 1092 ft) > > > 07:57:25 up 11:34, 1 user, load average: 0.37, 0.41, 0.33 > > > Ubuntu 14.04.3 LTS, kernel 4.2.3-040203-generic #201510030832 SMP Sat > > > Oct 3 12:34:31 UTC 2015 > > > > > > ___ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Chris > KeyID 0xE372A7DA98E6705C > 31.11°N 97.89°W (Elev. 1092 ft) > 14:45:43 up 18:22, 1 user, load average: 0.49, 0.24, 0.17 > Ubuntu 14.04.3 LTS, kernel 4.2.3-040203-generic #201510030832 SMP Sat > Oct 3 12:34:31 UTC 2015 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Make plane fb tracking work correctly, v2.
On Wed, 2015-09-23 at 13:27 +0200, Maarten Lankhorst wrote: > atomic->disabled_planes is a hack that had to exist because > prepare_fb was only called when a new fb was set. This messed > up fb tracking in some circumstances like aborts from > interruptible waits. As a result interruptible waiting in > prepare_plane_fb was forbidden, but other errors could still > cause frontbuffer tracking to be messed up. > > Now that prepare_fb is always called, this hack is no longer > required and prepare_fb may fail without consequences. > > Changes since v1: > - Clean up a few fb tracking warnings by changing plane->fb to > plane->state->fb. > > Signed-off-by: Maarten Lankhorst> --- > drivers/gpu/drm/i915/intel_display.c | 47 > ++-- > drivers/gpu/drm/i915/intel_drv.h | 1 - > 2 files changed, 18 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index fc0086748b71..ac97af69be62 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4752,17 +4752,6 @@ static void intel_pre_plane_update(struct intel_crtc > *crtc) > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc_atomic_commit *atomic = >atomic; > - struct drm_plane *p; > - > - /* Track fb's for any planes being disabled */ > - drm_for_each_plane_mask(p, dev, atomic->disabled_planes) { > - struct intel_plane *plane = to_intel_plane(p); > - > - mutex_lock(>struct_mutex); > - i915_gem_track_fb(intel_fb_obj(plane->base.fb), NULL, > - plane->frontbuffer_bit); > - mutex_unlock(>struct_mutex); > - } > > if (atomic->wait_for_flips) > intel_crtc_wait_for_pending_flips(>base); > @@ -11561,14 +11550,6 @@ int intel_plane_atomic_calc_changes(struct > drm_crtc_state *crtc_state, > return ret; > } > > - /* > - * Disabling a plane is always okay; we just need to update > - * fb tracking in a special way since cleanup_fb() won't > - * get called by the plane helpers. > - */ > - if (old_plane_state->base.fb && !fb) > - intel_crtc->atomic.disabled_planes |= 1 << i; > - > was_visible = old_plane_state->visible; > visible = to_intel_plane_state(plane_state)->visible; > > @@ -13318,15 +13299,17 @@ intel_prepare_plane_fb(struct drm_plane *plane, > struct drm_framebuffer *fb = new_state->fb; > struct intel_plane *intel_plane = to_intel_plane(plane); > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); > + struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb); > int ret = 0; > > - if (!obj) > + if (!obj && !old_obj) > return 0; > > mutex_lock(>struct_mutex); > > - if (plane->type == DRM_PLANE_TYPE_CURSOR && > + if (!obj) { > + ret = 0; > + } else if (plane->type == DRM_PLANE_TYPE_CURSOR && > INTEL_INFO(dev)->cursor_needs_physical) { > int align = IS_I830(dev) ? 16 * 1024 : 256; > ret = i915_gem_object_attach_phys(obj, align); > @@ -13356,17 +13339,23 @@ intel_cleanup_plane_fb(struct drm_plane *plane, > const struct drm_plane_state *old_state) > { > struct drm_device *dev = plane->dev; > - struct drm_i915_gem_object *obj = intel_fb_obj(old_state->fb); > + struct intel_plane *intel_plane = to_intel_plane(plane); > + struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb); > + struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb); > > - if (!obj) > + if (!obj && !old_obj) > return; > > - if (plane->type != DRM_PLANE_TYPE_CURSOR || > - !INTEL_INFO(dev)->cursor_needs_physical) { > - mutex_lock(>struct_mutex); > + mutex_lock(>struct_mutex); > + if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR || > + !INTEL_INFO(dev)->cursor_needs_physical)) > intel_unpin_fb_obj(old_state->fb, old_state); > - mutex_unlock(>struct_mutex); > - } > + > + /* prepare_fb aborted? */ > + if ((old_obj && (old_obj->frontbuffer_bits & > intel_plane->frontbuffer_bit)) || > + (obj && !(obj->frontbuffer_bits & intel_plane->frontbuffer_bit))) > + i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit); I'm not a fan of this big condition. Would it make sense to add a new parameter to cleanup_fb() that tells us if this is an abort clean up? But in any case, that can be done later. Reviewed-by: Ander Conselvan de Oliveira ___ Intel-gfx mailing list
Re: [Intel-gfx] [RFCv2 DP-typeC 2/6] drm/i915/dp: Reuse shared DPLL if it exists already
On Wed, 14 Oct 2015, Durgadoss Rwrote: > Do not call intel_get_shared_dpll() if there exists a > valid shared DPLL already. > > Signed-off-by: Durgadoss R > --- > drivers/gpu/drm/i915/intel_ddi.c | 70 > > drivers/gpu/drm/i915/intel_display.c | 2 +- > drivers/gpu/drm/i915/intel_drv.h | 2 +- > 3 files changed, 42 insertions(+), 32 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index 9098c12..8e4ea36 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1258,7 +1258,8 @@ hsw_ddi_calculate_wrpll(int clock /* in Hz */, > static bool > hsw_ddi_pll_select(struct intel_crtc *intel_crtc, > struct intel_crtc_state *crtc_state, > -struct intel_encoder *intel_encoder) > +struct intel_encoder *intel_encoder, > +bool find_dpll) > { > int clock = crtc_state->port_clock; > > @@ -1278,14 +1279,16 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc, > > crtc_state->dpll_hw_state.wrpll = val; > > - pll = intel_get_shared_dpll(intel_crtc, crtc_state); > - if (pll == NULL) { > - DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", > - pipe_name(intel_crtc->pipe)); > - return false; > - } > + if (find_dpll) { > + pll = intel_get_shared_dpll(intel_crtc, crtc_state); > + if (pll == NULL) { > + DRM_DEBUG_DRIVER("failed to find PLL for pipe > %c\n", > + pipe_name(intel_crtc->pipe)); > + return false; > + } You have to do a *lot* of passing around parameters here. I wonder (and really, I don't know) if we could make intel_get_shared_dpll() grow smarts about reusing the pll with intel_crtc_to_shared_dpll() when it's there already. BR, Jani. > > - crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id); > + crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id); > + } > } > > return true; > @@ -1540,7 +1543,8 @@ skip_remaining_dividers: > static bool > skl_ddi_pll_select(struct intel_crtc *intel_crtc, > struct intel_crtc_state *crtc_state, > -struct intel_encoder *intel_encoder) > +struct intel_encoder *intel_encoder, > +bool find_dpll) > { > struct intel_shared_dpll *pll; > uint32_t ctrl1, cfgcr1, cfgcr2; > @@ -1594,15 +1598,17 @@ skl_ddi_pll_select(struct intel_crtc *intel_crtc, > crtc_state->dpll_hw_state.cfgcr1 = cfgcr1; > crtc_state->dpll_hw_state.cfgcr2 = cfgcr2; > > - pll = intel_get_shared_dpll(intel_crtc, crtc_state); > - if (pll == NULL) { > - DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", > - pipe_name(intel_crtc->pipe)); > - return false; > - } > + if (find_dpll) { > + pll = intel_get_shared_dpll(intel_crtc, crtc_state); > + if (pll == NULL) { > + DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", > + pipe_name(intel_crtc->pipe)); > + return false; > + } > > - /* shared DPLL id 0 is DPLL 1 */ > - crtc_state->ddi_pll_sel = pll->id + 1; > + /* shared DPLL id 0 is DPLL 1 */ > + crtc_state->ddi_pll_sel = pll->id + 1; > + } > > return true; > } > @@ -1632,7 +1638,8 @@ static const struct bxt_clk_div bxt_dp_clk_val[] = { > static bool > bxt_ddi_pll_select(struct intel_crtc *intel_crtc, > struct intel_crtc_state *crtc_state, > -struct intel_encoder *intel_encoder) > +struct intel_encoder *intel_encoder, > +bool find_pll) > { > struct intel_shared_dpll *pll; > struct bxt_clk_div clk_div = {0}; > @@ -1741,15 +1748,17 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc, > crtc_state->dpll_hw_state.pcsdw12 = > LANESTAGGER_STRAP_OVRD | lanestagger; > > - pll = intel_get_shared_dpll(intel_crtc, crtc_state); > - if (pll == NULL) { > - DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", > - pipe_name(intel_crtc->pipe)); > - return false; > - } > + if (find_pll) { > + pll = intel_get_shared_dpll(intel_crtc, crtc_state); > + if (pll == NULL) { > + DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", > + pipe_name(intel_crtc->pipe)); > + return false; > + } > > - /* shared DPLL id 0 is DPLL A */ > -