* [PATCH 0/2] drm/i915: Fix the invisible cursor syndrome
@ 2015-12-14 11:16 ville.syrjala
2015-12-14 11:16 ` [PATCH 1/2] drm/i915: Kill intel_crtc->cursor_bo ville.syrjala
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: ville.syrjala @ 2015-12-14 11:16 UTC (permalink / raw)
To: intel-gfx; +Cc: Takashi Iwai
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
These two should provide the minimal fix for the invisible cursor
problem that's been plaguing me and some other people. It's caused
by the cursor bo getting bound at ggtt offset 0, which the code then
considers to mean "not enabled". These patches get rid of that assumption,
with the caveat that we may do some needless cursor register writes.
I have a more generic fix for that (and a ton of other cursor improvements)
at [1], but I figured that I'll just post the critical fixes now and allow
the others to stew a bit more in the branch until maybe early next year.
I assume the problem is also related to the lack of fbdev pinning that
Chris has fixed [2]. With fbdev permanalty pinned at offset 0, there's
no opportunity for the cursor bo to travel there. Without fbdev I suppose
something else rings, status page, etc. could get permanently placed
at offset 0, so not sure if this can be hit in that case (didn't try it).
I'm a bit wary about adding cc stable to these since I suspect that
backporting them too far will cause regressions, on account of the
atomic/plane stuff probably not being up to the task in older kernels.
So maybe better get these in w/o cc stable and consider a backport again
later if it looks necessary.
[1] git://github.com/vsyrjala/linux.git cursor_improvements
[2] http://lists.freedesktop.org/archives/intel-gfx/2015-December/082114.html
Ville Syrjälä (2):
drm/i915: Kill intel_crtc->cursor_bo
drm/i915: Drop the broken cursor base==0 special casing
drivers/gpu/drm/i915/intel_display.c | 34 ++++++++++++----------------------
drivers/gpu/drm/i915/intel_drv.h | 1 -
2 files changed, 12 insertions(+), 23 deletions(-)
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
--
2.4.10
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] drm/i915: Kill intel_crtc->cursor_bo 2015-12-14 11:16 [PATCH 0/2] drm/i915: Fix the invisible cursor syndrome ville.syrjala @ 2015-12-14 11:16 ` ville.syrjala 2015-12-14 11:32 ` Chris Wilson 2015-12-14 15:35 ` [PATCH v2 " ville.syrjala 2015-12-14 11:16 ` [PATCH 2/2] drm/i915: Drop the broken cursor base==0 special casing ville.syrjala 2015-12-14 20:23 ` [PATCH 0/2] drm/i915: Fix the invisible cursor syndrome Ville Syrjälä 2 siblings, 2 replies; 9+ messages in thread From: ville.syrjala @ 2015-12-14 11:16 UTC (permalink / raw) To: intel-gfx; +Cc: Takashi Iwai From: Ville Syrjälä <ville.syrjala@linux.intel.com> The vma may have been rebound between the last time the cursor was enabled and now, so skipping the cursor gtt offset deduction is not safe unless we would also reset cursor_bo to NULL when disabling the cursor. Just thow cursor_bo to the bin instead since it's lost all other uses thanks to universal plane support. Cc: Takashi Iwai <tiwai@suse.de> Cc: Jani Nikula <jani.nikula@linux.intel.com> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 5 ----- drivers/gpu/drm/i915/intel_drv.h | 1 - 2 files changed, 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e9bb860a55dc..f2a0151b3f14 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14105,9 +14105,6 @@ intel_commit_cursor_plane(struct drm_plane *plane, crtc = crtc ? crtc : plane->crtc; intel_crtc = to_intel_crtc(crtc); - if (intel_crtc->cursor_bo == obj) - goto update; - if (!obj) addr = 0; else if (!INTEL_INFO(dev)->cursor_needs_physical) @@ -14116,9 +14113,7 @@ intel_commit_cursor_plane(struct drm_plane *plane, addr = obj->phys_handle->busaddr; intel_crtc->cursor_addr = addr; - intel_crtc->cursor_bo = obj; -update: intel_crtc_update_cursor(crtc, state->visible); } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 76dfa286cd95..6324c782d062 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -568,7 +568,6 @@ struct intel_crtc { int adjusted_x; int adjusted_y; - struct drm_i915_gem_object *cursor_bo; uint32_t cursor_addr; uint32_t cursor_cntl; uint32_t cursor_size; -- 2.4.10 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm/i915: Kill intel_crtc->cursor_bo 2015-12-14 11:16 ` [PATCH 1/2] drm/i915: Kill intel_crtc->cursor_bo ville.syrjala @ 2015-12-14 11:32 ` Chris Wilson 2015-12-14 15:30 ` Ville Syrjälä 2015-12-14 15:35 ` [PATCH v2 " ville.syrjala 1 sibling, 1 reply; 9+ messages in thread From: Chris Wilson @ 2015-12-14 11:32 UTC (permalink / raw) To: ville.syrjala; +Cc: Takashi Iwai, intel-gfx On Mon, Dec 14, 2015 at 01:16:47PM +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The vma may have been rebound between the last time the cursor was > enabled and now, so skipping the cursor gtt offset deduction is not > safe unless we would also reset cursor_bo to NULL when disabling the > cursor. Just thow cursor_bo to the bin instead since it's lost all > other uses thanks to universal plane support. You could also mention that since updating the cursor is so slow now through the universal-plane support, that a fast path to avoid a few writes/checks is also irrelevant. -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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm/i915: Kill intel_crtc->cursor_bo 2015-12-14 11:32 ` Chris Wilson @ 2015-12-14 15:30 ` Ville Syrjälä 0 siblings, 0 replies; 9+ messages in thread From: Ville Syrjälä @ 2015-12-14 15:30 UTC (permalink / raw) To: Chris Wilson, intel-gfx, Takashi Iwai On Mon, Dec 14, 2015 at 11:32:23AM +0000, Chris Wilson wrote: > On Mon, Dec 14, 2015 at 01:16:47PM +0200, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > The vma may have been rebound between the last time the cursor was > > enabled and now, so skipping the cursor gtt offset deduction is not > > safe unless we would also reset cursor_bo to NULL when disabling the > > cursor. Just thow cursor_bo to the bin instead since it's lost all > > other uses thanks to universal plane support. > > You could also mention that since updating the cursor is so slow now > through the universal-plane support, that a fast path to avoid a few > writes/checks is also irrelevant. I can add a note. At some point we really should start measuring things and try to get some of the speed back for common stuff like cursor movement and page flips. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] drm/i915: Kill intel_crtc->cursor_bo 2015-12-14 11:16 ` [PATCH 1/2] drm/i915: Kill intel_crtc->cursor_bo ville.syrjala 2015-12-14 11:32 ` Chris Wilson @ 2015-12-14 15:35 ` ville.syrjala 2015-12-14 15:37 ` Chris Wilson 1 sibling, 1 reply; 9+ messages in thread From: ville.syrjala @ 2015-12-14 15:35 UTC (permalink / raw) To: intel-gfx; +Cc: Takashi Iwai From: Ville Syrjälä <ville.syrjala@linux.intel.com> The vma may have been rebound between the last time the cursor was enabled and now, so skipping the cursor gtt offset deduction is not safe unless we would also reset cursor_bo to NULL when disabling the cursor. Just thow cursor_bo to the bin instead since it's lost all other uses thanks to universal plane support. Chris pointed out that cursor updates are currently too slow via universal planes that micro optimizations like these wouldn't even help. v2: Add a note about futility of micro optimizations (Chris) References: http://lists.freedesktop.org/archives/intel-gfx/2015-December/082976.html Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Takashi Iwai <tiwai@suse.de> Cc: Jani Nikula <jani.nikula@linux.intel.com> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 5 ----- drivers/gpu/drm/i915/intel_drv.h | 1 - 2 files changed, 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e9bb860a55dc..f2a0151b3f14 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14105,9 +14105,6 @@ intel_commit_cursor_plane(struct drm_plane *plane, crtc = crtc ? crtc : plane->crtc; intel_crtc = to_intel_crtc(crtc); - if (intel_crtc->cursor_bo == obj) - goto update; - if (!obj) addr = 0; else if (!INTEL_INFO(dev)->cursor_needs_physical) @@ -14116,9 +14113,7 @@ intel_commit_cursor_plane(struct drm_plane *plane, addr = obj->phys_handle->busaddr; intel_crtc->cursor_addr = addr; - intel_crtc->cursor_bo = obj; -update: intel_crtc_update_cursor(crtc, state->visible); } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 76dfa286cd95..6324c782d062 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -568,7 +568,6 @@ struct intel_crtc { int adjusted_x; int adjusted_y; - struct drm_i915_gem_object *cursor_bo; uint32_t cursor_addr; uint32_t cursor_cntl; uint32_t cursor_size; -- 2.4.10 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] drm/i915: Kill intel_crtc->cursor_bo 2015-12-14 15:35 ` [PATCH v2 " ville.syrjala @ 2015-12-14 15:37 ` Chris Wilson 0 siblings, 0 replies; 9+ messages in thread From: Chris Wilson @ 2015-12-14 15:37 UTC (permalink / raw) To: ville.syrjala; +Cc: Takashi Iwai, intel-gfx On Mon, Dec 14, 2015 at 05:35:02PM +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The vma may have been rebound between the last time the cursor was > enabled and now, so skipping the cursor gtt offset deduction is not > safe unless we would also reset cursor_bo to NULL when disabling the > cursor. Just thow cursor_bo to the bin instead since it's lost all > other uses thanks to universal plane support. > > Chris pointed out that cursor updates are currently too slow > via universal planes that micro optimizations like these wouldn't > even help. > > v2: Add a note about futility of micro optimizations (Chris) > > References: http://lists.freedesktop.org/archives/intel-gfx/2015-December/082976.html > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Takashi Iwai <tiwai@suse.de> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] drm/i915: Drop the broken cursor base==0 special casing 2015-12-14 11:16 [PATCH 0/2] drm/i915: Fix the invisible cursor syndrome ville.syrjala 2015-12-14 11:16 ` [PATCH 1/2] drm/i915: Kill intel_crtc->cursor_bo ville.syrjala @ 2015-12-14 11:16 ` ville.syrjala 2015-12-14 11:33 ` Chris Wilson 2015-12-14 20:23 ` [PATCH 0/2] drm/i915: Fix the invisible cursor syndrome Ville Syrjälä 2 siblings, 1 reply; 9+ messages in thread From: ville.syrjala @ 2015-12-14 11:16 UTC (permalink / raw) To: intel-gfx; +Cc: Takashi Iwai From: Ville Syrjälä <ville.syrjala@linux.intel.com> The cursor code tries to treat base==0 to mean disabled. That fails when the cursor bo gets bound at ggtt offset 0, and the user is left looking at an invisible cursor. We lose the disabled->disabled optimization, but that seems like something better handled at a slightly higher level. Cc: Takashi Iwai <tiwai@suse.de> Cc: Jani Nikula <jani.nikula@linux.intel.com> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f2a0151b3f14..ee84b517eb74 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10013,14 +10013,14 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, return true; } -static void i845_update_cursor(struct drm_crtc *crtc, u32 base) +static void i845_update_cursor(struct drm_crtc *crtc, u32 base, bool on) { struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); uint32_t cntl = 0, size = 0; - if (base) { + if (on) { unsigned int width = intel_crtc->base.cursor->state->crtc_w; unsigned int height = intel_crtc->base.cursor->state->crtc_h; unsigned int stride = roundup_pow_of_two(width) * 4; @@ -10075,16 +10075,15 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base) } } -static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) +static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base, bool on) { struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); int pipe = intel_crtc->pipe; - uint32_t cntl; + uint32_t cntl = 0; - cntl = 0; - if (base) { + if (on) { cntl = MCURSOR_GAMMA_ENABLE; switch (intel_crtc->base.cursor->state->crtc_w) { case 64: @@ -10135,18 +10134,17 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, int y = cursor_state->crtc_y; u32 base = 0, pos = 0; - if (on) - base = intel_crtc->cursor_addr; + base = intel_crtc->cursor_addr; if (x >= intel_crtc->config->pipe_src_w) - base = 0; + on = false; if (y >= intel_crtc->config->pipe_src_h) - base = 0; + on = false; if (x < 0) { if (x + cursor_state->crtc_w <= 0) - base = 0; + on = false; pos |= CURSOR_POS_SIGN << CURSOR_X_SHIFT; x = -x; @@ -10155,16 +10153,13 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, if (y < 0) { if (y + cursor_state->crtc_h <= 0) - base = 0; + on = false; pos |= CURSOR_POS_SIGN << CURSOR_Y_SHIFT; y = -y; } pos |= y << CURSOR_Y_SHIFT; - if (base == 0 && intel_crtc->cursor_base == 0) - return; - I915_WRITE(CURPOS(pipe), pos); /* ILK+ do this automagically */ @@ -10175,9 +10170,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, } if (IS_845G(dev) || IS_I865G(dev)) - i845_update_cursor(crtc, base); + i845_update_cursor(crtc, base, on); else - i9xx_update_cursor(crtc, base); + i9xx_update_cursor(crtc, base, on); } static bool cursor_size_ok(struct drm_device *dev, -- 2.4.10 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/i915: Drop the broken cursor base==0 special casing 2015-12-14 11:16 ` [PATCH 2/2] drm/i915: Drop the broken cursor base==0 special casing ville.syrjala @ 2015-12-14 11:33 ` Chris Wilson 0 siblings, 0 replies; 9+ messages in thread From: Chris Wilson @ 2015-12-14 11:33 UTC (permalink / raw) To: ville.syrjala; +Cc: Takashi Iwai, intel-gfx On Mon, Dec 14, 2015 at 01:16:48PM +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The cursor code tries to treat base==0 to mean disabled. That fails > when the cursor bo gets bound at ggtt offset 0, and the user is left > looking at an invisible cursor. > > We lose the disabled->disabled optimization, but that seems like > something better handled at a slightly higher level. > > Cc: Takashi Iwai <tiwai@suse.de> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] drm/i915: Fix the invisible cursor syndrome 2015-12-14 11:16 [PATCH 0/2] drm/i915: Fix the invisible cursor syndrome ville.syrjala 2015-12-14 11:16 ` [PATCH 1/2] drm/i915: Kill intel_crtc->cursor_bo ville.syrjala 2015-12-14 11:16 ` [PATCH 2/2] drm/i915: Drop the broken cursor base==0 special casing ville.syrjala @ 2015-12-14 20:23 ` Ville Syrjälä 2 siblings, 0 replies; 9+ messages in thread From: Ville Syrjälä @ 2015-12-14 20:23 UTC (permalink / raw) To: intel-gfx; +Cc: Takashi Iwai On Mon, Dec 14, 2015 at 01:16:46PM +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > These two should provide the minimal fix for the invisible cursor > problem that's been plaguing me and some other people. It's caused > by the cursor bo getting bound at ggtt offset 0, which the code then > considers to mean "not enabled". These patches get rid of that assumption, > with the caveat that we may do some needless cursor register writes. > I have a more generic fix for that (and a ton of other cursor improvements) > at [1], but I figured that I'll just post the critical fixes now and allow > the others to stew a bit more in the branch until maybe early next year. > > I assume the problem is also related to the lack of fbdev pinning that > Chris has fixed [2]. With fbdev permanalty pinned at offset 0, there's > no opportunity for the cursor bo to travel there. Without fbdev I suppose > something else rings, status page, etc. could get permanently placed > at offset 0, so not sure if this can be hit in that case (didn't try it). > > I'm a bit wary about adding cc stable to these since I suspect that > backporting them too far will cause regressions, on account of the > atomic/plane stuff probably not being up to the task in older kernels. > So maybe better get these in w/o cc stable and consider a backport again > later if it looks necessary. > > [1] git://github.com/vsyrjala/linux.git cursor_improvements > [2] http://lists.freedesktop.org/archives/intel-gfx/2015-December/082114.html > > Ville Syrjälä (2): > drm/i915: Kill intel_crtc->cursor_bo > drm/i915: Drop the broken cursor base==0 special casing Both patches pushed to dinq with cc: fixes. Thanks for the reviews. > > drivers/gpu/drm/i915/intel_display.c | 34 ++++++++++++---------------------- > drivers/gpu/drm/i915/intel_drv.h | 1 - > 2 files changed, 12 insertions(+), 23 deletions(-) > > Cc: Takashi Iwai <tiwai@suse.de> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > -- > 2.4.10 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-12-14 20:23 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-14 11:16 [PATCH 0/2] drm/i915: Fix the invisible cursor syndrome ville.syrjala 2015-12-14 11:16 ` [PATCH 1/2] drm/i915: Kill intel_crtc->cursor_bo ville.syrjala 2015-12-14 11:32 ` Chris Wilson 2015-12-14 15:30 ` Ville Syrjälä 2015-12-14 15:35 ` [PATCH v2 " ville.syrjala 2015-12-14 15:37 ` Chris Wilson 2015-12-14 11:16 ` [PATCH 2/2] drm/i915: Drop the broken cursor base==0 special casing ville.syrjala 2015-12-14 11:33 ` Chris Wilson 2015-12-14 20:23 ` [PATCH 0/2] drm/i915: Fix the invisible cursor syndrome Ville Syrjälä
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.