| Message ID | 1424990924-27220-1-git-send-email-joe.konno@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 26, 2015 at 2:48 PM, Joe Konno <joe.konno@linux.intel.com> wrote: > > From: Joe Konno <joe.konno@intel.com> > > In instances where cursor sizes change, as in Chromium Ozone/Freon, > watermarks should be recomputed. There should be no hard-coded > assumptions about cursor widths. This was corrected originally here: > > commit 64f962e3e38bf6f40bbd2462f8380dee0369e1bf > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Wed Mar 26 12:38:15 2014 +0000 > > drm/i915: Recompute WM when the cursor size changes > > However, it seems the recompute logic got lost in refactoring. > Re-introduce the relevant WM re-compute code from the original patch. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Matt Roper <matthew.d.roper@intel.com> > Fixes: 32b7eeec4d1e ("drm/i915: Refactor work that can sleep out of commit (v7)") > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89346 > Signed-off-by: Joe Konno <joe.konno@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index b0f113d9daab..c5cbfea0551e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12234,6 +12234,7 @@ intel_commit_cursor_plane(struct drm_plane *plane, > struct intel_plane *intel_plane = to_intel_plane(plane); > struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb); > uint32_t addr; > + unsigned old_width; > > crtc = crtc ? crtc : plane->crtc; > intel_crtc = to_intel_crtc(crtc); > @@ -12243,6 +12244,7 @@ intel_commit_cursor_plane(struct drm_plane *plane, > crtc->cursor_y = state->base.crtc_y; > > intel_plane->obj = obj; > + old_width = intel_crtc->cursor_width; > > if (intel_crtc->cursor_bo == obj) > goto update; > @@ -12260,8 +12262,11 @@ update: > intel_crtc->cursor_width = state->base.crtc_w; > intel_crtc->cursor_height = state->base.crtc_h; > > - if (intel_crtc->active) > + if (intel_crtc->active) { > + if (old_width != intel_crtc->cursor_width) > + intel_update_watermarks(crtc); > intel_crtc_update_cursor(crtc, state->visible); > + } > } > > static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, > -- > 2.3.0 > Tested-by: James Ausmus <james.ausmus@intel.com> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- James Ausmus Sr. Software Engineer SSG-OTC ChromeOS Integration
On Thu, Feb 26, 2015 at 02:48:44PM -0800, Joe Konno wrote: > From: Joe Konno <joe.konno@intel.com> > > In instances where cursor sizes change, as in Chromium Ozone/Freon, > watermarks should be recomputed. There should be no hard-coded > assumptions about cursor widths. This was corrected originally here: > > commit 64f962e3e38bf6f40bbd2462f8380dee0369e1bf > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Wed Mar 26 12:38:15 2014 +0000 > > drm/i915: Recompute WM when the cursor size changes > > However, it seems the recompute logic got lost in refactoring. > Re-introduce the relevant WM re-compute code from the original patch. I don't believe the recompute logic got lost, it just got moved around a bit so that it wouldn't happen during our time-sensitive vblank evasion while interrupts are disabled. The old_width != intel_crtc->cursor_width check that you're re-adding below already happens in intel_check_cursor_plane(): finish: if (intel_crtc->active) { if (intel_crtc->cursor_width != state->base.crtc_w) intel_crtc->atomic.update_wm = true; We just set a flag so that we know whether we'll need to update watermarks later or not. Then when we get to the commit phase, in intel_begin_crtc_commit(), we do the following before entering vblank evasion: if (intel_crtc->atomic.update_wm) intel_update_watermarks(crtc); Some of the watermark code we have today sleeps, so we can't call it under vblank evasion (i.e., during the commit phase while interrupts are disabled). Our high-level atomic workflow (which legacy plane ioctls also flow through internally) looks something like this: - Some userspace ioctl (SetCursor, SetPlane, etc.) - intel_check_TYPE_plane() - check validity of request, bail if invalid - set flags in intel_crtc->atomic for various stuff we will need to do before/after vblank evasion (e.g., wm's) - intel_begin_crtc_commit() - Perform sleepable operations noted in intel_crtc->atomic - intel_pipe_update_start (interrupts are now disabled!) - intel_commit_TYPE_plane() - intel_finish_crtc_commit() - intel_pipe_update_end (interrupts are now reenabled) - Perform sleepable operations noted in intel_crtc->atomic So your patch below could result in sleeps happening while vblanks are disabled, which is bad (IIRC, most of those sleeps are in the SKL codepath right now, but I think there's a workaround-related wait for IVB as well). Our watermark code needs a lot of work to beat it into proper shape for atomic and that's what I'm working on at the moment. Matt > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Matt Roper <matthew.d.roper@intel.com> > Fixes: 32b7eeec4d1e ("drm/i915: Refactor work that can sleep out of commit (v7)") > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89346 > Signed-off-by: Joe Konno <joe.konno@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index b0f113d9daab..c5cbfea0551e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12234,6 +12234,7 @@ intel_commit_cursor_plane(struct drm_plane *plane, > struct intel_plane *intel_plane = to_intel_plane(plane); > struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb); > uint32_t addr; > + unsigned old_width; > > crtc = crtc ? crtc : plane->crtc; > intel_crtc = to_intel_crtc(crtc); > @@ -12243,6 +12244,7 @@ intel_commit_cursor_plane(struct drm_plane *plane, > crtc->cursor_y = state->base.crtc_y; > > intel_plane->obj = obj; > + old_width = intel_crtc->cursor_width; > > if (intel_crtc->cursor_bo == obj) > goto update; > @@ -12260,8 +12262,11 @@ update: > intel_crtc->cursor_width = state->base.crtc_w; > intel_crtc->cursor_height = state->base.crtc_h; > > - if (intel_crtc->active) > + if (intel_crtc->active) { > + if (old_width != intel_crtc->cursor_width) > + intel_update_watermarks(crtc); > intel_crtc_update_cursor(crtc, state->visible); > + } > } > > static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, > -- > 2.3.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Feb 26, 2015 at 05:47:35PM -0800, Matt Roper wrote: > On Thu, Feb 26, 2015 at 02:48:44PM -0800, Joe Konno wrote: > > From: Joe Konno <joe.konno@intel.com> > > > > In instances where cursor sizes change, as in Chromium Ozone/Freon, > > watermarks should be recomputed. There should be no hard-coded > > assumptions about cursor widths. This was corrected originally here: > > > > commit 64f962e3e38bf6f40bbd2462f8380dee0369e1bf > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > Date: Wed Mar 26 12:38:15 2014 +0000 > > > > drm/i915: Recompute WM when the cursor size changes > > > > However, it seems the recompute logic got lost in refactoring. > > Re-introduce the relevant WM re-compute code from the original patch. > > I don't believe the recompute logic got lost, it just got moved around a > bit so that it wouldn't happen during our time-sensitive vblank evasion > while interrupts are disabled. The old_width != intel_crtc->cursor_width > check that you're re-adding below already happens in > intel_check_cursor_plane(): > > finish: > if (intel_crtc->active) { > if (intel_crtc->cursor_width != state->base.crtc_w) > intel_crtc->atomic.update_wm = true; > > We just set a flag so that we know whether we'll need to update > watermarks later or not. Then when we get to the commit phase, in > intel_begin_crtc_commit(), we do the following before entering vblank > evasion: > > if (intel_crtc->atomic.update_wm) > intel_update_watermarks(crtc); > > Some of the watermark code we have today sleeps, so we can't call it > under vblank evasion (i.e., during the commit phase while interrupts are > disabled). > > Our high-level atomic workflow (which legacy plane ioctls also flow > through internally) looks something like this: > > - Some userspace ioctl (SetCursor, SetPlane, etc.) > - intel_check_TYPE_plane() > - check validity of request, bail if invalid > - set flags in intel_crtc->atomic for various stuff we will > need to do before/after vblank evasion (e.g., wm's) > - intel_begin_crtc_commit() > - Perform sleepable operations noted in intel_crtc->atomic > - intel_pipe_update_start (interrupts are now disabled!) > - intel_commit_TYPE_plane() > - intel_finish_crtc_commit() > - intel_pipe_update_end (interrupts are now reenabled) > - Perform sleepable operations noted in intel_crtc->atomic > > So your patch below could result in sleeps happening while vblanks are > disabled, which is bad (IIRC, most of those sleeps are in the SKL > codepath right now, but I think there's a workaround-related wait for > IVB as well). > > Our watermark code needs a lot of work to beat it into proper shape for > atomic and that's what I'm working on at the moment. Alternative issue is that wm recompute happens before we update all the legacy state. Tvrkto just stumbled over that for some of his skl watermark code, he had to switch a few places in the wm code from looking at plane->fb to look at plane->state->fb. Similar changes might be needed for the cursor wm code - cursor_width/height is kinda redundant since universal planes support. -Daniel
On 02/27/2015 06:53 AM, Daniel Vetter wrote: > On Thu, Feb 26, 2015 at 05:47:35PM -0800, Matt Roper wrote: >> So your patch below could result in sleeps happening while vblanks are >> disabled, which is bad (IIRC, most of those sleeps are in the SKL >> codepath right now, but I think there's a workaround-related wait for >> IVB as well). >> >> Our watermark code needs a lot of work to beat it into proper shape for >> atomic and that's what I'm working on at the moment. > > Alternative issue is that wm recompute happens before we update all the > legacy state. Tvrkto just stumbled over that for some of his skl watermark > code, he had to switch a few places in the wm code from looking at > plane->fb to look at plane->state->fb. > > Similar changes might be needed for the cursor wm code - > cursor_width/height is kinda redundant since universal planes support. Thanks for the insights, Matt and Dan. It's clear to me from Matt's analysis that my patch is a hack at best and nowhere near a proper fix. This patch was submitted in the hopes of fixing a substantial graphical regression. Shall my patch be retooled as a "better" work-around, or will the regression stand until the watermark code is beat into proper shape?
On Fri, Feb 27, 2015 at 08:21:34AM -0800, Joe Konno wrote: > On 02/27/2015 06:53 AM, Daniel Vetter wrote: > > On Thu, Feb 26, 2015 at 05:47:35PM -0800, Matt Roper wrote: > >> So your patch below could result in sleeps happening while vblanks are > >> disabled, which is bad (IIRC, most of those sleeps are in the SKL > >> codepath right now, but I think there's a workaround-related wait for > >> IVB as well). > >> > >> Our watermark code needs a lot of work to beat it into proper shape for > >> atomic and that's what I'm working on at the moment. > > > > Alternative issue is that wm recompute happens before we update all the > > legacy state. Tvrkto just stumbled over that for some of his skl watermark > > code, he had to switch a few places in the wm code from looking at > > plane->fb to look at plane->state->fb. > > > > Similar changes might be needed for the cursor wm code - > > cursor_width/height is kinda redundant since universal planes support. > > Thanks for the insights, Matt and Dan. It's clear to me from Matt's analysis > that my patch is a hack at best and nowhere near a proper fix. > > This patch was submitted in the hopes of fixing a substantial graphical > regression. Shall my patch be retooled as a "better" work-around, or will the > regression stand until the watermark code is beat into proper shape? I have a couple patches already written that I think might solve your problem. I'll extract those from my in-progress tree and post them shortly for you to try. Matt
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b0f113d9daab..c5cbfea0551e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12234,6 +12234,7 @@ intel_commit_cursor_plane(struct drm_plane *plane, struct intel_plane *intel_plane = to_intel_plane(plane); struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb); uint32_t addr; + unsigned old_width; crtc = crtc ? crtc : plane->crtc; intel_crtc = to_intel_crtc(crtc); @@ -12243,6 +12244,7 @@ intel_commit_cursor_plane(struct drm_plane *plane, crtc->cursor_y = state->base.crtc_y; intel_plane->obj = obj; + old_width = intel_crtc->cursor_width; if (intel_crtc->cursor_bo == obj) goto update; @@ -12260,8 +12262,11 @@ update: intel_crtc->cursor_width = state->base.crtc_w; intel_crtc->cursor_height = state->base.crtc_h; - if (intel_crtc->active) + if (intel_crtc->active) { + if (old_width != intel_crtc->cursor_width) + intel_update_watermarks(crtc); intel_crtc_update_cursor(crtc, state->visible); + } } static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,